fix(core): bound CDP sends so a stalled browser can't hang the CLI

packages/core/src/browser.js

@@ -9,7 +9,7 @@ import rimraf from 'rimraf';
 import logger from '@percy/logger';
 import install from './install.js';
 import { MAX_CDP_PAYLOAD } from './network.js';
-import Session from './session.js';
+import Session, { cdpTimeout, cdpTimeoutMessage } from './session.js';
 import Page from './page.js';
 
 // Chrome features Percy disables for v143 new-headless asset discovery.
@@ -152,6 +152,7 @@ export class Browser extends EventEmitter {
     // Reset state for fresh launch
     this.readyState = null;
     this._closed = null;
+    // close() above already cleared every pending callback and its deadline
     this.#callbacks.clear();
     this.sessions.clear();
 
@@ -204,6 +205,7 @@ export class Browser extends EventEmitter {
 
     // reject any pending callbacks
     for (let callback of this.#callbacks.values()) {
+      clearTimeout(callback.timer);
       callback.reject(Object.assign(callback.error, {
         message: `Protocol error (${callback.method}): Browser closed.`
       }));
@@ -294,9 +296,26 @@ export class Browser extends EventEmitter {
       // send the message payload
       this.ws.send(JSON.stringify({ id, method, params }));
 
-      // will resolve or reject when a matching response is received
+      // will resolve or reject when a matching response is received, or when
+      // the deadline below expires because no response is ever coming. The
+      // browser process can go silent the same way a renderer can - target
+      // creation and attachment strand here otherwise, before any page exists.
       return new Promise((resolve, reject) => {
-        this.#callbacks.set(id, { error: new Error(), resolve, reject, method });
+        let callback = { error: new Error(), resolve, reject, method };
+        let timeout = cdpTimeout();
+
+        if (timeout > 0) {
+          callback.timer = setTimeout(() => {
+            this.#callbacks.delete(id);
+            this.log.debug(`Protocol timeout (${method}) after ${timeout}ms`);
+
+            reject(Object.assign(callback.error, {
+              message: cdpTimeoutMessage(method, timeout)
+            }));
+          }, timeout);
+        }
+
+        this.#callbacks.set(id, callback);
       });
     }
   }
@@ -368,6 +387,7 @@ export class Browser extends EventEmitter {
       // resolve or reject a pending promise created with #send()
       let callback = this.#callbacks.get(data.id);
       this.#callbacks.delete(data.id);
+      clearTimeout(callback.timer);
 
       /* istanbul ignore next: races with page._handleMessage() */
       if (data.error) {

packages/core/src/session.js

@@ -1,7 +1,52 @@
 import EventEmitter from 'events';
 import logger from '@percy/logger';
 
+// Node-side ceiling on a single CDP round trip. Chrome does not always answer:
+// a renderer that is frozen, starved of CPU, or swapped out keeps its target
+// attached and simply never replies. In-page deadlines cannot rescue that case
+// — their timers live in the very renderer that stopped running — so with no
+// deadline here `send()` never settles and the whole CLI blocks forever with
+// no error, no log and an unfinalized build. Reproducible by freezing the
+// renderer (`Page.setWebLifecycleState`) or descheduling it (SIGSTOP), both of
+// which a small, memory-pressured CI agent can do on its own.
+export const DEFAULT_CDP_TIMEOUT = 120000;
+
+// The longest wait a caller could legitimately be running *inside* the page,
+// doubled. Keeps the deadline above those so raising one of them can never
+// turn into a spurious protocol timeout.
+function inPageCeiling() {
+  return Math.max(
+    parseInt(process.env.PERCY_PAGE_LOAD_TIMEOUT, 10) || 0,
+    parseInt(process.env.PERCY_STORY_RENDER_TIMEOUT, 10) || 0
+  ) * 2;
+}
+
+// Resolved once per process, and memoized so a mid-run env change cannot make
+// concurrent sessions disagree. `PERCY_CDP_TIMEOUT=0` disables the deadline.
+export function cdpTimeout() {
+  if (Session.TIMEOUT !== undefined) return Session.TIMEOUT;
+  let configured = parseInt(process.env.PERCY_CDP_TIMEOUT, 10);
+
+  Session.TIMEOUT = Number.isNaN(configured)
+    ? Math.max(DEFAULT_CDP_TIMEOUT, inPageCeiling())
+    : Math.max(0, configured);
+
+  return Session.TIMEOUT;
+}
+
+// Shared by both CDP deadlines - the session-scoped one below and the
+// browser-scoped one in browser.js.
+export function cdpTimeoutMessage(method, timeout) {
+  return `Protocol error (${method}): Timed out after ${timeout}ms ` +
+    'waiting for a response from the browser. The browser stopped responding — a ' +
+    'renderer or browser process that is frozen, starved of CPU, or swapped out ' +
+    'never replies and never runs its own timers. Raise PERCY_CDP_TIMEOUT if this ' +
+    'command is legitimately slower than that, or set it to 0 to disable the deadline.';
+}
+
 export class Session extends EventEmitter {
+  static TIMEOUT = undefined;
+
   #callbacks = new Map();
 
   log = logger('core:session');
@@ -45,9 +90,24 @@ export class Session extends EventEmitter {
     // send a raw message to the browser so we can provide a sessionId
     let id = await this.browser.send({ sessionId: this.sessionId, method, params });
 
-    // will resolve or reject when a matching response is received
+    // will resolve or reject when a matching response is received, or when the
+    // deadline below expires because no response is ever coming
     return new Promise((resolve, reject) => {
-      this.#callbacks.set(id, { error: new Error(), resolve, reject, method });
+      let callback = { error: new Error(), resolve, reject, method };
+      let timeout = cdpTimeout();
+
+      if (timeout > 0) {
+        callback.timer = setTimeout(() => {
+          this.#callbacks.delete(id);
+          this.log.debug(`Protocol timeout (${method}) after ${timeout}ms`);
+
+          reject(Object.assign(callback.error, {
+            message: cdpTimeoutMessage(method, timeout)
+          }));
+        }, timeout);
+      }
+
+      this.#callbacks.set(id, callback);
     });
   }
 
@@ -56,6 +116,7 @@ export class Session extends EventEmitter {
       // resolve or reject a pending promise created with #send()
       let callback = this.#callbacks.get(data.id);
       this.#callbacks.delete(data.id);
+      clearTimeout(callback.timer);
 
       /* istanbul ignore next: races with browser._handleMessage() */
       if (data.error) {
@@ -77,6 +138,7 @@ export class Session extends EventEmitter {
 
     // reject any pending callbacks
     for (let callback of this.#callbacks.values()) {
+      clearTimeout(callback.timer);
       callback.reject(Object.assign(callback.error, {
         message: `Protocol error (${callback.method}): ${this.closedReason}`
       }));

packages/core/test/unit/session.test.js

@@ -0,0 +1,173 @@
+import { setupTest } from '../helpers/index.js';
+import { Session } from '../../src/session.js';
+import { Browser } from '../../src/browser.js';
+
+// A browser stub whose send() only hands back a message id and never delivers
+// a reply, which is what a frozen or starved renderer looks like from Node.
+function makeBrowser() {
+  let lastId = 0;
+  return {
+    sessions: new Map(),
+    percy: { config: { discovery: {} } },
+    sent: [],
+    async send(message) {
+      this.sent.push(message);
+      return ++lastId;
+    }
+  };
+}
+
+// browser.send() is awaited before the callback is registered, so give that
+// microtask a chance to land before simulating a reply
+const tick = () => new Promise(r => setTimeout(r, 10));
+
+function makeSession(browser) {
+  return new Session(browser, {
+    params: {
+      sessionId: 'session-1',
+      targetInfo: { targetId: 'target-1', type: 'page' }
+    }
+  });
+}
+
+describe('Unit / Session', () => {
+  let browser, session;
+
+  beforeEach(async () => {
+    await setupTest();
+    Session.TIMEOUT = undefined;
+    delete process.env.PERCY_CDP_TIMEOUT;
+    browser = makeBrowser();
+    session = makeSession(browser);
+  });
+
+  afterEach(() => {
+    Session.TIMEOUT = undefined;
+    delete process.env.PERCY_CDP_TIMEOUT;
+  });
+
+  // A renderer that stops running (frozen, starved, swapped out)
+  // keeps its target attached and simply never replies. In-page deadlines
+  // cannot save us — their timers live in that same stalled renderer — so
+  // without a Node-side deadline the whole CLI blocks forever, silently.
+  it('rejects a pending command when the browser never replies', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '150';
+
+    await expectAsync(session.send('Runtime.callFunctionOn', {}))
+      .toBeRejectedWithError(/Protocol error \(Runtime\.callFunctionOn\): Timed out after 150ms/);
+  });
+
+  it('names the command and points at the override in the error', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '100';
+
+    await expectAsync(session.send('Page.navigate', {}))
+      .toBeRejectedWithError(/Page\.navigate[\s\S]*PERCY_CDP_TIMEOUT/);
+  });
+
+  it('does not reject when a reply arrives before the deadline', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '2000';
+
+    let pending = session.send('Runtime.evaluate', {});
+    await tick();
+    session._handleMessage({ id: 1, result: { value: 'ok' } });
+
+    await expectAsync(pending).toBeResolvedTo({ value: 'ok' });
+  });
+
+  it('clears the deadline once a reply arrives, so it cannot fire later', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '100';
+
+    let pending = session.send('Runtime.evaluate', {});
+    await tick();
+    session._handleMessage({ id: 1, result: { value: 'ok' } });
+    await expectAsync(pending).toBeResolvedTo({ value: 'ok' });
+
+    // outlive the deadline - nothing should blow up or reject afterwards
+    await new Promise(r => setTimeout(r, 250));
+  });
+
+  it('clears the deadline when the session closes first', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '5000';
+
+    let pending = session.send('Runtime.evaluate', {});
+    await tick();
+    session._handleClose();
+
+    await expectAsync(pending).toBeRejectedWithError(/Session closed\./);
+  });
+
+  it('can be disabled with PERCY_CDP_TIMEOUT=0', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '0';
+
+    let settled = false;
+    session.send('Runtime.evaluate', {}).then(() => { settled = true; }, () => { settled = true; });
+
+    await new Promise(r => setTimeout(r, 200));
+    expect(settled).toBe(false);
+  });
+});
+
+// The browser process can go silent the same way a renderer can. Target
+// creation and attachment happen on the browser-scoped connection, before any
+// page exists, so a deadline on Session alone leaves that path unbounded.
+describe('Unit / Browser CDP deadline', () => {
+  let browser;
+
+  beforeEach(async () => {
+    await setupTest();
+    Session.TIMEOUT = undefined;
+    delete process.env.PERCY_CDP_TIMEOUT;
+    browser = new Browser({ config: { discovery: {} } });
+    browser.ws = { send: () => {} };
+    browser.isConnected = () => true;
+  });
+
+  afterEach(() => {
+    Session.TIMEOUT = undefined;
+    delete process.env.PERCY_CDP_TIMEOUT;
+  });
+
+  it('rejects a pending browser command when no reply arrives', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '150';
+
+    await expectAsync(browser.send('Target.createBrowserContext', {}))
+      .toBeRejectedWithError(/Protocol error \(Target\.createBrowserContext\): Timed out after 150ms/);
+  });
+
+  it('does not reject when a reply arrives before the deadline', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '2000';
+
+    let pending = browser.send('Target.createTarget', {});
+    browser._handleMessage(JSON.stringify({ id: 1, result: { targetId: 't1' } }));
+
+    await expectAsync(pending).toBeResolvedTo({ targetId: 't1' });
+  });
+
+  it('clears the deadline once a reply arrives, so it cannot fire later', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '100';
+
+    let pending = browser.send('Target.attachToTarget', {});
+    browser._handleMessage(JSON.stringify({ id: 1, result: { sessionId: 's1' } }));
+    await expectAsync(pending).toBeResolvedTo({ sessionId: 's1' });
+
+    await new Promise(r => setTimeout(r, 250));
+  });
+
+  it('leaves the raw-message path untimed, since a session owns that reply', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '100';
+
+    // passing a raw message returns the id synchronously - no promise to bound
+    await expectAsync(browser.send({ sessionId: 's1', method: 'Runtime.evaluate' }))
+      .toBeResolvedTo(1);
+  });
+
+  it('can be disabled with PERCY_CDP_TIMEOUT=0', async () => {
+    process.env.PERCY_CDP_TIMEOUT = '0';
+
+    let settled = false;
+    browser.send('Target.createBrowserContext', {}).then(() => { settled = true; }, () => { settled = true; });
+
+    await new Promise(r => setTimeout(r, 200));
+    expect(settled).toBe(false);
+  });
+});