From 80bdb4d45b9bcb0f96064aeb031c9286f3e91ae7 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 7 May 2026 13:18:39 +0800 Subject: [PATCH] Use tree-kill for daemon shutdown (#788) * Use tree-kill for daemon shutdown * fix: update lockfile signatures and Nix hash * Use tree-kill for daemon shutdown --------- Co-authored-by: github-actions[bot] --- .github/workflows/ci.yml | 2 +- nix/package.nix | 2 +- package-lock.json | 3 +- packages/cli/package.json | 1 + .../cli/src/commands/daemon/local-daemon.ts | 48 ++-- packages/cli/src/commands/daemon/stop.ts | 2 + .../tests/33-daemon-stop-tree-kill.test.ts | 176 ++++++++++++ .../desktop/src/daemon/daemon-manager.test.ts | 42 +++ packages/desktop/src/daemon/daemon-manager.ts | 57 +--- packages/server/package.json | 1 + .../agent/providers/codex-app-server-agent.ts | 4 +- .../server/agent/providers/opencode-agent.ts | 4 +- .../server/src/utils/process-tree.test.ts | 259 ------------------ packages/server/src/utils/process-tree.ts | 155 ----------- packages/server/src/utils/tree-kill.test.ts | 184 +++++++++++++ packages/server/src/utils/tree-kill.ts | 115 ++++++++ 16 files changed, 561 insertions(+), 494 deletions(-) create mode 100644 packages/cli/tests/33-daemon-stop-tree-kill.test.ts delete mode 100644 packages/server/src/utils/process-tree.test.ts delete mode 100644 packages/server/src/utils/process-tree.ts create mode 100644 packages/server/src/utils/tree-kill.test.ts create mode 100644 packages/server/src/utils/tree-kill.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 213ca7625..c37346fef 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,7 +124,7 @@ jobs: src/utils/spawn.launch-regression.test.ts src/utils/spawn.percent-escape.test.ts src/utils/spawn.test.ts - src/utils/process-tree.test.ts + src/utils/tree-kill.test.ts src/utils/run-git-command.test.ts src/utils/checkout-git-rev-parse.test.ts src/terminal/worker-terminal-manager.test.ts diff --git a/nix/package.nix b/nix/package.nix index 1861f4377..a164a1764 100644 --- a/nix/package.nix +++ b/nix/package.nix @@ -42,7 +42,7 @@ buildNpmPackage rec { # To update: run `nix build` with lib.fakeHash, copy the `got:` hash. # CI auto-updates this when package-lock.json changes (see .github/workflows/). - npmDepsHash = "sha256-uwVCaND/FlOpcwB4/XOsyMi9moD/UuPQjMNOgswmz1U="; + npmDepsHash = "sha256-mGnJDX1LOORj7fDRPcJYIFG0D+rLDyom6LktWhwZasw="; # Prevent onnxruntime-node's install script from running during automatic # npm rebuild (it tries to download from api.nuget.org, which fails in the sandbox). diff --git a/package-lock.json b/package-lock.json index ba896dd26..b52b10234 100644 --- a/package-lock.json +++ b/package-lock.json @@ -36354,7 +36354,6 @@ "version": "1.2.2", "resolved": "https://registry.npmjs.org/tree-kill/-/tree-kill-1.2.2.tgz", "integrity": "sha512-L0Orpi8qGpRG//Nd+H90vFB+3iHnue1zSSGmNOOCh1GLJ7rUKVwV2HvijphGQS2UmhUZewS9VgvxYIdgr+fG1A==", - "dev": true, "license": "MIT", "bin": { "tree-kill": "cli.js" @@ -38681,6 +38680,7 @@ "chalk": "^5.3.0", "commander": "^12.0.0", "mime-types": "^2.1.35", + "tree-kill": "^1.2.2", "ws": "^8.14.2", "yaml": "^2.8.2" }, @@ -38878,6 +38878,7 @@ "sherpa-onnx": "1.12.28", "sherpa-onnx-node": "1.12.28", "strip-ansi": "^7.1.2", + "tree-kill": "^1.2.2", "uuid": "^9.0.1", "which": "^5.0.0", "ws": "^8.14.2", diff --git a/packages/cli/package.json b/packages/cli/package.json index 154d849e5..77077002b 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -28,6 +28,7 @@ "chalk": "^5.3.0", "commander": "^12.0.0", "mime-types": "^2.1.35", + "tree-kill": "^1.2.2", "ws": "^8.14.2", "yaml": "^2.8.2" }, diff --git a/packages/cli/src/commands/daemon/local-daemon.ts b/packages/cli/src/commands/daemon/local-daemon.ts index ea485da6a..4e2eab7cb 100644 --- a/packages/cli/src/commands/daemon/local-daemon.ts +++ b/packages/cli/src/commands/daemon/local-daemon.ts @@ -3,6 +3,7 @@ import { existsSync, readFileSync } from "node:fs"; import { createRequire } from "node:module"; import path from "node:path"; import { loadConfig, resolvePaseoHome, spawnProcess } from "@getpaseo/server"; +import treeKill from "tree-kill"; import { tryConnectToDaemon } from "../../utils/client.js"; export interface DaemonStartOptions { @@ -272,27 +273,40 @@ function signalProcessSafely(pid: number, signal: NodeJS.Signals): boolean { } } -function signalProcessGroupSafely(pid: number, signal: NodeJS.Signals): boolean { +async function signalProcessTreeSafely(pid: number, signal: NodeJS.Signals): Promise { if (!Number.isInteger(pid) || pid <= 1 || pid === process.pid) { return false; } - if (process.platform === "win32") { - return signalProcessSafely(pid, signal); - } + return new Promise((resolve, reject) => { + treeKill(pid, signal, (err) => { + if (!err) { + resolve(true); + return; + } + const code = readNodeErrnoCode(err); + if (code === "ESRCH") { + resolve(false); + return; + } + if (code === "EPERM") { + resolve(true); + return; + } + reject(err); + }); + }); +} + +async function signalProcessTreeOrOwnerSafely( + pid: number, + signal: NodeJS.Signals, +): Promise { try { - process.kill(-pid, signal); - return true; - } catch (err) { - const code = readNodeErrnoCode(err); - if (code === "ESRCH") { - return signalProcessSafely(pid, signal); - } - if (code === "EPERM") { - return true; - } - throw err; + return await signalProcessTreeSafely(pid, signal); + } catch { + return signalProcessSafely(pid, signal); } } @@ -530,7 +544,7 @@ export async function stopLocalDaemon( const fallbackMessage = shutdownAttempt.requested ? null : shutdownAttempt.reason; let forced = false; if (!lifecycleRequested) { - const signaled = signalProcessSafely(pid, "SIGTERM"); + const signaled = await signalProcessTreeOrOwnerSafely(pid, "SIGTERM"); if (!signaled) { return { action: "not_running", @@ -545,7 +559,7 @@ export async function stopLocalDaemon( let stopped = await waitForPidExit(pid, timeoutMs); if (!stopped && options.force) { forced = true; - signalProcessGroupSafely(pid, "SIGKILL"); + await signalProcessTreeOrOwnerSafely(pid, "SIGKILL"); stopped = await waitForPidExit(pid, killTimeoutMs); } diff --git a/packages/cli/src/commands/daemon/stop.ts b/packages/cli/src/commands/daemon/stop.ts index c08cbea50..b893f6677 100644 --- a/packages/cli/src/commands/daemon/stop.ts +++ b/packages/cli/src/commands/daemon/stop.ts @@ -15,6 +15,7 @@ interface StopResult { action: "stopped" | "not_running"; home: string; pid: string; + forced: boolean; message: string; } @@ -73,6 +74,7 @@ export async function runStopCommand( action: result.action, home: result.home, pid: result.pid === null ? "-" : String(result.pid), + forced: result.forced, message: result.message, }, schema: stopResultSchema, diff --git a/packages/cli/tests/33-daemon-stop-tree-kill.test.ts b/packages/cli/tests/33-daemon-stop-tree-kill.test.ts new file mode 100644 index 000000000..8a60fbcb9 --- /dev/null +++ b/packages/cli/tests/33-daemon-stop-tree-kill.test.ts @@ -0,0 +1,176 @@ +#!/usr/bin/env npx tsx + +/** + * Regression: forced daemon stop must kill descendants even when they started + * their own process group. The pid lock still points at the daemon owner; the + * child stays linked by PPID but would survive a process-group-only kill. + */ + +import assert from "node:assert"; +import { spawn, type ChildProcess } from "node:child_process"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { $ } from "zx"; + +$.verbose = false; + +const testEnv = { + PASEO_LOCAL_SPEECH_AUTO_DOWNLOAD: process.env.PASEO_LOCAL_SPEECH_AUTO_DOWNLOAD ?? "0", + PASEO_DICTATION_ENABLED: process.env.PASEO_DICTATION_ENABLED ?? "0", + PASEO_VOICE_MODE_ENABLED: process.env.PASEO_VOICE_MODE_ENABLED ?? "0", +}; + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +function isProcessRunning(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) { + return false; + } + + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +async function waitFor( + check: () => Promise | boolean, + timeoutMs: number, + message: string, +): Promise { + const deadline = Date.now() + timeoutMs; + async function poll(): Promise { + if (await check()) return; + if (Date.now() >= deadline) throw new Error(message); + await sleep(50); + return poll(); + } + return poll(); +} + +async function readPidFileNumber(filePath: string): Promise { + try { + const raw = (await readFile(filePath, "utf-8")).trim(); + const pid = Number.parseInt(raw, 10); + return Number.isInteger(pid) && pid > 0 ? pid : null; + } catch { + return null; + } +} + +function killIfRunning(pid: number | null): void { + if (!pid || !isProcessRunning(pid)) return; + try { + process.kill(pid, "SIGKILL"); + } catch { + // ignore cleanup races + } +} + +console.log("=== Daemon Stop Tree Kill Regression ===\n"); + +if (process.platform === "win32") { + console.log("Skipping separate process-group regression on Windows"); + process.exit(0); +} + +const paseoHome = await mkdtemp(join(tmpdir(), "paseo-stop-tree-kill-")); +const childPidPath = join(paseoHome, "descendant.pid"); +let ownerProcess: ChildProcess | null = null; +let descendantPid: number | null = null; + +try { + await mkdir(paseoHome, { recursive: true }); + + console.log("Test 1: start daemon-owner fixture with a detached descendant"); + ownerProcess = spawn( + process.execPath, + [ + "-e", + ` + const { spawn } = require("node:child_process"); + process.on("SIGTERM", () => {}); + const child = spawn(process.execPath, [ + "-e", + ${JSON.stringify(` + const fs = require("node:fs"); + process.on("SIGTERM", () => {}); + fs.writeFileSync(${JSON.stringify(childPidPath)}, String(process.pid)); + setInterval(() => {}, 1000); + `)} + ], { detached: true, stdio: "ignore" }); + child.unref(); + setInterval(() => {}, 1000); + `, + ], + { + stdio: "ignore", + }, + ); + + assert(ownerProcess.pid, "owner pid should exist"); + await writeFile( + join(paseoHome, "paseo.pid"), + JSON.stringify({ + pid: ownerProcess.pid, + listen: "127.0.0.1:1", + startedAt: new Date().toISOString(), + }), + ); + + await waitFor( + async () => { + descendantPid = await readPidFileNumber(childPidPath); + return ( + isProcessRunning(ownerProcess?.pid ?? -1) && + descendantPid !== null && + isProcessRunning(descendantPid) + ); + }, + 5000, + "owner descendant did not become running in time", + ); + console.log(`✓ owner ${ownerProcess.pid} started descendant ${descendantPid}\n`); + + console.log("Test 2: forced daemon stop kills owner and separate-PGID descendant"); + const stopResult = + await $`PASEO_HOME=${paseoHome} PASEO_LOCAL_SPEECH_AUTO_DOWNLOAD=${testEnv.PASEO_LOCAL_SPEECH_AUTO_DOWNLOAD} PASEO_DICTATION_ENABLED=${testEnv.PASEO_DICTATION_ENABLED} PASEO_VOICE_MODE_ENABLED=${testEnv.PASEO_VOICE_MODE_ENABLED} npx paseo daemon stop --home ${paseoHome} --json --timeout 1 --force --kill-timeout 2`.nothrow(); + assert.strictEqual(stopResult.exitCode, 0, `stop should succeed: ${stopResult.stderr}`); + const parsed = JSON.parse(stopResult.stdout) as { + action?: unknown; + forced?: unknown; + message?: unknown; + }; + assert.deepStrictEqual( + { + action: parsed.action, + forced: parsed.forced, + message: parsed.message, + }, + { + action: "stopped", + forced: true, + message: "Daemon owner process was force-stopped", + }, + `stop should report forced tree cleanup: ${stopResult.stdout}`, + ); + + const ownerPid = ownerProcess.pid; + await waitFor( + () => !isProcessRunning(ownerPid ?? -1) && !isProcessRunning(descendantPid ?? -1), + 5000, + `owner (${ownerPid}) or descendant (${descendantPid}) survived forced stop`, + ); + console.log("✓ forced stop killed the full process tree\n"); +} finally { + killIfRunning(ownerProcess?.pid ?? null); + killIfRunning(descendantPid); + await rm(paseoHome, { recursive: true, force: true }); +} + +console.log("=== Daemon stop tree kill regression test passed ==="); diff --git a/packages/desktop/src/daemon/daemon-manager.test.ts b/packages/desktop/src/daemon/daemon-manager.test.ts index ff9fcbf54..88d4348b0 100644 --- a/packages/desktop/src/daemon/daemon-manager.test.ts +++ b/packages/desktop/src/daemon/daemon-manager.test.ts @@ -112,4 +112,46 @@ describe("daemon-manager commands", () => { expect(mocks.runCliJsonCommand).toHaveBeenCalledWith(["daemon", "status", "--json"]); }); + + it("routes running desktop daemon stops through CLI daemon stop", async () => { + mocks.runCliJsonCommand + .mockResolvedValueOnce({ + localDaemon: "running", + serverId: "server-1", + pid: 4242, + listen: "127.0.0.1:6767", + desktopManaged: true, + }) + .mockResolvedValueOnce({ action: "stopped" }) + .mockResolvedValueOnce({ + localDaemon: "stopped", + serverId: "", + }); + const handlers = createDaemonCommandHandlers(); + + await expect(handlers.stop_desktop_daemon()).resolves.toEqual({ + serverId: "", + status: "stopped", + listen: null, + hostname: null, + pid: null, + home: "/tmp/paseo-home", + version: null, + desktopManaged: false, + error: null, + }); + + expect(mocks.runCliJsonCommand).toHaveBeenNthCalledWith(1, ["daemon", "status", "--json"]); + expect(mocks.runCliJsonCommand).toHaveBeenNthCalledWith(2, [ + "daemon", + "stop", + "--json", + "--timeout", + "5", + "--force", + "--kill-timeout", + "5", + ]); + expect(mocks.runCliJsonCommand).toHaveBeenNthCalledWith(3, ["daemon", "status", "--json"]); + }); }); diff --git a/packages/desktop/src/daemon/daemon-manager.ts b/packages/desktop/src/daemon/daemon-manager.ts index 6da383602..ee974b62b 100644 --- a/packages/desktop/src/daemon/daemon-manager.ts +++ b/packages/desktop/src/daemon/daemon-manager.ts @@ -43,11 +43,8 @@ import { getDesktopSettingsStore } from "../settings/desktop-settings-electron.j import { isRunningUnderARM64Translation } from "../system/arm64-translation.js"; const DAEMON_LOG_FILENAME = "daemon.log"; -const PID_POLL_INTERVAL_MS = 100; const STARTUP_POLL_INTERVAL_MS = 200; const STARTUP_POLL_MAX_ATTEMPTS = 150; -const STOP_TIMEOUT_MS = 15_000; -const KILL_TIMEOUT_MS = 3_000; const DETACHED_STARTUP_GRACE_MS = 1200; type DesktopDaemonState = "starting" | "running" | "stopped" | "errored"; @@ -136,52 +133,12 @@ function isProcessRunning(pid: number): boolean { } } -function signalProcessSafely(pid: number, signal: NodeJS.Signals): boolean { - if (!Number.isInteger(pid) || pid <= 1 || pid === process.pid) return false; - try { - process.kill(pid, signal); - return true; - } catch (err) { - if (typeof err === "object" && err !== null && "code" in err) { - if (err.code === "ESRCH") return false; - if (err.code === "EPERM") return true; - } - throw err; - } -} - -function signalProcessGroupSafely(pid: number, signal: NodeJS.Signals): boolean { - if (!Number.isInteger(pid) || pid <= 1 || pid === process.pid) return false; - if (process.platform === "win32") return signalProcessSafely(pid, signal); - try { - process.kill(-pid, signal); - return true; - } catch (err) { - if (typeof err === "object" && err !== null && "code" in err) { - if (err.code === "ESRCH") return signalProcessSafely(pid, signal); - if (err.code === "EPERM") return true; - } - throw err; - } -} - function sleep(ms: number): Promise { return new Promise((resolve) => { setTimeout(resolve, ms); }); } -async function waitForPidExit(pid: number, timeoutMs: number): Promise { - const deadline = Date.now() + timeoutMs; - async function poll(): Promise { - if (!isProcessRunning(pid)) return true; - if (Date.now() >= deadline) return !isProcessRunning(pid); - await sleep(PID_POLL_INTERVAL_MS); - return poll(); - } - return poll(); -} - function tailFile(filePath: string, lines = 50): string { try { const content = readFileSync(filePath, "utf-8"); @@ -449,19 +406,7 @@ export async function stopDesktopDaemon(): Promise { const status = await resolveDesktopDaemonStatus(); if (status.status !== "running" || !status.pid) return status; - const pid = status.pid; - signalProcessSafely(pid, "SIGTERM"); - - let stopped = await waitForPidExit(pid, STOP_TIMEOUT_MS); - if (!stopped) { - signalProcessGroupSafely(pid, "SIGKILL"); - stopped = await waitForPidExit(pid, KILL_TIMEOUT_MS); - } - - if (!stopped) { - throw new Error(`Timed out waiting for daemon PID ${pid} to stop`); - } - + await stopDesktopDaemonViaCli(); return await resolveDesktopDaemonStatus(); } diff --git a/packages/server/package.json b/packages/server/package.json index f26ce7872..82fccb69d 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -87,6 +87,7 @@ "sherpa-onnx": "1.12.28", "sherpa-onnx-node": "1.12.28", "strip-ansi": "^7.1.2", + "tree-kill": "^1.2.2", "uuid": "^9.0.1", "which": "^5.0.0", "ws": "^8.14.2", diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.ts index ff2edd443..44fdd8a3a 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.ts @@ -51,7 +51,7 @@ import { type ProviderRuntimeSettings, } from "../provider-launch-config.js"; import { findExecutable, isCommandAvailable } from "../../../utils/executable.js"; -import { terminateProcessTree } from "../../../utils/process-tree.js"; +import { terminateWithTreeKill } from "../../../utils/tree-kill.js"; import { spawnProcess } from "../../../utils/spawn.js"; import { extractCodexTerminalSessionId, nonEmptyString } from "./tool-call-mapper-utils.js"; import { buildCodexFeatures, codexModelSupportsFastMode } from "./codex-feature-definitions.js"; @@ -787,7 +787,7 @@ class CodexAppServerClient { } catch { // ignore } - const result = await terminateProcessTree(this.child, { + const result = await terminateWithTreeKill(this.child, { gracefulTimeoutMs: APP_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT_MS, forceTimeoutMs: APP_SERVER_FORCE_SHUTDOWN_TIMEOUT_MS, onForceSignal: () => { diff --git a/packages/server/src/server/agent/providers/opencode-agent.ts b/packages/server/src/server/agent/providers/opencode-agent.ts index 9b44eb7fe..2e8057db6 100644 --- a/packages/server/src/server/agent/providers/opencode-agent.ts +++ b/packages/server/src/server/agent/providers/opencode-agent.ts @@ -47,7 +47,7 @@ import { type ProviderRuntimeSettings, } from "../provider-launch-config.js"; import { findExecutable, isCommandAvailable } from "../../../utils/executable.js"; -import { terminateProcessTree } from "../../../utils/process-tree.js"; +import { terminateWithTreeKill } from "../../../utils/tree-kill.js"; import { withTimeout } from "../../../utils/promise-timeout.js"; import { spawnProcess } from "../../../utils/spawn.js"; import { buildToolCallDisplayModel } from "../../../shared/tool-call-display.js"; @@ -975,7 +975,7 @@ export class OpenCodeServerManager { if (server.process.killed) { return; } - const result = await terminateProcessTree(server.process, { + const result = await terminateWithTreeKill(server.process, { gracefulTimeoutMs: OPENCODE_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT_MS, forceTimeoutMs: OPENCODE_SERVER_FORCE_SHUTDOWN_TIMEOUT_MS, onForceSignal: () => { diff --git a/packages/server/src/utils/process-tree.test.ts b/packages/server/src/utils/process-tree.test.ts deleted file mode 100644 index e56e71f01..000000000 --- a/packages/server/src/utils/process-tree.test.ts +++ /dev/null @@ -1,259 +0,0 @@ -import { execFile, spawn } from "node:child_process"; -import { EventEmitter } from "node:events"; -import { promisify } from "node:util"; -import { describe, expect, test, vi } from "vitest"; - -import { signalProcessTree, terminateProcessTree, type ProcessTreeTarget } from "./process-tree.js"; - -const execFileAsync = promisify(execFile); - -describe("signalProcessTree", () => { - test.each(["darwin", "linux"] as const)("signals the %s process group", (platform) => { - const child = createChild({ pid: 1234 }); - const kill = vi.fn(() => true); - - signalProcessTree(child, "SIGTERM", { platform, kill }); - - expect(kill).toHaveBeenCalledWith(-1234, "SIGTERM"); - expect(child.kill).not.toHaveBeenCalled(); - }); - - test("falls back to direct child signaling when POSIX process group signaling fails", () => { - const child = createChild({ pid: 1234 }); - - signalProcessTree(child, "SIGTERM", { - platform: "darwin", - kill: vi.fn(() => { - throw new Error("no process group"); - }), - }); - - expect(child.kill).toHaveBeenCalledWith("SIGTERM"); - }); - - test("uses taskkill for Windows process trees", () => { - const child = createChild({ pid: 1234 }); - const execFileMock = vi.fn((_file, _args, callback) => { - callback(null); - }); - - signalProcessTree(child, "SIGTERM", { platform: "win32", execFile: execFileMock }); - - expect(execFileMock).toHaveBeenCalledWith( - "taskkill", - ["/pid", "1234", "/T"], - expect.any(Function), - ); - expect(child.kill).not.toHaveBeenCalled(); - }); - - test("forces Windows process tree cleanup for SIGKILL", () => { - const child = createChild({ pid: 1234 }); - const execFileMock = vi.fn((_file, _args, callback) => { - callback(null); - }); - - signalProcessTree(child, "SIGKILL", { platform: "win32", execFile: execFileMock }); - - expect(execFileMock).toHaveBeenCalledWith( - "taskkill", - ["/pid", "1234", "/T", "/F"], - expect.any(Function), - ); - }); - - test("falls back to direct child signaling when taskkill cannot run", () => { - const child = createChild({ pid: 1234 }); - const execFileMock = vi.fn((_file, _args, callback) => { - callback(Object.assign(new Error("taskkill failed"), { code: "ENOENT" })); - }); - - signalProcessTree(child, "SIGTERM", { platform: "win32", execFile: execFileMock }); - - expect(child.kill).toHaveBeenCalledWith("SIGTERM"); - }); - - test("does not signal an exited child", () => { - const child = createChild({ pid: 1234, exitCode: 0 }); - const kill = vi.fn(() => true); - - signalProcessTree(child, "SIGTERM", { platform: "darwin", kill }); - - expect(kill).not.toHaveBeenCalled(); - expect(child.kill).not.toHaveBeenCalled(); - }); - - test("terminateProcessTree returns after graceful exit", async () => { - const child = createEventedChild({ pid: 1234 }); - const kill = vi.fn(() => true); - - const resultPromise = terminateProcessTree(child, { - platform: "darwin", - gracefulTimeoutMs: 100, - forceTimeoutMs: 100, - kill, - }); - child.emit("exit"); - - await expect(resultPromise).resolves.toBe("terminated"); - expect(kill).toHaveBeenCalledWith(-1234, "SIGTERM"); - expect(kill).not.toHaveBeenCalledWith(-1234, "SIGKILL"); - }); - - test("terminateProcessTree sends SIGKILL after graceful timeout", async () => { - vi.useFakeTimers(); - try { - const child = createEventedChild({ pid: 1234 }); - const kill = vi.fn(() => true); - const onForceSignal = vi.fn(); - - const resultPromise = terminateProcessTree(child, { - platform: "darwin", - gracefulTimeoutMs: 100, - forceTimeoutMs: 100, - kill, - onForceSignal, - }); - - await vi.advanceTimersByTimeAsync(100); - expect(onForceSignal).toHaveBeenCalledTimes(1); - expect(kill).toHaveBeenNthCalledWith(1, -1234, "SIGTERM"); - expect(kill).toHaveBeenNthCalledWith(2, -1234, "SIGKILL"); - - child.emit("exit"); - await expect(resultPromise).resolves.toBe("killed"); - } finally { - vi.useRealTimers(); - } - }); - - test("terminateProcessTree reports when forced cleanup does not exit", async () => { - vi.useFakeTimers(); - try { - const child = createEventedChild({ pid: 1234 }); - const kill = vi.fn(() => true); - - const resultPromise = terminateProcessTree(child, { - platform: "darwin", - gracefulTimeoutMs: 100, - forceTimeoutMs: 100, - kill, - }); - - await vi.advanceTimersByTimeAsync(200); - - await expect(resultPromise).resolves.toBe("kill-timeout"); - expect(kill).toHaveBeenNthCalledWith(1, -1234, "SIGTERM"); - expect(kill).toHaveBeenNthCalledWith(2, -1234, "SIGKILL"); - } finally { - vi.useRealTimers(); - } - }); - - test.skipIf(process.platform === "win32")("kills a detached POSIX process group", async () => { - const child = spawn("/bin/sh", ["-c", "sleep 60 & wait"], { - detached: true, - stdio: "ignore", - }); - const pid = child.pid; - if (typeof pid !== "number") { - throw new Error("spawned shell did not expose a pid"); - } - - let childPids: number[] = []; - try { - childPids = await waitForChildren(pid); - await expect( - terminateProcessTree(child, { - gracefulTimeoutMs: 1000, - forceTimeoutMs: 1000, - }), - ).resolves.toMatch(/^(terminated|killed)$/); - await waitForNoLivePids(childPids); - } finally { - cleanupPidGroup(pid); - for (const childPid of childPids) { - cleanupPid(childPid); - } - } - }); -}); - -function createChild(options: { pid?: number; exitCode?: number | null } = {}): ProcessTreeTarget { - return { - pid: options.pid, - exitCode: options.exitCode ?? null, - signalCode: null, - kill: vi.fn(() => true), - }; -} - -function createEventedChild(options: { pid?: number }): ProcessTreeTarget & EventEmitter { - const child = new EventEmitter() as ProcessTreeTarget & EventEmitter; - child.pid = options.pid; - child.exitCode = null; - child.signalCode = null; - child.kill = vi.fn(() => true); - return child; -} - -async function waitForChildren(parentPid: number): Promise { - return waitFor(async () => { - const { stdout } = await execFileAsync("pgrep", ["-P", String(parentPid)]).catch(() => ({ - stdout: "", - })); - const pids = stdout - .trim() - .split(/\s+/) - .filter(Boolean) - .map((value) => Number.parseInt(value, 10)) - .filter((value) => Number.isInteger(value) && value > 0); - return pids.length > 0 ? pids : null; - }); -} - -async function waitForNoLivePids(pids: number[]): Promise { - await waitFor(() => (pids.every((pid) => !isPidAlive(pid)) ? true : null)); -} - -async function waitFor(probe: () => T | null | Promise): Promise { - const deadline = Date.now() + 3000; - let lastError: unknown; - while (Date.now() < deadline) { - try { - const value = await probe(); - if (value !== null) { - return value; - } - } catch (error) { - lastError = error; - } - await new Promise((resolve) => setTimeout(resolve, 50)); - } - throw lastError instanceof Error ? lastError : new Error("timed out waiting for condition"); -} - -function isPidAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch { - return false; - } -} - -function cleanupPidGroup(pid: number): void { - try { - process.kill(-pid, "SIGKILL"); - } catch { - // ignore cleanup races - } -} - -function cleanupPid(pid: number): void { - try { - process.kill(pid, "SIGKILL"); - } catch { - // ignore cleanup races - } -} diff --git a/packages/server/src/utils/process-tree.ts b/packages/server/src/utils/process-tree.ts deleted file mode 100644 index 5c0a37f35..000000000 --- a/packages/server/src/utils/process-tree.ts +++ /dev/null @@ -1,155 +0,0 @@ -import { execFile } from "node:child_process"; - -export interface ProcessTreeTarget { - pid?: number; - killed?: boolean; - exitCode?: number | null; - signalCode?: NodeJS.Signals | null; - kill(signal?: NodeJS.Signals | number): boolean; - once?(event: "exit", listener: () => void): unknown; -} - -interface SignalProcessTreeOptions { - platform?: NodeJS.Platform; - kill?: (pid: number, signal?: NodeJS.Signals | number) => boolean; - execFile?: ( - file: string, - args: string[], - callback: (error: NodeJS.ErrnoException | null) => void, - ) => unknown; -} - -interface TerminateProcessTreeOptions extends SignalProcessTreeOptions { - gracefulSignal?: NodeJS.Signals; - forceSignal?: NodeJS.Signals; - gracefulTimeoutMs: number; - forceTimeoutMs?: number; - onForceSignal?: () => void; -} - -export type TerminateProcessTreeResult = - | "already-exited" - | "terminated" - | "killed" - | "kill-timeout"; - -export async function terminateProcessTree( - child: ProcessTreeTarget, - options: TerminateProcessTreeOptions, -): Promise { - if (isProcessExited(child)) { - return "already-exited"; - } - - const exitPromise = waitForProcessExit(child); - signalProcessTree(child, options.gracefulSignal ?? "SIGTERM", options); - if (await waitForExitOrTimeout(exitPromise, options.gracefulTimeoutMs)) { - return "terminated"; - } - - options.onForceSignal?.(); - signalProcessTree(child, options.forceSignal ?? "SIGKILL", options); - if (options.forceTimeoutMs === undefined) { - return "killed"; - } - return (await waitForExitOrTimeout(exitPromise, options.forceTimeoutMs)) - ? "killed" - : "kill-timeout"; -} - -export function signalProcessTree( - child: ProcessTreeTarget, - signal: NodeJS.Signals, - options: SignalProcessTreeOptions = {}, -): void { - if (isProcessExited(child)) { - return; - } - - const pid = child.pid; - if (typeof pid === "number" && pid > 0) { - const platform = options.platform ?? process.platform; - if (platform === "win32") { - signalWindowsProcessTree(child, pid, signal, options); - return; - } - - try { - (options.kill ?? process.kill.bind(process))(-pid, signal); - return; - } catch { - // Fall back to the direct child when no separate process group exists. - } - } - - signalDirectChild(child, signal); -} - -function signalWindowsProcessTree( - child: ProcessTreeTarget, - pid: number, - signal: NodeJS.Signals, - options: SignalProcessTreeOptions, -): void { - const args = ["/pid", String(pid), "/T"]; - if (signal === "SIGKILL") { - args.push("/F"); - } - - try { - (options.execFile ?? execFile)("taskkill", args, (error) => { - if (error) { - signalDirectChild(child, signal); - } - }); - } catch { - signalDirectChild(child, signal); - } -} - -function signalDirectChild(child: ProcessTreeTarget, signal: NodeJS.Signals): void { - try { - child.kill(signal); - } catch { - // Ignore cleanup races. - } -} - -function isProcessExited(child: ProcessTreeTarget): boolean { - return ( - (child.exitCode !== null && child.exitCode !== undefined) || - (child.signalCode !== null && child.signalCode !== undefined) - ); -} - -function waitForProcessExit(child: ProcessTreeTarget): Promise { - if (isProcessExited(child)) { - return Promise.resolve(); - } - if (!child.once) { - return new Promise(() => undefined); - } - - return new Promise((resolve) => { - child.once?.("exit", resolve); - }); -} - -async function waitForExitOrTimeout( - exitPromise: Promise, - timeoutMs: number, -): Promise { - let timer: NodeJS.Timeout | null = null; - try { - return await Promise.race([ - exitPromise.then(() => true), - new Promise((resolve) => { - timer = setTimeout(() => resolve(false), timeoutMs); - }), - ]); - } finally { - if (timer) { - clearTimeout(timer); - } - } -} diff --git a/packages/server/src/utils/tree-kill.test.ts b/packages/server/src/utils/tree-kill.test.ts new file mode 100644 index 000000000..79881448f --- /dev/null +++ b/packages/server/src/utils/tree-kill.test.ts @@ -0,0 +1,184 @@ +import { spawn, type ChildProcess } from "node:child_process"; +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, test } from "vitest"; +import { terminateWithTreeKill } from "./tree-kill.js"; + +const pollIntervalMs = 50; + +let tempDir: string | null = null; +let ownerProcess: ChildProcess | null = null; +let descendantPid: number | null = null; + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +function isProcessRunning(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) { + return false; + } + + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +async function waitFor( + check: () => Promise | boolean, + timeoutMs: number, + message: string, +): Promise { + const deadline = Date.now() + timeoutMs; + async function poll(): Promise { + if (await check()) return; + if (Date.now() >= deadline) throw new Error(message); + await sleep(pollIntervalMs); + return poll(); + } + return poll(); +} + +async function readPidFileNumber(filePath: string): Promise { + try { + const raw = (await readFile(filePath, "utf-8")).trim(); + const pid = Number.parseInt(raw, 10); + return Number.isInteger(pid) && pid > 0 ? pid : null; + } catch { + return null; + } +} + +function killIfRunning(pid: number | null | undefined): void { + if (!pid || !isProcessRunning(pid)) return; + try { + process.kill(pid, "SIGKILL"); + } catch { + // Ignore cleanup races. + } +} + +function spawnOwnerWithDescendant(options: { + childPidPath: string; + detachedDescendant: boolean; +}): ChildProcess { + const descendantOptions = options.detachedDescendant + ? '{ detached: true, stdio: "ignore" }' + : '{ stdio: "ignore" }'; + const childUnref = options.detachedDescendant ? "child.unref();" : ""; + + return spawn( + process.execPath, + [ + "-e", + ` + const { spawn } = require("node:child_process"); + process.on("SIGTERM", () => {}); + const child = spawn(process.execPath, [ + "-e", + ${JSON.stringify(` + const fs = require("node:fs"); + process.on("SIGTERM", () => {}); + fs.writeFileSync(${JSON.stringify(options.childPidPath)}, String(process.pid)); + setInterval(() => {}, 1000); + `)} + ], ${descendantOptions}); + ${childUnref} + setInterval(() => {}, 1000); + `, + ], + { stdio: "ignore" }, + ); +} + +async function waitForFixtureReady(childPidPath: string): Promise { + await waitFor( + async () => { + descendantPid = await readPidFileNumber(childPidPath); + return ( + isProcessRunning(ownerProcess?.pid ?? -1) && + descendantPid !== null && + isProcessRunning(descendantPid) + ); + }, + 5000, + "owner descendant did not become running in time", + ); +} + +async function expectOwnerAndDescendantStopped(message: string): Promise { + await waitFor( + () => !isProcessRunning(ownerProcess?.pid ?? -1) && !isProcessRunning(descendantPid ?? -1), + 5000, + message, + ); +} + +afterEach(async () => { + killIfRunning(ownerProcess?.pid); + killIfRunning(descendantPid); + ownerProcess = null; + descendantPid = null; + + if (tempDir) { + await rm(tempDir, { recursive: true, force: true }); + tempDir = null; + } +}); + +describe("terminateWithTreeKill", () => { + test.runIf(process.platform === "win32")( + "kills Windows descendants through taskkill tree cleanup", + async () => { + tempDir = await mkdtemp(join(tmpdir(), "paseo-server-tree-kill-")); + const childPidPath = join(tempDir, "descendant.pid"); + + ownerProcess = spawnOwnerWithDescendant({ + childPidPath, + detachedDescendant: false, + }); + expect(ownerProcess.pid).toBeTypeOf("number"); + await waitForFixtureReady(childPidPath); + + const result = await terminateWithTreeKill(ownerProcess, { + gracefulTimeoutMs: 2000, + forceTimeoutMs: 2000, + }); + + // tree-kill uses taskkill /T /F on Windows, so the first signal is already forceful. + expect(result).toBe("terminated"); + await expectOwnerAndDescendantStopped( + "owner or Windows descendant survived terminateWithTreeKill", + ); + }, + ); + + test.runIf(process.platform !== "win32")( + "force-kills descendants that started their own process group", + async () => { + tempDir = await mkdtemp(join(tmpdir(), "paseo-server-tree-kill-")); + const childPidPath = join(tempDir, "descendant.pid"); + + ownerProcess = spawnOwnerWithDescendant({ + childPidPath, + detachedDescendant: true, + }); + expect(ownerProcess.pid).toBeTypeOf("number"); + await waitForFixtureReady(childPidPath); + + const result = await terminateWithTreeKill(ownerProcess, { + gracefulTimeoutMs: 100, + forceTimeoutMs: 2000, + }); + + expect(result).toBe("killed"); + await expectOwnerAndDescendantStopped( + "owner or separate-process-group descendant survived terminateWithTreeKill", + ); + }, + ); +}); diff --git a/packages/server/src/utils/tree-kill.ts b/packages/server/src/utils/tree-kill.ts new file mode 100644 index 000000000..255e3b1c2 --- /dev/null +++ b/packages/server/src/utils/tree-kill.ts @@ -0,0 +1,115 @@ +import treeKill from "tree-kill"; + +export interface TreeKillTarget { + pid?: number; + exitCode?: number | null; + signalCode?: NodeJS.Signals | null; + kill(signal?: NodeJS.Signals | number): boolean; + once?(event: "exit", listener: () => void): unknown; +} + +interface TerminateWithTreeKillOptions { + gracefulSignal?: NodeJS.Signals; + forceSignal?: NodeJS.Signals; + gracefulTimeoutMs: number; + forceTimeoutMs?: number; + onForceSignal?: () => void; +} + +export type TerminateWithTreeKillResult = + | "already-exited" + | "terminated" + | "killed" + | "kill-timeout"; + +export async function terminateWithTreeKill( + child: TreeKillTarget, + options: TerminateWithTreeKillOptions, +): Promise { + if (isProcessExited(child)) { + return "already-exited"; + } + + const exitPromise = waitForProcessExit(child); + await signalTreeOrChild(child, options.gracefulSignal ?? "SIGTERM"); + if (await waitForExitOrTimeout(exitPromise, options.gracefulTimeoutMs)) { + return "terminated"; + } + + options.onForceSignal?.(); + await signalTreeOrChild(child, options.forceSignal ?? "SIGKILL"); + if (options.forceTimeoutMs === undefined) { + return "killed"; + } + return (await waitForExitOrTimeout(exitPromise, options.forceTimeoutMs)) + ? "killed" + : "kill-timeout"; +} + +function signalTreeOrChild(child: TreeKillTarget, signal: NodeJS.Signals): Promise { + if (isProcessExited(child)) { + return Promise.resolve(); + } + + const pid = child.pid; + if (typeof pid !== "number" || pid <= 0) { + signalDirectChild(child, signal); + return Promise.resolve(); + } + + return new Promise((resolve) => { + treeKill(pid, signal, (error) => { + if (error) { + signalDirectChild(child, signal); + } + resolve(); + }); + }); +} + +function signalDirectChild(child: TreeKillTarget, signal: NodeJS.Signals): void { + try { + child.kill(signal); + } catch { + // Ignore cleanup races. + } +} + +function isProcessExited(child: TreeKillTarget): boolean { + return ( + (child.exitCode !== null && child.exitCode !== undefined) || + (child.signalCode !== null && child.signalCode !== undefined) + ); +} + +function waitForProcessExit(child: TreeKillTarget): Promise { + if (isProcessExited(child)) { + return Promise.resolve(); + } + if (!child.once) { + return new Promise(() => undefined); + } + + return new Promise((resolve) => { + child.once?.("exit", resolve); + }); +} + +async function waitForExitOrTimeout( + exitPromise: Promise, + timeoutMs: number, +): Promise { + let timer: NodeJS.Timeout | null = null; + try { + return await Promise.race([ + exitPromise.then(() => true), + new Promise((resolve) => { + timer = setTimeout(() => resolve(false), timeoutMs); + }), + ]); + } finally { + if (timer) { + clearTimeout(timer); + } + } +}