From 120cc6bedb28ba767c517e1fd68b1d258c6d1d0e Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Tue, 28 Jul 2026 16:04:07 +0200 Subject: [PATCH 1/2] fix(billing): weekKey compound-sparse index collapses legacy usage to one doc per org (#3991) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The (organizationId, weekKey) unique index declared sparse: true on a COMPOUND index — MongoDB only excludes a doc from a compound sparse index when ALL indexed fields are missing, and organizationId is always present. Every legacy (weekKey-less) usage document was indexed too (weekKey: null), so a second legacy month for the same org collided as a duplicate key and increment()'s retry silently lost the write. - Model: partialFilterExpression { weekKey: { $exists: true } }, explicit distinct name (avoids an IndexOptionsConflict boot-crash against the still-live old-named index on already-deployed DBs). - New migration: creates the new index alongside the old one, then drops the old one — dup pre-check, idempotent, skip-window fast path, E11000-catch abort, mirroring the #3990 migration's safety patterns. - Service: increment() now logs loud (with context) instead of silently swallowing a lost write on the (now anomalous) null case. - Regression + migration integration tests, MIGRATIONS.md entry. --- MIGRATIONS.md | 33 +++ ...8120000-fix-usage-weekkey-index-partial.js | 230 +++++++++++++++ .../models/billing.usage.model.mongoose.js | 40 ++- .../repositories/billing.usage.repository.js | 8 +- .../billing/services/billing.usage.service.js | 33 ++- ....usage.bootIndexReady.integration.tests.js | 27 +- ...ling.usage.repository.integration.tests.js | 2 +- .../tests/billing.usage.service.unit.tests.js | 42 +++ ...rtialFilter.migration.integration.tests.js | 271 ++++++++++++++++++ 9 files changed, 678 insertions(+), 8 deletions(-) create mode 100644 modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js create mode 100644 modules/billing/tests/billing.usage.weekKeyIndexPartialFilter.migration.integration.tests.js diff --git a/MIGRATIONS.md b/MIGRATIONS.md index ba9909f7a..e2a57982f 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -4,6 +4,39 @@ Breaking changes and upgrade notes for downstream projects. --- +## Billing usage weekKey-index partial-filter fix (2026-07-28) + +Fixes a silent write-loss bug: the meter-mode `(organizationId, weekKey)` unique index on `billingusages` declared `sparse: true` on a COMPOUND index. MongoDB's sparse-exclusion rule for a compound index only skips a document when it is missing **ALL** indexed fields — since `organizationId` is always present (on legacy AND meter-mode documents alike), sparse never excluded anything: every legacy (weekKey-less) document was indexed too, with `weekKey` treated as `null`. A second legacy document for the **same organization** — regardless of month — then collided on `{organizationId, weekKey: null}` and was rejected as a duplicate key; `BillingUsageRepository.increment`'s duplicate-key retry filter (`{organizationId, month}`) matched nothing for the new month, so the write silently resolved to `null` with **no error surfaced**. Net effect: under `meterMode: false` (the default), a downstream consumer's legacy usage counters could only ever be recorded for the **first** month an organization was active — every subsequent month's write was lost. + +### What changed (this repo) + +- **`modules/billing/models/billing.usage.model.mongoose.js`** — the `(organizationId, weekKey)` index now declares `partialFilterExpression: { weekKey: { $exists: true } }` instead of `sparse: true`, and an **explicit distinct name** `organizationId_1_weekKey_1_partial` (was the default `organizationId_1_weekKey_1`). The distinct name is required, not cosmetic: the old `sparse: true` spec is valid MongoDB syntax and is already LIVE on every deployed database (unlike #3990's invalid spec, which never built anywhere) — reusing the same default name would make autoIndex (which now runs BEFORE migrations and surfaces build failures loudly, #3990) reject with `IndexOptionsConflict` on every boot until an operator manually dropped the old index, a self-inflicted boot-crash loop. The distinct name lets the new index build ALONGSIDE the still-live old one with no conflict (mirrors `modules/users/migrations/20260610120000-users-email-ci-unique-index.js`'s coexistence technique). +- **New migration `modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js`** — the authoritative creator (new index) + old-index dropper on already-deployed databases: **(a)** duplicate pre-check on the meter shape (`weekKey` present, grouped by `organizationId`+`weekKey`, count>1), abort loud on any pre-existing duplicate; **(b)** create the new partial index first (idempotent — skipped if already exact-spec), so there is never a window without a uniqueness guard on meter-mode documents; **(c)** drop the old sparse index (and any other divergent same-key index). Skip-window fast path when the new index is already exact-spec and the old one is already gone (steady-state no-op). A duplicate-key error on the final `createIndex` call (a concurrent race landing a duplicate between the pre-check and the create) is caught and converted into the same actionable abort as the pre-check, never a bare driver error. Idempotent on re-run. +- **`modules/billing/services/billing.usage.service.js`** — `increment()` now logs `logger.error` with full context (`organizationId`, `month`, `key`, `amount`) whenever the repository's duplicate-key retry matches nothing (an anomaly that should not occur post-fix, but is no longer silently invisible if it ever does — silent-catch convention: a swallowed write failure must never be invisible). Does not throw — no caller (in this repo or any downstream consumer, since `increment` is public API) ever treated a non-null return as guaranteed, and throwing would be a breaking behavior change for a generic stack module. + +### Pre-deploy duplicate-data audit (downstreams using meter-mode billing usage) + +Run this against your production `billingusages` collection **before** deploying this change. Any result means the migration will abort boot until you remediate (delete/merge the offending rows) — better to catch it ahead of time: + +```js +db.billingusages.aggregate([ + { $match: { weekKey: { $exists: true } } }, + { $group: { _id: { organizationId: '$organizationId', weekKey: '$weekKey' }, count: { $sum: 1 }, ids: { $push: '$_id' } } }, + { $match: { count: { $gt: 1 } } }, +]); +``` + +In practice this should always return empty — the old `sparse: true` index already enforced uniqueness correctly for documents that DO have `weekKey` set (sparse behaves as intended when the field is present); this audit is a defensive pre-check, not a known-affected case. Downstreams running exclusively in legacy (non-meter) mode will also always get an empty result (no document ever has `weekKey` set). + +### Action required for downstream projects (`/update-stack`) + +1. All changes are devkit-owned stack files → arrive via `/update-stack` (`--theirs`). +2. Run the duplicate-data audit above against prod before deploying — expected empty, but confirm. +3. No manual index action needed: the migration creates the new index and drops the old one automatically at boot, with no window where meter-mode documents lack a uniqueness guard. +4. If your downstream project runs `meterMode: false` (the default) and has been live for more than one month per organization, expect this migration to un-block legacy usage tracking that was previously silently stuck at month one — verify your usage dashboards after deploying. + +--- + ## Boot awaits index builds; billing usage month-index partial-filter fix (2026-07-27) Fixes a silent index-creation failure: the legacy `(organizationId, month)` unique index on `billingusages` declared `partialFilterExpression: { weekKey: { $exists: false } }`, which MongoDB does not support (only `$eq`, `$exists: true`, `$gt`, `$gte`, `$lt`, `$lte`, `$type`, and top-level `$and` are allowed inside a partial filter). Mongoose autoIndex reported the creation failure on the model's unlistened `'index'` event, so the index **never existed on any deployed database** — the uniqueness guard ran on application code alone (a racy upsert). While fixing it, boot itself was hardened: it no longer treats `mongoose.connect()` resolving as "ready" (autoIndex builds run in the background). diff --git a/modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js b/modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js new file mode 100644 index 000000000..244430617 --- /dev/null +++ b/modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js @@ -0,0 +1,230 @@ +/** + * Module dependencies + */ +import mongoose from 'mongoose'; + +const OLD_INDEX_NAME = 'organizationId_1_weekKey_1'; +const NEW_INDEX_NAME = 'organizationId_1_weekKey_1_partial'; +const INDEX_KEY = { organizationId: 1, weekKey: 1 }; + +/** + * @desc Exact-key match helper: a two-field index keyed (organizationId:1, weekKey:1) + * in that order. + * @param {Object} ix - an index document from listIndexes() + * @returns {boolean} true when the index key is exactly { organizationId:1, weekKey:1 } + */ +const sameKey = (ix) => { + const keys = Object.keys(ix.key || {}); + return keys.length === 2 && keys[0] === 'organizationId' && keys[1] === 'weekKey' + && ix.key.organizationId === 1 && ix.key.weekKey === 1; +}; + +/** + * @desc Exact-spec match helper: `sameKey` PLUS the correct name, uniqueness, + * and partialFilterExpression — the full target shape this migration installs. + * @param {Object} ix - an index document from listIndexes() + * @returns {boolean} true when ix is already the fully-installed target index + */ +const isExactTargetIndex = (ix) => + sameKey(ix) && ix.name === NEW_INDEX_NAME && ix.unique === true && ix.partialFilterExpression?.weekKey?.$exists === true; + +/** + * @desc Group meter-mode documents (weekKey present) by (organizationId, + * weekKey) and return only groups with more than one document — the shape the + * unique partial index would reject. Shared by the upfront pre-check (a) and + * the post-createIndex E11000 re-derivation, so both report the identical set. + * @param {import('mongodb').Collection} usages - the billingusages collection. + * @returns {Promise>} + */ +const findWeekKeyDuplicates = (usages) => + usages + .aggregate([ + { $match: { weekKey: { $exists: true } } }, + { $group: { _id: { organizationId: '$organizationId', weekKey: '$weekKey' }, count: { $sum: 1 }, ids: { $push: '$_id' } } }, + { $match: { count: { $gt: 1 } } }, + ]) + .toArray(); + +/** + * Migration: working (organizationId, weekKey) meter-mode partial-unique index (#3991). + * + * The schema previously declared `sparse: true` on this COMPOUND index. MongoDB's + * sparse-exclusion rule for a compound index only skips a document when it is + * missing ALL indexed fields — since `organizationId` is always present (on + * legacy AND meter-mode documents alike), sparse never excluded anything: every + * legacy (weekKey-less) document was indexed too, with weekKey treated as + * `null`. A second legacy document for the SAME org (any month) collided on + * `{organizationId, weekKey: null}` and was rejected as a duplicate key; + * `BillingUsageRepository.increment`'s duplicate-key retry filter + * (`{organizationId, month}`) then matched nothing for the new month, so the + * write silently resolved to `null` with no document ever created — legacy + * usage tracking stopped after an organization's first active month. Unlike + * #3990 (an invalid `$exists: false` partial filter that never built ANYWHERE), + * this `sparse: true` spec IS valid MongoDB syntax — it built successfully and + * is LIVE on every already-deployed database, just with the wrong semantics. + * + * New spec (the schema declares the IDENTICAL twin): unique on + * { organizationId: 1, weekKey: 1 }, partialFilterExpression + * { weekKey: { $exists: true } } — only meter-mode documents (which always set + * weekKey) are covered. + * + * Distinct name, NOT a drop-then-recreate-under-the-same-name swap (#3990's + * approach does not apply here): because the OLD index is already live and + * valid, giving the new spec the SAME default name + * (`organizationId_1_weekKey_1`) would make mongoose's autoIndex — which now + * runs BEFORE migrations and SURFACES build failures loudly (#3990's + * `awaitIndexBuilds()`) — reject with IndexOptionsConflict (same name, + * different options as the still-live old index) on the very FIRST boot after + * this schema change deploys, crashing bootstrap before `migrations.run()` + * ever executes. That would be a self-inflicted boot-crash loop: the migration + * that is supposed to drop the old index never gets a chance to run. Naming + * the new index `organizationId_1_weekKey_1_partial` lets autoIndex build it + * ALONGSIDE the old one without conflict (two indexes on the same key under + * different names coexist in MongoDB — mirrors the exact technique in + * `modules/users/migrations/20260610120000-users-email-ci-unique-index.js`, + * used there for the same class of problem: swapping a live unique index's + * options without a name collision). + * + * Ordering: + * (a) Duplicate pre-check FIRST, ZERO writes. Group the meter shape + * (weekKey present) by (organizationId, weekKey), abort loud on any + * count > 1. Defensive: the OLD sparse index already enforced + * uniqueness among weekKey-bearing documents (sparse behaves correctly + * when the field IS present), so no duplicate should exist here in + * practice — but this migration must not assume that and must never + * silently swallow a violation if one somehow does. + * (b) Create the NEW partial index FIRST (idempotent — skipped if already + * exact-spec). Safe to do while the OLD index is still live: it only + * indexes documents where weekKey exists, and (a) already proved no + * duplicate exists among those. There is never a window without SOME + * uniqueness constraint on meter-mode documents. + * (c) Drop the OLD index (by its default name, or any other divergent + * same-key index) now that the NEW one is live and enforcing. + * Idempotent on re-run: a second `up()` finds the new index already exact-spec + * and the old index already absent — the skip-window fast path below makes it + * a no-op read. + * + * Skip-window fast path: if the new index is already the exact target shape + * AND the old index is already gone, the whole create/drop sequence is + * skipped entirely. This is the common case on every boot after the first one + * that ever ran this migration to completion on a given database (including + * every other instance in a rolling deploy racing this same migration on the + * SAME target database). + * + * Residual unguarded window (HONEST — not fully eliminated): a duplicate-key + * error on the final createIndex call (a concurrent write landing a + * genuine duplicate weekKey pair between the pre-check and the create) is + * caught and converted into the same loud, actionable abort as (a) — never a + * bare driver error — so the failure mode this migration produces is always + * the documented "abort loud, remediate, re-run" path. `runMigration` + * (lib/services/migrations.js) unclaims on throw, so the very next boot + * retries, and (a)'s pre-check now sees the race-created duplicate and aborts + * with full detail before touching the index at all. + * + * autoIndex race: mongoose autoIndex:true (the default — db.options sets no + * override) builds the schema-declared twin (same name) on connect; identical + * specs make the race benign. This migration is the AUTHORITATIVE creator + + * old-index dropper on already-deployed databases (it owns the drop autoIndex + * never performs — autoIndex only adds/rebuilds schema-declared indexes, it + * never removes indexes the schema no longer declares). + * + * @returns {Promise} + */ +export async function up() { + const usages = mongoose.connection.db.collection('billingusages'); + + // ── (a) Duplicate pre-check FIRST — zero writes so far. `.aggregate()` on a + // missing/empty collection just returns an empty result, so this is also + // safe on a fresh database. + const duplicates = await findWeekKeyDuplicates(usages); + + if (duplicates.length > 0) { + // Emit only usage document ids — never organization identities — in + // operator-facing errors. + const sample = duplicates + .slice(0, 10) + .map((d) => `(${d.count} docs, ids: ${d.ids.slice(0, 3).join(',')}${d.ids.length > 3 ? ',…' : ''})`) + .join('; '); + throw new Error( + `[migration] usage-weekkey-index-partial ABORTED: ${duplicates.length} duplicate meter-mode usage (organizationId, weekKey) group(s) would violate the unique index. ` + + `Remediate (delete/merge the duplicate rows) before re-running — this migration will NOT pick winners. Sample (ids only): ${sample}`, + ); + } + + // Snapshot existing indexes once (listIndexes throws if the collection does not + // exist yet — tolerate that: a fresh DB has no billingusages collection and + // autoIndex / syncIndexes will create the index from the schema declaration). + let existing = []; + try { + existing = await usages.listIndexes().toArray(); + } catch (err) { + if (err?.codeName === 'NamespaceNotFound' || err?.code === 26) { + console.info('[migration] usage-weekkey-index-partial: billingusages collection does not exist yet — nothing to migrate'); + return; + } + throw err; + } + + // ── Skip-window fast path ── If the new index is already the exact target + // shape AND the old index is already gone, there is nothing left to do. + const oldStillPresent = existing.some((ix) => ix.name === OLD_INDEX_NAME); + if (existing.some(isExactTargetIndex) && !oldStillPresent) { + console.info('[migration] usage-weekkey-index-partial: new index already matches the target spec and the old index is already gone — skipping entirely'); + return; + } + + // ── (b) Create the NEW partial index FIRST (idempotent) — before dropping + // the old one, so there is never a window without SOME uniqueness + // constraint on meter-mode documents. + if (!existing.some(isExactTargetIndex)) { + try { + await usages.createIndex(INDEX_KEY, { + unique: true, + name: NEW_INDEX_NAME, + partialFilterExpression: { weekKey: { $exists: true } }, + }); + console.info('[migration] usage-weekkey-index-partial: created partial-unique index on (organizationId, weekKey)'); + } catch (err) { + if (err?.code !== 11000) throw err; + + const raceDuplicates = await findWeekKeyDuplicates(usages); + const sample = raceDuplicates.length > 0 + ? raceDuplicates + .slice(0, 10) + .map((d) => `(${d.count} docs, ids: ${d.ids.slice(0, 3).join(',')}${d.ids.length > 3 ? ',…' : ''})`) + .join('; ') + : `could not re-derive the offending pair(s) from a fresh aggregate — raw driver error: ${err.message}`; + throw new Error( + `[migration] usage-weekkey-index-partial ABORTED on index create: a duplicate (organizationId, weekKey) pair landed during migration — ` + + `a concurrent write racing this migration. Remediate (delete/merge the duplicate rows) and re-run — the next boot's pre-check will catch this up front. Sample: ${sample}`, + ); + } + } else { + console.info('[migration] usage-weekkey-index-partial: partial-unique index already present — skipping create'); + } + + // ── (c) Drop the OLD index (and any other divergent same-key index) now + // that the new one is live and enforcing. ── + for (const ix of existing) { + if (ix.name === '_id_' || ix.name === NEW_INDEX_NAME) continue; + if (sameKey(ix) || ix.name === OLD_INDEX_NAME) { + await usages.dropIndex(ix.name); + console.info(`[migration] usage-weekkey-index-partial: dropped legacy index '${ix.name}' (sparse, superseded by the partial index)`); + } + } +} + +/** + * Down: no-op (warn). The pre-fix state was a compound sparse index that + * silently collapsed legacy usage to one document per organization forever — + * restoring it would reintroduce the bug. Rollback = revert the schema + * declaration deliberately, then drop the partial index by hand if truly + * needed. + * + * @returns {void} + */ +export function down() { + console.warn( + '[migration] usage-weekkey-index-partial DOWN: no-op; drop the (organizationId, weekKey) partial index manually only alongside a deliberate schema revert', + ); +} diff --git a/modules/billing/models/billing.usage.model.mongoose.js b/modules/billing/models/billing.usage.model.mongoose.js index c356a333f..6fc4c851e 100644 --- a/modules/billing/models/billing.usage.model.mongoose.js +++ b/modules/billing/models/billing.usage.model.mongoose.js @@ -158,10 +158,44 @@ UsageMongoose.index( ); /** - * Meter-mode unique index: (organizationId, weekKey) — sparse so it only - * indexes documents that have weekKey populated (meter-mode docs only). + * Meter-mode unique index: (organizationId, weekKey). + * + * Partial filter: `{ weekKey: { $exists: true } }` — only meter-mode documents + * (which always set weekKey) are covered by this uniqueness constraint. The + * previous `sparse: true` looked equivalent but is WRONG for a COMPOUND index: + * MongoDB's sparse-exclusion rule only skips a document from a compound sparse + * index when it is missing ALL indexed fields, not just some. `organizationId` + * is always present on every document (legacy or meter), so sparse never + * excluded anything here — every legacy (weekKey-less) document was indexed + * too, with weekKey treated as `null`. A second legacy document for the SAME + * org (any month) then collided on `{organizationId, weekKey: null}` and was + * rejected as a duplicate key — `BillingUsageRepository.increment`'s + * duplicate-key retry filter (`{organizationId, month}`) matched nothing for + * the new month, so the write silently resolved to `null` with no document + * ever created. Net effect: under `meterMode: false` (the default), legacy + * usage could only ever be recorded for the FIRST month an organization was + * active (#3991). + * + * Explicit name `organizationId_1_weekKey_1_partial` — deliberately DIFFERENT + * from the default `organizationId_1_weekKey_1` the old `sparse: true` spec + * used, so the new partial index can be created ALONGSIDE the still-live old + * one on an already-deployed database without an IndexOptionsConflict. Boot + * now awaits + surfaces index-build failures loudly (#3990's + * `awaitIndexBuilds()`, which runs BEFORE migrations): reusing the same + * default name here would make `createIndex` reject with IndexOptionsConflict + * (same name, different options as the live old index) on every boot until an + * operator manually dropped the old index out of band — a self-inflicted + * boot-crash loop, since the migration that is supposed to drop it never gets + * a chance to run. Mirrors the distinct-name coexistence technique in + * `modules/users/migrations/20260610120000-users-email-ci-unique-index.js`. + * Migration `modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js` + * is the authoritative creator (new index) + old-index dropper on + * already-deployed databases. */ -UsageMongoose.index({ organizationId: 1, weekKey: 1 }, { unique: true, sparse: true }); +UsageMongoose.index( + { organizationId: 1, weekKey: 1 }, + { unique: true, name: 'organizationId_1_weekKey_1_partial', partialFilterExpression: { weekKey: { $exists: true } } }, +); /** * TTL index: automatically purge archived usage documents after 1 year. diff --git a/modules/billing/repositories/billing.usage.repository.js b/modules/billing/repositories/billing.usage.repository.js index e91c007ec..46fcbf4bf 100644 --- a/modules/billing/repositories/billing.usage.repository.js +++ b/modules/billing/repositories/billing.usage.repository.js @@ -31,7 +31,13 @@ const get = (organizationId, month) => { * @param {String} month - The month in YYYY-MM format. * @param {String} key - The counter key to increment (e.g. 'executions'). * @param {Number} amount - The amount to increment by. - * @returns {Promise} The updated usage document. + * @returns {Promise} The updated usage document, or (anomalous — + * see `BillingUsageService.increment`, which logs this loudly) `null` when a + * duplicate-key retry's exact-match filter finds nothing, meaning the write + * was lost. Should not happen in normal operation post-#3991 (the retry + * filter is `{organizationId, month}`, identical to what the winning + * concurrent upsert just created), but is not asserted against here — the + * repository stays a thin data layer; callers decide how loud to be. */ const increment = async (organizationId, month, key, amount) => { if (!mongoose.Types.ObjectId.isValid(organizationId)) return null; diff --git a/modules/billing/services/billing.usage.service.js b/modules/billing/services/billing.usage.service.js index 07ee247c1..e0563eb72 100644 --- a/modules/billing/services/billing.usage.service.js +++ b/modules/billing/services/billing.usage.service.js @@ -29,12 +29,41 @@ const thresholdFields = { /** * @desc Increment a usage counter for the given organization (current month). + * Hardens the repository's silent-null anomaly (#3991 follow-up): + * `UsageRepository.increment` can return `null` when its + * duplicate-key retry's exact-match filter finds nothing — meaning + * the write was lost. This should not happen in normal operation + * post-#3991 (the (organizationId, weekKey) index no longer + * collides across legacy documents for the same org, and the + * retry filter exactly matches what a winning concurrent upsert + * on the SAME org+month would have just created), so hitting it + * now signals a genuine anomaly worth operator visibility — not a + * thrown error, since no caller in this repo (or, being public + * devkit API, any downstream consumer we cannot audit here) ever + * treated a non-null return as guaranteed, and throwing would be + * a breaking behavior change for a generic stack module. Per the + * silent-catch convention (a swallowed write failure must never + * be invisible), this converts the silent null into a LOUD, + * logged one instead. * @param {String} organizationId - The organization ID. * @param {String} key - The counter key to increment. * @param {Number} amount - The amount to increment by. - * @returns {Promise} The updated usage document. + * @returns {Promise} The updated usage document, or `null` on the + * (now logged) anomalous lost-write case. */ -const increment = (organizationId, key, amount) => UsageRepository.increment(organizationId, currentMonth(), key, amount); +const increment = async (organizationId, key, amount) => { + const month = currentMonth(); + const doc = await UsageRepository.increment(organizationId, month, key, amount); + if (!doc) { + logger.error('[billing.usage] increment lost a write — duplicate-key retry matched no document', { + organizationId, + month, + key, + amount, + }); + } + return doc; +}; /** * @desc Get usage for the given organization (current month). diff --git a/modules/billing/tests/billing.usage.bootIndexReady.integration.tests.js b/modules/billing/tests/billing.usage.bootIndexReady.integration.tests.js index 1ba22611d..2a2d8f781 100644 --- a/modules/billing/tests/billing.usage.bootIndexReady.integration.tests.js +++ b/modules/billing/tests/billing.usage.bootIndexReady.integration.tests.js @@ -58,8 +58,33 @@ describe('BillingUsage — boot-time index readiness (#3990):', () => { expect(monthIx.partialFilterExpression).toEqual({ legacyPeriod: { $exists: true } }); expect(weekIx).toBeDefined(); + expect(weekIx.name).toBe('organizationId_1_weekKey_1_partial'); expect(weekIx.unique).toBe(true); - expect(weekIx.sparse).toBe(true); + expect(weekIx.partialFilterExpression).toEqual({ weekKey: { $exists: true } }); + }); + + test('legacy (weekKey-less) increments for the same org across two DIFFERENT months both persist (#3991)', async () => { + // The regression this migration fixes: the old compound `sparse: true` + // index indexed every document (organizationId is never missing), so a + // second legacy month for the same org collided on {organizationId, + // weekKey: null} and silently lost the write. Reproduces the issue's own + // repro verbatim against the real repository functions. + const orgId = new mongoose.Types.ObjectId(); + trackedOrgIds.push(orgId); + + const january = await BillingUsageRepository.increment(orgId.toString(), '2199-01', 'executions', 5); + expect(january).not.toBeNull(); + expect(january.counters.executions).toBe(5); + + const february = await BillingUsageRepository.increment(orgId.toString(), '2199-02', 'executions', 7); + expect(february).not.toBeNull(); + expect(february.counters.executions).toBe(7); + + const docs = await collection.find({ organizationId: orgId }).toArray(); + expect(docs).toHaveLength(2); + const byMonth = Object.fromEntries(docs.map((d) => [d.month, d.counters.executions])); + expect(byMonth['2199-01']).toBe(5); + expect(byMonth['2199-02']).toBe(7); }); test('meter replay is deterministic immediately after boot', async () => { diff --git a/modules/billing/tests/billing.usage.repository.integration.tests.js b/modules/billing/tests/billing.usage.repository.integration.tests.js index f5e5680ab..9f3e37bda 100644 --- a/modules/billing/tests/billing.usage.repository.integration.tests.js +++ b/modules/billing/tests/billing.usage.repository.integration.tests.js @@ -22,7 +22,7 @@ describe('BillingUsageRepository integration tests:', () => { collection = mongoose.connection.db.collection('billingusages'); await collection.createIndex( { organizationId: 1, weekKey: 1 }, - { unique: true, sparse: true, name: 'organizationId_1_weekKey_1' }, + { unique: true, name: 'organizationId_1_weekKey_1_partial', partialFilterExpression: { weekKey: { $exists: true } } }, ); BillingUsageRepository = (await import('../repositories/billing.usage.repository.js')).default; }); diff --git a/modules/billing/tests/billing.usage.service.unit.tests.js b/modules/billing/tests/billing.usage.service.unit.tests.js index aabe3c876..a09227e49 100644 --- a/modules/billing/tests/billing.usage.service.unit.tests.js +++ b/modules/billing/tests/billing.usage.service.unit.tests.js @@ -137,6 +137,48 @@ describe('BillingUsageService — meter extensions unit tests:', () => { }); }); + /** + * #3991 follow-up — `UsageRepository.increment` can (anomalously, post-fix) + * return null when its duplicate-key retry matches nothing. The service + * wrapper must convert that silent null into a LOUD, logged one (silent-catch + * convention) without throwing (no caller — in this repo or downstream — + * ever treated a non-null return as guaranteed, and throwing would be a + * breaking behavior change for this generic devkit module). + */ + describe('increment (legacy month-keyed) — #3991 loud-null hardening', () => { + test('happy path — returns the doc, never logs', async () => { + const loggerMod = await import('../../../lib/services/logger.js'); + const mockLoggerError = loggerMod.default.error; + const doc = makeUsageDoc({ month: '2026-06' }); + mockUsageRepository.increment.mockResolvedValue(doc); + + const result = await BillingUsageService.increment(orgId, 'executions', 1); + + expect(result).toBe(doc); + expect(mockUsageRepository.increment).toHaveBeenCalledWith( + orgId, + expect.stringMatching(/^\d{4}-\d{2}$/), + 'executions', + 1, + ); + expect(mockLoggerError).not.toHaveBeenCalled(); + }); + + test('anomalous lost write (duplicate-key retry matched nothing) — logs error with context, still returns null', async () => { + const loggerMod = await import('../../../lib/services/logger.js'); + const mockLoggerError = loggerMod.default.error; + mockUsageRepository.increment.mockResolvedValue(null); + + const result = await BillingUsageService.increment(orgId, 'executions', 5); + + expect(result).toBeNull(); + expect(mockLoggerError).toHaveBeenCalledWith( + '[billing.usage] increment lost a write — duplicate-key retry matched no document', + expect.objectContaining({ organizationId: orgId, key: 'executions', amount: 5 }), + ); + }); + }); + describe('incrementMeter — no-op when meterMode=false', () => { test('should return applied=false immediately', async () => { mockConfig.billing.meterMode = false; diff --git a/modules/billing/tests/billing.usage.weekKeyIndexPartialFilter.migration.integration.tests.js b/modules/billing/tests/billing.usage.weekKeyIndexPartialFilter.migration.integration.tests.js new file mode 100644 index 000000000..2bee1b97e --- /dev/null +++ b/modules/billing/tests/billing.usage.weekKeyIndexPartialFilter.migration.integration.tests.js @@ -0,0 +1,271 @@ +/** + * Module dependencies. + */ +import mongoose from 'mongoose'; + +import { jest, beforeAll, afterAll, describe, test, expect } from '@jest/globals'; +import { bootstrap } from '../../../lib/app.js'; +import mongooseService from '../../../lib/services/mongoose.js'; + +import { up } from '../migrations/20260728120000-fix-usage-weekkey-index-partial.js'; + +const OLD_INDEX_NAME = 'organizationId_1_weekKey_1'; +const NEW_INDEX_NAME = 'organizationId_1_weekKey_1_partial'; +const INDEX_KEY = { organizationId: 1, weekKey: 1 }; + +/** + * Migration `20260728120000-fix-usage-weekkey-index-partial` (#3991). + * + * The old schema spec used `sparse: true` on a COMPOUND index — valid syntax, + * but MongoDB's sparse-exclusion rule only skips a document when it is missing + * ALL indexed fields. `organizationId` is always present, so every legacy + * (weekKey-less) document was indexed too (weekKey treated as `null`), and a + * second legacy month for the same org collided as a duplicate key, silently + * losing the write (see the model comment + `BillingUsageRepository.increment`). + * + * Unlike #3990 (an invalid partial filter that never built ANYWHERE), this old + * index IS live on every already-deployed database. The fix therefore cannot + * reuse the old default name (`organizationId_1_weekKey_1`) for the new spec — + * doing so would make autoIndex (which now runs BEFORE migrations and SURFACES + * build failures loudly, #3990) reject with IndexOptionsConflict on every boot + * until an operator manually intervened. The new index gets an explicit + * DIFFERENT name (`organizationId_1_weekKey_1_partial`) so it can be built + * ALONGSIDE the still-live old one without conflict; this migration is the + * authoritative creator of the new index AND dropper of the old one. + * + * `ensureBootBuiltIndex()` below reproduces the REAL post-deploy pre-migration + * state directly (bypassing this migration): the new schema-declared index + * already live (autoIndex built it under its distinct name with no conflict) + * while the old sparse index — which the new schema no longer declares, so + * autoIndex never touches it — is STILL live too. Verifies the fixed end state + * via the RAW collection driver. + */ +describe('Migration usage-weekkey-index-partial:', () => { + let usages; + const orgId = new mongoose.Types.ObjectId(); + + beforeAll(async () => { + await bootstrap(); + usages = mongoose.connection.db.collection('billingusages'); + }); + + afterAll(async () => { + try { + await usages.deleteMany({ organizationId: orgId }); + } catch (_) { /* cleanup */ } + try { + await mongooseService.disconnect(); + } catch (e) { + console.log(e); + expect(e).toBeFalsy(); + } + }); + + /** + * @desc Finds an index by name in the billingusages collection. + * @param {string} name - the index name to look up. + * @returns {Promise} the index descriptor if found, undefined otherwise. + */ + const findIndex = async (name) => { + const indexes = await usages.listIndexes().toArray(); + return indexes.find((ix) => ix.name === name); + }; + + /** + * @desc Simulates the real post-deploy, pre-migration state: the + * schema-declared new partial index already live (autoIndex built it under + * its own distinct name, no conflict with the old one) WHILE the old sparse + * index — no longer schema-declared, so autoIndex never drops it — is still + * live too. Used to prove `up()` stays safe and correctly drops the old + * index when it finds this coexisting state going in. + * @returns {Promise} + */ + const ensureBootBuiltIndex = async () => { + try { + await usages.dropIndex(NEW_INDEX_NAME); + } catch (_) { /* already absent */ } + try { + await usages.dropIndex(OLD_INDEX_NAME); + } catch (_) { /* already absent */ } + await usages.createIndex(INDEX_KEY, { + unique: true, + name: NEW_INDEX_NAME, + partialFilterExpression: { weekKey: { $exists: true } }, + }); + await usages.createIndex(INDEX_KEY, { unique: true, sparse: true, name: OLD_INDEX_NAME }); + }; + + test('up() creates organizationId_1_weekKey_1_partial with the exact spec', async () => { + await up(); + const ix = await findIndex(NEW_INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.key).toEqual({ organizationId: 1, weekKey: 1 }); + expect(ix.unique).toBe(true); + expect(ix.partialFilterExpression).toEqual({ weekKey: { $exists: true } }); + }); + + test('succeeds when boot already built the new index AND the legacy sparse index is still live (real-world already-deployed state, #3991)', async () => { + const legacyDocId = new mongoose.Types.ObjectId(); + const meterDocId = new mongoose.Types.ObjectId(); + try { + await ensureBootBuiltIndex(); + await usages.insertOne({ _id: legacyDocId, organizationId: orgId, month: '2020-05', legacyPeriod: true, counters: { executions: 1 } }); + await usages.insertOne({ _id: meterDocId, organizationId: orgId, month: '2020-05', weekKey: '2020-W20', meterUsed: 3, meterQuota: 100 }); + + await up(); + + const ix = await findIndex(NEW_INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.key).toEqual({ organizationId: 1, weekKey: 1 }); + expect(ix.unique).toBe(true); + expect(ix.partialFilterExpression).toEqual({ weekKey: { $exists: true } }); + expect(await findIndex(OLD_INDEX_NAME)).toBeUndefined(); + + // Documents are untouched — this migration is index-only, no backfill. + const legacyDoc = await usages.findOne({ _id: legacyDocId }); + const meterDoc = await usages.findOne({ _id: meterDocId }); + expect(legacyDoc.counters.executions).toBe(1); + expect(meterDoc.meterUsed).toBe(3); + } finally { + await usages.deleteMany({ _id: { $in: [legacyDocId, meterDocId] } }); + await up(); // restore the migrated end state for the suites that follow + } + }); + + test('is idempotent — a second run leaves exactly one index on the key', async () => { + await up(); + await up(); + const indexes = await usages.listIndexes().toArray(); + const sameKeyIndexes = indexes.filter((ix) => ix.key && ix.key.organizationId === 1 && ix.key.weekKey === 1); + expect(sameKeyIndexes.length).toBe(1); + expect(sameKeyIndexes[0].name).toBe(NEW_INDEX_NAME); + }); + + test('drops a same-key index living under another name and installs the canonical one', async () => { + try { await usages.dropIndex(NEW_INDEX_NAME); } catch (_) { /* already absent */ } + try { await usages.dropIndex(OLD_INDEX_NAME); } catch (_) { /* already absent */ } + await usages.createIndex(INDEX_KEY, { name: 'divergent_weekkey_index' }); + await up(); + expect(await findIndex('divergent_weekkey_index')).toBeUndefined(); + const ix = await findIndex(NEW_INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.unique).toBe(true); + }); + + test('ABORTS on pre-existing duplicate meter-mode (organizationId, weekKey) pairs without touching indexes', async () => { + const dupA = new mongoose.Types.ObjectId(); + const dupB = new mongoose.Types.ObjectId(); + // Drop every index on this key so both duplicate inserts can land — the + // abort must happen before any index write, so the pre-check cannot + // depend on any constraint already being absent OR present. + const weekKeyIndexes = (await usages.listIndexes().toArray()).filter((ix) => ix.key?.organizationId === 1 && ix.key?.weekKey === 1); + try { + for (const ix of weekKeyIndexes) { + await usages.dropIndex(ix.name); + } + await usages.insertOne({ _id: dupA, organizationId: orgId, month: '2021-06', weekKey: '2021-W24', meterUsed: 1, meterQuota: 100 }); + await usages.insertOne({ _id: dupB, organizationId: orgId, month: '2021-06', weekKey: '2021-W24', meterUsed: 2, meterQuota: 100 }); + + await expect(up()).rejects.toThrow(/duplicate meter-mode usage/); + + // Abort happened before ANY index write — neither the old nor the new + // index exists (both were dropped above, and up() never got past the + // pre-check). + expect(await findIndex(NEW_INDEX_NAME)).toBeUndefined(); + expect(await findIndex(OLD_INDEX_NAME)).toBeUndefined(); + } finally { + await usages.deleteMany({ _id: { $in: [dupA, dupB] } }); + // Replay the FULL captured descriptor(s) rather than a hand-picked + // subset, then converge via up() for the suites that follow. + for (const ix of weekKeyIndexes) { + const { key, name, ...options } = ix; + delete options.v; // server-assigned index format version — not a createIndex option + await usages.createIndex(key, { name, ...options }); + } + await up(); + } + }); + + test('createIndex E11000 during index creation aborts loud, same shape as the pre-check', async () => { + // Simulates a concurrent write landing a duplicate pair between the + // pre-check and the create call — force the full create path to run + // (drop every index on the key), then make createIndex reject with a raw + // E11000 the way the MongoDB server would on a genuine race, proving the + // migration converts that into the same documented, actionable abort as + // the upfront pre-check rather than letting a bare driver error escape. + const weekKeyIndexes = (await usages.listIndexes().toArray()).filter((ix) => ix.key?.organizationId === 1 && ix.key?.weekKey === 1); + try { + for (const ix of weekKeyIndexes) { + await usages.dropIndex(ix.name); + } + + const realCollection = mongoose.connection.db.collection('billingusages'); + const createIndexSpy = jest.spyOn(realCollection, 'createIndex').mockRejectedValueOnce( + Object.assign(new Error('E11000 duplicate key error collection: billingusages index: organizationId_1_weekKey_1_partial'), { code: 11000 }), + ); + const collectionSpy = jest.spyOn(mongoose.connection.db, 'collection').mockReturnValue(realCollection); + + try { + await expect(up()).rejects.toThrow(/duplicate \(organizationId, weekKey\) pair landed during migration/); + } finally { + collectionSpy.mockRestore(); + createIndexSpy.mockRestore(); + } + } finally { + for (const ix of weekKeyIndexes) { + const { key, name, ...options } = ix; + delete options.v; + try { await usages.dropIndex(name); } catch (_) { /* already absent */ } + await usages.createIndex(key, { name, ...options }); + } + await up(); // restore the migrated end state for the suites that follow + } + }); + + test('skip-window fast path — a converged database does not touch indexes', async () => { + // Converge first (idempotent, matches whatever end state prior tests left). + await up(); + + const infoSpy = jest.spyOn(console, 'info').mockImplementation(() => {}); + try { + await up(); + const messages = infoSpy.mock.calls.map((args) => args[0]); + expect(messages.some((m) => m.includes('skipping entirely'))).toBe(true); + expect(messages.some((m) => m.includes('dropped legacy index'))).toBe(false); + expect(messages.some((m) => m.includes('created partial-unique index'))).toBe(false); + } finally { + infoSpy.mockRestore(); + } + + const ix = await findIndex(NEW_INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.unique).toBe(true); + expect(ix.partialFilterExpression).toEqual({ weekKey: { $exists: true } }); + }); + + test('schema twin is IDENTICAL — syncIndexes() has nothing to drop or rebuild', async () => { + await up(); + const BillingUsage = mongoose.model('BillingUsage'); + const dropped = await BillingUsage.syncIndexes(); + expect(dropped).toEqual([]); + expect(await findIndex(NEW_INDEX_NAME)).toBeDefined(); + }); + + test('DB backstop: a second meter-mode row for the same (org, weekKey) rejects with E11000', async () => { + await up(); + const BillingUsage = mongoose.model('BillingUsage'); + await BillingUsage.create({ organizationId: orgId, month: '2022-09', weekKey: '2022-W36', meterUsed: 1, meterQuota: 100 }); + await expect( + BillingUsage.create({ organizationId: orgId, month: '2022-09', weekKey: '2022-W36', meterUsed: 2, meterQuota: 100 }), + ).rejects.toMatchObject({ code: 11000 }); + expect(await BillingUsage.countDocuments({ organizationId: orgId, weekKey: '2022-W36' })).toBe(1); + }); + + test('DB backstop: legacy (weekKey-less) rows for the same org across DIFFERENT months stay OUTSIDE the partial index and both persist (#3991)', async () => { + await up(); + const BillingUsage = mongoose.model('BillingUsage'); + await BillingUsage.create({ organizationId: orgId, month: '2023-03', legacyPeriod: true, counters: { executions: 1 } }); + await BillingUsage.create({ organizationId: orgId, month: '2023-04', legacyPeriod: true, counters: { executions: 1 } }); + expect(await BillingUsage.countDocuments({ organizationId: orgId, month: { $in: ['2023-03', '2023-04'] } })).toBe(2); + }); +}); From bd36564387e692edca81bc4c9f074791042773b8 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Tue, 28 Jul 2026 16:41:48 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(billing):=20review=20pass=20=E2=80=94?= =?UTF-8?q?=20divergent=20index=20recovery=20+=20null-return=20doc=20(#399?= =?UTF-8?q?1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Migration: drop a pre-existing NEW_INDEX_NAME index with a divergent spec before createIndex, so a hand-fixed/earlier-iteration index doesn't wedge every future boot in an IndexOptionsConflict crash loop (step (c) never drops NEW_INDEX_NAME, so it could never self-heal otherwise). - Repository: document that increment() also returns null on an invalid organizationId (no write attempted), not only on the anomalous duplicate-key-retry-miss case. - Add a regression test for the divergent-NEW_INDEX_NAME recovery path. --- ...728120000-fix-usage-weekkey-index-partial.js | 10 ++++++++++ .../repositories/billing.usage.repository.js | 16 +++++++++------- ...PartialFilter.migration.integration.tests.js | 17 +++++++++++++++++ 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js b/modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js index 244430617..e4a8a08b0 100644 --- a/modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js +++ b/modules/billing/migrations/20260728120000-fix-usage-weekkey-index-partial.js @@ -177,6 +177,16 @@ export async function up() { // the old one, so there is never a window without SOME uniqueness // constraint on meter-mode documents. if (!existing.some(isExactTargetIndex)) { + // A same-name index under NEW_INDEX_NAME with a divergent spec (a hand-fix + // or an earlier iteration) would make createIndex reject forever with + // IndexOptionsConflict (code 85) — step (c) below explicitly skips + // NEW_INDEX_NAME, so that divergent index would never get dropped and + // every subsequent boot would fail identically. Drop it first so the + // target spec can converge. + if (existing.some((ix) => ix.name === NEW_INDEX_NAME)) { + await usages.dropIndex(NEW_INDEX_NAME); + console.info(`[migration] usage-weekkey-index-partial: dropped divergent index '${NEW_INDEX_NAME}' before recreating it with the target spec`); + } try { await usages.createIndex(INDEX_KEY, { unique: true, diff --git a/modules/billing/repositories/billing.usage.repository.js b/modules/billing/repositories/billing.usage.repository.js index 46fcbf4bf..1de7f2b33 100644 --- a/modules/billing/repositories/billing.usage.repository.js +++ b/modules/billing/repositories/billing.usage.repository.js @@ -31,13 +31,15 @@ const get = (organizationId, month) => { * @param {String} month - The month in YYYY-MM format. * @param {String} key - The counter key to increment (e.g. 'executions'). * @param {Number} amount - The amount to increment by. - * @returns {Promise} The updated usage document, or (anomalous — - * see `BillingUsageService.increment`, which logs this loudly) `null` when a - * duplicate-key retry's exact-match filter finds nothing, meaning the write - * was lost. Should not happen in normal operation post-#3991 (the retry - * filter is `{organizationId, month}`, identical to what the winning - * concurrent upsert just created), but is not asserted against here — the - * repository stays a thin data layer; callers decide how loud to be. + * @returns {Promise} The updated usage document, or `null` when + * `organizationId` is not a valid ObjectId (no write attempted), or + * (anomalous — see `BillingUsageService.increment`, which logs this loudly) + * `null` when a duplicate-key retry's exact-match filter finds nothing, + * meaning the write was lost. Should not happen in normal operation + * post-#3991 (the retry filter is `{organizationId, month}`, identical to + * what the winning concurrent upsert just created), but is not asserted + * against here — the repository stays a thin data layer; callers decide how + * loud to be. */ const increment = async (organizationId, month, key, amount) => { if (!mongoose.Types.ObjectId.isValid(organizationId)) return null; diff --git a/modules/billing/tests/billing.usage.weekKeyIndexPartialFilter.migration.integration.tests.js b/modules/billing/tests/billing.usage.weekKeyIndexPartialFilter.migration.integration.tests.js index 2bee1b97e..51c9ef196 100644 --- a/modules/billing/tests/billing.usage.weekKeyIndexPartialFilter.migration.integration.tests.js +++ b/modules/billing/tests/billing.usage.weekKeyIndexPartialFilter.migration.integration.tests.js @@ -152,6 +152,23 @@ describe('Migration usage-weekkey-index-partial:', () => { expect(ix.unique).toBe(true); }); + test('an index already under NEW_INDEX_NAME with divergent options is dropped and recreated to the target spec (not a permanent IndexOptionsConflict)', async () => { + try { await usages.dropIndex(NEW_INDEX_NAME); } catch (_) { /* already absent */ } + try { await usages.dropIndex(OLD_INDEX_NAME); } catch (_) { /* already absent */ } + // A same-name index with a divergent spec (e.g. a hand-fix or an earlier + // iteration of this migration) must not be a permanent IndexOptionsConflict + // — up() must drop and recreate it under the target spec, not crash-loop. + await usages.createIndex(INDEX_KEY, { name: NEW_INDEX_NAME, unique: false }); + + await up(); + + const ix = await findIndex(NEW_INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.key).toEqual({ organizationId: 1, weekKey: 1 }); + expect(ix.unique).toBe(true); + expect(ix.partialFilterExpression).toEqual({ weekKey: { $exists: true } }); + }); + test('ABORTS on pre-existing duplicate meter-mode (organizationId, weekKey) pairs without touching indexes', async () => { const dupA = new mongoose.Types.ObjectId(); const dupB = new mongoose.Types.ObjectId();