fix(ecstore): tolerate illumos/Solaris EEXIST for non-empty directory removal (#4995)

POSIX lets rmdir report a non-empty directory as either ENOTEMPTY or EEXIST. Linux/macOS/Windows use ENOTEMPTY (ErrorKind::DirectoryNotEmpty); illumos/Solaris return EEXIST (errno 17), which Rust surfaces as ErrorKind::AlreadyExists and which a DirectoryNotEmpty match never catches.

LocalDisk::delete_file removes xl.meta and then recurses to rmdir the object directory, tolerating only NotFound and DirectoryNotEmpty. Since #4300 (transactional delete rollback-staging, new in beta9) the object directory still holds the rollback backup dir when that rmdir runs — the caller removes it only after write quorum is confirmed — so the rmdir legitimately reports "not empty". On Linux that is tolerated; on Solaris it is EEXIST, which fell through to the catch-all arm and became FileAccessDeniedWithContext. That failed the delete commit, rolled the metadata back, and left the object undeletable, so the client retried indefinitely with a spurious FileAccessDenied and no EACCES anywhere (rustfs/rustfs#4978). The same Linux-errno assumption also broke non-force DeleteBucket on a populated bucket on Solaris.

Add a portable is_dir_not_empty_error classifier (DirectoryNotEmpty kind plus raw ENOTEMPTY/EEXIST), mirroring MinIO's isSysErrNotEmpty, and use it at the two directory-removal sites via is_benign_object_rmdir_error (delete_file) and classify_delete_volume_error (delete_volume). The classifier is applied only at rmdir/remove_dir_all sites, where EEXIST unambiguously means "not empty", so EEXIST keeps its normal meaning everywhere else. rmdir never returns EEXIST on Linux/macOS/Windows, so the new raw-errno branch is unreachable there and the change is a strict no-op off illumos/Solaris.

Adds unit tests for the classifier (DirectoryNotEmpty/ENOTEMPTY/EEXIST match, EACCES/ENOENT reject, real non-empty rmdir against the host errno) and call-site decision tests that make a Solaris EEXIST regression detectable on Linux CI.

Fixes #4978

crates/ecstore/src/disk/local.rs

@@ -28,7 +28,7 @@ use crate::disk::{
     format::FormatV3,
     fs::{O_APPEND, O_CREATE, O_RDONLY, O_TRUNC, O_WRONLY, access, lstat, lstat_std, remove, remove_all_std, remove_std, rename},
     os,
-    os::{check_path_length, is_empty_dir, is_root_disk, rename_all, rename_all_ignore_missing_source},
+    os::{check_path_length, is_dir_not_empty_error, is_empty_dir, is_root_disk, rename_all, rename_all_ignore_missing_source},
 };
 use crate::erasure::coding::{self, bitrot_verify};
 use crate::runtime::sources as runtime_sources;
@@ -103,10 +103,6 @@ fn remove_file_if_exists(path: &Path) -> std::io::Result<()> {
     }
 }
 
-fn is_remove_dir_not_empty_error(kind: ErrorKind) -> bool {
-    matches!(kind, ErrorKind::DirectoryNotEmpty | ErrorKind::AlreadyExists)
-}
-
 fn remove_dir_all_if_exists(path: &Path) -> std::io::Result<()> {
     match std::fs::remove_dir_all(path) {
         Ok(()) => Ok(()),
@@ -237,6 +233,31 @@ async fn restore_delete_rollback_after_error(
     err
 }
 
+/// Whether a failed `remove_dir` while cleaning up an object path is benign.
+///
+/// The directory is either already gone (`NotFound`) or still holds sibling
+/// entries owned by another step — e.g. the delete rollback-staging dir, which
+/// the caller removes only after write quorum is confirmed — in which case it is
+/// left intact. illumos/Solaris report the still-populated case as EEXIST rather
+/// than ENOTEMPTY (see [`is_dir_not_empty_error`]), so matching `ErrorKind`
+/// alone misclassifies it as a hard failure and turns a benign delete into a
+/// spurious `FileAccessDenied` (rustfs/rustfs#4978).
+fn is_benign_object_rmdir_error(err: &std::io::Error) -> bool {
+    err.kind() == ErrorKind::NotFound || is_dir_not_empty_error(err)
+}
+
+/// Classify a `delete_volume` removal error. A non-force `remove_dir` on a
+/// populated bucket must map to `VolumeNotEmpty`; illumos/Solaris report that as
+/// EEXIST rather than ENOTEMPTY, which `to_volume_error` would otherwise pass
+/// through as a raw OS error (rustfs/rustfs#4978).
+fn classify_delete_volume_error(err: std::io::Error) -> DiskError {
+    if is_dir_not_empty_error(&err) {
+        DiskError::VolumeNotEmpty
+    } else {
+        to_volume_error(err).into()
+    }
+}
+
 const LOG_COMPONENT_ECSTORE: &str = "ecstore";
 const LOG_SUBSYSTEM_DISK_LOCAL: &str = "disk_local";
 const EVENT_DISK_LOCAL_STARTUP_CLEANUP: &str = "disk_local_startup_cleanup";
@@ -4363,24 +4384,23 @@ impl LocalDisk {
             // debug!("delete_file remove_dir {:?}", &delete_path);
             if let Err(err) = fs::remove_dir(&delete_path).await {
                 // debug!("remove_dir err {:?} when {:?}", &err, &delete_path);
-                match err.kind() {
-                    ErrorKind::NotFound => (),
-                    kind if is_remove_dir_not_empty_error(kind) => (),
-                    kind => {
-                        warn!(
-                            event = EVENT_DISK_LOCAL_DELETE_FAILED,
-                            component = LOG_COMPONENT_ECSTORE,
-                            subsystem = LOG_SUBSYSTEM_DISK_LOCAL,
-                            path = ?delete_path,
-                            operation = "remove_dir",
-                            error_kind = %kind,
-                            "Disk local delete failed"
-                        );
-                        return Err(Error::other(FileAccessDeniedWithContext {
-                            path: delete_path.clone(),
-                            source: err,
-                        }));
-                    }
+                // A missing or still-populated directory is benign here; see
+                // is_benign_object_rmdir_error (handles the illumos/Solaris EEXIST
+                // convention, rustfs/rustfs#4978).
+                if !is_benign_object_rmdir_error(&err) {
+                    warn!(
+                        event = EVENT_DISK_LOCAL_DELETE_FAILED,
+                        component = LOG_COMPONENT_ECSTORE,
+                        subsystem = LOG_SUBSYSTEM_DISK_LOCAL,
+                        path = ?delete_path,
+                        operation = "remove_dir",
+                        error_kind = %err.kind(),
+                        "Disk local delete failed"
+                    );
+                    return Err(Error::other(FileAccessDeniedWithContext {
+                        path: delete_path.clone(),
+                        source: err,
+                    }));
                 }
             }
             // debug!("delete_file remove_dir done {:?}", &delete_path);
@@ -7796,7 +7816,7 @@ impl DiskAPI for LocalDisk {
         };
 
         if let Err(err) = res {
-            let e: DiskError = to_volume_error(err).into();
+            let e: DiskError = classify_delete_volume_error(err);
             if e != DiskError::VolumeNotFound {
                 return Err(e);
             }
@@ -7933,13 +7953,6 @@ mod test {
         }
     }
 
-    #[test]
-    fn remove_dir_not_empty_accepts_illumos_eexist() {
-        assert!(is_remove_dir_not_empty_error(ErrorKind::DirectoryNotEmpty));
-        assert!(is_remove_dir_not_empty_error(ErrorKind::AlreadyExists));
-        assert!(!is_remove_dir_not_empty_error(ErrorKind::PermissionDenied));
-    }
-
     impl<'a> MakeWriter<'a> for CapturedLogs {
         type Writer = CapturedLogWriter;
 
@@ -7987,6 +8000,50 @@ mod test {
         assert!(!rollback_dir.is_nil());
     }
 
+    // Call-site guards for rustfs/rustfs#4978. On Linux/macOS CI a real
+    // non-empty rmdir yields ENOTEMPTY (already tolerated by the pre-fix paths),
+    // so an end-to-end delete test cannot detect a call-site regression. These
+    // drive the decision functions the call sites use with a synthetic Solaris
+    // EEXIST, so reverting the fix at either site turns a test red on any host.
+    #[test]
+    fn is_benign_object_rmdir_error_tolerates_missing_and_non_empty() {
+        assert!(is_benign_object_rmdir_error(&std::io::Error::from(std::io::ErrorKind::NotFound)));
+        assert!(is_benign_object_rmdir_error(&std::io::Error::from(std::io::ErrorKind::DirectoryNotEmpty)));
+        // A genuine failure must still propagate.
+        assert!(!is_benign_object_rmdir_error(&std::io::Error::from(std::io::ErrorKind::PermissionDenied)));
+        #[cfg(unix)]
+        {
+            assert!(is_benign_object_rmdir_error(&std::io::Error::from_raw_os_error(libc::ENOTEMPTY)));
+            // illumos/Solaris report a non-empty rmdir as EEXIST, not ENOTEMPTY.
+            assert!(is_benign_object_rmdir_error(&std::io::Error::from_raw_os_error(libc::EEXIST)));
+            assert!(!is_benign_object_rmdir_error(&std::io::Error::from_raw_os_error(libc::EACCES)));
+        }
+    }
+
+    #[test]
+    fn classify_delete_volume_error_maps_not_empty_and_missing() {
+        assert!(matches!(
+            classify_delete_volume_error(std::io::Error::from(std::io::ErrorKind::DirectoryNotEmpty)),
+            DiskError::VolumeNotEmpty
+        ));
+        assert!(matches!(
+            classify_delete_volume_error(std::io::Error::from(std::io::ErrorKind::NotFound)),
+            DiskError::VolumeNotFound
+        ));
+        #[cfg(unix)]
+        {
+            assert!(matches!(
+                classify_delete_volume_error(std::io::Error::from_raw_os_error(libc::ENOTEMPTY)),
+                DiskError::VolumeNotEmpty
+            ));
+            // illumos/Solaris non-empty rmdir -> EEXIST must still refuse the bucket.
+            assert!(matches!(
+                classify_delete_volume_error(std::io::Error::from_raw_os_error(libc::EEXIST)),
+                DiskError::VolumeNotEmpty
+            ));
+        }
+    }
+
     async fn ensure_test_volume(disk: &LocalDisk, volume: &str) {
         match disk.make_volume(volume).await {
             Ok(()) | Err(DiskError::VolumeExists) => {}

crates/ecstore/src/disk/os.rs

@@ -645,6 +645,34 @@ pub fn file_exists(path: impl AsRef<Path>) -> bool {
     std::fs::metadata(path.as_ref()).map(|_| true).unwrap_or(false)
 }
 
+/// Whether an [`io::Error`] means "the directory is not empty".
+///
+/// POSIX lets `rmdir`/`rename` report a non-empty directory as either
+/// `ENOTEMPTY` or `EEXIST`. Linux uses `ENOTEMPTY` (which Rust surfaces as
+/// [`io::ErrorKind::DirectoryNotEmpty`]); illumos/Solaris return `EEXIST`
+/// (errno 17), which Rust surfaces as [`io::ErrorKind::AlreadyExists`] and
+/// which the `DirectoryNotEmpty` kind therefore never catches. Matching only on
+/// the kind silently misclassifies the Solaris case as a hard failure, so
+/// callers that must treat a still-populated directory as benign (deleting the
+/// object metadata while a rollback-staging dir remains, non-force
+/// `DeleteBucket` on a populated bucket) have to test the raw errno as well.
+/// Mirrors MinIO's `isSysErrNotEmpty`.
+pub fn is_dir_not_empty_error(err: &io::Error) -> bool {
+    // Linux/Windows: ENOTEMPTY / ERROR_DIR_NOT_EMPTY -> DirectoryNotEmpty.
+    if err.kind() == io::ErrorKind::DirectoryNotEmpty {
+        return true;
+    }
+    // illumos/Solaris report a non-empty `rmdir`/`rename` as EEXIST (errno 17),
+    // which Rust surfaces as `AlreadyExists` (so the `DirectoryNotEmpty` kind
+    // never catches it). Confirm against the raw errno directly so the
+    // classification holds regardless of how the platform std maps it.
+    #[cfg(unix)]
+    if matches!(err.raw_os_error(), Some(libc::ENOTEMPTY) | Some(libc::EEXIST)) {
+        return true;
+    }
+    false
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -809,6 +837,62 @@ mod tests {
         assert!(!should_retry_rename(&denied, 1));
     }
 
+    #[test]
+    fn is_dir_not_empty_error_recognizes_directory_not_empty_kind() {
+        let err = io::Error::from(io::ErrorKind::DirectoryNotEmpty);
+        assert!(is_dir_not_empty_error(&err));
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn is_dir_not_empty_error_recognizes_raw_enotempty() {
+        // Linux/BSD/macOS non-empty rmdir/rename errno.
+        let err = io::Error::from_raw_os_error(libc::ENOTEMPTY);
+        assert!(is_dir_not_empty_error(&err));
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn is_dir_not_empty_error_recognizes_solaris_eexist() {
+        // illumos/Solaris report a non-empty rmdir/rename as EEXIST, which Rust
+        // surfaces as `AlreadyExists` (never `DirectoryNotEmpty`). This is the
+        // core of rustfs/rustfs#4978: matching only the kind misclassified this
+        // benign condition as a hard failure.
+        let err = io::Error::from_raw_os_error(libc::EEXIST);
+        assert_eq!(err.kind(), io::ErrorKind::AlreadyExists);
+        assert!(is_dir_not_empty_error(&err));
+    }
+
+    #[test]
+    fn is_dir_not_empty_error_rejects_unrelated_errors() {
+        assert!(!is_dir_not_empty_error(&io::Error::from(io::ErrorKind::NotFound)));
+        assert!(!is_dir_not_empty_error(&io::Error::from(io::ErrorKind::PermissionDenied)));
+        #[cfg(unix)]
+        {
+            assert!(!is_dir_not_empty_error(&io::Error::from_raw_os_error(libc::EACCES)));
+            assert!(!is_dir_not_empty_error(&io::Error::from_raw_os_error(libc::ENOENT)));
+        }
+    }
+
+    #[tokio::test]
+    async fn is_dir_not_empty_error_matches_real_non_empty_rmdir() {
+        // Validate against the host's actual errno, whatever it is: Linux/macOS
+        // return ENOTEMPTY, illumos/Solaris return EEXIST. The removal must be
+        // classified as "not empty" on every platform.
+        let temp_dir = tempdir().expect("create temp dir");
+        let populated = temp_dir.path().join("populated");
+        std::fs::create_dir(&populated).expect("create dir");
+        std::fs::write(populated.join("child"), b"x").expect("write child");
+
+        let err = std::fs::remove_dir(&populated).expect_err("non-empty rmdir must fail");
+        assert!(
+            is_dir_not_empty_error(&err),
+            "non-empty rmdir must classify as not-empty, got kind {:?} errno {:?}",
+            err.kind(),
+            err.raw_os_error()
+        );
+    }
+
     #[tokio::test]
     async fn rename_all_moves_existing_directory_tree() {
         // Guards the rename_data commit path, which funnels through