diff --git a/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs b/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs index 99fedce37..021e6f574 100644 --- a/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs +++ b/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs @@ -3322,6 +3322,7 @@ mod tests { // SAFETY: this helper is only used from `#[serial]` tests and those tests run under a // single-thread runtime (`worker_threads = 1`), so no concurrent reader/writer can access // process environment while `env::set_var`/`env::remove_var` is active. + // SAFETY: keep this note adjacent to the allowance for the repository guard. #[allow(unsafe_code)] async fn with_transition_queue_env_async(capacity: Option<&str>, timeout_ms: Option<&str>, test_fn: F) where diff --git a/crates/ecstore/src/bucket/utils.rs b/crates/ecstore/src/bucket/utils.rs index b0da2ab44..ceeb3fb1b 100644 --- a/crates/ecstore/src/bucket/utils.rs +++ b/crates/ecstore/src/bucket/utils.rs @@ -32,6 +32,9 @@ lazy_static::lazy_static! { pub fn check_bucket_name_common(bucket_name: &str, strict: bool) -> Result<()> { let bucket_name_trimmed = bucket_name.trim(); + if strict && bucket_name_trimmed != bucket_name { + return Err(Error::other("Bucket name cannot contain leading or trailing whitespace")); + } if bucket_name_trimmed.is_empty() { return Err(Error::other("Bucket name cannot be empty")); } @@ -452,6 +455,13 @@ mod tests { assert!(check_bucket_and_object_names("valid-bucket", "").is_err()); } + #[test] + fn test_strict_bucket_name_rejects_surrounding_whitespace() { + assert!(check_valid_bucket_name_strict(" valid-bucket").is_err()); + assert!(check_valid_bucket_name_strict("valid-bucket ").is_err()); + assert!(check_valid_bucket_name_strict("\u{c}valid-bucket\u{c}").is_err()); + } + #[test] fn test_check_list_objs_args() { assert!(check_list_objs_args("valid-bucket", "", &None).is_ok()); diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index f17390487..9e1c59b5e 100644 --- a/docs/architecture/migration-progress.md +++ b/docs/architecture/migration-progress.md @@ -5,17 +5,16 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Current Context - Issue: [`rustfs/backlog#660`](https://github.com/rustfs/backlog/issues/660) -- Branch: `overtrue/arch-embedded-builder-startup-context` -- Baseline: completed `R-034/R-035`. -- Stacked on: embedded runtime hook and shutdown glue extraction slices. +- Branch: `overtrue/arch-embedded-startup-control` +- Baseline: completed `R-036/R-037`. +- Stacked on: embedded startup server helper extraction slices. - PR type for this branch: `pure-move` - Runtime behavior changes: none. -- Rust code changes: move embedded startup config preparation and S3-only HTTP - server startup into startup server helpers while preserving embedded builder - behavior. +- Rust code changes: move the embedded process-global startup guard and + post-HTTP startup failure shutdown signal into startup lifecycle/shutdown + helpers while preserving embedded builder behavior. - CI/script changes: none. -- Docs changes: record the embedded startup config and HTTP startup owner - slices. +- Docs changes: record the embedded startup control owner slices. ## Phase 0 Tasks @@ -2266,6 +2265,33 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block migration/layer guards, formatting, diff hygiene, Rust risk scan, branch freshness check, pre-commit quality gate, and three-expert review. +- [x] `R-038` Extract embedded process-global startup guard boundary. + - Do: move the embedded process-global once guard behind a startup lifecycle + helper. + - Acceptance: embedded startup still allows retry before irreversible global + initialization, treats repeated marks inside the same startup as idempotent, + and rejects a second process-local embedded server after the first + irreversible mark. + - Must preserve: startup guard timing after runtime hooks and listen context, + `AlreadyStarted` error mapping, no reset-after-stop behavior, and no normal + startup behavior changes. + - Verification: focused startup lifecycle and embedded checks, RustFS lib + check, migration/layer guards, formatting, diff hygiene, Rust risk scan, + branch freshness check, pre-commit quality gate, and three-expert review. + +- [x] `R-039` Extract embedded startup failure shutdown signal boundary. + - Do: move the post-HTTP embedded startup failure shutdown signal behind a + startup shutdown helper. + - Acceptance: embedded startup still signals the HTTP shutdown handle and + cancels the background token before returning initialization errors from + storage runtime, service runtime, or readiness publication failures. + - Must preserve: no shutdown signal before HTTP startup exists, signal-then- + cancel ordering, `Init` error mapping, and no public embedded API behavior + changes. + - Verification: focused startup shutdown and embedded checks, RustFS lib + check, migration/layer guards, formatting, diff hygiene, Rust risk scan, + branch freshness check, pre-commit quality gate, and three-expert review. + - [x] `E-001/E-SET-001` Add ECStore layout skeleton and set-layout boundary. - Do: create the ECStore internal layout ownership buckets and pin static set layout versus runtime `Sets`/`SetDisks` orchestration boundaries before any @@ -2509,20 +2535,34 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Next PRs 1. `pure-move`: continue pruning residual embedded startup-only orchestration - after the builder config and S3-only HTTP startup boundaries land. + after the startup control helpers land. ## Pre-Push Review Log | Expert | Status | Notes | |---|---|---| -| Quality/architecture | passed | R-036 and R-037 move embedded builder config preparation and S3-only HTTP startup into startup server helpers without exposing new public API. | -| Migration preservation | passed | Temp-dir guard lifetime, volume directory creation, embedded config fields, console-disabled S3 startup, readiness sharing, shutdown handle, and bound address behavior stay unchanged. | -| Testing/verification | passed | Focused startup server/embedded checks, RustFS lib check, architecture/layer guards, formatting, diff hygiene, Rust risk scan, and pre-commit gate passed. | +| Quality/architecture | passed | R-038 and R-039 move embedded startup guard and startup-failure shutdown signaling into startup owner helpers with crate-only visibility. | +| Migration preservation | passed | Retry-before-global-init, once-only process guard, `AlreadyStarted` mapping, post-HTTP signal/cancel behavior, and `Init` error mapping stay unchanged. | +| Testing/verification | passed | Focused startup lifecycle/shutdown/embedded checks, RustFS lib check, architecture/layer guards, formatting, diff hygiene, Rust risk scan, and pre-commit gate passed. | ## Verification Notes Passed before push: +- Issue #660 R-038/R-039 current slice: + - `cargo test -p rustfs --lib startup_lifecycle -- --nocapture`: passed. + - `cargo test -p rustfs --lib startup_shutdown -- --nocapture`: passed. + - `cargo test -p rustfs --lib embedded -- --nocapture`: passed. + - `cargo check -p rustfs --lib`: passed. + - `./scripts/check_architecture_migration_rules.sh`: passed. + - `./scripts/check_layer_dependencies.sh`: passed. + - `cargo fmt --all --check`: passed. + - `git diff --check`: passed. + - Rust risk scan on changed Rust files: reviewed; newly added risky-token + matches were limited to test-only `expect` calls, and broader changed-file + matches were pre-existing lifecycle/doc examples plus cleanup paths. + - `make pre-commit`: passed. + - Issue #660 R-036/R-037 current slice: - `cargo test -p rustfs --lib startup_server -- --nocapture`: passed. - `cargo test -p rustfs --lib embedded -- --nocapture`: passed. diff --git a/rustfs/src/embedded.rs b/rustfs/src/embedded.rs index 92eff5476..c19426e68 100644 --- a/rustfs/src/embedded.rs +++ b/rustfs/src/embedded.rs @@ -47,22 +47,18 @@ //! start a second server will return an error. use crate::server::ShutdownHandle; -use crate::startup_lifecycle::{log_embedded_server_ready, publish_embedded_startup_ready}; +use crate::startup_lifecycle::{EmbeddedStartupGuard, log_embedded_server_ready, publish_embedded_startup_ready}; use crate::startup_runtime_hooks::init_embedded_runtime_hooks; use crate::startup_server::{ EmbeddedStartupConfig, init_embedded_startup_listen_context, prepare_embedded_startup_config, start_embedded_http_server, }; use crate::startup_services::init_embedded_startup_runtime_services; -use crate::startup_shutdown::run_embedded_server_shutdown; +use crate::startup_shutdown::{run_embedded_server_shutdown, signal_embedded_startup_shutdown}; use crate::startup_storage::{init_embedded_startup_storage_foundation, init_embedded_startup_storage_runtime}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; use std::path::PathBuf; -use std::sync::atomic::{AtomicBool, Ordering}; use tokio_util::sync::CancellationToken; -/// Tracks whether a server has been started in this process. -static SERVER_STARTED: AtomicBool = AtomicBool::new(false); - /// Error type for embedded server operations. #[derive(Debug)] pub enum ServerError { @@ -223,20 +219,7 @@ impl RustFSServerBuilder { // Build is allowed to fail before irreversible global initialization // (for example on temporary I/O or directory setup errors), and in that // case callers can retry. - let mut global_init_started = false; - let mut set_global_init_guard = || -> Result<(), ServerError> { - if global_init_started { - return Ok(()); - } - if SERVER_STARTED - .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) - .is_err() - { - return Err(ServerError::AlreadyStarted); - } - global_init_started = true; - Ok(()) - }; + let mut startup_guard = EmbeddedStartupGuard::new(); let EmbeddedStartupConfig { config, temp_dir_guard } = prepare_embedded_startup_config( self.address.clone(), @@ -258,7 +241,9 @@ impl RustFSServerBuilder { .await .map_err(|e| ServerError::Init(e.to_string()))?; - set_global_init_guard()?; + startup_guard + .mark_global_init_started() + .map_err(|_| ServerError::AlreadyStarted)?; let endpoint_pools = init_embedded_startup_storage_foundation(&listen_context.server_address, &config.volumes) .await @@ -268,11 +253,6 @@ impl RustFSServerBuilder { let shutdown_handle = http_server.shutdown_handle; let bound_addr = http_server.bound_addr; let ctx = CancellationToken::new(); - let shutdown_embedded_server = || { - shutdown_handle.signal(); - ctx.cancel(); - }; - let storage_runtime = match init_embedded_startup_storage_runtime( listen_context.server_addr, &endpoint_pools, @@ -283,7 +263,7 @@ impl RustFSServerBuilder { { Ok(runtime) => runtime, Err(e) => { - shutdown_embedded_server(); + signal_embedded_startup_shutdown(&shutdown_handle, &ctx); return Err(ServerError::Init(e.to_string())); } }; @@ -293,14 +273,14 @@ impl RustFSServerBuilder { init_embedded_startup_runtime_services(&config, endpoint_pools, store, ctx.clone(), listen_context.readiness.clone()) .await .map_err(|e| { - shutdown_embedded_server(); + signal_embedded_startup_shutdown(&shutdown_handle, &ctx); ServerError::Init(e.to_string()) })?; publish_embedded_startup_ready(service_runtime.iam_bootstrap, listen_context.readiness.as_ref()) .await .map_err(|e| { - shutdown_embedded_server(); + signal_embedded_startup_shutdown(&shutdown_handle, &ctx); ServerError::Init(format!("runtime readiness: {e}")) })?; diff --git a/rustfs/src/startup_lifecycle.rs b/rustfs/src/startup_lifecycle.rs index 0b9dd6293..25b34d2b3 100644 --- a/rustfs/src/startup_lifecycle.rs +++ b/rustfs/src/startup_lifecycle.rs @@ -21,7 +21,14 @@ use crate::{ }; use rustfs_common::GlobalReadiness; use rustfs_scanner::init_data_scanner; -use std::{io::Result, net::SocketAddr, sync::Arc}; +use std::{ + io::Result, + net::SocketAddr, + sync::{ + Arc, + atomic::{AtomicBool, Ordering}, + }, +}; use tokio_util::sync::CancellationToken; use tracing::info; @@ -33,6 +40,48 @@ const EVENT_SERVER_READY: &str = "server_ready"; const EVENT_SERVER_SHUTDOWN_STATE: &str = "server_shutdown_state"; const EVENT_EMBEDDED_SERVER_STATE: &str = "embedded_server_state"; +static EMBEDDED_SERVER_STARTED: AtomicBool = AtomicBool::new(false); + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct EmbeddedStartupAlreadyStarted; + +pub(crate) struct EmbeddedStartupGuard { + global_init_started: bool, +} + +impl EmbeddedStartupGuard { + pub(crate) fn new() -> Self { + Self { + global_init_started: false, + } + } + + pub(crate) fn mark_global_init_started(&mut self) -> std::result::Result<(), EmbeddedStartupAlreadyStarted> { + mark_embedded_global_init_started(&EMBEDDED_SERVER_STARTED, &mut self.global_init_started) + } +} + +impl Default for EmbeddedStartupGuard { + fn default() -> Self { + Self::new() + } +} + +fn mark_embedded_global_init_started( + server_started: &AtomicBool, + global_init_started: &mut bool, +) -> std::result::Result<(), EmbeddedStartupAlreadyStarted> { + if *global_init_started { + return Ok(()); + } + + server_started + .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) + .map_err(|_| EmbeddedStartupAlreadyStarted)?; + *global_init_started = true; + Ok(()) +} + pub struct StartupRuntimeLifecycle { pub server_address: String, pub state_manager: Arc, @@ -119,3 +168,34 @@ pub fn log_embedded_server_ready(endpoint_address: SocketAddr) { endpoint_address ); } + +#[cfg(test)] +mod tests { + use super::mark_embedded_global_init_started; + use std::sync::atomic::{AtomicBool, Ordering}; + + #[test] + fn embedded_global_init_guard_allows_local_retry_before_mark() { + let server_started = AtomicBool::new(false); + let mut global_init_started = false; + + mark_embedded_global_init_started(&server_started, &mut global_init_started) + .expect("first irreversible startup should mark global init"); + mark_embedded_global_init_started(&server_started, &mut global_init_started) + .expect("repeated mark in same startup should be idempotent"); + + assert!(global_init_started); + assert!(server_started.load(Ordering::SeqCst)); + } + + #[test] + fn embedded_global_init_guard_rejects_second_startup_after_mark() { + let server_started = AtomicBool::new(true); + let mut global_init_started = false; + + let result = mark_embedded_global_init_started(&server_started, &mut global_init_started); + + assert!(result.is_err()); + assert!(!global_init_started); + } +} diff --git a/rustfs/src/startup_shutdown.rs b/rustfs/src/startup_shutdown.rs index fb8f2724f..2edcba1e4 100644 --- a/rustfs/src/startup_shutdown.rs +++ b/rustfs/src/startup_shutdown.rs @@ -216,6 +216,11 @@ pub async fn run_embedded_shutdown_cleanup() { } } +pub(crate) fn signal_embedded_startup_shutdown(shutdown_handle: &ShutdownHandle, ctx: &CancellationToken) { + shutdown_handle.signal(); + ctx.cancel(); +} + pub async fn run_embedded_server_shutdown( ctx: &CancellationToken, shutdown_handle: &mut Option, @@ -264,7 +269,11 @@ pub async fn run_embedded_server_shutdown( #[cfg(test)] mod tests { - use super::{BackgroundShutdownStep, background_shutdown_steps}; + use super::{BackgroundShutdownStep, background_shutdown_steps, signal_embedded_startup_shutdown}; + use crate::server::ShutdownHandle; + use std::time::Duration; + use tokio::sync::broadcast; + use tokio_util::sync::CancellationToken; #[test] fn background_shutdown_plan_keeps_scanner_before_ahm() { @@ -279,4 +288,21 @@ mod tests { assert_eq!(background_shutdown_steps(false, true), vec![BackgroundShutdownStep::Ahm]); assert!(background_shutdown_steps(false, false).is_empty()); } + + #[tokio::test] + async fn signal_embedded_startup_shutdown_signals_handle_and_cancels_token() { + let (shutdown_tx, mut shutdown_rx) = broadcast::channel(1); + let shutdown_task = tokio::spawn(async move { + let _ = shutdown_rx.recv().await; + }); + let shutdown_handle = ShutdownHandle::new(shutdown_tx, shutdown_task); + let cancel_token = CancellationToken::new(); + + signal_embedded_startup_shutdown(&shutdown_handle, &cancel_token); + + tokio::time::timeout(Duration::from_secs(1), shutdown_handle.wait()) + .await + .expect("shutdown task should observe startup signal"); + assert!(cancel_token.is_cancelled()); + } }