diff --git a/.changeset/wait-timer-job-released-with-the-pause.md b/.changeset/wait-timer-job-released-with-the-pause.md new file mode 100644 index 0000000000..69903051ce --- /dev/null +++ b/.changeset/wait-timer-job-released-with-the-pause.md @@ -0,0 +1,47 @@ +--- +"@objectstack/service-automation": minor +--- + +fix(automation): a `wait` timer's wake-up job is dropped when the run leaves the node, not only when the timer fires (#5512) + +A timer `wait` arms a one-shot job on entry (`flow-wait::`, +`{ type: 'once', at }`) and, until now, only that job's own callback ever tore it +down. Every other way out of the pause left it armed: + +- resumed early through the REST resume endpoint (`POST + /api/v1/automation/:name/runs/:runId/resume` — a door the #3801 resume gate + deliberately leaves open for `screen`/`wait` pauses) or the SDK equivalent; +- cancelled while parked (`cancelRun`, ADR-0044); +- terminally failed under a subflow ancestor. + +Reported from 17.0-rc2 acceptance: a `wait P1D` pause resumed early ran to +completion while its one-shot stayed `active: true` in `sys_job` with tomorrow's +deadline. For the next 24h anyone reading `sys_job` saw "a run is still waiting +to be woken" — the row contradicted the run — and when the deadline arrived the +job fired a resume at a run that had completed the day before (harmless: the +engine reports a machine-state error and the callback discards it, then the job +self-cancels). A long-running org accumulated one stale row per early wake-up. + +**What changed.** The engine now tells the node its pause is over. `NodeExecutor` +gains an optional `onSuspensionReleased(release)` — the mirror of `suspend: true` +— called from the single choke point every consumption of a suspension already +passes through, with the `runId`, the node, the `correlation` the node minted at +suspend time, and why the pause ended (`resumed` / `cancelled` / `failed`). The +`wait` node implements it by cancelling the one-shot whose name it recognises as +its own, so the `sys_job` row goes inactive the moment the run leaves the node, +whichever route it left by. `SuspensionRelease` / `SuspensionReleaseReason` are +exported for plugin nodes that arm something on entry (a lease, a reminder, a +timeout) and need the same teardown. + +Teardown is best-effort and runs after the suspension is consumed: a job service +that is down or throwing can neither delay nor fail the continuation — the engine +logs one warning naming the correlation an operator would cancel by hand. Node +types that arm nothing are unaffected (the hook is optional), and a pause that +armed no job — a signal wait, or a timer with no parseable duration — cancels +nothing, since its correlation is not a job name. Deprecated ADR-0018 node +aliases delegate the hook to their canonical executor, so authoring the old type +name cannot silently lose the teardown. + +The timer callback keeps its own `finally` cancel: the two answer different +questions — "the run left the node" versus "this one-shot has had its single +shot", including shots that did not consume a pause. `cancel` is idempotent. diff --git a/packages/services/service-automation/src/builtin/wait-node.test.ts b/packages/services/service-automation/src/builtin/wait-node.test.ts index 8ce7fcce1b..aba4180f90 100644 --- a/packages/services/service-automation/src/builtin/wait-node.test.ts +++ b/packages/services/service-automation/src/builtin/wait-node.test.ts @@ -140,6 +140,143 @@ describe('wait node executor', () => { }); }); +/** + * #5512 — the one-shot wake-up job is dropped when the run leaves the wait node, + * whichever route it leaves by. + * + * The reported symptom: a `wait P1D` pause resumed early through the REST resume + * endpoint (a door the #3801 gate deliberately leaves open for `wait`) ran to + * completion while its `flow-wait::` one-shot stayed `active` in + * `sys_job` with tomorrow's deadline — for 24h it read as "a run is still waiting + * to be woken", and then fired a ghost `resume` at a run that had completed the + * day before. Only the timer's OWN callback dropped its job. + * + * `cancelled` here is the fake job service's log of `IJobService.cancel(name)` — + * the call the DbJobAdapter turns into `active: false` on the `sys_job` row. + */ +describe('wait timer teardown when the pause ends another way (#5512)', () => { + let engine: AutomationEngine; + let ran: string[]; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + ran = []; + engine.registerNodeExecutor(markerExecutor(ran)); + }); + + it('cancels the one-shot when an external resume cuts a timer wait short', async () => { + const { ctx, scheduled, cancelled } = fakeJobCtx(); + registerWaitNode(engine, ctx); + engine.registerFlow('wait_flow', waitFlow({ eventType: 'timer', timerDuration: 'P1D' })); + + const paused = await engine.execute('wait_flow'); + expect(paused.status).toBe('paused'); + expect(scheduled).toHaveLength(1); // armed for +24h + expect(cancelled).toEqual([]); + + // The REST resume door: no signal, no job involvement — exactly the repro. + const resumed = await engine.resume(paused.runId!); + expect(resumed.success).toBe(true); + expect(ran).toEqual(['after']); // the run completed + expect(engine.listSuspendedRuns()).toEqual([]); + + // …and tomorrow's wake-up is gone with it, instead of lingering `active`. + expect(cancelled).toEqual([scheduled[0].name]); + expect(scheduled[0].name).toBe(`flow-wait:${paused.runId}:pause`); + }); + + it('cancels the one-shot when the parked run is cancelled (ADR-0044)', async () => { + const { ctx, scheduled, cancelled } = fakeJobCtx(); + registerWaitNode(engine, ctx); + engine.registerFlow('wait_flow', waitFlow({ eventType: 'timer', timerDuration: 'P1D' })); + + const paused = await engine.execute('wait_flow'); + expect(await engine.cancelRun(paused.runId!, 'window abandoned')).toBe(true); + + expect(cancelled).toEqual([scheduled[0].name]); + expect(ran).toEqual([]); // cancelled, not continued + }); + + it('cancels the re-armed one-shot too (cold boot, then an external resume)', async () => { + const store = new InMemorySuspendedRunStore(); + const config = { eventType: 'timer', timerDuration: 'P1D' }; + + // Process 1: suspend at the wait, then "die". + const boot1 = fakeJobCtx(); + const e1 = new AutomationEngine(silentLogger()); + e1.registerNodeExecutor(markerExecutor([])); + registerWaitNode(e1, boot1.ctx); + e1.setSuspendedRunStore(store); + e1.registerFlow('wait_flow', waitFlow(config)); + const paused = await e1.execute('wait_flow'); + + // Process 2: cold boot + re-arm, then someone resumes the run by hand. + const boot2 = fakeJobCtx(); + registerWaitNode(engine, boot2.ctx); + engine.setSuspendedRunStore(store); + engine.registerFlow('wait_flow', waitFlow(config)); + const job = boot2.ctx.getService('job') as IJobService; + expect(await rearmSuspendedWaitTimers(engine, store, job, silentLogger())).toBe(1); + expect(boot2.scheduled).toHaveLength(1); + + const resumed = await engine.resume(paused.runId!); + expect(resumed.success).toBe(true); + expect(ran).toEqual(['after']); + // The re-armed job carries the same name, so the same teardown reaches it. + expect(boot2.cancelled).toEqual([`flow-wait:${paused.runId}:pause`]); + }); + + it('cancels nothing for a signal wait — it armed no job to cancel', async () => { + const { ctx, scheduled, cancelled } = fakeJobCtx(); + registerWaitNode(engine, ctx); + engine.registerFlow('wait_flow', waitFlow({ eventType: 'signal', signalName: 'contract.renewed' })); + + const paused = await engine.execute('wait_flow'); + expect(scheduled).toEqual([]); + const resumed = await engine.resume(paused.runId!); + + expect(resumed.success).toBe(true); + expect(ran).toEqual(['after']); + // The correlation of a signal wait is the AUTHOR's signal name, not a job + // name — the teardown must not hand it to `cancel()`. + expect(cancelled).toEqual([]); + }); + + it('cancels nothing for a timer wait that armed no job (no parseable duration)', async () => { + const { ctx, scheduled, cancelled } = fakeJobCtx(); + registerWaitNode(engine, ctx); + // No `timerDuration` ⇒ no deadline ⇒ nothing scheduled; the pause carries + // the degraded `timer:` correlation instead of a job name. + engine.registerFlow('wait_flow', waitFlow({ eventType: 'timer' })); + + const paused = await engine.execute('wait_flow'); + expect(scheduled).toEqual([]); + expect(engine.listSuspendedRuns()[0]).toMatchObject({ correlation: 'timer:pause' }); + + const resumed = await engine.resume(paused.runId!); + expect(resumed.success).toBe(true); + expect(cancelled).toEqual([]); + }); + + it('still cancels exactly once when the timer itself fires (idempotent teardown)', async () => { + const { ctx, scheduled, cancelled } = fakeJobCtx(); + registerWaitNode(engine, ctx); + engine.registerFlow('wait_flow', waitFlow({ eventType: 'timer', timerDuration: 'PT2H' })); + + const paused = await engine.execute('wait_flow'); + await scheduled[0].handler({ jobId: scheduled[0].name }); + + expect(ran).toEqual(['after']); + // Two teardowns now cover this path — the release hook (the run left the + // node) and the one-shot's own `finally` (the job had its single shot) — and + // they target the same name. `cancel` is idempotent, so what is pinned is + // "cancelled, and nothing else cancelled"; the call COUNT is deliberately + // not pinned, since which of the two fires is not a behavioural promise. + expect(cancelled.length).toBeGreaterThan(0); + expect([...new Set(cancelled)]).toEqual([`flow-wait:${paused.runId}:pause`]); + }); +}); + /** * The loose `config.*` back door the executor used to read alongside * `waitEventConfig` graduated into the ADR-0087 D2 conversion layer diff --git a/packages/services/service-automation/src/builtin/wait-node.ts b/packages/services/service-automation/src/builtin/wait-node.ts index 70b69a4263..40f3c425f9 100644 --- a/packages/services/service-automation/src/builtin/wait-node.ts +++ b/packages/services/service-automation/src/builtin/wait-node.ts @@ -5,6 +5,17 @@ import { defineActionDescriptor } from '@objectstack/spec/automation'; import type { IJobService } from '@objectstack/spec/contracts'; import type { AutomationEngine, SuspendedRunStore } from '../engine.js'; +/** + * The one-shot wake-up job's name for a timer `wait` pause — and, by + * construction, the `correlation` that pause suspends with. One declaration, so + * the three sites that must agree on it cannot drift: the arming path, the + * cold-boot re-arm ({@link rearmSuspendedWaitTimers}), and the teardown when the + * run leaves the node (#5512). + */ +function waitTimerJobName(runId: string, nodeId: string): string { + return `flow-wait:${runId}:${nodeId}`; +} + /** * `wait` built-in node — a durable pause (ADR-0019 suspend/resume), the timer / * signal sibling of the human-input `screen` and `approval` nodes. @@ -22,6 +33,10 @@ import type { AutomationEngine, SuspendedRunStore } from '../engine.js'; * the correlation key; an external producer resumes the run when the event * arrives (`resume(runId)`), exactly like a decision-less approval. * + * Whatever wakes the run, the one-shot job is dropped when the pause ends — see + * `onSuspensionReleased` below (#5512). A timer wait cut short by an external + * `resume` used to leave its wake-up armed for the full duration. + * * Reads its own run id from the `$runId` variable the engine injects at start * (same mechanism the approval node uses to map external state back to the run). */ @@ -79,13 +94,19 @@ export function registerWaitNode(engine: AutomationEngine, ctx: PluginContext): const job = getJobService(); if (job && runId != null && at) { - const jobName = `flow-wait:${String(runId)}:${node.id}`; + const jobName = waitTimerJobName(String(runId), node.id); try { await job.schedule(jobName, { type: 'once', at }, async () => { try { await engine.resume(String(runId)); } finally { - // One-shot: drop the job so it never re-fires. + // One-shot: drop the job so it never re-fires. Kept alongside + // the `onSuspensionReleased` teardown below because the two + // answer different questions: that one fires when the RUN + // leaves the node, this one when the JOB has had its single + // shot — including the shots that did not consume a pause (the + // store was unreachable, another resume was already in + // flight). Both are `cancel`, which is idempotent. try { await job.cancel?.(jobName); } catch { @@ -116,6 +137,34 @@ export function registerWaitNode(engine: AutomationEngine, ctx: PluginContext): const signal = String(wec.signalName ?? `wait:${node.id}`); return { success: true, suspend: true, correlation: signal }; }, + + /** + * Disarm the one-shot wake-up when the run leaves this node by ANY route + * (#5512). Until this existed only the timer's own callback dropped its job, + * so a wait cut short — an external `resume` through the REST door (which + * the #3801 gate deliberately allows for `wait`), a `cancelRun`, a subflow + * ancestor failing — left the one-shot armed: it stayed `active` in + * `sys_job` with tomorrow's `schedule_expression`, read to every operator + * and test as "a run is still waiting to be woken", and eventually fired a + * ghost `resume` at a run that had completed the day before. + * + * The pause is already consumed when this runs, so cancelling cannot strand + * the run; and `cancel` on a name the job service no longer holds is a + * no-op, so a race with the timer's own teardown is harmless. + */ + async onSuspensionReleased({ runId, nodeId, correlation }) { + // Only a pause that actually armed a job carries its name as the + // correlation. The degraded timer (`timer:`) and every signal wait + // (the author's own signal name) armed nothing, so there is nothing to + // cancel — and reconstructing the name we mint, rather than prefix-testing + // a string we may not own, keeps this from ever cancelling by coincidence. + if (correlation !== waitTimerJobName(runId, nodeId)) return; + const job = getJobService(); + if (!job?.cancel) return; + // Errors propagate: the engine catches them and logs one line naming this + // correlation — which is the job name an operator would cancel by hand. + await job.cancel(correlation); + }, }); ctx.logger.info('[Wait Node] 1 built-in node executor registered'); @@ -217,7 +266,7 @@ export async function rearmSuspendedWaitTimers( continue; } - const jobName = `flow-wait:${run.runId}:${run.nodeId}`; + const jobName = waitTimerJobName(run.runId, run.nodeId); try { await job.schedule(jobName, { type: 'once', at: wakeAt }, async () => { try { diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 039bcf1179..546de97857 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -209,6 +209,57 @@ export interface NodeExecutor { variables: Map, context: AutomationContext, ): Promise; + + /** + * The mirror of `suspend: true` — called once a suspension THIS node type + * created is consumed, whichever path consumed it (#5512). + * + * A pausing executor usually arms something external on entry (a one-shot + * wake-up job, a reminder, a lease). Only its own wake path used to tear + * that down, so a pause ended by *anything else* — an external + * `resume(runId)` through the REST door, a {@link AutomationEngine.cancelRun}, + * a subflow ancestor failing — left the armature live: #5512 was a + * `flow-wait` one-shot still `active` in `sys_job` a day after its run had + * completed, pointed at a run that no longer existed. + * + * Called by {@link AutomationEngine.forgetSuspendedRun}, the single choke + * point every consumption goes through, so an executor that implements this + * disarms on **all** of them and never needs to know which one fired. It + * runs after the suspension is gone from the cache and the durable store: + * teardown is best-effort observability work and must not hold up (or fail) + * the continuation — the engine catches and logs whatever it throws. + * + * @param release - Which suspension ended, and how. `correlation` is the + * handle the executor itself returned at suspend time. + */ + onSuspensionReleased?(release: SuspensionRelease): Promise | void; +} + +/** How a suspension ended — see {@link SuspensionRelease}. */ +export type SuspensionReleaseReason = + /** Continued past the paused node (timer fired, signal arrived, screen submitted, …). */ + | 'resumed' + /** Terminally failed while paused (e.g. a subflow descendant failed under it). */ + | 'failed' + /** Terminally cancelled while paused ({@link AutomationEngine.cancelRun}, ADR-0044). */ + | 'cancelled'; + +/** + * A consumed suspension, handed to {@link NodeExecutor.onSuspensionReleased} so + * the node that armed a pause can tear down what it armed (#5512). + */ +export interface SuspensionRelease { + runId: string; + flowName: string; + /** The node the run was paused at — the one whose executor is notified. */ + nodeId: string; + /** + * The correlation key the node returned with `suspend: true` (its own handle + * on the pause — e.g. the `wait` node's one-shot job name). Absent when the + * node suspended without one. + */ + correlation?: string; + reason: SuspensionReleaseReason; } /** @@ -1182,18 +1233,75 @@ export class AutomationEngine implements IAutomationService { * Drop a suspended run from the in-memory cache and (best-effort) the * durable store. Called once the run is claimed for resume or reaches a * terminal state. + * + * This is the ONE choke point through which every consumption of a + * suspension passes (resume, terminal failure, cancel), which is why it — + * and not any individual caller — notifies the paused node's executor that + * its pause is over ({@link NodeExecutor.onSuspensionReleased}, #5512). It + * therefore takes the whole {@link SuspendedRun}: the notification needs the + * node and the correlation the executor minted, not just the id. */ - private async forgetSuspendedRun(runId: string): Promise { - this.suspendedRuns.delete(runId); + private async forgetSuspendedRun(run: SuspendedRun, reason: SuspensionReleaseReason): Promise { + this.suspendedRuns.delete(run.runId); if (this.store) { try { - await this.store.delete(runId); + await this.store.delete(run.runId); } catch (err) { this.logger.warn( - `[automation] failed to delete suspended run '${runId}' from durable store: ${(err as Error).message}`, + `[automation] failed to delete suspended run '${run.runId}' from durable store: ${(err as Error).message}`, ); } } + await this.releaseSuspension(run, reason); + } + + /** + * Tell the executor of the node a run was paused at that the pause is over, + * so it can disarm whatever it armed on entry (#5512). + * + * Runs AFTER the suspension is gone from cache and store: the run has + * definitively left the node, so a slow or broken job service can neither + * delay the continuation nor resurrect the pause. Failures are logged and + * swallowed for the same reason — a wake-up that outlives its run is a + * misleading `sys_job` row, not a broken run, and the log names the handle + * an operator needs to clean it up by hand. + * + * Routed by the node's own registry type (recorded on the suspension, + * falling back to the live flow for rows persisted before `nodeType` + * existed) rather than broadcast to every listener — the executor that + * created the pause is the one that owns tearing it down, and nothing else + * has to pattern-match correlation strings it does not own. + */ + private async releaseSuspension(run: SuspendedRun, reason: SuspensionReleaseReason): Promise { + const nodeType = this.resolveSuspendedNodeType(run); + const executor = nodeType ? this.nodeExecutors.get(nodeType) : undefined; + if (!executor?.onSuspensionReleased) return; + try { + await executor.onSuspensionReleased({ + runId: run.runId, + flowName: run.flowName, + nodeId: run.nodeId, + correlation: run.correlation, + reason, + }); + } catch (err) { + this.logger.warn( + `[automation] run '${run.runId}': '${nodeType}' node '${run.nodeId}' failed to release its suspension ` + + `(reason: ${reason}, correlation: ${run.correlation ?? 'none'}): ${(err as Error)?.message ?? err} — ` + + `the run continued; whatever the node armed on entry may still be scheduled`, + ); + } + } + + /** + * The registry type of the node a run is paused at: what the suspension + * recorded at pause time, falling back to the live flow definition for rows + * persisted before `nodeType` existed. Recorded-first on purpose — a flow + * republished mid-pause must not re-type the node out from under a run + * (see {@link SuspendedRun.nodeType}). + */ + private resolveSuspendedNodeType(run: SuspendedRun): string | undefined { + return run.nodeType ?? this.flows.get(run.flowName)?.nodes.find(n => n.id === run.nodeId)?.type; } // ── Plugin Extension API ────────────────────────────── @@ -1276,6 +1384,16 @@ export class AutomationEngine implements IAutomationService { } return target.execute(node, variables, context); }, + // Delegated for the same reason `resolveResumeAuthority` walks the + // alias to its canonical: a pause created through the old type name + // must not lose a capability the canonical declares. Without this, + // aliasing a pausing type would silently stop it disarming what it + // armed on entry (#5512) — the alias's own executor implements + // nothing. No alias of a pausing type exists today; this keeps it + // from becoming a hole the day one does. + async onSuspensionReleased(release) { + await engine.nodeExecutors.get(canonicalType)?.onSuspensionReleased?.(release); + }, }); this.logger.info(`Node alias registered: ${alias} → ${canonicalType} (deprecated)`); } @@ -2535,7 +2653,7 @@ export class AutomationEngine implements IAutomationService { const run = await this.resolveEffectiveSuspension(runId); if (!run) return null; - const nodeType = run.nodeType ?? this.flows.get(run.flowName)?.nodes.find(n => n.id === run.nodeId)?.type; + const nodeType = this.resolveSuspendedNodeType(run); if (!nodeType) return null; if (this.resolveResumeAuthority(nodeType) !== 'service') return null; // The owning service stamped the signal — this resume IS the recorded @@ -2859,7 +2977,9 @@ export class AutomationEngine implements IAutomationService { // resumes exactly once per pause, and a duplicate resume after a // partial restart must not double-run side effects. (Folding the // signal above is pure in-memory work, not downstream work.) - await this.forgetSuspendedRun(runId); + // This is also where the paused node learns its pause is over and + // disarms what it armed on entry (#5512) — see forgetSuspendedRun. + await this.forgetSuspendedRun(run, 'resumed'); const steps = run.steps; const context = run.context; @@ -3138,7 +3258,7 @@ export class AutomationEngine implements IAutomationService { * descendant fails — the ancestor awaiting it can never be resumed. */ private async failSuspendedRun(run: SuspendedRun, error: string): Promise { - await this.forgetSuspendedRun(run.runId); + await this.forgetSuspendedRun(run, 'failed'); this.recordLog({ id: run.runId, flowName: run.flowName, @@ -3178,7 +3298,7 @@ export class AutomationEngine implements IAutomationService { } } if (!run) return false; - await this.forgetSuspendedRun(runId); + await this.forgetSuspendedRun(run, 'cancelled'); this.recordLog({ id: run.runId, flowName: run.flowName, diff --git a/packages/services/service-automation/src/index.ts b/packages/services/service-automation/src/index.ts index d0b997c77b..7bebb0d3f6 100644 --- a/packages/services/service-automation/src/index.ts +++ b/packages/services/service-automation/src/index.ts @@ -7,6 +7,10 @@ export type { RunSummaryLogLevel, NodeExecutor, NodeExecutionResult, + // The teardown half of a pause (#5512): a plugin node that arms something on + // entry needs these to name the callback that disarms it. + SuspensionRelease, + SuspensionReleaseReason, FlowTrigger, FlowTriggerBinding, ConnectorActionHandler, diff --git a/packages/services/service-automation/src/suspension-release.test.ts b/packages/services/service-automation/src/suspension-release.test.ts new file mode 100644 index 0000000000..150e36a6a0 --- /dev/null +++ b/packages/services/service-automation/src/suspension-release.test.ts @@ -0,0 +1,292 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from './engine.js'; +import type { NodeExecutor, SuspensionRelease } from './engine.js'; +import { InMemorySuspendedRunStore } from './suspended-run-store.js'; +import { registerSubflowNode } from './builtin/subflow-node.js'; + +/** + * `NodeExecutor.onSuspensionReleased` — the engine side of #5512. + * + * A pausing node arms something external on entry (the `wait` node's one-shot + * wake-up job; a lease, a reminder, a timeout in a plugin's node). Before this + * hook the only route that tore that armature down was the node's OWN wake path, + * so a pause ended any other way left it live — which is how a `flow-wait` + * one-shot stayed `active` in `sys_job` for a day after its run completed. + * + * These tests pin the CONTRACT rather than the wait node's use of it: which + * executor is notified, with what, on which routes, and what happens when the + * teardown itself fails. + */ + +function silentLogger(warns: string[] = []) { + return { + info() {}, + warn(msg: string) { warns.push(String(msg)); }, + error() {}, + debug() {}, + child() { return silentLogger(warns); }, + } as any; +} + +function ctx() { + return { logger: silentLogger(), getService() { throw new Error('none'); } } as any; +} + +/** A pausing executor that records every release it is told about. */ +function recordingPauser(type: string, seen: SuspensionRelease[]): NodeExecutor { + return { + type, + async execute(node) { + // A correlation stands in for "the handle on whatever I armed on entry". + return { success: true, suspend: true, correlation: `test-armature:${node.id}` }; + }, + onSuspensionReleased(release) { + seen.push(release); + }, + }; +} + +/** Records the order downstream nodes ran, to prove the continuation survived. */ +function markerExecutor(ran: string[]): NodeExecutor { + return { type: 'mark', async execute(node) { ran.push(node.id); return { success: true }; } }; +} + +const pauseFlow = (pauseType: string) => ({ + name: 'pause_flow', + label: 'Pause Flow', + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'hold', type: pauseType, label: 'Hold' }, + { id: 'after', type: 'mark', label: 'After' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'hold' }, + { id: 'e2', source: 'hold', target: 'after' }, + { id: 'e3', source: 'after', target: 'end' }, + ], +}); + +describe('NodeExecutor.onSuspensionReleased (#5512)', () => { + let engine: AutomationEngine; + let ran: string[]; + let seen: SuspensionRelease[]; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + ran = []; + seen = []; + engine.registerNodeExecutor(markerExecutor(ran)); + engine.registerNodeExecutor(recordingPauser('pauser', seen)); + }); + + it('notifies the paused node on resume, with the correlation it suspended with', async () => { + engine.registerFlow('pause_flow', pauseFlow('pauser')); + + const paused = await engine.execute('pause_flow'); + expect(paused.status).toBe('paused'); + expect(seen).toEqual([]); // nothing released yet — the run is still parked + + const resumed = await engine.resume(paused.runId!); + expect(resumed.success).toBe(true); + expect(ran).toEqual(['after']); + expect(seen).toEqual([ + { + runId: paused.runId, + flowName: 'pause_flow', + nodeId: 'hold', + correlation: 'test-armature:hold', + reason: 'resumed', + }, + ]); + }); + + it('notifies on cancelRun — a run cancelled while parked still has an armature to drop', async () => { + engine.registerFlow('pause_flow', pauseFlow('pauser')); + + const paused = await engine.execute('pause_flow'); + expect(await engine.cancelRun(paused.runId!, 'abandoned')).toBe(true); + + expect(seen).toHaveLength(1); + expect(seen[0]).toMatchObject({ runId: paused.runId, nodeId: 'hold', reason: 'cancelled' }); + expect(ran).toEqual([]); // cancelled, not continued + }); + + it('notifies exactly once, and only the executor of the node that paused', async () => { + const otherSeen: SuspensionRelease[] = []; + engine.registerNodeExecutor(recordingPauser('other_pauser', otherSeen)); + engine.registerFlow('pause_flow', pauseFlow('pauser')); + + const paused = await engine.execute('pause_flow'); + await engine.resume(paused.runId!); + + expect(seen).toHaveLength(1); + expect(otherSeen).toEqual([]); // not a broadcast — routed by the paused node's type + }); + + it('is silent for a pausing executor that implements no teardown', async () => { + engine.registerNodeExecutor({ + type: 'bare_pauser', + async execute() { return { success: true, suspend: true }; }, + } as NodeExecutor); + engine.registerFlow('pause_flow', pauseFlow('bare_pauser')); + + const paused = await engine.execute('pause_flow'); + const resumed = await engine.resume(paused.runId!); + expect(resumed.success).toBe(true); + expect(ran).toEqual(['after']); // the optional hook changes nothing for executors without it + }); + + it('routes a suspension rehydrated from the durable store (fresh engine, cold path)', async () => { + const store = new InMemorySuspendedRunStore(); + engine.setSuspendedRunStore(store); + engine.registerFlow('pause_flow', pauseFlow('pauser')); + const paused = await engine.execute('pause_flow'); + + // A second "process": new engine, same durable store — the run is resumed + // from the stored row, not the in-memory cache. + const seen2: SuspensionRelease[] = []; + const ran2: string[] = []; + const engine2 = new AutomationEngine(silentLogger(), store); + engine2.registerNodeExecutor(markerExecutor(ran2)); + engine2.registerNodeExecutor(recordingPauser('pauser', seen2)); + engine2.registerFlow('pause_flow', pauseFlow('pauser')); + + const resumed = await engine2.resume(paused.runId!); + expect(resumed.success).toBe(true); + expect(ran2).toEqual(['after']); + expect(seen2).toHaveLength(1); + expect(seen2[0]).toMatchObject({ nodeId: 'hold', correlation: 'test-armature:hold', reason: 'resumed' }); + expect(seen).toEqual([]); // released on the engine that consumed it, not the one that armed it + }); + + it('a teardown that throws is logged and does not fail the continuation', async () => { + const warns: string[] = []; + const loud = new AutomationEngine(silentLogger(warns)); + const ranLoud: string[] = []; + loud.registerNodeExecutor(markerExecutor(ranLoud)); + loud.registerNodeExecutor({ + type: 'pauser', + async execute(node) { return { success: true, suspend: true, correlation: `test-armature:${node.id}` }; }, + async onSuspensionReleased() { throw new Error('job service exploded'); }, + } as NodeExecutor); + loud.registerFlow('pause_flow', pauseFlow('pauser')); + + const paused = await loud.execute('pause_flow'); + const resumed = await loud.resume(paused.runId!); + + expect(resumed.success).toBe(true); // the run is what matters — teardown is best-effort + expect(ranLoud).toEqual(['after']); + const warn = warns.find((w) => w.includes('failed to release its suspension')); + expect(warn).toBeTruthy(); + // The line has to name the handle an operator would clean up by hand. + expect(warn).toContain('test-armature:hold'); + expect(warn).toContain('job service exploded'); + }); + + it('delegates through a deprecated node alias to the canonical executor', async () => { + engine.registerNodeAlias('pauser_old', 'pauser'); + engine.registerFlow('pause_flow', pauseFlow('pauser_old')); + + const paused = await engine.execute('pause_flow'); + const resumed = await engine.resume(paused.runId!); + + expect(resumed.success).toBe(true); + // The alias's synthesized executor implements no teardown of its own; the + // capability must not be lost just because the old type name was authored. + expect(seen).toHaveLength(1); + expect(seen[0]).toMatchObject({ nodeId: 'hold', reason: 'resumed' }); + }); +}); + +/** + * The `failed` reason, on the one path that produces it: a `subflow` ancestor + * terminally failed by its child (`failSuspendedRun`). It is delivered through + * the same choke point as the other two, which is the property worth pinning — + * an executor that implements the hook disarms on ALL routes out of a pause, not + * just the one it knows about. + * + * The real `subflow` executor is used, wrapped in a recording decorator: the + * subflow node closes over the live engine (it runs the child flow through it), + * so a hand-written stand-in would be testing the stand-in. + */ +describe('onSuspensionReleased — the `failed` route (subflow ancestor)', () => { + it('reports reason `failed` when a subflow descendant fails under the pause', async () => { + const engine = new AutomationEngine(silentLogger()); + const seen: SuspensionRelease[] = []; + + // Capture the executor `registerSubflowNode` registers, then re-register it + // wrapped — same `execute`, plus a recording teardown. + let real: NodeExecutor | undefined; + const register = engine.registerNodeExecutor.bind(engine); + (engine as unknown as { registerNodeExecutor: (e: NodeExecutor) => void }).registerNodeExecutor = (e) => { + if (e.type === 'subflow') real = e; + register(e); + }; + registerSubflowNode(engine, ctx()); + expect(real).toBeTruthy(); + register({ + type: 'subflow', + descriptor: real!.descriptor, + execute: (node, variables, context) => real!.execute(node, variables, context), + onSuspensionReleased(release) { seen.push(release); }, + }); + + engine.registerNodeExecutor({ + type: 'pauser', + async execute() { return { success: true, suspend: true }; }, + } as NodeExecutor); + engine.registerNodeExecutor({ + type: 'failer', + async execute() { return { success: false, error: 'boom' }; }, + } as NodeExecutor); + + engine.registerFlow('child_flow', { + name: 'child_flow', + label: 'Child', + type: 'autolaunched', + nodes: [ + { id: 'cs', type: 'start', label: 'Start' }, + { id: 'cp', type: 'pauser', label: 'Child Pause' }, + { id: 'cf', type: 'failer', label: 'Child Fail' }, + { id: 'ce', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'c1', source: 'cs', target: 'cp' }, + { id: 'c2', source: 'cp', target: 'cf' }, + { id: 'c3', source: 'cf', target: 'ce' }, + ], + }); + engine.registerFlow('parent_flow', { + name: 'parent_flow', + label: 'Parent', + type: 'autolaunched', + nodes: [ + { id: 'ps', type: 'start', label: 'Start' }, + { id: 'call', type: 'subflow', label: 'Call Child', config: { flowName: 'child_flow' } }, + { id: 'pe', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'p1', source: 'ps', target: 'call' }, + { id: 'p2', source: 'call', target: 'pe' }, + ], + }); + + const parent = await engine.execute('parent_flow'); + expect(parent.status).toBe('paused'); // parent parked on the subflow node + const child = engine.listSuspendedRuns().find((r) => r.flowName === 'child_flow')!; + expect(child).toBeTruthy(); + + const childRes = await engine.resume(child.runId); + expect(childRes.success).toBe(false); // the child failed after its pause + + // The parent's suspension was consumed by the failure path, and its node + // was told so — with `failed`, not `resumed`. + const failed = seen.filter((r) => r.reason === 'failed'); + expect(failed).toHaveLength(1); + expect(failed[0]).toMatchObject({ runId: parent.runId, nodeId: 'call', reason: 'failed' }); + }); +});