diff --git a/playwriter/src/cdp-relay.ts b/playwriter/src/cdp-relay.ts index f68eed8..21f7b2e 100644 --- a/playwriter/src/cdp-relay.ts +++ b/playwriter/src/cdp-relay.ts @@ -210,14 +210,31 @@ export async function startPlayWriterCDPRelayServer({ const messageStr = JSON.stringify(messageToSend) + // Helper to safely send to a WebSocket, catching errors from closing connections. + // When a Playwright client closes its WebSocket, there's a race window where: + // 1. Playwright's _onClose runs (clears callbacks map) + // 2. We might still have messages in flight or try to send + // This can cause "Assertion error" in Playwright's crConnection.js if a response + // arrives after callbacks were cleared. We wrap in try-catch to handle this gracefully. + const safeSend = (client: PlaywrightClient) => { + try { + client.ws.send(messageStr) + } catch (e) { + // WebSocket might be closing/closed - this is expected during disconnect + logger?.log(pc.gray(`[Relay] Skipped sending to closing client ${client.id}: ${(e as Error).message}`)) + } + } + if (clientId) { const client = playwrightClients.get(clientId) if (client) { - client.ws.send(messageStr) + safeSend(client) } } else { - for (const client of playwrightClients.values()) { - client.ws.send(messageStr) + // Copy the clients array to avoid issues if a client disconnects during iteration + const clients = Array.from(playwrightClients.values()) + for (const client of clients) { + safeSend(client) } } } diff --git a/playwriter/src/relay-session.test.ts b/playwriter/src/relay-session.test.ts index ec38ff2..273dd98 100644 --- a/playwriter/src/relay-session.test.ts +++ b/playwriter/src/relay-session.test.ts @@ -5,7 +5,7 @@ import { getCdpUrl } from './utils.js' import { getCDPSessionForPage } from './cdp-session.js' import { Debugger } from './debugger.js' import { Editor } from './editor.js' -import { setupTestContext, cleanupTestContext, getExtensionServiceWorker, createSseServer, type TestContext, withTimeout, js } from './test-utils.js' +import { setupTestContext, cleanupTestContext, getExtensionServiceWorker, createSseServer, safeCloseCDPBrowser, type TestContext, withTimeout, js } from './test-utils.js' import './test-declarations.js' const TEST_PORT = 19993 @@ -992,9 +992,7 @@ describe('Service Worker Target Tests', () => { const title = await targetPage!.title() expect(title).toBeTruthy() - // Small delay to let pending CDP operations flush before closing - await new Promise(r => setTimeout(r, 100)) - await browser.close() + await safeCloseCDPBrowser(browser) await page.close() }, 60000) diff --git a/playwriter/src/test-utils.ts b/playwriter/src/test-utils.ts index 2bee8ba..d31be60 100644 --- a/playwriter/src/test-utils.ts +++ b/playwriter/src/test-utils.ts @@ -303,6 +303,29 @@ export function tryJsonParse(str: string) { } } +/** + * Safely close a browser connected via connectOverCDP. + * + * Playwright's CRConnection uses async message handling (messageWrap) that can cause + * a race condition where _onClose() runs before all pending _onMessage() handlers complete. + * This results in "Assertion error" from crConnection.js when a CDP response arrives + * after callbacks were cleared by dispose(). + * + * This helper waits for the message queue to drain before closing, avoiding the race. + * + * @param browser - Browser instance from chromium.connectOverCDP() + * @param drainDelayMs - Time to wait for pending messages to be processed (default: 50ms) + */ +export async function safeCloseCDPBrowser( + browser: Awaited>, + drainDelayMs = 50 +): Promise { + // Wait for any queued message handlers to run + // This gives Playwright's messageWrap time to process pending CDP responses + await new Promise(r => setTimeout(r, drainDelayMs)) + await browser.close() +} + export type SimpleServer = { baseUrl: string close: () => Promise diff --git a/playwriter/vitest.setup.ts b/playwriter/vitest.setup.ts index ae83ef6..c86e76a 100644 --- a/playwriter/vitest.setup.ts +++ b/playwriter/vitest.setup.ts @@ -1,25 +1,43 @@ /** - * Vitest setup file - handles unhandled rejections from Playwright CDP cleanup. + * Vitest setup - handles Playwright CDP disconnect race condition. * - * When tests use connectOverCDP() to connect to Chrome, Playwright may have - * pending CDP messages when browser.close() is called. These messages get - * rejected when the connection closes, causing "Assertion error" from - * Playwright's internal crConnection.js. This is expected cleanup behavior, - * not a real error. + * ROOT CAUSE (from Playwright source crConnection.ts:164): + * + * _onMessage(object: ProtocolResponse) { + * if (object.id && this._callbacks.has(object.id)) { + * // Handle response with matching callback + * } else if (object.id && object.error?.code === -32001) { + * // Closed session error - ignore + * } else { + * assert(!object.id); // ← FAILS: expects event, got orphaned response + * } + * } + * + * WHY IT HAPPENS: + * 1. Relay sends CDP response to Playwright + * 2. Playwright's messageWrap() schedules _onMessage for next task + * 3. browser.close() is called + * 4. _onClose() fires IMMEDIATELY and clears callbacks via dispose() + * 5. Scheduled _onMessage finally runs + * 6. Looks for callback → NOT FOUND → assertion fails + * + * This is a race condition in Playwright's async message handling that we cannot + * fix without patching Playwright. The assertion error during disconnect is benign + * and expected - it just means a CDP response arrived after we stopped caring. + * + * See: https://github.com/microsoft/playwright/blob/main/packages/playwright-core/src/server/chromium/crConnection.ts */ process.on('unhandledRejection', (reason: any) => { - // Suppress Playwright's internal assertion errors during CDP cleanup - // These happen when browser.close() is called with pending CDP messages - if (reason?.message === 'Assertion error' || reason?.name === 'AssertionError') { - // Check if it's from Playwright's internal code + // Check if this is Playwright's CDP disconnect assertion error + if (reason?.message === 'Assertion error') { const stack = reason?.stack || '' if (stack.includes('crConnection.js') || stack.includes('crSession')) { - // Silently ignore - this is expected cleanup behavior + // Benign race condition during disconnect - suppress return } } - // Re-throw other unhandled rejections - console.error('Unhandled rejection:', reason) + // Re-throw other unhandled rejections to fail the test + throw reason })