fix(billing): classify non-transient Stripe errors via rawType + auth/permission#3700
Merged
Merged
Conversation
…/permission 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
… rawType branch 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.
|
Caution Review failedFailed to post review comments WalkthroughA new Stripe error classification helper ( ChangesStripe Error Classification and Retry Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refines billing’s Stripe retry behavior by centralizing and expanding “non-transient Stripe error” detection, ensuring deterministic failures (bad params/auth/permission/idempotency) short-circuit retryWithBackoff instead of burning retry budget.
Changes:
- Add
isNonTransientStripeError(err)helper that correctly distinguishes Stripe SDK class-name.typefrom wire-type.rawType/ raw-object.type. - Wire the helper into the PaymentIntent metadata backfill retry predicate in
billing.webhook.service.js. - Add unit tests covering SDK-class, rawType, raw-object, transient-class, card-error exclusion, and guard cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| modules/billing/lib/billing.stripe-errors.js | Introduces a shared helper to classify deterministic (non-transient) Stripe errors across SDK and raw error shapes. |
| modules/billing/services/billing.webhook.service.js | Uses the shared helper to short-circuit retries for deterministic Stripe errors during PI metadata backfill. |
| modules/billing/tests/billing.stripe-errors.unit.tests.js | Adds unit coverage for the classifier, including rawType handling and non-Stripe inputs. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3700 +/- ##
==========================================
+ Coverage 89.72% 89.73% +0.01%
==========================================
Files 142 143 +1
Lines 4789 4794 +5
Branches 1503 1505 +2
==========================================
+ Hits 4297 4302 +5
Misses 385 385
Partials 107 107
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #3697. Extracts
isNonTransientStripeError(err)intomodules/billing/lib/billing.stripe-errors.jsand wires it into the webhook PI-backfill retry (shouldRetry: (err) => !isNonTransientStripeError(err)).Fixes two issues from the
/critical-reviewof #3697 (filed as #3699):err.rawTypevserr.type— stripe-node setserr.typeto the class name (StripeInvalidRequestError); the wire string (invalid_request_error) lives onerr.rawType. Verified innode_modules/stripe/cjs/Error.js(this.type = type || this.constructor.name; each subclasssuper(raw, 'StripeXError')). The olderr.type === 'invalid_request_error'was dead for SDK errors.StripeAuthenticationError(401) +StripePermissionError(403) are equally deterministic; now short-circuited.The classifier keys on the SDK class name (
.type) for the non-transient set plusinvalid_request_erroron.type/.rawTypefor unwrapped objects.Scope notes
StripeIdempotencyError(400) included (deterministic).StripeCardError(402) excluded — some decline codes (processing_error,issuer_unavailable) are transient.billing.admin.controller.jsleft as-is (different HTTP-422 mapping semantics +charge_already_refunded; can adopt the helper later).Validation
npm run test:unit— 1521/1521 green (newbilling.stripe-errors.unit.tests.js: class names, rawType branch, raw-object, transient classes false, card excluded, null/non-object guards)Closes #3699
Summary by CodeRabbit
Bug Fixes
Tests