From b537f9dc52f9984ec473096d40469f5943ad8593 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 23 May 2026 09:03:22 +0200 Subject: [PATCH 1/2] fix(billing): classify non-transient Stripe errors via rawType + auth/permission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retry short-circuit predicate checked err.type === 'invalid_request_error', but stripe-node sets err.type to the class name (StripeInvalidRequestError) and exposes the wire type on err.rawType — so that string was dead for SDK errors. It also missed StripeAuthenticationError (401) and StripePermissionError (403), which are equally deterministic and wasted the full retry budget. Extract isNonTransientStripeError() in billing.stripe-errors.js: match the SDK class names on .type plus invalid_request_error on .type/.rawType, and wire it into the PI-backfill retry. billing.admin.controller.js left as-is (different HTTP-422 mapping semantics; can adopt the helper later). Closes #3699 --- modules/billing/lib/billing.stripe-errors.js | 30 ++++++++++++ .../services/billing.webhook.service.js | 9 ++-- .../tests/billing.stripe-errors.unit.tests.js | 46 +++++++++++++++++++ 3 files changed, 80 insertions(+), 5 deletions(-) create mode 100644 modules/billing/lib/billing.stripe-errors.js create mode 100644 modules/billing/tests/billing.stripe-errors.unit.tests.js diff --git a/modules/billing/lib/billing.stripe-errors.js b/modules/billing/lib/billing.stripe-errors.js new file mode 100644 index 000000000..8a3741a19 --- /dev/null +++ b/modules/billing/lib/billing.stripe-errors.js @@ -0,0 +1,30 @@ +/** + * Classify Stripe errors for retry / short-circuit decisions. + * + * stripe-node sets `err.type` to the error CLASS NAME (Error.js: `this.type = type || this.constructor.name`, + * and every subclass passes its own name via `super(raw, 'StripeXError')`), while the raw API error type + * string (e.g. `'invalid_request_error'`) lives on `err.rawType`. Unwrapped/raw API error objects instead + * carry the wire type directly on `.type`. Both shapes are handled below. + */ + +const NON_TRANSIENT_STRIPE_ERROR_CLASSES = new Set([ + 'StripeInvalidRequestError', // 400/404 — bad params, deterministic + 'StripeAuthenticationError', // 401 — bad/missing API key, deterministic + 'StripePermissionError', // 403 — key lacks permission for the resource, deterministic +]); + +/** + * True when a Stripe error is deterministic and will never succeed on retry + * (invalid request, authentication, or permission failures). Transient errors + * (api_error/500, connection, rate_limit/429) return false so they keep retrying. + * + * @param {unknown} err + * @returns {boolean} + */ +export function isNonTransientStripeError(err) { + if (!err || typeof err !== 'object') return false; + // SDK-wrapped errors expose the class name on `.type`. + if (NON_TRANSIENT_STRIPE_ERROR_CLASSES.has(err.type)) return true; + // SDK mirrors the wire type on `.rawType`; unwrapped API error objects carry it on `.type`. + return err.rawType === 'invalid_request_error' || err.type === 'invalid_request_error'; +} diff --git a/modules/billing/services/billing.webhook.service.js b/modules/billing/services/billing.webhook.service.js index 2da19412a..0bfac6db9 100644 --- a/modules/billing/services/billing.webhook.service.js +++ b/modules/billing/services/billing.webhook.service.js @@ -15,6 +15,7 @@ import BillingResetService from './billing.reset.service.js'; import billingEvents from '../lib/events.js'; import { SENTINEL_PENDING } from '../lib/billing.constants.js'; import { retryWithBackoff } from '../lib/billing.retry.js'; +import { isNonTransientStripeError } from '../lib/billing.stripe-errors.js'; /** * Treats a stripeSessionId as "unresolved" when absent, empty, or still the @@ -326,11 +327,9 @@ const handleCheckoutPaymentCompleted = async (session) => { { 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', + // Skip retries on deterministic Stripe errors (invalid request / auth / permission) — + // they never succeed on retry and only delay the dead-letter path. See billing.stripe-errors.js. + shouldRetry: (err) => !isNonTransientStripeError(err), }, ); } catch (err) { diff --git a/modules/billing/tests/billing.stripe-errors.unit.tests.js b/modules/billing/tests/billing.stripe-errors.unit.tests.js new file mode 100644 index 000000000..c305b1614 --- /dev/null +++ b/modules/billing/tests/billing.stripe-errors.unit.tests.js @@ -0,0 +1,46 @@ +/** + * Module dependencies. + */ +import { describe, test, expect } from '@jest/globals'; +import { isNonTransientStripeError } from '../lib/billing.stripe-errors.js'; + +/** + * Unit tests for isNonTransientStripeError — deterministic Stripe errors that + * should short-circuit retries (invalid request / authentication / permission), + * across both SDK-wrapped (.type = class name) and raw (.type = wire string) shapes. + */ +describe('isNonTransientStripeError', () => { + test.each(['StripeInvalidRequestError', 'StripeAuthenticationError', 'StripePermissionError'])( + 'returns true for SDK error class %s (err.type = class name)', + (type) => { + expect(isNonTransientStripeError({ type })).toBe(true); + }, + ); + + test('returns true for an SDK-wrapped error carrying rawType invalid_request_error', () => { + expect( + isNonTransientStripeError({ type: 'StripeInvalidRequestError', rawType: 'invalid_request_error' }), + ).toBe(true); + }); + + test('returns true for an unwrapped API error object (type = invalid_request_error)', () => { + expect(isNonTransientStripeError({ type: 'invalid_request_error' })).toBe(true); + }); + + test.each(['StripeAPIError', 'StripeConnectionError', 'StripeRateLimitError'])( + 'returns false for transient SDK error class %s', + (type) => { + expect(isNonTransientStripeError({ type })).toBe(false); + }, + ); + + test('returns false for a generic non-Stripe error', () => { + expect(isNonTransientStripeError(new Error('boom'))).toBe(false); + }); + + test('returns false for null / undefined / non-object', () => { + expect(isNonTransientStripeError(null)).toBe(false); + expect(isNonTransientStripeError(undefined)).toBe(false); + expect(isNonTransientStripeError('invalid_request_error')).toBe(false); + }); +}); From 6c5d51b39fe5a48440797428ab5ddb6dfef7f056 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 23 May 2026 13:54:08 +0200 Subject: [PATCH 2/2] fix(billing): add StripeIdempotencyError to non-transient set + cover rawType branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-push review flagged the general isNonTransientStripeError helper omitted StripeIdempotencyError (400, deterministic — reused key with conflicting params). Add it. StripeCardError (402) stays excluded since some decline codes (processing_error, issuer_unavailable) are transient. Add tests for the rawType-only branch and the card exclusion; drop a mislabeled test. --- modules/billing/lib/billing.stripe-errors.js | 7 +++-- .../tests/billing.stripe-errors.unit.tests.js | 26 ++++++++++++------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/modules/billing/lib/billing.stripe-errors.js b/modules/billing/lib/billing.stripe-errors.js index 8a3741a19..bf4d183d0 100644 --- a/modules/billing/lib/billing.stripe-errors.js +++ b/modules/billing/lib/billing.stripe-errors.js @@ -9,14 +9,17 @@ const NON_TRANSIENT_STRIPE_ERROR_CLASSES = new Set([ 'StripeInvalidRequestError', // 400/404 — bad params, deterministic + 'StripeIdempotencyError', // 400 — idempotency key reused with conflicting params, deterministic 'StripeAuthenticationError', // 401 — bad/missing API key, deterministic 'StripePermissionError', // 403 — key lacks permission for the resource, deterministic ]); /** * True when a Stripe error is deterministic and will never succeed on retry - * (invalid request, authentication, or permission failures). Transient errors - * (api_error/500, connection, rate_limit/429) return false so they keep retrying. + * (invalid request, idempotency, authentication, or permission failures). Transient + * errors (api_error/500, connection, rate_limit/429) return false so they keep + * retrying. StripeCardError (402) is intentionally excluded — some decline codes + * (processing_error, issuer_unavailable) are transient. * * @param {unknown} err * @returns {boolean} diff --git a/modules/billing/tests/billing.stripe-errors.unit.tests.js b/modules/billing/tests/billing.stripe-errors.unit.tests.js index c305b1614..acb2a3f82 100644 --- a/modules/billing/tests/billing.stripe-errors.unit.tests.js +++ b/modules/billing/tests/billing.stripe-errors.unit.tests.js @@ -10,17 +10,19 @@ import { isNonTransientStripeError } from '../lib/billing.stripe-errors.js'; * across both SDK-wrapped (.type = class name) and raw (.type = wire string) shapes. */ describe('isNonTransientStripeError', () => { - test.each(['StripeInvalidRequestError', 'StripeAuthenticationError', 'StripePermissionError'])( - 'returns true for SDK error class %s (err.type = class name)', - (type) => { - expect(isNonTransientStripeError({ type })).toBe(true); - }, - ); + test.each([ + 'StripeInvalidRequestError', + 'StripeIdempotencyError', + 'StripeAuthenticationError', + 'StripePermissionError', + ])('returns true for SDK error class %s (err.type = class name)', (type) => { + expect(isNonTransientStripeError({ type })).toBe(true); + }); - test('returns true for an SDK-wrapped error carrying rawType invalid_request_error', () => { - expect( - isNonTransientStripeError({ type: 'StripeInvalidRequestError', rawType: 'invalid_request_error' }), - ).toBe(true); + test('returns true via the rawType branch when the class is not listed', () => { + expect(isNonTransientStripeError({ type: 'StripeFutureUnknownError', rawType: 'invalid_request_error' })).toBe( + true, + ); }); test('returns true for an unwrapped API error object (type = invalid_request_error)', () => { @@ -34,6 +36,10 @@ describe('isNonTransientStripeError', () => { }, ); + test('returns false for StripeCardError (402 — some decline codes are transient)', () => { + expect(isNonTransientStripeError({ type: 'StripeCardError' })).toBe(false); + }); + test('returns false for a generic non-Stripe error', () => { expect(isNonTransientStripeError(new Error('boom'))).toBe(false); });