diff --git a/hermes_cli/container_boot.py b/hermes_cli/container_boot.py index 2cc9c306fd2..66f8f51766e 100644 --- a/hermes_cli/container_boot.py +++ b/hermes_cli/container_boot.py @@ -198,7 +198,7 @@ def _register_service(scandir: Path, profile: str, *, start: bool) -> None: # env can set it via the profile's config.yaml (which the gateway # itself loads). run = service_dir / "run" - run.write_text(S6ServiceManager._render_run_script(profile, port=0, extra_env={})) + run.write_text(S6ServiceManager._render_run_script(profile, extra_env={})) run.chmod(0o755) # Persistent log rotation (OQ8-C). diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index 3031fa3867b..e6979320afd 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -977,26 +977,6 @@ def delete_profile(name: str, yes: bool = False) -> Path: return profile_dir -def _allocate_gateway_port(profile_name: str) -> int: - """Deterministic port allocation for a profile's s6-supervised gateway. - - Phase 4 of the s6-overlay supervision plan. Ports live in - [9200, 9800) — a 600-port window starting just past the dashboard - default (9119). Allocation is deterministic via SHA-256 of the - profile name so the same profile always gets the same port across - container restarts. - - Collision probability is small (~1/600 per pair of profiles); if - it happens the gateway will fail to bind with a clear OSError and - the caller can set ``HERMES_GATEWAY_PORT`` to override. The - Phase 4 plan accepts this rather than carrying explicit allocator - state in the persistent volume. - """ - import hashlib - h = int(hashlib.sha256(profile_name.encode()).hexdigest()[:8], 16) - return 9200 + (h % 600) - - def _maybe_register_gateway_service(profile_name: str) -> None: """Register a profile's gateway with s6 inside the container. @@ -1004,11 +984,16 @@ def _maybe_register_gateway_service(profile_name: str) -> None: ``NotImplementedError`` on ``register_profile_gateway`` and the existing per-profile unit-generation paths handle lifecycle. - Best-effort: any error (no backend detected, port collision, s6 - not yet ready, etc.) is logged and swallowed so profile creation - doesn't fail because the s6 supervision tree is in a weird state. - The user can re-register manually later via the gateway start - command, which goes through the same dispatch path. + Best-effort: any error (no backend detected, s6 not yet ready, + etc.) is logged and swallowed so profile creation doesn't fail + because the s6 supervision tree is in a weird state. The user + can re-register manually later via the gateway start command, + which goes through the same dispatch path. + + Port selection is governed by the profile's ``config.yaml`` + (``[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). """ try: from hermes_cli.service_manager import get_service_manager @@ -1017,9 +1002,8 @@ def _maybe_register_gateway_service(profile_name: str) -> None: return # no backend on this host — nothing to do if not mgr.supports_runtime_registration(): return # host backend; no-op - port = _allocate_gateway_port(profile_name) try: - mgr.register_profile_gateway(profile_name, port=port) + mgr.register_profile_gateway(profile_name) except ValueError: # Already registered (e.g. the container-boot reconciler ran # first and brought up a stale slot). That's fine. diff --git a/hermes_cli/service_manager.py b/hermes_cli/service_manager.py index f8f99051317..22aa08c4479 100644 --- a/hermes_cli/service_manager.py +++ b/hermes_cli/service_manager.py @@ -76,7 +76,6 @@ class ServiceManager(Protocol): self, profile: str, *, - port: int, extra_env: dict[str, str] | None = None, ) -> None: ... def unregister_profile_gateway(self, profile: str) -> None: ... @@ -175,7 +174,6 @@ class _RegistrationUnsupportedMixin: self, profile: str, *, - port: int, extra_env: dict[str, str] | None = None, ) -> None: raise NotImplementedError( @@ -421,7 +419,6 @@ class S6ServiceManager: @staticmethod def _render_run_script( profile: str, - port: int, extra_env: dict[str, str], ) -> str: """Generate the run script for a profile-gateway s6 service. @@ -446,16 +443,15 @@ class S6ServiceManager: would instead look up ``$HERMES_HOME/profiles/default/`` — a completely different (and almost always nonexistent) profile. - Note: the ``port`` parameter is accepted for API parity with - :meth:`register_profile_gateway` but is currently ignored — the - gateway picks its bind port from the profile's config.yaml - (``[gateway] port = ...``). A future signature change may carry - it through as an ``HERMES_GATEWAY_PORT`` env var; until then, - the in-config value wins and the constructor's ``port`` arg - is essentially documentation for "what port the profile would - use if we wired it through". See Phase 4 Task 4.1 for the - deterministic allocator and the SHA-256-derived range - [9200, 9800). + Port selection: the gateway picks its bind port from the + profile's ``config.yaml`` (``[gateway] port = ...``) — that + is the single source of truth. Previously this method took a + ``port`` parameter that was passed in but never substituted + into the rendered script (it was carried in for "API parity" + with a deterministic SHA-256 allocator in + ``hermes_cli.profiles._allocate_gateway_port``). PR #30136 + review item I5 retired both the allocator and the parameter + because they were dead code through the entire stack. """ import shlex lines = [ @@ -592,7 +588,6 @@ class S6ServiceManager: self, profile: str, *, - port: int, extra_env: dict[str, str] | None = None, ) -> None: """Create the s6 service directory for a profile gateway. @@ -629,7 +624,7 @@ class S6ServiceManager: try: (tmp_dir / "type").write_text("longrun\n") - run_script = self._render_run_script(profile, port, extra_env or {}) + run_script = self._render_run_script(profile, extra_env or {}) run_path = tmp_dir / "run" run_path.write_text(run_script) run_path.chmod(0o755) diff --git a/tests/docker/test_s6_profile_gateway_integration.py b/tests/docker/test_s6_profile_gateway_integration.py index 103664e2895..22b41ca5ace 100644 --- a/tests/docker/test_s6_profile_gateway_integration.py +++ b/tests/docker/test_s6_profile_gateway_integration.py @@ -29,7 +29,7 @@ _REGISTER_SCRIPT = """ import sys sys.path.insert(0, "/opt/hermes") from hermes_cli.service_manager import S6ServiceManager -S6ServiceManager().register_profile_gateway("phase3test", port=9301) +S6ServiceManager().register_profile_gateway("phase3test") # Don't worry about whether the gateway actually starts — we only care # that the supervision slot was created. The gateway run script will # likely error out (no profile config exists) but that's expected. diff --git a/tests/hermes_cli/test_profiles_s6_hooks.py b/tests/hermes_cli/test_profiles_s6_hooks.py index 73a25f90d8f..c0ce1d0b189 100644 --- a/tests/hermes_cli/test_profiles_s6_hooks.py +++ b/tests/hermes_cli/test_profiles_s6_hooks.py @@ -1,6 +1,6 @@ """Tests for the Phase 4 s6 hooks in hermes_cli.profiles. -Specifically: _allocate_gateway_port, _maybe_register_gateway_service, +Specifically: _maybe_register_gateway_service, _maybe_unregister_gateway_service. The integration with create_profile and delete_profile is covered indirectly by the existing TestCreateProfile and TestDeleteProfile classes in @@ -14,42 +14,11 @@ from typing import Any import pytest from hermes_cli.profiles import ( - _allocate_gateway_port, _maybe_register_gateway_service, _maybe_unregister_gateway_service, ) -# --------------------------------------------------------------------------- -# _allocate_gateway_port -# --------------------------------------------------------------------------- - - -def test_allocate_gateway_port_is_deterministic() -> None: - """Same profile name → same port across calls. This matters because - a profile's gateway must come back up on the same port across - container restarts.""" - a = _allocate_gateway_port("coder") - b = _allocate_gateway_port("coder") - assert a == b - - -def test_allocate_gateway_port_in_advertised_range() -> None: - """[9200, 9800) — the window the helper's docstring promises.""" - for name in ("a", "b", "coder", "assistant", "very-long-profile-name-here"): - port = _allocate_gateway_port(name) - assert 9200 <= port < 9800, f"{name} got {port}" - - -def test_allocate_gateway_port_distributes_across_range() -> None: - """Sanity check: ports for ~100 random-ish names should land in - enough distinct buckets that the distribution is plausibly uniform. - Catches accidental hash truncation that would collapse the range.""" - ports = {_allocate_gateway_port(f"profile-{i}") for i in range(100)} - # 100 inputs mapped into 600 slots — expect at least ~60 distinct. - assert len(ports) >= 60, f"Only {len(ports)} distinct ports across 100 names" - - # --------------------------------------------------------------------------- # _maybe_register_gateway_service / _maybe_unregister_gateway_service # --------------------------------------------------------------------------- @@ -74,7 +43,7 @@ class _S6Manager: kind = "s6" def __init__(self) -> None: - self.registered: list[tuple[str, int]] = [] + self.registered: list[str] = [] self.unregistered: list[str] = [] self.raise_on_register: Exception | None = None self.raise_on_unregister: Exception | None = None @@ -83,12 +52,12 @@ class _S6Manager: return True def register_profile_gateway( - self, profile: str, *, port: int, + self, profile: str, *, extra_env: dict[str, str] | None = None, ) -> None: if self.raise_on_register is not None: raise self.raise_on_register - self.registered.append((profile, port)) + self.registered.append(profile) def unregister_profile_gateway(self, profile: str) -> None: if self.raise_on_unregister is not None: @@ -111,10 +80,7 @@ def test_register_calls_through_on_s6(monkeypatch: pytest.MonkeyPatch) -> None: "hermes_cli.service_manager.get_service_manager", lambda: mgr, ) _maybe_register_gateway_service("coder") - assert len(mgr.registered) == 1 - profile, port = mgr.registered[0] - assert profile == "coder" - assert 9200 <= port < 9800 + assert mgr.registered == ["coder"] def test_register_swallows_duplicate_value_error( diff --git a/tests/hermes_cli/test_service_manager.py b/tests/hermes_cli/test_service_manager.py index e9c85f33267..b05c02c01a8 100644 --- a/tests/hermes_cli/test_service_manager.py +++ b/tests/hermes_cli/test_service_manager.py @@ -174,7 +174,7 @@ def test_systemd_manager_kind_and_registration_unsupported() -> None: assert mgr.kind == "systemd" assert mgr.supports_runtime_registration() is False with pytest.raises(NotImplementedError): - mgr.register_profile_gateway("foo", port=9100) + mgr.register_profile_gateway("foo") with pytest.raises(NotImplementedError): mgr.unregister_profile_gateway("foo") assert mgr.list_profile_gateways() == [] @@ -187,7 +187,7 @@ def test_launchd_manager_kind_and_registration_unsupported() -> None: assert mgr.kind == "launchd" assert mgr.supports_runtime_registration() is False with pytest.raises(NotImplementedError): - mgr.register_profile_gateway("foo", port=9100) + mgr.register_profile_gateway("foo") assert mgr.list_profile_gateways() == [] assert isinstance(mgr, ServiceManager) @@ -197,7 +197,7 @@ def test_windows_manager_kind_and_registration_unsupported() -> None: assert mgr.kind == "windows" assert mgr.supports_runtime_registration() is False with pytest.raises(NotImplementedError): - mgr.register_profile_gateway("foo", port=9100) + mgr.register_profile_gateway("foo") assert isinstance(mgr, ServiceManager) @@ -417,7 +417,7 @@ def test_s6_register_creates_service_dir_and_triggers_scan( ) -> None: from hermes_cli.service_manager import S6ServiceManager mgr = S6ServiceManager(scandir=s6_scandir) - mgr.register_profile_gateway("coder", port=9150) + mgr.register_profile_gateway("coder") svc_dir = s6_scandir / "gateway-coder" assert svc_dir.is_dir() @@ -454,7 +454,7 @@ def test_s6_register_extra_env_is_quoted(s6_scandir, fake_subprocess_run) -> Non from hermes_cli.service_manager import S6ServiceManager mgr = S6ServiceManager(scandir=s6_scandir) mgr.register_profile_gateway( - "x", port=9300, extra_env={"FOO": "bar baz", "QUOTED": "a'b"}, + "x", extra_env={"FOO": "bar baz", "QUOTED": "a'b"}, ) run_text = (s6_scandir / "gateway-x" / "run").read_text() # shlex.quote should have wrapped both values @@ -466,7 +466,7 @@ def test_s6_register_rejects_invalid_profile_name(s6_scandir) -> None: from hermes_cli.service_manager import S6ServiceManager mgr = S6ServiceManager(scandir=s6_scandir) with pytest.raises(ValueError): - mgr.register_profile_gateway("Bad/Name", port=9100) + mgr.register_profile_gateway("Bad/Name") def test_s6_register_rejects_duplicate(s6_scandir, fake_subprocess_run) -> None: @@ -474,7 +474,7 @@ def test_s6_register_rejects_duplicate(s6_scandir, fake_subprocess_run) -> None: mgr = S6ServiceManager(scandir=s6_scandir) (s6_scandir / "gateway-coder").mkdir(parents=True) with pytest.raises(ValueError, match="already registered"): - mgr.register_profile_gateway("coder", port=9150) + mgr.register_profile_gateway("coder") def test_s6_register_rolls_back_on_svscanctl_failure( @@ -494,7 +494,7 @@ def test_s6_register_rolls_back_on_svscanctl_failure( mgr = S6ServiceManager(scandir=s6_scandir) with pytest.raises(RuntimeError, match="s6-svscanctl failed"): - mgr.register_profile_gateway("coder", port=9150) + mgr.register_profile_gateway("coder") assert not (s6_scandir / "gateway-coder").exists()