From 41ca4c0bfc2e3cfeb18cb590149e28fab0aeb42f Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Fri, 11 Sep 2026 00:28:34 +0530 Subject: [PATCH 1/3] fix(sdk-utils): bound the readiness eval on the Node side (PER-10756) PercyDOM.waitForReady() enforces its own timeout with an in-page setTimeout, and every individual check settles on a setTimeout or setInterval. On a page whose timers are faked and paused -- Playwright's page.clock.pauseAt(), sinon useFakeTimers, jest fake timers -- none of those can ever fire, so the promise page.evaluate() is awaiting stays pending for the life of the page. The result is that percySnapshot() hangs the caller's test until the test runner's own timeout kills it, and every interaction after the snapshot then fails against a torn-down page: [percy:playwright] waitForReady failed, proceeding to serialize: page.evaluate: Test timeout of 30000ms exceeded. [percy:playwright] Could not take DOM snapshot "test" [percy:playwright] page.evaluate: Target page, context or browser has been closed Reproduced with @percy/dom 1.32.9 under page.clock.pauseAt(): the gate is still pending 14s in, well past the balanced preset's 10s in-page timeout, which cannot fire either. Node is the only side of the eval boundary guaranteed to have a real clock, so the backstop belongs here rather than in any one SDK. The gate now races the eval against a deadline of the effective in-page timeout plus a 3s grace, and on expiry logs at debug and returns null -- the same graceful degradation the gate already had for a rejected eval, so serialize still runs and the snapshot is captured ungated. An abandoned eval's later rejection (the driver closing the page at end of test) is swallowed so it never surfaces as an unhandled rejection. Fixing it in runReadinessGate covers every SDK that routes through it (Playwright, Puppeteer, Selenium-js, WebdriverIO, Nightwatch, Ember) in one place. PRESET_TIMEOUT_MS mirrors PRESETS in @percy/dom's readiness.js, which only exists in the browser bundle. A parity test reads the dom source and asserts the timeouts still match, following the same approach as the existing serialize-frames constant mirror. Until this ships, the workaround is to turn the gate off for suites that fake the clock: # .percy.yml snapshot: readiness: preset: disabled Co-Authored-By: Claude Opus 5 (1M context) --- packages/sdk-utils/src/index.js | 2 + packages/sdk-utils/src/serialize-dom.js | 57 ++++++++++++- packages/sdk-utils/test/index.test.js | 102 ++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 1 deletion(-) diff --git a/packages/sdk-utils/src/index.js b/packages/sdk-utils/src/index.js index a4d5aec40..639b0b357 100644 --- a/packages/sdk-utils/src/index.js +++ b/packages/sdk-utils/src/index.js @@ -15,6 +15,7 @@ import { waitForReadyScript, getReadinessConfig, isReadinessDisabled, + readinessDeadlineMs, runReadinessGate } from './serialize-dom.js'; @@ -111,6 +112,7 @@ export { waitForReadyScript, getReadinessConfig, isReadinessDisabled, + readinessDeadlineMs, runReadinessGate }; diff --git a/packages/sdk-utils/src/serialize-dom.js b/packages/sdk-utils/src/serialize-dom.js index ef7a15a3e..f5c1bc219 100644 --- a/packages/sdk-utils/src/serialize-dom.js +++ b/packages/sdk-utils/src/serialize-dom.js @@ -106,17 +106,72 @@ export function waitForReadyScript(readinessConfig = {}, { callback = false } = // options, // { callback: true, log } // ); +// Effective in-page timeout for each readiness preset, mirroring PRESETS in +// @percy/dom's readiness.js. Duplicated here because the presets live in the +// browser bundle, and the Node side needs the number to size its own deadline. +const PRESET_TIMEOUT_MS = { balanced: 10000, strict: 30000, fast: 5000 }; + +// Added to the in-page timeout before the Node side gives up, so a gate that is +// merely slow (eval round-trip, check teardown) is never pre-empted. +const READINESS_DEADLINE_GRACE_MS = 3000; + +// Wall-clock budget the Node side gives the readiness eval. +// +// PercyDOM.waitForReady() bounds itself with an in-page `setTimeout`, and every +// individual check settles on a `setTimeout`/`setInterval`. On a page whose +// timers are faked and paused -- Playwright's `page.clock.pauseAt()`, sinon +// `useFakeTimers`, jest fake timers -- neither the checks nor the gate's own +// timeout can ever fire, so the eval stays pending for the life of the page and +// hangs the test that called percySnapshot(). Node is the only side of that +// boundary guaranteed to have a real clock, so the backstop belongs here. +export function readinessDeadlineMs(readinessConfig = {}) { + let timeout = readinessConfig.timeoutMs ?? readinessConfig.timeout_ms ?? + PRESET_TIMEOUT_MS[readinessConfig.preset] ?? PRESET_TIMEOUT_MS.balanced; + let max = readinessConfig.maxTimeoutMs ?? readinessConfig.max_timeout_ms; + if (max != null) timeout = Math.min(timeout, max); + return timeout + READINESS_DEADLINE_GRACE_MS; +} + +const READINESS_DEADLINE_HIT = Symbol('readiness-deadline'); + export async function runReadinessGate(evalScript, snapshotOptions = {}, { callback = false, log } = {}) { if (isReadinessDisabled(snapshotOptions)) return null; const config = getReadinessConfig(snapshotOptions); const script = waitForReadyScript(config, { callback }); + const deadline = readinessDeadlineMs(config); + let timer; + try { - return await evalScript(script); + const evaluation = Promise.resolve(evalScript(script)); + // Hitting the deadline abandons `evaluation`, which may still reject much + // later -- when the driver tears the page down at end of test, say. Swallow + // that here so it never surfaces as an unhandled rejection. + evaluation.catch(() => {}); + + const result = await Promise.race([ + evaluation, + new Promise(resolve => { timer = setTimeout(() => resolve(READINESS_DEADLINE_HIT), deadline); }) + ]); + + if (result === READINESS_DEADLINE_HIT) { + if (log && typeof log.debug === 'function') { + log.debug( + `waitForReady did not settle within ${deadline}ms, proceeding to serialize. ` + + 'If this page fakes or pauses timers (e.g. Playwright page.clock), disable ' + + 'the readiness gate with snapshot.readiness.preset: disabled.' + ); + } + return null; + } + + return result; } catch (err) { if (log && typeof log.debug === 'function') { log.debug(`waitForReady failed, proceeding to serialize: ${err?.message || err}`); } return null; + } finally { + clearTimeout(timer); } } diff --git a/packages/sdk-utils/test/index.test.js b/packages/sdk-utils/test/index.test.js index 11ad23782..8e086fd1b 100644 --- a/packages/sdk-utils/test/index.test.js +++ b/packages/sdk-utils/test/index.test.js @@ -1043,6 +1043,108 @@ describe('SDK Utils', () => { let result = await runReadinessGate(() => Promise.reject(new Error('no log')), {}); expect(result).toBe(null); }); + + // PER-10756: PercyDOM.waitForReady() enforces its own timeout with an + // in-page setTimeout, so a page with faked/paused timers (Playwright + // page.clock.pauseAt, sinon useFakeTimers) can never settle the gate. The + // Node-side deadline is what keeps percySnapshot() from hanging the test. + it('returns null when evalScript never settles, without blocking on it', async () => { + let logged; + let result = await runReadinessGate( + () => new Promise(() => {}), + { readiness: { timeoutMs: 1000 } }, + { log: { debug: (m) => { logged = m; } } } + ); + expect(result).toBe(null); + expect(logged).toContain('did not settle within 4000ms'); + expect(logged).toContain('preset: disabled'); + }); + + it('does not raise an unhandled rejection when an abandoned eval rejects later', async () => { + let rejectEval; + let result = await runReadinessGate( + () => new Promise((resolve, reject) => { rejectEval = reject; }), + { readiness: { timeoutMs: 1000 } } + ); + expect(result).toBe(null); + // The driver tearing the page down after the deadline must stay silent. + rejectEval(new Error('Target page, context or browser has been closed')); + await new Promise(resolve => setTimeout(resolve, 10)); + }); + + it('still returns diagnostics from an eval that settles before the deadline', async () => { + let diagnostics = { passed: true, timed_out: false, preset: 'balanced' }; + let result = await runReadinessGate( + () => new Promise(resolve => setTimeout(() => resolve(diagnostics), 10)), + { readiness: { timeoutMs: 1000 } } + ); + expect(result).toEqual(diagnostics); + }); + }); + + describe('readinessDeadlineMs(readinessConfig)', () => { + let { readinessDeadlineMs } = utils; + + it('defaults to the balanced preset timeout plus grace', () => { + expect(readinessDeadlineMs()).toBe(13000); + expect(readinessDeadlineMs({})).toBe(13000); + expect(readinessDeadlineMs({ preset: 'balanced' })).toBe(13000); + }); + + it('uses the timeout of the named preset', () => { + expect(readinessDeadlineMs({ preset: 'strict' })).toBe(33000); + expect(readinessDeadlineMs({ preset: 'fast' })).toBe(8000); + }); + + it('falls back to balanced for an unknown preset', () => { + expect(readinessDeadlineMs({ preset: 'nonsense' })).toBe(13000); + }); + + it('prefers an explicit timeout over the preset, in either naming', () => { + expect(readinessDeadlineMs({ preset: 'strict', timeoutMs: 5000 })).toBe(8000); + expect(readinessDeadlineMs({ preset: 'strict', timeout_ms: 5000 })).toBe(8000); + }); + + it('clamps to maxTimeoutMs when that is lower', () => { + expect(readinessDeadlineMs({ timeoutMs: 20000, maxTimeoutMs: 6000 })).toBe(9000); + expect(readinessDeadlineMs({ timeoutMs: 20000, max_timeout_ms: 6000 })).toBe(9000); + // A higher cap leaves the configured timeout alone. + expect(readinessDeadlineMs({ timeoutMs: 4000, maxTimeoutMs: 30000 })).toBe(7000); + }); + + // Node-only, same rationale as the serialize-frames parity test above: + // PRESET_TIMEOUT_MS mirrors PRESETS in @percy/dom's readiness.js, which + // only exists in the browser bundle. Read the dom source and assert the + // timeouts still match, so drift fails loudly instead of silently + // producing a Node deadline shorter than the in-page one. + const isNodeEnv = typeof process !== 'undefined' && + typeof process.cwd === 'function' && + !!(process.versions && process.versions.node); + const itNodeEnv = isNodeEnv ? it : xit; + + itNodeEnv('stays in lockstep with the presets in @percy/dom/src/readiness.js', async () => { + const fs = await import('fs'); + const path = await import('path'); + const domSource = fs.readFileSync( + path.resolve(process.cwd(), '../dom/src/readiness.js'), + 'utf8' + ); + // Pull `timeout_ms:` out of each preset block in PRESETS. + const presets = domSource + .slice(domSource.indexOf('const PRESETS = {')) + .match(/(balanced|strict|fast):\s*\{[^}]*?timeout_ms:\s*(\d+)/g) + .reduce((acc, block) => { + const [, name, ms] = block.match(/(balanced|strict|fast):[\s\S]*timeout_ms:\s*(\d+)/); + acc[name] = Number(ms); + return acc; + }, {}); + + expect(presets).toEqual({ balanced: 10000, strict: 30000, fast: 5000 }); + // Every dom preset timeout must be the base of our deadline. + for (const [preset, timeout] of Object.entries(presets)) { + expect(readinessDeadlineMs({ preset })).toBe(timeout + 3000); + } + }); }); describe('mergeSnapshotOptions(options)', () => { From 8787e64f30a8f05753c2d8ddf9d3c086146734dd Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Fri, 11 Sep 2026 00:49:15 +0530 Subject: [PATCH 2/3] fix(sdk-utils): capture the native timer so a frozen Node clock cannot disable the deadline Review finding on the previous commit: the deadline was scheduled with a bare `setTimeout`, which resolves the global binding at call time. jest's and sinon's fake timers replace exactly that global, so a consumer whose Node test process had fake timers installed would schedule the deadline on a frozen clock and hang just as before -- one layer up from the in-page freeze the gate exists to survive, and directly against the "Node has a real clock" rationale. Reproduced by emulating what those libraries do to the globals (replace setTimeout, never advance): the gate hung indefinitely. With the timer captured at module load it returns null at the real 4s deadline and schedules nothing on the frozen clock. The capture holds because the SDK imports this module at require time, before a test body reaches useFakeTimers(). Deliberately NOT `import { setTimeout } from 'node:timers'`, which would additionally be immune to import order: this file is imported statically by index.js, which is bundled for the browser (the package's `browser` field), and the sole Node-only dependency here is reached through a lazy `await import('./proxy.js')` in request.js precisely to keep builtins out of that bundle. A static builtin import would defeat that. Co-Authored-By: Claude Opus 5 (1M context) --- packages/sdk-utils/src/serialize-dom.js | 22 +++++++++++++++++-- packages/sdk-utils/test/index.test.js | 28 +++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/packages/sdk-utils/src/serialize-dom.js b/packages/sdk-utils/src/serialize-dom.js index f5c1bc219..7d7dc10ea 100644 --- a/packages/sdk-utils/src/serialize-dom.js +++ b/packages/sdk-utils/src/serialize-dom.js @@ -132,6 +132,24 @@ export function readinessDeadlineMs(readinessConfig = {}) { return timeout + READINESS_DEADLINE_GRACE_MS; } +// Captured once at module load, because the deadline below must not be the very +// thing a faked clock disables. jest's and sinon's fake timers replace the +// *global* `setTimeout` binding, and a bare `setTimeout(...)` call resolves that +// global at call time -- so a consumer whose Node test process has fake timers +// installed would schedule the deadline on a frozen clock and hang exactly as +// before, one layer up from the in-page freeze this gate exists to survive. +// +// A module-scope capture holds because the SDK imports this module at require +// time, before a test body reaches `useFakeTimers()`. It is deliberately not +// `import { setTimeout } from 'node:timers'`, which would be immune to import +// order but pulls a Node builtin into this file -- and this file is imported +// statically by index.js, which is bundled for the browser too (see the package's +// `browser` field). The one Node-only dependency in this package is reached by a +// lazy `await import('./proxy.js')` in request.js precisely to keep builtins out +// of that bundle; a static builtin import here would defeat it. +const nativeSetTimeout = globalThis.setTimeout; +const nativeClearTimeout = globalThis.clearTimeout; + const READINESS_DEADLINE_HIT = Symbol('readiness-deadline'); export async function runReadinessGate(evalScript, snapshotOptions = {}, { callback = false, log } = {}) { @@ -150,7 +168,7 @@ export async function runReadinessGate(evalScript, snapshotOptions = {}, { callb const result = await Promise.race([ evaluation, - new Promise(resolve => { timer = setTimeout(() => resolve(READINESS_DEADLINE_HIT), deadline); }) + new Promise(resolve => { timer = nativeSetTimeout(() => resolve(READINESS_DEADLINE_HIT), deadline); }) ]); if (result === READINESS_DEADLINE_HIT) { @@ -171,7 +189,7 @@ export async function runReadinessGate(evalScript, snapshotOptions = {}, { callb } return null; } finally { - clearTimeout(timer); + nativeClearTimeout(timer); } } diff --git a/packages/sdk-utils/test/index.test.js b/packages/sdk-utils/test/index.test.js index 8e086fd1b..84fc52eec 100644 --- a/packages/sdk-utils/test/index.test.js +++ b/packages/sdk-utils/test/index.test.js @@ -1072,6 +1072,34 @@ describe('SDK Utils', () => { await new Promise(resolve => setTimeout(resolve, 10)); }); + // Regression: the deadline must not be schedulable on a clock the consumer + // has frozen. jest/sinon fake timers replace the global setTimeout binding, + // so a bare setTimeout(...) here would land on the frozen clock and hang + // exactly as the in-page gate does -- see the module-scope capture. + it('survives Node-side fake timers replacing the global setTimeout', async () => { + let realSetTimeout = globalThis.setTimeout; + let realClearTimeout = globalThis.clearTimeout; + let scheduledOnFrozenClock = 0; + + // Stand in for useFakeTimers() with no clock advancement: replace the + // globals with timers that are recorded and never fire. + globalThis.setTimeout = () => { scheduledOnFrozenClock++; return 0; }; + globalThis.clearTimeout = () => {}; + + try { + let result = await runReadinessGate( + () => new Promise(() => {}), + { readiness: { timeoutMs: 1000 } } + ); + expect(result).toBe(null); + // Nothing was handed to the frozen clock — the capture was used. + expect(scheduledOnFrozenClock).toBe(0); + } finally { + globalThis.setTimeout = realSetTimeout; + globalThis.clearTimeout = realClearTimeout; + } + }); + it('still returns diagnostics from an eval that settles before the deadline', async () => { let diagnostics = { passed: true, timed_out: false, preset: 'balanced' }; let result = await runReadinessGate( From 07814bd6a1455b42c1e5787524a377a5c39d3913 Mon Sep 17 00:00:00 2001 From: rishigupta1599 Date: Fri, 11 Sep 2026 09:56:19 +0530 Subject: [PATCH 3/3] test(sdk-utils): bound the fake-timer spec so a regression fails instead of cascading Review finding on the previous commit: the new regression spec was unsound in exactly the case it exists to catch. It replaces the global setTimeout, then awaits runReadinessGate. Against a regression the gate never settles, so the await never returns, the try/finally never restores the globals, and every later spec using a bare setTimeout hangs too -- one regression reported as a suite-wide cascade of unrelated timeouts. The spec now races the gate against a rejection on the real timer. 6000ms sits above the genuine 4000ms deadline (timeoutMs 1000 + 3000ms grace) so a healthy run resolves on its own, and below jasmine's 10s DEFAULT_TIMEOUT_INTERVAL (scripts/test-helpers.js) so this rejection, not the runner, reports the failure. Verified by running the spec body against both module versions: against the pre-capture code it now fails at 6003ms with a named error and the globals are restored (later bare-setTimeout specs unaffected); against the current code it passes at 4002ms. Also corrects the rationale comment, which overclaimed in two places: - The module-scope capture is not a guarantee. Fake timers installed before this module is first evaluated -- jest `fakeTimers.enableGlobally`, a setupFiles useFakeTimers(), or resetModules() under an already-faked clock -- capture the fake and the hang returns. Those consumers need preset: disabled, and the comment now says so rather than implying import order always saves us. - "The one Node-only dependency" was wrong; request.js also imports http/https dynamically and proxy.js pulls net/tls. Reworded, and noted that the rollup config suppresses MISSING_NODE_BUILTINS, so a static builtin import here would not fail the build -- it would ship a browser bundle that breaks at runtime. Co-Authored-By: Claude Opus 5 (1M context) --- packages/sdk-utils/src/serialize-dom.js | 25 +++++++++++++++++-------- packages/sdk-utils/test/index.test.js | 22 ++++++++++++++++++---- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/packages/sdk-utils/src/serialize-dom.js b/packages/sdk-utils/src/serialize-dom.js index 7d7dc10ea..937ea38b7 100644 --- a/packages/sdk-utils/src/serialize-dom.js +++ b/packages/sdk-utils/src/serialize-dom.js @@ -139,14 +139,23 @@ export function readinessDeadlineMs(readinessConfig = {}) { // installed would schedule the deadline on a frozen clock and hang exactly as // before, one layer up from the in-page freeze this gate exists to survive. // -// A module-scope capture holds because the SDK imports this module at require -// time, before a test body reaches `useFakeTimers()`. It is deliberately not -// `import { setTimeout } from 'node:timers'`, which would be immune to import -// order but pulls a Node builtin into this file -- and this file is imported -// statically by index.js, which is bundled for the browser too (see the package's -// `browser` field). The one Node-only dependency in this package is reached by a -// lazy `await import('./proxy.js')` in request.js precisely to keep builtins out -// of that bundle; a static builtin import here would defeat it. +// The capture holds for the ordinary case: the SDK imports this module at +// require time, before a test body or beforeEach reaches `useFakeTimers()`. It +// is NOT a guarantee. Fake timers installed before this module is first +// evaluated -- jest's `fakeTimers: { enableGlobally: true }`, a `useFakeTimers()` +// call in `setupFiles`, or `resetModules()` + a fresh require under an already +// faked clock -- capture the fake, and the hang returns. Those consumers need +// `snapshot.readiness.preset: disabled`. +// +// Deliberately not `import { setTimeout } from 'node:timers'`, which would be +// immune to import order too: this file is imported statically by index.js, and +// index.js is the rollup entry for the browser bundle (see the package's +// `browser` field). Node-only code in this package is always reached through a +// lazy `await import(...)` -- `http`/`https` in request.js, `./proxy.js` and its +// `net`/`tls` imports -- precisely to keep builtins out of that graph. A static +// builtin import here would not even fail the build, since the rollup config +// suppresses MISSING_NODE_BUILTINS; it would ship a browser bundle that breaks +// at runtime, which is worse. const nativeSetTimeout = globalThis.setTimeout; const nativeClearTimeout = globalThis.clearTimeout; diff --git a/packages/sdk-utils/test/index.test.js b/packages/sdk-utils/test/index.test.js index 84fc52eec..776d3ab12 100644 --- a/packages/sdk-utils/test/index.test.js +++ b/packages/sdk-utils/test/index.test.js @@ -1087,10 +1087,24 @@ describe('SDK Utils', () => { globalThis.clearTimeout = () => {}; try { - let result = await runReadinessGate( - () => new Promise(() => {}), - { readiness: { timeoutMs: 1000 } } - ); + // Bound the wait on the REAL timer. Without it, a regression here (the + // deadline scheduled on the frozen clock) never settles the await, so + // the finally below never restores the globals and every later spec + // using a bare setTimeout hangs too — one regression becomes a + // suite-wide cascade instead of one clean failure. 6000ms is above the + // real 4000ms deadline (timeoutMs 1000 + 3000ms grace) so a healthy run + // resolves on its own, and below jasmine's 10s spec timeout so this + // rejection, not the runner, is what reports the regression. + let result = await Promise.race([ + runReadinessGate( + () => new Promise(() => {}), + { readiness: { timeoutMs: 1000 } } + ), + new Promise((resolve, reject) => realSetTimeout(() => reject(new Error( + 'readiness deadline never fired: it was scheduled on the frozen global clock ' + + 'instead of the timer captured at module load' + )), 6000)) + ]); expect(result).toBe(null); // Nothing was handed to the frozen clock — the capture was used. expect(scheduledOnFrozenClock).toBe(0);