From 5a0b6ca907437c8f1e9b27ce5a98e065192f0179 Mon Sep 17 00:00:00 2001 From: iris Date: Fri, 29 May 2026 19:13:01 +0200 Subject: [PATCH] identity: serialise env-mutating tests on a module mutex (damocles #595 nit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cargo's default test runner parallelises tests within a binary, so the original 'tests run serially' comment was wrong — two `with_env` calls running concurrently would race the process-wide HIVE_LABEL / HYPERHIVE_HIVE_DOMAIN state. Added a module-scope `static ENV_LOCK: Mutex<()>` and acquire it at the top of `with_env` so each set / run / restore window is exclusive. Poison recovery via `unwrap_or_else(into_inner)` so a single test panic doesn't cascade through the rest of the module. Lighter than pulling in serial_test for one module. No new deps. --- hive-ag3nt/src/identity.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/hive-ag3nt/src/identity.rs b/hive-ag3nt/src/identity.rs index e49b0cee..0bc4ce51 100644 --- a/hive-ag3nt/src/identity.rs +++ b/hive-ag3nt/src/identity.rs @@ -65,14 +65,27 @@ pub fn qualify(label: &str) -> String { #[cfg(test)] mod tests { use super::*; + use std::sync::Mutex; + + /// Per damocles's #595 review: cargo's test runner parallelises by + /// default, so a `with_env` helper that mutates process-wide env vars + /// races between tests in this module. Serialise on a module-scope + /// mutex so each `with_env` call holds the lock for its set / run / + /// restore window. Cheap (each test body is microseconds) and avoids + /// pulling in `serial_test` for just one module. + static ENV_LOCK: Mutex<()> = Mutex::new(()); /// Helper: run `f` with a clean env, restoring previous values on exit. - /// Tests run serially in this module (no `#[parallel]`) to avoid the - /// process-wide env var contention. + /// Acquires `ENV_LOCK` first so concurrent tests don't race the env-var + /// state. If a previous test panicked while holding the lock the + /// mutex would be poisoned — we use `lock().unwrap_or_else(|e| e.into_inner())` + /// to recover so a single test failure doesn't cascade through the + /// whole module. fn with_env(label: Option<&str>, domain: Option<&str>, f: F) { + let _guard = ENV_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); let prev_label = env::var("HIVE_LABEL").ok(); let prev_domain = env::var("HYPERHIVE_HIVE_DOMAIN").ok(); - // SAFETY: tests are single-threaded; we restore in the same scope. + // SAFETY: serialised by ENV_LOCK above; restore in the same scope. unsafe { match label { Some(v) => env::set_var("HIVE_LABEL", v),