diff --git a/nodedb/src/config/server/env/cluster.rs b/nodedb/src/config/server/env/cluster.rs index 57d14e22f..a2196b185 100644 --- a/nodedb/src/config/server/env/cluster.rs +++ b/nodedb/src/config/server/env/cluster.rs @@ -118,6 +118,7 @@ mod tests { #[test] fn env_cluster_overrides() { + let _env_guard = super::super::test_support::env_lock().lock().unwrap(); // Always start clean. unsafe { std::env::remove_var("NODEDB_NODE_ID"); @@ -131,7 +132,7 @@ mod tests { cluster: Some(make_cluster(1)), ..Default::default() }; - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert_eq!( cfg.cluster.as_ref().unwrap().node_id, 42, @@ -143,7 +144,7 @@ mod tests { unsafe { std::env::set_var("NODEDB_NODE_ID", "99") }; let mut cfg = ServerConfig::default(); - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert!( cfg.cluster.is_none(), "NODEDB_NODE_ID with no [cluster] section must not create cluster" @@ -157,7 +158,7 @@ mod tests { cluster: Some(make_cluster(7)), ..Default::default() }; - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert_eq!( cfg.cluster.as_ref().unwrap().node_id, 7, @@ -172,7 +173,7 @@ mod tests { cluster: Some(make_cluster(1)), ..Default::default() }; - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); let seeds = &cfg.cluster.as_ref().unwrap().seed_nodes; assert_eq!(seeds.len(), 2, "two seed addresses should be applied"); assert_eq!(seeds[0].to_string(), "10.0.0.1:9400"); @@ -190,7 +191,7 @@ mod tests { }), ..Default::default() }; - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); let seeds = &cfg.cluster.as_ref().unwrap().seed_nodes; assert_eq!( seeds.len(), diff --git a/nodedb/src/config/server/env/dispatch.rs b/nodedb/src/config/server/env/dispatch.rs index 2f3166e7d..7b67212af 100644 --- a/nodedb/src/config/server/env/dispatch.rs +++ b/nodedb/src/config/server/env/dispatch.rs @@ -55,13 +55,14 @@ use crate::config::server::ServerConfig; /// handled separately by `crate::config::auth::AuthConfig::resolve_superuser_password()` /// (called from `main.rs`) so that the value is never passed through logging /// code paths or stored in `ServerConfig` where it could appear in debug output. -pub fn apply_env_overrides(config: &mut ServerConfig) { +pub fn apply_env_overrides(config: &mut ServerConfig) -> crate::Result<()> { apply_host_and_ports(config); apply_cluster_overrides(config); - apply_numeric_settings(config); + apply_numeric_settings(config)?; apply_tls_overrides(config); apply_wal_tuning(config); apply_checkpoint_tuning(config); apply_timeseries_overrides(config); super::super::observability::apply_observability_env(&mut config.observability); + Ok(()) } diff --git a/nodedb/src/config/server/env/host_ports.rs b/nodedb/src/config/server/env/host_ports.rs index 8f4ec7c15..c75158b07 100644 --- a/nodedb/src/config/server/env/host_ports.rs +++ b/nodedb/src/config/server/env/host_ports.rs @@ -91,9 +91,10 @@ mod tests { #[test] fn env_data_dir_override() { + let _env_guard = super::super::test_support::env_lock().lock().unwrap(); unsafe { std::env::set_var("NODEDB_DATA_DIR", "/tmp/test-nodedb") }; let mut cfg = ServerConfig::default(); - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert_eq!( cfg.server.data_dir, std::path::PathBuf::from("/tmp/test-nodedb") @@ -105,17 +106,18 @@ mod tests { /// env-var races (env vars are process-global, Rust tests run in parallel). #[test] fn env_memory_limit_overrides() { + let _env_guard = super::super::test_support::env_lock().lock().unwrap(); // ── Valid value → overrides memory_limit ── unsafe { std::env::set_var("NODEDB_MEMORY_LIMIT", "2GiB") }; let mut cfg = ServerConfig::default(); - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert_eq!(cfg.server.memory_limit, 2 * 1024 * 1024 * 1024); // ── Malformed value → memory_limit unchanged ── unsafe { std::env::set_var("NODEDB_MEMORY_LIMIT", "notanumber") }; let mut cfg = ServerConfig::default(); let before = cfg.server.memory_limit; - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert_eq!( cfg.server.memory_limit, before, "malformed value must not change config" @@ -128,17 +130,18 @@ mod tests { /// env-var races (env vars are process-global, Rust tests run in parallel). #[test] fn env_sync_port_overrides() { + let _env_guard = super::super::test_support::env_lock().lock().unwrap(); // ── Valid value → overrides ports.sync ── unsafe { std::env::set_var("NODEDB_PORT_SYNC", "19090") }; let mut cfg = ServerConfig::default(); - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert_eq!(cfg.server.ports.sync, 19090); // ── Malformed value → ports.sync unchanged ── unsafe { std::env::set_var("NODEDB_PORT_SYNC", "notaport") }; let mut cfg = ServerConfig::default(); let before = cfg.server.ports.sync; - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert_eq!( cfg.server.ports.sync, before, "malformed value must not change config" diff --git a/nodedb/src/config/server/env/mod.rs b/nodedb/src/config/server/env/mod.rs index acf12f532..ab51952fb 100644 --- a/nodedb/src/config/server/env/mod.rs +++ b/nodedb/src/config/server/env/mod.rs @@ -18,3 +18,16 @@ mod wal; pub use cluster::parse_seed_nodes; pub use dispatch::apply_env_overrides; pub use memory_size::parse_memory_size; + +/// Serialize tests that read/write process env vars. Parallel cargo test +/// threads otherwise race on `std::env` (e.g. a strict-numeric test setting +/// NODEDB_DATA_PLANE_CORES while a host_ports test reads it). +#[cfg(test)] +pub(crate) mod test_support { + use std::sync::{Mutex, OnceLock}; + + pub(crate) fn env_lock() -> &'static Mutex<()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) + } +} diff --git a/nodedb/src/config/server/env/numeric.rs b/nodedb/src/config/server/env/numeric.rs index 09bb5a0a1..a89c1859f 100644 --- a/nodedb/src/config/server/env/numeric.rs +++ b/nodedb/src/config/server/env/numeric.rs @@ -5,47 +5,49 @@ use crate::config::server::{LogFormat, ServerConfig}; -pub(super) fn apply_numeric_settings(config: &mut ServerConfig) { - if let Ok(val) = std::env::var("NODEDB_DATA_PLANE_CORES") { - match val.trim().parse::() { - Ok(cores) => { - tracing::info!( - env_var = "NODEDB_DATA_PLANE_CORES", - value = cores, - "environment variable override applied" - ); - config.server.data_plane_cores = cores; - } - Err(_) => { - tracing::warn!( - env_var = "NODEDB_DATA_PLANE_CORES", - value = %val, - "ignoring malformed environment variable (expected usize), using config value" - ); - } +/// Parse a positive-or-nonzero usize env override, failing boot on malformed +/// input instead of silently keeping the compiled default (issue #277). +fn apply_positive_usize(var: &str, target: &mut usize, allow_zero: bool) -> crate::Result<()> { + if let Ok(val) = std::env::var(var) { + let parsed = val + .trim() + .parse::() + .map_err(|_| crate::Error::Config { + detail: format!("invalid value '{val}' for {var}: expected positive integer"), + })?; + if !allow_zero && parsed == 0 { + return Err(crate::Error::Config { + detail: format!("{var} must be greater than zero"), + }); } + tracing::info!( + env_var = var, + value = parsed, + "environment variable override applied" + ); + *target = parsed; } + Ok(()) +} - if let Ok(val) = std::env::var("NODEDB_MAX_CONNECTIONS") { - match val.trim().parse::() { - Ok(n) => { - tracing::info!( - env_var = "NODEDB_MAX_CONNECTIONS", - value = n, - "environment variable override applied" - ); - config.server.max_connections = n; - } - Err(_) => { - tracing::warn!( - env_var = "NODEDB_MAX_CONNECTIONS", - value = %val, - "ignoring malformed environment variable (expected usize), using config value" - ); - } - } - } +pub(super) fn apply_numeric_settings(config: &mut ServerConfig) -> crate::Result<()> { + apply_positive_usize( + "NODEDB_DATA_PLANE_CORES", + &mut config.server.data_plane_cores, + false, + )?; + apply_positive_usize( + "NODEDB_MAX_CONNECTIONS", + &mut config.server.max_connections, + false, + )?; + apply_log_format_override(config); + Ok(()) +} +/// NODEDB_LOG_FORMAT is a string-valued override. Malformed values stay a +/// warning (non-numeric path, out of C3 scope). +fn apply_log_format_override(config: &mut ServerConfig) { if let Ok(val) = std::env::var("NODEDB_LOG_FORMAT") { let normalised = val.trim().to_lowercase(); match normalised.as_str() { @@ -75,3 +77,44 @@ pub(super) fn apply_numeric_settings(config: &mut ServerConfig) { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn c3_malformed_env_errors_instead_of_silent_default() { + let _env_guard = super::super::test_support::env_lock().lock().unwrap(); + unsafe { std::env::set_var("NODEDB_DATA_PLANE_CORES", "abc") }; + let mut cfg = ServerConfig::default(); + let err = apply_numeric_settings(&mut cfg).unwrap_err(); + assert!( + err.to_string().contains("NODEDB_DATA_PLANE_CORES"), + "C3: error must name the env var, got: {err}" + ); + unsafe { std::env::remove_var("NODEDB_DATA_PLANE_CORES") }; + } + + #[test] + fn c3_valid_env_still_overrides() { + let _env_guard = super::super::test_support::env_lock().lock().unwrap(); + unsafe { std::env::set_var("NODEDB_DATA_PLANE_CORES", "8") }; + let mut cfg = ServerConfig::default(); + apply_numeric_settings(&mut cfg).unwrap(); + assert_eq!(cfg.server.data_plane_cores, 8); + unsafe { std::env::remove_var("NODEDB_DATA_PLANE_CORES") }; + } + + #[test] + fn c3_zero_value_rejected() { + let _env_guard = super::super::test_support::env_lock().lock().unwrap(); + unsafe { std::env::set_var("NODEDB_DATA_PLANE_CORES", "0") }; + let mut cfg = ServerConfig::default(); + let err = apply_numeric_settings(&mut cfg).unwrap_err(); + assert!( + err.to_string().contains("greater than zero"), + "C3: zero cores must be rejected, got: {err}" + ); + unsafe { std::env::remove_var("NODEDB_DATA_PLANE_CORES") }; + } +} diff --git a/nodedb/src/config/server/env/wal.rs b/nodedb/src/config/server/env/wal.rs index fc3d494c7..d1c2d0221 100644 --- a/nodedb/src/config/server/env/wal.rs +++ b/nodedb/src/config/server/env/wal.rs @@ -50,18 +50,19 @@ mod tests { /// it off — an absent or malformed env var must never be read as one. #[test] fn env_wal_direct_io_override() { + let _env_guard = super::super::test_support::env_lock().lock().unwrap(); let mut cfg = ServerConfig::default(); - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert!(cfg.tuning.wal.direct_io, "default must be direct I/O"); unsafe { std::env::set_var("NODEDB_WAL_DIRECT_IO", "false") }; let mut cfg = ServerConfig::default(); - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert!(!cfg.tuning.wal.direct_io); unsafe { std::env::set_var("NODEDB_WAL_DIRECT_IO", "nonsense") }; let mut cfg = ServerConfig::default(); - apply_env_overrides(&mut cfg); + apply_env_overrides(&mut cfg).unwrap(); assert!( cfg.tuning.wal.direct_io, "a malformed value must not silently disable direct I/O" diff --git a/nodedb/src/main.rs b/nodedb/src/main.rs index d22d3ad15..95917731c 100644 --- a/nodedb/src/main.rs +++ b/nodedb/src/main.rs @@ -96,7 +96,7 @@ async fn server_main() -> anyhow::Result<()> { // correct in case NODEDB_DATA_DIR / NODEDB_MEMORY_LIMIT also affect it. // The overrides are re-applied silently here; the real log messages // will be emitted by the second call after the subscriber is registered. - apply_env_overrides(&mut config); + apply_env_overrides(&mut config)?; // Own the black-box recorder before the subscriber is built: the panic hook // it installs chains in front of the one above, and the reports directory @@ -126,7 +126,7 @@ async fn server_main() -> anyhow::Result<()> { // Re-apply env overrides after tracing initializes so that // info!/warn! messages are actually emitted for operators. - apply_env_overrides(&mut config); + apply_env_overrides(&mut config)?; let cluster_mode_str = startup_log::log_boot_banner(&config_path, &config);