diff --git a/contributors/emails/krzysztof.radzikowski@gmail.com b/contributors/emails/krzysztof.radzikowski@gmail.com new file mode 100644 index 0000000000..16e2d30311 --- /dev/null +++ b/contributors/emails/krzysztof.radzikowski@gmail.com @@ -0,0 +1 @@ +damadorPL diff --git a/hermes_cli/main.py b/hermes_cli/main.py index fe54ca3b2f..ce0691e033 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5056,6 +5056,7 @@ _LAZY_COMMAND_EXPORTS = { "_capture_active_lazy_features", "_capture_active_tool_dependencies", "_capture_head_sha", + "_classify_concurrent_instance", "_assess_parked_branch_switch", "_branch_head_label", "_branch_head_suffix", @@ -5076,6 +5077,7 @@ _LAZY_COMMAND_EXPORTS = { "_ensure_uv_for_termux", "_finish_dashboard_update_cleanup", "_fleet_probe_expected_runtimes", + "_filter_non_gateway_concurrent_instances", "_for_each_systemd_gateway_unit", "_format_concurrent_instances_message", "_format_time_ago", diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 2fb06d2028..e9d630defd 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -2578,6 +2578,65 @@ def _format_concurrent_instances_message( lines.append(" confirmed those processes will not write to the venv.") return "\n".join(lines) + +def _classify_concurrent_instance(pid: int) -> str: + """Return ``"gateway"`` when ``pid``'s command line is a gateway runtime. + + Delegates to ``_is_pausable_gateway`` — the same canonical + ``gateway run`` matcher (``gateway.status.looks_like_gateway_command_line``, + shlex-tokenized, profile-selector aware) used by the Desktop preflight + exemption and the venv-holder guard fallback — so a PID classified as + ``"gateway"`` here is exactly the set the pause/kill+restart machinery + downstream will stop. That symmetry is what lets the pre-update + concurrent gate skip the abort for gateway-only matches: the gateway is + going to be stopped by ``_pause_windows_gateways_for_update()`` moments + later anyway, so refusing the update just to make the user kill it + manually is friction without benefit. + + Returns ``"non-gateway"`` when the cmdline doesn't match, and + ``"unknown"`` when psutil can't read it (process gone, access denied, + psutil missing). The gate treats ``"unknown"`` as non-gateway — we'd + rather block an update we could have completed than proceed against a + process we couldn't positively identify as a gateway. + """ + try: + import psutil # noqa: PLC0415 + except Exception: + return "unknown" + + try: + proc = psutil.Process(int(pid)) + cmdline_list = proc.cmdline() + except Exception: + return "unknown" + + from hermes_cli._scan_venv_blockers import _is_pausable_gateway # noqa: PLC0415 + + cmdline = " ".join(cmdline_list or []) + if _is_pausable_gateway(cmdline): + 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 pause + machinery (``_pause_windows_gateways_for_update``) and the post-update + kill+restart block handle it — the update proceeds. If anything else (a + TUI shell, a Hermes Desktop backend child, an unrelated ``hermes`` REPL) + is in the list, the gate still aborts with the existing message, since + those have no pause machinery downstream. + """ + 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 _upgrade_pip_before_lazy_refresh( install_cmd_prefix: list[str], *, @@ -5868,13 +5927,30 @@ 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 (#37039): when every concurrent instance is a gateway + # runtime, the pause machinery a few lines below + # (``_pause_windows_gateways_for_update``) stops it before any file + # mutation, and the post-update restart phase brings it back. Aborting + # just to make the user run the same kill manually is friction without + # benefit. Anything not positively identified as a gateway (TUI shell, + # Desktop backend child, unreadable cmdline) still aborts exactly as + # before. if _m()._is_windows() and not getattr(args, "force", False): scripts_dir = _m()._venv_scripts_dir() if scripts_dir is not None: concurrent = _m()._detect_concurrent_hermes_instances(scripts_dir) if concurrent: - print(_format_concurrent_instances_message(concurrent, scripts_dir)) - sys.exit(2) + non_gateway = _m()._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 7c6aef5f5f..143a9eaf98 100644 --- a/tests/hermes_cli/test_update_concurrent_quarantine.py +++ b/tests/hermes_cli/test_update_concurrent_quarantine.py @@ -516,5 +516,209 @@ def test_unreadable_argv_falls_back_to_the_captured_prefix(monkeypatch): # --------------------------------------------------------------------------- +# --------------------------------------------------------------------------- +# _classify_concurrent_instance / _filter_non_gateway_concurrent_instances +# +# #37039: the pre-update concurrent-instance gate lets the update proceed +# when every concurrent hermes.exe is a gateway runtime — the pause +# machinery (_pause_windows_gateways_for_update) stops those before any +# file mutation and the post-update restart phase brings them back. +# Classification delegates to _is_pausable_gateway → the canonical +# gateway.status.looks_like_gateway_command_line matcher, so the gate's +# exemption and the pause discovery cannot drift apart. +# --------------------------------------------------------------------------- + + +def _fake_psutil_classify(argv_by_pid): + """psutil stand-in serving .cmdline() per pid; unknown pids raise.""" + + class FakeProc: + def __init__(self, pid): + if pid not in argv_by_pid: + raise ValueError(f"no such pid {pid}") + self._argv = argv_by_pid[pid] + + def cmdline(self): + return self._argv + + return types.SimpleNamespace(Process=FakeProc) + + +def test_classify_concurrent_instance_recognises_gateway_runtimes(monkeypatch): + """Gateway runtime command lines classify as ``gateway`` regardless of + launcher shape (python -m, hermes.exe shim, hermes-gateway.exe, + gateway/run.py, bare `hermes gateway` which defaults to run).""" + cases = [ + [r"C:\venv\Scripts\python.exe", "-m", "hermes_cli.main", "gateway", "run"], + [r"C:\venv\Scripts\hermes.exe", "gateway", "run"], + [r"C:\venv\Scripts\hermes-gateway.exe"], + [r"C:\venv\Scripts\python.exe", "gateway/run.py"], + ["hermes.exe", "GATEWAY", "RUN"], # matcher is case-insensitive + ["hermes.exe", "gateway"], # bare `hermes gateway` defaults to run + # profile selector before the subcommand — canonical matcher strips it + ["hermes.exe", "--profile", "work", "gateway", "run"], + ] + for argv in cases: + monkeypatch.setitem(sys.modules, "psutil", _fake_psutil_classify({77: argv})) + result = cli_main._classify_concurrent_instance(77) + assert result == "gateway", f"expected gateway for {argv!r}, got {result!r}" + + +def test_classify_concurrent_instance_recognises_non_gateways(monkeypatch): + """Non-runtime command lines classify as ``non-gateway`` — including + gateway MANAGEMENT subcommands (`gateway status`), which the canonical + matcher rejects but a substring matcher would misclassify. These keep + the pre-update abort.""" + cases = [ + [r"C:\venv\Scripts\hermes.exe"], # interactive REPL + [r"C:\venv\Scripts\hermes.exe", "dashboard"], + ["hermes.exe", "gateway", "status"], # management, not runtime + ["hermes.exe", "gateway", "stop"], + ["python", "-m", "hermes_cli.main"], + [], + ] + for argv in cases: + monkeypatch.setitem(sys.modules, "psutil", _fake_psutil_classify({77: argv})) + result = cli_main._classify_concurrent_instance(77) + assert result == "non-gateway", ( + f"expected non-gateway for {argv!r}, got {result!r}" + ) + + +def test_classify_concurrent_instance_unknown_on_psutil_error(monkeypatch): + """Unreadable cmdline (process gone / AccessDenied) → ``unknown`` — + treated as non-gateway by the filter, so the gate still aborts.""" + monkeypatch.setitem(sys.modules, "psutil", _fake_psutil_classify({})) + assert cli_main._classify_concurrent_instance(4242) == "unknown" + + +def test_classify_concurrent_instance_unknown_without_psutil(monkeypatch): + """Missing psutil entirely → ``unknown``, never a crash.""" + monkeypatch.setitem(sys.modules, "psutil", None) + assert cli_main._classify_concurrent_instance(4242) == "unknown" + + +def test_filter_non_gateway_concurrent_instances_splits(monkeypatch): + """Gateway PIDs drop out of the abort list; REPL/dashboard/unknown stay.""" + monkeypatch.setitem( + sys.modules, + "psutil", + _fake_psutil_classify( + { + 100: ["hermes.exe", "gateway", "run"], + 200: ["hermes.exe"], # REPL — keep + 300: ["hermes.exe", "dashboard"], # keep + # 400 missing → unknown → keep + } + ), + ) + matches = [ + (100, "hermes.exe"), + (200, "hermes.exe"), + (300, "hermes.exe"), + (400, "hermes.exe"), + ] + kept = cli_main._filter_non_gateway_concurrent_instances(matches) + assert kept == [(200, "hermes.exe"), (300, "hermes.exe"), (400, "hermes.exe")] + + +def test_filter_non_gateway_concurrent_instances_gateway_only(monkeypatch): + """All-gateway match list filters to empty — the gate lets the update + proceed and the pause machinery handles the gateways.""" + monkeypatch.setitem( + sys.modules, + "psutil", + _fake_psutil_classify( + { + 111: ["hermes.exe", "gateway", "run"], + 222: [r"C:\venv\Scripts\hermes-gateway.exe"], + } + ), + ) + matches = [(111, "hermes.exe"), (222, "hermes-gateway.exe")] + assert cli_main._filter_non_gateway_concurrent_instances(matches) == [] + + +# --------------------------------------------------------------------------- +# _cmd_update_impl integration with the relaxed pre-update gate (#37039) +# --------------------------------------------------------------------------- + + +def _update_args(): + return SimpleNamespace( + check=False, + gateway=False, + yes=False, + force=False, + backup=False, + no_backup=True, + ) + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_update_gate_skips_abort_when_only_concurrent_is_gateway( + _winp, tmp_path, capsys +): + """Regression test for #37039: with only gateway processes concurrent, + the gate must NOT sys.exit(2) — the update proceeds to the pre-update + backup step (sentinel), and the pause machinery owns the gateways.""" + scripts_dir = tmp_path / "Scripts" + scripts_dir.mkdir() + + 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=[] + ) as mock_filter, patch.object( + cli_main, "_run_pre_update_backup" + ) as mock_backup: + mock_backup.side_effect = RuntimeError("reached post-gate body") + with pytest.raises(RuntimeError, match="reached post-gate body"): + cli_main._cmd_update_impl(_update_args(), gateway_mode=False) + + mock_filter.assert_called_once() + mock_backup.assert_called_once() + captured = capsys.readouterr().out + assert "Another hermes.exe is running" not in captured + + +@patch.object(cli_main, "_is_windows", return_value=True) +def test_update_gate_still_aborts_on_non_gateway_concurrent( + _winp, tmp_path, capsys +): + """A non-gateway concurrent instance must still abort with exit 2, and + the message must list only the non-gateway PIDs (the gateway is not the + user's problem to kill).""" + scripts_dir = tmp_path / "Scripts" + scripts_dir.mkdir() + + 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"), (3000, "hermes.exe")], + ), patch.object( + cli_main, + "_filter_non_gateway_concurrent_instances", + return_value=[(3000, "hermes.exe")], + ), patch.object( + cli_main, "_run_pre_update_backup" + ) as mock_backup: + with pytest.raises(SystemExit) as excinfo: + cli_main._cmd_update_impl(_update_args(), gateway_mode=False) + + assert excinfo.value.code == 2 + mock_backup.assert_not_called() + captured = capsys.readouterr().out + assert "3000" in captured + assert "1000" not in captured # gateway PID no longer blamed + assert "--force" in captured + + diff --git a/tests/hermes_cli/test_venv_holder_windows_live.py b/tests/hermes_cli/test_venv_holder_windows_live.py index abad856e74..e1efa14d82 100644 --- a/tests/hermes_cli/test_venv_holder_windows_live.py +++ b/tests/hermes_cli/test_venv_holder_windows_live.py @@ -250,3 +250,58 @@ class TestAncestorExclusion: "gateway ancestor invisible to venv scan — /update from the " f"gateway can never pause it (#87594): {payload}" ) + + +class TestConcurrentGateClassification: + """#37039 — the pre-update concurrent-instance gate must classify LIVE + processes: gateway runtimes drop out of the abort list (the pause + machinery owns them), everything else keeps aborting the update.""" + + def test_live_gateway_process_classified_gateway(self): + """A real process whose argv carries `-m hermes_cli.main gateway run` + classifies as ``gateway`` via real psutil against the live table.""" + from hermes_cli.update_cmd import _classify_concurrent_instance + + proc = _spawn(["-m", "hermes_cli.main", "gateway", "run"]) + try: + assert _classify_concurrent_instance(proc.pid) == "gateway" + finally: + _kill(proc) + + def test_live_non_gateway_processes_keep_the_abort(self): + """A REPL-shaped process and a gateway MANAGEMENT command both + classify as ``non-gateway`` — they stay in the abort list.""" + from hermes_cli.update_cmd import _classify_concurrent_instance + + repl = _spawn(["-m", "hermes_cli.main"]) + mgmt = _spawn(["-m", "hermes_cli.main", "gateway", "status"]) + try: + assert _classify_concurrent_instance(repl.pid) == "non-gateway" + assert _classify_concurrent_instance(mgmt.pid) == "non-gateway" + finally: + _kill(repl, mgmt) + + def test_live_filter_drops_only_the_gateway(self): + """End-to-end filter over a mixed live process set: the gateway PID + drops, the serve-backend PID stays, a dead PID stays (unknown).""" + from hermes_cli.update_cmd import ( + _filter_non_gateway_concurrent_instances, + ) + + gw = _spawn(["-m", "hermes_cli.main", "gateway", "run"]) + backend = _spawn(["-m", "hermes_cli.main", "serve", "--port", "8127"]) + dead = _spawn([]) + _kill(dead) # reaped → unreadable cmdline → unknown → kept + try: + matches = [ + (gw.pid, "hermes.exe"), + (backend.pid, "hermes.exe"), + (dead.pid, "hermes.exe"), + ] + kept = _filter_non_gateway_concurrent_instances(matches) + kept_pids = {pid for pid, _ in kept} + assert gw.pid not in kept_pids, "gateway must drop from abort list" + assert backend.pid in kept_pids, "serve backend must keep aborting" + assert dead.pid in kept_pids, "unknown must keep aborting" + finally: + _kill(gw, backend)