From 8ac3ca7fe5811632823141da4a41e606e210c9a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sat, 22 Aug 2026 22:50:29 +0000 Subject: [PATCH] forgetting a conversation needs both of its ids, not one twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `chat_session_events` is keyed on officer's SESSION KEY. The other three tables are keyed on the harness's TRANSCRIPT ID. Both are uuids for the same conversation, and `forgetChatSessions` took one list and passed it to all four — so the events delete matched nothing. Not sometimes: by construction, every time. A deleted conversation left its entire durable output behind. It survived two verifications because counting events BY TRANSCRIPT ID also returns zero by construction, and zero read as "cleaned". What settled it was reading an orphaned event instead of counting it — the payload carries both ids side by side and says outright which key the row is filed under. So the parameter is now named for its id space. That is the whole defence and it is the only one available: nothing else distinguishes the two, since both are uuids of the same shape, and a positional `string[]` cannot be got wrong loudly. Empty `sessionKeys` 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 a commander's children. Co-Authored-By: Claude Opus 5 --- .../officer_db/src/chat-session-cleanup.ts | 50 +++++++++++++++---- src/servers/api/chat/chat.ts | 18 ++++--- src/servers/api/command-hq/router.ts | 10 +++- 3 files changed, 61 insertions(+), 17 deletions(-) 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.