fix(FN-5256): keep live task worktrees through pause/resume races

Three independent reliability fixes that all surfaced as the same bug:
live tasks losing their worktrees mid-execution and emitting
`wrong_toplevel` errors.

Fix A — executor stale-self-owned classifier:
`reconcileSelfOwnedActiveSessionForRemoval` now takes a process-active
probe (`executingTaskLock.has`) and a minimum-idle window (default 5s)
in addition to the existing in-memory `activeWorktrees` binding probe.
Recently-registered or still-running entries are refused with
`process-active-refuses` / `too-recent-refuses`, with audit-grade
log lines. Both the pre-remove path
(`reconcileSelfOwnedBeforeRemove`), the post-throw retry in
`removeOwnWorktreeWithReconcile`, and the defensive reconcile in
`removeWorktree` route through the same hardened gates.

Fix B — pause synchronously reaps the agent session:
New `awaitAbortInFlightTaskWork` mirrors the existing fire-and-forget
abort but awaits each `session.abort()` /
`stepExecutor.terminateAllSessions()` /
`workflowSession.abort()`. `parkTaskAfterWorkflowStepPause` calls it
before `moveTask("todo")`, and the `task:updated` user-pause handler
routes through it, so a fast re-dispatch can no longer race a still-
live shell.

Fix C — self-healing realpath + active-task skip:
`reconcileTaskWorktreeMetadata` now realpath-normalizes both sides of
the registry comparison (handling macOS `/private/var/...`) and
refuses to clear `worktree`/`branch` on in-progress or in-review
tasks. The skip emits a new
`task:auto-recover-worktree-metadata-skipped-active` audit event;
executor-level recovery paths remain in charge of active tasks.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-05-21 09:06:23 -07:00
parent 2cdb51126d
commit cf0101be7c
16 changed files with 545 additions and 59 deletions

View File

@@ -94,7 +94,9 @@ describe("activeSessionRegistry", () => {
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-1", kind: "executor", ownerKey: "FN-1" });
expect(
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false),
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false, {
minIdleMs: 0,
}),
).toEqual({ action: "reconciled" });
expect(activeSessionRegistry.lookupByPath("/tmp/w1")).toBeNull();
});
@@ -103,10 +105,75 @@ describe("activeSessionRegistry", () => {
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-1", kind: "executor", ownerKey: "FN-1" });
expect(
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false),
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false, {
minIdleMs: 0,
}),
).toEqual({ action: "reconciled" });
expect(
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false),
reconcileSelfOwnedActiveSessionForRemoval(activeSessionRegistry, "/tmp/w1", "FN-1", () => false, {
minIdleMs: 0,
}),
).toEqual({ action: "no-entry" });
});
it("FN-5256: refuses reconcile when processActiveProbe returns true", () => {
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-1", kind: "executor", ownerKey: "FN-1" });
const outcome = reconcileSelfOwnedActiveSessionForRemoval(
activeSessionRegistry,
"/tmp/w1",
"FN-1",
() => false,
{ processActiveProbe: () => true, minIdleMs: 0 },
);
expect(outcome).toEqual({ action: "process-active-refuses", ownerTaskId: "FN-1" });
expect(activeSessionRegistry.lookupByPath("/tmp/w1")?.taskId).toBe("FN-1");
});
it("FN-5256: refuses reconcile when registration is younger than minIdleMs", () => {
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-1", kind: "executor", ownerKey: "FN-1" });
const registeredAt = activeSessionRegistry.lookupByPath("/tmp/w1")!.registeredAt;
const outcome = reconcileSelfOwnedActiveSessionForRemoval(
activeSessionRegistry,
"/tmp/w1",
"FN-1",
() => false,
{ minIdleMs: 5000, now: () => registeredAt + 100 },
);
expect(outcome).toMatchObject({ action: "too-recent-refuses", ownerTaskId: "FN-1", minIdleMs: 5000 });
expect(activeSessionRegistry.lookupByPath("/tmp/w1")?.taskId).toBe("FN-1");
});
it("FN-5256: reconciles when all signals clean (default min-idle window elapsed)", () => {
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-1", kind: "executor", ownerKey: "FN-1" });
const registeredAt = activeSessionRegistry.lookupByPath("/tmp/w1")!.registeredAt;
const outcome = reconcileSelfOwnedActiveSessionForRemoval(
activeSessionRegistry,
"/tmp/w1",
"FN-1",
() => false,
{
processActiveProbe: () => false,
minIdleMs: 5000,
now: () => registeredAt + 6000,
},
);
expect(outcome).toEqual({ action: "reconciled" });
expect(activeSessionRegistry.lookupByPath("/tmp/w1")).toBeNull();
});
it("FN-5256: live-binding takes precedence over process-active and too-recent", () => {
activeSessionRegistry.registerPath("/tmp/w1", { taskId: "FN-1", kind: "executor", ownerKey: "FN-1" });
const outcome = reconcileSelfOwnedActiveSessionForRemoval(
activeSessionRegistry,
"/tmp/w1",
"FN-1",
() => true,
{ processActiveProbe: () => true, minIdleMs: 5000 },
);
expect(outcome).toEqual({ action: "live-binding-refuses", ownerTaskId: "FN-1" });
});
});

View File

@@ -2723,3 +2723,112 @@ describe("StepSessionExecutor integration", () => {
});
});
describe("FN-5256 awaitAbortInFlightTaskWork pause synchronization", () => {
it("synchronously disposes activeSessions on pause via task:updated", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
let abortResolve: (() => void) | undefined;
const abortPromise = new Promise<void>((resolve) => {
abortResolve = resolve;
});
const abortSpy = vi.fn(() => abortPromise);
const disposeSpy = vi.fn();
(executor as any).activeSessions.set("FN-PAUSE-1", {
session: { abort: abortSpy, dispose: disposeSpy, steer: vi.fn() },
seenSteeringIds: new Set(),
});
const taskUpdatedHandler = (store.on as unknown as ReturnType<typeof vi.fn>).mock.calls
.find((call: any[]) => call[0] === "task:updated")?.[1] as (task: any) => Promise<void>;
expect(taskUpdatedHandler).toBeTypeOf("function");
const settled = taskUpdatedHandler({
id: "FN-PAUSE-1",
title: "pause",
description: "pause",
column: "in-progress",
paused: true,
dependencies: [],
steps: [],
currentStep: 0,
log: [],
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
} satisfies Task);
// Before abort resolves, dispose must NOT have been called — i.e. handler is
// truly awaiting the abort, not fire-and-forgetting it.
await new Promise((resolve) => setImmediate(resolve));
expect(disposeSpy).not.toHaveBeenCalled();
abortResolve?.();
await settled;
expect(abortSpy).toHaveBeenCalledTimes(1);
expect(disposeSpy).toHaveBeenCalledTimes(1);
expect((executor as any).activeSessions.has("FN-PAUSE-1")).toBe(false);
});
it("synchronously reaps workflow-step + step-session surfaces on pause", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
const stepTerminate = vi.fn().mockResolvedValue(undefined);
(executor as any).activeStepExecutors.set("FN-PAUSE-2", {
terminateAllSessions: stepTerminate,
});
const workflowAbort = vi.fn().mockResolvedValue(undefined);
const workflowDispose = vi.fn();
(executor as any).activeWorkflowStepSessions.set("FN-PAUSE-2", {
abort: workflowAbort,
dispose: workflowDispose,
});
const taskUpdatedHandler = (store.on as unknown as ReturnType<typeof vi.fn>).mock.calls
.find((call: any[]) => call[0] === "task:updated")?.[1] as (task: any) => Promise<void>;
await taskUpdatedHandler({
id: "FN-PAUSE-2",
title: "pause",
description: "pause",
column: "in-progress",
paused: true,
dependencies: [],
steps: [],
currentStep: 0,
log: [],
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
} satisfies Task);
expect(stepTerminate).toHaveBeenCalledTimes(1);
expect(workflowAbort).toHaveBeenCalledTimes(1);
expect(workflowDispose).toHaveBeenCalledTimes(1);
expect((executor as any).activeStepExecutors.has("FN-PAUSE-2")).toBe(false);
expect((executor as any).activeWorkflowStepSessions.has("FN-PAUSE-2")).toBe(false);
});
it("awaitAbortInFlightTaskWork awaits abort before dispose for all surfaces", async () => {
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
const order: string[] = [];
const sessionAbort = vi.fn().mockImplementation(async () => {
await new Promise((r) => setTimeout(r, 5));
order.push("session-abort");
});
const sessionDispose = vi.fn(() => order.push("session-dispose"));
(executor as any).activeSessions.set("FN-PAUSE-3", {
session: { abort: sessionAbort, dispose: sessionDispose, steer: vi.fn() },
seenSteeringIds: new Set(),
});
await (executor as any).awaitAbortInFlightTaskWork("FN-PAUSE-3", "test reason");
expect(order).toEqual(["session-abort", "session-dispose"]);
expect((executor as any).activeSessions.has("FN-PAUSE-3")).toBe(false);
});
});

View File

@@ -1939,7 +1939,10 @@ describe("TaskExecutor task:updated listener guards", () => {
} satisfies Task)).resolves.toBeUndefined();
expect(terminateAllSessions).toHaveBeenCalledTimes(1);
expect(executorLog.error).toHaveBeenCalledWith("Uncaught error in task:updated listener:", terminateError);
// FN-5256: the pause handler now routes through awaitAbortInFlightTaskWork,
// which internally catches/logs the per-surface failure. The error still hits
// executorLog.error but via the granular message path.
expect(executorLog.error).toHaveBeenCalledWith("Failed to terminate step sessions for FN-001:", terminateError);
});
});

View File

@@ -19,6 +19,8 @@ describe("FN-4973: executor worktree conflict cleanup", () => {
const executor = new TaskExecutor(store, "/tmp/test");
store.listTasks.mockResolvedValue([]);
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: "FN-4973", kind: "executor", ownerKey: "FN-4973" });
// FN-5256: backdate so the new min-idle window doesn't refuse the reconcile.
(activeSessionRegistry.lookupByPath(CONFLICT_PATH) as any).registeredAt = 0;
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
const result = await (executor as any).cleanupConflictingWorktree(CONFLICT_PATH, "fusion/fn-4973", "FN-4973");
@@ -85,6 +87,8 @@ describe("FN-4973: executor worktree conflict cleanup", () => {
removeSpy
.mockImplementationOnce(async () => {
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: "FN-4973", kind: "executor", ownerKey: "FN-4973" });
// FN-5256: backdate so the post-throw reconcile is not refused by min-idle.
(activeSessionRegistry.lookupByPath(CONFLICT_PATH) as any).registeredAt = 0;
throw new ActiveSessionWorktreeRemovalError({
worktreePath: CONFLICT_PATH,
taskId: "FN-4973",

View File

@@ -25,6 +25,7 @@ describe("FN-5346 reliability interactions: post-completion stale self-owned bin
const executor = new TaskExecutor(store, ROOT);
(executor as any).activeWorktrees.set(TASK_ID, PATH);
activeSessionRegistry.registerPath(PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
(activeSessionRegistry.lookupByPath(PATH) as any).registeredAt = 0;
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
await executor.cleanup(TASK_ID);
@@ -43,6 +44,7 @@ describe("FN-5346 reliability interactions: post-completion stale self-owned bin
const store = createMockStore();
const executor = new TaskExecutor(store, ROOT);
activeSessionRegistry.registerPath(PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
(activeSessionRegistry.lookupByPath(PATH) as any).registeredAt = 0;
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
await (executor as any).handleDepAbortCleanup(TASK_ID, PATH);
@@ -96,6 +98,7 @@ describe("FN-5346 reliability interactions: post-completion stale self-owned bin
const executor = new TaskExecutor(store, ROOT);
(executor as any).activeWorktrees.set(TASK_ID, PATH);
activeSessionRegistry.registerPath(PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
(activeSessionRegistry.lookupByPath(PATH) as any).registeredAt = 0;
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);
await executor.cleanup(TASK_ID);

View File

@@ -74,6 +74,8 @@ describe("FN-4973 reliability interactions: stale self-owned active-session reco
store.listTasks.mockResolvedValue([]);
const executor = new TaskExecutor(store, "/tmp/test");
activeSessionRegistry.registerPath(CONFLICT_PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
// FN-5256: backdate so the new min-idle window doesn't refuse the reconcile.
(activeSessionRegistry.lookupByPath(CONFLICT_PATH) as any).registeredAt = 0;
vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);

View File

@@ -25,6 +25,8 @@ describe("FN-4976: stale self-owned activeSessionRegistry deadlock backstop", ()
const store = createMockStore();
store.listTasks.mockResolvedValue([]);
activeSessionRegistry.registerPath(PATH, { taskId: TASK_ID, kind: "executor", ownerKey: TASK_ID });
// FN-5256: backdate so the new min-idle window doesn't refuse the reconcile.
(activeSessionRegistry.lookupByPath(PATH) as any).registeredAt = 0;
const executor = new TaskExecutor(store, ROOT);
const removeSpy = vi.spyOn(worktreePoolModule, "removeWorktree").mockResolvedValue(undefined);

View File

@@ -129,7 +129,10 @@ describeIfGit("SelfHealingManager recoverAlreadyMergedReviewTasks (real git)", (
expect(task.mergeDetails?.mergeConfirmed).toBe(true);
expect(existsSync(worktreePath)).toBe(false);
expect(git(repo, "git worktree list")).not.toContain(worktreePath);
expect((store as any).recordRunAuditEvent).toHaveBeenCalledTimes(3);
// FN-5256: reconcileTaskWorktreeMetadata now normalizes via realpath, so the
// (formerly false-stale) macOS realpath mismatch no longer triggers an extra
// worktree-metadata-cleared audit event for this in-review task.
expect((store as any).recordRunAuditEvent).toHaveBeenCalledTimes(2);
expect((store as any).recordRunAuditEvent).toHaveBeenCalledWith(
expect.objectContaining({
domain: "database",

View File

@@ -1,4 +1,4 @@
import { mkdtempSync, mkdirSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs";
import { mkdtempSync, mkdirSync, readFileSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs";
import { fileURLToPath } from "node:url";
import { tmpdir } from "node:os";
import { join } from "node:path";
@@ -167,3 +167,98 @@ describe("reconcileTaskWorktreeMetadata matrix", () => {
expect((store as any).updateTask).not.toHaveBeenCalled();
});
});
describe("FN-5256 reconcileTaskWorktreeMetadata reliability", () => {
let rootDir = "";
let store: TaskStore;
beforeEach(() => {
rootDir = mkdtempSync(join(tmpdir(), "fn-5256-"));
git(rootDir, "init -b main");
git(rootDir, "config user.name 'Fusion'");
git(rootDir, "config user.email 'hi@runfusion.ai'");
writeFileSync(join(rootDir, "README.md"), "root\n");
git(rootDir, "add README.md");
git(rootDir, "commit -m 'init'");
store = new TaskStore(rootDir, undefined, { inMemoryDb: false });
});
afterEach(() => {
try {
store?.close();
} catch {
// noop
}
if (rootDir) rmSync(rootDir, { recursive: true, force: true });
vi.clearAllMocks();
});
it("FN-5256: realpath-normalizes both sides so a symlinked task.worktree is not falsely flagged stale", async () => {
await store.createTask({ title: "FN-5256 symlink", description: "symlink case" });
const [task] = await store.listTasks();
mkdirSync(join(rootDir, ".worktrees"), { recursive: true });
const realWorktreeDir = join(realpathSync(rootDir), ".worktrees", "real-leaf");
const branch = `fusion/${task.id.toLowerCase()}`;
git(rootDir, `branch ${branch}`);
git(rootDir, `worktree add ${realWorktreeDir} ${branch}`);
// Create a symlink that points at the registered worktree's parent. The task
// metadata persists the symlinked path; the registry will surface realpath.
const symlinkParent = join(rootDir, ".worktrees-symlink");
symlinkSync(join(rootDir, ".worktrees"), symlinkParent, "dir");
const symlinkedTaskWorktree = join(symlinkParent, "real-leaf");
await store.updateTask(task.id, { worktree: symlinkedTaskWorktree, branch });
const auditSpy = vi.spyOn(store, "recordRunAuditEvent");
const manager = new SelfHealingManager(store, {
rootDir,
getExecutingTaskIds: () => new Set<string>(),
});
const repaired = await (manager as any).reconcileTaskWorktreeMetadata();
expect(repaired).toBe(0);
const updated = await store.getTask(task.id);
expect(updated?.worktree).toBe(symlinkedTaskWorktree);
expect(updated?.branch).toBe(branch);
expect(auditSpy).not.toHaveBeenCalledWith(
expect.objectContaining({ mutationType: "task:auto-recover-worktree-metadata-cleared" }),
);
});
it("FN-5256: refuses to clear worktree metadata for an in-progress task with a stale-flagged worktree", async () => {
await store.createTask({ title: "FN-5256 in-progress", description: "stale active task" });
const [task] = await store.listTasks();
await store.moveTask(task.id, "todo");
await store.moveTask(task.id, "in-progress");
// The worktree path is bogus (not registered) but the task is active. The
// reconciler must not yank metadata out from under a running task.
const stalePath = join(rootDir, ".worktrees", "ghost-leaf");
mkdirSync(stalePath, { recursive: true });
await store.updateTask(task.id, { worktree: stalePath, branch: `fusion/${task.id.toLowerCase()}` });
const auditSpy = vi.spyOn(store, "recordRunAuditEvent");
const manager = new SelfHealingManager(store, {
rootDir,
getExecutingTaskIds: () => new Set<string>(),
});
const repaired = await (manager as any).reconcileTaskWorktreeMetadata();
expect(repaired).toBe(0);
const updated = await store.getTask(task.id);
expect(updated?.worktree).toBe(stalePath);
expect(updated?.branch).toBe(`fusion/${task.id.toLowerCase()}`);
expect(auditSpy).not.toHaveBeenCalledWith(
expect.objectContaining({ mutationType: "task:auto-recover-worktree-metadata-cleared" }),
);
expect(auditSpy).toHaveBeenCalledWith(
expect.objectContaining({ mutationType: "task:auto-recover-worktree-metadata-skipped-active" }),
);
});
});

View File

@@ -873,6 +873,9 @@ describe("removeWorktree", () => {
reason: RemovalReason.ExecutorDispose,
expectedOwnerTaskId: "FN-1",
liveOwnerProbe: () => false,
// FN-5256: opt out of the min-idle window so this defensive-reconcile test
// is unaffected by the new warm-up gate.
reconcileMinIdleMs: 0,
});
expect(activeSessionRegistry.lookupByPath("/repo/.worktrees/fn-1")).toBeNull();