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

[PierreBrisorgueil]

Summary

• Add optional shouldRetry(err) predicate to retryWithBackoff (defaults to always-retry — backward-compatible for all existing callers).
• Wire a Stripe non-transient classifier at the PI-metadata-backfill call site: StripeInvalidRequestError / invalid_request_error short-circuits straight to the dead-letter path instead of burning ~600ms of backoff.
• New billing.retry.unit.tests.js: success / transient-retry / exhaustion / short-circuit (both type spellings) / arg validation — fake timers, no real delay.
• The err.stack finding from fix(billing): refine retryWithBackoff — short-circuit on non-transient Stripe errors + restore err.stack #3691 is already satisfied by fix(billing): retry refund correlation backfill 3x + dead-letter #3690 (both logger.error calls include stack) — no change needed here.

Closes #3691

Scope

• Module(s) impacted: modules/billing/lib/billing.retry.js, modules/billing/services/billing.webhook.service.js, modules/billing/tests/billing.retry.unit.tests.js
• Cross-module impact: none — retryWithBackoff is only called in billing.webhook.service.js; new param is optional and defaults to prior behaviour
• Risk level: low

Validation

• npm run lint
• npm run test:unit — 1509/1509 green (incl. 7 new retry tests, sub-second with fake timers)
• Short-circuit asserts 1 Stripe call (not 3) for both invalid_request_error and StripeInvalidRequestError

Guardrails check

• No secrets or credentials introduced
• No risky rename/move of core stack paths
• Changes remain merge-friendly for downstream projects (new shouldRetry param is optional with backward-compatible default)
• Tests added for the new behaviour

Notes for reviewers

• Security considerations: none — this only affects retry loop behaviour, not auth or data paths
• Mergeability considerations: no conflict risk; only billing module touched; downstream projects use retryWithBackoff via the default (always-retry) path so zero propagation needed
• Follow-up tasks: none

[coderabbitai]

Walkthrough

retryWithBackoff utility gains a shouldRetry predicate option (default true) to short-circuit retries on non-transient errors. Integration into webhook PaymentIntent backfill applies predicate to skip retries on Stripe invalid_request_error. Comprehensive Jest test suite covers all paths including validation and error-type detection.

Changes

Retry predicate enhancement for deterministic Stripe error short-circuiting

Layer / File(s)	Summary
Core retry function: shouldRetry predicate and validation modules/billing/lib/billing.retry.js	retryWithBackoff signature extended with shouldRetry option (defaults to always-true predicate). JSDoc updated to document the predicate and error-rethrow behavior. Function validates that shouldRetry is callable; retry loop checks predicate after each error and rethrows immediately when predicate returns false, skipping remaining attempts and backoff delays.
Webhook PaymentIntent backfill: apply shouldRetry for Stripe errors modules/billing/services/billing.webhook.service.js	handleCheckoutPaymentCompleted passes shouldRetry predicate to retryWithBackoff during PaymentIntent metadata update to prevent retry on StripeInvalidRequestError (invalid_request_error type). Existing retry attempt count and dead-letter fallback flow unchanged.
Comprehensive test suite for retryWithBackoff modules/billing/tests/billing.retry.unit.tests.js	Jest unit tests verify success without retry, transient failure recovery through retry, exhaustion failure, deterministic short-circuit when shouldRetry returns false, Stripe invalid-request-error type detection and short-circuit, input validation that rejects non-function shouldRetry with TypeError, and proper cleanup with fake timers.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

• #3691: Documents the exact findings and acceptance criteria that this PR implements—short-circuit on invalid_request_error, test coverage for the predicate short-circuit path, and error handling behavior.

• #3689: Requests switching affected webhook test to fake timers to avoid retry-induced delays, which is addressed by the test suite's use of Jest fake timers in billing.retry.unit.tests.js.

Possibly related PRs

• pierreb-devkit/Node#3690: Both PRs modify the same refund-correlation PaymentIntent metadata backfill flow by extending/using retryWithBackoff in modules/billing/lib/billing.retry.js and wiring it into modules/billing/services/billing.webhook.service.js (main PR adds shouldRetry short-circuiting for deterministic Stripe errors).

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately summarizes the main change: adding a short-circuit mechanism to retryWithBackoff for non-transient Stripe errors.
Linked Issues check	✅ Passed	The PR addresses all core requirements from #3691: adds shouldRetry predicate to retryWithBackoff, wires Stripe error classifier to short-circuit non-transient errors, and includes unit tests covering both Stripe error type spellings and short-circuit validation.
Out of Scope Changes check	✅ Passed	The PR description notes that err.stack concern is already addressed by #3690, so no changes to logger.error are included. All modifications directly support the short-circuit mechanism for non-transient Stripe errors, staying focused on the stated objective.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	PR description includes all required template sections: Summary, Scope, Validation, Guardrails, and Notes for reviewers. All checkboxes are marked complete and rationale is provided.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/billing-retry-shortcircuit

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.72%. Comparing base (3df0034) to head (3c27a01).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3697      +/-   ##
==========================================
+ Coverage   89.71%   89.72%   +0.01%     
==========================================
  Files         142      142              
  Lines        4784     4789       +5     
  Branches     1500     1503       +3     
==========================================
+ Hits         4292     4297       +5     
  Misses        385      385              
  Partials      107      107              

Flag	Coverage Δ
integration	59.46% <0.00%> (-0.07%)	⬇️
unit	66.21% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3df0034...3c27a01. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

This PR refines the billing retry utility to allow callers to short-circuit retries on non-transient errors, and applies that behavior to the Stripe PaymentIntent metadata backfill path to avoid unnecessary backoff delays on deterministic Stripe client failures.

Changes:

• Extend retryWithBackoff to accept an optional shouldRetry(err) predicate (default: always retry) with argument validation.
• Wire a Stripe invalid-request classifier into the webhook backfill call site to skip retries for invalid_request_error / StripeInvalidRequestError.
• Add a dedicated billing.retry.unit.tests.js suite covering success, transient retry, exhaustion, short-circuit behavior, and validation (using fake timers).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
modules/billing/lib/billing.retry.js	Adds shouldRetry predicate support + validation to the backoff retry utility.
modules/billing/services/billing.webhook.service.js	Uses shouldRetry to short-circuit retries for deterministic Stripe invalid-request errors during PI metadata backfill.
modules/billing/tests/billing.retry.unit.tests.js	New unit tests validating retry/short-circuit/validation behavior without real timer delays.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@modules/billing/services/billing.webhook.service.js`:
- Around line 326-334: The failure log wording is inaccurate when the new
shouldRetry predicate short-circuits retries; update the logger call that
currently reads like "failed after retries" to reflect whether retries were
attempted or skipped by shouldRetry. Locate the shouldRetry predicate and the
corresponding error logging in the webhook processing (the block that logs
failures after the retry policy) and change the message to distinguish two
cases: (a) retries were skipped due to a deterministic Stripe error (include
err.type or a phrase like "retries skipped - deterministic error"), and (b)
retries were attempted and exhausted (keep "failed after retries"); include the
error type/details in both logs for clarity.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5a7896ea-9879-4efb-ab20-e885823f6e36

📥 Commits

Reviewing files that changed from the base of the PR and between 3df0034 and 46efee0.

📒 Files selected for processing (3)

• modules/billing/lib/billing.retry.js
• modules/billing/services/billing.webhook.service.js
• modules/billing/tests/billing.retry.unit.tests.js