fix(service-automation): seed a retry attempt's variable environment through the same chokepoint as attempt 1

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

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<string, unknown> {
+ 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[] = [];
 

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<string, unknown> } | null) => ({
+ ...(s?.variables ?? {}),
+ $runId: '<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);