Skip to content

fix(server): remove checkpoint refs when deleting threads - #24949

Closed
Bil0000 wants to merge 1 commit into
pingdotgg:mainfrom
Bil0000:t3/explain-discussion-complaints
Closed

Bil0000 wants to merge 1 commit into
pingdotgg:mainfrom
Bil0000:t3/explain-discussion-complaints

Conversation

@Bil0000

@Bil0000 Bil0000 commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Deleting a thread left its Git checkpoint refs behind, which cluttered Git tools and kept old snapshot data reachable. Deletion now queues cleanup after checkpoint writes, removes only that thread's old and current refs, and handles removed worktrees through the project repo. Archived threads keep their restore history.

Addresses #13263 (comment).

Related bug report: #24083.

Verified with failing regression tests before the fix, real Git tests for packed refs, shared worktrees and lock failures, a capture-order test, and vp run check:changed.

Refs from threads deleted before this change need manual cleanup. Git's normal maintenance reclaims the unused data.

Built with GPT-6.1 Sol high/priority in the Codex harness, with Astra high design and review.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 25, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR adds checkpoint cleanup to thread deletion across worktrees and project repositories, including new outbox sequencing and atomic Git ref deletion. The behavior is well-tested but cross-cutting and destructive on an existing production path, warranting human review.

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

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between c5332bf and 541e43b.

Files selected for processing: 19
  • apps/server/src/checkpointing/CheckpointStore.test.ts
  • apps/server/src/checkpointing/CheckpointStore.ts
  • apps/server/src/checkpointing/Utils.ts
  • apps/server/src/guest/GuestShareService.test.ts
  • apps/server/src/orchestration-v2/CheckpointCaptureService.test.ts
  • apps/server/src/orchestration-v2/CheckpointCaptureService.ts
  • apps/server/src/orchestration-v2/CheckpointService.ts
  • apps/server/src/orchestration-v2/EffectOutbox.ts
  • apps/server/src/orchestration-v2/EffectWorker.ts
  • apps/server/src/orchestration-v2/ResourceCleanupService.test.ts
  • apps/server/src/orchestration-v2/ResourceCleanupService.ts
  • apps/server/src/orchestration-v2/RunFinalizationQueue.test.ts
  • apps/server/src/orchestration-v2/ThreadDeletion.test.ts
  • apps/server/src/orchestration-v2/ThreadDeletion.ts
  • apps/server/src/orchestration-v2/runtimeLayer.test.ts
  • apps/server/src/orchestration-v2/runtimeLayer.ts
  • apps/server/src/project/ProjectService.deletion.test.ts
  • apps/server/src/vcs/GitVcsDriver.ts
  • apps/server/src/vcs/VcsDriver.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

Thread deletion now queues checkpoint-ref cleanup. The cleanup service removes matching refs from relevant Git repositories. Git validates and deletes explicit refs and prefix matches atomically. Checkpoint capture now returns early for deleted threads.

Changes: Checkpoint Cleanup

Layer / File(s) Summary
Checkpoint ref deletion
apps/server/src/checkpointing/*, apps/server/src/orchestration-v2/CheckpointService.ts, apps/server/src/vcs/*
The deletion input accepts ref prefixes. Thread and scope prefix helpers provide ref paths. Git validates refs and prefixes, expands matching refs, and deletes the collected refs in one transaction. Tests cover exact and prefix deletion, repeated calls, locked refs, invalid inputs, and preservation of unrelated refs.
Deleted thread cleanup
apps/server/src/orchestration-v2/ResourceCleanupService.ts, apps/server/src/orchestration-v2/ResourceCleanupService.test.ts, apps/server/src/orchestration-v2/runtimeLayer.ts, apps/server/src/orchestration-v2/runtimeLayer.test.ts
ResourceCleanupService skips cleanup for threads that are not deleted. For deleted threads, it checks relevant directories and removes matching refs from existing repositories. The service dependencies and tests are updated.
Deletion effect and capture handling
apps/server/src/orchestration-v2/{EffectOutbox.ts,EffectWorker.ts,ThreadDeletion.ts,ThreadDeletion.test.ts,RunFinalizationQueue.test.ts,CheckpointCaptureService.ts,CheckpointCaptureService.test.ts}, apps/server/src/project/ProjectService.deletion.test.ts, apps/server/src/guest/GuestShareService.test.ts
Thread deletion plans a replay-safe checkpoint.cleanup effect, and the effect worker calls the cleanup service. Tests cover effect ordering and project deletion. Checkpoint capture returns early for deleted threads, with test coverage for deleted and rolled-back runs.

Priority: Low

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ThreadDeletion
  participant EffectOutbox
  participant EffectWorker
  participant ResourceCleanupService
  participant GitVcsDriver
  ThreadDeletion->>EffectOutbox: enqueue checkpoint.cleanup
  EffectOutbox->>EffectWorker: provide cleanup effect
  EffectWorker->>ResourceCleanupService: cleanupCheckpointRefs(threadId)
  ResourceCleanupService->>GitVcsDriver: deleteCheckpointRefs(prefixes)
Loading

Possibly related issues

Suggested labels: bugfix, size:L

Suggested reviewers: yash-singh1, maria-rcks, juliusmarminge

Merge Risk: Minimal · up to 541e4

Thread deletion can proceed with checkpoint-ref cleanup; no issue identified here prevents merging after normal checks.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description explains the problem and change, links the related discussion and bug report, and lists focused verification. It does not provide the explicit maintainer approval required by the Scope… Add the maintainer’s explicit approval of the change’s direction and scope, including the approval comment. Also state the observed results of the verification checks.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: removing checkpoint refs when threads are deleted.
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.
Full details: Description check

Explanation

The description explains the problem and change, links the related discussion and bug report, and lists focused verification. It does not provide the explicit maintainer approval required by the Scope and approval section.

Resolution

Add the maintainer’s explicit approval of the change’s direction and scope, including the approval comment. Also state the observed results of the verification checks.

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 juliusmarminge added takeover t3 automation: takeover pr:in-progress A lead is working on this PR labels Oct 26, 2026
@juliusmarminge juliusmarminge self-assigned this Oct 26, 2026

Copy link
Copy Markdown
Member

Taking over this one

Copy link
Copy Markdown
Member

Thanks @Bil0000 for identifying the checkpoint-ref leak and establishing the reproduction, scoped deletion, and archived-history requirements. Rebuilt this as #25182: a durable cleanup payload, bounded Git ref deletion, and cleanup that retries after lock contention and restart without blocking other resource cleanup. I compared the original diff against the replacement; legacy and current refs, removed-worktree fallback, capture ordering, and project deletion are covered. The original test collection was replaced with focused behavioral regressions and isolated before/after server proof. Closing this in favor of the credited replacement.

@juliusmarminge juliusmarminge removed the pr:in-progress A lead is working on this PR label Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). takeover t3 automation: takeover vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants