mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-28 18:19:28 +00:00
fix(api_server): mark rejected API_SERVER_KEY as non-retryable fatal error
When _api_key_passes_startup_guard() rejects the key (missing, placeholder/too short, or fail-closed unverifiable strength), connect() returned a bare False with no fatal-error info. gateway.run's reconnect watcher treats that as transient and re-queues with backoff forever — each retry re-instantiating the adapter and its ResponseStore sqlite connection. Observed in production (#37011): ~501 leaked connections (1002 fds) over ~2.5 days until EMFILE made the whole gateway unresponsive. Set a non-retryable fatal error (api_server_key_invalid) in connect() when the guard rejects, covering all three rejection branches, so the platform drops from the reconnect queue; recover with `/platform resume api_server` after fixing the key. Same treatment as the port-conflict guard (api_server_port_in_use, #65665 /bda8bd76a8). Tests mirror the port-conflict precedent: each rejection path asserts connect() is False, has_fatal_error True, fatal_error_retryable False, and fatal_error_code api_server_key_invalid, plus a strong-key control. Re-implementation of #38803 by @cifangyiquan against current main — their patch targeted the old inline guard in connect() which was since extracted to _api_key_passes_startup_guard() (and gained the fail-closed branch in683059feb5), so the original diff no longer applies. Their production diagnosis and non-retryable direction preserved. Refs: #38803, #37011
This commit is contained in:
parent
9e4b89857a
commit
089f09fa5f
3 changed files with 94 additions and 0 deletions
1
contributors/emails/lg_329@163.com
Normal file
1
contributors/emails/lg_329@163.com
Normal file
|
|
@ -0,0 +1 @@
|
||||||
|
cifangyiquan
|
||||||
|
|
@ -5526,6 +5526,26 @@ class APIServerAdapter(BasePlatformAdapter):
|
||||||
return False
|
return False
|
||||||
|
|
||||||
if not self._api_key_passes_startup_guard():
|
if not self._api_key_passes_startup_guard():
|
||||||
|
# A rejected API_SERVER_KEY is a configuration error, not a
|
||||||
|
# transient blip — the key will not become valid on its own. A
|
||||||
|
# bare ``return False`` makes the reconnect watcher in
|
||||||
|
# gateway.run treat it as retryable and loop forever at the
|
||||||
|
# backoff cap, re-instantiating the adapter (and its
|
||||||
|
# ResponseStore sqlite connection) every retry (#38803: ~501
|
||||||
|
# leaked connections / 1002 fds over 2.5 days until EMFILE took
|
||||||
|
# the whole gateway down). Non-retryable drops it from the
|
||||||
|
# reconnect queue — same treatment as the port-conflict guard
|
||||||
|
# (api_server_port_in_use). The guard already logged the
|
||||||
|
# specific rejection reason just above.
|
||||||
|
self._set_fatal_error(
|
||||||
|
"api_server_key_invalid",
|
||||||
|
"API_SERVER_KEY was rejected by the startup guard (missing, "
|
||||||
|
"placeholder/too short, or strength unverifiable — see the "
|
||||||
|
"error logged above). Generate a strong secret (e.g. "
|
||||||
|
"`openssl rand -hex 32`), set API_SERVER_KEY, then "
|
||||||
|
"`/platform resume api_server`.",
|
||||||
|
retryable=False,
|
||||||
|
)
|
||||||
return False
|
return False
|
||||||
|
|
||||||
try:
|
try:
|
||||||
|
|
|
||||||
|
|
@ -4868,3 +4868,76 @@ class TestApiKeyStartupGuardFailsClosed:
|
||||||
def test_missing_key_still_refused(self):
|
def test_missing_key_still_refused(self):
|
||||||
"""Control: the empty-key branch is unchanged."""
|
"""Control: the empty-key branch is unchanged."""
|
||||||
assert self._guard("") is False
|
assert self._guard("") is False
|
||||||
|
|
||||||
|
|
||||||
|
class TestKeyRejectionSetsNonRetryableFatalError:
|
||||||
|
"""Each startup-guard rejection must set a non-retryable fatal error so
|
||||||
|
the reconnect watcher drops the platform from the retry queue instead of
|
||||||
|
looping indefinitely.
|
||||||
|
|
||||||
|
Previously connect() returned bare ``False``, which gateway.run treated
|
||||||
|
as retryable — re-queueing every backoff interval forever and
|
||||||
|
re-instantiating the adapter (with its ResponseStore sqlite connection)
|
||||||
|
each retry (#38803: ~501 leaked connections / 1002 fds over 2.5 days,
|
||||||
|
ending in EMFILE for the whole gateway). Mirrors the port-conflict
|
||||||
|
precedent (test_port_conflict_sets_non_retryable_fatal_error, #65665).
|
||||||
|
"""
|
||||||
|
|
||||||
|
@staticmethod
|
||||||
|
def _make_adapter(key, monkeypatch):
|
||||||
|
monkeypatch.delenv("API_SERVER_KEY", raising=False)
|
||||||
|
return APIServerAdapter(
|
||||||
|
PlatformConfig(
|
||||||
|
enabled=True,
|
||||||
|
extra={"host": "127.0.0.1", "port": 0, "key": key},
|
||||||
|
)
|
||||||
|
)
|
||||||
|
|
||||||
|
@staticmethod
|
||||||
|
async def _assert_key_rejection_is_fatal(adapter):
|
||||||
|
try:
|
||||||
|
assert await adapter.connect() is False
|
||||||
|
assert adapter.has_fatal_error is True
|
||||||
|
assert adapter.fatal_error_retryable is False
|
||||||
|
assert adapter.fatal_error_code == "api_server_key_invalid"
|
||||||
|
assert "API_SERVER_KEY" in (adapter.fatal_error_message or "")
|
||||||
|
finally:
|
||||||
|
await adapter.disconnect()
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_missing_key_sets_non_retryable_fatal_error(self, monkeypatch):
|
||||||
|
adapter = self._make_adapter("", monkeypatch)
|
||||||
|
await self._assert_key_rejection_is_fatal(adapter)
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_weak_key_sets_non_retryable_fatal_error(self, monkeypatch):
|
||||||
|
"""Placeholder / <16-char keys are rejected by has_usable_secret."""
|
||||||
|
adapter = self._make_adapter("test", monkeypatch)
|
||||||
|
await self._assert_key_rejection_is_fatal(adapter)
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_unverifiable_key_sets_non_retryable_fatal_error(self, monkeypatch):
|
||||||
|
"""The fail-closed branch: a strong key whose strength cannot be
|
||||||
|
verified (hermes_cli.auth unimportable) must also be non-retryable —
|
||||||
|
the install won't repair itself between retries."""
|
||||||
|
adapter = self._make_adapter("a" * 40, monkeypatch)
|
||||||
|
real_import = __import__
|
||||||
|
|
||||||
|
def _blocked(name, *args, **kwargs):
|
||||||
|
if name == "hermes_cli.auth":
|
||||||
|
raise ImportError("simulated: hermes_cli.auth unavailable")
|
||||||
|
return real_import(name, *args, **kwargs)
|
||||||
|
|
||||||
|
with patch("builtins.__import__", _blocked):
|
||||||
|
await self._assert_key_rejection_is_fatal(adapter)
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_strong_key_leaves_no_fatal_error(self, monkeypatch):
|
||||||
|
"""Control: a successful connect() must not carry a fatal error."""
|
||||||
|
adapter = self._make_adapter("a" * 40, monkeypatch)
|
||||||
|
try:
|
||||||
|
assert await adapter.connect() is True
|
||||||
|
assert adapter.has_fatal_error is False
|
||||||
|
assert adapter.fatal_error_retryable is True
|
||||||
|
finally:
|
||||||
|
await adapter.disconnect()
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue