Make the frontend-build launch timeout actually bound startup on Windows

graphlink_app/graphlink_frontend_bootstrap.py

@@ -19,6 +19,7 @@
 
 import logging
 import os
+import signal
 import subprocess
 import shutil
 import sys
@@ -282,6 +283,7 @@ def _require_node_and_npm() -> tuple[str, str]:
     try:
         version_output = subprocess.run(
             [node_path, "--version"], capture_output=True, text=True, check=True,
+            encoding="utf-8", errors="replace",
             timeout=30, **_subprocess_no_window_kwargs(),
         ).stdout.strip()
     except (subprocess.CalledProcessError, subprocess.TimeoutExpired, OSError) as exc:
@@ -308,6 +310,50 @@ def _require_node_and_npm() -> tuple[str, str]:
     return node_path, npm_path
 
 
+def _terminate_process_tree(process: subprocess.Popen) -> None:
+    """Kill `process` AND every descendant it spawned.
+
+    `subprocess`'s own kill (what `run(timeout=...)` does internally) signals
+    only the *direct* child. Here that child is `cmd.exe` (npm ships as
+    `npm.CMD`) and the real `node`/vite build is a grandchild that inherited
+    our stdout pipe - so killing just `cmd.exe` leaves `node` running and
+    holding the pipe open, and the post-kill read then blocks forever waiting
+    for an EOF that never comes. That was the whole defect: the timeout never
+    actually bounded launch on Windows. Killing the tree closes the pipe and
+    lets the drain read finish.
+    """
+    if process.poll() is not None:
+        return
+    if sys.platform == "win32":
+        # taskkill /T walks the child tree by parent-PID; /F forces it. Always
+        # present on Windows. CREATE_NO_WINDOW so this cleanup does not itself
+        # flash a console window in a windowed (pythonw) launch.
+        try:
+            subprocess.run(
+                ["taskkill", "/F", "/T", "/PID", str(process.pid)],
+                capture_output=True, timeout=10,
+                creationflags=subprocess.CREATE_NO_WINDOW,
+            )
+            return
+        except (OSError, subprocess.SubprocessError):
+            pass  # fall through to the direct-child best effort below
+    else:
+        # _run_npm starts the child in its own session (start_new_session),
+        # so the whole build tree shares one process group we can signal at
+        # once. getpgid can race the process exiting - treat that as done.
+        try:
+            os.killpg(os.getpgid(process.pid), signal.SIGKILL)
+            return
+        except (OSError, ProcessLookupError):
+            pass
+    # Last resort if the tree-kill was unavailable: at least signal the direct
+    # child. Better than nothing, though a grandchild may still linger.
+    try:
+        process.kill()
+    except OSError:
+        pass
+
+
 def _run_npm(
     npm_path: str,
     args: list[str],
@@ -319,35 +365,53 @@ def _run_npm(
     if extra_env:
         env.update(extra_env)
     command = " ".join(["npm", *args])
-    try:
-        subprocess.run(
-            [npm_path, *args], cwd=WEB_UI_DIR, env=env, check=True,
-            capture_output=True, text=True, timeout=timeout_seconds,
-            **_subprocess_no_window_kwargs(),
-        )
-    except subprocess.TimeoutExpired as exc:
-        # Without this, a wedged npm blocked startup forever with no window
-        # and no error. Decode captured output defensively - TimeoutExpired
-        # can carry bytes or None even in text mode.
-        def _as_text(stream) -> str:
-            if stream is None:
-                return ""
-            return stream.decode("utf-8", errors="replace") if isinstance(stream, bytes) else str(stream)
 
+    popen_kwargs = dict(
+        cwd=WEB_UI_DIR, env=env,
+        stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+        # Decode explicitly as UTF-8: the OS default on Windows is cp1252,
+        # which mojibakes npm's UTF-8 output and can raise an uncaught
+        # UnicodeDecodeError on its undefined bytes - aborting launch with a
+        # bare traceback. errors="replace" keeps the message readable no
+        # matter what npm emits.
+        text=True, encoding="utf-8", errors="replace",
+        **_subprocess_no_window_kwargs(),
+    )
+    if sys.platform != "win32":
+        # Own session/process group so _terminate_process_tree can take down
+        # the whole build tree at once via killpg. On Windows taskkill /T
+        # walks the PID tree directly, so no group is needed there.
+        popen_kwargs["start_new_session"] = True
+
+    process = subprocess.Popen([npm_path, *args], **popen_kwargs)
+    try:
+        stdout, stderr = process.communicate(timeout=timeout_seconds)
+    except subprocess.TimeoutExpired:
+        # This used to be subprocess.run's job, but its timeout kills only the
+        # direct child (cmd.exe) and then re-blocks on the pipe the surviving
+        # node grandchild still holds - so the timeout never fired and launch
+        # hung forever with no window and no error. Kill the entire tree, then
+        # drain with a hard cap so a wedged grandchild can't re-hang the read.
+        _terminate_process_tree(process)
+        try:
+            stdout, stderr = process.communicate(timeout=10)
+        except subprocess.TimeoutExpired:
+            stdout, stderr = "", ""
         raise FrontendBootstrapError(
             f"`{command}` did not finish within {timeout_seconds}s while building "
             f"Graphlink's frontend in {WEB_UI_DIR} - npm appears hung.\n\n"
             f"Try running the command manually in web_ui/ (deleting node_modules/ "
             f"and re-running often clears a wedged install), or set "
             f"{DEV_MODE_ENV_VAR}=1 to skip build orchestration if you manage the "
-            f"frontend build yourself.\n\n--- partial stdout ---\n{_as_text(exc.stdout)}"
-            f"\n--- partial stderr ---\n{_as_text(exc.stderr)}"
-        ) from exc
-    except subprocess.CalledProcessError as exc:
+            f"frontend build yourself.\n\n--- partial stdout ---\n{stdout or ''}"
+            f"\n--- partial stderr ---\n{stderr or ''}"
+        )
+
+    if process.returncode != 0:
         raise FrontendBootstrapError(
-            f"`{command}` failed (exit code {exc.returncode}) while building Graphlink's "
-            f"frontend in {WEB_UI_DIR}.\n\n--- stdout ---\n{exc.stdout}\n--- stderr ---\n{exc.stderr}"
-        ) from exc
+            f"`{command}` failed (exit code {process.returncode}) while building Graphlink's "
+            f"frontend in {WEB_UI_DIR}.\n\n--- stdout ---\n{stdout or ''}\n--- stderr ---\n{stderr or ''}"
+        )
 
 
 def ensure_frontend_built() -> None:

graphlink_app/tests/test_frontend_bootstrap.py

@@ -374,14 +374,13 @@ def test_fresh_workspace_never_calls_npm_at_all(self, workspace, monkeypatch):
         assert called["run_npm"] is False
 
     def test_npm_failure_raises_an_actionable_error_with_command_output(self, monkeypatch):
-        monkeypatch.setattr(
-            gfb.subprocess, "run",
-            lambda *a, **k: (_ for _ in ()).throw(
-                subprocess.CalledProcessError(1, ["npm", "run", "build"], output="stdout text", stderr="stderr text")
-            ),
-        )
-        with pytest.raises(gfb.FrontendBootstrapError, match="npm run build"):
+        fake = _FakePopen(returncode=1, stdout="stdout text", stderr="stderr text")
+        monkeypatch.setattr(gfb.subprocess, "Popen", lambda *a, **k: fake)
+        with pytest.raises(gfb.FrontendBootstrapError, match="npm run build") as exc_info:
             gfb._run_npm("npm", ["run", "build"])
+        message = str(exc_info.value)
+        assert "exit code 1" in message
+        assert "stdout text" in message and "stderr text" in message
 
 
 class TestGraphlinkAppBootstrapFailureWiring:
@@ -426,3 +425,139 @@ def test_logs_shows_dialog_marks_clean_exit_and_exits_with_code_1(self, graphlin
         )
         assert "Node.js was not found" in str(calls[0][1])
         assert "Node.js was not found" in calls[1][1][2]  # QMessageBox.critical(parent, title, text)
+
+
+class _FakePopen:
+    """Deterministic subprocess.Popen stand-in for _run_npm control-flow tests.
+
+    Real Popen sets .returncode after communicate(); this mirrors that and can
+    be told to raise TimeoutExpired on the first and/or second communicate() so
+    the timeout -> tree-kill -> drain path is exercised without a real
+    subprocess.
+    """
+
+    def __init__(self, *, returncode=0, stdout="", stderr="",
+                 timeout_first=False, timeout_second=False):
+        self.pid = 424242
+        self.returncode = returncode
+        self._stdout = stdout
+        self._stderr = stderr
+        self._timeout_first = timeout_first
+        self._timeout_second = timeout_second
+        self._calls = 0
+
+    def communicate(self, timeout=None):
+        self._calls += 1
+        if self._calls == 1 and self._timeout_first:
+            raise subprocess.TimeoutExpired(cmd="npm", timeout=timeout)
+        if self._calls == 2 and self._timeout_second:
+            raise subprocess.TimeoutExpired(cmd="npm", timeout=timeout)
+        return self._stdout, self._stderr
+
+    def poll(self):
+        return self.returncode
+
+
+class TestRunNpm:
+    """_run_npm's timeout is the fix for the launch hang: subprocess.run's own
+    timeout killed only the direct child (cmd.exe, since npm is npm.CMD) and
+    then re-blocked on the pipe the surviving node grandchild still held, so it
+    never fired. These pin the corrected control flow."""
+
+    def test_success_returns_none(self, monkeypatch, tmp_path):
+        monkeypatch.setattr(gfb, "WEB_UI_DIR", tmp_path)
+        monkeypatch.setattr(gfb.subprocess, "Popen", lambda *a, **k: _FakePopen(returncode=0))
+        assert gfb._run_npm("npm", ["run", "build"]) is None
+
+    def test_timeout_kills_the_whole_tree_and_raises_instead_of_hanging(self, monkeypatch, tmp_path):
+        monkeypatch.setattr(gfb, "WEB_UI_DIR", tmp_path)
+        fake = _FakePopen(timeout_first=True, stdout="partial-out", stderr="partial-err")
+        monkeypatch.setattr(gfb.subprocess, "Popen", lambda *a, **k: fake)
+        killed = []
+        monkeypatch.setattr(gfb, "_terminate_process_tree", lambda p: killed.append(p))
+        with pytest.raises(gfb.FrontendBootstrapError, match="did not finish within 1s") as exc_info:
+            gfb._run_npm("npm", ["run", "build"], timeout_seconds=1)
+        assert killed == [fake], "the whole process tree must be killed on timeout, not just cmd.exe"
+        message = str(exc_info.value)
+        assert "partial-out" in message and "partial-err" in message
+
+    def test_timeout_still_raises_when_the_drain_read_also_hangs(self, monkeypatch, tmp_path):
+        # Belt-and-suspenders: even if the post-kill drain times out too,
+        # _run_npm must raise the actionable error, never propagate a raw
+        # TimeoutExpired or block indefinitely on the second read.
+        monkeypatch.setattr(gfb, "WEB_UI_DIR", tmp_path)
+        fake = _FakePopen(timeout_first=True, timeout_second=True)
+        monkeypatch.setattr(gfb.subprocess, "Popen", lambda *a, **k: fake)
+        monkeypatch.setattr(gfb, "_terminate_process_tree", lambda p: None)
+        with pytest.raises(gfb.FrontendBootstrapError, match="npm appears hung"):
+            gfb._run_npm("npm", ["run", "build"], timeout_seconds=1)
+
+    def test_nonzero_exit_raises_with_command_and_output(self, monkeypatch, tmp_path):
+        monkeypatch.setattr(gfb, "WEB_UI_DIR", tmp_path)
+        fake = _FakePopen(returncode=2, stdout="out-x", stderr="err-y")
+        monkeypatch.setattr(gfb.subprocess, "Popen", lambda *a, **k: fake)
+        with pytest.raises(gfb.FrontendBootstrapError, match="npm ci") as exc_info:
+            gfb._run_npm("npm", ["ci"])
+        message = str(exc_info.value)
+        assert "exit code 2" in message
+        assert "out-x" in message and "err-y" in message
+
+    def test_output_is_decoded_as_utf8_not_the_windows_default_codec(self, monkeypatch, tmp_path):
+        # text=True with no encoding decodes as cp1252 on Windows, which raises
+        # UnicodeDecodeError on its undefined bytes (0x81/0x8d/...) - a bare
+        # traceback that aborts launch. A real subprocess emits exactly those
+        # bytes; _run_npm must decode them (replacement chars) and raise the
+        # actionable error rather than crash on the decode.
+        monkeypatch.setattr(gfb, "WEB_UI_DIR", tmp_path)
+        script = (
+            "import sys; "
+            "sys.stdout.buffer.write(b'\\x81\\x8d cafe \\xe2\\x9c\\x93'); "
+            "sys.stdout.flush(); sys.exit(3)"
+        )
+        with pytest.raises(gfb.FrontendBootstrapError, match="exit code 3"):
+            gfb._run_npm(sys.executable, ["-c", script])
+
+
+class TestTerminateProcessTree:
+    def test_kills_grandchildren_not_just_the_direct_child(self, tmp_path):
+        # The crux of the launch-hang fix: killing the direct child is not
+        # enough - the grandchild (real npm's node process) must die too, or it
+        # keeps the stdout pipe open and the drain read blocks forever. A parent
+        # spawns a grandchild that writes a "finished" marker after a short
+        # sleep; a correct tree-kill prevents that marker from ever appearing.
+        started = tmp_path / "gc_started"
+        finished = tmp_path / "gc_finished"
+        grandchild = (
+            "import sys, time, pathlib; "
+            "pathlib.Path(sys.argv[1]).write_text('x'); "
+            "time.sleep(3); "
+            "pathlib.Path(sys.argv[2]).write_text('x')"
+        )
+        parent = (
+            "import subprocess, sys, time; "
+            "subprocess.Popen([sys.executable, '-c', sys.argv[1], sys.argv[2], sys.argv[3]]); "
+            "time.sleep(30)"
+        )
+        popen_kwargs = {}
+        if sys.platform != "win32":
+            # Mirror _run_npm: own session so killpg reaches the whole tree.
+            popen_kwargs["start_new_session"] = True
+        proc = subprocess.Popen(
+            [sys.executable, "-c", parent, grandchild, str(started), str(finished)],
+            **popen_kwargs,
+        )
+        try:
+            deadline = time.time() + 10
+            while not started.exists() and time.time() < deadline:
+                time.sleep(0.05)
+            assert started.exists(), "grandchild never started; test setup failed"
+
+            gfb._terminate_process_tree(proc)
+            proc.wait(timeout=10)
+
+            # Well past when a surviving grandchild would have written its
+            # marker (it sleeps 3s; we killed it within ~0.2s of its start).
+            time.sleep(5)
+            assert not finished.exists(), "grandchild survived the tree-kill (its pipe would hang launch)"
+        finally:
+            gfb._terminate_process_tree(proc)