From fb156803ed6816eb42478dc8675b02b79e0a3a40 Mon Sep 17 00:00:00 2001 From: Paavo Pokkinen Date: Tue, 22 Sep 2026 11:22:21 +0300 Subject: [PATCH] fix(tests): serialize all env-touching tests on the crate-wide mutex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cargo test --lib` failed reliably on a 14-core machine: 20 of 352 tests failed, all cascading from a single race. Three modules opted out of the crate-wide lock in [lib.rs](src/lib.rs#L21). `client` and `secrets` each declared their own module-private `ENV_MUTEX`, so their guards serialized only against tests in the same module. The 17 `client` tests that set `HOME` therefore ran concurrently with the `config`, `run` and `sandbox` tests that the crate-wide mutex serializes; `config::tests::agent_filesystem_paths_resolved` read another test's `HOME`, panicked while holding the crate-wide mutex, and poisoned it for the 19 further tests that `unwrap()` that lock. Four `exec` tests took no lock at all. They compare `build_env()`'s snapshot of the environment against a second `std::env::var` read of the same variable, and `ESSENTIAL_VARS` covers `HOME`, `LANG`, `LC_ALL` and `TZ` — all mutated by `run` tests, so the value could change between the two reads. With every env-touching test on one mutex, 40 consecutive parallel runs pass, so the Nix package no longer needs `dontUseCargoParallelTests`. Claude-Session: https://claude.ai/code/session_01E3R8fS7y8zYjoEzjorYTBJ --- flake.nix | 4 ---- src/client.rs | 17 +++++++++++------ src/exec.rs | 17 +++++++++++++++++ src/secrets.rs | 27 ++++++++++++++------------- 4 files changed, 42 insertions(+), 23 deletions(-) diff --git a/flake.nix b/flake.nix index e3e5ea7..532e30c 100644 --- a/flake.nix +++ b/flake.nix @@ -44,10 +44,6 @@ # an OS sandbox inside the Nix build sandbox, which is unavailable there. cargoTestFlags = [ "--lib" ]; - # Several unit tests mutate HOME and fail when run in parallel - # (reproducible with plain `cargo test --lib` on a many-core machine). - dontUseCargoParallelTests = true; - meta = { description = "Credential broker for AI agents — tools get your secrets, the agent never does"; homepage = "https://github.com/ModernPath/airlock"; diff --git a/src/client.rs b/src/client.rs index 7d6515a..6f9d12f 100644 --- a/src/client.rs +++ b/src/client.rs @@ -424,12 +424,12 @@ mod tests { // ── Helpers ────────────────────────────────────────────────────────── - use std::sync::{Mutex, MutexGuard}; + use std::sync::MutexGuard; - /// Global mutex serializing tests that modify env vars. - static ENV_MUTEX: Mutex<()> = Mutex::new(()); - - /// RAII guard for temporary environment variable overrides. + /// RAII guard for temporary environment variable overrides. Holds + /// [`crate::test_support::ENV_MUTEX`] — the crate-wide lock — so these + /// tests serialize against every other test that touches the process + /// environment, not just the ones in this module. struct TempEnvVar { key: String, prev: Option, @@ -438,8 +438,13 @@ mod tests { impl TempEnvVar { fn new(key: &str, value: &str) -> Self { - let lock = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + let lock = crate::test_support::ENV_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); let prev = std::env::var(key).ok(); + // SAFETY: we hold the crate-wide ENV_MUTEX, so no other test + // thread anywhere in the suite is reading or writing env vars + // concurrently. unsafe { std::env::set_var(key, value) }; Self { key: key.to_string(), diff --git a/src/exec.rs b/src/exec.rs index 2336cde..baf14e5 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -587,6 +587,15 @@ mod tests { use std::time::Duration; use super::*; + use crate::test_support::ENV_MUTEX; + + /// Hold the crate-wide env lock. The tests below compare `build_env()`'s + /// snapshot of the environment against a second `std::env::var` read of the + /// same variable; without the lock a concurrent test mutating `HOME` or + /// `LANG` can change the answer between the two reads. + fn env_lock() -> std::sync::MutexGuard<'static, ()> { + ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()) + } // ── Binary resolution ──────────────────────────────────────────────────── @@ -704,6 +713,8 @@ mod tests { /// The output map contains `PATH` with the value from the daemon's environment. #[test] fn build_env_contains_path_from_daemon_env() { + let _guard = env_lock(); + // PATH is almost certainly set in any test environment; guard anyway. let Ok(expected_path) = std::env::var("PATH") else { // PATH is not set — nothing to verify. @@ -722,6 +733,8 @@ mod tests { /// in the daemon's environment. #[test] fn build_env_contains_essential_vars_when_present_in_daemon_env() { + let _guard = env_lock(); + let env = build_env(&[]); for var in &["HOME", "TERM", "LANG", "USER"] { @@ -752,6 +765,8 @@ mod tests { /// 2. Every key is either a declared secret or an essential variable. #[test] fn build_env_has_exactly_secrets_plus_present_essential_vars() { + let _guard = env_lock(); + let secrets = vec![ ("SECRET_ALPHA".to_string(), "value_a".to_string()), ("SECRET_BETA".to_string(), "value_b".to_string()), @@ -837,6 +852,8 @@ mod tests { /// not appear. #[test] fn build_env_omits_absent_essential_vars_without_error() { + let _guard = env_lock(); + let env = build_env(&[]); match std::env::var("TERM") { diff --git a/src/secrets.rs b/src/secrets.rs index 3e2859c..e075ea8 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -379,22 +379,18 @@ mod tests { use super::*; use std::collections::HashMap; use std::path::PathBuf; - use std::sync::{Mutex, MutexGuard}; + use std::sync::MutexGuard; use std::time::Duration; use crate::config::{Config, SecretSpec}; // ── Helpers ────────────────────────────────────────────────────────── - /// Global mutex that serializes all tests that modify environment variables. - /// - /// Environment variables are process-global state. Without serialization, - /// concurrent tests that modify env vars race against each other, producing - /// flaky failures. - static ENV_MUTEX: Mutex<()> = Mutex::new(()); - /// RAII guard that sets environment variables for the duration of a test - /// and restores them when dropped. Also holds the [`ENV_MUTEX`] lock. + /// and restores them when dropped. Holds + /// [`crate::test_support::ENV_MUTEX`] — the crate-wide lock — so these + /// tests serialize against every other test that touches the process + /// environment, not just the ones in this module. struct EnvGuard { vars: Vec<(String, Option)>, _lock: MutexGuard<'static, ()>, @@ -404,14 +400,17 @@ mod tests { /// Set the given environment variables, saving their previous values /// for restoration on drop. fn new(vars: &[(&str, &str)]) -> Self { - let lock = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + let lock = crate::test_support::ENV_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); let mut saved = Vec::with_capacity(vars.len()); for (key, value) in vars { let prev = std::env::var(*key).ok(); saved.push((key.to_string(), prev)); - // SAFETY: We hold ENV_MUTEX, so no other test thread is reading - // or writing env vars concurrently within this test module. + // SAFETY: we hold the crate-wide ENV_MUTEX, so no other test + // thread anywhere in the suite is reading or writing env vars + // concurrently. unsafe { std::env::set_var(*key, *value) }; } @@ -424,7 +423,9 @@ mod tests { /// Acquire the env mutex without setting any variables. Useful when /// we need to set and clear in a specific order within the test body. fn lock_only() -> Self { - let lock = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); + let lock = crate::test_support::ENV_MUTEX + .lock() + .unwrap_or_else(|e| e.into_inner()); Self { vars: Vec::new(), _lock: lock,