diff --git a/Cargo.lock b/Cargo.lock index e824ff26a..15d6ffc12 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11829,6 +11829,7 @@ dependencies = [ "futures-util", "libc", "pin-project-lite", + "slab", "tokio", ] diff --git a/crates/config/src/constants/object.rs b/crates/config/src/constants/object.rs index 28222c824..bab33270b 100644 --- a/crates/config/src/constants/object.rs +++ b/crates/config/src/constants/object.rs @@ -116,6 +116,27 @@ pub const ENV_OBJECT_GET_SKIP_BITROT_VERIFY: &str = "RUSTFS_OBJECT_GET_SKIP_BITR /// Default: bitrot verification is enabled on GetObject reads (do not skip). pub const DEFAULT_OBJECT_GET_SKIP_BITROT_VERIFY: bool = false; +/// Request writing the complete remote-tier version state into object metadata. +/// +/// This remains ineffective until +/// [`ENV_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED`] is also enabled. +pub const ENV_TIER_REMOTE_VERSION_STATE_WRITE: &str = "RUSTFS_TIER_REMOTE_VERSION_STATE_WRITE"; +pub const DEFAULT_TIER_REMOTE_VERSION_STATE_WRITE: bool = false; + +/// Operator-attested fleet-wide confirmation for +/// [`ENV_TIER_REMOTE_VERSION_STATE_WRITE`]. +/// +/// This flag is an operational contract, not automatic capability discovery. +/// Operators may enable it only after every node that can write or read +/// transitioned object metadata supports the remote version-state schema and +/// semantics. Keeping the confirmation separate makes a single-node request or +/// a writer whose local opt-in is removed fail closed. +pub const ENV_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED: &str = "RUSTFS_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED"; +pub const DEFAULT_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED: bool = false; + +const _: () = assert!(!DEFAULT_TIER_REMOTE_VERSION_STATE_WRITE); +const _: () = assert!(!DEFAULT_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED); + // ============================================================================= // Concurrent Request Fix - Timeout and Backpressure Configuration // ============================================================================= @@ -617,3 +638,15 @@ pub const ENV_OBJECT_IO_RANDOM_READAHEAD_DISABLE_CONCURRENCY: &str = "RUSTFS_OBJ /// Default read-ahead disable concurrency threshold: 4. pub const DEFAULT_OBJECT_IO_RANDOM_READAHEAD_DISABLE_CONCURRENCY: usize = 4; + +#[cfg(test)] +mod remote_version_state_tests { + #[test] + fn remote_version_state_gate_uses_stable_environment_names() { + assert_eq!(super::ENV_TIER_REMOTE_VERSION_STATE_WRITE, "RUSTFS_TIER_REMOTE_VERSION_STATE_WRITE"); + assert_eq!( + super::ENV_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED, + "RUSTFS_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED" + ); + } +} diff --git a/crates/e2e_test/src/reliant/grpc_lock_server.rs b/crates/e2e_test/src/reliant/grpc_lock_server.rs index 301125188..76284ad2d 100644 --- a/crates/e2e_test/src/reliant/grpc_lock_server.rs +++ b/crates/e2e_test/src/reliant/grpc_lock_server.rs @@ -23,7 +23,8 @@ use rustfs_protos::{ proto_gen::node_service::{ BatchGenerallyLockRequest, BatchGenerallyLockResponse, BatchReadVersionRequest, BatchReadVersionResponse, GenerallyLockRequest, GenerallyLockResponse, GenerallyLockResult, PingRequest, PingResponse, - node_service_server::NodeService, + SnapshotLeaseMutationResponse, SnapshotLeaseReleaseRequest, SnapshotLeaseRenewRequest, SnapshotLeaseRequest, + SnapshotLeaseResponse, node_service_server::NodeService, }, }; use std::pin::Pin; @@ -104,6 +105,27 @@ impl NodeService for MinimalLockNodeService { Err(Status::unimplemented("MinimalLockNodeService only supports lock RPCs")) } + async fn acquire_snapshot_lease( + &self, + _request: Request, + ) -> Result, Status> { + Err(Status::unimplemented("MinimalLockNodeService only supports lock RPCs")) + } + + async fn renew_snapshot_lease( + &self, + _request: Request, + ) -> Result, Status> { + Err(Status::unimplemented("MinimalLockNodeService only supports lock RPCs")) + } + + async fn release_snapshot_lease( + &self, + _request: Request, + ) -> Result, Status> { + Err(Status::unimplemented("MinimalLockNodeService only supports lock RPCs")) + } + async fn lock(&self, request: Request) -> Result, Status> { let request = request.into_inner(); let args: LockRequest = match serde_json::from_str(&request.args) { diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index 3491103ef..0aafe9ded 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -305,7 +305,8 @@ pub mod disk { CheckPartsResp, DeleteOptions, Disk, DiskAPI, DiskInfo, DiskInfoOptions, DiskLocation, DiskOption, DiskStore, FileInfoVersions, FileReader, FileWriter, HEALING_MARKER_PATH, NsScannerOpenRequest, OldCurrentSize, PartTransactionAction, RUSTFS_META_BUCKET, ReadMultipleReq, ReadMultipleResp, ReadOptions, RenameDataResp, - STORAGE_FORMAT_FILE, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, new_disk, validate_batch_read_version_item_count, + STORAGE_FORMAT_FILE, SnapshotLeaseToken, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, new_disk, + validate_batch_read_version_item_count, }; pub use bytes::Bytes; pub use endpoint::Endpoint; diff --git a/crates/ecstore/src/bucket/metadata_sys.rs b/crates/ecstore/src/bucket/metadata_sys.rs index dbd9bb573..2a30a17be 100644 --- a/crates/ecstore/src/bucket/metadata_sys.rs +++ b/crates/ecstore/src/bucket/metadata_sys.rs @@ -23,7 +23,7 @@ use crate::error::{Error, Result, is_err_bucket_not_found}; use crate::runtime::sources as runtime_sources; use crate::storage_api_contracts::heal::HealOperations as _; use crate::storage_api_contracts::namespace::NamespaceLocking as _; -use crate::store::ECStore; +use crate::store::{ECStore, await_bucket_namespace_operation}; use futures::future::join_all; use rustfs_common::heal_channel::HealOpts; use rustfs_policy::policy::BucketPolicy; @@ -37,13 +37,19 @@ use std::collections::HashSet; use std::time::Duration; use std::{collections::HashMap, sync::Arc}; use time::OffsetDateTime; -use tokio::sync::RwLock; +use tokio::sync::{Mutex, RwLock}; use tokio::time::sleep; use tokio_util::sync::CancellationToken; use tracing::{error, warn}; const BUCKET_METADATA_REFRESH_INTERVAL: Duration = Duration::from_secs(15 * 60); +#[derive(Clone, Copy)] +enum MetadataLoadMode { + Initial, + Refresh, +} + pub async fn init_bucket_metadata_sys(api: Arc, buckets: Vec) { // The metadata system is inherently per-store (it holds the store handle // and that store's bucket cache), so it lives on the store's own instance @@ -140,7 +146,8 @@ async fn refresh_buckets_metadata_once(sys: Arc>) { for chunk in buckets.chunks(count) { let sys = sys.read().await; - sys.concurrent_load(chunk, &mut failed_buckets).await; + sys.concurrent_load(chunk, &mut failed_buckets, MetadataLoadMode::Refresh) + .await; } if !failed_buckets.is_empty() { @@ -461,6 +468,9 @@ const ABSENT_BUCKET_METADATA_MAX_ENTRIES: u64 = 10_000; #[derive(Debug)] pub struct BucketMetadataSys { metadata_map: RwLock>>, + metadata_publish_lock: Mutex<()>, + #[cfg(test)] + lazy_load_lock_probe: std::sync::atomic::AtomicBool, /// Buckets recently observed to have no persisted metadata. Serving the /// fabricated default from here (instead of re-reading disk) keeps the /// per-request cost of repeated lookups for such names bounded — without @@ -476,6 +486,9 @@ impl BucketMetadataSys { pub fn new(api: Arc) -> Self { Self { metadata_map: RwLock::new(HashMap::new()), + metadata_publish_lock: Mutex::new(()), + #[cfg(test)] + lazy_load_lock_probe: std::sync::atomic::AtomicBool::new(false), absent_metadata: moka::future::Cache::builder() .max_capacity(ABSENT_BUCKET_METADATA_MAX_ENTRIES) .time_to_live(ABSENT_BUCKET_METADATA_TTL) @@ -502,11 +515,13 @@ impl BucketMetadataSys { loop { if buckets.len() < count { - self.concurrent_load(buckets, &mut failed_buckets).await; + self.concurrent_load(buckets, &mut failed_buckets, MetadataLoadMode::Initial) + .await; break; } - self.concurrent_load(&buckets[..count], &mut failed_buckets).await; + self.concurrent_load(&buckets[..count], &mut failed_buckets, MetadataLoadMode::Initial) + .await; buckets = &buckets[count..] } @@ -517,7 +532,7 @@ impl BucketMetadataSys { Ok(()) } - async fn concurrent_load(&self, buckets: &[String], failed_buckets: &mut HashSet) { + async fn concurrent_load(&self, buckets: &[String], failed_buckets: &mut HashSet, mode: MetadataLoadMode) { let mut futures = Vec::new(); for bucket in buckets.iter() { @@ -525,16 +540,55 @@ impl BucketMetadataSys { let bucket = bucket.clone(); futures.push(async move { sleep(Duration::from_millis(30)).await; - let _ = api - .heal_bucket( - &bucket, - &HealOpts { - recreate: true, - ..Default::default() - }, - ) - .await; - load_bucket_metadata_parse_with_presence(self.api.clone(), bucket.as_str(), true).await + match mode { + MetadataLoadMode::Initial => { + let _ = api + .heal_bucket( + &bucket, + &HealOpts { + recreate: true, + ..Default::default() + }, + ) + .await; + let (bm, persisted) = + load_bucket_metadata_parse_with_presence(self.api.clone(), bucket.as_str(), true).await?; + if persisted { + self.set(bucket, Arc::new(bm)).await; + } else { + let _publish_guard = self.metadata_publish_lock.lock().await; + let mut map = self.metadata_map.write().await; + map.entry(bucket).or_insert_with(|| Arc::new(bm)); + } + } + MetadataLoadMode::Refresh => { + let expected = self.metadata_map.read().await.get(&bucket).cloned(); + let heal_lock = api.new_ns_lock(&bucket, &bucket).await?; + let heal_guard = heal_lock.get_read_lock(crate::set_disk::get_lock_acquire_timeout()).await?; + await_bucket_namespace_operation( + Some(&heal_guard), + &bucket, + "bucket metadata refresh heal", + api.heal_bucket(&bucket, &HealOpts::default()), + ) + .await?; + drop(heal_guard); + let (bm, persisted) = + load_bucket_metadata_parse_with_presence(self.api.clone(), bucket.as_str(), true).await?; + let publish_lock = api.new_ns_lock(&bucket, &bucket).await?; + let guard = publish_lock + .get_read_lock(crate::set_disk::get_lock_acquire_timeout()) + .await?; + if guard.is_lock_lost() { + return Err(Error::other(format!( + "bucket namespace lock was lost before bucket metadata refresh publish: {bucket}" + ))); + } + self.publish_refresh_if_unchanged(&bucket, expected.as_ref(), bm, persisted) + .await; + } + } + Ok::<(), Error>(()) }); } @@ -542,26 +596,7 @@ impl BucketMetadataSys { for (idx, res) in results.into_iter().enumerate() { match res { - Ok((bm, persisted)) => { - if let Some(bucket) = buckets.get(idx) { - if persisted { - self.set(bucket.clone(), Arc::new(bm)).await; - } else { - // A fabricated default (no persisted metadata - // readable right now) must never REPLACE an - // existing entry: the periodic refresh would - // otherwise downgrade a lock-enabled bucket to an - // authoritative "no lock" default on a transient - // ConfigNotFound, disabling the object-lock - // delete gate and wiping its target/durability - // sync state. Insert-if-vacant keeps the startup - // behavior for legacy buckets without a metadata - // file, atomically under the map write lock. - let mut map = self.metadata_map.write().await; - map.entry(bucket.clone()).or_insert_with(|| Arc::new(bm)); - } - } - } + Ok(()) => {} Err(e) => { error!("Unable to load bucket metadata, will be retried: {:?}", e); if let Some(bucket) = buckets.get(idx) { @@ -572,6 +607,32 @@ impl BucketMetadataSys { } } + async fn publish_refresh_if_unchanged( + &self, + bucket: &str, + expected: Option<&Arc>, + metadata: BucketMetadata, + persisted: bool, + ) { + if !persisted { + return; + } + let _publish_guard = self.metadata_publish_lock.lock().await; + let metadata = Arc::new(metadata); + let mut map = self.metadata_map.write().await; + let unchanged = expected + .zip(map.get(bucket)) + .is_some_and(|(expected, current)| Arc::ptr_eq(expected, current)); + if !unchanged { + return; + } + map.insert(bucket.to_string(), Arc::clone(&metadata)); + drop(map); + self.absent_metadata.invalidate(bucket).await; + sync_bucket_target_sys(bucket, &metadata).await; + sync_bucket_durability(bucket, &metadata); + } + pub async fn get(&self, bucket: &str) -> Result> { if is_meta_bucketname(bucket) { return Err(Error::ConfigNotFound); @@ -587,6 +648,7 @@ impl BucketMetadataSys { pub async fn set(&self, bucket: String, bm: Arc) { if !is_meta_bucketname(&bucket) { + let _publish_guard = self.metadata_publish_lock.lock().await; let mut map = self.metadata_map.write().await; map.insert(bucket.clone(), bm.clone()); drop(map); @@ -604,6 +666,7 @@ impl BucketMetadataSys { if is_meta_bucketname(bucket) { return false; } + let _publish_guard = self.metadata_publish_lock.lock().await; let mut map = self.metadata_map.write().await; let removed = map.remove(bucket).is_some(); drop(map); @@ -735,7 +798,24 @@ impl BucketMetadataSys { return Ok((Arc::new(bm), true)); } - let (bm, persisted) = match load_bucket_metadata_parse_with_presence(self.api.clone(), bucket, true).await { + let lock = self.api.new_ns_lock(bucket, bucket).await?; + let guard = lock.get_read_lock(crate::set_disk::get_lock_acquire_timeout()).await?; + #[cfg(test)] + if self.lazy_load_lock_probe.load(std::sync::atomic::Ordering::Relaxed) { + let competing = self.api.new_ns_lock(bucket, bucket).await?; + assert!( + competing.get_write_lock(Duration::from_millis(20)).await.is_err(), + "lazy metadata IO must start while the bucket namespace read lock is held" + ); + } + let (bm, persisted) = match await_bucket_namespace_operation( + Some(&guard), + bucket, + "lazy bucket metadata load", + Box::pin(load_bucket_metadata_parse_with_presence(self.api.clone(), bucket, true)), + ) + .await + { Ok(res) => res, Err(err) => { return if *self.initialized.read().await { @@ -759,9 +839,33 @@ impl BucketMetadataSys { // defaults for buckets listed on disk — legacy buckets without a // metadata file — but never lets one replace an existing entry.) if persisted { + await_bucket_namespace_operation( + Some(&guard), + bucket, + "lazy bucket metadata existence check", + Box::pin(async { + self.api + .peer_sys + .get_bucket_info(bucket, &crate::storage_api_contracts::bucket::BucketOptions::default()) + .await + .map(|_| ()) + .map_err(Into::into) + }), + ) + .await?; + if guard.is_lock_lost() { + return Err(Error::other(format!( + "bucket namespace lock was lost before lazy bucket metadata publish: {bucket}" + ))); + } + let _publish_guard = self.metadata_publish_lock.lock().await; let mut map = self.metadata_map.write().await; + if let Some(current) = map.get(bucket) { + return Ok((Arc::clone(current), true)); + } map.insert(bucket.to_string(), bm.clone()); drop(map); + self.absent_metadata.invalidate(bucket).await; sync_bucket_target_sys(bucket, &bm).await; sync_bucket_durability(bucket, &bm); } else { @@ -1047,12 +1151,13 @@ mod tests { /// Pins the fail-closed caching contract of the lazy `get_config` path /// and the refresh no-replace rule: fabricated defaults are returned but /// never served by the map-only `get()`, persisted metadata is cached on - /// lazy load (superseding a recorded absence), and a refresh-load miss - /// never replaces an existing entry. + /// lazy load (superseding a recorded absence), a refresh-load miss never + /// replaces an existing entry or heals a deleted bucket, and initial load + /// still heals buckets discovered from storage. #[tokio::test] async fn get_config_never_caches_fabricated_defaults_as_authoritative() { - let (_dirs, ecstore) = isolated_store_over_temp_disks().await; - let sys = BucketMetadataSys::new(ecstore); + let (dirs, ecstore) = isolated_store_over_temp_disks().await; + let sys = Arc::new(BucketMetadataSys::new(ecstore)); // (a) Miss: the fabricated default is returned but not cached. let (bm, _) = sys @@ -1078,6 +1183,9 @@ mod tests { let mut persisted = BucketMetadata::new("absent-bucket"); persisted.policy_config_json = b"persisted-marker".to_vec(); sys.persist_and_set(persisted).await.expect("metadata should persist"); + for dir in &dirs { + std::fs::create_dir_all(dir.path().join("absent-bucket")).expect("persisted bucket directory should be created"); + } sys.metadata_map.write().await.clear(); let _ = sys .get_config("absent-bucket") @@ -1089,14 +1197,47 @@ mod tests { .expect("lazily loaded persisted metadata must be cached"); assert_eq!(cached.policy_config_json, b"persisted-marker".to_vec()); - // (c) A refresh-load miss (no persisted metadata readable) must not - // replace an existing entry. + // (c) Persisted metadata left behind after physical deletion must not + // be lazily republished as a live bucket generation. + let mut deleted_lazy = BucketMetadata::new("deleted-lazy-bucket"); + deleted_lazy.policy_config_json = b"stale-generation".to_vec(); + sys.persist_and_set(deleted_lazy) + .await + .expect("stale metadata should persist"); + sys.metadata_map.write().await.remove("deleted-lazy-bucket"); + assert!( + sys.get_config("deleted-lazy-bucket").await.is_err(), + "lazy load must fail when the physical bucket no longer exists" + ); + assert!(sys.get("deleted-lazy-bucket").await.is_err()); + + // (d) The namespace generation fence must be acquired before lazy + // metadata IO, so a writer can replace the generation atomically. + let fenced_bucket = "fenced-lazy-bucket"; + for dir in &dirs { + std::fs::create_dir_all(dir.path().join(fenced_bucket)).unwrap(); + } + let mut old_fenced = BucketMetadata::new(fenced_bucket); + old_fenced.policy_config_json = b"old-fenced-generation".to_vec(); + sys.persist_and_set(old_fenced).await.unwrap(); + sys.metadata_map.write().await.remove(fenced_bucket); + sys.lazy_load_lock_probe.store(true, std::sync::atomic::Ordering::Relaxed); + let (loaded, _) = sys.get_config(fenced_bucket).await.unwrap(); + sys.lazy_load_lock_probe.store(false, std::sync::atomic::Ordering::Relaxed); + assert_eq!(loaded.policy_config_json, b"old-fenced-generation".to_vec()); + + // (e) A refresh-load miss for a bucket that still exists must not + // replace an existing entry with a fabricated default. let mut kept = BucketMetadata::new("kept-bucket"); kept.policy_config_json = b"kept-marker".to_vec(); sys.set("kept-bucket".to_string(), Arc::new(kept)).await; + for dir in &dirs { + std::fs::create_dir_all(dir.path().join("kept-bucket")).expect("kept bucket directory should be created"); + } let mut failed = HashSet::new(); let refresh_targets = vec!["kept-bucket".to_string()]; - sys.concurrent_load(&refresh_targets, &mut failed).await; + sys.concurrent_load(&refresh_targets, &mut failed, MetadataLoadMode::Refresh) + .await; let kept = sys .get("kept-bucket") .await @@ -1106,6 +1247,50 @@ mod tests { b"kept-marker".to_vec(), "a fabricated refresh default must not replace real metadata" ); + + // (f) A stale cache entry for a physically deleted bucket must not + // recreate the bucket during periodic refresh. + sys.set("deleted-bucket".to_string(), Arc::new(BucketMetadata::new("deleted-bucket"))) + .await; + let deleted_targets = vec!["deleted-bucket".to_string()]; + sys.concurrent_load(&deleted_targets, &mut failed, MetadataLoadMode::Refresh) + .await; + assert!( + dirs.iter().all(|dir| !dir.path().join("deleted-bucket").exists()), + "periodic refresh must not recreate a bucket from stale cached metadata" + ); + + // (g) Metadata loaded for an old bucket generation must not replace + // metadata published by delete plus same-name recreation. + let old = Arc::new(BucketMetadata::new("recreated-bucket")); + sys.set("recreated-bucket".to_string(), Arc::clone(&old)).await; + let mut recreated = BucketMetadata::new("recreated-bucket"); + recreated.policy_config_json = b"new-generation".to_vec(); + sys.set("recreated-bucket".to_string(), Arc::new(recreated)).await; + let mut stale = BucketMetadata::new("recreated-bucket"); + stale.policy_config_json = b"old-generation".to_vec(); + sys.publish_refresh_if_unchanged("recreated-bucket", Some(&old), stale, true) + .await; + assert_eq!(sys.get("recreated-bucket").await.unwrap().policy_config_json, b"new-generation".to_vec()); + + // (f) Refresh retains periodic healing for a partially missing bucket. + sys.set("partial-bucket".to_string(), Arc::new(BucketMetadata::new("partial-bucket"))) + .await; + for dir in dirs.iter().take(3) { + std::fs::create_dir_all(dir.path().join("partial-bucket")).unwrap(); + } + sys.concurrent_load(&["partial-bucket".to_string()], &mut failed, MetadataLoadMode::Refresh) + .await; + assert!(dirs.iter().all(|dir| dir.path().join("partial-bucket").is_dir())); + + // (g) Initial discovery retains the historical unconditional heal. + let initial_targets = vec!["initial-bucket".to_string()]; + sys.concurrent_load(&initial_targets, &mut failed, MetadataLoadMode::Initial) + .await; + assert!( + dirs.iter().all(|dir| dir.path().join("initial-bucket").is_dir()), + "initial load must heal buckets discovered from storage" + ); } #[tokio::test] @@ -1130,12 +1315,18 @@ mod tests { #[tokio::test] async fn update_config_with_persists_tagging_rewrite_across_disk_reload() { use crate::bucket::metadata::BUCKET_TAGGING_CONFIG; + use crate::storage_api_contracts::bucket::MakeBucketOptions; use s3s::dto::Tag; let (_dirs, ecstore) = isolated_store_over_temp_disks().await; - let mut sys = BucketMetadataSys::new(ecstore); let bucket = "swift-tagging-bucket"; + ecstore + .peer_sys + .make_bucket(bucket, &MakeBucketOptions::default()) + .await + .expect("bucket volume should be created"); + let mut sys = BucketMetadataSys::new(ecstore); sys.persist_and_set(BucketMetadata::new(bucket)) .await .expect("initial metadata should persist"); diff --git a/crates/ecstore/src/cluster/rpc/peer_rest_client.rs b/crates/ecstore/src/cluster/rpc/peer_rest_client.rs index 11a32b001..ab7a62fc8 100644 --- a/crates/ecstore/src/cluster/rpc/peer_rest_client.rs +++ b/crates/ecstore/src/cluster/rpc/peer_rest_client.rs @@ -1628,10 +1628,14 @@ impl PeerRestClient { pub async fn load_transition_tier_config(&self) -> Result<()> { match self.load_transition_tier_config_outcome().await { TierConfigReloadOutcome::Success => Ok(()), - TierConfigReloadOutcome::TransientReconnect(err) | TierConfigReloadOutcome::TransientRetrySameChannel(err) => { - self.finalize_result(Err(err)).await - } - TierConfigReloadOutcome::Terminal(err) => Err(err), + // Only a reconnect-class failure says anything about the channel. + // `finalize_result` marks the peer offline and evicts its connection + // whenever the message looks network-like, and a peer that answered + // and rejected the apply can easily report one ("release RPC failed: + // transport error"). Routing those through here would gate a healthy, + // responding peer out of every unrelated RPC. + TierConfigReloadOutcome::TransientReconnect(err) => self.finalize_result(Err(err)).await, + TierConfigReloadOutcome::TransientRetrySameChannel(err) | TierConfigReloadOutcome::Terminal(err) => Err(err), } } @@ -1710,13 +1714,24 @@ fn is_tier_config_reload_connection_failure(err: &Error) -> bool { message_has_network_needle(&message) } +/// Classifies a reload the peer answered but refused to apply. +/// +/// The peer replied, so the channel is healthy and only the remote apply +/// failed. Those failures are transient by nature: the reload reads the tier +/// mutation intents and takes the distributed tier-config lock, both of which +/// fail while any other node is restarting or while the lock quorum is briefly +/// disturbed. Retiring the worker on the first such rejection leaves that peer +/// pinned to the old configuration with nothing left to heal it, so it answers +/// `TierNotFound` for a tier the rest of the cluster already committed until a +/// second admin mutation happens to spawn a fresh worker. +/// +/// Convergence is the whole point of this path, so a rejection is retried on +/// the same channel. The worker's exponential backoff caps the cost at one +/// reload every `TIER_CONFIG_RELOAD_RETRY_CAP`, and `Terminal` stays reachable +/// for transport and gRPC status failures, which is where a genuinely +/// unrecoverable peer surfaces. fn tier_config_reload_remote_failure(error_info: Option) -> TierConfigReloadOutcome { - let error_info = error_info.unwrap_or_default(); - if matches!(error_info.as_str(), "errServerNotInitialized" | "ServerNotInitialized") { - TierConfigReloadOutcome::TransientRetrySameChannel(Error::other(error_info)) - } else { - TierConfigReloadOutcome::Terminal(Error::other(error_info)) - } + TierConfigReloadOutcome::TransientRetrySameChannel(Error::other(error_info.unwrap_or_default())) } fn tier_config_reload_status_outcome(status: tonic::Status) -> TierConfigReloadOutcome { @@ -1726,6 +1741,14 @@ fn tier_config_reload_status_outcome(status: tonic::Status) -> TierConfigReloadO TierConfigReloadOutcome::TransientReconnect(status.into()) } else if status.code() == Code::Unknown && status.message().starts_with("Service was not ready:") { TierConfigReloadOutcome::TransientRetrySameChannel(status.into()) + } else if status.code() == Code::Unknown + && is_tier_config_reload_connection_failure(&Error::other(status.message().to_string())) + { + // tonic reports a connection dropped mid-call as `Unknown` carrying the + // transport error text rather than as `Unavailable`, which is what a peer + // restarting under an active mutation produces. Reconnect and retry, so + // the restart does not permanently retire this peer's reload worker. + TierConfigReloadOutcome::TransientReconnect(status.into()) } else { TierConfigReloadOutcome::Terminal(status.into()) } @@ -2279,9 +2302,12 @@ mod tests { tier_config_reload_status_outcome(tonic::Status::cancelled("request cancelled")), TierConfigReloadOutcome::Terminal(_) )); + // A peer that answered and then refused the apply is retried rather than + // retired: the channel is healthy, so the rejection reflects remote state + // that the next attempt can find healed. assert!(matches!( tier_config_reload_remote_failure(Some("backend unavailable".to_string())), - TierConfigReloadOutcome::Terminal(_) + TierConfigReloadOutcome::TransientRetrySameChannel(_) )); assert!(matches!( tier_config_reload_remote_failure(Some("errServerNotInitialized".to_string())), @@ -2305,6 +2331,50 @@ mod tests { )); } + /// A tier mutation issued while another node restarts must still converge on + /// the nodes that stayed up. Those peers answer the reload RPC and reject the + /// apply, because reloading reads the tier mutation intents and takes the + /// distributed tier-config lock while the lock quorum is still disturbed. + /// Classifying those rejections as terminal retired the reload worker on its + /// first attempt and pinned the peer to the previous configuration, so it + /// served `TierNotFound` for an already-committed tier until an unrelated + /// second admin mutation spawned a new worker. + #[test] + fn tier_config_reload_retries_peers_that_reject_the_apply_mid_restart() { + for error_info in [ + "Lock acquisition timeout for resource '.rustfs.sys/config/tier-config.bin.lock' after 5s", + "Resource '.rustfs.sys/config/tier-config.bin.lock' is already locked by node-3", + "Internal error: release RPC failed: transport error", + "save_config_with_opts: err: PreconditionFailed", + "erasure read quorum", + ] { + assert!( + matches!( + tier_config_reload_remote_failure(Some(error_info.to_string())), + TierConfigReloadOutcome::TransientRetrySameChannel(_) + ), + "a peer that rejected the apply must stay retryable so it converges: {error_info}" + ); + } + + // An absent error message is still a rejection, not a reason to stop. + assert!(matches!( + tier_config_reload_remote_failure(None), + TierConfigReloadOutcome::TransientRetrySameChannel(_) + )); + + // tonic surfaces a connection dropped mid-call as `Unknown`, not `Unavailable`. + assert!(matches!( + tier_config_reload_status_outcome(tonic::Status::unknown("transport error")), + TierConfigReloadOutcome::TransientReconnect(_) + )); + // An `Unknown` that is not transport-shaped stays terminal. + assert!(matches!( + tier_config_reload_status_outcome(tonic::Status::unknown("peer response unknown")), + TierConfigReloadOutcome::Terminal(_) + )); + } + #[tokio::test] async fn tier_config_reload_single_attempt_clears_offline_gate_without_redial() { let client = test_peer_client(); diff --git a/crates/ecstore/src/cluster/rpc/peer_s3_client.rs b/crates/ecstore/src/cluster/rpc/peer_s3_client.rs index 17d42e84d..67d04ab23 100644 --- a/crates/ecstore/src/cluster/rpc/peer_s3_client.rs +++ b/crates/ecstore/src/cluster/rpc/peer_s3_client.rs @@ -89,6 +89,22 @@ fn reduce_pool_write_quorum_errs(per_pool_errs: &[Option]) -> Option]) -> Result<()> { + if opts.recreate { + return Ok(()); + } + if let Some(err) = pool_errs + .iter() + .flatten() + .find(|err| **err != Error::DiskNotFound && **err != Error::VolumeNotFound) + { + return Err(err.clone()); + } + opts.remove = is_all_buckets_not_found(pool_errs); + opts.recreate = !opts.remove; + Ok(()) +} + #[async_trait] pub trait PeerS3Client: Debug + Sync + Send + 'static { async fn heal_bucket(&self, bucket: &str, opts: &HealOpts) -> Result; @@ -159,10 +175,7 @@ impl S3PeerSys { pool_errs.push(reduce_pool_write_quorum_errs(&per_pool_errs)); } - if !opts.recreate { - opts.remove = is_all_buckets_not_found(&pool_errs); - opts.recreate = !opts.remove; - } + resolve_heal_bucket_mode(&mut opts, &pool_errs)?; let mut futures = Vec::new(); let heal_bucket_results = Arc::new(RwLock::new(vec![HealResultItem::default(); self.clients.len()])); @@ -1618,6 +1631,30 @@ mod tests { assert_eq!(err, Error::VolumeExists); } + #[test] + fn heal_bucket_mode_fails_closed_on_incomplete_topology() { + let mut opts = HealOpts::default(); + assert_eq!( + resolve_heal_bucket_mode(&mut opts, &[Some(Error::ErasureWriteQuorum)]), + Err(Error::ErasureWriteQuorum) + ); + assert!(!opts.recreate); + assert!(!opts.remove); + } + + #[test] + fn heal_bucket_mode_distinguishes_deleted_and_partial_buckets() { + let mut deleted = HealOpts::default(); + resolve_heal_bucket_mode(&mut deleted, &[Some(Error::VolumeNotFound)]).unwrap(); + assert!(deleted.remove); + assert!(!deleted.recreate); + + let mut partial = HealOpts::default(); + resolve_heal_bucket_mode(&mut partial, &[None, Some(Error::VolumeNotFound)]).unwrap(); + assert!(!partial.remove); + assert!(partial.recreate); + } + #[tokio::test] async fn test_make_bucket_reduces_quorum_by_pool_participants() { let peer_sys = S3PeerSys { diff --git a/crates/ecstore/src/cluster/rpc/remote_disk.rs b/crates/ecstore/src/cluster/rpc/remote_disk.rs index 300f443d2..ffeaa2791 100644 --- a/crates/ecstore/src/cluster/rpc/remote_disk.rs +++ b/crates/ecstore/src/cluster/rpc/remote_disk.rs @@ -25,7 +25,7 @@ use crate::disk::error::{Error, Result}; use crate::disk::{ BatchReadVersionReq, BatchReadVersionResp, CheckPartsResp, DeleteOptions, DiskAPI, DiskInfo, DiskInfoOptions, DiskLocation, DiskOption, FileInfoVersions, FileReader, FileWriter, PartTransactionAction, ReadMultipleReq, ReadMultipleResp, ReadOptions, - RenameDataResp, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, batch_read_version_one_by_one, + RenameDataResp, SnapshotLeaseToken, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, batch_read_version_one_by_one, disk_store::{ DEFAULT_RUSTFS_DRIVE_ACTIVE_MONITORING, ENV_RUSTFS_DRIVE_ACTIVE_MONITORING, SKIP_IF_SUCCESS_BEFORE, get_drive_active_check_interval, get_drive_active_check_timeout, get_drive_disk_info_timeout, get_drive_list_dir_timeout, @@ -50,8 +50,9 @@ use rustfs_protos::proto_gen::node_service::{ DeleteVersionRequest, DeleteVersionsRequest, DeleteVolumeRequest, DiskInfoRequest, ListDirRequest, ListVolumesRequest, MakeVolumeRequest, MakeVolumesRequest, PreparePartTransactionRequest, ReadAllRequest, ReadMetadataRequest, ReadMultipleRequest, ReadMultipleResponse, ReadPartsRequest, ReadVersionRequest, ReadXlRequest, RenameDataRequest, - RenameFileRequest, SettlePartTransactionRequest, StatVolumeRequest, UpdateMetadataRequest, VerifyFileRequest, - WriteAllRequest, WriteMetadataRequest, node_service_client::NodeServiceClient, + RenameFileRequest, SettlePartTransactionRequest, SnapshotLeaseReleaseRequest, SnapshotLeaseRenewRequest, + SnapshotLeaseRequest, SnapshotLeaseResponse, StatVolumeRequest, UpdateMetadataRequest, VerifyFileRequest, WriteAllRequest, + WriteMetadataRequest, node_service_client::NodeServiceClient, }; use serde::{Serialize, de::DeserializeOwned}; use std::{ @@ -100,6 +101,18 @@ const LOG_COMPONENT_ECSTORE: &str = "ecstore"; const LOG_SUBSYSTEM_REMOTE_DISK: &str = "remote_disk"; const EVENT_REMOTE_DISK_HEALTH: &str = "remote_disk_health"; const EVENT_REMOTE_DISK_RPC: &str = "remote_disk_rpc"; +const SNAPSHOT_LEASE_PROTOCOL_VERSION: u32 = 1; +pub const REMOTE_SNAPSHOT_LEASE_TTL: Duration = Duration::from_secs(60); + +fn snapshot_lease_token_from_response(response: SnapshotLeaseResponse) -> Result { + if !response.success { + return Err(response.error.unwrap_or_default().into()); + } + if response.protocol_version != SNAPSHOT_LEASE_PROTOCOL_VERSION { + return Err(Error::other("remote snapshot lease protocol is incompatible")); + } + SnapshotLeaseToken::from_slice(&response.token) +} /// Bind a mutating disk RPC to its canonical body: the digest lands in the request metadata, and /// the signing interceptor folds it (plus a replay-protected nonce) into the v2 signature scope @@ -1784,6 +1797,81 @@ impl DiskAPI for RemoteDisk { .await } + async fn acquire_snapshot_lease(&self, volume: &str, path: &str) -> Result { + self.execute_with_timeout( + || async { + let mut client = self + .get_client() + .await + .map_err(|err| Error::other(format!("can not get client, err: {err}")))?; + let mut request = Request::new(SnapshotLeaseRequest { + disk: self.endpoint.to_string(), + volume: volume.to_string(), + path: path.to_string(), + ttl_ms: u64::try_from(REMOTE_SNAPSHOT_LEASE_TTL.as_millis()) + .map_err(|_| Error::other("snapshot lease TTL cannot be represented"))?, + }); + let canonical_body = rustfs_protos::canonical_snapshot_lease_request_body(request.get_ref()); + attach_mutation_body_digest(&mut request, canonical_body, "acquire_snapshot_lease")?; + let response = client.acquire_snapshot_lease(request).await?.into_inner(); + snapshot_lease_token_from_response(response) + }, + get_max_timeout_duration(), + ) + .await + } + + async fn renew_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> Result { + self.execute_with_timeout( + || async { + let mut client = self + .get_client() + .await + .map_err(|err| Error::other(format!("can not get client, err: {err}")))?; + let mut request = Request::new(SnapshotLeaseRenewRequest { + disk: self.endpoint.to_string(), + volume: volume.to_string(), + path: path.to_string(), + token: token.as_bytes().to_vec().into(), + ttl_ms: u64::try_from(REMOTE_SNAPSHOT_LEASE_TTL.as_millis()) + .map_err(|_| Error::other("snapshot lease TTL cannot be represented"))?, + }); + let canonical_body = rustfs_protos::canonical_snapshot_lease_renew_request_body(request.get_ref()); + attach_mutation_body_digest(&mut request, canonical_body, "renew_snapshot_lease")?; + let response = client.renew_snapshot_lease(request).await?.into_inner(); + snapshot_lease_token_from_response(response) + }, + get_max_timeout_duration(), + ) + .await + } + + async fn release_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> Result<()> { + self.execute_with_timeout( + || async { + let mut client = self + .get_client() + .await + .map_err(|err| Error::other(format!("can not get client, err: {err}")))?; + let mut request = Request::new(SnapshotLeaseReleaseRequest { + disk: self.endpoint.to_string(), + volume: volume.to_string(), + path: path.to_string(), + token: token.as_bytes().to_vec().into(), + }); + let canonical_body = rustfs_protos::canonical_snapshot_lease_release_request_body(request.get_ref()); + attach_mutation_body_digest(&mut request, canonical_body, "release_snapshot_lease")?; + let response = client.release_snapshot_lease(request).await?.into_inner(); + if !response.success { + return Err(response.error.unwrap_or_default().into()); + } + Ok(()) + }, + get_max_timeout_duration(), + ) + .await + } + #[tracing::instrument(level = "trace", skip_all)] async fn write_metadata(&self, _org_volume: &str, volume: &str, path: &str, fi: FileInfo) -> Result<()> { trace!( @@ -2930,6 +3018,34 @@ mod tests { static INIT: Once = Once::new(); + #[test] + fn snapshot_lease_response_requires_current_protocol_and_valid_token() { + let token = SnapshotLeaseToken::new(); + let response = SnapshotLeaseResponse { + success: true, + token: token.as_bytes().to_vec().into(), + protocol_version: SNAPSHOT_LEASE_PROTOCOL_VERSION, + error: None, + }; + assert_eq!(snapshot_lease_token_from_response(response).unwrap(), token); + + let incompatible = SnapshotLeaseResponse { + success: true, + token: token.as_bytes().to_vec().into(), + protocol_version: SNAPSHOT_LEASE_PROTOCOL_VERSION + 1, + error: None, + }; + assert!(snapshot_lease_token_from_response(incompatible).is_err()); + + let malformed = SnapshotLeaseResponse { + success: true, + token: Bytes::from_static(b"not-a-uuid"), + protocol_version: SNAPSHOT_LEASE_PROTOCOL_VERSION, + error: None, + }; + assert!(snapshot_lease_token_from_response(malformed).is_err()); + } + #[test] fn list_volumes_decode_rejects_a_malformed_entry() { let valid = serde_json::to_string(&VolumeInfo { diff --git a/crates/ecstore/src/disk/disk_store.rs b/crates/ecstore/src/disk/disk_store.rs index 940638853..1183d7c18 100644 --- a/crates/ecstore/src/disk/disk_store.rs +++ b/crates/ecstore/src/disk/disk_store.rs @@ -1365,6 +1365,14 @@ impl DiskAPI for LocalDiskWrapper { .await } + async fn renew_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> Result { + self.track_disk_health( + || async { self.disk.renew_snapshot_lease(volume, path, token).await }, + get_max_timeout_duration(), + ) + .await + } + async fn delete_data_dir(&self, volume: &str, path: &str, opts: DeleteOptions) -> Result { self.track_disk_health( || async { self.disk.delete_data_dir(volume, path, opts).await }, diff --git a/crates/ecstore/src/disk/local.rs b/crates/ecstore/src/disk/local.rs index 991a2248b..c45a9ec32 100644 --- a/crates/ecstore/src/disk/local.rs +++ b/crates/ecstore/src/disk/local.rs @@ -7908,6 +7908,23 @@ impl DiskAPI for LocalDisk { } } + async fn renew_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> Result { + let key = SnapshotLeaseKey { + volume: volume.to_string(), + path: path.to_string(), + }; + let mut registry = self.snapshot_leases.lock().await; + let Some(entry) = registry.entries.get_mut(&key) else { + return Err(DiskError::FileNotFound); + }; + if entry.deleting || !entry.tokens.remove(&token) { + return Err(DiskError::FileNotFound); + } + let renewed = SnapshotLeaseToken::new(); + entry.tokens.insert(renewed); + Ok(renewed) + } + async fn delete_data_dir(&self, volume: &str, path: &str, opts: DeleteOptions) -> Result { let key = SnapshotLeaseKey { volume: volume.to_string(), @@ -14957,6 +14974,13 @@ mod test { .acquire_snapshot_lease(volume, &data_dir) .await .expect("second lease should be acquired"); + let renewed = disk + .renew_snapshot_lease(volume, &data_dir, first) + .await + .expect("first lease should renew atomically"); + disk.release_snapshot_lease(volume, &data_dir, first) + .await + .expect("the superseded token should be idempotent"); let status = disk .delete_data_dir( volume, @@ -14976,9 +15000,9 @@ mod test { Bytes::from_static(b"later") ); - disk.release_snapshot_lease(volume, &data_dir, first) + disk.release_snapshot_lease(volume, &data_dir, renewed) .await - .expect("first lease release should succeed"); + .expect("renewed lease release should succeed"); assert!( disk.read_all(volume, &first_part).await.is_ok(), "one remaining lease must keep the data directory" diff --git a/crates/ecstore/src/disk/mod.rs b/crates/ecstore/src/disk/mod.rs index ee947d291..4ac894373 100644 --- a/crates/ecstore/src/disk/mod.rs +++ b/crates/ecstore/src/disk/mod.rs @@ -79,6 +79,18 @@ impl SnapshotLeaseToken { pub fn new() -> Self { Self(Uuid::new_v4()) } + + pub fn from_slice(bytes: &[u8]) -> Result { + let uuid = Uuid::from_slice(bytes).map_err(|_| Error::other("invalid snapshot lease token"))?; + if uuid.is_nil() { + return Err(Error::other("invalid snapshot lease token")); + } + Ok(Self(uuid)) + } + + pub fn as_bytes(&self) -> &[u8; 16] { + self.0.as_bytes() + } } impl Default for SnapshotLeaseToken { @@ -284,6 +296,13 @@ impl DiskAPI for Disk { } } + async fn renew_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> Result { + match self { + Disk::Local(local_disk) => local_disk.renew_snapshot_lease(volume, path, token).await, + Disk::Remote(remote_disk) => remote_disk.renew_snapshot_lease(volume, path, token).await, + } + } + async fn delete_data_dir(&self, volume: &str, path: &str, opts: DeleteOptions) -> Result { match self { Disk::Local(local_disk) => local_disk.delete_data_dir(volume, path, opts).await, @@ -694,6 +713,9 @@ pub trait DiskAPI: Debug + Send + Sync + 'static { async fn release_snapshot_lease(&self, _volume: &str, _path: &str, _token: SnapshotLeaseToken) -> Result<()> { Err(Error::other("snapshot leases are not supported by this disk")) } + async fn renew_snapshot_lease(&self, _volume: &str, _path: &str, _token: SnapshotLeaseToken) -> Result { + Err(Error::other("snapshot leases are not supported by this disk")) + } async fn delete_data_dir(&self, volume: &str, path: &str, opts: DeleteOptions) -> Result { self.delete(volume, path, opts).await?; Ok(DataDirDeleteStatus::Deleted) diff --git a/crates/ecstore/src/services/notification_sys.rs b/crates/ecstore/src/services/notification_sys.rs index 13ef3e09a..f4e82ffb9 100644 --- a/crates/ecstore/src/services/notification_sys.rs +++ b/crates/ecstore/src/services/notification_sys.rs @@ -1344,8 +1344,11 @@ async fn run_tier_config_reload_worker( } TierConfigReloadFinish::Pending => retry_attempt = 0, }, - TierConfigReloadOutcome::Terminal(_) => match sys.finish_tier_config_reload_worker(&host) { + TierConfigReloadOutcome::Terminal(err) => match sys.finish_tier_config_reload_worker(&host) { TierConfigReloadFinish::Completed => { + // This peer keeps the previous tier configuration for good, so record + // why. Dropping the error here hides the only evidence of a divergent + // node behind an outcome label that cannot be acted on. warn!( event = EVENT_NOTIFICATION_PEER_PROPAGATION, component = LOG_COMPONENT_ECSTORE, @@ -1353,6 +1356,7 @@ async fn run_tier_config_reload_worker( action = "reload_transition_tier_config", host, outcome = "terminal", + error = ?err, "tier configuration reload stopped after a terminal outcome" ); return; diff --git a/crates/ecstore/src/set_disk/metadata.rs b/crates/ecstore/src/set_disk/metadata.rs index 7eb4c57ad..e1cfea0e0 100644 --- a/crates/ecstore/src/set_disk/metadata.rs +++ b/crates/ecstore/src/set_disk/metadata.rs @@ -555,7 +555,7 @@ impl SetDisks { } } - fn file_info_quorum_hash(meta: &FileInfo) -> [u8; 32] { + pub(super) fn file_info_quorum_hash(meta: &FileInfo) -> [u8; 32] { let mut hasher = Sha256::new(); Self::update_file_info_quorum_hash(&mut hasher, meta); let digest = hasher.finalize(); diff --git a/crates/ecstore/src/set_disk/ops/heal.rs b/crates/ecstore/src/set_disk/ops/heal.rs index 672af4c66..db9f27c7e 100644 --- a/crates/ecstore/src/set_disk/ops/heal.rs +++ b/crates/ecstore/src/set_disk/ops/heal.rs @@ -92,6 +92,69 @@ struct PartFailureSummary { bitrot_failure: bool, } +#[derive(Clone)] +struct RecoverableMetaCandidate { + identity: [u8; 32], + file_info: FileInfo, + data_count: usize, + local_payload: bool, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum DanglingDeleteSafety { + UnsafeToDelete, + NoRecoverableCandidate, +} + +#[cfg(test)] +struct DanglingCheckPartsFailure { + key: DanglingCheckPartsFailureKey, +} + +#[cfg(test)] +type DanglingCheckPartsFailureKey = (String, String, usize); + +#[cfg(test)] +type DanglingCheckPartsFailures = HashMap; + +#[cfg(test)] +fn dangling_check_parts_failures() -> &'static std::sync::Mutex { + static FAILURES: std::sync::OnceLock> = std::sync::OnceLock::new(); + FAILURES.get_or_init(|| std::sync::Mutex::new(HashMap::new())) +} + +#[cfg(test)] +impl DanglingCheckPartsFailure { + fn install(bucket: &str, object: &str, disk_index: usize, error: DiskError) -> Self { + let key = (bucket.to_string(), object.to_string(), disk_index); + let previous = dangling_check_parts_failures() + .lock() + .expect("dangling check-parts failure registry should not poison") + .insert(key.clone(), error); + assert!(previous.is_none(), "dangling check-parts failure already installed"); + Self { key } + } +} + +#[cfg(test)] +impl Drop for DanglingCheckPartsFailure { + fn drop(&mut self) { + dangling_check_parts_failures() + .lock() + .expect("dangling check-parts failure registry should not poison") + .remove(&self.key); + } +} + +#[cfg(test)] +fn injected_dangling_check_parts_error(bucket: &str, object: &str, disk_index: usize) -> Option { + dangling_check_parts_failures() + .lock() + .expect("dangling check-parts failure registry should not poison") + .get(&(bucket.to_string(), object.to_string(), disk_index)) + .cloned() +} + fn first_unhealthy_part_summary( data_errs_by_part: &HashMap>, parts: &[ObjectPartInfo], @@ -125,39 +188,17 @@ impl SetDisks { version_id: &str, opts: &HealOpts, ) -> disk::error::Result<(HealResultItem, Option)> { - // `allow_meta_regen` is true on the first pass: a version whose data shards - // physically survive (>= data_blocks) but whose xl.meta fell below - // read-quorum is RESCUED (missing xl.meta regenerated) rather than - // dangling-deleted. The re-drive after a rescue sets it false so the - // regeneration can happen at most once (no unbounded recursion). - Box::pin(self.heal_object_with_regen(bucket, object, version_id, opts, true)).await - } - - /// Best-effort orphan-data-dir reclaim for an object that is healthy on this - /// set. Wraps [`Self::reclaim_orphan_data_dirs`] with the shared logging so - /// both `heal_object` exits — the already-healthy early return and the - /// post-heal tail — reclaim identically. Never fails the heal: delete errors - /// are logged and swallowed. Callers must gate this on `!opts.dry_run`. - async fn reclaim_orphan_data_dirs_best_effort(&self, bucket: &str, object: &str) { - match self.reclaim_orphan_data_dirs(bucket, object).await { - Ok(removed) if removed > 0 => { - info!(bucket, object, removed, "heal_object: reclaimed orphaned data directories"); - } - Ok(_) => {} - Err(e) => { - warn!(bucket, object, error = %e, "heal_object: orphan data-dir reclaim failed"); - } - } + Box::pin(self.heal_object_with_explicit_version_regen(bucket, object, version_id, opts, true)).await } #[allow(clippy::too_many_lines)] - async fn heal_object_with_regen( + async fn heal_object_with_explicit_version_regen( &self, bucket: &str, object: &str, version_id: &str, opts: &HealOpts, - allow_meta_regen: bool, + allow_explicit_version_regen: bool, ) -> disk::error::Result<(HealResultItem, Option)> { info!(?opts, "Starting heal_object"); @@ -354,22 +395,6 @@ impl SetDisks { } } - // DATA-SAFETY GUARD (backlog#920, decision 1): before any - // dangling delete, if the version's DATA shards physically - // survive on >= data_blocks disks it is RECONSTRUCTABLE. - // Regenerate the missing xl.meta from a surviving valid - // FileInfo and re-drive the heal instead of destroying a - // recoverable version. Torn writes (< data_blocks data - // shards) fall through to the existing dangling behavior. - if cannot_heal - && allow_meta_regen - && self - .try_regenerate_recoverable_meta(bucket, object, &parts_metadata, &errs, &disks) - .await? - { - return Box::pin(self.heal_object_with_regen(bucket, object, version_id, opts, false)).await; - } - if cannot_heal { let total_disks = parts_metadata.len(); let healthy_count = total_disks.saturating_sub(disks_to_heal_count); @@ -422,6 +447,20 @@ impl SetDisks { ); } + // `disks_with_all_parts` normalizes conflicting entries + // in `parts_metadata` to defaults. Re-read only before + // destructive cleanup so the guard sees every original + // identity. + let (delete_guard_metadata, delete_guard_errs) = + Self::read_all_fileinfo(&disks, "", bucket, object, version_id, true, true, false).await?; + if self + .dangling_delete_safety(bucket, object, &delete_guard_metadata, &delete_guard_errs, &disks) + .await? + == DanglingDeleteSafety::UnsafeToDelete + { + return Ok((result, Some(cannot_heal_err))); + } + // Allow for dangling deletes, on versions that have DataDir missing etc. // this would end up restoring the correct readable versions. return match self @@ -837,17 +876,25 @@ impl SetDisks { } } Err(err) => { - // DATA-SAFETY GUARD (backlog#920, decision 1): meta quorum failed, - // but the version's DATA may still physically survive on enough - // disks (xl.meta lost on > parity disks while part files remain). - // Rescue it by regenerating the missing xl.meta and re-driving heal - // instead of dangling-deleting a reconstructable version. - if allow_meta_regen + if allow_explicit_version_regen + && !version_id.is_empty() && self - .try_regenerate_recoverable_meta(bucket, object, &parts_metadata, &errs, &disks) + .try_regenerate_explicit_version_meta(bucket, object, version_id, &parts_metadata, &errs, &disks) .await? { - return Box::pin(self.heal_object_with_regen(bucket, object, version_id, opts, false)).await; + return Box::pin(self.heal_object_with_explicit_version_regen(bucket, object, version_id, opts, false)).await; + } + + if self + .dangling_delete_safety(bucket, object, &parts_metadata, &errs, &disks) + .await? + == DanglingDeleteSafety::UnsafeToDelete + { + return Ok(( + self.default_heal_result(FileInfo::default(), &errs, bucket, object, version_id) + .await, + Some(err), + )); } let data_errs_by_part = HashMap::new(); @@ -883,129 +930,226 @@ impl SetDisks { } } - /// backlog#920 (decision 1): rescue a version that meta-quorum logic would - /// otherwise dangling-DELETE, when its DATA is still reconstructable. - /// - /// Returns `Ok(true)` if the version was rescued (missing xl.meta regenerated - /// on at least one disk, so a re-driven heal can reconstruct it), `Ok(false)` - /// to fall through to the existing dangling-delete behavior. - /// - /// Recoverability is computed by physically probing part files across ALL - /// disks in the set with `check_parts` — including disks whose xl.meta is - /// absent (a lost xl.meta does not lose the sibling `part.*` data). If at - /// least `data_blocks` disks hold every part of a surviving valid FileInfo, - /// the object is EC-reconstructable, so we regenerate that FileInfo's xl.meta - /// on every disk whose metadata is absent (via `write_metadata`, which merges - /// into any existing xl.meta). Delete markers, remote/transitioned versions, - /// and genuine torn writes (< `data_blocks` surviving data shards) are NOT - /// rescued — they keep the current dangling-delete-after-grace behavior, so no - /// regression on those paths. - async fn try_regenerate_recoverable_meta( + async fn try_regenerate_explicit_version_meta( + &self, + bucket: &str, + object: &str, + version_id: &str, + parts_metadata: &[FileInfo], + errs: &[Option], + disks: &[Option], + ) -> disk::error::Result { + let Ok(version_id) = Uuid::parse_str(version_id) else { + return Ok(false); + }; + let candidates = parts_metadata + .iter() + .zip(errs.iter()) + .filter_map(|(file_info, err)| { + (err.is_none() + && file_info_is_valid_for_metadata(file_info) + && file_info.version_id == Some(version_id) + && file_info.has_valid_erasure_geometry() + && !file_info.deleted + && !file_info.is_remote() + && file_info.data_dir.is_some() + && !file_info.parts.is_empty() + && file_info.erasure.data_blocks > 0 + && file_info + .erasure + .data_blocks + .checked_add(file_info.erasure.parity_blocks) + .is_some_and(|shards| shards == disks.len())) + .then_some(file_info) + }) + .collect::>(); + let Some(candidate) = candidates.first().copied() else { + return Ok(false); + }; + let identity = Self::file_info_quorum_hash(candidate); + if candidates + .iter() + .any(|file_info| Self::file_info_quorum_hash(file_info) != identity) + { + return Ok(false); + } + + let mut available = 0usize; + for disk in disks { + let Some(disk) = disk else { + return Ok(false); + }; + match disk.check_parts(bucket, object, candidate).await { + Ok(response) + if !response.results.is_empty() && response.results.iter().all(|result| *result == CHECK_PART_SUCCESS) => + { + available += 1; + } + Ok(_) + | Err( + DiskError::FileNotFound + | DiskError::FileVersionNotFound + | DiskError::PathNotFound + | DiskError::VolumeNotFound, + ) => {} + Err(_) => return Ok(false), + } + } + if available < candidate.erasure.data_blocks { + return Ok(false); + } + + let mut wrote = 0usize; + for (index, disk) in disks.iter().enumerate() { + let Some(disk) = disk else { + return Ok(false); + }; + let metadata_absent = matches!( + errs.get(index).and_then(Option::as_ref), + Some(DiskError::FileNotFound | DiskError::FileVersionNotFound) + ); + if !metadata_absent { + continue; + } + let Some(&shard_index) = candidate.erasure.distribution.get(index) else { + return Ok(false); + }; + let mut regenerated = candidate.clone(); + regenerated.fresh = false; + regenerated.erasure.index = shard_index; + match disk.write_metadata("", bucket, object, regenerated).await { + Ok(()) => wrote += 1, + Err(error) => { + warn!( + bucket, + object, + disk_index = index, + error = %error, + "failed to regenerate recoverable xl.meta" + ); + } + } + } + Ok(wrote > 0) + } + + /// Best-effort orphan-data-dir reclaim for an object that is healthy on this + /// set. Wraps [`Self::reclaim_orphan_data_dirs`] with the shared logging so + /// both `heal_object` exits — the already-healthy early return and the + /// post-heal tail — reclaim identically. Never fails the heal: delete errors + /// are logged and swallowed. Callers must gate this on `!opts.dry_run`. + async fn reclaim_orphan_data_dirs_best_effort(&self, bucket: &str, object: &str) { + match self.reclaim_orphan_data_dirs(bucket, object).await { + Ok(removed) if removed > 0 => { + info!(bucket, object, removed, "heal_object: reclaimed orphaned data directories"); + } + Ok(_) => {} + Err(e) => { + warn!(bucket, object, error = %e, "heal_object: orphan data-dir reclaim failed"); + } + } + } + + /// Prevent dangling cleanup when surviving state cannot prove that deletion + /// is safe. Part presence proves only recoverability, never commit: the write + /// path can durably rename data before xl.meta is committed. + async fn dangling_delete_safety( &self, bucket: &str, object: &str, parts_metadata: &[FileInfo], errs: &[Option], disks: &[Option], - ) -> disk::error::Result { - // A surviving valid, non-deleted, non-remote data FileInfo to rebuild from. - let Some(surviving) = parts_metadata + ) -> disk::error::Result { + if disks.iter().any(Option::is_none) + || errs.iter().flatten().any(|err| { + !matches!( + err, + DiskError::FileNotFound + | DiskError::FileVersionNotFound + | DiskError::PathNotFound + | DiskError::VolumeNotFound + ) + }) + { + return Ok(DanglingDeleteSafety::UnsafeToDelete); + } + + let mut candidates = Vec::::with_capacity(parts_metadata.len()); + for (fi, err) in parts_metadata.iter().zip(errs.iter()) { + if err.is_some() || !file_info_is_valid_for_metadata(fi) { + continue; + } + + let identity = Self::file_info_quorum_hash(fi); + if !candidates.iter().any(|candidate| candidate.identity == identity) { + let local_payload = fi.has_valid_erasure_geometry() + && !fi.deleted + && !fi.is_remote() + && fi.data_dir.is_some() + && !fi.parts.is_empty() + && fi.erasure.data_blocks > 0 + && fi + .erasure + .data_blocks + .checked_add(fi.erasure.parity_blocks) + .is_some_and(|shards| shards == disks.len()); + candidates.push(RecoverableMetaCandidate { + identity, + file_info: fi.clone(), + data_count: 0, + local_payload, + }); + } + } + + if candidates .iter() - .find(|fi| fi.has_valid_erasure_geometry() && !fi.deleted && !fi.is_remote()) - .cloned() - else { - return Ok(false); - }; - - // Without a data_dir + parts there is no data to prove recoverable. - if surviving.data_dir.is_none() || surviving.parts.is_empty() { - return Ok(false); - } - let data_blocks = surviving.erasure.data_blocks; - if data_blocks == 0 { - return Ok(false); + .any(|candidate| candidate.file_info.deleted || candidate.file_info.is_remote()) + || candidates.len() > 1 + { + return Ok(DanglingDeleteSafety::UnsafeToDelete); } - // Physically probe part presence on EVERY online disk using the surviving - // FileInfo's data_dir/parts. `check_parts` stats `object//part.N` - // directly, so it counts disks that still hold the data even if their - // xl.meta was deleted. - let mut available = 0usize; - for disk in disks.iter().flatten() { - if let Ok(resp) = disk.check_parts(bucket, object, &surviving).await - && !resp.results.is_empty() - && resp.results.iter().all(|r| *r == CHECK_PART_SUCCESS) - { - available += 1; - } - } + for candidate in candidates.iter_mut().filter(|candidate| candidate.local_payload) { + for (disk_index, disk) in disks.iter().enumerate() { + let Some(disk) = disk else { + return Ok(DanglingDeleteSafety::UnsafeToDelete); + }; + #[cfg(test)] + let check_result = match injected_dangling_check_parts_error(bucket, object, disk_index) { + Some(error) => Err(error), + None => disk.check_parts(bucket, object, &candidate.file_info).await, + }; + #[cfg(not(test))] + let check_result = disk.check_parts(bucket, object, &candidate.file_info).await; - // Torn write: fewer than data_blocks surviving data shards is genuinely - // unrecoverable — preserve the current dangling behavior (no resurrection). - if available < data_blocks { - debug!( - bucket, - object, - available, - data_blocks, - "heal_object: version not reconstructable (torn write), keeping dangling behavior" - ); - return Ok(false); - } - - // Reconstructable: regenerate the surviving xl.meta on every disk whose - // metadata is absent so the version regains read-quorum. Each disk gets its - // OWN shard index: the disk at physical position `index` holds shard - // `distribution[index]` (mirrors `shuffle_disks` + the write path's - // `erasure.index = shuffled_pos + 1`). Copying the surviving disk's index - // verbatim would write an inconsistent xl.meta that the re-heal then treats - // as corrupt. - let distribution = &surviving.erasure.distribution; - let mut wrote = 0usize; - for (index, disk) in disks.iter().enumerate() { - let Some(disk) = disk else { continue }; - let meta_absent = matches!( - errs.get(index).and_then(Option::as_ref), - Some(DiskError::FileNotFound | DiskError::FileVersionNotFound) - ) || !parts_metadata.get(index).map(FileInfo::is_valid).unwrap_or(false); - if !meta_absent { - continue; - } - // Without a known shard index for this position we cannot write a - // consistent xl.meta; leave it for the normal heal to reconstruct. - let Some(&shard_index) = distribution.get(index) else { - continue; - }; - let mut regen = surviving.clone(); - regen.fresh = false; // merge into any existing xl.meta on the disk - regen.erasure.index = shard_index; - match disk.write_metadata("", bucket, object, regen).await { - Ok(()) => wrote += 1, - Err(e) => { - warn!( - bucket, - object, - disk_index = index, - error = %e, - "heal_object: failed to regenerate recoverable xl.meta on disk" - ); + match check_result { + Ok(resp) if !resp.results.is_empty() && resp.results.iter().all(|result| *result == CHECK_PART_SUCCESS) => { + candidate.data_count += 1; + } + Ok(_) => {} + Err( + DiskError::FileNotFound + | DiskError::FileVersionNotFound + | DiskError::PathNotFound + | DiskError::VolumeNotFound, + ) => {} + Err(_) => return Ok(DanglingDeleteSafety::UnsafeToDelete), } } } - if wrote == 0 { - return Ok(false); - } - - info!( - bucket, - object, - available, - data_blocks, - regenerated_meta_disks = wrote, - "heal_object: rescued reconstructable sub-quorum version by regenerating xl.meta" - ); - Ok(true) + Ok( + if candidates + .iter() + .any(|candidate| candidate.local_payload && candidate.data_count >= candidate.file_info.erasure.data_blocks) + { + DanglingDeleteSafety::UnsafeToDelete + } else { + DanglingDeleteSafety::NoRecoverableCandidate + }, + ) } pub(in crate::set_disk) async fn heal_object_dir_locked( @@ -1393,7 +1537,7 @@ impl crate::storage_api_contracts::heal::HealOperations for SetDisks { #[cfg(test)] mod heal_result_report_tests { - use super::SetDisks; + use super::{DanglingCheckPartsFailure, DanglingDeleteSafety, SetDisks}; use super::{HEAL_RENAME_INCOMPLETE, HealRenameFailureScope}; use crate::disk::endpoint::Endpoint; use crate::disk::error::DiskError; @@ -1405,10 +1549,12 @@ mod heal_result_report_tests { use crate::storage_api_contracts::object::{ObjectIO as _, ObjectOperations as _}; use crate::{config::storageclass, store::init_format::save_format_file}; use rustfs_common::heal_channel::{DriveState, HealOpts, HealScanMode}; - use rustfs_filemeta::{BLOCK_SIZE_V2, FileInfo}; + use rustfs_filemeta::{BLOCK_SIZE_V2, FileInfo, ObjectPartInfo, TRANSITION_COMPLETE}; use std::sync::Arc; use tempfile::TempDir; + use time::OffsetDateTime; use tokio::sync::RwLock; + use uuid::Uuid; async fn real_disk() -> (TempDir, Endpoint, DiskStore) { let dir = tempfile::tempdir().expect("tempdir should be created"); @@ -1446,6 +1592,68 @@ mod heal_result_report_tests { .await } + fn meta_regen_test_fileinfo(object: &str, data_dir: Uuid, mod_time: i64, disk_index: usize) -> FileInfo { + let mut fi = FileInfo::new(object, 2, 2); + fi.data_dir = Some(data_dir); + fi.mod_time = Some(OffsetDateTime::from_unix_timestamp(mod_time).expect("test timestamp should parse")); + fi.size = 1; + fi.parts = vec![ObjectPartInfo { + number: 1, + size: 1, + actual_size: 1, + ..Default::default() + }]; + fi.erasure.index = fi.erasure.distribution[disk_index]; + fi + } + + async fn meta_regen_test_set( + bucket: &str, + object: &str, + data_dirs: &[(Uuid, usize)], + ) -> (Vec, Arc, Vec>) { + let mut temp_dirs = Vec::new(); + let mut endpoints = Vec::new(); + let mut disks = Vec::new(); + for disk_index in 0..4 { + let (temp_dir, endpoint, disk) = real_disk().await; + disk.make_volume(bucket).await.expect("test bucket should be created"); + for (data_dir, shard_count) in data_dirs { + if disk_index >= *shard_count { + continue; + } + let part_dir = temp_dir.path().join(bucket).join(object).join(data_dir.to_string()); + tokio::fs::create_dir_all(&part_dir) + .await + .expect("test data directory should be created"); + tokio::fs::write(part_dir.join("part.1"), [1u8; 2]) + .await + .expect("test data shard should be written"); + } + temp_dirs.push(temp_dir); + endpoints.push(endpoint); + disks.push(Some(disk)); + } + + let set = set_disks_with(disks.clone(), endpoints, 2).await; + (temp_dirs, set, disks) + } + + async fn seed_meta_regen_test_metadata( + disks: &[Option], + disk_index: usize, + bucket: &str, + object: &str, + file_info: &FileInfo, + ) { + disks[disk_index] + .as_ref() + .expect("metadata test disk should be online") + .write_metadata("", bucket, object, file_info.clone()) + .await + .expect("test metadata should be written"); + } + async fn formatted_single_disk_no_parity_set() -> (TempDir, Arc) { let format = FormatV3::new(1, 1); let dir = tempfile::tempdir().expect("tempdir should be created"); @@ -1753,6 +1961,312 @@ mod heal_result_report_tests { assert_eq!(result.before.drives[3].state, DriveState::Ok.to_string()); } + #[tokio::test] + async fn dangling_delete_guard_preserves_conflicting_identities_without_writing_metadata() { + let bucket = "bucket-delete-guard-conflict"; + let object = "object.bin"; + let old_data_dir = Uuid::parse_str("99999999-9999-9999-9999-999999999999").expect("old data dir should parse"); + let new_data_dir = Uuid::parse_str("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa").expect("new data dir should parse"); + let (_temp_dirs, set, disks) = meta_regen_test_set(bucket, object, &[(old_data_dir, 4), (new_data_dir, 2)]).await; + let version_id = Uuid::parse_str("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb").expect("version id should parse"); + let mut metadata = vec![ + meta_regen_test_fileinfo(object, old_data_dir, 9, 0), + meta_regen_test_fileinfo(object, new_data_dir, 10, 1), + FileInfo::default(), + FileInfo::default(), + ]; + metadata[0].version_id = Some(version_id); + metadata[1].version_id = Some(version_id); + assert_eq!( + metadata[0].version_id, metadata[1].version_id, + "the conflicting candidates must share one version id" + ); + seed_meta_regen_test_metadata(&disks, 0, bucket, object, &metadata[0]).await; + seed_meta_regen_test_metadata(&disks, 1, bucket, object, &metadata[1]).await; + let errs = vec![None, None, Some(DiskError::FileNotFound), Some(DiskError::FileNotFound)]; + + assert!( + set.dangling_delete_safety(bucket, object, &metadata, &errs, &disks) + .await + .expect("conflicting identities should be classified") + == DanglingDeleteSafety::UnsafeToDelete + ); + let reversed = vec![ + metadata[1].clone(), + metadata[0].clone(), + FileInfo::default(), + FileInfo::default(), + ]; + assert!( + set.dangling_delete_safety(bucket, object, &reversed, &errs, &disks) + .await + .expect("reversed identities should be classified") + == DanglingDeleteSafety::UnsafeToDelete + ); + let version_id = version_id.to_string(); + assert!( + !set.try_regenerate_explicit_version_meta(bucket, object, &version_id, &metadata, &errs, &disks) + .await + .expect("conflicting explicit-version candidates should be rejected"), + "an explicit version must not select between conflicting metadata identities" + ); + for disk_index in [2, 3] { + assert!( + matches!( + disks[disk_index] + .as_ref() + .expect("test disk should be online") + .read_version("", bucket, object, "", &ReadOptions::default()) + .await, + Err(DiskError::FileNotFound) + ), + "the delete guard must not manufacture metadata on missing disks" + ); + } + let old = disks[0] + .as_ref() + .expect("first test disk should be online") + .read_version("", bucket, object, "", &ReadOptions::default()) + .await + .expect("old metadata should remain readable"); + let new = disks[1] + .as_ref() + .expect("second test disk should be online") + .read_version("", bucket, object, "", &ReadOptions::default()) + .await + .expect("new metadata should remain readable"); + assert_eq!(old.data_dir, Some(old_data_dir)); + assert_eq!(new.data_dir, Some(new_data_dir)); + } + + #[tokio::test] + async fn heal_meta_quorum_failure_preserves_reconstructable_uncommitted_candidate() { + let bucket = "bucket-delete-guard-reconstructable"; + let object = "object.bin"; + let data_dir = Uuid::parse_str("33333333-3333-3333-3333-333333333333").expect("data dir should parse"); + let (_temp_dirs, set, disks) = meta_regen_test_set(bucket, object, &[(data_dir, 2)]).await; + let metadata = [ + meta_regen_test_fileinfo(object, data_dir, 3, 0), + FileInfo::default(), + FileInfo::default(), + FileInfo::default(), + ]; + seed_meta_regen_test_metadata(&disks, 0, bucket, object, &metadata[0]).await; + let (observed_metadata, observed_errs) = SetDisks::read_all_fileinfo(&disks, "", bucket, object, "", true, true, false) + .await + .expect("test metadata should be readable across the set"); + assert_eq!( + set.dangling_delete_safety(bucket, object, &observed_metadata, &observed_errs, &disks) + .await + .expect("observed reconstructable candidate should be classified"), + DanglingDeleteSafety::UnsafeToDelete + ); + + let (_, err) = set + .heal_object( + bucket, + object, + "", + &HealOpts { + no_lock: true, + ..Default::default() + }, + ) + .await + .expect("unsafe dangling state should be reported without deletion"); + assert_eq!(err, Some(DiskError::FileNotFound)); + let surviving = disks[0] + .as_ref() + .expect("first test disk should be online") + .read_version("", bucket, object, "", &ReadOptions::default()) + .await + .expect("the only metadata copy must be preserved"); + assert_eq!(surviving.data_dir, Some(data_dir)); + assert!( + matches!( + disks[1] + .as_ref() + .expect("second test disk should be online") + .read_version("", bucket, object, "", &ReadOptions::default()) + .await, + Err(DiskError::FileNotFound) + ), + "the delete guard must not propagate metadata" + ); + } + + #[tokio::test] + async fn heal_meta_quorum_failure_preserves_candidate_when_required_shard_disk_is_offline() { + let bucket = "bucket-delete-guard-offline"; + let object = "object.bin"; + let data_dir = Uuid::parse_str("44444444-4444-4444-4444-444444444444").expect("data dir should parse"); + let (temp_dirs, set, disks) = meta_regen_test_set(bucket, object, &[(data_dir, 2)]).await; + let metadata = meta_regen_test_fileinfo(object, data_dir, 4, 0); + seed_meta_regen_test_metadata(&disks, 0, bucket, object, &metadata).await; + set.disks.write().await[1] = None; + + let (_, err) = set + .heal_object( + bucket, + object, + "", + &HealOpts { + no_lock: true, + ..Default::default() + }, + ) + .await + .expect("offline shard state should be reported without deletion"); + assert_eq!(err, Some(DiskError::FileNotFound)); + let surviving = disks[0] + .as_ref() + .expect("first test disk should be online") + .read_version("", bucket, object, "", &ReadOptions::default()) + .await + .expect("offline uncertainty must preserve the surviving metadata"); + assert_eq!(surviving.data_dir, Some(data_dir)); + assert!( + temp_dirs[0] + .path() + .join(bucket) + .join(object) + .join(data_dir.to_string()) + .join("part.1") + .is_file(), + "offline uncertainty must preserve the last online shard" + ); + } + + #[tokio::test] + async fn heal_meta_quorum_failure_preserves_candidate_when_part_probe_times_out() { + let bucket = "bucket-delete-guard-timeout"; + let object = "object.bin"; + let data_dir = Uuid::parse_str("55555555-5555-5555-5555-555555555555").expect("data dir should parse"); + let (temp_dirs, set, disks) = meta_regen_test_set(bucket, object, &[(data_dir, 2)]).await; + let metadata = meta_regen_test_fileinfo(object, data_dir, 5, 0); + seed_meta_regen_test_metadata(&disks, 0, bucket, object, &metadata).await; + let _failure = DanglingCheckPartsFailure::install(bucket, object, 1, DiskError::Timeout); + + let (_, err) = set + .heal_object( + bucket, + object, + "", + &HealOpts { + no_lock: true, + ..Default::default() + }, + ) + .await + .expect("part probe timeout should be reported without deletion"); + assert_eq!(err, Some(DiskError::FileNotFound)); + let surviving = disks[0] + .as_ref() + .expect("first test disk should be online") + .read_version("", bucket, object, "", &ReadOptions::default()) + .await + .expect("probe uncertainty must preserve the surviving metadata"); + assert_eq!(surviving.data_dir, Some(data_dir)); + assert!( + temp_dirs[0] + .path() + .join(bucket) + .join(object) + .join(data_dir.to_string()) + .join("part.1") + .is_file(), + "probe uncertainty must preserve the last confirmed shard" + ); + } + + #[tokio::test] + async fn dangling_delete_guard_ignores_set_incompatible_geometry() { + let bucket = "bucket-delete-guard-short-geometry"; + let object = "object.bin"; + let data_dir = Uuid::parse_str("abababab-abab-abab-abab-abababababab").expect("data dir should parse"); + let (_temp_dirs, set, disks) = meta_regen_test_set(bucket, object, &[(data_dir, 1)]).await; + let mut candidate = FileInfo::new(object, 1, 0); + candidate.data_dir = Some(data_dir); + candidate.mod_time = Some(OffsetDateTime::from_unix_timestamp(18).expect("timestamp should parse")); + candidate.size = 1; + candidate.parts = vec![ObjectPartInfo { + number: 1, + size: 1, + actual_size: 1, + ..Default::default() + }]; + candidate.erasure.index = candidate.erasure.distribution[0]; + seed_meta_regen_test_metadata(&disks, 0, bucket, object, &candidate).await; + let metadata = vec![candidate, FileInfo::default(), FileInfo::default(), FileInfo::default()]; + let errs = vec![ + None, + Some(DiskError::FileNotFound), + Some(DiskError::FileNotFound), + Some(DiskError::FileNotFound), + ]; + + assert!( + set.dangling_delete_safety(bucket, object, &metadata, &errs, &disks) + .await + .expect("set-incompatible geometry should be classified") + == DanglingDeleteSafety::NoRecoverableCandidate + ); + } + + #[tokio::test] + async fn dangling_delete_guard_preserves_delete_marker_and_remote_metadata() { + let bucket = "bucket-delete-guard-nonlocal"; + let object = "object.bin"; + let (_temp_dirs, set, disks) = meta_regen_test_set(bucket, object, &[]).await; + let marker = FileInfo { + name: object.to_string(), + version_id: Some(Uuid::parse_str("eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee").expect("version id should parse")), + deleted: true, + mod_time: Some(OffsetDateTime::from_unix_timestamp(14).expect("marker timestamp should parse")), + ..Default::default() + }; + let remote_dir = Uuid::parse_str("89898989-8989-8989-8989-898989898989").expect("remote data dir should parse"); + let mut remote = meta_regen_test_fileinfo(object, remote_dir, 15, 1); + remote.transition_status = TRANSITION_COMPLETE.to_string(); + remote.transition_tier = "WARM".to_string(); + remote.transitioned_objname = "remote/object.bin".to_string(); + + for metadata in [marker, remote] { + let candidates = vec![metadata, FileInfo::default(), FileInfo::default(), FileInfo::default()]; + let errs = vec![ + None, + Some(DiskError::FileNotFound), + Some(DiskError::FileNotFound), + Some(DiskError::FileNotFound), + ]; + assert_eq!( + set.dangling_delete_safety(bucket, object, &candidates, &errs, &disks) + .await + .expect("non-local metadata should be classified"), + DanglingDeleteSafety::UnsafeToDelete + ); + } + } + + #[tokio::test] + async fn dangling_delete_guard_preserves_metadata_read_uncertainty() { + let bucket = "bucket-delete-guard-read-error"; + let object = "object.bin"; + let (_temp_dirs, set, disks) = meta_regen_test_set(bucket, object, &[]).await; + let metadata = vec![FileInfo::default(); disks.len()]; + + for read_error in [DiskError::Timeout, DiskError::DiskAccessDenied, DiskError::DiskNotFound] { + let mut errs = vec![Some(DiskError::FileNotFound); disks.len()]; + errs[0] = Some(read_error); + assert_eq!( + set.dangling_delete_safety(bucket, object, &metadata, &errs, &disks) + .await + .expect("metadata read uncertainty should be classified"), + DanglingDeleteSafety::UnsafeToDelete + ); + } + } + #[tokio::test] async fn heal_no_parity_bitrot_reports_unrecoverable_integrity_failure() { let (dir, set) = formatted_single_disk_no_parity_set().await; diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index 9817bf485..079a64842 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -2315,23 +2315,50 @@ async fn pause_transition_commit(bucket: &str, object: &str, pause: TransitionCo fn persisted_transition_version( remote_version: &str, +) -> std::io::Result<(Option, rustfs_filemeta::TransitionVersionState)> { + persisted_transition_version_with_gate(remote_version, remote_version_state_writer_enabled()) +} + +fn remote_version_state_writer_enabled() -> bool { + remote_version_state_writer_enabled_for( + rustfs_utils::get_env_bool( + rustfs_config::ENV_TIER_REMOTE_VERSION_STATE_WRITE, + rustfs_config::DEFAULT_TIER_REMOTE_VERSION_STATE_WRITE, + ), + rustfs_utils::get_env_bool( + rustfs_config::ENV_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED, + rustfs_config::DEFAULT_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED, + ), + ) +} + +fn remote_version_state_writer_enabled_for(requested: bool, fleet_confirmed: bool) -> bool { + requested && fleet_confirmed +} + +fn persisted_transition_version_with_gate( + remote_version: &str, + remote_version_state_writer_enabled: bool, ) -> std::io::Result<(Option, rustfs_filemeta::TransitionVersionState)> { if remote_version.is_empty() { return Ok((None, rustfs_filemeta::TransitionVersionState::KnownDisabled)); } - let version_id = Uuid::parse_str(remote_version).map_err(|_| { - std::io::Error::new( - std::io::ErrorKind::Unsupported, - "opaque remote tier versions require the cluster capability gate", - ) - })?; - if version_id.is_nil() { - return Err(std::io::Error::new( + + match Uuid::parse_str(remote_version) { + Ok(version_id) if version_id.is_nil() => Err(std::io::Error::new( std::io::ErrorKind::InvalidData, "remote tier returned a nil object version ID", - )); + )), + Ok(_) => Ok((Some(remote_version.to_string()), rustfs_filemeta::TransitionVersionState::Exact)), + Err(_) if !remote_version_state_writer_enabled => Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "opaque remote tier versions require the operator-attested fleet gate", + )), + Err(_) if remote_version == "null" => { + Ok((Some(remote_version.to_string()), rustfs_filemeta::TransitionVersionState::SuspendedNull)) + } + Err(_) => Ok((Some(remote_version.to_string()), rustfs_filemeta::TransitionVersionState::Exact)), } - Ok((Some(remote_version.to_string()), rustfs_filemeta::TransitionVersionState::Exact)) } #[cfg(test)] @@ -2569,7 +2596,10 @@ mod transition_upload_completion_tests { #[cfg(test)] mod transition_version_id_tests { - use super::{TransitionUploadCandidate, persisted_transition_version}; + use super::{ + TransitionUploadCandidate, persisted_transition_version, persisted_transition_version_with_gate, + remote_version_state_writer_enabled_for, + }; use rustfs_filemeta::TransitionVersionState; use uuid::Uuid; @@ -2608,6 +2638,44 @@ mod transition_version_id_tests { "opaque-version-token" ); } + + #[test] + fn remote_version_state_writer_requires_request_and_fleet_confirmation() { + for (case, requested, fleet_confirmed, expected) in [ + ("old defaults", false, false, false), + ("missing fleet confirmation", true, false, false), + ("missing local opt-in", false, true, false), + ("explicitly unconfirmed fleet", true, false, false), + ("rolled-back writer", false, true, false), + ("fully upgraded fleet", true, true, true), + ] { + assert_eq!(remote_version_state_writer_enabled_for(requested, fleet_confirmed), expected, "{case}"); + } + } + + #[test] + fn fleet_gate_enables_null_and_opaque_remote_version_states() { + for (remote_version, expected) in [ + ("null", (Some("null".to_string()), TransitionVersionState::SuspendedNull)), + ( + "opaque-version-token", + (Some("opaque-version-token".to_string()), TransitionVersionState::Exact), + ), + ] { + assert!( + persisted_transition_version_with_gate(remote_version, false).is_err(), + "missing fleet confirmation must reject {remote_version:?}" + ); + assert_eq!( + persisted_transition_version_with_gate(remote_version, true).expect("fleet-confirmed state must be persisted"), + expected + ); + } + assert_eq!( + persisted_transition_version_with_gate("", true).expect("empty remote version identifies an unversioned tier"), + (None, TransitionVersionState::KnownDisabled) + ); + } } impl SetDisks { diff --git a/crates/ecstore/src/store/bucket.rs b/crates/ecstore/src/store/bucket.rs index fcdc67a44..4d5d4840b 100644 --- a/crates/ecstore/src/store/bucket.rs +++ b/crates/ecstore/src/store/bucket.rs @@ -87,7 +87,7 @@ fn bucket_deleted_marker_volume(bucket: &str) -> String { format!("{RUSTFS_META_BUCKET}/{}", bucket_deleted_marker_prefix(bucket)) } -async fn await_bucket_namespace_operation( +pub(crate) async fn await_bucket_namespace_operation( guard: Option<&rustfs_lock::NamespaceLockGuard>, bucket: &str, operation: &'static str, diff --git a/crates/ecstore/src/store/mod.rs b/crates/ecstore/src/store/mod.rs index bad794f01..4c9ea3039 100644 --- a/crates/ecstore/src/store/mod.rs +++ b/crates/ecstore/src/store/mod.rs @@ -141,6 +141,7 @@ fn should_enqueue_transition_immediately(oi: &ObjectInfo) -> bool { const MAX_UPLOADS_LIST: usize = 10000; mod bucket; +pub(crate) use bucket::await_bucket_namespace_operation; mod heal; mod heal_walk; pub use heal_walk::HealWalkVersion; diff --git a/crates/protocols/src/swift/account.rs b/crates/protocols/src/swift/account.rs index cf3aa538c..8abaa849f 100644 --- a/crates/protocols/src/swift/account.rs +++ b/crates/protocols/src/swift/account.rs @@ -14,11 +14,11 @@ //! Swift account operations and validation +use super::metadata_update::{ACCOUNT_META_TAG_PREFIX, MetadataUpdate}; use super::storage_api::account::{BucketOperations, MakeBucketOptions}; use super::{SwiftError, SwiftResult}; -use super::{get_swift_bucket_metadata, resolve_swift_object_store_handle, update_swift_bucket_tagging, validate_metadata}; +use super::{get_swift_bucket_metadata, resolve_swift_object_store_handle, update_swift_bucket_tagging}; use rustfs_credentials::Credentials; -use s3s::dto::{Tag, Tagging}; use sha2::{Digest, Sha256}; use std::collections::HashMap; @@ -131,9 +131,8 @@ pub async fn get_account_metadata(account: &str, _credentials: &Option, + update: &MetadataUpdate, credentials: &Option, ) -> SwiftResult<()> { let Some(credentials) = credentials.as_ref() else { @@ -171,8 +176,15 @@ pub async fn update_account_metadata( // These tags are persisted into the bucket metadata file, which every // later config write rewrites in full — so unbounded metadata inflates - // the cost of unrelated writes for the life of the account. - validate_metadata(metadata)?; + // the cost of unrelated writes for the life of the account. The item + // count is capped against the merged result, inside the rewrite. + update.validate()?; + + // An update that names no item changes nothing, so there is no reason to + // bring the account's metadata bucket into existence for it. + if update.is_empty() { + return Ok(()); + } let bucket_name = get_account_metadata_bucket_name(account); @@ -190,32 +202,10 @@ pub async fn update_account_metadata( .map_err(|e| SwiftError::InternalServerError(format!("Failed to create account metadata bucket: {}", e)))?; } - // Rewrite the persisted tags: replace swift-account-meta-* tags with the - // new metadata while preserving other tags. An empty result clears the - // tagging config. - update_swift_bucket_tagging(bucket_name, |current| { - let mut tagging = current.cloned().unwrap_or_else(|| Tagging { tag_set: vec![] }); - - tagging.tag_set.retain(|tag| { - if let Some(key) = &tag.key { - !key.starts_with("swift-account-meta-") - } else { - true - } - }); - - for (key, value) in metadata { - tagging.tag_set.push(Tag { - key: Some(format!("swift-account-meta-{}", key)), - value: Some(value.clone()), - }); - } - - tagging - }) - .await?; - - Ok(()) + // Merge into the persisted tags: only the swift-account-meta-* items this + // update names change, and non-Swift tags are left alone. An empty result + // clears the tagging config. + update_swift_bucket_tagging(bucket_name, |current| update.apply_to_tags(current, ACCOUNT_META_TAG_PREFIX)).await } /// Get TempURL key for account diff --git a/crates/protocols/src/swift/container.rs b/crates/protocols/src/swift/container.rs index fb0ad807d..495664525 100644 --- a/crates/protocols/src/swift/container.rs +++ b/crates/protocols/src/swift/container.rs @@ -17,15 +17,13 @@ //! This module implements Swift container CRUD operations and container-bucket translation. use super::account::validate_account_access; +use super::metadata_update::{CONTAINER_META_TAG_PREFIX, MetadataUpdate}; use super::storage_api::container::{ BucketInfo, BucketOperations, BucketOptions, DeleteBucketOptions, ListOperations as _, MakeBucketOptions, }; use super::types::Container; use super::{SwiftError, SwiftResult}; -use super::{ - get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, update_swift_bucket_tagging, - validate_metadata, -}; +use super::{get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, update_swift_bucket_tagging}; use rustfs_credentials::Credentials; use s3s::dto::{Tag, Tagging}; use sha2::{Digest, Sha256}; @@ -62,30 +60,6 @@ fn sanitize_storage_error(operation: &str, error: E) -> Sw SwiftError::InternalServerError(format!("{} operation failed", operation)) } -/// Convert Swift container metadata to S3 tags -/// -/// Swift container metadata uses X-Container-Meta-* headers. -/// We store these as S3 tags with "swift-meta-" prefix to distinguish from regular bucket tags. -/// -/// Example: X-Container-Meta-Color: Blue → S3 Tag: swift-meta-color=Blue -fn swift_metadata_to_s3_tags(metadata: &std::collections::HashMap) -> Option { - let mut tags = Vec::new(); - - for (key, value) in metadata { - // Store with "swift-meta-" prefix to namespace container metadata - tags.push(Tag { - key: Some(format!("swift-meta-{}", key.to_lowercase())), - value: Some(value.clone()), - }); - } - - if tags.is_empty() { - None - } else { - Some(Tagging { tag_set: tags }) - } -} - /// Convert S3 tags back to Swift container metadata /// /// Extracts only tags with "swift-meta-" prefix, which represent Swift container metadata. @@ -94,11 +68,9 @@ fn s3_tags_to_swift_metadata(tagging: &Tagging) -> std::collections::HashMap, + update: MetadataUpdate, ) -> SwiftResult<()> { // Validate account access and extract project_id let project_id = validate_account_access(account, credentials)?; @@ -488,8 +463,9 @@ pub async fn update_container_metadata( // These tags are persisted into the bucket metadata file, which every // later config write rewrites in full — so unbounded metadata inflates - // the cost of unrelated writes for the life of the container. - validate_metadata(&metadata)?; + // the cost of unrelated writes for the life of the container. The item + // count is capped against the merged result, inside the rewrite. + update.validate()?; // Create mapper with default config (tenant prefixing enabled) let mapper = ContainerMapper::default(); @@ -502,7 +478,9 @@ pub async fn update_container_metadata( return Err(SwiftError::InternalServerError("Storage layer not initialized".to_string())); }; - // Verify container exists + // Verify container exists. Checked before the empty-update shortcut below, + // so a POST to a container that does not exist still answers 404 whether + // or not it carried metadata. store .get_bucket_info(&bucket_name, &BucketOptions::default()) .await @@ -514,32 +492,19 @@ pub async fn update_container_metadata( } })?; - // Rewrite the persisted tags: replace swift-meta-* tags with the new - // metadata while preserving non-Swift tags. An empty result clears the - // tagging config. - update_swift_bucket_tagging(bucket_name, |current| { - let mut tagging = current.cloned().unwrap_or_else(|| Tagging { tag_set: vec![] }); + // An update that names no item leaves stored metadata alone, so skip the + // persisted write and the peer reload it triggers rather than rewriting + // the config to its current value. The handler reaches here on every + // container POST, including ACL-only and versioning-only ones. + if update.is_empty() { + return Ok(()); + } - tagging.tag_set.retain(|tag| { - if let Some(key) = &tag.key { - !key.starts_with("swift-meta-") - } else { - true // Keep tags with no key (shouldn't happen, but be safe) - } - }); - - if let Some(mut new_tagging) = swift_metadata_to_s3_tags(&metadata) { - tagging.tag_set.append(&mut new_tagging.tag_set); - } - // If metadata.is_empty() and swift_metadata_to_s3_tags returns None, - // we've already removed swift-meta-* tags above, so only non-Swift - // tags remain - - tagging - }) - .await?; - - Ok(()) + // Merge into the persisted tags: only the swift-meta-* items this update + // names change, so the container's other metadata — and the ACL and + // versioning tags sharing this tag set — survive. An empty result clears + // the tagging config. + update_swift_bucket_tagging(bucket_name, |current| update.apply_to_tags(current, CONTAINER_META_TAG_PREFIX)).await } /// Delete a container @@ -814,7 +779,7 @@ pub async fn enable_versioning( value: Some(archive_container.to_string()), // Store Swift container name, not S3 bucket name }); - tagging + Ok(tagging) }) .await?; @@ -871,7 +836,7 @@ pub async fn disable_versioning(account: &str, container: &str, credentials: &Cr .tag_set .retain(|tag| tag.key.as_deref() != Some("swift-versions-location")); - tagging + Ok(tagging) }) .await?; @@ -1026,7 +991,7 @@ pub async fn set_container_acl( }); } - tagging + Ok(tagging) }) .await?; @@ -1374,64 +1339,6 @@ mod tests { assert!(first_char.is_ascii_lowercase() || first_char.is_ascii_digit()); } - #[test] - fn test_swift_metadata_to_s3_tags() { - let mut metadata = std::collections::HashMap::new(); - metadata.insert("color".to_string(), "blue".to_string()); - metadata.insert("description".to_string(), "test container".to_string()); - - let tagging = swift_metadata_to_s3_tags(&metadata).unwrap(); - assert_eq!(tagging.tag_set.len(), 2); - - // Verify tags have swift-meta- prefix - let color_tag = tagging - .tag_set - .iter() - .find(|t| t.key.as_deref() == Some("swift-meta-color")) - .expect("color tag not found"); - assert_eq!(color_tag.value.as_deref(), Some("blue")); - - let desc_tag = tagging - .tag_set - .iter() - .find(|t| t.key.as_deref() == Some("swift-meta-description")) - .expect("description tag not found"); - assert_eq!(desc_tag.value.as_deref(), Some("test container")); - } - - #[test] - fn test_swift_metadata_to_s3_tags_empty() { - let metadata = std::collections::HashMap::new(); - let tagging = swift_metadata_to_s3_tags(&metadata); - assert!(tagging.is_none()); - } - - #[test] - fn test_swift_metadata_to_s3_tags_case_normalization() { - let mut metadata = std::collections::HashMap::new(); - metadata.insert("Color".to_string(), "Red".to_string()); - metadata.insert("PRIORITY".to_string(), "High".to_string()); - - let tagging = swift_metadata_to_s3_tags(&metadata).unwrap(); - - // Keys should be lowercased - assert!(tagging.tag_set.iter().any(|t| t.key.as_deref() == Some("swift-meta-color"))); - assert!( - tagging - .tag_set - .iter() - .any(|t| t.key.as_deref() == Some("swift-meta-priority")) - ); - - // Values should be preserved as-is - let color_tag = tagging - .tag_set - .iter() - .find(|t| t.key.as_deref() == Some("swift-meta-color")) - .unwrap(); - assert_eq!(color_tag.value.as_deref(), Some("Red")); - } - #[test] fn test_s3_tags_to_swift_metadata() { let tagging = Tagging { @@ -1487,91 +1394,80 @@ mod tests { #[test] fn test_metadata_roundtrip() { - // Test that we can convert metadata -> tags -> metadata without loss - let mut original_metadata = std::collections::HashMap::new(); - original_metadata.insert("color".to_string(), "blue".to_string()); - original_metadata.insert("owner".to_string(), "alice".to_string()); - original_metadata.insert("priority".to_string(), "high".to_string()); + // What a POST writes must be what a HEAD reads back: run the items + // through the tag-merge write path and the tag-parse read path. + let update = MetadataUpdate::default() + .set("color", "blue") + .set("owner", "alice") + .set("priority", "high"); - let tagging = swift_metadata_to_s3_tags(&original_metadata).unwrap(); - let recovered_metadata = s3_tags_to_swift_metadata(&tagging); + let tagging = update + .apply_to_tags(None, CONTAINER_META_TAG_PREFIX) + .expect("merge should be accepted"); + let recovered = s3_tags_to_swift_metadata(&tagging); - assert_eq!(recovered_metadata.len(), original_metadata.len()); - for (key, value) in &original_metadata { - assert_eq!( - recovered_metadata.get(&key.to_lowercase()), - Some(value), - "Metadata key {} not preserved in roundtrip", - key - ); - } + assert_eq!(recovered.len(), 3); + assert_eq!(recovered.get("color").map(String::as_str), Some("blue")); + assert_eq!(recovered.get("owner").map(String::as_str), Some("alice")); + assert_eq!(recovered.get("priority").map(String::as_str), Some("high")); } #[test] fn test_tag_preservation_merge_with_existing() { - // Test merging Swift metadata with existing non-Swift tags - let mut existing_tagging = Tagging { + // A container metadata POST shares its tag set with the container ACL + // and versioning tags, and with whatever S3 tags the bucket carries. + // Only the swift-meta-* items the POST names may change. + let existing = Tagging { tag_set: vec![ Tag { key: Some("swift-meta-color".to_string()), value: Some("blue".to_string()), }, + Tag { + key: Some("swift-acl-read".to_string()), + value: Some(".r:*".to_string()), + }, + Tag { + key: Some("swift-versions-location".to_string()), + value: Some("archive".to_string()), + }, Tag { key: Some("env".to_string()), value: Some("production".to_string()), }, - Tag { - key: Some("team".to_string()), - value: Some("backend".to_string()), - }, ], }; - // Remove old swift-meta-* tags - existing_tagging.tag_set.retain(|tag| { - if let Some(key) = &tag.key { - !key.starts_with("swift-meta-") - } else { - true - } - }); + let merged = MetadataUpdate::default() + .set("description", "test") + .apply_to_tags(Some(&existing), CONTAINER_META_TAG_PREFIX) + .expect("merge should be accepted"); + let tag = |key: &str| { + merged + .tag_set + .iter() + .find(|t| t.key.as_deref() == Some(key)) + .and_then(|t| t.value.as_deref()) + }; - // Add new Swift metadata - let mut new_metadata = std::collections::HashMap::new(); - new_metadata.insert("description".to_string(), "test".to_string()); - let mut new_tagging = swift_metadata_to_s3_tags(&new_metadata).unwrap(); - - // Merge - existing_tagging.tag_set.append(&mut new_tagging.tag_set); - - // Verify: should have env, team, and new swift-meta-description - assert_eq!(existing_tagging.tag_set.len(), 3); - - let has_env = existing_tagging.tag_set.iter().any(|t| t.key.as_deref() == Some("env")); - let has_team = existing_tagging.tag_set.iter().any(|t| t.key.as_deref() == Some("team")); - let has_description = existing_tagging - .tag_set - .iter() - .any(|t| t.key.as_deref() == Some("swift-meta-description")); - - assert!(has_env, "env tag should be preserved"); - assert!(has_team, "team tag should be preserved"); - assert!(has_description, "swift-meta-description should be added"); + assert_eq!(tag("swift-meta-description"), Some("test"), "the new item should be added"); + assert_eq!(tag("swift-meta-color"), Some("blue"), "an unnamed item should be preserved"); + assert_eq!(tag("swift-acl-read"), Some(".r:*"), "the ACL tag should be preserved"); + assert_eq!(tag("swift-versions-location"), Some("archive"), "the versioning tag should be preserved"); + assert_eq!(tag("env"), Some("production"), "non-Swift tags should be preserved"); + assert_eq!(merged.tag_set.len(), 5); } #[test] fn test_tag_preservation_remove_only_swift() { - // Test that clearing Swift metadata preserves non-Swift tags - let mut existing_tagging = Tagging { + // Removing the last container metadata item leaves the other tags — + // and so must not clear the tagging config. + let existing = Tagging { tag_set: vec![ Tag { key: Some("swift-meta-color".to_string()), value: Some("blue".to_string()), }, - Tag { - key: Some("swift-meta-owner".to_string()), - value: Some("alice".to_string()), - }, Tag { key: Some("env".to_string()), value: Some("production".to_string()), @@ -1583,37 +1479,28 @@ mod tests { ], }; - // Remove swift-meta-* tags (simulating empty metadata update) - existing_tagging.tag_set.retain(|tag| { - if let Some(key) = &tag.key { - !key.starts_with("swift-meta-") - } else { - true - } - }); + let merged = MetadataUpdate::default() + .remove("color") + .apply_to_tags(Some(&existing), CONTAINER_META_TAG_PREFIX) + .expect("merge should be accepted"); - // Verify: should only have env and cost-center - assert_eq!(existing_tagging.tag_set.len(), 2); - - let has_env = existing_tagging.tag_set.iter().any(|t| t.key.as_deref() == Some("env")); - let has_cost_center = existing_tagging - .tag_set - .iter() - .any(|t| t.key.as_deref() == Some("cost-center")); - let has_swift_meta = existing_tagging - .tag_set - .iter() - .any(|t| t.key.as_ref().is_some_and(|k| k.starts_with("swift-meta-"))); - - assert!(has_env, "env tag should be preserved"); - assert!(has_cost_center, "cost-center tag should be preserved"); - assert!(!has_swift_meta, "all swift-meta-* tags should be removed"); + assert_eq!(merged.tag_set.len(), 2); + assert!(merged.tag_set.iter().any(|t| t.key.as_deref() == Some("env"))); + assert!(merged.tag_set.iter().any(|t| t.key.as_deref() == Some("cost-center"))); + assert!( + !merged + .tag_set + .iter() + .any(|t| t.key.as_ref().is_some_and(|k| k.starts_with(CONTAINER_META_TAG_PREFIX))), + "the removed item should be gone" + ); } #[test] fn test_tag_preservation_empty_after_swift_removal() { - // Test that if only Swift tags exist, clearing them results in empty tagging - let mut existing_tagging = Tagging { + // Removing every item when nothing else is tagged empties the tag set, + // which is how the caller knows to clear the tagging config. + let existing = Tagging { tag_set: vec![ Tag { key: Some("swift-meta-color".to_string()), @@ -1626,38 +1513,13 @@ mod tests { ], }; - // Remove swift-meta-* tags - existing_tagging.tag_set.retain(|tag| { - if let Some(key) = &tag.key { - !key.starts_with("swift-meta-") - } else { - true - } - }); + let merged = MetadataUpdate::default() + .remove("color") + .remove("owner") + .apply_to_tags(Some(&existing), CONTAINER_META_TAG_PREFIX) + .expect("merge should be accepted"); - // Verify: should be empty - assert!( - existing_tagging.tag_set.is_empty(), - "tagging should be empty after removing all swift-meta-* tags" - ); - } - - #[test] - fn test_tag_preservation_no_existing_tags() { - // Test adding Swift metadata when no tags exist - let existing_tagging = Tagging { tag_set: vec![] }; - - let mut new_metadata = std::collections::HashMap::new(); - new_metadata.insert("color".to_string(), "blue".to_string()); - let mut new_tagging = swift_metadata_to_s3_tags(&new_metadata).unwrap(); - - let mut merged = existing_tagging.clone(); - merged.tag_set.append(&mut new_tagging.tag_set); - - // Verify: should have only the new Swift tag - assert_eq!(merged.tag_set.len(), 1); - assert_eq!(merged.tag_set[0].key.as_deref(), Some("swift-meta-color")); - assert_eq!(merged.tag_set[0].value.as_deref(), Some("blue")); + assert!(merged.tag_set.is_empty(), "tagging should be empty after removing every swift-meta-* tag"); } // Object Versioning Tests diff --git a/crates/protocols/src/swift/handler.rs b/crates/protocols/src/swift/handler.rs index 9961cddf2..0d9cdc455 100644 --- a/crates/protocols/src/swift/handler.rs +++ b/crates/protocols/src/swift/handler.rs @@ -20,6 +20,7 @@ use super::container; use super::dlo; +use super::metadata_update::MetadataUpdate; use super::object; use super::slo; use super::tempurl; @@ -321,32 +322,15 @@ async fn handle_authenticated_request( Err(SwiftError::NotImplemented("Swift Account HEAD operation not yet implemented".to_string())) } Method::POST => { - // Account metadata update - extract headers - let mut metadata = std::collections::HashMap::new(); + // Account metadata update. Additive, per Swift: the request + // names the items to set (X-Account-Meta-*) and the items to + // drop (X-Remove-Account-Meta-*, or an empty value), and + // everything it does not name keeps its stored value. The + // TempURL signing key lives here, so a replacing POST would + // invalidate every outstanding signature for the account. + let update = MetadataUpdate::from_account_headers(&headers); - // Extract X-Account-Meta-* headers - for (key, value) in &headers { - let key_str = key.as_str(); - if let Some(meta_key) = key_str.strip_prefix("x-account-meta-") { - // Strip "x-account-meta-" - if let Ok(value_str) = value.to_str() { - metadata.insert(meta_key.to_string(), value_str.to_string()); - } - } - } - - // Special handling for TempURL key headers - // X-Account-Meta-Temp-URL-Key or X-Account-Meta-Temp-Url-Key - if let Some(tempurl_key) = headers - .get("x-account-meta-temp-url-key") - .or_else(|| headers.get("x-account-meta-temp-Url-key")) - && let Ok(key_str) = tempurl_key.to_str() - { - metadata.insert("temp-url-key".to_string(), key_str.to_string()); - } - - // Update account metadata - super::account::update_account_metadata(&account, &metadata, &credentials_opt).await?; + super::account::update_account_metadata(&account, &update, &credentials_opt).await?; let trans_id = generate_trans_id(); Response::builder() @@ -581,17 +565,15 @@ async fn handle_authenticated_request( container::set_container_acl(&account, &container, new_read, new_write, &credentials).await?; } - // Update container metadata - now we have access to request headers - let mut metadata = std::collections::HashMap::new(); - for (name, value) in headers.iter() { - if let Some(meta_key) = name.as_str().strip_prefix("x-container-meta-") - && let Ok(value_str) = value.to_str() - { - metadata.insert(meta_key.to_string(), value_str.to_string()); - } - } + // Update container metadata. Additive, per Swift: items the + // request does not name keep their stored value, and removal + // is explicit (X-Remove-Container-Meta-*, or an empty value). + // Still called when the request named no item — an ACL-only + // or versioning-only POST — because this is what reports 404 + // for a container that does not exist. + let update = MetadataUpdate::from_container_headers(&headers); - container::update_container_metadata(&account, &container, &credentials, metadata).await?; + container::update_container_metadata(&account, &container, &credentials, update).await?; let trans_id = generate_trans_id(); Response::builder() diff --git a/crates/protocols/src/swift/metadata_update.rs b/crates/protocols/src/swift/metadata_update.rs new file mode 100644 index 000000000..ee7ca2857 --- /dev/null +++ b/crates/protocols/src/swift/metadata_update.rs @@ -0,0 +1,410 @@ +// Copyright 2024 RustFS Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Account and container metadata updates +//! +//! Swift account and container POSTs are *additive*: an item the request does +//! not mention keeps its stored value, and removal is explicit — either +//! `X-Remove-{Account,Container}-Meta-{name}`, or the item sent with an empty +//! value. Object POST is the one that replaces the whole set; `swift::object` +//! handles that separately and must keep doing so. +//! +//! Getting this wrong is not a cosmetic divergence. Account metadata holds the +//! TempURL signing key, so a POST that set an unrelated item while dropping +//! the rest would invalidate every outstanding TempURL and FormPost signature +//! for the account. +//! +//! This module turns a request's headers into the items to write and the items +//! to drop, and applies that to the bucket tag set the metadata is persisted +//! in. + +use super::{MAX_METADATA_COUNT, MAX_METADATA_VALUE_SIZE, SwiftError, SwiftResult}; +use axum::http::HeaderMap; +use s3s::dto::{Tag, Tagging}; +use std::collections::{BTreeMap, BTreeSet}; + +/// Request header prefix carrying an account metadata item. +const ACCOUNT_META_HEADER_PREFIX: &str = "x-account-meta-"; +/// Request header prefix removing an account metadata item. +const ACCOUNT_META_REMOVE_HEADER_PREFIX: &str = "x-remove-account-meta-"; +/// Request header prefix carrying a container metadata item. +const CONTAINER_META_HEADER_PREFIX: &str = "x-container-meta-"; +/// Request header prefix removing a container metadata item. +const CONTAINER_META_REMOVE_HEADER_PREFIX: &str = "x-remove-container-meta-"; + +/// Bucket-tag namespace holding account metadata items. +pub(crate) const ACCOUNT_META_TAG_PREFIX: &str = "swift-account-meta-"; +/// Bucket-tag namespace holding container metadata items. +/// +/// Deliberately narrower than the `swift-` tags around it: the container ACL +/// (`swift-acl-*`) and versioning (`swift-versions-location`) tags share this +/// tag set and must survive a metadata POST. +pub(crate) const CONTAINER_META_TAG_PREFIX: &str = "swift-meta-"; + +/// The metadata changes carried by one account or container POST. +/// +/// Item names are held lowercased. Swift metadata names are case-insensitive, +/// HTTP header names arrive lowercased anyway, and a removal has to match the +/// name a previous POST stored — so normalizing once here is what makes +/// `X-Remove-Container-Meta-Color` find a stored `color`. +/// +/// Ordered rather than hashed so the persisted tag set — and therefore the +/// serialized XML — comes out in a stable order for a given update. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct MetadataUpdate { + /// Items to write, by name. + items: BTreeMap, + /// Names to drop. + removals: BTreeSet, +} + +impl MetadataUpdate { + /// Write `name` = `value`. + pub fn set(mut self, name: &str, value: &str) -> Self { + let name = name.to_lowercase(); + self.removals.remove(&name); + self.items.insert(name, value.to_string()); + self + } + + /// Drop `name`, if it is stored. + pub fn remove(mut self, name: &str) -> Self { + let name = name.to_lowercase(); + self.items.remove(&name); + self.removals.insert(name); + self + } + + /// Parse the metadata headers of an account POST. + pub(crate) fn from_account_headers(headers: &HeaderMap) -> Self { + Self::from_headers(headers, ACCOUNT_META_HEADER_PREFIX, ACCOUNT_META_REMOVE_HEADER_PREFIX) + } + + /// Parse the metadata headers of a container POST. + pub(crate) fn from_container_headers(headers: &HeaderMap) -> Self { + Self::from_headers(headers, CONTAINER_META_HEADER_PREFIX, CONTAINER_META_REMOVE_HEADER_PREFIX) + } + + /// Parse metadata headers under `item_prefix`, and removals under + /// `remove_prefix`. Both prefixes are lowercase, matching how `http` + /// normalizes header names. + /// + /// An item sent with an empty value is a removal — the deletion path + /// Swift clients use when they do not send a dedicated remove header. + /// A name carried by both header forms is removed: the explicit removal + /// wins, as it does in Swift, where a remove header is rewritten into an + /// empty-valued item header. + fn from_headers(headers: &HeaderMap, item_prefix: &str, remove_prefix: &str) -> Self { + let mut update = Self::default(); + + for (name, value) in headers { + let Some(item) = name.as_str().strip_prefix(item_prefix) else { + continue; + }; + // A value that is not valid UTF-8 cannot be stored as a tag. + // Skipping it matches how the rest of the Swift handlers treat + // unreadable header values. + let Ok(value) = value.to_str() else { + continue; + }; + + update = if value.is_empty() { + update.remove(item) + } else { + update.set(item, value) + }; + } + + for name in headers.keys() { + if let Some(item) = name.as_str().strip_prefix(remove_prefix) { + update = update.remove(item); + } + } + + update + } + + /// Whether this update changes anything. + /// + /// An ACL-only or versioning-only container POST produces an empty update: + /// it names no metadata item, so it must leave stored metadata alone. + pub(crate) fn is_empty(&self) -> bool { + self.items.is_empty() && self.removals.is_empty() + } + + /// Reject oversized values before anything is persisted. + /// + /// The item *count* is not checked here — an additive POST has to be + /// measured against the merged result, which only [`Self::apply_to_tags`] + /// can see. + pub(crate) fn validate(&self) -> SwiftResult<()> { + for (name, value) in &self.items { + if value.len() > MAX_METADATA_VALUE_SIZE { + return Err(SwiftError::BadRequest(format!( + "Metadata value for '{}' too large: {} bytes (max: {} bytes)", + name, + value.len(), + MAX_METADATA_VALUE_SIZE + ))); + } + } + + Ok(()) + } + + /// Merge this update into the persisted tag set. + /// + /// `prefix` is the tag namespace holding the items. Tags outside it — the + /// container ACL and versioning tags, plus any S3 tags the bucket carries + /// — are untouched, and so are the namespaced items this update does not + /// name. + /// + /// The item-count cap is checked against the merged result rather than the + /// request: because POSTs are additive, a client could otherwise walk past + /// it one header at a time. This runs inside the bucket metadata write + /// guard, so the count it checks is the one about to be persisted. + pub(crate) fn apply_to_tags(&self, current: Option<&Tagging>, prefix: &str) -> SwiftResult { + let mut tagging = current.cloned().unwrap_or_else(|| Tagging { tag_set: vec![] }); + + tagging.tag_set.retain(|tag| match item_name(tag, prefix) { + Some(name) => !self.items.contains_key(name) && !self.removals.contains(name), + None => true, + }); + + let merged = tagging.tag_set.iter().filter(|tag| item_name(tag, prefix).is_some()).count() + self.items.len(); + if merged > MAX_METADATA_COUNT { + return Err(SwiftError::BadRequest(format!( + "Too many metadata headers: {} (max: {})", + merged, MAX_METADATA_COUNT + ))); + } + + for (name, value) in &self.items { + tagging.tag_set.push(Tag { + key: Some(format!("{}{}", prefix, name)), + value: Some(value.clone()), + }); + } + + Ok(tagging) + } +} + +/// The metadata item name a tag carries, or `None` if the tag does not belong +/// to this namespace. +fn item_name<'a>(tag: &'a Tag, prefix: &str) -> Option<&'a str> { + tag.key.as_deref()?.strip_prefix(prefix) +} + +#[cfg(test)] +mod tests { + use super::*; + use axum::http::{HeaderName, HeaderValue}; + + fn headers(pairs: &[(&str, &str)]) -> HeaderMap { + let mut map = HeaderMap::new(); + for (name, value) in pairs { + map.insert( + HeaderName::from_bytes(name.as_bytes()).expect("test header name should parse"), + HeaderValue::from_str(value).expect("test header value should parse"), + ); + } + map + } + + fn tags(pairs: &[(&str, &str)]) -> Tagging { + Tagging { + tag_set: pairs + .iter() + .map(|(key, value)| Tag { + key: Some((*key).to_string()), + value: Some((*value).to_string()), + }) + .collect(), + } + } + + fn tag_value<'a>(tagging: &'a Tagging, key: &str) -> Option<&'a str> { + tagging + .tag_set + .iter() + .find(|tag| tag.key.as_deref() == Some(key)) + .and_then(|tag| tag.value.as_deref()) + } + + #[test] + fn from_headers_collects_items_and_ignores_unrelated_headers() { + let update = MetadataUpdate::from_container_headers(&headers(&[ + ("x-container-meta-color", "blue"), + ("x-container-read", ".r:*"), + ("content-type", "text/plain"), + ])); + + assert_eq!(update, MetadataUpdate::default().set("color", "blue")); + } + + #[test] + fn from_headers_lowercases_item_names() { + // `http` normalizes header names on the way in, so the mixed case a + // client sends is already gone by the time a handler sees it; the + // lowercasing here is what makes a directly built update match. + let update = MetadataUpdate::from_container_headers(&headers(&[("X-Container-Meta-Color", "Blue")])); + + assert_eq!(update, MetadataUpdate::default().set("COLOR", "Blue")); + } + + #[test] + fn empty_value_is_a_removal() { + let update = MetadataUpdate::from_container_headers(&headers(&[("x-container-meta-color", "")])); + + assert_eq!(update, MetadataUpdate::default().remove("color")); + } + + #[test] + fn remove_header_drops_the_item() { + let update = MetadataUpdate::from_account_headers(&headers(&[("x-remove-account-meta-temp-url-key", "x")])); + + assert_eq!(update, MetadataUpdate::default().remove("temp-url-key")); + } + + #[test] + fn remove_header_wins_over_a_value_for_the_same_item() { + let update = MetadataUpdate::from_container_headers(&headers(&[ + ("x-container-meta-color", "blue"), + ("x-remove-container-meta-color", "x"), + ])); + + assert_eq!(update, MetadataUpdate::default().remove("color")); + } + + #[test] + fn a_removal_header_is_not_mistaken_for_an_item() { + // "x-remove-container-meta-color" must not also parse as the item + // "remove-container-meta-color" or similar. + let update = MetadataUpdate::from_container_headers(&headers(&[("x-remove-container-meta-color", "x")])); + + assert!(update.items.is_empty(), "a removal header must not set an item"); + } + + #[test] + fn a_request_with_no_metadata_headers_is_empty() { + assert!(MetadataUpdate::from_container_headers(&headers(&[("x-container-read", ".r:*")])).is_empty()); + assert!(MetadataUpdate::default().is_empty()); + assert!(!MetadataUpdate::default().remove("color").is_empty()); + } + + #[test] + fn apply_preserves_items_the_update_does_not_name() { + let current = tags(&[ + ("swift-meta-color", "blue"), + ("swift-meta-season", "summer"), + ("swift-acl-read", ".r:*"), + ("swift-versions-location", "archive"), + ("unrelated-s3-tag", "keep"), + ]); + + let merged = MetadataUpdate::default() + .set("mood", "calm") + .apply_to_tags(Some(¤t), CONTAINER_META_TAG_PREFIX) + .expect("merge should be accepted"); + + assert_eq!(tag_value(&merged, "swift-meta-color"), Some("blue")); + assert_eq!(tag_value(&merged, "swift-meta-season"), Some("summer")); + assert_eq!(tag_value(&merged, "swift-meta-mood"), Some("calm")); + assert_eq!(tag_value(&merged, "swift-acl-read"), Some(".r:*")); + assert_eq!(tag_value(&merged, "swift-versions-location"), Some("archive")); + assert_eq!(tag_value(&merged, "unrelated-s3-tag"), Some("keep")); + } + + #[test] + fn apply_overwrites_a_named_item_exactly_once() { + let current = tags(&[("swift-meta-color", "blue")]); + + let merged = MetadataUpdate::default() + .set("color", "red") + .apply_to_tags(Some(¤t), CONTAINER_META_TAG_PREFIX) + .expect("merge should be accepted"); + + assert_eq!(merged.tag_set.len(), 1, "overwriting must not duplicate the tag"); + assert_eq!(tag_value(&merged, "swift-meta-color"), Some("red")); + } + + #[test] + fn apply_drops_only_the_removed_item() { + let current = tags(&[ + ("swift-meta-color", "blue"), + ("swift-meta-season", "summer"), + ("swift-acl-read", ".r:*"), + ]); + + let merged = MetadataUpdate::default() + .remove("color") + .apply_to_tags(Some(¤t), CONTAINER_META_TAG_PREFIX) + .expect("merge should be accepted"); + + assert_eq!(tag_value(&merged, "swift-meta-color"), None); + assert_eq!(tag_value(&merged, "swift-meta-season"), Some("summer")); + assert_eq!(tag_value(&merged, "swift-acl-read"), Some(".r:*")); + } + + #[test] + fn apply_to_an_untagged_bucket_starts_from_nothing() { + let merged = MetadataUpdate::default() + .set("color", "blue") + .apply_to_tags(None, ACCOUNT_META_TAG_PREFIX) + .expect("merge should be accepted"); + + assert_eq!(tag_value(&merged, "swift-account-meta-color"), Some("blue")); + } + + #[test] + fn apply_caps_the_merged_item_count_not_the_request() { + let current = Tagging { + tag_set: (0..MAX_METADATA_COUNT) + .map(|i| Tag { + key: Some(format!("{}item{}", CONTAINER_META_TAG_PREFIX, i)), + value: Some("v".to_string()), + }) + .collect(), + }; + + // One more item than the container already stores: rejected, even + // though the request itself carries a single header. + let err = MetadataUpdate::default() + .set("overflow", "v") + .apply_to_tags(Some(¤t), CONTAINER_META_TAG_PREFIX) + .expect_err("exceeding the item cap must be rejected"); + assert!(matches!(err, SwiftError::BadRequest(_)), "expected BadRequest, got {err:?}"); + + // Overwriting an item already counted stays at the cap. + MetadataUpdate::default() + .set("item0", "v2") + .apply_to_tags(Some(¤t), CONTAINER_META_TAG_PREFIX) + .expect("overwriting an existing item must not trip the cap"); + } + + #[test] + fn validate_rejects_oversized_values() { + let err = MetadataUpdate::default() + .set("color", &"b".repeat(MAX_METADATA_VALUE_SIZE + 1)) + .validate() + .expect_err("an oversized value must be rejected"); + assert!(matches!(err, SwiftError::BadRequest(_)), "expected BadRequest, got {err:?}"); + + MetadataUpdate::default() + .set("color", &"b".repeat(MAX_METADATA_VALUE_SIZE)) + .validate() + .expect("a value at the limit must be accepted"); + } +} diff --git a/crates/protocols/src/swift/mod.rs b/crates/protocols/src/swift/mod.rs index 881b8fd4f..14d07559a 100644 --- a/crates/protocols/src/swift/mod.rs +++ b/crates/protocols/src/swift/mod.rs @@ -44,6 +44,7 @@ pub mod expiration; pub mod expiration_worker; pub mod formpost; pub mod handler; +pub mod metadata_update; pub mod object; pub mod quota; pub mod router; @@ -57,6 +58,7 @@ pub mod types; pub mod versioning; pub use errors::{SwiftError, SwiftResult}; +pub use metadata_update::MetadataUpdate; pub use router::{SwiftRoute, SwiftRouter}; /// Maximum number of metadata headers allowed per resource (Swift standard) @@ -71,9 +73,10 @@ pub(crate) const MAX_METADATA_VALUE_SIZE: usize = 256; /// - Total number of metadata entries doesn't exceed MAX_METADATA_COUNT /// - Individual metadata values don't exceed MAX_METADATA_VALUE_SIZE /// -/// Applies to object, container and account metadata alike: all three are -/// persisted, and container/account metadata additionally lands in the -/// bucket metadata file that every later config write rewrites in full. +/// This is the object-metadata form, where a POST replaces the whole set and +/// the request is therefore the complete result. Account and container POSTs +/// are additive, so their count has to be measured against the merged state +/// instead — see [`MetadataUpdate::apply_to_tags`]. /// /// Returns error if limits are exceeded. pub(crate) fn validate_metadata(metadata: &std::collections::HashMap) -> SwiftResult<()> { diff --git a/crates/protocols/src/swift/storage_api.rs b/crates/protocols/src/swift/storage_api.rs index 70e7b052f..8381b8c64 100644 --- a/crates/protocols/src/swift/storage_api.rs +++ b/crates/protocols/src/swift/storage_api.rs @@ -88,6 +88,10 @@ pub(crate) async fn get_swift_bucket_metadata(bucket: &str) -> SwiftStorageResul /// disk-truth reloads. Peers are then told to reload, matching what the S3 /// handlers do after a config write. /// +/// A rewrite may also refuse the update outright, for limits that can only be +/// judged against the state being merged into. That verdict is the client's +/// answer, so it is returned as-is rather than folded into a storage error. +/// /// Storage failures are logged in full and reported to the client as a /// generic error: these now carry real disk and quorum detail, which does not /// belong in a Swift response body. The one exception is an unreadable @@ -95,8 +99,12 @@ pub(crate) async fn get_swift_bucket_metadata(bucket: &str) -> SwiftStorageResul /// to act on it. pub(crate) async fn update_swift_bucket_tagging(bucket: String, rewrite: F) -> SwiftResult<()> where - F: FnOnce(Option<&Tagging>) -> Tagging + Send, + F: FnOnce(Option<&Tagging>) -> SwiftResult + Send, { + // Carries a rewrite's refusal back out past the storage error the config + // write has to fail with to abort the transaction. + let mut rejected = None; + let result = update_config_with(&bucket, BUCKET_TAGGING_CONFIG, |bm| { // Merging onto an unparseable tag set would silently drop every tag // the bucket has — including the container ACL and versioning tags — @@ -106,7 +114,14 @@ where return Err(SwiftStorageError::other(UNREADABLE_TAGGING_SENTINEL)); } - let tagging = rewrite(bm.tagging_config.as_ref()); + let tagging = match rewrite(bm.tagging_config.as_ref()) { + Ok(tagging) => tagging, + Err(err) => { + rejected = Some(err); + return Err(SwiftStorageError::other("swift: tagging rewrite rejected the update")); + } + }; + if tagging.tag_set.is_empty() { Ok(Vec::new()) } else { @@ -118,6 +133,12 @@ where }) .await; + // Nothing was written, but that is the rewrite's own decision about the + // request rather than a storage fault, so it is not logged as one. + if let Some(err) = rejected { + return Err(err); + } + if let Err(err) = result { let unreadable = err.to_string().contains(UNREADABLE_TAGGING_SENTINEL); tracing::error!( diff --git a/crates/protocols/tests/swift_metadata_persistence.rs b/crates/protocols/tests/swift_metadata_persistence.rs index 25a7e42ad..86f205a45 100644 --- a/crates/protocols/tests/swift_metadata_persistence.rs +++ b/crates/protocols/tests/swift_metadata_persistence.rs @@ -16,15 +16,20 @@ //! metadata file, not just the in-memory cache. The metadata has to survive //! the disk-truth reloads performed by peer LoadBucketMetadata notifications //! and the periodic refresh loop — and, transitively, a process restart. +//! +//! Durability is what makes the *content* of those writes matter, so this file +//! also covers Swift's additive account/container POST semantics: an item the +//! request does not name keeps its stored value, and removal is explicit. The +//! two are tested together because a reload is the only way to tell a real +//! merge from one that happened to look right in the cache. #![cfg(feature = "swift")] use std::collections::HashMap; use rustfs_credentials::Credentials; -use rustfs_protocols::swift::SwiftError; use rustfs_protocols::swift::container::{ContainerMapper, update_container_metadata}; -use rustfs_protocols::swift::{account, container}; +use rustfs_protocols::swift::{MetadataUpdate, SwiftError, account, container}; use rustfs_test_utils::TestECStoreEnv; use serde_json::json; use sha2::{Digest, Sha256}; @@ -97,6 +102,8 @@ async fn swift_metadata_writes_are_durable() { posts_survive_disk_truth_reload(&env).await; tag_writers_preserve_each_others_state(&env).await; + unrelated_account_posts_preserve_the_tempurl_key().await; + acl_only_posts_leave_container_metadata_alone(&env).await; versioning_writes_reject_missing_containers().await; } @@ -109,11 +116,14 @@ async fn posts_survive_disk_truth_reload(env: &TestECStoreEnv) { let bucket = ContainerMapper::default().swift_to_s3_bucket(swift_container, project_id); env.make_bucket(&bucket, false).await; - let mut metadata = HashMap::new(); - metadata.insert("color".to_string(), "blue".to_string()); - update_container_metadata(&swift_account, swift_container, &credentials, metadata) - .await - .expect("container metadata POST should succeed"); + update_container_metadata( + &swift_account, + swift_container, + &credentials, + MetadataUpdate::default().set("color", "blue"), + ) + .await + .expect("container metadata POST should succeed"); reload_bucket_metadata_from_disk(&bucket).await; @@ -124,22 +134,47 @@ async fn posts_survive_disk_truth_reload(env: &TestECStoreEnv) { "container metadata POST must survive a disk-truth metadata reload" ); - // A follow-up POST replaces the Swift metadata and that replacement must - // survive a reload too (the rewrite merges against disk state, so the - // previous value must actually be gone). - let mut metadata = HashMap::new(); - metadata.insert("season".to_string(), "summer".to_string()); - update_container_metadata(&swift_account, swift_container, &credentials, metadata) - .await - .expect("second container metadata POST should succeed"); + // A follow-up POST names a different item. Swift POSTs are additive, so + // the new item lands and the first one stays — and both are still there + // after a reload, which is what proves the merge ran against disk state + // rather than looking right in the cache. + update_container_metadata( + &swift_account, + swift_container, + &credentials, + MetadataUpdate::default().set("season", "summer"), + ) + .await + .expect("second container metadata POST should succeed"); reload_bucket_metadata_from_disk(&bucket).await; let container_meta = persisted_container_metadata(&bucket).await; assert_eq!(container_meta.get("season").map(String::as_str), Some("summer")); + assert_eq!( + container_meta.get("color").map(String::as_str), + Some("blue"), + "a POST that does not name an item must not delete it" + ); + + // X-Remove-Container-Meta-Color is the deletion path, and it has to be + // durable in the other direction: the removed item must not come back + // from the persisted tags on reload. + update_container_metadata(&swift_account, swift_container, &credentials, MetadataUpdate::default().remove("color")) + .await + .expect("container metadata removal should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + let container_meta = persisted_container_metadata(&bucket).await; assert!( !container_meta.contains_key("color"), - "replaced container metadata must not resurrect on reload" + "an explicitly removed item must not resurrect on reload" + ); + assert_eq!( + container_meta.get("season").map(String::as_str), + Some("summer"), + "removing one item must not disturb the others" ); // --- Container versioning POST (X-Versions-Location) --- @@ -163,11 +198,13 @@ async fn posts_survive_disk_truth_reload(env: &TestECStoreEnv) { ); // --- Account metadata POST (TempURL keys etc.) --- - let mut account_meta = HashMap::new(); - account_meta.insert("temp-url-key".to_string(), "s3cr3t".to_string()); - account::update_account_metadata(&swift_account, &account_meta, &Some(credentials.clone())) - .await - .expect("account metadata POST should succeed"); + account::update_account_metadata( + &swift_account, + &MetadataUpdate::default().set("temp-url-key", "s3cr3t"), + &Some(credentials.clone()), + ) + .await + .expect("account metadata POST should succeed"); reload_bucket_metadata_from_disk(&account_metadata_bucket_name(&swift_account)).await; @@ -196,9 +233,7 @@ async fn tag_writers_preserve_each_others_state(env: &TestECStoreEnv) { env.make_bucket(&ContainerMapper::default().swift_to_s3_bucket(archive, project_id), false) .await; - let mut metadata = HashMap::new(); - metadata.insert("color".to_string(), "blue".to_string()); - update_container_metadata(&swift_account, container, &credentials, metadata) + update_container_metadata(&swift_account, container, &credentials, MetadataUpdate::default().set("color", "blue")) .await .expect("container metadata POST should succeed"); container::enable_versioning(&swift_account, container, archive, &credentials) @@ -252,6 +287,111 @@ async fn tag_writers_preserve_each_others_state(env: &TestECStoreEnv) { assert!(!acl.read.is_empty(), "disable_versioning must not disturb the ACL"); } +/// The failure additive POSTs exist to prevent. Account metadata holds the +/// TempURL signing key, so a POST that replaced the whole set — setting a +/// quota, say — would delete that key and permanently invalidate every +/// outstanding TempURL and FormPost signature for the account. Durable +/// storage made that loss permanent instead of a cache blip, so the check +/// runs against reloaded disk state. +/// +/// Relies on the ambient store the caller's `TestECStoreEnv` published; the +/// account metadata bucket is created by the write path itself. +async fn unrelated_account_posts_preserve_the_tempurl_key() { + let project_id = "swiftmergeproj"; + let swift_account = format!("AUTH_{project_id}"); + let credentials = Some(keystone_credentials(project_id)); + let account_bucket = account_metadata_bucket_name(&swift_account); + + account::update_account_metadata(&swift_account, &MetadataUpdate::default().set("temp-url-key", "s3cr3t"), &credentials) + .await + .expect("TempURL key POST should succeed"); + + // A later POST that has nothing to do with the TempURL key. + account::update_account_metadata(&swift_account, &MetadataUpdate::default().set("quota-bytes", "100"), &credentials) + .await + .expect("quota POST should succeed"); + + reload_bucket_metadata_from_disk(&account_bucket).await; + + let loaded = account::get_account_metadata(&swift_account, &None) + .await + .expect("account metadata should load"); + assert_eq!( + loaded.get("temp-url-key").map(String::as_str), + Some("s3cr3t"), + "an unrelated account POST must not delete the TempURL signing key" + ); + assert_eq!(loaded.get("quota-bytes").map(String::as_str), Some("100")); + + // And the lookup that actually breaks when the key is lost still resolves. + assert_eq!( + account::get_tempurl_key(&swift_account, &None) + .await + .expect("TempURL key should load") + .as_deref(), + Some("s3cr3t"), + "TempURL signature validation must still find the key" + ); + + // X-Remove-Account-Meta-Quota-Bytes drops that item and nothing else. + account::update_account_metadata(&swift_account, &MetadataUpdate::default().remove("quota-bytes"), &credentials) + .await + .expect("account metadata removal should succeed"); + + reload_bucket_metadata_from_disk(&account_bucket).await; + + let loaded = account::get_account_metadata(&swift_account, &None) + .await + .expect("account metadata should load"); + assert!( + !loaded.contains_key("quota-bytes"), + "an explicitly removed item must not resurrect on reload" + ); + assert_eq!( + loaded.get("temp-url-key").map(String::as_str), + Some("s3cr3t"), + "removing one item must not disturb the TempURL key" + ); +} + +/// An ACL-only or versioning-only container POST carries no `X-Container-Meta-*` +/// header, but the handler still runs the metadata step on every POST. That +/// step must leave stored metadata alone rather than treating "named nothing" +/// as "wants everything gone". +async fn acl_only_posts_leave_container_metadata_alone(env: &TestECStoreEnv) { + let project_id = "swiftaclonlyproj"; + let swift_account = format!("AUTH_{project_id}"); + let credentials = keystone_credentials(project_id); + let container = "gallery"; + let bucket = ContainerMapper::default().swift_to_s3_bucket(container, project_id); + env.make_bucket(&bucket, false).await; + + update_container_metadata(&swift_account, container, &credentials, MetadataUpdate::default().set("color", "blue")) + .await + .expect("container metadata POST should succeed"); + + // What the handler does for `POST … -H 'X-Container-Read: .r:*'`: set the + // ACL, then run the metadata step with an update that names no item. + container::set_container_acl(&swift_account, container, Some(".r:*"), None, &credentials) + .await + .expect("ACL-only POST should succeed"); + update_container_metadata(&swift_account, container, &credentials, MetadataUpdate::default()) + .await + .expect("the metadata step of an ACL-only POST should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + assert_eq!( + persisted_container_metadata(&bucket).await.get("color").map(String::as_str), + Some("blue"), + "an ACL-only POST must not wipe X-Container-Meta-*" + ); + let acl = container::get_container_acl(&swift_account, container, &credentials) + .await + .expect("container ACL should load"); + assert!(!acl.read.is_empty(), "the ACL that POST set must be stored"); +} + /// A container that does not exist must not get metadata persisted for it: /// the metadata loader turns "nothing on disk" into a fresh default, so an /// unguarded rewrite would create an orphan metadata file and cache a @@ -270,6 +410,16 @@ async fn versioning_writes_reject_missing_containers() { "expected NotFound for a missing container, got {err:?}" ); + // Naming no item skips the persisted write, but must not skip the + // existence check that turns a POST to a missing container into a 404. + let err = update_container_metadata(&swift_account, missing, &credentials, MetadataUpdate::default()) + .await + .expect_err("a metadata POST to a missing container must fail"); + assert!( + matches!(err, SwiftError::NotFound(_)), + "expected NotFound for a missing container, got {err:?}" + ); + let bucket = ContainerMapper::default().swift_to_s3_bucket(missing, project_id); assert!( get_bucket_metadata(&bucket).await.is_err(), @@ -291,8 +441,7 @@ async fn account_metadata_write_rejects_foreign_and_anonymous_callers() { let victim_account = "AUTH_victimproject"; let attacker_credentials = keystone_credentials("attackerproject"); - let mut poisoned = HashMap::new(); - poisoned.insert("temp-url-key".to_string(), "attacker-key".to_string()); + let poisoned = MetadataUpdate::default().set("temp-url-key", "attacker-key"); let err = account::update_account_metadata(victim_account, &poisoned, &Some(attacker_credentials)) .await diff --git a/crates/protos/src/generated/proto_gen/node_service.rs b/crates/protos/src/generated/proto_gen/node_service.rs index 8bc8a386f..35c320d7a 100644 --- a/crates/protos/src/generated/proto_gen/node_service.rs +++ b/crates/protos/src/generated/proto_gen/node_service.rs @@ -484,6 +484,59 @@ pub struct DeletePathsResponse { pub error: ::core::option::Option, } #[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] +pub struct SnapshotLeaseRequest { + #[prost(string, tag = "1")] + pub disk: ::prost::alloc::string::String, + #[prost(string, tag = "2")] + pub volume: ::prost::alloc::string::String, + #[prost(string, tag = "3")] + pub path: ::prost::alloc::string::String, + #[prost(uint64, tag = "4")] + pub ttl_ms: u64, +} +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] +pub struct SnapshotLeaseRenewRequest { + #[prost(string, tag = "1")] + pub disk: ::prost::alloc::string::String, + #[prost(string, tag = "2")] + pub volume: ::prost::alloc::string::String, + #[prost(string, tag = "3")] + pub path: ::prost::alloc::string::String, + #[prost(bytes = "bytes", tag = "4")] + pub token: ::prost::bytes::Bytes, + #[prost(uint64, tag = "5")] + pub ttl_ms: u64, +} +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] +pub struct SnapshotLeaseReleaseRequest { + #[prost(string, tag = "1")] + pub disk: ::prost::alloc::string::String, + #[prost(string, tag = "2")] + pub volume: ::prost::alloc::string::String, + #[prost(string, tag = "3")] + pub path: ::prost::alloc::string::String, + #[prost(bytes = "bytes", tag = "4")] + pub token: ::prost::bytes::Bytes, +} +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] +pub struct SnapshotLeaseResponse { + #[prost(bool, tag = "1")] + pub success: bool, + #[prost(bytes = "bytes", tag = "2")] + pub token: ::prost::bytes::Bytes, + #[prost(uint32, tag = "3")] + pub protocol_version: u32, + #[prost(message, optional, tag = "4")] + pub error: ::core::option::Option, +} +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] +pub struct SnapshotLeaseMutationResponse { + #[prost(bool, tag = "1")] + pub success: bool, + #[prost(message, optional, tag = "2")] + pub error: ::core::option::Option, +} +#[derive(Clone, PartialEq, Eq, Hash, ::prost::Message)] pub struct ReadMetadataRequest { #[prost(string, tag = "1")] pub disk: ::prost::alloc::string::String, @@ -1595,6 +1648,51 @@ pub mod node_service_client { .insert(GrpcMethod::new("node_service.NodeService", "Delete")); self.inner.unary(req, path, codec).await } + pub async fn acquire_snapshot_lease( + &mut self, + request: impl tonic::IntoRequest, + ) -> std::result::Result, tonic::Status> { + self.inner + .ready() + .await + .map_err(|e| tonic::Status::unknown(format!("Service was not ready: {}", e.into())))?; + let codec = tonic_prost::ProstCodec::default(); + let path = http::uri::PathAndQuery::from_static("/node_service.NodeService/AcquireSnapshotLease"); + let mut req = request.into_request(); + req.extensions_mut() + .insert(GrpcMethod::new("node_service.NodeService", "AcquireSnapshotLease")); + self.inner.unary(req, path, codec).await + } + pub async fn renew_snapshot_lease( + &mut self, + request: impl tonic::IntoRequest, + ) -> std::result::Result, tonic::Status> { + self.inner + .ready() + .await + .map_err(|e| tonic::Status::unknown(format!("Service was not ready: {}", e.into())))?; + let codec = tonic_prost::ProstCodec::default(); + let path = http::uri::PathAndQuery::from_static("/node_service.NodeService/RenewSnapshotLease"); + let mut req = request.into_request(); + req.extensions_mut() + .insert(GrpcMethod::new("node_service.NodeService", "RenewSnapshotLease")); + self.inner.unary(req, path, codec).await + } + pub async fn release_snapshot_lease( + &mut self, + request: impl tonic::IntoRequest, + ) -> std::result::Result, tonic::Status> { + self.inner + .ready() + .await + .map_err(|e| tonic::Status::unknown(format!("Service was not ready: {}", e.into())))?; + let codec = tonic_prost::ProstCodec::default(); + let path = http::uri::PathAndQuery::from_static("/node_service.NodeService/ReleaseSnapshotLease"); + let mut req = request.into_request(); + req.extensions_mut() + .insert(GrpcMethod::new("node_service.NodeService", "ReleaseSnapshotLease")); + self.inner.unary(req, path, codec).await + } pub async fn verify_file( &mut self, request: impl tonic::IntoRequest, @@ -2784,6 +2882,18 @@ pub mod node_service_server { &self, request: tonic::Request, ) -> std::result::Result, tonic::Status>; + async fn acquire_snapshot_lease( + &self, + request: tonic::Request, + ) -> std::result::Result, tonic::Status>; + async fn renew_snapshot_lease( + &self, + request: tonic::Request, + ) -> std::result::Result, tonic::Status>; + async fn release_snapshot_lease( + &self, + request: tonic::Request, + ) -> std::result::Result, tonic::Status>; async fn verify_file( &self, request: tonic::Request, @@ -3426,6 +3536,90 @@ pub mod node_service_server { }; Box::pin(fut) } + "/node_service.NodeService/AcquireSnapshotLease" => { + #[allow(non_camel_case_types)] + struct AcquireSnapshotLeaseSvc(pub Arc); + impl tonic::server::UnaryService for AcquireSnapshotLeaseSvc { + type Response = super::SnapshotLeaseResponse; + type Future = BoxFuture, tonic::Status>; + fn call(&mut self, request: tonic::Request) -> Self::Future { + let inner = Arc::clone(&self.0); + let fut = async move { ::acquire_snapshot_lease(&inner, request).await }; + Box::pin(fut) + } + } + let accept_compression_encodings = self.accept_compression_encodings; + let send_compression_encodings = self.send_compression_encodings; + let max_decoding_message_size = self.max_decoding_message_size; + let max_encoding_message_size = self.max_encoding_message_size; + let inner = self.inner.clone(); + let fut = async move { + let method = AcquireSnapshotLeaseSvc(inner); + let codec = tonic_prost::ProstCodec::default(); + let mut grpc = tonic::server::Grpc::new(codec) + .apply_compression_config(accept_compression_encodings, send_compression_encodings) + .apply_max_message_size_config(max_decoding_message_size, max_encoding_message_size); + let res = grpc.unary(method, req).await; + Ok(res) + }; + Box::pin(fut) + } + "/node_service.NodeService/RenewSnapshotLease" => { + #[allow(non_camel_case_types)] + struct RenewSnapshotLeaseSvc(pub Arc); + impl tonic::server::UnaryService for RenewSnapshotLeaseSvc { + type Response = super::SnapshotLeaseResponse; + type Future = BoxFuture, tonic::Status>; + fn call(&mut self, request: tonic::Request) -> Self::Future { + let inner = Arc::clone(&self.0); + let fut = async move { ::renew_snapshot_lease(&inner, request).await }; + Box::pin(fut) + } + } + let accept_compression_encodings = self.accept_compression_encodings; + let send_compression_encodings = self.send_compression_encodings; + let max_decoding_message_size = self.max_decoding_message_size; + let max_encoding_message_size = self.max_encoding_message_size; + let inner = self.inner.clone(); + let fut = async move { + let method = RenewSnapshotLeaseSvc(inner); + let codec = tonic_prost::ProstCodec::default(); + let mut grpc = tonic::server::Grpc::new(codec) + .apply_compression_config(accept_compression_encodings, send_compression_encodings) + .apply_max_message_size_config(max_decoding_message_size, max_encoding_message_size); + let res = grpc.unary(method, req).await; + Ok(res) + }; + Box::pin(fut) + } + "/node_service.NodeService/ReleaseSnapshotLease" => { + #[allow(non_camel_case_types)] + struct ReleaseSnapshotLeaseSvc(pub Arc); + impl tonic::server::UnaryService for ReleaseSnapshotLeaseSvc { + type Response = super::SnapshotLeaseMutationResponse; + type Future = BoxFuture, tonic::Status>; + fn call(&mut self, request: tonic::Request) -> Self::Future { + let inner = Arc::clone(&self.0); + let fut = async move { ::release_snapshot_lease(&inner, request).await }; + Box::pin(fut) + } + } + let accept_compression_encodings = self.accept_compression_encodings; + let send_compression_encodings = self.send_compression_encodings; + let max_decoding_message_size = self.max_decoding_message_size; + let max_encoding_message_size = self.max_encoding_message_size; + let inner = self.inner.clone(); + let fut = async move { + let method = ReleaseSnapshotLeaseSvc(inner); + let codec = tonic_prost::ProstCodec::default(); + let mut grpc = tonic::server::Grpc::new(codec) + .apply_compression_config(accept_compression_encodings, send_compression_encodings) + .apply_max_message_size_config(max_decoding_message_size, max_encoding_message_size); + let res = grpc.unary(method, req).await; + Ok(res) + }; + Box::pin(fut) + } "/node_service.NodeService/VerifyFile" => { #[allow(non_camel_case_types)] struct VerifyFileSvc(pub Arc); diff --git a/crates/protos/src/lib.rs b/crates/protos/src/lib.rs index 165ac3c52..5bbdcd2f9 100644 --- a/crates/protos/src/lib.rs +++ b/crates/protos/src/lib.rs @@ -451,6 +451,10 @@ impl CanonicalBodyBuilder { self.body.push(u8::from(field)); } + fn push_u64(&mut self, field: u64) { + self.body.extend_from_slice(&field.to_be_bytes()); + } + fn push_count(&mut self, count: usize) -> Result<(), std::num::TryFromIntError> { self.body.extend_from_slice(&u64::try_from(count)?.to_be_bytes()); Ok(()) @@ -575,6 +579,40 @@ pub fn canonical_delete_paths_request_body( Ok(body.finish()) } +pub fn canonical_snapshot_lease_request_body( + request: &proto_gen::node_service::SnapshotLeaseRequest, +) -> Result, std::num::TryFromIntError> { + let mut body = CanonicalBodyBuilder::new(b"rustfs-snapshot-lease-request-v1\0"); + body.push_str(&request.disk)?; + body.push_str(&request.volume)?; + body.push_str(&request.path)?; + body.push_u64(request.ttl_ms); + Ok(body.finish()) +} + +pub fn canonical_snapshot_lease_renew_request_body( + request: &proto_gen::node_service::SnapshotLeaseRenewRequest, +) -> Result, std::num::TryFromIntError> { + let mut body = CanonicalBodyBuilder::new(b"rustfs-snapshot-lease-renew-request-v1\0"); + body.push_str(&request.disk)?; + body.push_str(&request.volume)?; + body.push_str(&request.path)?; + body.push_bytes(&request.token)?; + body.push_u64(request.ttl_ms); + Ok(body.finish()) +} + +pub fn canonical_snapshot_lease_release_request_body( + request: &proto_gen::node_service::SnapshotLeaseReleaseRequest, +) -> Result, std::num::TryFromIntError> { + let mut body = CanonicalBodyBuilder::new(b"rustfs-snapshot-lease-release-request-v1\0"); + body.push_str(&request.disk)?; + body.push_str(&request.volume)?; + body.push_str(&request.path)?; + body.push_bytes(&request.token)?; + Ok(body.finish()) +} + pub fn canonical_rename_file_request_body( request: &proto_gen::node_service::RenameFileRequest, ) -> Result, std::num::TryFromIntError> { @@ -662,7 +700,8 @@ mod disk_mutation_canonical_tests { use super::proto_gen::node_service::{ DeletePathsRequest, DeleteRequest, DeleteVersionRequest, DeleteVersionsRequest, DeleteVolumeRequest, MakeVolumeRequest, MakeVolumesRequest, PreparePartTransactionRequest, RenameDataRequest, RenameFileRequest, RenamePartRequest, - SettlePartTransactionRequest, UpdateMetadataRequest, WriteAllRequest, WriteMetadataRequest, + SettlePartTransactionRequest, SnapshotLeaseReleaseRequest, SnapshotLeaseRenewRequest, SnapshotLeaseRequest, + UpdateMetadataRequest, WriteAllRequest, WriteMetadataRequest, }; use super::*; @@ -1020,6 +1059,58 @@ mod disk_mutation_canonical_tests { ); } + #[test] + fn snapshot_lease_canonical_bodies_bind_every_field() { + let acquire = SnapshotLeaseRequest { + disk: "d".into(), + volume: "v".into(), + path: "p".into(), + ttl_ms: 60_000, + }; + let mut acquire_bodies = vec![canonical_snapshot_lease_request_body(&acquire).unwrap()]; + for mutate in [ + |r: &mut SnapshotLeaseRequest| r.disk = "d2".into(), + |r: &mut SnapshotLeaseRequest| r.volume = "v2".into(), + |r: &mut SnapshotLeaseRequest| r.path = "p2".into(), + |r: &mut SnapshotLeaseRequest| r.ttl_ms = 60_001, + ] { + let mut request = acquire.clone(); + mutate(&mut request); + acquire_bodies.push(canonical_snapshot_lease_request_body(&request).unwrap()); + } + assert_all_distinct(&acquire_bodies); + + let renew = SnapshotLeaseRenewRequest { + disk: "d".into(), + volume: "v".into(), + path: "p".into(), + token: vec![1; 16].into(), + ttl_ms: 60_000, + }; + let mut changed_token = renew.clone(); + changed_token.token = vec![2; 16].into(); + let mut changed_ttl = renew.clone(); + changed_ttl.ttl_ms += 1; + assert_all_distinct(&[ + canonical_snapshot_lease_renew_request_body(&renew).unwrap(), + canonical_snapshot_lease_renew_request_body(&changed_token).unwrap(), + canonical_snapshot_lease_renew_request_body(&changed_ttl).unwrap(), + ]); + + let release = SnapshotLeaseReleaseRequest { + disk: "d".into(), + volume: "v".into(), + path: "p".into(), + token: vec![1; 16].into(), + }; + let mut changed_release = release.clone(); + changed_release.token = vec![2; 16].into(); + assert_ne!( + canonical_snapshot_lease_release_request_body(&release).unwrap(), + canonical_snapshot_lease_release_request_body(&changed_release).unwrap() + ); + } + #[test] fn disk_mutation_canonical_domains_are_distinct_per_message() { // The same field values must never authenticate one RPC's request as another's. diff --git a/crates/protos/src/node.proto b/crates/protos/src/node.proto index 36df6b5cb..112e88763 100644 --- a/crates/protos/src/node.proto +++ b/crates/protos/src/node.proto @@ -342,6 +342,40 @@ message DeletePathsResponse { optional Error error = 2; } +message SnapshotLeaseRequest { + string disk = 1; + string volume = 2; + string path = 3; + uint64 ttl_ms = 4; +} + +message SnapshotLeaseRenewRequest { + string disk = 1; + string volume = 2; + string path = 3; + bytes token = 4; + uint64 ttl_ms = 5; +} + +message SnapshotLeaseReleaseRequest { + string disk = 1; + string volume = 2; + string path = 3; + bytes token = 4; +} + +message SnapshotLeaseResponse { + bool success = 1; + bytes token = 2; + uint32 protocol_version = 3; + optional Error error = 4; +} + +message SnapshotLeaseMutationResponse { + bool success = 1; + optional Error error = 2; +} + message ReadMetadataRequest { string disk = 1; string volume = 2; @@ -966,6 +1000,9 @@ service NodeService { rpc ReadAll(ReadAllRequest) returns (ReadAllResponse) {}; rpc WriteAll(WriteAllRequest) returns (WriteAllResponse) {}; rpc Delete(DeleteRequest) returns (DeleteResponse) {}; + rpc AcquireSnapshotLease(SnapshotLeaseRequest) returns (SnapshotLeaseResponse) {}; + rpc RenewSnapshotLease(SnapshotLeaseRenewRequest) returns (SnapshotLeaseResponse) {}; + rpc ReleaseSnapshotLease(SnapshotLeaseReleaseRequest) returns (SnapshotLeaseMutationResponse) {}; rpc VerifyFile(VerifyFileRequest) returns (VerifyFileResponse) {}; rpc ReadParts(ReadPartsRequest) returns (ReadPartsResponse) {}; rpc CheckParts(CheckPartsRequest) returns (CheckPartsResponse) {}; diff --git a/rustfs/Cargo.toml b/rustfs/Cargo.toml index 13dca03c4..59e4d64d2 100644 --- a/rustfs/Cargo.toml +++ b/rustfs/Cargo.toml @@ -125,7 +125,7 @@ tokio = { workspace = true, features = ["rt-multi-thread", "macros", "net", "sig tokio-rustls = { workspace = true, default-features = false, features = ["logging", "tls12", "aws-lc-rs"] } aws-sdk-s3 = { workspace = true, default-features = false, features = ["sigv4a", "default-https-client", "rt-tokio"] } tokio-stream.workspace = true -tokio-util = { workspace = true, features = ["io", "compat"] } +tokio-util = { workspace = true, features = ["io", "compat", "time"] } tonic = { workspace = true, features = ["gzip", "deflate"] } tower = { workspace = true, features = ["timeout"] } tower-http = { workspace = true, features = ["trace", "compression-full", "cors", "catch-panic", "timeout", "limit", "request-id", "add-extension"] } diff --git a/rustfs/src/storage/rpc/node_service.rs b/rustfs/src/storage/rpc/node_service.rs index a7d2b272c..bf2f81f71 100644 --- a/rustfs/src/storage/rpc/node_service.rs +++ b/rustfs/src/storage/rpc/node_service.rs @@ -343,6 +343,7 @@ mod metrics; pub struct NodeService { local_peer: LocalPeerS3Client, context: Option>, + snapshot_lease_expiry: disk::SnapshotLeaseExpiryScheduler, } impl std::fmt::Debug for NodeService { @@ -361,7 +362,11 @@ pub fn make_server() -> NodeService { pub fn make_server_for_context(context: Option>) -> NodeService { let local_peer = LocalPeerS3Client::new(None, None); - NodeService { local_peer, context } + NodeService { + local_peer, + context, + snapshot_lease_expiry: disk::SnapshotLeaseExpiryScheduler::new(), + } } #[derive(Clone, Debug, Default)] @@ -1124,6 +1129,24 @@ impl Node for NodeService { async fn delete_paths(&self, request: Request) -> Result, Status> { self.handle_delete_paths(request).await } + async fn acquire_snapshot_lease( + &self, + request: Request, + ) -> Result, Status> { + self.handle_acquire_snapshot_lease(request).await + } + async fn renew_snapshot_lease( + &self, + request: Request, + ) -> Result, Status> { + self.handle_renew_snapshot_lease(request).await + } + async fn release_snapshot_lease( + &self, + request: Request, + ) -> Result, Status> { + self.handle_release_snapshot_lease(request).await + } async fn read_metadata(&self, request: Request) -> Result, Status> { self.handle_read_metadata(request).await } diff --git a/rustfs/src/storage/rpc/node_service/disk.rs b/rustfs/src/storage/rpc/node_service/disk.rs index e6c3fb60d..d5fa44815 100644 --- a/rustfs/src/storage/rpc/node_service/disk.rs +++ b/rustfs/src/storage/rpc/node_service/disk.rs @@ -14,11 +14,12 @@ use super::NodeService; use crate::storage::storage_api::rpc_consumer::node_service::{ - BatchReadVersionReq, BatchReadVersionResp, DeleteOptions, DiskError, DiskInfoOptions, FileInfoVersions, ReadMultipleReq, - ReadMultipleResp, ReadOptions, StorageDiskRpcExt as _, UpdateMetadataOpts, validate_batch_read_version_item_count, + BatchReadVersionReq, BatchReadVersionResp, DeleteOptions, DiskError, DiskInfoOptions, DiskStore, FileInfoVersions, + ReadMultipleReq, ReadMultipleResp, ReadOptions, StorageDiskRpcExt as _, UpdateMetadataOpts, + validate_batch_read_version_item_count, }; use crate::storage::storage_api::runtime_sources_consumer::runtime_sources; -use crate::storage::storage_api::{PartTransactionAction, verify_tonic_mutation_body_digest}; +use crate::storage::storage_api::{PartTransactionAction, SnapshotLeaseToken, verify_tonic_mutation_body_digest}; use bytes::Bytes; use rustfs_filemeta::FileInfo; use rustfs_io_metrics::internode_metrics::{ @@ -28,13 +29,96 @@ use rustfs_io_metrics::internode_metrics::{ }; use rustfs_protos::proto_gen::node_service::*; use serde::de::DeserializeOwned; -use std::io::Cursor; +use std::{collections::HashMap, io::Cursor, time::Duration}; +use tokio::sync::mpsc; +use tokio_util::time::DelayQueue; use tonic::{Request, Response, Status}; use tracing::debug; /// Initial capacity hint (bytes) for msgpack encode buffers, sized to cover a typical single- /// version `FileInfo` without repeated growth reallocations. Larger payloads still grow as needed. const MSGPACK_ENCODE_CAPACITY_HINT: usize = 512; +const SNAPSHOT_LEASE_PROTOCOL_VERSION: u32 = 1; +const SNAPSHOT_LEASE_MIN_TTL: Duration = Duration::from_secs(5); +const SNAPSHOT_LEASE_MAX_TTL: Duration = Duration::from_secs(5 * 60); + +struct SnapshotLeaseExpiry { + disk: DiskStore, + volume: String, + path: String, + token: SnapshotLeaseToken, +} + +pub(super) struct SnapshotLeaseExpiryScheduler { + tx: mpsc::UnboundedSender, +} + +enum SnapshotLeaseExpiryCommand { + Schedule(SnapshotLeaseExpiry, Duration), + Cancel(SnapshotLeaseToken), +} + +impl SnapshotLeaseExpiryScheduler { + pub(super) fn new() -> Self { + let (tx, mut rx) = mpsc::unbounded_channel(); + tokio::spawn(async move { + let mut expirations: DelayQueue = DelayQueue::new(); + let mut keys = HashMap::new(); + loop { + tokio::select! { + Some(command) = rx.recv() => { + match command { + SnapshotLeaseExpiryCommand::Schedule(expiry, ttl) => { + if let Some(key) = keys.remove(&expiry.token) { + expirations.remove(&key); + } + let token = expiry.token; + let key = expirations.insert(expiry, ttl); + keys.insert(token, key); + } + SnapshotLeaseExpiryCommand::Cancel(token) => { + if let Some(key) = keys.remove(&token) { + expirations.remove(&key); + } + } + } + } + Some(expired) = futures_util::StreamExt::next(&mut expirations), if !expirations.is_empty() => { + let expiry = expired.into_inner(); + keys.remove(&expiry.token); + let _ = expiry + .disk + .release_snapshot_lease(&expiry.volume, &expiry.path, expiry.token) + .await; + } + else => break, + } + } + }); + Self { tx } + } + + fn schedule(&self, expiry: SnapshotLeaseExpiry, ttl: Duration) -> Result<(), SnapshotLeaseExpiry> { + self.tx + .send(SnapshotLeaseExpiryCommand::Schedule(expiry, ttl)) + .map_err(|err| match err.0 { + SnapshotLeaseExpiryCommand::Schedule(expiry, _) => expiry, + SnapshotLeaseExpiryCommand::Cancel(_) => unreachable!(), + }) + } + + fn cancel(&self, token: SnapshotLeaseToken) { + let _ = self.tx.send(SnapshotLeaseExpiryCommand::Cancel(token)); + } +} + +fn snapshot_lease_ttl(ttl_ms: u64) -> Result { + let ttl = Duration::from_millis(ttl_ms); + if !(SNAPSHOT_LEASE_MIN_TTL..=SNAPSHOT_LEASE_MAX_TTL).contains(&ttl) { + return Err(Status::invalid_argument("snapshot lease TTL is outside the supported range")); + } + Ok(ttl) +} fn decode_msgpack_or_json( binary: &[u8], @@ -165,6 +249,146 @@ fn encode_batch_read_version_response_payloads( } impl NodeService { + pub(super) async fn handle_acquire_snapshot_lease( + &self, + request: Request, + ) -> Result, Status> { + verify_disk_mutation_digest( + &request, + rustfs_protos::canonical_snapshot_lease_request_body(request.get_ref()), + "acquire_snapshot_lease", + )?; + let request = request.into_inner(); + let ttl = snapshot_lease_ttl(request.ttl_ms)?; + let Some(disk) = self.find_disk(&request.disk).await else { + return Ok(Response::new(SnapshotLeaseResponse { + success: false, + token: Bytes::new(), + protocol_version: SNAPSHOT_LEASE_PROTOCOL_VERSION, + error: Some(DiskError::other("cannot find disk").into()), + })); + }; + match disk.acquire_snapshot_lease(&request.volume, &request.path).await { + Ok(token) => { + if let Err(expiry) = self.snapshot_lease_expiry.schedule( + SnapshotLeaseExpiry { + disk, + volume: request.volume, + path: request.path, + token, + }, + ttl, + ) { + let _ = expiry + .disk + .release_snapshot_lease(&expiry.volume, &expiry.path, expiry.token) + .await; + return Err(Status::internal("snapshot lease expiry scheduler is unavailable")); + } + Ok(Response::new(SnapshotLeaseResponse { + success: true, + token: Bytes::copy_from_slice(token.as_bytes()), + protocol_version: SNAPSHOT_LEASE_PROTOCOL_VERSION, + error: None, + })) + } + Err(err) => Ok(Response::new(SnapshotLeaseResponse { + success: false, + token: Bytes::new(), + protocol_version: SNAPSHOT_LEASE_PROTOCOL_VERSION, + error: Some(err.into()), + })), + } + } + + pub(super) async fn handle_renew_snapshot_lease( + &self, + request: Request, + ) -> Result, Status> { + verify_disk_mutation_digest( + &request, + rustfs_protos::canonical_snapshot_lease_renew_request_body(request.get_ref()), + "renew_snapshot_lease", + )?; + let request = request.into_inner(); + let ttl = snapshot_lease_ttl(request.ttl_ms)?; + let token = + SnapshotLeaseToken::from_slice(&request.token).map_err(|_| Status::invalid_argument("invalid lease token"))?; + let Some(disk) = self.find_disk(&request.disk).await else { + return Ok(Response::new(SnapshotLeaseResponse { + success: false, + token: Bytes::new(), + protocol_version: SNAPSHOT_LEASE_PROTOCOL_VERSION, + error: Some(DiskError::other("cannot find disk").into()), + })); + }; + match disk.renew_snapshot_lease(&request.volume, &request.path, token).await { + Ok(renewed) => { + if let Err(expiry) = self.snapshot_lease_expiry.schedule( + SnapshotLeaseExpiry { + disk, + volume: request.volume, + path: request.path, + token: renewed, + }, + ttl, + ) { + let _ = expiry + .disk + .release_snapshot_lease(&expiry.volume, &expiry.path, expiry.token) + .await; + return Err(Status::internal("snapshot lease expiry scheduler is unavailable")); + } + self.snapshot_lease_expiry.cancel(token); + Ok(Response::new(SnapshotLeaseResponse { + success: true, + token: Bytes::copy_from_slice(renewed.as_bytes()), + protocol_version: SNAPSHOT_LEASE_PROTOCOL_VERSION, + error: None, + })) + } + Err(err) => Ok(Response::new(SnapshotLeaseResponse { + success: false, + token: Bytes::new(), + protocol_version: SNAPSHOT_LEASE_PROTOCOL_VERSION, + error: Some(err.into()), + })), + } + } + + pub(super) async fn handle_release_snapshot_lease( + &self, + request: Request, + ) -> Result, Status> { + verify_disk_mutation_digest( + &request, + rustfs_protos::canonical_snapshot_lease_release_request_body(request.get_ref()), + "release_snapshot_lease", + )?; + let request = request.into_inner(); + let token = + SnapshotLeaseToken::from_slice(&request.token).map_err(|_| Status::invalid_argument("invalid lease token"))?; + let Some(disk) = self.find_disk(&request.disk).await else { + return Ok(Response::new(SnapshotLeaseMutationResponse { + success: false, + error: Some(DiskError::other("cannot find disk").into()), + })); + }; + match disk.release_snapshot_lease(&request.volume, &request.path, token).await { + Ok(()) => { + self.snapshot_lease_expiry.cancel(token); + Ok(Response::new(SnapshotLeaseMutationResponse { + success: true, + error: None, + })) + } + Err(err) => Ok(Response::new(SnapshotLeaseMutationResponse { + success: false, + error: Some(err.into()), + })), + } + } + pub(super) async fn handle_disk_info(&self, request: Request) -> Result, Status> { let request = request.into_inner(); if let Some(disk) = self.find_disk(&request.disk).await { @@ -1368,8 +1592,9 @@ impl NodeService { #[cfg(test)] mod tests { use super::{ - compat_response_json, decode_msgpack_or_json, encode_batch_read_version_response_payloads, encode_msgpack, - encode_msgpack_named, encode_read_multiple_response_payloads, + SNAPSHOT_LEASE_MAX_TTL, SNAPSHOT_LEASE_MIN_TTL, compat_response_json, decode_msgpack_or_json, + encode_batch_read_version_response_payloads, encode_msgpack, encode_msgpack_named, + encode_read_multiple_response_payloads, snapshot_lease_ttl, }; use crate::storage::storage_api::ReadMultipleResp; use crate::storage::storage_api::rpc_consumer::node_service::BatchReadVersionResp; @@ -1382,6 +1607,14 @@ mod tests { count: u32, } + #[test] + fn snapshot_lease_ttl_rejects_values_outside_server_bounds() { + assert!(snapshot_lease_ttl(4_999).is_err()); + assert_eq!(snapshot_lease_ttl(5_000).unwrap(), SNAPSHOT_LEASE_MIN_TTL); + assert_eq!(snapshot_lease_ttl(300_000).unwrap(), SNAPSHOT_LEASE_MAX_TTL); + assert!(snapshot_lease_ttl(300_001).is_err()); + } + #[test] fn decode_msgpack_or_json_prefers_binary_payload() { let payload = SamplePayload { diff --git a/rustfs/src/storage/storage_api.rs b/rustfs/src/storage/storage_api.rs index 1735ce2b4..859c9e384 100644 --- a/rustfs/src/storage/storage_api.rs +++ b/rustfs/src/storage/storage_api.rs @@ -431,7 +431,7 @@ pub(crate) mod ecstore_disk { pub(crate) use rustfs_ecstore::api::disk::{ BatchReadVersionReq, BatchReadVersionResp, CheckPartsResp, DeleteOptions, DiskAPI, DiskInfo, DiskInfoOptions, DiskStore, FileInfoVersions, FileReader, FileWriter, OldCurrentSize, PartTransactionAction, RUSTFS_META_BUCKET, ReadMultipleReq, - ReadMultipleResp, ReadOptions, RenameDataResp, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, + ReadMultipleResp, ReadOptions, RenameDataResp, SnapshotLeaseToken, UpdateMetadataOpts, VolumeInfo, WalkDirOptions, get_object_disk_read_timeout, validate_batch_read_version_item_count, }; pub(crate) use rustfs_ecstore::api::disk::{endpoint, error, error_reduce}; @@ -606,6 +606,7 @@ pub(crate) type ExpiryState = ecstore_bucket::lifecycle::bucket_lifecycle_ops::E pub(crate) type FileInfoVersions = ecstore_disk::FileInfoVersions; pub(crate) type FileReader = ecstore_disk::FileReader; pub(crate) type FileWriter = ecstore_disk::FileWriter; +pub(crate) type SnapshotLeaseToken = ecstore_disk::SnapshotLeaseToken; pub(crate) type FS = super::ecfs::FS; pub(crate) type HashReader = ecstore_rio::HashReader; pub(crate) type InstanceContext = ecstore_runtime::InstanceContext; @@ -1075,6 +1076,9 @@ pub(crate) trait StorageDiskRpcExt { ) -> DiskResult<()>; async fn read_metadata(&self, volume: &str, path: &str) -> DiskResult; async fn delete_paths(&self, volume: &str, paths: &[String]) -> DiskResult<()>; + async fn acquire_snapshot_lease(&self, volume: &str, path: &str) -> DiskResult; + async fn renew_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> DiskResult; + async fn release_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> DiskResult<()>; async fn stat_volume(&self, volume: &str) -> DiskResult; async fn list_volumes(&self) -> DiskResult>; async fn make_volume(&self, volume: &str) -> DiskResult<()>; @@ -1201,6 +1205,18 @@ where ecstore_disk::DiskAPI::delete_paths(self, volume, paths).await } + async fn acquire_snapshot_lease(&self, volume: &str, path: &str) -> DiskResult { + ecstore_disk::DiskAPI::acquire_snapshot_lease(self, volume, path).await + } + + async fn renew_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> DiskResult { + ecstore_disk::DiskAPI::renew_snapshot_lease(self, volume, path, token).await + } + + async fn release_snapshot_lease(&self, volume: &str, path: &str, token: SnapshotLeaseToken) -> DiskResult<()> { + ecstore_disk::DiskAPI::release_snapshot_lease(self, volume, path, token).await + } + async fn stat_volume(&self, volume: &str) -> DiskResult { ecstore_disk::DiskAPI::stat_volume(self, volume).await }