diff --git a/modules/billing/lib/billing.retry.js b/modules/billing/lib/billing.retry.js index a238157bd..d5448869b 100644 --- a/modules/billing/lib/billing.retry.js +++ b/modules/billing/lib/billing.retry.js @@ -6,27 +6,37 @@ * each non-final attempt i. * * Returns the result of the first successful call, or throws the last - * error after all attempts are exhausted. + * error after all attempts are exhausted. If `shouldRetry(err)` returns + * false for a thrown error, the error is rethrown immediately without + * further attempts or delay. * * @param {() => Promise} fn - Async function to attempt. * @param {object} [opts] * @param {number} [opts.attempts=3] - Maximum number of attempts (including the first call). * @param {number} [opts.baseMs=200] - Base delay in ms for the first retry. + * @param {(err: unknown) => boolean} [opts.shouldRetry] - Predicate; return false to stop retrying for a given error. Defaults to always retry. * @returns {Promise} */ -export async function retryWithBackoff(fn, { attempts = 3, baseMs = 200 } = {}) { +export async function retryWithBackoff(fn, { attempts = 3, baseMs = 200, shouldRetry = () => true } = {}) { if (!Number.isInteger(attempts) || attempts < 1) { throw new TypeError(`retryWithBackoff: attempts must be a positive integer, received ${attempts}`); } if (!Number.isFinite(baseMs) || baseMs < 0) { throw new TypeError(`retryWithBackoff: baseMs must be a non-negative finite number, received ${baseMs}`); } + if (typeof shouldRetry !== 'function') { + throw new TypeError(`retryWithBackoff: shouldRetry must be a function, received ${typeof shouldRetry}`); + } let lastErr; for (let i = 0; i < attempts; i++) { try { return await fn(); } catch (err) { lastErr = err; + // Deterministic errors never succeed on retry — rethrow now instead of burning the backoff budget. + if (!shouldRetry(err)) { + throw err; + } if (i < attempts - 1) { await new Promise((resolve) => setTimeout(resolve, baseMs * 2 ** i)); } diff --git a/modules/billing/services/billing.webhook.service.js b/modules/billing/services/billing.webhook.service.js index 987edc874..2da19412a 100644 --- a/modules/billing/services/billing.webhook.service.js +++ b/modules/billing/services/billing.webhook.service.js @@ -320,14 +320,22 @@ const handleCheckoutPaymentCompleted = async (session) => { organizationId, packId, kind: 'extras', - stripeSessionId, // real cs_* ID (replaces SENTINEL_PENDING) + stripeSessionId, // real cs_* ID (replaces SENTINEL_PENDING) }, }), - { attempts: 3, baseMs: 200 }, + { + attempts: 3, + baseMs: 200, + // Skip retries on Stripe invalid-request errors (invalid_request_error / + // StripeInvalidRequestError) — these are deterministic client errors that never + // succeed on retry and only delay the dead-letter path. + shouldRetry: (err) => + err?.type !== 'StripeInvalidRequestError' && err?.type !== 'invalid_request_error', + }, ); } catch (err) { logger.error( - '[billing.webhook] PI metadata backfill failed after retries — refund correlation at risk', + '[billing.webhook] PI metadata backfill failed (retries exhausted or skipped) — refund correlation at risk', { paymentIntentId, stripeSessionId, error: err?.message ?? String(err), stack: err?.stack }, ); try { diff --git a/modules/billing/tests/billing.refund-correlation.unit.tests.js b/modules/billing/tests/billing.refund-correlation.unit.tests.js index 2593e2aad..e9b0fc796 100644 --- a/modules/billing/tests/billing.refund-correlation.unit.tests.js +++ b/modules/billing/tests/billing.refund-correlation.unit.tests.js @@ -158,7 +158,7 @@ describe('checkout.session.completed — metadata backfill retry:', () => { // logger.error should be called for the final failure. expect(mockLogger.error).toHaveBeenCalledWith( - expect.stringContaining('backfill failed after retries'), + expect.stringContaining('backfill failed (retries exhausted or skipped)'), expect.objectContaining({ paymentIntentId }), ); @@ -185,7 +185,7 @@ describe('checkout.session.completed — metadata backfill retry:', () => { // Both error paths should be logged. expect(mockLogger.error).toHaveBeenCalledTimes(2); expect(mockLogger.error).toHaveBeenCalledWith( - expect.stringContaining('backfill failed after retries'), + expect.stringContaining('backfill failed (retries exhausted or skipped)'), expect.objectContaining({ paymentIntentId }), ); expect(mockLogger.error).toHaveBeenCalledWith( diff --git a/modules/billing/tests/billing.retry.unit.tests.js b/modules/billing/tests/billing.retry.unit.tests.js new file mode 100644 index 000000000..49366f4da --- /dev/null +++ b/modules/billing/tests/billing.retry.unit.tests.js @@ -0,0 +1,93 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, afterEach, expect } from '@jest/globals'; +import { retryWithBackoff } from '../lib/billing.retry.js'; + +/** + * Unit tests for retryWithBackoff: + * - success / transient-retry / exhaustion paths + * - shouldRetry short-circuit on non-transient (deterministic) errors + * - argument validation + * Retry-delay paths use fake timers so the suite never incurs real backoff waits. + */ +describe('retryWithBackoff', () => { + afterEach(() => { + jest.useRealTimers(); + }); + + test('returns the result on first success without retrying', async () => { + const fn = jest.fn().mockResolvedValue('ok'); + await expect(retryWithBackoff(fn)).resolves.toBe('ok'); + expect(fn).toHaveBeenCalledTimes(1); + }); + + test('retries a transient failure then resolves', async () => { + jest.useFakeTimers(); + const fn = jest.fn().mockRejectedValueOnce(new Error('boom')).mockResolvedValueOnce('ok'); + const promise = retryWithBackoff(fn, { attempts: 3, baseMs: 200 }); + await jest.runAllTimersAsync(); + await expect(promise).resolves.toBe('ok'); + expect(fn).toHaveBeenCalledTimes(2); + }); + + test('throws the last error after exhausting all attempts', async () => { + jest.useFakeTimers(); + const fn = jest.fn().mockRejectedValue(new Error('always fails')); + const promise = retryWithBackoff(fn, { attempts: 3, baseMs: 200 }); + const assertion = expect(promise).rejects.toThrow('always fails'); + await jest.runAllTimersAsync(); + await assertion; + expect(fn).toHaveBeenCalledTimes(3); + }); + + test('short-circuits (1 call, not 3) when shouldRetry returns false', async () => { + const err = Object.assign(new Error('bad params'), { type: 'invalid_request_error' }); + const fn = jest.fn().mockRejectedValue(err); + await expect( + retryWithBackoff(fn, { + attempts: 3, + baseMs: 200, + shouldRetry: (e) => e?.type !== 'StripeInvalidRequestError' && e?.type !== 'invalid_request_error', + }), + ).rejects.toThrow('bad params'); + expect(fn).toHaveBeenCalledTimes(1); + }); + + test('short-circuits on the StripeInvalidRequestError class-name type too', async () => { + // stripe-node exposes the error class name on .type in some SDK versions and the + // raw API type string in others; the call-site predicate guards both, so cover both. + const err = Object.assign(new Error('bad params'), { type: 'StripeInvalidRequestError' }); + const fn = jest.fn().mockRejectedValue(err); + await expect( + retryWithBackoff(fn, { + attempts: 3, + baseMs: 200, + shouldRetry: (e) => e?.type !== 'StripeInvalidRequestError' && e?.type !== 'invalid_request_error', + }), + ).rejects.toThrow('bad params'); + expect(fn).toHaveBeenCalledTimes(1); + }); + + test('retries to exhaustion when shouldRetry returns true', async () => { + jest.useFakeTimers(); + const fn = jest.fn().mockRejectedValue(new Error('transient')); + const promise = retryWithBackoff(fn, { + attempts: 3, + baseMs: 200, + shouldRetry: () => true, + }); + const assertion = expect(promise).rejects.toThrow('transient'); + await jest.runAllTimersAsync(); + await assertion; + expect(fn).toHaveBeenCalledTimes(3); + }); + + test('rejects invalid attempts / baseMs / shouldRetry with TypeError', async () => { + const fn = jest.fn().mockResolvedValue('ok'); + await expect(retryWithBackoff(fn, { attempts: 0 })).rejects.toThrow(TypeError); + await expect(retryWithBackoff(fn, { baseMs: -1 })).rejects.toThrow(TypeError); + await expect(retryWithBackoff(fn, { shouldRetry: 'nope' })).rejects.toThrow(TypeError); + expect(fn).not.toHaveBeenCalled(); + }); +});