diff --git a/changelog.d/fixes/sqljs-atomic-persist.md b/changelog.d/fixes/sqljs-atomic-persist.md new file mode 100644 index 0000000000..db50495f4d --- /dev/null +++ b/changelog.d/fixes/sqljs-atomic-persist.md @@ -0,0 +1 @@ +- **fix(db):** the sql.js fallback now publishes the database atomically — temp file in the same directory, `fsync`, then `rename()` — instead of rewriting it in place with `writeFileSync`. sql.js has no incremental write path, so every save rewrote the whole image through an `O_TRUNC` open: for the duration of the write the on-disk database was 0 bytes and then partial, a window that scales with database size and recurs on every save. Unlike better-sqlite3 / node:sqlite, that window is not covered by SQLite's locking protocol, so it was visible to every OTHER process reading the same file (a backup job, a metrics exporter, an operator running `sqlite3`), which got `SQLITE_CORRUPT` — "database disk image is malformed" — while `PRAGMA integrity_check` passed moments later. It also closes a total-loss window: a crash mid-write used to leave the real database truncated, and now only leaves a stale temp file diff --git a/src/lib/db/adapters/sqljsAdapter.ts b/src/lib/db/adapters/sqljsAdapter.ts index 42abd2158a..886c52672b 100644 --- a/src/lib/db/adapters/sqljsAdapter.ts +++ b/src/lib/db/adapters/sqljsAdapter.ts @@ -127,10 +127,62 @@ export async function createSqlJsAdapter(filePath: string): Promise | null = null; let _isOpen = true; + /** + * Writes the whole database image out atomically: temp file in the SAME + * directory, fsync, then `rename()` over the destination. + * + * WHY NOT `writeFileSync(filePath, …)` DIRECTLY + * --------------------------------------------- + * sql.js has no incremental write path — every save rewrites the entire image. + * `writeFileSync` opens the destination with `O_TRUNC`, so for the whole + * duration of the write the on-disk database is 0 bytes and then partial. The + * window scales with the database size and recurs on every save, so on a busy + * instance it is open a significant fraction of the time. + * + * Unlike better-sqlite3 / node:sqlite, that window is not protected by SQLite's + * locking protocol, so it is visible to every OTHER process that reads the same + * file — a backup job, a metrics exporter, an operator running `sqlite3`. Those + * readers get `SQLITE_CORRUPT` ("database disk image is malformed") even though + * `PRAGMA integrity_check` passes moments later, which makes the failure look + * random and points the blame at the reader. + * + * `rename()` within a directory is atomic on POSIX and on Windows for a + * same-volume replace, so a reader now sees either the previous image or the + * new one — never a truncated one. It also removes the total-loss window: a + * crash mid-write used to leave the real database truncated, while it now only + * leaves a stale temp file behind. + */ function persist(): void { if (filePath === ":memory:") return; const data = db.export(); - fs.writeFileSync(filePath, Buffer.from(data)); + // Same directory, so `rename` stays within one filesystem — a temp file in + // os.tmpdir() would make it a cross-device copy, which is not atomic. + const tmpPath = `${filePath}.tmp-${process.pid}-${Date.now()}`; + let fd: number | null = null; + try { + fd = fs.openSync(tmpPath, "w"); + fs.writeFileSync(fd, Buffer.from(data)); + // The rename is atomic, but only orders against data that already reached + // the disk; without this an unclean shutdown can publish an empty file. + fs.fsyncSync(fd); + fs.closeSync(fd); + fd = null; + fs.renameSync(tmpPath, filePath); + } catch (err) { + if (fd !== null) { + try { + fs.closeSync(fd); + } catch { + /* already closed */ + } + } + try { + fs.unlinkSync(tmpPath); + } catch { + /* never created, or already gone */ + } + throw err; + } dirty = false; } diff --git a/tests/unit/db-sqljs-atomic-persist.test.ts b/tests/unit/db-sqljs-atomic-persist.test.ts new file mode 100644 index 0000000000..bb61f2a5c3 --- /dev/null +++ b/tests/unit/db-sqljs-atomic-persist.test.ts @@ -0,0 +1,116 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// Regression guard: sql.js has no incremental write path, so every save rewrites +// the whole database image. When that write went through +// `fs.writeFileSync(filePath, …)`, the destination was opened with `O_TRUNC` — +// for the whole duration of the write the on-disk database was 0 bytes and then +// partial. Unlike better-sqlite3 / node:sqlite, that window is NOT covered by +// SQLite's locking protocol, so it was visible to every other process reading the +// same file (backup job, metrics exporter, an operator running `sqlite3`). Those +// readers got SQLITE_CORRUPT — "database disk image is malformed" — while +// `PRAGMA integrity_check` passed moments later, which made the failure look +// random and blamed the reader. The window scales with database size and recurs +// on every save. +// +// The fix writes to a temp file in the same directory and `rename()`s it over the +// destination. The property that distinguishes the two implementations, and the +// one asserted below, is inode identity: `rename` publishes a NEW inode, so a +// reader that already opened the file keeps reading a complete, coherent image, +// whereas `writeFileSync` mutates the inode the reader is holding. +// +// This is deliberately not a timing race — a sleep-based test would be flaky and +// would not prove anything about small databases that get written in one go. + +async function openAdapter(sqliteFile: string) { + const { createSqlJsAdapter } = await import("../../src/lib/db/adapters/sqljsAdapter"); + return createSqlJsAdapter(sqliteFile); +} + +test( + "sql.js persist() publishes the database atomically — a reader holding the file " + + "open never observes a truncated image (rename, not in-place O_TRUNC)", + async () => { + const dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-sqljs-atomic-")); + const sqliteFile = path.join(dataDir, "storage.sqlite"); + let adapter: Awaited> | null = null; + let readerFd: number | null = null; + try { + adapter = await openAdapter(sqliteFile); + adapter.exec("CREATE TABLE t (id INTEGER PRIMARY KEY, v TEXT)"); + adapter.exec("INSERT INTO t (v) VALUES ('first')"); + adapter.checkpoint(); + + assert.ok(fs.existsSync(sqliteFile), "first checkpoint should have written the database"); + const firstBytes = fs.readFileSync(sqliteFile); + const firstInode = fs.statSync(sqliteFile).ino; + + // A concurrent reader that opened the file before the next save. It keeps + // reading through THIS descriptor, exactly like another process mid-read. + readerFd = fs.openSync(sqliteFile, "r"); + + // Grow the image so the second save is unmistakably a different payload. + for (let i = 0; i < 200; i++) { + adapter.exec(`INSERT INTO t (v) VALUES ('row-${i}')`); + } + adapter.checkpoint(); + + // 1. The reader's descriptor still resolves to a COMPLETE image. Under + // writeFileSync it resolves to the same inode that was truncated and + // rewritten, so this read returns the new (or a torn) payload. + const viaReader = Buffer.alloc(firstBytes.length); + const read = fs.readSync(readerFd, viaReader, 0, firstBytes.length, 0); + assert.equal(read, firstBytes.length, "the pre-opened descriptor lost bytes mid-write"); + assert.deepEqual( + viaReader, + firstBytes, + "a reader holding the file open observed the image change underneath it — " + + "persist() replaced the file in place instead of renaming a new one over it" + ); + assert.equal( + viaReader.subarray(0, 15).toString("latin1"), + "SQLite format 3", + "the pre-opened descriptor no longer sees a valid SQLite header" + ); + + // 2. The published file is the NEW image, on a NEW inode — that is what + // makes the swap atomic for everyone who opens it afterwards. + const secondInode = fs.statSync(sqliteFile).ino; + assert.notEqual( + secondInode, + firstInode, + "persist() reused the same inode — the write was not published by rename()" + ); + assert.equal( + fs.readFileSync(sqliteFile).subarray(0, 15).toString("latin1"), + "SQLite format 3", + "the published file is not a valid SQLite image" + ); + + // 3. No temp file survives a successful save. + const leftovers = fs.readdirSync(dataDir).filter((n) => n.startsWith("storage.sqlite.tmp-")); + assert.deepEqual(leftovers, [], "persist() left a temporary file behind"); + } finally { + if (readerFd !== null) fs.closeSync(readerFd); + if (adapter?.open) adapter.close(); + fs.rmSync(dataDir, { recursive: true, force: true }); + } + } +); + +test("sql.js persist() is a no-op for :memory: databases (no temp file, no throw)", async () => { + const dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-sqljs-atomic-mem-")); + let adapter: Awaited> | null = null; + try { + adapter = await openAdapter(":memory:"); + adapter.exec("CREATE TABLE t (id INTEGER PRIMARY KEY)"); + adapter.checkpoint(); + assert.deepEqual(fs.readdirSync(dataDir), [], "an in-memory database wrote to disk"); + } finally { + if (adapter?.open) adapter.close(); + fs.rmSync(dataDir, { recursive: true, force: true }); + } +});