From 1d9ffbdc42fe2a572cd4a02abd28d6cd6e4f0764 Mon Sep 17 00:00:00 2001 From: Krzysztof Radzikowski Date: Tue, 2 Jun 2026 00:03:36 +0200 Subject: [PATCH] fix(update): skip concurrent-instance abort when only the gateway is running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, `hermes update` aborts with exit code 2 whenever any other process whose `.exe` is the venv `hermes.exe` or `hermes-gateway.exe` shim is running — including the gateway itself, which the post-update kill+restart block is going to reap anyway. The user is forced to `taskkill /F /IM hermes.exe` every running instance and retry, even though the update would have worked fine: the post-update sweep matches gateway PIDs by command line ("hermes gateway" / "hermes.exe gateway" / etc.), not by the venv shim path, and it correctly excludes the calling process and its ancestor chain. The fix classifies each concurrent-instance match by reading its cmdline through psutil and comparing it to the same gateway-marker substrings the post-update sweep uses. If every concurrent match is a gateway, the update proceeds; the gateway PID is reaped by the post-update block a few seconds later. Unrelated concurrent instances (TUI shell, Hermes Desktop backend child, a plain REPL) still trigger the existing abort message, since those aren't covered by the post-update sweep. Files changed: - hermes_cli/main.py: add _classify_concurrent_instance() and _filter_non_gateway_concurrent_instances(); gate the pre-update concurrent check on the non-gateway subset only. - tests/hermes_cli/test_update_concurrent_quarantine.py: add classification, filter, and end-to-end cmd_update tests covering both the gateway-only (skip-abort) and mixed (still-abort) paths. --- hermes_cli/main.py | 81 +++++- .../test_update_concurrent_quarantine.py | 270 ++++++++++++++++++ 2 files changed, 349 insertions(+), 2 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 13d8277528978..620c492647764 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -8235,6 +8235,70 @@ def _format_concurrent_instances_message( return "\n".join(lines) +def _classify_concurrent_instance(pid: int) -> str: + """Return ``"gateway"`` if ``pid``'s command line looks like a gateway. + + Mirrors the substring patterns used by + ``hermes_cli.gateway._scan_gateway_pids`` so a PID classified as + ``"gateway"`` here is the same set of PIDs the post-update kill+restart + block will reap. That symmetry is what lets the pre-update concurrent + gate safely skip the abort for gateway-only matches: the gateway is + going to be killed anyway, so refusing to update just to make the user + kill it manually is friction without benefit. + + Returns ``"non-gateway"`` when the cmdline doesn't match the gateway + patterns, and ``"unknown"`` when psutil can't read it (process gone, + access denied, etc.). The pre-update gate treats ``"unknown"`` as + non-gateway (i.e. errs on the side of the existing abort) — we'd + rather block an update we could have completed than kill something + we couldn't positively identify as a gateway. + """ + try: + import psutil + except Exception: + return "unknown" + + try: + proc = psutil.Process(int(pid)) + cmdline_list = proc.cmdline() + except Exception: + return "unknown" + + cmdline = " ".join(cmdline_list or []) + cmdline_lc = cmdline.lower() + gateway_markers = ( + "hermes_cli.main gateway", + "hermes_cli/main.py gateway", + "hermes gateway", + "hermes.exe gateway", + "hermes-gateway.exe", + "gateway/run.py", + ) + if any(marker in cmdline_lc for marker in gateway_markers): + return "gateway" + return "non-gateway" + + +def _filter_non_gateway_concurrent_instances( + matches: list[tuple[int, str]], +) -> list[tuple[int, str]]: + """Return only the concurrent-instance matches that are NOT the gateway. + + Used by the pre-update concurrent gate to decide whether to abort + ``hermes update``. If every concurrent instance is a gateway, the + post-update kill+restart block will handle it — we let the update + proceed. If anything else (a TUI shell, a Hermes Desktop backend + child, an unrelated ``hermes`` REPL) is in the list, we still abort + with the existing message, since those aren't covered by the + post-update sweep. + """ + non_gateway: list[tuple[int, str]] = [] + for pid, name in matches: + if _classify_concurrent_instance(pid) != "gateway": + non_gateway.append((pid, name)) + return non_gateway + + def _quarantine_running_hermes_exe( scripts_dir: Path, *, max_attempts: int = 4 ) -> list[tuple[Path, Path]]: @@ -9301,13 +9365,26 @@ def _cmd_update_impl(args, gateway_mode: bool): # open. Continuing would result in a string of WinError 32 warnings and # then either a deferred-rename leftover or a failed git-pull fast path # that silently falls back to the slower ZIP route. See issue #26670. + # + # Exception: when the only concurrent instance is a gateway process, the + # post-update logic at the bottom of this function (the gateway kill+ + # restart block) already handles it cleanly — it matches gateways by + # command line ("hermes gateway" / "hermes.exe gateway" / etc.), not by + # the venv shim .exe path, so the gateway is always stopped and respawned + # after a successful code update. Aborting the update just to make the + # user do the same kill manually is pointless friction — the gateway is + # going to be killed anyway. Skip the abort in that case and let the + # update flow continue; the gateway PID gets reaped by the post-update + # block a few seconds later. if _is_windows() and not getattr(args, "force", False): scripts_dir = _venv_scripts_dir() if scripts_dir is not None: concurrent = _detect_concurrent_hermes_instances(scripts_dir) if concurrent: - print(_format_concurrent_instances_message(concurrent, scripts_dir)) - sys.exit(2) + non_gateway = _filter_non_gateway_concurrent_instances(concurrent) + if non_gateway: + print(_format_concurrent_instances_message(non_gateway, scripts_dir)) + sys.exit(2) # Pre-update backup — runs before any git/file mutation so users can # always roll back to the exact state they had before this update. diff --git a/tests/hermes_cli/test_update_concurrent_quarantine.py b/tests/hermes_cli/test_update_concurrent_quarantine.py index fe14856fd7eaa..2d6c86895ed38 100644 --- a/tests/hermes_cli/test_update_concurrent_quarantine.py +++ b/tests/hermes_cli/test_update_concurrent_quarantine.py @@ -530,3 +530,273 @@ def test_cmd_update_force_bypasses_concurrent_check(_winp, tmp_path): # When --force is set, we should not have even consulted psutil. detect.assert_not_called() + + +# --------------------------------------------------------------------------- +# _classify_concurrent_instance / _filter_non_gateway_concurrent_instances +# +# These helpers were added so the pre-update concurrent-instance gate can +# let the update proceed when the only concurrent hermes.exe is a gateway +# process — the post-update kill+restart block at the bottom of cmd_update +# already handles gateway PIDs by command-line match, so refusing the +# update just to make the user kill the gateway manually is friction +# without benefit. +# --------------------------------------------------------------------------- + + +def _make_psutil_proc_with_cmdline(pid: int, cmdline: list[str]): + """Mock a psutil.Process with a .cmdline() method returning ``cmdline``.""" + proc = MagicMock() + proc.cmdline = MagicMock(return_value=list(cmdline)) + return proc + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_classify_concurrent_instance_recognises_hermes_gateway(_winp): + """A process whose cmdline contains the ``gateway`` keyword classifies + as ``"gateway"`` regardless of the launcher (python -m, hermes.exe + shim, hermes-gateway.exe shim, or direct gateway/run.py invocation).""" + from hermes_cli.main import _classify_concurrent_instance + + cases = [ + # python -m hermes_cli.main gateway run + ["C:/venv/Scripts/python.exe", "-m", "hermes_cli.main", "gateway", "run"], + # hermes.exe gateway run (Windows console-script shim) + ["C:/venv/Scripts/hermes.exe", "gateway", "run"], + # hermes-gateway.exe direct + ["C:/venv/Scripts/hermes-gateway.exe"], + # direct python gateway/run.py (development launch) + ["C:/venv/Scripts/python.exe", "gateway/run.py"], + # case-insensitive: uppercase GATEWAY + ["hermes.exe", "GATEWAY", "run"], + ] + for cmdline in cases: + fake = types.SimpleNamespace( + Process=lambda pid: _make_psutil_proc_with_cmdline(pid, cmdline) + ) + with patch.dict(sys.modules, {"psutil": fake}): + result = _classify_concurrent_instance(1234) + assert result == "gateway", f"expected gateway for cmdline={cmdline!r}, got {result!r}" + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_classify_concurrent_instance_recognises_non_gateway(_winp): + """Processes without ``gateway`` in their cmdline classify as + ``"non-gateway"``. This is the bucket that should still trigger the + pre-update abort (TUI shells, Desktop backend, plain REPLs).""" + from hermes_cli.main import _classify_concurrent_instance + + cases = [ + # Interactive REPL — no gateway subcommand + ["C:/venv/Scripts/hermes.exe"], + # Dashboard + ["C:/venv/Scripts/hermes.exe", "dashboard"], + # Direct python -m hermes_cli.main (no subcommand) + ["python", "-m", "hermes_cli.main"], + # Empty cmdline + [], + ] + for cmdline in cases: + fake = types.SimpleNamespace( + Process=lambda pid: _make_psutil_proc_with_cmdline(pid, cmdline) + ) + with patch.dict(sys.modules, {"psutil": fake}): + result = _classify_concurrent_instance(1234) + assert result == "non-gateway", f"expected non-gateway for cmdline={cmdline!r}, got {result!r}" + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_classify_concurrent_instance_returns_unknown_on_psutil_error(_winp): + """If psutil can't read the cmdline (process gone, AccessDenied on a + session boundary, etc.) the helper returns ``"unknown"``. The pre-update + gate treats ``"unknown"`` as non-gateway — we'd rather block an update + we could have completed than kill something we couldn't positively + identify as a gateway.""" + from hermes_cli.main import _classify_concurrent_instance + + class _BrokenProc: + def cmdline(self): # noqa: D401 — psutil Process duck type + raise psutil.AccessDenied(pid=1234) + + fake = types.SimpleNamespace(Process=lambda pid: _BrokenProc()) + import psutil as _real_psutil + with patch.dict(sys.modules, {"psutil": _real_psutil}): + result = _classify_concurrent_instance(1234) + assert result == "unknown" + + +def test_classify_concurrent_instance_returns_unknown_without_psutil(): + """If psutil isn't importable at all (very rare but possible in slim + installs), the helper returns ``"unknown"`` rather than crashing.""" + from hermes_cli.main import _classify_concurrent_instance + + with patch.dict(sys.modules, {"psutil": None}): + result = _classify_concurrent_instance(1234) + assert result == "unknown" + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_filter_non_gateway_concurrent_instances_splits_correctly(_winp): + """The filter drops PIDs whose cmdline looks like a gateway and keeps + the rest. The pre-update gate then aborts only on the kept list.""" + from hermes_cli.main import _filter_non_gateway_concurrent_instances + + # Three PIDs: one gateway (drop), one TUI/REPL (keep), one Desktop + # backend child (keep). The gate should only block on the two + # non-gateway entries; the gateway is left for the post-update sweep. + def _make_filter_psutil(): + def _proc_factory(pid: int): + if pid == 100: + return _make_psutil_proc_with_cmdline( + pid, ["hermes.exe", "gateway", "run"] + ) + if pid == 200: + return _make_psutil_proc_with_cmdline( + pid, ["hermes.exe"] # interactive REPL + ) + if pid == 300: + return _make_psutil_proc_with_cmdline( + pid, ["hermes.exe", "dashboard"] + ) + raise AssertionError(f"unexpected pid {pid}") + + return types.SimpleNamespace(Process=_proc_factory) + + matches = [(100, "hermes.exe"), (200, "hermes.exe"), (300, "hermes.exe")] + + with patch.dict(sys.modules, {"psutil": _make_filter_psutil()}): + kept = _filter_non_gateway_concurrent_instances(matches) + + assert kept == [(200, "hermes.exe"), (300, "hermes.exe")] + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_filter_non_gateway_concurrent_instances_drops_gateway_only(_winp): + """If every concurrent instance is a gateway, the filter returns an + empty list. The pre-update gate sees no non-gateway matches and lets + the update proceed — the gateway PID will be reaped by the post-update + kill+restart block.""" + from hermes_cli.main import _filter_non_gateway_concurrent_instances + + def _proc_factory(pid: int): + return _make_psutil_proc_with_cmdline( + pid, ["hermes.exe", "gateway", "run"] + ) + + matches = [(111, "hermes.exe"), (222, "hermes-gateway.exe")] + + with patch.dict(sys.modules, {"psutil": types.SimpleNamespace(Process=_proc_factory)}): + kept = _filter_non_gateway_concurrent_instances(matches) + + assert kept == [] + + +# --------------------------------------------------------------------------- +# cmd_update integration with the relaxed pre-update gate +# --------------------------------------------------------------------------- + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_cmd_update_skips_abort_when_only_concurrent_is_gateway( + _winp, tmp_path, capsys +): + """Regression test for the user-reported issue: when the gateway is + the only other ``hermes.exe`` running, ``hermes update`` used to abort + with exit code 2, forcing the user to kill all hermes.exe manually + before retrying. The post-update kill+restart block already handles + gateway PIDs cleanly, so the gate should now let the update proceed + instead.""" + scripts_dir = tmp_path / "Scripts" + scripts_dir.mkdir() + + args = SimpleNamespace( + check=False, + gateway=False, + yes=False, + force=False, + backup=False, + no_backup=True, + ) + + # Two concurrent matches, BOTH gateway — filter should return []. + # Short-circuit at the first call after the gate so we don't have to + # mock the entire update pipeline. + with patch.object( + cli_main, "_venv_scripts_dir", return_value=scripts_dir + ), patch.object( + cli_main, + "_detect_concurrent_hermes_instances", + return_value=[(1000, "hermes.exe"), (2000, "hermes-gateway.exe")], + ), patch.object( + cli_main, "_filter_non_gateway_concurrent_instances", return_value=[] + ), patch.object( + cli_main, "_run_pre_update_backup" + ) as mock_backup, patch.object( + cli_main, "_install_hangup_protection", return_value={} + ), patch.object( + cli_main, "_finalize_update_output" + ): + # Should NOT raise SystemExit. The sentinel that fires at the next + # real step (pre-update backup) lets us assert we got past the + # gate without mocking the whole pipeline. + mock_backup.side_effect = RuntimeError("reached post-gate body") + with pytest.raises(RuntimeError, match="reached post-gate body"): + cli_main.cmd_update(args) + + # Pre-update backup was reached — gate let us through. + mock_backup.assert_called_once() + + captured = capsys.readouterr().out + assert "Another hermes.exe is running" not in captured + assert "--force" not in captured # the abort-message override hint + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_cmd_update_still_aborts_on_non_gateway_concurrent( + _winp, tmp_path, capsys +): + """The gate must STILL abort when an unrelated concurrent hermes.exe + is in the list (TUI, Desktop backend, plain REPL). Only gateway-only + matches are exempted — see test_cmd_update_skips_abort_when_only_* + for the relaxed case.""" + scripts_dir = tmp_path / "Scripts" + scripts_dir.mkdir() + + args = SimpleNamespace( + check=False, + gateway=False, + yes=False, + force=False, + backup=False, + no_backup=True, + ) + + with patch.object( + cli_main, "_venv_scripts_dir", return_value=scripts_dir + ), patch.object( + cli_main, + "_detect_concurrent_hermes_instances", + return_value=[ + (1000, "hermes.exe"), # would be classified as gateway + (3000, "hermes.exe"), # would be classified as non-gateway + ], + ), patch.object( + cli_main, + "_filter_non_gateway_concurrent_instances", + return_value=[(3000, "hermes.exe")], # simulated filter result + ), patch.object( + cli_main, "_run_pre_update_backup" + ) as mock_backup, patch.object( + cli_main, "_install_hangup_protection", return_value={} + ), patch.object( + cli_main, "_finalize_update_output" + ): + with pytest.raises(SystemExit) as excinfo: + cli_main.cmd_update(args) + + assert excinfo.value.code == 2 + mock_backup.assert_not_called() + + captured = capsys.readouterr().out + assert "3000" in captured + assert "--force" in captured