Deep Review: Stall Watchdog → Bounded Recovery (#1626)

I did a thorough read of all 8 changed files. The design is sound — treating a stall as "just another interruption" and routing it through the existing recovery machinery is the right call. The implementation correctly reuses _beginChatRecoveryIncident, the existing budget (maxAttempts + wall-clock + progress-aware reset), and the existing _chatRecoveryContinue continuation path. That said, I found one concrete bug and several edge cases worth discussing.

Bug: Orphaned JSDoc block

The new _routeStallToBoundedRecovery method was inserted between the JSDoc comment for _rescheduleRecoveryAfterStableTimeout and the method definition itself (think.ts:7745-7821). This means:

1. _rescheduleRecoveryAfterStableTimeout (line 7822) now has no docstring — the docstring above it is _routeStallToBoundedRecovery's.
2. _routeStallToBoundedRecovery (line 7772) has two consecutive JSDoc blocks — the first one (lines 7745-7754) is actually _rescheduleRecoveryAfterStableTimeout's orphaned docstring.

The first JSDoc block is silently ignored (the second one wins), but the orphaned docstring is confusing for anyone reading this code. The fix is to move the new method either above or below the _rescheduleRecoveryAfterStableTimeout docstring+method pair, or re-attach the orphaned docstring to its method.

Edge Cases & Questions

1. Stall during a continuation that is itself a stall recovery

If the provider is persistently hanging and the first stall routes to recovery, the continuation itself will also stall and hit the watchdog again. The code handles this because _routeStallToBoundedRecovery calls _beginChatRecoveryIncident which advances the attempt counter on the same incident (keyed by the same user message). Eventually the budget exhausts and it falls through to terminal. This is correct, but worth verifying: does the continuation also have chatStreamStallTimeoutMs armed? Yes — both _streamResult (line 6671) and _streamResultToRpcCallback (line 6302) read the instance property each time, so the watchdog is always armed for continuations. Good.

2. Stall with zero accumulated parts

In _streamResultToRpcCallback (line 6416): if accumulator.parts.length === 0 (the provider parked before emitting any chunks), the stall-recovery path skips the persist step (assistantMsg remains null) and targetAssistantId is undefined. The continuation will then run with no prior assistant message to re-anchor from, which should be equivalent to a fresh turn (the model sees the user message and starts fresh). This seems correct — just calling it out because it's an implicit behavior rather than an explicit one.

3. Stall fires during tool execution (watchdog gap in docs)

The README correctly documents that chatStreamStallTimeoutMs "measures the gap between UI-message-stream chunks, which includes time spent executing server-side tools." This is the key usability concern: a tool that takes 3 minutes to run will trigger a 120s watchdog. The README's advice ("set it comfortably above your slowest tool") is good, but there's no per-turn override for chatStreamStallTimeoutMs in TurnConfig — if a subclass knows a particular turn will invoke a slow tool, they can't bump the timeout for just that turn. This is a DX gap (not a bug, and probably fine for v1 since the property can be mutated in beforeTurn), but worth noting for future API design.

4. _streamResultToRpcCallback vs _streamResult asymmetry

The two catch blocks that handle ChatStreamStalledError are structurally different:

• In _streamResultToRpcCallback (line 6415): the recovery path calls return; (void return), does NOT fire the response hook, does NOT call callback.onDone(). This means the RPC caller (parent agent) gets neither onDone nor onError — the callback is silently abandoned. The comment says "the scheduled continuation owns the real terminal outcome" which mirrors deploy interruption, but this does mean the parent agent's chat() call will hang until the continuation eventually fires onDone. Is the RPC promise for chat() resolved at this point? If the for await loop completed (via the catch), and the method returns, yes — the RPC resolves. But the parent never learns the turn was interrupted and recovered. This is probably fine for the sub-agent case (the parent just sees a delayed completion), but it's a subtle semantic difference from an abort (where onError fires).

• In _streamResult (line 6753): the recovery path returns { status: "aborted" }. This correctly reports the interrupted status to the caller (e.g., saveMessages/submissions).

5. { idempotent: true } for the stall-recovery schedule

Line 7817: await this.schedule(0, "_chatRecoveryContinue", ..., { idempotent: true }). The comment on _rescheduleRecoveryAfterStableTimeout (line 7843-7846) explicitly says "Must NOT be idempotent" for reschedules. The initial stall-recovery schedule uses idempotent: true which is correct (it's the first schedule, not a reschedule from within an executing alarm row). Just want to confirm: if two stalls fire in rapid succession (e.g., a watchdog fires, recovery begins, the continuation immediately stalls again before the first schedule's alarm fires), the idempotent schedule deduplicates correctly. Yes — the second schedule(0, ...) with idempotent: true would be a no-op if the first hasn't fired yet. This is correct behavior.

6. Progress marker not bumped on stall recovery

When a stall fires mid-stream, chunks were flowing before the hang. Those chunks were stored durably via _storeChunkDurably which calls _bumpChatRecoveryProgress. So progress IS recorded before the stall fires — this is correct. The recovery incident will see currentProgress > prevProgress if the stalled attempt produced any durable content, and will reset the attempt counter. Good.

UX/DX Assessment

What's good:

• Zero-config for the common case. If you've set chatStreamStallTimeoutMs and left chatRecovery at the default true, stalls just recover. No new flags, no new hooks, no new config objects.
• The "fail fast" knob is the existing maxAttempts/budget. Consistent with deploy recovery.
• Observability is there: chat:stream:stalled, chat:recovery:detected, chat:recovery:scheduled, chat:recovery:attempt, chat:recovery:exhausted events all fire at the right points.
• Backward compatible: with chatRecovery: false, the behavior is unchanged (terminal error).

What could be better (future, not blockers):

• A per-turn chatStreamStallTimeoutMs override via TurnConfig would let subclasses handle known-slow turns (e.g., tool-heavy turns) without permanently raising the global timeout.
• The _streamResultToRpcCallback stall path silently abandoning the StreamCallback is defensible but undocumented. A brief note in the README's stall-recovery section about sub-agent behavior would help.

Tests

• Unit test (think-session.test.ts:520-535): Good coverage — verifies no terminal error, scheduled continuation fires, and final transcript has content. The _stallAttemptsRemaining gating in the mock model is clean.
• Existing watchdog→terminal test now correctly runs with chatRecovery = false — this is the right way to preserve both test paths.
• E2E test (stall-recovery.test.ts): Tests the real alarm path with wrangler dev. The 60s poll loop with 2.5s intervals is generous. The assertion on hasFiberRows === false is important (no orphaned fibers). The 120_000 timeout is appropriate for CI.

Summary

The implementation is correct and well-designed. The one concrete issue is the orphaned JSDoc block at line 7745 which should be fixed before merge. The rest are observations and future DX suggestions, not blockers.