fix(automation): carry both regions' steps when a try_catch catch region fails (#14813)

`try_catch` returns a failure from three sites. #14184 taught the engine's
returned-failure branch to fold `childSteps` and taught the no-`catch`
producer to supply them; the `catch`-present-and-failing return was left
unfolded and still discarded the whole step record.

It is the worst of the three for an operator, because two regions ran: the
try region may have written rows before it failed and the handler may have
written more before IT failed, yet the run log kept a step for neither, so
the #4354 summary reported `acted: 0` over writes that had landed.

The catch region now gets the same `partialSteps` sink the try region
already had (`runRegion`'s fifth argument) and the failing return carries
`[...failedAttemptSteps, ...catchAttemptSteps]` — failed try attempts
first, matching the successful-catch return's ordering. `runRegion`'s
tagger already supplies `regionKind` on its failure path, so no tagging is
added here and no engine change is needed.

The pin that recorded the old boundary ("carries no try steps") is
inverted in place with its comment rewritten to say what it used to assert
and what moved it, rather than deleted.


Claude-Session: https://claude.ai/code/session_01AUF1NoViznQK32gqpK8wS8

Co-authored-by: Claude <noreply@anthropic.com>

Author: claude[bot]

.changeset/try-catch-failing-catch-step-record.md

@@ -0,0 +1,37 @@
+---
+"@objectstack/service-automation": patch
+---
+
+fix(automation): a `try_catch` whose `catch` region itself fails now keeps the step record of both regions
+
+`try_catch` returns a failure from three sites. #13803 taught the engine to fold
+a dying container's carried steps off the THROW channel, #14184 taught its
+returned-failure branch (`if (!result.success)`) to do the same, and #14184 also
+taught the first producer — a `try_catch` with no `catch` region — to supply
+them. The second producer was left unfolded: when a `catch` region is present
+and the handler itself fails, the return dropped `childSteps` entirely.
+
+That is the same defect one path over, and the worst of the three for an
+operator, because TWO regions ran. The try region may have written rows before
+it failed; the handler may have written more before IT failed; the run log kept
+a step for neither, so the run summary folded over that log reported `acted: 0`
+over writes that had genuinely landed. `acted: 0` on a failed run reads as
+"nothing happened, safe to re-run", which for a non-idempotent region invites
+double-execution.
+
+Closing it needed the half that was genuinely missing rather than the available
+one: the failed try attempts were already in scope, but the catch region ran
+without a `partialSteps` sink, so when the handler threw, the handler's own
+completed steps unwound with the stack. The catch region now receives the same
+sink the try region already had (`runRegion`'s fifth argument), and the failing
+return carries `[...failedTryAttempts, ...catchAttempts]` — failed try attempts
+first, because they happened first, which is the ordering the successful-catch
+return has always used. `runRegion`'s existing tagger supplies `regionKind:
+'try'` / `'catch'` and `parentNodeId` on its failure path as well as its
+success path, so the two halves stay distinguishable in the log.
+
+Additive to the RECORD only. This return already reported failure with the same
+error text, already produced a `NODE_FAILURE` step, already set `$error` and was
+already routable by a `fault` edge; none of that moves, and neither does the
+successful-catch path or the retry/throw semantics. No engine change was needed
+— the fold that reads these steps has been in place since #14184.

packages/services/service-automation/src/builtin/try-catch-node.ts

@@ -161,12 +161,23 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex
       // The try region (and any retries) failed. Run the catch handler if present.
       if (catchRegion != null) {
         variables.set(errorVariable, { nodeId: node.id, message: lastError });
+        // #14222: sink for the catch region's OWN partial steps, filled by
+        // `runRegion` only if the handler itself throws. Without it the catch
+        // region's completed steps unwound with the stack exactly as the try
+        // region's did before #7546 — see the failing-catch return below.
+        const catchAttemptSteps: StepLogEntry[] = [];
         try {
           // #1479: surface the catch handler region's steps.
-          const catchSteps = await engine.runRegion(catchRegion, variables, ctxOrEmpty, {
-            parentNodeId: node.id,
-            regionKind: 'catch',
-          });
+          const catchSteps = await engine.runRegion(
+            catchRegion,
+            variables,
+            ctxOrEmpty,
+            {
+              parentNodeId: node.id,
+              regionKind: 'catch',
+            },
+            catchAttemptSteps,
+          );
           return {
             success: true,
             output: { attempts: maxRetries + 1, caught: true, error: lastError },
@@ -177,7 +188,34 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex
           };
         } catch (catchErr) {
           const catchMsg = catchErr instanceof Error ? catchErr.message : String(catchErr);
-          return { success: false, error: `try_catch '${node.id}': catch region failed — ${catchMsg}` };
+          // #14222 — the THIRD returned-failure path, and the last one still
+          // discarding its record. #13803 taught the engine to fold a dying
+          // container's steps off the THROW channel and #14184 taught the
+          // returned-failure branch the same, but only the no-`catch` producer
+          // was taught to supply them. This return is the worst of the three
+          // for an operator, because TWO regions ran: the try region may have
+          // written rows before it failed, the handler may have written more
+          // before IT failed, and the run log kept a step for neither — so the
+          // #4354 summary folded over that log reported `acted: 0` over writes
+          // that had genuinely landed. `acted: 0` on a failed run reads as
+          // "nothing happened, safe to re-run".
+          //
+          // Ordering mirrors the successful-catch return directly above: the
+          // failed try attempts come FIRST because they happened first, then
+          // whatever the handler got through. `runRegion` has already tagged
+          // both sets (`parentNodeId`, `regionKind: 'try'` / `'catch'`) on its
+          // failure path as well as its success path, so the two halves stay
+          // distinguishable in the log without anything being tagged here.
+          //
+          // Additive to the RECORD only: this return already reported failure
+          // with this error text, already produced a `NODE_FAILURE` step and
+          // was already routable by a `fault` edge. No engine change — the
+          // #14184 fold on `if (!result.success)` is already the reader.
+          return {
+            success: false,
+            error: `try_catch '${node.id}': catch region failed — ${catchMsg}`,
+            childSteps: [...failedAttemptSteps, ...catchAttemptSteps],
+          };
         }
       }
 

packages/services/service-automation/src/builtin/try-catch-returned-failure-steps.test.ts

@@ -50,6 +50,19 @@ import { registerLogicNodes } from './logic-nodes.js';
  * contents and the same fault-edge routing. This is a record fix; it does not
  * touch accept/reject. The nesting group pins the other risk the new fold
  * introduces — that a carried step could now reach the log twice.
+ *
+ * ## The third returned-failure path (#14222)
+ *
+ * `try_catch` returns failure from THREE sites, and #14184 taught only one of
+ * them (no `catch` region) to carry its steps. The second — a `catch` region
+ * that itself fails — kept discarding the record, and this file pinned that as
+ * current behaviour rather than endorsing it. #14222 closed it by giving the
+ * catch region the same `partialSteps` sink the try region already had, so the
+ * failing-catch return now carries `[...failedTryAttempts, ...catchAttempts]`.
+ * That pin is INVERTED in place below, comment and all: what it used to assert
+ * is written out there, because the boundary it recorded is what makes the
+ * change legible. (The third site is the config-parse refusal, where nothing
+ * ran and there is no record to carry.)
  */
 
 function silentLogger(): any {
@@ -322,7 +335,7 @@ describe('#14184 a no-catch try_catch keeps the record of the writes its try reg
       expectNeverUnderReports(res.summary?.acted);
     });
 
-    it('a failing `catch` region still fails with the catch error and carries no try steps', async () => {
+    it('a failing `catch` region fails with the catch error AND carries both regions\' steps', async () => {
       const { res, record } = await run({
         try: WRITE_WRITE_BOOM,
         catch: { nodes: [{ id: 'handler', type: 'boom', label: 'Handler' }], edges: [] },
@@ -333,13 +346,77 @@ describe('#14184 a no-catch try_catch keeps the record of the writes its try reg
         "Node 'guard' failed: try_catch 'guard': catch region failed — "
           + "Node 'handler' failed: boom: at least one recipient is required",
       );
-      // A DIFFERENT return, and this card does not touch it: it still carries
-      // no `childSteps`, so the log keeps no try-region steps. That is the same
-      // defect one path over and it is filed as #14222, not endorsed here —
-      // closing it needs a `partialSteps` sink for the catch region, which is a
-      // new seam rather than a mirror of this change. Whoever fixes #14222
-      // updates this pin deliberately.
-      expect((record?.steps ?? []).filter(s => s.regionKind === 'try')).toHaveLength(0);
+
+      // INVERTED by #14222 — and the record of what it used to say is the point.
+      // Until then this case asserted the OPPOSITE:
+      //
+      //     expect((record?.steps ?? []).filter(s => s.regionKind === 'try'))
+      //       .toHaveLength(0);
+      //
+      // with a comment saying that was a DIFFERENT return which #14184 did not
+      // touch: it carried no `childSteps`, so the log kept no try-region steps,
+      // and closing that gap needed a `partialSteps` sink for the catch region
+      // — a new seam rather than a mirror of #14184's change. The assertion was
+      // a deliberate record of the boundary #14184 stopped at, never an
+      // endorsement, and it named #14222 as the card that would move it.
+      //
+      // #14222 moved it. It added the sink (the fifth `runRegion` argument the
+      // try region already received), and the triage ruling that closed it is a
+      // restore-invariant: the run log must carry every step that ran,
+      // whichever region ran it. So the pin now reads the other way — and reads
+      // the full shape rather than just presence: ordering is failed try
+      // attempts FIRST (they happened first), which is the same rule the
+      // successful-catch return has always used, and `runRegion`'s own tagger
+      // supplies `regionKind` on its failure path as well as its success path.
+      const grouped = (record?.steps ?? [])
+        .filter(s => s.parentNodeId === 'guard')
+        .map(s => `${s.regionKind}:${s.nodeId}:${s.status}`);
+      expect(grouped).toEqual([
+        'try:w1:success',
+        'try:w2:success',
+        'try:bang:failure',
+        'catch:handler:failure',
+      ]);
+    });
+
+    it('a `catch` region that writes before failing reports the writes from BOTH regions', async () => {
+      const { res, record } = await run({
+        try: WRITE_WRITE_BOOM,
+        catch: {
+          nodes: [
+            { id: 'cw1', type: 'write', label: 'Compensating write' },
+            { id: 'handler', type: 'boom', label: 'Handler' },
+          ],
+          edges: [{ id: 'c1', source: 'cw1', target: 'handler' }],
+        },
+      });
+
+      // The shape the card calls the worst of the three for an operator: TWO
+      // regions ran and both wrote before failing. Three rows are in the store.
+      expect(res.success).toBe(false);
+      expect(written).toEqual(['w1', 'w2', 'cw1']);
+      expect(realWrites()).toBe(3);
+      expectNeverUnderReports(res.summary?.acted);
+      expect(res.summary?.acted).toBe(realWrites());
+
+      // This is the half the sink adds. The try region's steps alone — the
+      // one-liner #14184 could have written here — would leave `cw1`
+      // unrecorded and `acted` stuck at 2 over three writes that landed.
+      const grouped = (record?.steps ?? [])
+        .filter(s => s.parentNodeId === 'guard')
+        .map(s => `${s.regionKind}:${s.nodeId}:${s.status}`);
+      expect(grouped).toEqual([
+        'try:w1:success',
+        'try:w2:success',
+        'try:bang:failure',
+        'catch:cw1:success',
+        'catch:handler:failure',
+      ]);
+
+      // Two sinks feed one return now, so pin what that risks: each carried
+      // step still reaches the log exactly once.
+      const steps = record?.steps ?? [];
+      expect(new Set(steps).size).toBe(steps.length);
     });
   });