fix: stall watchdog no longer kills long-running tools - #104
Conversation
The 300s turn-stall watchdog treated ANY provider-event silence as a wedged
turn, but its core assumption ("long-running tools still emit tool_progress /
partial events") is false: a multi-minute Bash run, Task subagent, or web
research emits NOTHING until it completes. The watchdog was force-recovering
actively-working sessions — tearing down the subprocess, cancelling tool
calls, and telling the user "Turn timed out — no activity for 300s".
Two distinct paths had the bug:
1. #consumeEvents watchdog: silence is now only a stall signal while the
MODEL should be producing output. A shared #watchdogPaused() gate (status
tool_running, waiting_approval, or pending approvals) pauses it. Crucially
the pause is re-checked WHEN THE TIMER FIRES, not just when it is armed —
the timer arms while status is still "thinking", and a tool_start landing
right after arming previously left the old timer counting down over a
legitimately-silent tool execution. The in-flight iterator pull now
persists across loop iterations so the paused re-check can loop back and
re-await the same pull (no concurrent iter.next()).
2. #sendInner liveness guard: sending a message into a >stallMs-silent run
(e.g. "how's it going?" during a long exploration) force-recovered the run
before queuing. It now respects the same pause gate — mid-tool sends queue
mid-turn instead of killing in-flight work.
Wedge protection is NOT lost: silence while thinking still recovers, MCP
calls have their own finer timeout (mcpToolTimeoutMs), SDK built-ins carry
tool-level timeouts, a dead subprocess closes the event stream (ending the
iterator), and the user can always interrupt.
Tests: long-silent tool execution survives past the stall window (regression
for the arming race — this test fails on the pre-fix code); mid-tool send is
not treated as a stall; existing T9 wedge-recovery / disabled-watchdog /
approval-pause tests unchanged and green. Full suite 833 pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe session stall watchdog now pauses recovery during tool execution, waiting approval, and pending approvals through a new ChangesStall Watchdog Pause Behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Session
participant Watchdog as Stall Timer
participant Tool as Tool Execution
Client->>Session: send()
Session->>Tool: start tool
Tool-->>Session: tool_running / silence
Watchdog->>Session: stall timeout fires
Session->>Session: `#watchdogPaused`()
alt paused
Session-->>Watchdog: continue waiting
else not paused
Session->>Session: `#recoverStalledRun`()
end
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 76.34% 76.35% +0.01%
==========================================
Files 70 70
Lines 11539 11547 +8
==========================================
+ Hits 8809 8817 +8
Misses 2730 2730
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tests/session-integration.test.ts (1)
1289-1333: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStrengthen assertion to catch duplicate/overlapping turns.
This test's own comment acknowledges "the mock lacks pushMidTurn, so the send legitimately starts a fresh turn" — but nothing here verifies that starting that fresh turn didn't leave the original tool-executing run orphaned. Asserting only on the absence of a "timed out/stalled" message wouldn't catch the scenario described in the
session.ts#sendInnerreview comment (a secondrunTurn()call on the same provider while the first run/queue is never closed).Consider adding
expect(provider.capturedOpts.length).toBe(1)(or asserting onprovider.teardownCount/queue state) to make an unintended duplicate turn fail loudly instead of passing silently.🤖 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/tests/session-integration.test.ts` around lines 1289 - 1333, Strengthen the session integration test in the "a send arriving mid-long-tool queues instead of killing the run (liveness guard pauses too)" case so it also detects duplicate or overlapping turns, not just stalled-message absence. Update the test around session.send, makeSession, and MockSessionProvider assertions to verify only one provider turn/run is active or started (for example via provider.capturedOpts length, teardownCount, or equivalent queue state) so a second runTurn on the same provider cannot silently orphan the original tool-executing run.
🤖 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.
Nitpick comments:
In `@src/tests/session-integration.test.ts`:
- Around line 1289-1333: Strengthen the session integration test in the "a send
arriving mid-long-tool queues instead of killing the run (liveness guard pauses
too)" case so it also detects duplicate or overlapping turns, not just
stalled-message absence. Update the test around session.send, makeSession, and
MockSessionProvider assertions to verify only one provider turn/run is active or
started (for example via provider.capturedOpts length, teardownCount, or
equivalent queue state) so a second runTurn on the same provider cannot silently
orphan the original tool-executing run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 191e1f4a-84cd-4127-9ca8-c0aa230313f9
📒 Files selected for processing (3)
src/config.tssrc/daemon/session.tssrc/tests/session-integration.test.ts
Address CodeRabbit review on #104: asserting only the absence of a "timed out" message could let an overlapping-turn regression pass silently. Now also pins exactly two runTurn() invocations (first run + the send's fresh turn, no third) and zero forced provider teardowns — the stall-recovery path calls provider.teardown(), so the old bug shows teardownCount >= 1 here. Values verified deterministic across runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Applied CodeRabbit's nitpick in 0b47633: the mid-tool-send test now pins the post-send provider state — exactly two |
1 participant
The bug (user-reported workflow breaker)
The 300s turn-stall watchdog treats any provider-event silence as a wedged turn. Its core assumption — written in the code as "long-running tools still emit tool_progress / partial events" — is empirically false: a multi-minute Bash run, a Task subagent exploring a large codebase, or web research emits nothing until it completes. Result: actively-working sessions were force-recovered mid-task — subprocess reaped, tool calls cancelled, "Turn timed out — no activity for 300s. The session was reset".
Two paths fixed (shared
#watchdogPaused()gate)Silence is now only a stall signal while the model should be producing output (status
thinking). The gate pauses duringtool_running,waiting_approval, and pending approvals.#consumeEventswatchdog — the subtle part: the pause is re-checked when the timer fires, not just when it's armed. The timer arms while status is stillthinking; atool_startlanding right after arming previously left the old timer counting down over a legitimately-silent tool execution and killing it the moment the window lapsed. (The debug trace showed exactly this: recovery fired while status wastool_running.) The in-flight iterator pull now persists across loop iterations, so the paused re-check loops back to re-await the same pull — no concurrentiter.next()against the queue.#sendInnerliveness guard — sending a message into a >stallMs-silent run ("how's it going?" during a long exploration) force-recovered the run before queuing. It now respects the same gate: mid-tool sends queue mid-turn instead of killing in-flight work. (This guard also previously ignored pending approvals — sending during a >5min-unanswered approval prompt killed the run too.)Wedge protection is NOT lost
thinkingstill recovers (existing T9 tests unchanged, green).mcpToolTimeoutMs(fires first by config invariant); SDK built-ins carry their own tool-level timeouts.Tests
pushMidTurn, so the assertion targets the stall-recovery signal specifically — the real ClaudeProvider queues mid-turn and the run survives).🤖 Generated with Claude Code
Summary by CodeRabbit