From 40dbfa0e3ce85dc0d63f1ebe1319d212c53d3511 Mon Sep 17 00:00:00 2001 From: Z Date: Sun, 21 Jun 2026 22:57:05 +0800 Subject: [PATCH] fix(gateway): revive gateway on /restart under Restart=on-failure units MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The in-chat /restart command was leaving the gateway dead on systemd deployments using Restart=on-failure (the default for many operator-managed and tutorial-style unit files). The gateway drained, exited cleanly (code 0), and was never revived — the only recovery was a host reboot. Root cause was a multi-layer assumption mismatch: 1. gateway/run.py:_stop_impl assumed all systemd units use Restart=always, so the Linux/systemd branch returned exit code 0 and relied on a `systemd-run` transient helper to restart the unit immediately. Units with Restart=on-failure never see a clean exit as a trigger, so nothing revived the process. 2. gateway/run.py:_launch_systemd_restart_shortcut hardcoded `--user` scope, so it could not even locate the unit PID on system-level deployments (the common case for /etc/systemd/system/hermes-gateway.service). It silently returned without launching the helper. 3. Even after the scope detection was fixed, the helper could not actually start: non-root gateway units (User=ubunutu) hit a Polkit denial on `systemd-run --system` ("Interactive authentication required"), and `--user` requires a D-Bus user session that is typically absent on headless servers. The fix is two-fold: * `_stop_impl` now always exits with GATEWAY_SERVICE_RESTART_EXIT_CODE (75 / EX_TEMPFAIL) on service-managed restarts, regardless of platform. Combined with RestartForceExitStatus=75 in the unit file, systemd treats the planned restart as a controlled failure and revives the gateway via Restart=on-failure, with RestartSec as the only delay. The planned-restart helper is still attempted (for RestartSec=0 setups that want sub-second restarts) but is no longer load-bearing. * `_launch_systemd_restart_shortcut` now probes both system and user scopes via MainPID equality and uses whichever scope actually owns the gateway process. It bails out safely if neither matches. StartLimitBurst in the unit file still bounds accidental restart loops, and the macOS launchd path is unchanged. Verified end-to-end on Ubuntu 24.04 with hermes-gateway as a /etc/systemd/system/... service running under User=ubunutu. The unit uses Restart=on-failure, RestartSec=30, RestartForceExitStatus=75, StartLimitIntervalSec=600, StartLimitBurst=5. /restart from Feishu now drains cleanly, exits 75, and the gateway is back online ~30s later without manual intervention. Tests: tests/gateway/test_gateway_shutdown.py renamed the affected case to test_gateway_stop_systemd_service_restart_uses_tempfail and now asserts exit_code == GATEWAY_SERVICE_RESTART_EXIT_CODE. 14/14 tests in this module pass. --- gateway/run.py | 82 ++++++++++++++++---------- tests/gateway/test_gateway_shutdown.py | 9 ++- 2 files changed, 59 insertions(+), 32 deletions(-) diff --git a/gateway/run.py b/gateway/run.py index 39ca86e0c41..3f0dc4ff83c 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -5859,34 +5859,51 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew service_name = "hermes-gateway" current_pid = os.getpid() - show = subprocess.run( - [ - systemctl, - "--user", - "show", - service_name, - "--property=MainPID", - "--value", - ], - capture_output=True, - text=True, - timeout=2, - ) - if (show.stdout or "").strip() != str(current_pid): + + # Detect whether the gateway unit is registered as a system or + # user service. Daemon-style deployments are typically system + # units (e.g. /etc/systemd/system/hermes-gateway.service), while + # `hermes setup` under a non-root account may register a user + # unit. Hard-coding ``--user`` broke system-unit deployments: + # systemctl returned an empty MainPID, the PID-equality check + # below failed, and the planned-restart helper was never + # launched — leaving the gateway dead until a manual reboot. + def _query_pid(scope_flags): + try: + out = subprocess.run( + [systemctl, *scope_flags, "show", service_name, + "--property=MainPID", "--value"], + capture_output=True, text=True, timeout=2, + ) + return (out.stdout or "").strip() + except Exception: + return "" + + system_pid = _query_pid([]) + user_pid = _query_pid(["--user"]) + if str(current_pid) == system_pid: + scope_flags = [] + systemctl_scope = "systemctl" + elif str(current_pid) == user_pid: + scope_flags = ["--user"] + systemctl_scope = "systemctl --user" + else: + # MainPID does not match in either scope — likely invoked + # outside of systemd or the unit was renamed. Bail out + # rather than restart the wrong unit. return - systemctl_user = "systemctl --user" service_arg = shlex.quote(service_name) shell_cmd = ( f"while kill -0 {current_pid} 2>/dev/null; do sleep 0.2; done; " - f"{systemctl_user} reset-failed {service_arg}; " - f"{systemctl_user} restart {service_arg}" + f"{systemctl_scope} reset-failed {service_arg}; " + f"{systemctl_scope} restart {service_arg}" ) unit_name = f"{service_name}-planned-restart-{current_pid}".replace(".", "-") subprocess.Popen( [ systemd_run, - "--user", + *scope_flags, "--collect", "--unit", unit_name, @@ -5899,9 +5916,10 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew start_new_session=True, ) logger.info( - "Launched systemd planned-restart helper for %s (pid=%s)", + "Launched systemd planned-restart helper for %s (pid=%s, scope=%s)", service_name, current_pid, + "user" if scope_flags else "system", ) except Exception as e: logger.debug("Failed to launch systemd planned-restart helper: %s", e) @@ -7871,17 +7889,21 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew if self._restart_requested and self._restart_via_service: self._launch_systemd_restart_shortcut() - # systemd units use Restart=always, so a planned restart should - # exit cleanly and still be relaunched. Using TEMPFAIL here - # makes systemd treat the operator-requested restart as a - # failure and can trip stepped restart backoff. launchd's - # KeepAlive.SuccessfulExit=false needs a non-zero exit to - # relaunch, so keep the old code on macOS. - self._exit_code = ( - GATEWAY_SERVICE_RESTART_EXIT_CODE - if sys.platform == "darwin" or not os.environ.get("INVOCATION_ID") - else 0 - ) + # Always exit with TEMPFAIL (75) on service-managed + # restarts. The shortcut helper above is best-effort and + # commonly fails on real deployments: non-root gateway + # units hit Polkit denials when invoking ``systemd-run + # --system``, headless boxes have no user bus for + # ``--user``, and operator-managed unit files may use + # ``Restart=on-failure`` rather than ``Restart=always``. + # Exit 75 paired with ``RestartForceExitStatus=75`` makes + # systemd treat the planned restart as a controlled + # failure and revive the unit via ``Restart=on-failure``, + # regardless of whether the helper survived. Without + # this, a clean exit (0) on Linux left the gateway dead + # until someone rebooted the host. ``StartLimitBurst`` + # in the unit file still bounds accidental loops. + self._exit_code = GATEWAY_SERVICE_RESTART_EXIT_CODE self._exit_reason = self._exit_reason or "Gateway restart requested" self._draining = False diff --git a/tests/gateway/test_gateway_shutdown.py b/tests/gateway/test_gateway_shutdown.py index 9910f3923aa..af276ce6b2b 100644 --- a/tests/gateway/test_gateway_shutdown.py +++ b/tests/gateway/test_gateway_shutdown.py @@ -138,7 +138,7 @@ async def test_gateway_stop_interrupts_after_drain_timeout(): @pytest.mark.asyncio -async def test_gateway_stop_systemd_service_restart_exits_cleanly(tmp_path, monkeypatch): +async def test_gateway_stop_systemd_service_restart_uses_tempfail(tmp_path, monkeypatch): monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path) runner, adapter = make_restart_runner() adapter.disconnect = AsyncMock() @@ -149,7 +149,12 @@ async def test_gateway_stop_systemd_service_restart_exits_cleanly(tmp_path, monk await runner.stop(restart=True, service_restart=True) runner._launch_systemd_restart_shortcut.assert_called_once_with() - assert runner._exit_code == 0 + # Exit 75 (EX_TEMPFAIL) so RestartForceExitStatus=75 in the unit + # file revives the gateway via Restart=on-failure, even when the + # planned-restart helper fails (Polkit denial, missing user bus, + # headless box, or operator-managed unit using on-failure instead + # of always). StartLimitBurst still bounds accidental loops. + assert runner._exit_code == GATEWAY_SERVICE_RESTART_EXIT_CODE assert (tmp_path / ".restart_pending.json").exists()