Buffer a client tool result that arrives before the SDK asks for it - #330730
Buffer a client tool result that arrives before the SDK asks for it#330730Ryan Ewen (RyanEwen) wants to merge 3 commits into
Conversation
The workbench starts a client tool from the streamed tool call and can finish it before the SDK has invoked that tool and registered its handler. `respond` drops a result that nothing is parked on, so the handler registers a moment later and waits forever for a result that no longer exists. The turn never completes and nothing is reported to the user or the model. Buffer instead, which is what the Copilot agent already does on the same path. `PendingRequestRegistry.register` and `registerAndFire` both collect an early result, so the late handler resolves immediately rather than re-running the tool. Observed in a dev container, where the client tool round trip crosses the websocket transport and loses the race far more often than a local window. Auto-approved tool calls always lost it, since nothing delays the workbench; approving in chat usually won it, because the wait gave the SDK time to register.
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Tyler James Leonhardt (@TylerLeonhardt)Matched files:
|
There was a problem hiding this comment.
Pull request overview
Fixes a race where client-tool results arrive before Claude SDK handler registration and are dropped.
Changes:
- Buffers unmatched client-tool results.
- Adds regression coverage for early result delivery.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
claudeAgentSession.ts |
Buffers early tool results. |
claudeAgent.test.ts |
Tests late SDK registration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Buffering every unmatched completion retained one result per SDK-owned tool call until the registry was cleared, and results can be large. Bound `_earlyResults` and evict the oldest instead. Condense the method JSDoc and the test comment to the repository's limits.
|
Related: these three share a root cause. Noting it so they can be triaged together rather than as three unrelated patches. The workbench executes a client tool off the stream-mapper
If client tools executed only on the SDK-driven invocation, with the stream-mapper ready used for rendering only, all three would be structurally impossible: one authoritative trigger, never without real input, and registration ordered before execution. I have not proposed that as a PR because I do not know whether the stream-mapper ready is deliberately actionable. Starting the tool as soon as its arguments finish streaming, instead of waiting for the SDK round trip, is a plausible latency win, and the permission flow may already depend on the current ordering. That is a call for whoever owns the design. Each of the three fixes a reproducible bug on its own and carries a test that fails without it, so they are safe to land in the meantime. If the structural fix is preferred, #330683 and #330684 can reasonably be closed in its favour; #330730 is worth keeping either way, since buffering a result that arrives before its handler is the same defensive behaviour |
|
Follow-up on the "shared root cause" note above: after tracing the design, this PR is not defensive hardening around that seam — it completes the intended design, and the structural alternative I floated should NOT be pursued. Eager execution is deliberate. The agent host itself asks the workbench to run a client tool the moment its input finishes streaming: a The lineage confirms it: the buffer half was added for Copilot in 216f88d ("agentHost: fix orphaned client tool calls after window reload", June 2026) — the same class of bug this PR fixes — and Gating client tools on the SDK-driven invocation instead would forfeit the latency win the eager path exists for, and would re-break what 216f88d fixed. |
| private readonly _earlyResults = new Map<string, TResult>(); | ||
|
|
||
| /** Upper bound on {@link _earlyResults}, so completions that never register cannot accumulate. */ | ||
| private static readonly _maxBufferedResults = 16; |
There was a problem hiding this comment.
16 seems arbitrary. It's perfectly valid for an agent to make N+1 tool calls for any _maxBufferedResults value. A cap here indicates to me we have unsoundness elsewhere; I would seek to address or at least understand that.
| completeClientToolCall(toolCallId: string, result: ToolCallResult): boolean { | ||
| const converted = convertToolCallResult(result, toolCallId); | ||
| return this._pendingClientToolCalls.respond(toolCallId, converted); | ||
| const settled = this._pendingClientToolCalls.respond(toolCallId, converted); |
There was a problem hiding this comment.
We can just change to responseOrBuffer entirely here, the first respond() guard is just duplicative
… bound The cap was covering for indiscriminate forwarding: every ChatToolCallComplete was handed to the provider, including SDK-owned calls that never register a handler, so their results sat buffered for the life of the session. Skip a completion whose tool call is not client-contributed, which leaves the buffer bounded by the client tool calls actually in flight, and remove the arbitrary limit. respondOrBuffer now reports whether it settled, so completeClientToolCall is a single call rather than a respond guard followed by the same check.
Correct, and the unsoundness is one layer up. Fixed at the source: a completion whose tool call is not client-contributed is no longer forwarded. Only client tools have a parked handler awaiting a result, and SDK-owned calls carry no contributor at all, so the check is unambiguous. A call that cannot be found is still forwarded, since it cannot be ruled out. The buffer is now bounded by the client tool calls actually in flight, and the cap is gone. The Copilot reviewer flagged the same accumulation from the other end.
Done. New test: a client-contributed completion is forwarded and an SDK-owned one is not. The bound test is replaced by one covering the settled/buffered return. Worth noting for triage: the race this PR handles is itself downstream of #330899. Client tools are executed off the streamed ready rather than the SDK invocation, so a result can be produced before anything awaits it. If execution were driven by the invocation, registration would always precede execution and the buffer would be dead code rather than a safety net. Filed with the two sibling defects, and I have a branch exploring the single-trigger change if that direction is wanted. |
Fixes the stall in #322990.
A client tool call ends up stuck forever: the tool runs, the workbench reports its result, and the turn never finishes. Nothing is shown to the user and nothing reaches the model.
Cause
The workbench starts a client tool from the streamed tool call (the
content_block_stopready) and can finish it before the SDK has invoked that tool and registered its handler.ClaudeAgentSession.completeClientToolCallusedPendingRequestRegistry.respond, which discards a result when nothing is parked on that id. The handler registers a moment later and waits forever for a result that no longer exists.The
CopilotAgentSessionequivalent already usesrespondOrBufferon this path.Fix
Buffer when nothing is parked.
registerandregisterAndFireboth collect an early result, so the late handler resolves immediately instead of the tool being re-run.Evidence
Instrumented against a real 1.133.0 dev container, logging whether the completion matched a parked handler and every id passed to
register:With the fix, the same call logs the buffered result being collected on registration, and the turn continues:
Reproduction rule, before the fix:
Auto-approval leaves nothing to delay the workbench, so it always loses the race. Approving in chat usually wins it, because the wait gives the SDK time to register. This is also why it shows up in remote windows: the client tool round trip crosses the websocket transport.
Testing
a result delivered before the SDK registers is buffered, not dropped. Without the fix it fails by timing out, which is exactly the production symptom../scripts/test.sh --grep ClaudeAgent: 266 passing, 3 failing, the same 3 failing on an unmodified checkout (model catalog / credential tests, unrelated).npm run typecheck-clientclean.