Repository navigation
fix(cli): first render on Windows no longer fails while a new browser or ffmpeg is scanned - #5625
Merged
Merged
Conversation
… or ffmpeg is scanned
Contributor
Edit accuracy: accurate 271 (base branch 271), smooth 180 of thoseThe gate passes. Quarantined, measured but not gated (0) |
somanshreddy
approved these changes
Oct 15, 2026
somanshreddy
left a comment
somanshreddy
left a comment
Contributor
There was a problem hiding this comment.
Somu review: head ea6cf731. Clean, approving.
What I checked
- Kill failure no longer replaces the real reason. In
runCancellableProcess, both theclosehandler andfinishRejectnow ignore a rejectedterminationTask. A tree-kill failure therefore can't overwrite the timeout, abort or ENOBUFS reason.- This doesn't introduce a new hang. Both paths still settle only on
closeorerror, as before. If a kill really fails and the child stays alive, the old code hung in the same place.
- This doesn't introduce a new hang. Both paths still settle only on
- Regression test is red-first.
cancellableProcess.killFailure.test.tsmocksterminateProcessTreeto reject while the real child exits at 400 ms.- On the old code, the
closehandler routed the kill error throughfinishReject, so the message wastaskkill exited with status 128. - At this head it is
Process timed out after 50 ms/ETIMEDOUT, which is what the test asserts.
- On the old code, the
- One shared 60 s budget.
FIRST_LAUNCH_TIMEOUT_MSreplaces all six 5 s probes: Chrome--version, ffmpeg/ffprobe-version, and both-encoderspaths. The worst first launch you measured was 13.4 s, so 60 s leaves headroom, and a binary that truly hangs still fails. - CI: all required checks green, including Tests and Render on windows-latest. Mergeable.
Nits (non-blocking)
- Kill failure is now silent. When the parent has exited but taskkill still fails, any surviving grandchildren go unreported. That's fine for
--versionprobes, but adebug-level log in the.catchwould keep the signal. - Sync encoder probe can block longer.
detectH264EncoderModeusesexecFileSync, so a hung ffmpeg now blocks the event loop for up to 60 s instead of 5 s.render.tsuses the async variant, so this only affects sync callers.
— Somu
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes
On Windows, the first render after Chrome or ffmpeg is freshly installed no longer fails its startup checks. When a check does time out, the error now says it timed out.
Why
Windows antivirus scans a freshly written executable on its first launch, and the
--version/-version/-encodersprobes gave it 5 s. Measured on a Windows 11 x64 PC with Microsoft Defender (each on a fresh copy of the binary, 3 tries each):CPU load on its own is not the cause: during a 4K x265 encode at 93 to 100% CPU,
chrome-headless-shell --versionon an already-scanned binary took 42 to 55 ms across 10 runs.A real render hit this and failed with
Chrome cannot start: Failed to run "...chrome-headless-shell.exe" --version., with no reason given. That was a second bug. On timeout the runner kills the process tree withtaskkill /T /F. If the process exits before taskkill gets to it, taskkill exits with 128, and its error replaced the timeout. So the user saw neither the timeout nor its cause.How
FIRST_LAUNCH_TIMEOUT_MS = 60_000(inbrowser/ffmpeg.ts) is the one budget for the probes of these binaries: Chrome--version, ffmpeg/ffprobe-versionand both-encodersprobes. 60 s is about 4.5x the worst first launch measured. A hung binary still fails, only later.runCancellableProcess: once the child has closed, a failed tree kill no longer replaces the reason it was stopped (timeout, abort, output limit).What I measured
cancellableProcess.killFailure.test.ts(new): the tree kill fails and the run still rejects withETIMEDOUT. On the old code it rejects withtaskkill exited with status 128.What I did NOT exercise