fix(webapp): Celery publishes no longer freeze a backend worker when Redis stalls (#841) #850

Merged

claude-bot merged 3 commits from fix/841-blocking-loop into main 2026-09-26 17:57:15 +00:00

Conversation 0 Commits 3 Files changed 17 +561 -63

claude-bot commented 2026-09-26 16:49:00 +00:00

Collaborator

Copy link

Refs #841. The issue stays open: this fixes the real blocking it suspected, but the evidence shows that blocking isn't what killed the workers (see the comment on #841).

What was wrong

Every task.delay() / apply_async() / control.revoke() reachable from an async def ran on the event loop. That was 47 sites across 12 modules, including notify() once per recipient, through a sync helper.

A .delay() also subscribes the enqueuing process to the result backend, which had no socket timeout and a 20 × 1 s retry policy. Measured against a real Redis:

Redis	One .delay() held the loop for
healthy	~3 ms
stopped	19 s
paused	indefinitely; stuck after 5 minutes

While it waited, that worker served nothing and couldn't shut down cleanly. Under uvicorn --workers 2 with Redis paused, the worker handling a notify served 0 of its keep-alive requests for 26 s and needed a SIGKILL.

The fix

• app/enqueue.py: await enqueue(task, …) and await off_loop(fn, …) run publishes on one dedicated thread per process.
  • Callers still await, so commit-then-enqueue ordering, returned task ids and every except around a refused enqueue are unchanged.
  • Context vars (request id, user id) are copied into the thread.
  • One thread rather than a pool: Celery's result backend is per-thread, and a stall shouldn't eat FastAPI's shared threadpool.
  • The thread is re-created after a fork.
• All 47 sites converted. Four helpers that read ORM attributes became async so those reads stay on the loop.
• celery_app: the result backend gets redis_socket_connect_timeout=2, redis_socket_timeout=2 and a 2-retry policy, matching the broker.
  • A paused Redis now costs a publish ~8 s, spent off the loop; a stopped one costs under 1 s.
  • Trade-off: a worker's result write gives up after 2 retries. Nothing in the app reads task results.
• The same repro after the fix: the other requests on the notifying worker were all served at ~0 s latency while its notify waited 8.4 s and then returned 200. Shutdown was clean.

Guard: tests/test_enqueue_off_loop.py

• A static AST check fails the build if a publish can run in an async def outside app/tasks/.
  • It catches direct calls, and calls through a sync helper found by a transitive closure (that's how notify() publishes).
  • It has a self-test on a synthetic tree.
• A deterministic behavioural test: a confirm's publish is held on an event while /health on the same loop must still answer. There are no wall-clock bounds.
• Both fail on origin/main, with the 47 sites listed.

Tests

• Targeted backend run (83 files touching publish, notification, session, lore, draft, vote, summary and import code): 1,738 passed.
• The new and updated helper tests: 14 passed.
• ruff: clean.

Heads-up: lane 7 (#846) and lane 6 will conflict in notification_service.py and session_service.py. After they merge, the guard catches any new direct publish.

🤖 Generated with Claude Code

Refs #841. The issue stays open: this fixes the real blocking it suspected, but the evidence shows that blocking isn't what killed the workers (see the comment on #841). ## What was wrong Every `task.delay()` / `apply_async()` / `control.revoke()` reachable from an `async def` ran on the event loop. That was 47 sites across 12 modules, including `notify()` once per recipient, through a sync helper. A `.delay()` also subscribes the enqueuing process to the result backend, which had **no socket timeout** and a 20 × 1 s retry policy. Measured against a real Redis: | Redis | One `.delay()` held the loop for | |---|---| | healthy | ~3 ms | | stopped | 19 s | | paused | indefinitely; stuck after 5 minutes | While it waited, that worker served nothing and couldn't shut down cleanly. Under uvicorn `--workers 2` with Redis paused, the worker handling a notify served 0 of its keep-alive requests for 26 s and needed a SIGKILL. ## The fix - **`app/enqueue.py`:** `await enqueue(task, …)` and `await off_loop(fn, …)` run publishes on one dedicated thread per process. - Callers still await, so commit-then-enqueue ordering, returned task ids and every `except` around a refused enqueue are unchanged. - Context vars (request id, user id) are copied into the thread. - One thread rather than a pool: Celery's result backend is per-thread, and a stall shouldn't eat FastAPI's shared threadpool. - The thread is re-created after a fork. - **All 47 sites converted.** Four helpers that read ORM attributes became `async` so those reads stay on the loop. - **`celery_app`:** the result backend gets `redis_socket_connect_timeout=2`, `redis_socket_timeout=2` and a 2-retry policy, matching the broker. - A paused Redis now costs a publish ~8 s, spent off the loop; a stopped one costs under 1 s. - Trade-off: a worker's result write gives up after 2 retries. Nothing in the app reads task results. - **The same repro after the fix:** the other requests on the notifying worker were all served at ~0 s latency while its notify waited 8.4 s and then returned 200. Shutdown was clean. ## Guard: `tests/test_enqueue_off_loop.py` - **A static AST check** fails the build if a publish can run in an `async def` outside `app/tasks/`. - It catches direct calls, and calls through a sync helper found by a transitive closure (that's how `notify()` publishes). - It has a self-test on a synthetic tree. - **A deterministic behavioural test:** a confirm's publish is held on an event while `/health` on the same loop must still answer. There are no wall-clock bounds. - **Both fail on `origin/main`,** with the 47 sites listed. ## Tests - Targeted backend run (83 files touching publish, notification, session, lore, draft, vote, summary and import code): **1,738 passed**. - The new and updated helper tests: 14 passed. - ruff: clean. **Heads-up:** lane 7 (#846) and lane 6 will conflict in `notification_service.py` and `session_service.py`. After they merge, the guard catches any new direct publish. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 2 commits 2026-09-26 16:49:01 +00:00

fix(backend): bound the Celery result backend like the broker (#841) ... 68d3bff51f

Every .delay() subscribes the enqueueing process to the task's result
channel before it publishes, so the web process holds a pub/sub
connection to the result backend (DB 2). The broker's socket timeouts
and publish retry policy do not reach it. With Celery's defaults a
paused Redis (TCP accepted, nothing answering) hung a .delay() for more
than five minutes in on_connect, and a refused connection retried
twenty times a second apart: 19 s per .delay(), measured.

2 s connect/socket timeouts and two retries bring that to about 8 s
(paused) and under 1 s (refused). Nothing reads task results, so the
worker's shorter result-write retry costs only a logged error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

fix(backend): make every Celery publish off the event loop (#841) ... 643e5ce433

task.delay() is blocking network I/O, and 47 call sites in 12 modules
made it inside async code: every in-app notification (once per
recipient), session confirm/cancel/complete/reopen, vote updates, lore
summaries, drafts, workbench prefetches, audio retries. While a publish
waited, nothing else in that uvicorn worker ran; on a stalled Redis the
worker stopped answering entirely and could not even shut down.

app/enqueue.py runs publishes on one dedicated thread per process:
`await enqueue(task, ...)` for .delay(), `await off_loop(fn, ...)` for
apply_async, control.revoke and sync helpers that publish. The caller
still awaits the publish, so commit-then-enqueue ordering, returned task
ids and every except around a refused enqueue behave as before; the task
method is looked up on the loop so tests that patch .delay still see it.
One thread, not a pool: Celery keeps its result backend per thread, and
a stall should not eat FastAPI's shared threadpool. The executor is
re-created after fork (Celery prefork runs the same services) and the
context is copied so logs keep their request id.

Sync helpers that read ORM state (enqueue_lore_entry_summary and the
vote-close scheduling in session_service) became async so attributes are
still read on the loop; pure-value helpers stay sync behind off_loop.

tests/test_enqueue_off_loop.py: a static guard that fails if a publish
is reachable from an async def outside app/tasks (directly, or through a
sync helper by name), and a deterministic behavioural test that /health
is answered while a session confirm's notification publish is stalled.
Both fail on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude-bot referenced this pull request 2026-09-26 16:49:08 +00:00

A backend worker died under load after a notifying POST (blocking calls in async handlers?) #841

rbrooks added 1 commit 2026-09-26 17:17:39 +00:00

Merge remote-tracking branch 'origin/main' into fix/841-blocking-loop ... 610100c067

# Conflicts:
#	CHANGELOG.md
#	webapp/backend/app/routers/admin.py
#	webapp/backend/app/routers/users.py

claude-bot merged commit c0197ec0ca into main 2026-09-26 17:57:15 +00:00

claude-bot referenced this pull request from a commit 2026-09-26 17:57:17 +00:00

Merge PR #850: Celery publishes no longer freeze a backend worker when Redis stalls (#841)

rbrooks referenced this pull request from a commit 2026-09-26 19:46:55 +00:00

Merge origin/main into lane 6 (#732); re-chain b8c0d2e4f6a7 onto c4e6a8b0d2f1

claude-bot referenced this pull request 2026-09-26 19:47:09 +00:00

feat: lane 6, prep and planning: the prep sheet, the tool margin, History and Planning, J3 and the seed (#732) #858

claude-bot referenced this pull request from a commit 2026-09-28 05:19:35 +00:00

feat(webapp): the map's server — atlases, maps, markers, links, trail stops (#734, lane 8 PR 6)

claude-bot referenced this pull request 2026-10-01 19:23:24 +00:00

Redis calls time out instead of hanging a request; notify stops at a dead broker (#841) #1002

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

backend

Issue related to the backend

  

bot

Issue related to the bot

  

bug

Something is not working

  

duplicate

This issue or pull request already exists

  

enhancement

New feature

  

frontend

Issue related to the frontend

  

help wanted

Need some help

  

invalid

Something is wrong

  

question

More information is needed

  

recording

Issue related to the recording system

  

security

Security hardening / vulnerability work

  

wontfix

This won't be fixed

No labels

backend

bot

bug

duplicate

enhancement

frontend

help wanted

invalid

question

recording

security

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rbrooks/Quest-Board!850

No description provided.