fix(models): close review findings on the ETag refresh path
- Conditional GET now requires a servable in-memory registry: an
If-None-Match sent while holding no cache invited a 304 against
nothing, permanently serving {} with a blocking foreground fetch on
every call (the exact #35838 class this PR fixes). Empirically
repro'd and verified fixed (corrupt cache + stale sidecar: was 3
calls -> {} forever; now 1 unconditional fetch -> real data).
- ETag persists atomically WITH the cache body via
_commit_registry -> _save_disk_cache(data, etag), wiring up the
previously-dead etag param; the sidecar can no longer get ahead of
the registry it vouches for. _save_etag now uses
utils.atomic_write_text (unique tempnames + fsync) instead of a
hand-rolled fixed-name .tmp replace.
- Corrupt/unreadable disk cache clears the ETag sidecar so the
refetch is unconditional; _confirm_cache_not_modified keeps a
defense-in-depth guard (clear sidecar + arm backoff) should a 304
ever land on an empty registry.
- allow_network=True paths use the zero-arg fetch_models_dev() call
shape at all sites (was 1 of 5) — ~46 test sites monkeypatch it
with zero-arg lambdas; the unconditional kwarg broke
test_xiaomi_provider (verified fail->pass).
- _get_models_dev_url falls back to the MODELS_DEV_URL module global
(not the constant) so existing patch sites keep working.
- Tests: replaced two mock-riddled corrupt-cache tests with real
tmp_path file tests; added regression tests for the 304/empty-cache
loop, sidecar clearing, and conditional-GET gating.
This commit is contained in:
kshitij
parent
acd8737c10
commit
b1ce502535
|
|
@ -256,15 +256,28 @@ def _load_etag() -> str:
|
|||
def _save_etag(etag: str) -> None:
|
||||
"""Persist an ETag to the sidecar file atomically."""
|
||||
try:
|
||||
from utils import atomic_write_text
|
||||
|
||||
etag_path = _get_etag_path()
|
||||
etag_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
tmp = etag_path.with_suffix(".tmp")
|
||||
tmp.write_text(etag, encoding="utf-8")
|
||||
tmp.replace(etag_path)
|
||||
atomic_write_text(etag_path, etag)
|
||||
except Exception as e:
|
||||
logger.debug("Failed to save models.dev ETag: %s", e)
|
||||
|
||||
|
||||
def _clear_etag() -> None:
|
||||
"""Delete the ETag sidecar so the next fetch is unconditional.
|
||||
|
||||
Called when the cached registry the ETag vouches for is gone or
|
||||
unusable — sending If-None-Match without a servable cache invites a
|
||||
304 that would leave the process with no data at all.
|
||||
"""
|
||||
try:
|
||||
_get_etag_path().unlink(missing_ok=True)
|
||||
except Exception as e:
|
||||
logger.debug("Failed to clear models.dev ETag: %s", e)
|
||||
|
||||
|
||||
def _get_models_dev_url() -> str:
|
||||
"""Resolve the models.dev API URL, honoring a config.yaml override.
|
||||
|
||||
|
|
@ -280,7 +293,9 @@ def _get_models_dev_url() -> str:
|
|||
return url.strip()
|
||||
except Exception:
|
||||
pass
|
||||
return _DEFAULT_MODELS_DEV_URL
|
||||
# Fall back to the module global (not the constant) so existing
|
||||
# code/tests that patch MODELS_DEV_URL keep working.
|
||||
return MODELS_DEV_URL
|
||||
|
||||
|
||||
def _validate_registry(data: Any) -> bool:
|
||||
|
|
@ -305,12 +320,17 @@ def _load_disk_cache() -> Dict[str, Any]:
|
|||
"models.dev disk cache is corrupt or empty; ignoring "
|
||||
"(will refetch from network)"
|
||||
)
|
||||
# The sidecar vouches for a registry we no longer hold —
|
||||
# drop it so the refetch is unconditional (a 304 against
|
||||
# a missing cache would leave us with no data at all).
|
||||
_clear_etag()
|
||||
return {}
|
||||
return data
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
"Failed to load models.dev disk cache; ignoring: %s", e
|
||||
)
|
||||
_clear_etag()
|
||||
return {}
|
||||
|
||||
|
||||
|
|
@ -359,21 +379,30 @@ class _NotModified(Exception):
|
|||
"""Server returned 304 Not Modified — existing cache is still valid."""
|
||||
|
||||
|
||||
def _fetch_models_dev_from_network() -> Dict[str, Any]:
|
||||
def _fetch_models_dev_from_network() -> Tuple[Dict[str, Any], str]:
|
||||
"""Fetch the live models.dev registry without touching local caches.
|
||||
|
||||
Uses ETag conditional GET: sends ``If-None-Match`` when a cached ETag
|
||||
exists. A 304 Not Modified response means the cached registry is still
|
||||
current; this raises ``_NotModified`` so the caller can re-confirm the
|
||||
existing cache's freshness without re-downloading the full payload.
|
||||
exists AND the process holds a servable registry the 304 can
|
||||
re-confirm. A conditional request without a cache invites a 304 that
|
||||
leaves the process with no data at all (and, before this guard, a
|
||||
permanent empty-registry loop when the sidecar outlived a corrupt
|
||||
cache file). A 304 raises ``_NotModified`` so the caller can
|
||||
re-confirm the existing cache's freshness without re-downloading the
|
||||
full payload.
|
||||
|
||||
Raises on network errors and on an empty/invalid registry payload.
|
||||
Returns ``(registry, etag)``; the etag is empty when the server sent
|
||||
none. The caller persists it together with the cache body
|
||||
(``_commit_registry``) so the sidecar can never get ahead of the data
|
||||
it vouches for. Raises on network errors and on an empty/invalid
|
||||
registry payload.
|
||||
"""
|
||||
url = _get_models_dev_url()
|
||||
headers: Dict[str, str] = {}
|
||||
etag = _load_etag()
|
||||
if etag:
|
||||
headers["If-None-Match"] = etag
|
||||
if _models_dev_cache:
|
||||
etag = _load_etag()
|
||||
if etag:
|
||||
headers["If-None-Match"] = etag
|
||||
|
||||
# Tuple (connect, read): a flat timeout=15 let a blackholed connect
|
||||
# stall the first-turn critical path for the full 15 s. 5 s connect
|
||||
|
|
@ -387,16 +416,10 @@ def _fetch_models_dev_from_network() -> Dict[str, Any]:
|
|||
|
||||
response.raise_for_status()
|
||||
data = response.json()
|
||||
if not isinstance(data, dict) or not data:
|
||||
if not _validate_registry(data):
|
||||
raise ValueError("models.dev returned an empty or invalid registry")
|
||||
|
||||
# Persist the new ETag alongside the cache so the next conditional
|
||||
# GET can short-circuit.
|
||||
new_etag = response.headers.get("ETag", "")
|
||||
if new_etag:
|
||||
_save_etag(new_etag)
|
||||
|
||||
return data
|
||||
return data, response.headers.get("ETag", "")
|
||||
|
||||
|
||||
def _mark_stale_cache_grace() -> None:
|
||||
|
|
@ -412,7 +435,7 @@ def _mark_stale_cache_grace() -> None:
|
|||
_models_dev_cache_time = grace_time
|
||||
|
||||
|
||||
def _commit_registry(data: Dict[str, Any], *, where: str) -> None:
|
||||
def _commit_registry(data: Dict[str, Any], *, etag: str = "", where: str) -> None:
|
||||
"""Persist a freshly fetched registry: disk + in-mem + clear backoff.
|
||||
|
||||
Callers must hold ``_models_dev_fetch_lock`` so a failing refresh on one
|
||||
|
|
@ -421,7 +444,7 @@ def _commit_registry(data: Dict[str, Any], *, where: str) -> None:
|
|||
immediately after a successful ``force_refresh``).
|
||||
"""
|
||||
global _models_dev_cache, _models_dev_cache_time, _models_dev_retry_after
|
||||
_save_disk_cache(data)
|
||||
_save_disk_cache(data, etag)
|
||||
_models_dev_cache = data
|
||||
_models_dev_cache_time = time.time()
|
||||
_models_dev_retry_after = 0
|
||||
|
|
@ -442,6 +465,21 @@ def _confirm_cache_not_modified(*, where: str) -> None:
|
|||
unchanged, only its freshness marker is advanced.
|
||||
"""
|
||||
global _models_dev_cache_time, _models_dev_retry_after
|
||||
if not _models_dev_cache:
|
||||
# Pathological: a 304 arrived but we hold no registry. Should be
|
||||
# unreachable now that conditional GETs require a servable cache
|
||||
# (see _fetch_models_dev_from_network); kept as defense in depth
|
||||
# because this state previously caused a permanent empty-registry
|
||||
# loop. Drop the sidecar so the next attempt is unconditional and
|
||||
# arm the normal failure backoff instead of marking {} "fresh".
|
||||
_clear_etag()
|
||||
_models_dev_retry_after = time.time() + _MODELS_DEV_RETRY_DELAY
|
||||
logger.warning(
|
||||
"models.dev returned 304 but no cached registry is held (%s); "
|
||||
"cleared ETag sidecar, will refetch unconditionally",
|
||||
where,
|
||||
)
|
||||
return
|
||||
_models_dev_cache_time = time.time()
|
||||
_models_dev_retry_after = 0
|
||||
logger.debug(
|
||||
|
|
@ -470,9 +508,9 @@ def _background_refresh_models_dev() -> None:
|
|||
"""Best-effort refresh after serving stale cache data."""
|
||||
global _models_dev_refresh_in_flight
|
||||
try:
|
||||
data = _fetch_models_dev_from_network()
|
||||
data, etag = _fetch_models_dev_from_network()
|
||||
with _models_dev_fetch_lock:
|
||||
_commit_registry(data, where="background")
|
||||
_commit_registry(data, etag=etag, where="background")
|
||||
except _NotModified:
|
||||
with _models_dev_fetch_lock:
|
||||
_confirm_cache_not_modified(where="background")
|
||||
|
|
@ -627,8 +665,8 @@ def fetch_models_dev(
|
|||
return _models_dev_cache
|
||||
|
||||
try:
|
||||
data = _fetch_models_dev_from_network()
|
||||
_commit_registry(data, where="foreground")
|
||||
data, etag = _fetch_models_dev_from_network()
|
||||
_commit_registry(data, etag=etag, where="foreground")
|
||||
return data
|
||||
except _NotModified:
|
||||
# Server confirmed our cache is still valid. Re-confirm freshness
|
||||
|
|
@ -679,7 +717,14 @@ def lookup_models_dev_context(
|
|||
if not mdev_provider_id:
|
||||
return _default_override_context(provider)
|
||||
|
||||
data = fetch_models_dev(allow_network=allow_network)
|
||||
# NOTE: keep the zero-argument call on the allow_network path. Dozens
|
||||
# of test sites monkeypatch fetch_models_dev with zero-arg lambdas;
|
||||
# passing the kwarg unconditionally breaks them all (TypeError).
|
||||
data = (
|
||||
fetch_models_dev()
|
||||
if allow_network
|
||||
else fetch_models_dev(allow_network=False)
|
||||
)
|
||||
provider_data = data.get(mdev_provider_id)
|
||||
if not isinstance(provider_data, dict):
|
||||
return _default_override_context(provider)
|
||||
|
|
@ -1022,7 +1067,14 @@ def _get_provider_models(
|
|||
if not mdev_provider_id:
|
||||
return None
|
||||
|
||||
data = fetch_models_dev(allow_network=allow_network)
|
||||
# NOTE: keep the zero-argument call on the allow_network path. Dozens
|
||||
# of test sites monkeypatch fetch_models_dev with zero-arg lambdas;
|
||||
# passing the kwarg unconditionally breaks them all (TypeError).
|
||||
data = (
|
||||
fetch_models_dev()
|
||||
if allow_network
|
||||
else fetch_models_dev(allow_network=False)
|
||||
)
|
||||
provider_data = data.get(mdev_provider_id)
|
||||
if not isinstance(provider_data, dict):
|
||||
return None
|
||||
|
|
@ -1418,7 +1470,14 @@ def get_model_info(
|
|||
shaped = _merge_catalog_entry_with_override(base, override)
|
||||
return _parse_model_info(model_id, shaped, mdev_id)
|
||||
|
||||
data = fetch_models_dev(allow_network=allow_network)
|
||||
# NOTE: keep the zero-argument call on the allow_network path. Dozens
|
||||
# of test sites monkeypatch fetch_models_dev with zero-arg lambdas;
|
||||
# passing the kwarg unconditionally breaks them all (TypeError).
|
||||
data = (
|
||||
fetch_models_dev()
|
||||
if allow_network
|
||||
else fetch_models_dev(allow_network=False)
|
||||
)
|
||||
pdata = data.get(mdev_id)
|
||||
if not isinstance(pdata, dict):
|
||||
return _from_override_alone()
|
||||
|
|
|
|||
|
|
@ -252,8 +252,10 @@ class TestFetchModelsDev:
|
|||
md._models_dev_refresh_in_flight = True
|
||||
md._background_refresh_models_dev()
|
||||
|
||||
mock_save.assert_called_once_with(SAMPLE_REGISTRY)
|
||||
mock_save_etag.assert_called_once_with('"abc123"')
|
||||
# ETag is committed together with the cache body so the sidecar
|
||||
# can never get ahead of the data it vouches for.
|
||||
mock_save.assert_called_once_with(SAMPLE_REGISTRY, '"abc123"')
|
||||
mock_save_etag.assert_not_called()
|
||||
assert md._models_dev_cache == SAMPLE_REGISTRY
|
||||
assert md._models_dev_cache_time > 0
|
||||
assert md._models_dev_retry_after == 0
|
||||
|
|
@ -372,12 +374,17 @@ class TestETagConditionalGet:
|
|||
response.raise_for_status = MagicMock()
|
||||
mock_get.return_value = response
|
||||
|
||||
# Conditional GET requires a servable in-memory registry — an
|
||||
# If-None-Match without one invites a 304 against nothing.
|
||||
md._models_dev_cache = SAMPLE_REGISTRY
|
||||
md._models_dev_cache_time = 0
|
||||
|
||||
with patch.object(md, "_disk_cache_age_seconds", return_value=None), \
|
||||
patch.object(md, "_load_disk_cache", return_value={}), \
|
||||
patch.object(md, "_save_disk_cache"), \
|
||||
patch.object(md, "_load_etag", return_value='"v1"'), \
|
||||
patch.object(md, "_save_etag"):
|
||||
fetch_models_dev()
|
||||
fetch_models_dev(force_refresh=True)
|
||||
|
||||
call_kwargs = mock_get.call_args
|
||||
headers = call_kwargs.kwargs.get("headers", {})
|
||||
|
|
@ -448,12 +455,14 @@ class TestETagConditionalGet:
|
|||
|
||||
with patch.object(md, "_disk_cache_age_seconds", return_value=None), \
|
||||
patch.object(md, "_load_disk_cache", return_value={}), \
|
||||
patch.object(md, "_save_disk_cache"), \
|
||||
patch.object(md, "_save_disk_cache") as mock_save, \
|
||||
patch.object(md, "_load_etag", return_value=""), \
|
||||
patch.object(md, "_save_etag") as mock_save_etag:
|
||||
fetch_models_dev()
|
||||
|
||||
mock_save_etag.assert_called_once_with('"new-etag"')
|
||||
# ETag rides along with the cache body into _save_disk_cache.
|
||||
mock_save.assert_called_once_with(SAMPLE_REGISTRY, '"new-etag"')
|
||||
mock_save_etag.assert_not_called()
|
||||
|
||||
@patch("agent.models_dev.requests.get")
|
||||
def test_no_etag_header_sent_without_cached_etag(self, mock_get):
|
||||
|
|
@ -498,48 +507,105 @@ class TestCorruptCacheRejection:
|
|||
def test_validate_registry_accepts_populated_dict(self):
|
||||
assert _validate_registry({"anthropic": {}})
|
||||
|
||||
@patch("agent.models_dev.requests.get")
|
||||
def test_corrupt_json_rejected_with_warning(self, mock_get, caplog):
|
||||
"""Invalid JSON on disk is ignored, not served as {}."""
|
||||
import agent.models_dev as md
|
||||
import json as _json
|
||||
|
||||
mock_get.side_effect = OSError("unreachable")
|
||||
md._models_dev_cache = {}
|
||||
md._models_dev_cache_time = 0
|
||||
|
||||
with patch.object(md, "_disk_cache_age_seconds", return_value=0), \
|
||||
patch.object(md, "_get_cache_path") as mock_path, \
|
||||
patch.object(md, "_load_etag", return_value=""):
|
||||
mock_path.return_value.exists.return_value = True
|
||||
mock_path.return_value.open.return_value.__enter__.return_value.read.return_value = "not json"
|
||||
# json.load will raise on invalid JSON
|
||||
with patch("builtins.open", side_effect=_json.JSONDecodeError("msg", "doc", 0)):
|
||||
with patch.object(md, "_load_disk_cache", wraps=md._load_disk_cache):
|
||||
result = fetch_models_dev()
|
||||
|
||||
# Returns empty dict, not the corrupt data
|
||||
assert result == {}
|
||||
|
||||
@patch("agent.models_dev.requests.get")
|
||||
def test_empty_dict_cache_rejected(self, mock_get, caplog):
|
||||
"""An empty dict in the cache file is rejected with a warning."""
|
||||
import agent.models_dev as md
|
||||
def test_corrupt_json_on_disk_rejected_with_warning(self, tmp_path, caplog):
|
||||
"""Invalid JSON in a REAL cache file is rejected with a warning."""
|
||||
import logging
|
||||
|
||||
mock_get.side_effect = OSError("unreachable")
|
||||
md._models_dev_cache = {}
|
||||
md._models_dev_cache_time = 0
|
||||
import agent.models_dev as md
|
||||
|
||||
with patch.object(md, "_disk_cache_age_seconds", return_value=0), \
|
||||
patch.object(md, "_load_disk_cache", return_value={}), \
|
||||
patch.object(md, "_load_etag", return_value=""), \
|
||||
patch.object(md, "_save_disk_cache"):
|
||||
cache = tmp_path / "models_dev_cache.json"
|
||||
cache.write_text("not json{{{", encoding="utf-8")
|
||||
with patch.object(md, "_get_cache_path", return_value=cache), \
|
||||
patch.object(md, "_get_etag_path", return_value=tmp_path / "models_dev_cache.etag"):
|
||||
with caplog.at_level(logging.WARNING):
|
||||
# _load_disk_cache returns {} for empty dict, which is correct
|
||||
result = fetch_models_dev()
|
||||
result = md._load_disk_cache()
|
||||
|
||||
assert result == {}
|
||||
assert any("disk cache" in r.message for r in caplog.records)
|
||||
|
||||
def test_empty_dict_on_disk_rejected_with_warning(self, tmp_path, caplog):
|
||||
"""A REAL cache file containing {} is rejected with a warning."""
|
||||
import logging
|
||||
|
||||
import agent.models_dev as md
|
||||
|
||||
cache = tmp_path / "models_dev_cache.json"
|
||||
cache.write_text("{}", encoding="utf-8")
|
||||
with patch.object(md, "_get_cache_path", return_value=cache), \
|
||||
patch.object(md, "_get_etag_path", return_value=tmp_path / "models_dev_cache.etag"):
|
||||
with caplog.at_level(logging.WARNING):
|
||||
result = md._load_disk_cache()
|
||||
|
||||
assert result == {}
|
||||
assert any("corrupt or empty" in r.message for r in caplog.records)
|
||||
|
||||
def test_corrupt_cache_clears_etag_sidecar(self, tmp_path):
|
||||
"""Rejecting a corrupt cache must drop the ETag sidecar (#35838 loop).
|
||||
|
||||
If the sidecar outlives the registry it vouches for, the next
|
||||
conditional GET draws a 304 against nothing and the process serves
|
||||
{} forever. Clearing the sidecar forces an unconditional refetch.
|
||||
"""
|
||||
import agent.models_dev as md
|
||||
|
||||
cache = tmp_path / "models_dev_cache.json"
|
||||
etag = tmp_path / "models_dev_cache.etag"
|
||||
cache.write_text("corrupt!!", encoding="utf-8")
|
||||
etag.write_text("stale-etag", encoding="utf-8")
|
||||
|
||||
with patch.object(md, "_get_cache_path", return_value=cache), \
|
||||
patch.object(md, "_get_etag_path", return_value=etag):
|
||||
result = md._load_disk_cache()
|
||||
|
||||
assert result == {}
|
||||
assert not etag.exists()
|
||||
|
||||
def test_conditional_get_skipped_without_servable_cache(self):
|
||||
"""No If-None-Match header when the process holds no registry.
|
||||
|
||||
A conditional GET without a servable cache invites a 304 that
|
||||
leaves the process with no data at all — the permanent
|
||||
empty-registry loop. The header is only sent when _models_dev_cache
|
||||
is populated.
|
||||
"""
|
||||
import agent.models_dev as md
|
||||
|
||||
captured: dict = {}
|
||||
|
||||
def fake_get(url, headers=None, timeout=None):
|
||||
captured["headers"] = dict(headers or {})
|
||||
resp = MagicMock()
|
||||
resp.status_code = 200
|
||||
resp.json.return_value = {"anthropic": {"models": {}}}
|
||||
resp.headers = {"ETag": "fresh"}
|
||||
return resp
|
||||
|
||||
with patch.object(md.requests, "get", side_effect=fake_get), \
|
||||
patch.object(md, "_load_etag", return_value="stale-etag"), \
|
||||
patch.object(md, "_models_dev_cache", {}):
|
||||
data, etag = md._fetch_models_dev_from_network()
|
||||
|
||||
assert "If-None-Match" not in captured["headers"]
|
||||
assert data == {"anthropic": {"models": {}}}
|
||||
assert etag == "fresh"
|
||||
|
||||
def test_304_with_empty_cache_arms_backoff_and_clears_etag(self, tmp_path):
|
||||
"""Defense in depth: a 304 landing on an empty registry must not
|
||||
mark {} as fresh — it clears the sidecar and arms the backoff."""
|
||||
import agent.models_dev as md
|
||||
|
||||
etag = tmp_path / "models_dev_cache.etag"
|
||||
etag.write_text("stale", encoding="utf-8")
|
||||
|
||||
with patch.object(md, "_get_etag_path", return_value=etag), \
|
||||
patch.object(md, "_models_dev_cache", {}):
|
||||
before = md._models_dev_retry_after
|
||||
try:
|
||||
md._confirm_cache_not_modified(where="test")
|
||||
assert not etag.exists()
|
||||
assert md._models_dev_retry_after > time.time() - 1
|
||||
finally:
|
||||
md._models_dev_retry_after = before
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -676,7 +742,10 @@ class TestNoNetworkOnHotPaths:
|
|||
with patch("agent.models_dev.fetch_models_dev") as mock_fetch:
|
||||
mock_fetch.return_value = CAPS_REGISTRY
|
||||
get_model_capabilities("anthropic", "claude-sonnet-4", allow_network=True)
|
||||
mock_fetch.assert_called_once_with(allow_network=True)
|
||||
# allow_network=True uses the zero-arg call shape so the dozens of
|
||||
# test sites that monkeypatch fetch_models_dev with zero-arg
|
||||
# lambdas keep working.
|
||||
mock_fetch.assert_called_once_with()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in New Issue