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.
This commit is contained in:
teknium1
@@ -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/<profile>/` 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`)
|
||||
|
||||
|
||||
@@ -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")
|
||||
|
||||
100
hermes_cli/gateway_supervised_restart.py
Normal file
100
hermes_cli/gateway_supervised_restart.py
Normal file
@@ -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 <restart>)" 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)
|
||||
@@ -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 <restart>)" — 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 <restart>)" — 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"
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user