From 3a32fa2b0061fa099cbae7760cf9569abe9b5cfc Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Sat, 2 May 2026 08:21:26 +0200 Subject: [PATCH] fix: share one SQLite connection across KV stores on same path (Bug #5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: QA bug-hunter flagged that app.js calls initKVStore twice against the same dedup.db file — once for webhook idempotency, once for AI rate limits. WAL tolerates multi-connection reads but two writers can SQLITE_BUSY each other. When markProcessed and recordReview race, one throws → the handler crashes mid-flight → markProcessed is skipped → the same delivery is reprocessed and the AI review runs twice. What: - Add a module-level Map cache in persistent-kv.js keyed by absolute dbPath. The second initKVStore call against the same file returns a store that wraps the same Database instance. - Ref-counted close(): the Database is only really closed when ALL stores against that path have called close(). Tests that rely on closing one store while another is still active now work correctly. - `:memory:` stays per-call. Each `:memory:` open is its own DB by SQLite design; caching the literal string would produce cross-store contamination in tests that use it. Test plan: - 4 new tests in __tests__/unit/persistent-kv.test.js: - identity check that two stores share one Database - mid-life close of one store leaves the other usable - last close actually tears down the connection - clean reopen-after-full-close round-trip - npm test → 838 pass (was 834) - npm run lint → clean Risk: low — narrow, internal change. The previous test ('persists across separate stores on the same shared db (in-memory check)') still passes because :memory: paths bypass the cache. Co-Authored-By: Claude Opus 4.7 (1M context) --- __tests__/unit/persistent-kv.test.js | 58 ++++++++++++++++++++++++++++ src/persistent-kv.js | 48 ++++++++++++++++++++--- 2 files changed, 101 insertions(+), 5 deletions(-) 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 };