From 931b1b62b603ca30d4fc499b01d3926062a52573 Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Sat, 15 Aug 2026 17:22:53 +0530 Subject: [PATCH] fix(core): the SDK version check blocked every snapshot POST, hanging Cypress at 45s (PER-10514) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /percy/snapshot` does not answer the SDK until `percy.snapshot()` resolves (api.js:230-239). The first thing that generator awaited was not asset discovery — it was `checkSDKVersion()`, which calls `https://api.github.com/repos/percy//releases` purely to print an "[SDK Update Available]" warning. That request is issued with `http.request()` and no socket timeout and no 'timeout' handler, so a peer that completes the TCP handshake and then goes silent — a corporate proxy, an egress firewall that drops rather than rejects, GitHub throttling — hangs it forever. `retries: 0` cannot rescue it because the single attempt never settles. @percy/cypress caps that POST at CY_TIMEOUT = 45000 (index.js:10, :540), so the customer sees: cy.then() timed out after waiting 45000ms. Your callback function returned a promise that never resolved. with `utils.postSnapshot` in the printed callback, intermittently, for as long as their CI network occasionally can't reach GitHub. `sdkInfoDisplayed` was also set *after* the await, so while the first check hung the flag stayed false and every subsequent snapshot in that process started its own GitHub request — one bad network moment cost the whole run, not one test. Reproduced with a TCP black-hole as HTTPS_PROXY (NO_PROXY for the local addresses): POST /percy/snapshot answers in 617ms normally and had still not answered after 70s with api.github.com unreachable. - percy.js: set the flag first and run the check detached — a snapshot never waits on it, and exactly one check is started per process. - utils.js: bound the GitHub request at 5s. Kept short because the detached socket is a live handle that would otherwise hold the event loop open at shutdown. - client/utils.js: make the `timeout` option mean something. Node's `timeout` only *emits* an event; without destroying the request, passing a timeout was a no-op. Co-Authored-By: Claude Opus 5 (1M context) --- packages/client/src/utils.js | 13 ++++++++++ packages/client/test/unit/request.test.js | 23 +++++++++++++++++ packages/core/src/percy.js | 12 +++++++-- packages/core/src/utils.js | 12 ++++++++- packages/core/test/snapshot.test.js | 30 ++++++++++++++++++++++- 5 files changed, 86 insertions(+), 4 deletions(-) diff --git a/packages/client/src/utils.js b/packages/client/src/utils.js index 323d4ca9b..f5a3927b1 100644 --- a/packages/client/src/utils.js +++ b/packages/client/src/utils.js @@ -228,6 +228,19 @@ export async function request(url, options = {}, callback) { let req = http.request(requestOptions); req.on('response', handleResponse); req.on('error', handleError); + + // Node's `timeout` option only *emits* a 'timeout' event once the socket + // has been idle that long — it does not abort anything. Without this + // listener a caller passing `timeout` still waits forever on a peer that + // accepts the connection and then goes silent, and the promise never + // settles. Destroying the request surfaces it through 'error' instead. + if (requestOptions.timeout) { + req.on('timeout', () => req.destroy(Object.assign( + new Error(`Request to ${url} timed out after ${requestOptions.timeout}ms`), + { code: 'ETIMEDOUT' } + ))); + } + req.end(body); }, { retries, interval }); } diff --git a/packages/client/test/unit/request.test.js b/packages/client/test/unit/request.test.js index 05fe441e7..5666ce002 100644 --- a/packages/client/test/unit/request.test.js +++ b/packages/client/test/unit/request.test.js @@ -210,6 +210,29 @@ describe('Unit / Request', () => { .toBeRejectedWithError('403 \nSTOP'); }); + describe('timeout', () => { + let silent; + + beforeEach(async () => { + // a server that accepts the connection and then never answers + silent = await createTestServer({ port: 8081 }, () => {}).start(); + }); + + afterEach(async () => { + await silent?.close(); + }); + + it('rejects instead of hanging forever when a timeout is given', async () => { + await expectAsync(silent.request('/', { timeout: 200, retries: 0 })) + .toBeRejectedWithError(/timed out after 200ms/); + }); + + it('does not time out requests that answer in time', async () => { + await expectAsync(server.request('/', { timeout: 10000 })) + .toBeResolvedTo('test'); + }); + }); + describe('retries', () => { it('automatically retries server 500 errors', async () => { let responses = [[502], [503], [520], [200]]; diff --git a/packages/core/src/percy.js b/packages/core/src/percy.js index 933215e9b..05a535a2d 100644 --- a/packages/core/src/percy.js +++ b/packages/core/src/percy.js @@ -646,10 +646,18 @@ export class Percy { let server; try { - // Check SDK version + // Check SDK version. This generator runs inside the POST /percy/snapshot + // request handler (see api.js), so anything awaited here delays the HTTP + // response the SDK is blocked on. checkSDKVersion reaches out to + // api.github.com — an unrelated third party that a corporate proxy, + // egress firewall or GitHub throttling can leave hanging — so it must + // never gate a snapshot. Set the flag first so exactly one check is ever + // started (it used to be set after the await, which meant every snapshot + // posted during a slow check started its own), and run it detached: + // checkSDKVersion swallows its own errors and only logs. if (!this.sdkInfoDisplayed && options.clientInfo) { - await checkSDKVersion(options.clientInfo); this.sdkInfoDisplayed = true; + checkSDKVersion(options.clientInfo); } if ('serve' in options) { // create and start a static server diff --git a/packages/core/src/utils.js b/packages/core/src/utils.js index a28ea02ca..917e36988 100644 --- a/packages/core/src/utils.js +++ b/packages/core/src/utils.js @@ -978,6 +978,12 @@ export async function* maybeScrollToBottom(page, discovery) { } } +// How long the (purely informational) SDK version check may spend talking to +// api.github.com before it gives up. Kept short deliberately: the check now runs +// detached, so its socket is a live handle that would otherwise hold the event +// loop open at shutdown for as long as the peer stays silent. +const SDK_VERSION_CHECK_TIMEOUT = 5000; + // Package to GitHub repo mapping const PACKAGE_TO_REPO = { '@percy/selenium-webdriver': 'percy-selenium-js', @@ -1022,9 +1028,13 @@ export async function checkSDKVersion(clientInfo) { return; } - // Fetch latest version from GitHub releases + // Fetch latest version from GitHub releases. api.github.com is a third + // party we don't control and networks routinely black-hole it (proxies, + // egress firewalls, rate limiting), so this is bounded — without a timeout + // the socket stays open for the life of the CLI process. const githubData = await request(`https://api.github.com/repos/percy/${repoName}/releases?page=1`, { headers: { 'User-Agent': '@percy/cli' }, + timeout: SDK_VERSION_CHECK_TIMEOUT, retries: 0 }); diff --git a/packages/core/test/snapshot.test.js b/packages/core/test/snapshot.test.js index cc86acbe5..21bd6819c 100644 --- a/packages/core/test/snapshot.test.js +++ b/packages/core/test/snapshot.test.js @@ -1,5 +1,5 @@ import { sha256hash, base64encode } from '@percy/client/utils'; -import { logger, api, setupTest, createTestServer, dedent } from './helpers/index.js'; +import { logger, api, setupTest, createTestServer, dedent, mockRequests } from './helpers/index.js'; import { waitFor } from '@percy/core/utils'; import Percy from '@percy/core'; import { handleSyncJob } from '../src/snapshot.js'; @@ -48,6 +48,34 @@ describe('Snapshot', () => { expect(() => percy.snapshot({})).toThrowError('Not running'); }); + // PER-10514: POST /percy/snapshot does not answer until percy.snapshot() + // resolves, so anything the generator awaits stalls the SDK. The version + // check talks to api.github.com — a third party that proxies and egress + // firewalls routinely leave hanging — and it must never gate a snapshot. + it('does not wait on the SDK version check to take a snapshot', async () => { + let ghAPI = await mockRequests('https://api.github.com'); + // accept the request and never answer, the way a black-holing proxy does + ghAPI.and.returnValue(new Promise(() => {})); + + let tooSlow = new Promise((resolve, reject) => setTimeout(() => { + reject(new Error('percy.snapshot() blocked on the SDK version check')); + }, 10000)); + + await expectAsync(Promise.race([ + percy.snapshot({ + name: 'test snapshot', + url: 'http://localhost:8000', + domSnapshot: testDOM, + clientInfo: '@percy/cypress/3.1.9' + }), + tooSlow + ])).toBeResolved(); + + await percy.idle(); + expect(api.requests['/builds/123/snapshots'][0].body.data.attributes.name) + .toEqual('test snapshot'); + }); + it('errors when missing a url', () => { expect(() => percy.snapshot({ name: 'test snapshot' })) .toThrowError('Missing required URL for snapshot');