From 8731bb91f57e31edc7690f107eb3a3ccfe49fd82 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 14 Sep 2026 18:01:37 -0700 Subject: [PATCH] refactor(gateway): move the supervised-restart handback into gateway_supervised_restart.py The handback logic was appended to the hermes_cli/gateway.py facade; it now lives in a topical sibling. Supervisor detection also reads the gateway's own declaration (control socket `identify` -> supervisor: "external", then the live argv marker, then the argv the gateway stamped into gateway_state.json) so a gateway whose command line cannot be read via psutil is still handed back rather than SIGTERMed and shadowed by a foreground run. Tests trimmed to the two invariants (handback with fresh-PID success; either failure branch never takes ownership) plus the plain-manual control. Docs: `hermes gateway restart` is now part of the --external-supervisor contract. --- hermes_cli/AGENTS.md | 5 + hermes_cli/gateway.py | 74 ++----------- hermes_cli/gateway_supervised_restart.py | 100 ++++++++++++++++++ ...est_gateway_restart_external_supervisor.py | 77 +++++--------- website/docs/reference/cli-commands.md | 6 +- 5 files changed, 142 insertions(+), 120 deletions(-) create mode 100644 hermes_cli/gateway_supervised_restart.py diff --git a/hermes_cli/AGENTS.md b/hermes_cli/AGENTS.md index 20eaeabbad..ac36c5f3cd 100644 --- a/hermes_cli/AGENTS.md +++ b/hermes_cli/AGENTS.md @@ -155,6 +155,11 @@ other service domain / UNIX user / HERMES_HOME outside `profiles/` — notices f blockers for the hook) and the `gateway.auto_multiplex_migration` opt-out (#109954). Blockers reuse `GatewayRunner._adapter_credential_fingerprint` and `platform_binds_port`; "has a `/p//` ingress" is the adapter class attribute `serves_profile_prefix` — set it on a new HTTP-inbound adapter when it answers the prefix, never extend a list here. +`hermes gateway restart` for a gateway Hermes did not install (custom launchd agent / unit running +`gateway run --external-supervisor`): `gateway_supervised_restart.py` — the gateway's SELF-declared +supervisor (control-socket `identify`, then the argv marker) decides; hand back via SIGUSR1 and wait +for a fresh supervised PID, never stop + foreground `run_gateway` (that stamps the CLI's PID and wedges +every KeepAlive respawn, #110637). ## Nous free tier (`hermes_cli/anon_auth.py`) diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index c6080e8c1a..39a6ef0ab8 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -6262,36 +6262,6 @@ def _restart_all(system: bool) -> None: _service_call(kind, "start", system) -def _wait_for_supervised_gateway_replacement( - old_pid: int, timeout: float | None = None, *, poll_interval: float = 0.5 -) -> int | None: - """Poll the pidfile until the supervisor's replacement gateway registers a fresh PID. - - A graceful SIGUSR1 exit only proves the old process left — an unloaded, broken, or - stopped-retrying supervisor leaves the gateway down. Custom-supervisor counterpart of - ``_wait_for_launchd_service_pid``: the label is invisible to launchctl queries, so - identity comes from ``get_running_pid``'s lock+PID liveness verification and freshness - from ``!= old_pid``. Returns the fresh PID, or None once ``timeout`` passes. - """ - from gateway.status import get_running_pid - - if timeout is None: - timeout = SUPERVISED_REPLACEMENT_VERIFY_TIMEOUT - deadline = time.monotonic() + max(timeout, 0.5) - while True: - pid = get_running_pid() - if pid is not None and pid > 0 and pid != old_pid: - return pid - if time.monotonic() >= deadline: - return None - time.sleep(poll_interval) - - -# A custom KeepAlive supervisor keeps its own respawn interval (launchd's is ~once per 10s, -# per LAUNCHD_SUPERVISION_VERIFY_TIMEOUT); 15s matches _wait_for_launchd_service_pid's budget. -SUPERVISED_REPLACEMENT_VERIFY_TIMEOUT = 15.0 - - def _cmd_restart(args): _refuse_from_inside_gateway("restart", "restart loops") system = getattr(args, "system", False) @@ -6340,43 +6310,17 @@ def _cmd_restart(args): ) sys.exit(1) - # An externally-supervised gateway (custom launchd agent running `gateway run - # --external-supervisor`) must restart by exiting back to its supervisor. A foreground - # run here stamps this CLI's PID as the gateway; every KeepAlive respawn then refuses - # with "Gateway already running" and the gateway stays down until the restart process - # is killed (#110637). Same argv marker the update path trusts - # (_prepare_profile_gateway_update_restart). + # A gateway that declares an external supervisor (custom launchd agent / unit running + # `gateway run --external-supervisor`) restarts by exiting back to it: the stop + foreground + # run below would stamp this CLI's PID as the gateway and wedge every respawn (#110637). from gateway.status import get_running_pid + from hermes_cli.gateway_supervised_restart import ( + gateway_declares_external_supervisor, restart_externally_supervised_gateway, + ) supervised_pid = get_running_pid() - supervised_argv = _capture_gateway_argv(supervised_pid) if supervised_pid else None - if supervised_argv and "--external-supervisor" in supervised_argv: - wait_budget = _get_restart_exit_wait_budget() - print(f"→ Restarting externally-supervised gateway (PID {supervised_pid}) — " - f"draining in-flight runs (up to {wait_budget:.0f}s)...") - if _graceful_restart_via_sigusr1(supervised_pid, wait_budget): - # A clean exit doesn't prove supervision — the marker lives in argv, not in a - # loaded job. launchd_restart verifies a replacement PID for the same reason: - # a broken/unloaded supervisor must surface as a failed restart, never a - # success printed over a dead gateway. - replacement_pid = _wait_for_supervised_gateway_replacement(supervised_pid) - if replacement_pid is not None: - print() - print(f"✓ Gateway relaunched by its supervisor (PID {replacement_pid})") - return - print("⚠ Supervisor did not relaunch the gateway after its graceful exit") - else: - print(f"⚠ Gateway drain timed out after {wait_budget:.0f}s") - # The supervisor stays the sole restart owner: falling through to stop + foreground - # run would SIGTERM a KeepAlive-armed process and stamp this CLI's PID, so every - # supervisor respawn then refuses with "Gateway already running" (#110637) — the - # exact wedge this handback exists to prevent. Fail loudly and leave ownership be. - _print_lines( - "", - "✗ Not stopping or foreground-running a supervisor-owned gateway.", - " Check the supervisor (it may be unloaded, wedged, or stopped retrying),", - " then rerun once it is healthy: hermes gateway restart", - ) - sys.exit(1) + if supervised_pid and gateway_declares_external_supervisor(supervised_pid): + restart_externally_supervised_gateway(supervised_pid) + return if stop_profile_gateway(): print("✓ Stopped gateway for this profile") diff --git a/hermes_cli/gateway_supervised_restart.py b/hermes_cli/gateway_supervised_restart.py new file mode 100644 index 0000000000..c7823ab5c4 --- /dev/null +++ b/hermes_cli/gateway_supervised_restart.py @@ -0,0 +1,100 @@ +"""``hermes gateway restart`` for a gateway whose supervisor Hermes did not install. + +A custom launchd agent / systemd unit / any KeepAlive-style manager running ``gateway run +--external-supervisor`` owns the respawn. The manual fallback in ``_cmd_restart`` (SIGTERM, then a +foreground ``run_gateway`` inside the restart CLI) stamps the CLI's own PID as the gateway, so every +supervisor respawn refuses with "Gateway already running (PID )" and the gateway stays +down until the restart process is killed (#110637). The gateway must instead exit back to its +supervisor (SIGUSR1 drain), and success is a fresh supervised PID — never the bare exit. +""" + +from __future__ import annotations + +import sys +import time +from pathlib import Path + +# A custom KeepAlive supervisor keeps its own respawn interval (launchd's is ~once per 10s, +# per LAUNCHD_SUPERVISION_VERIFY_TIMEOUT); 15s matches _wait_for_launchd_service_pid's budget. +SUPERVISED_REPLACEMENT_VERIFY_TIMEOUT = 15.0 + + +def gateway_declares_external_supervisor(pid: int, home: Path | None = None) -> bool: + """True when the running gateway ``pid`` was launched for an external supervisor. + + The supervisor is SELF-declared by the gateway from its launch context: the control socket + ``identify`` answer (``supervisor: "external"``), else the ``--external-supervisor`` argv marker + read live (same marker ``_prepare_profile_gateway_update_restart`` trusts), else the argv the + gateway stamped into ``gateway_state.json`` when psutil cannot read the live command line. + """ + if not pid or pid <= 1: + return False + from gateway.control_socket import identify_gateway + from gateway.status import _get_process_hermes_home, read_runtime_status + from hermes_cli.gateway import _capture_gateway_argv + + home = home or _get_process_hermes_home() + identity = identify_gateway(home) or {} + if identity.get("pid") == pid and identity.get("supervisor"): + return identity.get("supervisor") == "external" + argv = _capture_gateway_argv(pid) + if argv is None: + record = read_runtime_status(home / "gateway_state.json") or {} + argv = record.get("argv") if record.get("pid") == pid else None + return bool(argv) and "--external-supervisor" in argv + + +def _wait_for_supervised_gateway_replacement( + old_pid: int, timeout: float | None = None, *, poll_interval: float = 0.5 +) -> int | None: + """Poll the pidfile until the supervisor's replacement gateway registers a fresh PID. + + A graceful SIGUSR1 exit only proves the old process left — an unloaded, broken, or + stopped-retrying supervisor leaves the gateway down. Custom-supervisor counterpart of + ``_wait_for_launchd_service_pid``: the label is invisible to launchctl queries, so identity + comes from ``get_running_pid``'s lock+PID liveness verification and freshness from ``!= old_pid``. + Returns the fresh PID, or None once ``timeout`` passes. + """ + from gateway.status import get_running_pid + + if timeout is None: + timeout = SUPERVISED_REPLACEMENT_VERIFY_TIMEOUT + deadline = time.monotonic() + max(timeout, 0.5) + while True: + pid = get_running_pid() + if pid is not None and pid > 0 and pid != old_pid: + return pid + if time.monotonic() >= deadline: + return None + time.sleep(poll_interval) + + +def restart_externally_supervised_gateway(supervised_pid: int) -> None: + """Hand ``supervised_pid`` back to its supervisor (SIGUSR1 drain) and report the fresh PID. + + Never falls through to SIGTERM + foreground run on either failure branch: that would + SIGTERM a KeepAlive-armed process and stamp this CLI's PID, recreating the competing-owner + wedge (#110637). A broken/unloaded supervisor surfaces as exit 1, not a success printed over + a dead gateway (the contract ``launchd_restart`` enforces via ``_wait_for_launchd_service_pid``). + """ + from hermes_cli.gateway import _get_restart_exit_wait_budget, _graceful_restart_via_sigusr1, _print_lines + + wait_budget = _get_restart_exit_wait_budget() + print(f"→ Restarting externally-supervised gateway (PID {supervised_pid}) — " + f"draining in-flight runs (up to {wait_budget:.0f}s)...") + if _graceful_restart_via_sigusr1(supervised_pid, wait_budget): + replacement_pid = _wait_for_supervised_gateway_replacement(supervised_pid) + if replacement_pid is not None: + print() + print(f"✓ Gateway relaunched by its supervisor (PID {replacement_pid})") + return + print("⚠ Supervisor did not relaunch the gateway after its graceful exit") + else: + print(f"⚠ Gateway drain timed out after {wait_budget:.0f}s") + _print_lines( + "", + "✗ Not stopping or foreground-running a supervisor-owned gateway.", + " Check the supervisor (it may be unloaded, wedged, or stopped retrying),", + " then rerun once it is healthy: hermes gateway restart", + ) + sys.exit(1) diff --git a/tests/hermes_cli/test_gateway_restart_external_supervisor.py b/tests/hermes_cli/test_gateway_restart_external_supervisor.py index c797736735..6cfafcec2d 100644 --- a/tests/hermes_cli/test_gateway_restart_external_supervisor.py +++ b/tests/hermes_cli/test_gateway_restart_external_supervisor.py @@ -2,19 +2,10 @@ A custom launchd agent (a plist/label outside the canonical ``ai.hermes.gateway`` path, so ``_installed_service_kind_for`` returns None) fell through to the manual stop + foreground -``run_gateway`` fallback. The foreground run stamps the restart CLI's own PID into -gateway.pid, and every KeepAlive respawn of ``gateway run --external-supervisor`` then refuses -with "Gateway already running (PID )" — the gateway stays down until the restart -process is killed (#110637). The fix trusts the same ``--external-supervisor`` argv marker -the update path uses (``_prepare_profile_gateway_update_restart``) and SIGUSR1s the gateway -so it drains, exits, and lets the supervisor relaunch it. - -The handback keeps the supervisor the sole restart owner on both lifecycle branches: a -graceful exit only reports success after a fresh, pidfile-verified replacement PID appears -(a broken/unloaded supervisor must fail loudly, not print success over a dead gateway — the -same contract ``launchd_restart`` enforces via ``_wait_for_launchd_service_pid``), and a -drain timeout never falls back to SIGTERM + foreground run (that fallback recreates the -competing-owner wedge this fix exists to remove). +``run_gateway`` fallback. The foreground run stamps the restart CLI's own PID into gateway.pid, +and every KeepAlive respawn of ``gateway run --external-supervisor`` then refuses with +"Gateway already running (PID )" — the gateway stays down until the restart process is +killed (#110637). """ from types import SimpleNamespace @@ -22,6 +13,7 @@ import pytest from gateway import status as gateway_status from hermes_cli import gateway as gw +from hermes_cli import gateway_supervised_restart as supervised SUPERVISED_ARGV = [ "/usr/bin/python", "-m", "hermes_cli.main", "gateway", "run", "--external-supervisor", @@ -31,16 +23,19 @@ SUPERVISED_ARGV = [ @pytest.fixture def restart_calls(monkeypatch): """Drive ``_cmd_restart`` to the manual fallback (no service kind) and record the exits.""" - calls = {"sigusr1": None, "stopped": False, "started": False} - replacement = {"pid": 5555} + calls = {"sigusr1": None, "stopped": False, "started": False, "sigusr1_returns": True, "replacement": 5555} def _running_pid(*a, **k): - # The supervised gateway holds the pidfile until the drain completes; after a - # graceful exit only the supervisor's replacement can register a fresh PID. + # The supervised gateway holds the pidfile until the drain completes; after a graceful + # exit only the supervisor's replacement can register a fresh PID. if calls["sigusr1"] is not None and calls["sigusr1_returns"]: - return replacement["pid"] + return calls["replacement"] return 4321 + def _sigusr1(pid, budget, **k): + calls["sigusr1"] = (pid, budget) + return calls["sigusr1_returns"] + monkeypatch.setattr(gw, "_refuse_from_inside_gateway", lambda *a, **k: None) monkeypatch.setattr(gw, "_guard_named_profile_under_multiplexer", lambda **k: None) monkeypatch.setattr(gw, "_dispatch_via_service_manager_if_s6", lambda *a, **k: False) @@ -48,18 +43,12 @@ def restart_calls(monkeypatch): monkeypatch.setattr(gw, "_get_restart_exit_wait_budget", lambda: 7.0) monkeypatch.setattr(gateway_status, "get_running_pid", _running_pid) monkeypatch.setattr(gw, "_capture_gateway_argv", lambda pid: SUPERVISED_ARGV if pid == 4321 else None) - monkeypatch.setattr(gw, "SUPERVISED_REPLACEMENT_VERIFY_TIMEOUT", 0.5) - - def _sigusr1(pid, budget, **k): - calls["sigusr1"] = (pid, budget) - return calls["sigusr1_returns"] - + monkeypatch.setattr("gateway.control_socket.identify_gateway", lambda home, **k: None) + monkeypatch.setattr(supervised, "SUPERVISED_REPLACEMENT_VERIFY_TIMEOUT", 0.5) monkeypatch.setattr(gw, "_graceful_restart_via_sigusr1", _sigusr1) monkeypatch.setattr(gw, "stop_profile_gateway", lambda: calls.__setitem__("stopped", True) or True) monkeypatch.setattr(gw, "_wait_for_gateway_exit", lambda **k: None) monkeypatch.setattr(gw, "run_gateway", lambda **k: calls.__setitem__("started", True)) - calls["sigusr1_returns"] = True - calls["_replacement"] = replacement return calls @@ -73,31 +62,22 @@ def test_external_supervisor_gateway_restarts_via_sigusr1_handback(restart_calls assert not restart_calls["stopped"], "the CLI must not SIGTERM a supervisor-owned gateway" assert not restart_calls["started"], "a foreground run would stamp the CLI PID and wedge respawns" out = capsys.readouterr().out - assert "relaunched by its supervisor" in out, ( - "success must ride on the verified replacement, not the bare exit" + assert "relaunched by its supervisor" in out and "5555" in out, ( + "success must ride on the verified replacement PID, not the bare exit" ) - assert "5555" in out, "the fresh supervisor PID is the success evidence" -def test_graceful_exit_without_replacement_fails_loud(restart_calls, capsys): - # An unloaded/broken supervisor never relaunches: the bare exit must not read as success. - restart_calls["_replacement"]["pid"] = None - with pytest.raises(SystemExit) as exc: - _run_restart() - assert exc.value.code == 1 - assert not restart_calls["stopped"] and not restart_calls["started"], \ - "the CLI must not take restart ownership from a broken supervisor either" - assert "did not relaunch" in capsys.readouterr().out - - -def test_drain_timeout_fails_loud_without_taking_ownership(restart_calls, capsys): - restart_calls["sigusr1_returns"] = False +@pytest.mark.parametrize("sigusr1_returns, replacement", [(True, None), (False, 5555)]) +def test_handback_failure_never_takes_ownership(restart_calls, sigusr1_returns, replacement): + # An unloaded supervisor (clean exit, no replacement) or a drain timeout must fail loudly; + # SIGTERM + foreground run would recreate the competing-owner wedge (#110637). + restart_calls["sigusr1_returns"] = sigusr1_returns + restart_calls["replacement"] = replacement with pytest.raises(SystemExit) as exc: _run_restart() assert exc.value.code == 1 assert restart_calls["sigusr1"] == (4321, 7.0), "the graceful handback must be attempted first" - assert not restart_calls["stopped"] and not restart_calls["started"], \ - "SIGTERM + foreground run would recreate the competing-owner wedge (#110637)" + assert not restart_calls["stopped"] and not restart_calls["started"] def test_plain_manual_gateway_still_uses_stop_and_run(restart_calls, monkeypatch): @@ -105,12 +85,3 @@ def test_plain_manual_gateway_still_uses_stop_and_run(restart_calls, monkeypatch _run_restart() assert restart_calls["sigusr1"] is None, "no supervisor marker: the detached fallback is the restart" assert restart_calls["started"], "a plain manually-run gateway must still be restarted in-process" - - -@pytest.mark.parametrize("pid,argv", [(None, None), (4321, None)]) -def test_uncapturable_gateway_still_uses_stop_and_run(restart_calls, monkeypatch, pid, argv): - monkeypatch.setattr(gateway_status, "get_running_pid", lambda *a, **k: pid) - monkeypatch.setattr(gw, "_capture_gateway_argv", lambda p: argv) - _run_restart() - assert restart_calls["sigusr1"] is None - assert restart_calls["started"], "capture failure must degrade to the existing fallback, never a silent no-op" diff --git a/website/docs/reference/cli-commands.md b/website/docs/reference/cli-commands.md index 5e8b567d98..8e4181da81 100644 --- a/website/docs/reference/cli-commands.md +++ b/website/docs/reference/cli-commands.md @@ -313,8 +313,10 @@ Options: | `--no-supervise` | On `run`: inside the s6-overlay Docker image, opt out of auto-supervision and use pre-s6 foreground semantics — gateway runs as the container's main process with no auto-restart. No-op outside the s6 image. Equivalent to setting `HERMES_GATEWAY_NO_SUPERVISE=1`. | | `--external-supervisor` | On `run`: declare that a wrapper-provided process manager owns the foreground gateway. Use this when `sudo`, `env -i`, or another wrapper strips launchd/systemd's native environment marker. In-chat restarts and updates exit back to that manager instead of spawning a detached replacement. | -`--external-supervisor` is a restart-policy contract: an in-chat restart or -service-restart update exits with status `75`, so the wrapper's supervisor must +`--external-supervisor` is a restart-policy contract: an in-chat restart, +`hermes gateway restart`, or service-restart update exits with status `75` +(the CLI then waits for the supervisor's fresh PID instead of running a +foreground gateway of its own), so the wrapper's supervisor must relaunch the gateway after that nonzero exit. For systemd, use `Restart=on-failure` or `Restart=always` and do not include `75` in `RestartPreventExitStatus`; for launchd, configure `KeepAlive` to relaunch after