From 569fa3ec87932be849343a213c5f405ce0e4bbdb Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Sat, 18 Jul 2026 10:37:43 +0800 Subject: [PATCH] fix(ecstore): tolerate illumos/Solaris EEXIST for non-empty directory removal (#4995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 119 +++++++++++++++++++++++-------- crates/ecstore/src/disk/os.rs | 84 ++++++++++++++++++++++ 2 files changed, 172 insertions(+), 31 deletions(-) diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index cc9a9c619..9d0c7fca0 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/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) => {} diff --git a/crates/ecstore/src/disk/os.rs b/crates/ecstore/src/disk/os.rs index bc5f430fa..40986be7c 100644 --- a/crates/ecstore/src/disk/os.rs +++ b/crates/ecstore/src/disk/os.rs @@ -645,6 +645,34 @@ pub fn file_exists(path: impl AsRef) -> 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