Skip to content

fix(server): settle a turn when its provider session closes - #16181

Closed
Adamulek123 wants to merge 2 commits into
pingdotgg:mainfrom
Adamulek123:fix/v2-adapter-terminal-on-session-close
Closed

Adamulek123 wants to merge 2 commits into
pingdotgg:mainfrom
Adamulek123:fix/v2-adapter-terminal-on-session-close

Conversation

@Adamulek123

@Adamulek123 Adamulek123 commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Problem

When a provider session is closed while a turn is still live, the run is left unsettled. Closing a session emitted nothing at all from three of the four V2 adapters, and Pi had no code path that emits turn.terminal during teardown. The turn stays open in the UI indefinitely, with no terminal status and no way to close it out.

This is the adapter-side closure of the gap named in #15197 ("provider returns success without emitting turn.terminal"). #15298 owns the Stop half; this does not depend on it, because RunExecutionService already guards on rootRunFinalized, so a late terminal cannot settle a run twice.

Change

Each of the four V2 adapters now emits a terminal event when its session closes with a live turn, so the run reaches a durable terminal state instead of hanging:

  • CursorAdapterV2 -- emits for the open turn.
  • OpenCodeAdapterV2 -- emits for the open turn.
  • OpenCode2AdapterV2 -- emits for the open turn.
  • PiAdapterV2 -- adds the teardown emission it previously lacked.

Terminal-on-close was deliberately split out of #15767. That PR reduces whole-text emissions while coalescing streamed deltas, and a session-close terminal is a lifecycle decision rather than a coalescing one. #15767 keeps only the part that is the coalescer's: a session that closes mid-response still projects what it buffered, but does not settle.

OpenCodeAdapterV2's message-cache deletion is also not here. It belongs to #15760, and keeping it out avoids a conflict.

Scope and approval

Follow-up to the adapter audit of #2829, and a direct implementation of the adapter-side gap in #15197. This is one underlying problem -- "a closed provider session leaves its turn unsettled" -- across the four adapters that share the teardown path.

Verification

Focused adapter tests, run in apps/server:

  • vp test run over CursorAdapterV2.test.ts, OpenCodeAdapterV2.test.ts, OpenCode2AdapterV2.test.ts, PiAdapterV2.test.ts -- 193 passed, 0 failed.
  • vp run typecheck in apps/server -- exit 0.
  • vp lint --report-unused-disable-directives apps/server/src/orchestration-v2/Adapters -- exit 0 (existing warnings only, none in changed lines).

Each adapter gained coverage asserting that closing a session with a live turn produces exactly one terminal event, and that it does not double-settle.

Not checked: end-to-end behavior against live provider CLIs, and multi-device relay.

Model and harness: opencode/space-bunny-free in T3 Code (OpenCode harness), on top of the #2829 adapter audit.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 5, 2026
Closing a session with a live turn left the run unsettled: three of the
four adapters emitted nothing at all, and Pi had no code path that emits
turn.terminal during teardown. This is the adapter-side closure of the
gap named in pingdotgg#15197, where a provider "returns success without emitting
turn.terminal". pingdotgg#15298 owns the Stop half; this does not depend on it —
RunExecutionService already guards on rootRunFinalized, so a late
terminal cannot settle a run twice.

OpenCodeAdapterV2's message-cache deletion is deliberately NOT here; it
belongs to the reclamation branch and would otherwise collide.
@Adamulek123
Adamulek123 force-pushed the fix/v2-adapter-terminal-on-session-close branch from f4843fa to 1e39859 Compare October 6, 2026 22:21
@Adamulek123

Copy link
Copy Markdown
Contributor Author

Closing this draft: the adapter teardown terminals do not reach managed run subscribers, so it does not deliver the durable settlement promised in its title and description.

The blocking reason. ProviderSessionManager ends those subscriptions before it closes the adapter scope. On the release path it calls endSubscribers / closeSubscribers / failSubscribers, and each does Ref.getAndSet(entry.eventSubscribers, new Map()), emptying the map before Scope.close invokes the adapter finalizers this PR adds. The event pump publishes through that now-empty map. On shutdown, closeSubscribers additionally clears the queues, and RunExecutionService returns early on a null terminal. No production path delivers these terminals to the runs they promise to settle; the adapter tests pass only because they open raw runtimes with consumers outside the session scope.

I confirmed this ordering is pre-existing on main, not introduced here.

Two adapter-level defects found in review, kept here for whoever picks this up:

  • Pi teardown suppresses a recorded failure. finalizeTurn selects const failure = cancelled || turn.interrupted ? null : turn.failure; and teardown always passes cancelled = true, but Pi assigns turn.failure at message_end and on exhausted retry while the turn is still active. Closing in that window reports a genuinely failed turn as cancelled.
  • Cursor finalizes before draining the SDK callback chain. Normal completion awaits it; close does not, and handlers early-return once the context is finalized, so a queued text delta would be discarded. This is a mechanism I confirmed by reading, not a reproduced failure.

The tests do not prove what the body claims. Every adapter test collects via Stream.takeUntil(... "turn.terminal") or a first-match helper, so all of them stop at the first terminal. They demonstrate first-terminal emission and status. They do not demonstrate uniqueness, durable settlement, or managed delivery.

Overlap is heavier than I originally described. #15307 was closed and replaced by #15776 for stream-end settlement. #15298 was closed as superseded by merged #15442, which handles Stop recovery via settleInterruptedRun. #15571 has merged for interrupted-start cleanup. #15778 replaces #15308 for request retirement. Together these cover the settlement exits this draft was reaching for, so I am not filing the subscriber-ordering observation as a standalone bug either: the shutdown suppression is deliberate, with an existing comment stating that restart recovery owns that state, and changing it needs a shutdown-policy decision rather than a patch.

Open question for maintainers, since I would rather ask than guess: is an observable terminal-on-session-close contract wanted beyond #15776 and #15442? If yes, the right starting point is a failing managed-session-to-SQLite regression plus an explicit decision about shutdown and restart semantics, not another four-adapter emission patch.

Findings preserved in the audit notes and in issue #15197, which remains open.

@Adamulek123

Copy link
Copy Markdown
Contributor Author

Closed per the review above. Reopen if a maintainer wants the terminal-on-session-close contract pursued with a managed-session persistence regression.

@Adamulek123 Adamulek123 closed this Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant