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

A timed-out git worktree add leaks the worktree it half-created: #274's cleanup does not cover the add itself #309

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

Found by a delegated hunter; I read the cited lines and confirmed them.

The gap

In GitWorktrees.add, the add command runs outside the try that guards everything after it:

exec("git", "-C", repoRoot, "worktree", "add", wt, "-b", branch, base);
try {
    afterWorktreeAdded.accept(wt);
    requireCredentialFreeHttpsOrigin(wt);
    configureEnvironmentCredentialHelper(repoRoot, wt);
    configureHttpsUrlRewriteForSshOrigin(repoRoot, wt);
    isolateToolSurface(wt);
} catch (RuntimeException e) {
    cleanupAfterAddFailure(repoRoot, wt, branch, e);
    throw e;
}

That is deliberate, and the javadoc on cleanupAfterAddFailure says why:

add() has already created the worktree and its branch by the time any step from afterWorktreeAdded through isolateToolSurface can throw

The assumption behind that is that git worktree add is atomic on failure. It is, for an error git reports itself: git cleans up after its own failure and exits non-zero, having created nothing. It is not atomic when we kill it:

if (!p.waitFor(30, TimeUnit.SECONDS)) {
    p.destroyForcibly();
    throw new WorktreeException("command timed out: " + ...);
}

A fixed, non-configurable 30 seconds, then destroyForcibly mid-checkout. By then git has registered .git/worktrees/<nonce>/ and started writing files. The WorktreeException propagates from outside the try, so cleanupAfterAddFailure never runs.

The same applies to the InterruptedException branch a few lines below, which also calls destroyForcibly and throws.

Nothing downstream cleans it up either

The hunter checked the caller and I confirmed it: in SessionManager.acquireWithWorktree, path stays null when worktrees.add() throws, so the if (path != null) cleanup — the guard #274's fix was built around — never fires. No layer reclaims this.

Direction of harm

Resource leak, not data loss. Say that plainly: no worker has been started, so the half-created worktree holds nobody's work, and losing it costs nothing. What is left behind is a registered-but-broken worktree entry, a directory, and possibly the new branch, with nothing tracking any of them. A later git worktree add on the same path fails, and git worktree list shows an entry an operator has to prune by hand.

So this is the #274 shape reappearing through a different trigger, at lower severity.

Reachability — honest version

Neither the hunter nor I reproduced a 30-second git worktree add. This repo is small and the operation takes well under a second. It needs a genuinely slow checkout: a large repo, a cold filesystem cache, a network-mounted worktree root, or an LFS fetch. The code path itself is certain; the trigger is plausible and unmeasured. Do not write the ticket up, or the commit message, as though a timeout has been observed.

What I want

Goal: every exit from add() that can leave a partially-created worktree behind must clean it up, and the javadoc must stop claiming an assumption that is not true.

Invariants:

  1. The original exception must reach the caller unchanged. cleanupAfterAddFailure exists to tidy up, not to convert or swallow — a cleanup failure must not replace the real cause.
  2. Cleanup must stay best-effort. A failure inside it gets logged, never thrown.
  3. A failure where git genuinely created nothing must not become noisy. Cleaning up something that does not exist is fine; logging a scary warning every time an ordinary "branch already exists" error occurs is not.

Candidate mechanism, offered as a candidate only: bring the exec inside the same try, so every failure after the point where git may have created state goes through one cleanup path. Check invariant 3 before you commit to it — that widens cleanup to cover ordinary git-reported failures too, where nothing was created, so make sure cleanupAfterAddFailure is quiet in that case. If it is not, say so and either make it quiet or narrow the fix to the timeout and interrupt paths, and justify which you chose.

Also fix the javadoc. It currently states the false assumption as fact, and that sentence is what made this gap invisible.

Rules

  • Prove it with a test that fails without the fix. You will need to make exec time out or be interrupted rather than waiting 30 real seconds — find the seam that lets you do that, and if there is no clean one, say so plainly and explain what you tested instead rather than inventing a test that reaches around the class.
  • Never run git worktree remove, git worktree prune, or any destructive git command against this repo or its worktrees. Other workers are live in them right now. Use a throwaway repo for experiments — and note in your report that a throwaway repo lacks this one's submodule, its --skip-worktree file and its per-worktree config, so say plainly which parts your test does and does not cover.
  • Do not run git stash — the stash is shared across every worktree here.
  • Mutation proof required: revert the fix, quote the real failure output, restore it.
  • Run cd fleetd && mvn clean install unpiped, and quote the real Tests run: and BUILD lines. Never pipe maven through tail/head, and never read $? after a pipe — after a pipe it is the last command's status, not Maven's.
Found by a delegated hunter; I read the cited lines and confirmed them. ## The gap In `GitWorktrees.add`, the `add` command runs **outside** the try that guards everything after it: ```java exec("git", "-C", repoRoot, "worktree", "add", wt, "-b", branch, base); try { afterWorktreeAdded.accept(wt); requireCredentialFreeHttpsOrigin(wt); configureEnvironmentCredentialHelper(repoRoot, wt); configureHttpsUrlRewriteForSshOrigin(repoRoot, wt); isolateToolSurface(wt); } catch (RuntimeException e) { cleanupAfterAddFailure(repoRoot, wt, branch, e); throw e; } ``` That is deliberate, and the javadoc on `cleanupAfterAddFailure` says why: > `add()` has already created the worktree and its branch by the time any step from `afterWorktreeAdded` through `isolateToolSurface` can throw **The assumption behind that is that `git worktree add` is atomic on failure.** It is, for an error git reports itself: git cleans up after its own failure and exits non-zero, having created nothing. It is not atomic when *we* kill it: ```java if (!p.waitFor(30, TimeUnit.SECONDS)) { p.destroyForcibly(); throw new WorktreeException("command timed out: " + ...); } ``` A fixed, non-configurable 30 seconds, then `destroyForcibly` mid-checkout. By then git has registered `.git/worktrees/<nonce>/` and started writing files. The `WorktreeException` propagates from *outside* the try, so `cleanupAfterAddFailure` never runs. The same applies to the `InterruptedException` branch a few lines below, which also calls `destroyForcibly` and throws. ## Nothing downstream cleans it up either The hunter checked the caller and I confirmed it: in `SessionManager.acquireWithWorktree`, `path` stays `null` when `worktrees.add()` throws, so the `if (path != null)` cleanup — the guard #274's fix was built around — never fires. No layer reclaims this. ## Direction of harm Resource leak, **not** data loss. Say that plainly: no worker has been started, so the half-created worktree holds nobody's work, and losing it costs nothing. What is left behind is a registered-but-broken worktree entry, a directory, and possibly the new branch, with nothing tracking any of them. A later `git worktree add` on the same path fails, and `git worktree list` shows an entry an operator has to prune by hand. So this is the #274 shape reappearing through a different trigger, at lower severity. ## Reachability — honest version Neither the hunter nor I reproduced a 30-second `git worktree add`. This repo is small and the operation takes well under a second. It needs a genuinely slow checkout: a large repo, a cold filesystem cache, a network-mounted worktree root, or an LFS fetch. The code path itself is certain; the trigger is plausible and unmeasured. **Do not write the ticket up, or the commit message, as though a timeout has been observed.** ## What I want **Goal:** every exit from `add()` that can leave a partially-created worktree behind must clean it up, and the javadoc must stop claiming an assumption that is not true. **Invariants:** 1. The original exception must reach the caller unchanged. `cleanupAfterAddFailure` exists to tidy up, not to convert or swallow — a cleanup failure must not replace the real cause. 2. Cleanup must stay best-effort. A failure inside it gets logged, never thrown. 3. A failure where git genuinely created nothing must not become noisy. Cleaning up something that does not exist is fine; logging a scary warning every time an ordinary "branch already exists" error occurs is not. **Candidate mechanism**, offered as a candidate only: bring the `exec` inside the same try, so every failure after the point where git may have created state goes through one cleanup path. **Check invariant 3 before you commit to it** — that widens cleanup to cover ordinary git-reported failures too, where nothing was created, so make sure `cleanupAfterAddFailure` is quiet in that case. If it is not, say so and either make it quiet or narrow the fix to the timeout and interrupt paths, and justify which you chose. Also **fix the javadoc**. It currently states the false assumption as fact, and that sentence is what made this gap invisible. ## Rules - Prove it with a test that fails without the fix. You will need to make `exec` time out or be interrupted rather than waiting 30 real seconds — find the seam that lets you do that, and if there is no clean one, say so plainly and explain what you tested instead rather than inventing a test that reaches around the class. - **Never run `git worktree remove`, `git worktree prune`, or any destructive git command against this repo or its worktrees.** Other workers are live in them right now. Use a throwaway repo for experiments — and note in your report that a throwaway repo lacks this one's submodule, its `--skip-worktree` file and its per-worktree config, so say plainly which parts your test does and does not cover. - Do not run `git stash` — the stash is shared across every worktree here. - Mutation proof required: revert the fix, quote the real failure output, restore it. - Run `cd fleetd && mvn clean install` **unpiped**, and quote the real `Tests run:` and `BUILD` lines. Never pipe maven through `tail`/`head`, and never read `$?` after a pipe — after a pipe it is the last command's status, not Maven's.
ltms commented 2026-09-04 08:30:34 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main in b2a58cb.

What I checked myself

The fix introduced a data-loss path, and the worker found it and guarded it. This was not in my ticket. Moving git worktree add inside the try means cleanupAfterAddFailure now runs on an ordinary Git refusal too — including "a branch named X already exists" — and that cleanup calls deleteBranch(repoRoot, branch) with -D. Without a guard the fix would delete a pre-existing branch that the failed spawn never owned.

The worker's Files.exists(worktreePath) guard stops that. I proved both halves:

  1. Git really does create nothing on that refusal. Live probe in a throwaway repo:
$ git worktree add .../fresh1 -b mine HEAD
Preparing worktree (new branch 'mine')
fatal: a branch named 'mine' already exists
exit=255
$ ls -d .../fresh1
No such file or directory

So the guard's premise holds: no directory ⇒ nothing to clean ⇒ the branch is safe.

  1. My own mutation, different from the worker's. The worker reverted the placement change. I instead removed only the guard (if (!Files.exists(...)) → if (false)), keeping everything else:
[ERROR] GitWorktreesTest.addFailureBeforeCreatingAWorktreeIsQuiet:448
        the existing branch must remain ==> expected: <true> but was: <false>

The branch was in fact deleted. The risk is real, the guard closes it, and the test pins it.

Residual gap, accepted: if Git ever left a branch behind with no directory, the guard would skip cleanup and that branch leaks. That is a much smaller cost than deleting an operator's branch, and the probe above says Git does not do it on the failure that actually happens here.

Build

cd fleetd && mvn clean install, unpiped: Tests run: 1321, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Correction to my ticket

My ticket described this as a plain resource leak and said nothing about the branch-deletion risk the obvious fix creates. That is the third time in this batch that my brief was the weaker half. The worker reported the deviation and tested it — the outcome I ask for.

Merged to `main` in `b2a58cb`. ## What I checked myself **The fix introduced a data-loss path, and the worker found it and guarded it.** This was not in my ticket. Moving `git worktree add` inside the `try` means `cleanupAfterAddFailure` now runs on an ordinary Git refusal too — including "a branch named X already exists" — and that cleanup calls `deleteBranch(repoRoot, branch)` with `-D`. Without a guard the fix would delete a pre-existing branch that the failed spawn never owned. The worker's `Files.exists(worktreePath)` guard stops that. I proved both halves: 1. **Git really does create nothing on that refusal.** Live probe in a throwaway repo: ``` $ git worktree add .../fresh1 -b mine HEAD Preparing worktree (new branch 'mine') fatal: a branch named 'mine' already exists exit=255 $ ls -d .../fresh1 No such file or directory ``` So the guard's premise holds: no directory ⇒ nothing to clean ⇒ the branch is safe. 2. **My own mutation, different from the worker's.** The worker reverted the placement change. I instead removed *only* the guard (`if (!Files.exists(...))` → `if (false)`), keeping everything else: ``` [ERROR] GitWorktreesTest.addFailureBeforeCreatingAWorktreeIsQuiet:448 the existing branch must remain ==> expected: <true> but was: <false> ``` The branch was in fact deleted. The risk is real, the guard closes it, and the test pins it. **Residual gap, accepted:** if Git ever left a branch behind with no directory, the guard would skip cleanup and that branch leaks. That is a much smaller cost than deleting an operator's branch, and the probe above says Git does not do it on the failure that actually happens here. ## Build `cd fleetd && mvn clean install`, unpiped: `Tests run: 1321, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`. ## Correction to my ticket My ticket described this as a plain resource leak and said nothing about the branch-deletion risk the obvious fix creates. That is the third time in this batch that my brief was the weaker half. The worker reported the deviation and tested it — the outcome I ask for.
ltms closed this issue 2026-09-04 08:31:00 +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#309
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?