Skip to content

fix(billing): V8.1 post-merge hardening #3629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PierreBrisorgueil merged 3 commits into from
May 7, 2026
Merged

fix(billing): V8.1 post-merge hardening #3629

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e76019b
fix(billing): v8.1 post-merge hardening
PierreBrisorgueil May 7, 2026
b5cfb57
fix(billing): include error stacks + cover syncOrganizationPlan failu…
PierreBrisorgueil May 7, 2026
0c8604e
test(billing): cover evtErr inner catch + validatePlan warn branch fo…
PierreBrisorgueil May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/billing/lib/billing.markerBump.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ export const bumpEventMarkers = (family, source) => {
const fieldPrefix = family === 'invoice' ? 'lastInvoiceEvent' : 'lastSubscriptionEvent';
return {
[`${fieldPrefix}CreatedAt`]: Math.floor(ms / 1000),
[`${fieldPrefix}Id`]: `${source}-${ms}`,
[`${fieldPrefix}Id`]: `~${source}-${ms}`,
};
};
2 changes: 1 addition & 1 deletion modules/billing/services/billing.reconcile.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const RECONCILE_PAGE_SIZE = 100;
/**
* Statuses reconciled against Stripe.
*/
const RECONCILE_STATUSES = ['active', 'past_due', 'trialing'];
const RECONCILE_STATUSES = ['active', 'past_due', 'trialing', 'unpaid'];

/**
* Valid plan names from config.
Expand Down
22 changes: 20 additions & 2 deletions modules/billing/services/billing.webhook.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ const validPlans = new Set(config.billing?.plans || ['free', 'starter', 'pro', '
* @param {string} plan - The plan name to validate.
* @returns {string|null} The plan name if valid, null otherwise.
*/
const validatePlan = (plan) => (validPlans.has(plan) ? plan : null);
const validatePlan = (plan) => {
if (validPlans.has(plan)) return plan;
if (plan) logger.warn('[billing.webhook] validatePlan: unrecognized planId', { raw: plan, validPlans: [...validPlans] });
return null;
};

/**
* Plan rank lookup — higher index means higher-tier plan.
Expand Down Expand Up @@ -601,7 +605,21 @@ const handleInvoicePaymentSucceeded = async (invoice, event) => {
}

if (isPastDue && resolvedPlan) {
await syncOrganizationPlan(organizationId, resolvedPlan);
try {
await syncOrganizationPlan(organizationId, resolvedPlan);
} catch (syncErr) {
logger.error('[billing.webhook] syncOrganizationPlan failed (non-fatal)', {
organizationId,
error: syncErr?.message ?? String(syncErr),
});
try {
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),
});
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
};

Expand Down
2 changes: 1 addition & 1 deletion modules/billing/tests/billing.admin.service.unit.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ describe('BillingAdminService unit tests:', () => {

expect(lastSubscriptionEventCreatedAt).toBeGreaterThanOrEqual(before);
expect(lastSubscriptionEventCreatedAt).toBeLessThanOrEqual(after);
expect(lastSubscriptionEventId).toMatch(/^admin-cancel-\d+$/);
expect(lastSubscriptionEventId).toMatch(/^~admin-cancel-\d+$/);
});
});

Expand Down
24 changes: 24 additions & 0 deletions modules/billing/tests/billing.quota.unit.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,5 +621,29 @@ describe('requireQuota middleware:', () => {
// Fail-closed branch must short-circuit before the meter check
expect(mockBillingUsageService.getMeter).not.toHaveBeenCalled();
});

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();
});
Comment on lines +625 to +647

@coderabbitai coderabbitai Bot May 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

});
});
4 changes: 2 additions & 2 deletions modules/billing/tests/billing.subscription.repository.unit.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ describe('BillingSubscriptionRepository unit tests:', () => {

expect(lastSubscriptionEventCreatedAt).toBeGreaterThanOrEqual(before);
expect(lastSubscriptionEventCreatedAt).toBeLessThanOrEqual(after);
expect(lastSubscriptionEventId).toMatch(/^admin-bump-\d+$/);
expect(lastSubscriptionEventId).toMatch(/^~admin-bump-\d+$/);
});
});

Expand All @@ -433,7 +433,7 @@ describe('BillingSubscriptionRepository unit tests:', () => {

expect(lastSubscriptionEventCreatedAt).toBeGreaterThanOrEqual(before);
expect(lastSubscriptionEventCreatedAt).toBeLessThanOrEqual(after);
expect(lastSubscriptionEventId).toMatch(/^dunning-\d+$/);
expect(lastSubscriptionEventId).toMatch(/^~dunning-\d+$/);
});
});
});
Loading