Current behaviour
resilient-fetcher (published 1.0.1) retries every non-ok response below 500, even though it documents "retry on network errors and 5xx responses". The test that would catch this is skipped.
packages/resilient-fetcher/src/index.ts:
- Lines 35-38,
defaultRetryOn: returns true when response is null, else response.status >= 500. For a 404 with a real response, this returns false.
- Lines 89-97: on
!response.ok, the retryOn(null, response) guard at line 91 correctly returns false for a 404, so it falls through to throw new Error(\Request failed with status ${response.status}`)` at line 97.
- Lines 106-123: that throw is caught by the same function's
catch. Line 119 calls retryOn(err, null) with response = null, and the default returns true for a null response, so the request is retried.
Net effect: every non-ok status below 500 throws at line 97, is caught, and gets retried anyway. The documented "5xx only" behaviour does not hold.
packages/resilient-fetcher/src/index.test.ts:80-81 has test.skip('does not retry on 4xx by default', ...) with a comment claiming the logic is "verified working in integration". The one test that pins this down never runs.
Confirm:
sed -n '35,127p' packages/resilient-fetcher/src/index.ts
sed -n '80,92p' packages/resilient-fetcher/src/index.test.ts
Why it matters
A 404 or a 400 gets retried up to retries times, adding latency and load for responses that will never change. It also contradicts the package's own documentation, which callers rely on when deciding whether to add their own retry logic.
Fix shape
In the catch, only consult retryOn for genuine transport and timeout errors, and let the non-retryable HTTP throw from line 97 propagate. Then un-skip the test.
Related, fold in while here: lines 82-85 spread finalOptions then overwrite signal with the internal controller's signal, so a caller-supplied AbortSignal is silently ignored.
How you know it worked
Un-skip the 4xx test and pnpm vitest run packages/resilient-fetcher passes with mockFetch called once.
Current behaviour
resilient-fetcher(published 1.0.1) retries every non-ok response below 500, even though it documents "retry on network errors and 5xx responses". The test that would catch this is skipped.packages/resilient-fetcher/src/index.ts:defaultRetryOn: returnstruewhenresponseis null, elseresponse.status >= 500. For a 404 with a real response, this returnsfalse.!response.ok, theretryOn(null, response)guard at line 91 correctly returns false for a 404, so it falls through tothrow new Error(\Request failed with status ${response.status}`)` at line 97.catch. Line 119 callsretryOn(err, null)withresponse= null, and the default returnstruefor a null response, so the request is retried.Net effect: every non-ok status below 500 throws at line 97, is caught, and gets retried anyway. The documented "5xx only" behaviour does not hold.
packages/resilient-fetcher/src/index.test.ts:80-81hastest.skip('does not retry on 4xx by default', ...)with a comment claiming the logic is "verified working in integration". The one test that pins this down never runs.Confirm:
Why it matters
A 404 or a 400 gets retried up to
retriestimes, adding latency and load for responses that will never change. It also contradicts the package's own documentation, which callers rely on when deciding whether to add their own retry logic.Fix shape
In the
catch, only consultretryOnfor genuine transport and timeout errors, and let the non-retryable HTTP throw from line 97 propagate. Then un-skip the test.Related, fold in while here: lines 82-85 spread
finalOptionsthen overwritesignalwith the internal controller's signal, so a caller-suppliedAbortSignalis silently ignored.How you know it worked
Un-skip the 4xx test and
pnpm vitest run packages/resilient-fetcherpasses withmockFetchcalled once.