mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-30 06:41:51 +00:00
fix(service_manager): rip out dead port parameter
PR #30136 review caught: `_allocate_gateway_port()` in profiles.py computed a SHA-256-derived port that was threaded through `register_profile_gateway(profile, port=N)` → `_render_run_script(profile, port, extra_env)` → and then **ignored**. The rendered run script picked the bind port from the profile's config.yaml (`[gateway] port = …`), never from the allocator. So the entire allocator + parameter chain was dead code. Remove: * `hermes_cli.profiles._allocate_gateway_port` (deterministic SHA-256 → [9200, 9800) — never used). * `port` kwarg from `ServiceManager.register_profile_gateway` (Protocol + Mixin + S6 implementation). * `port` positional arg from `_render_run_script(profile, port, extra_env)` — now `_render_run_script(profile, extra_env)`. * The pass-through call in `profiles._maybe_register_gateway_service`. config.yaml is now the single source of truth for gateway port selection — matches reality and reduces the API surface. Three explanatory comments in service_manager.py / profiles.py document the retirement so future readers don't reach for the allocator and find a ghost. Tests: drop the three `_allocate_gateway_port` tests; update fakes' signatures throughout test_service_manager.py and test_profiles_s6_hooks.py to match the new no-port API.
This commit is contained in:
parent
7b16e4448a
commit
8b6733ebe2
6 changed files with 36 additions and 91 deletions
|
|
@ -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).
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue