fix(host-service): decide workspace-delete cleanup on git state, not error text#5138
Conversation
…error text Workspace deletes were failing for users in two ways after the #4801 cleanup reorder: hard "Failed to remove worktree" errors that aborted the delete, and scary "Failed to delete branch ... not found" warnings on deletes that had actually succeeded. Both steps now branch on git's real state instead of parsing locale- and version-dependent error strings: - Worktree removal: run `worktree remove --force --force` against the canonical (realpath) path so a symlinked stored path (macOS /var -> /private/var) still matches its registration, then check `git worktree list --porcelain`. Continue if the path is no longer registered; throw only if git still tracks a live worktree we couldn't remove (preserving the no-orphan-disk guarantee). No blanket `worktree prune` -- `--force --force` already unregisters a missing-dir worktree, and a global prune would clobber other workspaces' stale metadata. - Branch delete: check existence with `git show-ref --verify --quiet`. An absent ref is the goal of deleteBranch, so treat it as success instead of surfacing a warning. A ref that exists but fails `-D` still warns. Unit mock updated to model the new `worktree list` / `show-ref` reads.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWorkspace cleanup now adds git path normalization and explicit state verification to make local workspace cleanup deterministic. Worktree removal computes canonical paths to handle symlinks, then verifies removal by querying git's worktree registry; branch deletion first checks branch existence before attempting removal. ChangesDeterministic Workspace Cleanup
Sequence DiagramsequenceDiagram
participant WorkspaceCleanup
participant Git
participant CloudBackend
WorkspaceCleanup->>WorkspaceCleanup: canonicalize worktree path
WorkspaceCleanup->>Git: git worktree remove --force --force (canonical path)
WorkspaceCleanup->>Git: git worktree list (verify removal)
alt still registered
WorkspaceCleanup->>WorkspaceCleanup: throw INTERNAL_SERVER_ERROR
else removed from registry
WorkspaceCleanup->>CloudBackend: proceed to delete cloud row
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 3 individual chapters for you:
Chapters generated by Stage for commit f16641c on Jun 5, 2026 4:44am UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/host-service/test/workspace-cleanup.test.ts (1)
67-70: ⚡ Quick winAdd a regression test for unexpected
show-reffailuresPlease add a case where
git.raw(["show-ref", ...])fails for reasons other than missing ref and assert cleanup reports a warning instead ofbranchDeleted: true. This guards the new branch-existence contract and prevents silent false-success regressions.Also applies to: 83-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/test/workspace-cleanup.test.ts` around lines 67 - 70, Add a regression test case to workspace-cleanup.test.ts that simulates git.raw(["show-ref", ...]) failing with an error that is not "ref not found" (e.g., throw new Error("permission denied")) by changing the branchExists mock (the async mock around spec.branchExists currently throwing "ref not found") to throw a non-ref-not-found error for one test case; then assert that the cleanup result reports a warning (not branchDeleted: true) and that no silent success is returned. Repeat the same adjustment for the similar mock case at the other location referenced (the block around lines 83-85) so both paths validate that unexpected show-ref failures produce a warning instead of treating the branch as deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts`:
- Around line 399-409: The helper localBranchExists currently swallows all
errors from git.raw and returns false; change it to only return false for the
explicit "ref not found" case and rethrow other errors so they surface as
warnings. Update localBranchExists (the call to
git.raw(["show-ref","--verify","--quiet",`refs/heads/${branch}`])) to catch the
thrown error, inspect the Git error (e.g. check error.exitCode === 1 or the
error.message/stderr that indicates the ref does not exist), return false for
that specific case, and rethrow the error for any other exit codes or messages.
---
Nitpick comments:
In `@packages/host-service/test/workspace-cleanup.test.ts`:
- Around line 67-70: Add a regression test case to workspace-cleanup.test.ts
that simulates git.raw(["show-ref", ...]) failing with an error that is not "ref
not found" (e.g., throw new Error("permission denied")) by changing the
branchExists mock (the async mock around spec.branchExists currently throwing
"ref not found") to throw a non-ref-not-found error for one test case; then
assert that the cleanup result reports a warning (not branchDeleted: true) and
that no silent success is returned. Repeat the same adjustment for the similar
mock case at the other location referenced (the block around lines 83-85) so
both paths validate that unexpected show-ref failures produce a warning instead
of treating the branch as deleted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c06ac882-1796-4dd9-aa33-cf142feccb80
📒 Files selected for processing (2)
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.tspackages/host-service/test/workspace-cleanup.test.ts
Greptile SummaryThis PR fixes two workspace-delete failure modes introduced by #4801: hard failures from error-text–dependent worktree removal, and false "branch not found" warnings when a branch was already gone. It replaces both with authoritative git-state checks — consulting
Confidence Score: 3/5The cleanup logic is sound in the common case, but a git error on the post-remove worktree list call is silently treated as not registered, allowing the cloud row to be deleted while the worktree directory remains on disk. The authoritative-git-state approach is the right direction, but listGitWorktrees unconditionally swallows its own errors and returns an empty slice. Any transient or structural failure of git worktree list makes isRegisteredWorktree return false, causing worktreeRemoved = true and letting the cloud delete proceed with the directory still present. There is no test covering this failure path. The core logic in workspace-cleanup.ts around isRegisteredWorktree deserves a second look; worktree-list.ts may need a throwing variant or the caller should not tolerate an empty result when the list command itself errors.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts | Replaces error-string-based worktree/branch cleanup with git-state checks via listGitWorktrees/show-ref; the worktree check delegates to a helper that silently returns [] on any git error, which can falsely indicate not registered and allow cloud delete to proceed with the directory still on disk. |
| packages/host-service/test/workspace-cleanup.test.ts | Test mock updated to model worktree list and show-ref git reads; the key failure scenario (worktree list itself throwing) is not covered, leaving the silent-pass behavior untested. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[destroy called] --> B{worktreePath exists on disk?}
B -- No --> C[worktreeRemoved = true]
B -- Yes --> D[open git repo]
D -- throws --> E{path still exists?}
E -- Yes --> F[throw INTERNAL_SERVER_ERROR]
E -- No --> C
D -- ok --> G[git worktree remove --force --force canonicalPath]
G --> H[git worktree list --porcelain via listGitWorktrees]
H -- list fails silently returns [] --> I[isRegisteredWorktree = false]
H -- list succeeds --> J{path still in registry?}
J -- Yes --> F
J -- No --> I
I --> K[worktreeRemoved = true]
K --> L[cloud delete]
C --> L
L --> M{deleteBranch?}
M -- No --> N[sqlite cleanup]
M -- Yes --> O[show-ref --verify refs/heads/branch]
O -- throws any error --> P[branchDeleted = true, no delete attempted]
O -- success --> Q[git branch -D]
Q -- ok --> R[branchDeleted = true]
Q -- throws --> S[warning added, branchDeleted = false]
P --> N
R --> N
S --> N
N --> T[return result]
Comments Outside Diff (1)
-
packages/host-service/test/workspace-cleanup.test.ts, line 374-413 (link)Missing test:
worktree listfailure during the post-remove checkThe test covers
worktreeRemovethrowing +worktreeListreturning the path still registered (the normal failure path). It does not coverworktreeListitself throwing an exception — the case wherelistGitWorktreesswallows the error and returns[], causing the code to conclude removal succeeded and proceed to cloud delete with the directory still on disk. Adding a test whereworktreeList: () => Promise.reject(new Error("list boom"))would surface the silent-failure described in the related comment.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/test/workspace-cleanup.test.ts Line: 374-413 Comment: **Missing test: `worktree list` failure during the post-remove check** The test covers `worktreeRemove` throwing + `worktreeList` returning the path still registered (the normal failure path). It does not cover `worktreeList` itself throwing an exception — the case where `listGitWorktrees` swallows the error and returns `[]`, causing the code to conclude removal succeeded and proceed to cloud delete with the directory still on disk. Adding a test where `worktreeList: () => Promise.reject(new Error("list boom"))` would surface the silent-failure described in the related comment. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts:322-331
**`listGitWorktrees` failure silently looks like "not registered"**
`listGitWorktrees` (in `worktree-list.ts`) catches all exceptions and returns `[]`. When the `git worktree list --porcelain` call fails for any reason (broken index, lock contention, transient IO error), `isRegisteredWorktree` receives an empty list, returns `false`, and the code falls through to `worktreeRemoved = true` — allowing the cloud delete to proceed even if the directory is still on disk. This is the exact orphan-disk scenario #4801 was designed to prevent.
A `git worktree list` failure should be treated as "state unknown" and block the cloud delete, not silently pass. Consider propagating the error from `listGitWorktrees` in this context (or wrapping the call to throw instead of return `[]`).
### Issue 2 of 3
packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts:399-408
**`localBranchExists` conflates "ref not found" with git infrastructure errors**
`git show-ref --verify` exits with code 1 when the ref is absent, but exits with code 128 (`fatal:`) when the repo is inaccessible or corrupted. Both cause `simple-git` to throw, so both return `false` here. If git is momentarily unavailable, `localBranchExists` returns `false`, `branchDeleted` is set to `true`, and the branch is never actually deleted — producing a misleading return value to the caller.
### Issue 3 of 3
packages/host-service/test/workspace-cleanup.test.ts:374-413
**Missing test: `worktree list` failure during the post-remove check**
The test covers `worktreeRemove` throwing + `worktreeList` returning the path still registered (the normal failure path). It does not cover `worktreeList` itself throwing an exception — the case where `listGitWorktrees` swallows the error and returns `[]`, causing the code to conclude removal succeeded and proceed to cloud delete with the directory still on disk. Adding a test where `worktreeList: () => Promise.reject(new Error("list boom"))` would surface the silent-failure described in the related comment.
Reviews (1): Last reviewed commit: "fix(host-service): decide workspace-dele..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (await isRegisteredWorktree(git, local.worktreePath)) { | ||
| // git still tracks a live worktree here — removal genuinely | ||
| // failed. Keep the cloud row so the workspace stays visible and | ||
| // retryable instead of orphaning disk past the cloud commit point. | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: `Failed to remove worktree at ${local.worktreePath}`, | ||
| }); | ||
| } | ||
| worktreeRemoved = true; |
There was a problem hiding this comment.
listGitWorktrees failure silently looks like "not registered"
listGitWorktrees (in worktree-list.ts) catches all exceptions and returns []. When the git worktree list --porcelain call fails for any reason (broken index, lock contention, transient IO error), isRegisteredWorktree receives an empty list, returns false, and the code falls through to worktreeRemoved = true — allowing the cloud delete to proceed even if the directory is still on disk. This is the exact orphan-disk scenario #4801 was designed to prevent.
A git worktree list failure should be treated as "state unknown" and block the cloud delete, not silently pass. Consider propagating the error from listGitWorktrees in this context (or wrapping the call to throw instead of return []).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts
Line: 322-331
Comment:
**`listGitWorktrees` failure silently looks like "not registered"**
`listGitWorktrees` (in `worktree-list.ts`) catches all exceptions and returns `[]`. When the `git worktree list --porcelain` call fails for any reason (broken index, lock contention, transient IO error), `isRegisteredWorktree` receives an empty list, returns `false`, and the code falls through to `worktreeRemoved = true` — allowing the cloud delete to proceed even if the directory is still on disk. This is the exact orphan-disk scenario #4801 was designed to prevent.
A `git worktree list` failure should be treated as "state unknown" and block the cloud delete, not silently pass. Consider propagating the error from `listGitWorktrees` in this context (or wrapping the call to throw instead of return `[]`).
How can I resolve this? If you propose a fix, please make it concise.| async function localBranchExists( | ||
| git: Awaited<ReturnType<HostServiceContext["git"]>>, | ||
| branch: string, | ||
| ): Promise<boolean> { | ||
| try { | ||
| await git.raw(["show-ref", "--verify", "--quiet", `refs/heads/${branch}`]); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
localBranchExists conflates "ref not found" with git infrastructure errors
git show-ref --verify exits with code 1 when the ref is absent, but exits with code 128 (fatal:) when the repo is inaccessible or corrupted. Both cause simple-git to throw, so both return false here. If git is momentarily unavailable, localBranchExists returns false, branchDeleted is set to true, and the branch is never actually deleted — producing a misleading return value to the caller.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts
Line: 399-408
Comment:
**`localBranchExists` conflates "ref not found" with git infrastructure errors**
`git show-ref --verify` exits with code 1 when the ref is absent, but exits with code 128 (`fatal:`) when the repo is inaccessible or corrupted. Both cause `simple-git` to throw, so both return `false` here. If git is momentarily unavailable, `localBranchExists` returns `false`, `branchDeleted` is set to `true`, and the branch is never actually deleted — producing a misleading return value to the caller.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Architecture diagram
sequenceDiagram
participant Client as API Client
participant Handler as runDestroy()
participant Git as Git CLI
participant RegCheck as isRegisteredWorktree()
participant BrCheck as localBranchExists()
Note over Handler,BrCheck: NEW: Cleanup success decided on git state, not error text
Client->>Handler: destroy workspace
Note over Handler: Canonicalize worktree path<br/>(resolve /var↔/private/var)
Handler->>Handler: git worktree remove --force --force <canonicalPath><br/>(best‑effort, ignore error)
Handler->>RegCheck: is this path still registered?
RegCheck->>Git: git worktree list --porcelain
Git-->>RegCheck: output (realpath‑canonicalized entries)
RegCheck-->>Handler: true / false
alt still registered → genuine failure
Handler-->>Client: TRPCError(INTERNAL_SERVER_ERROR)
else unregistered → success
Handler->>Handler: worktreeRemoved = true
end
Handler->>BrCheck: does branch exist?
BrCheck->>Git: git show-ref --verify --quiet refs/heads/<branch>
alt exit code 0 → branch exists
BrCheck-->>Handler: true
Handler->>Git: git branch -D <branch>
alt delete OK
Git-->>Handler: success
Handler->>Handler: branchDeleted = true
else delete fails
Git-->>Handler: error
Handler->>Handler: push warning
end
else exit code non‑zero → branch absent
BrCheck-->>Handler: false
Handler->>Handler: branchDeleted = true (no warning)
end
Handler-->>Client: { success, branchDeleted, warnings }
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…success Address review feedback: both state checks swallowed all errors and returned the "happy" value, so a transient git failure read as false success — the orphan-disk / silent-skip outcome this change is meant to prevent. - isRegisteredWorktree now calls `git.raw` directly instead of the swallowing `listGitWorktrees`, so a failed `worktree list` throws. The worktree step treats that "state unknown" as still-registered and blocks the cloud delete rather than proceeding. - localBranchExists uses `git branch --list` (exit 0 whether or not the ref exists), so a thrown error is a real git failure, not a missing ref. The branch step's try/catch turns such a failure into a warning instead of a false `branchDeleted = true`. Unit mock models `branch --list` as the existence probe.
|
Addressed in f16641c. Both helpers were swallowing all git errors and returning the happy value, so a transient git failure read as false success — the exact orphan-disk / silent-skip outcome this PR prevents.
|
1 participant
Problem
Users have been hitting workspace-delete failures since #4801's cleanup reorder, in two distinct shapes:
Failed to remove worktree at …thrown errors that abort the whole delete. [codex] Fix worktree delete cleanup ordering #4801 converted worktree-removal failure from a best-effort warning into a hard throw and dropped fix(host-service): tolerate locked+missing worktrees in destroy #4693's tolerant substring matcher, leaving onlyexistsSyncas the success signal. Any worktree state where the directory still exists butgit worktree removeerrors (locked, or — common on macOS — git registered the worktree under a realpath-resolved path like/private/var/...while the stored path is/var/...) now fails the delete outright.Failed to delete branch '…' not foundtoasts on deletes that actually succeeded (cloud row already gone, worktree already removed). The branch ref was simply already absent — which is exactly whatdeleteBranchis trying to achieve.Both came from deciding cleanup success by inspecting git's error text, which is locale- and version-dependent and brittle by nature (the reason #4801 removed the matcher in the first place).
Fix
Decide on git's actual state, never on error strings:
worktree remove --force --forceagainst the canonical (realpath) path so a symlinked stored path still matches its registration, then consultgit worktree list --porcelain. Continue if the path is no longer registered; throw only if git still tracks a live worktree we genuinely couldn't remove (preserves [codex] Fix worktree delete cleanup ordering #4801's no-orphan-disk guarantee). No blanketworktree prune—--force --forcealready unregisters a missing-dir worktree, and a global prune would clobber other workspaces' stale metadata (guarded by an existing integration test).git show-ref --verify --quiet(exit-code based). An absent ref → success, no warning. A ref that exists but fails-D→ still warns.Reuses the canonical
listGitWorktrees/normalizeWorktreePathhelpers (the latter already handles the/var↔/private/varrealpath canonicalization).Tests
makeCtxunit mock to model the two new git reads (worktree list,show-ref) and reworked the worktree-failure test to express failure as a still-registered worktree rather than a thrown remove.bun testfor the 5 cleanup-touching files: 70/70 pass. Lint clean, typecheck exit 0.Verified against prod
Reproduced the reported case (Roshvan's
Roshvan/eat-147-journey-transparency): the v2 cloud row was already gone, confirming the delete had committed and only the cosmetic branch warning was left. After this change that path returns{ success: true, branchDeleted: true, warnings: [] }— the user just sees the workspace disappear, no warning toast.Summary by cubic
Fixes workspace delete failures by deciding cleanup based on git state instead of parsing error text. Also guards against git read failures masquerading as success.
git worktree remove --force --forceon the canonical path, then verify withgit worktree list --porcelainviagit.raw. If the list read fails, treat state as unknown and block; continue only if unregistered. Throw if still registered. No global prune.git branch --list. Missing ref counts as success; warn only on a real git failure or a-Dfailure when the ref exists.worktree listandbranch --list; update failure case to assert on a still-registered worktree.Written for commit f16641c. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests