feat(storage): clean up settled thread worktrees#15208

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Restore the cleaned worktree before opening the rollback session.

When cleanup acquires the corresponding workspace lease first and removes the checkout, rollback waits but does not recreate it. The production runtime policy sets cwd to thread.worktreePath, and ProviderSessionManagerV2.open returns ProviderWorkspaceMissingError when that path is missing. Because sessions.open runs before provider rollback and optional checkpoint restoration, rollback can fail before either operation. The cleanup-first test currently asserts this failure with a mocked open. Recreate the missing worktree from the retained branch under the lease before sessions.open, following the prune-and-create recovery used by ProviderTurnStartService.

✅ Addressed in commits fa441e3 to 254eca7

Fixed in 254eca7e6d. Rollback validates its target before filesystem work and recreates a missing checkout from the retained branch under the existing workspace lease, before restore safety checks and sessions.open. The regression removes and recreates a real temporary directory and covers cleanup-first, rollback-first, existing checkouts, and both restoreFiles modes. A separate startup fix in fa441e3aae releases the lease after session and running state are persisted, before provider work; that also fixes the Test Server 6 MCP timeout. All 112 focused tests across six files and server type checking pass.

@Bil0000, thanks for the fix. The supplied code validates the rollback target, then recreates the missing checkout under the workspace lease before restore safety checks and sessions.open. This addresses the cleanup-first failure raised in this thread.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

You are interacting with an AI system.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Use one physical path identity for cleanup and provider startup.

existing_worktree can preserve a symlink alias. Cleanup groups threads by path.resolve, so a canonical thread and an alias thread become separate candidates. The canonical thread can pass the idle, settled, terminal, session, Git, and final-state checks while the alias thread starts.

Cleanup then holds the canonical lease, while ProviderTurnStartService holds the alias lease. Cleanup can call git.removeWorktree while the alias startup is using the same physical checkout. If startup observes the removal, its recovery path calls createWorktree with the alias path. That can fail or leave the provider turn without the expected checkout.

Normalize existing-worktree paths to one physical canonical identity before persisting them. Use that identity for cleanup grouping and checks, provider startup leasing, and missing-worktree recreation. Normalizing only the new lease key is insufficient. For a missing worktree, canonicalize its existing parent and append the worktree name instead of resolving the missing target itself.

✅ Addressed in commit 77fc976

@coderabbitai Fixed in 77fc976a2801a69d4109edc213c12ef32553e4eb. Thread launch stores the physical checkout path before initial creation and deferred setup, and normalizes created, resumed and retried paths while preserving renamed branches. Legacy startup and rollback projections resolve the same physical path before taking the lease; recovery and provider cwd use that path too. Cleanup resolves initial and final thread groups, archived/deleted paths, provider-session cwd, terminal cwd/worktree paths and project roots. Terminal open/restart also use the physical lease key before their first event.

The resolver uses realPath for existing targets, follows dangling links, and resolves the existing parent plus missing names for absent targets. Other filesystem errors and link cycles fail rather than falling back to an alias. The startup lease still ends before the long provider turn.

Alias regressions failed before the fix. The primary agent ran all 360 focused tests across 11 files, including both race orders, shared/session/terminal aliases, missing and dangling paths, deferred launch and branch-preserving retry. Server type checking passed with exit status 0 and no TypeScript errors; targeted lint and formatting passed. Please confirm this addresses the finding. GitHub currently rate-limits review-thread reads/resolution, so I could not mark this thread resolved myself.

@Bil0000 Yes. I inspected commit 77fc976a2801a69d4109edc213c12ef32553e4eb. The changes address this finding.

• Thread launch persists the physical checkout path and preserves renamed branches during retry.
• Startup and rollback use the resolved path for the workspace lease, recovery, and provider cwd.
• Cleanup resolves paths before initial grouping and final safety checks, including shared threads, sessions, terminals, and project roots.
• Terminal open/restart use the same physical lease key.
• The resolver handles missing targets and dangling links without falling back to an alias on other filesystem errors.
• The long provider turn remains outside the startup lease.

The regression assertions cover both race orders and the relevant alias cases. I did not rerun tests or type checking in this inspection.

Thanks for the detailed explanation.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

You are interacting with an AI system.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Re-read the projection after acquiring the workspace lease.

readProjection runs before withWorkspaceLease, so the lease protects only execute. If startup acquires the lease first, it can change projection-backed state while rollback waits. Rollback then uses stale data for target validation, session selection, runsToRollback, provider turns, and stale checkpoints. Resolve the lease key from the first read, acquire the lease, and re-read the projection inside it. Retry if the worktree path changes between reads.

✅ Addressed in commits 5be57b7 to 335659c

Fixed in 5be57b7f56. Rollback uses the first read only to choose a physical workspace lease, then reads the projection again under that lease. A changed physical worktree path releases the old permit and retries under the new path before restore safety checks, checkout recovery, or session setup. Regression tests cover a checkpoint becoming stale while queued and path changes from a worktree or the project root.

@Bil0000, thanks for the fix. The supplied code re-reads the projection under the workspace lease. If the physical worktree path changes, it releases the lease and retries before executing rollback. This addresses the stale-projection concern.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

You are interacting with an AI system.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Re-read the start context after acquiring the workspace lease.

getTurnStartContext runs before withWorkspaceLease. A queued second start can therefore pass the stale "starting" check and perform provider-session preparation from an old projection before writeIfRunCurrent rejects its events.

Use the first read only to determine the lease key. Read the projection again inside the lease-wrapped effect.

✅ Addressed in commits 5be57b7 to 335659c

Fixed in 5be57b7f56. Startup reads current turn-start state after acquiring the workspace lease, and switches leases if the physical worktree path changed while waiting. The stale-running-state regression confirms that no provider session or root run starts from the old snapshot. Additional tests cover worktree-to-worktree and root-to-worktree changes. The four startup-memory cases pass with their history-release assertions unchanged; their expected read count now includes the required fresh read.

@Bil0000, thanks for the fix. The supplied code reads getTurnStartContext again inside the workspace lease. If the physical worktree path changed, startup releases the lease and retries with the new path. This addresses the stale-projection finding and keeps the provider turn outside the startup lease.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

You are interacting with an AI system.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

A session path that fails to resolve aborts the cleanup of this worktree only. Confirm that this is the intended behavior.

Effect.forEach fails on the first resolveWorkspacePath error, for example EACCES or a symlink cycle in an unrelated session's cwd. The outer Effect.catch logs that error at debug level and skips this worktree. Because the failure aborts the removal, the change does not delete anything unsafely. However, one malformed session cwd blocks cleanup of every worktree on every sweep, and the only record is a debug-level log. The same failure applies to hasTerminal at Lines 143-146, where any live terminal's cwd is resolved. Consider these changes:

• Treat an unresolvable session path or terminal path as a non-match. Fall back to the lexical path.resolve.
• Log the failure at warning level.

✅ Addressed in commit c81c14e

Kept the deletion guard fail-closed. A path lookup error cannot prove that an active provider or terminal is outside the candidate checkout, so a lexical non-match could permit deletion of a workspace that is still in use. Missing paths already resolve through their existing parent, including dangling aliases. 335659c539 adds a warning for provider-session resolution failures; terminal failures already reach the sweep warning. New tests use real symlink cycles for both provider sessions and terminals and verify that the checkout stays on disk and a warning is emitted. The safe next step is to repair the unresolved path and let a later sweep retry.

@Bil0000, you are right. A path-resolution failure does not prove that an active provider session or terminal is outside the checkout. My lexical-fallback suggestion could permit unsafe deletion, so I withdraw it.

The supplied code logs provider-session resolution failures at warning level and preserves the failure. Terminal resolution failures propagate to the sweep warning. Keeping deletion fail-closed, repairing the unresolved path, and retrying on a later sweep is the correct behavior.

Thanks for the guidance.

✅ Review thread resolved.

You are interacting with an AI system.

🟡 Medium src/storageCleanup.ts:164

A symlink cycle in any active or archived thread now makes readThreads fail, so cleanWorktrees aborts and skips every otherwise eligible worktree in that sweep. Handle resolveWorkspacePath errors per thread and fall back to the existing path.resolve value instead of propagating the failure globally.

No longer relevant as of c81c14e8c249db737680ca1f78da1c8315a2a5de

Confirmed that a resolution failure in an active or archived thread blocks the worktree sweep. This is an intentional safety guard: those thread paths are also used to detect shared checkouts, and an unresolved physical path cannot prove that a thread is outside a candidate checkout. The proposed path.resolve fallback catches all resolver errors, including unreadable aliases that could point to that candidate, and could allow deletion of a shared workspace. I kept the guard and the existing sweep warning. dfd163f665 adds real symlink-cycle cases for both active and archived threads; each preserves an otherwise eligible checkout, performs zero removals, and logs the warning. Repairing the unresolved thread path lets a later sweep retry. All 371 focused tests and server type checking pass.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The "Delete now" gate in CleanupSection does not count settled retention as a rule.

hasRules uses a local enabled predicate at Lines 274-278. That predicate omits worktreeSettledAfterDays. Suppose a user enables only "Delete settled worktrees". The panel then shows "Turn on a cleanup rule to use this." and disables Delete now. The server-side worktreeCleanupEnabled treats the same rules as enabled. Add the new field to the predicate.

✅ Addressed in commit faaaab3

Fixed in faaaab3b374f14c3e23ba5e5e494258266a8445a. The Delete now gate now counts non-null settled retention for both machine and project rules. The rendered regression tests failed before the fix and pass now. They cover immediate machine retention, project custom retention, the disabled state with all rules off, and the cleanup action. All 30 focused settings tests, web type checking, targeted lint, and formatting pass.

@Bil0000 Thanks for the fix. The supplied code confirms that hasRules now counts non-null worktreeSettledAfterDays for both machine rules and project custom rules. The reported regression tests cover the enabled state, the all-rules-off state, and the cleanup action.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

You are interacting with an AI system.