diff --git a/tests/runner-e2e/README.md b/tests/runner-e2e/README.md index 9ad8ead828..adbd7496a8 100644 --- a/tests/runner-e2e/README.md +++ b/tests/runner-e2e/README.md @@ -1092,9 +1092,18 @@ alive to stop the embedded database after the replacement starts. On Unix, the server has its own process group so Playwright's wrapper-group shutdown cannot signal it independently. Signal handling, restart cleanup, and final cleanup share one stop operation per server: one graceful signal, a 30-second wait, then -SIGKILL to the owned server group and a 5-second exit wait if needed. Windows -retains direct-child signaling. This prevents a second graceful signal from -interrupting asynchronous warm-session retirement. +SIGKILL to revalidated owned groups and a 5-second exit wait if needed. The +wrapper retains observed descendants across an early server exit and checks +PID/start identity before group signals. A failed cleanup can resume its saved +phase without another graceful signal. Windows retains direct-child signaling. +This prevents a second graceful signal from interrupting asynchronous warm-session +retirement. + +Launcher cancellation first signals its outer group, so Playwright and the wrapper +own graceful server shutdown. It allows 45 seconds for that chain before bounded +5-second forced cleanup of revalidated descendants. Normal launcher exit uses the +same retained descendant inventory. Zombies count as stopped; an unavailable or +uncertain identity inspection fails cleanup instead of granting signal authority. Everyday restart and Stop scenarios exempt only their recorded cancellation, graceful-shutdown interruption, or process-loss outcome. A later adapter error diff --git a/tests/runner-e2e/launch.ts b/tests/runner-e2e/launch.ts index 6017360ee8..ea74bf8aea 100644 --- a/tests/runner-e2e/launch.ts +++ b/tests/runner-e2e/launch.ts @@ -1,3 +1,4 @@ +import { createProcessTreeOwner, stopOwnedProcessTree } from "./process-tree-owner.js"; import { randomBytes } from "node:crypto"; import { prepareCodexCiSandbox, requiresCodexCiSandbox } from "./codex-ci-sandbox.js"; import { spawn } from "node:child_process"; @@ -61,11 +62,8 @@ import { } from "./result-exit-guard.js"; import { observeDescendantProcessTree, - refreshContinuouslyLiveProcessGroups, - revalidateObservedProcessGroups, - safeProcessGroupTerminationOrder, type ObservedProcessGroup, - type ProcessObservation, + readProcessTable, } from "./process-tree.js"; const repositoryRoot = path.resolve(import.meta.dirname, "../.."); @@ -75,19 +73,6 @@ const activeProcessGroups = new Set(); const activeProcessCleanup = new Map>(); const activeProcessTerminators = new Map void>(); const completedResultExitGraceMs = 120_000; -const diagnosticProcessKinds = new Set([ - "bash", - "chrome", - "codex", - "google-chrome", - "node", - "paperclip-runnerd", - "playwright", - "pnpm", - "postgres", - "sh", - "tsx", -]); let cancelled = false; function cleanId(value: string) { @@ -161,72 +146,6 @@ interface ProcessTreeDiagnostic { groups: ObservedProcessGroup[]; } -function observedGroupsSelectedForTermination( - groups: readonly ObservedProcessGroup[], - processGroupIds: readonly number[], -) { - const selected = new Set(processGroupIds); - return groups.filter((group) => selected.has(group.processGroupId)); -} - -async function readProcessTable(): Promise { - if (process.platform === "win32") { - return null; - } - return await new Promise((resolve) => { - const inspector = spawn( - "ps", - ["-e", "-o", "pid=,ppid=,pgid=,lstart=,comm="], - { - env: { PATH: "/usr/bin:/bin", LANG: "C", LC_ALL: "C" }, - stdio: ["ignore", "pipe", "ignore"], - }, - ); - let output = ""; - let settled = false; - const finish = (value: ProcessObservation[] | null) => { - if (settled) return; - settled = true; - clearTimeout(inspectionTimeout); - resolve(value); - }; - const inspectionTimeout = setTimeout(() => { - inspector.kill("SIGKILL"); - finish(null); - }, 5_000); - inspectionTimeout.unref(); - inspector.stdout?.setEncoding("utf8").on("data", (chunk: string) => { - output = `${output}${chunk}`.slice(-1024 * 1024); - }); - inspector.once("error", () => finish(null)); - inspector.once("close", () => { - const observations = output - .split(/\r?\n/) - .map((line) => - /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(\S+\s+\S+\s+\d+\s+\d{2}:\d{2}:\d{2}\s+\d{4})\s+(.+?)\s*$/.exec( - line, - ), - ) - .filter((match): match is RegExpExecArray => match !== null) - .map((match): ProcessObservation => { - // A target process can choose its own argv and process name. Emit a - // fixed category instead of target-controlled text so diagnostics - // can never turn that metadata into a secret-exfiltration channel. - const command = path.basename(match[5]!); - const kind = diagnosticProcessKinds.has(command) ? command : "other"; - return { - pid: Number(match[1]), - parentPid: Number(match[2]), - processGroupId: Number(match[3]), - started: match[4]!, - kind, - }; - }); - finish(observations); - }); - }).catch(() => null); -} - async function processTreeDiagnostic( rootPid: number, ): Promise { @@ -353,6 +272,7 @@ async function runProcess( }); if (!child.pid) throw new Error("Failed to start Playwright"); activeProcessGroups.add(child.pid); + const processOwner = createProcessTreeOwner(child); let outputTail = ""; const recordOutput = (chunk: Buffer, destination: NodeJS.WriteStream) => { destination.write(chunk); @@ -370,90 +290,11 @@ async function runProcess( let childSettled = false; let postResultStallError: string | null = null; let boundedCleanup: Promise | undefined; - const forceStopDirectChild = () => { - if (!childSettled && child.exitCode === null && child.signalCode === null) { - child.kill("SIGKILL"); - } - }; - const stopChildTree = (diagnostic?: ProcessTreeDiagnostic) => { + const stopChildTree = (_diagnostic?: ProcessTreeDiagnostic) => { if (boundedCleanup) return; - const rootProcessGroupId = child.pid!; - boundedCleanup = (async () => { - const snapshot = diagnostic ?? (await processTreeDiagnostic(child.pid!)); - const validationTable = await readProcessTable(); - const currentProcessGroupId = validationTable - ? (validationTable.find((candidate) => candidate.pid === process.pid) - ?.processGroupId ?? null) - : null; - const observedGroups = validationTable - ? revalidateObservedProcessGroups(snapshot.groups, validationTable) - : []; - const terminationOrder = safeProcessGroupTerminationOrder({ - rootProcessGroupId, - currentProcessGroupId, - groups: observedGroups, - }); - const verifiedGroups = observedGroupsSelectedForTermination( - observedGroups, - terminationOrder, - ); - if (verifiedGroups.length === 0) { - forceStopDirectChild(); - return "Could not revalidate an owned process group before cleanup"; - } - for (const processGroupId of terminationOrder) { - stopProcessGroup(processGroupId, "SIGTERM"); - } - - let remainingGroups = verifiedGroups; - const gracefulDeadline = Date.now() + 5_000; - while (remainingGroups.length > 0 && Date.now() < gracefulDeadline) { - const table = await readProcessTable(); - if (table) { - remainingGroups = refreshContinuouslyLiveProcessGroups( - remainingGroups, - table, - ); - } - if (remainingGroups.length > 0) await wait(50); - } - const remainingGroupIds = new Set( - remainingGroups.map((group) => group.processGroupId), - ); - const forcedOrder = safeProcessGroupTerminationOrder({ - rootProcessGroupId, - currentProcessGroupId, - groups: remainingGroups, - }).filter((processGroupId) => remainingGroupIds.has(processGroupId)); - for (const processGroupId of forcedOrder) { - stopProcessGroup(processGroupId, "SIGKILL"); - } - - const forcedDeadline = Date.now() + 5_000; - let forcedVerificationUnavailable = false; - while (Date.now() < forcedDeadline) { - const table = await readProcessTable(); - if (!table) { - forcedVerificationUnavailable = true; - await wait(50); - continue; - } - forcedVerificationUnavailable = false; - remainingGroups = refreshContinuouslyLiveProcessGroups( - remainingGroups, - table, - ); - if (remainingGroups.length === 0) return null; - await wait(50); - } - if (forcedVerificationUnavailable) { - return "Could not verify descendant process exit after SIGKILL"; - } - return `Verified descendant process groups ${remainingGroups - .map((group) => group.processGroupId) - .join(",")} survived SIGKILL`; - })(); - activeProcessCleanup.set(rootProcessGroupId, boundedCleanup); + boundedCleanup = stopOwnedProcessTree(child, processOwner) + .then(() => null, error => error instanceof Error ? error.message : String(error)); + activeProcessCleanup.set(child.pid!, boundedCleanup); }; activeProcessTerminators.set(child.pid, stopChildTree); const resultExitGuard = createResultExitGuard({ @@ -517,11 +358,11 @@ async function runProcess( }); if (timer) clearTimeout(timer); if (completionPoll) clearInterval(completionPoll); - const processCleanupError = child.pid - ? await (boundedCleanup ?? - activeProcessCleanup.get(child.pid) ?? - terminateProcessGroup(child.pid)) - : null; + // Even a successful launcher exit can leave an already-observed detached + // server behind. Retire that retained tree, never only the root group. + stopChildTree(); + const processCleanupError = await boundedCleanup!; + processOwner.stopObserving(); if (child.pid) { activeProcessGroups.delete(child.pid); activeProcessCleanup.delete(child.pid); diff --git a/tests/runner-e2e/process-tree-owner.test.ts b/tests/runner-e2e/process-tree-owner.test.ts new file mode 100644 index 0000000000..a5b9849674 --- /dev/null +++ b/tests/runner-e2e/process-tree-owner.test.ts @@ -0,0 +1,66 @@ +import type { ChildProcess } from "node:child_process"; +import { expect, it } from "vitest"; +import { createProcessTreeOwner } from "./process-tree-owner.js"; +import type { ProcessObservation } from "./process-tree.js"; + +function row(pid: number, parentPid: number, processGroupId: number, started = `start-${pid}`, state = "S"): ProcessObservation { + return { pid, parentPid, processGroupId, started, state, kind: "node" }; +} +function fixture(initial: ProcessObservation[]) { + let table = [row(process.pid, 1, 10), ...initial]; + const root = { pid: 100, exitCode: null as number | null, signalCode: null as NodeJS.Signals | null }; + const signals: Array<[number, NodeJS.Signals]> = []; + const owner = createProcessTreeOwner(root as ChildProcess, { + readTable: async () => table, + signalGroup: (pid, signal) => { signals.push([pid, signal]); }, + }); + return { root, owner, signals, replace: (rows: ProcessObservation[]) => { table = [row(process.pid, 1, 10), ...rows]; } }; +} + +it.skipIf(process.platform === "win32")("revalidates PID/start before signaling and refuses a recycled group", async () => { + const f = fixture([row(100, process.pid, 100), row(200, 100, 200)]); + try { + await f.owner.observe(); + f.replace([row(100, process.pid, 100), row(200, 1, 200, "new-start")]); + await expect(f.owner.signal("SIGKILL")).rejects.toThrow("identity became uncertain"); + expect(f.signals).toEqual([]); + } finally { f.owner.stopObserving(); } +}); + +it.skipIf(process.platform === "win32")("retains observed descendants when the root exits and excludes the caller group", async () => { + const f = fixture([row(100, process.pid, 100), row(200, 100, 200), row(300, 100, 10), row(999, process.pid, 999)]); + try { + await f.owner.observe(); + f.root.exitCode = 1; + f.replace([row(200, 1, 200), row(300, 1, 10), row(999, process.pid, 999)]); + await f.owner.signal("SIGTERM"); + expect(f.signals).toEqual([[200, "SIGTERM"]]); + } finally { f.owner.stopObserving(); } +}); + +it.skipIf(process.platform === "win32")("treats an unreaped zombie as stopped without signaling a replacement", async () => { + const f = fixture([row(100, process.pid, 100), row(200, 100, 200)]); + try { + await f.owner.observe(); + f.root.exitCode = 1; + f.replace([row(200, 1, 200, "start-200", "Z")]); + await f.owner.signal("SIGKILL"); + expect(f.owner.liveGroups()).toEqual([]); + expect(f.signals).toEqual([]); + } finally { f.owner.stopObserving(); } +}); + +it.skipIf(process.platform === "win32")("sends launcher grace only to the root while retaining nested groups for escalation", async () => { + const f = fixture([row(100, process.pid, 100), row(200, 100, 200), row(300, 200, 300)]); + try { + await f.owner.observe(); + await f.owner.signal("SIGTERM", f.owner.gracefulRoots()); + expect(f.signals).toEqual([[100, "SIGTERM"]]); + f.root.exitCode = 1; + f.replace([row(200, 1, 200), row(300, 200, 300)]); + await f.owner.observe(); + expect([...f.owner.gracefulRoots()]).toEqual([200]); + await f.owner.signal("SIGKILL"); + expect(f.signals.slice(1).map(([pid]) => pid).sort()).toEqual([200, 300]); + } finally { f.owner.stopObserving(); } +}); diff --git a/tests/runner-e2e/process-tree-owner.ts b/tests/runner-e2e/process-tree-owner.ts new file mode 100644 index 0000000000..3301d0885b --- /dev/null +++ b/tests/runner-e2e/process-tree-owner.ts @@ -0,0 +1,130 @@ +import type { ChildProcess } from "node:child_process"; +import { + observeDescendantProcessTree, readProcessTable, revalidateObservedProcessGroups, + refreshContinuouslyLiveProcessGroups, safeProcessGroupTerminationOrder, + type ObservedProcessGroup, type ProcessObservation, +} from "./process-tree.js"; + +export function createProcessTreeOwner(root: ChildProcess, options: { + readTable?: () => Promise; + signalGroup?: (pid: number, signal: NodeJS.Signals) => void; +} = {}) { + let groups: ObservedProcessGroup[] = []; + let table: ProcessObservation[] = []; + let rootStarted: string | undefined; + let observing: Promise | undefined; + const readTable = options.readTable ?? readProcessTable; + const signalGroup = options.signalGroup ?? ((pid, signal) => process.kill(-pid, signal)); + const exited = () => root.exitCode !== null || root.signalCode !== null || !root.pid; + const running = (candidate: ProcessObservation) => !candidate.state?.startsWith("Z"); + + function observe(): Promise { + if (process.platform === "win32") return Promise.resolve(); + if (observing) return observing; + observing = (async () => { + const next = await readTable(); + if (!next) throw new Error("Could not inspect owned process identities"); + const callerGroup = next.find(row => row.pid === process.pid)?.processGroupId; + if (callerGroup === undefined) throw new Error("Cleanup caller group identity is unavailable"); + const rootRow = next.find(row => row.pid === root.pid); + if (!rootStarted && !exited()) rootStarted = rootRow?.started; + const retained = revalidateObservedProcessGroups(groups, next); + // Never acquire a recycled group from its numeric ID alone. + const lost = groups.filter(group => !retained.includes(group) && next.some(row => row.processGroupId === group.processGroupId && running(row))); + if (lost.length) throw new Error("Owned process group identity became uncertain"); + const refreshed = refreshContinuouslyLiveProcessGroups(retained, next) + .filter(group => group.processGroupId !== callerGroup); + const anchors = refreshed.flatMap(group => group.members.map(member => member.pid)); + if (rootRow && rootStarted === rootRow.started) anchors.push(rootRow.pid); + const byGroup = new Map(refreshed.map(group => [group.processGroupId, group])); + for (const pid of anchors) { + for (const group of observeDescendantProcessTree(next, pid).groups) { + if (group.processGroupId !== callerGroup) byGroup.set(group.processGroupId, group); + } + } + groups = [...byGroup.values()]; + table = next; + })().finally(() => { observing = undefined; }); + return observing; + } + const timer = process.platform === "win32" ? undefined : setInterval(() => { + void observe().catch(() => { /* Stop must obtain a fresh successful inspection. */ }); + }, 250); + timer?.unref(); + void observe().catch(() => {}); + + function liveGroups() { + return groups.filter(group => table.some(row => row.processGroupId === group.processGroupId && running(row))); + } + async function signal(signal: NodeJS.Signals, selected?: ReadonlySet) { + await observe(); + const currentProcessGroupId = table.find(row => row.pid === process.pid)?.processGroupId ?? null; + if (currentProcessGroupId === null) throw new Error("Cleanup caller group identity is unavailable"); + const live = liveGroups(); + const ordered = safeProcessGroupTerminationOrder({ rootProcessGroupId: root.pid ?? -1, currentProcessGroupId, groups: live }); + const signaled: number[] = []; + for (const pid of ordered) { + if (selected && !selected.has(pid)) continue; + try { signalGroup(pid, signal); } + catch (error) { if ((error as NodeJS.ErrnoException).code !== "ESRCH") throw error; } + signaled.push(pid); + } + return signaled; + } + function gracefulRoots() { + const live = liveGroups(); + const ids = new Set(live.map(group => group.processGroupId)); + if (root.pid && ids.has(root.pid)) return new Set([root.pid]); + // After the launcher's leader exits, request shutdown only from the + // outermost remaining owners; their wrappers still own child signaling. + const byPid = new Map(table.map(row => [row.pid, row])); + return new Set(live.filter(group => !table.some(row => { + if (row.processGroupId !== group.processGroupId) return false; + const seen = new Set(); + let parent = byPid.get(row.parentPid); + while (parent && !seen.has(parent.pid)) { + seen.add(parent.pid); + if (parent.processGroupId !== group.processGroupId && ids.has(parent.processGroupId)) return true; + parent = byPid.get(parent.parentPid); + } + return false; + })).map(group => group.processGroupId)); + } + return { observe, signal, liveGroups, gracefulRoots, stopObserving: () => { if (timer) clearInterval(timer); } }; +} + + +/** Give the launcher/wrapper chain sole graceful-signal ownership, then retire + * only still-observed descendants. Used on normal exit as well as cancellation. */ +export async function stopOwnedProcessTree( + child: ChildProcess, + owner: ReturnType, + gracefulMs = 45_000, + forcedMs = 5_000, +): Promise { + const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); + if (process.platform === "win32") { + if (child.exitCode === null && child.signalCode === null) child.kill("SIGTERM"); + } else { + await owner.observe(); + await owner.signal("SIGTERM", owner.gracefulRoots()); + } + const stopped = async () => { + await owner.observe(); + return (child.exitCode !== null || child.signalCode !== null) && owner.liveGroups().length === 0; + }; + const deadline = Date.now() + gracefulMs; + while (Date.now() < deadline) { + if (await stopped()) return; + await wait(50); + } + if (process.platform === "win32") { + if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL"); + } else await owner.signal("SIGKILL"); + const forcedDeadline = Date.now() + forcedMs; + do { + if (await stopped()) return; + await wait(50); + } while (Date.now() < forcedDeadline); + throw new Error("Observed launcher descendants remained after bounded cleanup"); +} diff --git a/tests/runner-e2e/process-tree.ts b/tests/runner-e2e/process-tree.ts index a7f0824b0f..7c939848fa 100644 --- a/tests/runner-e2e/process-tree.ts +++ b/tests/runner-e2e/process-tree.ts @@ -1,9 +1,13 @@ +import { spawn } from "node:child_process"; +import path from "node:path"; + export interface ProcessObservation { pid: number; parentPid: number; processGroupId: number; started: string; kind: string; + state?: string; } export interface ObservedProcessGroup { @@ -140,3 +144,86 @@ export function refreshContinuouslyLiveProcessGroups( : []; }); } + +const diagnosticProcessKinds = new Set([ + "bash", + "chrome", + "codex", + "google-chrome", + "node", + "paperclip-runnerd", + "playwright", + "pnpm", + "postgres", + "sh", + "tsx", +]); + +export async function readProcessTable(): Promise { + if (process.platform === "win32") { + return null; + } + return await new Promise((resolve) => { + const inspector = spawn( + "ps", + ["-e", "-o", "pid=,ppid=,pgid=,lstart=,stat=,comm="], + { + env: { PATH: "/usr/bin:/bin", LANG: "C", LC_ALL: "C" }, + stdio: ["ignore", "pipe", "ignore"], + // A wrapper-group shutdown must not interrupt its own bounded audit. + detached: true, + }, + ); + let output = ""; + let settled = false; + let truncated = false; + const finish = (value: ProcessObservation[] | null) => { + if (settled) return; + settled = true; + clearTimeout(inspectionTimeout); + resolve(value); + }; + const inspectionTimeout = setTimeout(() => { + inspector.kill("SIGKILL"); + finish(null); + }, 5_000); + inspectionTimeout.unref(); + inspector.stdout?.setEncoding("utf8").on("data", (chunk: string) => { + if (output.length + chunk.length > 1024 * 1024) { + truncated = true; + inspector.kill("SIGKILL"); + finish(null); + return; + } + output += chunk; + }); + inspector.once("error", () => finish(null)); + inspector.once("close", (code) => { + if (code !== 0 || truncated) { finish(null); return; } + const observations = output + .split(/\r?\n/) + .map((line) => + /^\s*(\d+)\s+(\d+)\s+(\d+)\s+(\S+\s+\S+\s+\d+\s+\d{2}:\d{2}:\d{2}\s+\d{4})\s+(\S+)\s+(.+?)\s*$/.exec( + line, + ), + ) + .filter((match): match is RegExpExecArray => match !== null) + .map((match): ProcessObservation => { + // A target process can choose its own argv and process name. Emit a + // fixed category instead of target-controlled text so diagnostics + // can never turn that metadata into a secret-exfiltration channel. + const command = path.basename(match[6]!); + const kind = diagnosticProcessKinds.has(command) ? command : "other"; + return { + pid: Number(match[1]), + parentPid: Number(match[2]), + processGroupId: Number(match[3]), + started: match[4]!, + kind, + state: match[5]!, + }; + }); + finish(observations); + }); + }).catch(() => null); +} diff --git a/tests/runner-e2e/server-stop.test.ts b/tests/runner-e2e/server-stop.test.ts index 3ee29ac56f..49b487f14d 100644 --- a/tests/runner-e2e/server-stop.test.ts +++ b/tests/runner-e2e/server-stop.test.ts @@ -1,10 +1,12 @@ import { spawn, type ChildProcess } from "node:child_process"; import { once } from "node:events"; -import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { afterEach, expect, it } from "vitest"; import { createRunnerE2EServerStopper, runnerE2EServerDetached } from "./server-stop.js"; +import { readProcessTable } from "./process-tree.js"; +import { createProcessTreeOwner, stopOwnedProcessTree } from "./process-tree-owner.js"; import { runnerE2ETypeScriptProcessArgs } from "./web-server-command.js"; const children: ChildProcess[] = []; @@ -115,7 +117,7 @@ it("settles a failed spawn without attempting escalation", async () => { expect(logs).toEqual([]); }); -it.skipIf(process.platform === "win32")("keeps external wrapper-group signals out of an asynchronously closing server", async () => { +it.skipIf(process.platform === "win32")("lets launcher cancellation join wrapper shutdown without signaling the server twice", async () => { const { root, entry } = await fixture(); const supervisor = path.join(root, "supervisor.mts"); const helper = path.join(import.meta.dirname, "server-stop.ts"); @@ -138,7 +140,10 @@ it.skipIf(process.platform === "win32")("keeps external wrapper-group signals ou detached: true, stdio: ["ignore", "pipe", "pipe", "ipc"], }); children.push(wrapper); + let wrapperErrors = ""; + wrapper.stderr?.on("data", chunk => { wrapperErrors += chunk; }); let workerPid: number | undefined; + const owner = createProcessTreeOwner(wrapper); try { workerPid = (await once(wrapper, "message"))[0].pid; const stopping = once(wrapper, "message"); @@ -149,14 +154,17 @@ it.skipIf(process.platform === "win32")("keeps external wrapper-group signals ou const messages: unknown[] = []; wrapper.on("message", message => messages.push(message)); const exited = once(wrapper, "exit"); - process.kill(-wrapper.pid!, "SIGTERM"); + await owner.observe(); + const cancellation = stopOwnedProcessTree(wrapper, owner, 2_000, 2_000); await new Promise(resolve => setTimeout(resolve, 250)); if (wrapper.connected) wrapper.send("finish"); await exited; - expect(messages).toContainEqual({ message: "close-complete", pid: workerPid }); + await cancellation; + expect(messages, wrapperErrors).toContainEqual({ message: "close-complete", pid: workerPid }); expect(messages).toContainEqual({ exitCode: 0, signalCode: null }); expect(() => process.kill(workerPid!, 0)).toThrow(); } finally { + owner.stopObserving(); if (workerPid) { try { process.kill(-workerPid, "SIGKILL"); } catch { /* reaped */ } } } }); @@ -175,11 +183,78 @@ it.skipIf(process.platform === "win32")("escalates the owned server group so a h try { await stopper(50).stop(child); expect(child.signalCode).toBe("SIGKILL"); - // Reparented children may be reaped a little after the direct child's exit. - await expect.poll(() => { - try { process.kill(pid, 0); return true; } catch { return false; } - }, { timeout: 2_000 }).toBe(false); + // A stopped orphan may remain a zombie until the host's init reaps it. + const table = await readProcessTable(); + expect(table).not.toBeNull(); + expect(table!.some(row => row.pid === pid && !row.state?.startsWith("Z"))).toBe(false); } finally { try { process.kill(pid, "SIGKILL"); } catch { /* reaped */ } } }); + + +it.skipIf(process.platform === "win32")("retires observed descendants after the server leader exits early", async () => { + const { root, entry } = await fixture(); + const marker = path.join(root, "descendant-closed"); + const descendant = ` + process.once('SIGTERM', () => setTimeout(() => { + require('node:fs').writeFileSync(${JSON.stringify(marker)}, 'closed'); process.exit(0); + }, 50)); + process.send('ready'); setInterval(() => {}, 1000); + `; + await writeFile(entry, ` + const child = require('node:child_process').spawn(process.execPath, ['-e', ${JSON.stringify(descendant)}], { + stdio: ['ignore', 'ignore', 'ignore', 'ipc'], + }); + child.once('message', () => process.send({ pid: child.pid })); + process.once('message', () => process.exit(1)); + `); + const child = start(entry); + const pid = (await once(child, "message"))[0].pid as number; + const { stop } = stopper(); + try { + await stop.watch(child); + const exited = once(child, "exit"); + child.send("crash"); + await exited; + await stop(child); + expect(await readFile(marker, "utf8")).toBe("closed"); + const table = await readProcessTable(); + expect(table).not.toBeNull(); + expect(table!.some(row => row.pid === pid && !row.state?.startsWith("Z"))).toBe(false); + } finally { try { process.kill(pid, "SIGKILL"); } catch { /* stopped */ } } +}); + +it.skipIf(process.platform === "win32")("recovers a failed cleanup without repeating the graceful signal", async () => { + const { entry } = await fixture(); + const child = start(entry); + await once(child, "message"); + let failInspection = false; + let owner: ReturnType | undefined; + const messages: unknown[] = []; + child.on("message", message => { + messages.push(message); + if (message === "stopping") failInspection = true; + }); + const stop = createRunnerE2EServerStopper({ + gracefulTimeoutMs: 2_000, forcedTimeoutMs: 2_000, + hasSpawnError: () => false, markExpectedStop: () => {}, log: () => {}, + createOwner: candidate => { + owner = createProcessTreeOwner(candidate); + return { ...owner, observe: async () => { + if (failInspection) { failInspection = false; throw new Error("transient inspection failure"); } + await owner!.observe(); + } }; + }, + }); + try { + await expect(stop(child)).rejects.toThrow("transient inspection failure"); + const recovery = stop(child); + // A second graceful signal would terminate this child's once-handler. + await new Promise(resolve => setTimeout(resolve, 50)); + if (child.connected) child.send("finish"); + await recovery; + expect(messages).toEqual(["stopping", "close-complete"]); + expect([child.exitCode, child.signalCode]).toEqual([0, null]); + } finally { owner?.stopObserving(); } +}); diff --git a/tests/runner-e2e/server-stop.ts b/tests/runner-e2e/server-stop.ts index b0bc059761..7a28abb9c4 100644 --- a/tests/runner-e2e/server-stop.ts +++ b/tests/runner-e2e/server-stop.ts @@ -1,7 +1,9 @@ import type { ChildProcess } from "node:child_process"; +import { createProcessTreeOwner } from "./process-tree-owner.js"; // The wrapper receives Playwright's group signal; only it signals Paperclip. export const runnerE2EServerDetached = process.platform !== "win32"; +const wait = (ms: number) => new Promise(resolve => setTimeout(resolve, ms)); export function createRunnerE2EServerStopper(options: { gracefulTimeoutMs: number; @@ -9,52 +11,68 @@ export function createRunnerE2EServerStopper(options: { hasSpawnError: (child: ChildProcess) => boolean; markExpectedStop: (child: ChildProcess) => void; log: (message: string) => void; + createOwner?: typeof createProcessTreeOwner; }) { - const stops = new WeakMap>(); - const exited = (child: ChildProcess) => - child.exitCode !== null || child.signalCode !== null || options.hasSpawnError(child); - - function waitForExit(child: ChildProcess, timeoutMs: number) { - if (exited(child)) return Promise.resolve(true); - return new Promise((resolve) => { - const finish = (didExit: boolean) => { - clearTimeout(timer); - child.off("exit", onExit); - child.off("error", onExit); - resolve(didExit); - }; - const onExit = () => finish(true); - const timer = setTimeout(() => finish(false), timeoutMs); - child.once("exit", onExit); - child.once("error", onExit); - }); - } - - async function stopOnce(child: ChildProcess, signal: NodeJS.Signals) { - if (exited(child)) return; - try { child.kill(signal); } - catch (error) { if (!exited(child)) throw error; } - if (await waitForExit(child, options.gracefulTimeoutMs)) return; - options.log(`\nPaperclip did not stop within ${options.gracefulTimeoutMs}ms; sending SIGKILL\n`); - try { - // This still-live direct child leads the private group we spawned. Kill - // its group only on timeout, so descendants cannot outlive escalation. - if (runnerE2EServerDetached && child.pid) process.kill(-child.pid, "SIGKILL"); - else child.kill("SIGKILL"); - } catch (error) { if (!exited(child)) throw error; } - if (!(await waitForExit(child, options.forcedTimeoutMs))) { - throw new Error("Paperclip server did not exit after SIGKILL"); - } - } - - return (child: ChildProcess, signal: NodeJS.Signals = "SIGTERM"): Promise => { - options.markExpectedStop(child); - let stop = stops.get(child); - if (!stop) { - // Publish ownership before invoking any callback or delivering a signal. - stop = Promise.resolve().then(() => stopOnce(child, signal)); - stops.set(child, stop); - } - return stop; + type State = { + owner: ReturnType; + promise?: Promise; + gracefulDeadline?: number; + gracefulSent: boolean; + groupSignals: Set; + escalationLogged: boolean; }; + const states = new WeakMap(); + const exited = (child: ChildProcess) => child.exitCode !== null || child.signalCode !== null || options.hasSpawnError(child); + function watch(child: ChildProcess) { + let state = states.get(child); + if (!state) { + state = { owner: (options.createOwner ?? createProcessTreeOwner)(child), gracefulSent: false, groupSignals: new Set(), escalationLogged: false }; + states.set(child, state); + } + return state; + } + async function stopOnce(child: ChildProcess, state: State, signal: NodeJS.Signals) { + await state.owner.observe(); + state.gracefulDeadline ??= Date.now() + options.gracefulTimeoutMs; + if (!exited(child) && !state.gracefulSent) { + child.kill(signal); + state.gracefulSent = true; + } + while (Date.now() < state.gracefulDeadline) { + await state.owner.observe(); + if (exited(child)) { + if (!state.owner.liveGroups().length) { state.owner.stopObserving(); return; } + // The leader is gone. Retire only descendants whose start identities + // we observed while they belonged to this server, including private groups. + const unsignaled = new Set(state.owner.liveGroups().map(group => group.processGroupId).filter(pid => !state.groupSignals.has(pid))); + for (const pid of await state.owner.signal("SIGTERM", unsignaled)) state.groupSignals.add(pid); + } + await wait(25); + } + if (!state.escalationLogged) { + options.log(`\nPaperclip did not stop within ${options.gracefulTimeoutMs}ms; sending SIGKILL\n`); + state.escalationLogged = true; + } + if (runnerE2EServerDetached) await state.owner.signal("SIGKILL"); + else if (!exited(child)) child.kill("SIGKILL"); + const deadline = Date.now() + options.forcedTimeoutMs; + do { + await state.owner.observe(); + if (exited(child) && !state.owner.liveGroups().length) { state.owner.stopObserving(); return; } + await wait(25); + } while (Date.now() < deadline); + throw new Error("Paperclip server or observed descendants did not exit after SIGKILL"); + } + const stop = (child: ChildProcess, signal: NodeJS.Signals = "SIGTERM"): Promise => { + options.markExpectedStop(child); + const state = watch(child); + state.promise ??= Promise.resolve().then(() => stopOnce(child, state, signal)).catch(error => { + // Retain the signal/deadline phase, but let final cleanup recover from an + // inspection/signaling failure without delivering another graceful signal. + state.promise = undefined; + throw error; + }); + return state.promise; + }; + return Object.assign(stop, { watch: (child: ChildProcess) => watch(child).owner.observe() }); } diff --git a/tests/runner-e2e/server.ts b/tests/runner-e2e/server.ts index 3b80282f1f..9acb4292a2 100644 --- a/tests/runner-e2e/server.ts +++ b/tests/runner-e2e/server.ts @@ -106,7 +106,7 @@ function describeChildExit(candidate: ChildProcess) { return `server exited code=${String(candidate.exitCode)} signal=${String(candidate.signalCode)}`; } -function startServer() { +async function startServer() { if (shutdownRequested()) { throw new Error("Refusing to start Paperclip after wrapper shutdown"); } @@ -155,6 +155,7 @@ function startServer() { // The main/error path awaits this same promise and reports any failure. void stopServer(candidate, shutdownSignal).catch(() => {}); } + await stopServer.watch(candidate); return candidate; } @@ -280,7 +281,7 @@ async function restartServer(requestId: string) { } appendLog(`Restart request ${requestId}: starting Paperclip\n`); - const replacement = startServer(); + const replacement = await startServer(); await waitForHealth(replacement); if (shutdownRequested()) { throw new Error("Wrapper shutdown interrupted the Paperclip restart"); @@ -313,7 +314,7 @@ async function supervise() { }); // Postgres needs the socket itself; release immediately before child spawn. await databaseReservation?.close(); - startServer(); + await startServer(); let lastRestartRequestId: string | null = null; while (!shutdownRequested()) { if (unexpectedChildFailure) throw unexpectedChildFailure;