Skip to content

resilient-fetcher retries every 4xx despite documenting 5xx only, and the guard test is skipped #3

Description

@arnelirobles

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions