From 0698e16bc58c71c24f42bca34804cd085edbb8df Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Mon, 6 Jul 2026 00:45:28 -0600 Subject: [PATCH] test: add direct unit coverage for closeDbPair/closeDbPairDeferred/closeDbDeferred MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closeDbPair, closeDbPairDeferred, and closeDbDeferred in src/db/connection.ts had zero direct test coverage despite being core resource-lifecycle primitives in a fanIn-55 file — the same category of gap where the openReadonlyWithNative leak went undetected (see openReadonlyWithNative-leak.test.ts). Adds three describe blocks to tests/unit/db.test.ts, alongside the existing openDb/closeDb coverage: - closeDbPair: native handle closes before the better-sqlite3 handle; a native close failure doesn't prevent the better-sqlite3 close; works with no native handle present. - closeDbPairDeferred: native closes synchronously within the call, while the better-sqlite3 close is deferred via closeDbDeferred (including when the native close throws). - closeDbDeferred: the advisory lock releases synchronously (verified via a real lock file), the handle itself closes on the next tick, and flushDeferredClose() closes it synchronously when called first — with the originally scheduled callback correctly skipping a second close. Fixes #1751 --- tests/unit/db.test.ts | 180 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 179 insertions(+), 1 deletion(-) diff --git a/tests/unit/db.test.ts b/tests/unit/db.test.ts index 65628e2ea..abd5895f7 100644 --- a/tests/unit/db.test.ts +++ b/tests/unit/db.test.ts @@ -9,7 +9,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import Database from 'better-sqlite3'; -import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; const execFileSyncSpy = vi.hoisted(() => vi.fn()); @@ -20,10 +20,16 @@ vi.mock('node:child_process', async (importOriginal) => { }); import { _resetRepoRootCache } from '../../src/db/connection.js'; +import type { LockedDatabase } from '../../src/db/index.js'; import { + acquireAdvisoryLock, closeDb, + closeDbDeferred, + closeDbPair, + closeDbPairDeferred, findDbPath, findRepoRoot, + flushDeferredClose, getBuildMeta, initSchema, MIGRATIONS, @@ -31,6 +37,7 @@ import { openReadonlyOrFail, setBuildMeta, } from '../../src/db/index.js'; +import type { NativeDatabase } from '../../src/types.js'; let tmpDir: string; @@ -126,6 +133,177 @@ describe('openDb', () => { }); }); +// ── closeDbPair / closeDbPairDeferred / closeDbDeferred (#1751) ──────────── +// These pair-close helpers only ever call `.close()` on whatever db/nativeDb +// handles they're given — they never open a DB themselves — so lightweight +// mock handles are enough to exercise close ordering and deferral timing. +// (Contrast with openReadonlyWithNative-leak.test.ts, which needs a real +// handle because that regression is about *opening*, not closing.) + +/** Resolves once the event loop has drained the `setImmediate` queue, i.e. + * after any close scheduled via `closeDbDeferred`'s own `setImmediate`. */ +function tick(): Promise { + return new Promise((resolve) => setImmediate(resolve)); +} + +function makeFakeDb(lockPath?: string): { + db: LockedDatabase; + close: ReturnType; +} { + const close = vi.fn(); + const db = { close, __lockPath: lockPath } as unknown as LockedDatabase; + return { db, close }; +} + +function makeFakeNativeDb(): { nativeDb: NativeDatabase; close: ReturnType } { + const close = vi.fn(); + const nativeDb = { close } as unknown as NativeDatabase; + return { nativeDb, close }; +} + +describe('closeDbPair', () => { + it('closes the native handle before the better-sqlite3 handle', () => { + const order: string[] = []; + const { db, close: dbClose } = makeFakeDb(); + const { nativeDb, close: nativeClose } = makeFakeNativeDb(); + dbClose.mockImplementation(() => order.push('db')); + nativeClose.mockImplementation(() => order.push('native')); + + closeDbPair({ db, nativeDb }); + + expect(order).toEqual(['native', 'db']); + }); + + it('still closes the better-sqlite3 handle when the native close throws', () => { + const { db, close: dbClose } = makeFakeDb(); + const { nativeDb, close: nativeClose } = makeFakeNativeDb(); + nativeClose.mockImplementation(() => { + throw new Error('native close boom'); + }); + + expect(() => closeDbPair({ db, nativeDb })).not.toThrow(); + + expect(nativeClose).toHaveBeenCalledTimes(1); + expect(dbClose).toHaveBeenCalledTimes(1); + }); + + it('closes the better-sqlite3 handle when there is no native handle', () => { + const { db, close: dbClose } = makeFakeDb(); + + closeDbPair({ db }); + + expect(dbClose).toHaveBeenCalledTimes(1); + }); +}); + +describe('closeDbPairDeferred', () => { + // Drain anything left in connection.ts's module-private deferred-close + // queue so state can't leak into a later test. + afterEach(async () => { + flushDeferredClose(); + await tick(); + }); + + it('closes the native handle immediately, synchronously within the call', () => { + const { db } = makeFakeDb(); + const { nativeDb, close: nativeClose } = makeFakeNativeDb(); + + closeDbPairDeferred({ db, nativeDb }); + + expect(nativeClose).toHaveBeenCalledTimes(1); + }); + + it('defers the better-sqlite3 close to the next tick instead of closing synchronously', async () => { + const { db, close: dbClose } = makeFakeDb(); + const { nativeDb } = makeFakeNativeDb(); + + closeDbPairDeferred({ db, nativeDb }); + expect(dbClose).not.toHaveBeenCalled(); + + await tick(); + + expect(dbClose).toHaveBeenCalledTimes(1); + }); + + it('still defers the better-sqlite3 close when the native close throws', async () => { + const { db, close: dbClose } = makeFakeDb(); + const { nativeDb, close: nativeClose } = makeFakeNativeDb(); + nativeClose.mockImplementation(() => { + throw new Error('native close boom'); + }); + + expect(() => closeDbPairDeferred({ db, nativeDb })).not.toThrow(); + expect(dbClose).not.toHaveBeenCalled(); + + await tick(); + + expect(dbClose).toHaveBeenCalledTimes(1); + }); +}); + +describe('closeDbDeferred', () => { + afterEach(async () => { + flushDeferredClose(); + await tick(); + }); + + it('releases the advisory lock synchronously while deferring the actual handle close', async () => { + const dbPath = path.join(tmpDir, 'deferred-close-lock.db'); + acquireAdvisoryLock(dbPath); + const lockPath = `${dbPath}.lock`; + expect(fs.existsSync(lockPath)).toBe(true); + + const { db, close } = makeFakeDb(lockPath); + + closeDbDeferred(db); + + // Lock release + clearing __lockPath happen synchronously, within the call. + expect(fs.existsSync(lockPath)).toBe(false); + expect(db.__lockPath).toBeUndefined(); + // The handle itself is not closed yet — that's deferred to the next tick. + expect(close).not.toHaveBeenCalled(); + + await tick(); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('closes the handle on the next tick when nothing flushes it early', async () => { + const { db, close } = makeFakeDb(); + + closeDbDeferred(db); + expect(close).not.toHaveBeenCalled(); + + await tick(); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('flushDeferredClose() closes the handle synchronously when called before the next tick', async () => { + const { db, close } = makeFakeDb(); + + closeDbDeferred(db); + expect(close).not.toHaveBeenCalled(); + + flushDeferredClose(); + expect(close).toHaveBeenCalledTimes(1); + + // The setImmediate callback scheduled by closeDbDeferred must notice the + // handle was already removed + closed by the flush, and must not close + // it a second time. + await tick(); + + expect(close).toHaveBeenCalledTimes(1); + }); + + it('does not attempt a lock release when the handle has no advisory lock path', () => { + const { db } = makeFakeDb(); + + expect(() => closeDbDeferred(db)).not.toThrow(); + expect(db.__lockPath).toBeUndefined(); + }); +}); + describe('findDbPath', () => { it('returns resolved custom path when provided', () => { const custom = path.join(tmpDir, 'custom.db');