fix: preserve worktree content during automatic cleanup (#3519)
## Summary - Centralize fail-closed cleanliness validation in the shared `removeWorktree` path for every automatic cleanup reason: pool prune, idle/cap sweeps, self-healing reclaim/conflict/stale-branch cleanup, merger cleanup, and step-session cleanup. - Preserve tracked, untracked, ignored, or unverifiable checkout content; native Git revalidates without `--force` at the deletion boundary and never falls through to recursive deletion for defensive callers. - Keep explicit destructive teardown reasons on their legacy forced path. - Replace recursive removal of unregistered worktree directories with non-recursive `rmdir`. - Preserve `.env` when cleanup is refused; prune only metadata for defensive registrations whose path already disappeared. - Use argv-based `execFileSync` in the real-worktree test fixture and provide labeled release metadata. ## Why this replaces #3461 #3461 accumulated quarantine, recovery, generated-residue classification, and race-repair machinery while addressing successive bot reviews. This replacement keeps one essential invariant in one shared path: > Automatic cleanup removes a worktree only when cleanliness can be proven; otherwise it preserves the checkout. ### Original-review requirements retained - Real Git regressions exercise dirty, clean, ignored `dist/manual.txt`, probe-failure, missing-path, and backend-routing paths. - Every automatic reclaim reason uses the same defensive guard. - Failed status probes and dangling/unregistered content fail closed. - Native defensive removal never uses the recursive fallback. - Worktrunk receives `--force` only from explicit caller intent. - A refused reclaim does not pre-delete the managed environment file. ### Deliberate simplifications - No pathname heuristics for `node_modules`, `dist`, or build output. - No quarantine, recovery directory, pointer stash, or restoration lifecycle. - No destructive reclamation of corrupt/dangling metadata when safety cannot be proven. These omissions prefer a recoverable disk leak over possible user-data loss. Add targeted reclamation only if operational evidence shows the conservative behavior is insufficient. ## Verification - `worktree-backend.test.ts`: 64 passed - `worktree-defensive-removal-preservation.real-git.test.ts`: 13 passed - `self-healing-tempdir-sweep.test.ts`: 26 passed - `worktree-pool.test.ts`: 60 passed - `worktree-pool-secrets-env-cleanup.test.ts`: 2 passed - `worktrunk-worktree-removal.test.ts`: 2 passed - `worktree-acquisition.test.ts`: 47 passed - Engine typecheck passed - ESLint passed - Changeset status passed - `git diff --check` passed ## Scope Worktree cleanup safety only. Supersedes #3461. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Worktree cleanup now preserves directories containing uncommitted, ignored, unverifiable, or user-created files. * Cleanup fails safely when worktree status cannot be verified. * Clean worktrees continue to be removed normally, while selected forced teardown scenarios retain their existing behavior. * Dangling or orphaned worktree directories are preserved instead of being deleted unexpectedly. * **Tests** * Added coverage for defensive removal, orphan preservation, and clean worktree pruning. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
7
.changeset/preserve-dirty-worktrees-minimal.md
Normal file
7
.changeset/preserve-dirty-worktrees-minimal.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Preserve dirty or unverifiable worktrees during automatic cleanup.
|
||||
category: fix
|
||||
dev: Automatic cleanup now fails closed for unverified content and revalidates cleanliness without force at removal time.
|
||||
@@ -0,0 +1,153 @@
|
||||
import { access, constants as fsConstants, mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import { dirname, join } from "node:path";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import {
|
||||
RemovalReason,
|
||||
removeWorktree,
|
||||
} from "../../worktree/worktree-backend.js";
|
||||
import { reapOrphanWorktrees } from "../../worktree/worktree-pool.js";
|
||||
import { git, hasGit } from "./_helpers.js";
|
||||
|
||||
async function pathExists(path: string): Promise<boolean> {
|
||||
try {
|
||||
await access(path, fsConstants.F_OK);
|
||||
return true;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
describe.skipIf(!hasGit)("reliability interactions: defensive removal preserves unverifiable content", () => {
|
||||
const roots: string[] = [];
|
||||
|
||||
afterEach(async () => {
|
||||
await Promise.all(roots.map((root) => rm(root, { recursive: true, force: true })));
|
||||
roots.length = 0;
|
||||
});
|
||||
|
||||
async function setupRepo(): Promise<string> {
|
||||
const root = await mkdtemp(join(tmpdir(), "fusion-defensive-remove-"));
|
||||
roots.push(root);
|
||||
git(root, "git init -b main");
|
||||
git(root, 'git config user.email "test@example.com"');
|
||||
git(root, 'git config user.name "Test User"');
|
||||
await writeFile(join(root, "README.md"), "# repo\n", "utf-8");
|
||||
await writeFile(join(root, ".gitignore"), "dist/\n", "utf-8");
|
||||
git(root, "git add README.md .gitignore");
|
||||
git(root, 'git commit -m "init"');
|
||||
await mkdir(join(root, ".worktrees"), { recursive: true });
|
||||
return root;
|
||||
}
|
||||
|
||||
async function createWorktree(root: string, name: string): Promise<string> {
|
||||
const worktreePath = join(root, ".worktrees", name);
|
||||
git(root, `git worktree add -b ${JSON.stringify(`fusion/${name}`)} ${JSON.stringify(worktreePath)}`);
|
||||
return worktreePath;
|
||||
}
|
||||
|
||||
it("pool-prune refuses and preserves a dirty registered worktree", async () => {
|
||||
const root = await setupRepo();
|
||||
const worktreePath = await createWorktree(root, "dirty-prune");
|
||||
await writeFile(join(worktreePath, "wip.txt"), "uncommitted\n", "utf-8");
|
||||
|
||||
await expect(
|
||||
removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.PoolPrune }),
|
||||
).rejects.toThrow(/preserving/);
|
||||
|
||||
expect(await readFile(join(worktreePath, "wip.txt"), "utf-8")).toBe("uncommitted\n");
|
||||
});
|
||||
|
||||
it("idle-sweep refuses and preserves a dirty registered worktree", async () => {
|
||||
const root = await setupRepo();
|
||||
const worktreePath = await createWorktree(root, "dirty-idle");
|
||||
await writeFile(join(worktreePath, "wip.txt"), "uncommitted\n", "utf-8");
|
||||
|
||||
await expect(
|
||||
removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.SelfHealingIdleSweep }),
|
||||
).rejects.toThrow(/preserving/);
|
||||
|
||||
expect(await pathExists(worktreePath)).toBe(true);
|
||||
});
|
||||
|
||||
it("pool-prune preserves user content under an ignored generated-looking path", async () => {
|
||||
const root = await setupRepo();
|
||||
const worktreePath = await createWorktree(root, "ignored-prune");
|
||||
await mkdir(join(worktreePath, "dist"), { recursive: true });
|
||||
await writeFile(join(worktreePath, "dist", "manual.txt"), "precious\n", "utf-8");
|
||||
|
||||
await expect(
|
||||
removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.PoolPrune }),
|
||||
).rejects.toThrow(/preserving/);
|
||||
|
||||
expect(await readFile(join(worktreePath, "dist", "manual.txt"), "utf-8")).toBe("precious\n");
|
||||
});
|
||||
|
||||
it.each([
|
||||
RemovalReason.MergerCleanup,
|
||||
RemovalReason.MergerPostMerge,
|
||||
RemovalReason.SelfHealingBranchConflict,
|
||||
RemovalReason.SelfHealingReclaim,
|
||||
RemovalReason.SelfHealingStaleActiveBranch,
|
||||
RemovalReason.StepSessionCleanup,
|
||||
])("%s preserves a dirty automatically managed worktree", async (reason) => {
|
||||
const root = await setupRepo();
|
||||
const worktreePath = await createWorktree(root, reason);
|
||||
await writeFile(join(worktreePath, "wip.txt"), "uncommitted\n", "utf-8");
|
||||
|
||||
await expect(removeWorktree({ rootDir: root, worktreePath, settings: {}, reason })).rejects.toThrow(/preserving/);
|
||||
|
||||
expect(await readFile(join(worktreePath, "wip.txt"), "utf-8")).toBe("uncommitted\n");
|
||||
});
|
||||
|
||||
it("a failing status probe preserves the checkout instead of enabling deletion", async () => {
|
||||
const root = await setupRepo();
|
||||
const worktreePath = await createWorktree(root, "corrupt-probe");
|
||||
// Corrupt the registration so the cleanliness probe cannot run at all.
|
||||
await rm(join(root, ".git", "worktrees", "corrupt-probe"), { recursive: true, force: true });
|
||||
|
||||
await expect(
|
||||
removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.PoolPrune }),
|
||||
).rejects.toThrow(/preserving/);
|
||||
|
||||
expect(await pathExists(join(worktreePath, "README.md"))).toBe(true);
|
||||
});
|
||||
|
||||
it("clean registered worktrees are still removed by pool-prune", async () => {
|
||||
const root = await setupRepo();
|
||||
const worktreePath = await createWorktree(root, "clean-prune");
|
||||
|
||||
await removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.PoolPrune });
|
||||
|
||||
expect(await pathExists(worktreePath)).toBe(false);
|
||||
});
|
||||
|
||||
it("addressed teardown reasons keep their legacy forced semantics on dirty worktrees", async () => {
|
||||
const root = await setupRepo();
|
||||
const worktreePath = await createWorktree(root, "task-reset-dirty");
|
||||
await writeFile(join(worktreePath, "wip.txt"), "uncommitted\n", "utf-8");
|
||||
|
||||
await removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.TaskReset });
|
||||
|
||||
expect(await pathExists(worktreePath)).toBe(false);
|
||||
});
|
||||
|
||||
it("startup reaper preserves dangling or content orphans", async () => {
|
||||
const root = await setupRepo();
|
||||
|
||||
const danglingOrphan = join(root, ".worktrees", "dangling-orphan");
|
||||
await mkdir(danglingOrphan, { recursive: true });
|
||||
await writeFile(join(danglingOrphan, ".git"), "gitdir: /nonexistent/admin\n", "utf-8");
|
||||
|
||||
const contentOrphan = join(root, ".worktrees", "content-orphan");
|
||||
await mkdir(contentOrphan, { recursive: true });
|
||||
await writeFile(join(contentOrphan, ".git"), "gitdir: /nonexistent/admin\n", "utf-8");
|
||||
await writeFile(join(contentOrphan, "wip.txt"), "precious\n", "utf-8");
|
||||
|
||||
await reapOrphanWorktrees(root);
|
||||
|
||||
expect(await pathExists(join(danglingOrphan, ".git"))).toBe(true);
|
||||
expect(await readFile(join(contentOrphan, "wip.txt"), "utf-8")).toBe("precious\n");
|
||||
expect(dirname(contentOrphan)).toBe(join(root, ".worktrees"));
|
||||
});
|
||||
});
|
||||
@@ -5,16 +5,21 @@ import { cleanupOrphanedWorktrees } from "../../worktree/worktree-pool.js";
|
||||
import { SelfHealingManager } from "../../self-healing.js";
|
||||
import { NativeWorktreeBackend, WorktrunkWorktreeBackend } from "../../worktree/worktree-backend.js";
|
||||
|
||||
const { execSpy, existsSpy, readdirSpy, readFileSpy } = vi.hoisted(() => ({
|
||||
execSpy: vi.fn(),
|
||||
existsSpy: vi.fn(() => true),
|
||||
readdirSpy: vi.fn(() => []),
|
||||
readFileSpy: vi.fn(() => ""),
|
||||
}));
|
||||
const { execSpy, execFileSpy, existsSpy, readdirSpy, readFileSpy } = vi.hoisted(() => {
|
||||
const execFileSpy = vi.fn().mockResolvedValue({ stdout: "", stderr: "" });
|
||||
(execFileSpy as any)[Symbol.for("nodejs.util.promisify.custom")] = execFileSpy;
|
||||
return {
|
||||
execSpy: vi.fn(),
|
||||
execFileSpy,
|
||||
existsSpy: vi.fn(() => true),
|
||||
readdirSpy: vi.fn(() => []),
|
||||
readFileSpy: vi.fn(() => ""),
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("node:child_process", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("node:child_process")>();
|
||||
return { ...actual, exec: execSpy };
|
||||
return { ...actual, exec: execSpy, execFile: execFileSpy };
|
||||
});
|
||||
|
||||
vi.mock("node:fs", async (importOriginal) => {
|
||||
@@ -39,6 +44,8 @@ describe("reliability interactions: worktrunk worktree removal routing", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
execSpy.mockImplementation((_cmd: string, _opts: unknown, cb: (err: unknown, stdout: string, stderr: string) => void) => cb(null, "", ""));
|
||||
execFileSpy.mockReset();
|
||||
execFileSpy.mockResolvedValue({ stdout: "", stderr: "" });
|
||||
// A workspace-group marker is an explicit delete veto in the ownership proof; these fixtures
|
||||
// are ordinary single-project worktrees, so the marker must be absent.
|
||||
existsSpy.mockImplementation(((path: string) => !String(path).endsWith("/.fusion-workspace-root")) as never);
|
||||
|
||||
@@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, utimesSync, writeFileSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { execFileSync, execSync } from "node:child_process";
|
||||
|
||||
const osState = vi.hoisted(() => ({ tempRoot: "" }));
|
||||
const fsState = vi.hoisted(() => ({
|
||||
@@ -174,6 +175,19 @@ function makeReclaimableWorktree(path: string, name: string): void {
|
||||
writeFileSync(join(path, ".git"), `gitdir: ${join(projectRoot, ".git", "worktrees", name)}\n`);
|
||||
}
|
||||
|
||||
function makeRealIdleWorktree(root: string, name: string): string {
|
||||
// Create a genuine git worktree with admin entry so the status probe succeeds.
|
||||
execSync("git init -b main", { cwd: root });
|
||||
execSync('git config user.email "test@example.com"', { cwd: root });
|
||||
execSync('git config user.name "Test"', { cwd: root });
|
||||
writeFileSync(join(root, "README.md"), "# fixture\n");
|
||||
execSync("git add README.md", { cwd: root });
|
||||
execSync('git commit -m init', { cwd: root });
|
||||
const worktreeDir = join(root, ".worktrees", name);
|
||||
execFileSync("git", ["worktree", "add", "-b", `fusion/${name}`, worktreeDir], { cwd: root });
|
||||
return worktreeDir;
|
||||
}
|
||||
|
||||
async function sweep(manager: SelfHealingManager): Promise<number> {
|
||||
return await (manager as any).cleanupStaleTempMergeWorktrees();
|
||||
}
|
||||
@@ -183,7 +197,7 @@ function sweepAudits(audits: any[]) {
|
||||
}
|
||||
|
||||
describe("SelfHealingManager worktrees-dir sweeps", () => {
|
||||
it("excludes internal containers from unregistered-orphan reap while removing genuine orphans", async () => {
|
||||
it("excludes internal containers and preserves unverifiable unregistered orphans", async () => {
|
||||
const worktreesDir = join(projectRoot, ".worktrees");
|
||||
const aiMergeContainer = join(worktreesDir, ".ai-merge");
|
||||
const recoveryContainer = join(worktreesDir, ".fusion-recovery");
|
||||
@@ -193,12 +207,12 @@ describe("SelfHealingManager worktrees-dir sweeps", () => {
|
||||
makeReclaimableWorktree(orphan, "half-built");
|
||||
const { manager } = makeManager({ recycleWorktrees: true });
|
||||
|
||||
await expect((manager as any).reapUnregisteredOrphans()).resolves.toBe(1);
|
||||
await expect((manager as any).reapUnregisteredOrphans()).resolves.toBe(0);
|
||||
|
||||
expect(existsSync(aiMergeContainer)).toBe(true);
|
||||
expect(existsSync(recoveryContainer)).toBe(true);
|
||||
expect(existsSync(orphan)).toBe(false);
|
||||
expect(fsState.rmCalls).toContain(orphan);
|
||||
expect(existsSync(orphan)).toBe(true);
|
||||
expect(fsState.rmCalls).not.toContain(orphan);
|
||||
expect(fsState.rmCalls).not.toContain(aiMergeContainer);
|
||||
expect(fsState.rmCalls).not.toContain(recoveryContainer);
|
||||
});
|
||||
@@ -207,10 +221,9 @@ describe("SelfHealingManager worktrees-dir sweeps", () => {
|
||||
const worktreesDir = join(projectRoot, ".worktrees");
|
||||
const aiMergeContainer = join(worktreesDir, ".ai-merge");
|
||||
const recoveryContainer = join(worktreesDir, ".fusion-recovery");
|
||||
const idle = join(worktreesDir, "idle-wt");
|
||||
mkdirSync(aiMergeContainer, { recursive: true });
|
||||
mkdirSync(recoveryContainer, { recursive: true });
|
||||
makeReclaimableWorktree(idle, "idle-wt");
|
||||
makeRealIdleWorktree(projectRoot, "idle-wt");
|
||||
childState.execStdout = gitWorktreeList(["idle-wt"]);
|
||||
const { manager } = makeManager({ maxWorktrees: 0 });
|
||||
|
||||
|
||||
@@ -12,6 +12,7 @@ import { activeSessionRegistry } from "../agents/active-session-registry.js";
|
||||
|
||||
const {
|
||||
execMock,
|
||||
execFileMock,
|
||||
accessMock,
|
||||
rmMock,
|
||||
chmodMock,
|
||||
@@ -26,8 +27,11 @@ const {
|
||||
} = vi.hoisted(() => {
|
||||
const mock = vi.fn();
|
||||
(mock as any)[Symbol.for("nodejs.util.promisify.custom")] = mock;
|
||||
const execFileMock = vi.fn().mockResolvedValue({ stdout: "", stderr: "" });
|
||||
(execFileMock as any)[Symbol.for("nodejs.util.promisify.custom")] = execFileMock;
|
||||
return {
|
||||
execMock: mock,
|
||||
execFileMock,
|
||||
accessMock: vi.fn(),
|
||||
rmMock: vi.fn(),
|
||||
chmodMock: vi.fn(),
|
||||
@@ -42,7 +46,7 @@ const {
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("node:child_process", () => ({ exec: execMock, execFile: vi.fn() }));
|
||||
vi.mock("node:child_process", () => ({ exec: execMock, execFile: execFileMock }));
|
||||
vi.mock("node:fs", () => ({ existsSync: existsSyncMock }));
|
||||
vi.mock("node:fs/promises", () => ({ access: accessMock, chmod: chmodMock, rm: rmMock }));
|
||||
vi.mock("../execution/branch-conflicts.js", () => ({
|
||||
@@ -79,6 +83,8 @@ vi.mock("../worktree/worktree-prune.js", () => ({
|
||||
|
||||
beforeEach(() => {
|
||||
execMock.mockReset();
|
||||
execFileMock.mockReset();
|
||||
execFileMock.mockResolvedValue({ stdout: "", stderr: "" });
|
||||
accessMock.mockReset();
|
||||
rmMock.mockReset();
|
||||
rmMock.mockResolvedValue(undefined as never);
|
||||
@@ -278,6 +284,24 @@ describe("NativeWorktreeBackend", () => {
|
||||
expect(pruneWorktreeAdminEntriesMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("prunes a missing defensive worktree registration without recursive fallback", async () => {
|
||||
execMock.mockRejectedValueOnce({ stderr: "fatal: '/repo/.worktrees/fn-1' is not a working tree" });
|
||||
existsSyncMock.mockReturnValue(false);
|
||||
|
||||
await new NativeWorktreeBackend().remove({
|
||||
rootDir: "/repo",
|
||||
worktreePath: "/repo/.worktrees/fn-1",
|
||||
force: false,
|
||||
});
|
||||
|
||||
expect(rmMock).not.toHaveBeenCalled();
|
||||
expect(pruneWorktreeAdminEntriesMock).toHaveBeenCalledWith(expect.objectContaining({
|
||||
rootDir: "/repo",
|
||||
reason: "remove-missing-fallback",
|
||||
target: "/repo/.worktrees/fn-1",
|
||||
}));
|
||||
});
|
||||
|
||||
it("retries errno-only recoverable cleanup failures before pruning once", async () => {
|
||||
const busy = Object.assign(new Error("busy"), { code: "EBUSY" });
|
||||
execMock.mockRejectedValueOnce(busy);
|
||||
@@ -954,7 +978,7 @@ describe("removeWorktree", () => {
|
||||
});
|
||||
|
||||
expect(execMock).toHaveBeenCalledWith(
|
||||
'git worktree remove --force "/repo/.worktrees/fn-1"',
|
||||
'git worktree remove "/repo/.worktrees/fn-1"',
|
||||
expect.objectContaining({ cwd: "/repo", timeout: 60000 }),
|
||||
);
|
||||
expect(audit.git).toHaveBeenCalledWith({ type: "worktree:remove", target: "/repo/.worktrees/fn-1" });
|
||||
|
||||
@@ -36,8 +36,8 @@ afterEach(async () => {
|
||||
await Promise.all(dirs.splice(0).map((d) => rm(d, { recursive: true, force: true })));
|
||||
});
|
||||
|
||||
describe("worktree-pool secrets cleanup hooks", () => {
|
||||
it("reapOrphanWorktrees invokes cleanup before removal", async () => {
|
||||
describe("worktree-pool secrets preservation", () => {
|
||||
it("preserves an unverifiable orphan instead of deleting its environment file", async () => {
|
||||
cleanupSecretsEnvFile.mockResolvedValue({ outcome: "cleaned", reason: "fingerprint-match" });
|
||||
const root = tmpRoot();
|
||||
const worktrees = join(root, ".worktrees");
|
||||
@@ -48,15 +48,12 @@ describe("worktree-pool secrets cleanup hooks", () => {
|
||||
const mod = await import("../worktree/worktree-pool.js");
|
||||
const removed = await mod.reapOrphanWorktrees(root);
|
||||
|
||||
expect(removed).toBe(1);
|
||||
expect(cleanupSecretsEnvFile).toHaveBeenCalledWith(expect.objectContaining({
|
||||
worktreePath: orphan,
|
||||
taskId: "orphan:orphan-1",
|
||||
}));
|
||||
expect(existsSync(orphan)).toBe(false);
|
||||
expect(removed).toBe(0);
|
||||
expect(cleanupSecretsEnvFile).not.toHaveBeenCalled();
|
||||
expect(existsSync(orphan)).toBe(true);
|
||||
});
|
||||
|
||||
it("cleanup failures do not block orphan removal", async () => {
|
||||
it("does not invoke secrets cleanup before preserving dangling metadata", async () => {
|
||||
cleanupSecretsEnvFile.mockRejectedValueOnce(new Error("cleanup failed"));
|
||||
const root = tmpRoot();
|
||||
const orphan = join(root, ".worktrees", "orphan-2");
|
||||
@@ -65,7 +62,8 @@ describe("worktree-pool secrets cleanup hooks", () => {
|
||||
const mod = await import("../worktree/worktree-pool.js");
|
||||
const removed = await mod.reapOrphanWorktrees(root);
|
||||
|
||||
expect(removed).toBe(1);
|
||||
expect(existsSync(orphan)).toBe(false);
|
||||
expect(removed).toBe(0);
|
||||
expect(cleanupSecretsEnvFile).not.toHaveBeenCalled();
|
||||
expect(existsSync(orphan)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1140,7 +1140,7 @@ describe("cleanupOrphanedWorktrees", () => {
|
||||
expect(removeCalls).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("excludes internal containers while still removing genuine unregistered orphans", async () => {
|
||||
it("excludes internal containers and preserves unregistered orphans", async () => {
|
||||
mockedReaddirSync.mockReturnValue([
|
||||
makeDirEntry(".ai-merge"),
|
||||
makeDirEntry(".fusion-recovery"),
|
||||
@@ -1152,16 +1152,13 @@ describe("cleanupOrphanedWorktrees", () => {
|
||||
|
||||
const cleaned = await cleanupOrphanedWorktrees("/root", store);
|
||||
|
||||
expect(cleaned).toBe(1);
|
||||
expect(mockedRmSync).toHaveBeenCalledWith("/root/.worktrees/broken-wt", {
|
||||
recursive: true,
|
||||
force: true,
|
||||
});
|
||||
expect(cleaned).toBe(0);
|
||||
expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/broken-wt", expect.anything());
|
||||
expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/.ai-merge", expect.anything());
|
||||
expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/.fusion-recovery", expect.anything());
|
||||
});
|
||||
|
||||
it("removes unregistered directories even when stale active task metadata references them", async () => {
|
||||
it("preserves unregistered directories referenced by stale active task metadata", async () => {
|
||||
mockedReaddirSync.mockReturnValue([
|
||||
makeDirEntry("broken-wt"),
|
||||
] as any);
|
||||
@@ -1173,14 +1170,9 @@ describe("cleanupOrphanedWorktrees", () => {
|
||||
|
||||
const cleaned = await cleanupOrphanedWorktrees("/root", store);
|
||||
|
||||
expect(cleaned).toBe(1);
|
||||
expect(mockedRmSync).toHaveBeenCalledWith("/root/.worktrees/broken-wt", {
|
||||
recursive: true,
|
||||
force: true,
|
||||
});
|
||||
expect(mockedPruneWorktreeAdminEntries).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ reason: "pool-cleanup-orphan", target: "/root/.worktrees/broken-wt" }),
|
||||
);
|
||||
expect(cleaned).toBe(0);
|
||||
expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/broken-wt", expect.anything());
|
||||
expect(mockedPruneWorktreeAdminEntries).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1207,13 +1199,8 @@ describe("reapOrphanWorktrees", () => {
|
||||
expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/.fusion-recovery", expect.anything());
|
||||
});
|
||||
|
||||
// FN-6782 follow-up: a directory whose `.git` points to a missing admin entry is leak
|
||||
// residue (invisible to `git worktree list`/`prune`), not "partially registered". It
|
||||
// must be reaped — otherwise it collides with freshly generated worktree names and
|
||||
// breaks `execute`. Previously the reaper skipped on mere `.git` presence.
|
||||
it("reaps a dir with a dangling .git pointer (admin gitdir missing)", async () => {
|
||||
it("preserves a dir with a dangling .git pointer", async () => {
|
||||
mockedReaddirSync.mockReturnValue([makeDirEntry("leaked-wt")] as any);
|
||||
// `.git` is a link FILE (not a dir); the worktree dir itself is a dir.
|
||||
mockedLstatSync.mockImplementation((p: any) =>
|
||||
(String(p).endsWith("/.git")
|
||||
? { isDirectory: () => false, isSymbolicLink: () => false }
|
||||
@@ -1222,14 +1209,13 @@ describe("reapOrphanWorktrees", () => {
|
||||
mockedReadFileSync.mockReturnValue("gitdir: /root/.git/worktrees/leaked-wt\n" as any);
|
||||
mockedExistsSync.mockImplementation((p) => {
|
||||
const s = String(p);
|
||||
// .worktrees root exists; the .git link file exists; the gitdir target does NOT.
|
||||
return s === "/root/.worktrees" || s === "/root/.worktrees/leaked-wt/.git";
|
||||
});
|
||||
|
||||
const removed = await reapOrphanWorktrees("/root");
|
||||
|
||||
expect(removed).toBe(1);
|
||||
expect(mockedRmSync).toHaveBeenCalledWith("/root/.worktrees/leaked-wt", { recursive: true, force: true });
|
||||
expect(removed).toBe(0);
|
||||
expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/leaked-wt", expect.anything());
|
||||
});
|
||||
|
||||
it("skips a dir with a valid .git pointer (admin gitdir exists)", async () => {
|
||||
|
||||
@@ -26,7 +26,7 @@
|
||||
|
||||
import { execSync } from "node:child_process";
|
||||
import { setImmediate as setImmediateCb } from "node:timers";
|
||||
import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, rmSync, statSync, writeFileSync } from "node:fs";
|
||||
import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, rmdirSync, rmSync, statSync, writeFileSync } from "node:fs";
|
||||
import { readFile } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import { basename, dirname, isAbsolute, join, relative, resolve } from "node:path";
|
||||
@@ -16617,7 +16617,9 @@ const movedTask = await this.store.moveTask(task.id, completeLane);
|
||||
continue;
|
||||
}
|
||||
try {
|
||||
rmSync(path, { recursive: true, force: true });
|
||||
// FNXC:WorktreeCleanup: rmdir is deliberately non-recursive. Any content
|
||||
// makes it fail closed and preserves the unregistered checkout.
|
||||
rmdirSync(path);
|
||||
log.log(`Cleaned unregistered worktree dir: ${path}`);
|
||||
cleaned++;
|
||||
} catch (err: unknown) {
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { exec } from "node:child_process";
|
||||
import { exec, execFile } from "node:child_process";
|
||||
import { existsSync } from "node:fs";
|
||||
import { access, rm } from "node:fs/promises";
|
||||
import { basename, resolve } from "node:path";
|
||||
@@ -27,6 +27,7 @@ import {
|
||||
import { parseStaleRegistrationPath, recoverStaleRegistration } from "./worktree-stale-registration.js";
|
||||
|
||||
const execAsync = promisify(exec);
|
||||
const execFileAsync = promisify(execFile);
|
||||
const NATIVE_TIMEOUT_MS = 120_000;
|
||||
const REMOVE_TIMEOUT_MS = 60_000;
|
||||
const MAX_BUFFER = 10 * 1024 * 1024;
|
||||
@@ -216,6 +217,7 @@ export interface WorktreeRemoveInput {
|
||||
worktreePath: string;
|
||||
branch?: string;
|
||||
taskId?: string;
|
||||
force?: boolean;
|
||||
}
|
||||
|
||||
export interface WorktreeSyncInput {
|
||||
@@ -679,7 +681,7 @@ export class NativeWorktreeBackend implements WorktreeBackend {
|
||||
|
||||
async remove(input: WorktreeRemoveInput): Promise<void> {
|
||||
try {
|
||||
await execAsync(`git worktree remove --force ${quoteShellArg(input.worktreePath)}`, {
|
||||
await execAsync(`git worktree remove${input.force === false ? "" : " --force"} ${quoteShellArg(input.worktreePath)}`, {
|
||||
cwd: input.rootDir,
|
||||
encoding: "utf-8",
|
||||
timeout: REMOVE_TIMEOUT_MS,
|
||||
@@ -687,6 +689,22 @@ export class NativeWorktreeBackend implements WorktreeBackend {
|
||||
});
|
||||
return;
|
||||
} catch (error) {
|
||||
// Defensive callers rely on Git's deletion-boundary dirty check. Never turn
|
||||
// that refusal into the recursive filesystem fallback below.
|
||||
if (input.force === false) {
|
||||
const missingPathError = /is not a working tree|no such file or directory|does not exist/i.test(getErrorMessageWithStderr(error));
|
||||
if (!existsSync(input.worktreePath) && missingPathError) {
|
||||
await pruneWorktreeAdminEntries({
|
||||
rootDir: input.rootDir,
|
||||
auditor: this.deps.audit,
|
||||
reason: "remove-missing-fallback",
|
||||
target: input.worktreePath,
|
||||
logger: this.deps.logger,
|
||||
});
|
||||
return;
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
if (!isRecoverableNativeWorktreeRemoveError(error)) {
|
||||
throw error;
|
||||
}
|
||||
@@ -951,7 +969,7 @@ export class WorktrunkWorktreeBackend implements WorktreeBackend {
|
||||
async remove(input: WorktreeRemoveInput): Promise<void> {
|
||||
const target = input.branch ?? input.worktreePath;
|
||||
try {
|
||||
await this.runWorktrunk(["remove", "--foreground", target], {
|
||||
await this.runWorktrunk(["remove", "--foreground", ...(input.force === true ? ["--force"] : []), target], {
|
||||
cwd: input.rootDir,
|
||||
operation: "remove",
|
||||
});
|
||||
@@ -1072,6 +1090,17 @@ const ALLOWED_FORCE_REASONS = new Set<RemovalReason>([
|
||||
RemovalReason.WorkspaceAcquireRollback,
|
||||
]);
|
||||
|
||||
const DEFENSIVE_REMOVAL_REASONS = new Set<RemovalReason>([
|
||||
RemovalReason.MergerCleanup,
|
||||
RemovalReason.MergerPostMerge,
|
||||
RemovalReason.PoolPrune,
|
||||
RemovalReason.SelfHealingBranchConflict,
|
||||
RemovalReason.SelfHealingIdleSweep,
|
||||
RemovalReason.SelfHealingReclaim,
|
||||
RemovalReason.SelfHealingStaleActiveBranch,
|
||||
RemovalReason.StepSessionCleanup,
|
||||
]);
|
||||
|
||||
export class InvalidForceUsageError extends Error {
|
||||
constructor(reason: RemovalReason) {
|
||||
super(`force=true is not allowed for removal reason '${reason}'`);
|
||||
@@ -1092,6 +1121,28 @@ export class ActiveSessionWorktreeRemovalError extends Error {
|
||||
}
|
||||
}
|
||||
|
||||
/** Fail closed when an automatic sweep cannot prove the checkout is empty of user content. */
|
||||
async function assertCleanForDefensiveRemoval(worktreePath: string): Promise<void> {
|
||||
// Nothing on disk means nothing to preserve — stale registrations prune normally below.
|
||||
if (!existsSync(worktreePath)) {
|
||||
return;
|
||||
}
|
||||
let stdout: string;
|
||||
try {
|
||||
({ stdout } = await execFileAsync("git", ["status", "--porcelain", "--ignored", "--untracked-files=all"], {
|
||||
cwd: worktreePath,
|
||||
encoding: "utf-8",
|
||||
timeout: 15_000,
|
||||
maxBuffer: MAX_BUFFER,
|
||||
}));
|
||||
} catch (error) {
|
||||
throw new Error(`preserving ${worktreePath}: status probe failed (${error instanceof Error ? error.message : String(error)})`);
|
||||
}
|
||||
if (stdout.trim().length > 0) {
|
||||
throw new Error(`preserving ${worktreePath}: uncommitted or ignored content present`);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* FNXC:WorkspaceWorktree 2026-08-20-07:08:
|
||||
* Force removal is reserved for explicit executor teardown paths and workspace-acquisition rollback
|
||||
@@ -1120,6 +1171,15 @@ export async function removeWorktree(input: {
|
||||
throw new InvalidForceUsageError(input.reason);
|
||||
}
|
||||
|
||||
const requiresCleanWorktree = DEFENSIVE_REMOVAL_REASONS.has(input.reason);
|
||||
|
||||
// FNXC:WorktreeCleanup:
|
||||
// Defensive sweeps must prove cleanliness before destroying anything. Dirty or
|
||||
// unverifiable content is preserved (fail closed) — callers treat the throw as "kept".
|
||||
if (requiresCleanWorktree) {
|
||||
await assertCleanForDefensiveRemoval(input.worktreePath);
|
||||
}
|
||||
|
||||
if (input.expectedOwnerTaskId && input.liveOwnerProbe) {
|
||||
const reconciled = reconcileSelfOwnedActiveSessionForRemoval(
|
||||
activeSessionRegistry,
|
||||
@@ -1169,6 +1229,7 @@ export async function removeWorktree(input: {
|
||||
rootDir: input.rootDir,
|
||||
worktreePath: input.worktreePath,
|
||||
taskId: input.taskId,
|
||||
force: requiresCleanWorktree ? false : input.force,
|
||||
};
|
||||
|
||||
if (input.force === false || typeof input.timeout === "number") {
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { exec, execFile } from "node:child_process";
|
||||
import { promisify } from "node:util";
|
||||
import { existsSync, lstatSync, readdirSync, readFileSync, rmSync, realpathSync } from "node:fs";
|
||||
import { existsSync, lstatSync, readdirSync, readFileSync, rmdirSync, realpathSync } from "node:fs";
|
||||
import { mkdir } from "node:fs/promises";
|
||||
import { basename, dirname, join, relative, resolve, isAbsolute } from "node:path";
|
||||
import type { SecretsStore, Settings, TaskStore, WorktrunkSettings, WorkspaceWorktreeContext } from "@fusion/core";
|
||||
@@ -18,7 +18,6 @@ import {
|
||||
removeWorktree as removeWorktreeViaBackend,
|
||||
resolveWorktreeBackend as resolveWorktreeBackendViaSettings,
|
||||
} from "./worktree-backend.js";
|
||||
import { cleanupSecretsEnvFile } from "./secrets-env-writer.js";
|
||||
import { removeDesktopBuildArtifacts } from "./worktree-desktop-artifacts.js";
|
||||
import { resolveIntegrationBranch } from "../merge/integration-branch.js";
|
||||
import type { RunAuditor } from "../util/run-audit.js";
|
||||
@@ -1026,22 +1025,6 @@ export async function cleanupOrphanedWorktrees(
|
||||
for (const worktreePath of candidates) {
|
||||
try {
|
||||
if (registeredWorktrees.has(resolve(worktreePath))) {
|
||||
const orphanTaskId = `orphan:${basename(worktreePath)}`;
|
||||
try {
|
||||
await cleanupSecretsEnvFile({
|
||||
worktreePath,
|
||||
taskId: orphanTaskId,
|
||||
expectedFingerprint: null,
|
||||
filename: ".env",
|
||||
audit: undefined,
|
||||
logger: worktreePoolLog,
|
||||
});
|
||||
} catch (error) {
|
||||
worktreePoolLog.warn(
|
||||
`secrets-env cleanup failed for registered orphan ${worktreePath}: ${error instanceof Error ? error.message : String(error)}`,
|
||||
);
|
||||
}
|
||||
|
||||
await removeWorktreeViaBackend({
|
||||
rootDir,
|
||||
worktreePath,
|
||||
@@ -1052,7 +1035,9 @@ export async function cleanupOrphanedWorktrees(
|
||||
if (!isInsideWorktreesDir(rootDir, worktreePath, settings)) {
|
||||
throw new Error(`Refusing to remove path outside .worktrees: ${worktreePath}`);
|
||||
}
|
||||
rmSync(worktreePath, { recursive: true, force: true });
|
||||
// FNXC:WorktreeCleanup: rmdir is deliberately non-recursive. Any content
|
||||
// makes it fail closed and preserves the unregistered checkout.
|
||||
rmdirSync(worktreePath);
|
||||
await pruneWorktreeAdminEntries({
|
||||
rootDir,
|
||||
reason: "pool-cleanup-orphan",
|
||||
@@ -1199,36 +1184,26 @@ export async function reapOrphanWorktrees(
|
||||
continue;
|
||||
}
|
||||
worktreePoolLog.debug(`reapOrphanWorktrees: ${name} has a dangling .git pointer (admin entry missing) — treating as orphan`);
|
||||
// fall through to removal
|
||||
// fall through to the non-recursive removal below; `.git` makes it fail closed.
|
||||
}
|
||||
|
||||
// This directory is on disk but has no valid .git entry and is not a registered
|
||||
// worktree — it is a half-initialized / leaked orphan. Remove it.
|
||||
try {
|
||||
try {
|
||||
await cleanupSecretsEnvFile({
|
||||
worktreePath: resolvedFull,
|
||||
taskId: `orphan:${name}`,
|
||||
expectedFingerprint: null,
|
||||
filename: ".env",
|
||||
logger: worktreePoolLog,
|
||||
});
|
||||
} catch (error) {
|
||||
worktreePoolLog.warn(`secrets-env cleanup failed for orphan ${name}: ${error instanceof Error ? error.message : String(error)}`);
|
||||
}
|
||||
rmSync(resolvedFull, { recursive: true, force: true });
|
||||
await pruneWorktreeAdminEntries({
|
||||
rootDir: projectRoot,
|
||||
reason: "pool-reap-orphan",
|
||||
target: resolvedFull,
|
||||
logger: worktreePoolLog,
|
||||
}).catch(() => undefined);
|
||||
worktreePoolLog.log(`reapOrphanWorktrees: removed half-initialized orphan ${name}`);
|
||||
removed++;
|
||||
rmdirSync(resolvedFull);
|
||||
} catch (err: unknown) {
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
worktreePoolLog.warn(`reapOrphanWorktrees: failed to remove ${name} — ${msg}`);
|
||||
continue;
|
||||
}
|
||||
await pruneWorktreeAdminEntries({
|
||||
rootDir: projectRoot,
|
||||
reason: "pool-reap-orphan",
|
||||
target: resolvedFull,
|
||||
logger: worktreePoolLog,
|
||||
}).catch(() => undefined);
|
||||
worktreePoolLog.log(`reapOrphanWorktrees: removed half-initialized orphan ${name}`);
|
||||
removed++;
|
||||
}
|
||||
|
||||
return removed;
|
||||
|
||||
Reference in New Issue
Block a user