From 8872cd137c3cb72809319567f0808fc03280d43a Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:40:19 +0530 Subject: [PATCH] fix: verify launchd replacement PID before trusting KeepAlive (review follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A graceful SIGUSR1 exit alone doesn't prove supervision: detached-fallback gateways (macOS 26 unsupported-domain marker) and unloaded jobs also exit cleanly with nobody to revive them, and _graceful_restart_via_sigusr1 returns True for an already-gone PID — the CLI would print success while the gateway stayed down. Poll _wait_for_launchd_service_pid (15s) after a graceful exit and fall through to kickstart -k when no replacement appears, mirroring systemd_restart's replacement observation. Adds the no-replacement regression test and strengthens the budget assertion. --- hermes_cli/gateway.py | 34 +++++++++---- tests/hermes_cli/test_gateway_service.py | 61 +++++++++++++++++++++++- 2 files changed, 84 insertions(+), 11 deletions(-) diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index ae8248057e..c65f9fe2d1 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -5599,16 +5599,30 @@ def launchd_restart(): 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" - ) + # The gateway exited with the planned-restart code. When + # launchd is actually supervising this label, KeepAlive + # revives it — do NOT kickstart (the replacement may already + # be up, and ``-k`` would kill it and restart a second time). + # But a graceful exit alone doesn't prove supervision: + # detached-fallback gateways and unloaded jobs also exit + # cleanly with nobody to revive them (and the SIGUSR1 helper + # returns True for an already-gone PID). Verify a replacement + # PID appears before trusting KeepAlive — mirrors the systemd + # branch's replacement observation. + if _wait_for_launchd_service_pid( + label, pid, timeout=15.0, domain=_launchd_domain() + ): + print("✓ Service restart requested") + _clear_launchd_unsupported_marker() + return + print( + "⚠ launchd did not revive the gateway after its graceful " + "exit — forcing restart" + ) + else: + print( + f"⚠ Gateway drain timed out after {wait_budget:.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 f51d665eca..13137f0316 100644 --- a/tests/hermes_cli/test_gateway_service.py +++ b/tests/hermes_cli/test_gateway_service.py @@ -963,6 +963,15 @@ class TestGatewaySystemServiceRouting: ), ) monkeypatch.setattr(gateway_cli, "_clear_launchd_unsupported_marker", lambda: None) + # KeepAlive revives the label on a fresh PID — replacement observed. + monkeypatch.setattr( + gateway_cli, + "_wait_for_launchd_service_pid", + lambda label, old_pid, timeout=10.0, *, domain: calls.append( + ("observe", label, old_pid, domain) + ) + or True, + ) gateway_cli.launchd_restart() @@ -971,10 +980,60 @@ class TestGatewaySystemServiceRouting: 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) + # The success message must follow an observed replacement PID. + assert ("observe", "ai.hermes.gateway", 654, "gui/501") in calls out = capsys.readouterr().out - assert "27" in out + assert "up to 27s" in out assert "up to 0s" not in out + def test_launchd_restart_forces_kickstart_when_no_replacement_appears( + self, monkeypatch, capsys + ): + """A graceful exit with no KeepAlive revival must not report success. + + Detached-fallback gateways (macOS 26 unsupported-domain marker) and + unloaded jobs also exit cleanly on SIGUSR1, but nobody revives them — + and ``_graceful_restart_via_sigusr1`` returns True for an already-gone + PID. Without replacement observation the CLI would print + \"✓ Service restart requested\" while the gateway stays down. + """ + 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, + ) + monkeypatch.setattr(gateway_cli, "_get_restart_exit_wait_budget", lambda: 27.0) + monkeypatch.setattr( + gateway_cli, "_graceful_restart_via_sigusr1", lambda pid, timeout: True + ) + monkeypatch.setattr( + gateway_cli, + "_wait_for_launchd_service_pid", + lambda label, old_pid, timeout=10.0, *, domain: False, + ) + 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() + + # No replacement observed → must escalate to kickstart -k. + assert any(call[0] == "kickstart" for call in calls) + out = capsys.readouterr().out + assert "did not revive" in out + assert "✓ Service restarted" in out +