From 7d69b02a293f266e1e3e29dc4b60aabb5200b199 Mon Sep 17 00:00:00 2001 From: Aaron Scherer <896295+cryptiklemur@users.noreply.github.com> Date: Thu, 17 Sep 2026 16:52:43 -0500 Subject: [PATCH] fix(db): skip integrity scans during health polling (#13149) * fix(db): skip integrity scans during health polling * test(db): update health error fixture for scan-free polling * docs(changelog): add fragment for health poll integrity skip * fix(db): keep the #13149 dashboard skip inside the #13717 managed health check Merge fallout only: runManagedDbHealthCheck moved behind the health coordinator on the release tip, so the per-call skipIntegrityCheck now travels through it. A waived integrity scan is part of the job identity, so it is never replayed from the 60s diagnosis cache to a caller that asked for the full scan. Co-authored-by: cryptiklemur --------- Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Co-authored-by: cryptiklemur --- .../fixes/13149-health-poll-skip-integrity.md | 1 + src/app/api/db/health/route.ts | 4 +- src/lib/db/core.ts | 10 +- src/lib/db/healthCheckRunner.ts | 28 +++-- .../unit/db-health-poll-nonblocking.test.mjs | 103 ++++++++++++++++++ .../rule12-error-sanitization-sweep.test.ts | 16 +-- 6 files changed, 141 insertions(+), 21 deletions(-) create mode 100644 changelog.d/fixes/13149-health-poll-skip-integrity.md create mode 100644 tests/unit/db-health-poll-nonblocking.test.mjs diff --git a/changelog.d/fixes/13149-health-poll-skip-integrity.md b/changelog.d/fixes/13149-health-poll-skip-integrity.md new file mode 100644 index 0000000000..c19ab5eb31 --- /dev/null +++ b/changelog.d/fixes/13149-health-poll-skip-integrity.md @@ -0,0 +1 @@ +- **fix(db):** authenticated `GET /api/db/health` polls no longer run a SQLite `quick_check`. The health dashboard polls every 15 seconds, and that scan ran synchronously on the request-serving event loop, blocking it for the length of the scan. Reference and state checks still run, and explicit repair requests keep integrity checks unless `OMNIROUTE_SKIP_DB_HEALTHCHECK=1` is set ([#13149](https://github.com/diegosouzapw/OmniRoute/pull/13149)) diff --git a/src/app/api/db/health/route.ts b/src/app/api/db/health/route.ts index 44bffb758c..d20f104310 100644 --- a/src/app/api/db/health/route.ts +++ b/src/app/api/db/health/route.ts @@ -9,7 +9,9 @@ export async function GET(request: Request) { } try { - return NextResponse.json(await runManagedDbHealthCheck({ autoRepair: false })); + return NextResponse.json( + await runManagedDbHealthCheck({ autoRepair: false, skipIntegrityCheck: true }) + ); } catch (error) { const message = error instanceof Error ? error.message : String(error); console.error("[API] DB health diagnosis failed:", message); diff --git a/src/lib/db/core.ts b/src/lib/db/core.ts index 910883c30d..16047035bb 100644 --- a/src/lib/db/core.ts +++ b/src/lib/db/core.ts @@ -943,9 +943,9 @@ function startDbHealthCheckScheduler(db: SqliteDatabase) { // The scheduler lives in ./walMaintenance (periodic TRUNCATE + busy warn + PASSIVE retry). const healthShutdown = new AbortController(); -const managedHealth = createDbHealthCoordinator(async (autoRepair) => { +const managedHealth = createDbHealthCoordinator(async (autoRepair, skipIntegrity) => { const db = getDbInstance(); - const skipIntegrityCheck = process.env.OMNIROUTE_SKIP_DB_HEALTHCHECK === "1"; + const skipIntegrityCheck = skipIntegrity || process.env.OMNIROUTE_SKIP_DB_HEALTHCHECK === "1"; const backupDir = DB_BACKUPS_DIR || path.join(DATA_DIR, "db_backups"); const result = db.driver === "sql.js" || db.name === ":memory:" || !db.name @@ -969,10 +969,10 @@ const managedHealth = createDbHealthCoordinator(async (autoRepair) => { if (result.repairedCount > 0) invalidateDbCache(); return result; }); - -export function runManagedDbHealthCheck(options?: { autoRepair?: boolean }) { +type ManagedHealthCheckOptions = { autoRepair?: boolean; skipIntegrityCheck?: boolean }; +export function runManagedDbHealthCheck(options?: ManagedHealthCheckOptions) { if (getPagerCorruption()) managedHealth.invalidate(); - return managedHealth.run(options?.autoRepair === true); + return managedHealth.run(options?.autoRepair === true, options?.skipIntegrityCheck === true); } export function getDbInstance(): SqliteDatabase { diff --git a/src/lib/db/healthCheckRunner.ts b/src/lib/db/healthCheckRunner.ts index 3ce6420fa5..0808dcc244 100644 --- a/src/lib/db/healthCheckRunner.ts +++ b/src/lib/db/healthCheckRunner.ts @@ -12,14 +12,23 @@ export interface DbHealthJob { pagerCorruption: PagerCorruptionNote | null; } +/** + * `skipIntegrityCheck` (#13149): a caller may waive the integrity scan — the dashboard + * poll does. It is part of the job identity, so a waived scan is never replayed from the + * cache to a caller that asked for a full one (the reverse direction is safe). + */ export function createDbHealthCoordinator( - execute: (autoRepair: boolean) => Promise, + execute: (autoRepair: boolean, skipIntegrityCheck: boolean) => Promise, options: { now?: () => number; cacheMs?: number } = {} ) { const now = options.now ?? Date.now; const cacheMs = options.cacheMs ?? 60_000; let stopping = false; - let active: { autoRepair: boolean; promise: Promise } | null = null; + let active: { + autoRepair: boolean; + skipIntegrityCheck: boolean; + promise: Promise; + } | null = null; let cached: { result: DbHealthCheckResult; expires: number } | null = null; return { get busy(): boolean { @@ -34,10 +43,13 @@ export function createDbHealthCoordinator( cancel(); await active?.promise.catch(() => {}); }, - run(autoRepair: boolean): Promise { + run(autoRepair: boolean, skipIntegrityCheck = false): Promise { if (stopping) return Promise.reject(new Error("Database health checks are stopping")); if (active) { - return active.autoRepair === autoRepair + // A run that DID scan integrity satisfies a caller that was willing to skip it, + // never the reverse (#13149) — so only widen, never narrow, the in-flight job. + return active.autoRepair === autoRepair && + (active.skipIntegrityCheck === skipIntegrityCheck || !active.skipIntegrityCheck) ? active.promise : Promise.reject(new Error("Database health check already in progress")); } @@ -49,9 +61,11 @@ export function createDbHealthCoordinator( resolve = yes; reject = no; }); - active = { autoRepair, promise }; + active = { autoRepair, skipIntegrityCheck, promise }; const succeed = (result: DbHealthCheckResult) => { - if (!autoRepair) cached = { result, expires: now() + cacheMs }; + // Only a full (integrity-scanning) diagnosis may be replayed from the cache: + // caching a skipped scan would silently downgrade a later full request (#13149). + if (!autoRepair && !skipIntegrityCheck) cached = { result, expires: now() + cacheMs }; active = null; resolve(result); }; @@ -60,7 +74,7 @@ export function createDbHealthCoordinator( reject(error); }; try { - execute(autoRepair).then(succeed, fail); + execute(autoRepair, skipIntegrityCheck).then(succeed, fail); } catch (error) { fail(error); } diff --git a/tests/unit/db-health-poll-nonblocking.test.mjs b/tests/unit/db-health-poll-nonblocking.test.mjs new file mode 100644 index 0000000000..4d72457e56 --- /dev/null +++ b/tests/unit/db-health-poll-nonblocking.test.mjs @@ -0,0 +1,103 @@ +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { stripTypeScriptTypes } from "node:module"; +import test from "node:test"; +import vm from "node:vm"; + +import { createDbHealthCoordinator } from "../../src/lib/db/healthCheckRunner.ts"; + +// The managed health check is loaded from the REAL core.ts source (sliced, not +// reimplemented) so this test cannot drift away from production. #13717 moved the +// body behind a coordinator, so the slice starts at the coordinator construction. +const core = readFileSync(new URL("../../src/lib/db/core.ts", import.meta.url), "utf8"); +const managedCheck = core + .slice( + core.indexOf("const healthShutdown = new AbortController();"), + core.indexOf("export function getDbInstance()") + ) + .replace("export function", "function"); +const route = readFileSync(new URL("../../src/app/api/db/health/route.ts", import.meta.url), "utf8") + .replace(/^import .*;\n/gm, "") + .replaceAll("export async function", "async function"); + +function loadCheck(skipEnv, authenticated = true) { + const calls = []; + const context = vm.createContext({ + process: { env: { OMNIROUTE_SKIP_DB_HEALTHCHECK: skipEnv } }, + AbortController, + createDbHealthCoordinator, + getPagerCorruption: () => null, + invalidateDbCache: () => {}, + path: { join: (...parts) => parts.join("/") }, + DB_BACKUPS_DIR: "/tmp/db_backups", + DATA_DIR: "/tmp", + writeManagedDbBackup: () => assert.fail("read-only polling must not create backups"), + runDbHealthInChild: () => assert.fail("stub DB must stay on the owning-connection path"), + // No `name`/`driver`: core takes the in-process branch, never forking a child. + getDbInstance: () => ({}), + runDbHealthCheck: (_db, options) => { + calls.push(options); + return { status: "healthy", repairedCount: 0 }; + }, + isAuthenticated: async () => authenticated, + NextResponse: { json: (body, options) => ({ body, status: options?.status ?? 200 }) }, + sanitizeErrorMessage: (message) => message, + console, + }); + vm.runInContext(stripTypeScriptTypes(managedCheck + route), context); + return { context, calls }; +} + +test("dashboard GET skips integrity scans even without a deployment opt-out", async () => { + const { context, calls } = loadCheck(undefined); + for (let poll = 0; poll < 3; poll++) { + const response = await vm.runInContext("GET({})", context); + assert.equal(response.status, 200, JSON.stringify(response.body)); + } + // Three real calls: a skipped scan is never replayed from the diagnosis cache, + // otherwise a later full diagnosis would be silently downgraded. + assert.equal(calls.length, 3); + for (const options of calls) { + assert.equal(options.autoRepair, false); + assert.equal(options.skipIntegrityCheck, true); + } +}); + +test("managed checks honor the deployment opt-out, including manual repair", async () => { + const { context, calls } = loadCheck("1"); + await vm.runInContext("runManagedDbHealthCheck({skipIntegrityCheck:false})", context); + const response = await vm.runInContext("POST({})", context); + assert.equal(response.status, 200, JSON.stringify(response.body)); + assert.equal(calls.length, 2); + assert.equal(calls[0].skipIntegrityCheck, true); + assert.equal(calls[1].skipIntegrityCheck, true); + assert.equal(calls[1].autoRepair, true); +}); + +test("explicit repair retains integrity scans when the deployment allows them", async () => { + const { context, calls } = loadCheck("0"); + const response = await vm.runInContext("POST({})", context); + assert.equal(response.status, 200, JSON.stringify(response.body)); + assert.equal(Boolean(calls[0].skipIntegrityCheck), false); + assert.equal(calls[0].autoRepair, true); +}); + +test("a full diagnosis is still cached, a skipped one is not", async () => { + const { context, calls } = loadCheck(undefined); + await vm.runInContext("runManagedDbHealthCheck({})", context); + await vm.runInContext("runManagedDbHealthCheck({})", context); + assert.equal(calls.length, 1, "full diagnosis is served from the 60s cache"); + assert.equal(Boolean(calls[0].skipIntegrityCheck), false); + await vm.runInContext("GET({})", context); + // The cached FULL result satisfies a caller willing to skip the scan. + assert.equal(calls.length, 1); +}); + +test("unauthenticated polling and repair do not touch the database", async () => { + const { context, calls } = loadCheck(undefined, false); + for (const method of ["GET", "POST"]) { + const response = await vm.runInContext(`${method}({})`, context); + assert.equal(response.status, 401); + } + assert.equal(calls.length, 0); +}); diff --git a/tests/unit/rule12-error-sanitization-sweep.test.ts b/tests/unit/rule12-error-sanitization-sweep.test.ts index 5f5048d7d0..ce50ad516d 100644 --- a/tests/unit/rule12-error-sanitization-sweep.test.ts +++ b/tests/unit/rule12-error-sanitization-sweep.test.ts @@ -77,17 +77,15 @@ function patchPrepareToThrow(sqlMatch: string): () => void { }; } -function patchPragmaToThrow(): () => void { +// #13717 runs the managed health check in a child process for a real file-backed DB; +// a child never inherits the JS stubs below, so force the owning-connection path. +// (#13149 additionally makes the dashboard GET skip the integrity scan, so the throw +// has to come from a prepare() on the schema-version probe, not from pragma().) +function forceOwningConnectionPath(): () => void { const db = core.getDbInstance(); - const orig = db.pragma.bind(db); - // Select the owning-connection path; child processes do not inherit JS stubs. const nameDescriptor = Object.getOwnPropertyDescriptor(db, "name"); Object.defineProperty(db, "name", { configurable: true, value: ":memory:" }); - (db as unknown as { pragma: unknown }).pragma = () => { - throw makeLeakyError(); - }; return () => { - (db as unknown as { pragma: unknown }).pragma = orig; if (nameDescriptor) Object.defineProperty(db, "name", nameDescriptor); }; } @@ -140,7 +138,8 @@ test("GET /api/cache/entries → 500 body is sanitized (shape { error })", async }); test("GET /api/db/health → 500 body is sanitized (shape { error: { message } })", async () => { - const restore = patchPragmaToThrow(); + const restoreName = forceOwningConnectionPath(); + const restore = patchPrepareToThrow("SELECT value FROM db_meta WHERE key = 'schema_version'"); try { const res = await dbHealthRoute.GET(makeRequest("http://localhost/api/db/health")); assert.equal(res.status, 500); @@ -149,5 +148,6 @@ test("GET /api/db/health → 500 body is sanitized (shape { error: { message } } assertSanitized(body.error.message, "db/health GET"); } finally { restore(); + restoreName(); } });