fix(update): restart a dashboard's systemd unit only when the unit's MainPID is the dashboard
A .service cgroup only says where a process was started. A dashboard started by hand from a shell that runs under some unit (a CI runner agent, cron.service, a tmux or IDE user service, the gateway's terminal tool) sits in that unit's cgroup, and hermes update restarted that unrelated unit instead of respawning the dashboard, leaving it down. _get_systemd_service_for_pid now claims the unit only when its live MainPID is the PID; otherwise the argv respawn path runs, which is what the update plan already predicted (manual-serve / respawn-argv).
This commit is contained in:
@@ -184,18 +184,38 @@ def _pid_unified_cgroup_entries(pid: int):
|
||||
|
||||
|
||||
def _get_systemd_service_for_pid(pid: int) -> str | None:
|
||||
"""The systemd service unit name *pid* belongs to (``hermes-serve.service``), or None.
|
||||
"""The systemd service unit that supervises *pid* (``hermes-serve.service``), or None.
|
||||
|
||||
None when the PID isn't part of a service, the file is unreadable, or off Linux.
|
||||
A ``.service`` cgroup alone only says where the process was started: a dashboard launched by
|
||||
hand from a shell that itself runs under some unit (a CI runner agent, ``cron.service``, a
|
||||
tmux or IDE user service, the gateway's own terminal tool) sits in THAT unit's cgroup. The
|
||||
unit owns the backend only when its live ``MainPID`` is this PID; otherwise restarting it
|
||||
restarts an unrelated service and leaves the dashboard down. None when the PID isn't part of
|
||||
a service, ownership can't be proved, the file is unreadable, or off Linux.
|
||||
"""
|
||||
for cg_path in _pid_unified_cgroup_entries(pid):
|
||||
if cg_path.endswith(".service"):
|
||||
svc_name = cg_path.rsplit("/", 1)[-1]
|
||||
if svc_name:
|
||||
if svc_name and _unit_main_pid_is(svc_name, cg_path, pid):
|
||||
return svc_name
|
||||
return None
|
||||
|
||||
|
||||
def _unit_main_pid_is(svc_name: str, cgroup_path: str, pid: int) -> bool:
|
||||
"""True when *svc_name*'s live ``MainPID`` is *pid* (read-only ``systemctl show``)."""
|
||||
scope = _extract_scope_from_cgroup(cgroup_path)
|
||||
scopes = {"user": [["--user"]], "system": [[]]}.get(scope or "", [[], ["--user"]])
|
||||
for scope_args in scopes:
|
||||
try:
|
||||
result = _run_probe(
|
||||
["systemctl", *scope_args, "show", svc_name, "--property=MainPID", "--value"], timeout=10)
|
||||
except _SYSTEMCTL_ERRORS:
|
||||
continue
|
||||
if result.returncode == 0 and (result.stdout or "").strip() == str(pid):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _extract_scope_from_cgroup(cgroup_entry: str) -> str | None:
|
||||
"""``user`` / ``system`` from a cgroup path (``/user.slice/…`` vs ``/system.slice/…``), else None."""
|
||||
if "/system.slice/" in cgroup_entry:
|
||||
|
||||
@@ -359,6 +359,47 @@ class TestSupervisedBackendRestart:
|
||||
restart.assert_not_called()
|
||||
assert result == {"matched": [], "killed": [], "failed": []}
|
||||
|
||||
@pytest.mark.parametrize("main_pid, restarted", [("991", False), ("4321", True)],
|
||||
ids=["foreign-unit-cgroup", "unit-main-process"])
|
||||
def test_only_the_unit_whose_main_process_is_the_backend_is_restarted(self, main_pid, restarted):
|
||||
"""A dashboard started by hand from a shell inside some unit (CI runner agent, cron, tmux,
|
||||
the gateway's terminal tool) sits in that unit's cgroup. Only a unit whose MainPID IS the
|
||||
backend supervises it; any other unit is not restarted and the backend is respawned from
|
||||
its argv instead."""
|
||||
live = self._live()
|
||||
argv = ["hermes", "dashboard", "--port", "8300"]
|
||||
unit_cgroup = "/system.slice/hosted-compute-agent.service"
|
||||
probes: list[list[str]] = []
|
||||
|
||||
def fake_probe(cmd, *, timeout):
|
||||
probes.append(list(cmd))
|
||||
out = main_pid if cmd[-2:] == ["--property=MainPID", "--value"] else ""
|
||||
return MagicMock(returncode=0, stdout=out, stderr="")
|
||||
|
||||
def fake_kill(pid, sig):
|
||||
if sig == 0:
|
||||
raise ProcessLookupError
|
||||
|
||||
with patch.object(main_dashboard, "_restart_managed_dashboard_service", return_value=False), \
|
||||
patch.object(live, "_find_stale_dashboard_pids", return_value=[4321]), \
|
||||
patch.object(main_dashboard, "_pid_unified_cgroup_entries", lambda pid: iter([unit_cgroup])), \
|
||||
patch.object(main_dashboard, "_run_probe", side_effect=fake_probe), \
|
||||
patch.object(main_dashboard, "_dashboard_cmdline_for_pid", return_value=argv), \
|
||||
patch("hermes_cli.dashboard_procs._hermes_home_for_pid", return_value=None), \
|
||||
patch.object(live, "_respawn_dashboard_processes", return_value=[]) as respawn, \
|
||||
patch("os.kill", side_effect=fake_kill), \
|
||||
patch("time.sleep"):
|
||||
result = _kill_stale_dashboard_processes(restart_managed=True)
|
||||
|
||||
restarts = [c for c in probes if "restart" in c]
|
||||
if restarted:
|
||||
assert restarts == [["systemctl", "restart", "hosted-compute-agent.service"]]
|
||||
respawn.assert_not_called()
|
||||
else:
|
||||
assert restarts == [], f"restarted a unit that does not supervise the dashboard: {restarts}"
|
||||
respawn.assert_called_once_with([argv])
|
||||
assert result["unrecovered"] == []
|
||||
|
||||
|
||||
class TestManualBackendRespawn:
|
||||
"""Manually-started dashboards/serves have their argv captured before the
|
||||
|
||||
Reference in New Issue
Block a user