fix(organizations): membership user_org_unique partial index root-cause

modules/organizations/migrations/20260612120000-membership-user-org-unique-index.js

@@ -0,0 +1,142 @@
+/**
+ * Module dependencies
+ */
+import mongoose from 'mongoose';
+
+const INDEX_NAME = 'user_org_unique';
+const INDEX_KEY = { userId: 1, organizationId: 1 };
+
+/**
+ * @desc Exact-key match helper: a two-field index keyed (userId:1, organizationId:1)
+ * in that order. Order-aware so the (organizationId, userId) prefix indexes are
+ * never touched.
+ * @param {Object} ix - an index document from listIndexes()
+ * @return {boolean} true when the index key is exactly { userId:1, organizationId:1 }
+ */
+const sameKey = (ix) => {
+  const keys = Object.keys(ix.key || {});
+  return keys.length === 2 && keys[0] === 'userId' && keys[1] === 'organizationId'
+    && ix.key.userId === 1 && ix.key.organizationId === 1;
+};
+
+/**
+ * Migration: working (userId, organizationId) partial-unique membership index (#3841).
+ *
+ * The schema previously declared partialFilterExpression
+ * `{ userId: { $exists: true, $ne: null } }` — MongoDB does NOT support `$ne` in a
+ * partialFilterExpression, so server-side creation always failed. 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 duplicate-membership guard
+ * ran on application code alone (racy findOne-then-create in addMember /
+ * createJoinRequest).
+ *
+ * New spec (the schema declares the IDENTICAL twin): unique on
+ * { userId: 1, organizationId: 1 }, partialFilterExpression
+ * { userId: { $type: 'objectId' } }. `$type` is a supported partial-index operator
+ * and preserves the original intent: `userId: null` rows are BSON type null, not
+ * objectId, so they stay excluded and may repeat per organization.
+ *
+ * Safety / ordering:
+ *   (a) Pre-check for existing duplicate (userId, organizationId) pairs 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. The thrown error lists document ids only (PII-safe).
+ *   (b) Drop any divergent index first: a same-key index under another name
+ *       (phantom/legacy spec) or a namesake whose options drifted — either would
+ *       conflict with or shadow the canonical index. Unlike the email-index
+ *       migration there is no "window without a constraint" to protect: the
+ *       constraint never materialized anywhere (that IS the bug), and dropping
+ *       first avoids an IndexOptionsConflict on create.
+ *   (c) Create `user_org_unique`. 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.
+ *
+ * @returns {Promise<void>}
+ */
+export async function up() {
+  const memberships = mongoose.connection.db.collection('memberships');
+
+  // ── (a) Pre-check: refuse to run if duplicate (userId, organizationId) pairs exist ──
+  // Scope to rows the partial index will cover (userId is an ObjectId): null-userId
+  // rows are excluded by the partialFilterExpression and may legitimately repeat.
+  const duplicates = await memberships
+    .aggregate([
+      { $match: { userId: { $type: 'objectId' } } },
+      { $group: { _id: { userId: '$userId', organizationId: '$organizationId' }, count: { $sum: 1 }, ids: { $push: '$_id' } } },
+      { $match: { count: { $gt: 1 } } },
+    ])
+    .toArray();
+
+  if (duplicates.length > 0) {
+    // Emit only membership document ids — never user/org 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] membership-user-org-unique-index ABORTED: ${duplicates.length} duplicate membership (userId, organizationId) 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 memberships collection and
+  // autoIndex / syncIndexes will create the index from the schema declaration).
+  let existing = [];
+  try {
+    existing = await memberships.listIndexes().toArray();
+  } catch (err) {
+    if (err?.codeName === 'NamespaceNotFound' || err?.code === 26) {
+      console.info('[migration] membership-user-org-unique-index: memberships collection does not exist yet — nothing to migrate');
+      return;
+    }
+    throw err;
+  }
+
+  // ── (b) 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?.userId?.$type === 'objectId';
+    if (exactShape) {
+      hasIndex = true;
+    } else if (keyMatches || ix.name === INDEX_NAME) {
+      await memberships.dropIndex(ix.name);
+      console.info(`[migration] membership-user-org-unique-index: dropped divergent index '${ix.name}'`);
+    }
+  }
+
+  // ── (c) Create the partial-unique index (idempotent) ──
+  if (!hasIndex) {
+    await memberships.createIndex(INDEX_KEY, {
+      unique: true,
+      name: INDEX_NAME,
+      partialFilterExpression: { userId: { $type: 'objectId' } },
+    });
+    console.info('[migration] membership-user-org-unique-index: created partial-unique index on (userId, organizationId)');
+  } else {
+    console.info('[migration] membership-user-org-unique-index: 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 `user_org_unique` by hand if truly
+ * needed.
+ *
+ * @returns {void}
+ */
+export function down() {
+  console.warn(
+    '[migration] membership-user-org-unique-index DOWN: no-op; drop user_org_unique manually only alongside a deliberate schema revert',
+  );
+}

modules/organizations/models/organizations.membership.model.mongoose.js

@@ -69,11 +69,25 @@ MembershipMongoose.pre('validate', function enforcePendingSource() {
 });
 
 /**
- * Compound unique index to prevent duplicate memberships
+ * Compound unique index preventing duplicate (userId, organizationId) memberships.
+ *
+ * The partialFilterExpression uses `$type: 'objectId'` — NOT `$ne: null` — because
+ * MongoDB does not support `$ne` (or `$not`) inside a partialFilterExpression. The
+ * previous spec (`{ userId: { $exists: true, $ne: null } }`) was rejected
+ * server-side on every build attempt; mongoose autoIndex reports that failure on
+ * the model's 'index' event, where nothing listens, so the index NEVER existed on
+ * any deployed database and duplicate protection silently fell back to the racy
+ * findOne-then-create guards in the membership service (#3841). `$type: 'objectId'`
+ * preserves the original intent: `userId: null` rows are BSON type null (not
+ * objectId), so they stay outside the index and may repeat per organization.
+ *
+ * The migration (migrations/20260612120000-membership-user-org-unique-index.js) is
+ * the AUTHORITATIVE creator; this declaration is its IDENTICAL twin (same key,
+ * options, AND explicit name) so autoIndex / syncIndexes stay idempotent.
  */
 MembershipMongoose.index(
   { userId: 1, organizationId: 1 },
-  { unique: true, partialFilterExpression: { userId: { $exists: true, $ne: null } } },
+  { unique: true, name: 'user_org_unique', partialFilterExpression: { userId: { $type: 'objectId' } } },
 );
 
 /**

modules/organizations/tests/organizations.membershipUserOrgUniqueIndex.migration.integration.tests.js

@@ -0,0 +1,144 @@
+/**
+ * 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 { PENDING_SOURCES } from '../lib/constants.js';
+
+import { up } from '../migrations/20260612120000-membership-user-org-unique-index.js';
+
+const INDEX_NAME = 'user_org_unique';
+
+/**
+ * Migration `20260612120000-membership-user-org-unique-index` (#3841).
+ *
+ * The old schema spec used `$ne` in its partialFilterExpression — unsupported by
+ * MongoDB — so the unique (userId, organizationId) 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() creates `user_org_unique` with the EXACT spec (key, unique, $type 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 (userId, organizationId) pairs ABORT the migration
+ *     before any index work.
+ */
+describe('Migration membership-user-org-unique-index:', () => {
+  let memberships;
+  const orgId = new mongoose.Types.ObjectId();
+  const userId = new mongoose.Types.ObjectId();
+
+  beforeAll(async () => {
+    await bootstrap();
+    memberships = mongoose.connection.db.collection('memberships');
+  });
+
+  afterAll(async () => {
+    try {
+      await memberships.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 memberships collection.
+   * @param {string} name - the index name to look up.
+   * @returns {Promise<Object|undefined>} the index descriptor if found, undefined otherwise.
+   */
+  const findIndex = async (name) => {
+    const indexes = await memberships.listIndexes().toArray();
+    return indexes.find((ix) => ix.name === name);
+  };
+
+  test('up() creates user_org_unique with the exact spec', async () => {
+    await up();
+    const ix = await findIndex(INDEX_NAME);
+    expect(ix).toBeDefined();
+    expect(ix.key).toEqual({ userId: 1, organizationId: 1 });
+    expect(ix.unique).toBe(true);
+    expect(ix.partialFilterExpression).toEqual({ userId: { $type: 'objectId' } });
+  });
+
+  test('is idempotent — a second run leaves exactly one index on the key', async () => {
+    await up();
+    await up();
+    const indexes = await memberships.listIndexes().toArray();
+    const sameKey = indexes.filter((ix) => ix.key && ix.key.userId === 1 && ix.key.organizationId === 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 memberships.dropIndex(INDEX_NAME); } catch (_) { /* already absent */ }
+    await memberships.createIndex({ userId: 1, organizationId: 1 }, { name: 'userId_1_organizationId_1' });
+    await up();
+    expect(await findIndex('userId_1_organizationId_1')).toBeUndefined();
+    const ix = await findIndex(INDEX_NAME);
+    expect(ix).toBeDefined();
+    expect(ix.unique).toBe(true);
+  });
+
+  test('ABORTS on pre-existing duplicate (userId, organizationId) pairs without touching indexes', async () => {
+    const dupA = new mongoose.Types.ObjectId();
+    const dupB = new mongoose.Types.ObjectId();
+    try {
+      try { await memberships.dropIndex(INDEX_NAME); } catch (_) { /* already absent */ }
+      await memberships.insertOne({
+        _id: dupA, userId, organizationId: orgId, role: 'member', status: 'pending',
+        source: PENDING_SOURCES.OWNER_ADD, createdAt: new Date(), updatedAt: new Date(),
+      });
+      await memberships.insertOne({
+        _id: dupB, userId, organizationId: orgId, role: 'member', status: 'pending',
+        source: PENDING_SOURCES.JOIN_REQUEST, createdAt: new Date(), updatedAt: new Date(),
+      });
+      await expect(up()).rejects.toThrow(/duplicate membership/);
+      // Abort happened before any index work — the index is still absent.
+      expect(await findIndex(INDEX_NAME)).toBeUndefined();
+    } finally {
+      await memberships.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 Membership = mongoose.model('Membership');
+    const dropped = await Membership.syncIndexes();
+    expect(dropped).toEqual([]);
+    expect(await findIndex(INDEX_NAME)).toBeDefined();
+  });
+
+  test('DB backstop: a second pending row for the same (user, org) rejects with E11000', async () => {
+    await up();
+    const Membership = mongoose.model('Membership');
+    // Bypass the service guards (findOne-then-create) on purpose: the index itself
+    // must reject the duplicate, proving addMember/createJoinRequest now have a
+    // race-proof DB backstop instead of an application-level check alone.
+    await Membership.create({
+      userId, organizationId: orgId, role: 'member',
+      status: 'pending', source: PENDING_SOURCES.OWNER_ADD,
+    });
+    await expect(
+      Membership.create({
+        userId, organizationId: orgId, role: 'member',
+        status: 'pending', source: PENDING_SOURCES.JOIN_REQUEST,
+      }),
+    ).rejects.toMatchObject({ code: 11000 });
+    expect(await Membership.countDocuments({ userId, organizationId: orgId })).toBe(1);
+  });
+
+  test('null-userId rows stay OUTSIDE the partial index (may repeat per org)', async () => {
+    const Membership = mongoose.model('Membership');
+    // userId defaults to null; status defaults to active (no source required).
+    await Membership.create({ organizationId: orgId });
+    await Membership.create({ organizationId: orgId });
+    expect(await Membership.countDocuments({ organizationId: orgId, userId: null })).toBe(2);
+  });
+});