From a5db5cdda823199e54cab6991e17e85ccaf23c9a Mon Sep 17 00:00:00 2001 From: Arnaud Ritti <77437157+arnaud-ritti@users.noreply.github.com> Date: Sat, 7 Nov 2026 23:39:34 +0100 Subject: [PATCH] fix(smoke): allow graceful server shutdown before forced cleanup (#2054) The smoke harness killed the server's process group right after closing its stdin, so the host never got to shut down and tool processes it had spawned in their own groups could be left running. Close now waits for the server's graceful EOF shutdown (bounded at 15 s), then kills the server's own group and every independent tool group it owned, reporting group-kill errors only after cleaning the rest. Covered by `smoke/server.test.ts` (graceful, failed initialize, hung shutdown, signal exit, spawn failure, missing `ps`, coalesced frames, kill errors). Co-authored-by: arnaud-ritti <77437157+arnaud-ritti@users.noreply.github.com> --- smoke/fixtures/server.cjs | 49 ++++++++++ smoke/server.test.ts | 152 +++++++++++++++++++++++++++++ smoke/server.ts | 198 ++++++++++++++++++++++++++++++-------- 3 files changed, 361 insertions(+), 38 deletions(-) create mode 100755 smoke/fixtures/server.cjs create mode 100644 smoke/server.test.ts diff --git a/smoke/fixtures/server.cjs b/smoke/fixtures/server.cjs new file mode 100755 index 000000000..dbb70eb3a --- /dev/null +++ b/smoke/fixtures/server.cjs @@ -0,0 +1,49 @@ +#!/usr/bin/env node +const { spawn } = require("node:child_process"); +const { writeFileSync } = require("node:fs"); +const { join } = require("node:path"); +const home = process.env.OPENIDEA_SERVER_HOME; +const mode = process.env.SMOKE_SERVER_MODE; +const tool = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + detached: mode !== "hang-inherited", + stdio: "ignore", +}); +const second = + mode === "signal-multiple" + ? spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { + detached: true, + stdio: "ignore", + }) + : undefined; +writeFileSync(join(home, "pids.json"), JSON.stringify({ server: process.pid, tool: tool.pid, second: second?.pid })); +let buffer = Buffer.alloc(0); +process.stdin.on("data", (chunk) => { + buffer = Buffer.concat([buffer, chunk]); + for (;;) { + const end = buffer.indexOf("\r\n\r\n"); + if (end < 0) return; + const length = Number(/Content-Length: (\d+)/i.exec(buffer.subarray(0, end).toString())[1]); + if (buffer.length < end + 4 + length) return; + const request = JSON.parse(buffer.subarray(end + 4, end + 4 + length)); + buffer = buffer.subarray(end + 4 + length); + const body = JSON.stringify({ + jsonrpc: "2.0", + id: request.id, + ...(mode === "reject" ? { error: { message: "initialize failed" } } : { result: {} }), + }); + process.stdout.write(`Content-Length: ${Buffer.byteLength(body)}\r\n\r\n${body}`); + } +}); +process.stdin.on("end", () => { + writeFileSync(join(home, "eof"), "received"); + if (mode.startsWith("hang")) return; + if (mode.startsWith("signal")) return process.kill(process.pid, "SIGTERM"); + setTimeout(() => { + tool.once("exit", () => { + writeFileSync(join(home, "shutdown"), "tool reaped"); + process.exit(0); + }); + tool.kill("SIGKILL"); + }, 150); +}); +setInterval(() => {}, 1000); diff --git a/smoke/server.test.ts b/smoke/server.test.ts new file mode 100644 index 000000000..c7344639d --- /dev/null +++ b/smoke/server.test.ts @@ -0,0 +1,152 @@ +// @vitest-environment node +import assert from "node:assert/strict"; +import { mkdtempSync, readFileSync, existsSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test, onTestFinished, vi } from "vitest"; +import { spawn } from "node:child_process"; +import { once } from "node:events"; +import { Server, repoRoot } from "./server.ts"; + +// The executable fixture uses a POSIX shebang. Windows teardown uses taskkill. +const options = { skip: process.platform === "win32", timeout: 30_000 }; + +async function fixture(mode = "graceful") { + const home = mkdtempSync(join(tmpdir(), "openidea-server-close-")); + const previous = process.env.OPENIDEA_SERVER; + process.env.OPENIDEA_SERVER = join(repoRoot, "smoke", "fixtures", "server.cjs"); + onTestFinished(async () => { + if (previous === undefined) delete process.env.OPENIDEA_SERVER; + else process.env.OPENIDEA_SERVER = previous; + if (existsSync(join(home, "pids.json"))) { + const pids = JSON.parse(readFileSync(join(home, "pids.json"), "utf8")); + for (const pid of Object.values(pids) as number[]) { + for (const target of [pid]) { + try { + process.kill(target, "SIGKILL"); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESRCH") throw error; + } + } + } + // Reap/observe every owned process before removing its home, even on failure. + await vi.waitFor( + () => { + for (const pid of Object.values(pids) as number[]) assertGone(pid); + }, + { timeout: 3000 }, + ); + } + rmSync(home, { recursive: true, force: true }); + }); + return { home, start: () => Server.start(home, { SMOKE_SERVER_MODE: mode }) }; +} + +function assertGone(pid: number) { + assert.throws(() => process.kill(pid, 0), { code: "ESRCH" }); +} + +test("close waits for graceful EOF cleanup of a tool in its own group", options, async () => { + const { home, start } = await fixture(); + const server = await start(); + const pids = JSON.parse(readFileSync(join(home, "pids.json"), "utf8")); + assert.deepEqual(await Promise.all([server.invoke("first", {}), server.invoke("second", {})]), [{}, {}]); + await Promise.all([server.close(), server.close()]); + assert.equal(readFileSync(join(home, "shutdown"), "utf8"), "tool reaped"); + assertGone(pids.server); + assertGone(pids.tool); + await server.close(); +}); + +test("failed initialize still runs graceful cleanup and preserves the error", options, async () => { + const { home, start } = await fixture("reject"); + await assert.rejects(start(), { message: "initialize failed" }); + assert.equal(readFileSync(join(home, "shutdown"), "utf8"), "tool reaped"); +}); + +test("hung EOF shutdown is bounded and kills independent tool groups", options, async () => { + const { home, start } = await fixture("hang"); + const server = await start(); + const pids = JSON.parse(readFileSync(join(home, "pids.json"), "utf8")); + const started = Date.now(); + await server.close(); + assert.ok(Date.now() - started >= 15_000); + assert.ok(Date.now() - started < 25_000); + assert.ok(existsSync(join(home, "eof"))); + assertGone(pids.server); + assertGone(pids.tool); +}); + +test("signal exit after EOF also cleans up independent tool groups", options, async () => { + const { home, start } = await fixture("signal"); + const server = await start(); + const pids = JSON.parse(readFileSync(join(home, "pids.json"), "utf8")); + await server.close(); + assertGone(pids.server); + assertGone(pids.tool); +}); + +test("spawn failure rejects without waiting for an exit event", options, async () => { + const { home } = await fixture(); + process.env.OPENIDEA_SERVER = join(home, "missing-server"); + await assert.rejects(Server.start(home), { code: "ENOENT" }); +}); + +test("missing ps preserves forced cleanup of the server's inherited group", options, async () => { + const { home, start } = await fixture("hang-inherited"); + const server = await start(); + const pids = JSON.parse(readFileSync(join(home, "pids.json"), "utf8")); + const previous = process.env.PATH; + process.env.PATH = home; + try { + await server.close(); + } finally { + if (previous === undefined) delete process.env.PATH; + else process.env.PATH = previous; + } + assertGone(pids.server); + assertGone(pids.tool); +}); + +test("fixture consumes multiple requests including coalesced frames", options, async () => { + const { home } = await fixture(); + const child = spawn(process.env.OPENIDEA_SERVER!, [], { + env: { ...process.env, OPENIDEA_SERVER_HOME: home, SMOKE_SERVER_MODE: "graceful" }, + detached: true, + }); + const exited = once(child, "exit"); + let output = ""; + child.stdout.on("data", (chunk) => { + output += chunk.toString(); + }); + const frame = (id: number) => { + const body = JSON.stringify({ jsonrpc: "2.0", id, method: "initialize" }); + return `Content-Length: ${Buffer.byteLength(body)}\r\n\r\n${body}`; + }; + // A single write contains two frames and part of the third header. + const third = frame(3); + child.stdin.write(frame(1) + frame(2) + third.slice(0, 10)); + await new Promise((resolve) => setTimeout(resolve, 50)); + child.stdin.end(third.slice(10)); + await exited; + const ids = [...output.matchAll(/"id":(\d+)/g)].map((match) => Number(match[1])); + assert.deepEqual(ids, [1, 2, 3]); +}); + +test("group-kill errors are reported after cleaning the remaining owned groups", options, async () => { + const { home, start } = await fixture("signal-multiple"); + const server = await start(); + const pids = JSON.parse(readFileSync(join(home, "pids.json"), "utf8")); + const kill = process.kill.bind(process); + const denied = Object.assign(new Error("group kill denied"), { code: "EPERM" }); + const spy = vi.spyOn(process, "kill").mockImplementation((pid, signal) => { + if (pid === -pids.tool && signal === "SIGKILL") throw denied; + return kill(pid, signal); + }); + try { + await assert.rejects(server.close(), denied); + assertGone(pids.second); + } finally { + spy.mockRestore(); + } +}); diff --git a/smoke/server.ts b/smoke/server.ts index aa867c127..9a7e66ed9 100644 --- a/smoke/server.ts +++ b/smoke/server.ts @@ -8,6 +8,8 @@ import { dirname, join } from "node:path"; const PROTOCOL_VERSION = 2; const WINDOW = "main"; +// Allow the server's ten-second host shutdown plus scheduling and I/O overhead. +const SHUTDOWN_TIMEOUT_MS = 15_000; // Not `new URL(…, import.meta.url)`: the jsdom environment's `URL` isn't Node's. export const repoRoot = join(dirname(import.meta.filename), ".."); @@ -27,24 +29,87 @@ interface Pending { function treeKill(pid: number | undefined): Promise { return new Promise((resolve) => { if (pid === undefined) return resolve(); - execFile("taskkill", ["/pid", String(pid), "/T", "/F"], () => resolve()); + execFile("taskkill", ["/pid", String(pid), "/T", "/F"], { timeout: 5000 }, () => resolve()); }); } -/** Resolves once no process group `pid` exists anymore (signal 0 only checks). */ +/** Poll for group/leader disappearance until timeoutMs elapses. */ async function waitForGroupExit(pid: number, timeoutMs = 5000): Promise { const start = Date.now(); for (;;) { - try { - process.kill(-pid, 0); - } catch { - return; - } - if (Date.now() - start > timeoutMs) return; + const alive = [pid, -pid].some((target) => { + try { + process.kill(target, 0); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESRCH") throw error; + return false; + } + }); + if (!alive || Date.now() - start > timeoutMs) return; await new Promise((r) => setTimeout(r, 50)); } } +interface ProcessInfo { + pid: number; + parent: number; + group: number; + started: string; +} + +function processTable(): Promise { + return new Promise((resolve) => { + execFile("ps", ["-axo", "pid=,ppid=,pgid=,lstart="], { timeout: 1000 }, (error, stdout) => { + if (error) return resolve([]); + const rows: ProcessInfo[] = []; + for (const line of stdout.split("\n")) { + const match = /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(.+)$/.exec(line); + if (match) + rows.push({ pid: Number(match[1]), parent: Number(match[2]), group: Number(match[3]), started: match[4]! }); + } + resolve(rows); + }); + }); +} + +/** Snapshot owned group members before EOF can orphan them. */ +async function ownedProcesses(pid: number): Promise { + const rows = await processTable(); + const descendants = new Set([pid]); + for (;;) { + const size = descendants.size; + for (const row of rows) { + if (descendants.has(row.parent)) descendants.add(row.pid); + } + if (size === descendants.size) break; + } + return rows.filter((row) => descendants.has(row.pid) && descendants.has(row.group)); +} + +/** Revalidate identities so a group id reused during shutdown is never signalled. */ +async function killOwnedGroups(owned: ProcessInfo[], errors: Set): Promise { + const current = await processTable(); + const identities = new Map(owned.map((row) => [row.pid, row])); + const groups = new Set(); + for (const row of current) { + const old = identities.get(row.pid); + if (old?.group === row.group && old.started === row.started) { + groups.add(row.group); + } + } + const killed: number[] = []; + for (const group of groups) { + try { + process.kill(-group, "SIGKILL"); + killed.push(group); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESRCH") errors.add(error); + } + } + return killed; +} + export class Server { private nextId = 1; private readonly pending = new Map(); @@ -55,7 +120,11 @@ export class Server { /** `event` notifications: an `IdeEvent` for a window (`null`: every window). */ onEvent: (window: string | null, event: unknown) => void = () => {}; - private constructor(private readonly child: ChildProcessWithoutNullStreams) { + private closing: Promise | undefined; + private readonly child: ChildProcessWithoutNullStreams; + + private constructor(child: ChildProcessWithoutNullStreams) { + this.child = child; child.stdout.on("data", (chunk: Buffer) => this.receive(chunk)); child.stderr.on("data", (chunk: Buffer) => { // The whole log, when asked for (OPENIDEA_SMOKE_LOG=; OPENIDEA_LOG sets its level). @@ -63,6 +132,15 @@ export class Server { this.stderr.push(chunk.toString()); if (this.stderr.length > 200) this.stderr.shift(); }); + child.on("error", (error) => { + for (const p of this.pending.values()) p.reject(error); + this.pending.clear(); + }); + // A process exiting during a request can close its stdin before the exit event. + child.stdin.on("error", (error) => { + for (const p of this.pending.values()) p.reject(error); + this.pending.clear(); + }); child.on("exit", (code) => { for (const p of this.pending.values()) p.reject(new Error(`openidea-server exited (${code}): ${this.log()}`)); this.pending.clear(); @@ -74,13 +152,17 @@ export class Server { const child = spawn(serverBinary(), ["--stdio"], { env: { ...process.env, OPENIDEA_SERVER_HOME: home, ...env }, stdio: ["pipe", "pipe", "pipe"], - // Its own process group: the language servers it starts (jdtls's JVM) stay in - // it, so `close` can reap them together with the server (see below). + // A dedicated group for forced cleanup; LSPs may create groups of their own. detached: true, }); const server = new Server(child); - await server.request("initialize", { version: PROTOCOL_VERSION }); - return server; + try { + await server.request("initialize", { version: PROTOCOL_VERSION }); + return server; + } catch (error) { + await server.close(); + throw error; + } } /** The tail of the server's log, for failure messages. */ @@ -103,36 +185,76 @@ export class Server { } } - /** - * Kills the server and everything it started; resolves once they're all gone. - * The server alone is not enough: a language server's own process (jdtls's JVM) - * outlives it and keeps writing under the test's home directory, so deleting that - * directory afterwards raced its writes (`ENOTEMPTY` on teardown). The server runs - * detached (its own process group), so the group is only it and its children. - */ - async close(): Promise { - if (this.child.exitCode === null && !this.child.killed) { + /** Let EOF shut down and reap tools before falling back to forced cleanup. */ + close(): Promise { + return (this.closing ??= this.shutdown()); + } + + private async shutdown(): Promise { + const pid = this.child.pid; + const owned = process.platform !== "win32" && pid !== undefined ? await ownedProcesses(pid) : []; + const killedGroups = new Set(); + const errors = new Set(); + const running = () => pid !== undefined && this.child.exitCode === null && this.child.signalCode === null; + if (running()) { + let timer: ReturnType | undefined; const exited = new Promise((resolve) => this.child.once("exit", () => resolve())); this.child.stdin.end(); - if (process.platform === "win32") { - // `child.kill()` stops only the server; take the whole tree while it is - // still enumerable, then wait for the exit below. - await treeKill(this.child.pid); - } else { - this.child.kill(); - } - await exited; - } - if (process.platform !== "win32" && this.child.pid !== undefined) { - // Reap what the server started: `SIGKILL` to the group, then wait until it is - // gone (termination is asynchronous; files held open can't be deleted meanwhile). try { - process.kill(-this.child.pid, "SIGKILL"); - } catch { - // Nothing left running. + await Promise.race([ + exited, + new Promise((resolve) => { + timer = setTimeout(resolve, SHUTDOWN_TIMEOUT_MS); + }), + ]); + } finally { + clearTimeout(timer); + } + if (running()) { + if (process.platform === "win32") { + // Enumerate the tree while its root is still alive. + await treeKill(pid); + } else { + if (pid !== undefined) owned.push(...(await ownedProcesses(pid))); + for (const group of await killOwnedGroups(owned, errors)) killedGroups.add(group); + } + // While the handle is running, its dedicated group cannot have been reused. + // Preserve inherited-group cleanup even when ps is unavailable. + if (running()) { + if (process.platform !== "win32" && pid !== undefined && !killedGroups.has(pid)) { + try { + process.kill(-pid, "SIGKILL"); + killedGroups.add(pid); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESRCH") errors.add(error); + } + } + try { + this.child.kill("SIGKILL"); + } catch (error) { + errors.add(error); + } + } + try { + await Promise.race([ + exited, + new Promise((resolve) => { + timer = setTimeout(resolve, 5000); + }), + ]); + if (running()) errors.add(new Error(`openidea-server (${pid}) did not exit after forced cleanup`)); + } finally { + clearTimeout(timer); + } } - await waitForGroupExit(this.child.pid); } + if (process.platform !== "win32") { + for (const group of await killOwnedGroups(owned, errors)) killedGroups.add(group); + } + const waits = await Promise.allSettled([...killedGroups].map((group) => waitForGroupExit(group))); + for (const result of waits) if (result.status === "rejected") errors.add(result.reason); + if (errors.size === 1) throw [...errors][0]; + if (errors.size > 1) throw new AggregateError(errors, "Smoke server cleanup failed"); } private request(method: string, params: unknown): Promise {