GitWorktrees.add() created the worktree and branch, then ran more steps that can throw — requireCredentialFreeHttpsOrigin among them, which is an intended security refusal, not an IO accident. Any throw meant add() never returned, so SessionManager.acquireWithWorktree never learned the path, its 'if (path != null)' cleanup could not fire, and the worktree and branch leaked with nothing tracking them. Every OTHER exit from that method was cleaned up correctly; only the exits inside add() were uncounted. add() now cleans up what it created before rethrowing, reusing remove() and additionally deleting the branch — a branch that never finished provisioning has no session and no PR behind it. Worktree first, since a checked-out branch cannot be deleted. Cleanup failure is logged and never masks the original exception. Verified by me: the real merge into current main builds green (1281 tests), and I reran the mutation myself without git stash — dropping the cleanup call fails the new test with 'the worktree directory leaked after a post-creation step threw'. The test drives add() itself through the existing afterWorktreeAdded seam, so the failure happens after the worktree exists rather than downstream in another caller.
This commit is contained in:
@@ -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).
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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.
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
* <p>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<String> 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. ----
|
||||
|
||||
|
||||
Reference in New Issue
Block a user