From 93963d55dfb5f32568ceaf77c6275017518eba63 Mon Sep 17 00:00:00 2001 From: Deathgiver Date: Sun, 20 Sep 2026 08:45:53 +0700 Subject: [PATCH] fix(api): stop resolving a contact's conversation and inbox arbitrarily MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The public `/v1/contacts/{identifier}/messages` handlers picked both halves of their target without an ordering, so which one they got was left to Postgres. `findByContactWithInboxes` ran `findFirst` with no `orderBy`, and a contact holds a DM conversation plus one comment thread per post they commented on — so the same contact could resolve to a different conversation on two consecutive calls. It now prefers the DM thread, which is what all three handlers are about. A contact who has only ever commented has no DM row and still resolves to their comment thread. Deciding that inside the lookup rather than behind a per-caller flag is the point: with only the send path preferring the DM thread, an integrator could POST a message, get 204, then list the conversation and not find it. There is no flag to forget, so send, list and get cannot drift. `resolveContactInboxForSend` took `contactInboxes[0]`. That relation is keyed by `contactId`, so it carries every inbox the contact has across every channel — an unordered `[0]` could address the wrong page entirely. It now comes from `findRecentByContactId`, the same resolution `resolveContactInboxForConversation` already used. Both orderings are spelled as SQL with `DESC NULLS LAST`. `lastActivityAt` is nullable with no default, so a freshly created conversation holds NULL and a plain DESC would rank a never-active conversation above every real one; the `{ lastActivityAt: "desc" }` object form cannot express the NULLS clause, and raw SQL reaches `orderBy` only through its callback form. This also matches `Conversation_workspaceId_lastActivityAt_id_idx`, declared `.desc().nullsLast()`. A wrong resolution failed asynchronously — the handler had already answered 204 — which is the silent-send shape reported in #879. Whether it accounts for the failure rate measured there is still unconfirmed, so this refs the issue rather than closing it. refs #879 --- ...ation-find-by-contact-with-inboxes.test.ts | 106 ++++++++++++++++++ ...ion-resolve-contact-inbox-for-send.test.ts | 80 ++++++++++++- packages/business/src/conversation/service.ts | 85 ++++++++++++-- 3 files changed, 253 insertions(+), 18 deletions(-) create mode 100644 packages/business/__tests__/conversation-find-by-contact-with-inboxes.test.ts diff --git a/packages/business/__tests__/conversation-find-by-contact-with-inboxes.test.ts b/packages/business/__tests__/conversation-find-by-contact-with-inboxes.test.ts new file mode 100644 index 0000000000..ad96a5f6cf --- /dev/null +++ b/packages/business/__tests__/conversation-find-by-contact-with-inboxes.test.ts @@ -0,0 +1,106 @@ +// @vitest-environment node +import { afterEach, describe, expect, test, vi } from "vitest" + +const { db } = await import("@chatbotx.io/database/client") +const { conversationModel } = await import("@chatbotx.io/database/schema") +const { conversationService } = await import("../src/conversation/service") + +const spyOnFindFirst = () => + vi.spyOn(db.query.conversationModel, "findFirst" as never) + +/** + * The literal text of the `orderBy` drizzle was handed. + * + * It arrives as drizzle's callback form — the only form that accepts a raw SQL + * expression — so it is invoked first. `queryChunks` then interleaves + * `StringChunk` (whose `value` is a string array) with column references, and + * those reference their table, so the object as a whole cannot be stringified. + */ +const orderBySql = (orderBy: unknown): string => { + const expression = + typeof orderBy === "function" + ? (orderBy as (table: unknown) => unknown)(conversationModel) + : orderBy + return ((expression as { queryChunks?: unknown[] }).queryChunks ?? []) + .flatMap((chunk) => (chunk as { value?: unknown }).value ?? []) + .filter((part): part is string => typeof part === "string") + .join("") +} + +afterEach(() => { + vi.restoreAllMocks() +}) + +describe("conversationService.findByContactWithInboxes", () => { + // `lastActivityAt` is nullable with no default, and Postgres puts NULLs FIRST + // on a plain DESC — so the object form `{ lastActivityAt: "desc" }` would + // rank a conversation that has never been active above every real one. + test("orders by lastActivityAt with NULLS LAST, not a bare DESC", async () => { + const findFirst = spyOnFindFirst().mockResolvedValue({ + id: "conv-dm", + contactInboxes: [], + } as never) + + await conversationService.findByContactWithInboxes({ + contactId: "contact-1", + workspaceId: "ws-1", + }) + + const { orderBy } = findFirst.mock.calls[0]?.[0] as { orderBy: unknown } + expect(orderBySql(orderBy)).toContain("DESC NULLS LAST") + }) + + // Every caller is a /v1/contacts/{identifier}/messages handler — send, list + // and get. When only the send path preferred the DM thread, an integrator + // could POST, get 204, then list and not find the message. There is no flag + // to forget, so the three cannot drift. + // `Conversation_contactId_dm_key` is unique on contactId where sourceId IS + // NULL, so this probe can match at most one row. + test("probes the DM thread first, with no caller opt-in", async () => { + const directMessage = { id: "conv-dm", sourceId: null, contactInboxes: [] } + const findFirst = spyOnFindFirst().mockResolvedValue(directMessage as never) + + const result = await conversationService.findByContactWithInboxes({ + contactId: "contact-1", + workspaceId: "ws-1", + }) + + expect(result).toEqual(directMessage) + expect(findFirst).toHaveBeenCalledTimes(1) + expect(findFirst).toHaveBeenCalledWith( + expect.objectContaining({ + where: { + contactId: "contact-1", + workspaceId: "ws-1", + sourceId: { isNull: true }, + }, + }), + ) + }) + + // A contact who has only ever commented has no DM thread to prefer. Falling + // through to the unfiltered lookup is what keeps them reachable. + test("falls back to any conversation when the contact has no DM thread", async () => { + const commentThread = { + id: "conv-comment", + sourceId: "page-1_post-1", + contactInboxes: [], + } + const findFirst = spyOnFindFirst() + .mockResolvedValueOnce(undefined as never) + .mockResolvedValueOnce(commentThread as never) + + const result = await conversationService.findByContactWithInboxes({ + contactId: "contact-1", + workspaceId: "ws-1", + }) + + expect(result).toEqual(commentThread) + expect(findFirst).toHaveBeenCalledTimes(2) + expect(findFirst).toHaveBeenLastCalledWith( + expect.objectContaining({ + where: { contactId: "contact-1", workspaceId: "ws-1" }, + }), + ) + }) +}) diff --git a/packages/business/__tests__/conversation-resolve-contact-inbox-for-send.test.ts b/packages/business/__tests__/conversation-resolve-contact-inbox-for-send.test.ts index 1e25e7f57c..422005237e 100644 --- a/packages/business/__tests__/conversation-resolve-contact-inbox-for-send.test.ts +++ b/packages/business/__tests__/conversation-resolve-contact-inbox-for-send.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, test, vi } from "vitest" const { conversationService } = await import("../src/conversation/service") +const { contactInboxService } = await import("../src/contact-inbox/service") describe("conversationService.resolveContactInboxForSend", () => { beforeEach(() => { @@ -28,22 +29,86 @@ describe("conversationService.resolveContactInboxForSend", () => { expect(result.conversation.id).toBe("conv-1") }) - test("falls back to the first contact inbox when inboxId is omitted", async () => { - const contactInboxA = { id: "ci-a", inboxId: "inbox-a" } - const contactInboxB = { id: "ci-b", inboxId: "inbox-b" } + // The relation on the conversation row is keyed by `contactId`, so it holds + // every inbox the contact has across every channel. Taking `[0]` of that + // unordered list could address the wrong page entirely. + test("resolves the most recently active contact inbox when inboxId is omitted", async () => { + const stale = { id: "ci-stale", inboxId: "inbox-a" } + const recent = { id: "ci-recent", inboxId: "inbox-b" } vi.spyOn(conversationService, "findByContactWithInboxes").mockResolvedValue( { id: "conv-1", - contactInboxes: [contactInboxA, contactInboxB], + contactInboxes: [stale, recent], } as never, ) + const findRecent = vi + .spyOn(contactInboxService, "findRecentByContactId") + .mockResolvedValue(recent as never) const result = await conversationService.resolveContactInboxForSend({ contactId: "contact-1", workspaceId: "ws-1", }) - expect(result.contactInbox).toEqual(contactInboxA) + expect(result.contactInbox).toEqual(recent) + expect(findRecent).toHaveBeenCalledWith({ + workspaceId: "ws-1", + contactId: "contact-1", + }) + }) + + // The DM-vs-comment-thread choice belongs to `findByContactWithInboxes` and + // is not a flag this caller passes — that is what keeps the send path and the + // list/get paths on the same conversation. Asserted here only so the send + // path cannot start routing around it. + test("takes the conversation findByContactWithInboxes settles on, passing no opt-in", async () => { + const findConversation = vi + .spyOn(conversationService, "findByContactWithInboxes") + .mockResolvedValue({ + id: "conv-dm", + sourceId: null, + contactInboxes: [{ id: "ci-a", inboxId: "inbox-a" }], + } as never) + vi.spyOn(contactInboxService, "findRecentByContactId").mockResolvedValue({ + id: "ci-a", + inboxId: "inbox-a", + } as never) + + const result = await conversationService.resolveContactInboxForSend({ + contactId: "contact-1", + workspaceId: "ws-1", + }) + + expect(findConversation).toHaveBeenCalledWith({ + contactId: "contact-1", + workspaceId: "ws-1", + }) + expect(result.conversation.id).toBe("conv-dm") + }) + + // A contact who has only ever commented has no DM conversation to prefer. + // Their comment conversation must still resolve, or comment-origin contacts + // become unreachable through the public API. + test("still resolves a comment conversation when the contact has no DM thread", async () => { + vi.spyOn(conversationService, "findByContactWithInboxes").mockResolvedValue( + { + id: "conv-comment", + sourceId: "page-1_post-1", + contactInboxes: [{ id: "ci-a", inboxId: "inbox-a" }], + } as never, + ) + vi.spyOn(contactInboxService, "findRecentByContactId").mockResolvedValue({ + id: "ci-a", + inboxId: "inbox-a", + } as never) + + const result = await conversationService.resolveContactInboxForSend({ + contactId: "contact-1", + workspaceId: "ws-1", + }) + + expect(result.conversation.id).toBe("conv-comment") + expect(result.contactInbox).toEqual({ id: "ci-a", inboxId: "inbox-a" }) }) test("404s when no conversation exists for the contact", async () => { @@ -76,13 +141,16 @@ describe("conversationService.resolveContactInboxForSend", () => { ).rejects.toMatchObject({ code: "notFound" }) }) - test("404s when the conversation has no contact inboxes at all", async () => { + test("404s when the contact has no contact inbox at all", async () => { vi.spyOn(conversationService, "findByContactWithInboxes").mockResolvedValue( { id: "conv-1", contactInboxes: [], } as never, ) + vi.spyOn(contactInboxService, "findRecentByContactId").mockResolvedValue( + undefined, + ) await expect( conversationService.resolveContactInboxForSend({ diff --git a/packages/business/src/conversation/service.ts b/packages/business/src/conversation/service.ts index 67f3152d91..40dae076bf 100644 --- a/packages/business/src/conversation/service.ts +++ b/packages/business/src/conversation/service.ts @@ -306,24 +306,81 @@ class ConversationService extends BaseService { return rows.length > 0 } + /** + * A contact can hold a DM conversation and one comment thread per post, so + * "the contact's conversation" is a choice, not a lookup. Without an order + * `findFirst` returned whichever row Postgres handed back — the same contact + * could resolve to a different conversation on two consecutive calls. + * + * The DM thread always wins. Every caller is a + * `/v1/contacts/{identifier}/messages` handler — send, list and get — and a + * direct message is what all three are about, so none of them wants a post's + * comment thread. Deciding it here rather than behind a per-caller flag is + * the point: when the send path preferred the DM thread and the read paths + * did not, an integrator could POST a message, get 204, then list the + * conversation and not find it. The three cannot drift if there is nothing + * to pass. A contact who has only ever commented has no DM row to prefer and + * still resolves to their comment thread. + * + * The DM probe is a single index hit: `Conversation_contactId_dm_key` is + * unique on `contactId` where `sourceId IS NULL`, so at most one row matches. + */ async findByContactWithInboxes(props: { contactId: string workspaceId: string tx?: DatabaseClient }): Promise { const { tx = db, contactId, workspaceId } = props - return (await tx.query.conversationModel.findFirst({ - where: { contactId, workspaceId }, - with: { contactInboxes: true }, - })) as ConversationWithContactInboxes | undefined + + const findFirstWhere = async (where: Record) => + (await tx.query.conversationModel.findFirst({ + where, + // Spelled as SQL because `lastActivityAt` is nullable with no default — + // a freshly created conversation has NULL — and Postgres puts NULLs + // FIRST on a plain DESC. The `{ lastActivityAt: "desc" }` object form + // cannot express the NULLS clause, so it would rank a conversation that + // has never been active above every real one. This also matches + // `Conversation_workspaceId_lastActivityAt_id_idx`, declared + // `.desc().nullsLast()`. Raw SQL reaches `orderBy` only through its + // callback form; the object form takes no expression. + orderBy: (table) => sql`${table.lastActivityAt} DESC NULLS LAST`, + with: { contactInboxes: true }, + })) as ConversationWithContactInboxes | undefined + + const directMessage = await findFirstWhere({ + contactId, + workspaceId, + sourceId: { isNull: true }, + }) + if (directMessage) { + return directMessage + } + + return await findFirstWhere({ contactId, workspaceId }) } /** * Shared by the public `/v1/contacts/{identifier}/messages|auto-replies|flows` * handlers: resolve the contact's conversation and the specific - * `ContactInbox` to send through (or the first one when `inboxId` is - * omitted), throwing the same 404 either way instead of repeating both - * lookups + both `notFoundException` calls at every call site. + * `ContactInbox` to send through, throwing the same 404 either way instead of + * repeating both lookups + both `notFoundException` calls at every call site. + * + * Both halves used to be picked arbitrarily, and a send that resolved wrong + * failed asynchronously — the handler had already answered 204. + * + * - The conversation comes from `findByContactWithInboxes`, which settles the + * DM-vs-comment-thread choice for the read paths too. + * - The `ContactInbox` now comes from `findRecentByContactId`. The relation + * on the conversation row is keyed by `contactId`, so it carries every + * inbox the contact has across every channel — taking `[0]` of an unordered + * list could address the wrong page entirely. An explicit `inboxId` still + * wins and is still matched against that relation. + * + * The channel is therefore decided by the `ContactInbox` alone — a + * `Conversation` carries no inbox or channel column, and the single DM row a + * contact owns is shared across all of their channels. So for a contact + * connected on more than one channel, omitting `inboxId` makes the channel a + * best guess ("most recently active"). Pass `inboxId` when it must be exact. */ async resolveContactInboxForSend(props: { contactId: string @@ -344,7 +401,10 @@ class ConversationService extends BaseService { const contactInbox = inboxId ? conversation.contactInboxes.find((ci) => ci.inboxId === inboxId) - : conversation.contactInboxes[0] + : await contactInboxService.findRecentByContactId({ + workspaceId, + contactId, + }) if (!contactInbox) { throw notFoundException("Conversation not found") } @@ -357,9 +417,8 @@ class ConversationService extends BaseService { * `createMessageAction`: resolve the `ContactInbox` to send an outgoing * message through, scoped to an already-identified `conversationId` rather * than `contactId` (see `resolveContactInboxForSend` for the public-API - * variant, which starts from `contactId` and falls back to the - * conversation's first `ContactInbox` instead of the most recently - * active one). + * variant, which starts from `contactId` and resolves the conversation too). + * Both settle on the same `ContactInbox` for a given contact. */ async resolveContactInboxForConversation(props: { conversation: Pick @@ -389,9 +448,11 @@ class ConversationService extends BaseService { // A contact can have multiple conversations (DM + comment threads), all // sharing the same ContactInbox — order by lastActivityAt so callers get // the conversation the contact is actually active in, not an arbitrary one. + // NULLS LAST is not optional here: the column is nullable with no default, + // so a plain DESC ranks a never-active conversation above every real one. return await tx.query.conversationModel.findFirst({ where: { contactId }, - orderBy: { lastActivityAt: "desc" }, + orderBy: (table) => sql`${table.lastActivityAt} DESC NULLS LAST`, }) }