diff --git a/.changeset/retry-attempt-variable-environment.md b/.changeset/retry-attempt-variable-environment.md new file mode 100644 index 0000000000..ff9d56c130 --- /dev/null +++ b/.changeset/retry-attempt-variable-environment.md @@ -0,0 +1,25 @@ +--- +'@objectstack/service-automation': patch +--- + +fix(service-automation): a retry attempt now runs with the same variable environment as the first + +`executeWithoutRetry()` — the method the retry loop re-runs a flow through on every +attempt — seeded only the flow's declared variables and `$record`, while the first +attempt also binds `record` plus the triggering record's flattened fields, `previous`, +`$runId`, `$flowName` and `$flowLabel`. Every retry attempt therefore ran in a strictly +smaller environment than attempt 1. + +Because conditions are strict CEL, where reading an unbound name aborts the predicate +rather than yielding `false`, this was user-visible exactly where retry is most used — +`errorHandling.strategy: 'retry'` on a record-change flow: + +- a start condition or edge predicate reading `previous` (the create-vs-update + discriminator) aborted on the retry, so the retry failed for a reason the first attempt + never hit — reading as a flaky flow rather than a defect; +- a bare reference to a triggering-record field (`status`, `budget`) aborted for the same + reason; +- a pausing node (e.g. Approval) reached on a retry attempt saw no `$runId`, so the + external state it minted could not be mapped back to the run for resume (ADR-0019). + +Both methods now seed through one shared chokepoint. First-attempt behaviour is unchanged. diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 8453289e02..2d61de4d9c 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -3077,38 +3077,19 @@ export class AutomationEngine implements IAutomationService { // runaway it exists to stop. let reentryHeld = false; - // Initialize variable context - const variables = this.seedDeclaredVariables(flow, context); - // Inject trigger record. `$record` is the canonical handle; `record` is a - // friendlier alias so templates/conditions can write `{record.title}` and - // `record.status`. We also flatten the record's own fields to top-level - // variables (so bare references like `status`/`budget` resolve in start - // conditions and edge predicates) WITHOUT clobbering flow inputs already - // seeded above. `previous` exposes the pre-update row for transition gates. - if (context?.record) { - variables.set('$record', context.record); - variables.set('record', context.record); - for (const [k, v] of Object.entries(context.record)) { - if (!variables.has(k)) variables.set(k, v); - } - } - // Always bind `previous` — to `null` on the create/insert leg (there is no - // prior row) — so a start condition can DISCRIMINATE create vs update on a - // `record-after-write` flow: `previous == null` is the create leg (#3427). - // Binding only-when-truthy left `previous` an unknown CEL variable on - // insert, so ANY reference to it (even `previous == null`) threw - // "Unknown variable: previous" and failed the whole condition. - variables.set('previous', context?.previous ?? null); - + // Initialize the run's variable environment. Every binding a run is + // entitled to — the flow's declared variables, the trigger record and + // its flattened fields, `previous`, and the engine-owned `$runId` / + // `$flowName` / `$flowLabel` — is seeded by `seedRunVariables`, the one + // chokepoint `executeWithoutRetry` shares (#9704). See that helper for + // why the seeding is not written out here any more. + // + // The run id is minted BEFORE the seeding because `$runId` is part of + // that environment. Order-safe: `nextRunId()` is a stateless random id + // (no counter to advance), and nothing between the old call site and + // this one reads or mints one. const runId = this.nextRunId(); - // Expose the run id to executors (ADR-0019): a pausing node (e.g. Approval) - // reads `$runId` to map its external state back to this run for resume. - variables.set('$runId', runId); - // Expose flow identity to executors so externalized state (e.g. an - // approval request row) can carry a human-readable origin. Captured in - // the variable snapshot, so still present after a suspend/resume. - variables.set('$flowName', flowName); - variables.set('$flowLabel', flow.label ?? flowName); + const variables = this.seedRunVariables(flow, flowName, context, runId); const startedAt = new Date().toISOString(); const steps: StepLogEntry[] = []; @@ -6315,6 +6296,78 @@ export class AutomationEngine implements IAutomationService { return variables; } + /** + * Seed a run's COMPLETE variable environment — the one chokepoint every + * attempt of every run goes through (#9704). + * + * `seedDeclaredVariables` above turns the flow's own DECLARATIONS into + * bindings; this adds everything the ENGINE owns on top of them, in the + * order the precedence rules require: + * + * 1. declared variables (params, then `defaultValue`) — seeded first, so + * the record flattening below cannot shadow a flow input; + * 2. the trigger record: `$record` is the canonical handle, `record` a + * friendlier alias so templates and conditions can write + * `{record.title}` / `record.status`, plus the record's own fields + * flattened to top-level names so bare references (`status`, `budget`) + * resolve in start conditions and edge predicates — WITHOUT clobbering + * anything already bound; + * 3. `previous`, bound ALWAYS — to `null` on the create/insert leg, since + * there is no prior row — so a start condition can DISCRIMINATE create + * from update on a `record-after-write` flow: `previous == null` is the + * create leg (#3427). Binding it only when truthy left `previous` an + * unknown CEL variable on insert, so ANY reference to it (even + * `previous == null`) threw "Unknown variable: previous" and failed the + * whole condition; + * 4. `$runId`, so a pausing node (e.g. Approval) can map its external + * state back to this run for resume (ADR-0019), and `$flowName` / + * `$flowLabel`, so externalized state carries a human-readable origin. + * All three are captured in the variable snapshot, so they survive a + * suspend/resume. + * + * ⚠️ It is ONE method because the two callers drifting apart is the defect + * it repairs, not a tidiness preference. `execute()` seeded all of the + * above and `executeWithoutRetry()` — the method `retryExecution` re-runs + * the flow through on EVERY retry attempt — seeded only (1) and `$record`, + * so a retry attempt ran in a strictly smaller environment than the first + * one: conditions are strict CEL, where reading an unbound name ABORTS the + * predicate instead of yielding `false` (#4697), so the retry failed for a + * reason attempt 1 never hit, which reads as a flaky flow rather than a + * defect. The two methods had already drifted once per card on four + * separate exits (#9378, #9415, #9414, #9510) before this one, always in + * the same direction — the copy that is not `execute()` is the one a repair + * forgets. `buildRunTrigger` is the same chokepoint pattern, for the same + * reason. ⛔ So a change here belongs here: re-inlining either caller's copy + * re-opens the drift, and `retry-attempt-pause.test.ts` pins the two + * snapshots against EACH OTHER precisely so it cannot happen silently. + * + * The caller mints `runId` and passes it in rather than this helper minting + * one, because the run id is the caller's own bookkeeping: it keys the log + * row, the continuation and the returned envelope, and a helper that + * produced a second one would put a `$runId` in the snapshot that names no + * run anybody can resume. + */ + private seedRunVariables( + flow: FlowParsed, + flowName: string, + context: AutomationContext | undefined, + runId: string, + ): Map { + const variables = this.seedDeclaredVariables(flow, context); + if (context?.record) { + variables.set('$record', context.record); + variables.set('record', context.record); + for (const [k, v] of Object.entries(context.record)) { + if (!variables.has(k)) variables.set(k, v); + } + } + variables.set('previous', context?.previous ?? null); + variables.set('$runId', runId); + variables.set('$flowName', flowName); + variables.set('$flowLabel', flow.label ?? flowName); + return variables; + } + /** * Execute a flow without triggering retry logic (used by retryExecution to prevent recursion). * @@ -6351,12 +6404,19 @@ export class AutomationEngine implements IAutomationService { return { success: false, code: 'FLOW_DISABLED', error: `Flow '${flowName}' is disabled` }; } - const variables = this.seedDeclaredVariables(flow, context); - if (context?.record) { - variables.set('$record', context.record); - } - + // [#9704] The SAME environment attempt 1 runs in — seeded through the + // same chokepoint `execute()` uses. This method used to seed only the + // declared variables and `$record`, so every retry attempt ran in a + // strictly smaller environment than the first: `record` and its + // flattened fields, `previous`, `$runId`, `$flowName` and `$flowLabel` + // were all absent. Under strict CEL an unbound name ABORTS the + // predicate rather than yielding false (#4697), so a start condition or + // edge predicate reading `previous` (#3427) or a bare record field + // failed on the retry for a reason attempt 1 never hit — a flaky flow, + // to its author — and a pausing node on a retry attempt had no `$runId` + // to map its external state back to this run with (ADR-0019). const runId = this.nextRunId(); + const variables = this.seedRunVariables(flow, flowName, context, runId); const startedAt = new Date().toISOString(); const steps: StepLogEntry[] = []; diff --git a/packages/services/service-automation/src/retry-attempt-pause.test.ts b/packages/services/service-automation/src/retry-attempt-pause.test.ts index 9a8d6cde45..4ad629c8ee 100644 --- a/packages/services/service-automation/src/retry-attempt-pause.test.ts +++ b/packages/services/service-automation/src/retry-attempt-pause.test.ts @@ -273,26 +273,53 @@ describe('#9510 — a pause on a RETRY attempt is durable, not a burned attempt' expect(storedViaRetry?.nodeType).toBe(storedViaExecute?.nodeType); expect(storedViaRetry?.correlation).toBe(storedViaExecute?.correlation); - // ⚠️ The one thing that does NOT match, asserted rather than skirted. - // `executeWithoutRetry` seeds none of the engine-owned variables + // ⭐ [#9704] The variable ENVIRONMENT matches too — and this block is + // where the divergence used to be pinned as measured behaviour. + // `executeWithoutRetry` seeded none of the engine-owned variables // `execute()` binds (`$runId`, `$flowName`, `$flowLabel`, `record` and - // its flattened fields, `previous`), so a retry attempt has always run - // in a smaller environment than the first — filed as #9704, a divergent - // run environment rather than a lost pause, and out of scope here: it - // afflicts every retry attempt, pausing or not, and predates this card. + // its flattened fields, `previous`), so a retry attempt ran in a + // strictly SMALLER environment than the first: under strict CEL an + // unbound name ABORTS a predicate instead of yielding false (#4697), so + // a start condition or edge predicate reading `previous` (#3427) or a + // bare record field failed on the retry for a reason attempt 1 never + // hit, and a pausing node on a retry attempt saw no `$runId` to map its + // external state back with (ADR-0019). Both methods now seed through + // one helper (`seedRunVariables`), so the two snapshots are compared to + // EACH OTHER — the same discipline the result envelope above uses. // - // It is pinned as TODAY's measured behaviour, deliberately, so #9704 - // cannot be repaired silently. When it is repaired these three - // assertions are the ones that go red, and the correct edit is to - // delete them and add `variables` to the parity block above. - const engineOwned = ['$runId', '$flowName', '$flowLabel', 'previous', 'record']; - for (const name of engineOwned) { - expect(Object.keys(storedViaExecute?.variables ?? {})).toContain(name); - expect(Object.keys(storedViaRetry?.variables ?? {})).not.toContain(name); - } - // What the two DO share: the run's own work. Both snapshots carry the - // pausing node's inputs, so the continuation is a real continuation on - // either route — the half this card is about. + // `$runId` is per-run by nature, so it is asserted against the run id + // each route actually returned and then normalized for the comparison. + // Asserting it by VALUE is the point: a snapshot merely *carrying* a + // `$runId` that names a different run is the ADR-0019 mapping hole this + // card is about, and a presence-only check cannot see it. + expect(storedViaRetry?.variables?.$runId).toBe(viaRetry.runId); + expect(storedViaExecute?.variables?.$runId).toBe(viaExecute.runId); + const vars = (s: { variables: Record } | null) => ({ + ...(s?.variables ?? {}), + $runId: '', + }); + expect(vars(storedViaRetry)).toEqual(vars(storedViaExecute)); + + // …and the engine-owned bindings pinned by VALUE on the RETRY route, + // not merely by parity: the comparison above is equally satisfied if + // BOTH routes lose them, which is the shape a later "simplification" of + // the shared helper would take. + expect(storedViaRetry?.variables?.$flowName).toBe('flaky_approval'); + expect(storedViaRetry?.variables?.$flowLabel).toBe('flaky_approval'); + // `previous` is bound ALWAYS — to `null` on the create leg, since that + // is what lets a start condition discriminate create vs update (#3427). + // `toHaveProperty` rather than a `?.previous` read: the defect was the + // key being ABSENT, and absent and `null` both read as `null`. + expect(storedViaRetry?.variables).toHaveProperty('previous', null); + expect(storedViaRetry?.variables?.record).toEqual({ id: 'ord_9510', amount: 500 }); + // The trigger record's own fields flattened to top-level names — what + // makes a bare `amount` reference resolve on a retry attempt. + expect(storedViaRetry?.variables?.amount).toBe(500); + expect(storedViaRetry?.variables?.id).toBe('ord_9510'); + + // What the two shared even BEFORE the repair: the run's own work. Both + // snapshots carry the pausing node's inputs, so the continuation is a + // real continuation on either route — the half #9510 was about. expect(storedViaRetry?.variables?.['flaky.ok']).toEqual(storedViaExecute?.variables?.['flaky.ok']); expect(storedViaRetry?.variables?.$record).toEqual(storedViaExecute?.variables?.$record);