fix(cdp): guard stale reconnect completions

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<void> | 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<void> {
+  private isCurrentConnectionGeneration(generation?: number): boolean {
+    return generation === undefined || generation === this.connectionGeneration;
+  }
+
+  private async connectInternal(options?: { autoLaunch?: boolean; budget?: Budget; generation?: number }): Promise<boolean> {
     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,14 +685,17 @@ 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()))
         : DEFAULT_PUPPETEER_CONNECT_TIMEOUT_MS;
 
       try {
         let wsConnectTid: ReturnType<typeof setTimeout>;
-        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<void> {
     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(),

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<RunResult> {
   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');

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);
+  });
+});