diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ef31e36..74fc11f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ ### Fixed +- **Opening a session no longer stalls just because Codex refreshed its model cache in the background.** The WebUI keys its 24-hour `/api/models` cache partly on Codex's `~/.codex/models_cache.json`, but that file was fingerprinted by `mtime` + size — and Codex rewrites it on its own refresh timer, bumping those stat fields even when the model catalog is byte-identical (only a volatile `fetched_at` timestamp changed). Every Codex refresh therefore invalidated the cache, and the next session open paid a full live rebuild whose serial provider probes stalled the open (#7540). The Codex cache is now fingerprinted by its *content* with the refresh-timestamp fields (`fetched_at`, `updated_at`) stripped, so a timestamp-only rewrite keeps the cache while any genuine catalog change (models, `etag`, `client_version`) still invalidates it. A malformed or pathologically deep cache file degrades safely to the old stat fingerprint rather than erroring. Thanks @webtecnica. (#7556, closes #7540) + - **A timed-out command-approval card can be dismissed again instead of getting stuck on screen forever.** When the agent raised a local approval with no gateway notifier registered, the raw pending entry was stored without an `approval_id`; the frontend needs that id to own the card, so a card that timed out (or otherwise went unanswered) could be neither approved nor dismissed and the poll re-served it indefinitely. Reconciliation now mints a stable `gwlocal-mirrorless:` id on the raw local entry itself — the same tokenization contract already used for gateway producers — so the id survives repeated polls and the client's dismiss marker sticks. Gateway/run-bearing entries are untouched, and the synthetic prefix can't be mistaken for gateway authority (routing keys on exact ids, `run_id`, and mirror tokens, never the prefix). Thanks @CharlesMcquade. (#7510) - **Manual "Regenerate title" now works for conversations that open with several queued user messages.** For a session whose transcript begins with consecutive user turns (no assistant reply before the second user message), the first-exchange walker stopped at the second user row with no assistant text, so "Regenerate title" skipped the LLM call, silently persisted the local fallback (a truncated first message), and returned `200` with that same wrong title on every retry — leaving the session permanently stuck. Manual regeneration now scans past the opening user rows to the first complete user+assistant pair so it reaches the aux model. The automatic in-stream title path is deliberately unchanged. Thanks @Raylands. (#7545, closes #7543) diff --git a/api/config.py b/api/config.py index 6578c8e1..686d2392 100644 --- a/api/config.py +++ b/api/config.py @@ -6182,6 +6182,97 @@ def _models_cache_file_fingerprint(path: Path) -> dict: return fingerprint +# Codex's ~/.codex/models_cache.json is rewritten on Codex's own refresh timer. +# Each rewrite bumps mtime_ns + size, but the payload usually only refreshes +# the volatile timestamp fields (`fetched_at`, plus `updated_at` when present) +# while the model catalog (client_version, etag, models[]) stays identical. +# Fingerprinting that file by stat (#2443's _models_cache_file_fingerprint) +# therefore invalidated the 24h /api/models cache on every Codex refresh, and +# the next session visit paid a full live rebuild whose serial provider probes +# starved the session-open path (#7540). +# +# This is a DENY-list, not an allow-list, on purpose — same safety direction as +# _AUTH_FINGERPRINT_VOLATILE_KEYS: every other field (client_version, etag, +# models, and any future model-affecting key) stays IN the fingerprint, so a +# genuine catalog change still invalidates the cache. +_CODEX_CACHE_FINGERPRINT_VOLATILE_KEYS = frozenset({ + # Whole-file refresh timestamp, rewritten by every Codex models refresh. + "fetched_at", + # Same-family save timestamp (mirrors the auth.json deny-list). + "updated_at", +}) + + +def _strip_volatile_codex_cache_fields(obj): + """Recursively drop refresh-timestamp-only keys from a Codex cache tree. + + Pure structural transform; never mutates the input. Any key NOT in the + deny-list is preserved verbatim so real catalog changes still show through + in the fingerprint. + """ + if isinstance(obj, dict): + return { + k: _strip_volatile_codex_cache_fields(v) + for k, v in obj.items() + if k not in _CODEX_CACHE_FINGERPRINT_VOLATILE_KEYS + } + if isinstance(obj, list): + return [_strip_volatile_codex_cache_fields(v) for v in obj] + return obj + + +def _codex_models_cache_fingerprint(path: Path) -> dict: + """Return a content fingerprint of Codex's models_cache.json. + + Unlike _models_cache_file_fingerprint() (mtime_ns + size), this hashes the + JSON content with the refresh-timestamp fields stripped, so a Codex + refresh that only bumps `fetched_at` does NOT invalidate the 24h + /api/models cache and therefore does not force the live rebuild that + stalled session opens (#7540). A change to anything that actually feeds the + Codex models we surface (client_version, etag, models[], an unknown future + field) still changes the hash and correctly busts the cache. + + Failure modes are deliberately conservative — a missing file is recorded, + and an unreadable/undecodable file falls back to the stat-based fingerprint + so behaviour is never *less* safe than the stat-only version. + """ + p = Path(path).expanduser() + fp: dict = {"path": str(p)} + try: + st = p.stat() + except OSError: + fp["missing"] = True + return fp + try: + raw = json.loads(p.read_text(encoding="utf-8")) + except Exception: + # Unreadable / corrupt / mid-write: keep the stat-based fingerprint. + # Strictly no less safe than the pre-fix behaviour (every write still + # invalidates) for this rare path only. + fp["mtime_ns"] = st.st_mtime_ns + fp["size"] = st.st_size + fp["semantic"] = "unparsed-fallback" + return fp + try: + # The recursive strip can raise (e.g. RecursionError on a pathologically + # deep JSON tree) — keep it inside the fallback try so any transform + # failure degrades to the stat fingerprint rather than 500ing /api/models. + stripped = _strip_volatile_codex_cache_fields(raw) + encoded = json.dumps( + stripped, + sort_keys=True, + separators=(",", ":"), + ensure_ascii=True, + default=str, + ).encode("utf-8") + fp["semantic_sha256"] = hashlib.sha256(encoded).hexdigest() + except Exception: + fp["mtime_ns"] = st.st_mtime_ns + fp["size"] = st.st_size + fp["semantic"] = "encode-fallback" + return fp + + def _models_cache_catalog_fingerprint() -> dict: """Return non-secret model-catalog identity metadata for cache invalidation. @@ -6191,6 +6282,13 @@ def _models_cache_catalog_fingerprint() -> dict: deterministic so a server restart after catalog changes does not keep serving an otherwise-valid persisted models_cache.json until the 24h TTL expires (#2443). + + The Codex axis uses a *content* fingerprint that excludes the refresh + timestamp fields (see _codex_models_cache_fingerprint): Codex rewrites + ~/.codex/models_cache.json on its own timer, bumping mtime_ns + size while + the model payload stays identical, so a stat-based fingerprint invalidated + the 24h cache on every Codex refresh and the next session visit paid a live + rebuild (#7540). """ catalog_payload = { "provider_models": _PROVIDER_MODELS, @@ -6211,7 +6309,7 @@ def _models_cache_catalog_fingerprint() -> dict: codex_home = Path(os.getenv("CODEX_HOME", "").strip() or (HOME / ".codex")).expanduser() return { "provider_catalog_sha256": provider_catalog_sha, - "codex_models_cache": _models_cache_file_fingerprint(codex_home / "models_cache.json"), + "codex_models_cache": _codex_models_cache_fingerprint(codex_home / "models_cache.json"), } diff --git a/tests/test_issue7540_codex_catalog_fingerprint.py b/tests/test_issue7540_codex_catalog_fingerprint.py new file mode 100644 index 00000000..6449948d --- /dev/null +++ b/tests/test_issue7540_codex_catalog_fingerprint.py @@ -0,0 +1,386 @@ +"""Regression tests for issue #7540 — Codex models_cache.json churn must not +bust a valid /api/models catalog cache and force a live rebuild on session open. + +Bug shape +--------- +``_models_cache_source_fingerprint()`` covers the small local catalogs the +catalog depends on, including Codex's ``~/.codex/models_cache.json``, and +fingerprinted that file by ``mtime_ns`` + ``size`` +(``_models_cache_file_fingerprint``, #2443). Codex rewrites that file on its +own refresh timer, and each rewrite bumps both stat fields even when the model +payload is byte-identical — only ``fetched_at`` / ``updated_at`` move. So: + + Codex refresh -> fingerprint differs + -> ``_get_fresh_memory_models_cache()`` rejects the warm 24h snapshot + -> the session visit (#4756) then finds ``_is_loadable_disk_cache()`` + rejecting the disk snapshot for the same reason + -> ``get_available_models(force_refresh=True)`` -> serial live provider + probes on the session-open path (the multi-second hang in the report). + +Fix (maintainer recommendation on #7540: semantic fingerprint) +-------------------------------------------------------------- +``_codex_models_cache_fingerprint()`` hashes the file's *content* with the +refresh-timestamp keys deny-listed, mirroring the auth.json fix +(``_auth_store_semantic_fingerprint``, RCA t_16551f61). Metadata churn is +invisible to the fingerprint, while anything that actually feeds the Codex +model list we merge still changes it. + +These tests are INVARIANTS, not change-detectors: + + * churn-immune — a ``fetched_at``-only rewrite keeps the + fingerprint identical, keeps a previously-valid + disk cache loadable, and keeps a session visit + served from memory (no live rebuild). + * real-change-invalidates — client_version / etag / models[] / an unknown + future field each flip the fingerprint AND reject + the previously-valid disk cache, so the fix cannot + over-stabilise into serving a stale catalog. +""" + +import copy +import json +import os +import time + +import pytest + +import api.config as config + + +_TS_1 = "2026-09-13T12:00:00+00:00" +_TS_2 = "2026-09-13T15:45:11+00:00" + + +class _LiveRebuildReached(BaseException): + """Sentinel raised if a session visit escalates to a live provider rebuild. + + Deliberately NOT an ``Exception``: the session-visit path catches broad + ``Exception`` around its ``force_refresh`` call and falls back to a stale + on-disk snapshot, which would mask the regression. + """ + + +def _codex_cache(fetched_at=_TS_1, *, client_version="0.51.0", + etag='"codex-etag-1"', models=None, extra_top=None): + """Build a realistic ~/.codex/models_cache.json payload.""" + if models is None: + models = [ + {"slug": "gpt-5.3-codex", "visibility": "list", "priority": 1}, + {"slug": "gpt-5.1-codex-mini", "visibility": "list", "priority": 2}, + ] + payload = { + "fetched_at": fetched_at, + "client_version": client_version, + "etag": etag, + "models": models, + } + if extra_top: + payload.update(extra_top) + return payload + + +def _write_codex(tmp_path, monkeypatch, payload) -> "os.PathLike": + """Write the Codex cache into an isolated CODEX_HOME and point config at it.""" + codex_home = tmp_path / "codex_home" + codex_home.mkdir(parents=True, exist_ok=True) + cache_path = codex_home / "models_cache.json" + cache_path.write_text(json.dumps(payload, indent=2), encoding="utf-8") + monkeypatch.setenv("CODEX_HOME", str(codex_home)) + return cache_path + + +def _rewrite(path, payload): + """Rewrite the Codex cache and deterministically bump its mtime. + + ``os.utime`` is used instead of ``time.sleep`` so the mtime_ns change is + guaranteed on every filesystem/timestamp-resolution combination — the test + must prove the *old* stat fingerprint churned, not rely on clock luck. + """ + path.write_text(json.dumps(payload, indent=2), encoding="utf-8") + bumped = path.stat().st_mtime + 7.0 + os.utime(path, (bumped, bumped)) + return path + + +def _catalog_payload(): + return { + "active_provider": "codex", + "default_model": "gpt-5.3-codex", + "configured_model_badges": {"gpt-5.3-codex": "Codex"}, + "groups": [{"name": "Codex", "models": ["gpt-5.3-codex"]}], + } + + +# ── churn-immunity invariant ──────────────────────────────────────────────── + + +def test_codex_fetched_at_only_rewrite_keeps_fingerprint_identical(tmp_path, monkeypatch): + """Codex's own refresh moves ``fetched_at`` only — same models, same size.""" + p = _write_codex(tmp_path, monkeypatch, _codex_cache(fetched_at=_TS_1)) + fp_before = config._codex_models_cache_fingerprint(p) + st_before = p.stat() + + _rewrite(p, _codex_cache(fetched_at=_TS_2)) + fp_after = config._codex_models_cache_fingerprint(p) + st_after = p.stat() + + # Pre-condition: this really is the mtime/size churn from the report — the + # old stat-based fingerprint WOULD have changed. + assert (st_before.st_mtime_ns, st_before.st_size) != ( + st_after.st_mtime_ns, st_after.st_size), "test setup: rewrite must churn the stat fields" + # Invariant: the semantic fingerprint does not. + assert fp_before == fp_after, ( + "A Codex refresh that only moves fetched_at must NOT change the catalog " + "fingerprint (#7540) — otherwise the 24h cache is rejected and the next " + "session visit pays a live rebuild" + ) + assert "semantic_sha256" in fp_before + + +def test_unsorted_codex_payload_key_order_keeps_fingerprint_identical(tmp_path, monkeypatch): + """Key order is not semantic: a reordered but otherwise identical file must + not bust the cache.""" + p = _write_codex(tmp_path, monkeypatch, _codex_cache()) + fp_before = config._codex_models_cache_fingerprint(p) + + reordered = { + "models": _codex_cache()["models"], + "etag": _codex_cache()["etag"], + "client_version": _codex_cache()["client_version"], + "fetched_at": _codex_cache()["fetched_at"], + } + _rewrite(p, reordered) + + assert config._codex_models_cache_fingerprint(p) == fp_before + + +def test_codex_metadata_churn_does_not_reject_valid_disk_models_cache(tmp_path, monkeypatch): + """End-to-end: the disk snapshot a warm server wrote must still load after + Codex refreshed its own cache.""" + p = _write_codex(tmp_path, monkeypatch, _codex_cache(fetched_at=_TS_1)) + monkeypatch.setattr(config, "_models_cache_path", tmp_path / "models_cache.json") + config._save_models_cache_to_disk(_catalog_payload()) + assert config._load_models_cache_from_disk() is not None + + _rewrite(p, _codex_cache(fetched_at=_TS_2)) + + assert config._load_models_cache_from_disk() is not None, ( + "Codex metadata churn must NOT reject a valid on-disk models cache " + "(#7540) — that rejection is what triggers the live rebuild" + ) + + +def test_session_visit_after_codex_refresh_needs_no_live_rebuild(tmp_path, monkeypatch): + """The user-visible symptom: open a conversation after Codex refreshed. + + The warm in-memory snapshot must still be served; a rebuild here is the + multi-second session-open hang in the report. + """ + _write_codex(tmp_path, monkeypatch, _codex_cache(fetched_at=_TS_1)) + monkeypatch.setattr(config, "_models_cache_path", tmp_path / "models_cache.json") + payload = _catalog_payload() + config._save_models_cache_to_disk(payload) + + # Warm the memory cache the way a normal /api/models call leaves it. + monkeypatch.setattr(config, "_available_models_cache", copy.deepcopy(payload)) + monkeypatch.setattr(config, "_available_models_cache_ts", time.monotonic()) + monkeypatch.setattr( + config, + "_available_models_cache_source_fingerprint", + config._models_cache_source_fingerprint(), + ) + + # Codex's refresh timer fires between two session opens. + _rewrite(tmp_path / "codex_home" / "models_cache.json", + _codex_cache(fetched_at=_TS_2)) + + def _boom(**_kwargs): + # BaseException, not Exception: get_available_models_for_session_visit + # swallows exceptions from its force_refresh path and falls back to the + # on-disk stale snapshot, so an AssertionError sentinel would be eaten + # and the test would pass while quietly taking the slow path. + raise _LiveRebuildReached( + "session visit fell through to the live rebuild: Codex fetched_at " + "churn invalidated a still-valid catalog cache (#7540)" + ) + + monkeypatch.setattr(config, "get_available_models", _boom) + + result = config.get_available_models_for_session_visit() + + assert result is not None + assert result["default_model"] == "gpt-5.3-codex" + + +def test_warm_memory_cache_survives_codex_churn(tmp_path, monkeypatch): + """Isolates the invalidation point itself (the memory-cache rejection that + precedes the disk load and the force_refresh).""" + _write_codex(tmp_path, monkeypatch, _codex_cache(fetched_at=_TS_1)) + monkeypatch.setattr(config, "_models_cache_path", tmp_path / "models_cache.json") + payload = _catalog_payload() + config._save_models_cache_to_disk(payload) + monkeypatch.setattr(config, "_available_models_cache", copy.deepcopy(payload)) + monkeypatch.setattr(config, "_available_models_cache_ts", time.monotonic()) + monkeypatch.setattr( + config, + "_available_models_cache_source_fingerprint", + config._models_cache_source_fingerprint(), + ) + + _rewrite(tmp_path / "codex_home" / "models_cache.json", + _codex_cache(fetched_at=_TS_2)) + + assert config._get_fresh_memory_models_cache(time.monotonic()) is not None, ( + "The warm in-memory catalog cache must survive a Codex fetched_at-only " + "refresh (#7540)" + ) + + +# ── real-change-invalidates invariant ─────────────────────────────────────── + + +def test_new_codex_model_changes_fingerprint(tmp_path, monkeypatch): + p = _write_codex(tmp_path, monkeypatch, _codex_cache()) + fp_before = config._codex_models_cache_fingerprint(p) + + models = _codex_cache()["models"] + [ + {"slug": "gpt-5.4-codex", "visibility": "list", "priority": 3}, + ] + _rewrite(p, _codex_cache(models=models)) + + assert config._codex_models_cache_fingerprint(p) != fp_before, ( + "A new Codex model changes the merged catalog and MUST bust the cache" + ) + + +def test_model_visibility_change_changes_fingerprint(tmp_path, monkeypatch): + """``visibility`` gates whether the slug is surfaced — a hide/show flip is a + real catalog change at unchanged file size.""" + p = _write_codex(tmp_path, monkeypatch, _codex_cache()) + fp_before = config._codex_models_cache_fingerprint(p) + + models = copy.deepcopy(_codex_cache()["models"]) + models[0]["visibility"] = "hidden" + _rewrite(p, _codex_cache(models=models)) + + assert config._codex_models_cache_fingerprint(p) != fp_before + + +@pytest.mark.parametrize( + "changed", + [ + {"client_version": "0.52.0"}, + {"etag": '"codex-etag-2"'}, + {"models": [{"slug": "gpt-5.3-codex", "priority": 9}]}, + ], +) +def test_codex_catalog_fields_stay_in_fingerprint(tmp_path, monkeypatch, changed): + """Everything that actually feeds the Codex model list stays covered.""" + p = _write_codex(tmp_path, monkeypatch, _codex_cache()) + fp_before = config._codex_models_cache_fingerprint(p) + + _rewrite(p, _codex_cache(**changed)) + + assert config._codex_models_cache_fingerprint(p) != fp_before + + +def test_unknown_codex_field_stays_in_fingerprint(tmp_path, monkeypatch): + """Deny-list (not allow-list) safety: an unknown field is NOT stripped, so a + hypothetical future model-gating field still busts the cache.""" + p = _write_codex(tmp_path, monkeypatch, + _codex_cache(extra_top={"some_future_model_gate": "v1"})) + fp_before = config._codex_models_cache_fingerprint(p) + + _rewrite(p, _codex_cache(extra_top={"some_future_model_gate": "v2"})) + + assert config._codex_models_cache_fingerprint(p) != fp_before, ( + "An unknown (non-deny-listed) field must remain in the fingerprint — the " + "deny-list must fail safe toward over-invalidation" + ) + + +def test_real_codex_change_still_rejects_previously_valid_disk_cache(tmp_path, monkeypatch): + """Anti-over-stabilisation mirror of the churn test: a disk snapshot that WAS + valid must be rejected once a genuine Codex model change lands.""" + p = _write_codex(tmp_path, monkeypatch, _codex_cache()) + monkeypatch.setattr(config, "_models_cache_path", tmp_path / "models_cache.json") + config._save_models_cache_to_disk(_catalog_payload()) + assert config._load_models_cache_from_disk() is not None + + _rewrite(p, _codex_cache(client_version="0.52.0")) + + assert config._load_models_cache_from_disk() is None, ( + "A real Codex catalog change MUST reject the stale disk cache — the fix " + "must not over-stabilise into serving wrong data" + ) + + +# ── helper unit guards ────────────────────────────────────────────────────── + + +def test_strip_volatile_codex_fields_is_pure_and_recursive(): + src = { + "fetched_at": "ts", + "client_version": "0.51.0", + "models": [{"slug": "gpt-5.3-codex", "updated_at": "ts", "priority": 1}], + } + snapshot = copy.deepcopy(src) + out = config._strip_volatile_codex_cache_fields(src) + + assert src == snapshot # input untouched (pure) + assert out == { + "client_version": "0.51.0", + "models": [{"slug": "gpt-5.3-codex", "priority": 1}], + } + + +def test_unparseable_codex_cache_falls_back_to_stat_fingerprint(tmp_path, monkeypatch): + """Corrupt / mid-write file: fall back to the old stat fingerprint instead of + silently pinning a hash of garbage — behaviour is never less safe than the + stat-based version, and a later good file still invalidates.""" + p = _write_codex(tmp_path, monkeypatch, _codex_cache()) + p.write_text('{"models": [', encoding="utf-8") + + fp = config._codex_models_cache_fingerprint(p) + assert fp.get("semantic") == "unparsed-fallback" + assert "mtime_ns" in fp and "size" in fp + + _rewrite(p, _codex_cache(fetched_at=_TS_2)) + assert config._codex_models_cache_fingerprint(p) != fp + + +def test_missing_codex_cache_fingerprint_is_stable_and_marked(tmp_path, monkeypatch): + monkeypatch.setenv("CODEX_HOME", str(tmp_path / "absent")) + fp = config._codex_models_cache_fingerprint(tmp_path / "absent" / "models_cache.json") + + assert fp.get("missing") is True + assert config._codex_models_cache_fingerprint( + tmp_path / "absent" / "models_cache.json") == fp + + +def test_deeply_nested_codex_cache_degrades_to_stat_fallback_without_crashing(tmp_path, monkeypatch): + """A strip failure (e.g. RecursionError on a deep tree) must not 500 /api/models. + + The recursive volatile-field strip runs inside the encode try/except, so if + it raises (RecursionError on a pathologically deep JSON tree, or any other + error) the fingerprint degrades to the stat-based fallback instead of the + exception escaping and turning /api/models into an HTTP 500. (#7540 gate + finding.) We force the strip to raise directly so the guard is exercised + deterministically regardless of the ambient recursion depth. + """ + p = _write_codex(tmp_path, monkeypatch, _codex_cache(fetched_at=_TS_1)) + + def _boom(_obj): + raise RecursionError("simulated deep-tree strip overflow") + + monkeypatch.setattr(config, "_strip_volatile_codex_cache_fields", _boom) + + # Must not raise; must degrade to the stat-based fingerprint. + fp = config._codex_models_cache_fingerprint(p) + fp2 = config._codex_models_cache_fingerprint(p) + + assert isinstance(fp, dict) + assert fp.get("semantic") == "encode-fallback" + assert "mtime_ns" in fp and "size" in fp + assert "semantic_sha256" not in fp # the content hash was NOT produced + assert fp2 == fp # stable across repeated calls on the unchanged file