fix(engine): heartbeat asked 'is this task finished?' with legacy ids — and one of the two sites writes status:failed onto completed work (#2769)
## Two heartbeat sites asked "is this task finished?" with the legacy ids `agent-heartbeat.ts` **4 → 0**. Neither site is cosmetic. **Linked-task clear.** The heartbeat clears an agent's assignment once its card is finished. Keyed on the literals, an agent on a renamed board stayed bound to a **completed** card indefinitely — every later heartbeat ran with stale task context instead of picking up new work, and nothing else clears it. **Worktree-acquisition gate.** Its failure bookkeeping runs only for a **non-terminal** task. A card in a renamed complete lane read as non-terminal, so an acquisition failure could stamp `status: "failed"` and an error message onto work that was **already done**. That second site *writes*, which drives the fallback direction: an unresolvable workflow degrades toward "terminal", because treating a finished card as unfinished is the expensive mistake here. Both sit in async paths — the first has `await taskStore.getTask(...)` three lines above — so this is an `await`, not a restructure. Extracted to one predicate rather than converted twice: they are the same question, and the two must not drift when one of them acts destructively. ## How this was found, and the part worth recording Generalising #2767. That PR marked a documented false positive the census kept advertising, so I swept for **other** files whose lifecycle literals were reasoned about in prose but still counted — to find out whether the trap was systemic. **It is not.** Of twelve candidate files, only this one carried real unconverted guards, and its "false positive" mentions turned out to be unrelated (detection heuristics, not column literals). The sweep mostly came back **negative**, and that is worth saying so nobody repeats it expecting a haul. ## Revert proof | reverted | result | |---|---| | neuter the resolution (predicate → literals) | **3 failed** / 2 passed | | shipped | **5 passed** | The two that survive the revert are the degraded-mode pair, which is correct — they assert the *legacy* answer, so they must pass either way. One case also pins that a legacy `done` id is **not** terminal on a board that does not declare it, which is what a board-wide union would get wrong. ## Verification - 13 heartbeat suites — **501 passed** (496 before, +5 new) - `pnpm test:gate` — **158 / 10 / 487 / 71** · `pnpm lint` clean · engine `tsc --noEmit` **0 errors** · `--strict` exits 0 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,79 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { isTaskInTerminalLane } from "../agent-heartbeat.js";
|
||||
import type { TaskStore, WorkflowIr } from "@fusion/core";
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-08-01-07:20 (fleet — two heartbeat terminal checks):
|
||||
|
||||
Both call sites asked "is this task finished?" with `column === "done" || "archived"`, and neither is
|
||||
cosmetic:
|
||||
|
||||
LINKED-TASK CLEAR. The heartbeat clears an agent's assignment once its card is finished. Keyed on
|
||||
the literals, an agent on a renamed board stayed bound to a COMPLETED card indefinitely, so every
|
||||
later heartbeat ran with stale task context instead of picking up new work.
|
||||
|
||||
WORKTREE-ACQUISITION GATE. Its failure bookkeeping runs only for a NON-terminal task. A card in a
|
||||
renamed complete lane read as non-terminal, so an acquisition failure could stamp
|
||||
`status: "failed"` and an error onto work that was already done. That site WRITES, which is why
|
||||
the fallback below degrades toward "terminal" rather than "unfinished".
|
||||
|
||||
Tested at the predicate because it IS the conversion — both call sites are one-line swaps to it. The
|
||||
cases are differential: one workflow SHAPE, two vocabularies, only the ids differ, so any difference
|
||||
between the runs is attributable to a surviving literal.
|
||||
*/
|
||||
|
||||
const RENAMED_IR = {
|
||||
version: "v2",
|
||||
id: "custom:renamed-terminal",
|
||||
nodes: [],
|
||||
edges: [],
|
||||
columns: [
|
||||
{ id: "drafting", label: "Drafting", traits: [{ trait: "hold", config: { release: "capacity" } }] },
|
||||
{ id: "building", label: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] },
|
||||
{ id: "shipped", label: "Shipped", traits: [{ trait: "complete" }] },
|
||||
{ id: "filed", label: "Filed", traits: [{ trait: "archived" }] },
|
||||
],
|
||||
} as unknown as WorkflowIr;
|
||||
|
||||
function storeFor(ir: WorkflowIr | undefined): TaskStore {
|
||||
const selection = { workflowId: "custom:renamed-terminal", stepIds: [] as string[] };
|
||||
return {
|
||||
getTaskWorkflowSelection: vi.fn(() => undefined),
|
||||
getTaskWorkflowSelectionAsync: vi.fn(async () => (ir ? selection : undefined)),
|
||||
getWorkflowDefinition: vi.fn(async () => (ir ? { ir } : undefined)),
|
||||
} as unknown as TaskStore;
|
||||
}
|
||||
|
||||
describe("heartbeat resolves the terminal lanes from the task's own board", () => {
|
||||
it("recognises a RENAMED complete lane as terminal", async () => {
|
||||
expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "shipped" })).toBe(true);
|
||||
});
|
||||
|
||||
it("recognises a RENAMED archived lane as terminal", async () => {
|
||||
expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "filed" })).toBe(true);
|
||||
});
|
||||
|
||||
/* The paired negative, and the one that matters most: the writing call site must not fire on it. */
|
||||
it("does NOT treat an active lane as terminal", async () => {
|
||||
expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "building" })).toBe(false);
|
||||
expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "drafting" })).toBe(false);
|
||||
});
|
||||
|
||||
/*
|
||||
A card sitting on a legacy id that the RENAMED board does not declare is NOT terminal there. This is
|
||||
the case a `terminal.has(column)`-style union would get wrong, and it is why the check compares
|
||||
against the task's own resolved lanes rather than a board-wide set.
|
||||
*/
|
||||
it("does not treat a legacy id as terminal on a board that does not declare it", async () => {
|
||||
expect(await isTaskInTerminalLane(storeFor(RENAMED_IR), { id: "FN-1", column: "done" })).toBe(false);
|
||||
});
|
||||
|
||||
/* Degraded mode: an unresolvable workflow keeps exactly the legacy answer, both directions. */
|
||||
it("falls back to the legacy pair when the workflow cannot be resolved", async () => {
|
||||
const store = storeFor(undefined);
|
||||
|
||||
expect(await isTaskInTerminalLane(store, { id: "FN-1", column: "done" })).toBe(true);
|
||||
expect(await isTaskInTerminalLane(store, { id: "FN-1", column: "archived" })).toBe(true);
|
||||
expect(await isTaskInTerminalLane(store, { id: "FN-1", column: "in-progress" })).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -36,6 +36,7 @@ import {
|
||||
resolveEffectivePlannerHeartbeatPatrolEnabled,
|
||||
resolveReboundTarget,
|
||||
resolveWorkflowIrForTask,
|
||||
resolveTaskLifecycleColumns,
|
||||
} from "@fusion/core";
|
||||
import type { ToolDefinition } from "@earendil-works/pi-coding-agent";
|
||||
import { Type, type Static } from "@earendil-works/pi-ai";
|
||||
@@ -676,6 +677,35 @@ async function getHeartbeatMemorySettings(taskStore: TaskStore): Promise<Setting
|
||||
* Detects missed heartbeats, auto-terminates unresponsive agents,
|
||||
* and provides the Paperclip-style execution engine via executeHeartbeat().
|
||||
*/
|
||||
/**
|
||||
* FNXC:WorkflowLifecycleColumns 2026-08-01-07:20 (fleet — heartbeat terminal checks):
|
||||
* Is this task finished — resting in its OWN board's complete or archived lane?
|
||||
*
|
||||
* Both heartbeat call sites asked with `column === "done" || "archived"`. Neither is cosmetic:
|
||||
*
|
||||
* - the linked-task check clears an agent's assignment once its card is finished. Keyed on the
|
||||
* literals, an agent on a renamed board stayed bound to a completed card indefinitely, so every
|
||||
* later heartbeat ran with stale task context instead of picking up new work.
|
||||
* - the worktree-acquisition retry gate runs its failure bookkeeping only for a NON-terminal task.
|
||||
* A card resting in a renamed complete lane read as non-terminal, so an acquisition failure
|
||||
* could stamp `status: "failed"` and an error onto work that was already done.
|
||||
*
|
||||
* Fail-soft to the legacy pair: an unresolvable workflow keeps exactly today's answer rather than
|
||||
* treating every card as unfinished, which is the expensive direction here (the second site WRITES).
|
||||
*/
|
||||
export async function isTaskInTerminalLane(
|
||||
taskStore: TaskStore,
|
||||
task: { id: string; column: string },
|
||||
cache?: Map<string, import("@fusion/core").WorkflowIr>,
|
||||
): Promise<boolean> {
|
||||
const columns = await resolveTaskLifecycleColumns(taskStore, task.id, cache).catch(() => undefined);
|
||||
/* DELIBERATE-LITERAL — the no-metadata fallback. Deleting it makes an unresolvable workflow read
|
||||
as NEVER terminal, which is the direction that writes: the second call site would then run its
|
||||
failure bookkeeping against finished work. Strictly worse than the legacy answer. */
|
||||
if (!columns) return task.column === "done" || task.column === "archived";
|
||||
return task.column === columns.complete || task.column === columns.archived;
|
||||
}
|
||||
|
||||
export class HeartbeatMonitor {
|
||||
private store: AgentStore;
|
||||
private configStore: AgentStore;
|
||||
@@ -2413,7 +2443,7 @@ export class HeartbeatMonitor {
|
||||
return (await this.store.getRunDetail(agentId, run.id))!;
|
||||
}
|
||||
|
||||
if (taskDetail.column === "done" || taskDetail.column === "archived") {
|
||||
if (await isTaskInTerminalLane(taskStore, taskDetail)) {
|
||||
if (agent.taskId === resolvedTaskId) {
|
||||
heartbeatLog.log(
|
||||
`Agent ${agentId} linked task ${resolvedTaskId} is ${taskDetail.column} — clearing assignment and running heartbeat without task context`,
|
||||
@@ -2852,7 +2882,7 @@ export class HeartbeatMonitor {
|
||||
const attemptsSoFar = priorAttempts + 1;
|
||||
const retryCapExhausted = attemptsSoFar >= MAX_HEARTBEAT_WORKTREE_ACQUISITION_RETRIES;
|
||||
|
||||
if (taskDetail.column !== "done" && taskDetail.column !== "archived") {
|
||||
if (!(await isTaskInTerminalLane(taskStore, taskDetail))) {
|
||||
if (retryCapExhausted) {
|
||||
const exhaustionMessage = `Worktree acquisition failed after ${MAX_HEARTBEAT_WORKTREE_ACQUISITION_RETRIES} heartbeat attempts for branch "${taskDetail.branch ?? `fusion/${taskDetail.id.toLowerCase()}`}": ${detail}`;
|
||||
await taskStore.updateTask(taskDetail.id, {
|
||||
|
||||
@@ -17,7 +17,6 @@
|
||||
"packages/engine/src/restart-recovery-coordinator.ts": 5,
|
||||
"packages/dashboard/app/components/TaskDetailModal.tsx": 4,
|
||||
"packages/dashboard/src/routes/register-git-github.ts": 4,
|
||||
"packages/engine/src/agent-heartbeat.ts": 4,
|
||||
"packages/engine/src/replan-target.ts": 4,
|
||||
"packages/engine/src/triage.ts": 4,
|
||||
"packages/core/src/async-mission-store-queries.ts": 3,
|
||||
@@ -154,6 +153,8 @@
|
||||
"packages/dashboard/src/reliability-metrics.ts\u0000in-progress": 1,
|
||||
"packages/dashboard/src/routes/register-task-workflow-routes.ts\u0000todo": 1,
|
||||
"packages/dashboard/src/routes/register-task-workflow-routes.ts\u0000triage": 1,
|
||||
"packages/engine/src/agent-heartbeat.ts\u0000archived": 1,
|
||||
"packages/engine/src/agent-heartbeat.ts\u0000done": 1,
|
||||
"packages/engine/src/hold-release.ts\u0000archived": 1,
|
||||
"packages/engine/src/hold-release.ts\u0000done": 1,
|
||||
"packages/engine/src/hold-release.ts\u0000in-review": 1,
|
||||
|
||||
Reference in New Issue
Block a user