From dd5e8f95b1fe6a2bde64f7aba5cffe80bdd006b1 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Tue, 1 Sep 2026 19:59:03 +0200 Subject: [PATCH] Fix graceful daemon shutdown on Windows (#4168) * test(server): probe supervised Windows cleanup * test(server): report both cleanup failures * fix(server): gracefully stop supervised workers --- .../tests/22-daemon-stop-supervisor.test.ts | 9 +- .../25-daemon-restart-supervisor.test.ts | 9 +- .../server/scripts/supervisor.logging.test.ts | 94 +++++++++++++++---- packages/server/scripts/supervisor.ts | 53 ++++++++--- packages/server/src/server/daemon-worker.ts | 22 +++-- 5 files changed, 139 insertions(+), 48 deletions(-) diff --git a/packages/cli/tests/22-daemon-stop-supervisor.test.ts b/packages/cli/tests/22-daemon-stop-supervisor.test.ts index 277e9fd4ad..f6687703fa 100644 --- a/packages/cli/tests/22-daemon-stop-supervisor.test.ts +++ b/packages/cli/tests/22-daemon-stop-supervisor.test.ts @@ -218,9 +218,12 @@ try { `stop should log lifecycle shutdown reason from daemon worker, logs:\n${capturedSupervisorLogs}`, ); assert( - capturedSupervisorLogs.includes('"msg":"Supervisor sending signal to worker"') && - capturedSupervisorLogs.includes('"signal":"SIGTERM"'), - `stop should log supervisor signal dispatch, logs:\n${capturedSupervisorLogs}`, + capturedSupervisorLogs.includes('"msg":"Supervisor requesting graceful worker shutdown"'), + `stop should log the graceful worker shutdown request, logs:\n${capturedSupervisorLogs}`, + ); + assert( + capturedSupervisorLogs.includes("Server closed"), + `stop should run daemon cleanup before the worker exits, logs:\n${capturedSupervisorLogs}`, ); assert( !capturedSupervisorLogs.includes("cli_shutdown"), diff --git a/packages/cli/tests/25-daemon-restart-supervisor.test.ts b/packages/cli/tests/25-daemon-restart-supervisor.test.ts index 10e559a2db..040fba7f2d 100644 --- a/packages/cli/tests/25-daemon-restart-supervisor.test.ts +++ b/packages/cli/tests/25-daemon-restart-supervisor.test.ts @@ -234,9 +234,12 @@ try { `restart should log lifecycle restart reason from daemon worker, logs:\n${capturedSupervisorLogs}`, ); assert( - capturedSupervisorLogs.includes('"msg":"Supervisor sending signal to worker"') && - capturedSupervisorLogs.includes('"signal":"SIGTERM"'), - `restart should log supervisor signal dispatch, logs:\n${capturedSupervisorLogs}`, + capturedSupervisorLogs.includes('"msg":"Supervisor requesting graceful worker shutdown"'), + `restart should log the graceful worker shutdown request, logs:\n${capturedSupervisorLogs}`, + ); + assert( + capturedSupervisorLogs.includes("Server closed"), + `restart should run daemon cleanup before replacing the worker, logs:\n${capturedSupervisorLogs}`, ); console.log("✓ app-style restart keeps daemon healthy and restarts worker\n"); } finally { diff --git a/packages/server/scripts/supervisor.logging.test.ts b/packages/server/scripts/supervisor.logging.test.ts index 9f61aa283d..9b4c622860 100644 --- a/packages/server/scripts/supervisor.logging.test.ts +++ b/packages/server/scripts/supervisor.logging.test.ts @@ -10,6 +10,15 @@ import { resolveSupervisorLogFile } from "./supervisor-log-config.js"; const repoRoot = path.resolve(fileURLToPath(new URL("../../..", import.meta.url))); const supervisorPath = fileURLToPath(new URL("./supervisor.ts", import.meta.url)); +function isProcessRunning(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + async function runSupervisorFixture(options: { workerSource: string; restartOnCrash?: boolean; @@ -172,9 +181,12 @@ describe("supervisor durable logging", () => { expect(result.log).toContain("raw stderr line\n"); }); - test("logs the worker shutdown reason before signaling the worker", async () => { + test("logs the worker shutdown reason before requesting graceful shutdown", async () => { const result = await runSupervisorFixture({ workerSource: ` + process.on("message", (message) => { + if (message?.type === "paseo:graceful-shutdown") process.exit(0); + }); process.send?.({ type: "paseo:shutdown", reason: "client_shutdown_rpc" }); setInterval(() => {}, 1000); `, @@ -184,17 +196,61 @@ describe("supervisor durable logging", () => { expect(result.signal).toBeNull(); expect(result.log).toContain('"msg":"Worker requested shutdown"'); expect(result.log).toContain('"reason":"client_shutdown_rpc"'); - expect(result.log).toContain('"msg":"Supervisor sending signal to worker"'); - expect(result.log).toContain('"signal":"SIGTERM"'); + expect(result.log).toContain('"msg":"Supervisor requesting graceful worker shutdown"'); expect(result.log).toContain('"workerPid":'); }); + test("lets the worker clean up its descendant before supervised shutdown", async () => { + const result = await runSupervisorFixture({ + workerSource: ` + import { spawn } from "node:child_process"; + + const descendant = spawn( + process.execPath, + ["-e", "setInterval(() => {}, 1000)"], + { detached: true, stdio: "ignore" }, + ); + descendant.unref(); + process.stdout.write(\`DESCENDANT_PID=\${descendant.pid}\\n\`); + + process.on("message", (message) => { + if (message?.type !== "paseo:graceful-shutdown") return; + descendant.once("exit", () => { + process.stdout.write("GRACEFUL_CLEANUP_RAN\\n"); + process.exit(0); + }); + descendant.kill("SIGTERM"); + }); + + process.send?.({ type: "paseo:shutdown", reason: "descendant_cleanup_probe" }); + setInterval(() => {}, 1000); + `, + }); + + const descendantPid = Number.parseInt( + result.stdout.match(/DESCENDANT_PID=(\d+)/)?.[1] ?? "", + 10, + ); + expect(Number.isInteger(descendantPid)).toBe(true); + + const descendantSurvived = isProcessRunning(descendantPid); + if (descendantSurvived) { + process.kill(descendantPid, "SIGKILL"); + } + + expect.soft(result.stdout).toContain("GRACEFUL_CLEANUP_RAN"); + expect(descendantSurvived).toBe(false); + }); + test("does not restart a worker based on heartbeat absence", async () => { const result = await runSupervisorFixture({ timeoutMs: 20_000, workerSource: ` import { existsSync, writeFileSync } from "node:fs"; + process.on("message", (message) => { + if (message?.type === "paseo:graceful-shutdown") process.exit(0); + }); const marker = process.argv[1] + ".started"; if (!existsSync(marker)) { writeFileSync(marker, "started"); @@ -216,26 +272,22 @@ describe("supervisor durable logging", () => { expect(result.log).not.toContain('"msg":"Worker heartbeat timed out; restarting worker"'); }, 25_000); - test.skipIf(isPlatform("win32"))( - "forces shutdown when a worker ignores SIGTERM", - async () => { - const result = await runSupervisorFixture({ - timeoutMs: 15_000, - workerSource: ` - process.on("SIGTERM", () => {}); + test("forces shutdown when a worker ignores the graceful shutdown request", async () => { + const result = await runSupervisorFixture({ + timeoutMs: 15_000, + workerSource: ` process.send?.({ type: "paseo:shutdown", reason: "stalled_worker_shutdown" }); setInterval(() => {}, 1_000); `, - }); + }); - expect(result.code).toBe(0); - expect(result.signal).toBeNull(); - expect(result.log).toContain('"reason":"stalled_worker_shutdown"'); - expect(result.log).toContain('"msg":"Worker did not exit after SIGTERM; forcing SIGKILL"'); - expect(result.log).toContain('"signal":"SIGKILL"'); - }, - 20_000, - ); + expect(result.code).toBe(0); + expect(result.signal).toBeNull(); + expect(result.log).toContain('"reason":"stalled_worker_shutdown"'); + expect(result.log).toContain( + '"msg":"Worker did not exit after graceful shutdown request; forcing process tree kill"', + ); + }, 20_000); test.skipIf(isPlatform("win32"))( "restarts after worker exit while a descendant retains the worker stdio", @@ -246,6 +298,9 @@ describe("supervisor durable logging", () => { import { spawn } from "node:child_process"; import { existsSync, writeFileSync } from "node:fs"; + process.on("message", (message) => { + if (message?.type === "paseo:graceful-shutdown") process.exit(0); + }); const marker = process.argv[1] + ".started"; if (!existsSync(marker)) { writeFileSync(marker, "started"); @@ -255,7 +310,6 @@ describe("supervisor durable logging", () => { { detached: true, stdio: ["ignore", "inherit", "inherit"] }, ); descendant.unref(); - process.on("SIGTERM", () => process.exit(0)); process.send?.({ type: "paseo:restart", reason: "stdio_descendant" }); setInterval(() => {}, 1000); } else { diff --git a/packages/server/scripts/supervisor.ts b/packages/server/scripts/supervisor.ts index 9c6b33f06b..f2d9fc7665 100644 --- a/packages/server/scripts/supervisor.ts +++ b/packages/server/scripts/supervisor.ts @@ -33,6 +33,11 @@ interface SupervisorHeartbeatMessage { type: "paseo:supervisor-heartbeat"; } +interface SupervisorGracefulShutdownMessage { + type: "paseo:graceful-shutdown"; + reason: string; +} + interface SupervisorOptions { name: string; startupMessage: string; @@ -192,11 +197,14 @@ export function runSupervisor(options: SupervisorOptions): SupervisorController if (child !== currentChild) { return; } - writeLifecycleLog("Worker did not exit after SIGTERM; forcing SIGKILL", { - reason, - supervisorPid: process.pid, - workerPid: currentChild.pid ?? null, - }); + writeLifecycleLog( + "Worker did not exit after graceful shutdown request; forcing process tree kill", + { + reason, + supervisorPid: process.pid, + workerPid: currentChild.pid ?? null, + }, + ); void signalProcessTree(currentChild, "SIGKILL").catch((error) => { writeLifecycleLog("Failed to force-kill worker process tree", { error: error instanceof Error ? error.message : String(error), @@ -327,17 +335,38 @@ export function runSupervisor(options: SupervisorOptions): SupervisorController }); }; - const signalWorker = (signal: NodeJS.Signals, reason: string): void => { + const requestWorkerShutdown = (reason: string): void => { if (!child) { return; } - writeLifecycleLog("Supervisor sending signal to worker", { + const currentChild = child; + const message: SupervisorGracefulShutdownMessage = { + type: "paseo:graceful-shutdown", + reason, + }; + writeLifecycleLog("Supervisor requesting graceful worker shutdown", { reason, - signal, supervisorPid: process.pid, - workerPid: child.pid ?? null, + workerPid: currentChild.pid ?? null, + }); + if (!currentChild.connected) { + writeLifecycleLog("Graceful worker shutdown IPC unavailable", { + reason, + supervisorPid: process.pid, + workerPid: currentChild.pid ?? null, + }); + return; + } + currentChild.send?.(message, (error) => { + if (error) { + writeLifecycleLog("Graceful worker shutdown IPC send failed", { + error: error instanceof Error ? error.message : String(error), + reason, + supervisorPid: process.pid, + workerPid: currentChild.pid ?? null, + }); + } }); - child.kill(signal); }; const requestRestart = (reason: string) => { @@ -347,7 +376,7 @@ export function runSupervisor(options: SupervisorOptions): SupervisorController restarting = true; writeLifecycleLog("Restart requested", { reason }); log(`${reason}. Stopping worker for restart...`); - signalWorker("SIGTERM", reason); + requestWorkerShutdown(reason); scheduleForceKill(reason); }; @@ -363,7 +392,7 @@ export function runSupervisor(options: SupervisorOptions): SupervisorController exitSupervisor(0); return; } - signalWorker("SIGTERM", reason); + requestWorkerShutdown(reason); scheduleForceKill(reason); }; diff --git a/packages/server/src/server/daemon-worker.ts b/packages/server/src/server/daemon-worker.ts index 922b79118a..f7a770d8be 100644 --- a/packages/server/src/server/daemon-worker.ts +++ b/packages/server/src/server/daemon-worker.ts @@ -23,10 +23,6 @@ type SupervisorLifecycleMessage = reason?: string; }; -interface SupervisorHeartbeatMessage { - type: "paseo:supervisor-heartbeat"; -} - interface BootstrapResult { paseoHome: string; logger: ReturnType; @@ -272,13 +268,19 @@ async function main() { }; process.on("message", (message: unknown) => { - if ( - typeof message === "object" && - message !== null && - "type" in message && - (message as SupervisorHeartbeatMessage).type === "paseo:supervisor-heartbeat" - ) { + if (typeof message !== "object" || message === null || !("type" in message)) { + return; + } + const type = (message as { type?: unknown }).type; + if (type === "paseo:supervisor-heartbeat") { lastSupervisorHeartbeatAt = Date.now(); + return; + } + if (type === "paseo:graceful-shutdown") { + const reason = (message as { reason?: unknown }).reason; + beginShutdown("Supervisor shutdown request", { + reason: typeof reason === "string" ? reason : "supervisor_requested_shutdown", + }); } }); process.on("disconnect", () => exitAfterSupervisorLoss("ipc_disconnect_event"));