Context
Follow-up nits from /critical-review on PR #3697 (retryWithBackoff short-circuit). Both LOW, non-blocking — the merged predicate is correct for the common case; these refine it.
Findings
1. invalid_request_error string is effectively dead code on err.type
modules/billing/services/billing.webhook.service.js call-site predicate:
shouldRetry: (err) => err?.type !== 'StripeInvalidRequestError' && err?.type !== 'invalid_request_error'
In stripe-node, StripeError.type resolves to the constructor name ('StripeInvalidRequestError'); the raw API type string ('invalid_request_error') lives on err.rawType. So the second condition never matches a real SDK error — only hand-crafted plain objects. Harmless (never false-positives) but misleading.
Same pattern in modules/billing/controllers/billing.admin.controller.js:61.
Fix: match err.rawType === 'invalid_request_error' for the API-string path (or drop it and rely on the constructor-name check).
2. Auth/permission errors are also non-transient
StripeAuthenticationError (401) and StripePermissionError (403) are deterministic — a bad/over-scoped API key never succeeds on retry. Currently they burn all 3 attempts (~600ms) before the same dead-letter outcome. Add them to the non-transient set.
Acceptance
- Predicate matches the API type via
err.rawType, not the dead err.type string
StripeAuthenticationError / StripePermissionError short-circuit (1 call, not 3)
- Unit tests cover the added types
- Optional: extract a shared
isNonTransientStripeError(err) helper (rule of three — webhook service + admin controller both classify)
Source
/critical-review on PR #3697, 2026-05-22.
Context
Follow-up nits from
/critical-reviewon PR #3697 (retryWithBackoff short-circuit). Both LOW, non-blocking — the merged predicate is correct for the common case; these refine it.Findings
1.
invalid_request_errorstring is effectively dead code onerr.typemodules/billing/services/billing.webhook.service.jscall-site predicate:In stripe-node,
StripeError.typeresolves to the constructor name ('StripeInvalidRequestError'); the raw API type string ('invalid_request_error') lives onerr.rawType. So the second condition never matches a real SDK error — only hand-crafted plain objects. Harmless (never false-positives) but misleading.Same pattern in
modules/billing/controllers/billing.admin.controller.js:61.Fix: match
err.rawType === 'invalid_request_error'for the API-string path (or drop it and rely on the constructor-name check).2. Auth/permission errors are also non-transient
StripeAuthenticationError(401) andStripePermissionError(403) are deterministic — a bad/over-scoped API key never succeeds on retry. Currently they burn all 3 attempts (~600ms) before the same dead-letter outcome. Add them to the non-transient set.Acceptance
err.rawType, not the deaderr.typestringStripeAuthenticationError/StripePermissionErrorshort-circuit (1 call, not 3)isNonTransientStripeError(err)helper (rule of three — webhook service + admin controller both classify)Source
/critical-reviewon PR #3697, 2026-05-22.