diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index 5125bed8f..113a8c503 100644 --- a/docs/architecture/migration-progress.md +++ b/docs/architecture/migration-progress.md @@ -5,16 +5,17 @@ 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-handle-boundaries` -- Baseline: completed `R-040/R-041`. -- Stacked on: embedded build orchestration owner slice. +- Branch: `overtrue/arch-embedded-builder-shell` +- Baseline: completed `R-042/R-043`. +- Stacked on: embedded handle boundary slice. - PR type for this branch: `pure-move` - Runtime behavior changes: none. -- Rust code changes: move embedded endpoint normalization and synchronous drop - cleanup behind startup lifecycle/shutdown helpers while preserving the public - embedded server handle behavior. -- CI/script changes: none. -- Docs changes: record the embedded handle boundary slices. +- Rust code changes: keep the public embedded builder as a thin shell over + crate-only startup arguments and move embedded port probing behind + startup-server helpers. +- CI/script changes: make the unsafe allowance checker avoid a local + `pipefail` false positive after `rg -q` matches nearby `SAFETY:` comments. +- Docs changes: record the embedded builder shell slices. ## Phase 0 Tasks @@ -2344,6 +2345,28 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block check, migration/layer guards, formatting, diff hygiene, Rust risk scan, branch freshness check, pre-commit quality gate, and three-expert review. +- [x] `R-044` Keep embedded builder state in startup args. + - Do: replace duplicated public builder private fields with crate-only + embedded startup arguments while preserving the fluent builder API. + - Acceptance: builder defaults, fluent setters, server credential accessors, + region accessors, and startup arguments remain behaviorally unchanged. + - Must preserve: public builder signatures, default address and credentials, + volume replacement semantics, region publication, and error mapping. + - Verification: focused embedded/startup-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-045` Move embedded port probing behind startup server. + - Do: delegate public embedded available-port probing to a crate-only startup + server helper. + - Acceptance: `find_available_port` still returns a bindable localhost TCP + port and preserves the same public result type. + - Must preserve: public helper signature, localhost bind target, ephemeral + port behavior, and no embedded startup side effects. + - Verification: focused startup-server 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 @@ -2586,21 +2609,36 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Next PRs -1. `pure-move`: continue pruning residual embedded public builder-only - boundaries after the embedded handle helper slice lands. +1. `pure-move`: continue pruning residual embedded builder/startup-only + boundaries after the embedded builder shell slice lands. ## Pre-Push Review Log | Expert | Status | Notes | |---|---|---| -| Quality/architecture | passed | R-042 and R-043 move endpoint normalization and synchronous drop cleanup into crate-only startup lifecycle/shutdown helpers while keeping the public embedded API surface unchanged. | -| Migration preservation | passed | Endpoint URL normalization, bound-address accessors, ready logging, async shutdown, drop cancellation, shutdown signaling, and temp-dir cleanup behavior 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. | +| Quality/architecture | passed | R-044 and R-045 keep the public embedded builder as a thin shell over crate-only startup args and move port probing behind startup-server helpers without widening the public API. | +| Migration preservation | passed | Builder defaults, fluent setters, volume replacement, credential/region accessors, public port helper signature, and localhost ephemeral port behavior stay unchanged. | +| Testing/verification | passed | Focused embedded/startup-embedded/startup-server checks, RustFS lib check, architecture/layer/unsafe guards, formatting, diff hygiene, Rust risk scan, and pre-commit gate passed. | ## Verification Notes Passed before push: +- Issue #660 R-044/R-045 current slice: + - `cargo test -p rustfs --lib embedded -- --nocapture`: passed. + - `cargo test -p rustfs --lib startup_embedded -- --nocapture`: passed. + - `cargo test -p rustfs --lib startup_server -- --nocapture`: passed. + - `cargo check -p rustfs --lib`: passed. + - `cargo fmt --all --check`: passed. + - `git diff --check`: passed. + - `./scripts/check_architecture_migration_rules.sh`: passed. + - `./scripts/check_layer_dependencies.sh`: passed. + - Rust risk scan on changed Rust files: passed; matches were limited to + existing doc examples and test-only `expect` calls. + - `./scripts/check_unsafe_code_allowances.sh`: passed after avoiding a local + `pipefail` false positive when `rg -q` finds nearby `SAFETY:` comments. + - `make pre-commit`: passed. + - Issue #660 R-042/R-043 current slice: - `cargo test -p rustfs --lib startup_lifecycle -- --nocapture`: passed. - `cargo test -p rustfs --lib startup_shutdown -- --nocapture`: passed. diff --git a/rustfs/src/embedded.rs b/rustfs/src/embedded.rs index 460dde346..dff02cccc 100644 --- a/rustfs/src/embedded.rs +++ b/rustfs/src/embedded.rs @@ -49,6 +49,7 @@ use crate::server::ShutdownHandle; use crate::startup_embedded::{EmbeddedStartupArgs, EmbeddedStartupError, run_embedded_startup}; use crate::startup_lifecycle::{embedded_endpoint_address, log_embedded_server_ready}; +use crate::startup_server::find_embedded_available_port; use crate::startup_shutdown::{run_embedded_server_drop_cleanup, run_embedded_server_shutdown}; use std::net::SocketAddr; use std::path::PathBuf; @@ -123,11 +124,7 @@ impl From for ServerError { /// # } /// ``` pub struct RustFSServerBuilder { - address: String, - access_key: String, - secret_key: String, - volumes: Vec, - region: String, + startup_args: EmbeddedStartupArgs, } impl Default for RustFSServerBuilder { @@ -149,11 +146,7 @@ impl RustFSServerBuilder { /// not suitable. pub fn new() -> Self { Self { - address: "127.0.0.1:9000".to_string(), - access_key: rustfs_credentials::DEFAULT_ACCESS_KEY.to_string(), - secret_key: rustfs_credentials::DEFAULT_SECRET_KEY.to_string(), - volumes: Vec::new(), - region: rustfs_config::RUSTFS_REGION.to_string(), + startup_args: EmbeddedStartupArgs::new_default(), } } @@ -167,25 +160,25 @@ impl RustFSServerBuilder { /// [`build`](Self::build), but that is too late for the earlier /// initialization that depends on the configured address. pub fn address(mut self, addr: impl Into) -> Self { - self.address = addr.into(); + self.startup_args.address = addr.into(); self } /// Set the S3 access key (default: `"rustfsadmin"`). pub fn access_key(mut self, key: impl Into) -> Self { - self.access_key = key.into(); + self.startup_args.access_key = key.into(); self } /// Set the S3 secret key (default: `"rustfsadmin"`). pub fn secret_key(mut self, key: impl Into) -> Self { - self.secret_key = key.into(); + self.startup_args.secret_key = key.into(); self } /// Set the AWS region (default: `"us-east-1"`). pub fn region(mut self, region: impl Into) -> Self { - self.region = region.into(); + self.startup_args.region = region.into(); self } @@ -194,13 +187,13 @@ impl RustFSServerBuilder { /// If no volumes are added, a temporary directory with a single drive is /// created automatically (and cleaned up on [`RustFSServer::shutdown`]). pub fn volume(mut self, path: impl Into) -> Self { - self.volumes.push(path.into()); + self.startup_args.volumes.push(path.into()); self } /// Set multiple volume paths at once, replacing any previously set volumes. pub fn volumes(mut self, paths: Vec) -> Self { - self.volumes = paths; + self.startup_args.volumes = paths; self } @@ -219,20 +212,17 @@ impl RustFSServerBuilder { } async fn do_build(&mut self) -> Result { - let started = run_embedded_startup(EmbeddedStartupArgs { - address: self.address.clone(), - access_key: self.access_key.clone(), - secret_key: self.secret_key.clone(), - volumes: self.volumes.clone(), - region: self.region.clone(), - }) - .await?; + let startup_args = self.startup_args.clone(); + let access_key = startup_args.access_key.clone(); + let secret_key = startup_args.secret_key.clone(); + let region = startup_args.region.clone(); + let started = run_embedded_startup(startup_args).await?; let server = RustFSServer { address: started.bound_addr, - access_key: self.access_key.clone(), - secret_key: self.secret_key.clone(), - region: self.region.clone(), + access_key, + secret_key, + region, shutdown_handle: Some(started.shutdown_handle), cancel_token: started.cancel_token, temp_dir: started.temp_dir, @@ -331,8 +321,5 @@ impl Drop for RustFSServer { /// } /// ``` pub fn find_available_port() -> Result { - let listener = std::net::TcpListener::bind("127.0.0.1:0")?; - let port = listener.local_addr()?.port(); - drop(listener); - Ok(port) + find_embedded_available_port() } diff --git a/rustfs/src/startup_embedded.rs b/rustfs/src/startup_embedded.rs index d06d1e82b..7bba978c1 100644 --- a/rustfs/src/startup_embedded.rs +++ b/rustfs/src/startup_embedded.rs @@ -26,6 +26,7 @@ use crate::{ use std::{io, net::SocketAddr, path::PathBuf}; use tokio_util::sync::CancellationToken; +#[derive(Clone)] pub(crate) struct EmbeddedStartupArgs { pub address: String, pub access_key: String, @@ -34,6 +35,18 @@ pub(crate) struct EmbeddedStartupArgs { pub region: String, } +impl EmbeddedStartupArgs { + pub(crate) fn new_default() -> Self { + Self { + address: "127.0.0.1:9000".to_string(), + access_key: rustfs_credentials::DEFAULT_ACCESS_KEY.to_string(), + secret_key: rustfs_credentials::DEFAULT_SECRET_KEY.to_string(), + volumes: Vec::new(), + region: rustfs_config::RUSTFS_REGION.to_string(), + } + } +} + pub(crate) struct EmbeddedStartedServer { pub bound_addr: SocketAddr, pub shutdown_handle: ShutdownHandle, @@ -130,3 +143,19 @@ pub(crate) async fn run_embedded_startup(args: EmbeddedStartupArgs) -> Result EmbeddedStartupError { EmbeddedStartupError::Init(err.to_string()) } + +#[cfg(test)] +mod tests { + use super::EmbeddedStartupArgs; + + #[test] + fn embedded_startup_args_default_matches_public_builder_defaults() { + let args = EmbeddedStartupArgs::new_default(); + + assert_eq!(args.address, "127.0.0.1:9000"); + assert_eq!(args.access_key, rustfs_credentials::DEFAULT_ACCESS_KEY); + assert_eq!(args.secret_key, rustfs_credentials::DEFAULT_SECRET_KEY); + assert_eq!(args.region, rustfs_config::RUSTFS_REGION); + assert!(args.volumes.is_empty()); + } +} diff --git a/rustfs/src/startup_server.rs b/rustfs/src/startup_server.rs index baab7232b..054cb5176 100644 --- a/rustfs/src/startup_server.rs +++ b/rustfs/src/startup_server.rs @@ -149,6 +149,13 @@ pub(crate) async fn prepare_embedded_startup_config( Ok(EmbeddedStartupConfig { config, temp_dir_guard }) } +pub(crate) fn find_embedded_available_port() -> Result { + let listener = std::net::TcpListener::bind("127.0.0.1:0")?; + let port = listener.local_addr()?.port(); + drop(listener); + Ok(port) +} + pub async fn init_embedded_startup_listen_context(config: &Config) -> Result { let readiness = Arc::new(GlobalReadiness::new()); @@ -279,7 +286,8 @@ fn console_http_server_config(config: &Config) -> Option { #[cfg(test)] mod tests { use super::{ - DEFAULT_CREDENTIALS_WARNING_MESSAGE, console_http_server_config, prepare_embedded_startup_config, s3_http_server_config, + DEFAULT_CREDENTIALS_WARNING_MESSAGE, console_http_server_config, find_embedded_available_port, + prepare_embedded_startup_config, s3_http_server_config, }; use crate::config::Config; @@ -349,6 +357,13 @@ mod tests { assert!(!DEFAULT_CREDENTIALS_WARNING_MESSAGE.contains(rustfs_credentials::DEFAULT_SECRET_KEY)); } + #[test] + fn find_embedded_available_port_returns_tcp_port() { + let port = find_embedded_available_port().expect("available port should be found"); + + assert_ne!(port, 0); + } + #[tokio::test] async fn prepare_embedded_startup_config_creates_temp_volume_when_missing() { let prepared = prepare_embedded_startup_config( diff --git a/scripts/check_unsafe_code_allowances.sh b/scripts/check_unsafe_code_allowances.sh index 3f65199c9..6ab9d9cd8 100755 --- a/scripts/check_unsafe_code_allowances.sh +++ b/scripts/check_unsafe_code_allowances.sh @@ -11,7 +11,8 @@ status=0 while IFS=: read -r file line _; do start=$((line > 3 ? line - 3 : 1)) end=$((line + 6)) - if ! sed -n "${start},${end}p" "$file" | rg -q "SAFETY:"; then + window="$(sed -n "${start},${end}p" "$file")" + if ! rg -q "SAFETY:" <<<"$window"; then printf '%s:%s: unsafe_code allowance must have a nearby SAFETY comment\n' "$file" "$line" >&2 status=1 fi