Skip to content

fix(server): stop superseded provider startups - #15784

Open
Adamulek123 wants to merge 1 commit into
pingdotgg:mainfrom
Adamulek123:fix/v2-startup-ownership-focused
Open

Adamulek123 wants to merge 1 commit into
pingdotgg:mainfrom
Adamulek123:fix/v2-startup-ownership-focused

Conversation

@Adamulek123

Copy link
Copy Markdown
Contributor

Problem

A provider start can lose ownership while resolving policy or opening a session. The run may still be starting under a replacement attempt, but the old startup continues opening or loading native state. This was found in the orchestration-v2 audit that produced #15311.

Change

Check the current attempt and starting status after policy resolution and after session opening, before fresh-thread creation, native resume or fork. The production change is two existing ownership checks. Upstream's native-thread turn accounting remains intact.

Scope and approval

This replaces only the startup-ownership part of #15311. It is a server reliability fix for superseded work, shared by all clients and providers. No prior maintainer approval is claimed. Missing-project diagnostics and recovery type cleanup are deferred separately.

Verification

  • All 22 ProviderTurnStartService.test.ts tests passed.
  • Deferred cases replace the active attempt while policy resolution or opening is suspended, keeping the run starting. The old attempt performs no native ensure, resume or fork and writes no events. Removing both guards makes all four cases fail; restoring them passes.
  • Existing stop-during-policy/open, start failure and local-command tests remain green. Tests run the real start service with controlled provider/store boundaries, rather than a live native session or SQLite persistence.
  • Scoped server typecheck, changed-file lint and formatting passed. No live-client verification or performance measurement was performed.

Implemented with GPT-6.1 Sol through Codex. Verification uses Effect/Vitest and PowerShell.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 4, 2026
@Adamulek123
Adamulek123 marked this pull request as ready for review October 4, 2026 22:23
@macroscopeapp

macroscopeapp Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at f8fe927

Macroscope's review found this PR approvable — The change is limited to two ownership checks that prevent superseded server startups from opening sessions or loading native threads, with focused regression coverage for the affected phases. Existing current-attempt behavior, schemas, defaults, and deployment configuration remain unchanged.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

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

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 4, 2026
@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
docs/internals/effect-services.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9c0d171a-5588-43e3-97ad-343feab481a9
📥 Commits

Reviewing files that changed from the base of the PR and between efecd3c and f8fe927.

📒 Files selected for processing (2)
  • apps/server/src/orchestration-v2/ProviderTurnStartService.test.ts
  • apps/server/src/orchestration-v2/ProviderTurnStartService.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.


📝 Walkthrough

Walkthrough

Provider startup now checks whether the active attempt remains in "starting" status before and after opening a provider session. Tests cover interrupted and superseded attempts, and verify that thread loading and event persistence do not continue in those cases.

Changes

Provider startup

Layer / File(s) Summary
Guard startup against attempt changes
apps/server/src/orchestration-v2/ProviderTurnStartService.ts, apps/server/src/orchestration-v2/ProviderTurnStartService.test.ts
Startup returns if the active attempt is no longer in "starting" status before or after opening a provider session. Tests cover interruption and replacement during policy resolution or session opening, including ensure, resume, and fork thread loading. A session-open failure test verifies that ensureThread is not called.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to f8fe9

The startup guards stop superseded attempts from continuing to thread loading. No actionable merge-blocking risk remains after normal checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to f8fe9

The change restricts superseded startup work without adding access or authority. Existing session reuse and cleanup remain responsible for sessions already opened, avoiding unconditional closure of a session needed by a replacement attempt. No material security risk was found introduced or worsened.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The assessed exposure is the existing run and provider-session lifecycle. The new checks restrict continuation by stale attempts; they do not grant additional provider authority or expose the test controls to production callers.

Trust Boundaries and Controls

  • observed — The control checks both active-attempt identity and run status. Keeping a run starting while replacing its attempt therefore cannot satisfy the stale attempt's new continuation checks.

Resilience and Maintainability Implications

  • inferred — A post-open stale return does not establish an orphaned session: replacement attempts using the same session ID can reuse the runtime, while manager-owned idle release checks activity and generation before removal. Avoiding unconditional release also avoids closing a session merely because its original opener lost attempt ownership. Operational cleanup under live-adapter failure remains unverified.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: stopping provider startups that have been superseded.
Description check ✅ Passed The description includes all required sections and explains the problem, change, scope, and focused verification results. It describes the fix as a bounded server reliability change, although it does …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 11, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 11, 2026 05:56

Dismissing prior approval to re-evaluate f8fe927

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:XS 0-9 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