From abaeebbefa3072dadba9fb33d79cb68b29faa71f Mon Sep 17 00:00:00 2001 From: Katia Bulatova Date: Thu, 6 Aug 2026 13:48:58 +0000 Subject: [PATCH] fix(webapp): make a repeated watch card close leave the revision alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `settleInvestigationStateAndCloseCard()` bumped the revision before it looked at the transcript, so a redelivered or replayed action settled the row a second time while the append refused the duplicate card. The row then held revision 2 and the transcript's terminal card revision 1: the live run rendered 2, a refresh rendered 1, for a tool advertised as idempotent on the action id. The transaction now locks the investigation, checks the tenancy triple, locks the chat, and looks for the message id before it writes anything. An action already in the transcript returns the stored card and the current revision untouched. Lock order stays investigation then chat, matching `persistTurn` and the sweep's `settleInvestigationAndCloseCard`; taking the chat first here would deadlock against them. A missing or deleted chat was indistinguishable from an already-closed card — both came back `closed: false` — and the settle committed anyway, which is exactly the terminal-row-without-a-card the transaction exists to prevent. It is now `{ ok: false, error: "chat_missing" }`, and the watch lane logs it as the race it is rather than a fault. --- .../dashboardAgentWatchCardAtomicity.test.ts | 65 +++++++++++++- .../dashboard-agent-db/src/queries.ts | 85 ++++++++++++++++++- .../dashboard-agent/src/watch-actions.ts | 11 +-- 3 files changed, 151 insertions(+), 10 deletions(-) diff --git a/apps/webapp/test/dashboardAgentWatchCardAtomicity.test.ts b/apps/webapp/test/dashboardAgentWatchCardAtomicity.test.ts index 89cad2079..7d11d2bc1 100644 --- a/apps/webapp/test/dashboardAgentWatchCardAtomicity.test.ts +++ b/apps/webapp/test/dashboardAgentWatchCardAtomicity.test.ts @@ -5,6 +5,7 @@ import { getInvestigation, listStaleOpenInvestigations, settleInvestigationStateAndCloseCard, + softDeleteChat, upsertInvestigationRevision, type DashboardAgentDb, type DashboardAgentDbClient, @@ -121,7 +122,8 @@ describe("closing a consented watch investigation's card", () => { investigationStateSchema.parse((await getInvestigation(agentDb, { id }))?.state).outcome ).toBe("inconclusive"); - // The lane dedupes on the action, so a redelivered kick closes nothing twice. + // The lane dedupes on the action, so a redelivered kick closes nothing twice — + // and must not bump the revision, or the row runs ahead of the stored card. const again = await settleInvestigationStateAndCloseCard(agentDb, { id, chatId, @@ -130,8 +132,65 @@ describe("closing a consented watch investigation's card", () => { state: forceSettledInvestigationState(openState()), messageId: MESSAGE_ID, }); - expect(again).toMatchObject({ ok: true, closed: false }); - expect((await transcript(chatId)).length).toBe(1); + expect(again).toMatchObject({ ok: true, id, revision: 1, closed: false }); + expect((await getInvestigation(agentDb, { id }))?.revision).toBe(1); + + const after = await transcript(chatId); + expect(after.map((message) => message.id)).toEqual([MESSAGE_ID]); + expect(after[0]!.parts[0]!.output.blocks[0]).toMatchObject({ id, revision: 1 }); + // The replayed result is the card the transcript holds, not a second rendering. + expect((again as { card: unknown }).card).toEqual(after[0]); + }, + 30_000 + ); + + postgresTest( + "settles nothing when the chat was deleted, so the sweep still selects the row", + async ({ prisma, postgresContainer }) => { + const chatId = "chat_watch_card_deleted"; + await boot(prisma, postgresContainer.getConnectionUri(), chatId); + const id = await seed(chatId, openState()); + await softDeleteChat(agentDb, { chatId, userId: USER_ID }); + + expect( + await settleInvestigationStateAndCloseCard(agentDb, { + id, + chatId, + projectRef: PROJECT_REF, + environmentRef: ENV_REF, + state: forceSettledInvestigationState(openState()), + messageId: MESSAGE_ID, + }) + ).toEqual({ ok: false, error: "chat_missing" }); + + const row = await getInvestigation(agentDb, { id }); + expect(row?.revision).toBe(0); + expect((row!.state as { outcome?: string }).outcome).toBe("in_progress"); + }, + 30_000 + ); + + postgresTest( + "settles nothing when the chat row was never there", + async ({ prisma, postgresContainer }) => { + const chatId = "chat_watch_card_absent"; + await boot(prisma, postgresContainer.getConnectionUri(), "chat_watch_card_present"); + const id = await seed(chatId, openState()); + + expect( + await settleInvestigationStateAndCloseCard(agentDb, { + id, + chatId, + projectRef: PROJECT_REF, + environmentRef: ENV_REF, + state: forceSettledInvestigationState(openState()), + messageId: MESSAGE_ID, + }) + ).toEqual({ ok: false, error: "chat_missing" }); + + const row = await getInvestigation(agentDb, { id }); + expect(row?.revision).toBe(0); + expect((row!.state as { outcome?: string }).outcome).toBe("in_progress"); }, 30_000 ); diff --git a/internal-packages/dashboard-agent-db/src/queries.ts b/internal-packages/dashboard-agent-db/src/queries.ts index 9473b4f40..c090b71f5 100644 --- a/internal-packages/dashboard-agent-db/src/queries.ts +++ b/internal-packages/dashboard-agent-db/src/queries.ts @@ -857,16 +857,43 @@ export type ClosedInvestigationCard = id: string; revision: number; card: InvestigationCardMessage; - /** False when that message id was already in the chat. */ + /** False when that message id was already in the chat, so this call wrote nothing. */ closed: boolean; } - | { ok: false; error: "not_found" | "context_mismatch" }; + | { ok: false; error: "not_found" | "context_mismatch" | "chat_missing" }; + +/** The stored message under `messageId`, read from the transcript rather than rebuilt. */ +async function storedMessageById( + tx: DashboardAgentDbOrTx, + params: { chatId: string; messageId: string } +): Promise { + const rows = await tx + .select({ + message: sql`( + select message + from jsonb_array_elements(coalesce(${chats.messages}, '[]'::jsonb)) as message + where message->>'id' = ${params.messageId} + limit 1 + )`, + }) + .from(chats) + .where(eq(chats.id, params.chatId)) + .limit(1); + + return rows[0]?.message ?? null; +} /** * Same atomicity as {@link settleInvestigationAndCloseCard}, for a caller that brings * its own terminal state and its own message id — the consented watch investigation, * which dedupes on the action rather than on the revision. * + * Idempotent on that message id, and the locks are what make it so: a redelivered + * action must not bump the revision, or the row moves ahead of the card the transcript + * already holds and the panel renders a different revision before and after a refresh. + * A missing or deleted chat settles nothing — a terminal row with no card is the + * permanent spinner this transaction exists to prevent. + * * Throwing is the point: the caller's retry only happens if the failure reaches it, and * a rolled-back settle leaves the `in_progress` row the stale sweep still selects. */ @@ -882,6 +909,56 @@ export async function settleInvestigationStateAndCloseCard( } ): Promise { return db.transaction(async (tx) => { + // Investigation before chat, the order `persistTurn` and the sweep's + // `settleInvestigationAndCloseCard` already take. Reversing it here would deadlock + // against them. + const investigationRows = await tx + .select({ + id: investigations.id, + revision: investigations.revision, + chatId: investigations.chatId, + projectRef: investigations.projectRef, + environmentRef: investigations.environmentRef, + }) + .from(investigations) + .where(eq(investigations.id, params.id)) + .limit(1) + .for("update"); + + const investigation = investigationRows[0]; + if (!investigation) return { ok: false, error: "not_found" }; + if ( + investigation.chatId !== params.chatId || + investigation.projectRef !== params.projectRef || + investigation.environmentRef !== params.environmentRef + ) { + return { ok: false, error: "context_mismatch" }; + } + + const chatRows = await tx + .select({ id: chats.id, deletedAt: chats.deletedAt }) + .from(chats) + .where(eq(chats.id, params.chatId)) + .limit(1) + .for("update"); + + const chat = chatRows[0]; + if (!chat || chat.deletedAt) return { ok: false, error: "chat_missing" }; + + const already = await storedMessageById(tx, { + chatId: params.chatId, + messageId: params.messageId, + }); + if (already) { + return { + ok: true, + id: investigation.id, + revision: investigation.revision, + card: already, + closed: false, + }; + } + const result = await upsertInvestigationRevision(tx, { id: params.id, chatId: params.chatId, @@ -905,6 +982,10 @@ export async function settleInvestigationStateAndCloseCard( chatId: params.chatId, message: card, }); + if (!closed) { + throw new Error(`Investigation ${result.id} settled without appending its closing card`); + } + return { ok: true, id: result.id, revision: result.revision, card, closed }; }); } diff --git a/internal-packages/dashboard-agent/src/watch-actions.ts b/internal-packages/dashboard-agent/src/watch-actions.ts index 455ea17e6..ab0acce37 100644 --- a/internal-packages/dashboard-agent/src/watch-actions.ts +++ b/internal-packages/dashboard-agent/src/watch-actions.ts @@ -623,11 +623,12 @@ async function closeCardInTranscript(args: { messageId: args.messageId, }); if (!result.ok) { - logger.error("dashboard-agent watch investigation couldn't close its card", { - chatId, - investigationId, - error: result.error, - }); + // A chat deleted mid-investigation is a race, not a fault: nothing settled, and + // there is no transcript left to close the card in. + const message = "dashboard-agent watch investigation couldn't close its card"; + const details = { chatId, investigationId, error: result.error }; + if (result.error === "chat_missing") logger.warn(message, details); + else logger.error(message, details); return; }