test(hive-c0re): one crate-wide lock for env-mutating tests

Review finding from argus. The new endpoint test carried a SAFETY comment
claiming no other test in its module asserts on the variables it perturbs —
the wrong boundary. The module is not the unit that shares the environment,
the process is: meta.rs's render_flake_injects_otel_when_signalled mutates
the same HYPERHIVE_OTEL_ENDPOINT, both land in the one hive-c0re test binary,
and cargo runs it at default parallelism with no serialisation anywhere in
the crate. Each test independently claimed exclusive ownership of shared
global state, which is the instrument-that-looks-solid class the endpoint
change's own gate reasoning warns about.

Adds test_env with a single ENV_LOCK, taken by both. No new dependency: this
is the pattern hive-bash-mcp and hive-agent already use, and hive-bash-mcp's
helper records why it has to be crate-wide rather than per-module — two
per-module mutexes serialise nothing against each other, which produced a
CI-only flake there.

The asymmetry that makes this hard to see locally is worth stating: an agent
container has the hyperhive variables ambient-set, so a losing race still
finds a plausible value and the test passes; the nix sandbox strips them, so
only there can one thread delete a variable out from under another. Verified
in that shape with `env -u HYPERHIVE_OTEL_ENDPOINT -u
OTEL_EXPORTER_OTLP_ENDPOINT`, five consecutive runs green — a sanity check,
not a proof, since a race cannot be shown absent by running. What makes it
correct is structural: both tests take the same lock.

Deliberately scoped to the pair that overlaps. meta.rs has three further
env-mutating tests (HIVE_FORGE_URL twice, the TLS CA pair) that race with
each other, untouched here and tracked separately, because the fix is not
the mechanical one it looks like: std::sync::Mutex is not reentrant, so
adding a lock to a test whose helpers also lock deadlocks. That needs
reading per test rather than a sweep.
This commit is contained in:
atlas 2026-08-19 00:42:26 +02:00 committed by mara
commit af3a9e5433
4 changed files with 55 additions and 4 deletions

View file

@ -36,6 +36,8 @@ mod socket_server;
mod stats;
mod stores;
mod swarm_status;
#[cfg(test)]
mod test_env;
mod webhook_secret;
mod workers;

View file

@ -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",

View file

@ -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");

39
hive-c0re/src/test_env.rs Normal file
View file

@ -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)
}