fix(browser): bounded tab teardown waits

- Applied close deadlines to cmux surfaces, orphan targets, and browser handles.
- Surfaced the backend, tab name, and pending cleanup resource on timeout.
- Forced stuck headless browser processes down after Browser.close timed out.

Fixes #5259
This commit is contained in:
roboomp
2026-07-14 17:36:41 +00:00
parent 20c0a2e410
commit 7b399b32a2
5 changed files with 136 additions and 19 deletions
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Fixed
- Fixed browser tabs hanging indefinitely at `Closing <tab>` when a worker, CDP target, browser process, or cmux surface stalls during teardown; close deadlines now release the operation with backend, tab, and pending-resource diagnostics. ([#5259](https://github.com/can1357/oh-my-pi/issues/5259))
## [16.4.6] - 2026-07-12
### Added
+4 -3
View File
@@ -199,7 +199,7 @@ export class BrowserTool implements AgentTool<typeof browserSchema, BrowserToolD
case "open":
return await this.#open(name, params, details, timeoutMs, signal);
case "close":
return await this.#close(name, params, details, signal);
return await this.#close(name, params, details, timeoutMs, signal);
case "run":
return await this.#run(name, params, details, timeoutMs, signal);
default:
@@ -284,15 +284,16 @@ export class BrowserTool implements AgentTool<typeof browserSchema, BrowserToolD
name: string,
params: BrowserParams,
details: BrowserToolDetails,
timeoutMs: number,
signal?: AbortSignal,
): Promise<AgentToolResult<BrowserToolDetails>> {
const kill = !!params.kill;
if (params.all) {
const count = await untilAborted(signal, () => releaseAllTabs({ kill }));
const count = await untilAborted(signal, () => releaseAllTabs({ kill, timeoutMs }));
details.result = `Closed ${count} tab(s)`;
return toolResult(details).text(details.result).done();
}
const closed = await untilAborted(signal, () => releaseTab(name, { kill }));
const closed = await untilAborted(signal, () => releaseTab(name, { kill, timeoutMs }));
details.result = closed ? `Closed tab ${JSON.stringify(name)}` : `No tab named ${JSON.stringify(name)}`;
return toolResult(details).text(details.result).done();
}
@@ -1,5 +1,5 @@
import * as path from "node:path";
import { logger } from "@oh-my-pi/pi-utils";
import { logger, withTimeout } from "@oh-my-pi/pi-utils";
import type { Subprocess } from "bun";
import type { Browser, CDPSession } from "puppeteer-core";
import { ToolAbortError, ToolError } from "../tool-errors";
@@ -40,6 +40,15 @@ export interface CmuxBrowserHandle extends BrowserHandleCommon {
export type BrowserHandle = PuppeteerBrowserHandle | CmuxBrowserHandle;
/** Controls bounded browser-handle teardown and identifies the owning resource in timeout diagnostics. */
export interface ReleaseBrowserOptions {
kill: boolean;
timeoutMs?: number;
resource?: string;
}
const DEFAULT_BROWSER_CLOSE_TIMEOUT_MS = 5_000;
const browsers = new Map<string, BrowserHandle>();
function browserKey(kind: BrowserKind): string {
@@ -214,7 +223,7 @@ export function holdBrowser(handle: BrowserHandle): void {
handle.refCount++;
}
export async function releaseBrowser(handle: BrowserHandle, opts: { kill: boolean }): Promise<void> {
export async function releaseBrowser(handle: BrowserHandle, opts: ReleaseBrowserOptions): Promise<void> {
handle.refCount = Math.max(0, handle.refCount - 1);
if (handle.refCount === 0) {
// Only evict if the registry still points at THIS handle. After a disconnect,
@@ -225,17 +234,32 @@ export async function releaseBrowser(handle: BrowserHandle, opts: { kill: boolea
}
}
async function disposeBrowserHandle(handle: BrowserHandle, opts: { kill: boolean }): Promise<void> {
async function disposeBrowserHandle(handle: BrowserHandle, opts: ReleaseBrowserOptions): Promise<void> {
if ("client" in handle) {
handle.client.close();
return;
}
if (handle.kind.kind === "headless") {
if (handle.browser.connected) {
const timeoutMs = opts.timeoutMs ?? DEFAULT_BROWSER_CLOSE_TIMEOUT_MS;
const resource = opts.resource ?? handle.key;
const timeoutMessage = `Timed out after ${timeoutMs}ms closing headless browser for ${resource}; pending resource: Puppeteer Browser.close()`;
try {
await handle.browser.close();
await withTimeout(handle.browser.close(), timeoutMs, timeoutMessage);
} catch (err) {
logger.debug("Failed to close headless browser", { error: (err as Error).message });
if (err instanceof Error && err.message === timeoutMessage) {
const process = handle.browser.process();
try {
handle.browser.disconnect();
} catch {}
try {
process?.kill();
} catch {}
throw new ToolError(timeoutMessage);
}
logger.debug("Failed to close headless browser", {
error: err instanceof Error ? err.message : String(err),
});
}
}
return;
@@ -1,4 +1,4 @@
import { getPuppeteerDir, logger, postmortem, Snowflake, workerHostEntry } from "@oh-my-pi/pi-utils";
import { getPuppeteerDir, logger, postmortem, Snowflake, withTimeout, workerHostEntry } from "@oh-my-pi/pi-utils";
import type { Page, Target } from "puppeteer-core";
import { callSessionTool } from "../../eval/js/tool-bridge";
import { webpExclusionForModel } from "../../utils/image-loading";
@@ -123,6 +123,8 @@ export interface RunInTabOptions {
export interface ReleaseTabOptions {
kill?: boolean;
/** Maximum time for each asynchronous cleanup resource before close fails with diagnostics. */
timeoutMs?: number;
}
const tabs = new Map<string, TabSession>();
@@ -131,6 +133,22 @@ const tabs = new Map<string, TabSession>();
// awaits) cannot interleave and leak a worker + browser refCount.
const acquireChains = new Map<string, Promise<void>>();
const GRACE_MS = 750;
const DEFAULT_TAB_CLOSE_TIMEOUT_MS = 5_000;
async function waitForTabCleanup<T>(
tab: TabSession,
timeoutMs: number,
pendingResource: string,
promise: Promise<T>,
): Promise<T> {
const message = `Timed out after ${timeoutMs}ms closing ${tab.kindTag} browser tab ${JSON.stringify(tab.name)}; pending resource: ${pendingResource}`;
try {
return await withTimeout(promise, timeoutMs, message);
} catch (error) {
if (error instanceof Error && error.message === message) throw new ToolError(message);
throw error;
}
}
export function getTab(name: string): TabSession | undefined {
return tabs.get(name);
@@ -507,26 +525,42 @@ export async function releaseTab(name: string, opts: ReleaseTabOptions = {}): Pr
pending.reject(closeError);
}
tab.pending.clear();
const timeoutMs = opts.timeoutMs ?? DEFAULT_TAB_CLOSE_TIMEOUT_MS;
if (tab.backend === "cmux") {
let nonLastCloseError: unknown;
let closeError: unknown;
if (wasAlive && tab.cmuxOwnsSurface) {
try {
await tab.browser.client.request("surface.close", { surface_id: tab.targetId });
await waitForTabCleanup(
tab,
timeoutMs,
`cmux surface ${JSON.stringify(tab.targetId)} (surface.close)`,
tab.browser.client.request("surface.close", { surface_id: tab.targetId }, { timeoutMs }),
);
} catch (err) {
if (isLastSurfaceCloseError(err)) {
logger.debug("Leaving cmux browser surface open because it is the last surface in the workspace", {
error: err instanceof Error ? err.message : String(err),
});
} else {
nonLastCloseError = err;
closeError = err;
}
}
}
await releaseBrowser(tab.browser, { kill: opts.kill ?? false });
tabs.delete(name);
if (nonLastCloseError) throw nonLastCloseError;
try {
await releaseBrowser(tab.browser, {
kill: opts.kill ?? false,
timeoutMs,
resource: `tab ${JSON.stringify(name)}`,
});
} catch (error) {
closeError ??= error;
} finally {
tabs.delete(name);
}
if (closeError) throw closeError;
return true;
}
let cleanupError: unknown;
let forced = false;
if (wasAlive) {
try {
@@ -537,9 +571,30 @@ export async function releaseTab(name: string, opts: ReleaseTabOptions = {}): Pr
}
}
await tab.worker.terminate().catch(() => undefined);
if (forced && tab.kindTag === "headless") await closeOrphanTarget(tab);
await releaseBrowser(tab.browser, { kill: opts.kill ?? false });
tabs.delete(name);
if (forced && tab.kindTag === "headless") {
try {
await waitForTabCleanup(
tab,
timeoutMs,
`orphan CDP target ${JSON.stringify(tab.targetId)} (Page.close)`,
closeOrphanTarget(tab),
);
} catch (error) {
cleanupError = error;
}
}
try {
await releaseBrowser(tab.browser, {
kill: opts.kill ?? false,
timeoutMs,
resource: `tab ${JSON.stringify(name)}`,
});
} catch (error) {
cleanupError ??= error;
} finally {
tabs.delete(name);
}
if (cleanupError) throw cleanupError;
return true;
}
@@ -16,7 +16,7 @@
* spied so no real cmux socket / puppeteer process is needed.
*/
import { afterEach, describe, expect, it, spyOn } from "bun:test";
import { afterEach, describe, expect, it, spyOn, vi } from "bun:test";
import type { CmuxKind } from "@oh-my-pi/pi-coding-agent/tools/browser/cmux/rpc";
import { CmuxSocketClient } from "@oh-my-pi/pi-coding-agent/tools/browser/cmux/socket-client";
import { acquireBrowser, getBrowsersMapForTest } from "@oh-my-pi/pi-coding-agent/tools/browser/registry";
@@ -173,3 +173,36 @@ describe("browser lifecycle — session-scoped teardown reaps owned tabs", () =>
expect(getTabsMapForTest().has("reuse-tab")).toBe(false);
});
});
describe("browser lifecycle — close deadlines", () => {
afterEach(async () => {
vi.useRealTimers();
vi.restoreAllMocks();
await drainAllTabs();
});
it("rejects a stuck close with the backend, tab, and pending resource", async () => {
vi.useFakeTimers();
spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined);
spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined);
const stuck = Promise.withResolvers<Record<string, unknown>>();
spyOn(CmuxSocketClient.prototype, "request").mockImplementation(async method => {
if (method === "browser.open_split") return { surface_id: "probe-surface", url: "about:blank" };
if (method === "surface.close") return await stuck.promise;
return {};
});
const kind: CmuxKind = { kind: "cmux", socketPath: "/tmp/omp-close-deadline.sock" };
const browser = await acquireBrowser(kind, { cwd: "/tmp" });
await acquireTab("probe", browser, { timeoutMs: 1_000 });
const close = releaseTab("probe", { timeoutMs: 100 });
vi.advanceTimersByTime(100);
await expect(close).rejects.toThrow(
'Timed out after 100ms closing cmux browser tab "probe"; pending resource: cmux surface "probe-surface" (surface.close)',
);
expect(getTabsMapForTest().has("probe")).toBe(false);
expect(getBrowsersMapForTest().size).toBe(0);
});
});