P0 fix: stop reaping worktrees out from under live planners (FN-6756) (#2531)
User-reported: worktrees deleted while a planning agent was still working in them. Small, isolated, ahead of all remaining capacity work. ## Mechanism `clearPhantomExecutorBinding` is documented as *"the last line of defense against pulling a worktree out from under a running agent"*. It computed liveness from four sets — `activeSessions`, `activeStepExecutors`, `activeWorkflowStepSessions`, `activeCliTaskSessions` — **all TaskExecutor-owned**. A triage PLANNING session is owned by `TriageProcessor`, lives in *its own* `activeSessions` map, and registers in the module-level `activeSessionRegistry`. It matched none of the four. Worse: the method **writes** to that registry (unregistering the task’s paths) but never **read** it as a liveness signal. It destroyed the very evidence that proved the planner alive. Under plan-in-place a card is specified while it sits in `todo`/`triage`, and `reapLeakedConcurrencySlots` treats both as reapable on a rationale written *before* planning moved there (“a task waiting to run must not pin a worktree”). Every gate ahead of the last one passes for a planner: | Gate | Saves a planner? | |---|---| | in `listWorktreeHolders()`? | **No** — `ensureTaskWorktreeForPlanning` → `ensureGraphCustomNodeWorktree` → `addActiveWorktree` (`executor.ts:8581`) | | reapable column? | **No** — plan-in-place keeps the card in `todo`/`triage` | | in the executor’s `executing` set? | **No** — a planner is triage-owned | | 60 s `LEAKED_WORKTREE_SLOT_GRACE_MS` | **No** — keyed on `columnMovedAt`, and planning routinely runs for minutes | So the broken guard decided alone. ## This is FN-8600 recurring through a second sweep That fix registered planning paths in the registry and taught the **self-owned-branch reclaim** sweep to consult `isPathActive`. The leaked-slot reaper never got the same signal — fixed at one surface, not enumerated across all. Exactly what the AGENTS.md Surface Enumeration rule exists to prevent. ## Fix The refusal now also fires when `activeSessionRegistry.pathsForTask(taskId)` is non-empty. Keyed on **any** registered path rather than on kind: the point is that a registered surface of any kind means someone is working in that worktree. ## Enumeration — the part that stops a third recurrence The guard is a **chokepoint**, so this covers every caller rather than just the reported one: - `reapLeakedConcurrencySlots` — the reported path - `recoverPausedAbortFailures` — **had the identical executor-only pre-gate** - the `preserveWorktrees: true` reclaim Audited the rest of self-healing’s liveness gates: the self-owned-branch reclaim, worktree-metadata reconcile and PR-branch sweeps already consult `isPathActive`/`lookupByPath`. The three that read only `getExecutingTaskIds` — `checkStuckBudget`, `recoverCompletedTasks`, `recoverStrandedCompletedTodoTasks` — move columns and never destroy a worktree, so they are noted rather than changed. ## Trade-off, stated plainly A leaked registry entry now blocks this sweep instead of a live planner losing its worktree. That is the strictly safer failure and the one the “last line of defense” wording already promises. The registry is process-local and in-memory, so a leak cannot outlive the process, and stale entries have their own reconciler. **A test pins that a genuine phantom — no executor surface AND no registration — still clears**, so this is not a blanket refusal that would trade this bug for a wedged queue. **The 60 s grace is deliberately unchanged.** Raising it would only make the bug rarer and harder to reproduce; the liveness gate was the defect. ## Verification Revert-proof, measured: removing the registry term turns **3 of the 4** new tests red, including the end-to-end sweep case (card in `triage`, past the grace, executor sets empty → asserts the slot is not reaped and the worktree survives). The 4th stays green both ways *by design* — it is the anti-overcorrection guard. `pnpm lint` clean · engine `tsc` clean · `pnpm test:gate` green (309 + 10 + 71) · new suite 4/4. The 2 failures in `self-healing.test.ts` / `-completion-fanout.test.ts` are **pre-existing** — identical with this change stashed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented active planning worktrees from being mistakenly deleted or reclaimed while related planning sessions are still active. * Enhanced session liveness checks so phantom executor bindings are not cleared when a live session is registered. * Updated paused abort recovery to defer or abort safely when a live planning session is detected, avoiding unintended task/worktree mutations. * **Tests** * Added regression coverage for leaked-slot reaping, paused abort recovery behavior, phantom binding refusal, and end-to-end sweep outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
7
.changeset/fix-planner-worktree-reap.md
Normal file
7
.changeset/fix-planner-worktree-reap.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Fix worktrees being deleted while a planning agent was still working in them.
|
||||
category: fix
|
||||
dev: `clearPhantomExecutorBinding` computed liveness from four TaskExecutor-owned session maps only, so a triage planning session — owned by TriageProcessor and registered in the module-level `activeSessionRegistry` — was invisible to it. Under plan-in-place a card is specified while it sits in `todo`/`triage`, both reapable by `reapLeakedConcurrencySlots`, and planning routinely outlives its 60s grace; every earlier gate passed, so this method decided alone, returned true, released the slot and then unregistered the planner's own registry paths. It now also refuses when `activeSessionRegistry.pathsForTask(taskId)` is non-empty. Being a chokepoint, this covers all three callers (`reapLeakedConcurrencySlots`, `recoverPausedAbortFailures`, and the `preserveWorktrees` reclaim). The 60s grace is deliberately unchanged — a longer timeout would only make the bug rarer.
|
||||
@@ -0,0 +1,364 @@
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { TaskExecutor } from "../executor.js";
|
||||
import { activeSessionRegistry } from "../active-session-registry.js";
|
||||
import { SelfHealingManager } from "../self-healing.js";
|
||||
import { PAUSE_ABORT_PARK_ERROR_MARKER, PAUSE_ABORT_PARK_OPERATOR_MARKER } from "../self-healing-constants.js";
|
||||
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-02:10 (FN-6756 — planner worktrees reaped from under live planners):
|
||||
REGRESSION SUITE for a bug users hit: worktrees reaped while a planner was still
|
||||
working in them.
|
||||
|
||||
MECHANISM. Under plan-in-place, specification runs while the card sits in
|
||||
`todo`/`triage`. `reapLeakedConcurrencySlots` treats both columns as reapable
|
||||
("a task waiting to run must not pin a worktree" — written before planning moved
|
||||
there), and every gate ahead of the last one passes for a planner:
|
||||
|
||||
- it IS a `listWorktreeHolders()` row: ensureTaskWorktreeForPlanning ->
|
||||
ensureGraphCustomNodeWorktree -> addActiveWorktree
|
||||
- `todo`/`triage` is a reapable column
|
||||
- it is NOT in the executor's `executing` set — a planner is triage-owned
|
||||
- planning routinely outlives the 60s LEAKED_WORKTREE_SLOT_GRACE_MS
|
||||
|
||||
...leaving `clearPhantomExecutorBinding` deciding alone. It computed liveness from
|
||||
four TaskExecutor-owned sets only, so a triage planning session — which lives in
|
||||
TriageProcessor's OWN activeSessions map and registers in the module-level
|
||||
activeSessionRegistry — matched none of them. It returned true, released the slot,
|
||||
and then unregistered the planner's registry paths: destroying the evidence that
|
||||
proved the planner alive.
|
||||
|
||||
This is FN-8600 recurring through a second sweep. That fix registered planning
|
||||
paths in the registry and taught the self-owned-branch reclaim sweep to consult
|
||||
`isPathActive`. The leaked-slot reaper never got the same signal — fixed at one
|
||||
surface, not enumerated across all.
|
||||
|
||||
The tests below assert the invariant at BOTH levels, because either alone is
|
||||
insufficient: the unit case pins the guard, and the sweep case pins that the guard
|
||||
is actually reached and honored by the reaper.
|
||||
*/
|
||||
|
||||
function makeExecutorWithHeldWorktree(taskId: string, worktreePath: string): TaskExecutor {
|
||||
const executor = Object.create(TaskExecutor.prototype) as TaskExecutor;
|
||||
const priv = executor as unknown as Record<string, unknown>;
|
||||
// Exactly the surfaces the guard consults, all EMPTY — the true state during
|
||||
// planning, since the planner's session is held by TriageProcessor.
|
||||
priv.activeSessions = new Map();
|
||||
priv.activeStepExecutors = new Map();
|
||||
priv.activeWorkflowStepSessions = new Map();
|
||||
priv.activeCliTaskSessions = new Map();
|
||||
priv.activeWorktrees = new Map([[taskId, new Set([worktreePath])]]);
|
||||
priv.executing = new Set();
|
||||
priv.recoveringCompleted = new Set();
|
||||
priv.resumingUnpaused = new Set();
|
||||
priv.approvalSuspended = new Set();
|
||||
priv.approvalResumeAfterUnwind = new Set();
|
||||
priv.effectiveColumnAgentByTask = new Map();
|
||||
return executor;
|
||||
}
|
||||
|
||||
const PLANNER_TASK = "FN-6756-PLANNER";
|
||||
const PLANNER_WORKTREE = "/tmp/fn-6756-planner-worktree";
|
||||
|
||||
afterEach(() => {
|
||||
activeSessionRegistry.clear();
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
describe("FN-6756: a live planner's worktree survives the leaked-slot reaper", () => {
|
||||
/*
|
||||
Reverting the `registeredSessionPaths.length > 0` term in
|
||||
clearPhantomExecutorBinding turns this red: the method returns true and, worse,
|
||||
unregisters the planner's path on its way out.
|
||||
*/
|
||||
it("clearPhantomExecutorBinding refuses when the task holds a registered session path", () => {
|
||||
const executor = makeExecutorWithHeldWorktree(PLANNER_TASK, PLANNER_WORKTREE);
|
||||
activeSessionRegistry.registerPath(PLANNER_WORKTREE, {
|
||||
taskId: PLANNER_TASK,
|
||||
kind: "planning",
|
||||
ownerKey: "triage:plan",
|
||||
});
|
||||
|
||||
expect(executor.clearPhantomExecutorBinding(PLANNER_TASK)).toBe(false);
|
||||
|
||||
// The binding and the registration must both survive the refusal — a refusal
|
||||
// that still tore down state would be worse than none.
|
||||
expect(activeSessionRegistry.isPathActive(PLANNER_WORKTREE)).toBe(true);
|
||||
expect(
|
||||
(executor as unknown as { activeWorktrees: Map<string, Set<string>> }).activeWorktrees.get(PLANNER_TASK),
|
||||
).toEqual(new Set([PLANNER_WORKTREE]));
|
||||
});
|
||||
|
||||
/*
|
||||
The kind is deliberately not part of the guard: any registered surface means
|
||||
someone is working in that worktree. Pinning one representative non-executor kind
|
||||
keeps a future "only refuse for kind === planning" narrowing honest.
|
||||
*/
|
||||
it("refuses for any registered session kind, not just planning", () => {
|
||||
for (const kind of ["planning", "ai-merge", "step-session"] as const) {
|
||||
activeSessionRegistry.clear();
|
||||
const executor = makeExecutorWithHeldWorktree(PLANNER_TASK, PLANNER_WORKTREE);
|
||||
activeSessionRegistry.registerPath(PLANNER_WORKTREE, {
|
||||
taskId: PLANNER_TASK,
|
||||
kind,
|
||||
ownerKey: `owner:${kind}`,
|
||||
});
|
||||
expect(executor.clearPhantomExecutorBinding(PLANNER_TASK), `kind=${kind}`).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
/*
|
||||
The guard must NOT become a blanket refusal: a genuinely phantom binding — no
|
||||
executor surface AND no registration — is exactly what FN-6736's reaper exists to
|
||||
clear, and blocking it would trade this bug for a wedged queue.
|
||||
*/
|
||||
it("still clears a genuine phantom binding, and actually removes the binding", () => {
|
||||
const phantomTask = "FN-6756-PHANTOM";
|
||||
const phantomWorktree = "/tmp/fn-6756-phantom-worktree";
|
||||
const executor = makeExecutorWithHeldWorktree(phantomTask, phantomWorktree);
|
||||
const priv = executor as unknown as { activeWorktrees: Map<string, Set<string>>; executing: Set<string> };
|
||||
priv.executing.add(phantomTask);
|
||||
expect(activeSessionRegistry.pathsForTask(phantomTask)).toEqual([]);
|
||||
|
||||
expect(executor.clearPhantomExecutorBinding(phantomTask)).toBe(true);
|
||||
|
||||
/*
|
||||
Asserting the RETURN VALUE alone would pass a cleanup that reports success
|
||||
without doing anything — the exact shape of defect this suite exists to catch
|
||||
on the other side. Pin the observable effect too.
|
||||
*/
|
||||
expect(priv.activeWorktrees.has(phantomTask)).toBe(false);
|
||||
expect(priv.executing.has(phantomTask)).toBe(false);
|
||||
});
|
||||
|
||||
/*
|
||||
END TO END through the sweep itself. The unit case above proves the guard; this
|
||||
proves the reaper reaches and honors it for a card in a reapable column, past the
|
||||
grace, with the executor's sets empty — i.e. the exact reported shape.
|
||||
*/
|
||||
/*
|
||||
BOTH reaper surfaces, not just the reported one. `reapLeakedConcurrencySlots`
|
||||
treats `todo` AND `triage` as reapable, and plan-in-place puts specification in
|
||||
`todo` while Coding (Ideas) intake sits in `triage` — so a repro-only test would
|
||||
leave half the exposed surface unguarded. AGENTS.md Surface Enumeration: the
|
||||
regression test asserts the invariant across ALL known surfaces, not the single
|
||||
reported reproduction.
|
||||
*/
|
||||
it.each(["todo", "triage"] as const)(
|
||||
"reapLeakedConcurrencySlots does not reap a planning card in %s past the grace",
|
||||
async (column) => {
|
||||
const executor = makeExecutorWithHeldWorktree(PLANNER_TASK, PLANNER_WORKTREE);
|
||||
activeSessionRegistry.registerPath(PLANNER_WORKTREE, {
|
||||
taskId: PLANNER_TASK,
|
||||
kind: "planning",
|
||||
ownerKey: "triage:plan",
|
||||
});
|
||||
|
||||
// Entered the column well past LEAKED_WORKTREE_SLOT_GRACE_MS (60s).
|
||||
const staleEntry = new Date(Date.now() - 10 * 60_000).toISOString();
|
||||
const store = {
|
||||
getSettings: vi.fn(async () => ({ globalPause: false, enginePaused: false })),
|
||||
getTask: vi.fn(async () => ({
|
||||
id: PLANNER_TASK,
|
||||
column,
|
||||
status: "planning",
|
||||
columnMovedAt: staleEntry,
|
||||
updatedAt: staleEntry,
|
||||
})),
|
||||
logEntry: vi.fn(async () => undefined),
|
||||
};
|
||||
|
||||
const manager = Object.create(SelfHealingManager.prototype) as SelfHealingManager;
|
||||
(manager as unknown as Record<string, unknown>).store = store;
|
||||
(manager as unknown as Record<string, unknown>).options = {
|
||||
listWorktreeHolders: () => [{ taskId: PLANNER_TASK, worktreePath: PLANNER_WORKTREE }],
|
||||
getExecutingTaskIds: () => new Set<string>(),
|
||||
clearPhantomExecutorBinding: (taskId: string) => executor.clearPhantomExecutorBinding(taskId),
|
||||
};
|
||||
|
||||
const reaped = await manager.reapLeakedConcurrencySlots();
|
||||
|
||||
expect(reaped, `a live planner's slot must not be reaped from ${column}`).toBe(0);
|
||||
expect(activeSessionRegistry.isPathActive(PLANNER_WORKTREE)).toBe(true);
|
||||
// The binding must SURVIVE, not merely go unreported.
|
||||
expect(
|
||||
(executor as unknown as { activeWorktrees: Map<string, Set<string>> }).activeWorktrees.get(PLANNER_TASK),
|
||||
).toEqual(new Set([PLANNER_WORKTREE]));
|
||||
expect(store.logEntry).not.toHaveBeenCalled();
|
||||
},
|
||||
);
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-04:20 (FN-6756 — the SECOND door, PR #2531 review):
|
||||
greptile caught that the chokepoint fix was INCOMPLETE. Adding a refusal to
|
||||
`clearPhantomExecutorBinding` only protects callers that HONOR the return value.
|
||||
`recoverPausedAbortFailures` discarded it, so a live planner was still losing its
|
||||
worktree through pause-abort recovery — and that path moves the card to `todo`
|
||||
BEFORE releasing ownership, so the executor-only gate let it be requeued too.
|
||||
|
||||
This is the same failure that produced the original bug: FN-8600 was fixed at the
|
||||
reclaim sweep and never enumerated to the leaked-slot reaper; the leaked-slot
|
||||
reaper was then fixed and not enumerated to pause-abort. Hence a test per caller,
|
||||
not per report.
|
||||
*/
|
||||
it("recoverPausedAbortFailures defers while a live session path is registered", async () => {
|
||||
activeSessionRegistry.registerPath(PLANNER_WORKTREE, {
|
||||
taskId: PLANNER_TASK,
|
||||
kind: "planning",
|
||||
ownerKey: "triage:plan",
|
||||
});
|
||||
|
||||
const parkedTask = {
|
||||
id: PLANNER_TASK,
|
||||
column: "todo",
|
||||
status: "failed",
|
||||
error: `${PAUSE_ABORT_PARK_ERROR_MARKER} — ${PAUSE_ABORT_PARK_OPERATOR_MARKER}`,
|
||||
paused: false,
|
||||
userPaused: false,
|
||||
steps: [],
|
||||
};
|
||||
const store = {
|
||||
getSettings: vi.fn(async () => ({ globalPause: false, enginePaused: false })),
|
||||
listTasks: vi.fn(async () => [parkedTask]),
|
||||
getTask: vi.fn(async () => parkedTask),
|
||||
moveTask: vi.fn(async () => undefined),
|
||||
updateTask: vi.fn(async () => undefined),
|
||||
logEntry: vi.fn(async () => undefined),
|
||||
recordRunAuditEvent: vi.fn(async () => undefined),
|
||||
};
|
||||
|
||||
const executorProbe = makeExecutorWithHeldWorktree(PLANNER_TASK, PLANNER_WORKTREE);
|
||||
const clearPhantomExecutorBinding = vi.fn(() => true);
|
||||
const manager = Object.create(SelfHealingManager.prototype) as SelfHealingManager;
|
||||
(manager as unknown as Record<string, unknown>).store = store;
|
||||
(manager as unknown as Record<string, unknown>).options = {
|
||||
getExecutingTaskIds: () => new Set<string>(),
|
||||
hasLiveSessionSurface: (id: string) => executorProbe.hasLiveSessionSurface(id),
|
||||
clearPhantomExecutorBinding,
|
||||
};
|
||||
|
||||
const recovered = await manager.recoverPausedAbortFailures();
|
||||
|
||||
expect(recovered, "a card with a live planner must not be recovered").toBe(0);
|
||||
// Neither the backward move nor the ownership release may happen.
|
||||
expect(store.moveTask).not.toHaveBeenCalled();
|
||||
expect(clearPhantomExecutorBinding).not.toHaveBeenCalled();
|
||||
expect(activeSessionRegistry.isPathActive(PLANNER_WORKTREE)).toBe(true);
|
||||
});
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-05:10 (FN-6756 — the reporting half, PR #2531 review):
|
||||
A refused release must not merely fail to clear — it must not NARRATE success.
|
||||
|
||||
The original defect had two halves and the second is why nobody caught it from
|
||||
logs: after ignoring the refusal, the path still wrote "Auto-recovered: pause-abort
|
||||
park cleared", emitted `task:auto-recover-paused-abort-park`, and incremented the
|
||||
recovered counter. An operator reading the task log saw a clean recovery at the
|
||||
exact moment their planner lost its worktree.
|
||||
|
||||
This drives the refusal through the executor's REAL guard (not a stubbed boolean),
|
||||
with the registry pre-gate deliberately bypassed — the task has no registered path,
|
||||
but an executor session surface is live. That is the defense-in-depth branch, and
|
||||
it asserts the full no-op: no un-park, no move, no log, no audit, no count.
|
||||
*/
|
||||
it("a live executor session defers pause-abort recovery without logging, auditing or counting it", async () => {
|
||||
const taskId = "FN-6756-REFUSED";
|
||||
const executor = makeExecutorWithHeldWorktree(taskId, "/tmp/fn-6756-refused-worktree");
|
||||
// Live EXECUTOR session surface, and NO registry entry — so the registry
|
||||
// pre-gate lets this through and clearPhantomExecutorBinding itself refuses.
|
||||
(executor as unknown as { activeSessions: Map<string, unknown> }).activeSessions.set(taskId, { dispose() {} });
|
||||
expect(activeSessionRegistry.pathsForTask(taskId)).toEqual([]);
|
||||
|
||||
const parkedTask = {
|
||||
id: taskId,
|
||||
column: "in-progress",
|
||||
status: "failed",
|
||||
error: `${PAUSE_ABORT_PARK_ERROR_MARKER} — ${PAUSE_ABORT_PARK_OPERATOR_MARKER}`,
|
||||
paused: false,
|
||||
userPaused: false,
|
||||
steps: [],
|
||||
};
|
||||
const store = {
|
||||
getSettings: vi.fn(async () => ({ globalPause: false, enginePaused: false })),
|
||||
listTasks: vi.fn(async () => [parkedTask]),
|
||||
getTask: vi.fn(async () => parkedTask),
|
||||
moveTask: vi.fn(async () => undefined),
|
||||
updateTask: vi.fn(async () => undefined),
|
||||
logEntry: vi.fn(async () => undefined),
|
||||
recordRunAuditEvent: vi.fn(async () => undefined),
|
||||
};
|
||||
|
||||
const manager = Object.create(SelfHealingManager.prototype) as SelfHealingManager;
|
||||
(manager as unknown as Record<string, unknown>).store = store;
|
||||
(manager as unknown as Record<string, unknown>).options = {
|
||||
getExecutingTaskIds: () => new Set<string>(),
|
||||
hasLiveSessionSurface: (id: string) => executor.hasLiveSessionSurface(id),
|
||||
clearPhantomExecutorBinding: (id: string) => executor.clearPhantomExecutorBinding(id),
|
||||
};
|
||||
|
||||
const recovered = await manager.recoverPausedAbortFailures();
|
||||
|
||||
expect(recovered, "a refused release must not be counted as a recovery").toBe(0);
|
||||
expect(store.updateTask, "the park must not be cleared").not.toHaveBeenCalled();
|
||||
expect(store.moveTask, "the card must not be requeued").not.toHaveBeenCalled();
|
||||
expect(store.logEntry, "no 'Auto-recovered' entry may be written").not.toHaveBeenCalled();
|
||||
expect(store.recordRunAuditEvent, "no recovery audit may be emitted").not.toHaveBeenCalled();
|
||||
// ...and the worktree the live session is using survives.
|
||||
expect(
|
||||
(executor as unknown as { activeWorktrees: Map<string, Set<string>> }).activeWorktrees.has(taskId),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-06:05 (FN-6756 — the torn write, PR #2531 review):
|
||||
ORDERING. greptile caught that my first correction traded one fault for another:
|
||||
hoisting the release ABOVE the fallible writes meant an `updateTask`/`moveTask`
|
||||
rejection landed AFTER ownership had already been given up — the task un-repaired,
|
||||
the slot released, and nothing owning the repair. Same shape U12 hit on re-home:
|
||||
irreversible step committed before the fallible step.
|
||||
|
||||
The release now runs LAST. This drives a write failure and asserts ownership is
|
||||
still held, so the next sweep can retry against intact state.
|
||||
*/
|
||||
it("keeps worktree ownership when a recovery write fails", async () => {
|
||||
const taskId = "FN-6756-TORN";
|
||||
const worktree = "/tmp/fn-6756-torn-worktree";
|
||||
const executor = makeExecutorWithHeldWorktree(taskId, worktree);
|
||||
|
||||
const parkedTask = {
|
||||
id: taskId,
|
||||
column: "in-progress",
|
||||
status: "failed",
|
||||
error: `${PAUSE_ABORT_PARK_ERROR_MARKER} — ${PAUSE_ABORT_PARK_OPERATOR_MARKER}`,
|
||||
paused: false,
|
||||
userPaused: false,
|
||||
steps: [],
|
||||
};
|
||||
const store = {
|
||||
getSettings: vi.fn(async () => ({ globalPause: false, enginePaused: false })),
|
||||
listTasks: vi.fn(async () => [parkedTask]),
|
||||
getTask: vi.fn(async () => parkedTask),
|
||||
// The fallible write rejects, exactly as a store conflict would.
|
||||
updateTask: vi.fn(async () => { throw new Error("store rejected the un-park"); }),
|
||||
moveTask: vi.fn(async () => undefined),
|
||||
logEntry: vi.fn(async () => undefined),
|
||||
recordRunAuditEvent: vi.fn(async () => undefined),
|
||||
};
|
||||
|
||||
const clearPhantomExecutorBinding = vi.fn((id: string) => executor.clearPhantomExecutorBinding(id));
|
||||
const manager = Object.create(SelfHealingManager.prototype) as SelfHealingManager;
|
||||
(manager as unknown as Record<string, unknown>).store = store;
|
||||
(manager as unknown as Record<string, unknown>).options = {
|
||||
getExecutingTaskIds: () => new Set<string>(),
|
||||
hasLiveSessionSurface: (id: string) => executor.hasLiveSessionSurface(id),
|
||||
clearPhantomExecutorBinding,
|
||||
};
|
||||
|
||||
const recovered = await manager.recoverPausedAbortFailures();
|
||||
|
||||
expect(recovered, "a failed write must not count as a recovery").toBe(0);
|
||||
expect(clearPhantomExecutorBinding, "ownership must not be released before the writes commit").not.toHaveBeenCalled();
|
||||
expect(
|
||||
(executor as unknown as { activeWorktrees: Map<string, Set<string>> }).activeWorktrees.has(taskId),
|
||||
"ownership must survive so the next sweep retries against intact state",
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -2699,12 +2699,71 @@ export class TaskExecutor {
|
||||
* FNXC:ExecutorBinding 2026-06-30-00:00:
|
||||
* `preserveWorktrees: true` is the FN-6736 self-healing path. When the caller has already committed to `moveTask(..., { preserveWorktree: true })`, unregistering the held worktree path from `activeSessionRegistry` defeats the preserve: re-dispatch then sees the path as free and re-acquires a brand-new worktree (observed on FN-7249: gentle-peach orphaned, rosy-thorn rebuilt ~20s after reclaim). The preserve variant clears only the in-memory executor/lock bookkeeping and leaves the session-registry path entry intact so the re-dispatch reattaches to the same worktree. Non-self-healing callers (leaked-slot reaper, pause-abort recovery) keep the default full-clear behavior.
|
||||
*/
|
||||
clearPhantomExecutorBinding(taskId: string, options: { preserveWorktrees?: boolean } = {}): boolean {
|
||||
const hasLiveSessionSurface = this.activeSessions.has(taskId)
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-06:05 (FN-6756 — one liveness predicate, PR #2531 review):
|
||||
READ-ONLY liveness probe, extracted so callers can ASK before they mutate.
|
||||
|
||||
`clearPhantomExecutorBinding` both answers "is this live?" and performs a
|
||||
destructive release, which forced every caller into a false choice: check first
|
||||
and release ownership before their own fallible writes (a torn write — ownership
|
||||
gone, task un-repaired, nobody owning the repair), or write first and discover the
|
||||
refusal too late. Splitting the question from the act lets a caller gate on
|
||||
liveness with no side effect and release only after its writes have committed.
|
||||
|
||||
Deliberately the SAME expression the destructive path uses, not a copy: a probe
|
||||
that could disagree with the guard it stands in for is worse than no probe, and
|
||||
independent re-derivation of "liveness" at each call site is precisely how this
|
||||
bug reached users three times (reclaim sweep -> leaked-slot reaper -> pause-abort).
|
||||
|
||||
Registry paths count. A triage PLANNING session is owned by TriageProcessor and
|
||||
appears in NONE of the four executor-owned maps; it registers here instead.
|
||||
*/
|
||||
hasLiveSessionSurface(taskId: string): boolean {
|
||||
return this.activeSessions.has(taskId)
|
||||
|| this.activeStepExecutors.has(taskId)
|
||||
|| this.activeWorkflowStepSessions.has(taskId)
|
||||
|| this.activeCliTaskSessions.has(taskId);
|
||||
if (hasLiveSessionSurface) {
|
||||
|| this.activeCliTaskSessions.has(taskId)
|
||||
|| activeSessionRegistry.pathsForTask(taskId).length > 0;
|
||||
}
|
||||
|
||||
clearPhantomExecutorBinding(taskId: string, options: { preserveWorktrees?: boolean } = {}): boolean {
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-02:10 (FN-6756 — planner worktrees reaped from under live planners):
|
||||
THE REGISTRY IS PART OF THE LIVENESS SIGNAL, not just something this method
|
||||
tears down.
|
||||
|
||||
This is documented as "the last line of defense against pulling a worktree out
|
||||
from under a running agent" (see `reapLeakedConcurrencySlots`). It was blind to
|
||||
an entire class of agent. The four sets below are all TaskExecutor-owned; a
|
||||
triage PLANNING session is owned by `TriageProcessor` and lives in ITS OWN
|
||||
`activeSessions` map, so a live planner matched none of them.
|
||||
|
||||
The consequence was not theoretical — it is FN-8600 recurring through a second
|
||||
door. Under plan-in-place a card is specified while it sits in `todo`/`triage`,
|
||||
both of which `reapLeakedConcurrencySlots` treats as reapable, and planning
|
||||
routinely outlives that sweep's 60s grace. Every earlier gate passes for a
|
||||
planner (not in the executor's `executing` set, reapable column, past grace), so
|
||||
this method decided alone — and returned true, releasing the slot and then
|
||||
UNREGISTERING the planner's own registry paths below. It destroyed the very
|
||||
evidence that proves the planner alive.
|
||||
|
||||
FN-8600 fixed the self-owned-branch reclaim sweep by registering planning paths
|
||||
here (`triage.ts` acquireActiveSessionPath, and see the "planning" kind note in
|
||||
active-session-registry.ts). That fix landed at ONE surface. This is the second,
|
||||
which is what the AGENTS.md Surface Enumeration rule exists to prevent.
|
||||
|
||||
Deliberately keyed on ANY registered path for the task, not on kind: the point
|
||||
is that a registered session surface of any kind means someone is working in
|
||||
that worktree. A leaked entry now blocks THIS sweep rather than a live planner
|
||||
losing its worktree — the strictly safer failure, and the one the "last line of
|
||||
defense" wording already promises. The registry is process-local and in-memory,
|
||||
so a leak cannot outlive the process; stale entries have their own reconciler
|
||||
(`reconcileStaleSelfOwned`) and the reclaim-aware `acquireActiveSessionPath`.
|
||||
|
||||
NOT fixed by raising the grace period: a longer timeout only makes this rarer
|
||||
and harder to reproduce. The liveness gate is the bug.
|
||||
*/
|
||||
if (this.hasLiveSessionSurface(taskId)) {
|
||||
executorLog.warn(`${taskId}: refusing to clear phantom executor binding because a live session surface is still registered`);
|
||||
return false;
|
||||
}
|
||||
|
||||
@@ -1444,6 +1444,15 @@ export class InProcessRuntime
|
||||
recoverFailedPreMergeStep: (task) => this.executor.recoverFailedPreMergeWorkflowStep(task),
|
||||
getExecutingTaskIds: () => this.executor?.getExecutingTaskIds() ?? new Set<string>(),
|
||||
clearPhantomExecutorBinding: (taskId: string, options?: { preserveWorktrees?: boolean }) => this.executor?.clearPhantomExecutorBinding(taskId, options),
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-06:05 (FN-6756):
|
||||
Wire the read-only liveness probe. self-healing.ts's own comment records that
|
||||
`releaseExecutorWorktreeOwnership` was a declared-but-never-wired option that
|
||||
silently no-opped; an unwired probe here would be worse — `?.() === true` is
|
||||
false when unwired, so every sweep gating on it would silently stop deferring
|
||||
for live sessions and the FN-6756 fix would evaporate without a test failing.
|
||||
*/
|
||||
hasLiveSessionSurface: (taskId: string) => this.executor?.hasLiveSessionSurface(taskId) ?? false,
|
||||
listWorktreeHolders: () => this.executor?.listWorktreeHolders() ?? [],
|
||||
// FNXC:PlanningEvacuation 2026-07-25-23:00: the executor owns the release safety conditions.
|
||||
releasePreExecutionWorktree: (taskId, reason) =>
|
||||
|
||||
@@ -297,6 +297,14 @@ export interface SelfHealingOptions {
|
||||
*/
|
||||
clearPhantomExecutorBinding?: (taskId: string, options?: { preserveWorktrees?: boolean }) => boolean | void;
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-06:05 (FN-6756):
|
||||
READ-ONLY liveness probe. Lets a sweep gate on "is an agent working this task?"
|
||||
WITHOUT the destructive release, so it can refuse before mutating and still hold
|
||||
ownership until its own writes commit. Same expression as
|
||||
clearPhantomExecutorBinding's refusal, exposed rather than re-derived.
|
||||
*/
|
||||
hasLiveSessionSurface?: (taskId: string) => boolean;
|
||||
/*
|
||||
FNXC:PlanningEvacuation 2026-07-25-23:00:
|
||||
Releases a task's PRE-EXECUTION worktree (acquired at planning time) when the card is parked
|
||||
without ever executing. The executor owns the safety conditions — never executed, no live session,
|
||||
@@ -10420,6 +10428,24 @@ export class SelfHealingManager extends SelfHealingGitEvidence {
|
||||
const fresh = await this.store.getTask(task.id);
|
||||
const latestExecutingIds = this.options.getExecutingTaskIds?.() ?? new Set<string>();
|
||||
if (!fresh) continue;
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-06:05 (FN-6756 — gate before mutating, PR #2531 review):
|
||||
`latestExecutingIds` is TaskExecutor-owned and blind to a triage PLANNING
|
||||
session, exactly as clearPhantomExecutorBinding's four session maps were.
|
||||
This sweep does not merely clear a binding — it un-parks the row, requeues
|
||||
the card and releases worktree ownership, so an executor-only gate let a
|
||||
live planner be requeued and stripped of its worktree through a second
|
||||
door after the leaked-slot reaper's was closed.
|
||||
|
||||
Gate on the SHARED read-only probe here, before any mutation, so a live
|
||||
session defers the whole recovery to a later sweep with nothing written,
|
||||
nothing logged and nothing counted. The destructive release stays BELOW the
|
||||
fallible writes — see the ordering note there.
|
||||
*/
|
||||
if (this.options.hasLiveSessionSurface?.(fresh.id) === true) {
|
||||
log.debug(`[self-healing] deferring pause-abort recovery for ${fresh.id}: a live session surface is registered`);
|
||||
continue;
|
||||
}
|
||||
const route = this.classifyPausedAbortWorkflowRecovery(fresh, settings, latestExecutingIds.has(fresh.id));
|
||||
if (route.kind === "no-action") {
|
||||
continue;
|
||||
@@ -10457,12 +10483,34 @@ export class SelfHealingManager extends SelfHealingGitEvidence {
|
||||
});
|
||||
await this.store.updateTask(task.id, { workflowTransitionNotification });
|
||||
}
|
||||
// Release any in-memory worktree ownership the leaked park may still
|
||||
// pin, so the requeued task does not re-block the concurrency gate.
|
||||
// FNXC:WorkflowLifecycle 2026-06-20-00:00: use clearPhantomExecutorBinding
|
||||
// (wired + live-session-refusal guarded), NOT releaseExecutorWorktreeOwnership
|
||||
// which is a declared-but-never-wired option — it would silently no-op.
|
||||
this.options.clearPhantomExecutorBinding?.(task.id);
|
||||
/*
|
||||
FNXC:NodeWorktreeIsolation 2026-07-29-06:05 (FN-6756 — ordering, PR #2531 review):
|
||||
Release worktree ownership only AFTER the fallible writes have committed.
|
||||
|
||||
This sat here originally with its return DISCARDED, which let a refusal be
|
||||
followed by "Auto-recovered…", the audit and `recovered++` — reporting
|
||||
success while pulling a worktree from under a live planner, which is why it
|
||||
went unnoticed. My first correction hoisted the release ABOVE the writes so
|
||||
a refusal could abort cleanly, and that traded one fault for another: an
|
||||
`updateTask`/`moveTask` rejection after a SUCCESSFUL release left ownership
|
||||
given up with the task un-repaired and nothing owning the repair — the same
|
||||
torn-write shape U12 hit on re-home, irreversible step before fallible step.
|
||||
|
||||
Both faults are fixed by splitting the question from the act: liveness is
|
||||
gated ABOVE via the read-only probe (so a live session never reaches these
|
||||
writes), and the irreversible release runs LAST, once the writes it depends
|
||||
on have landed. A throw before this point leaves ownership intact and the
|
||||
park in place for the next sweep.
|
||||
|
||||
The refusal is still honored as defense-in-depth: reaching it means a
|
||||
session started between the probe and here, so ownership stays with that
|
||||
session. The un-park and requeue genuinely happened, so the recovery is
|
||||
still counted — the warning records only that the worktree was not released.
|
||||
*/
|
||||
const phantomReleased = this.options.clearPhantomExecutorBinding?.(task.id);
|
||||
if (phantomReleased === false) {
|
||||
log.warn(`[self-healing] pause-abort recovery for ${task.id}: worktree ownership retained — a session started before the release`);
|
||||
}
|
||||
|
||||
await this.store.logEntry(
|
||||
task.id,
|
||||
|
||||
Reference in New Issue
Block a user