fix(test): make the workspace-summary cache test deterministic on Windows - #221
fix(test): make the workspace-summary cache test deterministic on Windows#221Broccolito wants to merge 1 commit into
Conversation
…dows
`agents::workspace_summary::tests::test_cache_serves_and_invalidates_on_mtime`
failed once on `test (windows-latest)` (run 34513035963, on a PR whose diff does
not touch this file) and passed on a rerun of the same commit. Two defects, both
in the test.
**The promised nudge did not exist.** The test wrote a second file and
immediately re-read, under a comment reading "mtime resolution can be coarse;
nudge the directory to be safe" — with no nudge following it. The Windows clock
ticks roughly every 15ms and NTFS updates a directory's own mtime lazily, so the
two writes could leave it byte-identical; the cache then served the stale map
and neither summary named the new file.
**The assertion could not fail.** `refreshed.contains("two.txt") ||
first.contains("two.txt")` captured `first` before `two.txt` existed, so the
second disjunct was dead. It read as tolerating a race and tolerated nothing.
Take the signal rather than hope for it: `filetime::set_file_mtime` pins the
directory's mtime (`std::fs` cannot — a directory needs
`FILE_FLAG_BACKUP_SEMANTICS` to open for write on Windows), and `pin_dir_mtime`
reads it back, so a filesystem that refuses the value fails naming the timestamp
instead of downstream where a stale map reads as a cache bug. The test's name
promises two behaviours and now checks both: half 1 restores the mtime and
asserts the cached map comes back unchanged, half 2 moves it forward and asserts
the new file surfaces. Verified by mutation — dropping the mtime comparison
fails half 2, and never serving from the cache fails half 1.
`clear_cache()` becomes `forget_cached(working_dir)`, with both it and
`workspace_summary` deriving the key through a shared `cache_key`. `CACHE` is
process-global and `cargo test` runs a crate's unit tests in parallel threads of
one process, where `test_collect_moim_uses_minute_granularity` reaches it via
`collect_moim`. Keys never collide, since each test owns a `tempdir`, so the
whole-map `clear()` was the only cross-test reach; narrowing it to one key
removes the coupling rather than serializing around it.
The two config keys the test rests on are now asserted up front:
`CONTEXT_WORKSPACE_SUMMARY` and `CONTEXT_WORKSPACE_SUMMARY_TTL_SECS` resolve
from the environment or `config.yaml`, and the first is the documented lever for
a slow workspace walk — a machine with it off would otherwise have failed with a
message about the file map rather than about its own configuration.
|
Superseded by #218. The flake this fixes was in The "cache serves" half is now covered by Closed without merging; the branch is deleted. |
1 participant
The flake
agents::workspace_summary::tests::test_cache_serves_and_invalidates_on_mtimefailed once on
test (windows-latest)— run34513035963,
3693 passed; 1 failed, panicking atworkspace_summary.rs:457with "newtop-level file surfaces after mtime change". A rerun of the same commit passed
with no code change; macOS and Ubuntu passed both times; the PR carrying it
(#219) does not touch this file. Both defects were in the test, and both are
visible by reading it.
Defect 1 — the promised nudge did not exist
The test wrote
two.txtand immediately re-read, under a comment reading"mtime resolution can be coarse; nudge the directory to be safe" — with no
nudge following it. The Windows clock ticks roughly every 15 ms and NTFS updates
a directory's own mtime lazily, so the two writes could leave
dir_mtimebyte-identical. The cache then judged its entry fresh and served the stale map,
and neither summary named the new file.
Fixed by taking the signal rather than hoping for it:
filetime::set_file_mtimepins the directory's mtime to a fixed instant.
std::fscannot do this portably— a directory needs
FILE_FLAG_BACKUP_SEMANTICSto open for write on Windows,which is exactly what
filetimeencapsulates. It was already inCargo.locktransitively, so this adds a dependency edge and no new package (the lockfile
diff is one line).
A
pin_dir_mtimehelper reads the timestamp back and asserts the filesystemkept it, so a platform that quietly refuses the value fails naming the
timestamp instead of downstream, where a stale map reads as a cache bug. That
is the general shape of the original bug: an unstated precondition failing
somewhere that blames the thing it precedes.
Defect 2 — the assertion could not fail
firstwas captured beforetwo.txtexisted, so the second disjunct was dead.It read as tolerating a race and tolerated nothing.
The test's name promises two behaviours, and it now checks both:
two.txt, restore the mtime to what thecached entry recorded, and assert the stale map comes back unchanged.
the new file surfaces.
What the dead disjunct gestured at is now a live assertion in the right place:
assert!(!first.contains("two.txt")), pinning the pre-state where it isactually true.
Verified by mutation
Since the defect being fixed was an assertion that could not fail, both halves
were checked against a broken implementation:
freshdrops thedir_mtimecomparison (never invalidates)fresh = false(never serves)two.txtpresentIs the cache process-global? Yes — but
#[serial]is not the fixCACHEis astatic Lazy<Mutex<HashMap<..>>>, andtest_collect_moim_uses_minute_granularity(extension_manager.rs:4278) doesreach it from a parallel thread through
collect_moim. But it caches under/tmpwhile this test caches under its owntempdir: keys never collide, sono test can corrupt another's entry.
The entire coupling was the blunt whole-map
clear_cache()— called only bythis test, and wiping the neighbour's entry on every run. So the blunt tool is
removed rather than serialized around:
clear_cache()becomesforget_cached(working_dir), with it andworkspace_summaryderiving the keythrough a shared
cache_key()so the two can never drift.#[serial]would not have helped, for two independent reasons: with keysalready private there is nothing to serialize, and a lone
#[serial]ordersonly against tests that also carry the attribute — which the
collect_moimneighbour does not. Narrowing the reach is what actually holds.
Also
The two config keys the test rests on are asserted up front.
CONTEXT_WORKSPACE_SUMMARYandCONTEXT_WORKSPACE_SUMMARY_TTL_SECSresolvefrom the environment or
config.yaml, and the first is the documented lever fora slow workspace walk — a developer with it off would otherwise have hit a
cryptic
.expect("first summary")rather than a message about their ownconfiguration.
Deliberately not changed
The same coarse-mtime behaviour means production on Windows can serve a stale
map until the 30 s TTL lapses when a file lands in the same clock tick as the
previous walk. That is within the documented contract — the rendered header says
"may be truncated or slightly stale", and the TTL exists precisely as the
backstop for when mtime is an imperfect signal. It is a weaker signal on Windows
than on Unix, not a defect, so it is left alone.
Testing
cargo test -p biorouter --lib -- agents::workspace_summary→ 11 passed, 0 failedcargo clippy -p biorouter --lib --tests --all-features→ cleancargo fmt -p biorouter -- --check→ cleanAll with
BIOROUTER_DISABLE_KEYRING=true.