diff --git a/src/cdp/client.ts b/src/cdp/client.ts index 6fa1ab241..be42fb9af 100644 --- a/src/cdp/client.ts +++ b/src/cdp/client.ts @@ -116,6 +116,8 @@ export class CDPClient { private lastCookieScanResult: CookieScanResult | null = null; /** Coalesces concurrent connect() calls — only one connectInternal() runs at a time. */ private pendingConnect: Promise | null = null; + /** Invalidates stale async connection attempts that resolve after a reconnect. */ + private connectionGeneration = 0; /** Timestamp of last successful connection verification (heartbeat or active probe). */ private lastVerifiedAt = 0; @@ -653,11 +655,16 @@ export class CDPClient { * are both capped by the remaining budget so the total wall-clock time * stays within the caller's deadline. */ - private async connectInternal(options?: { autoLaunch?: boolean; budget?: Budget }): Promise { + private isCurrentConnectionGeneration(generation?: number): boolean { + return generation === undefined || generation === this.connectionGeneration; + } + + private async connectInternal(options?: { autoLaunch?: boolean; budget?: Budget; generation?: number }): Promise { this.disconnectRequested = false; const launcher = getChromeLauncher(this.port); const autoLaunch = options?.autoLaunch ?? this.autoLaunch; const budget = options?.budget; + const generation = options?.generation; const budgetDriven = !!budget && !isLegacyBudgetMode(); // Retry loop: after macOS sleep/wake, Chrome's WebSocket listener may be in a @@ -678,6 +685,9 @@ export class CDPClient { // Re-fetch instance on each attempt — Chrome may have regenerated its UUID const instance = await launcher.ensureChrome({ autoLaunch }); + if (!this.isCurrentConnectionGeneration(generation)) { + return false; + } const wsTimeoutMs = budgetDriven ? Math.max(1, Math.min(DEFAULT_PUPPETEER_CONNECT_TIMEOUT_MS, budget!.remaining())) @@ -685,7 +695,7 @@ export class CDPClient { try { let wsConnectTid: ReturnType; - this.browser = await Promise.race([ + const connectedBrowser = await Promise.race([ puppeteer.connect({ browserWSEndpoint: instance.wsEndpoint, defaultViewport: null, @@ -699,12 +709,27 @@ export class CDPClient { }), ]) as Browser; + if (!this.isCurrentConnectionGeneration(generation)) { + connectedBrowser.removeAllListeners('disconnected'); + connectedBrowser.removeAllListeners('targetdestroyed'); + connectedBrowser.removeAllListeners('targetchanged'); + connectedBrowser.removeAllListeners('targetcreated'); + connectedBrowser.disconnect().catch(() => {}); + return false; + } + + this.browser = connectedBrowser; + if (attempt > 1) { const remainingStr = budgetDriven ? `, budget remaining=${budget!.remaining()}ms` : `/${maxConnectRetries}`; console.error(`[CDPClient] connectInternal succeeded on attempt ${attempt}${remainingStr}`); } break; // Success — exit retry loop } catch (err) { + if (!this.isCurrentConnectionGeneration(generation)) { + return false; + } + // Clean up any partially-connected browser from this attempt to prevent // orphaned event listeners from firing handleDisconnect on an old browser. if (this.browser) { @@ -846,6 +871,7 @@ export class CDPClient { type: 'connected', timestamp: Date.now(), }); + return true; } /** @@ -900,22 +926,31 @@ export class CDPClient { } this.connectionState = 'connecting'; - this.pendingConnect = (async () => { + const generation = ++this.connectionGeneration; + const pendingConnect = (async () => { try { - await this.connectInternal({ budget }); + const connected = await this.connectInternal({ budget, generation }); + if (connected === false) { + return; + } this.lastVerifiedAt = Date.now(); this.startHeartbeat(); console.error('[CDPClient] Connected to Chrome'); } catch (err) { - this.connectionState = 'disconnected'; + if (this.isCurrentConnectionGeneration(generation)) { + this.connectionState = 'disconnected'; + } throw err; } })(); + this.pendingConnect = pendingConnect; try { - await this.pendingConnect; + await pendingConnect; } finally { - this.pendingConnect = null; + if (this.pendingConnect === pendingConnect) { + this.pendingConnect = null; + } } } @@ -930,6 +965,7 @@ export class CDPClient { */ async forceReconnect(options?: { budget?: Budget }): Promise { const budget = options?.budget; + const generation = ++this.connectionGeneration; // Invalidate any in-flight connect() — we're replacing the connection entirely this.pendingConnect = null; this.stopHeartbeat(); @@ -965,7 +1001,10 @@ export class CDPClient { try { // Do NOT auto-launch Chrome on heartbeat-triggered reconnect. // If Chrome was closed, stay disconnected until the next tool call. - await this.connectInternal({ autoLaunch: false, budget }); + const connected = await this.connectInternal({ autoLaunch: false, budget, generation }); + if (connected === false) { + return; + } this.lastVerifiedAt = Date.now(); this.consecutiveHeartbeatFailures = 0; this.startHeartbeat(); @@ -982,7 +1021,9 @@ export class CDPClient { console.error('[CDPClient] Cookie restore after reconnection failed (non-fatal):', err); } } catch (err) { - this.connectionState = 'disconnected'; + if (this.isCurrentConnectionGeneration(generation)) { + this.connectionState = 'disconnected'; + } this.emitConnectionEvent({ type: 'reconnect_failed', timestamp: Date.now(), diff --git a/tests/cli/admin-keys.test.ts b/tests/cli/admin-keys.test.ts index 13ece7b44..18f567be7 100644 --- a/tests/cli/admin-keys.test.ts +++ b/tests/cli/admin-keys.test.ts @@ -38,6 +38,27 @@ function extractToken(stdout: string): string { return m[0]; } +/** + * Extract a JSON array line from captured stdout. The in-process harness can + * intermittently see unrelated console.error noise from a prior leaked timer + * (e.g. "[WorkflowEngine] …") get rendered into the captured stdout buffer on + * macOS workers. The CLI's `list --json` only ever emits exactly one line that + * begins with '[' and parses as a JSON array, so scanning line-by-line for + * that shape is both sufficient and robust. + */ +function extractJsonArray(stdout: string): string { + for (const line of stdout.split(/\r?\n/).map((item) => item.trim()).filter(Boolean)) { + if (!line.startsWith('[')) continue; + try { + const parsed = JSON.parse(line); + if (Array.isArray(parsed)) return line; + } catch { + // Ignore unrelated bracket-prefixed test noise such as [WorkflowEngine]. + } + } + throw new Error(`No JSON array found in stdout: ${JSON.stringify(stdout)}`); +} + async function runCli(argv: string[]): Promise { const program = new Command(); program.exitOverride((err) => { @@ -191,7 +212,7 @@ describe('admin keys CLI', () => { const listed = await runCli(['admin', 'keys', 'list', '--json']); expect(listed.exitCode).toBeNull(); - const parsed = JSON.parse(listed.stdout) as Array<{ keyId: string; tenantId: string }>; + const parsed = JSON.parse(extractJsonArray(listed.stdout)) as Array<{ keyId: string; tenantId: string }>; expect(Array.isArray(parsed)).toBe(true); expect(parsed).toHaveLength(1); expect(parsed[0].tenantId).toBe('acme'); @@ -212,7 +233,7 @@ describe('admin keys CLI', () => { expect(revoked.stderr).toContain('Revoked'); const listed = await runCli(['admin', 'keys', 'list', '--json']); - const parsed = JSON.parse(listed.stdout) as Array<{ keyId: string; revokedAt?: number }>; + const parsed = JSON.parse(extractJsonArray(listed.stdout)) as Array<{ keyId: string; revokedAt?: number }>; const row = parsed.find((r) => r.keyId === keyId); expect(row).toBeDefined(); expect(typeof row!.revokedAt).toBe('number'); @@ -238,7 +259,7 @@ describe('admin keys CLI', () => { expect(rotated.stdout + rotated.stderr).not.toContain(firstPlaintext); const listed = await runCli(['admin', 'keys', 'list', '--json']); - const parsed = JSON.parse(listed.stdout) as Array<{ keyId: string; revokedAt?: number }>; + const parsed = JSON.parse(extractJsonArray(listed.stdout)) as Array<{ keyId: string; revokedAt?: number }>; const oldRow = parsed.find((r) => r.keyId === firstKeyId); expect(oldRow).toBeDefined(); expect(typeof oldRow!.revokedAt).toBe('number'); diff --git a/tests/src/cdp-connect-coalescing.test.ts b/tests/src/cdp-connect-coalescing.test.ts index 73d1dc1b4..f98e46f88 100644 --- a/tests/src/cdp-connect-coalescing.test.ts +++ b/tests/src/cdp-connect-coalescing.test.ts @@ -341,8 +341,44 @@ describe('CDPClient – forceReconnect invalidates pending connects', () => { resolveSlowConnect!(); await connectPromise; - // The browser should be the one from forceReconnect - expect((client as any).browser).toBe(newBrowser); - stopHeartbeat(client); - }); -}); + // The browser should be the one from forceReconnect + expect((client as any).browser).toBe(newBrowser); + stopHeartbeat(client); + }); + + test('stale connectInternal result cannot resurrect old browser after forceReconnect', async () => { + const client = new CDPClient({ port: 9222 }); + const staleBrowser = createMockBrowser('ws://stale'); + const newBrowser = createMockBrowser('ws://new'); + + let resolveStaleConnect: (() => void) | null = null; + let resolveNewConnect: (() => void) | null = null; + + mockPuppeteerConnect + .mockImplementationOnce(() => new Promise((resolve) => { + resolveStaleConnect = () => resolve(staleBrowser); + })) + .mockImplementationOnce(() => new Promise((resolve) => { + resolveNewConnect = () => resolve(newBrowser); + })); + + const connectPromise = client.connect(); + await Promise.resolve(); + expect(mockPuppeteerConnect).toHaveBeenCalledTimes(1); + + const reconnectPromise = client.forceReconnect(); + await Promise.resolve(); + expect(mockPuppeteerConnect).toHaveBeenCalledTimes(2); + + resolveNewConnect!(); + await reconnectPromise; + expect((client as any).browser).toBe(newBrowser); + + resolveStaleConnect!(); + await connectPromise; + + expect((client as any).browser).toBe(newBrowser); + expect(staleBrowser.disconnect).toHaveBeenCalled(); + stopHeartbeat(client); + }); +});