diff --git a/bin/cli/commands/stop.mjs b/bin/cli/commands/stop.mjs index bd244b0a93..b3dbf64b40 100644 --- a/bin/cli/commands/stop.mjs +++ b/bin/cli/commands/stop.mjs @@ -8,6 +8,7 @@ import { sleep, } from "../utils/pid.mjs"; import { t } from "../i18n.mjs"; +import { stopProcessGracefully } from "../../../src/shared/platform/windowsProcess.ts"; const execFileAsync = promisify(execFile); @@ -27,18 +28,11 @@ export async function runStopCommand(opts = {}) { if (pid && isPidRunning(pid)) { console.log(t("stop.stopping", { pid })); try { - process.kill(pid, "SIGTERM"); - - let waited = 0; - while (waited < 5000 && isPidRunning(pid)) { - await sleep(100); - waited += 100; - } - - if (isPidRunning(pid)) { - process.kill(pid, "SIGKILL"); - await sleep(500); - } + // #8045: on win32, process.kill(pid, "SIGTERM") unconditionally force-terminates + // the target instead of delivering an interceptable signal, racing (and beating) + // the server's own async graceful shutdown / WAL checkpoint. stopProcessGracefully + // skips the immediate SIGTERM on win32 and just polls before escalating to SIGKILL. + await stopProcessGracefully({ pid, timeoutMs: 5000, isPidRunning, sleep }); killAllSubprocesses(); cleanupPidFile("server"); diff --git a/bin/cli/runtime/processSupervisor.mjs b/bin/cli/runtime/processSupervisor.mjs index b5fa60ffde..8afa50744f 100644 --- a/bin/cli/runtime/processSupervisor.mjs +++ b/bin/cli/runtime/processSupervisor.mjs @@ -1,6 +1,6 @@ import { spawn } from "node:child_process"; import { dirname } from "node:path"; -import { writePidFile, cleanupPidFile, killAllSubprocesses } from "../utils/pid.mjs"; +import { writePidFile, cleanupPidFile, killAllSubprocesses, isPidRunning } from "../utils/pid.mjs"; import { RESTART_RESET_MS, DEFAULT_MAX_RESTARTS, @@ -9,6 +9,7 @@ import { waitUntilPortFree, } from "./supervisorPolicy.mjs"; import { buildNodeHeapArgs } from "../../../scripts/build/runtime-env.mjs"; +import { stopProcessGracefully } from "../../../src/shared/platform/windowsProcess.ts"; const CRASH_LOG_LINES = 50; @@ -135,14 +136,13 @@ export class ServerSupervisor { stop() { this.isShuttingDown = true; if (this.child?.pid) { - try { - process.kill(this.child.pid, "SIGTERM"); - } catch {} - setTimeout(() => { - try { - process.kill(this.child.pid, "SIGKILL"); - } catch {} - }, 5000); + // #8045: on win32, process.kill(pid, "SIGTERM") unconditionally force-terminates + // the target — it is never a real, interceptable signal there. The child already + // receives the real CTRL_C_EVENT/CTRL_CLOSE_EVENT independently (it shares the + // console) and runs its own async graceful shutdown (WAL checkpoint). Sending + // SIGTERM immediately on win32 races and beats that cleanup. Fire-and-forget: + // stop() itself stays sync so callers keep their existing control flow. + void stopProcessGracefully({ pid: this.child.pid, timeoutMs: 5000, isPidRunning }); } killAllSubprocesses(); } diff --git a/changelog.d/fixes/8045-windows-wal-checkpoint.md b/changelog.d/fixes/8045-windows-wal-checkpoint.md new file mode 100644 index 0000000000..f76203b76f --- /dev/null +++ b/changelog.d/fixes/8045-windows-wal-checkpoint.md @@ -0,0 +1 @@ +- fix(db): register SIGHUP handler and stop force-killing the server on win32 stop paths so storage.sqlite's WAL gets checkpointed on shutdown (#8045) diff --git a/src/lib/gracefulShutdown.ts b/src/lib/gracefulShutdown.ts index d75bd6e13e..e123d729c3 100644 --- a/src/lib/gracefulShutdown.ts +++ b/src/lib/gracefulShutdown.ts @@ -161,6 +161,11 @@ export function initGracefulShutdown(): void { process.on("SIGTERM", () => shutdown("SIGTERM")); process.on("SIGINT", () => shutdown("SIGINT")); + // #8045: on Windows, closing the console window delivers CTRL_CLOSE_EVENT, which + // Node/libuv maps to a JS-visible "SIGHUP" event — without this listener, closing + // the window never runs cleanup() (WAL checkpoint + closeDbInstance()), leaving + // storage.sqlite's WAL un-checkpointed for the next launch. + process.on("SIGHUP", () => shutdown("SIGHUP")); console.log("[Shutdown] Graceful shutdown handlers registered."); } diff --git a/src/shared/platform/windowsProcess.ts b/src/shared/platform/windowsProcess.ts new file mode 100644 index 0000000000..1e1e01b3ce --- /dev/null +++ b/src/shared/platform/windowsProcess.ts @@ -0,0 +1,88 @@ +/** + * Platform-aware graceful process termination for the CLI's own child/self stop + * paths (`bin/cli/runtime/processSupervisor.mjs`, `bin/cli/commands/stop.mjs`). + * + * #8045: on win32, `process.kill(pid, "SIGTERM")` is documented by Node.js to cause + * "unconditional termination of the target process" — it is never a real, interceptable + * signal there, identical to SIGKILL. Sending it to the OmniRoute server child + * immediately on every stop/Ctrl+C force-kills it before its own async + * `initGracefulShutdown()` cleanup (WAL checkpoint + closeDbInstance()) has any + * realistic chance to run, corrupting storage.sqlite's WAL state for the next launch. + * + * On win32, the target process already receives the real CTRL_C_EVENT/CTRL_CLOSE_EVENT + * independently (it shares the console) and runs its own graceful shutdown — so instead + * of racing it with an immediate force-kill, this helper polls for exit and only + * escalates to SIGKILL if the process is still alive after the timeout. + * + * @module shared/platform/windowsProcess + */ + +export interface StopProcessGracefullyOptions { + /** PID of the target process to stop. */ + pid: number; + /** Max time to wait for the process to exit on its own before escalating (ms). */ + timeoutMs?: number; + /** Poll interval while waiting for exit (ms). */ + pollIntervalMs?: number; + /** Injectable liveness check (defaults to `process.kill(pid, 0)`-based check). */ + isPidRunning?: (pid: number) => boolean; + /** Injectable sleep (for tests). */ + sleep?: (ms: number) => Promise; + /** Injectable platform override (for tests); defaults to `process.platform`. */ + platform?: NodeJS.Platform; +} + +const defaultIsPidRunning = (pid: number): boolean => { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +}; + +const defaultSleep = (ms: number): Promise => + new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Stop `pid` without force-killing it immediately on win32. + * + * - Non-win32: sends SIGTERM immediately (unchanged prior behavior — SIGTERM is a + * real, interceptable signal on POSIX, so this still lets the target run its own + * graceful shutdown before an eventual SIGKILL escalation). + * - win32: does NOT send SIGTERM (it would force-kill immediately, racing the + * target's own console-close/Ctrl+C handling). Instead polls `isPidRunning` for up + * to `timeoutMs`, then escalates to SIGKILL only if the process is still alive. + */ +export async function stopProcessGracefully(options: StopProcessGracefullyOptions): Promise { + const { + pid, + timeoutMs = 5000, + pollIntervalMs = 100, + isPidRunning = defaultIsPidRunning, + sleep = defaultSleep, + platform = process.platform, + } = options; + + if (platform !== "win32") { + try { + process.kill(pid, "SIGTERM"); + } catch { + // Process already gone — nothing to escalate. + return; + } + } + + const start = Date.now(); + while (Date.now() - start < timeoutMs && isPidRunning(pid)) { + await sleep(pollIntervalMs); + } + + if (isPidRunning(pid)) { + try { + process.kill(pid, "SIGKILL"); + } catch { + // Already gone. + } + } +} diff --git a/tests/unit/graceful-shutdown-sighup-8045.test.ts b/tests/unit/graceful-shutdown-sighup-8045.test.ts new file mode 100644 index 0000000000..b12c1304a1 --- /dev/null +++ b/tests/unit/graceful-shutdown-sighup-8045.test.ts @@ -0,0 +1,23 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +// #8045: on Windows, closing the console window delivers CTRL_CLOSE_EVENT, which +// Node/libuv maps to a JS-visible "SIGHUP" event (confirmed: nodejs/node#10165, +// Node process docs — "SIGHUP is generated on Windows when the console window is +// closed"). Before this fix, initGracefulShutdown() only registered SIGTERM/SIGINT, +// so the "close the window" path never ran cleanup() (WAL checkpoint(TRUNCATE) + +// closeDbInstance()), leaving storage.sqlite's WAL un-checkpointed for the next launch. +test("initGracefulShutdown registers a SIGHUP handler (Windows console-close path)", async () => { + const before = process.listenerCount("SIGHUP"); + const { initGracefulShutdown } = await import("../../src/lib/gracefulShutdown.ts"); + initGracefulShutdown(); + const after = process.listenerCount("SIGHUP"); + assert.ok( + after > before, + `Expected initGracefulShutdown() to add a SIGHUP listener (before=${before}, after=${after}).` + ); + + // Clean up: remove all SIGHUP listeners added by this test so it doesn't leak + // into other test files sharing the same process. + process.removeAllListeners("SIGHUP"); +}); diff --git a/tests/unit/windows-process-stop-8045.test.ts b/tests/unit/windows-process-stop-8045.test.ts new file mode 100644 index 0000000000..6eb4746e3d --- /dev/null +++ b/tests/unit/windows-process-stop-8045.test.ts @@ -0,0 +1,97 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { stopProcessGracefully } from "../../src/shared/platform/windowsProcess.ts"; + +// #8045: on win32, process.kill(pid, "SIGTERM") is documented to unconditionally +// force-terminate the target — never a real, interceptable signal. Sending it +// immediately races (and beats) the target's own async graceful-shutdown WAL +// checkpoint. stopProcessGracefully() must NOT send SIGTERM on win32. + +test("stopProcessGracefully: on win32 does NOT send SIGTERM, only polls then escalates to SIGKILL", async () => { + const signalsSent: string[] = []; + let running = true; + const sleeps: number[] = []; + + await stopProcessGracefully({ + pid: 4242, + timeoutMs: 300, + pollIntervalMs: 50, + platform: "win32", + isPidRunning: () => running, + sleep: async (ms) => { + sleeps.push(ms); + // Simulate the process exiting on its own shortly after the first poll, + // mimicking its own SIGHUP/CTRL_CLOSE_EVENT-driven graceful shutdown. + if (sleeps.length >= 2) running = false; + }, + }); + + assert.ok( + !signalsSent.includes("SIGTERM"), + "must not send SIGTERM on win32 (it force-kills unconditionally there)" + ); +}); + +test("stopProcessGracefully: on win32 escalates to SIGKILL if the process never exits", async () => { + const killed: Array<{ pid: number; signal: string }> = []; + const originalKill = process.kill; + // @ts-expect-error — test-only override to observe signals without touching a real PID. + process.kill = (pid: number, signal?: string | number) => { + killed.push({ pid, signal: String(signal) }); + return true; + }; + + try { + await stopProcessGracefully({ + pid: 4243, + timeoutMs: 100, + pollIntervalMs: 20, + platform: "win32", + isPidRunning: () => true, // never exits on its own + sleep: async () => {}, + }); + } finally { + process.kill = originalKill; + } + + assert.ok( + killed.some((k) => k.signal === "SIGKILL"), + `expected an eventual SIGKILL escalation, got: ${JSON.stringify(killed)}` + ); + assert.ok( + !killed.some((k) => k.signal === "SIGTERM"), + "must never send SIGTERM on win32" + ); +}); + +test("stopProcessGracefully: on non-win32 sends SIGTERM immediately (unchanged POSIX behavior)", async () => { + const killed: Array<{ pid: number; signal: string }> = []; + const originalKill = process.kill; + // @ts-expect-error — test-only override. + process.kill = (pid: number, signal?: string | number) => { + killed.push({ pid, signal: String(signal) }); + return true; + }; + + let running = true; + try { + await stopProcessGracefully({ + pid: 4244, + timeoutMs: 200, + pollIntervalMs: 20, + platform: "linux", + isPidRunning: () => running, + sleep: async () => { + running = false; + }, + }); + } finally { + process.kill = originalKill; + } + + assert.equal(killed[0]?.signal, "SIGTERM", "must send SIGTERM immediately on POSIX"); + assert.ok( + !killed.some((k) => k.signal === "SIGKILL"), + "must not escalate to SIGKILL once the process exited" + ); +});