fix(profiles): short-circuit s6 hooks on host before importing service_manager

Follow-up to @benbarclay's Docker s6 PR (#30136). The Phase 4 hooks
`_maybe_register_gateway_service` and `_maybe_unregister_gateway_service`
were already documented as "no-op on host", but they reached that no-op
by:

  1. importing `hermes_cli.service_manager`
  2. calling `get_service_manager()` (which calls `detect_service_manager()`)
  3. checking `mgr.supports_runtime_registration()` and returning False

If anything in step 1 or 2 raised an unexpected exception (e.g. a host
machine with a partial s6 install — `/proc/1/comm == s6-svscan` somehow,
but `/run/s6/basedir` absent, or vice versa), the `except Exception`
in the hook would print a confusing "⚠ Could not register s6 gateway
service: ..." warning on a non-container machine that has never touched
the container.

Reorder so `detect_service_manager() != "s6"` is checked FIRST, and
return silently for any detection failure. Host machines now:

  - never import the s6 backend
  - never call get_service_manager()
  - never print an s6-shaped warning under any failure mode

E2E confirmed on host Linux (systemd):
  `_maybe_register_gateway_service(...)` produces empty stdout,
  detect_service_manager() returns "systemd".

Existing tests updated to patch `detect_service_manager` for the s6
call-through cases (they previously relied on get_service_manager
being the only gate, which is no longer true). Added one new test —
`test_register_silent_when_detect_throws` — asserting that a broken
detector cannot leak a warning to host users.

cc @benbarclay — visible behavior change vs. your branch is one
fewer code path on host. Test changes are minimal (one helper +
`_patch_detect_s6` opt-in per s6 test). Happy to revert if you
prefer the original shape.
This commit is contained in:
teknium1 2026-05-24 18:07:47 -07:00
parent 6c49bdc4f4
commit a4092ab217
No known key found for this signature in database
2 changed files with 80 additions and 0 deletions

View file

@ -994,12 +994,30 @@ def _maybe_register_gateway_service(profile_name: str) -> None:
(``[gateway] port = ``) there is no Python-side allocator
(PR #30136 review item I5 retired the SHA-256-derived range
[9200, 9800) because it was dead code through the entire stack).
Host short-circuit: check ``detect_service_manager()`` first and
return immediately if it isn't ``"s6"``. This keeps host
(systemd/launchd/windows) profile creation completely silent
no ``get_service_manager()`` call, no exception path, no chance
of the `` Could not register s6 gateway service`` warning ever
rendering on a non-container machine. The earlier
``supports_runtime_registration()`` check still catches the case
where detection somehow returns ``"s6"`` but the backend isn't
actually the S6 one.
"""
try:
from hermes_cli.service_manager import detect_service_manager
if detect_service_manager() != "s6":
return # host path — silent, no registration needed
from hermes_cli.service_manager import get_service_manager
mgr = get_service_manager()
except RuntimeError:
return # no backend on this host — nothing to do
except Exception:
# Defensive: detect_service_manager failed for some other
# reason. Stay silent on host rather than printing a confusing
# s6 warning to users who have never touched the container.
return
if not mgr.supports_runtime_registration():
return # host backend; no-op
try:
@ -1018,12 +1036,20 @@ def _maybe_unregister_gateway_service(profile_name: str) -> None:
No-op on host. Idempotent: absent services are silently skipped
by ``unregister_profile_gateway``.
Same host short-circuit as :func:`_maybe_register_gateway_service`
see that docstring.
"""
try:
from hermes_cli.service_manager import detect_service_manager
if detect_service_manager() != "s6":
return # host path — silent
from hermes_cli.service_manager import get_service_manager
mgr = get_service_manager()
except RuntimeError:
return
except Exception:
return
if not mgr.supports_runtime_registration():
return
try:

View file

@ -65,7 +65,30 @@ class _S6Manager:
self.unregistered.append(profile)
def _patch_detect_s6(monkeypatch: pytest.MonkeyPatch) -> None:
"""Pretend we're inside an s6 container so the host short-circuit
in :func:`_maybe_register_gateway_service` /
:func:`_maybe_unregister_gateway_service` doesn't fire.
Without this, ``detect_service_manager()`` runs its real
implementation (host Linux/macOS in CI), returns ``"systemd"`` or
``"launchd"``, and the hooks return early before reaching the
patched ``get_service_manager``. Each s6-call-through test
explicitly opts into this so the host-no-op tests can still
exercise the early-return path.
"""
monkeypatch.setattr(
"hermes_cli.service_manager.detect_service_manager",
lambda: "s6",
)
def test_register_noop_on_host(monkeypatch: pytest.MonkeyPatch) -> None:
# NOTE: deliberately DO NOT patch detect_service_manager — we want
# the real host detection to kick in and short-circuit before
# get_service_manager is ever called. The lambda below is a
# defense-in-depth assertion that get_service_manager is never
# reached on host.
monkeypatch.setattr(
"hermes_cli.service_manager.get_service_manager",
lambda: _HostManager(),
@ -75,6 +98,7 @@ def test_register_noop_on_host(monkeypatch: pytest.MonkeyPatch) -> None:
def test_register_calls_through_on_s6(monkeypatch: pytest.MonkeyPatch) -> None:
_patch_detect_s6(monkeypatch)
mgr = _S6Manager()
monkeypatch.setattr(
"hermes_cli.service_manager.get_service_manager", lambda: mgr,
@ -88,6 +112,7 @@ def test_register_swallows_duplicate_value_error(
) -> None:
"""A pre-existing s6 registration (from container-boot reconcile)
is a benign condition register must not propagate ValueError."""
_patch_detect_s6(monkeypatch)
mgr = _S6Manager()
mgr.raise_on_register = ValueError("already registered")
monkeypatch.setattr(
@ -102,6 +127,7 @@ def test_register_swallows_arbitrary_error(
) -> None:
"""Even an unexpected exception from the manager must not bring
down `hermes profile create` print and continue."""
_patch_detect_s6(monkeypatch)
mgr = _S6Manager()
mgr.raise_on_register = RuntimeError("svscanctl exploded")
monkeypatch.setattr(
@ -117,6 +143,7 @@ def test_register_swallows_no_backend_runtime_error(
) -> None:
"""When `get_service_manager()` raises RuntimeError (no backend
detected), the hook must silently no-op."""
_patch_detect_s6(monkeypatch)
def _no_backend() -> None:
raise RuntimeError("no supported service manager detected")
monkeypatch.setattr(
@ -126,7 +153,32 @@ def test_register_swallows_no_backend_runtime_error(
_maybe_register_gateway_service("anywhere")
def test_register_silent_when_detect_throws(
monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str],
) -> None:
"""If detect_service_manager itself raises (e.g. a partial s6
install on a host machine), the hook must stay silent no
confusing s6 warning printed to a user who has never touched a
container."""
def _broken_detect() -> str:
raise RuntimeError("detection blew up")
monkeypatch.setattr(
"hermes_cli.service_manager.detect_service_manager", _broken_detect,
)
# If get_service_manager is reached, the test will assert via
# _HostManager.register. It must NOT be reached.
monkeypatch.setattr(
"hermes_cli.service_manager.get_service_manager",
lambda: _HostManager(),
)
_maybe_register_gateway_service("anywhere")
captured = capsys.readouterr()
assert "Could not register" not in captured.out
assert captured.out == ""
def test_unregister_noop_on_host(monkeypatch: pytest.MonkeyPatch) -> None:
# Same as test_register_noop_on_host: rely on real host detection.
monkeypatch.setattr(
"hermes_cli.service_manager.get_service_manager",
lambda: _HostManager(),
@ -135,6 +187,7 @@ def test_unregister_noop_on_host(monkeypatch: pytest.MonkeyPatch) -> None:
def test_unregister_calls_through_on_s6(monkeypatch: pytest.MonkeyPatch) -> None:
_patch_detect_s6(monkeypatch)
mgr = _S6Manager()
monkeypatch.setattr(
"hermes_cli.service_manager.get_service_manager", lambda: mgr,
@ -146,6 +199,7 @@ def test_unregister_calls_through_on_s6(monkeypatch: pytest.MonkeyPatch) -> None
def test_unregister_swallows_errors(
monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str],
) -> None:
_patch_detect_s6(monkeypatch)
mgr = _S6Manager()
mgr.raise_on_unregister = RuntimeError("svc gone weird")
monkeypatch.setattr(