fix(update): restart hermes-serve systemd units alongside gateways

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]] = []
 

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,

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
 
@@ -3452,7 +3462,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,15 +3477,34 @@ 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.
+        # ``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:
             process_unit(svc_name)
         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 +3518,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")
@@ -5030,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
@@ -5203,14 +5239,14 @@ 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 = []
             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 +5263,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 +5310,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:
@@ -5728,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:")

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,58 @@ 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"]
+
+    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):
+        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")

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