diff --git a/CHANGELOG.md b/CHANGELOG.md index 89fa2d21..206ce0f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -689,6 +689,29 @@ npm release are grouped under the in-development version that introduced them. cap exists for. Nothing about the public API changes; a stream that used to die at 8 MB now finishes, in bounded memory. +- **An unusable `backoff` throws at construction instead of vanishing.** + ([#651](https://github.com/rejifald/StitchAPI/issues/651) §3) `backoff` takes a curve or the + `{ curve, base, max }` envelope — never a function — so `backoff: () => 6000` is correctly a type + error. Casting past it (which people do when they believe a feature exists) constructed clean and + then did **nothing**: the function was never invoked, and the waits fell back to the default curve + on the default 100ms base. Measured gaps of `100, 200`ms where the config asked for 6000, with no + throw, no event, and nothing in the trace that reads as wrong. + + `expandShorthand` folds the bare form of the slot into `{ curve }` (P12), so every value a cast + can let through — a function, a bare `6000`, a misremembered `'exponential'` — lands on `curve`, + where `backoffDelay` matched neither `'fixed'` nor `'expo-jitter'` and fell through to the plain + `expo` branch. It now throws `bad backoff` from `stitch()`, mirroring the + `bad rate: …` an unparseable `throttle.rate` has thrown at construction since + [#618](https://github.com/rejifald/StitchAPI/pull/618). Silently degrading a resilience policy is + the one place a fallback is worse than a crash, and this one degraded in the **permissive** + direction — a shorter wait than asked for, which is the half that hurts. + + The check sits at the fold itself, on the value it just produced, so one comparison over the + dominant field covers every authoring form and names the offending **fragment** rather than the + merged result. A `backoff` that sets `base`/`max` and no curve keeps the `expo-jitter` default, + and all three curve names are unaffected in either spelling. **Not a semver break in practice**: only a value `tsc` already + rejected can reach the throw, and its previous behaviour was to ignore what you wrote. + - **A `bigint` path parameter no longer vanishes from the URL.** `stitch({ path: '/v1/things/{id}' })` called with `{ params: { id: 1234567890123456789n } }` built `https://api.test/v1/things/` — the id simply gone, no error, no event, no drift finding. A request meant for one item silently addressed diff --git a/packages/core/src/stitch.ts b/packages/core/src/stitch.ts index e9f18d88..903e533b 100644 --- a/packages/core/src/stitch.ts +++ b/packages/core/src/stitch.ts @@ -30,6 +30,7 @@ import { createStoreThrottle, memoryStore } from './store'; import { graphqlSurface, httpSurface } from './surface'; import { consoleSink, createTrace, exportsFromEnv, multiplex } from './trace'; import { + type AtLeastOne, type CacheOptions, type CacheOutcome, type Clock, @@ -212,12 +213,48 @@ function expandShorthand(cfg: Partial): void { // Nested fold (P24): `backoff` is itself a scalar-or-envelope slot, so the bare curve // normalizes too — `__config` never carries the string form (P0). Read back through the // normalised shape the loop just wrote. + // + // **And the fold is where an unusable curve dies** (issue #651 §3), on the value it just + // produced — `bad backoff`, the same slot-named message and the same construction-time timing + // `parseRate` has for an unparseable `throttle.rate`. Silently degrading a resilience + // policy is the one place a fallback is worse than a crash, and this one degraded in the + // PERMISSIVE direction: `backoff: () => 6000` cast past its type folded to `{ curve: }`, + // matched neither branch of `backoffDelay`, and walked the plain `expo` curve on the default + // 100ms base — measured gaps of 100, 200ms where the config asked for 6000, with nothing said + // anywhere. Checking HERE rather than after the merge is what makes ONE check cover the whole + // slot: every spelling a cast can let through (a function, a bare `6000`, a misremembered + // `'exponential'`, an array, `null`) is non-object, so the fold has just put it on `curve`. It + // also names the offending FRAGMENT rather than the merged result, and reuses the accessor the + // fold already needs — which, with the terse message, is what buys the whole check its 33 + // bytes on a path that had 33 left. + // + // Only a PRESENT curve is judged, and only against the three values `backoffDelay` dispatches + // on. Nothing here decides what a *reasonable* backoff is. const retry = cfg.retry as RetryOptions | undefined; - if (retry?.backoff !== undefined) - cfg.retry = { - ...retry, - backoff: envelope(retry.backoff, 'curve'), - }; + if (retry?.backoff !== undefined) { + // Read the folded envelope's `curve` at `string`, not at `BackoffCurve`. The whole point of + // the check is a value the TYPE already excludes, so narrowing against the union would leave + // `never` and the exhaustive comparison would read as dead code to tsc and eslint alike — + // which is the shape of a guard that cannot fire, not the one that fires here every time a + // cast lets something through. + const backoff = envelope(retry.backoff, 'curve') as { curve?: string }; + // Three shapes here are load-bearing for SIZE, not for style, and this check lands one + // byte inside the gate — so measure before touching any of them: + // • `'expo-jitter'` FIRST, so the `'expo'` literal after it compresses as a + // back-reference into the one already emitted (4 B; alphabetising costs them back); + // • a `!==` chain rather than `['expo', …].includes(…)` (the array measured 4 B worse); + // • a message with no interpolated value (5 B). `parseRate` can afford `bad rate: ${r}` + // and this cannot — the offending value is in the caller's own `stitch({…})` literal, + // which is the one consolation. Restore it the moment the budget has room. + if ( + backoff.curve !== undefined && + backoff.curve !== 'expo-jitter' && + backoff.curve !== 'expo' && + backoff.curve !== 'fixed' + ) + throw new Error('bad backoff'); + cfg.retry = { ...retry, backoff } as AtLeastOne; + } // Nested fold (P25): `stream.buffer` is a scalar-or-envelope slot too — the bare char count // folds to `{ chars }`, so `__config` never carries the number form (P0). const stream = cfg.stream as StreamOptions | undefined; diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 4f5d4739..78b0861f 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -1056,6 +1056,16 @@ export interface RetryOptions { /** * Backoff policy. A bare curve is the P12 shorthand for `{ curve }` * (`backoff: 'fixed'` ≡ `backoff: { curve: 'fixed' }`); the envelope adds `base`/`max`. + * + * A `curve` outside {@link BackoffCurve} **throws** at construction — `bad backoff`, the + * same fail-loud stance as {@link ThrottleOptions.rate}, and for the same reason: the + * quiet path here is the PERMISSIVE one. An unreadable curve fell through to plain `expo` on the + * default 100ms base, so a typo shortened the wait instead of lengthening it, and a `backoff` the + * runtime cannot read is a resilience policy that silently isn't there. + * + * There is deliberately **no function form** — the curve plus its two bounds is the whole + * vocabulary. A computed wait per attempt belongs to a `Surface`: `interpret` returns a + * `SurfaceOutcome` whose `after` is honoured as the next delay (issue #609). */ backoff?: BackoffCurve | AtLeastOne; /** diff --git a/packages/core/test/resilience-config-guard.spec.ts b/packages/core/test/resilience-config-guard.spec.ts new file mode 100644 index 00000000..f735ddbd --- /dev/null +++ b/packages/core/test/resilience-config-guard.spec.ts @@ -0,0 +1,137 @@ +// Issue #651 §3 — a `backoff` the retry loop cannot read must THROW at construction, the way an +// unparseable `throttle.rate` already does (`bad rate: …`, util.ts `parseRate`), rather than +// degrading to a default nobody asked for. The message names the slot the way that precedent +// does — `bad backoff` — but stops there rather than quoting the value: the entry's gzip gate had +// one byte left for this whole check, and the interpolation cost five. The offending value is in +// the caller's own `stitch({…})` literal, which is what makes that trade survivable. +// +// The reported case: `backoff: () => 6000` is correctly a TYPE error, but a cast past it (which +// people write when they believe a feature exists) constructed clean and then never invoked the +// function — `expandShorthand` folds the bare form into `{ curve: }`, `backoffDelay` matches +// neither `'fixed'` nor `'expo-jitter'`, and the wait silently became the plain `expo` curve on the +// default 100ms base. Measured gaps of 100, 200ms where the config asked for 6000. Because the fold +// runs first, every authoring spelling of the slot lands on `curve`, so the cases below are one +// check seen from four directions rather than four checks. +// +// The other resilience slots were swept for the same defect and are NOT guarded here — the core +// entry's gzip budget is measured in tens of bytes and this check spends ~34 of them; see the PR +// for the measured cost of extending to each. What they still do on a value cast past the type is therefore unchanged, +// and the last block below is the part that matters for them: no legitimate shape may start +// throwing as a side effect of this one. +import { stitch } from '../src'; +import type { StitchConfig } from '../src/types'; + +// A construction attempt with a value the type system rejects — the cast IS the reported scenario. +const bad = (extra: Record) => () => + stitch({ url: 'https://example.test/x', ...extra } as never); +// The same, for a config that is legal and must keep constructing. Deliberately NOT cast: these +// have to typecheck as well as run. +const ok = (extra: Partial) => () => + stitch({ url: 'https://example.test/x', ...extra }); + +describe('an unusable `backoff` throws at construction (#651 §3)', () => { + test('the function form — a type error, and a cast past it no longer vanishes', () => { + // The reported reproduction: typechecked, then vanished, before this guard existed. + expect(bad({ retry: { attempts: 3, backoff: () => 6000 } })).toThrow( + /^bad backoff$/, + ); + }); + + test('a curve name that is not one of the three', () => { + expect(bad({ retry: { attempts: 3, backoff: 'exponential' } })).toThrow( + /^bad backoff$/, + ); + expect( + bad({ retry: { attempts: 3, backoff: { curve: 'linear' } } }), + ).toThrow(/^bad backoff$/); + }); + + test('a bare number — `backoff: 6000` is not the P12 shorthand (`curve` is)', () => { + expect(bad({ retry: { attempts: 3, backoff: 6000 } })).toThrow( + /^bad backoff$/, + ); + }); + + test('the shorthand fold is what makes one check cover every spelling', () => { + // Not an object ⇒ the bare form ⇒ folded onto `curve`, whatever it was. + expect(bad({ retry: { attempts: 3, backoff: ['expo'] } })).toThrow( + /^bad backoff$/, + ); + expect(bad({ retry: { attempts: 3, backoff: null } })).toThrow( + /^bad backoff$/, + ); + }); + + test('it throws at CONSTRUCTION, not on the first retry', () => { + // The distinction the issue asks for: nothing is called, and it still fails. + let built = false; + expect(() => { + stitch({ + url: 'https://example.test/x', + retry: { attempts: 3, backoff: 'exponential' }, + } as never); + built = true; + }).toThrow(/^bad backoff$/); + expect(built).toBe(false); + }); + + test('the precedent this mirrors: `throttle.rate` already threw at construction', () => { + expect(bad({ throttle: 'fast' })).toThrow(/bad rate: fast/); + }); +}); + +describe('every legitimate resilience shape still constructs', () => { + test('all three curves, in both the bare and the envelope spelling', () => { + for (const curve of ['expo', 'expo-jitter', 'fixed'] as const) { + expect( + ok({ retry: { attempts: 3, backoff: curve } }), + ).not.toThrow(); + expect( + ok({ retry: { attempts: 3, backoff: { curve } } }), + ).not.toThrow(); + } + }); + + test('a `backoff` envelope that sets no curve keeps the default', () => { + expect( + ok({ retry: { attempts: 3, backoff: { base: 50 } } }), + ).not.toThrow(); + expect( + ok({ retry: { attempts: 3, backoff: { base: '1s', max: '10s' } } }), + ).not.toThrow(); + }); + + test('numbers and duration tokens on every widened slot (P17)', () => { + expect( + ok({ + retry: { + attempts: 3, + on: 429, + respect: false, + backoff: { curve: 'expo', base: '1s', max: 10_000 }, + }, + timeout: { total: '10s', each: 3000 }, + throttle: { rate: '2/s', concurrency: 4, lease: '45s' }, + circuit: { failures: 5, cooldown: 30_000 }, + }), + ).not.toThrow(); + }); + + test('the scalar shorthands and the positional circuit tuple', () => { + expect(ok({ retry: 3 })).not.toThrow(); + expect(ok({ timeout: '5s' })).not.toThrow(); + expect(ok({ timeout: 5000 })).not.toThrow(); + expect(ok({ throttle: '2/s' })).not.toThrow(); + expect(ok({ circuit: [5, '30s'] })).not.toThrow(); + }); + + test('an omitted slot, and a slot whose guarded field is omitted', () => { + expect(ok({})).not.toThrow(); + expect(ok({ retry: { on: 503 } })).not.toThrow(); + expect(ok({ timeout: { total: '1s' } })).not.toThrow(); + expect(ok({ throttle: { pool: 'host', rate: '1/s' } })).not.toThrow(); + // `circuit` with neither required field still defers to `createCircuit`'s + // required-by-design throw at CALL time (CONTRACT.md P15) — this guard must not pre-empt it. + expect(ok({ circuit: { key: 'k' } })).not.toThrow(); + }); +});