From db34db0c81d42b67cafa9aa2cdd70a776af2e659 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 01:16:25 +0000 Subject: [PATCH] fix(spec,service-automation)!: `errorHandling.maxRetries` has one default, and `strategy: 'retry'` states its count (#4247) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `flow.errorHandling.maxRetries` was declared twice with different values: `FlowSchema` said `.default(0)`, while the engine's `retryExecution` read `errorHandling.maxRetries ?? 3`. `??` fires only on `undefined`, so the winner was decided by the ROUTE a flow took into the engine, not by what its author wrote — 0 retries for a flow parsed by the schema, 3 for a definition built by hand and handed to the engine. The neighbouring `retryDelayMs ?? 1000` / `backoffMultiplier ?? 1` agreed with their `.default()`s; only `maxRetries` disagreed. The engine now keeps no defaults of its own: `retryExecution` takes the parsed `NonNullable` and destructures all five knobs, no `??`. That is safe because `AutomationEngine.flows` only ever holds `FlowSchema.parse` output (`registerFlow` parses; the version-history rollback re-seats an already-parsed snapshot), and the parsed parameter type is what keeps a second set of defaults from growing back — a knob the spec stops defaulting becomes a compile error rather than a silent engine-side guess (Prime Directive #12). BREAKING: `strategy: 'retry'` now requires `maxRetries` >= 1. With the engine's copy gone an unstated count is unambiguously 0, and 'retry' with 0 attempts runs the flow once and stops — `strategy: 'fail'` under another label, a declared capability the runtime does not deliver. Rather than pick 0 or 3 for the author, the schema refuses the combination in both spellings (omitted → defaulted 0, and an explicit 0) with the prescription in the message; a retry re-runs the WHOLE flow, side effects included, so the count is the author's to state. `maxRetries: 0` stays legal under 'fail'/'continue', which never read it. The migration chain surfaces this as the semantic TODO `flow-retry-max-retries-required` — there is no lossless rewrite, so `os migrate meta` delegates the choice instead of guessing it. Closes #4247 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SNpSdWvbikA9PiwXzkAH4i --- .changeset/flow-max-retries-single-default.md | 56 +++++++ content/docs/automation/flows.mdx | 41 ++++- content/docs/releases/v17.mdx | 11 ++ docs/protocol-upgrade-guide.md | 5 + packages/cli/test/migrate-meta.e2e.test.ts | 5 +- .../services/service-automation/src/engine.ts | 35 ++-- .../src/flow-retry-attempt-count.test.ts | 150 ++++++++++++++++++ packages/spec/liveness/flow.json | 4 +- packages/spec/spec-changes.json | 14 ++ packages/spec/src/automation/flow.test.ts | 80 +++++++++- packages/spec/src/automation/flow.zod.ts | 43 ++++- packages/spec/src/migrations/registry.ts | 36 ++++- 12 files changed, 458 insertions(+), 22 deletions(-) create mode 100644 .changeset/flow-max-retries-single-default.md create mode 100644 packages/services/service-automation/src/flow-retry-attempt-count.test.ts diff --git a/.changeset/flow-max-retries-single-default.md b/.changeset/flow-max-retries-single-default.md new file mode 100644 index 0000000000..a8206ad11c --- /dev/null +++ b/.changeset/flow-max-retries-single-default.md @@ -0,0 +1,56 @@ +--- +"@objectstack/spec": major +"@objectstack/service-automation": major +--- + +fix(spec,service-automation)!: `errorHandling.maxRetries` has one default, and `strategy: 'retry'` states its count (#4247) + +`flow.errorHandling.maxRetries` was declared twice, with different values: + +- **spec** — `FlowSchema` (`automation/flow.zod.ts`): `.default(0)` +- **engine** — `retryExecution` (`service-automation/src/engine.ts`): + `errorHandling.maxRetries ?? 3` + +`??` fires only on `undefined`, so the winner was decided by the ROUTE a flow +took into the engine, not by what its author wrote: + +| Path | `errorHandling.maxRetries` | Retries | +|:---|:---|---:| +| parsed by `FlowSchema` (`.default(0)` fills it) | `0` | **0** | +| object built by hand and fed to the engine | `undefined` | **3** | + +One authored intent — "I didn't write a count" — two behaviors. The neighbouring +`retryDelayMs ?? 1000` / `backoffMultiplier ?? 1` agreed with their `.default()`s; +only `maxRetries` disagreed, which reads as a schema default changed from 3 to 0 +without the engine following, not as a deliberate two-track design. + +**The engine keeps no defaults of its own.** `retryExecution` now takes the +parsed `NonNullable` and destructures all five +knobs — no `??`. This is safe because `AutomationEngine.flows` only ever holds +`FlowSchema.parse` output (`registerFlow` parses; the version-history rollback +re-seats an already-parsed snapshot), and it is what keeps a second set of +defaults from growing back: a knob the spec stops defaulting becomes a compile +error rather than a silent engine-side guess. Per Prime Directive #12 the spec +is the one contract; a consumer-side fallback is a second de-facto one. + +**BREAKING — `strategy: 'retry'` now requires `maxRetries` >= 1.** With the +engine's copy gone, an unstated count is unambiguously `0`, and `'retry'` with 0 +attempts runs the flow once and stops — i.e. `strategy: 'fail'` wearing another +label, a declared capability the runtime does not deliver (Prime Directive #10 +corollary). Rather than pick 0 or 3 on the author's behalf, `FlowSchema` refuses +the combination in both spellings (omitted → defaulted 0, and an explicit 0), +with the prescription in the message. A retry re-runs the **whole flow from the +start** — records created again, callouts fired again — which is not a number to +guess for someone. + +FROM → TO: + +- `errorHandling: { strategy: 'retry' }` → `errorHandling: { strategy: 'retry', maxRetries: 3 }` + (or `strategy: 'fail'` if no retry was intended — that is what it did). +- `errorHandling: { strategy: 'retry', maxRetries: 0 }` → same choice, spelled out. + +Unaffected: `maxRetries: 0` under `strategy: 'fail'` / `'continue'` (neither +reads it, and a fully spelled-out block stays legal), flows with no +`errorHandling` at all, and every flow that already states a count — including +the `try_catch` node's own `config.retry`, which is a separate per-region policy +(`control-flow.zod.ts`) and is unchanged. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index df40e3f0e9..f160e843e5 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -665,7 +665,7 @@ The two recovery mechanisms operate at different scopes and do not compound: | | Scope | On failure | | :--- | :--- | :--- | | `fault` edge | one node | traversal continues from the handler; the run completes | -| `errorHandling: { strategy: 'retry' }` | the whole flow | the flow re-runs **from the start** | +| `errorHandling: { strategy: 'retry', maxRetries: n }` | the whole flow | the flow re-runs **from the start**, up to `n` more times | A failure a fault edge handled is not a flow failure, so it does **not** consume a retry. That matters because flow-level retry replays every node that already @@ -707,13 +707,46 @@ errorHandling: { | Property | Type | Description | | :--- | :--- | :--- | -| `strategy` | `enum` | `'fail'` (stop) or `'retry'` (re-run the whole flow). `'continue'` parses but the engine branches only on `'retry'`, so it behaves exactly like `'fail'` — use a `fault` edge to keep going past a failed node | -| `maxRetries` | `number` | Maximum retry attempts (0-10) | -| `retryDelayMs` | `number` | Delay between retries (ms) | +| `strategy` | `enum` | `'fail'` (stop) or `'retry'` (re-run the whole flow). `'continue'` parses but the engine branches only on `'retry'`, so it behaves exactly like `'fail'` — use a `fault` edge to keep going past a failed node (default `'fail'`) | +| `maxRetries` | `number` | Retry attempts **after** the initial one, `0`–`10`. Under `strategy: 'retry'` it must be at least `1` and there is no default — see below (default `0`, i.e. no retries, for the strategies that never retry) | +| `retryDelayMs` | `number` | Delay between retries (ms) (default `1000`) | | `backoffMultiplier` | `number` | Exponential backoff multiplier (default `1`) | | `maxRetryDelayMs` | `number` | Ceiling on the backed-off delay (default `30000`) | | `jitter` | `boolean` | Randomize the delay to avoid a thundering herd (default `false`) | +`maxRetries` counts the **re-runs**, not the total attempts: `maxRetries: 2` +runs the flow up to three times. + +### `strategy: 'retry'` has to say how many times + +There is no default retry count. `strategy: 'retry'` without `maxRetries` — or +with `maxRetries: 0` — is refused when the flow is registered: + +```typescript +// ❌ rejected: "retry" that retries zero times is just "fail" +errorHandling: { strategy: 'retry' } + +// ✅ state the attempts +errorHandling: { strategy: 'retry', maxRetries: 3, retryDelayMs: 5000 } +``` + +A retry re-runs the **whole flow from the start**, so every node that already +succeeded runs again — records get created again, callouts fire again. That is +too consequential a number to pick on the author's behalf, and picking `0` +would make opting into `'retry'` do nothing at all. The knobs above are read +only under `'retry'`; a fully spelled-out block under `'fail'` or `'continue'` +is fine and simply ignored. + + + Before ObjectStack 17 the count depended on how the flow reached the engine: + a flow parsed by `FlowSchema` retried `0` times when the count was unstated, + while a hand-built definition passed straight to the engine retried `3` — the + schema and the engine each carried a default and they disagreed + ([#4247](https://github.com/objectstack-ai/objectstack/issues/4247)). The + engine's copy is gone; the schema is the only source, and the case that was + ambiguous is now rejected instead of guessed. + + ## Discovery & Registration You almost never call `engine.registerFlow()` directly. The diff --git a/content/docs/releases/v17.mdx b/content/docs/releases/v17.mdx index 7824fa29e0..9666b0ec28 100644 --- a/content/docs/releases/v17.mdx +++ b/content/docs/releases/v17.mdx @@ -930,6 +930,17 @@ from `@objectstack/spec` directly. - **`sys_view_definition`'s all-six `apiMethods` whitelist is dropped** (#3026). - **`os migrate plan` shows index drift** — index DDL is no longer applied silently at boot (#3728). +- **A flow's `errorHandling.strategy: 'retry'` must state `maxRetries` (>= 1)** + (#4247). `maxRetries` had two defaults — `.default(0)` in `FlowSchema` and + `?? 3` in the engine's `retryExecution` — so an unstated count retried 0 times + for a flow that had been through the schema and 3 times for a definition + handed to the engine directly. The engine's copy is gone (it reads the parsed + block with no fallback), and the case that was ambiguous is rejected rather + than guessed: retrying zero times is `strategy: 'fail'` under another name, + and a retry re-runs the *whole* flow, so the count is the author's to state. + Fix: write `{ strategy: 'retry', maxRetries: 3 }`, or `strategy: 'fail'` if no + retry was intended. `maxRetries: 0` stays legal under `'fail'` / `'continue'`, + which never read it. ## New capabilities in 17.0.0 diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 1c0fbf743c..bee5068a0a 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -142,6 +142,8 @@ On the wire contract it also retires the `/analytics/query` request ENVELOPE (#3 The close-out sweep finishes the enforce-or-remove worklist across the remaining types: action `shortcut`/`bulkEnabled` (no keydown path; the multi-select toolbar reads the view's bulkActions), flow `active`/`template`/node `outputSchema`/errorHandling `fallbackNodeId` (`active: false` never stopped a flow — `status` is the enforced lifecycle; faults route via per-node fault edges), the inert view keys (list `responsive`/`performance`, form `data`/`defaultSort`/`aria` — list aria/data stay live), dashboard and widget `aria`/`performance`, `agent.knowledge` (declaring sources never scoped retrieval — absorbs the former topics→sources rename), and `skill.triggerPhrases` (phrases were never matched; routing is triggerConditions + the agent allowlist). All pure lossless deletes, each tombstoned at its schema with the prescription. +One flow key changes WITHOUT a lossless target: `errorHandling.maxRetries` (#4247). It carried two defaults — `.default(0)` in FlowSchema and `maxRetries ?? 3` in the engine's retryExecution — and because `??` fires only on `undefined`, an unstated count meant 0 retries for a flow parsed by the schema and 3 for a definition handed to the engine directly: the retry count was a function of the route in, not of the authored flow. The engine's copy is deleted (it reads the parsed block, no fallback), which makes an unstated count unambiguously 0 — and `strategy: 'retry'` that retries zero times is `strategy: 'fail'` under another name, the declared-not-delivered shape ADR-0049 exists to close. The schema therefore requires `maxRetries` at least 1 under `'retry'`, in both spellings (omitted, and an explicit 0). This is the one v17 flow change the chain cannot apply for you: choosing the count is a judgment about re-running the WHOLE flow with its side effects, so it is a semantic TODO rather than a rewrite. + ### Mechanical (applied for you) | Conversion | Surface | Change | Load window | @@ -169,6 +171,9 @@ The close-out sweep finishes the enforce-or-remove worklist across the remaining ### Semantic (delegated to you, with acceptance criteria) +- **`flow-retry-max-retries-required`** — `flow.errorHandling.maxRetries (under strategy: 'retry')` → an explicit count >= 1 (e.g. maxRetries: 3), or strategy: 'fail' + - Why not automatic: maxRetries had two defaults — FlowSchema `.default(0)` and the engine's `maxRetries ?? 3` — so an unstated count retried 0 times through the schema and 3 times through a hand-built definition (#4247). With the engine's copy removed the unstated count is unambiguously 0, and retrying zero times is exactly `strategy: 'fail'`, so the schema now refuses the combination instead of it silently doing nothing. There is no lossless rewrite: 0 preserves the behaviour a parsed flow got but contradicts what its author wrote, and any positive count is a NEW decision about re-running the whole flow with its side effects. That choice is the author's. + - Done when: Every flow declaring `errorHandling.strategy: 'retry'` also declares `maxRetries` >= 1, and each count was chosen knowing a retry replays the flow FROM THE START (records re-created, callouts re-fired); flows that never actually wanted retries say `strategy: 'fail'`. No flow fails to register with the maxRetries prescription. - **`analytics-query-request-envelope-retired`** — `api.analyticsQueryRequest.query` → bare AnalyticsQuery body (top-level cube/measures/dimensions/where/...) - Why not automatic: The { cube, query: {...} } envelope was an HTTP-wire dialect of the retired degraded analytics shim (#3891), never stored in stack metadata — there is no source for the chain to rewrite. Callers of POST /analytics/query and /analytics/sql must move the query.* fields to the body top level themselves. - Done when: Every /analytics/query and /analytics/sql call sends the bare AnalyticsQuery shape and succeeds; no request answers 400 VALIDATION_FAILED with the envelope prescription. diff --git a/packages/cli/test/migrate-meta.e2e.test.ts b/packages/cli/test/migrate-meta.e2e.test.ts index b32893d1c9..04cd64699f 100644 --- a/packages/cli/test/migrate-meta.e2e.test.ts +++ b/packages/cli/test/migrate-meta.e2e.test.ts @@ -62,7 +62,10 @@ export default { type: 'autolaunched', active: false, // 16: removed (status is the lifecycle) template: true, // 16: removed (no reader) - errorHandling: { strategy: 'retry', fallbackNodeId: 'n9' }, // 16: fault edges own this + // 16: fallbackNodeId removed (fault edges own this). maxRetries is stated + // because #4247 refuses a zero-attempt 'retry' — the count is the one v17 + // flow change the chain surfaces as a semantic TODO instead of rewriting. + errorHandling: { strategy: 'retry', maxRetries: 2, fallbackNodeId: 'n9' }, nodes: [ { id: 'n1', type: 'start', label: 'Start', outputSchema: { ok: { type: 'boolean' } } }, // 16: removed { id: 'n2', type: 'delete_record', label: 'Purge', config: { objectName: 'e2e_ticket', filter: { done: true } } }, // canonical since protocol 11 — must pass through untouched diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 15bd386b66..3672145b50 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -3485,24 +3485,35 @@ 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. + * + * 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 + * (`maxRetries ?? 3` against `.default(0)`), which made the retry count a + * function of how the flow reached the engine rather than of what its + * author wrote. `this.flows` only ever holds `FlowSchema.parse` output — + * `registerFlow` parses, and the version-history rollback re-seats a + * previously parsed snapshot — so every field below is present, and typing + * the parameter as the parsed shape is what keeps a second set of defaults + * from growing back here: a knob the spec stops defaulting becomes a + * compile error, not a silent engine-side guess. */ private async retryExecution( flowName: string, context: AutomationContext | undefined, startTime: number, - errorHandling: { - maxRetries?: number; - retryDelayMs?: number; - backoffMultiplier?: number; - maxRetryDelayMs?: number; - jitter?: boolean; - }, + errorHandling: NonNullable, ): Promise { - const maxRetries = errorHandling.maxRetries ?? 3; - const baseDelay = errorHandling.retryDelayMs ?? 1000; - const multiplier = errorHandling.backoffMultiplier ?? 1; - const maxDelay = errorHandling.maxRetryDelayMs ?? 30000; - const useJitter = errorHandling.jitter ?? false; + // `maxRetries >= 1` is guaranteed under `strategy: 'retry'` — the schema + // refuses the zero-attempt spelling of "retry" (#4247), so reaching this + // method always means at least one re-run. + const { + maxRetries, + retryDelayMs: baseDelay, + backoffMultiplier: multiplier, + maxRetryDelayMs: maxDelay, + jitter: useJitter, + } = errorHandling; let lastError = 'Max retries exceeded'; for (let i = 0; i < maxRetries; i++) { diff --git a/packages/services/service-automation/src/flow-retry-attempt-count.test.ts b/packages/services/service-automation/src/flow-retry-attempt-count.test.ts new file mode 100644 index 0000000000..0fde446a80 --- /dev/null +++ b/packages/services/service-automation/src/flow-retry-attempt-count.test.ts @@ -0,0 +1,150 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { AutomationEngine } from './engine.js'; + +/** + * #4247 — how many times does a flow that asked to retry actually retry? + * + * There used to be two answers. `FlowSchema` declared + * `errorHandling.maxRetries` with `.default(0)`; `retryExecution` read + * `errorHandling.maxRetries ?? 3`. `??` only fires on `undefined`, so the + * winner was decided by ROUTE, not by authoring: a flow parsed by the schema + * carried an explicit `0` and never retried, while a hand-built object handed + * straight to the engine kept `undefined` and retried three times. Same + * authored intent ("I didn't write a count"), two behaviors. + * + * The resolution is contract-first (AGENTS.md Prime Directive #12): the spec + * holds the only defaults, the engine reads the parsed block with no `??` of + * its own, and the one combination that was inert either way — + * `strategy: 'retry'` with zero attempts — is refused at authoring rather than + * silently behaving like `strategy: 'fail'`. + * + * These tests pin the count itself, because that is the part a reader cannot + * confirm from the schema alone: `maxRetries` is RETRIES, so the flow runs + * `maxRetries + 1` times in total. + */ + +function createTestLogger(): any { + return { info: () => {}, warn: () => {}, error: () => {}, debug: () => {}, child: () => createTestLogger() }; +} + +/** An always-failing single-node flow that counts how many times it ran. */ +function failingFlowEngine(errorHandling: unknown) { + const engine = new AutomationEngine(createTestLogger()); + const runs = { count: 0 }; + + engine.registerNodeExecutor({ + type: 'script', + async execute() { + runs.count++; + return { success: false, error: 'downstream 503' }; + }, + }); + + const register = () => engine.registerFlow('retrying', { + name: 'retrying', + label: 'Retrying', + type: 'autolaunched', + errorHandling, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'work', type: 'script' as any, label: 'Work' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'work' }, + { id: 'e1', source: 'work', target: 'end' }, + ], + } as any); + + return { engine, runs, register }; +} + +describe('#4247 — the retry count comes from the flow, not from the engine', () => { + it('runs maxRetries + 1 times: the initial attempt plus the declared retries', async () => { + const { engine, runs, register } = failingFlowEngine({ + strategy: 'retry', + maxRetries: 2, + retryDelayMs: 0, + }); + register(); + + const result = await engine.execute('retrying'); + + expect(result.success).toBe(false); + // 1 initial attempt + 2 retries. Neither 1 (the old parsed-flow + // behavior for an unstated count) nor 4 (the old `?? 3` fallback). + expect(runs.count).toBe(3); + }); + + it('honours a one-attempt retry — the smallest count the schema now allows', async () => { + const { engine, runs, register } = failingFlowEngine({ + strategy: 'retry', + maxRetries: 1, + retryDelayMs: 0, + }); + register(); + + await engine.execute('retrying'); + + expect(runs.count).toBe(2); + }); + + it("refuses `strategy: 'retry'` with no maxRetries instead of guessing a count", () => { + const { register } = failingFlowEngine({ strategy: 'retry', retryDelayMs: 0 }); + + // The pre-#4247 outcomes were "registers, never retries" (schema route) + // and "registers, retries 3×" (direct route). Now it is neither: the + // flow does not register at all, and the error says what to write. + expect(register).toThrow(/maxRetries/); + expect(register).toThrow(/strategy: 'fail'/); + }); + + it("refuses `strategy: 'retry'` with an explicit maxRetries: 0 — that is `'fail'`", () => { + const { register } = failingFlowEngine({ strategy: 'retry', maxRetries: 0, retryDelayMs: 0 }); + + expect(register).toThrow(/maxRetries/); + }); + + it("leaves the retry knobs alone under 'fail' — a fully spelled-out block stays legal", async () => { + const { engine, runs, register } = failingFlowEngine({ + strategy: 'fail', + maxRetries: 0, + retryDelayMs: 0, + backoffMultiplier: 1, + maxRetryDelayMs: 0, + jitter: false, + }); + register(); + + const result = await engine.execute('retrying'); + + expect(result.success).toBe(false); + expect(runs.count).toBe(1); + }); + + it('hands retryExecution a fully-populated block — the engine needs no fallback of its own', async () => { + const { engine, register } = failingFlowEngine({ + strategy: 'retry', + maxRetries: 1, + retryDelayMs: 0, + }); + register(); + + // `this.flows` only ever holds `FlowSchema.parse` output, so every knob + // `retryExecution` destructures is present without a `??`. This is the + // invariant that made deleting the engine-side defaults safe — if a + // future schema change made one of these optional again, the engine + // would stop compiling rather than quietly re-invent a default. + const stored = await engine.getFlow('retrying'); + expect(stored?.errorHandling).toMatchObject({ + strategy: 'retry', + maxRetries: 1, + retryDelayMs: 0, + backoffMultiplier: 1, + maxRetryDelayMs: 30000, + jitter: false, + }); + }); +}); diff --git a/packages/spec/liveness/flow.json b/packages/spec/liveness/flow.json index 0cbaea6b90..45aa5fd582 100644 --- a/packages/spec/liveness/flow.json +++ b/packages/spec/liveness/flow.json @@ -114,7 +114,9 @@ }, "maxRetries": { "status": "live", - "evidence": "packages/services/service-automation/src/engine.ts" + "verifiedAt": "2026-07-31", + "evidence": "packages/services/service-automation/src/engine.ts", + "note": "LIVE, and now single-sourced (#4247). It was live under TWO defaults — `.default(0)` here, `maxRetries ?? 3` in retryExecution — so the count depended on whether the flow had been through FlowSchema. The engine's fallback is deleted (it destructures the parsed block), and the schema refuses `strategy: 'retry'` with maxRetries < 1, since a zero-attempt retry is `strategy: 'fail'` under another name." }, "retryDelayMs": { "status": "live", diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index 7aaba8330a..5b0d24a1cb 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -260,6 +260,13 @@ "toMajor": 16, "rationale": "The `.strict()` flip turns a previously silently-stripped unknown key into a parse error. There is no mapping target for an arbitrary unknown key — auto-deleting it would be exactly the silent data loss ADR-0078 bans — so each occurrence needs the author to decide: bind a `dataset` and select `dimensions`/`values`, move a renderer setting under `options`, or delete the dead key." }, + { + "surface": "flow.errorHandling.maxRetries (under strategy: 'retry')", + "replacement": "an explicit count >= 1 (e.g. maxRetries: 3), or strategy: 'fail'", + "migrationId": "flow-retry-max-retries-required", + "toMajor": 17, + "rationale": "maxRetries had two defaults — FlowSchema `.default(0)` and the engine's `maxRetries ?? 3` — so an unstated count retried 0 times through the schema and 3 times through a hand-built definition (#4247). With the engine's copy removed the unstated count is unambiguously 0, and retrying zero times is exactly `strategy: 'fail'`, so the schema now refuses the combination instead of it silently doing nothing. There is no lossless rewrite: 0 preserves the behaviour a parsed flow got but contradicts what its author wrote, and any positive count is a NEW decision about re-running the whole flow with its side effects. That choice is the author's." + }, { "surface": "api.analyticsQueryRequest.query", "replacement": "bare AnalyticsQuery body (top-level cube/measures/dimensions/where/...)", @@ -599,6 +606,13 @@ } ], "migrated": [ + { + "surface": "flow.errorHandling.maxRetries (under strategy: 'retry')", + "replacement": "an explicit count >= 1 (e.g. maxRetries: 3), or strategy: 'fail'", + "migrationId": "flow-retry-max-retries-required", + "toMajor": 17, + "rationale": "maxRetries had two defaults — FlowSchema `.default(0)` and the engine's `maxRetries ?? 3` — so an unstated count retried 0 times through the schema and 3 times through a hand-built definition (#4247). With the engine's copy removed the unstated count is unambiguously 0, and retrying zero times is exactly `strategy: 'fail'`, so the schema now refuses the combination instead of it silently doing nothing. There is no lossless rewrite: 0 preserves the behaviour a parsed flow got but contradicts what its author wrote, and any positive count is a NEW decision about re-running the whole flow with its side effects. That choice is the author's." + }, { "surface": "api.analyticsQueryRequest.query", "replacement": "bare AnalyticsQuery body (top-level cube/measures/dimensions/where/...)", diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index 777ed319ba..52510225a1 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -595,6 +595,82 @@ describe('FlowSchema - errorHandling', () => { expect(result.errorHandling?.maxRetries).toBe(0); }); + /** + * #4247 — `maxRetries` had two defaults: `.default(0)` here and + * `errorHandling.maxRetries ?? 3` in the engine's `retryExecution`. Because + * `??` only fires on `undefined`, an unstated count meant 0 retries for a + * parsed flow and 3 for a hand-built one — the retry count depended on the + * route into the engine, not on what the author wrote. + * + * The engine's copy is gone; this block is the only source. The count that + * was ambiguous is also the one that was inert — `strategy: 'retry'` with 0 + * attempts runs the flow once and stops, i.e. `strategy: 'fail'` under + * another name — so it is refused rather than defaulted to some number + * nobody wrote. A retry re-runs the WHOLE flow, side effects included; that + * is not a count to guess on the author's behalf. + */ + describe('#4247 — one default, and no zero-attempt "retry"', () => { + const retryFlow = (errorHandling: unknown) => FlowSchema.safeParse({ + name: 'retry_flow', + label: 'Retry Flow', + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + errorHandling, + }); + + it("REJECTS strategy: 'retry' with no maxRetries — the count is stated, never guessed", () => { + const result = retryFlow({ strategy: 'retry' }); + + expect(result.success).toBe(false); + const issue = result.error!.issues.find((i) => i.path.join('.') === 'errorHandling.maxRetries'); + expect(issue, 'the refusal should point at maxRetries').toBeDefined(); + // The message carries the prescription, not just the complaint. + expect(issue!.message).toContain('maxRetries: 3'); + expect(issue!.message).toContain("strategy: 'fail'"); + }); + + it("REJECTS strategy: 'retry' with an explicit maxRetries: 0 — same no-op, spelled out", () => { + const result = retryFlow({ strategy: 'retry', maxRetries: 0 }); + + expect(result.success).toBe(false); + expect(result.error!.issues.some((i) => i.path.join('.') === 'errorHandling.maxRetries')).toBe(true); + }); + + it("accepts the smallest honest retry — strategy: 'retry' with maxRetries: 1", () => { + const result = retryFlow({ strategy: 'retry', maxRetries: 1 }); + + expect(result.success).toBe(true); + expect(result.data!.errorHandling?.maxRetries).toBe(1); + }); + + it("keeps maxRetries: 0 legal under 'fail' and 'continue' — they never read it", () => { + for (const strategy of ['fail', 'continue'] as const) { + const result = retryFlow({ strategy, maxRetries: 0, retryDelayMs: 0, backoffMultiplier: 1 }); + expect(result.success, `${strategy} should accept a spelled-out block`).toBe(true); + } + }); + + it('parses every retry knob to a concrete value — the engine reads them with no fallback', () => { + const result = retryFlow({ strategy: 'retry', maxRetries: 2 }); + + // `retryExecution` destructures these five directly. Each must survive + // the parse as a number/boolean, or the engine would be back to + // re-inventing a default for whichever one went missing. + expect(result.data!.errorHandling).toMatchObject({ + strategy: 'retry', + maxRetries: 2, + retryDelayMs: 1000, + backoffMultiplier: 1, + maxRetryDelayMs: 30000, + jitter: false, + }); + }); + }); + it('REJECTS the retired errorHandling.fallbackNodeId — faults route via fault edges (#3896)', () => { expect(() => FlowSchema.parse({ name: 'fallback_flow', @@ -657,7 +733,9 @@ describe('FlowSchema - errorHandling', () => { { id: 'end', type: 'end', label: 'End' }, ], edges: [{ id: 'e1', source: 'start', target: 'end' }], - errorHandling: { strategy: 'retry' }, + // `maxRetries` is stated because #4247 refuses a zero-attempt 'retry'; + // the backoff knobs under test still default. + errorHandling: { strategy: 'retry', maxRetries: 3 }, }); expect(result.success).toBe(true); if (result.success) { diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index 5ce568cc3f..32d97fe23b 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -428,10 +428,30 @@ export const FlowSchema = lazySchema(() => z.object({ 'record-change flow fired by a write that carried no user.', ), - /** Error Handling Strategy */ + /** + * Error Handling Strategy. + * + * **These defaults are the only defaults** (#4247). The engine reads the + * parsed block field-by-field with no fallback of its own — `retryExecution` + * used to carry `errorHandling.maxRetries ?? 3` beside a schema that said + * `.default(0)`, so "how many times does an under-specified flow retry?" had + * two answers and the winner depended on whether that flow had been through + * `FlowSchema` (0) or hand-built and fed to the engine directly (3). One + * contract, one number: whatever this block parses to is what runs. + * + * The retry knobs are read **only** under `strategy: 'retry'`; `'fail'` and + * `'continue'` ignore them (a fully spelled-out block under `'fail'` is + * common and stays legal). + */ errorHandling: z.object({ strategy: z.enum(['fail', 'retry', 'continue']).default('fail').describe('How to handle node execution errors'), - maxRetries: z.number().int().min(0).max(10).default(0).describe('Number of retry attempts (only for retry strategy)'), + // Default 0 = "no retries", which is the right reading for the two + // strategies that never retry. Under `strategy: 'retry'` it would instead + // mean "retry, zero times" — refused below rather than defaulted to some + // count, because a retry re-runs the WHOLE flow (CRUD side effects and + // all) and nobody should have that number picked for them. + maxRetries: z.number().int().min(0).max(10).default(0) + .describe("Number of retry attempts. Read only under strategy: 'retry', which requires >= 1"), retryDelayMs: z.number().int().min(0).default(1000).describe('Delay between retries in milliseconds'), backoffMultiplier: z.number().min(1).default(1).describe('Multiplier for exponential backoff between retries'), maxRetryDelayMs: z.number().int().min(0).default(30000).describe('Maximum delay between retries in milliseconds'), @@ -446,6 +466,25 @@ export const FlowSchema = lazySchema(() => z.object({ 'configured here silently did not exist. Delete the key and draw a fault edge from ' + 'the failing node to the handler node instead.', ), + }).superRefine((eh, ctx) => { + // `strategy: 'retry'` with 0 attempts is `strategy: 'fail'` wearing a + // different label — the flow runs once and stops. That is a declared + // capability the runtime does not deliver (AGENTS.md Prime Directive #10 + // corollary), and omitting `maxRetries` produced exactly it. Refuse the + // combination in both spellings — written 0 and defaulted 0 — so the + // attempt count is stated wherever retrying is asked for. + if (eh.strategy === 'retry' && eh.maxRetries < 1) { + ctx.addIssue({ + code: 'custom', + path: ['maxRetries'], + message: + "`errorHandling.strategy: 'retry'` requires `maxRetries` >= 1 — retrying zero " + + "times is exactly `strategy: 'fail'`, so a flow that omits the count (it " + + 'defaults to 0) or writes 0 never retries. State the attempts explicitly ' + + "(e.g. `maxRetries: 3`), or use `strategy: 'fail'` if no retry is wanted. " + + 'Note a retry re-runs the WHOLE flow, so side-effecting nodes run again.', + }); + } }).optional().describe('Flow-level error handling configuration'), /** * ADR-0010 §3.7 — Package-level protection envelope. Package diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 6ddd56f826..b4e5bd8c14 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -456,7 +456,21 @@ const step17: MigrationStep = { 'scoped retrieval — absorbs the former topics→sources rename), and ' + "`skill.triggerPhrases` (phrases were never matched; routing is " + 'triggerConditions + the agent allowlist). All pure lossless deletes, each ' + - 'tombstoned at its schema with the prescription.', + 'tombstoned at its schema with the prescription.\n\n' + + 'One flow key changes WITHOUT a lossless target: `errorHandling.maxRetries` ' + + '(#4247). It carried two defaults — `.default(0)` in FlowSchema and ' + + "`maxRetries ?? 3` in the engine's retryExecution — and because `??` fires " + + 'only on `undefined`, an unstated count meant 0 retries for a flow parsed by ' + + 'the schema and 3 for a definition handed to the engine directly: the retry ' + + "count was a function of the route in, not of the authored flow. The engine's " + + 'copy is deleted (it reads the parsed block, no fallback), which makes an ' + + "unstated count unambiguously 0 — and `strategy: 'retry'` that retries zero " + + "times is `strategy: 'fail'` under another name, the declared-not-delivered " + + 'shape ADR-0049 exists to close. The schema therefore requires `maxRetries` ' + + "at least 1 under `'retry'`, in both spellings (omitted, and an explicit 0). " + + 'This is the one v17 flow change the chain cannot apply for you: choosing the ' + + 'count is a judgment about re-running the WHOLE flow with its side effects, ' + + 'so it is a semantic TODO rather than a rewrite.', conversionIds: [ 'action-execute-to-target', 'field-conditionalRequired-to-requiredWhen', @@ -480,6 +494,26 @@ const step17: MigrationStep = { 'skill-trigger-phrases-removed', ], semantic: [ + { + id: 'flow-retry-max-retries-required', + surface: "flow.errorHandling.maxRetries (under strategy: 'retry')", + replacement: 'an explicit count >= 1 (e.g. maxRetries: 3), or strategy: \'fail\'', + reason: + 'maxRetries had two defaults — FlowSchema `.default(0)` and the engine\'s ' + + '`maxRetries ?? 3` — so an unstated count retried 0 times through the schema and 3 ' + + 'times through a hand-built definition (#4247). With the engine\'s copy removed the ' + + 'unstated count is unambiguously 0, and retrying zero times is exactly ' + + "`strategy: 'fail'`, so the schema now refuses the combination instead of it silently " + + 'doing nothing. There is no lossless rewrite: 0 preserves the behaviour a parsed flow ' + + 'got but contradicts what its author wrote, and any positive count is a NEW decision ' + + 'about re-running the whole flow with its side effects. That choice is the author\'s.', + acceptanceCriteria: + "Every flow declaring `errorHandling.strategy: 'retry'` also declares " + + '`maxRetries` >= 1, and each count was chosen knowing a retry replays the flow FROM ' + + 'THE START (records re-created, callouts re-fired); flows that never actually wanted ' + + "retries say `strategy: 'fail'`. No flow fails to register with the maxRetries " + + 'prescription.', + }, { id: 'analytics-query-request-envelope-retired', surface: 'api.analyticsQueryRequest.query',