Skip to content

fix(orchestration-v2): hold provider session ownership until cleanup finishes - #11492

Closed
saphid wants to merge 12 commits into
pingdotgg:t3code/codex-turn-mappingfrom
saphid:work/ov2-20260913-01
Closed

saphid wants to merge 12 commits into
pingdotgg:t3code/codex-turn-mappingfrom
saphid:work/ov2-20260913-01

Conversation

@saphid

@saphid saphid commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Ports #9806 onto t3code/codex-turn-mapping.

A V2 provider session removed its session-map entry before scope cleanup finished, so a replacement open could start a new process while the old one still owned credentials and resources — and a hung cleanup silently unblocked reuse.

  • Releasing sessions keep a pending-release record until cleanup actually completes; replacement opens and runtime thread attachments (ensureThread/resumeThread/forkThread/startTurn) for its recorded threads are blocked.
  • Repeated close/detach/closeInstance calls join the same cleanup Deferred rather than racing it; a detach that loses its session to a concurrent close joins the pending cleanup instead of reporting success over still-running work.
  • Shared MCP credentials are revoked only when no live session, reservation, or pending release still holds them.
  • The 30s wait reports an honest error projection state without claiming the process stopped; successful cleanup clears the block, failed cleanup keeps blocking until restart.
  • Layer shutdown still only joins live sessions, so a hung pending finalizer cannot stall teardown.

Post-review hardening vs. #9806: the runtime attachment gate runs outside observeActivity (which swallows bookkeeping failures) and outside startTurn's busy/idle catch (a rejected attach must not decrement the peer's busy count).

Test plan

  • Restack onto 9fcc9a4bdb: vp test run apps/server/src/orchestration-v2/ProviderSessionManager.test.ts apps/server/src/provider/Layers/ProviderAuthService.test.ts --maxWorkers=1 — 169 tests passed. Auth tests preserve upstream shared-binding replacement/exclusion cases and verify a failed peer close prevents invalidation and logout.
  • vp test run apps/server/src/provider/Layers/ProviderAuthService.test.ts apps/server/src/orchestration-v2/ProviderSessionManager.test.ts apps/server/src/orchestration-v2/Adapters/ClaudeAdapterV2.test.ts apps/server/src/orchestration-v2/Adapters/CursorAdapterV2.test.ts apps/server/src/orchestration-v2/Adapters/OpenCodeAdapterV2.test.ts apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts apps/server/src/mcp/OrchestratorMcpToolkit.integration.test.ts apps/server/src/orchestration-v2/testkit/OrchestratorReplayRecovery.integration.test.ts apps/server/src/serverRuntimeStartup.test.ts --maxWorkers=2 — 472 tests passed and the original scheduler sweep timed out; all seven adjacent suites passed (307 tests). The revised core-suite result is above.
  • pnpm run typecheck in apps/server passed; targeted formatting check passed.
  • Verification caveat: the 520-position attachment scheduler sweep hit the existing 60-second test limit under heavy host load, including an isolated retry. It is now four independent 130-position cases: all 520 positions and assertions remain, and no timeout was changed. The updated lifecycle/auth run passed 169 tests; the seven adjacent files passed 307 tests on the new base. SWE-2 Max independently reviewed the batching change with no actionable findings.
  • Extra bot round: detach preserves an existing ProviderSessionReleaseError instead of overwriting its reason. The stalled-attach regression reproduced before the fix; the three focused detach timeout cases pass afterward.
  • Historical reproduction against the original base: 3 replacement-blocking tests failed without the fix (prior implementation evidence).

SWE-2 (Devin) via T3 Code / Cursor harness.
Coordination trace: T3 thread 063442b9-45e8-4f0e-bc47-edea523ce7d0; campaign saphid/t3code-personal#298; original implementation #9806.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Sep 13, 2026
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The PR substantially changes production provider-session ownership, concurrency, cleanup, credential revocation, authentication flows, and server shutdown behavior across multiple components. Its size and sensitive auth/credential lifecycle impact exceed the scope of changes suitable for automatic approval.

Not approved because:

  • Per-PR cost limit exceeded (workspace setting). Approvability relies on correctness review in order to determine eligibility

Review your spending limits in Billing settings, or comment @macroscope-app review this PR to bypass the limit and review now. You can add or adjust custom eligibility rules. Learn more.

@saphid

saphid commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Note on the three red checks — all reproduce on the base branch (0d606c8a2c) without this diff and are unrelated to the change:

  • Check (knip:check): flags apps/server/scripts/verify-background-live.ts as an unused file — introduced by b929c4702b on the base. Reproduces locally.
  • Test: 5 failures in apps/mobile/src/state/thread-order.test.ts ("shared mobile pending move" suite). Same 5 failures reproduce locally on the base; this diff touches only server orchestration + docs.
  • Release Smoke: ERR_PNPM_UNUSED_PATCH — expo-audio@57.0.4 patch no longer matches any dependency in the base's lockfile.

Checks covering this change are green: Test Server 1–3, Rust, CodeRabbit.

@saphid
saphid force-pushed the work/ov2-20260913-01 branch from a38ea46 to 1310472 Compare September 13, 2026 01:31
@saphid
saphid force-pushed the work/ov2-20260913-01 branch from 1310472 to b3e7733 Compare September 13, 2026 13:18
@github-actions github-actions Bot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Sep 13, 2026
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
@cursor

cursor Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

1 similar comment
@cursor

cursor Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch from a8cc38b to 834edb9 Compare September 14, 2026 21:15
@macroscopeapp

macroscopeapp Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Macroscope skipped reviewing this pull request. Per-PR cost limit exceeded (workspace setting).

Reviews on this PR have cost $44.13 so far. This review would add an estimated $9.20, bringing the total to $53.33 — above your per-PR limit of $50.00.

Tip

To get this pull request reviewed, you can:

  1. Comment @macroscope-app on this PR to request a manual review (monthly spend limits still apply).
  2. Exclude large or generated files from review by adding a pattern to your .macroscope/ignore.md — note that creating this file replaces Macroscope's built-in default ignores rather than extending them.
  3. Raise your cost limit in your workspace billing settings.

Turn off this reminder going forward

@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch from 600a8a5 to d8c75ec Compare September 15, 2026 00:23
@saphid
saphid force-pushed the work/ov2-20260913-01 branch from 2b49d24 to 3b5f8cb Compare September 15, 2026 01:16
@saphid

saphid commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Worker 01 — provider-session cleanup ownership (rebased 8e1d53834e)

Eighth base rewrite absorbed (08a1b86b652a → f08294a11c42): all 34 commits replayed, zero conflicts, diff still exactly 4 files (+ 1 disclosed empty retrigger commit — see below). All campaign content byte-identical to the verified head.

  • Focused tests on the replayed tree: 128/128; OrchestratorReplayFixtures.integration.test.ts 73/73 locally.
  • CI on 8e1d53834e: all 18 check-runs complete — 12 success / 6 skipped / 0 failures.
  • One flake handled: Test Server 3 failed on 6b37937b9e at a nondeterministic replay-gate step (queued_cancelled_while_active/codex, turn/completed:reached=false). The file passes 73/73 locally on the exact head; the base only touched unrelated adapter files. No upstream rerun rights, so 8e1d53834e is a disclosed empty commit that re-ran the shard green.
  • Review threads: 12 total / 0 unresolved; no new feedback.
  • Independent cross-provider review of the exact head remains skipped for unavailable Codex capacity (weekly 0% until Sep 19); serial review queue owns the post-babysitting pass.

@juliusmarminge juliusmarminge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reopened so the smaller version can land on this PR. See my comment above for the requested shape: a releasing map of deferreds that open/ensureThread/resumeThread await and repeated close/detach join, keep the existing 30s bound, drop the opening-record/revocation-chain/in-band-drain machinery and the serverRuntimeStartup.ts worker changes (separate PR if still needed), and aim for under ~300 lines including two focused tests. Please force-push the rewrite rather than stacking on the current head.

@juliusmarminge juliusmarminge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reopened so the smaller version can land on this PR. See my comment above for the requested shape: a releasing map of deferreds that open/ensureThread/resumeThread await and repeated close/detach join, keep the existing 30s bound, drop the opening-record/revocation-chain/in-band-drain machinery and the serverRuntimeStartup.ts worker changes (separate PR if still needed), and aim for under ~300 lines including two focused tests. Please force-push the rewrite rather than stacking on the current head.

@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch 10 times, most recently from fe4f6ad to 87c67bd Compare September 25, 2026 05:55
@saphid
saphid force-pushed the work/ov2-20260913-01 branch from 668a26f to ff93972 Compare September 27, 2026 00:39
github-actions Bot and others added 11 commits September 27, 2026 10:59
Preserve the complete PR-owned snapshot at 6b22b7f5cee379310267c716b830db0ce484909b while removing historical upstream merge ancestry. Original PR: pingdotgg#11492. The nine later follow-up commits are replayed separately.
…up failures

A terminal detach could win the reacquire race between attachThreadOrReject
releasing the thread-attach lock and the resource-creating adapter call
reacquiring it, stripping the attachment and its credential claim before the
op was admitted. The lock now covers bookkeeping and the adapter call in one
hold, and markBusy moves before the attach to avoid a lifecycle-lock cycle.

Native close failures were also swallowed by Effect.ignore in the Cursor,
Claude, ACP, and OpenCode session finalizers, converting failed cleanup into
a clean release over still-owned provider resources. They now propagate (or
die inside Effect.addFinalizer slots) so the manager keeps ownership, and a
failed startup close retains the MCP credential reservation until cleanup
status is known. stopSessions now delegates to the closeInstance barrier so
sign-in/out joins in-flight startups and failed cleanups instead of trusting
projected status.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… marks

Astra xhigh follow-ups on the provider-cleanup contract:

- Claude interrupt timeouts only drop queryContext when the native close
  succeeded; a failed close stays tracked so the session scope retries it
  instead of reporting a clean release over a live CLI.
- startTurn and compactThread unwind their busy mark with onExit so
  interruption while queued on the attach lock cannot pin the session
  busy forever (catch only covered typed failures).
- The attach/detach race regression now sweeps 60 deterministic
  scheduler offsets through the credential-issuance gate.
- Close-propagation tests inject typed Effect.fail errors instead of
  defects, since Effect.ignore preserves defects either way.

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ttach-race sweep before the gate

The onExit unwind for startTurn/compactThread decremented busyCount on
any non-success exit, so an interrupt landing before markBusy's increment
stole a different holder's mark and idled a live turn. markBusy now takes
an acquisition flag set inside the same uninterruptible region as the
increment, and the unwind only calls markIdle when this call actually
took the mark.

The attach-lock sweep aimed the stepping scheduler after opening the
credential gate, but Deferred.succeed resumes waiters inline on the
synchronous scheduler, so the target never engaged. The sweep now arms
before the gate opens and reaches offset 520 — past the ~505-op
bookkeeping-to-admission boundary where the unfused release/reacquire
gap actually sat.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… typecheck

The two-branch modify callback inferred the union's first member as the
result type, leaving outcome unknown under the repo-wide CI typecheck.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ay outbox before its worker restarts

A stalled adapter acquisition runs inside an uninterruptible mask, so an
unbounded Fiber.interrupt on the effect worker could pin shutdown before
provider-session cleanup ever ran. The stop wait is now bounded by the
same 30s bound used for cleanup, at both the shutdown finalizer and the
relay-startup failure path.

The replay harness restart path also forked the effect worker daemon
with its layer before reconcileAfterProcessLoss ran, letting the daemon
claim persisted rows that reconciliation then reset to pending. The
restart path now builds the layer without the daemon, reconciles, then
forks the worker — matching production ordering.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…edge teardown

forkScoped registers its own unbounded Fiber.interrupt finalizer, and it
lands after the shutdown teardown finalizer, so it runs first on scope
close — a stalled worker still pinned teardown before the bounded wait
could run. The worker is now forked detached and bound by a
bounded-interrupt finalizer that clears workerFiberRef, keeping a single
30s bound before providerSessions.shutdown.

The replay restart path also now honours a caller's runEffectWorker: false
instead of forking the daemon unconditionally after reconciliation.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
A detached worker was forked before workerFiberRef recorded it and its
bounded-interrupt finalizer was registered, so an interrupt landing
between the fork and the record orphaned a worker parked at activation —
free to run against a closed runtime once activation released. The fork,
ownership record, and finalizer registration now run uninterruptibly, so
every spawned worker is either fully owned or never created.

The regression sweeps hold offsets around the handoff and asserts the
worker can never run after scope close plus activation release; it fails
on the non-atomic version.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
decorateRuntime carried injectHistory through the spread unguarded, so a
runtime a release or replacement already retired could still invoke native
thread/inject_items — e.g. a Codex history injection racing a close during
ContextHandoffDelivery's pending-write suspension could land on a dying
session yet still be durably marked injected.

Decorate it like snapshot/rollback: live-runtime checks around the activity
touch, then admission into inflightAdapterOps so a release drains the call
before scope close and the ownership recheck refuses stale or replaced
runtimes, plus the detached-thread refusal when the provider thread carries
an appThreadId.

Regression coverage proves a released or same-id-replaced runtime rejects
injectHistory before the adapter runs (invocation counter stays 0) while a
live runtime is admitted.

Model: SWE-2 Max (Devin/Cognition) via Cursor harness in T3 Code.
@saphid
saphid force-pushed the work/ov2-20260913-01 branch from ff93972 to 25c1917 Compare September 27, 2026 01:20
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
@juliusmarminge
juliusmarminge deleted the branch pingdotgg:t3code/codex-turn-mapping October 2, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ 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.

2 participants