diff --git a/crates/batten/src/perf.rs b/crates/batten/src/perf.rs index a8012ca45..eb92c146f 100644 --- a/crates/batten/src/perf.rs +++ b/crates/batten/src/perf.rs @@ -613,6 +613,20 @@ fn take_arm(shared: &Path, dest: &Path, what: &str) -> Result<()> { Ok(()) } +/// Whether identical arms mean a build handed back the wrong binary. +/// +/// ONLY WHEN THE BASE WAS BUILT IN THIS RUN (CLOUD-2107). CLOUD-2060's mechanism +/// is two builds into the shared directory in one run, the second inheriting the +/// first's `batten` units through cargo's mtime fingerprint. A base answered from +/// the cache — CLOUD-2097's kept head, or an earlier lap's build for the same key +/// — lent nothing to this run's head build, and a head that is byte-identical to +/// it is a change that did not touch the binary: measured on CLOUD-2106's +/// test-only lap, which this guard refused before any measurement. +//MUTANT identical-arms-always-suspect|s@^ !null \&\& base_built_now \&\& identical$@ !null \&\& identical@|identical_arms_are_suspect_only_when_the_base_was_built_this_run +fn arms_suspect(null: bool, base_built_now: bool, identical: bool) -> bool { + !null && base_built_now && identical +} + /// Whether two files hold the same bytes; an unreadable side is never "same". fn same_bytes(left: &Path, right: &Path) -> bool { match (std::fs::read(left), std::fs::read(right)) { @@ -1120,6 +1134,7 @@ fn measure(repo: &Path, options: Options, base_sha: &str) -> Result> let out = out_dir(repo)?; let shared = arms_target_dir(&perf_dir(repo)); + let mut base_built_now = false; let (base_bin, base_tree) = if options.null { // The null experiment: the same bytes as both arms. COPIED rather than // aliased so hyperfine sees two distinct commands and cannot @@ -1155,6 +1170,7 @@ fn measure(repo: &Path, options: Options, base_sha: &str) -> Result> seed_base_target_dir(&perf, &key)?; } if !base_arm_is_built(&perf, &key) { + base_built_now = true; build(&base_tree, Some(&shared), "base", PAIR_PROFILE)?; // A cargo that exits 0 without leaving the binary is could-not-look, // never a measurement: hyperfine would report the missing path as a @@ -1189,11 +1205,13 @@ fn measure(repo: &Path, options: Options, base_sha: &str) -> Result> } } // THE GUARD THE MEASURED DEFECT LACKED (CLOUD-2060): a pair whose arms are - // the same bytes measures nothing, and its ratio of ~1 reads as a pass. A lap - // reaches here only when the crate changed, so identical arms mean a build - // handed back the wrong binary. The null experiment copies one arm on - // purpose, after this. - if !options.null && same_bytes(&base_bin, &head_bin) { + // the same bytes measures nothing, and its ratio of ~1 reads as a pass. The + // null experiment copies one arm on purpose, after this. + if arms_suspect( + options.null, + base_built_now, + same_bytes(&base_bin, &head_bin), + ) { bail!( "perf-pair: the head arm is byte-identical to the base arm, so a build handed back the wrong binary. No measurement." ); @@ -3362,6 +3380,22 @@ mod tests { assert!(!switches_arm(Some("head"), "head")); } + /// CLOUD-2107: identical arms are a broken build only when this run built the + /// base into the shared directory; a cached base cannot have lent its units. + #[test] + fn identical_arms_are_suspect_only_when_the_base_was_built_this_run() { + assert!(arms_suspect(false, true, true), "CLOUD-2060's own case"); + assert!( + !arms_suspect(false, false, true), + "a cached base equal to the head is an unchanged binary" + ); + assert!(!arms_suspect(false, true, false)); + assert!( + !arms_suspect(true, true, true), + "the null experiment is exempt" + ); + } + /// CLOUD-2060: the guard that reads two identical arms as a broken build. #[test] fn identical_arms_are_the_same_bytes() -> std::io::Result<()> { diff --git a/crates/batten/tests/it/bypass_scrub.rs b/crates/batten/tests/it/bypass_scrub.rs index dd424ca74..d42ba1e68 100644 --- a/crates/batten/tests/it/bypass_scrub.rs +++ b/crates/batten/tests/it/bypass_scrub.rs @@ -87,6 +87,44 @@ fn every_row_declared_hatch_is_scrubbed() { } } +/// THE CHEAP READ IS HELD TO THE FULL LOAD (CLOUD-2106). +/// +/// `common` reads the hatches through a lenient TOML view rather than the +/// validated load, because the load was 96% of a spawning case's own CPU. A cheap +/// read that missed a row would scrub less, silently, so both readers are compared +/// on a config that DOES declare a hatch — the committed one declares none today, +/// which would make the comparison vacuous — and on the committed file itself. +#[test] +fn the_cheap_bypass_read_names_what_the_full_load_names() { + let root = common::scratch_repo("bypass-cheap-read"); + write( + &root, + "batten.toml", + "version = 1\n\n[[rule]]\nid = \"no-touching\"\nkind = \"shape\"\n\ + scope = \"mediated_call\"\nseverity = \"deny\"\npattern = \"touch guarded.txt\"\n\ + reason = \"the fixture declares a hatch\"\nbypass_env = \"BATTEN_FIXTURE_HATCH\"\n", + ); + let full = |path: &std::path::Path| -> Vec { + let mut names: Vec = batten::config::load(path) + .expect("the config loads") + .rules + .iter() + .filter_map(|rule| rule.bypass_env.clone()) + .collect(); + names.sort(); + names.dedup(); + names + }; + let fixture = root.join("batten.toml"); + assert_eq!( + common::bypass_env_vars_in(&fixture), + vec![String::from("BATTEN_FIXTURE_HATCH")] + ); + assert_eq!(common::bypass_env_vars_in(&fixture), full(&fixture)); + let committed = at_root("batten.toml"); + assert_eq!(common::bypass_env_vars_in(&committed), full(&committed)); +} + /// THE REMOVED GLOBAL HATCH OPENS NOTHING, asserted on the compiled binary. /// /// `BATTEN_HOOK_BYPASS` used to turn this refusal into an allow; the engine no diff --git a/crates/batten/tests/it/common/mod.rs b/crates/batten/tests/it/common/mod.rs index 6d919d2fa..c3147f2cc 100644 --- a/crates/batten/tests/it/common/mod.rs +++ b/crates/batten/tests/it/common/mod.rs @@ -325,23 +325,49 @@ fn bypass_env_vars() -> Vec { // list of hatch names. CLOUD-1227 is explicit about why: a list "stops // covering the next row somebody adds, silently, in the direction that // weakens the suite". The signature is unchanged so no caller has to know. - static NAMES: std::sync::LazyLock> = std::sync::LazyLock::new(|| { - let mut names = Vec::new(); - if let Ok(config) = batten::config::load(&at_root("batten.toml")) { - names.extend( - config - .rules - .iter() - .filter_map(|rule| rule.bypass_env.clone()), - ); - } - names.sort(); - names.dedup(); - names - }); + // + // AND READ CHEAPLY, BECAUSE THE MEMO IS PER PROCESS (CLOUD-2106). nextest + // runs one process per case, so the `LazyLock` saves nothing across the suite + // and every spawning case paid the whole validated load at the test binary's + // opt-level 0: 242M of `cli::help_leads_with_the_crate_description`'s 251M + // instructions. The names are still read out of the committed file; only the + // `[[rule]]` rows' `bypass_env` is looked at, and + // `the_cheap_bypass_read_names_what_the_full_load_names` holds this read to + // the full load's answer. + static NAMES: std::sync::LazyLock> = + std::sync::LazyLock::new(|| bypass_env_vars_in(&at_root("batten.toml"))); NAMES.clone() } +/// `[[rule]].bypass_env` out of the config at `path`, sorted and deduplicated, +/// without the validated load. A file that will not read or parse yields nothing, +/// as the full load's failure did. +/// +/// A two-field serde view rather than a `toml::Value` tree: everything but the +/// `[[rule]]` rows' one column is skipped by the deserializer instead of built. +//MUTANT bypass-read-drops-rows|s@^ .filter_map(\x7crule\x7c rule.bypass_env)$@ .filter_map(\x7crule\x7c rule.bypass_env.filter(\x7c_\x7c false))@|the_cheap_bypass_read_names_what_the_full_load_names +pub(crate) fn bypass_env_vars_in(path: &Path) -> Vec { + #[derive(serde::Deserialize)] + struct Rows { + #[serde(default)] + rule: Vec, + } + #[derive(serde::Deserialize)] + struct Row { + bypass_env: Option, + } + let mut names: Vec = fs::read_to_string(path) + .ok() + .and_then(|text| toml::from_str::(&text).ok()) + .map_or_else(Vec::new, |rows| rows.rule) + .into_iter() + .filter_map(|rule| rule.bypass_env) + .collect(); + names.sort(); + names.dedup(); + names +} + /// The compiled binary, with the ambient environment scrubbed. /// /// Unconditional by design: a helper that scrubbed only where a suite