fix(think): hold continuation barrier for a slow tool sibling pending only in the streaming accumulator (#1649 follow-up) - #1663
Closed
threepointone wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: c0070a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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://cdn.statically.io/img/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: |
Contributor
Author
Follow-up: deeper review + added coverageDid a full edge-case review and added two more layers of coverage (pushed in Edge cases reviewed
Added coverage
Both fail without the fix — the e2e reproduces the exact customer signature ( Verification: in-process #1649 suite 11 passed, typecheck 91/91, lint clean, e2e passes against real |
… only in the streaming accumulator (#1649 follow-up) ## Problem #1657 fixed the parallel-tool clobber, but a customer hit the same "The tool call was interrupted before a result was recorded." error on a SLOW client-resolved sibling (an RPC taking 2-5s) running beside a fast one. When the model fans out parallel client tool calls in one streaming step: 1. The fast sibling resolves mid-stream and is applied to the accumulator. 2. Its `autoContinue` schedules the continuation and arms the 50ms barrier timer. 3. The timer fires while the assistant message still lives ONLY in `_streamingAssistant` (not yet persisted). 4. `_hasIncompleteToolBatch()` scanned only `this.messages`, so it inspected a stale prior leaf, reported "not mid-batch", and the fast path fired the continuation — bypassing the barrier entirely. 5. The stream ends and persists with the slow sibling still `input-available`; the continuation's transcript repair errors it. 6. The slow RPC result arrives 2-5s later — too late. The bypass was masked by a NOTE comment claiming the turn queue guarantees the result is settled before the continuation's repair runs. That holds for a fast sibling that settles before end-of-stream, but not for a slow RPC sibling. ## Fix `_hasIncompleteToolBatch()` now inspects the in-flight `_streamingAssistant` accumulator first (the true current leaf), falling back to the latest persisted assistant message. A slow sibling still `input-available` in the accumulator now keeps the barrier closed until its result lands or the existing 60s timeout elapses. The part-scan is extracted into a shared `_partsAreMidBatch` helper. ## ai-chat (defensive) The same guard is applied to `@cloudflare/ai-chat`'s `_hasIncompleteToolBatch`. ai-chat's barrier runs in the post-stream continuation turn (where `_streamingMessage` is already null), so it does not exhibit the bypass today; this keeps the two implementations aligned. Mirrors the existing `hasPendingInteraction()`, which already checks `_streamingMessage`. ## Tests - Deterministic guard (`detectsMidBatchInStreamingAccumulator`): a settled `tc-fast` beside a pending `tc-slow` exposed only in the accumulator must report mid-batch; cleared accumulator + empty history must not (no false positive). - Integration test: two parallel client tools streamed, fast answered mid-stream with autoContinue, slow answered after the stream ends. Asserts the slow tool ends `output-available` (not errored) with exactly one continuation. - Both verified to FAIL without the fix — the integration test reproduces the exact customer signature (`expected 'output-error' to be 'output-available'`). All checks pass: format, oxlint, typecheck (91/91), think (495), ai-chat (566).
…ow-sibling barrier (#1649) Follow-up coverage for the streaming-accumulator barrier fix. - Approval path: generalize `detectsMidBatchInStreamingAccumulator` to take the pending sibling's state and add a test asserting the barrier holds when the slow sibling is `approval-requested` (not just `input-available`). - Full wrangler-dev e2e: add `ThinkParallelClientToolE2EAgent` (deterministic mock model emitting two parallel client tool calls, holding the stream open) with `@callable` helpers (`streamingToolState`, `toolStates`, `continuationCount`). New `parallel-client-tool.test.ts` spins up a real `wrangler dev` worker, answers the fast tool mid-stream and the slow tool after the stream ends, and asserts the slow tool settles `output-available` with exactly one continuation. Both verified to FAIL without the fix — the e2e reproduces the exact customer signature (`expected 'output-error' to be 'output-available'`) deterministically across all 3 retries. In-process #1649 suite: 11 passed; typecheck 91/91.
Co-authored-by: Cursor <cursoragent@cursor.com>
threepointone
force-pushed
the
fix/streaming-accumulator-tool-batch-barrier-1649
branch
from
June 3, 2026 10:00
00d50bb to
50771cb
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
threepointone
added a commit
that referenced
this pull request
Jun 3, 2026
#1663 tried to fix #1649 by making `_hasIncompleteToolBatch` read the streaming accumulator (`_partsAreMidBatch`). That approach never landed (it couldn't see a sibling the model hadn't streamed yet), and our fix uses the stream-active gate instead — so #1663's four accumulator-scan UNIT tests target an implementation we deliberately don't have and were not ported. Their behavioral intent is covered end-to-end by the existing stream-active-gate tests. Ported the two scenarios that carry real signal for our design: - Different ordering from the headline race: BOTH parallel tool calls streamed up front and visible in the accumulator, the fast one answered mid-stream, the slow sibling answered only AFTER stream end. Guards the gate across stream finalize (would fire prematurely and error the slow sibling without it). - Approval-path variant: a parallel batch whose slow sibling awaits an APPROVAL response rather than a result. Locks down that `_hasIncompleteToolBatch` treats `approval-requested` as pending so the barrier holds until approval, then fires exactly once (the approval analog of the existing parallel-results barrier test). client-tools suite 74/74; oxlint clean; all 91 projects typecheck.
Contributor
Author
|
clsoing this in facour of #1667 for now |
threepointone
added a commit
that referenced
this pull request
Jun 3, 2026
…te (fixes #1649) (#1667) * fix(think): make parallel-tool auto-continuation barrier event-driven (#1650) The #1649 barrier made auto-continuation wait for all of a step's parallel client-tool results before firing, but bounded the wait with a fixed 60s timeout that fired through on expiry. That timeout was the wrong primary mechanism: - A human-in-the-loop tool with no `execute` (an `ask_user`/`display_ui`-style prompt) emitted in parallel with a fast tool legitimately parks at `input-available` for as long as the user takes to answer. The barrier would fire through after 60s and repair the still-open tool to errored mid-answer. - A true orphan (client disconnects mid-batch) pinned the isolate alive via `keepAlive` for the full 60s before falling through to the repair backstop. Auto-continuation is only ever triggered by a tool-result/approval event, so the barrier is now purely event-driven: - When the coalesce timer fires on an incomplete batch, Think drains the in-flight applies (`_drainInteractionApplies` — awaits the apply tail and re-reads it so a sibling enqueued mid-drain is still awaited), re-checks, and — if a sibling is still unanswered — returns WITHOUT firing and WITHOUT holding the isolate, leaving `_continuation.pending` in place. - The next sibling's result re-arms the coalesce timer and re-runs the check; the continuation fires exactly once when the final sibling lands. - If the in-memory pending state is lost to eviction between siblings, the final result re-creates it from the persisted transcript and fires with a complete batch — self-healing across hibernation. - A true orphan never auto-continues and never pins the isolate, which is correct: there is nothing valid to continue, and a later user turn / chat recovery repairs the dangling tool. This removes `AUTO_CONTINUATION_PENDING_TOOL_TIMEOUT_MS` (and its `console.warn`) from the Think path entirely. Errored-sibling completion: because the barrier now keys off events rather than polling message state, a result that COMPLETES a parallel batch but carries `autoContinue: false` (the client sends this for errored tool results) would no longer re-trigger the check, stranding the continuation a successful sibling already requested. `_rearmPendingAutoContinuationForBatch` re-arms the barrier for such a result ONLY when a pending continuation already exists, so the batch still continues exactly once — without ever creating a continuation for a standalone errored tool (documented single-tool error behavior is preserved). Double-fire safety is preserved via `_continuationBarrierActive` (a sibling that re-arms the timer mid-drain is absorbed by the in-progress drain) and by making the fire/return decision synchronous in the drain's `.finally`, so a macrotask timer cannot interleave. `@cloudflare/ai-chat` keeps the bounded-wait barrier for now (its barrier runs inside the queued continuation turn and can't return-and-wait without occupying the chat-turn queue); making it event-driven requires moving the batch gate before queueing, tracked alongside the think<->ai-chat unification (#1642). Tests (packages/think/src/tests/client-tools.test.ts): - human-in-the-loop tool parked beside a fast tool: no fire-through, no repair, fires once on the human's answer - errored sibling (autoContinue:false) completes the batch: continues once - standalone errored tool (autoContinue:false, no opted-in sibling): no continue - self-heals when the pending continuation is evicted mid-batch All existing #1649 regression tests remain green (70/70). * fix(think): hold auto-continuation while streaming — fixes #1649 headline mid-stream race The event-driven barrier alone did not fix #1649's actual reproduction. Per abhagsain's debug-log analysis on the issue: the model emits parallel tool calls SEQUENTIALLY within one step, so a fast client tool can resolve and round-trip its result to the server WHILE the model is still streaming the slower siblings. At that moment the siblings exist nowhere — not in `this.messages`, not in the in-flight `_streamingAssistant` accumulator — so no batch-completeness check can see them. The barrier bypassed, the continuation was enqueued, and when it ran (after the stream persisted all tool parts) it repaired the now-materialized-but-still-pending siblings to errored → MissingToolResultsError / death spiral. The only signal that "more tool calls may still arrive" is that the stream is open. So: - Stream-active gate: `_fireAutoContinuationWhenStable` now returns early while `_streamingAssistant` is non-null (also re-checked in the drain `.finally`). Mid-stream the batch can still grow, so no completeness check is meaningful. - Stream-finalize re-trigger: `_onStreamingTurnFinalized` (called at the two normal stream-finalize sites) clears the accumulator AND re-runs the barrier check. This is essential for an all-fast batch whose every result landed mid-stream — once the stream ends there is no further tool-result event to re-arm the barrier, so without this re-check the held continuation would never fire (deadlock). The abort/recovery paths keep a plain clear; recovery re-runs the turn and its own finalize re-triggers the held barrier. Corrected the prior (incorrect) NOTE that claimed turn-queue ordering made the mid-stream bypass safe — abhagsain's logs disprove it: the continuation runs after persist but still errors the pending siblings. Tests (packages/think/src/tests/client-tools.test.ts) with a new mock model (`createMidStreamParallelToolModel`) that emits a fast tool, holds the stream open, then emits a slow tool: - waits for a sibling emitted LATER in the same stream before continuing (the #1649 headline repro): fast tool resolved mid-stream, no premature continuation, no repair of the slow tool, fires once when the slow tool answers - all-fast batch resolved entirely mid-stream: fires exactly once via the stream-finalize re-check (guards the deadlock) Confidence-checked: both new tests FAIL with the gate + finalize re-trigger disabled (the all-fast case fires 2 continuations), and pass with them. Full client-tools suite 72/72; oxlint clean; all 91 projects typecheck. * test(think): port worthwhile coverage from the abandoned #1663 attempt #1663 tried to fix #1649 by making `_hasIncompleteToolBatch` read the streaming accumulator (`_partsAreMidBatch`). That approach never landed (it couldn't see a sibling the model hadn't streamed yet), and our fix uses the stream-active gate instead — so #1663's four accumulator-scan UNIT tests target an implementation we deliberately don't have and were not ported. Their behavioral intent is covered end-to-end by the existing stream-active-gate tests. Ported the two scenarios that carry real signal for our design: - Different ordering from the headline race: BOTH parallel tool calls streamed up front and visible in the accumulator, the fast one answered mid-stream, the slow sibling answered only AFTER stream end. Guards the gate across stream finalize (would fire prematurely and error the slow sibling without it). - Approval-path variant: a parallel batch whose slow sibling awaits an APPROVAL response rather than a result. Locks down that `_hasIncompleteToolBatch` treats `approval-requested` as pending so the barrier holds until approval, then fires exactly once (the approval analog of the existing parallel-results barrier test). client-tools suite 74/74; oxlint clean; all 91 projects typecheck.
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.
Summary
Follow-up to #1657. A customer reported that #1657 fixed the fast parallel tool case but they still hit
"The tool call was interrupted before a result was recorded."on a slow client-resolved sibling (an RPC taking 2–5s) running beside a fast one (e.g. "delete the 4th slide and rewrite the 1st" →deleteSlidesfast +modifySlideslow).Their root-cause analysis was correct:
_hasIncompleteToolBatchdidn't account for the in-flight streaming accumulator.Root cause
When the model fans out parallel client tool calls in one streaming step:
_streamingAssistant.autoContinueschedules the continuation and arms the 50ms barrier timer._streamingAssistant— not yet persisted._hasIncompleteToolBatch()scanned onlythis.messages, so it inspected a stale prior leaf, reported "not mid-batch", and the fast path fired the continuation — bypassing the barrier.input-available; the continuation's transcript repair errors it.A
NOTEcomment had claimed this was safe because "the turn queue persists the (now settled) result before the continuation's repair runs". That holds for a fast sibling that settles before end-of-stream, but not for a slow RPC sibling that settles after.Fix
_hasIncompleteToolBatch()now inspects the in-flight_streamingAssistantaccumulator first (the true current leaf), falling back to the latest persisted assistant message. A slow sibling stillinput-availablein the accumulator now keeps the barrier closed until its result lands or the existing 60s timeout elapses. The part-scan is extracted into a shared_partsAreMidBatchhelper, and the misleadingNOTEis corrected.ai-chat (defensive)
The same guard is applied to
@cloudflare/ai-chat's_hasIncompleteToolBatch. ai-chat's barrier runs in the post-stream continuation turn (where_streamingMessageis already null), so it does not exhibit the bypass today — this keeps the two implementations aligned and mirrors the existinghasPendingInteraction(), which already checks_streamingMessage.Tests
detectsMidBatchInStreamingAccumulator): a settledtc-fastbeside a pendingtc-slowexposed only in the accumulator must report mid-batch; a cleared accumulator + empty history must not (no false positive).autoContinue, slow answered after the stream ends. Asserts the slow tool endsoutput-available(not errored) with exactly one continuation.expected 'output-error' to be 'output-available').Verification
npm run format,oxlint— 0 warnings / 0 errorsnpm run typecheck— 91/91 projects@cloudflare/think— 495 passed@cloudflare/ai-chat— 566 passedChangeset
patchfor@cloudflare/think(real fix) and@cloudflare/ai-chat(defensive symmetry).