Close abandoned turns deterministically - #334127
Conversation
Nothing closed a turn whose producer had gone: a dropped completion left a tool stuck Running, a spinner that never stopped, steering refused for lack of an in-flight turn, and a session that could never be released. The only watchdog was telemetry, which reports and dispatches nothing. Implement the deterministic half of the fix. Claude now implements IAgent.getTurnDiagnosticSnapshot. The prompt queue reports whether a turn is still queued and whether it was already yielded to the SDK, the pipeline records how each turn's call settled plus a quiet-time figure, and the session appends the ids parked in its permission, user-input and client-tool registries. After agent.chats.sendMessage resolves, and on the resume path, AgentSideEffects checks the turn. Claude's send resolves only once the final result has been dispatched, so a turn that is still active at that point lost its completion: it is closed as complete when the provider reports the call resolved, or failed with a non-resumable executionAbandoned when it reports rejected or never started. Providers opt into that invariant with callSettlesWithTurn, so Copilot (whose send returns before session.idle ends the turn) and Codex are unaffected. A turn parked on a request the user can see is never closed. Two root-cause fixes keep the reaper a backstop rather than the primary path. A stale tool ready is now denied instead of dropped, which used to leak the SDK's canUseTool for any tool that reached a terminal state before its ask arrived, and removing an active client fails the client tool calls it left parked. On a terminal action, a subagent whose spawning tool call already finished ends with its parent. A still-running spawn is a background subagent that outlives its parent turn by design, so the check reads the tool call before the terminal action force-cancels it. Both host-synthesized terminal sites now go through one _endTurnFromHost helper that guards on the turn still being active and not already cancelled, so a user cancel racing the reaper ends the turn exactly once. There is no wall-clock or quiet-time trigger: quietMs is collected for diagnostics only and nothing acts on it.
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: TylerLeonhardtMatched files:
|
There was a problem hiding this comment.
🟡 Changes recommended
Rejected sends and some synthesized terminal paths can still leak state or strand turns.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds deterministic cleanup for abandoned Agent Host turns and fixes related Claude request leaks.
Changes:
- Adds provider diagnostics and post-send abandoned-turn closure.
- Tracks and fails parked Claude client-tool calls.
- Adds regression coverage for diagnostics, cancellation, and subagents.
File summaries
| File | Description |
|---|---|
common/agent.ts |
Extends turn diagnostics. |
node/agentSideEffects.ts |
Closes abandoned turns and handles stale requests. |
node/claude/claudeAgent.ts |
Exposes diagnostics and client cleanup. |
node/claude/claudeAgentSession.ts |
Reports pending requests and fails parked calls. |
node/claude/claudePromptQueue.ts |
Exposes queued-turn state. |
node/claude/claudeSdkOptions.ts |
Tags client-tool requests with owners. |
node/claude/claudeSdkPipeline.ts |
Tracks call settlement and provider liveness. |
node/claude/clientTools/claudeClientToolMcpServer.ts |
Passes tool identity during result registration. |
test/node/agentSideEffects.test.ts |
Tests turn cleanup and stale permission handling. |
test/node/claudeAgent.test.ts |
Tests Claude diagnostics and client removal. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| const sessionUri = isAhpChatChannel(channel) ? parseRequiredSessionUriFromChatUri(channel) : channel; | ||
| const clientContext = this._turnTracker.getClientTelemetryContext(channel, turnId); | ||
| this._stateManager.dispatchServerAction(channel, action); |
There was a problem hiding this comment.
Correct, and the worst of the four: this change would have created the exact condition it exists to remove. _endTurnFromHost dispatched directly, skipping the _endSubagentTurns call the provider-signal path runs at its terminal block, so the reaper closing an abandoned parent left the child chat's turn active forever.
Fixed in d24c72b inside _endTurnFromHost, so every host-side close gets it rather than only the reaper. Resolved spawns are captured before the dispatch, because the reducer's endTurn force-cancels every non-terminal tool call and reading spawn status afterwards makes a live background spawn look finished. It recurses through nested subagents and terminates on the finite spawn tree.
Test: closing an abandoned parent also ends its resolved subagent turns, which fails without the fix.
AI disclosure: this comment and the related code were written with the assistance of AI.
| return undefined; | ||
| } | ||
| const server = await buildClientToolMcpServer(tools, id => registry.register(id), sdkService); | ||
| const server = await buildClientToolMcpServer(tools, (id, toolName) => registry.register(id, toolDiff.model.ownerOf(toolName)), sdkService); |
There was a problem hiding this comment.
Correct, and it is a hole the ownership tagging in this PR opened. removeActiveClient calls removeClientTools, which only mutates the tool diff, so the already-built MCP server keeps serving the tool. ownerOf then returns undefined and failClientToolCalls's predicate compares against a client id, which can never match undefined.
It is worse than a leak: the parked call would appear in pendingHostRequests forever, and this PR's own gate treats a provider-tracked request as a reason to leave the turn alone, so the diagnostics added here would have held the turn open permanently.
Fixed in d24c72b: the client tool answers with an MCP error instead of parking when there is no current owner. The guard is deliberately owner === undefined && !hasBufferedResult(id) rather than a bare early return, because a sibling change fails ownerless calls from the mapper side by buffering a result that the handler's later register consumes. Those cannot double-answer, since the SDK only observes the handler's return value, but a naive early return would have silently broken that path. Tests: fails a call whose client has gone rather than parking one nothing can answer, which times out without the fix because the handler parks forever, and still consumes a result buffered before the handler ran for the ordering.
AI disclosure: this comment and the related code were written with the assistance of AI.
| ).then(() => { | ||
| this._closeAbandonedTurn(agent, channel, URI.parse(channel), action.turnId, this._chatContext(sessionChannel, channel), execution.duration + execution.stopWatch.elapsed()); |
There was a problem hiding this comment.
Correct. Deletion happened at three sites plus channel teardown, and the new host-side close was not one of them.
Fixed in d24c72b by centralizing it in _endTurnFromHost, as you suggested, rather than adding a fourth deletion site. Every host-side close now drops the record, along with the subagent cleanup added for the sibling point on this review.
Test: closing an abandoned resumed turn drops its resumed execution record, which fails without the fix.
AI disclosure: this comment and the related code were written with the assistance of AI.
| } | ||
| this._turnTracker.setCurrentStage(turnChannel, turnId, 'provider'); | ||
| await agent.chats.sendMessage(chatUri, contribution.message.text, resolvedWorkingDirectories, resolvedAttachments, turnId, senderClientId, clientContext.clientType, sendContext); | ||
| this._closeAbandonedTurn(agent, turnChannel, chatUri, turnId, sendContext, this._turnDuration(turnStopWatch)); |
There was a problem hiding this comment.
Correct that this path never reports executionAbandoned, and fixed in d24c72b, but deliberately for only half of it, because mapping every rejection to that outcome would lose information.
The two paths are not symmetric. On the fulfilled path the host synthesizes an error precisely because none exists: the send resolved, nothing threw, and the snapshot is the only evidence the turn died. On the rejection path the thrown error is the reason the snapshot says rejected, and it is strictly richer: buildTurnFailureError decodes proxy error markers into _meta.chatError so core renders a localized message for authentication, quota and unsupported-model failures. Replacing that with a fixed string would discard real diagnostics and collapse distinct provider failures into one bucket in the turn-completed telemetry. Worth noting too that the catch already dispatches an error and completes the turn unconditionally, so no turn was stranded here; this was about the reported outcome, not the defect class the PR removes.
The line drawn is whether the provider ever took the turn. notStarted, which is a send throwing before the queue push such as a failed rebind, has no provider-side story, so the host reports the non-resumable abandoned outcome through the shared helper and gets the same subagent and resumed-execution cleanup as any other host-side close. Everything else keeps its own error untouched, pinned by a genuine send failure keeps its own error even when the provider call was rejected, which asserts both the error type and that the provider's message survives.
Test for the fixed half: a send that rejects before the provider took the turn is reported as abandoned.
AI disclosure: this comment and the related code were written with the assistance of AI.
Follow-up to the abandoned-turn reaper, addressing four review findings. A host-synthesized terminal action bypassed the cleanup that the provider signal path runs, so closing an abandoned parent left its subagent chats with stranded active turns: the reaper was creating the condition it exists to clear. It also never dropped the turn's resumed-execution record, so repeated abandoned resumes retained usage and stopwatch data until channel teardown. Both now happen inside _endTurnFromHost, which every host-side close already goes through, rather than at a fourth call site. Resolved spawns are read before the dispatch because ending a turn force-cancels its running tool calls, which would otherwise make a live background spawn look resolved. Tagging a parked client tool call with its owning client left a hole: removing a client only marks the tool diff for the next send, so the SDK can still invoke an already-built tool whose client has gone. The owner lookup then returned undefined, the call parked under an owner no failClientToolCalls call could ever match, and the new diagnostics reported it as a pending host request forever, holding the turn open. Such a call is now answered with an MCP error instead of being parked. A result buffered before the handler ran still wins, so a mapper-side failure is not double-answered. The post-send check only ran when sendMessage fulfilled, so a turn the provider never took was reported as a generic send failure. The rejection path now consults the snapshot too. The line is whether the provider ever took the turn: notStarted means there is no provider-side story and the host reports the non-resumable executionAbandoned outcome, while a call the provider did take keeps its own error, which carries the real message and the decoded proxy error metadata that core renders. Still no wall-clock or quiet-time trigger, and a turn parked on a request that is visible in state or tracked by the provider is left alone on both paths.
Closes a turn whose producer has gone, deterministically and with no timers, and fixes two leaks that were creating abandoned turns in the first place.
Why
Nothing ever ends a turn once the thing that should have ended it is gone. Every stranded-turn symptom shares this root: a tool stuck Running forever, a response that spins indefinitely, steering silently refused because there is no in-flight turn, queued messages never admitted, and a session that can never be released because it still looks busy. The only existing watchdog is telemetry, which reports a hung turn and dispatches nothing.
What closes a turn
One deterministic rule, no wall clock. After
sendMessageresolves, if the turn it sent is still the active turn, its completion was dropped, because for a provider whose call settles with the turn the terminal signal is dispatched before that promise's continuation runs. Such a turn is completed when the provider reports its call resolved, or failed as non-resumableexecutionAbandonedwhen it reports rejected or never started.This is gated on a new provider-declared
callSettlesWithTurn, not inferred. Copilot'ssendresolves with a message id and its turn ends later on idle, and Codex resolves early too, so a snapshot-only gate would have closed live turns for both on nearly every send. Only the Claude pipeline sets the flag. There is a regression test for a provider whose send returns before its turn ends.A turn is never closed while anything is legitimately waiting: a request the provider still tracks, or one the user can see in state as an unanswered input request or a tool call pending confirmation, result confirmation or authentication.
quietMsis collected in the snapshot and deliberately read by nothing.Two leaks that created abandoned turns
Supporting work
Claude now implements the existing
IAgent.getTurnDiagnosticSnapshotseam that Copilot already implemented and Claude reported as unsupported: the prompt queue can say whether a turn is still queued or yielded, the pipeline records each turn as resolved or rejected and derives session state from its rebind and disposal flags, and the session appends the ids currently parked in its permission, user-input and client-tool registries. Both synthesized-terminal sites now route through one_endTurnFromHosthelper that guards the turn is still active and not already cancelled, so a user cancel racing the close ends the turn exactly once.One ordering detail worth noting: the reducer force-cancels every non-terminal tool call at turn end, so a subagent spawn's status must be read before the parent's terminal action is dispatched, or a live background spawn looks finished.
Tests
Fourteen, including that a dropped completion closes the turn, a rejected call fails it, a pending call is left alone, a provider whose send returns early is untouched, a turn parked on a request the provider tracks or the user can see is untouched, a cancellation racing the close ends the turn once, a resolved subagent ends with its parent while a live background spawn does not, and a stale tool ready is denied.
Reverting the source and keeping the tests produces eight failures. Across the agent-host and Copilot suites: 1297 passing, 0 failing, with Copilot at 747 passing and unchanged. No new type errors, nothing under the synced protocol directory touched.
Not included
No wall-clock or quiet-time trigger, and nothing keyed on the hang telemetry event. Those need tuning against real sessions and belong in a separate change.
Part of #333174.
AI disclosure: this comment and the related code were written with the assistance of AI.