Make the frontend-build launch timeout actually bound startup on Windows#91
Merged
Merged
Conversation
The _run_npm timeout added in #88 to stop a wedged npm from hanging launch never fired on Windows. subprocess.run(timeout=...) kills only the direct child, but npm resolves to npm.CMD, so the direct child is cmd.exe and the real node/vite build is a GRANDCHILD that inherited our stdout pipe. On timeout, run() kills cmd.exe then re-blocks in communicate() waiting for a pipe EOF the surviving node grandchild still holds - so TimeoutExpired never raised and startup hung forever with no window and no error, the exact "sits looping, never opens" symptom the timeout was meant to prevent. (Reproduced: a 3s timeout returned at 12.1s.) Switch _run_npm to Popen + communicate(timeout=...) and, on timeout, kill the WHOLE process tree (taskkill /F /T on win32, killpg on POSIX via a new session) before a bounded drain read, so a wedged grandchild can no longer hang the read. Verified against the real topology: the wedged-grandchild case now bounds in ~3s instead of blocking indefinitely. Also decode all npm subprocess output as utf-8 with errors="replace". The old text=True used the OS default (cp1252 on Windows), which mojibakes npm's UTF-8 glyphs into the error message and raises an uncaught UnicodeDecodeError on its undefined bytes (0x81/0x8d/...) - a bare traceback that aborts launch. The TimeoutExpired branch already decoded defensively; the main and non-zero-exit paths (and the node --version probe) did not. Tests: fake-Popen coverage for the success / timeout-kills-tree / drain-also-hangs / non-zero-exit / utf-8-decode paths, plus a real-subprocess test proving _terminate_process_tree kills a pipe-holding grandchild. Full suite 1692 passed; real npm happy path and real wedged-grandchild timeout both confirmed on Windows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 22, 2026
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.
Summary
The
_run_npmtimeout added in #88 (to stop a wedgednpmfrom hanging launch) never fired on Windows.subprocess.run(timeout=...)kills only the direct child, but npm resolves tonpm.CMD, so the direct child iscmd.exeand the realnode/vite build is a grandchild that inherited our stdout pipe. On timeout,run()killscmd.exethen re-blocks incommunicate()waiting for a pipe EOF the survivingnodegrandchild still holds — soTimeoutExpirednever raised and startup hung forever with no window and no error. That's the exact "sits looping, never opens" symptom the timeout was meant to prevent. Reproduced: a 3s timeout returned at 12.1s.Fix
_run_npmnow usesPopen+communicate(timeout=...), and on timeout kills the whole process tree —taskkill /F /Ton win32,os.killpgon POSIX (child launched in its own session) — followed by a bounded drain read so a wedged grandchild can't re-hang the second read either.utf-8/errors="replace". The oldtext=Trueused the OS default codec (cp1252 on Windows), which mojibakes npm's UTF-8 glyphs and raises an uncaughtUnicodeDecodeErroron undefined bytes — a bare traceback that aborts launch. TheTimeoutExpiredbranch already decoded defensively; the main path, the non-zero-exit path, and thenode --versionprobe did not.No behavior change on the happy path: same output capture, same
CREATE_NO_WINDOW, same actionableFrontendBootstrapErrormessages.Test plan
Popencoverage: success / timeout-kills-the-whole-tree / drain-also-hangs / non-zero-exit / utf-8-decode-of-real-bytes_terminate_process_treekills a pipe-holding grandchild (not just the direct child)_run_npmhappy path runs clean against real npm; the wedged-grandchild timeout now bounds in ~3s where it previously blocked indefinitelyFound by an adversarial bug-scan of the recent migration changes; first of several fixes from that scan.