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

Merged

claude-bot merged 3 commits from fix/841-redis-timeouts into main 2026-10-01 20:16:57 +00:00

Conversation 0 Commits 3 Files changed 11 +411 -51

claude-bot commented 2026-10-01 19:23:24 +00:00

Collaborator

Copy link

Two of #841's follow-ups. The blocking-loop part was fixed in #850 and is not touched here. Refs #841 (not closing it: see the end).

1. Redis clients have timeouts

A paused or unreachable Redis used to leave the request that touched it waiting for ever. It didn't block the event loop, but it never answered.

• app/redis_pool.py now holds the timeouts in one place: 2 s to connect, 10 s for a reply. The shared pool uses them, and a new adhoc_client() replaces every hand-built aioredis.from_url(...) (/ready, the names cache, the relationship-proposal helpers, the workbench pool, the planning tasks). The sync pool in bot_pubsub uses them too.
• Why 10 s for a reply and not 2 s. On an async client the wait is measured on the event loop, so a loop that stalls for a few seconds under load (#841 measured such stalls) counts against it. Session lookups don't catch Redis errors, so a short timeout would turn a stall into an error on every signed-in request. Ten seconds still ends a request against a hung Redis.
• Blocking reads keep no read timeout. The one in the backend is the live recording-status stream (get_message(timeout=30)), which now uses blocking_client(). The backend has no consumer of the bot-notify stream; it only adds to it.
• A timeout takes the same path a Redis outage already does at each site: /ready answers 503 naming Redis (and now closes its client when the ping fails), the cache helpers fall through, and the sites that raised still raise.
• A guard test fails if any module under app/ builds a Redis client from the URL without the timeouts.

2. notify() stops at a dead broker

With the broker unreachable, each recipient's publish cost its own timeout, one after another. Now the first connectivity failure in a notify() call skips the rest of that batch and logs once how many were skipped.

• The in-app rows are written before any publish, so everyone still gets their notification in the app.
• A failure that isn't connectivity (one bad payload) doesn't stop the others.
• Skipped recipients are not retried, which is how the one that failed was already treated.

Not in this PR

The backend's mem_limit and uvicorn's worker health-check timeout, which are the owner's call. The worker kills #841 was opened for have not been seen since the CI runner's capacity was raised; #841 stays open for that decision.

Checked

• Full backend suite: 6594 passed, 14 skipped. After merging main: tests/test_redis_timeouts.py, tests/export, tests/wiki, 616 passed.
• ruff 0.4.4 format and check clean; changelog check OK.
• No migration, no bot API change.

🤖 Generated with Claude Code

Two of #841's follow-ups. The blocking-loop part was fixed in #850 and is not touched here. Refs #841 (not closing it: see the end). ## 1. Redis clients have timeouts A paused or unreachable Redis used to leave the request that touched it waiting for ever. It didn't block the event loop, but it never answered. - `app/redis_pool.py` now holds the timeouts in one place: **2 s to connect, 10 s for a reply**. The shared pool uses them, and a new `adhoc_client()` replaces every hand-built `aioredis.from_url(...)` (`/ready`, the names cache, the relationship-proposal helpers, the workbench pool, the planning tasks). The sync pool in `bot_pubsub` uses them too. - **Why 10 s for a reply and not 2 s.** On an async client the wait is measured on the event loop, so a loop that stalls for a few seconds under load (#841 measured such stalls) counts against it. Session lookups don't catch Redis errors, so a short timeout would turn a stall into an error on every signed-in request. Ten seconds still ends a request against a hung Redis. - **Blocking reads keep no read timeout.** The one in the backend is the live recording-status stream (`get_message(timeout=30)`), which now uses `blocking_client()`. The backend has no consumer of the bot-notify stream; it only adds to it. - A timeout takes the same path a Redis outage already does at each site: `/ready` answers 503 naming Redis (and now closes its client when the ping fails), the cache helpers fall through, and the sites that raised still raise. - A guard test fails if any module under `app/` builds a Redis client from the URL without the timeouts. ## 2. `notify()` stops at a dead broker With the broker unreachable, each recipient's publish cost its own timeout, one after another. Now the first connectivity failure in a `notify()` call skips the rest of that batch and logs once how many were skipped. - The in-app rows are written before any publish, so everyone still gets their notification in the app. - A failure that isn't connectivity (one bad payload) doesn't stop the others. - Skipped recipients are not retried, which is how the one that failed was already treated. ## Not in this PR The backend's `mem_limit` and uvicorn's worker health-check timeout, which are the owner's call. The worker kills #841 was opened for have not been seen since the CI runner's capacity was raised; #841 stays open for that decision. ## Checked - Full backend suite: 6594 passed, 14 skipped. After merging main: `tests/test_redis_timeouts.py`, `tests/export`, `tests/wiki`, 616 passed. - ruff 0.4.4 format and check clean; changelog check OK. - No migration, no bot API change. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 3 commits 2026-10-01 19:23:25 +00:00

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

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Redis replies get ten seconds, not two: an event-loop stall must not fail a session lookup (#841) ... 86559cf581

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Merge remote-tracking branch 'origin/main' into fix/841-redis-timeouts c4d82aec11

claude-bot referenced this pull request 2026-10-01 20:16:35 +00:00

Dev deploy, 1 Oct: lane 11, the R3 fixes and today's issue fixes #1004

claude-bot merged commit c95fe032d5 into main 2026-10-01 20:16:57 +00:00

claude-bot referenced this pull request from a commit 2026-10-01 20:16:58 +00:00

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

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

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

claude-bot referenced this pull request 2026-10-01 23:39:05 +00:00

After /record/start returns 200, GET /sessions/{id} once still read "confirmed" (D116) #845

claude-bot referenced this pull request 2026-10-05 05:45:41 +00:00

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

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

1 participant

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!1002

No description provided.