Skip to content

fix(server): storage cleanup no longer keeps worktrees that a provider subagent used - #19147

Closed
jlcara wants to merge 1 commit into
pingdotgg:mainfrom
jlcara:fix/storage-cleanup-subagent-threads
Closed

jlcara wants to merge 1 commit into
pingdotgg:mainfrom
jlcara:fix/storage-cleanup-subagent-threads

Conversation

@jlcara

@jlcara jlcara commented Oct 13, 2026

Copy link
Copy Markdown

Problem

Automatic worktree cleanup keeps any worktree that more than one thread uses ("Shared by N threads"). Each subagent a provider spawns (Claude's Agent tool, Codex or Cursor native subagents) is stored as its own thread with its parent's worktreePath, and it is never archived or deleted with its parent. So once a thread starts one subagent, its worktree is kept forever, even after the user deletes the thread.

Reported in #19144. On my machine a run reported 0 removed, 111 kept. Subagent threads blocked 44 of 81 worktrees.

Repro: turn on "Delete unchanged worktrees", let the agent in a worktree thread start one subagent, settle or delete the thread, then run Settings → Storage → Delete now. The worktree is kept with "Shared by 2 threads".

Change

storageCleanup.ts ignores provider-native subagent threads (isProviderNativeSubagentThread) when it counts the threads that use a worktree. It counts in two places: the first check, and the recheck just before removal. Both now use one usesWorktree predicate.

delegate_task children (creationSource: "mcp") still count, because the user can message, archive and delete them separately. A subagent that is still working stays protected by its parent's active run and by the existing open-provider-session check.

Scope and approval

This is a small, focused fix for an obvious bug, filed as #19144. It changes one predicate in one server file and adds one test. It changes no contract, client, setting or default.

Verification

  • New test in apps/server/src/storageCleanup.test.ts, "does not count a provider-native subagent as sharing its parent's worktree". With a delegate_task child, the worktree is kept ("Shared by 2 threads"). With a provider subagent child listed first, the worktree is removed and reported under the parent thread.
    • Without the fix: × … expected { … } to match object { outcome: 'removed', … }
    • With the fix: Tests 1 passed
  • vp test run src/storageCleanup.test.ts: 55 passed, 2 failed. The same 2 tests ("compares the checked-out branch without a full status" and "aggregates expired artifacts and rotated logs…") also fail on unchanged main on my machine (macOS, Node 26.11 where the repo asks for Node 24), so this change does not cause them.
  • vp run typecheck in apps/server: exit 0. vp fmt and vp lint are clean on both files.
  • Not checked: a manual cleanup run in a dev build.

Made with Claude Opus 5.5 in Claude Code, run from T3 Code.

…r subagent used

Provider-native subagent threads run in their parent's checkout and are never archived or deleted with it, so counting them as sharing the worktree kept it forever. Closes pingdotgg#19144.
@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 13, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at eb96803

Macroscope's review found this PR approvable — This is a small, self-contained server bug fix that corrects worktree sharing counts for provider-native subagents while preserving protection for user-addressable delegated children. The behavior is covered by a focused regression test and does not change defaults, contracts, or deployment configuration.

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

@coderabbitai

coderabbitai Bot commented Oct 13, 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: 1bd7a9e3-acbe-4f7b-a01c-1148a0c2f0b6
📥 Commits

Reviewing files that changed from the base of the PR and between c616e16 and eb96803.

📒 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

Storage cleanup no longer counts provider-native subagent threads when checking whether a worktree is shared. An integration test compares cleanup outcomes for MCP-created and provider-native subagents that share a parent worktree.

Changes: Worktree cleanup

Layer / File(s) Summary
Exclude provider-native subagents from worktree sharing
apps/server/src/storageCleanup.ts, apps/server/src/storageCleanup.test.ts
A shared predicate excludes provider-native subagent threads from worktree grouping and the later sharing check. The integration test verifies that an MCP-created child keeps the worktree, while a provider-native child does not.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to eb968

Provider-native child threads no longer keep a worktree counted as shared, while active parent runs and matching open provider sessions remain protected. No concrete merge-blocking risk remains.

Security architecture risk: ⚪ Minimal · up to eb968

The cleanup change removes obsolete sharing blockers without changing public interfaces or granting additional authority. Active parent work, open provider sessions, and existing filesystem safeguards still prevent removal. No material security risk introduced by this change was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The destructive outcome is Git worktree removal under the existing cleanup policy. Candidates must remain inside managed worktree roots, cannot contain project checkouts, and must be linked worktrees rather than main checkouts.

Trust Boundaries and Controls

  • observed — Excluding child records does not bypass the independent active-use guard: any persisted non-stopped provider session with a cwd equal to or inside the worktree blocks deletion. Cleanup repeats this check after measurement.
  • observed — For live parents, cleanup rejects active runs, pending background work, runtime requests, and queued starts, then rechecks identity and activity. For deleted parents, unfinished durable deletion effects block removal. Final branch, commit, local-change, and settings checks remain in place.

Resilience and Maintainability Implications

  • inferred — The change preserves the existing failure-containment protocol rather than replacing it with child-count eligibility alone. The lease is process-local, not a cross-process lock; that limitation predates this PR.
Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Description check Passed The description covers the problem, change, scope and approval basis, verification results, known test failures, environment details, and agent attribution. It is specific and aligned with the pull re…
Title check Passed The title clearly and concisely identifies the main fix: storage cleanup no longer retains worktrees used by provider-native subagents.
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
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@juliusmarminge

Copy link
Copy Markdown
Member

Note

Grok responding on behalf of Julius.

Closing as superseded by #19748, which merged and closed #19144. Thanks for the report and the earlier fix PR.

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