diff --git a/.changeset/27743-try-catch-resume-carry-refusal.md b/.changeset/27743-try-catch-resume-carry-refusal.md new file mode 100644 index 00000000000..c466ec3e008 --- /dev/null +++ b/.changeset/27743-try-catch-resume-carry-refusal.md @@ -0,0 +1,15 @@ +--- +'@objectstack/service-automation': patch +--- + +fix(service-automation): a refusal met inside a `try_catch` with no catch region, or by a `subflow` child that pauses and fails on resume, reaches the flow's caller as the structured refusal + +Clause-②: no +Metadata-protocol: none + +A flow whose data node met a refusal (a permission, a validation rule, a duplicate, an app hook's declared refusal) reports it on the run's result as `innerRefusal` (`{ code, message }`) and the producer-marked `userMessage`, and `map` and `subflow` carry it across themselves. Two more paths built a new failure and lost both members, so the caller got only the node chain as text: + +- A `try_catch` with no catch region. It now carries the refusal its last failed attempt met, so a retry ladder reports the last attempt's refusal, never an earlier one's. +- A `subflow` child that paused (a screen, an approval, a wait) and then met a refusal on resume, when the caller resumed the parent's run id. The parent's failure now carries the child's refusal, through nested subflows too. + +Resuming the child's own run id (the leg a `map` item takes) already carried it, and is now pinned. A failure that met no refusal carries no member, as before, and the `error` text is unchanged. 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 e9eb374d2cc..5186063a00b 100644 --- a/packages/services/service-automation/src/builtin/try-catch-node.ts +++ b/packages/services/service-automation/src/builtin/try-catch-node.ts @@ -9,6 +9,7 @@ import { parseNodeConfig } from './parse-config.js'; import { currentLoopIteration } from './loop-frame.js'; import { isRegionSuspensionRefusal } from '../region-suspension-refusal.js'; import { attachPartialSteps } from '../partial-steps.js'; +import { readCarriedRefusal, type CarriedRefusal } from '../inner-refusal.js'; /** * `try_catch` built-in node — **structured try/catch/retry** (ADR-0031 §Decision 3). @@ -150,6 +151,12 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex // region could never distinguish "the row is already there" from any // other failure — the whole point of commit c5a7448d5. let lastErrorCode: string | undefined; + // #27743 — the refusal the LAST failed attempt carried (`inner-refusal.ts`), + // read off the brand the engine put on that attempt's throw. Replaced on + // every failed attempt, so a retry ladder reports its last attempt's + // refusal and never an earlier one's — `retryExecution`'s rule — and an + // attempt that met no refusal leaves `{}`. + let lastCarried: CarriedRefusal = {}; const failedAttemptSteps: StepLogEntry[] = []; for (let attempt = 0; attempt <= maxRetries; attempt++) { if (attempt > 0) { @@ -234,6 +241,7 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex throw err; } lastError = err instanceof Error ? err.message : String(err); + lastCarried = readCarriedRefusal(err); const innerError = variables.get('$error'); // Only a `$error` that actually CHANGED (identity, not content — // two failures can legitimately share a message) belongs to THIS @@ -365,9 +373,18 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex // already routable by a `fault` edge. Adding `childSteps` moves none of // that — unlike the rejected "make `loop` swallow its throw and return" // shape (see `partial-steps.ts`), which would have changed all four. + // + // #27743 — and the try region's REFUSAL rides out the same way. This + // return re-words the region's failure into a new one, and the brand on + // the region's throw does not cross that, so a refusal met inside a + // guarded region reached the run's caller as text only. Spread here, the + // engine brands this node's throw like any data node's and the members + // reach the terminal exit (`inner-refusal.ts`); `{}` when the last + // attempt met none. The `error` text does not move. return { success: false, error: `try_catch '${node.id}': try region failed — ${lastError}`, + ...lastCarried, childSteps: failedAttemptSteps, }; }, diff --git a/packages/services/service-automation/src/flow-inner-refusal.test.ts b/packages/services/service-automation/src/flow-inner-refusal.test.ts index 272cd9681ac..5a261cbd0c0 100644 --- a/packages/services/service-automation/src/flow-inner-refusal.test.ts +++ b/packages/services/service-automation/src/flow-inner-refusal.test.ts @@ -25,7 +25,10 @@ * through does not lose them; a `fault` edge that handles the failure * leaves the run with none; a `map` item's or a `subflow` child's * refusal crosses that container (nested too), and a child that met no - * refusal carries none; + * refusal carries none; (#27743) a `try_catch` with no catch region + * carries its last failed attempt's refusal, and a `subflow` child that + * paused and fails on resume carries it to a caller holding either the + * parent's run id or the child's (the leg a `map` item takes); * 4. ⛔ `error` is byte-identical to the composition it always had — the * diagnostic channel does not move. * @@ -43,6 +46,7 @@ import { registerCrudNodes } from './builtin/crud-nodes.js'; import { registerLoopNode } from './builtin/loop-node.js'; import { registerMapNode } from './builtin/map-node.js'; import { registerSubflowNode } from './builtin/subflow-node.js'; +import { registerTryCatchNode } from './builtin/try-catch-node.js'; import { DuplicateRecordError, ValidationError } from '@objectstack/objectql'; import { PermissionDeniedError } from '@objectstack/plugin-security'; import { defineActionDescriptor } from '@objectstack/spec/automation'; @@ -498,3 +502,242 @@ describe('a `map` item and a `subflow` child carry the refusal across the contai expect(outer.error).toBe(`Node 'call' failed: subflow '${WRITER}' failed: ${childText(fault)}`); }); }); + +describe('#27743 — a `try_catch` with no catch region carries the guarded refusal', () => { + /** start → guard (try_catch, try = [mk], no catch) → end. */ + function guardedFlow(retry?: Record): any { + return { + name: 'guarded', label: 'Guarded', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'guard', type: 'try_catch', label: 'Guard', + config: { + try: { + nodes: [ + { id: 'mk', type: 'create_record', label: 'Create', config: { objectName: 'crm_account', fields: { name: 'Acme' } } }, + ], + edges: [], + }, + ...(retry ? { retry } : {}), + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'guard' }, + { id: 'e2', source: 'guard', target: 'end' }, + ], + }; + } + + function guardedEngine(data: any): AutomationEngine { + const engine = new AutomationEngine(makeLogger()); + const ctx = ctxWith(data); + registerCrudNodes(engine, ctx); + registerTryCatchNode(engine, ctx); + return engine; + } + + it('the region\'s refusal reaches the run — code and userMessage — and the text keeps its chain', async () => { + const err = markedHook(); + const engine = guardedEngine(refusingData(err)); + engine.registerFlow('guarded', guardedFlow()); + + const result = await engine.execute('guarded', { userId: 'u1' }); + + expect(result.status).toBe('failed'); + expect(result.innerRefusal).toEqual({ code: 'RESOURCE_CONFLICT', message: err.message }); + expect(result.userMessage).toBe(HOOK_USER_TEXT); + expect(result.error).toBe( + `Node 'guard' failed: try_catch 'guard': try region failed — ` + + `Node 'mk' failed: create_record(crm_account) failed: ${err.message}`, + ); + }); + + it('a retry ladder carries the LAST attempt\'s refusal, never an earlier one\'s', async () => { + const first = validation(); + const last = permission(); + let calls = 0; + const data = refusingData(undefined); + data.insert = async () => { calls += 1; throw calls === 1 ? first : last; }; + const engine = guardedEngine(data); + engine.registerFlow('guarded', guardedFlow({ maxRetries: 1, backoffMs: 0 })); + + const result = await engine.execute('guarded', { userId: 'u1' }); + + expect(calls).toBe(2); + expect(result.status).toBe('failed'); + expect(result.innerRefusal).toEqual({ code: 'PERMISSION_DENIED', message: last.message }); + }); + + it('control: a retry whose last attempt met a FAULT carries neither member, even after an earlier refusal', async () => { + let calls = 0; + const data = refusingData(undefined); + data.insert = async () => { + calls += 1; + throw calls === 1 ? validation() : new Error('ECONNREFUSED: could not reach the store'); + }; + const engine = guardedEngine(data); + engine.registerFlow('guarded', guardedFlow({ maxRetries: 1, backoffMs: 0 })); + + const result = await engine.execute('guarded', { userId: 'u1' }); + + expect(calls).toBe(2); + expect(result.status).toBe('failed'); + expect('innerRefusal' in result).toBe(false); + expect('userMessage' in result).toBe(false); + }); +}); + +describe('#27743 — a `map` item or `subflow` child that pauses and fails on resume', () => { + /** start → hold (pause) → mk (create_record) → end — the child that fails after its pause. */ + const PAUSING_WRITER = 'hold_then_write'; + + function pausingEngine(err: unknown): AutomationEngine { + const engine = new AutomationEngine(makeLogger()); + const ctx = ctxWith(refusingData(err)); + registerCrudNodes(engine, ctx); + registerMapNode(engine, ctx); + registerSubflowNode(engine, ctx); + engine.registerNodeExecutor({ + type: 'hold', + descriptor: defineActionDescriptor({ type: 'hold', version: '1.0.0', name: 'hold', supportsPause: true, resumeAuthority: 'any' }), + async execute() { return { success: true, suspend: true }; }, + } as NodeExecutor); + engine.registerNodeExecutor({ + type: 'seed', + async execute(_node, variables) { + variables.set('rows', [{ n: 1 }]); + return { success: true }; + }, + } as NodeExecutor); + engine.registerFlow(PAUSING_WRITER, { + name: PAUSING_WRITER, label: 'Hold then write', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'hold', type: 'hold', label: 'Hold' }, + { id: 'mk', type: 'create_record', label: 'Create', config: { objectName: 'crm_account', fields: { name: 'Acme' } } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'hold' }, + { id: 'e2', source: 'hold', target: 'mk' }, + { id: 'e3', source: 'mk', target: 'end' }, + ], + } as any); + return engine; + } + + function subflowFlow(name: string, flowName: string): any { + return { + name, label: name, type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'call', type: 'subflow', label: 'Call', config: { flowName } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'call' }, + { id: 'e2', source: 'call', target: 'end' }, + ], + }; + } + + function mapFlow(name: string, flowName: string): any { + return { + name, label: name, type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'query', type: 'seed', label: 'Seed' }, + { + id: 'each', type: 'map', label: 'Each', + config: { collection: { dialect: 'cel', source: 'rows' }, flowName, iteratorVariable: 'row' }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'query' }, + { id: 'e2', source: 'query', target: 'each' }, + { id: 'e3', source: 'each', target: 'end' }, + ], + }; + } + + const childRunIdOf = (engine: AutomationEngine) => + engine.listSuspendedRuns().find(r => r.flowName === PAUSING_WRITER)?.runId; + + it('subflow, resumed at the PARENT\'s run id (the delegated leg): the child\'s refusal reaches the caller', async () => { + const err = markedHook(); + const engine = pausingEngine(err); + engine.registerFlow('outer', subflowFlow('outer', PAUSING_WRITER)); + + const paused = await engine.execute('outer', { userId: 'u1' }); + expect(paused.status).toBe('paused'); + const childRunId = childRunIdOf(engine)!; + const result = await engine.resume(paused.runId!, {}); + + expect(result.success).toBe(false); + expect(result.innerRefusal).toEqual({ code: 'RESOURCE_CONFLICT', message: err.message }); + expect(result.userMessage).toBe(HOOK_USER_TEXT); + expect(result.error).toBe( + `subflow run '${childRunId}' (${PAUSING_WRITER}) failed: ` + + `Node 'mk' failed: create_record(crm_account) failed: ${err.message}`, + ); + }); + + it('subflow → subflow, resumed at the TOP run id: the refusal crosses both delegated frames', async () => { + const err = permission(); + const engine = pausingEngine(err); + engine.registerFlow('middle', subflowFlow('middle', PAUSING_WRITER)); + engine.registerFlow('outer', subflowFlow('outer', 'middle')); + + const paused = await engine.execute('outer', { userId: 'u1' }); + expect(paused.status).toBe('paused'); + const result = await engine.resume(paused.runId!, {}); + + expect(result.success).toBe(false); + expect(result.innerRefusal).toEqual({ code: 'PERMISSION_DENIED', message: err.message }); + }); + + it('subflow, resumed at the CHILD\'s run id (the up-bubble leg): the caller already gets the refusal', async () => { + const err = markedHook(); + const engine = pausingEngine(err); + engine.registerFlow('outer', subflowFlow('outer', PAUSING_WRITER)); + + const paused = await engine.execute('outer', { userId: 'u1' }); + expect(paused.status).toBe('paused'); + const result = await engine.resume(childRunIdOf(engine)!, {}); + + expect(result.success).toBe(false); + expect(result.status).toBe('stranded'); + expect(result.innerRefusal).toEqual({ code: 'RESOURCE_CONFLICT', message: err.message }); + expect(result.userMessage).toBe(HOOK_USER_TEXT); + }); + + it('map, its item resumed at the CHILD\'s run id (the bubble leg a `map` takes): the caller already gets the refusal', async () => { + const err = validation(); + const engine = pausingEngine(err); + engine.registerFlow('recalc', mapFlow('recalc', PAUSING_WRITER)); + + const paused = await engine.execute('recalc', { userId: 'u1' }); + expect(paused.status).toBe('paused'); + const result = await engine.resume(childRunIdOf(engine)!, {}); + + expect(result.success).toBe(false); + expect(result.status).toBe('stranded'); + expect(result.innerRefusal).toEqual({ code: 'VALIDATION_FAILED', message: 'Amount must be positive' }); + }); + + it('control: a child that fails on resume with a FAULT carries neither member on the delegated leg', async () => { + const engine = pausingEngine(new Error('ECONNREFUSED: could not reach the store')); + engine.registerFlow('outer', subflowFlow('outer', PAUSING_WRITER)); + + const paused = await engine.execute('outer', { userId: 'u1' }); + const result = await engine.resume(paused.runId!, {}); + + expect(result.success).toBe(false); + expect('innerRefusal' in result).toBe(false); + expect('userMessage' in result).toBe(false); + }); +}); diff --git a/packages/services/service-automation/src/inner-refusal.ts b/packages/services/service-automation/src/inner-refusal.ts index 5c32cdeb170..3a3603fbd5a 100644 --- a/packages/services/service-automation/src/inner-refusal.ts +++ b/packages/services/service-automation/src/inner-refusal.ts @@ -124,10 +124,27 @@ export function refusalCarriedBy(err: unknown): CarriedRefusal { * data node's and the refusal reaches the top run however deep it was met. * The child's result is the only source: a child that met no refusal spreads * `{}`, and a `map` stops at its first failing item, so it carries that - * item's. A `try_catch` with no catch region still builds a new error the - * brand does not cross; the run's result then carries no member, which the - * spec's "present only when a refusal was met" allows and which never - * invents one. + * item's. + * + * The other carriers (#27743), each measured at `execute` / `resume`: + * + * - A `try_catch` with NO catch region re-words its try region's failure into + * a new returned failure too, and carries the members its LAST failed + * attempt's throw carried (`readCarriedRefusal`), so a retry ladder reports + * the last attempt's refusal and never an earlier one's. + * - A `subflow` child that PAUSED and then fails on resume, resumed at the + * parent's run id (the delegated leg in `run-resume.ts`): the parent's + * re-worded failure carries the child's result's members + * (`carriedRefusalOfResult(childRes)`), through every delegated frame. + * - Resumed at the CHILD's run id instead (the up-bubble leg, and the only + * leg a `map` item takes), the caller already holds the child's own + * terminal result, which carries the members; the ancestors that leg fails + * record no member, and no caller reads their result. + * + * Not a carrier: a `try_catch` whose CATCH region itself fails builds its + * failure from the handler's error and carries no member; the run's result + * then carries none, which the spec's "present only when a refusal was met" + * allows and which never invents one. */ export const CARRIED_REFUSAL: unique symbol = Symbol.for( 'objectstack.automation.carriedRefusal', @@ -164,9 +181,10 @@ export function readCarriedRefusal(err: unknown): CarriedRefusal { /** * The members an attempt's RESULT carries — for `retryExecution`, whose * exhausted exit reports the last attempt's failure and must report that - * attempt's refusal beside it, never an earlier attempt's — and for the + * attempt's refusal beside it, never an earlier attempt's — for the * `subflow` / `map` failure arms, which carry their child run's refusal onto - * the failure they return. + * the failure they return, and for the delegated resume leg, which carries a + * paused-then-failed child's refusal onto the parent's. */ export function carriedRefusalOfResult(result: AutomationResult): CarriedRefusal { return pickCarried(result); diff --git a/packages/services/service-automation/src/run-resume.ts b/packages/services/service-automation/src/run-resume.ts index a963e52437d..51da355caca 100644 --- a/packages/services/service-automation/src/run-resume.ts +++ b/packages/services/service-automation/src/run-resume.ts @@ -21,7 +21,7 @@ import { RESUME_AUTHORITY_SERVICE } from '@objectstack/spec/contracts'; import { runIsUnscopedUserMode, flowTouchesData, runOrgUserIds, runTriggerUserId, type SeededRunContext } from './runtime-identity.js'; import { refuseNode } from './guard-refusal.js'; import { describeThrownForLog } from './thrown-cause-diagnostics.js'; -import { readCarriedRefusal } from './inner-refusal.js'; +import { carriedRefusalOfResult, readCarriedRefusal } from './inner-refusal.js'; import { summarizeRun } from './run-summary.js'; // The values below (`buildRunTrigger`, `FlowRefusalSignal`, `isRefusalSignal`, // `isSuspendSignal`, `isRetryableResumeRefusal`, `carriesSubflowChildOutput` @@ -1190,7 +1190,19 @@ export async function resumeInternal( error, this.consumedSuspensions.has(childRunId) ? childRunId : undefined, ); - return { success: false, error, durationMs: Date.now() - run.startTime, flowLabel: flow.label }; + // #27743 — the child's refusal, carried beside the + // re-worded `error` exactly as the synchronous `subflow` + // arm carries it (`inner-refusal.ts`): this is the leg a + // caller holding the PARENT's run id takes, and without it + // a refusal met after the child's pause reached that + // caller as text only. `{}` for a child that met none. + return { + success: false, + error, + durationMs: Date.now() - run.startTime, + flowLabel: flow.label, + ...carriedRefusalOfResult(childRes), + }; } // [#18714] DELEGATED-LEG REFUSAL. The child ran to a // refusing terminal — an `end` declaring