Fix/opencode stop restore - #8819
Conversation
interruptTurn previously only called session.abort and emitted turn.aborted, but never cleared activeTurnId or set the provider session back to ready. The provider session therefore stayed "running" with the interrupted turnId, leaving the UI stuck and the underlying promptAsync unusable. Observed snapshot after pressing Stop: - latestTurn.state="interrupted" but session.status="running" with same activeTurnId (62f30810-649e-440e-b72c-a0dbae77e26f) - UI shows running spinner, next turn cannot be sent, model continues in background. Fix: make abort best-effort (log warning instead of failing the interrupt), emit turn.aborted with the resolved targetTurnId, then clear activeTurnId/agent/variant and update the provider session to "ready". Fixes Stop button for opencode/muse-spark and other OpenCode models on Desktop 0.0.37-nightly.20260830.1227.
- Capture activeTurnId before first yield and clear only when it still matches the snapshot, preventing a newer turn started while session.abort was awaiting from being incorrectly cleared. - Use Effect.catch (not catchAll) per codebase convention for intentionally handling the whole error channel. - Add focused tests: interrupt emits turn.aborted and returns to ready; abort failure still clears active turn and leaves session ready.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟠 High Layers/OpenCodeAdapter.ts:1601
When a steer reuses activeTurnId while session.abort is awaiting, this guard still matches snapshotActiveTurnId, so interruptTurn emits turn.aborted and sets the session to ready even though the newer prompt may still be executing remotely. Subsequent input is then treated as a new turn while OpenCode is busy; use a generation/token or other synchronization that distinguishes the specific in-flight turn from a reused steer.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OpenCodeAdapter.ts around line 1601:
When a steer reuses `activeTurnId` while `session.abort` is awaiting, this guard still matches `snapshotActiveTurnId`, so `interruptTurn` emits `turn.aborted` and sets the session to `ready` even though the newer prompt may still be executing remotely. Subsequent input is then treated as a new turn while OpenCode is busy; use a generation/token or other synchronization that distinguishes the specific in-flight turn from a reused steer.
There was a problem hiding this comment.
Effect service conventions review of the OpenCode adapter change. Two findings in apps/server/src/provider/Layers/OpenCodeAdapter.ts: an unbounded raw cause in the new best-effort abort log payload, and dead snapshot locals kept alive only by void statements. The added tests and the snapshot/compare-and-clear logic otherwise look consistent with repo conventions.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Logging the raw cause puts an unbounded wire payload in the log record: runOpenCodeSdk stores the SDK v2 rejection ({ request, response, error }) as cause, so this can serialize response bodies and request headers — including the Authorization: Basic opencode:<serverPassword> header used for external servers. Consider annotating only bounded, structural fields (the file's typed error already carries operation, and errorTag is the pattern used elsewhere in this directory, e.g. ProviderSessionReaper/CursorProvider).
| Effect.catch((error) => | |
| Effect.logWarning("OpenCode session.abort failed during interrupt", { | |
| errorTag: error._tag, | |
| operation: error.operation, | |
| sessionID: context.openCodeSessionId, | |
| }), |
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
snapshotActiveAgent / snapshotActiveVariant are never read — the two void statements exist only to silence the unused-variable check, and the clear below resets context.activeAgent/activeVariant directly. Suggest dropping the dead locals (keep snapshotActiveTurnId, which the guard does use).
Posted via Macroscope — Effect Service Conventions
| // Best-effort abort: even if the remote abort fails we still want to | ||
| // clear the local active-turn state so the session does not stay | ||
| // stuck in "running". | ||
| yield* runOpenCodeSdk("session.abort", () => |
There was a problem hiding this comment.
🟡 Medium Layers/OpenCodeAdapter.ts:2911
Concurrent interruptTurn calls for the same active turn each invoke session.abort and emit turn.aborted, so one user interruption produces duplicate remote aborts and duplicate terminal lifecycle events. Because this implementation snapshots and awaits before marking the turn as interrupted, later callers cannot observe that the first cancellation is in flight; restore a per-turn coalescing guard (for example, have subsequent callers await the existing context.cancellation.completion).
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OpenCodeAdapter.ts around line 2911:
Concurrent `interruptTurn` calls for the same active turn each invoke `session.abort` and emit `turn.aborted`, so one user interruption produces duplicate remote aborts and duplicate terminal lifecycle events. Because this implementation snapshots and awaits before marking the turn as interrupted, later callers cannot observe that the first cancellation is in flight; restore a per-turn coalescing guard (for example, have subsequent callers await the existing `context.cancellation.completion`).
| // Best-effort abort: even if the remote abort fails we still want to | ||
| // clear the local active-turn state so the session does not stay | ||
| // stuck in "running". | ||
| yield* runOpenCodeSdk("session.abort", () => |
There was a problem hiding this comment.
🟠 High Layers/OpenCodeAdapter.ts:2911
interruptTurn can remain suspended forever when session.abort never settles, so the subsequent turn.aborted emission and transition to ready never run and Stop hangs. Add the 10-second timeout that bounded this request before the best-effort error handler.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OpenCodeAdapter.ts around line 2911:
`interruptTurn` can remain suspended forever when `session.abort` never settles, so the subsequent `turn.aborted` emission and transition to `ready` never run and Stop hangs. Add the 10-second timeout that bounded this request before the best-effort error handler.
| const snapshotActiveVariant = context.activeVariant; | ||
| void snapshotActiveAgent; | ||
| void snapshotActiveVariant; | ||
| const targetTurnId = turnId ?? snapshotActiveTurnId; |
There was a problem hiding this comment.
🟠 High Layers/OpenCodeAdapter.ts:2907
A delayed interruptTurn for a stale turnId aborts the currently active OpenCode session and emits turn.aborted for the old turn, leaving the new turn running remotely while T3 reports the session as ready. Check that a supplied turnId matches context.activeTurnId before calling session.abort.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/OpenCodeAdapter.ts around line 2907:
A delayed `interruptTurn` for a stale `turnId` aborts the currently active OpenCode session and emits `turn.aborted` for the old turn, leaving the new turn running remotely while T3 reports the session as ready. Check that a supplied `turnId` matches `context.activeTurnId` before calling `session.abort`.
There was a problem hiding this comment.
Effect Service Conventions
Findings in apps/server/src/provider/Layers/OpenCodeAdapter.ts and its test double. The rewritten interruptTurn drops Effect interruption/timeout plumbing that every sibling session.abort call site keeps, leaves dead placeholder statements, and orphans the session cancellation state machine. The test double changes also remove state and stubs that other tests in the same file still use.
Details inline.
Posted via Macroscope — Effect Service Conventions
| const snapshotActiveAgent = context.activeAgent; | ||
| const snapshotActiveVariant = context.activeVariant; | ||
| void snapshotActiveAgent; | ||
| void snapshotActiveVariant; |
There was a problem hiding this comment.
These two snapshots are never read — they are only discarded with void. Consider dropping the dead declarations (the agent/variant fields are already cleared below under the snapshotActiveTurnId guard).
| const snapshotActiveAgent = context.activeAgent; | |
| const snapshotActiveVariant = context.activeVariant; | |
| void snapshotActiveAgent; | |
| void snapshotActiveVariant; |
Posted via Macroscope — Effect Service Conventions
| yield* runOpenCodeSdk("session.abort", () => | ||
| context.client.session.abort({ sessionID: context.openCodeSessionId }), | ||
| ).pipe( |
There was a problem hiding this comment.
This call no longer forwards the AbortSignal and no longer bounds the request, unlike every other session.abort call site in this module (lines 702, 1116, 2751), so fiber interruption can't cancel the in-flight SDK request and interruptTurn can await indefinitely. Suggest restoring the signal and the 10s bound (Effect.catch below will also cover the TimeoutError).
| yield* runOpenCodeSdk("session.abort", () => | |
| context.client.session.abort({ sessionID: context.openCodeSessionId }), | |
| ).pipe( | |
| yield* runOpenCodeSdk("session.abort", (signal) => | |
| context.client.session.abort({ sessionID: context.openCodeSessionId }, { signal }), | |
| ).pipe( | |
| Effect.timeout("10 seconds"), |
Posted via Macroscope — Effect Service Conventions
| if (runtimeMock.state.abortError) { | ||
| throw runtimeMock.state.abortError; | ||
| } | ||
| await runtimeMock.state.abortImplementation?.(sessionID, options?.signal); | ||
| }, | ||
| status: async () => { | ||
| runtimeMock.state.sessionStatusCalls += 1; | ||
| if (runtimeMock.state.sessionStatusImplementation) { | ||
| return await runtimeMock.state.sessionStatusImplementation(); | ||
| } | ||
| if (runtimeMock.state.sessionStatusFailures > 0) { | ||
| runtimeMock.state.sessionStatusFailures -= 1; | ||
| throw new Error("status failed"); | ||
| } | ||
| return { | ||
| data: | ||
| runtimeMock.state.sessionStatus === "idle" | ||
| ? {} | ||
| : { "http://127.0.0.1:9999/session": { type: "busy" as const } }, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
The session.status stub was deleted from the runtime test double while production code still calls client.session.status (OpenCodeAdapter.ts:1027, 1220) and ~25 tests in this file still configure sessionStatusImplementation / assert on sessionStatusCalls. The deleted regression test (does not let an old idle status complete a successful steer) also removes coverage for the stale-idle path rather than updating it. Suggest restoring the status stub and adapting the existing test instead of deleting it, and keeping options.signal captured here.
Posted via Macroscope — Effect Service Conventions
| if (targetTurnId) { | ||
| yield* emit({ | ||
| ...(yield* buildEventBase({ | ||
| threadId, | ||
| turnId: targetTurnId, | ||
| })), | ||
| type: "turn.aborted", | ||
| payload: { | ||
| reason: "Interrupted by user.", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (context.cancellation === cancellation) { | ||
| if (cancellation.turnId !== undefined) { | ||
| yield* interruptOpenCodeTurn(context, cancellation.turnId); | ||
| } else { | ||
| context.cancellation = undefined; | ||
| context.reconcileIdleStatus = true; | ||
| } | ||
| // Clear only if the active turn still matches the snapshot we | ||
| // interrupted — a newer turn may have started while abort was in | ||
| // flight and must not be cleared. | ||
| if (context.activeTurnId !== undefined && context.activeTurnId === snapshotActiveTurnId) { | ||
| context.activeTurnId = undefined; | ||
| context.activeAgent = undefined; | ||
| context.activeVariant = undefined; | ||
| yield* updateProviderSession(context, { status: "ready" }, { clearActiveTurnId: true }); | ||
| } |
There was a problem hiding this comment.
Removing the cancellation handshake here leaves a large block of now-unreachable machinery in this service: context.cancellation is only ever assigned undefined after this change, so failPendingOpenCodeCancellation (682), interruptOpenCodeTurn (1309) and its context.interruptedTurnId bookkeeping, and the idle-reconciliation branches keyed on cancellation?.turnId (2127, 2153-2173, 2594, 2679, 2832, 2864) can no longer run. Either finish the migration by deleting the orphaned fields/helpers, or keep interruptTurn participating in that state machine — leaving both halves makes the interrupt path's invariants unverifiable. The turnId argument is also no longer honored: a stale turnId now aborts the session and emits turn.aborted for a turn that isn't active.
Posted via Macroscope — Effect Service Conventions
| abortImplementation: null as | ||
| | ((sessionID: string, signal?: AbortSignal) => Promise<void>) | ||
| | null, | ||
| abortError: null as Error | null, |
There was a problem hiding this comment.
abortSignals and abortImplementation were removed from the mock state, but the same file still reads them at lines 597-603, 3498-3499 and 3697, so this file no longer type-checks. Suggest keeping those fields (and the options.signal capture in the abort stub) alongside the new abortError.
Posted via Macroscope — Effect Service Conventions
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 777a6f4. Configure here.
| method: "session.abort", | ||
| detail: "OpenCode session abort did not complete within 10 seconds.", | ||
| cause, | ||
| }), |
There was a problem hiding this comment.
Stop no longer marks session interrupted
High Severity
The new interruptTurn emits turn.aborted and may set ready, but it never sets cancellation, interruptedTurnId, or reconcileIdleStatus, and it does not cancel promptAdmission or pending idle reconciliation. Event handlers still use those flags to treat MessageAbortedError as a user stop, hold stale idle, and suppress leftover output. After Stop, the abort error is handled as a real failure, the session can flip to error, and a following turn can be completed or failed by leftover idle or abort events.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 777a6f4. Configure here.
| }; | ||
| context.cancellation = cancellation; | ||
| const promptAdmission = context.promptAdmission; | ||
| if (promptAdmission !== undefined && promptAdmission.turnId === interruptedTurnId) { |
There was a problem hiding this comment.
Hung abort leaves session running
High Severity
interruptTurn now awaits session.abort with no timeout and without passing the SDK AbortSignal. Local ready/clear logic runs only after that call returns. If the remote abort hangs, Stop never emits turn.aborted and never clears activeTurnId, so the session stays running.
Reviewed by Cursor Bugbot for commit 777a6f4. Configure here.
| if (options?.signal) { | ||
| runtimeMock.state.abortSignals.push(options.signal); | ||
| if (runtimeMock.state.abortError) { | ||
| throw runtimeMock.state.abortError; |
There was a problem hiding this comment.
Test mock dropped session.status
Medium Severity
While replacing abortImplementation with abortError, the mock session.status method was removed. Adapter code and many tests still call session.status and assign sessionStatusImplementation, so idle-reconciliation and steer-admission tests now throw or hang instead of exercising those paths.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 777a6f4. Configure here.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — The production adapter rewrites OpenCode Stop semantics across abort handling, turn identity, cancellation state, and session readiness, with the abort request no longer bounded or signal-aware. Unresolved lifecycle, sensitive logging, and test-harness concerns leave material runtime and verification risk. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
1 participant
Note
Medium Risk
Changes core turn-interrupt and session status semantics for OpenCode; behavior now favors local recovery over surfacing abort failures, which may diverge from tests or callers that expected
interruptTurnto fail whensession.abortfails.Overview
OpenCode
interruptTurnis rewritten so Stop reliably unblocks the UI even when the remote abort fails or races with a new turn.The adapter snapshots
activeTurnIdbefore awaitingsession.abort, then calls abort best-effort (errors are logged, not returned). It always emitsturn.abortedfor the interrupted turn and sets the session toreadyonly if the active turn still matches that snapshot, so a turn started during abort is not cleared.The previous path—cancellation deferrals, racing abort against idle events, 10s timeouts with
AbortSignal, and failing the interrupt when abort errors—is removed from this flow.Tests add coverage for successful interrupt (including a follow-up
sendTurn) and for abort throwing while local state still clears; the runtime test double gainsabortErrorinstead of the older abort signal/implementation hooks used in the diff’s mock changes.Reviewed by Cursor Bugbot for commit 777a6f4. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Simplify
interruptTurnto best-effort abort and always restore session to readymakeOpenCodeAdapter.interruptTurnwith a best-effort flow: snapshot the active turn, callsession.abort, emitturn.aborted, and clear local state toreadyregardless of whether abort succeeds.turnIddoes not match the currentactiveTurnId; it now derivestargetTurnIdand conditionally clears state only if the active turn still matches the snapshot.OpenCodeRuntimeTestDoubletest client to use a singleabortErrorfield instead ofAbortSignal/abortImplementationtracking, and removes thesession.status()method.turn.abortedand restoring to ready; interrupt clearing state even when abort throws; passing agent/variant options for a custom-bound provider instance id.interruptTurnno longer fails with timeouts or propagates abort errors, and callers that relied on a mismatchedturnIdcausing an early return will now see an event emitted and state cleared.📊 Macroscope summarized d130560. 1 file reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues