diff --git a/.changeset/fix-orphan-worktree-dir-cleanup.md b/.changeset/fix-orphan-worktree-dir-cleanup.md new file mode 100644 index 0000000000..61306949dd --- /dev/null +++ b/.changeset/fix-orphan-worktree-dir-cleanup.md @@ -0,0 +1,10 @@ +--- +"@runfusion/fusion": patch +--- + +Fix worktree-creation failures (and the `Workflow graph terminated with failure at node 'execute'` they surface as) caused by leaked orphan worktree directories. + +A directory under `.worktrees/` that survives with a *dangling* `.git` pointer — present on disk, but the `.git/worktrees/` admin entry it references is gone — is invisible to `git worktree list` and untouched by `git worktree prune`, yet collides with a freshly generated worktree name. When the executor then tries to clean up the "conflict", `git worktree remove --force` fails with `is not a working tree` and the whole `execute` node fails after 3 attempts. + +- **On-demand recovery (`executor.ts`):** the FN-4813 stale-conflict recovery now also treats `is not a working tree` and `ENOENT` (not just `validation failed, cannot remove working tree`) as "no live worktree at this path" — it prunes any admin entry, force-removes the leftover directory, and proceeds with fresh worktree creation instead of failing. +- **Leak prevention (`worktree-pool.ts`):** `reapOrphanWorktrees` previously skipped any dir on the mere *presence* of a `.git` file ("may be partially registered"), contradicting its own documented invariant. It now resolves the `.git` pointer and only skips when the gitdir target actually exists; a dangling pointer is reaped like any other half-initialized orphan, so these directories no longer accumulate across runs. diff --git a/packages/engine/src/__tests__/worktree-pool.test.ts b/packages/engine/src/__tests__/worktree-pool.test.ts index aae3bc59ba..d6e27a405a 100644 --- a/packages/engine/src/__tests__/worktree-pool.test.ts +++ b/packages/engine/src/__tests__/worktree-pool.test.ts @@ -52,6 +52,7 @@ vi.mock("node:fs", () => ({ existsSync: vi.fn().mockReturnValue(true), lstatSync: vi.fn().mockReturnValue({ isDirectory: () => true, isSymbolicLink: () => false }), readdirSync: vi.fn().mockReturnValue([]), + readFileSync: vi.fn().mockReturnValue(""), rmSync: vi.fn(), })); @@ -73,13 +74,14 @@ import { import { BranchConflictError } from "../branch-conflicts.js"; import * as branchConflictModule from "../branch-conflicts.js"; import { execSync } from "node:child_process"; -import { existsSync, lstatSync, readdirSync, rmSync } from "node:fs"; +import { existsSync, lstatSync, readdirSync, readFileSync, rmSync } from "node:fs"; import type { Task, Column } from "@fusion/core"; const mockedExecSync = vi.mocked(execSync); const mockedExistsSync = vi.mocked(existsSync); const mockedLstatSync = vi.mocked(lstatSync); const mockedReaddirSync = vi.mocked(readdirSync); +const mockedReadFileSync = vi.mocked(readFileSync); const mockedRmSync = vi.mocked(rmSync); const mockedPruneWorktreeAdminEntries = vi.mocked(worktreePrune.pruneWorktreeAdminEntries); const TEST_TASK_ID = "FN-test"; @@ -1109,5 +1111,50 @@ describe("reapOrphanWorktrees", () => { expect(mockedRmSync).toHaveBeenCalledWith("/root/.worktrees/half-built", { recursive: true, force: true }); expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/.ai-merge", 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 () => { + 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 } + : { isDirectory: () => true, isSymbolicLink: () => false }) as any, + ); + 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 }); + }); + + it("skips a dir with a valid .git pointer (admin gitdir exists)", async () => { + mockedReaddirSync.mockReturnValue([makeDirEntry("live-wt")] as any); + mockedLstatSync.mockImplementation((p: any) => + (String(p).endsWith("/.git") + ? { isDirectory: () => false, isSymbolicLink: () => false } + : { isDirectory: () => true, isSymbolicLink: () => false }) as any, + ); + mockedReadFileSync.mockReturnValue("gitdir: /root/.git/worktrees/live-wt\n" as any); + mockedExistsSync.mockImplementation((p) => { + const s = String(p); + // The gitdir target exists too → treat as (maybe) registered, leave it alone. + return s === "/root/.worktrees" || s === "/root/.worktrees/live-wt/.git" || s === "/root/.git/worktrees/live-wt"; + }); + + const removed = await reapOrphanWorktrees("/root"); + + expect(removed).toBe(0); + expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/live-wt", expect.anything()); + }); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 4fd29ef683..e3e4c8d53b 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -14247,14 +14247,24 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit return true; } catch (error: unknown) { const errorMessage = error instanceof Error ? error.message : String(error); - // FN-4811 follow-up (FN-4813): when `git worktree remove --force` fails with - // "fatal: validation failed, cannot remove working tree", the worktree directory - // doesn't exist on disk and the git admin entry (if any) is stale. Treat as - // already-cleaned: prune the stale admin entry, best-effort delete the branch, and - // return success so the caller can proceed with fresh worktree creation. Without - // this recovery, every `tryCreateWorktree` retry on a stale conflict path fails - // with "automatic cleanup failed". - if (/validation failed, cannot remove working tree/i.test(errorMessage)) { + // FN-4811 follow-up (FN-4813): when `git worktree remove --force` fails because the + // conflicting path isn't a recoverable git worktree, treat it as already-cleaned: + // prune any stale admin entry, force-remove the leftover directory, best-effort delete + // the branch, and return success so the caller can proceed with fresh worktree creation. + // Without this recovery, every `tryCreateWorktree` retry on such a path fails with + // "automatic cleanup failed". + // + // Three variants land here, all meaning "no live worktree to preserve at this path": + // 1. `validation failed, cannot remove working tree` — stale admin entry, dir missing. + // 2. `is not a working tree` — an orphan directory exists on disk but git never + // registered it (e.g. a leaked worktree dir that outlived its admin entry). This + // is the FN-6782 leak residue that collides with freshly generated worktree names. + // 3. `No such file or directory` / ENOENT — the path is already gone. + const staleConflictPath = + /validation failed, cannot remove working tree/i.test(errorMessage) || + /is not a working tree/i.test(errorMessage) || + /no such file or directory|ENOENT/i.test(errorMessage); + if (staleConflictPath) { try { await execAsync("git worktree prune", { cwd: this.rootDir, @@ -14265,6 +14275,14 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit const pruneMsg = pruneErr instanceof Error ? pruneErr.message : String(pruneErr); executorLog.warn(`${taskId}: git worktree prune failed during stale-path cleanup of ${worktreePath}: ${pruneMsg}`); } + // An orphan directory ("is not a working tree") won't be removed by prune — git + // doesn't track it. Force-remove the leftover dir so the colliding name is free. + try { + await rm(worktreePath, { recursive: true, force: true }); + } catch (rmErr: unknown) { + const rmMsg = rmErr instanceof Error ? rmErr.message : String(rmErr); + executorLog.warn(`${taskId}: failed to remove orphan worktree directory ${worktreePath}: ${rmMsg}`); + } try { await execAsync(`git branch -D "${branch}"`, { cwd: this.rootDir }); this.store.clearStaleExecutionStartBranchReferences([branch], taskId); @@ -14273,7 +14291,7 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit } await this.store.logEntry( taskId, - `Cleaned up stale conflicting worktree admin entry (validation failed — path likely missing on disk)`, + `Cleaned up stale conflicting worktree (no live worktree at path — pruned admin entry and removed orphan directory)`, worktreePath, ); return true; diff --git a/packages/engine/src/worktree-pool.ts b/packages/engine/src/worktree-pool.ts index 58ed42fb9f..a7f4135837 100644 --- a/packages/engine/src/worktree-pool.ts +++ b/packages/engine/src/worktree-pool.ts @@ -1,7 +1,7 @@ import { exec } from "node:child_process"; import { promisify } from "node:util"; -import { existsSync, lstatSync, readdirSync, rmSync, realpathSync } from "node:fs"; -import { basename, join, relative, resolve, isAbsolute } from "node:path"; +import { existsSync, lstatSync, readdirSync, readFileSync, rmSync, realpathSync } from "node:fs"; +import { basename, dirname, join, relative, resolve, isAbsolute } from "node:path"; import type { ColumnId, SecretsStore, Settings, TaskStore, WorktrunkSettings } from "@fusion/core"; import { assertCleanBranchAtBase, inspectBranchConflict } from "./branch-conflicts.js"; import { worktreePoolLog } from "./logger.js"; @@ -848,6 +848,34 @@ export async function cleanupOrphanedWorktrees( * @param projectRoot - Absolute path to the project root (parent of `.worktrees/`) * @returns Number of orphan directories removed */ +/** + * Resolve a worktree's `.git` pointer to the gitdir admin path it references. + * + * Returns: + * - `"directory"` if `.git` is a real directory (a normal repo, not a worktree + * link) — callers should treat that as "leave it alone". + * - an absolute path string for a `gitdir: ` link file (relative targets + * are resolved against the worktree directory). + * - `null` if the pointer can't be read or parsed. + * + * Callers decide whether the target exists; a missing target means the link is + * dangling (leak residue) and the directory is safe to reap. + */ +function resolveGitdirPointer(dotGitPath: string): string | "directory" | null { + try { + if (lstatSync(dotGitPath).isDirectory()) { + return "directory"; + } + const raw = readFileSync(dotGitPath, "utf8").trim(); + const match = /^gitdir:\s*(.+)$/.exec(raw); + if (!match) return null; + const target = match[1].trim(); + return isAbsolute(target) ? target : resolve(dirname(dotGitPath), target); + } catch { + return null; + } +} + export async function reapOrphanWorktrees( projectRoot: string, settings?: Pick, @@ -902,16 +930,29 @@ export async function reapOrphanWorktrees( // Belt-and-suspenders: skip if a .git file exists AND points to an existing gitdir. // This guards against races where git registered the worktree between our list // call and now, or against a broken repo whose porcelain is unreliable. + // + // FN-6782 follow-up: a *dangling* `.git` (file present, but the admin entry it + // points to is gone) is NOT "partially registered" — it is leak residue from a + // worktree whose admin entry was pruned while the directory survived. Such a dir + // is invisible to `git worktree list`/`prune` yet collides with freshly generated + // worktree names and breaks `execute` (cleanup can't `git worktree remove` a path + // git never registered). Only skip when the gitdir target actually exists; reap + // dangling pointers like any other half-initialized orphan. const dotGit = join(resolvedFull, ".git"); if (existsSync(dotGit)) { - // If there's a .git file/dir, don't touch it — assertValidWorktreeSession - // will handle it on the next agent start. - worktreePoolLog.log(`reapOrphanWorktrees: skipping ${name} (has .git entry but not in registered list — may be partially registered)`); - continue; + const gitdirTarget = resolveGitdirPointer(dotGit); + if (gitdirTarget === "directory" || (gitdirTarget && existsSync(gitdirTarget))) { + // Valid registration (or a real .git dir) — leave it; assertValidWorktreeSession + // will handle it on the next agent start. + worktreePoolLog.log(`reapOrphanWorktrees: skipping ${name} (has .git entry but not in registered list — may be partially registered)`); + continue; + } + worktreePoolLog.log(`reapOrphanWorktrees: ${name} has a dangling .git pointer (admin entry missing) — treating as orphan`); + // fall through to removal } - // This directory is on disk but has no .git entry and is not a registered - // worktree — it is a half-initialized orphan. Remove it. + // 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({