fix(service-automation): a retry attempt that PAUSES is a durable pause, not a failed attempt — executeWithoutRetry gets the ADR-0019 suspend arm (#9510)

.changeset/retry-attempt-pause-suspend-arm.md

@@ -0,0 +1,67 @@
+---
+"@objectstack/service-automation": patch
+"@objectstack/runtime": patch
+---
+
+fix(service-automation): a retry attempt that PAUSES is a durable pause, not a failed attempt — `executeWithoutRetry` gets the ADR-0019 suspend arm (#9510)
+
+`execute()`'s catch tests the suspend signal FIRST, and that arm is what makes
+ADR-0019's durable pause work: it snapshots the live variables, calls
+`persistSuspendedRun`, records a `paused` log entry and returns
+`{ success: true, status: 'paused', runId }`.
+
+`executeWithoutRetry()` — the method `retryExecution` re-runs the flow through on
+**every** retry attempt — had no such arm. A `FlowSuspendSignal` thrown on a
+retry attempt fell into the generic failure path, and four things were lost at
+once:
+
+1. `persistSuspendedRun` never ran, so **the continuation was never stored** and
+ the run could not be resumed by anyone, ever;
+2. the run log recorded `failed` for a run that asked to pause;
+3. the caller got `status: 'failed'`, with the suspend signal stringified into
+ `error` (`FlowSuspendSignal` is not an `Error`);
+4. `retryExecution` reads only `result.success`, so the pause counted as one more
+ failed attempt: the loop burned the rest of the budget, and every further
+ attempt re-entered the pausing node and orphaned another suspension.
+
+Only a LATER attempt is exposed — `execute()` handles the first one correctly,
+and a flow reaches `retryExecution` only after a failure. The reachable shape is
+the ordinary one: `errorHandling.strategy: 'retry'` on a flow whose flaky
+HTTP/connector call is followed by an `approval` or `screen` node.
+
+**⚠️ Runs already lost to this defect are NOT recoverable.** Nothing was written
+for them — no `sys_automation_run` row, no in-memory suspension — so there is no
+continuation to rehydrate and no repair, here or later, can bring one back. The
+run log holds a `failed` entry naming the flow and the trigger; those runs have
+to be triggered again. What this change fixes is every run from here on.
+
+**The repair is a restoration of a stated contract on a path that never got it,
+not a new capability.** `AutomationResult.status: 'paused'` and ADR-0019 already
+describe exactly this behaviour, and `execute()`'s own arm already implements it;
+the retry path simply never received it. The alternative — refusing
+`strategy: 'retry'` combined with a pausing node at authoring time — was
+considered and rejected: it over-refuses (a pausing node can sit on a branch the
+retrying path never reaches), under-refuses (a pausing node behind a runtime
+condition is not statically decidable), and would ban the one combination authors
+most reasonably reach for.
+
+**The cost, and what was done about it.** Lifting the arm makes `retryExecution`
+able to return a NON-TERMINAL result, and both of its readers were taught the
+third state explicitly rather than left to a branch that happens to fall through:
+the retry loop returns a paused attempt because it PAUSED (tested on `status`,
+before the `success` check that means "this attempt succeeded"), and the trigger
+route answers it from its own arm. The retry accounting is untouched — a
+genuinely failing attempt still consumes one, `maxRetries` still bounds the loop,
+and the loop stops only because the attempt did not fail.
+
+**Both routes give one answer**, pinned as an equality rather than verified in
+isolation: a pause on attempt 1 and a pause on attempt 3 produce the same engine
+result and the same wire response, so no caller can tell which attempt paused.
+
+Two adjacent gaps were measured out of this work and filed rather than absorbed:
+a retry attempt runs with a smaller variable environment than the first (#9704),
+and a flow's declared retry policy stops applying once a run pauses (#9705) —
+the latter being the measured answer to "what happens to the retry budget when a
+paused run is resumed and then fails": neither inherited nor fresh, because the
+resume path has no retry loop at all. Both are pinned as today's behaviour so
+neither can change by accident.

packages/runtime/src/domains/automation-trigger-paused-run.test.ts

@@ -0,0 +1,167 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #9510 — the trigger door answers a PAUSED run as the third state, deliberately.
+ *
+ * The engine repair that lifted `execute()`'s ADR-0019 suspend arm into
+ * `executeWithoutRetry` gave `retryExecution` a NON-TERMINAL result to return: a
+ * retry attempt that reaches a pausing node now comes back as
+ * `{ success: true, status: 'paused', runId }` instead of being reported as a
+ * failed attempt with its continuation dropped. This door is one of the two
+ * readers that had only ever seen terminal results out of that path.
+ *
+ * What is pinned here is the door's READING, driven with scripted
+ * `AutomationResult`s so every arm is reachable without a real engine (the
+ * end-to-end sentence — that a real engine's two producers reach this door as
+ * ONE answer — is `@objectstack/verify`'s
+ * `automation-trigger-paused-run.test.ts`, and the engine-side equality is
+ * `service-automation`'s `retry-attempt-pause.test.ts`).
+ *
+ * Both spellings of the door are exercised for every arm, from one table, for
+ * the reason #9378's suite states: they share one context builder and one
+ * response mapper, and a test covering only the canonical spelling would let the
+ * legacy one — the one the SDK actually calls — drift unnoticed.
+ *
+ * ⚠️ The paused answer is deliberately IDENTICAL to the terminal-success one on
+ * the wire, so these assertions cannot be satisfied by "some 200". They pin the
+ * payload a caller resumes with: `status`, `runId`, `screen`, and the absence of
+ * any refusal envelope.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+
+import { HttpDispatcher } from '../http-dispatcher.js';
+import { classifyFlowRefusal, isPausedRun } from '../flow-dispatch-status.js';
+import type { AutomationResult } from '@objectstack/spec/contracts';
+
+const CTX = { request: {}, executionContext: { userId: 'user_1' } } as any;
+
+/** Both spellings of the same door. `path` takes the flow name. */
+const ROUTES: Array<{ label: string; path: (flow: string) => string }> = [
+ { label: 'POST /:name/trigger', path: (f) => `/${f}/trigger` },
+ { label: 'legacy POST /trigger/:name', path: (f) => `/trigger/${f}` },
+];
+
+function makeDispatcher(result: AutomationResult) {
+ const flows = new Map([['flaky_approval', { name: 'flaky_approval' }]]);
+ const execute = vi.fn(async (): Promise<AutomationResult> => result);
+ const getFlow = vi.fn(async (name: string) => flows.get(name) ?? null);
+ const services: Record<string, unknown> = { automation: { execute, getFlow } };
+ const resolve = (name: string) => services[name];
+ const kernel: any = {
+ getService: resolve,
+ getServiceAsync: async (name: string) => resolve(name),
+ context: { getService: resolve },
+ };
+ return new HttpDispatcher(kernel);
+}
+
+/**
+ * The engine's paused result, in the shape BOTH producers build it — the arm in
+ * `execute()`'s catch and the one restored to `executeWithoutRetry`. The two are
+ * byte-identical apart from the ids, which is the point: this door must not be
+ * able to tell which attempt paused.
+ */
+const PAUSED: AutomationResult = {
+ success: true,
+ status: 'paused',
+ runId: 'run_7f0a',
+ durationMs: 12,
+ screen: {
+ title: 'Approve the order',
+ fields: [{ name: 'verdict', type: 'text', label: 'Verdict' }],
+ } as AutomationResult['screen'],
+};
+
+describe('#9510 — a triggered run that PAUSED is answered as the third state', () => {
+ for (const route of ROUTES) {
+ it(`${route.label}: answers 200 carrying the runId the caller resumes with`, async () => {
+ const dispatcher = makeDispatcher(PAUSED);
+
+ const result = await dispatcher.handleAutomation(route.path('flaky_approval'), 'POST', {}, CTX);
+
+ expect(result.handled).toBe(true);
+ expect(result.response?.status).toBe(200);
+ expect(result.response?.body?.success).toBe(true);
+ // The run is ALIVE and parked. Without these two the caller has no
+ // way to continue it, which is the harm #9510 is about — a 200 that
+ // merely "looks fine" is not the contract.
+ expect(result.response?.body?.data?.status).toBe('paused');
+ expect(result.response?.body?.data?.runId).toBe('run_7f0a');
+ // The screen a screen-flow runner renders travels with it.
+ expect(result.response?.body?.data?.screen?.title).toBe('Approve the order');
+ // …and it is NOT dressed as a refusal: a paused run has no error
+ // envelope, no code, and nothing for a status-blind caller to read
+ // as a failure.
+ expect(result.response?.body?.error).toBeUndefined();
+ });
+
+ it(`${route.label}: a pause with no screen (approval, wait) is still the paused answer`, async () => {
+ // `screen` is a screen-flow field; an `approval` or `wait` node
+ // pauses without one, and the body a caller resumes with must be
+ // the same either way.
+ //
+ // ⚠️ Honest about its own reach: this door answers a pause and a
+ // terminal success IDENTICALLY on the wire — deliberately, since
+ // both are `200` plus the engine result — so no route-level
+ // assertion can tell which arm produced the response. What it pins
+ // is the PAYLOAD (`status`, `runId`, no refusal envelope); that the
+ // arm reads the lifecycle verdict rather than sniffing `screen` is
+ // pinned on `isPausedRun` itself, below.
+ const dispatcher = makeDispatcher({ success: true, status: 'paused', runId: 'run_b21', durationMs: 3 });
+
+ const result = await dispatcher.handleAutomation(route.path('flaky_approval'), 'POST', {}, CTX);
+
+ expect(result.response?.status).toBe(200);
+ expect(result.response?.body?.data?.status).toBe('paused');
+ expect(result.response?.body?.data?.runId).toBe('run_b21');
+ expect(result.response?.body?.error).toBeUndefined();
+ });
+ }
+});
+
+describe('#9510 — the shared dispatch table names the non-terminal state', () => {
+ it('classifies a paused run as no refusal at all', () => {
+ expect(classifyFlowRefusal('flaky_approval', PAUSED)).toBeUndefined();
+ });
+
+ it('reads the producer\'s lifecycle verdict, never the incidental fields', () => {
+ // `runId` and `screen` ride along on a pause; neither DEFINES it.
+ expect(isPausedRun(PAUSED)).toBe(true);
+ expect(isPausedRun({ success: true, runId: 'run_x' })).toBe(false);
+ expect(isPausedRun({ success: false, status: 'failed', error: 'boom' })).toBe(false);
+ expect(isPausedRun(undefined)).toBe(false);
+ expect(isPausedRun(null)).toBe(false);
+
+ // ⭐ The two cases that actually SEPARATE reading the verdict from
+ // sniffing a companion field — and the reason they are spelled out:
+ // every assertion above is satisfied by a predicate that returns
+ // `!!result.screen`, because a screen happens to accompany the paused
+ // fixture and to be absent from all the negatives. A pin that cannot
+ // fail against the tolerant-consumer shape PD #12 forbids is not
+ // pinning anything, so these two carry the sentence:
+ //
+ // - an `approval` or `wait` pause has NO screen and is still a pause;
+ expect(isPausedRun({ success: true, status: 'paused', runId: 'run_b21' })).toBe(true);
+ // - a screen on a result that is not parked does NOT make it one.
+ expect(isPausedRun({ success: true, status: 'completed', screen: PAUSED.screen })).toBe(false);
+ });
+
+ it('never promotes a paused run into the FLOW_FAILED row, even against the grain', () => {
+ // Defensive rather than reachable: no producer stamps this pair today.
+ // It states which field decides when they disagree — a LIVE suspended
+ // run, continuation persisted and waiting for a `resume()`, must not be
+ // reported to its caller as a run that failed. That is #9510's defect
+ // wearing transport clothing.
+ const contradictory = { success: false, status: 'paused', runId: 'run_c3' } as AutomationResult;
+
+ expect(classifyFlowRefusal('flaky_approval', contradictory)).toBeUndefined();
+ });
+
+ it('still classifies the terminal refusal rows — the new arm narrows nothing', () => {
+ expect(classifyFlowRefusal('f', { success: false, status: 'failed', error: 'boom' })?.code)
+ .toBe('FLOW_FAILED');
+ expect(classifyFlowRefusal('f', { success: false, code: 'FLOW_DISABLED' })?.status).toBe(409);
+ expect(classifyFlowRefusal('f', { success: false, code: 'FLOW_NO_START_NODE' })?.status).toBe(422);
+ });
+});

packages/runtime/src/domains/automation.ts

@@ -27,6 +27,7 @@ import {
  classifyFlowRefusal,
  flowIsUnknown,
  flowNotFoundMessage,
+ isPausedRun,
  FLOW_NOT_FOUND_STATUS,
 } from '../flow-dispatch-status.js';
 import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js';
@@ -489,6 +490,14 @@ function flowDefinitionRefusal(err: any): unknown {
  * | flow disabled | never dispatched | `409` `FLOW_DISABLED` |
  * | flow has no start node | never dispatched | `422` `FLOW_NO_START_NODE` |
  * | ran and failed (incl. the retry-strategy exits) | ran, rejected | `400` `FLOW_FAILED` |
+ * | ran and PAUSED (whichever attempt) | ran, suspended | `200` + `runId` / `screen` |
+ *
+ * [#9510] The last row is NON-TERMINAL and is answered by its own arm at the
+ * bottom of this function. The durable pause is not a refusal, and since the
+ * suspend arm was restored to the engine's retry path it arrives from two
+ * producers — `execute()`'s catch and `retryExecution` — which this door must
+ * NOT be able to tell apart. See that arm for why it is written separately from
+ * the terminal success it happens to answer identically.
  *
  * [#9446] **The table itself now lives in `../flow-dispatch-status.js`** — one
  * definition, read by this door and by `/actions` (`action-execution.ts`) —
@@ -586,6 +595,34 @@ async function respondToFlowTrigger(
  response: deps.error(refusal.message, refusal.status, { code: refusal.code, ...runDetails }),
  };
  }
+ // [#9510] THE THIRD STATE, answered deliberately. A run that dispatched and
+ // then SUSPENDED at a pausing node (ADR-0019) is neither refused nor
+ // finished: its continuation is persisted, and the `200` here carries the
+ // `runId` — and the `screen`, for a screen flow — that the caller continues
+ // it with at `POST /:name/runs/:runId/resume`, the door just below.
+ //
+ // The answer is unchanged from what this door has always given a paused
+ // run, and that IS the requirement rather than an accident of ordering: it
+ // must be the SAME answer a pause on the first attempt gets, because a
+ // pause on a retry attempt is the same user-visible situation reached by a
+ // different route. Two answers for one situation would replace #9510's LOST
+ // pause with an inconsistent one. Pinned as an equality between the two
+ // routes — engine-side in `service-automation`'s
+ // `retry-attempt-pause.test.ts`, and on the wire through a real engine in
+ // `@objectstack/verify`'s `automation-trigger-paused-run.test.ts`.
+ //
+ // ⛔ Its own arm even though it returns what the terminal exit below
+ // returns. The two are different STATEMENTS about the run — "still running,
+ // here is how to continue it" versus "it finished" — and collapsing them
+ // recreates exactly the fall-through this card is about: a non-terminal
+ // result that no reader on the path ever names is one edit away from being
+ // classified as a terminal one.
+ if (isPausedRun(result)) {
+ return { handled: true, response: deps.success(result) };
+ }
+ // Terminal success: the run reached an `end` node, and `deps.success` serves
+ // the engine result as the response data (`output`, `successMessage`,
+ // `summary`).
  return { handled: true, response: deps.success(result) };
 }
 
@@ -606,7 +643,9 @@ async function respondToFlowTrigger(
  * POST /:name/trigger → execute (legacy: trigger/:name also supported;
  * unknown name → 404, disabled → 409 `FLOW_DISABLED`,
  * no start node → 422 `FLOW_NO_START_NODE`, a run that
- * ran and failed → 400 `FLOW_FAILED`; #9378 + #9415)
+ * ran and failed → 400 `FLOW_FAILED`; #9378 + #9415;
+ * a run that PAUSED → 200 with `runId` / `screen`,
+ * on whichever attempt it paused — #9510)
  * POST /:name/toggle → toggleFlow (unknown name → 404, #7535)
  * GET /:name/runs → listRuns (query: limit, cursor — validated, #7300;
  * status — validated AND honoured, #7359)