From bd16737dd87c9339814b67cedfa4bded75b05058 Mon Sep 17 00:00:00 2001 From: Josh Wright Date: Fri, 9 Oct 2026 17:05:06 -0400 Subject: [PATCH] Remove shared prepared statement cache (Refs #76) --- src/lib/server/db/index.ts | 22 +++---- tests/unit/db-statements.test.ts | 61 +++++++++++++++++-- tests/unit/host-observation-retention.test.ts | 12 ++-- 3 files changed, 68 insertions(+), 27 deletions(-) diff --git a/src/lib/server/db/index.ts b/src/lib/server/db/index.ts index ff948a5..1eb9b10 100644 --- a/src/lib/server/db/index.ts +++ b/src/lib/server/db/index.ts @@ -645,21 +645,15 @@ export function db(): Database.Database { throw err; } - // Cache only runtime queries, after migrations have finished. Statements - // belong to this connection, so reopening the DB starts with an empty cache. - // Callers share statement options (e.g. raw()); use consistent options for - // identical SQL and pass values to run/get/all rather than binding permanently. - const statements = new Map(); + // Instrument runtime queries after migrations. Each prepare must return a + // fresh statement: modes, permanent bindings, and active iterators are mutable + // state owned by the caller, even when another caller uses identical SQL. const slowMs = Number(process.env.EC_SLOW_SQL_MS ?? 0); - const prepare = handle.prepare.bind(handle); - handle.prepare = ((sql: string) => { - let statement = statements.get(sql); - if (!statement) { - statement = timeStatement(prepare(sql), sql, slowMs); - statements.set(sql, statement); - } - return statement; - }) as typeof handle.prepare; + if (Number.isFinite(slowMs) && slowMs > 0) { + const prepare = handle.prepare.bind(handle); + handle.prepare = ((sql: string) => + timeStatement(prepare(sql), sql, slowMs)) as typeof handle.prepare; + } _db = handle; optimizeTimer = setInterval(() => { diff --git a/tests/unit/db-statements.test.ts b/tests/unit/db-statements.test.ts index 98d1d49..1fc4028 100644 --- a/tests/unit/db-statements.test.ts +++ b/tests/unit/db-statements.test.ts @@ -8,9 +8,10 @@ beforeEach(async () => { }); afterEach(async () => { await fixture.cleanup(); + vi.unstubAllEnvs(); }); -describe('prepared statement caching', () => { +describe('database statements', () => { it('tunes the controller connection and stops periodic optimization when closed', async () => { const { db, closeDb } = await import('../../src/lib/server/db/index.js'); vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval'] }); @@ -30,7 +31,7 @@ describe('prepared statement caching', () => { vi.useRealTimers(); } }); - it('reuses statements with fresh parameters and remains usable after rollback', async () => { + it('keeps caller-owned statements reusable with fresh parameters after rollback', async () => { const { db } = await import('../../src/lib/server/db/index.js'); const insertSql = 'INSERT INTO settings (key, value) VALUES (?, ?)'; const selectSql = 'SELECT value FROM settings WHERE key = ?'; @@ -45,8 +46,8 @@ describe('prepared statement caching', () => { ).toThrow('rollback'); db().prepare(insertSql).run('cache-second', 'two'); - expect(db().prepare(insertSql)).toBe(insert); - expect(db().prepare(selectSql)).toBe(select); + expect(db().prepare(insertSql)).not.toBe(insert); + expect(db().prepare(selectSql)).not.toBe(select); expect(select.get('cache-first')).toEqual({ value: 'one' }); expect(select.get('cache-second')).toEqual({ value: 'two' }); expect(select.get('cache-rollback')).toBeUndefined(); @@ -65,6 +66,56 @@ describe('prepared statement caching', () => { expect(statement).not.toBe(oldStatement); expect(statement.database).not.toBe(oldStatement.database); expect(statement.get('cache-persisted')).toEqual({ value: 'saved' }); - expect(db().prepare(sql)).toBe(statement); + expect(db().prepare(sql)).not.toBe(statement); + }); + + describe.each([0, 20])('statement isolation with EC_SLOW_SQL_MS=%i', (slowMs) => { + beforeEach(() => vi.stubEnv('EC_SLOW_SQL_MS', String(slowMs))); + + it.each([ + ['raw', [1]], + ['pluck', 1], + ['expand', { $: { value: 1 } }], + ['safeIntegers', { value: 1n }] + ] as const)('keeps %s mode local to its statement', async (mode, expected) => { + const { db } = await import('../../src/lib/server/db/index.js'); + const sql = 'SELECT 1 AS value'; + const customized = db().prepare(sql)[mode](); + const standard = db().prepare(sql); + + expect(standard.get()).toEqual({ value: 1 }); + expect(customized.get()).toEqual(expected); + customized[mode](false); + standard[mode](); + expect(customized.get()).toEqual({ value: 1 }); + expect(standard.get()).toEqual(expected); + expect(db().prepare(sql).get()).toEqual({ value: 1 }); + }); + + it('keeps permanent bindings local to their statement', async () => { + const { db } = await import('../../src/lib/server/db/index.js'); + const sql = 'SELECT ? AS value'; + const bound = db().prepare(sql).bind('first'); + + expect(db().prepare(sql).get('second')).toEqual({ value: 'second' }); + expect(bound.get()).toEqual({ value: 'first' }); + expect(db().prepare(sql).bind('third').get()).toEqual({ value: 'third' }); + expect(bound.get()).toEqual({ value: 'first' }); + }); + + it('allows identical queries while another statement is being iterated', async () => { + const { db } = await import('../../src/lib/server/db/index.js'); + const sql = 'SELECT 1 AS value UNION ALL SELECT 2 AS value'; + const iterator = db().prepare(sql).iterate(); + try { + expect(iterator.next()).toEqual({ value: { value: 1 }, done: false }); + expect(db().prepare(sql).get()).toEqual({ value: 1 }); + expect([...db().prepare(sql).iterate()]).toEqual([{ value: 1 }, { value: 2 }]); + expect(iterator.next()).toEqual({ value: { value: 2 }, done: false }); + expect(iterator.next().done).toBe(true); + } finally { + iterator.return?.(); + } + }); }); }); diff --git a/tests/unit/host-observation-retention.test.ts b/tests/unit/host-observation-retention.test.ts index d740ade..00fdcfe 100644 --- a/tests/unit/host-observation-retention.test.ts +++ b/tests/unit/host-observation-retention.test.ts @@ -328,9 +328,9 @@ describe('routine host observation retention', () => { ).toEqual({ count: 2000 }); }); - it('reuses the same three delete statements across different batch sizes', async () => { + it('uses the same three delete query shapes across different batch sizes', async () => { const api = await setup(); - const statements = new Map>(); + const deleteQueries = new Set(); for (const count of [1, 2, 7, 50, 500]) { api.handle.transaction(() => { const insert = api.handle.prepare(`INSERT INTO ec_host_operations @@ -346,13 +346,9 @@ describe('routine host observation retention', () => { .filter((sql) => sql.startsWith('DELETE FROM ec_host_')); prepare.mockRestore(); expect(deletes).toHaveLength(3); - for (const sql of deletes) { - const statement = api.handle.prepare(sql); - if (statements.has(sql)) expect(statement).toBe(statements.get(sql)); - statements.set(sql, statement); - } + for (const sql of deletes) deleteQueries.add(sql); } - expect(statements.size).toBe(3); + expect(deleteQueries.size).toBe(3); expect(api.handle.pragma('foreign_key_check')).toEqual([]); });