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

fleetd #274: clean up the worktree and branch when GitWorktrees.add() fails after creating them #277

Closed
agent wants to merge 0 commits from worker/fix-274-worktree-leak-b0095d-7 into main
pull from: worker/fix-274-worktree-leak-b0095d-7
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 05:07:05 +02:00
Member
Copy Link
Copy Source

fleetd #274 — GitWorktrees.add() leaks a worktree and branch when a post-creation step throws

The defect

GitWorktrees.add() created the worktree and its branch (git worktree add ... -b <branch>),
then ran several more steps that can throw: afterWorktreeAdded, requireCredentialFreeHttpsOrigin
(an intended security refusal, not only an IO accident), configureEnvironmentCredentialHelper,
configureHttpsUrlRewriteForSshOrigin, isolateToolSurface. If any of those threw, add() never
returned. In SessionManager.acquireWithWorktree the local path stayed null, so the catch
block's if (path != null) cleanup guard never ran — the worktree directory and its branch leaked
on disk forever, with nothing tracking them (the warning line even logged path=null).

The fix

fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java:

  • Wrapped the steps after git worktree add in a try/catch.
  • On failure, added cleanupAfterAddFailure(repoRoot, worktreePath, branch, original): reuses
    remove() (the same git worktree remove --force path every other exit in this class already
    uses) and additionally force-deletes the new branch (git branch -D) — remove() alone
    deliberately leaves a released session's branch behind for later recovery, but a branch that
    never finished provisioning has nothing else pointing at it, so it must go too.
  • Cleanup failures are only logged (matching the existing pattern in
    SessionManager.acquireWithWorktree's own catch block); the original exception is always
    rethrown, never masked.
  • SessionManager was not touched — its if (path != null) guard is correct for the failures
    it can actually see.

Test

fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java —
addCleansUpTheWorktreeAndBranchWhenAPostCreationStepThrows: drives add() itself via the
existing afterWorktreeAdded test seam (the same one
provisioningRefusesAWorktreeWhoseOriginStillHasHttpsUserInfo uses) with a mutation that trips
requireCredentialFreeHttpsOrigin moments later, strictly after the worktree and branch already
exist. Asserts both the worktree directory and the branch (refs/heads/<branch>) are gone.

Mutation check, done by hand: git stash push -- fleetd/.../GitWorktrees.java to revert the
fix while keeping the new test, then
mvn -Dtest=GitWorktreesTest#addCleansUpTheWorktreeAndBranchWhenAPostCreationStepThrows test.
Result: red —
AssertionFailedError: the worktree directory leaked after a post-creation step threw ==> expected: <false> but was: <true>. Fix restored afterward (see caveat below), then reconfirmed
green.

Also found (not fixed — out of scope)

Same shape (creates an external resource, then does more work that can throw before returning a
handle to the caller), read directly, not fixed:

  • fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java:788-818 (writeCharterFile):
    creates a temp dir/file (Files.createTempDirectory, under the memberHerdrSocket branch) then
    calls EnvAllowListScrub.shareWithGroup(dir, group), which can throw before the file handle is
    returned. Less severe than #274 — deleteOnExit() is registered, so a JVM exit eventually cleans
    it up — but the same "resource created, then more throwing work before the caller gets a handle"
    shape.

Build

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

Caveat for review — a shared-refs/stash race during the mutation check

This repo's refs/stash lives in the common git directory, so it is shared by every worktree,
not per-worktree. While I ran the mutation check (git stash push to revert my fix, run the test,
git stash pop to restore), a different concurrent worker
(worker/fix-273-exhausted-pattern-9665b5-6, fixing fleetd #273 in a sibling worktree of this same
repo) was doing its own git stash at the same time. My git stash pop popped their top-of-stack
entry (a FleetConfig.java change) into my working directory instead of my own entry, and — from
the trail left in git stash list/git fsck afterward — it looks like their own pop or push may
similarly have consumed mine.

I recovered by:

  1. Finding my own stash commit among git fsck --no-reflog's dangling commits (by grepping commit
    messages for fix-274-worktree-leak), confirming its diff matched exactly what I had written,
    and git checkout <that-sha> -- .../GitWorktrees.java to restore it.
  2. Reverting the stray FleetConfig.java change back to HEAD in my own worktree (confirmed via
    git diff — empty).
  3. Finding two commits that looked like the other worker's FleetConfig.java WIP among the same
    dangling-commit list (fix-273-exhausted-pattern, two candidate versions) and restoring both
    back onto the shared refs/stash stack (git update-ref -m "<original message>" refs/stash <sha>, once per candidate) so their lead/worker can recover whichever is current, since I can't
    tell from here which one is newer for them.

I did not touch FleetConfig.java as part of my own change — git diff against HEAD for
that file is empty in my final worktree state, and it is not part of this PR's diff. This is worth
flagging to whichever lead owns #273: their worktree's refs/stash-based flow (if any) may show
unexpected entries, and the two restored stash entries in the shared stack are named
worker/fix-273-exhausted-pattern-9665b5-6. This entire episode was a side effect of my own
mutation-check workflow using git stash, not caused by anything in the fix itself, and it never
touched any file in the eventual commit.

## fleetd #274 — GitWorktrees.add() leaks a worktree and branch when a post-creation step throws ### The defect `GitWorktrees.add()` created the worktree and its branch (`git worktree add ... -b <branch>`), then ran several more steps that can throw: `afterWorktreeAdded`, `requireCredentialFreeHttpsOrigin` (an intended security refusal, not only an IO accident), `configureEnvironmentCredentialHelper`, `configureHttpsUrlRewriteForSshOrigin`, `isolateToolSurface`. If any of those threw, `add()` never returned. In `SessionManager.acquireWithWorktree` the local `path` stayed `null`, so the catch block's `if (path != null)` cleanup guard never ran — the worktree directory and its branch leaked on disk forever, with nothing tracking them (the warning line even logged `path=null`). ### The fix `fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java`: - Wrapped the steps after `git worktree add` in a try/catch. - On failure, added `cleanupAfterAddFailure(repoRoot, worktreePath, branch, original)`: reuses `remove()` (the same `git worktree remove --force` path every other exit in this class already uses) and additionally force-deletes the new branch (`git branch -D`) — `remove()` alone deliberately leaves a *released* session's branch behind for later recovery, but a branch that never finished provisioning has nothing else pointing at it, so it must go too. - Cleanup failures are only logged (matching the existing pattern in `SessionManager.acquireWithWorktree`'s own catch block); the *original* exception is always rethrown, never masked. - `SessionManager` was **not** touched — its `if (path != null)` guard is correct for the failures it can actually see. ### Test `fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java` — `addCleansUpTheWorktreeAndBranchWhenAPostCreationStepThrows`: drives `add()` itself via the existing `afterWorktreeAdded` test seam (the same one `provisioningRefusesAWorktreeWhoseOriginStillHasHttpsUserInfo` uses) with a mutation that trips `requireCredentialFreeHttpsOrigin` moments later, strictly *after* the worktree and branch already exist. Asserts both the worktree directory and the branch (`refs/heads/<branch>`) are gone. **Mutation check, done by hand:** `git stash push -- fleetd/.../GitWorktrees.java` to revert the fix while keeping the new test, then `mvn -Dtest=GitWorktreesTest#addCleansUpTheWorktreeAndBranchWhenAPostCreationStepThrows test`. Result: **red** — `AssertionFailedError: the worktree directory leaked after a post-creation step threw ==> expected: <false> but was: <true>`. Fix restored afterward (see caveat below), then reconfirmed green. ### Also found (not fixed — out of scope) Same shape (creates an external resource, then does more work that can throw before returning a handle to the caller), read directly, not fixed: - `fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java:788-818` (`writeCharterFile`): creates a temp dir/file (`Files.createTempDirectory`, under the `memberHerdrSocket` branch) then calls `EnvAllowListScrub.shareWithGroup(dir, group)`, which can throw before the file handle is returned. Less severe than #274 — `deleteOnExit()` is registered, so a JVM exit eventually cleans it up — but the same "resource created, then more throwing work before the caller gets a handle" shape. ### Build `cd fleetd && mvn clean install` (unpiped): **BUILD SUCCESS**, `Tests run: 1275, Failures: 0, Errors: 0, Skipped: 0`. ### Caveat for review — a shared-`refs/stash` race during the mutation check This repo's `refs/stash` lives in the **common** git directory, so it is shared by every worktree, not per-worktree. While I ran the mutation check (`git stash push` to revert my fix, run the test, `git stash pop` to restore), a different concurrent worker (`worker/fix-273-exhausted-pattern-9665b5-6`, fixing fleetd #273 in a sibling worktree of this same repo) was doing its own `git stash` at the same time. My `git stash pop` popped *their* top-of-stack entry (a `FleetConfig.java` change) into my working directory instead of my own entry, and — from the trail left in `git stash list`/`git fsck` afterward — it looks like their own pop or push may similarly have consumed mine. I recovered by: 1. Finding my own stash commit among `git fsck --no-reflog`'s dangling commits (by grepping commit messages for `fix-274-worktree-leak`), confirming its diff matched exactly what I had written, and `git checkout <that-sha> -- .../GitWorktrees.java` to restore it. 2. Reverting the stray `FleetConfig.java` change back to `HEAD` in my own worktree (confirmed via `git diff` — empty). 3. Finding two commits that looked like the other worker's `FleetConfig.java` WIP among the same dangling-commit list (`fix-273-exhausted-pattern`, two candidate versions) and restoring *both* back onto the shared `refs/stash` stack (`git update-ref -m "<original message>" refs/stash <sha>`, once per candidate) so their lead/worker can recover whichever is current, since I can't tell from here which one is newer for them. I did **not** touch `FleetConfig.java` as part of my own change — `git diff` against `HEAD` for that file is empty in my final worktree state, and it is not part of this PR's diff. This is worth flagging to whichever lead owns #273: their worktree's `refs/stash`-based flow (if any) may show unexpected entries, and the two restored stash entries in the shared stack are named `worker/fix-273-exhausted-pattern-9665b5-6`. This entire episode was a side effect of my own mutation-check workflow using `git stash`, not caused by anything in the fix itself, and it never touched any file in the eventual commit.
agent added 1 commit 2026-09-04 05:07:05 +02:00
fleetd #274: clean up the worktree and branch when add() fails after creating them
CI / contract (pull_request) Successful in 56s
Details
CI / build (pull_request) Successful in 1m22s
Details
282a2fc2b8 
GitWorktrees.add() created the worktree and branch, then ran several more
steps that can throw (requireCredentialFreeHttpsOrigin — an intended
security refusal, not only an IO accident — plus the credential-helper and
tool-surface isolation steps). Any exception there meant add() never
returned, so its caller (SessionManager#acquireWithWorktree) never learned
the path: its local `path` stayed null, the `if (path != null)` cleanup
guard never ran, and the worktree directory and branch leaked on disk
forever with nothing tracking them.

Wrap those steps in try/catch; on failure, clean up via the same
`git worktree remove --force` path remove() already uses, additionally
force-delete the new branch (remove() alone deliberately leaves a
released session's branch behind, but a branch that never finished
provisioning has nothing else pointing at it), log the cleanup outcome,
and rethrow the original exception so it is never masked.

Test drives add() itself via the existing afterWorktreeAdded seam with a
mutation that trips requireCredentialFreeHttpsOrigin after the worktree
exists, then asserts both the worktree directory and the branch are gone.
Reverting the fix (git stash on GitWorktrees.java, test unchanged) turns
it red: "the worktree directory leaked after a post-creation step threw
==> expected: <false> but was: <true>". Restored afterward.

mvn clean install: BUILD SUCCESS, Tests run: 1275, Failures: 0, Errors: 0
ltms closed this pull request 2026-09-04 05:27:37 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 56s
Details
CI / build (pull_request) Successful in 1m22s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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#277
Delete Branch "worker/fix-274-worktree-leak-b0095d-7"

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?