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');