Harden orphan-worktree/stale-task-dir cleanup (code-review follow-up)

Addresses findings from a multi-agent review of the two prior fixes.

P0 (executor.ts): the stale-conflict recovery force-removed worktreePath with
no bounds check; that path can come from a git admin entry resolving outside
.worktrees/. Now refuses unless the path is inside the worktrees dir, not a
symlink (realpathSync), not a registered worktree, and not actively owned, and
re-verifies liveness in the catch instead of trusting the error string. Also
excludes spawn failures (spawn git ENOENT) from the stale-path classification.

worktree-pool.ts: resolveGitdirPointer -> dotGitPointerIsDangling. Reaps only
when a .git link's gitdir target is confirmed missing; a real .git dir,
unparseable pointer, or any read/stat failure is treated as NOT dangling
(conservative) so a transient read error on a live worktree can't trigger rm.
Drops the string|"directory"|null sentinel union.

core store.ts: bypass the reconcile recency window when the live task table is
empty (corruption/restore: surviving task.json keep old mtimes) and when
fusion.db was auto-recovered on startup, so .recover row loss isn't stranded.
Adds an ignoreRecencyWindow option.

Tests: executor recovery + out-of-bounds refusal, unparseable .git skip,
recency boundary, empty-DB/forced bypass. engine 135 + core 12 green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-06-20 12:46:34 -07:00
parent 26ebb9206f
commit 7e7eb6299d
8 changed files with 244 additions and 47 deletions

View File

@@ -0,0 +1,11 @@
---
"@runfusion/fusion": patch
"@fusion/core": patch
---
Harden the orphan-worktree and stale-task-dir cleanup fixes (code-review follow-up).
- **executor.ts (P0):** the stale-conflict recovery's `rm(worktreePath, { recursive, force })` had no bounds check. `worktreePath` can originate from a git worktree admin entry that resolves outside `.worktrees/`, so an out-of-bounds or symlinked path could be force-removed. The recovery now refuses unless the path is inside the configured worktrees dir, is not a symlink (checked via `realpathSync`), is not a registered git worktree, and is not actively owned — and it re-verifies liveness inside the catch rather than trusting the error string. It also excludes `spawn` failures (e.g. `spawn git ENOENT` when git is missing) so a missing-binary error is no longer misread as a successful stale-path cleanup.
- **worktree-pool.ts:** `resolveGitdirPointer` is replaced by `dotGitPointerIsDangling`, which reaps **only** when a `.git` link's gitdir target is confirmed missing. A real `.git` directory, an unparseable pointer, or any read/stat failure now returns "not dangling" (conservative) so a transient read error on a live worktree's `.git` can't cause a force-remove. Removes the `string | "directory" | null` sentinel union.
- **core store.ts:** the `reconcileOrphanedTaskDirs` recency window is now bypassed when the live task table is empty (the corruption/restore case — surviving `task.json` files keep old mtimes), and when a corrupt `fusion.db` was auto-recovered on startup, so `.recover` row loss is not stranded by the gate. Adds an `ignoreRecencyWindow` option for explicit callers.
- Tests for all of the above: executor recovery + out-of-bounds refusal, unparseable `.git` skip, recency boundary, empty-DB/forced bypass.

View File

@@ -124,6 +124,12 @@ describe("TaskStore orphaned task-dir reconciliation", () => {
it("skips a stale orphan task dir beyond the recency window (no resurrection of old deleted tasks)", async () => {
// Regression: legacy hard-deletes left no tombstone, so an ancient task.json lingering
// on disk was silently re-imported onto the live board ("all task IDs reset" failure).
// A live task must exist so the recency window applies (an empty DB bypasses it — see
// the corruption-recovery tests below).
await store.createTaskWithReservedId(
{ description: "Keeps the board non-empty" },
{ taskId: "FN-9200", applyDefaultWorkflowSteps: false, invokeTaskCreatedHook: false },
);
const orphan = await createDiskOnlyTask("FN-9110");
// Backdate the task.json well beyond the 7-day recency window.
const eightDaysAgo = new Date(Date.now() - 8 * 24 * 60 * 60 * 1000);
@@ -139,6 +145,51 @@ describe("TaskStore orphaned task-dir reconciliation", () => {
await expect(store.getTask(orphan.id)).rejects.toThrow("Task FN-9110 not found");
});
it("recovers a stale orphan dir just inside the recency window (boundary)", async () => {
await store.createTaskWithReservedId(
{ description: "Keeps the board non-empty" },
{ taskId: "FN-9201", applyDefaultWorkflowSteps: false, invokeTaskCreatedHook: false },
);
const orphan = await createDiskOnlyTask("FN-9111");
// ~6 days old — comfortably inside the 7-day window.
const sixDaysAgo = new Date(Date.now() - 6 * 24 * 60 * 60 * 1000);
const taskJsonPath = join(rootDir, ".fusion", "tasks", orphan.id, "task.json");
await utimes(taskJsonPath, sixDaysAgo, sixDaysAgo);
const result = await store.reconcileOrphanedTaskDirs();
expect(result.recovered).toContain(orphan.id);
});
it("bypasses the recency window when the live task table is empty (corruption / restore recovery)", async () => {
// Restore-from-old-backup: surviving task.json files keep their original (old) mtimes and
// the DB has no live rows. The recency gate must NOT strand them — that is the exact
// recovery the sweep exists for.
const orphan = await createDiskOnlyTask("FN-9112");
const thirtyDaysAgo = new Date(Date.now() - 30 * 24 * 60 * 60 * 1000);
const taskJsonPath = join(rootDir, ".fusion", "tasks", orphan.id, "task.json");
await utimes(taskJsonPath, thirtyDaysAgo, thirtyDaysAgo);
const result = await store.reconcileOrphanedTaskDirs();
expect(result.recovered).toContain(orphan.id);
});
it("bypasses the recency window when the caller forces it (ignoreRecencyWindow)", async () => {
await store.createTaskWithReservedId(
{ description: "Keeps the board non-empty" },
{ taskId: "FN-9202", applyDefaultWorkflowSteps: false, invokeTaskCreatedHook: false },
);
const orphan = await createDiskOnlyTask("FN-9113");
const thirtyDaysAgo = new Date(Date.now() - 30 * 24 * 60 * 60 * 1000);
const taskJsonPath = join(rootDir, ".fusion", "tasks", orphan.id, "task.json");
await utimes(taskJsonPath, thirtyDaysAgo, thirtyDaysAgo);
const result = await store.reconcileOrphanedTaskDirs({ ignoreRecencyWindow: true });
expect(result.recovered).toContain(orphan.id);
});
it("skips malformed task.json and directories without task.json without throwing", async () => {
const malformedDir = join(rootDir, ".fusion", "tasks", "FN-9106");
await mkdir(malformedDir, { recursive: true });

View File

@@ -1556,6 +1556,8 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
private configLock: Promise<void> = Promise.resolve();
/** Startup/open guard for distributed_task_id_state reconciliation. */
private taskIdStateReconciled = false;
/** Set when startup auto-recovery rebuilt a corrupt fusion.db; lets the orphan reconcile bypass its recency window so rows dropped by `.recover` are recovered even with old task.json mtimes. */
private dbWasCorruptionRecovered = false;
/** Cached startup/refresh integrity report for allocator-related task ID anomalies. */
private taskIdIntegrityReport: TaskIdIntegrityReport = {
status: "ok",
@@ -1817,6 +1819,10 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
try {
const recovery = Database.recoverIfCorrupt(this.fusionDir);
if (recovery.status === "recovered") {
// A `.recover` rebuild can drop task rows whose task.json survived on disk. Let the
// orphan reconcile below bypass its recency window so those rows are recovered even
// when their (possibly old) task.json mtime would otherwise fail the gate.
this.dbWasCorruptionRecovered = true;
storeLog.warn("Recovered corrupt fusion.db on startup", {
phase: "init:db-autorecover",
corruptBackupPath: recovery.corruptBackupPath,
@@ -1890,7 +1896,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
this.taskIdStateReconciled = false;
this.reconcileDistributedTaskIdStateOnOpen();
try {
await this.reconcileOrphanedTaskDirs();
await this.reconcileOrphanedTaskDirs({ ignoreRecencyWindow: this.dbWasCorruptionRecovered });
} catch (err) {
storeLog.warn("Orphaned task-dir reconcile failed during init (non-fatal)", {
phase: "init:orphaned-task-dir-reconcile",
@@ -3238,7 +3244,9 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS}
* FNXC:TaskStoreConsistency 2026-06-20-00:00:
* Heartbeat-created tasks persisted on disk but missing from the SQLite index were invisible to fn_task_list/fn_task_show (FN-6783/FN-6784). Reconcile re-imports orphaned task.json rows non-destructively and uses the same exists-anywhere guard as create-time ID allocation so soft-deleted, archived, and tombstoned IDs are never resurrected.
*/
async reconcileOrphanedTaskDirs(): Promise<{ recovered: string[]; skipped: Array<{ id: string; reason: string }> }> {
async reconcileOrphanedTaskDirs(
opts: { ignoreRecencyWindow?: boolean } = {},
): Promise<{ recovered: string[]; skipped: Array<{ id: string; reason: string }> }> {
const result: { recovered: string[]; skipped: Array<{ id: string; reason: string }> } = {
recovered: [],
skipped: [],
@@ -3248,6 +3256,24 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS}
return result;
}
// The recency window stops legacy hard-deleted dirs (no tombstone) from being silently
// resurrected onto a populated board. But the sweep's other job is recovering rows lost to
// DB corruption or a restore-from-old-backup — where the surviving task.json files keep
// their original (often >7-day-old) mtimes and the DB is empty. Detect that case: when the
// live task table is empty, bypass the recency gate so corruption recovery isn't defeated by
// the same guard added to stop resurrection. Callers may also force the bypass explicitly.
let dbHasLiveTasks = true;
try {
const row = this.db
.prepare('SELECT EXISTS(SELECT 1 FROM tasks WHERE deletedAt IS NULL LIMIT 1) AS present')
.get() as { present?: number } | undefined;
dbHasLiveTasks = (row?.present ?? 0) === 1;
} catch {
// If the count probe fails, keep the gate on (conservative — don't mass-resurrect).
dbHasLiveTasks = true;
}
const applyRecencyWindow = !opts.ignoreRecencyWindow && dbHasLiveTasks;
let entries: Dirent[];
try {
entries = await readdir(this.tasksDir, { withFileTypes: true });
@@ -3279,24 +3305,27 @@ ${TASK_UPSERT_SQL_ASSIGNMENTS}
// DB row would otherwise be silently re-imported onto the live board (the
// "all task IDs reset / starting over" failure). Only reconcile dirs whose
// task.json was modified within the recency window; older orphans are left for
// explicit recovery (unarchive/restore) or directory cleanup.
try {
const { mtimeMs } = await stat(taskJsonPath);
const ageMs = Date.now() - mtimeMs;
if (ageMs > RECONCILE_ORPHAN_TASK_DIR_MAX_AGE_MS) {
result.skipped.push({ id, reason: "stale-orphan-dir-beyond-recency-window" });
storeLog.warn("Skipping stale orphaned task-dir reconcile (beyond recency window)", {
phase: "reconcileOrphanedTaskDirs:recency",
taskId: id,
taskJsonPath,
ageMs,
maxAgeMs: RECONCILE_ORPHAN_TASK_DIR_MAX_AGE_MS,
});
// explicit recovery (unarchive/restore) or directory cleanup. Skipped entirely when
// the DB is empty / a caller forces recovery (corruption/restore path — see above).
if (applyRecencyWindow) {
try {
const { mtimeMs } = await stat(taskJsonPath);
const ageMs = Date.now() - mtimeMs;
if (ageMs > RECONCILE_ORPHAN_TASK_DIR_MAX_AGE_MS) {
result.skipped.push({ id, reason: "stale-orphan-dir-beyond-recency-window" });
storeLog.warn("Skipping stale orphaned task-dir reconcile (beyond recency window)", {
phase: "reconcileOrphanedTaskDirs:recency",
taskId: id,
taskJsonPath,
ageMs,
maxAgeMs: RECONCILE_ORPHAN_TASK_DIR_MAX_AGE_MS,
});
continue;
}
} catch (error) {
result.skipped.push({ id, reason: `stat-failed: ${error instanceof Error ? error.message : String(error)}` });
continue;
}
} catch (error) {
result.skipped.push({ id, reason: `stat-failed: ${error instanceof Error ? error.message : String(error)}` });
continue;
}
let task: Task;

View File

@@ -219,6 +219,7 @@ vi.mock("node:child_process", async () => {
vi.mock("node:fs", () => ({
existsSync: vi.fn().mockReturnValue(true),
realpathSync: vi.fn((path: string) => path),
lstatSync: vi.fn(() => ({ isSymbolicLink: () => false, isDirectory: () => true })),
}));
export const mockExecuteAll: Mock<() => Promise<unknown[]>> = vi.fn().mockResolvedValue([]);

View File

@@ -78,6 +78,50 @@ describe("FN-4973: executor worktree conflict cleanup", () => {
expect(activeSessionRegistry.lookupByPath(CONFLICT_PATH)?.taskId).toBe("FN-OTHER");
});
it("recovers a genuine orphan dir when git reports 'is not a working tree'", async () => {
// FN-6782: a leaked orphan dir (dir on disk, admin entry gone) makes `git worktree remove`
// fail with "is not a working tree". The stale-path recovery should prune, clean up, and
// return true so fresh creation can proceed.
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([]);
vi.spyOn(worktreePoolModule, "removeWorktree").mockRejectedValue(
new Error("fatal: '/tmp/test/.worktrees/stale-self-owned' is not a working tree"),
);
const result = await (executor as any).cleanupConflictingWorktree(CONFLICT_PATH, "fusion/fn-4973", "FN-4973");
expect(result).toBe(true);
expect(store.logEntry).toHaveBeenCalledWith(
"FN-4973",
expect.stringContaining("Cleaned up stale conflicting worktree"),
CONFLICT_PATH,
);
});
it("refuses stale-path cleanup (no force-rm) for a conflict path outside .worktrees/", async () => {
// Security regression: the recovery's rm must be bounded to .worktrees/. A git admin entry
// can point anywhere; an out-of-bounds path must be refused, not force-removed.
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([]);
const OUTSIDE_PATH = "/tmp/test/not-worktrees/escapee";
vi.spyOn(worktreePoolModule, "removeWorktree").mockRejectedValue(
new Error("fatal: '/tmp/test/not-worktrees/escapee' is not a working tree"),
);
const result = await (executor as any).cleanupConflictingWorktree(OUTSIDE_PATH, "fusion/fn-4973", "FN-4973");
expect(result).toBe(false);
expect(store.logEntry).toHaveBeenCalledWith(
"FN-4973",
expect.stringContaining("Refused stale-path cleanup"),
OUTSIDE_PATH,
);
});
it("reconciles once on race-window ActiveSessionWorktreeRemovalError then retries removal", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");

View File

@@ -1156,5 +1156,26 @@ describe("reapOrphanWorktrees", () => {
expect(removed).toBe(0);
expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/live-wt", expect.anything());
});
it("does NOT reap a dir whose .git is unparseable (conservative — only confirmed-dangling pointers)", async () => {
// A transient read error or a garbage .git (no `gitdir:` line) must not be treated as
// dangling — reaping on uncertainty could delete a genuinely-live worktree.
mockedReaddirSync.mockReturnValue([makeDirEntry("maybe-wt")] as any);
mockedLstatSync.mockImplementation((p: any) =>
(String(p).endsWith("/.git")
? { isDirectory: () => false, isSymbolicLink: () => false }
: { isDirectory: () => true, isSymbolicLink: () => false }) as any,
);
mockedReadFileSync.mockReturnValue("not a gitdir pointer at all\n" as any);
mockedExistsSync.mockImplementation((p) => {
const s = String(p);
return s === "/root/.worktrees" || s === "/root/.worktrees/maybe-wt/.git";
});
const removed = await reapOrphanWorktrees("/root");
expect(removed).toBe(0);
expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/maybe-wt", expect.anything());
});
});

View File

@@ -6,7 +6,7 @@ import { setImmediate as setImmediateCb } from "node:timers";
// Internal git plumbing intentionally bypasses sandbox backends.
const execAsync = promisify(exec);
import { delimiter, isAbsolute, join, relative, resolve as resolvePath } from "node:path";
import { existsSync, realpathSync } from "node:fs";
import { existsSync, lstatSync, realpathSync } from "node:fs";
import { readFile, rm, writeFile } from "node:fs/promises";
import type { TaskStore, Task, TaskDetail, TaskTokenUsage, StepStatus, Settings, WorkflowStep, MissionStore, Slice, AgentState, AgentCapability, RunMutationContext, AgentHeartbeatConfig, Agent, AgentMemoryInclusionMode, ProjectSettings, MergeResult, WorkflowIrNode, WorkflowIrNodeKind } from "@fusion/core";
import { getUnmetSchedulingDependencies } from "./scheduler.js";
@@ -14260,11 +14260,50 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
// 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 =
//
// Exclude spawn failures (e.g. `spawn git ENOENT` when the git binary is missing or not
// on PATH): those are environment errors, not "path is not a worktree" signals, and must
// not be misread as a successful stale-path cleanup.
const err = error as NodeJS.ErrnoException;
const isSpawnFailure = typeof err?.syscall === "string" && err.syscall.startsWith("spawn");
const staleConflictPath = !isSpawnFailure && (
/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);
/no such file or directory|ENOENT/i.test(errorMessage)
);
if (staleConflictPath) {
// The error string alone is NOT authoritative — it can name an unrelated path, or fire
// on a live worktree under a racing/transient failure. Re-verify on disk before any
// destructive action and refuse to force-remove anything that is still a real worktree,
// out of bounds, reached through a symlink, or actively owned by a live session. Only a
// genuine orphan directory inside the configured worktrees tree is safe to delete.
const settings = await this.store.getSettings();
const stillRegistered = await isRegisteredGitWorktree(this.rootDir, worktreePath).catch(() => true);
const activeOwner = await this.findActiveWorktreeOwner(worktreePath, taskId).catch(() => "unknown");
let safeToRemove = isInsideWorktreesDir(this.rootDir, worktreePath, settings) && !stillRegistered && activeOwner === null;
if (safeToRemove && existsSync(worktreePath)) {
try {
if (lstatSync(worktreePath).isSymbolicLink()) {
safeToRemove = false;
} else if (!isInsideWorktreesDir(this.rootDir, realpathSync(worktreePath), settings)) {
safeToRemove = false;
}
} catch {
// Stat failed (path vanished mid-check) — nothing to remove; the prune/branch
// cleanup below is still safe to run.
}
}
if (!safeToRemove) {
// A real/registered/out-of-bounds/owned/symlinked path we must not touch. Surface as a
// cleanup failure so the operator-recovery path handles it instead of silently
// claiming success (and never `rm -rf`-ing something we shouldn't).
await this.store.logEntry(
taskId,
`Refused stale-path cleanup — path is not a safe orphan (registered=${stillRegistered}, owner=${activeOwner ?? "none"})`,
worktreePath,
);
return false;
}
try {
await execAsync("git worktree prune", {
cwd: this.rootDir,
@@ -14277,11 +14316,13 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
}
// 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}`);
if (existsSync(worktreePath)) {
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 });

View File

@@ -849,30 +849,30 @@ export async function cleanupOrphanedWorktrees(
* @returns Number of orphan directories removed
*/
/**
* Resolve a worktree's `.git` pointer to the gitdir admin path it references.
* Decide whether a worktree's `.git` pointer is *dangling* — present on disk but
* referencing a `.git/worktrees/<name>` admin entry that no longer exists. A
* dangling pointer is FN-6782 leak residue: invisible to `git worktree list` /
* `prune`, yet it collides with freshly generated worktree names.
*
* 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: <path>` 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.
* Returns `true` ONLY when the pointer is confidently classifiable as dangling:
* a `gitdir: <path>` link file (relative targets resolved against the worktree
* dir) whose target is confirmed missing. Returns `false` for everything else —
* a real `.git` directory, a live gitdir target, an unparseable pointer, OR any
* read/stat failure. The conservative default matters: callers reap on `true`,
* so a transient read error (EACCES/EBUSY) on a genuinely-live worktree's `.git`
* must never be misread as dangling and force-removed.
*/
function resolveGitdirPointer(dotGitPath: string): string | "directory" | null {
function dotGitPointerIsDangling(dotGitPath: string): boolean {
try {
if (lstatSync(dotGitPath).isDirectory()) {
return "directory";
}
if (lstatSync(dotGitPath).isDirectory()) return false;
const raw = readFileSync(dotGitPath, "utf8").trim();
const match = /^gitdir:\s*(.+)$/.exec(raw);
if (!match) return null;
if (!match) return false;
const target = match[1].trim();
return isAbsolute(target) ? target : resolve(dirname(dotGitPath), target);
const resolved = isAbsolute(target) ? target : resolve(dirname(dotGitPath), target);
return !existsSync(resolved);
} catch {
return null;
return false;
}
}
@@ -940,10 +940,9 @@ export async function reapOrphanWorktrees(
// dangling pointers like any other half-initialized orphan.
const dotGit = join(resolvedFull, ".git");
if (existsSync(dotGit)) {
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.
if (!dotGitPointerIsDangling(dotGit)) {
// Valid registration, a real .git dir, or a pointer we couldn't positively classify as
// dangling — leave it; assertValidWorktreeSession handles it on the next agent start.
worktreePoolLog.log(`reapOrphanWorktrees: skipping ${name} (has .git entry but not in registered list — may be partially registered)`);
continue;
}