root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(core): startup recovery deadlocked on its own per-task lock (#2809)

Browse Source 
## 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) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-30 13:58:02 -07:00
committed by GitHub
parent 109204c590
commit a6af3188df
4 changed files with 255 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
.changeset/transition-pending-recovery-deadlock.md Normal file
View File

@@ -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.

4
packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts
View File

@@ -35,10 +35,6 @@ reason; a sync-resolved lifecycle guard is a guard that cannot fire.
*/
const ALLOWED_CALL_SITES: ReadonlyMap<string, string> = 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.",

44
packages/core/src/task-store/lifecycle-ops.ts
View File

@@ -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",

208
packages/engine/src/__tests__/transition-pending-recovery-deadlock.pg.test.ts Normal file
View File

@@ -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<typeof setTimeout> | undefined;
try {
return await Promise.race([
recoverStaleTransitionPendingImpl(store),
new Promise<never>((_, 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 `"<trait>: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<string> {
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 });
});
});