Skip to content

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

Open
juliusmarminge wants to merge 6 commits into
mainfrom
takeover/pr-24949
Open

juliusmarminge wants to merge 6 commits into
mainfrom
takeover/pr-24949

Conversation

@juliusmarminge

Copy link
Copy Markdown
Member

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 discussion #13263.

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.

Takeover

Replaces #24949, with thanks to @Bil0000 for identifying the leak and establishing its reproduction and cleanup scope.
Rebuilt the deletion plan around a durable cleanup payload; Git stays in the VCS layer. Cleanup runs last, retries with capped backoff, deletes bounded batches, and never follows symbolic refs.
The original description above is preserved; its verification statements describe the contributor's work. The verification below applies to this replacement.

Evidence

Before/after matrix · Running-server transcript · Independent proof audit: PASS

Fresh isolated servers at before 9296c20e82cef89371e2a4fb433f31c314384261 and after de68dbb1e55355950669fbb33a67aa0f6e2a8c8e. Three threads each completed two ACP fixture-provider turns with real server checkpoint capture: nine refs before deletion. Before retained all three deleted-thread refs; after removed those three and preserved all six archived/retained refs and object IDs. A real ref lock failed cleanup; retry persisted across server restart and succeeded after unlock. Fixture turns made no file edits; this proof covers ref ownership/removal and recovery, not diff fidelity or commercial-model inference.

Verification log
  • Main merged by merge commit; no checkpoint/VCS/deletion source differences from the reviewed head.
  • Four independent full review rounds (Codex gpt-6.1-sol and Claude claude-opus-5-5); both final verdicts clear. Resolved finite retries, swallowed probe errors, output truncation, cleanup starvation, malformed Git config, and symbolic-ref dereferencing.
  • Regression checks failed before their fixes: missing outbox cleanup; lock contention beyond five attempts; malformed Git config; large packed-ref listing; cleanup ordering; symbolic target preservation.
  • Final scoped run: 205 tests passed across nine files.
  • GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1 vp run check:changed: passed format, lint, both knip passes, GraphQL generation, affected typechecks (two packages), tests (12 files). Process-local Git configuration isolation avoids this machine's remote URL rewrite; no global configuration changed.
  • Proof audit passed at the exact after SHA. Servers stopped by captured PIDs; no live T3 userdata accessed.
  • Windows inspected only. Existing missing-Git skip behavior retained. CI owns full suites and build.

Implementation and tests: Claude Opus 5.5 in the Claude Agent harness. PR lead, independent review and proof: GPT-6.1 Sol in the Codex harness.

juliusmarminge and others added 6 commits October 26, 2026 00:23
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Thread deletion now enqueues a replay-safe checkpoint.cleanup outbox effect
carrying the thread's checkpoint cwds and scope IDs. ResourceCleanupService
deletes every ref under the thread's V1 turn namespace and each V2 scope
namespace in one git update-ref transaction, so a locked ref fails the
effect and the worker retries it instead of silently skipping.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Lock contention outlasting the worker's attempt budget terminalized the
cleanup, and detection errors were read as "not a repository", so the
effect reported success without deleting refs. Cleanup now retries on the
existing capped backoff and skips only directories that no longer exist
or are not Git repositories.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r cleanup

Ref listing ran through VcsProcess's 1 MB output cap, which truncates
silently, so a namespace with ~20k refs was acknowledged with thousands
left. Enumerate and delete in bounded --count batches until none remain,
failing (not truncating) if one listing ever overflows.

Checkpoint cleanup retries until it succeeds and the outbox orders effects
per thread, so it now queues after scheduled-task, terminal, preview, and
attachment cleanup instead of blocking them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Repository detection reported a repository with a malformed config as
not a repository, so cleanup succeeded and left the deleted thread's refs
behind. The namespace sweep now skips a cwd only when Git itself says it is
not a repository (or Git is absent); any other failure keeps the outbox
effect retryable. Deletes also pass --no-deref so a symbolic ref in a
deleted namespace never removes another thread's ref.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Oct 26, 2026
@github-actions github-actions Bot added the size:L 100-499 changed lines (additions + deletions). label Oct 26, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — Thread and project deletion now enqueue destructive Git checkpoint-ref cleanup across multiple workspaces, with transactional deletion and retry-until-success behavior; rollback error handling also changes. The intent is a focused cleanup fix with strong tests, but its cross-cutting production impact is broader than a small self-contained change.

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

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB −7 B (−0.1%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB −7 B (−0.6%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB +7 B (+0.1%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB +7 B (+0.6%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: 06e80ac · PR result: de68dbb · Source CI: failure

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Oct 26, 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 06e80ac and de68dbb.

Files selected for processing: 17
  • apps/server/src/checkpointing/CheckpointStore.ts
  • apps/server/src/checkpointing/Utils.ts
  • apps/server/src/guest/GuestShareService.test.ts
  • apps/server/src/orchestration-v2/CheckpointRollbackService.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/Orchestrator.ts
  • apps/server/src/orchestration-v2/ResourceCleanupService.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/project/ProjectService.deletion.test.ts
  • apps/server/src/project/ProjectService.ts
  • apps/server/src/vcs/GitVcsDriver.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; 0 remain after this review.

Walkthrough

Thread deletion now schedules cleanup of checkpoint references in relevant workspaces. The cleanup effect removes thread and scope namespaces, skips missing or non-repository directories, and retries failures until cleanup succeeds. Git namespace deletion validates inputs and processes refs in batches.

Changes: Checkpoint cleanup

Layer / File(s) Summary
Reference namespace deletion
apps/server/src/checkpointing/*, apps/server/src/orchestration-v2/CheckpointService.ts, apps/server/src/orchestration-v2/CheckpointRollbackService.ts, apps/server/src/vcs/*
Thread and scope namespace helpers provide reference prefixes. The checkpoint store and Git driver support namespace deletion, with validation and batched ref removal. Rollback logs stale-ref deletion failures. Tests cover large namespaces and symbolic refs.
Thread deletion cleanup scheduling
apps/server/src/orchestration-v2/EffectOutbox.ts, apps/server/src/orchestration-v2/ThreadDeletion.ts, apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/project/ProjectService.ts, apps/server/src/orchestration-v2/ThreadDeletion.test.ts, apps/server/src/project/ProjectService.deletion.test.ts
The deletion planner gathers thread worktree, project workspace, and checkpoint-scope paths and adds a cleanup effect. Deletion callers provide project workspace and scope data. The outbox accepts and requeues the effect after process loss.
Checkpoint cleanup execution and retries
apps/server/src/orchestration-v2/ResourceCleanupService.ts, apps/server/src/orchestration-v2/EffectWorker.ts, apps/server/src/orchestration-v2/runtimeLayer.test.ts, apps/server/src/guest/GuestShareService.test.ts
The cleanup service deletes thread and scope namespaces in existing working directories. The worker retries cleanup effects until success. Tests cover lock retries, process-loss recovery, missing directories, and Git errors.

Priority: Normal

Estimated code review effort: 4 (Complex) | ~40 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ThreadDeletion
  participant EffectOutbox
  participant EffectWorker
  participant ResourceCleanupService
  participant CheckpointStore
  ThreadDeletion->>EffectOutbox: enqueue checkpoint.cleanup
  EffectOutbox->>EffectWorker: provide cleanup effect
  EffectWorker->>ResourceCleanupService: pass thread, workspaces, and scope IDs
  ResourceCleanupService->>CheckpointStore: delete namespaces in existing directories
Loading

Possibly related issues

  • Issue 24083 reports unpruned checkpoint refs from deleted threads. This change schedules deletion of thread and scope refs, including legacy turn refs.

Suggested labels: bugfix, size:L

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

Merge Risk: Minimal · up to de68d

No concrete issue remains that should block merging after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: removing checkpoint references when threads are deleted.
Description check Passed The description covers the problem, implementation, scope references, verification results, known limitations, and agent attribution. It provides detailed evidence and test results that align with the…
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 covers the problem, implementation, scope references, verification results, known limitations, and agent attribution. It provides detailed evidence and test results that align with the required template.

Finishing Touches: 💡 2
⚔️ Resolve merge conflicts: 💡
  • Resolve merge conflict in branch takeover/pr-24949
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • 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 bugfix Fix PR opened by an issue lead; enters the PR loop pr:gate-failed A land gate failed labels Oct 26, 2026

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

bugfix Fix PR opened by an issue lead; enters the PR loop pr:gate-failed A land gate failed size:L 100-499 changed lines (additions + deletions). 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.

1 participant