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
This commit is contained in:
5
.changeset/fn-6625-finalize-to-review-abort.md
Normal file
5
.changeset/fn-6625-finalize-to-review-abort.md
Normal file
@@ -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.
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -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<string, "global-pause" | "merge-seam" | "hard-cancel">();
|
||||
private pausedAbortProvenance = new Map<string, "global-pause" | "merge-seam" | "hard-cancel" | "completion-finalize">();
|
||||
/** Tasks that had a dependency added mid-execution (abort + discard worktree). */
|
||||
private depAborted = new Set<string>();
|
||||
/** 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<string>();
|
||||
|
||||
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<boolean> {
|
||||
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 {
|
||||
|
||||
Reference in New Issue
Block a user