Repository navigation
fix(check): a Chrome that never becomes ready ends the check with a named error - #7360
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes at fdc4b7e1. There's one blocker, and it's small.
Blocker: the Comments workflow is red. The comment ratchet fails on checkDeadline.ts:
RAISED comment share: packages/cli/src/utils/checkDeadline.ts is 1.5% (4/274), at the base 0.4% (1/247)
3 comment line(s) added. Cut them, or move the explanation into a name or a type.
Three comments were added: the doc comment on CHROME_START_MS, the second line of the class doc, and the doc comment on start. The names already carry most of that meaning, for example CHROME_START_MS and CheckChromeStartError. Cutting the three lines should clear the ratchet. If you'd rather keep one, FIRST_RUN_DOWNLOAD_MS would say the same thing as a name. The test plan didn't run this check: fallow audit and format:check don't cover the ratchet.
Checked at this head. The logic looks right:
- The 180 s window, or the idle window if that's longer, only arms while the phase is
launchand the check is waiting. start()(wired toonBrowserReadyon both open paths) switches to the normal idle clock.copiesstill runs beforelaunchand still doesn't spend the window, which the new test shows.runBrowserCheckruns insidedeadline.run, so the expiry race ends the check even when a Chrome launch or download ignores the abort.- The heartbeat now starts at the first
enter(), butannouncestill stays quiet forcopies.expireandcloseclear the heartbeat. - Tests, on a built checkout:
checkDeadline.test.ts,check.test.tsandcheckPipeline*gave 220/220 passed. - The "fails without the fix" claim holds. With
launchingforced tofalse, exactly the two named tests fail ("a Chrome that never becomes ready ends the check with a named error" and the pipeline version), and the other 218 pass. - oxfmt is clean on the changed files.
- The rest of CI is green or still running.
Not blocking: the replaced test, "browser provisioning does not spend the check budget", reverses an earlier choice on purpose: launch is now bounded at 180 s. The docs paragraph and the hyperframes browser ensure hint cover a slow first-run download, so I'm fine with it.
I'll approve once Comments is green on the new head.
There was a problem hiding this comment.
Approve at 1a36232f.
What changed since fdc4b7e1: only checkDeadline.ts, +3/−6. The three added comment lines are gone, CHROME_START_MS is renamed CHROME_START_WINDOW_MS, and the class doc is back to its one line on main. Nothing in the behavior changed.
Checks at this head:
checkDeadline.test.ts,check.test.tsandcheckPipeline*: 220/220 passed.- The "fails without the fix" claim still holds. With
launchingforced tofalse, the two named tests fail and the others pass. - oxfmt is clean.
- CI is fully green: CI, CodeQL, Comments (the ratchet now passes), Docs, preview-regression, regression, PR captures, Player perf and Windows render verification.
My notes on the logic from the last review still apply, and none of them block.
…, heygen-com#7360, heygen-com#7347); review to PR heygen-com#7362 Adopt heygen-com#7357 and heygen-com#7361 (upload completion waits and retries), heygen-com#7360 (Chrome start failure names its cause in check) and heygen-com#7347 (Safari perf tooling); release v0.9.20 not applicable. Pin open heygen-com#7362 (needs its golden LFS pointer when adopted); record issue heygen-com#7363. Monitored 126 -> 127. Co-authored-by: Miguel Ángel <[email protected]> Co-Authored-By: Claude Sonnet 5.5 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
What
hyperframes checkcould print nothing and never end when Chrome did not become ready: the check's idle clock only started once Chrome was ready, so the phases before it (the video copies, then starting Chrome) were not covered by any deadline of the check's own.Chrome did not start within 180 s while starting Chrome. Seconds per phase: ..., with a hint to runhyperframes browser ensure(a first run downloads Chrome).Why
A report:
hyperframes check --jsonon Windows (0.9.19, win32 x64) printed nothing for 200+ s and the caller waited on it with no error. 0.9.19 predates the stderr phase lines, so the caller saw no output at all. This change makes the CLI always end, with a named error, inside a bounded time whichever phase stalls from Chrome start onward. I did not reproduce the original stall: I have no Windows machine, so the cause of Chrome not becoming ready on that machine is still unknown; the new finding records how long each step took, so the next report names it.Size
One class and its tests (
checkDeadline, one pipeline test) plus one paragraph indocs/packages/cli.mdx.Test plan
vitest runofcheckDeadline.test.ts,check.test.tsandcheckPipeline*: 220 passed, 3 runs in a row (run on a built checkout).bun run format:checkexit 0;fallow audit --base origin/main --fail-on-issuesexit 0 ("No issues in 56 changed files"); oxlint exit 0; tsc: no error in the changed files.