From 9507f4382e912d200919ee6c511319f0a300a2c2 Mon Sep 17 00:00:00 2001 From: iso2kx <8766057+iso2kx@users.noreply.github.com> Date: Thu, 30 Jul 2026 13:32:37 +0800 Subject: [PATCH] fix(update): stop the venv-side launcher of each paused Windows gateway MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows a gateway started through the venv shim is a two-process chain: venv\Scripts\python.exe (launcher — keeps venv .pyd files mapped) └─ uv\python\...\python.exe (worker — writes the gateway PID file) `_pause_windows_gateways_for_update()` builds its pause set from `find_gateway_pids()`, which reads the PID file and therefore only ever sees the *worker*. The venv-holder guard immediately downstream (`_detect_venv_python_processes()`) matches on the venv path prefix, so it only ever sees the *launcher*. The two sets are disjoint. A gateway the updater had just gracefully drained still left its launcher alive, the guard reported that launcher as a venv holder, and the update aborted — every time. On the Desktop path this surfaces as the dead-end dialog: [updates] venv-blocked: 2 process(es) hold the install PID ... python.exe ...\venv\Scripts\python.exe -m hermes_cli.main gateway run --replace Note the reported holder is a gateway the updater believes it stopped. The Desktop path is affected because `hermes-setup.exe` runs `hermes update --yes --gateway --force`, and `--force` deliberately does NOT bypass the venv guard (that needs `--force-venv`), so the abort is correct behaviour reacting to an incomplete pause. Fix: after the graceful drain, walk one hop up from each mapped gateway PID and force-kill parents that live under the project venv. Deliberately additive, not a substitution: - The planned-stop marker and the graceful drain still target the worker (the PID that wrote the PID file), so clean shutdown is unchanged and updates don't get pushed onto the hard-kill path. - `terminate_pid(force=True)` is `taskkill /T` (tree kill), so killing a launcher that outlived its worker also reaps stragglers. - `_resume_windows_gateways_after_update()` needs no change: the mapped respawn argv is rebuilt from the profile name (`_gateway_run_args_for_profile`), never from the killed PID, and the restart watcher's `_pid_exists()` wait still terminates because the tree kill takes the whole chain down. - Only the venv-side parent is returned. Unrelated ancestors (a Scheduled Task's `cmd.exe`, an operator's shell) are ignored, and the caller's own process chain is excluded so a CLI `hermes update` never nominates itself. Tests assert the invariant the two PID-resolution paths must satisfy — the pause's kill set must cover the guard's abort set — rather than snapshotting PIDs. Verified to fail without the fix: AssertionError: pause stopped [] but the venv guard aborts on [400] — disjoint sets abort the update --- hermes_cli/main.py | 1 + hermes_cli/update_cmd.py | 78 ++++++++++- .../test_update_concurrent_quarantine.py | 132 ++++++++++++++++++ 3 files changed, 210 insertions(+), 1 deletion(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 69651b671a8eb..7ed04d5956791 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5071,6 +5071,7 @@ from hermes_cli.update_cmd import ( # noqa: F401 _upgrade_pip_before_lazy_refresh, _validate_critical_files_syntax, _venv_core_imports_healthy, + _venv_launcher_ancestors, _wait_for_windows_update_gateway_exit, _warn_incomplete_gateway_fleet_restart, _web_build_toolchain_ready, diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 6b777ff2b2bb6..cb7efc14593aa 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -2683,6 +2683,72 @@ def _format_venv_python_holders_message(matches: list[tuple[int, str, str]]) -> lines.append(" (or use `hermes update --force-venv` to proceed anyway at your own risk)") return "\n".join(lines) +def _venv_launcher_ancestors(pids: list[int]) -> list[int]: + """Return venv-interpreter ancestors of *pids* that hold the install open. + + On Windows a gateway started through the venv shim is a **two-process + chain**: ``venv\\Scripts\\python.exe`` (the launcher, which keeps native + ``.pyd`` files from the venv mapped) spawns the actual interpreter from + uv's managed CPython directory (``AppData\\Roaming\\uv\\python\\...``). + The gateway writes its PID file from the *child*, so + ``find_gateway_pids()`` — and therefore this module's pause set — only + ever sees the uv-side worker. + + ``_detect_venv_python_processes()`` matches on the venv path prefix, so + the guard downstream of the pause sees the *launcher* instead. The two + sets are disjoint, which meant a paused gateway still tripped the + venv-holder guard and aborted the update every time (the Desktop + "venv-blocked: N process(es) hold the install" dead-end, where the + reported holder is a gateway the updater believes it already stopped). + + Walking one hop up from each mapped gateway PID and keeping ancestors + that live under the project venv closes the gap. Only the venv-side + parent is returned — unrelated ancestors (the Scheduled Task's + ``cmd.exe``, an operator's shell) are ignored so we never widen the + blast radius beyond the gateway's own launcher. Never raises. + """ + if not _m()._is_windows() or not pids: + return [] + try: + import psutil + except Exception: + return [] + + venv_dir = _m().PROJECT_ROOT / "venv" + try: + venv_prefix = str(venv_dir.resolve()).lower().rstrip(os.sep) + os.sep + except OSError: + venv_prefix = str(venv_dir).lower().rstrip(os.sep) + os.sep + + # Never return ourselves or our own ancestry: a CLI ``hermes update`` + # runs from the venv python and would otherwise nominate itself. + skip: set[int] = {os.getpid()} + try: + for anc in psutil.Process().parents(): + skip.add(int(anc.pid)) + except Exception: + pass + + found: list[int] = [] + for pid in pids: + try: + parent = psutil.Process(int(pid)).parent() + except Exception: + continue + if parent is None: + continue + ppid = int(parent.pid) + if ppid in skip or ppid in found or ppid in set(pids): + continue + try: + exe = (parent.exe() or "").lower() + except Exception: + continue + if exe.startswith(venv_prefix): + found.append(ppid) + return found + + def _pause_windows_gateways_for_update() -> dict | None: """Stop running Windows gateways before mutating the checkout or venv. @@ -2783,8 +2849,18 @@ def _pause_windows_gateways_for_update() -> dict | None: logger.debug("Could not capture argv for unmapped gateway %s: %s", pid, exc) unmapped.append({"pid": int(pid), "argv": argv}) + # A gateway's graceful drain above targets the PID that wrote the PID file + # (the uv-side worker). On Windows that worker's parent is usually the + # venv-side ``python.exe`` launcher, which keeps venv ``.pyd`` files mapped + # and is what ``_detect_venv_python_processes()`` reports downstream. Left + # alive, it trips the venv-holder guard and aborts the update even though + # the gateway itself is stopped. Force-kill those launchers alongside the + # survivors; ``terminate_pid(force=True)`` is a tree kill, so a launcher + # that outlived its worker takes any stragglers with it. + launcher_pids = _m()._venv_launcher_ancestors(mapped_pids) + force_killed = [] - for pid in sorted(set(survivors).union(unmapped_pids)): + for pid in sorted(set(survivors).union(unmapped_pids).union(launcher_pids)): try: terminate_pid(int(pid), force=True) force_killed.append(int(pid)) diff --git a/tests/hermes_cli/test_update_concurrent_quarantine.py b/tests/hermes_cli/test_update_concurrent_quarantine.py index b04cb7d2c72d2..c736a445eaa33 100644 --- a/tests/hermes_cli/test_update_concurrent_quarantine.py +++ b/tests/hermes_cli/test_update_concurrent_quarantine.py @@ -270,6 +270,138 @@ def test_pause_windows_gateways_for_update_stops_profile_and_unmapped_pids( assert "Restart manually after update" not in captured +# --------------------------------------------------------------------------- +# venv-side launcher ancestors (the uv launcher/worker split) +# +# A gateway started through the venv shim is two processes: +# venv\Scripts\python.exe (launcher) -> uv\python\...\python.exe (worker) +# The gateway's PID file records the WORKER, so find_gateway_pids() (and the +# pause set built from it) only ever sees the worker. The venv-holder guard +# matches on the venv path prefix, so it only ever sees the LAUNCHER. The two +# sets were disjoint: a gateway the updater had just stopped still tripped the +# guard, aborting every update ("venv-blocked: N process(es) hold the install"). +# --------------------------------------------------------------------------- + + +def _fake_psutil_tree(tree, venv_exe, worker_exe): + """Build a psutil stand-in where ``tree`` maps worker pid -> parent pid. + + Parents whose pid is even are venv-side (``venv_exe``); odd parents are + unrelated ancestors (``worker_exe``) that must NOT be returned. + """ + + class FakeProc: + def __init__(self, pid): + self.pid = pid + if pid not in tree and pid not in tree.values(): + raise ValueError(f"no such pid {pid}") + + def parent(self): + ppid = tree.get(self.pid) + return FakeProc(ppid) if ppid else None + + def parents(self): + return [] + + def exe(self): + # Parents of workers are the launchers under test. + return venv_exe if self.pid % 2 == 0 else worker_exe + + mod = types.SimpleNamespace(Process=FakeProc) + return mod + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_venv_launcher_ancestors_returns_venv_side_parent(_winp, monkeypatch): + """The worker's venv-side parent is reported so the guard set is covered.""" + venv_exe = str(cli_main.PROJECT_ROOT / "venv" / "Scripts" / "python.exe") + worker_exe = r"C:\Users\x\AppData\Roaming\uv\python\cpython-3.11\python.exe" + + # worker 200 -> launcher 100 (even == venv-side) + fake = _fake_psutil_tree({200: 100}, venv_exe, worker_exe) + monkeypatch.setitem(sys.modules, "psutil", fake) + + assert cli_main._venv_launcher_ancestors([200]) == [100] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_venv_launcher_ancestors_ignores_non_venv_parents(_winp, monkeypatch): + """A Scheduled Task's cmd.exe / an operator shell is not a venv holder.""" + venv_exe = str(cli_main.PROJECT_ROOT / "venv" / "Scripts" / "python.exe") + worker_exe = r"C:\Windows\System32\cmd.exe" + + # worker 200 -> parent 101 (odd == NOT venv-side) + fake = _fake_psutil_tree({200: 101}, venv_exe, worker_exe) + monkeypatch.setitem(sys.modules, "psutil", fake) + + assert cli_main._venv_launcher_ancestors([200]) == [] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_venv_launcher_ancestors_is_empty_without_pids(_winp): + """No mapped gateways means nothing to walk up from.""" + assert cli_main._venv_launcher_ancestors([]) == [] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_pause_kill_set_covers_venv_guard_abort_set( + _winp, + monkeypatch, + tmp_path, +): + """INVARIANT: whatever the venv guard would abort on must be stopped. + + This is the contract the two PID-resolution paths must satisfy. Before the + launcher walk existed, ``terminated`` held only the uv-side worker while + the guard reported the venv-side launcher, so the update aborted forever + despite a "successful" pause. + """ + import hermes_cli.gateway as gateway_mod + import gateway.status as status_mod + + venv_exe = str(cli_main.PROJECT_ROOT / "venv" / "Scripts" / "python.exe") + worker_exe = r"C:\Users\x\AppData\Roaming\uv\python\cpython-3.11\python.exe" + + profile_home = tmp_path / "profiles" / "default" + profile_home.mkdir(parents=True) + # The PID file records the WORKER (even-numbered parent 400 is its launcher). + worker_pid, launcher_pid = 500, 400 + profile_proc = SimpleNamespace( + profile="default", path=profile_home, pid=worker_pid + ) + + monkeypatch.setattr(gateway_mod, "find_gateway_pids", lambda **_k: [worker_pid]) + monkeypatch.setattr( + gateway_mod, "find_profile_gateway_processes", lambda **_k: [profile_proc] + ) + monkeypatch.setattr(gateway_mod, "_get_restart_drain_timeout", lambda: 0.1) + # Graceful drain succeeds: the worker exits, leaving zero survivors. This is + # precisely the case that used to leave the launcher alive and abort. + monkeypatch.setattr( + cli_main, "_wait_for_windows_update_gateway_exit", lambda pids, *, timeout: set() + ) + + fake = _fake_psutil_tree({worker_pid: launcher_pid}, venv_exe, worker_exe) + monkeypatch.setitem(sys.modules, "psutil", fake) + + terminated = [] + monkeypatch.setattr( + status_mod, + "terminate_pid", + lambda pid, force=False: terminated.append(int(pid)), + ) + + cli_main._pause_windows_gateways_for_update() + + # What the downstream venv-holder guard would report as blocking. + guard_would_abort_on = {launcher_pid} + assert guard_would_abort_on.issubset(set(terminated)), ( + f"pause stopped {sorted(terminated)} but the venv guard aborts on " + f"{sorted(guard_would_abort_on)} — disjoint sets abort the update" + ) + + +