Merge #309: clean up partial worktree state when git worktree add fails

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

@@ -93,6 +93,9 @@ public final class GitWorktrees implements Worktrees {
     /** OS group name for {@link #shareWithGroup} (fleetd #185 stage 3); {@code null} ⇒ feature off. */
     private final String group;
     private final Consumer<String> afterWorktreeAdded;
+    /** How the initial {@code git worktree add} command runs. Package-private test seam for an
+     *  interrupted command after Git has made worktree state. */
+    private final Function<String[], String> worktreeAddRunner;
     /** How {@link #shareWithGroup}'s processes (git config / chgrp / chmod / find) actually run.
      *  Defaults to the real {@link #exec(String...)}. Package-private test seam so a unit test can
      *  prove "no group configured ⇒ zero processes spawned" and inspect exactly what a configured
@@ -132,7 +135,13 @@ public final class GitWorktrees implements Worktrees {
 
     /** Test seam combining a configurable {@code group} with {@link #afterWorktreeAdded}. */
     GitWorktrees(String configuredRoot, String group, Consumer<String> afterWorktreeAdded) {
-        this(configuredRoot, group, afterWorktreeAdded, null);
+        this(configuredRoot, group, afterWorktreeAdded, null, null);
+    }
+
+    /** Test seam for changing how {@link #shareWithGroup}'s processes run. */
+    GitWorktrees(String configuredRoot, String group, Consumer<String> afterWorktreeAdded,
+                  Function<String[], String> shareGroupRunner) {
+        this(configuredRoot, group, afterWorktreeAdded, shareGroupRunner, null);
     }
 
     /**
@@ -141,13 +150,15 @@ public final class GitWorktrees implements Worktrees {
      * exactly what commands a configured group runs, without a real second OS user/group.
      *
      * @param shareGroupRunner {@code null} ⇒ the real {@link #exec(String...)}.
+     * @param worktreeAddRunner {@code null} ⇒ the real {@link #exec(String...)}.
      */
     GitWorktrees(String configuredRoot, String group, Consumer<String> afterWorktreeAdded,
-                 Function<String[], String> shareGroupRunner) {
+                  Function<String[], String> shareGroupRunner, Function<String[], String> worktreeAddRunner) {
         this.configuredRoot = configuredRoot;
         this.group = (group == null || group.isBlank()) ? null : group;
         this.afterWorktreeAdded = afterWorktreeAdded == null ? _ -> {} : afterWorktreeAdded;
         this.shareGroupRunner = shareGroupRunner != null ? shareGroupRunner : this::exec;
+        this.worktreeAddRunner = worktreeAddRunner != null ? worktreeAddRunner : this::exec;
     }
 
     @Override
@@ -166,8 +177,8 @@ public final class GitWorktrees implements Worktrees {
         String wt = path.toAbsolutePath().toString();
         log.info("adding worktree branch={} path={} base={}", branch, wt, base);
         removeUserInfoFromHttpsOrigin(repoRoot);
-        exec("git", "-C", repoRoot, "worktree", "add", wt, "-b", branch, base);
         try {
+            worktreeAddRunner.apply(new String[] {"git", "-C", repoRoot, "worktree", "add", wt, "-b", branch, base});
             afterWorktreeAdded.accept(wt);
             requireCredentialFreeHttpsOrigin(wt);
             configureEnvironmentCredentialHelper(repoRoot, wt);
@@ -181,10 +192,11 @@ public final class GitWorktrees implements Worktrees {
     }
 
     /**
-     * {@code add()} has already created the worktree and its branch by the time any step from
-     * {@link #afterWorktreeAdded} through {@link #isolateToolSurface} can throw — including
+     * {@code git worktree add} may have created the worktree and its branch by the time it, or any
+     * later step through {@link #isolateToolSurface}, throws. This includes
      * {@link #requireCredentialFreeHttpsOrigin}, an intended security refusal, not only an IO
-     * accident. Without this, {@code add()} never returns, so its caller
+     * accident. A Git-reported {@code worktree add} failure usually creates nothing, but an
+     * interrupted command can leave partial state. Without cleanup, {@code add()} never returns, so its caller
      * ({@code SessionManager#acquireWithWorktree}) never receives a path to register or clean up:
      * its local {@code path} stays null, the {@code if (path != null)} guard in its own catch block
      * never runs, and the worktree directory and branch leak on disk forever with nothing tracking
@@ -204,6 +216,10 @@ public final class GitWorktrees implements Worktrees {
      * used in {@code SessionManager#acquireWithWorktree}'s own catch block.
      */
     private void cleanupAfterAddFailure(String repoRoot, String worktreePath, String branch, RuntimeException original) {
+        if (!Files.exists(Path.of(worktreePath))) {
+            log.debug("provisioning failed before worktree {} existed; nothing to clean up", worktreePath);
+            return;
+        }
         log.warn("provisioning failed for branch={} path={}: {} — cleaning up before rethrowing",
                 branch, worktreePath, original.getMessage());
         try {

fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java

@@ -403,6 +403,52 @@ class GitWorktreesTest {
         assertTrue(heads.isBlank(), "the branch leaked after a post-creation step threw:\n" + heads);
     }
 
+    /**
+     * fleetd #309. The add runner is a narrow seam for the case where Git has created state but
+     * the caller then kills the process. The runner first performs the real add in this throwaway
+     * repo, then throws the same kind of exception that {@link GitWorktrees#exec} uses for a timeout.
+     * This proves the failure path cleans both real Git objects without waiting for a slow checkout.
+     */
+    @Test
+    void addCleansUpWhenTheWorktreeAddRunnerFailsAfterCreatingState(@TempDir Path tmp) throws Exception {
+        Path repo = initRepo(tmp.resolve("repo"));
+        String branch = "cb-309-timeout";
+        WorktreeException timeout = new WorktreeException("command timed out: synthetic git worktree add");
+        AtomicReference<String> createdPath = new AtomicReference<>();
+        GitWorktrees worktrees = new GitWorktrees(tmp.resolve("wts").toString(), null, null, null, command -> {
+            createdPath.set(command[5]);
+            try {
+                git(repo, "worktree", "add", command[5], "-b", command[7], command[8]);
+            } catch (Exception e) {
+                throw new AssertionError("test setup could not create the worktree", e);
+            }
+            throw timeout;
+        });
+
+        WorktreeException thrown = assertThrows(WorktreeException.class,
+                () -> worktrees.add(repo.toString(), branch, "HEAD"));
+
+        assertSame(timeout, thrown, "cleanup must not replace the add failure");
+        assertNotNull(createdPath.get(), "the add runner must receive the worktree path");
+        assertFalse(Files.exists(Path.of(createdPath.get())),
+                "the worktree directory leaked after the add runner failed");
+        assertFalse(refExists(repo, "refs/heads/" + branch), "the branch leaked after the add runner failed");
+    }
+
+    /** An ordinary Git refusal must not delete the existing branch or log a cleanup warning. */
+    @Test
+    void addFailureBeforeCreatingAWorktreeIsQuiet(@TempDir Path tmp) throws Exception {
+        Path repo = initRepo(tmp.resolve("repo"));
+        String branch = "already-exists";
+        git(repo, "branch", branch);
+
+        assertThrows(WorktreeException.class, () -> new GitWorktrees(tmp.resolve("wts").toString())
+                .add(repo.toString(), branch, "HEAD"));
+
+        assertTrue(refExists(repo, "refs/heads/" + branch), "the existing branch must remain");
+        assertTrue(capturedMessages().isEmpty(), "an ordinary Git refusal logged a warning: " + capturedMessages());
+    }
+
     // ---- CB-189: broader remote-URL coverage — every remote, both fetch and push URLs, any
     // non-SSH scheme. Reporting only, additive to the origin/https strip-and-refuse tests above. ----