diff --git a/.changeset/23543-f2-job-retry-policy-without-attempts-refused.md b/.changeset/23543-f2-job-retry-policy-without-attempts-refused.md new file mode 100644 index 00000000000..2d55d825d89 --- /dev/null +++ b/.changeset/23543-f2-job-retry-policy-without-attempts-refused.md @@ -0,0 +1,29 @@ +--- +'@objectstack/spec': minor +'@objectstack/service-job': patch +--- + +feat(spec)!: a job `retryPolicy` block whose `maxRetries` is omitted or 0 is refused, and `retry-policy-converged` leaves the load path + +Clause-②: yes (narrowing) + +**BREAKING (authoring):** `maxRetries` has defaulted to 0 since 17, so a `job.retryPolicy` block that omits it, or writes 0, buys no retry: the job runs once. Through 17 the loader hid that by writing the pre-17 `maxRetries: 3` and `backoffMultiplier: 2` into such a block with a notice. At 18 the ADR-0087 D2 conversion `retry-policy-converged` is retired from the load path (`retiredAfter: '17.7.0'`), and the job parse refuses the block at `retryPolicy.maxRetries`, at every door that parses an authored job (`defineStack`, `os validate`, `os build`, and `os lint` when the config calls it): "A retry block with no attempts is no retry block: state `maxRetries` ≥ 1, for example 3, or delete the block." The rule is the job's, the same one `flow.errorHandling` applies to `strategy: 'retry'`; the shared `RetryPolicySchema` is unchanged, so a `try_catch` node's `retry` region is judged as before. + +Migration: + +- `retryPolicy: {}` or `retryPolicy: { backoffMs: 5000 }` → `retryPolicy: { maxRetries: 3, backoffMs: 5000 }` (3 is the count such a block ran with before 17), or delete the block if the job should run once. +- `retryPolicy: { maxRetries: 0, … }` → delete the block: a job with no `retryPolicy` is a single attempt. + +What an author who still writes it sees: `defineStack` throws `STACK_SCHEMA_INVALID` (422) with one issue at `jobs.N.retryPolicy.maxRetries` carrying the prescription above, and `os validate` prints the same issue. Nothing is rewritten silently any more. + +Not refused: a block that states `maxRetries` and omits `backoffMultiplier`. It keeps the declared default of 1, a flat delay, where the loader used to write 2. State `backoffMultiplier: 2` to keep the old exponential curve. + +Run `os migrate meta --from 16` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. For such a block the edit writes both pre-17 numbers (`maxRetries: 3`, `backoffMultiplier: 2`). The D3 entry `job-retry-policy-without-attempts-refused` states the decision left to you: keep the written count, or delete the block. + +The flow half of the conversion (`retryDelayMs` → `backoffMs` on `flow.errorHandling` and a `try_catch` node's `retry`) needed nothing more: both surfaces already refuse `retryDelayMs` with the rename to `backoffMs`. + +Stored metadata needs no edit. Stored-row rehydration, the automation engine's flow rehydration, the metadata save door's flow fallback and the artifact-ingestion door inside its window still replay the conversion, so a job row stored without a count keeps loading with the three retries it ran with. + +`@objectstack/service-job`: a comment-only change in `runWithPolicy`, whose `maxRetries` default is now reached only by a caller that builds the options by hand. + + diff --git a/content/docs/automation/jobs.mdx b/content/docs/automation/jobs.mdx index e6e23b61b4b..9b4e81df2ff 100644 --- a/content/docs/automation/jobs.mdx +++ b/content/docs/automation/jobs.mdx @@ -385,13 +385,15 @@ Delay before retry *n* is `min(backoffMs * backoffMultiplier^(n-1), maxRetryDela optionally jittered. `maxRetries` counts retries **after** the initial attempt and is capped at 10. -Two defaults worth knowing before you rely on the block: +Two rules worth knowing before you rely on the block: -- **`maxRetries` defaults to `0`** — declaring `retryPolicy` without stating a - count still means *no retry*. State a count to opt in. +- **A block must state `maxRetries` ≥ 1.** A `retryPolicy` that omits it or + writes `0` would buy no retry, so it is refused at `objectstack validate` with + "a retry block with no attempts is no retry block". State a count, or delete + the block: a job with no `retryPolicy` runs once. - **`backoffMultiplier` defaults to `1`** — a flat delay, not exponential. -Those defaults are why the block is closed: a key it does not declare — `maxRetry` +The count rule and that default are why the block is closed: a key it does not declare — `maxRetry` for `maxRetries`, a `maxDelayMs` borrowed from another retry vocabulary — is refused at `objectstack validate` with the declared key it was near, rather than dropped and replaced by a default that never retries. diff --git a/content/docs/references/system/job.mdx b/content/docs/references/system/job.mdx index b14ac2f4a08..884b58f83d9 100644 --- a/content/docs/references/system/job.mdx +++ b/content/docs/references/system/job.mdx @@ -61,7 +61,7 @@ const result = CronScheduleSchema.parse(data); | **body** | `{ language: 'js'; source: string; capabilities?: Enum<'api.read' \| 'api.write' \| 'api.transaction' \| 'crypto.uuid' \| 'log'>[]; timeoutMs?: integer; … }` | optional | Job body — a sandboxed JS (L2) body, the same shape hooks and actions use; an expression (L1) body is refused, because a job runs for its effects and an expression has none. Preferred over `handler`: when both are present `body` wins. It runs in the QuickJS sandbox with no module scope (no imports, no helpers or constants from the surrounding file): it reaches data only through `ctx.api` under its declared `capabilities` (`api.read` / `api.write` / `api.transaction`) and logs through `ctx.log` (`log`); the in-process handler context (`ql`, `logger`, `bundle`) does not exist there. Its time limit is the job's `timeoutMs` (see there): long-running work declares a `timeoutMs` that covers it, or splits into bounded runs that each finish within it. Every door that brings an artifact in schedules a job's `body` — the boot, and `os package install` on install and on every restart — while a `handler` is code that travels only in the artifact's runtime module and runs only on a boot that loads it (a config, or `os start --artifact`); `os package install` therefore refuses an enabled job with no `body`. A `pull` is data too, so an enabled `pull` job is judged by its `pull` instead: it installs when the `pull` binds (it names a mapping the package declares, with a `connectorSource`) and is refused when it does not, as is a job whose `body` the declaration refuses. Refused beside `pull`. | | **pull** | `{ mapping: string }` | optional | Pull run form: on each run the platform pulls the named mapping's `connectorSource` (one action call, one response) and writes the rows through the import runner — no code. A refused pull records the run `failed` (retried per `retryPolicy`); a pull whose rows the import runner refused records it `degraded`. Data like `body`, so every door schedules it. Refused beside `body` or `handler`. | | **organization** | `string` | optional | Organization id (sys_organization.id) this job runs as — its `body`'s `ctx.api`, the execution context its `handler` is handed, and its `pull`'s reads and writes alike, as a system run carrying that organization. A scheduled run has no session to inherit one from. Judged at bind by the posture rule scheduled flows use: required under the isolated tenancy posture (a job that declares none is not scheduled); optional under group (undeclared, the run carries no organization and a tenant-scoped write it makes is refused); not required under single (the install's one organization is resolved beneath each write). Where declared, it is the organization the run acts as on every posture. | -| **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. Omit the block for a single attempt; declaring it without `maxRetries` also means no retry since 17.0.0 — state a count to opt in. | +| **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. Omit the block for a single attempt; a block must state `maxRetries` >= 1 — one that omits it or writes 0 is refused, since it would buy no retry. | | **timeoutMs** | `integer` | optional | Per-attempt time limit in milliseconds; an over-limit run is recorded with execution status "timeout". A `handler` run is abandoned, not forcibly cancelled. For a job with a `body` this is the ONE time limit: one attempt is one sandbox invocation, the runtime bounds that invocation by this value, and the body shape's own `timeoutMs` (capped at 30000 for hooks and actions) is refused on a job — so this key, which has no such cap, is where long-running work states how long it needs. Omit for no per-attempt limit; a `body` run is then still bounded by the sandbox's own default invocation limits. | | **timeout** | `never` | optional | [REMOVED] `job.timeout` was removed in @objectstack/spec 17 — its unit (milliseconds) lived only in the description while the sibling `retryPolicy.backoffMs` spells its own, so the same number read as two conventions on one surface. Rename the key to `timeoutMs`; the value (milliseconds) is unchanged. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **enabled** | `boolean` | optional (default: `true`) | Whether the job is enabled | diff --git a/packages/cli/test/migrate-meta-composed-load-conversions.test.ts b/packages/cli/test/migrate-meta-composed-load-conversions.test.ts index 2ddafb292a5..459761e2707 100644 --- a/packages/cli/test/migrate-meta-composed-load-conversions.test.ts +++ b/packages/cli/test/migrate-meta-composed-load-conversions.test.ts @@ -2,12 +2,17 @@ /** * `os migrate meta` on a COMPOSED project — - * `composeStacks([defineStack(…), …], { manifest: 'preserve' })` — whose - * package body carries a spelling the load still converts (a flow-level retry - * policy's `retryDelayMs`, renamed `backoffMs` by `retry-policy-converged`) - * (#22256, the composed half). It was measured on `datasources[].driver: - * 'mongo'`, until that spelling was retired as a value at 18 and its - * conversion left the load path (#23543 F1). + * `composeStacks([defineStack(…), …], { manifest: 'preserve' })` — and the + * authored-source load that serves it (#22256, the composed half). + * + * It was measured on a package body carrying a spelling the load still + * converted: `datasources[].driver: 'mongo'` until #23543 F1, then a flow-level + * retry policy's `retryDelayMs` until F2 retired `retry-policy-converged` from + * the load path too. After F2 no load-path conversion is a step the chain + * replays from its floor (`--from 16` replays steps 17 and 18, and every entry + * of either is retired), so the defect's own reading — the chain missing what + * the load converted — has no subject on this tree. What the fix changed is + * read directly instead (see "What is pinned"). * * ## The defect * @@ -24,19 +29,25 @@ * * ## What is pinned * - * 1. An accepted input: the conversion is listed under the body's path, - * `--write` writes it into the input's own file and nothing else, the - * re-run applies nothing, and a strict load converts nothing any more. - * 2. Where the authored body comes from: the authored-source load's body - * carries the authored spelling, and every other command's load still - * carries the converted one. - * 3. A refused input (re-produced by the shim) carrying the same spelling - * beside a refused one: each conversion is listed once and both are - * written. - * 4. An input defined twice starts from the innermost argument, once. - * 5. The control: an already-canonical composed project writes nothing, and + * 1. Where the authored body comes from: the authored-source load assembles + * the package body from the input's ARGUMENT, and every other command's + * load from the stack `defineStack` built. Every accepted input has a + * difference between the two whether or not anything converts: the strict + * parse fills the declared defaults into what it returns, and the argument + * carries only what the author wrote. + * 2. An accepted input defined twice starts from the innermost argument. + * 3. A refused input (re-produced by the shim) carrying two spellings the + * chain converts (one step-17, one step-18): each conversion is listed + * once, both are written into the input's own file and nothing else, and + * a refused input defined twice is listed once. + * 4. The control: an already-canonical composed project writes nothing, and * its `--json` summary equals the one the built composition gives. * + * Retired at F2: "an accepted input: lists a conversion the load already + * applied; --write writes it into the input's file, and the load stops + * converting". Its first half has no subject (above); its write / re-run / + * clean-reload half is pinned by case 3 at the same layer. + * * The strict reload is read through `stackConversions`, the producers' own * record of what they converted, and not through the stderr notice: that * notice is printed once per process. @@ -155,7 +166,6 @@ async function quietLoad(dir: string, authoredSource = false) { const json = (run: Run): any => JSON.parse(run.stdout); const sites = (list: Array<{ conversionId: string; path: string; from?: string; to?: string }>) => list.map(({ conversionId, path, from, to }) => ({ conversionId, path, from, to })); -const conversionIds = (list: readonly { conversionId: string }[]) => list.map((c) => c.conversionId); const bodyErrorHandling = (config: any): Record => config.packages[0].manifest.flows[0].errorHandling; beforeAll(() => { @@ -173,11 +183,12 @@ afterAll(() => { }); /** - * The service package: a stack the current schema accepts, whose one - * non-canonical spelling is a key the load still converts (`retiredFromLoadPath` - * unset): the flow-level retry policy's `retryDelayMs`, which - * `retry-policy-converged` renames `backoffMs` before the parse, so the call is - * accepted. + * The service package, whose one non-canonical spelling is the flow-level retry + * policy's `retryDelayMs`. The current schema REFUSES it at the `retryDelayMs` + * tombstone (`retry-policy-converged` is retired from the load path, #23543 + * F2), and the chain's step 17 renames it `backoffMs`. + * {@link CANONICAL_SERVICE} is the same package as the author would write it + * today, which the schema accepts. */ const SERVICE_STACK = `import { defineStack } from '@objectstack/spec'; @@ -235,6 +246,11 @@ const RETRY_BODY_SITE = { }; const AUTHORED_DELAY = 'retryDelayMs: 2000'; const CANONICAL_DELAY = 'backoffMs: 2000'; +const CANONICAL_SERVICE = edit(SERVICE_STACK, AUTHORED_DELAY, CANONICAL_DELAY); +/** The flow's policy exactly as the author wrote it … */ +const AUTHORED_POLICY = { strategy: 'retry', maxRetries: 3, backoffMs: 2000 }; +/** … and as the strict parse returns it: the declared defaults filled in. */ +const BUILT_POLICY = { ...AUTHORED_POLICY, backoffMultiplier: 1, maxRetryDelayMs: 30000, jitter: false }; const TIME_BODY_SITE = { conversionId: 'time-default-utc-suffix-dropped', path: 'packages[0].manifest.objects[0].fields.starts_at.defaultValue', @@ -242,75 +258,41 @@ const TIME_BODY_SITE = { to: '"10:00"', }; -describe('an accepted input: the chain reads the package body as the author wrote it', () => { - it('lists a conversion the load already applied; --write writes it into the input’s file, and the load stops converting', async () => { - const dir = writeProject(composed(SERVICE_STACK)); - const before = snapshot(dir); - - // The control for the last reading below: today the strict load converts it. - const strictBefore = await quietLoad(dir); - expect(strictBefore.stackProvenance).toBe(true); - expect(conversionIds(strictBefore.stackConversions)).toEqual([RETRY_CONVERSION]); - - const dry = json(await runMeta(dir, ['--from', '16', '--json'])); - expect(sites(dry.applied)).toEqual([RETRY_BODY_SITE]); - expect(dry.schemaValid).toBe(true); - - const run = await runMeta(dir, ['--from', '16', '--write', '--json']); - expect(run.exitCode, run.stdout + run.stderr).toBeUndefined(); - const out = json(run); - expect(sites(out.applied)).toEqual([RETRY_BODY_SITE]); - expect(out.write.status).toBe('written'); - expect(out.write.files).toEqual([{ file: 'src/service.stack.ts', sites: 1 }]); - expect(out.write.manual).toEqual([]); - expect(out.write.unexplained).toEqual([]); - - // The site and nothing else: every other file keeps its bytes. - expect(snapshot(dir)).toEqual({ - ...before, - 'src/service.stack.ts': edit(before['src/service.stack.ts']!, AUTHORED_DELAY, CANONICAL_DELAY), - }); - - const again = json(await runMeta(dir, ['--from', '16', '--json'])); - expect(again.applied).toEqual([]); - expect(again.schemaValid).toBe(true); - - // The load every other command uses — strict, no shim — converts nothing now. - const strictAfter = await quietLoad(dir); - expect(strictAfter.stackProvenance).toBe(true); - expect(strictAfter.stackConversions).toEqual([]); - }, RUN_TIMEOUT); - - it('the authored-source load’s package body carries the authored spelling; every other load’s, the converted one', async () => { - const dir = writeProject(composed(SERVICE_STACK)); +describe('an accepted input: the authored-source load assembles the body the author wrote', () => { + it('the authored-source load’s package body is the input’s argument; every other load’s, the built stack', async () => { + const dir = writeProject(composed(CANONICAL_SERVICE)); const authored = await quietLoad(dir, true); - expect(bodyErrorHandling(authored.config)).toMatchObject({ retryDelayMs: 2000 }); - expect(bodyErrorHandling(authored.config)).not.toHaveProperty('backoffMs'); + expect(bodyErrorHandling(authored.config)).toEqual(AUTHORED_POLICY); expect(authored.stackProvenance).toBe(true); + expect(authored.stackConversions).toEqual([]); const strict = await quietLoad(dir); - expect(bodyErrorHandling(strict.config)).toMatchObject({ backoffMs: 2000 }); - expect(bodyErrorHandling(strict.config)).not.toHaveProperty('retryDelayMs'); + expect(bodyErrorHandling(strict.config)).toEqual(BUILT_POLICY); }, RUN_TIMEOUT); - it('an input defined twice starts from the innermost argument, and is listed once', async () => { + it('an input defined twice starts from the innermost argument', async () => { const dir = writeProject(composed(edit( - edit(SERVICE_STACK, 'export const ServiceStack = defineStack({', 'export const ServiceStack = defineStack(defineStack({'), + edit(CANONICAL_SERVICE, 'export const ServiceStack = defineStack({', 'export const ServiceStack = defineStack(defineStack({'), '\n});\n', '\n}));\n', ))); + // The outer call's argument is the inner call's BUILT stack, defaults and all. + const authored = await quietLoad(dir, true); + expect(bodyErrorHandling(authored.config)).toEqual(AUTHORED_POLICY); + const out = json(await runMeta(dir, ['--from', '16', '--json'])); - expect(sites(out.applied)).toEqual([RETRY_BODY_SITE]); + expect(out.applied).toEqual([]); expect(out.schemaValid).toBe(true); }, RUN_TIMEOUT); }); -describe('a refused input the shim produces again, carrying a load-path spelling too', () => { - // The `time` default with the UTC suffix is refused by the current schema, - // so the shim hands the call's argument through and produces it again in - // `strict: false` mode for the composer; the flow's retry delay key is the - // input's one load-path conversion. +describe('a refused input the shim produces again, carrying two spellings the chain converts', () => { + // The `time` default with the UTC suffix (step 18) and the flow's + // `retryDelayMs` (step 17) are both refused by the current schema, so the + // shim hands the call's argument through and produces it again in + // `strict: false` mode for the composer. It carried a load-path spelling + // until #23543 F2; with none left that the chain replays, both are refusals. const MIXED = edit( SERVICE_STACK, " name: { type: 'text', label: 'Name' },\n", @@ -346,10 +328,21 @@ describe('a refused input the shim produces again, carrying a load-path spelling expect(strict.stackProvenance).toBe(true); expect(strict.stackConversions).toEqual([]); }, RUN_TIMEOUT); + + it('an input defined twice starts from the innermost argument, and is listed once', async () => { + const dir = writeProject(composed(edit( + edit(SERVICE_STACK, 'export const ServiceStack = defineStack({', 'export const ServiceStack = defineStack(defineStack({'), + '\n});\n', + '\n}));\n', + ))); + const out = json(await runMeta(dir, ['--from', '16', '--json'])); + expect(sites(out.applied)).toEqual([RETRY_BODY_SITE]); + expect(out.schemaValid).toBe(true); + }, RUN_TIMEOUT); }); describe('control: an already-canonical composed project', () => { - const CANONICAL = edit(SERVICE_STACK, AUTHORED_DELAY, CANONICAL_DELAY); + const CANONICAL = CANONICAL_SERVICE; it('writes nothing, and its --json summary is the one the built composition gives', async () => { const dir = writeProject(composed(CANONICAL)); diff --git a/packages/cli/test/migrate-meta-composed.test.ts b/packages/cli/test/migrate-meta-composed.test.ts index fda2ddea394..7688ee373fb 100644 --- a/packages/cli/test/migrate-meta-composed.test.ts +++ b/packages/cli/test/migrate-meta-composed.test.ts @@ -269,10 +269,13 @@ export default defineStack({ }); `; // The same stack with a flow whose retry policy spells its delay the old - // way: a conversion the load still applies. The raw hand-back keeps it - // visible to the chain. (It was a datasource `driver: 'mongo'` until that - // spelling was retired as a value at 18, #23543 F1.) - const SINGLE_WITH_LOAD_PATH_CONVERSION = edit( + // way: a second refused spelling, which the chain's step 17 renames. The raw + // hand-back keeps it visible to the chain. It was a conversion the load + // still applied — a datasource `driver: 'mongo'` until #23543 F1, then this + // `retryDelayMs` until F2 retired `retry-policy-converged` from the load + // path — and no load-path conversion is a step the chain replays from its + // floor any more, so the row reads the two-spelling refused arm. + const SINGLE_WITH_SECOND_REFUSED_SPELLING = edit( SINGLE, ' ],\n});\n', ' ],\n' @@ -297,7 +300,7 @@ export default defineStack({ it.each([ { name: 'a refused stack', source: SINGLE, applied: [TIME_SITE], writtenSites: 1 }, - { name: 'a refused stack carrying a load-path conversion', source: SINGLE_WITH_LOAD_PATH_CONVERSION, applied: [RETRY_SITE, TIME_SITE], writtenSites: 2 }, + { name: 'a refused stack carrying a second refused spelling', source: SINGLE_WITH_SECOND_REFUSED_SPELLING, applied: [RETRY_SITE, TIME_SITE], writtenSites: 2 }, ])('$name: `applied` and `write.files` as measured before the change', async ({ source, applied, writtenSites }) => { const dir = writeProject({ 'objectstack.config.ts': source }); const dry = json(await runMeta(dir, ['--from', '16', '--json'])); diff --git a/packages/cli/test/migrate-meta-load-conversions.test.ts b/packages/cli/test/migrate-meta-load-conversions.test.ts index 33dbdc22a34..b9d2fd9d790 100644 --- a/packages/cli/test/migrate-meta-load-conversions.test.ts +++ b/packages/cli/test/migrate-meta-load-conversions.test.ts @@ -2,11 +2,15 @@ /** * `os migrate meta` on a stack whose `defineStack` call the current schema - * ACCEPTS, carrying a spelling the load still converts (a flow-level retry - * policy's `retryDelayMs`, renamed `backoffMs` by `retry-policy-converged`). - * It was measured on `driver: 'mongo'`, until that spelling was retired as a - * value at 18 and its conversion left the load path (#23543 F1): an authored - * `mongo` is now refused, so it no longer exercises the accepted arm. + * ACCEPTS, and the authored-source load that serves it. + * + * It was measured on a spelling the load still converted: `driver: 'mongo'` + * until #23543 F1, then a flow-level retry policy's `retryDelayMs` until F2 + * retired `retry-policy-converged` from the load path too. After F2 no load-path + * conversion is a step the chain replays from its floor (`--from 16` replays + * steps 17 and 18, and every entry of either is retired), so the defect's own + * reading — the chain missing what the load converted — has no subject on this + * tree. What the fix changed is read directly instead (see "What is pinned"). * * ## The defect * @@ -20,16 +24,23 @@ * * ## What is pinned * - * 1. The fix: the conversion is listed, `--write` writes it, the re-run - * applies nothing, and a strict load converts nothing any more. - * 2. Where the argument is read: off the default export, before the - * named-export merge (which spreads it into a new object), and only for - * the authored-source load. - * 3. A stack defined twice starts from the innermost argument, once. - * 4. The control: an already-canonical source writes nothing, and its + * 1. The fix, read where it lives: the authored-source load hands back the + * call's ARGUMENT, not the stack `defineStack` built. Every accepted call + * has a difference between the two whether or not anything converts: the + * strict parse fills the declared defaults into what it returns, and the + * argument carries only what the author wrote. Read off the default + * export, before the named-export merge (which spreads it into a new + * object), and only for the authored-source load. + * 2. A stack defined twice starts from the innermost argument. + * 3. The control: an already-canonical source writes nothing, and its * `--json` summary equals the one the built stack gives. - * 5. A stack the schema refuses for one spelling while carrying a load-path - * spelling too: each conversion is applied once, and both are written. + * 4. A stack the schema refuses for two spellings the chain converts (one + * step-17, one step-18): each conversion is applied once, both are + * written, and a stack defined twice is listed once. + * + * Retired at F2: "lists a conversion the load already applied; --write writes + * it, and the load stops converting". Its first half has no subject (above); + * its write / re-run / clean-reload half is pinned by case 4 at the same layer. * * The strict reload is read through `stackConversions`, the producer's own * record of what it converted, and not through the stderr notice: that notice @@ -150,7 +161,6 @@ async function quietLoad(dir: string, authoredSource = false) { const json = (run: Run): any => JSON.parse(run.stdout); const sites = (list: Array<{ conversionId: string; path: string; from?: string; to?: string }>) => list.map(({ conversionId, path, from, to }) => ({ conversionId, path, from, to })); -const conversionIds = (list: readonly { conversionId: string }[]) => list.map((c) => c.conversionId); beforeAll(() => { root = mkdtempSync(join(tmpdir(), 'os-migrate-meta-load-conversions-')); @@ -167,10 +177,12 @@ afterAll(() => { }); /** - * A stack the current schema accepts. Its one non-canonical spelling is a key - * the load still converts (`retiredFromLoadPath` unset): the flow-level retry - * policy's `retryDelayMs`, which `retry-policy-converged` renames `backoffMs` - * before the parse, so the call is accepted. + * A stack whose one non-canonical spelling is the flow-level retry policy's + * `retryDelayMs`. The current schema REFUSES it at the `retryDelayMs` + * tombstone (`retry-policy-converged` is retired from the load path, #23543 + * F2), and the chain's step 17 renames it `backoffMs`. {@link CANONICAL_SOURCE} + * is the same stack as the author would write it today, which the schema + * accepts. */ const RETRY_SOURCE = `import { defineStack } from '@objectstack/spec'; @@ -202,6 +214,11 @@ const RETRY_CONVERSION = 'retry-policy-converged'; const RETRY_SITE = { conversionId: RETRY_CONVERSION, path: 'flows[0].errorHandling.backoffMs', from: 'retryDelayMs', to: 'backoffMs' }; const AUTHORED_DELAY = 'retryDelayMs: 2000'; const CANONICAL_DELAY = 'backoffMs: 2000'; +const CANONICAL_SOURCE = edit(RETRY_SOURCE, AUTHORED_DELAY, CANONICAL_DELAY); +/** The flow's policy exactly as the author wrote it … */ +const AUTHORED_POLICY = { strategy: 'retry', maxRetries: 3, backoffMs: 2000 }; +/** … and as the strict parse returns it: the declared defaults filled in. */ +const BUILT_POLICY = { ...AUTHORED_POLICY, backoffMultiplier: 1, maxRetryDelayMs: 30000, jitter: false }; const TIME_SITE = { conversionId: 'time-default-utc-suffix-dropped', path: 'objects[0].fields.starts_at.defaultValue', @@ -209,77 +226,45 @@ const TIME_SITE = { to: '"10:00"', }; -describe('an accepted stack: the chain starts from what the author wrote', () => { - it('lists a conversion the load already applied; --write writes it, and the load stops converting', async () => { - const dir = writeProject({ 'objectstack.config.ts': RETRY_SOURCE }); - - // The control for the last reading below: today the strict load converts it. - const before = await quietLoad(dir); - expect(before.stackProvenance).toBe(true); - expect(conversionIds(before.stackConversions)).toEqual([RETRY_CONVERSION]); - - const dry = json(await runMeta(dir, ['--from', '16', '--json'])); - expect(sites(dry.applied)).toEqual([RETRY_SITE]); - expect(dry.schemaValid).toBe(true); - - const run = await runMeta(dir, ['--from', '16', '--write', '--json']); - expect(run.exitCode, run.stdout + run.stderr).toBeUndefined(); - const out = json(run); - expect(sites(out.applied)).toEqual([RETRY_SITE]); - expect(out.write.status).toBe('written'); - expect(out.write.files).toEqual([{ file: 'objectstack.config.ts', sites: 1 }]); - expect(out.write.manual).toEqual([]); - expect(out.write.unexplained).toEqual([]); - expect(readFileSync(join(dir, 'objectstack.config.ts'), 'utf8')).toBe( - edit(RETRY_SOURCE, AUTHORED_DELAY, CANONICAL_DELAY), - ); - - const again = json(await runMeta(dir, ['--from', '16', '--json'])); - expect(again.applied).toEqual([]); - expect(again.schemaValid).toBe(true); - - // The load every other command uses — strict, no shim — converts nothing now. - const after = await quietLoad(dir); - expect(after.stackProvenance).toBe(true); - expect(after.stackConversions).toEqual([]); - }, RUN_TIMEOUT); - - it('the authored-source load reads the argument off the default export, before the named exports are merged', async () => { +describe('an accepted stack: the authored-source load hands back what the author wrote', () => { + it('reads the argument off the default export, before the named exports are merged', async () => { const dir = writeProject({ - 'objectstack.config.ts': `${RETRY_SOURCE}\nexport const onEnable = async () => {};\n`, + 'objectstack.config.ts': `${CANONICAL_SOURCE}\nexport const onEnable = async () => {};\n`, }); const authored = await quietLoad(dir, true); - expect(authored.config.flows[0].errorHandling).toMatchObject({ retryDelayMs: 2000 }); - expect(authored.config.flows[0].errorHandling).not.toHaveProperty('backoffMs'); + expect(authored.config.flows[0].errorHandling).toEqual(AUTHORED_POLICY); expect(authored.namedExports).toEqual(['onEnable']); expect(typeof authored.config.onEnable).toBe('function'); // The producer's mark and record are still read off the stack it built. expect(authored.stackProvenance).toBe(true); - expect(conversionIds(authored.stackConversions)).toEqual([RETRY_CONVERSION]); + expect(authored.stackConversions).toEqual([]); // Every other command still reads the stack `defineStack` built. const strict = await quietLoad(dir); - expect(strict.config.flows[0].errorHandling).toMatchObject({ backoffMs: 2000 }); - expect(strict.config.flows[0].errorHandling).not.toHaveProperty('retryDelayMs'); + expect(strict.config.flows[0].errorHandling).toEqual(BUILT_POLICY); }, RUN_TIMEOUT); - it('a stack defined twice starts from the innermost argument, and converts it once', async () => { + it('a stack defined twice starts from the innermost argument', async () => { const dir = writeProject({ 'objectstack.config.ts': edit( - edit(RETRY_SOURCE, 'export default defineStack({', 'export default defineStack(defineStack({'), + edit(CANONICAL_SOURCE, 'export default defineStack({', 'export default defineStack(defineStack({'), '\n});\n', '\n}));\n', ), }); + // The outer call's argument is the inner call's BUILT stack, defaults and all. + const authored = await quietLoad(dir, true); + expect(authored.config.flows[0].errorHandling).toEqual(AUTHORED_POLICY); + const out = json(await runMeta(dir, ['--from', '16', '--json'])); - expect(sites(out.applied)).toEqual([RETRY_SITE]); + expect(out.applied).toEqual([]); expect(out.schemaValid).toBe(true); }, RUN_TIMEOUT); }); describe('control: an already-canonical source', () => { - const CANONICAL = edit(RETRY_SOURCE, AUTHORED_DELAY, CANONICAL_DELAY); + const CANONICAL = CANONICAL_SOURCE; it('writes nothing, and its --json summary is the one the built stack gives', async () => { const dir = writeProject({ 'objectstack.config.ts': CANONICAL }); @@ -304,10 +289,11 @@ describe('control: an already-canonical source', () => { }, RUN_TIMEOUT); }); -describe('a stack with one refused spelling and one load-path spelling', () => { - // The `time` default with the UTC suffix is refused by the current schema, - // so this call hands its argument through; the flow is valid, so its retry - // delay key is the only load-path conversion in it. +describe('a stack with two refused spellings the chain converts', () => { + // The `time` default with the UTC suffix (step 18) and the flow's + // `retryDelayMs` (step 17) are both refused by the current schema, so this + // call hands its argument through. It carried a load-path spelling until + // #23543 F2; with none left that the chain replays, both arms are refusals. const MIXED = edit( RETRY_SOURCE, " name: { type: 'text', label: 'Name' },\n", @@ -337,4 +323,17 @@ describe('a stack with one refused spelling and one load-path spelling', () => { expect(strict.stackProvenance).toBe(true); expect(strict.stackConversions).toEqual([]); }, RUN_TIMEOUT); + + it('a stack defined twice starts from the innermost argument, and is listed once', async () => { + const dir = writeProject({ + 'objectstack.config.ts': edit( + edit(RETRY_SOURCE, 'export default defineStack({', 'export default defineStack(defineStack({'), + '\n});\n', + '\n}));\n', + ), + }); + const out = json(await runMeta(dir, ['--from', '16', '--json'])); + expect(sites(out.applied)).toEqual([RETRY_SITE]); + expect(out.schemaValid).toBe(true); + }, RUN_TIMEOUT); }); diff --git a/packages/cli/test/retry-policy-key-validate-door.test.ts b/packages/cli/test/retry-policy-key-validate-door.test.ts index 4b8880f1a12..491369bb356 100644 --- a/packages/cli/test/retry-policy-key-validate-door.test.ts +++ b/packages/cli/test/retry-policy-key-validate-door.test.ts @@ -92,10 +92,12 @@ const FIXTURES = { /** The measured slip, `maxRetry` for `maxRetries`, under a `try_catch` node's `retry` AND on a job's `retryPolicy`. */ slip: stackOf({ maxRetry: 2 }, { ...DECLARED_POLICY, maxRetry: 3 }), /** - * Every declared key on the job, and on the flow the pre-17 `retryDelayMs` - * alone, which the conversion renames before the judge sees it. + * Every declared key on the job and on the flow. (The flow spelled its base + * delay with the pre-17 `retryDelayMs` until #23543 F2 retired + * `retry-policy-converged` from the load path; an authored `retryDelayMs` now + * meets its tombstone at this door, so the fixture writes `backoffMs`.) */ - declared: stackOf({ maxRetries: 2, retryDelayMs: 500, backoffMultiplier: 2, maxRetryDelayMs: 10000, jitter: true }, DECLARED_POLICY), + declared: stackOf({ maxRetries: 2, backoffMs: 500, backoffMultiplier: 2, maxRetryDelayMs: 10000, jitter: true }, DECLARED_POLICY), } as const; interface Run { diff --git a/packages/services/service-job/src/run-with-policy.ts b/packages/services/service-job/src/run-with-policy.ts index ad203171f28..7616b0a7131 100644 --- a/packages/services/service-job/src/run-with-policy.ts +++ b/packages/services/service-job/src/run-with-policy.ts @@ -22,8 +22,10 @@ export class JobTimeoutError extends Error { * * `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. + * conversion writes them in, and since 18 only the stored-row seams and + * `os migrate meta` replay it. An authored `job.retryPolicy` can no longer + * leave the count at 0: the job parse refuses such a block (#23543 F2), so the + * `maxRetries` default here is reached only by a hand-built caller. */ const RETRY_DEFAULTS = { maxRetries: 0, diff --git a/packages/spec/liveness/flow.json b/packages/spec/liveness/flow.json index e76994f62f6..20114a17729 100644 --- a/packages/spec/liveness/flow.json +++ b/packages/spec/liveness/flow.json @@ -136,7 +136,7 @@ "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." + "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 by the `retry-policy-converged` conversion wherever stored documents are replayed; it left the load path at 18 (#23543 F2). 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/src/automation/flow-builtin-node-config-keys.test.ts b/packages/spec/src/automation/flow-builtin-node-config-keys.test.ts index 348efc7472f..57b18269536 100644 --- a/packages/spec/src/automation/flow-builtin-node-config-keys.test.ts +++ b/packages/spec/src/automation/flow-builtin-node-config-keys.test.ts @@ -395,13 +395,29 @@ describe('try_catch: its retry is closed, so the spec judges its keys like every expect(saved.success ? [] : saved.error.issues.map((i) => i.path.join('.'))).toEqual(['nodes.1.config.retry.maxRetry']); }); - it('CONTROL: a pre-17 retryDelayMs alone is converted first where the door converts', () => { + // `retry-policy-converged` left the load path at 18 (#23543 F2): the doors + // that still convert first are the ones replaying retired entries — the + // automation engine's `registerFlow` and the other rehydration seams — and + // the authoring door now meets the tombstone instead. + it('CONTROL: a pre-17 retryDelayMs alone is converted first where the door replays the conversion', () => { + const legacy = flowWith('try_catch', { try: TRY_REGION, retry: { maxRetries: 1, retryDelayMs: 500 } }, 'retry_legacy'); + const replayed = applyConversionsToFlow(structuredClone(legacy), { includeRetired: true }); + expect(FlowSchema.safeParse(replayed).success).toBe(true); + const stack = { manifest: { id: 'com.example.retry', name: 'retry', version: '1.0.0', type: 'app', namespace: 'rty' }, objects: [{ name: 'rty_task', label: 'Task', fields: { title: { type: 'text', label: 'Title' } } }], - flows: [flowWith('try_catch', { try: TRY_REGION, retry: { maxRetries: 1, retryDelayMs: 500 } }, 'retry_legacy')], + flows: [legacy], }; - expect(() => defineStack(stack as never)).not.toThrow(); + let refusal: { code?: unknown; issues?: Array<{ path: unknown[]; message: string }> } | undefined; + try { + defineStack(stack as never); + } catch (error) { + refusal = error as typeof refusal; + } + expect(refusal?.code).toBe('STACK_SCHEMA_INVALID'); + expect(refusal!.issues!.map((i) => i.path.join('.'))).toEqual(['flows.0.nodes.1.config.retry.retryDelayMs']); + expect(refusal!.issues![0]!.message).toContain('Rename the key to `backoffMs`'); }); }); diff --git a/packages/spec/src/conversions/conversions.test.ts b/packages/spec/src/conversions/conversions.test.ts index b600d8b724b..58d21b5550a 100644 --- a/packages/spec/src/conversions/conversions.test.ts +++ b/packages/spec/src/conversions/conversions.test.ts @@ -14,6 +14,7 @@ import { import { normalizeStackInput } from '../shared/metadata-collection.zod.js'; import { ElementButtonPropsSchema, PageHeaderProps, PageTabsProps } from '../ui/component.zod.js'; import { PageSchema } from '../ui/page.zod.js'; +import { JobSchema, RetryPolicySchema } from '../system/job.zod.js'; import { defineStack } from '../stack.zod.js'; import { applyMetaMigrations } from '../migrations/chain.js'; import { applyConversions, applyConversionsToFlow, collectConversionNotices } from './apply.js'; @@ -402,7 +403,6 @@ describe('conversion layer (ADR-0087 D2)', () => { expect(kept).toEqual([ 'flow-node-http-callout-rename', 'page-component-visibility-to-visibleWhen', - 'retry-policy-converged', 'view-visibleOn-to-visibleWhen', ]); for (const id of [ @@ -421,6 +421,9 @@ describe('conversion layer (ADR-0087 D2)', () => { // keys and each lift deleted the keys its block shadows. 'flow-node-wait-event-config-lift', 'flow-node-connector-config-lift', + // #23543 F2: retired once the job parse refused a `retryPolicy` block + // whose `maxRetries` is omitted or 0. + 'retry-policy-converged', ]) { expect(ALL_CONVERSIONS.find((c) => c.id === id), id).toMatchObject({ retiredFromLoadPath: true, retiredAfter: '17.7.0' }); } @@ -1122,6 +1125,88 @@ describe('conversion layer (ADR-0087 D2)', () => { }); }); + // #23543 F2 — the job half of `retry-policy-converged` was the only reason + // it stayed on the load path. The retirement's claims: an AUTHORED block with + // no attempts meets the prescription instead of a silent `maxRetries: 3`; a + // STORED row still loads with the numbers it ran with (the data-at-rest seams + // replay the entry); the flow half needed nothing beyond the retirement, + // because the `retryDelayMs` tombstone already carries its rename. + describe('retry-policy-converged is retired (#23543 F2)', () => { + const jobOf = (retryPolicy: Record) => ({ + name: 'nightly_sync', + schedule: { type: 'cron', expression: '0 0 * * *' }, + handler: 'syncOrders', + retryPolicy, + }); + const refusalOf = (authored: Record) => { + let refusal: unknown; + try { + defineStack(authored as never); + } catch (error) { + refusal = error; + } + expect(refusal).toMatchObject({ code: 'STACK_SCHEMA_INVALID', status: 422 }); + return (refusal as { issues: { path: PropertyKey[]; message: string }[] }).issues; + }; + + it.each([ + ['omits `maxRetries`', { backoffMs: 5000 }], + ['writes `maxRetries: 0`', { maxRetries: 0, backoffMs: 5000 }], + ])('an authored job block that %s reaches defineStack as written and is refused at the key', (_name, policy) => { + const issues = refusalOf({ jobs: [jobOf(policy)] }); + // The count is the ONE issue: the job is otherwise complete. + expect(issues.map((i) => i.path.join('.'))).toEqual(['jobs.0.retryPolicy.maxRetries']); + expect(issues[0]!.message.startsWith( + 'A retry block with no attempts is no retry block: state `maxRetries` ≥ 1, for example 3, or delete the block.', + )).toBe(true); + }); + + it('is retired from the load path — the authoring funnel writes no count into an authored block', () => { + const before = { jobs: [jobOf({ backoffMs: 5000 })] }; + const { stack, notices } = collectConversionNotices(structuredClone(before)); + expect(stack).toEqual(before); + expect(notices).toHaveLength(0); + }); + + it('a stored job row with no count still loads at the rehydration seam, with the pre-17 numbers', () => { + const raw = jobOf({ backoffMs: 5000 }); + // The row as written is what the parse now refuses … + expect(JobSchema.safeParse(raw).success).toBe(false); + // … and the seam's replay (stored-row rehydration) makes it load, three retries and all. + const converted = applyConversionsToStoredItem('job', structuredClone(raw)); + expect(converted.retryPolicy).toEqual({ backoffMs: 5000, maxRetries: 3, backoffMultiplier: 2 }); + expect(JobSchema.safeParse(converted).success).toBe(true); + }); + + it('the flow half needed nothing more: an authored `retryDelayMs` meets its tombstone naming `backoffMs`', () => { + const issues = refusalOf({ + flows: [{ + name: 'sync_orders', + label: 'Sync', + type: 'autolaunched', + errorHandling: { strategy: 'retry', maxRetries: 3, retryDelayMs: 2000 }, + nodes: [{ id: 'start', type: 'start', label: 'Start' }, { id: 'end', type: 'end', label: 'End' }], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + }], + }); + expect(issues.map((i) => i.path.join('.'))).toEqual(['flows.0.errorHandling.retryDelayMs']); + expect(issues[0]!.message).toContain('Rename the key to `backoffMs`'); + }); + + it('CONTROL — a job with no block, and a block stating `maxRetries: 1`, both parse; an omitted multiplier keeps its default', () => { + const { retryPolicy: _none, ...bare } = jobOf({}); + expect(JobSchema.safeParse(bare).success).toBe(true); + const one = JobSchema.safeParse(jobOf({ maxRetries: 1 })); + expect(one.success).toBe(true); + expect(one.data!.retryPolicy).toMatchObject({ maxRetries: 1, backoffMultiplier: 1 }); + expect(() => defineStack({ jobs: [jobOf({ maxRetries: 1 }), { ...bare, name: 'weekly_purge' }] } as never)).not.toThrow(); + }); + + it('the same block on a `try_catch` node\'s `retry` is not this rule\'s — the shared schema is unchanged', () => { + expect(RetryPolicySchema.safeParse({ backoffMs: 5000 }).success).toBe(true); + }); + }); + // Commit e2798fab7 renamed the canonical id `mongo` → `mongodb`; #23543 F1 // retired `mongo` as a value at 18. Three claims hold together: the stored // value still CONVERGES (the data-at-rest seams replay the retired entry), an diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index 73c229f4d93..bdae19d849f 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -4257,22 +4257,35 @@ const objectIndexTypePartialRemoved: MetadataConversion = { * default flip 3 → 0 never needed a materialization step while the job one * did: nothing was ever deployed under the old reading. * - * `retiredFromLoadPath` is NOT set, and the base-delay rename is not why: - * every surface that carried `retryDelayMs` already refuses it with the rename - * to `backoffMs` (the `RetryPolicySchema` tombstone, which a `try_catch` node's - * `config.retry` is, and `flow.errorHandling`'s own). - * - * **A recorded verdict at 18 (#23487):** it stays, for the job half alone. It - * writes the pre-17 `maxRetries: 3` / `backoffMultiplier: 2` into a source - * `job.retryPolicy` that omits them; retired, the authoring funnel would stop - * doing that, and such a job would silently drop from three retries to none - * under the merged 0 / 1 defaults — a default flip no gate can see. It can - * retire once that default flip has a ruling for authored sources (or the entry - * is recorded as permanent). + * ## Retired from the load path at 18 (#23543, ruling F2 A) + * + * The base-delay rename never needed the load path: every surface that carried + * `retryDelayMs` refuses it with the rename to `backoffMs` (the + * `RetryPolicySchema` tombstone, which a `try_catch` node's `config.retry` is, + * and `flow.errorHandling`'s own). It stayed for the job half alone (the + * verdict #23487 recorded): retired, the authoring funnel would stop writing + * `maxRetries: 3` into a source block that omits it, and such a job would + * silently drop from three retries to none — a default flip no gate can see. + * The ruling made that drop loud instead: the job parse refuses a + * `retryPolicy` whose `maxRetries` is omitted or 0, naming the fix + * (`JOB_RETRY_POLICY_NO_ATTEMPTS`, `system/job.zod.ts`), so no authored job + * changes its retry count without its author being told. An omitted + * `backoffMultiplier` is not refused and now keeps its declared default (1): + * it shapes the delay curve, not whether the job retries. + * + * The authoring funnel (`defineStack`, `os validate` / `os build` / `os lint`) + * no longer replays it. Stored documents are the `includeRetired` seams — + * stored-row rehydration (`applyConversionsToStoredItem`), the automation + * engine's flow rehydration, the metadata save door's flow fallback, the + * artifact-ingestion door inside its window — and `os migrate meta --from 16` + * (a step-17 member, carried by the D3 entry `job-retry-policy-without-attempts-refused`), + * so a job stored or written before 17 keeps the three retries it ran with. */ const retryPolicyConverged: MetadataConversion = { id: 'retry-policy-converged', toMajor: 17, + retiredFromLoadPath: true, + retiredAfter: '17.7.0', surface: 'flow.errorHandling.retryDelayMs / flow.node.config.retry.retryDelayMs / job.retryPolicy.maxRetries / job.retryPolicy.backoffMultiplier', summary: "retry policy unified across job.retryPolicy, try_catch retry and flow.errorHandling: base delay 'retryDelayMs' → 'backoffMs', " + @@ -12178,8 +12191,9 @@ const hookTimeoutToTimeoutMs: MetadataConversion = { * rename `hookTimeoutToTimeoutMs` documents. The sibling `retryPolicy.backoffMs` * spelled its unit; the per-attempt limit did not. Same posture: retired from * the load path, tombstoned at the schema, replayable here. The fixture stays - * clear of `retryPolicy` on purpose — `retry-policy-converged` is still in its - * live window on that block and would fire on it. + * clear of `retryPolicy` on purpose — `retry-policy-converged` replays on that + * block wherever retired entries do (the fixture harness among them) and would + * fire on it. */ const jobTimeoutToTimeoutMs: MetadataConversion = { id: 'job-timeout-to-timeout-ms', diff --git a/packages/spec/src/migrations/entries/semantic/18.job-retry-policy-without-attempts-refused.ts b/packages/spec/src/migrations/entries/semantic/18.job-retry-policy-without-attempts-refused.ts new file mode 100644 index 00000000000..5a3e21eb328 --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/18.job-retry-policy-without-attempts-refused.ts @@ -0,0 +1,41 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +// #23543 F2 — the job default flip's one D3 entry. The D2 half is +// `retry-policy-converged` (a step-17 conversion), which writes the pre-17 +// `maxRetries: 3` / `backoffMultiplier: 2` into every job retry block the chain +// can reach and is now retired from the load path; this entry states the +// refusal an authored source meets at 18, and the choice no conversion can make +// for the author — whether a job that never stated a count should retry at all. +export const entry: SemanticMigration = { + id: 'job-retry-policy-without-attempts-refused', + // No backticks and no pipes in `surface` — build-upgrade-guide.ts renders it + // inside a code span AND a table cell. + surface: 'job.retryPolicy declared with maxRetries omitted or 0 (system/job.zod.ts)', + replacement: + '`retryPolicy: { maxRetries: N, … }` with N ≥ 1 (for example 3, the count such a block ran with ' + + 'before 17), or no `retryPolicy` block at all for a single attempt', + reason: + 'A maintainer ruling retired the job half of `retry-policy-converged` from the load path at 18. ' + + '`maxRetries` has defaulted to 0 since 17, so a `retryPolicy` block that omits it (or writes 0) buys ' + + 'no retry: the job runs once. Through 17 the loader hid that by writing the pre-17 `maxRetries: 3` ' + + 'and `backoffMultiplier: 2` into such a block with a notice. At 18 the job parse refuses the block ' + + 'at `retryPolicy.maxRetries` instead, at `objectstack validate`, `objectstack build` and ' + + '`defineStack`, so under `objectstack lint` too when the config calls it: "a retry block with no ' + + 'attempts is no retry block". The D2 conversion makes the 17 edit wherever the chain ' + + 'reaches: `objectstack migrate meta` lists every block in your metadata sources that omits the ' + + 'count and writes both pre-17 numbers into it, which keeps the retries the job ran with; a block ' + + 'that writes `maxRetries: 0` it does not list, and you delete that block by hand (or state a ' + + 'count). Stored jobs keep loading ' + + 'with the same numbers, because every rehydration seam replays the conversion. Decide per job: ' + + 'keep the written count, or delete the block if the job should run once. A block that already ' + + 'states `maxRetries` and omits `backoffMultiplier` is not refused, and its delay is now flat ' + + '(multiplier 1) where the loader used to write 2: state `backoffMultiplier: 2` to keep the old ' + + 'curve.', + acceptanceCriteria: + '`objectstack validate` reports no refusal at a `jobs[].retryPolicy.maxRetries` path; every job that ' + + 'should retry states `maxRetries` (1 to 10) in its block, every job that should run once has no ' + + 'block, and a failing run is attempted the stated number of extra times.', + conversionIds: ['retry-policy-converged'], +}; diff --git a/packages/spec/src/shared/retired-key-removal-version.pin.test.ts b/packages/spec/src/shared/retired-key-removal-version.pin.test.ts index b10e065cd16..064dfb14742 100644 --- a/packages/spec/src/shared/retired-key-removal-version.pin.test.ts +++ b/packages/spec/src/shared/retired-key-removal-version.pin.test.ts @@ -91,6 +91,7 @@ import { SharingLevel } from '../security/sharing.zod'; import { DatasourceSchema } from '../data/datasource.zod'; import { PageSchema } from '../ui/page.zod'; import { FlowSchema } from '../automation/flow.zod'; +import { JobSchema } from '../system/job.zod'; import { PROTOCOL_MAJOR } from '../kernel/protocol-version'; import { EmailTemplateDefinitionSchema } from '../system/email-template.zod'; import { @@ -356,6 +357,21 @@ const JOIN: Readonly> = { 'nodes', ), ], + // #23543 F2: a default flip made loud — a job's retry block with no + // attempts, refused by the job parse at the key; the job is otherwise + // complete, so the block is the only issue. + 'retry-policy-converged': [ + refusal( + 'a job retryPolicy with maxRetries omitted', + () => JobSchema.safeParse({ + name: 'nightly_sync', + schedule: { type: 'cron', expression: '0 0 * * *' }, + handler: 'syncOrders', + retryPolicy: { backoffMs: 5000 }, + }), + 'retryPolicy', + ), + ], // Shipped by a 17.x minor; `toMajor: 18` like the rows above. 'page-header-breadcrumb-removed': [tombstone(PageHeaderProps, 'page:header', 'breadcrumb')], 'object-grid-default-sort-removed': [tombstone(ObjectGridPropsSchema, 'object-grid', 'defaultSort')], diff --git a/packages/spec/src/shared/retry-policy.zod.ts b/packages/spec/src/shared/retry-policy.zod.ts index c06e5bd1a66..68d57c1f8ea 100644 --- a/packages/spec/src/shared/retry-policy.zod.ts +++ b/packages/spec/src/shared/retry-policy.zod.ts @@ -122,9 +122,11 @@ export function retryPolicyShape() { // precisely the quiet-failure class ADR-0049 exists to remove. Every owning // shape is strict since #22343, and 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; - // a copy that conversion leaves (a `backoffMs` beside it with a different - // value, or a `null`) meets this tombstone at parse. + // only carry the key. `retry-policy-converged` rewrites it wherever stored + // documents are replayed (it left the load path at 18, #23543 F2, so an + // authored copy meets this tombstone directly); a copy that conversion + // leaves (a `backoffMs` beside it with a different value, or a `null`) + // meets it at parse too. retryDelayMs: retiredKey( '`retryDelayMs` was removed in @objectstack/spec 17.0.0 — the retry policy now ' + 'has ONE spelling for its base delay across every surface that carries it: `job.retryPolicy`, ' + @@ -154,7 +156,10 @@ export function retryPolicyShape() { * 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. + * changes behaviour. What changed is what a NEWLY authored omission means — + * and since 18 (#23543 F2) a JOB may not author it at all: `JobSchema` refuses + * a `retryPolicy` whose `maxRetries` is omitted or 0, while this shared schema + * keeps the default for a `try_catch` node's `retry`. * * The ETL half never got a conversion branch — deliberately, and now * permanently: an ETL pipeline was not a `defineStack` collection and diff --git a/packages/spec/src/stack-authored-input.test.ts b/packages/spec/src/stack-authored-input.test.ts index b918a9329c5..ace9ced9c48 100644 --- a/packages/spec/src/stack-authored-input.test.ts +++ b/packages/spec/src/stack-authored-input.test.ts @@ -28,6 +28,46 @@ */ import { describe, it, expect, vi, afterEach } from 'vitest'; import { composeStacks, defineStack, hasStackProvenance, stackConversionsOf } from './stack.zod'; +import type { MetadataConversion } from './conversions/types'; + +/** + * The subject is the load-time pass ITSELF, not any one entry, so the probe is + * a conversion this file owns: a live copy of the flow-level `retryDelayMs` → + * `backoffMs` rename, appended to the table the pass reads. It was a real entry + * until there was none left to borrow — `datasource-driver-mongo-to-mongodb` + * until #23543 F1, then `retry-policy-converged` until F2 retired it too — and + * every entry still live on the load path retires inside the same card (F4, + * F5-http). A probe nobody retires keeps this pin about the key, not about + * whichever entry happens to be live. + * + * `vi.mock` replaces the registry module for the whole file, so `apply.ts` + * (which `defineStack` calls) reads the extended table; nothing outside this + * file sees the probe. + */ +const PROBE = 'stack-authored-input-probe'; + +vi.mock('./conversions/registry', async (importOriginal) => { + const original = await importOriginal(); + const { mapCollection, renameKey } = await import('./conversions/walk'); + const probe: MetadataConversion = { + id: 'stack-authored-input-probe', + toMajor: 17, + surface: 'flow.errorHandling.retryDelayMs (test probe)', + summary: 'test-only probe: a live load-path rename for the authored-input pin', + apply(stack, emit) { + return 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 }; + }); + }, + fixture: { before: {}, after: {}, expectedNotices: 0 }, + }; + return { ...original, ALL_CONVERSIONS: [...original.ALL_CONVERSIONS, probe] }; +}); afterEach(() => { vi.restoreAllMocks(); @@ -36,14 +76,12 @@ afterEach(() => { /** Spelled here, by its registry name, exactly as the CLI spells it: the key is not exported. */ const AUTHORED_INPUT = Symbol.for('objectstack.stack.authoredInput'); -const RETRY = 'retry-policy-converged'; - /** * A stack the strict parse accepts, whose one non-canonical spelling is a key - * the load still converts: a flow-level retry policy's `retryDelayMs`, renamed - * `backoffMs` by `retry-policy-converged` while that entry is on the load path. - * (It was `datasources[].driver: 'mongo'` until that spelling was retired as a - * value at 18 and its conversion left the load path, #23543 F1.) + * the load converts: a flow-level retry policy's `retryDelayMs`, renamed + * `backoffMs` by the {@link PROBE} before the parse. (The real entry, + * `retry-policy-converged`, is retired from the load path, so without the + * probe the strict parse would refuse this spelling at its tombstone.) */ const service = () => ({ manifest: { id: 'com.probe.svc', name: 'Probe Service', namespace: 'probe', version: '1.0.0', type: 'module' as const }, @@ -90,14 +128,14 @@ describe('a non-strict call carrying the key returns its input as authored', () const plain = defineStack(service() as never, { strict: false }); converted(plain); - expect(ids(plain)).toEqual([RETRY]); + expect(ids(plain)).toEqual([PROBE]); }); it('a strict call ignores the key: the pass runs and is recorded', () => { quiet(); const strict = defineStack(service() as never, { [AUTHORED_INPUT]: true } as never); converted(strict); - expect(ids(strict)).toEqual([RETRY]); + expect(ids(strict)).toEqual([PROBE]); }); }); @@ -121,6 +159,6 @@ describe('composeStacks over inputs produced as authored', () => { { manifest: 'preserve' }, ); converted(body(plain, 0)); - expect(ids(plain)).toEqual([RETRY]); + expect(ids(plain)).toEqual([PROBE]); }); }); diff --git a/packages/spec/src/system/job.test.ts b/packages/spec/src/system/job.test.ts index 73414559c35..9d1b901835e 100644 --- a/packages/spec/src/system/job.test.ts +++ b/packages/spec/src/system/job.test.ts @@ -270,8 +270,9 @@ describe('RetryPolicySchema', () => { // `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. + // documents keep the old numbers — `retry-policy-converged` writes them in. + // This is the SHARED schema's default; on a job, a block that leaves the + // count at 0 is refused since 18 (#23543 F2, `JobSchema` below). it('applies opt-in defaults: a declared but empty policy does not retry', () => { const policy = RetryPolicySchema.parse({}); @@ -513,6 +514,38 @@ describe('JobSchema', () => { // its L2 member only — and `handler` is deprecated beside it. The four presence // shapes, the L1 refusal and the one time-limit spelling are pinned here; the // runtime binder that SCHEDULES a body is a separate change. +// #23543 F2 (ruling 6182001882 item 2): a job's retry block must buy a retry. +describe('JobSchema.retryPolicy — a block with no attempts is refused', () => { + const job = (retryPolicy?: Record) => ({ + name: 'nightly_sync', + schedule: { type: 'cron' as const, expression: '0 0 * * *' }, + handler: 'syncOrders', + ...(retryPolicy === undefined ? {} : { retryPolicy }), + }); + + it.each([ + ['omitted', {}], + ['omitted beside other keys', { backoffMs: 5000, jitter: true }], + ['written 0', { maxRetries: 0 }], + ])('refuses `maxRetries` %s at the key, with the prescription', (_name, policy) => { + const parsed = JobSchema.safeParse(job(policy)); + expect(parsed.success).toBe(false); + expect(parsed.error!.issues).toHaveLength(1); + const [issue] = parsed.error!.issues; + expect(issue!.code).toBe('custom'); + expect(issue!.path).toEqual(['retryPolicy', 'maxRetries']); + expect(issue!.message.startsWith( + 'A retry block with no attempts is no retry block: state `maxRetries` ≥ 1, for example 3, or delete the block.', + )).toBe(true); + }); + + it('CONTROL — no block is a single attempt, and `maxRetries: 1` parses with the multiplier at its default', () => { + expect(JobSchema.safeParse(job()).success).toBe(true); + const parsed = JobSchema.parse(job({ maxRetries: 1 })); + expect(parsed.retryPolicy).toMatchObject({ maxRetries: 1, backoffMultiplier: 1 }); + }); +}); + describe('JobSchema.body', () => { const base = { name: 'nightly_sweep', schedule: { type: 'interval' as const, intervalMs: 60000 } }; const body = { language: 'js' as const, source: "await ctx.api.object('task').find({});", capabilities: ['api.read' as const] }; @@ -791,9 +824,8 @@ describe('Job Scheduling Integration', () => { at: '2024-12-31T00:00:00Z', }, handler: 'jobs/migration.ts', - retryPolicy: { - maxRetries: 0, // No retries for migrations - }, + // No retries for migrations: no `retryPolicy` block. A block with + // `maxRetries: 0` is refused (#23543 F2). timeoutMs: 7200000, // 2 hours }; diff --git a/packages/spec/src/system/job.zod.ts b/packages/spec/src/system/job.zod.ts index f3680455f83..28e227b31f4 100644 --- a/packages/spec/src/system/job.zod.ts +++ b/packages/spec/src/system/job.zod.ts @@ -116,17 +116,21 @@ export type OnceSchedule = z.input; * publishing the name (and its `system/RetryPolicy` def, keyed by entry * namespace). * - * Three things change for job authors, all covered by the + * Three things changed for job authors at 17, 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". + * no deployed job changes behaviour. * - `maxRetries` is capped at 10 and `backoffMultiplier` floored at 1 (the * automation constraints). Both reject loudly at parse time. + * + * At 18 (#23543 F2) the conversion left the load path, and a job's block must + * state `maxRetries` >= 1: one that omits it or writes 0 is refused + * (`JOB_RETRY_POLICY_NO_ATTEMPTS` below). Stored rows still replay the + * conversion, so a row written before 17 keeps its three retries. */ export { RetryPolicySchema, type RetryPolicy, type RetryPolicyParsed } from '../shared/retry-policy.zod'; import { RetryPolicySchema } from '../shared/retry-policy.zod'; @@ -248,6 +252,34 @@ const JOB_BODY_TIMEOUT_REFUSED = + 'runtime bounds the sandbox run by it. Move the value to `timeoutMs` on the job (milliseconds, ' + 'unchanged).'; +/** + * A `retryPolicy` block that buys no retry (#23543 F2, ruling 6182001882 item + * 2). `maxRetries` defaults to 0 on the shared policy, so a block that omits + * it — or writes 0 — parses to "run once, never retry": a declared retry the + * runtime does not deliver (AGENTS.md Prime Directive #10 corollary). Until 18 + * the `retry-policy-converged` load conversion hid that by writing the pre-17 + * `maxRetries: 3` into such a block; it is retired from the load path, so the + * job parse refuses the block in both spellings — written 0 and defaulted 0 — + * the way `flow.errorHandling` refuses `strategy: 'retry'` with no attempts. + * + * Judged here, on the job, and not on `RetryPolicySchema`: that schema is + * shared with a `try_catch` node's `retry` region, which this ruling does not + * cover. A job with no block stays a single attempt, and an omitted + * `backoffMultiplier` keeps its declared default (1): it only shapes the delay + * curve, so there is no silent loss behind it to refuse. + * + * The judgement reads the PARSED policy: the default fills an omitted count + * with 0 before the refine runs, so omitted and 0 are one case by construction. + */ +const JOB_RETRY_POLICY_NO_ATTEMPTS = + 'A retry block with no attempts is no retry block: state `maxRetries` ≥ 1, for example 3, or delete the ' + + 'block. This `retryPolicy` omits `maxRetries` or writes 0, so the job would run once and never retry. ' + + 'Until @objectstack/spec 18 the loader wrote `maxRetries: 3` into a block that omitted it (the pre-17 ' + + 'default); it no longer does, and a retry is a decision the job states, because a retry re-runs ' + + 'whatever the failed attempt already did. ' + + 'Run `os migrate meta --from 16` to list the mechanical edits for existing sources; `--write` applies ' + + 'the ones it can prove, and you apply the rest by hand.'; + export const JobSchema = lazySchema(() => strictObject({ surface: 'this job', history: @@ -363,7 +395,7 @@ export const JobSchema = lazySchema(() => strictObject({ + '`degraded`. Data like `body`, so every door schedules it. Refused beside `body` or `handler`.', ), organization: ScheduleOrganizationSchema.optional().describe(JOB_ORGANIZATION_DESCRIPTION), - 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. Omit the block for a single attempt; declaring it without `maxRetries` also means no retry since 17.0.0 — state a count to opt in.'), + 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. Omit the block for a single attempt; a block must state `maxRetries` >= 1 — one that omits it or writes 0 is refused, since it would buy no retry.'), // Renamed from `timeout` (#14478): the unit (milliseconds) lived only in the // description while the sibling `retryPolicy.backoffMs` spells its own. // Tombstoned rather than deleted so the rejection carries the rename. @@ -394,6 +426,14 @@ export const JobSchema = lazySchema(() => strictObject({ .refine((job) => job.pull === undefined || (job.body === undefined && job.handler === undefined), { message: JOB_PULL_EXCLUSIVE, path: ['pull'], + }) +// A rule over ONE key of the block, written on the job rather than on the +// shared `RetryPolicySchema` (see JOB_RETRY_POLICY_NO_ATTEMPTS). Like the +// refine above it has no arm in the closed projection list; it lands on the +// job's root site, which `dropped-refinements.baseline.json` already records. + .refine((job) => job.retryPolicy === undefined || job.retryPolicy.maxRetries >= 1, { + message: JOB_RETRY_POLICY_NO_ATTEMPTS, + path: ['retryPolicy', 'maxRetries'], })); export type Job = z.input;