diff --git a/ERRORS.md b/ERRORS.md index 9a64d1e6c..96a1500a0 100644 --- a/ERRORS.md +++ b/ERRORS.md @@ -35,3 +35,4 @@ Use this file as a compact memory of recurring AI mistakes. - [2026-07-16] architecture: `users.service.js remove()`'s sole-owner cascade called `OrganizationsRepository.remove()` directly instead of `organizations.crud.service.js#remove()`, silently skipping `runOrganizationRemovedHandlers` (the `onOrganizationRemoved` seam `modules/tasks/tasks.init.js` registers for org-scoped task cleanup) -> deleting an org must always route through the owning module's SERVICE method, never straight to its Repository, even for a "bare" cascade delete from another module — a direct-repository shortcut silently bypasses any registered removal hook; a stale cross-test fixture in `tasks.integration.tests.js` (an orphaned-but-still-existing task reused across unrelated test cases) was inadvertently relying on this bug and needed a properly isolated fixture once the cascade actually cleaned up; see pierreb-devkit/Node#3965 - [2026-07-17] error handling: shared helpers (`lib/helpers/mailer/index.js#sendMail`, `modules/audit/services/audit.service.js#log`) wrapped the failing call in their own try/catch, logged a single generic line, and resolved `null` -> every caller's own context-rich `.catch()` (action/userId/orgId) became dead code on a real outage, since the promise never rejected; a central helper must let the underlying call's rejection propagate and let each CALL SITE own the "never break the main flow" `.catch()` with its own context — never swallow centrally just because most callers currently attach a catch; see pierreb-devkit/Node#3966 - [2026-07-17] billing/stripe: #3964 fixed the admin (409 abort) and reconcile (skip-comparison) call sites of the shared plan resolver but left `billing.webhook.service.js resolvePlan`'s `?? 'free'` fallback in place -> a webhook for an existing paid org whose Stripe price is unresolvable (unmapped `priceId`, no valid `metadata.planId` — e.g. a manually-sold enterprise price) silently downgraded the org to `'free'` on the next `customer.subscription.updated`/`.created`/`invoice.payment_succeeded` event; a webhook cannot 409-abort mid-flight like the admin path, so the fix instead retains the org's/subscription's already-loaded `.plan` (never a hardcoded `'free'`) and emits `billing.webhook.plan_unresolved` for manual review — only a brand-new subscription with zero prior plan reference (no DB row anywhere for that org) still defaults to `'free'`; see pierreb-devkit/Node#3970 +- [2026-07-25] billing: `forceRotateForPlanChange` (week-quota snapshot rotation) was wired only into `handleSubscriptionUpdated`'s plan-change block -> `handleCheckoutCompleted` and both branches of `handleSubscriptionCreated` never rotated the current-week quota snapshot after a mid-week plan activation, so an upgraded org kept the previous plan's weekly quota (often 0 on free→paid) until the next weekly reset; fixed by calling the same non-fatal, logged `forceRotateForPlanChange(organizationId, { preserveUsage: true })` from all three activation call sites, and by making `billing.usage.service.js incrementMeter` read the live plan quota (already fetched for the snapshot write) instead of the potentially-stale `updatedDoc.meterQuota` for overflow decisions — mirrors the existing live-quota display fix in `billing.controller.js`; see pierreb-devkit/Node#3988 diff --git a/modules/billing/services/billing.usage.service.js b/modules/billing/services/billing.usage.service.js index daacc099e..07ee247c1 100644 --- a/modules/billing/services/billing.usage.service.js +++ b/modules/billing/services/billing.usage.service.js @@ -114,14 +114,20 @@ const incrementMeter = async (organizationId, units, breakdown, idempotencyKey) return { applied: false, meterUsed: existing?.meterUsed ?? 0, - meterQuota: existing?.meterQuota ?? meterQuota, + // Live plan quota (fetched above), not the stored snapshot — same rationale as the + // non-replay path below: the stored week-doc snapshot goes stale after a mid-week + // plan change, so a replayed call must not report a pre-rotation quota either. + meterQuota, extrasConsumed: 0, alertCrossed: null, }; } const newMeterUsed = updatedDoc.meterUsed ?? 0; - const effectiveQuota = updatedDoc.meterQuota ?? meterQuota; + // Use the LIVE plan quota (fetched above), not updatedDoc.meterQuota — the stored + // week-doc snapshot goes stale after a mid-week plan change: the live plan config + // is authoritative for overflow decisions (mirrors the display fix in billing.controller.js). + const effectiveQuota = meterQuota; // Overflow detection: units consumed beyond the plan quota go to extras. // Free plan (effectiveQuota === 0): every unit must be debited from extras — diff --git a/modules/billing/services/billing.webhook.service.js b/modules/billing/services/billing.webhook.service.js index ec3b86f24..c48129127 100644 --- a/modules/billing/services/billing.webhook.service.js +++ b/modules/billing/services/billing.webhook.service.js @@ -308,6 +308,19 @@ const handleCheckoutCompleted = async (session, event) => { } await syncOrganizationPlan(organizationId, plan); + + // Checkout activation can land mid-week on a pre-existing week doc (created earlier under + // the prior plan) — refresh the stored quota snapshot so it stays honest for reconcile/ + // display-fallback. Non-fatal: usage attribution itself reads the live quota + // (billing.usage.service.js), so a failure here does not misprice usage, only the snapshot. + try { + await BillingResetService.forceRotateForPlanChange(organizationId, { preserveUsage: true }); + } catch (err) { + logger.warn('[billing.webhook] forceRotateForPlanChange failed after checkout.session.completed', { + organizationId, + error: err?.message ?? String(err), + }); + } }; /** @@ -1222,6 +1235,17 @@ const handleSubscriptionCreated = async (subscription, event) => { throw err; } await syncOrganizationPlan(organizationId, newPlan); + + // Keeps the stored week snapshot honest for reconcile/display-fallback — same + // rationale as the checkout.session.completed activation path above. + try { + await BillingResetService.forceRotateForPlanChange(organizationId, { preserveUsage: true }); + } catch (err) { + logger.warn('[billing.webhook] forceRotateForPlanChange failed after subscription.created (new row)', { + organizationId, + error: err?.message ?? String(err), + }); + } return; } @@ -1247,6 +1271,17 @@ const handleSubscriptionCreated = async (subscription, event) => { } await syncOrganizationPlan(organizationId, newPlan); + + // Keeps the stored week snapshot honest for reconcile/display-fallback — same + // rationale as the checkout.session.completed activation path above. + try { + await BillingResetService.forceRotateForPlanChange(organizationId, { preserveUsage: true }); + } catch (err) { + logger.warn('[billing.webhook] forceRotateForPlanChange failed after subscription.created (existing row)', { + organizationId, + error: err?.message ?? String(err), + }); + } }; /** diff --git a/modules/billing/tests/billing.lifecycle.integration.tests.js b/modules/billing/tests/billing.lifecycle.integration.tests.js index a9f684d66..f3f18eb11 100644 --- a/modules/billing/tests/billing.lifecycle.integration.tests.js +++ b/modules/billing/tests/billing.lifecycle.integration.tests.js @@ -46,6 +46,20 @@ describe('Billing meter lifecycle integration tests:', () => { // to the test DB before tests run. Prevents E11000 flakes on the first resetWeek sweep. await Subscription.syncIndexes(); + // Mock Stripe so handleCheckoutCompleted can call stripe.subscriptions.retrieve without a + // real network call — mirrors billing.webhook.integration.tests.js. Only handleCheckoutCompleted + // and handleCheckoutPaymentCompleted read getStripe() in this service; neither of the other + // handlers exercised in this file (handleSubscriptionUpdated, handleSubscriptionCreated, + // BillingMeterService.attribute, BillingUsageService.incrementMeter) touch it, so this mock + // is inert for those tests. + jest.unstable_mockModule('../lib/stripe.js', () => ({ + default: jest.fn(() => ({ + subscriptions: { + retrieve: jest.fn().mockResolvedValue({ status: 'active' }), + }, + })), + })); + BillingWebhookService = (await import('../services/billing.webhook.service.js')).default; BillingMeterService = (await import('../services/billing.meter.service.js')).default; BillingUsageService = (await import('../services/billing.usage.service.js')).default; @@ -137,6 +151,146 @@ describe('Billing meter lifecycle integration tests:', () => { expect(usage.meterBreakdown).toEqual({ scrap: 25 }); }); + test('checkout activation refreshes the active week quota snapshot and attribution reads the live quota', async () => { + // Two distinct plan tiers from the project's enum, same rationale as the test above: this + // repo's validPlans enum (billing.webhook.service.js) is frozen at import time from + // config.billing.plans, not the planDefinitions this test overrides below — so pick real, + // already-valid plan ids rather than hardcoding literals. + const plans = Array.isArray(config.billing?.plans) && config.billing.plans.length >= 2 + ? config.billing.plans + : ['starter', 'pro']; + const initialPlan = plans[0]; + const upgradePlan = plans[plans.length - 1]; + const initialVersion = `${initialPlan}-v1`; + const upgradeVersion = `${upgradePlan}-v2`; + const upgradeQuota = 1000; + + config.billing.planDefinitions = [ + { planId: initialPlan, version: initialVersion, meterQuota: 0, ratios: { scrap: 1 } }, + { planId: upgradePlan, version: upgradeVersion, meterQuota: upgradeQuota, ratios: { scrap: 1 } }, + ]; + + const organizationId = new mongoose.Types.ObjectId(); + const weekKey = isoWeekKey(new Date()); + await Organization.create({ _id: organizationId, name: 'Checkout Activation Org', slug: 'checkout-activation-org', plan: initialPlan }); + await Subscription.create({ + organization: organizationId, + stripeCustomerId: 'cus_checkout_activation', + stripeSubscriptionId: 'sub_checkout_activation', + plan: initialPlan, + status: 'active', + }); + + // Current-week doc pre-exists under the initial plan (created earlier that day, before checkout + // completed) — reproduces the latent gap: no activation handler ever rotated this snapshot, so + // it stayed stale at quota=0 after the mid-week upgrade. organizationId must be the ObjectId, + // not its string form — the schema field is ObjectId-typed. + await BillingUsage.create({ + organizationId, + month: '2026-07', + weekKey, + counters: {}, + meterUsed: 500, + meterQuota: 0, + planVersion: initialVersion, + meterBreakdown: { scrap: 500 }, + consumedAttributionKeys: [], + }); + + await BillingWebhookService.handleCheckoutCompleted( + { + customer: 'cus_checkout_activation', + subscription: 'sub_checkout_activation', + metadata: { organizationId: organizationId.toString(), plan: upgradePlan }, + }, + { id: 'evt_checkout_activation', created: Math.floor(Date.now() / 1000) }, + ); + + // (a) refresh-on-activation: the stored week snapshot must be rotated to the new plan's quota. + const usageAfterActivation = await BillingUsage.findOne({ organizationId, weekKey }).lean(); + expect(usageAfterActivation.meterQuota).toBe(upgradeQuota); + expect(usageAfterActivation.planVersion).toBe(upgradeVersion); + expect(usageAfterActivation.meterUsed).toBe(500); + + // (b) live-quota attribution: further usage is measured against the live upgraded quota, so it + // stays within quota (500 + 50 = 550 < 1000) and never drains extras. + const result = await BillingUsageService.incrementMeter( + organizationId.toString(), + 50, + { scrap: 50 }, + `${organizationId.toString()}:post-activation`, + ); + + expect(result.applied).toBe(true); + expect(result.meterUsed).toBe(550); + expect(result.meterQuota).toBe(upgradeQuota); + expect(result.extrasConsumed).toBe(0); + + // No extras balance/ledger doc was ever created — quota-first, extras untouched. + const balance = await BillingExtraBalance.findOne({ organization: organizationId }).lean(); + expect(balance).toBeNull(); + }); + + test('subscription.created (existing row) refreshes the active week quota snapshot on a mid-week plan change', async () => { + const plans = Array.isArray(config.billing?.plans) && config.billing.plans.length >= 2 + ? config.billing.plans + : ['starter', 'pro']; + const initialPlan = plans[0]; + const upgradePlan = plans[plans.length - 1]; + const initialVersion = `${initialPlan}-v1`; + const upgradeVersion = `${upgradePlan}-v2`; + const upgradeQuota = 2000; + + config.billing.planDefinitions = [ + { planId: initialPlan, version: initialVersion, meterQuota: 0, ratios: { scrap: 1 } }, + { planId: upgradePlan, version: upgradeVersion, meterQuota: upgradeQuota, ratios: { scrap: 1 } }, + ]; + + const organizationId = new mongoose.Types.ObjectId(); + const weekKey = isoWeekKey(new Date()); + await Organization.create({ _id: organizationId, name: 'Subscription Created Org', slug: 'subscription-created-org', plan: initialPlan }); + // Existing row (found via stripeSubscriptionId) exercises the "existing row" branch of + // handleSubscriptionCreated — a subscription.created delivered for an already-known + // subscription (e.g. a Dashboard-driven plan swap) rather than a fresh checkout. + await Subscription.create({ + organization: organizationId, + stripeCustomerId: 'cus_subscription_created', + stripeSubscriptionId: 'sub_subscription_created', + plan: initialPlan, + status: 'active', + }); + await BillingUsage.create({ + organizationId, + month: '2026-07', + weekKey, + counters: {}, + meterUsed: 300, + meterQuota: 0, + planVersion: initialVersion, + meterBreakdown: { scrap: 300 }, + consumedAttributionKeys: [], + }); + + await BillingWebhookService.handleSubscriptionCreated( + { + id: 'sub_subscription_created', + customer: 'cus_subscription_created', + status: 'active', + current_period_start: Math.floor(Date.now() / 1000) - 24 * 60 * 60, + items: { data: [{ price: { metadata: { planId: upgradePlan } } }] }, + }, + { id: 'evt_subscription_created', created: Math.floor(Date.now() / 1000) }, + ); + + const usageAfterActivation = await BillingUsage.findOne({ organizationId, weekKey }).lean(); + expect(usageAfterActivation.meterQuota).toBe(upgradeQuota); + expect(usageAfterActivation.planVersion).toBe(upgradeVersion); + expect(usageAfterActivation.meterUsed).toBe(300); + + const subscription = await Subscription.findOne({ organization: organizationId }).lean(); + expect(subscription.plan).toBe(upgradePlan); + }); + test('attribute applies usage inline — no outbox collection created', async () => { config.billing.planDefinitions = [ { planId: 'pro', version: 'pro-v1', meterQuota: 100000, ratios: { scrap: 1 } }, diff --git a/modules/billing/tests/billing.usage.service.unit.tests.js b/modules/billing/tests/billing.usage.service.unit.tests.js index 8a04edfb3..aabe3c876 100644 --- a/modules/billing/tests/billing.usage.service.unit.tests.js +++ b/modules/billing/tests/billing.usage.service.unit.tests.js @@ -216,6 +216,22 @@ describe('BillingUsageService — meter extensions unit tests:', () => { expect(result.meterUsed).toBe(100); }); + test('replay returns the LIVE plan quota, not the stored (possibly stale) snapshot', async () => { + // Same rationale as the non-replay overflow path: the stored week-doc snapshot can be + // stale after a mid-week plan change (e.g. rotation hasn't run yet, or failed non-fatally). + // A replayed call must report the live quota too, not a pre-rotation value. + mockSubscriptionRepository.findPlan.mockResolvedValue({ plan: 'pro' }); + mockPlanService.getActivePlan.mockReturnValue(makePlan({ meterQuota: 1000 })); + mockUsageRepository.incrementMeter.mockResolvedValue(null); + // Stored doc still carries the stale pre-upgrade quota (0). + mockUsageRepository.findByWeek.mockResolvedValue(makeUsageDoc({ meterUsed: 100, meterQuota: 0 })); + + const result = await BillingUsageService.incrementMeter(orgId, 100, {}, 'hist_replay_stale_snapshot'); + + expect(result.applied).toBe(false); + expect(result.meterQuota).toBe(1000); + }); + test('incrementMeter same idempotencyKey twice → second is no-op', async () => { mockSubscriptionRepository.findPlan.mockResolvedValue({ plan: 'pro' }); mockPlanService.getActivePlan.mockReturnValue(makePlan()); diff --git a/modules/billing/tests/billing.webhook.checkout.unit.tests.js b/modules/billing/tests/billing.webhook.checkout.unit.tests.js index 79fd9b6e7..1f6ed5664 100644 --- a/modules/billing/tests/billing.webhook.checkout.unit.tests.js +++ b/modules/billing/tests/billing.webhook.checkout.unit.tests.js @@ -14,6 +14,7 @@ describe('Billing webhook checkout unit tests:', () => { let mockSubscriptionRepository; let mockOrganizationRepository; let mockExtraService; + let mockResetService; let mockStripeInstance; const orgId = '507f1f77bcf86cd799439011'; @@ -70,8 +71,12 @@ describe('Billing webhook checkout unit tests:', () => { default: mockExtraService, })); + mockResetService = { + resetWeek: jest.fn(), + forceRotateForPlanChange: jest.fn().mockResolvedValue({}), + }; jest.unstable_mockModule('../services/billing.reset.service.js', () => ({ - default: { resetWeek: jest.fn() }, + default: mockResetService, })); jest.unstable_mockModule('../lib/events.js', () => ({ @@ -332,6 +337,50 @@ describe('Billing webhook checkout unit tests:', () => { ); }); + test('forceRotateForPlanChange called (preserveUsage: true) after activation', async () => { + const existing = { _id: subId, organization: orgId }; + mockSubscriptionRepository.findByOrganization.mockResolvedValue(existing); + mockSubscriptionRepository.updateIfEventNewer.mockResolvedValue({ _id: subId }); + + await BillingWebhookService.handleCheckoutCompleted( + { + customer: 'cus_123', + subscription: 'sub_456', + metadata: { organizationId: orgId, plan: 'pro' }, + }, + checkoutEvent, + ); + + expect(mockResetService.forceRotateForPlanChange).toHaveBeenCalledWith(orgId, { preserveUsage: true }); + }); + + test('forceRotateForPlanChange error is non-fatal (logged, does not throw)', async () => { + const existing = { _id: subId, organization: orgId }; + mockSubscriptionRepository.findByOrganization.mockResolvedValue(existing); + mockSubscriptionRepository.updateIfEventNewer.mockResolvedValue({ _id: subId }); + mockResetService.forceRotateForPlanChange.mockRejectedValue(new Error('db unavailable')); + + const { default: mockLogger } = await import('../../../lib/services/logger.js'); + + await expect( + BillingWebhookService.handleCheckoutCompleted( + { + customer: 'cus_123', + subscription: 'sub_456', + metadata: { organizationId: orgId, plan: 'pro' }, + }, + checkoutEvent, + ), + ).resolves.not.toThrow(); + // The subscription write already committed before the rotation call — a rotation + // failure must not roll it back or propagate. + expect(mockSubscriptionRepository.updateIfEventNewer).toHaveBeenCalled(); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[billing.webhook] forceRotateForPlanChange failed after checkout.session.completed', + expect.objectContaining({ organizationId: orgId }), + ); + }); + test('should create subscription with seeded markers when none exists', async () => { mockSubscriptionRepository.findByOrganization.mockResolvedValue(null); mockSubscriptionRepository.create.mockResolvedValue({}); diff --git a/modules/billing/tests/billing.webhook.priceid-map.unit.tests.js b/modules/billing/tests/billing.webhook.priceid-map.unit.tests.js index f5bdcc66f..4d179cb46 100644 --- a/modules/billing/tests/billing.webhook.priceid-map.unit.tests.js +++ b/modules/billing/tests/billing.webhook.priceid-map.unit.tests.js @@ -418,6 +418,32 @@ describe('handleSubscriptionCreated — safety-net handler:', () => { expect(mocks.mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'pro'); }); + test('existing doc: forceRotateForPlanChange called (preserveUsage: true) after successful update', async () => { + const existing = { _id: subId, organization: orgId }; + mocks.mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); + + await BillingWebhookService.handleSubscriptionCreated(makeSubscription(), makeEvent()); + + expect(mocks.mockResetService.forceRotateForPlanChange).toHaveBeenCalledWith(orgId, { preserveUsage: true }); + }); + + test('existing doc: forceRotateForPlanChange error is non-fatal (logged, does not throw)', async () => { + const existing = { _id: subId, organization: orgId }; + mocks.mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); + mocks.mockResetService.forceRotateForPlanChange.mockRejectedValue(new Error('db unavailable')); + + const { default: mockLogger } = await import('../../../lib/services/logger.js'); + + await expect( + BillingWebhookService.handleSubscriptionCreated(makeSubscription(), makeEvent()), + ).resolves.not.toThrow(); + expect(mocks.mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'pro'); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[billing.webhook] forceRotateForPlanChange failed after subscription.created (existing row)', + expect.objectContaining({ organizationId: orgId }), + ); + }); + test('existing doc: stale event — updateIfEventNewer returns null, syncOrganizationPlan NOT called', async () => { const existing = { _id: subId, organization: orgId }; mocks.mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(existing); @@ -470,6 +496,43 @@ describe('handleSubscriptionCreated — safety-net handler:', () => { expect(mocks.mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'pro'); }); + test('Dashboard subscription: create succeeds — forceRotateForPlanChange called (preserveUsage: true)', async () => { + // Unit-level coverage of the new-row branch's rotation call. A real-DB integration test + // cannot exercise this: `organization` is a unique field on the Subscription schema + // (billing.subscription.model.mongoose.js), so `orgSub` being found via + // findByStripeCustomerId always means a row already exists for that org, and a real + // Subscription.create() for the same org always throws E11000 (see the sibling + // "race condition: E11000" test below) before reaching this call. Mocking the repository + // lets us assert the rotation wiring on the success branch in isolation. + const orgSub = { organization: orgId }; + mocks.mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(null); + mocks.mockSubscriptionRepository.findByStripeCustomerId.mockResolvedValue(orgSub); + + await BillingWebhookService.handleSubscriptionCreated(makeSubscription(), makeEvent()); + + expect(mocks.mockResetService.forceRotateForPlanChange).toHaveBeenCalledWith(orgId, { preserveUsage: true }); + }); + + test('Dashboard subscription: forceRotateForPlanChange error is non-fatal (logged, does not throw)', async () => { + const orgSub = { organization: orgId }; + mocks.mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(null); + mocks.mockSubscriptionRepository.findByStripeCustomerId.mockResolvedValue(orgSub); + mocks.mockResetService.forceRotateForPlanChange.mockRejectedValue(new Error('db unavailable')); + + const { default: mockLogger } = await import('../../../lib/services/logger.js'); + + await expect( + BillingWebhookService.handleSubscriptionCreated(makeSubscription(), makeEvent()), + ).resolves.not.toThrow(); + // Row creation + plan sync already committed before the rotation call — a rotation + // failure must not roll those back or propagate. + expect(mocks.mockOrganizationRepository.setPlan).toHaveBeenCalledWith(orgId, 'pro'); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[billing.webhook] forceRotateForPlanChange failed after subscription.created (new row)', + expect.objectContaining({ organizationId: orgId }), + ); + }); + test('Dashboard subscription: resolves plan from priceId map (no metadata)', async () => { const orgSub = { organization: orgId }; mocks.mockSubscriptionRepository.findByStripeSubscriptionId.mockResolvedValue(null); @@ -565,6 +628,9 @@ describe('handleSubscriptionCreated — safety-net handler:', () => { const result = await BillingWebhookService.handleSubscriptionCreated(makeSubscription(), makeEvent()); expect(result).toEqual({ skipped: true, reason: 'duplicate' }); + // The duplicate-skip return happens before syncOrganizationPlan — rotation must not fire + // on a row this handler never actually wrote. + expect(mocks.mockResetService.forceRotateForPlanChange).not.toHaveBeenCalled(); }); test('non-duplicate DB error on create — rethrows', async () => {