Skip to content

fix(server): a thread cannot be archived while its turn is running - #15363

Open
Mnigos wants to merge 1 commit into
pingdotgg:mainfrom
Mnigos:archive-rejected-while-turn-running
Open

Mnigos wants to merge 1 commit into
pingdotgg:mainfrom
Mnigos:archive-rejected-while-turn-running

Conversation

@Mnigos

@Mnigos Mnigos commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #15135.

Problem

thread.archive only refused a thread that was already archived. It cancelled queued runs, then detached every live provider session with revokeMcpCredential: true, even while a run was preparing, starting or running. An agent that archives its own thread with t3_thread_organize {action:"archive"} always hits this, because MCP mutations require the caller to own an active run. For a single-thread session like Claude, the detach closes the run's event stream, and the turn fails with the generic "The provider event stream closed unexpectedly". The archive itself succeeds. The clients already block archiving in this state (threadRuntimeCanArchive), but the server did not.

Change

  • Guard. thread.archive now fails with a new typed OrchestratorThreadTurnRunningError when any run of the thread is preparing, starting or running. It is checked before any archive event or detach effect is planned, so a refused archive commits nothing. queued runs are still cancelled by the archive, and a waiting run (post-turn drain) still archives. The same guard applies whether the agent targets its own thread or another one, and it covers web and mobile dispatches too.
  • Typed error. Settle uses the generic OrchestratorDispatchError with a text cause. A dedicated tag (like OrchestratorSubagentThreadReadOnlyError) lets callers react to this rejection without parsing text. Its fixed message, "This thread cannot be archived while a turn is running. Archive it after the turn ends.", reaches web and mobile through the existing user-facing dispatch error mapping.
  • MCP. t3_thread_organize maps that tag to invalid_request with "A thread cannot be archived while a turn is running. Archive it after the turn ends, from another thread or in the app." Every other dispatch error keeps the generic orchestration_error. The message points elsewhere because an agent cannot archive its own thread once its turn has ended.
  • Comments. The detach comment in Orchestrator.ts assumed a guard that did not exist; it now names the real guards. The web thread-menu comment now says the server rejects archive while a run is preparing, starting or running, and that idle attached providers still archive.

Scope and approval

This follows the triage comment: "make thread.archive reject a thread with a preparing, starting, or running run, the way settle does. The agent would then get a clear error and the turn could finish, while queued work can still be cancelled." Deferring a self-archive until the turn ends was proposed and closed in #14328 (#14328), so this PR refuses instead of deferring.

Open #14907 (#14907) relies on archiving a thread whose run is still preparing. This guard follows the triage and the clients' existing threadRuntimeCanArchive rule, which already block preparing. With it, #14907 would need to settle the preparing run first, or maintainers can decide which behavior wins.

#15136 reports a related symptom from a planned detach on a workspace change. With this guard, archive no longer detaches a provider mid-turn. The workspace-change detach is a separate path and is not changed here.

Server only, plus one web comment. No contract change.

Verification

Observed result. Real dev servers at 18b2132, before and after the fix, driven through the authenticated MCP HTTP endpoint with the thread's own credential from the real Claude adapter. A deterministic Claude CLI stand-in holds the provider turn open until released; no live model call. The capture used the starting/running guard, before preparing was added; that status is covered by the tests below.

Action Before After
t3_thread_organize {action:"archive"} during the turn Returns {sequence}; archives the thread; the run fails 8 ms later with "The provider event stream closed unexpectedly…" Returns invalid_request with the message above; the thread stays unarchived and the run keeps running
Release the held turn Already failed Completes with a persisted checkpoint
Archive in the app after the turn ends — Succeeds; the completed run stays completed

Before, archived and failed:
before

After, archive refused and the turn still running:
after refused

After, the turn completed:
after completed

After, archived in the app:
after archived

Tests. 148 pass across 11 server suites: 63 in runtimeLayer.test.ts and OrchestratorMcpToolkit.integration.test.ts, plus 85 in the nine other suites that dispatch thread.archive. The tests cover:

  • refusal for preparing, starting and running, with no events, no outbox effects, an unchanged event sequence and archivedAt still null; the same thread archives once its run completes;
  • a waiting run still archives, and queued work is still cancelled;
  • MCP self-archive returns invalid_request with the provider session still attached;
  • archiving another running thread over MCP is refused, then succeeds after that thread's run completes and is checkpointed.

Orchestrator.control-reads.test.ts now ends its deferred (preparing) run before archiving. Negative control: with the guard disabled, the three status cases and the MCP integration test fail (4 failed, 62 passed across those three files). Server typecheck has no errors or warnings. Targeted lint passes, apart from one existing layerUnavailable warning. Format and both knip phases pass.

Not checked: a live Claude model and its tool selection, native desktop and mobile clients, other real providers, relay and tunnel modes, project deletion or scheduled tasks at runtime, a deliberately delayed overlap between checkpoint capture and detach, background-subagent termination, and the separate preparing-run race in #14907.

Implemented with Claude Code (Claude Opus 5.5, coordinated by Claude Fable 5.1); tests, independent review and the observed result by GPT-6 Astra via Codex.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 3, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 96d01d0

Macroscope's review found this PR approvable — This is a focused server bug fix that prevents archive from detaching provider sessions during an active turn, while preserving queued and idle-thread behavior. The implementation is localized and backed by targeted orchestration and MCP tests.

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

@coderabbitai

coderabbitai Bot commented Oct 3, 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 — configured

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: 49f3ad1f-e49b-42e5-b383-15ab11841290
📥 Commits

Reviewing files that changed from the base of the PR and between 00eb8f6 and 96d01d0.

📒 Files selected for processing (6)
  • apps/server/src/mcp/OrchestratorMcpToolkit.integration.test.ts
  • apps/server/src/mcp/toolkits/thread/handlers.ts
  • apps/server/src/orchestration-v2/Orchestrator.control-reads.test.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/orchestration-v2/runtimeLayer.test.ts
  • apps/web/src/components/threadActionMenu.logic.ts

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


📝 Walkthrough

Walkthrough

Thread archiving now fails with invalid_request when the thread has a run in preparing, starting, or running. Tests cover these rejected states, allowed archive states, and MCP responses.

Changes

Thread archive active-turn guard

Layer / File(s) Summary
Orchestrator archive guard
apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/orchestration-v2/runtimeLayer.test.ts, apps/server/src/orchestration-v2/Orchestrator.control-reads.test.ts
The orchestrator rejects archive commands when a run is preparing, starting, or running. Runtime tests cover rejection without persisted events or effects, successful archive after completion, and archive behavior for waiting runs.
MCP archive response
apps/server/src/mcp/toolkits/thread/handlers.ts, apps/server/src/mcp/OrchestratorMcpToolkit.integration.test.ts, apps/web/src/components/threadActionMenu.logic.ts
The MCP handler maps the active-turn error to invalid_request. Integration tests cover self-archive and archiving another thread, including success after the run completes. The web comment describes the rejected run states.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 96d01

Archiving is refused while a turn is active and remains available afterward. No merge-blocking issue is identified; merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 96d01

The change prevents archiving from disrupting an active turn, while preserving the inspected project and caller-ownership checks. No new material security issue was established. Confidence remains limited around cleanup failures and recovery after a successful archive.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The MCP archive target can be caller-selected, but resolution uses the caller's project ID and dispatch uses the resolved thread ID. Within that boundary, successful archive can cancel queued work and initiate provider-session, credential and terminal cleanup for the target thread.

Security Findings and Attack Paths

  • inferred — The new rejection distinguishes active-turn state, but existing project-scoped thread reads already return recent run statuses. That counterevidence does not support treating this response as a new material disclosure or authorization bypass in the inspected flow.

Trust Boundaries and Controls

  • observed — The access path requires an orchestration-capable credential and an existing, non-deleted caller. Mutation additionally requires an unarchived caller with an activeRunId and providerInstanceId matching the invocation, then invokes runtime-mode and interaction-mode checks before dispatch.

Resilience and Maintainability Implications

  • observed — The inspected cleanup path forwards credential revocation to the session manager, which attempts revocation even when a repeated detach finds no attachment. Detach and terminal cleanup are classified for process-loss replay. Effect execution failures retry with backoff and eventually become failed effects, so the code does not establish unconditional cleanup completion.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: the server rejects archiving while a thread turn is running.
Description check ✅ Passed The description covers the problem, change, scope and approval, and verification. It includes focused test results, observed behavior, and checks not performed.
Linked Issues check ✅ Passed [ #15135 ] requires either deferred self-archive or a clear refusal that lets the active turn finish. Orchestrator.ts rejects preparing, starting, and running runs before archive effects are p…
Out of Scope Changes check ✅ Passed All changes support [ #15135 ]. The runtime and MCP tests verify the guard and error behavior. The server and web comment updates document the archive guard. No unrelated functional changes appear in …
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 6…
✨ 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.

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

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Agent archiving its own thread with t3_thread_organize fails the running turn with "provider event stream closed unexpectedly"

1 participant