diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 1a14a1e0fe975..831514cd25237 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -7253,6 +7253,60 @@ def _hermes_exe_shims(scripts_dir: Path) -> list[Path]: ] +def _walk_shim_ancestor_pids(psutil_mod, shim_paths: set[str]) -> set[int]: + """Collect PIDs of ancestor processes whose ``exe`` matches a shim path. + + On Windows the ``hermes`` console-script is a distlib-generated + ``Scripts\\hermes.exe`` launcher that spawns a child ``python.exe`` and + waits for it. Detection runs inside that child, where ``os.getpid()`` + returns the *child* PID — so the launcher's PID is left dangling in + ``psutil.process_iter`` and gets reported as "another hermes.exe" even + though it's the very shim that started this invocation (issue #29341). + + Walk the ancestor chain and treat every consecutive ancestor whose + executable resolves to one of our shim paths as part of the same + logical invocation. Stop at the first non-shim ancestor so a legitimate + second ``hermes`` somewhere in the tree (e.g. a Hermes Desktop child + sitting *under* a non-Hermes parent) is still flagged. + + The walk is bounded (max 16 hops) so a misbehaving ``parent()`` chain + can't loop us forever. Every psutil/OS error is swallowed: this is a + best-effort assist for the existing ``os.getpid()`` exclusion. + """ + extra: set[int] = set() + if not shim_paths: + return extra + try: + proc = psutil_mod.Process() + except Exception: + return extra + for _ in range(16): + try: + parent = proc.parent() + except Exception: + break + if parent is None: + break + try: + parent_exe = parent.exe() + except Exception: + break + if not parent_exe: + break + try: + parent_exe_norm = str(Path(parent_exe).resolve()).lower() + except (OSError, ValueError): + parent_exe_norm = str(parent_exe).lower() + if parent_exe_norm not in shim_paths: + break + try: + extra.add(int(parent.pid)) + except Exception: + break + proc = parent + return extra + + def _detect_concurrent_hermes_instances( scripts_dir: Path, *, exclude_pid: int | None = None ) -> list[tuple[int, str]]: @@ -7267,8 +7321,9 @@ def _detect_concurrent_hermes_instances( This helper enumerates processes whose ``exe`` matches one of the venv's shims (``hermes.exe`` / ``hermes-gateway.exe``) and returns ``(pid, - process_name)`` pairs. The caller's own PID is excluded so the running - ``hermes update`` invocation never reports itself. + process_name)`` pairs. The caller's own PID is excluded, and so is the + chain of immediate ancestor shims that spawned the current Python + process (the distlib console-script launcher — see issue #29341). Returns an empty list off-Windows, on missing psutil, or when no other instances exist. Never raises — process enumeration is best-effort. @@ -7281,8 +7336,8 @@ def _detect_concurrent_hermes_instances( except Exception: return [] - if exclude_pid is None: - exclude_pid = os.getpid() + seed_pid = exclude_pid if exclude_pid is not None else os.getpid() + exclude_pids: set[int] = {seed_pid} # Resolve every shim path to its canonical form once for cheap comparison. shim_paths: set[str] = set() @@ -7294,6 +7349,12 @@ def _detect_concurrent_hermes_instances( if not shim_paths: return [] + # Exclude the launcher-shim ancestor chain so the very ``hermes.exe`` + # that started this invocation isn't reported as a concurrent peer + # (#29341). Without this, ``hermes update`` would always require + # ``--force`` on Windows even when no other Hermes process is alive. + exclude_pids.update(_walk_shim_ancestor_pids(psutil, shim_paths)) + matches: list[tuple[int, str]] = [] try: proc_iter = psutil.process_iter(["pid", "exe", "name"]) @@ -7307,7 +7368,7 @@ def _detect_concurrent_hermes_instances( continue pid = info.get("pid") exe = info.get("exe") - if not exe or pid is None or pid == exclude_pid: + if not exe or pid is None or pid in exclude_pids: continue try: exe_norm = str(Path(exe).resolve()).lower() diff --git a/tests/hermes_cli/test_update_concurrent_quarantine.py b/tests/hermes_cli/test_update_concurrent_quarantine.py index dbf1f3ee5f8e9..276ea16d68e71 100644 --- a/tests/hermes_cli/test_update_concurrent_quarantine.py +++ b/tests/hermes_cli/test_update_concurrent_quarantine.py @@ -118,6 +118,247 @@ def test_detect_concurrent_is_noop_off_windows(_winp, tmp_path): assert cli_main._detect_concurrent_hermes_instances(tmp_path) == [] +# --------------------------------------------------------------------------- +# Launcher-shim ancestor exclusion (issue #29341) +# --------------------------------------------------------------------------- +# +# On Windows ``hermes`` is a distlib-generated ``Scripts\\hermes.exe`` console +# launcher that spawns a child ``python.exe`` and stays alive waiting on it. +# Detection runs inside the *child* (where ``os.getpid()`` lives), so the +# launcher PID is left in ``process_iter`` and gets reported as "another +# hermes.exe is running". The fix walks the parent chain and excludes every +# consecutive ancestor whose ``exe`` resolves to one of the shim paths. + + +def _fake_psutil_with_chain( + procs, *, parent_chain, my_pid: int | None = None, +): + """Build a psutil stand-in with both ``process_iter`` and ancestor walk. + + ``parent_chain`` is the list of ``(pid, exe)`` returned by + ``Process().parent()...parent()`` starting from the current process. + The first element is the immediate parent; an empty list means the + current process has no parent (terminates the walk). + """ + if my_pid is None: + my_pid = os.getpid() + + class _FakeProc: + def __init__(self, pid, exe): + self.pid = pid + self._exe = exe + self._idx = -1 # index into parent_chain + + def exe(self): + return self._exe + + def parent(self): + nxt = self._idx + 1 + if nxt >= len(parent_chain): + return None + pid, exe = parent_chain[nxt] + p = _FakeProc(pid, exe) + p._idx = nxt + return p + + def _Process(pid=None): + return _FakeProc(my_pid, None) + + return types.SimpleNamespace( + process_iter=lambda attrs: iter(procs), + Process=_Process, + Error=Exception, + NoSuchProcess=Exception, + AccessDenied=Exception, + ) + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_detect_concurrent_excludes_launcher_shim_parent(_winp, tmp_path): + """The distlib ``hermes.exe`` launcher (parent of this Python) is excluded. + + Reproduces the exact scenario from #29341: PowerShell → hermes.exe (PID + 18608) → python.exe (current). Without the parent-chain walk, 18608 + would be flagged as a concurrent peer every time. + """ + scripts_dir = tmp_path + shim = scripts_dir / "hermes.exe" + shim.write_bytes(b"") + launcher_pid = os.getpid() + 1 + procs = [ + _make_proc(launcher_pid, str(shim), "hermes.exe"), + ] + fake = _fake_psutil_with_chain( + procs, + parent_chain=[(launcher_pid, str(shim))], + ) + with patch.dict(sys.modules, {"psutil": fake}): + result = cli_main._detect_concurrent_hermes_instances(scripts_dir) + + assert result == [] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_detect_concurrent_still_flags_unrelated_hermes_peer(_winp, tmp_path): + """Excluding the launcher ancestor must NOT mask genuinely concurrent peers. + + A second ``hermes.exe`` somewhere else in the process table (e.g. Hermes + Desktop's backend child, or a second terminal) is a sibling, not an + ancestor — the walk must not exclude it. + """ + scripts_dir = tmp_path + shim = scripts_dir / "hermes.exe" + shim.write_bytes(b"") + launcher_pid = os.getpid() + 1 + real_peer_pid = os.getpid() + 2 # NOT in the parent chain + procs = [ + _make_proc(launcher_pid, str(shim), "hermes.exe"), + _make_proc(real_peer_pid, str(shim), "hermes.exe"), + ] + fake = _fake_psutil_with_chain( + procs, + parent_chain=[(launcher_pid, str(shim))], + ) + with patch.dict(sys.modules, {"psutil": fake}): + result = cli_main._detect_concurrent_hermes_instances(scripts_dir) + + assert result == [(real_peer_pid, "hermes.exe")] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_detect_concurrent_walks_multiple_shim_ancestors(_winp, tmp_path): + """Walk keeps climbing while ancestors are shims, stops at first non-shim. + + Some installer layouts double-wrap: ``hermes.exe`` (outer) → + ``hermes-gateway.exe`` (inner, for some niche commands) → python. + Both should be excluded; the shell ancestor above them must not be + walked through (and isn't a shim anyway). + """ + scripts_dir = tmp_path + outer = scripts_dir / "hermes.exe" + inner = scripts_dir / "hermes-gateway.exe" + outer.write_bytes(b"") + inner.write_bytes(b"") + outer_pid = os.getpid() + 10 + inner_pid = os.getpid() + 11 + shell_pid = os.getpid() + 12 + procs = [ + _make_proc(outer_pid, str(outer), "hermes.exe"), + _make_proc(inner_pid, str(inner), "hermes-gateway.exe"), + _make_proc(shell_pid, r"C:\\Windows\\System32\\cmd.exe", "cmd.exe"), + ] + fake = _fake_psutil_with_chain( + procs, + parent_chain=[ + (inner_pid, str(inner)), + (outer_pid, str(outer)), + (shell_pid, r"C:\\Windows\\System32\\cmd.exe"), + ], + ) + with patch.dict(sys.modules, {"psutil": fake}): + result = cli_main._detect_concurrent_hermes_instances(scripts_dir) + + assert result == [] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_detect_concurrent_walk_stops_at_first_non_shim_ancestor(_winp, tmp_path): + """A non-shim ancestor terminates the walk so deeper shims stay visible. + + Concocted but important contract: if the immediate parent is a shell + and there happens to be a ``hermes.exe`` further up the tree (e.g. a + user wrapper script), we do NOT skip past the shell to exclude it. + Anything beyond the first non-shim ancestor is treated as foreign. + """ + scripts_dir = tmp_path + shim = scripts_dir / "hermes.exe" + shim.write_bytes(b"") + shell_pid = os.getpid() + 20 + far_hermes_pid = os.getpid() + 21 + procs = [ + _make_proc(far_hermes_pid, str(shim), "hermes.exe"), + ] + fake = _fake_psutil_with_chain( + procs, + parent_chain=[ + (shell_pid, r"C:\\Windows\\System32\\cmd.exe"), + (far_hermes_pid, str(shim)), + ], + ) + with patch.dict(sys.modules, {"psutil": fake}): + result = cli_main._detect_concurrent_hermes_instances(scripts_dir) + + # The hermes.exe above the cmd.exe ancestor is NOT excluded — it's a + # genuinely separate Hermes process from this invocation's POV. + assert result == [(far_hermes_pid, "hermes.exe")] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_detect_concurrent_walk_tolerates_psutil_errors(_winp, tmp_path): + """An exception in the parent walk must not crash detection. + + psutil routinely raises ``AccessDenied`` / ``NoSuchProcess`` on the + PID 0 / PID 4 ancestors on Windows. The walk should degrade + gracefully — at worst the launcher gets reported, but the rest of + the gate must keep working. + """ + scripts_dir = tmp_path + shim = scripts_dir / "hermes.exe" + shim.write_bytes(b"") + peer_pid = os.getpid() + 30 + + class _ExplodingProc: + pid = -1 + + def parent(self): + raise RuntimeError("simulated AccessDenied") + + def exe(self): + raise RuntimeError("simulated AccessDenied") + + procs = [_make_proc(peer_pid, str(shim), "hermes.exe")] + fake = types.SimpleNamespace( + process_iter=lambda attrs: iter(procs), + Process=lambda pid=None: _ExplodingProc(), + Error=Exception, + ) + with patch.dict(sys.modules, {"psutil": fake}): + result = cli_main._detect_concurrent_hermes_instances(scripts_dir) + + # The peer is reported (no special launcher to exclude got found), and + # nothing crashed. + assert result == [(peer_pid, "hermes.exe")] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_detect_concurrent_walk_is_bounded(_winp, tmp_path): + """A pathological parent chain must not loop forever. + + Defensive: walk caps at 16 hops so a buggy psutil/proc table can't + hang ``hermes update``. + """ + scripts_dir = tmp_path + shim = scripts_dir / "hermes.exe" + shim.write_bytes(b"") + + # Build an infinite-looking chain of shim ancestors. The walk must + # terminate on its own bound and not stack-overflow / hang. + long_chain = [(os.getpid() + 100 + i, str(shim)) for i in range(64)] + procs = [_make_proc(pid, exe, "hermes.exe") for pid, exe in long_chain] + + fake = _fake_psutil_with_chain(procs, parent_chain=long_chain) + with patch.dict(sys.modules, {"psutil": fake}): + result = cli_main._detect_concurrent_hermes_instances(scripts_dir) + + # First 16 ancestors are excluded; the remaining 48 are reported. The + # exact split is implementation-detail-coupled to the bound, but the + # invariant we care about is: detection terminated AND did not + # exclude more than the bound. + excluded = len(long_chain) - len(result) + assert excluded <= 16 + 1 # +1 for os.getpid() + assert len(result) >= len(long_chain) - 16 + + # --------------------------------------------------------------------------- # _format_concurrent_instances_message # --------------------------------------------------------------------------- diff --git a/website/docs/getting-started/updating.md b/website/docs/getting-started/updating.md index 4a6c9b4ba9262..5ccb472344303 100644 --- a/website/docs/getting-started/updating.md +++ b/website/docs/getting-started/updating.md @@ -89,6 +89,8 @@ $ hermes update Close the listed processes and re-run. If you're sure the concurrent process won't interfere (rare — usually only useful when an antivirus shim is mis-attributed), pass `--force` to skip the check. In that case the updater will still retry the `.exe` rename with exponential backoff and, on stubborn locks, schedule the replacement for next reboot via `MoveFileEx(MOVEFILE_DELAY_UNTIL_REBOOT)` so the update can complete. +The detector deliberately ignores the distlib-generated `Scripts\hermes.exe` console launcher that started the current invocation — that launcher is a parent of the Python process doing the update, not a separate Hermes session. If you do see a PID reported, it really is a different process (Hermes Desktop's backend child, a second terminal's REPL, the gateway, …), not the launcher shim that just printed this message. + Expected output looks like: ```