From 65b7151dbd95767d0d203711216e43b0aa43113f Mon Sep 17 00:00:00 2001
From: Rob Hilgefort <rjhilgefort@gmail.com>
Date: Wed, 5 Aug 2026 21:07:52 -0600
Subject: [PATCH] fix(launchd): require a supervised PID to call a reload
 successful
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The reload retry loop treated `launchctl list <label>` exit 0 as success,
but exit 0 also covers a registered-but-not-running definition (macOS 26+
`state = not running`) — the same trap _probe_launchd_service_running
already guards against. Require a PID so success means launchd is
supervising a live process, in both the Python loop and the shell helper.

Verified against live launchd: a RunAtLoad=false job reports exit 0 with
no PID, which the old check accepted and the new one rejects.

Note this is NOT what distinguishes a draining instance — measured, the
label deregisters within ~1s of bootout while the old process drains on.
Waiting for the old PID to exit is what covers that.
---
 hermes_cli/gateway.py                    | 38 ++++++++---
 tests/hermes_cli/test_gateway_service.py | 86 ++++++++++++++----------
 2 files changed, 79 insertions(+), 45 deletions(-)

diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py
index ab33508d115f9..dfcc66b179ed9 100644
--- a/hermes_cli/gateway.py
+++ b/hermes_cli/gateway.py
@@ -3899,17 +3899,34 @@ def _append_launchd_reload_log(message: str) -> None:
         pass
 
 
-def _launchctl_label_registered(label: str) -> bool:
-    """True when ``launchctl list <label>`` reports the job as registered."""
+def _launchctl_label_supervising_process(label: str) -> bool:
+    """True when launchd both knows ``label`` AND is running a process for it.
+
+    A bare ``launchctl list <label>`` exit-0 only proves a *definition* is
+    registered — it also returns 0 for ``state = not running`` (macOS 26+),
+    which is why :func:`_probe_launchd_service_running` already insists on a
+    PID. The reload's success check needs the same standard: ending the retry
+    loop on "a definition exists" can report success for a job launchd is not
+    actually running.
+
+    Measured against live launchd (2026-08-05): immediately after ``bootout``
+    the label deregisters within ~1s (rc=113) while the old process keeps
+    draining, so this is NOT what distinguishes a draining instance from a
+    fresh one — waiting for the old PID to exit before bootstrapping is what
+    does that. This check is the narrower guarantee: success means launchd is
+    supervising a live process.
+    """
     try:
         result = subprocess.run(
             ["launchctl", "list", label],
             check=False,
             timeout=10,
-            stdout=subprocess.DEVNULL,
-            stderr=subprocess.DEVNULL,
+            capture_output=True,
+            text=True, encoding='utf-8', errors='replace',
         )
-        return result.returncode == 0
+        if result.returncode != 0:
+            return False
+        return _parse_launchd_pid_from_list_output(result.stdout) is not None
     except (subprocess.TimeoutExpired, OSError):
         return False
 
@@ -3939,11 +3956,11 @@ def _retry_launchctl_bootstrap_until_registered(
         attempt += 1
         try:
             _launchctl_bootstrap(domain, plist_path, label, timeout=30)
-            if _launchctl_label_registered(label):
+            if _launchctl_label_supervising_process(label):
                 return True
             _append_launchd_reload_log(
                 f"bootstrap attempt {attempt} exited 0 but {domain}/{label} "
-                f"is not registered (launchctl list) — retrying"
+                f"has no supervised process (launchctl list) — retrying"
             )
         except subprocess.CalledProcessError as exc:
             _append_launchd_reload_log(
@@ -4295,12 +4312,15 @@ def refresh_launchd_plist_if_needed() -> bool:
             f"_deadline=$(($(date +%s) + {_reload_budget})); "
             f"while :; do "
             f"  launchctl bootstrap {shlex.quote(domain)} {shlex.quote(str(plist_path))} 2>/dev/null; "
-            f"  if launchctl list {shlex.quote(label)} >/dev/null 2>&1; then break; fi; "
+            # Require a PID, not just exit 0: a bare `launchctl list` also
+            # succeeds for a registered-but-not-running definition, which would
+            # end this loop reporting success for a job launchd isn't running.
+            f"  if launchctl list {shlex.quote(label)} 2>/dev/null | grep -q '\"PID\"'; then break; fi; "
             f"  echo \"[$(date '+%Y-%m-%d %H:%M:%S %z')] bootstrap not yet registered for {shlex.quote(target)} — retrying\" >> {shlex.quote(str(reload_log_path))}; "
             f"  if [ $(date +%s) -ge $_deadline ]; then break; fi; "
             f"  sleep 2; "
             f"done; "
-            f"if ! launchctl list {shlex.quote(label)} >/dev/null 2>&1; then "
+            f"if ! launchctl list {shlex.quote(label)} 2>/dev/null | grep -q '\"PID\"'; then "
             f"  echo \"[$(date '+%Y-%m-%d %H:%M:%S %z')] FAILED launchd reload for {shlex.quote(target)} — service NOT registered after {_reload_budget}s of retries\" >> {shlex.quote(str(reload_log_path))}; "
             f"fi; "
             # Submitted jobs stay registered with launchd after the script
diff --git a/tests/hermes_cli/test_gateway_service.py b/tests/hermes_cli/test_gateway_service.py
index ea2c60f4d2d0f..a5d9ad6dee95a 100644
--- a/tests/hermes_cli/test_gateway_service.py
+++ b/tests/hermes_cli/test_gateway_service.py
@@ -525,38 +525,6 @@ class TestLaunchdServiceRecovery:
         assert waited and waited[0][0] == 4242
 
 
-    def test_registered_but_not_running_is_not_success(self, monkeypatch):
-        """A definition with no PID must not end the loop.
-
-        `launchctl list` exits 0 for a registered-but-not-running job (macOS
-        26+ `state = not running`), so exit-0 alone would report success for a
-        gateway launchd is not actually running. Verified against live launchd
-        on 2026-08-05.
-        """
-        list_calls = {"n": 0}
-
-        def fake_run(cmd, check=False, **kwargs):
-            if cmd[:2] == ["launchctl", "list"]:
-                list_calls["n"] += 1
-                # Registered (exit 0) but no PID line — never running.
-                return SimpleNamespace(
-                    returncode=0,
-                    stdout='{\n\t"Label" = "ai.hermes.gateway";\n};',
-                    stderr="",
-                )
-            return SimpleNamespace(returncode=0, stdout="", stderr="")
-
-        monkeypatch.setattr(gateway_cli.subprocess, "run", fake_run)
-        monkeypatch.setattr(gateway_cli.time, "sleep", lambda *_a, **_k: None)
-
-        ok = gateway_cli._retry_launchctl_bootstrap_until_registered(
-            self.DOMAIN, self.PLIST, self.LABEL,
-            deadline=gateway_cli.time.monotonic() - 1,  # already expired
-        )
-        assert ok is False
-        assert list_calls["n"] >= 1
-
-
     def test_launchd_domain_uses_user_domain(self, monkeypatch):
         # The user/<uid> domain (not gui/<uid>) is the one reachable from
         # non-Aqua/background sessions on macOS 26+ (issue #23387).
@@ -1937,14 +1905,24 @@ class TestRetryLaunchctlBootstrapUntilRegistered:
     PLIST = "/tmp/ai.hermes.gateway.plist"
     LABEL = "ai.hermes.gateway"
 
+    # `launchctl list <label>` output for a job launchd is actively running.
+    # Success requires a PID here, not just exit 0 — exit 0 alone also covers a
+    # registered-but-not-running definition (macOS 26+ `state = not running`).
+    RUNNING_LIST_OUTPUT = '{\n\t"PID" = 4242;\n\t"Label" = "ai.hermes.gateway";\n};'
+
     def test_returns_true_once_label_is_registered(self, monkeypatch):
-        """Success requires launchctl list to confirm registration, not just
-        a zero bootstrap exit."""
+        """Success requires launchctl list to confirm a supervised process, not
+        just a zero bootstrap exit."""
         list_results = iter([1, 0])  # first check: not registered, second: registered
 
         def fake_run(cmd, check=False, **kwargs):
             if cmd[:2] == ["launchctl", "list"]:
-                return SimpleNamespace(returncode=next(list_results))
+                rc = next(list_results)
+                return SimpleNamespace(
+                    returncode=rc,
+                    stdout=self.RUNNING_LIST_OUTPUT if rc == 0 else "",
+                    stderr="",
+                )
             return SimpleNamespace(returncode=0, stdout="", stderr="")
 
         monkeypatch.setattr(gateway_cli.subprocess, "run", fake_run)
@@ -1969,7 +1947,12 @@ class TestRetryLaunchctlBootstrapUntilRegistered:
                 return SimpleNamespace(returncode=0, stdout="", stderr="")
             if cmd[:2] == ["launchctl", "list"]:
                 # registered only after the second (successful) bootstrap
-                return SimpleNamespace(returncode=0 if attempts["bootstrap"] >= 2 else 1)
+                ok = attempts["bootstrap"] >= 2
+                return SimpleNamespace(
+                    returncode=0 if ok else 1,
+                    stdout=self.RUNNING_LIST_OUTPUT if ok else "",
+                    stderr="",
+                )
             return SimpleNamespace(returncode=0, stdout="", stderr="")
 
         monkeypatch.setattr(gateway_cli.subprocess, "run", fake_run)
@@ -1982,3 +1965,34 @@ class TestRetryLaunchctlBootstrapUntilRegistered:
         assert ok is True
         assert attempts["bootstrap"] >= 2  # the timeout was retried, not raised
 
+    def test_registered_but_not_running_is_not_success(self, monkeypatch):
+        """A definition with no PID must not end the loop.
+
+        `launchctl list` exits 0 for a registered-but-not-running job (macOS
+        26+ `state = not running`), so exit-0 alone would report success for a
+        gateway launchd is not actually running. Verified against live launchd
+        on 2026-08-05.
+        """
+        list_calls = {"n": 0}
+
+        def fake_run(cmd, check=False, **kwargs):
+            if cmd[:2] == ["launchctl", "list"]:
+                list_calls["n"] += 1
+                # Registered (exit 0) but no PID line — never running.
+                return SimpleNamespace(
+                    returncode=0,
+                    stdout='{\n\t"Label" = "ai.hermes.gateway";\n};',
+                    stderr="",
+                )
+            return SimpleNamespace(returncode=0, stdout="", stderr="")
+
+        monkeypatch.setattr(gateway_cli.subprocess, "run", fake_run)
+        monkeypatch.setattr(gateway_cli.time, "sleep", lambda *_a, **_k: None)
+
+        ok = gateway_cli._retry_launchctl_bootstrap_until_registered(
+            self.DOMAIN, self.PLIST, self.LABEL,
+            deadline=gateway_cli.time.monotonic() - 1,  # already expired
+        )
+        assert ok is False
+        assert list_calls["n"] >= 1
+