refactor: keep embedded builder as startup shell (#3665)

2026-07-26 08:18:18 +00:00 · 2026-06-20 22:17:31 +08:00
parent 150ac967d3
commit 6d13a0f15a
5 changed files with 116 additions and 46 deletions
@@ -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.
@@ -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<EmbeddedStartupError> for ServerError {
 /// # }
 /// ```
 pub struct RustFSServerBuilder {
-    address: String,
-    access_key: String,
-    secret_key: String,
-    volumes: Vec<String>,
-    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<String>) -> 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<String>) -> 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<String>) -> 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<String>) -> 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<String>) -> 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<String>) -> Self {
-        self.volumes = paths;
+        self.startup_args.volumes = paths;
        self
    }

@@ -219,20 +212,17 @@ impl RustFSServerBuilder {
    }

    async fn do_build(&mut self) -> Result<RustFSServer, ServerError> {
-        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<u16, std::io::Error> {
-    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()
 }
@@ -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<Em
 fn init_error(err: impl std::fmt::Display) -> 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());
+    }
+}
@@ -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<u16> {
+    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<EmbeddedStartupListenContext> {
    let readiness = Arc::new(GlobalReadiness::new());

@@ -279,7 +286,8 @@ fn console_http_server_config(config: &Config) -> Option<Config> {
 #[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(
@@ -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