From 3b32b535d05416f550f58393cb1eaa9bd66886e7 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 18 Jun 2026 11:18:03 -0700 Subject: [PATCH] FN-6644: preserve finalized graph aborts Keep no-commit completion handoffs in review when teardown reclassifies their abort provenance. - Track completed finalize-to-review handoffs with a durable executor marker. - Suppress false operator-action graph failures after hard-cancel teardown overwrites completion-finalize provenance. - Cover preserved user/global pause, merge-seam, hard-cancel, terminal, and redispatch cleanup behavior. - Document the finalized completion abort exception and add a patch changeset. Files changed: .changeset/fn-6644-finalize-to-review-abort-overwrite.md | 5 + docs/architecture.md | 2 +- packages/engine/src/__tests__/executor-recovery.test.ts | 300 +++++++++++++++++++++ packages/engine/src/executor.ts | 40 ++- 4 files changed, 342 insertions(+), 5 deletions(-) Fusion-Task-Id: FN-6644 Fusion-Task-Lineage: 569fa84f-2cbe-45ae-b12c-874dd45cea73 --- ...6644-finalize-to-review-abort-overwrite.md | 5 + docs/architecture.md | 2 +- .../src/__tests__/executor-recovery.test.ts | 300 ++++++++++++++++++ packages/engine/src/executor.ts | 40 ++- 4 files changed, 342 insertions(+), 5 deletions(-) create mode 100644 .changeset/fn-6644-finalize-to-review-abort-overwrite.md diff --git a/.changeset/fn-6644-finalize-to-review-abort-overwrite.md b/.changeset/fn-6644-finalize-to-review-abort-overwrite.md new file mode 100644 index 0000000000..06d1d4f52f --- /dev/null +++ b/.changeset/fn-6644-finalize-to-review-abort-overwrite.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Completed/no-commit executions that finalize to review no longer get re-parked failed when later teardown overwrites completion-finalize abort provenance with a hard-cancel marker. Genuine user/global pauses, merge-seam retry routing, and active-execution hard-cancel behavior are preserved. diff --git a/docs/architecture.md b/docs/architecture.md index 00e46335da..b6cf163080 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1279,7 +1279,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. 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. +- 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 completed/no-commit finalize-to-review teardown (FN-6625/FN-6644): once `handoffTaskToReview(..., "paused-after-completion")` records durable completion-finalized state, a trailing graph abort resolves as an already-advanced benign graph exit even if later teardown re-marked the abort provenance from `completion-finalize` to `hard-cancel`. Genuine `userPaused`/global-pause exits and active-execution hard-cancels still use the operator-action path. `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 0bdca49027..e079724db5 100644 --- a/packages/engine/src/__tests__/executor-recovery.test.ts +++ b/packages/engine/src/__tests__/executor-recovery.test.ts @@ -1148,6 +1148,58 @@ describe("TaskExecutor bounded recovery retries", () => { expect(store.handoffToReview).not.toHaveBeenCalled(); }); + it("treats completion-finalize graph exits as benign after teardown re-marks hard-cancel (FN-6644)", 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).markCompletionFinalized("FN-001"); + + await (executor as any).awaitAbortInFlightTaskWork("FN-001", "completion-finalize teardown after handoff"); + expect((executor as any).pausedAbortProvenance.get("FN-001")).toBe("hard-cancel"); + + 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 = [ @@ -1339,6 +1391,254 @@ describe("TaskExecutor bounded recovery retries", () => { }); + describe("completion-finalize hard-cancel overwrite classification (FN-6644)", () => { + /* + Surface Enumeration coverage: + - Classifier branch: the overwrite-sequence tests drive `handleGraphFailure` after durable completion-finalized state survives a later `hard-cancel` re-mark and assert the benign already-advanced branch. + - Provenance-overwrite ordering: each benign case calls `markCompletionFinalized(...)` first, then `awaitAbortInFlightTaskWork(...)`, proving `completion-finalize → hard-cancel` cannot re-park a finalized row failed. + - Abort provenance sources: hard-cancel overwrite is reproduced here; user pause/global pause/merge-seam companion tests prove their existing provenance categories still win. + - Both completion-finalize paths: production uses the same `markCompletionFinalized(...)` helper at the graceful-session-exit and finally-block `handoffTaskToReview(task, "paused-after-completion")` sites. + - Failed-node identity: benign overwrite coverage uses both `execute` and a non-execute node so the fix is keyed on durable completion state rather than node id. + - Column/progress states: finalized in-review and already-terminal done rows are benign; active in-progress hard-cancel remains pause-preserved; pending-step in-review hard-cancel remains operator-action failure. + - Data states: userPaused true, global-pause, hard-cancel overwrite after completion, genuine hard-cancel before completion, and merge-seam retry routing are asserted without weakening the already-status/error guard above. + - Shared hooks / cleanup sites: `clearPausedAborted(...)` and `execute(...)` clear the durable marker; the final test asserts a new dispatch drops stale suppression state. + - No leftover shells: durable completion state is in-memory only and is cleared on re-dispatch/backward cleanup, preventing a later genuine run from inheriting suppression. + */ + const makeCompletedTask = (overrides: Partial = {}) => ({ + id: "FN-001", + title: "Test", + description: "Test", + column: "in-progress", + status: undefined, + dependencies: [], + steps: [ + { name: "Preflight", status: "done" }, + { name: "Verify", status: "done" }, + ], + currentStep: 1, + log: [{ timestamp: new Date().toISOString(), action: "Execution paused after completion — finalizing to in-review" }], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + ...overrides, + }) as Task; + + it.each(["execute", "verifySentinel"] as const)( + "treats finalized-completion graph exits as benign after hard-cancel overwrite at node %s", + async (nodeId) => { + const store = createMockStore(); + const task = makeCompletedTask(); + 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).markCompletionFinalized("FN-001"); + + await (executor as any).awaitAbortInFlightTaskWork("FN-001", "completion-finalize teardown after handoff"); + expect((executor as any).pausedAbortProvenance.get("FN-001")).toBe("hard-cancel"); + + await (executor as any).handleGraphFailure(task, { + disposition: "failed", + outcome: "failure", + visitedNodeIds: [nodeId], + }); + + 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(); + }, + ); + + it("keeps already-terminal finalized-completion rows benign after hard-cancel overwrite", async () => { + const store = createMockStore(); + const task = makeCompletedTask(); + store.getTask.mockResolvedValue({ + ...task, + column: "done", + paused: false, + userPaused: false, + status: undefined, + error: null, + }); + const executor = new TaskExecutor(store, "/tmp/test", {}); + (executor as any).markCompletionFinalized("FN-001"); + await (executor as any).awaitAbortInFlightTaskWork("FN-001", "completion-finalize teardown after done handoff"); + + 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 'done' — no further action needed"); + 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(); + }); + + it("preserves explicit user-pause parking even when durable completion state exists", async () => { + const store = createMockStore(); + const task = makeCompletedTask(); + store.getTask.mockResolvedValue({ + ...task, + column: "in-review", + paused: true, + userPaused: true, + status: undefined, + error: null, + }); + const executor = new TaskExecutor(store, "/tmp/test", {}); + (executor as any).markCompletionFinalized("FN-001"); + await (executor as any).awaitAbortInFlightTaskWork("FN-001", "completion-finalize teardown after handoff"); + + await (executor as any).handleGraphFailure(task, { + disposition: "failed", + outcome: "failure", + visitedNodeIds: ["execute"], + }); + + const expectedMessage = "Workflow graph failure surfaced after paused explicit user pause in 'in-review' at node 'execute' — operator action required; retry or explicitly unpause/resume after inspecting the task"; + expect(store.logEntry).toHaveBeenCalledWith("FN-001", expectedMessage, undefined, undefined); + expect(store.updateTask).toHaveBeenCalledWith("FN-001", { error: expectedMessage, status: "failed" }, undefined); + expect(store.moveTask).not.toHaveBeenCalled(); + }); + + it("preserves global-pause parking even when durable completion state exists", async () => { + const store = createMockStore(); + const task = makeCompletedTask(); + 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).markCompletionFinalized("FN-001"); + (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"; + expect(store.updateTask).toHaveBeenCalledWith("FN-001", { error: expectedMessage, status: "failed" }, undefined); + expect(store.moveTask).not.toHaveBeenCalled(); + }); + + it("preserves merge-seam retry routing when merge provenance coexists with stale durable completion state", async () => { + const store = createMockStore(); + const task = makeCompletedTask(); + store.getTask.mockResolvedValue({ + ...task, + column: "in-review", + paused: false, + userPaused: false, + status: undefined, + error: null, + mergeRetries: null, + }); + const executor = new TaskExecutor(store, "/tmp/test", {}); + const mergeRequester = vi.fn(async () => ({ merged: false, noOp: false, reason: "merge-conflict" })); + executor.setMergeRequester(mergeRequester as any); + (executor as any).markCompletionFinalized("FN-001"); + (executor as any).markPausedAborted("FN-001", "merge-seam"); + + await (executor as any).handleGraphFailure(task, { + disposition: "failed", + outcome: "failure", + visitedNodeIds: ["requestMerge"], + }); + + const messages = store.logEntry.mock.calls.map((call) => call[1]).join("\n"); + expect(messages).toContain("Workflow graph merge failure at node 'requestMerge' routed to bounded auto-merge retry after merge-seam abort"); + expect(messages).not.toContain("operator action required"); + expect(mergeRequester).toHaveBeenCalledWith("FN-001"); + expect(store.updateTask).not.toHaveBeenCalledWith( + "FN-001", + expect.objectContaining({ status: "failed" }), + expect.anything(), + ); + }); + + it("preserves genuine pending-step hard-cancel parking when no completion finalize occurred", async () => { + const store = createMockStore(); + const task = makeCompletedTask({ + steps: [{ name: "Preflight", status: "pending" }], + currentStep: 0, + log: [{ timestamp: new Date().toISOString(), action: "Resuming execution after unpause" }], + }); + 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", "hard-cancel"); + + await (executor as any).handleGraphFailure(task, { + disposition: "failed", + outcome: "failure", + visitedNodeIds: ["execute"], + }); + + const expectedMessage = "Workflow graph failure surfaced after paused engine abort during pause/resume in 'in-review' at node 'execute' — operator action required; retry or explicitly unpause/resume after inspecting the task"; + expect(store.updateTask).toHaveBeenCalledWith("FN-001", { error: expectedMessage, status: "failed" }, undefined); + expect(store.moveTask).not.toHaveBeenCalled(); + }); + + it("clears durable completion state on new execution dispatch so suppression cannot leak across runs", async () => { + const store = createMockStore(); + const task = makeCompletedTask({ steps: [{ name: "Preflight", status: "pending" }], currentStep: 0 }); + store.getTask.mockResolvedValue({ + ...task, + column: "in-review", + paused: false, + userPaused: false, + status: undefined, + error: null, + }); + const executor = new TaskExecutor(store, "/tmp/test", { + workflowAuthoritativeDispatch: async () => true, + }); + (executor as any).markCompletionFinalized("FN-001"); + await executor.execute(task); + (executor as any).markPausedAborted("FN-001", "hard-cancel"); + + await (executor as any).handleGraphFailure(task, { + disposition: "failed", + outcome: "failure", + visitedNodeIds: ["execute"], + }); + + const expectedMessage = "Workflow graph failure surfaced after paused engine abort during pause/resume in 'in-review' at node 'execute' — operator action required; retry or explicitly unpause/resume after inspecting the task"; + expect((executor as any).completionFinalizedTaskIds.has("FN-001")).toBe(false); + expect(store.updateTask).toHaveBeenCalledWith("FN-001", { error: expectedMessage, status: "failed" }, undefined); + }); + }); + 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 d606f27544..6410a42005 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -1487,6 +1487,11 @@ export class TaskExecutor { * 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(); + /** + * FNXC:WorkflowLifecycle 2026-06-18-10:56: + * FN-6644 makes completed/no-commit finalize-to-review state durable beyond volatile pause provenance. FN-6641 showed FN-6625 was incomplete because teardown can re-mark `completion-finalize` as `hard-cancel`; this marker keeps the already-finalized handoff from being re-parked as an operator-action pause abort while preserving genuine live pauses and active hard-cancels. + */ + private completionFinalizedTaskIds = new Set(); /** 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). */ @@ -1515,9 +1520,15 @@ export class TaskExecutor { this.pausedAbortProvenance.set(taskId, provenance); } + private markCompletionFinalized(taskId: string): void { + this.markPausedAborted(taskId, "completion-finalize"); + this.completionFinalizedTaskIds.add(taskId); + } + private clearPausedAborted(taskId: string): void { this.pausedAborted.delete(taskId); this.pausedAbortProvenance.delete(taskId); + this.completionFinalizedTaskIds.delete(taskId); } private setActiveSession(taskId: string, sessionState: ActiveExecutorSessionState, worktreePath: string): void { @@ -6440,12 +6451,26 @@ export class TaskExecutor { 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 completionFinalized = completionFinalizeAborted || this.completionFinalizedTaskIds.has(task.id); + /* + 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. + + FNXC:WorkflowLifecycle 2026-06-18-10:57: + FN-6644: a completed/no-commit execution that already finalized to in-review must not be re-parked as an operator-action pause abort when later teardown overwrites FN-6625 `completion-finalize` provenance with `hard-cancel` (FN-6641). Only suppress the pause-abort branch for already-finalized, non-in-progress rows with no live user/global pause; active execution hard-cancel and genuine pause/global-pause still park or preserve exactly as before. + */ + const suppressFinalizedCompletionAbort = Boolean( + completionFinalized + && live.column !== "in-progress" + && !live.userPaused + && live.paused !== true + && abortProvenance !== "global-pause" + && !mergeSeamAborted, + ); const genuinePauseAbort = Boolean( live.userPaused || abortProvenance === "global-pause" || (live.paused && !mergeSeamAborted) - || (pausedAborted && !mergeSeamAborted && !completionFinalizeAborted), + || (pausedAborted && !mergeSeamAborted && !completionFinalizeAborted && !suppressFinalizedCompletionAbort), ); if (genuinePauseAbort) { /* @@ -6671,6 +6696,7 @@ export class TaskExecutor { } async execute(task: Task): Promise { + this.completionFinalizedTaskIds.delete(task.id); // Workflow graph interpreter routing (cutover M-C): graph-selected tasks // are orchestrated by the interpreter. The execute seam re-enters this // method with a completion interceptor registered (which claims the task @@ -8154,8 +8180,11 @@ export class TaskExecutor { /* 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. + + FNXC:WorkflowLifecycle 2026-06-18-10:58: + FN-6644/FN-6641: the graceful-session-exit handoff must also record durable completed-finalize state because a later teardown can re-mark the abort as `hard-cancel`. The classifier uses that durable handoff marker, not the volatile provenance alone, to keep completed no-commit tasks from being re-parked failed. */ - this.markPausedAborted(task.id, "completion-finalize"); + this.markCompletionFinalized(task.id); await this.handoffTaskToReview(task, "paused-after-completion"); this.clearCompletedTaskWatchdog(task.id); this.options.onComplete?.(task); @@ -8705,8 +8734,11 @@ export class TaskExecutor { /* 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. + + FNXC:WorkflowLifecycle 2026-06-18-10:59: + FN-6644/FN-6641: the finally-block handoff must record durable completed-finalize state because a later teardown can overwrite provenance to `hard-cancel`. The classifier must still resolve that completed no-commit tail failure benignly without weakening genuine pause or active hard-cancel behavior. */ - this.markPausedAborted(task.id, "completion-finalize"); + this.markCompletionFinalized(task.id); await this.handoffTaskToReview(task, "paused-after-completion"); this.options.onComplete?.(task); } else {