From 150ac967d3114d02e2b2a411e9b22db19fce8217 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Sat, 20 Jun 2026 22:12:50 +0800 Subject: [PATCH] refactor: extract embedded handle boundaries (#3664) --- docs/architecture/migration-progress.md | 63 ++++++++++++++++++++----- rustfs/src/embedded.rs | 24 ++-------- rustfs/src/startup_lifecycle.rs | 44 +++++++++++++++-- rustfs/src/startup_shutdown.rs | 43 ++++++++++++++++- 4 files changed, 139 insertions(+), 35 deletions(-) diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index bbd7e8dc9..5125bed8f 100644 --- a/docs/architecture/migration-progress.md +++ b/docs/architecture/migration-progress.md @@ -5,16 +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-build-orchestration` -- Baseline: completed `R-038/R-039`. -- Stacked on: embedded startup control helper extraction slice. +- Branch: `overtrue/arch-embedded-handle-boundaries` +- Baseline: completed `R-040/R-041`. +- Stacked on: embedded build orchestration owner slice. - PR type for this branch: `pure-move` - Runtime behavior changes: none. -- Rust code changes: move the embedded build startup sequence into a crate-only - startup owner while preserving the public embedded builder and server handle - behavior. +- 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 build orchestration owner slices. +- Docs changes: record the embedded handle boundary slices. ## Phase 0 Tasks @@ -2320,6 +2320,30 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block guards, formatting, diff hygiene, Rust risk scan, branch freshness check, pre-commit quality gate, and three-expert review. +- [x] `R-042` Extract embedded endpoint normalization. + - Do: move unspecified-address endpoint normalization into a crate-only + startup lifecycle helper. + - Acceptance: embedded endpoint reporting still rewrites unspecified IPv4 and + IPv6 bind addresses to localhost while preserving concrete bound hosts. + - Must preserve: public endpoint URL shape, `address()` returning the bound + socket address, ready-log endpoint text, and no public embedded API + signature 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-043` Extract embedded drop cleanup boundary. + - Do: move synchronous embedded server drop cleanup into a crate-only startup + shutdown helper. + - Acceptance: dropping a server still cancels the token, signals the shutdown + handle, and best-effort removes the temporary directory. + - Must preserve: explicit async shutdown behavior, shutdown handle ownership, + temp-dir cleanup behavior, ignored drop cleanup errors, and no public + embedded API signature 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 @@ -2562,21 +2586,36 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Next PRs -1. `pure-move`: continue pruning residual embedded shutdown/handle-only - boundaries after the embedded build orchestration helper lands. +1. `pure-move`: continue pruning residual embedded public builder-only + boundaries after the embedded handle helper slice lands. ## Pre-Push Review Log | Expert | Status | Notes | |---|---|---| -| Quality/architecture | passed | R-040 and R-041 move embedded startup orchestration into a crate-only startup owner while keeping `embedded.rs` focused on public API and handle behavior. | -| Migration preservation | passed | Initialization order, startup guard timing, temp-dir lifetime, post-HTTP shutdown signaling, error mapping, endpoint normalization, and shutdown/drop behavior stay unchanged. | -| Testing/verification | passed | Focused embedded checks, RustFS lib check, architecture/layer guards, formatting, diff hygiene, Rust risk scan, and pre-commit gate passed. | +| 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. | ## Verification Notes Passed before push: +- 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. + - `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: passed; the only production risky + token was the intended move of embedded drop `remove_dir_all` cleanup from + the public embedded handle into `startup_shutdown`. + - `./scripts/check_unsafe_code_allowances.sh`: passed. + - `make pre-commit`: passed. + - Issue #660 R-040/R-041 current slice: - `cargo test -p rustfs --lib embedded -- --nocapture`: passed. - `cargo check -p rustfs --lib`: passed. diff --git a/rustfs/src/embedded.rs b/rustfs/src/embedded.rs index 1161f78dd..460dde346 100644 --- a/rustfs/src/embedded.rs +++ b/rustfs/src/embedded.rs @@ -48,9 +48,9 @@ use crate::server::ShutdownHandle; use crate::startup_embedded::{EmbeddedStartupArgs, EmbeddedStartupError, run_embedded_startup}; -use crate::startup_lifecycle::log_embedded_server_ready; -use crate::startup_shutdown::run_embedded_server_shutdown; -use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; +use crate::startup_lifecycle::{embedded_endpoint_address, log_embedded_server_ready}; +use crate::startup_shutdown::{run_embedded_server_drop_cleanup, run_embedded_server_shutdown}; +use std::net::SocketAddr; use std::path::PathBuf; use tokio_util::sync::CancellationToken; @@ -263,14 +263,7 @@ pub struct RustFSServer { impl RustFSServer { fn endpoint_address(&self) -> SocketAddr { - let ip = match self.address.ip() { - ip @ IpAddr::V4(v4) if !v4.is_unspecified() => ip, - IpAddr::V4(_) => IpAddr::V4(Ipv4Addr::LOCALHOST), - ip @ IpAddr::V6(v6) if !v6.is_unspecified() => ip, - IpAddr::V6(_) => IpAddr::V6(Ipv6Addr::LOCALHOST), - }; - - SocketAddr::new(ip, self.address.port()) + embedded_endpoint_address(self.address) } /// The HTTP endpoint URL (e.g. `"http://127.0.0.1:54321"`). @@ -312,14 +305,7 @@ impl RustFSServer { impl Drop for RustFSServer { fn drop(&mut self) { - // Best-effort synchronous cleanup. - self.cancel_token.cancel(); - if let Some(shutdown_handle) = self.shutdown_handle.take() { - shutdown_handle.signal(); - } - if let Some(ref dir) = self.temp_dir { - let _ = std::fs::remove_dir_all(dir); - } + run_embedded_server_drop_cleanup(&self.cancel_token, &mut self.shutdown_handle, self.temp_dir.as_deref()); } } diff --git a/rustfs/src/startup_lifecycle.rs b/rustfs/src/startup_lifecycle.rs index 25b34d2b3..dbd0e5600 100644 --- a/rustfs/src/startup_lifecycle.rs +++ b/rustfs/src/startup_lifecycle.rs @@ -23,7 +23,7 @@ use rustfs_common::GlobalReadiness; use rustfs_scanner::init_data_scanner; use std::{ io::Result, - net::SocketAddr, + net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, sync::{ Arc, atomic::{AtomicBool, Ordering}, @@ -157,6 +157,17 @@ pub async fn publish_embedded_startup_ready(iam_bootstrap: IamBootstrapDispositi Ok(()) } +pub(crate) fn embedded_endpoint_address(address: SocketAddr) -> SocketAddr { + let ip = match address.ip() { + ip @ IpAddr::V4(v4) if !v4.is_unspecified() => ip, + IpAddr::V4(_) => IpAddr::V4(Ipv4Addr::LOCALHOST), + ip @ IpAddr::V6(v6) if !v6.is_unspecified() => ip, + IpAddr::V6(_) => IpAddr::V6(Ipv6Addr::LOCALHOST), + }; + + SocketAddr::new(ip, address.port()) +} + pub fn log_embedded_server_ready(endpoint_address: SocketAddr) { info!( target: "rustfs::embedded", @@ -171,8 +182,11 @@ pub fn log_embedded_server_ready(endpoint_address: SocketAddr) { #[cfg(test)] mod tests { - use super::mark_embedded_global_init_started; - use std::sync::atomic::{AtomicBool, Ordering}; + use super::{embedded_endpoint_address, mark_embedded_global_init_started}; + use std::{ + net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}, + sync::atomic::{AtomicBool, Ordering}, + }; #[test] fn embedded_global_init_guard_allows_local_retry_before_mark() { @@ -198,4 +212,28 @@ mod tests { assert!(result.is_err()); assert!(!global_init_started); } + + #[test] + fn embedded_endpoint_address_rewrites_unspecified_hosts() { + assert_eq!( + embedded_endpoint_address(SocketAddr::new(IpAddr::V4(Ipv4Addr::UNSPECIFIED), 9000)), + SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 9000) + ); + assert_eq!( + embedded_endpoint_address(SocketAddr::new(IpAddr::V6(Ipv6Addr::UNSPECIFIED), 9001)), + SocketAddr::new(IpAddr::V6(Ipv6Addr::LOCALHOST), 9001) + ); + } + + #[test] + fn embedded_endpoint_address_preserves_bound_hosts() { + assert_eq!( + embedded_endpoint_address(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(192, 0, 2, 10)), 9000)), + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(192, 0, 2, 10)), 9000) + ); + assert_eq!( + embedded_endpoint_address(SocketAddr::new(IpAddr::V6(Ipv6Addr::LOCALHOST), 9001)), + SocketAddr::new(IpAddr::V6(Ipv6Addr::LOCALHOST), 9001) + ); + } } diff --git a/rustfs/src/startup_shutdown.rs b/rustfs/src/startup_shutdown.rs index 2edcba1e4..9e2bd8dca 100644 --- a/rustfs/src/startup_shutdown.rs +++ b/rustfs/src/startup_shutdown.rs @@ -221,6 +221,20 @@ pub(crate) fn signal_embedded_startup_shutdown(shutdown_handle: &ShutdownHandle, ctx.cancel(); } +pub(crate) fn run_embedded_server_drop_cleanup( + ctx: &CancellationToken, + shutdown_handle: &mut Option, + temp_dir: Option<&Path>, +) { + ctx.cancel(); + if let Some(shutdown_handle) = shutdown_handle.take() { + shutdown_handle.signal(); + } + if let Some(dir) = temp_dir { + let _ = std::fs::remove_dir_all(dir); + } +} + pub async fn run_embedded_server_shutdown( ctx: &CancellationToken, shutdown_handle: &mut Option, @@ -269,7 +283,9 @@ pub async fn run_embedded_server_shutdown( #[cfg(test)] mod tests { - use super::{BackgroundShutdownStep, background_shutdown_steps, signal_embedded_startup_shutdown}; + use super::{ + BackgroundShutdownStep, background_shutdown_steps, run_embedded_server_drop_cleanup, signal_embedded_startup_shutdown, + }; use crate::server::ShutdownHandle; use std::time::Duration; use tokio::sync::broadcast; @@ -305,4 +321,29 @@ mod tests { .expect("shutdown task should observe startup signal"); assert!(cancel_token.is_cancelled()); } + + #[tokio::test] + async fn embedded_drop_cleanup_signals_handle_cancels_token_and_removes_temp_dir() { + let (shutdown_tx, mut shutdown_rx) = broadcast::channel(1); + let (observed_tx, observed_rx) = tokio::sync::oneshot::channel(); + let shutdown_task = tokio::spawn(async move { + let _ = shutdown_rx.recv().await; + let _ = observed_tx.send(()); + }); + let shutdown_handle = ShutdownHandle::new(shutdown_tx, shutdown_task); + let mut shutdown_handle = Some(shutdown_handle); + let cancel_token = CancellationToken::new(); + let temp_dir = tempfile::tempdir().expect("temp dir should create"); + let temp_path = temp_dir.path().to_path_buf(); + + run_embedded_server_drop_cleanup(&cancel_token, &mut shutdown_handle, Some(temp_dir.path())); + + tokio::time::timeout(Duration::from_secs(1), observed_rx) + .await + .expect("drop cleanup should signal shutdown") + .expect("shutdown signal should be delivered"); + assert!(cancel_token.is_cancelled()); + assert!(shutdown_handle.is_none()); + assert!(!temp_path.exists()); + } }