diff --git a/.changeset/fix-planner-worktree-reap.md b/.changeset/fix-planner-worktree-reap.md new file mode 100644 index 0000000000..998b2451f4 --- /dev/null +++ b/.changeset/fix-planner-worktree-reap.md @@ -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. diff --git a/packages/engine/src/__tests__/leaked-slot-reaper-planner-liveness.test.ts b/packages/engine/src/__tests__/leaked-slot-reaper-planner-liveness.test.ts new file mode 100644 index 0000000000..94a49ecb81 --- /dev/null +++ b/packages/engine/src/__tests__/leaked-slot-reaper-planner-liveness.test.ts @@ -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; + // 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> }).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>; executing: Set }; + 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).store = store; + (manager as unknown as Record).options = { + listWorktreeHolders: () => [{ taskId: PLANNER_TASK, worktreePath: PLANNER_WORKTREE }], + getExecutingTaskIds: () => new Set(), + 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> }).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).store = store; + (manager as unknown as Record).options = { + getExecutingTaskIds: () => new Set(), + 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 }).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).store = store; + (manager as unknown as Record).options = { + getExecutingTaskIds: () => new Set(), + 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> }).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).store = store; + (manager as unknown as Record).options = { + getExecutingTaskIds: () => new Set(), + 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> }).activeWorktrees.has(taskId), + "ownership must survive so the next sweep retries against intact state", + ).toBe(true); + }); +}); \ No newline at end of file diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 96c84b4e34..b41579a7ea 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -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; } diff --git a/packages/engine/src/runtimes/in-process-runtime.ts b/packages/engine/src/runtimes/in-process-runtime.ts index 10515e39f2..cf22183322 100644 --- a/packages/engine/src/runtimes/in-process-runtime.ts +++ b/packages/engine/src/runtimes/in-process-runtime.ts @@ -1444,6 +1444,15 @@ export class InProcessRuntime recoverFailedPreMergeStep: (task) => this.executor.recoverFailedPreMergeWorkflowStep(task), getExecutingTaskIds: () => this.executor?.getExecutingTaskIds() ?? new Set(), 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) => diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 6c408cdf5d..326698b780 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -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(); 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,