diff --git a/.changeset/retry-vocab-converge-inline-blocks.md b/.changeset/retry-vocab-converge-inline-blocks.md new file mode 100644 index 0000000000..928e1bc22a --- /dev/null +++ b/.changeset/retry-vocab-converge-inline-blocks.md @@ -0,0 +1,94 @@ +--- +"@objectstack/spec": major +"@objectstack/service-automation": major +--- + +**The retry policy's last two dialects converge** (#4964 `flow.errorHandling`, #4962 +`ETLPipeline.retry`). + +#4661 converged the retry policy onto one declaration. It converged the two shapes that +published the **same exported name** (`RetryPolicy` from `./automation` and `./system` — +the #4411 trap), because that is the question the dual-source instrument asks. Two more +encodings of the identical concept were outside its vision *by construction*: both are +anonymous inline `z.object`s nested in a bigger schema, with no exported name to collide. + +The cost of the gap fell on the author who did the right thing. `shared/retry-policy.zod.ts` +tombstoned `retryDelayMs` and told them to write `backoffMs` — and `flow.errorHandling` +then **rejected** `backoffMs` and demanded `retryDelayMs`. Reading the newer file was +punished, and which file an AI author reads first is arbitrary. + +All four surfaces — `job.retryPolicy`, a `try_catch` node's `retry`, `flow.errorHandling` +and an ETL pipeline's `retry` — now build from one shared shape. + +## FROM → TO + +### `flow.errorHandling` (#4964) + +| | FROM | TO | +|---|---|---| +| base delay | `retryDelayMs`, min 0, default 1000 | **`backoffMs`**, min 0, default 1000 | +| `maxRetries` / `backoffMultiplier` / `maxRetryDelayMs` / `jitter` | *(already identical)* | unchanged | +| `strategy` | `'fail' \| 'retry' \| 'continue'` | unchanged — it selects *whether* the policy runs, so it stays outside it | + +One key, one word, no default changes. Every other key, bound and default already +matched the converged policy, which is exactly why the divergence survived a release: +it looked reviewed. + +### `ETLPipeline.retry` (#4962) + +| | FROM | TO | +|---|---|---| +| count | `maxAttempts`, min 0, **default 3**, unbounded | **`maxRetries`**, 0–**10**, **default 0** | +| base delay | `backoffMs`, default **60000** | `backoffMs`, default **1000** | +| `backoffMultiplier` | *(absent)* | ≥1, default 1 | +| `maxRetryDelayMs` | *(absent)* | default 30000 | +| `jitter` | *(absent)* | default false | + +## What you must change + +**1. Rename `retryDelayMs` → `backoffMs`** in any `flow.errorHandling` block. The value +(milliseconds before the first retry) is unchanged. The old spelling is **tombstoned**, +not deleted, so it rejects with the rename rather than being silently stripped, and +`os migrate meta --from 16` (the `retry-policy-converged` conversion, now with a +flow-level branch) rewrites it for you. + +**2. Rename `maxAttempts` → `maxRetries`** in any `ETLPipeline.retry` block. **The number +does not change** — both counted the retries *after* the initial attempt. Do **not** +subtract one: that adjustment belongs to `integration/connector.zod.ts`'s +identically-spelled `RetryConfig.maxAttempts`, which *includes* the first attempt and is +deliberately **not** part of this convergence. + +**3. If an ETL pipeline relied on the implicit retry count, write it out.** `retry: {}` +used to mean three re-runs 60s apart; it now means **none**. State `maxRetries: 3` (and +`backoffMs: 60000` for the old delay) to keep the old behaviour. + +## Why the ETL default flips to 0 + +Not merely to follow #4661. An ETL destination is a foreign system *by definition* — a +warehouse, an API, someone else's database. A silent retry against a non-idempotent +destination is a **duplicate write**: a second invoice, a second export, a second +webhook. Default 0 makes retrying something an author states, and thereby claims +idempotency for. An unstated key is precisely where LLM-authored metadata hides this. + +## Migration surface + +**`flow.errorHandling`** is live: `service-automation`'s `retryExecution` reads the key +(it now destructures `backoffMs`), and the D2 conversion covers stored and authored +flows, so no deployed stack changes behaviour. + +**`ETLPipeline.retry` has an empty migration surface today, and that is why now was the +moment.** `etl.zod.ts` has no parse site in objectstack / objectui / cloud (批 12's +measurement) and an ETL pipeline is not a `defineStack` collection, so there is no stored +document a conversion could walk — it deliberately gets a tombstone and **no** D2 step, +rather than a walker advertising coverage that does not exist. Once an ETL engine lands, +flipping this default stops being a schema edit and becomes a behaviour change to every +deployed pipeline. + +## Also + +The two automation retry surfaces now carry the **same** curated unknown-key table, so an +author learns one lesson instead of two, and `retry-policy.test.ts` gains a +concept-level guard: all four surfaces are asserted to expose the same key set and the +same defaults, by parse rather than by inspecting how each obtains them. Adding a fifth +retry surface without wiring it to the shared shape now fails a test — which is the check +that would have caught both of these issues, and the one the name-based scan could never be. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index b6794e2549..abe181219c 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -458,7 +458,7 @@ events). try: { nodes: [{ id: 'charge', type: 'http', label: 'Charge', config: { /* … */ } }], edges: [] }, catch: { nodes: [{ id: 'flag', type: 'update_record', label: 'Flag failure', config: { /* … */ } }], edges: [] }, errorVariable: '$error', - retry: { maxRetries: 3, retryDelayMs: 1000, backoffMultiplier: 2 }, + retry: { maxRetries: 3, backoffMs: 1000, backoffMultiplier: 2 }, }, } ``` @@ -956,15 +956,25 @@ Configure how errors are handled during flow execution: errorHandling: { strategy: 'retry', // 'fail' | 'retry' | 'continue' maxRetries: 3, - retryDelayMs: 5000, + backoffMs: 5000, } ``` +The retry knobs are the **one** retry policy the platform has (`RetryPolicySchema`), +shared with `job.retryPolicy`, a `try_catch` node's `retry` and an ETL pipeline's +`retry`. A spelling learned on any one of them is correct on all of them. + + + **17.0.0 breaking:** the base delay here was `retryDelayMs` and is now + `backoffMs` — same value, same meaning. `retryDelayMs` is rejected with the + rename; `os migrate meta --from 16` rewrites it for you. + + | 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 (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`) | +| `backoffMs` | `number` | Base delay before the first retry (ms); subsequent delays multiply by `backoffMultiplier` (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`) | @@ -982,7 +992,7 @@ with `maxRetries: 0` — is refused when the flow is registered: errorHandling: { strategy: 'retry' } // ✅ state the attempts -errorHandling: { strategy: 'retry', maxRetries: 3, retryDelayMs: 5000 } +errorHandling: { strategy: 'retry', maxRetries: 3, backoffMs: 5000 } ``` A retry re-runs the **whole flow from the start**, so every node that already diff --git a/content/docs/references/api/automation-api.mdx b/content/docs/references/api/automation-api.mdx index 1011bdfedc..e19adf0b21 100644 --- a/content/docs/references/api/automation-api.mdx +++ b/content/docs/references/api/automation-api.mdx @@ -109,7 +109,7 @@ const result = AutomationApiErrorCode.parse(data); | **edges** | `{ id: string; source: string; target: string; condition?: string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }; … }[]` | ✅ | Flow connections | | **active** | `any` | optional | [REMOVED] `flow.active` was removed in @objectstack/spec 17.0.0 (#3896 audit close-out) — it never had an effect: the engine arms flows from `status`, and `active: false` did NOT stop a flow (worse, the default read as disabled while the engine treated unset as enabled). Delete the key. Use `status: 'obsolete'` (or 'invalid') to unbind and disable a flow, `status: 'active'` to arm it. | | **runAs** | `Enum<'system' \| 'user'>` | optional | Execution identity for the run: system = elevated (bypasses RLS), user = the triggering user (RLS-respecting). A run with no trigger user has no identity to scope to, so under user its data operations are REFUSED — declare system to make the elevation explicit. This covers schedule/time-relative/api triggers AND any record-change flow fired by a write that carried no user. | -| **errorHandling** | `{ strategy?: Enum<'fail' \| 'retry' \| 'continue'>; maxRetries?: integer; retryDelayMs?: integer; backoffMultiplier?: number; … }` | optional | Flow-level error handling configuration | +| **errorHandling** | `{ strategy?: Enum<'fail' \| 'retry' \| 'continue'>; maxRetries?: integer; backoffMs?: integer; backoffMultiplier?: number; … }` | optional | Flow-level error handling configuration | | **protection** | `{ lock: Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>; reason: string; docsUrl?: string }` | optional | Package author protection block — lock policy for this flow. | | **_lock** | `Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>` | optional | Item-level lock — controls overlay & delete (ADR-0010). | | **_lockReason** | `string` | optional | Human-readable reason shown when a write is refused by _lock. | diff --git a/content/docs/references/automation/etl.mdx b/content/docs/references/automation/etl.mdx index f5554cef69..d609e6515c 100644 --- a/content/docs/references/automation/etl.mdx +++ b/content/docs/references/automation/etl.mdx @@ -189,7 +189,7 @@ const result = ETLDestinationSchema.parse(data); | **syncMode** | `Enum<'full' \| 'incremental' \| 'cdc'>` | optional | Sync mode | | **schedule** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Cron schedule expression | | **enabled** | `boolean` | optional | Pipeline enabled status | -| **retry** | `{ maxAttempts?: integer; backoffMs?: integer }` | optional | Retry configuration | +| **retry** | `{ maxRetries?: integer; backoffMs?: integer; backoffMultiplier?: number; maxRetryDelayMs?: integer; … }` | optional | Retry configuration | | **notifications** | `{ onSuccess?: string[]; onFailure?: string[] }` | optional | Notification settings | | **tags** | `string[]` | optional | Pipeline tags | | **metadata** | `Record` | optional | Custom metadata | diff --git a/content/docs/references/automation/flow.mdx b/content/docs/references/automation/flow.mdx index db8c4a3795..b38566e59c 100644 --- a/content/docs/references/automation/flow.mdx +++ b/content/docs/references/automation/flow.mdx @@ -59,7 +59,7 @@ const result = FlowSchema.parse(data); | **edges** | `{ id: string; source: string; target: string; condition?: string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }; … }[]` | ✅ | Flow connections | | **active** | `any` | optional | [REMOVED] `flow.active` was removed in @objectstack/spec 17.0.0 (#3896 audit close-out) — it never had an effect: the engine arms flows from `status`, and `active: false` did NOT stop a flow (worse, the default read as disabled while the engine treated unset as enabled). Delete the key. Use `status: 'obsolete'` (or 'invalid') to unbind and disable a flow, `status: 'active'` to arm it. | | **runAs** | `Enum<'system' \| 'user'>` | optional | Execution identity for the run: system = elevated (bypasses RLS), user = the triggering user (RLS-respecting). A run with no trigger user has no identity to scope to, so under user its data operations are REFUSED — declare system to make the elevation explicit. This covers schedule/time-relative/api triggers AND any record-change flow fired by a write that carried no user. | -| **errorHandling** | `{ strategy?: Enum<'fail' \| 'retry' \| 'continue'>; maxRetries?: integer; retryDelayMs?: integer; backoffMultiplier?: number; … }` | optional | Flow-level error handling configuration | +| **errorHandling** | `{ strategy?: Enum<'fail' \| 'retry' \| 'continue'>; maxRetries?: integer; backoffMs?: integer; backoffMultiplier?: number; … }` | optional | Flow-level error handling configuration | | **protection** | `{ lock: Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>; reason: string; docsUrl?: string }` | optional | Package author protection block — lock policy for this flow. | | **_lock** | `Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>` | optional | Item-level lock — controls overlay & delete (ADR-0010). | | **_lockReason** | `string` | optional | Human-readable reason shown when a write is refused by _lock. | diff --git a/content/docs/references/automation/retry-policy.mdx b/content/docs/references/automation/retry-policy.mdx index e89e986509..8c3eb418e5 100644 --- a/content/docs/references/automation/retry-policy.mdx +++ b/content/docs/references/automation/retry-policy.mdx @@ -28,7 +28,7 @@ const result = RetryPolicySchema.parse(data); | **backoffMultiplier** | `number` | ✅ | Exponential backoff multiplier; 1 (the default) keeps the delay flat | | **maxRetryDelayMs** | `integer` | ✅ | Ceiling for a single backoff delay (ms) | | **jitter** | `boolean` | ✅ | Randomize each delay within [50%, 100%] of its computed value — spreads a thundering herd of simultaneous retries | -| **retryDelayMs** | `any` | optional | [REMOVED] `retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661) — the retry policy now has one spelling for its base delay across `job.retryPolicy` and a `try_catch` node's `retry`. Rename the key to `backoffMs`; the value (milliseconds before the first retry) is unchanged. `os migrate meta --from 16` rewrites it for you. | +| **retryDelayMs** | `any` | optional | [REMOVED] `retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661, #4964) — the retry policy now has ONE spelling for its base delay across every surface that carries it: `job.retryPolicy`, a `try_catch` node's `retry`, `flow.errorHandling` and an ETL pipeline's `retry`. Rename the key to `backoffMs`; the value (milliseconds before the first retry) is unchanged. `os migrate meta --from 16` rewrites it for you. | --- diff --git a/content/docs/references/system/retry-policy.mdx b/content/docs/references/system/retry-policy.mdx index b8b7822586..fbcd410ded 100644 --- a/content/docs/references/system/retry-policy.mdx +++ b/content/docs/references/system/retry-policy.mdx @@ -28,7 +28,7 @@ const result = RetryPolicySchema.parse(data); | **backoffMultiplier** | `number` | ✅ | Exponential backoff multiplier; 1 (the default) keeps the delay flat | | **maxRetryDelayMs** | `integer` | ✅ | Ceiling for a single backoff delay (ms) | | **jitter** | `boolean` | ✅ | Randomize each delay within [50%, 100%] of its computed value — spreads a thundering herd of simultaneous retries | -| **retryDelayMs** | `any` | optional | [REMOVED] `retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661) — the retry policy now has one spelling for its base delay across `job.retryPolicy` and a `try_catch` node's `retry`. Rename the key to `backoffMs`; the value (milliseconds before the first retry) is unchanged. `os migrate meta --from 16` rewrites it for you. | +| **retryDelayMs** | `any` | optional | [REMOVED] `retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661, #4964) — the retry policy now has ONE spelling for its base delay across every surface that carries it: `job.retryPolicy`, a `try_catch` node's `retry`, `flow.errorHandling` and an ETL pipeline's `retry`. Rename the key to `backoffMs`; the value (milliseconds before the first retry) is unchanged. `os migrate meta --from 16` rewrites it for you. | --- diff --git a/docs/audits/2026-07-unknown-key-strictness-ledger.md b/docs/audits/2026-07-unknown-key-strictness-ledger.md index fbfa8d96d4..0352b6809c 100644 --- a/docs/audits/2026-07-unknown-key-strictness-ledger.md +++ b/docs/audits/2026-07-unknown-key-strictness-ledger.md @@ -619,11 +619,11 @@ not verdicts). | File | Sites | Class | Note | |---|---|---|---| -| `flow.zod.ts` | 11 | authorable | **strict as of #4001** — the four outer authoring shapes at step 1, and **the six nested blocks at batch 11** (`FlowNode.connectorConfig` / `.position` / `.inputSchema` / `.waitEventConfig` / `.boundaryConfig`, `Flow.errorHandling`). The gap between those two dates is this campaign's own finding 17 inside its own file: closing the shells left the gate rejecting `nodee:` at node level while `connectorConfig: { connectorId, actionId, params: {…} }` parsed clean and the executor dispatched `input ?? {}` — a successful connector call carrying nothing. Worth recording precisely, because the obvious example is the wrong one: a slip on a REQUIRED key was always loud (it then reads as missing). What `.strip` swallowed here is the OPTIONAL half — the input map, the retry budget, `interrupting: false`, `required: true` — i.e. exactly the keys an author adds to CONSTRAIN behaviour, replaced by a permissive default without a word. Two things stay open and are now pinned in code with the reason, so a later sweep stops rather than "finishes" the file: the node `config` slot (ADR-0018 plugin namespace) and `FlowVersionHistorySchema` (the file's only WIRE shape — emitted on publish, never authored; its `definition` is `FlowSchema`, so the authored half inside a history record is gated anyway) | -| `etl.zod.ts` | 10 | mixed | **7 strict as of #4001 批 12** — the authoring half (`ETLSource` + `.incremental`, `ETLDestination`, `ETLTransformation`, `ETLPipeline` + `.retry` + `.notifications`). The other 3 — `ETLPipelineRun` + `.stats` + `.error` — are **deliberately left open**: engine-emitted run state (an id it minted, a status it reached, counters it accumulated), same disposition and same reason as `FlowVersionHistorySchema` above and all of `execution.zod.ts`. The exemption is recorded on the schema itself, not only here, because a note only this file carries is a note the next sweep does not read. The old blanket `authorable (p)` was too wide; verification split it. ⚠️ **Read the classification caveat before reusing this verdict**: `etl.zod.ts` has NO parse site in objectstack / objectui / cloud, so neither half could be settled by pointing at a live call. The 7 are authorable because the exported schema and type ARE the door (`SYNC_ARCHITECTURE.md` and the module's `@example` both hand-write `const p: ETLPipeline = { … }`) — the `webhook.zod.ts` posture. The 3 are wire on the shape's semantics plus settled precedent, NOT on an emit site anyone can point at today; if an ETL engine ever lands and a run result turns out to be operator-authored, that verdict is the one to revisit. Two out-of-scope findings were filed rather than fixed here: the `retry` block is a third retry-policy vocabulary #4661's convergence never reached (#4962), and all nine type aliases export the parsed shape under the bare name, which is why the SYNC_ARCHITECTURE.md pipeline examples do not compile (#4963). **−12 at #4738**: `sync.zod.ts` (the L1 "Simple Sync" file — `DataSyncConfig`, its `ConflictResolution` enum and satellites, formerly this row's co-candidate) was deleted whole rather than hardened: three-repo zero importers, no parse site, defs unreachable from the metadata-type roots (#4650 gate), so there was no author for strictness to protect (#4535 C13+C15). The integration-side `ConflictResolution` → `ConnectorConflictResolution` rename in the same change is name-only and moves no sites | +| `flow.zod.ts` | 11 | authorable | **strict as of #4001** — the four outer authoring shapes at step 1, and **the six nested blocks at batch 11** (`FlowNode.connectorConfig` / `.position` / `.inputSchema` / `.waitEventConfig` / `.boundaryConfig`, `Flow.errorHandling`). The gap between those two dates is this campaign's own finding 17 inside its own file: closing the shells left the gate rejecting `nodee:` at node level while `connectorConfig: { connectorId, actionId, params: {…} }` parsed clean and the executor dispatched `input ?? {}` — a successful connector call carrying nothing. Worth recording precisely, because the obvious example is the wrong one: a slip on a REQUIRED key was always loud (it then reads as missing). What `.strip` swallowed here is the OPTIONAL half — the input map, the retry budget, `interrupting: false`, `required: true` — i.e. exactly the keys an author adds to CONSTRAIN behaviour, replaced by a permissive default without a word. `Flow.errorHandling` gained a second chapter at **#4964**: closing it in 批 11 revealed (rather than caused) that its retry keys were a THIRD encoding of the policy #4661 had converged — it spelled the base delay `retryDelayMs` where the shared declaration spells it `backoffMs` and tombstones the old word, so the strictness this row records was, for one release, rejecting an author for having read the newer file. The block now builds from `retryPolicyShape()`. Site count unchanged; only the vocabulary. Two things stay open and are now pinned in code with the reason, so a later sweep stops rather than "finishes" the file: the node `config` slot (ADR-0018 plugin namespace) and `FlowVersionHistorySchema` (the file's only WIRE shape — emitted on publish, never authored; its `definition` is `FlowSchema`, so the authored half inside a history record is gated anyway) | +| `etl.zod.ts` | 10 | mixed | **7 strict as of #4001 批 12** — the authoring half (`ETLSource` + `.incremental`, `ETLDestination`, `ETLTransformation`, `ETLPipeline` + `.retry` + `.notifications`). The other 3 — `ETLPipelineRun` + `.stats` + `.error` — are **deliberately left open**: engine-emitted run state (an id it minted, a status it reached, counters it accumulated), same disposition and same reason as `FlowVersionHistorySchema` above and all of `execution.zod.ts`. The exemption is recorded on the schema itself, not only here, because a note only this file carries is a note the next sweep does not read. The old blanket `authorable (p)` was too wide; verification split it. ⚠️ **Read the classification caveat before reusing this verdict**: `etl.zod.ts` has NO parse site in objectstack / objectui / cloud, so neither half could be settled by pointing at a live call. The 7 are authorable because the exported schema and type ARE the door (`SYNC_ARCHITECTURE.md` and the module's `@example` both hand-write `const p: ETLPipeline = { … }`) — the `webhook.zod.ts` posture. The 3 are wire on the shape's semantics plus settled precedent, NOT on an emit site anyone can point at today; if an ETL engine ever lands and a run result turns out to be operator-authored, that verdict is the one to revisit. Two out-of-scope findings were filed rather than fixed here; **the first is now closed**: the `retry` block was a third retry-policy vocabulary #4661's convergence never reached (#4962 — converged onto `shared/RetryPolicySchema` in the v17 window, together with `flow.errorHandling` (#4964), the fourth. Both were anonymous inline blocks, so the dual-source instrument that drove #4661 could not see them: it asks how many declarations share an exported NAME, and neither has one. 批 12's five curated `retry` entries described that divergence and dissolved with it — the block's site count is unchanged, only its vocabulary). Still open: all nine type aliases export the parsed shape under the bare name, which is why the SYNC_ARCHITECTURE.md pipeline examples do not compile (#4963). **−12 at #4738**: `sync.zod.ts` (the L1 "Simple Sync" file — `DataSyncConfig`, its `ConflictResolution` enum and satellites, formerly this row's co-candidate) was deleted whole rather than hardened: three-repo zero importers, no parse site, defs unreachable from the metadata-type roots (#4650 gate), so there was no author for strictness to protect (#4535 C13+C15). The integration-side `ConflictResolution` → `ConnectorConflictResolution` rename in the same change is name-only and moves no sites | | `execution.zod.ts` | 13 | wire | run-state envelopes — never strict. +5 at #4354 (the run-summary family: step metrics / skip reason / per-node / per-gate / the summary itself) — engine-emitted telemetry read by the Console and by operator queries, nobody authors them, so the `wire` verdict covers them unchanged | | `state-machine.zod.ts` | 6 | authorable | **strict as of #4001 批 10** — all six sites (`ActionRef` / `GuardRef` / `Transition` / `StateNode` + `.meta` / `StateMachine`). **The `(p)` was NOT a formality here.** ADR-0020 retired this XState shape as a *record-lifecycle* declaration — the top-level `workflow` metadata type and `object.stateMachines` are both gone, and a record's transitions live on the `state_machine` VALIDATION RULE instead — so had those been the only doors this file would be DEAD surface, and the correct action would have been to fix its class, not close it. One authoring door survives: `ai/agent.zod.ts`'s `lifecycle` is `StateMachineSchema`, and `agent` is a registered type, so `defineStack({ agents })` / meta REST / the Studio agent form all reach here through `AgentSchema.parse()`. Verified by parse: an agent whose lifecycle carried `stats`, a state with `onn` (one keystroke from `on`) and a `meta` with two unknown keys **parsed clean**, returning a machine with NO transitions at all — the declaration whose whole job is to deny undeclared transitions, silently emptied and reported valid. `.meta` was checked for the #4909 open-slot case and is CLOSED: the hand-written `StateNodeConfig` type declares exactly its four keys (passthrough would open the Zod while `tsc` stayed shut), nothing in the repo reads any `meta` key, and the prior behaviour was strip — an author's `meta` arrived as `{}` — so there was no openness to preserve. ⚠️ `ActionRef` / `GuardRef` are UNIONS: a strict branch's message does not reach the top (zod raises one `invalid_union` whose message is the literal `"Invalid input"`, with the real prescription nested in `issue.errors[]`), which `formatZodError` then flattens away — filed, not fixed here. **−1 at #4658**: the orphan `EventSchema` (`{ type, schema }`, an XState-style signal declaration nothing referenced — `StateMachineSchema` names event types as `on:` record keys) was deleted rather than converged with `kernel/events/core.zod.ts`'s envelope `EventSchema`, whose key set it did not intersect (#4535 C6). The remaining 6 sites and their verdict are unchanged | -| `control-flow.zod.ts` | 5 | authorable | **strict as of #4001 批 10** — all five sites (`FlowRegion` / `Loop` / `ParallelBranch` / `Parallel` / `TryCatch`). The `(p)` resolves to authorable on the executors' own parse seam (`parseNodeConfig`, #4277) plus `validateControlFlow`'s region parse. **`validateControlFlow` is a sibling guard, not a key gate, and the two do not fight**: it answers single-entry / single-exit / acyclic, which no key check can decide, and the schema answers key membership, which no structural check can decide. They meet at exactly one seam — the guard `safeParse`s each region slot before analyzing it, so an undeclared region key now surfaces there as `: invalid region — `, the guard's framing wrapping the schema's prescription. Nothing was duplicated and nothing removed; the guard simply stopped silently repairing its own input before judging it. Two curation entries had to be MEASURED rather than reasoned: the bare edit-distance fallback answers `itemVariable` with **`indexVariable`** — binding the loop INDEX where the author wanted the ITEM — so the alias exists to overrule a confidently wrong suggestion from this campaign's own helper (the `pii` → `min` shape, third instance); and `join`/`joinGateway` needed two DISTINCT prescriptions because `guidance` emits one bullet per key verbatim, so a shared string printed the same paragraph twice. Its test instrument also had to be rebuilt: `region-slots.test.ts` probed every construct with every candidate key at once and depended on `.strip` to discard the mismatches, so it returned "no schema accepts any region" the moment the shapes closed — it failed loudly, which is the only reason this is a footnote and not a fourth finding-3. Structural validation by `validateControlFlow` remains. **−1 at #4661**: `RetryPolicySchema` moved out to `shared/retry-policy.zod.ts` — `./automation` and `./system` published the same name for two different declarations (#4411), so the retry policy converged onto one. The site still exists and is still non-strict and authorable; it is simply no longer in a directory this ledger sections. ⚠️ That is a coverage gap worth knowing about: this audit sections `ui/` / `data/` / `automation/` / `security/` / `studio/` only, so a `shared/` shape is unaudited by construction. The tolerance is deliberate here — the `retryDelayMs` → `backoffMs` rename is tombstoned via `retiredKey()` precisely because a non-strict parent would otherwise swallow the old spelling | +| `control-flow.zod.ts` | 5 | authorable | **strict as of #4001 批 10** — all five sites (`FlowRegion` / `Loop` / `ParallelBranch` / `Parallel` / `TryCatch`). The `(p)` resolves to authorable on the executors' own parse seam (`parseNodeConfig`, #4277) plus `validateControlFlow`'s region parse. **`validateControlFlow` is a sibling guard, not a key gate, and the two do not fight**: it answers single-entry / single-exit / acyclic, which no key check can decide, and the schema answers key membership, which no structural check can decide. They meet at exactly one seam — the guard `safeParse`s each region slot before analyzing it, so an undeclared region key now surfaces there as `: invalid region — `, the guard's framing wrapping the schema's prescription. Nothing was duplicated and nothing removed; the guard simply stopped silently repairing its own input before judging it. Two curation entries had to be MEASURED rather than reasoned: the bare edit-distance fallback answers `itemVariable` with **`indexVariable`** — binding the loop INDEX where the author wanted the ITEM — so the alias exists to overrule a confidently wrong suggestion from this campaign's own helper (the `pii` → `min` shape, third instance); and `join`/`joinGateway` needed two DISTINCT prescriptions because `guidance` emits one bullet per key verbatim, so a shared string printed the same paragraph twice. Its test instrument also had to be rebuilt: `region-slots.test.ts` probed every construct with every candidate key at once and depended on `.strip` to discard the mismatches, so it returned "no schema accepts any region" the moment the shapes closed — it failed loudly, which is the only reason this is a footnote and not a fourth finding-3. Structural validation by `validateControlFlow` remains. **−1 at #4661**: `RetryPolicySchema` moved out to `shared/retry-policy.zod.ts` — `./automation` and `./system` published the same name for two different declarations (#4411), so the retry policy converged onto one. The site still exists and is still non-strict and authorable; it is simply no longer in a directory this ledger sections. ⚠️ That is a coverage gap worth knowing about: this audit sections `ui/` / `data/` / `automation/` / `security/` / `studio/` only, so a `shared/` shape is unaudited by construction. The tolerance is deliberate here — the `retryDelayMs` → `backoffMs` rename is tombstoned via `retiredKey()` precisely because a non-strict parent would otherwise swallow the old spelling. **#4964 widened that rename to `flow.errorHandling`**, which spelled the base delay the pre-17 way while the shared policy tombstoned it — so the two automation retry surfaces now teach the same word, and the tombstone's prescription names all four surfaces instead of the two #4661 could see | | `bpmn-interop.zod.ts` | 5 | wire (p) | interop import shapes | | `approval.zod.ts` | 4 | authorable | **strict as of #4001 step 3** — all four authoring schemas (node config / approver / escalation / decision-output). The published JSON schema carries `additionalProperties: false` into the Studio form AND `registerFlow()` config validation (#4027/#4040), so an unknown key in an approval node's `config` is rejected at registration too — verified: `z.toJSONSchema` on the strict lazySchema does not throw (#3746 hazard checked) | | `node-executor.zod.ts` | 4 | wire | executor contract | @@ -702,8 +702,8 @@ is complete and so nobody re-triages them from scratch next batch. | File | Strip | Sites | Class | Batch | |---|---|---|---|---| | `execution.zod.ts` | 13 | 13 | wire | **out of scope** — engine-emitted run state; the ledger row already says "never strict" | -| `etl.zod.ts` | 3 | 10 | wire | **Authorable half closed at 批 12** (7 sites: `ETLSource` + `.incremental`, `ETLDestination`, `ETLTransformation`, `ETLPipeline` + `.retry` + `.notifications`). What is left is `ETLPipelineRun` + `.stats` + `.error` — engine-emitted run state, exempt for the `FlowVersionHistorySchema` reason and pinned as such in `etl.test.ts`, so closing it means deleting a test that says not to. **This row shrinks without disappearing** — the second in `automation/` to do so, after `flow.zod.ts` reached its own wire floor of 1 at 批 11 (the two batches were in flight together and arrived at the same shape independently, which is the better evidence that it is the right one). Worth naming because the reverse pin cannot see it: the pin fires on zero, so a row that stops at its wire floor looks exactly like a row nobody finished. The Class column is the only thing separating them — read it before treating this as unfinished work | -| `flow.zod.ts` | 1 | 11 | wire | **batch 11 closed the 6 authorable** (`FlowNode.connectorConfig` / `.position` / `.inputSchema` / `.waitEventConfig` / `.boundaryConfig`, `Flow.errorHandling`). The 1 left is `FlowVersionHistorySchema`, which this table has exempted since it was written — **do not close it**: it is emitted on publish, not authored, so closing it makes a future emitter-side field a parse failure for whoever reads history. The exemption now also lives beside the schema and in `flow.test.ts`, because a row in a table is not where the next person to open that file will look | +| `etl.zod.ts` | 3 | 10 | wire | **Authorable half closed at 批 12** (7 sites: `ETLSource` + `.incremental`, `ETLDestination`, `ETLTransformation`, `ETLPipeline` + `.retry` + `.notifications`). `.retry` was re-pointed at the shared `RetryPolicySchema` at #4962 (`maxAttempts` → `maxRetries`, default 3 → 0, three knobs gained) — a vocabulary change inside an already-closed site, so this row's numbers do not move. What is left is `ETLPipelineRun` + `.stats` + `.error` — engine-emitted run state, exempt for the `FlowVersionHistorySchema` reason and pinned as such in `etl.test.ts`, so closing it means deleting a test that says not to. **This row shrinks without disappearing** — the second in `automation/` to do so, after `flow.zod.ts` reached its own wire floor of 1 at 批 11 (the two batches were in flight together and arrived at the same shape independently, which is the better evidence that it is the right one). Worth naming because the reverse pin cannot see it: the pin fires on zero, so a row that stops at its wire floor looks exactly like a row nobody finished. The Class column is the only thing separating them — read it before treating this as unfinished work | +| `flow.zod.ts` | 1 | 11 | wire | **batch 11 closed the 6 authorable** (`FlowNode.connectorConfig` / `.position` / `.inputSchema` / `.waitEventConfig` / `.boundaryConfig`, `Flow.errorHandling`). (`Flow.errorHandling`'s retry keys were re-pointed at the shared `RetryPolicySchema` at #4964 — a vocabulary change inside an already-closed site, so this row's numbers do not move.) The 1 left is `FlowVersionHistorySchema`, which this table has exempted since it was written — **do not close it**: it is emitted on publish, not authored, so closing it makes a future emitter-side field a parse failure for whoever reads history. The exemption now also lives beside the schema and in `flow.test.ts`, because a row in a table is not where the next person to open that file will look | | `bpmn-interop.zod.ts` | 5 | 5 | wire (p) | **out of scope** — third-party BPMN import/export shapes; strictness turns an upstream addition into our parse crash | | `node-executor.zod.ts` | 4 | 4 | wire | **out of scope** — executor registration contract, code-to-code | diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 729ee9fd9f..270e9c45a6 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -176,6 +176,8 @@ The same window converges the retry policy (#4661). `@objectstack/spec/automatio The subtle half is the defaults, and it is worth stating because no gate can see it: `job.retryPolicy` defaulted `maxRetries: 3` / `backoffMultiplier: 2` while the automation shape defaulted 0 / 1, and the authorable-surface gate compares KEY SETS — a changed default is invisible to it, to the tombstone mechanism and to `spec_changes` alike. The merged declaration takes 0 / 1 (retry replays side effects, so it is opt-in — the same reading already recorded in `flow-retry-max-retries-required`), and the conversion writes the pre-17 numbers into every existing `job.retryPolicy` that omitted them. Deployed stacks therefore keep their exact behaviour; what changes is only what a NEWLY authored omission means. +That convergence then had to be finished twice more, and WHY it was incomplete is the part worth carrying forward (#4964, #4962). It was driven by the dual-source instrument, which asks "how many declarations publish the same exported NAME?" — so it could not see the two encodings of the identical policy that have no exported name at all, being anonymous inline blocks nested in a bigger schema: `flow.errorHandling` and `ETLPipeline.retry`. The instrument was not broken and answered its own question exactly; that question was simply not "how many shapes does this ONE concept have?", which is what everybody read off it. The cost of the gap is concrete and falls on the author who did the right thing: `shared/retry-policy.zod.ts` tombstoned `retryDelayMs` and told them to write `backoffMs`, and `flow.errorHandling` then rejected `backoffMs` and demanded `retryDelayMs` — reading the newer file was punished. Both blocks now build from one shared shape. `flow.errorHandling` costs nothing beyond the same `retryDelayMs` → `backoffMs` rename (every other key, bound and default already matched, which is exactly why it looked reviewed), and the conversion covers it. `ETLPipeline.retry` costs a rename of the COUNT — `maxAttempts` → `maxRetries`, same number, do NOT subtract one: that adjustment belongs to `integration/connector.zod.ts`'s identically-spelled `RetryConfig.maxAttempts`, which INCLUDES the first attempt — plus the same default flip (3 → 0) and three keys it never had (`backoffMultiplier` / `maxRetryDelayMs` / `jitter`, so a nightly warehouse pipeline can stop retrying flat, uncapped and unjittered every 60s). The ETL half gets a tombstone and no conversion step, deliberately: an ETL pipeline is not a `defineStack` collection and `etl.zod.ts` has no parse site in any of the three repos, so there is no stored document to walk and a step for it would advertise coverage it does not have. Nothing deployed moves; the migration surface is empty and this is the cheapest this convergence will ever be. + The same enforce-or-remove pass reaches the event vocabulary: `DataEventType` drops `data.field.changed` (#4673). It had no producer anywhere — the engine emits `data.record.{created,updated,deleted}` and, since #4639, `data.records.{updated,deleted}` — so a subscriber switching on it held a branch that could never run, and the `switch` still compiled, which is why an empty member could sit in a public enum this long. It could not have been implemented against this contract as written: `DataEventSchema` is record-shaped and has no `field` / `oldValue` / `newValue` slot, so the member advertised a granularity the payload has no room for. Nothing is lost — per-field detail already rides on `data.record.updated` as `changes` (with `before` / `after`), one event per write instead of N on a wide table. Like the driver contract above it is a runtime surface, never stored in stack metadata, so it is one semantic TODO for event consumers rather than a source rewrite, and it carries no tombstone: a removed enum VALUE cannot hold a fix-it error, exactly as the sharing-rule `full` retirement noted. Should a real per-field stream ever be wanted, it earns its own contract on the #4639 precedent rather than reclaiming this slot. The object capability block closes out the same ADR-0049 pass: `enable.trash` and `enable.mru` left the schema in the 16.x line (#3207, the #2377 close-out — every delete has always been a hard delete and MRU tracking was never implemented, so both default-true flags gated nothing), and the `.strict()` capabilities block rejects them with the prescription. This step registers the migration surface that removal was missing: stored 16.x rows replay clean instead of flagging `metadata_spec_invalid`, and `os migrate meta --from 16` rewrites authored sources. Soft delete stays parked at #3146; if built it returns as a live enforced flag rather than by reviving these keys. @@ -233,7 +235,7 @@ Finally it CONVERGES `dashboard.widgets[].compareTo` (#5011) — the one entry i | `translation-validation-messages-removed` | `translation.validationMessages` | translation key 'validationMessages' removed (#4667 — no resolver read it, so a translated rule message was stored and never shown; #3778's migration table had been steering retired `errors:` authors into it). Author the message on the rule itself (`object.validations[].message`) | retired — `migrate meta` only | | `datasource-config-driver-key-aliases` | `datasource.config` | datasource config keys → canonical per driver: sqlite 'file'/'database' → 'filename', postgres/mysql 'connectionString' → 'url' and 'user' → 'username', mongo 'uri' → 'url' and 'user' → 'username' (#4456 — driver-factory `??` fallback graduation) | retired — `migrate meta` only | | `flow-node-script-branch-keys-removed` | `flow.node.script.config.actionType / flow.node.script.config.template / flow.node.script.config.recipients / flow.node.script.config.variables / flow.node.script.config.script` | script flow-node config keys 'actionType' (→ 'function' when it was shorthand for one; otherwise removed — 'email'/'slack' were logger-backed stubs that delivered nothing), plus 'template' / 'recipients' / 'variables' (fed those stubs) and 'script' (inline JS the runtime never executed) (#4343) | retired — `migrate meta` only | -| `retry-policy-converged` | `flow.node.config.retry.retryDelayMs / job.retryPolicy.maxRetries / job.retryPolicy.backoffMultiplier` | retry policy unified across job.retryPolicy and try_catch retry: base delay 'retryDelayMs' → 'backoffMs', and the pre-17 job defaults (maxRetries 3, backoffMultiplier 2) written out explicitly now that the merged default is 0 / 1 (#4661) | live — protocol 17 loader accepts the old shape | +| `retry-policy-converged` | `flow.errorHandling.retryDelayMs / flow.node.config.retry.retryDelayMs / job.retryPolicy.maxRetries / job.retryPolicy.backoffMultiplier` | retry policy unified across job.retryPolicy, try_catch retry and flow.errorHandling: base delay 'retryDelayMs' → 'backoffMs', and the pre-17 job defaults (maxRetries 3, backoffMultiplier 2) written out explicitly now that the merged default is 0 / 1 (#4661, #4964) | live — protocol 17 loader accepts the old shape | | `object-managed-by-system-to-system-data` | `object.managedBy` | object managedBy 'system' → 'system-data' (#3355 — ADR-0103's residual bucket named the engine-owned half v16 had already moved out to `engine-owned`; the rename leaves the name describing what the bucket actually holds: admin/user-writable platform data) | retired — `migrate meta` only | | `object-enable-trash-mru-removed` | `object.enable.trash / object.enable.mru` | object capability flags 'enable.trash'/'enable.mru' removed (#3207, #2377 close-out — no recycle bin and no MRU tracking ever ran; both default-true flags gated nothing) | retired — `migrate meta` only | | `hook-body-crypto-hash-removed` | `hook.body.capabilities / action.body.capabilities` | script-body capability token 'crypto.hash' removed (#4391 — the sandbox never installed ctx.crypto.hash, so the token granted a call that always threw; the CLI inferred it too) | retired — `migrate meta` only | @@ -247,6 +249,9 @@ Finally it CONVERGES `dashboard.widgets[].compareTo` (#5011) — the one entry i - **`job-retry-policy-constraints-tightened`** — `job.retryPolicy.maxRetries (> 10) / job.retryPolicy.backoffMultiplier (< 1)` → maxRetries <= 10, and backoffMultiplier >= 1 - Why not automatic: The converged RetryPolicy (#4661) keeps the automation side's bounds, which the job side never had: `maxRetries` is capped at 10 and `backoffMultiplier` floored at 1. Neither has a lossless rewrite. Clamping `maxRetries: 20` to 10 would halve a retry budget its author chose, and a `backoffMultiplier` below 1 describes a delay that SHRINKS on each attempt — retrying a failing dependency ever faster, which is the opposite of backoff and was never a shape the engine meant to offer. Both now fail at parse time with the bound named, rather than being silently reinterpreted. Choosing the replacement count (or accepting the cap) is the author's call. - Done when: Every job declaring `retryPolicy` parses: no `maxRetries` above 10 and no `backoffMultiplier` below 1 remain, and each adjusted value was re-chosen knowing a retry re-runs the handler with its writes and callouts. No job fails to register with the retry-policy bound prescription. +- **`etl-retry-converged-onto-retry-policy`** — `etlPipeline.retry.maxAttempts (and any count above 10)` → maxRetries, same number — plus an explicit count if you relied on the old default of 3 + - Why not automatic: An ETL pipeline's `retry` was a THIRD retry vocabulary that #4661's convergence never reached, because that pass was driven by duplicated exported NAMES and this block is an anonymous inline object (#4962). It now carries the shared `RetryPolicySchema` contract, which changes three things with no single lossless rewrite between them. The rename `maxAttempts` → `maxRetries` IS lossless and the tombstone performs it — both keys counted the retries AFTER the initial attempt, so the number does not change, and subtracting one (correct for `integration/connector.zod.ts`'s identically-spelled `RetryConfig.maxAttempts`, which includes the first attempt) would silently run one attempt fewer than asked. What needs a human: the count now DEFAULTS TO 0 instead of 3, so a pipeline that wrote `retry: {}` or omitted the count bought three silent re-runs and now buys none. That is deliberate and the business case is the destination — an ETL destination is a foreign system by definition, and an implicit retry against a non-idempotent one is a duplicate write (a second invoice, a second export, a second webhook). Retrying is now something an author states and thereby claims idempotency for. The shared contract also caps `maxRetries` at 10, which this block never did; clamping a larger budget would silently halve a number its author chose, so it fails at parse with the bound named instead. + - Done when: No ETL pipeline declares `retry.maxAttempts`; every one that wants retries declares `maxRetries` >= 1 explicitly (the number carried over unchanged from `maxAttempts`), and every pipeline that was relying on the old implicit 3 has either written `maxRetries: 3` or been re-decided against the duplicate-write risk at its destination. No count exceeds 10. Pipelines that want the old flat 60s backoff state `backoffMs: 60000` explicitly, since the shared default is 1000. - **`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. diff --git a/examples/app-crm/src/flows/convert-lead.flow.ts b/examples/app-crm/src/flows/convert-lead.flow.ts index f8106daa3c..7246c69eb2 100644 --- a/examples/app-crm/src/flows/convert-lead.flow.ts +++ b/examples/app-crm/src/flows/convert-lead.flow.ts @@ -175,7 +175,7 @@ export const ConvertLeadScreenFlow = defineFlow({ errorHandling: { strategy: 'fail', maxRetries: 0, - retryDelayMs: 0, + backoffMs: 0, backoffMultiplier: 1, maxRetryDelayMs: 0, jitter: false, diff --git a/packages/services/service-automation/README.md b/packages/services/service-automation/README.md index 41dfd1f9cc..87baf215bd 100644 --- a/packages/services/service-automation/README.md +++ b/packages/services/service-automation/README.md @@ -310,7 +310,7 @@ author-visible split/join gateway. try: { nodes: [{ id: 'charge', type: 'http', label: 'Charge', config: { /* … */ } }], edges: [] }, catch: { nodes: [{ id: 'flag', type: 'update_record', label: 'Flag failure', config: { /* … */ } }], edges: [] }, errorVariable: '$error', - retry: { maxRetries: 3, retryDelayMs: 1000, backoffMultiplier: 2 }, + retry: { maxRetries: 3, backoffMs: 1000, backoffMultiplier: 2 }, }, } ``` diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 950c1f02b9..a7223b33b2 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -4645,9 +4645,14 @@ export class AutomationEngine implements IAutomationService { // `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. + // `backoffMs` (was `retryDelayMs`) since spec 17.0.0 — `errorHandling` + // now carries the converged `RetryPolicySchema` contract, so this reads + // the same key the `try_catch` executor and `runWithPolicy` read + // (#4661, #4964). Destructured, not `??`-defaulted: the parsed block is + // the only source of these numbers (#4247). const { maxRetries, - retryDelayMs: baseDelay, + backoffMs: baseDelay, backoffMultiplier: multiplier, maxRetryDelayMs: maxDelay, jitter: useJitter, diff --git a/packages/services/service-automation/src/fault-edge-guard-containment.test.ts b/packages/services/service-automation/src/fault-edge-guard-containment.test.ts index 45155ac882..7dc2965092 100644 --- a/packages/services/service-automation/src/fault-edge-guard-containment.test.ts +++ b/packages/services/service-automation/src/fault-edge-guard-containment.test.ts @@ -322,7 +322,7 @@ describe('#3863 — a handled failure does not trigger flow-level retry', () => name: 'handled_no_retry', label: 'Handled No Retry', type: 'autolaunched', - errorHandling: { strategy: 'retry', maxRetries: 3, retryDelayMs: 1 }, + errorHandling: { strategy: 'retry', maxRetries: 3, backoffMs: 1 }, nodes: [ { id: 'start', type: 'start', label: 'Start' }, { id: 'upstream', type: 'script' as any, label: 'Upstream' }, 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 index 0fde446a80..a76fd9338e 100644 --- a/packages/services/service-automation/src/flow-retry-attempt-count.test.ts +++ b/packages/services/service-automation/src/flow-retry-attempt-count.test.ts @@ -66,7 +66,7 @@ describe('#4247 — the retry count comes from the flow, not from the engine', ( const { engine, runs, register } = failingFlowEngine({ strategy: 'retry', maxRetries: 2, - retryDelayMs: 0, + backoffMs: 0, }); register(); @@ -82,7 +82,7 @@ describe('#4247 — the retry count comes from the flow, not from the engine', ( const { engine, runs, register } = failingFlowEngine({ strategy: 'retry', maxRetries: 1, - retryDelayMs: 0, + backoffMs: 0, }); register(); @@ -92,7 +92,7 @@ describe('#4247 — the retry count comes from the flow, not from the engine', ( }); it("refuses `strategy: 'retry'` with no maxRetries instead of guessing a count", () => { - const { register } = failingFlowEngine({ strategy: 'retry', retryDelayMs: 0 }); + const { register } = failingFlowEngine({ strategy: 'retry', backoffMs: 0 }); // The pre-#4247 outcomes were "registers, never retries" (schema route) // and "registers, retries 3×" (direct route). Now it is neither: the @@ -102,7 +102,7 @@ describe('#4247 — the retry count comes from the flow, not from the engine', ( }); it("refuses `strategy: 'retry'` with an explicit maxRetries: 0 — that is `'fail'`", () => { - const { register } = failingFlowEngine({ strategy: 'retry', maxRetries: 0, retryDelayMs: 0 }); + const { register } = failingFlowEngine({ strategy: 'retry', maxRetries: 0, backoffMs: 0 }); expect(register).toThrow(/maxRetries/); }); @@ -111,7 +111,7 @@ describe('#4247 — the retry count comes from the flow, not from the engine', ( const { engine, runs, register } = failingFlowEngine({ strategy: 'fail', maxRetries: 0, - retryDelayMs: 0, + backoffMs: 0, backoffMultiplier: 1, maxRetryDelayMs: 0, jitter: false, @@ -128,7 +128,7 @@ describe('#4247 — the retry count comes from the flow, not from the engine', ( const { engine, register } = failingFlowEngine({ strategy: 'retry', maxRetries: 1, - retryDelayMs: 0, + backoffMs: 0, }); register(); @@ -141,7 +141,7 @@ describe('#4247 — the retry count comes from the flow, not from the engine', ( expect(stored?.errorHandling).toMatchObject({ strategy: 'retry', maxRetries: 1, - retryDelayMs: 0, + backoffMs: 0, backoffMultiplier: 1, maxRetryDelayMs: 30000, jitter: false, diff --git a/packages/spec/liveness/flow.json b/packages/spec/liveness/flow.json index 45aa5fd582..5a2bcdcf1b 100644 --- a/packages/spec/liveness/flow.json +++ b/packages/spec/liveness/flow.json @@ -118,9 +118,16 @@ "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": { + "backoffMs": { "status": "live", - "evidence": "packages/services/service-automation/src/engine.ts" + "verifiedAt": "2026-08-04", + "evidence": "packages/services/service-automation/src/engine.ts", + "note": "Was `retryDelayMs` until 17.0.0 (#4964). This block was a THIRD encoding of the retry policy #4661 converged, invisible to that pass because it is an anonymous inline block with no exported name — the dual-source instrument judges duplicated export NAMES. It now builds from `retryPolicyShape()`, so the base delay is spelled `backoffMs` here exactly as on `job.retryPolicy` and a `try_catch` node's `retry`; `retryDelayMs` is tombstoned and rewritten by `retry-policy-converged`. `retryExecution` destructures the parsed key." + }, + "retryDelayMs": { + "status": "dead", + "verifiedAt": "2026-08-04", + "note": "RENAMED to `backoffMs` 2026-08-04 (#4964) — the flow-level retry keys converged onto the shared `RetryPolicySchema`, whose tombstone this key now is (it arrives here via `retryPolicyShape()`, so authoring it is a tsc error and a parse error carrying the rename). Rewritten on the load path by the `retry-policy-converged` conversion. The row stays because retiredKey keeps the key in the walked shape (the rls.priority precedent). Note this key was LIVE right up to the rename — `retryExecution` read it — so `dead` here means retired, not never-read." }, "backoffMultiplier": { "status": "live", diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index f33b19f252..a31c063975 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -279,8 +279,8 @@ "toMajor": 17 }, { - "surface": "flow.node.config.retry.retryDelayMs / job.retryPolicy.maxRetries / job.retryPolicy.backoffMultiplier", - "to": "retry policy unified across job.retryPolicy and try_catch retry: base delay 'retryDelayMs' → 'backoffMs', and the pre-17 job defaults (maxRetries 3, backoffMultiplier 2) written out explicitly now that the merged default is 0 / 1 (#4661)", + "surface": "flow.errorHandling.retryDelayMs / flow.node.config.retry.retryDelayMs / job.retryPolicy.maxRetries / job.retryPolicy.backoffMultiplier", + "to": "retry policy unified across job.retryPolicy, try_catch retry and flow.errorHandling: base delay 'retryDelayMs' → 'backoffMs', and the pre-17 job defaults (maxRetries 3, backoffMultiplier 2) written out explicitly now that the merged default is 0 / 1 (#4661, #4964)", "conversionId": "retry-policy-converged", "toMajor": 17 }, @@ -394,6 +394,13 @@ "toMajor": 17, "rationale": "The converged RetryPolicy (#4661) keeps the automation side's bounds, which the job side never had: `maxRetries` is capped at 10 and `backoffMultiplier` floored at 1. Neither has a lossless rewrite. Clamping `maxRetries: 20` to 10 would halve a retry budget its author chose, and a `backoffMultiplier` below 1 describes a delay that SHRINKS on each attempt — retrying a failing dependency ever faster, which is the opposite of backoff and was never a shape the engine meant to offer. Both now fail at parse time with the bound named, rather than being silently reinterpreted. Choosing the replacement count (or accepting the cap) is the author's call." }, + { + "surface": "etlPipeline.retry.maxAttempts (and any count above 10)", + "replacement": "maxRetries, same number — plus an explicit count if you relied on the old default of 3", + "migrationId": "etl-retry-converged-onto-retry-policy", + "toMajor": 17, + "rationale": "An ETL pipeline's `retry` was a THIRD retry vocabulary that #4661's convergence never reached, because that pass was driven by duplicated exported NAMES and this block is an anonymous inline object (#4962). It now carries the shared `RetryPolicySchema` contract, which changes three things with no single lossless rewrite between them. The rename `maxAttempts` → `maxRetries` IS lossless and the tombstone performs it — both keys counted the retries AFTER the initial attempt, so the number does not change, and subtracting one (correct for `integration/connector.zod.ts`'s identically-spelled `RetryConfig.maxAttempts`, which includes the first attempt) would silently run one attempt fewer than asked. What needs a human: the count now DEFAULTS TO 0 instead of 3, so a pipeline that wrote `retry: {}` or omitted the count bought three silent re-runs and now buys none. That is deliberate and the business case is the destination — an ETL destination is a foreign system by definition, and an implicit retry against a non-idempotent one is a duplicate write (a second invoice, a second export, a second webhook). Retrying is now something an author states and thereby claims idempotency for. The shared contract also caps `maxRetries` at 10, which this block never did; clamping a larger budget would silently halve a number its author chose, so it fails at parse with the bound named instead." + }, { "surface": "flow.errorHandling.maxRetries (under strategy: 'retry')", "replacement": "an explicit count >= 1 (e.g. maxRetries: 3), or strategy: 'fail'", @@ -934,8 +941,8 @@ "toMajor": 17 }, { - "surface": "flow.node.config.retry.retryDelayMs / job.retryPolicy.maxRetries / job.retryPolicy.backoffMultiplier", - "to": "retry policy unified across job.retryPolicy and try_catch retry: base delay 'retryDelayMs' → 'backoffMs', and the pre-17 job defaults (maxRetries 3, backoffMultiplier 2) written out explicitly now that the merged default is 0 / 1 (#4661)", + "surface": "flow.errorHandling.retryDelayMs / flow.node.config.retry.retryDelayMs / job.retryPolicy.maxRetries / job.retryPolicy.backoffMultiplier", + "to": "retry policy unified across job.retryPolicy, try_catch retry and flow.errorHandling: base delay 'retryDelayMs' → 'backoffMs', and the pre-17 job defaults (maxRetries 3, backoffMultiplier 2) written out explicitly now that the merged default is 0 / 1 (#4661, #4964)", "conversionId": "retry-policy-converged", "toMajor": 17 }, @@ -979,6 +986,13 @@ "toMajor": 17, "rationale": "The converged RetryPolicy (#4661) keeps the automation side's bounds, which the job side never had: `maxRetries` is capped at 10 and `backoffMultiplier` floored at 1. Neither has a lossless rewrite. Clamping `maxRetries: 20` to 10 would halve a retry budget its author chose, and a `backoffMultiplier` below 1 describes a delay that SHRINKS on each attempt — retrying a failing dependency ever faster, which is the opposite of backoff and was never a shape the engine meant to offer. Both now fail at parse time with the bound named, rather than being silently reinterpreted. Choosing the replacement count (or accepting the cap) is the author's call." }, + { + "surface": "etlPipeline.retry.maxAttempts (and any count above 10)", + "replacement": "maxRetries, same number — plus an explicit count if you relied on the old default of 3", + "migrationId": "etl-retry-converged-onto-retry-policy", + "toMajor": 17, + "rationale": "An ETL pipeline's `retry` was a THIRD retry vocabulary that #4661's convergence never reached, because that pass was driven by duplicated exported NAMES and this block is an anonymous inline object (#4962). It now carries the shared `RetryPolicySchema` contract, which changes three things with no single lossless rewrite between them. The rename `maxAttempts` → `maxRetries` IS lossless and the tombstone performs it — both keys counted the retries AFTER the initial attempt, so the number does not change, and subtracting one (correct for `integration/connector.zod.ts`'s identically-spelled `RetryConfig.maxAttempts`, which includes the first attempt) would silently run one attempt fewer than asked. What needs a human: the count now DEFAULTS TO 0 instead of 3, so a pipeline that wrote `retry: {}` or omitted the count bought three silent re-runs and now buys none. That is deliberate and the business case is the destination — an ETL destination is a foreign system by definition, and an implicit retry against a non-idempotent one is a duplicate write (a second invoice, a second export, a second webhook). Retrying is now something an author states and thereby claims idempotency for. The shared contract also caps `maxRetries` at 10, which this block never did; clamping a larger budget would silently halve a number its author chose, so it fails at parse with the bound named instead." + }, { "surface": "flow.errorHandling.maxRetries (under strategy: 'retry')", "replacement": "an explicit count >= 1 (e.g. maxRetries: 3), or strategy: 'fail'", diff --git a/packages/spec/src/automation/etl.test.ts b/packages/spec/src/automation/etl.test.ts index f42423d628..8567bbb5ac 100644 --- a/packages/spec/src/automation/etl.test.ts +++ b/packages/spec/src/automation/etl.test.ts @@ -183,7 +183,7 @@ describe('ETLPipelineSchema', () => { syncMode: 'incremental', schedule: '0 2 * * *', enabled: true, - retry: { maxAttempts: 5, backoffMs: 120000 }, + retry: { maxRetries: 5, backoffMs: 120000 }, notifications: { onSuccess: ['data-team@example.com'], onFailure: ['ops@example.com'], @@ -207,13 +207,46 @@ describe('ETLPipelineSchema', () => { })).toThrow(); }); - it('should apply retry defaults when provided', () => { + /** + * #4962 — retry is OPT-IN, and this is the assertion that says so. + * + * Until 17 this block defaulted `maxAttempts: 3` / `backoffMs: 60000`, so + * `retry: {}` bought three silent re-runs a minute apart. It now carries the + * converged `RetryPolicySchema` contract, whose count defaults to 0. The + * business ground is the destination: an ETL destination is a foreign system + * by definition, so an implicit retry against a non-idempotent one is a + * duplicate write. Nothing deployed moves — `etl.zod.ts` has no parse site + * and an ETL pipeline is not a `defineStack` collection — which is exactly + * why this was the cheapest moment to fix the direction. + */ + it('defaults the retry count to 0 — declaring the block does not buy retries (#4962)', () => { const result = ETLPipelineSchema.parse({ ...minimalPipeline, retry: {}, }); - expect(result.retry?.maxAttempts).toBe(3); - expect(result.retry?.backoffMs).toBe(60000); + expect(result.retry?.maxRetries).toBe(0); + expect(result.retry?.backoffMs).toBe(1000); + }); + + it('accepts the three knobs this block never had before the convergence (#4962)', () => { + // `backoffMultiplier` / `maxRetryDelayMs` / `jitter` were 批 12's + // "documented ABSENCE" guidance entries — a nightly warehouse pipeline + // could only retry flat, uncapped and unjittered, the textbook + // thundering herd. + const result = ETLPipelineSchema.parse({ + ...minimalPipeline, + retry: { maxRetries: 3, backoffMs: 60000, backoffMultiplier: 2, maxRetryDelayMs: 600000, jitter: true }, + }); + expect(result.retry).toMatchObject({ + maxRetries: 3, backoffMs: 60000, backoffMultiplier: 2, maxRetryDelayMs: 600000, jitter: true, + }); + }); + + it('caps the retry count at 10, the shared contract\'s bound (#4962)', () => { + // The old inline block had no upper bound. Clamping silently would halve a + // budget its author chose, so the bound is refused at parse instead. + expect(() => ETLPipelineSchema.parse({ ...minimalPipeline, retry: { maxRetries: 11 } })).toThrow(); + expect(() => ETLPipelineSchema.parse({ ...minimalPipeline, retry: { maxRetries: 10 } })).not.toThrow(); }); }); @@ -337,7 +370,7 @@ const VALID_PIPELINE = { syncMode: 'incremental', schedule: '0 2 * * *', enabled: true, - retry: { maxAttempts: 5, backoffMs: 120000 }, + retry: { maxRetries: 5, backoffMs: 120000 }, notifications: { onSuccess: ['data@example.com'], onFailure: ['ops@example.com'] }, tags: ['analytics'], metadata: { owner: 'data-team' }, @@ -449,19 +482,55 @@ describe('[#4001 批 12] curated prescriptions — each anchored to a sibling co expect(rejectionFor(['notifications'], 'onError')).toContain('`onError` → `onFailure`'); }); - it('renames `maxRetries` to `maxAttempts`, and points `retryDelayMs` at `backoffMs`', () => { - expect(rejectionFor(['retry'], 'maxRetries')).toContain('`maxRetries` → `maxAttempts`'); + /** + * The four 批 12 curation entries that #4962 DISSOLVED, asserted from the + * other side so a regression reads as a failure rather than as silence. + * + * 批 12 could only make this divergence audible: `maxRetries` was aliased + * *to* `maxAttempts` (pointing authors away from the canonical spelling), and + * `backoffMultiplier` / `maxRetryDelayMs` / `jitter` each carried a + * "documented ABSENCE" guidance entry. Convergence removes the divergence the + * entries described, so the entries had to go with it — a curated message + * outliving the shape it describes is worse than none, because it is + * confidently wrong. + */ + it('no longer points `maxRetries` at `maxAttempts` — the alias inverted (#4962)', () => { + // `maxRetries` is now a DECLARED key: writing it must parse, not suggest. + const result = ETLPipelineSchema.safeParse(pipelineWith(['retry'], 'maxRetries', 3)); + expect(result.success, result.success ? '' : JSON.stringify(result.error.issues)).toBe(true); + }); + + it('tombstones `maxAttempts` with the rename AND the off-by-one warning (#4962)', () => { + const retired = rejectionFor(['retry'], 'maxAttempts'); + expect(retired).toContain('was removed'); + expect(retired).toContain('maxRetries'); + expect(retired).toContain('#4962'); + // The number does NOT change — and the message must say so, because the + // identically-spelled connector key IS off by one. + expect(retired).toContain('NUMBER IS UNCHANGED'); + expect(retired).toContain('RetryConfig.maxAttempts'); + // The default flip has to travel with the rename, or an author does a + // lossless-looking rename and silently loses their three retries. + expect(retired).toContain('maxRetries: 3'); + }); + + it('tombstones `retryDelayMs` via the shared policy, naming this surface (#4661, #4964)', () => { const retired = rejectionFor(['retry'], 'retryDelayMs'); expect(retired).toContain('backoffMs'); expect(retired).toContain('#4661'); }); - it('names the three converged-policy keys this block deliberately lacks (#4962)', () => { - for (const absent of ['backoffMultiplier', 'maxRetryDelayMs', 'jitter']) { - const message = rejectionFor(['retry'], absent); - expect(message, `${absent} must carry the absence prescription`).toContain('documented ABSENCE'); - expect(message).toContain('#4962'); + it('DECLARES the three keys 批 12 documented as absent (#4962)', () => { + for (const key of ['backoffMultiplier', 'maxRetryDelayMs', 'jitter']) { + const message = rejectionFor(['retry'], key); + // 'x' is the wrong TYPE for all three, so a rejection is expected — what + // must be gone is the absence prescription: the key is real now. + expect(message, `${key} must no longer be described as absent`).not.toContain('documented ABSENCE'); } + const parsed = ETLPipelineSchema.safeParse( + pipelineWith(['retry'], 'jitter', true), + ); + expect(parsed.success).toBe(true); }); it('explains that pipeline direction is structural, not a key', () => { diff --git a/packages/spec/src/automation/etl.zod.ts b/packages/spec/src/automation/etl.zod.ts index c98dc52dbe..7d00fea21f 100644 --- a/packages/spec/src/automation/etl.zod.ts +++ b/packages/spec/src/automation/etl.zod.ts @@ -2,6 +2,8 @@ import { z } from 'zod'; import { CronExpressionInputSchema } from '../shared/expression.zod'; +import { retiredKey } from '../shared/retired-key'; +import { retryPolicyShape } from '../shared/retry-policy.zod'; import { strictObject } from '../shared/strict-object'; /** @@ -210,41 +212,41 @@ const ETL_PIPELINE_GUIDANCE: Readonly> = { const ETL_RETRY_HISTORY = 'Until #4001 an undeclared key here was dropped silently and the block fell back to its defaults ' - + '(3 attempts, 60s) while reporting the authored policy as accepted.'; + + '(3 attempts, 60s) while reporting the authored policy as accepted. Until #4962 this block was a ' + + 'SEPARATE retry vocabulary — it spelled the count `maxAttempts`, defaulted it to 3, and declared no ' + + 'backoff multiplier, ceiling or jitter; it now carries the converged `RetryPolicySchema` contract.'; /** * Anchor: `RetryPolicySchema` (`shared/retry-policy.zod.ts`) — the retry policy * #4661 converged onto ONE declaration for `job.retryPolicy` and a `try_catch` - * node's `retry` region. This block is a **third** encoding of the same concept - * that the convergence did not reach, because it is an anonymous inline object - * with no exported name and so never appeared in the #4411 / #4535 dual-source - * scan that drove that work. + * node's `retry` region. This block was a **third** encoding of the same + * concept that the convergence did not reach, because it is an anonymous + * inline object with no exported name and so never appeared in the #4411 / + * #4535 dual-source scan that drove that work. (`Flow.errorHandling` was the + * fourth — #4964, same construction, same blind spot.) * - * Closing this shape does not fix the divergence — it makes it audible. The - * five entries below are the whole diff between the two vocabularies, stated - * where an author hits it. Whether to converge (and which default `maxAttempts` - * should then take: #4661 argues 0, this block ships 3) is #4962 — a contract - * decision, deliberately not made inside a strictness batch. + * 批 12 could only make the divergence *audible*: it closed the shape and spent + * five curated entries stating the diff between the two vocabularies where an + * author hits it. #4962 removed the diff instead, so all five entries are gone + * — four of them (`retryDelayMs` plus the "documented absence" of + * `backoffMultiplier` / `maxRetryDelayMs` / `jitter`) because those keys are + * now DECLARED here, and the fifth (`maxRetries` → `maxAttempts`) because it + * pointed the wrong way: `maxRetries` is the canonical spelling and + * `maxAttempts` is the tombstone. + * + * What survives is anchored the same way 批 12's entries were — to sibling + * contracts that exist in this repo and spell the same knob differently: + * `integration/connector.zod.ts`'s `RetryConfig` (`initialDelayMs`, + * `maxDelayMs`), and the plain-English count forms. This is deliberately the + * SAME table `Flow.errorHandling` carries, because after the convergence the + * two surfaces are the same contract and an author should not learn two + * different lessons from them. */ const ETL_RETRY_ALIASES: Readonly> = { - maxRetries: 'maxAttempts', -}; - -/** The three keys the converged policy declares and this block deliberately does not. */ -const etlRetryAbsence = (key: string): string => - `\`${key}\` is declared on the converged \`RetryPolicySchema\` (\`shared/retry-policy.zod.ts\`, #4661) but ` - + `NOT on an ETL pipeline's \`retry\`, which declares only \`maxAttempts\` + \`backoffMs\` — a flat, uncapped, ` - + `unjittered backoff. This is a documented ABSENCE, not a typo: nothing here would read the key. Converging ` - + `the two vocabularies is tracked as #4962.`; - -const ETL_RETRY_GUIDANCE: Readonly> = { - retryDelayMs: - '`retryDelayMs` was the pre-17 automation-side spelling of the base delay and was removed in ' - + '@objectstack/spec 17.0.0 (#4661); it is tombstoned on `RetryPolicySchema`. This block already spells it ' - + '`backoffMs` — rename the key, the value (milliseconds before the first retry) is unchanged.', - backoffMultiplier: etlRetryAbsence('backoffMultiplier'), - maxRetryDelayMs: etlRetryAbsence('maxRetryDelayMs'), - jitter: etlRetryAbsence('jitter'), + initialDelayMs: 'backoffMs', + maxDelayMs: 'maxRetryDelayMs', + retries: 'maxRetries', + attempts: 'maxRetries', }; const ETL_NOTIFICATIONS_HISTORY = @@ -485,16 +487,61 @@ export const ETLPipelineSchema = lazySchema(() => strictObject({ enabled: z.boolean().default(true).describe('Pipeline enabled status'), /** - * Retry configuration for failed runs + * Retry configuration for failed runs — the converged `RetryPolicySchema` + * contract (#4962), shared with `job.retryPolicy`, a `try_catch` node's + * `retry` and `flow.errorHandling`. + * + * Three things changed when this block stopped being its own vocabulary, and + * all three are breaking (17.0.0): + * + * 1. `maxAttempts` → `maxRetries`. Pure rename, value preserved: both count + * the retries AFTER the initial attempt. (Do NOT carry the off-by-one + * that `integration/connector.zod.ts`'s `RetryConfig.maxAttempts` needs — + * that key INCLUDES the first attempt and is a different number. The + * tombstone below says so, because the same word means two things one + * directory apart.) + * 2. The count now defaults to **0**, not 3. A pipeline that declared + * `retry: {}` used to buy three silent re-runs; it now buys none until + * the author states a count. An ETL destination is a foreign system by + * definition, so an implicit retry against a non-idempotent one is a + * duplicate write — a second invoice, a second export, a second webhook. + * That is the failure mode hardest to catch in tests and most expensive + * in production, and an unstated key is exactly where LLM-authored + * metadata hides it. + * 3. `backoffMultiplier` / `maxRetryDelayMs` / `jitter` are now declarable. + * They were the documented absence 批 12 spent three guidance entries on: + * a nightly warehouse pipeline retrying every 60s, flat and unjittered, + * is the textbook thundering herd. + * + * The base delay's default follows the shared contract (1000ms, not this + * block's old 60000ms). Nothing deployed moves: `etl.zod.ts` has no parse + * site in objectstack / objectui / cloud and an ETL pipeline is not a + * `defineStack` collection, so there is no stored pipeline for a default to + * change under. State `backoffMs` explicitly if you want the old minute. */ retry: strictObject({ surface: "this ETL pipeline's retry configuration", history: ETL_RETRY_HISTORY, aliases: ETL_RETRY_ALIASES, - guidance: ETL_RETRY_GUIDANCE, }, { - maxAttempts: z.number().int().min(0).default(3).describe('Max retry attempts'), - backoffMs: z.number().int().min(0).default(60000).describe('Backoff in milliseconds'), + ...retryPolicyShape(), + + // ── Tombstone (ADR-0087) ────────────────────────────────────────── + // The count's pre-17 ETL spelling. Tombstoned rather than deleted even + // though this shape IS strict: an unknown-key rejection would carry the + // key, and what an upgrading author needs is the RENAME plus the warning + // that the identically-spelled connector key is a different number. + maxAttempts: retiredKey( + '`maxAttempts` was removed from an ETL pipeline\'s `retry` in @objectstack/spec 17.0.0 ' + + '(#4962) — the retry policy now has ONE vocabulary across `job.retryPolicy`, a ' + + '`try_catch` node\'s `retry`, `flow.errorHandling` and this block. Rename the key to ' + + '`maxRetries`; the NUMBER IS UNCHANGED, because this block\'s `maxAttempts` already ' + + 'counted the retries after the initial attempt. Do not subtract one — that adjustment ' + + 'belongs to `integration/connector.zod.ts`\'s `RetryConfig.maxAttempts`, which is a ' + + 'different key that INCLUDES the first attempt. Note the default also changed: an ' + + 'omitted count used to mean 3 retries and now means 0, so if you were relying on the ' + + 'old default, write `maxRetries: 3` explicitly.', + ), }).optional().describe('Retry configuration'), /** diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index c3a0e3bf17..432af50024 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -572,12 +572,12 @@ describe('FlowSchema - errorHandling', () => { errorHandling: { strategy: 'retry', maxRetries: 3, - retryDelayMs: 2000, + backoffMs: 2000, }, }); expect(result.errorHandling?.strategy).toBe('retry'); expect(result.errorHandling?.maxRetries).toBe(3); - expect(result.errorHandling?.retryDelayMs).toBe(2000); + expect(result.errorHandling?.backoffMs).toBe(2000); }); it('should default errorHandling strategy to fail', () => { @@ -649,7 +649,7 @@ describe('FlowSchema - errorHandling', () => { 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 }); + const result = retryFlow({ strategy, maxRetries: 0, backoffMs: 0, backoffMultiplier: 1 }); expect(result.success, `${strategy} should accept a spelled-out block`).toBe(true); } }); @@ -663,7 +663,7 @@ describe('FlowSchema - errorHandling', () => { expect(result.data!.errorHandling).toMatchObject({ strategy: 'retry', maxRetries: 2, - retryDelayMs: 1000, + backoffMs: 1000, backoffMultiplier: 1, maxRetryDelayMs: 30000, jitter: false, @@ -709,7 +709,7 @@ describe('FlowSchema - errorHandling', () => { errorHandling: { strategy: 'retry', maxRetries: 5, - retryDelayMs: 1000, + backoffMs: 1000, backoffMultiplier: 2, maxRetryDelayMs: 30000, jitter: true, @@ -1382,13 +1382,33 @@ describe('unknown keys are rejected, not stripped (#4001)', () => { expect(issue!.message).toContain('`cancelActivity` → `interrupting`'); }); - it('errorHandling: `backoffMs` is the sibling retry policy\'s converged spelling (#4661)', () => { - const issue = unknownKeyIssue(FlowSchema, { + it('errorHandling: `backoffMs` is now ACCEPTED — it is the converged spelling (#4964)', () => { + // This assertion used to be its exact inverse: the block demanded + // `retryDelayMs` and rejected `backoffMs`, so an author who had read + // `shared/retry-policy.zod.ts` (where `retryDelayMs` is tombstoned and + // `backoffMs` prescribed) was rejected for learning the canonical word. + // Both surfaces now build from `retryPolicyShape()`. + const result = FlowSchema.safeParse({ ...minimalFlow, errorHandling: { strategy: 'retry', maxRetries: 3, backoffMs: 5000 }, }); - expect(issue!.message).toContain("flow's `errorHandling` block"); - expect(issue!.message).toContain('`backoffMs` → `retryDelayMs`'); + expect(result.success).toBe(true); + expect(result.data!.errorHandling!.backoffMs).toBe(5000); + }); + + it('errorHandling: `retryDelayMs` is the tombstone and carries the rename (#4964)', () => { + const result = FlowSchema.safeParse({ + ...minimalFlow, + errorHandling: { strategy: 'retry', maxRetries: 3, retryDelayMs: 5000 }, + }); + expect(result.success).toBe(false); + const message = JSON.stringify(result.error!.issues); + expect(message).toContain('was removed in @objectstack/spec 17.0.0'); + expect(message).toContain('backoffMs'); + // The prescription must name THIS surface, not only the two #4661 knew + // about — naming a scope narrower than the truth is what let this + // divergence read as reviewed for a whole release. + expect(message).toContain('flow.errorHandling'); }); it('errorHandling: `maxAttempts` gets the off-by-one, not a rename', () => { @@ -1426,7 +1446,7 @@ describe('unknown keys are rejected, not stripped (#4001)', () => { const flow = FlowSchema.parse({ ...minimalFlow, errorHandling: { - strategy: 'retry', maxRetries: 2, retryDelayMs: 10, backoffMultiplier: 2, + strategy: 'retry', maxRetries: 2, backoffMs: 10, backoffMultiplier: 2, maxRetryDelayMs: 100, jitter: true, }, }); diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index fdd43409a6..f57b7f611e 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -21,6 +21,7 @@ import { strictUnknownKeyError } from '../shared/suggestions.zod'; */ import { lazySchema } from '../shared/lazy-schema'; import { retiredKey } from '../shared/retired-key'; +import { retryPolicyShape } from '../shared/retry-policy.zod'; import { strictObject } from '../shared/strict-object'; export const FlowNodeAction = z.enum([ 'start', // Trigger @@ -607,6 +608,15 @@ export const FlowSchema = lazySchema(() => z.object({ /** * Error Handling Strategy. * + * The retry knobs are the converged `RetryPolicySchema` contract, shared with + * `job.retryPolicy`, a `try_catch` node's `retry` and an ETL pipeline's + * `retry` (#4661 + #4964 — see `shared/retry-policy.zod.ts`). Until 17 this + * block spelled the base delay `retryDelayMs` while the converged policy + * spelled it `backoffMs`, so an author who read the newer file and brought + * the word here had it silently stripped (pre-批 11) or rejected (post-批 11) + * — being punished for learning the canonical spelling. `strategy` stays + * here: it selects *whether* the policy runs, it is not part of the policy. + * * **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 @@ -625,17 +635,19 @@ export const FlowSchema = lazySchema(() => z.object({ // Every one of these is a real, in-repo spelling of the same knob on a // NEIGHBOURING retry surface — which is what makes this table an // empirical claim rather than a guess about typos: - // `shared/retry-policy.zod.ts` (#4661, job.retryPolicy + a try_catch - // node's `retry`) → `backoffMs`, and it TOMBSTONED `retryDelayMs` - // as "the automation-side spelling", so an author who learned the - // converged word and brings it here is being punished for reading - // the newer file. // `integration/connector.zod.ts` RetryConfig → `initialDelayMs`, // `maxDelayMs`. // `retries`/`attempts` are the plain-English forms; `onError` is n8n's // word for the strategy switch. - backoffMs: 'retryDelayMs', - initialDelayMs: 'retryDelayMs', + // + // `backoffMs` was HERE until #4964, pointing at `retryDelayMs` — i.e. + // this table used to punish an author for having read the newer file + // (`shared/retry-policy.zod.ts` tombstoned `retryDelayMs` as "the + // automation-side spelling" in #4661, and then this surface still + // demanded it). The alias is gone because the divergence is gone: the + // block now builds from `retryPolicyShape()`, `backoffMs` IS the key, + // and `retryDelayMs` is the tombstone that arrives with it. + initialDelayMs: 'backoffMs', maxDelayMs: 'maxRetryDelayMs', retries: 'maxRetries', attempts: 'maxRetries', @@ -661,20 +673,39 @@ export const FlowSchema = lazySchema(() => z.object({ }, history: 'Until #4001 these were dropped silently — the block still parsed, so a retry budget ' + - 'or backoff the author configured was replaced by this block\'s defaults without a word.', + 'or backoff the author configured was replaced by this block\'s defaults without a word. ' + + 'Since #4964 the retry keys are the converged `RetryPolicySchema` contract, so a spelling ' + + 'learned on `job.retryPolicy` or a `try_catch` node\'s `retry` is correct here too.', }, { strategy: z.enum(['fail', 'retry', 'continue']).default('fail').describe('How to handle node execution errors'), - // 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'), - jitter: z.boolean().default(false).describe('Add random jitter to retry delay to avoid thundering herd'), + + // ── The retry policy itself: ONE declaration (#4964) ──────────────── + // `maxRetries` / `backoffMs` / `backoffMultiplier` / `maxRetryDelayMs` / + // `jitter`, plus the `retryDelayMs` tombstone, all arrive from + // `shared/retry-policy.zod.ts`. Before #4964 they were hand-copied here, + // and the copy had drifted in exactly one word — this block spelled the + // base delay `retryDelayMs` where the converged policy spells it + // `backoffMs`. Every other key, bound and default already matched, which + // is what made the divergence so durable: it looked reviewed. + // + // The spread is what keeps that from happening again. A key added to the + // policy lands on all four surfaces at once, instead of on the ones + // whoever added it happened to grep for. + ...retryPolicyShape(), + + // The ONE site-specific override, and it is prose only — same type, same + // bounds, same default, all still single-sourced above. `.describe()` + // lands in `content/docs/references/`, and the flow surface has a reading + // the other three do not: the count is read only under `strategy: + // 'retry'`, where the `superRefine` below then requires >= 1 (#4247). + // Default 0 = "no retries" is the right reading for the two strategies + // that never retry; under `'retry'` it would 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: retryPolicyShape().maxRetries + .describe("Retry attempts after the initial one. Read only under strategy: 'retry', which requires >= 1; 0 (the default) means no retry."), + // `fallbackNodeId` REMOVED (#3896 audit close-out): the engine routes // unrecoverable errors via per-node FAULT EDGES, never this — an author // who configured a fallback here had none. diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index 59c81f4518..0d91d45977 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -3459,9 +3459,10 @@ const objectEnableTrashMruRemoved: MetadataConversion = { * see the `datasource-inert-blocks-removed` note above, which leans on * exactly that distinction), so the platform drops from three spellings to * two rather than four. `retryDelayMs` is tombstoned (`retiredKey`) — NOT - * deleted — because neither owning shape is `.strict()`: a plain deletion - * would have Zod silently swallow the authored number and fall back to the - * 1000ms default, which is the quiet-failure class ADR-0049 removes. + * deleted — because two of the four owning shapes are not `.strict()`: a + * plain deletion would have Zod silently swallow the authored number and + * fall back to the 1000ms default, which is the quiet-failure class + * ADR-0049 removes. * * 2. **The defaults were opposite, and no gate can see a default.** Pre-17, * `job.retryPolicy` defaulted `maxRetries: 3` / `backoffMultiplier: 2` @@ -3479,6 +3480,33 @@ const objectEnableTrashMruRemoved: MetadataConversion = { * Jobs with no `retryPolicy` block at all are left alone — absence already * meant a single attempt on both sides of the change. * + * ## The two surfaces this entry grew to cover (#4964 / #4962) + * + * The convergence above was driven by the dual-source instrument, whose + * question is "how many declarations share one exported NAME?". Two further + * encodings of the identical policy were invisible to it because they are + * anonymous inline `z.object`s with no exported name at all — and after a + * convergence lands, a surviving dialect reads as reviewed-and-kept rather + * than missed: + * + * - **`flow.errorHandling`** (#4964) spelled the base delay `retryDelayMs`; + * every other key, bound and default already matched. Step 0 below renames + * it, so the ONE authorable casualty of the whole convergence is still just + * that word — now retired everywhere it was ever legal rather than on two + * surfaces out of four. + * - **`ETLPipeline.retry`** (#4962) spelled the count `maxAttempts` and + * defaulted it to 3. It gets **no step here, deliberately.** An ETL pipeline + * is not a `defineStack` collection and `etl.zod.ts` has no parse site in + * objectstack / objectui / cloud (批 12's measurement), so there is no + * stored or authored document a walker could reach: a branch for it would be + * dead code claiming migration coverage that does not exist, which is the + * ADR-0049 failure this registry is supposed to prevent, not commit. Its + * `maxAttempts` tombstone carries the rename AND the default change, and the + * tombstone reaches the only doors that exist (`tsc` at the authoring site, + * and the parse). That is also why the ETL default flip 3 → 0 needs no + * materialization step while the job one did: nothing is deployed under the + * old reading. + * * `retiredFromLoadPath` is NOT set: `FlowNodeSchema.config` is an unconstrained * record, so no schema rejection can reach `config.retry.retryDelayMs` and the * conversion layer is the only seam that can declare and retire that spelling. @@ -3488,13 +3516,34 @@ const objectEnableTrashMruRemoved: MetadataConversion = { const retryPolicyConverged: MetadataConversion = { id: 'retry-policy-converged', toMajor: 17, - surface: 'flow.node.config.retry.retryDelayMs / job.retryPolicy.maxRetries / job.retryPolicy.backoffMultiplier', + surface: 'flow.errorHandling.retryDelayMs / flow.node.config.retry.retryDelayMs / job.retryPolicy.maxRetries / job.retryPolicy.backoffMultiplier', summary: - "retry policy unified across job.retryPolicy and try_catch retry: base delay 'retryDelayMs' → 'backoffMs', " + - "and the pre-17 job defaults (maxRetries 3, backoffMultiplier 2) written out explicitly now that the merged default is 0 / 1 (#4661)", + "retry policy unified across job.retryPolicy, try_catch retry and flow.errorHandling: base delay 'retryDelayMs' → 'backoffMs', " + + "and the pre-17 job defaults (maxRetries 3, backoffMultiplier 2) written out explicitly now that the merged default is 0 / 1 (#4661, #4964)", apply(stack, emit) { + // ── 0. flows: errorHandling.retryDelayMs → errorHandling.backoffMs ─ + // + // The FLOW-LEVEL retry policy (#4964). Same rename as the try_catch node + // below and for the same reason, reached one level up: `errorHandling` + // hangs off the flow document, not off a node, so `mapFlowNodes` walks + // straight past it — which is a small echo of why this divergence survived + // #4661 at all. `renameKey` leaves an already-canonical `backoffMs` alone + // and, when BOTH spellings are present, leaves the alias shadowed rather + // than guessing which number the author meant; the strict block then + // rejects naming both. (#4923 is queued to revisit that shadowing rule — + // this entry deliberately relies on the shared helper's semantics rather + // than open-coding its own, so it moves with that ruling.) + const withErrorHandling = mapCollection(stack, 'flows', (flow, path) => { + const eh = flow.errorHandling; + if (!eh || typeof eh !== 'object' || Array.isArray(eh)) return flow; + const renamed = renameKey(eh as Record, 'retryDelayMs', 'backoffMs'); + if (renamed === null) return flow; + emit({ from: 'retryDelayMs', to: 'backoffMs', path: `${path}.errorHandling.backoffMs` }); + return { ...flow, errorHandling: renamed }; + }); + // ── 1. try_catch nodes: retry.retryDelayMs → retry.backoffMs ────── - const withFlows = mapFlowNodes(stack, (node, path) => { + const withFlows = mapFlowNodes(withErrorHandling, (node, path) => { if (node.type !== 'try_catch') return node; const config = node.config; if (!config || typeof config !== 'object' || Array.isArray(config)) return node; @@ -3536,6 +3585,9 @@ const retryPolicyConverged: MetadataConversion = { before: { flows: [{ name: 'sync_orders', + // Flow-LEVEL policy (#4964) — one level up from the nodes, so the node + // walk below never sees it. + errorHandling: { strategy: 'retry', maxRetries: 3, retryDelayMs: 2000 }, nodes: [ { id: 'n1', type: 'start' }, { @@ -3553,6 +3605,11 @@ const retryPolicyConverged: MetadataConversion = { config: { try: { nodes: [], edges: [] }, retry: { maxRetries: 2, backoffMs: 250 } }, }, ], + }, { + // Flow-level block already canonical — left alone, no notice. + name: 'roll_up', + errorHandling: { strategy: 'retry', maxRetries: 1, backoffMs: 100 }, + nodes: [{ id: 'n1', type: 'start' }], }], jobs: [ // Omits both defaults — both get written out. @@ -3566,6 +3623,7 @@ const retryPolicyConverged: MetadataConversion = { after: { flows: [{ name: 'sync_orders', + errorHandling: { strategy: 'retry', maxRetries: 3, backoffMs: 2000 }, nodes: [ { id: 'n1', type: 'start' }, { @@ -3582,6 +3640,10 @@ const retryPolicyConverged: MetadataConversion = { config: { try: { nodes: [], edges: [] }, retry: { maxRetries: 2, backoffMs: 250 } }, }, ], + }, { + name: 'roll_up', + errorHandling: { strategy: 'retry', maxRetries: 1, backoffMs: 100 }, + nodes: [{ id: 'n1', type: 'start' }], }], jobs: [ { name: 'nightly_sync', schedule: { type: 'cron', expression: '0 0 * * *' }, handler: 'jobs.ts:sync', retryPolicy: { backoffMs: 5000, maxRetries: 3, backoffMultiplier: 2 } }, @@ -3589,8 +3651,9 @@ const retryPolicyConverged: MetadataConversion = { { name: 'weekly_purge', schedule: { type: 'cron', expression: '0 0 * * 0' }, handler: 'jobs.ts:purge' }, ], }, - // n2's rename, plus nightly_sync's two materialized defaults. - expectedNotices: 3, + // sync_orders' flow-level rename, n2's node-level rename, plus + // nightly_sync's two materialized defaults. + expectedNotices: 4, }, }; diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 8da8183a73..fbe4d74668 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -693,6 +693,30 @@ const step17: MigrationStep = { + 'the pre-17 numbers into every existing `job.retryPolicy` that omitted them. Deployed ' + 'stacks therefore keep their exact behaviour; what changes is only what a NEWLY authored ' + 'omission means.\n\n' + + 'That convergence then had to be finished twice more, and WHY it was incomplete is the ' + + 'part worth carrying forward (#4964, #4962). It was driven by the dual-source instrument, ' + + 'which asks "how many declarations publish the same exported NAME?" — so it could not see ' + + 'the two encodings of the identical policy that have no exported name at all, being ' + + 'anonymous inline blocks nested in a bigger schema: `flow.errorHandling` and ' + + '`ETLPipeline.retry`. The instrument was not broken and answered its own question exactly; ' + + 'that question was simply not "how many shapes does this ONE concept have?", which is what ' + + 'everybody read off it. The cost of the gap is concrete and falls on the author who did the ' + + 'right thing: `shared/retry-policy.zod.ts` tombstoned `retryDelayMs` and told them to write ' + + '`backoffMs`, and `flow.errorHandling` then rejected `backoffMs` and demanded `retryDelayMs` ' + + '— reading the newer file was punished. Both blocks now build from one shared shape. ' + + '`flow.errorHandling` costs nothing beyond the same `retryDelayMs` → `backoffMs` rename ' + + '(every other key, bound and default already matched, which is exactly why it looked ' + + 'reviewed), and the conversion covers it. `ETLPipeline.retry` costs a rename of the COUNT ' + + '— `maxAttempts` → `maxRetries`, same number, do NOT subtract one: that adjustment belongs ' + + "to `integration/connector.zod.ts`'s identically-spelled `RetryConfig.maxAttempts`, which " + + 'INCLUDES the first attempt — plus the same default flip (3 → 0) and three keys it never ' + + 'had (`backoffMultiplier` / `maxRetryDelayMs` / `jitter`, so a nightly warehouse pipeline ' + + 'can stop retrying flat, uncapped and unjittered every 60s). The ETL half gets a tombstone ' + + 'and no conversion step, deliberately: an ETL pipeline is not a `defineStack` collection ' + + 'and `etl.zod.ts` has no parse site in any of the three repos, so there is no stored ' + + 'document to walk and a step for it would advertise coverage it does not have. Nothing ' + + 'deployed moves; the migration surface is empty and this is the cheapest this convergence ' + + 'will ever be.\n\n' + 'The same enforce-or-remove pass reaches the event vocabulary: `DataEventType` drops ' + '`data.field.changed` (#4673). It had no producer anywhere — the engine emits ' + '`data.record.{created,updated,deleted}` and, since #4639, `data.records.{updated,' @@ -922,6 +946,36 @@ const step17: MigrationStep = { + 'retry re-runs the handler with its writes and callouts. No job fails to register ' + 'with the retry-policy bound prescription.', }, + { + id: 'etl-retry-converged-onto-retry-policy', + surface: 'etlPipeline.retry.maxAttempts (and any count above 10)', + replacement: 'maxRetries, same number — plus an explicit count if you relied on the old default of 3', + reason: + 'An ETL pipeline\'s `retry` was a THIRD retry vocabulary that #4661\'s convergence never ' + + 'reached, because that pass was driven by duplicated exported NAMES and this block is an ' + + 'anonymous inline object (#4962). It now carries the shared `RetryPolicySchema` contract, ' + + 'which changes three things with no single lossless rewrite between them. The rename ' + + '`maxAttempts` → `maxRetries` IS lossless and the tombstone performs it — both keys ' + + 'counted the retries AFTER the initial attempt, so the number does not change, and ' + + 'subtracting one (correct for `integration/connector.zod.ts`\'s identically-spelled ' + + '`RetryConfig.maxAttempts`, which includes the first attempt) would silently run one ' + + 'attempt fewer than asked. What needs a human: the count now DEFAULTS TO 0 instead of 3, ' + + 'so a pipeline that wrote `retry: {}` or omitted the count bought three silent re-runs ' + + 'and now buys none. That is deliberate and the business case is the destination — an ETL ' + + 'destination is a foreign system by definition, and an implicit retry against a ' + + 'non-idempotent one is a duplicate write (a second invoice, a second export, a second ' + + 'webhook). Retrying is now something an author states and thereby claims idempotency for. ' + + 'The shared contract also caps `maxRetries` at 10, which this block never did; clamping ' + + 'a larger budget would silently halve a number its author chose, so it fails at parse ' + + 'with the bound named instead.', + acceptanceCriteria: + 'No ETL pipeline declares `retry.maxAttempts`; every one that wants retries declares ' + + '`maxRetries` >= 1 explicitly (the number carried over unchanged from `maxAttempts`), and ' + + 'every pipeline that was relying on the old implicit 3 has either written `maxRetries: 3` ' + + 'or been re-decided against the duplicate-write risk at its destination. No count exceeds ' + + '10. Pipelines that want the old flat 60s backoff state `backoffMs: 60000` explicitly, ' + + 'since the shared default is 1000.', + }, { id: 'flow-retry-max-retries-required', surface: "flow.errorHandling.maxRetries (under strategy: 'retry')", diff --git a/packages/spec/src/shared/retry-policy.test.ts b/packages/spec/src/shared/retry-policy.test.ts index 7318f093a3..169f887c73 100644 --- a/packages/spec/src/shared/retry-policy.test.ts +++ b/packages/spec/src/shared/retry-policy.test.ts @@ -102,3 +102,108 @@ describe('RetryPolicySchema — converged shape', () => { expect(() => RetryPolicySchema.parse({ maxRetries: 11 })).toThrow(); }); }); + +/** + * The assertion class that would have caught #4964 and #4962 — and the reason + * it did not exist before them. + * + * #4661 converged the two declarations that shared an exported NAME, because + * that is the question `check:dual-source-exports` asks. It cannot ask "how + * many shapes does this one CONCEPT have?", so the two encodings with no + * exported name at all — `Flow.errorHandling` and `ETLPipeline.retry`, both + * anonymous inline `z.object`s — were outside its vision by construction, and + * a surviving dialect after a completed convergence reads as reviewed-and-kept + * rather than missed. + * + * This block asks the concept-level question directly, against the four + * surfaces that carry a retry policy. It is deliberately a PARSE comparison + * rather than a source or `.shape` inspection: it is blind to how a surface + * obtains the contract (spread, reference, or — the failure it exists to + * catch — a fresh hand-copied key list), and sees only whether an author gets + * the same keys and the same numbers on all four. + * + * Adding a fifth retry surface without wiring `retryPolicyShape()` fails here. + */ +describe('every retry surface carries ONE contract (#4661, #4964, #4962)', () => { + const POLICY_KEYS = ['maxRetries', 'backoffMs', 'backoffMultiplier', 'maxRetryDelayMs', 'jitter']; + const POLICY_DEFAULTS = { + maxRetries: 0, backoffMs: 1000, backoffMultiplier: 1, maxRetryDelayMs: 30000, jitter: false, + }; + + const minimalFlow = { + name: 'f', label: 'F', type: 'autolaunched' as const, + nodes: [{ id: 'n1', type: 'start', label: 'S' }], edges: [], + }; + const minimalPipeline = { + name: 'p', label: 'P', + source: { type: 'api' as const, connector: 'sf', config: {} }, + destination: { type: 'database' as const, connector: 'pg', config: {} }, + }; + + /** The parsed retry region of each surface, given an EMPTY authored block. */ + const surfaces = (): ReadonlyArray]> => [ + ['job.retryPolicy / try_catch retry', RetryPolicySchema.parse({}) as Record], + [ + 'flow.errorHandling', + Automation.FlowSchema.parse({ ...minimalFlow, errorHandling: {} }).errorHandling as Record, + ], + [ + 'etlPipeline.retry', + Automation.ETLPipelineSchema.parse({ ...minimalPipeline, retry: {} }).retry as Record, + ], + ]; + + it('declares the same key set everywhere (modulo flow-only `strategy`)', () => { + for (const [label, parsed] of surfaces()) { + // `strategy` selects WHETHER the policy runs; it is not part of it. + const keys = Object.keys(parsed).filter((k) => k !== 'strategy').sort(); + expect(keys, `${label} must carry exactly the converged key set`).toEqual([...POLICY_KEYS].sort()); + } + }); + + it('applies the same defaults everywhere — including the opt-in count of 0', () => { + // The half no gate can see: `authorable-surface.json` compares key sets and + // a default is not a key. `ETLPipeline.retry` defaulted the count to 3 + // until #4962 while every sibling defaulted 0, and nothing failed. + for (const [label, parsed] of surfaces()) { + for (const [key, value] of Object.entries(POLICY_DEFAULTS)) { + expect(parsed[key], `${label}.${key} must default to ${value}`).toBe(value); + } + } + }); + + it('retires the two pre-17 spellings wherever they were legal', () => { + // `retryDelayMs` (automation base delay) and `maxAttempts` (the ETL count). + // Both tombstoned rather than deleted, so the rejection carries the rename + // instead of a bare "unrecognized key" — or, on the non-strict surfaces, a + // silent strip back to the default. + const flowRetired = Automation.FlowSchema.safeParse({ + ...minimalFlow, errorHandling: { strategy: 'retry', maxRetries: 2, retryDelayMs: 500 }, + }); + expect(flowRetired.success).toBe(false); + expect(JSON.stringify(flowRetired.error!.issues)).toMatch(/backoffMs/); + + const etlRetired = Automation.ETLPipelineSchema.safeParse({ + ...minimalPipeline, retry: { maxAttempts: 3 }, + }); + expect(etlRetired.success).toBe(false); + expect(JSON.stringify(etlRetired.error!.issues)).toMatch(/maxRetries/); + }); + + it('accepts a policy authored once and pasted onto any surface', () => { + // The whole point, stated as the author experiences it: learn the words on + // one surface, write them on any other. Before #4964 this exact object was + // REJECTED by `flow.errorHandling` (which demanded `retryDelayMs`) and by + // `ETLPipeline.retry` (which demanded `maxAttempts` and declared none of + // the last three keys). + const policy = { + maxRetries: 3, backoffMs: 5000, backoffMultiplier: 2, maxRetryDelayMs: 60000, jitter: true, + }; + + expect(RetryPolicySchema.safeParse(policy).success).toBe(true); + expect(Automation.FlowSchema.safeParse({ + ...minimalFlow, errorHandling: { strategy: 'retry', ...policy }, + }).success).toBe(true); + expect(Automation.ETLPipelineSchema.safeParse({ ...minimalPipeline, retry: policy }).success).toBe(true); + }); +}); diff --git a/packages/spec/src/shared/retry-policy.zod.ts b/packages/spec/src/shared/retry-policy.zod.ts index dfb7cfe842..98497a7737 100644 --- a/packages/spec/src/shared/retry-policy.zod.ts +++ b/packages/spec/src/shared/retry-policy.zod.ts @@ -4,7 +4,8 @@ * @module shared/retry-policy * * The **single declaration** of the exponential-backoff retry policy (#4661, - * the #4535 C8 dual-source cluster). + * the #4535 C8 dual-source cluster; completed for the anonymous inline blocks + * by #4964 / #4962). * * Until 17 this shape existed twice — `automation/control-flow.zod.ts` (the * `try_catch` node's `retry` region) and `system/job.zod.ts` (`job.retryPolicy`) @@ -16,6 +17,32 @@ * base delay (`retryDelayMs` vs `backoffMs`), two keys only one side had * (`maxRetryDelayMs` / `jitter`), and the defaults. * + * ## Why it existed FOUR times, and what the first convergence could not see + * + * #4661 was driven by the dual-source instrument (#4411 / #4535 C8), whose + * question is "how many declarations publish the same exported NAME?". Two more + * encodings of this identical concept were invisible to it **by construction**, + * because neither has an exported name at all — both are anonymous inline + * `z.object`s nested inside a bigger schema: + * + * - `automation/flow.zod.ts` → `Flow.errorHandling` (#4964) — spelled the base + * delay `retryDelayMs`, every other key already identical. + * - `automation/etl.zod.ts` → `ETLPipeline.retry` (#4962) — spelled the count + * `maxAttempts`, defaulted it to **3** (the opposite of the opt-in reading + * below), and declared no `backoffMultiplier` / `maxRetryDelayMs` / `jitter` + * at all, so its backoff was flat, uncapped and unjittered. + * + * That is the campaign's "instrument misreports coverage" class, in its purest + * form: the instrument was not broken and answered its own question exactly — + * the question simply was not the one whose answer everybody read off it. After + * a convergence completes, a surviving dialect reads as *reviewed and kept*. + * + * Both now build from {@link retryPolicyShape}, so the four surfaces share one + * declaration of the key set, the bounds and the defaults. The two of them that + * are `.strict()` keep their own `strictObject` curation and their own extra + * keys (`Flow.errorHandling.strategy`) — what is shared is the *contract*, not + * the surface's framing of it. + * * ## Why this file, and why it is not in `shared/index.ts` * * The published JSON-Schema def key is `/`, derived from @@ -45,21 +72,86 @@ import { lazySchema } from './lazy-schema'; import { retiredKey } from './retired-key'; /** - * Exponential-backoff retry policy — the one shape for both `job.retryPolicy` - * and a `try_catch` node's `retry` region. + * The retry policy's raw Zod shape — key set, bounds, defaults and prose, in + * ONE place. + * + * Two of the four surfaces that carry this policy cannot simply reference + * {@link RetryPolicySchema}: `Flow.errorHandling` and `ETLPipeline.retry` are + * `.strict()` (`strictObject`, the #4001 campaign standard) and the flow one + * also carries `strategy` plus its own `superRefine`. Handing them the *shape* + * rather than the *schema* is what lets them stay strict, keep their curated + * unknown-key tables, and still have exactly one declaration of what a retry + * policy IS — the alternative (a fifth hand-copied key list) is the debt #4964 + * and #4962 exist to remove. + * + * It is a function rather than a const for the same reason every schema here is + * wrapped in `lazySchema`: a module-level shape would allocate its five Zod + * nodes at import time for every consumer, including the ones that never parse + * a retry policy. + * + * NOT re-exported by any barrel — see the module note above. It is an + * intra-package construction detail, not authorable surface. + */ +export function retryPolicyShape() { + return { + maxRetries: z.number().int().min(0).max(10).default(0) + .describe('Retry attempts after the initial one. 0 (the default) means no retry — state a count to opt in.'), + backoffMs: z.number().int().min(0).default(1000) + .describe('Base delay before the first retry (ms); subsequent delays multiply by backoffMultiplier'), + backoffMultiplier: z.number().min(1).default(1) + .describe('Exponential backoff multiplier; 1 (the default) keeps the delay flat'), + maxRetryDelayMs: z.number().int().min(0).default(30000) + .describe('Ceiling for a single backoff delay (ms)'), + jitter: z.boolean().default(false) + .describe('Randomize each delay within [50%, 100%] of its computed value — spreads a thundering herd of simultaneous retries'), + + // ── Tombstone (ADR-0087) ──────────────────────────────────────────── + // `retryDelayMs` was the automation-side spelling of `backoffMs`, on BOTH + // `try_catch`'s `retry` (#4661) and `Flow.errorHandling` (#4964). It is + // tombstoned rather than deleted because two of the four owning shapes are + // not `.strict()`: a plain deletion would have Zod silently strip the + // authored value and drop the delay back to the 1000ms default, which is + // precisely the quiet-failure class ADR-0049 exists to remove. On the two + // strict surfaces the tombstone is still the better channel — it carries + // the rename, where a bare unknown-key rejection would only carry the key. + // `retry-policy-converged` rewrites it on the load path. + retryDelayMs: retiredKey( + '`retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661, #4964) — the retry policy now ' + + 'has ONE spelling for its base delay across every surface that carries it: `job.retryPolicy`, ' + + "a `try_catch` node's `retry`, `flow.errorHandling` and an ETL pipeline's `retry`. " + + 'Rename the key to `backoffMs`; the value (milliseconds before the first retry) ' + + 'is unchanged. `os migrate meta --from 16` rewrites it for you.', + ), + }; +} + +/** + * Exponential-backoff retry policy — the named schema for `job.retryPolicy` and + * a `try_catch` node's `retry` region. `Flow.errorHandling` and + * `ETLPipeline.retry` carry the same contract via {@link retryPolicyShape}, + * which they must, being `.strict()` (see that function's note). * * Delay before retry *n* is `min(backoffMs * backoffMultiplier^(n-1), * maxRetryDelayMs)`, optionally jittered. * - * ## Defaults are opt-in, not opt-out (17.0.0, #4661) + * ## Defaults are opt-in, not opt-out (17.0.0, #4661, #4962) * * `maxRetries` defaults to **0** — declaring a retry block does not by itself - * buy retries. The pre-17 `job.retryPolicy` defaulted to 3, so a job that wrote - * `{ backoffMs: 5000 }` and nothing else silently got three attempts; the + * buy retries. Two pre-17 surfaces defaulted it to 3: `job.retryPolicy`, so a + * job that wrote `{ backoffMs: 5000 }` and nothing else silently got three + * attempts, and `ETLPipeline.retry` (as `maxAttempts`). The * `retry-policy-converged` conversion writes that `3` (and the old * `backoffMultiplier: 2`) into existing job documents, so no deployed stack * changes behaviour. What changes is what a NEWLY authored omission means. * + * The ETL half needed no conversion branch and deliberately has none: an ETL + * pipeline is not a `defineStack` collection and `etl.zod.ts` has no parse site + * anywhere in objectstack / objectui / cloud (批 12's measurement), so there is + * no stored document for a D2 walker to reach. Writing one anyway would be a + * conversion advertising coverage it does not have. The `maxAttempts` tombstone + * on that block is the whole migration channel, and it reaches the only door + * that exists — `tsc` at the authoring site, and the parse. + * * The reason to make absence mean "no retry" rather than "retry three times": * a retry replays whatever the attempt already did — a job handler's writes and * callouts, a `try` region's side effects. An implicit retry is the failure mode @@ -69,32 +161,7 @@ import { retiredKey } from './retired-key'; * step (`flow-retry-max-retries-required`, #4247): an unstated count is * unambiguously 0, and "retry zero times" is a decision the author must state. */ -export const RetryPolicySchema = lazySchema(() => z.object({ - maxRetries: z.number().int().min(0).max(10).default(0) - .describe('Retry attempts after the initial one. 0 (the default) means no retry — state a count to opt in.'), - backoffMs: z.number().int().min(0).default(1000) - .describe('Base delay before the first retry (ms); subsequent delays multiply by backoffMultiplier'), - backoffMultiplier: z.number().min(1).default(1) - .describe('Exponential backoff multiplier; 1 (the default) keeps the delay flat'), - maxRetryDelayMs: z.number().int().min(0).default(30000) - .describe('Ceiling for a single backoff delay (ms)'), - jitter: z.boolean().default(false) - .describe('Randomize each delay within [50%, 100%] of its computed value — spreads a thundering herd of simultaneous retries'), - - // ── Tombstone (ADR-0087) ──────────────────────────────────────────── - // `retryDelayMs` was the automation-side spelling of `backoffMs`. It is the - // ONE authorable key this convergence costs, and it is tombstoned rather than - // deleted because neither owning shape is `.strict()`: a plain deletion would - // have Zod silently strip the authored value and drop the delay back to the - // 1000ms default, which is precisely the quiet-failure class ADR-0049 exists - // to remove. `retry-policy-converged` rewrites the key. - retryDelayMs: retiredKey( - '`retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661) — the retry policy now ' + - 'has one spelling for its base delay across `job.retryPolicy` and a `try_catch` node\'s ' + - '`retry`. Rename the key to `backoffMs`; the value (milliseconds before the first retry) ' + - 'is unchanged. `os migrate meta --from 16` rewrites it for you.', - ), -})); +export const RetryPolicySchema = lazySchema(() => z.object(retryPolicyShape())); /** * What an author writes — every key optional, defaults unapplied. diff --git a/skills/objectstack-automation/references/_index.md b/skills/objectstack-automation/references/_index.md index 11781ec4db..2cdc43d4b2 100644 --- a/skills/objectstack-automation/references/_index.md +++ b/skills/objectstack-automation/references/_index.md @@ -26,6 +26,7 @@ from `node_modules` — there is no local copy in the skill bundle. - `node_modules/@objectstack/spec/src/shared/expression.zod.ts` — Expression Protocol - `node_modules/@objectstack/spec/src/shared/identifiers.zod.ts` — System Identifier Schema - `node_modules/@objectstack/spec/src/shared/protection.zod.ts` — Package-level metadata protection (ADR-0010 §3.7 — Phase 4.3) +- `node_modules/@objectstack/spec/src/shared/retry-policy.zod.ts` — The **single declaration** of the exponential-backoff retry policy (#4661, - `node_modules/@objectstack/spec/src/shared/suggestions.zod.ts` — "Did you mean?" Suggestion Utilities ## How to read these