From 98ccf8a2938807f497c482832be26251bcb088b6 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 18 Jun 2026 00:01:32 -0700 Subject: [PATCH] FN-6625: classify completion finalization aborts Prevent completed no-commit executions that already advanced to review from being re-parked as pause-abort failures. - Add completion-finalize pause-abort provenance and exclude it from genuine pause handling after review handoff. - Mark paused-after-completion finalization paths with the new provenance before handing tasks to review. - Cover the finalize-to-review abort recovery path with executor regression tests and document the lifecycle exception. - Add a patch changeset for the published Fusion package. Files changed: .changeset/fn-6625-finalize-to-review-abort.md | 5 + docs/architecture.md | 2 +- .../engine/src/__tests__/executor-recovery.test.ts | 158 ++++++++++++++++++++- packages/engine/src/executor.ts | 26 +++- 4 files changed, 185 insertions(+), 6 deletions(-) Fusion-Task-Id: FN-6625 Fusion-Task-Lineage: 728f6fe5-4c27-4597-b17e-e16ff97b9277 --- .../fn-6625-finalize-to-review-abort.md | 5 + docs/architecture.md | 2 +- .../src/__tests__/executor-recovery.test.ts | 158 +++++++++++++++++- packages/engine/src/executor.ts | 26 ++- 4 files changed, 185 insertions(+), 6 deletions(-) create mode 100644 .changeset/fn-6625-finalize-to-review-abort.md diff --git a/.changeset/fn-6625-finalize-to-review-abort.md b/.changeset/fn-6625-finalize-to-review-abort.md new file mode 100644 index 0000000000..85373cc7d8 --- /dev/null +++ b/.changeset/fn-6625-finalize-to-review-abort.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Completed no-commit executions that finalize to in-review are no longer re-parked as failed "engine abort during pause/resume" operator-action graph failures; genuine pause and hard-cancel semantics are preserved. diff --git a/docs/architecture.md b/docs/architecture.md index 9e922b9b81..6ce2ffd493 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1277,7 +1277,7 @@ The columns/traits track moved *board* policy (transitions, capacity, hold, merg - A `parse-steps` node reads a workflow-declared **artifact** (PROMPT.md is just the default workflow's declared `step-source` artifact) and runs a registry **parser** (`step-headings`, `json-steps`, or a plugin-contributed parser) to write `Task.steps[]`. It is the only graph-side step-list writer and must dominate any `foreach`. Parsers fail closed to a routable `outcome:parse-error`. - A `foreach(source:"task-steps")` node instantiates an inline template subgraph once per planned step, with `mode` (sequential/parallel) and `isolation` (shared/worktree) as explicit axes and per-instance run-state pinned + persisted for crash-safe resume. - Resume-limbo graph failures are retried only through a narrow persisted counter (`Task.graphResumeRetryCount`, max 2). The executor classifies a failure as transient only when it happens immediately after the engine restart/unpause resume log marker, reports no graph `reason`, has no completed step progress, and the task has no durable `lastError`/`failureReason`; it clears transient `status`/`error`, logs the auto-retry, and schedules one more graph execution. Any explicit graph reason, completed step progress, durable task error, missing resume marker, or exhausted counter remains a genuine `status:"failed"` disposition and goes to review handoff, preserving the FN-5704 anti-loop contract. -- Paused graph exits are benign only while the task is still in `in-progress`; that is the user-pause/engine-pause state where preserving the pause without requeueing is intentional. If the graph reports a pause/abort exit after the task has already advanced to another live column (for example `in-review` after an unpause/resume race), `TaskExecutor.handleGraphFailure()` surfaces the boundary as operator-actionable failure evidence (`status:"failed"`/`error` when no failure is already present, plus a task-log entry) and does **not** move, rewind, or auto-merge the task. `done` and `archived` remain terminal and keep their column/status, while existing failure details are preserved. +- Paused graph exits are benign only while the task is still in `in-progress`; that is the user-pause/engine-pause state where preserving the pause without requeueing is intentional. If the graph reports a pause/abort exit after the task has already advanced to another live column (for example `in-review` after an unpause/resume race), `TaskExecutor.handleGraphFailure()` surfaces the boundary as operator-actionable failure evidence (`status:"failed"`/`error` when no failure is already present, plus a task-log entry) and does **not** move, rewind, or auto-merge the task. The exception is FN-6625 `completion-finalize` provenance: a completed/no-commit execution whose teardown abort arrives after `handoffTaskToReview(..., "paused-after-completion")` resolves as an already-advanced benign graph exit instead of being re-parked failed. `done` and `archived` remain terminal and keep their column/status, while existing failure details are preserved. - A `step-review` node surfaces reviewer verdicts (APPROVE/REVISE/RETHINK/UNAVAILABLE) as outcome edges; `rework` edges (the only legal graph cycles, bounded per instance) route REVISE/RETHINK back to `step-execute`, with RETHINK traversal triggering the reset seam. - A `code` node runs sandboxed TypeScript (esbuild + child process, clamped timeout, no store handle) for arbitrary computed routing/field logic — the same trust tier as project-local script steps. diff --git a/packages/engine/src/__tests__/executor-recovery.test.ts b/packages/engine/src/__tests__/executor-recovery.test.ts index d6308c1b3c..0bdca49027 100644 --- a/packages/engine/src/__tests__/executor-recovery.test.ts +++ b/packages/engine/src/__tests__/executor-recovery.test.ts @@ -1087,7 +1087,68 @@ describe("TaskExecutor bounded recovery retries", () => { expect(store.handoffToReview).not.toHaveBeenCalled(); }); - it("surfaces pausedAborted in-review graph exits as workflow failures", async () => { + describe("completion-finalize abort classification (FN-6625)", () => { + /* + Surface Enumeration coverage: + - Classifier branch: completion-finalize provenance bypasses operator-action parking while hard-cancel, userPaused, and global-pause coverage remains in this suite. + - Abort provenance sources: the new completion-finalize value is asserted here; FN-6568 below covers merge-seam/global-pause and the hard-cancel test in this block preserves generic operator-cancel behavior. + - Completion-finalize paths: executor.ts marks both graceful-session-exit and finally-block handoffTaskToReview("paused-after-completion") sites; this direct classifier test reproduces the shared trailing graph failure. + - Failed-node identity: the symptom uses execute, but the production predicate keys on provenance/completion state, not a node-id allow-list. + - Column/progress states: in-review finalized-completion is benign; existing adjacent tests cover in-progress pause preservation and done/todo non-execution exits. + - Data states: userPaused true is covered above, paused true without userPaused is covered below, completion-finalize and hard-cancel are covered here, global-pause/merge-seam are covered in the FN-6568 block, and already-status/error-set guard is preserved here. + - Preserved semantics: genuine user/global pause parking, merge-seam retry routing, and genuine hard-cancel parking remain asserted without backward moveTask calls. + - No leftover shells: completion-finalize is stored in pausedAbortProvenance and cleared through the existing clearPausedAborted helper used by every cleanup site. + */ + it("treats completion-finalize pausedAborted in-review graph exits as benign", async () => { + const store = createMockStore(); + const steps = [ + { name: "Preflight", status: "done" }, + { name: "Implement", status: "done" }, + ]; + const task = { + id: "FN-001", + title: "Test", + description: "Test", + column: "in-progress", + status: undefined, + dependencies: [], + steps, + currentStep: 1, + log: [{ timestamp: new Date().toISOString(), action: "Execution paused after completion — finalizing to in-review" }], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as Task; + store.getTask.mockResolvedValue({ + ...task, + column: "in-review", + paused: false, + userPaused: false, + status: undefined, + error: null, + }); + const executor = new TaskExecutor(store, "/tmp/test", {}); + (executor as any).markPausedAborted("FN-001", "completion-finalize"); + + await (executor as any).handleGraphFailure(task, { + disposition: "failed", + outcome: "failure", + visitedNodeIds: ["execute"], + }); + + const messages = store.logEntry.mock.calls.map((call) => call[1]).join("\n"); + expect(messages).toContain("Workflow graph run ended after task already advanced to 'in-review' — no further action needed"); + expect(messages).not.toContain("engine abort during pause/resume"); + expect(messages).not.toContain("operator action required"); + 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 genuine hard-cancel pausedAborted in-review graph exits as workflow failures", async () => { const store = createMockStore(); const steps = [ { name: "Preflight", status: "pending" }, @@ -1183,6 +1244,101 @@ describe("TaskExecutor bounded recovery retries", () => { expect(store.handoffToReview).not.toHaveBeenCalled(); }); + it("preserves global-pause provenance as operator-action parking for execute-node in-review graph exits", async () => { + const store = createMockStore(); + const task = { + id: "FN-001", + title: "Test", + description: "Test", + column: "in-progress", + status: undefined, + dependencies: [], + steps: [{ name: "Preflight", status: "done" }], + currentStep: 1, + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as Task; + store.getTask.mockResolvedValue({ + ...task, + column: "in-review", + paused: false, + userPaused: false, + status: undefined, + error: null, + }); + const executor = new TaskExecutor(store, "/tmp/test", {}); + (executor as any).markPausedAborted("FN-001", "global-pause"); + + await (executor as any).handleGraphFailure(task, { + disposition: "failed", + outcome: "failure", + visitedNodeIds: ["execute"], + }); + + const expectedMessage = "Workflow graph failure surfaced after paused global pause in 'in-review' at node 'execute' — operator action required; retry or explicitly unpause/resume after inspecting the task"; + const messages = store.logEntry.mock.calls.map((call) => call[1]).join("\n"); + expect(messages).toContain("global pause"); + expect(messages).toContain("operator action required"); + expect(store.updateTask).toHaveBeenCalledWith("FN-001", { error: expectedMessage, status: "failed" }, undefined); + expect(store.moveTask).not.toHaveBeenCalled(); + expect(store.handoffToReview).not.toHaveBeenCalled(); + }); + + it("keeps genuine in-progress hard-cancel aborts active and pause-preserved", async () => { + 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: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as Task; + store.getTask.mockResolvedValue({ + ...task, + column: "in-progress", + paused: false, + userPaused: false, + status: undefined, + error: null, + }); + const abort = vi.fn().mockResolvedValue(undefined); + const dispose = vi.fn(); + const executor = new TaskExecutor(store, "/tmp/test", {}); + (executor as any).activeSessions.set("FN-001", { session: { abort, dispose, state: {} } }); + + await (executor as any).awaitAbortInFlightTaskWork("FN-001", "user move in-progress to todo", { userCanceled: true }); + await (executor as any).handleGraphFailure(task, { + disposition: "failed", + outcome: "failure", + visitedNodeIds: ["execute"], + }); + + expect(abort).toHaveBeenCalledTimes(1); + expect(dispose).toHaveBeenCalledTimes(1); + expect((executor as any).userCanceledTaskIds.has("FN-001")).toBe(true); + expect((executor as any).pausedAbortProvenance.get("FN-001")).toBe("hard-cancel"); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-001", + "Workflow graph run ended while task is paused — pause state preserved", + undefined, + undefined, + ); + expect(store.updateTask).not.toHaveBeenCalledWith( + "FN-001", + expect.objectContaining({ status: "failed" }), + expect.anything(), + ); + }); + + }); + it("keeps genuine in-progress user pauses benign even with partial step progress", async () => { const store = createMockStore(); const task = { diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 573c4b15b9..f0d4118321 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -1482,8 +1482,11 @@ export class TaskExecutor { /** * FNXC:WorkflowLifecycle 2026-06-17-03:42: * FN-6568 separates pause provenance from the legacy pausedAborted hard-cancel bit. Merge-seam/internal aborts caused FN-6528/FN-6531/FN-6534/FN-6537 to look like pause/resume aborts and left mergeRetries=NULL, so handleGraphFailure must know whether the abort came from global pause, the merge seam, or a generic hard cancel before choosing operator-action parking. + * + * FNXC:WorkflowLifecycle 2026-06-17-23:31: + * FN-6625 adds completion-finalize provenance for the FN-6614 symptom where a completed/no-commit execution already handed off to in-review, then a trailing graph abort looked like a pause/resume engine abort and re-parked the task failed. Completion-finalize is sibling provenance to FN-6568 merge-seam, not operator pause intent. */ - private pausedAbortProvenance = new Map(); + private pausedAbortProvenance = new Map(); /** Tasks that had a dependency added mid-execution (abort + discard worktree). */ private depAborted = new Set(); /** Tasks killed by stuck task detector. Value = shouldRequeue (budget not exhausted). */ @@ -1507,7 +1510,7 @@ export class TaskExecutor { /** Set of ephemeral spawned agent IDs with in-flight cleanup (prevents duplicate deletion attempts). */ private pendingEphemeralDeletions = new Set(); - private markPausedAborted(taskId: string, provenance: "global-pause" | "merge-seam" | "hard-cancel" = "hard-cancel"): void { + private markPausedAborted(taskId: string, provenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" = "hard-cancel"): void { this.pausedAborted.add(taskId); this.pausedAbortProvenance.set(taskId, provenance); } @@ -6407,7 +6410,7 @@ export class TaskExecutor { private async routeGraphMergeFailureToRetry( live: TaskDetail, result: WorkflowGraphTaskRunResult, - abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | undefined, + abortProvenance: "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize" | undefined, ): Promise { if (!this.mergeRequester) return false; const failedNode = result.visitedNodeIds[result.visitedNodeIds.length - 1] ?? "unknown"; @@ -6436,11 +6439,13 @@ export class TaskExecutor { const pausedAborted = this.pausedAborted.has(task.id); const abortProvenance = this.pausedAbortProvenance.get(task.id); const mergeSeamAborted = abortProvenance === "merge-seam"; + const completionFinalizeAborted = abortProvenance === "completion-finalize"; + // FNXC:WorkflowLifecycle 2026-06-17-23:39: A real live pause still parks even if stale provenance says completion-finalize; completed handoff rows are expected to be unpaused. const genuinePauseAbort = Boolean( live.userPaused || abortProvenance === "global-pause" || (live.paused && !mergeSeamAborted) - || (pausedAborted && !mergeSeamAborted), + || (pausedAborted && !mergeSeamAborted && !completionFinalizeAborted), ); if (genuinePauseAbort) { /* @@ -6449,6 +6454,9 @@ export class TaskExecutor { FNXC:WorkflowLifecycle 2026-06-17-03:48: FN-6568: merge-seam aborts are not pause provenance. A non-paused merge-node failure must bypass this operator-action pause branch so FN-6528/FN-6531/FN-6534/FN-6537-style failures route to bounded auto-merge retry instead of being parked failed with mergeRetries=NULL. + + FNXC:WorkflowLifecycle 2026-06-17-23:32: + FN-6625: completion-finalize aborts are teardown artifacts after a completed/no-commit execution has already advanced to in-review. Without excluding that provenance, the FN-6614 execute-node tail failure was mislabeled as an operator-action pause abort and re-parked failed. */ const pauseProvenance = live.userPaused ? "explicit user pause" @@ -8140,6 +8148,11 @@ export class TaskExecutor { executorLog.log(`${task.id} paused after completion (graceful session exit) — finalizing to in-review`); await this.store.logEntry(task.id, "Execution paused after completion — finalizing to in-review"); await this.persistTokenUsage(task.id); + /* + FNXC:WorkflowLifecycle 2026-06-17-23:33: + FN-6625: the completed/no-commit handoff may dispose graph execution after the task is already in-review. Mark that abort as completion-finalize so a trailing FN-6614-style graph failure resolves benignly instead of looking like a user/global pause; FN-6568 uses the same provenance seam for merge aborts. + */ + this.markPausedAborted(task.id, "completion-finalize"); await this.handoffTaskToReview(task, "paused-after-completion"); this.clearCompletedTaskWatchdog(task.id); this.options.onComplete?.(task); @@ -8686,6 +8699,11 @@ export class TaskExecutor { executorLog.log(`${task.id} paused after completion — finalizing to in-review`); await this.store.logEntry(task.id, "Execution paused after completion — finalizing to in-review", undefined, this.getRunContextFor(task.id)); await this.persistTokenUsage(task.id); + /* + FNXC:WorkflowLifecycle 2026-06-17-23:33: + FN-6625: the completed/no-commit handoff may dispose graph execution after the task is already in-review. Mark that abort as completion-finalize so a trailing FN-6614-style graph failure resolves benignly instead of looking like a user/global pause; FN-6568 uses the same provenance seam for merge aborts. + */ + this.markPausedAborted(task.id, "completion-finalize"); await this.handoffTaskToReview(task, "paused-after-completion"); this.options.onComplete?.(task); } else {