From 0fc14fc2e9dc691a4d4fd92696aa8ee7dbb5b004 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 6 Jul 2026 05:19:11 -0700 Subject: [PATCH] fix(gateway): drop --replace + Restart=on-failure in generated service units Under a process supervisor (systemd/launchd), --replace makes each supervised restart kill its predecessor, producing self-kill loops, and Restart=always revives even clean manual stops. Generated units now run plain 'gateway run' with Restart=on-failure (+RestartForceExitStatus=75 for drain-restarts); the Nix module default follows suit. --replace stays on the manual/detached fallback paths where no supervisor owns the lifecycle. --- hermes_cli/gateway.py | 16 ++++++----- nix/nixosModules.nix | 4 +-- tests/hermes_cli/test_gateway_service.py | 35 ++++++++++++++++++++---- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index b04652de8fb..b395e14341d 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -250,8 +250,11 @@ def _graceful_restart_via_sigusr1(pid: int, drain_timeout: float) -> bool: SIGUSR1 is wired in gateway/run.py to ``request_restart(via_service=True)`` which drains in-flight agent runs (up to ``agent.restart_drain_timeout`` - seconds), then exits. Both systemd (``Restart=always``) and launchd - (unconditional ``KeepAlive``) restart on any exit. + seconds), then exits with code 75. Systemd units generated by Hermes use + ``Restart=on-failure`` together with ``RestartForceExitStatus=75`` so the + service is relaunched after the graceful exit without reviving clean + ``--replace`` takeovers. launchd (unconditional ``KeepAlive + ``) restarts on any exit. This is the drain-aware alternative to ``systemctl restart`` / ``SIGTERM``, which SIGKILL in-flight agents after a short timeout. @@ -652,7 +655,7 @@ def _gateway_run_args_for_profile(profile: str) -> list[str]: args = [get_python_path(), "-m", "hermes_cli.main"] if profile != "default": args.extend(["--profile", profile]) - args.extend(["gateway", "run", "--replace"]) + args.extend(["gateway", "run"]) return args @@ -2729,7 +2732,7 @@ Environment="LOGNAME={username}" Environment="PATH={sane_path}" Environment="VIRTUAL_ENV={venv_dir}" Environment="HERMES_HOME={hermes_home}" -Restart=always +Restart=on-failure RestartSec=5 RestartForceExitStatus={GATEWAY_SERVICE_RESTART_EXIT_CODE} KillMode=mixed @@ -2763,7 +2766,7 @@ WorkingDirectory={working_dir} Environment="PATH={sane_path}" Environment="VIRTUAL_ENV={venv_dir}" Environment="HERMES_HOME={hermes_home}" -Restart=always +Restart=on-failure RestartSec=5 RestartForceExitStatus={GATEWAY_SERVICE_RESTART_EXIT_CODE} KillMode=mixed @@ -3852,7 +3855,6 @@ def generate_launchd_plist() -> str: [ "gateway", "run", - "--replace", ] ) prog_args_xml = "\n ".join(prog_args) @@ -4705,7 +4707,7 @@ def run_gateway(verbose: int = 0, quiet: bool = False, replace: bool = False, fo print() # Exit with code 1 if gateway fails to connect any platform, - # so systemd Restart=always will retry on transient errors + # so systemd Restart=on-failure will retry on transient errors verbosity = None if quiet else verbose # ── Exit-path diagnostics ──────────────────────────────────────────── diff --git a/nix/nixosModules.nix b/nix/nixosModules.nix index 4458926b0f0..42e14bd01c2 100644 --- a/nix/nixosModules.nix +++ b/nix/nixosModules.nix @@ -541,7 +541,7 @@ restart = mkOption { type = types.str; - default = "always"; + default = "on-failure"; description = "systemd Restart= policy."; }; @@ -983,7 +983,7 @@ --env MESSAGING_CWD=${containerWorkDir} \ ${lib.concatStringsSep " " cfg.container.extraOptions} \ ${cfg.container.image} \ - ${containerDataDir}/current-package/bin/hermes gateway run --replace ${lib.concatStringsSep " " cfg.extraArgs} + ${containerDataDir}/current-package/bin/hermes gateway run ${lib.concatStringsSep " " cfg.extraArgs} echo "${containerIdentity}" > ${identityFile} fi diff --git a/tests/hermes_cli/test_gateway_service.py b/tests/hermes_cli/test_gateway_service.py index c2d03625dc3..d20e5051370 100644 --- a/tests/hermes_cli/test_gateway_service.py +++ b/tests/hermes_cli/test_gateway_service.py @@ -426,6 +426,8 @@ class TestGeneratedSystemdUnits: assert "ExecStart=" in unit assert "ExecStop=" not in unit assert "ExecReload=/bin/kill -USR1 $MAINPID" in unit + assert "Restart=on-failure" in unit + assert "Restart=always" not in unit assert f"RestartForceExitStatus={GATEWAY_SERVICE_RESTART_EXIT_CODE}" in unit # TimeoutStopSec must exceed the default drain_timeout (60s) so # systemd doesn't SIGKILL the cgroup before post-interrupt cleanup @@ -537,6 +539,8 @@ class TestGeneratedSystemdUnits: assert "ExecStart=" in unit assert "ExecStop=" not in unit assert "ExecReload=/bin/kill -USR1 $MAINPID" in unit + assert "Restart=on-failure" in unit + assert "Restart=always" not in unit assert f"RestartForceExitStatus={GATEWAY_SERVICE_RESTART_EXIT_CODE}" in unit # TimeoutStopSec must exceed the default drain_timeout (60s) so # systemd doesn't SIGKILL the cgroup before post-interrupt cleanup @@ -664,7 +668,10 @@ class TestLaunchdServiceRecovery: label = gateway_cli.get_launchd_label() domain = gateway_cli._launchd_domain() - assert "--replace" in plist_path.read_text(encoding="utf-8") + plist_text = plist_path.read_text(encoding="utf-8") + # The generator is stubbed above; assert the repair path rewrote the + # plist with the (synthetic) generator output. + assert plist_text == gateway_cli.generate_launchd_plist() # The calls list includes launchctl print probes from _launchd_domain() # before the bootout/bootstrap calls. Filter to only bootout/bootstrap. service_calls = [c for c in calls if "bootout" in c or "bootstrap" in c] @@ -2412,10 +2419,10 @@ class TestProfileArg: unit = gateway_cli.generate_systemd_unit(system=False) assert "--profile mybot" in unit assert "gateway run" in unit - # Under a process supervisor (Restart=always), --replace makes each - # restart kill its predecessor → self-kill loop. The systemd unit must - # NOT use --replace; the supervisor owns the lifecycle. (--replace stays - # on the manual launchd fallback path — see test_launchd_plist_includes_profile.) + # Under a process supervisor, --replace makes each restart kill its + # predecessor → self-kill loop. Neither the systemd unit nor the + # launchd plist may use --replace; the supervisor owns the lifecycle. + # (--replace stays on the manual/detached fallback paths only.) assert "--replace" not in unit def test_systemd_unit_for_target_user_includes_named_profile(self, tmp_path, monkeypatch): @@ -2450,6 +2457,24 @@ class TestProfileArg: plist = gateway_cli.generate_launchd_plist() assert "--profile" in plist assert "mybot" in plist + assert "--replace" not in plist + + def test_gateway_run_args_for_profile_omit_replace(self, monkeypatch): + monkeypatch.setattr(gateway_cli, "get_python_path", lambda: "/venv/bin/python") + + default_args = gateway_cli._gateway_run_args_for_profile("default") + named_args = gateway_cli._gateway_run_args_for_profile("mybot") + + assert default_args == ["/venv/bin/python", "-m", "hermes_cli.main", "gateway", "run"] + assert named_args == [ + "/venv/bin/python", + "-m", + "hermes_cli.main", + "--profile", + "mybot", + "gateway", + "run", + ] def test_launchd_plist_supports_aqua_and_background_sessions(self): # macOS 26+ only loads the agent in non-Aqua sessions when the plist