fix: recover wedged turns when provider event stream stalls (#46) - #47
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds configurable session stall and MCP tool timeouts, stall detection and recovery in ChangesSession timeouts and recovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
#completeActiveTools() was called in the text_done event handler. In the real Claude Agent SDK the committed assistant message (which fires text_done) is emitted BEFORE the user/tool_result message (which fires tool_complete). This meant any tool still in #activeToolMsgIds at text_done time was immediately cancelled, then tool_complete arrived but found the correlation maps cleared — so the tool stayed permanently "cancelled — interrupted" in the UI. The prior T8c fix removed the call from tool_start but kept it in text_done, relying on the wrong assumption that the mock ordering (tool_complete before text_done) matched the real SDK. The mock tests passed while the real-world bug persisted. Fix: remove #completeActiveTools() from text_done entirely. The #consumeEvents finally block is the correct and sufficient cleanup point — it runs on every turn end (normal, interrupted, or error) and only touches tools whose tool_complete never arrived. Adds T8(d) regression guard: tool_start → text_done → tool_complete → turn_done must produce "completed", not "cancelled". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…turn When a now-priority message is pushed mid-turn, the SDK ends the current turn (emitting turn_done, often with isError) before starting the continuation turn for the injected message. Previously, #consumeEvents broke on that first turn_done and left the continuation turn unread — the session went idle/error and the user had to send "please continue" manually. The fix adds #pendingMidTurnCount (incremented in #sendInner before each pushMidTurn call, decremented in #consumeEvents when the intermediate turn_done is absorbed). When a turn_done arrives with a pending count, #consumeEvents flushes per-turn state (active tools, assistant stream, thinking, stale approvals), records partial cost, re-asserts "thinking" status, and continues the loop instead of breaking. The counter is always reset to 0 in the finally block and on explicit interrupt() so no orphaned count can block a future turn. A new T9 integration test drives a custom TurnRun with pushMidTurn support and verifies the session ends idle with the continuation message present and no "Error:" system message. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A session could get stuck in "replying" forever with no messages shown (both Telegram and web UI) when a provider's event stream went silent without a terminal turn_done — e.g. a Bash command or MCP gateway call that never returns. #consumeEvents blocked in `for await (run.events)` with no watchdog, so #activeRun/status never reset; every subsequent send was then swallowed into the dead run via the wasWorking mid-turn push path, and the user never got a reply. This is a latent bug present on main as well (not introduced by the multi-provider work) — but high severity, since any single hung tool bricks the session until an explicit interrupt. Fix: - Stall watchdog in #consumeEvents: drive the iterator manually and race each pull against session.turnStallTimeoutMs (default 300s, 0 = off). On silence, force-recover via #recoverStalledRun. - #recoverStalledRun: flush streaming UI, release pending approvals, drop the run, emit a clear "Turn timed out" notice, reset to idle, and hard teardown the provider to reap the (presumed hung) subprocess. Does NOT route through #teardownProvider (which awaits the session consumer — i.e. potentially itself), avoiding self-await deadlock. - Liveness guard in #sendInner: a send onto an apparently-dead run recovers it and starts a fresh turn instead of silently queueing. - Config: session.turnStallTimeoutMs + CODEOID_TURN_STALL_TIMEOUT_MS. - MockSessionProvider: opt-in `stall` mode + close-on-teardown (mirrors ClaudeProvider) for deterministic offline testing. - Tests (T9): watchdog recovers + reaps; next send works (no permanent wedge); watchdog-off leaves the turn blocked (no false recovery). Verified: typecheck clean, biome clean, 617/617 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
saucam
force-pushed
the
fix/turn-stall-watchdog
branch
from
June 29, 2026 01:06
7304cc5 to
f792a91
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.ts`:
- Line 435: The CODEOID_TURN_STALL_TIMEOUT_MS override is currently applied
after RootSchema.safeParse(), so it can bypass the non-negative validation and
disable the stall watchdog in Session. Update the config flow in src/config.ts
so this override is validated with the same non-negative schema used for
session.turnStallTimeoutMs, or re-run config validation after the ENV_OVERRIDES
loop. Make sure the fix is applied in the config parsing path that handles
RootSchema and ENV_OVERRIDES so environment precedence still works without
allowing invalid values.
In `@src/daemon/session.ts`:
- Around line 1625-1637: The stall watchdog in the iterator loop is still racing
`iter.next()` against `stallMs`, which can fire during a valid manual approval
wait after `tool_start`. Update the `session.ts` logic around the
`stall`/`Promise.race` block so the watchdog is paused or bypassed while
awaiting user approval, and only resumes once the approval flow completes. Use
the existing iterator handling in this section to ensure `approve()` waits are
not treated as stalls and do not reset the run.
- Around line 1643-1646: Events from an abandoned run can still be processed
after `#recoverStalledRun`() starts a fresh turn, causing mixed state. In the
event loop inside `#sendInner`, add a run-ownership check before handling
next.value so any late event from the previous run is ignored. Use the existing
run/session identifiers around the iter.next() handling to verify the event
still belongs to the current run before updating `#lastEventAt` or dispatching the
event.
- Around line 1753-1756: The stalled-session message in `#recoverStalledRun`()
should not tell users to resend because when it is reached from `#sendInner` the
current message already continues into a fresh turn. Update the system text
built by this method to use neutral wording that fits both recovery paths, and
adjust the send-path invocation in `#sendInner` so it doesn’t surface a
duplicate-send prompt.
In `@src/tests/session-integration.test.ts`:
- Around line 181-200: The waitForStatus helper can miss a status transition if
session.status changes before the watcher is attached, causing a timeout even
though the target was reached. Update waitForStatus to close this race by
re-checking session.status immediately after session.attach(watcher) (or by
attaching first and then reading state), and resolve/clean up if the target
status is already set. Use the waitForStatus function and
session.attach/session.detach logic to place the fix.
- Around line 1063-1075: The test is checking broadcasts from a client that is
never attached to the session, so received stays empty and the timeout assertion
is meaningless. In session-integration.test.ts, wire the makeClient() result
into the session before sending the turn, using the existing session setup
around makeSession and send so received reflects actual session.message
broadcasts. Then keep the stalledMsg lookup against received to validate that
the disabled watchdog path does not emit a timeout notice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d2843f7-b0c1-4576-b6ee-30fc32415d1d
📒 Files selected for processing (4)
src/config.tssrc/daemon/providers/mock/session-provider.tssrc/daemon/session.tssrc/tests/session-integration.test.ts
Four findings from CodeRabbit on PR #47, all valid: - config: env overrides are applied after RootSchema.safeParse, so CODEOID_TURN_STALL_TIMEOUT_MS=-1 bypassed z.number().min(0) and silently disabled the watchdog (stallMs > 0 → false). Re-validate the merged config through RootSchema after the override loop — also guards the autoRotate percentage bounds and every other constrained override. - watchdog: a pending manual tool approval is a legitimate indefinite silent period (provider blocks on canUseTool). Pause the stall race while status is waiting_approval / #pendingApprovals is non-empty so a slow human approval isn't mistaken for a hung stream. - consumer: add a run-ownership check before processing an event, so a late event from a run abandoned by #sendInner's liveness-guard recovery can't leak into the fresh turn. - message: when #recoverStalledRun is invoked from #sendInner the saved message is retried in a fresh turn — say so, instead of telling the user to resend (which would duplicate). Tests: +2 config (override applies; negative rejected) and +1 integration (watchdog stays paused through a pending approval). 620/620 pass, biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
+ Coverage 80.62% 81.07% +0.44%
==========================================
Files 55 55
Lines 7418 7593 +175
==========================================
+ Hits 5981 6156 +175
Misses 1437 1437
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- waitForStatus: close the race where the status flips to the target between the initial check and session.attach() — re-check current status after attaching so the helper can't miss the only broadcast and time out. - Attach the test client in the watchdog-disabled and approval-pause tests. `received` was never populated (client created but not attached), so the "no timeout notice" assertions passed vacuously even if the disabled / paused watchdog path had wrongly emitted one. 620/620 pass, biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make the SDK signal a hung MCP call instead of relying solely on the session stall watchdog. The watchdog is a coarse, provider-agnostic last-resort; the precise fix for the case we actually hit (an unresponsive MCP gateway) is the SDK's own per-server tool-call timeout. - Apply session.mcpToolTimeoutMs (default 120000) to external/user MCP servers via each server's SDK `timeout`. A hung call now returns an SDK error event the turn loop acts on, rather than going silent. Explicit per-server timeouts are preserved; the in-process memory server is left untouched; 0 disables (use SDK default). - Default (120s) sits BELOW turnStallTimeoutMs (300s) so the SDK fires first and the watchdog stays a true backstop. - Config: session.mcpToolTimeoutMs + CODEOID_MCP_TOOL_TIMEOUT_MS. - Tests: withMcpToolTimeout (inject / don't-override / no-op) + config (default ordering + env override). 625/625 pass, biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/daemon/providers/claude/index.ts (1)
643-648: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftValidate the enriched server config with a Zod schema instead of double-casting.
This helper is still operating on runtime-loaded config, but
cfg as unknown as Record<string, unknown>and the final cast back toMcpServerConfigskip validation entirely. Please run the timeout-enriched object back through the MCP server schema (or extractparseMcpServerConfiginto a reusable Zod schema) before returning it. As per coding guidelines, "Use Zod for validation of runtime data and configuration".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daemon/providers/claude/index.ts` around lines 643 - 648, The server config enrichment logic in the loop over Object.entries(servers) is bypassing validation by double-casting through unknown and back to McpServerConfig. Update this path to validate the timeout-enriched object with the existing MCP server Zod schema instead of casting, ideally by reusing or extracting the schema behind parseMcpServerConfig. Keep the runtime behavior the same, but ensure the final value assigned to out[name] is the parsed/validated result rather than an unchecked object.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tests/config.test.ts`:
- Around line 161-173: The timeout-order contract is only covered for defaults,
not env overrides; update the config validation in src/config.ts so the session
schema enforces mcpToolTimeoutMs is always below turnStallTimeoutMs with a
cross-field Zod refinement. Add a regression test in src/tests/config.test.ts
using loadConfig for an invalid override pair that sets
CODEOID_MCP_TOOL_TIMEOUT_MS above turnStallTimeoutMs and asserts validation
fails, while keeping the existing default-order test for the Session config.
---
Nitpick comments:
In `@src/daemon/providers/claude/index.ts`:
- Around line 643-648: The server config enrichment logic in the loop over
Object.entries(servers) is bypassing validation by double-casting through
unknown and back to McpServerConfig. Update this path to validate the
timeout-enriched object with the existing MCP server Zod schema instead of
casting, ideally by reusing or extracting the schema behind
parseMcpServerConfig. Keep the runtime behavior the same, but ensure the final
value assigned to out[name] is the parsed/validated result rather than an
unchecked object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 507cd6d0-b238-4db1-852c-f2c88b157f07
📒 Files selected for processing (4)
src/config.tssrc/daemon/providers/claude/index.tssrc/tests/config.test.tssrc/tests/provider-claude.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config.ts
CodeRabbit: the "SDK signals first" layering was only locked in by the default values. An env override / config file could set the MCP timeout at or above the stall timeout, so the coarse watchdog would force-recover before the SDK's clean per-tool error fired — silently breaking the documented contract. Add a cross-field Zod refinement on SessionSchema. Because env overrides are re-validated through RootSchema after they're applied, this catches a bad override pair too, not just file config. Opt-out cases are exempt: turnStallTimeoutMs=0 (watchdog off → nothing to race) and mcpToolTimeoutMs=0 (use SDK default → relationship moot). The error message names the fix (lower one, or set either to 0). Tests: reject an out-of-order override pair; allow MCP >= stall when the watchdog is disabled. Adjusted the existing stall-override test to stay above the 120s MCP default. 627/627 pass, biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #46.
Problem
A session could get stuck in "replying" forever with no messages displayed (both Telegram and web UI) when a provider's event stream went silent without a terminal
turn_done— e.g. aBashcommand or MCP gateway call that never returns.Session.#consumeEventsblocked infor await (const event of run.events)with no watchdog (only a 5s ZeroID fence existed). When the stream stalled, the loop never exited →finallynever ran →#activeRunand status stayed pinned attool_running.wasWorking && #activeRun?.pushMidTurnfast-path in#sendInner, pushed into the dead run, and returned without starting a new turn. The user saw their message + "⎆ Queued mid-turn", then nothing.Observed live: a session whose last transcript record was a
Bashtool_callwith no result; SDK subprocess alive 8h+ but sleeping.interruptrecovered it.Not a regression — the wedge mechanism exists on
maintoo (the multi-provider refactor inherited it unchanged). High severity regardless: any single hung tool bricks the session.Fix
#consumeEvents— drives the async iterator manually and races each pull againstsession.turnStallTimeoutMs(default300000,0disables). On silence, force-recovers.#recoverStalledRun— flushes streaming UI, releases pending approvals, drops the run, emits a clear⚠️ Turn timed out…notice, resets status toidle, and hard-tears-down the provider to reap the presumed-hung subprocess. Deliberately does not route through#teardownProvider(which awaits the session event-consumer task — i.e. potentially itself), avoiding a self-await deadlock; it only awaitsprovider.teardown()(the provider's own pump).#sendInner— a send arriving on an apparently-dead run recovers it and starts a fresh turn instead of silently queueing into the wedge.session.turnStallTimeoutMs+CODEOID_TURN_STALL_TIMEOUT_MSenv override. Generous default because long-running tools still emittool_progress/partial events, so total silence for the window is a reliable hang signal.stallmode (emit then leave the queue open) + close-on-teardown, mirroringClaudeProvider, for deterministic offline tests.Layered defense (added in review)
The watchdog is deliberately a coarse, provider-agnostic backstop — the SDK exposes no whole-turn / idle liveness signal, only per-tool timeouts. So the precise fix for the trigger we actually hit (a hung MCP gateway call) is the SDK's own per-server tool-call timeout:
session.mcpToolTimeoutMs(default120000) is applied to external/user MCP servers via each server's SDKtimeout. A hung call now returns an SDK error event the turn loop acts on, instead of going silent.turnStallTimeoutMs(300s) so the SDK signals first; the stall watchdog only fires for what per-tool timeouts can't cover (a non-MCP hang, a genuinely silent stream) and for non-Claude providers with their own stall modes.0= use SDK default.We continue to rely on the SDK's real terminal signals (stream end, errors/throws incl. subprocess crash,
result→turn_done) — those are unchanged and not duplicated.Tests
New T9 group in
session-integration.test.ts:idle, emits the timeout notice, and reaps the provider.0), a silent turn stays blocked (no false recovery).Verified:
tsc --noEmitclean,biomeclean, 617/617 tests pass.Base & bundled commits
Targets
main. The stall watchdog wraps the#pendingMidTurnCountmid-turn-absorb logic, which isn't onmainyet, so this PR bundles the two prerequisite consumer fixes it builds on:fix: remove premature tool cancellation from text_done handlerfix: consume mid-turn continuation turn after SDK interrupts current turnfix: recover wedged turns when provider event stream stalls (#46)← this fixAll three are consumer-loop fixes; bundling keeps
main's turn-handling internally consistent. Merge once →mainhas the fix; restart the daemon frommainto pick it up.🤖 Generated with Claude Code
Summary by CodeRabbit
session.turnStallTimeoutMs(default 300,000ms) viaCODEOID_TURN_STALL_TIMEOUT_MS(supports0to disable).session.mcpToolTimeoutMs(default 120,000ms) viaCODEOID_MCP_TOOL_TIMEOUT_MSto apply timeouts to MCP tool calls.