diff --git a/docs/reference/database-dialects.md b/docs/reference/database-dialects.md index cac8e3ff3..11b783cc8 100644 --- a/docs/reference/database-dialects.md +++ b/docs/reference/database-dialects.md @@ -106,11 +106,14 @@ export interface DbClient { unsafe(sql: string, params?: unknown[]): Promise> transaction(fn: (tx: DbClient) => Promise): Promise + close(): Promise readonly dialect: Dialect } ``` +`close()` releases the backing resource: the SQLite file handle plus its WAL and SHM siblings, or the Postgres connection pool. Queries issued after it reject. The long-lived server client never calls it — it exists for callers with a bounded lifetime, chiefly test teardown, which must release the handle before deleting the database file. Windows refuses to unlink an open file, so relying on garbage collection is not portable. On the transaction-scoped client passed to a `.transaction()` callback it is a no-op, since the enclosing client owns the pool. + `DbClient` is callable as a **tagged template**: ```ts diff --git a/server/db/client.ts b/server/db/client.ts index 7837833d4..1bc97a3ef 100644 --- a/server/db/client.ts +++ b/server/db/client.ts @@ -22,6 +22,7 @@ export function placeholder(dialect: Dialect, index: number): string { * Tagged-template callable returning DbResult, plus: * - .unsafe(...) — execute raw SQL strings (e.g. stored migration blocks) * - .transaction(fn) — runs a callback inside a DB transaction + * - .close() — release the underlying handle or connection pool * - .dialect — which SQL dialect the backing database speaks */ export interface DbClient { @@ -31,5 +32,18 @@ export interface DbClient { ): Promise> unsafe>(sql: string, params?: unknown[]): Promise> transaction(fn: (tx: DbClient) => Promise): Promise + /** + * Release the backing resource: the SQLite file handle (and its WAL/SHM + * siblings) or the Postgres connection pool. Queries issued after close + * reject. Long-lived server clients never call this; it exists for callers + * with a bounded lifetime, chiefly test teardown, which must release the + * handle before deleting the database file. Windows refuses to unlink a + * file that is still open, so leaving it to garbage collection is not + * portable. + * + * On a transaction-scoped client (the `tx` passed to `.transaction()`) + * this is a no-op: the enclosing client owns the resource. + */ + close(): Promise readonly dialect: Dialect } diff --git a/server/db/postgres.ts b/server/db/postgres.ts index a2acbb77a..23eefb67e 100644 --- a/server/db/postgres.ts +++ b/server/db/postgres.ts @@ -3,7 +3,7 @@ import type { DbClient, DbResult } from './client' export function createPostgresClient(connectionString: string): DbClient { const sql = new SQL(connectionString) - return wrapSql(sql) + return wrapSql(sql, true) } /** @@ -48,7 +48,16 @@ function resultRowCount(result: Row[]): number { return typeof count === 'number' ? count : result.length } -function wrapSql(sql: SQL): DbClient { +/** + * Wrap a Bun SQL handle in the DbClient interface. + * + * `ownsPool` distinguishes the client returned by `createPostgresClient`, + * which owns the connection pool, from the transaction-scoped client handed + * to a `.transaction()` callback, which borrows it. Only the owner may close + * the pool; closing it mid-transaction would tear the connection out from + * under the surrounding `sql.begin()`. + */ +function wrapSql(sql: SQL, ownsPool = false): DbClient { const fn = (async >( strings: TemplateStringsArray, ...values: unknown[] @@ -71,5 +80,10 @@ function wrapSql(sql: SQL): DbClient { return await sql.begin(async (txSql) => cb(wrapSql(txSql as unknown as SQL))) } + fn.close = async (): Promise => { + if (!ownsPool) return + await sql.close() + } + return Object.assign(fn, { dialect: 'postgres' as const }) } diff --git a/server/db/sqlite.ts b/server/db/sqlite.ts index eb9249dbf..88804cf4a 100644 --- a/server/db/sqlite.ts +++ b/server/db/sqlite.ts @@ -158,5 +158,12 @@ export function createSqliteClient(filename: string): DbClient { return result } + fn.close = async (): Promise => { + // Releases the file handle plus the -wal and -shm siblings created by + // `PRAGMA journal_mode = WAL` above, so the database file can be deleted + // on every platform. + db.close() + } + return Object.assign(fn, { dialect: 'sqlite' as const }) } diff --git a/src/__tests__/db/createDbClient.test.ts b/src/__tests__/db/createDbClient.test.ts index 770a63493..42e528aa8 100644 --- a/src/__tests__/db/createDbClient.test.ts +++ b/src/__tests__/db/createDbClient.test.ts @@ -46,6 +46,9 @@ describe('createDbClient — DATABASE_URL dialect selection', () => { const { rows } = await db<{ count: number }>`select count(*) as count from schema_migrations` expect(rows[0]?.count).toBe(sqliteMigrations.length) + + // Release the handle before withTempDir's finally removes the directory. + await db.close() } }) }) diff --git a/src/__tests__/db/dbClientClose.test.ts b/src/__tests__/db/dbClientClose.test.ts new file mode 100644 index 000000000..bc840c68d --- /dev/null +++ b/src/__tests__/db/dbClientClose.test.ts @@ -0,0 +1,75 @@ +/** + * Cover for DbClient.close(). + * + * Test teardown deletes the temp directory holding a file-backed SQLite + * database. Windows refuses to unlink a file whose handle is still open, so + * every teardown path must release the client first — that is the defect + * behind the Windows suite failures in #284. The EBUSY symptom itself is not + * portable, so these tests assert the invariant instead: after close() the + * handle is released and the file is removable. + */ +import { mkdtemp, mkdir, rm, readdir } from 'node:fs/promises' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { describe, expect, test } from 'bun:test' +import { createSqliteClient } from '../../../server/db/sqlite' +import { createTestDb } from '../helpers/createTestDb' + +async function withTempDir(fn: (dir: string) => Promise): Promise { + const dir = await mkdtemp(join(tmpdir(), 'instatic-db-close-')) + try { + return await fn(dir) + } finally { + await rm(dir, { recursive: true, force: true }) + } +} + +describe('DbClient.close', () => { + test('releases the handle so later queries reject', async () => { + await withTempDir(async (dir) => { + const db = createSqliteClient(join(dir, 'test.db')) + const { rows } = await db<{ one: number }>`select 1 as one` + expect(rows[0]?.one).toBe(1) + + await db.close() + + await expect(db`select 1 as one`).rejects.toThrow() + }) + }) + + test('is safe to call more than once', async () => { + await withTempDir(async (dir) => { + const db = createSqliteClient(join(dir, 'test.db')) + await db`select 1` + await db.close() + await expect(db.close()).resolves.toBeUndefined() + }) + }) + + test('leaves the database file removable, including its WAL siblings', async () => { + await withTempDir(async (dir) => { + // createSqliteClient opens the file directly; only createDbClient + // creates parent directories. + const nested = join(dir, 'nested') + await mkdir(nested, { recursive: true }) + const db = createSqliteClient(join(nested, 'test.db')) + // Force a write so the WAL and SHM siblings actually exist. + await db.unsafe('create table t (id integer primary key)') + await db.unsafe('insert into t (id) values (1)') + expect((await readdir(nested)).length).toBeGreaterThan(0) + + await db.close() + + await expect(rm(nested, { recursive: true, force: true })).resolves.toBeUndefined() + }) + }) + + test('createTestDb cleanup closes the client before removing the directory', async () => { + const { db, cleanup } = await createTestDb() + await db`select 1` + + await cleanup() + + await expect(db`select 1`).rejects.toThrow() + }) +}) diff --git a/src/__tests__/helpers/createTestDb.ts b/src/__tests__/helpers/createTestDb.ts index 53106dbc5..996b5860d 100644 --- a/src/__tests__/helpers/createTestDb.ts +++ b/src/__tests__/helpers/createTestDb.ts @@ -14,9 +14,11 @@ export interface TestDb { * with all migrations applied. Each call produces a unique, independent DB. * * Set `DB=postgres TEST_POSTGRES_URL=postgres://...` to run against a real - * Postgres instance instead. The helper supports that mode at the type level; - * connection-pool teardown is left to process exit until DbClient grows a - * close() method. + * Postgres instance instead. + * + * `cleanup()` closes the client before removing anything on disk. Windows + * refuses to unlink a file that is still open, so a SQLite handle left to + * garbage collection makes teardown fail there with EBUSY. * * @example * const { db, cleanup } = await createTestDb() @@ -35,9 +37,7 @@ export async function createTestDb(): Promise { return { db, cleanup: async () => { - // TODO: extend DbClient with a close() method to properly terminate the - // Postgres connection pool. For now the process-level teardown is enough - // for the opt-in PG test mode. + await db.close() }, } } @@ -51,10 +51,9 @@ export async function createTestDb(): Promise { return { db, cleanup: async () => { - // Remove the entire temp directory. bun:sqlite doesn't expose a close() - // method on our DbClient interface; on macOS/Linux the file can still be - // deleted while the handle is open, and the handle goes out of scope once - // the test function returns. + // Close before unlinking: the file, and its WAL/SHM siblings, stay + // locked on Windows while the handle is open. + await db.close() await fs.rm(path.dirname(tmpFile), { recursive: true, force: true }) }, } diff --git a/src/__tests__/server/dbTestFake.ts b/src/__tests__/server/dbTestFake.ts index 79edcab3a..208cbd332 100644 --- a/src/__tests__/server/dbTestFake.ts +++ b/src/__tests__/server/dbTestFake.ts @@ -30,5 +30,8 @@ export function createFakeDb( }) as DbClient fn.unsafe = async (sql: string, params: unknown[] = []) => handler(sql, params) fn.transaction = async (cb: (tx: DbClient) => Promise): Promise => cb(fn) + // No resource to release; present so fakes satisfy DbClient at runtime, not + // only through the `as DbClient` assertion. + fn.close = async (): Promise => {} return Object.assign(fn, { dialect: 'sqlite' as const }) } diff --git a/src/__tests__/server/importEndpointGuidance.test.ts b/src/__tests__/server/importEndpointGuidance.test.ts index ef851f4b4..7c383aa7f 100644 --- a/src/__tests__/server/importEndpointGuidance.test.ts +++ b/src/__tests__/server/importEndpointGuidance.test.ts @@ -23,7 +23,13 @@ async function setupDb(): Promise<{ db: DbClient; cleanup: () => Promise } const dir = await mkdtemp(join(tmpdir(), 'instatic-import-guide-')) const db = createSqliteClient(join(dir, 'test.db')) await runMigrations(db, sqliteMigrations) - return { db, cleanup: async () => { await rm(dir, { recursive: true, force: true }) } } + return { + db, + cleanup: async () => { + await db.close() + await rm(dir, { recursive: true, force: true }) + }, + } } /** Minimal ZIP local-file-header prefix — enough for magic-byte detection. */ diff --git a/src/__tests__/server/pluginScheduler.test.ts b/src/__tests__/server/pluginScheduler.test.ts index c00d26143..59a387cc8 100644 --- a/src/__tests__/server/pluginScheduler.test.ts +++ b/src/__tests__/server/pluginScheduler.test.ts @@ -122,6 +122,7 @@ async function setupDb(): Promise<{ db: DbClient; cleanup: () => Promise } return { db, cleanup: async () => { + await db.close() await rm(dir, { recursive: true, force: true }) }, } diff --git a/src/__tests__/server/postTypeBuiltInFields.test.ts b/src/__tests__/server/postTypeBuiltInFields.test.ts index 81cd7b9e2..d73101817 100644 --- a/src/__tests__/server/postTypeBuiltInFields.test.ts +++ b/src/__tests__/server/postTypeBuiltInFields.test.ts @@ -34,7 +34,13 @@ async function setupDb(): Promise<{ db: DbClient; cleanup: () => Promise } const dir = await mkdtemp(join(tmpdir(), 'instatic-posttype-')) const db = createSqliteClient(join(dir, 'test.db')) await runMigrations(db, sqliteMigrations) - return { db, cleanup: async () => { await rm(dir, { recursive: true, force: true }) } } + return { + db, + cleanup: async () => { + await db.close() + await rm(dir, { recursive: true, force: true }) + }, + } } describe('createDataTable — post-type built-in fields', () => {