fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

GitWorktrees.add() leaks the worktree and branch when a step after git worktree add throws #274

New Issue
Closed
opened 2026-09-04 04:56:02 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 04:56:02 +02:00
Owner
Copy Link
Copy Source

Found by a bug-hunt fan-out; I verified this one in the code myself.

What is wrong

GitWorktrees.add() creates the worktree and branch, then runs five more steps that can throw:

exec("git", "-C", repoRoot, "worktree", "add", wt, "-b", branch, base);  // line 169 — created here
afterWorktreeAdded.accept(wt);
requireCredentialFreeHttpsOrigin(wt);          // an INTENDED security refusal
configureEnvironmentCredentialHelper(repoRoot, wt);
configureHttpsUrlRewriteForSshOrigin(repoRoot, wt);
isolateToolSurface(wt);
return wt;                                     // only now does the caller learn the path

If any of them throws, add() never returns, so in SessionManager.acquireWithWorktree the local path (line 493) is still null and the catch block's cleanup cannot run:

} catch (RuntimeException e) {
    log.warn("spawn failed for ... path={}: {}", ..., path, e.getMessage());
    if (path != null) {          // never true for a failure inside add()
        worktrees.remove(repoRoot, path);
    }
    throw e;
}

The worktree directory and its new branch stay on disk permanently. They are never registered as a session, so no reaper or sweep will ever collect them. The warning even prints path=null, so the operator is not told where the leak is.

Why this is reachable, not theoretical

requireCredentialFreeHttpsOrigin is not an IO accident — it is a deliberate security refusal that fires when a credential is present in the origin URL. The designed-to-fire path is the one that leaks.

Every other exit from acquireWithWorktree (overlayParity, shareWithGroup, spawn) is cleaned up correctly. Only the failures internal to add() lack teardown — the exits after the point of no return were not counted.

Fix

Make add() clean up what it created: wrap everything after the git worktree add so that any exception removes the worktree (git worktree remove --force, and the branch) before rethrowing. Cleanup belongs where the existing cleanup is — the caller's contract stays "if add throws, nothing was created".

Prove it by removing the fix and watching the test go red: the test must drive add() with a step that throws (not a hand-built failure downstream of it) and then assert the directory and branch are both gone.

Found by a bug-hunt fan-out; **I verified this one in the code myself.** ## What is wrong `GitWorktrees.add()` creates the worktree and branch, then runs five more steps that can throw: ```java exec("git", "-C", repoRoot, "worktree", "add", wt, "-b", branch, base); // line 169 — created here afterWorktreeAdded.accept(wt); requireCredentialFreeHttpsOrigin(wt); // an INTENDED security refusal configureEnvironmentCredentialHelper(repoRoot, wt); configureHttpsUrlRewriteForSshOrigin(repoRoot, wt); isolateToolSurface(wt); return wt; // only now does the caller learn the path ``` If any of them throws, `add()` never returns, so in `SessionManager.acquireWithWorktree` the local `path` (line 493) is still `null` and the catch block's cleanup cannot run: ```java } catch (RuntimeException e) { log.warn("spawn failed for ... path={}: {}", ..., path, e.getMessage()); if (path != null) { // never true for a failure inside add() worktrees.remove(repoRoot, path); } throw e; } ``` The worktree directory and its new branch stay on disk permanently. They are never registered as a session, so no reaper or sweep will ever collect them. The warning even prints `path=null`, so the operator is not told where the leak is. ## Why this is reachable, not theoretical `requireCredentialFreeHttpsOrigin` is not an IO accident — it is a deliberate security refusal that fires when a credential is present in the origin URL. The designed-to-fire path is the one that leaks. Every *other* exit from `acquireWithWorktree` (overlayParity, shareWithGroup, spawn) is cleaned up correctly. Only the failures internal to `add()` lack teardown — the exits after the point of no return were not counted. ## Fix Make `add()` clean up what it created: wrap everything after the `git worktree add` so that any exception removes the worktree (`git worktree remove --force`, and the branch) before rethrowing. Cleanup belongs where the existing cleanup is — the caller's contract stays "if `add` throws, nothing was created". Prove it by removing the fix and watching the test go red: the test must drive `add()` with a step that throws (not a hand-built failure downstream of it) and then assert the directory and branch are both gone.
ltms commented 2026-09-04 05:14:52 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main in 1e41bd6 (PR #277).

Verified by me, not on the worker's word:

  • the actual merge into current main builds green — 1281 tests, BUILD SUCCESS.
  • I reran the mutation independently, and deliberately not with git stash (see below): dropping the cleanupAfterAddFailure(...) call fails the new test with the worktree directory leaked after a post-creation step threw ==> expected: <false> but was: <true>.
  • the diff is exactly the two intended files. I checked specifically that no FleetConfig.java change rode along, given the incident below.

The fix reuses remove() rather than a bespoke removal, deletes the branch as well, orders the two correctly (a checked-out branch cannot be deleted until its worktree is gone), and logs a cleanup failure without ever masking the original exception. The asymmetry with a released session — which deliberately keeps its branch — is argued in the javadoc, and I agree with it: a provision that never completed has no session and no PR behind it.

The test drives add() itself through the existing afterWorktreeAdded seam, so the failure happens after the worktree exists rather than downstream in some other caller.

The real story of this ticket: git stash is shared across worktrees

Both this worker and the #273 worker ran git stash for their mutation checks at the same time, in sibling worktrees. refs/stash is not per-worktree — it is one stack in the repo's common git dir, shared by the primary's checkout and every worker worktree. So each worker's stash pop took the other's entry.

I measured it afterwards: git stash list from a worker worktree and from the primary's checkout return byte-identical output, and refs/stash is a single common ref.

Both workers noticed, and both recovered carefully. This one found its own content among git fsck --no-reflog dangling commits, verified the diff matched what it had written, restored it, reverted the stray FleetConfig.java back to HEAD, and pushed the other worker's candidate WIP commits back onto the shared stack with git update-ref so nothing was lost. Neither PR ended up with a wrong file in it — I verified both diffs.

That is a good outcome from a bad hazard, and the hazard was mine to prevent, not theirs: I ran them in parallel without knowing this. The implementer skill now forbids git stash outright and points at a wip: commit or a patch file instead (7d54344), and 11-Features.md records it under Isolated worktree per worker.

Also reported, not filed

ClaudeCodeLauncher.java:788-818 (writeCharterFile) has the same shape — Files.createTempDirectory then shareWithGroup, which can throw before the handle is returned. The worker correctly rated it milder: deleteOnExit() is registered, so the JVM cleans it up eventually. Recording it here rather than filing a ticket for a leak that self-heals on exit.

Merged to `main` in `1e41bd6` (PR #277). Verified by me, not on the worker's word: * the **actual merge** into current `main` builds green — 1281 tests, `BUILD SUCCESS`. * I reran the mutation independently, and deliberately **not** with `git stash` (see below): dropping the `cleanupAfterAddFailure(...)` call fails the new test with `the worktree directory leaked after a post-creation step threw ==> expected: <false> but was: <true>`. * the diff is exactly the two intended files. I checked specifically that no `FleetConfig.java` change rode along, given the incident below. The fix reuses `remove()` rather than a bespoke removal, deletes the branch as well, orders the two correctly (a checked-out branch cannot be deleted until its worktree is gone), and logs a cleanup failure without ever masking the original exception. The asymmetry with a *released* session — which deliberately keeps its branch — is argued in the javadoc, and I agree with it: a provision that never completed has no session and no PR behind it. The test drives `add()` itself through the existing `afterWorktreeAdded` seam, so the failure happens after the worktree exists rather than downstream in some other caller. ## The real story of this ticket: `git stash` is shared across worktrees Both this worker and the #273 worker ran `git stash` for their mutation checks at the same time, in sibling worktrees. `refs/stash` is **not** per-worktree — it is one stack in the repo's common git dir, shared by the primary's checkout and every worker worktree. So each worker's `stash pop` took the *other's* entry. I measured it afterwards: `git stash list` from a worker worktree and from the primary's checkout return byte-identical output, and `refs/stash` is a single common ref. Both workers noticed, and both recovered carefully. This one found its own content among `git fsck --no-reflog` dangling commits, verified the diff matched what it had written, restored it, reverted the stray `FleetConfig.java` back to `HEAD`, and pushed the other worker's candidate WIP commits back onto the shared stack with `git update-ref` so nothing was lost. Neither PR ended up with a wrong file in it — I verified both diffs. That is a good outcome from a bad hazard, and the hazard was mine to prevent, not theirs: I ran them in parallel without knowing this. The `implementer` skill now forbids `git stash` outright and points at a `wip:` commit or a patch file instead (`7d54344`), and `11-Features.md` records it under *Isolated worktree per worker*. ## Also reported, not filed `ClaudeCodeLauncher.java:788-818` (`writeCharterFile`) has the same shape — `Files.createTempDirectory` then `shareWithGroup`, which can throw before the handle is returned. The worker correctly rated it milder: `deleteOnExit()` is registered, so the JVM cleans it up eventually. Recording it here rather than filing a ticket for a leak that self-heals on exit.
ltms closed this issue 2026-09-04 05:14:58 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#274
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?