Skip to content

Commit f07055a

Browse files
miguel-heygenmiguel-heygen
authored
fix(cli): close every browser on SIGTERM and SIGINT via one launch owner (#4910)
* fix(cli): close every browser on SIGTERM and SIGINT through one launch owner * fix(cli): own SIGTERM before the GPU probe browser launches * fix(cli): keep tests on the managed GPU mode and drop the engine signal flag * test(cli): give the browser fallback fake a process handle
1 parent 5632356 commit f07055a

12 files changed

Lines changed: 238 additions & 32 deletions

‎packages/cli/src/beats/headlessAnalyzer.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { launchManagedBrowser } from "../browser/launch.js";
12
// Run the shared beat detection (@hyperframes/core/beats) in a headless Chrome
23
// so results match the Studio exactly — same Web Audio decode + same
34
// bpm-detective. Used by the `beats` CLI command to write the beat file before
@@ -138,7 +139,7 @@ export async function analyzeBeatsHeadless(audioBytes: Buffer): Promise<Headless
138139
const { ensureBrowser } = await import("../browser/manager.js");
139140
const puppeteer = await import("puppeteer-core");
140141
const browser = await ensureBrowser();
141-
const chrome: Browser = await puppeteer.default.launch({
142+
const chrome: Browser = await launchManagedBrowser(puppeteer.default, {
142143
headless: true,
143144
executablePath: browser.executablePath,
144145
args: ["--no-sandbox", "--disable-dev-shm-usage", "--autoplay-policy=no-user-gesture-required"],

‎packages/cli/src/browser/launch.fixture.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// fallow-ignore-file unused-file
2+
import { writeFileSync } from "node:fs";
3+
import { registerRootExitRequester } from "../utils/commandResult.js";
4+
import { ensureBrowser } from "./manager.js";
5+
import { launchManagedBrowser, resolveManagedGpuMode } from "./launch.js";
6+
7+
const [profileDir, readyPath] = process.argv.slice(2);
8+
if (!profileDir || !readyPath) throw new Error("Missing fixture arguments");
9+
10+
// Stand-in for cli.ts: records the requested exit code, then ends the process.
11+
registerRootExitRequester((exitCode) => {
12+
writeFileSync(`${readyPath}.exit`, String(exitCode));
13+
process.kill(process.pid, "SIGKILL");
14+
});
15+
const { executablePath } = await ensureBrowser();
16+
const puppeteer = await import("puppeteer-core");
17+
if (process.argv[4] === "probe") {
18+
writeFileSync(readyPath, "probing");
19+
await resolveManagedGpuMode("auto", executablePath);
20+
}
21+
writeFileSync(readyPath, "launching");
22+
const browser = await launchManagedBrowser(puppeteer.default, {
23+
headless: true,
24+
executablePath,
25+
args: ["--no-sandbox", `--user-data-dir=${profileDir}`],
26+
});
27+
writeFileSync(readyPath, "ready");
28+
await browser.pages();
29+
setInterval(() => undefined, 1_000);

‎packages/cli/src/browser/launch.process.test.ts‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
import { spawn, type ChildProcess } from "node:child_process";
2+
import { existsSync, mkdtempSync, readFileSync, readdirSync } from "node:fs";
3+
import { tmpdir } from "node:os";
4+
import { dirname, join } from "node:path";
5+
import { fileURLToPath } from "node:url";
6+
import { afterEach, describe, expect, it } from "vitest";
7+
import { waitForTestCondition } from "../utils/processTestUtils.js";
8+
9+
const fixturePath = join(dirname(fileURLToPath(import.meta.url)), "launch.fixture.ts");
10+
const children = new Set<ChildProcess>();
11+
12+
/** Pids of every process whose command line carries this profile dir (Chromium and its helpers). */
13+
function browserPids(profileDir: string): number[] {
14+
return readdirSync("/proc")
15+
.filter((name) => /^\d+$/.test(name))
16+
.filter((name) => {
17+
try {
18+
return readFileSync(`/proc/${name}/cmdline`, "utf8").includes(profileDir);
19+
} catch {
20+
return false;
21+
}
22+
})
23+
.map(Number);
24+
}
25+
26+
function startFixture(profileDir: string, readyPath: string, mode = "launch"): ChildProcess {
27+
const child = spawn(
28+
process.execPath,
29+
["--import", "tsx", fixturePath, profileDir, readyPath, mode],
30+
{ stdio: "ignore" },
31+
);
32+
children.add(child);
33+
return child;
34+
}
35+
36+
const exited = (child: ChildProcess) =>
37+
new Promise<{ code: number | null; signal: string | null }>((resolve) =>
38+
child.once("exit", (code, signal) => resolve({ code, signal })),
39+
);
40+
41+
afterEach(() => {
42+
for (const child of children) child.kill("SIGKILL");
43+
children.clear();
44+
});
45+
46+
describe.skipIf(process.platform !== "linux")("managed browser shutdown", () => {
47+
it.each([
48+
["SIGTERM", "ready", 143],
49+
["SIGINT", "ready", 130],
50+
["SIGTERM", "launching", 143],
51+
] as const)(
52+
"%s while %s leaves no browser and asks the CLI to exit",
53+
async (signal, phase, code) => {
54+
const dir = mkdtempSync(join(tmpdir(), "hf-launch-"));
55+
const profileDir = join(dir, "profile");
56+
const readyPath = join(dir, "ready");
57+
const child = startFixture(profileDir, readyPath);
58+
const done = exited(child);
59+
await waitForTestCondition(
60+
() => existsSync(readyPath) && readFileSync(readyPath, "utf8") === phase,
61+
30_000,
62+
);
63+
if (phase === "ready")
64+
await waitForTestCondition(() => browserPids(profileDir).length > 0, 30_000);
65+
child.kill(signal);
66+
await done;
67+
expect(readFileSync(`${readyPath}.exit`, "utf8")).toBe(String(code));
68+
await waitForTestCondition(() => browserPids(profileDir).length === 0, 5_000);
69+
},
70+
60_000,
71+
);
72+
73+
it("SIGTERM during the GPU probe asks the CLI to exit instead of being swallowed", async () => {
74+
const dir = mkdtempSync(join(tmpdir(), "hf-launch-"));
75+
const readyPath = join(dir, "ready");
76+
const child = startFixture(join(dir, "profile"), readyPath, "probe");
77+
const done = exited(child);
78+
await waitForTestCondition(
79+
() => existsSync(readyPath) && readFileSync(readyPath, "utf8") === "probing",
80+
30_000,
81+
);
82+
child.kill("SIGTERM");
83+
await done;
84+
expect(readFileSync(`${readyPath}.exit`, "utf8")).toBe("143");
85+
}, 60_000);
86+
});

‎packages/cli/src/browser/launch.ts‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
import type { Browser, LaunchOptions, PuppeteerNode } from "puppeteer-core";
2+
import {
3+
resolveCaptureBrowserGpuMode,
4+
type BrowserGpuMode,
5+
type ResolvedBrowserGpuMode,
6+
} from "./gpuPolicy.js";
7+
import { requestCliExit } from "../utils/commandResult.js";
8+
9+
const browsers = new Set<Browser>();
10+
const pending = new Map<Promise<Browser>, AbortController>();
11+
const CLOSE_GRACE_MS = 1_000;
12+
let listening = false;
13+
let shutdown: Promise<void> | undefined;
14+
15+
/** CLI signals close browser admission and native processes before the root exits. */
16+
function stopForSignal(signal: "SIGINT" | "SIGTERM"): void {
17+
if (shutdown) return;
18+
shutdown = closeManagedBrowsers();
19+
void shutdown.then(
20+
() => requestCliExit(signal === "SIGINT" ? 130 : 143),
21+
(error: unknown) => {
22+
console.error("Browser shutdown failed:", error);
23+
requestCliExit(1);
24+
},
25+
);
26+
}
27+
28+
function ownSignals(): void {
29+
if (listening) return;
30+
listening = true;
31+
process.on("SIGINT", () => stopForSignal("SIGINT"));
32+
process.on("SIGTERM", () => stopForSignal("SIGTERM"));
33+
}
34+
35+
/** The GPU probe launches its own Chrome, so the signal owner must exist before it runs. */
36+
export async function resolveManagedGpuMode(
37+
requestedMode: BrowserGpuMode,
38+
chromePath?: string,
39+
): Promise<ResolvedBrowserGpuMode> {
40+
ownSignals();
41+
return resolveCaptureBrowserGpuMode(requestedMode, chromePath);
42+
}
43+
44+
/** One owner for every CLI browser, including a launch cancelled before its connection is ready. */
45+
export async function launchManagedBrowser(
46+
puppeteer: PuppeteerNode,
47+
options: LaunchOptions,
48+
): Promise<Browser> {
49+
if (shutdown) throw new Error("The CLI is stopping; no browser can start.");
50+
ownSignals();
51+
const abort = new AbortController();
52+
const signal = options.signal ? AbortSignal.any([options.signal, abort.signal]) : abort.signal;
53+
const launch = puppeteer.launch({
54+
...options,
55+
signal,
56+
handleSIGINT: false,
57+
handleSIGTERM: false,
58+
});
59+
pending.set(launch, abort);
60+
try {
61+
const browser = await launch;
62+
const child = browser.process();
63+
if (!child) throw new Error("A launched browser has no owned process.");
64+
browsers.add(browser);
65+
child.once("exit", () => browsers.delete(browser));
66+
return browser;
67+
} finally {
68+
pending.delete(launch);
69+
}
70+
}
71+
72+
/** Resolved launches keep their signal live; only pending launches are aborted during shutdown. */
73+
async function closeManagedBrowsers(): Promise<void> {
74+
for (const abort of pending.values()) abort.abort();
75+
await Promise.allSettled(pending.keys());
76+
await Promise.all([...browsers].map(closeBrowser));
77+
}
78+
79+
async function closeBrowser(browser: Browser): Promise<void> {
80+
const child = browser.process();
81+
if (!child) throw new Error("A managed browser lost its owned process.");
82+
if (child.exitCode !== null || child.signalCode !== null) return;
83+
const exited = new Promise<void>((resolve) => child.once("exit", () => resolve()));
84+
let timer: NodeJS.Timeout | undefined;
85+
const grace = new Promise<void>((resolve) => {
86+
timer = setTimeout(resolve, CLOSE_GRACE_MS);
87+
});
88+
try {
89+
await Promise.race([
90+
browser.close().catch((error: unknown) => console.warn("Browser close failed:", error)),
91+
grace,
92+
]);
93+
if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL");
94+
await exited;
95+
} finally {
96+
clearTimeout(timer);
97+
}
98+
}

‎packages/cli/src/capture/captureAttempt.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { launchManagedBrowser } from "../browser/launch.js";
12
import { LottieDiscovery } from "./lottieDiscovery.js";
23
import { createCaptureDownloadBudget } from "./readBoundedResponse.js";
34
/**
@@ -111,7 +112,7 @@ export async function captureWebsiteAttempt(
111112
const { ensureBrowser } = await import("../browser/manager.js");
112113
const browser = await ensureBrowser();
113114
const puppeteer = await import("puppeteer-core");
114-
const chromeBrowser = await puppeteer.default.launch({
115+
const chromeBrowser = await launchManagedBrowser(puppeteer.default, {
115116
headless: true,
116117
executablePath: browser.executablePath,
117118
protocolTimeout: captureProtocolTimeoutMs(timeout, budgetMs),

‎packages/cli/src/capture/captureCompositionFrame.browserFallback.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { EventEmitter } from "node:events";
12
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
23

34
const mocks = vi.hoisted(() => ({
@@ -54,6 +55,7 @@ function fakeBrowser() {
5455
browser: {
5556
newPage: vi.fn(async () => page),
5657
close: vi.fn(async () => undefined),
58+
process: vi.fn(() => new EventEmitter()),
5759
},
5860
page,
5961
};

‎packages/cli/src/capture/captureCompositionFrame.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -236,9 +236,9 @@ describe("screenshot Chrome arguments", () => {
236236
);
237237
const layoutSource = readFileSync(new URL("../commands/layout.ts", import.meta.url), "utf8");
238238

239-
expect(captureSource).toContain("resolveCaptureBrowserGpuMode(");
239+
expect(captureSource).toContain("resolveManagedGpuMode(");
240240
expect(captureSource).toContain("{ browserGpuMode: resolvedGpuMode }");
241-
expect(layoutSource).toContain("resolveCaptureBrowserGpuMode(");
241+
expect(layoutSource).toContain("resolveManagedGpuMode(");
242242
expect(layoutSource).toContain("{ browserGpuMode: resolvedGpuMode }");
243243
});
244244
});

‎packages/cli/src/capture/captureCompositionFrame.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1+
import { launchManagedBrowser, resolveManagedGpuMode } from "../browser/launch.js";
12
import { spawn } from "node:child_process";
23
import type { Browser, Page } from "puppeteer-core";
34
import { c } from "../ui/colors.js";
45
import {
56
assertWebGpuAdapterAvailable,
67
compositionRequiresWebGpu,
7-
resolveCaptureBrowserGpuMode,
88
resolveLocalBrowserGpuMode,
99
type BrowserGpuMode,
1010
} from "../browser/gpuPolicy.js";
@@ -179,8 +179,8 @@ export async function openSettledCompositionPage(
179179
const requestedGpuMode = options.browserGpuMode ?? resolveCliChromeGpuMode();
180180
const requiresWebGpu = compositionRequiresWebGpu(html);
181181
const launch = async (executablePath: string): Promise<Browser> => {
182-
const resolvedGpuMode = await resolveCaptureBrowserGpuMode(requestedGpuMode, executablePath);
183-
return puppeteer.default.launch({
182+
const resolvedGpuMode = await resolveManagedGpuMode(requestedGpuMode, executablePath);
183+
return launchManagedBrowser(puppeteer.default, {
184184
headless: true,
185185
executablePath,
186186
args: buildChromeArgs(

‎packages/cli/src/commands/layout.ts‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { launchManagedBrowser, resolveManagedGpuMode } from "../browser/launch.js";
12
import { failCommand, setCommandExitCode } from "../utils/commandResult.js";
23
import { defineCommand } from "citty";
34
import { existsSync, readFileSync } from "node:fs";
@@ -203,12 +204,8 @@ async function runLayoutAudit(
203204
const { ensureBrowser } = await import("../browser/manager.js");
204205
const puppeteer = await import("puppeteer-core");
205206
const { buildChromeArgs } = await import("@hyperframes/engine");
206-
const {
207-
assertWebGpuAdapterAvailable,
208-
compositionRequiresWebGpu,
209-
resolveCaptureBrowserGpuMode,
210-
resolveLocalBrowserGpuMode,
211-
} = await import("../browser/gpuPolicy.js");
207+
const { assertWebGpuAdapterAvailable, compositionRequiresWebGpu, resolveLocalBrowserGpuMode } =
208+
await import("../browser/gpuPolicy.js");
212209
const html = await bundleProjectHtml(projectDir);
213210
const server = await serveStaticProjectHtml(
214211
projectDir,
@@ -220,12 +217,9 @@ async function runLayoutAudit(
220217
try {
221218
const browser = await ensureBrowser();
222219
const requestedGpuMode = resolveLocalBrowserGpuMode();
223-
const resolvedGpuMode = await resolveCaptureBrowserGpuMode(
224-
requestedGpuMode,
225-
browser.executablePath,
226-
);
220+
const resolvedGpuMode = await resolveManagedGpuMode(requestedGpuMode, browser.executablePath);
227221
const requiresWebGpu = compositionRequiresWebGpu(html);
228-
chromeBrowser = await puppeteer.default.launch({
222+
chromeBrowser = await launchManagedBrowser(puppeteer.default, {
229223
headless: true,
230224
executablePath: browser.executablePath,
231225
args: buildChromeArgs(

‎packages/cli/src/commands/motionShot.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ describe("motion-shot adapter seeking", () => {
8282
it("launches through the shared GPU policy and WebGPU requirement guard", () => {
8383
const source = readFileSync(motionShotSourcePath, "utf8");
8484

85-
expect(source).toContain("resolveCaptureBrowserGpuMode");
85+
expect(source).toContain("resolveManagedGpuMode");
8686
expect(source).toContain("compositionRequiresWebGpu(html)");
8787
expect(source).toContain("assertWebGpuAdapterAvailable(page, requiresWebGpu)");
8888
expect(source).toContain("{ browserGpuMode: resolvedGpuMode }");

0 commit comments

Comments
 (0)