diff --git a/gateway/status.py b/gateway/status.py index 1fbbe10a1e2f..135d7e580931 100644 --- a/gateway/status.py +++ b/gateway/status.py @@ -514,6 +514,21 @@ def looks_like_gateway_runtime_command_line(command: str | None) -> bool: state. Keep the public ``looks_like_gateway_command_line()`` strict, and use this broader matcher only when validating Hermes-owned runtime records or no-supervisor cleanup scans. + + Verified launch shapes (all must keep working): + - ``python -m hermes_cli.main gateway run|restart`` (venv shim / CLI) + - ``python gateway/run.py ...`` or an absolute path ending in it + - ``hermes-gateway[.exe]`` standalone entrypoints + - ``--profile``/``-p`` selectors on either side of the subcommand + (stripped before matching) + + Deliberately NOT extended with a fallback signal (e.g. "venv exe + + ancestor argv contains gateway"): this matcher is also used to validate + that a candidate process is Hermes-owned, and widening it risks + classifying an unrelated venv process as the gateway. If a future launch + shape needs to be recognised, add it to ``_gateway_command_subcommand`` + and extend the test matrix in + ``tests/hermes_cli/test_update_venv_launcher_ancestor_gating.py``. """ return _gateway_command_subcommand(command) in {"run", "restart"} diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index c64080ab0cb8..d355303a2fb2 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -506,10 +506,17 @@ def _scan_gateway_pids( a live gateway when the PID file is stale/missing, and ``--all`` sweeps can discover gateways outside the current profile. """ - # Exclude the entire ancestor chain so the CLI process that invoked this - # scan (e.g. ``hermes gateway status``) is never mistaken for a running - # gateway. See #13242. - exclude_pids = exclude_pids | _get_ancestor_pids() + # Ancestors are suppressed so the CLI process that invoked this scan (e.g. + # ``hermes gateway status``) is never mistaken for a running gateway (see + # #13242) -- but NOT unconditionally. ``hermes update --gateway`` is + # spawned BY the gateway when ``/update`` is issued from a messaging + # platform, which puts a real ``gateway run`` in our own ancestor chain. + # Excluding it hid the gateway from the update pause machinery, so the + # update never paused it and then aborted on the venv-holder guard with + # "Other Hermes processes are running from this install's venv" (#87594). + # Held separately from ``exclude_pids`` (which the CALLER owns and means + # unconditionally) and consulted with the command line in hand below. + ancestor_pids = _get_ancestor_pids() pids: list[int] = [] # Strict command-line matcher shared with gateway.status: requires the # actual ``gateway run`` subcommand (or the dedicated entrypoints), so this @@ -558,6 +565,20 @@ def _matches_gateway_runtime(command: str) -> bool: return True return include_restart_managers and looks_like_gateway_runtime_command_line(command) + def _suppressed_as_ancestor(pid: int, command: str) -> bool: + """True when ``pid`` is our own ancestor and is not a gateway runtime. + + The #13242 exclusion exists to keep the invoking CLI (``hermes gateway + status``, ``hermes update``) from being counted as a gateway. Those + command lines are not gateway runtimes, so gating the exclusion on the + matcher preserves that intent exactly while leaving a real ``gateway + run`` parent visible -- which is what the update pause path needs when + the gateway is the process that spawned it. + """ + if pid not in ancestor_pids: + return False + return not looks_like_gateway_runtime_command_line(command) + try: if is_windows(): # Prefer wmic when present (fast, stable output format). On @@ -626,9 +647,13 @@ def _matches_gateway_runtime(command: str) -> bool: all_profiles or _matches_current_profile(current_cmd) ): try: - _append_unique_pid(pids, int(pid_str), exclude_pids) + scanned_pid = int(pid_str) except ValueError: - pass + scanned_pid = None + if scanned_pid is not None and not _suppressed_as_ancestor( + scanned_pid, current_cmd + ): + _append_unique_pid(pids, scanned_pid, exclude_pids) current_cmd = "" else: # Try /proc first (works in Docker without procps installed), @@ -647,8 +672,10 @@ def _matches_gateway_runtime(command: str) -> bool: with open(f"/proc/{pid}/cmdline", "rb") as _f: cmdline = _f.read().decode("utf-8", errors="replace") cmdline = cmdline.replace("\x00", " ") - if _matches_gateway_runtime(cmdline) and ( - all_profiles or _matches_current_profile(cmdline) + if ( + _matches_gateway_runtime(cmdline) + and (all_profiles or _matches_current_profile(cmdline)) + and not _suppressed_as_ancestor(pid, cmdline) ): _append_unique_pid(pids, pid, exclude_pids) except (OSError, PermissionError): @@ -696,8 +723,10 @@ def _matches_gateway_runtime(command: str) -> bool: if pid is None: continue - if _matches_gateway_runtime(command) and ( - all_profiles or _matches_current_profile(command) + if ( + _matches_gateway_runtime(command) + and (all_profiles or _matches_current_profile(command)) + and not _suppressed_as_ancestor(pid, command) ): _append_unique_pid(pids, pid, exclude_pids) except (OSError, subprocess.TimeoutExpired): diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 3cf74ff1b08c..02a582baa83c 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -3909,12 +3909,57 @@ def _venv_launcher_ancestors(pids: list[int]) -> list[int]: # Never return ourselves or our own ancestry: a CLI ``hermes update`` # runs from the venv python and would otherwise nominate itself. + # Exception (mirrors PR #87608 / #87594): an ancestor that is ITSELF a + # running gateway (command line matches ``gateway run``) is a real + # venv-holder, not a false positive — when the update was spawned by the + # gateway (e.g. /update from a messaging platform), the gateway launcher + # sits in this process's ancestry, and skipping it leaves the launcher + # locking venv .pyd files so the update still aborts on the venv-holder + # guard. Only skip ancestors that are NOT gateways. skip: set[int] = {os.getpid()} + # Narrow the try to the fragile pieces (import + walking the ancestry) + # instead of wrapping the whole loop: per-ancestor cmdline() errors are + # already handled by the inner try (anc_cmdline = ""), and a fallback + # that re-walks the ancestry would silently duplicate the loop. If the + # matcher cannot be imported we degrade to the previous behaviour — + # skip everything — and log why. try: - for anc in psutil.Process().parents(): - skip.add(int(anc.pid)) - except Exception: - pass + from gateway.status import looks_like_gateway_runtime_command_line + ancestors = psutil.Process().parents() + except Exception as exc: + # Fallback to the previous behaviour: skip the whole ancestry. + # Log it: without this, an update that trips the venv-holder guard + # (silently) never shows *why* — the gateway launcher got hidden by + # the very fallback this fix was meant to remove. + logger.warning("ancestor gating unavailable (%s); skipping whole ancestry", exc) + try: + for anc in psutil.Process().parents(): + skip.add(int(anc.pid)) + except Exception: + pass + ancestors = [] + + for anc in ancestors: + anc_pid = int(anc.pid) + try: + # Keep quotes when re-serializing: the matcher tokenizes via + # shlex, so a path containing spaces (e.g. "C:\Program Files\...") + # must stay quoted or it would be split and fail to match. + # NOTE: `os.name == "nt"` is unreachable here — the function + # already returned [] unless _is_windows(). The branch is + # deliberately kept so the re-serialization stays correct if + # this helper is ever lifted out of the Windows guard. + raw = anc.cmdline() or [] + anc_cmdline = ( + subprocess.list2cmdline(raw) if os.name == "nt" else " ".join(raw) + ) + except Exception: + anc_cmdline = "" + if anc_cmdline and looks_like_gateway_runtime_command_line(anc_cmdline): + # A gateway ancestor is the very process we want to pause — + # keep it out of the skip set so the launcher is found below. + continue + skip.add(anc_pid) found: list[int] = [] for pid in pids: diff --git a/tests/hermes_cli/test_gateway_proc_fallback.py b/tests/hermes_cli/test_gateway_proc_fallback.py index c2f447965022..907663699b3f 100644 --- a/tests/hermes_cli/test_gateway_proc_fallback.py +++ b/tests/hermes_cli/test_gateway_proc_fallback.py @@ -114,6 +114,51 @@ def _open(path, mode="r", **kwargs): mock_ps.assert_not_called() # /proc dir existed, so ps not called +# --------------------------------------------------------------------------- +# Windows wmic branch: ancestor suppression gated on the gateway matcher +# (#87594 / #87608). The /proc tests above are Linux-only; these run anywhere. +# (Reuses the _GATEWAY_CMD / _OTHER_CMD constants defined at module top.) +# --------------------------------------------------------------------------- + + +def _wmic_result(entries: dict): + """Return a subprocess.run()-shaped result with WMIC LIST-format output.""" + out = "" + for pid, cmd in entries.items(): + out += f"CommandLine={cmd}\r\nProcessId={pid}\r\n\r\n" + return MagicMock(returncode=0, stdout=out, stderr="") + + +class TestWindowsAncestorSuppression: + """_suppressed_as_ancestor: an ancestor that IS a gateway runtime is kept + visible (so `hermes update --gateway` can pause it) instead of being + dropped by the old blanket ancestor exclusion (#13242 changed by #87594).""" + + def _run_scan(self, monkeypatch, entries: dict, ancestor_pids: set): + monkeypatch.setattr(gateway_mod, "is_windows", lambda: True) + monkeypatch.setattr(gateway_mod, "is_macos", lambda: False) + monkeypatch.setattr( + gateway_mod, "_get_ancestor_pids", lambda: set(ancestor_pids) + ) + with ( + patch("shutil.which", return_value=r"C:\Windows\System32\wbem\wmic.exe"), + patch( + "hermes_cli._subprocess_compat.bounded_probe_run", + return_value=_wmic_result(entries), + ), + ): + return gateway_mod._scan_gateway_pids(set(), all_profiles=True) + + def test_gateway_runtime_ancestor_is_kept(self, monkeypatch): + """The gateway launcher sits in our ancestry with a ``gateway run`` + cmdline; it must still be returned (the update pause path needs it).""" + entries = { + 100: _GATEWAY_CMD, # gateway runtime AND our ancestor + 200: _GATEWAY_CMD, # gateway runtime, not an ancestor + } + pids = self._run_scan(monkeypatch, entries, ancestor_pids={100}) + assert 100 in pids, "gateway-runtime ancestor must not be suppressed" + assert 200 in pids class TestPsFallbackBsdCompat: """Verify the ps fallback command uses BSD/macOS-compatible flags. diff --git a/tests/hermes_cli/test_update_venv_launcher_ancestor_gating.py b/tests/hermes_cli/test_update_venv_launcher_ancestor_gating.py new file mode 100644 index 000000000000..4a3d19db6ba1 --- /dev/null +++ b/tests/hermes_cli/test_update_venv_launcher_ancestor_gating.py @@ -0,0 +1,276 @@ +"""Test the ancestor-gating fix in _venv_launcher_ancestors (#87666). + +The gateway is a two-process chain (venv launcher -> uv worker). When /update +is spawned BY the gateway, the launcher sits in the updater's own ancestor +chain and the old code skipped the whole chain, hiding the launcher. This +fix keeps gateway-runtime ancestors out of `skip` so the launcher is found. +""" +import sys +import types +from unittest.mock import patch + +import hermes_cli.update_cmd as cli_main +from hermes_cli import main as cli_main_module + + +def _fake_psutil_with_ancestry(proc_tree, cmdlines): + """Build a psutil stand-in. + + ``proc_tree`` maps pid -> parent pid (None for root). + ``cmdlines`` maps pid -> argv list, for the skip-set gating. + ``parents()`` on the CURRENT process returns the full ancestor chain + (excluding self), which is what _venv_launcher_ancestors uses to build + its skip set. + """ + + class FakeProc: + def __init__(self, pid=None): + # psutil.Process() with no args returns the CURRENT process. + if pid is None: + pid = cli_main.os.getpid() + self.pid = pid + + def cmdline(self): + if self.pid not in cmdlines: + raise psutil_error("no such process") + return cmdlines[self.pid] + + def parent(self): + ppid = proc_tree.get(self.pid) + if ppid is None: + return None + return FakeProc(ppid) + + def parents(self): + # current process ancestry: walk up from self + chain = [] + cur = self.pid + seen = set() + while cur in proc_tree and proc_tree[cur] is not None: + cur = proc_tree[cur] + if cur in seen: + break + seen.add(cur) + chain.append(FakeProc(cur)) + return chain + + def exe(self): + # A pid is a venv launcher iff its cmdline starts with a venv + # Scripts\\python.exe (mirrors _detect_venv_python_processes). + # NOTE: the returned path is the REAL project venv, which may + # differ from the hardcoded cmdline path (e.g. "C:\\Program + # Files\\Hermes\\...") — that is deliberate: exe() only feeds the + # startswith(venv_prefix) check, while the cmdline separately + # exercises the quoting path independent of CI's checkout dir. + raw = cmdlines.get(self.pid) + if raw and str(raw[0]).lower().endswith(r"venv\scripts\python.exe"): + return str((cli_main._m().PROJECT_ROOT / "venv" / "Scripts" / "python.exe")).lower() + return r"C:\Users\x\uv\python.exe" + + class psutil_error(Exception): + pass + + mod = types.SimpleNamespace(Process=FakeProc, NoSuchProcess=psutil_error) + return mod + + +# The updater's ancestry: updater(300) -> worker(200, uv) -> launcher(100, venv) -> wscript(1) +# The launcher (100) is the venv holder we must find. It's in the updater's +# ancestor chain, so the OLD code skipped it (bug); the fix keeps it visible. +ANCESTRY = {300: 200, 200: 100, 100: 1, 1: None} + +GATEWAY_WORKER_CMD = [ + r"C:\Users\x\AppData\Roaming\uv\python\cpython-3.11\python.exe", + "-m", "hermes_cli.main", "gateway", "run", "--replace", +] +GATEWAY_LAUNCHER_CMD = [ + r"C:\hermes\venv\Scripts\python.exe", + "-m", "hermes_cli.main", "gateway", "run", +] +WSCRIPT_CMD = [r"C:\Windows\System32\wscript.exe", "Hermes_Gateway.vbs"] + + +@patch.object(cli_main_module, "_is_windows", return_value=True) +def test_gateway_launcher_in_updater_ancestry_is_found(_winp, monkeypatch): + """The gateway launcher in the updater's own ancestry must be returned.""" + cmdlines = {300: WSCRIPT_CMD, 200: GATEWAY_WORKER_CMD, 100: GATEWAY_LAUNCHER_CMD, 1: []} + fake = _fake_psutil_with_ancestry(ANCESTRY, cmdlines) + monkeypatch.setitem(sys.modules, "psutil", fake) + + # updater = pid 300 (the process whose parents() we simulate) + monkeypatch.setattr(cli_main.os, "getpid", lambda: 300) + + found = cli_main._venv_launcher_ancestors([200]) # worker + # Not just "100 in found": the fix must return exactly the launcher and + # nothing else — a stray ancestor (e.g. the wscript) would trip the + # venv-holder guard downstream. + assert found == [100], f"launcher 100 should be found, got {found}" + + +@patch.object(cli_main_module, "_is_windows", return_value=True) +def test_non_gateway_ancestors_still_skipped(_winp, monkeypatch): + """A non-gateway ancestor (e.g. plain shell) must still be skipped.""" + # updater(300) -> shell(250) -> wscript(1), and the worker also hangs off + # the shell so ppid 250 actually reaches the skip check. + # 250's cmdline starts with the venv python (so exe() reports it under + # the venv prefix — it *would* be returned if gating dropped it) but is + # NOT a gateway runtime, so it must be skipped. + tree = {300: 250, 250: 1, 200: 250, 1: None} + cmdlines = { + 300: [ + r"C:\Program Files\Hermes\venv\Scripts\python.exe", + "-m", "hermes_cli.main", "update", + ], + 250: [ + r"C:\Program Files\Hermes\venv\Scripts\python.exe", + "-m", "hermes_cli.main", "update", "--gateway", + ], + 200: GATEWAY_WORKER_CMD, + 1: [], + } + fake = _fake_psutil_with_ancestry(tree, cmdlines) + monkeypatch.setitem(sys.modules, "psutil", fake) + monkeypatch.setattr(cli_main.os, "getpid", lambda: 300) + + found = cli_main._venv_launcher_ancestors([200]) + assert found == [], "a non-gateway venv ancestor must still be skipped" + + +@patch.object(cli_main_module, "_is_windows", return_value=True) +def test_gateway_launcher_with_spaces_in_path_still_found(_winp, monkeypatch): + """list2cmdline quoting: paths with spaces must not break matching.""" + cmdlines = { + 300: WSCRIPT_CMD, + 200: [ + r"C:\Users\x\AppData\Roaming\uv\python\cpython-3.11\python.exe", + "-m", "hermes_cli.main", "gateway", "run", "--replace", + ], + 100: [ + # Hardcoded: a path with spaces regardless of where CI checks + # out the repo (deriving from PROJECT_ROOT would make this test + # a byte-identical copy of the first one on space-free hosts). + r"C:\Program Files\Hermes\venv\Scripts\python.exe", + "-m", "hermes_cli.main", "gateway", "run", + ], + 1: [], + } + fake = _fake_psutil_with_ancestry(ANCESTRY, cmdlines) + monkeypatch.setitem(sys.modules, "psutil", fake) + monkeypatch.setattr(cli_main.os, "getpid", lambda: 300) + + # Pin os.name to "nt": the serialization branch is `list2cmdline` only on + # Windows, and this test must exercise it even on POSIX CI hosts. + monkeypatch.setattr(cli_main.os, "name", "nt") + + # Spy on list2cmdline so a failure says which half broke: the quoting + # (serialization) or the downstream matcher. + serialized: list[str] = [] + orig_list2cmdline = cli_main.subprocess.list2cmdline + + def _spy(raw): + text = orig_list2cmdline(raw) + if "Program Files" in text: + serialized.append(text) + return text + + monkeypatch.setattr(cli_main.subprocess, "list2cmdline", _spy) + + found = cli_main._venv_launcher_ancestors([200]) + assert found == [100], f"launcher 100 (space in path) should be found, got {found}" + assert serialized, "list2cmdline was never called with the spaced launcher path" + # any() rather than serialized[0]: don't depend on ancestor walk order. + assert any( + '"C:\\Program Files\\Hermes\\venv\\Scripts\\python.exe"' in text + for text in serialized + ), f"spaced launcher path should stay quoted, got: {serialized}" + + +@patch.object(cli_main_module, "_is_windows", return_value=True) +def test_gateway_restart_launcher_in_updater_ancestry_is_found(_winp, monkeypatch): + """A launcher whose argv reads ``gateway restart`` is still a venv holder. + + On hosts without a service manager the restart fallback runs + ``run_gateway()`` in-process while argv still reads ``gateway restart``, + so the matcher must treat it as a runtime ancestor too (PR #87666 keeps + such ancestors out of the skip set). + """ + cmdlines = { + 300: WSCRIPT_CMD, + 200: [ + r"C:\Users\x\AppData\Roaming\uv\python\cpython-3.11\python.exe", + "-m", "hermes_cli.main", "gateway", "restart", + ], + 100: [ + r"C:\Program Files\Hermes\venv\Scripts\python.exe", + "-m", "hermes_cli.main", "gateway", "restart", + ], + 1: [], + } + fake = _fake_psutil_with_ancestry(ANCESTRY, cmdlines) + monkeypatch.setitem(sys.modules, "psutil", fake) + monkeypatch.setattr(cli_main.os, "getpid", lambda: 300) + + found = cli_main._venv_launcher_ancestors([200]) + # ``restart`` is a management subcommand on service-managed hosts, but the + # no-supervisor fallback makes it a runtime — so it is found, not skipped. + assert found == [100], f"restart launcher 100 should be found, got {found}" + + +@patch.object(cli_main_module, "_is_windows", return_value=True) +def test_matcher_import_failure_falls_back_to_skip_all(_winp, monkeypatch, caplog): + """If the gateway-status matcher cannot be imported, fall back to the old + skip-everything behaviour and log why (never crash, never silently hang + the update on the venv-holder guard).""" + # Full ancestry WITH a venv launcher (100): without the fallback walk the + # launcher would be found, so this asserts the fallback really skips. + cmdlines = { + 300: WSCRIPT_CMD, + 200: GATEWAY_WORKER_CMD, + 100: GATEWAY_LAUNCHER_CMD, + 1: [], + } + fake = _fake_psutil_with_ancestry(ANCESTRY, cmdlines) + monkeypatch.setitem(sys.modules, "psutil", fake) + monkeypatch.setattr(cli_main.os, "getpid", lambda: 300) + + # Replace gateway.status with a module that lacks the matcher, so the + # `from gateway.status import looks_like_gateway_runtime_command_line` + # inside _venv_launcher_ancestors raises ImportError. + import types as _types + + monkeypatch.setitem( + sys.modules, + "gateway.status", + _types.SimpleNamespace(__name__="gateway.status"), + ) + + with caplog.at_level("DEBUG", logger="hermes_cli.update_cmd"): + found = cli_main._venv_launcher_ancestors([200]) + assert found == [], "fallback skips everything, so no launcher is found" + assert any( + "ancestor gating unavailable" in r.message for r in caplog.records + ), "fallback must be logged so the silent dead-end is debuggable" + + +@patch.object(cli_main_module, "_is_windows", return_value=True) +def test_ancestry_walk_failure_does_not_crash(_winp, monkeypatch): + """If psutil.Process().parents() raises, the function must not propagate: + degrade to an empty ancestry and still return a sane (empty) result.""" + class _ExplodingProcess: + def __init__(self, pid=None): + pass + + def parents(self): + raise RuntimeError("simulated psutil walk failure") + + def parent(self): + raise RuntimeError("simulated psutil walk failure") + + fake = types.SimpleNamespace( + Process=_ExplodingProcess, NoSuchProcess=RuntimeError + ) + monkeypatch.setitem(sys.modules, "psutil", fake) + monkeypatch.setattr(cli_main.os, "getpid", lambda: 300) + + found = cli_main._venv_launcher_ancestors([200]) + assert found == [], "walk failure must degrade to no matches, not crash"