diff --git a/.changeset/retry-policy-dual-source-c8.md b/.changeset/retry-policy-dual-source-c8.md new file mode 100644 index 0000000000..0711e442d6 --- /dev/null +++ b/.changeset/retry-policy-dual-source-c8.md @@ -0,0 +1,84 @@ +--- +"@objectstack/spec": major +--- + +**Retry policy converges onto one declaration** (#4661 — the #4535 C8 dual-source cluster). + +`@objectstack/spec/automation` and `@objectstack/spec/system` both exported +`RetryPolicySchema` / `RetryPolicy`, resolving to **different declarations** — so the +shape you got depended only on which entry you imported (the #4411 trap). They were +never two concepts: the `try_catch` node's `retry` region and `job.retryPolicy` both +compute `delay = base * multiplier^(retry-1)`, and both executors implemented that +identical formula. There is now one declaration, re-exported by both entries, carrying +the union of what the two sides could express. + +## FROM → TO + +| | FROM `./automation` | FROM `./system` | TO (both entries) | +|---|---|---|---| +| base delay | `retryDelayMs`, min 0, default 1000 | `backoffMs`, positive, default 1000 | **`backoffMs`**, min 0, default 1000 | +| `maxRetries` | 0–10, default **0** | ≥0 unbounded, default **3** | 0–**10**, default **0** | +| `backoffMultiplier` | ≥**1**, default **1** | positive, default **2** | ≥**1**, default **1** | +| `maxRetryDelayMs` | default 30000 | *(absent)* | default 30000 | +| `jitter` | default false | *(absent)* | default false | +| `RetryPolicy` type | `z.input` | `z.infer` | `z.input` (+ new `RetryPolicyParsed` for `z.infer`) | + +## What you must change + +**1. Rename `retryDelayMs` → `backoffMs`** in any `try_catch` node's `retry` block. +The value (milliseconds before the first retry) is unchanged. The old spelling is +**tombstoned**, not deleted — it rejects with the rename prescription instead of being +silently swallowed, because neither owning schema is `.strict()`. Automated: + +``` +os migrate meta --from 16 +``` + +**2. Nothing for existing jobs — but read this if you author new ones.** `maxRetries` +now defaults to **0** and `backoffMultiplier` to **1**, where `job.retryPolicy` +previously defaulted to 3 and 2. Left alone that would silently stop deployed jobs from +retrying, so the `retry-policy-converged` conversion **writes the pre-17 numbers +explicitly into every existing `job.retryPolicy`** that omitted them: + +```jsonc +// before // after `os migrate meta` +{ "backoffMs": 5000 } { "backoffMs": 5000, "maxRetries": 3, "backoffMultiplier": 2 } +``` + +Deployed stacks therefore keep their exact behaviour. What changes is what a **newly +authored** omission means: declaring a retry block without `maxRetries` now means *no +retry*. Retry is opt-in because a retry replays whatever the attempt already did — a job +handler's writes and callouts, a `try` region's side effects — and an implicit replay is +the failure mode hardest to catch in tests and most expensive in production. (The same +reading is already recorded for flow-level retry in `flow-retry-max-retries-required`, +#4247.) + +> This defaults change is the part **no gate can see**: the authorable-surface ratchet +> compares key sets, and a default is not a key. It is called out here because a +> changeset is the only channel that carries it. + +**3. Two bounds now apply to jobs that did not have them** — `maxRetries` is capped at +**10** and `backoffMultiplier` floored at **1**. Both fail loudly at parse time rather +than being silently reinterpreted; neither has a lossless rewrite, so they are recorded +as the `job-retry-policy-constraints-tightened` semantic migration note. A multiplier +below 1 described a delay that *shrinks* on each attempt — retrying a failing dependency +ever faster, the opposite of backoff. + +**4. `import type { RetryPolicy } from '@objectstack/spec/system'` is now the input +shape** (every key optional) rather than the post-parse shape. Use the new +`RetryPolicyParsed` where you need defaults applied. + +## Not related to `mapping.errorPolicy` (#4509 / #4664, same release) + +17.0.0 also retires `mapping.errorPolicy`, whose values included `'retry'`. That is a +different thing on a different type: an inert enum on the stored **mapping**, whose +prescription is "error handling on the import path belongs to the import REQUEST's own +options". It does **not** migrate to a `retryPolicy` block, and nothing in this change +affects it. + +## What you gain + +`job.retryPolicy` accepts **`maxRetryDelayMs`** (ceiling on a single backoff delay) and +**`jitter`** (randomize each delay into [50%, 100%]). Both are enforced by +`runWithPolicy`, not merely declared — jitter is what stops a fleet of jobs that failed +on one outage from retrying in lockstep. diff --git a/content/docs/references/automation/control-flow.mdx b/content/docs/references/automation/control-flow.mdx index 147bbdf545..5c98907410 100644 --- a/content/docs/references/automation/control-flow.mdx +++ b/content/docs/references/automation/control-flow.mdx @@ -149,7 +149,7 @@ const result = FlowRegionSchema.parse(data); | **try** | `{ nodes: { id: string; type: string; label: string; config?: Record; … }[]; edges?: { id: string; source: string; target: string; condition?: string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }; … }[] }` | ✅ | Protected region | | **catch** | `{ nodes: { id: string; type: string; label: string; config?: Record; … }[]; edges?: { id: string; source: string; target: string; condition?: string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }; … }[] }` | optional | Handler region run when the try region fails | | **errorVariable** | `string` | optional | Variable holding the caught error in the catch region | -| **retry** | `{ maxRetries?: integer; retryDelayMs?: integer; backoffMultiplier?: number; maxRetryDelayMs?: integer; … }` | optional | Optional retry policy for the try region | +| **retry** | `{ maxRetries?: integer; backoffMs?: integer; backoffMultiplier?: number; maxRetryDelayMs?: integer; … }` | optional | Optional retry policy for the try region | --- diff --git a/content/docs/references/automation/job.mdx b/content/docs/references/automation/job.mdx deleted file mode 100644 index 6acf71d321..0000000000 --- a/content/docs/references/automation/job.mdx +++ /dev/null @@ -1,34 +0,0 @@ ---- -title: Job -description: Job protocol schemas ---- - -{/* ⚠️ AUTO-GENERATED — DO NOT EDIT. Run build-docs.ts to regenerate. Hand-written docs live in the module folders under content/docs/. */} - -## TypeScript Usage - -```typescript -import { RetryPolicySchema } from '@objectstack/spec/automation'; -import type { RetryPolicy } from '@objectstack/spec/automation'; - -// Validate data -const result = RetryPolicySchema.parse(data); -``` - ---- - -## RetryPolicy - -### Properties - -| Property | Type | Required | Description | -| :--- | :--- | :--- | :--- | -| **maxRetries** | `integer` | ✅ | Retry attempts before giving up | -| **retryDelayMs** | `integer` | ✅ | Base delay between retries (ms) | -| **backoffMultiplier** | `number` | ✅ | Exponential backoff multiplier | -| **maxRetryDelayMs** | `integer` | ✅ | Maximum delay between retries (ms) | -| **jitter** | `boolean` | ✅ | Add random jitter to retry delay | - - ---- - diff --git a/content/docs/references/automation/meta.json b/content/docs/references/automation/meta.json index 1021c8f5b5..2745091a7f 100644 --- a/content/docs/references/automation/meta.json +++ b/content/docs/references/automation/meta.json @@ -17,12 +17,12 @@ "webhook", "---Approvals & Jobs---", "approval", - "job", "---More---", "builtin-node-config", "events-core", "flow-function", "io-node-config", + "retry-policy", "schemaless-node-config" ] } \ No newline at end of file diff --git a/content/docs/references/automation/retry-policy.mdx b/content/docs/references/automation/retry-policy.mdx new file mode 100644 index 0000000000..e89e986509 --- /dev/null +++ b/content/docs/references/automation/retry-policy.mdx @@ -0,0 +1,35 @@ +--- +title: Retry Policy +description: Retry Policy protocol schemas +--- + +{/* ⚠️ AUTO-GENERATED — DO NOT EDIT. Run build-docs.ts to regenerate. Hand-written docs live in the module folders under content/docs/. */} + +## TypeScript Usage + +```typescript +import { RetryPolicySchema } from '@objectstack/spec/automation'; +import type { RetryPolicy } from '@objectstack/spec/automation'; + +// Validate data +const result = RetryPolicySchema.parse(data); +``` + +--- + +## RetryPolicy + +### Properties + +| Property | Type | Required | Description | +| :--- | :--- | :--- | :--- | +| **maxRetries** | `integer` | ✅ | Retry attempts after the initial one. 0 (the default) means no retry — state a count to opt in. | +| **backoffMs** | `integer` | ✅ | Base delay before the first retry (ms); subsequent delays multiply by backoffMultiplier | +| **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. | + + +--- + diff --git a/content/docs/references/system/job.mdx b/content/docs/references/system/job.mdx index 54d159f9d2..627262997c 100644 --- a/content/docs/references/system/job.mdx +++ b/content/docs/references/system/job.mdx @@ -16,8 +16,8 @@ Schedule jobs using cron expressions ## TypeScript Usage ```typescript -import { CronScheduleSchema, IntervalScheduleSchema, JobSchema, JobExecutionSchema, JobExecutionStatus, OnceScheduleSchema, RetryPolicySchema, ScheduleSchema } from '@objectstack/spec/system'; -import type { CronSchedule, IntervalSchedule, Job, JobExecution, JobExecutionStatus, OnceSchedule, RetryPolicy, Schedule } from '@objectstack/spec/system'; +import { CronScheduleSchema, IntervalScheduleSchema, JobSchema, JobExecutionSchema, JobExecutionStatus, OnceScheduleSchema, ScheduleSchema } from '@objectstack/spec/system'; +import type { CronSchedule, IntervalSchedule, Job, JobExecution, JobExecutionStatus, OnceSchedule, Schedule } from '@objectstack/spec/system'; // Validate data const result = CronScheduleSchema.parse(data); @@ -62,7 +62,7 @@ const result = CronScheduleSchema.parse(data); | **description** | `string` | optional | Job description / purpose | | **schedule** | `{ type: 'cron'; expression: string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }; timezone?: string } \| { type: 'interval'; intervalMs: integer } \| { type: 'once'; at: string }` | ✅ | Job schedule configuration | | **handler** | `string` | ✅ | Handler function name (must match a key in `defineStack({ functions })`) | -| **retryPolicy** | `{ maxRetries?: integer; backoffMs?: integer; backoffMultiplier?: number }` | optional | Retry policy: failed runs (including timeouts) are retried with exponential backoff (delay = backoffMs * backoffMultiplier^(retry-1)) up to maxRetries retries after the initial attempt (#3494). Omit for the legacy single-attempt behavior. | +| **retryPolicy** | `{ maxRetries?: integer; backoffMs?: integer; backoffMultiplier?: number; maxRetryDelayMs?: integer; … }` | optional | Retry policy: failed runs (including timeouts) are retried with exponential backoff (delay = min(backoffMs * backoffMultiplier^(retry-1), maxRetryDelayMs), optionally jittered) up to maxRetries retries after the initial attempt (#3494). Omit the block for a single attempt; declaring it without `maxRetries` also means no retry since 17.0.0 (#4661) — state a count to opt in. | | **timeout** | `integer` | optional | Per-attempt time limit in milliseconds; an over-limit run is recorded with execution status "timeout" (#3494). The in-flight handler is abandoned, not forcibly cancelled. Omit for no time limit. | | **enabled** | `boolean` | optional | Whether the job is enabled | | **_lock** | `Enum<'none' \| 'no-overlay' \| 'no-delete' \| 'full'>` | optional | Item-level lock — controls overlay & delete (ADR-0010). | @@ -114,19 +114,6 @@ const result = CronScheduleSchema.parse(data); | **at** | `string` | ✅ | ISO 8601 datetime when to execute | ---- - -## RetryPolicy - -### Properties - -| Property | Type | Required | Description | -| :--- | :--- | :--- | :--- | -| **maxRetries** | `integer` | ✅ | Maximum number of retry attempts | -| **backoffMs** | `integer` | ✅ | Initial backoff delay in milliseconds | -| **backoffMultiplier** | `number` | ✅ | Multiplier for exponential backoff | - - --- ## Schedule diff --git a/content/docs/references/system/meta.json b/content/docs/references/system/meta.json index 42f1559f7a..56a5dde8a0 100644 --- a/content/docs/references/system/meta.json +++ b/content/docs/references/system/meta.json @@ -45,6 +45,7 @@ "collaboration", "doc", "---More---", - "metadata-types" + "metadata-types", + "retry-policy" ] } \ No newline at end of file diff --git a/content/docs/references/system/retry-policy.mdx b/content/docs/references/system/retry-policy.mdx new file mode 100644 index 0000000000..b8b7822586 --- /dev/null +++ b/content/docs/references/system/retry-policy.mdx @@ -0,0 +1,35 @@ +--- +title: Retry Policy +description: Retry Policy protocol schemas +--- + +{/* ⚠️ AUTO-GENERATED — DO NOT EDIT. Run build-docs.ts to regenerate. Hand-written docs live in the module folders under content/docs/. */} + +## TypeScript Usage + +```typescript +import { RetryPolicySchema } from '@objectstack/spec/system'; +import type { RetryPolicy } from '@objectstack/spec/system'; + +// Validate data +const result = RetryPolicySchema.parse(data); +``` + +--- + +## RetryPolicy + +### Properties + +| Property | Type | Required | Description | +| :--- | :--- | :--- | :--- | +| **maxRetries** | `integer` | ✅ | Retry attempts after the initial one. 0 (the default) means no retry — state a count to opt in. | +| **backoffMs** | `integer` | ✅ | Base delay before the first retry (ms); subsequent delays multiply by backoffMultiplier | +| **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. | + + +--- + diff --git a/docs/audits/2026-07-unknown-key-strictness-ledger.md b/docs/audits/2026-07-unknown-key-strictness-ledger.md index 0404cf4cc4..607ff76ac0 100644 --- a/docs/audits/2026-07-unknown-key-strictness-ledger.md +++ b/docs/audits/2026-07-unknown-key-strictness-ledger.md @@ -477,7 +477,7 @@ tightening (the #4001 "sharing-rule lesson": candidates, not verdicts). | `validation.zod.ts` | 6 | authorable | **strict as of #4001 batch 3b** — a `z.lazy()` discriminated union, so the one-call conversion does not apply: each of the six variants builds its own `strictObject` from a shared `BASE_VALIDATION_SHAPE`. Closing the base alone would have rejected correctly but suggested from the SHARED keys only, so a typo of a variant's own key (`transtions` → `transitions`) would get no rename. Site count 1 → 6 because the six variants are now object sites in their own right. The ADR-0010 envelope lives in the shared shape, so all six inherit it | | `field-value.zod.ts` / `seed.zod.ts` | 1+1 | mixed (p) | `seed` is strict (registered-types batch) | -### `automation/` — 88 sites +### `automation/` — 87 sites | File | Sites | Class | Note | |---|---|---|---| @@ -485,7 +485,7 @@ tightening (the #4001 "sharing-rule lesson": candidates, not verdicts). | `sync.zod.ts` / `etl.zod.ts` | 12+10 | authorable (p) | authored pipelines — **candidates** | | `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` | 7 | authorable (p) | | -| `control-flow.zod.ts` | 6 | authorable (p) | validated structurally by `validateControlFlow` | +| `control-flow.zod.ts` | 5 | authorable (p) | validated structurally by `validateControlFlow`. **−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 | | `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 | diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 0c5568f8ea..88aace74c8 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -168,6 +168,10 @@ Separately, `object.managedBy: 'system'` is retired in favour of `'system-data'` Finally, five keys retire because the advisory lint could never have warned about them (#4509): mapping `extractQuery` / `errorPolicy` / `batchSize`, and app `contextSelectors[].includeAll` / `.placement`. Four of the five carry schema DEFAULTS, and a default materialises at parse time — so the liveness lint cannot tell a value the author wrote from one the schema supplied, and marking them would have warned on every mapping and every selector in existence. For a key in that state removal is not the escalation after a warning; it is the only channel that ever reaches the author, which is why they ship inside the 17.0.0 window rather than after a deprecation cycle. What they claimed: `extractQuery` promised an export path no exporter implements (exports go through the ordinary query API); `errorPolicy` offered skip/abort/retry where error handling belongs to the import REQUEST; `batchSize` sized batches the write path sizes itself; `placement` offered a topbar that places nothing. `includeAll` is the one worth reading twice — it was not unread but deliberately DISOBEYED, because context selectors are mandatory-scope and an "All" row would clear the scope: on Studio's package selector that means listing the platform's own system/cloud kernel packages to a developer who scoped to their package. `STUDIO_APP` authored `includeAll: true` against a renderer that ignored it. The mapping prescription for `batchSize` deliberately offers no rename: bulk-action, connector, sync, offline, seed-loader and NoSQL-cursor `batchSize` are all live, but each is a different key sizing its own path — the same trap `datasource.retryPolicy` vs `hook`/`job` `retryPolicy` had to defuse one issue earlier. +The same window converges the retry policy (#4661). `@objectstack/spec/automation` and `@objectstack/spec/system` each exported a `RetryPolicy`/`RetryPolicySchema` resolving to a DIFFERENT declaration, so which shape a consumer got depended only on the import path (#4411) — yet both computed `delay = base * multiplier^(retry-1)` and both executors implemented that same formula. One declaration now serves both entries with the union of their capabilities, so `job.retryPolicy` gains the `maxRetryDelayMs` ceiling and `jitter` (both enforced in `runWithPolicy`, not merely declared — jitter is what stops a fleet of jobs that failed on one outage from retrying in lockstep). The single authorable casualty is the automation spelling of the base delay: `retryDelayMs` → `backoffMs`, a pure rename that replays losslessly and is what the already-enforced retry policies (`job.retryPolicy`, `hook.retryPolicy`) call it. + +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. + ### Mechanical (applied for you) | Conversion | Surface | Change | Load window | @@ -201,10 +205,14 @@ Finally, five keys retire because the advisory lint could never have warned abou | `mapping-inert-keys-removed` | `mapping.extractQuery / mapping.errorPolicy / mapping.batchSize` | mapping keys 'extractQuery'/'errorPolicy'/'batchSize' removed (#4509 — no exporter reads a mapping, error handling belongs to the import request, and the write path sizes its own batches) | 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 | | `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 | ### Semantic (delegated to you, with acceptance criteria) +- **`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. - **`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/packages/services/service-automation/src/builtin/config-schemas.test.ts b/packages/services/service-automation/src/builtin/config-schemas.test.ts index 2c70cabe59..20b9be0fc6 100644 --- a/packages/services/service-automation/src/builtin/config-schemas.test.ts +++ b/packages/services/service-automation/src/builtin/config-schemas.test.ts @@ -203,6 +203,16 @@ describe('builtin node configSchemas — designer parity (#3304)', () => { expect(retry?.maxRetries?.maximum).toBe(10); expect(retry?.backoffMultiplier?.minimum).toBe(1); expect(retry?.jitter?.type).toBe('boolean'); + + // #4661: the base delay is `backoffMs` — one spelling shared with + // `job.retryPolicy` since the retry policy converged onto a single + // declaration. Pinned here because the form/Zod ledger test compares only + // the TOP-LEVEL config keys (`try`/`catch`/`errorVariable`/`retry`), so + // nothing else would notice this nested key being renamed back or the + // Studio form drifting off the contract it is supposed to drive. + expect(retry?.backoffMs?.type).toBe('integer'); + expect(retry?.backoffMs?.minimum).toBe(0); + expect(retry?.retryDelayMs).toBeUndefined(); }); }); diff --git a/packages/services/service-automation/src/builtin/try-catch-node.ts b/packages/services/service-automation/src/builtin/try-catch-node.ts index 6faf3f5b09..bf497e402b 100644 --- a/packages/services/service-automation/src/builtin/try-catch-node.ts +++ b/packages/services/service-automation/src/builtin/try-catch-node.ts @@ -59,7 +59,9 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex type: 'object', properties: { maxRetries: { type: 'integer', minimum: 0, maximum: 10 }, - retryDelayMs: { type: 'integer', minimum: 0 }, + // `backoffMs` (was `retryDelayMs`) since spec 17.0.0 — one retry + // policy spelling across job.retryPolicy and try_catch (#4661). + backoffMs: { type: 'integer', minimum: 0 }, backoffMultiplier: { type: 'number', minimum: 1 }, maxRetryDelayMs: { type: 'integer', minimum: 0 }, jitter: { type: 'boolean' }, @@ -72,9 +74,11 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex async execute(node, variables, context) { // Parse against the ADR-0031 contract. Note the retry defaults now come // from the CONTRACT (RetryPolicySchema): a declared `retry` block that - // omits `retryDelayMs` gets the documented 1000ms base delay, where this + // omits `backoffMs` gets the documented 1000ms base delay, where this // executor historically filled in 0 — the declared default is the - // enforced one (#4277). + // enforced one (#4277). Since spec 17.0.0 that contract is one shared + // declaration with `job.retryPolicy`, and the base delay is spelled + // `backoffMs` (was `retryDelayMs`, tombstoned + converted — #4661). const parsed = parseNodeConfig('try_catch', node.id, TryCatchConfigSchema, node.config); if (!parsed.ok) return parsed.refusal; const cfg = parsed.config; @@ -85,7 +89,7 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex const ctxOrEmpty = context ?? ({} as AutomationContext); const maxRetries = retry?.maxRetries ?? 0; - const baseDelay = retry?.retryDelayMs ?? 0; + const baseDelay = retry?.backoffMs ?? 0; const multiplier = retry?.backoffMultiplier ?? 1; const maxDelay = retry?.maxRetryDelayMs ?? 30000; const useJitter = retry?.jitter === true; diff --git a/packages/services/service-job/src/run-with-policy.ts b/packages/services/service-job/src/run-with-policy.ts index 07c67c5357..2dbe2fb93b 100644 --- a/packages/services/service-job/src/run-with-policy.ts +++ b/packages/services/service-job/src/run-with-policy.ts @@ -13,7 +13,25 @@ export class JobTimeoutError extends Error { } } -const RETRY_DEFAULTS = { maxRetries: 3, backoffMs: 1000, backoffMultiplier: 2 } as const; +/** + * Mirrors the declared defaults of `RetryPolicySchema` + * (`@objectstack/spec` `shared/retry-policy.zod.ts`) — the declared default IS + * the enforced one (#4277). They apply only to a caller that hand-builds + * `JobScheduleOptions` without going through Zod; an authored `job.retryPolicy` + * arrives already defaulted. + * + * `maxRetries` was 3 and `backoffMultiplier` 2 before 17.0.0 (#4661). Existing + * job documents keep those numbers — the `retry-policy-converged` + * conversion writes them in — so this change is only about what an omission + * means from now on: no retry unless asked for. + */ +const RETRY_DEFAULTS = { + maxRetries: 0, + backoffMs: 1000, + backoffMultiplier: 1, + maxRetryDelayMs: 30000, + jitter: false, +} as const; function sleep(ms: number): Promise { return new Promise((resolve) => { @@ -41,9 +59,14 @@ function withTimeout(run: () => Promise, jobId: string, timeoutMs?: number * {@link JobTimeoutError}. JavaScript cannot forcibly cancel the in-flight * handler — the attempt is abandoned, not killed. * - `retryPolicy` re-runs failed attempts (including timeouts) with - * exponential backoff: delay = backoffMs * backoffMultiplier^(retry-1), - * up to maxRetries retries after the initial attempt. The last error is - * rethrown when all attempts fail. + * exponential backoff: delay = min(backoffMs * backoffMultiplier^(retry-1), + * maxRetryDelayMs), randomized into [50%, 100%] when `jitter` is set, up to + * maxRetries retries after the initial attempt. The last error is rethrown + * when all attempts fail. + * - `maxRetryDelayMs` / `jitter` arrived with the 17.0.0 retry-policy + * convergence (#4661) and are enforced here, not merely declared: jitter is + * what stops a fleet of jobs that failed on the same outage from retrying in + * lockstep. */ export async function runWithPolicy( jobId: string, @@ -58,11 +81,16 @@ export async function runWithPolicy( const maxRetries = options.retryPolicy.maxRetries ?? RETRY_DEFAULTS.maxRetries; const backoffMs = options.retryPolicy.backoffMs ?? RETRY_DEFAULTS.backoffMs; const multiplier = options.retryPolicy.backoffMultiplier ?? RETRY_DEFAULTS.backoffMultiplier; + const maxRetryDelayMs = options.retryPolicy.maxRetryDelayMs ?? RETRY_DEFAULTS.maxRetryDelayMs; + const jitter = options.retryPolicy.jitter ?? RETRY_DEFAULTS.jitter; let lastError: unknown; for (let attempt = 0; attempt <= maxRetries; attempt++) { if (attempt > 0) { - await sleep(backoffMs * Math.pow(multiplier, attempt - 1)); + // Same formula the try_catch executor runs — one policy, one backoff. + let delay = Math.min(backoffMs * Math.pow(multiplier, attempt - 1), maxRetryDelayMs); + if (jitter) delay = delay * (0.5 + Math.random() * 0.5); + await sleep(delay); } try { await withTimeout(run, jobId, timeoutMs); diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 45aa1895bd..b6d6151030 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -1196,6 +1196,7 @@ "ResolverDoc (interface)", "ResultDialogLike (interface)", "RetryPolicy (type)", + "RetryPolicyParsed (type)", "RetryPolicySchema (const)", "RollbackPlan (type)", "RollbackPlanSchema (const)", @@ -2259,6 +2260,7 @@ "RegionAnalysis (interface)", "ResolvedFlowNodeExpression (interface)", "RetryPolicy (type)", + "RetryPolicyParsed (type)", "RetryPolicySchema (const)", "SCHEMALESS_NODE_CONFIG_SCHEMAS (const)", "ScheduleState (type)", diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index 4c8fe0787f..21d30bb563 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -2436,11 +2436,12 @@ "automation/ParallelBranch:name", "automation/ParallelBranch:nodes", "automation/ParallelConfig:branches", + "automation/RetryPolicy:backoffMs", "automation/RetryPolicy:backoffMultiplier", "automation/RetryPolicy:jitter", "automation/RetryPolicy:maxRetries", "automation/RetryPolicy:maxRetryDelayMs", - "automation/RetryPolicy:retryDelayMs", + "automation/RetryPolicy:retryDelayMs [RETIRED]", "automation/ScheduleState:consecutiveFailures", "automation/ScheduleState:createdAt", "automation/ScheduleState:createdBy", @@ -6626,7 +6627,10 @@ "system/ResolvedSettingValue:value", "system/RetryPolicy:backoffMs", "system/RetryPolicy:backoffMultiplier", + "system/RetryPolicy:jitter", "system/RetryPolicy:maxRetries", + "system/RetryPolicy:maxRetryDelayMs", + "system/RetryPolicy:retryDelayMs [RETIRED]", "system/RollbackPlan:description", "system/RollbackPlan:steps", "system/RollbackPlan:testProcedure", diff --git a/packages/spec/dual-source-exports.baseline.json b/packages/spec/dual-source-exports.baseline.json index 4f054737cc..3d2885b5e2 100644 --- a/packages/spec/dual-source-exports.baseline.json +++ b/packages/spec/dual-source-exports.baseline.json @@ -18,8 +18,6 @@ "PackageDependencySchema — [./cloud (const)] ≠ [./kernel (const)]", "RateLimitConfig — [./integration (type)] ≠ [./shared (type)]", "RateLimitConfigSchema — [./integration (const)] ≠ [./shared (const)]", - "RetryPolicy — [./automation (type)] ≠ [./system (type)]", - "RetryPolicySchema — [./automation (const)] ≠ [./system (const)]", "TenantPlan — [./cloud (type)] ≠ [./system (type)]", "TenantPlanSchema — [./cloud (const)] ≠ [./system (const)]" ] diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index 08d240f1fb..f1704f966d 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -242,6 +242,12 @@ "conversionId": "flow-node-script-branch-keys-removed", "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)", + "conversionId": "retry-policy-converged", + "toMajor": 17 + }, { "surface": "object.managedBy", "to": "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)", @@ -320,6 +326,13 @@ "toMajor": 16, "rationale": "The `.strict()` flip turns a previously silently-stripped unknown key into a parse error. There is no mapping target for an arbitrary unknown key — auto-deleting it would be exactly the silent data loss ADR-0078 bans — so each occurrence needs the author to decide: bind a `dataset` and select `dimensions`/`values`, move a renderer setting under `options`, or delete the dead key." }, + { + "surface": "job.retryPolicy.maxRetries (> 10) / job.retryPolicy.backoffMultiplier (< 1)", + "replacement": "maxRetries <= 10, and backoffMultiplier >= 1", + "migrationId": "job-retry-policy-constraints-tightened", + "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": "flow.errorHandling.maxRetries (under strategy: 'retry')", "replacement": "an explicit count >= 1 (e.g. maxRetries: 3), or strategy: 'fail'", @@ -774,6 +787,12 @@ "conversionId": "flow-node-script-branch-keys-removed", "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)", + "conversionId": "retry-policy-converged", + "toMajor": 17 + }, { "surface": "object.managedBy", "to": "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)", @@ -782,6 +801,13 @@ } ], "migrated": [ + { + "surface": "job.retryPolicy.maxRetries (> 10) / job.retryPolicy.backoffMultiplier (< 1)", + "replacement": "maxRetries <= 10, and backoffMultiplier >= 1", + "migrationId": "job-retry-policy-constraints-tightened", + "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": "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/control-flow.test.ts b/packages/spec/src/automation/control-flow.test.ts index 5485ab671e..985c0eeabe 100644 --- a/packages/spec/src/automation/control-flow.test.ts +++ b/packages/spec/src/automation/control-flow.test.ts @@ -104,10 +104,37 @@ describe('TryCatchConfigSchema', () => { const parsed = TryCatchConfigSchema.parse({ try: { nodes: [node('t')] }, catch: { nodes: [node('c')] }, - retry: { maxRetries: 3, retryDelayMs: 500 }, + retry: { maxRetries: 3, backoffMs: 500 }, }); expect(parsed.errorVariable).toBe('$error'); expect(parsed.retry?.maxRetries).toBe(3); + expect(parsed.retry?.backoffMs).toBe(500); + }); + + // #4661: `retryDelayMs` was the automation spelling of `backoffMs` before the + // retry policy converged onto one declaration. It is TOMBSTONED rather than + // deleted precisely because this schema is not `.strict()` — a plain removal + // would have Zod swallow the authored number and silently fall back to the + // 1000ms default. Assert the loud rejection, not merely its absence. + it('rejects the retired `retryDelayMs` spelling with the rename prescription', () => { + const parse = () => TryCatchConfigSchema.parse({ + try: { nodes: [node('t')] }, + retry: { maxRetries: 3, retryDelayMs: 500 }, + }); + expect(parse).toThrow(/backoffMs/); + expect(parse).toThrow(/retryDelayMs/); + }); + + it('retry is opt-in: a declared but empty retry block does not retry', () => { + const parsed = TryCatchConfigSchema.parse({ + try: { nodes: [node('t')] }, + retry: {}, + }); + expect(parsed.retry?.maxRetries).toBe(0); + expect(parsed.retry?.backoffMs).toBe(1000); + expect(parsed.retry?.backoffMultiplier).toBe(1); + expect(parsed.retry?.maxRetryDelayMs).toBe(30000); + expect(parsed.retry?.jitter).toBe(false); }); }); diff --git a/packages/spec/src/automation/control-flow.zod.ts b/packages/spec/src/automation/control-flow.zod.ts index 9a29ccfc7b..796aa43577 100644 --- a/packages/spec/src/automation/control-flow.zod.ts +++ b/packages/spec/src/automation/control-flow.zod.ts @@ -165,18 +165,22 @@ export type ParallelConfigParsed = z.infer; /** * Structured retry policy — surfaces the engine's existing exponential-backoff - * retry (`FlowSchema.errorHandling`) as a per-construct policy. Mirrors that - * shape so the engine can reuse one backoff implementation. + * retry (`FlowSchema.errorHandling`) as a per-construct policy. + * + * The declaration moved to `shared/retry-policy.zod.ts` in 17.0.0 (#4661): the + * identically-named `system/job.zod.ts` shape was the same concept under a + * different spelling, so `@objectstack/spec/automation` and + * `@objectstack/spec/system` handed out two different `RetryPolicy` types for + * one idea (the #4411 trap). One declaration now serves both entries. For THIS + * entry the visible change is the base delay: `retryDelayMs` → `backoffMs` + * (tombstoned, with a conversion), plus `maxRetries`/`backoffMultiplier` + * defaults that are unchanged here — 0 and 1 were already the automation values. + * + * Re-exported so `./automation` keeps publishing the name (and its + * `automation/RetryPolicy` JSON-Schema def, which is keyed by entry namespace). */ -export const RetryPolicySchema = lazySchema(() => z.object({ - maxRetries: z.number().int().min(0).max(10).default(0).describe('Retry attempts before giving up'), - retryDelayMs: z.number().int().min(0).default(1000).describe('Base delay between retries (ms)'), - backoffMultiplier: z.number().min(1).default(1).describe('Exponential backoff multiplier'), - maxRetryDelayMs: z.number().int().min(0).default(30000).describe('Maximum delay between retries (ms)'), - jitter: z.boolean().default(false).describe('Add random jitter to retry delay'), -})); - -export type RetryPolicy = z.input; +export { RetryPolicySchema, type RetryPolicy, type RetryPolicyParsed } from '../shared/retry-policy.zod'; +import { RetryPolicySchema } from '../shared/retry-policy.zod'; /** * `try_catch` config — structured error handling. The `try` region runs; if it diff --git a/packages/spec/src/contracts/job-service.ts b/packages/spec/src/contracts/job-service.ts index 10c4f1af87..876eb2e50d 100644 --- a/packages/spec/src/contracts/job-service.ts +++ b/packages/spec/src/contracts/job-service.ts @@ -50,16 +50,25 @@ export interface JobSchedule { export type JobHandler = (context: { jobId: string; data?: unknown }) => Promise; /** - * Retry policy for a scheduled job (mirrors the authorable - * `RetryPolicySchema` in system/job.zod.ts). + * Retry policy for a scheduled job (mirrors the authorable `RetryPolicySchema`, + * which since 17.0.0 is declared once in `shared/retry-policy.zod.ts` and + * re-exported by both `./automation` and `./system` — #4661). + * + * Defaults restated here because this is a hand-written boundary type: a caller + * that builds `JobScheduleOptions` itself never goes through Zod, so + * `runWithPolicy` applies the same values the schema declares. */ export interface JobRetryPolicy { - /** Maximum number of retry attempts after the initial run (default 3) */ + /** Retry attempts after the initial run. 0 (the default since 17.0.0, #4661) means no retry. */ maxRetries?: number; - /** Initial backoff delay in milliseconds (default 1000) */ + /** Base delay before the first retry, in milliseconds (default 1000) */ backoffMs?: number; - /** Multiplier for exponential backoff (default 2) */ + /** Multiplier for exponential backoff (default 1 since 17.0.0, #4661 — a flat delay) */ backoffMultiplier?: number; + /** Ceiling for a single backoff delay, in milliseconds (default 30000) */ + maxRetryDelayMs?: number; + /** Randomize each delay within [50%, 100%] of its computed value (default false) */ + jitter?: boolean; } /** diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index 3f509b264f..6df4eeb25d 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -2914,6 +2914,159 @@ const objectManagedBySystemToSystemData: MetadataConversion = { }, }; +/** + * The retry policy converges to one declaration (protocol 17, #4661 — the + * #4535 C8 dual-source cluster). + * + * `@objectstack/spec/automation` and `@objectstack/spec/system` both exported a + * `RetryPolicy` / `RetryPolicySchema`, resolving to DIFFERENT declarations — so + * which shape a consumer got depended only on the import path (#4411). They + * were never two concepts: `try_catch`'s `retry` region and `job.retryPolicy` + * both compute `delay = base * multiplier^(retry-1)`, and the two executors + * implemented that identical formula. What differed was cosmetic and + * accidental, and this conversion pays for both halves of the merge: + * + * 1. **The base delay had two spellings.** automation said `retryDelayMs`, + * system said `backoffMs`. `backoffMs` wins — it is what the *enforced* + * retry policies already spell it (`job.retryPolicy` and `hook.retryPolicy`; + * 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. + * + * 2. **The defaults were opposite, and no gate can see a default.** Pre-17, + * `job.retryPolicy` defaulted `maxRetries: 3` / `backoffMultiplier: 2` + * while the automation shape defaulted 0 / 1. The merged declaration takes + * 0 / 1 (retry is opt-in: a retry replays whatever the attempt already did, + * and an implicit replay of side effects is the failure mode hardest to + * catch in tests — the same reading already recorded for flow-level retry + * in `flow-retry-max-retries-required`, #4247). Taken alone that would + * SILENTLY stop existing jobs from retrying, and the authorable-surface + * gate would never notice: it compares key sets, and a default is not a key. + * So this conversion writes the pre-17 numbers into every existing + * `job.retryPolicy` that omitted them. Deployed stacks keep their exact + * behaviour; only a newly authored omission means "no retry". + * + * Jobs with no `retryPolicy` block at all are left alone — absence already + * meant a single attempt on both sides of the change. + * + * `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. + * The `RetryPolicySchema` tombstone still fires for anyone who reaches the + * policy through a parsed job. + */ +const retryPolicyConverged: MetadataConversion = { + id: 'retry-policy-converged', + toMajor: 17, + surface: '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)", + apply(stack, emit) { + // ── 1. try_catch nodes: retry.retryDelayMs → retry.backoffMs ────── + const withFlows = mapFlowNodes(stack, (node, path) => { + if (node.type !== 'try_catch') return node; + const config = node.config; + if (!config || typeof config !== 'object' || Array.isArray(config)) return node; + const configDict = config as Record; + const retry = configDict.retry; + if (!retry || typeof retry !== 'object' || Array.isArray(retry)) return node; + const renamed = renameKey(retry as Record, 'retryDelayMs', 'backoffMs'); + if (renamed === null) return node; + emit({ from: 'retryDelayMs', to: 'backoffMs', path: `${path}.config.retry.backoffMs` }); + return { ...node, config: { ...configDict, retry: renamed } }; + }); + + // ── 2. jobs: materialize the pre-17 implicit defaults ───────────── + return mapCollection(withFlows, 'jobs', (job, path) => { + const policy = job.retryPolicy; + if (!policy || typeof policy !== 'object' || Array.isArray(policy)) return job; + const policyDict = policy as Record; + let next = policyDict; + if (next.maxRetries === undefined) { + next = { ...next, maxRetries: 3 }; + emit({ + from: 'maxRetries unset (implied 3)', + to: 'maxRetries: 3', + path: `${path}.retryPolicy.maxRetries`, + }); + } + if (next.backoffMultiplier === undefined) { + next = { ...next, backoffMultiplier: 2 }; + emit({ + from: 'backoffMultiplier unset (implied 2)', + to: 'backoffMultiplier: 2', + path: `${path}.retryPolicy.backoffMultiplier`, + }); + } + return next === policyDict ? job : { ...job, retryPolicy: next }; + }); + }, + fixture: { + before: { + flows: [{ + name: 'sync_orders', + nodes: [ + { id: 'n1', type: 'start' }, + { + id: 'n2', + type: 'try_catch', + config: { + try: { nodes: [], edges: [] }, + retry: { maxRetries: 3, retryDelayMs: 500, jitter: true }, + }, + }, + // Already canonical — left alone, contributes no notice. + { + id: 'n3', + type: 'try_catch', + config: { try: { nodes: [], edges: [] }, retry: { maxRetries: 2, backoffMs: 250 } }, + }, + ], + }], + jobs: [ + // Omits both defaults — both get written out. + { name: 'nightly_sync', schedule: { type: 'cron', expression: '0 0 * * *' }, handler: 'jobs.ts:sync', retryPolicy: { backoffMs: 5000 } }, + // States both — untouched. + { name: 'hourly_roll', schedule: { type: 'cron', expression: '0 * * * *' }, handler: 'jobs.ts:roll', retryPolicy: { maxRetries: 1, backoffMultiplier: 3 } }, + // No policy block at all — absence already meant one attempt. + { name: 'weekly_purge', schedule: { type: 'cron', expression: '0 0 * * 0' }, handler: 'jobs.ts:purge' }, + ], + }, + after: { + flows: [{ + name: 'sync_orders', + nodes: [ + { id: 'n1', type: 'start' }, + { + id: 'n2', + type: 'try_catch', + config: { + try: { nodes: [], edges: [] }, + retry: { maxRetries: 3, jitter: true, backoffMs: 500 }, + }, + }, + { + id: 'n3', + type: 'try_catch', + config: { try: { nodes: [], edges: [] }, retry: { maxRetries: 2, backoffMs: 250 } }, + }, + ], + }], + jobs: [ + { name: 'nightly_sync', schedule: { type: 'cron', expression: '0 0 * * *' }, handler: 'jobs.ts:sync', retryPolicy: { backoffMs: 5000, maxRetries: 3, backoffMultiplier: 2 } }, + { name: 'hourly_roll', schedule: { type: 'cron', expression: '0 * * * *' }, handler: 'jobs.ts:roll', retryPolicy: { maxRetries: 1, backoffMultiplier: 3 } }, + { 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, + }, +}; + export const CONVERSIONS_BY_MAJOR: Readonly> = { 11: [flowNodeHttpRename, pageKindJsxToHtml, flowNodeFilterAlias, objectCompactLayoutRename], 13: [stackRolesToPositions, owdLegacyReadAliases, sharingRecipientRoleToPosition], @@ -2951,6 +3104,7 @@ export const CONVERSIONS_BY_MAJOR: Readonly 10) / job.retryPolicy.backoffMultiplier (< 1)', + replacement: 'maxRetries <= 10, and backoffMultiplier >= 1', + reason: + '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.', + acceptanceCriteria: + '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.', + }, { 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 new file mode 100644 index 0000000000..7318f093a3 --- /dev/null +++ b/packages/spec/src/shared/retry-policy.test.ts @@ -0,0 +1,104 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The retry policy is ONE declaration (#4661 — the #4535 C8 dual-source cluster). + * + * `@objectstack/spec/automation` and `@objectstack/spec/system` both publish + * `RetryPolicySchema` / `RetryPolicy`. Until 17 those names resolved to two + * different declarations, so the shape a consumer got depended only on which + * entry they imported (the #4411 trap) — and the two disagreed on the base + * delay's spelling (`retryDelayMs` vs `backoffMs`), on two keys only one side + * had, and on the defaults for `maxRetries` / `backoffMultiplier`. + * + * ## Why these assertions run at RUNTIME + * + * A compile-time pin cannot fail in this package (#4642): `tsconfig.json` + * excludes `**\/*.test.ts` so `pnpm typecheck` never compiles this file, and + * vitest transpiles without typechecking — a conditional-type assertion here + * would be inert. Worse, `keyof typeof import(...)` enumerates only VALUE + * exports, so a bare type name cannot be asserted that way at all. These are + * reference-identity and shape checks on the loaded module namespaces, which + * actually execute. + * + * The reference-identity check is the load-bearing one: `check:dual-source-exports` + * judges by symbol identity, so `Automation.RetryPolicySchema === + * System.RetryPolicySchema` is exactly the invariant whose loss would put the + * two baseline rows back. + */ + +import { describe, it, expect } from 'vitest'; +import * as Automation from '../automation'; +import * as System from '../system'; +import { RetryPolicySchema } from './retry-policy.zod'; + +describe('RetryPolicy is a single declaration across entries (#4661)', () => { + it('./automation and ./system export the SAME RetryPolicySchema object', () => { + expect(Automation.RetryPolicySchema).toBe(System.RetryPolicySchema); + }); + + it('both entries re-export the canonical shared declaration', () => { + expect(Automation.RetryPolicySchema).toBe(RetryPolicySchema); + expect(System.RetryPolicySchema).toBe(RetryPolicySchema); + }); + + // Identity alone would still pass if someone converged the two onto a NEW + // shape that quietly dropped a key. Pin the authorable key set too — this is + // the same list `authorable-surface.json` carries for both def keys. + it('publishes exactly the converged key set from both entries', () => { + const expected = ['maxRetries', 'backoffMs', 'backoffMultiplier', 'maxRetryDelayMs', 'jitter']; + + for (const entry of [Automation.RetryPolicySchema, System.RetryPolicySchema]) { + const parsed = entry.parse({}); + expect(Object.keys(parsed).sort()).toEqual([...expected].sort()); + } + }); + + // The defaults are the half of this convergence NO gate can see: the + // authorable-surface ratchet compares key sets, and a default is not a key. + // If they are ever flipped back, deployed jobs silently start retrying again + // (or try_catch regions silently start replaying their side effects), so pin + // the numbers explicitly. + it('pins the opt-in defaults that no gate can observe', () => { + const parsed = RetryPolicySchema.parse({}); + + expect(parsed.maxRetries).toBe(0); + expect(parsed.backoffMs).toBe(1000); + expect(parsed.backoffMultiplier).toBe(1); + expect(parsed.maxRetryDelayMs).toBe(30000); + expect(parsed.jitter).toBe(false); + }); + + it('keeps the `retryDelayMs` tombstone loud from both entries', () => { + for (const entry of [Automation.RetryPolicySchema, System.RetryPolicySchema]) { + const parse = () => entry.parse({ retryDelayMs: 500 }); + expect(parse).toThrow(/backoffMs/); + } + }); +}); + +describe('RetryPolicySchema — converged shape', () => { + it('computes a bounded exponential backoff', () => { + const policy = RetryPolicySchema.parse({ + maxRetries: 5, + backoffMs: 1000, + backoffMultiplier: 2, + maxRetryDelayMs: 5000, + }); + + // The formula both executors run: min(base * multiplier^(n-1), ceiling). + const delays = [1, 2, 3, 4, 5].map((n) => + Math.min(policy.backoffMs * Math.pow(policy.backoffMultiplier, n - 1), policy.maxRetryDelayMs), + ); + + expect(delays).toEqual([1000, 2000, 4000, 5000, 5000]); + }); + + it('rejects a shrinking backoff (multiplier below 1)', () => { + expect(() => RetryPolicySchema.parse({ backoffMultiplier: 0.5 })).toThrow(); + }); + + it('caps maxRetries at 10', () => { + expect(() => RetryPolicySchema.parse({ maxRetries: 10 })).not.toThrow(); + expect(() => RetryPolicySchema.parse({ maxRetries: 11 })).toThrow(); + }); +}); diff --git a/packages/spec/src/shared/retry-policy.zod.ts b/packages/spec/src/shared/retry-policy.zod.ts new file mode 100644 index 0000000000..dfb7cfe842 --- /dev/null +++ b/packages/spec/src/shared/retry-policy.zod.ts @@ -0,0 +1,110 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * @module shared/retry-policy + * + * The **single declaration** of the exponential-backoff retry policy (#4661, + * the #4535 C8 dual-source cluster). + * + * 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`) + * — under the *same exported name*, so which `RetryPolicy` a consumer got + * depended only on whether they imported `@objectstack/spec/automation` or + * `@objectstack/spec/system` (the #4411 trap). They were not two concepts: + * both drive `delay = base * multiplier^(retry-1)`, and both executors + * implemented that identical formula. What differed was the *spelling* of the + * base delay (`retryDelayMs` vs `backoffMs`), two keys only one side had + * (`maxRetryDelayMs` / `jitter`), and the defaults. + * + * ## Why this file, and why it is not in `shared/index.ts` + * + * The published JSON-Schema def key is `/`, derived from + * which entry barrel re-exports the const (`scripts/build-schemas.ts` iterates + * the namespace objects). Keeping ONE declaration re-exported from both + * `./automation` and `./system` therefore preserves *both* def keys + * (`automation/RetryPolicy` and `system/RetryPolicy`) with an identical key set + * — which is exactly what makes this convergence cost a single authorable key + * instead of eight. + * + * It is deliberately NOT added to `shared/index.ts`: exporting it from a third + * entry would publish a third `shared/RetryPolicy` def and add five more rows to + * `authorable-surface.json` for a def no author ever writes directly (#4535 §1 + * already flags that file for over-collecting). Same reason `retired-key.ts`, + * `strict-object.ts` and `connector-auth.zod.ts` sit here without a barrel line. + * + * The home is `shared/` rather than either domain because the two owning + * schemas must not depend on each other: `automation/control-flow.zod.ts` + * imports the whole `flow.zod` node/edge graph, and pulling that into + * `system/job.zod.ts` to reach a five-field policy would be a real edge in the + * package graph for no runtime need. This module depends on nothing but `zod` + * and `lazySchema`. + */ + +import { z } from 'zod'; +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. + * + * Delay before retry *n* is `min(backoffMs * backoffMultiplier^(n-1), + * maxRetryDelayMs)`, optionally jittered. + * + * ## Defaults are opt-in, not opt-out (17.0.0, #4661) + * + * `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 + * `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 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 + * that is hardest to catch in tests and most expensive in production, and + * metadata written by an LLM is exactly where an unstated key hides. The same + * reading is already recorded for flow-level retry in the protocol-17 migration + * 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.', + ), +})); + +/** + * What an author writes — every key optional, defaults unapplied. + * + * Note for pre-17 `@objectstack/spec/system` consumers: this used to be + * `z.infer` (the post-parse shape, every key present) on that entry only. It is + * now the input shape on both, matching the house `X` / `XParsed` convention + * used by the sibling control-flow configs. + */ +export type RetryPolicy = z.input; + +/** The post-parse shape — every key present, defaults applied. */ +export type RetryPolicyParsed = z.infer; diff --git a/packages/spec/src/system/job.test.ts b/packages/spec/src/system/job.test.ts index 215a1fbd91..3c321c36b6 100644 --- a/packages/spec/src/system/job.test.ts +++ b/packages/spec/src/system/job.test.ts @@ -184,12 +184,45 @@ describe('RetryPolicySchema', () => { expect(() => RetryPolicySchema.parse(policy)).not.toThrow(); }); - it('should apply default values', () => { + // #4661: retry is opt-in since 17.0.0. `maxRetries` defaulted to 3 and + // `backoffMultiplier` to 2 while this shape was job-only; the converged + // declaration takes the automation side's 0 / 1, because a retry replays the + // handler's writes and callouts and that has to be asked for. Existing job + // documents keep the old numbers — `retry-policy-converged` writes them in — + // so what changed is only what a NEWLY authored omission means. + it('applies opt-in defaults: a declared but empty policy does not retry', () => { const policy = RetryPolicySchema.parse({}); - expect(policy.maxRetries).toBe(3); + expect(policy.maxRetries).toBe(0); expect(policy.backoffMs).toBe(1000); - expect(policy.backoffMultiplier).toBe(2); + expect(policy.backoffMultiplier).toBe(1); + expect(policy.maxRetryDelayMs).toBe(30000); + expect(policy.jitter).toBe(false); + }); + + // The two keys the convergence brought over from the automation side. They + // are declared here only because `runWithPolicy` actually honours them + // (ADR-0049: declared IS enforced). + it('accepts the maxRetryDelayMs ceiling and jitter', () => { + const policy = RetryPolicySchema.parse({ maxRetries: 5, maxRetryDelayMs: 60000, jitter: true }); + + expect(policy.maxRetryDelayMs).toBe(60000); + expect(policy.jitter).toBe(true); + }); + + it('rejects the retired `retryDelayMs` spelling with the rename prescription', () => { + const parse = () => RetryPolicySchema.parse({ retryDelayMs: 500 }); + + expect(parse).toThrow(/backoffMs/); + expect(parse).toThrow(/retryDelayMs/); + }); + + // Bounds the job side did not have before the merge. Both fail loudly rather + // than being silently reinterpreted — see the + // `job-retry-policy-constraints-tightened` semantic migration note. + it('enforces the converged bounds (maxRetries <= 10, backoffMultiplier >= 1)', () => { + expect(() => RetryPolicySchema.parse({ maxRetries: 20 })).toThrow(); + expect(() => RetryPolicySchema.parse({ backoffMultiplier: 0.5 })).toThrow(); }); it('should accept zero retries', () => { diff --git a/packages/spec/src/system/job.zod.ts b/packages/spec/src/system/job.zod.ts index 02dc744828..f1ebf20fb0 100644 --- a/packages/spec/src/system/job.zod.ts +++ b/packages/spec/src/system/job.zod.ts @@ -56,16 +56,31 @@ export type OnceSchedule = z.infer; // name: `Schedule`. /** - * Retry Policy Schema - * Configuration for job retry behavior with exponential backoff + * Retry Policy Schema — job retry behaviour with exponential backoff. + * + * The declaration moved to `shared/retry-policy.zod.ts` in 17.0.0 (#4661). + * `@objectstack/spec/automation` exported an identically-named, differently + * shaped `RetryPolicy` for the `try_catch` node's `retry` region, so which type + * a consumer got depended only on the import path (the #4411 trap) — and they + * were never two concepts: both compute + * `delay = base * multiplier^(retry-1)`. Re-exported so `./system` keeps + * publishing the name (and its `system/RetryPolicy` def, keyed by entry + * namespace). + * + * Three things change for job authors, all covered by the + * `retry-policy-converged` conversion: + * + * - `maxRetryDelayMs` and `jitter` are now available here (they were + * automation-only). Both are honoured by `runWithPolicy`. + * - `maxRetries` now defaults to **0**, not 3, and `backoffMultiplier` to 1, + * not 2 — the conversion writes the old values into existing documents, so + * no deployed job changes behaviour; only a NEWLY authored omission means + * "no retry". + * - `maxRetries` is capped at 10 and `backoffMultiplier` floored at 1 (the + * automation constraints). Both reject loudly at parse time. */ -export const RetryPolicySchema = lazySchema(() => z.object({ - maxRetries: z.number().int().min(0).default(3).describe('Maximum number of retry attempts'), - backoffMs: z.number().int().positive().default(1000).describe('Initial backoff delay in milliseconds'), - backoffMultiplier: z.number().positive().default(2).describe('Multiplier for exponential backoff'), -})); - -export type RetryPolicy = z.infer; +export { RetryPolicySchema, type RetryPolicy, type RetryPolicyParsed } from '../shared/retry-policy.zod'; +import { RetryPolicySchema } from '../shared/retry-policy.zod'; /** * Job Schema @@ -99,7 +114,7 @@ export const JobSchema = lazySchema(() => strictObject({ description: z.string().optional().describe('Job description / purpose'), schedule: ScheduleSchema.describe('Job schedule configuration'), handler: z.string().describe('Handler function name (must match a key in `defineStack({ functions })`)'), - retryPolicy: RetryPolicySchema.optional().describe('Retry policy: failed runs (including timeouts) are retried with exponential backoff (delay = backoffMs * backoffMultiplier^(retry-1)) up to maxRetries retries after the initial attempt (#3494). Omit for the legacy single-attempt behavior.'), + retryPolicy: RetryPolicySchema.optional().describe('Retry policy: failed runs (including timeouts) are retried with exponential backoff (delay = min(backoffMs * backoffMultiplier^(retry-1), maxRetryDelayMs), optionally jittered) up to maxRetries retries after the initial attempt (#3494). Omit the block for a single attempt; declaring it without `maxRetries` also means no retry since 17.0.0 (#4661) — state a count to opt in.'), timeout: z.number().int().positive().optional().describe('Per-attempt time limit in milliseconds; an over-limit run is recorded with execution status "timeout" (#3494). The in-flight handler is abandoned, not forcibly cancelled. Omit for no time limit.'), enabled: z.boolean().default(true).describe('Whether the job is enabled'),