diff --git a/crates/concurrency/src/manager.rs b/crates/concurrency/src/manager.rs index e005d9fc0..e302c52be 100644 --- a/crates/concurrency/src/manager.rs +++ b/crates/concurrency/src/manager.rs @@ -14,7 +14,7 @@ //! Main concurrency manager -use crate::config::ConcurrencyConfig; +use crate::config::{ConcurrencyConfig, ConfigError}; use std::sync::Arc; /// Snapshot of disk permit queue usage for GetObject orchestration. @@ -76,13 +76,7 @@ pub struct ConcurrencyManager { } impl ConcurrencyManager { - /// Create a new concurrency manager with the given configuration - pub fn new(config: ConcurrencyConfig) -> Self { - // Validate configuration - if let Err(e) = config.validate() { - panic!("Invalid concurrency configuration: {}", e); - } - + fn build(config: ConcurrencyConfig) -> Self { Self { #[cfg(feature = "timeout")] timeout: Arc::new(crate::timeout::TimeoutManager::from_policy(config.timeout_policy)), @@ -106,14 +100,57 @@ impl ConcurrencyManager { } } + /// Try to create a new concurrency manager with the given configuration. + pub fn try_new(config: ConcurrencyConfig) -> Result { + config.validate()?; + Ok(Self::build(config)) + } + + /// Create a new concurrency manager with the given configuration. + /// + /// Invalid configurations are downgraded to the default configuration instead of + /// panicking so startup/runtime callers can remain fail-safe in production paths. + pub fn new(config: ConcurrencyConfig) -> Self { + match Self::try_new(config) { + Ok(manager) => manager, + Err(err) => { + tracing::warn!( + event = "concurrency_manager.invalid_config_fallback", + component = "concurrency", + subsystem = "manager", + error = %err, + "Invalid concurrency configuration detected; falling back to defaults" + ); + Self::build(ConcurrencyConfig::default()) + } + } + } + /// Create with default configuration pub fn with_defaults() -> Self { - Self::new(ConcurrencyConfig::default()) + Self::build(ConcurrencyConfig::default()) + } + + /// Try to create a manager from environment-derived configuration. + pub fn try_from_env() -> Result { + Self::try_new(ConcurrencyConfig::from_env()) } /// Create from environment variables pub fn from_env() -> Self { - Self::new(ConcurrencyConfig::from_env()) + match Self::try_from_env() { + Ok(manager) => manager, + Err(err) => { + tracing::warn!( + event = "concurrency_manager.invalid_env_fallback", + component = "concurrency", + subsystem = "manager", + error = %err, + "Invalid environment-derived concurrency configuration detected; falling back to defaults" + ); + Self::build(ConcurrencyConfig::default()) + } + } } /// Get the configuration @@ -305,6 +342,40 @@ mod tests { assert!(manager.config().validate().is_ok()); } + #[test] + fn test_try_new_returns_error_for_invalid_config() { + let config = ConcurrencyConfig { + timeout_policy: crate::TimeoutManagerPolicy { + default_timeout: std::time::Duration::from_secs(10), + max_timeout: std::time::Duration::from_secs(1), + ..Default::default() + }, + ..Default::default() + }; + + let result = ConcurrencyManager::try_new(config); + assert!(matches!(result, Err(ConfigError::InvalidTimeout(_)))); + } + + #[test] + fn test_new_falls_back_to_default_for_invalid_config() { + let config = ConcurrencyConfig { + timeout_policy: crate::TimeoutManagerPolicy { + default_timeout: std::time::Duration::from_secs(10), + max_timeout: std::time::Duration::from_secs(1), + ..Default::default() + }, + ..Default::default() + }; + + let manager = ConcurrencyManager::new(config); + assert!(manager.config().validate().is_ok()); + assert_eq!( + manager.config().timeout_policy.default_timeout, + ConcurrencyConfig::default().timeout_policy.default_timeout + ); + } + #[tokio::test] async fn test_manager_lifecycle() { let manager = ConcurrencyManager::with_defaults(); diff --git a/crates/filemeta/src/filemeta.rs b/crates/filemeta/src/filemeta.rs index 022d19caa..17b244bdb 100644 --- a/crates/filemeta/src/filemeta.rs +++ b/crates/filemeta/src/filemeta.rs @@ -652,7 +652,7 @@ impl FileMeta { } else { self.versions.remove(i); - let (free_version, to_free) = obj.init_free_version(fi); + let (free_version, to_free) = obj.init_free_version(fi)?; if to_free { self.add_version_filemata(free_version).err() diff --git a/crates/filemeta/src/filemeta/version.rs b/crates/filemeta/src/filemeta/version.rs index dfe06b0d2..272ec6c66 100644 --- a/crates/filemeta/src/filemeta/version.rs +++ b/crates/filemeta/src/filemeta/version.rs @@ -2140,18 +2140,14 @@ impl MetaObject { [bytes[0], bytes[1], bytes[2], bytes[3]] } - pub fn init_free_version(&self, fi: &FileInfo) -> (FileMetaVersion, bool) { + pub fn init_free_version(&self, fi: &FileInfo) -> Result<(FileMetaVersion, bool)> { if fi.skip_tier_free_version() { - return (FileMetaVersion::default(), false); + return Ok((FileMetaVersion::default(), false)); } if let Some(status) = get_bytes(&self.meta_sys, SUFFIX_TRANSITION_STATUS) && status == TRANSITION_COMPLETE.as_bytes().to_vec() { - let vid = Uuid::parse_str(&fi.tier_free_version_id()); - if let Err(err) = vid { - panic!("Invalid Tier Object delete marker versionId {} {}", fi.tier_free_version_id(), err); - } - let vid = vid.unwrap(); + let vid = Uuid::parse_str(&fi.tier_free_version_id())?; let mut free_entry = FileMetaVersion { version_type: VersionType::Delete, write_version: 0, @@ -2176,9 +2172,9 @@ impl MetaObject { insert_bytes(&mut delete_marker.meta_sys, suffix, v); } } - return (free_entry, true); + return Ok((free_entry, true)); } - (FileMetaVersion::default(), false) + Ok((FileMetaVersion::default(), false)) } } @@ -3391,4 +3387,20 @@ mod tests { .into_fileinfo("b", "k", false); assert_eq!(fi.transition_version_id, Some(id)); } + + #[test] + fn meta_object_init_free_version_rejects_invalid_tier_free_version_id() { + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_TRANSITION_STATUS, TRANSITION_COMPLETE.as_bytes().to_vec()); + + let obj = make_meta_object_with_sys(sys); + let mut fi = FileInfo::new("object", 2, 2); + fi.set_tier_free_version_id("not-a-uuid"); + + let err = obj + .init_free_version(&fi) + .expect_err("invalid free-version UUID should return an error instead of panicking"); + + assert!(matches!(err, Error::UuidParse(_))); + } } diff --git a/crates/heal/src/heal/manager.rs b/crates/heal/src/heal/manager.rs index bbf4224fa..0b05c9b2b 100644 --- a/crates/heal/src/heal/manager.rs +++ b/crates/heal/src/heal/manager.rs @@ -2813,10 +2813,10 @@ mod tests { .pop_runnable(|request| can_schedule_request(request, &running, 1)) .expect("should find runnable request"); - match popped.heal_type { - HealType::ErasureSet { set_disk_id, .. } => assert_eq!(set_disk_id, "pool_0_set_2"), - other => panic!("expected erasure set request, got {other:?}"), - } + assert!(matches!( + popped.heal_type, + HealType::ErasureSet { ref set_disk_id, .. } if set_disk_id == "pool_0_set_2" + )); } #[test] diff --git a/crates/lock/src/fast_lock/manager.rs b/crates/lock/src/fast_lock/manager.rs index 4eca21210..7b7801d49 100644 --- a/crates/lock/src/fast_lock/manager.rs +++ b/crates/lock/src/fast_lock/manager.rs @@ -15,7 +15,9 @@ use std::sync::Arc; use tokio::sync::RwLock; use tokio::time::{Instant, interval}; +use tracing::warn; +use crate::LockError; use crate::fast_lock::{ guard::FastLockGuard, manager_trait::LockManager, @@ -42,8 +44,37 @@ impl FastObjectLockManager { /// Create new lock manager with custom config pub fn with_config(config: LockConfig) -> Self { + if let Err(err) = Self::validate_config(&config) { + warn!( + error = %err, + shard_count = config.shard_count, + fallback_shard_count = crate::fast_lock::DEFAULT_SHARD_COUNT, + "Invalid lock manager configuration, falling back to defaults" + ); + return Self::build(LockConfig::default()); + } + + Self::build(config) + } + + /// Create new lock manager with custom config, returning an explicit error for invalid input. + pub fn try_with_config(config: LockConfig) -> crate::Result { + Self::validate_config(&config)?; + Ok(Self::build(config)) + } + + fn validate_config(config: &LockConfig) -> crate::Result<()> { + if config.shard_count == 0 || !config.shard_count.is_power_of_two() { + return Err(LockError::configuration(format!( + "shard count must be a non-zero power of 2, got {}", + config.shard_count + ))); + } + Ok(()) + } + + fn build(config: LockConfig) -> Self { let shard_count = config.shard_count; - assert!(shard_count.is_power_of_two(), "Shard count must be power of 2"); let shards: Vec> = (0..shard_count).map(|i| Arc::new(LockShard::new(i))).collect(); diff --git a/crates/lock/src/fast_lock/tests.rs b/crates/lock/src/fast_lock/tests.rs index 4c4a9a01f..ea05d9b9b 100644 --- a/crates/lock/src/fast_lock/tests.rs +++ b/crates/lock/src/fast_lock/tests.rs @@ -14,8 +14,9 @@ #[cfg(test)] mod fast_lock_tests { - use crate::fast_lock::FastObjectLockManager; + use crate::LockError; use crate::fast_lock::types::{LockConfig, LockMode, LockPriority, LockResult, ObjectKey, ObjectLockRequest}; + use crate::fast_lock::{DEFAULT_SHARD_COUNT, FastObjectLockManager}; use std::sync::Arc; use std::time::Duration; use tokio::time::sleep; @@ -31,6 +32,32 @@ mod fast_lock_tests { FastObjectLockManager::with_config(config) } + #[test] + fn try_with_config_returns_error_for_invalid_shard_count() { + let config = LockConfig { + shard_count: 3, + ..LockConfig::default() + }; + + let err = FastObjectLockManager::try_with_config(config) + .expect_err("non-power-of-two shard counts should return an explicit configuration error"); + + assert!(matches!(err, LockError::Configuration { .. })); + assert!(err.to_string().contains("shard count must be a non-zero power of 2")); + } + + #[tokio::test] + async fn with_config_falls_back_to_default_for_invalid_shard_count() { + let config = LockConfig { + shard_count: 3, + ..LockConfig::default() + }; + + let manager = FastObjectLockManager::with_config(config); + + assert_eq!(manager.shards.len(), DEFAULT_SHARD_COUNT); + } + #[tokio::test] async fn test_basic_write_lock_acquire_release() { let manager = create_test_manager(); diff --git a/rustfs/src/config/config_test.rs b/rustfs/src/config/config_test.rs index 0276cfe73..9e73e4c91 100644 --- a/rustfs/src/config/config_test.rs +++ b/rustfs/src/config/config_test.rs @@ -15,6 +15,7 @@ #[cfg(test)] #[allow(unsafe_op_in_unsafe_fn)] mod tests { + use crate::config::cli::default_server_opts; use crate::config::{CommandResult, Config, Opt, TlsCommands}; use crate::storage::DisksLayout; use rustfs_config::{DEFAULT_CONSOLE_ADDRESS, DEFAULT_CONSOLE_ENABLE, DEFAULT_OBS_ENDPOINT, RUSTFS_REGION}; @@ -100,6 +101,18 @@ mod tests { } } + #[test] + #[serial] + fn test_parse_from_non_server_commands_falls_back_without_panicking() { + let info_opt = Opt::parse_from(["rustfs", "info"]); + let tls_opt = Opt::parse_from(["rustfs", "tls", "inspect", "--path", "/tmp/certs"]); + + assert!(info_opt.volumes.is_empty()); + assert!(tls_opt.volumes.is_empty()); + assert_eq!(info_opt.address, default_server_opts().address); + assert_eq!(tls_opt.address, default_server_opts().address); + } + #[test] #[serial] fn test_default_console_configuration() { diff --git a/rustfs/src/config/opt.rs b/rustfs/src/config/opt.rs index 5209ed5ff..6d1ea71dd 100644 --- a/rustfs/src/config/opt.rs +++ b/rustfs/src/config/opt.rs @@ -98,13 +98,7 @@ impl Opt { let cli = Cli::parse_from(args); match cli.command { Some(Commands::Server(opts)) => Self::from_server_opts(*opts), - Some(Commands::Info(_)) => { - // This should not happen in parse_from, as it's handled by parse_command - panic!("Info command should be handled by parse_command"); - } - Some(Commands::Tls(_)) => { - panic!("TLS command should be handled by parse_command"); - } + Some(Commands::Info(_)) | Some(Commands::Tls(_)) => Self::from_server_opts(default_server_opts()), None => { // Default to server with empty volumes (will be filled from env) Self::from_server_opts(default_server_opts()) diff --git a/rustfs/src/init.rs b/rustfs/src/init.rs index 6130fbaf0..b9245fade 100644 --- a/rustfs/src/init.rs +++ b/rustfs/src/init.rs @@ -491,7 +491,7 @@ pub async fn init_kms_system(config: &config::Config) -> std::io::Result<()> { /// # Arguments /// * `config` - The application configuration options pub fn init_buffer_profile_system(config: &config::Config) { - use crate::config::{RustFSBufferConfig, WorkloadProfile, init_global_buffer_config, set_buffer_profile_enabled}; + use crate::config::{WorkloadProfile, init_global_buffer_config, set_buffer_profile_enabled}; // Whether buffer profiling is disabled or not, it is enabled by default, unless the user explicitly sets '--buffer-profile-disable' or 'RUSTFS_BUFFER_PROFILE_DISABLE=true' if config.buffer_profile_disable { @@ -563,43 +563,20 @@ pub fn init_buffer_profile_system(config: &config::Config) { "Selected buffer profile" ); - // Create and validate buffer configuration - let mut buffer_config = RustFSBufferConfig::new(profile); - if let Err(e) = buffer_config.validate() { + let fallback_profile = WorkloadProfile::from_name(DEFAULT_BUFFER_PROFILE); + let Some(buffer_config) = resolve_buffer_profile_config(profile, fallback_profile) else { warn!( target: "rustfs::init", event = "buffer_profile_validation_failed", component = LOG_COMPONENT_INIT, subsystem = LOG_SUBSYSTEM_BUFFER, - error = %e, + error = "all buffer profile configurations rejected", fallback_profile = DEFAULT_BUFFER_PROFILE, - "Buffer profile validation failed" + "Buffer profile initialization disabled after validation failures" ); - // Fall back to a known-good profile to avoid installing an invalid configuration - let fallback_profile = WorkloadProfile::from_name(DEFAULT_BUFFER_PROFILE); - info!( - target: "rustfs::init", - event = "buffer_profile_fallback", - component = LOG_COMPONENT_INIT, - subsystem = LOG_SUBSYSTEM_BUFFER, - profile = ?fallback_profile, - "Using fallback buffer profile" - ); - let fallback_config = RustFSBufferConfig::new(fallback_profile); - if let Err(e2) = fallback_config.validate() { - error!( - target: "rustfs::init", - event = "buffer_profile_validation_failed", - component = LOG_COMPONENT_INIT, - subsystem = LOG_SUBSYSTEM_BUFFER, - error = %e2, - fallback_profile = DEFAULT_BUFFER_PROFILE, - "Fallback buffer profile validation failed" - ); - panic!("Failed to initialize a valid RustFS buffer configuration"); - } - buffer_config = fallback_config; - } + set_buffer_profile_enabled(false); + return; + }; // Log the workload profile name let workload_name = buffer_config.workload_name(); @@ -630,6 +607,53 @@ pub fn init_buffer_profile_system(config: &config::Config) { } } +fn resolve_buffer_profile_config( + profile: crate::config::WorkloadProfile, + fallback_profile: crate::config::WorkloadProfile, +) -> Option { + use crate::config::RustFSBufferConfig; + + let buffer_config = RustFSBufferConfig::new(profile); + if let Err(err) = buffer_config.validate() { + warn!( + target: "rustfs::init", + event = "buffer_profile_validation_failed", + component = LOG_COMPONENT_INIT, + subsystem = LOG_SUBSYSTEM_BUFFER, + error = %err, + fallback_profile = DEFAULT_BUFFER_PROFILE, + "Buffer profile validation failed" + ); + + info!( + target: "rustfs::init", + event = "buffer_profile_fallback", + component = LOG_COMPONENT_INIT, + subsystem = LOG_SUBSYSTEM_BUFFER, + profile = ?fallback_profile, + "Using fallback buffer profile" + ); + + let fallback_config = RustFSBufferConfig::new(fallback_profile); + if let Err(fallback_err) = fallback_config.validate() { + error!( + target: "rustfs::init", + event = "buffer_profile_validation_failed", + component = LOG_COMPONENT_INIT, + subsystem = LOG_SUBSYSTEM_BUFFER, + error = %fallback_err, + fallback_profile = DEFAULT_BUFFER_PROFILE, + "Fallback buffer profile validation failed" + ); + return None; + } + + Some(fallback_config) + } else { + Some(buffer_config) + } +} + /// Parse and normalize server address for FTP/FTPS /// Forces IPv4 binding to avoid libunftp IPv6 compatibility issues #[allow(dead_code)] @@ -1258,3 +1282,39 @@ pub async fn init_sftp_system() -> Result, Box permit, - Err(error) => panic!("disk read permit acquisition failed: {error}"), - }; + let _permit = permit.ok(); let snapshot = manager.get_object_admission_snapshot(); assert_eq!(snapshot.active, Some(1)); diff --git a/rustfs/src/storage/helper.rs b/rustfs/src/storage/helper.rs index 866cef0bd..72f641335 100644 --- a/rustfs/src/storage/helper.rs +++ b/rustfs/src/storage/helper.rs @@ -500,10 +500,10 @@ mod tests { }); let helper = OperationHelper::new(&req, EventName::ObjectTaggingPut, S3Operation::PutObjectTagging); - let event_args = match &helper { - OperationHelper::Enabled(state) => state.event_builder.clone().expect("event builder should exist").build(), - OperationHelper::Disabled => panic!("helper should be enabled when notify/audit switches are on"), + let OperationHelper::Enabled(state) = &helper else { + panic!("helper should be enabled when notify/audit switches are on"); }; + let event_args = state.event_builder.clone().expect("event builder should exist").build(); assert_eq!(event_args.bucket_name, "issue-2292-bucket"); assert_eq!(event_args.object.bucket, "issue-2292-bucket"); @@ -552,13 +552,11 @@ mod tests { let helper = OperationHelper::new(&req, EventName::ObjectAccessedGet, S3Operation::GetObject); // Verify the helper stored the RequestContext - match &helper { - OperationHelper::Enabled(state) => { - assert!(state.request_context.is_some()); - assert_eq!(state.request_context.as_ref().unwrap().request_id, "ingress-canonical-uuid"); - } - OperationHelper::Disabled => panic!("helper should be enabled when notify/audit switches are on"), - } + let OperationHelper::Enabled(state) = &helper else { + panic!("helper should be enabled when notify/audit switches are on"); + }; + assert!(state.request_context.is_some()); + assert_eq!(state.request_context.as_ref().unwrap().request_id, "ingress-canonical-uuid"); }, ); } @@ -595,10 +593,10 @@ mod tests { let helper = OperationHelper::new(&req, EventName::ObjectAccessedGet, S3Operation::GetObject); // Verify the helper has no RequestContext - match &helper { - OperationHelper::Enabled(state) => assert!(state.request_context.is_none()), - OperationHelper::Disabled => panic!("helper should be enabled when notify/audit switches are on"), - } + let OperationHelper::Enabled(state) = &helper else { + panic!("helper should be enabled when notify/audit switches are on"); + }; + assert!(state.request_context.is_none()); }, ); }