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:
Thomas Bachem
2026-07-13 10:58:13 +02:00
committed by GitHub
parent 4094e8eb27
commit 3e8d92245c
7 changed files with 140 additions and 4 deletions
+27 -2
View File
@@ -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
View File
@@ -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);
+3
View File
@@ -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) {
+2
View File
@@ -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();
}
+64 -1
View File
@@ -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({
+26
View File
@@ -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');
+1
View File
@@ -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();