diff --git a/changelog.d/8091-build-cache-compiler-identity.md b/changelog.d/8091-build-cache-compiler-identity.md new file mode 100644 index 0000000000..54c79730fd --- /dev/null +++ b/changelog.d/8091-build-cache-compiler-identity.md @@ -0,0 +1,35 @@ +### Fixed + +- **The build cache no longer hands back a binary built by a different + compiler.** The `perry_build_id` check re-fingerprinted the path *recorded in + the manifest* and compared it to the recorded value — which asks "is the + binary I recorded still unchanged?", and is trivially true whenever a + different `perry` runs the second build. The recorded binary is sitting + exactly where it was, so the check passed, the cache reported + `"hit": true, "reason": "manifest-match"`, and the build was skipped + entirely: no relink, output file untouched, nothing printed, exit 0. + + It now compares against the compiler running now, via the same + `current_perry_fingerprint()` used when the manifest is written. + + `perry_version` did not cover this. During pass development the version + rarely moves between rebuilds, which is the reason `perry_build_id` exists + at all (#544) — this restores the guarantee that issue was closed on. + + How to recognise it: a `.ts` probe compiled by a pre-fix compiler kept its + stale executable when recompiled by a fixed one, so a genuine fix read as not + working and the phantom was bisected onto an unrelated commit. Touching the + source does not help, because sources are verified by sha256 rather than + mtime; only a different output path or a cleared cache does. + + Changed: `crates/perry/src/commands/compile/build_cache.rs` — the + `perry-build-id` arm of `BuildCacheProbe::probe`. + + Validation: `cargo test -p perry --bins build_cache` (4 passed). Two tests + cover it — one pins the two comparison expressions in isolation, and + `a_foreign_build_id_misses_at_the_probe` writes a manifest claiming a + different compiler's build id and drives the real decision path, asserting + the miss is `perry-build-id` specifically rather than an incidental later + check. Sabotage-verified: restoring the old self-comparison at the call site + turns the probe test red while the expression test stays green, so the + guarding test is the one that actually holds the fix in place. diff --git a/crates/perry/src/commands/compile/build_cache.rs b/crates/perry/src/commands/compile/build_cache.rs index a8b0fdf2e8..057e38d4d1 100644 --- a/crates/perry/src/commands/compile/build_cache.rs +++ b/crates/perry/src/commands/compile/build_cache.rs @@ -134,7 +134,119 @@ const BUILD_CACHE_ENV_EXCLUSIONS: &[&str] = &[ #[cfg(test)] mod tests { - use super::{BUILD_CACHE_ENV_EXCLUSIONS, BUILD_CACHE_ENV_VARS}; + use super::{ + absolute_identity, current_env, current_perry_fingerprint, file_fingerprint, + file_fingerprint_from_str, BuildCacheManifest, BuildCacheProbe, BUILD_CACHE_ENV_EXCLUSIONS, + BUILD_CACHE_ENV_VARS, BUILD_CACHE_MANIFEST_VERSION, + }; + + /// The build cache must compare against the compiler RUNNING NOW, not the + /// one that wrote the manifest. + /// + /// The bug this pins: the check used to re-fingerprint the path recorded in + /// the manifest and compare it to the recorded value. That asks "is the + /// binary I recorded still unchanged?", which is trivially true when a + /// DIFFERENT perry runs the second build — the recorded binary is sitting + /// right where it was. The cache then reported `manifest-match`, skipped + /// the build, and handed back the first compiler's executable while + /// printing nothing and exiting 0. + /// + /// `perry_version` does not cover this: during pass development the + /// version rarely moves between rebuilds, which is why `perry_build_id` + /// exists at all (#544). + #[test] + fn a_manifest_from_another_compiler_does_not_match_this_one() { + // Stand in for "the compiler that wrote the manifest": any other file + // that exists and is not this executable. Its own fingerprint is + // self-consistent, which is exactly what made the old check pass. + let other = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("Cargo.toml"); + let recorded = file_fingerprint(&other).expect("fingerprint the stand-in"); + assert_eq!( + file_fingerprint_from_str(&recorded.path).ok(), + Some(recorded.clone()), + "precondition: the recorded binary is unchanged on disk, so the OLD \ + check would have passed here — without this the test proves nothing" + ); + + let running = current_perry_fingerprint().expect("fingerprint the test binary"); + assert_ne!( + running, recorded, + "a manifest written by a different compiler must not match" + ); + } + + /// The expression test above pins the two comparisons in isolation, but it + /// never calls `probe()` — reverting the production call site to the buggy + /// form leaves it green. This one drives the real decision path, so it is + /// the one that actually guards the fix. + /// + /// Verified by sabotage: restoring + /// `file_fingerprint_from_str(&manifest.perry_build_id.path)` at the call + /// site turns this red while the expression test stays green. + #[test] + fn a_foreign_build_id_misses_at_the_probe() { + let dir = tempfile::tempdir().expect("tempdir"); + let input = dir.path().join("in.ts"); + let output = dir.path().join("out.bin"); + let manifest_path = dir.path().join("manifest.json"); + std::fs::write(&input, b"export {}\n").expect("write input"); + std::fs::write(&output, b"binary").expect("write output"); + + // A build id belonging to some other compiler: any real file that is + // not this executable. It is unchanged on disk, which is precisely the + // condition under which the old self-comparison passed. + let foreign = file_fingerprint( + &std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("Cargo.toml"), + ) + .expect("fingerprint the foreign build id"); + assert_ne!( + foreign, + current_perry_fingerprint().expect("fingerprint the running binary"), + "precondition: the manifest must claim a DIFFERENT compiler" + ); + + let manifest = BuildCacheManifest { + version: BUILD_CACHE_MANIFEST_VERSION, + perry_version: env!("CARGO_PKG_VERSION").to_string(), + perry_build_id: foreign, + args_key: "args".to_string(), + env: current_env(), + input_path: absolute_identity(&input), + output_path: absolute_identity(&output), + target: "native".to_string(), + compiled_features: Vec::new(), + sources: Vec::new(), + config_inputs: Vec::new(), + runtime_inputs: Vec::new(), + object_fingerprints: Vec::new(), + native_modules: 0, + js_modules: 0, + output: file_fingerprint(&output).expect("fingerprint output"), + }; + std::fs::write( + &manifest_path, + serde_json::to_string(&manifest).expect("serialize manifest"), + ) + .expect("write manifest"); + + let probe = BuildCacheProbe { + args_key: "args".to_string(), + manifest_path, + output_path: output, + target_name: "native".to_string(), + input_path: input, + project_root: dir.path().to_path_buf(), + cache_root: dir.path().to_path_buf(), + eligible: Ok(()), + }; + + let stats = probe.probe(); + assert!(!stats.hit, "a manifest from another compiler must not hit"); + assert_eq!( + stats.reason, "perry-build-id", + "must miss on the build id specifically, not incidentally on a later check" + ); + } #[test] fn binding_policy_switches_are_build_cache_inputs() { @@ -337,9 +449,23 @@ impl BuildCacheProbe { if manifest.output_path != absolute_identity(&self.output_path) { return miss("output-path"); } - if file_fingerprint_from_str(&manifest.perry_build_id.path).ok() - != Some(manifest.perry_build_id.clone()) - { + // Compare against the compiler RUNNING NOW, not the one the manifest + // was written by. Re-fingerprinting `manifest.perry_build_id.path` + // asks "is the binary I recorded still unchanged?", which is trivially + // true whenever a DIFFERENT perry does the second build — its path is + // not the recorded one, so the recorded binary sits there untouched + // and the check passes. The cache then hands back the first compiler's + // executable and skips the build entirely, reporting + // `"hit": true, "reason": "manifest-match"` and printing nothing. + // + // That is not hypothetical: it cost a full false-regression hunt. A + // probe compiled by a pre-fix perry kept its stale output when + // recompiled by a fixed one, the fix read as not working, and the + // phantom bisected onto an unrelated commit. `perry_version` above + // does not cover it either — during pass development the version + // rarely moves between rebuilds, which is the whole reason + // `perry_build_id` exists (#544). + if current_perry_fingerprint().ok() != Some(manifest.perry_build_id.clone()) { return miss("perry-build-id"); } if verify_files(&manifest.sources).is_err() {