Skip to content

perf(skills): cache config.yaml reads with mtime/size invalidation - #16202

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:perf/cache-skills-config-yaml-reads
Closed

perf(skills): cache config.yaml reads with mtime/size invalidation#16202
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:perf/cache-skills-config-yaml-reads

Conversation

@briandevans

@briandevans briandevans commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

Summary

build_skills_system_prompt() calls get_disabled_skill_names() and get_external_skills_dirs() on every invocation — and both run before the in-process LRU prompt cache lookup at agent/prompt_builder.py:663. Each previously re-read and re-parsed ~/.hermes/config.yaml, so building (or even cache-hitting) the skills prompt did two file reads + two YAML parses per call. In gateway mode that's per session.

This PR adds a tiny in-process cache around the config-file read inside agent/skill_utils.py, keyed by config path and invalidated by (st_mtime_ns, st_size, st_ino).

Benchmark on macOS APFS, parsing a representative skills.disabled config 5000×:

cached:   8.0 µs/call
uncached: 44.9 µs/call
speedup:  5.6×

That's ~37 µs trimmed off every build_skills_system_prompt() cache hit and ~74 µs off every cold cache miss — multiplied by every gateway session and every skills_list tool call.

Audit / why no downside

Concern Resolution
Cache stale after user edit (vim, editor save) In-place edits change mtime_ns → cache invalidates.
Cache stale after atomic write atomic_yaml_write (used by gateway/run.py and hermes_cli/config.py to mutate config.yaml at runtime) does temp-file + os.replace, which always assigns a fresh inode. st_ino in the signature catches this even on filesystems with coarse mtime resolution.
Same-size in-place rewrite at same nanosecond Inode unchanged but mtime_ns is full ns-resolution on APFS/ext4 — collision is vanishingly improbable. Even on FAT32 (2-second resolution), this requires identical content size with different content, which the file would have to be. Edge case is far below background failure rates of the surrounding system.
Concurrent readers All cache mutations are inside threading.Lock. Verified with 8 threads × 50 reads, zero corruption.
Caller mutates returned set/list get_disabled_skill_names returns a fresh set built by _normalize_string_set; get_external_skills_dirs returns a fresh list. Verified with mutation tests.
Caller mutates returned cached dict _read_config_cached is a private helper; both in-process callers in skill_utils.py are read-only. Documented as read-only contract.
Missing config.yaml Returns {}, cached with sig=None. Self-corrects when file is created (different sig → re-read).
Malformed YAML Exception caught, returns {}, cached. Logged at DEBUG level (existing behaviour preserved).
Memory growth in tests with many monkeypatch.setenv("HERMES_HOME", ...) Cache capped at 32 entries with FIFO eviction. Verified by test.
Cross-test pollution Per-test tmp_path ⇒ unique config path ⇒ unique cache key. Test fixture also clears the cache before/after each test.
Production callers expecting fresh reads All 14 production callers (tools/skills_tool.py, tools/credential_files.py, hermes_cli/skills_hub.py, hermes_cli/commands.py, agent/skill_commands.py, gateway/run.py, agent/prompt_builder.py) are read-only. None edit config.yaml then immediately re-read inside one call frame; the few that write follow the "takes effect on next message" pattern, by which point the cache has already invalidated via stat-sig change.

Test plan

  • pytest tests/agent/test_skill_utils_config_cache.py -v9 cases, all pass
    • hot-path: repeated reads avoid YAML parse
    • mtime change invalidates cache
    • missing file: cached as empty
    • malformed YAML: cached as empty
    • atomic_yaml_write round-trip invalidates cache
    • mutating returned set doesn't corrupt cache
    • mutating returned list doesn't corrupt cache
    • 8 concurrent reader threads × 50 reads — no corruption, no races
    • cache stays bounded under many distinct HERMES_HOME paths
  • 385 existing skill/prompt tests pass: tests/agent/test_external_skills.py tests/agent/test_prompt_builder.py tests/agent/test_skill_commands.py tests/hermes_cli/test_skills_config.py tests/hermes_cli/test_skills_hub.py tests/tools/test_skill_manager_tool.py tests/tools/test_skills_sync.py tests/tools/test_skill_view_path_check.py tests/tools/test_skills_guard.py tests/tools/test_skills_hub_clawhub.py
  • Manual bench (above) confirms ~5.6× speedup on hot path
  • Tested on macOS (Darwin 25.5)

What changed

  • agent/skill_utils.py: new _read_config_cached() + _clear_config_cache() helpers; get_disabled_skill_names and get_external_skills_dirs route through them
  • tests/agent/test_skill_utils_config_cache.py: 9 tests pinning hit/miss/invalidation/concurrency/mutation-safety/bounded-memory behaviour

Copilot AI review requested due to automatic review settings April 26, 2026 19:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds an in-process cache for ~/.hermes/config.yaml parsing in agent/skill_utils.py so repeated calls to skill config helpers don’t re-read/re-parse YAML on every build_skills_system_prompt() invocation.

Changes:

  • Introduce _read_config_cached() with (st_mtime_ns, st_size) invalidation and bounded in-memory storage.
  • Route get_disabled_skill_names() and get_external_skills_dirs() through the cached config reader.
  • Add focused pytest coverage for cache hits, invalidation, missing file behavior, and malformed YAML handling.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
agent/skill_utils.py Implements a lock-protected, bounded cache for config.yaml parsing and updates key helpers to use it.
tests/agent/test_skill_utils_config_cache.py Adds tests to pin cache behavior (deduping parses, invalidation on change, and safe fallback cases).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alt-glitch alt-glitch added type/perf Performance improvement or optimization P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/skills Skills system (list, view, manage) labels Apr 26, 2026
build_skills_system_prompt() calls get_disabled_skill_names() and
get_external_skills_dirs() on every invocation — including before the
in-process LRU prompt cache lookup — so each call previously re-read and
re-parsed ~/.hermes/config.yaml twice. In gateway mode that's two file
reads + two YAML parses per session.

Add a small in-process cache keyed by config path, invalidated by
(st_mtime_ns, st_size). User edits still take effect immediately because
any write changes mtime (and usually size). Bench on macOS APFS: ~6.3x
faster on the cached path (51us -> 8us per call).

No behaviour change — same return values, just memoized.
Audit follow-up to the prior commit:

* Add ``st_ino`` to the cache signature.  ``atomic_yaml_write`` (used by
  gateway/CLI runtime config edits) writes a temp file and ``os.replace``s
  it onto config.yaml — the new file always has a fresh inode.  On
  filesystems with coarse mtime resolution, two same-size writes within
  one tick would otherwise share the same (mtime, size) tuple; including
  the inode makes the invalidation bulletproof against atomic replacement.

* Strengthen the read-only contract docstring on the returned dict and
  document why callers (``get_disabled_skill_names`` /
  ``get_external_skills_dirs``) are safe — both return fresh sets/lists.

* Expand the test suite from 4 to 9 cases:
  - atomic_yaml_write round-trip invalidates the cache (the actual
    runtime path used by gateway/CLI to edit config.yaml)
  - mutating the returned set never corrupts subsequent reads
  - mutating the returned list never corrupts subsequent reads
  - 8 concurrent reader threads × 50 reads each — no corruption, no
    races, all results identical
  - cache size stays bounded (≤ MAX_ENTRIES) under many distinct
    HERMES_HOME paths
@briandevans
briandevans force-pushed the perf/cache-skills-config-yaml-reads branch from eb77e46 to 66bbe1e Compare April 29, 2026 19:07
@briandevans

Copy link
Copy Markdown
Contributor Author

Closing — this fix has effectively landed via #22138 (commit 0ec052ca2 "perf(cli): cut ~19s from 'hermes' cold start"). That PR added the same (config_path, mtime_ns) in-process memo on get_external_skills_dirs() in agent/skill_utils.py with the same stat-vs-parse cost analysis. Happy to reopen if there's still a remaining gap I missed.

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 tool/skills Skills system (list, view, manage) type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants