fix(dm): atomically create agent DM rooms

[lilyshen0722]

Summary

• add a canonical unordered pair key with a partial unique Mongo index for agent-dm rooms
• replace the read-then-save creation path with an atomic upsert; only the insert winner provisions PG and agent installations
• lazily claim a matching legacy two-member room before creating, so rollout does not add another room for an existing pair

Verification

• npm test -- --runInBand __tests__/services/dmService.agentDm.test.ts — 19/19
• Mutation proof: restoring the prior read-then-save path made the 16-way race regression fail with 14 distinct rooms.

Scope

This prevents new duplicate pairs and deterministically reuses a legacy two-member room. It deliberately does not delete or merge the one existing duplicate pair: safely reconciling its Mongo/PG message history and installations needs a separately approved data repair.

[lilyshen0722]

Gate at 4e0b7160. Ran it rather than read it: 19/19 on Node 22, and the index probed directly.

Approve, with one addition I'd like before the press — the mechanism this PR is named for has no test.

The body says "a partial unique Mongo index for agent-dm rooms" and the schema comment calls it "the storage-level backstop." I mutated it:

mutation	result
delete unique: true from the partial index	19/19 green
delete await Pod.init() from the harness	19/19 green
flip updatedExisting !== false → === false	1 red (creates AgentInstallation rows) — covered

Two independent signals that nothing in the suite can see the index. The 16-way race test does discriminate — your own mutation proof (read-then-save ⇒ 14 rooms) is real and I believe it — but what it discriminates is upsert vs read-then-save, not index vs no index. Those are different claims and only one of them is pinned.

That distinction matters in production specifically. In-process, sixteen Promise.all callers share one connection pool and the upsert alone is enough. Across backend pods it is not: two concurrent upserts that both miss the query predicate can both insert, and the unique index is the only thing that stops them. The suite is blind to exactly the deployment where the backstop earns its name.

The index itself is correct — I verified it separately, so this is a coverage gap, not a defect. Built index at Pod.init():

{"key":{"agentDmPairKey":1},"unique":true,
 "partialFilterExpression":{"type":"agent-dm","agentDmPairKey":{"$type":"string"}}}

Three cases, all passing against your head; drop them in as dmService.agentDmPairIndex.test.js or fold them into the existing file:

const base = (key, type = 'agent-dm') => ({
  name: 'x', type, joinPolicy: 'invite-only',
  createdBy: new mongoose.Types.ObjectId(),
  members: [new mongoose.Types.ObjectId(), new mongoose.Types.ObjectId()],
  ...(key === undefined ? {} : { agentDmPairKey: key }),
});

it('the partial unique index rejects a second row with the same pair key', async () => {
  await Pod.create(base('a:b'));
  await expect(Pod.create(base('a:b'))).rejects.toMatchObject({ code: 11000 });
});
it('two keyless legacy rows coexist — the partial filter excludes them', async () => {
  await Pod.create(base(undefined)); await Pod.create(base(undefined));
  expect(await Pod.countDocuments({ agentDmPairKey: { $exists: false } })).toBe(2);
});
it('a non-agent-dm row with the same key is not constrained', async () => {
  await Pod.create(base('c:d')); await Pod.create(base('c:d', 'chat'));
  expect(await Pod.countDocuments({ agentDmPairKey: 'c:d' })).toBe(2);
});

The second and third are not padding — they are what makes the first non-vacuous. Case 2 is the rollout precondition (the live duplicate pair is keyless, so the index build cannot fail on it), and case 3 pins the type half of the partial filter, which a later edit could drop without any other test noticing.

Three smaller things, none blocking.

1. Delivery matches house precedent, and I checked rather than assumed. Task.ts:92-100 uses the same partial-not-sparse + $type: 'string' idiom and its comment establishes boot-time autoIndex as the accepted build path, so no migration is needed here. One difference: Task pins an explicit index name. Yours doesn't, which is fine for a genuinely new index with no prior variant to match — but if a hand-built index ever lands on the live cluster first, the names have to agree or autoIndex will fight it.

2. result.lastErrorObject?.updatedExisting !== false fails toward the wrong side. When lastErrorObject is absent entirely, undefined !== false is true and the branch skips provisioning — for a room that was just inserted. That is a new room with no PG sync, which 404s on the first chat read and errors nowhere. Reachable only if the driver stops populating the field, so it is a hardening note, not a bug: keying on lastErrorObject?.upserted being present says what you mean and fails toward a redundant provision instead of a silent half-created room.

3. An undocumented behaviour change worth a comment in the code. The upsert does not run PodSchema.pre('save'), whose only job is if (this.isNew && !this.members.includes(this.createdBy)) this.members.push(this.createdBy). The sole production caller (agentsRuntime.ts:1058) passes creatorUserId: callerAgentUser._id, always one of the pair, so the hook was a no-op and nothing changes today. It is worth a line anyway, because the hook was a latent path to a three-member agent-dm — which DM_POD_TYPES_GUARD forbids — and losing it is an improvement a future editor might "restore" without knowing.

Scope agreement. Not merging the one live duplicate is the right call and the body says so plainly. The lazy-claim ordering handles it correctly: the oldest room takes the key, the second stays keyless, and a later call for that pair 11000s on the claim and returns the keyed winner. I read that path; I did not construct the two-duplicate fixture to execute it.

Not verified. Test & Coverage and E2E Tests were still pending when I gated — the other nine are green. I did not run the full backend suite, only this file. And I did not exercise the cross-process race that motivates the index; nothing in this repo's tiers can, which is the reason the direct index assertion above is the substitute.

[lilyshen0722]

Correcting my own gate. @pod-architect raised the §3.10 consequence at 10:14:54Z, four minutes before I posted mine, and I did not read it. Their finding outranks my coverage note: mine is "add a test", theirs is a reason not to merge yet. My gate said I read the lazy-claim ordering and called it correct — I checked that it terminates, not what it costs.

The mechanism, restated with what I could verify at origin/main.

findOne(...).sort({ createdAt: 1, _id: 1 }) claims the oldest keyless room. After the first call post-deploy, getOrCreateAgentDmRoom resolves that pair to the older room forever. The newer one is never returned to an agent again — commonly_dm_agent and every runtime path go through this function.

How reachable the orphan stays, checked rather than assumed:

• getPodsByType('agent-dm') (podController.ts:276-296) filters to caller membership for personal pod types, admins included. The members of an a2a DM are the two agents, so a human is not one — that listing returns neither room to a human.
• getPodById serves it, but only via the §3.7 fan-out (podController.ts:344-356), which requires already holding the pod id.

So "nothing returns them" is slightly stronger than I can confirm — the rows are reachable by id — but the practical shape is worse than a deferral implies: the agents lose the history unconditionally. They resume in the older room carrying none of the newer room's conversation, and nothing in the product merges the two. Which room holds the real conversation is the open question, and it is the one nobody has answered.

I cannot answer it either — I have no query path to the live DB from this seat. That makes it a precondition on the press, not a review nit.

A cheap change that is separable from the data repair, and which I'd want regardless of how the duplicate resolves:

const candidates = await Pod.countDocuments({
  type: 'agent-dm', members: { $all: [aId, bId] },
  agentDmPairKey: { $exists: false },
  $expr: { $eq: [{ $size: '$members' }, 2] },
});
if (candidates > 1) console.warn('[agent-dm] %d keyless rooms for pair %s — claiming oldest, orphaning %d', candidates, pairKey, candidates - 1);

findOne().sort() picks one of N and records nothing. The ambiguity is real exactly once per duplicated pair, it is invisible in every log today, and this makes the one moment it happens observable. If the answer turns out to be "claim the room with the most recent message" rather than the oldest, that is a one-line change to the sort — but it should be a decision with the count in front of it, not a default.

My earlier gate stands on its own terms — the index is correct, the three index cases are still worth adding, and the updatedExisting and pre('save') notes are unchanged. I'm withdrawing only the sentence where I called the lazy-claim ordering "correct": it is deterministic, which is not the same thing.