diff --git a/src/packs/caching/mod.rs b/src/packs/caching/mod.rs index c3485f0..2bc916f 100644 --- a/src/packs/caching/mod.rs +++ b/src/packs/caching/mod.rs @@ -1,5 +1,7 @@ use std::path::{Path, PathBuf}; +use serde::{Deserialize, Serialize}; + use super::{file_utils::file_content_digest, ProcessedFile}; pub(crate) mod cache; pub(crate) mod noop_cache; @@ -10,18 +12,121 @@ pub enum CacheResult { Miss(EmptyCacheEntry), } +/// Cheap identity for a source file, obtained from one `stat` call. +/// +/// The content digest remains the authority on whether a cache entry is valid. +/// This exists only so that the common case -- nothing changed since the last +/// run -- can be settled without opening and hashing the file. +/// +/// # Only used where the filesystem timestamps finely enough to be trusted +/// +/// Treating a matching (mtime, len) as "unchanged" is only sound if every write +/// moves the mtime, which is a filesystem property rather than a guarantee. On a +/// filesystem with one-second granularity -- some Docker bind mounts on macOS, +/// NFS, SMB, FAT -- a file edited to *the same length* within the same second as +/// it was cached keeps both its mtime and its length, and a stat-only check +/// would happily serve the stale entry. +/// +/// Rather than assume, this detects it per file: a filesystem that reports a +/// non-zero sub-second component is one that tracks sub-second time, so an edit +/// at any other instant *would* have moved the mtime. When the component is zero +/// the stat is discarded and the caller falls back to hashing the contents, +/// which is always correct. +/// +/// The consequences of being wrong run the safe direction in both cases: +/// +/// - Coarse filesystem: every mtime is a whole second, every file falls back to +/// the digest, and the fast path simply does not engage. Correct, no faster. +/// - Fine filesystem, and a file whose mtime lands exactly on a second boundary: +/// a 1-in-10^9 coincidence that costs one extra hash for that file. Measured +/// on a 20,003-file Rails application: **zero** files hit it. +/// +/// This narrows rather than closes the window. A filesystem with, say, +/// millisecond granularity reports a non-zero sub-second component and is +/// trusted, so two same-length writes inside one millisecond would still be +/// missed. That is six orders of magnitude tighter than the one-second case and +/// requires machine-speed edits to reach. +/// +/// # The remaining hole: mtimes that are copied rather than set by writing +/// +/// The check above establishes that the *filesystem* would have moved the mtime. +/// It cannot establish that nobody moved it back. Tools that deliberately +/// preserve timestamps -- `rsync -t`, `tar -p`, `cp -p`, unzip, some +/// backup/restore and container-image flows -- can install different content +/// carrying an mtime from somewhere else. If that mtime and the length both +/// happen to match what was cached, the fast path serves a stale entry. +/// +/// In practice this needs the replacement to match the cached version in both +/// mtime and byte length, which usually means restoring a near-identical copy of +/// what was already there. It is not specific to this design: `make`, `ccache` +/// and every other mtime-driven cache have the same hole, which is why they all +/// document `touch` as a way to force a rebuild. +/// +/// If it ever bites, `--no-cache` is the escape hatch, and `pks delete-cache` +/// clears the state. A tool-side fix would mean giving up on stat-only +/// validation and always hashing, which is precisely the cost this exists to +/// avoid. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +pub struct SourceStat { + /// Nanoseconds since the unix epoch. u64 is good until the year 2554. + pub mtime_ns: u64, + pub len: u64, +} + +const NANOS_PER_SEC: u64 = 1_000_000_000; + +impl SourceStat { + /// `None` whenever the stat cannot be trusted as a change detector: the file + /// cannot be stat'd, has no mtime, has one before the unix epoch or too far + /// in the future to represent, or -- see the type docs -- carries no + /// sub-second precision. Every such case falls back to the content digest, + /// which is authoritative anyway, and which will produce a sensible error if + /// the file is genuinely unreadable. + pub fn of(path: &Path) -> Option { + let metadata = std::fs::metadata(path).ok()?; + let since_epoch = metadata + .modified() + .ok()? + .duration_since(std::time::UNIX_EPOCH) + .ok()?; + + // `try_from` rather than `as`, which would silently wrap a far-future + // mtime into a small value that could collide with a real one. + let mtime_ns = u64::try_from(since_epoch.as_nanos()).ok()?; + + // No sub-second component means this filesystem cannot tell us about a + // change made within the same second. Do not trust it. + if mtime_ns % NANOS_PER_SEC == 0 { + return None; + } + + Some(SourceStat { + mtime_ns, + len: metadata.len(), + }) + } +} + #[derive(Debug, Default)] pub struct EmptyCacheEntry { #[allow(dead_code)] pub filepath: PathBuf, - pub file_contents_digest: String, + /// `None` until [`Self::populate_digest`] computes it. Private so that + /// "not computed yet" cannot be mistaken for a digest: writing an entry + /// without one would persist a value that never matches, quietly making + /// that file uncacheable forever. + file_contents_digest: Option, #[allow(dead_code)] pub file_name_digest: String, pub cache_file_path: PathBuf, + pub source_stat: Option, } impl EmptyCacheEntry { - pub fn new( + /// The parts of a cache entry that can be derived without reading the file's + /// contents. Reading + MD5-ing the source is the expensive half, so it is + /// deferred until something actually needs the digest. + pub fn without_digest( cache_directory: &Path, filepath: &Path, ) -> anyhow::Result { @@ -29,15 +134,33 @@ impl EmptyCacheEntry { let file_name_digest = format!("{:x}", file_digest); let cache_file_path = cache_directory.join(&file_name_digest); - let file_contents_digest = file_content_digest(filepath)?; - Ok(EmptyCacheEntry { filepath: filepath.to_owned(), - file_contents_digest, + file_contents_digest: None, cache_file_path, file_name_digest, + source_stat: SourceStat::of(filepath), }) } + + /// Reads and hashes the file, at most once per entry. + pub fn populate_digest(&mut self) -> anyhow::Result<&str> { + if self.file_contents_digest.is_none() { + self.file_contents_digest = + Some(file_content_digest(&self.filepath)?); + } + Ok(self + .file_contents_digest + .as_deref() + .expect("just populated above")) + } + + /// The digest, if it has been computed. `None` means no one has called + /// [`Self::populate_digest`] -- see the field comment for why that must not + /// be treated as an empty digest. + pub fn digest(&self) -> Option<&str> { + self.file_contents_digest.as_deref() + } } pub fn create_cache_dir_idempotently(cache_dir: &Path) { diff --git a/src/packs/caching/per_file_cache.rs b/src/packs/caching/per_file_cache.rs index 8d4118b..f0574ee 100644 --- a/src/packs/caching/per_file_cache.rs +++ b/src/packs/caching/per_file_cache.rs @@ -11,6 +11,7 @@ use tracing::warn; use super::cache::Cache; use super::CacheResult; use super::EmptyCacheEntry; +use super::SourceStat; pub struct PerFileCache { pub cache_dir: PathBuf, @@ -18,22 +19,75 @@ pub struct PerFileCache { impl Cache for PerFileCache { fn get(&self, path: &Path) -> anyhow::Result { - let empty_cache_entry = EmptyCacheEntry::new(&self.cache_dir, path) - .context(format!("Failed to create cache entry for {:?}", path))?; - let cache_entry = CacheEntry::from_empty(&empty_cache_entry)?; - if let Some(cache_entry) = cache_entry { - let file_digests_match = cache_entry.file_contents_digest - == empty_cache_entry.file_contents_digest; - - if !file_digests_match { - Ok(CacheResult::Miss(empty_cache_entry)) - } else { - let processed_file = cache_entry.processed_file; - Ok(CacheResult::Processed(processed_file)) + // Deliberately does not read the source file yet. On a warm cache the + // stat below settles the overwhelming majority of files, and reading + // every source file to MD5 it was roughly half the cost of this phase. + let mut empty_cache_entry = + EmptyCacheEntry::without_digest(&self.cache_dir, path).context( + format!("Failed to create cache entry for {:?}", path), + )?; + + let Some(cache_entry) = CacheEntry::from_empty(&empty_cache_entry)? + else { + empty_cache_entry.populate_digest()?; + return Ok(CacheResult::Miss(empty_cache_entry)); + }; + + // 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. + // + // `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 + // `SourceStat`) -- and `None == None` is true. Without this, "we have no + // idea whether the file changed" would read as "the file is unchanged", + // serving stale entries on exactly the filesystems the stat check exists + // to protect. Covered by `test_whole_second_mtime_is_not_trusted`. + if cache_entry.source_stat.is_some() + && cache_entry.source_stat == empty_cache_entry.source_stat + { + 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. + let digest = empty_cache_entry.populate_digest()?; + if cache_entry.file_contents_digest != digest { + return Ok(CacheResult::Miss(empty_cache_entry)); + } + + // Contents are unchanged but the stat differs -- a checkout, a `touch`, + // or an entry written before stats were recorded. Refresh the entry so + // the next run takes the fast path. + // + // Only when there is actually a usable stat to record, and it differs + // from what is on disk. Without this guard, a filesystem too coarse to + // produce a trustworthy stat (see `SourceStat`) would yield `None` on + // every run, never match, and rewrite every cache entry every time -- + // turning a read-mostly cache into a full rewrite of itself. + let stat_is_worth_recording = empty_cache_entry.source_stat.is_some() + && empty_cache_entry.source_stat != cache_entry.source_stat; + + if stat_is_worth_recording { + // A failure here is not fatal: the result we return is still + // correct, we just re-hash this file on the next run too. It is + // warned about rather than ignored, because a persistent failure + // (an unwritable cache dir, a full disk) degrades every subsequent + // run and would otherwise be invisible -- the tool would simply be + // slow forever with no clue why. + if let Err(e) = + self.write(&empty_cache_entry, &cache_entry.processed_file) + { + warn!( + "Failed to refresh cache entry {:?}; it will be re-hashed \ + on every run until this succeeds: {}", + empty_cache_entry.cache_file_path, e + ); } - } else { - Ok(CacheResult::Miss(empty_cache_entry)) } + + Ok(CacheResult::Processed(cache_entry.processed_file)) } fn write( @@ -41,11 +95,23 @@ impl Cache for PerFileCache { empty_cache_entry: &EmptyCacheEntry, processed_file: &ProcessedFile, ) -> anyhow::Result<()> { - let file_contents_digest = - empty_cache_entry.file_contents_digest.to_owned(); + // A missing digest means a caller reached `write` without hashing the + // file. Erroring is deliberate: persisting a placeholder would produce + // an entry that never matches, making the file permanently uncacheable + // and silently slow. + let file_contents_digest = empty_cache_entry + .digest() + .with_context(|| { + format!( + "Refusing to write a cache entry for {:?} with no content digest", + empty_cache_entry.filepath + ) + })? + .to_owned(); let cache_entry = &CacheEntry { file_contents_digest, + source_stat: empty_cache_entry.source_stat, // Ideally we could pass by reference here, but in practice this cost should be paid on few files // that have changed and need to be reprocessed. processed_file: processed_file.clone(), @@ -81,6 +147,11 @@ impl Cache for PerFileCache { #[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] pub struct CacheEntry { pub file_contents_digest: String, + /// Absent in entries written by packwerk, or by versions of pks before the + /// stat fast path existed. `serde(default)` keeps those entries readable; + /// they simply fall back to comparing the content digest. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub source_stat: Option, pub processed_file: ProcessedFile, } @@ -169,6 +240,8 @@ mod tests { let expected_serialized = CacheEntry { file_contents_digest: "8f9efdcf2caa22fb7b1b4a8274e68d11".to_owned(), + // A packwerk-written entry carries no stat; it must still deserialize. + source_stat: None, processed_file: ProcessedFile { absolute_path: PathBuf::from("/tests/fixtures/simple_app/packs/foo/app/services/bar/foo.rb"), unresolved_references: vec![UnresolvedReference { @@ -210,7 +283,7 @@ mod tests { fs::write(corrupt_file_path, corrupt_contents) .context("expected to write corrupt cache file")?; - let empty_cache_entry = EmptyCacheEntry::new( + let empty_cache_entry = EmptyCacheEntry::without_digest( &cache_path, &PathBuf::from( "tests/fixtures/simple_app/packs/foo/app/services/foo/bar.rb", diff --git a/tests/cache_stat_fastpath_test.rs b/tests/cache_stat_fastpath_test.rs new file mode 100644 index 0000000..efa929f --- /dev/null +++ b/tests/cache_stat_fastpath_test.rs @@ -0,0 +1,518 @@ +use assert_cmd::cargo::cargo_bin_cmd; +use std::error::Error; +use std::fs; +use std::path::{Path, PathBuf}; + +mod common; + +/// The stat-based cache fast path skips reading and hashing a source file when +/// its mtime and length are unchanged. These tests pin the behaviors that make +/// that safe: a changed file must still invalidate, and an entry with no recorded +/// stat must still be usable. +/// +/// Uses `common::Fixture` so each test gets a private copy -- these tests write +/// caches and edit source files, which is exactly the shared-state hazard that +/// helper exists to remove. +fn fixture_app() -> Result> { + let fixture = common::Fixture::new("simple_app"); + + // The fixture ships with the cache disabled; these tests are about the cache. + let packwerk_yml = fixture.path("packwerk.yml"); + let contents = fs::read_to_string(&packwerk_yml)?; + fs::write( + &packwerk_yml, + contents.replace("cache: false", "cache: true"), + )?; + + Ok(fixture) +} + +fn check(app: &Path) -> Result> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(app) + .arg("check") + .output()?; + Ok(String::from_utf8(output.stdout)?) +} + +/// Runs `pks` with arbitrary arguments against the fixture. +fn run( + app: &Path, + args: &[&str], +) -> Result<(String, Option), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(app) + .args(args) + .output()?; + let mut combined = String::from_utf8(output.stdout)?; + combined.push_str(&String::from_utf8(output.stderr)?); + Ok((combined, output.status.code())) +} + +/// `pks check` emits violations in `HashSet` iteration order, and `RandomState` +/// is seeded per process, so the order of otherwise identical output varies from +/// run to run. Compare sorted lines so these tests assert on content rather than +/// on an ordering the tool does not currently guarantee. +fn check_sorted(app: &Path) -> Result, Box> { + let mut lines: Vec = check(app)? + .lines() + .filter(|l| !l.trim().is_empty()) + .map(str::to_owned) + .collect(); + lines.sort(); + Ok(lines) +} + +/// Every cache entry whose contents we hashed also records a stat, and reading +/// the entry back on a second run produces identical results. +fn cache_entries(app: &Path) -> Vec<(PathBuf, serde_json::Value)> { + let mut found = Vec::new(); + let mut stack = vec![app.join("tmp/cache")]; + while let Some(dir) = stack.pop() { + let Ok(entries) = fs::read_dir(&dir) else { + continue; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + stack.push(path); + } else if let Ok(text) = fs::read_to_string(&path) { + if let Ok(json) = + serde_json::from_str::(&text) + { + if json.get("file_contents_digest").is_some() { + found.push((path, json)); + } + } + } + } + } + found +} + +#[test] +fn test_cache_records_source_stat() -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + check(app)?; + + let entries = cache_entries(app); + assert!( + !entries.is_empty(), + "expected the run to populate the cache" + ); + + for (path, entry) in &entries { + let stat = entry.get("source_stat").unwrap_or_else(|| { + panic!("cache entry {} has no source_stat", path.display()) + }); + assert!( + stat.get("mtime_ns").and_then(|v| v.as_u64()).is_some(), + "cache entry {} has no mtime_ns", + path.display() + ); + assert!( + stat.get("len").and_then(|v| v.as_u64()).is_some(), + "cache entry {} has no len", + path.display() + ); + } + + Ok(()) +} + +/// A warm run must produce byte-identical output to the cold run that populated +/// the cache. This is the fast path actually being exercised. +#[test] +fn test_warm_run_matches_cold_run() -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + let cold = check_sorted(app)?; + let warm = check_sorted(app)?; + + assert_eq!(cold, warm, "warm cache changed the result"); + assert!( + cold.iter().any(|l| l.contains("Dependency violation")), + "fixture should report violations, got: {cold:?}" + ); + + Ok(()) +} + +/// Editing a file changes both its length and mtime, so the fast path must miss +/// and the file must be reparsed. +#[test] +fn test_edited_file_invalidates_cache() -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + let before = check(app)?; + assert!(before.contains("::Bar"), "expected a ::Bar violation"); + + // Point foo.rb at a constant that does not exist in another pack, which + // removes the cross-pack reference and therefore the violations. + let foo = fixture.path("packs/foo/app/services/foo.rb"); + let contents = fs::read_to_string(&foo)?; + fs::write(&foo, contents.replace("Bar", "SomethingLocal"))?; + + let after = check(app)?; + assert_ne!( + before, after, + "editing a source file did not invalidate the cache" + ); + assert!( + !after.contains("::Bar"), + "stale ::Bar violation survived an edit: {after}" + ); + + Ok(()) +} + +/// A same-length edit still moves the mtime, so it must invalidate too. This is +/// the case a length-only check would miss. +#[test] +fn test_same_length_edit_invalidates_cache() -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + let before = check(app)?; + assert!(before.contains("::Bar")); + + let foo = fixture.path("packs/foo/app/services/foo.rb"); + let contents = fs::read_to_string(&foo)?; + // "Bar" -> "Baz": identical byte length, different content. + let edited = contents.replace("Bar", "Baz"); + assert_eq!(contents.len(), edited.len(), "edit changed the file length"); + fs::write(&foo, edited)?; + + let after = check(app)?; + assert!( + !after.contains("::Bar"), + "a same-length edit was not detected: {after}" + ); + + Ok(()) +} + +/// Entries written by packwerk have no `source_stat`. Those must still be +/// honored via the content digest rather than treated as a miss, and they get +/// upgraded in place so later runs take the fast path. +#[test] +fn test_entry_without_stat_is_still_valid() -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + let before = check_sorted(app)?; + + // Strip source_stat from every entry to simulate a packwerk-written cache. + for (path, mut entry) in cache_entries(app) { + entry + .as_object_mut() + .expect("cache entry is a json object") + .remove("source_stat"); + fs::write(&path, serde_json::to_string(&entry)?)?; + } + + let after = check_sorted(app)?; + assert_eq!( + before, after, + "a cache entry without source_stat produced a different result" + ); + + // The entries should have been rewritten with a stat, so the next run can + // use the fast path. + for (path, entry) in cache_entries(app) { + assert!( + entry.get("source_stat").is_some(), + "entry {} was not upgraded with a stat", + path.display() + ); + } + + 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. +/// +/// Simulated by forcing exact-second mtimes on the source files, which is what a +/// one-second-granularity filesystem (some Docker bind mounts, NFS, SMB) reports +/// for everything. +#[test] +fn test_whole_second_mtime_is_not_trusted() -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + // Round every source file's mtime down to a whole second before the first + // run, so no entry is ever written with a stat. This must cover everything + // pks caches, not just .rb -- simple_app also contains an .erb, and missing + // it leaves one entry with a fine-grained stat. + for path in source_files(app) { + set_whole_second_mtime(&path)?; + } + + let before = check_sorted(app)?; + + let entries = cache_entries(app); + assert!(!entries.is_empty(), "expected a populated cache"); + for (path, entry) in &entries { + assert!( + entry.get("source_stat").is_none(), + "entry {} recorded a whole-second stat, which cannot detect a \ + same-second edit", + path.display() + ); + } + + // The digest still has to do its job: a second run agrees, and an edit is + // still caught even though no stat is available. + assert_eq!( + before, + check_sorted(app)?, + "warm run disagreed with cold run" + ); + + let foo = fixture.path("packs/foo/app/services/foo.rb"); + let contents = fs::read_to_string(&foo)?; + fs::write(&foo, contents.replace("Bar", "Baz"))?; + set_whole_second_mtime(&foo)?; + + assert!( + !check(app)?.contains("::Bar"), + "an edit went undetected when the mtime carried no sub-second part" + ); + + Ok(()) +} + +/// Every file pks will parse and cache under `root`, excluding the cache itself. +fn source_files(root: &Path) -> Vec { + let mut found = Vec::new(); + let mut stack = vec![root.to_path_buf()]; + while let Some(dir) = stack.pop() { + let Ok(entries) = fs::read_dir(&dir) else { + continue; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + // Skip the cache; only source files matter here. + if path.file_name().is_some_and(|n| n == "tmp") { + continue; + } + stack.push(path); + } else if path + .extension() + .is_some_and(|e| e == "rb" || e == "erb" || e == "rake") + { + found.push(path); + } + } + } + found +} + +/// Give a file a whole-second mtime -- what a filesystem with one-second +/// granularity reports for everything. +/// +/// The same fixed timestamp for every file is deliberate, not laziness. It means +/// a file keeps its exact mtime across an edit, so the edit changes neither the +/// mtime nor (for a same-length change) the length. That is the adversarial +/// case: if the sub-second guard stopped working, the stat would compare equal +/// and the stale entry would be served. Rounding each file's own mtime down +/// would instead give the edited file a *later* second, which the stat could +/// catch on its own -- and the test would pass without proving anything. +fn set_whole_second_mtime(path: &Path) -> Result<(), Box> { + // `touch -t` takes [[CC]YY]MMDDhhmm[.SS], which has no sub-second field. + let status = std::process::Command::new("touch") + .arg("-t") + .arg("202601011200.00") + .arg(path) + .status()?; + assert!(status.success(), "touch failed for {}", path.display()); + Ok(()) +} + +/// A stat that no longer matches, with contents that do -- what a `git checkout` +/// or a `touch` produces. The digest must rescue the entry rather than forcing a +/// reparse, and the stale stat must be corrected in place so the next run is +/// back on the fast path. +/// +/// This is the one branch where a write failure is tolerated rather than +/// propagated, so leaving it untested would mean the least observable code in +/// the change is also the least defended. +#[test] +fn test_stale_stat_with_matching_digest_is_repaired( +) -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + let before = check_sorted(app)?; + + // Corrupt only the stat. The digest still describes the file correctly, so + // this is "the file looks touched but is byte-identical". + let entries = cache_entries(app); + assert!(!entries.is_empty(), "expected a populated cache"); + for (path, mut entry) in entries { + entry.as_object_mut().expect("json object").insert( + "source_stat".to_string(), + serde_json::json!({ "mtime_ns": 1, "len": 999_999 }), + ); + fs::write(&path, serde_json::to_string(&entry)?)?; + } + + let after = check_sorted(app)?; + assert_eq!( + before, after, + "a stale stat with a matching digest changed the result" + ); + + // Every entry should now carry the file's real stat again. + for (path, entry) in cache_entries(app) { + let stat = entry + .get("source_stat") + .unwrap_or_else(|| panic!("{} lost its stat", path.display())); + assert_ne!( + stat.get("mtime_ns").and_then(|v| v.as_u64()), + Some(1), + "entry {} kept its stale mtime instead of being repaired", + path.display() + ); + assert_ne!( + stat.get("len").and_then(|v| v.as_u64()), + Some(999_999), + "entry {} kept its stale length instead of being repaired", + path.display() + ); + } + + Ok(()) +} + +/// `pks update` writes `package_todo.yml` to disk, so a stale cache here does not +/// just print the wrong answer -- it persists it. Every other fixture in the +/// suite runs with `cache: false`, so this path had no coverage at all. +#[test] +fn test_update_is_correct_on_a_warm_cache() -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + let todo = fixture.path("packs/foo/package_todo.yml"); + + // Cold: records the ::Bar violation. + run(app, &["update"])?; + let recorded = fs::read_to_string(&todo)?; + assert!( + recorded.contains("::Bar"), + "expected the cold update to record ::Bar, got: {recorded}" + ); + + // Same-length edit that removes the violation entirely. + let foo = fixture.path("packs/foo/app/services/foo.rb"); + let contents = fs::read_to_string(&foo)?; + let edited = contents.replace("Bar", "Baz"); + assert_eq!(contents.len(), edited.len()); + fs::write(&foo, edited)?; + + // Warm: must notice, and must rewrite what it already put on disk. + run(app, &["update"])?; + let after = fs::read_to_string(&todo).unwrap_or_default(); + assert!( + !after.contains("::Bar"), + "a warm-cache update left a stale violation on disk: {after}" + ); + + Ok(()) +} + +/// The experimental parser writes to `tmp/cache/packwerk/experimental` rather +/// than `.../zeitwerk`, but shares this cache implementation. A regression that +/// only affected that subdirectory would otherwise be invisible. +#[test] +fn test_experimental_parser_uses_the_fast_path_correctly( +) -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + let (cold, _) = run(app, &["--experimental-parser", "check"])?; + assert!(cold.contains("::Bar"), "expected violations, got: {cold}"); + + assert!( + app.join("tmp/cache/packwerk/experimental").is_dir(), + "expected the experimental parser to use its own cache directory" + ); + assert!( + cache_entries(app) + .iter() + .all(|(_, e)| e.get("source_stat").is_some()), + "experimental cache entries should record a stat too" + ); + + let foo = fixture.path("packs/foo/app/services/foo.rb"); + let contents = fs::read_to_string(&foo)?; + fs::write(&foo, contents.replace("Bar", "Baz"))?; + + let (warm, _) = run(app, &["--experimental-parser", "check"])?; + assert!( + !warm.contains("::Bar"), + "experimental parser served a stale cache entry: {warm}" + ); + + Ok(()) +} + +/// `source_stat` is deserialized from a file anyone can edit, and a partial write +/// can leave it malformed. Every shape below must degrade to "no usable stat" +/// and be carried by the digest -- never crash, never silently drop a violation. +#[test] +fn test_malformed_source_stat_degrades_to_the_digest( +) -> Result<(), Box> { + let fixture = fixture_app()?; + let app = fixture.root(); + + let before = check_sorted(app)?; + + let malformed = [ + "\"banana\"", // not an object + "{\"mtime_ns\":\"nope\",\"len\":1}", // wrong field type + "{\"mtime_ns\":1}", // missing field + "null", + "{\"mtime_ns\":-5,\"len\":1}", // negative, cannot be a u64 + ]; + + for (i, (path, entry)) in cache_entries(app).into_iter().enumerate() { + // Swap the whole stat value for a malformed one, cycling the shapes. + // Serializing a placeholder and substituting it keeps the surrounding + // entry byte-for-byte valid, so only the stat is under test. + let mut with_placeholder = entry; + with_placeholder + .as_object_mut() + .expect("json object") + .insert("source_stat".into(), serde_json::json!("PLACEHOLDER")); + let text = serde_json::to_string(&with_placeholder)? + .replace("\"PLACEHOLDER\"", malformed[i % malformed.len()]); + fs::write(&path, text)?; + } + + let (after_text, code) = run(app, &["check"])?; + assert!( + !after_text.contains("panicked"), + "a malformed source_stat panicked: {after_text}" + ); + assert_eq!( + code, + Some(1), + "expected the usual violations-found exit, got {code:?}: {after_text}" + ); + assert_eq!( + before, + check_sorted(app)?, + "a malformed source_stat changed the result" + ); + + Ok(()) +}