fix: drain tool results after stream disconnect#1252
Conversation
|
Warning Review limit reached
More reviews will be available in 7 minutes and 28 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds tool lifecycle callbacks ( ChangesTool lifecycle and cancellation-driven abort flow
Permission-agent test fixture cleanup
Sequence Diagram(s)sequenceDiagram
participant PromptLoop
participant SessionRunState
participant SessionProcessor
participant LLMStream as LLM.stream
participant ToolExecute as Tool.execute
PromptLoop->>SessionRunState: ensureRunning({ onCancel: abortTools })
SessionProcessor->>LLMStream: stream(toolAbortSignal, toolLifecycle)
LLMStream->>ToolExecute: execute(args, { abortSignal })
Note over ToolExecute: tool running
PromptLoop->>SessionRunState: cancel(sessionID, meta)
SessionRunState->>PromptLoop: onCancel(meta)
PromptLoop->>SessionProcessor: abortTools()
SessionProcessor->>LLMStream: abortProcessTools() / toolAbortController.abort()
LLMStream-->>ToolExecute: abortSignal fires
ToolExecute-->>SessionProcessor: lifecycle.failed(toolCallID, AbortError)
SessionProcessor->>SessionProcessor: finalizeToolLifecycles() with bounded drain
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Astro-Han
added
enhancement
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces tool lifecycle hooks (started, completed, failed) and abort signals to manage tool execution, tracking, and cleanup during LLM streams. It also adds robust state tracking to prevent duplicate execution records and includes a test verifying tool completion persistence after stream timeouts. The review feedback highlights two important improvements: decoupling the tool execution try-catch block from the completion callback to prevent incorrect failure reporting, and wrapping JSON.stringify in a try-catch block to handle potential serialization errors safely.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/opencode/src/session/run-state.ts (1)
217-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQueued
ensureRunning()calls never receive the newonCancelhook.
ensureRunning()registersoptions.onCancelbeforewithActiveRun(...), butcancel()returns on Line 220 unless this session already has a busy runner. A run that's still waiting on the directory lock has no runner yet, so its observer is skipped and the queued work can still start after the user cancels. Either notify observers before this early return, or registeronCancelonly once the run is actually active.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/session/run-state.ts` around lines 217 - 225, The cancel() implementation in SessionRunState currently returns early when there is no busy runner, skipping notifyCancelObservers and leaving queued ensureRunning() tasks without the new onCancel hook; update cancel(sessionID, meta) to ensure observers get notified even when no existing.busy runner exists (e.g., call data.notifyCancelObservers(sessionID, meta) before the early return) or alternatively change ensureRunning()/withActiveRun so onCancel is registered only after a runner is created; reference functions/objects: cancel, ensureRunning, withActiveRun, InstanceState.get(state), data.runners, and data.notifyCancelObservers to locate and apply the fix.packages/opencode/src/session/processor.ts (1)
913-939:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap this
switchcase in its own block.
const executionStarted = ...is declared directly undercase "tool-input-start":, so Biome'snoSwitchDeclarationsrule flags this as an error. If lint is gating CI, this change won't merge until the case is braced.Suggested fix
- case "tool-input-start": + case "tool-input-start": { if (ctx.assistantMessage.summary) { throw new Error(`Tool call not allowed while generating summary: ${value.toolName}`) } const part = yield* session.updatePart({ id: ctx.toolcalls[value.id]?.partID ?? PartID.ascending(), @@ } yield* applyPendingToolUpdates(value.id) return + }Static analysis already reports this as a Biome
noSwitchDeclarationserror.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/session/processor.ts` around lines 913 - 939, The switch case for "tool-input-start" declares variables (e.g., const executionStarted) directly under the case which triggers the Biome noSwitchDeclarations rule; fix it by wrapping the entire case body in its own block (add { ... } after case "tool-input-start":) so declarations like executionStarted, part, and the ctx.toolcalls assignment are scoped properly; ensure the existing calls to session.updatePart, ctx.pendingStartedToolCalls.delete, Deferred.make, and applyPendingToolUpdates(value.id) remain inside that new block.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/opencode/src/session/processor.ts`:
- Around line 913-939: The switch case for "tool-input-start" declares variables
(e.g., const executionStarted) directly under the case which triggers the Biome
noSwitchDeclarations rule; fix it by wrapping the entire case body in its own
block (add { ... } after case "tool-input-start":) so declarations like
executionStarted, part, and the ctx.toolcalls assignment are scoped properly;
ensure the existing calls to session.updatePart,
ctx.pendingStartedToolCalls.delete, Deferred.make, and
applyPendingToolUpdates(value.id) remain inside that new block.
In `@packages/opencode/src/session/run-state.ts`:
- Around line 217-225: The cancel() implementation in SessionRunState currently
returns early when there is no busy runner, skipping notifyCancelObservers and
leaving queued ensureRunning() tasks without the new onCancel hook; update
cancel(sessionID, meta) to ensure observers get notified even when no
existing.busy runner exists (e.g., call data.notifyCancelObservers(sessionID,
meta) before the early return) or alternatively change
ensureRunning()/withActiveRun so onCancel is registered only after a runner is
created; reference functions/objects: cancel, ensureRunning, withActiveRun,
InstanceState.get(state), data.runners, and data.notifyCancelObservers to locate
and apply the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 882645fc-04dd-4d16-a884-ad6e677ad5c8
📒 Files selected for processing (6)
packages/opencode/src/session/llm.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/run-state.tspackages/opencode/test/session/compaction.test.tspackages/opencode/test/session/processor-effect.test.ts
Provider-side ContextOverflowError handling now respects compaction.auto: false for normal assistant turns instead of setting needsCompaction and silently compacting. This imports the Line B #30749 runtime correctness fix while keeping the scope isolated from #927/tool-drain/recovery work and from PR #1252, whose processor.ts collision was owner-accepted as rebase-later. Changes: - Stop normal assistant provider overflow with the provider ContextOverflowError when auto compaction is disabled. - Preserve summary/compaction assistant overflow behavior so SessionCompaction still writes its explicit compact failure error. - Add focused coverage for disabled-auto provider overflow and the summary-compaction boundary. Verification: - RED: bun --cwd packages/opencode test test/session/processor-effect.test.ts -t "stops structured context overflow when auto compaction is disabled" failed because the processor returned "compact". - bun --cwd packages/opencode test test/session/processor-effect.test.ts -t "stop structured context overflow when auto compaction is disabled" -> pass. - bun --cwd packages/opencode test test/session/processor-effect.test.ts -t "compact on structured context overflow" -> pass. - bun --cwd packages/opencode test test/session/compaction.test.ts -t "marks summary message as errored on compact result when auto compaction is disabled" -> pass. - bun --cwd packages/opencode test test/session/processor-effect.test.ts -> 36 pass. - bun --cwd packages/opencode test test/session/compaction.test.ts -> 51 pass. - bun run --cwd packages/opencode typecheck -> pass. - git diff --check -> pass. - Fresh-eye reviewer rerun after semantic diff change -> no issues found. - CI run 27288199573 -> success. - windows-advisory run 27288199605 -> success.
Root cause / goal: SessionPrompt persists an assistant scaffold before processor handling fully owns the turn. Cancellation in that early window could leave an empty unfinished assistant message with no abort error or completion timestamp, poisoning later session history. This ports the focused upstream anomalyco/opencode#27254 behavior only. Change boundary: - Finalize the current unfinished assistant scaffold from the shared prompt-loop interrupt cleanup path when cancellation happens before processor cleanup owns the assistant. - Preserve existing cancel semantics and diagnostics, and avoid importing #1252 tool-drain behavior, #30804 filtering, provider/MCP/UI/package changes, or adjacent refactors. - Add regression coverage for cancellation after assistant scaffold persistence and after processor handle creation but before process starts. Verification: - RED proof: new regression first failed because the canceled scaffold had no MessageAbortedError. - Focused regression: bun test --timeout 30000 ./test/session/prompt-effect.test.ts -t "cancel after assistant scaffold save finalizes before processor handle|cancel after processor handle creation finalizes before process starts" -> 2 passed. - Prompt interruption suite: bun test --timeout 30000 ./test/session/prompt-effect.test.ts -> 65 passed. - Processor abort slice: bun test --timeout 30000 ./test/session/processor-effect.test.ts -t "abort|interrupt" -> 6 passed. - Typecheck: bun run typecheck in packages/opencode -> passed. - Whitespace: git diff --check origin/dev...HEAD -> passed. - PR CI: required checks green; full current check set including windows-advisory green. Reviews: - Terminal Claude review: PASS after final diff. - Independent terminal Occam fresh-eye review: PASS, no P0/P1/P2 after addressing the post-handle/pre-process cancellation window. - GitHub review threads: none unresolved. Residual risk: Canceled assistants still follow existing semantics and do not set finish; this PR only ensures aborted scaffolds carry MessageAbortedError and time.completed.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/session/processor.ts (1)
1056-1092:⚠️ Potential issue | 🟠 MajorWrap the
tool-input-startswitch case in braces
case "tool-input-start":declaresconstbindings (lifecycle,part,current) directly in the case without a block, which trips Biome’snoSwitchDeclarationspattern and can create shared TDZ/scope issues across cases. Wrap the case in{ ... }.🧩 Minimal fix
- case "tool-input-start": + case "tool-input-start": { if (ctx.assistantMessage.summary) { throw new Error(`Tool call not allowed while generating summary: ${value.toolName}`) } const lifecycle = yield* getToolLifecycleRecord(value.id) if (lifecycle.settled) return const part = yield* session.updatePart({ id: lifecycle.partID ?? PartID.ascending(), messageID: ctx.assistantMessage.id, sessionID: ctx.assistantMessage.sessionID, type: "tool", tool: value.toolName, callID: value.id, state: { status: "pending", input: {}, raw: "" }, metadata: value.providerExecuted ? { providerExecuted: true } : undefined, } satisfies MessageV2.ToolPart) ctx.toolcalls[value.id] = { ...lifecycle, partID: part.id, messageID: part.messageID, sessionID: part.sessionID, attemptID, } const current = ctx.toolcalls[value.id] if (current.executionStarted && current.startedToolName && !current.executionStartedRecorded) { current.executionStartedRecorded = true ctx.runTrace.recordToolExecutionStarted({ attemptID, at: Date.now(), monotonicMs: performance.now(), toolName: RunObservability.safeToolName(current.startedToolName), effect: RunObservability.toolEffect(current.startedToolName), }) } yield* Deferred.succeed(current.partCreated, undefined).pipe(Effect.ignore) yield* applyPendingToolUpdates(value.id) return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/session/processor.ts` around lines 1056 - 1092, The switch case for "tool-input-start" declares block-scoped consts (lifecycle, part, current) without its own block which violates noSwitchDeclarations; wrap the entire case body in braces { ... } so those const bindings are scoped to this case, preserving the existing flow/returns and ensuring you still call getToolLifecycleRecord, session.updatePart, set ctx.toolcalls[value.id], record executionStarted with ctx.runTrace, Deferred.succeed(...), and applyPendingToolUpdates(value.id) as before.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/session/processor.ts`:
- Around line 413-425: getToolLifecycleRecord currently creates new Deferreds
even after cleanup() clears ctx.toolcalls, causing callers like
completeToolExecution()/failToolExecution() and LLM.wrapToolsWithLifecycle() to
wait forever; change getToolLifecycleRecord to detect teardown (e.g.,
ctx.toolcalls missing or a ctx.draining/teardown flag set by cleanup) and return
a pre-resolved/pre-rejected lifecycle record whose partCreated/ready/done
deferreds are already completed so downstream awaits never block after teardown;
update cleanup() to set the teardown flag (or clear ctx.toolcalls) consistently
and ensure completeToolExecution()/failToolExecution() will work with the
pre-resolved record.
---
Outside diff comments:
In `@packages/opencode/src/session/processor.ts`:
- Around line 1056-1092: The switch case for "tool-input-start" declares
block-scoped consts (lifecycle, part, current) without its own block which
violates noSwitchDeclarations; wrap the entire case body in braces { ... } so
those const bindings are scoped to this case, preserving the existing
flow/returns and ensuring you still call getToolLifecycleRecord,
session.updatePart, set ctx.toolcalls[value.id], record executionStarted with
ctx.runTrace, Deferred.succeed(...), and applyPendingToolUpdates(value.id) as
before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cd72894e-8aed-4e57-a1e8-a0ce7703b562
📒 Files selected for processing (10)
packages/opencode/src/session/llm.tspackages/opencode/src/session/message-v2.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/prompt.tspackages/opencode/src/session/run-state.tspackages/opencode/test/permission-agent.test.tspackages/opencode/test/session/compaction.test.tspackages/opencode/test/session/llm.test.tspackages/opencode/test/session/processor-effect.test.tspackages/opencode/test/session/prompt-effect.test.ts
💤 Files with no reviewable changes (1)
- packages/opencode/test/permission-agent.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/session/run-state.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/session/processor.ts (1)
1071-1108:⚠️ Potential issue | 🔴 CriticalWrap the
casebody in braces to fix the BiomenoSwitchDeclarationserror.Line 1075 declares
const lifecycledirectly in an unbracedswitchcase, which violates Biome'slint/correctness/noSwitchDeclarationsrule (enforced as error). Wrap the case body in braces to scope the declaration correctly.Proposed fix
- case "tool-input-start": + case "tool-input-start": { if (ctx.assistantMessage.summary) { throw new Error(`Tool call not allowed while generating summary: ${value.toolName}`) } const lifecycle = yield* getToolLifecycleRecord(value.id, attemptID) if (!lifecycle) return @@ yield* Deferred.succeed(current.partCreated, undefined).pipe(Effect.ignore) yield* applyPendingToolUpdates(value.id) return + } case "tool-input-delta": return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/session/processor.ts` around lines 1071 - 1108, The case "tool-input-start" block violates Biome's noSwitchDeclarations rule because variable declarations like const lifecycle are made directly in the unbraced case without proper scoping. Wrap the entire body of the case "tool-input-start" statement in curly braces to create a proper scope for these declarations and resolve the linting error.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/opencode/src/session/processor.ts`:
- Around line 1071-1108: The case "tool-input-start" block violates Biome's
noSwitchDeclarations rule because variable declarations like const lifecycle are
made directly in the unbraced case without proper scoping. Wrap the entire body
of the case "tool-input-start" statement in curly braces to create a proper
scope for these declarations and resolve the linting error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 34df687b-e889-40b1-8b12-8aca27c18d68
📒 Files selected for processing (5)
packages/opencode/src/session/processor.tspackages/opencode/src/session/run-state.tspackages/opencode/test/session/message-v2.test.tspackages/opencode/test/session/processor-effect.test.tspackages/opencode/test/session/prompt-effect.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/src/session/run-state.ts
- packages/opencode/test/session/processor-effect.test.ts
1 participant
Summary
tool-resultortool-errorevent.Why
A provider stream timeout or transport disconnect after tool execution starts could abort the tool's local signal and let cleanup mark the tool part as interrupted before the completed result was persisted. PR1 for #927 needs the tool execution/result path to survive stream lifecycle failure without adding recovery UI or replay behavior.
Related Issue
Part of #927.
Human Review Status
Pending
Review Focus
Please focus on the tool abort signal split, the idempotent lifecycle callbacks, the
tool-callreadiness gate before terminal lifecycle writes, the bounded cleanup drain for started tools after stream failure, and the run-state cancel observer that aborts tools even while cleanup is draining.Risk Notes
Runtime behavior changes for session tool execution on both macOS and Windows, but no OS-specific paths or packaging surfaces changed. The main behavior tradeoff is intentional: after a non-interrupt stream failure, started tools are allowed to finish up to the recovery drain timeout instead of being forced into the old one-second cleanup path; genuinely dead tools are still finalized instead of hanging the session indefinitely, and user cancel/lifecycle close can abort the drain directly.
Fresh-eye and Claude review found races around fast tool completion, abort-aware tool failure, lifecycle-close drain semantics, unbounded drain, drain-time cancellation, started-but-not-materialized tool calls, leftover Deferred cleanup, and early failure persistence. This PR includes follow-up commits covering those with the readiness gate, abort-aware failure guard, lifecycle-close abort, bounded drain, session cancel observer, remaining Deferred cleanup, pending-start reconciliation, and regression tests.
Skipped conditional checklist items: screenshots are not applicable because there are no visible UI or copy changes; docs/release/dependencies are not applicable because none were touched.
Migration note: this PR intentionally keeps the recovery fix in the legacy
packages/opencodeprocessor/LLM bridge. The behavior aligns with the upstream Core v2 runtime direction: local tool execution is tracked independently from provider stream events, tool settlements are drained before continuation or recovery decisions, and unresolved tools are finalized instead of being left pending. If PawWork later ports the upstreampackages/coresession runner and unified tool runtime, the right migration path is to carry these regression scenarios into the Core runner and delete this bridge, not grow it into a parallel runtime.How To Verify
Screenshots or Recordings
Not applicable. No visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override if wrong, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Bug Fixes