From 1741a290b541432de71a2d157af00d880283feac Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Fri, 11 Sep 2026 23:08:00 +0000 Subject: [PATCH 1/4] fix(lease): give the poison cooldown its writer, so a clone that reddened CI takes no further turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lease::cooling` landed pure, documented at length and unit-tested, with nothing calling it. Its first parameter is `poisoned_at: Option<&str>` and no code path in the tree ever produced a `Some`, so every real call would have passed `None` and returned `false` on the first line. The behaviour its own doc comment promises — "a holder whose matrix goes red releases and may win the very next acquire, replay onto the same trunk and buy another matrix on the same defect" — described `main`, not the state before some earlier fix. This wires the two ends it was missing. THE WRITER. `land::poisons_this_clone` is the cell, pure and free-standing for the reason `progress_of` is: a decision reachable only through a git resolution and a filesystem write is a decision nothing tests. It is `(Wait, Violation, Red, Own)` — the wait's own refusal, never a stale red in `seen`; `Red` and not `Pending` or `None`, because a could-not-look is not a poisoning; and `Own` and not `Borrowed`, because on a borrowed base the red may be the speculated base's fault and charging it here would let a neighbour's bad tree evict an innocent agent. `note_the_cooldown` records the trunk position. THE READER. `still_cooling` consults `cooling` in `run_lease_acquire`, after `turn` has said whose turn it is and before the compare-and-swap. The order keeps the two separable: folding it into `turn` would put a local file inside a decision every other clone must be able to reproduce from the ref alone. THE RECORD NAMES ITS OWN REF. It stores `\t` so the reader resolves what the writer used. The first spelling wrote a bare sha, which made the reader assume `origin/main`: on any other base it would compare against an unrelated ref, never match, and answer "not cooling" — a fail-open arm inside a mechanism that exists to fail closed, which is the shape CLOUD-1792 was filed over one module away. Naming the ref costs one field and removes the assumption. A record missing its tab is malformed rather than read loosely, because reading it loosely would resurrect exactly that assumption. An unreadable record reads as UNPOISONED, the opposite of `Local::holder`'s rule and for the opposite reason: a defaulted holder id lets two clones claim one lease, while a cooldown defaulting to ON strands a clone over a file it cannot parse. The anchor is a trunk POSITION throughout, never an instant. The cooldown lapses on two observable events — the trunk moving, or the pool going idle — so no wall clock enters a mechanism that would obviously have been written as a timer (CLOUD-1784's decomposition). `//MUTANT poison-never-recorded` inverts the cell. Measured: the declared case dies under it and the two record twins stay green, so the cases discriminate the decision rather than the plumbing around it. Closes CLOUD-1797 --- crates/batten/src/land.rs | 39 ++++++ crates/batten/src/lease.rs | 56 +++++++++ crates/batten/src/lib.rs | 102 ++++++++++++++++ crates/batten/tests/it/lease_lifecycle.rs | 140 ++++++++++++++++++++++ 4 files changed, 337 insertions(+) diff --git a/crates/batten/src/land.rs b/crates/batten/src/land.rs index 134472001..62b1661bf 100644 --- a/crates/batten/src/land.rs +++ b/crates/batten/src/land.rs @@ -1459,6 +1459,45 @@ impl Basis { } } +/// Did THIS clone just redden the metered matrix on a base of its own? +/// +/// The predicate behind the poison cooldown (CLOUD-1797). It is the same cell +/// [`progress_of`] answers `Stop` for, narrowed by the basis, and each part of +/// it is load-bearing: +/// +/// - **`Wait` and `Violation`** is the wait's OWN refusal. Keying on a red +/// reading alone would fire on a wait that succeeded while a stale red sat in +/// `seen` — the defect `progress_of`'s own comment records one cell below. +/// - **`Red`, never `Pending` or `None`.** A could-not-look is not a poisoning, +/// and a cooldown placed on a network blip holds a clone back over nothing. +/// - **`Own`, never `Borrowed`.** On a borrowed base the red may be the +/// speculated base's fault, so charging it here would let a neighbour's bad +/// tree evict an innocent agent — the asymmetry `charge_the_lap` already +/// refuses for the same reason. +/// +/// Pure, and free-standing rather than a branch inside the lap, so the cell can +/// be decided in a test instead of only through a git resolution and a +/// filesystem write. +//MUTANT-SUITE crates/batten/tests/it/lease_lifecycle.rs +//MUTANT poison-never-recorded|s@ matches!(@ !matches!(@|a_red_wait_on_this_clones_own_base_is_what_poisons_it +#[must_use] +pub const fn poisons_this_clone( + step: Step, + code: crate::exit::ExitCode, + seen: Option, + basis: Basis, +) -> bool { + matches!( + (step, code, seen, basis), + ( + Step::Wait, + crate::exit::ExitCode::Violation, + Some(TapVerdict::Red), + Basis::Own + ) + ) +} + #[must_use] pub const fn progress_of( step: Step, diff --git a/crates/batten/src/lease.rs b/crates/batten/src/lease.rs index 0df8b2216..929521cfb 100644 --- a/crates/batten/src/lease.rs +++ b/crates/batten/src/lease.rs @@ -2398,6 +2398,62 @@ impl Local { Ok(minted) } + /// Record that THIS clone's head reddened the metered matrix on `trunk`. + /// + /// The written value is a trunk POSITION, never an instant, and that is the + /// whole reason [`cooling`] can decide without a clock: the cooldown lapses + /// when the trunk moves off this sha or when the pool goes idle, both of them + /// observable events (CLOUD-1784's decomposition, CLOUD-1797's wiring). + /// + /// Beside `holder` in the same per-clone directory, because it is the same + /// kind of fact — something this clone knows about itself that must outlive + /// the process, since the `land` that reddened CI and the `acquire` that must + /// be held back are different processes. + /// + /// # THE RECORD NAMES ITS OWN REF, and that is not decoration + /// + /// It stores `\t`, so the reader resolves the ref the + /// WRITER used rather than a constant of its own. A reader that assumed + /// `origin/main` would compare this clone's poisoned sha against an unrelated + /// ref on any other base, never match, and answer "not cooling" — a + /// fail-open arm inside a mechanism that exists to fail closed, which is the + /// shape CLOUD-1792 was filed over. Naming the ref costs one field and + /// removes the assumption entirely. + /// + /// # Errors + /// + /// A directory or file this clone cannot write. **Never swallowed**: a poison + /// that failed to record reads as an unpoisoned clone on the next acquire, + /// which is the defect this exists to close. + pub fn poison(&self, tracking: &str, at: &str) -> Result<()> { + std::fs::create_dir_all(&self.dir)?; + std::fs::write(self.dir.join("poisoned"), format!("{tracking}\t{at}\n"))?; + Ok(()) + } + + /// The ref and position this clone last poisoned, if it has poisoned one. + /// + /// `None` for an absent, empty or malformed record, and for one this clone + /// cannot read. **Unreadable reads as unpoisoned on purpose** — the opposite + /// of [`Local::holder`]'s rule, and for the opposite reason: a holder id that + /// defaulted would let two clones claim one lease, while a cooldown that + /// defaulted to ON would hold a clone back over a file it cannot read, which + /// is the single-agent stranding [`cooling`]'s idle-pool clause exists to + /// prevent. + /// + /// A record missing its tab is malformed rather than a bare sha, because a + /// bare sha is what the FIRST spelling of this wrote and reading it loosely + /// would resurrect the assumption the tab exists to remove. + #[must_use] + pub fn poisoned(&self) -> Option<(String, String)> { + let record = std::fs::read_to_string(self.dir.join("poisoned")).ok()?; + let (tracking, at) = record.trim().split_once('\t')?; + if tracking.is_empty() || at.is_empty() { + return None; + } + Some((tracking.to_owned(), at.to_owned())) + } + /// How long `token` has been what this clone sees under `name`, on OUR clock. /// /// **Expiry alone is not safe to steal on**, which is the whole reason this diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 25e57129c..0157e5c7a 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -7401,6 +7401,9 @@ fn run_land_lap( // different world than the one that answered (CLOUD-1306). let basis = land::Basis::of(bet.live()); note_the_poison(step, code, basis, &mut bet); + if land::poisons_this_clone(step, code, seen, basis) { + note_the_cooldown(root, reference, out, err)?; + } match land::progress_of(step, code, seen, basis) { land::Progress::Proceed => {} // `None` is not merged, or nobody could say. Either way this is @@ -8052,6 +8055,77 @@ fn note_the_poison( } } +/// Record the poison cooldown when THIS branch's own head reddened the matrix. +/// +/// The mirror of [`note_the_poison`], and deliberately not folded into it: that +/// one is about somebody ELSE's base and lives in the `Bet`, which dies with the +/// process. This one is about THIS clone and must outlive the process, because +/// the `land` that reddened CI and the `acquire` that must be held back are +/// different runs (CLOUD-1797). +/// +/// # The cell, and why each part of it is load-bearing +/// +/// `(Wait, Violation, Red, Own)` — the same cell [`land::progress_of`] answers +/// `Stop` for, narrowed by the basis: +/// +/// - **`Wait` and `Violation`** is the wait's own refusal. Keying on a red +/// reading alone would fire on a wait that SUCCEEDED while a stale red sat in +/// `seen`, which is the defect `progress_of`'s own comment records. +/// - **`Red`, never `Pending` or `None`.** Could-not-look is not a poisoning, and +/// a cooldown placed on a network blip would hold this clone back over nothing. +/// - **`Own`, never `Borrowed`.** On a borrowed base the red may be the +/// speculated base's fault, and charging it to this clone would let a +/// neighbour's bad tree evict an innocent agent — the same asymmetry +/// `charge_the_lap` already refuses. +/// +/// # What it records +/// +/// The trunk POSITION, never an instant. [`lease::cooling`] lapses the cooldown +/// when the trunk moves off that sha or when the pool goes idle, so the whole +/// mechanism is two observable events and no clock (CLOUD-1784). +/// +/// # Why a failure here reports rather than returns +/// +/// The lap is already stopping — this cell is `Progress::Stop`. Failing the land +/// over a bookkeeping write would replace a reported stop with an unreported one +/// and tell the operator less. It is loud on `err` because a cooldown that did +/// not record reads as an unpoisoned clone on the next acquire, which is exactly +/// the hole this closes. +/// +/// **The cell is [`land::poisons_this_clone`], asked by the caller**, for the +/// reason [`land::progress_of`] is a free function rather than a branch inside +/// this loop: the decision is the part worth testing, and a decision reachable +/// only through a git resolution and a filesystem write is a decision nothing +/// tests. +fn note_the_cooldown( + root: &Path, + reference: &str, + out: &mut dyn Write, + err: &mut dyn Write, +) -> Result<()> { + let tracking = land::tracking_ref(reference); + let Ok(Some(trunk)) = git::resolve_ref(root, &tracking) else { + writeln!( + err, + "::error:: land: {tracking} would not read, so the poison cooldown has no base to name" + )?; + return Ok(()); + }; + match lease_identity(root).and_then(|(local, _)| { + local + .poison(&tracking, &trunk) + .map_err(|why| format!("cannot record the poison cooldown: {why}")) + }) { + // Pointer-only: the base, never the record. + Ok(()) => writeln!( + out, + "land: cooling on {trunk} — this head reddened the matrix" + )?, + Err(why) => writeln!(err, "::error:: land: {why}")?, + } + Ok(()) +} + /// Refresh the trunk's remote-tracking ref before a speculation reads it. /// /// **EVERY SPECULATION READ OF THE TRUNK WAS ONE LAP BEHIND** (CLOUD-1620's @@ -10833,6 +10907,16 @@ fn run_lease_acquire( } } lease::Turn::Take(why) => { + // THE POISON COOLDOWN, ASKED AFTER THE TURN AND BEFORE THE CAS + // (CLOUD-1797). `turn` decides whose turn it is; this decides whether + // THIS clone may take a turn it is otherwise owed, and the order is + // what keeps the two separable — folding it into `turn` would put a + // local file inside a decision every other clone must be able to + // reproduce from the ref alone. + if let Some(reason) = still_cooling(root, &local, &observed) { + writeln!(out, "lease: {reason}")?; + return Ok(ExitCode::Violation); + } let body = lease::claim(terms, &holder, branch, &head, now); match lease::cas(terms, &observed, &body, now) { Ok(lease::Outcome::Applied) => { @@ -10857,6 +10941,24 @@ fn run_lease_acquire( } } +/// Why this clone may not take a turn it is otherwise owed, or `None`. +/// +/// The reading half of [`note_the_cooldown`], and the whole of the decision is +/// [`lease::cooling`] — this resolves the two inputs and renders the reason. +/// +/// **It resolves the ref the WRITER recorded**, never a constant of its own. A +/// ref that will not read answers `None`: a could-not-look is not evidence this +/// clone is still poisoned, and holding it back on one would strand a clone over +/// a ref that was renamed or pruned. +fn still_cooling(root: &Path, local: &lease::Local, observed: &lease::Observed) -> Option { + let (tracking, at) = local.poisoned()?; + let trunk = git::resolve_ref(root, &tracking).ok().flatten()?; + // Pointer-only: the ref and the position, never the lease body. + lease::cooling(Some(&at), &trunk, observed).then(|| { + format!("cooling — this clone reddened the matrix on {tracking} at {at}, which is still trunk, and the pool is not idle") + }) +} + /// `lease renew`: extend this clone's lease by one term. fn run_lease_renew( root: &Path, diff --git a/crates/batten/tests/it/lease_lifecycle.rs b/crates/batten/tests/it/lease_lifecycle.rs index 2920345f5..1ca2bdfdf 100644 --- a/crates/batten/tests/it/lease_lifecycle.rs +++ b/crates/batten/tests/it/lease_lifecycle.rs @@ -459,3 +459,143 @@ fn the_rendered_body_opens_with_the_banner_and_ends_with_the_nonce() { "and the head that is about to become main" ); } + +// --- the poison cooldown, end to end through its three pieces (CLOUD-1797) ---- +// +// `lease::cooling` landed pure, documented and unit-tested with NOTHING calling +// it: no code path produced a `Some` for its `poisoned_at`, so every real call +// would have passed `None` and returned `false` on the first line. The cases +// below pin the two halves that were missing — the cell that decides a clone +// poisoned itself, and the record that carries the decision across processes. + +/// **THE CELL.** A red wait, on this clone's OWN base, is what poisons it. +/// +/// Each of the four negatives is a way to hold an innocent clone back, and each +/// was reachable before the qualifier set was complete. The borrowed-base row is +/// the one a simplification deletes first: it looks like a duplicate of the +/// `Own` row above it and is the entire difference between charging a clone for +/// its own bad tree and charging it for a neighbour's. +#[test] +fn a_red_wait_on_this_clones_own_base_is_what_poisons_it() { + use batten::exit::ExitCode; + use batten::land::{Basis, Step, TapVerdict, poisons_this_clone}; + + assert!( + poisons_this_clone( + Step::Wait, + ExitCode::Violation, + Some(TapVerdict::Red), + Basis::Own + ), + "a red wait this clone owns is the whole subject of the cooldown" + ); + + assert!( + !poisons_this_clone( + Step::Wait, + ExitCode::Violation, + Some(TapVerdict::Red), + Basis::Borrowed + ), + "a borrowed base's red may be the base's fault, and charging it here \ + would let a neighbour's bad tree evict an innocent agent" + ); + assert!( + !poisons_this_clone(Step::Wait, ExitCode::Violation, None, Basis::Own), + "could-not-look is not a poisoning" + ); + assert!( + !poisons_this_clone( + Step::Wait, + ExitCode::Violation, + Some(TapVerdict::Pending), + Basis::Own + ), + "pending is not red" + ); + assert!( + !poisons_this_clone( + Step::Wait, + ExitCode::Success, + Some(TapVerdict::Red), + Basis::Own + ), + "a wait that SUCCEEDED with a stale red in hand is not a refusal — this \ + is the cell progress_of's own comment records as the defect" + ); + assert!( + !poisons_this_clone( + Step::Verify, + ExitCode::Violation, + Some(TapVerdict::Red), + Basis::Own + ), + "verify's refusal is a gate's, not the metered matrix going red" + ); +} + +/// **THE RECORD, and the assumption its shape removes.** +/// +/// It round-trips, and it stores the ref BESIDE the sha so the reader resolves +/// what the writer used. A bare sha would have made the reader assume +/// `origin/main`, compare against an unrelated ref on any other base, never +/// match, and answer "not cooling" — a fail-open arm inside a mechanism that +/// exists to fail closed, which is exactly what CLOUD-1792 was filed over one +/// module away. +#[test] +fn the_poison_record_names_the_ref_it_resolved_not_only_the_sha() { + let dir = crate::common::scratch("lease-poison-record"); + let local = batten::lease::Local::under(&dir); + + assert_eq!( + local.poisoned(), + None, + "an unpoisoned clone records nothing" + ); + + local + .poison("origin/release/1.x", "deadbeef") + .expect("the record is writable"); + assert_eq!( + local.poisoned(), + Some((String::from("origin/release/1.x"), String::from("deadbeef"))), + "the reader gets back the ref the writer used, not a constant of its own" + ); + + // A LATER POISONING REPLACES THE EARLIER ONE. Two records would leave the + // reader choosing, and the cooldown is a claim about ONE base. + local + .poison("origin/main", "cafe1234") + .expect("the record is rewritable"); + assert_eq!( + local.poisoned(), + Some((String::from("origin/main"), String::from("cafe1234"))) + ); +} + +/// **A MALFORMED RECORD READS AS UNPOISONED, and that direction is chosen.** +/// +/// The opposite of `Local::holder`'s rule, for the opposite reason: a holder id +/// that defaulted would let two clones claim one lease, while a cooldown that +/// defaulted to ON would hold a clone back over a file it cannot parse — the +/// single-agent stranding `cooling`'s idle-pool clause exists to prevent. +/// +/// The bare-sha row is not hypothetical: it is what the first spelling of this +/// wrote, and reading it loosely would resurrect the very assumption the tab +/// exists to remove. +#[test] +fn an_empty_or_untabbed_poison_record_reads_as_unpoisoned() { + let dir = crate::common::scratch("lease-poison-malformed"); + let local = batten::lease::Local::under(&dir); + std::fs::create_dir_all(&local.dir).expect("the bookkeeping directory"); + let record = local.dir.join("poisoned"); + + for malformed in ["", " \n", "deadbeef\n", "\tdeadbeef\n", "origin/main\t\n"] { + std::fs::write(&record, malformed).expect("the fixture is writable"); + assert_eq!( + local.poisoned(), + None, + "a record this clone cannot parse is not evidence it is still poisoned: {malformed:?}" + ); + } +} From 7e5fb5956f64a5705405fa2acadf7348696ee5fd Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Fri, 11 Sep 2026 23:47:14 +0000 Subject: [PATCH 2/4] fix(lease): give the stand-down notice its sender, so a stale holder can be asked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lease::notice` mints a body carrying a stand-down request. The whole receiving half landed wired — `stand_down_requested`, `Beat::StandDown`, `honour_the_notice`, latched across laps — and `notice` itself had one caller in the tree, its own unit test. So `stand_down` was empty on every lease in the fleet and the receiving half was unreachable in production. That is what made `Body::writer` unable to earn its place. The field exists so a waiter can SEE that the holder runs an older build; seeing it and having no way to act left exactly one remedy — wait out the TTL — which is the outcome the field was added to avoid. THE SENDER is one arm of `run_lease_acquire`'s `Turn::Wait`, and it changes nothing else about the wait: the waiter still answers `Violation`, still does not take the lease, still does not retry. Asking is a REQUEST the holder may decline, which is what makes it the eviction path that stays safe under a false suspicion — `turn`'s steal arms are the unsafe ones and they are untouched. THE DECISION is `lease::worth_asking`, pure, so the three guards are decidable without a ref, a CAS or a wire. Each is a distinct way to get this wrong: - STRICTLY NEWER (`lease::outranks`). An equal build is not out-ranked — two clones on one version asking each other to stand down is a request loop with no newer party to win it. A newer holder must never be asked: that inversion lets an old clone evict the fleet, which is what `Body::writer`'s own doc refuses when it says the field decides nothing by itself. - IDEMPOTENT. A body already carrying a request is left alone. Without it every lap of every waiter rewrites the ref and the holder's heartbeat CAS fails against a stream of cosmetic updates — eviction by contention rather than by asking, the steal this path exists to avoid. A blank `stand-down:` is an absence, not a request, because a rendered body round-trips to that spelling. - NEVER SELF-ADDRESSED, the mirror of `stand_down_requested`'s guard one step earlier: that one stops a holder honouring a notice it minted onto itself, this one stops it being written. `outranks` answers false on every uncertain reading — an unparseable version on either side, and a fourth segment, which would otherwise rank two distinct builds equal. A body written before `writer` existed carries an empty one, and that is the shape that actually occurs on upgrade. A lost CAS is silent: the holder beat, released, or another waiter asked first, and all three make the request moot or already-made. `//MUTANT notice-ignores-the-writer` drops the version comparison. Measured: the declared case dies, the unparseable-writer case dies with it, and the idempotence twin stays green — so the clauses are pinned independently rather than by one case that happens to cover them all. Closes CLOUD-1798 --- crates/batten/src/lease.rs | 61 ++++++++++++ crates/batten/src/lib.rs | 62 ++++++++++++ crates/batten/tests/it/lease_lifecycle.rs | 112 ++++++++++++++++++++++ 3 files changed, 235 insertions(+) diff --git a/crates/batten/src/lease.rs b/crates/batten/src/lease.rs index 929521cfb..a83d7c700 100644 --- a/crates/batten/src/lease.rs +++ b/crates/batten/src/lease.rs @@ -2309,6 +2309,67 @@ pub fn notice(body: &Body, from: &str) -> Body { } } +/// Should `ours` ask a holder writing `theirs` to stand down? +/// +/// The predicate behind the only sender of [`notice`] (CLOUD-1798). Strictly +/// newer, and **every uncertain reading answers `false`**: +/// +/// - **An equal build is not out-ranked.** Two clones on the same version asking +/// each other to stand down is a request loop with no newer party to win it. +/// - **An unreadable version on either side is not evidence of anything.** A +/// body minted by a build whose version this one cannot parse might be newer; +/// reading it as older would let an old clone evict the whole fleet, which is +/// the inversion [`Body::writer`]'s own doc refuses when it says the field +/// decides nothing on its own. +/// +/// A pre-release or build suffix is cut before the comparison rather than +/// ordered, because the question here is "is the holder behind us" and no +/// decision in this repository turns on `-rc.1` ordering. Saying so is cheaper +/// than implementing an ordering nothing asks for. +#[must_use] +pub fn outranks(ours: &str, theirs: &str) -> bool { + fn triple(version: &str) -> Option<(u64, u64, u64)> { + let core = version.trim().split(['-', '+']).next().unwrap_or_default(); + let mut parts = core.split('.'); + let mut next = || parts.next()?.parse::().ok(); + let (major, minor, patch) = (next()?, next()?, next()?); + // A FOURTH SEGMENT IS NOT A VERSION THIS UNDERSTANDS. Reading `1.2.3.4` + // as `1.2.3` would rank two distinct builds equal. + parts.next().is_none().then_some((major, minor, patch)) + } + match (triple(ours), triple(theirs)) { + (Some(ours), Some(theirs)) => ours > theirs, + _ => false, + } +} + +/// Is it worth `asker` leaving a stand-down request on this body? +/// +/// The three guards of the notice sender as one pure predicate, so the decision +/// is testable without a ref, a CAS or a wire (CLOUD-1798). Each clause is a +/// distinct way to get this wrong: +/// +/// 1. **Strictly newer** — [`outranks`], which refuses an equal build and every +/// unreadable one. +/// 2. **Not already asked** — idempotence. Without it every lap of every waiter +/// rewrites the ref and the holder's own heartbeat CAS fails against a stream +/// of cosmetic updates, evicting by contention rather than by asking. +/// 3. **Never self-addressed** — the mirror of +/// [`stand_down_requested`]'s own guard, one step earlier: that one stops a +/// holder honouring a notice it minted onto itself, this one stops it being +/// written. +/// +/// A released or expired body is not excluded here and does not need to be: the +/// caller only reaches this on `Turn::Wait`, which those states never produce. +//MUTANT-SUITE crates/batten/tests/it/lease_lifecycle.rs +//MUTANT notice-ignores-the-writer|s@ outranks(\&writer_version(), \&body.writer)@ true@|a_waiter_asks_only_a_strictly_older_holder_to_stand_down +#[must_use] +pub fn worth_asking(body: &Body, asker: &str) -> bool { + outranks(&writer_version(), &body.writer) + && body.stand_down.trim().is_empty() + && body.holder != asker +} + /// Has a peer asked this holder to stand down? /// /// A pure reading over a body already in hand, so the beat pays no extra fetch diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 0157e5c7a..b6f2fab2a 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -10872,6 +10872,19 @@ fn run_lease_acquire( match &observed { lease::Observed::Held { body, .. } => { writeln!(out, "lease: held by {}", body.holder)?; + // THE ONLY SENDER OF A STAND-DOWN NOTICE (CLOUD-1798). The + // receiving half — `stand_down_requested`, `Beat::StandDown`, + // `honour_the_notice` — landed wired and this arm is what was + // missing, so `stand_down` was empty on every lease in the + // fleet. + // + // IT DOES NOT CHANGE THE WAIT. The waiter still answers + // `Violation`, still does not take the lease and still does + // not retry: asking is a REQUEST the holder may decline, + // which is what makes it the eviction path that is safe + // under a false suspicion. `turn`'s steal arms are the + // unsafe ones and they are unchanged. + ask_the_holder_to_stand_down(terms, &observed, body, &holder, now, out, err)?; Ok(ExitCode::Violation) } // **A REF THAT IS NOT A LEASE IS A WAIT, NOT AN INTERNAL ERROR** @@ -10941,6 +10954,55 @@ fn run_lease_acquire( } } +/// Leave a stand-down request on a holder this clone out-ranks, if it should. +/// +/// **Three guards, and each is a way to get this wrong:** +/// +/// - **Strictly newer only** ([`lease::outranks`]). A waiter asking an equal or +/// NEWER holder to stand down inverts the design and would let an old clone +/// evict the fleet. +/// - **Idempotent.** A body already carrying a request is left alone. Without +/// this, every lap of every waiter rewrites the ref, and the holder's own +/// heartbeat CAS fails against a stream of cosmetic updates — a request that +/// bullies the holder off the lease by contention rather than by asking. +/// - **Never self-addressed.** Guarded here as well as in +/// [`lease::stand_down_requested`], because the reader's guard protects the +/// holder from a notice it minted onto itself and this one stops it being +/// written at all. +/// +/// # Why a lost CAS is silent +/// +/// Somebody else moved the ref — the holder beat, released, or another waiter +/// asked first. All three make the request moot or already-made, and none is +/// this clone's problem. Retrying would be the tight spin `run_lease_acquire`'s +/// own CAS arm refuses for the same reason. +fn ask_the_holder_to_stand_down( + terms: &lease::Terms, + observed: &lease::Observed, + body: &lease::Body, + holder: &str, + now: i64, + out: &mut dyn Write, + err: &mut dyn Write, +) -> Result<()> { + if !lease::worth_asking(body, holder) { + return Ok(()); + } + match lease::cas(terms, observed, &lease::notice(body, holder), now) { + // Pointer-only: who was asked and which build, never the body. + Ok(lease::Outcome::Applied) => writeln!( + out, + "lease: asked {} to stand down — it writes {} and this clone writes {}", + body.holder, + body.writer, + lease::writer_version() + )?, + Ok(lease::Outcome::Rejected { .. }) => {} + Err(reason) => writeln!(err, "::error:: lease: the stand-down request: {reason}")?, + } + Ok(()) +} + /// Why this clone may not take a turn it is otherwise owed, or `None`. /// /// The reading half of [`note_the_cooldown`], and the whole of the decision is diff --git a/crates/batten/tests/it/lease_lifecycle.rs b/crates/batten/tests/it/lease_lifecycle.rs index 1ca2bdfdf..1eccb7ba0 100644 --- a/crates/batten/tests/it/lease_lifecycle.rs +++ b/crates/batten/tests/it/lease_lifecycle.rs @@ -599,3 +599,115 @@ fn an_empty_or_untabbed_poison_record_reads_as_unpoisoned() { ); } } + +// --- the stand-down sender, and the three ways it goes wrong (CLOUD-1798) ----- +// +// `lease::notice` landed with the whole receiving half wired — `stand_down_ +// requested`, `Beat::StandDown`, `honour_the_notice` — and not one non-test +// caller, so `stand_down` was empty on every lease in the fleet. `worth_asking` +// is the sender's decision, pure so it can be decided without a ref or a CAS. + +/// **THE CASE.** A waiter asks a STRICTLY older holder, and nobody else. +/// +/// The equal-build row is the one that matters most and looks most like a +/// formality: two clones on the same version asking each other to stand down is +/// a request loop with no newer party to win it. The newer-holder row is the +/// inversion — an old clone evicting the fleet — which is what `Body::writer`'s +/// own doc refuses when it says the field decides nothing by itself. +#[test] +fn a_waiter_asks_only_a_strictly_older_holder_to_stand_down() { + use batten::lease::{worth_asking, writer_version}; + + let older = Body { + writer: String::from("0.0.1"), + ..held("work", NOW + 60) + }; + assert!( + worth_asking(&older, RIVAL), + "a holder behind this build is the whole subject of the request" + ); + + let equal = Body { + writer: writer_version(), + ..held("work", NOW + 60) + }; + assert!( + !worth_asking(&equal, RIVAL), + "an equal build is not out-ranked, and asking it would be a loop with no \ + newer party to win it" + ); + + let newer = Body { + writer: String::from("999.0.0"), + ..held("work", NOW + 60) + }; + assert!( + !worth_asking(&newer, RIVAL), + "asking a NEWER holder to stand down lets an old clone evict the fleet" + ); +} + +/// **AN UNREADABLE VERSION IS NOT AN OLD ONE**, on either side. +/// +/// A body minted by a build whose version this one cannot parse might be newer. +/// Reading it as older is the same inversion as the row above, reached through a +/// parse instead of a comparison — and a body written before `writer` existed +/// carries an EMPTY one, which is the shape that actually occurs on upgrade. +#[test] +fn a_writer_this_build_cannot_parse_is_never_treated_as_older() { + use batten::lease::worth_asking; + + for unreadable in ["", "tomorrow", "1.2", "1.2.3.4", "v1.2.3", "1.2.x"] { + let body = Body { + writer: String::from(unreadable), + ..held("work", NOW + 60) + }; + assert!( + !worth_asking(&body, RIVAL), + "an unparseable writer is not evidence the holder is behind: {unreadable:?}" + ); + } +} + +/// **IDEMPOTENT, AND NEVER SELF-ADDRESSED.** +/// +/// Without the first, every lap of every waiter rewrites the ref and the +/// holder's own heartbeat CAS fails against a stream of cosmetic updates — +/// eviction by contention rather than by asking, which is precisely the steal +/// this path exists to avoid. The second is the mirror of +/// `stand_down_requested`'s guard, one step earlier. +#[test] +fn a_body_already_asked_or_asked_by_its_own_holder_is_left_alone() { + use batten::lease::worth_asking; + + let base = Body { + writer: String::from("0.0.1"), + ..held("work", NOW + 60) + }; + + let asked = Body { + stand_down: String::from("clone-c"), + ..base.clone() + }; + assert!( + !worth_asking(&asked, RIVAL), + "a request already standing is not re-written every lap" + ); + + assert!( + !worth_asking(&base, HOLDER), + "the holder does not ask itself to stand down" + ); + + // AND THE WHITESPACE SPELLING IS THE SAME STATE. A `stand-down: ` line that + // round-tripped through a render carries a blank rather than an absence, and + // reading that as "nobody asked" would re-write on every lap after all. + let blank = Body { + stand_down: String::from(" "), + ..base.clone() + }; + assert!( + worth_asking(&blank, RIVAL), + "a blank request is an absence, so this one IS worth asking" + ); +} From 4d06ac91542de236363ed5524f0dd470adef77a8 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Fri, 11 Sep 2026 23:55:47 +0000 Subject: [PATCH 3/4] chore(mutant): declare engine-land, now that land.rs carries a mutation row The census holds a module that DECLARES rows to the enforced set: rows nobody sweeps are the coverage-shaped nothing the verb exists to refuse. Adding `//MUTANT poison-never-recorded` to `land.rs` therefore made it a subject, and a subject absent from `$MUTANT_GATES` reads `uncovered`. Caught by `mutate::this_repositorys_own_census_is_closed`, which is the case that makes every census fixture evidence about this repository rather than about a toy tree. Worth recording because the near-miss was mine: `engine-landed` (for `landed.rs`) was already in the set, a hand check for the substring `engine-land` matched inside it, and the set read as already correct. The name is a whole element, not a prefix. Refs: CLOUD-1797 --- mise.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mise.toml b/mise.toml index 048ebf874..2049fc0b0 100644 --- a/mise.toml +++ b/mise.toml @@ -617,7 +617,7 @@ CI_FANIN_WORKFLOW = ".github/workflows/ci.yml" BATS_TEST_TIMEOUT = "300" REGORUS_OPA_COMPLIANCE = "1.2.0" REGORUS_OPA_COMPLIANCE_FOR = "0.11" -MUTANT_GATES = "mise,attestation-check,engine-checks-green,engine-config,engine-doctor,engine-landed,engine-perf,engine-mcp,engine-pinned,engine-ready,engine-verdict,engine-wiring,engine-surface,agentic-experiment-record,awk-regex-check,bats-invocation,batten-glob-check,board-diff-overlap,board-payloads,board-sweep,branch-age-check,cap-drift,cfg-gated-test,ci-cache-declared,ci-hygiene,ci-parity,ci-slow-needed,ci-suite-lane,ci-tools-check,claim-before-code,claim-order-is-stated,claimed-keys,closing-key-check,coderabbit-config-check,commit-hygiene,connector-allow-guard,connector-allow-resolve,container-preflight,darwin-link,deferral-check,denials-outlive-the-turn,digest-major-agreement,doctor,done-check,done-pr-check,duplicate-close-check,egress-fencing,evaluator-closure-check,evaluator-io-check,filed-here,finding-sink-check,fixture-forks,forge-verdict-required,graph-check,harness-grant,harness-wiring,hk-fix-selection,hk-plan-required,hook-pin-check,hook-skip-local,in-progress-drain,install-check,land-divergence-assert,landed-check,landing-loop,landing-roster-guarded,leased-push,license-table-check,linear-check,lock-complete,macos-link-check,mcp-allow-check,mcp-attach-check,mcp-timeout-budget,merged-pr-keys,mise-action-floor,mise-pin-agreement,module-map-check,msrv-pin-agreement,mutation-declared-case,no-doctests,nonverdict-assert,ntia-check,obligations-bound,perf-assert,pinned-toolchain,pipefail-grep-check,plan-complete,pr-partition-restated,pr-unsubscribed,privileged-lane,prose-only,publish-credential-check,ready-cites-check,ready-lint,reclaim-census,release-assets-check,release-due,release-provision-parity,release-tag-shape,release-tracking-check,released,remedy-authorship,repetition-without-progress,report-only-check,review-answered,review-dispatched,run-shape,rust-paths-check,sbom,sbom-inventory,serena-mcp,shell-hygiene,shell-retirement,shell-write-advisory,signing-posture,sonar-gate,spec-ref-check,stop-posture,stop-posture-check,suite-bench-check,suite-subject-retirable,task-substitution,test-targets,timeout-check,token-bench-check,transcript-corpus-check,tree-clean,trunk-based,validator-verdict-clean,verdict-routes-resolve,weakens-declared,worktree-registration,spawn-widening,nextest-slow,engine-lease,engine-handler,engine-speculation,engine-pipeline,engine-policy,rules-paths-trigger,skill-frontmatter-complete" +MUTANT_GATES = "mise,attestation-check,engine-checks-green,engine-config,engine-doctor,engine-landed,engine-perf,engine-mcp,engine-pinned,engine-ready,engine-verdict,engine-wiring,engine-surface,agentic-experiment-record,awk-regex-check,bats-invocation,batten-glob-check,board-diff-overlap,board-payloads,board-sweep,branch-age-check,cap-drift,cfg-gated-test,ci-cache-declared,ci-hygiene,ci-parity,ci-slow-needed,ci-suite-lane,ci-tools-check,claim-before-code,claim-order-is-stated,claimed-keys,closing-key-check,coderabbit-config-check,commit-hygiene,connector-allow-guard,connector-allow-resolve,container-preflight,darwin-link,deferral-check,denials-outlive-the-turn,digest-major-agreement,doctor,done-check,done-pr-check,duplicate-close-check,egress-fencing,evaluator-closure-check,evaluator-io-check,filed-here,finding-sink-check,fixture-forks,forge-verdict-required,graph-check,harness-grant,harness-wiring,hk-fix-selection,hk-plan-required,hook-pin-check,hook-skip-local,in-progress-drain,install-check,land-divergence-assert,landed-check,landing-loop,landing-roster-guarded,leased-push,license-table-check,linear-check,lock-complete,macos-link-check,mcp-allow-check,mcp-attach-check,mcp-timeout-budget,merged-pr-keys,mise-action-floor,mise-pin-agreement,module-map-check,msrv-pin-agreement,mutation-declared-case,no-doctests,nonverdict-assert,ntia-check,obligations-bound,perf-assert,pinned-toolchain,pipefail-grep-check,plan-complete,pr-partition-restated,pr-unsubscribed,privileged-lane,prose-only,publish-credential-check,ready-cites-check,ready-lint,reclaim-census,release-assets-check,release-due,release-provision-parity,release-tag-shape,release-tracking-check,released,remedy-authorship,repetition-without-progress,report-only-check,review-answered,review-dispatched,run-shape,rust-paths-check,sbom,sbom-inventory,serena-mcp,shell-hygiene,shell-retirement,shell-write-advisory,signing-posture,sonar-gate,spec-ref-check,stop-posture,stop-posture-check,suite-bench-check,suite-subject-retirable,task-substitution,test-targets,timeout-check,token-bench-check,transcript-corpus-check,tree-clean,trunk-based,validator-verdict-clean,verdict-routes-resolve,weakens-declared,worktree-registration,spawn-widening,nextest-slow,engine-lease,engine-handler,engine-speculation,engine-pipeline,engine-policy,rules-paths-trigger,skill-frontmatter-complete,engine-land" # --- GitHub reachability behind an egress proxy (Claude Code web sandbox etc.) --- # mise resolves every tool's release through GitHub's *API* host, api.github.com. From d27f4ba14c42fe4a321c0a8db1f57ed1e6ebfcbd Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Sat, 12 Sep 2026 03:04:27 +0000 Subject: [PATCH 4/4] fix(perf): raise the sample to what this comparison's own null needs to decide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `perf-gate` refused #950 at `passthrough` 1.707x. The branch touches no budgeted path. `perf pair --null` — which measures the identical binary as both arms, so every ratio it produces is 1.0 plus pure noise — says why. `REGRESSION_RATIO`'s comment records the original null (2026-08-11, 100 runs after 10 warmups, spread 0.966 to 1.102) and closes with "Re-measure with `perf pair --null`". Re-measured on a busier container at those same defaults: check 4.99 -> 9.37 1.88x noop 7.87 -> 3.55 0.45x posttool 16.26 -> 18.27 1.12x passthrough 3.90 -> 4.21 1.08x wired 106.19 -> 111.40 1.05x hook 7.82 -> 7.80 1.00x A control at 1.88x is refused by a 1.30x threshold, and 0.45x is the same noise with the sign reversed — a binary measured 2.2x faster than itself. Three real pair runs on one unchanged branch each named a different path as the regression, and the `passthrough` base arm alone read 4.43, 7.91 and 8.69 ms. At 300 runs after 30 warmups, same container, same binary: 0.95 to 1.145 — back inside the band the original experiment produced, and clear of the threshold. SO THE CONSTANT MOVES AND THE THRESHOLD DOES NOT. This module's differencing premise — both arms on one machine within seconds, so the noise divides out — holds under steady load and fails when a preemption burst lands inside one arm's window and not the other's. A larger sample makes the burst a smaller fraction of each window. It is the remedy that adds no machinery: interleaving the arms, re-measuring on a failure, and self-calibrating the threshold per run were each considered and are each a way of working around a sample that simply needs to be bigger. Raising `REGRESSION_RATIO` instead was rejected outright. It would have to clear 1.88x, which is above the 1.462x real regression CLOUD-875 measured and caught — trading a false positive for a false negative on the gate whose job is catching the second. `DEFAULT_RUNS` and `DEFAULT_WARMUP` were bare constants carrying no measurement of their own, unlike the threshold below them, so this overturns no recorded experiment. It supplies the one that was never taken, and records both nulls where the threshold records its own. The cost is three times the runs, paid only by a commit touching crate source, a manifest or the lockfile — the gate skips clean otherwise. The case is a FLOOR, not an equality: `== "300"` would pass a later edit back to a sample this comparison is measured to fail at, so long as whoever made it moved the case too. `//MUTANT sample-too-small-to-decide` drops it back to 100 and the case dies. Closes CLOUD-1803 --- crates/batten/src/perf.rs | 44 +++++++++++++++++- crates/batten/tests/it/perf_pair.rs | 71 +++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/crates/batten/src/perf.rs b/crates/batten/src/perf.rs index e9b372cf9..b7fd787b5 100644 --- a/crates/batten/src/perf.rs +++ b/crates/batten/src/perf.rs @@ -87,8 +87,48 @@ const WARMUP_VAR: &str = "BENCH_WARMUP"; const OUT_DIR_VAR: &str = "BENCH_OUT_DIR"; const DEFAULT_BASE_REF: &str = "origin/main"; -const DEFAULT_RUNS: &str = "100"; -const DEFAULT_WARMUP: &str = "10"; + +/// The sample size, **measured against this comparison's own null** (CLOUD-1803). +/// +/// [`REGRESSION_RATIO`] records the 2026-08-11 null — 100 runs after 10 warmups, +/// the identical binary as both arms, spreading 0.966 to 1.102 — and closes with +/// "Re-measure with `perf pair --null`". Re-measured 2026-09-12 on a busier +/// container at those same 100/10 defaults, that spread is **0.45 to 1.88**: +/// +/// | path | ratio, identical binaries | +/// | -- | -- | +/// | `check` | **1.88** | +/// | `noop` | **0.45** | +/// | `posttool` | 1.12 | +/// | `passthrough` | 1.08 | +/// | `wired` | 1.05 | +/// | `hook` | 1.00 | +/// +/// A control at 1.88 is refused by a 1.30 threshold, so the gate was deciding +/// about the machine. Measured again at **300 runs after 30 warmups**, same +/// container, same binary: 0.95 to 1.145 — back inside the band the original +/// experiment produced, and clear of the threshold. +/// +/// **So the constant moved and the threshold did not.** The differencing premise +/// in this module's header — both arms on one machine within seconds, so the +/// noise divides out — holds for a machine under steady load and fails when a +/// preemption burst lands inside one arm's window and not the other's. A larger +/// sample is what makes a burst a smaller fraction of each window; it is the +/// remedy that adds no machinery, and interleaving the arms, re-measuring on a +/// failure, and self-calibrating the threshold per run were each considered and +/// are each a way of working around a sample that simply needs to be bigger. +/// +/// Raising [`REGRESSION_RATIO`] instead was rejected outright: it would have to +/// clear 1.88, which is above the 1.462 real regression CLOUD-875 measured and +/// caught, trading a false positive for a false negative. +/// +/// The cost is three times the runs, paid only by a commit that touches crate +/// source, a manifest or the lockfile — [`Decision`] skips clean otherwise. +//MUTANT-SUITE crates/batten/tests/it/perf_pair.rs +//MUTANT sample-too-small-to-decide|s@const DEFAULT_RUNS: \&str = "300";@const DEFAULT_RUNS: \&str = "100";@|the_default_sample_is_at_least_what_the_null_was_remeasured_at +const DEFAULT_RUNS: &str = "300"; +/// Warmups, moved with [`DEFAULT_RUNS`] and measured in the same pair of nulls. +const DEFAULT_WARMUP: &str = "30"; const DEFAULT_OUT_DIR: &str = "target/perf"; /// What a diff has to touch for the measurement to be worth taking. diff --git a/crates/batten/tests/it/perf_pair.rs b/crates/batten/tests/it/perf_pair.rs index 9e48cbaee..a1dc51375 100644 --- a/crates/batten/tests/it/perf_pair.rs +++ b/crates/batten/tests/it/perf_pair.rs @@ -348,3 +348,74 @@ fn every_path_perf_assert_budgets_is_paired() { ); } } + +/// **The default sample is at least what the null was re-measured at.** +/// +/// A FLOOR, never an equality, and the difference is the whole case. Asserting +/// `== "300"` would pass any later edit back to a sample this comparison is +/// measured to fail at, so long as whoever made it moved the case too. A floor +/// says what was established rather than what is currently written. +/// +/// # What was established +/// +/// `perf pair --null` measures the identical binary as both arms, so every ratio +/// it produces is 1.0 plus pure noise. `REGRESSION_RATIO` records the original: +/// 2026-08-11, 100 runs after 10 warmups, spread 0.966 to 1.102. Re-measured +/// 2026-09-12 on a busier container at those same defaults the spread was 0.45 +/// to 1.88 — a control at 1.88 against a 1.30 threshold, which is the gate +/// deciding about the machine. At 300 runs after 30 warmups: 0.95 to 1.145. +/// +/// # Why this is a test and not only a comment +/// +/// Because the number it defends is invisible in its effect. A sample too small +/// does not fail loudly — it makes a random budgeted path exceed the threshold on +/// a random run, which reads as a regression in whatever branch happened to be +/// measured. `scanner_taxonomy.rs`'s shape: the prose carries the measurement and +/// the assertion stops it evaporating. It does NOT re-run the null, for +/// `lease_namespace_premise.rs`'s reason — a case that benchmarked a real binary +/// would take minutes and would itself be the noisy measurement it is about. +#[test] +fn the_default_sample_is_at_least_what_the_null_was_remeasured_at() { + /// The re-measured sample, below which the null is known to exceed the + /// threshold on this class of container. + const MEASURED_AT: u32 = 300; + + let module = std::fs::read_to_string(common::at_root("crates/batten/src/perf.rs")) + .expect("the module is where the ledger says it is"); + + let declared = |name: &str| -> u32 { + let needle = format!("const {name}: &str = \""); + let rest = module + .split_once(&needle) + .unwrap_or_else(|| panic!("{name} is declared as a string constant")) + .1; + rest.split_once('"') + .expect("the constant is closed") + .0 + .parse() + .expect("the constant is a count") + }; + + assert!( + declared("DEFAULT_RUNS") >= MEASURED_AT, + "the default run count is below the sample the null was re-measured at \ + ({MEASURED_AT}); at 100 runs the identical binary measured 1.88x against \ + a 1.30x threshold, so the gate decides about the machine — see CLOUD-1803" + ); + assert!( + declared("DEFAULT_WARMUP") >= MEASURED_AT / 10, + "the warmups moved with the runs in the same pair of nulls, and a sample \ + grown without them re-admits the cold-start the warmups exist to drop" + ); + + // THE MEASUREMENT IS NAMED SO IT CAN BE RE-RUN, which is what separates this + // from a bare constant somebody must trust. Both arms of the controlled pair + // have to stay legible or the next reader re-derives instead of re-measuring. + for evidence in ["--null", "1.88", "CLOUD-1803"] { + assert!( + module.contains(evidence), + "crates/batten/src/perf.rs must keep naming {evidence:?}, so the \ + sample size reads as a measurement rather than a preference" + ); + } +}