From ebad613cb7c3db50eeffe0e3366ce0b9297a5901 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 12:27:56 +0000 Subject: [PATCH] fix(automation): publish `$error` on the throw arm before deciding whether the failure routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine's returned-failure arm rewrites the run-wide `$error` and `.error` unconditionally, and only then asks whether a `fault` edge may route the failure. The throw arm did both inside `if (faultEdge)`, so a thrown failure with no fault edge of its own left `$error` naming an earlier, unrelated failure — and a node inside a structured region never has a fault edge of its own, because the region's synthetic sub-flow carries only the region's own edges. The message and the code came from two different failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --- .../throw-arm-error-refresh-symmetry.md | 13 + .../create-record-duplicate-code.test.ts | 26 +- .../src/builtin/try-catch-node.ts | 31 ++- .../services/service-automation/src/engine.ts | 36 ++- .../src/throw-arm-error-refresh.test.ts | 247 ++++++++++++++++++ 5 files changed, 327 insertions(+), 26 deletions(-) create mode 100644 .changeset/throw-arm-error-refresh-symmetry.md create mode 100644 packages/services/service-automation/src/throw-arm-error-refresh.test.ts diff --git a/.changeset/throw-arm-error-refresh-symmetry.md b/.changeset/throw-arm-error-refresh-symmetry.md new file mode 100644 index 0000000000..2f9804dbef --- /dev/null +++ b/.changeset/throw-arm-error-refresh-symmetry.md @@ -0,0 +1,13 @@ +--- +"@objectstack/service-automation": patch +--- + +`$error` now names the most recent failure in a flow run, whichever way that failure arrived. + +The automation engine has two failure arms. When a node FAILS BY RETURNING `{ success: false }`, the engine rewrote the run-wide `$error` (and `.error`) and then decided whether a `fault` edge could route it. When a node FAILED BY THROWING — a `timeoutMs` firing, a dying nested container, a thrown guard — it did both **inside** the `fault`-edge branch, so a thrown failure with no `fault` edge of its own left `$error` holding an earlier, unrelated failure's value. + +A node inside a structured region never has a `fault` edge of its own: the region's synthetic sub-flow carries only the region's own edges. So every thrown failure inside a `try_catch`, `loop` body or other region hit this. The result was not a crash but a plausible-looking wrong value: **the message and the code came from two different failures** — `{ code: 'DUPLICATE_RECORD', message: "Node 'mk' timed out after 20ms" }` — and a catch region branching on `{$error.code}` swallowed a store failure as "the row is already there" while the run reported success. + +The throw arm now publishes `$error` and `.error` before deciding whether the failure routes, exactly as the returned-failure arm does. What a thrown failure publishes is `{ nodeId, message }`: there is no node result on that path, so no `output` and no classified `code` exist to carry — and that absence is the right answer for a throw rather than a reason to leave a stale `code` standing. + +Routing is unchanged. A guard refusal that throws (ADR-0049's unscoped-run refusal, for one) is still un-routable, still fatal, and still reports its own message; the thrown value itself is rethrown exactly as caught. diff --git a/packages/services/service-automation/src/builtin/create-record-duplicate-code.test.ts b/packages/services/service-automation/src/builtin/create-record-duplicate-code.test.ts index b7201b108b..1f16919cb6 100644 --- a/packages/services/service-automation/src/builtin/create-record-duplicate-code.test.ts +++ b/packages/services/service-automation/src/builtin/create-record-duplicate-code.test.ts @@ -256,18 +256,20 @@ describe('#14419 (PR #14948 patch round 1) — a stale $error.code must not leak /** * The tier contract review's reproduction. `try_catch`'s catch region reads * `code` off the run-wide `$error` (necessarily — see the second describe - * block above). But the engine only REWRITES `$error` when a failing node - * RETURNS `{ success: false }`, or when it THROWS through a node with its - * OWN `fault` edge (`engine.ts`'s `executeNode`, the throw arm). A node - * inside a `try_catch`'s try region has no fault edge of its own — the - * region's synthetic sub-flow carries only the region's own edges — so a - * node that FAILS BY THROWING (a `timeoutMs` firing, here) leaves `$error` - * exactly as an EARLIER, unrelated failure left it. Without the identity - * guard in `try-catch-node.ts` (`innerError !== errorBefore`), that earlier - * failure's `DUPLICATE_RECORD` code leaks onto this one — a store failure - * misread as a duplicate and SWALLOWED, through a different door than - * #14419's original bug but the exact same failure mode: the very thing - * fence 4 of the ruling of record exists to rule out. + * block above), so a stale `$error` becomes a store failure misread as a + * duplicate and SWALLOWED — a different door than #14419's original bug, + * the exact same failure mode, and the very thing fence 4 of the ruling of + * record exists to rule out. + * + * Two independent things keep these two flows green, and this block pins the + * OUTCOME rather than either mechanism: the identity guard in + * `try-catch-node.ts` (`innerError !== errorBefore`), which is what closed + * them when this block was written, and #14955's removal of the underlying + * asymmetry — the engine's throw arm now publishes `$error` unconditionally, + * so the thrown timeout here names itself and carries no `code`, instead of + * leaving an earlier failure's `DUPLICATE_RECORD` standing. Either one alone + * holds these two; `throw-arm-error-refresh.test.ts` carries the shape that + * needs the second. */ function registerProbes(engine: AutomationEngine, seen: Array<{ code?: string; message?: string }>, ran: string[]): void { engine.registerNodeExecutor({ diff --git a/packages/services/service-automation/src/builtin/try-catch-node.ts b/packages/services/service-automation/src/builtin/try-catch-node.ts index 04359f3391..80021174d6 100644 --- a/packages/services/service-automation/src/builtin/try-catch-node.ts +++ b/packages/services/service-automation/src/builtin/try-catch-node.ts @@ -158,18 +158,25 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex // Sink for THIS attempt's partial steps, filled by `runRegion` only if // the attempt throws (#7546). const attemptSteps: StepLogEntry[] = []; - // #14948 review — the run-wide `$error` this attempt STARTS with. The - // engine rewrites `$error` (a fresh object, see engine.ts's - // `executeNode`) only when a failing node RETURNS `{ success: false }`, - // or when it THROWS through a node that has its OWN `fault` edge — and - // a node inside this region's synthetic sub-flow never has one (the - // sub-flow carries only the region's own edges). So a node that FAILS - // BY THROWING (a `timeoutMs` firing, a dying nested container, a - // thrown guard) leaves `$error` exactly as an EARLIER failure left it. - // Without this identity check, that earlier failure's `code` (e.g. a - // sibling row's `DUPLICATE_RECORD`) leaks onto an unrelated later - // failure's binding — a store failure misread as a duplicate through - // the very door this card exists to close. + // #14948 review — the run-wide `$error` this attempt STARTS with, kept + // as the outer bound on what this attempt is allowed to claim as its + // own failure. + // + // It was load-bearing on its own until #14955: the engine's throw arm + // used to rewrite `$error` only for a node with its OWN `fault` edge, + // and a node inside this region's synthetic sub-flow never has one (the + // sub-flow carries only the region's own edges), so a node that FAILED + // BY THROWING left `$error` exactly as an EARLIER failure had left it — + // and that earlier failure's `code` (a sibling row's + // `DUPLICATE_RECORD`, say) leaked onto an unrelated later failure's + // binding, a store failure misread as a duplicate. #14955 made the + // throw arm publish unconditionally, so a thrown node failure now names + // itself here and carries no `code` of its own. + // + // The check stays because a throw is not always a NODE failure: a + // durable pause refused inside a region, a missing region entry, or + // anything else `runRegion` raises before reaching a node never touches + // `$error` at all, and the stale value must not be claimed then either. const errorBefore = variables.get('$error'); try { // #1479: surface the successful try region's steps. diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 717d14273c..8b8aeaf9ed 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -7877,6 +7877,40 @@ export class AutomationEngine implements IAutomationService { steps.push(...carriedSteps); } + // #14955 — publish the failure BEFORE deciding whether it + // routes, exactly as the returned-failure arm below does. + // + // This write used to sit inside `if (faultEdge)`, and no reason + // for that was ever recorded: both arms were written in one + // commit, and only this one conflated "publish the failure" with + // "route the failure" — the arm below already separated them. + // The consequence was invisible rather than loud. A node inside a + // region never has a `fault` edge of its own (the region's + // synthetic sub-flow carries only the region's own edges — see + // {@link AutomationEngine.runRegion}), so EVERY thrown failure + // inside a region left `$error` naming an earlier, unrelated + // failure. The first reader that cared about `$error`'s freshness + // — `try_catch`'s `code` binding (#14419) — met it immediately + // and bound a message and a code that came from two different + // failures: `{ code: 'DUPLICATE_RECORD', message: "Node 'mk' + // timed out after 20ms" }`, swallowed by a catch region reading + // it as "the row is already there", the run reporting success. + // + // `{ nodeId, message }` and nothing more: there is no + // `NodeExecutionResult` on this path, so no `output` and no + // classified `code` exist to carry — and that absence is the + // correct answer for a throw, not a reason to leave a stale + // `code` standing. This is what the documented contract already + // promised — `{$error}` names the most recent failure + // (`content/docs/automation/flows.mdx`) — and now holds. + // + // Publishing is not routing: the guard-refusal rule below is + // untouched and still decides, alone, which failures a `fault` + // edge may carry. Nor is the thrown value touched — `execErr` is + // rethrown below exactly as caught. + variables.set('$error', { nodeId: node.id, message: errMsg }); + this.setNodeError(variables, node.id, errMsg); + // #3863 — a guard that THROWS is as un-routable as one that // returns: `UnscopedRunDataAccessError` (ADR-0049/#1888) reports // that the metadata would run unscoped, and rerouting it would @@ -7885,8 +7919,6 @@ export class AutomationEngine implements IAutomationService { ? undefined : flow.edges.find(e => e.source === node.id && e.type === 'fault'); if (faultEdge) { - variables.set('$error', { nodeId: node.id, message: errMsg }); - this.setNodeError(variables, node.id, errMsg); const faultTarget = flow.nodes.find(n => n.id === faultEdge.target); if (faultTarget) { await this.executeNode(faultTarget, flow, variables, context, steps); diff --git a/packages/services/service-automation/src/throw-arm-error-refresh.test.ts b/packages/services/service-automation/src/throw-arm-error-refresh.test.ts new file mode 100644 index 0000000000..c58d5f661c --- /dev/null +++ b/packages/services/service-automation/src/throw-arm-error-refresh.test.ts @@ -0,0 +1,247 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #14955 — the run-wide `$error` must name the LAST failure, whichever way + * that failure arrived. + * + * `executeNode` has two failure arms. The returned-failure arm (`!result.success`) + * rewrites `$error` and `.error` unconditionally, then decides whether a + * `fault` edge may route it. The throw arm used to do both inside `if (faultEdge)`, + * so a thrown failure with no fault edge of its own left `$error` holding an + * EARLIER failure's value — and a node inside a region never has a fault edge of + * its own, because the region's synthetic sub-flow carries only the region's own + * edges (`runRegion`). + * + * **The message and the code come from two different failures.** That is the + * shape this file drives: not a variable read back in isolation, but two real + * flows whose recovery takes a DIFFERENT EDGE depending on whether `$error` is + * fresh — because the failure mode here is a plausible-looking wrong value, not + * a crash, and an assertion on the value alone would not have caught it either. + */ +import { describe, it, expect } from 'vitest'; +import { AutomationEngine } from './engine.js'; +import type { NodeExecutor } from './engine.js'; +import { registerCrudNodes } from './builtin/crud-nodes.js'; +import { registerTryCatchNode } from './builtin/try-catch-node.js'; +import { DuplicateRecordError } from '@objectstack/objectql'; + +function makeLogger(): any { + const l: any = { info() {}, warn() {}, error() {}, debug() {} }; + l.child = () => l; + return l; +} + +const ctxWith = (data: any): any => ({ + logger: makeLogger(), + getService: (n: string) => (n === 'data' ? data : undefined), +}); + +describe('#14955 — the throw arm refreshes `$error` like the returned-failure arm', () => { + it('a thrown failure with no fault edge of its own names ITSELF on the run-wide `$error`', async () => { + // `tc` binds its caught error to `$caught`, deliberately NOT to `$error`, + // so the catch region reads the ENGINE's run-wide variable rather than + // `try_catch`'s own rebuilt binding. + const engine = new AutomationEngine(makeLogger()); + let seenError: any; + let seenNodeScoped: any; + const data: any = { + async insert() { + throw new DuplicateRecordError('lead', new Error('driver: unique violation'), 'email'); + }, + }; + registerCrudNodes(engine, ctxWith(data)); + registerTryCatchNode(engine, ctxWith(data)); + engine.registerNodeExecutor({ + type: 'store_down', + async execute() { + throw new Error('ECONNREFUSED: could not reach the store'); + }, + } as NodeExecutor); + engine.registerNodeExecutor({ + type: 'probe', + async execute(_node, variables) { + seenError = variables.get('$error'); + seenNodeScoped = variables.get('boom'); + return { success: true }; + }, + } as NodeExecutor); + engine.registerFlow('stale_error', { + name: 'stale_error', label: 'Stale error', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'a', type: 'create_record', label: 'A', config: { objectName: 'lead', fields: { email: 'a@b.com' } } }, + { id: 'recoverA', type: 'probe_noop', label: 'Recover A' }, + { + id: 'tc', type: 'try_catch', label: 'Guarded', + config: { + errorVariable: '$caught', + try: { nodes: [{ id: 'boom', type: 'store_down', label: 'Boom' }], edges: [] }, + catch: { nodes: [{ id: 'look', type: 'probe', label: 'Look' }], edges: [] }, + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'a' }, + // `a` fails as a duplicate and is fault-routed — this is what leaves + // `DUPLICATE_RECORD` sitting on `$error` before `boom` ever runs. + { id: 'ef', source: 'a', target: 'recoverA', type: 'fault' }, + { id: 'e2', source: 'recoverA', target: 'tc' }, + { id: 'e3', source: 'tc', target: 'end' }, + ], + } as any); + engine.registerNodeExecutor({ type: 'probe_noop', async execute() { return { success: true }; } } as NodeExecutor); + + await engine.execute('stale_error', { userId: 'u1' } as any); + + // The last failure was `boom`'s throw, not `a`'s duplicate. + expect(seenError?.nodeId).toBe('boom'); + expect(seenError?.message).toContain('ECONNREFUSED'); + // `a`'s classified code must not still be sitting there. + expect(seenError?.code).toBeUndefined(); + // `.error` is written on the same terms as on the returned-failure arm. + expect(seenNodeScoped?.error).toContain('ECONNREFUSED'); + }); + + it('a swallowed failure\'s code does not leak onto a LATER thrown failure in the same try region', async () => { + // The residual the PR #14948 identity guard cannot see: that guard + // compares `$error` against what the attempt STARTED with, so a rewrite + // that happens INSIDE the attempt window (here: an inner `try_catch` + // binding its own swallowed duplicate) passes the identity check, and the + // outer container binds a `DUPLICATE_RECORD` code onto a timeout's message. + const engine = new AutomationEngine(makeLogger()); + const ran: string[] = []; + const seen: Array<{ code?: string; message?: string }> = []; + let call = 0; + const data: any = { + async insert() { + call++; + if (call === 1) throw new DuplicateRecordError('lead', new Error('driver: unique violation'), 'email'); + // The store hangs — the node's own `timeoutMs` fires, which is a + // THROW, not a returned failure. + await new Promise(() => {}); + }, + }; + registerCrudNodes(engine, ctxWith(data)); + registerTryCatchNode(engine, ctxWith(data)); + engine.registerNodeExecutor({ + type: 'checkpoint', + async execute(node) { ran.push(String((node.config as any)?.tag)); return { success: true }; }, + } as NodeExecutor); + engine.registerNodeExecutor({ + type: 'swallow_duplicate_else_reraise', + async execute(_node, variables) { + const err = variables.get('$error') as { code?: string; message?: string } | undefined; + seen.push({ code: err?.code, message: err?.message }); + if (err?.code === 'DUPLICATE_RECORD') { ran.push('swallowed'); return { success: true }; } + ran.push('reraised'); + throw new Error(`re-raising: ${err?.message}`); + }, + } as NodeExecutor); + engine.registerFlow('nested', { + name: 'nested', label: 'Nested', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'outer', type: 'try_catch', label: 'Outer', + config: { + try: { + nodes: [ + { + id: 'inner', type: 'try_catch', label: 'Inner', + config: { + try: { nodes: [{ id: 'mk', type: 'create_record', label: 'Create', config: { objectName: 'lead', fields: { email: 'a@b.com' } } }], edges: [] }, + catch: { nodes: [{ id: 'innerHandle', type: 'checkpoint', label: 'Inner handle', config: { tag: 'inner-swallowed' } }], edges: [] }, + }, + }, + { id: 'slow', type: 'create_record', label: 'Slow create', timeoutMs: 20, config: { objectName: 'lead', fields: { email: 'c@d.com' } } }, + ], + edges: [{ id: 'r1', source: 'inner', target: 'slow' }], + }, + catch: { nodes: [{ id: 'outerHandle', type: 'swallow_duplicate_else_reraise', label: 'Outer handle' }], edges: [] }, + }, + }, + { id: 'after', type: 'checkpoint', label: 'After', config: { tag: 'after' } }, + { id: 'escalate', type: 'checkpoint', label: 'Escalate', config: { tag: 'escalate' } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'outer' }, + { id: 'e2', source: 'outer', target: 'after' }, + { id: 'e3', source: 'outer', target: 'escalate', type: 'fault' }, + { id: 'e4', source: 'after', target: 'end' }, + { id: 'e5', source: 'escalate', target: 'end' }, + ], + } as any); + + await engine.execute('nested', { userId: 'u1' } as any); + + // The outer handler saw a TIMEOUT, and must not have been told it was a duplicate. + expect(seen[0]?.message).toContain('timed out'); + expect(seen[0]?.code).toBeUndefined(); + // Different edges, not just a different value: the timeout escalates. + expect(ran).toEqual(['inner-swallowed', 'reraised', 'escalate']); + }); + + it('a THROWN guard refusal is still un-routable, refresh or no refresh (#3863)', async () => { + // The refresh moved OUT of `if (faultEdge)`; the guard-refusal rule that + // decides ROUTING did not move. A guard refusal that throws must still + // abort the run with its own message and never reach the destructive + // verb — including when an earlier fault-routed failure left a + // recoverable-looking `$error` behind for the new unconditional write to + // overwrite. + // + // The seed is a plain executor, NOT a `create_record`: this run carries + // no trigger user (that is what arms the ADR-0049 guard below), and a + // data node in the same run would be refused by that very guard before it + // could seed anything. + const engine = new AutomationEngine(makeLogger()); + const ran: string[] = []; + const seen: { deleted: boolean } = { deleted: false }; + const data: any = { + find: async () => [], + findOne: async () => null, + update: async () => ({ modified: 0 }), + delete: async () => { seen.deleted = true; return { deleted: 1 }; }, + getObject: () => ({ name: 'deal', fields: {} }), + }; + registerCrudNodes(engine, ctxWith(data)); + engine.registerNodeExecutor({ + type: 'fails_with_code', + async execute() { return { success: false, error: 'upstream 503', code: 'DUPLICATE_RECORD' }; }, + } as NodeExecutor); + engine.registerNodeExecutor({ + type: 'checkpoint', + async execute(node) { ran.push(String((node.config as any)?.tag)); return { success: true }; }, + } as NodeExecutor); + engine.registerFlow('guard_after_stale', { + name: 'guard_after_stale', label: 'Guard after stale', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'a', type: 'fails_with_code', label: 'A' }, + { id: 'recoverA', type: 'checkpoint', label: 'Recover A', config: { tag: 'recoverA' } }, + { id: 'op', type: 'delete_record', label: 'Delete', config: { objectName: 'deal', filter: { status: 'closed' } } }, + { id: 'handler', type: 'checkpoint', label: 'Handler', config: { tag: 'handler' } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'a' }, + { id: 'ef', source: 'a', target: 'recoverA', type: 'fault' }, + { id: 'e2', source: 'recoverA', target: 'op' }, + { id: 'e3', source: 'op', target: 'end' }, + { id: 'e_fault', source: 'op', target: 'handler', type: 'fault' }, + { id: 'e4', source: 'handler', target: 'end' }, + ], + } as any); + + // No trigger user and the default `runAs: 'user'` — the ADR-0049/#1888 + // unscoped case, which THROWS its refusal. + const result = await engine.execute('guard_after_stale'); + + expect(result.success).toBe(false); + expect(result.error).toContain('runAs'); + // The seed recovered; the guard refusal did NOT route to its handler. + expect(ran).toEqual(['recoverA']); + expect(seen.deleted).toBe(false); + }); +});