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..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 @@ -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,38 @@ 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. + // 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}"), + } + 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..237f5b7a53 --- /dev/null +++ b/crates/onnx-runtime-hostmon/src/hostlock.rs @@ -0,0 +1,813 @@ +//! 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 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, + /// 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), + 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. + 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(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()), + }, + } +} + +/// [`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()) +} + +/// 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(), + owner_raw: 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(), + owner_raw: "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(_) + )); + } + + /// 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()); + } + + /// 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() { + 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..ee9efd0b22 --- /dev/null +++ b/crates/onnx-runtime-hostmon/tests/agrees_with_hostlock_sh.rs @@ -0,0 +1,300 @@ +//! 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) +} + +/// 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 + // 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 = ScratchLock(lock_dir("hostlock-agreement")); + let dir = &dir.0; + + 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 = Anchor::spawn(); + let anchor_pid = anchor.pid(); + + 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.retire(); + + 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 + ); +} + +/// 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 = ScratchLock(lock_dir("hostlock-zombie")); + let dir = &dir.0; + + 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"); +} 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