Check the pks version before taking the cache's stat fast path

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<EmptyCacheEntry> {
-        // 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 {

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(())

tests/cache_stat_fastpath_test.rs

@@ -367,6 +367,49 @@ fn test_entry_without_stat_is_still_valid() -> Result<(), Box<dyn Error>> {
     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<dyn Error>> {
+    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.