fix(billing): V8.1 post-merge hardening#3629
Conversation
F1: wrap syncOrganizationPlan in try/catch in handleInvoicePaymentSucceeded —
DB blip no longer dead-letters handler; emits billing.organization.sync_failed
F2: prepend '~' to synthetic event IDs in bumpEventMarkers — same-second lex
tiebreaker now sorts synthetic bumps after Stripe evt_* IDs
F3: validatePlan warns on unrecognized non-empty planId before returning null
F4: add 'unpaid' to RECONCILE_STATUSES so reconciler covers unpaid subs
F5: add V8-C2b — canceled subscription in meter mode must be fail-closed
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughEvent marker IDs are prefixed with ChangesBilling Event Markers and Reconciliation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
Hardening pass for the billing module (V8.1) focused on webhook resilience/observability, reconciliation coverage, and event-ordering marker behavior, with accompanying unit test updates.
Changes:
- Add warning logs for unrecognized Stripe plan metadata and make
syncOrganizationPlanfailures non-fatal in the webhook flow. - Include
unpaidsubscriptions in the Stripe reconciliation scan. - Adjust synthetic event marker IDs and update/add unit tests (including a new canceled-subscription meter-mode regression test).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/billing/services/billing.webhook.service.js | Adds warn logging for unknown plan IDs; makes org-plan sync failures non-fatal with additional observability. |
| modules/billing/services/billing.reconcile.service.js | Expands reconciliation statuses to include unpaid. |
| modules/billing/lib/billing.markerBump.js | Changes synthetic event marker ID format to include a prefix intended to affect lexicographic tiebreaking. |
| modules/billing/tests/billing.subscription.repository.unit.tests.js | Updates expectations for marker ID format used by direct-write subscription updates. |
| modules/billing/tests/billing.admin.service.unit.tests.js | Updates expectations for marker ID format used by admin cancellation. |
| modules/billing/tests/billing.quota.unit.tests.js | Adds regression test ensuring canceled subscriptions fail-closed to free quota in meter mode. |
| return { | ||
| [`${fieldPrefix}CreatedAt`]: Math.floor(ms / 1000), | ||
| [`${fieldPrefix}Id`]: `${source}-${ms}`, | ||
| [`${fieldPrefix}Id`]: `~${source}-${ms}`, |
| } catch (syncErr) { | ||
| logger.error('[billing.webhook] syncOrganizationPlan failed (non-fatal)', { | ||
| organizationId, | ||
| error: syncErr?.message ?? String(syncErr), |
| billingEvents.emit('billing.organization.sync_failed', { organizationId, source: 'dunning_recovery' }); | ||
| } catch (evtErr) { | ||
| logger.error('[billing.webhook] billing.organization.sync_failed listener error (non-fatal)', { | ||
| error: evtErr?.message ?? String(evtErr), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 608-622: The two new logger.error calls inside the
syncOrganizationPlan catch block are missing the error stack; update the first
logger.error (inside catch(syncErr)) to include stack: syncErr?.stack (in
addition to error: syncErr?.message ?? String(syncErr)), and update the second
logger.error (inside catch(evtErr)) to include stack: evtErr?.stack (in addition
to error: evtErr?.message ?? String(evtErr)); locate these in the
billing.webhook.service.js surrounding the syncOrganizationPlan(organizationId,
resolvedPlan) call and the
billingEvents.emit('billing.organization.sync_failed', ...) listener catch and
add the stack fields to match the file's existing logging pattern.
In `@modules/billing/tests/billing.quota.unit.tests.js`:
- Around line 625-647: The test "V8-C2b: canceled subscription in meter mode →
402 METER_EXHAUSTED with free quota (not paid)" currently sets
mockBillingUsageService.getMeter.mockResolvedValue(null) but later asserts
getMeter is never called; add a one-line inline comment next to the
mockBillingUsageService.getMeter.mockResolvedValue(null) setup explaining this
is a defensive/no-op stub to mirror V8-C2 and to prevent accidental test
breakage if call paths change, referencing the mockBillingUsageService.getMeter
and the test name so future readers understand why the mock exists despite the
expect(mockBillingUsageService.getMeter).not.toHaveBeenCalled() assertion.
🪄 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: 0719e308-d673-4521-a39d-017749023689
📒 Files selected for processing (6)
modules/billing/lib/billing.markerBump.jsmodules/billing/services/billing.reconcile.service.jsmodules/billing/services/billing.webhook.service.jsmodules/billing/tests/billing.admin.service.unit.tests.jsmodules/billing/tests/billing.quota.unit.tests.jsmodules/billing/tests/billing.subscription.repository.unit.tests.js
| test('V8-C2b: canceled subscription in meter mode → 402 METER_EXHAUSTED with free quota (not paid)', async () => { | ||
| // Org has status='canceled' (subscription ended). Paid plan must NOT bleed through. | ||
| // Fail-closed branch routes to free plan, same as incomplete. | ||
| mockSubscriptionRepository.findByOrganization.mockResolvedValue({ plan: 'pro', status: 'canceled' }); | ||
| mockBillingUsageService.getMeter.mockResolvedValue(null); | ||
| mockBillingExtraBalanceRepository.getBalance.mockResolvedValue(0); | ||
| mockBillingPlanService.getActivePlan.mockReturnValue({ meterQuota: 0, version: 'v1' }); | ||
|
|
||
| await requireQuota('scraps', 'create')(req, res, next); | ||
|
|
||
| expect(next).not.toHaveBeenCalled(); | ||
| expect(res.status).toHaveBeenCalledWith(402); | ||
| const payload = res.json.mock.calls[0][0]; | ||
| const errData = JSON.parse(payload.error); | ||
| expect(errData.type).toBe('METER_EXHAUSTED'); | ||
| // Free plan quota (0), not the pro paid quota | ||
| expect(errData.meterQuota).toBe(0); | ||
| // getActivePlan called with the free/default plan id, not 'pro' | ||
| expect(mockBillingPlanService.getActivePlan).toHaveBeenCalledWith('free'); | ||
| expect(mockBillingPlanService.getActivePlan).not.toHaveBeenCalledWith('pro'); | ||
| // Fail-closed branch must short-circuit before the meter check | ||
| expect(mockBillingUsageService.getMeter).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
LGTM — V8-C2b correctly mirrors V8-C2 for canceled status.
The test structure, mock setup, and assertions are consistent with the existing V8-C2 (incomplete) test at lines 600–623. The fail-closed invariants (getMeter not called, getActivePlan called with 'free', quota=0 → 402 METER_EXHAUSTED) are all correctly verified.
One minor readability note: Line 629 sets up mockBillingUsageService.getMeter.mockResolvedValue(null) as a defensive safety net, but Line 646 asserts getMeter is never called. A brief inline comment explaining this is intentional defensive setup (as in V8-C2) would prevent future readers from removing the mock setup thinking it's dead code.
💡 Optional: add clarifying comment on the getMeter mock
mockSubscriptionRepository.findByOrganization.mockResolvedValue({ plan: 'pro', status: 'canceled' });
- mockBillingUsageService.getMeter.mockResolvedValue(null);
+ mockBillingUsageService.getMeter.mockResolvedValue(null); // defensive: fail-closed branch must short-circuit before this is reached
mockBillingExtraBalanceRepository.getBalance.mockResolvedValue(0);🤖 Prompt for 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.
In `@modules/billing/tests/billing.quota.unit.tests.js` around lines 625 - 647,
The test "V8-C2b: canceled subscription in meter mode → 402 METER_EXHAUSTED with
free quota (not paid)" currently sets
mockBillingUsageService.getMeter.mockResolvedValue(null) but later asserts
getMeter is never called; add a one-line inline comment next to the
mockBillingUsageService.getMeter.mockResolvedValue(null) setup explaining this
is a defensive/no-op stub to mirror V8-C2 and to prevent accidental test
breakage if call paths change, referencing the mockBillingUsageService.getMeter
and the test name so future readers understand why the mock exists despite the
expect(mockBillingUsageService.getMeter).not.toHaveBeenCalled() assertion.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3629 +/- ##
==========================================
+ Coverage 88.86% 88.89% +0.02%
==========================================
Files 135 135
Lines 4635 4645 +10
Branches 1429 1432 +3
==========================================
+ Hits 4119 4129 +10
Misses 403 403
Partials 113 113
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:
|
…re path Add stack: err?.stack to both logger.error calls in the syncOrganizationPlan catch block to match the convention used throughout billing.webhook.service.js. Add unit test covering the dunning recovery setPlan failure path: asserts non-fatal behaviour, logger.error call, and billing.organization.sync_failed emit.
…r codecov/patch Add two more unit tests to close remaining uncovered lines in the V8.1 patch: - sync_failed listener throws: asserts inner evtErr catch is non-fatal and logs correctly - validatePlan warns on unrecognized non-empty planId (e.g. Stripe product ID)
PierreBrisorgueil
dismissed
coderabbitai[bot]’s stale review
Both actionable issues fixed: stack fields added to both logger.error calls, and full patch coverage added (syncOrganizationPlan failure, evtErr inner catch, validatePlan warn branch). CI green + codecov/patch passing.
Summary
Post-merge hardening pass (V8.1) on the billing module, cross-validated by Sonnet code-reviewer + DeepSeek V4-pro independent review. Addresses 5 issues found in the V8 audit that were not part of the original V8 PRs.
Fixes
syncOrganizationPlan(billing.webhook.service.js): wrap the dunning-recovery call in a non-fatal try/catch. A transient org-sync failure was previously propagating and returning 500 to Stripe, triggering unnecessary redeliveries and masking the real error.~prefix (billing.markerBump.js): prefix synthetic event IDs with~(ASCII 126, sorts afterz). Ensures direct-write markers (admin overrides, dunning sweep) always lose the lex tiebreaker to any real Stripeevt_*re-delivery within the same second — so Stripe ground-truth can always overwrite stale synthetic state.validatePlanwarn on unrecognized planId (billing.webhook.service.js): replace silent null-return with a structuredlogger.warnwhen a raw plan metadata value is not in the known-plans set. Surfaces Stripe metadata misconfiguration in observability instead of silently downgrading.unpaidadded toRECONCILE_STATUSES(billing.reconcile.service.js):unpaidis a valid Stripe subscription status (already inbilling.statusesconfig enum + Mongoose model + Zod schema). Excluding it from reconciliation leftunpaidsubscriptions invisible to the divergence detector.billing.quota.unit.tests.js): new unit test V8-C2b asserting that acanceledsubscription in meter mode routes to the fail-closed free-plan branch and never bleeds through paid-plan quota.Test plan
~prefix)getMeternot calledfix/billing-v8-hardeningSummary by CodeRabbit
New Features
Bug Fixes
Tests