Skip to content

fix(models): write context-length cache atomically - #40919

Closed
sasquatch9818 wants to merge 1 commit into
NousResearch:mainfrom
sasquatch9818:fix/atomic-context-length-cache-write
Closed

fix(models): write context-length cache atomically#40919
sasquatch9818 wants to merge 1 commit into
NousResearch:mainfrom
sasquatch9818:fix/atomic-context-length-cache-write

Conversation

@sasquatch9818

Copy link
Copy Markdown
Contributor

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.

What does this PR do?

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.

Related Issue

N/A

Type of Change

  • 🐛 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)

Changes Made

  • 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.

How to Test

  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.

Checklist

Code

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

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) — the helper uses os.replace, which is atomic on both
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

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.

## What does this PR do?

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.

## Related Issue

N/A

## Type of Change

- [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)

## Changes Made

- `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.

## How to Test

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.

## Checklist

### Code

- [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)

### Documentation & Housekeeping

- [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
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists labels Jun 7, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Thanks for this fix. Verified the data-loss scenario and the repair:

  1. Root cause confirmed: save_context_length and _invalidate_cached_context_length both used truncating open(path, "w") + yaml.dump(). A crash/kill between truncate and dump-complete leaves the file empty or partial. The downstream _load_context_cache() catches yaml.YAMLError and returns {} — silently wiping every cached context length.

  2. Fix completeness: Both write sites in model_metadata.py are converted (lines ~854 and ~882 in the original). No other open(path, "w") + yaml.dump patterns remain in this file.

  3. atomic_yaml_write from utils: Reuses the existing atomic-write helper (temp file + fsync + os.replace), which is the right approach. The old path.parent.mkdir(parents=True, exist_ok=True) is no longer needed because atomic_yaml_write handles directory creation internally.

  4. Test quality: The crash-simulation test is excellent — it monkeypatches atomic_replace to raise OSError, then verifies: (a) the original file survives byte-for-byte, (b) the cached value is still readable, (c) no stray temp files remain. This directly validates the data-loss invariant.

Clean fix for a real data-safety bug. 👍

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused crash-safety fix. Current main still performs direct truncating writes in save_context_length() (agent/model_metadata.py:1110) and _invalidate_cached_context_length() (agent/model_metadata.py:1159). The proposed helper already writes, fsyncs, and swaps a temp file (utils.py:227-293), so it addresses the stated failure mode without adding new surface area.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #85509 (merge commit 6def7ce) — your commit was cherry-picked onto current main with your authorship preserved in git log. Yours was picked over the duplicate #35140 for the regression test (interrupted write leaves the old cache intact) and the explanatory comments; @annguyenNous submitted the same fix 8 days earlier and gets first-submitter credit in the salvage PR body. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants