From fb74fad062ce92118ca4608eca9aa9c19e3b88b3 Mon Sep 17 00:00:00 2001 From: j-goodz <22462858+j-goodz@users.noreply.github.com> Date: Tue, 18 Aug 2026 18:14:26 -0400 Subject: [PATCH] fix(relay): crate-wide EnvGuard ends order-dependent env race between config test writers and unguarded from_env readers Signed-off-by: j-goodz <22462858+j-goodz@users.noreply.github.com> --- crates/buzz-relay/src/api/admin/mod.rs | 5 +- crates/buzz-relay/src/api/bridge.rs | 5 +- crates/buzz-relay/src/api/git/policy.rs | 5 +- crates/buzz-relay/src/api/invites.rs | 5 +- crates/buzz-relay/src/api/media.rs | 5 +- crates/buzz-relay/src/api/operator.rs | 5 +- crates/buzz-relay/src/config.rs | 270 ++++++------------ crates/buzz-relay/src/handlers/event.rs | 5 +- .../src/handlers/identity_archive.rs | 5 +- crates/buzz-relay/src/handlers/relay_admin.rs | 5 +- crates/buzz-relay/src/lib.rs | 3 + crates/buzz-relay/src/mesh_boot.rs | 10 +- crates/buzz-relay/src/state.rs | 5 +- crates/buzz-relay/src/telemetry.rs | 28 +- crates/buzz-relay/src/test_env.rs | 107 +++++++ 15 files changed, 252 insertions(+), 216 deletions(-) create mode 100644 crates/buzz-relay/src/test_env.rs diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index 21f30065f0..58baccd827 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -332,7 +332,10 @@ mod tests { use tower::ServiceExt; async fn test_state() -> Arc { - let mut config = crate::config::Config::from_env().expect("default config loads"); + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().expect("default config loads") + }; config.require_relay_membership = false; config.redis_url = "redis://127.0.0.1:1".to_string(); config.admin = Some(crate::config::AdminConfig { diff --git a/crates/buzz-relay/src/api/bridge.rs b/crates/buzz-relay/src/api/bridge.rs index 8fdea4b3c0..d445830fc4 100644 --- a/crates/buzz-relay/src/api/bridge.rs +++ b/crates/buzz-relay/src/api/bridge.rs @@ -3446,7 +3446,10 @@ mod tests { /// /// Returns `None` when local Postgres is not reachable. async fn bridge_handler_test_state() -> Option> { - let mut config = crate::config::Config::from_env().ok()?; + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().ok()? + }; config.database_url = TEST_DB_URL.to_string(); // Use the real local Redis so enforce_http_admission can pass. config.redis_url = diff --git a/crates/buzz-relay/src/api/git/policy.rs b/crates/buzz-relay/src/api/git/policy.rs index 32d63f4600..d5162cad6b 100644 --- a/crates/buzz-relay/src/api/git/policy.rs +++ b/crates/buzz-relay/src/api/git/policy.rs @@ -825,7 +825,10 @@ printf '%s' "$HMAC_INPUT" | openssl dgst -sha256 -hmac "{secret}" -hex 2>/dev/nu const TEST_DB_URL: &str = "postgres://buzz:buzz_dev@localhost:5432/buzz"; // sadscan:disable np.postgres.1 async fn policy_test_state() -> Arc { - let mut config = crate::config::Config::from_env().expect("default config loads"); + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().expect("default config loads") + }; config.require_relay_membership = false; config.redis_url = "redis://127.0.0.1:1".to_string(); config.database_url = std::env::var("BUZZ_TEST_DATABASE_URL") diff --git a/crates/buzz-relay/src/api/invites.rs b/crates/buzz-relay/src/api/invites.rs index d09c7fc611..b0b2300abf 100644 --- a/crates/buzz-relay/src/api/invites.rs +++ b/crates/buzz-relay/src/api/invites.rs @@ -660,7 +660,10 @@ mod tests { /// Build a closed-relay (`require_relay_membership = true`) test state with /// a fresh community on `host`; returns `None` when Postgres is unavailable. async fn invite_test_state(host: &str) -> Option> { - let mut config = crate::config::Config::from_env().ok()?; + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().ok()? + }; let database_url = std::env::var("BUZZ_TEST_DATABASE_URL") .or_else(|_| std::env::var("DATABASE_URL")) .unwrap_or_else(|_| TEST_DB_URL.to_string()); diff --git a/crates/buzz-relay/src/api/media.rs b/crates/buzz-relay/src/api/media.rs index 3b6e07bad6..306f70166a 100644 --- a/crates/buzz-relay/src/api/media.rs +++ b/crates/buzz-relay/src/api/media.rs @@ -987,7 +987,10 @@ mod tests { } async fn test_state() -> Arc { - let mut config = crate::config::Config::from_env().expect("default config loads"); + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().expect("default config loads") + }; config.require_relay_membership = false; config.redis_url = "redis://127.0.0.1:1".to_string(); config.media_uploads_per_minute = 1; diff --git a/crates/buzz-relay/src/api/operator.rs b/crates/buzz-relay/src/api/operator.rs index 5b69a43874..33a7a1fc6f 100644 --- a/crates/buzz-relay/src/api/operator.rs +++ b/crates/buzz-relay/src/api/operator.rs @@ -570,7 +570,10 @@ mod tests { } async fn operator_test_state(operator_keys: &[Keys]) -> Option> { - let mut config = crate::config::Config::from_env().ok()?; + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().ok()? + }; config.database_url = TEST_DB_URL.to_string(); config.redis_url = "redis://127.0.0.1:1".to_string(); config.relay_url = "wss://tenant.example".to_string(); diff --git a/crates/buzz-relay/src/config.rs b/crates/buzz-relay/src/config.rs index 037c6b1dd3..3757d8a2a9 100644 --- a/crates/buzz-relay/src/config.rs +++ b/crates/buzz-relay/src/config.rs @@ -1049,11 +1049,6 @@ impl Config { mod tests { use super::*; - // Mutex to serialize tests that mutate environment variables. - // Parallel env-var mutation causes `defaults_are_valid` to see the invalid - // value set by `invalid_bind_addr_returns_error`, causing a flaky failure. - static ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); - /// Look up against a fixed set, standing in for process env. fn env_of<'a>(set: &'a [(&'a str, &'a str)]) -> impl Fn(&str) -> Option + use<'a> { move |name| { @@ -1109,7 +1104,7 @@ mod tests { #[test] fn defaults_are_valid() { - let _guard = ENV_MUTEX.lock().unwrap(); + let _env = crate::test_env::EnvGuard::new(); let config = Config::from_env().expect("default config"); assert!(config.bind_addr.port() > 0); assert!(!config.database_url.is_empty()); @@ -1161,24 +1156,16 @@ mod tests { #[test] fn s3_addressing_style_env_accepts_virtual_and_rejects_invalid_values() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_S3_ADDRESSING_STYLE"); - - std::env::set_var("BUZZ_S3_ADDRESSING_STYLE", "virtual"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_S3_ADDRESSING_STYLE", "virtual"); let configured = Config::from_env() .expect("virtual style config") .media .s3_addressing_style; - std::env::set_var("BUZZ_S3_ADDRESSING_STYLE", "auto"); + env.set_now("BUZZ_S3_ADDRESSING_STYLE", "auto"); let invalid = Config::from_env(); - if let Some(value) = previous { - std::env::set_var("BUZZ_S3_ADDRESSING_STYLE", value); - } else { - std::env::remove_var("BUZZ_S3_ADDRESSING_STYLE"); - } - assert_eq!(configured, buzz_media::config::S3AddressingStyle::Virtual); assert!(matches!( invalid, @@ -1192,21 +1179,14 @@ mod tests { fn s3_addressing_style_env_rejects_non_unicode_values() { use std::os::unix::ffi::OsStringExt; - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_S3_ADDRESSING_STYLE"); - std::env::set_var( + let mut env = crate::test_env::EnvGuard::new(); + env.set_now( "BUZZ_S3_ADDRESSING_STYLE", std::ffi::OsString::from_vec(vec![0xff]), ); let invalid = Config::from_env(); - if let Some(value) = previous { - std::env::set_var("BUZZ_S3_ADDRESSING_STYLE", value); - } else { - std::env::remove_var("BUZZ_S3_ADDRESSING_STYLE"); - } - assert!(matches!( invalid, Err(ConfigError::InvalidValue(ref message)) @@ -1216,24 +1196,16 @@ mod tests { #[test] fn redis_pool_size_env_override_and_invalid_fallback() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_REDIS_POOL_SIZE"); - - std::env::set_var("BUZZ_REDIS_POOL_SIZE", "32"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_REDIS_POOL_SIZE", "32"); let overridden = Config::from_env().expect("config").redis_pool_size; - std::env::set_var("BUZZ_REDIS_POOL_SIZE", "0"); + env.set_now("BUZZ_REDIS_POOL_SIZE", "0"); let zero = Config::from_env().expect("config").redis_pool_size; - std::env::set_var("BUZZ_REDIS_POOL_SIZE", "not-a-number"); + env.set_now("BUZZ_REDIS_POOL_SIZE", "not-a-number"); let junk = Config::from_env().expect("config").redis_pool_size; - if let Some(value) = previous { - std::env::set_var("BUZZ_REDIS_POOL_SIZE", value); - } else { - std::env::remove_var("BUZZ_REDIS_POOL_SIZE"); - } - assert_eq!(overridden, 32); assert_eq!(zero, 16, "zero must fall back to the default"); assert_eq!(junk, 16, "unparsable value must fall back to the default"); @@ -1241,24 +1213,16 @@ mod tests { #[test] fn db_pool_size_env_override_and_invalid_fallback() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_DB_POOL_SIZE"); - - std::env::set_var("BUZZ_DB_POOL_SIZE", "80"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_DB_POOL_SIZE", "80"); let overridden = Config::from_env().expect("config").db_pool_size; - std::env::set_var("BUZZ_DB_POOL_SIZE", "0"); + env.set_now("BUZZ_DB_POOL_SIZE", "0"); let zero = Config::from_env().expect("config").db_pool_size; - std::env::set_var("BUZZ_DB_POOL_SIZE", "not-a-number"); + env.set_now("BUZZ_DB_POOL_SIZE", "not-a-number"); let junk = Config::from_env().expect("config").db_pool_size; - if let Some(value) = previous { - std::env::set_var("BUZZ_DB_POOL_SIZE", value); - } else { - std::env::remove_var("BUZZ_DB_POOL_SIZE"); - } - assert_eq!(overridden, 80); assert_eq!(zero, 50, "zero must fall back to the default"); assert_eq!(junk, 50, "unparsable value must fall back to the default"); @@ -1266,27 +1230,19 @@ mod tests { #[test] fn db_read_pool_size_env_override_and_invalid_fallback() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_DB_READ_POOL_SIZE"); - - std::env::remove_var("BUZZ_DB_READ_POOL_SIZE"); + let mut env = crate::test_env::EnvGuard::new(); + env.remove_now("BUZZ_DB_READ_POOL_SIZE"); let unset = Config::from_env().expect("config").db_read_pool_size; - std::env::set_var("BUZZ_DB_READ_POOL_SIZE", "40"); + env.set_now("BUZZ_DB_READ_POOL_SIZE", "40"); let overridden = Config::from_env().expect("config").db_read_pool_size; - std::env::set_var("BUZZ_DB_READ_POOL_SIZE", "0"); + env.set_now("BUZZ_DB_READ_POOL_SIZE", "0"); let zero = Config::from_env().expect("config").db_read_pool_size; - std::env::set_var("BUZZ_DB_READ_POOL_SIZE", "not-a-number"); + env.set_now("BUZZ_DB_READ_POOL_SIZE", "not-a-number"); let junk = Config::from_env().expect("config").db_read_pool_size; - if let Some(value) = previous { - std::env::set_var("BUZZ_DB_READ_POOL_SIZE", value); - } else { - std::env::remove_var("BUZZ_DB_READ_POOL_SIZE"); - } - assert_eq!(unset, None, "unset must inherit the writer pool sizing"); assert_eq!(overridden, Some(40)); assert_eq!(zero, None, "zero must fall back to inheriting"); @@ -1295,24 +1251,17 @@ mod tests { #[test] fn read_database_url_unset_or_blank_is_none() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("READ_DATABASE_URL"); + let mut env = crate::test_env::EnvGuard::new(); - std::env::remove_var("READ_DATABASE_URL"); + env.remove_now("READ_DATABASE_URL"); let unset = Config::from_env().expect("config").read_database_url; - std::env::set_var("READ_DATABASE_URL", " "); + env.set_now("READ_DATABASE_URL", " "); let blank = Config::from_env().expect("config").read_database_url; - std::env::set_var("READ_DATABASE_URL", "postgres://buzz:pw@replica:5432/buzz"); // sadscan:disable np.postgres.1 + env.set_now("READ_DATABASE_URL", "postgres://buzz:pw@replica:5432/buzz"); // sadscan:disable np.postgres.1 let set = Config::from_env().expect("config").read_database_url; - if let Some(value) = previous { - std::env::set_var("READ_DATABASE_URL", value); - } else { - std::env::remove_var("READ_DATABASE_URL"); - } - assert_eq!(unset, None, "unset READ_DATABASE_URL must disable routing"); assert_eq!(blank, None, "blank READ_DATABASE_URL must disable routing"); assert_eq!( @@ -1323,39 +1272,30 @@ mod tests { #[test] fn replica_read_max_age_defaults_off_and_rejects_junk() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_REPLICA_READ_MAX_AGE_MS"); - let previous_old = std::env::var_os("BUZZ_REPLICA_HEAD_MAX_AGE_SECS"); - std::env::remove_var("BUZZ_REPLICA_HEAD_MAX_AGE_SECS"); + let mut env = crate::test_env::EnvGuard::new(); + + env.remove_now("BUZZ_REPLICA_HEAD_MAX_AGE_SECS"); - std::env::remove_var("BUZZ_REPLICA_READ_MAX_AGE_MS"); + env.remove_now("BUZZ_REPLICA_READ_MAX_AGE_MS"); let unset = Config::from_env().expect("config").replica_read_max_age_ms; - std::env::set_var("BUZZ_REPLICA_READ_MAX_AGE_MS", "1000"); + env.set_now("BUZZ_REPLICA_READ_MAX_AGE_MS", "1000"); let set = Config::from_env().expect("config").replica_read_max_age_ms; - std::env::set_var("BUZZ_REPLICA_READ_MAX_AGE_MS", "0"); + env.set_now("BUZZ_REPLICA_READ_MAX_AGE_MS", "0"); let zero = Config::from_env().expect("config").replica_read_max_age_ms; - std::env::set_var("BUZZ_REPLICA_READ_MAX_AGE_MS", "soon"); + env.set_now("BUZZ_REPLICA_READ_MAX_AGE_MS", "soon"); let junk = Config::from_env(); // The retired seconds-denominated name must be a hard startup // error even alongside a valid new-name value: silently ignoring // it (or honouring it) would mean 1000x the intended budget. - std::env::set_var("BUZZ_REPLICA_READ_MAX_AGE_MS", "1000"); - std::env::set_var("BUZZ_REPLICA_HEAD_MAX_AGE_SECS", "5"); + env.set_now("BUZZ_REPLICA_READ_MAX_AGE_MS", "1000"); + env.set_now("BUZZ_REPLICA_HEAD_MAX_AGE_SECS", "5"); let old_name = Config::from_env(); - std::env::remove_var("BUZZ_REPLICA_HEAD_MAX_AGE_SECS"); - if let Some(value) = previous { - std::env::set_var("BUZZ_REPLICA_READ_MAX_AGE_MS", value); - } else { - std::env::remove_var("BUZZ_REPLICA_READ_MAX_AGE_MS"); - } - if let Some(value) = previous_old { - std::env::set_var("BUZZ_REPLICA_HEAD_MAX_AGE_SECS", value); - } + env.remove_now("BUZZ_REPLICA_HEAD_MAX_AGE_SECS"); assert_eq!(unset, 0, "replica read routing must default off"); assert_eq!(set, 1000); @@ -1375,40 +1315,33 @@ mod tests { #[test] fn drain_jitter_defaults_off_and_rejects_junk() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_DRAIN_JITTER_MS"); + let mut env = crate::test_env::EnvGuard::new(); - std::env::remove_var("BUZZ_DRAIN_JITTER_MS"); + env.remove_now("BUZZ_DRAIN_JITTER_MS"); let unset = Config::from_env().expect("config").drain_jitter_ms; - std::env::set_var("BUZZ_DRAIN_JITTER_MS", "20000"); + env.set_now("BUZZ_DRAIN_JITTER_MS", "20000"); let set = Config::from_env().expect("config").drain_jitter_ms; - std::env::set_var("BUZZ_DRAIN_JITTER_MS", "60000"); + env.set_now("BUZZ_DRAIN_JITTER_MS", "60000"); let capped = Config::from_env().expect("config").drain_jitter_ms; - std::env::set_var("BUZZ_DRAIN_JITTER_MS", "0"); + env.set_now("BUZZ_DRAIN_JITTER_MS", "0"); let zero = Config::from_env().expect("config").drain_jitter_ms; - std::env::set_var("BUZZ_DRAIN_JITTER_MS", "soon"); + env.set_now("BUZZ_DRAIN_JITTER_MS", "soon"); let junk = Config::from_env(); - std::env::set_var("BUZZ_DRAIN_JITTER_MS", ""); + env.set_now("BUZZ_DRAIN_JITTER_MS", ""); let empty = Config::from_env() .expect("empty is a valid kill switch") .drain_jitter_ms; - std::env::set_var("BUZZ_DRAIN_JITTER_MS", " "); + env.set_now("BUZZ_DRAIN_JITTER_MS", " "); let blank = Config::from_env() .expect("whitespace-only is a valid kill switch") .drain_jitter_ms; - if let Some(value) = previous { - std::env::set_var("BUZZ_DRAIN_JITTER_MS", value); - } else { - std::env::remove_var("BUZZ_DRAIN_JITTER_MS"); - } - assert_eq!(unset, 0, "drain jitter must default off"); assert_eq!(set, MAX_DRAIN_JITTER_MS); assert_eq!( @@ -1429,30 +1362,20 @@ mod tests { #[test] fn audit_logging_defaults_on_and_accepts_explicit_off() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_AUDIT_ENABLED"); - std::env::remove_var("BUZZ_AUDIT_ENABLED"); + let mut env = crate::test_env::EnvGuard::new(); + + env.remove_now("BUZZ_AUDIT_ENABLED"); assert!(parse_bool("BUZZ_AUDIT_ENABLED", true).unwrap()); - std::env::set_var("BUZZ_AUDIT_ENABLED", "false"); + env.set_now("BUZZ_AUDIT_ENABLED", "false"); assert!(!parse_bool("BUZZ_AUDIT_ENABLED", true).unwrap()); - if let Some(value) = previous { - std::env::set_var("BUZZ_AUDIT_ENABLED", value); - } else { - std::env::remove_var("BUZZ_AUDIT_ENABLED"); - } } #[test] fn audit_logging_rejects_invalid_boolean() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_AUDIT_ENABLED"); - std::env::set_var("BUZZ_AUDIT_ENABLED", "sometimes"); + let mut env = crate::test_env::EnvGuard::new(); + + env.set_now("BUZZ_AUDIT_ENABLED", "sometimes"); let result = parse_bool("BUZZ_AUDIT_ENABLED", true); - if let Some(value) = previous { - std::env::set_var("BUZZ_AUDIT_ENABLED", value); - } else { - std::env::remove_var("BUZZ_AUDIT_ENABLED"); - } assert!(matches!( result, Err(ConfigError::InvalidValue(ref message)) @@ -1462,15 +1385,10 @@ mod tests { #[test] fn join_policy_age_attestation_rejects_invalid_boolean() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_AGE_ATTESTATION_REQUIRED"); - std::env::set_var("BUZZ_AGE_ATTESTATION_REQUIRED", "sometimes"); + let mut env = crate::test_env::EnvGuard::new(); + + env.set_now("BUZZ_AGE_ATTESTATION_REQUIRED", "sometimes"); let result = parse_optional_bool("BUZZ_AGE_ATTESTATION_REQUIRED"); - if let Some(value) = previous { - std::env::set_var("BUZZ_AGE_ATTESTATION_REQUIRED", value); - } else { - std::env::remove_var("BUZZ_AGE_ATTESTATION_REQUIRED"); - } assert!(matches!( result, Err(ConfigError::InvalidValue(ref message)) @@ -1480,16 +1398,13 @@ mod tests { #[test] fn rate_limits_can_be_overridden() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var("BUZZ_RATE_LIMIT_HUMAN_MESSAGES_PER_MIN", "1001"); - std::env::set_var("BUZZ_RATE_LIMIT_HUMAN_API_CALLS_PER_MIN", "1002"); - std::env::set_var("BUZZ_RATE_LIMIT_HUMAN_WS_EVENTS_PER_SEC", "1003"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_RATE_LIMIT_HUMAN_MESSAGES_PER_MIN", "1001"); + env.set_now("BUZZ_RATE_LIMIT_HUMAN_API_CALLS_PER_MIN", "1002"); + env.set_now("BUZZ_RATE_LIMIT_HUMAN_WS_EVENTS_PER_SEC", "1003"); let config = Config::from_env().expect("config"); - std::env::remove_var("BUZZ_RATE_LIMIT_HUMAN_MESSAGES_PER_MIN"); - std::env::remove_var("BUZZ_RATE_LIMIT_HUMAN_API_CALLS_PER_MIN"); - std::env::remove_var("BUZZ_RATE_LIMIT_HUMAN_WS_EVENTS_PER_SEC"); assert_eq!(config.auth.rate_limits.human_messages_per_min, 1001); assert_eq!(config.auth.rate_limits.human_api_calls_per_min, 1002); assert_eq!(config.auth.rate_limits.human_ws_events_per_sec, 1003); @@ -1497,10 +1412,9 @@ mod tests { #[test] fn rate_limit_overrides_reject_zero() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var("BUZZ_RATE_LIMIT_HUMAN_WS_EVENTS_PER_SEC", "0"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_RATE_LIMIT_HUMAN_WS_EVENTS_PER_SEC", "0"); let result = Config::from_env(); - std::env::remove_var("BUZZ_RATE_LIMIT_HUMAN_WS_EVENTS_PER_SEC"); assert!(matches!( result, @@ -1511,18 +1425,16 @@ mod tests { #[test] fn relay_operator_pubkeys_parse_dedupe_and_normalize() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var( + let mut env = crate::test_env::EnvGuard::new(); + env.set_now( "RELAY_OPERATOR_PUBKEYS", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA,bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", ); - std::env::set_var( + env.set_now( "RELAY_OPERATOR_API_ORIGIN", "http://buzz.mesh.bb-production.com", ); let config = Config::from_env().expect("config"); - std::env::remove_var("RELAY_OPERATOR_PUBKEYS"); - std::env::remove_var("RELAY_OPERATOR_API_ORIGIN"); assert_eq!( config.relay_operator_pubkeys, @@ -1535,10 +1447,9 @@ mod tests { #[test] fn relay_operator_pubkeys_invalid_entry_is_error() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var("RELAY_OPERATOR_PUBKEYS", "not-a-pubkey"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("RELAY_OPERATOR_PUBKEYS", "not-a-pubkey"); let result = Config::from_env(); - std::env::remove_var("RELAY_OPERATOR_PUBKEYS"); assert!(matches!( result, @@ -1548,14 +1459,13 @@ mod tests { #[test] fn relay_operator_pubkeys_require_api_origin() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var( + let mut env = crate::test_env::EnvGuard::new(); + env.set_now( "RELAY_OPERATOR_PUBKEYS", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", ); - std::env::remove_var("RELAY_OPERATOR_API_ORIGIN"); + env.remove_now("RELAY_OPERATOR_API_ORIGIN"); let result = Config::from_env(); - std::env::remove_var("RELAY_OPERATOR_PUBKEYS"); assert!(matches!( result, @@ -1565,10 +1475,9 @@ mod tests { #[test] fn relay_operator_api_origin_rejects_paths() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var("RELAY_OPERATOR_API_ORIGIN", "https://buzz.example/operator"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("RELAY_OPERATOR_API_ORIGIN", "https://buzz.example/operator"); let result = Config::from_env(); - std::env::remove_var("RELAY_OPERATOR_API_ORIGIN"); assert!(matches!( result, @@ -1578,9 +1487,8 @@ mod tests { #[test] fn push_gateway_defaults_to_buzz_and_can_be_disabled() { - let _guard = ENV_MUTEX.lock().unwrap(); - let previous = std::env::var_os("BUZZ_PUSH_GATEWAY_DELIVERY_URL"); - std::env::remove_var("BUZZ_PUSH_GATEWAY_DELIVERY_URL"); + let mut env = crate::test_env::EnvGuard::new(); + env.remove_now("BUZZ_PUSH_GATEWAY_DELIVERY_URL"); let config = Config::from_env().expect("default config"); assert_eq!( config @@ -1590,15 +1498,9 @@ mod tests { Some(DEFAULT_PUSH_GATEWAY_DELIVERY_URL) ); - std::env::set_var("BUZZ_PUSH_GATEWAY_DELIVERY_URL", ""); + env.set_now("BUZZ_PUSH_GATEWAY_DELIVERY_URL", ""); let config = Config::from_env().expect("disabled push config"); assert!(config.push_gateway_delivery_url.is_none()); - - if let Some(value) = previous { - std::env::set_var("BUZZ_PUSH_GATEWAY_DELIVERY_URL", value); - } else { - std::env::remove_var("BUZZ_PUSH_GATEWAY_DELIVERY_URL"); - } } #[test] @@ -1619,10 +1521,9 @@ mod tests { #[test] fn invalid_push_gateway_timeout_is_not_silently_defaulted() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var("BUZZ_PUSH_GATEWAY_TIMEOUT_MS", "99"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_PUSH_GATEWAY_TIMEOUT_MS", "99"); let result = Config::from_env(); - std::env::remove_var("BUZZ_PUSH_GATEWAY_TIMEOUT_MS"); assert!(matches!( result, Err(ConfigError::InvalidValue(ref message)) @@ -1632,10 +1533,9 @@ mod tests { #[test] fn invalid_push_executor_key_id_is_rejected() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var("BUZZ_PUSH_EXECUTOR_KEY_ID", ""); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_PUSH_EXECUTOR_KEY_ID", ""); let result = Config::from_env(); - std::env::remove_var("BUZZ_PUSH_EXECUTOR_KEY_ID"); assert!(matches!( result, Err(ConfigError::InvalidValue(ref message)) @@ -1645,10 +1545,9 @@ mod tests { #[test] fn huddle_audio_available_can_be_disabled_for_horizontal_scaling() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var("BUZZ_HUDDLE_AUDIO_AVAILABLE", "false"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_HUDDLE_AUDIO_AVAILABLE", "false"); let config = Config::from_env().expect("config"); - std::env::remove_var("BUZZ_HUDDLE_AUDIO_AVAILABLE"); assert!( !config.huddle_audio_available, "BUZZ_HUDDLE_AUDIO_AVAILABLE=false must disable huddle audio (multi-pod deployments)" @@ -1665,17 +1564,16 @@ mod tests { #[test] fn pairing_relay_url_accepts_websocket_urls_and_rejects_http() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var("BUZZ_PAIRING_RELAY_URL", "wss://pairing.buzz.xyz"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_PAIRING_RELAY_URL", "wss://pairing.buzz.xyz"); let config = Config::from_env().expect("config"); assert_eq!( config.pairing_relay_url.as_deref(), Some("wss://pairing.buzz.xyz") ); - std::env::set_var("BUZZ_PAIRING_RELAY_URL", "https://pairing.buzz.xyz"); + env.set_now("BUZZ_PAIRING_RELAY_URL", "https://pairing.buzz.xyz"); let result = Config::from_env(); - std::env::remove_var("BUZZ_PAIRING_RELAY_URL"); assert!(matches!( result, Err(ConfigError::InvalidValue(ref msg)) if msg.contains("BUZZ_PAIRING_RELAY_URL") @@ -1684,16 +1582,15 @@ mod tests { #[test] fn max_frame_bytes_can_be_configured() { - let _guard = ENV_MUTEX.lock().unwrap(); - std::env::set_var("BUZZ_MAX_FRAME_BYTES", "262144"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("BUZZ_MAX_FRAME_BYTES", "262144"); let config = Config::from_env().expect("config"); - std::env::remove_var("BUZZ_MAX_FRAME_BYTES"); assert_eq!(config.max_frame_bytes, 262_144); } #[test] fn git_repo_path_is_created_if_missing() { - let _guard = ENV_MUTEX.lock().unwrap(); + let mut env = crate::test_env::EnvGuard::new(); // Pick a path under temp_dir that definitely doesn't exist yet. let base = std::env::temp_dir().join(format!( "buzz-test-git-repo-path-{}-{}", @@ -1706,9 +1603,8 @@ mod tests { let nested = base.join("nested").join("repos"); assert!(!nested.exists(), "test precondition: path must not exist"); - std::env::set_var("BUZZ_GIT_REPO_PATH", &nested); + env.set_now("BUZZ_GIT_REPO_PATH", &nested); let result = Config::from_env(); - std::env::remove_var("BUZZ_GIT_REPO_PATH"); let config = result.expect("config should self-bootstrap missing git_repo_path"); assert_eq!(config.git_repo_path, nested); diff --git a/crates/buzz-relay/src/handlers/event.rs b/crates/buzz-relay/src/handlers/event.rs index ccba40f328..a737174c34 100644 --- a/crates/buzz-relay/src/handlers/event.rs +++ b/crates/buzz-relay/src/handlers/event.rs @@ -2005,7 +2005,10 @@ mod tests { use crate::state::AppState; pub(super) fn test_config() -> crate::config::Config { - let mut config = crate::config::Config::from_env().expect("default config loads"); + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().expect("default config loads") + }; config.require_relay_membership = false; config.redis_url = "redis://127.0.0.1:1".to_string(); config diff --git a/crates/buzz-relay/src/handlers/identity_archive.rs b/crates/buzz-relay/src/handlers/identity_archive.rs index 9da920483f..772b5b06dc 100644 --- a/crates/buzz-relay/src/handlers/identity_archive.rs +++ b/crates/buzz-relay/src/handlers/identity_archive.rs @@ -443,7 +443,10 @@ mod tests { async fn test_state(pool: sqlx::PgPool) -> Option> { let db = buzz_db::Db::from_pool(pool.clone()); - let config = crate::config::Config::from_env().ok()?; + let config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().ok()? + }; let redis_pool = deadpool_redis::Config::from_url(&config.redis_url) .create_pool(Some(deadpool_redis::Runtime::Tokio1)) .ok()?; diff --git a/crates/buzz-relay/src/handlers/relay_admin.rs b/crates/buzz-relay/src/handlers/relay_admin.rs index 3782f2c516..93b7cc8696 100644 --- a/crates/buzz-relay/src/handlers/relay_admin.rs +++ b/crates/buzz-relay/src/handlers/relay_admin.rs @@ -705,7 +705,10 @@ mod tests { host: &str, require_relay_membership: bool, ) -> (Arc, TenantContext) { - let mut config = crate::config::Config::from_env().expect("config from env"); + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().expect("config from env") + }; let database_url = std::env::var("BUZZ_TEST_DATABASE_URL") .or_else(|_| std::env::var("DATABASE_URL")) .unwrap_or_else(|_| TEST_DB_URL.to_string()); diff --git a/crates/buzz-relay/src/lib.rs b/crates/buzz-relay/src/lib.rs index 314adad92e..9de8f8625e 100644 --- a/crates/buzz-relay/src/lib.rs +++ b/crates/buzz-relay/src/lib.rs @@ -44,6 +44,9 @@ pub mod subscription; pub mod telemetry; /// Row-zero host binding: resolve the request community from the connection host. pub mod tenant; +/// Crate-wide env lock + restore guard for tests that touch process env. +#[cfg(test)] +pub(crate) mod test_env; /// Relay-side tunnel session directory and routing. pub mod tunnel; /// Webhook secret generation and constant-time comparison. diff --git a/crates/buzz-relay/src/mesh_boot.rs b/crates/buzz-relay/src/mesh_boot.rs index cd7c427c72..391ecd798a 100644 --- a/crates/buzz-relay/src/mesh_boot.rs +++ b/crates/buzz-relay/src/mesh_boot.rs @@ -530,7 +530,10 @@ mod tests { /// ever reached Redis this test would hang/fail. #[tokio::test] async fn mesh_off_boots_nothing() { - let mut config = crate::config::Config::from_env().expect("default config loads"); + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().expect("default config loads") + }; config.mesh.enabled = false; let pool = deadpool_redis::Config::from_url("redis://127.0.0.1:1") // unroutable .create_pool(Some(deadpool_redis::Runtime::Tokio1)) @@ -557,7 +560,10 @@ mod tests { if std::env::var("BUZZ_MESH").is_ok() { return; // externally forced — skip rather than assert a lie } - let config = crate::config::Config::from_env().expect("default config loads"); + let config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().expect("default config loads") + }; assert!(!config.mesh.enabled, "BUZZ_MESH absent must mean mesh off"); } diff --git a/crates/buzz-relay/src/state.rs b/crates/buzz-relay/src/state.rs index 2f544e188c..95eac5aaab 100644 --- a/crates/buzz-relay/src/state.rs +++ b/crates/buzz-relay/src/state.rs @@ -1399,7 +1399,10 @@ mod tests { } async fn test_state() -> Arc { - let mut config = crate::config::Config::from_env().expect("default config loads"); + let mut config = { + let _env = crate::test_env::EnvGuard::new(); + crate::config::Config::from_env().expect("default config loads") + }; config.require_relay_membership = false; config.redis_url = "redis://127.0.0.1:1".to_string(); let pool = sqlx::PgPool::connect_lazy(&config.database_url).expect("lazy pg pool"); diff --git a/crates/buzz-relay/src/telemetry.rs b/crates/buzz-relay/src/telemetry.rs index 91bd92f0f3..2472dba5f2 100644 --- a/crates/buzz-relay/src/telemetry.rs +++ b/crates/buzz-relay/src/telemetry.rs @@ -278,10 +278,6 @@ mod tests { }; use tracing_subscriber::prelude::*; - // Env vars are process-global — serialize tests that mutate them to prevent - // cross-test races when the suite runs with multiple threads. - static ENV_LOCK: Mutex<()> = Mutex::new(()); - // Helper: read service.name from the Resource's schema_url-independent KV list. fn service_name_from(resource: &Resource) -> Option { resource @@ -495,9 +491,9 @@ mod tests { #[test] fn test_service_resource_default_when_env_unset() { - let _guard = ENV_LOCK.lock().unwrap(); - std::env::remove_var("OTEL_SERVICE_NAME"); - std::env::remove_var("OTEL_RESOURCE_ATTRIBUTES"); + let mut env = crate::test_env::EnvGuard::new(); + env.remove_now("OTEL_SERVICE_NAME"); + env.remove_now("OTEL_RESOURCE_ATTRIBUTES"); let r = service_resource(); assert_eq!( @@ -509,12 +505,11 @@ mod tests { #[test] fn test_service_resource_honors_otel_service_name() { - let _guard = ENV_LOCK.lock().unwrap(); - std::env::set_var("OTEL_SERVICE_NAME", "my-custom-relay"); - std::env::remove_var("OTEL_RESOURCE_ATTRIBUTES"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("OTEL_SERVICE_NAME", "my-custom-relay"); + env.remove_now("OTEL_RESOURCE_ATTRIBUTES"); let r = service_resource(); - std::env::remove_var("OTEL_SERVICE_NAME"); assert_eq!( service_name_from(&r).as_deref(), @@ -525,12 +520,11 @@ mod tests { #[test] fn test_service_resource_empty_string_falls_back_to_default() { - let _guard = ENV_LOCK.lock().unwrap(); - std::env::set_var("OTEL_SERVICE_NAME", ""); - std::env::remove_var("OTEL_RESOURCE_ATTRIBUTES"); + let mut env = crate::test_env::EnvGuard::new(); + env.set_now("OTEL_SERVICE_NAME", ""); + env.remove_now("OTEL_RESOURCE_ATTRIBUTES"); let r = service_resource(); - std::env::remove_var("OTEL_SERVICE_NAME"); assert_eq!( service_name_from(&r).as_deref(), @@ -541,8 +535,8 @@ mod tests { #[test] fn test_try_init_tracer_disabled_when_endpoint_unset() { - let _guard = ENV_LOCK.lock().unwrap(); - std::env::remove_var("OTEL_EXPORTER_OTLP_ENDPOINT"); + let mut env = crate::test_env::EnvGuard::new(); + env.remove_now("OTEL_EXPORTER_OTLP_ENDPOINT"); let resource = Resource::builder_empty() .with_attribute(KeyValue::new("service.name", "test")) diff --git a/crates/buzz-relay/src/test_env.rs b/crates/buzz-relay/src/test_env.rs new file mode 100644 index 0000000000..6ac2066cbb --- /dev/null +++ b/crates/buzz-relay/src/test_env.rs @@ -0,0 +1,107 @@ +//! Shared process-global env guard for tests. +//! +//! Rust's default test runner executes tests on parallel threads in one +//! process, so any test that mutates process-global environment variables +//! (`std::env::set_var` / `remove_var`) can be observed mid-flight by another +//! test reading the same variable. This crate previously had two independent +//! per-module mutexes (`ENV_MUTEX` in `config.rs`, `ENV_LOCK` in +//! `telemetry.rs`) serializing the *writers* — but test helpers in a dozen +//! other modules call `Config::from_env()` (which reads the very vars +//! config.rs tests mutate, e.g. `BUZZ_S3_ADDRESSING_STYLE`, +//! `BUZZ_REDIS_POOL_SIZE`) while holding **no lock at all**. A reader +//! interleaving with a writer mid-mutation observes an invalid value and +//! `from_env()` errors — an order-dependent flake. +//! +//! [`EnvGuard`] fixes both halves: a single crate-wide mutex serializes every +//! env-mutating test AND every env-reading helper, and the guard records each +//! touched key's prior value so it is restored on drop — even when the test +//! panics or returns early. +//! +//! Reader idiom (test helpers that only need a consistent snapshot): +//! ```ignore +//! let config = { +//! let _env = crate::test_env::EnvGuard::new(); +//! crate::config::Config::from_env().expect("default config loads") +//! }; +//! ``` +//! Keep the guard scope tight and synchronous — never hold it across an +//! `.await`. +//! +//! A test must create at most one guard at a time (builder-style +//! `set`/`remove` chain, or `new()` followed by `set_now`/`remove_now`); +//! creating a second guard while one is live would deadlock on the mutex. + +use std::ffi::{OsStr, OsString}; +use std::sync::{Mutex, MutexGuard}; + +static ENV_LOCK: Mutex<()> = Mutex::new(()); + +/// Serializes process-global env mutations across the crate's tests and +/// restores every touched key to its pre-test value on drop. +pub struct EnvGuard { + _lock: MutexGuard<'static, ()>, + /// `(key, prior_value)`, in first-touch order; restored in reverse on drop. + originals: Vec<(&'static str, Option)>, +} + +impl EnvGuard { + /// Acquire the crate-wide env lock. The lock is held until the guard is + /// dropped, so no two tests can interleave env mutations. + pub fn new() -> Self { + Self { + _lock: ENV_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()), + originals: Vec::new(), + } + } + + fn record(&mut self, key: &'static str) { + if !self.originals.iter().any(|(k, _)| *k == key) { + self.originals.push((key, std::env::var_os(key))); + } + } + + /// Set `key` while holding the lock and record its prior value for + /// restoration. Builder style: chain multiple calls before binding. + pub fn set(mut self, key: &'static str, value: impl AsRef) -> Self { + self.set_now(key, value); + self + } + + /// Set `key` (mid-test mutation while the guard already holds the lock). + pub fn set_now(&mut self, key: &'static str, value: impl AsRef) { + self.record(key); + std::env::set_var(key, value); + } + + /// Remove `key` while holding the lock and record its prior value for + /// restoration. Builder style: chain multiple calls before binding. + pub fn remove(mut self, key: &'static str) -> Self { + self.remove_now(key); + self + } + + /// Remove `key` (mid-test mutation while the guard already holds the lock). + pub fn remove_now(&mut self, key: &'static str) { + self.record(key); + std::env::remove_var(key); + } +} + +impl Default for EnvGuard { + fn default() -> Self { + Self::new() + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + for (key, original) in self.originals.iter().rev() { + match original { + Some(value) => std::env::set_var(key, value), + None => std::env::remove_var(key), + } + } + } +}