From c3df0f641b4c10689ccef01d2704470858be5e58 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 19:59:03 -0700 Subject: [PATCH] executor: orphaned tasks were never resumed after a restart on a renamed board (#2947) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resumeOrphaned` is the only path that recovers tasks after a crash or restart. On a board with renamed columns it recovered **nothing**. ## A missed pair, not an unconverted read ```ts const tasks = await this.listWipLaneTasks(); // resolved by role — already converted const inProgress = tasks.filter( (t) => t.column === "in-progress" && …, // literal — discards everything the read found ); ``` The read was already resolved. The filter directly beneath it re-asserted the literal on the rows that read returned, so the sweep found the orphans and threw them all away. **This is the worse half of the class, and it hid well:** - the read *looks* converted, so scanning for `listTasks({ column: "…" })` finds nothing; - the census scores only the comparison, so the backlog number moves the **wrong way** as you convert; - a **structural test already existed** pinning "the read asks for resolved lanes" — `executor-resume-query-lanes.test.ts` — and it was green the entire time the sweep was dead. A test asserting the read exists says nothing about the filter beneath it. The failure only surfaces after a crash, when an operator is already investigating the crash and has every reason to blame that instead. ## The ratchet, generalised #2944 ratcheted this class inside `self-healing.ts` after review found one instance and a follow-up audit found five more. This generalises it to every engine source: a function that resolves lanes **and** compares a column id in the same body is a pair. Excluded, deliberately: - the **fallback arm** of a resolved ternary (`lanes ? lanes.has(c) : c === "done"`) — the correct shape; - four files whose literals are deliberate, each with the reason recorded: `ephemeral-worker-manager` (unresolvable-workflow default), `triage` (the U11 orphan case), `scheduler` and `replan-target` (sync listeners on the inert sync IR reader, already pinned by `sync-workflow-ir-is-always-default.pg.test.ts`); - `self-healing.ts`, because it has a **dedicated** ratchet that is strictly more precise. Two ratchets allowlisting the same site is one fact with two owners, free to drift — the exact failure mode this program keeps hitting. One file, one ratchet. It carries a positive control: a wrong source path would make every case pass by scanning nothing. **I swept the rest of the engine with it and executor.ts was the only genuine hit** — everything else is documented-deliberate or blocked on the inert sync reader. ## Revert results Each measured by restoring the literal filter and re-running: | | reverted → | | --- | --- | | behavioural case | fails — the renamed card is dropped and the sweep returns before touching it | | the ratchet | fails, naming the site: `resumeOrphaned: executor.ts:5974` | A non-vacuous companion (card in the review lane → not resumed) rules out a filter that matches everything: a card in review has no session to resume, and re-dispatching it would restart finished work. **Measured:** `executor.ts` column guards 8 → 7; baseline re-recorded downward. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; executor prompt/soft-delete/resume suites plus the new ratchet, 357 passed; `tsc` engine clean; `pnpm lint`, `check:changesets`, census `--strict` and `check-sql-column-literals` clean, each run explicitly. --- .changeset/executor-resume-orphaned-lane.md | 7 + .../executor-resume-lanes-resolved.test.ts | 63 ++++++++ .../resolved-read-with-literal-filter.test.ts | 151 ++++++++++++++++++ packages/engine/src/executor.ts | 16 +- .../lib/lifecycle-column-census-baseline.json | 2 +- 5 files changed, 237 insertions(+), 2 deletions(-) create mode 100644 .changeset/executor-resume-orphaned-lane.md create mode 100644 packages/engine/src/__tests__/resolved-read-with-literal-filter.test.ts diff --git a/.changeset/executor-resume-orphaned-lane.md b/.changeset/executor-resume-orphaned-lane.md new file mode 100644 index 0000000000..30201b52c9 --- /dev/null +++ b/.changeset/executor-resume-orphaned-lane.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Orphaned tasks are resumed after a restart on boards with renamed columns. +category: fix +dev: `resumeOrphaned` read the wip lane by role via `listWipLaneTasks()` but its filter still compared `t.column === "in-progress"`, so on a renamed board the read found the orphans and the filter discarded all of them. The filter now tests membership of the resolved wip columns. diff --git a/packages/engine/src/__tests__/executor-resume-lanes-resolved.test.ts b/packages/engine/src/__tests__/executor-resume-lanes-resolved.test.ts index 11fa99a39e..3ed646d060 100644 --- a/packages/engine/src/__tests__/executor-resume-lanes-resolved.test.ts +++ b/packages/engine/src/__tests__/executor-resume-lanes-resolved.test.ts @@ -217,3 +217,66 @@ describe("resume lanes come from the task's own workflow", () => { }); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-19:30 (a MISSED PAIR in resumeOrphaned): +`listWipLaneTasks()` already resolved the wip lane by role, and the filter beneath it did NOT — it +re-asserted the literal `in-progress` on the rows that read returned. So on a renamed board the read +found the orphans and the filter discarded every one. + +That is the worse half of this pattern and the reason the sibling structural test was not enough: the +read looks converted, the census scores only the comparison, and the sweep silently does nothing. The +consequence here is that orphaned tasks are NEVER resumed after a crash or restart — the single path +that recovers them — and the failure surfaces only once an operator is already investigating a crash. + +`isTaskWorkComplete` is the first thing done per surviving task, before any dispatch, worktree probing +or git, so it is the observable that needs no live registry. + +REVERT CHECK, measured: with the filter back on `t.column === "in-progress"`, this fails — the renamed +card is dropped and the sweep returns before touching it. +*/ +describe("resumeOrphaned filters by the board's OWN wip lane, not the literal", () => { + function orphanHarness(column: string) { + const { store, executor } = harness(RENAMED_IR); + const task = { + id: "FN-ORPHAN-RESUME", + column, + title: "orphaned by a restart", + description: "", + dependencies: [], + steps: [{ id: "s1", status: "pending" }], + currentStep: 0, + log: [], + createdAt: "2026-07-30T00:00:00.000Z", + updatedAt: "2026-07-30T00:00:00.000Z", + }; + const widened = store as unknown as Record; + widened.getSettings = async () => ({ globalPause: false, enginePaused: false }); + widened.listTasks = vi.fn(async (options?: { column?: string }) => + (options?.column === undefined || options.column === column ? [task] : [])); + widened.listWorkflowDefinitions = async () => [{ ir: RENAMED_IR }]; + const isTaskWorkComplete = vi.fn(() => true); + Object.assign(executor, { isTaskWorkComplete, recoverCompletedTask: vi.fn(async () => undefined) }); + return { executor, isTaskWorkComplete }; + } + + it("reaches an orphan sitting in the RENAMED wip lane", async () => { + const { executor, isTaskWorkComplete } = orphanHarness("building"); + + await executor.resumeOrphaned(); + + expect(isTaskWorkComplete).toHaveBeenCalledWith(expect.objectContaining({ id: "FN-ORPHAN-RESUME" })); + }); + + it("does not resume a card outside the wip lane", async () => { + /* + Non-vacuous companion: a card in the board's REVIEW lane is not an orphaned execution — it has no + session to resume, and re-dispatching it would restart finished work. + */ + const { executor, isTaskWorkComplete } = orphanHarness("checking"); + + await executor.resumeOrphaned(); + + expect(isTaskWorkComplete).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/engine/src/__tests__/resolved-read-with-literal-filter.test.ts b/packages/engine/src/__tests__/resolved-read-with-literal-filter.test.ts new file mode 100644 index 0000000000..c2d50a92de --- /dev/null +++ b/packages/engine/src/__tests__/resolved-read-with-literal-filter.test.ts @@ -0,0 +1,151 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-31-19:55 (the MISSED PAIR ratchet, generalised past self-healing): + +THE DEFECT THIS CATCHES, found in `executor.ts` after the sibling ratchet found five in +`self-healing.ts`: a function resolves its lane by ROLE and then re-asserts a column LITERAL on the +rows it just read. + + const tasks = await this.listWipLaneTasks(); // resolved by role + const inProgress = tasks.filter((t) => t.column === "in-progress" && …); // literal + +On a renamed board the read finds the work and the filter discards all of it. This is worse than an +unconverted read, and much harder to notice: + + - the read LOOKS converted, so a reviewer scanning for `listTasks({ column: "…" })` sees nothing; + - the census scores only the comparison, so the backlog number moves the wrong way; + - a STRUCTURAL test that pins "the read asks for resolved lanes" passes — `resumeOrphaned` had one, + and it was green for the entire time the sweep was dead. + +`resumeOrphaned` is the only path that recovers orphaned tasks after a crash or restart, so its +failure surfaced only when an operator was already investigating a crash. + +WHAT COUNTS AS A PAIR: the same function both resolves lanes and compares a column id. The fallback +arm of a resolved ternary is NOT a pair — `lanes ? lanes.has(c) : c === "done"` is the correct shape, +and the literal only answers when resolution produced nothing. +*/ +import { describe, expect, it } from "vitest"; +import { readFileSync, readdirSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { join } from "node:path"; + +const SRC = fileURLToPath(new URL("..", import.meta.url)); + +const RESOLVER_CALLS = [ + "resolveProjectColumnsForRoles(", + "listWipLaneTasks(", + "resolveWorkflowIrForTaskWithProvenance(", +]; + +const LITERAL = /\.column\s*(?:!==|===)\s*"(?:todo|in-progress|in-review|done|archived|triage)"/; +/** `lanes ? lanes.has(task.column) : task.column === "done"` — the resolved answer wins; correct. */ +const RESOLVED_FALLBACK_ARM = /(?:\.includes|\.has)\(\s*[A-Za-z_.]*\.column\s*\)/; + +/* +Documented exceptions. Each is a literal that survives ON PURPOSE, with the reason recorded at the +site. An entry here asserts the degraded answer is harmless — not that the literal is invisible. +*/ +const ALLOWED: ReadonlyArray<{ file: string; because: string }> = [ + { + file: "ephemeral-worker-manager.ts", + because: "the unresolvable-workflow default: when no IR resolves there is nothing to resolve against", + }, + { + file: "triage.ts", + because: "the U11 orphan case — a row resting in a column its workflow no longer declares has no trait to resolve", + }, + { + file: "scheduler.ts", + because: "sync event listeners; resolveTaskWorkflowIrSync is inert in production and adding an await reorders handlers (pinned by sync-workflow-ir-is-always-default.pg.test.ts)", + }, + { + file: "replan-target.ts", + because: "same inert sync reader as scheduler.ts", + }, +]; + +function stripComments(source: string): string[] { + const out: string[] = []; + let inBlock = false; + for (const line of source.split("\n")) { + const trimmed = line.trim(); + if (inBlock) { + out.push(""); + if (line.includes("*/")) inBlock = false; + continue; + } + if (trimmed.startsWith("/*")) { + out.push(""); + if (!line.includes("*/")) inBlock = true; + continue; + } + if (trimmed.startsWith("//")) { out.push(""); continue; } + out.push(line); + } + return out; +} + +/* +Files with a DEDICATED ratchet of their own are skipped here, not double-allowlisted. + +`self-healing.ts` is owned by `self-healing-converted-sweeps-have-no-literal-lane-guards.test.ts`, +which is strictly more precise: it derives its converted-sweep list from the source and allows exactly +one documented literal (the log-dedup closure in `clearStaleBlockedBy`). Adding a second allowance for +the same site here would give one fact two owners that can drift apart — the failure mode this whole +program keeps hitting. One file, one ratchet. +*/ +const OWNED_ELSEWHERE = new Set(["self-healing.ts"]); + +/** Engine sources, excluding tests. Shallow by design — the class lives in the big lane files. */ +function engineSources(): string[] { + return readdirSync(SRC) + .filter((name) => name.endsWith(".ts") && !name.endsWith(".d.ts") && !OWNED_ELSEWHERE.has(name)) + .map((name) => join(SRC, name)); +} + +function owningFunction(lines: string[], index: number): string | null { + for (let i = index; i >= 0; i--) { + const match = /^ {2}(?:private |public |static )*(?:async )?([a-zA-Z][A-Za-z0-9_]*)\s*[(<]/.exec(lines[i]!); + if (match) return match[1]!; + } + return null; +} + +describe("a function that resolves lanes does not also compare a column id", () => { + const files = engineSources(); + + it("finds engine sources to scan (guards against a vacuous sweep)", () => { + /* A wrong SRC path would make every case below pass by scanning nothing. */ + expect(files.length).toBeGreaterThan(20); + }); + + for (const path of files) { + const name = path.split("/").pop()!; + const allowance = ALLOWED.find((entry) => entry.file === name); + + it(`${name} keeps no literal beside a resolved read`, () => { + const lines = stripComments(readFileSync(path, "utf8")); + const byFunction = new Map(); + + lines.forEach((line, index) => { + const owner = owningFunction(lines, index); + if (!owner) return; + const entry = byFunction.get(owner) ?? { resolves: false, literals: [] }; + if (RESOLVER_CALLS.some((call) => line.includes(call))) entry.resolves = true; + if (LITERAL.test(line) && !RESOLVED_FALLBACK_ARM.test(line)) { + entry.literals.push(`${name}:${index + 1} — ${line.trim()}`); + } + byFunction.set(owner, entry); + }); + + const pairs = [...byFunction.entries()] + .filter(([, entry]) => entry.resolves && entry.literals.length > 0) + .flatMap(([fn, entry]) => entry.literals.map((l) => `${fn}: ${l}`)); + + if (allowance) { + expect(pairs.length, `${name} is allowed documented literals (${allowance.because}) — review any change here:\n${pairs.join("\n")}`).toBeGreaterThanOrEqual(0); + return; + } + expect(pairs, `${name} resolves lanes AND compares a column id in the same function:\n${pairs.join("\n")}`).toEqual([]); + }); + } +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 9203ee7364..69ad821d28 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -5955,9 +5955,23 @@ export class TaskExecutor { return; } + /* + FNXC:WorkflowResolvedColumns 2026-07-31-19:20 (a MISSED PAIR, the class #2879 ratcheted): + `listWipLaneTasks()` above already resolves the wip lane by role. This filter did not — it re-asserted + the literal `in-progress` on the rows that read returned, so on a renamed board the read found the + orphans and the filter dropped every one. + + That is the worse half of the pattern: the read looks converted, the census counts only the + comparison, and the sweep silently does nothing. Here it means orphaned tasks are NEVER resumed after + a crash or restart — the one path that recovers them. + + The rows come from a `listTasks({ column })` per resolved column, so a row is in that column by + definition; the re-assert only ever had value as a stale-snapshot guard, which membership preserves. + */ + const wipColumns = await resolveProjectColumnsForRoles(this.store, ["countsTowardWip"]); const tasks = await this.listWipLaneTasks(); const inProgress = tasks.filter( - (t) => t.column === "in-progress" && !t.deletedAt && !this.executing.has(t.id) && !t.paused, + (t) => wipColumns.has(t.column) && !t.deletedAt && !this.executing.has(t.id) && !t.paused, ); if (inProgress.length === 0) return; diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index c5d34d5d5e..554d92f495 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -3,7 +3,7 @@ "byFile": { "packages/engine/src/self-healing.ts": 56, "packages/engine/src/scheduler.ts": 12, - "packages/engine/src/executor.ts": 8, + "packages/engine/src/executor.ts": 7, "packages/engine/src/notification/notification-service.ts": 5, "packages/engine/src/replan-target.ts": 4, "packages/engine/src/restart-recovery-coordinator.ts": 4,