Stop retrying 4xx and honor caller AbortSignal in resilient-fetcher

.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.

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) => {

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);
         }
     }
 }