From c79fdf02b0ccd898496d7e08281b0f886bf62de3 Mon Sep 17 00:00:00 2001 From: os-project-manager Date: Tue, 18 Aug 2026 22:41:52 +0000 Subject: [PATCH 1/2] fix(service-automation): a retry attempt that PAUSES is a durable pause, not a failed attempt (#9510) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `execute()`'s catch tests the suspend signal first, and that arm is what makes ADR-0019's durable pause work. `executeWithoutRetry()` — which `retryExecution` re-runs the flow through on every retry attempt — had no such arm, so a `FlowSuspendSignal` thrown on a retry attempt fell into the generic failure path: `persistSuspendedRun` never ran, so the continuation was never stored and the run could not be resumed by anyone; the run log recorded `failed`; the caller got `status: 'failed'`; and the retry loop, reading only `result.success`, counted the pause as one more failed attempt and burned the rest of the budget re-entering the pausing node. Lifts the suspend arm into `executeWithoutRetry`, and teaches both readers of the now non-terminal `retryExecution` result the third state deliberately: the retry loop returns a paused attempt because it paused, tested on `status` before the `success` check that means "this attempt succeeded"; the trigger route answers it from its own arm, off a named predicate on the shared flow-dispatch table. Retry accounting is untouched — a failing attempt still consumes one and `maxRetries` still bounds the loop. Both routes to a pause are pinned as an equality rather than verified in isolation, engine-side and end-to-end through a real dispatcher, so no caller can tell which attempt paused. Runs already lost to this defect are not recoverable: nothing was ever written for them. Refs #9414 / PR #9514 (the sibling repair on the same three methods), ADR-0019. Findings filed out of this work: #9704, #9705. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza --- .changeset/retry-attempt-pause-suspend-arm.md | 67 ++++ .../automation-trigger-paused-run.test.ts | 149 ++++++++ packages/runtime/src/domains/automation.ts | 41 ++- packages/runtime/src/flow-dispatch-status.ts | 58 ++- .../services/service-automation/src/engine.ts | 140 ++++++- .../src/retry-attempt-pause.test.ts | 346 ++++++++++++++++++ .../src/automation-trigger-paused-run.test.ts | 206 +++++++++++ 7 files changed, 1001 insertions(+), 6 deletions(-) create mode 100644 .changeset/retry-attempt-pause-suspend-arm.md create mode 100644 packages/runtime/src/domains/automation-trigger-paused-run.test.ts create mode 100644 packages/services/service-automation/src/retry-attempt-pause.test.ts create mode 100644 packages/verify/src/automation-trigger-paused-run.test.ts diff --git a/.changeset/retry-attempt-pause-suspend-arm.md b/.changeset/retry-attempt-pause-suspend-arm.md new file mode 100644 index 0000000000..485da43f97 --- /dev/null +++ b/.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. diff --git a/packages/runtime/src/domains/automation-trigger-paused-run.test.ts b/packages/runtime/src/domains/automation-trigger-paused-run.test.ts new file mode 100644 index 0000000000..80e649a16d --- /dev/null +++ b/packages/runtime/src/domains/automation-trigger-paused-run.test.ts @@ -0,0 +1,149 @@ +// 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 => result); + const getFlow = vi.fn(async (name: string) => flows.get(name) ?? null); + const services: Record = { 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. The state is read off `status`, so its + // absence must change nothing — a door that keyed on `screen` would + // report every approval pause as a terminal success. + 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. A + // reader that sniffed them would call a resume refusal (which also + // carries a run id) a live pause. + 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); + }); + + 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); + }); +}); diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 17c985538f..9ccf12279a 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/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) diff --git a/packages/runtime/src/flow-dispatch-status.ts b/packages/runtime/src/flow-dispatch-status.ts index 6a72b3bdba..30fd00c5dd 100644 --- a/packages/runtime/src/flow-dispatch-status.ts +++ b/packages/runtime/src/flow-dispatch-status.ts @@ -10,6 +10,25 @@ * | flow disabled | never dispatched | `409` `FLOW_DISABLED` | * | flow has no start node | never dispatched | `422` `FLOW_NO_START_NODE` | * | ran and failed | ran, rejected | `400` `FLOW_FAILED` | + * | ran and PAUSED | ran, suspended | not a refusal — see below | + * + * ## The fifth row is NON-TERMINAL, and it is written down on purpose (#9510) + * + * A run that reaches a pausing node suspends: its continuation is persisted + * (ADR-0019), it answers `{ success: true, status: 'paused', runId }`, and it + * finishes later through `resume()`. It is neither a refusal nor a completed + * run, and it is served as it always has been — the engine result on a `200`, + * carrying the `runId` (and `screen`) a caller continues it with. + * + * It earns a row because it stopped being a single-producer answer. Until #9510 + * only `execute()`'s own catch could pause a triggered run; `executeWithoutRetry` + * had no suspend arm, so a pause on a RETRY attempt was reported as a failed run + * with its continuation silently dropped. With that arm restored the same + * `paused` result now also arrives through `retryExecution` — same shape, second + * producer. A third state reaching a reader that knows two is how one defect + * becomes another, so every reader of this table is TOLD the state exists + * instead of discovering it by falling off the end of + * {@link classifyFlowRefusal}. * * ## Why the table is a module and not a mapper inside one route * @@ -119,9 +138,31 @@ export async function flowIsUnknown(automation: unknown, flowName: string): Prom } /** - * The three result-borne rows: which HTTP answer this engine result declares, - * or `undefined` when the producer classified nothing (see the module note on - * why that is deliberately not a row). + * The table's NON-TERMINAL row: a run that dispatched and then SUSPENDED at a + * pausing node (ADR-0019 durable pause) — `{ success: true, status: 'paused', + * runId }`, with the continuation persisted under that `runId` (#9510). + * + * Read by a door that wants to answer the third state deliberately instead of + * letting it fall through a terminal-shaped branch. It reads the producer's own + * lifecycle verdict — the same field {@link classifyFlowRefusal} reads and the + * same one the engine writes to the run log — never `runId` / `screen` / the + * message, which are incidental to the state. + * + * ⚠️ The ANSWER for a paused run stays each door's own, exactly as the envelope + * is: both trigger doors serve it as today's `200` plus the engine result. What + * is shared, and what this predicate is for, is the QUESTION — so no door can + * read a live pause as a terminal outcome while believing it implements this + * table. + */ +export function isPausedRun(result: AutomationResult | null | undefined): boolean { + return !!result && typeof result === 'object' && result.status === 'paused'; +} + +/** + * The three result-borne REFUSAL rows: which HTTP answer this engine result + * declares, or `undefined` when the producer classified nothing (see the module + * note on why that is deliberately not a row) — and `undefined` for a PAUSED + * run too, which is not a refusal at all (#9510). * * `flowName` is used ONLY to fill a row's default message when the producer * wrote none; it never affects the classification. @@ -130,7 +171,16 @@ export function classifyFlowRefusal( flowName: string, result: AutomationResult | null | undefined, ): FlowRefusal | undefined { - if (!result || typeof result !== 'object' || result.success !== false) return undefined; + if (!result || typeof result !== 'object') return undefined; + // [#9510] A live suspended run is never a refusal, said by naming the state + // rather than by relying on a paused result also being `success: true`. + // ⚠️ Honest about its own weight: this changes no answer today — the + // `success` line below already returns `undefined` for it. What it buys is + // that the non-terminal state is NAMED in the one function every door reads + // the table through, so a future row for unclassified `success: false` + // refusals cannot capture a paused run on its way past. + if (isPausedRun(result)) return undefined; + if (result.success !== false) return undefined; const message = typeof result.error === 'string' && result.error ? result.error : undefined; // ── never dispatched: the producer says WHICH refusal (#9415) ────────── diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index c51fdec4bd..8453289e02 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -3329,7 +3329,23 @@ export class AutomationEngine implements IAutomationService { error: errorMessage, }); - // Error handling strategy + // Error handling strategy. + // + // [#9510] ⚠️ This handoff can now return a NON-TERMINAL result: a + // retry attempt that reaches a pausing node comes back as + // `{ success: true, status: 'paused', runId }` — the same shape the + // suspend arm above returns — and `execute()` forwards it + // unchanged, so its contract is ONE answer per situation rather + // than one per attempt number. Deliberate, not incidental: a caller + // branching on `status` sees no difference between a pause on + // attempt 1 and a pause on attempt 3, which is what + // `AutomationResult` already promises. + // + // ⚠️ The `runId` that comes back is then the PAUSED ATTEMPT's, not + // this failed first attempt's. They are different runs with + // different log rows, and the paused one is the key + // `persistSuspendedRun` stored the continuation under — so it is + // the only id `resume()` can be called with. if (flow.errorHandling?.strategy === 'retry') { return this.retryExecution(flowName, context, startTime, flow.errorHandling, flow.errorMessage); } @@ -6138,6 +6154,17 @@ export class AutomationEngine implements IAutomationService { * Retry execution with exponential backoff, jitter, and recursive protection. * Uses an iterative loop with an internal retry flag to prevent recursive call stacking. * + * ⚠️ [#9510] NOT a terminal-only method. Three results leave it: an attempt + * that succeeded, the exhausted-budget failure at the bottom, and — since + * the ADR-0019 suspend arm was restored to `executeWithoutRetry` — an + * attempt that PAUSED, returned unchanged from inside the loop. The paused + * one is non-terminal: the run is durably suspended and finishes later + * through `resume()`, not here. Its readers are told so explicitly (the + * loop below, `execute()`'s handoff, and the flow-dispatch status table in + * `@objectstack/runtime`) rather than discovering it by falling through a + * `success` check — a non-terminal result reaching a reader that assumes + * terminal is how this defect becomes a different one. + * * Reads the PARSED `errorHandling` block straight — no `??` fallbacks * (#4247). It used to declare every knob optional and re-state a default * for each, and one of those copies disagreed with the schema @@ -6191,6 +6218,29 @@ export class AutomationEngine implements IAutomationService { // Execute directly without recursion into retryExecution again const result = await this.executeWithoutRetry(flowName, context); + // [#9510] THE THIRD STATE, read deliberately and BEFORE `success`. + // Since the ADR-0019 suspend arm was restored to + // `executeWithoutRetry`, an attempt can end NON-TERMINAL: the run + // asked to pause, its continuation is persisted under + // `result.runId`, and it is waiting for a `resume()` that may + // arrive days later. Returning it is the only correct answer — + // re-running the flow would start the paused work a SECOND time + // while the first continuation is still live and resumable, which + // is what the pre-repair loop did on every remaining attempt. + // + // ⛔ Not left to the `result.success` line below, even though a + // paused result is `success: true` and would fall through it today. + // That line means "this attempt SUCCEEDED, stop retrying" — a + // different sentence about a different outcome — and a + // non-terminal result recognised only by a branch that names + // something else is one edit away from being misread again. The + // reason this returns is the PAUSE, so the pause is what it tests. + // + // ⛔ And it weakens no retry accounting: nothing above is skipped, + // reset or shortened, `maxRetries` still bounds the loop, and a + // genuinely failing attempt still consumes one. The loop stops here + // only because this attempt did not fail. + if (result.status === 'paused') return result; if (result.success) return result; lastError = result.error ?? 'Unknown error'; } @@ -6267,6 +6317,13 @@ export class AutomationEngine implements IAutomationService { /** * Execute a flow without triggering retry logic (used by retryExecution to prevent recursion). + * + * [#9510] It exits the way `execute()` exits, and that parity is the + * contract rather than a coincidence: a run that suspends here is a durable + * ADR-0019 pause — continuation persisted, `paused` log row — not a failed + * attempt, because it is the SAME event happening on a later attempt. Two + * answers for one user-visible situation is what this method's missing + * suspend arm produced. */ private async executeWithoutRetry( flowName: string, @@ -6351,6 +6408,87 @@ export class AutomationEngine implements IAutomationService { // work — the same route-dependent shape the fix is removing. return { success: true, output, durationMs, successMessage: flow.successMessage, summary: logged.summary }; } catch (err: unknown) { + // [#9510] A node asked to suspend the run (ADR-0019 durable pause) + // — here, on a RETRY attempt, the only way this method is ever + // reached. Tested FIRST and answered exactly as `execute()`'s own + // catch answers it, because it is the same event: A PAUSE IS NOT A + // FAILURE (`execute()`'s arm says so in those words, and ADR-0019 + // is the contract). A restoration of a stated contract on the one + // path that never got it — not a new capability, and the + // `AutomationResult.status` vocabulary it returns is unchanged. + // + // Without this arm the signal fell through to the generic failure + // path below and four halves of the pause were lost at once: + // + // 1. `persistSuspendedRun` never ran, so THE CONTINUATION WAS + // NEVER STORED and the run could not be resumed by anyone, + // ever — the headline harm, and why the pins for this are + // written against the store and a real `resume()` rather than + // against the status string; + // 2. the run log recorded `failed` for a run that asked to pause; + // 3. the caller got `status: 'failed'`, with the signal + // stringified into `error` (`FlowSuspendSignal` is not an + // `Error`); + // 4. `retryExecution` reads `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. + // + // ⛔ Deliberately NOT an authoring-time refusal of + // `errorHandling.strategy: 'retry'` combined with a pausing node. + // That refusal 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 ordinary shape this defect is reached through: a + // flaky HTTP/connector call followed by an `approval` or `screen` + // node — retry is what an author reaches for on the flaky half and + // the approval is what the business needs on the other. + // + // Every field is THIS attempt's own run bookkeeping (`runId`, + // `startedAt`, `steps`, `runContext`, `startTime`), never the + // failed attempt's, so the continuation is keyed by the run id this + // method returns — the only id `resume()` can be called with. + if (isSuspendSignal(err)) { + const durationMs = Date.now() - startTime; + // #7639 — ONE snapshot expression feeding BOTH consumers: the + // continuation the run will resume from, and the `paused` log + // entry run-detail serves. Same object, so what an operator + // reads can never disagree with what the run holds. + const variablesSnapshot = Object.fromEntries(variables); + await this.persistSuspendedRun({ + runId, + flowName, + flowVersion: flow.version, + nodeId: err.nodeId, + nodeType: err.nodeType, + variables: variablesSnapshot, + steps, + context: runContext, + startedAt, + startTime, + correlation: err.correlation, + screen: err.screen, + }); + this.recordLog({ + id: runId, + flowName, + flowVersion: flow.version, + status: 'paused', + startedAt, + durationMs, + trigger: buildRunTrigger(context), + steps, + variables: variablesSnapshot, + }); + return { + success: true, + status: 'paused', + runId, + durationMs, + screen: err.screen, + }; + } + const errorMessage = err instanceof Error ? err.message : String(err); const durationMs = Date.now() - startTime; const logged = this.recordLog({ diff --git a/packages/services/service-automation/src/retry-attempt-pause.test.ts b/packages/services/service-automation/src/retry-attempt-pause.test.ts new file mode 100644 index 0000000000..9a8d6cde45 --- /dev/null +++ b/packages/services/service-automation/src/retry-attempt-pause.test.ts @@ -0,0 +1,346 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #9510 — a retry attempt that PAUSES is a durable pause, not a failed attempt. + * + * `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, so 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 — the headline harm, and the + * reason the pins below are written against the STORE and a real `resume()` + * rather than against the status string. A repair that flipped `status` to + * `'paused'` and still dropped the continuation would satisfy a + * status-only assertion and leave the defect exactly where it was. + * 2. the run log recorded `failed` for a run that asked to pause; + * 3. the caller got `status: 'failed'` — with the signal stringified into + * `error`, since `FlowSuspendSignal` is not an `Error`; + * 4. `retryExecution` reads `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. + * + * ## Reachability, and why the fixture is shaped the way it is + * + * Only a LATER attempt is exposed — `execute()` handles the first one correctly, + * and a flow reaches `retryExecution` only after a failure. So the fixture is + * the ordinary shape the card names: a flaky call (`flaky`) followed by a + * pausing node (`gate`). `failFirstAttempts` decides which attempt reaches the + * gate, and that single knob is what makes the defect DETERMINISTIC rather than + * timing-dependent: `failFirstAttempts: 1` forces the pause onto attempt 2 every + * run, `failFirstAttempts: 0` puts the identical pause on attempt 1, through + * `execute()`'s own arm. The two are the same user-visible situation reached by + * two routes, which is why they are pinned AGAINST EACH OTHER below rather than + * each in isolation. + * + * The wire half — that both routes reach the trigger door as one answer — is + * pinned end-to-end in `@objectstack/verify`'s + * `automation-trigger-paused-run.test.ts`; the door's own reading of the third + * state is pinned in `@objectstack/runtime`'s + * `automation-trigger-paused-run.test.ts`. + */ + +import { describe, it, expect } from 'vitest'; + +import { AutomationEngine } from './engine.js'; +import { InMemorySuspendedRunStore } from './suspended-run-store.js'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; +import type { AutomationContext, AutomationResult } from '@objectstack/spec/contracts'; + +const silent = { info() {}, warn() {}, error() {}, debug() {}, child: () => silent } as never; + +/** + * `resumeAuthority: 'any'` is required of a pausing fixture since #5561 — these + * tests continue their pause through the public `resume` door. Nothing here is + * about the resume gate (`resume-authority-gate.test.ts` owns that). + */ +const pauser = defineActionDescriptor({ + type: 'gate', + version: '1.0.0', + name: 'gate', + supportsPause: true, + resumeAuthority: 'any', +}); + +interface Harness { + engine: AutomationEngine; + store: InMemorySuspendedRunStore; + /** How many times each node's executor actually ran. */ + calls: { flaky: number; gate: number; after: number }; + trigger(): Promise; +} + +/** + * start -> flaky -> gate (pauses) -> after -> end, under + * `errorHandling.strategy: 'retry'`. + * + * `failFirstAttempts` fails `flaky` on exactly that many leading attempts, so + * the attempt the `gate` is reached on is chosen by the caller, not by timing. + * `afterFails` fails the node BEHIND the pause, which is only ever reached + * through `resume()` — it is how the retry-budget question is measured. + */ +function bootFlow(opts: { + failFirstAttempts: number; + afterFails?: boolean; + maxRetries?: number; +}): Harness { + const store = new InMemorySuspendedRunStore(); + const engine = new AutomationEngine(silent, store); + const calls = { flaky: 0, gate: 0, after: 0 }; + + engine.registerNodeExecutor({ + type: 'flaky', + async execute() { + calls.flaky++; + return calls.flaky <= opts.failFirstAttempts + ? { success: false, error: 'connector 503' } + : { success: true, output: { ok: true } }; + }, + } as never); + + engine.registerNodeExecutor({ + type: 'gate', + descriptor: pauser, + async execute() { + calls.gate++; + return { success: true, suspend: true, correlation: 'approval:req-1' }; + }, + } as never); + + engine.registerNodeExecutor({ + type: 'after', + async execute() { + calls.after++; + return opts.afterFails + ? { success: false, error: 'post-approval write rejected' } + : { success: true, output: { done: true } }; + }, + } as never); + + engine.registerFlow('flaky_approval', { + name: 'flaky_approval', + label: 'flaky_approval', + type: 'autolaunched', + errorHandling: { strategy: 'retry', maxRetries: opts.maxRetries ?? 2, backoffMs: 0 }, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'flaky', type: 'flaky', label: 'Flaky call' }, + { id: 'gate', type: 'gate', label: 'Approval' }, + { id: 'after', type: 'after', label: 'After approval' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'flaky' }, + { id: 'e1', source: 'flaky', target: 'gate' }, + { id: 'e2', source: 'gate', target: 'after' }, + { id: 'e3', source: 'after', target: 'end' }, + ], + } as never); + + return { + engine, + store, + calls, + trigger: () => engine.execute('flaky_approval', { + event: 'test', + object: 'crm_order', + record: { id: 'ord_9510', amount: 500 }, + } as unknown as AutomationContext), + }; +} + +describe('#9510 — a pause on a RETRY attempt is durable, not a burned attempt', () => { + it('stores the continuation and the run is genuinely resumable — the durable half', async () => { + const h = bootFlow({ failFirstAttempts: 1 }); + + const res = await h.trigger(); + + // Attempt 1 failed at `flaky`; attempt 2 got past it and paused at `gate`. + expect(h.calls.flaky).toBe(2); + expect(h.calls.gate).toBe(1); + expect(res.status).toBe('paused'); + expect(res.runId).toBeDefined(); + + // ⭐ THE assertion this card is about: the continuation EXISTS. Before + // the repair nothing was written here at all, so the run was + // unresumable for good — no status string, message or duration says + // that, which is why the pin is on the store. + const stored = await h.store.load(res.runId as string); + expect(stored).not.toBeNull(); + expect(stored?.runId).toBe(res.runId); + expect(stored?.flowName).toBe('flaky_approval'); + expect(stored?.nodeId).toBe('gate'); + // The resume gate's key (#3801) — a continuation without it is resumable + // only by falling back to the live flow definition. + expect(stored?.nodeType).toBe('gate'); + expect(stored?.correlation).toBe('approval:req-1'); + // The snapshot is the RUN's own state, not an empty husk: what the + // pre-pause node actually produced is in it, under `.`. + expect(stored?.variables?.['flaky.ok']).toBe(true); + expect(stored?.variables?.$record).toEqual({ id: 'ord_9510', amount: 500 }); + + // …and "resumable" asserted by DOING it, not by inspecting a row: the + // run continues from `gate`'s out-edge and reaches `end`. + const resumed = await h.engine.resume(res.runId as string, { output: { verdict: 'approved' } } as never); + expect(resumed.success).toBe(true); + expect(resumed.status).not.toBe('paused'); + expect(h.calls.after).toBe(1); + // The suspension is consumed, exactly as a first-attempt pause's is. + expect(await h.store.load(res.runId as string)).toBeNull(); + }); + + it('records the run log as paused, never failed', async () => { + const h = bootFlow({ failFirstAttempts: 1 }); + + const res = await h.trigger(); + + // A run log that says a paused run failed is a second lie on the same + // event: the run is alive, parked and waiting for a human. + const run = await h.engine.getRun(res.runId as string); + expect(run?.status).toBe('paused'); + expect(run?.status).not.toBe('failed'); + // #7639 — ONE snapshot expression feeds both consumers, on this path + // too: what an operator reads on run-detail is by construction the + // state the continuation will resume from, never a second capture. + const stored = await h.store.load(res.runId as string); + expect(run?.variables).toEqual(stored?.variables); + }); + + it('stops the retry loop as a CONSEQUENCE of the pause — the accounting is untouched', async () => { + const h = bootFlow({ failFirstAttempts: 1, maxRetries: 2 }); + + const res = await h.trigger(); + + expect(res.status).toBe('paused'); + // The budget allowed 1 initial attempt + 2 retries = 3 runs of `flaky`. + // Only 2 happened, and the pausing node was entered exactly ONCE: the + // loop stopped because the attempt did not fail, not because anything + // stopped counting. Re-running the flow here would have started the + // approval a second time while the first continuation was still live — + // the pre-repair behaviour, which orphaned one suspension per attempt. + expect(h.calls.flaky).toBe(2); + expect(h.calls.gate).toBe(1); + }); + + it('still burns the full budget when attempts genuinely FAIL — the loop was not weakened', async () => { + // The guard against "make the loop stop" being implemented by deleting + // retry accounting: with the gate never reached, the count is the #4247 + // contract exactly — maxRetries + 1 attempts. + const h = bootFlow({ failFirstAttempts: 99, maxRetries: 2 }); + + const res = await h.trigger(); + + expect(res.success).toBe(false); + expect(res.status).toBe('failed'); + expect(h.calls.flaky).toBe(3); + expect(h.calls.gate).toBe(0); + }); + + it('answers a pause on attempt 2 exactly as it answers one on attempt 1 — the two producers pinned against each other', async () => { + // Same flow, same pausing node, same trigger. The ONLY difference is + // which attempt reaches the gate — i.e. which engine arm produced the + // pause: `execute()`'s catch, or the one restored to + // `executeWithoutRetry`. One user-visible situation must not have two + // answers, so the two results are compared to EACH OTHER rather than + // each to a hand-written expectation. + const first = bootFlow({ failFirstAttempts: 0 }); + const retry = bootFlow({ failFirstAttempts: 1 }); + + const viaExecute = await first.trigger(); + const viaRetry = await retry.trigger(); + + // Same producer arm reached by two routes: same keys, in the same shape. + expect(Object.keys(viaRetry).sort()).toEqual(Object.keys(viaExecute).sort()); + // …and the same values, except the two that are per-run by nature. + const shape = (r: AutomationResult) => ({ ...r, runId: '', durationMs: 0 }); + expect(shape(viaRetry)).toEqual(shape(viaExecute)); + expect(viaRetry.success).toBe(true); + expect(viaRetry.status).toBe('paused'); + + // The stored continuations match too — a matching return value over a + // missing continuation is the failure this card is about. + const storedViaExecute = await first.store.load(viaExecute.runId as string); + const storedViaRetry = await retry.store.load(viaRetry.runId as string); + expect(storedViaRetry).not.toBeNull(); + expect(Object.keys(storedViaRetry ?? {}).sort()).toEqual(Object.keys(storedViaExecute ?? {}).sort()); + expect(storedViaRetry?.nodeId).toBe(storedViaExecute?.nodeId); + 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 + // `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. + // + // 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. + expect(storedViaRetry?.variables?.['flaky.ok']).toEqual(storedViaExecute?.variables?.['flaky.ok']); + expect(storedViaRetry?.variables?.$record).toEqual(storedViaExecute?.variables?.$record); + + // The run LOG agrees on both routes as well. + expect((await retry.engine.getRun(viaRetry.runId as string))?.status) + .toBe((await first.engine.getRun(viaExecute.runId as string))?.status); + }); + + /** + * ⭐ The retry-budget question the ruling required to be ANSWERED rather + * than assumed, measured rather than reasoned: + * + * **A resumed run gets NO retries — on either route.** + * + * Two independent facts produce that answer, both read off `origin/main`: + * + * - `SuspendedRun` (engine.ts) declares no attempt/retry field of any kind, + * so the continuation CANNOT carry attempt state; and + * - `resumeInternal` never reads `flow.errorHandling` and never enters + * `retryExecution`, so a resumed run that fails is terminal. + * + * So the answer is neither "inherits the remaining attempts" nor "starts + * fresh": the retry budget does not survive a pause at all. This is + * PRE-EXISTING behaviour of every paused run, not something this card + * introduces — which is exactly why lifting the arm is safe here: the + * retry-path pause inherits the same answer the execute-path pause has + * always had, and the two stay consistent. + * + * That the resume path ignores a flow's declared retry policy is a real + * gap, but a DIFFERENT one, on a different method; it is filed as #9705 + * rather than absorbed here. What is pinned below is today's measured + * answer, so whatever that card decides is a deliberate change and not an + * accident. + */ + it('answers the retry-budget question: a resumed run does not retry, on either route', async () => { + for (const failFirstAttempts of [0, 1]) { + const h = bootFlow({ failFirstAttempts, afterFails: true, maxRetries: 2 }); + + const paused = await h.trigger(); + expect(paused.status).toBe('paused'); + + const resumed = await h.engine.resume(paused.runId as string, { output: { verdict: 'approved' } } as never); + + // The post-pause node rejected. The run is terminally failed and + // the node ran ONCE: no attempt was inherited and none was granted + // fresh, because `resumeInternal` has no retry path at all. + expect(resumed.success).toBe(false); + expect(h.calls.after).toBe(1); + } + }); +}); diff --git a/packages/verify/src/automation-trigger-paused-run.test.ts b/packages/verify/src/automation-trigger-paused-run.test.ts new file mode 100644 index 0000000000..f3c5020fb4 --- /dev/null +++ b/packages/verify/src/automation-trigger-paused-run.test.ts @@ -0,0 +1,206 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #9510 — a run that pauses on a RETRY attempt reaches the wire as a live, + * resumable pause, and as the SAME answer a first-attempt pause gets. + * + * This is the end-to-end half, and it lives here because `@objectstack/verify` + * is the one package depending on BOTH `@objectstack/runtime` (the trigger and + * resume doors) and `@objectstack/service-automation` (the engine that produces + * the result) — the same reason #9414's `automation-trigger-terminal-messages` + * suite sits beside it. The engine-side pins are in + * `packages/services/service-automation/src/retry-attempt-pause.test.ts`; the + * door-side pins, driven with scripted results, are in + * `packages/runtime/src/domains/automation-trigger-paused-run.test.ts`. Neither + * of those, alone or together, can make the sentence this file makes: the two + * ENGINE PRODUCERS reach the door as one wire answer, and the run a caller is + * handed can actually be continued. + * + * **The requirement is an equality, so it is written as one.** The ruling on + * this card is explicit that the trigger route's answer for a paused retry must + * match what `execute()`'s paused return already produces, *pinned against it + * rather than verified in isolation* — because two paths answering differently + * for one user-visible situation would replace a LOST pause with an inconsistent + * one. So the two responses below are compared to EACH OTHER, and the only + * fields normalised away are the two that are per-run by nature. + * + * **And a matching answer is not the whole contract.** The headline harm was + * that `persistSuspendedRun` never ran, so the continuation was never stored and + * the run could not be resumed by anyone. A repair that returned the right JSON + * over a missing continuation would satisfy an equality and leave that intact — + * so the paused-on-retry run is also CONTINUED here, through the real + * `POST /:name/runs/:runId/resume` door, and the run log is read back through + * the real run-detail door. + * + * ⚠️ This suite resolves both packages through their BUILT `dist/`, as every + * dependent of theirs in this workspace does. Rebuild + * `@objectstack/service-automation` and `@objectstack/runtime` before trusting a + * run of this file — and especially before trusting an ABLATED one, where a + * stale `dist` would run the pre-mutation code and report green over a mutation + * that never reached the artifact. + */ + +import { describe, it, expect } from 'vitest'; + +import { HttpDispatcher } from '@objectstack/runtime'; +import { AutomationEngine, InMemorySuspendedRunStore } from '@objectstack/service-automation'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; + +const CTX = { request: {}, executionContext: { userId: 'user_1' } } as never; + +function createTestLogger(): never { + const logger = { info: () => {}, warn: () => {}, error: () => {}, debug: () => {}, child: () => logger }; + return logger as never; +} + +/** + * `resumeAuthority: 'any'` because this suite continues its pause through the + * PUBLIC resume door; since #5561 a node type declaring none is gated shut. + */ +const gate = defineActionDescriptor({ + type: 'gate', + version: '1.0.0', + name: 'gate', + supportsPause: true, + resumeAuthority: 'any', +}); + +/** + * start -> flaky -> gate (pauses) -> after -> end, under + * `errorHandling.strategy: 'retry'` — the ordinary shape the card names: a + * flaky connector call followed by an approval. + * + * `failFirstAttempts` alone decides WHICH engine arm produces the pause, which + * is what makes the comparison below an apples-to-apples one: `0` pauses on + * attempt 1 through `execute()`'s own catch, `1` pauses on attempt 2 through the + * arm restored to `executeWithoutRetry`. Nothing else differs. + */ +function boot(failFirstAttempts: number): HttpDispatcher { + const engine = new AutomationEngine(createTestLogger(), new InMemorySuspendedRunStore()); + let flakyCalls = 0; + + engine.registerNodeExecutor({ + type: 'flaky', + async execute() { + flakyCalls++; + return flakyCalls <= failFirstAttempts + ? { success: false, error: 'connector 503' } + : { success: true, output: { ok: true } }; + }, + } as never); + engine.registerNodeExecutor({ + type: 'gate', + descriptor: gate, + async execute() { + return { success: true, suspend: true, correlation: 'approval:req-1' }; + }, + } as never); + engine.registerNodeExecutor({ + type: 'after', + async execute() { + return { success: true, output: { booked: true } }; + }, + } as never); + + engine.registerFlow('flaky_approval', { + name: 'flaky_approval', + label: 'flaky_approval', + type: 'autolaunched', + errorHandling: { strategy: 'retry', maxRetries: 2, backoffMs: 0 }, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'flaky', type: 'flaky', label: 'Flaky call' }, + { id: 'gate', type: 'gate', label: 'Approval' }, + { id: 'after', type: 'after', label: 'After approval' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'flaky' }, + { id: 'e1', source: 'flaky', target: 'gate' }, + { id: 'e2', source: 'gate', target: 'after' }, + { id: 'e3', source: 'after', target: 'end' }, + ], + } as never); + + const services: Record = { automation: engine }; + const resolve = (name: string): unknown => services[name]; + const kernel = { + getService: resolve, + getServiceAsync: async (name: string): Promise => resolve(name), + context: { getService: resolve }, + }; + return new HttpDispatcher(kernel as never); +} + +const trigger = (d: HttpDispatcher) => d.handleAutomation('/flaky_approval/trigger', 'POST', {}, CTX); + +/** The two fields that are per-run by nature and cannot be compared literally. */ +function normalise(body: any) { + return { ...body, data: { ...body?.data, runId: '', durationMs: 0 } }; +} + +describe('#9510 — a paused retry attempt reaches the wire as a live, resumable pause', () => { + it('answers 200 with the runId a caller resumes with, when the pause lands on attempt 2', async () => { + const dispatcher = boot(1); + + const res = await trigger(dispatcher); + + expect(res.response?.status).toBe(200); + expect(res.response?.body?.success).toBe(true); + // Before the repair this same dispatch answered 400 FLOW_FAILED with + // the suspend signal stringified into the message, and no run id at + // all — the pause, and the run, were simply gone. + expect(res.response?.body?.data?.status).toBe('paused'); + expect(typeof res.response?.body?.data?.runId).toBe('string'); + expect(res.response?.body?.error).toBeUndefined(); + }); + + it('answers a pause on attempt 2 EXACTLY as it answers one on attempt 1', async () => { + const viaExecute = await trigger(boot(0)); + const viaRetry = await trigger(boot(1)); + + expect(viaRetry.response?.status).toBe(viaExecute.response?.status); + // The whole body, compared to the other path's body rather than to a + // hand-written expectation: one user-visible situation, one answer, and + // no way for a caller to tell which attempt paused. + expect(normalise(viaRetry.response?.body)).toEqual(normalise(viaExecute.response?.body)); + }); + + it('hands back a run that can really be CONTINUED — the durable half, at the wire', async () => { + const dispatcher = boot(1); + const paused = await trigger(dispatcher); + const runId = paused.response?.body?.data?.runId as string; + + // The real resume door, exactly as a console or the SDK reaches it. + // This is the assertion a status-string-only repair could not pass: + // with no continuation stored, the engine answers RUN_NOT_FOUND and + // this door returns 404. + const resumed = await dispatcher.handleAutomation( + `/flaky_approval/runs/${runId}/resume`, + 'POST', + { output: { verdict: 'approved' } }, + CTX, + ); + + expect(resumed.response?.status).toBe(200); + expect(resumed.response?.body?.success).toBe(true); + // The run continued past the gate and finished, rather than re-pausing + // or reporting a stale suspension. + expect(resumed.response?.body?.data?.status).not.toBe('paused'); + expect(resumed.response?.body?.data?.success).toBe(true); + }); + + it('serves the run log as paused, not failed, on run-detail', async () => { + const dispatcher = boot(1); + const paused = await trigger(dispatcher); + const runId = paused.response?.body?.data?.runId as string; + + const detail = await dispatcher.handleAutomation(`/flaky_approval/runs/${runId}`, 'GET', undefined, CTX); + + expect(detail.response?.status).toBe(200); + // A run log that says a paused run failed is a second lie on the same + // event — the operator surface would show a dead run that is in fact + // parked and waiting for a human. + expect(detail.response?.body?.data?.status).toBe('paused'); + }); +}); From 3afe1db0612eeec3127bf727c03af41365d5895c Mon Sep 17 00:00:00 2001 From: os-project-manager Date: Tue, 18 Aug 2026 23:17:38 +0000 Subject: [PATCH 2/2] =?UTF-8?q?test(runtime):=20make=20the=20isPausedRun?= =?UTF-8?q?=20pin=20able=20to=20fail=20=E2=80=94=20separate=20the=20verdic?= =?UTF-8?q?t=20from=20the=20companion=20field?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reverse verification caught this: replacing `isPausedRun`'s body with `!!result.screen` — the tolerant-consumer sniff PD #12 forbids — left the whole runtime suite green. Every assertion in the pin was satisfied by it, because a screen happened to accompany the paused fixture and to be absent from all the negatives, so the test could not fail against the one shape it exists to forbid. Adds the two cases that separate the readings: an `approval`/`wait` pause with NO screen is still a pause, and a screen on a result that is not parked does not make it one. Both go red under that substitution. Also states plainly, at the route-level case, that this door answers a pause and a terminal success identically on the wire by design, so no route assertion can tell which arm produced the response — the discrimination lives on the predicate. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza --- .../automation-trigger-paused-run.test.ts | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/packages/runtime/src/domains/automation-trigger-paused-run.test.ts b/packages/runtime/src/domains/automation-trigger-paused-run.test.ts index 80e649a16d..f2aa52b319 100644 --- a/packages/runtime/src/domains/automation-trigger-paused-run.test.ts +++ b/packages/runtime/src/domains/automation-trigger-paused-run.test.ts @@ -98,9 +98,16 @@ describe('#9510 — a triggered run that PAUSED is answered as the third state', 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. The state is read off `status`, so its - // absence must change nothing — a door that keyed on `screen` would - // report every approval pause as a terminal success. + // 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); @@ -119,14 +126,25 @@ describe('#9510 — the shared dispatch table names the non-terminal state', () }); it('reads the producer\'s lifecycle verdict, never the incidental fields', () => { - // `runId` and `screen` ride along on a pause; neither DEFINES it. A - // reader that sniffed them would call a resume refusal (which also - // carries a run id) a live pause. + // `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', () => {