fix(automation): publish $error on the throw arm before deciding whether the failure routes

.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 `<nodeId>.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 `<nodeId>.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.

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({

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.

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);