From bee489d5638d98517942f9618b9da94abd581a8d Mon Sep 17 00:00:00 2001
From: Jeff Mettel <jeff.mettel@gmail.com>
Date: Sun, 23 Aug 2026 06:52:21 -0700
Subject: [PATCH] fix(update): restart a booted-out launchd gateway instead of
 silently skipping it (#74973, salvage #75021)

Port of @jeff-mettel's fix onto the post-#91378/#92902 fleet-restart
shape. The current-profile restart was gated on `launchctl list <label>`
exiting 0 - a booted-out job (plist present, definition deregistered:
crashed helper, manual bootout, failed prior update) fails that check,
so the branch silently skipped: no restart, no message, KeepAlive unable
to revive a definition launchd no longer knows, update printing
'Update complete!' with the gateway down. `launchctl list` is also
session-scoped and unreliable as a loaded/unloaded classifier.

- _restart_launchd_gateway_after_update() (his extraction, adapted):
  plist-exists is the ONLY gate; launchd_restart() owns the
  bootout/bootstrap/kickstart ladder for every plist-present state;
  every failure path is loud and names the manual recovery command.
  The gate-error 'except: pass' (the second silent variant) now counts
  the label failed and tells the operator.
- Success still requires the #92902 supervision verify (fresh
  supervised PID), composing his fix with the returned-is-not-supervised
  guard.
- His regression suite adapted to the (restarted, failed) contract; the
  old 'unregistered -> left alone' pinning test FLIPPED - it pinned the
  bug.

A/B: his suite + the flipped test red on merge-base product code
(silent skip live), green at head. No macOS CI lane exists; field
evidence is #74973's reproductions plus the launchctl print output
shapes pinned in the suite.
---
 hermes_cli/update_cmd.py                      | 131 +++++++----
 ...est_update_launchd_restart_verification.py |  17 +-
 .../test_update_launchd_unloaded_gateway.py   | 204 ++++++++++++++++++
 3 files changed, 300 insertions(+), 52 deletions(-)
 create mode 100644 tests/hermes_cli/test_update_launchd_unloaded_gateway.py

diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py
index 7f8caaad72..2fb06d2028 100644
--- a/hermes_cli/update_cmd.py
+++ b/hermes_cli/update_cmd.py
@@ -5140,6 +5140,85 @@ def _warn_incomplete_gateway_fleet_restart(failed_units: list) -> None:
         print("    launchctl kickstart -k gui/$UID/<label>   # macOS (or user/$UID)")
 
 
+def _restart_launchd_gateway_after_update(
+    *, supervision_verify: bool = True
+) -> tuple[list, list]:
+    """Restart the invoking profile's launchd gateway after an update.
+
+    #74973 (salvage #75021 by @jeff-mettel): the restart used to be gated on
+    ``launchctl list <label>`` exiting 0. A *booted-out* job — plist present,
+    definition deregistered from launchd (crashed helper, manual bootout,
+    failed prior update) — fails that check, so the whole branch silently
+    skipped: no restart, no message, ``KeepAlive`` unable to revive a
+    definition launchd no longer knows, and the update still printed
+    "Update complete!". ``launchctl list`` is also session-scoped and can
+    exit non-zero while the job is alive in its gui/user domain, so it is
+    not a reliable classifier at all.
+
+    The fix performs NO list-based classification: when the plist exists,
+    ``launchd_restart()`` always runs — it drains a live PID, kickstarts
+    with ``-k``, and owns the bootout/bootstrap/kickstart ladder for the
+    genuinely unloaded state. Every failure path is loud and names the
+    manual recovery command.
+
+    Returns ``(restarted_labels, failed_labels)``. With
+    ``supervision_verify`` (the update path), success additionally requires
+    launchd reporting a fresh supervised PID (#88848 — "the call returned"
+    is not "the gateway is supervised").
+    """
+    from hermes_cli.gateway import (
+        get_launchd_label,
+        get_launchd_plist_path,
+        launchd_restart,
+        wait_for_launchd_gateway_supervision,
+    )
+
+    current_label = get_launchd_label()
+    try:
+        if not get_launchd_plist_path().exists():
+            return [], []  # not a launchd install — nothing to do or warn
+        try:
+            launchd_restart()
+        except subprocess.CalledProcessError as e:
+            stderr = (getattr(e, "stderr", "") or "").strip()
+            print(
+                f"  ⚠ Gateway restart failed: {stderr}\n"
+                "    The gateway may be DOWN on pre-update code. "
+                "Recover manually: hermes gateway restart"
+            )
+            return [], [current_label]
+    except (FileNotFoundError, subprocess.TimeoutExpired) as e:
+        # A plist exists, so a gateway is SUPPOSED to be supervised here —
+        # a broken/missing/wedged launchctl is not proof nothing needs
+        # restarting. The old code `pass`ed here (#74973's second silent
+        # variant); count it and tell the operator.
+        print(
+            "  ⚠ Could not restart the gateway "
+            f"({e.__class__.__name__}: {e}).\n"
+            "    Recover manually: hermes gateway restart"
+        )
+        return [], [current_label]
+
+    if not supervision_verify:
+        return [current_label], []
+
+    # launchd_restart() returning is only "restart REQUESTED" — the
+    # self-restart branch hands work to the running gateway, a plist reload
+    # to a detached helper; both asynchronous. A helper that dies before its
+    # first bootstrap (#88848), or a bootstrap that exits 0 without
+    # registering (measured on macOS 26.6.1), otherwise reaches "Update
+    # complete!" with nothing supervising the gateway. Verified
+    # domain-agnostically (a domain locate fails on macOS-26 hosts whose
+    # per-user domains reject service management).
+    if wait_for_launchd_gateway_supervision(label=current_label):
+        return [current_label], []
+    print(
+        f"  ✗ {current_label} restarted but launchd is not supervising it.\n"
+        "    Check logs, then: hermes gateway restart"
+    )
+    return [], [current_label]
+
+
 def _restart_macos_launchd_gateways(
     restarted_services: list,
     failed_or_stale_units: list,
@@ -5175,54 +5254,12 @@ def _restart_macos_launchd_gateways(
     )
 
     # --- Current profile: unchanged single-service path ---------------------
-    # Gate order and predicate mirror the pre-fleet inline block exactly:
-    # plist first (no plist → zero launchctl calls), then the domain-agnostic
-    # `launchctl list` registration check — NOT a domain locate, which fails
-    # on macOS-26 hosts whose per-user domains reject service management
-    # even though launchd_restart() owns that fallback. Gate errors skip
-    # silently (best-effort, as before); only launchd_restart() itself
-    # failing counts toward the incomplete-update warning.
+    _restarted, _failed = _restart_launchd_gateway_after_update(
+        supervision_verify=True
+    )
+    restarted_services.extend(_restarted)
+    failed_or_stale_units.extend(_failed)
     current_label = get_launchd_label()
-    try:
-        if get_launchd_plist_path().exists() and _launchd_service_registered(
-            current_label
-        ):
-            try:
-                launchd_restart()
-            except subprocess.CalledProcessError as e:
-                stderr = (getattr(e, "stderr", "") or "").strip()
-                print(f"  ⚠ Gateway restart failed: {stderr}")
-                failed_or_stale_units.append(current_label)
-            else:
-                # Siblings below are only counted as restarted once launchd
-                # reports a fresh supervised pid; the invoking profile was
-                # counted on "launchd_restart() did not raise" alone. That is
-                # not the same claim: launchd_restart() returns as soon as the
-                # restart has been REQUESTED -- the self-restart branch hands
-                # the work to the running gateway and returns immediately, and
-                # a plist reload is handed to a detached helper. Both are
-                # asynchronous, so a helper that dies before its first
-                # bootstrap (#88848), or a `launchctl bootstrap` that exits 0
-                # without registering (measured on macOS 26.6.1), both reached
-                # "Update complete!" with nothing supervising the gateway.
-                #
-                # Verified domain-agnostically, NOT via
-                # _wait_for_launchd_service_pid: that needs an explicit domain,
-                # and the gate above deliberately avoids a domain locate
-                # because it fails on macOS-26 hosts whose per-user domains
-                # reject service management even though launchd_restart() owns
-                # that fallback.
-                if wait_for_launchd_gateway_supervision(label=current_label):
-                    restarted_services.append(current_label)
-                else:
-                    failed_or_stale_units.append(current_label)
-                    print(
-                        f"  ✗ {current_label} restarted but launchd is not "
-                        "supervising it.\n"
-                        "    Check logs, then: hermes gateway restart"
-                    )
-    except (FileNotFoundError, subprocess.TimeoutExpired):
-        pass
 
     # --- Sibling profiles ---------------------------------------------------
     for label in launchd_gateway_labels_for_install():
diff --git a/tests/hermes_cli/test_update_launchd_restart_verification.py b/tests/hermes_cli/test_update_launchd_restart_verification.py
index a447e217e3..db21980a2d 100644
--- a/tests/hermes_cli/test_update_launchd_restart_verification.py
+++ b/tests/hermes_cli/test_update_launchd_restart_verification.py
@@ -287,13 +287,20 @@ class TestInvokingProfileIsVerifiedLikeItsSiblings:
         assert calls["restart"] == 0
         assert calls["verify"] == 0
 
-    def test_unregistered_label_is_left_alone(self, monkeypatch):
-        """No launchd registration on this host means nothing to restart."""
+    def test_unregistered_label_is_restarted_not_skipped(self, monkeypatch):
+        """A booted-out job (plist present, deregistered) must be RESTARTED.
+
+        FLIPPED by the #74973 fix (salvage #75021): this test used to pin
+        'registered=False → nothing to restart', which was precisely the
+        silent-skip bug — launchctl list is session-scoped and non-zero
+        for booted-out jobs whose plist very much still wants a gateway;
+        launchd_restart() owns the bootout/bootstrap ladder for that state.
+        """
         calls = _patch_launchd_env(monkeypatch, registered=False)
 
-        assert _run_fleet_restart() == ([], [])
-        assert calls["restart"] == 0
-        assert calls["verify"] == 0
+        assert _run_fleet_restart() == ([LABEL], [])
+        assert calls["restart"] == 1
+        assert calls["verify"] == 1
 
 
 class TestIncompleteFleetWarningIsPlatformCorrect:
diff --git a/tests/hermes_cli/test_update_launchd_unloaded_gateway.py b/tests/hermes_cli/test_update_launchd_unloaded_gateway.py
new file mode 100644
index 0000000000..315fa72442
--- /dev/null
+++ b/tests/hermes_cli/test_update_launchd_unloaded_gateway.py
@@ -0,0 +1,204 @@
+"""Regression for #74973 — `hermes update` must not leave the gateway down.
+
+On macOS the update's launchd branch guarded the restart behind
+``launchctl list <label>`` exiting 0. A job that has been *booted out* of
+launchd exits non-zero there, so the whole restart branch was skipped — with
+no ``else`` and no message. The update printed ``✓ Update complete!`` and
+exited 0 while the gateway was stopped *and* deregistered, which ``KeepAlive``
+cannot recover because the job definition is gone. Messaging adapters and
+cron stayed dark until someone manually ran ``hermes gateway restart``.
+
+``launchctl list`` is also not a reliable loaded/unloaded classifier: it is
+session-scoped and can exit non-zero while the job is alive in its gui/user
+domain (PR #75021 review). The fix therefore does not classify at all — when
+the plist exists it always calls ``launchd_restart()``, which drains a live
+PID, kickstarts with ``-k``, and falls back to bootout/bootstrap/kickstart
+when the job is genuinely unloaded.
+"""
+
+from __future__ import annotations
+
+import subprocess
+
+import pytest
+
+from hermes_cli import update_cmd
+
+
+class _FakePlist:
+    def __init__(self, exists: bool = True) -> None:
+        self._exists = exists
+
+    def exists(self) -> bool:
+        return self._exists
+
+
+@pytest.fixture
+def launchd(monkeypatch):
+    """Stub hermes_cli.gateway so no real launchctl call is made."""
+    calls: list[str] = []
+    state = {"plist": _FakePlist(True), "restart_exc": None}
+    subprocess_calls: list[list] = []
+
+    import hermes_cli.gateway as gateway_mod
+
+    monkeypatch.setattr(gateway_mod, "get_launchd_label", lambda: "ai.hermes.gateway", raising=False)
+    monkeypatch.setattr(gateway_mod, "get_launchd_plist_path", lambda: state["plist"], raising=False)
+
+    def fake_restart():
+        if state["restart_exc"] is not None:
+            raise state["restart_exc"]
+        calls.append("restart")
+
+    monkeypatch.setattr(gateway_mod, "launchd_restart", fake_restart, raising=False)
+
+    def fake_run(*args, **kwargs):
+        subprocess_calls.append(args[0] if args else [])
+        return subprocess.CompletedProcess(args=args[0] if args else [], returncode=0, stdout="", stderr="")
+
+    monkeypatch.setattr(update_cmd.subprocess, "run", fake_run)
+    return calls, state, subprocess_calls
+
+
+class TestLaunchdRestartAfterUpdate:
+    def test_plist_present_always_restarts_without_classifying(self, launchd, capsys):
+        """The restart must not be gated on `launchctl list`.
+
+        `list` can exit non-zero while the job is alive in its domain
+        (`launchctl print gui/<uid>/<label>` reports state=running with a
+        PID). Routing that state to a plain start would leave the old-code
+        process running, because `kickstart` without `-k` does not terminate
+        a running service. The helper therefore performs no list-based
+        classification at all — `launchd_restart()` handles every
+        plist-present state.
+        """
+        calls, state, subprocess_calls = launchd
+
+        assert update_cmd._restart_launchd_gateway_after_update(supervision_verify=False) == (["ai.hermes.gateway"], [])
+        assert calls == ["restart"]
+        # No `launchctl list` classification happens in this helper.
+        assert subprocess_calls == []
+        assert "NOT running" not in capsys.readouterr().out
+
+    def test_restart_failure_warns_that_gateway_is_down(self, launchd, capsys):
+        calls, state, _ = launchd
+        state["restart_exc"] = subprocess.CalledProcessError(
+            returncode=1, cmd=["launchctl", "kickstart"], stderr="kickstart refused"
+        )
+
+        assert update_cmd._restart_launchd_gateway_after_update(supervision_verify=False) == ([], ["ai.hermes.gateway"])
+        out = capsys.readouterr().out
+        assert "Gateway restart failed" in out
+        assert "kickstart refused" in out
+        assert "hermes gateway restart" in out
+
+    @pytest.mark.parametrize(
+        "exc",
+        [
+            FileNotFoundError("launchctl"),
+            subprocess.TimeoutExpired(cmd=["launchctl", "kickstart"], timeout=90),
+        ],
+    )
+    def test_launchctl_unusable_is_not_swallowed(self, launchd, capsys, exc):
+        """A missing binary or a timeout used to `pass` silently."""
+        calls, state, _ = launchd
+        state["restart_exc"] = exc
+
+        assert update_cmd._restart_launchd_gateway_after_update(supervision_verify=False) == ([], ["ai.hermes.gateway"])
+        assert calls == []
+        out = capsys.readouterr().out
+        assert "Could not restart the gateway" in out
+        assert "hermes gateway restart" in out
+
+    def test_no_plist_is_not_a_launchd_install(self, launchd, capsys):
+        """No service definition → nothing to restart, and nothing to warn about."""
+        calls, state, _ = launchd
+        state["plist"] = _FakePlist(False)
+
+        assert update_cmd._restart_launchd_gateway_after_update(supervision_verify=False) == ([], [])
+        assert calls == []
+        assert capsys.readouterr().out == ""
+
+
+# `launchctl print gui/<uid>/<label>` excerpt for a running service, matching
+# the real output shape (tab-indented, lowercase `pid = <N>`).
+_PRINT_OUTPUT_RUNNING = """\
+ai.hermes.gateway = {
+\tactive count = 1
+\tpath = /Users/u/Library/LaunchAgents/ai.hermes.gateway.plist
+\tstate = running
+\tpid = 59038
+\tprogram = /Users/u/.hermes/bin/hermes
+}
+"""
+
+
+class TestServicePidSweepExclusion:
+    """Regression for the PR #75021 review: `_get_service_pids()` must not
+    rely on `launchctl list` alone.
+
+    In the session-scoped failure state (`list` exits non-zero while the
+    domain-qualified `print` reports a positive PID) the launchd-owned
+    gateway PID was missing from the exclusion set, so the post-update
+    manual-gateway sweep could kill the process launchd just (re)started.
+    """
+
+    @pytest.fixture
+    def macos_launchd(self, monkeypatch):
+        import hermes_cli.gateway as gateway_mod
+
+        state = {"list_rc": 1, "print_rc": 0, "print_out": _PRINT_OUTPUT_RUNNING}
+
+        monkeypatch.setattr(gateway_mod, "supports_systemd_services", lambda: False)
+        monkeypatch.setattr(gateway_mod, "is_macos", lambda: True)
+        monkeypatch.setattr(gateway_mod, "get_launchd_label", lambda: "ai.hermes.gateway")
+        monkeypatch.setattr(gateway_mod, "_launchd_domain", lambda: "gui/501")
+
+        def fake_run(argv, **kwargs):
+            if argv[:2] == ["launchctl", "list"]:
+                return subprocess.CompletedProcess(argv, state["list_rc"], stdout="", stderr="")
+            if argv[:2] == ["launchctl", "print"]:
+                return subprocess.CompletedProcess(
+                    argv, state["print_rc"], stdout=state["print_out"], stderr=""
+                )
+            raise AssertionError(f"unexpected subprocess call: {argv}")
+
+        monkeypatch.setattr(gateway_mod.subprocess, "run", fake_run)
+        return state
+
+    def test_list_failure_falls_back_to_domain_print(self, macos_launchd):
+        """`list` rc=1, `print` reports pid 59038 → the PID is still excluded."""
+        from hermes_cli.gateway import _get_service_pids
+
+        assert 59038 in _get_service_pids()
+
+    def test_both_interfaces_negative_means_no_pid(self, macos_launchd):
+        macos_launchd["print_rc"] = 113  # job genuinely not found in the domain
+
+        from hermes_cli.gateway import _get_service_pids
+
+        assert _get_service_pids() == set()
+
+    def test_registered_but_not_running_has_no_pid_line(self, macos_launchd):
+        macos_launchd["print_out"] = _PRINT_OUTPUT_RUNNING.replace("\tpid = 59038\n", "")
+
+        from hermes_cli.gateway import _get_service_pids
+
+        assert _get_service_pids() == set()
+
+
+class TestParseLaunchdPidFromPrintOutput:
+    def test_running_service(self):
+        from hermes_cli.gateway import _parse_launchd_pid_from_print_output
+
+        assert _parse_launchd_pid_from_print_output(_PRINT_OUTPUT_RUNNING) == 59038
+
+    def test_no_pid_line(self):
+        from hermes_cli.gateway import _parse_launchd_pid_from_print_output
+
+        assert _parse_launchd_pid_from_print_output("state = not running\n") is None
+
+    def test_nonpositive_pid_is_ignored(self):
+        from hermes_cli.gateway import _parse_launchd_pid_from_print_output
+
+        assert _parse_launchd_pid_from_print_output("\tpid = -1\n") is None