From a6af3188df2c5530130251b08d23f9a58e39f928 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 13:58:02 -0700 Subject: [PATCH] fix(core): startup recovery deadlocked on its own per-task lock (#2809) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The bug `recoverStaleTransitionPendingImpl` runs its whole per-task body inside `store.withTaskLock(id, …)`. On the PostgreSQL arm it then read the task with `store.getTask(id)` — and `getTaskImpl` opens with `store.withTaskLock(id, …)` too. **The per-task lock is non-reentrant.** This codebase states that invariant in prose in two other files: > "nesting inside `withTaskLock` would deadlock since the lock is non-reentrant" — `branch-and-pr-entities.ts:561` > "because the per-task lock is non-reentrant" — `workflow-ops.ts:464` So the sweep waited forever on a lock its own frame was holding. **PostgreSQL-only — which is every production install.** The SQLite arm on the very next line reads through `readTaskFromDb`, a lock-free row read. The backend-mode port swapped only the PostgreSQL arm to `getTask`. The fix restores a lock-free read (`readTaskRow`) on that arm; nothing else changes. ## Why it survived until now The branch is entered **only** when a stale marker names a plugin hook the trait registry still knows (`hasSurvivingPluginHook`). Three nearby cases all miss it: | marker | path | |---|---| | none | the row is never scanned | | only `default-workflow:postCommit` | `hasSurvivingPluginHook` false — marker just cleared | | names an **uninstalled** plugin hook | reconciled away as degraded; nothing survives to re-run | | names a **registered** plugin hook | **reaches the in-lock read → deadlock** | Those first three are what the existing tests cover. The fourth is precisely the state a crash mid-hook leaves behind. All four are asserted in the new suite so the path cannot be re-narrowed and called covered. ## Impact This sweep runs at **startup**. A task left with such a marker deadlocks startup recovery — and because it deadlocks *while holding the task's lock*, that task is also left permanently unlockable. ## How it was found, including a correction By **bisection**, not by reading. An earlier attempt of mine to drive this recovery reported that "the sweep never returns". That was wrong in a way worth recording: the sweep returns fine in three of the four cases, and generalising the one hang to the whole function is what hid the actual trigger across several sessions. Narrowing case by case — empty store, plain task, default-only marker, unknown-hook marker, registered-hook marker — put the fault on one line. ## Verification - **Mutation-verified against the real defect.** With the fix reverted, the regression case fails by name — `recoverStaleTransitionPendingImpl did not settle within 8000ms — deadlock` — while the other three stay green. That is the actual pre-fix behaviour, not a simulation of it. - Every case is **timeboxed** on purpose: a deadlock otherwise surfaces as a suite-level timeout naming no case, which is useless for locating the fault. The deadline is not a flake knob — the fixed code settles in ~150 ms and the broken code never settles, so there is no value in between to tune. - A **vacuity guard** (no markers → scans nothing) so a change that stopped listing marked rows can't leave the other cases green. - `pnpm test:gate` — **exit 0** - full live-PG E2E surface — **152/152** - `pnpm lint` — clean Changeset included (`patch`, category `fix`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../transition-pending-recovery-deadlock.md | 7 + ...ync-workflow-ir-callsite-allowlist.test.ts | 4 - packages/core/src/task-store/lifecycle-ops.ts | 44 +++- ...ition-pending-recovery-deadlock.pg.test.ts | 208 ++++++++++++++++++ 4 files changed, 255 insertions(+), 8 deletions(-) create mode 100644 .changeset/transition-pending-recovery-deadlock.md create mode 100644 packages/engine/src/__tests__/transition-pending-recovery-deadlock.pg.test.ts diff --git a/.changeset/transition-pending-recovery-deadlock.md b/.changeset/transition-pending-recovery-deadlock.md new file mode 100644 index 0000000000..7fb7d124bd --- /dev/null +++ b/.changeset/transition-pending-recovery-deadlock.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix a startup hang, and a skipped plugin hook, for tasks interrupted mid column-transition. +category: fix +dev: `recoverStaleTransitionPendingImpl` ran its per-task body inside `withTaskLock(id)` and then read the task with `store.getTask(id)`, which acquires the same non-reentrant lock. PostgreSQL-only — the SQLite arm already used the lock-free `readTaskFromDb`. Restores a lock-free read (`readTaskRow`) on the backend arm. Reachable only when a stale transition-pending marker names a plugin hook the trait registry still knows. Also switches that recovery's IR read from `resolveTaskWorkflowIrSync` (which returns the default workflow for every task under PostgreSQL, so a custom-workflow task's interrupted hook was silently skipped) to `resolveWorkflowIrForTask`, and drops the now-unused `lifecycle-ops.ts` entry from the sync-resolver call-site allow-list. diff --git a/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts b/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts index aa7fc111ea..4a1168e14f 100644 --- a/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts +++ b/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts @@ -35,10 +35,6 @@ reason; a sync-resolved lifecycle guard is a guard that cannot fire. */ const ALLOWED_CALL_SITES: ReadonlyMap = new Map([ - [ - "packages/core/src/task-store/lifecycle-ops.ts", - "Synchronous lifecycle bookkeeping inside a transaction.", - ], [ "packages/core/src/task-store/task-store-helpers.ts", "Synchronous helper shared by txn-hot paths.", diff --git a/packages/core/src/task-store/lifecycle-ops.ts b/packages/core/src/task-store/lifecycle-ops.ts index f333d6414d..283919abc4 100644 --- a/packages/core/src/task-store/lifecycle-ops.ts +++ b/packages/core/src/task-store/lifecycle-ops.ts @@ -32,7 +32,8 @@ import {getErrorMessage} from "../error-message.js"; import {type TaskRow} from "../task-store/persistence.js"; import {__setTaskActivityLogLimitsForTesting} from "../task-store/comments.js"; import {reconcileTaskIdStateAsync} from "../task-store/async-allocator.js"; -import {ACTIVE_TASK_FILTER, insertTaskRowInTransaction, isTaskIdConflictError as isPgTaskIdConflictError} from "./async-persistence.js"; +import {ACTIVE_TASK_FILTER, insertTaskRowInTransaction, isTaskIdConflictError as isPgTaskIdConflictError, readTaskRow} from "./async-persistence.js"; +import {resolveWorkflowIrForTask} from "../workflow-ir-resolver.js"; import {recordRunAuditEventWithinTransaction} from "../postgres/data-layer.js"; import * as schema from "../postgres/schema/index.js"; @@ -1045,11 +1046,46 @@ export async function recoverStaleTransitionPendingImpl(store: TaskStore): Promi // `default-workflow:postCommit` needs no re-run — just a clear). const hasSurvivingPluginHook = hooksRemaining.some((h) => h !== "default-workflow:postCommit"); if (hasSurvivingPluginHook) { + /* + FNXC:PostgresCutover 2026-07-31-15:40 (DEADLOCK, introduced by the backend-mode port above): + LOCK-FREE READ, and it must stay lock-free. This whole block runs inside + `store.withTaskLock(id, ...)`, and the per-task lock is NON-REENTRANT — the same invariant + `branch-and-pr-entities.ts` and `workflow-ops.ts` both state in prose. `store.getTask()` + acquires that lock (`getTaskImpl` opens with `store.withTaskLock(id, ...)`), so reading + through it here waits forever on a lock this very frame holds. + + The SQLite path on the line below never had the bug: `readTaskFromDb` is a lock-free row + read. The port swapped it for `getTask` on the backend arm only, so the deadlock is + PostgreSQL-only — which is every production install. + + Reachability is narrow but real, and it is exactly the state a crash leaves behind: the + branch runs only when a stale marker names a plugin hook the registry still knows + (`hasSurvivingPluginHook`). A marker with no plugin hook, or one naming an uninstalled + plugin, takes the degraded path and never reaches here — which is why every existing test + passes. This sweep runs at STARTUP, and it deadlocks while holding the task's lock, so the + affected task is also left permanently unlockable. + */ const task = backend - ? await store.getTask(id).catch(() => null) + ? await readTaskRow(store.asyncLayer!, id).catch(() => null) as { column?: string } | null : store.readTaskFromDb(id, { includeDeleted: false }); if (task) { - const ir = store.resolveTaskWorkflowIrSync(id); + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-18:40 (PR #2809 review — greptile P1): + ASYNC RESOLVER, because the sync one cannot answer here. `resolveTaskWorkflowIrSync` + returns the DEFAULT workflow IR for every task under PostgreSQL (its selection reader is + a cutover stub that answers `undefined` unconditionally). The hook runner below derives + its pending set from the columns of the IR it is handed, so with the default IR a task on + a CUSTOM workflow matched no plugin trait and its interrupted hook was silently skipped — + the recovery reported success having re-run nothing. + + Nothing forced the sync call: this frame is already `async`, and awaiting here does not + reorder anything (the marker read above is awaited on the same path). The sync reader was + simply the one the SQLite-era code had. + + This site is removed from the `resolveTaskWorkflowIrSync` call-site allow-list in the same + change, so the two cannot drift. + */ + const ir = await resolveWorkflowIrForTask(store, id); // fromColumn is unknown post-crash; the marker only records toColumn. // The hook runner keys onEnter off toColumn (and onExit off fromColumn); // re-running onEnter for the destination is the recoverable, idempotent @@ -1057,7 +1093,7 @@ export async function recoverStaleTransitionPendingImpl(store: TaskStore): Promi // toColumn at marker-write time, so current == toColumn and onExit is a // no-op, which is correct — we never re-fire an exit we may have run). try { - await store.runPluginColumnTransitionHooks(id, ir, task.column, live.toColumn); + await store.runPluginColumnTransitionHooks(id, ir, task.column as string, live.toColumn); } catch (err) { storeLog.warn("transitionPending recovery: hook re-run faulted (degraded)", { phase: "recover-stale-transition-pending", diff --git a/packages/engine/src/__tests__/transition-pending-recovery-deadlock.pg.test.ts b/packages/engine/src/__tests__/transition-pending-recovery-deadlock.pg.test.ts new file mode 100644 index 0000000000..c8132f305e --- /dev/null +++ b/packages/engine/src/__tests__/transition-pending-recovery-deadlock.pg.test.ts @@ -0,0 +1,208 @@ +/* +FNXC:PostgresCutover 2026-07-31-15:55 (regression — the startup sweep deadlocked on itself): + +`recoverStaleTransitionPendingImpl` runs its whole per-task body inside `store.withTaskLock(id, ...)`. +On the PostgreSQL arm it then read the task with `store.getTask(id)` — and `getTaskImpl` opens with +`store.withTaskLock(id, ...)` too. The per-task lock is NON-REENTRANT, an invariant this codebase +states in prose in two other files ("nesting inside withTaskLock would deadlock since the lock is +non-reentrant" in `branch-and-pr-entities.ts`; "because the per-task lock is non-reentrant" in +`workflow-ops.ts`). So the sweep waited forever on a lock its own frame was holding. + +SQLite never had it: that arm reads through `readTaskFromDb`, a lock-free row read. The backend port +swapped only the PostgreSQL arm to `getTask`, so the deadlock is PostgreSQL-only — which is every +production install. The fix restores a lock-free read (`readTaskRow`) on that arm. + +WHY IT SURVIVED: the branch is entered only when a stale marker names a plugin hook the trait +registry still knows (`hasSurvivingPluginHook`). Three nearby cases all miss it — + no marker at all -> the row is not scanned + marker with only the default -> `hasSurvivingPluginHook` is false; marker is just cleared + marker naming an UNKNOWN hook-> reconciled away as degraded, so nothing survives to re-run +— and those are the cases the existing tests cover. Only a marker naming a REGISTERED plugin hook +reaches the read, which is precisely the state a crash mid-hook leaves behind. All four are asserted +below so the next change cannot re-narrow the path and call it covered. + +FOUND BY BISECTION, not by reading. An earlier attempt to drive this recovery reported that "the +sweep never returns", which was wrong in a way worth recording: the sweep returns fine in three of +the four cases, and generalising the one hang to the whole function is what hid the actual trigger +for several sessions. The bisection is the four cases below. + +EVERY CASE IS TIMEBOXED. A deadlock regression manifests as a hang, and a hung test is reported as a +suite-level timeout that names no case — useless for locating the fault. Racing each call against an +explicit deadline turns it into a normal assertion failure that names the case and the condition. + +LANE. `.pg.test.ts`, skipped via `pgDescribe` when no PostgreSQL is reachable. Throwaway per-file +database; never port 4040. +*/ +import { beforeAll, beforeEach, afterEach, afterAll, expect, it } from "vitest"; +import "@fusion/core"; // registers the built-in column traits +import { + getTraitRegistry, + makeTransitionPending, + registerTraitHookImpl, + type TaskStore, +} from "@fusion/core"; + +import { recoverStaleTransitionPendingImpl } from "../../../core/src/task-store/lifecycle-ops.js"; +import { RENAMED_VOCAB, lifecycleIr } from "./_workflow-vocabulary-fixture.js"; +import { writeTransitionPendingAsync } from "../../../core/src/task-store/async-transition-pending.js"; +import { + pgDescribe, + createSharedPgTaskStoreTestHarness, + type SharedPgTaskStoreHarness, +} from "../../../core/src/__test-utils__/pg-test-harness.js"; + +/** Generous next to a sweep over one row, tight next to a deadlock. Not a flake knob: the fixed code + * completes in milliseconds and the broken code never completes at all, so there is no value in + * between for this to be tuned to. */ +const DEADLINE_MS = 8_000; + +const REGISTERED_TRAIT = "plugin:transition-pending-regression"; + +/** Set by the registered hook when the runner actually invokes it. */ +const firedFor: string[] = []; + +async function sweepWithin(store: TaskStore): Promise<{ scanned: number; recovered: number; degradedHooks: number }> { + let timer: ReturnType | undefined; + try { + return await Promise.race([ + recoverStaleTransitionPendingImpl(store), + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new Error(`recoverStaleTransitionPendingImpl did not settle within ${DEADLINE_MS}ms — deadlock`)), + DEADLINE_MS, + ); + }), + ]); + } finally { + if (timer) clearTimeout(timer); + } +} + +pgDescribe("stale transition-pending recovery does not deadlock on the per-task lock", () => { + const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ + prefix: "fusion_transition_deadlock", + }); + + beforeAll(async () => { + await h.beforeAll(); + /* A real plugin trait with a real hook, so `knownHookIds` contains it and a marker naming it + counts as SURVIVING. Registered through the production registry — nothing here is a stand-in. */ + getTraitRegistry().register({ + id: REGISTERED_TRAIT, + name: "transition pending regression probe", + flags: {}, + hooks: { onEnter: { id: `${REGISTERED_TRAIT}:onEnter` } }, + } as never); + /* THREE arguments — `(traitId, hookKind, impl)`. Registering with a composed `":onEnter"` + id and no kind silently registers nothing: the runner then resolves the hook to a no-op, so the + suite reads as "the hook never fired" and invites the conclusion that the recovery is broken. + Cost me a wrong diagnosis before the signature was checked. */ + registerTraitHookImpl( + REGISTERED_TRAIT, + "onEnter" as never, + (async (ctx: { task?: { id?: string } }) => { firedFor.push(ctx?.task?.id ?? "unknown"); }) as never, + ); + }); + afterAll(h.afterAll); + beforeEach(async () => { firedFor.length = 0; await h.beforeEach(); }); + afterEach(async () => { await h.afterEach(); }); + + /** A card carrying a stale transition-pending marker with the given hook ids. */ + async function markedCard(store: TaskStore, hookIds: string[], description: string): Promise { + const task = await store.createTask({ description }); + await writeTransitionPendingAsync( + store.asyncLayer!.db, + task.id, + makeTransitionPending("todo", hookIds, Date.now() - 10 * 60_000), + ); + return task.id; + } + + it("REGRESSION — a marker naming a REGISTERED plugin hook settles instead of hanging", async () => { + /* + The deadlock case, and the only one of the four that reaches the in-lock task read. Before the + fix this never returned; the assertion below was never reached and the suite died on a timeout + that named no case. + */ + const store = h.store(); + await markedCard(store, [`${REGISTERED_TRAIT}:onEnter`, "default-workflow:postCommit"], "registered hook"); + + const result = await sweepWithin(store); + + expect(result.scanned).toBe(1); + expect(result.recovered).toBe(1); + }); + + it("a marker naming an UNKNOWN plugin hook is reconciled as degraded", async () => { + /* Never reached the read — the unknown hook is reconciled away, so nothing survives to re-run. + Pinned so the fix cannot be "fixed" by narrowing the surviving-hook test instead. */ + const store = h.store(); + await markedCard(store, ["plugin:not-installed:onEnter", "default-workflow:postCommit"], "unknown hook"); + + const result = await sweepWithin(store); + + expect(result.recovered).toBe(1); + expect(result.degradedHooks).toBe(1); + }); + + it("a marker with only the default hook is cleared without a re-run", async () => { + const store = h.store(); + await markedCard(store, ["default-workflow:postCommit"], "default only"); + + const result = await sweepWithin(store); + + expect(result.recovered).toBe(1); + expect(result.degradedHooks).toBe(0); + }); + + it("REGRESSION — the interrupted hook is re-run for a task on a CUSTOM workflow", async () => { + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-18:55 (PR #2809 review — greptile P1): + THE SECOND DEFECT ON THIS LINE, and it only became reachable once the deadlock above was fixed. + The recovery resolved the task's IR with `resolveTaskWorkflowIrSync`, which hands back the DEFAULT + workflow for every task under PostgreSQL. The hook runner derives its pending set from the + columns of the IR it is given, so a task on a RENAMED board matched no plugin trait: the + interrupted hook was never re-run and the sweep reported success having done nothing. + + Observed state, not a spy on the resolver: the hook itself records the task id it ran for. The + board is renamed so the default IR cannot supply the column by accident — the `building` column + carrying the plugin trait exists on this workflow and on no other. + */ + const store = h.store(); + const ir = lifecycleIr(RENAMED_VOCAB, "custom:transition-pending") as unknown as { + columns: { id: string; traits: unknown[] }[]; + }; + ir.columns = ir.columns.map((column) => column.id === RENAMED_VOCAB.wip + ? { ...column, traits: [...column.traits, { trait: REGISTERED_TRAIT }] } + : column); + const created = await store.createWorkflowDefinition({ + name: "Transition pending custom board", + kind: "workflow", + ir, + } as never); + + const task = await store.createTask({ description: "custom board, interrupted hook" }); + await store.writeTaskWorkflowSelection(task.id, (created as { id: string }).id, []); + store.taskCache.delete(task.id); + await store.moveTask(task.id, RENAMED_VOCAB.wip as never, { recoveryRehome: true } as never); + await writeTransitionPendingAsync( + store.asyncLayer!.db, + task.id, + makeTransitionPending(RENAMED_VOCAB.wip, [`${REGISTERED_TRAIT}:onEnter`, "default-workflow:postCommit"], Date.now() - 10 * 60_000), + ); + + const result = await sweepWithin(store); + + expect(result.recovered).toBe(1); + expect(firedFor).toContain(task.id); + }); + + it("a store with no markers scans nothing", async () => { + /* The vacuity guard: without it, a change that stopped listing marked rows would leave every + case above green while the recovery did nothing at all. */ + const store = h.store(); + await store.createTask({ description: "no marker" }); + + expect(await sweepWithin(store)).toEqual({ scanned: 0, recovered: 0, degradedHooks: 0 }); + }); +});