Repository navigation
fix(think): don't re-arm auto-continuation barrier on RPC stall recovery - #1671
Merged
Merged
Conversation
Follow-up to #1667. Two findings from review of the event-driven auto-continuation barrier. #1 (fix): The RPC streaming path (`_streamResultToRpcCallback`) re-armed the auto-continuation coalesce timer in its `finally` even on the stream-stall recovery early-returns (`scheduled`/`exhausted`). This is unlike the WebSocket `_streamResult` recovery paths, which deliberately do a plain `this._streamingAssistant = null` WITHOUT re-arming, because the scheduled recovery continuation re-runs the turn and its own stream finalize re-triggers the held barrier. When a parallel tool batch had a pending continuation at the moment the stall watchdog fired, the RPC re-arm scheduled a 50ms coalesce timer that could fire `_fireAutoContinuation` alongside the alarm-scheduled recovery continuation -> a spurious double model invocation on the turn queue. The RPC recovery early-returns now mirror the WebSocket path: a `skipFinalizeRearm` flag makes the `finally` do a plain clear instead of `_onStreamingTurnFinalized()`, so the held barrier is re-triggered exactly once by the recovery continuation. #2 (document): The eviction self-healing path relies on the completing result carrying `autoContinue: true` (it re-creates `_continuation.pending` from the persisted transcript via `_scheduleAutoContinuation`). If the completing result is an errored `autoContinue: false` sibling AFTER eviction, `_rearmPendingAutoContinuationForBatch` finds no pending and no-ops -- the continuation is silently dropped. This is NOT a regression (the old in-memory 60s timer was equally eviction-fragile) and a later user message / chat recovery repairs the transcript; fixing it properly would require persisting the continuation-requested intent across eviction. Pinned the current behavior with an explicit test rather than changing it. Tests: - think-session.test.ts: asserts the coalesce timer is NOT re-armed after an RPC stall routes into recovery (while `_streamingAssistant` is still cleared). Adds `testStallRecoveryDoesNotRearmPendingContinuation`. - client-tools.test.ts: documents the eviction + errored-completing- result gap (0 continuations, both results still applied to transcript). npm run check passes; new + existing stall/self-heal tests green.
🦋 Changeset detectedLatest commit: 37a44b7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
Merged
This was referenced Jun 3, 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.
Follow-up to #1667 (event-driven auto-continuation barrier). Addresses two review findings.
Finding #1 — fix (real bug)
The RPC streaming path
_streamResultToRpcCallbackre-armed the auto-continuation coalesce timer in itsfinallyeven on the stream-stall recovery early-returns (scheduled/exhausted). Areturninside thecatchstill runs thefinally, so_onStreamingTurnFinalized()— which calls_rearmPendingAutoContinuationForBatch()— always fired.This diverges from the WebSocket
_streamResultrecovery paths, which deliberately do a plainthis._streamingAssistant = nullwithout re-arming (see the comment at the WSscheduledbranch: "recovery re-runs the turn and its own stream finalize re-triggers the held barrier").Impact: when a parallel tool batch had a pending continuation at the moment the stall watchdog fired, the RPC re-arm scheduled a 50ms coalesce timer that could fire
_fireAutoContinuationalongside the alarm-scheduled_chatRecoveryContinuecontinuation → a spurious double model invocation on the turn queue.idempotent: trueonly dedupes the scheduled task, not the auto-continuation.Fix: a
skipFinalizeRearmflag, set in thescheduled/exhaustedbranches, makes thefinallydo a plain_streamingAssistant = nullclear instead of_onStreamingTurnFinalized()— mirroring the WS path. The held barrier is then re-triggered exactly once, by the recovery continuation's own stream finalize.Finding #2 — document (not a regression, not safely fixable here)
The eviction self-healing path relies on the completing result carrying
autoContinue: true— that re-creates_continuation.pendingfrom the persisted transcript via_scheduleAutoContinuation. If the completing result is an erroredautoContinue: falsesibling after eviction,_rearmPendingAutoContinuationForBatchfinds no pending (evicted) and no-ops, so the continuation is silently dropped.The only signal that continuation was requested (the fast sibling's
autoContinue: true) lives in the in-memory pending, not the persisted transcript — so it can't self-heal after eviction. This is not a regression (the old in-memory 60s timer was equally eviction-fragile) and a later user message / chat recovery repairs the transcript. A proper fix would require persisting the continuation-requested intent across eviction. Pinned the current behavior with an explicit test rather than changing it.Tests
think-session.test.ts— "does not re-arm a held auto-continuation when an RPC stall routes into bounded recovery": seeds a pending continuation, stalls into recovery, and asserts the coalesce timer is not re-armed (while_streamingAssistantis still cleared). Adds thetestStallRecoveryDoesNotRearmPendingContinuationharness method.client-tools.test.ts— "documents the known gap: eviction + an errored autoContinue:false completing result drops the held continuation": asserts 0 continuations fire and both tool results are still applied to the transcript.Verification
npm run checkpasses (sherif, export checks, oxfmt, oxlint, typecheck across 91 projects).@cloudflare/think.