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.
This commit is contained in:
Jeff Mettel
+84
-47
@@ -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():
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user