From 84edce908aa5531fab58883b364b52f54c57b9b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Fri, 22 May 2026 23:53:46 -0400 Subject: [PATCH] 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. --- packages/cli/src/commands/preview.ts | 26 +++++++++++++++++---- packages/cli/src/utils/orphanCleanup.ts | 25 ++++++++++++++++---- packages/engine/src/utils/processTracker.ts | 24 ++++++++++++++++--- 3 files changed, 63 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/commands/preview.ts b/packages/cli/src/commands/preview.ts index 5925d4bcb..065842f2a 100644 --- a/packages/cli/src/commands/preview.ts +++ b/packages/cli/src/commands/preview.ts @@ -262,6 +262,8 @@ async function runDevMode( // SIGINT to the foreground process group (covers the common case), but // `kill ` only targets this process — the child tree (Vite + Chrome) // would survive without explicit cleanup. + // On Windows, killProcessTree is a no-op (pgrep/ps unavailable); Ctrl+C + // propagates via the console process group instead. const shutdown = () => { if (child.pid) killProcessTree(child.pid); }; @@ -366,6 +368,7 @@ async function runLocalStudioMode( }); } + // Same tree-kill handler as dev mode. No-op on Windows (see comment above). const shutdown = () => { if (child.pid) killProcessTree(child.pid); }; @@ -504,25 +507,38 @@ async function runEmbeddedMode( console.log(); console.log(` ${c.dim("Shutting down studio...")}`); - // Kill all child processes (browsers, ffmpeg) before closing the server. - // This is the centralized cleanup path — studioServer no longer registers - // its own per-process signal handlers. + // Hard deadline: if cleanup hangs (e.g. dead Chrome never responds to + // browser.close()), force exit. Armed before awaiting cleanup so it + // can't be blocked by a stuck drainBrowserPool(). + setTimeout(() => process.exit(0), 3000).unref(); + + // Kill ffmpeg first (sync, fast), then drain browsers (async, slower). const cleanup = async () => { const { closeThumbnailBrowser } = await import("../server/studioServer.js"); const { drainBrowserPool, killTrackedProcesses } = await import("@hyperframes/engine"); + killTrackedProcesses(); await closeThumbnailBrowser().catch(() => {}); await drainBrowserPool().catch(() => {}); - killTrackedProcesses(); }; cleanup() .catch(() => {}) .finally(() => { result.server.close(() => resolveRun()); - setTimeout(() => process.exit(0), 3000).unref(); }); }; process.once("SIGINT", shutdown); process.once("SIGTERM", shutdown); + + // Last-resort cleanup for crash paths (unhandled exceptions/rejections) + // that bypass the signal handlers. Eagerly resolve the sync killer so + // the 'exit' handler (which is synchronous) can call it directly. + import("@hyperframes/engine") + .then(({ killTrackedProcesses }) => { + process.once("exit", () => { + if (!shuttingDown) killTrackedProcesses(); + }); + }) + .catch(() => {}); }); } diff --git a/packages/cli/src/utils/orphanCleanup.ts b/packages/cli/src/utils/orphanCleanup.ts index 966ab8e7c..3b7157853 100644 --- a/packages/cli/src/utils/orphanCleanup.ts +++ b/packages/cli/src/utils/orphanCleanup.ts @@ -10,6 +10,9 @@ import { execSync } from "node:child_process"; * its parent died). We kill the orphan's entire subtree so child helper * processes (GPU, renderer, network, etc.) are also cleaned up. * + * Scoped to the current user via `pgrep -u` to avoid touching other + * users' processes on shared machines. + * * Returns the count of killed process trees. */ export function killOrphanedProcesses(): number { @@ -17,13 +20,10 @@ export function killOrphanedProcesses(): number { let killed = 0; - // chrome-headless-shell: used in production/CI via the engine's browser manager. for (const name of ["chrome-headless-shell", "chrome_headless_shell"]) { killed += killOrphansByName(name); } - // Puppeteer-launched Chrome (dev mode): identified by the temp profile dir - // that Puppeteer creates. This avoids killing the user's real Chrome. killed += killOrphansByName("puppeteer_dev_chrome_profile"); return killed; @@ -33,6 +33,9 @@ export function killOrphanedProcesses(): number { * Kill an entire process tree rooted at `pid`. Walks descendants * depth-first so children are killed before parents, preventing * re-adoption races. + * + * No-op on Windows — process groups are managed differently and + * the pgrep/ps utilities are not available. */ export function killProcessTree(pid: number, signal: NodeJS.Signals = "SIGTERM"): void { if (process.platform === "win32") return; @@ -73,9 +76,11 @@ function getDescendants(pid: number): number[] { } function killOrphansByName(processName: string): number { + const uid = getUid(); + const userFlag = uid !== null ? `-u ${uid} ` : ""; let pids: number[]; try { - const raw = execSync(`pgrep -f ${processName}`, { + const raw = execSync(`pgrep ${userFlag}-f ${processName}`, { encoding: "utf-8", timeout: 3000, }).trim(); @@ -97,6 +102,18 @@ function killOrphansByName(processName: string): number { return killed; } +let _cachedUid: string | null | undefined; + +function getUid(): string | null { + if (_cachedUid !== undefined) return _cachedUid; + try { + _cachedUid = execSync("id -u", { encoding: "utf-8", timeout: 1000 }).trim(); + } catch { + _cachedUid = null; + } + return _cachedUid; +} + function isOrphan(pid: number): boolean { try { const ppid = execSync(`ps -p ${pid} -o ppid=`, { diff --git a/packages/engine/src/utils/processTracker.ts b/packages/engine/src/utils/processTracker.ts index 660038ea3..c75c9ce84 100644 --- a/packages/engine/src/utils/processTracker.ts +++ b/packages/engine/src/utils/processTracker.ts @@ -9,15 +9,33 @@ export function trackChildProcess(proc: ChildProcess): void { proc.once("error", remove); } -export function killTrackedProcesses(signal: NodeJS.Signals = "SIGTERM"): void { +/** + * SIGTERM all tracked child processes, then SIGKILL any that survive + * after a short grace period. + */ +export function killTrackedProcesses(): void { + const alive: ChildProcess[] = []; for (const proc of tracked) { if (!proc.killed) { try { - proc.kill(signal); + proc.kill("SIGTERM"); + alive.push(proc); } catch { - // Best-effort — process may have already exited between the check and the kill. + // Already exited between the check and the kill. } } } tracked.clear(); + + if (alive.length === 0) return; + + setTimeout(() => { + for (const proc of alive) { + try { + proc.kill("SIGKILL"); + } catch { + // Already exited. + } + } + }, 500).unref(); }