diff --git a/.changeset/planning-handoff-atomic.md b/.changeset/planning-handoff-atomic.md new file mode 100644 index 0000000000..e912667b27 --- /dev/null +++ b/.changeset/planning-handoff-atomic.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix planned tasks stalling ~10 minutes in Todo before Plan Review starts. +category: fix +dev: The planning→plan-review handoff now retires its predecessor work item and installs the successor continuation in one transaction (`seedStrandedPlanReviewContinuation` gained `retirePredecessorId`); the specification-complete reaction consumes the seed result with bounded retries instead of dropping failures silently. diff --git a/docs/solutions/architecture-patterns/workflow-node-column-placement-and-graph-entry-contract.md b/docs/solutions/architecture-patterns/workflow-node-column-placement-and-graph-entry-contract.md index 27224737a6..76513777ba 100644 --- a/docs/solutions/architecture-patterns/workflow-node-column-placement-and-graph-entry-contract.md +++ b/docs/solutions/architecture-patterns/workflow-node-column-placement-and-graph-entry-contract.md @@ -91,7 +91,11 @@ own column, so the node must be where the card rests. `passed` `plan-review` entry to `workflowStepResults`. A held unreviewed card is the gate working; that path belongs to `pre-release-plan-review.test.ts`. - **Pre-existing cards** sitting in Todo with a real spec and no continuation are re-seeded - automatically by FN-8592's stranded-hold-continuation sweep. + automatically by FN-8592's stranded-hold-continuation sweep. Since 2026-08-13 that sweep is a pure + backstop: the primary handoff retires its own planning work item atomically with the successor + install (`retirePredecessorId`), so routine post-planning stranding — which once made the sweep + the de facto handoff at a ~10-minute delay per card — is a bug, not expected behavior. See + [planning-handoff-race-silently-strands-plan-review](../logic-errors/planning-handoff-race-silently-strands-plan-review.md). - **The trace no longer starts at `start`** for a card past intake. Assertions on `visitedNodeIds` should expect the card's column entry point. - **Coding (Ideas) no longer has a private planning shape.** Its planning-node re-home is deleted — diff --git a/docs/solutions/logic-errors/planning-handoff-race-silently-strands-plan-review.md b/docs/solutions/logic-errors/planning-handoff-race-silently-strands-plan-review.md new file mode 100644 index 0000000000..0b44ada533 --- /dev/null +++ b/docs/solutions/logic-errors/planning-handoff-race-silently-strands-plan-review.md @@ -0,0 +1,69 @@ +--- +category: logic-errors +module: "@fusion/engine (triage planning handoff), @fusion/core (workflow work items)" +tags: [workflow-work-items, continuation, race-condition, silent-drop, self-healing-backstop, plan-review] +problem_type: race_condition +applies_when: "A node-completion handoff installs its successor continuation outside the transaction that terminalizes the predecessor, or a fire-and-forget reaction discards a seed/install result." +--- + +# Planning handoff race silently strands Plan Review + +## Symptom + +Every freshly planned (or replanned) card sat idle in Todo for ~10 minutes with +"Execution dispatch refused — task is still unplanned" in its log, then moved on +after `task:reconcile-stranded-hold-continuation` fired. 529 self-healing +recoveries in 18 days — self-healing had silently become the PRIMARY handoff, +each one costing the card the sweep's full staleness grace. + +## Root cause + +Two independent defects compounded: + +1. **Ordering race.** Triage's `specifyTask` announced completion + (`onSpecifyComplete`) BEFORE its `finally` block transitioned the planning + work item to `succeeded`. The seed reaction fired concurrently, and + `seedPreReleasePlanReviewContinuation`'s all-kinds idle check counted the + caller's own still-`running` plan row as an "active continuation" and bailed. +2. **Silent drop.** `reactToSpecificationComplete` awaited the seed but + discarded its result, so the bail was invisible — no log, no retry, no audit. + The card stranded until FN-8592 self-healing re-seeded it. + +## Fix (2026-08-13) + +- `seedStrandedPlanReviewContinuation` gained `retirePredecessorId`: in ONE + transaction under the task advisory lock it excludes the named predecessor + from the idle check, re-verifies no OTHER active row and no passed Plan + Review, transitions the predecessor to `succeeded`, and installs the + successor. Checks run BEFORE the retirement, so a bailed seed mutates nothing. +- Triage threads `planningWorkItemId` through `PlanningHandoffReport`; the + runtime reaction passes it as `retirePredecessorId`. +- The reaction consumes the seed result: quiet parks (`awaiting-approval`, + `paused`, `plan-review-passed`, `no-pre-release-plan-review`) log and exit; + anomalies retry (1s, 5s) with a fresh task/IR snapshot per attempt (so a + mid-retry pause or `needs-replan` is honored) and warn loudly on exhaustion, + naming self-healing as the recovery owner. + +## Lessons + +- **A node transition is a handoff, not two writes.** Terminalizing the + predecessor and installing the successor must be one transaction; any code + that does them separately has an ordering race with every observer between. +- **A discarded result is a silent stall.** Fire-and-forget reactions must + consume the outcome and surface non-success loudly (see + [branch-group-name-collision-strands-mission-triage](branch-group-name-collision-strands-mission-triage.md)). +- **Self-healing frequency is a bug signal.** A "backstop" firing hundreds of + times is the primary path failing quietly; alert on recovery-event volume. +- **Reason-less bails poison retry loops.** Every refusal needs a named reason + so consumers can distinguish legitimate configuration (no pre-release Plan + Review node) from genuine anomalies worth retrying/warning. + +## Verification + +- `packages/core/src/__tests__/workflow-work-items-conditional-seed.test.ts` — + "planning handoff retires its own predecessor atomically": both race + orderings, only-the-named-predecessor, cross-task safety, no-mutation-on-bail. +- `packages/engine/src/__tests__/plan-approval-hold-invariant.test.ts` — "#2b + the handoff seeder does not bail on its own named predecessor". +- `packages/engine/src/__tests__/specify-complete-reaction.test.ts` — result + consumption, bounded retries, quiet parks, mid-retry pause/replan honoring. diff --git a/packages/core/src/__tests__/workflow-work-items-conditional-seed.test.ts b/packages/core/src/__tests__/workflow-work-items-conditional-seed.test.ts index f326ae3e6f..6a1e82fa1e 100644 --- a/packages/core/src/__tests__/workflow-work-items-conditional-seed.test.ts +++ b/packages/core/src/__tests__/workflow-work-items-conditional-seed.test.ts @@ -176,6 +176,102 @@ pgTest("FN-8592 conditional stranded Plan Review seed", () => { expect((await store.getTask(task.id)).workflowStepResults?.[0]?.status).toBe("passed"); }); + /* + FNXC:PlanningHandoffAtomicity 2026-08-13-03:49: + THE INVARIANT: the normal planning handoff installs the successor continuation no + matter which side of the race commits first — the runtime's specification-complete + reaction or triage's finally-block terminal transition of the plan work item. + Before `retirePredecessorId`, the reaction side lost that race constantly (the + seeder saw its own still-running predecessor as an active continuation and bailed + silently), stranding cards for FN-8592 self-healing to repair ~10 minutes later. + */ + describe("planning handoff retires its own predecessor atomically", () => { + function planPredecessor(taskId: string) { + return { + runId: `${taskId}:continuation:plan`, + taskId, + nodeId: "plan", + kind: "task" as const, + state: "running" as const, + stableWorkflowRunId: `${taskId}:workflow`, + continuationSequence: 0, + waitReason: "planning" as const, + sourceColumn: "todo", + targetColumn: "todo", + irHash: "ir-test", + }; + } + + it("retires a still-running predecessor and installs the successor in one call (reaction wins the race)", async () => { + const store = h.store(); + const task = await store.createTask({ description: "handoff race owner", column: "todo" }); + const pred = await store.upsertWorkflowWorkItem(planPredecessor(task.id)); + + await expect(store.seedStrandedPlanReviewContinuation(continuation(task.id, "handoff"), { + retirePredecessorId: pred.id, + })).resolves.toMatchObject({ seeded: true }); + + const rows = await store.listWorkflowWorkItemsForTask(task.id); + expect(rows).toEqual(expect.arrayContaining([ + expect.objectContaining({ id: pred.id, state: "succeeded" }), + expect.objectContaining({ runId: continuation(task.id, "handoff").runId, state: "runnable" }), + ])); + }); + + it("still seeds when the predecessor is already terminal (finally block won the race)", async () => { + const store = h.store(); + const task = await store.createTask({ description: "handoff terminal owner", column: "todo" }); + const pred = await store.upsertWorkflowWorkItem(planPredecessor(task.id)); + await store.transitionWorkflowWorkItem(pred.id, "succeeded", { leaseOwner: null, leaseExpiresAt: null }); + + await expect(store.seedStrandedPlanReviewContinuation(continuation(task.id, "handoff"), { + retirePredecessorId: pred.id, + })).resolves.toMatchObject({ seeded: true }); + }); + + it("still seeds when the named predecessor row no longer exists", async () => { + const store = h.store(); + const task = await store.createTask({ description: "handoff missing owner", column: "todo" }); + + await expect(store.seedStrandedPlanReviewContinuation(continuation(task.id, "handoff"), { + retirePredecessorId: "00000000-0000-0000-0000-000000000000", + })).resolves.toMatchObject({ seeded: true }); + }); + + it("retires ONLY the named predecessor: any other active row still blocks the seed", async () => { + const store = h.store(); + const task = await store.createTask({ description: "handoff blocked owner", column: "todo" }); + const pred = await store.upsertWorkflowWorkItem(planPredecessor(task.id)); + const other = await store.upsertWorkflowWorkItem(continuation(task.id, "other", "workflow-step")); + + await expect(store.seedStrandedPlanReviewContinuation(continuation(task.id, "handoff"), { + retirePredecessorId: pred.id, + })).resolves.toEqual({ seeded: false, reason: "active-continuation" }); + + const rows = await store.listWorkflowWorkItemsForTask(task.id); + expect(rows.find((row) => row.id === other.id)?.state).toBe("runnable"); + expect(rows.some((row) => row.runId === continuation(task.id, "handoff").runId)).toBe(false); + // FNXC:PlanningHandoffAtomicity 2026-08-13-04:20 (review finding): + // A bailed seed must mutate NOTHING — the named predecessor stays running, + // it is not laundered to succeeded on the way to a refusal. + expect(rows.find((row) => row.id === pred.id)?.state).toBe("running"); + }); + + it("never retires a predecessor belonging to a different task", async () => { + const store = h.store(); + const taskA = await store.createTask({ description: "handoff owner a", column: "todo" }); + const taskB = await store.createTask({ description: "handoff owner b", column: "todo" }); + const foreign = await store.upsertWorkflowWorkItem(planPredecessor(taskB.id)); + + await expect(store.seedStrandedPlanReviewContinuation(continuation(taskA.id, "handoff"), { + retirePredecessorId: foreign.id, + })).resolves.toMatchObject({ seeded: true }); + + const foreignRows = await store.listWorkflowWorkItemsForTask(taskB.id); + expect(foreignRows.find((row) => row.id === foreign.id)?.state).toBe("running"); + }); + }); + it("allows terminal continuations and non-passed Plan Review results", async () => { const store = h.store(); const task = await store.createTask({ description: "terminal continuation owner", column: "todo" }); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 6dc0c7d392..96468daaf6 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -2453,8 +2453,8 @@ export class TaskStore extends EventEmitter { async replaceActiveTaskWorkflowContinuation(input: WorkflowWorkItemUpsertInput & { kind: "task" }): Promise { return replaceActiveTaskWorkflowContinuationImpl(this, input); } - async seedStrandedPlanReviewContinuation(input: WorkflowWorkItemUpsertInput & { kind: "task" }): Promise<{ seeded: boolean; reason?: "active-continuation" | "plan-review-passed"; workItemId?: string }> { - return seedStrandedPlanReviewContinuationImpl(this, input); + async seedStrandedPlanReviewContinuation(input: WorkflowWorkItemUpsertInput & { kind: "task" }, options: { retirePredecessorId?: string } = {}): Promise<{ seeded: boolean; reason?: "active-continuation" | "plan-review-passed"; workItemId?: string }> { + return seedStrandedPlanReviewContinuationImpl(this, input, options); } async transitionWorkflowWorkItem( id: string, state: WorkflowWorkItemState, patch: WorkflowWorkItemTransitionPatch = {}, tx?: import("./postgres/data-layer.js").DbTransaction, ): Promise { return transitionWorkflowWorkItemImpl(this, id, state, patch, tx); diff --git a/packages/core/src/task-store/async/async-workflow-workitems.ts b/packages/core/src/task-store/async/async-workflow-workitems.ts index 224f296b57..cfb522362b 100644 --- a/packages/core/src/task-store/async/async-workflow-workitems.ts +++ b/packages/core/src/task-store/async/async-workflow-workitems.ts @@ -354,16 +354,43 @@ export async function replaceActiveTaskWorkflowContinuation( /** * FNXC:StrandedHoldContinuation 2026-07-26-12:00: - * FN-8592 repairs only an idle graph: this insert-only operation checks every - * active work-item kind and passed plan-review result after taking the shared - * advisory lock. A false result is a race loss for an already-qualified caller; - * it never retires a continuation or edits step results. + * FN-8592 repairs only an idle graph: this operation checks every active + * work-item kind and passed plan-review result after taking the shared + * advisory lock. A false result is a race loss for an already-qualified caller + * and mutates nothing. + * + * FNXC:PlanningHandoffAtomicity 2026-08-13-04:20: + * No longer strictly insert-only: on the SUCCESS path a caller-named + * `retirePredecessorId` row is transitioned to succeeded in the same + * transaction as the successor insert (see the block comment below). A bailed + * seed still mutates nothing — the checks run before the retirement. */ export async function seedStrandedPlanReviewContinuation( layer: AsyncDataLayer, input: WorkflowWorkItemUpsertInput & { kind: "task" }, + options: { retirePredecessorId?: string } = {}, ): Promise<{ seeded: boolean; reason?: "active-continuation" | "plan-review-passed"; workItemId?: string }> { return layer.transactionImmediate(async (tx) => withTaskWorkflowSerialization(tx, layer.projectId, input.taskId, async () => { + /* + FNXC:PlanningHandoffAtomicity 2026-08-13-03:49: + The normal planning handoff must retire its own predecessor continuation and install the + successor in ONE transaction under the task lock. Before this, triage announced specification + completion BEFORE its finally block marked the plan work item terminal, so the seeder's + all-kinds idle check saw the caller's own still-running plan row, bailed with + "active-continuation", and the result was silently dropped — the card stranded in the hold + column until FN-8592 self-healing re-seeded it ~10 minutes later (529 occurrences in 18 days). + `retirePredecessorId` names the finished planning row: the idle check excludes it, and if the + seed qualifies the row is transitioned to succeeded in the same transaction as the successor + insert, so BOTH orderings of "announce completion" vs "finally marks terminal" converge on the + successor being installed. A terminal or missing predecessor is not an error — it means the + caller's finally won the race, which is fine. + + FNXC:PlanningHandoffAtomicity 2026-08-13-04:20: + The retirement runs AFTER the bail checks, immediately before the successor insert (review + finding: retiring first meant a bailed seed still committed the predecessor's transition, + contradicting the "a false result mutates nothing" contract). Only the named predecessor is + ever retired; any OTHER active row still blocks the seed and leaves the predecessor untouched. + */ // FNXC:StrandedHoldContinuation 2026-07-27-01:15: // FN-8592's all-kinds idle check is project-partitioned as well as task // partitioned. Task ids can collide across embedded-PG projects, so a @@ -373,13 +400,27 @@ export async function seedStrandedPlanReviewContinuation( eq(schema.project.workflowWorkItems.taskId, input.taskId), inArray(schema.project.workflowWorkItems.state, [...ACTIVE_WORKFLOW_WORK_ITEM_STATES]), )); - if (active.length > 0) return { seeded: false, reason: "active-continuation" as const }; + const blocking = options.retirePredecessorId + ? active.filter((row) => row.id !== options.retirePredecessorId) + : active; + if (blocking.length > 0) return { seeded: false, reason: "active-continuation" as const }; const taskRows = await tx.select({ workflowStepResults: schema.project.tasks.workflowStepResults }).from(schema.project.tasks).where(and( projectScopeFor(schema.project.tasks.projectId, layer.projectId), eq(schema.project.tasks.id, input.taskId), )).limit(1); const results = taskRows[0]?.workflowStepResults as WorkflowStepResult[] | null | undefined; if (results?.some(isPlanReviewSatisfied)) return { seeded: false, reason: "plan-review-passed" as const }; + if (options.retirePredecessorId) { + const predRows = await tx.select().from(schema.project.workflowWorkItems).where(and( + projectScopeFor(schema.project.workflowWorkItems.projectId, layer.projectId), + eq(schema.project.workflowWorkItems.id, options.retirePredecessorId), + eq(schema.project.workflowWorkItems.taskId, input.taskId), + )).limit(1); + const pred = predRows[0] as WorkflowWorkItemRow | undefined; + if (pred && !isTerminalWorkflowWorkItemState(normalizeWorkflowWorkItemState(pred.state))) { + await transitionWorkflowWorkItem(layer, pred.id, "succeeded", { leaseOwner: null, leaseExpiresAt: null, lastError: null }, tx); + } + } const item = await upsertWorkflowWorkItem(layer, input, tx); return { seeded: true, workItemId: item.id }; })); diff --git a/packages/core/src/task-store/workflow-workitems-ops-2.ts b/packages/core/src/task-store/workflow-workitems-ops-2.ts index 4d79d84f6c..60a11261ea 100644 --- a/packages/core/src/task-store/workflow-workitems-ops-2.ts +++ b/packages/core/src/task-store/workflow-workitems-ops-2.ts @@ -26,12 +26,12 @@ export async function replaceActiveTaskWorkflowContinuationImpl( return replaceActiveTaskWorkflowContinuationAsync(store.asyncLayer!, input); } -export async function seedStrandedPlanReviewContinuationImpl(store: TaskStore, input: WorkflowWorkItemUpsertInput & { kind: "task" }): Promise<{ seeded: boolean; reason?: "active-continuation" | "plan-review-passed"; workItemId?: string }> { +export async function seedStrandedPlanReviewContinuationImpl(store: TaskStore, input: WorkflowWorkItemUpsertInput & { kind: "task" }, options: { retirePredecessorId?: string } = {}): Promise<{ seeded: boolean; reason?: "active-continuation" | "plan-review-passed"; workItemId?: string }> { /* FNXC:SqliteDualPathCleanup 2026-07-26-14:07: Stranded plan-review continuation seed is PostgreSQL-only (withTaskWorkflowSerialization). The SQLite transactionImmediate fallback is deleted. */ - return seedStrandedPlanReviewContinuationAsync(store.asyncLayer!, input); + return seedStrandedPlanReviewContinuationAsync(store.asyncLayer!, input, options); } export async function transitionWorkflowWorkItemImpl(store: TaskStore, id: string, state: WorkflowWorkItemState, patch: WorkflowWorkItemTransitionPatch = {}, tx?: DbTransaction,): Promise { diff --git a/packages/engine/src/__tests__/plan-approval-hold-invariant.test.ts b/packages/engine/src/__tests__/plan-approval-hold-invariant.test.ts index d769e5a565..b67c483873 100644 --- a/packages/engine/src/__tests__/plan-approval-hold-invariant.test.ts +++ b/packages/engine/src/__tests__/plan-approval-hold-invariant.test.ts @@ -285,6 +285,71 @@ function seedStore(): { store: TaskStore; seeded: () => number } { return { store, seeded: () => seeds }; } +/* +FNXC:PlanningHandoffAtomicity 2026-08-13-04:20: +THE INVARIANT: the normal planning handoff must not bail on its OWN predecessor. +Triage announces specification completion before its finally block marks the +planning work item terminal, so the seeder routinely observes that row still +`running`. Naming it via `retirePredecessorId` excludes it from the engine-side +active pre-check and routes to the atomic store op that retires it with the +successor install; any OTHER active row still blocks, and omitting the option +keeps the historical bail (which is what self-healing's idle-graph repair needs). +*/ +describe("#2b the handoff seeder does not bail on its own named predecessor", () => { + const runningPlanItem = { id: "wi-plan", state: "running" } as WorkflowWorkItem; + + it("seeds atomically past a still-running named predecessor", async () => { + const { store } = seedStore(); + (store.listWorkflowWorkItemsForTask as ReturnType).mockResolvedValue([runningPlanItem]); + + const result = await seedPreReleasePlanReviewContinuation(store, task(), planInPlaceIr(), { + retirePredecessorId: "wi-plan", + }); + + expect(result.seeded).toBe(true); + expect(store.seedStrandedPlanReviewContinuation).toHaveBeenCalledWith( + expect.objectContaining({ taskId: "FN-1", nodeId: PLAN_REVIEW_GROUP_ID }), + { retirePredecessorId: "wi-plan" }, + ); + expect(store.replaceActiveTaskWorkflowContinuation).not.toHaveBeenCalled(); + }); + + it("still bails on the same running row when no predecessor is named (opt-in)", async () => { + const { store, seeded } = seedStore(); + (store.listWorkflowWorkItemsForTask as ReturnType).mockResolvedValue([runningPlanItem]); + + const result = await seedPreReleasePlanReviewContinuation(store, task(), planInPlaceIr()); + + expect(result).toEqual({ seeded: false, reason: "active-continuation" }); + expect(seeded()).toBe(0); + }); + + it("still bails when a DIFFERENT active row exists alongside the named predecessor", async () => { + const { store, seeded } = seedStore(); + (store.listWorkflowWorkItemsForTask as ReturnType).mockResolvedValue([ + runningPlanItem, + { id: "wi-other", state: "runnable" } as WorkflowWorkItem, + ]); + + const result = await seedPreReleasePlanReviewContinuation(store, task(), planInPlaceIr(), { + retirePredecessorId: "wi-plan", + }); + + expect(result).toEqual({ seeded: false, reason: "active-continuation" }); + expect(seeded()).toBe(0); + }); + + it("reports a workflow without a pre-release Plan Review node as its own quiet reason", async () => { + const { store, seeded } = seedStore(); + const result = await seedPreReleasePlanReviewContinuation(store, task(), releaseIr(), { + retirePredecessorId: "wi-plan", + }); + + expect(result).toEqual({ seeded: false, reason: "no-pre-release-plan-review" }); + expect(seeded()).toBe(0); + }); +}); + describe("#2 neither continuation seeder arms a run for a card blocked on approval", () => { it("seeds for an ordinary specified card (the control)", async () => { const { store, seeded } = seedStore(); diff --git a/packages/engine/src/__tests__/specify-complete-reaction.test.ts b/packages/engine/src/__tests__/specify-complete-reaction.test.ts index 473c785e0e..eab342dbcd 100644 --- a/packages/engine/src/__tests__/specify-complete-reaction.test.ts +++ b/packages/engine/src/__tests__/specify-complete-reaction.test.ts @@ -31,6 +31,7 @@ import { describe, expect, it, vi } from "vitest"; import type { Task, WorkflowIr } from "@fusion/core"; import { reactToSpecificationComplete } from "../runtimes/in-process-runtime.js"; +import type { PlanReviewSeedBailReason } from "../plan-review-continuation.js"; import type { PlanningHandoffOutcome } from "../triage.js"; const IR = { version: "v2", name: "wf", columns: [], nodes: [], edges: [] } as unknown as WorkflowIr; @@ -38,22 +39,39 @@ const IR = { version: "v2", name: "wf", columns: [], nodes: [], edges: [] } as u // `null` means the row vanished. NOT `undefined`: that would trigger the default // parameter and silently hand the reaction a live task, turning the // vanished-task case into a duplicate of the control. -function harness(task: Task | null = { id: "FN-1", column: "todo" } as Task) { +function harness( + task: Task | null = { id: "FN-1", column: "todo" } as Task, + seedImpl?: (t: Task) => Promise<{ seeded: boolean; reason?: PlanReviewSeedBailReason }>, + // getTask is invoked once per attempt (the stale-snapshot fix); an impl override + // lets a test change the task's state between retry attempts. + getTaskImpl?: (call: number) => Task | null, +) { const seeded: string[] = []; const kicks: string[] = []; const logs: string[] = []; + const warns: string[] = []; + const sleeps: number[] = []; + let getTaskCalls = 0; return { seeded, kicks, logs, + warns, + sleeps, run: (outcome: PlanningHandoffOutcome) => reactToSpecificationComplete({ taskId: "FN-1", outcome, - getTask: async () => task ?? undefined, + getTask: async () => { + getTaskCalls += 1; + const resolved = getTaskImpl ? getTaskImpl(getTaskCalls) : task; + return resolved ?? undefined; + }, resolveIr: async () => IR, - seed: async (t) => { seeded.push(t.id); return { seeded: true }; }, + seed: async (t) => { seeded.push(t.id); return seedImpl ? seedImpl(t) : { seeded: true }; }, kick: () => { kicks.push("kick"); }, log: (m) => { logs.push(m); }, + warn: (m) => { warns.push(m); }, + sleep: async (ms) => { sleeps.push(ms); }, }), }; } @@ -125,9 +143,128 @@ describe("specification-complete reaction arms a plan review only on a real hand seed: async () => ({ seeded: true }), kick: () => {}, log: () => {}, + warn: () => {}, }); expect(getTask).not.toHaveBeenCalled(); expect(resolveIr).not.toHaveBeenCalled(); }); }); + +/* +FNXC:PlanningHandoffAtomicity 2026-08-13-03:49: +THE INVARIANT: a released card's Plan Review handoff is never dropped silently. +The seed result used to be discarded, so a bail (the seeder racing triage's own +finally-block terminal transition and seeing its caller as an "active +continuation") left the card stranded until self-healing re-seeded it ~10 minutes +later — 529 times in 18 days. The reaction must consume the result: quiet parks +stay quiet, anomalies retry a bounded number of times, and a final failure is +warned loudly with the recovery owner named. +*/ +describe("specification-complete reaction never drops a failed handoff silently", () => { + for (const reason of ["awaiting-approval", "paused", "plan-review-passed", "no-pre-release-plan-review"] as const) { + it(`treats ${reason} as a quiet park: no retry, no warning`, async () => { + const h = harness(undefined, async () => ({ seeded: false, reason })); + + await h.run("released"); + + expect(h.seeded).toEqual(["FN-1"]); + expect(h.kicks).toEqual([]); + expect(h.warns).toEqual([]); + expect(h.sleeps).toEqual([]); + expect(h.logs.some((m) => m.includes(reason))).toBe(true); + }); + } + + it("retries an active-continuation bail and succeeds when the writer clears", async () => { + let calls = 0; + const h = harness(undefined, async () => { + calls += 1; + return calls < 2 ? { seeded: false, reason: "active-continuation" } : { seeded: true }; + }); + + await h.run("released"); + + expect(calls).toBe(2); + expect(h.kicks).toEqual(["kick"]); + expect(h.sleeps).toEqual([1_000]); + expect(h.warns).toHaveLength(1); + expect(h.warns[0]).toContain("retrying"); + }); + + it("retries a thrown store error the same way as a bail", async () => { + let calls = 0; + const h = harness(undefined, async () => { + calls += 1; + if (calls < 2) throw new Error("transient db error"); + return { seeded: true }; + }); + + await h.run("released"); + + expect(calls).toBe(2); + expect(h.kicks).toEqual(["kick"]); + expect(h.warns[0]).toContain("transient db error"); + }); + + it("exhausts the bounded retries and names the self-healing recovery owner", async () => { + const h = harness(undefined, async () => ({ seeded: false, reason: "active-continuation" })); + + await h.run("released"); + + expect(h.seeded).toEqual(["FN-1", "FN-1", "FN-1"]); + expect(h.sleeps).toEqual([1_000, 5_000]); + expect(h.kicks).toEqual([]); + const final = h.warns[h.warns.length - 1]; + expect(final).toContain("failed after 3 attempts"); + expect(final).toContain("self-healing"); + }); + + /* + FNXC:PlanningHandoffAtomicity 2026-08-13-04:20: + The task snapshot is re-fetched per attempt, so operator state landing during a + retry delay is honored: a pause stops the loop without another seed, and a + needs-replan status (the plan was superseded mid-retry) exits quietly to the + replan loop. A single pre-loop snapshot could not honor either. + */ + it("honors an operator pause that lands between retry attempts", async () => { + const h = harness( + undefined, + async () => ({ seeded: false, reason: "active-continuation" }), + (call) => (call === 1 + ? ({ id: "FN-1", column: "todo" } as Task) + : ({ id: "FN-1", column: "todo", paused: true } as Task)), + ); + + await h.run("released"); + + expect(h.seeded).toEqual(["FN-1"]); + expect(h.kicks).toEqual([]); + expect(h.warns).toHaveLength(1); + }); + + it("exits quietly when a replan supersedes the plan between retry attempts", async () => { + const h = harness( + undefined, + async () => ({ seeded: false, reason: "active-continuation" }), + (call) => (call === 1 + ? ({ id: "FN-1", column: "todo" } as Task) + : ({ id: "FN-1", column: "todo", status: "needs-replan" } as Task)), + ); + + await h.run("released"); + + expect(h.seeded).toEqual(["FN-1"]); + expect(h.kicks).toEqual([]); + expect(h.logs.some((m) => m.includes("needs-replan"))).toBe(true); + }); + + it("never arms Plan Review for a card that is needs-replan at reaction time", async () => { + const h = harness({ id: "FN-1", column: "todo", status: "needs-replan" } as Task); + + await h.run("released"); + + expect(h.seeded).toEqual([]); + expect(h.kicks).toEqual([]); + }); +}); diff --git a/packages/engine/src/plan-review-continuation.ts b/packages/engine/src/plan-review-continuation.ts index 21872038d5..60c789c182 100644 --- a/packages/engine/src/plan-review-continuation.ts +++ b/packages/engine/src/plan-review-continuation.ts @@ -37,18 +37,33 @@ export type ApprovedPlanReviewHandoffResult = { * an active non-task continuation means the graph is not idle, even though the * newly seeded continuation itself remains kind `task` for processor parity. */ +/* +FNXC:PlanningHandoffAtomicity 2026-08-13-04:20: +The bail-reason union is exported so the runtime reaction's quiet-park set stays type-linked to it: +a rename or new reason here must be a compile error at the consumer, not a silent behavior change. +"no-pre-release-plan-review" exists because a workflow with no pre-release Plan Review node (or a +plan-review gate living in a WIP column) is a legitimate configuration — the reaction must treat +that bail as a quiet park, not an anomaly worth retries and warnings. +*/ +export type PlanReviewSeedBailReason = + | "no-pre-release-plan-review" + | "active-continuation" + | "plan-review-passed" + | "awaiting-approval" + | "paused"; + export async function seedPreReleasePlanReviewContinuation( store: TaskStore, task: Task, ir: WorkflowIr, - options: { atomic?: boolean } = {}, + options: { atomic?: boolean; retirePredecessorId?: string } = {}, ): Promise<{ seeded: boolean; - reason?: "active-continuation" | "plan-review-passed" | "awaiting-approval" | "paused"; + reason?: PlanReviewSeedBailReason; workItemId?: string; }> { const node = resolvePreReleasePlanReviewNode(ir); - if (!node || node.column !== task.column) return { seeded: false }; + if (!node || node.column !== task.column) return { seeded: false, reason: "no-pre-release-plan-review" }; /* FNXC:PlanApprovalHold 2026-07-27-19:30 (U7 / R4): Arming a runnable continuation is starting AI work on this card, so the parks @@ -70,7 +85,18 @@ export async function seedPreReleasePlanReviewContinuation( if (isTaskBlockedOnApproval(task)) return { seeded: false, reason: "awaiting-approval" }; if (task.paused === true || task.userPaused === true) return { seeded: false, reason: "paused" }; const items = await store.listWorkflowWorkItemsForTask(task.id); - const active = items.filter((item) => ACTIVE_WORKFLOW_WORK_ITEM_STATES.includes(item.state)); + /* + FNXC:PlanningHandoffAtomicity 2026-08-13-03:49: + The normal planning handoff names its own just-finished plan work item via + `retirePredecessorId`. That row is often still `running` here because triage + announces completion BEFORE its finally block marks the row terminal, so counting + it as "active" made the seeder bail on its own predecessor and strand the card + until FN-8592 self-healing re-seeded it ~10 minutes later. The predecessor is + excluded from this advisory pre-check and retired atomically with the successor + install inside the store operation; any OTHER active row still blocks the seed. + */ + const active = items.filter((item) => + ACTIVE_WORKFLOW_WORK_ITEM_STATES.includes(item.state) && item.id !== options.retirePredecessorId); if (active.length > 0) return { seeded: false, reason: "active-continuation" }; // FNXC:StrandedHoldContinuation 2026-07-26-16:10: // A terminal predecessor is still part of this task's durable run history. @@ -91,6 +117,16 @@ export async function seedPreReleasePlanReviewContinuation( targetColumn: task.column, irHash: computeWorkflowIrPin(ir, node.id).irHash, }; + /* + FNXC:PlanningHandoffAtomicity 2026-08-13-03:49: + A caller that names a predecessor gets the atomic conditional seed regardless of the + `atomic` flag: the pre-check above is advisory (outside any transaction), so the + retire-predecessor + idle-recheck + successor-install must all land in ONE store + transaction under the task lock for the handoff to be ordering-proof. + */ + if (options.retirePredecessorId) { + return store.seedStrandedPlanReviewContinuation(input, { retirePredecessorId: options.retirePredecessorId }); + } if (options.atomic) return store.seedStrandedPlanReviewContinuation(input); const item = await store.replaceActiveTaskWorkflowContinuation(input); return { seeded: true, workItemId: item.id }; @@ -120,9 +156,13 @@ export async function resumeApprovedPlanReviewHandoff( const seeded = await seedPreReleasePlanReviewContinuation(store, task, ir, { atomic: true }); if (seeded.seeded) return { resumed: true, reason: "seeded", workItemId: seeded.workItemId }; + // FNXC:PlanningHandoffAtomicity 2026-08-13-04:20: the no-node bail now carries its own reason + // token; this seam already pre-checked the node above, so map it to its legacy result name. return { resumed: false, - reason: seeded.reason ?? "not-plan-in-place", + reason: seeded.reason === undefined || seeded.reason === "no-pre-release-plan-review" + ? "not-plan-in-place" + : seeded.reason, }; } diff --git a/packages/engine/src/runtimes/in-process-runtime.ts b/packages/engine/src/runtimes/in-process-runtime.ts index 6e9e3b60ff..43706221f3 100644 --- a/packages/engine/src/runtimes/in-process-runtime.ts +++ b/packages/engine/src/runtimes/in-process-runtime.ts @@ -66,7 +66,7 @@ import { validateProjectNodeMapping } from "../project/node-dispatch-validation. import { attachAgentLinkSync } from "../agents/task-agent-sync.js"; import { createRunAuditor, generateSyntheticRunId } from "../util/run-audit.js"; import { setImmediate as setImmediateCb } from "node:timers"; -import { seedPreReleasePlanReviewContinuation } from "../plan-review-continuation.js"; +import { seedPreReleasePlanReviewContinuation, type PlanReviewSeedBailReason } from "../plan-review-continuation.js"; import { formatAdmissionCapacityQueuedReason, persistedTopLevelAgentTaskIdsFromStore, @@ -372,9 +372,18 @@ export interface SpecificationCompleteReactionDeps { outcome: PlanningHandoffOutcome; getTask: (taskId: string) => Promise; resolveIr: (taskId: string) => Promise; - seed: (task: Task, ir: WorkflowIr) => Promise<{ seeded: boolean; reason?: string }>; + seed: (task: Task, ir: WorkflowIr) => Promise<{ seeded: boolean; reason?: PlanReviewSeedBailReason }>; kick: () => void; log: (message: string) => void; + /* + FNXC:PlanningHandoffAtomicity 2026-08-13-03:49: + A seed outcome that is neither success nor a legitimate operator park is a broken + handoff that used to be dropped silently; it must be surfaced loudly because the + only remaining recovery owner is the FN-8592 self-healing sweep (~10 min later). + */ + warn: (message: string) => void; + /** Injectable delay for the bounded transient-failure retry; defaults to setTimeout. */ + sleep?: (ms: number) => Promise; } /** @@ -414,11 +423,69 @@ export async function reactToSpecificationComplete( return; } deps.log(`Specified ${deps.taskId} → todo`); - const live = await deps.getTask(deps.taskId); - if (!live || live.paused || live.userPaused) return; - const ir = await deps.resolveIr(live.id); - await deps.seed(live, ir); - deps.kick(); + /* + FNXC:PlanningHandoffAtomicity 2026-08-13-03:49: + The seed outcome was previously discarded, so a bailed handoff was invisible: the + seeder saw the caller's own still-running plan work item as an "active + continuation", returned {seeded:false}, and the card stranded in the hold column + until FN-8592 self-healing re-seeded it ~10 minutes later. The seed is now atomic + (it retires the named predecessor in the same transaction), so a bail here is a + genuine anomaly. Consume the result: retry a bounded number of times to absorb a + transient store error or a racing writer, then warn loudly — self-healing remains + the durable backstop, never the primary handoff. + + FNXC:PlanningHandoffAtomicity 2026-08-13-04:20: + The task and IR are re-fetched at the TOP OF EVERY ATTEMPT (review finding: a + single pre-loop snapshot let an operator pause or a replan landing during the + retry delays be ignored, arming Plan Review for a plan the operator had just + parked or superseded). The seeder re-applies the pause/approval guards from the + task object it is given, so a fresh snapshot per attempt is what makes those + guards current. A needs-replan status means the plan this handoff belongs to is + already superseded — quiet exit, the replan loop owns the card now. + QUIET_PARK_REASONS is typed against the seeder's exported reason union so a + renamed or added reason is a compile error here, not a silent misroute into the + retry/warn path. + */ + const sleep = deps.sleep ?? ((ms: number) => new Promise((resolve) => setTimeout(resolve, ms))); + const QUIET_PARK_REASONS: ReadonlySet = new Set([ + "no-pre-release-plan-review", + "awaiting-approval", + "paused", + "plan-review-passed", + ]); + const RETRY_DELAYS_MS = [1_000, 5_000]; + for (let attempt = 0; ; attempt++) { + let failure: string | null = null; + try { + const live = await deps.getTask(deps.taskId); + if (!live || live.paused || live.userPaused) return; + if (live.status === "needs-replan") { + deps.log(`Plan Review handoff for ${deps.taskId} not armed (needs-replan)`); + return; + } + const ir = await deps.resolveIr(live.id); + const result = await deps.seed(live, ir); + if (result.seeded) { + deps.kick(); + return; + } + if (result.reason && QUIET_PARK_REASONS.has(result.reason)) { + deps.log(`Plan Review handoff for ${deps.taskId} not armed (${result.reason})`); + return; + } + failure = result.reason ?? "not-seeded"; + } catch (error) { + failure = error instanceof Error ? error.message : String(error); + } + if (attempt >= RETRY_DELAYS_MS.length) { + deps.warn( + `Plan Review handoff for ${deps.taskId} failed after ${attempt + 1} attempts (${failure}) — stranded-continuation self-healing will recover it`, + ); + return; + } + deps.warn(`Plan Review handoff for ${deps.taskId} did not seed (${failure}) — retrying`); + await sleep(RETRY_DELAYS_MS[attempt]); + } } /** Everything the drain pass touches, injected so the pass is exercisable without @@ -1711,9 +1778,19 @@ export class InProcessRuntime outcome: report.outcome, getTask: (id) => Promise.resolve(this.taskStore.getTask(id)), resolveIr: (id) => resolveWorkflowIrForTask(this.taskStore, id), - seed: (task, ir) => seedPreReleasePlanReviewContinuation(this.taskStore, task, ir), + /* + FNXC:PlanningHandoffAtomicity 2026-08-13-03:49: + The report carries the planning session's own durable work item id so the + seeder can retire that exact predecessor row atomically with the successor + install. Without it the seeder raced triage's finally-block terminal + transition and bailed on its own caller (529 stranded cards in 18 days). + */ + seed: (task, ir) => seedPreReleasePlanReviewContinuation(this.taskStore, task, ir, { + retirePredecessorId: report.planningWorkItemId, + }), kick: () => this.kickWorkflowContinuationProcessor(), log: (message) => runtimeLog.log(message), + warn: (message) => runtimeLog.warn(message), }).catch((error) => { runtimeLog.error(`Failed to start Todo plan review for ${t.id}:`, error); }); diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index 2382be956f..d0225d55f6 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -348,6 +348,16 @@ export type PlanningHandoffOutcome = "released" | "parked" | "withheld"; * `finalizeApprovedTask` for why this is a report object and not a return value. */ export interface PlanningHandoffReport { outcome: PlanningHandoffOutcome; + /* + FNXC:PlanningHandoffAtomicity 2026-08-13-03:49: + The durable work item this planning session ran under. The runtime's + specification-complete reaction passes it to the Plan Review seeder so the + successor install can atomically retire this exact predecessor row instead of + bailing on it: triage announces completion BEFORE its finally block transitions + the row to succeeded, so without this id the seeder saw its own caller as an + "active continuation" and silently stranded the card for self-healing to repair. + */ + planningWorkItemId?: string; } @@ -3586,7 +3596,7 @@ export class TriageProcessor { // FNXC:TriagePlanningRetry 2026-08-03-00:20: A duplicate remains a separate closure // path, but fallback-authored or inherited markers cannot bypass clean-attempt admission. - const duplicateReport: PlanningHandoffReport = { outcome: "parked" }; + const duplicateReport: PlanningHandoffReport = { outcome: "parked", planningWorkItemId }; if (await this.tryFinalizeExplicitDuplicateMarker(task, written, settings, { isReplan, feedback, @@ -3609,6 +3619,7 @@ export class TriageProcessor { isReplan, feedback, }); + finalizeReport.planningWorkItemId = planningWorkItemId; this.options.onSpecifyComplete?.(task, finalizeReport); } finally { this.activeSessions.delete(task.id);