Run the TypeScript retry loop beneath the middleware chain - #538
Merged
Conversation
…s declared max The retry middleware retried by calling raw fetch from onResponse, which left the middleware chain — so each pass yielded at most one chained retry (SPEC waiver 2B.1) and intermediate attempts needed hand-rolled hook bookkeeping. Replace createRetryMiddleware with createRetryingFetch, a Swift-style attempt loop passed to openapi-fetch as the client's custom fetch. openapi-fetch calls it exactly once per logical request, after all onRequest middleware and before onResponse/onError, so: - Status retries run to the operation's declared retry.max (per-op metadata via the existing getRetryConfigForRequest lookup; no request annotation). - The loop owns every attempt's onRequestStart and each abandoned attempt's onRequestEnd/onRetry; the lifecycle middleware finalizes only the terminal attempt, after the cache middleware's 304 -> cached-200 transform. - RequestLifecycle re-keys from openapi-fetch's id to a WeakMap on the final Request object (identity holds from custom fetch to onResponse/onError), so release() bookkeeping becomes structural and attemptOf() gives way to a loop-local counter. - The body replay buffer is closure-local, captured via request.clone() before attempt 1 and only when the request can actually retry — the id-keyed bodyCache dies, and never-retryable POSTs skip buffering entirely. Network errors still rethrow immediately (addressed next). Un-skips the "GET operation retries on 503" conformance case: 3 requests with backoff, previously capped at 2.
…rminal With the loop beneath the middleware chain, a rejected fetch is finally catchable mid-flight: retry it under the same per-operation gate as status retries, so GETs and idempotent mutations re-send while a non-idempotent POST (NO_RETRY_CONFIG, one attempt) surfaces its original error untouched — the conformance runner classifies transport failures by the raw error, so identity is load-bearing and pinned by test. Aborts are terminal regardless of budget: caller cancellation must not re-send, and the request timeout is one budget shared by every attempt and backoff — once it fires, a retry would instantly re-reject. The existing timeout lifecycle test pins this at exactly one attempt. Two lifecycle tests flip deliberately, proven red first: a GET whose fetch keeps rejecting now runs to maxAttempts with balanced hooks per attempt ([1,2,3], all statusCode 0), and a 503 followed by fetch rejections now ends [503, 0, 0] instead of stopping at the second attempt. Un-skips "Network error on an idempotent POST is retried then succeeds": CompleteTodo re-sends after a transport failure and succeeds on the 204.
SPEC section 7: TypeScript joins the network-retrying group (idempotency-gated, aborts terminal), the one-retry cap disappears from the Gate 3 consumption table, the divergence bullets, and the enforcement paragraph, and Appendix F's TS and Swift rows now describe the loop-beneath-the-chain model. The zero-skip roster drops exactly the two TS lines whose gaps this branch closes. rubric-audit 2B.1 flips from waiver to plain pass, recording why the old note is obsolete (same shape as the 3C.6 flip). check-retry-metadata-parity gains a client.ts token row so the createRetryingFetch consumption site is guarded like base.ts, and the behavioral-proof pointer now names the TS retry tests. Test-file headers and comments stop describing the retry middleware that no longer exists.
jeremy
added
enhancement
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0776cd6c4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… aborts Codex review: AbortController.abort(reason) makes fetch reject with the caller's custom reason rather than a DOMException named AbortError, so the abort carve-out misclassified those cancellations as retryable network errors — spurious onRetry events, pointless backoff sleeps, and re-sends against an already-aborted signal. request.signal.aborted is now the authoritative test, with the DOMException check kept for abort-shaped rejections that arrive without an aborted signal. Regression test proven red first: a custom-reason abort retried to attempts [1,2,3]; it now stays terminal at one attempt with the reason's identity preserved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c476d9d646
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review, round 2: an abort — caller cancellation or the request-timeout budget — firing during a backoff was not observed by the plain setTimeout sleep, so the request stayed pending for the full delay and then began another attempt (start, auth refresh, fetch) against an already-aborted signal. sleep now races the delay against request.signal and rejects with the signal's abort reason the moment it fires, on both the status and network backoff paths. Attempt N was already finalized before the sleep, and attempt N+1 never begins, so starts and ends stay balanced; onRetry having announced N+1 is the inherent race of cancelling between announce and begin. Regression test proven red first: aborting 100ms into a 2s Retry-After backoff previously surfaced after 2054ms; it now rejects promptly with the caller's reason identity preserved and no second attempt.
Member
Author
|
Merging on green CI + Codex review convergence. Status for the record:
|
This was referenced Aug 1, 2026
Merged
jeremy
added a commit
that referenced
this pull request
Aug 19, 2026
Both landed in #538 and survived #655's sweep because its selector required a no-argument abort() and both pass a reason. The custom-reason test raced a 50ms timer against a 1000ms mocked response — #655's own numbers. It now aborts from inside the MSW handler, so the request has provably reached the transport, and rests on err === reason: the caller's own Error object, which the request timeout (a TimeoutError DOMException) and a completed request (undefined) cannot produce. The backoff test raced a 100ms timer against a 2s Retry-After window and paired it with a redundant Date.now() ceiling. It now aborts from hooks.onRetry, the seam executeWithRetry fires immediately before it sleeps, and rests on starts == [1]: a sleep that ignored the signal would run the delay out and then begin attempt 2, which is what the deleted ceiling was standing in for. The abort is queued as a microtask rather than called synchronously so it lands inside the sleep, exercising the abort listener rather than the already-aborted fast path.
jeremy
added a commit
that referenced
this pull request
Aug 21, 2026
…produced two of them (#794) * Abort from a seam, not a timer, in the two caller-abort lifecycle tests Both landed in #538 and survived #655's sweep because its selector required a no-argument abort() and both pass a reason. The custom-reason test raced a 50ms timer against a 1000ms mocked response — #655's own numbers. It now aborts from inside the MSW handler, so the request has provably reached the transport, and rests on err === reason: the caller's own Error object, which the request timeout (a TimeoutError DOMException) and a completed request (undefined) cannot produce. The backoff test raced a 100ms timer against a 2s Retry-After window and paired it with a redundant Date.now() ceiling. It now aborts from hooks.onRetry, the seam executeWithRetry fires immediately before it sleeps, and rests on starts == [1]: a sleep that ignored the signal would run the delay out and then begin attempt 2, which is what the deleted ceiling was standing in for. The abort is queued as a microtask rather than called synchronously so it lands inside the sleep, exercising the abort listener rather than the already-aborted fast path. * Hand the captured CONNECT preamble across a Queue instead of sharing a String test_proxy_credentials_are_percent_decoded filled a captured = +"" on the proxy thread and asserted on it from the main thread with no join, Queue or condvar. The ordering was incidental — the socket close — and the client's own timeout: 1 can raise while the proxy is still mid-gets on a loaded runner, which reads as absent credentials and points at percent-decoding, which is not the bug. Observed red on CI twice, on a branch touching only Go files. Distinct from #720: that was a find_proxy resolver stall, closed by #734 removing the resolver from these tests. This survives that fix. Queue rather than Thread#join because join(timeout) returning nil leaves you reading the shared String with no edge at all. Here the value's existence is the evidence — it can only be popped because the producer finished reading the preamble — so a wedged producer is an explicitly reported failure rather than an empty match. pop(timeout: 15) is a wait bound, not a timing assertion: this assertion is about content, so a generous wait costs nothing. The sub-second bounds #734 kept on the siblings, where the margin is the deadline, are untouched. * Record the corrected abort-on-a-timer selector where the next sweeper stands #655 scoped its fix with an rg typed into an issue body, requiring a no-argument abort(); #715 shipped no script, lint rule or make target, so there is no durable selector to widen. The corrected form goes in CONTRIBUTING.md under Testing, stated with its coverage bound rather than as a claim to cover every call site, alongside the rule it enforces and the Ruby reading of the same rule. Whether this deserves an actual gate is a separate decision, deliberately not taken here. * Gate the timer-scheduled abort with an oxlint rule, not another selector #655 declared this class contained on the strength of an rg typed into an issue body; two survivors shipped anyway and one went red in CI two months later. The finding that there is no durable instrument is the reason to build the first one, and this is the accident class — a colleague adding a timer, no adversary to route around it — so a syntactic invariant is the right shape. An oxlint JS plugin rather than a grep in a make target, and rather than introducing eslint: oxlint is already the repo's TS linter and supports local JS plugins, so this adds no dependency and no lockfile churn. It reads the AST, which is what lets it tell a timer that SCHEDULES an abort from one the abort is merely racing — the proximity selector I wrote first flags device.test.ts:1632 and the rule does not, so no suppression is needed there. The rule's header states what it cannot see rather than claiming every call site. The self-test runs beside the rule, not only when the rule is edited. oxlint's JS plugin API is alpha and un-semvered behind a caret range: a bump that stopped dispatching the visitor leaves the gate exiting 0 while matching nothing. Demonstrated by mutating create() to return a visitor that never fires — the gate stayed green, the self-test went red. Wired into make ts-check AND into the TypeScript CI job as its own steps, since no CI job in this repo runs make check. * Read the rule's findings as JSON, and run the self-test in both environments The gate's first CI run went red, and the rule was not the reason: oxlint detects GITHUB_ACTIONS and switches to the ##[error] annotation format, which the line-scraping parser could not match. The rule fired on all three positive fixtures; the harness could not see it. It failed closed, which is the behaviour it was built for, but a harness that reads differently on the machine that matters is a harness that will one day be wrong in the other direction. Two changes: parse --format json and match on the rule CODE rather than message text, so a finding also has to come from this rule and not merely exist; and run every case twice, once with GITHUB_ACTIONS unset and once with it set, so the environment is part of the matrix rather than something the local run happens to get right. Invoke the oxlint binary from node_modules/.bin directly instead of through npx, so resolution is the same everywhere too. Re-proved the disarmed-visitor mutation against the rewritten harness: the gate stays green under a simulated CI environment, and the self-test reds in both environments. * Assert the promptness the backoff-abort test is named for The attempt ledger only catches a sleep that ignores cancellation entirely. A sleep that noticed the abort but deferred its rejection to the timer's expiry never starts attempt 2 and still rejects with the caller's reason, so every assertion in the test passed — two seconds late — while the behavior in its name was broken. Demonstrated: that mutant runs green against the previous version in 2.77s and red against this one in 17ms. Fake timers close it by construction rather than by measurement. With setTimeout frozen, the 2s backoff timer fires only if the test advances the clock, and it never does, so "the request settled" and "it settled before the backoff elapsed" are one statement. The barrier is a count of event-loop turns via setImmediate, not an elapsed-time bound: under load a turn takes longer, but how many turns a settlement needs does not change. No wall-clock threshold comes back (#783). Also from review: the lint rule's ancestor walk compared the callback function against the timer's first argument, which is the wrapper node when TypeScript-only syntax sits in between — `setTimeout((() => c.abort()) as () => void, 50)` and the `satisfies` and `!` spellings all slipped a gate whose header claimed inline callbacks were covered. The walk now erases those wrappers first, with a self-test case that reds without the fix. CONTRIBUTING's population bound was dot-member only while the rule also recognizes bare `abort()` and `c["abort"]()`; it now covers all three spellings, which return the same 15 sites today. * Restore the frozen clock where a timed-out test can still reach it A `finally` inside the test does not run when vitest times the test out — the suspended async function is never resumed — and a hung test is exactly what a broken abort produces, so the one failure the cleanup existed for was the one it could not cover. Measured: on a timed-out test the finally does not run, the suite's afterEach does, and it sees vi.isFakeTimers() still true. The restore moves there, and the try/finally goes rather than sitting beside it covering strictly fewer paths. CONTRIBUTING said to drop the elapsed-time ceiling because identity and the attempt ledger already discriminate. That is true except when the named behavior is promptness, which is the case this PR just had to fix, so as written it told contributors to delete the only promptness check and put nothing back. It now says to replace it, and how: freeze the clock, count event-loop turns, never advance. * Make the population sweep as quote-blind as the rule it bounds The rule's isAbortCall reads the computed key's VALUE, so controller['abort']() is an abort to it exactly as controller["abort"]() is. The documented population bound — in the rule header and in CONTRIBUTING — matched only the double-quoted spelling, so a sweep typed from the docs could report one site fewer than the rule recognizes, which is the same drift between stated and actual coverage the bound exists to prevent. Widen it to both quote styles in both places, kept byte-identical. Both the old and the new command return the same 15 lines under typescript/tests today, so no classification changes. The self-test's computed-abort case now carries the single-quoted spelling too: a rule mutated to be quote-sensitive reports [2,3,4] where [2,3,4,5] is expected, in both environments, so the claim that the quote style is invisible to the rule is asserted rather than read. * Recognize a computed-literal timer property, as the rule already does for abort
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 SPEC waiver 2B.1 and the TS network-error divergence in one architectural move: the retry loop now runs beneath the openapi-fetch middleware chain, as the client's custom
fetch(Swift's #517 directive-loop is the model).What changed
Commit 1 — the loop beneath the chain.
createRetryMiddlewareis deleted and replaced bycreateRetryingFetch(lifecycle, authStrategy, enableRetry), passed tocreateClient({ fetch }). openapi-fetch calls it exactly once per logical request, after everyonRequestmiddleware and before everyonResponse/onError, so:retry.max(per-op metadata via the existinggetRetryConfigForRequestURL+method lookup — no request annotation, avoiding the TypeScript: one request lifecycle, so every attempt is observed exactly once #503 wire-leak class).onRequestStartand each abandoned attempt'sonRequestEnd/onRetry; the lifecycle middleware finalizes only the terminal attempt, after the cache middleware's 304 → cached-200 transform (the fromCache means served from the ETag cache, so require the header that proves it #505 crown-jewel test passes unmodified).RequestLifecyclere-keys from openapi-fetch'sidto aWeakMap<Request, AttemptState>(identity holds from custom fetch toonResponse/onError);release()becomes structural andattemptOf()gives way to a loop-local counter. The(failed, upcoming)onRetry pair is untouched.request.clone()before attempt 1, gated onmaxAttempts > 1— the id-keyedbodyCachedies, and never-retryable POSTs no longer buffer at all.Commit 2 — network-error retry, idempotency-gated. A rejected fetch is retried under the same per-operation gate as status retries: GETs and idempotent mutations re-send with backoff; a non-idempotent POST (
NO_RETRY_CONFIG) makes one attempt and surfaces its original error object (identity pinned by test — the conformance runner classifies transport failures by the raw error).AbortError/TimeoutErrorare terminal regardless of budget: caller cancellation must not re-send, and the request timeout is one budget shared by all attempts and backoffs.Commit 3 — doc sweep. SPEC §7 (network-retry grouping, Gate 3 table, divergence bullets, enforcement paragraph), Appendix F TS+Swift rows, the §19 zero-skip roster (exactly the two TS lines this PR closes), rubric-audit 2B.1 → plain pass (same shape as the 3C.6 flip), and a
client.tstoken row incheck-retry-metadata-parity.py.Conformance
Two
TS_SDK_SKIPSentries removed, both proven red first against the old build:expected 3 requests, got 2; now passes with real backoff (delayBetweenRequests >= 1000).expected 2 requests, got 1; now passes (CompleteTodo re-sends, succeeds on 204).TS skips drop 5 → 3 (integer precision waiver 1B.6 + two DownloadURL lines, a different gap — see below).
Red proofs (unit)
expected 2, got 3when the loop ignores per-opmax, and the non-idempotent-POST identity test failscalled 1 times, but got 3when the network catch ignores the idempotency gate.[1,2,3]all statusCode 0, and 503-then-rejections ends[503, 0, 0]. Both shown red before commit 2. The timeout test does NOT flip — it pins abort-is-terminal at exactly one attempt.Accepted behavior deltas (outside SDK-used surface)
fetchOptions.fetchoverride now bypasses retry and request-level hooks entirely (previously it got middleware retry).onRequestwith aResponsenow produces zero start/end events (previously start+end); openapi-fetch skips both the custom fetch andonResponseon that path.onRequestStartnow fires after ALLonRequestmiddleware rather than between auth and cache.Deliberately untouched
downloadURL's raw-fetch path and its two conformance skips (a different gap). The loop body is structured so a later rock can extractexecuteWithRetry— a pure function of request/config/hook seams — and letdownload.tshop 1 adopt it.Verification
check-retry-metadata-parity.pygreen, now including theclient.tsconsumption row.2B.1grep gate: remaining references are the §20 rubric table row (criterion still met), the rubric-audit historical note, and two test comments describing the closed waiver.Summary by cubic
Runs the TypeScript retry loop beneath the
openapi-fetchmiddleware via a custom fetch. Retries now go to each operation’s max, include idempotency-gated network-error retries, and aborts/timeouts are terminal — even during backoff.New Features
retry_on,maxAttempts) with backoff andRetry-After.Refactors
createRetryingFetch(lifecycle, authStrategy, enableRetry)passed viacreateClient({ fetch }).RequestLifecyclenow keys state byRequestviaWeakMap; removesrelease()andattemptOf().request.clone()when retries are possible; removes the id-keyed body cache.fetchOptions.fetchbypasses retry and hooks;onRequestshort-circuits emit no start/end; attempt 1 start fires after allonRequestmiddleware.Written for commit 32e7087. Summary will update on new commits.