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

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,10 +320,18 @@ 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 deterministic Stripe client errors (bad params / auth) — they
+            // never succeed on retry and only delay the dead-letter path. Same classification
+            // as billing.admin.controller.js.
+            shouldRetry: (err) =>
+              err?.type !== 'StripeInvalidRequestError' && err?.type !== 'invalid_request_error',
+          },
         );
       } catch (err) {
         logger.error(

modules/billing/tests/billing.retry.unit.tests.js

@@ -0,0 +1,78 @@
+/**
+ * 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 !== '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();
+  });
+});