mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 06:50:43 +00:00
fix(browser): reset Chrome MCP session after navigate_page timeout
navigateChromeMcpPage() now always passes a timeout to the Chrome MCP navigate_page tool (defaulting to CHROME_MCP_NAVIGATE_TIMEOUT_MS when the caller omits timeoutMs), and callTool() grows an optional safety-net that tears down a stuck session via Promise.race so the next caller gets a fresh subprocess. The catch block gains a transport-identity guard to avoid clobbering a concurrently-created replacement session.
This commit is contained in:
committed by
Peter Steinberger
parent
ef66798433
commit
dc9c46a8df
@@ -1,8 +1,9 @@
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
buildChromeMcpArgs,
|
||||
evaluateChromeMcpScript,
|
||||
listChromeMcpTabs,
|
||||
navigateChromeMcpPage,
|
||||
openChromeMcpTab,
|
||||
resetChromeMcpSessionsForTest,
|
||||
setChromeMcpSessionFactoryForTest,
|
||||
@@ -97,6 +98,10 @@ describe("chrome MCP page parsing", () => {
|
||||
await resetChromeMcpSessionsForTest();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
it("parses list_pages text responses when structuredContent is missing", async () => {
|
||||
const factory: ChromeMcpSessionFactory = async () => createFakeSession();
|
||||
setChromeMcpSessionFactoryForTest(factory);
|
||||
@@ -344,4 +349,64 @@ describe("chrome MCP page parsing", () => {
|
||||
expect(factoryCalls).toBe(2);
|
||||
expect(tabs).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("always passes a default timeout to navigate_page when none is specified", async () => {
|
||||
const session = createFakeSession();
|
||||
setChromeMcpSessionFactoryForTest(async () => session);
|
||||
|
||||
await navigateChromeMcpPage({
|
||||
profileName: "chrome-live",
|
||||
targetId: "1",
|
||||
url: "https://example.com",
|
||||
// intentionally no timeoutMs
|
||||
});
|
||||
|
||||
expect(session.client.callTool).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
name: "navigate_page",
|
||||
arguments: expect.objectContaining({ timeout: 20_000 }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("resets the Chrome MCP session when a navigate_page call hangs past the safety-net timeout", async () => {
|
||||
vi.useFakeTimers();
|
||||
let factoryCalls = 0;
|
||||
const factory: ChromeMcpSessionFactory = async () => {
|
||||
factoryCalls += 1;
|
||||
const session = createFakeSession();
|
||||
if (factoryCalls === 1) {
|
||||
// First session: all tool calls hang — simulates a Chrome MCP subprocess that is
|
||||
// completely blocked (e.g., stuck waiting for a slow navigation to complete).
|
||||
session.client.callTool = vi.fn(
|
||||
async () => new Promise<never>(() => {}),
|
||||
) as typeof session.client.callTool;
|
||||
}
|
||||
return session;
|
||||
};
|
||||
setChromeMcpSessionFactoryForTest(factory);
|
||||
|
||||
// Start navigation — will hang.
|
||||
const navPromise = navigateChromeMcpPage({
|
||||
profileName: "chrome-live",
|
||||
targetId: "1",
|
||||
url: "https://slow-site.example",
|
||||
});
|
||||
// Suppress unhandled-rejection detection: navPromise rejects during timer
|
||||
// advancement, before the expect below attaches its handler.
|
||||
void navPromise.catch(() => {});
|
||||
|
||||
// Advance past the 25 s safety-net (CHROME_MCP_NAVIGATE_TIMEOUT_MS 20 s + 5 s buffer).
|
||||
await vi.advanceTimersByTimeAsync(25_001);
|
||||
|
||||
await expect(navPromise).rejects.toThrow(/Chrome MCP "navigate_page".*timed out/);
|
||||
|
||||
// Switch back to real timers before testing reconnect behaviour.
|
||||
vi.useRealTimers();
|
||||
|
||||
// Next call must use a fresh session — factory is called a second time.
|
||||
const tabs = await listChromeMcpTabs("chrome-live");
|
||||
expect(factoryCalls).toBe(2);
|
||||
expect(tabs).toHaveLength(2);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -44,6 +44,7 @@ const DEFAULT_CHROME_MCP_ARGS = [
|
||||
];
|
||||
const CHROME_MCP_NEW_PAGE_TIMEOUT_MS = 5_000;
|
||||
const CHROME_MCP_NAVIGATE_TIMEOUT_MS = 20_000;
|
||||
const CHROME_MCP_NAVIGATE_CALL_SAFETY_TIMEOUT_MS = 25_000;
|
||||
|
||||
const sessions = new Map<string, ChromeMcpSession>();
|
||||
const pendingSessions = new Map<string, Promise<ChromeMcpSession>>();
|
||||
@@ -310,20 +311,55 @@ async function callTool(
|
||||
userDataDir: string | undefined,
|
||||
name: string,
|
||||
args: Record<string, unknown> = {},
|
||||
timeoutMs?: number,
|
||||
): Promise<ChromeMcpToolResult> {
|
||||
const cacheKey = buildChromeMcpSessionCacheKey(profileName, userDataDir);
|
||||
const session = await getSession(profileName, userDataDir);
|
||||
|
||||
const rawCall = session.client.callTool({
|
||||
name,
|
||||
arguments: args,
|
||||
}) as Promise<ChromeMcpToolResult>;
|
||||
|
||||
let timeoutHandle: ReturnType<typeof setTimeout> | undefined;
|
||||
const callPromise: Promise<ChromeMcpToolResult> =
|
||||
timeoutMs !== undefined && timeoutMs > 0
|
||||
? Promise.race([
|
||||
rawCall,
|
||||
new Promise<never>((_, reject) => {
|
||||
timeoutHandle = setTimeout(() => {
|
||||
// Use transport-identity check so we never delete a freshly-created replacement session.
|
||||
const cur = sessions.get(cacheKey);
|
||||
if (cur?.transport === session.transport) {
|
||||
sessions.delete(cacheKey);
|
||||
}
|
||||
void session.client.close().catch(() => {});
|
||||
reject(
|
||||
new Error(
|
||||
`Chrome MCP "${name}" timed out after ${timeoutMs}ms. Session reset for reconnect.`,
|
||||
),
|
||||
);
|
||||
}, timeoutMs);
|
||||
}),
|
||||
])
|
||||
: rawCall;
|
||||
|
||||
let result: ChromeMcpToolResult;
|
||||
try {
|
||||
result = (await session.client.callTool({
|
||||
name,
|
||||
arguments: args,
|
||||
})) as ChromeMcpToolResult;
|
||||
result = await callPromise;
|
||||
} catch (err) {
|
||||
// Transport/connection error — tear down session so it reconnects on next call
|
||||
sessions.delete(cacheKey);
|
||||
// Transport/connection error or safety-net timeout — tear down session so it reconnects.
|
||||
// Transport-identity check prevents clobbering a replacement session created concurrently.
|
||||
const cur = sessions.get(cacheKey);
|
||||
if (cur?.transport === session.transport) {
|
||||
sessions.delete(cacheKey);
|
||||
}
|
||||
await session.client.close().catch(() => {});
|
||||
throw err;
|
||||
} finally {
|
||||
if (timeoutHandle !== undefined) {
|
||||
clearTimeout(timeoutHandle);
|
||||
}
|
||||
}
|
||||
// Tool-level errors (element not found, script error, etc.) don't indicate a
|
||||
// broken connection — don't tear down the session for these.
|
||||
@@ -460,12 +496,19 @@ export async function navigateChromeMcpPage(params: {
|
||||
url: string;
|
||||
timeoutMs?: number;
|
||||
}): Promise<{ url: string }> {
|
||||
await callTool(params.profileName, params.userDataDir, "navigate_page", {
|
||||
pageId: parsePageId(params.targetId),
|
||||
type: "url",
|
||||
url: params.url,
|
||||
...(typeof params.timeoutMs === "number" ? { timeout: params.timeoutMs } : {}),
|
||||
});
|
||||
const resolvedTimeoutMs = params.timeoutMs ?? CHROME_MCP_NAVIGATE_TIMEOUT_MS;
|
||||
await callTool(
|
||||
params.profileName,
|
||||
params.userDataDir,
|
||||
"navigate_page",
|
||||
{
|
||||
pageId: parsePageId(params.targetId),
|
||||
type: "url",
|
||||
url: params.url,
|
||||
timeout: resolvedTimeoutMs,
|
||||
},
|
||||
CHROME_MCP_NAVIGATE_CALL_SAFETY_TIMEOUT_MS,
|
||||
);
|
||||
const page = await findPageById(
|
||||
params.profileName,
|
||||
parsePageId(params.targetId),
|
||||
|
||||
Reference in New Issue
Block a user