diff --git a/packages/core/src/browser.js b/packages/core/src/browser.js index fd526c4b4..fce541887 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; + // 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) { 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); + }); +});