Repository navigation
fix(worktrees): keep a workspace Windows refused to delete, and say why - #17173
brennanb2025 wants to merge 9 commits into
Conversation
Greptile SummaryThe PR keeps workspace metadata intact when orphan-directory deletion fails or cannot be proven safe, and provides actionable, localized Windows held-directory errors.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/shared/worktree/removal.ts | Adds Windows root-directory removal classification, held-directory guidance, and an unproven-orphan error anchor. |
| src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts | Preserves metadata and reports failure when orphan-directory cleanup fails or is unsafe. |
| src/main/runtime/orca-runtime.ts | Mirrors fail-closed orphan-removal behavior in the runtime service. |
| src/main/ipc/worktree-logic.ts | Enriches removal errors with actionable held-directory guidance while retaining raw details. |
| src/renderer/src/components/sidebar/delete-worktree-toast.ts | Maps held-directory and unproven-orphan failures to localized user-facing copy. |
| src/renderer/src/components/sidebar/force-delete-retry-toast.ts | Centralizes force-delete retry settlement and error-toast routing. |
| src/renderer/src/components/status-bar/workspace-space-force-delete.ts | Routes Space Manager force-delete results through the shared failure-copy funnel. |
Reviews (10): Last reviewed commit: "fix(worktrees): route every explicit For..." | Re-trigger Greptile
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorktree cleanup now records local directory removal failures instead of ignoring them. Cleanup paths throw formatted errors and preserve worktree metadata when deletion fails. Shared helpers classify Windows workspace-directory failures, including extended-length paths and prose-only errors, and append a directory-held hint. The delete-worktree toast displays localized guidance for held or inaccessible directories. Tests cover formatting, orphan cleanup, metadata preservation, and notification behavior. Merge Risk: 🔵 Low · up to The PR improves Windows workspace deletion recovery, but the current version can show the wrong message after a force-delete failure, includes a retry test that may inspect the wrong toast, and still leaves the new diagnostic outside the repository localization path. It is mergeable with explicit owner follow-up on these bounded issues. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a detailed explanation, rationale, linked issue, behavior changes, scope, and testing results. However, it does not include the required visual proof for a UI behavior change and omits several template sections and checklist items. Full details: Docstring CoverageExplanation Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 11 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f22f0137-c227-4b85-b8e3-c1ecd2fd1923
📒 Files selected for processing (7)
src/main/ipc/worktree-logic.test.tssrc/main/ipc/worktree-logic.tssrc/main/ipc/worktrees-removal-recovery.test.tssrc/main/ipc/worktrees/removal/remove-registered-local-worktree.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/shared/worktree/removal.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/worktree-removal-safety.ts (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a localization ID for the new diagnostic.
UNPROVEN_ORPHANED_WORKTREE_DIRECTORY_MESSAGEadds user-facing English text directly in the main-process module. Add an intent-named localization ID inconfig/i18n-translation-source.mdand resolve it through the existing localization path. This keeps the orphan-cleanup error localized.Based on learnings: newly added user-facing TypeScript strings must use intent-named localization IDs with named interpolation placeholders when interpolation is needed.
Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 089033dc-972d-49b3-b962-deef84557de6
📒 Files selected for processing (8)
src/main/ipc/worktree-logic.test.tssrc/main/ipc/worktree-logic.tssrc/main/ipc/worktrees-removal-recovery.test.tssrc/main/ipc/worktrees/removal/remove-registered-local-worktree.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/worktree-removal-safety.tssrc/shared/worktree/removal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/worktree/removal.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6852565f-3acd-4171-beca-e11c79c7b064
📒 Files selected for processing (2)
src/renderer/src/components/sidebar/run-worktree-delete-with-toast.test.tssrc/renderer/src/components/sidebar/run-worktree-delete-with-toast.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a4988cc-c54f-4e3b-9519-f062bca08618
📒 Files selected for processing (8)
src/main/ipc/worktree-logic.test.tssrc/renderer/src/components/sidebar/delete-worktree-toast.test.tssrc/renderer/src/components/sidebar/delete-worktree-toast.tssrc/renderer/src/components/sidebar/run-worktree-delete-with-toast.test.tssrc/renderer/src/components/sidebar/run-worktree-delete-with-toast.tssrc/renderer/src/components/sidebar/show-delete-worktree-error-toast.tssrc/renderer/src/i18n/locales/en.jsonsrc/shared/worktree/removal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/i18n/locales/en.json
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
Deleting a workspace on Windows can fail with `EBUSY: resource busy or locked, rmdir '<workspace>'` (STA-4895). Two things were wrong with what happened next. The message named the folder but not the cause, and there is nothing to retry: Windows refuses `rmdir` on a directory any process still has open, and a process's own current directory counts, so a shell or dev server left sitting in the workspace makes it permanently undeletable. The failure is now classified when it lands on the workspace directory itself -- not on a file inside it -- and the toast says what to close. Worse, the orphaned-worktree cleanup swallowed that failure with an empty `.catch`, then purged Orca's metadata and reported success. Every file was still on disk while the workspace vanished from the sidebar, leaving no UI to retry from. Both copies of that path (the IPC handler and the runtime service) now surface the failure and keep the row.
The refusal guard now throws when the directory survives, but nothing pinned the other half: a guard that refused unconditionally read as correct. Add the mirror control at both call sites -- directory already gone, so forgetting the row is the honest outcome -- plus the negative half of the raw-string arm (a child path is reported without claiming the folder itself is held). Also drop an exactly-duplicate disjunct: isHeldWorkspaceDirectoryRemovalError is message.includes(WORKSPACE_DIRECTORY_HELD_HINT).
The held-directory failure matches none of the force-delete reasons, so the toast fell through to the branch that renders the raw main-process error. The English hint appended in worktree-logic was therefore displayed to the user verbatim, untranslated, alongside the EBUSY prose and the native path. The hint stays where it is as the anchor its matcher keys on -- it crosses a process boundary and must not move -- and the toast now selects intent-named catalog copy for the case instead of echoing the wire text.
…py funnel The retry renders its own toast, so the main process's held-directory hint -- an English wire anchor, not display copy -- reached the user verbatim there even after the first delete toast was fixed.
The held-directory hint was routed to catalog copy, but the sibling string this PR added on the same branch was not: a refused orphan cleanup throws UNPROVEN_ORPHANED_WORKTREE_DIRECTORY_MESSAGE, which matches no force-delete reason, so getDeleteWorktreeToastCopy fell to `description: error` and rendered the main process's English sentence verbatim in the toast body. Give it the same treatment -- wire anchor stays put, copy comes from the catalog -- and pin the two literals together so rewording either side cannot silently un-localize the toast again. Also repairs the two typecheck breaks the retry-toast test shipped with (WorktreeRemovalTarget requires executionHostId).
…copy funnel The delete-failure toast has six routes, not four. The dialog's Force Delete button and the Space Manager's row recovery each ran the destructive retry themselves and rendered `result.error` straight into the toast body, so on Windows a user reading "EBUSY: resource busy or locked, rmdir 'C:\...'" plus the main process's English held-directory hint got exactly the leak this branch has patched four times already. All three surfaces that spend the PTY-stop waiver now settle through one helper, so the funnel is a route rather than a habit. A sweep test enumerates those surfaces and fails when a new one appears without it — the earlier leaks were all new routes. Also fixes the rejected force-delete retry titling itself "Failed to delete workspace" instead of "Force delete failed" (reported on the PR).
7c4be4d to
501178b
Compare
nwparker
left a comment
nwparker
left a comment
There was a problem hiding this comment.
Holding this. The diagnosis-and-say-why half is good, but the fail-closed half rests on a mechanism claim that in-repo evidence contradicts, and it creates a workspace row the user can no longer remove from anywhere in the app.
Blocking
1. The headline mechanism claim is disproven by code already in this repo.
The PR body argues EBUSY ... rmdir <workspace root> is the first syscall of the delete and therefore proves the tree is not left half-removed. src/main/host-tree-removal.ts — the function that performs this exact delete — opens with the opposite observation: "AV/indexers/late handle releases surface transient EBUSY/ENOTEMPTY/EPERM on a tree Node just emptied." Node v24 confirms it: internal/fs/rimraf has notEmptyErrorCodes = new SafeSet([ENOTEMPTY,EEXIST,EPERM]) and _rmdir routes any of those into _rmchildren, which deletes every child and then re-rmdirs the root. So an EPERM (or a first-pass ENOTEMPTY then a held root) surfaces as exactly EPERM|EBUSY ... rmdir <root> with the tree already emptied.
Consequence: the new hint fires on that shape, the fail-closed arm keeps the sidebar row, and Orca presents an intact-looking workspace whose contents were destroyed. On main the row was purged, which at least matched disk. That is a new user-facing failure mode built on a premise the PR states was never verified on Windows.
2. The refusal arm creates a permanently unremovable row. After the new throw, git worktree prune has already deregistered the row, so attempt 2 enters removeUnregisteredWorktree → canSafelyRemoveOrphanedWorktreeDirectory fails for the same reason → canCleanupUnregisteredOrcaLeftoverDirectory requires .git to be MISSING (worktree-removal-safety.ts:226-233) so it is false → isAlreadyRemovedWorktreePath false → dead-ends at Refusing to delete unregistered worktree path. The Workspace Cleanup "Remove from Orca" button is not an escape hatch: openWorkspaceCleanupForgetLocally routes to runWorktreeDelete, not mode: forget-local, so it hits the same dead end.
3. The retained workspace is already partially destroyed at the throw point. stopPtysForDestructiveWorktreeRemoval (remove-registered-local-worktree.ts:118-122) has killed its terminals and removeWorktreeLinkedPaths (:125-127) has unlinked its orca.yaml sharedDirectories and per-user shared paths (node_modules, .venv, …) before the git-remove attempt. "Keep the workspace so you can retry" hands back a workspace with its shared links gone and its git registration pruned, rendered as a normal row.
4. Remote/relay mixed versions. OrcaRuntimeService.removeManagedWorktree is an RPC a paired client calls. New host + old client turns a delete that previously succeeded into a hard failure, and the old client cannot localize the new anchors — it renders raw English and keeps a row it can never delete.
Should fix
- Diagnosability regression. Both new arms replace the raw error, and the tests pin that the description must not contain the errno or the path. The unproven-orphan copy says "Check the folder yourself and remove it once you are sure it is no longer needed" without ever naming the folder; only the display name survives, in the title.
formatWorktreeRemovalErrorcontract change is unscoped. Widening to!(error instanceof Error) && typeof error !== stringmeans every removal throw site on every platform now appends arbitrary string payloads into user-facing text and into the string the renderer classifiers regex over. Two pre-existing tests had to be rewritten to accept the new output — that is a behavior change, not an additive hint.- The throw also skips
cleanupUnusedWorktreePushTargetRemoteand thepreservedBranchCleanupByScope.delete. Every failed or refused orphan cleanup now leaks the per-worktree push-target remote in git config plus a stale preserved-branch scope entry, on top of the prune that already ran. - The new tests lock in the dead end rather than probing it.
keeps an existing orphan directory when its ownership cannot be provenasserts only that attempt 1 throws; nothing asserts any subsequent in-app route can still remove the row — which is the actual regression.
What I would land
The diagnosis + localized toast funnel is worth having. To get there:
- Verify the syscall ordering on real Windows for both
EBUSYandEPERM, empty and non-empty trees, before any fail-closed arm ships. - Keep the errno and the path in the toast.
- Add the "forget without deleting" escape hatch before the refusal arm can keep a row.
- Scope
formatWorktreeRemovalErrorback to its previous contract.
Happy to re-review once the Windows verification is attached — the base is otherwise clean against current main.
Fixes STA-4895 / #15541.
ELI5
Windows will not delete a folder while any program still has it open — and a program's own current folder counts. If a terminal, editor, or dev server is sitting inside a workspace, Windows refuses to delete it, forever, no matter how many times you retry.
Orca handled that badly in two ways. It showed you the raw operating-system complaint (
EBUSY: resource busy or locked, rmdir '...'), which names the folder but never says what to do about it. And on one code path it hid the failure: it threw the error away, forgot the workspace, and told you the delete worked — while every file was still on disk.This PR makes Orca say what is wrong, and stop claiming a delete happened when it did not.
User-facing before / after
Before
Failed to delete worktree at C:/…/my-workspace. EBUSY: resource busy or locked, rmdir 'C:\…\my-workspace'and a View button. Nothing in that told you a program was holding the folder, and retrying could not help.After
What is actually going on
fs.rm(dir, { recursive: true })triesrmdir(dir)first, before touching anything inside. SoEBUSY … rmdir '<workspace root>'is the first syscall of the delete, and it means a handle is open on the directory itself — on Windows, classically a process whose current directory it is. (Verified locally on Node 26: the equivalent first-syscall failure leaves every child file untouched.) That is why this specific shape is safe to diagnose, and why the tree is not left half-removed by it.The
EPERM/EACCEScodes ride along because libuv maps Windows sharing and access violations onto all three.Changes
src/shared/worktree/removal.ts—WORKSPACE_DIRECTORY_HELD_HINTplusisHeldWorkspaceDirectoryRemovalFailure, which fires only when the failing path is the workspace root. Matches both the structuredErrnoExceptionand the same failure arriving as prose, and undoespath.win32.toNamespacedPathso the\\?\-prefixed path Orca actually passes to the delete still compares equal. Gated on Windows path syntax, notprocess.platform, matching how the rest of this file decides.src/main/ipc/worktree-logic.ts—formatWorktreeRemovalErrorappends the hint. That function is the single funnel every removal throw site already uses, so the diagnosis reaches the toast regardless of which one raised the failure.src/main/ipc/worktrees/removal/remove-registered-local-worktree.tsandsrc/main/runtime/orca-runtime.ts— the orphan-cleanupremoveLocalWorktreePath(...).catch(() => {})becomes a captured failure that is surfaced after thegit worktree prune, instead of falling through to the metadata purge and a success return.Tests
Failing-first, 1-to-1 against
main(51ed7d4f678d):formatWorktreeRemovalErrorcaseskeeps the workspace when orphan cleanup cannot delete the directory(IPC)promise resolved "{}" instead of rejectingkeeps the runtime workspace when orphan cleanup cannot delete the directorypromise resolved "{}" instead of rejectingThe two negative cases (a busy file inside the workspace; a POSIX root reporting the same code) cannot be falsified by removing the rule, so they are falsified by ablation instead. Each gate ablated alone reddens exactly one test, and each mutation was confirmed applied before its result was trusted:
Full
src/main/runtime/orca-runtime.test.ts(1215) and the worktree removal/preflight/orphan/windows/cascade suites (163) pass.Not proven here
No Windows machine was available to this run. Everything above is proven from the code and from tests that run on macOS. A Windows box would be needed to confirm:
EBUSYin practice (asserted from documented Windows/libuv behaviour, not observed).EPERM/EACCESgenuinely appear for the same held-directory case, rather than onlyEBUSY.formatWorktreeRemovalErrorfrom a Node-side delete. One detail does not fit: every Node-side workspace delete in Orca goes throughtoHostRemovalPath, so its error would print\\?\C:\…— and the reported screenshot shows a bareC:\…. The prose matcher covers that shape either way, but the exact producer of that specific screenshot is not pinned. Sibling screenshots in the same issue (Timed out waiting for physical PTY teardown,connect ENOENT \\?\pipe\orca-terminal-host-v24-…) are different failures with their own existing handling.Scope
Deliberately not changed: the sibling branch that logs "Refusing recursive cleanup for unproven worktree directory" and then still purges metadata. That one never attempts a delete — it is a deliberate policy refusal for a worktree Git has already lost — so unwinding it is a different decision from "we tried, it failed, we said success". Flagging it rather than folding it in.