tests(vcs/cache): changed-window test can't catch a stale fingerprint (served superset re-windows to the same answer)

[dekobon]

Summary

a_changed_window_is_not_served_from_a_stale_fingerprint (tests/vcs_cache.rs) claims to prove that a shorter analysis window makes the cache entry "ignored" because the fingerprint changed, but its only assertion (cached == uncached) cannot distinguish "entry ignored" from "entry served and re-windowed" — so removing long_window_secs from fingerprint() would not fail it.

Location

• tests/vcs_cache.rs:327-345 (a_changed_window_is_not_served_from_a_stale_fingerprint)

Evidence

The test primes with the default 12-month long window, then re-runs with a 7-day long window and asserts the cached result equals a fresh 7-day walk:

build_history_index_cached(repo.path(), &opts(), &cfg).expect("prime");
let mut short = opts();
short.long_window_secs = 7 * DAY;
short.recent_window_secs = 2 * DAY;
let cached = build_history_index_cached(repo.path(), &short, &cfg).expect("short cached");
let uncached = build_history_index(repo.path(), &short).expect("short uncached");
assert_eq!(snapshot(&uncached), snapshot(&cached));

The comment asserts: "A shorter long window changes the fingerprint, so the entry is ignored ...". But the assertion holds even if the entry is served rather than ignored:

1. Pure-hit gate (src/vcs/git/cached.rs:104-106) is entry.is_compatible(fingerprint) && entry.walk_long_boundary <= long_boundary. The 12-month entry has walk_long_boundary = now - 12mo, the 7-day run has long_boundary = now - 7day, and (now - 12mo) <= (now - 7day) is true — so the second gate passes regardless of the fingerprint.
2. If the fingerprint check were broken (e.g. long_window_secs dropped from fingerprint() in src/vcs/cache.rs:228-239), is_compatible would return true and the 12-month entry would be served.
3. On a hit, assemble -> replay::replay re-windows the stored event superset with the current 7-day options: long_boundary = now - options.long_window_secs (src/vcs/replay.rs:180), filtering commit_time < long_boundary (:192). A 12-month superset re-windowed to 7 days yields exactly the fresh 7-day walk.

So the served-and-re-windowed path produces the identical correct answer, and the test passes whether the entry was ignored or wrongly served. The fingerprint's long_window_secs term — the very property the test name and comment claim to guard — is not exercised here.

Expected Behavior

A test named ..._is_not_served_from_a_stale_fingerprint should fail if the entry is served from a stale fingerprint. Concretely, dropping options.long_window_secs.hash(&mut hasher) from fingerprint() should make at least one test red.

Actual Behavior

Removing long_window_secs from fingerprint() leaves this integration test green (the served superset self-corrects via replay re-windowing). The fingerprint term is only caught by the unit test fingerprint_changes_with_walk_affecting_knobs (src/vcs/cache_tests.rs:67); at the integration/build_history_index_cached level the guard is untested.

Impact

A reviewer reading the test name/comment would believe window-driven fingerprint invalidation is exercised end-to-end. It is not. A regression that weakened the fingerprint (or the pure-hit compatibility gate) for the window dimension would ship green through this file. This is a bug-masking test-quality gap, not mere thinness: the assertion structurally cannot observe the behavior its name promises.

Note: distinct from #930 (unit-level is_compatible vcs_schema_version gap) and #810 (shallow-clone reuse). This is about the integration fingerprint/window guard being vacuous due to replay re-windowing.

---

Resolution

Fixed (commit 706ded7e, unpushed). a_changed_window_is_not_served_from_a_stale_fingerprint now empties the primed entry's events array (valid JSON, so it loads as a hit candidate) before the short-window run and changes only long_window_secs, so a wrongly-served stale entry replays zero events and yields an observably wrong empty index. A positive guard asserts the fresh walk ranks a.rs. Verified test-via-revert: removing long_window_secs.hash(...) from fingerprint() now makes the test red; restoring it passes.

[dekobon]

General Plan for Fix

Root Cause

The test relies solely on output equality (cached == uncached). Because the cached event log is a superset (12-month) re-windowed to the current (7-day) options at replay, a wrongly-served entry yields the identical correct result. The pure-hit second gate (walk_long_boundary <= long_boundary) also passes in the superset direction, so the fingerprint is the only thing that should block reuse — yet output equality cannot observe whether it did.

Implementation Steps

1. Give the test a way to distinguish "served" from "ignored". Cheapest reliable signal without new production hooks: after priming with the 12-month window, corrupt or delete the primed entry's events so a served hit would produce a wrong (e.g. empty) result, then assert the short-window run still matches a fresh short walk. If the fingerprint correctly invalidates, the corrupted entry is bypassed and the result is correct; if the fingerprint wrongly matches, replay over the corrupted/empty events diverges and the test fails. (Mirror the existing corrupt_entries helper, but target only the primed entry.)
   • Alternatively/additionally, assert on cache files: after the short-window run, a second entry file should exist under a fresh fingerprint-independent name only if a new walk wrote one — but entries are keyed by HEAD SHA (unchanged here), so file-count cannot discriminate. Prefer the corruption approach.
2. Add the symmetric direction as a separate test if not already covered: prime with a short window, then run with a longer window — here walk_long_boundary <= long_boundary is false, so the entry must be rejected by the boundary gate; assert correctness and that the longer walk is not served stale. This pins the gate that the current test accidentally leans on.
3. Keep the existing cached == uncached assertion as the correctness backstop; the new corruption/boundary assertions add the path-discrimination the name promises.

Tests to Add/Update

• Update a_changed_window_is_not_served_from_a_stale_fingerprint to corrupt the primed entry before the short-window run, so a wrongly-served hit produces an observably wrong result.
• (Optional) Add a_longer_window_is_not_served_from_a_shorter_cached_walk to pin the walk_long_boundary <= long_boundary boundary gate independently of the fingerprint.

Verification

• Revert options.long_window_secs.hash(&mut hasher) in fingerprint() (src/vcs/cache.rs) and confirm the updated test now FAILS (the corrupted superset is served and diverges). Restore.
• cargo test --features vcs-git --test vcs_cache passes with the fix in place.
• cargo clippy -p big-code-analysis --all-features clean.

[dekobon]

Fixed in commit 706ded7e (test(vcs): make changed-window test catch a stale fingerprint), not yet pushed.

Strengthened a_changed_window_is_not_served_from_a_stale_fingerprint in tests/vcs_cache.rs so a stale fingerprint is observable:

1. After priming with the 12-month window, a new empty_entry_events helper rewrites the persisted entry's events array to [] (still valid JSON, so cache::load accepts it as a hit candidate rather than discarding it as corrupt). A served hit now replays zero events and yields an empty index — diverging from a correct recompute, which the previous replay-re-windows self-correction masked.
2. The run now changes only long_window_secs (leaving recent_window_secs at its default), isolating the long_window_secs fingerprint term so dropping options.long_window_secs.hash(...) from fingerprint() makes the fingerprints collide and serves the emptied stale entry.
3. Added a positive guard that the fresh short-window walk ranks a.rs (commits_long > 0), so the emptied served hit is observably wrong rather than coincidentally empty-equals-empty.

Verified test-via-revert: removing options.long_window_secs.hash(&mut hasher) from fingerprint() now makes the test fail (served emptied entry: a.rs.commits_long = 0 vs correct 1); restoring it passes. cargo clippy --all-features clean.