Repository navigation
fix(cli): a Chrome download that stops sending is abandoned, retried, and releases the lock - #6222
Conversation
… and releases the lock
There was a problem hiding this comment.
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
Edit accuracy: accurate 271 (base branch 271), smooth 142 of thoseThe gate passes. Quarantined, measured but not gated (0) |
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 settingHYPERFRAMES_BROWSER_PATH. The install lock is released, so the nextcheck,renderorbrowser ensuredoes 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:withDownloadStallTimeoutwraps eachinstall()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.retryOnStallruns the whole install up to 3 times on a stall, clearing the staging directory between tries. Corrupt-archive recovery is unchanged.EnsureBrowserOptions.downloadStallMsoverrides 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 newbrowser-installmode 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 toonProgress, 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.downloadBaseUrlpasses the library'sbaseUrlthrough (a mirror); the tests use it to point at a local listener.Tests
New
downloadStall.process.test.tsruns a realensureBrowserin a child process against a local TCP listener: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.manager.test.tsnow replaces the child runner with the mocked library'sinstall, 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).