diff --git a/changelog.d/fixes/9541-fastpath-db-corruption.md b/changelog.d/fixes/9541-fastpath-db-corruption.md new file mode 100644 index 0000000000..b15f04fa65 --- /dev/null +++ b/changelog.d/fixes/9541-fastpath-db-corruption.md @@ -0,0 +1 @@ +- fix(db): add transient-error retry to corruption probe to prevent data loss under concurrent load (#9541) diff --git a/src/lib/db/core.ts b/src/lib/db/core.ts index 76731147d9..50d874a3ba 100644 --- a/src/lib/db/core.ts +++ b/src/lib/db/core.ts @@ -13,6 +13,7 @@ import { openDatabaseAsync, } from "./adapters/driverFactory"; import path from "path"; +import { retryProbeIfTransient } from "./probeUtils"; import fs from "fs"; import { resolveWritableDataDir, getLegacyDotDataDir } from "../dataPaths"; import { runMigrations } from "./migrationRunner"; @@ -1142,18 +1143,19 @@ export function getDbInstance(): SqliteDatabase { `Original error: ${message}` ); } - preservedCriticalState = captureCriticalDbState(sqliteFile); - - // SAFETY: Never delete the database — rename to backup so data can be recovered. - // The old code would silently destroy all user data on any probe failure. - const failedPath = sqliteFile + `.probe-failed-${Date.now()}`; - try { - fs.renameSync(sqliteFile, failedPath); - console.warn(`[DB] Renamed corrupt DB to ${path.basename(failedPath)}`); - failedProbePath = failedPath; - failedProbeMessage = message; - } catch { - /* ok */ + if (!retryProbeIfTransient(sqliteFile, e, openSqliteDatabase, closeProbeIfSafe)) { + preservedCriticalState = captureCriticalDbState(sqliteFile); + // SAFETY: Never delete the database — rename to backup so data can be recovered. + // The old code would silently destroy all user data on any probe failure. + const failedPath = sqliteFile + `.probe-failed-${Date.now()}`; + try { + fs.renameSync(sqliteFile, failedPath); + console.warn(`[DB] Renamed corrupt DB to ${path.basename(failedPath)}`); + failedProbePath = failedPath; + failedProbeMessage = message; + } catch { + /* ok */ + } } } } diff --git a/src/lib/db/probeUtils.ts b/src/lib/db/probeUtils.ts new file mode 100644 index 0000000000..e5f2bd3885 --- /dev/null +++ b/src/lib/db/probeUtils.ts @@ -0,0 +1,96 @@ +/** + * Probe-retry utilities for the SQLite corruption-probe path in getDbInstance(). + * + * Transient probe errors (SQLITE_BUSY, ENOENT, SQLITE_PROTOCOL, SQLITE_IOERR) + * should be retried with backoff instead of immediately renaming the DB away + * and creating an empty one (data loss under concurrent load, #9541). + */ +import fs from "node:fs"; +import path from "node:path"; + +/** + * Identifies transient SQLite/OS probe errors that should be retried instead of + * triggering the corruption-rename path. + * + * Transient errors are conditions that can self-resolve within milliseconds: + * - SQLITE_BUSY: database is locked by another connection + * - SQLITE_PROTOCOL: locking protocol violation + * - SQLITE_IOERR: disk I/O error (can be transient under load) + * - ENOENT: file disappeared (race with another process/worker deleting it) + * + * Fatal errors (native load failures, OOM, module-not-found) are NOT transient. + */ +export function isTransientProbeError(error: unknown): boolean { + const message = error instanceof Error ? error.message : String(error); + return /SQLITE_BUSY|SQLITE_PROTOCOL|SQLITE_IOERR|ENOENT/i.test(message); +} + +/** + * Synchronous sleep that blocks the event loop for `ms` milliseconds. + * Only used in the transient-probe-error retry path where we are already in + * a synchronous context (better-sqlite3). Uses `Atomics.wait` which yields to + * the OS scheduler during the wait, falling back to a busy-wait on runtimes + * where Atomics.wait is restricted. + */ +function syncSleep(ms: number): void { + if (typeof SharedArrayBuffer !== "undefined" && typeof Atomics !== "undefined") { + try { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); + return; + } catch { + // Atomics.wait may throw on restricted runtimes — fall through to busy-wait + } + } + const deadline = Date.now() + ms; + while (Date.now() < deadline) { + /* busy-wait */ + } +} + +/** + * Type for openSqliteDatabase callback — avoids importing the full SQLite adapter type. + */ +type OpenDbFn = ( + filePath: string, + options?: Record +) => { + driver: string; + open: boolean; + close(): void; +}; + +/** + * Retries opening a SQLite database probe when the initial attempt fails with + * a transient error. Uses exponential backoff (500ms, 1000ms, 2000ms). + * + * @param sqliteFile - Path to the SQLite database file + * @param openDb - Function to open the database (normally openSqliteDatabase) + * @param closeDb - Function to safely close the probe adapter + * @returns true if the retry succeeded (transient condition resolved) + * false if all retries were exhausted or error is non-transient + */ +export function retryProbeIfTransient( + sqliteFile: string, + probeError: unknown, + openDb: OpenDbFn, + closeDb: (adapter: { driver: string; open: boolean; close(): void } | null | undefined) => void +): boolean { + if (!isTransientProbeError(probeError)) return false; + + const retryDelays = [500, 1000, 2000]; + for (let i = 0; i < retryDelays.length; i++) { + syncSleep(retryDelays[i]); + try { + const retryAdapter = openDb(sqliteFile, { readonly: true }); + closeDb(retryAdapter); + return true; + } catch { + // Retry failed, try next delay + } + } + + console.warn( + `[DB] All ${retryDelays.length} transient probe retries exhausted — declaring corruption` + ); + return false; +} diff --git a/tests/unit/authz/probe-9033-repro.test.ts b/tests/unit/authz/probe-9033-repro.test.ts index 6044de755e..29e44ebde8 100644 --- a/tests/unit/authz/probe-9033-repro.test.ts +++ b/tests/unit/authz/probe-9033-repro.test.ts @@ -10,8 +10,14 @@ import os from "node:os"; import path from "node:path"; import { NextRequest } from "next/server"; -const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-9033-repro-")); -process.env.DATA_DIR = TEST_DATA_DIR; +const TEST_DATA_DIR = path.join(process.env.DATA_DIR!, "probe-9033-repro"); +// NOTE: Not reassigning process.env.DATA_DIR at module scope because +// node --test spawns test files as worker threads sharing process.env. +// A module-level DATA_DIR override would leak to ALL concurrently running +// workers, causing them to share the same SQLite file and race on it (#9541). +// isolateDataDir.ts (--import) already set DATA_DIR to a unique temp dir per +// process; we use a subdirectory within it instead. + process.env.JWT_SECRET = "test-secret-9033"; const core = await import("../../../src/lib/db/core.ts"); diff --git a/tests/unit/probe-9541-repro.test.ts b/tests/unit/probe-9541-repro.test.ts new file mode 100644 index 0000000000..1faab99844 --- /dev/null +++ b/tests/unit/probe-9541-repro.test.ts @@ -0,0 +1,135 @@ +// TDD verification for #9541 — DB corruption probe transient-error retry. +// +// RED: The repro confirms transient errors (BUSY, ENOENT, PROTOCOL, IOERR) +// fall through to the corruption-rename path (data loss confirmed). +// GREEN: After the fix, isTransientProbeError() exists in core.ts and correctly +// classifies transient vs fatal errors, and a retry loop prevents immediate +// corruption declaration. +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"; + +// Import the fix function from probeUtils.ts +const probeUtils = await import("../../src/lib/db/probeUtils.ts"); + +// ── Tests from the original probe that confirmed the bug ── + +test("FIX-GREEN: isTransientProbeError is exported and classifies BUSY", () => { + const busy = new Error("SQLITE_BUSY: database is locked"); + + // The fix must exist + assert.equal( + typeof probeUtils.isTransientProbeError, + "function", + "isTransientProbeError must be exported from core.ts" + ); + + assert.equal(probeUtils.isTransientProbeError(busy), true, "BUSY is transient"); +}); + +test("FIX-GREEN: isTransientProbeError does NOT classify fatal errors", () => { + const fatalPatterns = [ + "out of memory", + "allocation failure", + "Array buffer allocation failed", + "could not be found", + "Module did not self-register", + ]; + + for (const msg of fatalPatterns) { + assert.equal( + probeUtils.isTransientProbeError(new Error(msg)), + false, + `fatal should NOT be transient: ${msg}` + ); + } +}); + +test("FIX-GREEN: isTransientProbeError classifies BUSY/PROTOCOL/IOERR/ENOENT", () => { + const transientPatterns = [ + "SQLITE_BUSY: database is locked", + "SQLITE_PROTOCOL: locking protocol", + "SQLITE_IOERR: disk I/O error", + "ENOENT: no such file or directory, open '/tmp/db.sqlite'", + ]; + + for (const msg of transientPatterns) { + assert.equal(probeUtils.isTransientProbeError(new Error(msg)), true, `transient: ${msg}`); + } +}); + +test("FIX-GREEN: isTransientProbeError handles non-Error input gracefully", () => { + assert.equal(probeUtils.isTransientProbeError("SQLITE_BUSY"), true, "string error works"); + assert.equal( + probeUtils.isTransientProbeError("random string"), + false, + "non-matching string returns false" + ); + assert.equal(probeUtils.isTransientProbeError(null), false, "null returns false"); + assert.equal(probeUtils.isTransientProbeError(undefined), false, "undefined returns false"); + assert.equal(probeUtils.isTransientProbeError({}), false, "object without message returns false"); +}); + +test("BUG-CONFIRMED (regression guard): probe failure renames DB and loses persisted config", () => { + // This test confirms the SCENARIO we're preventing — if the probe path is + // reached (all transient retries exhausted or non-transient), data IS lost. + // This is the EXISTING behavior on non-transient errors; the fix only + // ADDED a retry window for transient errors before this path. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-9541-data-loss-")); + const sqliteFile = path.join(dir, "storage.sqlite"); + + try { + const header = Buffer.alloc(100); + header.write("SQLite format 3\0"); + fs.writeFileSync(sqliteFile, header); + fs.writeFileSync(sqliteFile, "DATA_MARKER_PERSISTED_CONFIG", { flag: "a" }); + + const beforeContent = fs.readFileSync(sqliteFile, "utf-8"); + assert.ok( + beforeContent.includes("DATA_MARKER_PERSISTED_CONFIG"), + "data must be present before probe failure" + ); + + // Simulate probe failure: rename + create new empty DB + const failedPath = sqliteFile + `.probe-failed-${Date.now()}`; + fs.renameSync(sqliteFile, failedPath); + const newHeader = Buffer.alloc(100); + newHeader.write("SQLite format 3\0"); + fs.writeFileSync(sqliteFile, newHeader); + + const afterContent = fs.readFileSync(sqliteFile, "utf-8"); + assert.equal( + afterContent.includes("DATA_MARKER_PERSISTED_CONFIG"), + false, + "data MUST be lost when DB is renamed and recreated (corruption path behavior)" + ); + } finally { + try { + fs.rmSync(dir, { recursive: true, force: true }); + } catch { + /* ok */ + } + } +}); + +test("FIX-GREEN: DATA_DIR no longer overridden at module scope in probe-9033-repro", async () => { + // Verify the fix in probe-9033-repro.test.ts no longer sets process.env.DATA_DIR + // at module scope. Read-only accesses to process.env.DATA_DIR are fine. + const reproTestSource = fs.readFileSync( + new URL("../../tests/unit/authz/probe-9033-repro.test.ts", import.meta.url), + "utf-8" + ); + + // Find lines that ASSIGN to process.env.DATA_DIR (not just read it) + const assignLines = reproTestSource + .split("\n") + .filter((line) => /process\.env\.DATA_DIR\s*=/.test(line) && !line.trim().startsWith("//")); + + assert.equal( + assignLines.length, + 0, + `probe-9033-repro must not assign process.env.DATA_DIR at module scope. Found: ${assignLines.map((l) => l.trim()).join(", ")}` + ); +});