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; }