fix(core): an abort surfaces the caller's reason; a cancel is never a retry - #674
Open
rejifald wants to merge 2 commits into
Open
fix(core): an abort surfaces the caller's reason; a cancel is never a retry#674rejifald wants to merge 2 commits into
rejifald wants to merge 2 commits into
Conversation
… retry
systemClock.sleep rejected every abort with a minted Error('aborted'),
so a caller who aborted a call parked in a retry backoff lost their
AbortError / custom abort(reason). The same logic already existed as
engine.ts abortReason and resilience.ts abortError — three hand-copies,
one drifted. util.ts now exports the one abortReason helper (reject with
signal.reason when it is an Error, else a generic abort Error) and
sleep, the engine, and withTimeout's link path all share it.
Second half: when a caller aborted mid-flight on a non-final attempt,
the attempt-loop catch still emitted a 'retry' progress event and fired
onRetry before the backoff sleep rejected — a phantom retry for a
deliberately cancelled call. The catch now rethrows immediately when
baseReq.signal is aborted (after onError, which legitimately observed
the attempt ending).
The mocking kit gets the same treatment for seam fidelity: manualClock
must behave like systemClock through the clock seam, and mockAdapter's
abortable delay must reject like real fetch (which surfaces
signal.reason) — without that, neither fix is observable under
injected-clock tests.
Tests: abort with a custom reason during backoff surfaces that reason
(one attempt, no leaked timer); abort mid-flight emits no retry event
and skips onRetry; direct pins that both clocks' sleep reject with the
same Error instance. All six fail against the pre-fix code.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ised ~21 → ~22 kB The abort-reason fix measures 24.08 / 21.51 KB gzip against a main at 24.08 / 21.50 — a +0.01 KB change against a stitch-scenario budget main had run down to 0.00 KB of headroom on. The conventional ~0.2 KB step (24.10→24.30 / 21.50→21.70), sized to the headroom the gate is meant to hold rather than to this change, so the next small core-path fix is not gated on a budget PR of its own. Full rationale lives with the budgets in bundle-size.mjs, including why the shared-helper dedup stays despite gzip charging for it (keeping the duplicated copies measured no better, with +0.19 KB minified). 21.51 crosses the advertised figure's rounding boundary, so the six managed sites under the bundle-advertised-size tether move ~21 → ~22 kB (the whole entry stays ~24; the core README's whole-entry brotli figure also moves to ~22, which 21.57 KB always rounded to). All nine yakir tethers check ok. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
What
Two abort-semantics bugs in the resilience chain, plus the mocking kit's mirrors of them:
systemClock.sleepswallowed the abort reason. It rejected every abort with a mintedError('aborted'), so a caller who aborted a call parked in a retry-backoff sleep lost theirAbortError/ customabort(reason). The same prefer-the-signal's-reason logic already existed twice —abortReason()in engine.ts andabortError()in resilience.ts — three hand-copies, one drifted.util.tsnow exports the oneabortReasonhelper andsleep, the engine's abort paths, andwithTimeout's signal link all share it.retryprogress event and fired theonRetryhook before dying in the backoff sleep — a phantom retry for a deliberately cancelled call. The catch now rethrows immediately whenbaseReq.signalis aborted (afteronError, which legitimately observed the attempt ending).manualClock.sleepnow rejects with the reason exactly likesystemClock.sleep(the clock seam's whole contract), andmockAdapter's abortable delay / pre-aborted check reject like realfetchdoes (which surfacessignal.reason). Without these, neither fix is observable under injected-clock tests.Tests
New
test/abort-reason.spec.tsplus one case inmanual-clock.spec.ts:safe()— one attempt only, backoff timer dropped, not leaked (manualClock, fully virtual);retryevents, never firesonRetry, and the error event carries the reason;sleepreject with the same Error instance the caller aborted with, plus the non-Error-reason fallback.All six fail against the pre-fix code (verified by reverting src and running). Full core suite: 1494 passed; eslint and
tsc --noEmitclean; pre-push gauntlet green.Reviewer notes
mainat 24.08 / 21.50). The fix is minified-smaller (−0.09 KB from the dedup) but gzip charges +0.01 KB — the deleted duplicate copies were near-free backreferences. Keeping them duplicated measured no better (21.51 either way, +0.19 KB minified), so the dedup stays;mainhad run down to 0.00 KB of stitch-scenario headroom, so the conventional ~0.2 KB step applies (the Implement ADR 0023 — fix the store-backed cold-start burst, then widen the rate denominator #620 shape: sized to the headroom the gate holds, not to this change). Full rationale is with the budgets inbundle-size.mjs.bundle-advertised-sizetether are updated (both READMEs, installation + principles pages, home metrics, docs source blurb; the whole entry stays ~24). The core README's whole-entry brotli figure moves to ~22 as well — 21.57 KB always rounded there.retryevent — but the subsequent backoff now rejects promptly with the caller's reason.🤖 Generated with Claude Code