Skip to content

Commit c79fdf0

Browse files
fix(service-automation): a retry attempt that PAUSES is a durable pause, not a failed attempt (#9510)
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y26DJEHSBhhAQ6wwfsHNza
1 parent 6cb88d9 commit c79fdf0

7 files changed

Lines changed: 1001 additions & 6 deletions

File tree

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
---
2+
"@objectstack/service-automation": patch
3+
"@objectstack/runtime": patch
4+
---
5+
6+
fix(service-automation): a retry attempt that PAUSES is a durable pause, not a failed attempt — `executeWithoutRetry` gets the ADR-0019 suspend arm (#9510)
7+
8+
`execute()`'s catch tests the suspend signal FIRST, and that arm is what makes
9+
ADR-0019's durable pause work: it snapshots the live variables, calls
10+
`persistSuspendedRun`, records a `paused` log entry and returns
11+
`{ success: true, status: 'paused', runId }`.
12+
13+
`executeWithoutRetry()` — the method `retryExecution` re-runs the flow through on
14+
**every** retry attempt — had no such arm. A `FlowSuspendSignal` thrown on a
15+
retry attempt fell into the generic failure path, and four things were lost at
16+
once:
17+
18+
1. `persistSuspendedRun` never ran, so **the continuation was never stored** and
19+
the run could not be resumed by anyone, ever;
20+
2. the run log recorded `failed` for a run that asked to pause;
21+
3. the caller got `status: 'failed'`, with the suspend signal stringified into
22+
`error` (`FlowSuspendSignal` is not an `Error`);
23+
4. `retryExecution` reads only `result.success`, so the pause counted as one more
24+
failed attempt: the loop burned the rest of the budget, and every further
25+
attempt re-entered the pausing node and orphaned another suspension.
26+
27+
Only a LATER attempt is exposed — `execute()` handles the first one correctly,
28+
and a flow reaches `retryExecution` only after a failure. The reachable shape is
29+
the ordinary one: `errorHandling.strategy: 'retry'` on a flow whose flaky
30+
HTTP/connector call is followed by an `approval` or `screen` node.
31+
32+
**⚠️ Runs already lost to this defect are NOT recoverable.** Nothing was written
33+
for them — no `sys_automation_run` row, no in-memory suspension — so there is no
34+
continuation to rehydrate and no repair, here or later, can bring one back. The
35+
run log holds a `failed` entry naming the flow and the trigger; those runs have
36+
to be triggered again. What this change fixes is every run from here on.
37+
38+
**The repair is a restoration of a stated contract on a path that never got it,
39+
not a new capability.** `AutomationResult.status: 'paused'` and ADR-0019 already
40+
describe exactly this behaviour, and `execute()`'s own arm already implements it;
41+
the retry path simply never received it. The alternative — refusing
42+
`strategy: 'retry'` combined with a pausing node at authoring time — was
43+
considered and rejected: it over-refuses (a pausing node can sit on a branch the
44+
retrying path never reaches), under-refuses (a pausing node behind a runtime
45+
condition is not statically decidable), and would ban the one combination authors
46+
most reasonably reach for.
47+
48+
**The cost, and what was done about it.** Lifting the arm makes `retryExecution`
49+
able to return a NON-TERMINAL result, and both of its readers were taught the
50+
third state explicitly rather than left to a branch that happens to fall through:
51+
the retry loop returns a paused attempt because it PAUSED (tested on `status`,
52+
before the `success` check that means "this attempt succeeded"), and the trigger
53+
route answers it from its own arm. The retry accounting is untouched — a
54+
genuinely failing attempt still consumes one, `maxRetries` still bounds the loop,
55+
and the loop stops only because the attempt did not fail.
56+
57+
**Both routes give one answer**, pinned as an equality rather than verified in
58+
isolation: a pause on attempt 1 and a pause on attempt 3 produce the same engine
59+
result and the same wire response, so no caller can tell which attempt paused.
60+
61+
Two adjacent gaps were measured out of this work and filed rather than absorbed:
62+
a retry attempt runs with a smaller variable environment than the first (#9704),
63+
and a flow's declared retry policy stops applying once a run pauses (#9705) —
64+
the latter being the measured answer to "what happens to the retry budget when a
65+
paused run is resumed and then fails": neither inherited nor fresh, because the
66+
resume path has no retry loop at all. Both are pinned as today's behaviour so
67+
neither can change by accident.
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #9510 — the trigger door answers a PAUSED run as the third state, deliberately.
5+
*
6+
* The engine repair that lifted `execute()`'s ADR-0019 suspend arm into
7+
* `executeWithoutRetry` gave `retryExecution` a NON-TERMINAL result to return: a
8+
* retry attempt that reaches a pausing node now comes back as
9+
* `{ success: true, status: 'paused', runId }` instead of being reported as a
10+
* failed attempt with its continuation dropped. This door is one of the two
11+
* readers that had only ever seen terminal results out of that path.
12+
*
13+
* What is pinned here is the door's READING, driven with scripted
14+
* `AutomationResult`s so every arm is reachable without a real engine (the
15+
* end-to-end sentence — that a real engine's two producers reach this door as
16+
* ONE answer — is `@objectstack/verify`'s
17+
* `automation-trigger-paused-run.test.ts`, and the engine-side equality is
18+
* `service-automation`'s `retry-attempt-pause.test.ts`).
19+
*
20+
* Both spellings of the door are exercised for every arm, from one table, for
21+
* the reason #9378's suite states: they share one context builder and one
22+
* response mapper, and a test covering only the canonical spelling would let the
23+
* legacy one — the one the SDK actually calls — drift unnoticed.
24+
*
25+
* ⚠️ The paused answer is deliberately IDENTICAL to the terminal-success one on
26+
* the wire, so these assertions cannot be satisfied by "some 200". They pin the
27+
* payload a caller resumes with: `status`, `runId`, `screen`, and the absence of
28+
* any refusal envelope.
29+
*/
30+
31+
import { describe, it, expect, vi } from 'vitest';
32+
33+
import { HttpDispatcher } from '../http-dispatcher.js';
34+
import { classifyFlowRefusal, isPausedRun } from '../flow-dispatch-status.js';
35+
import type { AutomationResult } from '@objectstack/spec/contracts';
36+
37+
const CTX = { request: {}, executionContext: { userId: 'user_1' } } as any;
38+
39+
/** Both spellings of the same door. `path` takes the flow name. */
40+
const ROUTES: Array<{ label: string; path: (flow: string) => string }> = [
41+
{ label: 'POST /:name/trigger', path: (f) => `/${f}/trigger` },
42+
{ label: 'legacy POST /trigger/:name', path: (f) => `/trigger/${f}` },
43+
];
44+
45+
function makeDispatcher(result: AutomationResult) {
46+
const flows = new Map([['flaky_approval', { name: 'flaky_approval' }]]);
47+
const execute = vi.fn(async (): Promise<AutomationResult> => result);
48+
const getFlow = vi.fn(async (name: string) => flows.get(name) ?? null);
49+
const services: Record<string, unknown> = { automation: { execute, getFlow } };
50+
const resolve = (name: string) => services[name];
51+
const kernel: any = {
52+
getService: resolve,
53+
getServiceAsync: async (name: string) => resolve(name),
54+
context: { getService: resolve },
55+
};
56+
return new HttpDispatcher(kernel);
57+
}
58+
59+
/**
60+
* The engine's paused result, in the shape BOTH producers build it — the arm in
61+
* `execute()`'s catch and the one restored to `executeWithoutRetry`. The two are
62+
* byte-identical apart from the ids, which is the point: this door must not be
63+
* able to tell which attempt paused.
64+
*/
65+
const PAUSED: AutomationResult = {
66+
success: true,
67+
status: 'paused',
68+
runId: 'run_7f0a',
69+
durationMs: 12,
70+
screen: {
71+
title: 'Approve the order',
72+
fields: [{ name: 'verdict', type: 'text', label: 'Verdict' }],
73+
} as AutomationResult['screen'],
74+
};
75+
76+
describe('#9510 — a triggered run that PAUSED is answered as the third state', () => {
77+
for (const route of ROUTES) {
78+
it(`${route.label}: answers 200 carrying the runId the caller resumes with`, async () => {
79+
const dispatcher = makeDispatcher(PAUSED);
80+
81+
const result = await dispatcher.handleAutomation(route.path('flaky_approval'), 'POST', {}, CTX);
82+
83+
expect(result.handled).toBe(true);
84+
expect(result.response?.status).toBe(200);
85+
expect(result.response?.body?.success).toBe(true);
86+
// The run is ALIVE and parked. Without these two the caller has no
87+
// way to continue it, which is the harm #9510 is about — a 200 that
88+
// merely "looks fine" is not the contract.
89+
expect(result.response?.body?.data?.status).toBe('paused');
90+
expect(result.response?.body?.data?.runId).toBe('run_7f0a');
91+
// The screen a screen-flow runner renders travels with it.
92+
expect(result.response?.body?.data?.screen?.title).toBe('Approve the order');
93+
// …and it is NOT dressed as a refusal: a paused run has no error
94+
// envelope, no code, and nothing for a status-blind caller to read
95+
// as a failure.
96+
expect(result.response?.body?.error).toBeUndefined();
97+
});
98+
99+
it(`${route.label}: a pause with no screen (approval, wait) is still the paused answer`, async () => {
100+
// `screen` is a screen-flow field; an `approval` or `wait` node
101+
// pauses without one. The state is read off `status`, so its
102+
// absence must change nothing — a door that keyed on `screen` would
103+
// report every approval pause as a terminal success.
104+
const dispatcher = makeDispatcher({ success: true, status: 'paused', runId: 'run_b21', durationMs: 3 });
105+
106+
const result = await dispatcher.handleAutomation(route.path('flaky_approval'), 'POST', {}, CTX);
107+
108+
expect(result.response?.status).toBe(200);
109+
expect(result.response?.body?.data?.status).toBe('paused');
110+
expect(result.response?.body?.data?.runId).toBe('run_b21');
111+
expect(result.response?.body?.error).toBeUndefined();
112+
});
113+
}
114+
});
115+
116+
describe('#9510 — the shared dispatch table names the non-terminal state', () => {
117+
it('classifies a paused run as no refusal at all', () => {
118+
expect(classifyFlowRefusal('flaky_approval', PAUSED)).toBeUndefined();
119+
});
120+
121+
it('reads the producer\'s lifecycle verdict, never the incidental fields', () => {
122+
// `runId` and `screen` ride along on a pause; neither DEFINES it. A
123+
// reader that sniffed them would call a resume refusal (which also
124+
// carries a run id) a live pause.
125+
expect(isPausedRun(PAUSED)).toBe(true);
126+
expect(isPausedRun({ success: true, runId: 'run_x' })).toBe(false);
127+
expect(isPausedRun({ success: false, status: 'failed', error: 'boom' })).toBe(false);
128+
expect(isPausedRun(undefined)).toBe(false);
129+
expect(isPausedRun(null)).toBe(false);
130+
});
131+
132+
it('never promotes a paused run into the FLOW_FAILED row, even against the grain', () => {
133+
// Defensive rather than reachable: no producer stamps this pair today.
134+
// It states which field decides when they disagree — a LIVE suspended
135+
// run, continuation persisted and waiting for a `resume()`, must not be
136+
// reported to its caller as a run that failed. That is #9510's defect
137+
// wearing transport clothing.
138+
const contradictory = { success: false, status: 'paused', runId: 'run_c3' } as AutomationResult;
139+
140+
expect(classifyFlowRefusal('flaky_approval', contradictory)).toBeUndefined();
141+
});
142+
143+
it('still classifies the terminal refusal rows — the new arm narrows nothing', () => {
144+
expect(classifyFlowRefusal('f', { success: false, status: 'failed', error: 'boom' })?.code)
145+
.toBe('FLOW_FAILED');
146+
expect(classifyFlowRefusal('f', { success: false, code: 'FLOW_DISABLED' })?.status).toBe(409);
147+
expect(classifyFlowRefusal('f', { success: false, code: 'FLOW_NO_START_NODE' })?.status).toBe(422);
148+
});
149+
});

packages/runtime/src/domains/automation.ts

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import {
2727
classifyFlowRefusal,
2828
flowIsUnknown,
2929
flowNotFoundMessage,
30+
isPausedRun,
3031
FLOW_NOT_FOUND_STATUS,
3132
} from '../flow-dispatch-status.js';
3233
import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js';
@@ -489,6 +490,14 @@ function flowDefinitionRefusal(err: any): unknown {
489490
* | flow disabled | never dispatched | `409` `FLOW_DISABLED` |
490491
* | flow has no start node | never dispatched | `422` `FLOW_NO_START_NODE` |
491492
* | ran and failed (incl. the retry-strategy exits) | ran, rejected | `400` `FLOW_FAILED` |
493+
* | ran and PAUSED (whichever attempt) | ran, suspended | `200` + `runId` / `screen` |
494+
*
495+
* [#9510] The last row is NON-TERMINAL and is answered by its own arm at the
496+
* bottom of this function. The durable pause is not a refusal, and since the
497+
* suspend arm was restored to the engine's retry path it arrives from two
498+
* producers — `execute()`'s catch and `retryExecution` — which this door must
499+
* NOT be able to tell apart. See that arm for why it is written separately from
500+
* the terminal success it happens to answer identically.
492501
*
493502
* [#9446] **The table itself now lives in `../flow-dispatch-status.js`** — one
494503
* definition, read by this door and by `/actions` (`action-execution.ts`) —
@@ -586,6 +595,34 @@ async function respondToFlowTrigger(
586595
response: deps.error(refusal.message, refusal.status, { code: refusal.code, ...runDetails }),
587596
};
588597
}
598+
// [#9510] THE THIRD STATE, answered deliberately. A run that dispatched and
599+
// then SUSPENDED at a pausing node (ADR-0019) is neither refused nor
600+
// finished: its continuation is persisted, and the `200` here carries the
601+
// `runId` — and the `screen`, for a screen flow — that the caller continues
602+
// it with at `POST /:name/runs/:runId/resume`, the door just below.
603+
//
604+
// The answer is unchanged from what this door has always given a paused
605+
// run, and that IS the requirement rather than an accident of ordering: it
606+
// must be the SAME answer a pause on the first attempt gets, because a
607+
// pause on a retry attempt is the same user-visible situation reached by a
608+
// different route. Two answers for one situation would replace #9510's LOST
609+
// pause with an inconsistent one. Pinned as an equality between the two
610+
// routes — engine-side in `service-automation`'s
611+
// `retry-attempt-pause.test.ts`, and on the wire through a real engine in
612+
// `@objectstack/verify`'s `automation-trigger-paused-run.test.ts`.
613+
//
614+
// ⛔ Its own arm even though it returns what the terminal exit below
615+
// returns. The two are different STATEMENTS about the run — "still running,
616+
// here is how to continue it" versus "it finished" — and collapsing them
617+
// recreates exactly the fall-through this card is about: a non-terminal
618+
// result that no reader on the path ever names is one edit away from being
619+
// classified as a terminal one.
620+
if (isPausedRun(result)) {
621+
return { handled: true, response: deps.success(result) };
622+
}
623+
// Terminal success: the run reached an `end` node, and `deps.success` serves
624+
// the engine result as the response data (`output`, `successMessage`,
625+
// `summary`).
589626
return { handled: true, response: deps.success(result) };
590627
}
591628

@@ -606,7 +643,9 @@ async function respondToFlowTrigger(
606643
* POST /:name/trigger → execute (legacy: trigger/:name also supported;
607644
* unknown name → 404, disabled → 409 `FLOW_DISABLED`,
608645
* no start node → 422 `FLOW_NO_START_NODE`, a run that
609-
* ran and failed → 400 `FLOW_FAILED`; #9378 + #9415)
646+
* ran and failed → 400 `FLOW_FAILED`; #9378 + #9415;
647+
* a run that PAUSED → 200 with `runId` / `screen`,
648+
* on whichever attempt it paused — #9510)
610649
* POST /:name/toggle → toggleFlow (unknown name → 404, #7535)
611650
* GET /:name/runs → listRuns (query: limit, cursor — validated, #7300;
612651
* status — validated AND honoured, #7359)

0 commit comments

Comments
 (0)