From d2fadb01bce83e50911093c3c7f7a8a479ab5433 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Wed, 16 Sep 2026 06:13:59 -0300 Subject: [PATCH] fix(db): reconcile INCREMENTAL auto_vacuum drift via the vacuum scheduler (#13432) (#13786) Merged in the 2026-09-16 sweep of the maintainer's own open PRs, at the owner's explicit instruction. No push was made to the PR branch: the merge took the head as the owning session left it (verified OPEN, non-draft and MERGEABLE against the release tip immediately before merging). --- .../13432-incremental-auto-vacuum-drift.md | 1 + src/lib/db/databaseSettings.ts | 2 + src/lib/db/optimizationSettings.ts | 71 ++++++++- src/lib/db/vacuumScheduler.ts | 85 ++++++++++- src/types/databaseSettings.ts | 8 + .../issue-13432-auto-vacuum-drift.test.ts | 138 ++++++++++++++++++ 6 files changed, 301 insertions(+), 4 deletions(-) create mode 100644 changelog.d/fixes/13432-incremental-auto-vacuum-drift.md create mode 100644 tests/unit/issue-13432-auto-vacuum-drift.test.ts diff --git a/changelog.d/fixes/13432-incremental-auto-vacuum-drift.md b/changelog.d/fixes/13432-incremental-auto-vacuum-drift.md new file mode 100644 index 0000000000..15af1af534 --- /dev/null +++ b/changelog.d/fixes/13432-incremental-auto-vacuum-drift.md @@ -0,0 +1 @@ +- **fix(db):** reconcile `auto_vacuum` drift between the configured INCREMENTAL mode and the live SQLite pragma — detected at startup and reconciled out-of-request by the vacuum scheduler, which now also runs a bounded `PRAGMA incremental_vacuum` reclaim instead of an unconditional full `VACUUM` once INCREMENTAL is actually in effect (#13432) — thanks @tolgaaksoy diff --git a/src/lib/db/databaseSettings.ts b/src/lib/db/databaseSettings.ts index 0a12729242..903f7cafcc 100644 --- a/src/lib/db/databaseSettings.ts +++ b/src/lib/db/databaseSettings.ts @@ -260,6 +260,8 @@ export function getDatabaseSettings(): DatabaseSettings { vacuumState.lastRunAt !== null ? new Date(vacuumState.lastRunAt).toISOString() : null, lastOptimizationAt: null, integrityCheck: getIntegrityCheck(), + autoVacuumDrift: vacuumState.autoVacuumDrift, + lastReclaimedPages: vacuumState.lastReclaimedPages, }, }; } diff --git a/src/lib/db/optimizationSettings.ts b/src/lib/db/optimizationSettings.ts index 636313c406..70967ae270 100644 --- a/src/lib/db/optimizationSettings.ts +++ b/src/lib/db/optimizationSettings.ts @@ -4,7 +4,29 @@ import type { SqliteAdapter } from "./adapters/types"; type SqliteDatabase = SqliteAdapter; type DatabaseOptimizationSettings = DatabaseSettings["optimization"]; -type AutoVacuumMode = DatabaseOptimizationSettings["autoVacuumMode"]; +export type AutoVacuumMode = DatabaseOptimizationSettings["autoVacuumMode"]; + +/** + * A mismatch between the configured `optimization.autoVacuumMode` (the + * `key_value` config store) and the live SQLite `auto_vacuum` pragma on the + * actual database file. See #13432 — migration 046 seeds the config value on + * every database (including pre-existing ones) but SQLite only applies + * `auto_vacuum` on a subsequent `VACUUM`, which the startup path deliberately + * never runs synchronously (that would reintroduce the blocking-VACUUM + * hazard tracked by #12821). + */ +export interface AutoVacuumDrift { + configured: AutoVacuumMode; + live: AutoVacuumMode; +} + +// Shared key_value coordinate for the persisted drift record. Written here +// (at boot, directly against the `db` handle being initialized — NOT via +// getDbInstance(), which is not yet set at this point in core.ts's boot +// sequence) and read/cleared by vacuumScheduler.ts once the scheduler runs +// the reconcile out-of-request. +const AUTO_VACUUM_DRIFT_NAMESPACE = "scheduler"; +const AUTO_VACUUM_DRIFT_KEY = "vacuumDrift"; const AUTO_VACUUM_MODE_TO_PRAGMA: Record = { NONE: 0, @@ -233,6 +255,40 @@ export function applyDatabaseOptimizationSettingsForDb( ); } +/** + * Compares the configured `autoVacuumMode` against the live SQLite pragma. + * Pure/read-only — never mutates the database. Returns `null` when they + * already agree. + */ +export function checkAutoVacuumDrift( + db: SqliteDatabase, + settings: DatabaseOptimizationSettings +): AutoVacuumDrift | null { + const liveMode = getAutoVacuumModeForDb(db); + if (liveMode === settings.autoVacuumMode) return null; + return { configured: settings.autoVacuumMode, live: liveMode }; +} + +/** + * Persists (or clears, when `drift` is `null`) the auto_vacuum drift record + * directly against the given `db` handle. Deliberately does NOT go through + * `getDbInstance()` — at the one call site that matters (startup, inside + * core.ts before `setDb()` has run) that would recurse back into database + * initialization. + */ +function persistAutoVacuumDriftRecord(db: SqliteDatabase, drift: AutoVacuumDrift | null): void { + try { + db.prepare("INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES (?, ?, ?)").run( + AUTO_VACUUM_DRIFT_NAMESPACE, + AUTO_VACUUM_DRIFT_KEY, + JSON.stringify(drift) + ); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.warn(`[DB] Failed to persist auto_vacuum drift record: ${message}`); + } +} + export function applyStoredDatabaseOptimizationSettings(db: SqliteDatabase): void { const settings = readDatabaseOptimizationSettings(db); // Startup can happen concurrently in test workers and clustered hosts. Only @@ -241,6 +297,19 @@ export function applyStoredDatabaseOptimizationSettings(db: SqliteDatabase): voi applyDatabaseOptimizationSettingsForDb(db, settings, { applyPersistent: false, }); + + // #13432: detect (but never synchronously fix — that would reintroduce the + // #12821 blocking-VACUUM-at-boot hazard) a drift between the configured + // autoVacuumMode and the live pragma. Reconciliation happens out-of-request + // on the next scheduled vacuumScheduler run (see vacuumScheduler.ts::runNow). + const drift = checkAutoVacuumDrift(db, settings); + if (drift) { + console.warn( + `[DB] auto_vacuum drift detected (configured=${drift.configured}, live=${drift.live}); ` + + `scheduling reconcile on the next vacuum-scheduler run` + ); + } + persistAutoVacuumDriftRecord(db, drift); } export function setAutoVacuumForDb(db: SqliteDatabase, mode: AutoVacuumMode): void { diff --git a/src/lib/db/vacuumScheduler.ts b/src/lib/db/vacuumScheduler.ts index ad0e183556..143313075b 100644 --- a/src/lib/db/vacuumScheduler.ts +++ b/src/lib/db/vacuumScheduler.ts @@ -1,7 +1,13 @@ import { DEFAULT_DATABASE_SETTINGS } from "@/types/databaseSettings"; import { MAX_TIMER_TIMEOUT_MS } from "@/shared/utils/runtimeTimeouts"; +import type { SqliteAdapter } from "./adapters/types"; import { getDbInstance } from "./core"; +import { + getAutoVacuumModeForDb, + setAutoVacuumForDb, + type AutoVacuumDrift, +} from "./optimizationSettings"; // Direct `key_value` access — the existing `keyValueStore` helpers only exist // in test fixtures; the 3 production call sites (pricingSync, jsonMigration, // serviceModels) all use `getDbInstance().prepare(...).run()` directly. We @@ -46,6 +52,10 @@ export interface VacuumSchedulerState { lastDurationMs: number | null; isRunning: boolean; nextRunAt: number | null; + /** #13432 — configured vs live auto_vacuum mismatch pending reconcile, or null once reconciled. */ + autoVacuumDrift: AutoVacuumDrift | null; + /** Pages freed by the most recent bounded `PRAGMA incremental_vacuum` batch, or null if the last run was a full VACUUM / drift reconcile. */ + lastReclaimedPages: number | null; } export type ScheduledVacuum = (typeof DEFAULT_DATABASE_SETTINGS)["optimization"]["scheduledVacuum"]; @@ -73,8 +83,19 @@ const STATE_DEFAULTS: VacuumSchedulerState = { lastDurationMs: null, isRunning: false, nextRunAt: null, + autoVacuumDrift: null, + lastReclaimedPages: null, }; +// Shared key_value coordinate with optimizationSettings.ts, which writes the +// initial drift record at boot (see AUTO_VACUUM_DRIFT_NAMESPACE/KEY there). +const AUTO_VACUUM_DRIFT_NAMESPACE = "scheduler"; +const AUTO_VACUUM_DRIFT_KEY = "vacuumDrift"; + +// Bounded per-run reclaim so a scheduled vacuum on a multi-GB INCREMENTAL +// database never blocks for as long as a full VACUUM would (#13432 fix #2). +const INCREMENTAL_VACUUM_BATCH_PAGES = 2000; + let timer: ReturnType | null = null; let currentState: VacuumSchedulerState = { ...STATE_DEFAULTS }; @@ -230,6 +251,35 @@ function persistState(): void { setKeyValue(KEY_VALUE_NAMESPACE, KEY_VALUE_KEY, JSON.stringify(currentState)); } +function isAutoVacuumDrift(value: unknown): value is AutoVacuumDrift { + return isRecord(value) && typeof value.configured === "string" && typeof value.live === "string"; +} + +function loadAutoVacuumDrift(): AutoVacuumDrift | null { + const raw = getKeyValue(AUTO_VACUUM_DRIFT_NAMESPACE, AUTO_VACUUM_DRIFT_KEY); + if (!raw) return null; + const parsed = parseJsonSafe(raw); + return isAutoVacuumDrift(parsed) ? parsed : null; +} + +function clearAutoVacuumDrift(): void { + setKeyValue(AUTO_VACUUM_DRIFT_NAMESPACE, AUTO_VACUUM_DRIFT_KEY, JSON.stringify(null)); +} + +/** + * Bounded reclaim step for a database already running `auto_vacuum=INCREMENTAL`: + * frees at most `INCREMENTAL_VACUUM_BATCH_PAGES` pages per scheduled run + * instead of the unconditional full `VACUUM` this scheduler used to always + * issue (#13432 fix #2 / reporter's suggested fix #2). Returns the number of + * freelist pages actually reclaimed by this batch. + */ +function runBoundedIncrementalVacuum(db: SqliteAdapter): number { + const before = Number(db.pragma("freelist_count", { simple: true }) ?? 0); + db.pragma(`incremental_vacuum(${INCREMENTAL_VACUUM_BATCH_PAGES})`); + const after = Number(db.pragma("freelist_count", { simple: true }) ?? 0); + return Math.max(0, before - after); +} + function loadPersistedState(): Partial { const raw = getKeyValue(KEY_VALUE_NAMESPACE, KEY_VALUE_KEY); if (!raw) return {}; @@ -252,7 +302,12 @@ export function refresh(): VacuumSchedulerState { return getState(); } -export async function runNow(): Promise<{ success: boolean; durationMs: number; error?: string }> { +export async function runNow(): Promise<{ + success: boolean; + durationMs: number; + error?: string; + reclaimedPages?: number; +}> { if (currentState.isRunning) { return { success: false, durationMs: 0, error: "already_running" }; } @@ -262,14 +317,34 @@ export async function runNow(): Promise<{ success: boolean; durationMs: number; const start = Date.now(); try { const db = getDbInstance(); - db.exec("VACUUM"); + let reclaimedPages: number | null = null; + + // #13432: reconcile a configured-vs-live auto_vacuum drift first, out of + // request handling, on this bounded/observable scheduled path — never + // synchronously at startup (see optimizationSettings.ts::applyStoredDatabaseOptimizationSettings). + const drift = loadAutoVacuumDrift(); + if (drift) { + console.log( + `[DB] Reconciling auto_vacuum drift (configured=${drift.configured}, live=${drift.live}): ` + + `running one-time conversion VACUUM to apply the configured mode to the database file.` + ); + setAutoVacuumForDb(db, drift.configured); + clearAutoVacuumDrift(); + } else if (getAutoVacuumModeForDb(db) === "INCREMENTAL") { + reclaimedPages = runBoundedIncrementalVacuum(db); + } else { + db.exec("VACUUM"); + } + const duration = Date.now() - start; currentState.lastRunAt = start; currentState.lastError = null; currentState.lastDurationMs = duration; + currentState.lastReclaimedPages = reclaimedPages; + currentState.autoVacuumDrift = loadAutoVacuumDrift(); currentState.isRunning = false; refresh(); // reset the next-run clock from this successful run - return { success: true, durationMs: duration }; + return { success: true, durationMs: duration, reclaimedPages: reclaimedPages ?? undefined }; } catch (err) { const message = err instanceof Error ? err.message : String(err); currentState.lastError = message; @@ -296,6 +371,10 @@ export function init(): VacuumSchedulerState { ...persisted, isRunning: false, // never resume a "running" state across restarts nextRunAt: null, // recompute below + // Always reload from the drift record's own key_value entry rather than + // trusting a stale copy embedded in the scheduler state blob — it is the + // source of truth optimizationSettings.ts writes at every boot. + autoVacuumDrift: loadAutoVacuumDrift(), }; return refresh(); } diff --git a/src/types/databaseSettings.ts b/src/types/databaseSettings.ts index 2b4820e198..999212c8c2 100644 --- a/src/types/databaseSettings.ts +++ b/src/types/databaseSettings.ts @@ -81,6 +81,14 @@ export interface DatabaseSettings { lastVacuumAt: string | null; lastOptimizationAt: string | null; integrityCheck: "ok" | "error" | null; + /** + * #13432 — non-null while the configured `optimization.autoVacuumMode` + * has not yet been applied to the live SQLite file. Cleared once the + * vacuum scheduler's next scheduled run reconciles it. + */ + autoVacuumDrift: { configured: string; live: string } | null; + /** Pages freed by the most recent bounded `PRAGMA incremental_vacuum` batch, or null. */ + lastReclaimedPages: number | null; }; } diff --git a/tests/unit/issue-13432-auto-vacuum-drift.test.ts b/tests/unit/issue-13432-auto-vacuum-drift.test.ts new file mode 100644 index 0000000000..6008e06acd --- /dev/null +++ b/tests/unit/issue-13432-auto-vacuum-drift.test.ts @@ -0,0 +1,138 @@ +/** + * Regression test for #13432 — migration 046 seeds + * `databaseSettings.autoVacuumMode = "INCREMENTAL"` into the key_value config + * store for every database it runs against, including pre-existing ones, but + * never touches the live SQLite `auto_vacuum` pragma (that requires a + * `VACUUM`). The only automatic startup path + * (`applyStoredDatabaseOptimizationSettings`) deliberately skips applying it + * synchronously — reapplying it inline at boot would reintroduce the + * blocking-VACUUM-on-a-multi-GB-database hazard tracked by #12821. + * + * Fix: detect the drift at startup (never fix it synchronously there), log + + * persist it, and let the existing out-of-request vacuumScheduler reconcile + * it (pragma flip + one-time conversion VACUUM) on its next scheduled run. + * Once auto_vacuum is actually INCREMENTAL, subsequent scheduled runs use a + * bounded `PRAGMA incremental_vacuum` batch instead of an unconditional full + * `VACUUM`. + * + * DB isolation pattern mirrors tests/unit/db/vacuum-scheduler.test.ts: + * temp DATA_DIR, resetDbInstance() before each test, cleanup in test.after(). + */ +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"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-13432-")); +const originalDataDir = process.env.DATA_DIR; + +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +core.resetDbInstance(); + +const optimizationSettings = await import("../../src/lib/db/optimizationSettings.ts"); +const scheduler = await import("../../src/lib/db/vacuumScheduler.ts"); + +function seedMigration046Config(mode: "NONE" | "FULL" | "INCREMENTAL") { + // Simulate src/lib/db/migrations/046_database_settings.sql:41 running + // against a pre-existing database: it only ever touches the config store. + const db = core.getDbInstance(); + db.prepare("INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES (?, ?, ?)").run( + "databaseSettings", + "autoVacuumMode", + JSON.stringify(mode) + ); +} + +function readPersistedDriftRecord(): unknown { + const db = core.getDbInstance(); + const row = db + .prepare("SELECT value FROM key_value WHERE namespace = ? AND key = ?") + .get("scheduler", "vacuumDrift") as { value: string } | undefined; + return row ? JSON.parse(row.value) : undefined; +} + +test.beforeEach(() => { + scheduler.__resetForTests(); + const db = core.getDbInstance(); + db.prepare("DELETE FROM key_value WHERE namespace IN ('scheduler', 'databaseSettings')").run(); +}); + +test.after(() => { + scheduler.__resetForTests(); + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + if (originalDataDir === undefined) delete process.env.DATA_DIR; + else process.env.DATA_DIR = originalDataDir; +}); + +test("issue #13432: startup drift detection records the mismatch WITHOUT running a blocking VACUUM", () => { + const db = core.getDbInstance(); + + const initialLiveMode = optimizationSettings.getAutoVacuumModeForDb(db); + assert.equal(initialLiveMode, "NONE", "sanity: fresh test DB defaults to auto_vacuum=NONE"); + + seedMigration046Config("INCREMENTAL"); + + // Simulate the next app startup (src/lib/db/core.ts calls this on every boot). + optimizationSettings.applyStoredDatabaseOptimizationSettings(db); + + // The core bug: the live pragma must NOT have been synchronously flipped — + // that is the #12821 blocking-VACUUM-at-boot hazard this fix must avoid. + assert.equal( + optimizationSettings.getAutoVacuumModeForDb(db), + "NONE", + "startup path must never run a synchronous VACUUM to fix drift" + ); + + // But the drift must now be detected and persisted for the scheduler to + // reconcile out-of-request. + const persisted = readPersistedDriftRecord(); + assert.deepEqual(persisted, { configured: "INCREMENTAL", live: "NONE" }); +}); + +test("issue #13432: vacuumScheduler.runNow() reconciles a pending startup drift out-of-request", async () => { + const db = core.getDbInstance(); + seedMigration046Config("INCREMENTAL"); + optimizationSettings.applyStoredDatabaseOptimizationSettings(db); + assert.deepEqual(readPersistedDriftRecord(), { configured: "INCREMENTAL", live: "NONE" }); + + scheduler.init(); + try { + const result = await scheduler.runNow(); + assert.equal(result.success, true); + + // The scheduler's bounded, out-of-request reconcile run is where the + // one-time conversion VACUUM is allowed to happen. + assert.equal(optimizationSettings.getAutoVacuumModeForDb(db), "INCREMENTAL"); + assert.equal(scheduler.getState().autoVacuumDrift, null); + assert.equal(readPersistedDriftRecord(), null); + } finally { + scheduler.stop(); + } +}); + +test("issue #13432: once INCREMENTAL is actually in effect and no drift remains, runNow() uses a bounded PRAGMA incremental_vacuum instead of a full VACUUM", async () => { + const db = core.getDbInstance(); + seedMigration046Config("INCREMENTAL"); + optimizationSettings.applyStoredDatabaseOptimizationSettings(db); + + scheduler.init(); + try { + // First run reconciles the drift (pragma flip + one-time VACUUM). + await scheduler.runNow(); + assert.equal(optimizationSettings.getAutoVacuumModeForDb(db), "INCREMENTAL"); + assert.equal(scheduler.getState().autoVacuumDrift, null); + + // Second run: no drift left, live mode is INCREMENTAL — must take the + // bounded incremental-vacuum path (reported via lastReclaimedPages), + // never the unconditional full VACUUM this scheduler used to always run. + const second = await scheduler.runNow(); + assert.equal(second.success, true); + assert.equal(typeof scheduler.getState().lastReclaimedPages, "number"); + } finally { + scheduler.stop(); + } +});