Skip to content

[finding] service-automation: the engine refreshes $error on the RETURNED-failure arm but only conditionally on the THROW arm, so $error is stale for any thrown failure inside a region — the root cause behind #14948's reproduced defect #14955

Description

@os-sales

Filed by the domain:services execution seat from a tier contract review of PR #14948 (#14419), which reproduced the consequence rather than inferring it. Observation class. Unassigned; domain:*, type and priority are triage's.

The asymmetry

In packages/services/service-automation/src/engine.ts, the run-wide $error variable is rewritten:

  • on the returned-failure arm — unconditionally (around :7167);
  • on the throw arm — only inside if (faultEdge) (around :7088).

A node inside a region never has a fault edge of its own: the region's synthetic sub-flow carries only the region's edges. ⇒ any thrown failure inside a region leaves $error holding the previous failure's value.

Why it was invisible until now

try_catch used to overwrite errorVariable wholesale with { nodeId, message } rebuilt from the caught exception's .message, so nothing stale could leak — the staleness was real but unobservable.

PR #14948 adds a code field that try_catch preserves from $error instead of rebuilding. That is the first consumer of $error's freshness, and it immediately met the asymmetry.

The measured consequence (from the review, on that PR's head)

Two flows, both run against the real engine:

  • loop { body: [ try_catch { try: [create_record, timeoutMs: 20], catch: [swallow-if-DUPLICATE_RECORD] } ] } — the spec's own canonical per-row containment spelling. Row 1's duplicate is swallowed correctly. Row 2's store hang was bound as { code: 'DUPLICATE_RECORD', message: "Node 'mk' timed out after 20ms" } and swallowed; the run reported success.
  • A(create_record dup, fault edge → recover) → try_catch { try: [B throws ECONNREFUSED] } — B bound as { code: 'DUPLICATE_RECORD', message: 'ECONNREFUSED…' } and swallowed.

The message and the code come from two different failures.

What is and is not being asked here

PR #14948 is taking the minimal identity guard (capture $error before the attempt; treat code as this attempt's only if the object identity changed) plus those two flows as pins. That closes the observable defect and is verified.

⛔ This card is not asking to re-litigate that. It records the underlying asymmetry, which the guard works around rather than removes, and which the next person to add a field to $error will meet in exactly the same way — with the same invisibility, because the failure mode is a plausible-looking wrong value rather than a crash.

Suggested direction (for triage, not a ruling)

The reviewer's alternative, recorded as the cleaner long-term shape: have the engine attach code to the Error it throws (around :7190) and have try_catch read it off the caught exception, so message and code are bound from the same failure by construction rather than by a freshness guard.

⚠️ That changes the engine's throw shape, which is a wider surface than a patch round should decide — which is why it was deliberately excluded from #14948 and filed here instead.

The more general question worth a decision: should the throw arm refresh $error unconditionally, matching the returned-failure arm? If there is a reason it is conditional on faultEdge, that reason is not recorded at the site and should be; if there is not, the asymmetry is the defect and the guard in #14948 becomes redundant.

Refs: PR #14948 / #14419 (where it was reproduced, and the guard that contains it) · the review's artefacts are named in its verdict comment on that PR

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions