From a8acef0ccdd0eff07f69de83b623090acde21054 Mon Sep 17 00:00:00 2001 From: Roman Onishchenko Date: Mon, 27 Apr 2026 14:18:35 +0200 Subject: [PATCH] feat(client): improve retry safety for non-idempotent requests --- packages/client/src/core/request.ts | 1 + packages/client/src/core/should-retry.ts | 15 ++- .../client/tests/integration/retry.test.ts | 76 ++++++++++++++- .../client/tests/unit/should-retry.test.ts | 97 ++++++++++++++++++- 4 files changed, 181 insertions(+), 8 deletions(-) diff --git a/packages/client/src/core/request.ts b/packages/client/src/core/request.ts index 5ad74a0..b64b29a 100644 --- a/packages/client/src/core/request.ts +++ b/packages/client/src/core/request.ts @@ -129,6 +129,7 @@ export async function request( attempt: execution.attempt, method: execution.request.method, retry, + idempotencyKey: execution.request.idempotencyKey, error, }); diff --git a/packages/client/src/core/should-retry.ts b/packages/client/src/core/should-retry.ts index c0b4192..ab45656 100644 --- a/packages/client/src/core/should-retry.ts +++ b/packages/client/src/core/should-retry.ts @@ -9,10 +9,17 @@ type ShouldRetryParams = { attempt: number; method: RequestMethod; retry: NormalizedRetryConfig; + idempotencyKey?: string | undefined; error: unknown; }; -export function shouldRetry({ attempt, method, retry, error }: ShouldRetryParams): boolean { +export function shouldRetry({ + attempt, + method, + retry, + idempotencyKey, + error, +}: ShouldRetryParams): boolean { if (attempt >= retry.attempts) { return false; } @@ -21,6 +28,12 @@ export function shouldRetry({ attempt, method, retry, error }: ShouldRetryParams return false; } + const isNonIdempotentMethod = method === 'POST' || method === 'PATCH'; + + if (isNonIdempotentMethod && !idempotencyKey) { + return false; + } + if (error instanceof NetworkError) { return retry.retryOn.includes('network-error'); } diff --git a/packages/client/tests/integration/retry.test.ts b/packages/client/tests/integration/retry.test.ts index 333a628..9d3ae00 100644 --- a/packages/client/tests/integration/retry.test.ts +++ b/packages/client/tests/integration/retry.test.ts @@ -106,7 +106,7 @@ describe('client retry', () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); - it('retries POST when explicitly allowed', async () => { + it('retries POST when explicitly allowed and idempotencyKey is provided', async () => { const fetchMock = vi .fn() .mockRejectedValueOnce(new Error('socket hang up')) @@ -128,14 +128,82 @@ describe('client retry', () => { }, }); - const result = await client.post<{ ok: boolean }>('/users', { - name: 'John', - }); + const result = await client.post<{ ok: boolean }>( + '/users', + { + name: 'John', + }, + { idempotencyKey: 'idem-123' }, + ); expect(result).toEqual({ ok: true }); expect(fetchMock).toHaveBeenCalledTimes(2); }); + it('does not retry POST requests without idempotencyKey even when POST is allowed', async () => { + const fetchMock = vi.fn().mockResolvedValue( + new Response(JSON.stringify({ error: 'server error' }), { + status: 500, + headers: { + 'content-type': 'application/json', + }, + }), + ); + + const client = createClient({ + baseUrl: 'https://api.example.com', + fetch: fetchMock, + retry: { + attempts: 3, + retryMethods: ['POST'], + retryOn: ['5xx'], + baseDelayMs: 0, + }, + }); + + await expect(client.post('/payments', { amount: 100 })).rejects.toThrow(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('retries POST requests with idempotencyKey when POST is allowed', async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce( + new Response(JSON.stringify({ error: 'server error' }), { + status: 500, + headers: { + 'content-type': 'application/json', + }, + }), + ) + .mockResolvedValueOnce( + new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { + 'content-type': 'application/json', + }, + }), + ); + + const client = createClient({ + baseUrl: 'https://api.example.com', + fetch: fetchMock, + retry: { + attempts: 2, + retryMethods: ['POST'], + retryOn: ['5xx'], + baseDelayMs: 0, + }, + }); + + await expect( + client.post('/payments', { amount: 100 }, { idempotencyKey: 'idem-123' }), + ).resolves.toEqual({ ok: true }); + + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + it('request retry config overrides client retry config', async () => { const fetchMock = vi.fn(async () => { return new Response(JSON.stringify({ message: 'Service Unavailable' }), { diff --git a/packages/client/tests/unit/should-retry.test.ts b/packages/client/tests/unit/should-retry.test.ts index 1239cfe..fe0f7bc 100644 --- a/packages/client/tests/unit/should-retry.test.ts +++ b/packages/client/tests/unit/should-retry.test.ts @@ -41,6 +41,7 @@ function createParams( attempt: number; method: RequestMethod; retry: Required; + idempotencyKey?: string; error: unknown; }> = {}, ) { @@ -120,21 +121,41 @@ describe('shouldRetry', () => { ).toBe(false); }); - it('retries POST when explicitly allowed', () => { + it('retries POST when explicitly allowed and idempotencyKey is provided', () => { expect( shouldRetry( createParams({ method: 'POST', + idempotencyKey: 'idem-123', retry: { ...defaultRetry, - retryMethods: ['GET', 'PUT', 'DELETE', 'POST'], + attempts: 3, + retryMethods: ['POST'], + retryOn: ['network-error'], }, - error: new NetworkError(), + error: new NetworkError('Network request failed', new Error('socket hang up')), }), ), ).toBe(true); }); + it('does not retry POST without idempotencyKey even when explicitly allowed', () => { + expect( + shouldRetry( + createParams({ + method: 'POST', + retry: { + ...defaultRetry, + attempts: 3, + retryMethods: ['POST'], + retryOn: ['network-error'], + }, + error: new NetworkError('Network request failed', new Error('socket hang up')), + }), + ), + ).toBe(false); + }); + it('does not retry on externally aborted requests', () => { expect( shouldRetry( @@ -154,4 +175,74 @@ describe('shouldRetry', () => { ), ).toBe(false); }); + + it('does not retry POST requests without idempotencyKey', () => { + expect( + shouldRetry( + createParams({ + method: 'POST', + retry: { + ...defaultRetry, + attempts: 3, + retryMethods: ['POST'], + retryOn: ['5xx'], + }, + error: createHttpError(500), + }), + ), + ).toBe(false); + }); + + it('retries POST requests with idempotencyKey when method and condition are allowed', () => { + expect( + shouldRetry( + createParams({ + method: 'POST', + idempotencyKey: 'idem-123', + retry: { + ...defaultRetry, + attempts: 3, + retryMethods: ['POST'], + retryOn: ['5xx'], + }, + error: createHttpError(500), + }), + ), + ).toBe(true); + }); + + it('does not retry PATCH requests without idempotencyKey', () => { + expect( + shouldRetry( + createParams({ + method: 'PATCH', + retry: { + ...defaultRetry, + attempts: 3, + retryMethods: ['PATCH'], + retryOn: ['5xx'], + }, + error: createHttpError(500), + }), + ), + ).toBe(false); + }); + + it('retries PATCH requests with idempotencyKey when method and condition are allowed', () => { + expect( + shouldRetry( + createParams({ + method: 'PATCH', + idempotencyKey: 'idem-123', + retry: { + ...defaultRetry, + attempts: 3, + retryMethods: ['PATCH'], + retryOn: ['5xx'], + }, + error: createHttpError(500), + }), + ), + ).toBe(true); + }); });