From 4f590f8879a026fb11423bfb96d3fb749ae6e5a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sat, 22 Aug 2026 23:19:07 +0000 Subject: [PATCH] pin both id spaces with a test that fails against the old code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified by reverting the fix and watching it go red on exactly the case the live delete hit — the post-restart one, where no caller can supply a session key. A regression test never seen to fail proves nothing. It touches the real database, which the rest of the suite avoids on purpose, and that is the point: this defect was never in a decision, it was in which column a `delete` matched. A test built on a fake store would have passed against the broken version and the fixed one alike. Contained by construction — every id is a synthetic uuid in a block no harness produces, nothing reads live rows, nothing deletes by anything but those ids, and each test removes what it wrote. The third case is the one that will earn its keep later: it asserts a delete leaves a NEIGHBOURING session's events alone. Resolving a key from row payloads is exactly the kind of query that starts matching too much when somebody widens it. Co-Authored-By: Claude Opus 5 --- .../src/chat-session-cleanup.test.ts | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 src/databases/officer_db/src/chat-session-cleanup.test.ts diff --git a/src/databases/officer_db/src/chat-session-cleanup.test.ts b/src/databases/officer_db/src/chat-session-cleanup.test.ts new file mode 100644 index 00000000..b85cc6bf --- /dev/null +++ b/src/databases/officer_db/src/chat-session-cleanup.test.ts @@ -0,0 +1,97 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import { inArray } from 'drizzle-orm'; +import { db } from './db'; +import { chatSessionEvents } from './chat-events/schema'; +import { chatSessionModels } from './chat-session-models/schema'; +import { forgetChatSessions } from './chat-session-cleanup'; + +/* + Deleting a conversation, across the two id spaces it actually spans. + + This file touches the real database, which the rest of the suite avoids on purpose. It has to: the defect + it exists to catch was never in a decision, it was in which column a `delete` matched. A test built on a + fake store would have passed against both the broken version and the fixed one. + + Safe because every id here is a synthetic uuid in a reserved block that no harness produces, and because + each test removes exactly what it wrote. Nothing reads live rows and nothing deletes by anything but these + ids. + + Two regressions are pinned here, and they are the same bug seen twice: + + 1. `chat_session_events` is keyed on officer's SESSION KEY while everything else is keyed on the TRANSCRIPT + id. One list passed to all four tables matched nothing in this one, ever, by construction — and counting + events by transcript id to check also returns zero by construction, so it read as clean. + 2. Resolving that key from a live in-memory map only works for a session younger than the process. After a + restart it comes back null, the key list arrives empty, and the events survive a delete that reports + success. The key has to come from the rows, which carry `claudeSessionId` in their payload. +*/ + +const TRANSCRIPT = '00000000-dead-4000-8000-000000000001'; +const SESSION_KEY = '00000000-dead-4000-8000-0000000000ff'; + +afterEach(async () => { + await db.delete(chatSessionEvents).where(inArray(chatSessionEvents.sessionId, [SESSION_KEY])); + await db.delete(chatSessionModels).where(inArray(chatSessionModels.sessionId, [TRANSCRIPT])); +}); + +const countEvents = async () => + ( + await db + .select() + .from(chatSessionEvents) + .where(inArray(chatSessionEvents.sessionId, [SESSION_KEY])) + ).length; + +describe('forgetChatSessions', () => { + test('removes events keyed on the session key, found via the transcript id in their payload', async () => { + // Exactly the real shape: one `result` carries both ids, the rest carry neither. + await db.insert(chatSessionEvents).values([ + { sessionId: SESSION_KEY, event: { type: 'result', sessionId: SESSION_KEY, claudeSessionId: TRANSCRIPT } }, + { sessionId: SESSION_KEY, event: { type: 'assistant:text', text: 'no ids on this one' } }, + { sessionId: SESSION_KEY, event: { type: 'tool:start', toolName: 'Bash' } }, + ]); + expect(await countEvents()).toBe(3); + + // No `sessionKeys` — this is the post-restart case, where the caller cannot resolve one. + await forgetChatSessions({ transcriptIds: [TRANSCRIPT] }); + + // All three, not just the one carrying the id: resolving the key covers the whole session. + expect(await countEvents()).toBe(0); + }); + + test('an explicit session key still works when no event names the transcript', async () => { + await db + .insert(chatSessionEvents) + .values([{ sessionId: SESSION_KEY, event: { type: 'assistant:text', text: 'nothing identifying here' } }]); + + await forgetChatSessions({ transcriptIds: [TRANSCRIPT], sessionKeys: [SESSION_KEY] }); + expect(await countEvents()).toBe(0); + }); + + test('leaves another session alone', async () => { + const OTHER = '00000000-dead-4000-8000-0000000000ee'; + await db + .insert(chatSessionEvents) + .values([{ sessionId: OTHER, event: { type: 'assistant:text', text: 'theirs' } }]); + try { + await forgetChatSessions({ transcriptIds: [TRANSCRIPT], sessionKeys: [SESSION_KEY] }); + const left = await db + .select() + .from(chatSessionEvents) + .where(inArray(chatSessionEvents.sessionId, [OTHER])); + expect(left.length).toBe(1); + } finally { + await db.delete(chatSessionEvents).where(inArray(chatSessionEvents.sessionId, [OTHER])); + } + }); + + test('forgets the transcript-keyed tables too', async () => { + await db.insert(chatSessionModels).values({ sessionId: TRANSCRIPT, model: 'claude-code/opus', planMode: false }); + await forgetChatSessions({ transcriptIds: [TRANSCRIPT] }); + const left = await db + .select() + .from(chatSessionModels) + .where(inArray(chatSessionModels.sessionId, [TRANSCRIPT])); + expect(left.length).toBe(0); + }); +});