From fadcaa3806b876c3d5bbd9e8cdf27d799223d827 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Tue, 11 Aug 2026 01:17:47 +0000 Subject: [PATCH 1/2] fix(update): restart hermes-serve systemd units alongside gateways MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hermes update discovered and restarted hermes-gateway* systemd units but never looked for hermes-serve* — the Desktop app's backend — so it kept running stale pre-update code until the user restarted it by hand (#83438). Extend the systemd unit discovery/restart loop to also match hermes-serve* units. They don't wire SIGUSR1 to a graceful drain (only gateway/run.py does), so restart eligibility for the graceful path is now gated on unit name via a small, directly-tested helper; hermes-serve units fall straight to the existing blunt systemctl restart path, matching the workaround the issue already documents. --- hermes_cli/main.py | 1 + hermes_cli/update_cmd.py | 71 ++++++++++++------- .../test_update_fleet_restart_timeout.py | 39 ++++++++++ 3 files changed, 86 insertions(+), 25 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index e81b57a493cd..b3b863aaa584 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5142,6 +5142,7 @@ def _clear_bytecode_cache(root: Path) -> int: _resume_windows_gateways_after_update, _run_logged_subprocess, _run_pre_update_backup, + _service_unit_supports_graceful_sigusr1_restart, _should_skip_upstream_prompt, _stash_apply_failed_only_on_existing_untracked, _stash_local_changes_if_needed, diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 5b810d04aba1..dbc160db0aec 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -3452,7 +3452,8 @@ def _for_each_systemd_gateway_unit( process_unit, on_unit_timeout, ) -> None: - """Process each ``hermes-gateway*.service`` from ``systemctl list-units``. + """Process each ``hermes-gateway*.service``/``hermes-serve*.service`` unit + from ``systemctl list-units``. ``subprocess.TimeoutExpired`` raised by ``process_unit`` is isolated to that unit via ``on_unit_timeout`` so one wedged systemctl call cannot @@ -3466,8 +3467,8 @@ def _for_each_systemd_gateway_unit( if not unit.endswith(".service"): continue # list-units is already pattern-filtered, but keep the name gate so a - # stray non-gateway line cannot enter the restart path. - if not unit.startswith("hermes-gateway"): + # stray non-gateway/serve line cannot enter the restart path. + if not (unit.startswith("hermes-gateway") or unit.startswith("hermes-serve")): continue svc_name = unit.removesuffix(".service") try: @@ -3475,6 +3476,18 @@ def _for_each_systemd_gateway_unit( except subprocess.TimeoutExpired as exc: on_unit_timeout(svc_name, exc) +def _service_unit_supports_graceful_sigusr1_restart(svc_name: str) -> bool: + """Whether *svc_name* wires SIGUSR1 to a graceful drain-then-restart. + + Only ``hermes-gateway*`` units run ``gateway/run.py``, which installs the + SIGUSR1 handler. ``hermes-serve*`` units (#83438) don't, so sending them + SIGUSR1 would just invoke the default terminate action and burn the full + drain budget waiting for an exit that was never graceful — go straight to + the blunt ``systemctl restart`` path for those instead. + """ + return svc_name.startswith("hermes-gateway") + + def _warn_incomplete_gateway_fleet_restart(failed_units: list) -> None: """Print an explicit incomplete-update warning for unrestarted units.""" if not failed_units: @@ -3488,7 +3501,7 @@ def _warn_incomplete_gateway_fleet_restart(failed_units: list) -> None: seen.add(name) ordered.append(name) print() - print("⚠ Update incomplete — some gateway units were not restarted:") + print("⚠ Update incomplete — some units were not restarted:") for name in ordered: print(f" - {name}") print(" Skipped units may still be running pre-update code (mixed") @@ -5210,7 +5223,8 @@ def _resolve_manage_cmd(scope_: str, scope_cmd_: list, svc_name_: str): externally_supervised_profiles = [] # --- Systemd services (Linux) --- - # Discover all hermes-gateway* units (default + profiles) + # Discover all hermes-gateway* units (default + profiles) plus + # hermes-serve* units (the Desktop app's backend, #83438). if supports_systemd_services(): try: _ensure_user_systemd_env() @@ -5227,6 +5241,7 @@ def _resolve_manage_cmd(scope_: str, scope_cmd_: list, svc_name_: str): + [ "list-units", "hermes-gateway*", + "hermes-serve*", "--plain", "--no-legend", "--no-pager", @@ -5273,27 +5288,33 @@ def _restart_one_systemd_gateway_unit(svc_name: str) -> None: # The gateway's SIGUSR1 handler calls # request_restart(via_service=True) → drain → # exit; systemd's Restart=always respawns the unit. + # hermes-serve has no such handler (it isn't + # gateway/run.py), so skip straight to the blunt + # restart below rather than sending it an unhandled + # signal and waiting out the drain budget for + # nothing. _main_pid = 0 - try: - _show = subprocess.run( - scope_cmd - + [ - "show", - svc_name, - "--property=MainPID", - "--value", - ], - capture_output=True, - text=True, encoding="utf-8", errors="replace", - timeout=5, - ) - _main_pid = int((_show.stdout or "").strip() or 0) - except ( - ValueError, - subprocess.TimeoutExpired, - FileNotFoundError, - ): - _main_pid = 0 + if _service_unit_supports_graceful_sigusr1_restart(svc_name): + try: + _show = subprocess.run( + scope_cmd + + [ + "show", + svc_name, + "--property=MainPID", + "--value", + ], + capture_output=True, + text=True, encoding="utf-8", errors="replace", + timeout=5, + ) + _main_pid = int((_show.stdout or "").strip() or 0) + except ( + ValueError, + subprocess.TimeoutExpired, + FileNotFoundError, + ): + _main_pid = 0 _graceful_ok = False if _main_pid > 0: diff --git a/tests/hermes_cli/test_update_fleet_restart_timeout.py b/tests/hermes_cli/test_update_fleet_restart_timeout.py index 2de629208153..210a768095aa 100644 --- a/tests/hermes_cli/test_update_fleet_restart_timeout.py +++ b/tests/hermes_cli/test_update_fleet_restart_timeout.py @@ -15,6 +15,7 @@ from hermes_cli.main import ( _for_each_systemd_gateway_unit, + _service_unit_supports_graceful_sigusr1_restart, _warn_incomplete_gateway_fleet_restart, ) @@ -90,6 +91,44 @@ def test_non_gateway_units_in_list_output_are_ignored(self): assert seen == ["hermes-gateway-coder"] + def test_hermes_serve_units_are_included(self): + # #83438 — hermes update restarted hermes-gateway* units but left + # hermes-serve* (the Desktop app's backend) on stale pre-update code. + seen: list[str] = [] + + _for_each_systemd_gateway_unit( + "\n".join( + [ + "ssh.service loaded active running", + "hermes-serve.service loaded active running", + "hermes-serve-work.service loaded active running", + "hermes-gateway.service loaded active running", + "", + ] + ), + process_unit=seen.append, + on_unit_timeout=lambda *_: pytest.fail("unexpected timeout"), + ) + + assert seen == ["hermes-serve", "hermes-serve-work", "hermes-gateway"] + + +class TestGracefulSigusr1Eligibility: + def test_gateway_units_are_eligible(self): + assert _service_unit_supports_graceful_sigusr1_restart("hermes-gateway") + assert _service_unit_supports_graceful_sigusr1_restart( + "hermes-gateway-work" + ) + + def test_serve_units_are_not_eligible(self): + # hermes-serve doesn't run gateway/run.py, so it never installs the + # SIGUSR1 handler — sending it the signal would just terminate the + # process (the default action) instead of draining gracefully. + assert not _service_unit_supports_graceful_sigusr1_restart("hermes-serve") + assert not _service_unit_supports_graceful_sigusr1_restart( + "hermes-serve-work" + ) + def test_process_errors_other_than_timeout_still_propagate(self): def process_unit(_svc_name: str) -> None: raise RuntimeError("not a timeout") From 573c6e2e43a0520355faff13c5f00f538b80b344 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Sun, 16 Aug 2026 04:08:13 +0000 Subject: [PATCH 2/2] fix(update): tighten hermes-serve unit gate, dedupe fleet/cleanup restarts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #83595 flagged two service-lifecycle gaps in the hermes-serve restart support: - The unit-name gate accepted anything starting with "hermes-serve", which also matched the unrelated hermes-server.service. Require the exact base unit or the hyphenated profile family instead. - The fleet-restart loop and _finish_dashboard_update_cleanup() could both restart the same hermes-serve unit — the loop restarts it directly, then cleanup's PID scan finds the fresh process and restarts its owning unit again. Thread the fleet loop's restarted unit names through to _kill_stale_dashboard_processes() so it skips units already handled. --- hermes_cli/dashboard_procs.py | 28 +++++++++++-- hermes_cli/update_cmd.py | 40 ++++++++++++++++--- .../test_update_fleet_restart_timeout.py | 14 +++++++ .../hermes_cli/test_update_stale_dashboard.py | 24 +++++++++++ 4 files changed, 97 insertions(+), 9 deletions(-) diff --git a/hermes_cli/dashboard_procs.py b/hermes_cli/dashboard_procs.py index 35a047f6f1b0..c20fd2b277f7 100644 --- a/hermes_cli/dashboard_procs.py +++ b/hermes_cli/dashboard_procs.py @@ -148,6 +148,7 @@ def _kill_stale_dashboard_processes( reason: str = "the running backend no longer matches the updated frontend", *, restart_managed: bool = False, + already_restarted_units: "set[str] | None" = None, ) -> dict[str, list]: """Kill running ``hermes dashboard`` / ``hermes serve`` processes. @@ -171,6 +172,14 @@ def _kill_stale_dashboard_processes( e.g. a remote backend's ``hermes-serve.service``) has its owning unit restarted after the kill, because systemd treats our SIGTERM as a clean stop and ``Restart=on-failure`` would never fire (#68934). + + *already_restarted_units* names units (no ``.service`` suffix) the + caller already restarted directly — e.g. ``hermes update``'s systemd + fleet-restart loop, which restarts ``hermes-serve*`` units before this + function runs. Without excluding them, a Serve-only install's freshly + restarted process is found again here and restarted a second time for + no benefit (review on #83595). PIDs owned by one of these units are + left untouched. """ if restart_managed and _m()._restart_managed_dashboard_service(reason): return {"matched": [], "killed": [], "failed": []} @@ -199,9 +208,6 @@ def _kill_stale_dashboard_processes( if not pids: return {"matched": [], "killed": [], "failed": []} - print() - print(f"⟲ Stopping {len(pids)} dashboard process(es) ({reason})") - # Before killing, snapshot systemd cgroup info for each PID so we can # restart supervised services after the kill (the cgroup disappears # along with the process). Only meaningful on Linux, and only when the @@ -222,6 +228,22 @@ def _kill_stale_dashboard_processes( if cmdline: pid_cmdline[pid] = cmdline + if already_restarted_units: + # Already handled directly by the caller (e.g. hermes update's + # systemd fleet-restart loop) — leave these alone instead of + # killing and re-restarting a process that's already fresh. + pids = [ + pid + for pid in pids + if (pid_service.get(pid) or "").removesuffix(".service") + not in already_restarted_units + ] + if not pids: + return {"matched": [], "killed": [], "failed": []} + + print() + print(f"⟲ Stopping {len(pids)} dashboard process(es) ({reason})") + killed: list[int] = [] failed: list[tuple[int, str]] = [] diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index dbc160db0aec..7c28fcf0e60f 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -618,15 +618,25 @@ def _format_time_ago(iso_ts: str) -> str: except Exception: return "recently" -def _finish_dashboard_update_cleanup(node_failures: list[str]) -> None: - """Refresh managed dashboards or stop stale manual ones after an update.""" +def _finish_dashboard_update_cleanup( + node_failures: list[str], already_restarted_units: "set[str] | None" = None +) -> None: + """Refresh managed dashboards or stop stale manual ones after an update. + + *already_restarted_units* forwards the systemd unit names (no + ``.service`` suffix) that the fleet-restart loop already restarted + directly, so a Serve-only install's freshly restarted process isn't + found and restarted a second time here (review on #83595). + """ if node_failures: print() print(" ℹ Leaving running dashboard process(es) untouched because the") print(" Node.js dependency refresh did not complete.") return - stop_result = _m()._kill_stale_dashboard_processes(restart_managed=True) + stop_result = _m()._kill_stale_dashboard_processes( + restart_managed=True, already_restarted_units=already_restarted_units + ) if not stop_result.get("unrecovered"): return @@ -3468,7 +3478,14 @@ def _for_each_systemd_gateway_unit( continue # list-units is already pattern-filtered, but keep the name gate so a # stray non-gateway/serve line cannot enter the restart path. - if not (unit.startswith("hermes-gateway") or unit.startswith("hermes-serve")): + # ``unit.startswith("hermes-serve")`` alone would also accept the + # unrelated ``hermes-server.service`` — require the exact base unit + # or the hyphenated profile family instead (review on #83595). + if not ( + unit.startswith("hermes-gateway") + or unit == "hermes-serve.service" + or unit.startswith("hermes-serve-") + ): continue svc_name = unit.removesuffix(".service") try: @@ -5043,6 +5060,12 @@ def _print_items(items, label, key, fallback_key=None): pass gateway_fleet_restart_incomplete = False + # Declared outside the restart try/except below (and never reset + # to None) so it's always safe to read afterwards even if that + # block raises before reaching its own restart bookkeeping — + # needed to forward already-restarted units to + # ``_finish_dashboard_update_cleanup`` (review on #83595). + restarted_services: list = [] # Auto-restart ALL gateways after update. # The code update (git pull) is shared across all profiles, so every @@ -5216,7 +5239,6 @@ def _resolve_manage_cmd(scope_: str, scope_cmd_: list, svc_name_: str): except Exception: _drain_budget = 45.0 - restarted_services = [] failed_or_stale_units = [] killed_pids = set() relaunched_profiles = [] @@ -5749,7 +5771,13 @@ def _on_unit_timeout(svc_name: str, exc: subprocess.TimeoutExpired) -> None: # 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. - _finish_dashboard_update_cleanup(node_failures) + # + # Forward the systemd units restarted above (includes hermes-serve*, + # #83438) so a Serve-only install's freshly restarted process isn't + # found and restarted again below (review on #83595). + _finish_dashboard_update_cleanup( + node_failures, already_restarted_units=set(restarted_services) + ) print() print("Tip: You can now select a provider and model:") diff --git a/tests/hermes_cli/test_update_fleet_restart_timeout.py b/tests/hermes_cli/test_update_fleet_restart_timeout.py index 210a768095aa..67866be8105f 100644 --- a/tests/hermes_cli/test_update_fleet_restart_timeout.py +++ b/tests/hermes_cli/test_update_fleet_restart_timeout.py @@ -112,6 +112,20 @@ def test_hermes_serve_units_are_included(self): assert seen == ["hermes-serve", "hermes-serve-work", "hermes-gateway"] + def test_hermes_server_near_prefix_is_rejected(self): + # Review on #83595: a bare ``startswith("hermes-serve")`` gate also + # accepts the unrelated ``hermes-server.service``. Only the exact + # base unit or the hyphenated profile family should pass. + seen: list[str] = [] + + _for_each_systemd_gateway_unit( + _list_units_stdout(["hermes-server"]), + process_unit=seen.append, + on_unit_timeout=lambda *_: pytest.fail("unexpected timeout"), + ) + + assert seen == [] + class TestGracefulSigusr1Eligibility: def test_gateway_units_are_eligible(self): diff --git a/tests/hermes_cli/test_update_stale_dashboard.py b/tests/hermes_cli/test_update_stale_dashboard.py index 547b4f887ac8..3a43f5cf2eba 100644 --- a/tests/hermes_cli/test_update_stale_dashboard.py +++ b/tests/hermes_cli/test_update_stale_dashboard.py @@ -324,6 +324,30 @@ def fake_kill(pid, sig): # Supervised restart succeeded — no manual hint. assert "when you're ready" not in out + def test_already_restarted_unit_is_left_untouched(self): + """Review on #83595: hermes update's systemd fleet-restart loop may + already have restarted this PID's owning unit directly (e.g. a + Serve-only install). Passing it via already_restarted_units must + skip killing/restarting it again here.""" + live = self._live() + + with patch.object(live, "_restart_managed_dashboard_service", return_value=False), \ + patch.object(live, "_find_stale_dashboard_pids", return_value=[4321]), \ + patch.object(live, "_get_pid_cgroup_path", + return_value="/system.slice/hermes-serve.service"), \ + patch.object(live, "_get_systemd_service_for_pid", + return_value="hermes-serve.service"), \ + patch.object(live, "_try_restart_systemd_service") as restart, \ + patch("os.kill") as kill, \ + patch("time.sleep"): + result = _kill_stale_dashboard_processes( + restart_managed=True, already_restarted_units={"hermes-serve"} + ) + + kill.assert_not_called() + restart.assert_not_called() + assert result == {"matched": [], "killed": [], "failed": []} + class TestManualBackendRespawn: """Manually-started dashboards/serves have their argv captured before the