From 50be39d86cf05e1ec350d41dcf4d641c981c7e58 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 11:43:32 +0200 Subject: [PATCH 1/4] =?UTF-8?q?fix(billing):=20v4=20hardening=20=E2=80=94?= =?UTF-8?q?=20finish=20v3=20oversights=20+=20coh=C3=A9rence=20API=20consta?= =?UTF-8?q?nts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - weeklyReset cron: applyJitter (les 3 autres l'ont) - constants extraction: dollarsToUnitRatio + maxUnitsPerOperation + getDefaultPlanId - reset.service: drop isoWeekKey re-export (lib leak) - usage.service: 80% threshold no longer skipped on >100% jump (drop break) - usage.service + init: thresholdFields config validation at boot (meterMode-gated) - README: full config knobs table - nits: cron-utils fractional-input guard restored, Date.now comment, errors.js extension comment - alertCrossed JSDoc: document last-threshold-emitted semantics on multi-crossing --- modules/billing/README.md | 17 +++++++++ modules/billing/billing.init.js | 14 +++++++ modules/billing/crons/billing.weeklyReset.js | 11 +++++- modules/billing/lib/billing.constants.js | 24 ++++++++++++ modules/billing/lib/billing.cron-utils.js | 2 +- modules/billing/lib/billing.errors.js | 2 + .../billing/services/billing.meter.service.js | 12 ++++-- .../billing/services/billing.reset.service.js | 7 ++-- modules/billing/services/billing.service.js | 2 +- .../billing/services/billing.usage.service.js | 7 ++-- .../tests/billing.config-knobs.unit.tests.js | 33 ++++++++++++++++ .../billing.cron.weeklyReset.unit.tests.js | 14 +++++++ .../billing/tests/billing.init.unit.tests.js | 38 +++++++++++++++++++ .../billing.lifecycle.integration.tests.js | 5 +-- .../tests/billing.meter.service.unit.tests.js | 11 ++++++ .../tests/billing.reset.service.unit.tests.js | 7 ++-- .../tests/billing.usage.service.unit.tests.js | 25 ++++++++++++ 17 files changed, 212 insertions(+), 19 deletions(-) diff --git a/modules/billing/README.md b/modules/billing/README.md index bb82ac3d4..a497c964b 100644 --- a/modules/billing/README.md +++ b/modules/billing/README.md @@ -105,6 +105,23 @@ Consumers should NOT retry on `applied: true` — the outbox handles eventual co ## Meter hardening configuration +### Configuration knobs + +| Knob | Type | Default | Notes | +|------|------|---------|-------| +| `billing.meter.runBase` | number | 1 | METER_RUN_BASE base unit cost | +| `billing.meter.fallbackPlanId` | string \| null | null | Fallback plan when active not resolvable | +| `billing.meter.dollarsToUnitRatio` | number | 1000 | Dollar amount → meter unit conversion | +| `billing.meter.maxUnitsPerOperation` | number | Infinity | Cap per single attribute call | +| `billing.meter.ratioVersion` | string \| null | null | DOWNSTREAM-OVERRIDE-REQUIRED — pricing version namespace | +| `billing.outbox.maxRetryAttempts` | number | 5 | Outbox retry limit before exhausted | +| `billing.outbox.retryIntervalSec` | number | 300 | Cron retry interval | +| `billing.crons.jitterMaxMs` | number | 60000 | Cron startup jitter max | +| `billing.planChange.preserveUsageDefault` | boolean | true | forceRotateForPlanChange default | +| `billing.alerts.thresholdPercents` | number[] | [80, 100] | Schema-supported only — see thresholdFields | +| `billing.events.extrasExhausted` | string | 'billing.extras_debit.exhausted' | Event name for downstream alerting | +| `billing.defaultPlan` | string | 'free' | Default plan ID for fallback | + Defaults live in `modules/billing/config/billing.development.config.js` and can be overridden by downstream project config: ```js diff --git a/modules/billing/billing.init.js b/modules/billing/billing.init.js index 061b13e6b..a9a81336f 100644 --- a/modules/billing/billing.init.js +++ b/modules/billing/billing.init.js @@ -7,6 +7,7 @@ import AnalyticsService from '../../lib/services/analytics.js'; import billingEvents from './lib/events.js'; import BillingPlanService from './services/billing.plan.service.js'; import BillingUsageRepository from './repositories/billing.usage.repository.js'; +import { getAlertThresholdPercents } from './lib/billing.constants.js'; /** * Billing module initialisation. @@ -26,6 +27,19 @@ export default async (app) => { } } + // Validate alert threshold percents (meterMode only) — warn on configured values with no schema field. + // Only 80 and 100 have matching alertedAtN fields in BillingUsage; other values are silently skipped. + if (config?.billing?.meterMode) { + const SUPPORTED_THRESHOLD_PERCENTS = new Set([80, 100]); + for (const threshold of getAlertThresholdPercents()) { + if (!SUPPORTED_THRESHOLD_PERCENTS.has(threshold)) { + console.warn( + `[billing] Configured alert threshold ${threshold}% is not in schema-supported set [80, 100] — alert will be silently skipped`, + ); + } + } + } + // Update analytics group properties when a subscription plan changes billingEvents.on('plan.changed', ({ organizationId, newPlan }) => { try { diff --git a/modules/billing/crons/billing.weeklyReset.js b/modules/billing/crons/billing.weeklyReset.js index a38463d11..0cac3a7e1 100644 --- a/modules/billing/crons/billing.weeklyReset.js +++ b/modules/billing/crons/billing.weeklyReset.js @@ -13,9 +13,16 @@ process.env.NODE_ENV = process.env.NODE_ENV || 'development'; -const [{ default: config }, { default: mongooseService }] = await Promise.all([ +const [ + { default: config }, + { default: mongooseService }, + { applyJitter }, + { getCronJitterMaxMs }, +] = await Promise.all([ import('../../../config/index.js'), import('../../../lib/services/mongoose.js'), + import('../lib/billing.cron-utils.js'), + import('../lib/billing.constants.js'), ]); if (!config?.billing?.meterMode) { @@ -23,6 +30,8 @@ if (!config?.billing?.meterMode) { process.exit(0); } +await applyJitter(getCronJitterMaxMs()); + try { await mongooseService.connect(); diff --git a/modules/billing/lib/billing.constants.js b/modules/billing/lib/billing.constants.js index a368f2446..8587536fb 100644 --- a/modules/billing/lib/billing.constants.js +++ b/modules/billing/lib/billing.constants.js @@ -84,6 +84,30 @@ export const getAlertThresholdPercents = () => { .sort((a, b) => b - a); }; +/** + * @function getDollarsToUnitRatio + * @description Resolve the configured conversion factor from dollar amounts to meter units. + * @returns {number} Dollar-to-unit ratio (e.g. 1000 means $1 = 1000 units). + */ +export const getDollarsToUnitRatio = () => + config?.billing?.meter?.dollarsToUnitRatio ?? 1000; + +/** + * @function getMaxUnitsPerOperation + * @description Resolve the configured per-operation unit cap. Infinity means no cap. + * @returns {number} Maximum units allowed for a single attribute call. + */ +export const getMaxUnitsPerOperation = () => + config?.billing?.meter?.maxUnitsPerOperation ?? Infinity; + +/** + * @function getDefaultPlanId + * @description Resolve the default plan ID used as a fallback when no active subscription exists. + * @returns {string} Default plan identifier. + */ +export const getDefaultPlanId = () => + config?.billing?.defaultPlan ?? 'free'; + /** * @function getExtrasExhaustedEventName * @description Resolve the event name emitted when extras debit retries are exhausted. diff --git a/modules/billing/lib/billing.cron-utils.js b/modules/billing/lib/billing.cron-utils.js index 7c309af40..c6bbd397b 100644 --- a/modules/billing/lib/billing.cron-utils.js +++ b/modules/billing/lib/billing.cron-utils.js @@ -12,7 +12,7 @@ import { randomInt } from 'node:crypto'; export const applyJitter = async (maxMs) => { if (!Number.isFinite(maxMs) || maxMs <= 0) return 0; const jitterMaxMs = Math.floor(maxMs); - if (jitterMaxMs <= 0) return 0; + if (jitterMaxMs <= 0) return 0; // guard fractional inputs (e.g. 0.4) — randomInt(0,0) throws const delayMs = randomInt(0, jitterMaxMs); await new Promise((resolve) => setTimeout(resolve, delayMs)); return delayMs; diff --git a/modules/billing/lib/billing.errors.js b/modules/billing/lib/billing.errors.js index e476b5066..0e2f2d209 100644 --- a/modules/billing/lib/billing.errors.js +++ b/modules/billing/lib/billing.errors.js @@ -1,3 +1,5 @@ +// Extension point: add E_CAST, E_VALIDATION helpers here as needed + /** * @function isDuplicateKeyError * @description Identify Mongo duplicate-key errors across driver and string-only shapes. diff --git a/modules/billing/services/billing.meter.service.js b/modules/billing/services/billing.meter.service.js index 7396f4b7b..d87a0d34f 100644 --- a/modules/billing/services/billing.meter.service.js +++ b/modules/billing/services/billing.meter.service.js @@ -6,7 +6,13 @@ import BillingPlanService from './billing.plan.service.js'; import BillingUsageService from './billing.usage.service.js'; import BillingExtraService from './billing.extra.service.js'; import BillingMeterOutboxRepository from '../repositories/billing.meter.outbox.repository.js'; -import { getMeterFallbackPlanId, getMeterRunBase, METER_RUN_BASE } from '../lib/billing.constants.js'; +import { + getMeterFallbackPlanId, + getMeterRunBase, + getDollarsToUnitRatio, + getMaxUnitsPerOperation, + METER_RUN_BASE, +} from '../lib/billing.constants.js'; export { METER_RUN_BASE }; @@ -35,7 +41,7 @@ const unitsFromCosts = async (costs, planId, ratioVersion) => { return { totalUnits: getMeterRunBase(), breakdown: {} }; } - const dollarsToUnitRatio = config?.billing?.meter?.dollarsToUnitRatio ?? 1000; + const dollarsToUnitRatio = getDollarsToUnitRatio(); const hasBillableCost = Object.values(costs).some( (cost) => typeof cost === 'number' && Number.isFinite(cost) && cost > 0, @@ -223,7 +229,7 @@ const attribute = async (history, organizationId, options = {}) => { ({ totalUnits, breakdown } = await unitsFromCosts(costs, planId, ratioVersion)); } - const maxUnits = config?.billing?.meter?.maxUnitsPerOperation ?? Infinity; + const maxUnits = getMaxUnitsPerOperation(); const cappedUnits = Math.min(totalUnits, maxUnits); const isCapped = cappedUnits < totalUnits; const cappedBreakdown = isCapped ? capBreakdown(breakdown, cappedUnits, totalUnits) : breakdown; diff --git a/modules/billing/services/billing.reset.service.js b/modules/billing/services/billing.reset.service.js index d94524695..68d715190 100644 --- a/modules/billing/services/billing.reset.service.js +++ b/modules/billing/services/billing.reset.service.js @@ -7,7 +7,7 @@ import BillingSubscriptionRepository from '../repositories/billing.subscription. import BillingPlanService from './billing.plan.service.js'; import billingEvents from '../lib/events.js'; import { isoWeekKey } from '../lib/billing.isoWeek.js'; -import { getPlanChangePreserveUsageDefault } from '../lib/billing.constants.js'; +import { getPlanChangePreserveUsageDefault, getDefaultPlanId } from '../lib/billing.constants.js'; import { isDuplicateKeyError } from '../lib/billing.errors.js'; /** @@ -42,7 +42,7 @@ const resetWeek = async (orgId, periodStart) => { // Step 2 — Fetch the active plan to snapshot quota/planVersion — lean projection (plan only, no populate). const subscription = await BillingSubscriptionRepository.findPlan(orgId); - const planId = subscription?.plan ?? config?.billing?.defaultPlan ?? 'free'; + const planId = subscription?.plan ?? getDefaultPlanId(); const activePlan = await BillingPlanService.getActivePlan(planId); const meterQuota = activePlan?.meterQuota ?? 0; const planVersion = activePlan?.version ?? null; @@ -102,7 +102,7 @@ const forceRotateForPlanChange = async (organizationId, options = {}) => { if (!existingDoc) return null; const subscription = await BillingSubscriptionRepository.findPlan(organizationId); - const planId = subscription?.plan ?? config?.billing?.defaultPlan ?? 'free'; + const planId = subscription?.plan ?? getDefaultPlanId(); const activePlan = await BillingPlanService.getActivePlan(planId); const newQuota = activePlan?.meterQuota ?? 0; const newVersion = activePlan?.version ?? null; @@ -181,5 +181,4 @@ export default { resetWeek, forceRotateForPlanChange, resetAllDue, - isoWeekKey, }; diff --git a/modules/billing/services/billing.service.js b/modules/billing/services/billing.service.js index 4d98ac115..5f96a8162 100644 --- a/modules/billing/services/billing.service.js +++ b/modules/billing/services/billing.service.js @@ -193,7 +193,7 @@ const createExtrasCheckout = async (organization, packId, successUrl, cancelUrl) const subscription = await _ensureStripeCustomer(stripe, organization); - // Use a timestamped idempotency key (debounce double-click within ~1s granularity) + // Use a timestamped idempotency key (~1ms granularity — insufficient for double-click protection, known design) const idempotencyKey = `extras_checkout_${String(organization._id)}_${packId}_${Date.now()}`; const extrasCheckoutParams = { diff --git a/modules/billing/services/billing.usage.service.js b/modules/billing/services/billing.usage.service.js index f91e73d08..25fee289a 100644 --- a/modules/billing/services/billing.usage.service.js +++ b/modules/billing/services/billing.usage.service.js @@ -8,7 +8,7 @@ import BillingMeterOutboxRepository from '../repositories/billing.meter.outbox.r import BillingPlanService from './billing.plan.service.js'; import billingEvents from '../lib/events.js'; import { currentWeekKey } from '../lib/billing.isoWeek.js'; -import { getAlertThresholdPercents } from '../lib/billing.constants.js'; +import { getAlertThresholdPercents, getDefaultPlanId } from '../lib/billing.constants.js'; import { isDuplicateKeyError } from '../lib/billing.errors.js'; /** @@ -70,6 +70,8 @@ const reset = (organizationId) => UsageRepository.reset(organizationId, currentM * @param {Object} breakdown - Feature-keyed breakdown: { featureKey: units }. * @param {string} idempotencyKey - Unique key for replay protection (usually history._id). * @returns {Promise<{applied: boolean, meterUsed: number, meterQuota: number, extrasConsumed: number, alertCrossed: string|null}>} + * `alertCrossed` is the last threshold emitted this call (lowest value when multiple thresholds crossed in one jump, + * e.g. 0%→150% emits both 80 and 100 — alertCrossed='80'). Informational only; events are the authoritative signal. */ // biome-ignore lint/correctness/useQwikValidLexicalScope: false positive — Node.js service, not Qwik const incrementMeter = async (organizationId, units, breakdown, idempotencyKey) => { @@ -82,7 +84,7 @@ const incrementMeter = async (organizationId, units, breakdown, idempotencyKey) // Fetch active plan for quota snapshot — lean projection (plan field only, no populate) const subscription = await BillingSubscriptionRepository.findPlan(organizationId); - const planId = subscription?.plan ?? config?.billing?.defaultPlan ?? 'free'; + const planId = subscription?.plan ?? getDefaultPlanId(); const activePlan = await BillingPlanService.getActivePlan(planId); const meterQuota = activePlan?.meterQuota ?? 0; const planVersion = activePlan?.version ?? null; @@ -157,7 +159,6 @@ const incrementMeter = async (organizationId, units, breakdown, idempotencyKey) meterUsed: newMeterUsed, meterQuota: effectiveQuota, }); - break; } } } diff --git a/modules/billing/tests/billing.config-knobs.unit.tests.js b/modules/billing/tests/billing.config-knobs.unit.tests.js index 6ead5fe3b..ae5b6652c 100644 --- a/modules/billing/tests/billing.config-knobs.unit.tests.js +++ b/modules/billing/tests/billing.config-knobs.unit.tests.js @@ -58,6 +58,36 @@ describe('billing config knob helpers:', () => { expect(constants.getExtrasExhaustedEventName()).toBe('billing.custom.exhausted'); }); + test('getDollarsToUnitRatio reads from config', () => { + mockConfig.billing.meter.dollarsToUnitRatio = 500; + expect(constants.getDollarsToUnitRatio()).toBe(500); + }); + + test('getDollarsToUnitRatio defaults to 1000', async () => { + mockConfig.billing = {}; + expect(constants.getDollarsToUnitRatio()).toBe(1000); + }); + + test('getMaxUnitsPerOperation reads from config', () => { + mockConfig.billing.meter.maxUnitsPerOperation = 25000; + expect(constants.getMaxUnitsPerOperation()).toBe(25000); + }); + + test('getMaxUnitsPerOperation defaults to Infinity', async () => { + mockConfig.billing = {}; + expect(constants.getMaxUnitsPerOperation()).toBe(Infinity); + }); + + test('getDefaultPlanId reads from config', () => { + mockConfig.billing.defaultPlan = 'starter'; + expect(constants.getDefaultPlanId()).toBe('starter'); + }); + + test('getDefaultPlanId defaults to free', async () => { + mockConfig.billing = {}; + expect(constants.getDefaultPlanId()).toBe('free'); + }); + test('keeps backward-compatible defaults and runBaseUnits alias', async () => { mockConfig.billing = { plans: ['free'], @@ -74,5 +104,8 @@ describe('billing config knob helpers:', () => { expect(constants.getPlanChangePreserveUsageDefault()).toBe(true); expect(constants.getAlertThresholdPercents()).toEqual([100, 80]); expect(constants.getExtrasExhaustedEventName()).toBe('billing.extras_debit.exhausted'); + expect(constants.getDollarsToUnitRatio()).toBe(1000); + expect(constants.getMaxUnitsPerOperation()).toBe(Infinity); + expect(constants.getDefaultPlanId()).toBe('free'); }); }); diff --git a/modules/billing/tests/billing.cron.weeklyReset.unit.tests.js b/modules/billing/tests/billing.cron.weeklyReset.unit.tests.js index 51bf7ce89..ff189b96d 100644 --- a/modules/billing/tests/billing.cron.weeklyReset.unit.tests.js +++ b/modules/billing/tests/billing.cron.weeklyReset.unit.tests.js @@ -10,6 +10,20 @@ import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globa * We test the underlying BillingResetService.resetAllDue integration path rather than the * script file directly (which would require a live DB connection). */ + +describe('billing.weeklyReset cron — applyJitter registration:', () => { + test('applyJitter is exported from billing.cron-utils (parity check with other crons)', async () => { + const { applyJitter } = await import('../lib/billing.cron-utils.js'); + expect(typeof applyJitter).toBe('function'); + }); + + test('getCronJitterMaxMs is exported from billing.constants (used in weeklyReset jitter call)', async () => { + const { getCronJitterMaxMs } = await import('../lib/billing.constants.js'); + expect(typeof getCronJitterMaxMs).toBe('function'); + expect(typeof getCronJitterMaxMs()).toBe('number'); + }); +}); + describe('billing.weeklyReset cron — BillingResetService.resetAllDue:', () => { let BillingResetService; let mockConfig; diff --git a/modules/billing/tests/billing.init.unit.tests.js b/modules/billing/tests/billing.init.unit.tests.js index bd00bd1d8..da8d37ea1 100644 --- a/modules/billing/tests/billing.init.unit.tests.js +++ b/modules/billing/tests/billing.init.unit.tests.js @@ -129,6 +129,44 @@ describe('billing.init unit tests:', () => { await expect(billingInit(mockApp)).rejects.toThrow('legacy consumedHistoryIds field still present'); }); + test('warns at boot when thresholdPercents contains unsupported value (not 80/100)', async () => { + mockConfig.billing.meterMode = true; + mockConfig.billing.alerts = { thresholdPercents: [75] }; + mockConfig.billing.plans = ['free']; + + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + await billingInit(mockApp); + + const warnings = warnSpy.mock.calls.map((c) => c[0]); + expect(warnings.some((w) => w.includes('75%') && w.includes('silently skipped'))).toBe(true); + }); + + test('does not warn at boot when thresholdPercents contains only supported values', async () => { + mockConfig.billing.meterMode = true; + mockConfig.billing.alerts = { thresholdPercents: [80, 100] }; + mockConfig.billing.plans = ['free']; + + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + await billingInit(mockApp); + + const warnings = warnSpy.mock.calls.map((c) => c[0]); + expect(warnings.some((w) => w.includes('silently skipped'))).toBe(false); + }); + + test('does not warn at boot for threshold validation when meterMode=false', async () => { + mockConfig.billing.meterMode = false; + mockConfig.billing.alerts = { thresholdPercents: [75] }; // unsupported, but gate skips check + + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + await billingInit(mockApp); + + const warnings = warnSpy.mock.calls.map((c) => c[0]); + expect(warnings.some((w) => w.includes('silently skipped'))).toBe(false); + }); + test('boot validator failure does not crash boot', async () => { mockConfig.billing.meterMode = true; mockConfig.billing.plans = ['free']; diff --git a/modules/billing/tests/billing.lifecycle.integration.tests.js b/modules/billing/tests/billing.lifecycle.integration.tests.js index 0f632077a..f445cae5d 100644 --- a/modules/billing/tests/billing.lifecycle.integration.tests.js +++ b/modules/billing/tests/billing.lifecycle.integration.tests.js @@ -6,6 +6,7 @@ import { describe, beforeAll, beforeEach, afterAll, afterEach, test, expect, jes import config from '../../../config/index.js'; import mongooseService from '../../../lib/services/mongoose.js'; +import { isoWeekKey } from '../lib/billing.isoWeek.js'; /** * Integration tests for meter lifecycle hardening. @@ -17,7 +18,6 @@ describe('Billing meter lifecycle integration tests:', () => { let Organization; let BillingMeterOutbox; let BillingExtraBalance; - let BillingResetService; let BillingWebhookService; let BillingUsageService; let BillingMeterService; @@ -57,7 +57,6 @@ describe('Billing meter lifecycle integration tests:', () => { BillingMeterOutbox = mongoose.model('BillingMeterOutbox'); BillingExtraBalance = mongoose.model('BillingExtraBalance'); - BillingResetService = (await import('../services/billing.reset.service.js')).default; BillingWebhookService = (await import('../services/billing.webhook.service.js')).default; BillingUsageService = (await import('../services/billing.usage.service.js')).default; BillingMeterService = (await import('../services/billing.meter.service.js')).default; @@ -95,7 +94,7 @@ describe('Billing meter lifecycle integration tests:', () => { test('plan.changed webhook updates active week quota snapshot mid-week', async () => { const organizationId = new mongoose.Types.ObjectId(); - const weekKey = BillingResetService.isoWeekKey(new Date()); + const weekKey = isoWeekKey(new Date()); await Organization.create({ _id: organizationId, name: 'Lifecycle Org', slug: 'lifecycle-org', plan: 'starter' }); await createActivePlan('pro', 'pro-v2', 1000); await Subscription.create({ diff --git a/modules/billing/tests/billing.meter.service.unit.tests.js b/modules/billing/tests/billing.meter.service.unit.tests.js index 68a80d0f5..80e02fa65 100644 --- a/modules/billing/tests/billing.meter.service.unit.tests.js +++ b/modules/billing/tests/billing.meter.service.unit.tests.js @@ -173,6 +173,17 @@ describe('BillingMeterService unit tests:', () => { expect(result.totalUnits).toBe(7); }); + test('unitsFromCosts reads dollarsToUnitRatio from getDollarsToUnitRatio() (mockable via config)', async () => { + mockConfig.billing.meter.dollarsToUnitRatio = 500; + mockBillingPlanService.getPlanByVersion.mockResolvedValue(makePlan({ ratios: { scrap: 1 } })); + + // cost=0.001, ratio=1, dollarsToUnitRatio=500 → floor(0.001 * 1 * 500) = 0 + // cost=0.01, ratio=1, dollarsToUnitRatio=500 → floor(0.01 * 1 * 500) = 5 + const result = await BillingMeterService.unitsFromCosts({ scrap: 0.01 }, 'pro', 'v1'); + + expect(result.totalUnits).toBe(5); + }); + test('throws when positive costs have no ratioVersion in meterMode', async () => { await expect( BillingMeterService.unitsFromCosts({ scrap: 0.001 }, 'pro', null), diff --git a/modules/billing/tests/billing.reset.service.unit.tests.js b/modules/billing/tests/billing.reset.service.unit.tests.js index 3aa27e631..8d87f7045 100644 --- a/modules/billing/tests/billing.reset.service.unit.tests.js +++ b/modules/billing/tests/billing.reset.service.unit.tests.js @@ -2,6 +2,7 @@ * Module dependencies. */ import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globals'; +import { isoWeekKey } from '../lib/billing.isoWeek.js'; /** * Unit tests for billing.reset.service.js @@ -115,19 +116,19 @@ describe('BillingResetService unit tests:', () => { describe('isoWeekKey', () => { test('should return correct week key for a known Monday', () => { // 2026-04-27 is a Monday, ISO week 18 - const result = BillingResetService.isoWeekKey(new Date('2026-04-27')); + const result = isoWeekKey(new Date('2026-04-27')); expect(result).toBe('2026-W18'); }); test('should return correct week key for a Sunday (same ISO week as preceding Monday)', () => { // 2026-05-03 is Sunday, still ISO week 18 - const result = BillingResetService.isoWeekKey(new Date('2026-05-03')); + const result = isoWeekKey(new Date('2026-05-03')); expect(result).toBe('2026-W18'); }); test('should compute week 1 for first week of year', () => { // 2026-01-01 is Thursday — ISO week 1 of 2026 - const result = BillingResetService.isoWeekKey(new Date('2026-01-01')); + const result = isoWeekKey(new Date('2026-01-01')); expect(result).toBe('2026-W01'); }); }); diff --git a/modules/billing/tests/billing.usage.service.unit.tests.js b/modules/billing/tests/billing.usage.service.unit.tests.js index ea87bad62..afcfd09b7 100644 --- a/modules/billing/tests/billing.usage.service.unit.tests.js +++ b/modules/billing/tests/billing.usage.service.unit.tests.js @@ -415,6 +415,31 @@ describe('BillingUsageService — meter extensions unit tests:', () => { expect(result.alertCrossed).toBeNull(); }); + test('should emit BOTH 80% and 100% when jumping from 0% to 150% in one attribution', async () => { + // Regression test: break was removing the 80% event when jump went past 100%. + // With the fix, the loop continues after marking 100% and also marks 80%. + mockSubscriptionRepository.findPlan.mockResolvedValue({ plan: 'pro' }); + mockPlanService.getActivePlan.mockResolvedValue(makePlan({ meterQuota: 500000 })); + // meterUsed = 750000 → 150% of 500000 → both 80 and 100 thresholds crossed + const updatedDoc = makeUsageDoc({ + meterUsed: 750000, + meterQuota: 500000, + alertedAt80: null, + alertedAt100: null, + }); + mockUsageRepository.incrementMeter.mockResolvedValue(updatedDoc); + // markThreshold called twice — both succeed + mockUsageRepository.markThreshold.mockResolvedValue({ modifiedCount: 1 }); + + const result = await BillingUsageService.incrementMeter(orgId, 750000, {}, 'hist_0to150pct'); + + // alertCrossed should be the last threshold set (80, since sort is DESC: 100 first, then 80) + expect(result.alertCrossed).toBe('80'); + expect(mockUsageRepository.markThreshold).toHaveBeenCalledTimes(2); + expect(mockUsageRepository.markThreshold).toHaveBeenCalledWith(updatedDoc._id, 'alertedAt100'); + expect(mockUsageRepository.markThreshold).toHaveBeenCalledWith(updatedDoc._id, 'alertedAt80'); + }); + test('should NOT set alertCrossed when markThreshold returns modifiedCount=0 (another pod won)', async () => { // markThreshold returns modifiedCount=0 → we lost the race, must not emit mockUsageRepository.markThreshold.mockResolvedValue({ modifiedCount: 0 }); From 577c74436dfffd28f87191ceb40c8cd9a2952e00 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 16:06:42 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(billing):=20address=20Opus=20review=20?= =?UTF-8?q?=E2=80=94=20requireQuota=20uses=20getDefaultPlanId?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - middleware: consolidate plan fallback via getDefaultPlanId() - usage.service: invariant comment on threshold loop ordering - usage.service: stale-memory comment on updatedDoc[field] - README: thresholdPercents notes complete - errors.js: drop YAGNI extension comment --- modules/billing/README.md | 2 +- modules/billing/lib/billing.errors.js | 2 -- modules/billing/middlewares/billing.requireQuota.js | 3 ++- modules/billing/services/billing.usage.service.js | 2 ++ 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/modules/billing/README.md b/modules/billing/README.md index a497c964b..9af4957e4 100644 --- a/modules/billing/README.md +++ b/modules/billing/README.md @@ -118,7 +118,7 @@ Consumers should NOT retry on `applied: true` — the outbox handles eventual co | `billing.outbox.retryIntervalSec` | number | 300 | Cron retry interval | | `billing.crons.jitterMaxMs` | number | 60000 | Cron startup jitter max | | `billing.planChange.preserveUsageDefault` | boolean | true | forceRotateForPlanChange default | -| `billing.alerts.thresholdPercents` | number[] | [80, 100] | Schema-supported only — see thresholdFields | +| `billing.alerts.thresholdPercents` | number[] | [80, 100] | Schema-supported only — others warn at boot, alert silently skipped | | `billing.events.extrasExhausted` | string | 'billing.extras_debit.exhausted' | Event name for downstream alerting | | `billing.defaultPlan` | string | 'free' | Default plan ID for fallback | diff --git a/modules/billing/lib/billing.errors.js b/modules/billing/lib/billing.errors.js index 0e2f2d209..e476b5066 100644 --- a/modules/billing/lib/billing.errors.js +++ b/modules/billing/lib/billing.errors.js @@ -1,5 +1,3 @@ -// Extension point: add E_CAST, E_VALIDATION helpers here as needed - /** * @function isDuplicateKeyError * @description Identify Mongo duplicate-key errors across driver and string-only shapes. diff --git a/modules/billing/middlewares/billing.requireQuota.js b/modules/billing/middlewares/billing.requireQuota.js index 4ce0351ad..9d7af8cf0 100644 --- a/modules/billing/middlewares/billing.requireQuota.js +++ b/modules/billing/middlewares/billing.requireQuota.js @@ -7,6 +7,7 @@ import BillingExtraBalanceRepository from '../repositories/billing.extraBalance. import BillingPlanService from '../services/billing.plan.service.js'; import { activeStatuses } from '../lib/constants.js'; +import { getDefaultPlanId } from '../lib/billing.constants.js'; import config from '../../../config/index.js'; import responses from '../../../lib/helpers/responses.js'; @@ -81,7 +82,7 @@ function requireQuota(resource, action) { // Don't create the doc here — let incrementMeter do it on first attribution. // Fall back to the plan quota so first-run requests are not blocked. // Reuse the `subscription` already fetched by the degraded-mode gate above. - const planId = subscription?.plan ?? config?.billing?.defaultPlan ?? 'free'; + const planId = subscription?.plan ?? getDefaultPlanId(); const activePlan = await BillingPlanService.getActivePlan(planId); // Plan missing (seeding / version bump in progress) → fail safe with 503 diff --git a/modules/billing/services/billing.usage.service.js b/modules/billing/services/billing.usage.service.js index 25fee289a..5ad484297 100644 --- a/modules/billing/services/billing.usage.service.js +++ b/modules/billing/services/billing.usage.service.js @@ -135,12 +135,14 @@ const incrementMeter = async (organizationId, units, breakdown, idempotencyKey) if (effectiveQuota > 0) { const pct = (newMeterUsed / effectiveQuota) * 100; + // loop runs DESC (e.g. [100, 80] from getAlertThresholdPercents()); alertCrossed retains the last (lowest) marked threshold by design. for (const threshold of getAlertThresholdPercents()) { const field = thresholdFields[threshold]; if (!field) { console.warn(`[billing.usage] threshold ${threshold}% has no schema field (only 80/100 are supported) — skipping`); continue; } + // updatedDoc is pre-mark snapshot; DB-side dedup enforced by markThreshold conditional update. if (pct < threshold || updatedDoc[field]) continue; let marked = false; From c94d9866d0de863c34739a42b90b4e751cc2d32c Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 16:19:21 +0200 Subject: [PATCH 3/4] fix(billing): address CodeRabbit/Codacy/Copilot v4 review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - constants: add type guards to getDollarsToUnitRatio (0/NaN → 1000), getMaxUnitsPerOperation (invalid → Infinity), getDefaultPlanId (empty → 'free') - constants: guard getAlertThresholdPercents against string env-override (e.g. DEVKIT_NODE_billing_alerts_thresholdPercents=80 — coerce to [80]) - weeklyReset cron: add mongooseService.loadModels() before connect() to match retry-pending-extras-debit.cron.js pattern (Copilot LYE-) - service: add random suffix to extras_checkout idempotency key to reduce collision risk on concurrent clicks (Codacy LXAx / CodeRabbit LXwS) - README: point defaults source to billing.constants.js, fix maxUnitsPerOperation default (10000 from dev config, Infinity from constant fallback), note ratioVersion is not wrapped in a constant (Copilot LYFL, CodeRabbit LXwQ) - tests: guard tests for new constant fallback behaviours (0/NaN/empty) - tests: add getAlertThresholdPercents string coercion test - tests: weeklyReset cron — replace export parity checks with contract test (applyJitter(getCronJitterMaxMs()) produces valid delay in [0, maxMs)) - tests: 0%→150% regression — add event emit assertions (LXwU) --- modules/billing/README.md | 18 ++++++------ modules/billing/crons/billing.weeklyReset.js | 1 + modules/billing/lib/billing.constants.js | 28 +++++++++++++----- modules/billing/services/billing.service.js | 5 ++-- .../tests/billing.config-knobs.unit.tests.js | 29 +++++++++++++++++++ .../billing.cron.weeklyReset.unit.tests.js | 29 ++++++++++++++----- .../billing.service.extras.unit.tests.js | 2 +- .../tests/billing.usage.service.unit.tests.js | 18 ++++++++++++ 8 files changed, 104 insertions(+), 26 deletions(-) diff --git a/modules/billing/README.md b/modules/billing/README.md index 9af4957e4..d5e057c46 100644 --- a/modules/billing/README.md +++ b/modules/billing/README.md @@ -107,22 +107,22 @@ Consumers should NOT retry on `applied: true` — the outbox handles eventual co ### Configuration knobs -| Knob | Type | Default | Notes | -|------|------|---------|-------| +| Knob | Type | Devkit default | Notes | +|------|------|----------------|-------| | `billing.meter.runBase` | number | 1 | METER_RUN_BASE base unit cost | | `billing.meter.fallbackPlanId` | string \| null | null | Fallback plan when active not resolvable | -| `billing.meter.dollarsToUnitRatio` | number | 1000 | Dollar amount → meter unit conversion | -| `billing.meter.maxUnitsPerOperation` | number | Infinity | Cap per single attribute call | -| `billing.meter.ratioVersion` | string \| null | null | DOWNSTREAM-OVERRIDE-REQUIRED — pricing version namespace | +| `billing.meter.dollarsToUnitRatio` | number | 1000 | Dollar → unit conversion. DOWNSTREAM-OVERRIDE-REQUIRED. Constant fallback: `getDollarsToUnitRatio()` | +| `billing.meter.maxUnitsPerOperation` | number | 10000 | Cap per single attribute call (dev config). Constant fallback: `Infinity` via `getMaxUnitsPerOperation()` | +| `billing.meter.ratioVersion` | string \| null | '2026.05' | DOWNSTREAM-OVERRIDE-REQUIRED — pricing version namespace. Read directly from config, no constant wrapper | | `billing.outbox.maxRetryAttempts` | number | 5 | Outbox retry limit before exhausted | | `billing.outbox.retryIntervalSec` | number | 300 | Cron retry interval | -| `billing.crons.jitterMaxMs` | number | 60000 | Cron startup jitter max | +| `billing.crons.jitterMaxMs` | number | 60000 | Cron startup jitter max. Constant fallback: `getCronJitterMaxMs()` | | `billing.planChange.preserveUsageDefault` | boolean | true | forceRotateForPlanChange default | -| `billing.alerts.thresholdPercents` | number[] | [80, 100] | Schema-supported only — others warn at boot, alert silently skipped | +| `billing.alerts.thresholdPercents` | number[] | [80, 100] | Schema-supported only — others warn at boot, alert silently skipped. Constant fallback: `getAlertThresholdPercents()` | | `billing.events.extrasExhausted` | string | 'billing.extras_debit.exhausted' | Event name for downstream alerting | -| `billing.defaultPlan` | string | 'free' | Default plan ID for fallback | +| `billing.defaultPlan` | string | 'free' | Default plan ID for fallback. Constant fallback: `getDefaultPlanId()` | -Defaults live in `modules/billing/config/billing.development.config.js` and can be overridden by downstream project config: +Canonical constant fallbacks live in `modules/billing/lib/billing.constants.js`. Downstream project overrides go in `modules/billing/config/billing.development.config.js`: ```js billing: { diff --git a/modules/billing/crons/billing.weeklyReset.js b/modules/billing/crons/billing.weeklyReset.js index 0cac3a7e1..7f1ad628c 100644 --- a/modules/billing/crons/billing.weeklyReset.js +++ b/modules/billing/crons/billing.weeklyReset.js @@ -33,6 +33,7 @@ if (!config?.billing?.meterMode) { await applyJitter(getCronJitterMaxMs()); try { + await mongooseService.loadModels(); await mongooseService.connect(); const { default: BillingResetService } = await import('../services/billing.reset.service.js'); diff --git a/modules/billing/lib/billing.constants.js b/modules/billing/lib/billing.constants.js index 8587536fb..6406fa2ea 100644 --- a/modules/billing/lib/billing.constants.js +++ b/modules/billing/lib/billing.constants.js @@ -78,8 +78,11 @@ export const getPlanChangePreserveUsageDefault = () => * @returns {number[]} Sorted threshold percentages. */ export const getAlertThresholdPercents = () => { - const thresholds = config?.billing?.alerts?.thresholdPercents ?? DEFAULT_ALERT_THRESHOLD_PERCENTS; + const raw = config?.billing?.alerts?.thresholdPercents ?? DEFAULT_ALERT_THRESHOLD_PERCENTS; + // Guard against env-override delivering a string (e.g. DEVKIT_NODE_billing_alerts_thresholdPercents=80) + const thresholds = Array.isArray(raw) ? raw : [raw].map(Number).filter(Number.isFinite); return thresholds + .map(Number) .filter((threshold) => Number.isFinite(threshold) && threshold > 0) .sort((a, b) => b - a); }; @@ -87,26 +90,37 @@ export const getAlertThresholdPercents = () => { /** * @function getDollarsToUnitRatio * @description Resolve the configured conversion factor from dollar amounts to meter units. + * Returns the raw config value when valid; falls back to 1000. * @returns {number} Dollar-to-unit ratio (e.g. 1000 means $1 = 1000 units). */ -export const getDollarsToUnitRatio = () => - config?.billing?.meter?.dollarsToUnitRatio ?? 1000; +export const getDollarsToUnitRatio = () => { + const ratio = Number(config?.billing?.meter?.dollarsToUnitRatio); + return Number.isFinite(ratio) && ratio > 0 ? ratio : 1000; +}; /** * @function getMaxUnitsPerOperation * @description Resolve the configured per-operation unit cap. Infinity means no cap. + * Returns the raw config value when valid (positive finite or Infinity); falls back to Infinity. * @returns {number} Maximum units allowed for a single attribute call. */ -export const getMaxUnitsPerOperation = () => - config?.billing?.meter?.maxUnitsPerOperation ?? Infinity; +export const getMaxUnitsPerOperation = () => { + const raw = config?.billing?.meter?.maxUnitsPerOperation; + if (raw === undefined || raw === null) return Infinity; + const cap = Number(raw); + return Number.isFinite(cap) && cap > 0 ? cap : Infinity; +}; /** * @function getDefaultPlanId * @description Resolve the default plan ID used as a fallback when no active subscription exists. + * Returns the raw config value when non-empty string; falls back to 'free'. * @returns {string} Default plan identifier. */ -export const getDefaultPlanId = () => - config?.billing?.defaultPlan ?? 'free'; +export const getDefaultPlanId = () => { + const planId = config?.billing?.defaultPlan; + return typeof planId === 'string' && planId.trim() ? planId : 'free'; +}; /** * @function getExtrasExhaustedEventName diff --git a/modules/billing/services/billing.service.js b/modules/billing/services/billing.service.js index 5f96a8162..aac84b2de 100644 --- a/modules/billing/services/billing.service.js +++ b/modules/billing/services/billing.service.js @@ -193,8 +193,9 @@ const createExtrasCheckout = async (organization, packId, successUrl, cancelUrl) const subscription = await _ensureStripeCustomer(stripe, organization); - // Use a timestamped idempotency key (~1ms granularity — insufficient for double-click protection, known design) - const idempotencyKey = `extras_checkout_${String(organization._id)}_${packId}_${Date.now()}`; + // Per-intent idempotency key: timestamp + random suffix reduces collision risk under concurrent clicks. + // Full deduplication would require a caller-provided stable intent id — deferred to a future improvement. + const idempotencyKey = `extras_checkout_${String(organization._id)}_${packId}_${Date.now()}_${Math.random().toString(36).slice(2, 7)}`; const extrasCheckoutParams = { customer: subscription.stripeCustomerId, diff --git a/modules/billing/tests/billing.config-knobs.unit.tests.js b/modules/billing/tests/billing.config-knobs.unit.tests.js index ae5b6652c..eada48299 100644 --- a/modules/billing/tests/billing.config-knobs.unit.tests.js +++ b/modules/billing/tests/billing.config-knobs.unit.tests.js @@ -68,6 +68,15 @@ describe('billing config knob helpers:', () => { expect(constants.getDollarsToUnitRatio()).toBe(1000); }); + test('getDollarsToUnitRatio falls back to 1000 on invalid config (0, negative, NaN)', () => { + mockConfig.billing.meter.dollarsToUnitRatio = 0; + expect(constants.getDollarsToUnitRatio()).toBe(1000); + mockConfig.billing.meter.dollarsToUnitRatio = -5; + expect(constants.getDollarsToUnitRatio()).toBe(1000); + mockConfig.billing.meter.dollarsToUnitRatio = NaN; + expect(constants.getDollarsToUnitRatio()).toBe(1000); + }); + test('getMaxUnitsPerOperation reads from config', () => { mockConfig.billing.meter.maxUnitsPerOperation = 25000; expect(constants.getMaxUnitsPerOperation()).toBe(25000); @@ -78,6 +87,13 @@ describe('billing config knob helpers:', () => { expect(constants.getMaxUnitsPerOperation()).toBe(Infinity); }); + test('getMaxUnitsPerOperation falls back to Infinity on invalid config (0, negative, NaN)', () => { + mockConfig.billing.meter.maxUnitsPerOperation = 0; + expect(constants.getMaxUnitsPerOperation()).toBe(Infinity); + mockConfig.billing.meter.maxUnitsPerOperation = -100; + expect(constants.getMaxUnitsPerOperation()).toBe(Infinity); + }); + test('getDefaultPlanId reads from config', () => { mockConfig.billing.defaultPlan = 'starter'; expect(constants.getDefaultPlanId()).toBe('starter'); @@ -88,6 +104,19 @@ describe('billing config knob helpers:', () => { expect(constants.getDefaultPlanId()).toBe('free'); }); + test('getDefaultPlanId falls back to free on empty/non-string config', () => { + mockConfig.billing.defaultPlan = ''; + expect(constants.getDefaultPlanId()).toBe('free'); + mockConfig.billing.defaultPlan = ' '; + expect(constants.getDefaultPlanId()).toBe('free'); + }); + + test('getAlertThresholdPercents coerces string env value to number (env-override guard)', () => { + // env vars deliver a string like '80' — should not throw, should return [80] + mockConfig.billing.alerts = { thresholdPercents: '80' }; + expect(constants.getAlertThresholdPercents()).toEqual([80]); + }); + test('keeps backward-compatible defaults and runBaseUnits alias', async () => { mockConfig.billing = { plans: ['free'], diff --git a/modules/billing/tests/billing.cron.weeklyReset.unit.tests.js b/modules/billing/tests/billing.cron.weeklyReset.unit.tests.js index ff189b96d..027900833 100644 --- a/modules/billing/tests/billing.cron.weeklyReset.unit.tests.js +++ b/modules/billing/tests/billing.cron.weeklyReset.unit.tests.js @@ -11,16 +11,31 @@ import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globa * script file directly (which would require a live DB connection). */ -describe('billing.weeklyReset cron — applyJitter registration:', () => { - test('applyJitter is exported from billing.cron-utils (parity check with other crons)', async () => { +describe('billing.weeklyReset cron — applyJitter contract:', () => { + test('applyJitter honours getCronJitterMaxMs bound: returns delay in [0, maxMs)', async () => { + // Contract test: weeklyReset calls applyJitter(getCronJitterMaxMs()). + // Verify the two helpers interoperate correctly (applyJitter respects the configured bound). + jest.resetModules(); + + const mockConfig = { billing: { meterMode: true, crons: { jitterMaxMs: 100 } } }; + jest.unstable_mockModule('../../../config/index.js', () => ({ default: mockConfig })); + + const { getCronJitterMaxMs } = await import('../lib/billing.constants.js'); const { applyJitter } = await import('../lib/billing.cron-utils.js'); - expect(typeof applyJitter).toBe('function'); + + const maxMs = getCronJitterMaxMs(); + expect(maxMs).toBe(100); + + const delay = await applyJitter(maxMs); + expect(delay).toBeGreaterThanOrEqual(0); + expect(delay).toBeLessThan(maxMs); }); - test('getCronJitterMaxMs is exported from billing.constants (used in weeklyReset jitter call)', async () => { - const { getCronJitterMaxMs } = await import('../lib/billing.constants.js'); - expect(typeof getCronJitterMaxMs).toBe('function'); - expect(typeof getCronJitterMaxMs()).toBe('number'); + test('applyJitter returns 0 for invalid (non-positive) jitter max — safe no-op', async () => { + const { applyJitter } = await import('../lib/billing.cron-utils.js'); + expect(await applyJitter(0)).toBe(0); + expect(await applyJitter(-1)).toBe(0); + expect(await applyJitter(NaN)).toBe(0); }); }); diff --git a/modules/billing/tests/billing.service.extras.unit.tests.js b/modules/billing/tests/billing.service.extras.unit.tests.js index f89fbba26..af949fe55 100644 --- a/modules/billing/tests/billing.service.extras.unit.tests.js +++ b/modules/billing/tests/billing.service.extras.unit.tests.js @@ -187,7 +187,7 @@ describe('BillingService.createExtrasCheckout unit tests:', () => { ); const [, options] = mockStripeInstance.checkout.sessions.create.mock.calls[0]; - expect(options.idempotencyKey).toMatch(/^extras_checkout_.*pack_2m_\d+$/); + expect(options.idempotencyKey).toMatch(/^extras_checkout_.*pack_2m_\d+_[a-z0-9]+$/); }); test('should set stripeSessionId to __pending__ in metadata', async () => { diff --git a/modules/billing/tests/billing.usage.service.unit.tests.js b/modules/billing/tests/billing.usage.service.unit.tests.js index afcfd09b7..68b5cb095 100644 --- a/modules/billing/tests/billing.usage.service.unit.tests.js +++ b/modules/billing/tests/billing.usage.service.unit.tests.js @@ -13,6 +13,7 @@ describe('BillingUsageService — meter extensions unit tests:', () => { let mockSubscriptionRepository; let mockMeterOutboxRepository; let mockConfig; + let mockBillingEventsEmit; const orgId = '507f1f77bcf86cd799439011'; @@ -86,6 +87,11 @@ describe('BillingUsageService — meter extensions unit tests:', () => { findByIdempotencyKey: jest.fn(), }; + mockBillingEventsEmit = jest.fn(); + jest.unstable_mockModule('../lib/events.js', () => ({ + default: { emit: mockBillingEventsEmit, on: jest.fn(), off: jest.fn() }, + })); + jest.unstable_mockModule('../../../config/index.js', () => ({ default: mockConfig, })); @@ -438,6 +444,18 @@ describe('BillingUsageService — meter extensions unit tests:', () => { expect(mockUsageRepository.markThreshold).toHaveBeenCalledTimes(2); expect(mockUsageRepository.markThreshold).toHaveBeenCalledWith(updatedDoc._id, 'alertedAt100'); expect(mockUsageRepository.markThreshold).toHaveBeenCalledWith(updatedDoc._id, 'alertedAt80'); + // Assert both meter.threshold_crossed events were emitted (100 first, then 80) + expect(mockBillingEventsEmit).toHaveBeenCalledTimes(2); + expect(mockBillingEventsEmit).toHaveBeenNthCalledWith( + 1, + 'meter.threshold_crossed', + expect.objectContaining({ threshold: 100 }), + ); + expect(mockBillingEventsEmit).toHaveBeenNthCalledWith( + 2, + 'meter.threshold_crossed', + expect.objectContaining({ threshold: 80 }), + ); }); test('should NOT set alertCrossed when markThreshold returns modifiedCount=0 (another pod won)', async () => { From a5b53291bee2573394306f66f982e5fd524b19d9 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 3 May 2026 16:27:10 +0200 Subject: [PATCH 4/4] fix(billing): use crypto.randomBytes for idempotency key nonce Math.random() in the idempotency key was flagged by Codacy as insecure random in a security-sensitive context. Switch to node:crypto randomBytes(4) which produces a cryptographically secure 8-char hex suffix. --- modules/billing/services/billing.service.js | 5 +++-- modules/billing/tests/billing.service.extras.unit.tests.js | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/modules/billing/services/billing.service.js b/modules/billing/services/billing.service.js index aac84b2de..52308c22e 100644 --- a/modules/billing/services/billing.service.js +++ b/modules/billing/services/billing.service.js @@ -1,6 +1,7 @@ /** * Module dependencies */ +import { randomBytes } from 'node:crypto'; import config from '../../../config/index.js'; import getStripe from '../lib/stripe.js'; import BillingPlansService from './billing.plans.service.js'; @@ -193,9 +194,9 @@ const createExtrasCheckout = async (organization, packId, successUrl, cancelUrl) const subscription = await _ensureStripeCustomer(stripe, organization); - // Per-intent idempotency key: timestamp + random suffix reduces collision risk under concurrent clicks. + // Per-intent idempotency key: timestamp + crypto-random suffix reduces collision risk under concurrent clicks. // Full deduplication would require a caller-provided stable intent id — deferred to a future improvement. - const idempotencyKey = `extras_checkout_${String(organization._id)}_${packId}_${Date.now()}_${Math.random().toString(36).slice(2, 7)}`; + const idempotencyKey = `extras_checkout_${String(organization._id)}_${packId}_${Date.now()}_${randomBytes(4).toString('hex')}`; const extrasCheckoutParams = { customer: subscription.stripeCustomerId, diff --git a/modules/billing/tests/billing.service.extras.unit.tests.js b/modules/billing/tests/billing.service.extras.unit.tests.js index af949fe55..c177002bc 100644 --- a/modules/billing/tests/billing.service.extras.unit.tests.js +++ b/modules/billing/tests/billing.service.extras.unit.tests.js @@ -187,7 +187,7 @@ describe('BillingService.createExtrasCheckout unit tests:', () => { ); const [, options] = mockStripeInstance.checkout.sessions.create.mock.calls[0]; - expect(options.idempotencyKey).toMatch(/^extras_checkout_.*pack_2m_\d+_[a-z0-9]+$/); + expect(options.idempotencyKey).toMatch(/^extras_checkout_.*pack_2m_\d+_[0-9a-f]+$/); }); test('should set stripeSessionId to __pending__ in metadata', async () => {