diff --git a/.config/nextest.toml b/.config/nextest.toml index a32170fdf..59851c212 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -1,3 +1,60 @@ +# THE SCRATCH PARENT IS COLLECTED ONCE PER RUN (CLOUD-1879). +# +# `common::scratch` wipes its OWN path before writing, always — `common/mod.rs`'s +# header is emphatic about it, and that is what keeps one case from inheriting +# another's leftovers. Nothing wiped the PARENT, so every run left its fixtures +# behind and the next run paid for them. +# +# MEASURED on this container, same commit, same profile, same binary, idle box. +# The only variable is how many stale directories `target/tmp` held at the start: +# +# entries at start wall summed agentic_record shell_retirement +# 0 194.5s 741.0s 8.7s 77.7s +# ~1250 203.7s 777.8s 10.8s 74.6s +# 3750 220.9s 846.3s 93.5s 74.8s +# +# +13.5% wall clock, monotonic, and one run creates 3248 entries / ~300 MB. +# `shell_retirement` is flat across all three, so the effect is specific rather +# than ambient noise. `agentic_record` swings 10.7x on sibling count ALONE — +# uniformly across every case in the file, including +# `a_tree_with_no_records_at_all_is_silent`, a synthetic empty-tree fixture, at +# 0.20s -> 4.51s. A 22x slowdown on a case with nothing to read is not that +# case's work. +# +# WHY A SETUP SCRIPT AND NOT `scratch` COLLECTING SIBLINGS. nextest runs each case +# in its own process, concurrently; a fixture deleting a sibling's directory is +# exactly the "one case deleting another's `.git` mid-run" failure +# `common/mod.rs` records being repaired one file at a time. A setup script runs +# ONCE, before any test process exists, so it cannot race one. +# +# WHY NOT IN `[tasks."test:cargo"]`. That task is one door — `verify`, `ci` and +# `mise run test` all reach the suite through it — but `windows`, `macos` and +# `musl` call `mise exec -- cargo nextest run --workspace` directly and would be +# uncovered. Those run on fresh runners where nothing accumulates, so the gap is +# harmless today and would be silent the day it is not. +# +# WIPE AT START, NEVER AT END, which is `cargo-test-support`'s property and worth +# keeping: a failing case leaves its tree on disk to `cd` into. Collection here is +# the previous run's, never this one's. +# +# The guard is not decoration: this removes a directory tree, so it refuses any +# path that is not the scratch parent rather than trusting the expansion. +experimental = ["setup-scripts"] + +[scripts.setup.clear-scratch] +# `tr -d` ON THE COUNT, AND IT IS A PORTABILITY BUG RATHER THAN TIDINESS. +# BSD `wc -l` (macOS) pads its output to a fixed width — `" 0"` — where GNU +# `wc -l` emits the bare digits. `$(...)` strips newlines and NOT leading spaces, +# so the published value parsed on the glibc and musl lanes and failed on macos +# alone. Measured: run 35539151175, `the_collector_ran_for_this_run` red with +# `got " 0"` while 5006 other cases passed. The contract this publishes is +# a COUNT, so it is normalised here rather than tolerated at every reader. +command = """sh -c 'd="${CARGO_TARGET_DIR:-target}/tmp"; case "$d" in */tmp) ;; *) echo "clear-scratch: refusing $d" >&2; exit 1 ;; esac; n=$(ls -1 "$d" 2>/dev/null | wc -l | tr -d "[:space:]"); rm -rf "$d" && mkdir -p "$d" && echo "BATTEN_SCRATCH_COLLECTED=$n" >> "$NEXTEST_ENV"'""" + +[[profile.default.scripts]] +filter = "all()" +setup = "clear-scratch" + # A WALL-CLOCK CASE MUST NOT SHARE THE BOX WITH THE SUITE (CLOUD-1444). # # `crates/batten/tests/it/shell_retirement_cost.rs` decides a step-change — is the diff --git a/Cargo.toml b/Cargo.toml index 273e00383..8ddd1a1ed 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -859,6 +859,50 @@ split-debuginfo = "unpacked" debug = 0 opt-level = 2 +# AND THE WORKSPACE CRATE TOO, BECAUSE THE SENTENCE ABOVE WAS WORTH 0.51 SECONDS +# (CLOUD-1878). +# +# `[profile.dev.package."*"]` covers DEPENDENCIES, never workspace members. So the +# `batten` library itself compiled at opt-level 0 inside every test binary, and +# the integration suite ran `policy::load`, `describe`'s serde work and the rules +# scanning unoptimised, in-process, 3568 times. CLOUD-1208 found the dependency +# half of this and stopped at the workspace boundary, deliberately, to keep the +# edit loop fast. +# +# Measured on this container, both arms idle with `target/tmp` cleared before each +# (CLOUD-1879 — without that collector neither arm is reproducible, and two +# earlier arms of this experiment were voided by exactly that): +# +# reading opt-0 opt-2 delta +# suite run, wall 194.5s 153.9s 1.26x +# suite run, summed 741.0s 585.7s -155.3s +# parallel efficiency 95.2% 95.2% unchanged +# incremental rebuild 9.96s 10.47s +0.51s +# cold build 150.5s 219.5s +69s +# +# EFFICIENCY IS IDENTICAL AT 95.2%, which is what makes this work removed rather +# than work rescheduled — a scheduling win would show up there and does not. +# Every file improved or held: `bats_invocation` 2.54x, `surface` 2.15x, +# `perf_assert` 1.65x, `config_lint` 1.64x, `ci_parity` 1.61x, the lib unit tests +# 1.59x, `cli` 1.38x, `shell_retirement` 1.28x. +# +# THE TRADE THE COMMENT ABOVE DEFENDS IS 0.51 SECONDS. Cargo's incremental +# compilation handles the workspace crate at opt-level 2 about as cheaply as at 0, +# because a one-file touch is dominated by the relink rather than by the +# optimiser. The suite paid 40.6s per run for that half-second, on four lanes. +# +# `debug = 1` and `split-debuginfo` are untouched, so every `batten` frame still +# reports its file and line — the half of the block above that is about +# readability is unaffected. +# +# THE COST, STATED: the cold build goes 150.5s -> 219.5s. That reading is n=1 and +# was taken while other work shared the box, so treat it as an upper bound until +# re-taken idle. On a CI cache hit the lane simply saves the run-phase delta; on a +# MISS it is +69s against -40.6s, and `ci.yml` records a miss on 3 of the last 4 +# runs — so the miss rate, not this profile, is the next thing to look at. +[profile.dev.package.batten] +opt-level = 2 + # The profile the distributed single binary is built with: maximal optimization # and the smallest artifact. Distinct from `release` (used for local/dev release # builds) so day-to-day `cargo build --release` stays fast. diff --git a/crates/batten/tests/it/ci_parity.rs b/crates/batten/tests/it/ci_parity.rs index a2fe88805..0c22f6fe6 100644 --- a/crates/batten/tests/it/ci_parity.rs +++ b/crates/batten/tests/it/ci_parity.rs @@ -850,6 +850,58 @@ fn a_zero_concurrent_limit_is_not_a_bound() { ); } +#[test] +fn the_committed_lane_reserves_a_slot_a_freeze_cannot_consume() { + // CLOUD-1881, and the subject is a COMPOSITION rather than either key alone. + // + // `stopUpdatingLabel` makes Renovate leave a labelled PR alone — no rebase, no + // new head, no CI spend, which is what CLOUD-1207 bought it for against a + // measured 372 workflow runs on one branch. But a frozen PR still OCCUPIES a + // concurrency slot. At `prConcurrentLimit: 1` there is no second one, so a + // single frozen head stops the lane for every ecosystem at once. + // + // MEASURED: #676 held the only slot for 28 days, in which Renovate opened + // nothing and `github:nextest-rs/nextest` fell ~41 releases behind. The key + // that wedged it was present, non-zero, and passed every check in this file — + // which is why the bound is `> 1` and not merely "declared". + // + // THE COMMITTED FILE, not a fixture. Every other renovate case here drives a + // synthetic `RENOVATE` const, and a synthetic config cannot go stale in + // production. This one asserts the lane this repository actually runs. + let text = fs::read_to_string(common::at_root("renovate.json5")) + .expect("renovate.json5 is committed at the repository root"); + + // Only a composition is refused, so a lane with no freeze label keeps its + // right to a single slot. Reading the label's presence rather than assuming it + // is what keeps this case honest if that key is ever retired. + if !text.contains("stopUpdatingLabel") { + return; + } + + // THE KEY LINE, NEVER THE PROSE. The first draft of this case used + // `split_once("prConcurrentLimit:")` over the whole file and read the value + // out of the comment three lines above the key — which documents the wedge by + // quoting `prConcurrentLimit: 1`. It failed against a file that already + // carried the fix. A comment line is skipped explicitly rather than hoping the + // key comes first, because this file's convention is reasoning beside keys and + // that reasoning quotes values by design. + let limit = text + .lines() + .map(str::trim_start) + .filter(|line| !line.starts_with("//")) + .find_map(|line| line.strip_prefix("prConcurrentLimit:")) + .map(|rest| rest.trim_start().trim_end_matches(',')) + .and_then(|value| value.parse::().ok()) + .expect("renovate.json5 declares a numeric `prConcurrentLimit`"); + + assert!( + limit > 1, + "`prConcurrentLimit: {limit}` composed with `stopUpdatingLabel` lets one frozen \ + pull request hold the only slot and stop every ecosystem's updates — measured at \ + 28 days on #676 (CLOUD-1881). Reserve a slot a freeze cannot consume." + ); +} + #[test] fn a_top_level_commit_type_does_not_satisfy_the_scoped_one() { // THE MEASURED DEFECT: a top-level key is silently outranked by the diff --git a/crates/batten/tests/it/main.rs b/crates/batten/tests/it/main.rs index c7819ea58..d5571a4e8 100644 --- a/crates/batten/tests/it/main.rs +++ b/crates/batten/tests/it/main.rs @@ -249,6 +249,7 @@ mod run_shape_guard_door; mod runner_verdict; mod sbom_inventory; mod scanner_taxonomy; +mod scratch_hygiene; mod secret_redaction; mod secrets_kind; mod semver_gate; diff --git a/crates/batten/tests/it/scratch_hygiene.rs b/crates/batten/tests/it/scratch_hygiene.rs new file mode 100644 index 000000000..94daa0257 --- /dev/null +++ b/crates/batten/tests/it/scratch_hygiene.rs @@ -0,0 +1,97 @@ +//! The scratch parent is collected once per run, and the collection is asserted +//! rather than declared (CLOUD-1879). +//! +//! # What went wrong +//! +//! `common::scratch` wipes its OWN path before writing, always. Nothing wiped the +//! PARENT, so every run left its fixtures behind — **3248 entries and ~300 MB per +//! run** — and the next run paid for them. Measured on this container, same +//! commit, same profile, same binary, idle box, varying only how many stale +//! directories `target/tmp` held at the start: +//! +//! | entries at start | wall | summed | `agentic_record` | `shell_retirement` | +//! | -- | -- | -- | -- | -- | +//! | 0 | 194.5s | 741.0s | 8.7s | 77.7s | +//! | ~1250 | 203.7s | 777.8s | 10.8s | 74.6s | +//! | 3750 | 220.9s | 846.3s | **93.5s** | 74.8s | +//! +//! **+13.5% wall clock, monotonic.** `shell_retirement` is flat across all three, +//! so the effect is specific rather than ambient noise. `agentic_record` swings +//! 10.7x on sibling count alone, uniformly across every case in that file — +//! including `a_tree_with_no_records_at_all_is_silent`, a synthetic empty-tree +//! fixture, at 0.20s → 4.51s. A 22x slowdown on a case with nothing to read is +//! not that case's own work. +//! +//! It also makes A/B measurement impossible: two arms run back to back are not +//! comparable, because the second starts with ~3250 more directories than the +//! first. Two arms of CLOUD-1878's experiment were voided by exactly that. +//! +//! # Why this asserts a RUN and not a CONFIG +//! +//! The obvious gate is "`.config/nextest.toml` declares a setup script". That is +//! the shape this repository has been bitten by: a gate that asserts a +//! declaration exists and is blind to whether it did anything — +//! `Builtins.shellcheck` read zero `.bats` files for its whole life and reported +//! green the entire time (`hk.pkl`). +//! +//! So the collector publishes its own reading through `$NEXTEST_ENV`, nextest's +//! sanctioned channel from a setup script to the tests, and this file asserts the +//! value arrived. A removed script, an unwired script, or one that failed before +//! its last statement all make the variable absent. **The gate fails when the +//! work did not happen, not when the declaration is missing.** + +// Panicking on setup failure is the idiomatic way for a test to fail loudly. +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use std::path::PathBuf; + +/// What the setup script publishes: how many entries the scratch parent held +/// *before* it was collected. +const COLLECTED: &str = "BATTEN_SCRATCH_COLLECTED"; + +/// The scratch parent itself, as the harness resolves it. +fn parent() -> PathBuf { + PathBuf::from(env!("CARGO_TARGET_TMPDIR")) +} + +#[test] +fn the_collector_ran_for_this_run() { + let reading = std::env::var(COLLECTED).unwrap_or_else(|_| { + panic!( + "{COLLECTED} is unset, so the scratch parent was not collected for this run. \ + It is published by the `clear-scratch` setup script in `.config/nextest.toml` \ + through $NEXTEST_ENV. Either the script is gone, or it is no longer wired to \ + this profile, or it failed before its last statement — and without it the \ + suite gets ~13.5% slower per accumulated run and no two timing arms are \ + comparable (CLOUD-1879)." + ) + }); + + // ANTI-VACUITY. A variable set to anything would satisfy a presence check, so + // the published value must be the shape the script actually writes: a count. + // This is what fails if the `$NEXTEST_ENV` line is reduced to a bare marker. + // + // `trim` BECAUSE A COUNT IS THE CONTRACT AND ITS PADDING IS NOT. BSD `wc -l` + // pads to a fixed width where GNU `wc -l` does not, so this read `" 0"` + // on macos and `"0"` on the two Linux lanes — one red case out of 5007, on the + // one lane whose libc differs. The collector now normalises it, and trimming + // here keeps the assertion about the VALUE rather than about which `wc` ran. + reading.trim().parse::().unwrap_or_else(|_| { + panic!("{COLLECTED} should be the entry count the collector found, got {reading:?}") + }); +} + +#[test] +fn the_parent_survives_its_own_collection() { + // The collector removes the parent and recreates it. Recreating is not + // optional: `common::scratch` joins onto this directory, so a collector that + // deleted without recreating would take every fixture in the run with it — + // and would do so only on the first run after a clean checkout, which is the + // worst possible time to discover it. + let parent = parent(); + assert!( + parent.is_dir(), + "the scratch parent {} must exist after collection — `common::scratch` joins onto it", + parent.display() + ); +} diff --git a/renovate.json5 b/renovate.json5 index 25f717c97..a4e84c7a7 100644 --- a/renovate.json5 +++ b/renovate.json5 @@ -142,12 +142,37 @@ // one matrix in flight, and a queue that drains in landing order rather than a // fan of siblings proposing overlapping groups. // - // It throttles; it does NOT wedge, which was the open question and is now + // It throttles; it does NOT wedge, which was the open question and was // measured (CLOUD-692): #493 merged at 06:49:36Z and the released slot became // #503 at 06:50:56Z, 80 seconds later. So the cost of holding it at 1 is // latency on the tail of the queue, not a stuck queue — and the lander is what // keeps that latency bounded. - prConcurrentLimit: 1, + // + // IT WEDGED, AND THE MEASUREMENT ABOVE IS WHY THAT WAS MISSED (CLOUD-1881). + // #493 and #503 both LANDED, so the slot released on its own. That says nothing + // about a head which never lands: `stopUpdatingLabel` below stops Renovate + // touching a labelled PR, and a frozen PR still OCCUPIES the slot. At a limit of + // 1 there is no second one, so one frozen head stops the lane for every + // ecosystem at once. The composition is the defect, not either key. + // + // MEASURED 2026-09-20. #676 (`renovate/actions`, two minor action bumps) opened + // 2026-08-23, was labelled `renovate-frozen`, and held the only slot for 28 + // days. In that window Renovate opened nothing: five Renovate PRs exist in this + // repository's whole history, and `main` carries ONE `(deps)` commit in its last + // 400 — a hand-written `fix(deps)` for RUSTSEC-2026-0285, not a bot bump. + // `github:nextest-rs/nextest` sat at `cargo-nextest-0.9.104` (2025-09-15) + // against 0.9.145, roughly 41 releases behind. + // + // 2, NOT UNBOUNDED, and the second slot is reserved against exactly this: a + // freeze may consume one slot and never the last one. The throttle's economy is + // untouched, because `draftPR: true` above means an accumulating draft buys zero + // runner minutes — the cost is one more open draft, never one more matrix. + // + // `lock-currency` cannot cover this and its failure mode is GREEN: it asks + // whether regenerating `mise.lock` would change it, which an exactly-pinned tool + // answers "no" forever. Its own header says so, and this file's header records + // the same blindness costing twelve Rust releases before. + prConcurrentLimit: 2, // Renovate's spelling of the 7-day cooldown CLOUD-105 bought on the Dependabot // lanes: let a churny release settle before a PR opens.