fix(FN-7965): honor terminal fn_task_done park instead of resurrecting the session
The in-session `fn_task_done` handler parks a task terminally (status=failed, worktree/branch/sessionFile cleared) once its refusal/invariant retry budget is exhausted. That write happens inside the live agent session, so the executor's no-fn_task_done retry loop never observed it and spawned a fresh session anyway. The retry completed, marked the task done, and dragged a worktree-less row into the pre-merge graph, where the first write-capable node failed on `no-worktree-for-write-node` — surfacing as a misleading "Workflow graph terminated with failure at node 'code-review-remediation'" instead of the real refusal. Observed on FN-7965 and again live on FN-7981. Re-read state at the top of the retry loop and honor the park. The status probe covers all three park sites (invariant-check, explicit refusal, implicit refusal) rather than the single reported repro. Deliberately not routed through the FN-4806 reclaim branch: its silent todo requeue would clear the park and, with the budget already spent, re-park on the next pickup in a todo->execute->park loop. The pre-existing reclaim probes could not catch this — they test `worktree === null`, but the store maps a cleared column to `undefined` (`task-store/serialization.ts`: `row.worktree || undefined`), so the existing test only passed because its mock returned a value production never emits. Tightening that probe regressed 7 fixtures and is left as separate work. Verified: new tests fail with the guard disabled; engine reliability surfaces show zero regressions vs baseline (17 pre-existing failures unchanged, 495->499 passing); engine-core gate suite 294/294. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
7
.changeset/fn-7965-honor-task-done-park.md
Normal file
7
.changeset/fn-7965-honor-task-done-park.md
Normal file
@@ -0,0 +1,7 @@
|
|||||||
|
---
|
||||||
|
"@runfusion/fusion": patch
|
||||||
|
---
|
||||||
|
|
||||||
|
summary: Tasks parked by a refused fn_task_done no longer resurrect and strand at code review.
|
||||||
|
category: fix
|
||||||
|
dev: FN-7965. `fn_task_done`'s in-session refusal handler parks the row terminally (status=failed, worktree/branch/sessionFile cleared) once `MAX_TASK_DONE_REQUEUE_RETRIES` is exhausted, but the executor's no-fn_task_done retry loop never observed that park and spawned a fresh session. That session completed and marked the task done against a worktree-less row, so the pre-merge graph failed on the first write-capable node (`no-worktree-for-write-node`) and surfaced as a misleading "Workflow graph terminated with failure at node 'code-review-remediation'". The loop now re-reads state and honors the park (`terminallyParked`), stopping without requeue or review handoff — deliberately not routed through the FN-4806 reclaim branch, whose silent todo requeue would clear the park and re-park on next pickup in a todo→execute→park loop. The pre-existing reclaim probes could not catch this: they test `worktree === null`, but the store maps a cleared column to `undefined` (`task-store/serialization.ts` — `row.worktree || undefined`); tightening that probe is left as separate work.
|
||||||
@@ -164,4 +164,72 @@ describe("reliability interactions: executor no-fn_task_done vs worktree reclaim
|
|||||||
expect.objectContaining({ status: "failed" }),
|
expect.objectContaining({ status: "failed" }),
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
/*
|
||||||
|
FNXC:ExecutorTaskDonePark 2026-07-15-16:10:
|
||||||
|
FN-7965 symptom verification. The in-session `fn_task_done` refusal handler parks the row terminally
|
||||||
|
(status=failed + cleared worktree/branch/sessionFile) once the refusal budget is exhausted. The retry
|
||||||
|
loop never observed that park and spawned a fresh session, which completed and marked the task done
|
||||||
|
against a worktree-less row — stranding the pre-merge graph on `no-worktree-for-write-node`.
|
||||||
|
Surface enumeration: the park must be honored whichever way the cleared binding surfaces (the real
|
||||||
|
store maps a cleared column to `undefined` via `row.worktree || undefined`, never `null`), and the
|
||||||
|
park must NOT be laundered into the FN-4806 silent requeue, which would clear the failure and
|
||||||
|
re-park on the next pickup in a todo→execute→park loop.
|
||||||
|
*/
|
||||||
|
it.each([
|
||||||
|
["binding cleared to undefined (real store contract)", { worktree: undefined, branch: undefined }],
|
||||||
|
["binding cleared to null (mirror/legacy shape)", { worktree: null, branch: null }],
|
||||||
|
["binding still present", {}],
|
||||||
|
])("honors a terminal fn_task_done park instead of retrying — %s", async (_label, cleared) => {
|
||||||
|
const store = createMockStore();
|
||||||
|
const initial = makeTask({ taskDoneRetryCount: 3 });
|
||||||
|
let getTaskCalls = 0;
|
||||||
|
// Mutate only what the STORE returns, never the object passed to execute(): in production the
|
||||||
|
// in-session tool handler writes the park while this loop still holds its original `task`.
|
||||||
|
store.getTask.mockImplementation(async () => {
|
||||||
|
getTaskCalls++;
|
||||||
|
return getTaskCalls >= 2
|
||||||
|
? { ...initial, status: "failed", error: "fn_task_done refused (bulk-step-completion-without-review)", ...cleared }
|
||||||
|
: { ...initial };
|
||||||
|
});
|
||||||
|
|
||||||
|
mockedCreateFnAgent.mockResolvedValue({ session: makeSession() } as any);
|
||||||
|
|
||||||
|
const executor = new TaskExecutor(store as any, "/tmp/test");
|
||||||
|
await executor.execute(makeTask({ taskDoneRetryCount: 3 }));
|
||||||
|
|
||||||
|
// No resurrection: the park must abort before a second session spawns.
|
||||||
|
expect(mockedCreateFnAgent).toHaveBeenCalledTimes(1);
|
||||||
|
expect(store.logEntry).toHaveBeenCalledWith(
|
||||||
|
"FN-4601",
|
||||||
|
expect.stringContaining("honoring park, not retrying"),
|
||||||
|
undefined,
|
||||||
|
expect.any(Object),
|
||||||
|
);
|
||||||
|
// The park must survive: never laundered into the FN-4806 silent requeue...
|
||||||
|
expect(store.moveTask).not.toHaveBeenCalledWith("FN-4601", "todo", { preserveProgress: true });
|
||||||
|
// ...and never handed off to review, which is what dragged the worktree-less row into the graph.
|
||||||
|
expect(store.moveTask).not.toHaveBeenCalledWith("FN-4601", "in-review");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("still retries when the task has not been parked", async () => {
|
||||||
|
// Negative control: the park check must not abort a healthy retry, or every no-fn_task_done
|
||||||
|
// session would degrade into a premature stop.
|
||||||
|
const store = createMockStore();
|
||||||
|
const state = makeTask();
|
||||||
|
store.getTask.mockImplementation(async () => ({ ...state }));
|
||||||
|
|
||||||
|
mockedCreateFnAgent.mockResolvedValue({ session: makeSession() } as any);
|
||||||
|
|
||||||
|
const executor = new TaskExecutor(store as any, "/tmp/test");
|
||||||
|
await executor.execute(state);
|
||||||
|
|
||||||
|
expect(mockedCreateFnAgent.mock.calls.length).toBeGreaterThan(1);
|
||||||
|
expect(store.logEntry).not.toHaveBeenCalledWith(
|
||||||
|
"FN-4601",
|
||||||
|
expect.stringContaining("honoring park, not retrying"),
|
||||||
|
undefined,
|
||||||
|
expect.any(Object),
|
||||||
|
);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -11518,8 +11518,26 @@ export class TaskExecutor {
|
|||||||
let retryAbortedDueToReclaim = false;
|
let retryAbortedDueToReclaim = false;
|
||||||
let refusalHandled = false;
|
let refusalHandled = false;
|
||||||
let pendingReviewParked = false;
|
let pendingReviewParked = false;
|
||||||
|
/* FNXC:ExecutorTaskDonePark 2026-07-15-16:10: FN-7965 — set when the row was terminally parked (status=failed) by the in-session fn_task_done refusal handler; suppresses both the retry and every post-loop completion/requeue branch so the park survives. */
|
||||||
|
let terminallyParked = false;
|
||||||
while (!taskDone && taskDoneSessionRetries < MAX_TASK_DONE_SESSION_RETRIES) {
|
while (!taskDone && taskDoneSessionRetries < MAX_TASK_DONE_SESSION_RETRIES) {
|
||||||
const liveTask = await this.store.getTask(task.id);
|
const liveTask = await this.store.getTask(task.id);
|
||||||
|
/*
|
||||||
|
FNXC:ExecutorTaskDonePark 2026-07-15-16:10:
|
||||||
|
FN-7965: the explicit `fn_task_done` tool handler parks the task terminally (status=failed, worktree/branch/sessionFile cleared) once the refusal retry budget is exhausted — but it runs INSIDE the agent session, so this loop never learned the row had been parked and spawned a retry session anyway. That session completed and marked the task done against a row with no worktree, so the pre-merge graph died on the first write-capable node with `no-worktree-for-write-node` and surfaced as a bogus "terminated at code-review-remediation" instead of the real refusal. Re-read state and honor the park.
|
||||||
|
This deliberately does NOT reuse the FN-4806 reclaim branch below: that silently requeues to `todo`, which would clear the park and — with the refusal budget already exhausted — re-park on the next pickup, looping todo→execute→park. A terminal park is the agent's own failure and must stay parked for a human.
|
||||||
|
Note the reclaim probes below cannot cover this: they test `liveTask.worktree === null`, but the store maps a cleared column to `undefined`, never `null` (`task-store/serialization.ts` — `row.worktree || undefined`). Tightening that probe is a separate change with real blast radius, so the park is detected by status here instead.
|
||||||
|
*/
|
||||||
|
if (liveTask.status === "failed") {
|
||||||
|
const parkMessage = `${task.id}: task parked failed during no-fn_task_done retry — honoring park, not retrying`;
|
||||||
|
executorLog.log(parkMessage);
|
||||||
|
await this.store.logEntry(task.id, parkMessage, undefined, this.getRunContextFor(task.id));
|
||||||
|
this.deleteActiveSession(task.id);
|
||||||
|
this.tokenUsageBaselines.delete(task.id);
|
||||||
|
session.dispose();
|
||||||
|
terminallyParked = true;
|
||||||
|
break;
|
||||||
|
}
|
||||||
const hasExplicitWorktreeBinding = typeof liveTask.worktree === "string" || liveTask.worktree === null;
|
const hasExplicitWorktreeBinding = typeof liveTask.worktree === "string" || liveTask.worktree === null;
|
||||||
const hasExplicitBranchBinding = typeof liveTask.branch === "string" || liveTask.branch === null;
|
const hasExplicitBranchBinding = typeof liveTask.branch === "string" || liveTask.branch === null;
|
||||||
const worktreeContractIntact = liveTask.column === "in-progress"
|
const worktreeContractIntact = liveTask.column === "in-progress"
|
||||||
@@ -11798,6 +11816,12 @@ export class TaskExecutor {
|
|||||||
this.clearCompletedTaskWatchdog(task.id);
|
this.clearCompletedTaskWatchdog(task.id);
|
||||||
executorLog.log(`✓ ${task.id} completed on retry → in-review`);
|
executorLog.log(`✓ ${task.id} completed on retry → in-review`);
|
||||||
this.signalTaskComplete(task);
|
this.signalTaskComplete(task);
|
||||||
|
} else if (terminallyParked) {
|
||||||
|
// FN-7965: the in-session refusal handler already wrote the terminal failure and cleared
|
||||||
|
// the binding. Nothing further to do — requeueing or handing off to review here is exactly
|
||||||
|
// the resurrection that stranded the pre-merge graph.
|
||||||
|
await this.persistTokenUsage(task.id);
|
||||||
|
return;
|
||||||
} else if (retryAbortedDueToReclaim) {
|
} else if (retryAbortedDueToReclaim) {
|
||||||
// FN-4806: Worktree/branch was reclaimed mid-retry by an engine-side housekeeping path
|
// FN-4806: Worktree/branch was reclaimed mid-retry by an engine-side housekeeping path
|
||||||
// (e.g. FN-4546 stale-active-branch reclaim, FN-4742 self-healing removals). This is NOT
|
// (e.g. FN-4546 stale-active-branch reclaim, FN-4742 self-healing removals). This is NOT
|
||||||
|
|||||||
Reference in New Issue
Block a user