From 1651285db0d7d67e7ac4258de2f4d34d21c32146 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 1 Oct 2026 18:23:30 -0700 Subject: [PATCH 1/2] Check the pks version before taking the cache's stat fast path c2fe84f (#66) added the pks version to each cache entry's content digest so that an upgrade would invalidate every entry. But the stat fast path from 96f7fb0 (#59) returns an entry as soon as its file's mtime and length match, without comparing the digest. So for any file that hadn't changed, an entry written by another version of pks was still served, and the CHANGELOG's "Upgrading pks invalidates cached results" didn't hold for any entry that recorded a stat. The fast path now also requires the entry's digest to end with this version's suffix. An entry from another version falls through to the digest comparison, misses, and is rewritten. The suffix is one constant, used both to build the digest and to check it, so the two can't drift apart. Neither change has shipped. v0.5.0 predates both, and its entries have no stat, so they already miss on the digest. The bug affects builds of main since #59, and would have affected every upgrade starting from the next release. The CHANGELOG entry already describes the fixed behavior, so it's unchanged. test_entry_from_another_pks_version_is_a_miss now also covers an entry carrying the file's real stat. Its comment said a real stat would take the fast path instead, which was the bug. A new integration test rewrites every entry as if another version had written it, with its references removed and its stat still matching, and checks that the violations are still reported. Both tests fail without the fix. --- src/packs/caching/mod.rs | 11 ++++--- src/packs/caching/per_file_cache.rs | 50 ++++++++++++++++++----------- tests/cache_stat_fastpath_test.rs | 44 +++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 23 deletions(-) diff --git a/src/packs/caching/mod.rs b/src/packs/caching/mod.rs index 5e3803b1..30456e86 100644 --- a/src/packs/caching/mod.rs +++ b/src/packs/caching/mod.rs @@ -113,6 +113,11 @@ impl SourceStat { } } +/// Appended to every content digest so a parsing-behavior upgrade invalidates +/// every entry, not just the files that actually changed. The stat fast path +/// checks for it too, since it never compares the digest itself. +pub const DIGEST_VERSION_SUFFIX: &str = concat!("-", env!("CARGO_PKG_VERSION")); + /// Everything about a source file that can be known without reading it: where /// its cache entry lives, and the stat used to check that entry against the file. /// @@ -150,13 +155,11 @@ impl CacheLookup { /// only because its `write` ignores the argument entirely and never persists /// anything. pub fn read_contents(self) -> anyhow::Result { - // Suffixed with the pks version so a parsing-behavior upgrade - // invalidates every entry, not just the files that actually changed. let file_contents_digest = format!( - "{}-{}", + "{}{}", file_content_digest(&self.filepath) .context("Failed to create cache entry")?, - env!("CARGO_PKG_VERSION") + DIGEST_VERSION_SUFFIX ); Ok(EmptyCacheEntry { diff --git a/src/packs/caching/per_file_cache.rs b/src/packs/caching/per_file_cache.rs index afb5a1bf..bc36239d 100644 --- a/src/packs/caching/per_file_cache.rs +++ b/src/packs/caching/per_file_cache.rs @@ -13,6 +13,7 @@ use super::CacheLookup; use super::CacheResult; use super::EmptyCacheEntry; use super::SourceStat; +use super::DIGEST_VERSION_SUFFIX; pub struct PerFileCache { pub cache_dir: PathBuf, @@ -32,6 +33,11 @@ impl Cache for PerFileCache { // Fast path: the file has the same mtime and length as when we cached // it, so it cannot have changed in any way we care about. // + // The version check stands in for the digest comparison this path + // skips. Without it, an entry written by another version of pks would + // be served whenever its file is untouched, and an upgrade that changes + // parsing would keep the old results. + // // `is_some()` is not redundant with the equality check and must not be // folded into it. Both sides are `None` whenever no usable stat exists -- // on a filesystem too coarse to be trusted, every file every run (see @@ -41,6 +47,9 @@ impl Cache for PerFileCache { // to protect. Covered by `test_whole_second_mtime_is_not_trusted`. if cache_entry.source_stat.is_some() && cache_entry.source_stat == lookup.source_stat + && cache_entry + .file_contents_digest + .ends_with(DIGEST_VERSION_SUFFIX) { return Ok(CacheResult::Processed(cache_entry.processed_file)); } @@ -304,26 +313,29 @@ mod tests { assert!(matches!(cache.get(&path)?, CacheResult::Processed(_))); // The bare content digest is what entries held before they recorded - // the pks version. + // the pks version. No stat forces the digest comparison, and the + // file's own stat takes the fast path; both must miss. let content_digest = file_content_digest(&path)?; - for file_contents_digest in - [content_digest.clone(), format!("{content_digest}-0.0.0")] - { - let entry = CacheEntry { - file_contents_digest, - // None forces the digest comparison; a real stat would hit the fast path instead. - source_stat: None, - processed_file: processed_file.clone(), - }; - fs::write( - &empty_cache_entry.cache_file_path, - serde_json::to_string(&entry)?, - )?; - assert!( - matches!(cache.get(&path)?, CacheResult::Miss(_)), - "served an entry with digest {}", - entry.file_contents_digest - ); + for source_stat in [None, empty_cache_entry.source_stat] { + for file_contents_digest in + [content_digest.clone(), format!("{content_digest}-0.0.0")] + { + let entry = CacheEntry { + file_contents_digest, + source_stat, + processed_file: processed_file.clone(), + }; + fs::write( + &empty_cache_entry.cache_file_path, + serde_json::to_string(&entry)?, + )?; + assert!( + matches!(cache.get(&path)?, CacheResult::Miss(_)), + "served an entry with digest {} and stat {:?}", + entry.file_contents_digest, + entry.source_stat + ); + } } Ok(()) diff --git a/tests/cache_stat_fastpath_test.rs b/tests/cache_stat_fastpath_test.rs index 95adc33e..69749bc1 100644 --- a/tests/cache_stat_fastpath_test.rs +++ b/tests/cache_stat_fastpath_test.rs @@ -367,6 +367,50 @@ fn test_entry_without_stat_is_still_valid() -> Result<(), Box> { Ok(()) } +/// After an upgrade that changes parsing, files that haven't changed must not +/// keep their old results. Simulated by rewriting every entry as if another +/// version of pks had written it, with its references removed, while keeping +/// the stat that still matches the file. If the fast path trusted the stat +/// alone, the violations would disappear. +#[test] +fn test_entry_from_another_pks_version_is_not_served_on_the_fast_path( +) -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + let before = check_sorted(app)?; + assert!(before.iter().any(|l| l.contains("::Bar"))); + + let entries = cache_entries(app); + assert!( + entries.iter().any(|(_, e)| e.get("source_stat").is_some()), + "expected at least one entry to record a stat, or this test cannot \ + reach the fast path" + ); + + for (path, mut entry) in entries { + let digest = entry["file_contents_digest"] + .as_str() + .expect("digest is a string"); + let (content_digest, _) = digest + .rsplit_once('-') + .expect("digest has a version suffix"); + entry["file_contents_digest"] = + serde_json::json!(format!("{content_digest}-0.0.0")); + entry["processed_file"]["unresolved_references"] = + serde_json::json!([]); + fs::write(&path, serde_json::to_string(&entry)?)?; + } + + assert_eq!( + before, + check_sorted(app)?, + "served a cache entry written by another version of pks" + ); + + Ok(()) +} + /// A filesystem that reports whole-second mtimes cannot tell us about an edit /// made within the same second, so the stat must not be recorded at all and the /// digest must carry the entry instead. From ec50be24fe962913e619fa27734077e15e12b8cb Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 2 Oct 2026 09:08:39 -0700 Subject: [PATCH 2/2] Tighten comments and tests around the fast path's version check The fast-path comment said a matching stat meant the file "cannot have changed in any way we care about", which the version paragraph right below it contradicts: the file is unchanged, but its entry can still be stale. The slow-path comment now lists an entry from another version among the reasons for getting there. The unit test now asserts that the file has a usable stat. On a filesystem with whole-second mtimes it has none, and the loop's second pass would quietly repeat the first instead of reaching the fast path. The integration test strips the version with split_once, since an md5 hex digest has no hyphens and a prerelease version can. --- src/packs/caching/per_file_cache.rs | 13 ++++++++++--- tests/cache_stat_fastpath_test.rs | 5 ++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/packs/caching/per_file_cache.rs b/src/packs/caching/per_file_cache.rs index bc36239d..4621c8e3 100644 --- a/src/packs/caching/per_file_cache.rs +++ b/src/packs/caching/per_file_cache.rs @@ -31,7 +31,8 @@ impl Cache for PerFileCache { }; // Fast path: the file has the same mtime and length as when we cached - // it, so it cannot have changed in any way we care about. + // it, so the file itself has not changed. That alone does not make the + // entry current. // // The version check stands in for the digest comparison this path // skips. Without it, an entry written by another version of pks would @@ -55,8 +56,9 @@ impl Cache for PerFileCache { } // Slow path: no stat recorded (entry predates this feature, or was - // written by packwerk), or the stat moved. The content digest is still - // the authority, so fall back to it. + // written by packwerk), the stat moved, or the entry was written by + // another version of pks. The content digest is still the authority, + // so fall back to it. let empty_cache_entry = lookup.read_contents()?; if cache_entry.file_contents_digest != empty_cache_entry.file_contents_digest @@ -315,6 +317,11 @@ mod tests { // The bare content digest is what entries held before they recorded // the pks version. No stat forces the digest comparison, and the // file's own stat takes the fast path; both must miss. + assert!( + empty_cache_entry.source_stat.is_some(), + "expected the file to have a usable stat, or this test cannot \ + reach the fast path" + ); let content_digest = file_content_digest(&path)?; for source_stat in [None, empty_cache_entry.source_stat] { for file_contents_digest in diff --git a/tests/cache_stat_fastpath_test.rs b/tests/cache_stat_fastpath_test.rs index 69749bc1..2e069933 100644 --- a/tests/cache_stat_fastpath_test.rs +++ b/tests/cache_stat_fastpath_test.rs @@ -392,9 +392,8 @@ fn test_entry_from_another_pks_version_is_not_served_on_the_fast_path( let digest = entry["file_contents_digest"] .as_str() .expect("digest is a string"); - let (content_digest, _) = digest - .rsplit_once('-') - .expect("digest has a version suffix"); + let (content_digest, _) = + digest.split_once('-').expect("digest has a version suffix"); entry["file_contents_digest"] = serde_json::json!(format!("{content_digest}-0.0.0")); entry["processed_file"]["unresolved_references"] =