fix(billing): short-circuit retryWithBackoff on non-transient Stripe errors (#3697)

* fix(billing): short-circuit retryWithBackoff on non-transient Stripe errors

retryWithBackoff retried every error type, wasting ~600ms of backoff on
deterministic StripeInvalidRequestError before dead-lettering. Add an
optional shouldRetry(err) predicate (default: always retry) and pass a
non-transient classifier at the PI-backfill call site.

The err.stack finding from the issue is already satisfied by #3690
(both logger.error calls include stack) — no change needed.

Closes #3691

* test(billing): cover StripeInvalidRequestError class-name branch in retry short-circuit

The shouldRetry predicate guards both Stripe error type spellings
(StripeInvalidRequestError class name + invalid_request_error raw type,
which vary by stripe-node version). Add a parallel short-circuit test for
the class-name value and align the existing test's predicate with the
production one. Closes a coverage gap flagged by the pre-push gate.

* fix(billing): address reviewer nits — narrow comment + improve log wording

- Narrow shouldRetry JSDoc comment to only mention invalid_request_error
  (not auth errors) — matches what the predicate actually checks
  (Copilot thread PRRT_kwDOBss37M6EM5HL)
- Change failure log from "failed after retries" to "failed (retries
  exhausted or skipped)" to reflect short-circuit path
  (CodeRabbit thread PRRT_kwDOBss37M6EM5c1)
- Update billing.refund-correlation tests to match new log wording

Author: PierreBrisorgueil

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<T>} 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<T>}
  */
-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));
       }

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 {

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(

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