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..4621c8e3 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, @@ -30,7 +31,13 @@ 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 + // 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 -- @@ -41,13 +48,17 @@ 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)); } // 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 @@ -304,26 +315,34 @@ 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. + 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 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..2e069933 100644 --- a/tests/cache_stat_fastpath_test.rs +++ b/tests/cache_stat_fastpath_test.rs @@ -367,6 +367,49 @@ 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.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"] = + 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.