fix(core): an unusable backoff throws at construction instead of vanishing - #666
Merged
Merged
Conversation
rejifald
force-pushed
the
claude/backoff-construction-validation
branch
2 times, most recently
from
August 5, 2026 19:24
6bdde83 to
5cc50dc
Compare
…nishing
`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, 200ms 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'`, an array, `null` — lands on `curve`, where `backoffDelay`
matched neither `'fixed'` nor `'expo-jitter'` and fell through to the plain
`expo` branch.
The guard sits at that fold, on the value it just produced, so one comparison
covers every authoring form, names the offending fragment rather than the merged
result, and reuses the accessor the fold already needs. It throws `bad backoff`,
mirroring the `bad rate: …` an unparseable `throttle.rate` has thrown at
construction since #618 — same lifecycle point, same reasoning: the quiet path
here is the PERMISSIVE one, so a typo shortened the wait instead of lengthening
it. Silently degrading a resilience policy is the one place a fallback is worse
than a crash.
Size is the binding constraint and it is now severe. #663 and #664 both landed
while this was in review, leaving 34 B free on `import { stitch }` — 33 before
the advertised kB rounds 21 → 22 and breaks the `bundle-advertised-size` tether.
This check is +33 (22 015 B, one byte inside the gate; entry +28 at 24 656 B,
22 B free). Getting there cost the interpolated value in the error message
(5 B); the curve-comparison order and the `!==` chain are worth another 8. All
three carry comments saying so, because they read as things to tidy. The value
should go back into the message at the next budget step.
Refs #651
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rejifald
force-pushed
the
claude/backoff-construction-validation
branch
from
August 5, 2026 19:28
5cc50dc to
520180b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes §3 only of #651 —
backoffas a function typechecks-then-vanishes. §1 and §2 are untouched; see What remains.Root cause
backoffacceptsBackoffCurve | AtLeastOne<BackoffOptions>— there is no function form — sobackoff: () => 6000is correctly a type error. Casting past it (which people do when they believe a feature exists) constructed clean and then did nothing.expandShorthandfolds the bare form of the slot into its dominant field (P12), so the function landed oncurve:envelopetreats anything that isn't a plain object as the bare form, so() => 6000became{ curve: () => 6000 }. ThenbackoffDelaydispatches oncurve:A function matches neither arm, so it fell through the
elseto the plainexpobranch on the default 100ms base — the issue's measured gaps of100, 200ms where the config asked for 6000. No throw, no event, nothing in the trace that reads as wrong.The important half is the direction: the quiet fallback is the permissive one. A typo made the client wait less than asked, i.e. retry harder against an endpoint the author was trying to back off from. That is the same failure ADR 0023 names ("quietly more permissive than requested") and the reason
parseRatefails loud.The fix, and how it mirrors the
throttle.rateprecedentparseRate(packages/core/src/util.ts) throwsbad rate: `${r}`for an unreadable rate, and reaches the caller at construction becausecreateThrottleruns insidemakeStitch. This copies all three properties of that precedent:throttle.rate(existing)retry.backoff(this PR)bad rate: fastbad backoff(see size)stitch()/seam()constructionstitch()/seam()constructionundefinedwould mean no limit at allImplementation is one comparison in
packages/core/src/stitch.ts, placed at the P12 fold itself — on the valueenvelopehas just produced:Three things fall out of that placement:
6000,'exponential',['expo']andnullall arrive as{ curve: <that> }. Guarded on!== undefined, so an envelope that sets onlybase/maxkeeps theexpo-jitterdefault.expandShorthandruns per layer, so a badbackoffin anextendschain is reported where it was written.composeis reached frommakeStitch, the single construction chokepoint, so this coversstitch,seam,llm,sse,download,graphqlandpostmessagealike.Two details in that snippet are load-bearing for size and carry comments saying so, because they look like things to tidy:
'expo-jitter'is compared first so the'expo'literal after it compresses as a back-reference into the one already emitted — 4 B.!==chain rather than['expo', …].includes(…)— the array measured 4 B worse. Theas { curve?: string }read is what keeps that chain lint-clean: narrowed againstBackoffCurvethe last comparison isnever, andno-unnecessary-condition(correctly) calls that dead code.parseRatecan affordbad rate: ${r}; this could not. It is the one concession I am unhappy about and it should be reverted at the next budget step (one line, +5 B).RetryOptions.backoff's JSDoc gains the fail-loud sentence, matching the oneThrottleOptions.ratealready carries, plus a pointer to the seam that does do a computed wait (SurfaceOutcome.after, #609) — which is the feature people were reaching for.The sweep: every resilience slot, and what I found
I swept
retry/circuit/timeout/throttle/backofffor the same defect. Only the curve check shipped, and the reason is the bundle-size budget, not the analysis — numbers below. Everything found is recorded here so the next PR does not re-derive it.retry.backoff(curve)6000/'exponential'/ array /null{ curve }, falls through to plainexpoon the 100 ms defaultthrottle.rate'fast','0/s','1/25d'bad rate: …at constructioncircuit.cooldown'soon'parseDuration→undefined→createCircuitthrows (at call time, under the P15 "requiresfailuresandcooldown" message)circuit.failures'five'failures >= maxis NaN-false ⇒ the breaker never tripsretry.attempts'3 times'attempt < maxis NaN-false ⇒ retry silently offretry.backoff.base/.max'6 seconds'parseDuration→undefined⇒ the 100 ms / 10 s defaultstimeout.total/.each'five seconds'parseDuration→undefined⇒ no deadline at all — the unbounded direction, closest of the lot torate's reasoningthrottle.lease'30 seconds'throttle.concurrency'two'inFlight < limitis false forever ⇒ the limiter never grants a slot (a hang, not a degrade)retry.on,throttle.pool,retry.respect,throttle.delegateacceptsStatusnormalises; a badpoolfalls to the'stitch'default; the booleans read by truthinessNo semantic validation was added anywhere: no range checks, no "this looks too large", no cross-field rules. The curve is a closed three-value enum the dispatcher switches on, so rejecting a fourth value is structural, not a judgement about what a reasonable backoff is.
Why the sweep stopped where it did — measured
Budget headroom on
mainis 100 B (whole entry) and 89 B (import { stitch }). I measured several encodings of the full sweep; the numbers are gzip deltas vsmain, entry /import { stitch }:!==chain at the fold (shipped)backoff.base+.maxifs (best of 4 tried)stitchgetPathgetPath, curve folded inThe table forms carry a ~48 B machinery premium and then cost ~6 B per slot; the direct forms start at ~46 B and cost ~19 B per slot. Neither shape reaches even the next two slots inside 89 B. The budget was not raised, per CONTRIBUTING and the gate's own comment.
Anyone picking up the rest should expect to need a budget PR of its own, and
timeout.total/.eachis the highest-value pair to take first: its silent fallback is unbounded, which is exactly the shapeparseRatefails loud for.Bundle size — exact bytes
pnpm --filter stitchapi check:size— passes, by one byte.Both PRs that were in flight landed while this was in review (#663 as
d133d86, #664 as48aa7ee), so the headroom this was sized against is gone. Baseline below ismainat48aa7ee. Local and CI agree exactly on these numbers — verified against CI's ownsizejob on48aa7ee(24.05 / 21.47 / 5.18 KB).48aa7ee)stitchapi— whole entryimport { stitch }stitchapi/authAdvertised rounded kB stay at 24 / 21, which is the harder of the two constraints:
Math.round(22016 / 1024)is 22, soimport { stitch }had 33 B — not 34 — before the figure the READMEs andinstallation.mdxquote flips and the blockingbundle-advertised-sizetether fails. That is not theoretical; it is how the first push of this PR failed CI.npx yakir check --tier executablenow reports 6/6 ok, agreeing on"21, 24".The honest read: this no longer fits, and I did not raise the budget
Every encoding I could find is in the table above; the correct-message version (
bad backoff: <value>) is +38 B, five over what is available. What shipped is the same check with the value dropped from the message — the only variant that fits, and it fits by one byte.I want to be plain that a 1 B margin is not a good place to leave the gate. It means the next core-path change of any size — including the follow-ups this very issue needs — has to open a budget PR anyway. My recommendation is to take the conventional ~0.2 KB step in
bundle-size.mjs(the same step the file's own comments describe, restoring the ~0.2 KB headroom the gate is meant to hold) and put the value back in the message in the same PR. I have not done it here because the gate's comment is explicit that bumping it must be a deliberate act in the PR that needs it, and this PR can be merged without one.Is it breaking? — no
!, and whyShipped as
fix(core):without the!marker:tscalready rejects can reach the throw.BackoffCurveis a closed union andbackoffhas no function arm, so every newly-throwing config was already a compile error. Reaching it needs anas never, anas any, a@ts-ignore, or plain JS.{ base }/{ max }-only envelopes, the scalar shorthands, the positionalcircuittuple, and everynumber | stringduration construct exactly as before — pinned by the second describe block.throttle.rate: '0/s'— a value that used to parse and silently mean no limit, now throwingbad rateat construction — shipped under Fixed, not as a breaking change (ADR 0023, Found while implementing). Same shape, same call.rcchannel differently can add the!without touching the diff.Tests
New file
packages/core/test/resilience-config-guard.spec.ts(11 tests). Written first and confirmed failing against unmodifiedsrc: 5 failed / 6 passed before the fix (the 6 passes are the "still constructs" regression guards plus the existingbad rateprecedent), 11/11 after.backoff: () => 6000cast past its type'exponential',{ curve: 'linear' }, a bare6000,['expo'],nullstitch(...)never runsthrottle.rateprecedent, pinned next to it so the two stay the same shapebackoffenvelope with no curve; a config exercisingretry/timeout/throttle/circuitwith both numbers and duration tokens; every scalar shorthand and the positional circuit tuple; omitted slots; andcircuit: { key }with neither required field, which must still reachcreateCircuit's P15 call-time throw rather than being pre-empted hereNo existing test was edited. The full workspace suite passes untouched.
Verification
All run from the repo root after rebasing onto
48aa7ee(currentmain, #663 and #664 included), all PASS:check:format,check:lint,check:types,check:types-d,test,check:size,check:contract,check:unknown-keys,check:changelog,check:docs-links. Core coverage also re-run: statements 91.21 / branches 84.06 / functions 93.05 / lines 92.81, all above the configured floors.Plus
npx yakir check --tier executable(the blockingdrift-executablejob): 6 tethers, 6 ok.No
eslint-disableand noeslint-suppressions.jsonentry was added, the budget was not raised, and there are no drive-by refactors — the diff touches only theretry.backofffold, that slot's JSDoc, the CHANGELOG and the new spec.What remains
verdict.flagreturnsok: truewith an error envelope. Not touched: its ask is an either/or (treat an absent flag path as a failure / a distinctunknown, or emit a drift-health signal), and picking the arm is a maintainer call..safe()downgradesRateLimitErrorand drops the body. Not touched, same reason: the ask offers documentation as an alternative to preservingbody/retryAfter. Worth connecting before anyone starts it: this is the sameasStitchErrorflattening atpackages/core/src/stitch.ts:738-745that a recent review found also flattensCircuitOpenError, and it is the same shape as the already-merged refactor(core)!:RateLimitErrorextendsStitchError#662 (RateLimitError extends StitchError) — so §2, theCircuitOpenErrorfinding, and refactor(core)!:RateLimitErrorextendsStitchError#662 look like one fix, not three.Hence
Refs #651, notCloses.🤖 Generated with Claude Code