fix(runtime): remove startup panic fallbacks (#3754)

* fix(runtime): remove startup panic fallbacks

* test(runtime): cover buffer profile fallback safety

* test(runtime): reduce panic-style assertions

* fix(runtime): expose fallible env config setup

* test(runtime): simplify permit acquisition assertion

* test(runtime): tighten operation helper assertions

* fix(filemeta): stop panicking on invalid free version ids

* fix(init): satisfy buffer profile clippy lints

* fix(lock): harden fast lock config construction

* chore(checks): refresh layer dependency baseline

---------

Signed-off-by: houseme <housemecn@gmail.com>
This commit is contained in:
houseme
2026-06-23 12:31:26 +08:00
committed by GitHub
parent 583a23bdf2
commit a6878e8fce
11 changed files with 285 additions and 82 deletions
+81 -10
View File
@@ -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<Self, ConfigError> {
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, ConfigError> {
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();
+1 -1
View File
@@ -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()
+21 -9
View File
@@ -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(_)));
}
}
+4 -4
View File
@@ -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]
+32 -1
View File
@@ -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> {
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<Arc<LockShard>> = (0..shard_count).map(|i| Arc::new(LockShard::new(i))).collect();
+28 -1
View File
@@ -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();
+13
View File
@@ -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() {
+1 -7
View File
@@ -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())
+91 -31
View File
@@ -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<crate::config::RustFSBufferConfig> {
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<Option<ShutdownHandle>, Box<dyn std::e
Ok(Some(ShutdownHandle::new(shutdown_tx, task_handle)))
}
}
#[cfg(test)]
mod tests {
use super::resolve_buffer_profile_config;
use crate::config::{BufferConfig, WorkloadProfile};
use rustfs_config::KI_B;
#[test]
fn resolve_buffer_profile_config_returns_fallback_when_primary_is_invalid() {
let invalid_primary = WorkloadProfile::Custom(BufferConfig {
min_size: 64 * KI_B,
max_size: 1024,
default_unknown: 64 * KI_B,
thresholds: vec![(1024, 64 * KI_B)],
});
let resolved = resolve_buffer_profile_config(invalid_primary, WorkloadProfile::GeneralPurpose)
.expect("fallback profile should be accepted");
assert_eq!(resolved.workload, WorkloadProfile::GeneralPurpose);
}
#[test]
fn resolve_buffer_profile_config_returns_none_when_primary_and_fallback_are_invalid() {
let invalid = WorkloadProfile::Custom(BufferConfig {
min_size: 64 * KI_B,
max_size: 1024,
default_unknown: 64 * KI_B,
thresholds: vec![(1024, 64 * KI_B)],
});
let resolved = resolve_buffer_profile_config(invalid.clone(), invalid);
assert!(resolved.is_none());
}
}
+1 -4
View File
@@ -684,10 +684,7 @@ mod integration_tests {
let permit = manager.acquire_disk_read_permit().await;
assert!(permit.is_ok());
let _permit = match permit {
Ok(permit) => 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));
+12 -14
View File
@@ -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());
},
);
}