fix(db): add transient-error retry to corruption probe to prevent data loss under concurrent load (#9541)

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)

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 */
+        }
       }
     }
   }

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<string, unknown>
+) => {
+  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;
+}

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");

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(", ")}`
+  );
+});