Skip to content

Commit 239e3a8

Browse files
fix(service-automation): the region refusal cancels its child run even when the suspension store cannot be read (#29329)
<!-- claude-projects-attribution: {"github_login":"os-steve"} --> _Requested by **Steve** · [project thread](https://claude.ai/code/project/chan_01FdL1XeUyvVNxLreh3F4Qi9?thread=cmsg_01FdL1XeUyvVNxLreh3F4Qi98Cgk5ia5FFRbkAcQ3MQ1FV)_ Fixes #29166 Item (1) is built here. Item (2) is answered by the seat's ruling (card comment 6576815635, option B): the run-time failure is the answer, nothing new is declined at arming, and this PR records that reading in two docblocks. Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:区域(loop / parallel / try_catch)内的子流程挂起被拒绝时,若暂停记录存储读不到、挂起链为空,现在仍对子运行调用一次 `cancelRun`,不再只留一条 warn。 - 为什么改:原来空链意味着零次取消,子运行可能一直停在存储里,之后被恢复去替一个早已失败的调用方继续跑。 - 风险与回滚:只在挂起链为空时多调用一次 `cancelRun`;无此挂起时它是空操作。拒绝本身(文字、不可路由)不变。回滚即还原 `dag-traversal.ts` 中这一个分支。 - 先看:`packages/services/service-automation/src/dag-traversal.ts`(`runRegion` 的空链分支),再看 `subflow-region-suspension.test.ts` 末尾新增的两例。 - 两分钟验证:`pnpm --filter @objectstack/service-automation exec vitest run src/subflow-region-suspension.test.ts`,预期 22 例全过。 - 需要你做的:无,席位落地。(第 2 项已由席位裁定为方案 B:不在布防时拒绝,运行期失败即答案,本 PR 只补注释记录这一读法。) ## What changes - `runRegion` (`packages/services/service-automation/src/dag-traversal.ts`): when the region refusal fires on a `subflow` / `map` node, the child run it parked is cancelled by walking `resolveSuspensionChain(childRunId)`. That walk reads through the degrading `loadSuspendedRun`, so a store it cannot read yields an empty chain, and the walk cancelled nothing. Now an empty chain calls `cancelRun(childRunId, reason)` once. `cancelRun` reads through the strict loader: if the store answers by then, the child is cancelled; if it still cannot be read, `cancelRun` itself logs the skipped cancellation at `error`, naming the run. If there really is no suspension under that id, `cancelRun` returns `false` and records nothing. - The refusal is unchanged and still thrown after the cancel (code, message, un-routability). - Item (2), option B, comments only: `subflow-region-guard.ts` and `region-suspension-refusal.ts` docblocks now record that the region arming decline is one level deep by design. A child's region call fails the CHILD run; its caller meets an ordinary, routable child failure and stays armed; keeping the refusal un-routable one level up would be a separate narrowing decision. - Changeset: `.changeset/29166-region-refusal-empty-chain-cancel.md`, `@objectstack/service-automation` patch. ## Tests Two cases added to `src/subflow-region-suspension.test.ts` (no existing test edited), with a store whose `load` fails for the child flow's runs: - **unreadable store** (always fails): the refusal text is unchanged, the child row is still in the store, `cancelRun` is called exactly once with the child run id, and exactly one `error` record is `cancelRun`'s own skipped-cancellation line for that id. - **store that answers again by the cancel** (fails once per run id): `cancelRun` is called once and the child run ends `cancelled`, with nothing left parked. Red first, then ablation (removing the new `cancelRun` call) — readings below. ## Acceptance notes - **Item (2): option B, by seat ruling (card comment 6576815635).** Measured on a scratch run at this branch (not committed): caller `deal_submit` has a top-level `subflow` to `deal_review`, whose `loop` body calls `deal_ask`, which suspends at a `wait`. The caller arms. Without a `fault` edge on its `subflow` node, its run fails loudly (`Node 'top_call' failed: subflow 'deal_review' failed: durable pause inside a structured region …`), `deal_review` ends `failed`, `deal_ask` ends `cancelled`, nothing parked. **With** a `fault` edge on that node, the caller's run completes with `success: true`: the child's refusal reaches the caller as an ordinary child failure, which the `subflow` contract routes. Declining such a caller (option A) would refuse a flow its author routed and that completes today, so the seat ruled B: the decline stays one level deep, recorded in the docblocks above. No behaviour change for item (2). - Partial chains are out of this PR: if the child is read but a deeper link's read fails, the chain stops at the last readable link and that deeper run is not cancelled (the degrading read's `warn` is the only record). The seat filed that as #29333. ## Verification <details><summary>Red first (fix absent)</summary> ``` × an unreadable store: … the child run gets exactly one cancelRun, … × a store that answers again by the cancel: … AssertionError: expected "cancelRun" to be called 1 times, but got 0 times Tests 2 failed | 20 passed (22) ``` </details> <details><summary>Green, then ablation (scripts/ablation-replace.mjs, on-disk proof)</summary> ``` green: Tests 22 passed (22) ablation-replace: anchor "await this.cancelRun(childRunId, reason);" x1 -> x0 ablation-replace: ok mutation landed: anchor 1 -> 0, blob 0395cdb -> 16347308d964 × an unreadable store … × a store that answers again … AssertionError: expected "cancelRun" to be called 1 times, but got 0 times Tests 2 failed | 20 passed (22) ablation-replace: ok restored: blob == HEAD (0395cdb) and `git diff HEAD` is empty ``` The test imports `./engine.js` from `src`, so no `dist` leg applies. </details> <details><summary>Final head 7aa1787 (comments-only commit on top of 36b3678)</summary> ``` turbo build ./packages/** then, under os-verify-lock (VERDICT command-exit 0): vitest run src/subflow-region-suspension.test.ts src/subflow-guard-region-calls.test.ts Test Files 2 passed (2) · Tests 37 passed (37) pnpm --filter @objectstack/service-automation typecheck exit 0 (check:test-typecheck OK, 0 pinned) dispatch-gates --ran: 61 derived, 61 run, 0 NOT-MEASURED, 0 UNRUN (all exit 0) ``` ``` node scripts/check-adr-0087-registration.mjs --base origin/main :: exit 0 node scripts/check-adr-0087-registration.mjs --self-test :: exit 0 node scripts/check-changeset-no-major.mjs --base origin/main :: exit 0 node scripts/check-closing-keyword-parity.mjs :: exit 0 node scripts/check-closing-keyword-parity.mjs --self-test :: exit 0 node scripts/check-comment-mask-adoption.mjs :: exit 0 node scripts/check-comment-mask-adoption.mjs --self-test :: exit 0 node scripts/check-comment-mask-corpus.mjs :: exit 0 node scripts/check-dts-emitted.mjs --self-test :: exit 0 node scripts/check-empty-changeset.mjs --base origin/main :: exit 0 node scripts/check-empty-changeset.mjs --self-test :: exit 0 node scripts/check-issue-citations.mjs :: exit 0 node scripts/check-plugin-teardown-shape.mjs :: exit 0 node scripts/check-plugin-teardown-shape.mjs --self-test :: exit 0 node scripts/check-tenant-audit-census.mjs :: exit 0 node scripts/check-tenant-audit-census.mjs --self-test :: exit 0 node scripts/ci/assert-turbo-remote-cache.mjs --self-test :: exit 0 node scripts/ci/plan-test-core-shards.mjs --self-test :: exit 0 node scripts/ci/prune-turbo-cache.mjs --self-test :: exit 0 node scripts/docs-audit/check-affected-docs.mjs :: exit 0 node scripts/docs-audit/check-drift-comment.mjs :: exit 0 node scripts/pm/release-rehearsal-clone.mjs --self-test :: exit 0 node scripts/release-pending-publish.mjs --self-test :: exit 0 pnpm --filter @objectstack/spec run check:duration-unit-keys :: exit 0 pnpm check:changeset-gate-self-tests :: exit 0 pnpm check:ci-filter-parity :: exit 0 pnpm check:closed-ceremony-text :: exit 0 pnpm check:cross-package-test-inputs :: exit 0 pnpm check:dispatcher-error-vocabulary :: exit 0 pnpm check:doc-authoring :: exit 0 pnpm check:driver-memory-census :: exit 0 pnpm check:dts-closure :: exit 0 pnpm check:dual-build-cjs-loads :: exit 0 pnpm check:durability-log-level :: exit 0 pnpm check:engine-double-contract :: exit 0 pnpm check:gitlink-declared :: exit 0 pnpm check:issue-citations :: exit 0 pnpm check:keyed-text-bounds :: exit 0 pnpm check:lean-entry-closure :: exit 0 pnpm check:logger-receiver-detach :: exit 0 pnpm check:nul-bytes :: exit 0 pnpm check:objectql-double-limit :: exit 0 pnpm check:org-identifier :: exit 0 pnpm check:page-declaration-shape :: exit 0 pnpm check:platform-object-tenancy-census :: exit 0 pnpm check:pm-changeset-deadline-census :: exit 0 pnpm check:published-files :: exit 0 pnpm check:query-options-erasure :: exit 0 pnpm check:refd-timer-probe :: exit 0 pnpm check:registry-log-declared :: exit 0 pnpm check:rest-log-spy-declared :: exit 0 pnpm check:slot-lookup :: exit 0 pnpm check:sourcemap-no-sources-content :: exit 0 pnpm check:system-context-census :: exit 0 pnpm check:test-source-alias :: exit 0 pnpm check:tier-file-adoption :: exit 0 pnpm check:type-check-coverage :: exit 0 pnpm check:type-check-debt :: exit 0 pnpm check:undeclared-dep-imports :: exit 0 pnpm check:watch-hint-literal :: exit 0 pnpm check:where-matcher :: exit 0 ``` </details> <details><summary>Earlier head 36b3678 (after merging main) — full package run</summary> ``` pnpm --filter @objectstack/service-automation test Test Files 259 passed (259) · Tests 3337 passed (3337) pnpm --filter @objectstack/service-automation typecheck exit 0 eslint --no-inline-config on the two changed .ts files exit 0 dispatch-gates --ran: 61 derived, 61 run, 0 NOT-MEASURED, 0 UNRUN (all exit 0) ``` On 7e91f5a, before the merge, the same 61 were green. Two of them first answered exit 3: `check-plugin-teardown-shape --self-test` (the fixture commit was not fetched) and `check:dual-build-cjs-loads` (the packages were not built). Both were re-run green after `git fetch origin 621a487` and a `./packages/**` build. </details> --- _Generated by [Claude Code](https://claude.ai/code/session_01ADHkmSmWmYVtGXXtTcxTr3)_ <!-- signed: domain:server#3 · os-steve · session_01ADHkmSmWmYVtGXXtTcxTr3 --> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d8b7ef4 commit 239e3a8

5 files changed

Lines changed: 118 additions & 0 deletions

File tree

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
'@objectstack/service-automation': patch
3+
---
4+
5+
fix(service-automation): the region refusal cancels the child run it leaves behind even when the suspension store cannot be read
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
When a `subflow` or `map` call inside a `loop` body, a `parallel` branch or a `try_catch` slot starts a child flow that suspends, the run fails at the region boundary and the child run it parked is cancelled. That cancel walked the child's suspension chain through a best-effort read, so a suspended-run store it could not read handed back an empty chain: nothing was cancelled, only a `warn` was logged, and the child run could stay parked, to be resumed later for a caller that had already failed. Now an empty chain still cancels the child run once through `cancelRun`. If the store answers by then, the child is cancelled; if it is still unreadable, `cancelRun` logs the skipped cancellation at `error`, naming the run to cancel by hand. The refusal itself, its text and its un-routability are unchanged.

‎packages/services/service-automation/src/dag-traversal.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -860,6 +860,18 @@ export async function runRegion(
860860
try {
861861
const chain = await this.resolveSuspensionChain(childRunId);
862862
const reason = `the run that started it failed at a region boundary: ${refusal.message}`;
863+
if (chain.length === 0) {
864+
// [#29166] The chain is read through the DEGRADING
865+
// loader, so a store it cannot read reads as "nothing
866+
// suspended" and walking it cancelled nothing, with
867+
// only that loader's `warn` said, while the child run
868+
// stayed parked. The child id is known here, so it is
869+
// cancelled once anyway: `cancelRun` reads through the
870+
// STRICT loader and owns the `error` record when the
871+
// store is still unreadable, and it is a no-op when
872+
// there really is no suspension under that id.
873+
await this.cancelRun(childRunId, reason);
874+
}
863875
for (const run of [...chain].reverse()) await this.cancelRun(run.runId, reason);
864876
} catch (cancelErr) {
865877
// #4632 verdict: DURABILITY, `error`, as `cancelRun`'s own

‎packages/services/service-automation/src/region-suspension-refusal.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ import { markGuardRefusal } from './guard-refusal.js';
3737
* `subflow` / `map`, `runRegion` also cancels the child run that node parked,
3838
* so nothing is left parked under a run that failed.
3939
*
40+
* The un-routability below holds inside the run that met the refusal. One call
41+
* level up it does not, on purpose ([#29166], seat ruling record 6576815635,
42+
* option B): when this refusal fails a CHILD run, its caller's `subflow` /
43+
* `map` node meets an ordinary child failure, routable like any other, and
44+
* the arming gate does not decline that caller. See `subflow-region-guard.ts`.
45+
*
4046
* ## Why a dedicated error and not the plain `Error` this replaces
4147
*
4248
* `runRegion` already converted a region-contained suspension — into a plain

‎packages/services/service-automation/src/subflow-region-guard.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,18 @@
2121
* child's whose own child can suspend, transitively. A child's region-nested
2222
* call is not read as a way for the CHILD to suspend: it fails the run at the
2323
* child's own region instead, so it never parks the caller.
24+
*
25+
* [#29166, seat ruling record 6576815635, option B] So the decline is one
26+
* level deep BY DESIGN, not by omission. A caller whose call (top-level or
27+
* not) reaches a child that itself makes a region call to a suspending flow is
28+
* armed: that child's region refusal fails the CHILD run, and the caller meets
29+
* it as an ordinary child failure, which the `subflow` / `map` contract makes
30+
* routable. Measured: with no `fault` edge on the caller's call node, the
31+
* caller's run fails loudly naming the child's region; with one, the caller's
32+
* run completes. Declining such a caller would refuse a flow whose author
33+
* routed that failure. The defect is the child's, and this gate names it on
34+
* the child the moment the child has a trigger of its own. Keeping the
35+
* refusal un-routable one level up would be a separate narrowing decision.
2436
*/
2537

2638
import { collectFlowGraphs } from '@objectstack/spec/automation';

‎packages/services/service-automation/src/subflow-region-suspension.test.ts‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,22 @@
2121
// caller. The refusal now cancels the child run, and any run that child is
2222
// parked on, through the engine's own `cancelRun`.
2323
//
24+
// [#29166] The chain that cancel walks is read through the DEGRADING loader
25+
// (`loadSuspendedRun`), so a store it cannot read hands back an empty chain.
26+
// Measured before that card: zero `cancelRun` calls, a `warn` from the
27+
// degrading read, and the child left parked with nothing at `error`. An empty
28+
// chain now still cancels the child run once, through `cancelRun`, whose
29+
// strict read owns the `error` record when the store stays unreadable.
30+
//
2431
// Neither half changes the refusal itself (code, text, un-routability) or
2532
// `FLOW_PAUSE_CAPABLE_NODE_TYPES`.
2633

2734
import { describe, it, expect, vi } from 'vitest';
2835
import { AutomationEngine } from './engine.js';
2936
import type { FlowTrigger, FlowTriggerBinding } from './engine.js';
3037
import { InMemoryFlowActivationStore } from './flow-activation-store.js';
38+
import { InMemorySuspendedRunStore } from './suspended-run-store.js';
39+
import type { SuspendedRun } from './engine.js';
3140
import {
3241
registerSubflowNode,
3342
registerMapNode,
@@ -283,3 +292,72 @@ describe('the region refusal cancels the child run it leaves behind (#28897)', (
283292
expect(parked).toEqual(['deal_review', 'deal_submit']);
284293
});
285294
});
295+
296+
/**
297+
* [#29166] A store whose `load` fails for the child flow's runs: always
298+
* (`'always'`), or only the first time each run id is read (`'once'`, a store
299+
* that answers again by the time the cancel reads it).
300+
*/
301+
class ChildLoadFailingStore extends InMemorySuspendedRunStore {
302+
private readonly failedOnce = new Set<string>();
303+
constructor(private readonly childFlow: string, private readonly mode: 'always' | 'once') {
304+
super();
305+
}
306+
override async load(runId: string): Promise<SuspendedRun | null> {
307+
const row = await super.load(runId);
308+
if (row?.flowName === this.childFlow && (this.mode === 'always' || !this.failedOnce.has(runId))) {
309+
this.failedOnce.add(runId);
310+
throw new Error('store connection reset');
311+
}
312+
return row;
313+
}
314+
}
315+
316+
describe('the region refusal still cancels the child run when its suspension chain cannot be read (#29166)', () => {
317+
function storeRig(mode: 'always' | 'once') {
318+
const r = rig();
319+
const store = new ChildLoadFailingStore('deal_review', mode);
320+
r.engine.setSuspendedRunStore(store);
321+
r.engine.registerFlow('deal_review', pausingChild('deal_review'));
322+
r.engine.registerFlow('deal_submit', regionCaller('deal_submit', 'deal_review', 'loop', 'subflow', false));
323+
const cancel = vi.spyOn(r.engine, 'cancelRun');
324+
return { ...r, store, cancel };
325+
}
326+
327+
it('an unreadable store: the refusal is thrown as before, the child run gets exactly one cancelRun, and the skipped cancellation is recorded at `error`', async () => {
328+
const { engine, logger, store, cancel } = storeRig('always');
329+
330+
const result: any = await engine.execute('deal_submit', {} as any);
331+
// The refusal itself is unchanged.
332+
expect(result.success).toBe(false);
333+
expect(String(result.error)).toContain(
334+
`durable pause inside a structured region: node 'step' (sub-flow 'deal_review') suspended`,
335+
);
336+
337+
const parked = await store.list();
338+
expect(parked.map((r) => r.flowName), 'the store could not be read, so the child is still parked').toEqual(['deal_review']);
339+
const childRunId = parked[0].runId;
340+
341+
expect(cancel).toHaveBeenCalledTimes(1);
342+
expect(cancel.mock.calls[0][0]).toBe(childRunId);
343+
expect(String(cancel.mock.calls[0][1])).toContain('durable pause inside a structured region');
344+
345+
const errors = logger.error.mock.calls.map((c: unknown[]) => String(c[0]));
346+
expect(
347+
errors.filter((m: string) => m.includes(`cancelRun('${childRunId}') could not read the durable suspended-run store`)),
348+
"cancelRun's strict read owns the error record",
349+
).toHaveLength(1);
350+
});
351+
352+
it('a store that answers again by the cancel: the child run the empty chain hid is cancelled, not left parked', async () => {
353+
const { engine, store, cancel } = storeRig('once');
354+
355+
const result: any = await engine.execute('deal_submit', {} as any);
356+
expect(result.success).toBe(false);
357+
358+
expect(cancel).toHaveBeenCalledTimes(1);
359+
expect(await store.list(), 'no run is left parked').toEqual([]);
360+
const runs = await engine.listRuns('deal_review');
361+
expect(runs.map((r: any) => r.status)).toEqual(['cancelled']);
362+
});
363+
});

0 commit comments

Comments
 (0)