mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-12 15:30:39 +00:00
fix: prevent act:evaluate hangs from getting browser tool stuck/killed (#13498)
* fix(browser): prevent permanent timeout after stuck evaluate Thread AbortSignal from client-fetch through dispatcher to Playwright operations. When a timeout fires, force-disconnect the Playwright CDP connection to unblock the serialized command queue, allowing the next call to reconnect transparently. Key changes: - client-fetch.ts: proper AbortController with signal propagation - pw-session.ts: new forceDisconnectPlaywrightForTarget() - pw-tools-core.interactions.ts: accept signal, align inner timeout to outer-500ms, inject in-browser Promise.race for async evaluates - routes/dispatcher.ts + types.ts: propagate signal through dispatch - server.ts + bridge-server.ts: Express middleware creates AbortSignal from request lifecycle - client-actions-core.ts: add timeoutMs to evaluate type Fixes #10994 * fix(browser): v2 - force-disconnect via Connection.close() instead of browser.close() When page.evaluate() is stuck on a hung CDP transport, browser.close() also hangs because it tries to send a close command through the same stuck pipe. v2 fix: forceDisconnectPlaywrightForTarget now directly calls Playwright's internal Connection.close() which locally rejects all pending callbacks and emits 'disconnected' without touching the network. This instantly unblocks all stuck Playwright operations. closePlaywrightBrowserConnection (clean shutdown) now also has a 3s timeout fallback that drops to forceDropConnection if browser.close() hangs. Fixes permanent browser timeout after stuck evaluate. * fix(browser): v3 - fire-and-forget browser.close() instead of Connection.close() v2's forceDropConnection called browser._connection.close() which corrupts the entire Playwright instance because Connection is shared across all objects (BrowserType, Browser, Page, etc.). This prevented reconnection with cascading 'connectOverCDP: Force-disconnected' errors. v3 fix: forceDisconnectPlaywrightForTarget now: 1. Nulls cached connection immediately 2. Fire-and-forgets browser.close() (doesn't await — it may hang) 3. Next connectBrowser() creates a fresh connectOverCDP WebSocket Each connectOverCDP creates an independent WebSocket to the CDP endpoint, so the new connection is unaffected by the old one's pending close. The old browser.close() eventually resolves when the in-browser evaluate timeout fires, or the old connection gets GC'd. * fix(browser): v4 - clear connecting state and remove stale disconnect listeners The reconnect was failing because: 1. forceDisconnectPlaywrightForTarget nulled cached but not connecting, so subsequent calls could await a stale promise 2. The old browser's 'disconnected' event handler raced with new connections, nulling the fresh cached reference Fix: null both cached and connecting, and removeAllListeners on the old browser before fire-and-forget close. * fix(browser): v5 - use raw CDP Runtime.terminateExecution to kill stuck evaluate When forceDisconnectPlaywrightForTarget fires, open a raw WebSocket to the stuck page's CDP endpoint and send Runtime.terminateExecution. This kills running JS without navigating away or crashing the page. Also clear connecting state and remove stale disconnect listeners. * fix(browser): abort cancels stuck evaluate * Browser: always cleanup evaluate abort listener * Chore: remove Playwright debug scripts * Docs: add CDP evaluate refactor plan * Browser: refactor Playwright force-disconnect * Browser: abort stops evaluate promptly * Node host: extract withTimeout helper * Browser: remove disconnected listener safely * Changelog: note act:evaluate hang fix --------- Co-authored-by: Bob <bob@dutifulbob.com>
This commit is contained in:
@@ -28,6 +28,19 @@ export async function startBrowserBridgeServer(params: {
|
||||
const port = params.port ?? 0;
|
||||
|
||||
const app = express();
|
||||
app.use((req, res, next) => {
|
||||
const ctrl = new AbortController();
|
||||
const abort = () => ctrl.abort(new Error("request aborted"));
|
||||
req.once("aborted", abort);
|
||||
res.once("close", () => {
|
||||
if (!res.writableEnded) {
|
||||
abort();
|
||||
}
|
||||
});
|
||||
// Make the signal available to browser route handlers (best-effort).
|
||||
(req as unknown as { signal?: AbortSignal }).signal = ctrl.signal;
|
||||
next();
|
||||
});
|
||||
app.use(express.json({ limit: "1mb" }));
|
||||
|
||||
const authToken = params.authToken?.trim();
|
||||
|
||||
@@ -16,7 +16,11 @@ type Pending = {
|
||||
reject: (err: Error) => void;
|
||||
};
|
||||
|
||||
export type CdpSendFn = (method: string, params?: Record<string, unknown>) => Promise<unknown>;
|
||||
export type CdpSendFn = (
|
||||
method: string,
|
||||
params?: Record<string, unknown>,
|
||||
sessionId?: string,
|
||||
) => Promise<unknown>;
|
||||
|
||||
export function getHeadersWithAuth(url: string, headers: Record<string, string> = {}) {
|
||||
const relayHeaders = getChromeExtensionRelayAuthHeaders(url);
|
||||
@@ -51,9 +55,13 @@ function createCdpSender(ws: WebSocket) {
|
||||
let nextId = 1;
|
||||
const pending = new Map<number, Pending>();
|
||||
|
||||
const send: CdpSendFn = (method: string, params?: Record<string, unknown>) => {
|
||||
const send: CdpSendFn = (
|
||||
method: string,
|
||||
params?: Record<string, unknown>,
|
||||
sessionId?: string,
|
||||
) => {
|
||||
const id = nextId++;
|
||||
const msg = { id, method, params };
|
||||
const msg = { id, method, params, sessionId };
|
||||
ws.send(JSON.stringify(msg));
|
||||
return new Promise<unknown>((resolve, reject) => {
|
||||
pending.set(id, { resolve, reject });
|
||||
@@ -72,6 +80,10 @@ function createCdpSender(ws: WebSocket) {
|
||||
}
|
||||
};
|
||||
|
||||
ws.on("error", (err) => {
|
||||
closeWithError(err instanceof Error ? err : new Error(String(err)));
|
||||
});
|
||||
|
||||
ws.on("message", (data) => {
|
||||
try {
|
||||
const parsed = JSON.parse(rawDataToString(data)) as CdpResponse;
|
||||
@@ -132,11 +144,15 @@ export async function fetchOk(url: string, timeoutMs = 1500, init?: RequestInit)
|
||||
export async function withCdpSocket<T>(
|
||||
wsUrl: string,
|
||||
fn: (send: CdpSendFn) => Promise<T>,
|
||||
opts?: { headers?: Record<string, string> },
|
||||
opts?: { headers?: Record<string, string>; handshakeTimeoutMs?: number },
|
||||
): Promise<T> {
|
||||
const headers = getHeadersWithAuth(wsUrl, opts?.headers ?? {});
|
||||
const handshakeTimeoutMs =
|
||||
typeof opts?.handshakeTimeoutMs === "number" && Number.isFinite(opts.handshakeTimeoutMs)
|
||||
? Math.max(1, Math.floor(opts.handshakeTimeoutMs))
|
||||
: 5000;
|
||||
const ws = new WebSocket(wsUrl, {
|
||||
handshakeTimeout: 5000,
|
||||
handshakeTimeout: handshakeTimeoutMs,
|
||||
...(Object.keys(headers).length ? { headers } : {}),
|
||||
});
|
||||
const { send, closeWithError } = createCdpSender(ws);
|
||||
@@ -144,9 +160,15 @@ export async function withCdpSocket<T>(
|
||||
const openPromise = new Promise<void>((resolve, reject) => {
|
||||
ws.once("open", () => resolve());
|
||||
ws.once("error", (err) => reject(err));
|
||||
ws.once("close", () => reject(new Error("CDP socket closed")));
|
||||
});
|
||||
|
||||
await openPromise;
|
||||
try {
|
||||
await openPromise;
|
||||
} catch (err) {
|
||||
closeWithError(err instanceof Error ? err : new Error(String(err)));
|
||||
throw err;
|
||||
}
|
||||
|
||||
try {
|
||||
return await fn(send);
|
||||
|
||||
@@ -83,7 +83,7 @@ export type BrowserActRequest =
|
||||
targetId?: string;
|
||||
timeoutMs?: number;
|
||||
}
|
||||
| { kind: "evaluate"; fn: string; ref?: string; targetId?: string }
|
||||
| { kind: "evaluate"; fn: string; ref?: string; targetId?: string; timeoutMs?: number }
|
||||
| { kind: "close"; targetId?: string };
|
||||
|
||||
export type BrowserActResponse = {
|
||||
|
||||
@@ -35,7 +35,18 @@ async function fetchHttpJson<T>(
|
||||
): Promise<T> {
|
||||
const timeoutMs = init.timeoutMs ?? 5000;
|
||||
const ctrl = new AbortController();
|
||||
const t = setTimeout(() => ctrl.abort(), timeoutMs);
|
||||
const upstreamSignal = init.signal;
|
||||
let upstreamAbortListener: (() => void) | undefined;
|
||||
if (upstreamSignal) {
|
||||
if (upstreamSignal.aborted) {
|
||||
ctrl.abort(upstreamSignal.reason);
|
||||
} else {
|
||||
upstreamAbortListener = () => ctrl.abort(upstreamSignal.reason);
|
||||
upstreamSignal.addEventListener("abort", upstreamAbortListener, { once: true });
|
||||
}
|
||||
}
|
||||
|
||||
const t = setTimeout(() => ctrl.abort(new Error("timed out")), timeoutMs);
|
||||
try {
|
||||
const res = await fetch(url, { ...init, signal: ctrl.signal });
|
||||
if (!res.ok) {
|
||||
@@ -45,6 +56,9 @@ async function fetchHttpJson<T>(
|
||||
return (await res.json()) as T;
|
||||
} finally {
|
||||
clearTimeout(t);
|
||||
if (upstreamSignal && upstreamAbortListener) {
|
||||
upstreamSignal.removeEventListener("abort", upstreamAbortListener);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -75,6 +89,32 @@ export async function fetchBrowserJson<T>(
|
||||
// keep as string
|
||||
}
|
||||
}
|
||||
|
||||
const abortCtrl = new AbortController();
|
||||
const upstreamSignal = init?.signal;
|
||||
let upstreamAbortListener: (() => void) | undefined;
|
||||
if (upstreamSignal) {
|
||||
if (upstreamSignal.aborted) {
|
||||
abortCtrl.abort(upstreamSignal.reason);
|
||||
} else {
|
||||
upstreamAbortListener = () => abortCtrl.abort(upstreamSignal.reason);
|
||||
upstreamSignal.addEventListener("abort", upstreamAbortListener, { once: true });
|
||||
}
|
||||
}
|
||||
|
||||
let abortListener: (() => void) | undefined;
|
||||
const abortPromise: Promise<never> = abortCtrl.signal.aborted
|
||||
? Promise.reject(abortCtrl.signal.reason ?? new Error("aborted"))
|
||||
: new Promise((_, reject) => {
|
||||
abortListener = () => reject(abortCtrl.signal.reason ?? new Error("aborted"));
|
||||
abortCtrl.signal.addEventListener("abort", abortListener, { once: true });
|
||||
});
|
||||
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
if (timeoutMs) {
|
||||
timer = setTimeout(() => abortCtrl.abort(new Error("timed out")), timeoutMs);
|
||||
}
|
||||
|
||||
const dispatchPromise = dispatcher.dispatch({
|
||||
method:
|
||||
init?.method?.toUpperCase() === "DELETE"
|
||||
@@ -85,16 +125,20 @@ export async function fetchBrowserJson<T>(
|
||||
path: parsed.pathname,
|
||||
query,
|
||||
body,
|
||||
signal: abortCtrl.signal,
|
||||
});
|
||||
|
||||
const result = await (timeoutMs
|
||||
? Promise.race([
|
||||
dispatchPromise,
|
||||
new Promise<never>((_, reject) =>
|
||||
setTimeout(() => reject(new Error("timed out")), timeoutMs),
|
||||
),
|
||||
])
|
||||
: dispatchPromise);
|
||||
const result = await Promise.race([dispatchPromise, abortPromise]).finally(() => {
|
||||
if (timer) {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
if (abortListener) {
|
||||
abortCtrl.signal.removeEventListener("abort", abortListener);
|
||||
}
|
||||
if (upstreamSignal && upstreamAbortListener) {
|
||||
upstreamSignal.removeEventListener("abort", upstreamAbortListener);
|
||||
}
|
||||
});
|
||||
|
||||
if (result.status >= 400) {
|
||||
const message =
|
||||
|
||||
@@ -4,6 +4,7 @@ export {
|
||||
closePlaywrightBrowserConnection,
|
||||
createPageViaPlaywright,
|
||||
ensurePageState,
|
||||
forceDisconnectPlaywrightForTarget,
|
||||
focusPageByTargetIdViaPlaywright,
|
||||
getPageForTargetId,
|
||||
listPagesViaPlaywright,
|
||||
|
||||
@@ -8,7 +8,8 @@ import type {
|
||||
} from "playwright-core";
|
||||
import { chromium } from "playwright-core";
|
||||
import { formatErrorMessage } from "../infra/errors.js";
|
||||
import { getHeadersWithAuth } from "./cdp.helpers.js";
|
||||
import { appendCdpPath, fetchJson, getHeadersWithAuth, withCdpSocket } from "./cdp.helpers.js";
|
||||
import { normalizeCdpWsUrl } from "./cdp.js";
|
||||
import { getChromeWebSocketUrl } from "./chrome.js";
|
||||
|
||||
export type BrowserConsoleMessage = {
|
||||
@@ -52,6 +53,7 @@ type TargetInfoResponse = {
|
||||
type ConnectedBrowser = {
|
||||
browser: Browser;
|
||||
cdpUrl: string;
|
||||
onDisconnected?: () => void;
|
||||
};
|
||||
|
||||
type PageState = {
|
||||
@@ -333,14 +335,15 @@ async function connectBrowser(cdpUrl: string): Promise<ConnectedBrowser> {
|
||||
const endpoint = wsUrl ?? normalized;
|
||||
const headers = getHeadersWithAuth(endpoint);
|
||||
const browser = await chromium.connectOverCDP(endpoint, { timeout, headers });
|
||||
const connected: ConnectedBrowser = { browser, cdpUrl: normalized };
|
||||
cached = connected;
|
||||
observeBrowser(browser);
|
||||
browser.on("disconnected", () => {
|
||||
const onDisconnected = () => {
|
||||
if (cached?.browser === browser) {
|
||||
cached = null;
|
||||
}
|
||||
});
|
||||
};
|
||||
const connected: ConnectedBrowser = { browser, cdpUrl: normalized, onDisconnected };
|
||||
cached = connected;
|
||||
browser.on("disconnected", onDisconnected);
|
||||
observeBrowser(browser);
|
||||
return connected;
|
||||
} catch (err) {
|
||||
lastErr = err;
|
||||
@@ -503,12 +506,168 @@ export function refLocator(page: Page, ref: string) {
|
||||
export async function closePlaywrightBrowserConnection(): Promise<void> {
|
||||
const cur = cached;
|
||||
cached = null;
|
||||
connecting = null;
|
||||
if (!cur) {
|
||||
return;
|
||||
}
|
||||
if (cur.onDisconnected && typeof cur.browser.off === "function") {
|
||||
cur.browser.off("disconnected", cur.onDisconnected);
|
||||
}
|
||||
await cur.browser.close().catch(() => {});
|
||||
}
|
||||
|
||||
function normalizeCdpHttpBaseForJsonEndpoints(cdpUrl: string): string {
|
||||
try {
|
||||
const url = new URL(cdpUrl);
|
||||
if (url.protocol === "ws:") {
|
||||
url.protocol = "http:";
|
||||
} else if (url.protocol === "wss:") {
|
||||
url.protocol = "https:";
|
||||
}
|
||||
url.pathname = url.pathname.replace(/\/devtools\/browser\/.*$/, "");
|
||||
url.pathname = url.pathname.replace(/\/cdp$/, "");
|
||||
return url.toString().replace(/\/$/, "");
|
||||
} catch {
|
||||
// Best-effort fallback for non-URL-ish inputs.
|
||||
return cdpUrl
|
||||
.replace(/^ws:/, "http:")
|
||||
.replace(/^wss:/, "https:")
|
||||
.replace(/\/devtools\/browser\/.*$/, "")
|
||||
.replace(/\/cdp$/, "")
|
||||
.replace(/\/$/, "");
|
||||
}
|
||||
}
|
||||
|
||||
function cdpSocketNeedsAttach(wsUrl: string): boolean {
|
||||
try {
|
||||
const pathname = new URL(wsUrl).pathname;
|
||||
return (
|
||||
pathname === "/cdp" || pathname.endsWith("/cdp") || pathname.includes("/devtools/browser/")
|
||||
);
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
async function tryTerminateExecutionViaCdp(opts: {
|
||||
cdpUrl: string;
|
||||
targetId: string;
|
||||
}): Promise<void> {
|
||||
const cdpHttpBase = normalizeCdpHttpBaseForJsonEndpoints(opts.cdpUrl);
|
||||
const listUrl = appendCdpPath(cdpHttpBase, "/json/list");
|
||||
|
||||
const pages = await fetchJson<
|
||||
Array<{
|
||||
id?: string;
|
||||
webSocketDebuggerUrl?: string;
|
||||
}>
|
||||
>(listUrl, 2000).catch(() => null);
|
||||
if (!pages || pages.length === 0) {
|
||||
return;
|
||||
}
|
||||
|
||||
const target = pages.find((p) => String(p.id ?? "").trim() === opts.targetId);
|
||||
const wsUrlRaw = String(target?.webSocketDebuggerUrl ?? "").trim();
|
||||
if (!wsUrlRaw) {
|
||||
return;
|
||||
}
|
||||
const wsUrl = normalizeCdpWsUrl(wsUrlRaw, cdpHttpBase);
|
||||
const needsAttach = cdpSocketNeedsAttach(wsUrl);
|
||||
|
||||
const runWithTimeout = async <T>(work: Promise<T>, ms: number): Promise<T> => {
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
const timeoutPromise = new Promise<never>((_, reject) => {
|
||||
timer = setTimeout(() => reject(new Error("CDP command timed out")), ms);
|
||||
});
|
||||
try {
|
||||
return await Promise.race([work, timeoutPromise]);
|
||||
} finally {
|
||||
if (timer) {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
await withCdpSocket(
|
||||
wsUrl,
|
||||
async (send) => {
|
||||
let sessionId: string | undefined;
|
||||
try {
|
||||
if (needsAttach) {
|
||||
const attached = (await runWithTimeout(
|
||||
send("Target.attachToTarget", { targetId: opts.targetId, flatten: true }),
|
||||
1500,
|
||||
)) as { sessionId?: unknown };
|
||||
if (typeof attached?.sessionId === "string" && attached.sessionId.trim()) {
|
||||
sessionId = attached.sessionId;
|
||||
}
|
||||
}
|
||||
await runWithTimeout(send("Runtime.terminateExecution", undefined, sessionId), 1500);
|
||||
if (sessionId) {
|
||||
// Best-effort cleanup; not required for termination to take effect.
|
||||
void send("Target.detachFromTarget", { sessionId }).catch(() => {});
|
||||
}
|
||||
} catch {
|
||||
// Best-effort; ignore
|
||||
}
|
||||
},
|
||||
{ handshakeTimeoutMs: 2000 },
|
||||
).catch(() => {});
|
||||
}
|
||||
|
||||
/**
|
||||
* Best-effort cancellation for stuck page operations.
|
||||
*
|
||||
* Playwright serializes CDP commands per page; a long-running or stuck operation (notably evaluate)
|
||||
* can block all subsequent commands. We cannot safely "cancel" an individual command, and we do
|
||||
* not want to close the actual Chromium tab. Instead, we disconnect Playwright's CDP connection
|
||||
* so in-flight commands fail fast and the next request reconnects transparently.
|
||||
*
|
||||
* IMPORTANT: We CANNOT call Connection.close() because Playwright shares a single Connection
|
||||
* across all objects (BrowserType, Browser, etc.). Closing it corrupts the entire Playwright
|
||||
* instance, preventing reconnection.
|
||||
*
|
||||
* Instead we:
|
||||
* 1. Null out `cached` so the next call triggers a fresh connectOverCDP
|
||||
* 2. Fire-and-forget browser.close() — it may hang but won't block us
|
||||
* 3. The next connectBrowser() creates a completely new CDP WebSocket connection
|
||||
*
|
||||
* The old browser.close() eventually resolves when the in-browser evaluate timeout fires,
|
||||
* or the old connection gets GC'd. Either way, it doesn't affect the fresh connection.
|
||||
*/
|
||||
export async function forceDisconnectPlaywrightForTarget(opts: {
|
||||
cdpUrl: string;
|
||||
targetId?: string;
|
||||
reason?: string;
|
||||
}): Promise<void> {
|
||||
const normalized = normalizeCdpUrl(opts.cdpUrl);
|
||||
if (cached?.cdpUrl !== normalized) {
|
||||
return;
|
||||
}
|
||||
const cur = cached;
|
||||
cached = null;
|
||||
// Also clear `connecting` so the next call does a fresh connectOverCDP
|
||||
// rather than awaiting a stale promise.
|
||||
connecting = null;
|
||||
if (cur) {
|
||||
// Remove the "disconnected" listener to prevent the old browser's teardown
|
||||
// from racing with a fresh connection and nulling the new `cached`.
|
||||
if (cur.onDisconnected && typeof cur.browser.off === "function") {
|
||||
cur.browser.off("disconnected", cur.onDisconnected);
|
||||
}
|
||||
|
||||
// Best-effort: kill any stuck JS to unblock the target's execution context before we
|
||||
// disconnect Playwright's CDP connection.
|
||||
const targetId = opts.targetId?.trim() || "";
|
||||
if (targetId) {
|
||||
await tryTerminateExecutionViaCdp({ cdpUrl: normalized, targetId }).catch(() => {});
|
||||
}
|
||||
|
||||
// Fire-and-forget: don't await because browser.close() may hang on the stuck CDP pipe.
|
||||
cur.browser.close().catch(() => {});
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* List all pages/tabs from the persistent Playwright connection.
|
||||
* Used for remote profiles where HTTP-based /json/list is ephemeral.
|
||||
|
||||
@@ -0,0 +1,95 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
|
||||
let page: { evaluate: ReturnType<typeof vi.fn> } | null = null;
|
||||
let locator: { evaluate: ReturnType<typeof vi.fn> } | null = null;
|
||||
|
||||
const forceDisconnectPlaywrightForTarget = vi.fn(async () => {});
|
||||
const getPageForTargetId = vi.fn(async () => {
|
||||
if (!page) {
|
||||
throw new Error("test: page not set");
|
||||
}
|
||||
return page;
|
||||
});
|
||||
const ensurePageState = vi.fn(() => {});
|
||||
const restoreRoleRefsForTarget = vi.fn(() => {});
|
||||
const refLocator = vi.fn(() => {
|
||||
if (!locator) {
|
||||
throw new Error("test: locator not set");
|
||||
}
|
||||
return locator;
|
||||
});
|
||||
|
||||
vi.mock("./pw-session.js", () => {
|
||||
return {
|
||||
ensurePageState,
|
||||
forceDisconnectPlaywrightForTarget,
|
||||
getPageForTargetId,
|
||||
refLocator,
|
||||
restoreRoleRefsForTarget,
|
||||
};
|
||||
});
|
||||
|
||||
describe("evaluateViaPlaywright (abort)", () => {
|
||||
it("rejects when aborted after page.evaluate starts", async () => {
|
||||
vi.clearAllMocks();
|
||||
const ctrl = new AbortController();
|
||||
|
||||
let evalCalled!: () => void;
|
||||
const evalCalledPromise = new Promise<void>((resolve) => {
|
||||
evalCalled = resolve;
|
||||
});
|
||||
|
||||
page = {
|
||||
evaluate: vi.fn(() => {
|
||||
evalCalled();
|
||||
return new Promise(() => {});
|
||||
}),
|
||||
};
|
||||
locator = { evaluate: vi.fn() };
|
||||
|
||||
const { evaluateViaPlaywright } = await import("./pw-tools-core.interactions.js");
|
||||
const p = evaluateViaPlaywright({
|
||||
cdpUrl: "http://127.0.0.1:9222",
|
||||
fn: "() => 1",
|
||||
signal: ctrl.signal,
|
||||
});
|
||||
|
||||
await evalCalledPromise;
|
||||
ctrl.abort(new Error("aborted by test"));
|
||||
|
||||
await expect(p).rejects.toThrow("aborted by test");
|
||||
expect(forceDisconnectPlaywrightForTarget).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects when aborted after locator.evaluate starts", async () => {
|
||||
vi.clearAllMocks();
|
||||
const ctrl = new AbortController();
|
||||
|
||||
let evalCalled!: () => void;
|
||||
const evalCalledPromise = new Promise<void>((resolve) => {
|
||||
evalCalled = resolve;
|
||||
});
|
||||
|
||||
page = { evaluate: vi.fn() };
|
||||
locator = {
|
||||
evaluate: vi.fn(() => {
|
||||
evalCalled();
|
||||
return new Promise(() => {});
|
||||
}),
|
||||
};
|
||||
|
||||
const { evaluateViaPlaywright } = await import("./pw-tools-core.interactions.js");
|
||||
const p = evaluateViaPlaywright({
|
||||
cdpUrl: "http://127.0.0.1:9222",
|
||||
fn: "(el) => el.textContent",
|
||||
ref: "e1",
|
||||
signal: ctrl.signal,
|
||||
});
|
||||
|
||||
await evalCalledPromise;
|
||||
ctrl.abort(new Error("aborted by test"));
|
||||
|
||||
await expect(p).rejects.toThrow("aborted by test");
|
||||
expect(forceDisconnectPlaywrightForTarget).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,7 @@
|
||||
import type { BrowserFormField } from "./client-actions-core.js";
|
||||
import {
|
||||
ensurePageState,
|
||||
forceDisconnectPlaywrightForTarget,
|
||||
getPageForTargetId,
|
||||
refLocator,
|
||||
restoreRoleRefsForTarget,
|
||||
@@ -221,6 +222,8 @@ export async function evaluateViaPlaywright(opts: {
|
||||
targetId?: string;
|
||||
fn: string;
|
||||
ref?: string;
|
||||
timeoutMs?: number;
|
||||
signal?: AbortSignal;
|
||||
}): Promise<unknown> {
|
||||
const fnText = String(opts.fn ?? "").trim();
|
||||
if (!fnText) {
|
||||
@@ -229,42 +232,139 @@ export async function evaluateViaPlaywright(opts: {
|
||||
const page = await getPageForTargetId(opts);
|
||||
ensurePageState(page);
|
||||
restoreRoleRefsForTarget({ cdpUrl: opts.cdpUrl, targetId: opts.targetId, page });
|
||||
if (opts.ref) {
|
||||
const locator = refLocator(page, opts.ref);
|
||||
// Use Function constructor at runtime to avoid esbuild adding __name helper
|
||||
// which doesn't exist in the browser context
|
||||
// eslint-disable-next-line @typescript-eslint/no-implied-eval -- required for browser-context eval
|
||||
const elementEvaluator = new Function(
|
||||
"el",
|
||||
"fnBody",
|
||||
`
|
||||
"use strict";
|
||||
try {
|
||||
var candidate = eval("(" + fnBody + ")");
|
||||
return typeof candidate === "function" ? candidate(el) : candidate;
|
||||
} catch (err) {
|
||||
throw new Error("Invalid evaluate function: " + (err && err.message ? err.message : String(err)));
|
||||
}
|
||||
`,
|
||||
) as (el: Element, fnBody: string) => unknown;
|
||||
return await locator.evaluate(elementEvaluator, fnText);
|
||||
// Clamp evaluate timeout to prevent permanently blocking Playwright's command queue.
|
||||
// Without this, a long-running async evaluate blocks all subsequent page operations
|
||||
// because Playwright serializes CDP commands per page.
|
||||
//
|
||||
// NOTE: Playwright's { timeout } on evaluate only applies to installing the function,
|
||||
// NOT to its execution time. We must inject a Promise.race timeout into the browser
|
||||
// context itself so async functions are bounded.
|
||||
const outerTimeout = normalizeTimeoutMs(opts.timeoutMs, 20_000);
|
||||
// Leave headroom for routing/serialization overhead so the outer request timeout
|
||||
// doesn't fire first and strand a long-running evaluate.
|
||||
let evaluateTimeout = Math.max(1000, Math.min(120_000, outerTimeout - 500));
|
||||
evaluateTimeout = Math.min(evaluateTimeout, outerTimeout);
|
||||
|
||||
const signal = opts.signal;
|
||||
let abortListener: (() => void) | undefined;
|
||||
let abortReject: ((reason: unknown) => void) | undefined;
|
||||
let abortPromise: Promise<never> | undefined;
|
||||
if (signal) {
|
||||
abortPromise = new Promise((_, reject) => {
|
||||
abortReject = reject;
|
||||
});
|
||||
// Ensure the abort promise never becomes an unhandled rejection if we throw early.
|
||||
void abortPromise.catch(() => {});
|
||||
}
|
||||
// Use Function constructor at runtime to avoid esbuild adding __name helper
|
||||
// which doesn't exist in the browser context
|
||||
// eslint-disable-next-line @typescript-eslint/no-implied-eval -- required for browser-context eval
|
||||
const browserEvaluator = new Function(
|
||||
"fnBody",
|
||||
`
|
||||
"use strict";
|
||||
try {
|
||||
var candidate = eval("(" + fnBody + ")");
|
||||
return typeof candidate === "function" ? candidate() : candidate;
|
||||
} catch (err) {
|
||||
throw new Error("Invalid evaluate function: " + (err && err.message ? err.message : String(err)));
|
||||
if (signal) {
|
||||
const disconnect = () => {
|
||||
void forceDisconnectPlaywrightForTarget({
|
||||
cdpUrl: opts.cdpUrl,
|
||||
targetId: opts.targetId,
|
||||
reason: "evaluate aborted",
|
||||
}).catch(() => {});
|
||||
};
|
||||
if (signal.aborted) {
|
||||
disconnect();
|
||||
throw signal.reason ?? new Error("aborted");
|
||||
}
|
||||
`,
|
||||
) as (fnBody: string) => unknown;
|
||||
return await page.evaluate(browserEvaluator, fnText);
|
||||
abortListener = () => {
|
||||
disconnect();
|
||||
abortReject?.(signal.reason ?? new Error("aborted"));
|
||||
};
|
||||
signal.addEventListener("abort", abortListener, { once: true });
|
||||
// If the signal aborted between the initial check and listener registration, handle it.
|
||||
if (signal.aborted) {
|
||||
abortListener();
|
||||
throw signal.reason ?? new Error("aborted");
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
if (opts.ref) {
|
||||
const locator = refLocator(page, opts.ref);
|
||||
// eslint-disable-next-line @typescript-eslint/no-implied-eval -- required for browser-context eval
|
||||
const elementEvaluator = new Function(
|
||||
"el",
|
||||
"args",
|
||||
`
|
||||
"use strict";
|
||||
var fnBody = args.fnBody, timeoutMs = args.timeoutMs;
|
||||
try {
|
||||
var candidate = eval("(" + fnBody + ")");
|
||||
var result = typeof candidate === "function" ? candidate(el) : candidate;
|
||||
if (result && typeof result.then === "function") {
|
||||
return Promise.race([
|
||||
result,
|
||||
new Promise(function(_, reject) {
|
||||
setTimeout(function() { reject(new Error("evaluate timed out after " + timeoutMs + "ms")); }, timeoutMs);
|
||||
})
|
||||
]);
|
||||
}
|
||||
return result;
|
||||
} catch (err) {
|
||||
throw new Error("Invalid evaluate function: " + (err && err.message ? err.message : String(err)));
|
||||
}
|
||||
`,
|
||||
) as (el: Element, args: { fnBody: string; timeoutMs: number }) => unknown;
|
||||
const evalPromise = locator.evaluate(elementEvaluator, {
|
||||
fnBody: fnText,
|
||||
timeoutMs: evaluateTimeout,
|
||||
});
|
||||
if (!abortPromise) {
|
||||
return await evalPromise;
|
||||
}
|
||||
try {
|
||||
return await Promise.race([evalPromise, abortPromise]);
|
||||
} catch (err) {
|
||||
// If abort wins the race, the underlying evaluate may reject later; ensure we don't
|
||||
// surface it as an unhandled rejection.
|
||||
void evalPromise.catch(() => {});
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-implied-eval -- required for browser-context eval
|
||||
const browserEvaluator = new Function(
|
||||
"args",
|
||||
`
|
||||
"use strict";
|
||||
var fnBody = args.fnBody, timeoutMs = args.timeoutMs;
|
||||
try {
|
||||
var candidate = eval("(" + fnBody + ")");
|
||||
var result = typeof candidate === "function" ? candidate() : candidate;
|
||||
if (result && typeof result.then === "function") {
|
||||
return Promise.race([
|
||||
result,
|
||||
new Promise(function(_, reject) {
|
||||
setTimeout(function() { reject(new Error("evaluate timed out after " + timeoutMs + "ms")); }, timeoutMs);
|
||||
})
|
||||
]);
|
||||
}
|
||||
return result;
|
||||
} catch (err) {
|
||||
throw new Error("Invalid evaluate function: " + (err && err.message ? err.message : String(err)));
|
||||
}
|
||||
`,
|
||||
) as (args: { fnBody: string; timeoutMs: number }) => unknown;
|
||||
const evalPromise = page.evaluate(browserEvaluator, {
|
||||
fnBody: fnText,
|
||||
timeoutMs: evaluateTimeout,
|
||||
});
|
||||
if (!abortPromise) {
|
||||
return await evalPromise;
|
||||
}
|
||||
try {
|
||||
return await Promise.race([evalPromise, abortPromise]);
|
||||
} catch (err) {
|
||||
void evalPromise.catch(() => {});
|
||||
throw err;
|
||||
}
|
||||
} finally {
|
||||
if (signal && abortListener) {
|
||||
signal.removeEventListener("abort", abortListener);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
export async function scrollIntoViewViaPlaywright(opts: {
|
||||
|
||||
@@ -306,12 +306,18 @@ export function registerBrowserAgentActRoutes(
|
||||
return jsonError(res, 400, "fn is required");
|
||||
}
|
||||
const ref = toStringOrEmpty(body.ref) || undefined;
|
||||
const result = await pw.evaluateViaPlaywright({
|
||||
const evalTimeoutMs = toNumber(body.timeoutMs);
|
||||
const evalRequest: Parameters<typeof pw.evaluateViaPlaywright>[0] = {
|
||||
cdpUrl,
|
||||
targetId: tab.targetId,
|
||||
fn,
|
||||
ref,
|
||||
});
|
||||
signal: req.signal,
|
||||
};
|
||||
if (evalTimeoutMs !== undefined) {
|
||||
evalRequest.timeoutMs = evalTimeoutMs;
|
||||
}
|
||||
const result = await pw.evaluateViaPlaywright(evalRequest);
|
||||
return res.json({
|
||||
ok: true,
|
||||
targetId: tab.targetId,
|
||||
|
||||
46
src/browser/routes/dispatcher.abort.test.ts
Normal file
46
src/browser/routes/dispatcher.abort.test.ts
Normal file
@@ -0,0 +1,46 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import type { BrowserRouteContext } from "../server-context.js";
|
||||
|
||||
vi.mock("./index.js", () => {
|
||||
return {
|
||||
registerBrowserRoutes(app: { get: (path: string, handler: unknown) => void }) {
|
||||
app.get(
|
||||
"/slow",
|
||||
async (req: { signal?: AbortSignal }, res: { json: (body: unknown) => void }) => {
|
||||
const signal = req.signal;
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
if (signal?.aborted) {
|
||||
reject(signal.reason ?? new Error("aborted"));
|
||||
return;
|
||||
}
|
||||
const onAbort = () => reject(signal?.reason ?? new Error("aborted"));
|
||||
signal?.addEventListener("abort", onAbort, { once: true });
|
||||
setTimeout(resolve, 50);
|
||||
});
|
||||
res.json({ ok: true });
|
||||
},
|
||||
);
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
describe("browser route dispatcher (abort)", () => {
|
||||
it("propagates AbortSignal and lets handlers observe abort", async () => {
|
||||
const { createBrowserRouteDispatcher } = await import("./dispatcher.js");
|
||||
const dispatcher = createBrowserRouteDispatcher({} as BrowserRouteContext);
|
||||
|
||||
const ctrl = new AbortController();
|
||||
const promise = dispatcher.dispatch({
|
||||
method: "GET",
|
||||
path: "/slow",
|
||||
signal: ctrl.signal,
|
||||
});
|
||||
|
||||
ctrl.abort(new Error("timed out"));
|
||||
|
||||
await expect(promise).resolves.toMatchObject({
|
||||
status: 500,
|
||||
body: { error: expect.stringContaining("timed out") },
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -8,6 +8,7 @@ type BrowserDispatchRequest = {
|
||||
path: string;
|
||||
query?: Record<string, unknown>;
|
||||
body?: unknown;
|
||||
signal?: AbortSignal;
|
||||
};
|
||||
|
||||
type BrowserDispatchResponse = {
|
||||
@@ -68,6 +69,7 @@ export function createBrowserRouteDispatcher(ctx: BrowserRouteContext) {
|
||||
const path = normalizePath(req.path);
|
||||
const query = req.query ?? {};
|
||||
const body = req.body;
|
||||
const signal = req.signal;
|
||||
|
||||
const match = registry.routes.find((route) => {
|
||||
if (route.method !== method) {
|
||||
@@ -108,6 +110,7 @@ export function createBrowserRouteDispatcher(ctx: BrowserRouteContext) {
|
||||
params,
|
||||
query,
|
||||
body,
|
||||
signal,
|
||||
},
|
||||
res,
|
||||
);
|
||||
|
||||
@@ -2,6 +2,11 @@ export type BrowserRequest = {
|
||||
params: Record<string, string>;
|
||||
query: Record<string, unknown>;
|
||||
body?: unknown;
|
||||
/**
|
||||
* Optional abort signal for in-process dispatch. This lets callers enforce
|
||||
* timeouts and (where supported) cancel long-running operations.
|
||||
*/
|
||||
signal?: AbortSignal;
|
||||
};
|
||||
|
||||
export type BrowserResponse = {
|
||||
|
||||
@@ -357,12 +357,15 @@ describe("browser control server", () => {
|
||||
});
|
||||
expect(evalRes.ok).toBe(true);
|
||||
expect(evalRes.result).toBe("ok");
|
||||
expect(pwMocks.evaluateViaPlaywright).toHaveBeenCalledWith({
|
||||
cdpUrl: cdpBaseUrl,
|
||||
targetId: "abcd1234",
|
||||
fn: "() => 1",
|
||||
ref: undefined,
|
||||
});
|
||||
expect(pwMocks.evaluateViaPlaywright).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
cdpUrl: cdpBaseUrl,
|
||||
targetId: "abcd1234",
|
||||
fn: "() => 1",
|
||||
ref: undefined,
|
||||
signal: expect.any(AbortSignal),
|
||||
}),
|
||||
);
|
||||
},
|
||||
slowTimeoutMs,
|
||||
);
|
||||
|
||||
@@ -24,6 +24,19 @@ export async function startBrowserControlServerFromConfig(): Promise<BrowserServ
|
||||
}
|
||||
|
||||
const app = express();
|
||||
app.use((req, res, next) => {
|
||||
const ctrl = new AbortController();
|
||||
const abort = () => ctrl.abort(new Error("request aborted"));
|
||||
req.once("aborted", abort);
|
||||
res.once("close", () => {
|
||||
if (!res.writableEnded) {
|
||||
abort();
|
||||
}
|
||||
});
|
||||
// Make the signal available to browser route handlers (best-effort).
|
||||
(req as unknown as { signal?: AbortSignal }).signal = ctrl.signal;
|
||||
next();
|
||||
});
|
||||
app.use(express.json({ limit: "1mb" }));
|
||||
|
||||
const ctx = createBrowserRouteContext({
|
||||
|
||||
@@ -45,6 +45,7 @@ import { detectMime } from "../media/mime.js";
|
||||
import { GATEWAY_CLIENT_MODES, GATEWAY_CLIENT_NAMES } from "../utils/message-channel.js";
|
||||
import { VERSION } from "../version.js";
|
||||
import { ensureNodeHostConfig, saveNodeHostConfig, type NodeHostGatewayConfig } from "./config.js";
|
||||
import { withTimeout } from "./with-timeout.js";
|
||||
|
||||
type NodeHostRunOptions = {
|
||||
gatewayHost: string;
|
||||
@@ -275,29 +276,6 @@ async function ensureBrowserControlService(): Promise<void> {
|
||||
return browserControlReady;
|
||||
}
|
||||
|
||||
async function withTimeout<T>(promise: Promise<T>, timeoutMs?: number, label?: string): Promise<T> {
|
||||
const resolved =
|
||||
typeof timeoutMs === "number" && Number.isFinite(timeoutMs)
|
||||
? Math.max(1, Math.floor(timeoutMs))
|
||||
: undefined;
|
||||
if (!resolved) {
|
||||
return await promise;
|
||||
}
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
const timeoutPromise = new Promise<never>((_, reject) => {
|
||||
timer = setTimeout(() => {
|
||||
reject(new Error(`${label ?? "request"} timed out`));
|
||||
}, resolved);
|
||||
});
|
||||
try {
|
||||
return await Promise.race([promise, timeoutPromise]);
|
||||
} finally {
|
||||
if (timer) {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
function isProfileAllowed(params: { allowProfiles: string[]; profile?: string | null }) {
|
||||
const { allowProfiles, profile } = params;
|
||||
if (!allowProfiles.length) {
|
||||
@@ -790,12 +768,14 @@ async function handleInvoke(
|
||||
}
|
||||
const dispatcher = createBrowserRouteDispatcher(createBrowserControlContext());
|
||||
const response = await withTimeout(
|
||||
dispatcher.dispatch({
|
||||
method: method === "DELETE" ? "DELETE" : method === "POST" ? "POST" : "GET",
|
||||
path,
|
||||
query,
|
||||
body,
|
||||
}),
|
||||
(signal) =>
|
||||
dispatcher.dispatch({
|
||||
method: method === "DELETE" ? "DELETE" : method === "POST" ? "POST" : "GET",
|
||||
path,
|
||||
query,
|
||||
body,
|
||||
signal,
|
||||
}),
|
||||
params.timeoutMs,
|
||||
"browser proxy request",
|
||||
);
|
||||
|
||||
34
src/node-host/with-timeout.ts
Normal file
34
src/node-host/with-timeout.ts
Normal file
@@ -0,0 +1,34 @@
|
||||
export async function withTimeout<T>(
|
||||
work: (signal: AbortSignal | undefined) => Promise<T>,
|
||||
timeoutMs?: number,
|
||||
label?: string,
|
||||
): Promise<T> {
|
||||
const resolved =
|
||||
typeof timeoutMs === "number" && Number.isFinite(timeoutMs)
|
||||
? Math.max(1, Math.floor(timeoutMs))
|
||||
: undefined;
|
||||
if (!resolved) {
|
||||
return await work(undefined);
|
||||
}
|
||||
|
||||
const abortCtrl = new AbortController();
|
||||
const timeoutError = new Error(`${label ?? "request"} timed out`);
|
||||
const timer = setTimeout(() => abortCtrl.abort(timeoutError), resolved);
|
||||
|
||||
let abortListener: (() => void) | undefined;
|
||||
const abortPromise: Promise<never> = abortCtrl.signal.aborted
|
||||
? Promise.reject(abortCtrl.signal.reason ?? timeoutError)
|
||||
: new Promise((_, reject) => {
|
||||
abortListener = () => reject(abortCtrl.signal.reason ?? timeoutError);
|
||||
abortCtrl.signal.addEventListener("abort", abortListener, { once: true });
|
||||
});
|
||||
|
||||
try {
|
||||
return await Promise.race([work(abortCtrl.signal), abortPromise]);
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
if (abortListener) {
|
||||
abortCtrl.signal.removeEventListener("abort", abortListener);
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user