fix(browser): bound open timeout and lease browser across tab acquisition
The browser tool's open action only passed the requested timeout to acquireTab; acquireBrowser ran under the caller signal alone, so CDP discovery/connect could run through its own fixed 5s/30s waits past the requested deadline. A freshly-created browser also sat in the registry at refCount 0 during worker/surface acquisition: the worker-abort branch released it only on tempHold (never on the fresh refCount-0 case), orphaning the handle, and two different-name opens sharing one refCount-0 browser let a single failure dispose it out from under the survivor. Compose one open deadline from the caller signal and params.timeout and thread it through both acquireBrowser and acquireTab; caller cancellation stays ToolAbortError, the requested timeout becomes a timeout ToolError. Hold one explicit registry lease across tab acquisition, released exactly once on the mutually-exclusive success/rollback paths, and make the worker-abort browser release mirror the error paths' refCount-0 check. Fixes #6365
This commit is contained in:
roboomp
@@ -7,6 +7,7 @@
|
||||
- Fixed credential-shaped tokens (GitHub/GitLab/OpenAI/Anthropic key patterns) being redacted from outbound provider requests even with `secrets.enabled` off; the pattern redaction now follows the `secrets.enabled` ("Hide Secrets") setting like the secret obfuscator.
|
||||
- Fixed Ctrl-clicking a wrapped OAuth authorization URL opening only the clicked row's truncated fragment by preserving the complete hyperlink target on every rendered row.
|
||||
- Fixed used-only absolute usage amounts across output surfaces: CLI now renders `$123.45 used`; the TUI shows a neutral, width-bounded amount instead of a pending/dotted/account-count placeholder; and ACP preserves `123.45 usd used` while suppressing duplicate window suffixes such as `— extra`. ([#5575](https://github.com/can1357/oh-my-pi/issues/5575))
|
||||
- Fixed the `browser` tool's `open` action ignoring the requested `timeout` during browser acquisition (CDP discovery/connect ran to its own fixed wait), and orphaning a freshly-created browser on abort/timeout before tab publication. The requested timeout now bounds the whole open lifecycle, and one explicit registry lease is held across tab acquisition so rollback disposes exactly the failed open — a concurrent open of a different tab name on the same browser can no longer dispose the browser out from under it. ([#6365](https://github.com/can1357/oh-my-pi/issues/6365))
|
||||
|
||||
## [17.0.9] - 2026-07-23
|
||||
|
||||
|
||||
@@ -7,9 +7,24 @@ import type { ToolSession } from "../sdk";
|
||||
import { enforceInlineByteCap } from "../session/streaming-output";
|
||||
import { truncateForPrompt } from "./approval";
|
||||
import { resolveCmuxKind } from "./browser/cmux/rpc";
|
||||
import { acquireBrowser, type BrowserHandle, type BrowserKind, type BrowserKindTag } from "./browser/registry";
|
||||
import {
|
||||
acquireBrowser,
|
||||
type BrowserHandle,
|
||||
type BrowserKind,
|
||||
type BrowserKindTag,
|
||||
holdBrowser,
|
||||
releaseBrowser,
|
||||
} from "./browser/registry";
|
||||
import type { Observation, ScreenshotResult } from "./browser/tab-protocol";
|
||||
import { acquireTab, dropHeadlessTabs, getTab, releaseAllTabs, releaseTab, runInTab } from "./browser/tab-supervisor";
|
||||
import {
|
||||
type AcquireTabResult,
|
||||
acquireTab,
|
||||
dropHeadlessTabs,
|
||||
getTab,
|
||||
releaseAllTabs,
|
||||
releaseTab,
|
||||
runInTab,
|
||||
} from "./browser/tab-supervisor";
|
||||
import type { OutputMeta } from "./output-meta";
|
||||
import { resolveToCwd } from "./path-utils";
|
||||
import { ToolAbortError, ToolError, throwIfAborted } from "./tool-errors";
|
||||
@@ -232,52 +247,84 @@ export class BrowserTool implements AgentTool<typeof browserSchema, BrowserToolD
|
||||
);
|
||||
}
|
||||
|
||||
const browser = await untilAborted(signal, () =>
|
||||
acquireBrowser(kind, {
|
||||
cwd: this.session.cwd,
|
||||
viewport: params.viewport
|
||||
? {
|
||||
width: params.viewport.width,
|
||||
height: params.viewport.height,
|
||||
deviceScaleFactor: params.viewport.scale,
|
||||
}
|
||||
: undefined,
|
||||
appArgs: params.app?.args,
|
||||
signal,
|
||||
}),
|
||||
);
|
||||
// The requested timeout must cover the *entire* open — browser
|
||||
// acquisition (CDP discovery/connect), queued tab acquisition, worker
|
||||
// creation, and navigation — not only `acquireTab`. Compose one deadline
|
||||
// from the caller signal and `params.timeout` and thread it through both
|
||||
// stages so a stalled acquisition rejects at the requested boundary.
|
||||
const timeoutSignal = AbortSignal.timeout(timeoutMs);
|
||||
const openSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal;
|
||||
try {
|
||||
const browser = await untilAborted(openSignal, () =>
|
||||
acquireBrowser(kind, {
|
||||
cwd: this.session.cwd,
|
||||
viewport: params.viewport
|
||||
? {
|
||||
width: params.viewport.width,
|
||||
height: params.viewport.height,
|
||||
deviceScaleFactor: params.viewport.scale,
|
||||
}
|
||||
: undefined,
|
||||
appArgs: params.app?.args,
|
||||
signal: openSignal,
|
||||
}),
|
||||
);
|
||||
|
||||
const result = await untilAborted(signal, () =>
|
||||
acquireTab(name, browser, {
|
||||
url: params.url,
|
||||
waitUntil: params.wait_until,
|
||||
viewport: params.viewport
|
||||
? {
|
||||
width: params.viewport.width,
|
||||
height: params.viewport.height,
|
||||
deviceScaleFactor: params.viewport.scale,
|
||||
}
|
||||
: undefined,
|
||||
target: params.app?.target,
|
||||
timeoutMs,
|
||||
dialogs: params.dialogs,
|
||||
signal,
|
||||
ownerSessionId: this.session.getSessionId?.() ?? undefined,
|
||||
}),
|
||||
);
|
||||
const tab = result.tab;
|
||||
const url = tab.info.url;
|
||||
const title = tab.info.title ?? "";
|
||||
details.url = url;
|
||||
details.viewport = tab.info.viewport;
|
||||
const verb = result.created ? "Opened" : "Reused";
|
||||
const lines = [
|
||||
`${verb} tab ${JSON.stringify(name)} on ${describeBrowser(browser)}`,
|
||||
`URL: ${url}`,
|
||||
title ? `Title: ${title}` : null,
|
||||
].filter((l): l is string => typeof l === "string");
|
||||
details.result = lines.join("\n");
|
||||
return toolResult(details).text(lines.join("\n")).done();
|
||||
// Hold one open-acquisition lease across the whole tab acquisition.
|
||||
// A freshly-created browser sits in the registry at refCount 0 until a
|
||||
// tab takes a hold; without this lease an abort/timeout mid-acquisition
|
||||
// (or a sibling open of a different tab name on the same browser that
|
||||
// fails) could dispose it out from under this operation. The lease is
|
||||
// released exactly once — the success and failure paths are mutually
|
||||
// exclusive — transferring ownership to the published tab on success or
|
||||
// rolling the fresh browser back on failure.
|
||||
holdBrowser(browser);
|
||||
let result: AcquireTabResult;
|
||||
try {
|
||||
result = await untilAborted(openSignal, () =>
|
||||
acquireTab(name, browser, {
|
||||
url: params.url,
|
||||
waitUntil: params.wait_until,
|
||||
viewport: params.viewport
|
||||
? {
|
||||
width: params.viewport.width,
|
||||
height: params.viewport.height,
|
||||
deviceScaleFactor: params.viewport.scale,
|
||||
}
|
||||
: undefined,
|
||||
target: params.app?.target,
|
||||
timeoutMs,
|
||||
dialogs: params.dialogs,
|
||||
signal: openSignal,
|
||||
ownerSessionId: this.session.getSessionId?.() ?? undefined,
|
||||
}),
|
||||
);
|
||||
} catch (error) {
|
||||
await releaseBrowser(browser, { kill: false });
|
||||
throw error;
|
||||
}
|
||||
await releaseBrowser(browser, { kill: false });
|
||||
|
||||
const tab = result.tab;
|
||||
const url = tab.info.url;
|
||||
const title = tab.info.title ?? "";
|
||||
details.url = url;
|
||||
details.viewport = tab.info.viewport;
|
||||
const verb = result.created ? "Opened" : "Reused";
|
||||
const lines = [
|
||||
`${verb} tab ${JSON.stringify(name)} on ${describeBrowser(browser)}`,
|
||||
`URL: ${url}`,
|
||||
title ? `Title: ${title}` : null,
|
||||
].filter((l): l is string => typeof l === "string");
|
||||
details.result = lines.join("\n");
|
||||
return toolResult(details).text(lines.join("\n")).done();
|
||||
} catch (error) {
|
||||
// Caller cancellation stays a ToolAbortError; the requested timeout
|
||||
// becomes a timeout ToolError; anything else passes through unchanged.
|
||||
if (signal?.aborted) throw error instanceof ToolAbortError ? error : new ToolAbortError();
|
||||
if (timeoutSignal.aborted) throw new ToolError(`Browser open timed out after ${timeoutMs}ms`);
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
async #close(
|
||||
|
||||
@@ -286,13 +286,16 @@ async function acquireTabImpl(
|
||||
}
|
||||
}
|
||||
|
||||
// If the caller aborted while we were spawning/initializing the worker,
|
||||
// tear the freshly-built worker down before publishing the tab so the
|
||||
// browser refCount (which `holdBrowser` below would take) never grows for
|
||||
// a tab nobody is waiting for.
|
||||
// If the caller aborted while we were spawning/initializing the worker, tear
|
||||
// the freshly-built worker down before publishing the tab so the browser
|
||||
// refCount (which `holdBrowser` below would take) never grows for a tab
|
||||
// nobody is waiting for. Mirror the error paths' `refCount === 0` release so
|
||||
// a fresh browser held by nothing but this aborted open is not orphaned in
|
||||
// the registry; a browser still leased/held elsewhere (refCount > 0) is left
|
||||
// for its owner to release.
|
||||
if (opts.signal?.aborted) {
|
||||
await worker.terminate().catch(() => undefined);
|
||||
if (tempHold) await releaseBrowser(browser, { kill: false }).catch(() => undefined);
|
||||
if (tempHold || browser.refCount === 0) await releaseBrowser(browser, { kill: false }).catch(() => undefined);
|
||||
throw new ToolAbortError("Browser tab open aborted");
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,211 @@
|
||||
/**
|
||||
* Regression tests for issue #6365: `BrowserTool.#open` must apply the
|
||||
* requested `timeout` to the *entire* open lifecycle (browser acquisition +
|
||||
* tab acquisition), and must hold one explicit browser lease across tab
|
||||
* acquisition so a refCount:0 browser is never orphaned by an abort/timeout
|
||||
* nor disposed out from under a concurrent open of a different tab name.
|
||||
*
|
||||
* The tool resolves the cmux backend (`CMUX_SOCKET_PATH` + settings), so
|
||||
* `CmuxSocketClient.prototype` is spied and no real socket / Chromium is used.
|
||||
*/
|
||||
|
||||
import { afterEach, beforeEach, describe, expect, it, spyOn, vi } from "bun:test";
|
||||
import { BrowserTool } from "@oh-my-pi/pi-coding-agent/tools/browser";
|
||||
import { CmuxSocketClient } from "@oh-my-pi/pi-coding-agent/tools/browser/cmux/socket-client";
|
||||
import { getBrowsersMapForTest } from "@oh-my-pi/pi-coding-agent/tools/browser/registry";
|
||||
import { getTabsMapForTest, releaseTab } from "@oh-my-pi/pi-coding-agent/tools/browser/tab-supervisor";
|
||||
import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools/index";
|
||||
import { ToolAbortError, ToolError } from "@oh-my-pi/pi-coding-agent/tools/tool-errors";
|
||||
|
||||
function makeSession(): ToolSession {
|
||||
return {
|
||||
cwd: "/tmp",
|
||||
hasUI: false,
|
||||
settings: {
|
||||
get: (key: string) => (key === "browser.cmux" ? true : key === "tools.maxTimeout" ? 0 : undefined),
|
||||
},
|
||||
getSessionId: () => "session-open-lease",
|
||||
} as unknown as ToolSession;
|
||||
}
|
||||
|
||||
async function drainAllTabs(): Promise<void> {
|
||||
for (const name of [...getTabsMapForTest().keys()]) {
|
||||
await releaseTab(name, { kill: false }).catch(() => undefined);
|
||||
}
|
||||
}
|
||||
|
||||
let prevSocketPath: string | undefined;
|
||||
|
||||
beforeEach(() => {
|
||||
prevSocketPath = process.env.CMUX_SOCKET_PATH;
|
||||
// Unique per test so the module-global browsers map (keyed by socket path)
|
||||
// never carries a handle across tests.
|
||||
process.env.CMUX_SOCKET_PATH = `/tmp/omp-open-lease-${process.pid}-${Math.random().toString(36).slice(2)}.sock`;
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
vi.useRealTimers();
|
||||
await drainAllTabs().catch(() => undefined);
|
||||
vi.restoreAllMocks();
|
||||
if (prevSocketPath === undefined) delete process.env.CMUX_SOCKET_PATH;
|
||||
else process.env.CMUX_SOCKET_PATH = prevSocketPath;
|
||||
});
|
||||
|
||||
describe("browser open — requested timeout bounds the whole acquisition (#6365)", () => {
|
||||
it("rejects with a timeout ToolError when browser acquisition stays pending past the deadline", async () => {
|
||||
vi.useFakeTimers();
|
||||
const connectGate = Promise.withResolvers<void>();
|
||||
spyOn(CmuxSocketClient.prototype, "connect").mockImplementation(async () => {
|
||||
await connectGate.promise;
|
||||
});
|
||||
const closeSpy = spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined);
|
||||
|
||||
const tool = new BrowserTool(makeSession());
|
||||
const open = tool.execute("call-timeout", { action: "open", name: "late", timeout: 1 });
|
||||
const settled = open.then(
|
||||
() => ({ ok: true as const }),
|
||||
(err: unknown) => ({ ok: false as const, err }),
|
||||
);
|
||||
|
||||
// The requested 1s deadline elapses while `acquireBrowser` is still
|
||||
// blocked on the (never-resolving) socket connect. Bun's fake timers fire
|
||||
// `AbortSignal.timeout` synchronously on advance; awaiting `settled` below
|
||||
// flushes the rejection.
|
||||
vi.advanceTimersByTime(1000);
|
||||
|
||||
const outcome = await settled;
|
||||
expect(outcome.ok).toBe(false);
|
||||
if (outcome.ok) throw new Error("unreachable");
|
||||
// The requested action timeout surfaces as a timeout ToolError — never a
|
||||
// ToolAbortError (that is reserved for caller cancellation).
|
||||
expect(outcome.err).toBeInstanceOf(ToolError);
|
||||
expect(outcome.err).not.toBeInstanceOf(ToolAbortError);
|
||||
expect((outcome.err as Error).message).toMatch(/timed out/i);
|
||||
|
||||
// Let the orphan launch resolve; the aborted deadline must dispose it so
|
||||
// no refCount:0 browser survives in the registry.
|
||||
connectGate.resolve();
|
||||
for (let i = 0; i < 20; i++) await Promise.resolve();
|
||||
expect(closeSpy).toHaveBeenCalledTimes(1);
|
||||
expect(getBrowsersMapForTest().size).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe("browser open — caller cancellation rolls back the fresh browser (#6365)", () => {
|
||||
it("aborting before tab publication rejects with ToolAbortError and leaves both maps empty", async () => {
|
||||
spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined);
|
||||
const closeSpy = spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined);
|
||||
const openSplitGate = Promise.withResolvers<void>();
|
||||
const surfaceClosed: string[] = [];
|
||||
spyOn(CmuxSocketClient.prototype, "request").mockImplementation(
|
||||
async (method: string, params: Record<string, unknown>): Promise<Record<string, unknown>> => {
|
||||
if (method === "browser.open_split") {
|
||||
await openSplitGate.promise;
|
||||
return { surface_id: "surface-abort", url: "about:blank" };
|
||||
}
|
||||
if (method === "surface.close") {
|
||||
surfaceClosed.push(String(params.surface_id));
|
||||
return {};
|
||||
}
|
||||
return {};
|
||||
},
|
||||
);
|
||||
|
||||
const tool = new BrowserTool(makeSession());
|
||||
const controller = new AbortController();
|
||||
const open = tool.execute("call-abort", { action: "open", name: "fresh", timeout: 30 }, controller.signal);
|
||||
const settled = open.then(
|
||||
() => ({ ok: true as const }),
|
||||
(err: unknown) => ({ ok: false as const, err }),
|
||||
);
|
||||
|
||||
// Browser acquisition has resolved; tab acquisition is parked in
|
||||
// `open_split`. Cancel here — before any tab is published.
|
||||
await Promise.resolve();
|
||||
controller.abort();
|
||||
|
||||
const outcome = await settled;
|
||||
expect(outcome.ok).toBe(false);
|
||||
if (outcome.ok) throw new Error("unreachable");
|
||||
expect(outcome.err).toBeInstanceOf(ToolAbortError);
|
||||
|
||||
// The open-acquisition lease rollback disposes the fresh browser exactly
|
||||
// once and leaves nothing owned solely by the failed open.
|
||||
expect(getTabsMapForTest().has("fresh")).toBe(false);
|
||||
expect(getBrowsersMapForTest().size).toBe(0);
|
||||
expect(closeSpy).toHaveBeenCalledTimes(1);
|
||||
|
||||
// Let the orphaned acquisition unwind so it does not leak past the test.
|
||||
openSplitGate.resolve();
|
||||
await Promise.resolve();
|
||||
});
|
||||
});
|
||||
|
||||
describe("browser open — concurrent different-name acquisitions each own a lease (#6365)", () => {
|
||||
it("aborting one open releases only its lease; the survivor keeps the browser and one tab", async () => {
|
||||
spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined);
|
||||
const closeSpy = spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined);
|
||||
const openGate = Promise.withResolvers<void>();
|
||||
let splitCount = 0;
|
||||
const aEntered = Promise.withResolvers<void>();
|
||||
const bEntered = Promise.withResolvers<void>();
|
||||
const surfaceClosed: string[] = [];
|
||||
spyOn(CmuxSocketClient.prototype, "request").mockImplementation(
|
||||
async (method: string, params: Record<string, unknown>): Promise<Record<string, unknown>> => {
|
||||
if (method === "browser.open_split") {
|
||||
const id = `surface-${++splitCount}`;
|
||||
(splitCount === 1 ? aEntered : bEntered).resolve();
|
||||
await openGate.promise;
|
||||
return { surface_id: id, url: "about:blank" };
|
||||
}
|
||||
if (method === "surface.close") {
|
||||
surfaceClosed.push(String(params.surface_id));
|
||||
return {};
|
||||
}
|
||||
return {};
|
||||
},
|
||||
);
|
||||
|
||||
const tool = new BrowserTool(makeSession());
|
||||
|
||||
// Open A first and wait until it is parked inside `open_split` — proof it
|
||||
// acquired the shared browser and took its open-acquisition lease.
|
||||
const controllerA = new AbortController();
|
||||
const openA = tool.execute("call-a", { action: "open", name: "tab-a", timeout: 30 }, controllerA.signal);
|
||||
const settledA = openA.then(
|
||||
() => ({ ok: true as const }),
|
||||
(err: unknown) => ({ ok: false as const, err }),
|
||||
);
|
||||
await aEntered.promise;
|
||||
expect(getBrowsersMapForTest().size).toBe(1);
|
||||
|
||||
// Open B against the SAME browser (different tab name). It reuses the
|
||||
// registry handle and takes its own lease; both are now parked.
|
||||
const openB = tool.execute("call-b", { action: "open", name: "tab-b", timeout: 30 });
|
||||
await bEntered.promise;
|
||||
|
||||
// Abort A while both are queued; releasing A's lease must not dispose the
|
||||
// browser B still needs.
|
||||
controllerA.abort();
|
||||
const outcomeA = await settledA;
|
||||
expect(outcomeA.ok).toBe(false);
|
||||
if (outcomeA.ok) throw new Error("unreachable");
|
||||
expect(outcomeA.err).toBeInstanceOf(ToolAbortError);
|
||||
|
||||
// Release the gate so B publishes its tab.
|
||||
openGate.resolve();
|
||||
const resultB = await openB;
|
||||
expect(resultB.content.some(part => part.type === "text" && /Opened tab "tab-b"/.test(part.text ?? ""))).toBe(
|
||||
true,
|
||||
);
|
||||
|
||||
// B's browser survived A's rollback: still present, never closed, exactly
|
||||
// one published tab. A's rollback closed only its own orphan surface.
|
||||
expect(getBrowsersMapForTest().size).toBe(1);
|
||||
expect(closeSpy).not.toHaveBeenCalled();
|
||||
expect(getTabsMapForTest().has("tab-b")).toBe(true);
|
||||
expect(getTabsMapForTest().has("tab-a")).toBe(false);
|
||||
expect(getTabsMapForTest().size).toBe(1);
|
||||
expect(surfaceClosed).toEqual(["surface-1"]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user