From f556edc10d9b1b41ff3ea8807105155910ab2087 Mon Sep 17 00:00:00 2001 From: Kshitij Kapoor Date: Thu, 9 Jul 2026 14:03:01 +0530 Subject: [PATCH] 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. --- agent/model_metadata.py | 67 +++++++--- scripts/release.py | 1 + tests/agent/test_probe_cache_followups.py | 148 +++++++++++++++++++++- 3 files changed, 194 insertions(+), 22 deletions(-) diff --git a/agent/model_metadata.py b/agent/model_metadata.py index 8f6d2594410..3e966899b9c 100644 --- a/agent/model_metadata.py +++ b/agent/model_metadata.py @@ -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) diff --git a/scripts/release.py b/scripts/release.py index 22c1854b8df..500c2ca05f5 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -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) } diff --git a/tests/agent/test_probe_cache_followups.py b/tests/agent/test_probe_cache_followups.py index 5dd6ace7bd3..f775b3f877e 100644 --- a/tests/agent/test_probe_cache_followups.py +++ b/tests/agent/test_probe_cache_followups.py @@ -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