Skip to content

fix(server): storage cleanup reaches archived threads' worktrees - #16473

Open
Info-Cado wants to merge 1 commit into
pingdotgg:mainfrom
Info-Cado:fix/storage-cleanup-archived-threads
Open

Info-Cado wants to merge 1 commit into
pingdotgg:mainfrom
Info-Cado:fix/storage-cleanup-archived-threads

Conversation

@Info-Cado

Copy link
Copy Markdown

Problem

Automatic storage cleanup never looks at archived threads, so their worktrees stay on disk under every worktree rule except deletion.

The V2 port of readThreads (apps/server/src/storageCleanup.ts:152-157, from #2829) read archived threads with getShellSnapshot({ location: "archive" }).threads. The store returns archived threads in archivedThreads and filters threads to archivedAt === null (ProjectionStore.ts SQL layer around line 5586, memory layer around line 5760). So that list is always empty. V1 included archived threads through getArchivedShellSnapshot(), and the storage guide does not exclude them.

Change

Read one unfiltered shell snapshot and use both threads and archivedThreads. The read moves into a small exported readStorageCleanupThreads(projections) so a test can run it against the real projection store.

Archived threads then go through the same checks as active ones: idle, single owner, terminals, sessions, managed path, clean Git state, ignored files, and the re-read before removal. A side effect, also a fix: an archived thread that shares a worktree now counts as a sharer. Before, cleanup could not see it, so the single-owner check and the deleted-thread path could treat a worktree it still used as unowned.

Scope and approval

There is no prior issue. I believe this qualifies as a very small, focused fix for an obvious bug: one read misuses the snapshot contract, and the fix restores V1 and documented behavior with no new setting or policy. I found it while adding evidence to #15146. It is independent of #15146/#15150 (status gate), #14742/#14847 (squash merges) and #16472 (subagent-shared worktrees). #15080 happens to make the same change inside a much larger rewrite.

Verification

  • New test V2 storage cleanup thread reads > includes archived threads seeds one active and one archived thread into ProjectionStore.layer on in-memory SQLite, then checks that readStorageCleanupThreads returns both.
  • With the old two-call read swapped back in, the test fails: expected [ 'thread-active' ] to deeply equal [ 'thread-active', 'thread-archived' ]. With the fix: vp test run apps/server/src/storageCleanup.test.ts, 10 passed.
  • tsc --noEmit in apps/server: no errors in the changed files. The only errors are 10 in src/process/externalLauncher.test.ts, which already fail on main at 9bd1d8009a. vp lint and vp fmt --check on both files are clean.
  • A Codex review (GPT-6.1-Sol, high) of the commit had no findings.

Not checked: a live sweep removing an archived thread's worktree in a running app. The rest of the sweep needs Git, settings, terminal and session services, and no test harness covers it today.

Written by Claude Opus 5.5 in T3 Code's Claude Code harness, reviewed by GPT-6.1-Sol in T3 Code's Codex harness.

🤖 Generated with Claude Code

The V2 port read archived threads from `getShellSnapshot({ location:
"archive" }).threads`, but the store returns them in `archivedThreads`,
so `.threads` was always empty there. Archived threads never became
cleanup candidates, and their worktrees stayed on disk.

Read one snapshot and include both `threads` and `archivedThreads`, as
V1 did with `getArchivedShellSnapshot`.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 6, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 8d4b734

Macroscope's review found this PR approvable — This is a small, well-scoped server bug fix that restores archived threads to the existing storage-cleanup pipeline without changing defaults or introducing new cleanup rules. The added projection-store test covers the corrected active-plus-archived thread read.

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

@coderabbitai

coderabbitai Bot commented Oct 6, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 848f233b-b430-4eeb-90c7-35b7d4afb37a
📥 Commits

Reviewing files that changed from the base of the PR and between 9bd1d80 and 8d4b734.

📒 Files selected for processing (2)
  • apps/server/src/storageCleanup.test.ts
  • apps/server/src/storageCleanup.ts

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


📝 Walkthrough

Walkthrough

The storage-cleanup code now reads active and archived threads from one shell snapshot. readThreads uses the new helper, and a test checks that it returns IDs for both thread states.

Changes

Storage cleanup thread reads

Layer / File(s) Summary
Read active and archived threads
apps/server/src/storageCleanup.ts, apps/server/src/storageCleanup.test.ts
The new readStorageCleanupThreads helper maps a shell snapshot to a combined active and archived thread list. readThreads uses the helper. A projection-backed test checks that both thread IDs are returned.

Priority: ➖ Normal

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

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 8d4b7

Storage cleanup now includes archived threads in its thread read. No merge-blocking issue is identified; a live cleanup sweep was not validated.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 8d4b7

The fix restores cleanup eligibility for archived worktrees without weakening the existing deletion checks. Including archived owners also improves protection for shared worktrees. No new externally reachable operation or material security regression was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The newly eligible assets are archived-thread worktrees subject to configured cleanup rules. Destructive operations remain restricted by managed canonical roots, project-root exclusion, linked-worktree checks and non-forced Git removal.

Trust Boundaries and Controls

  • observed — Projected path, branch and activity values do not directly authorize deletion. Cleanup validates filesystem containment, Git identity and cleanliness, single ownership, idle state, terminal use, provider sessions and unchanged settings. Archived threads now pass through these same controls, and archived sharers can cancel deletion.

Resilience and Maintainability Implications

  • observed — The guarded removal block uses a per-path, in-process semaphore lease with finalizer-based release. This supports local coordination but does not establish cross-process exclusion; the PR does not change this coordination mechanism.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: storage cleanup now reaches archived threads’ worktrees.
Description check ✅ Passed The description covers the problem, change, scope rationale, and focused verification results. It also states that a live cleanup sweep was not checked.
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.
✨ 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:S 10-29 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.

2 participants