Skip to content

Commit 84edce9

Browse files
committed
fix: address code review feedback on process cleanup
- Blocker: arm 3s force-exit timer BEFORE awaiting cleanup, not inside .finally(). Prevents hang if drainBrowserPool() blocks on dead Chrome. - Reorder cleanup: killTrackedProcesses() (sync, fast) runs first, then async browser drain. Ffmpeg dies immediately instead of surviving if the hard timer fires early. - SIGKILL escalation: processTracker now SIGTERMs all tracked processes, then SIGKILLs survivors after 500ms grace period. - Scope pgrep to current user (pgrep -u $(id -u)) so orphan detection doesn't touch other users' Chrome on shared machines. - Add process.on('exit') handler for crash paths (unhandled exceptions/rejections that bypass signal handlers). - Document Windows no-op behavior on killProcessTree handlers.
1 parent e87f5bb commit 84edce9

3 files changed

Lines changed: 63 additions & 12 deletions

File tree

packages/cli/src/commands/preview.ts

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,8 @@ async function runDevMode(
262262
// SIGINT to the foreground process group (covers the common case), but
263263
// `kill <pid>` only targets this process — the child tree (Vite + Chrome)
264264
// would survive without explicit cleanup.
265+
// On Windows, killProcessTree is a no-op (pgrep/ps unavailable); Ctrl+C
266+
// propagates via the console process group instead.
265267
const shutdown = () => {
266268
if (child.pid) killProcessTree(child.pid);
267269
};
@@ -366,6 +368,7 @@ async function runLocalStudioMode(
366368
});
367369
}
368370

371+
// Same tree-kill handler as dev mode. No-op on Windows (see comment above).
369372
const shutdown = () => {
370373
if (child.pid) killProcessTree(child.pid);
371374
};
@@ -504,25 +507,38 @@ async function runEmbeddedMode(
504507
console.log();
505508
console.log(` ${c.dim("Shutting down studio...")}`);
506509

507-
// Kill all child processes (browsers, ffmpeg) before closing the server.
508-
// This is the centralized cleanup path — studioServer no longer registers
509-
// its own per-process signal handlers.
510+
// Hard deadline: if cleanup hangs (e.g. dead Chrome never responds to
511+
// browser.close()), force exit. Armed before awaiting cleanup so it
512+
// can't be blocked by a stuck drainBrowserPool().
513+
setTimeout(() => process.exit(0), 3000).unref();
514+
515+
// Kill ffmpeg first (sync, fast), then drain browsers (async, slower).
510516
const cleanup = async () => {
511517
const { closeThumbnailBrowser } = await import("../server/studioServer.js");
512518
const { drainBrowserPool, killTrackedProcesses } = await import("@hyperframes/engine");
519+
killTrackedProcesses();
513520
await closeThumbnailBrowser().catch(() => {});
514521
await drainBrowserPool().catch(() => {});
515-
killTrackedProcesses();
516522
};
517523

518524
cleanup()
519525
.catch(() => {})
520526
.finally(() => {
521527
result.server.close(() => resolveRun());
522-
setTimeout(() => process.exit(0), 3000).unref();
523528
});
524529
};
525530
process.once("SIGINT", shutdown);
526531
process.once("SIGTERM", shutdown);
532+
533+
// Last-resort cleanup for crash paths (unhandled exceptions/rejections)
534+
// that bypass the signal handlers. Eagerly resolve the sync killer so
535+
// the 'exit' handler (which is synchronous) can call it directly.
536+
import("@hyperframes/engine")
537+
.then(({ killTrackedProcesses }) => {
538+
process.once("exit", () => {
539+
if (!shuttingDown) killTrackedProcesses();
540+
});
541+
})
542+
.catch(() => {});
527543
});
528544
}

packages/cli/src/utils/orphanCleanup.ts

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,20 +10,20 @@ import { execSync } from "node:child_process";
1010
* its parent died). We kill the orphan's entire subtree so child helper
1111
* processes (GPU, renderer, network, etc.) are also cleaned up.
1212
*
13+
* Scoped to the current user via `pgrep -u` to avoid touching other
14+
* users' processes on shared machines.
15+
*
1316
* Returns the count of killed process trees.
1417
*/
1518
export function killOrphanedProcesses(): number {
1619
if (process.platform === "win32") return 0;
1720

1821
let killed = 0;
1922

20-
// chrome-headless-shell: used in production/CI via the engine's browser manager.
2123
for (const name of ["chrome-headless-shell", "chrome_headless_shell"]) {
2224
killed += killOrphansByName(name);
2325
}
2426

25-
// Puppeteer-launched Chrome (dev mode): identified by the temp profile dir
26-
// that Puppeteer creates. This avoids killing the user's real Chrome.
2727
killed += killOrphansByName("puppeteer_dev_chrome_profile");
2828

2929
return killed;
@@ -33,6 +33,9 @@ export function killOrphanedProcesses(): number {
3333
* Kill an entire process tree rooted at `pid`. Walks descendants
3434
* depth-first so children are killed before parents, preventing
3535
* re-adoption races.
36+
*
37+
* No-op on Windows — process groups are managed differently and
38+
* the pgrep/ps utilities are not available.
3639
*/
3740
export function killProcessTree(pid: number, signal: NodeJS.Signals = "SIGTERM"): void {
3841
if (process.platform === "win32") return;
@@ -73,9 +76,11 @@ function getDescendants(pid: number): number[] {
7376
}
7477

7578
function killOrphansByName(processName: string): number {
79+
const uid = getUid();
80+
const userFlag = uid !== null ? `-u ${uid} ` : "";
7681
let pids: number[];
7782
try {
78-
const raw = execSync(`pgrep -f ${processName}`, {
83+
const raw = execSync(`pgrep ${userFlag}-f ${processName}`, {
7984
encoding: "utf-8",
8085
timeout: 3000,
8186
}).trim();
@@ -97,6 +102,18 @@ function killOrphansByName(processName: string): number {
97102
return killed;
98103
}
99104

105+
let _cachedUid: string | null | undefined;
106+
107+
function getUid(): string | null {
108+
if (_cachedUid !== undefined) return _cachedUid;
109+
try {
110+
_cachedUid = execSync("id -u", { encoding: "utf-8", timeout: 1000 }).trim();
111+
} catch {
112+
_cachedUid = null;
113+
}
114+
return _cachedUid;
115+
}
116+
100117
function isOrphan(pid: number): boolean {
101118
try {
102119
const ppid = execSync(`ps -p ${pid} -o ppid=`, {

packages/engine/src/utils/processTracker.ts

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,33 @@ export function trackChildProcess(proc: ChildProcess): void {
99
proc.once("error", remove);
1010
}
1111

12-
export function killTrackedProcesses(signal: NodeJS.Signals = "SIGTERM"): void {
12+
/**
13+
* SIGTERM all tracked child processes, then SIGKILL any that survive
14+
* after a short grace period.
15+
*/
16+
export function killTrackedProcesses(): void {
17+
const alive: ChildProcess[] = [];
1318
for (const proc of tracked) {
1419
if (!proc.killed) {
1520
try {
16-
proc.kill(signal);
21+
proc.kill("SIGTERM");
22+
alive.push(proc);
1723
} catch {
18-
// Best-effort — process may have already exited between the check and the kill.
24+
// Already exited between the check and the kill.
1925
}
2026
}
2127
}
2228
tracked.clear();
29+
30+
if (alive.length === 0) return;
31+
32+
setTimeout(() => {
33+
for (const proc of alive) {
34+
try {
35+
proc.kill("SIGKILL");
36+
} catch {
37+
// Already exited.
38+
}
39+
}
40+
}, 500).unref();
2341
}

0 commit comments

Comments
 (0)