From 96a9ba9e22105d5eab11ac58bb81af839a476960 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Wed, 26 Aug 2026 16:50:07 +0530 Subject: [PATCH 1/2] fix(core): bound CDP sends so a stalled browser can't hang the CLI Browser.send and Session.send resolved only on a matching CDP reply or a close, with no deadline of their own. Chrome does not always reply: a renderer or browser process that is frozen, starved of CPU, or swapped out keeps its target attached and simply goes silent. Callers cannot defend against this either, because their deadlines are in-page timers living in the very renderer that stopped running -- which is why @percy/storybook's PERCY_STORY_RENDER_TIMEOUT never fires in these runs. The result is a total silent stall: no error, no log, no retry, and a build left unfinalized until the server force-closes it hours later. Add a Node-side deadline that nothing inside the page can defeat, on both the session-scoped and browser-scoped connections. Target creation and attachment run on the browser connection before any page exists, so bounding sessions alone still strands that path. Default 120s, floored above the longest legitimate in-page wait and auto-raised with PERCY_PAGE_LOAD_TIMEOUT / PERCY_STORY_RENDER_TIMEOUT; PERCY_CDP_TIMEOUT overrides it, and 0 disables it. The raw-message path stays untimed -- a session owns that reply. On expiry it rejects, so existing retries handle it. Reproduced end to end on a 2-core container: an unfixed run goes silent after "Page created" until the 5-minute monitoring timeout, while the same run with the deadline reports "Protocol error (Runtime.enable): Timed out after 30000ms" and retries. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/browser.js | 26 +++- packages/core/src/session.js | 66 ++++++++- packages/core/test/unit/session.test.js | 173 ++++++++++++++++++++++++ 3 files changed, 260 insertions(+), 5 deletions(-) create mode 100644 packages/core/test/unit/session.test.js diff --git a/packages/core/src/browser.js b/packages/core/src/browser.js index fd526c4b4..2d2083486 100644 --- a/packages/core/src/browser.js +++ b/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; + for (let callback of this.#callbacks.values()) clearTimeout(callback.timer); 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) { diff --git a/packages/core/src/session.js b/packages/core/src/session.js index bd19e720b..6c11d7729 100644 --- a/packages/core/src/session.js +++ b/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}` })); diff --git a/packages/core/test/unit/session.test.js b/packages/core/test/unit/session.test.js new file mode 100644 index 000000000..535d43523 --- /dev/null +++ b/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); + }); +}); From b5c16e1f8e70184de04137b9dc276058e574b2c7 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Wed, 26 Aug 2026 20:46:35 +0530 Subject: [PATCH 2/2] fix(core): drop an unreachable clearTimeout from Browser.restart restart() force-closes the browser first, and close() already clears every pending callback's deadline before emptying the map -- so the loop added with the browser-scoped deadline could never execute. It shared a line with the statement after it, which is why coverage read 99.24% statements against 100% lines and tripped the 100% global threshold. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/browser.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/browser.js b/packages/core/src/browser.js index 2d2083486..fce541887 100644 --- a/packages/core/src/browser.js +++ b/packages/core/src/browser.js @@ -152,7 +152,7 @@ export class Browser extends EventEmitter { // Reset state for fresh launch this.readyState = null; this._closed = null; - for (let callback of this.#callbacks.values()) clearTimeout(callback.timer); + // close() above already cleared every pending callback and its deadline this.#callbacks.clear(); this.sessions.clear();