diff --git a/src/main/ai-vault-search/session-search-index-generation.test.ts b/src/main/ai-vault-search/session-search-index-generation.test.ts index 35883a62b07..a3fffd2648f 100644 --- a/src/main/ai-vault-search/session-search-index-generation.test.ts +++ b/src/main/ai-vault-search/session-search-index-generation.test.ts @@ -146,10 +146,13 @@ it('moves the generation forward when retention cuts a session loose', async () } }) -it('leaves the generation alone while a purge reclaims rows nothing can reach', async () => { - // The drain writes only `messages`, so it moves nothing. Bumping there would - // refuse every outstanding cursor once per batch, for rows whose session row - // is already gone and which therefore answer no search. +it('moves the generation when a purge reclaims rows nothing can reach', async () => { + // The drain writes only `messages`, and for a while that was argued to change + // no answer. Retrieval never saw those rows; the typo repair's dictionary + // did, because `messages_vocab` is a view over the FTS b-tree and lists a + // term whether or not a reader can reach it. See + // `session-search-orphan-rows.test.ts` for the answer that moved. The price + // of fencing it is a cursor refused once per batch while a purge runs. const root = await tempRoot() const path = join(root, 'index.sqlite') const db = reader(path) @@ -164,7 +167,30 @@ it('leaves the generation alone while a purge reclaims rows nothing can reach', expect(db.prepare('SELECT COUNT(*) AS c FROM messages').get()).not.toEqual({ c: 0 }) await store.purgeOlderThan(null) expect(db.prepare('SELECT COUNT(*) AS c FROM messages').get()).toEqual({ c: 0 }) - expect(readIndexGeneration(db)).toBe(orphaned) + expect(readIndexGeneration(db)).toBeGreaterThan(orphaned) + } finally { + store.close() + } +}) + +it("leaves the generation alone when a replace swaps a session's own rows", async () => { + // The same trigger must not fire here, or every re-read of a large transcript + // would move the generation once per deleted row on top of the one bump its + // file record already makes. A replace deletes rows whose session row still + // stands, which is what the trigger's `WHEN` clause tests. + const root = await tempRoot() + const path = join(root, 'index.sqlite') + const db = reader(path) + const store = new SessionSearchStore(path, (error) => { + throw error + }) + try { + await indexOneTranscript(root, store) + const rows = db.prepare('SELECT COUNT(*) AS c FROM messages').get() as { c: number } + const indexed = readIndexGeneration(db) + db.prepare('DELETE FROM messages WHERE session_row_id IN (SELECT id FROM sessions)').run() + expect(rows.c).toBeGreaterThan(0) + expect(readIndexGeneration(db)).toBe(indexed) } finally { store.close() } diff --git a/src/main/ai-vault-search/session-search-index-generation.ts b/src/main/ai-vault-search/session-search-index-generation.ts index 9f4ff91eb6c..21ed3890cdf 100644 --- a/src/main/ai-vault-search/session-search-index-generation.ts +++ b/src/main/ai-vault-search/session-search-index-generation.ts @@ -9,7 +9,8 @@ const GENERATION_KEY = 'index_generation' export const SESSION_SEARCH_GENERATION_TRIGGERS = [ 'search_generation_file_insert', 'search_generation_file_update', - 'search_generation_file_delete' + 'search_generation_file_delete', + 'search_generation_orphan_reclaim' ] as const const BUMP = `INSERT INTO meta(key, value) VALUES ('${GENERATION_KEY}', '1') @@ -18,17 +19,30 @@ const BUMP = `INSERT INTO meta(key, value) VALUES ('${GENERATION_KEY}', '1') /** * The fence, as three triggers on `files`. * - * Why `files` and not `sessions` or `messages`. Every transaction the store - * opens that can change what a search returns writes this table, and nothing - * else does: a committed read upserts the file's cursor beside its rows, a - * chunk of a long read upserts the partial sentinel beside its prefix, - * `removeFile` deletes the row with the session, and retention deletes the - * file row in the same transaction as the session row. The one write path that - * does not touch `files` is retention's orphan drain, and that is exactly the - * one that must not bump: those rows are already unreachable — their session - * row is gone and every retrieval inner-joins `sessions` — so reclaiming them - * changes no answer, while bumping would refuse every outstanding cursor once - * per 256 rows. + * Why `files`. Every transaction the store opens that can change what a search + * returns writes this table: a committed read upserts the file's cursor beside + * its rows, a chunk of a long read upserts the partial sentinel beside its + * prefix, `removeFile` deletes the row with the session, and retention deletes + * the file row in the same transaction as the session row. + * + * And why `messages` as well, for orphans only. Retention's second half + * reclaims rows whose session row is already gone, and touches neither table + * above. It was left unfenced on the argument that those rows answer nothing, + * which is true of retrieval and was not true of the whole engine: the typo + * repair's dictionary is `messages_vocab`, a view over the FTS b-tree that + * lists a term whether or not a reader can reach the rows carrying it, and + * reclaiming them moved which word a query was repaired to. The repair now + * counts live rows instead, so the common case is fixed at its source; this + * trigger is what makes the fence true rather than nearly true, because the + * vocabulary still decides which candidates survive its scan limit. + * + * The `WHEN` clause is what keeps it free. A replace and a `removeFile` delete + * a session's rows while its `sessions` row still stands, so neither fires + * here, and both already bump through `files`. Only the drain deletes a row + * whose session is gone. The cost of the fence is real and worth naming: a + * cursor outstanding while a purge runs is refused once per batch, which + * `SessionSearchCursorError` reports as `stale-generation` so a caller + * re-issues page one rather than showing anyone an error. * * A trigger rather than a call the writer makes, for two reasons. PR 4 does not * own the writer, and more importantly the fence has to hold for writers this @@ -52,6 +66,10 @@ END; CREATE TRIGGER IF NOT EXISTS search_generation_file_delete AFTER DELETE ON files BEGIN ${BUMP} END; +CREATE TRIGGER IF NOT EXISTS search_generation_orphan_reclaim AFTER DELETE ON messages +WHEN NOT EXISTS (SELECT 1 FROM sessions WHERE id = OLD.session_row_id) BEGIN + ${BUMP} +END; ` /** diff --git a/src/main/ai-vault-search/session-search-orphan-rows.test.ts b/src/main/ai-vault-search/session-search-orphan-rows.test.ts index 6656a1081e1..dfda2303104 100644 --- a/src/main/ai-vault-search/session-search-orphan-rows.test.ts +++ b/src/main/ai-vault-search/session-search-orphan-rows.test.ts @@ -1,4 +1,4 @@ -import { afterEach, expect, it } from 'vitest' +import { afterEach, describe, expect, it } from 'vitest' import type SyncDatabase from '../sqlite/sync-database' import { addSyntheticSession, @@ -6,8 +6,10 @@ import { type SessionSearchHarness } from './session-search-engine-test-fixture' import { identifierShadowText } from './session-search-identifier-split' +import { readIndexGeneration } from './session-search-index-generation' import { planSessionSearchQuery } from './session-search-query-planner' import { sessionSearchSnippet } from './session-search-snippet' +import type { SessionSearchCursorError } from './session-search-page-cursor' import { SessionSearchTypoRepair } from './session-search-typo-repair' // Retention deletes a session row in one small transaction and reclaims its @@ -34,7 +36,7 @@ afterEach(async () => { }) /** Two rows in the FTS table and the vocabulary, and no session row for them. */ -function plantOrphans(db: SyncDatabase): number[] { +function plantOrphans(db: SyncDatabase, text: string = ORPHAN_TEXT): number[] { const rowids: number[] = [] for (let n = 0; n < 2; n++) { const rowid = Number( @@ -44,7 +46,7 @@ function plantOrphans(db: SyncDatabase): number[] { ) db.prepare( 'INSERT INTO messages_fts(rowid,user_text,assistant_text,tool_text,identifiers) VALUES (?,?,?,?,?)' - ).run(rowid, ORPHAN_TEXT, '', '', identifierShadowText(ORPHAN_TEXT)) + ).run(rowid, text, '', '', identifierShadowText(text)) rowids.push(rowid) } return rowids @@ -105,3 +107,80 @@ it('still answers for the live session beside them', async () => { const { harness: open } = await withOrphans() expect(open.engine.search({ query: 'haystack' }).hits.map((hit) => hit.sessionId)).toEqual(['1']) }) + +// Reclaiming those rows is the other half. The drain deletes only from +// `messages`, so for a long time it was argued to change no answer and left +// outside the generation fence. Retrieval never saw them, but the typo repair's +// dictionary is `messages_vocab`, a view over the FTS b-tree that lists a term +// whether or not a reader can reach the rows carrying it — so the drain moved +// which word a query was repaired to, under a cursor that was still honoured. +describe('a purge reclaiming rows nothing can reach', () => { + /** A live session and a purged one that both carry `text`. */ + async function withReclaimable(): Promise { + harness = await openSessionSearchHarness('ss-orphan-drain') + // Two live rows, which is what makes `marmoset` eligible as a repair at all. + addSyntheticSession(harness.db, { id: 1, text: 'the marmoset lives here', rows: 2 }) + plantOrphans(harness.db) + return harness + } + + it('answers the same before and after, because the repair counts live rows', async () => { + const open = await withReclaimable() + const before = open.engine.search({ query: 'marmosett' }) + expect(before.planner.repairedTerms).toEqual(['marmoset']) + expect(before.hits.map((hit) => hit.sessionId)).toEqual(['1']) + + await open.store.purgeOlderThan(null) + expect(open.db.prepare('SELECT count(*) AS c FROM messages').get()).toEqual({ c: 2 }) + + const after = open.engine.search({ query: 'marmosett' }) + expect(after.planner.repairedTerms).toEqual(before.planner.repairedTerms) + expect(after.hits.map((hit) => hit.sessionId)).toEqual(before.hits.map((hit) => hit.sessionId)) + }) + + it('moves the generation anyway, so no cursor spans it', async () => { + // The repair counting live rows fixes the common case. It does not make the + // drain provably inert: `messages_vocab` still decides which candidates + // survive its scan limit, and reclaiming a term's last row changes where + // that limit cuts. The fence is what covers the rest, at the price of + // refusing a cursor once per batch while a purge runs. + const open = await withReclaimable() + // A second live session, so page one has a page two to be refused. + addSyntheticSession(open.db, { id: 2, text: 'the marmoset again', rows: 2 }) + const page = open.engine.search({ query: 'marmoset', limit: 1 }) + expect(page.page.cursor).not.toBeNull() + const before = readIndexGeneration(open.db) + + await open.store.purgeOlderThan(null) + + expect(readIndexGeneration(open.db)).toBeGreaterThan(before) + try { + open.engine.search({ query: 'marmoset', limit: 1, cursor: page.page.cursor! }) + expect.unreachable('a cursor must not span a purge') + } catch (error) { + expect((error as SessionSearchCursorError).rejection).toBe('stale-generation') + } + }) + + it('picks the same repair when an unreachable spelling was the more common one', async () => { + // Two candidates equally close to the query. `marmosetx` led on the old + // ranking only because two of its rows belonged to a session retention had + // already cut loose, so the drain swapped the repair under a live cursor. + harness = await openSessionSearchHarness('ss-orphan-drain-tie') + const db = harness.db + for (let id = 1; id <= 4; id++) { + addSyntheticSession(db, { id, text: `marmosetx session${id}` }) + } + for (let id = 5; id <= 9; id++) { + addSyntheticSession(db, { id, text: `marmosetq session${id}` }) + } + plantOrphans(db, 'marmosetx') + + const before = harness.engine.search({ query: 'marmosett' }) + expect(before.planner.repairedTerms).toEqual(['marmosetq']) + await harness.store.purgeOlderThan(null) + expect(harness.engine.search({ query: 'marmosett' }).planner.repairedTerms).toEqual( + before.planner.repairedTerms + ) + }) +})