fix: prune stale VCS refs and speed listRefs#94
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1acd5ac746
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const sharedByLiveThread = rows.some((other) => { | ||
| if (other.threadId === row.threadId || other.deletedAt !== null) { | ||
| return false; |
There was a problem hiding this comment.
When two old archived/stopped threads still reference the same removable worktree path, each row is eligible for the archived-age reap, but this overlap guard treats the other archived row as a live owner because it only ignores deletedAt !== null. As a result selectStaleWorktreeReapCandidates returns no candidate for either row, so shared archived worktrees can leak indefinitely instead of being reaped after ARCHIVED_WORKTREE_REAP_AGE_MS.
Useful? React with 👍 / 👎.
| }); | ||
| } | ||
|
|
||
| yield* git.pruneWorktrees(projectRoot).pipe( |
There was a problem hiding this comment.
For stale rows whose worktree directory was manually deleted, git worktree list --porcelain reports the entry as prunable; after git worktree prune removes that metadata, a later git worktree remove <path> exits 128 with not a working tree. Since this sweep prunes project worktrees here before reapWorktrees can dispatch the metadata clear, those threads keep a non-null worktreePath and every later sweep only logs worktree-reap-skipped.
Useful? React with 👍 / 👎.
| const checkpointResult = yield* checkpointStore.pruneCheckpointRefs({ | ||
| cwd: projectRoot, | ||
| keepPerThread: CHECKPOINT_REFS_KEEP_PER_THREAD, | ||
| }); |
There was a problem hiding this comment.
Unlike the following git.pruneWorktrees call, this checkpoint-prune effect is not caught inside pruneProjectRepository; if one repository is locked, corrupt, or git for-each-ref times out, the exception exits the loop in runSweep before later project roots are processed and before the global worktree reap runs. In a multi-project server, one bad repo therefore disables checkpoint pruning and stale worktree cleanup for every other project for that sweep.
Useful? React with 👍 / 👎.
| "for-each-ref", | ||
| "--format=%(refname)%09%(committerdate:unix)", | ||
| "refs/heads", | ||
| "refs/remotes", |
There was a problem hiding this comment.
With a stale remote-tracking ref whose target object has been pruned, git for-each-ref ... refs/remotes exits 128, so this new combined ref scan makes listRefs fail before it can return any local branches. The previous implementation listed local branches separately and tolerated remote/recency lookup failures, so branch pickers can now break entirely in repositories that only have a bad remote ref.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48d33d05f2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| path: candidate.path, | ||
| }), | ||
| ), | ||
| Effect.catchCause((cause) => |
There was a problem hiding this comment.
When the thread.meta.update dispatch fails after removeWorktree has already succeeded, this catch only logs and leaves the row with its old worktreePath. On the next sweep the git worktree entry has already been removed, so git worktree remove <path> fails and the outer catch only logs worktree-reap-skipped, meaning one transient dispatch/DB failure can leave the thread metadata stale indefinitely.
Useful? React with 👍 / 👎.
| INNER JOIN projection_projects p ON p.project_id = t.project_id | ||
| LEFT JOIN projection_thread_sessions s ON s.thread_id = t.thread_id | ||
| LEFT JOIN provider_session_runtime r ON r.thread_id = t.thread_id | ||
| WHERE p.deleted_at IS NULL |
There was a problem hiding this comment.
Filtering out deleted projects here prevents cleanup for threads deleted as part of a forced project delete: the decider emits thread.deleted events and then marks the project deleted, while ThreadDeletionReactor only stops sessions/terminals and does not remove worktrees. Those deleted thread rows can still have removable worktree_paths, but this query never returns them, so their worktrees are never reaped.
Useful? React with 👍 / 👎.
| WHERE thread_id = ${threadId} | ||
| AND checkpoint_turn_count IS NOT NULL | ||
| ) checkpoint_rows | ||
| WHERE checkpoint_rows.checkpoint_rank <= ${MAX_THREAD_CHECKPOINTS} |
There was a problem hiding this comment.
When a thread has more than MAX_THREAD_CHECKPOINTS checkpoints, this filtered context no longer contains older turn IDs. A delayed or replayed turn.diff.updated for one of those older turns makes ProviderRuntimeIngestion.hasCheckpointForTurn think the turn is untracked, so it dispatches a new thread.turn.diff.complete with maxCheckpointTurnCount + 1; ProjectionPipeline then updates that old turn and sets it as the latest turn, corrupting checkpoint ordering for long-running threads.
Useful? React with 👍 / 👎.
| (row.sessionStatus !== null && !REAPABLE_SESSION_STATUSES.has(row.sessionStatus)) || | ||
| (row.runtimeStatus !== null && !REAPABLE_SESSION_STATUSES.has(row.runtimeStatus)); | ||
|
|
||
| return hasLiveSessionStatus || row.pendingApprovalCount > 0 || row.pendingUserInputCount > 0; |
There was a problem hiding this comment.
Deleted threads can still have pending_approval_count or pending_user_input_count because thread.deleted does not clear those projections, and the decider allows deletion while requests are pending. Since this active check still treats those counters as blockers even when deletedAt is set, a deleted thread in an active project with a removable worktree can remain ineligible forever instead of being reaped after the archived/deleted age.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e41e9fe9d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .removeWorktree({ | ||
| cwd: candidate.projectCwd, | ||
| path: candidate.path, | ||
| }) |
There was a problem hiding this comment.
For stale archived/deleted worktrees that still contain agent changes, this call uses git worktree remove without force, and git worktree remove -h says --force is what removes dirty or locked worktrees. In that scenario the removal fails, git worktree prune does not unregister an existing dirty worktree, isWorktreeRegistered remains true, and the candidate is skipped on every sweep so its metadata and directory never get reaped. Pass force: true for these owned cleanup candidates or otherwise handle dirty worktree removal explicitly.
Useful? React with 👍 / 👎.
| .pipe(Effect.result); | ||
|
|
||
| if (Result.isFailure(result)) { | ||
| yield* Effect.logWarning("vcs.maintenance.worktree-list-failed", { |
There was a problem hiding this comment.
When the original project checkout has been deleted or moved, this git worktree list call fails and the helper returns true, so the sweep then tries gitWorkflow.removeWorktree with the same missing projectCwd; that also fails before reaching the candidate path, the second registration check returns true again, and the thread's stale worktree metadata is never cleared. This leaves deleted/archived threads stuck forever after a project root disappears, even if their removable worktree path is already gone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76cbdeb4c2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| WHERE thread_id = ${threadId} | ||
| AND checkpoint_turn_count IS NOT NULL | ||
| ) checkpoint_rows | ||
| WHERE checkpoint_rows.checkpoint_rank <= ${MAX_THREAD_CHECKPOINTS} |
There was a problem hiding this comment.
When a thread has more than MAX_THREAD_CHECKPOINTS checkpoints, this returns the newest 500 (for example counts 6..505), but the review UI requests a per-turn diff as checkpointTurnCount - 1 -> checkpointTurnCount. Selecting the oldest exposed checkpoint therefore asks for turn 5 -> 6, while turn 5 is filtered out of getThreadCheckpointContext; after the maintenance sweep the new pruning budget also deletes that predecessor ref. This leaves a visible retained checkpoint whose per-turn diff fails with an unavailable from ref, so either include the predecessor in the diff context/retention or hide that boundary checkpoint.
Useful? React with 👍 / 👎.
| const localRefListResult = yield* executeGit( | ||
| "GitVcsDriver.listRefs.localForEachRef", | ||
| input.cwd, | ||
| ["branch", "--no-color", "--no-column"], | ||
| ["for-each-ref", `--format=${refListFormat}`, "refs/heads"], |
There was a problem hiding this comment.
If a local branch ref points at a pruned/missing object, this new git for-each-ref ... refs/heads path exits 128 and listRefs fails before returning any healthy local branches. I reproduced Git's CLI behavior in that state: git branch --no-color --no-column still exits 0 and lists the valid branch, while git for-each-ref --format=%(refname)%09%(committerdate:unix) refs/heads aborts with fatal: missing object ...; this regresses the old separate name listing plus best-effort recency lookup, so one stale local ref can break the branch picker entirely.
Useful? React with 👍 / 👎.
| (row.sessionStatus !== null && !REAPABLE_SESSION_STATUSES.has(row.sessionStatus)) || | ||
| (row.runtimeStatus !== null && !REAPABLE_SESSION_STATUSES.has(row.runtimeStatus)); | ||
|
|
||
| return hasLiveSessionStatus || row.pendingApprovalCount > 0 || row.pendingUserInputCount > 0; |
There was a problem hiding this comment.
When a thread is archived while an approval or user-input request is pending, the archive path I checked only requires the thread not already be archived and the projection handler just sets archivedAt/updatedAt, so those pending counters can remain nonzero. This active check still treats archived rows with stale pending counts as live, which means an archived removable worktree in that state never becomes eligible for the ARCHIVED_WORKTREE_REAP_AGE_MS cleanup.
Useful? React with 👍 / 👎.
| readonly projectRootExists: boolean; | ||
| readonly worktreePathExists: boolean; | ||
| }): boolean { | ||
| return input.projectRootExists || input.worktreePathExists; |
There was a problem hiding this comment.
When the project directory still exists but has lost its .git directory and the candidate worktree path is already gone, git worktree list --porcelain exits 128 with fatal: not a git repository, but this helper returns true solely because projectRootExists is true. The sweep then treats the candidate as registered, removeWorktree/pruneWorktrees fail against the non-repo cwd, and the same stale worktreePath is retained forever instead of clearing metadata for the already-deleted worktree.
Useful? React with 👍 / 👎.
|
Superseded by #99. |
Summary
Validation