Pin the 23505 race arm via module mocks, not live concurrency

HTTP round-trips through createUser serialize concurrent callers so the
winner typically commits its INSERT before any other caller reaches
getConversation — everyone else then takes the pre-check ownership
path and the 23505 arm never fires. The integration test's warn-count
assertion was therefore flaky and failed in CI.

- setup.race.test.ts: new deterministic unit test that uses mock.module
  on db/operations to throw a synthetic 23505 and drive the re-check to
  return a conflicting owner. Pins the conversation_create_race_23505
  warn + no-generic-failure contract on every run with no DB needed.
- setup.integration.test.ts: drop the warn-count assertion and the
  warnSpy that backed it; keep the concurrency-safety assertions
  (single winner, no generic failure log, DB row owned by the winner).
  Comment points at setup.race.test.ts for the branch-level coverage.

Author: maschlr

src/services/chat/__tests__/setup.integration.test.ts

@@ -42,8 +42,15 @@ describeIfSupabase("[integration] ensureUserAndConversation — concurrent 23505
     jest.restoreAllMocks();
   });
 
-  test("exactly one winner across N callers; the 23505 arm is exercised, not the pre-check arm", async () => {
-    const warnSpy = jest.spyOn(logger, "warn").mockImplementation(() => undefined);
+  test("exactly one winner under concurrency; no generic failure logged", async () => {
+    // End-to-end concurrency lock: with N callers racing against the same
+    // conversationId, the DB ends up with exactly one owning user, the losers
+    // all get the standard "Access denied" string, and setup.ts never takes
+    // the generic create_conversation_failed arm. The 23505 branch itself is
+    // not reliably reachable from here (HTTP round-trips serialize the
+    // createUser step so the winner typically commits before other callers
+    // reach getConversation) — that branch is pinned deterministically by
+    // setup.race.test.ts using module mocks.
     const errorSpy = jest.spyOn(logger, "error").mockImplementation(() => undefined);
 
     const results = await Promise.all(
@@ -58,17 +65,9 @@ describeIfSupabase("[integration] ensureUserAndConversation — concurrent 23505
       expect(loser.error).toBe("Access denied: conversation belongs to another user");
     }
 
-    // The 23505 branch is the only path that logs this message. Asserting it
-    // fired at least once proves a racing caller actually reached the INSERT
-    // and hit Postgres' unique-violation — i.e. the branch the fix targets
-    // was exercised, not just the ownership-mismatch pre-check path.
-    const raceWarns = warnSpy.mock.calls.filter(
-      ([, msg]) => msg === "conversation_create_race_23505"
-    );
-    expect(raceWarns.length).toBeGreaterThan(0);
-
-    // The race must be recognized via the errCode `in`-check, not fall through
-    // to the generic failure arm that logs `create_conversation_failed`.
+    // Whichever path each loser took (pre-check or 23505), none should have
+    // landed in the generic failure arm — a resurgence of the `{...err}`
+    // spread regression would log this for any caller that raced to INSERT.
     const genericFailureCall = errorSpy.mock.calls.find(
       ([, msg]) => msg === "create_conversation_failed"
     );

src/services/chat/__tests__/setup.race.test.ts

@@ -0,0 +1,54 @@
+import { beforeAll, describe, expect, jest, mock, test } from "bun:test";
+import logger from "../../../utils/logger";
+
+// Deterministic unit test for the Postgres 23505 unique-violation arm of
+// ensureUserAndConversation. Concurrency alone (see setup.integration.test.ts)
+// can't reliably force that branch because HTTP round-trips serialize the
+// createUser step ahead of the conversation race. Here we inject the 23505
+// failure directly via module mocks, so the branch is exercised on every run.
+describe("ensureUserAndConversation — 23505 race arm (unit)", () => {
+  let ensureUserAndConversation: typeof import("../setup").ensureUserAndConversation;
+  let getConversationCallCount = 0;
+
+  beforeAll(async () => {
+    mock.module("../../../db/operations", () => ({
+      createConversation: mock(async () => {
+        throw Object.assign(new Error("duplicate key value"), { code: "23505" });
+      }),
+      // Unused on this path but required so dynamic import of setup.ts resolves
+      // every named binding it declares at module scope.
+      createConversationState: mock(async () => ({})),
+      createState: mock(async () => ({})),
+      createUser: mock(async () => null),
+      // First call (pre-check): pretend the conversation doesn't exist yet.
+      // Second call (re-check inside the 23505 arm): someone else now owns it.
+      getConversation: mock(async () => {
+        getConversationCallCount += 1;
+        if (getConversationCallCount === 1) throw new Error("not found");
+        return { id: "conv-1", user_id: "someone-else" };
+      }),
+      getConversationState: mock(async () => ({})),
+      updateConversation: mock(async () => undefined),
+    }));
+    ({ ensureUserAndConversation } = await import("../setup"));
+  });
+
+  test("warns conversation_create_race_23505 and returns Access denied; no generic failure log", async () => {
+    const warnSpy = jest.spyOn(logger, "warn").mockImplementation(() => undefined);
+    const errorSpy = jest.spyOn(logger, "error").mockImplementation(() => undefined);
+
+    const result = await ensureUserAndConversation("user-1", "conv-1");
+
+    expect(result.success).toBe(false);
+    expect(result.error).toBe("Access denied: conversation belongs to another user");
+
+    const raceWarn = warnSpy.mock.calls.find(([, msg]) => msg === "conversation_create_race_23505");
+    expect(raceWarn).toBeDefined();
+
+    const genericFail = errorSpy.mock.calls.find(([, msg]) => msg === "create_conversation_failed");
+    expect(genericFail).toBeUndefined();
+
+    warnSpy.mockRestore();
+    errorSpy.mockRestore();
+  });
+});