Skip to content

fix(cli): a Chrome download that stops sending is abandoned, retried, and releases the lock - #6222

Merged
miguel-heygen merged 4 commits into
mainfrom
fix/browser-download-stall
Oct 24, 2026
Merged

miguel-heygen merged 4 commits into
mainfrom
fix/browser-download-stall

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

What changes for you

A Chrome download that stops sending data no longer hangs the command and the install lock forever. After 60 s with no bytes the download is abandoned and retried (3 attempts); if it still stalls, the command fails with The Chrome download from <host> stalled: no data for 60s. plus the existing instructions for setting HYPERFRAMES_BROWSER_PATH. The install lock is released, so the next check, render or browser ensure does not wait behind it.

Why

@puppeteer/browsers' install() has no timeout, and the install lock's heartbeat keeps it fresh for as long as its holder lives. A download whose server accepts the connection and never answers was still alive six hours later, heartbeating, and every later command waited behind it.

How

In browser/manager.ts:

  • withDownloadStallTimeout wraps each install() call. It uses the library's progress callback as the heartbeat, starts the timer before the first byte, and stops it once all bytes have arrived so a slow unzip is never counted as a stall. The host in the message comes from the library's own download URL.
  • retryOnStall runs the whole install up to 3 times on a stall, clearing the staging directory between tries. Corrupt-archive recovery is unchanged.
  • EnsureBrowserOptions.downloadStallMs overrides the 60 s default (used by the test).

install() takes no abort signal, timeout or agent (checked in the pinned 3.2.2 and the latest 3.2.6), and it keeps its sockets open, so the download runs in a child process: a new browser-install mode of the existing render-setup worker (installInChild.ts, installRun.ts). The child writes progress and unzip-log lines on stdout; the parent keeps the stall timer, forwards progress to onProgress, and on a stall aborts the child, which kills its process tree and closes its sockets. Nothing global is touched. The timer allows 15 s for the child to start, then 60 s without bytes; it stops once all bytes have arrived so a slow unzip is never a stall. EnsureBrowserOptions.downloadBaseUrl passes the library's baseUrl through (a mirror); the tests use it to point at a local listener.

Tests

New downloadStall.process.test.ts runs a real ensureBrowser in a child process against a local TCP listener:

  • accepts and never answers: the process exits with code 1, the message names 127.0.0.1:<port>, it retried (Retrying (2/2)), and no install lock is left. With the stall timer removed the process never exits and the test fails at its 20 s guard.
  • keeps sending one byte every 80 ms (longer than the 200 ms stall limit overall): not called a stall. With only the start-up line re-arming the timer this test fails.

manager.test.ts now replaces the child runner with the mocked library's install, so its existing download tests run unchanged (68 pass). The browser directory passes (195 tests); tsc, lint, format and the comment checks are clean.

Not covered: a real stalled download from the Chrome host, and the Windows path (the same child runner; not run there).

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: REQUEST CHANGES · 1 blocker · 1 important · 0 nits · head 6bd9a481

The lock is released now, but hyperframes browser ensure still never exits after a stall: the abandoned transfers keep Node's event loop alive. Details and a repro are inline.

Scope

Read manager.ts and the new tests in full, plus @puppeteer/browsers 3.2.6 install.js and httpUtil.js. The 70 manager tests pass locally. End-to-end check: ran tsx src/cli.ts browser ensure --force with a preload that sends the Chrome host's https.request to a TCP listener that accepts and never answers. As a control, the same run against a closed port exits 1 immediately. CI was still in progress at review time. No prior reviews.

— Genesis reviewer

Comment thread packages/cli/src/browser/manager.ts Outdated
Comment thread packages/cli/src/browser/manager.ts
@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 142 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: APPROVE — 2 resolved, 0 open, 0 new · 6bd9a481 → f58d3470

Re-ran the stalled-host repro: browser ensure --force now exits 1 after about 185 s with 3 attempts, the host named, and no lock or staging leftovers.

— Genesis reviewer

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verdict: APPROVE — 0 resolved, 0 open, 0 new · f58d3470 → 67152eae

Test-only change: the socket error listener now sits on every accepted connection in the fixture server. All checks are green.

— Genesis reviewer

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit fcec26e Oct 24, 2026
68 checks passed
@miguel-heygen
miguel-heygen deleted the fix/browser-download-stall branch October 24, 2026 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant