fix(session-query): close review edge cases (round 4)
This commit is contained in:
@@ -24,8 +24,8 @@ The database is disposable but reset is guarded: a recognized incompatible searc
|
||||
|---|---:|---|
|
||||
| `path` | required | Dedicated derived-index SQLite path; `:memory:` is supported. |
|
||||
| `journalMode` | `wal` | `wal`, `delete`, `truncate`, or `persist`. |
|
||||
| `defaultLimit` | `20` | Page size when a request omits `limit`. |
|
||||
| `maxLimit` | `100` | Largest accepted request page size. |
|
||||
| `defaultLimit` | `20` | Page size when a request omits `limit`; at most `Number.MAX_SAFE_INTEGER - 1`. |
|
||||
| `maxLimit` | `100` | Largest accepted request page size; at most `Number.MAX_SAFE_INTEGER - 1`. |
|
||||
| `snippetChars` | `240` | Maximum snippet length in Unicode code points. |
|
||||
|
||||
## Tokenizer and limits
|
||||
|
||||
@@ -48,6 +48,7 @@ import {
|
||||
quoteFtsData,
|
||||
requestFingerprint,
|
||||
sanitizeFtsText,
|
||||
SQLITE_MAX_PAGE_LIMIT,
|
||||
} from './query.ts'
|
||||
|
||||
export {
|
||||
@@ -63,15 +64,19 @@ export const SESSION_QUERY_SQLITE_MAX_LIMIT = 100
|
||||
/** Default maximum snippet length in Unicode code points. */
|
||||
export const SESSION_QUERY_SQLITE_SNIPPET_CHARS = 240
|
||||
|
||||
// A serialized search tolerates one transient source change; repeated churn
|
||||
// fails instead of monopolizing the operation queue.
|
||||
const STABLE_OBSERVATION_ATTEMPTS = 2
|
||||
|
||||
/** SQLite session-search configuration. */
|
||||
export interface Config {
|
||||
/** Dedicated derived-index path; `:memory:` is supported for tests. */
|
||||
path: string
|
||||
/** SQLite journal mode. Defaults to `wal`. */
|
||||
journalMode?: JournalMode
|
||||
/** Page size when a request omits `limit`. Defaults to 20. */
|
||||
/** Page size when a request omits `limit`. At most `Number.MAX_SAFE_INTEGER - 1`; defaults to 20. */
|
||||
defaultLimit?: number
|
||||
/** Largest accepted page size. Defaults to 100. */
|
||||
/** Largest accepted page size. At most `Number.MAX_SAFE_INTEGER - 1`; defaults to 100. */
|
||||
maxLimit?: number
|
||||
/** Maximum snippet length in Unicode code points. Defaults to 240. */
|
||||
snippetChars?: number
|
||||
@@ -154,8 +159,8 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
static Config: z<Config> = z.object({
|
||||
path: z.string().required(),
|
||||
journalMode: z.union(['wal', 'delete', 'truncate', 'persist'] as const).default('wal'),
|
||||
defaultLimit: z.number().step(1).min(1).default(SESSION_QUERY_SQLITE_DEFAULT_LIMIT),
|
||||
maxLimit: z.number().step(1).min(1).default(SESSION_QUERY_SQLITE_MAX_LIMIT),
|
||||
defaultLimit: z.number().step(1).min(1).max(SQLITE_MAX_PAGE_LIMIT).default(SESSION_QUERY_SQLITE_DEFAULT_LIMIT),
|
||||
maxLimit: z.number().step(1).min(1).max(SQLITE_MAX_PAGE_LIMIT).default(SESSION_QUERY_SQLITE_MAX_LIMIT),
|
||||
snippetChars: z.number().step(1).min(1).default(SESSION_QUERY_SQLITE_SNIPPET_CHARS),
|
||||
})
|
||||
|
||||
@@ -206,14 +211,14 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
const signal = exec?.signal
|
||||
return this._serialized(signal, async () => {
|
||||
await this._ensureReady(signal)
|
||||
await this._reconcile(signal)
|
||||
const persistenceBinding = await this._reconcile(signal)
|
||||
assertNotAborted(signal)
|
||||
const generation = String(this._globalGeneration)
|
||||
const fingerprint = requestFingerprint(normalized)
|
||||
const offset = normalized.cursor === undefined
|
||||
? 0
|
||||
: decodeCursor(normalized.cursor, this._instance, 'sessions', fingerprint, generation)
|
||||
const rows = this._querySessions(normalized, offset)
|
||||
const rows = this._querySessions(normalized, offset, persistenceBinding)
|
||||
return page(rows, normalized.limit, row => this._sessionHit(row), cursorOffset => encodeCursor({
|
||||
version: 1,
|
||||
instance: this._instance,
|
||||
@@ -233,14 +238,14 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
const signal = exec?.signal
|
||||
return this._serialized(signal, async () => {
|
||||
await this._ensureReady(signal)
|
||||
await this._reconcile(signal)
|
||||
const persistenceBinding = await this._reconcile(signal)
|
||||
assertNotAborted(signal)
|
||||
const generation = this._targetGeneration(normalized.sessionId)
|
||||
const generation = this._targetGeneration(normalized.sessionId, persistenceBinding)
|
||||
const fingerprint = requestFingerprint(normalized)
|
||||
const offset = normalized.cursor === undefined
|
||||
? 0
|
||||
: decodeCursor(normalized.cursor, this._instance, 'events', fingerprint, generation)
|
||||
const rows = this._queryEvents(normalized, offset)
|
||||
const rows = this._queryEvents(normalized, offset, persistenceBinding)
|
||||
return page(rows, normalized.limit, row => this._eventHit(row), cursorOffset => encodeCursor({
|
||||
version: 1,
|
||||
instance: this._instance,
|
||||
@@ -316,7 +321,7 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
}
|
||||
}
|
||||
|
||||
private async _reconcile(signal: AbortSignal | undefined): Promise<void> {
|
||||
private async _reconcile(signal: AbortSignal | undefined): Promise<PersistenceBinding> {
|
||||
const db = this._requireDb()
|
||||
const persistedRows = db.prepare(
|
||||
'SELECT id, revision, generation FROM persisted_sessions',
|
||||
@@ -392,13 +397,14 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
if (pointerChanged) this._persistenceEpoch += 1
|
||||
this._localGeneration = nextLocalGeneration
|
||||
this._lastPersistenceIdentity = observation.persistenceBinding.identity
|
||||
return observation.persistenceBinding
|
||||
}
|
||||
|
||||
private async _observeStable(
|
||||
indexed: ReadonlyMap<SessionId, IndexedPersistedRow>,
|
||||
signal: AbortSignal | undefined,
|
||||
): Promise<Observation> {
|
||||
for (;;) {
|
||||
for (let attempt = 0; attempt < STABLE_OBSERVATION_ATTEMPTS; attempt += 1) {
|
||||
assertNotAborted(signal)
|
||||
const persistenceBinding = this._persistenceBinding
|
||||
const persistence = persistenceBinding.service
|
||||
@@ -446,6 +452,10 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
return { persistenceBinding, persisted, live }
|
||||
}
|
||||
}
|
||||
throw new SessionQueryError(
|
||||
'session-search persistence observation did not stabilize after one retry',
|
||||
'SESSION_QUERY_PERSISTENCE_FAILED',
|
||||
)
|
||||
}
|
||||
|
||||
private _mainGeneration(): number {
|
||||
@@ -540,7 +550,11 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
}
|
||||
}
|
||||
|
||||
private _querySessions(request: NormalizedSessionRequest, offset: number): SearchRow[] {
|
||||
private _querySessions(
|
||||
request: NormalizedSessionRequest,
|
||||
offset: number,
|
||||
persistenceBinding: PersistenceBinding,
|
||||
): SearchRow[] {
|
||||
const selected = selectedDocumentsSql()
|
||||
const sessionWhere = buildSessionWhere(request.sessionFilters)
|
||||
const eventWhere = buildEventWhere(request.eventFilters)
|
||||
@@ -562,7 +576,7 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
ORDER BY match_count DESC, document_length ASC, time DESC, session_id ASC, seq DESC
|
||||
LIMIT ? OFFSET ?
|
||||
`).all(
|
||||
...selectedDocumentsParams(request.query, this._persistenceBinding.service !== undefined),
|
||||
...selectedDocumentsParams(request.query, persistenceBinding.service !== undefined),
|
||||
...sessionWhere.params,
|
||||
...eventWhere.params,
|
||||
request.limit + 1,
|
||||
@@ -570,7 +584,11 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
) as unknown as SearchRow[]
|
||||
}
|
||||
|
||||
private _queryEvents(request: NormalizedEventRequest, offset: number): SearchRow[] {
|
||||
private _queryEvents(
|
||||
request: NormalizedEventRequest,
|
||||
offset: number,
|
||||
persistenceBinding: PersistenceBinding,
|
||||
): SearchRow[] {
|
||||
const selected = selectedDocumentsSql()
|
||||
const eventWhere = buildEventWhere(request.filters)
|
||||
const where = ['session_id = ?', eventWhere.sql].filter(Boolean).join(' AND ')
|
||||
@@ -581,7 +599,7 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
ORDER BY match_count DESC, document_length ASC, time DESC, seq DESC
|
||||
LIMIT ? OFFSET ?
|
||||
`).all(
|
||||
...selectedDocumentsParams(request.query, this._persistenceBinding.service !== undefined),
|
||||
...selectedDocumentsParams(request.query, persistenceBinding.service !== undefined),
|
||||
request.sessionId,
|
||||
...eventWhere.params,
|
||||
request.limit + 1,
|
||||
@@ -589,13 +607,13 @@ export class SessionSearchSqlite extends SessionSearchService {
|
||||
) as unknown as SearchRow[]
|
||||
}
|
||||
|
||||
private _targetGeneration(sessionId: SessionId): string {
|
||||
private _targetGeneration(sessionId: SessionId, persistenceBinding: PersistenceBinding): string {
|
||||
const db = this._requireDb()
|
||||
const live = db.prepare(
|
||||
'SELECT generation FROM temp.live_sessions WHERE id = ?',
|
||||
).get(sessionId) as { generation: number } | undefined
|
||||
if (live !== undefined) return `live:${live.generation}`
|
||||
if (this._persistenceBinding.service !== undefined) {
|
||||
if (persistenceBinding.service !== undefined) {
|
||||
const persisted = db.prepare(
|
||||
'SELECT generation FROM persisted_sessions WHERE id = ?',
|
||||
).get(sessionId) as { generation: number } | undefined
|
||||
@@ -850,8 +868,8 @@ function resolveConfig(config: Config): ResolvedConfig {
|
||||
if (typeof resolved.path !== 'string' || resolved.path.trim().length === 0) {
|
||||
throw invalidConfig('path must not be blank')
|
||||
}
|
||||
assertPositiveInteger('defaultLimit', resolved.defaultLimit)
|
||||
assertPositiveInteger('maxLimit', resolved.maxLimit)
|
||||
assertPageLimit('defaultLimit', resolved.defaultLimit)
|
||||
assertPageLimit('maxLimit', resolved.maxLimit)
|
||||
assertPositiveInteger('snippetChars', resolved.snippetChars)
|
||||
if (resolved.defaultLimit > resolved.maxLimit) {
|
||||
throw invalidConfig('defaultLimit must be less than or equal to maxLimit')
|
||||
@@ -865,6 +883,12 @@ function assertPositiveInteger(name: string, value: number): void {
|
||||
if (!Number.isInteger(value) || value < 1) throw invalidConfig(`${name} must be a positive integer`)
|
||||
}
|
||||
|
||||
function assertPageLimit(name: string, value: number): void {
|
||||
if (!Number.isSafeInteger(value) || value < 1 || value > SQLITE_MAX_PAGE_LIMIT) {
|
||||
throw invalidConfig(`${name} must be an integer between 1 and ${SQLITE_MAX_PAGE_LIMIT}`)
|
||||
}
|
||||
}
|
||||
|
||||
function invalidConfig(detail: string): SessionQueryError {
|
||||
return new SessionQueryError(
|
||||
`session-search SQLite config: ${detail}`,
|
||||
|
||||
@@ -20,6 +20,9 @@ export const FTS_HIGHLIGHT_START = '\uFDD0'
|
||||
/** Collision-free marker inserted after an FTS5 match by `highlight()`. */
|
||||
export const FTS_HIGHLIGHT_END = '\uFDD1'
|
||||
|
||||
/** Largest page size whose internal lookahead remains an exact SQLite integer binding. */
|
||||
export const SQLITE_MAX_PAGE_LIMIT = Number.MAX_SAFE_INTEGER - 1
|
||||
|
||||
/** Limit defaults needed to normalize a search request. */
|
||||
export interface QueryLimits {
|
||||
/** Page size used when the request omits one. */
|
||||
@@ -232,14 +235,17 @@ export function makeSnippet(markedText: string, maxChars: number): string {
|
||||
const characters = Array.from(clean)
|
||||
if (characters.length <= maxChars) return clean
|
||||
if (maxChars === 1) return '…'
|
||||
let start = Math.max(0, matchStart - Math.floor(maxChars / 3))
|
||||
let prefix = start > 0 ? '…' : ''
|
||||
const matchedIndex = Math.min(matchStart, characters.length - 1)
|
||||
let start = Math.max(0, matchedIndex - Math.floor(maxChars / 3))
|
||||
const prefix = start > 0 ? '…' : ''
|
||||
let suffix = '…'
|
||||
let contentLength = maxChars - prefix.length - suffix.length
|
||||
if (contentLength < 1) {
|
||||
start = 0
|
||||
prefix = ''
|
||||
contentLength = maxChars - 1
|
||||
start = matchedIndex
|
||||
suffix = ''
|
||||
contentLength = maxChars - prefix.length - suffix.length
|
||||
} else if (matchedIndex >= start + contentLength) {
|
||||
start = matchedIndex - contentLength + 1
|
||||
}
|
||||
let end = Math.min(characters.length, start + contentLength)
|
||||
if (end === characters.length) {
|
||||
@@ -326,9 +332,14 @@ function materializeMetadataFilters(
|
||||
|
||||
function normalizeLimit(value: number | undefined, limits: QueryLimits): number {
|
||||
const limit = value ?? limits.defaultLimit
|
||||
if (!Number.isInteger(limit) || limit < 1 || limit > limits.maxLimit) {
|
||||
const maxLimit = Math.min(limits.maxLimit, SQLITE_MAX_PAGE_LIMIT)
|
||||
if (
|
||||
!Number.isSafeInteger(limit)
|
||||
|| limit < 1
|
||||
|| limit > maxLimit
|
||||
) {
|
||||
throw new SessionQueryError(
|
||||
`session-search limit must be an integer between 1 and ${limits.maxLimit}`,
|
||||
`session-search limit must be an integer between 1 and ${maxLimit}`,
|
||||
'SESSION_QUERY_INVALID_LIMIT',
|
||||
)
|
||||
}
|
||||
|
||||
@@ -11,6 +11,7 @@ import {
|
||||
normalizeSessionRequest,
|
||||
quoteFtsData,
|
||||
requestFingerprint,
|
||||
SQLITE_MAX_PAGE_LIMIT,
|
||||
type NormalizedEventRequest,
|
||||
type NormalizedSessionRequest,
|
||||
} from '../src/query.ts'
|
||||
@@ -88,6 +89,12 @@ describe('SQLite search request normalization', () => {
|
||||
expect(() => normalizeEventRequest({ sessionId: SessionId('s'), query: 'x', limit }, limits))
|
||||
.toThrow(expectCode('SESSION_QUERY_INVALID_LIMIT'))
|
||||
}
|
||||
expect(() => normalizeEventRequest({
|
||||
sessionId: SessionId('s'),
|
||||
query: 'x',
|
||||
limit: SQLITE_MAX_PAGE_LIMIT + 1,
|
||||
}, { defaultLimit: 1, maxLimit: SQLITE_MAX_PAGE_LIMIT + 1 }))
|
||||
.toThrow(expectCode('SESSION_QUERY_INVALID_LIMIT'))
|
||||
})
|
||||
|
||||
it('materializes owned filter values during normalization', () => {
|
||||
@@ -214,7 +221,8 @@ describe('SQLite query identity and presentation', () => {
|
||||
expect(makeSnippet(`abcde${FTS_HIGHLIGHT_START}f${FTS_HIGHLIGHT_END}`, 1)).toBe('…')
|
||||
expect(makeSnippet('abcdefghij', 5)).toBe('abcd…')
|
||||
expect(makeSnippet(`ab${FTS_HIGHLIGHT_START}c${FTS_HIGHLIGHT_END}defghij`, 5)).toBe('…bcd…')
|
||||
expect(makeSnippet(`abcde${FTS_HIGHLIGHT_START}f${FTS_HIGHLIGHT_END}`, 2)).toBe('a…')
|
||||
expect(makeSnippet(`ab${FTS_HIGHLIGHT_START}c${FTS_HIGHLIGHT_END}defghij`, 3)).toBe('…c…')
|
||||
expect(makeSnippet(`abcde${FTS_HIGHLIGHT_START}f${FTS_HIGHLIGHT_END}`, 2)).toBe('…f')
|
||||
expect(makeSnippet(`abcde${FTS_HIGHLIGHT_START}f${FTS_HIGHLIGHT_END}`, 5)).toBe('…cdef')
|
||||
expect(makeSnippet(` x—${FTS_HIGHLIGHT_START}café${FTS_HIGHLIGHT_END}\n y `, 20))
|
||||
.toBe('x—café y')
|
||||
|
||||
@@ -387,6 +387,8 @@ describe('SQLite session search', () => {
|
||||
{ path: '' },
|
||||
{ path: ':memory:', defaultLimit: 0 },
|
||||
{ path: ':memory:', maxLimit: 0 },
|
||||
{ path: ':memory:', defaultLimit: 1e100 },
|
||||
{ path: ':memory:', maxLimit: 1e100 },
|
||||
{ path: ':memory:', snippetChars: 0 },
|
||||
{ path: ':memory:', defaultLimit: 3, maxLimit: 2 },
|
||||
{ path: ':memory:', journalMode: 'memory' },
|
||||
@@ -462,6 +464,39 @@ describe('SQLite reconciliation and source lifecycle', () => {
|
||||
.rejects.toThrow(expectCode('SESSION_QUERY_SESSION_NOT_FOUND'))
|
||||
})
|
||||
|
||||
it('uses the reconciled persistence binding through the query boundary', async () => {
|
||||
const durable = header('post-reconcile-unmount')
|
||||
TestPersistence.reset([{ meta: durable, events: [
|
||||
...messageEvents('durable needle', 1),
|
||||
{ ...messageEvents('durable needle again', 2)[0]!, seq: 1 },
|
||||
] }])
|
||||
const ctx = await liveContext({ path: ':memory:', defaultLimit: 1, maxLimit: 2 })
|
||||
const persistence = await ctx.plugin(TestPersistence)
|
||||
const internals = ctx.sessionSearch as unknown as {
|
||||
_reconcile(signal: AbortSignal | undefined): Promise<{
|
||||
identity: symbol
|
||||
service?: SessionPersistence
|
||||
}>
|
||||
}
|
||||
const reconcile = internals._reconcile.bind(internals)
|
||||
const boundary = vi.spyOn(internals, '_reconcile').mockImplementation(async (signal) => {
|
||||
const binding = await reconcile(signal)
|
||||
await persistence.dispose()
|
||||
return binding
|
||||
})
|
||||
|
||||
const page = await ctx.sessionSearch.searchEvents({
|
||||
sessionId: durable.id,
|
||||
query: 'needle',
|
||||
limit: 1,
|
||||
})
|
||||
expect(page.items).toMatchObject([{ sessionId: durable.id }])
|
||||
expect(page.nextCursor).toEqual(expect.any(String))
|
||||
boundary.mockRestore()
|
||||
await expect(ctx.sessionSearch.searchEvents({ sessionId: durable.id, query: 'needle' }))
|
||||
.rejects.toThrow(expectCode('SESSION_QUERY_SESSION_NOT_FOUND'))
|
||||
})
|
||||
|
||||
it('discards a stale list rejection when persistence unmounts during observation', async () => {
|
||||
const durable = header('racing')
|
||||
TestPersistence.reset([{ meta: durable, events: messageEvents('durable needle') }])
|
||||
@@ -561,6 +596,22 @@ describe('SQLite reconciliation and source lifecycle', () => {
|
||||
expect(TestPersistence.loads.get(added.id)).toBe(1)
|
||||
})
|
||||
|
||||
it('fails after one retry when persistence snapshots keep changing', async () => {
|
||||
const durable = header('continuous-mutation')
|
||||
TestPersistence.reset([{ meta: durable, events: messageEvents('durable needle') }])
|
||||
const ctx = await liveContext()
|
||||
await ctx.plugin(TestPersistence)
|
||||
let lists = 0
|
||||
TestPersistence.snapshotEffect = () => {
|
||||
lists += 1
|
||||
TestPersistence.set({ meta: durable, events: messageEvents(`durable needle ${lists}`) })
|
||||
}
|
||||
|
||||
await expect(ctx.sessionSearch.searchSessions({ query: 'needle' }))
|
||||
.rejects.toThrow(expectCode('SESSION_QUERY_PERSISTENCE_FAILED'))
|
||||
expect(lists).toBe(4)
|
||||
})
|
||||
|
||||
it('retries if the persistence binding changes while live sessions are observed', async () => {
|
||||
const durable = header('live-boundary-retry')
|
||||
TestPersistence.reset([{ meta: durable, events: messageEvents('durable needle') }])
|
||||
|
||||
Reference in New Issue
Block a user