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
This commit is contained in:
cxymds
2026-08-21 11:12:37 +08:00
committed by GitHub
parent 1aae680373
commit dfeb732ac8
+82 -32
View File
@@ -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<Uuid>, 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<PutObjectExtractOptions> {
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();