Skip to content

fix(agent): use atomic writes for context length YAML cache - #35140

Closed
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/model-metadata-atomic-write
Closed

fix(agent): use atomic writes for context length YAML cache#35140
annguyenNous wants to merge 1 commit into
NousResearch:mainfrom
annguyenNous:fix/model-metadata-atomic-write

Conversation

@annguyenNous

Copy link
Copy Markdown
Contributor

Problem

save_context_length() and _invalidate_cached_context_length() in agent/model_metadata.py write directly to the YAML context length cache file using open(path, "w") + yaml.dump(). If the process crashes mid-write, the file is truncated to zero or partial bytes, corrupting the cache and losing all cached context lengths.

The codebase already has atomic_yaml_write() in utils.py which uses the correct temp-file + fsync + os.replace() pattern.

Fix

Replace direct open(path, "w") + yaml.dump() calls with atomic_yaml_write(path, data). The existing helper handles directory creation, temp file, fsync, atomic rename, and file permission preservation.

Before vs After

Scenario Before After
Crash during write Cache file corrupted (0 bytes or partial YAML) Previous cache version intact
Normal write Direct write to target Temp file → fsync → atomic rename

Tests

All 102 model_metadata tests pass, including:

  • TestContextLengthCache::test_save_and_load
  • TestContextLengthCache::test_corrupted_yaml_returns_empty
  • TestContextLengthCache::test_idempotent_save

save_context_length() and _invalidate_cached_context_length() wrote
directly to the YAML cache file with open(path, 'w'). A crash mid-write
truncates the file to zero/partial bytes, corrupting the cache and
losing all cached context lengths.

Replace with atomic_yaml_write() from utils.py which uses temp file +
fsync + os.replace — the previous version remains intact if the process
crashes during the write.
@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 P3 Low — cosmetic, nice to have labels May 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Subset of #29019 which also converts model_metadata to atomic writes (among 6 other files). Focused single-file fix.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Overview

Clean 3-line fix replacing direct file writes with atomic_yaml_write for the context length YAML cache. 3 additions, 7 deletions.

What's Right

  • Uses existing atomic_yaml_write utility that handles temp file, fsync, os.replace, and permissions
  • Prevents cache corruption from mid-write crashes
  • Removes duplicated mkdir+open+write pattern (DRY improvement)
  • All 102 model_metadata tests pass
  • Minimal, focused change with clear safety benefit

Safety

  • atomic_yaml_write creates temp file in same directory (same filesystem, rename is atomic)
  • fsync ensures data reaches disk before rename
  • os.replace is atomic on POSIX, falls back to shutil.move on Windows
  • Previous cache version preserved if crash occurs during write

Reviewed by Hermes Agent (cron)

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused crash-safety fix. The premise is confirmed on current main: both save_context_length() and _invalidate_cached_context_length() still directly overwrite the cache at agent/model_metadata.py:1109 and agent/model_metadata.py:1157. The proposed helper already provides the intended temp-file, fsync, and replacement semantics (utils.py:227).

Problems

  • The PR does not add a regression test for these two call sites. Existing cache tests cover normal behavior (tests/agent/test_model_metadata.py:1483), while interruption preservation currently covers the helper alone (tests/hermes_cli/test_atomic_yaml_write.py:20).

Suggested changes

  • Add a targeted failure-path test that seeds the context-length cache, forces YAML serialization to fail during each writer, and verifies the original cache remains intact.

Automated hermes-sweeper review.

@teknium1

Copy link
Copy Markdown
Contributor

Resolved via PR #85509 (merge commit 6def7ce). You were the FIRST to submit this fix — 8 days before the duplicate #40919 — and you're credited as first submitter in the salvage PR body. The cherry-pick used #40919's implementation because it carried a regression test for the interrupted-write case; the code change itself is the same atomic_yaml_write() swap you proposed. Thanks for catching the torn-YAML wipe risk first.

@teknium1 teknium1 closed this Aug 13, 2026
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 P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants