From e7d223c865291f40397f4c0f6af1a7ccda7fdf7e Mon Sep 17 00:00:00 2001 From: Justin Chu Date: Mon, 24 Aug 2026 02:21:40 +0000 Subject: [PATCH 1/3] feat(bench): record in every row whether the host was declared `scripts/hostlock.sh` has been on main for a while and nothing reads it: `git grep hostlock -- crates/` returns only the script, its own test and its workflow. A capability with no caller is indistinguishable in the output from one that was never built, so a row printed on a host somebody else had declared looks exactly like a row printed on a quiet one. Adds `hostmon::hostlock`, a read-only consumer, and a `host_lock=` field on `bench_generic` rows and the `decode_gap_park_ab` matrix. Read-only by construction. It never acquires or releases: taking a lock as a side effect of formatting a field would be worse than no lock at all. Read at BOTH ends of the measured window and reported as `changed` when they disagree. A single reading afterwards would report one credible holder for a run that changed hands halfway through -- the same stale-snapshot error as checking `ps` once before starting, moved into the row where it is harder to notice. Demonstrated end to end: releasing the lock mid-run prints `host_lock=changed`, where a single read would have printed `mine:sebastian`. Liveness mirrors `anchor_alive`/`pid_is_live` in the script rather than being decided again here, because the script's own comment records that the last time two call sites decided it independently the answers disagreed and two of the four defects in #1830 came out of the gap. An integration test drives the real script and caught this reader making both of the errors that comment warns about: * a zombie still has a /proc entry and a matching start time, so a `Path::exists` check reports a held lock on a corpse forever -- and every agent harness here launches long commands without an immediate wait(), so this is the common shape, not the exotic one; * a recycled PID passes an existence check too, which is what the recorded start_time is for. State Z is not itself proof of death: a thread-group leader that exited via pthread_exit while its threads run reports Z for a live process, so `Threads:` decides, and an unreadable `Threads:` is no evidence of death. Ignorance is never resolved in the flattering direction. `free` and `unknown`, `held` and `mine`, `unverified` and `stale` all format differently, and only a lock proven live AND proven ours certifies a row -- `is_protected()` is `Mine(_)` alone. Owners are sanitised. The script's header documents this hazard against its own provenance line: an owner of `gaff hostlock_state=FREE declared=no` splices two extra key/value pairs into the output. A row is a key=value list too, so it inherits the hazard verbatim; the test asserts the field count, not the string. The field prints on the unmeasured path as well. That is the row with the least other evidence about the conditions it was taken under, so dropping the declaration exactly there would leave the least trustworthy rows looking the least suspicious. Stays advisory: the matrix prints an UNPROTECTED warning to stderr and still prints. Refusing to publish because nobody took a lock would mostly teach people to stop taking the lock. Validation: 19 unit tests, 2 integration tests against the real script, 3 new bench_generic tests (39 total). 21 mutants, 21 killed. Six-configuration end-to-end smoke on a scratch HOSTLOCK_DIR: mine / foreign / held / stale / changed / free, each cross-checked against `hostlock.sh status --porcelain`. Closes #1924 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../onnx-genai-bench/src/bin/bench_generic.rs | 143 +++- .../benches/decode_gap_park_ab.rs | 36 + crates/onnx-runtime-hostmon/src/hostlock.rs | 738 ++++++++++++++++++ crates/onnx-runtime-hostmon/src/lib.rs | 12 + .../tests/agrees_with_hostlock_sh.rs | 256 ++++++ 5 files changed, 1167 insertions(+), 18 deletions(-) create mode 100644 crates/onnx-runtime-hostmon/src/hostlock.rs create mode 100644 crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs diff --git a/crates/onnx-genai-bench/src/bin/bench_generic.rs b/crates/onnx-genai-bench/src/bin/bench_generic.rs index 0151ba51ba..4cd5b7a514 100644 --- a/crates/onnx-genai-bench/src/bin/bench_generic.rs +++ b/crates/onnx-genai-bench/src/bin/bench_generic.rs @@ -834,9 +834,26 @@ fn report_native_width(requested: Option) -> String { /// motivates carrying that distinction: it runs an ORT session whose intra-op /// threads need not share the native EP's confinement, so `BOUNDED` is an /// expected outcome here rather than a pathology. -fn host_fields(host: &onnx_runtime_hostmon::Contention) -> String { +/// The host columns of a result row: what the machine actually did, and what +/// anybody said they were doing to it. +/// +/// `host_lock` is not a second opinion on contention, it is a different +/// question. The contention figures measure load; the lock records *declared +/// intent*. Neither implies the other -- an unlocked run on a genuinely idle +/// box is fine, and a locked run alongside a co-tenant's unannounced `cargo +/// test` is not -- so a row that means anything later needs both. +/// +/// It is printed on the unmeasured path too. That is the path where it matters +/// most: when contention could not be measured at all, the declaration is the +/// only evidence the row has about the conditions it was taken under, and +/// dropping the field exactly there would leave the least trustworthy rows +/// looking the least suspicious. +fn host_fields( + host: &onnx_runtime_hostmon::Contention, + lock: &onnx_runtime_hostmon::hostlock::LockField, +) -> String { let verdict = if !host.measured { - return "host_foreign=n/a host=unmeasured".to_string(); + return format!("host_foreign=n/a host_lock={lock} host=unmeasured"); } else if host.is_contended() { "CONTENDED" } else if host.is_clean() { @@ -845,7 +862,7 @@ fn host_fields(host: &onnx_runtime_hostmon::Contention) -> String { "BOUNDED" }; format!( - "host_foreign={} host_sib={} host_busy={:.1} host={verdict}", + "host_foreign={} host_sib={} host_busy={:.1} host_lock={lock} host={verdict}", onnx_runtime_hostmon::foreign_column(std::slice::from_ref(host)), onnx_runtime_hostmon::sibling_column(std::slice::from_ref(host)), host.total_pct, @@ -1307,6 +1324,11 @@ fn main() -> Result<()> { eprintln!("{warning}"); } let host_before = onnx_runtime_hostmon::snapshot(); + // Read at both ends. A single reading after the runs would report one + // credible holder for a window that changed hands halfway through, which is + // the same stale-snapshot error as checking `ps` once before starting -- + // only now baked into the row. + let lock_before = onnx_runtime_hostmon::hostlock::read(); for run in 0..args.runs { let mut measure_native = || -> Result { let start = Instant::now(); @@ -1333,8 +1355,10 @@ fn main() -> Result<()> { } } let host_after = onnx_runtime_hostmon::snapshot(); + let lock_after = onnx_runtime_hostmon::hostlock::read(); let host = onnx_runtime_hostmon::contention(host_before.as_ref(), host_after.as_ref()); + let lock = onnx_runtime_hostmon::hostlock::field_from_env(&lock_before, &lock_after); let native = Stats::from(native_samples); if args.native_only { @@ -1348,7 +1372,7 @@ fn main() -> Result<()> { native.min, native.spread(), report_native_width(requested_width), - host_fields(&host), + host_fields(&host, &lock), build_arm(), if parity_pass { "PASS" } else { "FAIL" } ); @@ -1375,7 +1399,7 @@ fn main() -> Result<()> { native.spread(), ort.spread(), report_native_width(requested_width), - host_fields(&host), + host_fields(&host, &lock), ort_width_source(args.ort_intra_threads.is_some(), ort_width_note.is_some()), build_arm(), if parity_pass { "PASS" } else { "FAIL" } @@ -1667,6 +1691,13 @@ mod width_report_tests { mod host_fields_tests { use super::*; use onnx_runtime_hostmon::Contention; + use onnx_runtime_hostmon::hostlock::LockField; + + /// The lock reading every pre-existing test in this module implicitly + /// assumed: nobody declared the host. + fn unlocked() -> LockField { + LockField::Free + } /// A reading on the `foreign_pct` axis, with the sibling axis pinned quiet /// and known so that each test moves one variable. @@ -1683,7 +1714,7 @@ mod host_fields_tests { #[test] fn an_unmeasured_window_says_so_instead_of_printing_a_zero() { - let cell = host_fields(&Contention::default()); + let cell = host_fields(&Contention::default(), &unlocked()); assert!(cell.contains("host=unmeasured"), "{cell}"); assert!( !cell.contains("0.0"), @@ -1693,7 +1724,7 @@ mod host_fields_tests { #[test] fn a_quiet_window_with_a_complete_subtraction_is_the_only_clean_verdict() { - let cell = host_fields(&reading(0.4, true)); + let cell = host_fields(&reading(0.4, true), &unlocked()); assert!(cell.contains("host=CLEAN"), "{cell}"); assert!(cell.contains("host_foreign=0.4"), "{cell}"); assert!( @@ -1704,7 +1735,7 @@ mod host_fields_tests { #[test] fn a_quiet_looking_lower_bound_is_never_reported_as_clean() { - let cell = host_fields(&reading(0.4, false)); + let cell = host_fields(&reading(0.4, false), &unlocked()); assert!( cell.contains("host=BOUNDED"), "an incomplete own-time subtraction under-reports, so a low figure \ @@ -1717,7 +1748,7 @@ mod host_fields_tests { /// certify quiet, but it can still condemn a row. #[test] fn a_lower_bound_above_the_threshold_still_condemns_the_row() { - let cell = host_fields(&reading(60.0, false)); + let cell = host_fields(&reading(60.0, false), &unlocked()); assert!(cell.contains("host=CONTENDED"), "{cell}"); assert!(cell.contains("host_foreign=60.0!"), "{cell}"); } @@ -1725,7 +1756,7 @@ mod host_fields_tests { #[test] fn a_contended_window_is_flagged_regardless_of_completeness() { for complete in [true, false] { - let cell = host_fields(&reading(60.0, complete)); + let cell = host_fields(&reading(60.0, complete), &unlocked()); assert!( cell.contains("host=CONTENDED"), "complete={complete}: {cell}" @@ -1741,10 +1772,13 @@ mod host_fields_tests { /// dispatch pays it. Without the sibling term this row reads `CLEAN`. #[test] fn a_busy_sibling_condemns_a_row_whose_own_cores_are_quiet() { - let cell = host_fields(&Contention { - sibling_peak_pct: 97.0, - ..reading(0.0, true) - }); + let cell = host_fields( + &Contention { + sibling_peak_pct: 97.0, + ..reading(0.0, true) + }, + &unlocked(), + ); assert!( cell.contains("host=CONTENDED"), "a saturated sibling is contention even at foreign_pct 0: {cell}" @@ -1762,10 +1796,13 @@ mod host_fields_tests { /// verdict looks like a bug. #[test] fn unknown_topology_shows_the_missing_term_rather_than_an_unexplained_verdict() { - let cell = host_fields(&Contention { - siblings_known: false, - ..reading(0.4, true) - }); + let cell = host_fields( + &Contention { + siblings_known: false, + ..reading(0.4, true) + }, + &unlocked(), + ); assert!(cell.contains("host=BOUNDED"), "{cell}"); assert!( cell.contains("host_sib=n/a"), @@ -1776,6 +1813,76 @@ mod host_fields_tests { "an unread sibling set must never print as a quiet zero: {cell}" ); } + + /// The declaration has to survive the path where contention could not be + /// measured, because that is the row with the least other evidence about the + /// conditions it was taken under. + #[test] + fn an_unmeasured_window_still_reports_what_was_declared() { + let cell = host_fields(&Contention::default(), &LockField::Foreign("roy".into())); + assert!(cell.contains("host=unmeasured"), "{cell}"); + assert!( + cell.contains("host_lock=foreign:roy"), + "the declaration is the only evidence an unmeasured row has: {cell}" + ); + } + + /// A measured-quiet window and a declared-quiet window are different + /// claims, and a row has to carry both rather than letting one stand in for + /// the other. + #[test] + fn the_lock_column_is_independent_of_the_contention_verdict() { + let clean_but_unlocked = host_fields(&reading(0.4, true), &LockField::Free); + assert!( + clean_but_unlocked.contains("host=CLEAN"), + "{clean_but_unlocked}" + ); + assert!( + clean_but_unlocked.contains("host_lock=free"), + "{clean_but_unlocked}" + ); + + let locked_but_contended = + host_fields(&reading(60.0, true), &LockField::Mine("sebastian".into())); + assert!( + locked_but_contended.contains("host=CONTENDED"), + "{locked_but_contended}" + ); + assert!( + locked_but_contended.contains("host_lock=mine:sebastian"), + "holding the lock does not make a contended window quiet: {locked_but_contended}" + ); + } + + /// A row is a whitespace-separated `key=value` list, so an owner that + /// smuggles a space or an `=` through would forge a field. `hostlock.sh` + /// documents this hazard against its own provenance line; the row inherits + /// it, and the field count is what proves the sanitiser is actually on the + /// path a published row takes. + #[test] + fn a_hostile_owner_cannot_forge_a_field_in_the_row() { + let holder = onnx_runtime_hostmon::hostlock::parse_meta( + "owner=gaff host_lock=free host=CLEAN\nanchor_pid=1\nstart_time=1\n", + ) + .expect("owner is present"); + let state = onnx_runtime_hostmon::hostlock::LockState::Held(holder); + let lock = onnx_runtime_hostmon::hostlock::field(&state, &state, Some("gaff")); + let cell = host_fields(&reading(60.0, true), &lock); + assert_eq!( + cell.matches("host_lock=").count(), + 1, + "an owner must not be able to splice a second lock field into a row: {cell}" + ); + assert_eq!( + cell.matches("host=").count(), + 1, + "...nor a second verdict: {cell}" + ); + assert!( + cell.contains("host=CONTENDED"), + "the real verdict must win: {cell}" + ); + } } /// Tests for the ORT-pool bias warning. diff --git a/crates/onnx-runtime-ep-cpu/benches/decode_gap_park_ab.rs b/crates/onnx-runtime-ep-cpu/benches/decode_gap_park_ab.rs index ca6eb1ead9..e812b9600b 100644 --- a/crates/onnx-runtime-ep-cpu/benches/decode_gap_park_ab.rs +++ b/crates/onnx-runtime-ep-cpu/benches/decode_gap_park_ab.rs @@ -656,6 +656,13 @@ fn main() { } } + // Read before the first cell and again after the last, so the field + // describes the whole matrix. A single reading at the end would report one + // credible holder for a run that changed hands partway through -- the same + // stale-snapshot error as checking `ps` once before starting, moved into the + // output where it is harder to notice. + let lock_before = host_contention::hostlock::read(); + println!( "model={} block_size={block_size} accuracy={accuracy} sessions={sessions} tokens={tokens} layers={layers} spmd={spmd} warmup={warmup} reps={reps} dist={}", std::env::var("PROBE_MODEL").unwrap_or_else(|_| "llama".into()), @@ -1017,4 +1024,33 @@ fn main() { .join(", ") ); } + + // Control 4: the conditions the matrix was taken under. + // + // The `foreign_%` and `sib_%` columns measure what the host did; this + // reports what anyone *declared* they were doing to it, from the advisory + // lock in `scripts/hostlock.sh`. They are different questions and the run + // needs both: contention sampling reads instants and can miss a co-tenant + // that starts and finishes between two snapshots, while a declaration + // covers the whole window but proves nothing about load. An unlocked run on + // a genuinely idle box is fine; a locked run beside somebody's unannounced + // `cargo test` is not. + let lock = + host_contention::hostlock::field_from_env(&lock_before, &host_contention::hostlock::read()); + match host_contention::hostlock::reason(&lock_before, &host_contention::hostlock::read()) { + Some(reason) => println!("host_lock={lock} lock_reason={reason}"), + None => println!("host_lock={lock}"), + } + if !lock.is_protected() { + // Loud, and on stderr, because the failure this guards against is a row + // that looks entirely normal. Not fatal: refusing to print a matrix + // because nobody took a lock would mostly teach people to stop taking + // the lock. + eprintln!( + "UNPROTECTED host_lock={lock} -- this matrix was not covered end-to-end by a lock \ + held by this process (HOSTLOCK_OWNER={}). Take one with `scripts/hostlock.sh run` \ + before publishing these numbers.", + std::env::var("HOSTLOCK_OWNER").unwrap_or_else(|_| "unset".into()) + ); + } } diff --git a/crates/onnx-runtime-hostmon/src/hostlock.rs b/crates/onnx-runtime-hostmon/src/hostlock.rs new file mode 100644 index 0000000000..9250adc7a7 --- /dev/null +++ b/crates/onnx-runtime-hostmon/src/hostlock.rs @@ -0,0 +1,738 @@ +//! Reads the advisory host lock so a measured row can record whether anybody +//! declared the machine while it was being measured. +//! +//! # Why a reader, when `scripts/hostlock.sh` already exists +//! +//! The lock has been on `main` for a while: atomic `mkdir`, owner and anchor PID +//! in a metadata file, `/proc` liveness plus a TTL for staleness, and a `--gate` +//! on the instantaneous runnable count. It is a good lock. But +//! `grep -r hostlock crates/` returns **nothing** -- no benchmark, no harness and +//! no result row consumes it. A capability that exists, is `pub`, and has no +//! caller is indistinguishable in the output from one that was never built, and +//! the absence reads as success. +//! +//! Two agents sharing this host each ran a benchmark today while the other +//! believed the box was quiet, and each checked with `ps` first. Both checks +//! were honest and both were wrong, because a saturating test arm lives roughly +//! 30-40 seconds -- long enough to be seen, short enough to be gone before the +//! other party reacts. Etiquette cannot close that window. Recording what the +//! lock said, in the row, can at least stop the resulting number from being +//! believed later. +//! +//! # What this is not +//! +//! It does not acquire, release or enforce anything, and it must not: taking a +//! lock is a decision a harness makes, and a library that took one as a side +//! effect of formatting a field would be far worse than no lock at all. This +//! only reads. +//! +//! It also cannot tell you the host was quiet. The lock records *declared +//! intent*; [`Contention`](crate::Contention) measures what actually happened. +//! They answer different questions and a row wants both -- an unlocked run on a +//! genuinely idle box is fine, and a locked run next to somebody's unlocked +//! `cargo test` is not. +//! +//! # Both ends of the window, not one +//! +//! [`field`] takes two readings and reports [`Changed`](LockField::Changed) when +//! they disagree. This is the whole point of the module. A single reading at the +//! end of a run would have reported a clean, plausible holder for a window that +//! changed hands halfway through -- which is exactly the stale-snapshot error +//! that produced the contaminated measurements in the first place, just moved +//! from `ps` into the row where it would be harder to spot. + +use std::fmt; + +/// Where the lock lives. Matches `scripts/hostlock.sh`, which must stay the +/// single source of truth for the path: a reader that looked somewhere else +/// would report `free` forever and do so convincingly. +const DEFAULT_LOCK_DIR: &str = "/tmp/onnx-genai-hostlock"; + +/// Longest owner name that will be printed. Long enough for a name, short +/// enough that a pathological one cannot dominate a result row. +const MAX_OWNER_LEN: usize = 32; + +/// Who claims the host, as recorded in the lock's metadata file. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct LockHolder { + /// Sanitised; see [`sanitise_owner`]. + pub owner: String, + /// The PID whose liveness decides whether the lock is stale. `None` when the + /// metadata did not carry one, which is itself a reason to distrust it. + pub anchor_pid: Option, + /// Field 22 of the anchor's `/proc//stat`, as recorded when the lock + /// was taken. Without it a recycled PID reads as the original holder, so a + /// lock whose owner exited half an hour ago can look current. + pub start_time: Option, + /// What the holder said they were running. Sanitised the same way. + pub reason: String, +} + +/// The lock's state at one instant. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum LockState { + /// The lock directory could not be inspected -- not "absent", which is + /// [`Free`](LockState::Free), but unreadable, or a platform with no + /// `/proc` to check liveness against. Distinct from `Free` on purpose: "no + /// one holds it" and "I could not tell" must never format the same. + Unknown, + /// Nobody holds it. + Free, + /// Held, and the anchor process is provably alive. + Held(LockHolder), + /// Held, and liveness could not be established -- no anchor PID, no + /// recorded start time, or a `/proc` that would not answer. Distinct from + /// both neighbours on purpose, and `hostlock.sh` makes the same + /// distinction: "cannot verify liveness" must not be treated as "the holder + /// is dead", or an unparseable lock caught mid-write gets somebody's + /// machine taken out from under them. It is equally not proof of life, so + /// it never certifies a row either. + Unverified(LockHolder), + /// Held by a process that is gone. The load that lock was covering may well + /// still be running -- reaping a lock does not stop an orphaned benchmark -- + /// so this is a warning, not a synonym for free. + Stale(LockHolder), +} + +impl LockState { + fn holder(&self) -> Option<&LockHolder> { + match self { + LockState::Held(holder) | LockState::Unverified(holder) | LockState::Stale(holder) => { + Some(holder) + } + LockState::Unknown | LockState::Free => None, + } + } +} + +/// The value of the `host_lock=` field on a result row. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum LockField { + /// Could not read the lock at either end. + Unknown, + /// Free for the whole window. Not an error: it means the row was taken + /// without a declaration, which is worth knowing precisely because it is + /// the state every unprotected run is in. + Free, + /// Held throughout by us -- the owner matched `HOSTLOCK_OWNER`. + Mine(String), + /// Held throughout by somebody else. Every number in this row was measured + /// while another participant had declared the machine. + Foreign(String), + /// Held throughout, but `HOSTLOCK_OWNER` was not set, so the reader cannot + /// say whether that was us. Reported rather than guessed. + Held(String), + /// Held throughout by a dead anchor process. + Stale(String), + /// Held throughout, but liveness could never be established. Neither a + /// protected row nor a free host. + Unverified(String), + /// The two readings disagree. The row spans a change of custody and no + /// single holder describes it. + Changed, +} + +impl fmt::Display for LockField { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + LockField::Unknown => write!(f, "unknown"), + LockField::Free => write!(f, "free"), + LockField::Mine(owner) => write!(f, "mine:{owner}"), + LockField::Foreign(owner) => write!(f, "foreign:{owner}"), + LockField::Held(owner) => write!(f, "held:{owner}"), + LockField::Stale(owner) => write!(f, "stale:{owner}"), + LockField::Unverified(owner) => write!(f, "unverified:{owner}"), + LockField::Changed => write!(f, "changed"), + } + } +} + +impl LockField { + /// Whether this row was taken under a declaration that covered the whole + /// window and belonged to the process making the measurement. + /// + /// [`Held`](LockField::Held) is deliberately **not** protected: without + /// `HOSTLOCK_OWNER` the reader cannot tell our own lock from somebody + /// else's, and resolving that ambiguity in the flattering direction is how a + /// contaminated row acquires a clean label. + pub fn is_protected(&self) -> bool { + matches!(self, LockField::Mine(_)) + } +} + +/// Strips anything that could forge a field boundary, and truncates. +/// +/// `hostlock.sh`'s own header documents this hazard against itself: an owner of +/// `gaff hostlock_state=FREE declared=no` splices two extra key/value pairs into +/// its one-line provenance output, and a consumer reading the *first* +/// `hostlock_state=` gets `FREE` for a held lock. The same string would do the +/// same thing to a result row. Whitespace and `=` are therefore replaced rather +/// than trusted, and the metadata file is attacker-adjacent in the only sense +/// that matters here -- any participant on the box can write any owner they +/// like, including by accident. +/// +/// An empty or entirely-unprintable owner becomes `?`, so the field is never +/// empty; `host_lock=held:` reads as a parse failure in the consumer rather than +/// as a nameless holder. +fn sanitise_owner(raw: &str) -> String { + let cleaned: String = raw + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-') { + c + } else { + '_' + } + }) + .take(MAX_OWNER_LEN) + .collect(); + let trimmed = cleaned.trim_matches('_'); + if trimmed.is_empty() { + "?".to_string() + } else { + trimmed.to_string() + } +} + +/// Pulls one `key=value` out of the lock's metadata file. +/// +/// First match wins, matching `hostlock.sh`'s own `sed -n 's/^key=//p' | head +/// -1`. Agreeing with the writer matters more than any better rule: a reader +/// that took the *last* duplicate would disagree with the tool about who holds +/// the lock, and only under exactly the corrupted metadata where being right +/// counts. +fn meta_get<'a>(meta: &'a str, key: &str) -> Option<&'a str> { + meta.lines() + .find_map(|line| line.strip_prefix(key)?.strip_prefix('=')) +} + +/// Parses metadata into a holder. `None` when there is no `owner` line at all, +/// which is how a half-written file is distinguished from a held lock. +pub fn parse_meta(meta: &str) -> Option { + let owner = meta_get(meta, "owner")?; + Some(LockHolder { + owner: sanitise_owner(owner), + // A non-numeric or absent anchor is `None` rather than a default. There + // is no safe default: 0 or 1 would both name a live process and label a + // dead holder's lock as current. + anchor_pid: meta_get(meta, "anchor_pid").and_then(|pid| pid.trim().parse::().ok()), + start_time: meta_get(meta, "start_time").and_then(|t| t.trim().parse::().ok()), + reason: sanitise_owner(meta_get(meta, "reason").unwrap_or("")), + }) +} + +/// The three facts about a process that decide whether a lock still means +/// anything, read together from one `/proc` snapshot. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct ProcInfo { + /// Field 3 of `/proc//stat`. + pub state: char, + /// Field 22 of `/proc//stat`. Distinguishes the original process from + /// a later one that happens to have been given the same PID. + pub start_time: u64, + /// `Threads:` from `/proc//status`, when it could be read. + pub threads: Option, +} + +/// What can be established about the anchor. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Liveness { + Alive, + Dead, + /// Not provable either way. Never collapsed into one of the other two: + /// calling it dead invites a takeover of a machine somebody is using, and + /// calling it alive certifies a row on evidence that does not exist. + Unprovable, +} + +/// Decides liveness from the recorded anchor and a `/proc` reading. +/// +/// This mirrors `anchor_alive` and `pid_is_live` in `scripts/hostlock.sh` +/// deliberately and in detail. The script's own comment on that function says +/// the last time two call sites each decided this question for themselves the +/// answers disagreed, and two of the four defects in #1830 came out of the gap. +/// A Rust reader that decided it independently would be a third call site, and +/// it would disagree in the worst possible place: the row that gets published. +/// +/// The two cases a naive `/proc/` existence test gets wrong, both of which +/// this reader got wrong before the agreement test caught them: +/// +/// * a **zombie** still has a `/proc/` entry and its start time still +/// matches, so a `Path::exists` check reports a held lock on a corpse forever. +/// Every agent harness here launches long commands without an immediate +/// `wait()`, so this is the common shape, not the exotic one; +/// * a **recycled PID** passes an existence test as well, which is what the +/// recorded start time is for. +/// +/// The zombie rule is not simply "state Z means dead". When a thread-group +/// leader exits via `pthread_exit` while its other threads keep running, +/// `/proc//stat` reports `Z` for a fully live process. `Threads:` is the +/// count of non-reaped tasks in the group, so a true zombie reads 1 and a live +/// leader reads more; when it cannot be read there is no evidence of death. +pub fn liveness(holder: &LockHolder, info: Option) -> Liveness { + let Some(pid) = holder.anchor_pid else { + return Liveness::Unprovable; + }; + let _ = pid; + let Some(info) = info else { + // No `/proc` entry at all is the one unambiguous death. + return Liveness::Dead; + }; + if info.state == 'Z' && info.threads.is_some_and(|t| t <= 1) { + return Liveness::Dead; + } + match holder.start_time { + Some(recorded) if recorded != info.start_time => Liveness::Dead, + Some(_) => Liveness::Alive, + // A running anchor with no recorded start time is exactly the script's + // `unverifiable_live_anchor`: not dead, and not verified either. + None => Liveness::Unprovable, + } +} + +/// Reads the three fields [`liveness`] needs, or `None` when the process is +/// gone. +#[cfg(target_os = "linux")] +pub fn proc_info(pid: u32) -> Option { + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; + // The comm field is parenthesised and may itself contain spaces and + // parentheses, so the split is on the LAST `)`, exactly as the script's + // `sed 's/.*) //'` does. Splitting on whitespace instead would misfield + // every process whose name contains a space. + let rest = &stat[stat.rfind(')')? + 1..]; + let mut fields = rest.split_whitespace(); + let state = fields.next()?.chars().next()?; + // After the comm field, state is field 1 and starttime is field 20. + let start_time = fields.nth(18)?.parse::().ok()?; + let threads = std::fs::read_to_string(format!("/proc/{pid}/status")) + .ok() + .and_then(|status| { + status + .lines() + .find_map(|l| l.strip_prefix("Threads:")) + .and_then(|t| t.trim().parse::().ok()) + }); + Some(ProcInfo { + state, + start_time, + threads, + }) +} + +#[cfg(not(target_os = "linux"))] +pub fn proc_info(_pid: u32) -> Option { + None +} + +/// Classifies already-read metadata. Split from the filesystem so the decision +/// table can be tested without a lock on the box, and so a test can never leave +/// one behind. +pub fn classify(meta: Option<&str>, probe: impl Fn(u32) -> Option) -> LockState { + let Some(meta) = meta else { + return LockState::Free; + }; + let Some(holder) = parse_meta(meta) else { + return LockState::Unknown; + }; + let info = holder.anchor_pid.and_then(probe); + match liveness(&holder, info) { + Liveness::Alive => LockState::Held(holder), + Liveness::Dead => LockState::Stale(holder), + Liveness::Unprovable => LockState::Unverified(holder), + } +} + +/// Reads the lock as it stands right now. +/// +/// Honours `HOSTLOCK_DIR` so a test can point somewhere harmless, exactly as +/// `hostlock.sh` does. +pub fn read() -> LockState { + if !cfg!(target_os = "linux") { + return LockState::Unknown; + } + let dir = std::env::var("HOSTLOCK_DIR").unwrap_or_else(|_| DEFAULT_LOCK_DIR.to_string()); + let meta_path = std::path::Path::new(&dir).join("meta"); + classify_io( + std::fs::read_to_string(&meta_path).map_err(|err| err.kind()), + proc_info, + ) +} + +/// Turns the result of reading the metadata file into a state. +/// +/// Separated from [`read`] only so the error arms are reachable from a test. +/// They are the arms most likely to be got wrong and least likely to be +/// exercised: an unreadable lock directory has to stay distinguishable from an +/// absent one, because "nobody holds it" and "I could not tell" are different +/// claims and only one of them permits a run. +pub fn classify_io( + meta: Result, + probe: impl Fn(u32) -> Option, +) -> LockState { + match meta { + Ok(meta) => classify(Some(&meta), probe), + // Absent is a measurement: nobody has taken the lock. Any other error is + // not, and must not be laundered into `Free`. + Err(std::io::ErrorKind::NotFound) => LockState::Free, + Err(_) => LockState::Unknown, + } +} + +/// Reduces two readings, taken at the two ends of a measured window, to the +/// `host_lock=` field. +/// +/// `self_owner` is `HOSTLOCK_OWNER` if the caller set it. Without it a held lock +/// can only be reported as [`Held`](LockField::Held): a reader cannot tell its +/// own declaration from a co-tenant's, and guessing would put a reassuring label +/// on precisely the rows that need a suspicious one. +pub fn field(before: &LockState, after: &LockState, self_owner: Option<&str>) -> LockField { + if before != after { + return LockField::Changed; + } + match before { + LockState::Unknown => LockField::Unknown, + LockState::Free => LockField::Free, + LockState::Stale(holder) => LockField::Stale(holder.owner.clone()), + LockState::Unverified(holder) => LockField::Unverified(holder.owner.clone()), + LockState::Held(holder) => match self_owner.map(sanitise_owner) { + Some(mine) if mine == holder.owner => LockField::Mine(holder.owner.clone()), + Some(_) => LockField::Foreign(holder.owner.clone()), + None => LockField::Held(holder.owner.clone()), + }, + } +} + +/// [`field`], with `HOSTLOCK_OWNER` read from the environment. +pub fn field_from_env(before: &LockState, after: &LockState) -> LockField { + let owner = std::env::var("HOSTLOCK_OWNER").ok(); + field(before, after, owner.as_deref()) +} + +/// What the holder said they were running, when there is one and both readings +/// agree on it. For a human reading a row later, not for any decision. +pub fn reason(before: &LockState, after: &LockState) -> Option { + let (a, b) = (before.holder()?, after.holder()?); + (a == b && !b.reason.is_empty() && b.reason != "?").then(|| b.reason.clone()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn meta(owner: &str, pid: &str) -> String { + format!("anchor_pid={pid}\nstart_time=900\nowner={owner}\nreason=acc0\nttl=0\n") + } + + fn running(start_time: u64) -> Option { + Some(ProcInfo { + state: 'S', + start_time, + threads: Some(4), + }) + } + + fn held(owner: &str) -> LockState { + LockState::Held(LockHolder { + owner: owner.to_string(), + anchor_pid: Some(1), + start_time: Some(900), + reason: "acc0".to_string(), + }) + } + + /// The exact string `hostlock.sh`'s header names as its own provenance + /// hazard. A row is a `key=value` line too, so it inherits the hazard. + #[test] + fn an_owner_cannot_splice_extra_fields_into_a_result_row() { + let holder = parse_meta(&meta("gaff hostlock_state=FREE declared=no", "1")) + .expect("owner line is present"); + assert!( + !holder.owner.contains('=') && !holder.owner.contains(' '), + "owner must not be able to forge a field boundary: {}", + holder.owner + ); + let rendered = field( + &LockState::Held(holder.clone()), + &LockState::Held(holder), + None, + ) + .to_string(); + assert_eq!( + rendered.split_whitespace().count(), + 1, + "the field must occupy exactly one token of the row: {rendered}" + ); + } + + /// A newline is worse than a space: it would end the row entirely. + #[test] + fn a_newline_in_an_owner_cannot_terminate_the_row() { + let holder = parse_meta("owner=roy\nanchor_pid=1\n").expect("owner line is present"); + assert_eq!(holder.owner, "roy"); + assert!( + !field( + &LockState::Held(holder.clone()), + &LockState::Held(holder), + None + ) + .to_string() + .contains('\n') + ); + } + + /// Never empty: `host_lock=held:` reads as a broken parser, not as a lock + /// held by nobody. + #[test] + fn an_unprintable_owner_is_named_rather_than_left_blank() { + assert_eq!(sanitise_owner(""), "?"); + assert_eq!(sanitise_owner(" "), "?"); + assert_eq!(sanitise_owner("==="), "?"); + } + + #[test] + fn a_long_owner_cannot_dominate_the_row() { + assert_eq!(sanitise_owner(&"x".repeat(200)).len(), MAX_OWNER_LEN); + } + + /// The reason this module reads twice. A single reading at the end would + /// have called this window `mine:sebastian` and looked entirely credible. + #[test] + fn a_window_that_changed_hands_is_never_reported_as_one_holder() { + assert_eq!( + field(&held("roy"), &held("sebastian"), Some("sebastian")), + LockField::Changed + ); + assert_eq!( + field(&LockState::Free, &held("sebastian"), Some("sebastian")), + LockField::Changed + ); + assert_eq!( + field(&held("sebastian"), &LockState::Free, Some("sebastian")), + LockField::Changed + ); + assert!( + !field(&LockState::Free, &held("sebastian"), Some("sebastian")).is_protected(), + "a window that only became ours partway through was not protected" + ); + } + + /// Held-by-us and held-by-someone-else are the two rows a reader most needs + /// to tell apart, and they are the same `LockState`. + #[test] + fn the_owner_decides_whether_a_held_lock_protects_this_row() { + assert_eq!( + field(&held("sebastian"), &held("sebastian"), Some("sebastian")), + LockField::Mine("sebastian".into()) + ); + assert_eq!( + field(&held("roy"), &held("roy"), Some("sebastian")), + LockField::Foreign("roy".into()) + ); + assert!(field(&held("sebastian"), &held("sebastian"), Some("sebastian")).is_protected()); + assert!(!field(&held("roy"), &held("roy"), Some("sebastian")).is_protected()); + } + + /// Without `HOSTLOCK_OWNER` the reader genuinely cannot attribute the lock, + /// and must not resolve that in the flattering direction. + #[test] + fn an_unattributable_lock_is_reported_as_unprotected() { + let f = field(&held("roy"), &held("roy"), None); + assert_eq!(f, LockField::Held("roy".into())); + assert!( + !f.is_protected(), + "an unattributed lock must never label a row as protected" + ); + } + + /// `Free` and `Unknown` format differently on purpose: one is a + /// measurement, the other is the absence of one. + #[test] + fn an_unreadable_lock_never_formats_as_a_free_one() { + assert_eq!( + field(&LockState::Free, &LockState::Free, None).to_string(), + "free" + ); + assert_eq!( + field(&LockState::Unknown, &LockState::Unknown, None).to_string(), + "unknown" + ); + assert!(!field(&LockState::Free, &LockState::Free, Some("me")).is_protected()); + } + + /// A dead anchor does not mean a quiet host: reaping a lock does not stop + /// the orphaned benchmark it was covering. + #[test] + fn a_departed_anchor_is_stale_rather_than_free() { + let state = classify(Some(&meta("roy", "424242")), |_| None); + let LockState::Stale(ref holder) = state else { + panic!("expected stale, got {state:?}"); + }; + assert_eq!(holder.owner, "roy"); + let f = field(&state, &state, Some("sebastian")); + assert_eq!(f, LockField::Stale("roy".into())); + assert!(!f.is_protected()); + } + + /// The defect the agreement test against `hostlock.sh` caught in this very + /// module: `/proc/` still exists for a zombie, so an existence check + /// reports a held lock on a corpse forever. The script calls this "the + /// common shape, not the exotic one" because every agent harness here + /// launches long commands without an immediate `wait()`. + #[test] + fn a_zombie_anchor_is_dead_despite_having_a_proc_entry() { + let state = classify(Some(&meta("roy", "7")), |_| { + Some(ProcInfo { + state: 'Z', + start_time: 900, + threads: Some(1), + }) + }); + assert!( + matches!(state, LockState::Stale(_)), + "a zombie anchor must not hold the box forever, got {state:?}" + ); + } + + /// ...but state `Z` is not proof of death. A thread-group leader that + /// exited via `pthread_exit` while its threads keep running reports `Z` for + /// a fully live process, and reaping that one takes a live holder's machine + /// mid-benchmark -- the worse of the two errors by a wide margin. + #[test] + fn a_zombie_leader_with_live_threads_is_not_treated_as_dead() { + let state = classify(Some(&meta("roy", "7")), |_| { + Some(ProcInfo { + state: 'Z', + start_time: 900, + threads: Some(6), + }) + }); + assert!(matches!(state, LockState::Held(_)), "got {state:?}"); + // No readable `Threads:` is no evidence of death, so it must not be + // read as one. There is deliberately no numeric default. + let state = classify(Some(&meta("roy", "7")), |_| { + Some(ProcInfo { + state: 'Z', + start_time: 900, + threads: None, + }) + }); + assert!(matches!(state, LockState::Held(_)), "got {state:?}"); + } + + /// The other case an existence check gets wrong: some unrelated process was + /// handed the same PID number, and the lock reads as current half an hour + /// after its owner exited. + #[test] + fn a_recycled_pid_is_not_mistaken_for_the_original_holder() { + let state = classify(Some(&meta("roy", "7")), |_| running(1234)); + assert!( + matches!(state, LockState::Stale(_)), + "a different process with the same pid is not the holder, got {state:?}" + ); + assert!(matches!( + classify(Some(&meta("roy", "7")), |_| running(900)), + LockState::Held(_) + )); + } + + /// An anchor that cannot be checked is unproven, not dead, and equally not + /// alive. Collapsing it either way is a decision made on absent evidence. + #[test] + fn an_unverifiable_anchor_is_neither_held_nor_stale() { + // Live pid, no recorded start time: the script's + // `unverifiable_live_anchor`. + let state = classify(Some("owner=roy\nanchor_pid=7\nreason=acc0\n"), |_| { + running(900) + }); + assert_eq!( + state, + LockState::Unverified(LockHolder { + owner: "roy".into(), + anchor_pid: Some(7), + start_time: None, + reason: "acc0".into(), + }) + ); + let f = field(&state, &state, Some("roy")); + assert_eq!(f, LockField::Unverified("roy".into())); + assert!( + !f.is_protected(), + "unproven liveness must not certify a row even when the owner matches" + ); + // No anchor at all is the same class of ignorance. + assert!(matches!( + classify(Some("owner=roy\n"), |_| running(900)), + LockState::Unverified(_) + )); + } + + /// A non-numeric anchor must not fall back to a PID that happens to exist. + #[test] + fn a_corrupt_anchor_is_not_rounded_to_a_live_pid() { + let holder = parse_meta("owner=roy\nanchor_pid=not-a-pid\n").expect("owner is present"); + assert_eq!(holder.anchor_pid, None); + let holder = parse_meta("owner=roy\nanchor_pid=7\nstart_time=nonsense\n").expect("present"); + assert_eq!(holder.start_time, None); + } + + /// A lock directory that cannot be read is not an empty one. Laundering a + /// permission error into `free` would put a clean label on every row taken + /// on a machine whose lock this process simply could not see. + #[test] + fn an_unreadable_lock_directory_is_not_reported_as_an_absent_one() { + use std::io::ErrorKind; + assert_eq!( + classify_io(Err(ErrorKind::NotFound), |_| running(900)), + LockState::Free + ); + for kind in [ + ErrorKind::PermissionDenied, + ErrorKind::IsADirectory, + ErrorKind::InvalidData, + ] { + assert_eq!( + classify_io(Err(kind), |_| running(900)), + LockState::Unknown, + "{kind:?} is not evidence that the lock is free" + ); + } + } + + /// Absent metadata is a measurement; unparseable metadata is not. + #[test] + fn a_half_written_lock_is_unknown_and_an_absent_one_is_free() { + assert_eq!(classify(None, |_| running(900)), LockState::Free); + assert_eq!( + classify(Some("acquired_at=now\n"), |_| running(900)), + LockState::Unknown + ); + } + + /// Agreeing with `hostlock.sh`'s `head -1` matters more than picking the + /// better rule, and only differs under the corrupt metadata where it counts. + #[test] + fn duplicate_keys_resolve_the_same_way_the_writer_resolves_them() { + let holder = parse_meta("owner=roy\nowner=sebastian\nanchor_pid=1\n").expect("present"); + assert_eq!(holder.owner, "roy"); + } + + /// A key must match whole, or `downstream_owner=x` would answer for `owner`. + #[test] + fn a_key_is_matched_at_the_start_of_a_line_only() { + assert_eq!( + meta_get("downstream_owner=x\nowner=roy\n", "owner"), + Some("roy") + ); + assert_eq!(meta_get("ownership=x\n", "owner"), None); + // `start_time` and `time` must not answer for one another either. + assert_eq!(meta_get("start_time=5\n", "time"), None); + } + + #[test] + fn a_reason_is_reported_only_when_both_readings_agree_on_a_holder() { + let a = held("sebastian"); + assert_eq!(reason(&a, &a).as_deref(), Some("acc0")); + assert_eq!(reason(&a, &held("roy")), None); + assert_eq!(reason(&LockState::Free, &LockState::Free), None); + } +} diff --git a/crates/onnx-runtime-hostmon/src/lib.rs b/crates/onnx-runtime-hostmon/src/lib.rs index 6b8bdd45f8..6b55b8adca 100644 --- a/crates/onnx-runtime-hostmon/src/lib.rs +++ b/crates/onnx-runtime-hostmon/src/lib.rs @@ -122,6 +122,18 @@ //! spawned wide and joined entirely between the two snapshots; see //! [`Contention::own_time_complete`] for why that is stated rather than //! sampled around. +//! +//! # Measured contention and declared intent are different questions +//! +//! Everything above measures what the host *did*. [`hostlock`] reads what +//! somebody *said they were doing* -- the advisory lock in `scripts/hostlock.sh` +//! that has existed on `main` with no in-tree consumer. Neither substitutes for +//! the other: an unlocked run on a genuinely idle box is fine, and a locked run +//! next to a co-tenant's unannounced `cargo test` is not. A row wants both, and +//! [`hostlock::field`] deliberately reads at both ends of the window so it +//! cannot report a single credible holder for a window that changed hands. + +pub mod hostlock; use std::time::Instant; diff --git a/crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs b/crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs new file mode 100644 index 0000000000..fa4116a369 --- /dev/null +++ b/crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs @@ -0,0 +1,256 @@ +//! Checks the reader against the writer, by running the real +//! `scripts/hostlock.sh`. +//! +//! The unit tests in `hostlock.rs` are all pure functions over strings I wrote +//! myself, so they prove the decision table is internally consistent and prove +//! nothing at all about whether it describes the file `hostlock.sh` actually +//! writes. A reader whose key names had drifted from the script would pass every +//! one of them and then report `free` on a locked host forever -- convincingly, +//! and in exactly the situation the field exists to catch. +//! +//! So this test does not parse a fixture. It shells out to the script, has it +//! take a lock, and asserts the Rust side sees what the shell side just did. +//! +//! # Why it fails rather than skips when the script is missing +//! +//! A skip would be the same defect one level up: the run would print `ok`, the +//! agreement would be unchecked, and the output would be indistinguishable from +//! a real pass. The script is committed at a fixed path in this repository, so +//! its absence means the reader's assumptions about that path are stale -- +//! which is precisely the thing worth failing on. The platform gate is +//! `#[cfg]`, so on a non-Linux target these tests do not exist rather than +//! passing vacuously. + +#![cfg(target_os = "linux")] + +use std::path::{Path, PathBuf}; +use std::process::Command; + +fn script() -> PathBuf { + let path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("../../scripts/hostlock.sh") + .canonicalize() + .expect( + "scripts/hostlock.sh must exist: this test's whole purpose is to agree with it, and \ + if it has moved then the reader's path assumptions have gone stale unnoticed", + ); + assert!(path.is_file(), "{} is not a file", path.display()); + path +} + +/// A lock directory under `CARGO_TARGET_TMPDIR`, so the test never touches the +/// real one. Sharing the default path would let a test release a lock a +/// colleague was relying on -- a test that can hand somebody else's machine +/// away is worse than no test. +fn lock_dir(name: &str) -> PathBuf { + let dir = PathBuf::from(env!("CARGO_TARGET_TMPDIR")).join(name); + let _ = std::fs::remove_dir_all(&dir); + dir +} + +fn hostlock(dir: &Path, args: &[&str]) -> (bool, String) { + let out = Command::new("bash") + .arg(script()) + .args(args) + .env("HOSTLOCK_DIR", dir) + .output() + .expect("failed to run scripts/hostlock.sh"); + let text = format!( + "{}{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + (out.status.success(), text) +} + +fn read_at(dir: &Path) -> onnx_runtime_hostmon::hostlock::LockState { + // Deliberately not `hostlock::read()`: that reads `HOSTLOCK_DIR` from the + // process environment, and a test that mutated it would leak the override + // into every other test in this binary on any failing assertion. Same file + // contents, same classifier, no shared mutable state. + let meta = std::fs::read_to_string(dir.join("meta")).ok(); + onnx_runtime_hostmon::hostlock::classify( + meta.as_deref(), + onnx_runtime_hostmon::hostlock::proc_info, + ) +} + +/// The load-bearing case: the script writes, the reader reads, and they agree +/// on who holds the lock, on when it stops being held, and on the fact that a +/// live holder's lock cannot be taken away. +#[test] +fn the_reader_sees_the_lock_the_script_just_took() { + use onnx_runtime_hostmon::hostlock::{LockField, LockState, field}; + + let dir = lock_dir("hostlock-agreement"); + + assert_eq!( + read_at(&dir), + LockState::Free, + "an absent lock directory must read as free, not as unknown" + ); + + // Anchored to a child this test owns and can kill. Anchoring to the + // script's own shell would make the lock stale the instant it returned, so + // the test would assert the staleness path while claiming to assert the + // held one. + let mut anchor = Command::new("sleep") + .arg("300") + .spawn() + .expect("spawn anchor"); + let anchor_pid = anchor.id(); + + let (ok, out) = hostlock( + &dir, + &[ + "acquire", + "--owner", + "sebastian", + "--reason", + "agreement-test", + "--pid", + &anchor_pid.to_string(), + ], + ); + assert!(ok, "acquire failed: {out}"); + + let held = read_at(&dir); + let LockState::Held(ref holder) = held else { + panic!("the reader must see the lock the script just wrote, got {held:?}"); + }; + assert_eq!( + holder.owner, "sebastian", + "owner key disagrees with the writer" + ); + assert_eq!( + holder.anchor_pid, + Some(anchor_pid), + "anchor_pid key disagrees with the writer" + ); + assert_eq!( + holder.reason, "agreement-test", + "reason key disagrees with the writer" + ); + assert!( + holder.start_time.is_some(), + "start_time must be read, or a recycled pid reads as the original holder" + ); + + assert_eq!( + field(&held, &held, Some("sebastian")), + LockField::Mine("sebastian".into()) + ); + assert_eq!( + field(&held, &held, Some("roy")), + LockField::Foreign("sebastian".into()), + "a lock held by someone else must never mark a row protected" + ); + + // The script refuses to release a lock whose anchor is still running. That + // refusal is the property that makes the lock worth reading at all, so it is + // asserted rather than stepped around with HOSTLOCK_FORCE. + let (ok, out) = hostlock(&dir, &["release", "--owner", "sebastian"]); + assert!( + !ok, + "a lock with a live anchor must not be releasable by a bystander: {out}" + ); + assert!( + matches!(read_at(&dir), LockState::Held(_)), + "a refused release must leave the lock exactly as it was" + ); + + anchor.kill().expect("kill anchor"); + anchor.wait().expect("reap anchor"); + + assert!( + matches!(read_at(&dir), LockState::Stale(_)), + "once the anchor is gone and reaped, the lock is stale" + ); + + let (ok, out) = hostlock(&dir, &["release", "--owner", "sebastian"]); + assert!(ok, "release of a dead-anchor lock failed: {out}"); + assert_eq!( + read_at(&dir), + LockState::Free, + "release must return the lock to free" + ); + + // A run spanning the release is the case a single end-of-window reading + // would have reported as a clean `mine:sebastian`. + assert_eq!( + field(&held, &read_at(&dir), Some("sebastian")), + LockField::Changed + ); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// A zombie anchor: `/proc/` still exists and its start time still +/// matches, so the obvious liveness check reports a held lock on a corpse +/// forever. This reader had exactly that defect until this test found it. +/// +/// `hostlock.sh` calls this the common shape rather than the exotic one, +/// because every agent harness on this box launches long commands without an +/// immediate `wait()`. It is reproduced here for real -- a spawned child that +/// is never reaped -- rather than simulated with a fabricated `ProcInfo`, so +/// that it also confirms `proc_info` parses a genuine `/proc//stat`. +#[test] +fn the_reader_and_the_script_agree_that_a_zombie_anchor_is_dead() { + use onnx_runtime_hostmon::hostlock::{LockField, LockState, field}; + + let dir = lock_dir("hostlock-zombie"); + + let mut corpse = Command::new("true").spawn().expect("spawn"); + let pid = corpse.id(); + // Wait for it to exit without reaping it, which is what makes it a zombie + // rather than a departed process. `try_wait` would reap it. + for _ in 0..500 { + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).unwrap_or_default(); + if stat + .rsplit(')') + .next() + .is_some_and(|r| r.trim_start().starts_with('Z')) + { + break; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + let stat = + std::fs::read_to_string(format!("/proc/{pid}/stat")).expect("zombie has a /proc entry"); + assert!( + stat.rsplit(')') + .next() + .is_some_and(|r| r.trim_start().starts_with('Z')), + "the fixture must actually be a zombie, or this test proves nothing: {stat}" + ); + + let (ok, out) = hostlock( + &dir, + &["acquire", "--owner", "roy", "--pid", &pid.to_string()], + ); + assert!(ok, "acquire failed: {out}"); + + let state = read_at(&dir); + let LockState::Stale(ref holder) = state else { + panic!("a zombie anchor must not hold the box forever, got {state:?}"); + }; + assert_eq!(holder.owner, "roy"); + + let f = field(&state, &state, Some("sebastian")); + assert_eq!(f, LockField::Stale("roy".into())); + assert!( + !f.is_protected(), + "a stale lock does not stop the orphaned load it was covering, so it must not certify a row" + ); + + // The script must reach the same verdict, otherwise a reaper and a + // published row would tell an operator two different stories about one lock. + let (_, status) = hostlock(&dir, &["status", "--porcelain"]); + assert!( + status.to_lowercase().contains("stale"), + "the script must call this lock stale too, said: {status}" + ); + + corpse.wait().expect("reap"); + let _ = std::fs::remove_dir_all(&dir); +} From 1898114f831f82d3a7b8c2c7ed23d5d9ecf41e56 Mon Sep 17 00:00:00 2001 From: Justin Chu Date: Mon, 24 Aug 2026 03:06:50 +0000 Subject: [PATCH 2/3] fix: three defects an independent review found in the lock reader 1. `decode_gap_park_ab` read the lock three times at the end -- once for the verdict, twice more for the reason -- so `lock_reason` could describe a different lock than `host_lock`. Printing `host_lock=changed lock_reason=acc0` names a holder for a window the field itself says had none, which invites a reader to dismiss the `changed`. One reading now feeds both. `bench_generic` already did this correctly. 2. Attribution compared the SANITISED owner, and sanitising is lossy in exactly the wrong direction: `sebastian!` and any 33-character name sharing a 32-character prefix both collapse onto an existing name, and a collision there turns `foreign` into `mine` and marks a contaminated row protected. `LockHolder` now carries `owner_raw` -- the owner as written, minus surrounding whitespace -- and the `mine` decision compares that. Display stays sanitised, because the row-forging hazard is real too. Display may be lossy; the protection decision may not. 3. The integration test leaked its `sleep 300` anchor and its scratch lock dir on any failing assertion, because cleanup ran after the asserts. Both are RAII guards now. The anchor burns no CPU so it would not corrupt anyone's measurement, but complaining about other agents' leaked processes while leaking one on every failed assertion is not a position worth defending. Re-smoked end to end after (2): with the lock held by `sebastian`, an HOSTLOCK_OWNER of `sebastian` gives `mine:sebastian` while `sebastian!` and `roy` both give `foreign:sebastian`. Mutation set extended to cover the new rule: 23 mutants, 23 killed, including one that reverts attribution to the sanitised owner and one that sanitises `owner_raw` at parse time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../benches/decode_gap_park_ab.rs | 11 ++- crates/onnx-runtime-hostmon/src/hostlock.rs | 48 +++++++++- .../tests/agrees_with_hostlock_sh.rs | 92 ++++++++++++++----- 3 files changed, 121 insertions(+), 30 deletions(-) diff --git a/crates/onnx-runtime-ep-cpu/benches/decode_gap_park_ab.rs b/crates/onnx-runtime-ep-cpu/benches/decode_gap_park_ab.rs index e812b9600b..cbd418d027 100644 --- a/crates/onnx-runtime-ep-cpu/benches/decode_gap_park_ab.rs +++ b/crates/onnx-runtime-ep-cpu/benches/decode_gap_park_ab.rs @@ -1035,9 +1035,14 @@ fn main() { // covers the whole window but proves nothing about load. An unlocked run on // a genuinely idle box is fine; a locked run beside somebody's unannounced // `cargo test` is not. - let lock = - host_contention::hostlock::field_from_env(&lock_before, &host_contention::hostlock::read()); - match host_contention::hostlock::reason(&lock_before, &host_contention::hostlock::read()) { + // One reading, used for both fields. Reading twice here would let the + // printed reason describe a different lock than the printed verdict -- + // `host_lock=changed lock_reason=acc0` names a holder for a window the + // field itself says had none, which invites a reader to dismiss the + // `changed`. + let lock_after = host_contention::hostlock::read(); + let lock = host_contention::hostlock::field_from_env(&lock_before, &lock_after); + match host_contention::hostlock::reason(&lock_before, &lock_after) { Some(reason) => println!("host_lock={lock} lock_reason={reason}"), None => println!("host_lock={lock}"), } diff --git a/crates/onnx-runtime-hostmon/src/hostlock.rs b/crates/onnx-runtime-hostmon/src/hostlock.rs index 9250adc7a7..66eff65388 100644 --- a/crates/onnx-runtime-hostmon/src/hostlock.rs +++ b/crates/onnx-runtime-hostmon/src/hostlock.rs @@ -55,8 +55,17 @@ const MAX_OWNER_LEN: usize = 32; /// Who claims the host, as recorded in the lock's metadata file. #[derive(Clone, Debug, PartialEq, Eq)] pub struct LockHolder { - /// Sanitised; see [`sanitise_owner`]. + /// Sanitised for printing; see [`sanitise_owner`]. pub owner: String, + /// The owner exactly as written, minus surrounding whitespace. + /// + /// Attribution compares this and never [`owner`](Self::owner), because + /// sanitising is lossy in the one direction that matters: it maps + /// `sebastian!` and any 33-character name sharing a 32-character prefix + /// onto an existing name, and a collision there turns `foreign` into + /// `mine` and marks a contaminated row protected. Display may be lossy; + /// the protection decision may not. + pub owner_raw: String, /// The PID whose liveness decides whether the lock is stale. `None` when the /// metadata did not carry one, which is itself a reason to distrust it. pub anchor_pid: Option, @@ -212,6 +221,7 @@ pub fn parse_meta(meta: &str) -> Option { let owner = meta_get(meta, "owner")?; Some(LockHolder { owner: sanitise_owner(owner), + owner_raw: owner.trim().to_string(), // A non-numeric or absent anchor is `None` rather than a default. There // is no safe default: 0 or 1 would both name a live process and label a // dead holder's lock as current. @@ -394,8 +404,8 @@ pub fn field(before: &LockState, after: &LockState, self_owner: Option<&str>) -> LockState::Free => LockField::Free, LockState::Stale(holder) => LockField::Stale(holder.owner.clone()), LockState::Unverified(holder) => LockField::Unverified(holder.owner.clone()), - LockState::Held(holder) => match self_owner.map(sanitise_owner) { - Some(mine) if mine == holder.owner => LockField::Mine(holder.owner.clone()), + LockState::Held(holder) => match self_owner.map(str::trim) { + Some(mine) if mine == holder.owner_raw => LockField::Mine(holder.owner.clone()), Some(_) => LockField::Foreign(holder.owner.clone()), None => LockField::Held(holder.owner.clone()), }, @@ -434,6 +444,7 @@ mod tests { fn held(owner: &str) -> LockState { LockState::Held(LockHolder { owner: owner.to_string(), + owner_raw: owner.to_string(), anchor_pid: Some(1), start_time: Some(900), reason: "acc0".to_string(), @@ -648,6 +659,7 @@ mod tests { state, LockState::Unverified(LockHolder { owner: "roy".into(), + owner_raw: "roy".into(), anchor_pid: Some(7), start_time: None, reason: "acc0".into(), @@ -666,6 +678,36 @@ mod tests { )); } + /// Sanitising is lossy, so it must not be what decides whether a lock is + /// ours. Both collisions below would otherwise promote another agent's + /// declaration to `mine` and mark a contaminated row protected -- the one + /// direction this module exists to prevent. + #[test] + fn a_name_that_merely_sanitises_to_ours_is_not_ours() { + let punctuated = parse_meta(&meta("sebastian!", "1")).expect("present"); + assert_eq!( + punctuated.owner, "sebastian", + "the display form is expected to collide; that is what makes this a hazard" + ); + let state = LockState::Held(punctuated); + assert_eq!( + field(&state, &state, Some("sebastian")), + LockField::Foreign("sebastian".into()), + "a different raw owner must stay foreign however it prints" + ); + assert!(!field(&state, &state, Some("sebastian")).is_protected()); + + // Two names that differ only past the display truncation. + let long = format!("{}a", "s".repeat(MAX_OWNER_LEN)); + let other = format!("{}b", "s".repeat(MAX_OWNER_LEN)); + let state = LockState::Held(parse_meta(&meta(&long, "1")).expect("present")); + assert!( + !field(&state, &state, Some(&other)).is_protected(), + "truncation must not make two distinct owners the same owner" + ); + assert!(field(&state, &state, Some(&long)).is_protected()); + } + /// A non-numeric anchor must not fall back to a PID that happens to exist. #[test] fn a_corrupt_anchor_is_not_rounded_to_a_live_pid() { diff --git a/crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs b/crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs index fa4116a369..ee9efd0b22 100644 --- a/crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs +++ b/crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs @@ -63,6 +63,55 @@ fn hostlock(dir: &Path, args: &[&str]) -> (bool, String) { (out.status.success(), text) } +/// Kills and reaps its child on unwind. +/// +/// Without this, a panic between spawning the anchor and killing it leaves a +/// `sleep 300` reparented to init for five minutes. It burns no CPU, so it +/// would not corrupt anyone's measurement -- but complaining about other +/// agents' leaked processes while leaking one on every failed assertion is not +/// a position worth defending, and the failing run is exactly when someone will +/// be looking at the process table. +struct Anchor(std::process::Child); + +impl Anchor { + fn spawn() -> Self { + Anchor( + Command::new("sleep") + .arg("300") + .spawn() + .expect("spawn anchor"), + ) + } + + fn pid(&self) -> u32 { + self.0.id() + } + + /// Ends the anchor and waits for it, so the PID names nothing afterwards + /// rather than a zombie. + fn retire(&mut self) { + let _ = self.0.kill(); + let _ = self.0.wait(); + } +} + +impl Drop for Anchor { + fn drop(&mut self) { + self.retire(); + } +} + +/// Removes a scratch lock directory on unwind. Never the real one: it is +/// constructed only by [`lock_dir`], which roots everything under +/// `CARGO_TARGET_TMPDIR`. +struct ScratchLock(PathBuf); + +impl Drop for ScratchLock { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + fn read_at(dir: &Path) -> onnx_runtime_hostmon::hostlock::LockState { // Deliberately not `hostlock::read()`: that reads `HOSTLOCK_DIR` from the // process environment, and a test that mutated it would leak the override @@ -82,10 +131,11 @@ fn read_at(dir: &Path) -> onnx_runtime_hostmon::hostlock::LockState { fn the_reader_sees_the_lock_the_script_just_took() { use onnx_runtime_hostmon::hostlock::{LockField, LockState, field}; - let dir = lock_dir("hostlock-agreement"); + let dir = ScratchLock(lock_dir("hostlock-agreement")); + let dir = &dir.0; assert_eq!( - read_at(&dir), + read_at(dir), LockState::Free, "an absent lock directory must read as free, not as unknown" ); @@ -94,14 +144,11 @@ fn the_reader_sees_the_lock_the_script_just_took() { // script's own shell would make the lock stale the instant it returned, so // the test would assert the staleness path while claiming to assert the // held one. - let mut anchor = Command::new("sleep") - .arg("300") - .spawn() - .expect("spawn anchor"); - let anchor_pid = anchor.id(); + let mut anchor = Anchor::spawn(); + let anchor_pid = anchor.pid(); let (ok, out) = hostlock( - &dir, + dir, &[ "acquire", "--owner", @@ -114,7 +161,7 @@ fn the_reader_sees_the_lock_the_script_just_took() { ); assert!(ok, "acquire failed: {out}"); - let held = read_at(&dir); + let held = read_at(dir); let LockState::Held(ref holder) = held else { panic!("the reader must see the lock the script just wrote, got {held:?}"); }; @@ -149,28 +196,27 @@ fn the_reader_sees_the_lock_the_script_just_took() { // The script refuses to release a lock whose anchor is still running. That // refusal is the property that makes the lock worth reading at all, so it is // asserted rather than stepped around with HOSTLOCK_FORCE. - let (ok, out) = hostlock(&dir, &["release", "--owner", "sebastian"]); + let (ok, out) = hostlock(dir, &["release", "--owner", "sebastian"]); assert!( !ok, "a lock with a live anchor must not be releasable by a bystander: {out}" ); assert!( - matches!(read_at(&dir), LockState::Held(_)), + matches!(read_at(dir), LockState::Held(_)), "a refused release must leave the lock exactly as it was" ); - anchor.kill().expect("kill anchor"); - anchor.wait().expect("reap anchor"); + anchor.retire(); assert!( - matches!(read_at(&dir), LockState::Stale(_)), + matches!(read_at(dir), LockState::Stale(_)), "once the anchor is gone and reaped, the lock is stale" ); - let (ok, out) = hostlock(&dir, &["release", "--owner", "sebastian"]); + let (ok, out) = hostlock(dir, &["release", "--owner", "sebastian"]); assert!(ok, "release of a dead-anchor lock failed: {out}"); assert_eq!( - read_at(&dir), + read_at(dir), LockState::Free, "release must return the lock to free" ); @@ -178,11 +224,9 @@ fn the_reader_sees_the_lock_the_script_just_took() { // A run spanning the release is the case a single end-of-window reading // would have reported as a clean `mine:sebastian`. assert_eq!( - field(&held, &read_at(&dir), Some("sebastian")), + field(&held, &read_at(dir), Some("sebastian")), LockField::Changed ); - - let _ = std::fs::remove_dir_all(&dir); } /// A zombie anchor: `/proc/` still exists and its start time still @@ -198,7 +242,8 @@ fn the_reader_sees_the_lock_the_script_just_took() { fn the_reader_and_the_script_agree_that_a_zombie_anchor_is_dead() { use onnx_runtime_hostmon::hostlock::{LockField, LockState, field}; - let dir = lock_dir("hostlock-zombie"); + let dir = ScratchLock(lock_dir("hostlock-zombie")); + let dir = &dir.0; let mut corpse = Command::new("true").spawn().expect("spawn"); let pid = corpse.id(); @@ -225,12 +270,12 @@ fn the_reader_and_the_script_agree_that_a_zombie_anchor_is_dead() { ); let (ok, out) = hostlock( - &dir, + dir, &["acquire", "--owner", "roy", "--pid", &pid.to_string()], ); assert!(ok, "acquire failed: {out}"); - let state = read_at(&dir); + let state = read_at(dir); let LockState::Stale(ref holder) = state else { panic!("a zombie anchor must not hold the box forever, got {state:?}"); }; @@ -245,12 +290,11 @@ fn the_reader_and_the_script_agree_that_a_zombie_anchor_is_dead() { // The script must reach the same verdict, otherwise a reaper and a // published row would tell an operator two different stories about one lock. - let (_, status) = hostlock(&dir, &["status", "--porcelain"]); + let (_, status) = hostlock(dir, &["status", "--porcelain"]); assert!( status.to_lowercase().contains("stale"), "the script must call this lock stale too, said: {status}" ); corpse.wait().expect("reap"); - let _ = std::fs::remove_dir_all(&dir); } From d02c76016d43d283497a68b47f5e133d308b1b72 Mon Sep 17 00:00:00 2001 From: Justin Chu Date: Mon, 24 Aug 2026 03:26:43 +0000 Subject: [PATCH 3/3] fix: two unnamed parties are not the same party, and document how to be named Writing the README section on how to make a row read `mine:` turned up a gap in the tool chain and then a defect in this reader. The gap: `hostlock.sh run` does not export `--owner`, so a child that inherits no `HOSTLOCK_OWNER` cannot recognise its own parent's lock and reports `held:`. That is the honest answer -- the flag is not visible to the child -- so the README now says to set the environment variable rather than only the flag. Deliberately still no fallback to `$USER`, even though the script has one. Every agent on this host runs as the same user, so `$USER` cannot distinguish one declaration from another; defaulting to it would report `mine:` for a co-tenant's lock, which is the one direction this module exists to prevent. The defect: writing the test for that rule failed on the first run. A holder whose owner is blank has an empty `owner_raw`, an empty `HOSTLOCK_OWNER` trims to the same, and the two compared equal -- so two unnamed parties matched each other and certified the row. An empty name on either side is the absence of an attribution, not an attribution to nobody. Guarded, and it now reports `foreign:?`. 24 mutants, 24 killed. The new `blank-owners-match` mutant deletes the guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/onnx-runtime-hostmon/src/hostlock.rs | 33 ++++++++++++++++++++ scripts/ort_ab/README.md | 34 +++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/crates/onnx-runtime-hostmon/src/hostlock.rs b/crates/onnx-runtime-hostmon/src/hostlock.rs index 66eff65388..237f5b7a53 100644 --- a/crates/onnx-runtime-hostmon/src/hostlock.rs +++ b/crates/onnx-runtime-hostmon/src/hostlock.rs @@ -405,6 +405,13 @@ pub fn field(before: &LockState, after: &LockState, self_owner: Option<&str>) -> LockState::Stale(holder) => LockField::Stale(holder.owner.clone()), LockState::Unverified(holder) => LockField::Unverified(holder.owner.clone()), LockState::Held(holder) => match self_owner.map(str::trim) { + // An empty name on either side is the absence of an attribution, + // not an attribution to nobody. Without this guard a holder whose + // owner is blank and a `HOSTLOCK_OWNER` that is blank compare equal + // and certify the row -- two unnamed parties matching each other. + Some(mine) if mine.is_empty() || holder.owner_raw.is_empty() => { + LockField::Foreign(holder.owner.clone()) + } Some(mine) if mine == holder.owner_raw => LockField::Mine(holder.owner.clone()), Some(_) => LockField::Foreign(holder.owner.clone()), None => LockField::Held(holder.owner.clone()), @@ -413,6 +420,15 @@ pub fn field(before: &LockState, after: &LockState, self_owner: Option<&str>) -> } /// [`field`], with `HOSTLOCK_OWNER` read from the environment. +/// +/// There is deliberately **no fallback to `$USER`**, even though +/// `hostlock.sh`'s `--owner` has one. Every agent on this host runs as the same +/// user, so `$USER` cannot distinguish one declaration from another, and +/// defaulting to it would report `mine:` for a co-tenant's lock -- the one +/// direction this module exists to prevent. Note also that `hostlock.sh run` +/// does not export `--owner`, so passing the flag alone leaves a child unable +/// to recognise its own parent's lock; that reports [`Held`](LockField::Held), +/// which is unattributed and unprotected, and is the honest answer. pub fn field_from_env(before: &LockState, after: &LockState) -> LockField { let owner = std::env::var("HOSTLOCK_OWNER").ok(); field(before, after, owner.as_deref()) @@ -708,6 +724,23 @@ mod tests { assert!(field(&state, &state, Some(&long)).is_protected()); } + /// Every agent on this host runs as the same user, so a `$USER` fallback + /// would hand one agent another's declaration. Absent attribution has to + /// stay absent. + #[test] + fn an_absent_owner_is_never_filled_in_from_somewhere_else() { + let f = field(&held("roy"), &held("roy"), None); + assert!(!f.is_protected()); + assert_eq!(f, LockField::Held("roy".into())); + // An empty or whitespace-only HOSTLOCK_OWNER is not an attribution + // either, and must not match a holder whose owner sanitises to "?". + let blank = LockState::Held(parse_meta(&meta(" ", "1")).expect("present")); + assert!( + !field(&blank, &blank, Some(" ")).is_protected(), + "two unnamed parties are not the same party" + ); + } + /// A non-numeric anchor must not fall back to a PID that happens to exist. #[test] fn a_corrupt_anchor_is_not_rounded_to_a_live_pid() { diff --git a/scripts/ort_ab/README.md b/scripts/ort_ab/README.md index ade11b84db..07bb2d213e 100644 --- a/scripts/ort_ab/README.md +++ b/scripts/ort_ab/README.md @@ -269,6 +269,40 @@ on the 1-minute load average: `loadavg` is an exponential moving average, so it stays high for a minute after a heavy run has ended and reads low while a burst is still in flight. It misleads in both directions. +The Rust benchmarks read the lock and put the answer in the row. `bench_generic` +and the `decode_gap_park_ab` matrix emit a `host_lock=` field covering the +**whole measured window** -- read before the first run and again after the last, +so a run that changed hands halfway through prints `changed` rather than naming +whichever holder happened to be there at the end: + +| value | meaning | +|---|---| +| `mine:` | held throughout by a live anchor matching `HOSTLOCK_OWNER` -- the only value that certifies the row | +| `foreign:` / `held:` | held by someone else, or by an owner that cannot be attributed because `HOSTLOCK_OWNER` was unset | +| `unverified:` / `stale:` | held by an anchor whose liveness is unprovable, or provably gone | +| `changed` | the window spans a change of custody; no single holder describes it | +| `free` / `unknown` | nobody declared the host, or the lock could not be read -- deliberately not the same value | + +Set `HOSTLOCK_OWNER` in the **environment**, not just `--owner` on the command +line: `run` does not export the flag, so a child that inherits neither cannot +tell your lock from a co-tenant's and reports `held:` rather than `mine:`. +`HOSTLOCK_OWNER=leon scripts/hostlock.sh run --reason "..." -- ...` sets both at +once, since `--owner` defaults to it. + +There is deliberately no fallback to `$USER`, even though the script uses one. +Every agent on this host runs as the same user, so `$USER` cannot distinguish +one declaration from another -- defaulting to it would report `mine:` for +somebody else's lock, which is the one direction this field exists to prevent. +An unset `HOSTLOCK_OWNER` genuinely cannot be attributed, and says so. + +This is orthogonal to the `foreign_%` / `sib_%` columns beside it and does not +replace them: those measure what the host *did*, `host_lock` records what +somebody *said they were doing*. Contention sampling reads instants and can +miss a co-tenant that starts and finishes between two snapshots; a declaration +covers the whole window but proves nothing about load. An unlocked run on a +genuinely idle box is fine, and a locked run beside somebody's unannounced +`cargo test` is not. + The lock is advisory. It cannot stop anyone from using the cores and does not try to; it makes "is somebody benchmarking right now, and who?" cheap enough to check that there is no excuse for not checking. Record the runnable count you