diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 72d400ff193f..ff9743080765 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -6199,11 +6199,10 @@ def _find_stale_dashboard_pids( disk is updated, causing a silent frontend/backend mismatch (e.g. new auth headers the old backend doesn't recognise → every API call 401s). - The dashboard has no service manager (systemd / launchd), no PID file, - and we can't know the original launch args — so the only sane action - after an update is to kill the stale process and let the user restart - it. This helper is just the detection step; see - ``_kill_stale_dashboard_processes`` for the kill. + The dashboard may be manually started or managed by the optional + ``hermes-dashboard.service`` systemd unit. Managed units are restarted + through their owning systemd scope; only manually-started processes use + the kill path because we can't know their original launch args. *exclude_pids* is an optional set of PIDs that must never be returned. This is used by the Hermes Desktop Electron app to protect its own @@ -6543,8 +6542,121 @@ def _format_time_ago(iso_ts: str) -> str: return "recently" +_DASHBOARD_SYSTEMD_UNIT = "hermes-dashboard.service" + + +def _restart_managed_dashboard_service( + reason: str, + unit: str = _DASHBOARD_SYSTEMD_UNIT, +) -> bool: + """Restart a systemd-managed dashboard instead of raw-killing its PID. + + Returns True when a dashboard unit was found and handled (successfully or + with a printed actionable failure). Returning True deliberately prevents + the caller from falling back to ``os.kill``: systemd treats a direct + SIGTERM of the service's main PID as a clean stop, so ``Restart=on-failure`` + will not bring the dashboard back. + """ + if sys.platform == "win32": + return False + + def _systemctl(*args: str, timeout: int = 10) -> subprocess.CompletedProcess: + return subprocess.run( + ["systemctl", *args], + capture_output=True, + text=True, + timeout=timeout, + ) + + # Probe the user manager first: Hermes installs Linux services in the + # user's systemd scope by default. Only fall back to the system manager + # when the unit is not present there, preserving root/system deployments. + # Crucially, keep the selected scope for *all* probes and the restart — a + # user unit must never be restarted through the system manager (or raw-killed). + scope: tuple[str, ...] | None = None + listed: subprocess.CompletedProcess | None = None + for candidate in (("--user",), ()): + try: + result = _systemctl( + *candidate, "list-unit-files", unit, "--no-legend", "--no-pager" + ) + except (FileNotFoundError, subprocess.TimeoutExpired, OSError): + continue + if result.returncode != 0: + continue + unit_rows = (result.stdout or "").splitlines() + if any(row.split()[0:1] == [unit] for row in unit_rows if row.split()): + scope = candidate + listed = result + break + + if scope is None or listed is None: + return False + + try: + active = _systemctl(*scope, "is-active", unit) + enabled = _systemctl(*scope, "is-enabled", unit) + except (FileNotFoundError, subprocess.TimeoutExpired, OSError): + return False + + active_state = (active.stdout or "").strip() + enabled_state = (enabled.stdout or "").strip() + if active_state != "active" and enabled_state not in { + "enabled", + "enabled-runtime", + "linked", + "linked-runtime", + "static", + "generated", + }: + return False + + print() + print(f"⟲ Restarting managed dashboard service ({reason})") + + scope_label = "systemctl --user" if scope else "sudo systemctl" + restart = ("systemctl", *scope, "restart", unit) + commands = [restart] + if not scope: + # System units may require privilege escalation; user units must use + # the user manager directly and never prompt for sudo. + commands.append(("sudo", "-n", "systemctl", "restart", unit)) + + errors: list[str] = [] + for command in commands: + try: + result = subprocess.run( + list(command), + capture_output=True, + text=True, + timeout=60, + ) + except (FileNotFoundError, subprocess.TimeoutExpired, OSError) as e: + errors.append(f"{' '.join(command)}: {e}") + continue + if result.returncode == 0: + print(f" ✓ restarted {unit}") + return True + errors.append( + f"{' '.join(command)}: {(result.stderr or result.stdout or '').strip()}" + ) + + print(f" ✗ failed to restart {unit}") + for err in errors: + if err.strip(): + print(f" {err}") + print( + " Dashboard is managed by systemd; not raw-killing its PID because " + "systemd would treat that as a clean stop." + ) + print(f" Restart manually: {scope_label} restart {unit}") + return True + + def _kill_stale_dashboard_processes( reason: str = "the running backend no longer matches the updated frontend", + *, + restart_managed: bool = False, ) -> None: """Kill running ``hermes dashboard`` processes. @@ -6560,10 +6672,15 @@ def _kill_stale_dashboard_processes( Windows: ``taskkill /PID /F`` since there's no clean SIGTERM equivalent for background console apps. - The dashboard isn't auto-restarted because we don't know the original - launch args (--host, --port, --insecure, --tui, --no-open). The user - restarts it manually; a hint is printed. + Manually-started dashboards are not auto-restarted because we don't know + the original launch args (--host, --port, --insecure, --tui, --no-open). + When ``restart_managed`` is true (the ``hermes update`` path), a detected + ``hermes-dashboard.service`` is restarted through systemd instead of + raw-killing its main PID. """ + if restart_managed and _restart_managed_dashboard_service(reason): + return + # When the Hermes Desktop Electron app spawns this dashboard as a # backend child, it sets HERMES_DESKTOP_CHILD_PID so that the update # path can skip killing the desktop-managed process. (#37532) @@ -6915,7 +7032,7 @@ def _update_via_zip(args): print(" ℹ Leaving running dashboard process(es) untouched because the") print(" Node.js dependency refresh did not complete.") else: - _kill_stale_dashboard_processes() + _kill_stale_dashboard_processes(restart_managed=True) def _stash_local_changes_if_needed(git_cmd: list[str], cwd: Path) -> Optional[str]: @@ -11864,22 +11981,17 @@ def _cmd_update_impl(args, gateway_mode: bool): except Exception as e: logger.debug("Legacy unit check during update failed: %s", e) - # Kill stale dashboard processes — the dashboard has no service - # manager, so leaving it alive after a code update produces a - # silent frontend/backend mismatch. We can't auto-restart it - # (no saved launch args) but we can stop it, and a hint is - # printed for the user to re-launch. - # - # Exception: if the Node dependency refresh failed, the rebuilt - # frontend the new backend expects may not exist, so stopping a - # working dashboard would leave the user with nothing running - # rather than a usable (if mixed) state (#30271). Leave it alone. + # Restart a managed dashboard through systemd, or stop stale manual + # dashboard processes. Raw-killing a systemd-owned dashboard PID makes + # systemd treat it as a clean stop, leaving the Cloudflare origin dead. + # Preserve the safety rule above: a failed Node refresh leaves the + # currently running dashboard untouched. if node_failures: print() print(" ℹ Leaving running dashboard process(es) untouched because the") print(" Node.js dependency refresh did not complete.") else: - _kill_stale_dashboard_processes() + _kill_stale_dashboard_processes(restart_managed=True) print() print("Tip: You can now select a provider and model:") diff --git a/tests/hermes_cli/test_update_stale_dashboard.py b/tests/hermes_cli/test_update_stale_dashboard.py index 4134bacc9e87..fd26590786ec 100644 --- a/tests/hermes_cli/test_update_stale_dashboard.py +++ b/tests/hermes_cli/test_update_stale_dashboard.py @@ -23,6 +23,7 @@ import pytest from hermes_cli.main import ( _find_stale_dashboard_pids, _kill_stale_dashboard_processes, + _restart_managed_dashboard_service, _warn_stale_dashboard_processes, # back-compat alias ) @@ -47,6 +48,7 @@ def _refresh_bindings_against_live_module(): """ global _find_stale_dashboard_pids global _kill_stale_dashboard_processes + global _restart_managed_dashboard_service global _warn_stale_dashboard_processes live = sys.modules.get("hermes_cli.main") @@ -55,6 +57,7 @@ def _refresh_bindings_against_live_module(): _find_stale_dashboard_pids = live._find_stale_dashboard_pids _kill_stale_dashboard_processes = live._kill_stale_dashboard_processes + _restart_managed_dashboard_service = live._restart_managed_dashboard_service _warn_stale_dashboard_processes = live._warn_stale_dashboard_processes yield @@ -332,6 +335,131 @@ class TestKillStaleDashboardPosix: assert "✓ stopped PID 12345" in out assert "failed to stop" not in out + def test_update_path_restarts_managed_dashboard_instead_of_killing(self, capsys): + """A systemd-managed dashboard must be restarted through systemd. + + Raw-killing the unit's main PID makes systemd record a clean stop, so + Restart=on-failure does not recover the Cloudflare origin. + """ + calls: list[list[str]] = [] + + def fake_run(args, *a, **kw): + calls.append(list(args)) + if args == ["systemctl", "--user", "list-unit-files", "hermes-dashboard.service", "--no-legend", "--no-pager"]: + return MagicMock(returncode=0, stdout="", stderr="") + if args[:2] == ["systemctl", "list-unit-files"]: + return MagicMock(returncode=0, stdout="hermes-dashboard.service enabled enabled\n", stderr="") + if args[:2] == ["systemctl", "is-active"]: + return MagicMock(returncode=0, stdout="active\n", stderr="") + if args[:2] == ["systemctl", "is-enabled"]: + return MagicMock(returncode=0, stdout="enabled\n", stderr="") + if args == ["systemctl", "restart", "hermes-dashboard.service"]: + return MagicMock(returncode=0, stdout="", stderr="") + raise AssertionError(f"unexpected subprocess.run call: {args}") + + with patch("subprocess.run", side_effect=fake_run), \ + patch("hermes_cli.main._find_stale_dashboard_pids", + return_value=[12345]) as find_pids, \ + patch("os.kill") as kill: + _kill_stale_dashboard_processes(restart_managed=True) + + assert ["systemctl", "restart", "hermes-dashboard.service"] in calls + find_pids.assert_not_called() + kill.assert_not_called() + + out = capsys.readouterr().out + assert "Restarting managed dashboard service" in out + assert "✓ restarted hermes-dashboard.service" in out + + def test_user_scope_restart_never_falls_back_to_system_or_sudo(self, capsys): + """A user unit is discovered and restarted through ``systemctl --user``.""" + calls: list[list[str]] = [] + + def fake_run(args, *a, **kw): + calls.append(list(args)) + if args == ["systemctl", "--user", "list-unit-files", "hermes-dashboard.service", "--no-legend", "--no-pager"]: + return MagicMock(returncode=0, stdout="hermes-dashboard.service enabled enabled\n", stderr="") + if args == ["systemctl", "--user", "is-active", "hermes-dashboard.service"]: + return MagicMock(returncode=0, stdout="active\n", stderr="") + if args == ["systemctl", "--user", "is-enabled", "hermes-dashboard.service"]: + return MagicMock(returncode=0, stdout="enabled\n", stderr="") + if args == ["systemctl", "--user", "restart", "hermes-dashboard.service"]: + return MagicMock(returncode=0, stdout="", stderr="") + raise AssertionError(f"unexpected subprocess.run call: {args}") + + with patch("subprocess.run", side_effect=fake_run), \ + patch("hermes_cli.main._find_stale_dashboard_pids", return_value=[12345]) as find_pids, \ + patch("os.kill") as kill: + _kill_stale_dashboard_processes(restart_managed=True) + + assert calls == [ + ["systemctl", "--user", "list-unit-files", "hermes-dashboard.service", "--no-legend", "--no-pager"], + ["systemctl", "--user", "is-active", "hermes-dashboard.service"], + ["systemctl", "--user", "is-enabled", "hermes-dashboard.service"], + ["systemctl", "--user", "restart", "hermes-dashboard.service"], + ] + assert all(call[:1] != ["sudo"] and call[:2] != ["systemctl"] for call in calls) + find_pids.assert_not_called() + kill.assert_not_called() + assert "✓ restarted hermes-dashboard.service" in capsys.readouterr().out + + def test_user_scope_restart_failure_does_not_try_system_or_sudo(self): + """A failed user-manager restart remains fail-closed and never raw-kills.""" + calls: list[list[str]] = [] + + def fake_run(args, *a, **kw): + calls.append(list(args)) + if args == ["systemctl", "--user", "list-unit-files", "hermes-dashboard.service", "--no-legend", "--no-pager"]: + return MagicMock(returncode=0, stdout="hermes-dashboard.service enabled enabled\n", stderr="") + if args[-2:] == ["is-active", "hermes-dashboard.service"]: + return MagicMock(returncode=0, stdout="active\n", stderr="") + if args[-2:] == ["is-enabled", "hermes-dashboard.service"]: + return MagicMock(returncode=0, stdout="enabled\n", stderr="") + if args[-2:] == ["restart", "hermes-dashboard.service"]: + return MagicMock(returncode=1, stdout="", stderr="user manager unavailable") + raise AssertionError(f"unexpected subprocess.run call: {args}") + + with patch("subprocess.run", side_effect=fake_run), \ + patch("hermes_cli.main._find_stale_dashboard_pids") as find_pids, \ + patch("os.kill") as kill: + _kill_stale_dashboard_processes(restart_managed=True) + + assert calls[-1] == ["systemctl", "--user", "restart", "hermes-dashboard.service"] + assert not any(call[:1] == ["sudo"] or call == ["systemctl", "restart", "hermes-dashboard.service"] for call in calls) + find_pids.assert_not_called() + kill.assert_not_called() + + def test_managed_dashboard_restart_failure_does_not_raw_kill(self, capsys): + """If systemd restart cannot run, print the fix and do not kill the PID.""" + def fake_run(args, *a, **kw): + if args == ["systemctl", "--user", "list-unit-files", "hermes-dashboard.service", "--no-legend", "--no-pager"]: + return MagicMock(returncode=0, stdout="", stderr="") + if args[:2] == ["systemctl", "list-unit-files"]: + return MagicMock(returncode=0, stdout="hermes-dashboard.service enabled enabled\n", stderr="") + if args[:2] == ["systemctl", "is-active"]: + return MagicMock(returncode=0, stdout="active\n", stderr="") + if args[:2] == ["systemctl", "is-enabled"]: + return MagicMock(returncode=0, stdout="enabled\n", stderr="") + if args == ["systemctl", "restart", "hermes-dashboard.service"]: + return MagicMock(returncode=1, stdout="", stderr="Interactive authentication required.\n") + if args == ["sudo", "-n", "systemctl", "restart", "hermes-dashboard.service"]: + return MagicMock(returncode=1, stdout="", stderr="a password is required\n") + raise AssertionError(f"unexpected subprocess.run call: {args}") + + with patch("subprocess.run", side_effect=fake_run), \ + patch("hermes_cli.main._find_stale_dashboard_pids", + return_value=[12345]) as find_pids, \ + patch("os.kill") as kill: + _kill_stale_dashboard_processes(restart_managed=True) + + find_pids.assert_not_called() + kill.assert_not_called() + + out = capsys.readouterr().out + assert "failed to restart hermes-dashboard.service" in out + assert "not raw-killing its PID" in out + assert "sudo systemctl restart hermes-dashboard.service" in out + class TestKillStaleDashboardWindows: """Kill path on Windows: taskkill /F."""