fix(core): bound CDP sends so a stalled browser can't hang the CLI

[rishigupta1599]

Summary

Browser.send and Session.send resolved only on a matching CDP reply or a close — neither had a deadline of its own. Chrome does not always reply: a renderer or browser process that is frozen, starved of CPU, or swapped out keeps its target attached and simply goes silent.

Callers cannot defend against this either, because their deadlines are in-page timers living in the very renderer that stopped running. That is why @percy/storybook's PERCY_STORY_RENDER_TIMEOUT (added in percy/percy-storybook#1354, shipped in 10.0.2) never fires in these runs.

Net effect: a total silent stall — no error, no log, no retry, and a build left unfinalized until the server force-closes it ~8h later.

This adds a Node-side deadline that nothing inside the page can defeat, on both the session-scoped and browser-scoped connections — target creation and attachment run on the browser connection before any page exists, so bounding sessions alone still strands that path:

• Default 120s, floored above the longest legitimate in-page wait (~45s) and auto-raised if PERCY_PAGE_LOAD_TIMEOUT / PERCY_STORY_RENDER_TIMEOUT are raised, so tuning those can never cause a spurious protocol timeout.
• PERCY_CDP_TIMEOUT overrides it; PERCY_CDP_TIMEOUT=0 disables it.
• On expiry it rejects, so the SDK's existing withPage retry handles it — the same retry that already recovers a story whose preview page reloads mid-transition.
• The timer is cleared on reply and on close, so it can never fire late. The raw-message path stays untimed — a session owns that reply.

Why the previous fix wasn't enough

This was previously diagnosed as the preview page reloading itself (next/router calling window.location.reload()), and fixed with an in-page render deadline. That fix is correct and shipped, but this is a different branch of the same failure — reported runs on 10.0.2 still stall at the same story with the 30s deadline never firing.

Reproduced with the real CLI Page driving the real shipped evalSetCurrentStory:

scenario	in-page 30s deadline	CDP reply	result
story never renders (renderer running)	fires	—	clean error ✅
self-reload (next/router), 7 timings	n/a	rejects in 52–464ms	retry ✅
renderer crash	n/a	Session crashed!	rejects ✅
page frozen (Page.setWebLifecycleState)	can't fire	never	silent hang ∞
renderer SIGSTOP (alive, unscheduled)	can't fire	never	silent hang ∞

The last two rows match the reported signature exactly: total silence, no error, no timeout, CPU 0.5–5%, target still attached — consistent with a 2-core/8GB CI agent under memory pressure.

Test plan

• New unit specs — packages/core/test/unit/session.test.js (11 specs): for each of Session.send and Browser.send, the deadline fires and names the command, points at the PERCY_CDP_TIMEOUT override, does not fire when a reply arrives, is cleared on reply so it cannot fire late, is cleared on close, and is disabled by PERCY_CDP_TIMEOUT=0; plus the raw-message path staying untimed.
• Reproduced end to end on a 2-core container running the real percy storybook (@percy/cli 1.32.7 + @percy/storybook 10.0.2, Chrome 143 linux-x64). Unfixed, the run goes completely silent after Page created at ~0.1% CPU until the 5-minute monitoring timeout — no error, no timeout, no retry:

[percy:core:page] Page created (911ms)
[percy:core:page] Page created (35ms)
... monitoring heartbeats only ...
[percy:monitoring] Stopped monitoring system metrics

With the deadline backported onto that same 1.32.7 install, the identical run reports the hanging command and recovers via the existing retry:

[percy:core:session] Error: Protocol error (Runtime.enable): Timed out after 30000ms ...
[percy:core:session] Error: Protocol error (Network.enable): Timed out after 30000ms ...
[percy:storybook:utils] Retrying because error occurred in: about url, attempt: 1

That also identifies what was stranding the process — Runtime.enable / Network.enable, the first commands sent to a new page's session. Core asset-discovery plumbing, unrelated to any SDK.

• A/B confirms causation: with the deadline disabled (PERCY_CDP_TIMEOUT=0) the same runs hang indefinitely again.
• Normal path unaffected: with the renderer running, an in-page deadline still wins the race (4.0s vs a 6.0s CDP deadline), so callers keep their own better-quality errors.
• eslint clean.

Note on the browser-scoped deadline

The first version of this PR bounded Session.send only, on the reasoning that a wedged renderer leaves the browser process responsive. The container run disproved that: it stranded in browser.page() → Target.createBrowserContext, before any page existed. Browser.send has the identical unbounded-promise shape, so it is bounded here too.

[rishigupta1599]

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

[rishigupta1599]

[Medium] CDP timeout errors are not recognised by the browser-restart retry heuristic

discovery.js:722 restarts the browser on retry only when the error message contains Browser not connected, Browser closed, Session closed or Session crashed. The message this function builds matches none of them, so a protocol timeout never triggers percy.browser.restart().

Recorded Medium rather than the High originally proposed: browser.page(...) is called inside the retried generator (discovery.js:640), so each retry already gets a fresh page and renderer. For the frozen-renderer case this PR targets, the retry escapes the wedged renderer without a restart. The gap is real only for a wedged browser process, where the snapshot fails with a clear error after ~3x the deadline instead of hanging forever — slower recovery, not incorrect behaviour.

Suggestion: export an isCdpTimeoutError(error) predicate here next to cdpTimeoutMessage and use it in the onRetry check, so the restart path also fires on a protocol timeout and the contract is not a duplicated substring match.

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Timeout resolves into process-global, first-caller-wins state

The resolved deadline is frozen on the Session class for the life of the process, so the first CDP send anywhere wins. Two Percy instances in one process with different config silently share the first value, and no shared test helper resets it — a latent cross-spec pollution vector (discovery.test.js:2137 sets PERCY_PAGE_LOAD_TIMEOUT=80000, which would feed inPageCeiling() if it were the first caller).

Recorded Low rather than Medium: this mirrors the pre-existing Page.TIMEOUT at page.js:75 / page.js:406-408 rather than introducing the pattern, and the new specs reset it in both beforeEach and afterEach.

Suggestion: scope the resolved value to the Browser/Percy instance, or export a reset hook wired into the shared setupTest(). Best done as a follow-up covering Page.TIMEOUT too.

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] PERCY_STORY_RENDER_TIMEOUT has no consumer in this repo

This is its only occurrence in the monorepo, so a reader cannot tell from here whether it is intentional or a leftover. It is intentional — the Storybook SDK sets it outside this repo, and keeping the Node deadline above that in-page wait is the whole point of inPageCeiling().

Suggestion: name the owning package in the comment, e.g. "set by @percy/storybook; not read anywhere else in this repo", so the reference explains itself.

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Deadline timers are not unref'd

Same at browser.js:308. For a programmatic consumer that finishes without closing the session or browser, an in-flight command with no reply can hold the event loop open for the full deadline. Moot for percy exec, which calls process.exit() explicitly.

Suggestion: callback.timer.unref?.(), matching the existing precedent at server.js:226 — a watchdog timer should never be the reason a process cannot exit.

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Specs assert timing with real sleeps rather than fake timers

The 10ms tick() plus the 150/200/250ms waits add about a second of wall-clock time and make the "has not settled" assertions probabilistic under load rather than deterministic.

Suggestion: use jasmine.clock() for the scheduling assertions, keeping real timers only where the awaited browser.send() microtask genuinely needs them.

Reviewer: stack-code-reviewer

[rishigupta1599]

[Low] Browser-scope specs live in session.test.js

This block is here because the shared cdpTimeout/cdpTimeoutMessage exports live in session.js, which hurts discoverability for anyone looking for browser specs.

Suggestion: move the shared exports into a small cdp-timeout.js imported by both — which also decouples browser.js from Session's internals — and give the browser specs their own file.

Reviewer: stack-code-reviewer

[rishigupta1599]

Claude Code PR Review

PR: #2401 • Head: b5c16e1 • Reviewers: stack-code-reviewer

Summary

Adds a Node-side deadline to Session.send() and Browser.send() so a single CDP round trip can no longer wait forever. A renderer or browser process that is frozen, CPU-starved or swapped out keeps its target attached and simply never replies; in-page timers cannot rescue that case because they live in the very renderer that stopped running. Without a deadline the send never settles and the whole CLI blocks silently with no error and an unfinalized build. The deadline defaults to 120s, is raised to twice the largest configured in-page wait so it can never pre-empt one, and can be tuned or disabled with PERCY_CDP_TIMEOUT. Adds 11 unit specs covering both the session- and browser-scoped paths.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials in the diff.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	Env vars parsed with an explicit radix; NaN falls back to the default and negatives are clamped by Math.max(0, …).
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	Verified independently: both clearTimeout(callback.timer) read sites sit inside a #callbacks.has(data.id) guard (session.js:115, browser.js:386), so no undefined-callback dereference. The timeout handler deletes its id from the map first, so a late reply falls through the same guard and is ignored rather than double-settling. PERCY_CDP_TIMEOUT=0 cleanly disables the deadline.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Rejects with a named, actionable message plus a debug log; nothing swallowed.
High	Correctness	No race conditions or concurrency issues	Pass	Node is single-threaded, so the clearTimeout in the message handler always runs before any settle; reply-vs-timeout cannot interleave.
Medium	Testing	New code has corresponding tests	Pass	11 new specs across Unit / Session and Unit / Browser CDP deadline.
Medium	Testing	Error paths and edge cases tested	Pass	Deadline fires, names the command, points at the override, is cleared on reply, is cleared on close, is disabled at 0, and leaves the raw-message path untimed.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI green on this head: Test (including the @percy/core 100% coverage gate), Windows, Lint, Typecheck, Semgrep, Verify Executable.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data fetching.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	The static first-caller-wins memoization mirrors the pre-existing Page.TIMEOUT (page.js:75, page.js:406-408). One deviation noted below: server.js:226 .unref()s its timer and these do not.
Medium	Quality	Changes are focused (single concern)	Pass	Two source files and one new spec file, all one concern.
Low	Quality	Meaningful names, no dead code	Pass	The unreachable clearTimeout loop in restart() was removed in b5c16e1 rather than papered over with a test.
Low	Quality	Comments explain why, not what	Pass	Comments state the failure mode and why an in-page deadline cannot cover it.
Low	Quality	No unnecessary dependencies added	Pass	No dependency changes.

Findings

None of the findings below describe behaviour this PR makes worse than the status quo. Before the change, the failure mode is an unbounded silent hang; after it, every path is bounded and reported. The findings are unrealized improvements and polish.

• File: packages/core/src/discovery.js:722
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: onRetry restarts the browser only when the error message contains Browser not connected, Browser closed, Session closed or Session crashed. The new Protocol error (…): Timed out after Xms … message matches none of them, so a CDP timeout never triggers percy.browser.restart().
• Severity recorded Medium (reviewer proposed High) — the reviewer's rating rested on the claim that retries run "against the same still-frozen browser/session", i.e. that no recovery is attempted. That is not accurate: browser.page(…) is called inside the retried generator (discovery.js:640), so every retry gets a fresh page and a fresh renderer. For the frozen-renderer mechanism this PR targets — the dominant one — the retry escapes the wedged renderer on its own without needing a browser restart, which matches the observed A/B behaviour (reject at 7792ms, retry renders in 603ms). The gap is real only for a wedged browser process, where all three attempts time out and the snapshot then fails with a clear error after ~3× the deadline instead of hanging forever. That is slower recovery than it could be, not incorrect behaviour or a regression.
• Suggestion: Export an isCdpTimeoutError(error) predicate from session.js next to cdpTimeoutMessage and use it in the onRetry check, so the restart path also fires on a protocol timeout and the error-string contract is not duplicated as a fragile substring match. Worth a follow-up rather than a blocker.

---

• File: packages/core/src/session.js:26
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: cdpTimeout() resolves once per process and freezes the result on the Session class, so the first CDP send anywhere in the process wins. Two Percy instances in one process with different timeout config silently share the first one's value, and no shared test helper resets it — a latent cross-spec pollution vector (for example discovery.test.js:2137 sets PERCY_PAGE_LOAD_TIMEOUT=80000, which would feed inPageCeiling() if it were the first caller).
• Severity recorded Low (reviewer proposed Medium) — this is a pre-existing codebase pattern, not something the PR introduces: Page.TIMEOUT is memoized identically at page.js:75 and page.js:406-408. The new specs also reset Session.TIMEOUT in both beforeEach and afterEach, so the concrete risk on this PR is small. The observation is a fair codebase-wide one and the fix belongs to both statics together.
• Suggestion: Scope the resolved timeout to the Browser/Percy instance, or export a reset hook and wire it into the shared setupTest() so no future spec is exposed to stale state by accident. Treat as a follow-up covering Page.TIMEOUT as well.

---

• File: packages/core/src/session.js:20
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: PERCY_STORY_RENDER_TIMEOUT has no other producer or consumer anywhere in this monorepo — session.js:20 is its only occurrence — so a reader cannot tell from this repo alone whether it is intentional or a leftover.
• Suggestion: Confirmed intentional: it is the Storybook SDK's env var, set outside this repo, and the whole point of inPageCeiling() is to keep the Node deadline above whatever in-page wait that SDK is configured for. Name the owning package in the comment (for example "set by @percy/storybook; not read anywhere else in this repo") so the reference is self-explaining.

---

• File: packages/core/src/session.js:100
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The deadline timers are not .unref()'d (same at browser.js:308). For a programmatic consumer that finishes without closing the session or browser, an in-flight command with no reply can hold the event loop open for up to the full deadline. Moot for percy exec, which calls process.exit() explicitly.
• Suggestion: callback.timer.unref?.(), matching the existing precedent at server.js:226. A watchdog timer should never be the reason a process cannot exit.

---

• File: packages/core/test/unit/session.test.js:22
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The specs assert timing with real sleeps (a 10ms tick(), plus 150/200/250ms waits) rather than Jasmine's mock clock, adding about a second of wall-clock time and making the "has not settled" assertions probabilistic under load rather than deterministic.
• Suggestion: Use jasmine.clock() for the scheduling assertions and keep real timers only where the awaited browser.send() microtask genuinely requires them.

---

• File: packages/core/test/unit/session.test.js:113
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The Unit / Browser CDP deadline block lives in session.test.js because the shared cdpTimeout/cdpTimeoutMessage exports live in session.js, which hurts discoverability for anyone looking for browser specs.
• Suggestion: Move the shared exports into a small cdp-timeout.js imported by both, which also decouples browser.js from Session's internals, and give the browser specs their own file.

---

Verdict: PASS — the deadline is correct and well-tested, and every finding is a follow-up improvement rather than a defect introduced here.