From 9eb4a39cd434d993612c5daefda71e77fbe5cd49 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Mon, 26 Oct 2026 11:00:37 -0400 Subject: [PATCH 1/2] fix(cli): a killed CLI no longer leaves its child processes running --- .../src/browser/chromeOrphans.process.test.ts | 46 ++++++++++++----- packages/cli/src/utils/cancellableProcess.ts | 3 ++ packages/cli/src/utils/childReaper.fixture.ts | 26 ++++++++++ packages/cli/src/utils/childReaper.test.ts | 49 +++++++++++++++++++ packages/cli/src/utils/childReaper.ts | 48 ++++++++++++++++++ .../engine/src/services/browserManager.ts | 6 +-- packages/engine/src/services/hdrCapture.ts | 4 +- 7 files changed, 164 insertions(+), 18 deletions(-) create mode 100644 packages/cli/src/utils/childReaper.fixture.ts create mode 100644 packages/cli/src/utils/childReaper.test.ts create mode 100644 packages/cli/src/utils/childReaper.ts diff --git a/packages/cli/src/browser/chromeOrphans.process.test.ts b/packages/cli/src/browser/chromeOrphans.process.test.ts index 161127244f1..d91a1d040dc 100644 --- a/packages/cli/src/browser/chromeOrphans.process.test.ts +++ b/packages/cli/src/browser/chromeOrphans.process.test.ts @@ -15,15 +15,15 @@ import { const here = dirname(fileURLToPath(import.meta.url)); const cli = join(here, "..", "cli.ts"); -function project(busy: boolean): string { +function project(busy: boolean, seconds = 1): string { const dir = mkdtempSync(join(tmpdir(), "hf-chrome-orphans-")); onTestFinished(() => rm(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 })); mkdirSync(join(dir, "compositions")); writeFileSync( join(dir, "index.html"), - `
-
-`, + `
+
+`, ); const script = busy ? `const el=document.getElementById("box");while(el.offsetWidth<2){el.style.height="1px";}` @@ -35,15 +35,16 @@ function project(busy: boolean): string { return dir; } -function check(dir: string, entry = cli, env: Record = {}) { - const child = spawn( - process.execPath, - ["--import", "tsx", entry, "check", dir, "--json", "--no-browser-gpu", "--samples=1"], - { - env: { ...process.env, CI: "1", HYPERFRAMES_NO_TELEMETRY: "1", ...env }, - stdio: ["ignore", "ignore", "ignore"], - }, - ); +function check( + dir: string, + entry = cli, + env: Record = {}, + command = ["check", dir, "--json", "--no-browser-gpu", "--samples=1"], +) { + const child = spawn(process.execPath, ["--import", "tsx", entry, ...command], { + env: { ...process.env, CI: "1", HYPERFRAMES_NO_TELEMETRY: "1", ...env }, + stdio: ["ignore", "ignore", "ignore"], + }); const seen = { chrome: new Set(), watchdog: new Set() }; const record = () => { for (const pid of testProcessDescendants(child.pid!, /chrom/i)) seen.chrome.add(pid); @@ -89,3 +90,22 @@ it("leaves no Chrome process or watchdog behind after a check that finishes", as 10_000, ); }, 90_000); + +it("leaves no Chrome process behind when the CLI is killed mid-render", async () => { + const dir = project(false, 30); + const run = check(dir, cli, {}, [ + "render", + dir, + "--output", + join(dir, "out.mp4"), + "--quality", + "draft", + ]); + await waitForTestCondition(() => { + run.record(); + return run.seen.chrome.size >= 3; + }, 60_000); + expect(alive(run.seen.chrome).length).toBeGreaterThan(0); + run.child.kill("SIGKILL"); + await waitForTestCondition(() => alive(run.seen.chrome).length === 0, 30_000); +}, 120_000); diff --git a/packages/cli/src/utils/cancellableProcess.ts b/packages/cli/src/utils/cancellableProcess.ts index 59985b8a63d..cbf31ff2cf6 100644 --- a/packages/cli/src/utils/cancellableProcess.ts +++ b/packages/cli/src/utils/cancellableProcess.ts @@ -1,5 +1,6 @@ import { spawn, type ChildProcess, type SpawnOptions } from "node:child_process"; import { fileURLToPath } from "node:url"; +import { killWithThisProcess } from "./childReaper.js"; import { terminateProcessTree } from "./processTree.js"; import { renderSetupFailureFrom, renderSetupResultFrom } from "../renderSetupWorkerLifecycle.js"; @@ -75,6 +76,7 @@ export function runCancellableProcess( return; } + const release = child.pid ? killWithThisProcess(child.pid, detached) : undefined; if (child.pid) options.onSpawn?.(child.pid); let stdout = ""; let stderr = ""; @@ -127,6 +129,7 @@ export function runCancellableProcess( child.once("error", (error) => void finishReject(error)); // fallow-ignore-next-line complexity child.once("close", async (status, closeSignal) => { + release?.(); if (settled) return; if (terminationTask) terminationSignal = await terminationTask.catch(() => terminationSignal); if (settled) return; diff --git a/packages/cli/src/utils/childReaper.fixture.ts b/packages/cli/src/utils/childReaper.fixture.ts new file mode 100644 index 00000000000..86c8dbdfa77 --- /dev/null +++ b/packages/cli/src/utils/childReaper.fixture.ts @@ -0,0 +1,26 @@ +// fallow-ignore-file unused-file +import { writeFileSync } from "node:fs"; +import { runCancellableProcess } from "./cancellableProcess.js"; + +const [readyPath] = process.argv.slice(2); +if (!readyPath) throw new Error("Missing fixture arguments"); + +// Like a synchronous lint with the setup worker's signal handlers: its event loop never turns again, so SIGTERM is +// queued for a handler that never runs, and nothing inside it can notice the parent go. +const childSource = [ + 'import { spawn } from "node:child_process";', + 'import { writeFileSync } from "node:fs";', + 'process.on("SIGTERM", () => {});', + 'const grandchild = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" });', + "writeFileSync(process.argv[1], JSON.stringify({ child: process.pid, grandchild: grandchild.pid }));", + "Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0);", +].join("\n"); + +void runCancellableProcess( + process.execPath, + ["--input-type=module", "--eval", childSource, `${readyPath}.child`], + { + onSpawn: () => writeFileSync(readyPath, String(process.pid)), + }, +).catch(() => undefined); +setInterval(() => undefined, 1000); diff --git a/packages/cli/src/utils/childReaper.test.ts b/packages/cli/src/utils/childReaper.test.ts new file mode 100644 index 00000000000..adc8c6af1b0 --- /dev/null +++ b/packages/cli/src/utils/childReaper.test.ts @@ -0,0 +1,49 @@ +import { spawn, type ChildProcess } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, describe, expect, it } from "vitest"; +import { testProcessIsAlive, waitForTestCondition } from "./processTestUtils.js"; + +const fixturePath = join(dirname(fileURLToPath(import.meta.url)), "childReaper.fixture.ts"); +const leftovers: number[] = []; +let parent: ChildProcess | undefined; + +afterEach(() => { + parent?.kill("SIGKILL"); + for (const pid of leftovers.splice(0)) { + try { + process.kill(pid, "SIGKILL"); + } catch {} + } +}); + +describe.skipIf(process.platform === "win32")("a CLI killed outright", () => { + it("takes every process it started with it, a child stuck in synchronous work included", async () => { + const dir = mkdtempSync(join(tmpdir(), "hyperframes-child-reaper-")); + const readyPath = join(dir, "ready"); + try { + parent = spawn(process.execPath, ["--import", "tsx", fixturePath, readyPath], { + stdio: "ignore", + }); + await waitForTestCondition(() => existsSync(`${readyPath}.child`), 15_000); + await waitForTestCondition( + () => readFileSync(`${readyPath}.child`, "utf8").endsWith("}"), + 5_000, + ); + const { child, grandchild } = JSON.parse(readFileSync(`${readyPath}.child`, "utf8")) as { + child: number; + grandchild: number; + }; + leftovers.push(child, grandchild); + expect([child, grandchild].every(testProcessIsAlive)).toBe(true); + + parent.kill("SIGKILL"); + + await waitForTestCondition(() => ![child, grandchild].some(testProcessIsAlive), 5_000); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/cli/src/utils/childReaper.ts b/packages/cli/src/utils/childReaper.ts new file mode 100644 index 00000000000..b0d7a0ceb8f --- /dev/null +++ b/packages/cli/src/utils/childReaper.ts @@ -0,0 +1,48 @@ +import { spawn, type ChildProcess } from "node:child_process"; + +// Ends with this process's end of its stdin, however this process died, and kills what is still held. +const REAPER = `const held = new Map(); +let rest = ""; +const kill = (pid, group) => { + try { + if (process.platform === "win32") + require("node:child_process").spawnSync("taskkill", ["/PID", pid, "/T", "/F"], { windowsHide: true, timeout: 10000 }); + else process.kill(group ? -Number(pid) : Number(pid), "SIGKILL"); + } catch {} +}; +process.stdin.on("data", (data) => { + const lines = (rest + data).split("\\n"); + rest = lines.pop(); + for (const [op, pid, group] of lines.map((line) => line.split(" "))) + op === "+" ? held.set(pid, group === "g") : held.delete(pid); +}); +const done = () => { + for (const [pid, group] of held) kill(pid, group); + process.exit(0); +}; +process.stdin.on("end", done); +process.stdin.on("error", done);`; + +let reaper: ChildProcess | undefined; + +function lifeline(): NodeJS.WritableStream | null { + if (!reaper) { + reaper = spawn(process.execPath, ["-e", REAPER], { + detached: true, + stdio: ["pipe", "ignore", "ignore"], + windowsHide: true, + }); + reaper.on("error", () => {}); + reaper.stdin?.on("error", () => {}); + reaper.unref(); + (reaper.stdin as unknown as { unref?: () => void } | null)?.unref?.(); + } + return reaper.stdin; +} + +/** Kills `pid`, its whole process group when `group`, if this process ends before the returned release runs. */ +export function killWithThisProcess(pid: number, group: boolean): () => void { + const stdin = lifeline(); + stdin?.write(`+ ${pid} ${group ? "g" : "p"}\n`); + return () => void stdin?.write(`- ${pid}\n`); +} diff --git a/packages/engine/src/services/browserManager.ts b/packages/engine/src/services/browserManager.ts index 04b0f32d632..74351c199e8 100644 --- a/packages/engine/src/services/browserManager.ts +++ b/packages/engine/src/services/browserManager.ts @@ -48,7 +48,7 @@ export function setHostHandlesSigint(owned: boolean): void { hostHandlesSigint = owned; } -export const sigintLaunchOptions = () => ({ handleSIGINT: !hostHandlesSigint }); +export const ownedLaunchOptions = () => ({ handleSIGINT: !hostHandlesSigint, pipe: true }); interface WebGlProbeInfo { hasWebGL: boolean; @@ -110,7 +110,7 @@ async function probeHardwareWebGlInfo( let probeBrowser: Browser | undefined; try { const launchOptions: LaunchOptions = { - ...sigintLaunchOptions(), + ...ownedLaunchOptions(), headless: true, args: withHyperframesBrowserMarker(options.args), defaultViewport: { width: 64, height: 64 }, @@ -886,7 +886,7 @@ async function launchBrowser( let browser: Browser | undefined; const launch = (args: readonly string[]) => ppt.launch({ - ...sigintLaunchOptions(), + ...ownedLaunchOptions(), headless: true, args: withHyperframesBrowserMarker(args), defaultViewport: null, diff --git a/packages/engine/src/services/hdrCapture.ts b/packages/engine/src/services/hdrCapture.ts index bdb43f229fa..6f59f480780 100644 --- a/packages/engine/src/services/hdrCapture.ts +++ b/packages/engine/src/services/hdrCapture.ts @@ -21,7 +21,7 @@ import { withHyperframesBrowserMarker } from "@hyperframes/core/browser-marker"; import type { Page, Browser, PuppeteerNode } from "puppeteer-core"; -import { resolveFullChromePath, sigintLaunchOptions } from "./browserManager.js"; +import { resolveFullChromePath, ownedLaunchOptions } from "./browserManager.js"; // ── PQ (SMPTE 2084) OETF ───────────────────────────────────────────────────── @@ -247,7 +247,7 @@ export async function launchHdrBrowser( } const browser = await ppt.launch({ - ...sigintLaunchOptions(), + ...ownedLaunchOptions(), headless: false, executablePath: chromePath, args: withHyperframesBrowserMarker(buildHdrChromeArgs(width, height)), From 302e2aa9fc500059743fd920aa13666b2eec4cc0 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Mon, 26 Oct 2026 11:08:11 -0400 Subject: [PATCH 2/2] test(cli): shorten the reaper fixture comment to fit the comment ratchet --- packages/cli/src/utils/childReaper.fixture.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/cli/src/utils/childReaper.fixture.ts b/packages/cli/src/utils/childReaper.fixture.ts index 86c8dbdfa77..8f2705ce1ed 100644 --- a/packages/cli/src/utils/childReaper.fixture.ts +++ b/packages/cli/src/utils/childReaper.fixture.ts @@ -5,8 +5,7 @@ import { runCancellableProcess } from "./cancellableProcess.js"; const [readyPath] = process.argv.slice(2); if (!readyPath) throw new Error("Missing fixture arguments"); -// Like a synchronous lint with the setup worker's signal handlers: its event loop never turns again, so SIGTERM is -// queued for a handler that never runs, and nothing inside it can notice the parent go. +// Acts like a synchronous lint: its event loop never turns, so no handler in it can notice the parent go. const childSource = [ 'import { spawn } from "node:child_process";', 'import { writeFileSync } from "node:fs";',