Skip to content

test(skills): deflake config-cache invalidation on coarse-mtime filesystems - #65037

Closed
vadimcomanescu wants to merge 1 commit into
NousResearch:mainfrom
vadimcomanescu:test/deflake-skill-config-cache-mtime
Closed

test(skills): deflake config-cache invalidation on coarse-mtime filesystems#65037
vadimcomanescu wants to merge 1 commit into
NousResearch:mainfrom
vadimcomanescu:test/deflake-skill-config-cache-mtime

Conversation

@vadimcomanescu

@vadimcomanescu vadimcomanescu commented Jul 15, 2026

Copy link
Copy Markdown

What does this PR do?

Deflakes tests/agent/test_skill_utils.py::test_skill_config_raw_cache_invalidates_on_config_edit, which fails intermittently on CI runners whose filesystem has coarse (1-second) mtime granularity.

The raw config cache in agent/skill_utils.py keys entries on (path, st_mtime_ns, st_size) (introduced in #46149). The test writes config.yaml with disabled: [old-skill], reads it (populating the cache), rewrites it with disabled: [new-skill], then asserts the next read reflects the edit. Both configs are the same byte length (32 bytes), so st_size is identical and st_mtime_ns is the only field that can invalidate the entry.

The test forces a mtime change with os.utime(config_path, None) — added in #46149 alongside the cache precisely to make the edit detectable. But None stamps the current time: on a filesystem with 1s mtime granularity, when both writes land in the same second the second write shares the first write's mtime, the cache key doesn't change, the stale entry stays cached, and the final assertion flakes. Runners with nanosecond mtime resolution never hit this, which is why it passes almost everywhere and only flakes under coarse-granularity/loaded runners.

The fix keeps that intent but makes it deterministic: stamp an mtime a full second past the file's current value, which survives second-granularity truncation. It's a test-only change — the cache implementation is unchanged and correct for real usage, where config edits are always more than one mtime tick apart.

Related Issue

No tracked issue — surfaced as an intermittent CI failure. Searched open and merged PRs/issues for skill_config_raw_cache / get_disabled_skill_names cache and found no existing report or fix. Cache and test both originate in #46149.

Type of Change

  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tests/agent/test_skill_utils.py: replace os.utime(config_path, None) with an explicit +1s mtime bump (os.utime(config_path, ns=(bumped_ns, bumped_ns))) so the cache-invalidation assertion is deterministic under coarse mtime granularity. Comment added explaining why.

How to Test

  1. scripts/run_tests.sh tests/agent/test_skill_utils.py -q — 24 passed.
  2. The target test passes deterministically across repeated runs.
  3. No production code changed; the cache contract (mtime change ⇒ invalidation) is exactly what the test still verifies.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (test(skills): ...)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (single one-hunk test change)
  • I've run the test suite for the touched file and it passes
  • I've added/improved tests for my changes (this is the test change)
  • I've tested on my platform: macOS 15 (Darwin 25.5)

Documentation & Housekeeping

  • Documentation — N/A (test-only)
  • cli-config.yaml.example — N/A (no config keys)
  • CONTRIBUTING.md / AGENTS.md — N/A (no architecture/workflow change)
  • Cross-platform impact considered — the fix specifically hardens the test across filesystems with different mtime granularity
  • Tool descriptions/schemas — N/A

🤖 Generated with Claude Code

…ystems

The raw config cache keys on (path, st_mtime_ns, st_size). In
test_skill_config_raw_cache_invalidates_on_config_edit both configs are
the same byte length, so mtime is the only field that can invalidate the
entry. The test rewrote the file and called os.utime(config_path, None),
stamping the current time — on filesystems with coarse (1s) mtime
granularity the second write can share the first write's mtime when both
land in the same second, so the stale entry stays cached and the final
assertion flakes.

Bump the mtime a full second past its current value instead, which
survives second-granularity truncation and makes the invalidation
deterministic on every filesystem. No production change.
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation labels Jul 15, 2026

@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

Looks Good

  • Test(skills): deflake config-cache invalidation on coarse-mtime filesystems
  • 10 additions, 1 deletion — test improvement
  • No issues detected

Reviewed by Hermes Agent

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks @vadimcomanescu — closing: the sole target test (skill_config_raw_cache_invalidates…) was removed in the suite-wide test prune (PR #74383). The coarse-mtime deflake approach was sound — if the class resurfaces in a surviving test, this design is the right one.

@teknium1 teknium1 closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation tool/skills Skills system (list, view, manage) type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants