From 51a5e1c2755148095b0a4a3cd398e6f6a71d3648 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 9 Aug 2026 19:03:23 -0700 Subject: [PATCH] fix(agents): separate heartbeat runtime from workflow routability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runtimeConfig.enabled answered two different questions. Every consumer in the engine reads it as "run this agent's own durable heartbeat loop" — heartbeat scheduling, error recovery, self-healing, in-process runtime — except the workflow router, which also read it as "may own a workflow stage". That conflation caused both failures: - Built-in owners ship with the heartbeat off, which is CORRECT (they are invoked by the workflow engine and must not run autonomous loops or auto-claim work). That silently made every built-in role unroutable and deadlocked the board. - Enabling the heartbeat to restore routing then gave four agents autonomous loops and auto-claiming nobody asked for. Separate the flag: - runtimeConfig.enabled governs the heartbeat runtime ONLY - isWorkflowPrincipalEligible answers routability, and treats the four built-in role owners as routable structurally — there is no fallback if a role cannot route, so "unroutable" is not a state an operator can meaningfully select - provision built-ins { enabled: false, autoClaimRelevantTasks: false } - paused/errored still outranks the exemption, so it can never resurrect a broken principal Removes the earlier write-seam coercion that forced enabled:true — the invariant is now structural rather than fought for on every write. Also re-applies the principal-hold backoff ladder (15s -> 5m, checked before graph entry) into executor/execute-workflow-graph.ts. PR #3317's executor peel rewrote executor.ts from a pre-change base and dropped it. Co-Authored-By: Claude Opus 5 --- ...ate-heartbeat-from-workflow-routability.md | 7 ++ .../builtin-workflow-role-routability.test.ts | 74 ++++++++----------- packages/core/src/agents/agent-role-policy.ts | 54 +++++++------- packages/core/src/agents/agent-store.ts | 27 ++----- packages/core/src/index.ts | 1 - .../src/executor/execute-workflow-graph.ts | 61 +++++++++++++-- 6 files changed, 127 insertions(+), 97 deletions(-) create mode 100644 .changeset/separate-heartbeat-from-workflow-routability.md diff --git a/.changeset/separate-heartbeat-from-workflow-routability.md b/.changeset/separate-heartbeat-from-workflow-routability.md new file mode 100644 index 0000000000..9b8df0a948 --- /dev/null +++ b/.changeset/separate-heartbeat-from-workflow-routability.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Built-in workflow agents no longer need heartbeats enabled to receive work. +category: fix +dev: `runtimeConfig.enabled` governs the durable heartbeat runtime only; workflow-stage routability is answered by `isWorkflowPrincipalEligible`, which treats the four built-in role owners (triage/executor/reviewer/merger) as routable structurally. Built-ins are provisioned `{ enabled: false, autoClaimRelevantTasks: false }` — no autonomous loops, no auto-claim — while every built-in role stays routable. Replaces the earlier write-seam coercion that forced `enabled: true`. Also re-applies the principal-hold backoff ladder (15s→5m, checked before graph entry) into `executor/execute-workflow-graph.ts`, where PR #3317's executor peel dropped it. diff --git a/packages/core/src/agents/__tests__/builtin-workflow-role-routability.test.ts b/packages/core/src/agents/__tests__/builtin-workflow-role-routability.test.ts index 6dcacc4dcb..d51af9b48e 100644 --- a/packages/core/src/agents/__tests__/builtin-workflow-role-routability.test.ts +++ b/packages/core/src/agents/__tests__/builtin-workflow-role-routability.test.ts @@ -11,17 +11,16 @@ reviewer, merger) with `runtimeConfig: { enabled: false }`, while the router's ` unroutable BY CONSTRUCTION — a defect every fresh instance ships with, not a local misconfiguration. Nothing recovers on its own, because the pool can only change through operator action. -Invariant under test: a built-in workflow role owner is always routable. It is coerced back to routable at the -durable write seam, so no caller — REST, dashboard toggle, plugin, provisioning, config restore — can put the -system into the deadlocked state; and the static routability predicate is SHARED with the router so -"what provisioning produces" and "what routing accepts" cannot drift apart again. +First fix attempt made it worse in a different way: enabling the heartbeat to restore routing gave four agents +autonomous loops and auto-claiming nobody wanted. The real defect is that ONE flag answered two questions. + +Invariant under test: `runtimeConfig.enabled` governs the durable heartbeat runtime ONLY, and a built-in +workflow role owner is routable regardless of it. So the built-ins keep heartbeats and auto-claim OFF — which +is what they should be, since the workflow engine invokes them — while every built-in role stays routable. The +predicate is SHARED with the router so "what provisioning produces" and "what routing accepts" cannot drift. */ import { describe, expect, it } from "vitest"; -import { - enforceBuiltinWorkflowRoleRoutability, - isBuiltinWorkflowRoleAgent, - isWorkflowPrincipalEligible, -} from "../agent-role-policy.js"; +import { isBuiltinWorkflowRoleAgent, isWorkflowPrincipalEligible } from "../agent-role-policy.js"; const builtIn = (runtimeConfig?: Record) => ({ id: "agent-builtin", @@ -36,48 +35,35 @@ const operatorOwned = (runtimeConfig?: Record) => ({ }); describe("built-in workflow role routability invariant", () => { - it("coerces a disabled built-in owner back to routable", () => { - const result = enforceBuiltinWorkflowRoleRoutability(builtIn({ enabled: false })); - expect(result.runtimeConfig).toMatchObject({ enabled: true }); - expect(isWorkflowPrincipalEligible(result)).toBe(true); + it("routes a built-in owner whose heartbeat runtime is off", () => { + // The exact production state: heartbeat disabled (correct) must NOT mean unroutable. + expect(isWorkflowPrincipalEligible(builtIn({ enabled: false }))).toBe(true); + expect(isWorkflowPrincipalEligible(builtIn({ enabled: false, autoClaimRelevantTasks: false }))).toBe(true); }); - it("preserves every other runtimeConfig key while coercing", () => { - // The operator's heartbeat cadence and claim policy are theirs; only routability is non-negotiable. - const result = enforceBuiltinWorkflowRoleRoutability( - builtIn({ enabled: false, heartbeatIntervalMs: 3_600_000, autoClaimRelevantTasks: true }), - ); - expect(result.runtimeConfig).toEqual({ - enabled: true, - heartbeatIntervalMs: 3_600_000, - autoClaimRelevantTasks: true, - }); + it("keeps the heartbeat flag meaning only 'run the autonomous loop'", () => { + // Routability must not be achievable only by switching the heartbeat on — that regression gave four + // agents autonomous loops and auto-claiming nobody asked for. + const off = builtIn({ enabled: false }); + const on = builtIn({ enabled: true }); + expect(isWorkflowPrincipalEligible(off)).toBe(isWorkflowPrincipalEligible(on)); }); - it("leaves an operator-owned agent free to be disabled", () => { - // The invariant protects the engine's own principals, not every agent — operators keep their off switch. - const result = enforceBuiltinWorkflowRoleRoutability(operatorOwned({ enabled: false })); - expect(result.runtimeConfig).toMatchObject({ enabled: false }); - expect(isWorkflowPrincipalEligible(result)).toBe(false); - expect(isBuiltinWorkflowRoleAgent(result)).toBe(false); - }); - - it("is a no-op for an already-routable built-in owner (same reference, no churn)", () => { - const agent = builtIn({ enabled: true }); - expect(enforceBuiltinWorkflowRoleRoutability(agent)).toBe(agent); - const unset = builtIn(); - expect(enforceBuiltinWorkflowRoleRoutability(unset)).toBe(unset); + it("leaves an operator-owned agent unroutable when disabled", () => { + // The structural exemption is scoped to the engine's own principals; operators keep their off switch. + expect(isWorkflowPrincipalEligible(operatorOwned({ enabled: false }))).toBe(false); + expect(isWorkflowPrincipalEligible(operatorOwned({ enabled: true }))).toBe(true); + expect(isBuiltinWorkflowRoleAgent(operatorOwned())).toBe(false); }); /* - The predicate the router consults. `enabled === false` was the exact bit that made the pool look exhausted; - paused/errored agents must stay excluded so the coercion never resurrects a genuinely broken principal. + Paused/errored is a genuine unusability signal, so it outranks the built-in exemption — otherwise the + exemption would resurrect a broken principal and route work into it. */ - it("still excludes paused and errored agents from principal routing", () => { - expect(isWorkflowPrincipalEligible({ runtimeConfig: { enabled: true }, state: "paused" })).toBe(false); - expect(isWorkflowPrincipalEligible({ runtimeConfig: { enabled: true }, state: "error" })).toBe(false); - expect(isWorkflowPrincipalEligible({ runtimeConfig: { enabled: true }, state: "active" })).toBe(true); - // An unset runtimeConfig is routable: only an explicit `false` opts an agent out. - expect(isWorkflowPrincipalEligible({ state: "active" })).toBe(true); + it("still excludes paused and errored agents, including built-ins", () => { + expect(isWorkflowPrincipalEligible({ ...builtIn({ enabled: false }), state: "paused" })).toBe(false); + expect(isWorkflowPrincipalEligible({ ...builtIn({ enabled: false }), state: "error" })).toBe(false); + expect(isWorkflowPrincipalEligible({ ...builtIn({ enabled: false }), state: "active" })).toBe(true); + expect(isWorkflowPrincipalEligible({ ...operatorOwned(), state: "active" })).toBe(true); }); }); diff --git a/packages/core/src/agents/agent-role-policy.ts b/packages/core/src/agents/agent-role-policy.ts index ff4a97d460..f92ca8d1ea 100644 --- a/packages/core/src/agents/agent-role-policy.ts +++ b/packages/core/src/agents/agent-role-policy.ts @@ -96,35 +96,37 @@ export function isBuiltinWorkflowRoleAgent(agent: { metadata?: Record | null; - runtimeConfig?: Record | null; -}>(agent: T): T { - if (!isBuiltinWorkflowRoleAgent(agent)) return agent; - if (agent.runtimeConfig?.enabled !== false) return agent; - return { ...agent, runtimeConfig: { ...agent.runtimeConfig, enabled: true } }; -} +/* +FNXC:WorkflowAgentRouting 2026-08-10-01:15: +`runtimeConfig.enabled` means ONE thing: run this agent's own durable heartbeat loop. Every consumer in the +engine reads it that way — heartbeat scheduling, error recovery, self-healing, the in-process runtime — except +the workflow router, which also treated it as "may own a workflow stage". Those are different questions, and +conflating them is what produced BOTH failures here: + + - Built-in owners ship with the heartbeat off (correct — they are invoked BY the workflow engine and must not + run autonomous loops or auto-claim work), and that silently made every built-in role unroutable, deadlocking + the board. + - Turning the heartbeat on to restore routing then gave four agents autonomous loops nobody asked for. + +So the flag is separated: `enabled` governs the heartbeat runtime ONLY, and workflow routability is answered by +{@link isWorkflowPrincipalEligible}. For the four built-in owners routability is STRUCTURAL — they are the +engine's own principals for triage/executor/reviewer/merger, there is no fallback if a role cannot route, and +"unroutable" is not a state an operator can meaningfully select. To take one out of rotation, add your own +agent with that role and route to it; that leaves the role routable, which is the property this protects. +*/ export function isWorkflowPrincipalEligible( - agent: Pick & { state?: string; id?: string }, + agent: Pick & { + state?: string; + id?: string; + metadata?: Record | null; + }, ): boolean { - if (agent.runtimeConfig?.enabled === false) return false; - return agent.state !== "paused" && agent.state !== "error"; + // A paused or errored agent is genuinely unusable — that applies to built-ins too, so it is checked first. + if (agent.state === "paused" || agent.state === "error") return false; + // Built-in owners route regardless of their heartbeat setting; see the note above. + if (isBuiltinWorkflowRoleAgent(agent)) return true; + return agent.runtimeConfig?.enabled !== false; } export function isImplementationTask(task: Pick): boolean { diff --git a/packages/core/src/agents/agent-store.ts b/packages/core/src/agents/agent-store.ts index 04eda27402..855a712b12 100644 --- a/packages/core/src/agents/agent-store.ts +++ b/packages/core/src/agents/agent-store.ts @@ -110,7 +110,6 @@ import { BUILTIN_WORKFLOW_ROLE_AGENT_DEFAULT_LIST, type BuiltinWorkflowRole, } from "./workflow-role-agent-defaults.js"; -import { enforceBuiltinWorkflowRoleRoutability } from "./agent-role-policy.js"; const agentStoreLog = createLogger("agent-store"); @@ -2061,14 +2060,12 @@ export class AgentStore extends EventEmitter { metadata: { builtInWorkflowRole: true, workflowRole: definition.role }, /* FNXC:WorkflowAgentRouting 2026-08-10-01:15: - These four ARE the permanent principals that route built-in workflow stages, and the router treats - `runtimeConfig.enabled === false` as unavailable. Provisioning them disabled therefore created a - system that could not route ANY built-in role: every task held at its first workflow node with - `workflow-principal-role-pool-exhausted:`, and the held item was re-claimed with no backoff, - burning CPU and ~19k audit rows/hour while nothing executed. Seed them ENABLED so a fresh instance - can route out of the box; an operator can still disable one deliberately afterwards. + Heartbeat OFF and auto-claim OFF is correct for these four: they are invoked BY the workflow engine + as stage principals and must not run autonomous loops or claim work on their own. This no longer + costs routability — `isWorkflowPrincipalEligible` treats built-in owners as routable structurally, + precisely so the heartbeat setting and the routing question stay separate concerns. */ - runtimeConfig: { enabled: true }, + runtimeConfig: { enabled: false, autoClaimRelevantTasks: false }, instructionsText: definition.instructionsText, soul: definition.soul, bundleConfig: { ...BUILTIN_WORKFLOW_AGENT_BUNDLE_CONFIG, files: [...BUILTIN_WORKFLOW_AGENT_BUNDLE_CONFIG.files] }, @@ -2080,11 +2077,6 @@ export class AgentStore extends EventEmitter { || (agent.bundleConfig !== undefined && !canonicalOrPartialBundle) || (Boolean(agent.instructionsText?.trim()) && agent.instructionsText !== definition.instructionsText); const updates: Partial = {}; - // FNXC:WorkflowAgentRouting 2026-08-10-01:15: converge every existing built-in owner to routable. - // See enforceBuiltinWorkflowRoleRoutability — routability is an invariant for these four, not a setting. - if (agent.runtimeConfig?.enabled === false) { - updates.runtimeConfig = { ...agent.runtimeConfig, enabled: true }; - } if (!customInstructions && !agent.instructionsText?.trim()) updates.instructionsText = definition.instructionsText; if (!agent.soul?.trim()) updates.soul = definition.soul; // FNXC:WorkflowAgentIdentities 2026-08-08-06:38: Do not rewrite complete canonical @@ -3198,16 +3190,9 @@ export class AgentStore extends EventEmitter { } private async writeAgent(agent: Agent, executor?: QueryHandle): Promise { - /* - FNXC:WorkflowAgentRouting 2026-08-10-01:15: - The single durable write seam for agents, and therefore the only place the built-in-owner routability - invariant cannot be bypassed. Enforcing here — rather than in the REST handler — covers the API, the - dashboard toggle, plugins, provisioning, config-revision restores, and any future caller at once. - */ - const routable = enforceBuiltinWorkflowRoleRoutability(agent); // FNXC:SqliteFinalRemoval 2026-06-25-23:40: // Backend mode: delegate to async Drizzle writeAgent helper. - await writeAgentAsync(executor ?? this.asyncLayer!.db, routable, this.asyncLayer!.projectId); + await writeAgentAsync(executor ?? this.asyncLayer!.db, agent, this.asyncLayer!.projectId); return; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0c8098727c..15bb940258 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -798,7 +798,6 @@ export { canAgentReceiveImplementationTasks, isWorkflowPrincipalEligible, isBuiltinWorkflowRoleAgent, - enforceBuiltinWorkflowRoleRoutability, evaluateImplementationTaskBind, assertImplementationTaskBindAllowed, AgentTaskRoutingPolicyError, diff --git a/packages/engine/src/executor/execute-workflow-graph.ts b/packages/engine/src/executor/execute-workflow-graph.ts index dd45858637..3a879912d2 100644 --- a/packages/engine/src/executor/execute-workflow-graph.ts +++ b/packages/engine/src/executor/execute-workflow-graph.ts @@ -103,11 +103,43 @@ export type ExecuteWorkflowGraphDeps = { terminateAllChildren: AnyFn; }; +/* +FNXC:WorkflowAgentRouting 2026-08-10-01:15: +Backoff ladder for a workflow-principal hold. A hold had NO cooldown, so the scheduler re-dispatched instantly +and the run re-entered only to re-fence and re-park: observed at ~3.5 re-dispatches/second across every task, +pinning a core and writing ~19k `workflowWorkItem` audit rows/hour while nothing executed. The hold never +increments the work item's `attempt`, so no retry budget is consumed and no existing guard can ever fire. + +Module-scoped and in-memory on purpose, matching the session-contention hold: it needs no schema change, and a +restart clearing it is CORRECT — a restart is exactly when agent configuration may have changed. The ceiling is +generous because an unroutable role clears on OPERATOR action (enable or add an agent), never on its own, so +polling it every few seconds only burns CPU. +*/ +const PRINCIPAL_HOLD_BACKOFF_MS = process.env.VITEST || process.env.NODE_ENV === "test" ? 0 : 15_000; +const PRINCIPAL_HOLD_MAX_BACKOFF_MS = 300_000; +const principalHoldBackoff = new Map(); + +/** Clears the ladder for a task; exported so tests and recovery paths can reset it deterministically. */ +export function clearPrincipalHoldBackoff(taskId: string): void { + principalHoldBackoff.delete(taskId); +} + export async function executeWorkflowGraph( deps: ExecuteWorkflowGraphDeps, task: Task, opts?: { alreadyClaimed?: boolean }, ): Promise { + /* + FNXC:WorkflowAgentRouting 2026-08-10-01:15: + Honor an active principal-hold cooldown BEFORE the graph is entered — re-entering only to re-fence and + re-park is the hot loop itself, and it costs a graph run plus two work-item writes and two audit rows per + pass for a condition that cannot change without operator action. + */ + const cooling = principalHoldBackoff.get(task.id); + if (cooling && Date.now() < cooling.until && !opts?.alreadyClaimed) { + executorLog.debug(`[workflow-graph] ${task.id} deferred — principal hold cooling down (${cooling.reason})`); + return; + } // Claim synchronously before any await so concurrent execute() calls for // the same task cannot both enter graph routing (mirrors executingTaskLock). // executeCore may already have claimed before its pre-graph awaits (FN-8471). @@ -546,13 +578,29 @@ export async function executeWorkflowGraph( * never-clears composition fault (missing agent-store / IR). */ const neverClears = principalHoldReason.startsWith("workflow-principal-routing-unavailable:"); + /* + * FNXC:WorkflowAgentRouting 2026-08-10-01:15: + * Record the backoff keyed on the hold REASON. A changed reason resets the ladder (genuinely new + * information); repeats extend it. The first occurrence of a reason still logs immediately so the + * hold stays greppable, while repeats stay silent so neither the engine log nor the task log floods. + */ + const priorHold = principalHoldBackoff.get(task.id); + const repeated = priorHold?.reason === principalHoldReason; + const attempt = repeated ? priorHold!.attempt + 1 : 1; + principalHoldBackoff.set(task.id, { + reason: principalHoldReason, + attempt, + until: Date.now() + Math.min(PRINCIPAL_HOLD_MAX_BACKOFF_MS, PRINCIPAL_HOLD_BACKOFF_MS * 2 ** (attempt - 1)), + }); const holdMessage = `[workflow-graph] ${task.id} held at graph node — ${principalHoldReason}`; - if (neverClears) { - executorLog.error(`${holdMessage} (workflow principal routing is unavailable; this hold cannot self-clear)`); - } else { - executorLog.warn(holdMessage); + if (!repeated) { + if (neverClears) { + executorLog.error(`${holdMessage} (workflow principal routing is unavailable; this hold cannot self-clear)`); + } else { + executorLog.warn(holdMessage); + } + await deps.store.logEntry(task.id, `Workflow stage held — ${principalHoldReason}`).catch(() => undefined); } - await deps.store.logEntry(task.id, `Workflow stage held — ${principalHoldReason}`).catch(() => undefined); if ( continuation && typeof deps.store.transitionWorkflowWorkItem === "function" @@ -567,6 +615,9 @@ export async function executeWorkflowGraph( } return; } + // FNXC:WorkflowAgentRouting 2026-08-10-01:15: this run cleared the principal fence, so any prior hold is + // resolved — drop the ladder so a later hold starts from the short delay rather than a stale long one. + clearPrincipalHoldBackoff(task.id); /* Direct graph node fences are terminalized only after the interpreter * returns, preserving their historical principal through all handler and * tool-gate calls while ensuring completed work cannot render as active.