fix(core): customElements wait read its timeout from the wrong argument, hanging the CLI forever (PER-10405) by aryanku-dev · Pull Request #2383 · percy/cli

aryanku-dev · 2026-08-10T19:48:23Z

Symptom

percy snapshot hangs indefinitely — no error, no timeout, no output — against any live Next.js next start server. Bisects cleanly to @percy/cli: 1.31.13 works, 1.32.0 / 1.32.3 / 1.32.6 all hang identically. Reproduces on a completely unmodified create-next-app.

The reporter ruled out (all correctly): Chromium version (forced 1.31.13's binary into a 1.32.3 run), custom .percy.yml, snapshot count, localhost vs 127.0.0.1, the disk-backed logger, autoConfigureAllowedHostnames, and page content (a byte-identical static mirror of the same page does not hang). Only live Next server + 1.32.x hangs.

Root cause

packages/core/src/page.js:29 — WAIT_FOR_CUSTOM_ELEMENTS_BODY (added in 1.32.0) read its timeout from the wrong argument index.

serializeFunction() wraps a string body as:

(async function eval() { <body> })(percyHelpers, ...args)

so arguments[0] is the injected Percy helpers object and the caller's first argument lands at arguments[1]. The body read arguments[0]:

var deadline = Date.now() + (arguments[0] || 500);

The helpers object is truthy, so this became number + object → string concatenation. Probed live in the page during the repro:

{"argsLen":2,
 "arg0Type":"object",
 "arg0Keys":["config","snapshot","generatePromise","yieldFor","waitFor",
             "waitForTimeout","waitForSelector","waitForXPath","scrollToBottom"],
 "arg1":500,
 "deadlineValue":"1786389350594[object Object]",
 "deadlineType":"string",
 "comparisonResult":false}

Date.now() >= deadline therefore coerced to NaN and was permanently false — the deadline could never fire.

That is latent on most pages, because the other exit (if (!undef.length) return resolve()) clears immediately. Next.js is what makes it fatal: the app router mounts <next-route-announcer>, a tag Next never passes to customElements.define(). It matches :not(:defined) forever, and customElements.whenDefined('next-route-announcer') never resolves. Probe from the same run:

{"count":1,"names":["next-route-announcer"],"whenDefinedErrors":[]}

With both exits dead the poll re-ticked every 100 ms forever and the promise never settled. The body is awaited over CDP with awaitPromise: true, which has no protocol-level timeout, so page.snapshot() blocked forever.

This also explains the two things that made the report confusing:

Why no timeout fired. The hang is upstream of everything with a budget. PERCY_NETWORK_IDLE_WAIT_TIMEOUT is not involved — instrumenting the run shows network.idle() resolving normally twice before the hang.
Why a static mirror works. <next-route-announcer> is created by Next's client runtime at hydration, so it is absent from a static mirror of the server HTML → undef.length === 0 → immediate resolve.

Not Next-specific in principle: any page holding a never-registered custom-element tag (blocked third-party widget loader, typo'd tag name) hits the same hang. Next.js just guarantees one on every page.

Blast radius

The run hangs before build creation — the CLI issues exactly one API call (Fetching project domain config) and never calls POST /builds. No build appears in Percy at all, so these failures leave no server-side trace and are invisible to backend telemetry. Users see a wedged CI job.

Fix

packages/core/src/page.js:

Read the timeout from arguments[1], validated as a finite positive number. The serializeFunction argument-injection contract is now documented at the constant, since it is invisible from inside the body.
Add a settled guard plus an absolute setTimeout(finish, timeoutMs). This promise is awaited over CDP with no timeout of its own, so failing to settle must be structurally impossible, not merely unlikely — a ceiling, independent of the poll's own bookkeeping.
Route the tick chain's rejection path to finish (previously .then(tick) with no rejection handler: a throw on any tick after the first orphaned the outer promise), and wrap whenDefined(), which throws SyntaxError on an invalid custom element name.

Why tests didn't catch this

Useful for anyone adding a page-realm body later: the existing specs never execute this one. Both identity-match the exported constant and then bypass it — one asserts the wait eval count is 0, the other replaces the eval with throw new Error('boom'). The body only ever runs inside the page realm, where serializeFunction's argument injection applies, so a node-side suite can't reach it by construction. The spec below is the first that runs it in a real browser.

Testing

New regression spec in packages/core/test/percy.test.js, using the real browser harness against <next-route-announcer>. It asserts the fixture really is stuck (:not(:defined) count is 1) before asserting the wait settles on its own ceiling, so it can't silently pass without exercising the deadline branch.

✓ does not hang on a page holding a never-defined custom element
Executed 140 of 140 specs SUCCESS in 3 mins 14 secs.

End-to-end, against a bare create-next-app (next@16.3.0, react@19.2.8, node 22) served by next start -p 4176:

	result
`@percy/cli@1.32.6`	hangs past 120 s, killed by `timeout`, no build created
same run, patched	finalizes in 23 s — build #377

🤖 Generated with Claude Code

…nt, hanging the CLI forever (PER-10405) `percy snapshot` hung indefinitely — no error, no timeout, no output — on any page holding a custom-element tag that is never registered. Reported against every live Next.js `next start` server: the app router mounts `<next-route-announcer>`, which Next never passes to `customElements.define()`. Regression in 1.32.0 (1.31.13 predates the wait); 1.32.0/1.32.3/1.32.6 all hang. Root cause ---------- `serializeFunction()` wraps a string body as `(async function eval(){ <body> })(percyHelpers, ...args)`, so `arguments[0]` is the injected Percy helpers object and the caller's first argument lands at `arguments[1]`. `WAIT_FOR_CUSTOM_ELEMENTS_BODY` read `arguments[0]`, so: Date.now() + (arguments[0] || 500) => 1786389350594 + {config,snapshot,...} => "1786389350594[object Object]" // a String `Date.now() >= deadline` then compared a number against that string, coerced to NaN, and was permanently false. With `:not(:defined)` never clearing either, the poll had no exit: it re-ticked every 100ms forever and the returned promise never settled. The body is awaited over CDP with `awaitPromise: true`, which has no protocol-level timeout, so `page.snapshot()` blocked forever. Nothing downstream could report it — the run never even reached build creation, so no build appears in Percy at all. Fix --- - Read the timeout from `arguments[1]`, validated as a finite positive number. - Add a `settled` guard plus an absolute `setTimeout(finish, timeoutMs)` so the promise cannot fail to settle even if the tick chain breaks. This is awaited over CDP with no timeout of its own, so not settling must be structurally impossible rather than merely unlikely. - Route the tick chain's rejection path to `finish` and guard `whenDefined()`, which throws SyntaxError on an invalid custom element name. Verified against a bare `create-next-app` (next 16.3.0, node 22) served by `next start`: 1.32.6 hangs past 120s and creates no build; patched, the same run finalizes in 23s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pranavz28

We are changing core package. Recommended to run full SDK regression once

aryanku-dev

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

aryanku-dev · 2026-08-11T06:10:28Z

+      settled = true;
+      resolve();
+    }
+    setTimeout(finish, timeoutMs);


[Low] Ceiling setTimeout is never cleared on early resolve

When finish() is reached via the normal tick path (the common case — :not(:defined) empties on the first tick), this ceiling timer stays pending in the page realm until timeoutMs elapses. Harmless, since finish() is idempotent via settled, but it is an avoidable lingering timer.

Suggestion: capture the id and clear it in finish():

var t = setTimeout(finish, timeoutMs); function finish() { if (settled) return; settled = true; clearTimeout(t); resolve(); }

Reviewer: stack-code-reviewer

aryanku-dev · 2026-08-11T06:10:28Z

+    )).toBeResolved();
+    let elapsed = Date.now() - start;
+
+    expect(elapsed).toBeLessThan(5000);


[Low] Elapsed assertion is upper-bound only

< 5000 against a 500 ms timeout guards against a hang but not much else. Since the fixture element is genuinely never defined, the only way this resolves well before the deadline is a future regression that exits the wait early for the wrong reason — which this assertion would not catch.

Suggestion: pin the deadline branch with a loose lower bound alongside it.

Suggested change

expect(elapsed).toBeLessThan(5000);

expect(elapsed).toBeGreaterThanOrEqual(400);

expect(elapsed).toBeLessThan(5000);

Reviewer: stack-code-reviewer

aryanku-dev · 2026-08-11T06:10:30Z

Claude Code PR Review

PR: #2383 • Head: fb6efcd • Reviewers: stack-code-reviewer

Summary

Fixes an indefinite CLI hang (PER-10405) by correcting WAIT_FOR_CUSTOM_ELEMENTS_BODY to read its timeout from arguments[1] — serializeFunction() injects the Percy helpers object at arguments[0] — and adds a settled guard, an absolute ceiling setTimeout, a rejection path on the tick chain, and a try/catch around customElements.whenDefined(). Ships a browser-harness regression spec in packages/core/test/percy.test.js.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials, tokens, or URLs beyond a `localhost:8000` test fixture.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	The new `timeoutMs` guard (`typeof === 'number' && isFinite() && > 0`) is stricter than the old `\|\| 500` and rejects `NaN`, `Infinity`, and negatives.
High	Security	No IDOR — resource ownership validated	N/A	No resource access.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Pass	Argument-index claim verified against `serializeFunction()` (`packages/core/src/utils.js:696-702`) and the call site (`packages/core/src/page.js:270`). `arguments[0]` is always the injected helpers object; reading `arguments[1]` is correct. The `settled` flag makes double-resolve harmless and the ceiling `setTimeout` makes non-settlement structurally impossible.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	The previously-unhandled rejection path is now routed (`.then(tick, finish)`), and `whenDefined()` — which throws `SyntaxError` on invalid custom-element names — is wrapped. Swallowing is deliberate here: this is a best-effort wait, and failing closed would reintroduce the hang.
High	Correctness	No race conditions or concurrency issues	Pass	The ceiling timer and the tick chain can both reach `finish()`; the `settled` guard makes the race benign and idempotent.
Medium	Testing	New code has corresponding tests	Pass	New spec drives the body through the real `serializeFunction` + CDP `Runtime.callFunctionOn` path, which is the only level at which this argument-index bug is reachable.
Medium	Testing	Error paths and edge cases tested	Partial	The ceiling/deadline branch is covered. The `whenDefined()` `SyntaxError` path and the tick-chain rejection path are not directly exercised. Non-blocking — both are defensive additions, and the page realm makes them awkward to trigger.
Medium	Testing	Existing tests still pass (no regressions)	Pass	The two pre-existing specs match `WAIT_FOR_CUSTOM_ELEMENTS_BODY` by identity, not by argument count, so they are unaffected. Author reports 140/140 green; reviewer found no regression path.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Bounded by construction — this change is what makes the wait bounded.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	ES5-only constraint on the page-realm body is respected (no arrow functions, `let`/`const`, or optional chaining; global `isFinite` rather than `Number.isFinite`). ESLint clean on both files.
Medium	Quality	Changes are focused (single concern)	Pass	Two files, one concern.
Low	Quality	Meaningful names, no dead code	Pass	`timeoutMs`, `settled`, `finish` read clearly.
Low	Quality	Comments explain why, not what	Pass	The new comment documents the `serializeFunction` argument-injection contract — invisible from inside the body and the exact thing that caused the bug. Good addition.
Low	Quality	No unnecessary dependencies added	Pass	None added.

Findings

File: packages/core/src/page.js:43
Severity: Low
Reviewer: stack-code-reviewer
Issue: The ceiling setTimeout(finish, timeoutMs) is never cleared when finish() is reached earlier via the normal tick path (the common case — :not(:defined) empties on the first tick). Harmless, since finish() is idempotent via settled, but it leaves a pending timer alive in the page realm until the ceiling fires.

Suggestion: Capture the timer id and clear it in finish():

var t = setTimeout(finish, timeoutMs);
function finish() {
  if (settled) return;
  settled = true;
  clearTimeout(t);
  resolve();
}

File: packages/core/test/percy.test.js:245
Severity: Low
Reviewer: stack-code-reviewer
Issue: The elapsed-time assertion is an upper bound only (< 5000 against a 500 ms timeout). Because the fixture element is genuinely never defined, the only way this resolves well before the deadline is a future regression that makes the wait exit early for the wrong reason — and the current assertion would not catch it.
Suggestion: Add a loose lower bound alongside the existing check, e.g. expect(elapsed).toBeGreaterThanOrEqual(400);, so the spec pins the deadline branch rather than just the absence of a hang.

Raised by other reviewers (not independently confirmed)

@pranavz28 (approved, then commented): "We are changing core package. Recommended to run full SDK regression once." — A process recommendation rather than a code defect, so it is not gating here. Reasonable given @percy/core is a dependency of every SDK; the PR already reports an end-to-end create-next-app verification and 140/140 core specs, but a full SDK regression sweep before release is worth doing.

Verdict: PASS — root cause verified against the real serializeFunction/Page.eval implementation, the fix is ES5-safe and structurally sound, and the regression spec exercises the bug through the real CDP path. Two Low nits, neither blocking.

aryanku-dev requested a review from a team as a code owner August 10, 2026 19:48

aryanku-dev requested review from Shivanshu-07 and ninadbstack August 10, 2026 19:48

aryanku-dev force-pushed the fix/PER-10405-custom-elements-wait-hang branch from cf387bc to fb6efcd Compare August 11, 2026 04:41

pranavz28 approved these changes Aug 11, 2026

View reviewed changes

pranavz28 reviewed Aug 11, 2026

View reviewed changes

aryanku-dev commented Aug 11, 2026

View reviewed changes

aryanku-dev merged commit f86a18d into master Aug 11, 2026
48 checks passed

aryanku-dev deleted the fix/PER-10405-custom-elements-wait-hang branch August 11, 2026 07:59

Navigation Menu

Uh oh!

fix(core): customElements wait read its timeout from the wrong argument, hanging the CLI forever (PER-10405) - #2383

fix(core): customElements wait read its timeout from the wrong argument, hanging the CLI forever (PER-10405)#2383
aryanku-dev merged 1 commit into
masterfrom
fix/PER-10405-custom-elements-wait-hang

aryanku-dev commented Aug 10, 2026 •

edited by atlassian Bot

Loading

Uh oh!

pranavz28 left a comment

Uh oh!

aryanku-dev left a comment

Uh oh!

aryanku-dev Aug 11, 2026

Uh oh!

aryanku-dev Aug 11, 2026

Uh oh!

aryanku-dev commented Aug 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	expect(elapsed).toBeLessThan(5000);
	expect(elapsed).toBeGreaterThanOrEqual(400);
	expect(elapsed).toBeLessThan(5000);

Uh oh!

Conversation

aryanku-dev commented Aug 10, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Symptom

Root cause

Blast radius

Fix

Why tests didn't catch this

Testing

Uh oh!

pranavz28 left a comment

Choose a reason for hiding this comment

Uh oh!

aryanku-dev left a comment

Choose a reason for hiding this comment

Uh oh!

aryanku-dev Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

aryanku-dev Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

aryanku-dev commented Aug 11, 2026

Claude Code PR Review

Summary

Review Table

Findings

Raised by other reviewers (not independently confirmed)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aryanku-dev commented Aug 10, 2026 •

edited by atlassian Bot

Loading