diff --git a/graphlink_app/graphlink_frontend_bootstrap.py b/graphlink_app/graphlink_frontend_bootstrap.py index cf2dd2e..bf73c99 100644 --- a/graphlink_app/graphlink_frontend_bootstrap.py +++ b/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: diff --git a/graphlink_app/tests/test_frontend_bootstrap.py b/graphlink_app/tests/test_frontend_bootstrap.py index c6d0a9c..ff0438d 100644 --- a/graphlink_app/tests/test_frontend_bootstrap.py +++ b/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)