fix(models): write context-length cache atomically

save_context_length() and _invalidate_cached_context_length() did an
unguarded read-modify-write into $HERMES_HOME/context_length_cache.yaml.
The plain `open(path, "w")` truncates the file before the dump runs. If
the process is killed mid-dump, the file is left empty or partial. The
next _load_context_cache() swallows the YAML error and returns {} —
silently wiping every persisted context length. A concurrent process
reading between truncate and dump-complete also sees a torn file.

After the cache is lost, every model re-probes the network, and when a
probe fails it falls back to the generic 256K default — so a user on a
1M-window model ends up with a wrong, short context window.

Hermes routinely runs several processes against one shared $HERMES_HOME
(a cron agent plus an interactive session, multiple gateway sessions),
so this is hit in normal use.

Switch both writers to the existing utils.atomic_yaml_write helper
(temp file + fsync + os.replace, symlink- and mode-preserving). The real
file is only ever swapped from a fully written temp file, so an
interrupted write leaves the previous cache intact and readers never see
a partial file. Matches the atomic-write pattern already used for
auth.json, config.yaml, and other persisted state.

Makes the persistent model context-length cache write crash-safe. The
old non-atomic write could truncate or wipe the entire cache on an
interrupted or concurrent write, which then forces models onto the wrong
fallback context window. The fix routes both cache writers through the
repo's atomic temp-file + os.replace helper.

N/A

- [x] 🐛 Bug fix (non-breaking change that fixes an issue)
- [ ] ✨ New feature (non-breaking change that adds functionality)
- [ ] 🔒 Security fix
- [ ] 📝 Documentation update
- [ ] ✅ Tests (adding or improving test coverage)
- [ ] ♻️ Refactor (no behavior change)
- [ ] 🎯 New skill (bundled or hub)

- `agent/model_metadata.py`: `save_context_length()` and
  `_invalidate_cached_context_length()` now write via
  `utils.atomic_yaml_write` instead of a truncating `open(path, "w")`.
  Added the `atomic_yaml_write` import.
- `tests/agent/test_model_metadata.py`: added
  `test_write_failure_leaves_existing_cache_intact` — simulates a crash
  during the atomic swap and asserts the existing cache survives
  byte-for-byte with no stray temp file.

1. `pytest tests/agent/test_model_metadata.py -q` — 98 pass, including
   the new crash-safety test.
2. The new test seeds a valid cache, forces the swap step to raise, and
   confirms the file is not truncated and no `.cache_*.tmp` is left.
3. `ruff check agent/model_metadata.py` passes.

- [x] I've read the Contributing Guide
- [x] My commit messages follow Conventional Commits (`fix(scope):`, etc.)
- [x] I searched for existing PRs to make sure this isn't a duplicate
- [x] My PR contains **only** changes related to this fix
- [x] I've run the affected tests (`pytest tests/agent/test_model_metadata.py -q`) and they pass
- [x] I've added tests for my changes
- [x] I've tested on my platform: macOS 15 (Darwin 25.5)

- [x] I've updated relevant documentation (README, `docs/`, docstrings) — or N/A
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A
- [x] I've considered cross-platform impact (Windows, macOS) — the helper uses os.replace, which is atomic on both
- [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A
This commit is contained in:
sasquatch9818
2026-06-07 05:20:59 +03:00
committed by Teknium
parent acadd719d3
commit 6def7ce1df
2 changed files with 46 additions and 7 deletions
+11 -7
View File
@@ -21,7 +21,7 @@ import yaml
if TYPE_CHECKING: # pragma: no cover — runtime import is lazy (see below)
import requests
from utils import atomic_json_write, base_url_host_matches, base_url_hostname
from utils import atomic_json_write, atomic_yaml_write, base_url_host_matches, base_url_hostname
from hermes_constants import OPENROUTER_MODELS_URL
@@ -1476,9 +1476,13 @@ def save_context_length(model: str, base_url: str, length: int) -> None:
cache[key] = length
path = _get_context_cache_path()
try:
path.parent.mkdir(parents=True, exist_ok=True)
with open(path, "w", encoding="utf-8") as f:
yaml.dump({"context_lengths": cache}, f, default_flow_style=False)
# Atomic write (temp file + fsync + os.replace): a plain truncating
# ``open(path, "w")`` leaves the file empty/partial if the process is
# killed mid-dump, and the next _load_context_cache() swallows the
# resulting YAML error and returns {} — silently wiping EVERY cached
# context length. It also exposes torn reads to a concurrent process
# reading between truncate and dump-complete.
atomic_yaml_write(path, {"context_lengths": cache})
logger.info("Cached context length %s -> %s tokens", key, f"{length:,}")
except Exception as e:
logger.debug("Failed to save context length cache: %s", e)
@@ -1525,9 +1529,9 @@ def _invalidate_cached_context_length(model: str, base_url: str) -> None:
cache.pop(k, None)
path = _get_context_cache_path()
try:
path.parent.mkdir(parents=True, exist_ok=True)
with open(path, "w", encoding="utf-8") as f:
yaml.dump({"context_lengths": cache}, f, default_flow_style=False)
# Atomic write — see save_context_length() for why a plain truncating
# open() here risks wiping the entire cache on an interrupted dump.
atomic_yaml_write(path, {"context_lengths": cache})
except Exception as e:
logger.debug("Failed to invalidate context length cache entry %s: %s", key, e)
+35
View File
@@ -1206,6 +1206,41 @@ class TestContextLengthCache:
assert get_model_context_length("unknown/model", base_url="http://local") == 65536
def test_write_failure_leaves_existing_cache_intact(self, tmp_path, monkeypatch):
"""An interrupted write must not corrupt or wipe the existing cache.
The old non-atomic ``open(path, "w")`` truncated the file before
dumping, so a crash/kill mid-write left empty or partial YAML — and
the next load swallowed the error and returned ``{}``, silently
wiping EVERY persisted context length. The atomic temp-file +
``os.replace`` write leaves the previous file byte-for-byte intact
when the swap fails.
"""
import utils
import agent.model_metadata as mm
cache_file = tmp_path / "cache.yaml"
monkeypatch.setattr(mm, "_get_context_cache_path", lambda: cache_file)
# Seed a valid, populated cache.
save_context_length("model-a", "http://a", 64000)
original_bytes = cache_file.read_bytes()
# Simulate a crash during the atomic swap step.
def _boom(*_args, **_kwargs):
raise OSError("simulated crash during atomic replace")
monkeypatch.setattr(utils, "atomic_replace", _boom)
# save_context_length is best-effort and swallows the error.
save_context_length("model-b", "http://b", 128000)
# Original file survives untouched — not truncated or emptied.
assert cache_file.read_bytes() == original_bytes
assert get_cached_context_length("model-a", "http://a") == 64000
# The failed write must not leave a stray temp file behind.
assert list(cache_file.parent.glob(".cache_*.tmp")) == []
class TestGrok43StaleCacheGuard:
"""Pre-catalog builds resolved grok-4.3 via the generic 'grok-4' catch-all