fix(host-service): tolerate locked+missing worktrees in destroy#4693
Conversation
…ktrees `git worktree remove --force` fails with "cannot remove a locked working tree" when a workspace's worktree is locked and its directory has been manually deleted — the existing substring matcher didn't catch this and the step pushed a confusing "Failed to remove worktree" warning even though the dir was already gone. Escalate to `--force --force` (we're past the commit point) and add an `existsSync` fallback: if the worktree dir is gone, the user's intent is satisfied regardless of what git printed (locked, locale-translated, future phrasing). The substring matcher stays as defense for the rare race where the dir disappears mid-remove. Closes SUPER-779.
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Ready to review this PR? Stage has broken it down into 2 individual chapters for you:
Chapters generated by Stage for commit 2043ca0 on May 18, 2026 10:16pm UTC. |
|
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)
📝 WalkthroughWalkthroughThe workspace cleanup flow now reliably handles locked worktrees by importing ChangesWorkspace Cleanup Resilience
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Greptile SummaryThis PR fixes a case where
Confidence Score: 4/5Safe to merge; the destroy path is already a best-effort, warnings-only operation past the cloud-delete commit point, so the more aggressive double-force removal is appropriate. The two-line production change is narrow and well-targeted. The existsSync fallback path — where git fails even with --force --force but the directory is already gone — is not exercised by the new test, meaning git internal worktree metadata could theoretically be left stale while worktreeRemoved reports true. That gap is very unlikely in practice. The test comment in workspace-cleanup.integration.test.ts mildly overstates the role of the existsSync fallback; the production file itself is clean.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/workspace-cleanup/workspace-cleanup.ts | Escalates worktree removal to --force --force and adds existsSync fallback in catch; change is targeted and well-commented, with one minor gap where the fallback path could silently leave stale git-internal metadata if git fails even with double-force. |
| packages/host-service/test/integration/workspace-cleanup.integration.test.ts | New test correctly sets up the locked+missing scenario and asserts clean deletion with no warnings; the test exercises the --force --force success path (not the existsSync fallback), which the in-code comment slightly misstates. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[destroy called] --> B[Open git repo]
B -->|error| C[push warning: Failed to open repo]
B -->|success| D["git worktree remove --force --force"]
D -->|success| E[worktreeRemoved = true]
D -->|throws| F{existsSync worktreePath?}
F -->|false: dir already gone| G[worktreeRemoved = true, no warning]
F -->|true: dir still present| H{substring match?}
H -->|matches known message| I[worktreeRemoved = true, no warning]
H -->|no match| J[push warning: Failed to remove worktree]
E --> K[optional branch delete]
G --> K
I --> K
J --> K
K --> L[delete SQLite row]
L --> M[return result]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/host-service/test/integration/workspace-cleanup.integration.test.ts:196-200
The test comment implies that the `existsSync` fallback is what resolves the locked+missing case, but with `--force --force` in place git actually succeeds and removes its internal metadata cleanly — so the test exercises the success branch, not the fallback. The fallback (and the scenario where git's `.git/worktrees/` metadata might be left stale) goes untested. Worth a small wording tweak so future readers don't misread which branch they're actually covering.
```suggestion
// A locked worktree whose dir was manually deleted is the scenario
// that breaks the substring-based error matcher: git says
// "fatal: cannot remove a locked working tree" and single `--force`
// is not enough. `--force --force` overrides the lock and removes
// git's internal metadata cleanly (the success path). The existsSync
// fallback in the catch block is an additional safety net for any
// future git failure where the dir is already gone but git still
// throws; that path is not exercised here.
```
Reviews (1): Last reviewed commit: "fix(host-service): make workspace destro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // A locked worktree whose dir was manually deleted is the scenario | ||
| // that breaks the substring-based error matcher: git says | ||
| // "fatal: cannot remove a locked working tree" and single `--force` | ||
| // is not enough. `--force --force` plus the existsSync fallback | ||
| // closes the loop so the user always gets a clean delete. |
There was a problem hiding this comment.
existsSync fallback is what resolves the locked+missing case, but with --force --force in place git actually succeeds and removes its internal metadata cleanly — so the test exercises the success branch, not the fallback. The fallback (and the scenario where git's .git/worktrees/ metadata might be left stale) goes untested. Worth a small wording tweak so future readers don't misread which branch they're actually covering.
| // A locked worktree whose dir was manually deleted is the scenario | |
| // that breaks the substring-based error matcher: git says | |
| // "fatal: cannot remove a locked working tree" and single `--force` | |
| // is not enough. `--force --force` plus the existsSync fallback | |
| // closes the loop so the user always gets a clean delete. | |
| // A locked worktree whose dir was manually deleted is the scenario | |
| // that breaks the substring-based error matcher: git says | |
| // "fatal: cannot remove a locked working tree" and single `--force` | |
| // is not enough. `--force --force` overrides the lock and removes | |
| // git's internal metadata cleanly (the success path). The existsSync | |
| // fallback in the catch block is an additional safety net for any | |
| // future git failure where the dir is already gone but git still | |
| // throws; that path is not exercised here. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/test/integration/workspace-cleanup.integration.test.ts
Line: 196-200
Comment:
The test comment implies that the `existsSync` fallback is what resolves the locked+missing case, but with `--force --force` in place git actually succeeds and removes its internal metadata cleanly — so the test exercises the success branch, not the fallback. The fallback (and the scenario where git's `.git/worktrees/` metadata might be left stale) goes untested. Worth a small wording tweak so future readers don't misread which branch they're actually covering.
```suggestion
// A locked worktree whose dir was manually deleted is the scenario
// that breaks the substring-based error matcher: git says
// "fatal: cannot remove a locked working tree" and single `--force`
// is not enough. `--force --force` overrides the lock and removes
// git's internal metadata cleanly (the success path). The existsSync
// fallback in the catch block is an additional safety net for any
// future git failure where the dir is already gone but git still
// throws; that path is not exercised here.
```
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}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…ktrees (#4693) `git worktree remove --force` fails with "cannot remove a locked working tree" when a workspace's worktree is locked and its directory has been manually deleted — the existing substring matcher didn't catch this and the step pushed a confusing "Failed to remove worktree" warning even though the dir was already gone. Escalate to `--force --force` (we're past the commit point) and add an `existsSync` fallback: if the worktree dir is gone, the user's intent is satisfied regardless of what git printed (locked, locale-translated, future phrasing). The substring matcher stays as defense for the rare race where the dir disappears mid-remove. Closes SUPER-779.
…ktrees (superset-sh#4693) `git worktree remove --force` fails with "cannot remove a locked working tree" when a workspace's worktree is locked and its directory has been manually deleted — the existing substring matcher didn't catch this and the step pushed a confusing "Failed to remove worktree" warning even though the dir was already gone. Escalate to `--force --force` (we're past the commit point) and add an `existsSync` fallback: if the worktree dir is gone, the user's intent is satisfied regardless of what git printed (locked, locale-translated, future phrasing). The substring matcher stays as defense for the rare race where the dir disappears mid-remove. Closes SUPER-779.
1 participant
Summary
git worktree remove --forcefails withfatal: cannot remove a locked working tree; use 'remove -f -f' to override or unlock first. The existing substring matcher inworkspace-cleanup.ts:294-300doesn't catch this phrasing, so the step pushes a confusing "Failed to remove worktree" warning even though the directory the user wanted gone is already gone.--force --force(we're past the cloud-delete commit point — the workspace is already gone from the user's perspective, so the conservative single--forceno longer earns its keep) and add anexistsSyncfallback: any git failure whose post-condition is "the worktree dir is gone" is success, regardless of what git printed. The substring matcher stays as belt-and-braces for the rare race where the dir disappears mid-remove.worktreeRemoved: false, warnings non-empty) and passes after the fix.Closes SUPER-779.
Test plan
bun test packages/host-service/test/integration/workspace-cleanup.integration.test.ts— 10 pass (9 existing + 1 new).bun run lintclean.bunx tsc --noEmitclean.rm -rfits worktree dir, click delete from the sidebar — workspace disappears with no warning toasts.git worktree lock <path>first.Out of scope
The renderer-side paths (
useDestroyWorkspace.normalizeError, dialog inspect-failure handling) were verified to already swallow git failures correctly — no changes needed there.Summary by cubic
Makes
host-serviceworkspace destroy tolerate locked + missing git worktrees so deletes complete without false warnings. Addresses SUPER-779 by escalating remove and treating a missing worktree dir as success.git worktree remove --force --forceto handle locked worktrees after manualrm -rf.existsSync(worktreePath)is false after a git error, mark the worktree as removed; keep the substring matcher as a fallback.Written for commit 2043ca0. Summary will update on new commits. Review in cubic
Summary by CodeRabbit