From 002329adb518ccd994ede8aed8ce7e54244ce25f Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 13:18:57 +0700 Subject: [PATCH] #309: clean partial worktrees after add failure --- .../dev/ltms/fleet/session/GitWorktrees.java | 28 ++++++++--- .../ltms/fleet/session/GitWorktreesTest.java | 46 +++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index a9d2d86..1da0f95 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/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 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 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 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 afterWorktreeAdded, + Function 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 afterWorktreeAdded, - Function shareGroupRunner) { + Function shareGroupRunner, Function 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 { diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java index 0cdf9ff..971c4f8 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/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 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. ----