diff --git a/src/databases/officer_db/src/chat-session-cleanup.ts b/src/databases/officer_db/src/chat-session-cleanup.ts index 0f47daee..b287a4b1 100644 --- a/src/databases/officer_db/src/chat-session-cleanup.ts +++ b/src/databases/officer_db/src/chat-session-cleanup.ts @@ -10,9 +10,9 @@ import { nudges } from './nudges/schema'; ── Why this exists as one function ── - Four tables are keyed on a transcript id and none of them has a foreign key to anything, because the thing - they describe is a FILE — a transcript on disk, written by the harness. There is no row for a chat session - to reference, so the database cannot cascade and the delete path has to be explicit. + Four tables describe a conversation, and none has a foreign key to anything, because the thing they + describe is a FILE — a transcript on disk, written by the harness. There is no row for a chat session to + reference, so the database cannot cascade and the delete path has to be explicit. It was not. Deleting a conversation removed the transcript and left all four behind, for ever. Found on 2026-08-22 by counting rows after two commanders were deleted through the UI: `chat_session_models` held 36 @@ -24,19 +24,49 @@ import { nudges } from './nudges/schema'; ask for again. `nudges` is not: it is a SCHEDULE, and the tick in `nudger.ts` reads it. A nudge left behind by a deleted conversation is a timer that keeps firing at something that does not exist. - ── Why it takes a list ── + ── TWO id spaces, and this is where they were confused ── + + Three of these tables are keyed on the TRANSCRIPT id — the harness's uuid, which is also the filename. + `chat_session_events` is keyed on officer's own SESSION KEY, which is a different uuid for the same + conversation. Its rows carry both (`{sessionId, claudeSessionId}`), which is how the mismatch was finally + proved rather than argued. + + This function used to take one list and pass it to all four. So the events delete matched nothing, ever — + not sometimes, by construction — and a deleted conversation left its entire durable output behind. The + verification missed it twice, because counting events BY TRANSCRIPT ID also returns zero by construction, + and zero read as "cleaned". Found 2026-08-22 by reading an orphaned event payload instead of counting it. + + The parameter is named for its id space so a caller cannot pass the wrong one without writing the wrong + word. That is the entire defence: nothing else about the two distinguishes them, since both are uuids. + + ── Why each takes a list ── A `/clear` chain is several transcripts presenting as one conversation, and deleting it deletes every part. Forgetting one id and leaving its siblings is the same bug at a smaller scale, so the caller passes the ids it actually removed. */ -export async function forgetChatSessions(sessionIds: string[]): Promise { - if (!sessionIds.length) return; +type ForgetChatSessionsParams = { + /** Harness transcript uuids — the filenames that were removed, including every part of a `/clear` chain. */ + transcriptIds: string[]; + /** + * Officer's session keys for the same conversations, for `chat_session_events` alone. + * + * Empty is legitimate and common: a session spawned by `claude -p` outside officer's websocket never had a + * key and never wrote an event, which is exactly the case for a commander's children. + */ + sessionKeys?: string[]; +}; + +export async function forgetChatSessions({ transcriptIds, sessionKeys = [] }: ForgetChatSessionsParams): Promise { // Sequential rather than a transaction: these are independent tables with no invariant between them, and // a partial forget is strictly better than a failed delete that leaves the transcript gone and every row // in place. Each is idempotent — running it twice removes nothing the second time. - await db.delete(nudges).where(inArray(nudges.sessionId, sessionIds)); - await db.delete(chatSessionModels).where(inArray(chatSessionModels.sessionId, sessionIds)); - await db.delete(chatNotificationPrefs).where(inArray(chatNotificationPrefs.sessionId, sessionIds)); - await db.delete(chatSessionEvents).where(inArray(chatSessionEvents.sessionId, sessionIds)); + if (transcriptIds.length) { + await db.delete(nudges).where(inArray(nudges.sessionId, transcriptIds)); + await db.delete(chatSessionModels).where(inArray(chatSessionModels.sessionId, transcriptIds)); + await db.delete(chatNotificationPrefs).where(inArray(chatNotificationPrefs.sessionId, transcriptIds)); + } + if (sessionKeys.length) { + await db.delete(chatSessionEvents).where(inArray(chatSessionEvents.sessionId, sessionKeys)); + } } diff --git a/src/servers/api/chat/chat.ts b/src/servers/api/chat/chat.ts index f7d085a4..18bc972f 100644 --- a/src/servers/api/chat/chat.ts +++ b/src/servers/api/chat/chat.ts @@ -1,5 +1,5 @@ import type { Context } from 'hono'; -import { emitToSession } from './websocket'; +import { emitToSession, sessionKeyForTranscript } from './websocket'; import { pauseBackgroundTask, restartBackgroundTask } from './task-restart'; import { createRouter } from '../../create-router'; import * as errors from '@@/custom-errors'; @@ -422,11 +422,17 @@ chatRouter.delete('/sessions/:id', async (ctx) => { const removed = deleteClaudeSession(who, cwd, id); if (!removed.length) return ctx.text('Not found', 404); - // The transcript is gone; forget what the platform stored ABOUT it. Four tables are keyed on a transcript - // id with no foreign key — there is no row for a conversation to reference — so nothing cascades and this - // has to be explicit. One of them, `nudges`, is a live schedule: left behind it keeps firing at a - // conversation that no longer exists. - await forgetChatSessions(removed); + /* + The transcript is gone; forget what the platform stored ABOUT it. Four tables describe a conversation with + no foreign key — there is no row for one to reference — so nothing cascades and this has to be explicit. + One of them, `nudges`, is a live schedule: left behind it keeps firing at a conversation that is gone. + + Two id spaces, and they are not interchangeable: three tables are keyed on the transcript id, while + `chat_session_events` is keyed on officer's session key. Undefined here is the normal case for an old + conversation — no live session holds it, so nothing is keyed on a key to remove. + */ + const liveKey = sessionKeyForTranscript(id); + await forgetChatSessions({ transcriptIds: removed, sessionKeys: liveKey ? [liveKey] : [] }); return ctx.json({ ok: true }); }); diff --git a/src/servers/api/command-hq/router.ts b/src/servers/api/command-hq/router.ts index a5943c55..1091c4a8 100644 --- a/src/servers/api/command-hq/router.ts +++ b/src/servers/api/command-hq/router.ts @@ -608,7 +608,15 @@ commandHqRouter.delete('/commanders/:sessionId', async (ctx) => { output — except `nudges`, which is a live schedule that would keep firing at a conversation that no longer exists. */ - await forgetChatSessions(forgotten); + /* + `sessionKeys` carries the commander's own key and nothing else, on purpose. Children are spawned by + `claude -p` outside officer's websocket, so they never had a key and never wrote a durable event — an + empty entry for them would be noise, not caution. + + `sessionKey` is resolved above for the kill and reused here rather than looked up twice; when it is null + the session was already gone, and so were its events. + */ + await forgetChatSessions({ transcriptIds: forgotten, sessionKeys: sessionKey ? [sessionKey] : [] }); // Said back rather than a bare ok: this is the one irreversible action here, and the counts are what // make it auditable after the fact.