diff --git a/hive-c0re/src/main.rs b/hive-c0re/src/main.rs index 9515f47c..9efe1d51 100644 --- a/hive-c0re/src/main.rs +++ b/hive-c0re/src/main.rs @@ -36,6 +36,8 @@ mod socket_server; mod stats; mod stores; mod swarm_status; +#[cfg(test)] +mod test_env; mod webhook_secret; mod workers; diff --git a/hive-c0re/src/meta.rs b/hive-c0re/src/meta.rs index a17d0186..b3f220a6 100644 --- a/hive-c0re/src/meta.rs +++ b/hive-c0re/src/meta.rs @@ -2222,8 +2222,12 @@ mod tests { // no endpoint signal, no hyperhive.otel lines are emitted (agents // keep the the harness modules disabled default). // - // SAFETY: single-threaded mutation of process env vars no other - // test asserts on; restored before returning. + // Serialised against every other env-mutating test in the crate. + // `stats::otel_metrics::tests` perturbs HYPERHIVE_OTEL_ENDPOINT too, + // and lands in this same test binary — "no other test asserts on + // these" was true of this module and false of the process. + let _env = crate::test_env::lock(); + // SAFETY: serialised by the guard above; restored before returning. let render = || { render_flake( "github:example/hyperhive", diff --git a/hive-c0re/src/stats/otel_metrics.rs b/hive-c0re/src/stats/otel_metrics.rs index 0adf3f6a..24f012f1 100644 --- a/hive-c0re/src/stats/otel_metrics.rs +++ b/hive-c0re/src/stats/otel_metrics.rs @@ -377,8 +377,14 @@ mod tests { /// it resumes posting to a base URL that 404s in silence. #[test] fn endpoint_is_the_standard_otlp_var() { - // SAFETY: single-threaded mutation of a process env var no other test - // in this module asserts on; both names are cleared before returning. + // Serialised against every other env-mutating test in the crate. Both + // variables below are also read by `meta::tests` in this same test + // binary, so "no other test in this module" would be the wrong + // boundary — the module is not the unit that shares the environment, + // the process is. + let _env = crate::test_env::lock(); + // SAFETY: serialised by the guard above; both names are restored (to + // absent) before it drops at the end of this test. unsafe { std::env::remove_var("OTEL_EXPORTER_OTLP_ENDPOINT"); std::env::set_var("HYPERHIVE_OTEL_ENDPOINT", "http://hyperhive.invalid:4318"); diff --git a/hive-c0re/src/test_env.rs b/hive-c0re/src/test_env.rs new file mode 100644 index 00000000..3e5a95bb --- /dev/null +++ b/hive-c0re/src/test_env.rs @@ -0,0 +1,39 @@ +//! Test-only: the crate's single lock for tests that mutate process +//! environment variables. +//! +//! Environment variables are one process-global, and every `#[test]` in this +//! crate lands in the *same* test binary, run on parallel threads. A test that +//! sets a variable, asserts, then restores it is only safe against a *concurrent* +//! test if both take the same lock — so any test here that touches the +//! environment must route through [`lock`] rather than rolling its own +//! save/restore. +//! +//! ⚠️ **The lock has to be crate-wide, not per-module.** `hive-bash-mcp`'s +//! sibling helper records why: two separate per-module mutexes serialise +//! nothing against each other, and that produced a CI-only flake. The pair that +//! motivated this one is `meta::tests` (which renders a flake from +//! `HYPERHIVE_OTEL_*`) and `stats::otel_metrics::tests` (which asserts which +//! variable enables the exporter) — different modules, same variable, same +//! binary. +//! +//! ⚠️ **A green local run is weak evidence for this class.** An agent container +//! has the hyperhive variables ambient-set, so a losing race still finds a +//! plausible value; the nix sandbox strips them, so there the race can delete a +//! variable out from under another thread. Reproduce the sandbox shape with +//! `env -u HYPERHIVE_OTEL_ENDPOINT cargo test -p hive-c0re`. + +use std::sync::{Mutex, MutexGuard, PoisonError}; + +static ENV_LOCK: Mutex<()> = Mutex::new(()); + +/// Serialise this test against every other environment-mutating test in the +/// crate. Hold the returned guard for as long as the variables are perturbed — +/// bind it (`let _env = lock();`), never discard it with `let _ = lock();`, +/// which drops the guard immediately and serialises nothing. +/// +/// Recovers from poisoning: a test that panicked mid-mutation has already +/// failed and reported, and refusing to run every later test on top of that +/// turns one failure into a cascade that hides which test actually broke. +pub fn lock() -> MutexGuard<'static, ()> { + ENV_LOCK.lock().unwrap_or_else(PoisonError::into_inner) +}