Skip to content

fix(core): SDK version check blocked every snapshot POST, hanging Cypress at 45s (PER-10514) #2387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aryanku-dev merged 1 commit into from
Aug 18, 2026
Merged

fix(core): SDK version check blocked every snapshot POST, hanging Cypress at 45s (PER-10514) #2387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions packages/client/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(

@aryanku-dev aryanku-dev Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] ETIMEDOUT is not retryable — worth stating deliberately

This destroy error is tagged code: 'ETIMEDOUT', which is absent from RETRY_ERROR_CODES (packages/client/src/utils.js:117-120). Harmless today, since both callers that pass timeout (checkSDKVersion and Client#validateDomain at packages/client/src/client.js:1001-1018) also pass retries: 0. But now that timeout has teeth, a future caller combining timeout with non-zero retries gets a hard failure where a retry is the reasonable expectation, and nothing in the code says so.

Suggestion: make the decision explicit — either add 'ETIMEDOUT' to RETRY_ERROR_CODES, or add one line here noting timeout errors are intentionally not retried.

Reviewer: stack-code-reviewer

new Error(`Request to ${url} timed out after ${requestOptions.timeout}ms`),
{ code: 'ETIMEDOUT' }
)));
}

req.end(body);
}, { retries, interval });
}
Expand Down
23 changes: 23 additions & 0 deletions packages/client/test/unit/request.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]];
Expand Down
12 changes: 10 additions & 2 deletions packages/core/src/percy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@aryanku-dev aryanku-dev Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Fire-and-forget call has no defensive .catch()

This relies entirely on checkSDKVersion's internal try/catch for safety. That holds right now — a single try/catch wraps the whole body and the only await sits inside it — but any later refactor that adds code outside the try, or rethrows, reintroduces an unhandled rejection here with no test to catch it.

Suggestion: make the intent local and refactor-proof:

Suggested change
checkSDKVersion(options.clientInfo);
checkSDKVersion(options.clientInfo).catch(() => {});

Reviewer: stack-code-reviewer

}
if ('serve' in options) {
// create and start a static server
Expand Down
12 changes: 11 additions & 1 deletion packages/core/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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,

@aryanku-dev aryanku-dev Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] No regression test that this timeout is forwarded

Nothing asserts that timeout: SDK_VERSION_CHECK_TIMEOUT actually reaches the request. The existing describe('checkSDKVersion', …) specs stub http.request via mockRequests, so a real timeout could never fire there — deleting this line would keep every test green, even though it is the second of the PR's three defenses.

Suggestion: assert the option reaches the request:

it('passes a bounded timeout to the GitHub request', async () => {
  ghAPI.and.returnValue([200, []]);
  await checkSDKVersion('@percy/selenium-webdriver/2.2.0');
  expect(http.request).toHaveBeenCalledWith(
    jasmine.objectContaining({ timeout: 5000 }), jasmine.anything());
});

Reviewer: stack-code-reviewer

retries: 0
});

Expand Down
30 changes: 29 additions & 1 deletion packages/core/test/snapshot.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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));
Comment on lines +60 to +62

@aryanku-dev aryanku-dev Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Leaked 10s timer in the race guard

The setTimeout handle is never captured or cleared. In the expected case percy.snapshot() wins the race and this 10s timer keeps running afterwards — a live handle that can hold the worker open for up to 10s past this spec. It won't surface as an unhandled rejection (Promise.race attaches a handler), so it fails silently as wasted CI time.

Suggestion: capture the id and clear it in a finally:

let timer;
let tooSlow = new Promise((resolve, reject) => {
  timer = setTimeout(() => reject(new Error('percy.snapshot() blocked on the SDK version check')), 10000);
});
try {
  await expectAsync(Promise.race([percy.snapshot({ /* … */ }), tooSlow])).toBeResolved();
} finally {
  clearTimeout(timer);
}

Reviewer: stack-code-reviewer


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');
Expand Down
Loading