[romil-bijarnia]

Problem

Deleting a thread skips provider sessions whose persisted status is error. An errored session can still own a reusable live runtime: ProviderSessionManager deliberately retains those sessions. The delete therefore omits the per-thread detach effect and its MCP-credential revocation, leaving a deleted thread attached to the runtime with a valid credential.

Change

Skip only sessions that are already stopped. Errored sessions now use the same existing detach event and effect as other live sessions. Extend the existing deletion-planner case to require detaching both running and errored sessions, retain the stopped-session exclusion, and keep the earlier deletion timestamp.

The existing session manager still handles shared runtimes, per-thread unloading, credential revocation and idle release. This does not force-close the shared provider process or change session lifetime policy.

Scope and approval

A small, focused fix for an obvious omission in the existing delete lifecycle, submitted under the bug-fix exception. Two files, one underlying problem, no new settings, provider-specific behavior, contracts or UI.

Searched current open PRs for thread deletion and errored-session cleanup. #34175 concerns device claims during archive/delete, not provider-session detachment; #31658 concerns recovery of rejected Codex conversations.

Verification

Linux arm64, Node 26.12.1, base 42185a6206245c1cde5368e8ce368b384e7a400a.

• Red first: the extended deletion-planner case retains ["stopped", "error"] instead of ["stopped"] and omits the error session's detach effect on unchanged production code.

• With the fix, 5 focused cases pass across ThreadDeletion.test.ts and ProviderSessionManager.test.ts: deletion/resource planning, active-work disposal, reusable error states, terminal credential revocation, and independent detach on shared runtimes.

• A standalone runtime exercise reused the existing fake-provider fixture with the production session manager, in-memory SQLite projection/event stores, real MCP registry, and deletion planner. It emitted a reusable session error, awaited that exact event, committed the deletion events, and executed the planned detach through the production manager:

  before both runs: status=error, live=true, credentialValid=true
  unchanged planner: detachCount=0, deleted=true, credentialValid=true, mcpSessionPresent=true
  fixed planner:     detachCount=1, deleted=true, credentialValid=false, mcpSessionPresent=false
  

  No real provider turn, account, live database, or desktop UI was used. The exercise invokes the planned detach directly, rather than through the asynchronous effect worker. Existing worker routing is unchanged.

• vp run check:changed 42185a6206245c1cde5368e8ce368b384e7a400a --concurrency 2 passes format, lint, both knip checks, GraphQL, typechecks for 3 affected packages, and all 4 selected tests across 2 files.

[macroscopeapp]

Approvability

Verdict: Approved at 74ab021

Macroscope's review found this PR approvable — Deleting a thread now detaches errored provider sessions too, preventing live runtimes and credentials from lingering. The change reuses existing thread-scoped cleanup and shared-session safeguards.

^{You can add or adjust custom eligibility rules. Learn more.}

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Thread deletion now detaches provider sessions with error status and revokes their MCP credentials. Sessions with stopped status remain skipped.

Changes: Provider session cleanup

Layer / File(s)	Summary
Detach error-status sessions apps/server/src/orchestration-v2/ThreadDeletion.ts, apps/server/src/orchestration-v2/ThreadDeletion.test.ts	Error-status provider sessions now follow the detachment event and effect path. The test expects only stopped sessions to remain and checks for detachment with MCP credential revocation.

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

Change: Bug fix

Possibly related PRs

• pingdotgg/t3code#21984: Changes provider-session detach behavior and cleanup, which is connected to detaching errored sessions.

Suggested labels: bugfix, size:M

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to 74ab0

Thread deletion now plans cleanup for errored provider sessions. No actionable issue introduced by this change remains identified.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly and concisely identifies the main change: detaching errored provider sessions during thread deletion.
Description check		The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the bug, the implementation, the approval exception, focused test results, and verificatio…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the bug, the implementation, the approval exception, focused test results, and verification limits.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Recent review info

Run configuration

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: main
• File: .coderabbit.config.ts
• Commit: a03d9bc
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Advanced
• Run ID: 244c5670-d742-4c66-868c-43073642503d

Commits

Reviewing files that changed from the base of the PR and between a03d9bc and 74ab021.

Files selected for processing: 2

• apps/server/src/orchestration-v2/ThreadDeletion.test.ts
• apps/server/src/orchestration-v2/ThreadDeletion.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.

[binjamshid]

The fix matches the stated failure: ProviderSessionManager.detach is already a no-op when the entry is gone or the thread is not attached, and it still runs clearMcpSession when revokeMcpCredential is set, so emitting the detach for an error session is safe even when that runtime is actually dead. The projection drops the session on provider-session.detached, and the extended planner case pins both the effect and the projection result.

1. (low, follow-up, outside this diff) The same status !== "stopped" && status !== "error" filter also gates detaches on the provider-switch / fresh-context / worktree-change paths in apps/server/src/orchestration-v2/Orchestrator.ts (around lines 2130, 4576 and 6827 on current main). If an errored session can still own a reusable live runtime, those paths leave the thread attached in the same way. Keeping it out of this PR is reasonable; it may be worth a separate issue so the rule stays consistent.

The comment at ThreadDeletion.ts:186 explains the rule well. No other concerns.

[maria-rcks]

Note

Written by claude-opus-5-5 on behalf of Maria

Verified in a real dev environment

Ran vp run dev for github/main (f697d4db3e) and then for this PR's head (74ab021), each against its own seeded worktree state, on Linux. Driven with Playwright and /usr/bin/google-chrome, plus t3 CLI commands against the running server.

To get a live session in error, I used the Pi provider, added through Settings > Providers. Its turn failed with a real provider 401, which leaves the pi --mode rpc process running and the session at error. Claude never leaves a live session in error, so it can't reproduce this. I used Claude claude-haiku-5-5 for the neighbor checks.

On web, deleting a single thread from the sidebar does not reproduce the bug on main. useThreadActions first sends a client-requested provider-session.detach, which already kills the process and revokes the credential. The reachable web path is Settings > Projects > Remove project. It deletes the project's threads through ProjectService with no client pre-stop. Mobile thread delete also skips the pre-stop.

After Remove project on a project holding the errored Pi thread:

	main	PR
provider-session.detach effect for the deleted thread	none	succeeded, "Thread deleted."
deleted thread's pi process	still running	exited
MCP initialize with the deleted thread's bearer token	HTTP 200	HTTP 401 invalid_mcp_credential

main transcript

$ t3 thread start --provider pi --model anthropic/claude-haiku-5-5 --wait <pi-repo-main> 'say pong'
Thread mcp:6f59a4d1-... stopped with status failed: 401 authentication_error
$ # after Settings > Projects > Remove project
## provider processes running in /home/user/pi-repo-main
  47501       01:56 pi
MCP initialize with the deleted thread's credential -> HTTP 200 {"jsonrpc":"2.0","id":1,"result":{...}}
## effects queued for the deleted thread
provider-turn.start      succeeded
scheduled-tasks.cleanup  succeeded
terminal.cleanup         succeeded
preview.cleanup          succeeded

PR transcript

$ t3 thread start --provider pi --model anthropic/claude-haiku-5-5 --wait <pi-repo-pr> 'say pong'
Thread mcp:628f5a09-... stopped with status failed: 401 authentication_error
$ # before removal: pi pid 56074 running, credential -> HTTP 200
$ # after Settings > Projects > Remove project
## provider processes running in /home/user/pi-repo-pr
(none)
MCP initialize with the deleted thread's credential -> HTTP 401 {"error":"invalid_mcp_credential","message":"A valid T3 Code MCP credential is required."}
## effects queued for the deleted thread
provider-turn.start      succeeded
provider-session.detach  succeeded  Thread deleted.
scheduled-tasks.cleanup  succeeded
terminal.cleanup         succeeded
preview.cleanup          succeeded

Remove project on main (process and credential survive, see transcript):

https://uploads-production-47e4.up.railway.app/files/f2d7d4e4-d1cc-40e7-b54f-5aded37f2875/remove-project-main.mp4

Remove project on this PR (process exits and credential is revoked, see transcript):

https://uploads-production-47e4.up.railway.app/files/efe5498e-1618-4470-a96f-05950654d8b7/remove-project-pr.mp4

Neighbor checks on the PR build, all passing:

• Sidebar delete of an errored Pi thread: one client-requested detach, every cleanup effect succeeded, process gone, credential 401. No duplicate detach and no failed effect.
• Sidebar delete of a healthy Claude haiku thread: the detach succeeds and the claude process exits.
• Remove project with a healthy Claude haiku thread: the planner's "Thread deleted." detach succeeds and the process exits, same as before.

CI on 74ab021: 22 passing, 10 skipped. I made no code changes.

Not verified

• Mobile thread delete. The same server thread.delete path is exercised above, but I didn't run the native client here.
• OpenCode and Muse errored sessions. Their adapters also report a live error status, but I exercised only Pi.
• A Claude session that ended through runtime_error, where the runtime is already released and the new detach should be a no-op. I couldn't trigger that failure on demand.
• The provider_sessions projection row for the deleted thread stays error after removal, on both main and the PR. That isn't changed by this PR.