diff --git a/.changeset/try-catch-failing-catch-step-record.md b/.changeset/try-catch-failing-catch-step-record.md new file mode 100644 index 0000000000..6250fdc417 --- /dev/null +++ b/.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. 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 c94cba0fc5..66eac7ad9b 100644 --- a/packages/services/service-automation/src/builtin/try-catch-node.ts +++ b/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], + }; } } diff --git a/packages/services/service-automation/src/builtin/try-catch-returned-failure-steps.test.ts b/packages/services/service-automation/src/builtin/try-catch-returned-failure-steps.test.ts index c5a8f6384b..7361563b86 100644 --- a/packages/services/service-automation/src/builtin/try-catch-returned-failure-steps.test.ts +++ b/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); }); });