From 964356387462c2dc53624dae404958ab5d1e005c Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 19 Jun 2026 19:50:46 -0700 Subject: [PATCH 1/5] Fix pause-abort worktree leak + retry storm; add auto-recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A global pause/resume cycle parked tasks that had re-queued to todo as status:"failed" ("operator action required") and leaked their in-memory worktree slot. The scheduler kept re-dispatching the todo task, the genuine-pause-abort branch re-fired on the still-set pausedAborted marker, and it re-parked instantly with no backoff — a retry storm (75x/hr) that pinned maxWorktrees=3/3 and concurrency-starved the whole queue. - R1+R2 (executor.ts handleGraphFailure): treat a pause-abort that left a task in `todo` as benign (FN-6782) — don't park failed, clear the pausedAborted marker so the next dispatch is clean, and release the leaked activeWorktrees slot. Operator-action failure preserved for genuinely stranded non-todo columns (FN-6478). - A1 (self-healing.ts recoverPausedAbortFailures): new maintenance sweep that auto-recovers any pause-abort park still on the board and requeues it (status:null = schedulable) so the board self-heals. - run-audit.ts: new mutation types for the recovery telemetry. Corrected the spec's null-vs-queued assumption: the scheduler dispatch set is column==="todo" && !paused (scheduler.ts:1288); status:"queued" is the *blocked* marker, status:null is runnable — so recovered tasks are left null. Deferred (documented): A2 leaked-slot reaper needs a new executor listWorktreeHolders introspection API to reap in-memory worktree slots safely; R1 closes the observed leak at its source. Tests: self-healing-paused-abort-recovery.test.ts (3), executor-paused-abort-todo-benign.test.ts (2). Engine typecheck clean; 106 existing pause/graph-failure/limbo tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/fix-pause-abort-leak-storm.md | 8 ++ .../executor-paused-abort-todo-benign.test.ts | 98 +++++++++++++ ...self-healing-paused-abort-recovery.test.ts | 132 ++++++++++++++++++ packages/engine/src/executor.ts | 28 ++++ packages/engine/src/run-audit.ts | 4 + packages/engine/src/self-healing.ts | 93 ++++++++++++ 6 files changed, 363 insertions(+) create mode 100644 .changeset/fix-pause-abort-leak-storm.md create mode 100644 packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts create mode 100644 packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts diff --git a/.changeset/fix-pause-abort-leak-storm.md b/.changeset/fix-pause-abort-leak-storm.md new file mode 100644 index 0000000000..3d66855ff3 --- /dev/null +++ b/.changeset/fix-pause-abort-leak-storm.md @@ -0,0 +1,8 @@ +--- +"@runfusion/fusion": patch +--- + +Fix the global pause/resume failure mode that stalled the board: a pause-abort that left a task back in `todo` was parked `status:"failed"` ("operator action required") and leaked its in-memory worktree slot, producing an instant re-fail retry storm and concurrency-starving the whole queue. + +- Root cause: `handleGraphFailure` now treats a pause-abort that has re-queued a task to `todo` as benign (FN-6782) — it no longer parks it failed, clears the `pausedAborted` marker so the next dispatch starts clean, and releases the leaked worktree slot. +- Auto-recovery: a new `recoverPausedAbortFailures` self-healing sweep clears any pause-abort park (`status:"failed"` with "operator action required") still on the board and requeues it for normal scheduling, so the board self-heals without operator intervention. diff --git a/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts b/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts new file mode 100644 index 0000000000..9c5d55ec1b --- /dev/null +++ b/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts @@ -0,0 +1,98 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import "./executor-test-helpers.js"; +import { TaskExecutor } from "../executor.js"; +import { createMockStore, resetExecutorMocks } from "./executor-test-helpers.js"; +import type { TaskDetail } from "@fusion/core"; + +const now = "2026-06-20T00:00:00.000Z"; + +function makeTask(overrides: Partial = {}): TaskDetail { + return { + id: "FN-6782-T", + title: "pause-abort benign todo repro", + description: "Reproduces FN-6782 benign requeue-to-todo classification", + column: "todo", + dependencies: [], + steps: [{ name: "Implement", status: "pending" }], + currentStep: 0, + log: [], + branch: null, + baseBranch: "main", + worktree: "/tmp/fusion-fn-6782-t", + status: null, + error: null, + paused: false, + userPaused: false, + autoMerge: true, + mergeRetries: 0, + createdAt: now, + updatedAt: now, + ...overrides, + } as TaskDetail; +} + +function makeHarness(taskOverrides: Partial = {}) { + const store = createMockStore(); + const task = makeTask(taskOverrides); + store.getTask.mockResolvedValue(task); + store.getSettings.mockResolvedValue({ + maxConcurrent: 2, + maxWorktrees: 4, + pollIntervalMs: 15000, + autoMerge: true, + maxAutoMergeRetries: 3, + }); + const executor = new TaskExecutor(store, "/tmp/test", {}); + (executor as any).markPausedAborted(task.id, "hard-cancel"); + return { store, task, executor }; +} + +async function invokeGraphFailure(executor: TaskExecutor, task: TaskDetail) { + await (executor as any).handleGraphFailure(task, { + disposition: "failed", + outcome: "failure", + visitedNodeIds: ["plan", "execute"], + context: {}, + }); +} + +function logText(store: ReturnType): string { + return store.logEntry.mock.calls.map((call: unknown[]) => call[1]).join("\n"); +} + +describe("pause-abort benign requeue-to-todo (FN-6782)", () => { + beforeEach(() => { + resetExecutorMocks(); + }); + + it("does NOT park a todo-column pause-abort as failed (no retry storm)", async () => { + const { store, task, executor } = makeHarness({ column: "todo" }); + (executor as any).activeWorktrees.set(task.id, task.worktree); + + await invokeGraphFailure(executor, task); + + // Must NOT write status:"failed" — that was the storm trigger. + const parkedFailed = store.updateTask.mock.calls.some( + (call: unknown[]) => (call[1] as { status?: string } | undefined)?.status === "failed", + ); + expect(parkedFailed).toBe(false); + // Benign log surfaced. + expect(logText(store)).toContain("benign, cleared for normal scheduling"); + // pausedAborted marker cleared so the next dispatch starts clean. + expect((executor as any).pausedAborted.has(task.id)).toBe(false); + // Leaked worktree slot released. + expect((executor as any).activeWorktrees.has(task.id)).toBe(false); + }); + + it("STILL parks a non-todo (in-review) pause-abort as operator-action failed", async () => { + const { store, task, executor } = makeHarness({ column: "in-review" }); + + await invokeGraphFailure(executor, task); + + const parkedFailed = store.updateTask.mock.calls.some( + (call: unknown[]) => (call[1] as { status?: string } | undefined)?.status === "failed", + ); + expect(parkedFailed).toBe(true); + expect(logText(store)).toContain("operator action required"); + }); +}); diff --git a/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts b/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts new file mode 100644 index 0000000000..2946ee6fe8 --- /dev/null +++ b/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts @@ -0,0 +1,132 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { EventEmitter } from "node:events"; + +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, existsSync: vi.fn(actual.existsSync) }; +}); + +const { logger } = vi.hoisted(() => ({ logger: { log: vi.fn(), warn: vi.fn(), error: vi.fn() } })); +vi.mock("../logger.js", () => ({ + createLogger: vi.fn(() => logger), + schedulerLog: { log: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); + +vi.mock("../worktree-pool.js", () => ({ + WorktreePool: vi.fn(), + RemovalReason: {}, + scanIdleWorktrees: vi.fn().mockResolvedValue([]), + cleanupOrphanedWorktrees: vi.fn().mockResolvedValue(0), + isUsableTaskWorktree: vi.fn().mockResolvedValue(true), + removeWorktree: vi.fn().mockResolvedValue(undefined), + resolveWorktreeBackend: vi.fn(), +})); + +vi.mock("../merger.js", () => ({ classifyOwnedLandedEvidence: vi.fn() })); + +import { SelfHealingManager } from "../self-healing.js"; +import type { Settings, Task, TaskStore } from "@fusion/core"; + +const PARK_ERROR = + "Workflow graph failure surfaced after paused engine abort during pause/resume in 'todo' at node 'execute' — operator action required; retry or explicitly unpause/resume after inspecting the task"; + +function createMockStore(tasks: Task[]): TaskStore & EventEmitter { + const emitter = new EventEmitter(); + const byId = new Map(tasks.map((t) => [t.id, t])); + return Object.assign(emitter, { + getSettings: vi.fn().mockResolvedValue({ + autoMerge: true, + globalPause: false, + enginePaused: false, + maintenanceIntervalMs: 0, + } as unknown as Settings), + listTasks: vi.fn().mockResolvedValue(tasks), + getTask: vi.fn(async (id: string) => byId.get(id) ?? null), + updateTask: vi.fn().mockResolvedValue({} as Task), + logEntry: vi.fn().mockResolvedValue(undefined), + moveTask: vi.fn().mockResolvedValue(undefined), + recordRunAuditEvent: vi.fn().mockResolvedValue(undefined), + getRootDir: vi.fn().mockReturnValue("/tmp/test-project"), + }) as unknown as TaskStore & EventEmitter; +} + +function parkTask(overrides: Partial = {}): Task { + return { + id: "FN-7000", + column: "todo", + paused: false, + userPaused: false, + status: "failed", + error: PARK_ERROR, + steps: [{ status: "pending" }], + title: "parked task", + ...overrides, + } as unknown as Task; +} + +describe("recoverPausedAbortFailures", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-06-20T02:30:00.000Z")); + }); + afterEach(() => { + vi.useRealTimers(); + vi.clearAllMocks(); + }); + + it("clears a todo-column pause-abort park to schedulable (status:null) without moving it", async () => { + const store = createMockStore([parkTask({ id: "FN-7000", column: "todo" })]); + const release = vi.fn(); + const manager = new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + getExecutingTaskIds: () => new Set(), + releaseExecutorWorktreeOwnership: release, + }); + + const recovered = await manager.recoverPausedAbortFailures(); + + expect(recovered).toBe(1); + expect(store.updateTask).toHaveBeenCalledWith("FN-7000", { status: null, error: null }); + // Already in todo — must NOT be moved. + expect(store.moveTask).not.toHaveBeenCalled(); + expect(release).toHaveBeenCalledWith("FN-7000"); + expect(store.recordRunAuditEvent).toHaveBeenCalledWith( + expect.objectContaining({ mutationType: "task:auto-recover-paused-abort-park", target: "FN-7000" }), + ); + }); + + it("rehomes an in-progress pause-abort park back to todo", async () => { + const store = createMockStore([parkTask({ id: "FN-7001", column: "in-progress" })]); + const manager = new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + getExecutingTaskIds: () => new Set(), + }); + + const recovered = await manager.recoverPausedAbortFailures(); + + expect(recovered).toBe(1); + expect(store.moveTask).toHaveBeenCalledWith( + "FN-7001", + "todo", + { preserveProgress: true, moveSource: "engine", recoveryRehome: true }, + ); + }); + + it("skips paused, executing, in-review, and non-pause-abort failures", async () => { + const store = createMockStore([ + parkTask({ id: "FN-A", paused: true }), + parkTask({ id: "FN-B", column: "in-progress" }), // executing (below) + parkTask({ id: "FN-C", column: "in-review" }), // in-review park left for operator + parkTask({ id: "FN-D", error: "some other failure", status: "failed" }), + ]); + const manager = new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + getExecutingTaskIds: () => new Set(["FN-B"]), + }); + + const recovered = await manager.recoverPausedAbortFailures(); + + expect(recovered).toBe(0); + expect(store.updateTask).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 3358233f9f..5be0f7a3b3 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -6640,6 +6640,34 @@ export class TaskExecutor { ? "engine abort during pause/resume" : "task pause"; if (live.column !== "in-progress") { + // FN-6782: a pause/resume abort that has left the task back in `todo` + // is benign — the work is simply re-queued for a fresh dispatch, not + // stranded. Parking it `status: "failed"` (operator action required) + // here is what caused the retry storm: the scheduler re-dispatches the + // todo task, this branch re-fires on the still-set pausedAborted + // marker, and it re-parks instantly with no backoff. Treat `todo` like + // the in-progress benign case: clear the abort marker so the next + // dispatch starts clean, log, and return WITHOUT parking failed. The + // operator-action failure is preserved only for genuinely stranded + // non-todo columns (e.g. in-review), per FN-6478. + if (live.column === "todo") { + this.clearPausedAborted(task.id); + // FN-6782 leak fix: a task parked back to `todo` must not keep + // pinning its in-memory worktree slot. The execute() finally does + // not delete activeWorktrees on this early-return path, so without + // this release the slot leaks — a `todo` task stays a maxWorktrees + // holder and concurrency-blocks the whole queue (the FN-6756 + // "in todo yet still a holder, maxWorktrees=3/3" symptom). Mirror + // clearPhantomExecutorBinding's release semantics. Safe here: + // handleGraphFailure is terminal for this run (no seam re-entry), + // and the next dispatch re-acquires a fresh worktree. + this.activeWorktrees.delete(task.id); + const todoBenign = `Workflow graph run ended during ${pauseProvenance} with task re-queued to todo — benign, cleared for normal scheduling`; + executorLog.log(`${task.id}: ${todoBenign}`); + await this.store.logEntry(task.id, todoBenign, undefined, this.getRunContextFor(task.id)); + await this.persistTokenUsage(task.id); + return; + } const failedNode = result.visitedNodeIds[result.visitedNodeIds.length - 1] ?? "unknown"; const message = `Workflow graph failure surfaced after paused ${pauseProvenance} in '${live.column}' at node '${failedNode}' — operator action required; retry or explicitly unpause/resume after inspecting the task`; executorLog.warn(`${task.id}: ${message}`); diff --git a/packages/engine/src/run-audit.ts b/packages/engine/src/run-audit.ts index f43c202984..b6f8c5d43c 100644 --- a/packages/engine/src/run-audit.ts +++ b/packages/engine/src/run-audit.ts @@ -459,6 +459,10 @@ export type DatabaseMutationType = | "task:auto-recover-worktree-metadata-rebound" | "task:auto-recover-worktree-metadata-cleared" | "task:auto-recover-worktree-metadata-skipped-active" + // pause-abort auto-recovery (FN-6782): a global pause/resume park cleared and requeued + | "task:auto-recover-paused-abort-park" + // leaked concurrency-slot reaper: a worktree/lease/semaphore slot whose holder left in-progress + | "task:reap-leaked-concurrency-slot" // task:auto-archived-ghost-bug metadata: { findings: Array<{ construct: { kind: string; raw: string; filePath?: string; line?: number }; matched: boolean; probeError?: string; output?: string }>; reason: string } // task:auto-archived-duplicate metadata: { siblingTaskIds: string[]; scores: Record } | "task:auto-archived-ghost-bug" diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 457f96e9eb..64eebbafea 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -2111,6 +2111,7 @@ export class SelfHealingManager { { name: "recover-misclassified-failures", fn: () => this.recoverMisclassifiedFailures() }, { name: "recover-missing-worktree-review-failures", fn: () => this.recoverMissingWorktreeReviewFailures() }, { name: "recover-no-progress-no-task-done", fn: () => this.recoverNoProgressNoTaskDoneFailures() }, + { name: "recover-paused-abort-failures", fn: () => this.recoverPausedAbortFailures() }, { name: "recover-partial-progress-no-task-done", fn: () => this.recoverPartialProgressNoTaskDoneFailures() }, { name: "recover-orphaned-executions", fn: () => this.recoverOrphanedExecutions() }, { name: "recover-approved-triage", fn: () => this.recoverApprovedTriageTasks() }, @@ -7874,6 +7875,98 @@ export class SelfHealingManager { } } + /** + * FN-6782 / pause-abort auto-recovery: a global pause/resume cycle can leave a + * task parked `status: "failed"` with the "operator action required" message + * produced by the executor's genuine-pause-abort branch (executor.ts + * handleGraphFailure). Historically that required a human to retry/unpause. + * This sweep auto-recovers those parks: it clears the failed status/error and + * rehomes the task to `todo` with `status: null`. The scheduler treats a + * non-paused `todo` task with `status: null` as runnable (scheduler.ts builds + * its dispatch set from `column === "todo" && !paused`; `status: "queued"` is + * the *blocked* marker, not the runnable one), so clearing to null is what + * makes the task schedulable again. + * + * Guards mirror the other failed-task recoverers: never touch a task that is + * paused, currently executing, or whose error is not the pause-abort park. + */ + async recoverPausedAbortFailures(): Promise { + try { + const executingIds = this.options.getExecutingTaskIds?.() ?? new Set(); + const tasks = await this.store.listTasks({ slim: true }); + + const isPausedAbortPark = (t: Task): boolean => + t.status === "failed" && + typeof t.error === "string" && + t.error.includes("operator action required") && + t.error.includes("Workflow graph failure surfaced after paused"); + + const parked = tasks.filter((t) => + isPausedAbortPark(t) && + !t.paused && + !t.userPaused && + !executingIds.has(t.id) && + // Only recover columns that are safe to requeue. done/archived parks are + // terminal and in-review parks may carry merge state — leave those for + // the existing review recoverers / operator inspection. + (t.column === "todo" || t.column === "in-progress"), + ); + + if (parked.length === 0) return 0; + + log.warn(`Found ${parked.length} pause-abort park(s) requiring auto-recovery`); + + let recovered = 0; + for (const task of parked) { + try { + // Re-read to avoid acting on a stale snapshot after awaits. + const fresh = await this.store.getTask(task.id); + if (!fresh || !isPausedAbortPark(fresh) || fresh.paused || executingIds.has(fresh.id)) { + continue; + } + + await this.store.updateTask(task.id, { status: null, error: null }); + if (fresh.column !== "todo") { + await this.store.moveTask(task.id, "todo", { + preserveProgress: true, + moveSource: "engine", + recoveryRehome: true, + }); + } + // Release any in-memory worktree ownership the leaked park may still + // pin, so the requeued task does not re-block the concurrency gate. + this.options.releaseExecutorWorktreeOwnership?.(task.id); + + await this.store.logEntry( + task.id, + "Auto-recovered: pause-abort park cleared — requeued for normal scheduling", + ); + await this.store.recordRunAuditEvent?.({ + taskId: task.id, + agentId: "self-healing", + runId: generateSyntheticRunId("self-healing", task.id), + domain: "database", + mutationType: "task:auto-recover-paused-abort-park", + target: task.id, + metadata: { fromColumn: fresh.column }, + }); + log.log(`Recovered pause-abort park ${task.id}: ${task.title || task.description?.slice(0, 60) || "(untitled)"}`); + recovered++; + } catch (err: unknown) { const errorMessage = err instanceof Error ? err.message : String(err); + log.error(`Failed to recover pause-abort park ${task.id}: ${errorMessage}`); + } + } + + if (recovered > 0) { + log.log(`Recovered ${recovered} pause-abort park(s) → requeued to todo`); + } + return recovered; + } catch (err: unknown) { const errorMessage = err instanceof Error ? err.message : String(err); + log.error(`Pause-abort park recovery failed: ${errorMessage}`); + return 0; + } + } + async auditNoCommitsExpectedCandidates(): Promise { try { const inReviewTasks = await this.store.listTasks({ column: "in-review", slim: true }); From 8e4b9e4f0f3dbec37060c8376e6755293918e6b0 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 19 Jun 2026 20:03:24 -0700 Subject: [PATCH 2/5] Add leaked-slot reaper (A2) to self-healing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reapLeakedConcurrencySlots() reclaims in-memory worktree slots whose holder is no longer in-progress (the FN-6756 "in todo yet still a maxWorktrees holder" leak) without an engine restart — defense-in-depth behind the source fix. - executor: new listWorktreeHolders() read-only introspection over activeWorktrees; wired through in-process-runtime to SelfHealingManager. - reaper releases ONLY when every guard agrees: not executing, task missing or in todo/triage, past a 60s grace, and clearPhantomExecutor Binding itself refuses (returns false) if a live session surface is registered — so it can never pull a worktree from a running agent. - registered in maintenance batch 2 (respects globalPause/enginePaused skip + FN-4962 ordering). - widened the clearPhantomExecutorBinding option type to surface its boolean refusal signal. Engine typecheck clean; 19 tests pass (new reaper 7 cases + regression). Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/fix-pause-abort-leak-storm.md | 1 + .../self-healing-leaked-slot-reaper.test.ts | 163 ++++++++++++++++++ packages/engine/src/executor.ts | 19 ++ .../engine/src/runtimes/in-process-runtime.ts | 1 + packages/engine/src/self-healing.ts | 91 +++++++++- 5 files changed, 273 insertions(+), 2 deletions(-) create mode 100644 packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts diff --git a/.changeset/fix-pause-abort-leak-storm.md b/.changeset/fix-pause-abort-leak-storm.md index 3d66855ff3..d5d4487512 100644 --- a/.changeset/fix-pause-abort-leak-storm.md +++ b/.changeset/fix-pause-abort-leak-storm.md @@ -6,3 +6,4 @@ Fix the global pause/resume failure mode that stalled the board: a pause-abort t - Root cause: `handleGraphFailure` now treats a pause-abort that has re-queued a task to `todo` as benign (FN-6782) — it no longer parks it failed, clears the `pausedAborted` marker so the next dispatch starts clean, and releases the leaked worktree slot. - Auto-recovery: a new `recoverPausedAbortFailures` self-healing sweep clears any pause-abort park (`status:"failed"` with "operator action required") still on the board and requeues it for normal scheduling, so the board self-heals without operator intervention. +- Defense-in-depth: a new `reapLeakedConcurrencySlots` self-healing sweep reclaims any in-memory worktree slot whose holder is no longer in-progress (the "in todo yet still a `maxWorktrees` holder" leak), gated by the executor's live-session refusal so it can never pull a worktree out from under a running agent. This recovers a leaked slot from any future/unknown path without an engine restart. diff --git a/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts b/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts new file mode 100644 index 0000000000..512e4dd489 --- /dev/null +++ b/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts @@ -0,0 +1,163 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { EventEmitter } from "node:events"; + +const { logger } = vi.hoisted(() => ({ logger: { log: vi.fn(), warn: vi.fn(), error: vi.fn() } })); +vi.mock("../logger.js", () => ({ + createLogger: vi.fn(() => logger), + schedulerLog: { log: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); + +vi.mock("../worktree-pool.js", () => ({ + WorktreePool: vi.fn(), + RemovalReason: {}, + scanIdleWorktrees: vi.fn().mockResolvedValue([]), + cleanupOrphanedWorktrees: vi.fn().mockResolvedValue(0), + isUsableTaskWorktree: vi.fn().mockResolvedValue(true), + removeWorktree: vi.fn().mockResolvedValue(undefined), + resolveWorktreeBackend: vi.fn(), +})); + +vi.mock("../merger.js", () => ({ classifyOwnedLandedEvidence: vi.fn() })); + +import { SelfHealingManager } from "../self-healing.js"; +import type { Settings, Task, TaskStore } from "@fusion/core"; + +function createMockStore(overrides: Record = {}): TaskStore & EventEmitter { + const emitter = new EventEmitter(); + return Object.assign(emitter, { + getSettings: vi.fn().mockResolvedValue({ + autoMerge: true, + globalPause: false, + enginePaused: false, + maintenanceIntervalMs: 0, + } as unknown as Settings), + listTasks: vi.fn().mockResolvedValue([]), + getTask: vi.fn(), + updateTask: vi.fn().mockResolvedValue({} as Task), + logEntry: vi.fn().mockResolvedValue(undefined), + moveTask: vi.fn().mockResolvedValue(undefined), + recordRunAuditEvent: vi.fn().mockResolvedValue(undefined), + getRootDir: vi.fn().mockReturnValue("/tmp/test-project"), + ...overrides, + }) as unknown as TaskStore & EventEmitter; +} + +// A task that has sat in its column well past the reaper's 60s grace. +function taskRow(id: string, column: string, extra: Record = {}): Task { + return { + id, + column, + paused: false, + columnMovedAt: "2026-05-20T12:00:00.000Z", + updatedAt: "2026-05-20T12:00:00.000Z", + steps: [], + ...extra, + } as unknown as Task; +} + +describe("reapLeakedConcurrencySlots", () => { + let store: TaskStore & EventEmitter; + let clearPhantomExecutorBinding: ReturnType; + + beforeEach(() => { + vi.useFakeTimers(); + // Well past the 60s grace relative to the task rows' columnMovedAt. + vi.setSystemTime(new Date("2026-05-20T12:05:00.000Z")); + clearPhantomExecutorBinding = vi.fn().mockReturnValue(true); + store = createMockStore(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.clearAllMocks(); + }); + + function makeManager(holders: Array<{ taskId: string; worktreePath: string }>, executing: string[] = []) { + return new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + listWorktreeHolders: () => holders, + getExecutingTaskIds: () => new Set(executing), + clearPhantomExecutorBinding, + }); + } + + it("releases a leaked slot whose holder sits in todo and is not executing", async () => { + (store.getTask as ReturnType).mockResolvedValue(taskRow("FN-6756", "todo")); + const manager = makeManager([{ taskId: "FN-6756", worktreePath: "/wt/pearl-lark" }]); + + const reaped = await manager.reapLeakedConcurrencySlots(); + + expect(reaped).toBe(1); + expect(clearPhantomExecutorBinding).toHaveBeenCalledWith("FN-6756"); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-6756", + expect.stringContaining("released leaked worktree/concurrency slot"), + ); + }); + + it("does NOT release a legit in-progress holder", async () => { + (store.getTask as ReturnType).mockResolvedValue(taskRow("FN-6750", "in-progress")); + const manager = makeManager([{ taskId: "FN-6750", worktreePath: "/wt/proud-delta" }]); + + const reaped = await manager.reapLeakedConcurrencySlots(); + + expect(reaped).toBe(0); + expect(clearPhantomExecutorBinding).not.toHaveBeenCalled(); + }); + + it("does NOT release a holder that is still executing, even if its column looks reapable", async () => { + (store.getTask as ReturnType).mockResolvedValue(taskRow("FN-6700", "todo")); + const manager = makeManager([{ taskId: "FN-6700", worktreePath: "/wt/fast-falcon" }], ["FN-6700"]); + + const reaped = await manager.reapLeakedConcurrencySlots(); + + expect(reaped).toBe(0); + expect(clearPhantomExecutorBinding).not.toHaveBeenCalled(); + }); + + it("does NOT release an in-review holder (conservative — it legitimately keeps its worktree)", async () => { + (store.getTask as ReturnType).mockResolvedValue(taskRow("FN-6744", "in-review")); + const manager = makeManager([{ taskId: "FN-6744", worktreePath: "/wt/jade-grove" }]); + + const reaped = await manager.reapLeakedConcurrencySlots(); + + expect(reaped).toBe(0); + expect(clearPhantomExecutorBinding).not.toHaveBeenCalled(); + }); + + it("does NOT release a todo holder still inside the grace window", async () => { + (store.getTask as ReturnType).mockResolvedValue( + taskRow("FN-6753", "todo", { columnMovedAt: "2026-05-20T12:04:30.000Z" }), // 30s < 60s grace + ); + const manager = makeManager([{ taskId: "FN-6753", worktreePath: "/wt/swift-falcon" }]); + + const reaped = await manager.reapLeakedConcurrencySlots(); + + expect(reaped).toBe(0); + expect(clearPhantomExecutorBinding).not.toHaveBeenCalled(); + }); + + it("skips entirely under global pause", async () => { + (store.getSettings as ReturnType).mockResolvedValue({ + globalPause: true, + enginePaused: false, + } as unknown as Settings); + (store.getTask as ReturnType).mockResolvedValue(taskRow("FN-6756", "todo")); + const manager = makeManager([{ taskId: "FN-6756", worktreePath: "/wt/pearl-lark" }]); + + const reaped = await manager.reapLeakedConcurrencySlots(); + + expect(reaped).toBe(0); + expect(clearPhantomExecutorBinding).not.toHaveBeenCalled(); + }); + + it("reaps an orphan holder with no task row", async () => { + (store.getTask as ReturnType).mockResolvedValue(null); + const manager = makeManager([{ taskId: "FN-ghost", worktreePath: "/wt/gone" }]); + + const reaped = await manager.reapLeakedConcurrencySlots(); + + expect(reaped).toBe(1); + expect(clearPhantomExecutorBinding).toHaveBeenCalledWith("FN-ghost"); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 5be0f7a3b3..e69f9badd9 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -13986,6 +13986,25 @@ Backward compat fallback: if JSON is unavailable, you may still begin output wit * The requesting task is excluded from the check because `cleanupConflictingWorktree` is * only called for worktrees the requesting task is trying to displace. */ + /** + * FN-6782 leaked-slot reaper support: expose a read-only snapshot of the + * in-memory `activeWorktrees` holders so SelfHealingManager can cross-check + * each holder's task column and reclaim a slot whose holder is no longer + * legitimately in-progress (the "in todo yet still maxWorktrees holder" + * leak). Returns a copied array — never the live Map — so callers cannot + * mutate executor state. The actual release still goes through + * `clearPhantomExecutorBinding`, which refuses to detach live session + * surfaces, so this introspection cannot by itself pull a worktree out from + * under a running agent. + */ + listWorktreeHolders(): Array<{ taskId: string; worktreePath: string }> { + const holders: Array<{ taskId: string; worktreePath: string }> = []; + for (const [taskId, worktreePath] of this.activeWorktrees) { + holders.push({ taskId, worktreePath }); + } + return holders; + } + private async findActiveWorktreeOwner( worktreePath: string, requestingTaskId: string, diff --git a/packages/engine/src/runtimes/in-process-runtime.ts b/packages/engine/src/runtimes/in-process-runtime.ts index 25973f5f5d..c9a4d1506c 100644 --- a/packages/engine/src/runtimes/in-process-runtime.ts +++ b/packages/engine/src/runtimes/in-process-runtime.ts @@ -788,6 +788,7 @@ export class InProcessRuntime recoverFailedPreMergeStep: (task) => this.executor.recoverFailedPreMergeWorkflowStep(task), getExecutingTaskIds: () => this.executor?.getExecutingTaskIds() ?? new Set(), clearPhantomExecutorBinding: (taskId: string) => this.executor?.clearPhantomExecutorBinding(taskId), + listWorktreeHolders: () => this.executor?.listWorktreeHolders() ?? [], recoverApprovedTriageTask: (task) => this.triageProcessor?.recoverApprovedTask(task) ?? Promise.resolve(false), getPlanningTaskIds: () => this.triageProcessor?.getProcessingTaskIds() ?? new Set(), evictStaleTriageProcessing: () => this.triageProcessor?.evictStaleProcessing() ?? new Set(), diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 64eebbafea..da3acc1951 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -242,8 +242,19 @@ export interface SelfHealingOptions { rootDir: string; /** Optional callback to release TaskExecutor in-memory worktree ownership for a task. */ releaseExecutorWorktreeOwnership?: (taskId: string) => void; - /** Optional callback to clear a demonstrably-stale executor binding without touching live sessions. */ - clearPhantomExecutorBinding?: (taskId: string) => void; + /** + * FN-6782: read-only snapshot of the executor's in-memory worktree holders + * ({ taskId, worktreePath }), so the leaked-slot reaper can cross-check each + * holder's task column and reclaim a slot whose holder is no longer in-progress. + */ + listWorktreeHolders?: () => Array<{ taskId: string; worktreePath: string }>; + /** + * Optional callback to clear a demonstrably-stale executor binding without + * touching live sessions. Returns `true` if the binding was cleared, `false` + * if the executor refused because a live session surface is still registered + * (the leaked-slot reaper relies on this refusal signal). + */ + clearPhantomExecutorBinding?: (taskId: string) => boolean | void; /** Optional AgentStore for agent-level self-healing checks. */ agentStore?: AgentStore; /** Canonical stale-lease recovery manager. */ @@ -357,6 +368,12 @@ const STARVED_REFINEMENT_RECOVERY_GRACE_MS = 10 * 60_000; const STARVED_PEER_PROGRESS_THRESHOLD = 3; const STARVED_REFINEMENT_ESCALATION_COOLDOWN_MS = STARVED_REFINEMENT_RECOVERY_GRACE_MS * 4; const ORPHANED_EXECUTION_RECOVERY_GRACE_MS = 60_000; +/** + * FN-6782 leaked-slot reaper grace: a worktree holder whose task has sat in a + * reapable column (todo/triage) shorter than this is left alone, so the reaper + * never races a task mid-transition out of in-progress. + */ +const LEAKED_WORKTREE_SLOT_GRACE_MS = 60_000; export const VALIDATOR_RUN_STALE_MAX_AGE_MS = 6 * 60 * 60 * 1000; const ACTIVE_MERGE_STATUSES = new Set(["merging", "merging-pr", "merging-fix"]); const NON_TERMINAL_STEP_STATUSES = new Set(["pending", "in-progress"]); @@ -2136,6 +2153,10 @@ export class SelfHealingManager { { name: "recover-stale-transition-pending", fn: () => this.runStaleTransitionPendingSweep() }, { name: "reconcile-self-defeating-deps", fn: () => this.reconcileSelfDefeatingDependencies() }, { name: "reconcile-dependency-blocking-leases", fn: () => this.reconcileDependencyBlockingLeases() }, + // FN-6782: reclaim in-memory worktree slots whose holder is no longer + // in-progress (defense-in-depth for the pause-abort leak; conservative, + // gated by clearPhantomExecutorBinding's live-session refusal). + { name: "reap-leaked-concurrency-slots", fn: () => this.reapLeakedConcurrencySlots() }, { name: "reconcile-dependency-cycles", fn: () => this.reconcileDependencyCycles().then(() => undefined) }, { name: "reclaim-pr-conflicts", fn: () => this.reclaimPrConflicts() }, { name: "reclaim-self-owned-branch-conflicts", fn: () => this.reclaimSelfOwnedBranchConflicts() }, @@ -8005,6 +8026,72 @@ export class SelfHealingManager { } } + /** + * FN-6782 leaked-slot reaper (defense-in-depth). The source leak is closed in + * the executor's pause-abort park path, but a worktree slot leaked by any + * other/future path would silently pin `maxWorktrees` and concurrency-starve + * the whole queue (the FN-6756 "in todo yet still maxWorktrees=3/3 holder" + * symptom) until an engine restart. This sweep cross-checks each in-memory + * worktree holder against its task column and reclaims slots whose holder is + * no longer legitimately holding one. + * + * Conservative by construction — release happens ONLY when every guard agrees: + * - the holder is NOT in the executor's executing set; + * - its task is missing, or in `todo`/`triage` (a task waiting to run must + * not pin a worktree). `in-progress` and `in-review` holders legitimately + * retain their worktree; `done`/`archived` are handled by worktree-metadata + * reconcile + merge cleanup, so they are left alone here; + * - it has sat in the reapable column past a short grace (no mid-transition race); + * - and finally `clearPhantomExecutorBinding` itself refuses (returns false) + * if any live session surface is still registered — the last line of + * defense against pulling a worktree out from under a running agent. + */ + async reapLeakedConcurrencySlots(): Promise { + const settings = await this.store.getSettings(); + if (settings.globalPause || settings.enginePaused) { + return 0; + } + + const holders = this.options.listWorktreeHolders?.() ?? []; + if (holders.length === 0) return 0; + + const executingIds = this.options.getExecutingTaskIds?.() ?? new Set(); + const now = Date.now(); + let reaped = 0; + + for (const { taskId } of holders) { + try { + if (executingIds.has(taskId)) continue; + + const task = await this.store.getTask(taskId).catch(() => null); + const reapableColumn = !task || task.column === "todo" || task.column === "triage"; + if (!reapableColumn) continue; + + if (task) { + const since = new Date(task.columnMovedAt ?? task.updatedAt).getTime(); + if (Number.isFinite(since) && now - since < LEAKED_WORKTREE_SLOT_GRACE_MS) continue; + } + + const released = this.options.clearPhantomExecutorBinding?.(taskId); + // false = executor refused (live session surface); undefined = not wired. + if (released !== true) continue; + + reaped++; + await this.store.logEntry( + taskId, + "Auto-recovered: released leaked worktree/concurrency slot (holder no longer in-progress)", + ); + log.warn(`Reaped leaked worktree slot held by ${taskId} (column=${task?.column ?? "missing"})`); + } catch (err: unknown) { + const errorMessage = err instanceof Error ? err.message : String(err); + log.error(`Leaked-slot reaper failed for ${taskId}: ${errorMessage}`); + } + } + + if (reaped > 0) log.log(`Reaped ${reaped} leaked worktree slot(s)`); + return reaped; + } + /** * Recover executor tasks stranded in `in-progress` before a real session was * established, typically when the scheduler reserved a worktree path but the From 7eaf513944604fd791dde2fb24ae76e3fe2b64a4 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 19 Jun 2026 20:10:50 -0700 Subject: [PATCH 3/5] Address PR #1687 review: harden pause-abort recovery + FNXC comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Substantive (A1 recoverPausedAbortFailures): - Self-guard on globalPause/enginePaused at method entry (greptile P1) — the public method must not requeue tasks an operator intentionally froze. - Re-validate the FULL predicate with a FRESH executing set on the re-read before the backward move (coderabbit Major + greptile): add fresh.userPaused and column re-check so a task that became ineligible across awaits is skipped. - Isolate audit emission in its own try/catch (coderabbit) so an audit throw after a successful mutation can't log a false "recovery failed". - Decouple the recovery predicate from the literal error text via shared PAUSE_ABORT_PARK_ERROR_MARKER/OPERATOR_MARKER constants (greptile) — the executor builds the parked message from the same constants. - Use the wired clearPhantomExecutorBinding (live-session-guarded) instead of the declared-but-never-wired releaseExecutorWorktreeOwnership, which no-op'd. Nits: - FNXC-prefix new comments in executor.ts, run-audit.ts, and the benign test per repo comment policy. - Fix a test-only type error on the clearPhantomExecutorBinding mock. Added a test asserting the globalPause self-guard. Engine typecheck clean; pause-abort/reaper/benign + regression suites pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../executor-paused-abort-todo-benign.test.ts | 8 +-- .../self-healing-leaked-slot-reaper.test.ts | 2 +- ...self-healing-paused-abort-recovery.test.ts | 31 +++++++- packages/engine/src/executor.ts | 25 ++++--- packages/engine/src/run-audit.ts | 4 +- packages/engine/src/self-healing.ts | 70 +++++++++++++++---- 6 files changed, 105 insertions(+), 35 deletions(-) diff --git a/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts b/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts index 9c5d55ec1b..a78c07791e 100644 --- a/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts +++ b/packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts @@ -71,16 +71,16 @@ describe("pause-abort benign requeue-to-todo (FN-6782)", () => { await invokeGraphFailure(executor, task); - // Must NOT write status:"failed" — that was the storm trigger. + // FNXC:WorkflowLifecycle a todo pause-abort must NOT write status:"failed" — that was the storm trigger. const parkedFailed = store.updateTask.mock.calls.some( (call: unknown[]) => (call[1] as { status?: string } | undefined)?.status === "failed", ); expect(parkedFailed).toBe(false); - // Benign log surfaced. + // FNXC:WorkflowLifecycle the benign-clear log must surface for observability. expect(logText(store)).toContain("benign, cleared for normal scheduling"); - // pausedAborted marker cleared so the next dispatch starts clean. + // FNXC:WorkflowLifecycle the pausedAborted marker must be cleared so the next dispatch starts clean. expect((executor as any).pausedAborted.has(task.id)).toBe(false); - // Leaked worktree slot released. + // FNXC:WorkflowLifecycle the leaked worktree slot must be released to avoid board-wide concurrency blockage. expect((executor as any).activeWorktrees.has(task.id)).toBe(false); }); diff --git a/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts b/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts index 512e4dd489..eb6c75743f 100644 --- a/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts +++ b/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts @@ -77,7 +77,7 @@ describe("reapLeakedConcurrencySlots", () => { rootDir: "/tmp/test-project", listWorktreeHolders: () => holders, getExecutingTaskIds: () => new Set(executing), - clearPhantomExecutorBinding, + clearPhantomExecutorBinding: clearPhantomExecutorBinding as (taskId: string) => boolean | void, }); } diff --git a/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts b/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts index 2946ee6fe8..4ff04e103c 100644 --- a/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts +++ b/packages/engine/src/__tests__/self-healing-paused-abort-recovery.test.ts @@ -76,11 +76,11 @@ describe("recoverPausedAbortFailures", () => { it("clears a todo-column pause-abort park to schedulable (status:null) without moving it", async () => { const store = createMockStore([parkTask({ id: "FN-7000", column: "todo" })]); - const release = vi.fn(); + const clearBinding = vi.fn().mockReturnValue(true); const manager = new SelfHealingManager(store, { rootDir: "/tmp/test-project", getExecutingTaskIds: () => new Set(), - releaseExecutorWorktreeOwnership: release, + clearPhantomExecutorBinding: clearBinding as (taskId: string) => boolean | void, }); const recovered = await manager.recoverPausedAbortFailures(); @@ -89,7 +89,9 @@ describe("recoverPausedAbortFailures", () => { expect(store.updateTask).toHaveBeenCalledWith("FN-7000", { status: null, error: null }); // Already in todo — must NOT be moved. expect(store.moveTask).not.toHaveBeenCalled(); - expect(release).toHaveBeenCalledWith("FN-7000"); + // FNXC:WorkflowLifecycle A1 releases via the wired clearPhantomExecutorBinding, + // not the dead releaseExecutorWorktreeOwnership option (PR #1687 review). + expect(clearBinding).toHaveBeenCalledWith("FN-7000"); expect(store.recordRunAuditEvent).toHaveBeenCalledWith( expect.objectContaining({ mutationType: "task:auto-recover-paused-abort-park", target: "FN-7000" }), ); @@ -129,4 +131,27 @@ describe("recoverPausedAbortFailures", () => { expect(recovered).toBe(0); expect(store.updateTask).not.toHaveBeenCalled(); }); + + // FNXC:WorkflowLifecycle greptile P1 (PR #1687): the method self-guards on + // global/engine pause at its own entry, so calling it directly (test/API path) + // while the operator has frozen the board must be a no-op. + it("self-guards: does nothing while globalPause is set", async () => { + const store = createMockStore([parkTask({ id: "FN-7000", column: "todo" })]); + (store.getSettings as ReturnType).mockResolvedValue({ + autoMerge: true, + globalPause: true, + enginePaused: false, + maintenanceIntervalMs: 0, + } as unknown as Settings); + const manager = new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + getExecutingTaskIds: () => new Set(), + }); + + const recovered = await manager.recoverPausedAbortFailures(); + + expect(recovered).toBe(0); + expect(store.updateTask).not.toHaveBeenCalled(); + expect(store.moveTask).not.toHaveBeenCalled(); + }); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index e69f9badd9..f8776eba8d 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -158,7 +158,7 @@ import { isMissingWorktreeSessionStartFailure, } from "./restart-recovery-coordinator.js"; import { BranchWorktreeAutoRecoveryHandler } from "./auto-recovery-handlers/branch-worktree.js"; -import { autoRecoverWorktreeSessionStartFailure, MAX_WORKTREE_SESSION_RETRIES } from "./self-healing.js"; +import { autoRecoverWorktreeSessionStartFailure, MAX_WORKTREE_SESSION_RETRIES, PAUSE_ABORT_PARK_ERROR_MARKER, PAUSE_ABORT_PARK_OPERATOR_MARKER } from "./self-healing.js"; import { ContaminationAutoRecoveryHandler } from "./auto-recovery-handlers/contamination.js"; import { createFileScopeAutoRecoveryHandler } from "./auto-recovery-handlers/file-scope.js"; import { ReadonlyViolationError, filterCustomToolsForReadonly } from "./workflow-step-tool-policy.js"; @@ -6652,15 +6652,15 @@ export class TaskExecutor { // non-todo columns (e.g. in-review), per FN-6478. if (live.column === "todo") { this.clearPausedAborted(task.id); - // FN-6782 leak fix: a task parked back to `todo` must not keep - // pinning its in-memory worktree slot. The execute() finally does - // not delete activeWorktrees on this early-return path, so without - // this release the slot leaks — a `todo` task stays a maxWorktrees - // holder and concurrency-blocks the whole queue (the FN-6756 - // "in todo yet still a holder, maxWorktrees=3/3" symptom). Mirror - // clearPhantomExecutorBinding's release semantics. Safe here: - // handleGraphFailure is terminal for this run (no seam re-entry), - // and the next dispatch re-acquires a fresh worktree. + // FNXC:WorkflowLifecycle 2026-06-20-00:00: FN-6782 leak fix — a task + // parked back to `todo` must not keep pinning its in-memory worktree + // slot. The execute() finally does not delete activeWorktrees on this + // early-return path, so without this release the slot leaks — a `todo` + // task stays a maxWorktrees holder and concurrency-blocks the whole + // queue (the FN-6756 "in todo yet still a holder, maxWorktrees=3/3" + // symptom). Mirror clearPhantomExecutorBinding's release semantics. + // Safe here: handleGraphFailure is terminal for this run (no seam + // re-entry), and the next dispatch re-acquires a fresh worktree. this.activeWorktrees.delete(task.id); const todoBenign = `Workflow graph run ended during ${pauseProvenance} with task re-queued to todo — benign, cleared for normal scheduling`; executorLog.log(`${task.id}: ${todoBenign}`); @@ -6669,7 +6669,10 @@ export class TaskExecutor { return; } const failedNode = result.visitedNodeIds[result.visitedNodeIds.length - 1] ?? "unknown"; - const message = `Workflow graph failure surfaced after paused ${pauseProvenance} in '${live.column}' at node '${failedNode}' — operator action required; retry or explicitly unpause/resume after inspecting the task`; + // FNXC:WorkflowLifecycle 2026-06-20-00:00: build the parked-failure + // message from the shared markers so self-healing's recoverPausedAbortFailures + // predicate cannot drift out of sync with this text (PR #1687 review). + const message = `${PAUSE_ABORT_PARK_ERROR_MARKER} ${pauseProvenance} in '${live.column}' at node '${failedNode}' — ${PAUSE_ABORT_PARK_OPERATOR_MARKER}; retry or explicitly unpause/resume after inspecting the task`; executorLog.warn(`${task.id}: ${message}`); await this.store.logEntry(task.id, message, undefined, this.getRunContextFor(task.id)); if (live.column !== "done" && live.column !== "archived" && live.status == null && live.error == null) { diff --git a/packages/engine/src/run-audit.ts b/packages/engine/src/run-audit.ts index b6f8c5d43c..1baaf8da1d 100644 --- a/packages/engine/src/run-audit.ts +++ b/packages/engine/src/run-audit.ts @@ -459,9 +459,9 @@ export type DatabaseMutationType = | "task:auto-recover-worktree-metadata-rebound" | "task:auto-recover-worktree-metadata-cleared" | "task:auto-recover-worktree-metadata-skipped-active" - // pause-abort auto-recovery (FN-6782): a global pause/resume park cleared and requeued + // FNXC:Lifecycle FNXC_LOG 2026-06-20-00:00: FN-6782 — audit type for a global pause/resume park that was cleared and requeued by self-healing. | "task:auto-recover-paused-abort-park" - // leaked concurrency-slot reaper: a worktree/lease/semaphore slot whose holder left in-progress + // FNXC:Lifecycle FNXC_LOG 2026-06-20-00:00: audit type for reaping a leaked worktree/lease/semaphore slot whose holder left in-progress. | "task:reap-leaked-concurrency-slot" // task:auto-archived-ghost-bug metadata: { findings: Array<{ construct: { kind: string; raw: string; filePath?: string; line?: number }; matched: boolean; probeError?: string; output?: string }>; reason: string } // task:auto-archived-duplicate metadata: { siblingTaskIds: string[]; scores: Record } diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index da3acc1951..544022488e 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -412,6 +412,16 @@ const ORPHANED_WITH_WORKTREE_GRACE_MS = 300_000; */ const MAX_TASK_DONE_RETRIES = 3; export const MAX_WORKTREE_SESSION_RETRIES = 3; +/** + * FNXC:WorkflowLifecycle 2026-06-20-00:00: single source of truth for the + * pause-abort park error message markers. The executor's handleGraphFailure + * builds the parked-failure message from these, and `recoverPausedAbortFailures` + * matches on them — sharing the constants prevents the recovery predicate from + * silently drifting if the message text is ever edited (greptile review on + * PR #1687: a string-coupled predicate breaks with no compile-time signal). + */ +export const PAUSE_ABORT_PARK_ERROR_MARKER = "Workflow graph failure surfaced after paused"; +export const PAUSE_ABORT_PARK_OPERATOR_MARKER = "operator action required"; /** * FNXC:AutoMergeRetries 2026-06-17-04:20: * Keep this export as the historical default seed for tests and dashboard fallback alignment, but SelfHealingManager must call resolveMaxAutoMergeRetries(settings) at decision points so configured projects do not recover or stall at the old fixed value. @@ -7913,14 +7923,22 @@ export class SelfHealingManager { */ async recoverPausedAbortFailures(): Promise { try { + // FNXC:WorkflowLifecycle 2026-06-20-00:00: self-guard against global/engine + // pause at the method entry, not just the batch-2 runner. This method is + // public and exercised directly (tests, potential API path); without this, + // calling it while paused would requeue tasks the operator intentionally + // froze (greptile P1, PR #1687). Mirrors every peer recovery func. + const settings = await this.store.getSettings(); + if (settings.globalPause || settings.enginePaused) return 0; + const executingIds = this.options.getExecutingTaskIds?.() ?? new Set(); const tasks = await this.store.listTasks({ slim: true }); const isPausedAbortPark = (t: Task): boolean => t.status === "failed" && typeof t.error === "string" && - t.error.includes("operator action required") && - t.error.includes("Workflow graph failure surfaced after paused"); + t.error.includes(PAUSE_ABORT_PARK_OPERATOR_MARKER) && + t.error.includes(PAUSE_ABORT_PARK_ERROR_MARKER); const parked = tasks.filter((t) => isPausedAbortPark(t) && @@ -7940,9 +7958,22 @@ export class SelfHealingManager { let recovered = 0; for (const task of parked) { try { - // Re-read to avoid acting on a stale snapshot after awaits. + // FNXC:WorkflowLifecycle 2026-06-20-00:00: re-read AND re-validate the + // FULL predicate against the refreshed row with a FRESH executing set + // before mutating — the outer snapshot can go stale across awaits, so a + // task that became ineligible (paused, user-paused, started executing, + // or moved to a non-recoverable column) must not get a backward move + // applied (coderabbit Major + greptile, PR #1687). const fresh = await this.store.getTask(task.id); - if (!fresh || !isPausedAbortPark(fresh) || fresh.paused || executingIds.has(fresh.id)) { + const latestExecutingIds = this.options.getExecutingTaskIds?.() ?? new Set(); + if ( + !fresh || + !isPausedAbortPark(fresh) || + fresh.paused || + fresh.userPaused || + latestExecutingIds.has(fresh.id) || + !(fresh.column === "todo" || fresh.column === "in-progress") + ) { continue; } @@ -7956,21 +7987,32 @@ export class SelfHealingManager { } // Release any in-memory worktree ownership the leaked park may still // pin, so the requeued task does not re-block the concurrency gate. - this.options.releaseExecutorWorktreeOwnership?.(task.id); + // 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); await this.store.logEntry( task.id, "Auto-recovered: pause-abort park cleared — requeued for normal scheduling", ); - await this.store.recordRunAuditEvent?.({ - taskId: task.id, - agentId: "self-healing", - runId: generateSyntheticRunId("self-healing", task.id), - domain: "database", - mutationType: "task:auto-recover-paused-abort-park", - target: task.id, - metadata: { fromColumn: fresh.column }, - }); + // FNXC:WorkflowLifecycle 2026-06-20-00:00: audit emission is strictly + // best-effort — an audit throw AFTER the successful state mutation must + // not drop into the per-task catch and falsely log "recovery failed" / + // skip the recovered++ (coderabbit, PR #1687). + try { + await this.store.recordRunAuditEvent?.({ + taskId: task.id, + agentId: "self-healing", + runId: generateSyntheticRunId("self-healing", task.id), + domain: "database", + mutationType: "task:auto-recover-paused-abort-park", + target: task.id, + metadata: { fromColumn: fresh.column }, + }); + } catch (auditErr: unknown) { + log.warn(`Pause-abort park audit emission failed for ${task.id}: ${auditErr instanceof Error ? auditErr.message : String(auditErr)}`); + } log.log(`Recovered pause-abort park ${task.id}: ${task.title || task.description?.slice(0, 60) || "(untitled)"}`); recovered++; } catch (err: unknown) { const errorMessage = err instanceof Error ? err.message : String(err); From 8c93e2ad6ba8b3ba4e0980e1dd4c15864b61e0c1 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 19 Jun 2026 20:34:53 -0700 Subject: [PATCH 4/5] Re-check execution ownership before reaping a leaked slot (PR #1687) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit coderabbit Major: reapLeakedConcurrencySlots captured executingIds once before the loop, but each holder awaits getTask — a task could start executing mid-sweep and have its worktree slot pulled. Refresh the executing set immediately before clearPhantomExecutorBinding and skip if the holder is now executing (same race the A1 recovery fix closed). clearPhantomExecutorBinding's live-session refusal remains the last line of defense; this avoids racing it. Added a mid-sweep race test. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../self-healing-leaked-slot-reaper.test.ts | 20 +++++++++++++++++++ packages/engine/src/self-healing.ts | 9 +++++++++ 2 files changed, 29 insertions(+) diff --git a/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts b/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts index eb6c75743f..7f67f22206 100644 --- a/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts +++ b/packages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.ts @@ -160,4 +160,24 @@ describe("reapLeakedConcurrencySlots", () => { expect(reaped).toBe(1); expect(clearPhantomExecutorBinding).toHaveBeenCalledWith("FN-ghost"); }); + + // FNXC:WorkflowLifecycle coderabbit Major (PR #1687): a holder that starts + // executing AFTER the pre-loop snapshot but BEFORE the release must be spared + // by the fresh executing-set re-check. + it("does NOT release a holder that starts executing mid-sweep", async () => { + (store.getTask as ReturnType).mockResolvedValue(taskRow("FN-6760", "todo")); + let call = 0; + const manager = new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + listWorktreeHolders: () => [{ taskId: "FN-6760", worktreePath: "/wt/calm-iris" }], + // Pre-loop snapshot is empty; by the fresh re-check the task is executing. + getExecutingTaskIds: () => new Set(call++ === 0 ? [] : ["FN-6760"]), + clearPhantomExecutorBinding: clearPhantomExecutorBinding as (taskId: string) => boolean | void, + }); + + const reaped = await manager.reapLeakedConcurrencySlots(); + + expect(reaped).toBe(0); + expect(clearPhantomExecutorBinding).not.toHaveBeenCalled(); + }); }); diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 544022488e..59c9b854a4 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -8114,6 +8114,15 @@ export class SelfHealingManager { if (Number.isFinite(since) && now - since < LEAKED_WORKTREE_SLOT_GRACE_MS) continue; } + // FNXC:WorkflowLifecycle 2026-06-20-00:00: re-check execution ownership + // against a FRESH executing set immediately before releasing — the outer + // `executingIds` snapshot predates this holder's `getTask` await, so a + // task that started executing mid-sweep must not have its slot pulled + // (coderabbit Major, PR #1687). clearPhantomExecutorBinding's live-session + // refusal is the last line of defense, but this avoids racing it at all. + const latestExecutingIds = this.options.getExecutingTaskIds?.() ?? new Set(); + if (latestExecutingIds.has(taskId)) continue; + const released = this.options.clearPhantomExecutorBinding?.(taskId); // false = executor refused (live session surface); undefined = not wired. if (released !== true) continue; From 7f0ad62731ecb991c932bb3ba3ec0d11b5018e41 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 19 Jun 2026 20:53:37 -0700 Subject: [PATCH 5/5] Update executor-recovery test for benign todo pause-abort (FN-6782) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate test asserted the OLD behavior — a paused graph exit in the `todo` column parked `status:"failed"` with "operator action required". FN-6782 made the todo case benign (no failed park; benign log + cleared marker), so split the parameterized test: `todo` now asserts the benign path (never parked failed), `done` keeps the operator-action surfacing (log only, no park). Full engine-core gate suite passes (644/644). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/__tests__/executor-recovery.test.ts | 100 ++++++++++-------- 1 file changed, 57 insertions(+), 43 deletions(-) diff --git a/packages/engine/src/__tests__/executor-recovery.test.ts b/packages/engine/src/__tests__/executor-recovery.test.ts index 55f811c9d4..b3cffb439a 100644 --- a/packages/engine/src/__tests__/executor-recovery.test.ts +++ b/packages/engine/src/__tests__/executor-recovery.test.ts @@ -1878,51 +1878,65 @@ describe("TaskExecutor bounded recovery retries", () => { expect(store.handoffToReview).not.toHaveBeenCalled(); }); - it.each(["todo", "done"] as const)( - "surfaces paused graph exits in already-advanced %s column without lifecycle movement", - async (column) => { - const store = createMockStore(); - const task = { - id: "FN-001", - title: "Test", - description: "Test", - column: "in-progress", - status: undefined, - dependencies: [], - steps: [{ name: "Preflight", status: "pending" }], - currentStep: 0, - log: [{ timestamp: new Date().toISOString(), action: "Resuming execution after unpause" }], - createdAt: new Date().toISOString(), - updatedAt: new Date().toISOString(), - } as Task; - store.getTask.mockResolvedValue({ - ...task, - column, - paused: true, - status: undefined, - error: null, - }); - const executor = new TaskExecutor(store, "/tmp/test", {}); + function advancedColumnTask(): Task { + return { + id: "FN-001", + title: "Test", + description: "Test", + column: "in-progress", + status: undefined, + dependencies: [], + steps: [{ name: "Preflight", status: "pending" }], + currentStep: 0, + log: [{ timestamp: new Date().toISOString(), action: "Resuming execution after unpause" }], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as Task; + } - await (executor as any).handleGraphFailure(task, { - visitedNodeIds: ["execute"], - }); + // FN-6782: a paused graph exit that already landed back in `todo` is BENIGN — + // it must NOT be parked `failed` (that re-fail loop was the retry storm). It + // logs a benign line, clears the pause-abort marker, and leaves the task in + // todo for normal scheduling. (Previously this was surfaced as an + // operator-action failure; see the `done` case below for the still-surfaced path.) + it("treats a paused graph exit re-queued to todo as benign without parking failed", async () => { + const store = createMockStore(); + const task = advancedColumnTask(); + store.getTask.mockResolvedValue({ ...task, column: "todo", paused: true, status: undefined, error: null }); + const executor = new TaskExecutor(store, "/tmp/test", {}); - const expectedMessage = `Workflow graph failure surfaced after paused task pause in '${column}' at node 'execute' — operator action required; retry or explicitly unpause/resume after inspecting the task`; - expect(store.logEntry).toHaveBeenCalledWith("FN-001", expectedMessage, undefined, undefined); - if (column === "done") { - expect(store.updateTask).not.toHaveBeenCalledWith( - "FN-001", - expect.objectContaining({ status: "failed" }), - expect.anything(), - ); - } else { - expect(store.updateTask).toHaveBeenCalledWith("FN-001", { error: expectedMessage, status: "failed" }, undefined); - } - expect(store.moveTask).not.toHaveBeenCalled(); - expect(store.handoffToReview).not.toHaveBeenCalled(); - }, - ); + await (executor as any).handleGraphFailure(task, { visitedNodeIds: ["execute"] }); + + const benignMessage = "Workflow graph run ended during task pause with task re-queued to todo — benign, cleared for normal scheduling"; + expect(store.logEntry).toHaveBeenCalledWith("FN-001", benignMessage, undefined, undefined); + expect(store.updateTask).not.toHaveBeenCalledWith( + "FN-001", + expect.objectContaining({ status: "failed" }), + expect.anything(), + ); + expect(store.moveTask).not.toHaveBeenCalled(); + expect(store.handoffToReview).not.toHaveBeenCalled(); + }); + + it("surfaces a paused graph exit in an already-advanced done column without parking failed", async () => { + const store = createMockStore(); + const task = advancedColumnTask(); + store.getTask.mockResolvedValue({ ...task, column: "done", paused: true, status: undefined, error: null }); + const executor = new TaskExecutor(store, "/tmp/test", {}); + + await (executor as any).handleGraphFailure(task, { visitedNodeIds: ["execute"] }); + + const expectedMessage = "Workflow graph failure surfaced after paused task pause in 'done' at node 'execute' — operator action required; retry or explicitly unpause/resume after inspecting the task"; + expect(store.logEntry).toHaveBeenCalledWith("FN-001", expectedMessage, undefined, undefined); + // done/archived are terminal — surfaced via log only, never parked failed. + expect(store.updateTask).not.toHaveBeenCalledWith( + "FN-001", + expect.objectContaining({ status: "failed" }), + expect.anything(), + ); + expect(store.moveTask).not.toHaveBeenCalled(); + expect(store.handoffToReview).not.toHaveBeenCalled(); + }); describe("merge-seam abort classification (FN-6568)", () => { /*