From 7798241eab7249facec7c4f79ddc050bc14fd7da Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 7 Sep 2026 03:13:56 -0700 Subject: [PATCH] fix: retain pending fleet restarts until supervisors recover Discover systemd targets before stopping old processes, restart even when there are no gateway PIDs, and require successful scope listings plus active verification. Pending launchd recovery also retains failures for inaccessible listings and installed jobs without supervision. Keep existing PID cleanup intact but before recovery so it cannot kill freshly verified workers. Slim redo informed by #104274, #104283, and #104285. Co-authored-by: fangliquanflq Co-authored-by: liuhao1024 --- hermes_cli/update_cmd_fleet.py | 64 +++++++++------ .../test_pending_supervisor_recovery.py | 79 +++++++++++++++++++ .../test_update_fleet_restart_pending.py | 7 +- website/docs/getting-started/updating.md | 10 +++ 4 files changed, 135 insertions(+), 25 deletions(-) create mode 100644 tests/hermes_cli/test_pending_supervisor_recovery.py diff --git a/hermes_cli/update_cmd_fleet.py b/hermes_cli/update_cmd_fleet.py index a4246b6a69..daa53f29fd 100644 --- a/hermes_cli/update_cmd_fleet.py +++ b/hermes_cli/update_cmd_fleet.py @@ -195,29 +195,36 @@ def _needs_sudo(scope: str) -> bool: ) -def _restart_systemd_gateway_units_best_effort(failed: list) -> None: +def _restart_systemd_gateway_units_best_effort(failed: list, listings) -> None: """Best-effort ``systemctl restart`` of every hermes-gateway/serve unit.""" - for scope, scope_cmd, result in _systemd_gateway_unit_listings(): + answered = set() + for scope, scope_cmd, result in listings: + answered.add(scope) if result.returncode != 0: + failed.append(f"systemd-{scope} (listing failed)") continue def process_unit(svc_name: str, _scope=scope, _cmd=scope_cmd) -> None: - restart_cmd = list(_cmd) + ["--no-ask-password", "restart", svc_name] + manage_cmd = list(_cmd) + ["--no-ask-password"] if _needs_sudo(_scope): - restart_cmd = ["sudo", "-n"] + restart_cmd - _systemctl(restart_cmd, timeout=30) + manage_cmd = ["sudo", "-n"] + manage_cmd + result = _systemctl_reset_and_restart(manage_cmd, svc_name) + if result.returncode != 0 or not _wait_for_service_active(_cmd, svc_name): + failed.append(svc_name) _for_each_systemd_gateway_unit( result.stdout, process_unit=process_unit, on_unit_timeout=lambda svc_name, exc: failed.append(svc_name), ) + # A timeout or missing executable is not an empty scope. + failed.extend(f"systemd-{scope} (listing unavailable)" for scope, _ in _SYSTEMD_SCOPES if scope not in answered) def _run_pending_fleet_restart() -> bool: """Catch-up restart for gateways left on pre-update code. Never raises. - True when the restart completed or nothing was running; False if incomplete. + True when all discovered targets recovered (or none exist); False if incomplete. See #95294. """ @@ -244,22 +251,30 @@ def _run_pending_fleet_restart() -> bool: logger.debug("Pending fleet restart: gateway probe failed: %s", exc) pids = None - if pids == []: - print(" ✓ No running gateways — nothing to restart.") - return True - failed: list = [] try: + # Snapshot before stopping: Restart=no units can disappear from list-units on a clean exit. + systemd_listings = list(_systemd_gateway_unit_listings()) if supports_systemd_services() else None + # Stop old processes before supervisor recovery, never its freshly verified workers. + if pids != []: + try: + leftover = list(find_gateway_pids(all_profiles=True)) + except Exception: + leftover = list(pids or []) + if leftover: + with _best_effort('Pending fleet restart: PID stop failed: %s'): + kill_gateway_processes(all_profiles=True) + _wait_for_gateway_exit(timeout=5.0, force_after=None) # --- Systemd services (Linux) --- Discover all hermes-gateway* units (default + profiles) plus # hermes-serve* units (the Desktop app's backend, #83438). - if supports_systemd_services(): - _restart_systemd_gateway_units_best_effort(failed) + if systemd_listings is not None: + _restart_systemd_gateway_units_best_effort(failed, systemd_listings) # --- Launchd services (macOS) --- Restart EVERY ai.hermes.gateway* LaunchAgent, not only the # invoking profile's — parity with the systemd branch above (#41403). Per-label TimeoutExpired # isolation happens inside. if is_macos(): try: - _restart_macos_launchd_gateways([], failed, 45.0) + _restart_macos_launchd_gateways([], failed, 45.0, require_supervision=True) except Exception as exc: logger.debug("Pending fleet restart: launchd failed: %s", exc) failed.append("launchd") @@ -271,14 +286,6 @@ def _run_pending_fleet_restart() -> bool: except Exception as exc: logger.debug("Pending fleet restart: Windows failed: %s", exc) failed.append("windows-gateway") - try: - leftover = list(find_gateway_pids(all_profiles=True)) - except Exception: - leftover = list(pids or []) - if leftover: - with _best_effort('Pending fleet restart: PID stop failed: %s'): - kill_gateway_processes(all_profiles=True) - _wait_for_gateway_exit(timeout=5.0, force_after=None) if failed: _warn_incomplete_gateway_fleet_restart(failed) return False @@ -465,7 +472,9 @@ def _restart_launchd_gateway_after_update(*, supervision_verify: bool = True) -> return [], [current_label] -def _restart_macos_launchd_gateways(restarted_services: list, failed_or_stale_units: list, drain_budget: float) -> None: +def _restart_macos_launchd_gateways( + restarted_services: list, failed_or_stale_units: list, drain_budget: float, *, require_supervision: bool = False, +) -> None: """Restart every launchd-managed gateway after an update (macOS). The pull is shared across profiles, so every ``ai.hermes.gateway*`` LaunchAgent @@ -480,9 +489,14 @@ def _restart_macos_launchd_gateways(restarted_services: list, failed_or_stale_un cannot leave the rest of the fleet on old code (#68523). """ from hermes_cli.gateway import ( - get_launchd_label, launchd_gateway_labels_for_install, _graceful_restart_via_sigusr1, _launchd_kickstart, + get_launchd_label, get_launchd_plist_path, launchd_gateway_labels_for_install, _graceful_restart_via_sigusr1, _launchd_kickstart, _locate_launchd_gateway_service, _wait_for_launchd_service_pid, ) + if require_supervision: + listing = subprocess.run(["launchctl", "list"], capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=10) + if listing.returncode != 0: + failed_or_stale_units.append("launchd (listing failed)") + return _restarted, _failed = _restart_launchd_gateway_after_update(supervision_verify=True) restarted_services.extend(_restarted) failed_or_stale_units.extend(_failed) @@ -496,7 +510,9 @@ def _restart_macos_launchd_gateways(restarted_services: list, failed_or_stale_un # reuse that domain so a sibling is never probed in one and restarted in another. domain, old_pid = _locate_launchd_gateway_service(label) if domain is None: - continue # installed but not bootstrapped — nothing running old code here + if require_supervision and get_launchd_plist_path().with_name(f"{label}.plist").exists(): + failed_or_stale_units.append(label) + continue # A profile without an installed job has no restart target. graceful_ok = False if old_pid is not None and old_pid > 0: print(f" → {label}: draining (up to {int(drain_budget)}s)...") diff --git a/tests/hermes_cli/test_pending_supervisor_recovery.py b/tests/hermes_cli/test_pending_supervisor_recovery.py new file mode 100644 index 0000000000..46b974708f --- /dev/null +++ b/tests/hermes_cli/test_pending_supervisor_recovery.py @@ -0,0 +1,79 @@ +"""A pending restart is discharged by supervisor evidence, not an empty PID scan.""" +import subprocess +from types import SimpleNamespace + +import pytest + +from hermes_cli import gateway, main, update_cmd_fleet as fleet + + +@pytest.mark.linux_only +@pytest.mark.parametrize("failure", ["listing", "timeout", "missing", "restart", "inactive", "running", None]) +def test_pending_marker_requires_complete_systemd_recovery(monkeypatch, tmp_path, failure): + monkeypatch.setattr(main, "_purge_stale_hermes_modules", lambda: None) + stopped = [] + monkeypatch.setattr(gateway, "find_gateway_pids", lambda **kw: [123] if failure == "running" and not stopped else []) + monkeypatch.setattr(gateway, "kill_gateway_processes", lambda **kw: stopped.append(True)) + monkeypatch.setattr(gateway, "_wait_for_gateway_exit", lambda **kw: None) + monkeypatch.setattr(gateway, "supports_systemd_services", lambda: True) + monkeypatch.setattr(fleet, "_SYSTEMD_SCOPES", (("user", ["systemctl", "--user"]),)) + monkeypatch.setattr(fleet._time, "sleep", lambda _: None) + ticks = iter(range(1000)) + monkeypatch.setattr(fleet._time, "monotonic", lambda: next(ticks)) + recovered = [] + + def systemctl(cmd, **kw): + if "list-units" in cmd: + if stopped: + return SimpleNamespace(returncode=0, stdout="", stderr="") + if failure == "timeout": + raise subprocess.TimeoutExpired(cmd, 10) + if failure == "missing": + raise FileNotFoundError("systemctl") + return SimpleNamespace(returncode=int(failure == "listing"), stdout=( + "hermes-gateway-one.service loaded active running\n" + "hermes-gateway-two.service loaded failed failed\n"), stderr="") + bad = cmd[-1] == "hermes-gateway-two" + if "restart" in cmd: + recovered.append(cmd[-1]) + return SimpleNamespace(returncode=int(bad and failure == "restart"), stdout="") + if "is-active" in cmd: + active = not (bad and failure == "inactive") + return SimpleNamespace(returncode=0 if active else 3, stdout="active" if active else "inactive") + return SimpleNamespace(returncode=0, stdout="0s") + + monkeypatch.setattr(fleet, "_systemctl", systemctl) + marker = fleet._fleet_restart_pending_marker_path() + marker.write_text("expected_sha=pending\n") + if failure not in (None, "running"): + with pytest.raises(SystemExit, match="1"): + fleet._apply_pending_fleet_restart_catchup() + assert marker.exists() + else: + fleet._apply_pending_fleet_restart_catchup() + assert not marker.exists() + assert set(recovered) == {"hermes-gateway-one", "hermes-gateway-two"} + + +@pytest.mark.parametrize("failure", ["listing", "restart", "inactive", "unloaded", None]) +def test_pending_launchd_requires_complete_supervision(monkeypatch, tmp_path, failure): + # Host-independent subprocess-boundary fixture, not native launchd validation. + current, sibling = "ai.hermes.gateway", "ai.hermes.gateway-two" + (tmp_path / f"{sibling}.plist").touch() + monkeypatch.setattr(gateway, "get_launchd_label", lambda: current) + monkeypatch.setattr(gateway, "get_launchd_plist_path", lambda: tmp_path / f"{current}.plist") + monkeypatch.setattr(gateway, "launchd_gateway_labels_for_install", lambda: [current, sibling]) + monkeypatch.setattr(fleet, "_restart_launchd_gateway_after_update", lambda **kw: ([], [])) + monkeypatch.setattr(gateway, "_locate_launchd_gateway_service", lambda _: (None, None) if failure == "unloaded" else ("gui/501", None)) + monkeypatch.setattr(gateway, "_wait_for_launchd_service_pid", lambda *a, **kw: None if failure == "inactive" else 42) + + def kickstart(*args): + if failure == "restart": + raise subprocess.CalledProcessError(1, "launchctl") + + monkeypatch.setattr(gateway, "_launchd_kickstart", kickstart) + monkeypatch.setattr(fleet.subprocess, "run", lambda *a, **kw: SimpleNamespace(returncode=int(failure == "listing"), stdout="", stderr="")) + restarted, failed = [], [] + fleet._restart_macos_launchd_gateways(restarted, failed, 0, require_supervision=True) + assert bool(failed) is bool(failure) + assert (sibling in restarted) is (failure is None) diff --git a/tests/hermes_cli/test_update_fleet_restart_pending.py b/tests/hermes_cli/test_update_fleet_restart_pending.py index 4056d73bc6..8ccbaa004f 100644 --- a/tests/hermes_cli/test_update_fleet_restart_pending.py +++ b/tests/hermes_cli/test_update_fleet_restart_pending.py @@ -349,8 +349,13 @@ def test_run_pending_restart_true_when_no_gateways(monkeypatch, capsys): ) monkeypatch.setattr(hermes_main, "_purge_stale_hermes_modules", lambda: None) + # An empty PID scan is insufficient; both supervisor scopes must answer empty. + monkeypatch.setattr(update_cmd_fleet, "_systemd_gateway_unit_listings", lambda: [ + (scope, cmd, SimpleNamespace(returncode=0, stdout="")) + for scope, cmd in update_cmd_fleet._SYSTEMD_SCOPES + ]) assert update_cmd._run_pending_fleet_restart() is True - assert "nothing to restart" in capsys.readouterr().out + assert "Pending fleet restart completed" in capsys.readouterr().out # --------------------------------------------------------------------------- diff --git a/website/docs/getting-started/updating.md b/website/docs/getting-started/updating.md index 18663f49e4..136d56d6b8 100644 --- a/website/docs/getting-started/updating.md +++ b/website/docs/getting-started/updating.md @@ -100,6 +100,16 @@ The same inventory is embedded in every real update's receipt (`~/.hermes/logs/u Every `hermes update` run writes a machine-readable receipt to `~/.hermes/logs/update_receipts/` (last 20 kept, `latest.json` always points at the most recent): the pre-update fleet plan, each step taken, anything skipped and why, the gateway restart outcome, and the final fleet version matrix. After the restart phase the updater compares each live gateway's running code against the freshly updated checkout and prints a per-profile matrix — a gateway still serving pre-update code is reported loudly with the exact restart command, and the update exits non-zero so automation never treats a mixed-version fleet as healthy. Both `--plan` and the fleet check ask each running gateway directly over its local control socket (`gateway.sock` in the profile's data directory, a named pipe on Windows) when available, so version and supervisor information comes from the gateway itself; gateways from older versions are still discovered through their state files as before. +### Interrupted gateway restarts + +If an earlier update pulled code but did not finish restarting the fleet, the next +`hermes update` retries even when the checkout is already current. An empty process +scan does not prove recovery: failed systemd units and installed launchd jobs may +have no live PID. The pending restart marker is retained if supervisor discovery +fails, a restart fails, or a requested service cannot be verified active. The update +exits nonzero and reports the affected services; recover them with the printed +commands and retry `hermes update`. + ### Full pre-update backup: `--backup` For high-value profiles (production gateways, shared team installs) you can opt into a full pre-pull backup of `HERMES_HOME` (config, auth, sessions, skills, pairing):