diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index 7ba98bc252..ae8248057e 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -5549,7 +5549,6 @@ def _wait_for_launchd_service_pid( def launchd_restart(): label = get_launchd_label() target = f"{_launchd_domain()}/{label}" - drain_timeout = _get_restart_drain_timeout() from gateway.status import get_running_pid try: @@ -5573,26 +5572,43 @@ def launchd_restart(): _escalate_wedged_gateway(pid) pid = None if pid is not None: - # Announce the drain BEFORE waiting on it. This wait can run for - # the full drain budget (180s by default) while the old gateway - # finishes in-flight agent runs, and it streams into surfaces with - # no other feedback — the desktop updater's live output most of - # all, where a silent stop here reads as "update stuck" (#44515). - # Mirrors the systemd branch's "draining (up to Ns)..." line. + # Graceful in-band restart, mirroring the systemd branch. + # + # Previously this sent a bare SIGTERM and waited + # ``_get_restart_drain_timeout()`` — which defaults to 0, so the + # wait could never succeed and every restart fell through to + # ``kickstart -k``. A bare SIGTERM also leaves + # ``restart_requested`` False, so the gateway exits 1 instead of + # 75 and reports itself to chat as "shutting down" rather than + # "restarting", losing the resume_pending handoff. + # + # SIGUSR1 is the drain-aware path: refuse new turns, wait for + # in-flight work (``agent.restart_after_turn_timeout``), then + # stop() within ``agent.restart_drain_timeout``. The wait budget + # must cover BOTH phases plus headroom (#77184) — the raw drain + # timeout covers only the second. + # + # Announce the wait BEFORE it runs: it can last the full budget + # while the old gateway finishes in-flight agent runs, and it + # streams into surfaces with no other feedback — the desktop + # updater's live output most of all, where a silent stop here + # reads as "update stuck" (#44515). + wait_budget = _get_restart_exit_wait_budget() print( f"→ Stopping gateway (PID {pid}) — draining in-flight runs " - f"(up to {drain_timeout:.0f}s)..." + f"(up to {wait_budget:.0f}s)..." + ) + if _graceful_restart_via_sigusr1(pid, wait_budget): + # The gateway exited with the planned-restart code and + # launchd's unconditional KeepAlive revives it. Do NOT + # kickstart here: the replacement may already be up, and + # ``-k`` would kill it and restart a second time. + print("✓ Service restart requested") + _clear_launchd_unsupported_marker() + return + print( + f"⚠ Gateway drain timed out after {wait_budget:.0f}s — forcing launchd restart" ) - try: - terminate_pid(pid, force=False) - except (ProcessLookupError, PermissionError, OSError): - pid = None - if pid is not None: - exited = _wait_for_gateway_exit(timeout=drain_timeout, force_after=None) - if not exited: - print( - f"⚠ Gateway drain timed out after {drain_timeout:.0f}s — forcing launchd restart" - ) subprocess.run(["launchctl", "kickstart", "-k", target], check=True, timeout=90) print("✓ Service restarted") _clear_launchd_unsupported_marker() diff --git a/tests/hermes_cli/test_gateway_service.py b/tests/hermes_cli/test_gateway_service.py index 6864d151d6..f51d665eca 100644 --- a/tests/hermes_cli/test_gateway_service.py +++ b/tests/hermes_cli/test_gateway_service.py @@ -920,8 +920,60 @@ class TestGatewaySystemServiceRouting: assert result is False assert replacement_observed == [True] + def test_launchd_restart_uses_sigusr1_and_exit_wait_budget(self, monkeypatch, capsys): + """launchd_restart must take the same graceful path as systemd_restart. + Regression: it previously sent a bare SIGTERM and waited + ``_get_restart_drain_timeout()`` (default 0), so the wait could never + succeed and every restart fell through to ``kickstart -k``. A bare + SIGTERM leaves ``restart_requested`` False, so the gateway exits 1 + instead of 75 and announces itself as "shutting down" rather than + "restarting", dropping the resume_pending handoff. + """ + calls = [] + monkeypatch.setattr(gateway_cli, "get_launchd_label", lambda: "ai.hermes.gateway") + monkeypatch.setattr(gateway_cli, "_launchd_domain", lambda: "gui/501") + monkeypatch.setattr("gateway.status.get_running_pid", lambda *a, **k: 654) + monkeypatch.setattr(gateway_cli, "_request_gateway_self_restart", lambda pid: False) + monkeypatch.setattr( + gateway_cli, + "probe_gateway_loop_liveness", + lambda pid, **kw: gateway_cli.GATEWAY_LOOP_ALIVE, + ) + # Wait budget covers after-turn deferral + drain + headroom (#77184); + # the raw drain timeout (0 by default) must not be used here. + monkeypatch.setattr(gateway_cli, "_get_restart_drain_timeout", lambda: 0.0) + monkeypatch.setattr(gateway_cli, "_get_restart_exit_wait_budget", lambda: 27.0) + monkeypatch.setattr( + gateway_cli, + "_graceful_restart_via_sigusr1", + lambda pid, timeout: calls.append(("graceful", pid, timeout)) or True, + ) + monkeypatch.setattr( + gateway_cli, + "terminate_pid", + lambda pid, force=False: calls.append(("sigterm", pid)), + ) + monkeypatch.setattr( + gateway_cli.subprocess, + "run", + lambda *a, **k: calls.append(("kickstart", a[0])) or SimpleNamespace( + returncode=0, stdout="", stderr="" + ), + ) + monkeypatch.setattr(gateway_cli, "_clear_launchd_unsupported_marker", lambda: None) + + gateway_cli.launchd_restart() + + assert ("graceful", 654, 27.0) in calls + # A bare SIGTERM would strand the gateway on the unplanned-shutdown path. + assert not any(call[0] == "sigterm" for call in calls) + # ``-k`` after a successful graceful exit would kill the replacement. + assert not any(call[0] == "kickstart" for call in calls) + out = capsys.readouterr().out + assert "27" in out + assert "up to 0s" not in out diff --git a/tests/hermes_cli/test_update_wedged_gateway.py b/tests/hermes_cli/test_update_wedged_gateway.py index ab7e4733bd..9775bece76 100644 --- a/tests/hermes_cli/test_update_wedged_gateway.py +++ b/tests/hermes_cli/test_update_wedged_gateway.py @@ -193,6 +193,8 @@ class TestLaunchdRestartWedgedIntegration: monkeypatch.setattr(gateway_cli, "get_launchd_label", lambda: "ai.hermes.gateway") monkeypatch.setattr(gateway_cli, "_launchd_domain", lambda: "gui/501") monkeypatch.setattr(gateway_cli, "_get_restart_drain_timeout", lambda: 180.0) + # Wait budget covers after-turn deferral + drain + headroom (#77184). + monkeypatch.setattr(gateway_cli, "_get_restart_exit_wait_budget", lambda: 195.0) monkeypatch.setattr("gateway.status.get_running_pid", lambda *a, **k: 4242) monkeypatch.setattr( gateway_cli, "_request_gateway_self_restart", lambda pid: False @@ -212,10 +214,11 @@ class TestLaunchdRestartWedgedIntegration: "terminate_pid", lambda pid, force=False: events.append("sigterm"), ) + # Never let a real SIGUSR1 escape to PID 4242 during tests. monkeypatch.setattr( gateway_cli, - "_wait_for_gateway_exit", - lambda timeout, force_after=None: events.append(("drain", timeout)) or True, + "_graceful_restart_via_sigusr1", + lambda pid, timeout: events.append(("drain", pid, timeout)) or True, ) monkeypatch.setattr( gateway_cli.subprocess, @@ -241,11 +244,11 @@ class TestLaunchdRestartWedgedIntegration: events = self._setup(monkeypatch, gateway_cli.GATEWAY_LOOP_ALIVE) gateway_cli.launchd_restart() assert "escalate" not in events - assert ("drain", 180.0) in events + assert ("drain", 4242, 195.0) in events def test_unknown_liveness_keeps_full_drain_budget(self, monkeypatch): """Ambiguity (no heartbeat) must never trigger escalation.""" events = self._setup(monkeypatch, gateway_cli.GATEWAY_LOOP_UNKNOWN) gateway_cli.launchd_restart() assert "escalate" not in events - assert ("drain", 180.0) in events + assert ("drain", 4242, 195.0) in events