mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
fix(model_metadata): address Phase-2 review findings on probe caches
Structured review (2a/2b/2c) findings, all fixed:
- MAJOR: detect_local_server_type memo was process-lifetime with no
invalidation, permanently pinning a URL's server type. Now a bounded
1h TTL ((type, monotonic) tuples) so a backend swap on the same port
is re-detected. Test covers ollama->lm-studio swap after expiry.
- MAJOR: legacy disk-row compat was one-way. get_cached_context_length
and _invalidate_cached_context_length now consult the same key-shape
set {canonical, literal, canonical+slash} in both directions, so an
old slashed row is found (and cleared) when the runtime passes the
normalized URL. Tests pin both migration directions.
- MINOR: _localhost_to_ipv4 did whole-string replacement, which could
corrupt a proxy URL embedding http://localhost in its query. Now a
scheme-anchored host-only regex; localhost.example.com and embedded
substrings pass through. Tests added.
- MINOR: _invalidate_cached_context_length now also drops the
in-memory TTL probe rows for the pair, so a resolution inside the
TTL window can't re-persist the value just declared stale.
- Test gaps closed: detect-type cache hit + TTL-expiry re-detection,
ollama-show TTL expiry re-probe, reverse legacy-row lookups.
- Attribution gate: added zhchl@hermes-agent.local -> 8294 (PR #50572
author) to AUTHOR_MAP; the strict CI grep needs bare non-plus emails
literal in release.py.
Gates: ruff clean; targeted suites 202 passed / 0 failed; full
tests/agent 5426 passed with 17 failures identical on clean
upstream/main (pre-existing env-dependent anthropic/bedrock/credpool
tests); mypy delta vs base: 0 new errors; live smoke 6/6 PASS.
This commit is contained in:
parent
20cb385328
commit
f556edc10d
3 changed files with 194 additions and 22 deletions
|
|
@ -111,10 +111,15 @@ _MODEL_CACHE_TTL = 3600
|
|||
_endpoint_model_metadata_cache: Dict[str, Dict[str, Dict[str, Any]]] = {}
|
||||
_endpoint_model_metadata_cache_time: Dict[str, float] = {}
|
||||
_ENDPOINT_MODEL_CACHE_TTL = 300
|
||||
# Process-lifetime cache: after the first successful probe we remember the
|
||||
# Bounded-lifetime cache: after the first successful probe we remember the
|
||||
# server type so subsequent refreshes skip the full waterfall (no more 404
|
||||
# spam every 5 minutes on non-matching endpoints like /api/v1/models on vllm).
|
||||
_endpoint_probe_path_cache: Dict[str, str] = {}
|
||||
# Entries expire after _ENDPOINT_PROBE_TTL_SECONDS so a server swap on the
|
||||
# same port (stop Ollama, start LM Studio) is eventually re-detected instead
|
||||
# of being pinned to the stale type for the whole process lifetime.
|
||||
# Values are (server_type, monotonic_timestamp).
|
||||
_ENDPOINT_PROBE_TTL_SECONDS = 3600.0
|
||||
_endpoint_probe_path_cache: Dict[str, tuple] = {}
|
||||
|
||||
|
||||
def _get_model_metadata_cache_path() -> Path:
|
||||
|
|
@ -637,21 +642,26 @@ def is_local_endpoint(base_url: str) -> bool:
|
|||
|
||||
|
||||
def _localhost_to_ipv4(url: str) -> str:
|
||||
"""Rewrite a ``localhost`` host to ``127.0.0.1`` in a probe URL.
|
||||
"""Rewrite a ``localhost`` HOST to ``127.0.0.1`` in a probe URL.
|
||||
|
||||
On Windows dual-stack machines, httpx resolves ``localhost`` to ``::1``
|
||||
first and pays a ~2s IPv6 connect timeout before falling back to IPv4
|
||||
when the local server only listens on IPv4 (LM Studio, Ollama defaults).
|
||||
Probing the IPv4 loopback directly skips that penalty. Non-localhost
|
||||
URLs pass through unchanged.
|
||||
Probing the IPv4 loopback directly skips that penalty.
|
||||
|
||||
Only the URL's own host component is rewritten (anchored at the scheme),
|
||||
so a non-localhost URL whose path or query merely embeds the substring
|
||||
``http://localhost...`` (e.g. ``?upstream=http://localhost:11434``)
|
||||
passes through untouched.
|
||||
"""
|
||||
if not url:
|
||||
return url
|
||||
url = url.replace("://localhost:", "://127.0.0.1:")
|
||||
url = url.replace("://localhost/", "://127.0.0.1/")
|
||||
if url.endswith("://localhost"):
|
||||
url = url[:-len("localhost")] + "127.0.0.1"
|
||||
return url
|
||||
return re.sub(
|
||||
r"^(https?://)localhost(?=[:/]|$)",
|
||||
r"\g<1>127.0.0.1",
|
||||
url,
|
||||
count=1,
|
||||
)
|
||||
|
||||
|
||||
def detect_local_server_type(base_url: str, api_key: str = "") -> Optional[str]:
|
||||
|
|
@ -678,8 +688,8 @@ def detect_local_server_type(base_url: str, api_key: str = "") -> Optional[str]:
|
|||
lmstudio_url = _lmstudio_server_root(normalized)
|
||||
|
||||
cached = _endpoint_probe_path_cache.get(server_url)
|
||||
if cached is not None:
|
||||
return cached
|
||||
if cached is not None and (time.monotonic() - cached[1]) < _ENDPOINT_PROBE_TTL_SECONDS:
|
||||
return cached[0]
|
||||
|
||||
headers = _auth_headers(api_key)
|
||||
|
||||
|
|
@ -732,7 +742,7 @@ def detect_local_server_type(base_url: str, api_key: str = "") -> Optional[str]:
|
|||
pass
|
||||
|
||||
if result is not None:
|
||||
_endpoint_probe_path_cache[server_url] = result
|
||||
_endpoint_probe_path_cache[server_url] = (result, time.monotonic())
|
||||
return result
|
||||
|
||||
|
||||
|
|
@ -1106,9 +1116,15 @@ def get_cached_context_length(model: str, base_url: str) -> Optional[int]:
|
|||
if hit is not None:
|
||||
return hit
|
||||
# Legacy rows written before key normalization may carry a trailing
|
||||
# slash — honor them rather than re-probing.
|
||||
if base_url and base_url != base_url.rstrip("/"):
|
||||
return cache.get(f"{model}@{base_url}")
|
||||
# slash — honor them rather than re-probing. Checked regardless of the
|
||||
# caller's slash form: the row's shape and the caller's shape can differ
|
||||
# in either direction (old slashed row + new normalized config, or the
|
||||
# reverse), so probe the literal form and the slashed canonical form.
|
||||
for legacy_key in (f"{model}@{base_url}", f"{key}/"):
|
||||
if legacy_key != key:
|
||||
hit = cache.get(legacy_key)
|
||||
if hit is not None:
|
||||
return hit
|
||||
return None
|
||||
|
||||
|
||||
|
|
@ -1116,12 +1132,21 @@ def _invalidate_cached_context_length(model: str, base_url: str) -> None:
|
|||
"""Drop a stale cache entry so it gets re-resolved on the next lookup."""
|
||||
key = _context_cache_key(model, base_url)
|
||||
cache = _load_context_cache()
|
||||
# Also clear any legacy un-normalized row for the same pair.
|
||||
legacy_key = f"{model}@{base_url}"
|
||||
if key not in cache and legacy_key not in cache:
|
||||
# Invalidation must also drop the in-memory TTL probe entries for this
|
||||
# pair — otherwise the next resolution inside the TTL window reuses the
|
||||
# very value we just declared stale and re-persists it.
|
||||
bare = _strip_provider_prefix(model)
|
||||
stripped = (base_url or "").rstrip("/")
|
||||
_LOCAL_CTX_PROBE_CACHE.pop((bare, stripped), None)
|
||||
_LOCAL_CTX_PROBE_CACHE.pop(("ollama_show", bare, stripped), None)
|
||||
# Clear every key shape for this pair: canonical, the caller's literal
|
||||
# form, and the slashed legacy form — same set get_cached_context_length
|
||||
# consults, so a lookup can never resurrect a row invalidation missed.
|
||||
stale_keys = {key, f"{model}@{base_url}", f"{key}/"}
|
||||
if not any(k in cache for k in stale_keys):
|
||||
return
|
||||
cache.pop(key, None)
|
||||
cache.pop(legacy_key, None)
|
||||
for k in stale_keys:
|
||||
cache.pop(k, None)
|
||||
path = _get_context_cache_path()
|
||||
try:
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
|
|
|
|||
|
|
@ -1935,6 +1935,7 @@ AUTHOR_MAP = {
|
|||
"codeforgenet@icloud.com": "CodeForgeNet", # PR #47437 salvage (compact_rows blob skip)
|
||||
"i@dex.moe": "dexhunter", # PR #60339 salvage (skills snapshot manifest speedup)
|
||||
"1torhan@protonmail.com": "uzaylisak", # PR #29988 salvage (detect_local_server_type process-lifetime cache)
|
||||
"zhchl@hermes-agent.local": "8294", # PR #50572 salvage (honor config context_length on banner)
|
||||
}
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -18,11 +18,13 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
|
|||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _clear_probe_cache():
|
||||
"""Module-level TTL cache must not leak between tests."""
|
||||
"""Module-level caches must not leak between tests."""
|
||||
from agent import model_metadata
|
||||
model_metadata._LOCAL_CTX_PROBE_CACHE.clear()
|
||||
model_metadata._endpoint_probe_path_cache.clear()
|
||||
yield
|
||||
model_metadata._LOCAL_CTX_PROBE_CACHE.clear()
|
||||
model_metadata._endpoint_probe_path_cache.clear()
|
||||
|
||||
|
||||
def _mock_show_response(ctx=131072):
|
||||
|
|
@ -68,6 +70,24 @@ class TestOllamaApiShowCaching:
|
|||
|
||||
assert client.post.call_count == 2 # None was NOT cached
|
||||
|
||||
def test_ttl_expiry_reprobes(self):
|
||||
"""After the 30s TTL lapses, the next call must hit the network again."""
|
||||
from agent import model_metadata
|
||||
from agent.model_metadata import _query_ollama_api_show
|
||||
import time as _time
|
||||
|
||||
client = _client_mock(_mock_show_response(131072))
|
||||
with patch("httpx.Client", return_value=client):
|
||||
_query_ollama_api_show("llama3", "http://127.0.0.1:11434")
|
||||
# Age the entry past the TTL.
|
||||
((key, (val, _ts)),) = list(model_metadata._LOCAL_CTX_PROBE_CACHE.items())
|
||||
model_metadata._LOCAL_CTX_PROBE_CACHE[key] = (
|
||||
val, _time.monotonic() - model_metadata._LOCAL_CTX_PROBE_TTL_SECONDS - 1,
|
||||
)
|
||||
_query_ollama_api_show("llama3", "http://127.0.0.1:11434")
|
||||
|
||||
assert client.post.call_count == 2 # expired entry re-probed
|
||||
|
||||
def test_cache_key_does_not_collide_with_local_ctx_probe(self):
|
||||
"""The ollama_show namespace must not read _query_local_context_length rows."""
|
||||
from agent import model_metadata
|
||||
|
|
@ -87,6 +107,76 @@ class TestOllamaApiShowCaching:
|
|||
assert client.post.call_count == 1
|
||||
|
||||
|
||||
class TestDetectLocalServerTypeCache:
|
||||
"""#29988: detect_local_server_type memoized with a bounded TTL."""
|
||||
|
||||
def _get_client(self, server_type="ollama"):
|
||||
ollama_resp = MagicMock()
|
||||
ollama_resp.status_code = 200
|
||||
ollama_resp.json.return_value = {"models": []}
|
||||
miss = MagicMock()
|
||||
miss.status_code = 404
|
||||
|
||||
client = MagicMock()
|
||||
client.__enter__ = lambda s: client
|
||||
client.__exit__ = MagicMock(return_value=False)
|
||||
|
||||
def _get(url, *a, **k):
|
||||
if url.endswith("/api/tags"):
|
||||
return ollama_resp
|
||||
return miss
|
||||
|
||||
client.get.side_effect = _get
|
||||
return client
|
||||
|
||||
def test_second_call_served_from_cache(self):
|
||||
from agent.model_metadata import detect_local_server_type
|
||||
|
||||
client = self._get_client()
|
||||
with patch("httpx.Client", return_value=client):
|
||||
first = detect_local_server_type("http://127.0.0.1:11434")
|
||||
calls_after_first = client.get.call_count
|
||||
second = detect_local_server_type("http://127.0.0.1:11434")
|
||||
|
||||
assert first == second == "ollama"
|
||||
assert client.get.call_count == calls_after_first # no new HTTP traffic
|
||||
|
||||
def test_ttl_expiry_allows_server_swap_redetection(self):
|
||||
"""Stopping Ollama and starting LM Studio on the same port must be
|
||||
re-detected once the TTL lapses — the cache is bounded, not
|
||||
process-lifetime."""
|
||||
from agent import model_metadata
|
||||
from agent.model_metadata import detect_local_server_type
|
||||
import time as _time
|
||||
|
||||
client = self._get_client()
|
||||
with patch("httpx.Client", return_value=client):
|
||||
assert detect_local_server_type("http://127.0.0.1:11434") == "ollama"
|
||||
|
||||
# Age the entry past the TTL, then swap the backend behind the URL.
|
||||
((key, (val, _ts)),) = list(model_metadata._endpoint_probe_path_cache.items())
|
||||
model_metadata._endpoint_probe_path_cache[key] = (
|
||||
val, _time.monotonic() - model_metadata._ENDPOINT_PROBE_TTL_SECONDS - 1,
|
||||
)
|
||||
|
||||
lmstudio_resp = MagicMock()
|
||||
lmstudio_resp.status_code = 200
|
||||
lmstudio_resp.json.return_value = {"data": []}
|
||||
swap_client = MagicMock()
|
||||
swap_client.__enter__ = lambda s: swap_client
|
||||
swap_client.__exit__ = MagicMock(return_value=False)
|
||||
|
||||
def _get(url, *a, **k):
|
||||
if url.endswith("/api/v1/models"):
|
||||
return lmstudio_resp
|
||||
miss = MagicMock(); miss.status_code = 404
|
||||
return miss
|
||||
|
||||
swap_client.get.side_effect = _get
|
||||
with patch("httpx.Client", return_value=swap_client):
|
||||
assert detect_local_server_type("http://127.0.0.1:11434") == "lm-studio"
|
||||
|
||||
|
||||
class TestLocalhostIPv4SiblingSites:
|
||||
"""#37595 widened: every probe helper rewrites localhost→127.0.0.1,
|
||||
not just detect_local_server_type."""
|
||||
|
|
@ -102,6 +192,18 @@ class TestLocalhostIPv4SiblingSites:
|
|||
assert _localhost_to_ipv4("https://api.openai.com/v1") == "https://api.openai.com/v1"
|
||||
assert _localhost_to_ipv4("") == ""
|
||||
|
||||
def test_rewrite_is_host_only_not_substring(self):
|
||||
"""A URL that merely EMBEDS 'http://localhost' in its path/query must
|
||||
not be corrupted — only the URL's own host is rewritten."""
|
||||
from agent.model_metadata import _localhost_to_ipv4
|
||||
|
||||
proxied = "https://proxy.example.com/route?upstream=http://localhost:11434"
|
||||
assert _localhost_to_ipv4(proxied) == proxied
|
||||
# Host must be a full label: localhost.example.com is NOT localhost.
|
||||
assert _localhost_to_ipv4("http://localhost.example.com/v1") == (
|
||||
"http://localhost.example.com/v1"
|
||||
)
|
||||
|
||||
def test_ollama_api_show_probes_ipv4(self):
|
||||
from agent.model_metadata import _query_ollama_api_show
|
||||
|
||||
|
|
@ -150,6 +252,18 @@ class TestContextCacheKeyNormalization:
|
|||
|
||||
assert model_metadata.get_cached_context_length("m1", "http://host/v1/") == 128_000
|
||||
|
||||
def test_legacy_slashed_row_found_with_normalized_caller(self, tmp_path, monkeypatch):
|
||||
"""Reverse migration direction: old row has the slash, current runtime
|
||||
passes the normalized no-slash URL — must still hit, not re-probe."""
|
||||
import yaml
|
||||
from agent import model_metadata
|
||||
|
||||
path = tmp_path / "context_lengths.yaml"
|
||||
monkeypatch.setattr(model_metadata, "_get_context_cache_path", lambda: path)
|
||||
path.write_text(yaml.dump({"context_lengths": {"m1@http://host/v1/": 128_000}}))
|
||||
|
||||
assert model_metadata.get_cached_context_length("m1", "http://host/v1") == 128_000
|
||||
|
||||
def test_invalidate_clears_both_key_shapes(self, tmp_path, monkeypatch):
|
||||
import yaml
|
||||
from agent import model_metadata
|
||||
|
|
@ -165,3 +279,35 @@ class TestContextCacheKeyNormalization:
|
|||
cache = model_metadata._load_context_cache()
|
||||
assert "m1@http://host/v1" not in cache
|
||||
assert "m1@http://host/v1/" not in cache
|
||||
|
||||
def test_invalidate_with_normalized_caller_clears_legacy_row(self, tmp_path, monkeypatch):
|
||||
"""Reverse direction: invalidating with the no-slash URL must also
|
||||
drop a legacy slashed row, or the next lookup resurrects stale data."""
|
||||
import yaml
|
||||
from agent import model_metadata
|
||||
|
||||
path = tmp_path / "context_lengths.yaml"
|
||||
monkeypatch.setattr(model_metadata, "_get_context_cache_path", lambda: path)
|
||||
path.write_text(yaml.dump({"context_lengths": {"m1@http://host/v1/": 64_000}}))
|
||||
|
||||
model_metadata._invalidate_cached_context_length("m1", "http://host/v1")
|
||||
assert model_metadata.get_cached_context_length("m1", "http://host/v1") is None
|
||||
assert model_metadata.get_cached_context_length("m1", "http://host/v1/") is None
|
||||
|
||||
def test_invalidate_also_drops_in_memory_probe_entries(self, tmp_path, monkeypatch):
|
||||
"""Disk invalidation must clear the in-memory TTL rows too, or the
|
||||
next resolution inside the TTL window re-persists the stale value."""
|
||||
import time as _time
|
||||
from agent import model_metadata
|
||||
|
||||
path = tmp_path / "context_lengths.yaml"
|
||||
monkeypatch.setattr(model_metadata, "_get_context_cache_path", lambda: path)
|
||||
|
||||
now = _time.monotonic()
|
||||
model_metadata._LOCAL_CTX_PROBE_CACHE[("m1", "http://host/v1")] = (999, now)
|
||||
model_metadata._LOCAL_CTX_PROBE_CACHE[("ollama_show", "m1", "http://host/v1")] = (999, now)
|
||||
|
||||
model_metadata._invalidate_cached_context_length("m1", "http://host/v1")
|
||||
|
||||
assert ("m1", "http://host/v1") not in model_metadata._LOCAL_CTX_PROBE_CACHE
|
||||
assert ("ollama_show", "m1", "http://host/v1") not in model_metadata._LOCAL_CTX_PROBE_CACHE
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue