diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d4f153182..4678bf9d9 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -135,6 +135,7 @@ - Fixed Anthropic prompt-cache cold misses on session resume with multiple OAuth accounts: the account that served a session is now recorded in the session file (as a `credential_pin` sha-256 of the account + org/project scope, so exports carry no plaintext identity) and re-pinned on resume with the session's effective last-use time, so a fresh process no longer re-ranks accounts by usage headroom — which systematically routed away from the just-used account and cold-missed the entire account-scoped cache prefix. Sticky routing was previously stored only in the auth store's KV cache, which is in-memory when a remote auth broker is configured. - Fixed Anthropic prompt-cache cold misses on session resume with multiple OAuth accounts: the account that served a session is now recorded in the session file (as a PII-free `credential_pin` hash) and re-pinned on resume, so a fresh process no longer re-ranks accounts by usage headroom — which systematically routed away from the just-used account and cold-missed the entire account-scoped cache prefix. Sticky routing was previously stored only in the auth store's KV cache, which is in-memory when a remote auth broker is configured. - Fixed concurrent `createAgentSession` calls with the default agent id failing initialization with `Agent "Main" was replaced during session initialization` — each in-process embedder (e.g. the edit benchmark runner) can now pass a private registry via the newly exported `AgentRegistry`, keeping every top-level session's "Main" out of the process-global roster race. +- 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..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" }; @@ -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,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 { + 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( 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..5e21b284e --- /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 * 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-")); + 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(piUtils, "removeWithRetries").mockRestore(); + spyOn(piUtils.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 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(removeSpy).toHaveBeenCalledTimes(1); + expect(warnSpy).toHaveBeenCalledTimes(1); + } finally { + removeSpy.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); + }); +});