fix(tests): eliminate env mutex poison cascade (#1558)

* fix(tests): eliminate env mutex poison cascade and fix test flakiness

The shared ENV_MUTEX used by ~68 config tests would cascade a single
test panic into failures across every module. Replace all .unwrap() /
.expect() lock acquisitions with a poison-recovering lock_env() helper.
Consolidate rogue module-local ENV_LOCK instances (workspace, orchestrator,
bootstrap) onto the shared global mutex to prevent cross-module races.

Also fixes:
- gateway user_id fallback was hardcoded to "default" instead of owner_id
- test_ironclaw_env_path used LazyLock which is order-dependent

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* test(helpers): add regression test for lock_env poison recovery

Satisfies the regression-test-check CI gate by adding a test that
intentionally poisons ENV_MUTEX and verifies lock_env() recovers.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix(ci): detect test changes inside #[cfg(test)] regions

The regression test check relied on git diff -W to expand context to
function boundaries, but git doesn't recognize Rust `mod tests {}` as a
function boundary. Changes to imports, helpers, or lock calls inside
test modules were invisible to the check.

Add a line-level fallback: for each changed .rs file, find where
#[cfg(test)] starts and check if any diff hunk targets a line at or
after that boundary. This catches edits anywhere inside test modules
regardless of git's language awareness.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address PR review feedback

- Clear ENV_MUTEX poison after regression test so it doesn't leave
  global state dirty for subsequent tests.
- Fix CI regression-test-check to match #[cfg(test)] only when followed
  by `mod` (the test module pattern), avoiding false positives from
  standalone #[cfg(test)] items like statics or functions.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
Illia Polosukhin
2026-03-22 14:36:24 -07:00
committed by GitHub
co-authored by Claude Opus 4.6
parent fbce9a5fe3
commit 3aa36c8f55
19 changed files with 192 additions and 143 deletions
+3 -3
View File
@@ -63,12 +63,12 @@ impl BuilderModeConfig {
#[cfg(test)]
mod tests {
use super::*;
use crate::config::helpers::ENV_MUTEX;
use crate::config::helpers::lock_env;
use crate::settings::Settings;
#[test]
fn resolve_falls_back_to_settings() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
let mut settings = Settings::default();
settings.builder.max_iterations = 99;
settings.builder.auto_register = false;
@@ -80,7 +80,7 @@ mod tests {
#[test]
fn env_overrides_settings() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
let mut settings = Settings::default();
settings.builder.timeout_secs = 123;
+3 -3
View File
@@ -113,7 +113,7 @@ impl ChannelsConfig {
let gateway = if gateway_enabled {
let user_id = optional_env("GATEWAY_USER_ID")?
.or_else(|| cs.gateway_user_id.clone())
.unwrap_or_else(|| "default".to_string());
.unwrap_or_else(|| owner_id.to_string());
Some(GatewayConfig {
host: optional_env("GATEWAY_HOST")?
@@ -236,7 +236,7 @@ fn default_channels_dir() -> PathBuf {
#[cfg(test)]
mod tests {
use crate::config::channels::*;
use crate::config::helpers::ENV_MUTEX;
use crate::config::helpers::lock_env;
use crate::settings::Settings;
#[test]
@@ -395,7 +395,7 @@ mod tests {
#[test]
fn resolve_uses_settings_channel_values_with_owner_scope_user_ids() {
let _guard = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner());
let _guard = lock_env();
let mut settings = Settings::default();
settings.channels.http_enabled = true;
settings.channels.http_host = Some("127.0.0.2".to_string());
+7 -7
View File
@@ -196,7 +196,7 @@ impl EmbeddingsConfig {
#[cfg(test)]
mod tests {
use super::*;
use crate::config::helpers::ENV_MUTEX;
use crate::config::helpers::lock_env;
use crate::settings::{EmbeddingsSettings, Settings};
use crate::testing::credentials::*;
@@ -215,7 +215,7 @@ mod tests {
#[test]
fn embeddings_disabled_not_overridden_by_openai_key() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_embedding_env();
// SAFETY: Under ENV_MUTEX, no concurrent env access.
unsafe {
@@ -245,7 +245,7 @@ mod tests {
#[test]
fn embeddings_enabled_from_settings() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_embedding_env();
let settings = Settings {
@@ -265,7 +265,7 @@ mod tests {
#[test]
fn embeddings_env_override_takes_precedence() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_embedding_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -294,7 +294,7 @@ mod tests {
#[test]
fn embedding_base_url_parsed_from_env() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_embedding_env();
// SAFETY: Under ENV_MUTEX, no concurrent env access.
@@ -313,7 +313,7 @@ mod tests {
#[test]
fn embedding_base_url_defaults_to_none() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_embedding_env();
let settings = Settings::default();
@@ -326,7 +326,7 @@ mod tests {
#[test]
fn cache_size_zero_rejected() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_embedding_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
+31 -1
View File
@@ -14,6 +14,16 @@ use crate::config::INJECTED_VARS;
#[cfg(test)]
pub(crate) static ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(());
/// Acquire the env-var mutex, recovering from poison.
///
/// A poisoned mutex means a previous test panicked while holding the lock.
/// The env state might be slightly stale, but cascading every subsequent
/// test into a `PoisonError` panic is far worse. Recover and carry on.
#[cfg(test)]
pub(crate) fn lock_env() -> std::sync::MutexGuard<'static, ()> {
ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner())
}
/// Thread-safe mutable overlay for env vars set at runtime.
///
/// Unlike `INJECTED_VARS` (which is set once at startup from the secrets
@@ -353,7 +363,7 @@ mod tests {
#[test]
fn real_env_var_takes_priority_over_runtime_override() {
let _guard = ENV_MUTEX.lock().unwrap();
let _guard = lock_env();
let key = "IRONCLAW_TEST_ENV_PRIORITY_42";
// Set runtime override
@@ -372,6 +382,26 @@ mod tests {
assert_eq!(env_or_override(key), Some("override_value".to_string()));
}
// --- lock_env poison recovery (regression for env mutex cascade) ---
#[test]
fn lock_env_recovers_from_poisoned_mutex() {
// Simulate a poisoned mutex: spawn a thread that panics while holding the lock.
let _ = std::thread::spawn(|| {
let _guard = ENV_MUTEX.lock().unwrap();
panic!("intentional poison");
})
.join();
// The mutex is now poisoned. lock_env() should recover, not cascade.
assert!(ENV_MUTEX.lock().is_err(), "mutex should be poisoned");
let _guard = lock_env(); // must not panic
drop(_guard);
// Clean up so this test doesn't leave ENV_MUTEX permanently poisoned.
ENV_MUTEX.clear_poison();
}
// --- validate_base_url tests (regression for #1103) ---
#[test]
+26 -26
View File
@@ -532,7 +532,7 @@ pub fn default_session_path() -> PathBuf {
#[cfg(test)]
mod tests {
use super::*;
use crate::config::helpers::ENV_MUTEX;
use crate::config::helpers::lock_env;
use crate::settings::Settings;
use crate::testing::credentials::*;
@@ -548,7 +548,7 @@ mod tests {
#[test]
fn openai_compatible_uses_selected_model_when_llm_model_unset() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_compatible_env();
let settings = Settings {
@@ -566,7 +566,7 @@ mod tests {
#[test]
fn openai_compatible_llm_model_env_overrides_selected_model() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_compatible_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -690,7 +690,7 @@ mod tests {
#[test]
fn ollama_uses_selected_model_when_ollama_model_unset() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_ollama_env();
let settings = Settings {
@@ -707,7 +707,7 @@ mod tests {
#[test]
fn ollama_model_env_overrides_selected_model() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_ollama_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -733,7 +733,7 @@ mod tests {
#[test]
fn openai_compatible_preserves_dotted_model_name() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_compatible_env();
let settings = Settings {
@@ -754,7 +754,7 @@ mod tests {
#[test]
fn registry_provider_resolves_groq() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
std::env::remove_var("LLM_BACKEND");
@@ -779,7 +779,7 @@ mod tests {
#[test]
fn registry_provider_resolves_tinfoil() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
std::env::remove_var("LLM_BACKEND");
@@ -807,7 +807,7 @@ mod tests {
#[test]
fn registry_provider_alias_resolves_zai() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
std::env::remove_var("LLM_BACKEND");
@@ -832,7 +832,7 @@ mod tests {
#[test]
fn registry_provider_resolves_github_copilot_alias() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
std::env::set_var("LLM_BACKEND", "github-copilot");
@@ -880,7 +880,7 @@ mod tests {
#[test]
fn nearai_backend_has_no_registry_provider() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
std::env::remove_var("LLM_BACKEND");
@@ -894,7 +894,7 @@ mod tests {
#[test]
fn backend_alias_normalized_to_canonical_id() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_compatible_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -920,7 +920,7 @@ mod tests {
#[test]
fn unknown_backend_falls_back_to_openai_compatible() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_compatible_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -944,7 +944,7 @@ mod tests {
#[test]
fn nearai_aliases_all_resolve_to_nearai() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
for alias in &["nearai", "near_ai", "near"] {
// SAFETY: Under ENV_MUTEX.
@@ -971,7 +971,7 @@ mod tests {
#[test]
fn base_url_resolution_priority() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_compatible_env();
// SAFETY: Under ENV_MUTEX.
@@ -1029,7 +1029,7 @@ mod tests {
fn anthropic_oauth_token_sets_placeholder_api_key() {
use secrecy::ExposeSecret;
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_anthropic_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -1067,7 +1067,7 @@ mod tests {
fn anthropic_api_key_takes_priority_over_oauth() {
use secrecy::ExposeSecret;
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_anthropic_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -1100,7 +1100,7 @@ mod tests {
#[test]
fn non_anthropic_provider_has_no_oauth_token() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_anthropic_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -1208,7 +1208,7 @@ mod tests {
#[test]
fn test_request_timeout_defaults_to_120() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
std::env::remove_var("LLM_REQUEST_TIMEOUT_SECS");
@@ -1219,7 +1219,7 @@ mod tests {
#[test]
fn test_request_timeout_configurable() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
std::env::set_var("LLM_REQUEST_TIMEOUT_SECS", "300");
@@ -1246,7 +1246,7 @@ mod tests {
#[test]
fn openai_codex_resolves_config() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_codex_env();
let settings = Settings {
@@ -1266,7 +1266,7 @@ mod tests {
#[test]
fn openai_codex_model_env_resolution() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_codex_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -1290,7 +1290,7 @@ mod tests {
#[test]
fn openai_codex_falls_back_to_openai_model() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_codex_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -1314,7 +1314,7 @@ mod tests {
#[test]
fn openai_codex_falls_back_to_selected_model() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_codex_env();
let settings = Settings {
@@ -1331,7 +1331,7 @@ mod tests {
/// Regression: SSRF validation on OPENAI_CODEX_API_URL (#1103).
#[test]
fn openai_codex_rejects_ssrf_api_url() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_codex_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
@@ -1362,7 +1362,7 @@ mod tests {
/// Regression: SSRF validation on OPENAI_CODEX_AUTH_URL (#1103).
#[test]
fn openai_codex_rejects_ssrf_auth_url() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_openai_codex_env();
// SAFETY: Under ENV_MUTEX.
unsafe {
+3 -3
View File
@@ -19,12 +19,12 @@ pub(crate) fn resolve_safety_config(
#[cfg(test)]
mod tests {
use super::*;
use crate::config::helpers::ENV_MUTEX;
use crate::config::helpers::lock_env;
use crate::settings::Settings;
#[test]
fn resolve_falls_back_to_settings() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
let mut settings = Settings::default();
settings.safety.max_output_length = 42;
settings.safety.injection_check_enabled = false;
@@ -36,7 +36,7 @@ mod tests {
#[test]
fn env_overrides_settings() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
let mut settings = Settings::default();
settings.safety.max_output_length = 42;
+5 -15
View File
@@ -594,9 +594,7 @@ mod tests {
#[test]
fn sandbox_resolve_falls_back_to_settings() {
let _guard = crate::config::helpers::ENV_MUTEX
.lock()
.expect("env mutex poisoned");
let _guard = crate::config::helpers::lock_env();
let mut settings = crate::settings::Settings::default();
settings.sandbox.cpu_shares = 99;
settings.sandbox.auto_pull_image = false;
@@ -610,9 +608,7 @@ mod tests {
#[test]
fn sandbox_env_overrides_settings() {
let _guard = crate::config::helpers::ENV_MUTEX
.lock()
.expect("env mutex poisoned");
let _guard = crate::config::helpers::lock_env();
let mut settings = crate::settings::Settings::default();
settings.sandbox.timeout_secs = 999;
@@ -628,9 +624,7 @@ mod tests {
#[test]
fn claude_code_resolve_uses_settings_enabled() {
let _guard = crate::config::helpers::ENV_MUTEX
.lock()
.expect("env mutex poisoned");
let _guard = crate::config::helpers::lock_env();
let mut settings = crate::settings::Settings::default();
settings.sandbox.claude_code_enabled = true;
@@ -640,9 +634,7 @@ mod tests {
#[test]
fn claude_code_resolve_defaults_disabled() {
let _guard = crate::config::helpers::ENV_MUTEX
.lock()
.expect("env mutex poisoned");
let _guard = crate::config::helpers::lock_env();
let settings = crate::settings::Settings::default();
let cfg = ClaudeCodeConfig::resolve(&settings).expect("resolve");
assert!(!cfg.enabled);
@@ -650,9 +642,7 @@ mod tests {
#[test]
fn claude_code_env_overrides_settings() {
let _guard = crate::config::helpers::ENV_MUTEX
.lock()
.expect("env mutex poisoned");
let _guard = crate::config::helpers::lock_env();
let mut settings = crate::settings::Settings::default();
settings.sandbox.claude_code_enabled = true;
+7 -7
View File
@@ -92,7 +92,7 @@ impl WorkspaceSearchConfig {
#[cfg(test)]
mod tests {
use super::*;
use crate::config::helpers::ENV_MUTEX;
use crate::config::helpers::lock_env;
fn clear_search_env() {
// SAFETY: Only called under ENV_MUTEX in tests.
@@ -106,7 +106,7 @@ mod tests {
#[test]
fn defaults_when_no_env() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_search_env();
let config = WorkspaceSearchConfig::resolve().expect("should resolve");
@@ -118,7 +118,7 @@ mod tests {
#[test]
fn env_overrides() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_search_env();
// SAFETY: Under ENV_MUTEX.
@@ -140,7 +140,7 @@ mod tests {
#[test]
fn invalid_strategy_rejected() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_search_env();
// SAFETY: Under ENV_MUTEX.
@@ -156,7 +156,7 @@ mod tests {
#[test]
fn weighted_strategy_defaults() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_search_env();
// SAFETY: Under ENV_MUTEX.
@@ -175,7 +175,7 @@ mod tests {
#[test]
fn weighted_both_zero_rejected() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_search_env();
// SAFETY: Under ENV_MUTEX.
@@ -193,7 +193,7 @@ mod tests {
#[test]
fn rrf_both_zero_allowed() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
clear_search_env();
// SAFETY: Under ENV_MUTEX.
+3 -3
View File
@@ -95,12 +95,12 @@ impl WasmConfig {
#[cfg(test)]
mod tests {
use super::*;
use crate::config::helpers::ENV_MUTEX;
use crate::config::helpers::lock_env;
use crate::settings::Settings;
#[test]
fn resolve_falls_back_to_settings() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
let mut settings = Settings::default();
settings.wasm.default_memory_limit = 42;
settings.wasm.cache_compiled = false;
@@ -112,7 +112,7 @@ mod tests {
#[test]
fn env_overrides_settings() {
let _guard = ENV_MUTEX.lock().expect("env mutex poisoned");
let _guard = lock_env();
let mut settings = Settings::default();
settings.wasm.default_fuel_limit = 42;
+2 -5
View File
@@ -79,13 +79,10 @@ impl WorkspaceConfig {
#[cfg(test)]
mod tests {
use super::*;
use std::sync::Mutex;
// Serialize env-var-dependent tests to avoid races.
static ENV_LOCK: Mutex<()> = Mutex::new(());
use crate::config::helpers::lock_env;
fn with_env(key: &str, val: Option<&str>, f: impl FnOnce()) {
let _guard = ENV_LOCK.lock().unwrap();
let _guard = lock_env();
let prev = std::env::var(key).ok();
match val {
Some(v) => unsafe { std::env::set_var(key, v) },