diff --git a/.changeset/fix-no-4xx-retry.md b/.changeset/fix-no-4xx-retry.md new file mode 100644 index 0000000..951be08 --- /dev/null +++ b/.changeset/fix-no-4xx-retry.md @@ -0,0 +1,5 @@ +--- +"resilient-fetcher": patch +--- + +Stop retrying non-ok responses that retryOn rejects (4xx by default), and honor a caller-supplied AbortSignal. diff --git a/packages/resilient-fetcher/src/index.test.ts b/packages/resilient-fetcher/src/index.test.ts index a396d0d..b44acaa 100644 --- a/packages/resilient-fetcher/src/index.test.ts +++ b/packages/resilient-fetcher/src/index.test.ts @@ -77,8 +77,7 @@ describe('resilientFetch', () => { expect(mockFetch).toHaveBeenCalledTimes(2); }); - // TODO: Fix fake timer/AbortController conflict - logic verified working in integration - test.skip('does not retry on 4xx by default', async () => { + test('does not retry on 4xx by default', async () => { mockFetch.mockResolvedValue({ ok: false, status: 404 }); const promise = resilientFetch('https://example.com', { @@ -91,6 +90,22 @@ describe('resilientFetch', () => { expect(mockFetch).toHaveBeenCalledTimes(1); }); + test('retries 5xx up to the retry limit then throws', async () => { + mockFetch.mockResolvedValue({ ok: false, status: 503 }); + + const promise = resilientFetch('https://example.com', { + retries: 2, + retryDelay: 50 + }).catch(e => e); + + await vi.advanceTimersByTimeAsync(50); + await vi.advanceTimersByTimeAsync(50); + + const error = await promise; + expect(error.message).toBe('Request failed with status 503'); + expect(mockFetch).toHaveBeenCalledTimes(3); + }); + test('custom retryOn condition', async () => { mockFetch .mockResolvedValueOnce({ ok: false, status: 429 }) @@ -147,6 +162,42 @@ describe('resilientFetch', () => { expect(call[1].signal).toBeInstanceOf(AbortSignal); }); + test('caller abort stops the request and is not retried', async () => { + mockFetch.mockImplementation((_url, options) => { + return new Promise((_resolve, reject) => { + options.signal.addEventListener('abort', () => { + const err = new Error('Aborted'); + err.name = 'AbortError'; + reject(err); + }); + }); + }); + + const caller = new AbortController(); + const promise = resilientFetch('https://example.com', { + retries: 3, + retryDelay: 50, + signal: caller.signal + }).catch(e => e); + + caller.abort(); + await vi.advanceTimersByTimeAsync(500); + + const error = await promise; + expect(error.name).toBe('AbortError'); + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + + test('already aborted caller signal makes no fetch call', async () => { + const caller = new AbortController(); + caller.abort(); + + await expect( + resilientFetch('https://example.com', { signal: caller.signal }) + ).rejects.toMatchObject({ name: 'AbortError' }); + expect(mockFetch).not.toHaveBeenCalled(); + }); + test('aborts on timeout', async () => { vi.useRealTimers(); mockFetch.mockImplementation((_url, options) => { diff --git a/packages/resilient-fetcher/src/index.ts b/packages/resilient-fetcher/src/index.ts index 5c76d54..ffaaa95 100644 --- a/packages/resilient-fetcher/src/index.ts +++ b/packages/resilient-fetcher/src/index.ts @@ -37,6 +37,14 @@ const defaultRetryOn = (_error: Error | null, response: Response | null): boolea return response.status >= 500; }; +/** Thrown for a non-ok response that is not retried. Never passed to retryOn. */ +class HttpStatusError extends Error { + constructor(status: number) { + super(`Request failed with status ${status}`); + this.name = 'HttpStatusError'; + } +} + /** * A resilient wrapper around the native fetch API with retry and timeout capabilities. * @@ -74,9 +82,18 @@ export async function resilientFetch( finalOptions = await onRequest(finalUrl, fetchOptions); } + const callerSignal = finalOptions.signal ?? undefined; + while (true) { + // Do not start an attempt after the caller aborted (also covers the backoff sleep) + if (callerSignal?.aborted) { + throw callerSignal.reason ?? new DOMException('This operation was aborted', 'AbortError'); + } + const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), timeout); + const onCallerAbort = () => controller.abort(); + callerSignal?.addEventListener('abort', onCallerAbort, { once: true }); try { const response = await fetch(finalUrl, { @@ -94,7 +111,7 @@ export async function resilientFetch( await new Promise(resolve => setTimeout(resolve, delay)); continue; } - throw new Error(`Request failed with status ${response.status}`); + throw new HttpStatusError(response.status); } // Apply response interceptor @@ -105,6 +122,23 @@ export async function resilientFetch( return response; } catch (error) { clearTimeout(timeoutId); + + // The caller aborted: surface the abort and never retry + if (callerSignal?.aborted) { + if (onError) { + onError(error as Error); + } + throw error; + } + + // A non-ok HTTP response already went through retryOn above + if (error instanceof HttpStatusError) { + if (onError) { + onError(error); + } + throw error; + } + attempt++; const isAbort = (error as Error).name === 'AbortError'; @@ -123,6 +157,8 @@ export async function resilientFetch( } throw err; + } finally { + callerSignal?.removeEventListener('abort', onCallerAbort); } } }