Skip to content

fix(server): block V2 replacements until session cleanup completes - #9806

Open
shivamhwp wants to merge 6 commits into
pingdotgg:t3code/codex-turn-mappingfrom
shivamhwp:fix/v2-session-release-timeout
Open

fix(server): block V2 replacements until session cleanup completes#9806
shivamhwp wants to merge 6 commits into
pingdotgg:t3code/codex-turn-mappingfrom
shivamhwp:fix/v2-session-release-timeout

Conversation

@shivamhwp

@shivamhwp shivamhwp commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

V2 reports a provider session as stopped after a 30-second cleanup timeout, even when its scope is still closing. Removing the live entry also lets a replacement open before cleanup finishes.

Keep the pending release until cleanup completes. Timeouts return an error and record error status instead of stopped. Close and detach retries wait for the same cleanup. New sessions and new thread attachments covered by pending cleanup stay blocked; a thread already attached to another live session can keep reusing it. Failed cleanup keeps the replacement block until the environment restarts.

Targets the Orchestration V2 branch in #2829, rebased onto Julius's latest base at 26a48b00. The conflict resolution preserves the base's workspace-folder validation alongside the cleanup locks. This does not change shared-session detach policy or solve the separate shared-Codex-process account handoff problem. Related #8580 and #9211 change V1 switching, not this V2 release path.

The live-session reuse review finding is fixed in the rebased commit 1b23a2f2. On the current head, 38e1a9ae, all 106 focused tests across nine session-manager and consumer files pass, as do server typecheck and targeted lint/formatting. CI, Macroscope correctness, and Effect Service Conventions passed on 38e1a9ae. No review threads remain unresolved. Macroscope still requires human approval for the lifecycle and credential-handling scope.

Previous validation before this rebase: Two new regression cases reproduce the prior rejection and cover pending cleanup, timeout, late success, and permanent cleanup failure. All 76 focused tests across 11 files passed, with one optional fixture-recording test skipped. Server typecheck and targeted lint/formatting passed. The earlier rebased head passed 363 tests across 20 files, including provider adapters, replay, restart recovery, and independent lifecycle probes.

CI repeatedly failed or timed out fetching Ubuntu packages over HTTP, including on fresh-runner retries. The original separate commit, b0ccd56, changed the runner's Ubuntu package downloads to HTTPS without changing package sources or test commands. Full CI passed on that earlier head: https://github.com/pingdotgg/t3code/actions/runs/33936834660.

Before this rebase, Cursor Bugbot, Macroscope correctness, and Effect Service Conventions passed on b0ccd56. All review threads are resolved. Macroscope requires human approval because of the production lifecycle and credential-handling scope.

Screenshots

Before: no warning while cleanup is still running.

Before: cleanup times out without warning

After: the PR shows the cleanup timeout and blocks replacement sessions.

After: visible cleanup timeout warning

Recovery: the warning clears automatically after cleanup succeeds.

Recovery: cleanup completed and warning cleared

Captured in a real Chromium client connected to an isolated local server and the real Codex provider. Both the V2 base and PR head used a temporary five-second idle timer and a 120-second cleanup delay. The production 30-second cleanup timeout was unchanged.

  • Before, V2 base 3ed8c4df: the session was marked stopped after the timeout, with no warning while cleanup was still running.
  • After, PR head b0ccd56f: the session stayed in error and the app showed that replacement sessions were blocked until cleanup completed.
  • Recovery: cleanup finished, the stored error cleared, and the warning disappeared without dismissal or a page reload.

Temporary test hooks were removed after capture and are not included in this PR. Original PNGs and the captured session-state evidence are retained with the test artifacts.

Full test report.

Implemented and tested by GPT-6 using Codex in T3 Code for shivamhwp.


Note

High Risk
Changes core provider session lifecycle, MCP credential revocation timing, and open/close concurrency in Orchestration V2; failed or slow cleanup can block replacements until restart.

Overview
Orchestration V2 no longer treats a hung provider session close as successfully stopped. ProviderSessionManagerV2 keeps a releasing record after removing a session from the live map, runs scope cleanup on a background fiber, and waits up to 30 seconds; on timeout callers get a release error, projections move to error, and cleanup keeps running.

While that release is pending, open fails with ProviderSessionOpenError for the same session id or for threads tied to the releasing session (already-attached live sessions on other ids can still be reused). close/detach join the in-flight cleanup instead of starting parallel teardown; MCP credential revocation is deferred until after scope close, with guards for reservations and live holders.

CI apt steps on Blacksmith runners now rewrite mirror URLs from HTTP to HTTPS before apt-get update. Provider-switching docs describe the new cleanup and blocking rules.

Reviewed by Cursor Bugbot for commit b0ccd56. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Block V2 provider session replacements until cleanup completes

  • Replaces the previous acquire/use/release flow in ProviderSessionManagerV2.releaseEntry with an uninterruptible coordinator. Released sessions move into a releasing registry and stay there until cleanup finishes, so repeated close/detach calls share the same cleanup fiber instead of starting independent releases.
  • ProviderSessionManagerV2.open now rejects reuse of a provider-session ID that is still cleaning up, and rejects opening or attaching a recorded thread whose prior cleanup is pending. An already-attached live session on a different thread can still be reused during peer cleanup.
  • ProviderSessionManagerV2.detach on an already-releasing session now waits for the existing release instead of starting a new one, and records the detached thread ID as part of the pending cleanup.
  • Cleanup returns a 30-second ProviderSessionReleaseError on timeout but lets the fiber continue. Successful cleanup removes the block and revokes credentials subject to current reservations; failed cleanup keeps the block until restart.
  • Risk: releaseEntry now retains session entries in the releasing registry on cleanup failure — a fiber that hangs without hitting the 30s timeout will block replacement for that session/thread pair until process restart. Reviewers should check that all error paths in releaseEntry complete the completion deferred so waiters are not stranded.

Macroscope summarized 38e1a9a.

@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 4, 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
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
@shivamhwp

Copy link
Copy Markdown
Collaborator Author

Note: GPT-6 on behalf of shivam (@shivamhwp).

The first CI run found failures in unchanged V2-base files: Check reports formatting in seven files, Test Server 1 expects 58 migrations but the base registers 59, and Release Smoke fails on an unused expo-notifications@57.0.15 patch. None of those files are changed by this PR. Keeping those fixes separate from session cleanup.

The review fixes are in d68d735. All 34 focused session-manager and turn-start tests pass locally, along with targeted lint and server typecheck. Waiting for the new CI and review results.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — The production session manager now changes cleanup timing, replacement and thread-attachment rules, persisted error states, and MCP credential revocation across concurrent lifecycle operations. These broad runtime and credential-lifecycle effects require human review beyond the accompanying tests, documentation, and CI adjustment.

You can add or adjust custom eligibility rules. Learn more.

@shivamhwp

Copy link
Copy Markdown
Collaborator Author

Note: GPT-6 on behalf of shivam (@shivamhwp).

Latest head bf93d87 passes Bugbot and Macroscope correctness. All reported review threads are resolved. Locally, 58 focused tests across six session-manager consumer test files pass, as do targeted lint and server typecheck.

CI remains blocked outside this change: seven unchanged files fail formatting, the migration test expects 58 entries instead of the registered 59, and Release Smoke rejects the unused Expo notifications patch. Earlier Test Server 3 logs also show stale replay expectations for the runtime-info prompt suffix. I reproduced the migration failure, the seven formatting failures, and the tool_call_read_only/cursor replay mismatch locally. The relevant adapter, migration, fixture, and release files match the V2 base.

Leaving these base fixes out of the cleanup PR pending maintainer direction. Nothing has been merged.

@shivamhwp
shivamhwp force-pushed the fix/v2-session-release-timeout branch from c61d233 to c86ae9d Compare September 4, 2026 23:41
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts Outdated
Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts
@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch from f2e4a0d to 3ed8c4d Compare September 5, 2026 00:45
@shivamhwp
shivamhwp force-pushed the fix/v2-session-release-timeout branch from 8098b97 to bc10499 Compare September 5, 2026 01:05

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit bc10499. Configure here.

Comment thread apps/server/src/orchestration-v2/ProviderSessionManager.ts
@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch 2 times, most recently from b82facd to 2ac9bfe Compare September 5, 2026 04:56
@shivamhwp
shivamhwp force-pushed the fix/v2-session-release-timeout branch from b0ccd56 to 38e1a9a Compare September 6, 2026 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 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