From dfeb732ac876726ffde59eff987c622980aed5ac Mon Sep 17 00:00:00 2001 From: cxymds Date: Fri, 21 Aug 2026 11:12:37 +0800 Subject: [PATCH] fix: make DeleteObjects idempotent for raw not-found errors (#6323) * fix: make DeleteObjects idempotent for raw not-found errors * fix: cover DeleteObjects raw not-found result dispatch --- rustfs/src/app/object_usecase.rs | 114 ++++++++++++++++++++++--------- 1 file changed, 82 insertions(+), 32 deletions(-) diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index ee95b6244..a31ec1d19 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -3969,6 +3969,14 @@ fn delete_creates_delete_marker(opts: &ObjectOptions) -> bool { opts.version_id.is_none() && opts.versioned && !opts.version_suspended } +/// `DeleteObjects` is idempotent. A raw filesystem `NotFound` can cross the +/// distributed delete path instead of its usual typed missing-object error. +fn is_delete_objects_not_found(error: &EcstoreError) -> bool { + is_err_object_not_found(error) + || is_err_version_not_found(error) + || matches!(error, StorageError::Io(source) if source.kind() == std::io::ErrorKind::NotFound) +} + /// Bounded concurrency for the per-object pre-delete stat fanout in /// `execute_delete_objects` (backlog#929 / HP-8). Keeps the metadata reads for /// a 1000-key batch from serializing while capping the disk fanout pressure. @@ -4030,6 +4038,27 @@ fn delete_response_version_id(version_id: Option, synthetic_version_id: bo } } +fn reduce_delete_objects_result<'a>( + object: &ObjectToDelete, + deleted: &'a StorageDeletedObject, + error: Option<&EcstoreError>, + synthetic_version_id: bool, +) -> Result<&'a StorageDeletedObject, s3s::dto::Error> { + match error { + None => Ok(deleted), + Some(error) if is_delete_objects_not_found(error) => Ok(deleted), + Some(error) => { + let api_error = ApiError::from(error.clone()); + Err(s3s::dto::Error { + code: Some(api_error.code.as_str().to_string()), + key: Some(object.object_name.clone()), + message: Some(api_error.message), + version_id: delete_response_version_id(object.version_id, synthetic_version_id), + }) + } + } +} + fn resolve_put_object_extract_options(headers: &HeaderMap) -> S3Result { let prefix = snowball_meta_value(headers, SNOWBALL_PREFIX_HEADER_KEYS, SNOWBALL_PREFIX_SUFFIX_LOWER) .map(|value| normalize_snowball_prefix(&value)) @@ -8476,39 +8505,31 @@ impl DefaultObjectUsecase { for (i, err) in errs.iter().enumerate() { let didx = object_to_delete_idx[i]; - if err.is_none() - || err - .clone() - .is_some_and(|v| is_err_object_not_found(&v) || is_err_version_not_found(&v)) - { - delete_results[didx].delete_object = Some(dobjs[i].clone()); - let (versioned, version_suspended) = object_versioning[i]; - let creates_delete_marker = object_to_delete[i].version_id.is_none() && versioned && !version_suspended; - if creates_delete_marker { - record_bucket_delete_marker_memory(&bucket).await; - } else { - let size = object_sizes[i].max(0) as u64; - record_bucket_object_delete_memory( - &bucket, - size, - existing_object_infos[i].is_some() && object_to_delete[i].version_id.is_none(), - ) - .await; + match reduce_delete_objects_result( + &object_to_delete[i], + &dobjs[i], + err.as_ref(), + delete_results[didx].synthetic_version_id, + ) { + Ok(deleted_object) => { + delete_results[didx].delete_object = Some(deleted_object.clone()); + let (versioned, version_suspended) = object_versioning[i]; + let creates_delete_marker = object_to_delete[i].version_id.is_none() && versioned && !version_suspended; + if creates_delete_marker { + record_bucket_delete_marker_memory(&bucket).await; + } else { + let size = object_sizes[i].max(0) as u64; + record_bucket_object_delete_memory( + &bucket, + size, + existing_object_infos[i].is_some() && object_to_delete[i].version_id.is_none(), + ) + .await; + } + } + Err(error) => { + delete_results[didx].error = Some(error); } - continue; - } - - if let Some(err) = err.clone() { - let api_error = ApiError::from(err); - delete_results[didx].error = Some(s3s::dto::Error { - code: Some(api_error.code.as_str().to_string()), - key: Some(object_to_delete[i].object_name.clone()), - message: Some(api_error.message), - version_id: delete_response_version_id( - object_to_delete[i].version_id, - delete_results[didx].synthetic_version_id, - ), - }); } } @@ -17692,6 +17713,35 @@ mod tests { assert_eq!(internal_version_id, None); } + #[test] + fn delete_objects_treats_raw_io_not_found_as_idempotent() { + assert!(is_delete_objects_not_found(&StorageError::FileNotFound)); + assert!(is_delete_objects_not_found(&StorageError::Io(std::io::Error::from( + std::io::ErrorKind::NotFound, + )))); + assert!(!is_delete_objects_not_found(&StorageError::Io(std::io::Error::from( + std::io::ErrorKind::PermissionDenied, + )))); + assert!(!is_delete_objects_not_found(&StorageError::DiskNotFound)); + } + + #[test] + fn delete_objects_result_reducer_reports_raw_not_found_as_deleted() { + let object = ObjectToDelete { + object_name: "missing-key".to_string(), + ..Default::default() + }; + let deleted = StorageDeletedObject { + object_name: object.object_name.clone(), + ..Default::default() + }; + let error = StorageError::Io(std::io::Error::from(std::io::ErrorKind::NotFound)); + + let deleted = reduce_delete_objects_result(&object, &deleted, Some(&error), false) + .expect("raw not-found must produce a deleted result"); + assert_eq!(deleted.object_name, "missing-key"); + } + #[test] fn recursive_force_delete_requires_administrative_or_replica_context() { let mut headers = HeaderMap::new();