From 3d17bab5444fa7e422648d290c4752c18017661a Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 7 Apr 2026 23:11:55 -0700 Subject: [PATCH] fix(seaweed-volume): eliminate global S3 tier registry races in tests Multiple Rust tests were racing on the shared global S3TierRegistry by calling clear(), which wiped entries registered by concurrently running tests. Use test-specific backend IDs and targeted remove() instead of clear() so tests no longer interfere with each other. --- seaweed-volume/src/remote_storage/s3_tier.rs | 5 ++++ seaweed-volume/src/server/grpc_server.rs | 17 ++++++++++---- seaweed-volume/src/server/heartbeat.rs | 19 ++++++++-------- seaweed-volume/src/storage/volume.rs | 24 ++++++++++++-------- 4 files changed, 41 insertions(+), 24 deletions(-) diff --git a/seaweed-volume/src/remote_storage/s3_tier.rs b/seaweed-volume/src/remote_storage/s3_tier.rs index be88adcf8..9d3281634 100644 --- a/seaweed-volume/src/remote_storage/s3_tier.rs +++ b/seaweed-volume/src/remote_storage/s3_tier.rs @@ -486,6 +486,11 @@ impl S3TierRegistry { self.backends.keys().cloned().collect() } + /// Remove a backend by name. + pub fn remove(&mut self, name: &str) { + self.backends.remove(name); + } + pub fn clear(&mut self) { self.backends.clear(); } diff --git a/seaweed-volume/src/server/grpc_server.rs b/seaweed-volume/src/server/grpc_server.rs index e0d204fad..ecdee7894 100644 --- a/seaweed-volume/src/server/grpc_server.rs +++ b/seaweed-volume/src/server/grpc_server.rs @@ -4311,7 +4311,9 @@ mod tests { std::fs::remove_file(&dat_path).unwrap(); let (endpoint, shutdown_tx) = spawn_fake_s3_server(dat_bytes.clone()); - global_s3_tier_registry().write().unwrap().clear(); + // Use a test-specific backend_id to avoid racing with other tests + // that share the global registry. Never call clear() — only + // register/remove our own entries. let tier_config = S3TierConfig { access_key: "access".to_string(), secret_key: "secret".to_string(), @@ -4323,14 +4325,16 @@ mod tests { }; { let mut registry = global_s3_tier_registry().write().unwrap(); - registry.register("s3.default".to_string(), S3TierBackend::new(&tier_config)); - registry.register("s3".to_string(), S3TierBackend::new(&tier_config)); + registry.register( + "s3.incr_copy_test".to_string(), + S3TierBackend::new(&tier_config), + ); } let vif = crate::storage::volume::VifVolumeInfo { files: vec![crate::storage::volume::VifRemoteFile { backend_type: "s3".to_string(), - backend_id: "default".to_string(), + backend_id: "incr_copy_test".to_string(), key: "remote-key".to_string(), offset: 0, file_size: dat_bytes.len() as u64, @@ -4539,7 +4543,10 @@ mod tests { assert_eq!(copied, dat_bytes[super_block_size as usize..]); let _ = shutdown_tx.send(()); - global_s3_tier_registry().write().unwrap().clear(); + global_s3_tier_registry() + .write() + .unwrap() + .remove("s3.incr_copy_test"); } #[tokio::test] diff --git a/seaweed-volume/src/server/heartbeat.rs b/seaweed-volume/src/server/heartbeat.rs index 6fcfd523c..434e841cf 100644 --- a/seaweed-volume/src/server/heartbeat.rs +++ b/seaweed-volume/src/server/heartbeat.rs @@ -1408,10 +1408,8 @@ mod tests { #[test] fn test_apply_storage_backends_registers_s3_default_aliases() { let state = test_state_with_store(Store::new(NeedleMapKind::InMemory)); - crate::remote_storage::s3_tier::global_s3_tier_registry() - .write() - .unwrap() - .clear(); + // Do not call clear() on the global registry — other tests may be + // running concurrently. Just register our entries and verify them. apply_storage_backends( &state, @@ -1443,10 +1441,8 @@ mod tests { #[test] fn test_apply_storage_backends_ignores_unsupported_types() { let state = test_state_with_store(Store::new(NeedleMapKind::InMemory)); - crate::remote_storage::s3_tier::global_s3_tier_registry() - .write() - .unwrap() - .clear(); + // Do not call clear() on the global registry — other tests may be + // running concurrently. apply_storage_backends( &state, @@ -1457,12 +1453,17 @@ mod tests { }], ); + // The per-state registry is freshly created and should have no entries + // since "rclone" is unsupported. let registry = state.s3_tier_registry.read().unwrap(); assert!(registry.names().is_empty()); + // Only check that the unsupported type was not added to the global + // registry. Other tests may have their own entries present. let global_registry = crate::remote_storage::s3_tier::global_s3_tier_registry() .read() .unwrap(); - assert!(global_registry.names().is_empty()); + assert!(global_registry.get("rclone.default").is_none()); + assert!(global_registry.get("rclone").is_none()); } #[test] diff --git a/seaweed-volume/src/storage/volume.rs b/seaweed-volume/src/storage/volume.rs index 5e4688115..2ee94b5b2 100644 --- a/seaweed-volume/src/storage/volume.rs +++ b/seaweed-volume/src/storage/volume.rs @@ -3917,6 +3917,11 @@ mod tests { .unwrap(); assert!(deleted_size.0 > 0); assert_eq!(v.dat_file_size().unwrap(), dat_size_before_reload); + + crate::remote_storage::s3_tier::global_s3_tier_registry() + .write() + .unwrap() + .remove("s3.vif_rw_test"); } #[test] @@ -4079,10 +4084,9 @@ mod tests { std::fs::remove_file(&dat_path).unwrap(); let (endpoint, shutdown_tx) = spawn_fake_s3_server(dat_bytes.clone()); - crate::remote_storage::s3_tier::global_s3_tier_registry() - .write() - .unwrap() - .clear(); + // Use a test-specific backend_id to avoid racing with other tests + // that share the global registry. Never call clear() — only + // register/remove our own entries. let tier_config = crate::remote_storage::s3_tier::S3TierConfig { access_key: "access".to_string(), secret_key: "secret".to_string(), @@ -4097,11 +4101,7 @@ mod tests { .write() .unwrap(); registry.register( - "s3.default".to_string(), - crate::remote_storage::s3_tier::S3TierBackend::new(&tier_config), - ); - registry.register( - "s3".to_string(), + "s3.remote_only_rw".to_string(), crate::remote_storage::s3_tier::S3TierBackend::new(&tier_config), ); } @@ -4109,7 +4109,7 @@ mod tests { let vif = VifVolumeInfo { files: vec![VifRemoteFile { backend_type: "s3".to_string(), - backend_id: "default".to_string(), + backend_id: "remote_only_rw".to_string(), key: "remote-key".to_string(), offset: 0, file_size: dat_bytes.len() as u64, @@ -4166,6 +4166,10 @@ mod tests { assert_eq!(meta.data_size, 11); let _ = shutdown_tx.send(()); + crate::remote_storage::s3_tier::global_s3_tier_registry() + .write() + .unwrap() + .remove("s3.remote_only_rw"); } /// Volume destroy removes .vif alongside the primary data files.