fix(FN-4811): recover from validation-failed remove + collapse broken FN-4806 nested branches
Two follow-ups stacked on the FN-4811 active-worktree liveness gate:
1. Stale conflict-path recovery (FN-4813 production failure)
When 'git worktree remove --force' fails with 'fatal: validation failed,
cannot remove working tree', the worktree directory is missing on disk
and the git admin entry is stale. Without this recovery, every retry of
tryCreateWorktree on a stale conflict path failed 3 times with
'automatic cleanup failed', leaving tasks unable to create worktrees.
cleanupConflictingWorktree now catches that specific error class, runs
'git worktree prune' to drop the stale admin entry, best-effort deletes
the branch, and returns success so the caller can proceed.
Implementation note: the original attempt used existsSync(worktreePath)
as a pre-check, but vitest's vi.clearAllMocks() can leave the existsSync
mock returning undefined, causing the new branch to fire inside tests
that didn't expect it and leading to worker OOM in
executor-worktree.test.ts. The error-class-based catch is robust against
mock state and matches the real production failure signal exactly.
2. Collapsed broken FN-4806 nested branches
The previous FN-4806 refactor (commit 087b1a766) accidentally nested the
genuine 'agent finished without calling fn_task_done after N retries'
failure path INSIDE the silent-recovery branch, meaning ordinary
failures were being silently requeued (no status=failed, no onError, no
retry-budget burn) instead of being surfaced.
Restored the clean two-branch structure:
} else if (retryAbortedDueToReclaim) {
// silent recovery (FN-4806)
} else {
// genuine no-fn_task_done exhaustion: mark failed, onError, burn budget
}
Also clears baseCommitSha on silent recovery (matches the parallel
session-start-failure path's metadata clearing).
Tests:
- Adds 'FN-4811 follow-up (FN-4813): recovers from validation failed'
case to active-worktree-removal-liveness.test.ts (12 total cases).
- executor-recovery.test.ts no-fn_task_done reclaim coverage now
asserts baseCommitSha is cleared.
- executor-recovery.test.ts 'does not mark task as failed when invalid
transition error occurs on completion' regression fixed by restoring
the failure-path branch.
- executor-core.test.ts 'still enforces fn_task_done requirement in
fast mode' restored.
Full engine suite: 307 files, 5037 tests pass, 1 skipped. Lint clean.
Fusion-Task-Id: FN-4811
This commit is contained in:
11
.changeset/FN-4811-stale-conflict-path-recovery.md
Normal file
11
.changeset/FN-4811-stale-conflict-path-recovery.md
Normal file
@@ -0,0 +1,11 @@
|
|||||||
|
---
|
||||||
|
"@runfusion/fusion": patch
|
||||||
|
---
|
||||||
|
|
||||||
|
fix(FN-4811): recover from "validation failed, cannot remove working tree" + collapse broken FN-4806 nested branches
|
||||||
|
|
||||||
|
Two follow-ups stacked on the FN-4811 active-worktree liveness gate:
|
||||||
|
|
||||||
|
1. **Stale conflict-path recovery (was breaking real tasks).** When `git worktree remove --force` fails with `fatal: validation failed, cannot remove working tree`, the worktree directory is missing on disk and the git admin entry is stale. `cleanupConflictingWorktree` now catches that specific error, runs `git worktree prune`, best-effort deletes the branch, and returns success — so the caller can proceed with worktree creation instead of failing 3× with "automatic cleanup failed" (FN-4813 production failure).
|
||||||
|
|
||||||
|
2. **Collapsed broken FN-4806 nested branches.** The previous FN-4806 refactor accidentally nested the genuine "agent finished without calling fn_task_done" failure path inside the silent-recovery branch, so ordinary failures were being silently requeued instead of marked failed. Restored the clean two-branch structure: `else if (retryAbortedDueToReclaim)` silent-recovers, `else` marks failed/onError/burns budget. Also clears `baseCommitSha` on silent recovery (matches the parallel session-start-failure path).
|
||||||
@@ -17,7 +17,7 @@ import "../executor-test-helpers.js";
|
|||||||
import { TaskExecutor } from "../../executor.js";
|
import { TaskExecutor } from "../../executor.js";
|
||||||
import { BranchConflictError } from "../../branch-conflicts.js";
|
import { BranchConflictError } from "../../branch-conflicts.js";
|
||||||
import * as branchConflictModule from "../../branch-conflicts.js";
|
import * as branchConflictModule from "../../branch-conflicts.js";
|
||||||
import { createMockStore, resetExecutorMocks } from "../executor-test-helpers.js";
|
import { createMockStore, mockedExec, mockedExistsSync, resetExecutorMocks } from "../executor-test-helpers.js";
|
||||||
|
|
||||||
const ACTIVE_PATH = "/tmp/test/.worktrees/lemon-reef";
|
const ACTIVE_PATH = "/tmp/test/.worktrees/lemon-reef";
|
||||||
const STALE_PATH = "/tmp/test/.worktrees/azure-peach";
|
const STALE_PATH = "/tmp/test/.worktrees/azure-peach";
|
||||||
@@ -165,22 +165,61 @@ describe("FN-4811: active worktree removal liveness gate", () => {
|
|||||||
store.listTasks.mockResolvedValue([
|
store.listTasks.mockResolvedValue([
|
||||||
{ id: "FN-DONE", worktree: STALE_PATH, column: "done", paused: false },
|
{ id: "FN-DONE", worktree: STALE_PATH, column: "done", paused: false },
|
||||||
]);
|
]);
|
||||||
|
// existsSync defaults to true in helpers; this exercises the standard remove path.
|
||||||
|
mockedExistsSync.mockImplementation((p: string) => p === STALE_PATH);
|
||||||
|
|
||||||
// Spy on removeWorktree to confirm it's invoked. The mocked exec in test helpers will
|
|
||||||
// handle the actual git command without touching disk.
|
|
||||||
const result = await (executor as any).cleanupConflictingWorktree(
|
const result = await (executor as any).cleanupConflictingWorktree(
|
||||||
STALE_PATH,
|
STALE_PATH,
|
||||||
"fusion/fn-9999",
|
"fusion/fn-9999",
|
||||||
"FN-4811",
|
"FN-4811",
|
||||||
);
|
);
|
||||||
|
|
||||||
// result may be true or false depending on whether mocked exec succeeds, but the key
|
|
||||||
// assertion is that the refusal log was NOT emitted (i.e., we got past the gate).
|
|
||||||
const logCalls = store.logEntry.mock.calls.map((c: any[]) => String(c[1] ?? ""));
|
const logCalls = store.logEntry.mock.calls.map((c: any[]) => String(c[1] ?? ""));
|
||||||
expect(logCalls.some((m: string) => m.includes("Refused to remove conflicting worktree"))).toBe(false);
|
expect(logCalls.some((m: string) => m.includes("Refused to remove conflicting worktree"))).toBe(false);
|
||||||
// result is the actual outcome of the removal attempt; the gate didn't block it.
|
|
||||||
void result;
|
void result;
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("FN-4811 follow-up (FN-4813): recovers from 'validation failed, cannot remove working tree'", async () => {
|
||||||
|
// Regression: a stale worktree admin entry (or missing on-disk directory) causes
|
||||||
|
// `git worktree remove --force` to fail with 'validation failed, cannot remove
|
||||||
|
// working tree'. The cleanup must catch that specific error, prune the stale admin
|
||||||
|
// entry, best-effort delete the branch, and return success so the caller can proceed
|
||||||
|
// with worktree creation.
|
||||||
|
const store = createMockStore();
|
||||||
|
const executor = new TaskExecutor(store, "/tmp/test");
|
||||||
|
store.listTasks.mockResolvedValue([]);
|
||||||
|
|
||||||
|
const execCalls: string[] = [];
|
||||||
|
mockedExec.mockImplementation((cmd: string, _opts: unknown, cb: (err: Error | null, out: { stdout: string; stderr: string }) => void) => {
|
||||||
|
execCalls.push(cmd);
|
||||||
|
if (cmd.includes("git worktree remove")) {
|
||||||
|
const err: any = new Error(
|
||||||
|
`Command failed: ${cmd}\nfatal: validation failed, cannot remove working tree:`,
|
||||||
|
);
|
||||||
|
err.stderr = "fatal: validation failed, cannot remove working tree:";
|
||||||
|
cb(err, { stdout: "", stderr: err.stderr });
|
||||||
|
return { kill: () => undefined } as any;
|
||||||
|
}
|
||||||
|
cb(null, { stdout: "", stderr: "" });
|
||||||
|
return { kill: () => undefined } as any;
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await (executor as any).cleanupConflictingWorktree(
|
||||||
|
STALE_PATH,
|
||||||
|
"fusion/fn-9999",
|
||||||
|
"FN-4811",
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(result).toBe(true);
|
||||||
|
// Must have run prune after the validation-failed catch.
|
||||||
|
expect(execCalls.some((c) => c.includes("git worktree prune"))).toBe(true);
|
||||||
|
// Must have attempted branch -D as part of the recovery.
|
||||||
|
expect(execCalls.some((c) => c.includes('git branch -D "fusion/fn-9999"'))).toBe(true);
|
||||||
|
// Must have logged the stale-path cleanup outcome — NOT the generic failure log.
|
||||||
|
const logCalls = store.logEntry.mock.calls.map((c: any[]) => String(c[1] ?? ""));
|
||||||
|
expect(logCalls.some((m: string) => m.includes("Cleaned up stale conflicting worktree admin entry"))).toBe(true);
|
||||||
|
expect(logCalls.some((m: string) => m === "Failed to clean up conflicting worktree")).toBe(false);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("handleBranchConflict liveness gate", () => {
|
describe("handleBranchConflict liveness gate", () => {
|
||||||
|
|||||||
@@ -4046,54 +4046,43 @@ export class TaskExecutor {
|
|||||||
this.currentRunContext,
|
this.currentRunContext,
|
||||||
);
|
);
|
||||||
// Clear any stale binding so the next pickup creates a fresh worktree.
|
// Clear any stale binding so the next pickup creates a fresh worktree.
|
||||||
await this.store.updateTask(task.id, { worktree: null, branch: null });
|
// baseCommitSha is also cleared because it pinned to the now-reclaimed worktree;
|
||||||
|
// the next pickup will re-anchor it on the fresh checkout.
|
||||||
|
await this.store.updateTask(task.id, { worktree: null, branch: null, baseCommitSha: null });
|
||||||
await this.persistTokenUsage(task.id);
|
await this.persistTokenUsage(task.id);
|
||||||
await this.store.moveTask(task.id, "todo", { preserveProgress: true });
|
await this.store.moveTask(task.id, "todo", { preserveProgress: true });
|
||||||
executorLog.log(silentMessage);
|
executorLog.log(silentMessage);
|
||||||
} else {
|
} else {
|
||||||
|
// FN-4806: Genuine "agent finished without calling fn_task_done after N retries"
|
||||||
|
// exhaustion. Not a reclaim/self-heal — the agent had a fair chance and failed to
|
||||||
|
// signal completion. Mark failed, surface onError, and either requeue (budget
|
||||||
|
// remaining) or escalate to in-review (budget exhausted).
|
||||||
const priorRequeues = task.taskDoneRetryCount ?? 0;
|
const priorRequeues = task.taskDoneRetryCount ?? 0;
|
||||||
const nextRequeueCount = priorRequeues + 1;
|
const nextRequeueCount = priorRequeues + 1;
|
||||||
const errorMessage = `Agent finished without calling fn_task_done (after ${MAX_TASK_DONE_SESSION_RETRIES} retries)`;
|
const errorMessage = `Agent finished without calling fn_task_done (after ${MAX_TASK_DONE_SESSION_RETRIES} retries)`;
|
||||||
|
|
||||||
if (priorRequeues < MAX_TASK_DONE_REQUEUE_RETRIES) {
|
if (priorRequeues < MAX_TASK_DONE_REQUEUE_RETRIES) {
|
||||||
await this.store.updateTask(task.id, {
|
await this.store.updateTask(task.id, {
|
||||||
sessionFile: null,
|
status: "failed",
|
||||||
worktree: null,
|
error: errorMessage,
|
||||||
branch: null,
|
taskDoneRetryCount: nextRequeueCount,
|
||||||
baseCommitSha: null,
|
|
||||||
});
|
});
|
||||||
const reclaimMessage = "Worktree/branch reclaimed mid-retry — requeued to todo (engine self-heal, no failure)";
|
await this.store.logEntry(
|
||||||
await this.store.logEntry(task.id, reclaimMessage, undefined, this.currentRunContext);
|
task.id,
|
||||||
executorLog.log(`${task.id}: ${reclaimMessage}`);
|
`${errorMessage} — requeued to todo immediately (${nextRequeueCount}/${MAX_TASK_DONE_REQUEUE_RETRIES})`,
|
||||||
|
undefined,
|
||||||
|
this.currentRunContext,
|
||||||
|
);
|
||||||
await this.store.moveTask(task.id, "todo", { preserveProgress: true });
|
await this.store.moveTask(task.id, "todo", { preserveProgress: true });
|
||||||
|
executorLog.log(`✗ ${task.id} failed after ${MAX_TASK_DONE_SESSION_RETRIES} retries — requeued to todo (${nextRequeueCount}/${MAX_TASK_DONE_REQUEUE_RETRIES})`);
|
||||||
} else {
|
} else {
|
||||||
const priorRequeues = task.taskDoneRetryCount ?? 0;
|
await this.store.updateTask(task.id, { status: "failed", error: errorMessage });
|
||||||
const nextRequeueCount = priorRequeues + 1;
|
await this.store.logEntry(task.id, `${errorMessage} — moved to in-review for inspection`, undefined, this.currentRunContext);
|
||||||
const errorMessage = `Agent finished without calling fn_task_done (after ${MAX_TASK_DONE_SESSION_RETRIES} retries)`;
|
await this.persistTokenUsage(task.id);
|
||||||
|
await this.store.moveTask(task.id, "in-review");
|
||||||
if (priorRequeues < MAX_TASK_DONE_REQUEUE_RETRIES) {
|
executorLog.log(`✗ ${task.id} failed after ${MAX_TASK_DONE_SESSION_RETRIES} retries — no fn_task_done → in-review`);
|
||||||
await this.store.updateTask(task.id, {
|
|
||||||
status: "failed",
|
|
||||||
error: errorMessage,
|
|
||||||
taskDoneRetryCount: nextRequeueCount,
|
|
||||||
});
|
|
||||||
await this.store.logEntry(
|
|
||||||
task.id,
|
|
||||||
`${errorMessage} — requeued to todo immediately (${nextRequeueCount}/${MAX_TASK_DONE_REQUEUE_RETRIES})`,
|
|
||||||
undefined,
|
|
||||||
this.currentRunContext,
|
|
||||||
);
|
|
||||||
await this.store.moveTask(task.id, "todo", { preserveProgress: true });
|
|
||||||
executorLog.log(`✗ ${task.id} failed after ${MAX_TASK_DONE_SESSION_RETRIES} retries — requeued to todo (${nextRequeueCount}/${MAX_TASK_DONE_REQUEUE_RETRIES})`);
|
|
||||||
} else {
|
|
||||||
await this.store.updateTask(task.id, { status: "failed", error: errorMessage });
|
|
||||||
await this.store.logEntry(task.id, `${errorMessage} — moved to in-review for inspection`, undefined, this.currentRunContext);
|
|
||||||
await this.persistTokenUsage(task.id);
|
|
||||||
await this.store.moveTask(task.id, "in-review");
|
|
||||||
executorLog.log(`✗ ${task.id} failed after ${MAX_TASK_DONE_SESSION_RETRIES} retries — no fn_task_done → in-review`);
|
|
||||||
}
|
|
||||||
this.options.onError?.(task, new Error(errorMessage));
|
|
||||||
}
|
}
|
||||||
|
this.options.onError?.(task, new Error(errorMessage));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
} finally {
|
} finally {
|
||||||
@@ -8378,6 +8367,37 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit
|
|||||||
return true;
|
return true;
|
||||||
} catch (error: unknown) {
|
} catch (error: unknown) {
|
||||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
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)) {
|
||||||
|
try {
|
||||||
|
await execAsync("git worktree prune", {
|
||||||
|
cwd: this.rootDir,
|
||||||
|
timeout: 30_000,
|
||||||
|
maxBuffer: 10 * 1024 * 1024,
|
||||||
|
});
|
||||||
|
} catch (pruneErr: unknown) {
|
||||||
|
const pruneMsg = pruneErr instanceof Error ? pruneErr.message : String(pruneErr);
|
||||||
|
executorLog.warn(`${taskId}: git worktree prune failed during stale-path cleanup of ${worktreePath}: ${pruneMsg}`);
|
||||||
|
}
|
||||||
|
try {
|
||||||
|
await execAsync(`git branch -D "${branch}"`, { cwd: this.rootDir });
|
||||||
|
this.store.clearStaleExecutionStartBranchReferences([branch], taskId);
|
||||||
|
} catch {
|
||||||
|
// best-effort — branch may not exist, which is fine for a stale-path cleanup
|
||||||
|
}
|
||||||
|
await this.store.logEntry(
|
||||||
|
taskId,
|
||||||
|
`Cleaned up stale conflicting worktree admin entry (validation failed — path likely missing on disk)`,
|
||||||
|
worktreePath,
|
||||||
|
);
|
||||||
|
return true;
|
||||||
|
}
|
||||||
await this.store.logEntry(
|
await this.store.logEntry(
|
||||||
taskId,
|
taskId,
|
||||||
`Failed to clean up conflicting worktree`,
|
`Failed to clean up conflicting worktree`,
|
||||||
|
|||||||
Reference in New Issue
Block a user