Address PR review feedback (#1752)
- Verify the WAL->DELETE journal-mode switch instead of discarding exec()'s result. During a rolling upgrade a lingering WAL holder blocks the exclusive lock the switch needs, so SQLite either throws SQLITE_BUSY or no-ops and returns "wal". Capture both outcomes and warn loudly so the residual -shm SIGBUS surface is observable, rather than silently swallowed. - Do not rethrow: the condition is transient and self-healing (the next start after the last WAL holder exits migrates cleanly); hard-failing would make the central DB unopenable during the very upgrade window it describes. - Add a migration-path regression test (a WAL holder blocking the switch) that the prior fresh-DB-only tests did not cover. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -3,6 +3,7 @@ import { mkdtempSync, rmSync, statSync, existsSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { CentralDatabase, createCentralDatabase, toJson, fromJson } from "../central-db.js";
|
||||
import { DatabaseSync } from "../sqlite-adapter.js";
|
||||
|
||||
describe("CentralDatabase", () => {
|
||||
let tempDir: string;
|
||||
@@ -73,6 +74,49 @@ describe("CentralDatabase", () => {
|
||||
expect(synchronous.synchronous).toBe(2); // FULL — durability posture preserved
|
||||
});
|
||||
|
||||
it("warns (does not throw) when a WAL holder blocks the DELETE migration", () => {
|
||||
// Migration-path regression: during a rolling upgrade an old-version process
|
||||
// can still hold the central DB open in WAL mode. WAL→DELETE needs an exclusive
|
||||
// lock it cannot get, so SQLite keeps WAL and the PRAGMA *returns* "wal" instead
|
||||
// of throwing. The new connection must surface that loudly rather than silently
|
||||
// run with the SIGBUS `-shm` surface still present.
|
||||
const dbFile = join(tempDir, "fusion-central.db");
|
||||
const walHolder = new DatabaseSync(dbFile);
|
||||
walHolder.exec("PRAGMA journal_mode = WAL");
|
||||
walHolder.exec("CREATE TABLE IF NOT EXISTS lock_probe (id INTEGER PRIMARY KEY)");
|
||||
walHolder.exec("INSERT INTO lock_probe (id) VALUES (1)");
|
||||
// Hold an open read transaction so the switch cannot checkpoint/truncate the WAL.
|
||||
walHolder.exec("BEGIN");
|
||||
walHolder.prepare("SELECT * FROM lock_probe").all();
|
||||
|
||||
const warnings: string[] = [];
|
||||
const originalWarn = console.warn;
|
||||
console.warn = (...args: unknown[]) => {
|
||||
warnings.push(args.map(String).join(" "));
|
||||
};
|
||||
|
||||
let blocked: CentralDatabase | undefined;
|
||||
try {
|
||||
// busyTimeoutMs:0 → the failed switch returns immediately instead of waiting.
|
||||
expect(() => {
|
||||
blocked = new CentralDatabase(tempDir, { busyTimeoutMs: 0 });
|
||||
}).not.toThrow();
|
||||
|
||||
const mode = blocked!.prepare("PRAGMA journal_mode").get() as { journal_mode: string };
|
||||
// The switch failed: this connection is still WAL (documents the known gap)…
|
||||
expect(mode.journal_mode).toBe("wal");
|
||||
// …and the failure was surfaced, not swallowed.
|
||||
expect(
|
||||
warnings.some((w) => /journal_mode=DELETE did not take effect/.test(w)),
|
||||
).toBe(true);
|
||||
} finally {
|
||||
console.warn = originalWarn;
|
||||
blocked?.close();
|
||||
walHolder.exec("ROLLBACK");
|
||||
walHolder.close();
|
||||
}
|
||||
});
|
||||
|
||||
it("should seed lastModified on init", () => {
|
||||
db.init();
|
||||
const lastModified = db.getLastModified();
|
||||
|
||||
@@ -578,7 +578,43 @@ export class CentralDatabase implements CentralClaimStore {
|
||||
// and coordinates cross-process access via plain POSIX byte-range locks
|
||||
// instead; busy_timeout above absorbs the writer-serialization contention
|
||||
// that DELETE mode trades for WAL's reader/writer concurrency.
|
||||
this.db.exec("PRAGMA journal_mode = DELETE");
|
||||
//
|
||||
// FNXC:Database 2026-06-25-07:10:
|
||||
// The WAL→DELETE switch is NOT silent-safe: SQLite needs an exclusive lock to
|
||||
// checkpoint and drop `-wal`/`-shm`. If another connection still holds the DB
|
||||
// open in WAL mode (the rolling-upgrade window, where an old-version process is
|
||||
// still running) the switch cannot complete, and SQLite signals this in one of
|
||||
// TWO ways depending on busy_timeout: it throws SQLITE_BUSY ("database is
|
||||
// locked"), or it no-ops and the PRAGMA *returns the current mode* ("wal").
|
||||
// `exec()` would swallow the return value and let the throw abort the
|
||||
// constructor, so we capture both: try the switch, treat a throw or a non-DELETE
|
||||
// result identically, and warn loudly. We deliberately DO NOT rethrow — the
|
||||
// condition is transient and self-healing (the next start after the last WAL
|
||||
// holder exits migrates cleanly), and the residual SIGBUS surface during the
|
||||
// window is no worse than the pre-fix status quo. Hard-failing here would make
|
||||
// the central DB unopenable during the very upgrade window this describes.
|
||||
let journalMode: string | undefined;
|
||||
let switchError: unknown;
|
||||
try {
|
||||
const journalRow = this.db.prepare("PRAGMA journal_mode = DELETE").get() as
|
||||
| { journal_mode?: string }
|
||||
| undefined;
|
||||
journalMode = journalRow?.journal_mode?.toLowerCase();
|
||||
} catch (error) {
|
||||
switchError = error;
|
||||
}
|
||||
if (journalMode !== "delete") {
|
||||
const detail = switchError
|
||||
? `failed: ${switchError instanceof Error ? switchError.message : String(switchError)}`
|
||||
: `current mode: ${journalMode ?? "unknown"}`;
|
||||
console.warn(
|
||||
`[fusion:central-db] PRAGMA journal_mode=DELETE did not take effect ` +
|
||||
`(${detail}) at ${this.dbPath}. Another process likely still holds the ` +
|
||||
`database open in WAL mode; this connection keeps the WAL -shm mmap ` +
|
||||
`(SIGBUS) surface until all WAL-mode holders exit and a fresh process ` +
|
||||
`re-runs the migration.`,
|
||||
);
|
||||
}
|
||||
// synchronous=FULL is SQLite's compiled-in default; set explicitly so the
|
||||
// durability posture is intentional and visible, and so a future change to
|
||||
// synchronous=NORMAL is a deliberate edit, not an accidental drift. The WAL-only
|
||||
|
||||
Reference in New Issue
Block a user