Merge pull request #1697 from Runfusion/fix/engine-pause-resume-auto-continue
fix(engine): auto-continue agent session after engine-internal pause/resume abort
This commit is contained in:
5
.changeset/auto-continue-engine-pause-abort.md
Normal file
5
.changeset/auto-continue-engine-pause-abort.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
Auto-continue the agent session after an engine-internal pause/resume abort instead of re-queueing the task to todo. When the engine tears down in-flight work (hard-cancel) and the workflow graph run ends with the task back in `todo`, the executor now retries the agent session in place — bounded by the existing graph-resume retry budget with backoff, falling back to a benign re-queue only after retries are exhausted. Before re-dispatching, it re-checks the task at fire time and aborts the auto-continue if the task was paused, moved, or deleted during the backoff window, so genuine user/global/task pauses are never resumed against the operator's intent. The transient reclassification clears any stale `failed` status and emits an `Auto-recovered:` log so no spurious failure notification fires.
|
||||
@@ -1,4 +1,4 @@
|
||||
import { beforeEach, describe, expect, it } from "vitest";
|
||||
import { afterEach, 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";
|
||||
@@ -31,7 +31,12 @@ function makeTask(overrides: Partial<TaskDetail> = {}): TaskDetail {
|
||||
} as TaskDetail;
|
||||
}
|
||||
|
||||
function makeHarness(taskOverrides: Partial<TaskDetail> = {}) {
|
||||
type AbortProvenance = "hard-cancel" | "global-pause" | "merge-seam" | "completion-finalize";
|
||||
|
||||
function makeHarness(
|
||||
taskOverrides: Partial<TaskDetail> = {},
|
||||
provenance: AbortProvenance = "hard-cancel",
|
||||
) {
|
||||
const store = createMockStore();
|
||||
const task = makeTask(taskOverrides);
|
||||
store.getTask.mockResolvedValue(task);
|
||||
@@ -43,7 +48,7 @@ function makeHarness(taskOverrides: Partial<TaskDetail> = {}) {
|
||||
maxAutoMergeRetries: 3,
|
||||
});
|
||||
const executor = new TaskExecutor(store, "/tmp/test", {});
|
||||
(executor as any).markPausedAborted(task.id, "hard-cancel");
|
||||
(executor as any).markPausedAborted(task.id, provenance);
|
||||
return { store, task, executor };
|
||||
}
|
||||
|
||||
@@ -56,6 +61,13 @@ async function invokeGraphFailure(executor: TaskExecutor, task: TaskDetail) {
|
||||
});
|
||||
}
|
||||
|
||||
// Flush the unref'd setTimeout that schedules the in-place retry plus the async
|
||||
// re-fetch + execute() chain inside it. Fake timers keep this deterministic
|
||||
// (no real wall-clock wait) per the repo's no-slow-tests rule (FN-5048).
|
||||
async function flushScheduledRetry() {
|
||||
await vi.advanceTimersByTimeAsync(10);
|
||||
}
|
||||
|
||||
function logText(store: ReturnType<typeof createMockStore>): string {
|
||||
return store.logEntry.mock.calls.map((call: unknown[]) => call[1]).join("\n");
|
||||
}
|
||||
@@ -63,50 +75,97 @@ function logText(store: ReturnType<typeof createMockStore>): string {
|
||||
describe("pause-abort benign requeue-to-todo (FN-6782)", () => {
|
||||
beforeEach(() => {
|
||||
resetExecutorMocks();
|
||||
vi.useFakeTimers();
|
||||
});
|
||||
|
||||
it("does NOT park a todo-column pause-abort as failed (no retry storm)", async () => {
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
it("auto-continues the agent session for an engine-internal abort instead of re-queueing to todo", async () => {
|
||||
// An "engine abort during pause/resume" (pausedAborted hard-cancel, no user/
|
||||
// global pause) is engine-internal churn, not an operator action — the
|
||||
// executor must retry the agent session in place rather than bouncing the
|
||||
// task through todo (and must not fire a failure notification).
|
||||
const { store, task, executor } = makeHarness({ column: "todo" });
|
||||
(executor as any).activeWorktrees.set(task.id, task.worktree);
|
||||
const executeSpy = vi
|
||||
.spyOn(executor as any, "execute")
|
||||
.mockResolvedValue(undefined);
|
||||
|
||||
await invokeGraphFailure(executor, task);
|
||||
|
||||
// FNXC:WorkflowLifecycle a todo pause-abort must NOT write status:"failed" — that was the storm trigger.
|
||||
// It must NOT park 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);
|
||||
// FNXC:WorkflowLifecycle the benign-clear log must surface for observability.
|
||||
expect(logText(store)).toContain("benign, cleared for normal scheduling");
|
||||
// FNXC:WorkflowLifecycle the pausedAborted marker must be cleared so the next dispatch starts clean.
|
||||
expect((executor as any).pausedAborted.has(task.id)).toBe(false);
|
||||
// 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);
|
||||
// FNXC:WorkflowLifecycle 2026-06-20-19:58 a clean todo row (no stale status/error)
|
||||
// must NOT trigger the reconciliation write — the `live.status != null ||
|
||||
// live.error != null` guard skips it so the common benign re-queue stays a no-op.
|
||||
const clearedClean = store.updateTask.mock.calls.some(
|
||||
// It auto-continues instead of logging the benign re-queue line.
|
||||
expect(logText(store)).toContain("auto-continuing the agent session (1/2)");
|
||||
expect(logText(store)).not.toContain("benign, cleared for normal scheduling");
|
||||
// The bounded retry budget is incremented and any stale failure cleared.
|
||||
const bumpedRetry = store.updateTask.mock.calls.some(
|
||||
(call: unknown[]) => {
|
||||
const patch = call[1] as { status?: unknown; error?: unknown } | undefined;
|
||||
return patch?.status === null && patch?.error === null;
|
||||
const patch = call[1] as { graphResumeRetryCount?: number; status?: unknown } | undefined;
|
||||
return patch?.graphResumeRetryCount === 1 && patch?.status === null;
|
||||
},
|
||||
);
|
||||
expect(clearedClean).toBe(false);
|
||||
expect(bumpedRetry).toBe(true);
|
||||
// An `Auto-recovered:`-prefixed log suppresses the failure notification.
|
||||
expect(logText(store)).toContain("Auto-recovered: engine-internal pause/resume abort");
|
||||
// The pause-abort marker is cleared and the worktree slot released.
|
||||
expect((executor as any).pausedAborted.has(task.id)).toBe(false);
|
||||
expect((executor as any).activeWorktrees.has(task.id)).toBe(false);
|
||||
// The agent session is re-executed in place after the backoff window.
|
||||
await flushScheduledRetry();
|
||||
expect(executeSpy).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("clears a stale failed status when reclassifying a todo pause-abort as benign (no lingering failure notification)", async () => {
|
||||
// FNXC:WorkflowLifecycle 2026-06-20-19:58 a pause-abort parked status:"failed"
|
||||
// on an earlier non-todo observation stays dispatchable (scheduler filters
|
||||
// column+paused, not status) and re-enters this branch in todo. The benign
|
||||
// reclassification must reconcile the row to status:null/error:null —
|
||||
// otherwise the persisted failure survives, the board shows it failed, and
|
||||
// the deferred failure notification fires despite the benign log.
|
||||
it.each([
|
||||
{ label: "paused", patch: { paused: true } },
|
||||
{ label: "user-paused", patch: { userPaused: true } },
|
||||
{ label: "moved out of todo", patch: { column: "in-progress" } },
|
||||
{ label: "deleted", patch: { deletedAt: "2026-06-21T00:00:00.000Z" } },
|
||||
])(
|
||||
"fire-time guard: aborts the auto-continue when the task became $label during the backoff window",
|
||||
async ({ patch }) => {
|
||||
// The auto-continue re-fetches the task just before re-executing and must
|
||||
// bail if the operator paused/moved/deleted it during the backoff window —
|
||||
// the direct execute() bypasses the scheduler's pause filter, so without
|
||||
// this guard it would resume work the user just parked. The initial
|
||||
// `live` snapshot is a clean todo (so the auto-continue branch is entered
|
||||
// and a retry scheduled); the task then changes state before the timer
|
||||
// fires, and the fire-time re-fetch must abort the dispatch.
|
||||
const { store, task, executor } = makeHarness({ column: "todo" });
|
||||
(executor as any).activeWorktrees.set(task.id, task.worktree);
|
||||
const executeSpy = vi.spyOn(executor as any, "execute").mockResolvedValue(undefined);
|
||||
|
||||
await invokeGraphFailure(executor, task);
|
||||
// The auto-continue branch was entered and a retry scheduled...
|
||||
expect(logText(store)).toContain("auto-continuing the agent session");
|
||||
|
||||
// ...but the task changed state before the scheduled retry fires; the
|
||||
// fire-time re-fetch returns the mutated snapshot.
|
||||
store.getTask.mockResolvedValue({ ...task, ...patch } as typeof task);
|
||||
await flushScheduledRetry();
|
||||
|
||||
expect(executeSpy).not.toHaveBeenCalled();
|
||||
},
|
||||
);
|
||||
|
||||
it("clears a stale failed status when auto-continuing an engine-internal abort (no lingering failure notification)", async () => {
|
||||
// A pause-abort parked status:"failed" on an earlier non-todo observation
|
||||
// stays dispatchable (scheduler filters column+paused, not status) and
|
||||
// re-enters this branch in todo. Auto-continue must reconcile the row to
|
||||
// status:null/error:null — otherwise the persisted failure survives, the
|
||||
// board shows it failed, and the deferred failure notification fires.
|
||||
const { store, task, executor } = makeHarness({
|
||||
column: "todo",
|
||||
status: "failed",
|
||||
error: "Workflow graph failure surfaced after paused engine abort during pause/resume",
|
||||
});
|
||||
(executor as any).activeWorktrees.set(task.id, task.worktree);
|
||||
const executeSpy = vi.spyOn(executor as any, "execute").mockResolvedValue(undefined);
|
||||
|
||||
await invokeGraphFailure(executor, task);
|
||||
|
||||
@@ -121,12 +180,87 @@ describe("pause-abort benign requeue-to-todo (FN-6782)", () => {
|
||||
(call: unknown[]) => (call[1] as { status?: string } | undefined)?.status === "failed",
|
||||
);
|
||||
expect(reParkedFailed).toBe(false);
|
||||
expect(logText(store)).toContain("auto-continuing the agent session");
|
||||
expect(logText(store)).toContain("Auto-recovered: engine-internal pause/resume abort");
|
||||
await flushScheduledRetry();
|
||||
expect(executeSpy).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ label: "explicit user pause", overrides: { column: "todo", userPaused: true }, provenance: "hard-cancel" as const },
|
||||
{ label: "global pause", overrides: { column: "todo" }, provenance: "global-pause" as const },
|
||||
])(
|
||||
"does NOT auto-resume a $label that landed in todo",
|
||||
async ({ overrides, provenance }) => {
|
||||
// The auto-continue is scoped strictly to the engine-internal abort
|
||||
// provenance. A genuine operator pause (userPaused) or a global engine
|
||||
// pause that ended up in todo must stay parked-benign and wait for
|
||||
// explicit resume — auto-resuming it would override the operator's intent.
|
||||
const { store, task, executor } = makeHarness(overrides, provenance);
|
||||
(executor as any).activeWorktrees.set(task.id, task.worktree);
|
||||
const executeSpy = vi.spyOn(executor as any, "execute").mockResolvedValue(undefined);
|
||||
|
||||
await invokeGraphFailure(executor, task);
|
||||
|
||||
expect(logText(store)).toContain("benign, cleared for normal scheduling");
|
||||
expect(logText(store)).not.toContain("auto-continuing the agent session");
|
||||
await flushScheduledRetry();
|
||||
expect(executeSpy).not.toHaveBeenCalled();
|
||||
},
|
||||
);
|
||||
|
||||
it("falls back to a benign todo re-queue once internal retries are exhausted", async () => {
|
||||
// After MAX_TRANSIENT_GRAPH_RESUME_RETRIES (2) internal retries, a still-
|
||||
// wedged engine-internal abort must stop auto-continuing and fall through to
|
||||
// the benign re-queue (no failure notification, no retry storm).
|
||||
const { store, task, executor } = makeHarness({
|
||||
column: "todo",
|
||||
graphResumeRetryCount: 2,
|
||||
});
|
||||
(executor as any).activeWorktrees.set(task.id, task.worktree);
|
||||
const executeSpy = vi
|
||||
.spyOn(executor as any, "execute")
|
||||
.mockResolvedValue(undefined);
|
||||
|
||||
await invokeGraphFailure(executor, task);
|
||||
|
||||
expect(logText(store)).toContain("benign, cleared for normal scheduling");
|
||||
// FNXC:WorkflowLifecycle 2026-06-20-19:58 the clear path must emit an
|
||||
// `Auto-recovered:`-prefixed log so NotificationService proactively cancels
|
||||
// the pending failure timer (recoveredStatus path), not just suppress it at
|
||||
// fire time. Prefix is the documented self-healing recovery contract.
|
||||
expect(logText(store)).toContain("Auto-recovered: cleared stale pause-abort failure on todo re-queue");
|
||||
expect(logText(store)).not.toContain("auto-continuing the agent session");
|
||||
const parkedFailed = store.updateTask.mock.calls.some(
|
||||
(call: unknown[]) => (call[1] as { status?: string } | undefined)?.status === "failed",
|
||||
);
|
||||
expect(parkedFailed).toBe(false);
|
||||
await flushScheduledRetry();
|
||||
expect(executeSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("clears a stale failed status on the retries-exhausted benign fallback", async () => {
|
||||
// The retries-exhausted fallback shares the benign re-queue's stale-failure
|
||||
// reconciliation: a row carrying status:"failed" from an earlier non-todo
|
||||
// observation must be cleared and emit the `Auto-recovered:` log so the
|
||||
// deferred failure notification is suppressed even when auto-continue is
|
||||
// exhausted.
|
||||
const { store, task, executor } = makeHarness({
|
||||
column: "todo",
|
||||
graphResumeRetryCount: 2,
|
||||
status: "failed",
|
||||
error: "Workflow graph failure surfaced after paused engine abort during pause/resume",
|
||||
});
|
||||
(executor as any).activeWorktrees.set(task.id, task.worktree);
|
||||
|
||||
await invokeGraphFailure(executor, task);
|
||||
|
||||
expect(logText(store)).toContain("benign, cleared for normal scheduling");
|
||||
expect(logText(store)).toContain(
|
||||
"Auto-recovered: cleared stale pause-abort failure on todo re-queue",
|
||||
);
|
||||
const clearedFailure = store.updateTask.mock.calls.some(
|
||||
(call: unknown[]) => {
|
||||
const patch = call[1] as { status?: unknown; error?: unknown } | undefined;
|
||||
return patch?.status === null && patch?.error === null;
|
||||
},
|
||||
);
|
||||
expect(clearedFailure).toBe(true);
|
||||
});
|
||||
|
||||
it("STILL parks a non-todo (in-review) pause-abort as operator-action failed", async () => {
|
||||
|
||||
@@ -6804,6 +6804,13 @@ export class TaskExecutor {
|
||||
: pausedAborted
|
||||
? "engine abort during pause/resume"
|
||||
: "task pause";
|
||||
// Typed discriminant for the engine-internal abort case (mirrors the
|
||||
// `pauseProvenance === "engine abort during pause/resume"` arm above):
|
||||
// a hard-cancel teardown that is NOT a user pause or global pause. Used
|
||||
// to gate the auto-continue branch so the gate cannot silently drift if
|
||||
// the human-readable provenance label is ever revised.
|
||||
const isEngineInternalAbort =
|
||||
pausedAborted && !live.paused && !live.userPaused && abortProvenance !== "global-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
|
||||
@@ -6827,6 +6834,94 @@ export class TaskExecutor {
|
||||
// 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);
|
||||
// FNXC:WorkflowLifecycle 2026-06-20-22:42: FN-6782 follow-up — an
|
||||
// "engine abort during pause/resume" is NOT an operator action: the
|
||||
// engine tore down in-flight work (hard-cancel via
|
||||
// abortInFlightTaskWork) while the workflow graph run was ending and
|
||||
// the task got re-queued to todo. Bouncing it back through todo for
|
||||
// a fresh scheduler dispatch is observable churn and used to fire a
|
||||
// spurious failure notification. Instead, continue the agent session
|
||||
// automatically by re-executing in place, bounded by the same
|
||||
// graphResumeRetryCount budget + backoff as the transient-resume
|
||||
// path (and reset to 0 on the next clean graph completion, executor
|
||||
// ~4242) so a genuinely wedged task still falls through to the benign
|
||||
// re-queue after MAX retries rather than looping with no backoff.
|
||||
// Scoped strictly to the engine-internal abort provenance: an
|
||||
// explicit user pause / global pause / task pause that landed in todo
|
||||
// must still wait for an explicit resume (the benign re-queue below).
|
||||
// The graphResumeRetryCount budget is deliberately SHARED with the
|
||||
// transient-resume-after-restart path (executor ~6850): both are
|
||||
// "the graph run ended transiently, re-run it" recoveries, and a
|
||||
// single combined cap is the belt-and-suspenders guard the
|
||||
// executor-retry-storm tests assert against. The count is reset to 0
|
||||
// only on a clean graph completion (~4242) — NOT on the benign
|
||||
// fallback below, so a still-wedged task that exhausts the budget
|
||||
// stops auto-continuing instead of looping (resetting here would
|
||||
// reintroduce a slower storm).
|
||||
if (isEngineInternalAbort) {
|
||||
const priorRetries = live.graphResumeRetryCount ?? 0;
|
||||
if (priorRetries < MAX_TRANSIENT_GRAPH_RESUME_RETRIES) {
|
||||
const nextRetries = priorRetries + 1;
|
||||
const retryMessage = `Workflow graph run ended during ${pauseProvenance} — auto-continuing the agent session (${nextRetries}/${MAX_TRANSIENT_GRAPH_RESUME_RETRIES}) instead of re-queueing to todo`;
|
||||
executorLog.log(`${task.id}: ${retryMessage}`);
|
||||
await this.store.logEntry(task.id, retryMessage, undefined, this.getRunContextFor(task.id));
|
||||
// Emit the Auto-recovered marker BEFORE clearing status so the
|
||||
// status-clearing updateTask's task:updated event already carries
|
||||
// the recovery log — NotificationService.maybeSuppressTransientFailedNotification
|
||||
// (recoveredStatus path) then proactively cancels any pending
|
||||
// failure timer rather than relying on the race-contingent
|
||||
// fire-time re-check.
|
||||
await this.store.logEntry(task.id, "Auto-recovered: engine-internal pause/resume abort — retrying agent session, failure notification suppressed", undefined, this.getRunContextFor(task.id));
|
||||
await this.store.updateTask(task.id, { graphResumeRetryCount: nextRetries, status: null, error: null }, this.getRunContextFor(task.id));
|
||||
await this.persistTokenUsage(task.id);
|
||||
const scheduleRetry = () => {
|
||||
// Re-fetch at fire time: the snapshot is up to
|
||||
// TRANSIENT_GRAPH_RESUME_RETRY_BACKOFF_MS stale, and the direct
|
||||
// execute() bypasses the scheduler's pause filter (we cleared
|
||||
// pausedAborted at the top of this branch). If a user paused,
|
||||
// moved, or deleted the task during the backoff window, abort
|
||||
// the auto-continue and leave it to normal scheduling so we
|
||||
// never resume work the user just parked.
|
||||
void (async () => {
|
||||
try {
|
||||
const resumeTask = await this.store.getTask(task.id);
|
||||
if (
|
||||
resumeTask.deletedAt
|
||||
|| resumeTask.paused
|
||||
|| resumeTask.userPaused
|
||||
|| resumeTask.column !== "todo"
|
||||
) {
|
||||
executorLog.log(
|
||||
`${task.id}: skipping pause-abort auto-continue — task is now ${resumeTask.deletedAt ? "deleted" : resumeTask.paused || resumeTask.userPaused ? "paused" : `in '${resumeTask.column}'`} at retry fire time`,
|
||||
);
|
||||
return;
|
||||
}
|
||||
await this.execute(resumeTask);
|
||||
} catch (err) {
|
||||
executorLog.error(`Failed pause-abort internal retry for ${task.id}:`, err);
|
||||
}
|
||||
})();
|
||||
};
|
||||
if (TRANSIENT_GRAPH_RESUME_RETRY_BACKOFF_MS > 0) {
|
||||
const handle = setTimeout(scheduleRetry, TRANSIENT_GRAPH_RESUME_RETRY_BACKOFF_MS);
|
||||
handle.unref?.();
|
||||
} else {
|
||||
setTimeout(scheduleRetry, 0).unref?.();
|
||||
}
|
||||
return;
|
||||
}
|
||||
// Note: the count is left at MAX (not reset here) deliberately, so
|
||||
// this task stops auto-continuing until a clean graph completion
|
||||
// resets it (~4242). Because the budget is SHARED with the
|
||||
// transient-resume-after-restart path (~6869), a task that already
|
||||
// burned retries there starts here with a smaller auto-continue
|
||||
// budget — and vice versa. That cross-draining is intentional: a
|
||||
// single combined cap across both transient-recovery paths is what
|
||||
// bounds runaway re-runs, even if it means a repeatedly
|
||||
// hard-cancelled task that never completes cleanly exhausts the
|
||||
// shared budget and falls back to plain todo re-queueing.
|
||||
executorLog.warn(`${task.id}: engine abort during pause/resume exhausted ${MAX_TRANSIENT_GRAPH_RESUME_RETRIES} internal retries — falling back to benign todo re-queue`);
|
||||
}
|
||||
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));
|
||||
|
||||
Reference in New Issue
Block a user