From 6f26acfedc7e093cee77e64de028c47aea752224 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 13:04:11 +0000 Subject: [PATCH] fix(spec): stop teaching the retired ETL layer as a live retry surface (#6630, part 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #6414 retired the whole L2 ETL layer, but `shared/retry-policy.zod.ts` was updated in exactly one stanza and left every other `ETLPipeline.retry` mention in the present tense — including the `retryDelayMs` `retiredKey()` guidance, which is not documentation but the upgrade channel itself. - The tombstone string now enumerates the three surfaces that still carry the policy (`job.retryPolicy`, a `try_catch` node's `retry`, `flow.errorHandling`) instead of four. The prescription (`retryDelayMs` -> `backoffMs`, same value, same `os migrate meta --from 16` pointer) is untouched. - The module TSDoc's four-surface arithmetic and strict-surface counts are corrected, and the paragraph claiming a `maxAttempts` tombstone is the ETL migration channel now states what is true: no such tombstone exists, it went with the shape that carried it, and `RETIRED_DEFS_BY_MAJOR` plus the D3 `etl-pipeline-layer-retired` entry are the declaration (tsc TS2724/TS2305 is the door). - `conversions/registry.ts`'s matching stanza gets the same treatment. - New pin in `shared/retry-policy.test.ts` asserts the enumeration in both directions; no pre-existing assertion could see this drift. Regenerated `content/docs/references/{automation/control-flow,system/job}.mdx` (the tombstone's `.describe()` reaches them); no acceptance change anywhere. Part of #6630 — part 1 of 2; `automation/flow.zod.ts`'s three sites follow in part 2, after #5593 releases that file. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018ffcE95NaMJcL9XJ9VDYgk --- .../retry-policy-etl-tombstone-enumeration.md | 42 +++++++++++++ .../references/automation/control-flow.mdx | 2 +- content/docs/references/system/job.mdx | 2 +- packages/spec/src/conversions/registry.ts | 39 +++++++----- packages/spec/src/shared/retry-policy.test.ts | 41 ++++++++++++ packages/spec/src/shared/retry-policy.zod.ts | 63 +++++++++++-------- 6 files changed, 143 insertions(+), 46 deletions(-) create mode 100644 .changeset/retry-policy-etl-tombstone-enumeration.md diff --git a/.changeset/retry-policy-etl-tombstone-enumeration.md b/.changeset/retry-policy-etl-tombstone-enumeration.md new file mode 100644 index 0000000000..5ddf4ce34d --- /dev/null +++ b/.changeset/retry-policy-etl-tombstone-enumeration.md @@ -0,0 +1,42 @@ +--- +"@objectstack/spec": patch +--- + +fix(spec): the `retryDelayMs` tombstone no longer points an upgrading author at the retired ETL surface (#6630) + +#6414 retired the whole L2 ETL layer (`automation/etl.zod.ts`: `ETLPipeline`, +`ETLPipelineRun`, their source/destination/transformation vocabulary, four enums +and the `ETL` factory — 9 defs, 27 exported names). `shared/retry-policy.zod.ts` +was updated in exactly one stanza and left every other mention of +`ETLPipeline.retry` in the present tense — including the one string that is not +documentation. + +**The author-visible half.** `retryDelayMs`'s `retiredKey()` guidance is the +upgrade channel `shared/retired-key.ts` describes in its own words ("an agent +bumping `@objectstack/spec` sees THIS string, not our docs site"). It enumerated +four surfaces on which the converged spelling applies, and the fourth — "an ETL +pipeline's `retry`" — had been deleted in the same major. An author or agent +migrating `retryDelayMs` → `backoffMs` was therefore told, by the platform's own +upgrade prescription, that a surface exists where `tsc` now reports TS2724/TS2305. +The message now names exactly the three surfaces that still carry the policy: + +``` +… the retry policy now has ONE spelling for its base delay across every surface +that carries it: `job.retryPolicy`, a `try_catch` node's `retry` and +`flow.errorHandling`. Rename the key to `backoffMs`; … +``` + +The prescription itself is unchanged — same rename, same value semantics, same +`os migrate meta --from 16` pointer — and a new pin in +`shared/retry-policy.test.ts` now asserts the enumeration in both directions +(every live surface named, no retired one named) so the string cannot drift wide +again unobserved. + +**No acceptance change.** `retryPolicyShape()` and `RetryPolicySchema` keep their +exact key sets, bounds and defaults; `retryDelayMs` is still rejected, still with +a prescription. Everything else in this change is comment text: the module TSDoc +in `shared/retry-policy.zod.ts` (the four-surfaces arithmetic, the strict-surface +counts, and the paragraph that claimed a `maxAttempts` tombstone carries the ETL +migration — it does not; the tombstone went with the shape that carried it, and +`RETIRED_DEFS_BY_MAJOR` plus the D3 `etl-pipeline-layer-retired` entry are the +declaration) and the matching stanza in `conversions/registry.ts`. diff --git a/content/docs/references/automation/control-flow.mdx b/content/docs/references/automation/control-flow.mdx index c3331ccfb4..ed6f2c3ae9 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); | **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** | `never` | optional | [REMOVED] `retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661, #4964) — the retry policy now has ONE spelling for its base delay across every surface that carries it: `job.retryPolicy`, a `try_catch` node's `retry`, `flow.errorHandling` and an ETL pipeline's `retry`. Rename the key to `backoffMs`; the value (milliseconds before the first retry) is unchanged. `os migrate meta --from 16` rewrites it for you. | +| **retryDelayMs** | `never` | optional | [REMOVED] `retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661, #4964) — the retry policy now has ONE spelling for its base delay across every surface that carries it: `job.retryPolicy`, a `try_catch` node's `retry` and `flow.errorHandling`. 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 0986570879..a475f81e72 100644 --- a/content/docs/references/system/job.mdx +++ b/content/docs/references/system/job.mdx @@ -125,7 +125,7 @@ const result = CronScheduleSchema.parse(data); | **backoffMultiplier** | `number` | ✅ | Exponential backoff multiplier; 1 (the default) keeps the delay flat | | **maxRetryDelayMs** | `integer` | ✅ | Ceiling for a single backoff delay (ms) | | **jitter** | `boolean` | ✅ | Randomize each delay within [50%, 100%] of its computed value — spreads a thundering herd of simultaneous retries | -| **retryDelayMs** | `never` | optional | [REMOVED] `retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661, #4964) — the retry policy now has ONE spelling for its base delay across every surface that carries it: `job.retryPolicy`, a `try_catch` node's `retry`, `flow.errorHandling` and an ETL pipeline's `retry`. Rename the key to `backoffMs`; the value (milliseconds before the first retry) is unchanged. `os migrate meta --from 16` rewrites it for you. | +| **retryDelayMs** | `never` | optional | [REMOVED] `retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661, #4964) — the retry policy now has ONE spelling for its base delay across every surface that carries it: `job.retryPolicy`, a `try_catch` node's `retry` and `flow.errorHandling`. 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/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index 749c332d65..1c3bafc49a 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -3712,7 +3712,7 @@ const objectIndexTypePartialRemoved: MetadataConversion = { * see the `datasource-inert-blocks-removed` note above, which leans on * exactly that distinction), so the platform drops from three spellings to * two rather than four. `retryDelayMs` is tombstoned (`retiredKey`) — NOT - * deleted — because two of the four owning shapes are not `.strict()`: a + * deleted — because two of the three owning shapes are not `.strict()`: a * plain deletion would have Zod silently swallow the authored number and * fall back to the 1000ms default, which is the quiet-failure class * ADR-0049 removes. @@ -3733,32 +3733,37 @@ const objectIndexTypePartialRemoved: MetadataConversion = { * Jobs with no `retryPolicy` block at all are left alone — absence already * meant a single attempt on both sides of the change. * - * ## The two surfaces this entry grew to cover (#4964 / #4962) + * ## The ONE further surface this entry grew to cover (#4964 / #4962) * * The convergence above was driven by the dual-source instrument, whose * question is "how many declarations share one exported NAME?". Two further - * encodings of the identical policy were invisible to it because they are + * encodings of the identical policy were invisible to it because they were * anonymous inline `z.object`s with no exported name at all — and after a * convergence lands, a surviving dialect reads as reviewed-and-kept rather - * than missed: + * than missed. One of the two is still a surface; the other went with its + * layer: * * - **`flow.errorHandling`** (#4964) spelled the base delay `retryDelayMs`; * every other key, bound and default already matched. Step 0 below renames * it, so the ONE authorable casualty of the whole convergence is still just - * that word — now retired everywhere it was ever legal rather than on two - * surfaces out of four. + * that word — now retired everywhere it was ever legal rather than only on + * the two shapes #4661's instrument could see. * - **`ETLPipeline.retry`** (#4962) spelled the count `maxAttempts` and - * defaulted it to 3. It gets **no step here, deliberately.** An ETL pipeline - * is not a `defineStack` collection and `etl.zod.ts` has no parse site in - * objectstack / objectui / cloud (批 12's measurement), so there is no - * stored or authored document a walker could reach: a branch for it would be - * dead code claiming migration coverage that does not exist, which is the - * ADR-0049 failure this registry is supposed to prevent, not commit. Its - * `maxAttempts` tombstone carries the rename AND the default change, and the - * tombstone reaches the only doors that exist (`tsc` at the authoring site, - * and the parse). That is also why the ETL default flip 3 → 0 needs no - * materialization step while the job one did: nothing is deployed under the - * old reading. + * defaulted it to 3. It got **no step here, deliberately** — and never will. + * An ETL pipeline was not a `defineStack` collection and `etl.zod.ts` had no + * parse site in objectstack / objectui / cloud (批 12's measurement), so + * there was no stored or authored document a walker could reach: a branch + * for it would have been dead code claiming migration coverage that does not + * exist, which is the ADR-0049 failure this registry is supposed to prevent, + * not commit. #6414 then retired the whole L2 layer, and #4962's own entry + * (`etl-retry-converged-onto-retry-policy`) was ABSORBED into + * `etl-pipeline-layer-retired` inside the same unreleased major. There is no + * `maxAttempts` tombstone left to carry the rename or the default change: it + * went with the shape that carried it, which is strictly stronger, because + * no `retry` block survives to author the key into. `tsc` (TS2724/TS2305 on + * the removed ETL names) is the whole channel. That is also why the ETL + * 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: `FlowNodeSchema.config` is an unconstrained * record, so no schema rejection can reach `config.retry.retryDelayMs` and the diff --git a/packages/spec/src/shared/retry-policy.test.ts b/packages/spec/src/shared/retry-policy.test.ts index bb270ba074..e38fc1a9cf 100644 --- a/packages/spec/src/shared/retry-policy.test.ts +++ b/packages/spec/src/shared/retry-policy.test.ts @@ -76,6 +76,47 @@ describe('RetryPolicy is a single declaration across entries (#4661)', () => { expect(parse).toThrow(/backoffMs/); } }); + + // The tombstone STRING is not documentation. `shared/retired-key.ts` says so + // in its own words — "an agent bumping `@objectstack/spec` sees THIS string, + // not our docs site" — which makes its enumeration of carrying surfaces + // contract, not prose. It went stale exactly once and in the worst direction: + // it kept listing "an ETL pipeline's `retry`" after #6414 deleted the whole L2 + // layer, so the one message guaranteed to reach an upgrading author named a + // fourth place to write `backoffMs` on which `tsc` now reports TS2724/TS2305 + // (#6630). Nothing could see that: every pre-existing assertion on this string + // matched `/backoffMs/` or `/retryDelayMs/`, i.e. the prescription, never the + // enumeration. Pin the enumeration in BOTH directions. + // + // Note on the assertion shape: there is no ADR-0112 `code`/`status` envelope + // to assert here. `retiredKey()` is a Zod `never` whose issue carries the + // guidance as its `message`, so for this rejection class the wording IS the + // whole contract (#5240) and the message is the only thing worth asserting. + it('the tombstone enumerates exactly the surfaces that still carry the policy (#6630)', () => { + const result = RetryPolicySchema.safeParse({ retryDelayMs: 500 }); + expect(result.success).toBe(false); + const message = result.error!.issues.map((issue) => issue.message).join('\n'); + + // Every surface that builds from `retryPolicyShape()` today, spelled the way + // an author writes it. A prescription naming a scope NARROWER than the truth + // is the #4964 defect (a real surface left out reads as "not converged"). + for (const live of ['`job.retryPolicy`', "`try_catch` node's `retry`", '`flow.errorHandling`']) { + expect(message, `the prescription must still name ${live}`).toContain(live); + } + + // ...and nothing retired. Wider than the truth is the #6630 defect: the + // export absence is pinned in `automation/sync-retirement.test.ts`, so a + // surface named here that cannot be imported there is a signpost to a + // compile error. + expect( + message, + 'the prescription must not point at a surface #6414 retired', + ).not.toMatch(/\bETL\b/i); + + // None of the above may be bought by weakening the prescription itself. + expect(message).toContain('Rename the key to `backoffMs`'); + expect(message).toContain('os migrate meta --from 16'); + }); }); describe('RetryPolicySchema — converged shape', () => { diff --git a/packages/spec/src/shared/retry-policy.zod.ts b/packages/spec/src/shared/retry-policy.zod.ts index e81acb6202..3b0725b760 100644 --- a/packages/spec/src/shared/retry-policy.zod.ts +++ b/packages/spec/src/shared/retry-policy.zod.ts @@ -39,11 +39,12 @@ * the question simply was not the one whose answer everybody read off it. After * a convergence completes, a surviving dialect reads as *reviewed and kept*. * - * Both now build from {@link retryPolicyShape}, so the four surfaces share one - * declaration of the key set, the bounds and the defaults. The two of them that - * are `.strict()` keep their own `strictObject` curation and their own extra - * keys (`Flow.errorHandling.strategy`) — what is shared is the *contract*, not - * the surface's framing of it. + * The surviving one of those two builds from {@link retryPolicyShape} like the + * named surfaces do, so the three remaining surfaces share one declaration of + * the key set, the bounds and the defaults. The only one of them that is + * `.strict()` — `Flow.errorHandling` — keeps its own `strictObject` curation and + * its own extra keys (`strategy`); what is shared is the *contract*, not the + * surface's framing of it. * * ## Why this file, and why it is not in `shared/index.ts` * @@ -77,13 +78,14 @@ import { retiredKey } from './retired-key'; * The retry policy's raw Zod shape — key set, bounds, defaults and prose, in * ONE place. * - * Two of the four surfaces that carry this policy cannot simply reference - * {@link RetryPolicySchema}: `Flow.errorHandling` and `ETLPipeline.retry` are - * `.strict()` (`strictObject`, the #4001 campaign standard) and the flow one - * also carries `strategy` plus its own `superRefine`. Handing them the *shape* - * rather than the *schema* is what lets them stay strict, keep their curated - * unknown-key tables, and still have exactly one declaration of what a retry - * policy IS — the alternative (a fifth hand-copied key list) is the debt #4964 + * One of the three surfaces that carry this policy cannot simply reference + * {@link RetryPolicySchema}: `Flow.errorHandling` is `.strict()` + * (`strictObject`, the #4001 campaign standard) and also carries `strategy` plus + * its own `superRefine`. (`ETLPipeline.retry` was the second such surface until + * #6414 retired the whole L2 ETL layer.) Handing it the *shape* rather than the + * *schema* is what lets it stay strict, keep its curated unknown-key table, and + * still have exactly one declaration of what a retry policy IS — the + * alternative, one hand-copied key list per strict surface, is the debt #4964 * and #4962 exist to remove. * * It is a function rather than a const for the same reason every schema here is @@ -110,17 +112,17 @@ export function retryPolicyShape() { // ── Tombstone (ADR-0087) ──────────────────────────────────────────── // `retryDelayMs` was the automation-side spelling of `backoffMs`, on BOTH // `try_catch`'s `retry` (#4661) and `Flow.errorHandling` (#4964). It is - // tombstoned rather than deleted because two of the four owning shapes are + // tombstoned rather than deleted because two of the three owning shapes are // not `.strict()`: a plain deletion would have Zod silently strip the // authored value and drop the delay back to the 1000ms default, which is - // precisely the quiet-failure class ADR-0049 exists to remove. On the two - // strict surfaces the tombstone is still the better channel — it carries - // the rename, where a bare unknown-key rejection would only carry the key. + // precisely the quiet-failure class ADR-0049 exists to remove. On the strict + // surface the tombstone is still the better channel — it carries the + // rename, where a bare unknown-key rejection would only carry the key. // `retry-policy-converged` rewrites it on the load path. retryDelayMs: retiredKey( '`retryDelayMs` was removed in @objectstack/spec 17.0.0 (#4661, #4964) — the retry policy now ' + 'has ONE spelling for its base delay across every surface that carries it: `job.retryPolicy`, ' + - "a `try_catch` node's `retry`, `flow.errorHandling` and an ETL pipeline's `retry`. " + + "a `try_catch` node's `retry` and `flow.errorHandling`. " + 'Rename the key to `backoffMs`; the value (milliseconds before the first retry) ' + 'is unchanged. `os migrate meta --from 16` rewrites it for you.', ), @@ -129,9 +131,10 @@ export function retryPolicyShape() { /** * Exponential-backoff retry policy — the named schema for `job.retryPolicy` and - * a `try_catch` node's `retry` region. `Flow.errorHandling` and - * `ETLPipeline.retry` carry the same contract via {@link retryPolicyShape}, - * which they must, being `.strict()` (see that function's note). + * a `try_catch` node's `retry` region. `Flow.errorHandling` carries the same + * contract via {@link retryPolicyShape}, which it must, being `.strict()` (see + * that function's note). `ETLPipeline.retry` did the same until #6414 retired + * the L2 ETL layer. * * Delay before retry *n* is `min(backoffMs * backoffMultiplier^(n-1), * maxRetryDelayMs)`, optionally jittered. @@ -146,13 +149,19 @@ export function retryPolicyShape() { * `backoffMultiplier: 2`) into existing job documents, so no deployed stack * changes behaviour. What changes is what a NEWLY authored omission means. * - * The ETL half needed no conversion branch and deliberately has none: an ETL - * pipeline is not a `defineStack` collection and `etl.zod.ts` has no parse site - * anywhere in objectstack / objectui / cloud (批 12's measurement), so there is - * no stored document for a D2 walker to reach. Writing one anyway would be a - * conversion advertising coverage it does not have. The `maxAttempts` tombstone - * on that block is the whole migration channel, and it reaches the only door - * that exists — `tsc` at the authoring site, and the parse. + * The ETL half never got a conversion branch — deliberately, and now + * permanently: an ETL pipeline was not a `defineStack` collection and + * `etl.zod.ts` had no parse site anywhere in objectstack / objectui / cloud + * (批 12's measurement), so there was no stored document for a D2 walker to + * reach and writing one anyway would have been a conversion advertising + * coverage it did not have. #6414 then retired the whole L2 layer, which took + * `ETLPipeline.retry` with it. There is therefore NO `maxAttempts` tombstone: + * a tombstone keeps a key unwritable on a shape that survives, and no `retry` + * block survives to author the key into — an absence strictly stronger than a + * tombstone. What declares the removal is `RETIRED_DEFS_BY_MAJOR` plus the D3 + * `etl-pipeline-layer-retired` semantic migration (which ABSORBED #4962's own + * conversion entry for exactly this reason), and the door an upgrading author + * actually hits is `tsc` — TS2724/TS2305 on every removed ETL name. * * 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