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

[aryanku-dev]

Fixes PER-10514.

The report

A Cypress customer (@percy/cypress@3.1.9, @percy/cli@1.32.6) intermittently gets, on and off for months:

cy.then() timed out after waiting 45000ms.
Your callback function returned a promise that never resolved.

The callback function was:
async doc => {
  ...
  let response = await withRetry(async () => await withLog(async () => {
    return await utils.postSnapshot({ ... });
  }, 'posting dom snapshot', _throw));

The 45 000 ms is ours, not theirs — @percy/cypress hard-codes CY_TIMEOUT = 30 * 1000 * 1.5 (index.js:10) and applies it to the cy.document().then({ timeout: CY_TIMEOUT }, …) that posts the snapshot (index.js:540). So the message means exactly one thing: POST localhost:5338/percy/snapshot did not answer within 45 s.

Root cause

POST /percy/snapshot doesn't answer until percy.snapshot() resolves (core/src/api.js:230-239). The first thing that generator awaited was not asset discovery — it was the SDK version check (core/src/percy.js:649-653):

if (!this.sdkInfoDisplayed && options.clientInfo) {
  await checkSDKVersion(options.clientInfo);   // ← the SDK's HTTP response waits on this
  this.sdkInfoDisplayed = true;
}

checkSDKVersion() (core/src/utils.js:1016-1046) calls https://api.github.com/repos/percy/<sdk-repo>/releases?page=1 purely so we can log an [SDK Update Available] warning. @percy/cypress is in PACKAGE_TO_REPO, so every Cypress user takes this path.

That request goes through client/src/utils.js:228 — http.request(requestOptions) with no socket timeout and no 'timeout' handler. Node's default socket timeout is 0 (never). A peer that completes the TCP handshake and then goes silent — a corporate proxy, an egress firewall that drops instead of rejecting, GitHub throttling — hangs the promise forever. retries: 0 can't help: the single attempt never settles, so there is nothing to retry. The SDK's own withRetry can't help either — it only retries rejections.

So one unreachable third party we don't own stalls snapshots indefinitely.

Aggravator in the same block: this.sdkInfoDisplayed = true was set after the await. While the first check hangs the flag is still false, so every subsequent snapshot in that CLI process enters the branch and fires its own GitHub request. A single bad network moment costs the whole run, not one test — which is why the customer reports run-level failures rather than one flaky spec.

Reproduction

Black-hole api.github.com the way a dropping firewall does — a TCP server that accepts the connection and never writes a byte — set it as HTTPS_PROXY, NO_PROXY the local addresses, then POST /percy/snapshot with clientInfo: '@percy/cypress/3.1.9':

[repro] percy core started, address = http://localhost:5338
[blackhole] connection accepted, holding it open forever
[repro] *** 45000ms elapsed and POST /percy/snapshot has NOT responded ***
[repro] still hanging at 70002ms — no timeout exists on the github call

Identical run with api.github.com reachable:

[repro] RESPONSE after 617ms: 200 {"success":true}

617 ms vs >70 s. The whole difference is that one call.

The fix

• packages/core/src/percy.js — set sdkInfoDisplayed before the call and run checkSDKVersion() detached. A snapshot never waits on it, and exactly one check is started per CLI process. checkSDKVersion already swallows its own errors, so nothing escapes.
• packages/core/src/utils.js — bound the GitHub request at 5 s. Deliberately short: now that the check is detached, its socket is a live handle that would otherwise hold the event loop open at shutdown for as long as the peer stays silent.
• packages/client/src/utils.js — make the timeout option actually do something. Node's timeout option only emits a 'timeout' event; without destroying the request, passing a timeout was a no-op. Now timeout destroys the request with ETIMEDOUT, which surfaces through the existing 'error' path. ETIMEDOUT is not in RETRY_ERROR_CODES, so this does not change retry behaviour for any existing caller — and no existing caller passes timeout, so behaviour is unchanged unless opted into.

Testing

• packages/core/test/snapshot.test.js — "does not wait on the SDK version check to take a snapshot": mocks api.github.com with a reply that never resolves and asserts percy.snapshot() still resolves (and the snapshot still uploads). Hangs on master, passes here.
• packages/client/test/unit/request.test.js — a request with timeout against a server that accepts and never answers now rejects with "timed out after 200ms"; a normal request with a generous timeout is unaffected.

yarn workspace @percy/core test --node: 104 specs, 5 failures — the same 5 pre-existing runDoctorOnFailure failures that fail on unmodified master in this environment (@percy/cli-doctor isn't linked locally).
yarn workspace @percy/client test: 279 specs, 2 failures — the same 2 pre-existing proxy failures that fail on unmodified master (Node-version-dependent Invalid URL message).

Follow-ups, not in this PR

• @percy/sdk-utils's request.post passes timeout: 600000 (sdk-utils/src/request.js:32) — dead code for the same reason, in both the node and the window.fetch implementations. Worth fixing, but it changes behaviour for every SDK, so it doesn't belong here.
• Even with this fixed, POST /percy/snapshot still blocks on asset discovery. api.js already supports ?async; whether the Cypress SDK should use it is an SDK-side design question.

🤖 Generated with Claude Code

[aryanku-dev]

Claude Code Review (automated) — 4 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[aryanku-dev]

[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

[aryanku-dev]

[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

[aryanku-dev]

[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

[aryanku-dev]

[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

[aryanku-dev]

Claude Code PR Review

PR: #2387 • Head: 931b1b6 • Reviewers: stack-code-reviewer

Summary

Stops the informational SDK version check from gating snapshots: sdkInfoDisplayed is now set before the call and checkSDKVersion() runs detached, the GitHub request is bounded at 5 s, and @percy/client's previously-inert timeout option now destroys the request with ETIMEDOUT — fixing PER-10514, where a black-holed api.github.com hung POST /percy/snapshot and tripped Cypress's 45 s callback timeout.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials introduced; only a User-Agent header and a numeric timeout constant.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	repoName still comes from the fixed PACKAGE_TO_REPO map, not from user input.
High	Security	No IDOR — resource ownership validated	N/A	No resource access by identifier.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Pass	Call chain verified against api.js:230-238; the detach/flag-first ordering removes both the block and the duplicate-request amplifier. req.destroy(error) reliably emits 'error' on Node ≥14 (engines.node is >=14), and handleError.handled dedupes against a concurrent res error.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	checkSDKVersion wraps its whole body in one try/catch, so the detached call cannot produce an unhandled rejection today (see Low finding 4 for the fragility).
High	Correctness	No race conditions or concurrency issues	Pass	Setting sdkInfoDisplayed before the await closes the window in which concurrent snapshots each fired their own GitHub request.
Medium	Testing	New code has corresponding tests	Fail	Two of the three defenses are covered; the timeout: SDK_VERSION_CHECK_TIMEOUT forwarding in checkSDKVersion has no assertion — see finding 2.
Medium	Testing	Error paths and edge cases tested	Pass	The silent-server test in request.test.js exercises the new ETIMEDOUT path and genuinely fails without the fix.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Reported failures (@percy/core 5, @percy/client 2) reproduce on unmodified master in the same environment.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Strictly reduces work: one version check per CLI process instead of one per snapshot during a slow check.
Medium	Performance	Long-running tasks use background jobs	Pass	This is precisely the change — the check is moved off the request-response path.
Medium	Quality	Follows existing codebase patterns	Pass	Reuses the existing request() options bag and the established RETRY_ERROR_CODES/handleError flow.
Medium	Quality	Changes are focused (single concern)	Pass	Three edits all serve the one bug; the PR explicitly defers sdk-utils' equivalent dead timeout to a follow-up.
Low	Quality	Meaningful names, no dead code	Pass	SDK_VERSION_CHECK_TIMEOUT is well named; the change in fact removes dead code by giving timeout an effect.
Low	Quality	Comments explain why, not what	Pass	The new comments are unusually good — they explain the request-handler coupling and why the timeout is short.
Low	Quality	No unnecessary dependencies added	Pass	None added.

Findings

• File: packages/core/test/snapshot.test.js:60

• Severity: Medium

• Reviewer: stack-code-reviewer

• Issue: The tooSlow guard's setTimeout(..., 10000) handle is never captured or cleared. In the expected case percy.snapshot() wins the race and the 10 s timer keeps running afterwards — a live handle that can hold the worker open for up to 10 s past the spec. It will not 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);
  }

• File: packages/core/src/utils.js:1037

• Severity: Medium

• Reviewer: stack-code-reviewer

• Issue: checkSDKVersion now passes timeout: SDK_VERSION_CHECK_TIMEOUT, but no test asserts it is forwarded. The existing describe('checkSDKVersion', …) specs stub http.request via mockRequests, so a real timeout could never fire there. This is the second of the PR's three defenses and currently has zero regression protection — deleting the timeout: line would keep every test green.

• Suggestion: Assert the option reaches the request, e.g.

  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());
  });

• File: packages/client/src/utils.js:238

• Severity: Low

• Reviewer: stack-code-reviewer

• Issue: The destroy error is tagged code: 'ETIMEDOUT', which is deliberately absent from RETRY_ERROR_CODES (packages/client/src/utils.js:117-120). That is harmless today because both callers that pass timeout — checkSDKVersion and Client#validateDomain (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 will get a hard failure where they would reasonably expect a retry, with nothing in the code saying so.

• Suggestion: Make the decision explicit — either add 'ETIMEDOUT' to RETRY_ERROR_CODES, or add one line at the req.on('timeout', …) block noting that timeout errors are intentionally not retried.

• File: packages/core/src/percy.js:660

• Severity: Low

• Reviewer: stack-code-reviewer

• Issue: The fire-and-forget checkSDKVersion(options.clientInfo) relies entirely on the callee's internal try/catch for safety. That holds right now (a single try/catch wraps the whole body, and the only await is 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: checkSDKVersion(options.clientInfo).catch(() => {});

---

Verdict: PASS — the root cause is correctly identified and all three defenses are individually sound; the two Medium items are worth fixing before merge but neither is blocking.