Address PR #1687 review: harden pause-abort recovery + FNXC comments

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) <noreply@anthropic.com>
2026-06-19 20:10:50 -07:00
parent 8e4b9e4f0f
commit 7eaf513944
6 changed files with 105 additions and 35 deletions
--- 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);
  });

--- 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<string>(executing),
-      clearPhantomExecutorBinding,
+      clearPhantomExecutorBinding: clearPhantomExecutorBinding as (taskId: string) => boolean | void,
    });
  }

--- 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<string>(),
-      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<typeof vi.fn>).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<string>(),
+    });
+
+    const recovered = await manager.recoverPausedAbortFailures();
+
+    expect(recovered).toBe(0);
+    expect(store.updateTask).not.toHaveBeenCalled();
+    expect(store.moveTask).not.toHaveBeenCalled();
+  });
 });
--- 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) {
--- 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<string, number> }
--- 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<number> {
    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<string>();
      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<string>();
+          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);