From 336227bf002dcd014a91e0bf3f0fa28dc32357f2 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 15 Sep 2026 11:49:31 -0700 Subject: [PATCH] refactor(gateway): move the on-demand s6 slot helper into service_manager, trim tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reshape of the cherry-picked fix from #104194: - The SOUL.md gate + `register_profile_gateway(start_now=False)` now live in `hermes_cli/service_manager.py::register_unregistered_profile_gateway`, next to the s6 manager and `_profile_dir_for_gateway_service` it needs, instead of a private reach-in from the 6.5k-line `hermes_cli/gateway.py` facade. The facade only decides "start repairs, stop/restart re-raise" and keeps ONE error handler (S6Error is a RuntimeError; register's ValueError/RuntimeError/OSError surface as the same `✗` + exit 1). - Tests trimmed from four to two invariants: start on a real profile registers `down` and then starts; stop on an unregistered profile / start on a directory without SOUL.md keep the original error and mint nothing (parametrized). Dropped: the registration-failure traceback test (covered by the single except clause) and the duplicate no-marker/stop split. The test now resolves the profile dir through the real HERMES_HOME mapping instead of monkeypatching `_profile_dir_for_gateway_service`. - Docs: docker.md multi-profile section says `gateway start` inside the container registers a slot for a profile created from the host. --- hermes_cli/gateway.py | 58 +++--------- hermes_cli/service_manager.py | 18 ++++ tests/hermes_cli/test_gateway_s6_dispatch.py | 95 ++++++++------------ website/docs/user-guide/docker.md | 2 + 4 files changed, 70 insertions(+), 103 deletions(-) diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index 0f9b150bbd..195cc3cd0b 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -5861,7 +5861,8 @@ def _dispatch_via_service_manager_if_s6(action: str, profile: str | None = None) """Dispatch start/stop/restart via s6 inside an s6 container; True iff dispatched (caller returns). Profile defaults to the current one; missing slot / s6 errors become actionable CLI messages.""" from hermes_cli.service_manager import ( - GatewayNotRegisteredError, S6CommandError, detect_service_manager, get_service_manager, + GatewayNotRegisteredError, detect_service_manager, get_service_manager, + register_unregistered_profile_gateway, ) if detect_service_manager() != "s6": @@ -5871,55 +5872,24 @@ def _dispatch_via_service_manager_if_s6(action: str, profile: str | None = None) mgr = get_service_manager() if action not in ("start", "stop", "restart"): return False + service = f"gateway-{profile}" try: - getattr(mgr, action)(f"gateway-{profile}") - except GatewayNotRegisteredError as exc: - # A profile directory can exist without a slot: it was created against a bind-mounted - # HERMES_HOME from outside the container, where `profile create` cannot reach - # /run/service. Register it here instead of making the operator restart the container - # to let the boot reconciler notice — which is what profiles._maybe_register_gateway_service - # already promises. Anything else stays an actionable error. - if action != "start" or not _register_missing_gateway_slot(mgr, profile): - print(f"✗ {exc}") - sys.exit(1) - except S6CommandError as exc: + try: + getattr(mgr, action)(service) + except GatewayNotRegisteredError: + # A profile created from the HOST against a bind-mounted home has a directory but no + # slot (`profile create` cannot reach the container's /run/service). Only `start` + # repairs that; stop/restart on a missing slot stay an error. + if action != "start" or not register_unregistered_profile_gateway(mgr, profile): + raise + print(f"✓ registered the s6 gateway slot for profile {profile!r}") + mgr.start(service) + except (RuntimeError, ValueError, OSError) as exc: # S6Error is a RuntimeError print(f"✗ {exc}") sys.exit(1) return True -def _register_missing_gateway_slot(mgr, profile: str) -> bool: - """Register and start an s6 slot for an existing-but-unregistered profile. - - Returns False when ``profile`` is not a real profile, leaving the caller's "not registered" - error intact: the guard is the boot reconciler's own marker (``SOUL.md``, seeded by - ``profile create``), so a mistyped ``-p`` name or a stray directory cannot mint a phantom - slot for a profile that does not exist. - - Registers with ``start_now=False`` and then goes through the ordinary ``start`` path, so the - ``desired_state`` write that lets boot reconciliation restore want-up keeps a single owner. - """ - from hermes_cli.service_manager import ( - GatewayNotRegisteredError, S6CommandError, _profile_dir_for_gateway_service, - ) - - service_name = f"gateway-{profile}" - try: - profile_dir = _profile_dir_for_gateway_service(service_name) - except Exception: - return False - if not (profile_dir / "SOUL.md").exists(): - return False - try: - mgr.register_profile_gateway(profile, start_now=False) - mgr.start(service_name) - except (ValueError, RuntimeError, GatewayNotRegisteredError, S6CommandError, OSError) as exc: - print(f"✗ could not register the gateway slot for profile {profile!r}: {exc}") - sys.exit(1) - print(f"✓ registered the s6 gateway slot for profile {profile!r}") - return True - - def _dispatch_all_via_service_manager_if_s6(action: str) -> bool: """Dispatch ``--all`` stop/restart to every registered profile gateway under s6; True iff dispatched. A bare pkill is seen by s6-supervise as a crash and restarted ~1s later; the service manager flips diff --git a/hermes_cli/service_manager.py b/hermes_cli/service_manager.py index 5b924fd3a4..b79e1d22ef 100644 --- a/hermes_cli/service_manager.py +++ b/hermes_cli/service_manager.py @@ -270,6 +270,24 @@ def _write_gateway_desired_state(name: str, desired_state: str) -> None: return +def register_unregistered_profile_gateway(mgr: ServiceManager, profile: str) -> bool: + """Register a ``down`` s6 slot for a profile whose directory exists but was never registered. + + `hermes profile create` can only register a slot when it runs inside the container; created + from the host against a bind-mounted home, the directory lands where the container reads it + but no ``/run/service/gateway-`` exists, and the boot reconciler only notices on the + next container restart. Returns False without touching anything unless the directory carries + ``SOUL.md`` — the reconciler's own "real profile" marker — so a mistyped ``-p`` name cannot + mint a phantom slot. ``start_now=False``: the caller's ordinary ``start`` stays the single + owner of the ``desired_state`` write. + """ + profile_dir = _profile_dir_for_gateway_service(f"{S6_SERVICE_PREFIX}{profile}") + if not (profile_dir / "SOUL.md").exists(): + return False + mgr.register_profile_gateway(profile, start_now=False) + return True + + # s6-overlay installs its binaries under /command/ and only adds it to PATH inside the supervision # tree. Out-of-tree entry points (``docker exec``, the profile create/delete hooks) inherit the base # PATH, so every s6 invocation uses this absolute prefix. Not ``/usr/bin/s6-*``: the diff --git a/tests/hermes_cli/test_gateway_s6_dispatch.py b/tests/hermes_cli/test_gateway_s6_dispatch.py index 7755e8fc8b..2fb9bbb32c 100644 --- a/tests/hermes_cli/test_gateway_s6_dispatch.py +++ b/tests/hermes_cli/test_gateway_s6_dispatch.py @@ -152,107 +152,84 @@ def test_redirect_falls_back_when_sleep_missing( assert "`sleep` is unavailable" in err - - - - # --------------------------------------------------------------------------- -# Lazy slot registration — a profile dir that exists but was never registered +# On-demand slot registration — a profile dir that exists but was never registered (#111720) # --------------------------------------------------------------------------- class _UnregisteredRecorder(_CallRecorder): """Recorder whose slot is missing until ``register_profile_gateway`` runs.""" - def __init__(self, *, register_error: Exception | None = None) -> None: + def __init__(self) -> None: super().__init__() self.registered: list[tuple[str, bool]] = [] - self._register_error = register_error self._slots: set[str] = set() - def start(self, name: str) -> None: + def _svc(self, action: str, name: str) -> None: from hermes_cli.service_manager import GatewayNotRegisteredError if name not in self._slots: raise GatewayNotRegisteredError(name.removeprefix("gateway-")) - self.calls.append(("start", name)) + self.calls.append((action, name)) + + def start(self, name: str) -> None: + self._svc("start", name) + + def stop(self, name: str) -> None: + self._svc("stop", name) def register_profile_gateway(self, profile: str, *, start_now: bool = True) -> None: - if self._register_error is not None: - raise self._register_error self.registered.append((profile, start_now)) self._slots.add(f"gateway-{profile}") -def _arrange(monkeypatch, tmp_path, mgr, *, seed_soul: bool): - """Point the helper at ``tmp_path`` as the profile dir and force the s6 branch.""" +def _arrange(monkeypatch, tmp_path, mgr, *, profile: str, seed_soul: bool): + """Force the s6 branch and make ``tmp_path`` the shared HERMES_HOME the slot maps back to.""" from hermes_cli import gateway as gw from hermes_cli import service_manager as sm monkeypatch.setattr(sm, "detect_service_manager", lambda: "s6") monkeypatch.setattr(sm, "get_service_manager", lambda: mgr) - monkeypatch.setattr(sm, "_profile_dir_for_gateway_service", lambda name: tmp_path) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + profile_dir = tmp_path / "profiles" / profile + profile_dir.mkdir(parents=True) if seed_soul: - (tmp_path / "SOUL.md").write_text("# soul\n", encoding="utf-8") + (profile_dir / "SOUL.md").write_text("# soul\n", encoding="utf-8") return gw def test_start_registers_a_missing_slot_for_a_real_profile(monkeypatch, tmp_path, capsys): - """The bind-mounted-host-create shape: the directory exists (SOUL.md present) but no s6 - slot does. Starting must register and come up, not demand a container restart.""" + """A profile created from the host against a bind-mounted home has a directory (SOUL.md) but + no s6 slot. ``gateway start`` must register it and come up instead of demanding a container + restart; the registration is ``down`` so the ordinary ``start`` owns the desired-state write.""" mgr = _UnregisteredRecorder() - gw = _arrange(monkeypatch, tmp_path, mgr, seed_soul=True) + gw = _arrange(monkeypatch, tmp_path, mgr, profile="coder", seed_soul=True) assert gw._dispatch_via_service_manager_if_s6("start", "coder") is True - # start_now=False + the ordinary start path keeps ONE owner for the desired-state write. assert mgr.registered == [("coder", False)] assert mgr.calls == [("start", "gateway-coder")] assert "registered the s6 gateway slot" in capsys.readouterr().out -def test_start_refuses_to_mint_a_slot_without_the_real_profile_marker(monkeypatch, tmp_path, capsys): - """A mistyped ``-p`` name or a stray directory must keep the original error: SOUL.md is - the boot reconciler's own marker for "this is a real profile".""" +@pytest.mark.parametrize( + ("action", "seed_soul"), + [ + pytest.param("stop", True, id="stop-never-registers"), + pytest.param("start", False, id="no-soul-marker-mints-nothing"), + ], +) +def test_missing_slot_stays_an_error_outside_the_repair_case( + monkeypatch, tmp_path, capsys, action, seed_soul +): + """Only ``start`` on a real profile self-heals: stopping an unregistered profile and starting a + mistyped/stray directory (no SOUL.md) keep the original ✗ + exit 1 and mint no slot.""" mgr = _UnregisteredRecorder() - gw = _arrange(monkeypatch, tmp_path, mgr, seed_soul=False) + gw = _arrange(monkeypatch, tmp_path, mgr, profile="coder", seed_soul=seed_soul) with pytest.raises(SystemExit) as excinfo: - gw._dispatch_via_service_manager_if_s6("start", "codr") + gw._dispatch_via_service_manager_if_s6(action, "coder") assert excinfo.value.code == 1 assert mgr.registered == [] - assert "✗" in capsys.readouterr().out - - -def test_stop_never_registers_a_slot(monkeypatch, tmp_path, capsys): - """Only ``start`` self-heals. Stopping a profile that has no slot is still an error — - registering one just to stop it would be absurd.""" - mgr = _UnregisteredRecorder() - - def _stop(name: str) -> None: - from hermes_cli.service_manager import GatewayNotRegisteredError - raise GatewayNotRegisteredError(name.removeprefix("gateway-")) - - mgr.stop = _stop # type: ignore[method-assign] - gw = _arrange(monkeypatch, tmp_path, mgr, seed_soul=True) - - with pytest.raises(SystemExit) as excinfo: - gw._dispatch_via_service_manager_if_s6("stop", "coder") - - assert excinfo.value.code == 1 - assert mgr.registered == [] - - -def test_registration_failure_is_an_actionable_error_not_a_traceback(monkeypatch, tmp_path, capsys): - """``register_profile_gateway`` raises RuntimeError when s6-svscanctl fails and ValueError - on a slot that appeared underneath us. Both must surface as ✗ + exit 1.""" - mgr = _UnregisteredRecorder(register_error=RuntimeError("s6-svscanctl failed: no scandir")) - gw = _arrange(monkeypatch, tmp_path, mgr, seed_soul=True) - - with pytest.raises(SystemExit) as excinfo: - gw._dispatch_via_service_manager_if_s6("start", "coder") - - assert excinfo.value.code == 1 - out = capsys.readouterr().out - assert "could not register the gateway slot" in out - assert "s6-svscanctl failed" in out + assert mgr.calls == [] + assert "no such gateway 'coder'" in capsys.readouterr().out diff --git a/website/docs/user-guide/docker.md b/website/docs/user-guide/docker.md index 827b94d9c3..5d073664ce 100644 --- a/website/docs/user-guide/docker.md +++ b/website/docs/user-guide/docker.md @@ -235,6 +235,8 @@ Each profile created with `hermes profile create ` gets: - Per-profile rotated logs at `${HERMES_HOME}/logs/gateways//current` (10 archives × 1 MB each). - State persistence across container restarts: the boot-time reconciler reads `gateway_state.json` from each profile directory and brings the slot back up only for profiles whose last recorded state was `running`. Only a gateway you explicitly stopped (`hermes gateway stop`) stays down across a restart — a container restart, image upgrade, or unexpected exit leaves the recorded state as `running`, so the gateway auto-starts on the next boot. +A profile created from the **host** against a bind-mounted `~/.hermes` gets its directory but no slot (the host process cannot reach the container's `/run/service`). Inside the container, `hermes -p gateway start` registers the missing slot on demand and starts it — no `docker restart` needed. Only `start` does this, and only for a real profile directory (one carrying `SOUL.md`); `stop`/`restart` on an unregistered profile and a mistyped `-p` name still fail with `✗ no such gateway`. + The lifecycle commands you'd run on the host work the same way from inside the container: ```sh