fix: keep page ids unique across browser reconnects (#2345)
Closes #2339 Reconnecting after a browser restart builds a fresh `McpContext`, which restarted the page id counter at 1 – ids from before the restart silently resolved to unrelated pages of the new browser (repro in #2339). Two changes: - The new context continues the id counter where the previous one left off (`startingPageId` option), so a stale id now fails with the existing "No page found" error and the agent re-lists - The first response after a reconnect carries a one-time note ("the browser was restarted or reconnected since the last call. Page ids have changed..."), like the #2308 fallback note. Since tool errors run through the same response formatting, the note shows up together with the very error the stale id produces Verified against the #2339 repro: the stale `select_page` now returns the note plus "No page found", and the next listing shows the new browser's pages under fresh ids. One thing I noticed but left alone: The replaced context isn't `dispose()`d on reconnect – pre-existing, and everything it holds is tied to the dead browser anyway. Happy to add that here if you'd like.
This commit is contained in:
+27
-2
@@ -55,8 +55,17 @@ interface McpContextOptions {
|
||||
// capability. When false (default), file-writing tools are restricted to the
|
||||
// OS temp directory. When true, the previous permissive behavior is restored.
|
||||
allowUnrestrictedPaths?: boolean;
|
||||
// Whether this context replaces a previous one after a browser reconnect.
|
||||
// Surfaces a one-time note in the next response.
|
||||
reconnected?: boolean;
|
||||
}
|
||||
|
||||
// Page ids are handed out from a process-wide counter so they stay unique
|
||||
// across all contexts, in particular across browser reconnects. An id issued
|
||||
// before a reconnect then fails to resolve instead of hitting an unrelated
|
||||
// page of the reconnected browser.
|
||||
let nextPageId = 1;
|
||||
|
||||
export class McpContext implements Context {
|
||||
browser: Browser;
|
||||
logger: Logger;
|
||||
@@ -78,7 +87,7 @@ export class McpContext implements Context {
|
||||
#screenRecorderData: {recorder: ScreenRecorder; filePath: string} | null =
|
||||
null;
|
||||
|
||||
#nextPageId = 1;
|
||||
#reconnectNotice = false;
|
||||
#extensionPages = new WeakMap<Target, Page>();
|
||||
|
||||
#extensionServiceWorkerMap = new WeakMap<Target, string>();
|
||||
@@ -109,6 +118,7 @@ export class McpContext implements Context {
|
||||
this.#locatorClass = locatorClass;
|
||||
this.#options = options;
|
||||
this.#allowUnrestrictedPaths = options.allowUnrestrictedPaths ?? false;
|
||||
this.#reconnectNotice = options.reconnected ?? false;
|
||||
|
||||
this.#serviceWorkerConsoleCollector = new ServiceWorkerConsoleCollector(
|
||||
this.browser,
|
||||
@@ -185,6 +195,10 @@ export class McpContext implements Context {
|
||||
return context;
|
||||
}
|
||||
|
||||
static resetPageIdsForTesting(): void {
|
||||
nextPageId = 1;
|
||||
}
|
||||
|
||||
roots(): Root[] {
|
||||
return [
|
||||
...(this.#roots ?? []),
|
||||
@@ -358,6 +372,17 @@ export class McpContext implements Context {
|
||||
return page;
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns true once if this context was created by reconnecting after the
|
||||
* previous browser connection was lost, so the next response can surface a
|
||||
* note. Cleared on first call.
|
||||
*/
|
||||
consumeReconnectNotice(): boolean {
|
||||
const notice = this.#reconnectNotice;
|
||||
this.#reconnectNotice = false;
|
||||
return notice;
|
||||
}
|
||||
|
||||
getPageById(pageId: number): McpPage {
|
||||
const page = this.#mcpPages.values().find(mcpPage => mcpPage.id === pageId);
|
||||
if (!page) {
|
||||
@@ -449,7 +474,7 @@ export class McpContext implements Context {
|
||||
async #createMcpPage(page: Page): Promise<McpPage> {
|
||||
let mcpPage = this.#mcpPages.get(page);
|
||||
if (!mcpPage) {
|
||||
mcpPage = new McpPage(page, this.#nextPageId++, {
|
||||
mcpPage = new McpPage(page, nextPageId++, {
|
||||
locatorClass: this.#locatorClass,
|
||||
hasNetworkBlockOrAllowlist: this.#hasNetworkBlockOrAllowlist,
|
||||
isolatedContextName: this.#getBrowserContextToNameMap().get(
|
||||
|
||||
+17
-1
@@ -42,7 +42,7 @@ import type {
|
||||
Extension,
|
||||
HTTPRequest,
|
||||
} from './third_party/index.js';
|
||||
import {handleDialog} from './tools/pages.js';
|
||||
import {handleDialog, listPages} from './tools/pages.js';
|
||||
import type {ToolGroups} from './tools/thirdPartyDeveloper.js';
|
||||
import type {
|
||||
DevToolsData,
|
||||
@@ -269,6 +269,7 @@ export class McpResponse implements Response {
|
||||
#redactNetworkHeaders = true;
|
||||
#error?: Error;
|
||||
#attachedWaitForResult?: WaitForEventsResult;
|
||||
#reconnectNotice = false;
|
||||
|
||||
get #deviceScope(): DevTools.CrUXManager.DeviceScope {
|
||||
return this.#page?.viewport?.isMobile ? 'PHONE' : 'DESKTOP';
|
||||
@@ -286,6 +287,14 @@ export class McpResponse implements Response {
|
||||
this.#redactNetworkHeaders = value;
|
||||
}
|
||||
|
||||
/**
|
||||
* Surfaces a one-time note that the browser reconnected and page ids changed.
|
||||
* Set by the tool handler when the context reports a pending reconnect notice.
|
||||
*/
|
||||
setReconnectNotice(): void {
|
||||
this.#reconnectNotice = true;
|
||||
}
|
||||
|
||||
attachDevToolsData(data: DevToolsData): void {
|
||||
this.#devToolsData = data;
|
||||
}
|
||||
@@ -859,6 +868,7 @@ export class McpResponse implements Response {
|
||||
thirdPartyDeveloperTools?: object[];
|
||||
webmcpTools?: object[];
|
||||
message?: string;
|
||||
reconnected?: boolean;
|
||||
networkConditions?: string;
|
||||
navigationTimeout?: number;
|
||||
viewport?: object;
|
||||
@@ -921,6 +931,12 @@ export class McpResponse implements Response {
|
||||
}
|
||||
|
||||
const response = [];
|
||||
if (this.#reconnectNotice) {
|
||||
structuredContent.reconnected = true;
|
||||
response.push(
|
||||
`Note: the browser was restarted or reconnected since the last call. Page ids have changed. Call ${listPages().name} to see open pages.`,
|
||||
);
|
||||
}
|
||||
if (this.#textResponseLines.length) {
|
||||
structuredContent.message = this.#textResponseLines.join('\n');
|
||||
response.push(...this.#textResponseLines);
|
||||
|
||||
@@ -219,6 +219,9 @@ export class ToolHandler {
|
||||
: new McpResponse(this.serverArgs);
|
||||
|
||||
response.setRedactNetworkHeaders(this.serverArgs.redactNetworkHeaders);
|
||||
if (context.consumeReconnectNotice()) {
|
||||
response.setReconnectNotice();
|
||||
}
|
||||
try {
|
||||
if (this.tool.verifyFilesSchema) {
|
||||
for (const key of this.tool.verifyFilesSchema) {
|
||||
|
||||
@@ -154,6 +154,8 @@ export async function createMcpServer(
|
||||
allowList: allowlist,
|
||||
blocklist: blocklist,
|
||||
allowUnrestrictedPaths: serverArgs.allowUnrestrictedPaths,
|
||||
// Surfaces a one-time note in the next response after a reconnect.
|
||||
reconnected: context !== undefined,
|
||||
});
|
||||
await updateRoots();
|
||||
}
|
||||
|
||||
@@ -11,14 +11,17 @@ import path from 'node:path';
|
||||
import {afterEach, describe, it} from 'node:test';
|
||||
import {pathToFileURL} from 'node:url';
|
||||
|
||||
import logger from 'debug';
|
||||
import {Locator} from 'puppeteer';
|
||||
import sinon from 'sinon';
|
||||
|
||||
import {NetworkFormatter} from '../src/formatters/NetworkFormatter.js';
|
||||
import {McpContext} from '../src/McpContext.js';
|
||||
import {TextSnapshot} from '../src/TextSnapshot.js';
|
||||
import type {HTTPResponse} from '../src/third_party/index.js';
|
||||
import type {TraceResult} from '../src/trace-processing/parse.js';
|
||||
|
||||
import {getMockRequest, html, withMcpContext} from './utils.js';
|
||||
import {getMockRequest, html, withBrowser, withMcpContext} from './utils.js';
|
||||
|
||||
describe('McpContext', () => {
|
||||
afterEach(() => {
|
||||
@@ -222,6 +225,66 @@ describe('McpContext', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('continues page ids across contexts so stale ids do not resolve', async () => {
|
||||
await withBrowser(async browser => {
|
||||
const options = {
|
||||
experimentalDevToolsDebugging: false,
|
||||
performanceCrux: false,
|
||||
};
|
||||
const first = await McpContext.from(
|
||||
browser,
|
||||
logger('test'),
|
||||
options,
|
||||
Locator,
|
||||
);
|
||||
const idBeforeReconnect = (await first.newPage()).id;
|
||||
first.dispose();
|
||||
|
||||
// A new context (as created after a browser reconnect) continues the
|
||||
// shared id counter, so an id handed out before no longer resolves and
|
||||
// the next page keeps counting up rather than colliding with it.
|
||||
const second = await McpContext.from(
|
||||
browser,
|
||||
logger('test'),
|
||||
options,
|
||||
Locator,
|
||||
);
|
||||
try {
|
||||
assert.throws(
|
||||
() => second.getPageById(idBeforeReconnect),
|
||||
/No page found/,
|
||||
);
|
||||
assert.ok(
|
||||
(await second.newPage()).id > idBeforeReconnect,
|
||||
'ids continue past the pre-reconnect ids',
|
||||
);
|
||||
} finally {
|
||||
second.dispose();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
it('reports the reconnect notice once', async () => {
|
||||
await withBrowser(async browser => {
|
||||
const context = await McpContext.from(
|
||||
browser,
|
||||
logger('test'),
|
||||
{
|
||||
experimentalDevToolsDebugging: false,
|
||||
performanceCrux: false,
|
||||
reconnected: true,
|
||||
},
|
||||
Locator,
|
||||
);
|
||||
try {
|
||||
assert.ok(context.consumeReconnectNotice(), 'notice available once');
|
||||
assert.ok(!context.consumeReconnectNotice(), 'notice does not repeat');
|
||||
} finally {
|
||||
context.dispose();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
it('should include network requests in structured content', async t => {
|
||||
await withMcpContext(async (response, context) => {
|
||||
const mockRequest = getMockRequest({
|
||||
|
||||
@@ -63,6 +63,32 @@ describe('McpResponse', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('includes a reconnect notice only when set', async () => {
|
||||
await withMcpContext(async (response, context) => {
|
||||
const before = await response.handle('test', context);
|
||||
assert.ok(
|
||||
!JSON.stringify(before.content).includes('Page ids have changed'),
|
||||
'no reconnect notice by default',
|
||||
);
|
||||
assert.ok(
|
||||
!(before.structuredContent as {reconnected?: boolean}).reconnected,
|
||||
'structuredContent is not flagged reconnected by default',
|
||||
);
|
||||
|
||||
response.setReconnectNotice();
|
||||
const after = await response.handle('test', context);
|
||||
assert.ok(
|
||||
JSON.stringify(after.content).includes('Page ids have changed'),
|
||||
'reconnect notice is included once set',
|
||||
);
|
||||
assert.strictEqual(
|
||||
(after.structuredContent as {reconnected?: boolean}).reconnected,
|
||||
true,
|
||||
'structuredContent is flagged reconnected once set',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
it('allows response text lines to be added', async t => {
|
||||
await withMcpContext(async (response, context) => {
|
||||
response.appendResponseLine('Testing 1');
|
||||
|
||||
@@ -127,6 +127,7 @@ export async function withMcpContext(
|
||||
) {
|
||||
await withBrowser(async browser => {
|
||||
TextSnapshot.resetCounter();
|
||||
McpContext.resetPageIdsForTesting();
|
||||
const response = new McpResponse(args as ParsedArguments);
|
||||
if (context) {
|
||||
context.dispose();
|
||||
|
||||
Reference in New Issue
Block a user