diff --git a/CHANGELOG.md b/CHANGELOG.md index e1acb687..d5f41786 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,11 @@ ## [Unreleased] +## [v0.51.225] — 2026-06-03 — Release GS (stage-p7 — remote gateway health probe resolves gateway_state) + +### Fixed +- The remote-gateway health probe now correctly reports `gateway_state`, so the Tasks/Cron banner lights up for Docker / remote-gateway deployments. The probe previously hit `/health` and `/status` (neither returns `gateway_state`) and never queried `/health/detailed` (which does), so `gateway_state == "running"` was never observed remotely. The probe now tries `/health/detailed` first, parses the JSON body of a 2xx response to extract `gateway_state`, and unifies the gateway base-URL env precedence to `GATEWAY_HEALTH_URL` > `HERMES_GATEWAY_HEALTH_URL` > `HERMES_API_URL` (#3355, @rodboev). + ## [v0.51.224] — 2026-06-03 — Release GR (stage-p6 — profile tool/skill config authoritative on the streaming worker) ### Fixed diff --git a/api/agent_health.py b/api/agent_health.py index ef68f101..44cbd46e 100644 --- a/api/agent_health.py +++ b/api/agent_health.py @@ -306,38 +306,63 @@ def _runtime_detail_subset(runtime_status: dict[str, Any] | None) -> dict[str, A _REMOTE_PROBE_TIMEOUT_S: float = 2.0 _REMOTE_PROBE_CACHE_TTL_S: float = 5.0 -_REMOTE_PROBE_PATHS: tuple[str, ...] = ("/health", "/status", "/api/gateway/status") +_REMOTE_PROBE_PATHS: tuple[str, ...] = ("/health/detailed", "/health", "/v1/health") +# A gateway health payload is small JSON; cap the 2xx body read so a large or +# slow-trickled remote response can't hang /api/health/agent or balloon memory. +_REMOTE_PROBE_BODY_LIMIT_BYTES: int = 64 * 1024 _remote_probe_lock = threading.Lock() _remote_probe_cache: dict[str, Any] = {"url": None, "expires_at": 0.0, "result": None} def _remote_gateway_base_url() -> str | None: - raw = os.environ.get("HERMES_API_URL") - if not isinstance(raw, str): - return None - url = raw.strip() - if not url: - return None - return url.rstrip("/") + """Return an explicit remote gateway base URL, or None for local-only setups. + + Priority: GATEWAY_HEALTH_URL > HERMES_GATEWAY_HEALTH_URL > HERMES_API_URL. + Returns ``None`` when no env var is set so the caller falls through to + local PID/state checks. + + Any of these env vars may legitimately point AT a health endpoint + (e.g. ``GATEWAY_HEALTH_URL=http://host:8642/health``). Since the probe + appends ``/health/detailed`` etc. to the returned base, strip a trailing + health-path suffix first so we don't build ``/health/health/detailed`` + (mirrors the normalization in api/updates.py). + """ + for var in ("GATEWAY_HEALTH_URL", "HERMES_GATEWAY_HEALTH_URL", "HERMES_API_URL"): + val = os.environ.get(var, "").strip() + if val: + base = val.rstrip("/") + for suffix in ("/health/detailed", "/health", "/v1/health", "/status"): + if base.endswith(suffix): + base = base[: -len(suffix)].rstrip("/") + break + return base + return None -def _http_probe(url: str, timeout_s: float) -> tuple[bool, int | None, str | None]: - """GET ``url`` and return (ok, status_code, error_name). +def _http_probe(url: str, timeout_s: float) -> tuple[bool, int | None, str | None, bytes | None]: + """GET ``url`` and return (ok, status_code, error_name, body). ``ok`` is True only for a 2xx response. 5xx and network errors are not OK. 4xx is also treated as "responded" (the gateway is up, just answering 404 on this particular path) so the caller can move on to the next path. + ``body`` is the raw response bytes for 2xx responses, None otherwise. """ req = urllib_request.Request(url, method="GET") try: with urllib_request.urlopen(req, timeout=timeout_s) as resp: # noqa: S310 - trusted env var URL status = getattr(resp, "status", None) or resp.getcode() - return (200 <= int(status) < 300, int(status), None) + ok = 200 <= int(status) < 300 + # Cap the body read: we only need a small JSON health payload, and an + # unbounded resp.read() on a large/trickled 2xx body could hang the + # /api/health/agent handler or balloon memory. Read one byte over the + # cap so the caller can detect (and skip) an oversized body. + body = resp.read(_REMOTE_PROBE_BODY_LIMIT_BYTES + 1) if ok else None + return (ok, int(status), None, body) except urllib_error.HTTPError as exc: - return (False, int(exc.code), "HTTPError") + return (False, int(exc.code), "HTTPError", None) except Exception as exc: # urllib_error.URLError, socket.timeout, ssl, etc. - return (False, None, type(exc).__name__) + return (False, None, type(exc).__name__, None) def _probe_remote_gateway(base_url: str, *, now: float | None = None) -> dict[str, Any]: @@ -360,17 +385,28 @@ def _probe_remote_gateway(base_url: str, *, now: float | None = None) -> dict[st last_status: int | None = None last_error: str | None = None for path in _REMOTE_PROBE_PATHS: - ok, status, err = _http_probe(base_url + path, _REMOTE_PROBE_TIMEOUT_S) + ok, status, err, body = _http_probe(base_url + path, _REMOTE_PROBE_TIMEOUT_S) if ok: + details: dict[str, Any] = { + "state": "alive", + "reason": "remote_gateway", + "endpoint": base_url + path, + "status_code": status, + } + if body and len(body) <= _REMOTE_PROBE_BODY_LIMIT_BYTES: + try: + data = json.loads(body) + if isinstance(data, dict) and "gateway_state" in data: + details["gateway_state"] = data["gateway_state"] + except (json.JSONDecodeError, UnicodeDecodeError): + pass + # An over-cap body (len > limit, i.e. the +1 sentinel byte was read) + # is treated as "alive but no parseable gateway_state" — we still + # report the gateway as up, just without the detailed state. payload = { "alive": True, "checked_at": _checked_at(), - "details": { - "state": "alive", - "reason": "remote_gateway", - "endpoint": base_url + path, - "status_code": status, - }, + "details": details, } break # Remember the most informative failure signal we saw. diff --git a/tests/test_agent_health_remote.py b/tests/test_agent_health_remote.py index b6c7b0ef..a3ebde33 100644 --- a/tests/test_agent_health_remote.py +++ b/tests/test_agent_health_remote.py @@ -1,6 +1,7 @@ -"""Tests for HERMES_API_URL remote gateway probe (#3281).""" +"""Tests for HERMES_API_URL remote gateway probe (#3281, #3355).""" from __future__ import annotations +import json from unittest import mock import pytest @@ -16,14 +17,17 @@ def _clear_cache(): class _FakeResp: - def __init__(self, status: int = 200): + def __init__(self, status: int = 200, body: bytes = b""): self.status = status + self._body = body def getcode(self) -> int: return self.status - def read(self) -> bytes: - return b"" + def read(self, amt: int | None = None) -> bytes: + if amt is None: + return self._body + return self._body[:amt] def __enter__(self): return self @@ -99,3 +103,121 @@ def test_remote_probe_result_cached_for_5s(monkeypatch): assert call_count["n"] == 1 # checked_at is refreshed even on cache hit so the UI shows a current time. assert "checked_at" in second + + +# ── #3355: gateway_state extraction and probe-order tests ───────────────── + + +def test_gateway_state_populated_from_health_detailed(monkeypatch): + """Remote probe should extract gateway_state from /health/detailed JSON body.""" + monkeypatch.setenv("HERMES_API_URL", "http://fake-gateway:8642") + body = json.dumps({"gateway_state": "running", "uptime": 3600}).encode() + + def fake_urlopen(req, timeout=None): + return _FakeResp(200, body=body) + + with mock.patch.object(agent_health.urllib_request, "urlopen", fake_urlopen): + payload = agent_health.build_agent_health_payload() + + assert payload["alive"] is True + assert payload["details"]["gateway_state"] == "running" + assert payload["details"]["reason"] == "remote_gateway" + + +def test_probe_order_prefers_health_detailed(monkeypatch): + """First probe path should be /health/detailed so gateway_state is available.""" + monkeypatch.setenv("HERMES_API_URL", "http://fake-gateway:8642") + probed_urls: list[str] = [] + + def fake_urlopen(req, timeout=None): + probed_urls.append(req.full_url) + # Return 200 on first hit so the loop stops immediately + return _FakeResp(200, body=b'{"gateway_state": "running"}') + + with mock.patch.object(agent_health.urllib_request, "urlopen", fake_urlopen): + agent_health.build_agent_health_payload() + + assert len(probed_urls) == 1 + assert probed_urls[0] == "http://fake-gateway:8642/health/detailed" + + +def test_gateway_health_url_env_used(monkeypatch): + """GATEWAY_HEALTH_URL should be respected by the remote probe.""" + monkeypatch.delenv("HERMES_API_URL", raising=False) + monkeypatch.delenv("HERMES_GATEWAY_HEALTH_URL", raising=False) + monkeypatch.setenv("GATEWAY_HEALTH_URL", "http://custom:9999") + probed_urls: list[str] = [] + + def fake_urlopen(req, timeout=None): + probed_urls.append(req.full_url) + return _FakeResp(200, body=b'{}') + + with mock.patch.object(agent_health.urllib_request, "urlopen", fake_urlopen): + payload = agent_health.build_agent_health_payload() + + assert payload["alive"] is True + assert probed_urls[0].startswith("http://custom:9999/") + + +def test_default_url_when_no_env(monkeypatch): + """Without any gateway env vars, should fall back to local detection (not remote).""" + monkeypatch.delenv("HERMES_API_URL", raising=False) + monkeypatch.delenv("GATEWAY_HEALTH_URL", raising=False) + monkeypatch.delenv("HERMES_GATEWAY_HEALTH_URL", raising=False) + + # Force the local importlib path to fail so we hit "gateway_status_unavailable", + # proving the remote probe was NOT invoked. + def boom(name): + raise ModuleNotFoundError(name) + + with mock.patch.object(agent_health.importlib, "import_module", boom): + payload = agent_health.build_agent_health_payload() + + assert payload["alive"] is None + assert payload["details"]["reason"] == "gateway_status_unavailable" + + +def test_gateway_health_url_with_health_suffix_is_normalized(monkeypatch): + """A gateway env var that already points AT a health path must not produce + a doubled path like /health/health/detailed (#3355 Codex follow-up).""" + monkeypatch.delenv("HERMES_API_URL", raising=False) + monkeypatch.delenv("HERMES_GATEWAY_HEALTH_URL", raising=False) + monkeypatch.setenv("GATEWAY_HEALTH_URL", "http://gw:8642/health") + probed: list[str] = [] + + def fake_urlopen(req, timeout=None): + probed.append(req.full_url) + if req.full_url == "http://gw:8642/health/detailed": + return _FakeResp(200, body=json.dumps({"gateway_state": "running"}).encode()) + return _FakeResp(404) + + with mock.patch.object(agent_health.urllib_request, "urlopen", fake_urlopen): + payload = agent_health.build_agent_health_payload() + + assert all("/health/health" not in u for u in probed), probed + assert "http://gw:8642/health/detailed" in probed + assert payload["alive"] is True + assert payload["details"].get("gateway_state") == "running" + + +def test_oversized_remote_body_does_not_hang_and_skips_parse(monkeypatch): + """A 2xx response with a huge body must be capped (not read unbounded) and + still report the gateway alive, just without a parsed gateway_state.""" + monkeypatch.setenv("HERMES_API_URL", "http://gateway:8080") + huge = b"x" * (agent_health._REMOTE_PROBE_BODY_LIMIT_BYTES * 4) + captured_amt: list[int | None] = [] + + class _HugeResp(_FakeResp): + def read(self, amt: int | None = None): + captured_amt.append(amt) + return huge[:amt] if amt is not None else huge + + def fake_urlopen(req, timeout=None): + return _HugeResp(200, body=huge) + + with mock.patch.object(agent_health.urllib_request, "urlopen", fake_urlopen): + payload = agent_health.build_agent_health_payload() + + assert captured_amt and all(a is not None for a in captured_amt) + assert payload["alive"] is True + assert "gateway_state" not in payload["details"]