Skip to content

Keep request callbacks on the request, scope the bus dispatcher, and add an opt-in Redis pool - #24

Merged
adhikjoshi merged 2 commits into
mainfrom
fix/request-local-rebinding-and-redis-pool
Oct 15, 2026
Merged

adhikjoshi merged 2 commits into
mainfrom
fix/request-local-rebinding-and-redis-pool

Conversation

@adhikjoshi

@adhikjoshi adhikjoshi commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Problems (found on modelslab-frontend-v2's production image under Swoole, with recycling off and a probe of base-container state)

  1. refresh()/rebinding() went to the base app. AuthManager calls refresh('request', $guard, 'setRequest') for every guard; the auth manager is request-scoped, so each request left one rebound callback (holding the guard → request → session → user) on the worker for good — and the session guard was handed the base app's boot request, not the current one, so anything it reads from "its" request (remember-me cookie, HTTP Basic credentials) read an empty request. api/sanctum guards (built with viaRequest) were fine.
  2. terminating()/terminate() went to the base app. Every request's callbacks were kept, and base->terminate() ran all of them at the end of every later request. The Blade engine registers one each time it is resolved, and Octane forgets the blade/php engines after every request, so a worker kept and ran one more callback per request it had served — throughput fell as it aged, and any request callback re-ran in other requests.
  3. After-response jobs re-ran on every later request. The bus dispatcher is a worker singleton whose container is the base app, so dispatch(...)->afterResponse() (and Bus::batch) registered on the base terminating list (problem 2). Measured on the app: 1 job dispatched, then 40 other requests → the job ran 41 times.
  4. One new Redis connection per name per request (connect + AUTH + SELECT, closed at the end). Under load the closed sockets fill TIME_WAIT until connects fail with Cannot assign requested address (~370 req/s per container locally), and every request pays the round trips.

Fix

  • CoroutineApplication::rebinding()/refresh(): inside a request, for an abstract the request scope holds (in practice request), the callback is stored on the RequestScope and the scoped instance is returned; rebound() runs the request's callbacks. Outside a request: unchanged.
  • CoroutineApplication::terminating()/terminate(): inside a request, callbacks go to the RequestScope; terminate() runs the boot-time (base) callbacks, then the request's. Outside a request: unchanged.
  • The bus dispatcher is request-scoped (a clone holding the request's container); a Bus::fake() stays shared.
  • Opt-in per-worker phpredis pool (octane.swoole.redis_pool.enabled, default off): RedisConnectionPool, PooledPhpRedisConnection, PooledPhpRedisConnector, CoroutineRedisResolver. Each borrow gets a new lease around a pooled socket; the worker returns it after the request is torn down (detached before flush(), released in finally, like the DB pool) — only if provably clean: connected, not in MULTI/pipeline, same db and phpredis options as at creation, and no unfinished or state-changing command, raw ones included (SUBSCRIBE, WATCH, SELECT, MULTI via executeRaw, CLIENT, AUTH, HELLO, …). The returned lease loses the socket: a reference kept past the request throws LogicException instead of sharing it. Never blocks; max_idle, max_idle_time (pruned from the bottom of the stack too), ping_after_idle, max_lifetime; pid guard; configs that cannot be serialized are not pooled. Non-coroutine paths and child coroutines (go()) unchanged.
  • CI: Redis service + phpredis, so the Redis tests run instead of skipping.

Verification

  • Unit: CoroutineRebindingTest (5), CoroutineTerminatingTest (6, incl. after-response job runs once), RedisConnectionPoolTest (17, real Redis). Reverting each fix fails its tests; each guard is mutation-tested (rebinding, terminating, dispatcher scoping, clean-state snapshot, stateful/raw-command flag, lease revoke, idle cap, bottom-of-stack expiry).
  • Full suite (Swoole 6.3, PHP 8.3, with Redis): 225 tests green.
  • An adversarial review found: executeRaw bypassing the stateful check, sockets returned before teardown, LIFO idle never expiring, and the dispatcher's after-response path — all fixed above with tests.
  • Real app (modelslab-frontend-v2 production image, Octane prod flags, 8 workers, nginx in front, throwaway MySQL/Redis/Mongo), recycling off, 3×40 s mixed load:
v0.8.35 this branch
base rebound callbacks / wildcard listeners / terminating callbacks +3,042 / +1,021 / — 0 / 0 / 0
worker heap (sampled) 29 → 61 MB 11.1 → 11.2 MB
throughput, rounds 2–3 (req/40 s) 5,210 → 5,578 (from 13,533) 13,317 → 13,284 (flat)
web guard holds current request no yes
new Redis connections per request 2.002 0.004
after-response job runs (1 dispatch + 40 requests) 41 1

ABBA wrk (prod flags, recycling at 500; together with the app's own changes — Sentry view tracing off, pool on): render +22%, heavy render +87%, cache HIT +15%, JSON +16%. Sessions persist through pooled connections (0 new session keys over 20 requests); Vite preload headers 0/160 wrong; 0 errors in the log.

Known limits

  • Callbacks registered from a child coroutine (go() inside a request) still reach the base app, and its Redis connections are not pooled (closed as before).
  • A base-container singleton that captures a Redis connection during a request will get LogicException once the request ends (before: it kept a socket that was closed).

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

CoroutineApplication forwarded rebinding(), refresh() and terminating()
to the base application. AuthManager calls refresh('request', $guard,
'setRequest') for every guard, so each request left a callback that
holds the guard, request, session and user on the worker for good, and
the session guard got the base application's boot request instead of
the current one (remember-me and basic auth read an empty request).
Every terminating callback was kept too, and base->terminate() ran all
of them at the end of every later request: the Blade engine registers
one each time Octane re-resolves it, so a worker slowed down as it
aged, and a request's callbacks re-ran in other requests.

Inside a request these callbacks now live on the RequestScope:
rebinding a request-scoped abstract registers on the scope and returns
the scoped instance, and terminate() runs the boot-time callbacks and
then this request's. The bus dispatcher is scoped as well, because
dispatch()->afterResponse() and batches register on the dispatcher's
own container, which was the base application: an after-response job
ran again at the end of every later request on the worker.
Every request coroutine opened its own phpredis connections (connect,
AUTH, SELECT) and closed them at the end, so each request paid those
round trips and closed sockets piled up in TIME_WAIT until connects
failed with "Cannot assign requested address".

With octane.swoole.redis_pool.enabled, a clean socket goes back to a
per-worker pool when the request is torn down and the next request
borrows it in a new lease. It is reused only if it is still connected,
outside MULTI or a pipeline, on the database and options it was created
with, and ran no unfinished or state-changing command (including raw
ones); otherwise it is closed. The returned lease loses the socket, so
a reference kept past the request throws instead of sharing it.
Borrowing never waits; idle sockets expire and are pinged after idle.
Off by default; non-coroutine paths are unchanged.

CI now runs a Redis service so the Redis tests run instead of skipping.
@adhikjoshi
adhikjoshi merged commit 62963a3 into main Oct 15, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant