fix(update): gateway-only concurrent instances no longer abort hermes update (#37039)
On Windows, the pre-update concurrent-instance gate aborted with exit 2 whenever ANY other process held the venv hermes.exe shim — including the gateway itself, which _pause_windows_gateways_for_update() stops moments later and the post-update restart phase brings back. Users with a running gateway were forced into a manual taskkill dance before every update. The gate now filters gateway runtimes out of the abort list and proceeds when nothing else is concurrent. Classification delegates to _is_pausable_gateway -> gateway.status.looks_like_gateway_command_line (the canonical shlex-tokenized, profile-selector-aware matcher shared by the Desktop preflight exemption and the venv-holder guard fallback), so the gate's exemption and the pause machinery cannot drift apart. Anything not positively identified as a gateway — REPLs, dashboard, Desktop backend children, gateway MANAGEMENT commands like 'gateway status', unreadable cmdlines — still aborts exactly as before, and the abort message now lists only the PIDs that are actually the user's problem. Surgical reapply of PR #37039 by @damadorPL onto current main (the gate moved from hermes_cli/main.py to hermes_cli/update_cmd.py in the main.py decomposition); his substring classifier was replaced with the canonical matcher, which also fixes the 'hermes gateway status' misclassification flagged in review. Co-authored-by: Hermes <hermes@nousresearch.com>
This commit is contained in:
Krzysztof Radzikowski
committed by
Teknium
Teknium
parent
26777a4178
commit
36f1423411
@@ -0,0 +1 @@
|
||||
damadorPL
|
||||
@@ -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",
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user