From afd6f8eed4d717718bd866176710bc24ffafbcfa Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 30 Jul 2026 05:09:26 +0000 Subject: [PATCH 1/3] fix(browser): own headless chromium profile dir to survive windows ebusy cleanup launchHeadlessBrowser let puppeteer-core create and delete a temporary Chrome profile via an unretried rm() from an eager process-exit hook. On Windows, when an orphaned browser tree still held the profile lock, that rm threw EBUSY and rejected the eager promise with no handler attached, crashing OMP with an unhandled rejection during cleanup. OMP now passes an explicit --user-data-dir, which makes puppeteer treat the profile as non-temporary (ChromeLauncher.cleanUserDataDir becomes a no-op), and removes the directory itself on dispose with lock-tolerant retry, warning and leaving it in place if it stays busy rather than crashing. Fixes #7058 --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/tools/browser/launch.ts | 46 +++++++++++- .../src/tools/browser/registry.ts | 20 +++++- .../tools/browser-profile-cleanup.test.ts | 72 +++++++++++++++++++ 4 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 packages/coding-agent/test/tools/browser-profile-cleanup.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2e8ffcd37..8a41d4884 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -22,6 +22,7 @@ ### Fixed +- Fixed the browser tool crashing OMP with an unhandled `EBUSY: resource busy or locked, rm '\puppeteer_dev_chrome_profile-'` rejection when a headless Chromium profile was still locked during cleanup on Windows. `launchHeadlessBrowser` now owns the profile directory via an explicit `--user-data-dir` (disabling puppeteer's unretried temp cleanup) and removes it on dispose with lock-tolerant retry, warning and leaving the directory in place if it stays busy rather than crashing ([#7058](https://github.com/can1357/oh-my-pi/issues/7058)). - Fixed task tool blocks duplicating their per-agent progress rows into terminal scrollback on every update: live task frames now pin the transcript live region so mid-run rows are never recorded as frozen snapshots, and a detached background task freezes its progress the moment any of its rows commit to scrollback instead of mutating committed history. - Fixed Codex reset fireworks comparing different quota tiers or plans, preventing false celebrations when usage reports switch between Spark and base weekly limits. - Fixed Cursor ranged-read results losing the full file byte size after applying the requested window. diff --git a/packages/coding-agent/src/tools/browser/launch.ts b/packages/coding-agent/src/tools/browser/launch.ts index 6f0da7f56..26615b293 100644 --- a/packages/coding-agent/src/tools/browser/launch.ts +++ b/packages/coding-agent/src/tools/browser/launch.ts @@ -285,7 +285,18 @@ export interface LaunchHeadlessOptions { ignoreDefaultArgs?: readonly string[]; } -export async function launchHeadlessBrowser(opts: LaunchHeadlessOptions): Promise { +/** 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 { const vp = opts.viewport ?? DEFAULT_VIEWPORT; const initialViewport = { width: vp.width, @@ -316,8 +327,19 @@ export async function launchHeadlessBrowser(opts: LaunchHeadlessOptions): Promis for (const arg of opts.args ?? []) { if (!launchArgs.includes(arg)) launchArgs.push(arg); } + // 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}`); + } const executablePath = await ensureChromiumExecutable(); - return await puppeteer.launch({ + const browser = await puppeteer.launch({ headless: opts.headless, defaultViewport: opts.headless ? initialViewport : null, executablePath, @@ -325,6 +347,26 @@ export async function launchHeadlessBrowser(opts: LaunchHeadlessOptions): Promis ignoreDefaultArgs: [...new Set([...stealthIgnoreDefaultArgs(executablePath), ...(opts.ignoreDefaultArgs ?? [])])], protocolTimeout: BROWSER_PROTOCOL_TIMEOUT_MS, }); + return { browser, userDataDir }; +} + +/** + * 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. `fs.rm`'s native `maxRetries`/`retryDelay` back off on + * 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 { + try { + await fs.promises.rm(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } 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( diff --git a/packages/coding-agent/src/tools/browser/registry.ts b/packages/coding-agent/src/tools/browser/registry.ts index a34b63545..a7864fb85 100644 --- a/packages/coding-agent/src/tools/browser/registry.ts +++ b/packages/coding-agent/src/tools/browser/registry.ts @@ -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") { diff --git a/packages/coding-agent/test/tools/browser-profile-cleanup.test.ts b/packages/coding-agent/test/tools/browser-profile-cleanup.test.ts new file mode 100644 index 000000000..0fb35ca09 --- /dev/null +++ b/packages/coding-agent/test/tools/browser-profile-cleanup.test.ts @@ -0,0 +1,72 @@ +/** + * Regression test for issue #7058: on Windows, puppeteer-core deletes its temp + * Chrome profile with an unretried `rm()` from an eager process-exit hook, so an + * EBUSY on the still-locked profile surfaces as an unhandled rejection that + * crashes OMP. OMP now owns the profile directory and removes it itself with a + * lock-tolerant, warn-and-leave cleanup. + */ + +import { afterEach, describe, expect, it, spyOn } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { removeUserDataDir } from "@oh-my-pi/pi-coding-agent/tools/browser/launch"; +import { type BrowserHandle, releaseBrowser } from "@oh-my-pi/pi-coding-agent/tools/browser/registry"; +import { logger } from "@oh-my-pi/pi-utils"; + +async function makeProfileDir(): Promise { + const dir = await fs.promises.mkdtemp(path.join(os.tmpdir(), "omp-chrome-profile-test-")); + await Bun.write(path.join(dir, "SingletonLock"), "lock"); + await Bun.write(path.join(dir, "Default", "Preferences"), "{}"); + return dir; +} + +describe("headless Chromium profile cleanup (issue #7058)", () => { + afterEach(() => { + spyOn(fs.promises, "rm").mockRestore(); + spyOn(logger, "warn").mockRestore(); + }); + + it("removes an owned profile directory", async () => { + const dir = await makeProfileDir(); + await removeUserDataDir(dir); + expect(fs.existsSync(dir)).toBe(false); + }); + + it("warns and leaves the directory instead of throwing when it stays locked (EBUSY)", async () => { + const dir = await makeProfileDir(); + const ebusy = Object.assign(new Error(`EBUSY: resource busy or locked, rm '${dir}'`), { code: "EBUSY" }); + const rmSpy = spyOn(fs.promises, "rm").mockRejectedValue(ebusy); + const warnSpy = spyOn(logger, "warn"); + try { + // Must resolve — a cleanup failure never propagates as a crash. + await expect(removeUserDataDir(dir)).resolves.toBeUndefined(); + expect(rmSpy).toHaveBeenCalledTimes(1); + expect(warnSpy).toHaveBeenCalledTimes(1); + } finally { + rmSpy.mockRestore(); + // Real removal so the fixture does not leak. + await fs.promises.rm(dir, { recursive: true, force: true }); + } + }); + + it("removes the handle's profile directory when the headless browser is disposed", async () => { + const dir = await makeProfileDir(); + const handle = { + key: "headless:1", + kind: { kind: "headless", headless: true }, + refCount: 1, + userDataDir: dir, + browser: { + connected: true, + process: () => ({ pid: 4242 }), + close: () => Promise.resolve(), + }, + stealth: { browserSession: null, override: null }, + } as unknown as BrowserHandle; + + await releaseBrowser(handle, { kill: false }); + + expect(fs.existsSync(dir)).toBe(false); + }); +}); From 7ab3d2ecb6cb5f331f7749a69091f0a29068c217 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 30 Jul 2026 05:19:18 +0000 Subject: [PATCH 2/3] fix(browser): cleaned headless profile after failed launch Remove the OMP-owned Chromium user-data directory when executable resolution or puppeteer launch fails before a browser handle exists. Fixes #7058 --- .../coding-agent/src/tools/browser/launch.ts | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/packages/coding-agent/src/tools/browser/launch.ts b/packages/coding-agent/src/tools/browser/launch.ts index 26615b293..8c9e3c033 100644 --- a/packages/coding-agent/src/tools/browser/launch.ts +++ b/packages/coding-agent/src/tools/browser/launch.ts @@ -338,16 +338,23 @@ export async function launchHeadlessBrowser(opts: LaunchHeadlessOptions): Promis userDataDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), "omp-chrome-profile-")); launchArgs.push(`--user-data-dir=${userDataDir}`); } - 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 }; + 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; + } } /** From d0649f99172b7e39b0d5fbfe2a40f8c5994a92dc Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 30 Jul 2026 05:22:14 +0000 Subject: [PATCH 3/3] fix(browser): reused shared temp removal retries Route Chromium profile cleanup through pi-utils removeWithRetries while preserving browser-specific warn-and-leave behavior. Fixes #7058 --- packages/coding-agent/src/tools/browser/launch.ts | 10 +++++----- .../test/tools/browser-profile-cleanup.test.ts | 14 +++++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/coding-agent/src/tools/browser/launch.ts b/packages/coding-agent/src/tools/browser/launch.ts index 8c9e3c033..565296a98 100644 --- a/packages/coding-agent/src/tools/browser/launch.ts +++ b/packages/coding-agent/src/tools/browser/launch.ts @@ -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" }; @@ -360,14 +360,14 @@ export async function launchHeadlessBrowser(opts: LaunchHeadlessOptions): Promis /** * 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. `fs.rm`'s native `maxRetries`/`retryDelay` back off on - * 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 + * 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 { try { - await fs.promises.rm(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + await removeWithRetries(dir); } catch (error) { logger.warn("Left Chromium profile directory in place after cleanup failure", { dir, diff --git a/packages/coding-agent/test/tools/browser-profile-cleanup.test.ts b/packages/coding-agent/test/tools/browser-profile-cleanup.test.ts index 0fb35ca09..5e21b284e 100644 --- a/packages/coding-agent/test/tools/browser-profile-cleanup.test.ts +++ b/packages/coding-agent/test/tools/browser-profile-cleanup.test.ts @@ -12,7 +12,7 @@ import * as os from "node:os"; import * as path from "node:path"; import { removeUserDataDir } from "@oh-my-pi/pi-coding-agent/tools/browser/launch"; import { type BrowserHandle, releaseBrowser } from "@oh-my-pi/pi-coding-agent/tools/browser/registry"; -import { logger } from "@oh-my-pi/pi-utils"; +import * as piUtils from "@oh-my-pi/pi-utils"; async function makeProfileDir(): Promise { const dir = await fs.promises.mkdtemp(path.join(os.tmpdir(), "omp-chrome-profile-test-")); @@ -23,8 +23,8 @@ async function makeProfileDir(): Promise { describe("headless Chromium profile cleanup (issue #7058)", () => { afterEach(() => { - spyOn(fs.promises, "rm").mockRestore(); - spyOn(logger, "warn").mockRestore(); + spyOn(piUtils, "removeWithRetries").mockRestore(); + spyOn(piUtils.logger, "warn").mockRestore(); }); it("removes an owned profile directory", async () => { @@ -36,15 +36,15 @@ describe("headless Chromium profile cleanup (issue #7058)", () => { it("warns and leaves the directory instead of throwing when it stays locked (EBUSY)", async () => { const dir = await makeProfileDir(); const ebusy = Object.assign(new Error(`EBUSY: resource busy or locked, rm '${dir}'`), { code: "EBUSY" }); - const rmSpy = spyOn(fs.promises, "rm").mockRejectedValue(ebusy); - const warnSpy = spyOn(logger, "warn"); + const removeSpy = spyOn(piUtils, "removeWithRetries").mockRejectedValue(ebusy); + const warnSpy = spyOn(piUtils.logger, "warn"); try { // Must resolve — a cleanup failure never propagates as a crash. await expect(removeUserDataDir(dir)).resolves.toBeUndefined(); - expect(rmSpy).toHaveBeenCalledTimes(1); + expect(removeSpy).toHaveBeenCalledTimes(1); expect(warnSpy).toHaveBeenCalledTimes(1); } finally { - rmSpy.mockRestore(); + removeSpy.mockRestore(); // Real removal so the fixture does not leak. await fs.promises.rm(dir, { recursive: true, force: true }); }