Merge PR #7060: fix(browser): own headless Chromium profile dir to survive Windows EBUSY cleanup (@roboomp)

2026-07-31 19:42:43 +02:00
parent e1ad89abe1 d0649f9917
commit 63d6bd3064
4 changed files with 151 additions and 13 deletions
@@ -1,7 +1,7 @@
 import * as fs from "node:fs";
 import * as os from "node:os";
 import * as path from "node:path";
-import { $which, getPuppeteerDir, logger } from "@oh-my-pi/pi-utils";
+import { $which, getPuppeteerDir, logger, removeWithRetries } from "@oh-my-pi/pi-utils";
 import type * as BrowsersNs from "@puppeteer/browsers";
 import type { Browser, CDPSession, Page, default as Puppeteer, Target } from "puppeteer-core";
 import stealthTamperingScript from "../puppeteer/00_stealth_tampering.txt" with { type: "text" };
@@ -285,7 +285,18 @@ export interface LaunchHeadlessOptions {
 	ignoreDefaultArgs?: readonly string[];
 }

-export async function launchHeadlessBrowser(opts: LaunchHeadlessOptions): Promise<Browser> {
+/** Result of a headless Chromium launch. */
+export interface LaunchHeadlessResult {
+	browser: Browser;
+	/**
+	 * OMP-owned temporary Chromium profile directory to remove after the browser
+	 * process tree exits, or `undefined` when the caller supplied its own
+	 * `--user-data-dir` (which OMP must not delete).
+	 */
+	userDataDir?: string;
+}
+
+export async function launchHeadlessBrowser(opts: LaunchHeadlessOptions): Promise<LaunchHeadlessResult> {
 	const vp = opts.viewport ?? DEFAULT_VIEWPORT;
 	const initialViewport = {
 		width: vp.width,
@@ -316,15 +327,53 @@ export async function launchHeadlessBrowser(opts: LaunchHeadlessOptions): Promis
 	for (const arg of opts.args ?? []) {
 		if (!launchArgs.includes(arg)) launchArgs.push(arg);
 	}
-	const executablePath = await ensureChromiumExecutable();
-	return await puppeteer.launch({
-		headless: opts.headless,
-		defaultViewport: opts.headless ? initialViewport : null,
-		executablePath,
-		args: launchArgs,
-		ignoreDefaultArgs: [...new Set([...stealthIgnoreDefaultArgs(executablePath), ...(opts.ignoreDefaultArgs ?? [])])],
-		protocolTimeout: BROWSER_PROTOCOL_TIMEOUT_MS,
-	});
+	// Own the Chromium profile directory instead of letting puppeteer-core create
+	// (and delete) a temporary one. Passing `--user-data-dir` makes puppeteer
+	// treat the profile as non-temporary, so `ChromeLauncher.cleanUserDataDir`
+	// becomes a no-op and can no longer reject its eager process-exit hook with an
+	// unhandled EBUSY when Chromium still holds the profile lock on Windows
+	// (issue #7058). `removeUserDataDir` cleans it up on our terms instead.
+	let userDataDir: string | undefined;
+	if (!launchArgs.some(arg => arg.startsWith("--user-data-dir"))) {
+		userDataDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), "omp-chrome-profile-"));
+		launchArgs.push(`--user-data-dir=${userDataDir}`);
+	}
+	try {
+		const executablePath = await ensureChromiumExecutable();
+		const browser = await puppeteer.launch({
+			headless: opts.headless,
+			defaultViewport: opts.headless ? initialViewport : null,
+			executablePath,
+			args: launchArgs,
+			ignoreDefaultArgs: [
+				...new Set([...stealthIgnoreDefaultArgs(executablePath), ...(opts.ignoreDefaultArgs ?? [])]),
+			],
+			protocolTimeout: BROWSER_PROTOCOL_TIMEOUT_MS,
+		});
+		return { browser, userDataDir };
+	} catch (error) {
+		if (userDataDir) await removeUserDataDir(userDataDir);
+		throw error;
+	}
+}
+
+/**
+ * Remove an OMP-owned headless Chromium profile directory, tolerating the brief
+ * window on Windows in which Chromium (or an orphaned browser subprocess) still
+ * holds the profile lock. The shared temp remover centralizes retry handling
+ * for EBUSY/EPERM/ENOTEMPTY; if the directory is still busy afterwards we warn
+ * and leave it for a later cleanup pass rather than throwing — a shutdown cleanup
+ * failure must never crash the process (issue #7058).
+ */
+export async function removeUserDataDir(dir: string): Promise<void> {
+	try {
+		await removeWithRetries(dir);
+	} catch (error) {
+		logger.warn("Left Chromium profile directory in place after cleanup failure", {
+			dir,
+			error: error instanceof Error ? error.message : String(error),
+		});
+	}
 }

 export async function applyViewport(
@@ -6,7 +6,13 @@ import { ToolAbortError, ToolError } from "../tool-errors";
 import { findFreeCdpPort, findReusableCdp, gracefulKillTreeOnce, killExistingByPath, waitForCdp } from "./attach";
 import type { CmuxKind } from "./cmux/rpc";
 import { CmuxSocketClient } from "./cmux/socket-client";
-import { BROWSER_PROTOCOL_TIMEOUT_MS, launchHeadlessBrowser, loadPuppeteer, type UserAgentOverride } from "./launch";
+import {
+	BROWSER_PROTOCOL_TIMEOUT_MS,
+	launchHeadlessBrowser,
+	loadPuppeteer,
+	removeUserDataDir,
+	type UserAgentOverride,
+} from "./launch";

 export type PuppeteerBrowserKind =
 	| { kind: "headless"; headless: boolean }
@@ -35,6 +41,8 @@ export interface PuppeteerBrowserHandle extends BrowserHandleCommon {
 	browser: Browser;
 	cdpUrl?: string;
 	pid?: number;
+	/** OMP-owned temp Chromium profile directory removed on dispose (headless launches). */
+	userDataDir?: string;
 	subprocess?: Subprocess;
 	stealth: { browserSession: CDPSession | null; override: UserAgentOverride | null };
 }
@@ -134,11 +142,15 @@ async function openBrowserHandle(kind: BrowserKind, opts: AcquireBrowserOptions)
 		};
 	}
 	if (kind.kind === "headless") {
-		const browser = await launchHeadlessBrowser({ headless: kind.headless, viewport: opts.viewport });
+		const { browser, userDataDir } = await launchHeadlessBrowser({
+			headless: kind.headless,
+			viewport: opts.viewport,
+		});
 		return {
 			key: browserKey(kind),
 			kind,
 			browser,
+			userDataDir,
 			refCount: 0,
 			stealth: { browserSession: null, override: null },
 		};
@@ -259,6 +271,10 @@ async function disposeBrowserHandle(handle: BrowserHandle, opts: ReleaseBrowserO
 				if (proc?.pid !== undefined) await gracefulKillTreeOnce(proc.pid).catch(() => undefined);
 			}
 		}
+		// OMP owns the profile directory (puppeteer's temp cleanup is disabled by
+		// our explicit --user-data-dir), so remove it now the process tree has
+		// exited. Tolerant of the Windows lock-held window (issue #7058).
+		if (handle.userDataDir) await removeUserDataDir(handle.userDataDir);
 		return;
 	}
 	if (handle.kind.kind === "connected") {