mirror of
https://github.com/stablyai/orca.git
synced 2026-09-28 00:02:41 +00:00
fix(ai-vault-search): fence the rows a purge reclaims after it cuts a session loose
Retention's second half deletes from `messages` alone and touched neither `files` nor `sessions`, so it moved no generation. The argument was that those rows answer nothing, which was true of retrieval and not of the engine: the typo repair's dictionary is a view over the FTS b-tree and listed them, so a drain running between two pages swapped the repair under a cursor that was still honoured, and a search that had answered stopped answering. The commit before this one fixes that at its source by counting live rows. It does not make the drain provably inert — the vocabulary still decides which candidates survive its scan limit, and reclaiming a term's last row moves where that limit cuts — so the fence is what covers the rest. A fourth trigger, on `messages`, with a `WHEN` clause that is the whole reason it is affordable: a replace and a `removeFile` delete a session's rows while its `sessions` row still stands, so neither fires, and both already bump through `files`. Only the drain deletes a row whose session is gone. The price is named rather than avoided: a cursor outstanding while a purge runs is now refused once per batch, which `SessionSearchCursorError` reports as `stale-generation` so a caller re-issues page one. The test that pinned the old contract is replaced by one for the new one, and by one proving a replace still does not fire it.
This commit is contained in:
@@ -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()
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
`
|
||||
|
||||
/**
|
||||
|
||||
@@ -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<SessionSearchHarness> {
|
||||
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
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user