From 977e5a066be441f597e289608164cad37b8cece6 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 27 Jul 2026 10:03:36 +0200 Subject: [PATCH 1/4] fix(mongoose): await model index builds at startup (#3990) mongoose.connect() resolving does not mean indexes exist yet: autoIndex builds run in the background and startMongoose() never awaited them. On a brand-new database the first writes could land inside that build window, turning a unique-index idempotency guard (e.g. a duplicate upsert normally caught as E11000) into a no-op that creates a second, distinct document instead. Add mongooseService.awaitIndexBuilds(), calling Model#init() on every registered model, and await it in startMongoose() right after connect() so the app only reports ready once every model's indexes are built. This also surfaces index-creation errors (e.g. an unsupported partialFilterExpression operator) that a fire-and-forget autoIndex was silently swallowing on the model's unlistened 'index' event. Respects the effective autoIndex option: when it resolves falsy, init() still just resolves without forcing a build. --- lib/app.js | 5 ++ lib/services/mongoose.js | 21 +++++ .../mongoose.awaitIndexBuilds.unit.tests.js | 87 +++++++++++++++++++ 3 files changed, 113 insertions(+) create mode 100644 lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js diff --git a/lib/app.js b/lib/app.js index 20cda0c55..63753b32f 100644 --- a/lib/app.js +++ b/lib/app.js @@ -17,6 +17,11 @@ const startMongoose = async () => { try { await mongooseService.loadModels(); const connection = await mongooseService.connect(); + // connect() resolving only means the socket is up — autoIndex builds still + // run in the background. Wait for every model's indexes to finish before + // this function (and therefore bootstrap/start) resolves, so nothing can + // write through a unique-index idempotency guard before it exists (#3990). + await mongooseService.awaitIndexBuilds(); return connection; } catch (e) { throw new Error(e); diff --git a/lib/services/mongoose.js b/lib/services/mongoose.js index 1c501429f..144e53d41 100644 --- a/lib/services/mongoose.js +++ b/lib/services/mongoose.js @@ -62,6 +62,26 @@ const connect = async () => { } }; +/** + * @desc Await index readiness for every currently registered model. + * `mongoose.connect()` resolving does not mean indexes exist yet — autoIndex + * builds run in the background, so callers must explicitly wait before + * treating the app as ready. `Model#init()` resolves once that model's index + * builds finish; mongoose already calls it once automatically when the model + * is compiled (cached on `model.$init`), so this simply awaits builds that + * are already in flight rather than triggering a second build. + * It also SURFACES index-creation errors (e.g. an unsupported + * `partialFilterExpression` operator) that a fire-and-forget autoIndex would + * otherwise silently swallow on the model's unlistened `'index'` event — + * callers should expect this call to reject when a schema declares an + * invalid index (#3990). + * Respects the effective `autoIndex` option (schema > connection > global): + * when it resolves falsy, `Model#init()` still resolves — it just skips the + * index build rather than forcing one. + * @returns {Promise} Resolves once every registered model's index builds finish. + */ +const awaitIndexBuilds = () => Promise.all(mongoose.modelNames().map((name) => mongoose.model(name).init())); + /** * Disconnect from the MongoDB server */ @@ -73,6 +93,7 @@ const disconnect = async () => { export default { loadModels, connect, + awaitIndexBuilds, disconnect, resolveDebug, }; diff --git a/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js b/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js new file mode 100644 index 000000000..5434c36ba --- /dev/null +++ b/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js @@ -0,0 +1,87 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globals'; + +/** + * Unit tests — #3990. `mongoose.connect()` resolving does not mean indexes + * exist yet (autoIndex builds run in the background). `awaitIndexBuilds()` + * must call `Model#init()` on every currently registered model and only + * resolve once ALL of those builds finish — and it must propagate (not + * swallow) a rejection from any one of them, since that is exactly how an + * invalid index declaration (e.g. an unsupported partialFilterExpression + * operator) is surfaced instead of silently never building. + */ +describe('mongoose service — awaitIndexBuilds:', () => { + let mongoose; + let awaitIndexBuilds; + + beforeEach(async () => { + jest.resetModules(); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { db: { uri: 'mongodb://127.0.0.1:27017/NodeTest', options: {} }, files: { mongooseModels: [] } }, + })); + jest.unstable_mockModule('../logger.js', () => ({ + default: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, + })); + + mongoose = { + modelNames: jest.fn(() => ['User', 'BillingUsage']), + model: jest.fn(() => ({ init: jest.fn().mockResolvedValue(undefined) })), + connect: jest.fn(), + set: jest.fn(), + }; + jest.unstable_mockModule('mongoose', () => ({ default: mongoose })); + + const mod = await import('../mongoose.js'); + awaitIndexBuilds = mod.default.awaitIndexBuilds; + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + test('calls init() on every currently registered model', async () => { + await awaitIndexBuilds(); + expect(mongoose.modelNames).toHaveBeenCalled(); + expect(mongoose.model).toHaveBeenCalledWith('User'); + expect(mongoose.model).toHaveBeenCalledWith('BillingUsage'); + }); + + test('resolves only after every model init() promise settles', async () => { + let userResolved = false; + let billingResolved = false; + mongoose.model = jest.fn((name) => ({ + init: jest.fn(() => + new Promise((resolve) => { + setTimeout(() => { + if (name === 'User') userResolved = true; + if (name === 'BillingUsage') billingResolved = true; + resolve(undefined); + }, 5); + }), + ), + })); + + await awaitIndexBuilds(); + expect(userResolved).toBe(true); + expect(billingResolved).toBe(true); + }); + + test('propagates a model init() rejection instead of swallowing it (e.g. an invalid index declaration)', async () => { + mongoose.model = jest.fn((name) => ({ + init: + name === 'BillingUsage' + ? jest.fn().mockRejectedValue(new Error('unsupported partial filter operator')) + : jest.fn().mockResolvedValue(undefined), + })); + + await expect(awaitIndexBuilds()).rejects.toThrow('unsupported partial filter operator'); + }); + + test('resolves with no models registered (nothing to await)', async () => { + mongoose.modelNames = jest.fn(() => []); + await expect(awaitIndexBuilds()).resolves.toEqual([]); + expect(mongoose.model).not.toHaveBeenCalled(); + }); +}); From 2384063ddebefcac8c2474588e0550b48d5e4efc Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 27 Jul 2026 10:03:54 +0200 Subject: [PATCH 2/4] fix(billing): replace unsupported partial-filter on legacy usage index (#3990) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The (organizationId, month) unique index on legacy (non-meter) usage documents declared partialFilterExpression { weekKey: { $exists: false } } — MongoDB does not support $exists:false (or $ne) inside a partial filter, so the index silently never built on any database. Combined with the previous mongoose.connect()-doesn't-await-indexes gap, this guard has never actually enforced uniqueness anywhere. The legacy month-keyed path (BillingUsageRepository.increment/get/reset) is still live — it backs the meterMode:false (default) usage counters read via BillingUsageService.get() in the billing controller and quota service. Flip the condition to a supported positive check: a new `legacyPeriod` boolean, set only by increment()'s $setOnInsert on newly created legacy documents (never by the meter-mode paths, which key by weekKey), lets the index filter on `{ legacyPeriod: { $exists: true } }` instead. Migration 20260727120000 is the authoritative creator on already-deployed databases: it backfills legacyPeriod onto existing legacy documents, aborts on any pre-existing duplicate (organizationId, month) pair, and installs the corrected index — mirroring the same $exists/$ne pattern already fixed for the membership and email indexes. Adds boot-time index-readiness and migration coverage on a real Mongo: Model#init() surfaces the invalid filter, both unique indexes exist with the exact spec after boot, and meter/legacy replay stays a single document immediately after boot (no index-build window to race). --- ...00-fix-usage-month-index-partial-filter.js | 157 +++++++++++++++++ .../models/billing.usage.model.mongoose.js | 32 +++- .../billing/models/billing.usage.schema.js | 6 + .../repositories/billing.usage.repository.js | 6 +- ....usage.bootIndexReady.integration.tests.js | 110 ++++++++++++ ...rtialFilter.migration.integration.tests.js | 166 ++++++++++++++++++ .../billing.usage.repository.unit.tests.js | 58 ++++++ 7 files changed, 531 insertions(+), 4 deletions(-) create mode 100644 modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js create mode 100644 modules/billing/tests/billing.usage.bootIndexReady.integration.tests.js create mode 100644 modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js diff --git a/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js b/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js new file mode 100644 index 000000000..b90665e92 --- /dev/null +++ b/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js @@ -0,0 +1,157 @@ +/** + * Module dependencies + */ +import mongoose from 'mongoose'; + +const INDEX_NAME = 'organizationId_1_month_1'; +const INDEX_KEY = { organizationId: 1, month: 1 }; + +/** + * @desc Exact-key match helper: a two-field index keyed (organizationId:1, month:1) + * in that order. + * @param {Object} ix - an index document from listIndexes() + * @return {boolean} true when the index key is exactly { organizationId:1, month:1 } + */ +const sameKey = (ix) => { + const keys = Object.keys(ix.key || {}); + return keys.length === 2 && keys[0] === 'organizationId' && keys[1] === 'month' + && ix.key.organizationId === 1 && ix.key.month === 1; +}; + +/** + * Migration: working (organizationId, month) legacy-usage partial-unique index (#3990). + * + * The schema previously declared partialFilterExpression + * `{ weekKey: { $exists: false } }` — MongoDB does NOT support `$exists: false` + * (or `$ne`) inside a partialFilterExpression (only `$eq`, `$exists: true`, `$gt`, + * `$gte`, `$lt`, `$lte`, `$type`, and the top-level `$and` are allowed). Mongoose + * autoIndex reports that failure on the model's 'index' event, where nothing + * listens, so the index NEVER existed on any deployed database and the + * (organizationId, month) uniqueness guard for legacy (non-meter) usage + * documents ran on application code alone (racy findOneAndUpdate-with-upsert in + * BillingUsageRepository.increment). + * + * New spec (the schema declares the IDENTICAL twin): unique on + * { organizationId: 1, month: 1 }, partialFilterExpression + * { legacyPeriod: { $exists: true } }. `legacyPeriod` is a new boolean + * discriminator set only by the legacy write path (BillingUsageRepository + * .increment's $setOnInsert) — meter-mode documents (incrementMeter / + * upsertWeekSnapshot, keyed by weekKey) never set it. This flips the + * unsupported "weekKey absent" condition into a supported positive + * $exists:true check while preserving the original intent: only legacy, + * non-meter usage documents are covered by this uniqueness constraint. + * + * Safety / ordering: + * (a) Backfill `legacyPeriod: true` onto existing documents that have no + * weekKey and no legacyPeriod yet — exactly the pre-existing legacy + * documents this index is meant to cover. No-ops on a fresh database + * (empty/missing collection). + * (b) Pre-check for existing duplicate (organizationId, month) pairs among + * legacy documents that would violate the unique index. If any exist we + * ABORT (throw) WITHOUT touching indexes — picking which duplicate row + * wins is an operator decision, not a migration's call. + * (c) Drop any divergent index first: a same-key index under another name + * (phantom/legacy spec) or a namesake whose options drifted. + * (d) Create the index. Idempotent: re-running after success is a no-op. + * + * autoIndex race: mongoose autoIndex:true (the default — db.options sets no + * override) builds the schema-declared twin on connect; identical specs make + * the race benign and syncIndexes() idempotent. This migration is the + * AUTHORITATIVE creator for already-deployed databases. + * + * @returns {Promise} + */ +export async function up() { + const usages = mongoose.connection.db.collection('billingusages'); + + // ── (a) Backfill the discriminator onto existing legacy documents ── + // No-op (matches nothing) on a fresh database where the collection is empty + // or does not exist yet. + const backfillResult = await usages.updateMany( + { weekKey: { $exists: false }, legacyPeriod: { $exists: false } }, + { $set: { legacyPeriod: true } }, + ); + if (backfillResult.modifiedCount > 0) { + console.info(`[migration] usage-month-index-partial-filter: backfilled legacyPeriod on ${backfillResult.modifiedCount} document(s)`); + } + + // ── (b) Pre-check: refuse to run if duplicate (organizationId, month) pairs exist ── + const duplicates = await usages + .aggregate([ + { $match: { legacyPeriod: true } }, + { $group: { _id: { organizationId: '$organizationId', month: '$month' }, count: { $sum: 1 }, ids: { $push: '$_id' } } }, + { $match: { count: { $gt: 1 } } }, + ]) + .toArray(); + + 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-month-index-partial-filter ABORTED: ${duplicates.length} duplicate legacy usage (organizationId, month) 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-month-index-partial-filter: billingusages collection does not exist yet — nothing to migrate'); + return; + } + throw err; + } + + // ── (c) Drop divergent indexes / detect the exact expected shape ── + let hasIndex = false; + for (const ix of existing) { + if (ix.name === '_id_') continue; + const keyMatches = sameKey(ix); + const exactShape = keyMatches + && ix.name === INDEX_NAME + && ix.unique === true + && ix.partialFilterExpression?.legacyPeriod?.$exists === true; + if (exactShape) { + hasIndex = true; + } else if (keyMatches || ix.name === INDEX_NAME) { + await usages.dropIndex(ix.name); + console.info(`[migration] usage-month-index-partial-filter: dropped divergent index '${ix.name}'`); + } + } + + // ── (d) Create the partial-unique index (idempotent) ── + if (!hasIndex) { + await usages.createIndex(INDEX_KEY, { + unique: true, + name: INDEX_NAME, + partialFilterExpression: { legacyPeriod: { $exists: true } }, + }); + console.info('[migration] usage-month-index-partial-filter: created partial-unique index on (organizationId, month)'); + } else { + console.info('[migration] usage-month-index-partial-filter: partial-unique index already present — skipping create'); + } +} + +/** + * Down: no-op (warn). The pre-fix state was a unique guard that silently never + * existed — restoring "no index" would reintroduce the bug. Rollback = revert + * the schema declaration deliberately, then drop the index by hand if truly + * needed. The `legacyPeriod` backfill is left in place (harmless additive + * field with no other reader). + * + * @returns {void} + */ +export function down() { + console.warn( + '[migration] usage-month-index-partial-filter DOWN: no-op; drop the (organizationId, month) 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 c223a9459..c356a333f 100644 --- a/modules/billing/models/billing.usage.model.mongoose.js +++ b/modules/billing/models/billing.usage.model.mongoose.js @@ -35,6 +35,19 @@ const UsageMongoose = new Schema( type: Schema.Types.Mixed, default: () => ({}), }, + /** + * Discriminator set ONLY on legacy (non-meter) usage documents — written by + * BillingUsageRepository.increment's `$setOnInsert`, never by the meter-mode + * write paths (incrementMeter / upsertWeekSnapshot, which key by weekKey). + * Exists purely to express "this document has no weekKey" as a SUPPORTED + * partial-filter condition on the legacy unique index below: MongoDB + * partial filter expressions do not support `$exists: false` / `$ne` (see + * that index's comment), so the condition is phrased as a positive + * `$exists: true` check on a field only legacy documents ever carry. + */ + legacyPeriod: { + type: Boolean, + }, // ── Meter fields (sparse — only populated in meter mode) ───────────────── @@ -122,13 +135,26 @@ const UsageMongoose = new Schema( /** * Legacy unique index: (organizationId, month) — kept for non-meter downstream. - * Partial filter: only applies to documents without weekKey (non-meter mode). + * + * Partial filter: only applies to legacy (non-meter) documents, identified by + * the `legacyPeriod` discriminator (see field comment above) rather than by + * "weekKey is absent" directly. `$exists: false` (and `$ne`) are NOT supported + * inside a partialFilterExpression — MongoDB only allows `$eq`, `$exists: true`, + * `$gt`, `$gte`, `$lt`, `$lte`, `$type`, and the top-level `$and`. The previous + * `{ weekKey: { $exists: false } }` spec silently never built on ANY database: + * mongoose autoIndex reports the creation failure on the model's unlistened + * 'index' event, so this uniqueness guard ran on application code alone + * (upsert-based, racy under a concurrent create) until fixed (#3990). Migration + * `20260727120000-fix-usage-month-index-partial-filter.js` is the authoritative + * creator on already-deployed databases (backfills `legacyPeriod` first). + * * Meter-mode documents have weekKey set and can have multiple docs per month - * (one per ISO week), so they are excluded from this uniqueness constraint. + * (one per ISO week); they never carry `legacyPeriod`, so they stay excluded + * from this uniqueness constraint. */ UsageMongoose.index( { organizationId: 1, month: 1 }, - { unique: true, partialFilterExpression: { weekKey: { $exists: false } } }, + { unique: true, partialFilterExpression: { legacyPeriod: { $exists: true } } }, ); /** diff --git a/modules/billing/models/billing.usage.schema.js b/modules/billing/models/billing.usage.schema.js index b5c4e88ae..c16ec2fc7 100644 --- a/modules/billing/models/billing.usage.schema.js +++ b/modules/billing/models/billing.usage.schema.js @@ -23,6 +23,12 @@ const BillingUsage = z.object({ month: z.string().trim().regex(/^\d{4}-(0[1-9]|1[0-2])$/, 'month must be in YYYY-MM format'), counters: z.record(z.string(), z.number()).default(() => ({})), + /** + * Discriminator set only on legacy (non-meter) usage documents — see the + * model field comment. Never set on meter-mode (weekKey-keyed) documents. + */ + legacyPeriod: z.boolean().optional(), + // ── Meter fields (optional — only populated in meter mode) ─────────────── /** diff --git a/modules/billing/repositories/billing.usage.repository.js b/modules/billing/repositories/billing.usage.repository.js index 562ca2572..e91c007ec 100644 --- a/modules/billing/repositories/billing.usage.repository.js +++ b/modules/billing/repositories/billing.usage.repository.js @@ -23,6 +23,10 @@ const get = (organizationId, month) => { /** * @function increment * @description Atomically increment a counter key for the given org+month, with upsert. + * `$setOnInsert: { legacyPeriod: true }` marks newly-created documents as + * legacy (non-meter) — the discriminator the (organizationId, month) unique + * partial index filters on (see model comment; meter-mode documents, created + * via incrementMeter/upsertWeekSnapshot, never set this field). * @param {String} organizationId - The organization ID. * @param {String} month - The month in YYYY-MM format. * @param {String} key - The counter key to increment (e.g. 'executions'). @@ -35,7 +39,7 @@ const increment = async (organizationId, month, key, amount) => { try { return await BillingUsage.findOneAndUpdate( { organizationId, month }, - { $inc: { [`counters.${key}`]: amount } }, + { $inc: { [`counters.${key}`]: amount }, $setOnInsert: { legacyPeriod: true } }, { upsert: true, returnDocument: 'after', runValidators: true }, ).exec(); } catch (err) { diff --git a/modules/billing/tests/billing.usage.bootIndexReady.integration.tests.js b/modules/billing/tests/billing.usage.bootIndexReady.integration.tests.js new file mode 100644 index 000000000..1ba22611d --- /dev/null +++ b/modules/billing/tests/billing.usage.bootIndexReady.integration.tests.js @@ -0,0 +1,110 @@ +/** + * Module dependencies. + */ +import mongoose from 'mongoose'; +import { describe, beforeAll, afterEach, afterAll, test, expect } from '@jest/globals'; + +import mongooseService from '../../../lib/services/mongoose.js'; + +/** + * #3990 — `mongoose.connect()` resolving does not mean indexes exist: autoIndex + * builds run in the background and the old `startMongoose()` never awaited them. + * On a brand-new database the first writes could land inside that build window, + * turning BillingUsage's unique-index idempotency guards into a no-op — a + * duplicate upsert would create a SECOND document instead of hitting E11000. + * + * `Model#init()` (what `mongooseService.awaitIndexBuilds()` calls) caches its + * promise per model per connection — mongoose already calls it once + * automatically when a model is compiled, so a SECOND explicit call is a no-op + * rather than a genuine rebuild. That is exactly the real boot semantics (one + * connection, one index-build pass, before anything else runs), so this suite + * exercises it the same way: `beforeAll` drives the REAL boot path once + * (`loadModels -> connect -> awaitIndexBuilds`, the same sequence `lib/app.js + * #startMongoose` now runs), and every test below relies on that single, + * already-awaited end state — proving that once boot has resolved, nothing can + * observe a window where the unique indexes are not yet built. + */ +describe('BillingUsage — boot-time index readiness (#3990):', () => { + let BillingUsageRepository; + let collection; + const trackedOrgIds = []; + + beforeAll(async () => { + await mongooseService.loadModels(); + await mongooseService.connect(); + // The same call lib/app.js#startMongoose now makes before reporting ready. + await mongooseService.awaitIndexBuilds(); + collection = mongoose.connection.db.collection('billingusages'); + BillingUsageRepository = (await import('../repositories/billing.usage.repository.js')).default; + }); + + afterEach(async () => { + if (trackedOrgIds.length > 0) { + await collection.deleteMany({ organizationId: { $in: trackedOrgIds.splice(0) } }); + } + }); + + afterAll(async () => { + await mongooseService.disconnect(); + }); + + test('awaitIndexBuilds() resolves and both BillingUsage unique indexes exist', async () => { + const indexes = await collection.listIndexes().toArray(); + const monthIx = indexes.find((ix) => ix.key?.organizationId === 1 && ix.key?.month === 1); + const weekIx = indexes.find((ix) => ix.key?.organizationId === 1 && ix.key?.weekKey === 1); + + expect(monthIx).toBeDefined(); + expect(monthIx.unique).toBe(true); + expect(monthIx.partialFilterExpression).toEqual({ legacyPeriod: { $exists: true } }); + + expect(weekIx).toBeDefined(); + expect(weekIx.unique).toBe(true); + expect(weekIx.sparse).toBe(true); + }); + + test('meter replay is deterministic immediately after boot', async () => { + const orgId = new mongoose.Types.ObjectId(); + trackedOrgIds.push(orgId); + const weekKey = '2099-W05'; + const idempotencyKey = 'hist_boot_replay'; + const baseSnapshot = { month: '2099-05', meterQuota: 1000, planVersion: 'v1', resetAt: null }; + + // First write — creates the document. + const first = await BillingUsageRepository.incrementMeter(orgId.toString(), weekKey, 40, {}, idempotencyKey, baseSnapshot); + expect(first).not.toBeNull(); + expect(first.meterUsed).toBe(40); + + // Replay — same idempotencyKey. Because the unique index was already built + // before this write path could even start (boot awaited it in beforeAll), + // this must ALWAYS no-op (return null) — never create a second document. + // Before the fix, an app that raced its first writes against the index + // build could land here with the index still missing and double-write. + const replay = await BillingUsageRepository.incrementMeter(orgId.toString(), weekKey, 40, {}, idempotencyKey, baseSnapshot); + expect(replay).toBeNull(); + + const count = await collection.countDocuments({ organizationId: orgId, weekKey }); + expect(count).toBe(1); + const doc = await collection.findOne({ organizationId: orgId, weekKey }); + expect(doc.meterUsed).toBe(40); + }); + + test('legacy month-keyed increment stays a single document under concurrent upserts', async () => { + const orgId = new mongoose.Types.ObjectId(); + trackedOrgIds.push(orgId); + const month = '2099-05'; + + // Two concurrently-issued upserts simulate the exact race the unique index + // guards against — without it (or without the index being built yet), both + // could land as separate inserts. + await Promise.all([ + BillingUsageRepository.increment(orgId.toString(), month, 'executions', 1), + BillingUsageRepository.increment(orgId.toString(), month, 'executions', 1), + ]); + + const count = await collection.countDocuments({ organizationId: orgId, month }); + expect(count).toBe(1); + const doc = await collection.findOne({ organizationId: orgId, month }); + expect(doc.counters.executions).toBe(2); + expect(doc.legacyPeriod).toBe(true); + }); +}); diff --git a/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js b/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js new file mode 100644 index 000000000..b479e4066 --- /dev/null +++ b/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js @@ -0,0 +1,166 @@ +/** + * Module dependencies. + */ +import mongoose from 'mongoose'; + +import { 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/20260727120000-fix-usage-month-index-partial-filter.js'; + +const INDEX_NAME = 'organizationId_1_month_1'; + +/** + * Migration `20260727120000-fix-usage-month-index-partial-filter` (#3990). + * + * The old schema spec used `$exists: false` in its partialFilterExpression — + * unsupported by MongoDB — so the unique (organizationId, month) guard NEVER + * materialized on any database (autoIndex failures land on the model's + * unlistened 'index' event). Verifies the fixed end state via the RAW + * collection driver: + * - up() backfills the `legacyPeriod` discriminator onto pre-existing legacy + * (non-meter) documents BEFORE creating the index; + * - meter-mode documents (weekKey present) never get the discriminator; + * - up() creates `organizationId_1_month_1` with the EXACT spec (key, unique, + * $exists:true filter); + * - idempotent (a second run leaves exactly one index on the key); + * - a same-key index living under another name is dropped and replaced; + * - pre-existing duplicate legacy (organizationId, month) pairs ABORT the + * migration before any index work. + */ +describe('Migration usage-month-index-partial-filter:', () => { + 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); + }; + + test('up() creates organizationId_1_month_1 with the exact spec', async () => { + await up(); + const ix = await findIndex(INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.key).toEqual({ organizationId: 1, month: 1 }); + expect(ix.unique).toBe(true); + expect(ix.partialFilterExpression).toEqual({ legacyPeriod: { $exists: true } }); + }); + + 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 sameKey = indexes.filter((ix) => ix.key && ix.key.organizationId === 1 && ix.key.month === 1); + expect(sameKey.length).toBe(1); + expect(sameKey[0].name).toBe(INDEX_NAME); + }); + + test('drops a same-key index living under another name and installs the canonical one', async () => { + try { await usages.dropIndex(INDEX_NAME); } catch (_) { /* already absent */ } + await usages.createIndex({ organizationId: 1, month: 1 }, { name: 'legacy_org_month' }); + await up(); + expect(await findIndex('legacy_org_month')).toBeUndefined(); + const ix = await findIndex(INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.unique).toBe(true); + }); + + test('backfills legacyPeriod onto a pre-existing legacy document (no weekKey) before indexing', async () => { + const docId = new mongoose.Types.ObjectId(); + try { + try { await usages.dropIndex(INDEX_NAME); } catch (_) { /* already absent */ } + await usages.insertOne({ + _id: docId, organizationId: orgId, month: '2020-01', counters: { executions: 3 }, + }); + await up(); + const doc = await usages.findOne({ _id: docId }); + expect(doc.legacyPeriod).toBe(true); + } finally { + await usages.deleteMany({ _id: docId }); + await up(); // restore the migrated end state for the suites that follow + } + }); + + test('never backfills legacyPeriod onto a meter-mode document (weekKey present)', async () => { + const docId = new mongoose.Types.ObjectId(); + try { + await usages.insertOne({ + _id: docId, organizationId: orgId, month: '2020-01', weekKey: '2020-W01', meterUsed: 5, meterQuota: 100, + }); + await up(); + const doc = await usages.findOne({ _id: docId }); + expect(doc.legacyPeriod).toBeUndefined(); + } finally { + await usages.deleteMany({ _id: docId }); + } + }); + + test('ABORTS on pre-existing duplicate legacy (organizationId, month) pairs without touching indexes', async () => { + const dupA = new mongoose.Types.ObjectId(); + const dupB = new mongoose.Types.ObjectId(); + try { + try { await usages.dropIndex(INDEX_NAME); } catch (_) { /* already absent */ } + // Distinct dummy weekKey values so the two rows don't collide on the + // UNRELATED (organizationId, weekKey) unique index — this test is only + // about the (organizationId, month) duplicate pre-check. + await usages.insertOne({ _id: dupA, organizationId: orgId, month: '2021-06', weekKey: 'dupA-probe', legacyPeriod: true, counters: {} }); + await usages.insertOne({ _id: dupB, organizationId: orgId, month: '2021-06', weekKey: 'dupB-probe', legacyPeriod: true, counters: {} }); + await expect(up()).rejects.toThrow(/duplicate legacy usage/); + // Abort happened before any index work — the index is still absent. + expect(await findIndex(INDEX_NAME)).toBeUndefined(); + } finally { + await usages.deleteMany({ _id: { $in: [dupA, dupB] } }); + await up(); // restore the migrated end state for the suites that follow + } + }); + + 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(INDEX_NAME)).toBeDefined(); + }); + + test('DB backstop: a second legacy row for the same (org, month) rejects with E11000', async () => { + await up(); + const BillingUsage = mongoose.model('BillingUsage'); + // Bypass the repository's upsert-then-catch guard on purpose: the index + // itself must reject the duplicate. + await BillingUsage.create({ organizationId: orgId, month: '2022-09', legacyPeriod: true, counters: {} }); + await expect( + BillingUsage.create({ organizationId: orgId, month: '2022-09', legacyPeriod: true, counters: {} }), + ).rejects.toMatchObject({ code: 11000 }); + expect(await BillingUsage.countDocuments({ organizationId: orgId, month: '2022-09' })).toBe(1); + }); + + test('meter-mode rows stay OUTSIDE the partial index (may repeat per org/month, one per weekKey)', async () => { + const BillingUsage = mongoose.model('BillingUsage'); + await BillingUsage.create({ organizationId: orgId, month: '2023-03', weekKey: '2023-W09', meterUsed: 0, meterQuota: 0 }); + await BillingUsage.create({ organizationId: orgId, month: '2023-03', weekKey: '2023-W10', meterUsed: 0, meterQuota: 0 }); + expect(await BillingUsage.countDocuments({ organizationId: orgId, month: '2023-03' })).toBe(2); + }); +}); diff --git a/modules/billing/tests/billing.usage.repository.unit.tests.js b/modules/billing/tests/billing.usage.repository.unit.tests.js index 63441ebb2..e2e2656c6 100644 --- a/modules/billing/tests/billing.usage.repository.unit.tests.js +++ b/modules/billing/tests/billing.usage.repository.unit.tests.js @@ -455,4 +455,62 @@ describe('BillingUsageRepository — meter extensions unit tests:', () => { expect(result).toBe(2); }); }); + + /** + * #3990 — the (organizationId, month) unique partial index now filters on the + * `legacyPeriod` discriminator (unsupported `weekKey: { $exists: false }` before). + * `increment` is the only writer that creates new legacy documents, so it must + * mark them via $setOnInsert for the index's partial filter to ever cover them. + */ + describe('increment (legacy month-keyed)', () => { + const month = '2026-06'; + + test('upserts with $inc AND $setOnInsert: { legacyPeriod: true }', async () => { + const exec = jest.fn().mockResolvedValue(makeUsageDoc({ month, weekKey: undefined, counters: { executions: 1 } })); + mockModel.findOneAndUpdate.mockReturnValue({ exec }); + + await BillingUsageRepository.increment(orgId, month, 'executions', 1); + + expect(mockModel.findOneAndUpdate).toHaveBeenCalledWith( + { organizationId: orgId, month }, + { $inc: { 'counters.executions': 1 }, $setOnInsert: { legacyPeriod: true } }, + { upsert: true, returnDocument: 'after', runValidators: true }, + ); + expect(exec).toHaveBeenCalled(); + }); + + test('on a duplicate-key race, retries WITHOUT upsert (existing doc already carries legacyPeriod)', async () => { + const dupErr = Object.assign(new Error('E11000 duplicate key'), { code: 11000 }); + const firstExec = jest.fn().mockRejectedValue(dupErr); + const retryExec = jest.fn().mockResolvedValue(makeUsageDoc({ month, weekKey: undefined, counters: { executions: 2 } })); + mockModel.findOneAndUpdate + .mockReturnValueOnce({ exec: firstExec }) + .mockReturnValueOnce({ exec: retryExec }); + + const result = await BillingUsageRepository.increment(orgId, month, 'executions', 1); + + expect(mockModel.findOneAndUpdate).toHaveBeenNthCalledWith( + 2, + { organizationId: orgId, month }, + { $inc: { 'counters.executions': 1 } }, + { returnDocument: 'after', runValidators: true }, + ); + expect(result.counters.executions).toBe(2); + }); + + test('rejects an unsafe counter key without touching the model', async () => { + await expect(BillingUsageRepository.increment(orgId, month, 'bad key!', 1)).rejects.toThrow('Invalid counter key'); + expect(mockModel.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + test('returns null for an invalid organizationId without touching the model', async () => { + const { default: mongooseMock } = await import('mongoose'); + mongooseMock.Types.ObjectId.isValid.mockReturnValueOnce(false); + + const result = await BillingUsageRepository.increment('not-an-id', month, 'executions', 1); + + expect(result).toBeNull(); + expect(mockModel.findOneAndUpdate).not.toHaveBeenCalled(); + }); + }); }); From f50516fa6b5855f3276221c6312cc8305df5c1fd Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 27 Jul 2026 11:01:37 +0200 Subject: [PATCH 3/4] =?UTF-8?q?fix(mongoose):=20review=20pass=20=E2=80=94?= =?UTF-8?q?=20migration=20ordering,=20bounded=20index=20wait,=20MIGRATIONS?= =?UTF-8?q?=20entry=20(#3990)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - migration: precheck legacy (organizationId, month) duplicates via a plain query FIRST (zero writes), then drop/backfill/recreate the partial index — safe now that boot awaits index builds before migrations.run(), so the index may already be live-and-empty when up() runs - mongoose: awaitIndexBuilds() is now bounded by config.db.awaitIndexBuilds (default 60s timeout) — on timeout, boot continues in a degraded state with a loud warning instead of hanging forever; false disables the wait - MIGRATIONS.md: document the boot-semantics change, the new config knob, and a pre-deploy duplicate-data audit snippet for legacy billing usage --- MIGRATIONS.md | 35 ++++++ config/defaults/development.config.js | 9 ++ lib/services/mongoose.js | 72 +++++++++++- .../mongoose.awaitIndexBuilds.unit.tests.js | 77 +++++++++++- ...00-fix-usage-month-index-partial-filter.js | 110 ++++++++++-------- ...rtialFilter.migration.integration.tests.js | 107 +++++++++++++++-- 6 files changed, 349 insertions(+), 61 deletions(-) diff --git a/MIGRATIONS.md b/MIGRATIONS.md index 7b1238690..e9c8879b9 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -4,6 +4,41 @@ Breaking changes and upgrade notes for downstream projects. --- +## 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). + +### What changed (this repo) + +- **`lib/services/mongoose.js`** — new `awaitIndexBuilds()`, called by `lib/app.js#startMongoose()` right after `connect()` and BEFORE `migrations.run()`. It awaits every registered model's `Model#init()` (mongoose already triggers this once on model compile; this just awaits the in-flight promise) and now SURFACES a rejection instead of it being swallowed on the unlistened `'index'` event — **this applies to every module**, not just billing: any schema that declares an unsupported/invalid index will now fail loudly at boot (or time out — see the config knob below) instead of silently never building. +- **New config knob `db.awaitIndexBuilds`** (`config/defaults/development.config.js`, inherited by all envs) — bounds the wait so a big-collection index build can't stall readiness stack-wide on a rolling deploy: default `{ timeoutMs: 60000 }`. On timeout, boot **continues** in a degraded (pre-fix) state — the build keeps going in the background and a loud warning names the still-building model(s); an eventual build failure is still logged after the fact. Set to `false` to skip the wait entirely (restores the pre-#3990 fire-and-forget behavior). +- **`modules/billing/models/billing.usage.model.mongoose.js`** — the index now filters on a new `legacyPeriod: Boolean` discriminator (`partialFilterExpression: { legacyPeriod: { $exists: true } }`) instead of the unsupported `weekKey` negative check. `legacyPeriod` is set only by the legacy (non-meter) write path (`BillingUsageRepository.increment`'s `$setOnInsert`) — meter-mode documents never carry it. +- **New migration `modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js`** — the authoritative index creator on already-deployed databases. Ordering is deliberately boot-ordering-safe (boot now awaits index builds BEFORE migrations run, so the partial index above may already be LIVE AND EMPTY by the time this migration executes): **(a)** duplicate pre-check FIRST via a plain query (`weekKey: { $exists: false }`, not an index filter — zero writes), abort loud on any pre-existing duplicate `(organizationId, month)` pair; **(b)** drop the index if present (boot-built-empty or divergent); **(c)** backfill `legacyPeriod: true` onto legacy documents; **(d)** recreate the index. Idempotent on re-run. + +### Pre-deploy duplicate-data audit (downstreams using legacy/non-meter 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: false } } }, + { $group: { _id: { organizationId: '$organizationId', month: '$month' }, count: { $sum: 1 }, ids: { $push: '$_id' } } }, + { $match: { count: { $gt: 1 } } }, +]); +``` + +Downstreams running exclusively in meter mode (every `billingusages` document has `weekKey` set) will always get an empty result — this only applies to legacy (non-meter) usage tracking. + +### 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.** If it returns any group, resolve the duplicates first — otherwise the migration aborts boot on next deploy (loud error naming the offending doc ids, zero writes performed). +3. No action needed on the `db.awaitIndexBuilds` knob — default (`{ timeoutMs: 60000 }`) is safe for normal collection sizes. Only override it (in `config/defaults/{project}.config.js`) if you have an unusually large collection with a slow index build and want a longer/shorter timeout, or `false` to opt back into fire-and-forget autoIndex. +4. Because index-build failures now surface loudly stack-wide (not just for billing), watch the first post-deploy boot log for any `Index builds still in flight` warning or a boot failure — it means some model's schema declares an index MongoDB rejects, previously silent. +5. Migrations run at boot before `listen()`; the index swap + `legacyPeriod` backfill land automatically once the duplicate-data audit passes. + +--- + ## Config: `docs.excludeModules` — doc-only module exclusion (2026-06-29) New opt-in `config.docs.excludeModules` (default `[]` → **no behavior change**). It drops a module's `doc/*.yml` (OpenAPI) + `doc/guides/*.md` (guide tree) from the public spec (`/api/spec.json`) and guide tree (`/api/public/docs`), **independent of module runtime activation** — so it works even for **core** modules (`core`/`auth`/`users`/`home`), which `filterByActivation` never filters. diff --git a/config/defaults/development.config.js b/config/defaults/development.config.js index c5516cbb4..9fbb987d1 100644 --- a/config/defaults/development.config.js +++ b/config/defaults/development.config.js @@ -42,6 +42,15 @@ const config = { db: { uri: 'mongodb://127.0.0.1:27017/NodeDev', debug: true, + // Bounds lib/services/mongoose.js#awaitIndexBuilds (#3990 follow-up): boot + // awaits every model's index build up to timeoutMs before continuing, so a + // unique-index idempotency guard can't be raced by an early write. On + // timeout, boot continues in a degraded (pre-#3990) state and logs a loud + // warning instead of hanging forever — set to `false` to disable the wait + // entirely (fire-and-forget autoIndex, pre-#3990 behavior). + awaitIndexBuilds: { + timeoutMs: 60000, + }, options: { user: '', pass: '', diff --git a/lib/services/mongoose.js b/lib/services/mongoose.js index 144e53d41..87ebf2334 100644 --- a/lib/services/mongoose.js +++ b/lib/services/mongoose.js @@ -20,6 +20,12 @@ import logger from './logger.js'; */ const resolveDebug = (cfg = config) => Boolean(cfg?.db?.debug) && configHelper.isDevEnv(); +/** + * Default bound (ms) on {@link awaitIndexBuilds} when `config.db.awaitIndexBuilds` + * does not specify its own `timeoutMs` (#3990 follow-up). + */ +const DEFAULT_AWAIT_INDEX_BUILDS_TIMEOUT_MS = 60000; + /** * Load all mongoose related models */ @@ -78,9 +84,71 @@ const connect = async () => { * Respects the effective `autoIndex` option (schema > connection > global): * when it resolves falsy, `Model#init()` still resolves — it just skips the * index build rather than forcing one. - * @returns {Promise} Resolves once every registered model's index builds finish. + * + * Bounded by `config.db.awaitIndexBuilds` (#3990 follow-up — an unbounded + * wait here stalls readiness stack-wide on a rolling deploy whenever a + * big collection's index build takes a while): + * - `false` skips the wait entirely — index builds still happen via + * autoIndex, just fire-and-forget again (pre-#3990 behaviour). + * - any other value (default: `{}`) waits up to `timeoutMs` (default + * {@link DEFAULT_AWAIT_INDEX_BUILDS_TIMEOUT_MS}). On timeout, boot + * CONTINUES — a loud warning names the model(s) still building, and the + * in-flight build(s) keep going in the background; an eventual failure + * (e.g. an invalid index declaration) is still logged, just after boot + * already proceeded past this call. + * A genuine rejection that happens BEFORE the timeout (the common case — an + * invalid index declaration fails fast) still propagates and rejects this + * call, exactly as before. + * @param {object} [cfg=config] - application configuration object + * @returns {Promise} Resolves once every registered model's index builds + * finish, the configured timeout elapses, or immediately when disabled. */ -const awaitIndexBuilds = () => Promise.all(mongoose.modelNames().map((name) => mongoose.model(name).init())); +const awaitIndexBuilds = async (cfg = config) => { + const setting = cfg?.db?.awaitIndexBuilds; + if (setting === false) return; + + const timeoutMs = setting && typeof setting === 'object' && Number.isFinite(setting.timeoutMs) ? setting.timeoutMs : DEFAULT_AWAIT_INDEX_BUILDS_TIMEOUT_MS; + + const modelNames = mongoose.modelNames(); + if (modelNames.length === 0) return; + + const pending = new Set(modelNames); + const builds = Promise.all( + modelNames.map((name) => + mongoose.model(name) + .init() + .then((result) => { + pending.delete(name); + return result; + }), + ), + ); + + let timer; + const timeoutPromise = new Promise((resolve) => { + timer = setTimeout(() => resolve('timeout'), timeoutMs); + timer.unref?.(); + }); + + try { + const winner = await Promise.race([builds, timeoutPromise]); + if (winner === 'timeout') { + logger.warn( + chalk.red( + `Index builds still in flight after ${timeoutMs}ms — continuing boot in a DEGRADED state (writes may race a not-yet-built index) for model(s): ${[...pending].join(', ')}`, + ), + ); + // Let the build(s) keep going in the background; surface an eventual + // failure loudly instead of silently swallowing it, even though boot + // already continued past this call. + builds.catch((err) => { + logger.error(chalk.red('Index build failed after boot continued past the awaitIndexBuilds timeout:'), err); + }); + } + } finally { + clearTimeout(timer); + } +}; /** * Disconnect from the MongoDB server diff --git a/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js b/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js index 5434c36ba..665f1209a 100644 --- a/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js +++ b/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js @@ -11,6 +11,14 @@ import { jest, describe, test, beforeEach, afterEach, expect } from '@jest/globa * swallow) a rejection from any one of them, since that is exactly how an * invalid index declaration (e.g. an unsupported partialFilterExpression * operator) is surfaced instead of silently never building. + * + * Follow-up (#3990 review — unbounded boot block): `awaitIndexBuilds()` is + * now bounded by `config.db.awaitIndexBuilds`. On a timeout it must CONTINUE + * (not hang, not throw) and log a loud warning naming the still-building + * model(s); `config.db.awaitIndexBuilds === false` must skip the wait + * entirely. Those two paths need their own config mock (a non-default + * `db.awaitIndexBuilds`), so they set up their own isolated module registry + * rather than reusing the shared `beforeEach` below. */ describe('mongoose service — awaitIndexBuilds:', () => { let mongoose; @@ -81,7 +89,74 @@ describe('mongoose service — awaitIndexBuilds:', () => { test('resolves with no models registered (nothing to await)', async () => { mongoose.modelNames = jest.fn(() => []); - await expect(awaitIndexBuilds()).resolves.toEqual([]); + await expect(awaitIndexBuilds()).resolves.toBeUndefined(); expect(mongoose.model).not.toHaveBeenCalled(); }); + + test('continues boot after the configured timeout, logging the still-building model(s)', async () => { + jest.resetModules(); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + db: { uri: 'mongodb://127.0.0.1:27017/NodeTest', options: {}, awaitIndexBuilds: { timeoutMs: 15 } }, + files: { mongooseModels: [] }, + }, + })); + const warn = jest.fn(); + jest.unstable_mockModule('../logger.js', () => ({ + default: { info: jest.fn(), error: jest.fn(), warn }, + })); + + let resolveBillingInit; + const localMongoose = { + modelNames: jest.fn(() => ['User', 'BillingUsage']), + model: jest.fn((name) => ({ + init: + name === 'BillingUsage' + // Never settles within the 15ms timeout — simulates a + // still-in-flight build on a big collection. + ? jest.fn(() => new Promise((resolve) => { resolveBillingInit = resolve; })) + : jest.fn().mockResolvedValue(undefined), + })), + connect: jest.fn(), + set: jest.fn(), + }; + jest.unstable_mockModule('mongoose', () => ({ default: localMongoose })); + + const mod = await import('../mongoose.js'); + await expect(mod.default.awaitIndexBuilds()).resolves.toBeUndefined(); + + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toEqual(expect.stringContaining('BillingUsage')); + expect(warn.mock.calls[0][0]).not.toEqual(expect.stringContaining(': User')); + + // Let the deferred build settle so it doesn't leak into later tests. + resolveBillingInit(undefined); + }); + + test('config.db.awaitIndexBuilds === false skips the wait entirely', async () => { + jest.resetModules(); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + db: { uri: 'mongodb://127.0.0.1:27017/NodeTest', options: {}, awaitIndexBuilds: false }, + files: { mongooseModels: [] }, + }, + })); + jest.unstable_mockModule('../logger.js', () => ({ + default: { info: jest.fn(), error: jest.fn(), warn: jest.fn() }, + })); + + const localMongoose = { + modelNames: jest.fn(() => ['User', 'BillingUsage']), + model: jest.fn(() => ({ init: jest.fn().mockResolvedValue(undefined) })), + connect: jest.fn(), + set: jest.fn(), + }; + jest.unstable_mockModule('mongoose', () => ({ default: localMongoose })); + + const mod = await import('../mongoose.js'); + await expect(mod.default.awaitIndexBuilds()).resolves.toBeUndefined(); + + expect(localMongoose.modelNames).not.toHaveBeenCalled(); + expect(localMongoose.model).not.toHaveBeenCalled(); + }); }); diff --git a/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js b/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js index b90665e92..aaff8a90f 100644 --- a/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js +++ b/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js @@ -41,44 +41,52 @@ const sameKey = (ix) => { * $exists:true check while preserving the original intent: only legacy, * non-meter usage documents are covered by this uniqueness constraint. * - * Safety / ordering: - * (a) Backfill `legacyPeriod: true` onto existing documents that have no - * weekKey and no legacyPeriod yet — exactly the pre-existing legacy - * documents this index is meant to cover. No-ops on a fresh database - * (empty/missing collection). - * (b) Pre-check for existing duplicate (organizationId, month) pairs among - * legacy documents that would violate the unique index. If any exist we - * ABORT (throw) WITHOUT touching indexes — picking which duplicate row - * wins is an operator decision, not a migration's call. - * (c) Drop any divergent index first: a same-key index under another name - * (phantom/legacy spec) or a namesake whose options drifted. - * (d) Create the index. Idempotent: re-running after success is a no-op. + * Ordering (REWORKED — #3990 review): boot now runs + * `startMongoose()` (autoIndex + `awaitIndexBuilds()`) BEFORE + * `migrations.run()` (see lib/app.js#bootstrap). That means on a first boot + * after this schema change deploys, the partial index above may ALREADY be + * LIVE (built empty by autoIndex) by the time this migration's `up()` runs — + * this migration must be safe against that, not just against a fresh/absent + * index: + * (a) Duplicate pre-check FIRST, ZERO writes. Query (not index-filter) the + * legacy shape directly — `weekKey: { $exists: false }` is a supported + * QUERY filter (only partial-INDEX filters forbid `$exists: false`) — + * group by (organizationId, month), abort loud on any count > 1. This + * must run before any write below, because if the index is already + * live-and-empty (boot-built), the backfill write in (c) would + * otherwise hit a raw E11000 mid-`updateMany` on the first duplicate + * pair, leaving the collection partially backfilled. + * (b) Drop the partial index if present — it may be live-and-empty from + * boot, a divergent same-key index under another name, or absent on an + * old/never-migrated database. Dropping first guarantees no unique + * constraint is live while (c) writes. + * (c) Backfill `legacyPeriod: true` onto existing legacy documents. Safe + * now — no index is live to race against, and (a) already proved no + * duplicate pair exists. + * (d) Recreate the index (same spec as the schema declaration). + * Idempotent on re-run: a second `up()` finds no duplicates (backfill is a + * no-op the second time), drops the index it just created, and recreates it + * — same end state, just an extra drop/create round-trip. * * autoIndex race: mongoose autoIndex:true (the default — db.options sets no * override) builds the schema-declared twin on connect; identical specs make - * the race benign and syncIndexes() idempotent. This migration is the - * AUTHORITATIVE creator for already-deployed databases. + * the race benign. This migration is the AUTHORITATIVE creator on + * already-deployed databases (it owns the backfill autoIndex cannot do). * * @returns {Promise} */ export async function up() { const usages = mongoose.connection.db.collection('billingusages'); - // ── (a) Backfill the discriminator onto existing legacy documents ── - // No-op (matches nothing) on a fresh database where the collection is empty - // or does not exist yet. - const backfillResult = await usages.updateMany( - { weekKey: { $exists: false }, legacyPeriod: { $exists: false } }, - { $set: { legacyPeriod: true } }, - ); - if (backfillResult.modifiedCount > 0) { - console.info(`[migration] usage-month-index-partial-filter: backfilled legacyPeriod on ${backfillResult.modifiedCount} document(s)`); - } - - // ── (b) Pre-check: refuse to run if duplicate (organizationId, month) pairs exist ── + // ── (a) Duplicate pre-check FIRST — zero writes so far. Safe whether the + // partial index is already live (boot-built empty) or absent: this reads + // via a plain query filter, never a partial-index filter, so `$exists: + // false` is fine here. `.aggregate()` on a missing/empty collection just + // returns an empty result (unlike `.listIndexes()` below), so this is also + // safe on a fresh database. const duplicates = await usages .aggregate([ - { $match: { legacyPeriod: true } }, + { $match: { weekKey: { $exists: false } } }, { $group: { _id: { organizationId: '$organizationId', month: '$month' }, count: { $sum: 1 }, ids: { $push: '$_id' } } }, { $match: { count: { $gt: 1 } } }, ]) @@ -111,34 +119,40 @@ export async function up() { throw err; } - // ── (c) Drop divergent indexes / detect the exact expected shape ── - let hasIndex = false; + // ── (b) Drop the partial index if present, BEFORE the backfill write below. + // It may already be LIVE AND EMPTY — boot's awaitIndexBuilds() builds the + // schema-declared twin before this migration runs — or a divergent + // same-key index living under another name. Either way it must not be live + // while (c) writes `legacyPeriod`, since (a) already proved there is no + // duplicate to violate it, but a stale/empty index would still intercept + // every write in the updateMany one document at a time. for (const ix of existing) { if (ix.name === '_id_') continue; - const keyMatches = sameKey(ix); - const exactShape = keyMatches - && ix.name === INDEX_NAME - && ix.unique === true - && ix.partialFilterExpression?.legacyPeriod?.$exists === true; - if (exactShape) { - hasIndex = true; - } else if (keyMatches || ix.name === INDEX_NAME) { + if (sameKey(ix) || ix.name === INDEX_NAME) { await usages.dropIndex(ix.name); - console.info(`[migration] usage-month-index-partial-filter: dropped divergent index '${ix.name}'`); + console.info(`[migration] usage-month-index-partial-filter: dropped index '${ix.name}' before backfill (boot-built empty or divergent)`); } } - // ── (d) Create the partial-unique index (idempotent) ── - if (!hasIndex) { - await usages.createIndex(INDEX_KEY, { - unique: true, - name: INDEX_NAME, - partialFilterExpression: { legacyPeriod: { $exists: true } }, - }); - console.info('[migration] usage-month-index-partial-filter: created partial-unique index on (organizationId, month)'); - } else { - console.info('[migration] usage-month-index-partial-filter: partial-unique index already present — skipping create'); + // ── (c) Backfill the discriminator onto existing legacy documents ── + // No-op (matches nothing) on a fresh database where the collection is empty, + // and safe here — no unique constraint is live to race against, and (a) + // already confirmed no duplicate (organizationId, month) pair exists. + const backfillResult = await usages.updateMany( + { weekKey: { $exists: false }, legacyPeriod: { $exists: false } }, + { $set: { legacyPeriod: true } }, + ); + if (backfillResult.modifiedCount > 0) { + console.info(`[migration] usage-month-index-partial-filter: backfilled legacyPeriod on ${backfillResult.modifiedCount} document(s)`); } + + // ── (d) Recreate the partial-unique index (same spec as the schema declaration) ── + await usages.createIndex(INDEX_KEY, { + unique: true, + name: INDEX_NAME, + partialFilterExpression: { legacyPeriod: { $exists: true } }, + }); + console.info('[migration] usage-month-index-partial-filter: created partial-unique index on (organizationId, month)'); } /** diff --git a/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js b/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js index b479e4066..f438fe577 100644 --- a/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js +++ b/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js @@ -10,6 +10,7 @@ import mongooseService from '../../../lib/services/mongoose.js'; import { up } from '../migrations/20260727120000-fix-usage-month-index-partial-filter.js'; const INDEX_NAME = 'organizationId_1_month_1'; +const INDEX_KEY = { organizationId: 1, month: 1 }; /** * Migration `20260727120000-fix-usage-month-index-partial-filter` (#3990). @@ -27,7 +28,15 @@ const INDEX_NAME = 'organizationId_1_month_1'; * - idempotent (a second run leaves exactly one index on the key); * - a same-key index living under another name is dropped and replaced; * - pre-existing duplicate legacy (organizationId, month) pairs ABORT the - * migration before any index work. + * migration before any write — index or document; + * - REWORKED ORDERING (#3990 review): boot now runs `awaitIndexBuilds()` + * BEFORE `migrations.run()` (see lib/app.js#bootstrap), so on a first + * boot after this schema ships, the partial index may already be LIVE + * AND EMPTY by the time `up()` runs. `ensureBootBuiltIndex()` below + * reproduces exactly that state directly (bypassing this migration), and + * the success + abort tests below use REAL-WORLD fixtures — legacy docs + * that lack `legacyPeriod` entirely (the actual pre-migration shape) — + * against that already-live index. */ describe('Migration usage-month-index-partial-filter:', () => { let usages; @@ -60,6 +69,25 @@ describe('Migration usage-month-index-partial-filter:', () => { return indexes.find((ix) => ix.name === name); }; + /** + * @desc Simulates the boot-built state (#3990 review): creates the + * schema-declared partial index directly — bypassing this migration — the + * same end state `awaitIndexBuilds()` leaves BEFORE `migrations.run()` now + * runs (see lib/app.js#bootstrap). Used to prove `up()` stays safe when it + * finds this index already live-and-empty going in. + * @returns {Promise} + */ + const ensureBootBuiltIndex = async () => { + try { + await usages.dropIndex(INDEX_NAME); + } catch (_) { /* already absent */ } + await usages.createIndex(INDEX_KEY, { + unique: true, + name: INDEX_NAME, + partialFilterExpression: { legacyPeriod: { $exists: true } }, + }); + }; + test('up() creates organizationId_1_month_1 with the exact spec', async () => { await up(); const ix = await findIndex(INDEX_NAME); @@ -69,6 +97,32 @@ describe('Migration usage-month-index-partial-filter:', () => { expect(ix.partialFilterExpression).toEqual({ legacyPeriod: { $exists: true } }); }); + test('succeeds when boot already built the index empty and legacy docs lack legacyPeriod (real-world boot-ordering state, #3990)', async () => { + const docId = new mongoose.Types.ObjectId(); + try { + // Reproduce boot's actual state: awaitIndexBuilds() already built the + // schema-declared index (live, empty) BEFORE this migration runs. + await ensureBootBuiltIndex(); + // Real-world pre-migration document shape: legacy (no weekKey) and no + // legacyPeriod yet — backfilling it is exactly this migration's job. + await usages.insertOne({ _id: docId, organizationId: orgId, month: '2020-05', counters: { executions: 1 } }); + + await up(); + + const ix = await findIndex(INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.key).toEqual({ organizationId: 1, month: 1 }); + expect(ix.unique).toBe(true); + expect(ix.partialFilterExpression).toEqual({ legacyPeriod: { $exists: true } }); + + const doc = await usages.findOne({ _id: docId }); + expect(doc.legacyPeriod).toBe(true); + } finally { + await usages.deleteMany({ _id: docId }); + 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(); @@ -118,21 +172,54 @@ describe('Migration usage-month-index-partial-filter:', () => { } }); - test('ABORTS on pre-existing duplicate legacy (organizationId, month) pairs without touching indexes', async () => { + test('ABORTS on pre-existing duplicate legacy (organizationId, month) pairs without touching indexes or documents', async () => { const dupA = new mongoose.Types.ObjectId(); const dupB = new mongoose.Types.ObjectId(); + // The UNRELATED (organizationId, weekKey) sparse unique index is a + // COMPOUND sparse index: MongoDB only excludes a document from a + // compound sparse index when it is missing ALL indexed fields — since + // organizationId is always present, two legacy (weekKey-absent) docs for + // the SAME org collide on it too, independent of month. Such duplicate + // legacy rows realistically predate that index's existence (added + // 2026-05-01 by the meter-fields migration) — simulate that history by + // dropping it for the span of this test only, then restoring its exact + // spec. Out of scope for this migration (#3990 review only touches the + // (organizationId, month) index) — this is test setup, not a fix. + const weekKeyIndexes = (await usages.listIndexes().toArray()).filter((ix) => ix.key?.organizationId === 1 && ix.key?.weekKey === 1); try { - try { await usages.dropIndex(INDEX_NAME); } catch (_) { /* already absent */ } - // Distinct dummy weekKey values so the two rows don't collide on the - // UNRELATED (organizationId, weekKey) unique index — this test is only - // about the (organizationId, month) duplicate pre-check. - await usages.insertOne({ _id: dupA, organizationId: orgId, month: '2021-06', weekKey: 'dupA-probe', legacyPeriod: true, counters: {} }); - await usages.insertOne({ _id: dupB, organizationId: orgId, month: '2021-06', weekKey: 'dupB-probe', legacyPeriod: true, counters: {} }); + for (const ix of weekKeyIndexes) { + await usages.dropIndex(ix.name); + } + // Reproduce boot's actual state: the (organizationId, month) index is + // already live (empty) BEFORE this migration runs — the abort must + // still happen before any write, so the pre-check cannot depend on + // that index being absent. + await ensureBootBuiltIndex(); + // Real-world pre-migration shape: legacy (no weekKey), no legacyPeriod + // yet. Both docs are excluded from the (already-live) partial index — + // it only covers legacyPeriod:{$exists:true} — so inserting the + // duplicate pair here cannot itself trip E11000; only up()'s pre-check + // is under test. + await usages.insertOne({ _id: dupA, organizationId: orgId, month: '2021-06', counters: {} }); + await usages.insertOne({ _id: dupB, organizationId: orgId, month: '2021-06', counters: {} }); + await expect(up()).rejects.toThrow(/duplicate legacy usage/); - // Abort happened before any index work — the index is still absent. - expect(await findIndex(INDEX_NAME)).toBeUndefined(); + + // Abort happened before ANY write: neither doc was backfilled (zero + // docs modified) and the pre-existing (organizationId, month) index + // was left untouched. + const docA = await usages.findOne({ _id: dupA }); + const docB = await usages.findOne({ _id: dupB }); + expect(docA.legacyPeriod).toBeUndefined(); + expect(docB.legacyPeriod).toBeUndefined(); + const ix = await findIndex(INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.partialFilterExpression).toEqual({ legacyPeriod: { $exists: true } }); } finally { await usages.deleteMany({ _id: { $in: [dupA, dupB] } }); + for (const ix of weekKeyIndexes) { + await usages.createIndex(ix.key, { name: ix.name, unique: ix.unique, sparse: ix.sparse }); + } await up(); // restore the migrated end state for the suites that follow } }); From 3838d142b560367a97b2bfbcde313daa0f0e8541 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 27 Jul 2026 11:31:42 +0200 Subject: [PATCH 4/4] =?UTF-8?q?fix(billing):=20review=20pass=202=20?= =?UTF-8?q?=E2=80=94=20layering,=20resilient=20recreate,=20spec-faithful?= =?UTF-8?q?=20test=20(#3990)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit convergence pass on PR #3993: - lib/services/mongoose.js: awaitIndexBuilds() timeoutMs now coerces via Number(...) instead of Number.isFinite on the raw value, so a Layer-4 DEVKIT_NODE_* env override (always a string) is honored; non-positive values fall back to the default instead of racing to a ~0ms timeout. - migration 20260727120000: skip the drop->backfill->recreate window entirely once the index already matches the target spec and nothing is left to backfill (steady-state fast path); the final createIndex now catches a raced E11000 (concurrent old-instance write during a rolling deploy) and converts it into the same actionable abort as the upfront pre-check instead of a bare driver error. Documented the residual window + maintenance-window guidance honestly in the header and MIGRATIONS.md. - test restore of the (organizationId, weekKey) index now replays the full captured descriptor instead of a hand-picked key/name/unique/ sparse subset, so any other option round-trips and can't silently diverge from the schema declaration. - @return -> @returns JSDoc nit. Added regression coverage: numeric-string/invalid timeoutMs coercion (mongoose unit), skip-window fast path + E11000 abort conversion (migration integration). --- MIGRATIONS.md | 3 +- lib/services/mongoose.js | 9 +- .../mongoose.awaitIndexBuilds.unit.tests.js | 73 +++++++++++ ...00-fix-usage-month-index-partial-filter.js | 114 +++++++++++++++++- ...rtialFilter.migration.integration.tests.js | 63 +++++++++- 5 files changed, 252 insertions(+), 10 deletions(-) diff --git a/MIGRATIONS.md b/MIGRATIONS.md index e9c8879b9..ba9909f7a 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -13,7 +13,7 @@ Fixes a silent index-creation failure: the legacy `(organizationId, month)` uniq - **`lib/services/mongoose.js`** — new `awaitIndexBuilds()`, called by `lib/app.js#startMongoose()` right after `connect()` and BEFORE `migrations.run()`. It awaits every registered model's `Model#init()` (mongoose already triggers this once on model compile; this just awaits the in-flight promise) and now SURFACES a rejection instead of it being swallowed on the unlistened `'index'` event — **this applies to every module**, not just billing: any schema that declares an unsupported/invalid index will now fail loudly at boot (or time out — see the config knob below) instead of silently never building. - **New config knob `db.awaitIndexBuilds`** (`config/defaults/development.config.js`, inherited by all envs) — bounds the wait so a big-collection index build can't stall readiness stack-wide on a rolling deploy: default `{ timeoutMs: 60000 }`. On timeout, boot **continues** in a degraded (pre-fix) state — the build keeps going in the background and a loud warning names the still-building model(s); an eventual build failure is still logged after the fact. Set to `false` to skip the wait entirely (restores the pre-#3990 fire-and-forget behavior). - **`modules/billing/models/billing.usage.model.mongoose.js`** — the index now filters on a new `legacyPeriod: Boolean` discriminator (`partialFilterExpression: { legacyPeriod: { $exists: true } }`) instead of the unsupported `weekKey` negative check. `legacyPeriod` is set only by the legacy (non-meter) write path (`BillingUsageRepository.increment`'s `$setOnInsert`) — meter-mode documents never carry it. -- **New migration `modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js`** — the authoritative index creator on already-deployed databases. Ordering is deliberately boot-ordering-safe (boot now awaits index builds BEFORE migrations run, so the partial index above may already be LIVE AND EMPTY by the time this migration executes): **(a)** duplicate pre-check FIRST via a plain query (`weekKey: { $exists: false }`, not an index filter — zero writes), abort loud on any pre-existing duplicate `(organizationId, month)` pair; **(b)** drop the index if present (boot-built-empty or divergent); **(c)** backfill `legacyPeriod: true` onto legacy documents; **(d)** recreate the index. Idempotent on re-run. +- **New migration `modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js`** — the authoritative index creator on already-deployed databases. Ordering is deliberately boot-ordering-safe (boot now awaits index builds BEFORE migrations run, so the partial index above may already be LIVE AND EMPTY by the time this migration executes): **(a)** duplicate pre-check FIRST via a plain query (`weekKey: { $exists: false }`, not an index filter — zero writes), abort loud on any pre-existing duplicate `(organizationId, month)` pair; then, only if the index isn't already the exact target shape with nothing left to backfill (fast-path skip, the steady-state case): **(b)** drop the index if present (boot-built-empty or divergent); **(c)** backfill `legacyPeriod: true` onto legacy documents; **(d)** recreate the index — a duplicate-key error here (a still-serving old instance racing a write into the (b)-(d) window on a rolling deploy) is caught and re-thrown as the same actionable abort as (a), never a bare driver error. Idempotent on re-run. ### Pre-deploy duplicate-data audit (downstreams using legacy/non-meter billing usage) @@ -36,6 +36,7 @@ Downstreams running exclusively in meter mode (every `billingusages` document ha 3. No action needed on the `db.awaitIndexBuilds` knob — default (`{ timeoutMs: 60000 }`) is safe for normal collection sizes. Only override it (in `config/defaults/{project}.config.js`) if you have an unusually large collection with a slow index build and want a longer/shorter timeout, or `false` to opt back into fire-and-forget autoIndex. 4. Because index-build failures now surface loudly stack-wide (not just for billing), watch the first post-deploy boot log for any `Index builds still in flight` warning or a boot failure — it means some model's schema declares an index MongoDB rejects, previously silent. 5. Migrations run at boot before `listen()`; the index swap + `legacyPeriod` backfill land automatically once the duplicate-data audit passes. +6. **Rolling deploys only:** the very first successful run of this migration on a given database briefly drops the index while backfilling (old, still-serving instances writing into that window can trip a duplicate-key abort — self-healing, retried on next boot). For a strict no-window guarantee, run this specific deploy during a maintenance window or scale to a single instance first. Every later boot (including every other instance in the same rolling deploy once the database has converged) skips the window entirely. --- diff --git a/lib/services/mongoose.js b/lib/services/mongoose.js index 87ebf2334..ca0ec871e 100644 --- a/lib/services/mongoose.js +++ b/lib/services/mongoose.js @@ -107,7 +107,14 @@ const awaitIndexBuilds = async (cfg = config) => { const setting = cfg?.db?.awaitIndexBuilds; if (setting === false) return; - const timeoutMs = setting && typeof setting === 'object' && Number.isFinite(setting.timeoutMs) ? setting.timeoutMs : DEFAULT_AWAIT_INDEX_BUILDS_TIMEOUT_MS; + // Number(...) (not Number.isFinite directly on the raw value) so a numeric + // STRING survives — a `db.awaitIndexBuilds.timeoutMs` override supplied via a + // Layer-4 DEVKIT_NODE_* env var always arrives as a string (config/index.js + // only coerces the literal 'true'/'false', nothing numeric); reject + // non-positive values so a malformed override falls back to the default + // instead of racing straight to a 0ms/negative timeout. + const configuredTimeoutMs = setting && typeof setting === 'object' ? Number(setting.timeoutMs) : NaN; + const timeoutMs = Number.isFinite(configuredTimeoutMs) && configuredTimeoutMs > 0 ? configuredTimeoutMs : DEFAULT_AWAIT_INDEX_BUILDS_TIMEOUT_MS; const modelNames = mongoose.modelNames(); if (modelNames.length === 0) return; diff --git a/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js b/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js index 665f1209a..a005c03cd 100644 --- a/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js +++ b/lib/services/tests/mongoose.awaitIndexBuilds.unit.tests.js @@ -133,6 +133,79 @@ describe('mongoose service — awaitIndexBuilds:', () => { resolveBillingInit(undefined); }); + test('honors a numeric-STRING timeoutMs (a Layer-4 DEVKIT_NODE_* env override always arrives as a string, never a number)', async () => { + jest.resetModules(); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + db: { uri: 'mongodb://127.0.0.1:27017/NodeTest', options: {}, awaitIndexBuilds: { timeoutMs: '15' } }, + files: { mongooseModels: [] }, + }, + })); + const warn = jest.fn(); + jest.unstable_mockModule('../logger.js', () => ({ + default: { info: jest.fn(), error: jest.fn(), warn }, + })); + + let resolveBillingInit; + const localMongoose = { + modelNames: jest.fn(() => ['User', 'BillingUsage']), + model: jest.fn((name) => ({ + init: + name === 'BillingUsage' + // Never settles within the 15ms string-typed timeout. + ? jest.fn(() => new Promise((resolve) => { resolveBillingInit = resolve; })) + : jest.fn().mockResolvedValue(undefined), + })), + connect: jest.fn(), + set: jest.fn(), + }; + jest.unstable_mockModule('mongoose', () => ({ default: localMongoose })); + + const mod = await import('../mongoose.js'); + await expect(mod.default.awaitIndexBuilds()).resolves.toBeUndefined(); + + // If '15' (string) were rejected by Number.isFinite as pre-fix, the + // DEFAULT (60000ms) would apply instead and this warn would never fire + // within the test's lifetime. + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toEqual(expect.stringContaining('BillingUsage')); + + resolveBillingInit(undefined); + }); + + test('falls back to the default timeout on a non-positive/non-numeric timeoutMs override', async () => { + jest.resetModules(); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + // Malformed overrides must not race straight to a ~0ms timeout. + db: { uri: 'mongodb://127.0.0.1:27017/NodeTest', options: {}, awaitIndexBuilds: { timeoutMs: '-5' } }, + files: { mongooseModels: [] }, + }, + })); + const warn = jest.fn(); + jest.unstable_mockModule('../logger.js', () => ({ + default: { info: jest.fn(), error: jest.fn(), warn }, + })); + + const localMongoose = { + modelNames: jest.fn(() => ['User', 'BillingUsage']), + model: jest.fn(() => ({ + // Resolves quickly (5ms) — if the invalid override fell through to a + // ~0ms timeout instead of the 60000ms default, the timeout promise + // would win the race and warn() would fire. + init: jest.fn(() => new Promise((resolve) => { setTimeout(() => resolve(undefined), 5); })), + })), + connect: jest.fn(), + set: jest.fn(), + }; + jest.unstable_mockModule('mongoose', () => ({ default: localMongoose })); + + const mod = await import('../mongoose.js'); + await expect(mod.default.awaitIndexBuilds()).resolves.toBeUndefined(); + + expect(warn).not.toHaveBeenCalled(); + }); + test('config.db.awaitIndexBuilds === false skips the wait entirely', async () => { jest.resetModules(); jest.unstable_mockModule('../../../config/index.js', () => ({ diff --git a/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js b/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js index aaff8a90f..b57b1c90b 100644 --- a/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js +++ b/modules/billing/migrations/20260727120000-fix-usage-month-index-partial-filter.js @@ -10,7 +10,7 @@ const INDEX_KEY = { organizationId: 1, month: 1 }; * @desc Exact-key match helper: a two-field index keyed (organizationId:1, month:1) * in that order. * @param {Object} ix - an index document from listIndexes() - * @return {boolean} true when the index key is exactly { organizationId:1, month:1 } + * @returns {boolean} true when the index key is exactly { organizationId:1, month:1 } */ const sameKey = (ix) => { const keys = Object.keys(ix.key || {}); @@ -18,6 +18,15 @@ const sameKey = (ix) => { && ix.key.organizationId === 1 && ix.key.month === 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 === INDEX_NAME && ix.unique === true && ix.partialFilterExpression?.legacyPeriod?.$exists === true; + /** * Migration: working (organizationId, month) legacy-usage partial-unique index (#3990). * @@ -68,6 +77,49 @@ const sameKey = (ix) => { * no-op the second time), drops the index it just created, and recreates it * — same end state, just an extra drop/create round-trip. * + * Skip-window fast path (#3990 review): before doing any of (b)-(d), if the + * index is ALREADY the exact target shape (name/key/unique/partialFilter — + * see `isExactTargetIndex`) AND a cheap existence probe finds no legacy + * document still missing `legacyPeriod`, the whole drop→backfill→recreate + * sequence is skipped. 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) — steady state never re-opens the + * window below. + * + * Residual unguarded window (HONEST — not fully eliminated): the fast path + * above only fires once the database has already converged. On the FIRST + * successful run against a given database (or any run that still has legacy + * documents to backfill), (b)-(d) still execute and there is a real window + * with no unique constraint live on `billingusages`. Migrations run at boot + * BEFORE `listen()`, so THIS instance is not serving yet — but on a rolling + * deploy, previously-deployed instances of the OLD code are still serving and + * writing `legacyPeriod: true` on insert (racy upsert, pre-#3990 behaviour). + * If two such writes for the same (organizationId, month) land in the window, + * step (d)'s `createIndex` throws a raw duplicate-key error (E11000) instead + * of the pre-check's actionable message. That failure is now CAUGHT and + * converted into the same loud, actionable abort as (a) — re-deriving and + * naming the offending pair(s) — so the failure mode this migration produces + * is always the documented "abort loud, remediate, re-run" path, never a bare + * driver error. `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. + * For a STRICT guarantee (no window at all, not even a caught one), run this + * deploy during a maintenance window or scale to a single instance before + * rolling forward — the same operational note as other index-swap migrations + * in this file's project (see MIGRATIONS.md, "Boot awaits index builds; + * billing usage month-index partial-filter fix"). A fully atomic swap was + * evaluated (build the new spec under a temporary name, cut over, drop the + * old one — the trick the email-CI-unique-index migration uses) and rejected + * here: that trick only works because the OLD and NEW specs there could + * coexist under two different index NAMES with the SAME constraint semantics + * throughout; here the schema declares one canonical name + * (`organizationId_1_month_1`) for both the pre-existing (never-built, since + * it was invalid) and the fixed spec, and the actual goal of the drop is not + * a rename but making way for the backfill write itself — there is no second + * index that can hold the constraint meanwhile. + * * autoIndex race: mongoose autoIndex:true (the default — db.options sets no * override) builds the schema-declared twin on connect; identical specs make * the race benign. This migration is the AUTHORITATIVE creator on @@ -119,6 +171,25 @@ export async function up() { throw err; } + // ── Skip-window fast path (#3990 review) ── If the index is already the + // exact target shape, probe (cheap, read-only) whether any legacy document + // still needs the `legacyPeriod` backfill. When both hold, (b)-(d) below — + // and the unguarded window they open — are entirely unnecessary: there is + // nothing left to write, so nothing can race a live constraint. This is the + // common case on every boot after the first one that ever converged this + // database (see the migration header for the residual window that remains + // when this fast path does NOT apply). + if (existing.some(isExactTargetIndex)) { + const pending = await usages.findOne( + { weekKey: { $exists: false }, legacyPeriod: { $exists: false } }, + { projection: { _id: 1 } }, + ); + if (!pending) { + console.info('[migration] usage-month-index-partial-filter: index already matches the target spec and nothing left to backfill — skipping drop/recreate window entirely'); + return; + } + } + // ── (b) Drop the partial index if present, BEFORE the backfill write below. // It may already be LIVE AND EMPTY — boot's awaitIndexBuilds() builds the // schema-declared twin before this migration runs — or a divergent @@ -147,11 +218,42 @@ export async function up() { } // ── (d) Recreate the partial-unique index (same spec as the schema declaration) ── - await usages.createIndex(INDEX_KEY, { - unique: true, - name: INDEX_NAME, - partialFilterExpression: { legacyPeriod: { $exists: true } }, - }); + // A concurrent old-instance write (see migration header: previously-deployed + // pods still serving during a rolling deploy) can land a duplicate + // (organizationId, month) pair in the (b)-(d) window despite (a)'s + // pre-check, surfacing here as a raw E11000 on index build. Re-derive and + // name the offending pair(s) so this failure mode is always the same loud, + // actionable abort as (a) — never a bare driver error — and let the caller + // (lib/services/migrations.js#runMigration) unclaim so the next boot's (a) + // pre-check catches it up front. + try { + await usages.createIndex(INDEX_KEY, { + unique: true, + name: INDEX_NAME, + partialFilterExpression: { legacyPeriod: { $exists: true } }, + }); + } catch (err) { + if (err?.code !== 11000) throw err; + + const raceDuplicates = await usages + .aggregate([ + { $match: { legacyPeriod: { $exists: true } } }, + { $group: { _id: { organizationId: '$organizationId', month: '$month' }, count: { $sum: 1 }, ids: { $push: '$_id' } } }, + { $match: { count: { $gt: 1 } } }, + ]) + .toArray(); + 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-month-index-partial-filter ABORTED on index create: a duplicate (organizationId, month) pair landed during the drop→recreate window — ` + + `a concurrent write from a still-serving old instance on a rolling deploy (see migration header). ` + + `Remediate (delete/merge the duplicate rows) and re-run — the next boot's pre-check will catch this up front. Sample: ${sample}`, + ); + } console.info('[migration] usage-month-index-partial-filter: created partial-unique index on (organizationId, month)'); } diff --git a/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js b/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js index f438fe577..c6b35a0fa 100644 --- a/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js +++ b/modules/billing/tests/billing.usage.monthIndexPartialFilter.migration.integration.tests.js @@ -3,7 +3,7 @@ */ import mongoose from 'mongoose'; -import { beforeAll, afterAll, describe, test, expect } from '@jest/globals'; +import { jest, beforeAll, afterAll, describe, test, expect } from '@jest/globals'; import { bootstrap } from '../../../lib/app.js'; import mongooseService from '../../../lib/services/mongoose.js'; @@ -217,13 +217,72 @@ describe('Migration usage-month-index-partial-filter:', () => { expect(ix.partialFilterExpression).toEqual({ legacyPeriod: { $exists: true } }); } finally { await usages.deleteMany({ _id: { $in: [dupA, dupB] } }); + // Replay the FULL captured descriptor rather than a hand-picked + // key/name/unique/sparse subset: any other option on the original spec + // (partialFilterExpression, collation, ...) must round-trip too, or the + // restored index silently diverges from the schema declaration and the + // later `syncIndexes()` test fails on an unrelated, confusing mismatch. for (const ix of weekKeyIndexes) { - await usages.createIndex(ix.key, { name: ix.name, unique: ix.unique, sparse: ix.sparse }); + 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(); // restore the migrated end state for the suites that follow } }); + test('createIndex E11000 during the drop→recreate window aborts loud, same shape as the pre-check (#3990 review)', async () => { + // Simulates a concurrent old-instance write landing a duplicate pair in + // the (b)-(d) window on a rolling deploy (see migration header): force + // the full path to run (drop the index, seed an un-backfilled legacy + // doc), then make the FINAL 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 docId = new mongoose.Types.ObjectId(); + try { + try { await usages.dropIndex(INDEX_NAME); } catch (_) { /* already absent */ } + await usages.insertOne({ _id: docId, organizationId: orgId, month: '2024-01', counters: {} }); + + 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_month_1'), { code: 11000 }), + ); + const collectionSpy = jest.spyOn(mongoose.connection.db, 'collection').mockReturnValue(realCollection); + + try { + await expect(up()).rejects.toThrow(/duplicate \(organizationId, month\) pair landed during the drop.{1,3}recreate window/); + } finally { + collectionSpy.mockRestore(); + createIndexSpy.mockRestore(); + } + } finally { + await usages.deleteMany({ _id: docId }); + await up(); // restore the migrated end state for the suites that follow + } + }); + + test('skip-window fast path — a converged database does not drop/recreate the index (#3990 review)', 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 drop/recreate window entirely'))).toBe(true); + expect(messages.some((m) => m.includes('dropped index'))).toBe(false); + expect(messages.some((m) => m.includes('created partial-unique index'))).toBe(false); + } finally { + infoSpy.mockRestore(); + } + + const ix = await findIndex(INDEX_NAME); + expect(ix).toBeDefined(); + expect(ix.unique).toBe(true); + expect(ix.partialFilterExpression).toEqual({ legacyPeriod: { $exists: true } }); + }); + test('schema twin is IDENTICAL — syncIndexes() has nothing to drop or rebuild', async () => { await up(); const BillingUsage = mongoose.model('BillingUsage');