From 3c89c71f66932f060a28d9d4bbc9ad4ada14c95c Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Fri, 28 Aug 2026 07:55:05 +0800 Subject: [PATCH] fix(s3): round-trip null-version delete-marker identity (#6765) * fix(s3): round-trip null-version delete-marker identity through listing and delete responses On a versioning-suspended bucket, a null delete marker's identity was lost on the way back to the client at three points (issue #6745): ListObjectVersions advertised the marker's VersionId as the literal nil UUID instead of null; deleting by that id succeeded but the DeleteObjects/DeleteObject response reported the identity as null with no way to correlate it to the request; and the response lacked DeleteMarker/DeleteMarkerVersionId because the marker-ness comparison mixed the client-facing identity (Some(nil)) with the storage identity (None), so the removal also mis-recorded accounting and fired DeleteMarkerCreated semantics on later paths. - Listing (bucket_usecase, s3_api/bucket, build_list_versions_next_marker) now maps the synthesized nil UUID to the literal null everywhere it reaches the wire, and VersionMarker::parse folds a nil-UUID marker from older listings into VersionMarker::Null so pagination resumes correctly. - delete_objects normalizes both sides of the marker-ness comparison via delete_file_info_version_id (matching the adjacent explicit_delete_marker admission check) and reports DeleteMarkerVersionId as null for an explicit null-marker removal. - resolve_delete_version_state reports delete_marker for an explicit-version delete whose target is a delete marker even when the bucket is versioning-suspended, fixing x-amz-delete-marker on the single-object path. - The DeleteObjects response entry echoes the version identity the request addressed for marker removals, marker-removal accounting no longer records a marker creation, and notification events fire DeleteMarkerCreated only for actual marker creation. Fixes #6745 * fix(s3): keep null-marker removal write shape undeleted and report marker semantics response-side The first cut marked the storage delete request deleted for a null-marker removal, which FileMeta::delete_version interprets as the suspended-bucket delete-mints-a-marker write and re-creates the marker just removed. Carry marker-ness to responses via explicit_delete_removed_marker (single path) and a response-only branch flag (batch path) instead, keeping every storage write shape byte-identical to the pre-fix behavior. Adds an embedded end-to-end regression test covering the full issue #6745 round trip. --- crates/ecstore/src/set_disk/mod.rs | 101 +++++++++++++++++ crates/ecstore/src/set_disk/ops/object.rs | 55 +++++++-- crates/ecstore/src/store/list_objects.rs | 36 +++++- crates/storage-api/src/object.rs | 23 +++- rustfs/src/app/bucket_usecase.rs | 9 +- rustfs/src/app/object/delete.rs | 87 +++++++++++++- rustfs/src/storage/s3_api/bucket.rs | 30 ++++- rustfs/tests/embedded_test.rs | 131 ++++++++++++++++++++++ 8 files changed, 449 insertions(+), 23 deletions(-) diff --git a/crates/ecstore/src/set_disk/mod.rs b/crates/ecstore/src/set_disk/mod.rs index 83398c02e..2fa44581f 100644 --- a/crates/ecstore/src/set_disk/mod.rs +++ b/crates/ecstore/src/set_disk/mod.rs @@ -4817,6 +4817,28 @@ fn should_force_delete_marker_for_missing_version(opts: &ObjectOptions) -> bool opts.delete_marker || ((opts.versioned || opts.version_suspended) && opts.version_id.is_none() && !opts.data_movement) } +/// Whether a plain client delete addressed to an explicit version removed an +/// existing delete marker, so the response must carry delete-marker semantics +/// (`x-amz-delete-marker: true` / `DeleteMarker` in `DeleteObjects` entries). +/// +/// This is a response-side classification only. It must never feed the +/// storage write shape: a delete request marked `deleted` with the null +/// identity makes `FileMeta::delete_version` re-create the marker it just +/// removed (that is the suspended-bucket "delete mints a marker" write +/// path), which is why `resolve_delete_version_state` cannot simply report +/// `delete_marker` for suspended buckets (issue #6745). +/// +/// Purge/replica replication shapes are excluded, mirroring the clauses in +/// `resolve_delete_version_state` that clear `delete_marker` for them. +fn explicit_delete_removed_marker(opts: &ObjectOptions, goi: &ObjectInfo, version_found: bool) -> bool { + opts.version_id.is_some() + && version_found + && goi.delete_marker + && goi.version_purge_status.is_empty() + && opts.version_purge_status().is_empty() + && opts.delete_marker_replication_status().is_empty() +} + fn resolve_delete_version_state(opts: &ObjectOptions, goi: &ObjectInfo, version_found: bool) -> (bool, bool) { let mut mark_delete = goi.version_id.is_some() || ((opts.versioned || opts.version_suspended) && opts.version_id.is_none()); let mut delete_marker = opts.versioned; @@ -7038,6 +7060,85 @@ mod tests { ); } + #[test] + fn resolve_delete_version_state_keeps_null_marker_removal_write_shape_undeleted() { + // Removing a null delete marker by explicit version id on a + // versioning-suspended bucket must NOT mark the write request + // `deleted`: `FileMeta::delete_version` re-creates a marker for + // `deleted` requests carrying the null identity (the suspended-bucket + // "delete mints a marker" path), which would resurrect the marker + // being removed. The response-side marker semantics come from + // `explicit_delete_removed_marker` instead (issue #6745). + let opts = ObjectOptions { + version_suspended: true, + version_id: Some(Uuid::nil().to_string()), + ..Default::default() + }; + let current = ObjectInfo { + version_id: Some(Uuid::nil()), + delete_marker: true, + ..Default::default() + }; + + let (mark_delete, delete_marker) = resolve_delete_version_state(&opts, ¤t, true); + + assert!(!mark_delete); + assert!(!delete_marker, "the storage write for a null-marker removal must stay undeleted"); + assert!( + explicit_delete_removed_marker(&opts, ¤t, true), + "the response must still report delete-marker semantics" + ); + } + + #[test] + fn explicit_delete_removed_marker_is_limited_to_plain_marker_removals() { + let opts = ObjectOptions { + versioned: true, + version_id: Some(Uuid::new_v4().to_string()), + ..Default::default() + }; + let marker = ObjectInfo { + version_id: Some(Uuid::new_v4()), + delete_marker: true, + ..Default::default() + }; + assert!(explicit_delete_removed_marker(&opts, &marker, true)); + assert!( + !explicit_delete_removed_marker(&opts, &marker, false), + "missing versions are not marker removals" + ); + + let data_version = ObjectInfo { + version_id: marker.version_id, + delete_marker: false, + ..Default::default() + }; + assert!(!explicit_delete_removed_marker(&opts, &data_version, true)); + + let versionless = ObjectOptions { + versioned: true, + ..Default::default() + }; + assert!( + !explicit_delete_removed_marker(&versionless, &marker, true), + "marker creation (no version in the request) is not a removal" + ); + + let replica_purge = ObjectOptions { + versioned: true, + version_id: Some(Uuid::new_v4().to_string()), + delete_replication: Some(ReplicationState { + replica_status: ReplicationStatusType::Replica, + ..Default::default() + }), + ..Default::default() + }; + assert!( + !explicit_delete_removed_marker(&replica_purge, &marker, true), + "replication purge shapes keep their existing response semantics" + ); + } + #[test] fn resolve_delete_version_state_keeps_delete_marker_for_replica_marker_creation() { let opts = ObjectOptions { diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index 83604a719..1e5c84afe 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -42,12 +42,13 @@ use super::super::{ can_try_inline_data_shards_direct, check_object_lock_delete, check_object_lock_for_deletion_with_state, check_object_lock_retention_update, classify_get_codec_streaming_object_class, classify_put_write_path, classify_storage_error, collect_inline_data_shard_fileinfos_by_index, contains_key_str, create_bitrot_writer, debug, - delete_file_info_version_id, disk, ensure_delete_commit_locks_held, error, finish_set_disk_read_lock, - get_codec_streaming_reader_gate, get_object_body_cache_hook, get_raw_etag, get_small_object_direct_memory_decision, - get_stage_timer_if_enabled, get_str, get_transitioned_object_reader_with_tier_manager, inline_erasure_shard_file_offset, - inline_erasure_shard_size, insert_str, is_deadlock_detection_enabled, is_err_object_not_found, is_err_version_not_found, - is_explicit_null_version, is_lock_optimization_enabled, issue3031_diag_enabled, join_all, known_put_object_storage_size, - path_join_buf, put_restore_opts, record_compression_total_memory, record_get_codec_streaming_gate_decision, + delete_file_info_version_id, disk, ensure_delete_commit_locks_held, error, explicit_delete_removed_marker, + finish_set_disk_read_lock, get_codec_streaming_reader_gate, get_object_body_cache_hook, get_raw_etag, + get_small_object_direct_memory_decision, get_stage_timer_if_enabled, get_str, + get_transitioned_object_reader_with_tier_manager, inline_erasure_shard_file_offset, inline_erasure_shard_size, insert_str, + is_deadlock_detection_enabled, is_err_object_not_found, is_err_version_not_found, is_explicit_null_version, + is_lock_optimization_enabled, issue3031_diag_enabled, join_all, known_put_object_storage_size, path_join_buf, + put_restore_opts, record_compression_total_memory, record_get_codec_streaming_gate_decision, record_get_direct_memory_decision, record_get_object_pipeline_failure, record_get_object_pipeline_failure_for_path, record_get_object_reader_path_observation, record_get_stage_duration_if_enabled, record_lock_acquire, reduce_write_quorum_errs, release_materialized_read_lock, replication_write_may_pass_worm_gate, require_restore_operation_id, @@ -6521,7 +6522,21 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { } } - if goi.delete_marker && dobj.version_id.is_some() && goi.version_id == version_id { + // Same normalization as `explicit_delete_marker` above: `goi.version_id` + // is the client-facing identity (`Some(Uuid::nil())` for a null + // version) while `version_id` is the storage identity (`None` for an + // explicit null). Comparing them raw made a null delete marker's + // removal take the non-marker branch below, so the response lost + // `DeleteMarker`/`DeleteMarkerVersionId` and the removal was + // accounted as an object deletion (issue #6745). + let removed_delete_marker = + goi.delete_marker && dobj.version_id.is_some() && delete_file_info_version_id(goi.version_id) == version_id; + // Response-side only for the null identity: a delete request marked + // `deleted` with `version_id == None` makes `FileMeta::delete_version` + // re-create the marker it just removed (the suspended-bucket + // "delete mints a marker" write path), so the write shape must stay + // untouched for explicit null-marker removals. + if removed_delete_marker && version_id.is_some() { vr.deleted = true; vr.mod_time = goi.mod_time; } @@ -6540,11 +6555,21 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { } }; - if vr.deleted { + if vr.deleted || removed_delete_marker { del_objects[i] = DeletedObject { - delete_marker: vr.deleted, - delete_marker_version_id: vr.version_id, - delete_marker_mtime: vr.mod_time, + delete_marker: true, + // `vr.version_id` holds the storage identity, which is + // `None` for an explicit null-marker removal; report the + // client-facing null identity so the response can carry + // `DeleteMarkerVersionId` for the marker that was removed. + delete_marker_version_id: if explicit_null_version { + Some(Uuid::nil()) + } else { + vr.version_id + }, + // For a null-marker removal `vr` stays undeleted (write + // shape), so take the marker's mtime from the source. + delete_marker_mtime: vr.mod_time.or(goi.mod_time), object_name: vr.name.clone(), replication_state: vr.replication_state_internal.clone(), ..Default::default() @@ -7197,6 +7222,14 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { obj_info.user_defined = Arc::clone(&goi.user_defined); obj_info.parts = Arc::clone(&goi.parts); obj_info.user_tags = Arc::clone(&goi.user_tags); + // Report delete-marker semantics for an explicit-version delete whose + // target was a delete marker. On versioning-suspended buckets + // `resolve_delete_version_state` cannot mark the write request itself + // (a `deleted` write with the null identity re-creates the marker), so + // the marker-ness is restored on the response here (issue #6745). + if explicit_delete_removed_marker(&opts, &goi, version_found) { + obj_info.delete_marker = true; + } self.invalidate_get_object_metadata_cache(bucket, object).await; Ok(obj_info) } diff --git a/crates/ecstore/src/store/list_objects.rs b/crates/ecstore/src/store/list_objects.rs index e3ed34d24..f5e0b323a 100644 --- a/crates/ecstore/src/store/list_objects.rs +++ b/crates/ecstore/src/store/list_objects.rs @@ -2110,9 +2110,18 @@ fn build_list_versions_next_marker( cache_id: Option<&str>, ) -> (Option, Option) { if let Some(last) = objects.last() { + // A null version carries the synthesized `Some(Uuid::nil())` identity + // here; advertise it as the literal `null` marker so a resumed listing + // parses it back to `VersionMarker::Null` instead of a nil UUID that + // `find_version_index` can never match (issue #6745). ( Some(append_list_cache_id_to_marker(last.name.clone(), cache_id)), - Some(last.version_id.map(|v| v.to_string()).unwrap_or_else(|| "null".to_string())), + Some( + last.version_id + .filter(|v| !v.is_nil()) + .map(|v| v.to_string()) + .unwrap_or_else(|| "null".to_string()), + ), ) } else if let Some(last_prefix) = prefixes.last() { (Some(append_list_cache_id_to_marker(last_prefix.clone(), cache_id)), None) @@ -7759,6 +7768,31 @@ mod test { out } + // A truncated versions listing ending on a null version must advertise the + // literal `null` continuation marker: a nil UUID parses to + // `VersionMarker::Version(nil)`, which no stored version matches, so the + // resumed page would replay every version (issue #6745). + #[test] + fn build_list_versions_next_marker_reports_null_for_nil_version_id() { + let null_version = ObjectInfo { + name: "obj-a".to_owned(), + version_id: Some(uuid::Uuid::nil()), + ..Default::default() + }; + let real_version = ObjectInfo { + name: "obj-b".to_owned(), + version_id: Some(uuid::Uuid::from_u128(7)), + ..Default::default() + }; + + let (next_marker, next_version_idmarker) = super::build_list_versions_next_marker(&[null_version], &[], None); + assert_eq!(next_marker.as_deref(), Some("obj-a")); + assert_eq!(next_version_idmarker.as_deref(), Some("null")); + + let (_, next_version_idmarker) = super::build_list_versions_next_marker(&[real_version], &[], None); + assert_eq!(next_version_idmarker.as_deref(), Some(uuid::Uuid::from_u128(7).to_string().as_str())); + } + // ECA-03 / #944: a page whose raw keys fully collapse into fewer than max_keys // common prefixes must still report truncation and carry a continuation marker, // otherwise every key beyond the scan window is silently dropped. diff --git a/crates/storage-api/src/object.rs b/crates/storage-api/src/object.rs index 7959354dc..8452e62cd 100644 --- a/crates/storage-api/src/object.rs +++ b/crates/storage-api/src/object.rs @@ -167,7 +167,15 @@ impl VersionMarker { if marker == NULL_VERSION_MARKER { Ok(Self::Null) } else { - Ok(Self::Version(Uuid::parse_str(marker)?)) + let version = Uuid::parse_str(marker)?; + // Older releases advertised the null version as a nil UUID + // (issue #6745); a stored null version has no UUID, so resuming + // by `Version(nil)` could never match. Fold it into `Null`. + if version.is_nil() { + Ok(Self::Null) + } else { + Ok(Self::Version(version)) + } } } } @@ -714,6 +722,19 @@ fn is_modified_since(mod_time: &OffsetDateTime, given_time: &OffsetDateTime) -> mod tests { use super::*; + #[test] + fn version_marker_parse_folds_null_and_nil_uuid_into_null() { + assert_eq!(VersionMarker::parse("null"), Ok(VersionMarker::Null)); + // Older releases advertised the null version as a nil UUID + // (issue #6745); it must resume as the null marker, not a UUID no + // stored version carries. + assert_eq!(VersionMarker::parse(Uuid::nil().to_string()), Ok(VersionMarker::Null)); + + let version = Uuid::from_u128(7); + assert_eq!(VersionMarker::parse(version.to_string()), Ok(VersionMarker::Version(version))); + assert!(VersionMarker::parse("not-a-version").is_err()); + } + #[test] fn http_preconditions_ignore_empty_etag_headers() { let opts = HTTPPreconditions { diff --git a/rustfs/src/app/bucket_usecase.rs b/rustfs/src/app/bucket_usecase.rs index b7d526fd1..a62ea98bf 100644 --- a/rustfs/src/app/bucket_usecase.rs +++ b/rustfs/src/app/bucket_usecase.rs @@ -936,8 +936,13 @@ fn build_list_object_versions_metadata_output( .filter(|object| !object.name.is_empty()) .map(|object| { let object_name = encode_list_versions_value(&object.name, encoding_type); + // A null version surfaces as `Some(Uuid::nil())` on the + // client-facing `ObjectInfo`; AWS advertises it as the literal + // string `null`, and a nil UUID must never reach the wire + // (issue #6745). let version_id = object .version_id + .filter(|version| !version.is_nil()) .map(|version| version.to_string()) .unwrap_or_else(|| "null".to_string()); let permission = permissions.get(&object.name).copied().unwrap_or_default(); @@ -4159,7 +4164,9 @@ mod tests { match &output.entries[0] { ListObjectVersionMetadataEntry::Version(version, extension) => { assert_eq!(version.key.as_deref(), Some("obj-a")); - assert_eq!(version.version_id.as_deref(), Some(Uuid::nil().to_string().as_str())); + // A null version's synthesized nil UUID must surface as the + // literal `null`, never as `00000000-…` (issue #6745). + assert_eq!(version.version_id.as_deref(), Some("null")); assert_eq!(extension.user_tags.as_deref(), Some("env=prod")); assert_eq!(extension.internal, Some(ObjectInternalInfo { k: 4, m: 2 })); assert_eq!( diff --git a/rustfs/src/app/object/delete.rs b/rustfs/src/app/object/delete.rs index a77d84242..c797335fb 100644 --- a/rustfs/src/app/object/delete.rs +++ b/rustfs/src/app/object/delete.rs @@ -327,6 +327,27 @@ fn delete_response_version_id(version_id: Option, synthetic_version_id: bo } } +/// Version identity for a `DeleteObjects` `` entry (and its +/// notification). A delete marker removed by version id carries no storage +/// `version_id` on the committed result, so fall back to the identity the +/// request addressed — clients correlate response entries against what they +/// sent (issue #6745). Marker creation keeps `None`: its request carried no +/// version id. +fn delete_entry_response_version_id( + committed_version_id: Option, + committed_delete_marker: bool, + requested_version_id: Option, + synthetic_version_id: bool, +) -> Option { + delete_response_version_id(committed_version_id, synthetic_version_id).or_else(|| { + if committed_delete_marker { + delete_response_version_id(requested_version_id, synthetic_version_id) + } else { + None + } + }) +} + fn reduce_delete_objects_result<'a>( object: &ObjectToDelete, deleted: &'a StorageDeletedObject, @@ -395,6 +416,11 @@ impl DefaultObjectUsecase { delete_object: Option, error: Option, synthetic_version_id: bool, + // The version identity the request addressed, before any + // synthetic-directory synthesis. `None` means the request carried + // no version id, i.e. a committed delete marker was created, not + // removed (issue #6745). + requested_version_id: Option, } let mut delete_results = vec![DeleteResult::default(); delete.objects.len()]; @@ -481,6 +507,7 @@ impl DefaultObjectUsecase { ..Default::default() }; delete_results[idx].synthetic_version_id = synthetic_version_id; + delete_results[idx].requested_version_id = version_uuid; authorized_deletes.push(AuthorizedDelete { idx, object }); } @@ -648,10 +675,16 @@ impl DefaultObjectUsecase { let creates_delete_marker = object_to_delete[i].version_id.is_none() && versioned && !version_suspended; let committed_delete_marker = dobjs[i].delete_marker; let delete_accounting = accounting.get(i).and_then(Option::as_ref); + // `requested_version_id` distinguishes marker creation + // (no version in the request) from marker removal by + // version id; an explicit null-version request maps to + // `Some(Uuid::nil())`, which `delete_request_targets_current` + // would treat as versionless and mis-record a marker + // removal as a marker creation (issue #6745). let update = delete_memory_update( creates_delete_marker, committed_delete_marker, - delete_request_targets_current(object_to_delete[i].version_id), + delete_results[didx].requested_version_id.is_none(), delete_accounting.and_then(|value| value.size), delete_accounting.is_some_and(|value| value.removed_current_object), ); @@ -673,7 +706,12 @@ impl DefaultObjectUsecase { result.synthetic_version_id, ), key: Some(object.object_name.clone()), - version_id: delete_response_version_id(object.version_id, result.synthetic_version_id), + version_id: delete_entry_response_version_id( + object.version_id, + object.delete_marker, + result.requested_version_id, + result.synthetic_version_id, + ), }) .collect(); let deleted_cache_keys = delete_results @@ -732,7 +770,10 @@ impl DefaultObjectUsecase { let _activity_guard = DeleteTailActivityGuard::new(DeleteTailStage::Notify); for res in delete_results { if let Some(dobj) = res.delete_object { - let event_name = delete_event_name_for_marker(dobj.delete_marker); + // `DeleteMarkerCreated` is a creation event; a delete + // marker removed by an explicit version id is a plain + // versioned delete (issue #6745). + let event_name = delete_event_name_for_marker(dobj.delete_marker && res.requested_version_id.is_none()); let event_args = EventArgsBuilder::new( event_name, notify_bucket.clone(), @@ -742,7 +783,15 @@ impl DefaultObjectUsecase { ..Default::default() }), ) - .version_id(delete_response_version_id(dobj.version_id, res.synthetic_version_id).unwrap_or_default()) + .version_id( + delete_entry_response_version_id( + dobj.version_id, + dobj.delete_marker, + res.requested_version_id, + res.synthetic_version_id, + ) + .unwrap_or_default(), + ) .req_params(req_params.clone()) .resp_elements(resp_elements.clone()) .host(get_request_host(&req_headers)) @@ -1082,7 +1131,9 @@ impl DefaultObjectUsecase { ..Default::default() }; - let event_name = delete_event_name_for_marker(delete_marker); + // `DeleteMarkerCreated` is a creation event; a delete marker removed + // by an explicit version id is a plain versioned delete (issue #6745). + let event_name = delete_event_name_for_marker(delete_marker && version_id_clone.is_none()); helper = helper.event_name(event_name); helper = helper.object(obj_info).version_id(response_version_id.unwrap_or_default()); @@ -1118,6 +1169,32 @@ mod tests { assert_eq!(delete_response_version_id(None, false), None); } + #[test] + fn delete_entry_response_version_id_echoes_requested_identity_for_marker_removal() { + let version_id = Uuid::new_v4(); + + // Marker removed by explicit null version id: echo `null`. + assert_eq!( + delete_entry_response_version_id(None, true, Some(Uuid::nil()), false), + Some("null".to_string()) + ); + // Marker removed by a real version id: echo that id. + assert_eq!( + delete_entry_response_version_id(None, true, Some(version_id), false), + Some(version_id.to_string()) + ); + // Marker creation (no version in the request): no version identity. + assert_eq!(delete_entry_response_version_id(None, true, None, false), None); + // Non-marker deletes keep the committed identity and never fall back. + assert_eq!( + delete_entry_response_version_id(Some(version_id), false, Some(Uuid::nil()), false), + Some(version_id.to_string()) + ); + assert_eq!(delete_entry_response_version_id(None, false, Some(version_id), false), None); + // Synthetic directory deletes stay without a version identity. + assert_eq!(delete_entry_response_version_id(None, true, Some(Uuid::nil()), true), None); + } + #[tokio::test] async fn execute_delete_object_rejects_invalid_object_key() { let input = DeleteObjectInput::builder() diff --git a/rustfs/src/storage/s3_api/bucket.rs b/rustfs/src/storage/s3_api/bucket.rs index 6599ca107..4a6f5f8d5 100644 --- a/rustfs/src/storage/s3_api/bucket.rs +++ b/rustfs/src/storage/s3_api/bucket.rs @@ -26,6 +26,7 @@ use s3s::dto::{ use s3s::{S3Error, S3ErrorCode}; use tracing::debug; use urlencoding::encode; +use uuid::Uuid; use crate::storage::storage_api::s3_api_consumer::bucket::StorageObjectInfo as ObjectInfo; @@ -198,6 +199,16 @@ pub(crate) fn parse_list_objects_v2_params( }) } +/// A null version surfaces as `Some(Uuid::nil())` on the client-facing +/// `ObjectInfo`; AWS advertises it as the literal string `null`, and a nil +/// UUID must never reach the wire (issue #6745). +fn list_versions_response_version_id(version_id: Option) -> String { + version_id + .filter(|id| !id.is_nil()) + .map(|id| id.to_string()) + .unwrap_or_else(|| "null".to_string()) +} + pub(crate) fn build_list_object_versions_output( object_infos: ListObjectVersionsInfo, bucket: String, @@ -213,7 +224,7 @@ pub(crate) fn build_list_object_versions_output( key: Some(encode_output_value(&v.name)), last_modified: v.mod_time.map(Timestamp::from), size: Some(v.size), - version_id: Some(v.version_id.map(|id| id.to_string()).unwrap_or_else(|| "null".to_string())), + version_id: Some(list_versions_response_version_id(v.version_id)), is_latest: Some(v.is_latest), e_tag: v.etag.clone().map(|etag| to_s3s_etag(&etag)), storage_class: v.storage_class.clone().map(ObjectVersionStorageClass::from), @@ -227,7 +238,7 @@ pub(crate) fn build_list_object_versions_output( .filter(|o| o.delete_marker) .map(|o| DeleteMarkerEntry { key: Some(encode_output_value(&o.name)), - version_id: Some(o.version_id.map(|id| id.to_string()).unwrap_or_else(|| "null".to_string())), + version_id: Some(list_versions_response_version_id(o.version_id)), is_latest: Some(o.is_latest), last_modified: o.mod_time.map(Timestamp::from), ..Default::default() @@ -940,6 +951,13 @@ mod tests { is_latest: false, ..Default::default() }, + ObjectInfo { + name: "obj-null-delete-marker".to_string(), + delete_marker: true, + version_id: Some(Uuid::nil()), + is_latest: true, + ..Default::default() + }, ObjectInfo { name: String::new(), delete_marker: false, @@ -975,12 +993,16 @@ mod tests { let versions = output.versions.unwrap_or_default(); assert_eq!(versions.len(), 1); assert_eq!(versions[0].key, Some("obj-a".to_string())); - assert_eq!(versions[0].version_id, Some(Uuid::nil().to_string())); + // A null version's synthesized nil UUID must surface as the literal + // `null`, never as `00000000-…` (issue #6745). + assert_eq!(versions[0].version_id, Some("null".to_string())); let delete_markers = output.delete_markers.unwrap_or_default(); - assert_eq!(delete_markers.len(), 1); + assert_eq!(delete_markers.len(), 2); assert_eq!(delete_markers[0].key, Some("obj-delete-marker".to_string())); assert_eq!(delete_markers[0].version_id, Some("null".to_string())); + assert_eq!(delete_markers[1].key, Some("obj-null-delete-marker".to_string())); + assert_eq!(delete_markers[1].version_id, Some("null".to_string())); let prefixes = output.common_prefixes.unwrap_or_default(); assert_eq!( diff --git a/rustfs/tests/embedded_test.rs b/rustfs/tests/embedded_test.rs index a2ae48087..e46ccaa31 100644 --- a/rustfs/tests/embedded_test.rs +++ b/rustfs/tests/embedded_test.rs @@ -21,6 +21,7 @@ use aws_sdk_s3::config::{Credentials, Region}; use aws_sdk_s3::primitives::ByteStream; +use aws_sdk_s3::types::{BucketVersioningStatus, Delete, ObjectIdentifier, VersioningConfiguration}; use aws_sdk_s3::{Client, Config}; use rustfs::embedded::{RustFSServerBuilder, find_available_port}; @@ -118,3 +119,133 @@ async fn test_embedded_server_basic_s3_operations_body() { server.shutdown().await; } + +// Regression test for issue #6745: on a versioning-suspended bucket, a null +// delete marker's version identity must round-trip as the literal `null` +// through ListObjectVersions, DeleteObject, and DeleteObjects, and removing +// the marker by version id must carry the delete-marker flags. +#[test] +fn test_null_version_delete_marker_round_trip() { + common::run_embedded_test(test_null_version_delete_marker_round_trip_body); +} + +async fn test_null_version_delete_marker_round_trip_body() { + let port = match find_available_port() { + Ok(port) => port, + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return, + Err(err) => panic!("find free port: {err}"), + }; + let server = RustFSServerBuilder::new() + .address(format!("127.0.0.1:{port}")) + .access_key("testaccesskey") + .secret_key("testsecretkey") + .build() + .await + .expect("start embedded server"); + let client = s3_client(&server.endpoint(), server.access_key(), server.secret_key()); + + let bucket = "null-marker-bucket"; + client.create_bucket().bucket(bucket).send().await.expect("create bucket"); + for status in [BucketVersioningStatus::Enabled, BucketVersioningStatus::Suspended] { + client + .put_bucket_versioning() + .bucket(bucket) + .versioning_configuration(VersioningConfiguration::builder().status(status).build()) + .send() + .await + .expect("set bucket versioning state"); + } + + // A versionless DELETE on the suspended bucket mints a null delete marker + // and must report it as version `null`. + for key in ["doc/f1.txt", "doc/f2.txt"] { + client + .put_object() + .bucket(bucket) + .key(key) + .body(ByteStream::from_static(b"null-version payload")) + .send() + .await + .expect("put object"); + let deleted = client + .delete_object() + .bucket(bucket) + .key(key) + .send() + .await + .expect("delete object without version id"); + assert_eq!(deleted.delete_marker(), Some(true), "suspended-bucket delete should mint a marker"); + assert_eq!(deleted.version_id(), Some("null"), "the minted marker is the null version"); + } + + // The markers must be listed under the literal `null`, never a nil UUID. + let listed = client + .list_object_versions() + .bucket(bucket) + .send() + .await + .expect("list object versions"); + assert!(listed.versions().is_empty(), "the null versions were replaced by markers"); + let markers = listed.delete_markers(); + assert_eq!(markers.len(), 2); + for marker in markers { + assert_eq!(marker.version_id(), Some("null"), "listing must advertise the null version as `null`"); + } + + // Removing one marker by its listed version id over DeleteObject must + // acknowledge the marker identity on the wire. + let removed = client + .delete_object() + .bucket(bucket) + .key("doc/f1.txt") + .version_id("null") + .send() + .await + .expect("delete marker by null version id"); + assert_eq!( + removed.delete_marker(), + Some(true), + "x-amz-delete-marker must be true for a marker removal" + ); + assert_eq!(removed.version_id(), Some("null")); + + // Removing the other via DeleteObjects must produce an entry a client can + // correlate with its request: same key, version `null`, marker flags set. + let delete = Delete::builder() + .objects( + ObjectIdentifier::builder() + .key("doc/f2.txt") + .version_id("null") + .build() + .expect("object identifier"), + ) + .build() + .expect("delete payload"); + let batch = client + .delete_objects() + .bucket(bucket) + .delete(delete) + .send() + .await + .expect("delete objects by null version id"); + assert!(batch.errors().is_empty(), "batch delete reported errors: {:?}", batch.errors()); + let entries = batch.deleted(); + assert_eq!(entries.len(), 1); + assert_eq!(entries[0].key(), Some("doc/f2.txt")); + assert_eq!(entries[0].version_id(), Some("null"), "the entry must echo the requested identity"); + assert_eq!(entries[0].delete_marker(), Some(true)); + assert_eq!(entries[0].delete_marker_version_id(), Some("null")); + + // With identity round-tripping, one pass leaves the bucket truly empty + // and deletable. + let after = client + .list_object_versions() + .bucket(bucket) + .send() + .await + .expect("list versions after cleanup"); + assert!(after.versions().is_empty() && after.delete_markers().is_empty()); + client.delete_bucket().bucket(bucket).send().await.expect("delete bucket"); + + server.shutdown().await; +}