diff --git a/__tests__/unit/persistent-kv.test.js b/__tests__/unit/persistent-kv.test.js index c69ec00..409e414 100644 --- a/__tests__/unit/persistent-kv.test.js +++ b/__tests__/unit/persistent-kv.test.js @@ -1,3 +1,6 @@ +import os from 'node:os'; +import path from 'node:path'; +import fs from 'node:fs'; import { initKVStore } from '../../src/persistent-kv.js'; describe('persistent-kv', () => { @@ -68,4 +71,59 @@ describe('persistent-kv', () => { a.close(); b.close(); }); + + // Bug #5: two file-backed stores against the same dbPath must share one + // underlying connection so concurrent writes don't trip SQLITE_BUSY. + describe('shared file-backed connection (Bug #5)', () => { + let tmpDir; + let dbPath; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'temper-kv-test-')); + dbPath = path.join(tmpDir, 'shared.db'); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('shares one underlying Database across two stores on the same path', () => { + const a = initKVStore(dbPath, 'dedup'); + const b = initKVStore(dbPath, 'rate_limits'); + // ._db is the underlying better-sqlite3 Database; identity check is the + // tightest assertion that a single connection backs both stores. + expect(a._db).toBe(b._db); + a.close(); + b.close(); + }); + + it('keeps the connection alive while at least one store still uses it', () => { + const a = initKVStore(dbPath, 'dedup'); + const b = initKVStore(dbPath, 'rate_limits'); + a.set('hello', 'world'); + a.close(); + // a is closed but b still holds the connection — writes must succeed. + expect(() => b.set('still', 'ok')).not.toThrow(); + expect(b.get('still')).toBe('ok'); + b.close(); + }); + + it('closes the connection when the last store releases it', () => { + const a = initKVStore(dbPath, 'dedup'); + const dbRef = a._db; + a.close(); + // The Database is closed; using a closed handle throws. + expect(() => dbRef.prepare('SELECT 1').get()).toThrow(); + }); + + it('reopens cleanly after all stores have closed', () => { + const a = initKVStore(dbPath, 'dedup'); + a.set('x', '1'); + a.close(); + + const reopened = initKVStore(dbPath, 'dedup'); + expect(reopened.get('x')).toBe('1'); + reopened.close(); + }); + }); }); diff --git a/src/persistent-kv.js b/src/persistent-kv.js index 86b505d..a1fc043 100644 --- a/src/persistent-kv.js +++ b/src/persistent-kv.js @@ -9,11 +9,50 @@ import Database from 'better-sqlite3'; +// Bug #5: callers (app.js) use the same SQLite file for two logical +// stores ('webhook_dedup' and 'ai_rate_limits' both live in dedup.db). +// Opening two `Database` connections against the same file works for +// reads under WAL but produces SQLITE_BUSY on concurrent writes — and +// `markProcessed` + `recordReview` race regularly. Cache by absolute +// dbPath so the second call against the same file shares the same +// underlying connection. `:memory:` stays per-call: each `:memory:` +// open is its own DB by SQLite design, so caching it would produce +// surprising cross-store sharing in tests. +const _connections = new Map(); // dbPath → { db, refs } + +function acquireConnection(dbPath) { + if (dbPath === ':memory:') { + const db = new Database(':memory:'); + db.pragma('journal_mode = WAL'); + return { db, release: () => db.close() }; + } + let entry = _connections.get(dbPath); + if (!entry) { + const db = new Database(dbPath); + db.pragma('journal_mode = WAL'); + entry = { db, refs: 0 }; + _connections.set(dbPath, entry); + } + entry.refs += 1; + return { + db: entry.db, + release: () => { + entry.refs -= 1; + if (entry.refs <= 0) { + _connections.delete(dbPath); + entry.db.close(); + } + } + }; +} + /** * Initialise (or attach to) a SQLite-backed KV table. Tables are namespaced - * by `name` so multiple stores can share one database file. + * by `name` so multiple stores can share one database file. Multiple stores + * against the same file path share one underlying SQLite connection (see + * Bug #5 — concurrent writes from two connections produce SQLITE_BUSY). * - * @param {string} dbPath - SQLite file path + * @param {string} dbPath - SQLite file path (or ':memory:' for ephemeral) * @param {string} name - logical table name (alphanumeric / underscores) * @param {object} [opts] * @param {number} [opts.ttlMs] - default TTL applied when not provided per-write. @@ -26,8 +65,7 @@ export function initKVStore(dbPath, name, opts = {}) { } const ttlMs = opts.ttlMs; - const db = new Database(dbPath); - db.pragma('journal_mode = WAL'); + const { db, release } = acquireConnection(dbPath); db.exec(` CREATE TABLE IF NOT EXISTS kv_${name} ( key TEXT PRIMARY KEY, @@ -74,7 +112,7 @@ export function initKVStore(dbPath, name, opts = {}) { } function close() { - db.close(); + release(); } return { has, get, set, sweep, close, _db: db };