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 fc383f0..2534c61 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -167,14 +167,59 @@ public final class GitWorktrees implements Worktrees { log.info("adding worktree branch={} path={} base={}", branch, wt, base); removeUserInfoFromHttpsOrigin(repoRoot); exec("git", "-C", repoRoot, "worktree", "add", wt, "-b", branch, base); - afterWorktreeAdded.accept(wt); - requireCredentialFreeHttpsOrigin(wt); - configureEnvironmentCredentialHelper(repoRoot, wt); - configureHttpsUrlRewriteForSshOrigin(repoRoot, wt); - isolateToolSurface(wt); + try { + afterWorktreeAdded.accept(wt); + requireCredentialFreeHttpsOrigin(wt); + configureEnvironmentCredentialHelper(repoRoot, wt); + configureHttpsUrlRewriteForSshOrigin(repoRoot, wt); + isolateToolSurface(wt); + } catch (RuntimeException e) { + cleanupAfterAddFailure(repoRoot, wt, branch, e); + throw e; + } return wt; } + /** + * {@code add()} has already created the worktree and its branch by the time any step from + * {@link #afterWorktreeAdded} through {@link #isolateToolSurface} can throw — including + * {@link #requireCredentialFreeHttpsOrigin}, an intended security refusal, not only an IO + * accident. Without this, {@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 + * them (fleetd #274). + * + *

Reuses {@link #remove} — the same {@code git worktree remove --force} path every other + * cleanup exit in this class already goes through — rather than a bespoke removal. It + * additionally deletes {@code branch}: {@link #remove} alone deliberately leaves a released + * session's branch behind (a worker's branch is expected to outlive its worktree, for PRs and + * recovery), but a branch that never finished provisioning has no session, no PR, and nothing + * else pointing at it, so leaving it behind would just trade one leak for a smaller one. Forced + * (`-D`) because the branch is new and unmerged by construction. The worktree is removed first: + * a branch checked out by a worktree cannot be deleted until the worktree that holds it is gone. + * + *

Cleanup failure must never mask {@code original} — that is the exception that explains + * what actually went wrong — so a failure here is only logged, matching the pattern already + * used in {@code SessionManager#acquireWithWorktree}'s own catch block. + */ + private void cleanupAfterAddFailure(String repoRoot, String worktreePath, String branch, RuntimeException original) { + log.warn("provisioning failed for branch={} path={}: {} — cleaning up before rethrowing", + branch, worktreePath, original.getMessage()); + try { + remove(repoRoot, worktreePath); + } catch (RuntimeException cleanup) { + log.warn("failed to remove leaked worktree {} after provisioning error: {}", + worktreePath, cleanup.getMessage()); + } + try { + exec("git", "-C", repoRoot, "branch", "-D", branch); + } catch (RuntimeException cleanup) { + log.warn("failed to remove leaked branch {} after provisioning error: {}", + branch, cleanup.getMessage()); + } + } + /** * A linked worktree shares its primary checkout's git config. Remove HTTPS user info before * adding one, so a credential accidentally embedded in that config cannot reach the member. 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 523b9fd..0cdf9ff 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -21,6 +21,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import static org.junit.jupiter.api.Assertions.*; @@ -362,6 +363,46 @@ class GitWorktreesTest { assertEquals("worktree origin contains HTTPS user info; refusing provision", error.getMessage()); } + /** + * fleetd #274. {@code add()} creates the worktree and its branch, then runs several more steps + * that can throw — {@code requireCredentialFreeHttpsOrigin} among them, an intended security + * refusal, not an IO accident. Before the fix, any exception from those later steps left + * {@code add()} never returning, so its caller never learned the path and the worktree + * directory plus its branch leaked on disk forever with nothing tracking them. + * + *

This drives the exact same {@code afterWorktreeAdded} test seam as + * {@link #provisioningRefusesAWorktreeWhoseOriginStillHasHttpsUserInfo} — a mutation applied + * right after {@code git worktree add}, so the step that throws + * ({@code requireCredentialFreeHttpsOrigin}, reached moments later inside {@code add()} itself) + * runs strictly after the worktree and branch already exist, not downstream of {@code add()} + * in some other caller. {@code afterWorktreeAdded} also hands back the created path, so the + * assertions below don't have to guess the generated nonce. + */ + @Test + void addCleansUpTheWorktreeAndBranchWhenAPostCreationStepThrows(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + git(repo, "remote", "add", "origin", "https://git.ltms.dev/akb/kb.git"); + String branch = "cb-274-leak"; + AtomicReference createdPath = new AtomicReference<>(); + GitWorktrees worktrees = new GitWorktrees(tmp.resolve("wts").toString(), worktreePath -> { + createdPath.set(worktreePath); + try { + git(Path.of(worktreePath), "remote", "set-url", "origin", + "https://synthetic-test-token@git.ltms.dev/akb/kb.git"); + } catch (Exception e) { + throw new RuntimeException(e); + } + }); + + assertThrows(WorktreeException.class, () -> worktrees.add(repo.toString(), branch, "HEAD")); + + assertNotNull(createdPath.get(), "afterWorktreeAdded must have run with the created path"); + assertFalse(Files.exists(Path.of(createdPath.get())), + "the worktree directory leaked after a post-creation step threw"); + String heads = forEachRef(repo, "refs/heads/" + branch); + assertTrue(heads.isBlank(), "the branch leaked after a post-creation step threw:\n" + heads); + } + // ---- 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. ----