fix: keep scanner walk timeouts from offlining drives (#2996)

* fix: keep scanner walk timeouts from offlining drives Scanner walk operations can time out on large or slow directory listings without proving the backing drive is faulty. Keep the timeout error local to the scan while preserving failure marking for ordinary disk operations. Constraint: Scanner walk_dir can include listing work that exceeds the drive timeout under slow storage. Rejected: Disable timeout failure marking globally | real stuck disk operations must still affect drive health Confidence: high Scope-risk: narrow Directive: Do not route scanner/listing timeout back into drive offline state without reproducing issue #2651 Tested: cargo test -p rustfs-ecstore timeout Tested: cargo fmt --all --check Tested: cargo clippy --workspace --all-features --all-targets -- -D warnings Tested: make pre-commit with en_US.UTF-8 locale Related: https://github.com/rustfs/rustfs/issues/2651 * fix: keep remote scanner walks from offlining drives Remote walk_dir uses a streaming request that can hit total or stall timeouts during large scans without proving the remote drive is faulty. Route that scanner path through an explicit ignore action while preserving failure marking for ordinary remote operations. Also treat Duration::ZERO as no operation timeout for remote health tracking, matching the existing local disk wrapper contract while still marking network-like errors on default paths. Constraint: Scanner walk_dir streams can be slow because of directory size or consumer backpressure. Rejected: Disable remote timeout failure marking globally | normal remote disk operations still need to evict failed connections and update drive health Confidence: high Scope-risk: narrow Directive: Do not reintroduce remote scanner timeout failure marking without reproducing issue #2651 against remote disks. Tested: cargo test -p rustfs-ecstore execute_with_timeout Tested: cargo test -p rustfs-ecstore timeout Tested: cargo fmt --all --check Tested: cargo clippy --workspace --all-features --all-targets -- -D warnings Tested: make pre-commit with en_US.UTF-8 locale Related: https://github.com/rustfs/rustfs/issues/2651 * test: prove scanner walk backpressure keeps drives online Add a local scanner walk regression test where the output writer never makes progress. The test confirms the walk timeout returns without marking the drive faulty, covering a stall source that is not caused by object count or network transfer speed. Constraint: Scanner walk can block on downstream writer backpressure as well as disk or network IO.\nConfidence: high\nScope-risk: narrow\nTested: cargo test -p rustfs-ecstore walk_dir_writer_backpressure_timeout_does_not_mark_drive_failure\nTested: cargo test -p rustfs-ecstore timeout\nTested: cargo fmt --all --check\nTested: cargo clippy --workspace --all-features --all-targets -- -D warnings\nTested: make pre-commit --------- Co-authored-by: houseme <housemecn@gmail.com>
2026-07-26 16:28:15 +00:00 · 2026-05-18 23:04:05 +08:00
parent bd1e57293f
commit a66337bd28
2 changed files with 341 additions and 23 deletions
@@ -44,6 +44,12 @@ use uuid::Uuid;
 const DISK_HEALTH_OK: u32 = 0;
 const DISK_HEALTH_FAULTY: u32 = 1;

+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+enum TimeoutHealthAction {
+    MarkFailure,
+    IgnoreFailure,
+}
+
 pub const ENV_RUSTFS_DRIVE_ACTIVE_MONITORING: &str = "RUSTFS_DRIVE_ACTIVE_MONITORING";
 pub const DEFAULT_RUSTFS_DRIVE_ACTIVE_MONITORING: bool = true;
 pub const SKIP_IF_SUCCESS_BEFORE: Duration = Duration::from_secs(5);
@@ -804,6 +810,21 @@ impl LocalDiskWrapper {
        operation: F,
        timeout_duration: Duration,
    ) -> Result<T>
+    where
+        F: FnOnce() -> Fut,
+        Fut: std::future::Future<Output = Result<T>>,
+    {
+        self.track_disk_health_with_op_and_timeout_action(op, operation, timeout_duration, TimeoutHealthAction::MarkFailure)
+            .await
+    }
+
+    async fn track_disk_health_with_op_and_timeout_action<T, F, Fut>(
+        &self,
+        op: &'static str,
+        operation: F,
+        timeout_duration: Duration,
+        timeout_health_action: TimeoutHealthAction,
+    ) -> Result<T>
    where
        F: FnOnce() -> Fut,
        Fut: std::future::Future<Output = Result<T>>,
@@ -848,7 +869,9 @@ impl LocalDiskWrapper {
            Err(_) => {
                // Timeout occurred, mark disk as potentially faulty and decrement waiting counter
                self.health.decrement_waiting();
-                if self.health.mark_failure(&self.endpoint(), "operation_timeout") {
+                if timeout_health_action == TimeoutHealthAction::MarkFailure
+                    && self.health.mark_failure(&self.endpoint(), "operation_timeout")
+                {
                    self.spawn_recovery_monitor_if_needed();
                }
                counter!(
@@ -996,8 +1019,13 @@ impl DiskAPI for LocalDiskWrapper {
    }

    async fn walk_dir<W: tokio::io::AsyncWrite + Unpin + Send>(&self, opts: WalkDirOptions, wr: &mut W) -> Result<()> {
-        self.track_disk_health_with_op("walk_dir", || async { self.disk.walk_dir(opts, wr).await }, get_drive_walkdir_timeout())
-            .await
+        self.track_disk_health_with_op_and_timeout_action(
+            "walk_dir",
+            || async { self.disk.walk_dir(opts, wr).await },
+            get_drive_walkdir_timeout(),
+            TimeoutHealthAction::IgnoreFailure,
+        )
+        .await
    }

    async fn delete_version(
@@ -1203,6 +1231,28 @@ mod tests {
    use super::*;
    use crate::disk::endpoint::Endpoint;
    use crate::disk::health_state::RuntimeDriveHealthState;
+    use std::{
+        io,
+        pin::Pin,
+        task::{Context, Poll},
+    };
+    use tokio::io::AsyncWrite;
+
+    struct PendingWriter;
+
+    impl AsyncWrite for PendingWriter {
+        fn poll_write(self: Pin<&mut Self>, _cx: &mut Context<'_>, _buf: &[u8]) -> Poll<io::Result<usize>> {
+            Poll::Pending
+        }
+
+        fn poll_flush(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<io::Result<()>> {
+            Poll::Ready(Ok(()))
+        }
+
+        fn poll_shutdown(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<io::Result<()>> {
+            Poll::Ready(Ok(()))
+        }
+    }

    #[test]
    fn drive_metadata_timeout_uses_default_when_unset() {
@@ -1323,6 +1373,97 @@ mod tests {
        assert!(health.offline_duration().is_none());
    }

+    #[tokio::test]
+    async fn ignored_timeout_does_not_mark_drive_failure() {
+        let dir = tempfile::tempdir().expect("temp dir should be created");
+        let endpoint =
+            Endpoint::try_from(dir.path().to_str().expect("temp dir should be valid UTF-8")).expect("endpoint should parse");
+        let disk = Arc::new(LocalDisk::new(&endpoint, false).await.expect("local disk should be created"));
+        let wrapper = LocalDiskWrapper::new(disk, false);
+
+        let result = wrapper
+            .track_disk_health_with_op_and_timeout_action(
+                "walk_dir",
+                || async {
+                    tokio::time::sleep(Duration::from_millis(20)).await;
+                    Ok(())
+                },
+                Duration::from_millis(1),
+                TimeoutHealthAction::IgnoreFailure,
+            )
+            .await;
+
+        assert_eq!(result.expect_err("operation should time out"), DiskError::Timeout);
+        assert_eq!(wrapper.runtime_state(), RuntimeDriveHealthState::Online);
+        assert!(!wrapper.health.is_faulty());
+    }
+
+    #[tokio::test]
+    async fn walk_dir_writer_backpressure_timeout_does_not_mark_drive_failure() {
+        temp_env::async_with_vars([(rustfs_config::ENV_DRIVE_WALKDIR_TIMEOUT_SECS, Some("1"))], async {
+            let dir = tempfile::tempdir().expect("temp dir should be created");
+            let endpoint =
+                Endpoint::try_from(dir.path().to_str().expect("temp dir should be valid UTF-8")).expect("endpoint should parse");
+            let disk = Arc::new(LocalDisk::new(&endpoint, false).await.expect("local disk should be created"));
+            let wrapper = LocalDiskWrapper::new(disk, false);
+            let bucket = "test-bucket";
+            let object = "test-object";
+
+            wrapper.make_volume(bucket).await.expect("bucket should be created");
+
+            let mut file_info = FileInfo::new(&format!("{bucket}/{object}"), 1, 0);
+            file_info.volume = bucket.to_string();
+            file_info.name = object.to_string();
+            file_info.mod_time = Some(::time::OffsetDateTime::now_utc());
+            file_info.erasure.index = 1;
+
+            wrapper
+                .write_metadata("", bucket, object, file_info)
+                .await
+                .expect("object metadata should be written");
+
+            let mut writer = PendingWriter;
+            let result = wrapper
+                .walk_dir(
+                    WalkDirOptions {
+                        bucket: bucket.to_string(),
+                        recursive: true,
+                        ..Default::default()
+                    },
+                    &mut writer,
+                )
+                .await;
+
+            assert_eq!(result.expect_err("walk_dir should time out"), DiskError::Timeout);
+            assert_eq!(wrapper.runtime_state(), RuntimeDriveHealthState::Online);
+            assert!(!wrapper.health.is_faulty());
+        })
+        .await;
+    }
+
+    #[tokio::test]
+    async fn default_timeout_marks_drive_failure() {
+        let dir = tempfile::tempdir().expect("temp dir should be created");
+        let endpoint =
+            Endpoint::try_from(dir.path().to_str().expect("temp dir should be valid UTF-8")).expect("endpoint should parse");
+        let disk = Arc::new(LocalDisk::new(&endpoint, false).await.expect("local disk should be created"));
+        let wrapper = LocalDiskWrapper::new(disk, false);
+
+        let result = wrapper
+            .track_disk_health_with_op(
+                "read_metadata",
+                || async {
+                    tokio::time::sleep(Duration::from_millis(20)).await;
+                    Ok(())
+                },
+                Duration::from_millis(1),
+            )
+            .await;
+
+        assert_eq!(result.expect_err("operation should time out"), DiskError::Timeout);
+        assert_eq!(wrapper.runtime_state(), RuntimeDriveHealthState::Suspect);
+    }
+
    #[test]
    fn reset_for_store_init_retry_clears_faulty_and_back_online() {
        let endpoint = Endpoint::try_from("/tmp/reset-store-init-retry").expect("endpoint should parse");
@@ -68,6 +68,12 @@ use tonic::{Request, service::interceptor::InterceptedService, transport::Channe
 use tracing::{debug, info, warn};
 use uuid::Uuid;

+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+enum FailureHealthAction {
+    MarkFailure,
+    IgnoreFailure,
+}
+
 async fn copy_stream_with_buffer<R, W>(reader: &mut R, writer: &mut W, buffer_size: usize) -> io::Result<u64>
 where
    R: AsyncRead + Unpin,
@@ -320,6 +326,21 @@ impl RemoteDisk {
        operation: F,
        timeout_duration: Duration,
    ) -> Result<T>
+    where
+        F: FnOnce() -> Fut,
+        Fut: std::future::Future<Output = Result<T>>,
+    {
+        self.execute_with_timeout_for_op_and_health_action(op, operation, timeout_duration, FailureHealthAction::MarkFailure)
+            .await
+    }
+
+    async fn execute_with_timeout_for_op_and_health_action<T, F, Fut>(
+        &self,
+        op: &'static str,
+        operation: F,
+        timeout_duration: Duration,
+        failure_health_action: FailureHealthAction,
+    ) -> Result<T>
    where
        F: FnOnce() -> Fut,
        Fut: std::future::Future<Output = Result<T>>,
@@ -338,6 +359,17 @@ impl RemoteDisk {
        self.health.last_started.store(now, std::sync::atomic::Ordering::Relaxed);
        self.health.increment_waiting();

+        if timeout_duration == Duration::ZERO {
+            let operation_result = operation().await;
+            if operation_result.is_ok() {
+                self.health.log_success();
+            }
+            self.health.decrement_waiting();
+            self.handle_network_like_error(op, timeout_duration, &operation_result, failure_health_action)
+                .await;
+            return operation_result;
+        }
+
        // Execute operation with timeout
        let result = time::timeout(timeout_duration, operation()).await;

@@ -348,24 +380,8 @@ impl RemoteDisk {
                    self.health.log_success();
                }
                self.health.decrement_waiting();
-                if let Err(err) = &operation_result
-                    && is_network_like_disk_error(err)
-                {
-                    counter!(
-                        "rustfs_drive_op_network_error_total",
-                        "endpoint" => self.endpoint.to_string(),
-                        "op" => op.to_string()
-                    )
-                    .increment(1);
-                    warn!(
-                        endpoint = %self.endpoint,
-                        addr = %self.addr,
-                        op,
-                        timeout_ms = timeout_duration.as_millis(),
-                        "Remote disk operation returned a network-like error"
-                    );
-                    self.mark_faulty_and_evict("operation_network_error").await;
-                }
+                self.handle_network_like_error(op, timeout_duration, &operation_result, failure_health_action)
+                    .await;
                operation_result
            }
            Err(_) => {
@@ -377,7 +393,9 @@ impl RemoteDisk {
                    "op" => op.to_string()
                )
                .increment(1);
-                self.mark_faulty_and_evict("operation_timeout").await;
+                if failure_health_action == FailureHealthAction::MarkFailure {
+                    self.mark_faulty_and_evict("operation_timeout").await;
+                }
                warn!(
                    endpoint = %self.endpoint,
                    addr = %self.addr,
@@ -390,6 +408,35 @@ impl RemoteDisk {
        }
    }

+    async fn handle_network_like_error<T>(
+        &self,
+        op: &'static str,
+        timeout_duration: Duration,
+        operation_result: &Result<T>,
+        failure_health_action: FailureHealthAction,
+    ) {
+        if let Err(err) = operation_result
+            && is_network_like_disk_error(err)
+        {
+            counter!(
+                "rustfs_drive_op_network_error_total",
+                "endpoint" => self.endpoint.to_string(),
+                "op" => op.to_string()
+            )
+            .increment(1);
+            warn!(
+                endpoint = %self.endpoint,
+                addr = %self.addr,
+                op,
+                timeout_ms = timeout_duration.as_millis(),
+                "Remote disk operation returned a network-like error"
+            );
+            if failure_health_action == FailureHealthAction::MarkFailure {
+                self.mark_faulty_and_evict("operation_network_error").await;
+            }
+        }
+    }
+
    async fn mark_faulty_and_evict(&self, reason: &'static str) {
        if self.health.mark_offline(&self.endpoint, reason) {
            self.spawn_recovery_monitor_if_needed();
@@ -1146,7 +1193,8 @@ impl DiskAPI for RemoteDisk {
    async fn walk_dir<W: AsyncWrite + Unpin + Send>(&self, opts: WalkDirOptions, wr: &mut W) -> Result<()> {
        info!("walk_dir {}", self.endpoint.to_string());

-        self.execute_with_timeout(
+        self.execute_with_timeout_for_op_and_health_action(
+            "walk_dir",
            || async {
                let disk = self.disk_ref().await;

@@ -1172,6 +1220,7 @@ impl DiskAPI for RemoteDisk {
                Ok(())
            },
            get_drive_walkdir_timeout(),
+            FailureHealthAction::IgnoreFailure,
        )
        .await
    }
@@ -1994,6 +2043,82 @@ mod tests {
        assert!(!remote_disk.is_online().await, "remote disk should be marked faulty after timeout");
    }

+    #[tokio::test]
+    async fn test_execute_with_timeout_can_ignore_remote_timeout_failure() {
+        let url = url::Url::parse("http://remote-timeout-ignored:9000").unwrap();
+        let endpoint = Endpoint {
+            url,
+            is_local: false,
+            pool_idx: 0,
+            set_idx: 0,
+            disk_idx: 0,
+        };
+
+        let remote_disk = RemoteDisk::new(
+            &endpoint,
+            &DiskOption {
+                cleanup: false,
+                health_check: false,
+            },
+        )
+        .await
+        .unwrap();
+
+        let err = remote_disk
+            .execute_with_timeout_for_op_and_health_action(
+                "walk_dir",
+                || async {
+                    tokio::time::sleep(Duration::from_millis(50)).await;
+                    Ok::<(), Error>(())
+                },
+                Duration::from_millis(10),
+                FailureHealthAction::IgnoreFailure,
+            )
+            .await
+            .expect_err("timeout should fail");
+
+        assert!(err.to_string().contains("timeout"));
+        assert!(remote_disk.is_online().await, "ignored timeout should not mark remote disk faulty");
+    }
+
+    #[tokio::test]
+    async fn test_execute_with_timeout_zero_duration_waits_for_operation() {
+        let url = url::Url::parse("http://remote-no-timeout:9000").unwrap();
+        let endpoint = Endpoint {
+            url,
+            is_local: false,
+            pool_idx: 0,
+            set_idx: 0,
+            disk_idx: 0,
+        };
+
+        let remote_disk = RemoteDisk::new(
+            &endpoint,
+            &DiskOption {
+                cleanup: false,
+                health_check: false,
+            },
+        )
+        .await
+        .unwrap();
+
+        remote_disk
+            .execute_with_timeout(
+                || async {
+                    tokio::time::sleep(Duration::from_millis(10)).await;
+                    Ok::<(), Error>(())
+                },
+                Duration::ZERO,
+            )
+            .await
+            .expect("zero duration should disable the operation timeout");
+
+        assert!(
+            remote_disk.is_online().await,
+            "successful no-timeout operation should keep remote disk online"
+        );
+    }
+
    #[tokio::test]
    async fn test_execute_with_timeout_evicts_cached_connection() {
        let addr = "http://127.0.0.1:59991".to_string();
@@ -2136,6 +2261,58 @@ mod tests {
        );
    }

+    #[tokio::test]
+    async fn test_execute_with_timeout_can_ignore_network_like_error() {
+        let addr = "http://127.0.0.1:59995".to_string();
+        let url = url::Url::parse(&format!("{addr}/data")).unwrap();
+        let endpoint = Endpoint {
+            url,
+            is_local: false,
+            pool_idx: 0,
+            set_idx: 0,
+            disk_idx: 0,
+        };
+
+        let remote_disk = RemoteDisk::new(
+            &endpoint,
+            &DiskOption {
+                cleanup: false,
+                health_check: false,
+            },
+        )
+        .await
+        .unwrap();
+
+        let channel = TonicEndpoint::from_shared(addr.clone()).unwrap().connect_lazy();
+        GLOBAL_CONN_MAP.write().await.insert(addr.clone(), channel);
+
+        let err = remote_disk
+            .execute_with_timeout_for_op_and_health_action(
+                "walk_dir",
+                || async { Err::<(), Error>(DiskError::Io(std::io::Error::new(std::io::ErrorKind::TimedOut, "stall timeout"))) },
+                Duration::from_secs(1),
+                FailureHealthAction::IgnoreFailure,
+            )
+            .await
+            .expect_err("timeout-like operation error should fail");
+
+        assert_eq!(
+            match &err {
+                DiskError::Io(io_err) => io_err.kind(),
+                other => panic!("expected io timeout error, got {other:?}"),
+            },
+            std::io::ErrorKind::TimedOut
+        );
+        assert!(
+            remote_disk.is_online().await,
+            "ignored network-like error should not mark remote disk faulty"
+        );
+        assert!(
+            GLOBAL_CONN_MAP.read().await.contains_key(&addr),
+            "ignored network-like error should not evict cached connection"
+        );
+    }
+
    #[tokio::test]
    async fn test_execute_with_timeout_keeps_remote_disk_online_for_business_error() {
        let addr = "http://127.0.0.1:59994".to_string();