fix(backend): pool Redis per event loop, so the slot cap survives a Celery task (#573) #582
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/573-slot-event-loop"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #573.
Cause. Three module-global
redis.asyncio.ConnectionPools (provider_slots,bot_pubsub,auth/session). Aredis.asyncioconnection holds its loop's reader and writer; a Celery task'sasyncio.run()closes that loop; the next task's first command finds a dead connection. Reproduced against a real Redis before changing anything: thirtyasyncio.runcycles in one process degraded every other one with the exact production log line. Thebot_pubsubpool had the same bug, which means the "were bot-event stream entries trimmed unread?" check had been silently reporting no data rather than no loss whenever it ran as a worker's second-or-later task.Fix. New
app/redis_pool.py: pools cached per running loop object (not its id, which is reused), closed loops pruned on each call, no caching without a running loop. All three sites route through it; callers still receive a per-callRediswrapper so behaviour within a loop is unchanged and the existing test patches keep working. Measured the trade-off of a discarded pool per task over 1,000 cycles: descriptors oscillate with the cyclic collector and never trend up.Test. Three real
asyncio.runcalls in one process, asserting neither lease is degraded, both are in Redis, a third is refused by the cap, and three pools were built. Reverting to a process-global cache makes it fail with the production message.Full backend suite 3,025 passed; formatted with CI's ruff.
🤖 Generated with Claude Code
Every LLM call in the post-session fan-out logged Could not reach Redis to take a llm slot on llamacpp/10.3.0.28:8090 (RuntimeError: Event loop is closed); running without a concurrency cap for this call. and then ran uncapped. Redis was reachable throughout; the connection was not. A `redis.asyncio` connection belongs to the event loop it was opened on, the pools behind it were module globals built once per process, and a Celery worker runs every task inside its own `asyncio.run(...)` — so from the second task onwards the worker was issuing commands against a loop that had been closed. `app.database.task_session` already documents this exact hazard for SQLAlchemy; Redis had no equivalent. The cap failing open is what made it quiet: nothing errored, no session failed, and a self-hosted llama.cpp simply took more concurrent requests than it declares and slowed all of them down together — the failure #356 exists to prevent, with #356 apparently switched on. - `app/redis_pool.py` caches a pool per running event loop and drops any whose loop has closed. Callers still get a per-call client, so nothing else changes: within one loop connections are reused exactly as before. - `provider_slots._redis`, `bot_pubsub`'s four async callers and `auth.session._redis` all go through it. bot_pubsub is the same live bug on a quieter path — `check_bot_event_stream` and `warn_before_audio_deletion` are beat tasks, and `detect_unread_trim` reads any failure as "no stream yet", i.e. reports *no data loss*. auth.session has only request-path callers today and is converted so the first task-path caller does not rediscover this. `bot_pubsub._sync_pool` stays a module global: a synchronous connection has no loop to be bound to. - Nothing closes a discarded pool, because nothing can — its loop is gone. Dropping the reference suffices: measured over 1000 `asyncio.run` cycles against a real Redis, open descriptors oscillate between 5 and ~35 as the cyclic collector runs and return to 5 on a forced collection, with 1000/1000 acquires capped and none degraded. Before the change, 50% degraded. - The test is synchronous and calls `asyncio.run` three times, which is the worker's shape; the bug lives entirely at the boundary between loops, so a test inside one loop cannot see it. `tests/fake_redis.LoopBoundPool` gives the fake the one property of a real pool that matters here. Reverting `pool()` to a process-global cache fails it with the production warning verbatim. Backend: 3025 passed, 13 skipped. ruff check and ruff format (0.4.4) clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>