Worktree removal fault tolerance: delete first, deregister second - #73
Merged
Conversation
Spec, design and tasks for making worktree deletion fault tolerant. git worktree remove deletes its bookkeeping even when the directory deletion fails, leaving an orphan folder the app cannot see (no .git remains, so scanRepos skips it) and cannot retry (git: "is not a working tree"). Probes also found a data-loss path on the success side: git for Windows recurses into junctions, so removing a hook-created worktree empties the shared skills target while reporting success. Design inverts the order - the app deletes the tree itself (junction-safe, bounded retry), then git drops the bookkeeping - so a blocked deletion leaves the worktree registered, visible and retryable. WRFT-07 (create-time leftover collision) is deferred to a follow-up PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two full runs of untouched main came back red - "2 failed | 531 passed", then "14 failed | 519 passed" across 5 files. Every failure was a duration overrun against the 5s default (11430-15557ms); tree.test.ts passes alone in 9.3s. The real-git and real-process suites are simply starved under parallel load, and which ones lose the race varies per run. Raise testTimeout/hookTimeout to 30000 globally, lifting the fix worktree-manager.test.ts:445 already applies locally to its base-refresh block. A timeout is a ceiling, not a delay: passing tests are unaffected. Also widen one racing fixture: hook-shell's kill test gave ping a 500ms window, and under load ping emitted nothing before the kill, so the output-tail assertion saw ''. 1500ms still kills `ping -n 5` (~4s) mid-flight - no assertion weakened, no production code touched. Suite now green: 533 passed / 39 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
removeDirTree deletes a tree with Node fs.rm (which unlinks junctions instead of recursing into them, unlike git for Windows) and retries only lock-type failures every 250 ms until a 3000 ms budget runs out. Each attempt passes maxRetries: 0 because Node retries at every level of the recursive walk: maxRetries 5 measured 21 599 ms against a cwd-locked directory versus 2 ms for a single attempt. Giving up returns the blocked path and how many entries are still on disk, so the caller can report a retryable failure instead of deregistering a worktree whose files remain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six real-filesystem tests for the hazards that decide whether delete-first is safe. The junction test asserts the junction target's files are still readable after the worktree is removed, having first read one through the junction so the assertion cannot pass vacuously: this is the case git for Windows gets wrong, emptying the shared folder while reporting success. The rest cover a dangling junction, read-only content, a nested repository's 0444 object store, and a genuine external holder (a child process whose cwd is inside the tree), which must fail with the blocked path named and return inside 5000 ms, then delete cleanly once the holder is killed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`PorcelainBlock` gains `locked?: string`, read from git's `locked [reason]` line. The three states stay distinguishable: a reason string, `''` for a bare `locked`, and `undefined` when the worktree is not locked — `''` must never read as "unlocked", since T4's guard refuses on presence, not truth. `parsePorcelainBlocks` is exported for the unit tests, matching the stance already taken for `parseChangedFiles`. Purely additive: `listWorktrees` and `worktreeHosting` read the same fields as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
git is no longer the deleter. `removeWorktree` now runs the guard table
primary -> registered -> locked -> dirty, deletes the directory itself with
the injected `removeDirTree`, and only then calls plain `git worktree remove`
to drop the bookkeeping.
Two defects close with the reorder. git deletes its admin dir even when its
own deletion failed ("no going back from here"), leaving a folder that no
longer belongs to any worktree and that scanRepos cannot see; and git for
Windows recurses into directory junctions, emptying the AD-013 skills
targets while reporting success. Under delete-first a blocked deletion
returns before git is invoked, so the worktree stays registered, visible and
retryable.
Two guards are new and both must precede the deletion. The registered check
is the anti-`rm -rf` guard, so a failing `git worktree list` refuses rather
than guesses. The locked check is the only thing left enforcing
`git worktree lock`, since git's own refusal would now arrive after the
files were gone -- it therefore refuses under `force: true` as well. `force`
keeps its FRWT meaning: skip the dirty check, nothing else.
The failure message names the blocked path, the remaining entry count and
the fact that a retry works. The structured `leftover` field on
RemoveWorktreeResult lands in the next commit together with its renderer
consumer (lesson L-001).
The primary and dirty refusal messages are byte-identical and all existing
removeWorktree tests pass unmodified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RemoveWorktreeResult gains `leftover`, set only when the *deletion* gave up -- never on a guard refusal or a bookkeeping failure. Its presence is the renderer's proof that nothing was deregistered, so the same Remove button is a working retry rather than a dead end. Producer, contract and consumer land together (lesson L-001): removeWorktree attaches the payload, `worktrees:remove` widens through the shared alias, and WorktreeDetail stores `removeLeftover`, clears it at the top of both remove paths, and renders the blocked path on its own monospace row so a long path wraps inside the Danger section instead of stretching it. The structured block replaces the flat error line instead of sitting below it as design.md sketched: the main-side `error` is already self-contained (WRFT-04 AC 3 requires it for non-interactive callers), so rendering both would print the same long path twice. Every other failure keeps the flat line unchanged, as does the `setRemoving(false)` re-enable on failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Killing a shell does not kill its children (lesson L-003). `stop` used to kill and finalize synchronously, so `sessions:stop` resolved immediately and the renderer began deleting a worktree while the agent's children still held handles inside it -- the removal then failed on a lock the app itself was holding. `#start` now keeps an `exited` promise resolved from the existing onExit callback, and `stop` captures it before `#finalize` drops the Map entry, then awaits it against a SESSION_EXIT_WAIT_MS = 3000 cap. A wedged child must not block a removal forever; the deleter's retry loop and leftover report cover the residue. `#finalize` still runs before the first await, so the status flip stays synchronous and all seven existing sync call sites keep passing unmodified. The cap timer is cleared in a `finally` and unref'd -- unlike L-003's grace timer, whose unref let real work be skipped, this one only races a promise a live caller is already awaiting, so it can skip nothing. `killAll` stays fire-and-forget on purpose: awaiting it would add up to 3 s per session to app quit, and the kill plus the persisted status are already done by the time stop suspends. No code change was needed in index.ts -- the handler already returns the promise and ipcMain.handle awaits it; a comment records that the return is load-bearing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extends the CDP smoke with WRFT-06: park an external process's cwd inside a seeded worktree, click Remove, and assert the Danger section names the blocked path, states what is left and that the worktree stays registered, and that the row is still there after a real tree refresh -- the defect this feature fixes is that the row used to vanish while the folder stayed on disk. Then kill the holder and click Remove again: row gone, "Removed lock/me" toast, folder gone. The seed grows a third worktree (`api-lock-me`, branch `lock/me`) because the earlier checks remove both existing ones. Its `sub/` is empty on purpose: git ignores empty directories, so the worktree still reads clean and the first click takes the direct remove path. The holder's cwd sits in `sub/`, which makes it the reported blockedPath with `remaining: 1`, mirroring dir-remover.test.ts:322. The whole section runs in try/finally so a failed check can never leave a node.exe parked in the worktree, blocking later runs and the seed's own rmSync. NOT RUN HERE: a CDP smoke needs a live desktop session and a seeded workspace (TESTING.md: hand-run, never CI), and launching the app from an agent would interfere with the owner's desktop. Verified by `node --check` on both scripts and by reading them against T5's renderer markup. The live run stays open as an owner task. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AD-014 makes delete-first a project-level invariant: the app deletes the worktree directory itself and `git worktree remove` is called purely to drop bookkeeping, so no surface may use `git worktree remove --force` as a deleter. Records both measured reasons -- git for Windows recurses into junctions and emptied AD-013's shared skills target while reporting success, and git deletes its bookkeeping even when the tree deletion fails, which is what produced the invisible orphan -- plus the fixed guard order (primary -> registered -> locked -> dirty, all refusing before any deletion) and why the lock check has to be ours: git's own refusal would arrive after we had already deleted the tree. It also records that WRFT-07 is deferred to a follow-up PR by owner decision. Spec traceability moves WRFT-01..06 to "Implemented -- pending Verifier", not Verified: the independent Verifier has not run. WRFT-07 is marked Deferred with a pointer to T9-T11. WRFT-07 AC 1 is corrected -- createWorktree already guards its target at worktree-manager.ts:87-89 with "Target path already exists", so git's `fatal: already exists` is never reached and the AC upgrades an existing flat app message rather than replacing a git error. STATE.md was written section-scoped: one appended Decisions row and a replaced Handoff body. The Handoff now carries the commit map, the green counts, and the three outstanding owner items -- the T7 live smoke run, the visual pass on the Danger section, and the missing GitHub issue that blocks writing `Closes #<n>`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three `leftover:` occurrences in the WRFT failure tests were fixture
inputs handed to spyDeleter, never expectations on what removeWorktree
returns. Dropping the field from the failure result (Verifier mutation M6)
left all 80 tests green, while WorktreeDetail branches on it — losing it
would silently degrade the UI to the flat error line.
Assert the returned payload by value in both failure tests, and pin the
documented other half: a guard refusal carries no leftover at all.
Verified by re-running M6: 2 failed | 18 passed | 60 skipped, both on
"expected undefined to deeply equal { blockedPath, remaining: 3 }".
Production file restored byte-identical before the gate.
Gate: 564 passed / 40 files, typecheck clean, lint 0 errors / 18 warnings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The exact-count assertions for `remaining` drive the injected readEntries fake, so they cannot see a change to the real-fs wiring, and the one real-filesystem check used toBeGreaterThanOrEqual(1). Making the real readdir non-recursive (Verifier mutation M13) therefore left all 15 tests green, so an understated count could reach the user's error message. Add a real-fs test whose fixture makes the two readings impossible to confuse: a directories-only tree with the external holder's cwd three levels down, so nothing is deletable, the residue is deterministic, and a recursive read reports 3 where a top-level read reports 1. The existing >= 1 assertion is kept untouched. Verified by re-running M13: 1 failed | 15 passed, on "remaining: 3" vs "remaining: 1". Production file restored byte-identical before the gate. Gate: 565 passed / 40 files, typecheck clean, lint 0 errors / 18 warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ract Closes the Verifier's spec-precision gap P2. WRFT-04 AC 3 said `remaining` was "the count of entries still present under the worktree root", which read either way - a non-recursive readdir satisfied the prose while reporting 1 where the recursive reading reports 3. F2 pinned the recursive reading by test; this makes the spec say it. Also states explicitly that `leftover` is part of the returned contract (the renderer branches on it) and that guard refusals carry none, which is what F1 now asserts. Commits the Verifier's round-1 report alongside, unmodified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tory residue WRFT-04 AC 3 says `remaining` is the recursive count of every entry still present under the worktree root. The existing real-fs test pins the recursion but not the "every entry" half: its fixture is directories only, so a count that skipped files read the same 3 and passed. In the spec's headline residue (sub/, sub/deep.txt, untracked.txt) that reading reports 1 instead of 3. Add a second real-fs test whose residue is a single locked chain, wt/keep/a/held.txt, held by an external pwsh process opening the file with FileShare.None -- a cwd holder does not protect the files beside it, and Node's own handles never block deletion. The residue is exactly keep, keep\a and keep\a\held.txt, so one number separates four readings: every-entry 3, directories-only 2, files-only 1, top-level 1. Verified by mutation: filtering readEntries to isDirectory() (which previously left all 16 tests green), to isFile(), adding one phantom entry, and dropping the recursive flag each now fail this test. The afterEach cleanup retries its rmSync, since Windows releases a killed holder's file handle asynchronously and would otherwise fail EPERM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WRFT-04 AC 3 says the primary, unregistered, locked and dirty guards all refuse without a leftover, since nothing was deleted. Only the locked guard asserted it, so attaching a leftover to the primary-checkout refusal passed all 80 tests. That is not cosmetic: WorktreeDetail branches on the field to render the "still registered, so you can retry" block, and a refusal by a guard can never succeed on retry. Add the same one-line assertion to the dirty, primary-checkout and unregistered-path tests, leaving the locked one in place. Verified by mutation: a stray leftover on all three guard returns now fails all three tests, so each assertion is load-bearing, not just the one probed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Independent verification finished after three rounds. WRFT-01..05 move to Verified (AC evidence plus discrimination-sensor kills); WRFT-06 stays Unverified because its CDP smoke has never been executed; WRFT-07 remains deferred. Rounds 1 and 2 both failed, and both failures were in tests rather than production code: `leftover` was only ever a spy input, the recursive count was unpinned, the round-1 fix's fixture was directories-only and so blind to what it counted, and guard-refusal absence was asserted on one guard of four. Every fix was test-only - no production line changed across any of them. Three survivors are recorded as non-blocking with reasoning rather than being papered over, including one the Verifier recommends never fixing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
L-005 recurred on a second feature, meeting promote_threshold=2, so it moves to confirmed: real-git and real-process suites sitting near the default per-test timeout turn a gate red under parallel load with no production change. It cost two red baseline runs before T0 fixed it. Two new candidates, both from mutants that survived a round of verification: - L-006: a payload field that appears in a test only as an injected fake's *input* reads like coverage but proves nothing - dropping it from the real return left 80 tests green. - L-007: a test written to kill one named mutant can encode that mutant's blind spot in its own fixture. A directories-only residue pinned the recursive count while a directories-only count still survived. Hand-maintained: scripts/lessons.py does not exist in this repo, so lessons.json was edited directly and LESSONS.md re-rendered in the script's exact format. Recorded in the STATE.md handoff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 tasks
obogoni
pushed a commit
that referenced
this pull request
Aug 5, 2026
…status The handoff said the removal feature was unpushed with no PR and no issue; origin/main (7cc8c76) is in fact the PR #73 merge, with issue #72 closed on 2026-07-31. Corrects that, and notes that removal-branch commit 5e22450 was made after #73 merged and so reaches main via #75 instead. Also records why #75 targets main rather than stacking on the removal branch, and the two hand-testing gotchas found while discharging the end-to-end criterion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #72
What this fixes
Removing a worktree whose files are held by another process left an invisible orphan: git deleted its bookkeeping even though the directory deletion failed, so the worktree vanished from
git worktree listwhile its files stayed on disk — with no.gitleft,scanReposskips it, andgit worktree removeanswersfatal: 'is not a working tree'on any retry. The user saw "removal failed" and then watched the row disappear anyway.Probing that fault turned up a second, worse bug on the success path: git for Windows treats a junction as a directory and recurses into it, so
git worktree remove --forcewas emptying the shared target of AD-013's skills junctions while reporting success. Since a junction makes the worktree read as dirty (?? .skills/), the UI routed exactly those worktrees down the force path.The change
Removal is now delete-first: the app deletes the worktree tree itself with a junction-safe, deadline-bounded deleter, then calls
git worktree removepurely to drop bookkeeping. A blocked deletion returns before git runs, so the worktree stays registered and its row is the retry handle. TheUnregistered + Presentstate is unreachable by construction rather than handled after the fact.dir-remover.ts(new) — junction-safe recursive delete,maxRetries: 0per attempt inside a 250 ms / 3000 ms loop, returning{ blockedPath, remaining }when it gives up.maxRetries: 0is load-bearing: Node retries at every level of the walk, somaxRetries: 5measured 21 599 ms against a locked directory versus 786 ms for a self-managed loop.removeWorktree— six-step guard table (primary → registered → locked → dirty → delete → bookkeeping). The registered check doubles as the anti-rm -rfguard, and the newgit worktree lockcheck is mandatory because git's own lock refusal would otherwise arrive after we deleted the files.SessionManager.stop— resolves on the PTY's real exit (capped at 3000 ms) instead of returning immediately, so removal no longer races the terminals it just killed.killAll()stays fire-and-forget; awaiting it would add up to 3 s per session to app quit.Verification
Independent Verifier (author ≠ verifier), three rounds: FAIL (14/16 mutants killed) → FAIL (8/10) → PASS (12/15, 3 non-blocking survivors recorded with reasoning).
Every gap was in the tests, never the production code —
git diff --name-only dcc50dc..HEADtouches no production file. Round 1 foundleftoverwas only ever a spy input, never asserted; round 2 found the fix for that was blind to what it counted, because the fixture chosen for determinism was directories-only. The final fixture yields four different numbers (3 / 2 / 1 / 1) so one test discriminates every wrong reading.The sensor confirms the invariant is real: making the deletion-failure path call git anyway is killed, and a junction-following deleter is killed by the shared target's files vanishing — the AD-013 loss reproduced and detected.
566 tests / 40 files green, typecheck clean, lint 0 errors / 18 pre-existing warnings.
Note on the first commit
34f8970is not part of the feature. Two full runs of untouchedmaincame back red —2 failed | 531 passed, then14 failed | 519 passed— all pure timeout starvation of the real-git and real-process suites under parallel load, varying run to run. The gate was unreliable before any change here, so it had to be fixed before per-task gates meant anything. GlobaltestTimeout/hookTimeoutof 30 000 (a ceiling, not a delay) plus one racing fixture window widened. This was confirmed lesson L-005 recurring.Outstanding
node scripts/seed-smoke-remove.mjs→npm run dev -- -- --remote-debugging-port=9222→node scripts/smoke-remove.mjs, plus a visual pass on the long-path wrapping.tasks.md.🤖 Generated with Claude Code