From 490179c947a692a0525770644fd8f7144d918eb8 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Tue, 21 Jul 2026 09:35:04 +0000 Subject: [PATCH] fix(dashboard): don't evict live registration on a transient probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mcp/annotate.spec.ts suite flakes on macOS bots: `connectToDashboard` polls `cli list` until the dashboard app registers, but occasionally the registration vanishes for good and the poll never recovers ("dashboard app ... is not registered yet" / undefined `endpoint`). Root cause: `serverRegistry.list()` probes each descriptor with a single `net.connect` and unlinks the descriptor file the moment one probe fails. Under CI contention a live endpoint can transiently refuse/reset a connection (listen backlog, daemon mid-cold-start), so a healthy dashboard registration gets permanently deleted from disk — every later `list` then reports it missing. This is deeper than the poll cadence #41894 addressed. Fix the probe to retry a few times with a short per-attempt timeout before declaring the endpoint dead. A truly gone endpoint keeps failing fast and is still evicted; a busy one recovers and stays registered. DB evidence (from #41894, rolling window, macOS bots, expected=passed): every test in mcp/annotate.spec.ts flakes on mcp-macos-* bots. Top offenders on mcp-macos-latest-firefox (317 runs): "downloads zip with feedback.md" 21 fails (6.6%), "switch screencast to -s session" 20 (6.3%), "disengage annotate mode when client disconnects" 19 (6.0%), "abort annotation when last screenshot is removed" 18 (5.7%) — all the same endpoint TypeError on test timeout. #41894's macOS Chromium/Chrome jobs still failed with the same symptom. Verified locally on macOS 15 (Arm64): - New deterministic regression test tests/library/unit/server-registry.spec.ts: a registration whose first probe fails but whose endpoint comes up mid-retry survives; one that never becomes reachable is still evicted. Passes on chromium/firefox/webkit-library. - npm run test-mcp -- --project=chromium tests/mcp/annotate.spec.ts:110 --repeat-each=3 --workers=2 — 3/3 passed. - npm run flint — clean. Ref: https://github.com/microsoft/playwright/pull/41894 CI run: https://github.com/microsoft/playwright/actions/runs/29817954196 Suggested-reviewer: Skn0tt --- .../playwright-core/src/serverRegistry.ts | 49 +++++++--- tests/library/unit/server-registry.spec.ts | 93 +++++++++++++++++++ 2 files changed, 127 insertions(+), 15 deletions(-) create mode 100644 tests/library/unit/server-registry.spec.ts diff --git a/packages/playwright-core/src/serverRegistry.ts b/packages/playwright-core/src/serverRegistry.ts index 3f5ddb0a74a67..3c1b1ca6b2d72 100644 --- a/packages/playwright-core/src/serverRegistry.ts +++ b/packages/playwright-core/src/serverRegistry.ts @@ -220,25 +220,44 @@ class ServerRegistry extends EventEmitter { } } +// A single connection probe is not authoritative: under load a live endpoint can +// transiently refuse or reset a connection (listen backlog exhaustion, the daemon +// mid-cold-start, etc). Since `list()` unlinks descriptors that fail to connect, +// one such blip would permanently evict a healthy registration. Retry a few times +// with a short per-attempt timeout before declaring the endpoint dead — a truly +// gone endpoint keeps failing fast, while a busy one recovers. +const kConnectProbeAttempts = 5; +const kConnectProbeTimeout = 1000; +const kConnectProbeInterval = 100; + async function canConnectTo(descriptor: BrowserDescriptor): Promise { - if (!descriptor.endpoint) + const endpoint = descriptor.endpoint ?? (descriptor as any).pipeName; + if (!endpoint) return false; - if (descriptor.endpoint.startsWith('ws://') || descriptor.endpoint.startsWith('wss://')) { - return await new Promise(resolve => { - const url = new URL(descriptor.endpoint!); - const socket = net.createConnection(Number(url.port), url.hostname, () => { - socket.destroy(); - resolve(true); - }); - socket.on('error', () => resolve(false)); - }); + for (let attempt = 0; attempt < kConnectProbeAttempts; ++attempt) { + if (await attemptConnectTo(endpoint)) + return true; + if (attempt < kConnectProbeAttempts - 1) + await new Promise(resolve => setTimeout(resolve, kConnectProbeInterval)); } - return await new Promise(resolve => { - const socket = net.createConnection(descriptor.endpoint ?? (descriptor as any).pipeName, () => { + return false; +} + +function attemptConnectTo(endpoint: string): Promise { + return new Promise(resolve => { + let socket: net.Socket; + const done = (result: boolean) => { socket.destroy(); - resolve(true); - }); - socket.on('error', () => resolve(false)); + resolve(result); + }; + if (endpoint.startsWith('ws://') || endpoint.startsWith('wss://')) { + const url = new URL(endpoint); + socket = net.createConnection(Number(url.port), url.hostname, () => done(true)); + } else { + socket = net.createConnection(endpoint, () => done(true)); + } + socket.setTimeout(kConnectProbeTimeout, () => done(false)); + socket.on('error', () => done(false)); }); } diff --git a/tests/library/unit/server-registry.spec.ts b/tests/library/unit/server-registry.spec.ts new file mode 100644 index 0000000000000..67de37877cca7 --- /dev/null +++ b/tests/library/unit/server-registry.spec.ts @@ -0,0 +1,93 @@ +/** + * Copyright (c) Microsoft Corporation. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import fs from 'fs'; +import net from 'net'; +import os from 'os'; +import path from 'path'; + +import { test as it, expect } from '@playwright/test'; +import { serverRegistry } from '../../../packages/playwright-core/lib/serverRegistry'; + +async function freePort(): Promise { + const server = net.createServer(); + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + const port = (server.address() as net.AddressInfo).port; + await new Promise(resolve => server.close(() => resolve())); + return port; +} + +function writeDescriptor(dir: string, guid: string, endpoint: string) { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, guid), JSON.stringify({ + playwrightVersion: '1.0.0', + playwrightLib: '', + title: guid, + browser: { guid, browserName: 'chromium', launchOptions: {} }, + endpoint, + })); +} + +it.describe('serverRegistry health check', () => { + let registryDir: string; + let servers: net.Server[]; + + it.beforeEach(() => { + registryDir = fs.mkdtempSync(path.join(os.tmpdir(), 'pw-registry-')); + process.env.PWTEST_SERVER_REGISTRY = registryDir; + servers = []; + }); + + it.afterEach(async () => { + delete process.env.PWTEST_SERVER_REGISTRY; + for (const server of servers) + await new Promise(resolve => server.close(() => resolve())); + fs.rmSync(registryDir, { recursive: true, force: true }); + }); + + it('should keep a registration whose first connection probe fails but recovers', async () => { + const guid = 'guid-recovers'; + const port = await freePort(); + // Endpoint is initially unreachable (port refused), so the first probe(s) fail. + writeDescriptor(registryDir, guid, `ws://127.0.0.1:${port}`); + + // Bring the endpoint up shortly after, mid-way through the probe retries. + const listening = new Promise(resolve => { + setTimeout(() => { + const server = net.createServer(socket => socket.destroy()); + servers.push(server); + server.listen(port, '127.0.0.1', resolve); + }, 300); + }); + + const [result] = await Promise.all([serverRegistry.list(), listening]); + const descriptors = [...result.values()].flat(); + expect(descriptors.map(d => d.browser.guid)).toContain(guid); + // A transient probe failure must not evict a live registration from disk. + expect(fs.existsSync(path.join(registryDir, guid))).toBe(true); + }); + + it('should evict a registration whose endpoint never becomes reachable', async () => { + const guid = 'guid-dead'; + const port = await freePort(); + writeDescriptor(registryDir, guid, `ws://127.0.0.1:${port}`); + + const result = await serverRegistry.list(); + const descriptors = [...result.values()].flat(); + expect(descriptors.map(d => d.browser.guid)).not.toContain(guid); + expect(fs.existsSync(path.join(registryDir, guid))).toBe(false); + }); +});