From 018f27d1cda27939d0163759544461dc922c73d2 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Wed, 5 Aug 2026 11:50:29 +0800 Subject: [PATCH] test(ecstore): deflake multipart listing tests under plain cargo test (#5730) Multipart upload ids embed the process-global deployment id at both create time and list time. Under plain cargo test (thread-parallel, shared process globals) a concurrently running test that re-initializes a store can swap the global between the two reads, making full-upload-id equality assertions fail spuriously (observed: core::sets::tests::list_multipart_uploads_merges_all_sets_without_pagination_loss failing when run concurrently with bucket::quota tests, passing in isolation). Add a test-only upload_uuid_suffix helper next to deployment_upload_id and make the affected assertions compare only the decoded x suffix. Where suffix normalization changes within-key ordering (base64 alphabet order is not byte order), both sides are sorted before comparison. nextest/CI is unaffected (process-per-test); this only hardens local plain cargo test runs. --- crates/ecstore/src/core/sets.rs | 14 +++++- crates/ecstore/src/runtime/sources.rs | 16 +++++++ crates/ecstore/src/set_disk/ops/multipart.rs | 49 ++++++++++++++++---- 3 files changed, 70 insertions(+), 9 deletions(-) diff --git a/crates/ecstore/src/core/sets.rs b/crates/ecstore/src/core/sets.rs index 5c57677d3..8bbcc474e 100644 --- a/crates/ecstore/src/core/sets.rs +++ b/crates/ecstore/src/core/sets.rs @@ -1749,7 +1749,19 @@ mod tests { upload_id_marker = page.next_upload_id_marker; } - assert_eq!(actual, expected, "set-level merge must return every upload exactly once"); + // Compare only the decoded `x` suffixes: the full + // upload id embeds the process-global deployment id, which a + // concurrently running test can swap between create and list time. + let normalize = |uploads: &[(String, String)]| { + let mut normalized = uploads + .iter() + .map(|(key, upload_id)| (key.clone(), runtime_sources::upload_uuid_suffix(upload_id))) + .collect::>(); + normalized.sort(); + normalized + }; + let actual = normalize(&actual); + assert_eq!(actual, normalize(&expected), "set-level merge must return every upload exactly once"); let mut deduped = actual.clone(); deduped.dedup(); assert_eq!(deduped.len(), actual.len(), "set-level pagination must not duplicate uploads"); diff --git a/crates/ecstore/src/runtime/sources.rs b/crates/ecstore/src/runtime/sources.rs index db8c7b455..a728b7a42 100644 --- a/crates/ecstore/src/runtime/sources.rs +++ b/crates/ecstore/src/runtime/sources.rs @@ -246,6 +246,22 @@ pub fn deployment_id() -> Option { get_global_deployment_id() } +/// Test-only inverse of [`deployment_upload_id`]: returns the raw +/// `x` suffix without the deployment-id prefix. Under plain +/// `cargo test` (thread-parallel, shared process globals) a concurrently +/// running test that re-initializes a store can swap the global deployment id +/// between create time and list time, so assertions must compare only this +/// suffix, never the full encoded upload id. +#[cfg(test)] +pub(crate) fn upload_uuid_suffix(upload_id: &str) -> String { + base64_simd::URL_SAFE_NO_PAD + .decode_to_vec(upload_id.as_bytes()) + .ok() + .and_then(|decoded| String::from_utf8(decoded).ok()) + .and_then(|decoded| decoded.split_once('.').map(|(_, suffix)| suffix.to_owned())) + .unwrap_or_else(|| upload_id.to_owned()) +} + pub(crate) fn replication_pool() -> Option> { crate::runtime::global::current_ctx().replication_pool() } diff --git a/crates/ecstore/src/set_disk/ops/multipart.rs b/crates/ecstore/src/set_disk/ops/multipart.rs index 7e91a7531..43a8c1b2b 100644 --- a/crates/ecstore/src/set_disk/ops/multipart.rs +++ b/crates/ecstore/src/set_disk/ops/multipart.rs @@ -4043,6 +4043,9 @@ mod tests { } // Start more in-progress uploads on the same object than a single page holds. + // Track only the decoded `x` suffixes: the full upload id + // embeds the process-global deployment id, which a concurrently running + // test can swap between create and list time. let total = 5usize; let mut created = HashSet::new(); for _ in 0..total { @@ -4050,7 +4053,10 @@ mod tests { .new_multipart_upload(bucket, object, &ObjectOptions::default()) .await .expect("multipart upload should be created"); - assert!(created.insert(res.upload_id), "each upload id must be unique"); + assert!( + created.insert(runtime_sources::upload_uuid_suffix(&res.upload_id)), + "each upload id must be unique" + ); } // A single page must never return more than max_uploads entries. @@ -4105,7 +4111,7 @@ mod tests { assert!(page.uploads.len() <= 1, "max_uploads=1 must never return more than one upload"); for upload in &page.uploads { assert!( - seen.insert(upload.upload_id.clone()), + seen.insert(runtime_sources::upload_uuid_suffix(&upload.upload_id)), "upload {} was returned more than once across pages", upload.upload_id ); @@ -4134,13 +4140,16 @@ mod tests { disk.make_volume(bucket).await.expect("bucket volume should be created"); } + // Compare only the decoded `x` suffixes: the full + // upload id embeds the process-global deployment id, which a + // concurrently running test can swap between create and list time. let mut expected = Vec::new(); for object in ["logs/a.bin", "logs/a.bin", "logs/b.bin", "other/c.bin"] { let upload = set_disks .new_multipart_upload(bucket, object, &ObjectOptions::default()) .await .expect("multipart upload should be created"); - expected.push((object.to_string(), upload.upload_id)); + expected.push((object.to_string(), runtime_sources::upload_uuid_suffix(&upload.upload_id))); } expected.sort(); @@ -4148,11 +4157,12 @@ mod tests { .list_multipart_uploads_for_incarnation(bucket, "", None, None, None, 1000, None) .await .expect("bucket-wide multipart listing should succeed"); - let listed = all + let mut listed = all .uploads .iter() - .map(|upload| (upload.object.clone(), upload.upload_id.clone())) + .map(|upload| (upload.object.clone(), runtime_sources::upload_uuid_suffix(&upload.upload_id))) .collect::>(); + listed.sort(); assert_eq!(listed, expected); assert!(!all.is_truncated); @@ -4216,7 +4226,18 @@ mod tests { assert!(upload_id_marker.is_some()); } - assert_eq!(listed, expected); + // Compare only the decoded `x` suffixes: the full + // upload id embeds the process-global deployment id, which a + // concurrently running test can swap between create and list time. + let normalize = |uploads: &[(String, String)]| { + let mut normalized = uploads + .iter() + .map(|(object, upload_id)| (object.clone(), runtime_sources::upload_uuid_suffix(upload_id))) + .collect::>(); + normalized.sort(); + normalized + }; + assert_eq!(normalize(&listed), normalize(&expected)); let key_only = set_disks .list_multipart_uploads_for_incarnation(bucket, "logs/", Some("logs/a.bin".to_string()), None, None, 1000, None) @@ -4335,7 +4356,13 @@ mod tests { .await .expect("incarnation-scoped multipart listing should succeed"); assert_eq!(scoped.uploads.len(), 1); - assert_eq!(scoped.uploads[0].upload_id, current.upload_id); + // Compare only the decoded `x` suffixes: the full + // upload id embeds the process-global deployment id, which a + // concurrently running test can swap between create and list time. + assert_eq!( + runtime_sources::upload_uuid_suffix(&scoped.uploads[0].upload_id), + runtime_sources::upload_uuid_suffix(¤t.upload_id) + ); } /// The `.` prefix inside an upload id is read from a @@ -5012,7 +5039,13 @@ mod tests { .list_multipart_uploads_for_incarnation(bucket, object, None, None, None, 1000, None) .await .expect("listing multipart uploads should succeed"); - page.uploads.iter().any(|u| u.upload_id == upload_id) + // Compare only the decoded `x` suffixes: the full + // upload id embeds the process-global deployment id, which a + // concurrently running test can swap between create and list time. + let expected_suffix = runtime_sources::upload_uuid_suffix(upload_id); + page.uploads + .iter() + .any(|u| runtime_sources::upload_uuid_suffix(&u.upload_id) == expected_suffix) } #[tokio::test]