diff --git a/.config/nextest.toml b/.config/nextest.toml index 16418d2e6..0f1917bda 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -1,17 +1,14 @@ # nextest configuration for RustFS. # -# Serialize two known load-sensitive / global-state-sharing ecstore test groups -# so the full parallel nextest suite stops producing spurious failures -# (backlog #937). These tests pass in isolation but flake under the loaded -# parallel run for two distinct reasons: +# Serialize the ecstore tests that share the process-wide disk registry or +# exercise a multi-disk commit handoff across nextest process boundaries. # # * store::bucket::tests::bucket_delete_* share process/global state (disk # registry, lock client) and race make_bucket into InsufficientWriteQuorum # when run concurrently with other ecstore tests. # * bucket_lifecycle_ops::tests::concurrent_resend_same_part_commits_one_generation -# asserts a lock-acquire correctness property whose serialized cross-disk -# commits exceed the (already max'd, 60s) acquire deadline only when the -# suite saturates disk I/O. +# uses the shared multipart fixture and a deterministic uploadId-lock +# handoff, so it must not overlap another process mutating that fixture. # # serial_test's #[serial] attribute does NOT serialize these across runs: # nextest executes each test in its own process, where the in-process @@ -100,13 +97,6 @@ path = "junit.xml" # profile's own overrides list, not the default profile's). # =========================================================================== -# QUARANTINE: OPEN backlog#937 — concurrent_resend lock-acquire deadline flakes -# under saturated disk I/O in the full parallel suite. -[[profile.ci.overrides]] -filter = 'package(rustfs-ecstore) & test(concurrent_resend_same_part_commits_one_generation)' -test-group = 'ecstore-serial-flaky' -retries = 2 - # QUARANTINE: OPEN backlog#937 — store::bucket::tests::bucket_delete_* race # make_bucket into InsufficientWriteQuorum via shared global state under load. [[profile.ci.overrides]] @@ -114,6 +104,11 @@ filter = 'package(rustfs-ecstore) & test(/^store::bucket::tests::bucket_delete_( test-group = 'ecstore-serial-flaky' retries = 2 +# Keep the deterministic multipart handoff isolated across nextest processes. +[[profile.ci.overrides]] +filter = 'package(rustfs-ecstore) & test(concurrent_resend_same_part_commits_one_generation)' +test-group = 'ecstore-serial-flaky' + # QUARANTINE: OPEN rustfs#4690 — walk_dir stall-budget accounting test depends # on producer/consumer timing windows that stretch past the budget on loaded # CI runners (regression test for rustfs#4644; failed on a zero-Rust-diff PR). diff --git a/Cargo.lock b/Cargo.lock index 556b0ec36..15d6ffc12 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9732,6 +9732,7 @@ dependencies = [ "rustfs-policy", "rustfs-rio", "rustfs-storage-api", + "rustfs-test-utils", "rustfs-tls-runtime", "rustfs-trusted-proxies", "rustfs-utils", diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index eab2dedfa..0aafe9ded 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -128,7 +128,7 @@ pub mod bucket { get_object_lock_config, get_public_access_block_config, get_quota_config, get_replication_config, get_request_payment_config, get_sse_config, get_tagging_config, get_versioning_config, get_website_config, init_bucket_metadata_sys, list_bucket_targets, remove_bucket_metadata, set_bucket_metadata, update, - update_bucket_targets_under_transaction_lock, + update_bucket_targets_under_transaction_lock, update_config_with, }; } diff --git a/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs b/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs index 901f4ae50..5f270668f 100644 --- a/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs +++ b/crates/ecstore/src/bucket/lifecycle/bucket_lifecycle_ops.rs @@ -554,6 +554,7 @@ async fn delete_free_version_remote_object( oi: &ObjectInfo, tier_config_mgr: &Arc>, ) -> Result<(), std::io::Error> { + let version_id_exact = validate_transition_remote_version(oi)?; let identity = tier_destination_id_from_metadata(&oi.user_defined)? .ok_or_else(|| std::io::Error::other("tier free-version has no durable backend identity"))?; delete_object_from_remote_tier_idempotent_with_manager_and_identity( @@ -562,7 +563,7 @@ async fn delete_free_version_remote_object( &oi.transitioned_object.tier, identity, tier_config_mgr, - false, + version_id_exact, ) .await?; Ok(()) @@ -4201,6 +4202,23 @@ pub async fn get_transitioned_object_reader( get_transitioned_object_reader_with_tier_manager(bucket, object, rs, h, oi, opts, &tier_config_mgr).await } +fn validate_transition_remote_version(oi: &ObjectInfo) -> Result { + let version = oi.transitioned_object.version_id.as_str(); + match oi.transition_version_state { + rustfs_filemeta::TransitionVersionState::Unknown => Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "remote tier object version state is unknown", + )), + rustfs_filemeta::TransitionVersionState::KnownDisabled if version.is_empty() => Ok(false), + rustfs_filemeta::TransitionVersionState::SuspendedNull if version == "null" => Ok(true), + rustfs_filemeta::TransitionVersionState::Exact if !version.is_empty() && version != "null" => Ok(true), + _ => Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "remote tier object version state conflicts with its version ID", + )), + } +} + pub(crate) async fn get_transitioned_object_reader_with_tier_manager( bucket: &str, object: &str, @@ -4210,6 +4228,7 @@ pub(crate) async fn get_transitioned_object_reader_with_tier_manager( opts: &ObjectOptions, tier_config_mgr: &Arc>, ) -> Result { + validate_transition_remote_version(oi)?; let expected_identity = tier_destination_id_from_metadata(&oi.user_defined)?; let lease = match expected_identity { Some(identity) => { @@ -5506,6 +5525,7 @@ mod tests { tier: tier.clone(), ..Default::default() }, + transition_version_state: rustfs_filemeta::TransitionVersionState::Exact, ..Default::default() }; @@ -5569,6 +5589,7 @@ mod tests { tier, ..Default::default() }, + transition_version_state: rustfs_filemeta::TransitionVersionState::Exact, ..Default::default() }; @@ -5591,6 +5612,70 @@ mod tests { assert_eq!(backend.get_count().await, 0); } + #[cfg(feature = "test-util")] + #[tokio::test] + async fn transitioned_get_rejects_unknown_version_state_before_backend_io() { + let manager = TierConfigMgr::new(); + let tier = format!("COLDTIER{}", &Uuid::new_v4().simple().to_string()[..8]).to_uppercase(); + let backend = register_mock_tier(&manager, &tier).await; + let object_info = ObjectInfo { + bucket: "bucket".to_string(), + name: "object".to_string(), + size: 1, + transitioned_object: TransitionedObject { + name: "remote/object".to_string(), + version_id: String::new(), + status: crate::bucket::lifecycle::lifecycle::TRANSITION_COMPLETE.to_string(), + tier, + ..Default::default() + }, + transition_version_state: rustfs_filemeta::TransitionVersionState::Unknown, + ..Default::default() + }; + + let err = match get_transitioned_object_reader_with_tier_manager( + &object_info.bucket, + &object_info.name, + &None, + &HeaderMap::new(), + &object_info, + &ObjectOptions::default(), + &manager, + ) + .await + { + Ok(_) => panic!("unknown remote version state must fail before backend IO"), + Err(err) => err, + }; + + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + assert_eq!(backend.get_count().await, 0); + } + + #[cfg(feature = "test-util")] + #[tokio::test] + async fn free_version_delete_rejects_unknown_version_state_before_backend_io() { + let manager = TierConfigMgr::new(); + let backend = register_mock_tier(&manager, "WARM").await; + let object_info = ObjectInfo { + transitioned_object: TransitionedObject { + name: "remote/object".to_string(), + version_id: "legacy-version".to_string(), + tier: "WARM".to_string(), + ..Default::default() + }, + transition_version_state: rustfs_filemeta::TransitionVersionState::Unknown, + ..Default::default() + }; + + let err = super::delete_free_version_remote_object(&object_info, &manager) + .await + .expect_err("unknown remote version state must fail before backend IO"); + + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + assert_eq!(backend.remove_count().await, 0); + } + #[cfg(feature = "test-util")] #[tokio::test] async fn free_version_remote_delete_requires_persisted_destination_identity() { @@ -5640,6 +5725,7 @@ mod tests { oi.transitioned_object.tier = "WARM".to_string(); oi.transitioned_object.name = "remote/object".to_string(); oi.transitioned_object.version_id = "remote-version".to_string(); + oi.transition_version_state = rustfs_filemeta::TransitionVersionState::Exact; let local_delete_calls = Arc::new(std::sync::atomic::AtomicUsize::new(0)); let legacy_err = delete_free_version_remote_object_then(&oi, &manager, { @@ -5650,7 +5736,8 @@ mod tests { }) .await .expect_err("legacy free-version without identity must be retained"); - assert!(legacy_err.to_string().contains("no durable backend identity")); + assert_eq!(legacy_err.kind(), std::io::ErrorKind::Other); + assert_eq!(old_backend.remove_count().await, 0); assert_eq!(local_delete_calls.load(Ordering::Relaxed), 0); let mut invalid_metadata = HashMap::new(); @@ -5781,6 +5868,7 @@ mod tests { oi.transitioned_object.tier = "WARM".to_string(); oi.transitioned_object.name = "remote/object".to_string(); oi.transitioned_object.version_id = "remote-version".to_string(); + oi.transition_version_state = rustfs_filemeta::TransitionVersionState::Exact; let err = match get_transitioned_object_reader_with_tier_manager( "bucket", @@ -5796,7 +5884,12 @@ mod tests { Ok(_) => panic!("identity-bound GET must reject a same-name tier rebind"), Err(err) => err, }; - assert!(err.to_string().contains("identity no longer matches")); + assert_eq!(err.kind(), std::io::ErrorKind::Other); + let admin_err = err + .get_ref() + .and_then(|source| source.downcast_ref::()) + .expect("identity mismatch should retain the typed tier error"); + assert_eq!(admin_err.code, crate::services::tier::tier::ERR_TIER_INVALID_CONFIG.code); assert_eq!(new_backend.get_count().await, 0); oi.user_defined = Arc::new(HashMap::new()); @@ -5902,7 +5995,8 @@ mod tests { version_id: "remote-version".to_string(), tier_name: "WARM".to_string(), backend_identity: Some([1; 32]), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; let err = state @@ -6013,7 +6107,8 @@ mod tests { version_id: "remote-version".to_string(), tier_name: "WARM".to_string(), backend_identity: Some([1; 32]), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; state @@ -10081,6 +10176,87 @@ mod tests { (backend, identity_hex) } + #[cfg(feature = "test-util")] + #[tokio::test] + async fn journal_replay_rejects_unknown_version_state_before_backend_io() { + let (_disk_paths, ecstore) = setup_test_env().await; + let (backend, _) = register_recovery_mock_tier(&ecstore).await; + let identity = TierConfigMgr::acquire_operation_lease(&ecstore.tier_config_mgr(), "WARM") + .await + .expect("mock tier lease should be available") + .backend_identity(); + let je = Jentry { + obj_name: "remote/object".to_string(), + version_id: "legacy-version".to_string(), + tier_name: "WARM".to_string(), + backend_identity: Some(identity), + version_id_exact: false, + version_state: rustfs_filemeta::TransitionVersionState::Unknown, + }; + + let err = crate::bucket::lifecycle::tier_delete_journal::process_tier_delete_journal_entry(ecstore, &je) + .await + .expect_err("unknown journal state must fail before backend IO"); + + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + assert_eq!(backend.remove_count().await, 0); + } + + #[cfg(feature = "test-util")] + #[tokio::test] + async fn journal_replay_deletes_confirmed_exact_provider_token() { + let (_disk_paths, ecstore) = setup_test_env().await; + let (backend, _) = register_recovery_mock_tier(&ecstore).await; + let lease = TierConfigMgr::acquire_operation_lease(&ecstore.tier_config_mgr(), "WARM") + .await + .expect("mock tier lease should be available"); + let identity = lease.backend_identity(); + backend + .set_put_remote_version(Some("provider-version-token".to_string())) + .await; + lease + .put( + "remote/object", + crate::client::transition_api::ReaderImpl::Body(bytes::Bytes::from_static(b"candidate")), + 9, + ) + .await + .expect("confirmed remote candidate should be seeded"); + backend.set_remove_failure(true); + backend.set_reject_non_empty_remote_versions(true); + let je = Jentry { + obj_name: "remote/object".to_string(), + version_id: "provider-version-token".to_string(), + tier_name: "WARM".to_string(), + backend_identity: Some(identity), + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, + }; + + crate::set_disk::cleanup_rejected_transition_upload_durably( + &lease, + &je.obj_name, + &je.version_id, + true, + Some(ecstore.clone()), + ) + .await + .expect("failed immediate cleanup should remain durable in the journal"); + assert!(backend.contains(&je.obj_name).await); + + backend.set_remove_failure(false); + crate::bucket::lifecycle::tier_delete_journal::process_tier_delete_journal_entry(ecstore, &je) + .await + .expect("identity-bound exact journal must retry confirmed candidate cleanup"); + + assert!(!backend.contains(&je.obj_name).await); + assert_eq!(backend.exact_remove_count(), 2); + assert_eq!( + backend.remove_versions().await, + vec![("remote/object".to_string(), "provider-version-token".to_string())] + ); + } + async fn seed_recoverable_free_version( disk_paths: &[PathBuf], bucket: &str, @@ -10100,6 +10276,7 @@ mod tests { identity, ); } + let transition_version_id = Uuid::new_v4(); let mut metadata = FileMeta::new(); metadata .add_version(FileInfo { @@ -10108,7 +10285,9 @@ mod tests { version_id: Some(object_version_id), transition_status: crate::bucket::lifecycle::lifecycle::TRANSITION_COMPLETE.to_string(), transitioned_objname: format!("remote/{bucket}/{object}"), - transition_version_id: Some(Uuid::new_v4()), + transition_version_id: Some(transition_version_id), + transition_version: Some(transition_version_id.to_string()), + transition_version_state: rustfs_filemeta::TransitionVersionState::Exact, transition_tier: "WARM".to_string(), mod_time: Some(OffsetDateTime::now_utc()), metadata: transitioned_metadata, @@ -11112,6 +11291,7 @@ mod tests { #[tokio::test(flavor = "multi_thread")] #[serial] async fn concurrent_resend_same_part_commits_one_generation() { + use crate::set_disk::{MultipartCommitBarrier, MultipartCommitPause}; use crate::storage_api_contracts::object::ObjectIO as _; let (_paths, ecstore) = setup_test_env().await; @@ -11133,51 +11313,36 @@ mod tests { }) .collect(); - // Two independent causes can produce a spurious lock-acquire timeout - // here, and both must stay covered: - // 1. A lost/stolen fast-lock wakeup could strand a waiter until the - // deadline — fixed for real in fast_lock::shard by bounding each - // notification wait (NOTIFY_WAIT_CAP re-polling). - // 2. Under the full nextest suite on loaded CI disks, the - // *legitimately serialized* cross-disk commits can exceed the - // acquire deadline all by themselves — observed on CI at the 5s - // default and the 30s production default with six resends, and - // again at 60s, which is a hard ceiling: fast_lock clamps every - // requested timeout to MAX_ACQUIRE_TIMEOUT (60s), so raising the - // env override higher is a no-op (the Timeout error still reports - // the requested value). Keep the guard about the correctness - // property, not disk latency: request the full 60s ceiling and cap - // the queue depth at three resends, so the last waiter sits behind - // at most two serialized commits (~12s each on the slowest observed - // CI runner, comfortably inside the deadline). Three concurrent - // resends still race the streaming phase and contend on the commit - // lock, which is all the generation-mixing regression needs. - // `#[serial]` keeps the process-wide env override isolated. - let results = temp_env::async_with_vars([(rustfs_config::ENV_OBJECT_LOCK_ACQUIRE_TIMEOUT, Some("60"))], async { - let mut tasks = tokio::task::JoinSet::new(); - for payload in candidates.iter().cloned() { - let store = ecstore.clone(); - let bucket = bucket.clone(); - let upload_id = upload.upload_id.clone(); - tasks.spawn(async move { - let mut data = PutObjReader::from_vec(payload.clone()); - store - .put_object_part(&bucket, object, &upload_id, 1, &mut data, &ObjectOptions::default()) - .await - .map(|info| (info, payload)) - }); - } + let commit_barrier = MultipartCommitBarrier::install(&bucket, object, MultipartCommitPause::PutPartBeforeLockLost); + let start = Arc::new(tokio::sync::Barrier::new(candidates.len() + 1)); + let mut tasks = tokio::task::JoinSet::new(); + for payload in candidates.iter().cloned() { + let store = ecstore.clone(); + let bucket = bucket.clone(); + let upload_id = upload.upload_id.clone(); + let start = Arc::clone(&start); + tasks.spawn(async move { + start.wait().await; + let mut data = PutObjReader::from_vec(payload.clone()); + store + .put_object_part(&bucket, object, &upload_id, 1, &mut data, &ObjectOptions::default()) + .await + .map(|info| (info, payload)) + }); + } + start.wait().await; - // Every concurrent resend must succeed; the commit lock must never - // starve a waiter into a timeout. - let mut results = Vec::new(); - while let Some(joined) = tasks.join_next().await { - let outcome = joined.expect("put_object_part task should not panic"); - results.push(outcome.expect("every concurrent same-part resend must succeed without lock timeout")); - } - results - }) - .await; + // The first writer holds the uploadId commit lock while the other + // resends reach the same critical section. Releasing it proves the + // handoff without depending on saturated CI disk latency. + commit_barrier.wait_until_paused().await; + commit_barrier.release(); + + let mut results = Vec::new(); + while let Some(joined) = tasks.join_next().await { + let outcome = joined.expect("put_object_part task should not panic"); + results.push(outcome.expect("every concurrent same-part resend must succeed without lock timeout")); + } assert_eq!(results.len(), candidates.len()); // Exactly one generation is visible after the serialized commits, and its diff --git a/crates/ecstore/src/bucket/lifecycle/tier_delete_journal.rs b/crates/ecstore/src/bucket/lifecycle/tier_delete_journal.rs index 39d07ed05..a217d4978 100644 --- a/crates/ecstore/src/bucket/lifecycle/tier_delete_journal.rs +++ b/crates/ecstore/src/bucket/lifecycle/tier_delete_journal.rs @@ -20,7 +20,10 @@ use tokio_util::sync::CancellationToken; use tracing::{debug, warn}; use crate::bucket::lifecycle::config_boundary; -use crate::bucket::lifecycle::tier_sweeper::{Jentry, delete_object_from_remote_tier_idempotent_with_manager_and_identity}; +use crate::bucket::lifecycle::tier_sweeper::{ + Jentry, delete_confirmed_transition_candidate_exact_with_manager_and_identity, + delete_object_from_remote_tier_idempotent_with_manager_and_identity, +}; use crate::disk::RUSTFS_META_BUCKET; use crate::error::{Error, Result}; use crate::object_api::{GetObjectReader, ObjectInfo, ObjectOptions, PutObjReader}; @@ -42,6 +45,7 @@ const TIER_DELETE_JOURNAL_RECOVERY_INTERVAL: Duration = Duration::from_secs(60); const TIER_DELETE_JOURNAL_RECOVERY_TIMEOUT: Duration = Duration::from_secs(300); const TIER_DELETE_JOURNAL_VERSION: u8 = 2; const TIER_DELETE_JOURNAL_EXACT_VERSION: u8 = 3; +const TIER_DELETE_JOURNAL_STATE_VERSION: u8 = 4; pub(crate) const TIER_DELETE_JOURNAL_PREFIX: &str = "ilm/tier-delete-journal/"; #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] @@ -55,24 +59,35 @@ struct PersistedTierDeleteJournalEntry { backend_identity: Option<[u8; 32]>, #[serde(default, skip_serializing_if = "Option::is_none")] version_id_exact: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + version_state: Option, } impl PersistedTierDeleteJournalEntry { - fn from_jentry(je: &Jentry) -> Self { - Self { - version: if je.version_id_exact { - TIER_DELETE_JOURNAL_EXACT_VERSION - } else if je.backend_identity.is_some() { + fn from_jentry(je: &Jentry) -> Result { + validate_version_state(je.version_state, &je.version_id, je.version_id_exact)?; + let legacy_unknown = je.version_state == rustfs_filemeta::TransitionVersionState::Unknown; + let version = if legacy_unknown { + if je.backend_identity.is_some() { TIER_DELETE_JOURNAL_VERSION } else { 1 - }, + } + } else { + if je.backend_identity.is_none() { + return Err(Error::other("new tier delete journal entry is missing its backend identity")); + } + TIER_DELETE_JOURNAL_STATE_VERSION + }; + Ok(Self { + version, obj_name: je.obj_name.clone(), version_id: je.version_id.clone(), tier_name: je.tier_name.clone(), backend_identity: je.backend_identity, version_id_exact: je.version_id_exact.then_some(true), - } + version_state: (!legacy_unknown).then_some(je.version_state), + }) } fn into_jentry(self) -> Result { @@ -84,19 +99,23 @@ impl PersistedTierDeleteJournalEntry { if self.obj_name.is_empty() || self.tier_name.is_empty() { return Err(Error::other("tier delete journal entry is incomplete")); } - if self.version != TIER_DELETE_JOURNAL_EXACT_VERSION && self.version_id_exact.unwrap_or(false) { + if self.version != TIER_DELETE_JOURNAL_EXACT_VERSION + && self.version != TIER_DELETE_JOURNAL_STATE_VERSION + && self.version_id_exact.unwrap_or(false) + { return Err(Error::other( "legacy tier delete journal entry has an unsupported exact version constraint", )); } - let (backend_identity, version_id_exact) = match self.version { - 1 => (None, false), + let (backend_identity, version_id_exact, version_state) = match self.version { + 1 => (None, false, rustfs_filemeta::TransitionVersionState::Unknown), TIER_DELETE_JOURNAL_VERSION => ( Some( self.backend_identity .ok_or_else(|| Error::other("tier delete journal v2 entry is missing its backend identity"))?, ), false, + rustfs_filemeta::TransitionVersionState::Unknown, ), TIER_DELETE_JOURNAL_EXACT_VERSION => { if self.version_id.is_empty() || self.version_id_exact != Some(true) { @@ -108,6 +127,22 @@ impl PersistedTierDeleteJournalEntry { .ok_or_else(|| Error::other("tier delete journal v3 entry is missing its backend identity"))?, ), true, + rustfs_filemeta::TransitionVersionState::Exact, + ) + } + TIER_DELETE_JOURNAL_STATE_VERSION => { + let state = self + .version_state + .ok_or_else(|| Error::other("tier delete journal v4 entry is missing its version state"))?; + let exact = self.version_id_exact.unwrap_or(false); + validate_version_state(state, &self.version_id, exact)?; + ( + Some( + self.backend_identity + .ok_or_else(|| Error::other("tier delete journal v4 entry is missing its backend identity"))?, + ), + exact, + state, ) } version => return Err(Error::other(format!("unsupported tier delete journal version {version}"))), @@ -118,10 +153,30 @@ impl PersistedTierDeleteJournalEntry { tier_name: self.tier_name, backend_identity, version_id_exact, + version_state, }) } } +fn validate_version_state( + state: rustfs_filemeta::TransitionVersionState, + version_id: &str, + version_id_exact: bool, +) -> Result<()> { + use rustfs_filemeta::TransitionVersionState::{Exact, KnownDisabled, SuspendedNull, Unknown}; + + let valid = match state { + Unknown => !version_id_exact, + KnownDisabled => version_id.is_empty() && !version_id_exact, + SuspendedNull => version_id == "null" && version_id_exact, + Exact => !version_id.is_empty() && version_id != "null" && version_id_exact, + }; + if !valid { + return Err(Error::other("tier delete journal version state conflicts with its version id")); + } + Ok(()) +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct TierDeleteJournalRecoveryStats { pub scanned: usize, @@ -159,7 +214,7 @@ pub(crate) fn decode_tier_delete_journal_entry(data: &[u8]) -> Result { } pub(crate) fn encode_tier_delete_journal_entry(je: &Jentry) -> Result> { - serde_json::to_vec(&PersistedTierDeleteJournalEntry::from_jentry(je)) + serde_json::to_vec(&PersistedTierDeleteJournalEntry::from_jentry(je)?) .map_err(|err| Error::other(format!("encode tier delete journal failed: {err}"))) } @@ -209,18 +264,35 @@ where } pub async fn process_tier_delete_journal_entry(api: Arc, je: &Jentry) -> std::io::Result<()> { + if je.version_state == rustfs_filemeta::TransitionVersionState::Unknown { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "tier delete journal remote version state is unknown", + )); + } let backend_identity = je .backend_identity .ok_or_else(|| std::io::Error::other("legacy tier delete journal has no durable backend identity"))?; - delete_object_from_remote_tier_idempotent_with_manager_and_identity( - &je.obj_name, - &je.version_id, - &je.tier_name, - backend_identity, - &api.tier_config_mgr(), - je.version_id_exact, - ) - .await?; + if je.version_id_exact { + delete_confirmed_transition_candidate_exact_with_manager_and_identity( + &je.obj_name, + &je.version_id, + &je.tier_name, + backend_identity, + &api.tier_config_mgr(), + ) + .await?; + } else { + delete_object_from_remote_tier_idempotent_with_manager_and_identity( + &je.obj_name, + &je.version_id, + &je.tier_name, + backend_identity, + &api.tier_config_mgr(), + false, + ) + .await?; + } remove_tier_delete_journal_entry(api, je).await } @@ -406,8 +478,9 @@ where #[cfg(test)] mod tests { use super::{ - TIER_DELETE_JOURNAL_EXACT_VERSION, await_tier_delete_journal_recovery, decode_tier_delete_journal_entry, - encode_tier_delete_journal_entry, record_tier_delete_journal_backend_identity, tier_delete_journal_object_name, + TIER_DELETE_JOURNAL_EXACT_VERSION, TIER_DELETE_JOURNAL_STATE_VERSION, await_tier_delete_journal_recovery, + decode_tier_delete_journal_entry, encode_tier_delete_journal_entry, record_tier_delete_journal_backend_identity, + tier_delete_journal_object_name, }; use crate::bucket::lifecycle::tier_sweeper::Jentry; use crate::error::Result; @@ -420,7 +493,8 @@ mod tests { version_id: "remote-version".to_string(), tier_name: "WARM".to_string(), backend_identity: Some([7; 32]), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, } } @@ -436,6 +510,7 @@ mod tests { assert_eq!(decoded.tier_name, je.tier_name); assert_eq!(decoded.backend_identity, je.backend_identity); assert_eq!(decoded.version_id_exact, je.version_id_exact); + assert_eq!(decoded.version_state, je.version_state); } #[test] @@ -450,7 +525,7 @@ mod tests { let persisted: serde_json::Value = serde_json::from_slice(&encoded).expect("exact journal JSON should decode"); let decoded = decode_tier_delete_journal_entry(&encoded).expect("exact journal entry should decode"); - assert_eq!(persisted["version"], TIER_DELETE_JOURNAL_EXACT_VERSION); + assert_eq!(persisted["version"], TIER_DELETE_JOURNAL_STATE_VERSION); assert_eq!(persisted["version_id_exact"], true); assert!(decoded.version_id_exact); assert_ne!(tier_delete_journal_object_name(&exact), tier_delete_journal_object_name(&normalized)); @@ -513,6 +588,46 @@ mod tests { } } + #[test] + fn tier_delete_journal_rejects_conflicting_v4_version_states() { + let identity = vec![7_u8; 32]; + let invalid = [ + ("known-disabled", "unexpected", false), + ("suspended-null", "", true), + ("suspended-null", "null", false), + ("exact", "", true), + ("exact", "null", true), + ("exact", "version", false), + ("unknown", "version", true), + ]; + + for (state, version_id, exact) in invalid { + let persisted = serde_json::json!({ + "version": TIER_DELETE_JOURNAL_STATE_VERSION, + "obj_name": "remote/object", + "version_id": version_id, + "tier_name": "WARM", + "backend_identity": identity, + "version_id_exact": exact.then_some(true), + "version_state": state, + }); + let encoded = serde_json::to_vec(&persisted).expect("invalid journal fixture should encode"); + decode_tier_delete_journal_entry(&encoded).expect_err("conflicting v4 version state must fail closed"); + } + } + + #[test] + fn legacy_journals_decode_with_unknown_version_state() { + let v1 = br#"{"version":1,"obj_name":"remote/object","version_id":"opaque","tier_name":"WARM"}"#; + let v2 = br#"{"version":2,"obj_name":"remote/object","version_id":"opaque","tier_name":"WARM","backend_identity":[7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7]}"#; + + for payload in [v1.as_slice(), v2.as_slice()] { + let decoded = decode_tier_delete_journal_entry(payload).expect("legacy journal should decode"); + assert_eq!(decoded.version_state, rustfs_filemeta::TransitionVersionState::Unknown); + assert!(!decoded.version_id_exact); + } + } + #[test] fn tier_delete_journal_path_is_stable_and_sanitized() { let je = journal_entry(); @@ -530,6 +645,8 @@ mod tests { fn tier_delete_journal_paths_separate_legacy_and_backend_identities() { let mut legacy = journal_entry(); legacy.backend_identity = None; + legacy.version_id_exact = false; + legacy.version_state = rustfs_filemeta::TransitionVersionState::Unknown; let mut backend_a = journal_entry(); backend_a.backend_identity = Some([1; 32]); let mut backend_b = journal_entry(); @@ -575,6 +692,8 @@ mod tests { fn tier_delete_journal_without_transition_identity_stays_legacy() { let mut je = journal_entry(); je.backend_identity = None; + je.version_id_exact = false; + je.version_state = rustfs_filemeta::TransitionVersionState::Unknown; let encoded = encode_tier_delete_journal_entry(&je).expect("legacy journal should remain encodable"); let persisted: serde_json::Value = serde_json::from_slice(&encoded).expect("journal JSON should decode"); diff --git a/crates/ecstore/src/bucket/lifecycle/tier_sweeper.rs b/crates/ecstore/src/bucket/lifecycle/tier_sweeper.rs index 67ef6d15b..c4ca3b809 100644 --- a/crates/ecstore/src/bucket/lifecycle/tier_sweeper.rs +++ b/crates/ecstore/src/bucket/lifecycle/tier_sweeper.rs @@ -185,6 +185,7 @@ struct ObjSweeper { transition_status: String, transition_tier: String, transition_version_id: String, + transition_version_state: rustfs_filemeta::TransitionVersionState, remote_object: String, } @@ -231,7 +232,9 @@ impl ObjSweeper { } pub fn should_remove_remote_object(&self) -> Option { - if self.transition_status != lifecycle::TRANSITION_COMPLETE { + if self.transition_status != lifecycle::TRANSITION_COMPLETE + || self.transition_version_state == rustfs_filemeta::TransitionVersionState::Unknown + { return None; } @@ -249,7 +252,11 @@ impl ObjSweeper { version_id: self.transition_version_id.clone(), tier_name: self.transition_tier.clone(), backend_identity: None, - version_id_exact: false, + version_id_exact: matches!( + self.transition_version_state, + rustfs_filemeta::TransitionVersionState::SuspendedNull | rustfs_filemeta::TransitionVersionState::Exact + ), + version_state: self.transition_version_state, }); } None @@ -286,6 +293,7 @@ pub struct Jentry { pub(crate) tier_name: String, pub(crate) backend_identity: Option, pub(crate) version_id_exact: bool, + pub(crate) version_state: rustfs_filemeta::TransitionVersionState, } impl ExpiryOp for Jentry { @@ -330,7 +338,7 @@ async fn delete_object_from_remote_tier_raw_with_manager( let lease = TierConfigMgr::acquire_operation_lease(&tier_config_mgr, tier_name) .await .map_err(std::io::Error::other)?; - delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, &lease, false).await + delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, &lease, false, true).await } async fn delete_object_from_remote_tier_raw_with_lease( @@ -338,8 +346,11 @@ async fn delete_object_from_remote_tier_raw_with_lease( rv_id: &str, lease: &TierOperationLease, version_id_exact: bool, + validate_remote_version_id: bool, ) -> Result<(), std::io::Error> { - lease.validate_remote_version_id(rv_id)?; + if validate_remote_version_id { + lease.validate_remote_version_id(rv_id)?; + } if remote_delete_breaker_is_open(Instant::now()).await { metrics::counter!(METRIC_DELETE_REMOTE_BREAKER_TOTAL).increment(1); @@ -435,7 +446,53 @@ pub(crate) async fn delete_object_from_remote_tier_with_lease_idempotent( lease: &TierOperationLease, version_id_exact: bool, ) -> Result { - match delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, lease, version_id_exact).await { + delete_object_from_remote_tier_with_lease_idempotent_inner(obj_name, rv_id, lease, version_id_exact, true).await +} + +pub(crate) async fn delete_confirmed_transition_candidate_exact_with_lease_idempotent( + obj_name: &str, + rv_id: &str, + lease: &TierOperationLease, +) -> Result { + if rv_id.is_empty() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "confirmed versioned transition candidate requires a non-empty remote version", + )); + } + #[cfg(test)] + if obj_name == "remote/empty-guard-probe" { + CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } + delete_object_from_remote_tier_with_lease_idempotent_inner(obj_name, rv_id, lease, true, false).await +} + +#[cfg(test)] +static CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + +pub(crate) async fn delete_confirmed_transition_candidate_exact_with_manager_and_identity( + obj_name: &str, + rv_id: &str, + tier_name: &str, + backend_identity: TierDestinationId, + tier_config_mgr: &Arc>, +) -> Result { + let lease = TierConfigMgr::acquire_operation_lease_for_backend_identity(tier_config_mgr, tier_name, backend_identity) + .await + .map_err(std::io::Error::other)?; + delete_confirmed_transition_candidate_exact_with_lease_idempotent(obj_name, rv_id, &lease).await +} + +async fn delete_object_from_remote_tier_with_lease_idempotent_inner( + obj_name: &str, + rv_id: &str, + lease: &TierOperationLease, + version_id_exact: bool, + validate_remote_version_id: bool, +) -> Result { + match delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, lease, version_id_exact, validate_remote_version_id) + .await + { Ok(()) => Ok(RemoteTierDeleteOutcome::Deleted), Err(err) if is_remote_tier_not_found_error(&err) => Ok(RemoteTierDeleteOutcome::AlreadyRemoved), Err(err) => { @@ -460,6 +517,7 @@ pub fn transitioned_delete_journal_entry( versioned: bool, suspended: bool, transitioned: &TransitionedObject, + transition_version_state: rustfs_filemeta::TransitionVersionState, ) -> Option { let sweeper = ObjSweeper { version_id, @@ -468,6 +526,7 @@ pub fn transitioned_delete_journal_entry( transition_status: transitioned.status.clone(), transition_tier: transitioned.tier.clone(), transition_version_id: transitioned.version_id.clone(), + transition_version_state, remote_object: transitioned.name.clone(), ..Default::default() }; @@ -475,8 +534,13 @@ pub fn transitioned_delete_journal_entry( sweeper.should_remove_remote_object() } -pub fn transitioned_force_delete_journal_entry(transitioned: &TransitionedObject) -> Option { - if transitioned.status != lifecycle::TRANSITION_COMPLETE { +pub fn transitioned_force_delete_journal_entry( + transitioned: &TransitionedObject, + transition_version_state: rustfs_filemeta::TransitionVersionState, +) -> Option { + if transitioned.status != lifecycle::TRANSITION_COMPLETE + || transition_version_state == rustfs_filemeta::TransitionVersionState::Unknown + { return None; } @@ -485,7 +549,11 @@ pub fn transitioned_force_delete_journal_entry(transitioned: &TransitionedObject version_id: transitioned.version_id.clone(), tier_name: transitioned.tier.clone(), backend_identity: None, - version_id_exact: false, + version_id_exact: matches!( + transition_version_state, + rustfs_filemeta::TransitionVersionState::SuspendedNull | rustfs_filemeta::TransitionVersionState::Exact + ), + version_state: transition_version_state, }) } @@ -494,11 +562,14 @@ mod test { use crate::client::signer_error::invalid_utf8_header_error; use super::{ - ERR_REMOTE_DELETE_BREAKER_OPEN, ERR_REMOTE_DELETE_LIMITER_CLOSED, RemoteDeleteBreaker, RemoteTierDeleteOutcome, + CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES, ERR_REMOTE_DELETE_BREAKER_OPEN, ERR_REMOTE_DELETE_LIMITER_CLOSED, + RemoteDeleteBreaker, RemoteTierDeleteOutcome, delete_confirmed_transition_candidate_exact_with_manager_and_identity, delete_object_from_remote_tier_idempotent, delete_object_from_remote_tier_idempotent_with_manager_and_identity, - is_remote_tier_not_found_error, is_signer_header_error, set_remote_tier_delete_test_hook, - should_record_remote_delete_failure, + is_remote_tier_not_found_error, is_signer_header_error, lifecycle, set_remote_tier_delete_test_hook, + should_record_remote_delete_failure, transitioned_delete_journal_entry, transitioned_force_delete_journal_entry, }; + use crate::storage_api_contracts::lifecycle::TransitionedObject; + use rustfs_filemeta::TransitionVersionState; use std::io::{Error, ErrorKind}; use std::time::{Duration, Instant}; @@ -542,6 +613,43 @@ mod test { assert!(should_record_remote_delete_failure(&Error::other("NoSuchVersion"))); } + #[test] + fn transitioned_delete_journal_preserves_remote_version_state() { + let cases = [ + (TransitionVersionState::Unknown, "legacy-version", None), + (TransitionVersionState::KnownDisabled, "", Some(false)), + (TransitionVersionState::SuspendedNull, "null", Some(true)), + (TransitionVersionState::Exact, "opaque-version", Some(true)), + ]; + + for (state, version_id, expected_exact) in cases { + let transitioned = TransitionedObject { + name: "remote/object".to_string(), + version_id: version_id.to_string(), + tier: "WARM".to_string(), + status: lifecycle::TRANSITION_COMPLETE.to_string(), + ..Default::default() + }; + let regular = transitioned_delete_journal_entry(None, false, false, &transitioned, state); + let forced = transitioned_force_delete_journal_entry(&transitioned, state); + + match expected_exact { + Some(expected_exact) => { + let regular = regular.expect("known version state should produce a regular delete journal entry"); + assert_eq!(regular.version_state, state); + assert_eq!(regular.version_id_exact, expected_exact); + let forced = forced.expect("known version state should produce a forced delete journal entry"); + assert_eq!(forced.version_state, state); + assert_eq!(forced.version_id_exact, expected_exact); + } + None => { + assert!(regular.is_none()); + assert!(forced.is_none()); + } + } + } + } + #[tokio::test] #[serial_test::serial] async fn idempotent_remote_delete_treats_hooked_nosuchversion_as_already_removed() { @@ -664,6 +772,55 @@ mod test { assert_eq!(backend.remove_versions().await, vec![("remote/object".to_string(), String::new())]); } + #[cfg(feature = "test-util")] + #[tokio::test] + #[serial_test::serial] + async fn confirmed_transition_cleanup_deletes_exact_provider_token() { + CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES.store(0, std::sync::atomic::Ordering::Relaxed); + let manager = crate::services::tier::tier::TierConfigMgr::new(); + let backend = crate::services::tier::test_util::register_mock_tier(&manager, "WARM").await; + let lease = crate::services::tier::tier::TierConfigMgr::acquire_operation_lease(&manager, "WARM") + .await + .expect("test tier lease should be available"); + let identity = lease.backend_identity(); + drop(lease); + backend.set_reject_non_empty_remote_versions(true); + + let outcome = delete_confirmed_transition_candidate_exact_with_manager_and_identity( + "remote/object", + "provider-version-token", + "WARM", + identity, + &manager, + ) + .await + .expect("confirmed upload compensation should delete the exact provider token"); + + assert_eq!(outcome, RemoteTierDeleteOutcome::Deleted); + assert_eq!(backend.exact_remove_count(), 1); + assert_eq!( + backend.remove_versions().await, + vec![("remote/object".to_string(), "provider-version-token".to_string())] + ); + + let err = delete_confirmed_transition_candidate_exact_with_manager_and_identity( + "remote/empty-guard-probe", + "", + "WARM", + identity, + &manager, + ) + .await + .expect_err("confirmed versioned cleanup must reject an empty token"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!(backend.remove_count().await, 1); + assert_eq!( + CONFIRMED_TRANSITION_EMPTY_GUARD_DISPATCHES.load(std::sync::atomic::Ordering::Relaxed), + 0, + "empty remote versions must be rejected before exact cleanup dispatch" + ); + } + #[test] fn breaker_opens_at_threshold_and_recovers_after_window() { let mut breaker = RemoteDeleteBreaker::new(3, Duration::from_secs(30)); diff --git a/crates/ecstore/src/bucket/lifecycle/transition_transaction.rs b/crates/ecstore/src/bucket/lifecycle/transition_transaction.rs index 823ae1d2a..e3f3036e8 100644 --- a/crates/ecstore/src/bucket/lifecycle/transition_transaction.rs +++ b/crates/ecstore/src/bucket/lifecycle/transition_transaction.rs @@ -22,7 +22,10 @@ use uuid::Uuid; use crate::bucket::lifecycle::config_boundary; use crate::bucket::lifecycle::lifecycle::TRANSITION_COMPLETE; -use crate::bucket::lifecycle::tier_sweeper::delete_object_from_remote_tier_idempotent_with_manager_and_identity; +use crate::bucket::lifecycle::tier_sweeper::{ + delete_confirmed_transition_candidate_exact_with_manager_and_identity, + delete_object_from_remote_tier_idempotent_with_manager_and_identity, +}; use crate::disk::RUSTFS_META_BUCKET; use crate::error::{Error, Result as EcstoreResult}; use crate::object_api::ObjectOptions; @@ -708,6 +711,21 @@ async fn recover_unknown_upload_outcome( TransitionCandidateProbe::UnversionedPresent => { cleanup_recovered_unknown_upload_candidate(api, transaction, TransitionRemoteVersion::unversioned()).await } + TransitionCandidateProbe::VersionedPresent(version_id) + if Uuid::parse_str(&version_id).is_ok_and(|version_id| version_id.is_nil()) => + { + delete_confirmed_transition_candidate_exact_with_manager_and_identity( + &transaction.remote_object, + &version_id, + &transaction.tier_name, + transaction.backend_fingerprint, + &api.tier_config_mgr(), + ) + .await + .map_err(Error::other)?; + delete_transition_transaction_record(api, transaction.transaction_id).await?; + Ok(TransitionTransactionRecoveryOutcome::RemoteCandidateDeleted) + } TransitionCandidateProbe::VersionedPresent(version_id) => { cleanup_recovered_unknown_upload_candidate(api, transaction, TransitionRemoteVersion::versioned(version_id)).await } diff --git a/crates/ecstore/src/bucket/metadata.rs b/crates/ecstore/src/bucket/metadata.rs index eddce55fc..441036227 100644 --- a/crates/ecstore/src/bucket/metadata.rs +++ b/crates/ecstore/src/bucket/metadata.rs @@ -737,6 +737,9 @@ impl BucketMetadata { } BUCKET_TAGGING_CONFIG => { self.tagging_config_xml = data; + // Drop the parsed form (like lifecycle above) so clearing the + // payload can't leave stale parsed tags to be cached. + self.tagging_config = None; self.tagging_config_updated_at = updated; } BUCKET_QUOTA_CONFIG_FILE => { @@ -1318,6 +1321,30 @@ mod test { assert!(bm.lifecycle_config.is_none()); } + /// Companion to the lifecycle case above. `parse_all_configs` skips empty + /// XML rather than clearing, so without the explicit reset a cleared + /// tagging config would keep serving the previously parsed tags. + #[test] + fn tagging_update_config_clears_parsed_config_on_delete() { + let mut bm = BucketMetadata::new("test-bucket"); + let tagging_xml = br#"envprod"#; + + bm.update_config(BUCKET_TAGGING_CONFIG, tagging_xml.to_vec()) + .expect("tagging config should update"); + bm.parse_all_configs().expect("tagging config should parse"); + assert!(bm.tagging_config.is_some()); + + bm.update_config(BUCKET_TAGGING_CONFIG, Vec::new()) + .expect("tagging config delete should update metadata"); + + assert!(bm.tagging_config_xml.is_empty()); + assert!(bm.tagging_config.is_none()); + + // A re-parse must not resurrect them either. + bm.parse_all_configs().expect("cleared tagging should parse"); + assert!(bm.tagging_config.is_none()); + } + #[tokio::test] async fn marshal_msg_complete_example() { // Create a complete BucketMetadata with various configurations diff --git a/crates/ecstore/src/bucket/metadata_sys.rs b/crates/ecstore/src/bucket/metadata_sys.rs index 97dbf9147..dbd9bb573 100644 --- a/crates/ecstore/src/bucket/metadata_sys.rs +++ b/crates/ecstore/src/bucket/metadata_sys.rs @@ -239,6 +239,35 @@ pub async fn update_bucket_targets_under_transaction_lock(bucket: &str, data: Ve bucket_meta_sys.update(bucket, BUCKET_TARGETS_FILE, data).await } +/// Read-modify-write one bucket config file under the metadata system's +/// outer write guard. +/// +/// `mutate` sees the freshly loaded on-disk metadata and returns the +/// replacement payload for `config_file` (empty clears it, like +/// [`delete`]). Both the read and the persisted write happen inside the +/// same guard that [`update`] uses, so within this process the rewrite can +/// neither clobber a concurrent update to another config file nor lose a +/// concurrent write to the same one — unlike caching a mutated clone of +/// previously read metadata. +/// +/// This guard is process-local. Writers on other nodes still race, exactly +/// as they do for [`update`]: each rewrites the whole metadata file, so the +/// later save wins. What this narrows is the window — from "as stale as the +/// local cache" down to a single metadata read plus write. +pub async fn update_config_with(bucket: &str, config_file: &str, mutate: F) -> Result +where + F: FnOnce(&BucketMetadata) -> Result> + Send, +{ + let bucket_meta_sys_lock = get_bucket_metadata_sys()?; + let _targets_guard = if config_file == BUCKET_TARGETS_FILE { + Some(acquire_bucket_targets_transaction_lock(bucket).await?) + } else { + None + }; + let mut bucket_meta_sys = bucket_meta_sys_lock.write().await; + bucket_meta_sys.update_config_with(bucket, config_file, mutate).await +} + pub async fn acquire_bucket_targets_transaction_lock(bucket: &str) -> Result { let bucket_meta_sys_lock = get_bucket_metadata_sys()?; let api = bucket_meta_sys_lock.read().await.object_store(); @@ -603,24 +632,7 @@ impl BucketMetadataSys { return Err(Error::other("errServerNotInitialized")); }; - if is_meta_bucketname(bucket) { - return Err(Error::other("errInvalidArgument")); - } - - let mut bm = match load_bucket_metadata_parse(store, bucket, parse).await { - Ok(res) => res, - Err(err) => { - if !runtime_sources::setup_is_erasure().await - && !runtime_sources::setup_is_dist_erasure().await - && is_err_bucket_not_found(&err) - { - BucketMetadata::new(bucket) - } else { - error!("load bucket metadata failed: {}", err); - return Err(err); - } - } - }; + let mut bm = Self::load_bucket_metadata_for_update(store, bucket, parse).await?; let updated = bm.update_config(config_file, data)?; @@ -629,6 +641,49 @@ impl BucketMetadataSys { Ok(updated) } + /// See the free [`update_config_with`]: same load-mutate-persist cycle as + /// [`Self::update`], with the payload computed from the loaded metadata + /// instead of supplied up front. Loads through this system's own store so + /// the read and the persisted write target the same instance. + async fn update_config_with(&mut self, bucket: &str, config_file: &str, mutate: F) -> Result + where + F: FnOnce(&BucketMetadata) -> Result> + Send, + { + let mut bm = Self::load_bucket_metadata_for_update(self.api.clone(), bucket, true).await?; + + let data = mutate(&bm)?; + let updated = bm.update_config(config_file, data)?; + + self.save(bm).await?; + + Ok(updated) + } + + /// Load a bucket's on-disk metadata as the base of a config rewrite. + /// Outside erasure setups a missing metadata file degrades to a fresh + /// default (legacy buckets without one); erasure setups fail instead of + /// fabricating state that a quorum may still hold. + async fn load_bucket_metadata_for_update(store: Arc, bucket: &str, parse: bool) -> Result { + if is_meta_bucketname(bucket) { + return Err(Error::other("errInvalidArgument")); + } + + match load_bucket_metadata_parse(store, bucket, parse).await { + Ok(res) => Ok(res), + Err(err) => { + if !runtime_sources::setup_is_erasure().await + && !runtime_sources::setup_is_dist_erasure().await + && is_err_bucket_not_found(&err) + { + Ok(BucketMetadata::new(bucket)) + } else { + error!("load bucket metadata failed: {}", err); + Err(err) + } + } + } + } + async fn save(&self, bm: BucketMetadata) -> Result<()> { if is_meta_bucketname(&bm.name) { return Err(Error::other("errInvalidArgument")); @@ -1068,6 +1123,122 @@ mod tests { assert!(matches!(err, Error::Io(_)), "malformed persisted policy must surface its parse failure"); } + /// A tagging rewrite through `update_config_with` (the Swift metadata + /// POST path) is persisted: it survives a metadata reload from disk, and + /// an emptied rewrite clears the config in the cached copy too instead of + /// leaving stale parsed tags behind. + #[tokio::test] + async fn update_config_with_persists_tagging_rewrite_across_disk_reload() { + use crate::bucket::metadata::BUCKET_TAGGING_CONFIG; + use s3s::dto::Tag; + + let (_dirs, ecstore) = isolated_store_over_temp_disks().await; + let mut sys = BucketMetadataSys::new(ecstore); + + let bucket = "swift-tagging-bucket"; + sys.persist_and_set(BucketMetadata::new(bucket)) + .await + .expect("initial metadata should persist"); + + let tagging = Tagging { + tag_set: vec![Tag { + key: Some("swift-meta-color".to_string()), + value: Some("blue".to_string()), + }], + }; + let xml = crate::bucket::utils::serialize::(&tagging).expect("tagging should serialize"); + sys.update_config_with(bucket, BUCKET_TAGGING_CONFIG, move |bm| { + assert!(bm.tagging_config.is_none(), "rewrite must see the on-disk state"); + Ok(xml) + }) + .await + .expect("tagging rewrite should persist"); + + // Simulate the disk-truth reload that used to lose Swift writes: drop + // the cached entry and lazily re-load from the metadata file. + sys.metadata_map.write().await.clear(); + let (tags, _) = sys + .get_tagging_config(bucket) + .await + .expect("tagging must survive a reload from disk"); + assert_eq!(tags.tag_set.len(), 1); + assert_eq!(tags.tag_set[0].key.as_deref(), Some("swift-meta-color")); + assert_eq!(tags.tag_set[0].value.as_deref(), Some("blue")); + + // An emptied rewrite clears the config everywhere. + sys.update_config_with(bucket, BUCKET_TAGGING_CONFIG, |bm| { + assert!(bm.tagging_config.is_some(), "rewrite must see the persisted tags"); + Ok(Vec::new()) + }) + .await + .expect("clearing rewrite should persist"); + assert_eq!( + sys.get_tagging_config(bucket).await.unwrap_err(), + Error::ConfigNotFound, + "cleared tagging must not be served from the cache" + ); + sys.metadata_map.write().await.clear(); + assert_eq!( + sys.get_tagging_config(bucket).await.unwrap_err(), + Error::ConfigNotFound, + "cleared tagging must not reappear after a reload from disk" + ); + } + + /// The load and the persisted write share one write guard, so concurrent + /// rewrites of the same config compose instead of clobbering each other. + /// Moving the load outside that guard loses all but the last tag. + #[tokio::test] + async fn concurrent_update_config_with_calls_do_not_lose_writes() { + use crate::bucket::metadata::BUCKET_TAGGING_CONFIG; + use s3s::dto::Tag; + + let (_dirs, ecstore) = isolated_store_over_temp_disks().await; + let sys = Arc::new(RwLock::new(BucketMetadataSys::new(ecstore))); + + let bucket = "swift-tagging-concurrent"; + sys.read() + .await + .persist_and_set(BucketMetadata::new(bucket)) + .await + .expect("initial metadata should persist"); + + const WRITERS: usize = 8; + let mut handles = Vec::with_capacity(WRITERS); + for idx in 0..WRITERS { + let sys = sys.clone(); + handles.push(tokio::spawn(async move { + sys.write() + .await + .update_config_with(bucket, BUCKET_TAGGING_CONFIG, move |bm| { + // Each writer merges its own tag onto whatever is + // currently persisted — the Swift rewrite shape. + let mut tagging = bm.tagging_config.clone().unwrap_or_else(|| Tagging { tag_set: vec![] }); + tagging.tag_set.push(Tag { + key: Some(format!("swift-meta-key{idx}")), + value: Some(idx.to_string()), + }); + crate::bucket::utils::serialize::(&tagging).map_err(|e| Error::other(e.to_string())) + }) + .await + })); + } + + for handle in handles { + handle + .await + .expect("writer task should join") + .expect("rewrite should persist"); + } + + let (tags, _) = sys + .read() + .await + .get_tagging_config(bucket) + .await + .expect("tagging should be readable"); + assert_eq!(tags.tag_set.len(), WRITERS, "every concurrent rewrite must survive: {tags:?}"); + } fn target(bucket: &str, id: &str) -> BucketTarget { BucketTarget { diff --git a/crates/ecstore/src/config/com.rs b/crates/ecstore/src/config/com.rs index 0f3eb9ab5..539464173 100644 --- a/crates/ecstore/src/config/com.rs +++ b/crates/ecstore/src/config/com.rs @@ -2543,6 +2543,7 @@ mod tests { data_dir: None, delete_marker: false, transitioned_object: Default::default(), + transition_version_state: Default::default(), restore_ongoing: false, restore_expires: None, user_tags: Arc::new(String::new()), diff --git a/crates/ecstore/src/object_api/types.rs b/crates/ecstore/src/object_api/types.rs index c9a0e560b..dfd403e18 100644 --- a/crates/ecstore/src/object_api/types.rs +++ b/crates/ecstore/src/object_api/types.rs @@ -180,6 +180,7 @@ pub struct ObjectInfo { pub data_dir: Option, pub delete_marker: bool, pub transitioned_object: TransitionedObject, + pub transition_version_state: rustfs_filemeta::TransitionVersionState, pub restore_ongoing: bool, pub restore_expires: Option, pub user_tags: Arc, @@ -220,6 +221,7 @@ impl Clone for ObjectInfo { data_dir: self.data_dir, delete_marker: self.delete_marker, transitioned_object: self.transitioned_object.clone(), + transition_version_state: self.transition_version_state, restore_ongoing: self.restore_ongoing, restore_expires: self.restore_expires, user_tags: self.user_tags.clone(), @@ -464,11 +466,11 @@ impl ObjectInfo { let transitioned_object = TransitionedObject { name: fi.transitioned_objname.clone(), - version_id: if let Some(transition_version_id) = fi.transition_version_id { - transition_version_id.to_string() - } else { - "".to_string() - }, + version_id: fi + .transition_version + .clone() + .or_else(|| fi.transition_version_id.map(|version_id| version_id.to_string())) + .unwrap_or_default(), status: fi.transition_status.clone(), free_version: fi.tier_free_version(), tier: fi.transition_tier.clone(), @@ -537,6 +539,7 @@ impl ObjectInfo { inlined, user_defined: Arc::new(metadata), transitioned_object, + transition_version_state: fi.transition_version_state, checksum: fi.checksum.clone(), storage_class, restore_ongoing, diff --git a/crates/ecstore/src/services/tier/test_util.rs b/crates/ecstore/src/services/tier/test_util.rs index 1a2b37d26..751f2e92b 100644 --- a/crates/ecstore/src/services/tier/test_util.rs +++ b/crates/ecstore/src/services/tier/test_util.rs @@ -931,7 +931,10 @@ pub async fn read_transition_meta(disk_path: &Path, bucket: &str, object: &str) status: fi.transition_status.clone(), tier: fi.transition_tier.clone(), remote_object: fi.transitioned_objname.clone(), - remote_version_id: fi.transition_version_id.map(|id| id.to_string()), + remote_version_id: fi + .transition_version + .clone() + .or_else(|| fi.transition_version_id.map(|id| id.to_string())), free_version_count, }) } diff --git a/crates/ecstore/src/services/tier/tier.rs b/crates/ecstore/src/services/tier/tier.rs index c501d52ec..5ead6a9d9 100644 --- a/crates/ecstore/src/services/tier/tier.rs +++ b/crates/ecstore/src/services/tier/tier.rs @@ -9274,7 +9274,8 @@ mod tests { version_id: "v1".to_string(), tier_name: "COLD-A".to_string(), backend_identity: Some(current_identity), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; journal_store .insert_config_object( diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index 4972274f1..b8fd83ced 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -547,6 +547,8 @@ pub(in crate::set_disk) fn metadata_early_stop_candidate_matches(left: &FileInfo && left.transitioned_objname == right.transitioned_objname && left.transition_tier == right.transition_tier && left.transition_version_id == right.transition_version_id + && left.transition_version == right.transition_version + && left.transition_version_state == right.transition_version_state && left.expire_restored == right.expire_restored && left.size == right.size && left.mod_time == right.mod_time diff --git a/crates/ecstore/src/set_disk/metadata.rs b/crates/ecstore/src/set_disk/metadata.rs index 136a6cf7e..7eb4c57ad 100644 --- a/crates/ecstore/src/set_disk/metadata.rs +++ b/crates/ecstore/src/set_disk/metadata.rs @@ -578,6 +578,13 @@ impl SetDisks { Self::update_hash_str(hasher, &meta.transition_tier); Self::update_hash_str(hasher, &meta.transitioned_objname); Self::update_hash_optional_uuid(hasher, meta.transition_version_id); + Self::update_hash_optional_str(hasher, meta.transition_version.as_deref()); + hasher.update([match meta.transition_version_state { + rustfs_filemeta::TransitionVersionState::Unknown => 0, + rustfs_filemeta::TransitionVersionState::KnownDisabled => 1, + rustfs_filemeta::TransitionVersionState::SuspendedNull => 2, + rustfs_filemeta::TransitionVersionState::Exact => 3, + }]); Self::update_hash_optional_u32(hasher, meta.mode); Self::update_hash_optional_u64(hasher, meta.written_by_version); diff --git a/crates/ecstore/src/set_disk/mod.rs b/crates/ecstore/src/set_disk/mod.rs index ca81a41b8..b2e5c654e 100644 --- a/crates/ecstore/src/set_disk/mod.rs +++ b/crates/ecstore/src/set_disk/mod.rs @@ -854,9 +854,13 @@ mod core; mod ctx; mod metadata; mod ops; +#[cfg(test)] +pub(crate) use ops::multipart::{MultipartCommitBarrier, MultipartCommitPause}; #[cfg(feature = "test-util")] pub(crate) use ops::object::TransitionCleanupStoreBarrier as SetDiskTransitionCleanupStoreBarrier; pub(crate) use ops::object::body_cache_plaintext_len; +#[cfg(test)] +pub(crate) use ops::object::cleanup_rejected_transition_upload_durably; mod read; mod replication; pub(crate) mod shard_source; diff --git a/crates/ecstore/src/set_disk/ops/list.rs b/crates/ecstore/src/set_disk/ops/list.rs index 11384914c..85eaaa188 100644 --- a/crates/ecstore/src/set_disk/ops/list.rs +++ b/crates/ecstore/src/set_disk/ops/list.rs @@ -29,6 +29,12 @@ impl SetDisks { pub async fn delete_all(&self, bucket: &str, prefix: &str) -> Result<()> { ListOperations::new(self.ctx()).delete_all(bucket, prefix).await } + + pub(crate) async fn delete_all_with_quorum(&self, bucket: &str, prefix: &str, write_quorum: usize) -> Result<()> { + ListOperations::new(self.ctx()) + .delete_all_with_quorum(bucket, prefix, write_quorum) + .await + } } /// List/prefix maintenance operations, borrowing the `SetDisks` core state @@ -48,6 +54,14 @@ impl<'a> ListOperations<'a> { } pub(crate) async fn delete_all(&self, bucket: &str, prefix: &str) -> Result<()> { + self.delete_all_inner(bucket, prefix, None).await + } + + async fn delete_all_with_quorum(&self, bucket: &str, prefix: &str, write_quorum: usize) -> Result<()> { + self.delete_all_inner(bucket, prefix, Some(write_quorum)).await + } + + async fn delete_all_inner(&self, bucket: &str, prefix: &str, write_quorum: Option) -> Result<()> { let disks = self.ctx.disks().read().await; let disks = disks.clone(); @@ -79,6 +93,9 @@ impl<'a> ListOperations<'a> { Ok(_) => { errors.push(None); } + Err(DiskError::FileNotFound | DiskError::PathNotFound | DiskError::VolumeNotFound) => { + errors.push(None); + } Err(e) => { errors.push(Some(e)); } @@ -97,6 +114,12 @@ impl<'a> ListOperations<'a> { ); } + if let Some(write_quorum) = write_quorum + && let Some(err) = reduce_write_quorum_errs(&errors, OBJECT_OP_IGNORED_ERRS, write_quorum) + { + return Err(err.into()); + } + Ok(()) } } diff --git a/crates/ecstore/src/set_disk/ops/multipart.rs b/crates/ecstore/src/set_disk/ops/multipart.rs index fd3416fa8..6eaaa3399 100644 --- a/crates/ecstore/src/set_disk/ops/multipart.rs +++ b/crates/ecstore/src/set_disk/ops/multipart.rs @@ -26,6 +26,8 @@ use crate::crash_inject::{self, CrashPoint}; use crate::multipart_listing::paginate_multipart_listing; use futures::{StreamExt, stream}; use std::future::Future; +#[cfg(test)] +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use tokio::task::JoinSet; @@ -33,7 +35,7 @@ const MULTIPART_LIST_IO_CONCURRENCY: usize = 16; #[cfg(test)] #[derive(Clone, Copy, PartialEq, Eq)] -enum MultipartCommitPause { +pub(crate) enum MultipartCommitPause { PutPartBeforeLockLost, PutPartAfterRename, BeforeLockLost, @@ -45,12 +47,13 @@ struct MultipartCommitBarrierState { bucket: String, object: String, pause: MultipartCommitPause, + armed: AtomicBool, arrived: tokio::sync::Notify, release: tokio::sync::Notify, } #[cfg(test)] -struct MultipartCommitBarrier { +pub(crate) struct MultipartCommitBarrier { state: Arc, } @@ -60,11 +63,12 @@ static MULTIPART_COMMIT_BARRIER: std::sync::OnceLock Self { + pub(crate) fn install(bucket: &str, object: &str, pause: MultipartCommitPause) -> Self { let state = Arc::new(MultipartCommitBarrierState { bucket: bucket.to_string(), object: object.to_string(), pause, + armed: AtomicBool::new(true), arrived: tokio::sync::Notify::new(), release: tokio::sync::Notify::new(), }); @@ -78,13 +82,13 @@ impl MultipartCommitBarrier { Self { state } } - async fn wait_until_paused(&self) { + pub(crate) async fn wait_until_paused(&self) { tokio::time::timeout(Duration::from_secs(30), self.state.arrived.notified()) .await .expect("multipart completion should reach the deterministic commit barrier"); } - fn release(&self) { + pub(crate) fn release(&self) { self.state.release.notify_one(); } } @@ -112,7 +116,9 @@ async fn pause_multipart_commit(bucket: &str, object: &str, pause: MultipartComm .as_ref() .filter(|barrier| barrier.bucket == bucket && barrier.object == object && barrier.pause == pause) .cloned(); - if let Some(barrier) = barrier { + if let Some(barrier) = barrier + && barrier.armed.swap(false, Ordering::AcqRel) + { barrier.arrived.notify_one(); barrier.release.notified().await; } @@ -777,6 +783,9 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { mut max_parts: usize, opts: &ObjectOptions, ) -> Result { + let _upload_guard = self + .acquire_multipart_upload_read_lock("list_object_parts", bucket, object, upload_id, opts) + .await?; let (fi, _) = self.check_upload_id_exists(bucket, object, upload_id, false).await?; let upload_id_path = Self::get_upload_id_dir(bucket, object, upload_id); @@ -1254,10 +1263,15 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { let _upload_guard = self .acquire_multipart_upload_write_lock("abort_multipart_upload", bucket, object, upload_id, opts) .await?; - self.check_upload_id_exists(bucket, object, upload_id, false).await?; + let (fi, _) = self.check_upload_id_exists(bucket, object, upload_id, true).await?; let upload_id_path = Self::get_upload_id_dir(bucket, object, upload_id); - self.delete_all(RUSTFS_META_MULTIPART_BUCKET, &upload_id_path).await + self.delete_all_with_quorum( + RUSTFS_META_MULTIPART_BUCKET, + &upload_id_path, + fi.write_quorum(self.default_write_quorum()), + ) + .await } // complete_multipart_upload finished #[tracing::instrument(skip(self))] @@ -1815,7 +1829,36 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { #[cfg(test)] pause_multipart_commit(bucket, object, MultipartCommitPause::AfterRename).await; - drop(upload_guard); + + let cleanup_store = self.clone(); + let cleanup_upload_id_path = upload_id_path.clone(); + let cleanup_bucket = bucket.to_owned(); + let cleanup_object = object.to_owned(); + let cleanup_upload_id = upload_id.to_owned(); + let cleanup_handle = tokio::spawn(async move { + let _upload_guard = upload_guard; + if let Err(err) = cleanup_store + .delete_all_with_quorum(RUSTFS_META_MULTIPART_BUCKET, &cleanup_upload_id_path, write_quorum) + .await + { + warn!( + bucket = %cleanup_bucket, + object = %cleanup_object, + upload_id = %cleanup_upload_id, + error = ?err, + "completed multipart upload staging cleanup did not reach write quorum" + ); + } + }); + if let Err(err) = cleanup_handle.await { + warn!( + bucket = %bucket, + object = %object, + upload_id = %upload_id, + error = ?err, + "completed multipart upload staging cleanup task failed" + ); + } drop(object_lock_guard); // drop object lock guard to release the lock // backlog#1321: enqueue heal only when the committed replicas actually @@ -1860,12 +1903,6 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks { }); } - let upload_id_path = upload_id_path.clone(); - let store = self.clone(); - let _cleanup_handle = tokio::spawn(async move { - let _ = store.delete_all(RUSTFS_META_MULTIPART_BUCKET, &upload_id_path).await; - }); - for (i, op_disk) in online_disks.iter().enumerate() { if let Some(disk) = op_disk && disk.is_online().await @@ -2177,6 +2214,122 @@ mod tests { ) } + async fn assert_complete_first_linearizes(bucket: &'static str, object: &'static str, create_opts: ObjectOptions) { + let manager = Arc::new(rustfs_lock::GlobalLockManager::new()); + let signaling = Arc::new(SignalingLockClient::new(Arc::new(LocalClient::with_manager(manager)))); + let lockers: Vec> = vec![signaling.clone()]; + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks_with_lockers(4, 0, 2, lockers).await; + make_bucket_on_all(&disk_stores, bucket).await; + let (upload_id, parts) = stage_upload_with_create_opts(&set_disks, bucket, object, &[0x47; 4096], &create_opts).await; + let upload_id_path = SetDisks::get_upload_id_dir(bucket, object, &upload_id); + signaling.set_target(rustfs_lock::ObjectKey::new(RUSTFS_META_MULTIPART_BUCKET, upload_id_path)); + let _setup_type_guard = SetupTypeGuard::switch_to(SetupType::DistErasure).await; + let barrier = MultipartCommitBarrier::install(bucket, object, MultipartCommitPause::AfterRename); + + let complete_store = set_disks.clone(); + let complete_upload_id = upload_id.clone(); + let complete = tokio::spawn(async move { + complete_store + .complete_multipart_upload(bucket, object, &complete_upload_id, parts, &ObjectOptions::default()) + .await + }); + barrier.wait_until_paused().await; + + let abort_store = set_disks.clone(); + let abort_upload_id = upload_id.clone(); + let abort = tokio::spawn(async move { + abort_store + .abort_multipart_upload(bucket, object, &abort_upload_id, &ObjectOptions::default()) + .await + }); + signaling.wait_for_attempts(2).await; + assert!(!abort.is_finished(), "abort must wait for the completion upload lock"); + + barrier.release(); + complete + .await + .expect("completion task should not panic") + .expect("completion should win the upload finalization"); + let abort_err = abort + .await + .expect("abort task should not panic") + .expect_err("abort must observe the upload as finalized"); + assert!(matches!(abort_err, StorageError::InvalidUploadID(..))); + set_disks + .get_object_info(bucket, object, &ObjectOptions::default()) + .await + .expect("complete-first must leave the committed object readable"); + assert!(matches!( + set_disks.check_upload_id_exists(bucket, object, &upload_id, false).await, + Err(StorageError::InvalidUploadID(..)) + )); + } + + async fn assert_abort_first_linearizes(bucket: &'static str, object: &'static str, create_opts: ObjectOptions) { + let manager = Arc::new(rustfs_lock::GlobalLockManager::new()); + let signaling = Arc::new(SignalingLockClient::new(Arc::new(LocalClient::with_manager(manager)))); + let lockers: Vec> = vec![signaling.clone()]; + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks_with_lockers(4, 0, 2, lockers).await; + make_bucket_on_all(&disk_stores, bucket).await; + let (upload_id, parts) = stage_upload_with_create_opts(&set_disks, bucket, object, &[0x48; 4096], &create_opts).await; + let upload_id_path = SetDisks::get_upload_id_dir(bucket, object, &upload_id); + signaling.set_target(rustfs_lock::ObjectKey::new(RUSTFS_META_MULTIPART_BUCKET, upload_id_path.clone())); + let _setup_type_guard = SetupTypeGuard::switch_to(SetupType::DistErasure).await; + let object_holder = set_disks + .new_ns_lock(bucket, object) + .await + .expect("object namespace lock should be created") + .get_write_lock(Duration::from_secs(5)) + .await + .expect("test should hold the object lock"); + let holder = set_disks + .new_ns_lock(RUSTFS_META_MULTIPART_BUCKET, &upload_id_path) + .await + .expect("upload namespace lock should be created") + .get_write_lock(Duration::from_secs(5)) + .await + .expect("test should hold the upload lock"); + signaling.wait_for_attempts(1).await; + + let abort_store = set_disks.clone(); + let abort_upload_id = upload_id.clone(); + let abort = tokio::spawn(async move { + abort_store + .abort_multipart_upload(bucket, object, &abort_upload_id, &ObjectOptions::default()) + .await + }); + signaling.wait_for_attempts(2).await; + + let complete_store = set_disks.clone(); + let complete_upload_id = upload_id.clone(); + let complete = tokio::spawn(async move { + complete_store + .complete_multipart_upload(bucket, object, &complete_upload_id, parts, &ObjectOptions::default()) + .await + }); + drop(holder); + + abort + .await + .expect("abort task should not panic") + .expect("abort should win the upload finalization"); + drop(object_holder); + let complete_err = complete + .await + .expect("completion task should not panic") + .expect_err("completion must observe the aborted upload"); + assert!(matches!(complete_err, StorageError::InvalidUploadID(..))); + let object_err = set_disks + .get_object_info(bucket, object, &ObjectOptions::default()) + .await + .expect_err("abort-first must not publish an object"); + assert!(matches!(object_err, StorageError::ObjectNotFound(..))); + assert!(matches!( + set_disks.check_upload_id_exists(bucket, object, &upload_id, false).await, + Err(StorageError::InvalidUploadID(..)) + )); + } + async fn assert_quorum_minus_one_retry_preserves_completable_part( disk_count: usize, parity: usize, @@ -3039,6 +3192,81 @@ mod tests { .await; } + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn abort_and_complete_linearize_for_plain_sse_and_legacy_layouts() { + assert_complete_first_linearizes("multipart-complete-first-plain", "object", ObjectOptions::default()).await; + assert_abort_first_linearizes("multipart-abort-first-plain", "object", ObjectOptions::default()).await; + + let encrypted_opts = ObjectOptions { + user_defined: HashMap::from([(SSEC_ALGORITHM_HEADER.to_string(), "AES256".to_string())]), + ..Default::default() + }; + temp_env::async_with_vars([(crate::object_api::ENV_RUSTFS_ENCRYPTED_RANGE_SEEK, Some("true"))], async { + assert_complete_first_linearizes("multipart-complete-first-sse", "object", encrypted_opts.clone()).await; + assert_abort_first_linearizes("multipart-abort-first-sse", "object", encrypted_opts.clone()).await; + }) + .await; + temp_env::async_with_vars([(crate::object_api::ENV_RUSTFS_ENCRYPTED_RANGE_SEEK, Some("false"))], async { + assert_complete_first_linearizes("multipart-complete-first-legacy", "object", encrypted_opts.clone()).await; + assert_abort_first_linearizes("multipart-abort-first-legacy", "object", encrypted_opts).await; + }) + .await; + } + + #[tokio::test] + async fn abort_enforces_delete_write_quorum_boundary() { + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; + let bucket = "multipart-abort-delete-quorum"; + let object = "object"; + make_bucket_on_all(&disk_stores, bucket).await; + let quorum_upload = set_disks + .new_multipart_upload(bucket, object, &ObjectOptions::default()) + .await + .expect("multipart upload should be created"); + + let saved_disks = { + let mut disks = set_disks.disks.write().await; + let saved = disks.clone(); + disks[3] = None; + saved + }; + set_disks + .abort_multipart_upload(bucket, object, &quorum_upload.upload_id, &ObjectOptions::default()) + .await + .expect("abort should succeed at the exact delete write quorum"); + *set_disks.disks.write().await = saved_disks; + assert!(matches!( + set_disks + .check_upload_id_exists(bucket, object, &quorum_upload.upload_id, false) + .await, + Err(StorageError::InvalidUploadID(..)) + )); + + let below_quorum_upload = set_disks + .new_multipart_upload(bucket, object, &ObjectOptions::default()) + .await + .expect("second multipart upload should be created"); + let saved_disks = { + let mut disks = set_disks.disks.write().await; + let saved = disks.clone(); + disks[2] = None; + disks[3] = None; + saved + }; + let err = set_disks + .abort_multipart_upload(bucket, object, &below_quorum_upload.upload_id, &ObjectOptions::default()) + .await + .expect_err("abort must report a delete below write quorum"); + assert!(matches!(err, StorageError::ErasureWriteQuorum)); + + *set_disks.disks.write().await = saved_disks; + set_disks + .check_upload_id_exists(bucket, object, &below_quorum_upload.upload_id, false) + .await + .expect("failed abort must leave quorum-visible staging on the restored disks"); + } + #[tokio::test(flavor = "multi_thread")] #[serial] async fn complete_revalidates_layout_candidate_after_upload_lock() { @@ -3170,6 +3398,17 @@ mod tests { tokio::task::yield_now().await; assert!(!abort.is_finished(), "abort must wait until completion releases the upload lock"); + let list_store = set_disks.clone(); + let list_upload_id = upload_id.clone(); + let list = tokio::spawn(async move { + list_store + .list_object_parts(bucket, object, &list_upload_id, None, MAX_PARTS_COUNT, &ObjectOptions::default()) + .await + }); + signaling.wait_for_attempts(3).await; + tokio::task::yield_now().await; + assert!(!list.is_finished(), "ListParts must wait until completion releases the upload lock"); + barrier.release(); complete .await @@ -3180,10 +3419,68 @@ mod tests { .expect("abort task should not panic") .expect_err("the committed upload should no longer exist when abort acquires the lock"); assert!(matches!(abort_err, StorageError::InvalidUploadID(..))); + let list_err = list + .await + .expect("ListParts task should not panic") + .expect_err("the committed upload should no longer exist when ListParts acquires the lock"); + assert!(matches!(list_err, StorageError::InvalidUploadID(..))); }) .await; } + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn complete_validates_parts_after_an_inflight_upload_part_commit() { + let manager = Arc::new(rustfs_lock::GlobalLockManager::new()); + let signaling = Arc::new(SignalingLockClient::new(Arc::new(LocalClient::with_manager(manager)))); + let lockers: Vec> = vec![signaling.clone()]; + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks_with_lockers(4, 0, 2, lockers).await; + let bucket = "multipart-complete-put-part-race-bucket"; + let object = "object"; + make_bucket_on_all(&disk_stores, bucket).await; + let (upload_id, original_parts) = + stage_upload_with_create_opts(&set_disks, bucket, object, &[0x49; 4096], &ObjectOptions::default()).await; + let upload_id_path = SetDisks::get_upload_id_dir(bucket, object, &upload_id); + signaling.set_target(rustfs_lock::ObjectKey::new(RUSTFS_META_MULTIPART_BUCKET, upload_id_path)); + let _setup_type_guard = SetupTypeGuard::switch_to(SetupType::DistErasure).await; + let barrier = MultipartCommitBarrier::install(bucket, object, MultipartCommitPause::PutPartBeforeLockLost); + + let put_store = set_disks.clone(); + let put_upload_id = upload_id.clone(); + let put = tokio::spawn(async move { + let mut reader = PutObjReader::from_vec(vec![0x4a; 4096]); + put_store + .put_object_part(bucket, object, &put_upload_id, 1, &mut reader, &ObjectOptions::default()) + .await + }); + barrier.wait_until_paused().await; + + let complete_store = set_disks.clone(); + let complete_upload_id = upload_id.clone(); + let complete = tokio::spawn(async move { + complete_store + .complete_multipart_upload(bucket, object, &complete_upload_id, original_parts, &ObjectOptions::default()) + .await + }); + signaling.wait_for_attempts(2).await; + tokio::task::yield_now().await; + assert!(!complete.is_finished(), "completion must wait for the UploadPart commit lock"); + + barrier.release(); + put.await + .expect("UploadPart task should not panic") + .expect("UploadPart replacement should commit"); + let err = complete + .await + .expect("completion task should not panic") + .expect_err("completion must reject the stale ETag after UploadPart wins"); + assert!(matches!(err, StorageError::InvalidPart(..))); + set_disks + .list_object_parts(bucket, object, &upload_id, None, MAX_PARTS_COUNT, &ObjectOptions::default()) + .await + .expect("failed completion must leave the upload retryable"); + } + #[tokio::test(start_paused = true)] #[serial] async fn complete_fences_upload_lock_loss_before_commit() { diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index 287449f43..198846589 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -25,7 +25,10 @@ use crate::set_disk::read::GetObjectDownstreamWriter; use crate::bucket::lifecycle::{ tier_delete_journal::{persist_tier_delete_journal_entry, remove_tier_delete_journal_entry}, - tier_sweeper::{Jentry, RemoteTierDeleteOutcome, delete_object_from_remote_tier_with_lease_idempotent}, + tier_sweeper::{ + Jentry, RemoteTierDeleteOutcome, delete_confirmed_transition_candidate_exact_with_lease_idempotent, + delete_object_from_remote_tier_with_lease_idempotent, + }, transition_transaction::{ TransitionRemoteVersion, TransitionSourceIdentity, TransitionSourceVersionMode, TransitionTransaction, TransitionTransactionInit, TransitionTransactionState, delete_transition_transaction_record, @@ -1669,7 +1672,11 @@ pub(crate) async fn cleanup_uncommitted_transition_upload( cleanup_version: &str, version_id_exact: bool, ) -> std::io::Result { - delete_object_from_remote_tier_with_lease_idempotent(object, cleanup_version, lease, version_id_exact).await + if version_id_exact { + delete_confirmed_transition_candidate_exact_with_lease_idempotent(object, cleanup_version, lease).await + } else { + delete_object_from_remote_tier_with_lease_idempotent(object, cleanup_version, lease, false).await + } } fn log_transition_upload_cleanup_failure(lease: &TierOperationLease, object: &str, cleanup_version: &str, err: &std::io::Error) { @@ -1806,7 +1813,7 @@ impl Drop for TransitionUploadCleanup { } } -async fn cleanup_rejected_transition_upload_durably( +pub(crate) async fn cleanup_rejected_transition_upload_durably( lease: &TierOperationLease, object: &str, cleanup_version: &str, @@ -1819,6 +1826,13 @@ async fn cleanup_rejected_transition_upload_durably( tier_name: lease.tier_name().to_string(), backend_identity: Some(lease.backend_identity()), version_id_exact, + version_state: if !version_id_exact { + rustfs_filemeta::TransitionVersionState::KnownDisabled + } else if cleanup_version == "null" { + rustfs_filemeta::TransitionVersionState::SuspendedNull + } else { + rustfs_filemeta::TransitionVersionState::Exact + }, }; let journal_error = if let Some(api) = api.as_ref() { @@ -1978,12 +1992,83 @@ async fn advance_and_save_transition_transaction( next: TransitionTransactionState, remote_version: Option, ) -> Result<()> { + #[cfg(test)] + record_transition_uploaded_save_attempt(transaction, next); transaction .advance(transaction.fence(), next, remote_version) .map_err(Error::other)?; save_transition_transaction_if_available(api, transaction).await } +#[cfg(test)] +struct TransitionUploadedSaveProbeState { + bucket: String, + object: String, + attempts: std::sync::atomic::AtomicUsize, +} + +#[cfg(test)] +struct TransitionUploadedSaveProbe { + state: Arc, +} + +#[cfg(test)] +static TRANSITION_UPLOADED_SAVE_PROBE: std::sync::OnceLock>>> = + std::sync::OnceLock::new(); + +#[cfg(test)] +impl TransitionUploadedSaveProbe { + fn install(bucket: &str, object: &str) -> Self { + let state = Arc::new(TransitionUploadedSaveProbeState { + bucket: bucket.to_string(), + object: object.to_string(), + attempts: std::sync::atomic::AtomicUsize::new(0), + }); + let mut slot = TRANSITION_UPLOADED_SAVE_PROBE + .get_or_init(|| std::sync::Mutex::new(None)) + .lock() + .expect("transition uploaded-save probe mutex should not poison"); + assert!(slot.is_none(), "transition uploaded-save probe must be installed by one test at a time"); + *slot = Some(Arc::clone(&state)); + drop(slot); + Self { state } + } + + fn attempts(&self) -> usize { + self.state.attempts.load(std::sync::atomic::Ordering::Acquire) + } +} + +#[cfg(test)] +impl Drop for TransitionUploadedSaveProbe { + fn drop(&mut self) { + let mut slot = TRANSITION_UPLOADED_SAVE_PROBE + .get_or_init(|| std::sync::Mutex::new(None)) + .lock() + .expect("transition uploaded-save probe mutex should not poison"); + if slot.as_ref().is_some_and(|state| Arc::ptr_eq(state, &self.state)) { + *slot = None; + } + } +} + +#[cfg(test)] +fn record_transition_uploaded_save_attempt(transaction: &TransitionTransaction, next: TransitionTransactionState) { + if next != TransitionTransactionState::Uploaded { + return; + } + let state = TRANSITION_UPLOADED_SAVE_PROBE + .get_or_init(|| std::sync::Mutex::new(None)) + .lock() + .expect("transition uploaded-save probe mutex should not poison") + .as_ref() + .filter(|state| state.bucket == transaction.source.bucket && state.object == transaction.source.object) + .cloned(); + if let Some(state) = state { + state.attempts.fetch_add(1, std::sync::atomic::Ordering::AcqRel); + } +} + async fn delete_transition_transaction_if_available(api: Option<&Arc>, transaction_id: Uuid) -> Result<()> { if let Some(api) = api { return delete_transition_transaction_record(api.clone(), transaction_id).await; @@ -2234,11 +2319,25 @@ async fn pause_transition_commit(bucket: &str, object: &str, pause: TransitionCo } } -fn parse_transition_version_id(remote_version: &str) -> std::result::Result, uuid::Error> { +fn persisted_transition_version( + remote_version: &str, +) -> std::io::Result<(Option, rustfs_filemeta::TransitionVersionState)> { if remote_version.is_empty() { - return Ok(None); + return Ok((None, rustfs_filemeta::TransitionVersionState::KnownDisabled)); } - Uuid::parse_str(remote_version).map(|version_id| (!version_id.is_nil()).then_some(version_id)) + 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( + std::io::ErrorKind::InvalidData, + "remote tier returned a nil object version ID", + )); + } + Ok((Some(remote_version.to_string()), rustfs_filemeta::TransitionVersionState::Exact)) } #[cfg(test)] @@ -2476,16 +2575,17 @@ mod transition_upload_completion_tests { #[cfg(test)] mod transition_version_id_tests { - use super::{TransitionUploadCandidate, parse_transition_version_id}; + use super::{TransitionUploadCandidate, persisted_transition_version}; + use rustfs_filemeta::TransitionVersionState; use uuid::Uuid; #[test] fn normalizes_persisted_unversioned_ids_and_preserves_put_constraints() { - assert_eq!(parse_transition_version_id("").expect("empty remote version should be valid"), None); assert_eq!( - parse_transition_version_id(&Uuid::nil().to_string()).expect("nil remote version should be valid"), - None + persisted_transition_version("").expect("empty remote version identifies an unversioned tier"), + (None, TransitionVersionState::KnownDisabled) ); + assert!(persisted_transition_version(&Uuid::nil().to_string()).is_err()); let nil_put_response = Uuid::nil().to_string(); let nil_candidate = TransitionUploadCandidate::from_put_response(nil_put_response.clone()); assert_eq!(nil_candidate.cleanup_version(), nil_put_response); @@ -2497,12 +2597,14 @@ mod transition_version_id_tests { } #[test] - fn preserves_valid_remote_id_and_rejects_invalid_text() { + fn preserves_uuid_and_gates_opaque_remote_ids() { let version_id = Uuid::new_v4(); assert_eq!( - parse_transition_version_id(&version_id.to_string()).expect("UUID remote version should be valid"), - Some(version_id) + persisted_transition_version(&version_id.to_string()).expect("UUID remote version"), + (Some(version_id.to_string()), TransitionVersionState::Exact) ); + assert!(persisted_transition_version("null").is_err()); + assert!(persisted_transition_version("opaque-version-token").is_err()); assert_eq!( TransitionUploadCandidate::from_put_response(version_id.to_string()).cleanup_version(), version_id.to_string() @@ -2511,7 +2613,6 @@ mod transition_version_id_tests { TransitionUploadCandidate::from_put_response("opaque-version-token".to_string()).cleanup_version(), "opaque-version-token" ); - assert!(parse_transition_version_id("not-a-uuid").is_err()); } } @@ -3781,6 +3882,20 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { delete_transition_transaction_after_remote_cleanup(transaction_api.as_ref(), transaction_id, bucket, object).await; return Err(err.into()); } + let (transition_version_id, transition_version_state) = match persisted_transition_version(candidate.remote_version()) { + Ok(version) => version, + Err(err) => { + let cleanup_api = transition_cleanup_store(&self.ctx).await; + if let Err(cleanup_err) = upload_cleanup.cleanup_rejected_upload(cleanup_api).await { + return Err(StorageError::Io(std::io::Error::other(format!( + "{err}; rejected remote upload cleanup failed: {cleanup_err}" + )))); + } + delete_transition_transaction_after_remote_cleanup(transaction_api.as_ref(), transaction_id, bucket, object) + .await; + return Err(err.into()); + } + }; if let Err(err) = advance_and_save_transition_transaction( transaction_api.as_ref(), &mut transaction, @@ -3798,16 +3913,6 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { delete_transition_transaction_after_remote_cleanup(transaction_api.as_ref(), transaction_id, bucket, object).await; return Err(err); } - let transition_version_id = match parse_transition_version_id(candidate.remote_version()) { - Ok(version_id) => version_id, - Err(err) => { - if upload_cleanup.cleanup().await.is_ok() { - delete_transition_transaction_after_remote_cleanup(transaction_api.as_ref(), transaction_id, bucket, object) - .await; - } - return Err(err.into()); - } - }; let mut commit_opts = opts.clone(); commit_opts.no_lock = true; @@ -3865,7 +3970,11 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { current_fi.transition_status = TRANSITION_COMPLETE.to_string(); current_fi.transitioned_objname = dest_obj; current_fi.transition_tier = opts.transition.tier.clone(); - current_fi.transition_version_id = transition_version_id; + current_fi.transition_version_id = transition_version_id + .as_deref() + .and_then(|version_id| Uuid::parse_str(version_id).ok()); + current_fi.transition_version = transition_version_id; + current_fi.transition_version_state = transition_version_state; rustfs_utils::http::metadata_compat::insert_str( &mut current_fi.metadata, rustfs_utils::http::metadata_compat::SUFFIX_TRANSITION_TIER_DESTINATION_ID, @@ -4082,6 +4191,7 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { let mut p_reader = PutObjReader::new(hash_reader); return match self_.clone().put_object(bucket, object, &mut p_reader, &ropts).await { Ok(restored_info) => { + let restored_info = self_.finalize_restore_metadata(bucket, object, &restored_info, &opts).await?; send_event(EventArgs { event_name: EventName::ObjectRestoreCompleted.as_str().to_string(), bucket_name: bucket.to_string(), @@ -4216,6 +4326,7 @@ impl crate::storage_api_contracts::object::ObjectOperations for SetDisks { return set_restore_header_fn(&mut oi, Some(err)).await; } }; + let restored_info = self_.finalize_restore_metadata(bucket, object, &restored_info, opts).await?; send_event(EventArgs { event_name: EventName::ObjectRestoreCompleted.as_str().to_string(), bucket_name: bucket.to_string(), @@ -4674,6 +4785,33 @@ mod transition_commit_failure_tests { } #[tokio::test] + async fn rejected_unsupported_remote_versions_are_cleaned_up() { + for remote_version in ["null", "opaque-version-token"] { + let manager = TierConfigMgr::new(); + let backend = register_mock_tier(&manager, "WARM").await; + let lease = TierConfigMgr::acquire_operation_lease(&manager, "WARM") + .await + .expect("mock tier lease should be available"); + let candidate = TransitionUploadCandidate::from_put_response(remote_version.to_string()); + + persisted_transition_version(candidate.remote_version()).expect_err("unsupported writer version must fail closed"); + cleanup_rejected_transition_upload_durably( + &lease, + "remote/object", + candidate.cleanup_version(), + candidate.cleanup_version_is_exact(), + None, + ) + .await + .expect("rejected remote upload must be cleaned up"); + + assert_eq!( + backend.remove_versions().await, + vec![("remote/object".to_string(), candidate.cleanup_version().to_string())] + ); + } + } + #[serial_test::serial(restore_multipart_failure_point)] async fn multipart_restore_aborts_every_post_create_failure() { let (temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; @@ -6621,6 +6759,45 @@ mod transition_upload_integrity_tests { ); } + #[tokio::test] + #[serial_test::serial] + async fn unversioned_remote_version_is_persisted_without_version_id() { + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; + let bucket = "transition-unversioned-tier-bucket"; + let object = "object.bin"; + let payload = b"unversioned remote tier must commit without a version id".repeat(1024); + let original = write_source(&set_disks, &disk_stores, bucket, object, &payload).await; + let tier_name = format!("COLDTIER{}", &Uuid::new_v4().simple().to_string()[..8]).to_uppercase(); + let backend = register_mock_tier(&runtime_sources::global_tier_config_mgr(), &tier_name).await; + backend.set_put_remote_version(Some(String::new())).await; + let save_probe = TransitionUploadedSaveProbe::install(bucket, object); + + set_disks + .transition_object(bucket, object, &transition_options(&original, tier_name)) + .await + .expect("an unversioned remote version must commit"); + let (fi, _, _) = set_disks + .get_object_fileinfo( + bucket, + object, + &ObjectOptions { + no_lock: true, + metadata_cache_safe: false, + ..Default::default() + }, + true, + false, + ) + .await + .expect("committed unversioned transition metadata should be readable"); + assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version, None); + assert_eq!(fi.transition_version_state, rustfs_filemeta::TransitionVersionState::KnownDisabled); + assert_eq!(save_probe.attempts(), 1); + assert_eq!(backend.remove_count().await, 0); + assert_eq!(backend.object_count().await, 1); + } + #[tokio::test] #[serial_test::serial] async fn opaque_remote_version_is_cleaned_before_parse_failure() { @@ -6636,7 +6813,7 @@ mod transition_upload_integrity_tests { set_disks .transition_object(bucket, object, &transition_options(&original, tier_name)) .await - .expect_err("an unparseable remote version must fail closed"); + .expect_err("an opaque remote version must fail closed until the capability gate is active"); let removed_versions = backend.remove_versions().await; assert_eq!(removed_versions.len(), 1); assert_eq!(removed_versions[0].1, "opaque-version-token"); @@ -6644,6 +6821,38 @@ mod transition_upload_integrity_tests { assert_local_source_intact(&set_disks, bucket, object, &payload).await; } + #[tokio::test] + #[serial_test::serial] + async fn nil_remote_version_is_cleaned_exactly_before_transaction_persistence() { + let (_temp_dirs, disk_stores, set_disks) = hermetic_set_disks(4).await; + let bucket = "transition-nil-version-bucket"; + let object = "object.bin"; + let payload = b"nil remote version must retain local data".repeat(1024); + let original = write_source(&set_disks, &disk_stores, bucket, object, &payload).await; + let tier_name = format!("COLDTIER{}", &Uuid::new_v4().simple().to_string()[..8]).to_uppercase(); + let remote_version = Uuid::nil().to_string(); + let backend = register_mock_tier(&runtime_sources::global_tier_config_mgr(), &tier_name).await; + backend.set_put_remote_version(Some(remote_version.clone())).await; + let save_probe = TransitionUploadedSaveProbe::install(bucket, object); + + set_disks + .transition_object(bucket, object, &transition_options(&original, tier_name)) + .await + .expect_err("a nil remote version must fail closed before transaction persistence"); + let put_versions = backend.put_versions().await; + let removed_versions = backend.remove_versions().await; + assert_eq!(removed_versions, put_versions); + assert_eq!(removed_versions.len(), 1); + assert_eq!( + removed_versions.first().map(|(_, version)| version.as_str()), + Some(remote_version.as_str()) + ); + assert_eq!(save_probe.attempts(), 0, "nil remote version must be rejected before saving Uploaded"); + assert_eq!(backend.exact_remove_count(), 1); + assert_eq!(backend.object_count().await, 0); + assert_local_source_intact(&set_disks, bucket, object, &payload).await; + } + #[tokio::test] #[serial_test::serial] async fn authoritative_read_failure_after_upload_cleans_exact_candidate_and_preserves_source() { @@ -6991,23 +7200,36 @@ mod transition_source_identity_matrix_tests { let object = format!("identity-{index}.bin"); let payload = vec![u8::try_from(index + 1).expect("matrix index should fit u8"); 1024 * 1024]; let mut reader = PutObjReader::from_vec(payload); + let source_version_id = Uuid::new_v4(); + let source_opts = ObjectOptions { + version_id: Some(source_version_id.to_string()), + versioned: true, + ..Default::default() + }; let original = set_disks - .put_object(bucket, &object, &mut reader, &ObjectOptions::default()) + .put_object(bucket, &object, &mut reader, &source_opts) .await .expect("source object should be written"); let (source, _, _) = set_disks - .get_object_fileinfo(bucket, &object, &ObjectOptions::default(), true, false) + .get_object_fileinfo(bucket, &object, &source_opts, true, false) .await .expect("source metadata should resolve"); + assert_eq!(source.version_id, Some(source_version_id)); + assert_eq!( + transition_source_identity(bucket, &object, &source, &source_opts, &get_raw_etag(&source.metadata)) + .expect("persisted versioned source identity should build") + .version_mode, + TransitionSourceVersionMode::Versioned + ); let opts = ObjectOptions { no_lock: true, + versioned: true, transition: TransitionOptions { status: TRANSITION_PENDING.to_string(), tier: tier_name.clone(), etag: original.etag.clone().unwrap_or_default(), ..Default::default() }, - version_id: original.version_id.map(|version| version.to_string()), mod_time: original.mod_time, ..Default::default() }; @@ -7020,7 +7242,10 @@ mod transition_source_identity_matrix_tests { let mut changed = source.clone(); match field { - IdentityField::VersionId => changed.version_id = Some(Uuid::new_v4()), + IdentityField::VersionId => { + changed.version_id = Some(Uuid::new_v4()); + changed.fresh = true; + } IdentityField::DataDir => changed.data_dir = Some(Uuid::new_v4()), IdentityField::ModTime => { changed.mod_time = changed.mod_time.map(|value| value + time::Duration::nanoseconds(1)); @@ -7038,12 +7263,19 @@ mod transition_source_identity_matrix_tests { .await .expect("single-field metadata drift should be written"); } + let persisted_opts = ObjectOptions { + version_id: changed.version_id.map(|version_id| version_id.to_string()), + versioned: true, + ..Default::default() + }; + let (persisted, _, _) = set_disks + .get_object_fileinfo(bucket, &object, &persisted_opts, true, false) + .await + .expect("drifted source metadata should resolve"); put_barrier.release(); - transition - .await - .expect("transition task should not panic") - .expect_err("transition must reject a source whose identity changed after upload"); + let result = transition.await.expect("transition task should not panic"); + assert!(result.is_err(), "transition must reject {field:?} drift"); let expected_attempts = index + 1; assert_eq!(backend.put_count().await, expected_attempts); assert_eq!(backend.remove_count().await, expected_attempts); @@ -7054,26 +7286,26 @@ mod transition_source_identity_matrix_tests { ); match field { - IdentityField::VersionId => assert_ne!(source.version_id, changed.version_id), - IdentityField::DataDir => assert_ne!(source.data_dir, changed.data_dir), - IdentityField::ModTime => assert_ne!(source.mod_time, changed.mod_time), - IdentityField::Size => assert_ne!(source.size, changed.size), - IdentityField::Etag => assert_ne!(get_raw_etag(&source.metadata), get_raw_etag(&changed.metadata)), + IdentityField::VersionId => assert_ne!(source.version_id, persisted.version_id), + IdentityField::DataDir => assert_ne!(source.data_dir, persisted.data_dir), + IdentityField::ModTime => assert_ne!(source.mod_time, persisted.mod_time), + IdentityField::Size => assert_ne!(source.size, persisted.size), + IdentityField::Etag => assert_ne!(get_raw_etag(&source.metadata), get_raw_etag(&persisted.metadata)), } if !matches!(field, IdentityField::VersionId) { - assert_eq!(source.version_id, changed.version_id); + assert_eq!(source.version_id, persisted.version_id); } if !matches!(field, IdentityField::DataDir) { - assert_eq!(source.data_dir, changed.data_dir); + assert_eq!(source.data_dir, persisted.data_dir); } if !matches!(field, IdentityField::ModTime) { - assert_eq!(source.mod_time, changed.mod_time); + assert_eq!(source.mod_time, persisted.mod_time); } if !matches!(field, IdentityField::Size) { - assert_eq!(source.size, changed.size); + assert_eq!(source.size, persisted.size); } if !matches!(field, IdentityField::Etag) { - assert_eq!(get_raw_etag(&source.metadata), get_raw_etag(&changed.metadata)); + assert_eq!(get_raw_etag(&source.metadata), get_raw_etag(&persisted.metadata)); } } } diff --git a/crates/ecstore/src/set_disk/replication.rs b/crates/ecstore/src/set_disk/replication.rs index 611abeb7a..ad84d4842 100644 --- a/crates/ecstore/src/set_disk/replication.rs +++ b/crates/ecstore/src/set_disk/replication.rs @@ -13,7 +13,10 @@ // limitations under the License. use super::*; +use crate::bucket::lifecycle::lifecycle; +use rustfs_filemeta::RestoreStatusOps; use rustfs_utils::http::headers::{AMZ_RESTORE_EXPIRY_DAYS, AMZ_RESTORE_REQUEST_DATE}; +use s3s::dto::{RestoreStatus, Timestamp}; #[derive(Clone, Copy, Debug, Eq, PartialEq)] struct RestoreCleanupIdentity { @@ -43,6 +46,69 @@ impl RestoreCleanupIdentity { } impl SetDisks { + pub(super) async fn finalize_restore_metadata( + &self, + bucket: &str, + object: &str, + obj_info: &ObjectInfo, + opts: &ObjectOptions, + ) -> Result { + let expected = RestoreCleanupIdentity::from_object_info(obj_info); + let expected_operation_id = restore_operation_id_from_metadata(&opts.user_defined)?; + let expected_etag = obj_info + .etag + .clone() + .unwrap_or_else(|| get_raw_etag(obj_info.user_defined.as_ref())); + let version_id = expected.version_id.map(|v| v.to_string()); + let _lock_guard = if !opts.no_lock { + Some( + self.acquire_write_lock_diag("restore_finalize_metadata", bucket, object) + .await?, + ) + } else { + None + }; + let read_opts = ObjectOptions { + version_id, + versioned: opts.versioned, + version_suspended: opts.version_suspended, + ..Default::default() + }; + let (mut fi, _, disks) = self + .get_object_fileinfo_gated(bucket, object, &read_opts, false, false) + .await?; + if let Some(expected_operation_id) = expected_operation_id { + require_restore_operation_id(&fi.metadata, expected_operation_id)?; + } + if !expected.matches_file_info(&fi, &expected_etag) { + return Err(Error::other("restored object changed before restore metadata finalization")); + } + let restore_expiry = + lifecycle::expected_expiry_time(OffsetDateTime::now_utc(), opts.transition.restore_request.days.unwrap_or(1)); + fi.metadata.insert( + X_AMZ_RESTORE.as_str().to_string(), + RestoreStatus { + is_restore_in_progress: Some(false), + restore_expiry_date: Some(Timestamp::from(restore_expiry)), + } + .to_string(), + ); + self.invalidate_get_object_metadata_cache(bucket, object).await; + self.update_object_meta_with_opts( + bucket, + object, + fi.clone(), + disks.as_slice(), + &UpdateMetadataOpts { + replace_user_metadata: true, + ..Default::default() + }, + ) + .await?; + self.invalidate_get_object_metadata_cache(bucket, object).await; + Ok(ObjectInfo::from_file_info(&fi, bucket, object, opts.versioned || opts.version_suspended)) + } + pub async fn update_restore_metadata( &self, bucket: &str, diff --git a/crates/ecstore/src/set_disk/transition_matrix_tests.rs b/crates/ecstore/src/set_disk/transition_matrix_tests.rs index e77a0e437..a137d55a6 100644 --- a/crates/ecstore/src/set_disk/transition_matrix_tests.rs +++ b/crates/ecstore/src/set_disk/transition_matrix_tests.rs @@ -13,10 +13,11 @@ // limitations under the License. use super::*; -use crate::bucket::lifecycle::lifecycle::{TRANSITION_COMPLETE, TRANSITION_PENDING, TransitionOptions}; +use crate::bucket::lifecycle::lifecycle::{TRANSITION_COMPLETE, TRANSITION_PENDING, TransitionOptions, expected_expiry_time}; use crate::ecstore_validation_blackbox::make_local_set_disks; use crate::services::tier::test_util::register_mock_tier; use crate::storage_api_contracts::object::{ObjectIO as _, ObjectOperations as _}; +use rustfs_filemeta::{RestoreStatusOps as _, parse_restore_obj_status}; use tokio::io::AsyncReadExt; async fn prime_metadata_generation(set_disks: &SetDisks, bucket: &str, object: &str) -> GetObjectMetadataCacheKey { @@ -83,13 +84,57 @@ async fn transition_and_restore_reclaim_prior_metadata_generations() { let transitioned_generation = prime_metadata_generation(&set_disks, bucket, object).await; let mut restore_opts = ObjectOptions::default(); restore_opts.transition.restore_request.days = Some(1); - Arc::clone(&set_disks) - .restore_transitioned_object(bucket, object, &restore_opts) - .await - .expect("restore should succeed"); + let restore_started = OffsetDateTime::now_utc(); + let expiry_from_restore_start = temp_env::async_with_vars( + [ + ("RUSTFS_ILM_DEBUG_DAY_SECS", Some("1")), + ("RUSTFS_ILM_PROCESS_TIME", Some("1")), + ], + async { + let expiry_from_restore_start = expected_expiry_time(restore_started, 1); + let get_barrier = backend.arm_get_barrier().await; + let restore_set = Arc::clone(&set_disks); + let restore = + tokio::spawn(async move { restore_set.restore_transitioned_object(bucket, object, &restore_opts).await }); + get_barrier.wait_until_paused().await; + tokio::time::timeout(Duration::from_secs(5), async { + loop { + if expected_expiry_time(OffsetDateTime::now_utc(), 1) > expiry_from_restore_start { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("test clock should cross the next accelerated lifecycle boundary"); + get_barrier.release(); + restore + .await + .expect("restore task should join") + .expect("restore should succeed"); + expiry_from_restore_start + }, + ) + .await; assert_generation_reclaimed(&set_disks, &transitioned_generation).await; assert_eq!(backend.get_count().await, 1, "restore should read the remote candidate exactly once"); + let restored_info = set_disks + .get_object_info(bucket, object, &ObjectOptions::default()) + .await + .expect("restored object metadata should be readable"); + let restore_status = parse_restore_obj_status( + restored_info + .user_defined + .get(s3s::header::X_AMZ_RESTORE.as_str()) + .expect("completed restore header should be present"), + ) + .expect("completed restore header should parse"); + assert!( + restore_status.expiry().expect("completed restore should have an expiry") > expiry_from_restore_start, + "restore expiry must be based on completion, not the time the remote copy started" + ); + let mut restored = Vec::new(); set_disks .get_object_reader(bucket, object, None, HeaderMap::new(), &ObjectOptions::default()) diff --git a/crates/ecstore/src/store/init.rs b/crates/ecstore/src/store/init.rs index 5c410839e..1ce3bbd7f 100644 --- a/crates/ecstore/src/store/init.rs +++ b/crates/ecstore/src/store/init.rs @@ -1311,14 +1311,16 @@ mod tests { version_id: "version-a".to_string(), tier_name: tier_a.to_string(), backend_identity: Some(identity_a), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; let entry_b = Jentry { obj_name: "remote-b".to_string(), version_id: "version-b".to_string(), tier_name: tier_b.to_string(), backend_identity: Some(identity_b), - version_id_exact: false, + version_id_exact: true, + version_state: rustfs_filemeta::TransitionVersionState::Exact, }; let remove_a = backend_a.arm_failing_remove_barrier().await; persist_tier_delete_journal_entry(store_a.clone(), &entry_a) @@ -2720,10 +2722,12 @@ mod tests { #[serial_test::serial(storage_class_env)] async fn transition_transaction_recovery_deletes_provider_recovered_unknown_upload() { let versioned_remote = uuid::Uuid::new_v4().to_string(); + let nil_remote = uuid::Uuid::nil().to_string(); for (case, tier_name, remote_version) in [ ("missing", "TXPROBEMISSING", None), ("unversioned", "TXPROBEUNVERSIONED", Some(String::new())), ("versioned", "TXPROBEVERSIONED", Some(versioned_remote)), + ("nil-version", "TXPROBENILVERSION", Some(nil_remote)), ] { let temp_dir = tempfile::tempdir().expect("create temp store dir"); let (ctx, store, _shutdown) = without_storage_class_env(build_isolated_test_store( diff --git a/crates/ecstore/src/store/object.rs b/crates/ecstore/src/store/object.rs index 2a8d30ee4..159fbafa3 100644 --- a/crates/ecstore/src/store/object.rs +++ b/crates/ecstore/src/store/object.rs @@ -2601,10 +2601,10 @@ mod tests { // (backlog#1304): restore entry no longer serializes on the object lock. // The replacement semantics — non-blocking reads during the copy-back and // fast rejection of a concurrent restore — are covered end-to-end by - // `restore_object_usecase_reports_ongoing_conflict_and_completion` - // (rustfs/src/app/lifecycle_transition_api_test.rs) and at the lock level - // by the accept-guard test below; restore-vs-reader data protection lives - // in the inner put_object/complete_multipart_upload commit locks. + // `restore_object_usecase_reports_ongoing_conflict` + // (rustfs/src/app/lifecycle_transition_api_test.rs), while the SetDisks + // transition matrix covers the final local commit. Restore-vs-reader data + // protection lives in the inner put_object/complete_multipart_upload locks. #[tokio::test] #[serial_test::serial] async fn restore_accept_guard_serializes_concurrent_accepts() { diff --git a/crates/filemeta/src/fileinfo.rs b/crates/filemeta/src/fileinfo.rs index 2f1714e01..e1f1f2e1b 100644 --- a/crates/filemeta/src/fileinfo.rs +++ b/crates/filemeta/src/fileinfo.rs @@ -219,6 +219,16 @@ impl ErasureInfo { } // #[derive(Debug, Clone)] +#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Copy, Default)] +#[serde(rename_all = "kebab-case")] +pub enum TransitionVersionState { + #[default] + Unknown, + KnownDisabled, + SuspendedNull, + Exact, +} + #[derive(Serialize, Deserialize, Debug, PartialEq, Clone, Default)] pub struct FileInfo { pub volume: String, @@ -230,6 +240,10 @@ pub struct FileInfo { pub transitioned_objname: String, pub transition_tier: String, pub transition_version_id: Option, + #[serde(default)] + pub transition_version: Option, + #[serde(default)] + pub transition_version_state: TransitionVersionState, pub expire_restored: bool, pub data_dir: Option, pub mod_time: Option, @@ -459,6 +473,10 @@ impl FileInfo { if self.mod_time.is_none_or(|mod_time| mod_time <= OffsetDateTime::UNIX_EPOCH) || (!allow_nil_version_id && self.version_id.is_some_and(|version_id| version_id.is_nil())) || self.transition_version_id.is_some_and(|version_id| version_id.is_nil()) + || self + .transition_version + .as_ref() + .is_some_and(|version_id| version_id.is_empty()) || self.size != 0 || self.data_dir.is_some() || self.mode.is_some() @@ -492,6 +510,7 @@ impl FileInfo { || !self.transitioned_objname.is_empty() || !self.transition_tier.is_empty() || self.transition_version_id.is_some() + || self.transition_version.is_some() || self.expire_restored || self.size != 0 || self.data_dir.is_some() @@ -536,6 +555,25 @@ impl FileInfo { /// return `None`. pub fn validate(&self, mode: ValidationMode) -> Result> { self.validate_collection_bounds()?; + if let (Some(version), Some(version_id)) = (&self.transition_version, self.transition_version_id) + && Uuid::parse_str(version).ok() != Some(version_id) + { + return Err(Error::FileCorrupt); + } + let transition_state_valid = match self.transition_version_state { + TransitionVersionState::Unknown => true, + TransitionVersionState::KnownDisabled => self.transition_version.is_none() && self.transition_version_id.is_none(), + TransitionVersionState::SuspendedNull => { + self.transition_version.as_deref() == Some("null") && self.transition_version_id.is_none() + } + TransitionVersionState::Exact => self + .transition_version + .as_deref() + .is_some_and(|version| version != "null" && !version.is_empty()), + }; + if !transition_state_valid { + return Err(Error::FileCorrupt); + } let erasure_layout = match mode { ValidationMode::RequireErasure => Some(self.validate_erasure_geometry()?), @@ -832,6 +870,8 @@ impl FileInfo { && self.transition_tier == other.transition_tier && self.transitioned_objname == other.transitioned_objname && self.transition_version_id == other.transition_version_id + && self.transition_version == other.transition_version + && self.transition_version_state == other.transition_version_state } /// Check if metadata maps are equal @@ -1351,6 +1391,15 @@ mod tests { assert_file_corrupt(&fi, ValidationMode::DeleteOnly); } + #[test] + fn metadata_read_validation_rejects_conflicting_transition_versions() { + let mut fi = one_shard_validation_fileinfo(1); + fi.transition_version_id = Some(Uuid::new_v4()); + fi.transition_version = Some(Uuid::new_v4().to_string()); + + assert_file_corrupt(&fi, ValidationMode::RequireErasure); + } + #[test] fn metadata_read_validation_requires_canonical_delete_marker_shape() { let marker = FileInfo { @@ -1722,6 +1771,12 @@ mod tests { transitioned_objname, transition_tier, transition_version_id, + transition_version: transition_version_id.map(|version_id| version_id.to_string()), + transition_version_state: if transition_version_id.is_some() { + TransitionVersionState::Exact + } else { + TransitionVersionState::Unknown + }, expire_restored, data_dir, mod_time, diff --git a/crates/filemeta/src/filemeta/version.rs b/crates/filemeta/src/filemeta/version.rs index f491e75b9..091bfb527 100644 --- a/crates/filemeta/src/filemeta/version.rs +++ b/crates/filemeta/src/filemeta/version.rs @@ -26,13 +26,14 @@ use super::msgp_decode::{ PrependByteReader, prealloc_hint, read_exact_vec, read_nil_or_array_len, read_nil_or_map_len, skip_msgp_value, }; use super::*; -use crate::ChecksumInfo; +use crate::{ChecksumInfo, TransitionVersionState}; use rustfs_utils::HashAlgorithm; use rustfs_utils::http::{ RUSTFS_INTERNAL_PREFIX, SUFFIX_CRC, SUFFIX_FREE_VERSION, SUFFIX_INLINE_DATA, SUFFIX_PURGESTATUS, SUFFIX_TIER_FV_ID, SUFFIX_TIER_FV_MARKER, SUFFIX_TRANSITION_STATUS, SUFFIX_TRANSITION_TIER, SUFFIX_TRANSITION_TIER_DESTINATION_ID, - SUFFIX_TRANSITIONED_OBJECTNAME, SUFFIX_TRANSITIONED_VERSION_ID, contains_key_bytes, get_bytes, get_consistent_bytes, get_str, - has_internal_suffix, insert_bytes, is_internal_key, remove_bytes, strip_internal_prefix, + SUFFIX_TRANSITIONED_OBJECTNAME, SUFFIX_TRANSITIONED_VERSION_ID, SUFFIX_TRANSITIONED_VERSION_STATE, contains_key_bytes, + get_bytes, get_consistent_bytes, get_str, has_internal_suffix, insert_bytes, is_internal_key, remove_bytes, + strip_internal_prefix, }; const MSGPACK_EXT8: u8 = 0xc7; @@ -43,6 +44,7 @@ const MSGPACK_FIXEXT8: u8 = 0xd7; const MSGPACK_TIME_EXT_LEGACY: i8 = 5; const MSGPACK_TIME_EXT_OFFICIAL: i8 = -1; const MSGPACK_TIME_LEN: u8 = 12; +const MAX_TRANSITION_VERSION_LEN: usize = 1024; /// Sentinel signature returned when a version has no computable body (invalid / /// missing inner object). Mirrors MinIO's `signatureErr` so such versions never @@ -251,23 +253,93 @@ fn parse_legacy_uuid_bytes(bytes: &[u8], field: &str) -> Result> { /// Decode a stored transitioned-version-id from a version's `meta_sys`. /// -/// RustFS writes it as 16 raw UUID bytes; MinIO-migrated tiered objects store -/// the remote tier's version id as a UUID *string*. Accept both, and treat any -/// absent / nil / otherwise-unparseable value as "no tier version" (matching the -/// tolerant pre-hardening behavior) rather than failing the whole object read — -/// a malformed tier id must not make an otherwise-readable object unreadable. -fn transitioned_version_id_from_meta_sys(meta_sys: &HashMap>) -> Option { - let value = get_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_ID)?; +/// Legacy RustFS writes used 16 raw UUID bytes. New writes and MinIO-migrated +/// records use the provider's exact UTF-8 version text. Empty, nil UUID, and +/// malformed bytes are not usable remote versions. +fn transitioned_version_from_meta_sys(meta_sys: &HashMap>) -> Result> { + if !contains_key_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_ID) { + return Ok(None); + } + let Some(value) = get_consistent_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_ID) else { + return Ok(None); + }; + let value = value.to_vec(); if value.is_empty() { - return None; + return Ok(None); } if let Ok(id) = Uuid::from_slice(&value) { - return (!id.is_nil()).then_some(id); + return Ok((!id.is_nil()).then(|| id.to_string())); } - std::str::from_utf8(&value) + let Ok(value) = String::from_utf8(value) else { + return Ok(None); + }; + if value.is_empty() + || value.len() > MAX_TRANSITION_VERSION_LEN + || value.chars().any(char::is_control) + || Uuid::parse_str(&value).is_ok_and(|id| id.is_nil()) + { + Ok(None) + } else { + Ok(Some(value)) + } +} + +fn transition_version_state_from_meta_sys( + meta_sys: &HashMap>, + version: Option<&str>, +) -> Result { + if !contains_key_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_STATE) { + return Ok(TransitionVersionState::Unknown); + } + let value = get_consistent_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_STATE).ok_or(Error::FileCorrupt)?; + let state = match value { + b"known-disabled" => TransitionVersionState::KnownDisabled, + b"suspended-null" => TransitionVersionState::SuspendedNull, + b"exact" => TransitionVersionState::Exact, + b"unknown" => TransitionVersionState::Unknown, + _ => return Err(Error::FileCorrupt), + }; + let valid = match state { + TransitionVersionState::Unknown | TransitionVersionState::KnownDisabled => version.is_none(), + TransitionVersionState::SuspendedNull => version == Some("null"), + TransitionVersionState::Exact => version.is_some_and(|value| value != "null"), + }; + valid.then_some(state).ok_or(Error::FileCorrupt) +} + +fn transition_version_state_bytes(state: TransitionVersionState) -> &'static [u8] { + match state { + TransitionVersionState::Unknown => b"unknown", + TransitionVersionState::KnownDisabled => b"known-disabled", + TransitionVersionState::SuspendedNull => b"suspended-null", + TransitionVersionState::Exact => b"exact", + } +} + +fn set_transition_version_state(meta_sys: &mut HashMap>, state: TransitionVersionState) { + if state == TransitionVersionState::Unknown { + remove_bytes(meta_sys, SUFFIX_TRANSITIONED_VERSION_STATE); + } else { + insert_bytes( + meta_sys, + SUFFIX_TRANSITIONED_VERSION_STATE, + transition_version_state_bytes(state).to_vec(), + ); + } +} + +fn legacy_transitioned_version_id_from_meta_sys(meta_sys: &HashMap>) -> Option { + transitioned_version_from_meta_sys(meta_sys) .ok() - .and_then(|s| Uuid::parse_str(s.trim()).ok()) - .filter(|id| !id.is_nil()) + .flatten() + .and_then(|value| Uuid::parse_str(&value).ok()) +} + +fn transitioned_version_bytes(fi: &FileInfo) -> Option> { + fi.transition_version + .as_ref() + .map(|version| version.as_bytes().to_vec()) + .or_else(|| fi.transition_version_id.map(|version_id| version_id.as_bytes().to_vec())) } fn parse_legacy_erasure_algo(value: &str) -> ErasureAlgo { @@ -2398,7 +2470,9 @@ impl MetaObject { let transitioned_objname = get_bytes(&self.meta_sys, SUFFIX_TRANSITIONED_OBJECTNAME) .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); - let transition_version_id = transitioned_version_id_from_meta_sys(&self.meta_sys); + let transition_version = transitioned_version_from_meta_sys(&self.meta_sys)?; + let transition_version_state = transition_version_state_from_meta_sys(&self.meta_sys, transition_version.as_deref())?; + let transition_version_id = transition_version.as_deref().and_then(|value| Uuid::parse_str(value).ok()); let transition_tier = get_bytes(&self.meta_sys, SUFFIX_TRANSITION_TIER) .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); @@ -2419,6 +2493,8 @@ impl MetaObject { transition_status, transitioned_objname, transition_version_id, + transition_version, + transition_version_state, transition_tier, ..Default::default() }) @@ -2431,13 +2507,12 @@ impl MetaObject { SUFFIX_TRANSITIONED_OBJECTNAME, fi.transitioned_objname.as_bytes().to_vec(), ); - if let Some(transition_version_id) = fi.transition_version_id.as_ref() { - insert_bytes( - &mut self.meta_sys, - SUFFIX_TRANSITIONED_VERSION_ID, - transition_version_id.as_bytes().to_vec(), - ); + if let Some(transition_version) = transitioned_version_bytes(fi) { + insert_bytes(&mut self.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, transition_version); + } else { + remove_bytes(&mut self.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID); } + set_transition_version_state(&mut self.meta_sys, fi.transition_version_state); insert_bytes(&mut self.meta_sys, SUFFIX_TRANSITION_TIER, fi.transition_tier.as_bytes().to_vec()); if let Some(destination_id) = get_str(&fi.metadata, SUFFIX_TRANSITION_TIER_DESTINATION_ID) { insert_bytes(&mut self.meta_sys, SUFFIX_TRANSITION_TIER_DESTINATION_ID, destination_id.into_bytes()); @@ -2501,6 +2576,7 @@ impl MetaObject { SUFFIX_TRANSITION_TIER, SUFFIX_TRANSITIONED_OBJECTNAME, SUFFIX_TRANSITIONED_VERSION_ID, + SUFFIX_TRANSITIONED_VERSION_STATE, ] { if let Some(v) = get_bytes(&self.meta_sys, suffix) { insert_bytes(&mut delete_marker.meta_sys, suffix, v); @@ -2562,8 +2638,11 @@ impl From for MetaObject { ); } - if let Some(vid) = &value.transition_version_id { - insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, vid.as_bytes().to_vec()); + if let Some(transition_version) = transitioned_version_bytes(&value) { + insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, transition_version); + } + if !value.transition_status.is_empty() { + set_transition_version_state(&mut meta_sys, value.transition_version_state); } if !value.transition_tier.is_empty() { @@ -2706,7 +2785,11 @@ impl MetaDeleteMarker { .map(|v| String::from_utf8_lossy(&v).to_string()) .unwrap_or_default(); - fi.transition_version_id = transitioned_version_id_from_meta_sys(&self.meta_sys); + fi.transition_version = transitioned_version_from_meta_sys(&self.meta_sys).ok().flatten(); + fi.transition_version_id = legacy_transitioned_version_id_from_meta_sys(&self.meta_sys); + fi.transition_version_state = + transition_version_state_from_meta_sys(&self.meta_sys, fi.transition_version.as_deref()) + .unwrap_or(TransitionVersionState::Unknown); } fi @@ -2859,8 +2942,11 @@ impl From for MetaDeleteMarker { value.transitioned_objname.as_bytes().to_vec(), ); } - if let Some(version_id) = value.transition_version_id { - insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, version_id.as_bytes().to_vec()); + if let Some(transition_version) = transitioned_version_bytes(&value) { + insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, transition_version); + } + if !value.transition_status.is_empty() || value.tier_free_version() { + set_transition_version_state(&mut meta_sys, value.transition_version_state); } if !value.transition_tier.is_empty() { insert_bytes(&mut meta_sys, SUFFIX_TRANSITION_TIER, value.transition_tier.as_bytes().to_vec()); @@ -3412,7 +3498,7 @@ mod tests { .insert("x-rustfs-internal-healing".to_string(), "true".to_string()); marker.metadata.insert("content-type".to_string(), "text/plain".to_string()); let remote_version_id = Uuid::new_v4(); - marker.transition_version_id = Some(remote_version_id); + marker.transition_version = Some(remote_version_id.to_string()); let converted = MetaDeleteMarker::from(marker); @@ -3420,7 +3506,19 @@ mod tests { assert_eq!(converted.meta_sys.get("x-minio-internal-purgestatus"), Some(&b"pending".to_vec())); assert_eq!( get_bytes(&converted.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID), - Some(remote_version_id.as_bytes().to_vec()) + Some(remote_version_id.to_string().into_bytes()) + ); + assert_eq!( + converted + .meta_sys + .get(&format!("{RUSTFS_INTERNAL_PREFIX}{SUFFIX_TRANSITIONED_VERSION_ID}")), + Some(&remote_version_id.to_string().into_bytes()) + ); + assert_eq!( + converted + .meta_sys + .get(&format!("{}{SUFFIX_TRANSITIONED_VERSION_ID}", rustfs_utils::http::MINIO_INTERNAL_PREFIX)), + Some(&remote_version_id.to_string().into_bytes()) ); assert!(!converted.meta_sys.contains_key("x-rustfs-internal-healing")); assert!(!converted.meta_sys.contains_key("content-type")); @@ -4097,19 +4195,129 @@ mod tests { .into_fileinfo("b", "k", false) .expect("into_fileinfo"); assert_eq!(fi.transition_version_id, Some(id)); + assert_eq!(fi.transition_version, Some(id.to_string())); + assert_eq!(fi.transition_version_state, TransitionVersionState::Unknown); } #[test] - fn meta_object_transition_version_id_unparseable_stays_readable_as_none() { - // A non-UUID / non-16-byte tier version id must NOT make the object - // unreadable; it is tolerated as "no tier version" (compat with - // pre-hardening behavior and foreign/edge metadata). + fn meta_object_transition_version_id_opaque_text_is_preserved() { let mut sys = HashMap::new(); - insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"not-a-uuid".to_vec()); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"opaque-generation-42".to_vec()); let fi = make_meta_object_with_sys(sys) .into_fileinfo("b", "k", false) - .expect("unparseable transition version id must not fail the object read"); + .expect("opaque transition version id must decode"); assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version.as_deref(), Some("opaque-generation-42")); + assert_eq!(fi.transition_version_state, TransitionVersionState::Unknown); + } + + #[test] + fn meta_object_transition_version_state_exact_round_trips_dual_keys() { + let id = sample_version_id(); + let expected_version = id.to_string(); + let fi = FileInfo { + transition_status: "complete".to_string(), + transition_version: Some(expected_version.clone()), + transition_version_state: TransitionVersionState::Exact, + ..Default::default() + }; + + let object = MetaObject::from(fi); + assert_eq!( + object + .meta_sys + .get(&format!("{RUSTFS_INTERNAL_PREFIX}{SUFFIX_TRANSITIONED_VERSION_STATE}")) + .map(Vec::as_slice), + Some(b"exact".as_slice()) + ); + assert_eq!( + object + .meta_sys + .get(&format!( + "{}{SUFFIX_TRANSITIONED_VERSION_STATE}", + rustfs_utils::http::MINIO_INTERNAL_PREFIX + )) + .map(Vec::as_slice), + Some(b"exact".as_slice()) + ); + assert_eq!( + legacy_transitioned_version_id_from_meta_sys(&object.meta_sys), + Some(id), + "UUID exact writes must remain readable by the legacy UUID consumer" + ); + let decoded = object.into_fileinfo("b", "k", false).expect("exact state should round trip"); + assert_eq!(decoded.transition_version_state, TransitionVersionState::Exact); + assert_eq!(decoded.transition_version.as_deref(), Some(expected_version.as_str())); + } + + #[test] + fn set_transition_known_disabled_removes_stale_version_dual_keys() { + let mut meta_sys = HashMap::new(); + insert_bytes(&mut meta_sys, SUFFIX_TRANSITIONED_VERSION_ID, b"stale-legacy-version".to_vec()); + let mut object = make_meta_object_with_sys(meta_sys); + object.set_transition(&FileInfo { + transition_status: TRANSITION_COMPLETE.to_string(), + transitioned_objname: "remote/object".to_string(), + transition_version_state: TransitionVersionState::KnownDisabled, + transition_tier: "WARM".to_string(), + ..Default::default() + }); + + assert_eq!(get_bytes(&object.meta_sys, SUFFIX_TRANSITIONED_VERSION_ID), None); + assert!( + !object + .meta_sys + .contains_key(&format!("{RUSTFS_INTERNAL_PREFIX}{SUFFIX_TRANSITIONED_VERSION_ID}")) + ); + assert!( + !object + .meta_sys + .contains_key(&format!("{}{SUFFIX_TRANSITIONED_VERSION_ID}", rustfs_utils::http::MINIO_INTERNAL_PREFIX)) + ); + let decoded = object + .into_fileinfo("b", "k", false) + .expect("known-disabled transition must remain readable after replacing stale metadata"); + assert_eq!(decoded.transition_version, None); + assert_eq!(decoded.transition_version_state, TransitionVersionState::KnownDisabled); + } + + #[test] + fn meta_object_transition_version_state_conflict_fails_closed() { + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, sample_version_id().as_bytes().to_vec()); + sys.insert(format!("{RUSTFS_INTERNAL_PREFIX}{SUFFIX_TRANSITIONED_VERSION_STATE}"), b"exact".to_vec()); + sys.insert( + format!("{}{SUFFIX_TRANSITIONED_VERSION_STATE}", rustfs_utils::http::MINIO_INTERNAL_PREFIX), + b"known-disabled".to_vec(), + ); + + make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect_err("conflicting state keys must fail closed"); + } + + #[test] + fn meta_object_transition_version_id_invalid_utf8_yields_none() { + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, vec![0xff]); + let fi = make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect("invalid transition version bytes must not fail the object read"); + assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version, None); + } + + #[test] + fn meta_object_transition_version_id_unsafe_text_yields_none() { + for value in [b"opaque\0version".to_vec(), vec![b'x'; MAX_TRANSITION_VERSION_LEN + 1]] { + let mut sys = HashMap::new(); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, value); + let fi = make_meta_object_with_sys(sys) + .into_fileinfo("b", "k", false) + .expect("unsafe transition version text must not fail the object read"); + assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version, None); + } } #[test] @@ -4123,6 +4331,7 @@ mod tests { .into_fileinfo("b", "k", false) .expect("string-form transition version id must decode"); assert_eq!(fi.transition_version_id, Some(id)); + assert_eq!(fi.transition_version, Some(id.to_string())); } #[test] @@ -4152,16 +4361,14 @@ mod tests { } .into_fileinfo("b", "k", false); assert_eq!(fi.transition_version_id, Some(id)); + assert_eq!(fi.transition_version, Some(id.to_string())); } #[test] - fn delete_marker_free_version_transition_version_id_unparseable_stays_readable() { - // A malformed tier version id must not make a free-version record corrupt: - // it decodes to None and stays readable. Otherwise free-version expiry - // fails and the remote-tier object leaks. + fn delete_marker_free_version_transition_version_id_opaque_text_is_preserved() { let mut sys = HashMap::new(); insert_bytes(&mut sys, SUFFIX_FREE_VERSION, vec![]); - insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"not-a-uuid".to_vec()); + insert_bytes(&mut sys, SUFFIX_TRANSITIONED_VERSION_ID, b"opaque-generation-42".to_vec()); insert_bytes(&mut sys, SUFFIX_TRANSITION_TIER, b"WARM".to_vec()); insert_bytes(&mut sys, SUFFIX_TRANSITIONED_OBJECTNAME, b"remote-object".to_vec()); let fi = MetaDeleteMarker { @@ -4172,8 +4379,9 @@ mod tests { .into_fileinfo("b", "k", false); assert_eq!(fi.transition_version_id, None); + assert_eq!(fi.transition_version.as_deref(), Some("opaque-generation-42")); fi.validate_for_metadata_read() - .expect("free-version record with an unparseable tier id must remain readable"); + .expect("free-version record with an opaque tier id must remain readable"); } #[test] @@ -4193,6 +4401,7 @@ mod tests { .into_fileinfo("b", "k", false); assert_eq!(fi.transition_version_id, Some(id)); + assert_eq!(fi.transition_version, Some(id.to_string())); } #[test] diff --git a/crates/protocols/Cargo.toml b/crates/protocols/Cargo.toml index d9ea48b41..003e4c4af 100644 --- a/crates/protocols/Cargo.toml +++ b/crates/protocols/Cargo.toml @@ -134,6 +134,7 @@ socket2 = { workspace = true, optional = true, features = ["all"] } [dev-dependencies] tempfile = { workspace = true } proptest = "1" +rustfs-test-utils = { workspace = true } tracing-subscriber = { workspace = true, features = ["env-filter", "time"] } tokio = { workspace = true, features = ["test-util", "macros", "fs"] } diff --git a/crates/protocols/src/swift/account.rs b/crates/protocols/src/swift/account.rs index e76d4c850..cf3aa538c 100644 --- a/crates/protocols/src/swift/account.rs +++ b/crates/protocols/src/swift/account.rs @@ -16,12 +16,11 @@ use super::storage_api::account::{BucketOperations, MakeBucketOptions}; use super::{SwiftError, SwiftResult}; -use super::{get_swift_bucket_metadata, resolve_swift_object_store_handle, set_swift_bucket_metadata}; +use super::{get_swift_bucket_metadata, resolve_swift_object_store_handle, update_swift_bucket_tagging, validate_metadata}; use rustfs_credentials::Credentials; use s3s::dto::{Tag, Tagging}; use sha2::{Digest, Sha256}; use std::collections::HashMap; -use time; /// Validate that the authenticated user has access to the requested account /// @@ -148,15 +147,33 @@ pub async fn get_account_metadata(account: &str, _credentials: &Option, - _credentials: &Option, + credentials: &Option, ) -> SwiftResult<()> { + let Some(credentials) = credentials.as_ref() else { + return Err(SwiftError::Unauthorized( + "Keystone authentication required to update account metadata".to_string(), + )); + }; + validate_account_access(account, credentials)?; + + // 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)?; + let bucket_name = get_account_metadata_bucket_name(account); let Some(store) = resolve_swift_object_store_handle() else { @@ -173,57 +190,30 @@ pub async fn update_account_metadata( .map_err(|e| SwiftError::InternalServerError(format!("Failed to create account metadata bucket: {}", e)))?; } - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", 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![] }); - let mut bucket_meta_clone = (*bucket_meta).clone(); - - // Get existing tags, preserving non-Swift tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); - - // Remove old swift-account-meta-* tags while preserving other tags - existing_tagging.tag_set.retain(|tag| { - if let Some(key) = &tag.key { - !key.starts_with("swift-account-meta-") - } else { - true - } - }); - - // Add new metadata tags - for (key, value) in metadata { - existing_tagging.tag_set.push(Tag { - key: Some(format!("swift-account-meta-{}", key)), - value: Some(value.clone()), + tagging.tag_set.retain(|tag| { + if let Some(key) = &tag.key { + !key.starts_with("swift-account-meta-") + } else { + true + } }); - } - let now = time::OffsetDateTime::now_utc(); + for (key, value) in metadata { + tagging.tag_set.push(Tag { + key: Some(format!("swift-account-meta-{}", key)), + value: Some(value.clone()), + }); + } - if existing_tagging.tag_set.is_empty() { - // No tags remain; clear tagging config - bucket_meta_clone.tagging_config_xml = Vec::new(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = None; - } else { - // Serialize tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - } - - // Save updated metadata - set_swift_bucket_metadata(bucket_name.clone(), bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; Ok(()) } diff --git a/crates/protocols/src/swift/container.rs b/crates/protocols/src/swift/container.rs index ffbc24498..fb0ad807d 100644 --- a/crates/protocols/src/swift/container.rs +++ b/crates/protocols/src/swift/container.rs @@ -22,7 +22,10 @@ use super::storage_api::container::{ }; use super::types::Container; use super::{SwiftError, SwiftResult}; -use super::{get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, set_swift_bucket_metadata}; +use super::{ + get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, update_swift_bucket_tagging, + validate_metadata, +}; use rustfs_credentials::Credentials; use s3s::dto::{Tag, Tagging}; use sha2::{Digest, Sha256}; @@ -483,6 +486,11 @@ pub async fn update_container_metadata( // Validate container name validate_container_name(container)?; + // 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)?; + // Create mapper with default config (tenant prefixing enabled) let mapper = ContainerMapper::default(); @@ -506,57 +514,30 @@ pub async fn update_container_metadata( } })?; - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", e)))?; + // 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![] }); - let mut bucket_meta_clone = (*bucket_meta).clone(); + 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) + } + }); - // Get existing tags, preserving non-Swift tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); - - // Remove old swift-meta-* tags while preserving other tags - existing_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 - // Add new Swift metadata tags if provided - if let Some(mut new_tagging) = swift_metadata_to_s3_tags(&metadata) { - // Merge: existing non-Swift tags + new Swift tags - existing_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 - - let now = time::OffsetDateTime::now_utc(); - - if existing_tagging.tag_set.is_empty() { - // No tags remain after removing swift-meta-* tags; clear tagging config - bucket_meta_clone.tagging_config_xml = Vec::new(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = None; - } else { - // Serialize the merged tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - } - - // Save updated metadata - set_swift_bucket_metadata(bucket_name, bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; Ok(()) } @@ -819,44 +800,23 @@ pub async fn enable_versioning( } })?; - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", e)))?; + // Rewrite the persisted tags: replace any versioning tag with the new + // archive location while preserving all other tags. + update_swift_bucket_tagging(bucket_name, |current| { + let mut tagging = current.cloned().unwrap_or_else(|| Tagging { tag_set: vec![] }); - let mut bucket_meta_clone = (*bucket_meta).clone(); + tagging + .tag_set + .retain(|tag| tag.key.as_deref() != Some("swift-versions-location")); - // Get existing tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); + tagging.tag_set.push(Tag { + key: Some("swift-versions-location".to_string()), + value: Some(archive_container.to_string()), // Store Swift container name, not S3 bucket name + }); - // Remove old versioning tag if present - existing_tagging - .tag_set - .retain(|tag| tag.key.as_deref() != Some("swift-versions-location")); - - // Add new versioning tag - existing_tagging.tag_set.push(Tag { - key: Some("swift-versions-location".to_string()), - value: Some(archive_container.to_string()), // Store Swift container name, not S3 bucket name - }); - - let now = time::OffsetDateTime::now_utc(); - - // Serialize tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - - // Save updated metadata - set_swift_bucket_metadata(bucket_name, bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; Ok(()) } @@ -882,50 +842,38 @@ pub async fn disable_versioning(account: &str, container: &str, credentials: &Cr let mapper = ContainerMapper::default(); let bucket_name = mapper.swift_to_s3_bucket(container, &project_id); - // Verify container exists - let Some(_store) = resolve_swift_object_store_handle() else { + let Some(store) = resolve_swift_object_store_handle() else { return Err(SwiftError::InternalServerError("Storage layer not initialized".to_string())); }; - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) + // Verify container exists. Without this the rewrite below would persist a + // fabricated default for a container that does not exist: the metadata + // loader turns "no metadata on disk" into a fresh BucketMetadata, and + // writing that creates an orphan .metadata.bin and caches a fabricated + // default as authoritative. + store + .get_bucket_info(&bucket_name, &BucketOptions::default()) .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", e)))?; + .map_err(|e| { + if e.to_string().contains("not found") || e.to_string().contains("NoSuchBucket") { + SwiftError::NotFound(format!("Container '{}' not found", container)) + } else { + sanitize_storage_error("Container verification", e) + } + })?; - let mut bucket_meta_clone = (*bucket_meta).clone(); + // Rewrite the persisted tags: drop the versioning tag while preserving + // all 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![] }); - // Get existing tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); + tagging + .tag_set + .retain(|tag| tag.key.as_deref() != Some("swift-versions-location")); - // Remove versioning tag - existing_tagging - .tag_set - .retain(|tag| tag.key.as_deref() != Some("swift-versions-location")); - - let now = time::OffsetDateTime::now_utc(); - - if existing_tagging.tag_set.is_empty() { - // No tags remain; clear tagging config - bucket_meta_clone.tagging_config_xml = Vec::new(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = None; - } else { - // Serialize remaining tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - } - - // Save updated metadata - set_swift_bucket_metadata(bucket_name, bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; Ok(()) } @@ -1050,65 +998,37 @@ pub async fn set_container_acl( } })?; - // Load current bucket metadata - let bucket_meta = get_swift_bucket_metadata(&bucket_name) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to load bucket metadata: {}", e)))?; + // Rewrite the persisted tags: replace the ACL tags with the new grants + // while preserving all 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![] }); - let mut bucket_meta_clone = (*bucket_meta).clone(); + tagging + .tag_set + .retain(|tag| tag.key.as_deref() != Some("swift-acl-read") && tag.key.as_deref() != Some("swift-acl-write")); - // Get existing tags - let mut existing_tagging = bucket_meta_clone - .tagging_config - .clone() - .unwrap_or_else(|| Tagging { tag_set: vec![] }); + if let Some(read) = read_acl + && !read.trim().is_empty() + { + tagging.tag_set.push(Tag { + key: Some("swift-acl-read".to_string()), + value: Some(read.to_string()), + }); + } - // Remove old ACL tags - existing_tagging - .tag_set - .retain(|tag| tag.key.as_deref() != Some("swift-acl-read") && tag.key.as_deref() != Some("swift-acl-write")); + if let Some(write) = write_acl + && !write.trim().is_empty() + { + tagging.tag_set.push(Tag { + key: Some("swift-acl-write".to_string()), + value: Some(write.to_string()), + }); + } - // Add new read ACL tag if provided - if let Some(read) = read_acl - && !read.trim().is_empty() - { - existing_tagging.tag_set.push(Tag { - key: Some("swift-acl-read".to_string()), - value: Some(read.to_string()), - }); - } - - // Add new write ACL tag if provided - if let Some(write) = write_acl - && !write.trim().is_empty() - { - existing_tagging.tag_set.push(Tag { - key: Some("swift-acl-write".to_string()), - value: Some(write.to_string()), - }); - } - - let now = time::OffsetDateTime::now_utc(); - - if existing_tagging.tag_set.is_empty() { - // No tags remain; clear tagging config - bucket_meta_clone.tagging_config_xml = Vec::new(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = None; - } else { - // Serialize tags to XML - let tagging_xml = quick_xml::se::to_string(&existing_tagging) - .map_err(|e| SwiftError::InternalServerError(format!("Failed to serialize tags: {}", e)))?; - - bucket_meta_clone.tagging_config_xml = tagging_xml.into_bytes(); - bucket_meta_clone.tagging_config_updated_at = now; - bucket_meta_clone.tagging_config = Some(existing_tagging); - } - - // Save updated metadata - set_swift_bucket_metadata(bucket_name, bucket_meta_clone) - .await - .map_err(|e| SwiftError::InternalServerError(format!("Failed to save metadata: {}", e)))?; + tagging + }) + .await?; debug!( "Set ACLs for container {}/{}: read={:?}, write={:?}", diff --git a/crates/protocols/src/swift/mod.rs b/crates/protocols/src/swift/mod.rs index 2dba11b8d..881b8fd4f 100644 --- a/crates/protocols/src/swift/mod.rs +++ b/crates/protocols/src/swift/mod.rs @@ -58,10 +58,53 @@ pub mod versioning; pub use errors::{SwiftError, SwiftResult}; pub use router::{SwiftRoute, SwiftRouter}; + +/// Maximum number of metadata headers allowed per resource (Swift standard) +pub(crate) const MAX_METADATA_COUNT: usize = 90; + +/// Maximum size in bytes for a single metadata value (Swift standard) +pub(crate) const MAX_METADATA_VALUE_SIZE: usize = 256; + +/// Validate metadata against Swift limits +/// +/// Checks that: +/// - 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. +/// +/// Returns error if limits are exceeded. +pub(crate) fn validate_metadata(metadata: &std::collections::HashMap) -> SwiftResult<()> { + // Check total metadata count + if metadata.len() > MAX_METADATA_COUNT { + return Err(SwiftError::BadRequest(format!( + "Too many metadata headers: {} (max: {})", + metadata.len(), + MAX_METADATA_COUNT + ))); + } + + // Check individual value sizes + for (key, value) in metadata.iter() { + if value.len() > MAX_METADATA_VALUE_SIZE { + return Err(SwiftError::BadRequest(format!( + "Metadata value for '{}' too large: {} bytes (max: {} bytes)", + key, + value.len(), + MAX_METADATA_VALUE_SIZE + ))); + } + } + + Ok(()) +} + // Note: Container, Object, and SwiftMetadata types used by Swift implementation pub use storage_api::public_api::{SwiftGetObjectReader, SwiftObjectInfo, SwiftObjectOptions, SwiftPutObjReader}; pub(crate) use storage_api::public_api::{ - get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, set_swift_bucket_metadata, + get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, update_swift_bucket_tagging, }; #[allow(unused_imports)] pub use types::{Container, Object, SwiftMetadata}; diff --git a/crates/protocols/src/swift/object.rs b/crates/protocols/src/swift/object.rs index 66fa370c3..7620c5bd1 100644 --- a/crates/protocols/src/swift/object.rs +++ b/crates/protocols/src/swift/object.rs @@ -53,7 +53,7 @@ use super::account::validate_account_access; use super::container::ContainerMapper; use super::expiration_worker::{track_object_expiration, untrack_object_expiration}; use super::storage_api::object::{BucketOperations, BucketOptions, HTTPRangeSpec, ObjectIO as _, ObjectOperations as _}; -use super::{SwiftError, SwiftResult, resolve_swift_object_store_handle}; +use super::{SwiftError, SwiftResult, resolve_swift_object_store_handle, validate_metadata}; use axum::http::HeaderMap; use rustfs_credentials::Credentials; use rustfs_rio::HashReader; @@ -68,12 +68,6 @@ const LOG_SUBSYSTEM_SWIFT_OBJECT: &str = "swift_object"; const EVENT_SWIFT_OBJECT_STORAGE_STATE: &str = "swift_object_storage_state"; const SWIFT_DELETE_AT_METADATA: &str = "x-delete-at"; -/// Maximum number of metadata headers allowed per object (Swift standard) -const MAX_METADATA_COUNT: usize = 90; - -/// Maximum size in bytes for a single metadata value (Swift standard) -const MAX_METADATA_VALUE_SIZE: usize = 256; - /// Maximum object size in bytes (5GB - Swift default) const MAX_OBJECT_SIZE: i64 = 5 * 1024 * 1024 * 1024; @@ -234,38 +228,6 @@ impl Default for ObjectKeyMapper { } } -/// Validate metadata against Swift limits -/// -/// Checks that: -/// - Total number of metadata entries doesn't exceed MAX_METADATA_COUNT -/// - Individual metadata values don't exceed MAX_METADATA_VALUE_SIZE -/// -/// Returns error if limits are exceeded. -fn validate_metadata(metadata: &HashMap) -> SwiftResult<()> { - // Check total metadata count - if metadata.len() > MAX_METADATA_COUNT { - return Err(SwiftError::BadRequest(format!( - "Too many metadata headers: {} (max: {})", - metadata.len(), - MAX_METADATA_COUNT - ))); - } - - // Check individual value sizes - for (key, value) in metadata.iter() { - if value.len() > MAX_METADATA_VALUE_SIZE { - return Err(SwiftError::BadRequest(format!( - "Metadata value for '{}' too large: {} bytes (max: {} bytes)", - key, - value.len(), - MAX_METADATA_VALUE_SIZE - ))); - } - } - - Ok(()) -} - fn metadata_delete_at(metadata: &HashMap) -> Option { metadata .get(SWIFT_DELETE_AT_METADATA) diff --git a/crates/protocols/src/swift/storage_api.rs b/crates/protocols/src/swift/storage_api.rs index b65c711e5..70e7b052f 100644 --- a/crates/protocols/src/swift/storage_api.rs +++ b/crates/protocols/src/swift/storage_api.rs @@ -15,14 +15,19 @@ use std::collections::HashMap; use std::sync::Arc; +use rustfs_ecstore::api::bucket::metadata::BUCKET_TAGGING_CONFIG; pub(crate) use rustfs_ecstore::api::bucket::metadata::BucketMetadata as SwiftBucketMetadata; -use rustfs_ecstore::api::bucket::metadata_sys::{ - get as get_swift_bucket_metadata_from_backend, set_bucket_metadata as set_swift_bucket_metadata_in_backend, -}; +use rustfs_ecstore::api::bucket::metadata_sys::{get as get_swift_bucket_metadata_from_backend, update_config_with}; +use rustfs_ecstore::api::bucket::utils::serialize as serialize_bucket_config; +use rustfs_ecstore::api::error::Error as SwiftStorageError; pub(crate) use rustfs_ecstore::api::error::Result as SwiftStorageResult; +use rustfs_ecstore::api::notification::get_global_notification_sys; pub(crate) use rustfs_ecstore::api::runtime::object_store_handle as resolve_swift_object_store_handle; use rustfs_ecstore::api::storage::ECStore as SwiftStore; use rustfs_storage_api as storage_contracts; +use s3s::dto::Tagging; + +use super::{SwiftError, SwiftResult}; pub(crate) mod account { pub(crate) use super::storage_contracts::{BucketOperations, MakeBucketOptions}; @@ -45,7 +50,7 @@ pub(crate) mod object { pub(crate) mod public_api { pub use super::{SwiftGetObjectReader, SwiftObjectInfo, SwiftObjectOptions, SwiftPutObjReader}; pub(crate) use super::{ - get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, set_swift_bucket_metadata, + get_swift_bucket_metadata, get_swift_bucket_usage, resolve_swift_object_store_handle, update_swift_bucket_tagging, }; } @@ -53,6 +58,15 @@ pub(crate) mod versioning { pub(crate) use super::storage_contracts::{ListOperations, ObjectOperations}; } +const LOG_COMPONENT_PROTOCOLS: &str = "protocols"; +const LOG_SUBSYSTEM_SWIFT_STORAGE: &str = "swift_storage"; +const EVENT_SWIFT_BUCKET_TAGGING_UPDATE: &str = "swift_bucket_tagging_update"; + +/// Marks the refusal to rewrite an unreadable persisted tagging config, so the +/// caller can turn it into an actionable client error rather than a generic +/// storage failure. Carried through the ecstore error, which is a string type. +const UNREADABLE_TAGGING_SENTINEL: &str = "swift: persisted tagging config could not be parsed"; + pub type SwiftGetObjectReader = ::GetObjectReader; pub type SwiftObjectInfo = ::ObjectInfo; pub type SwiftObjectOptions = ::ObjectOptions; @@ -62,8 +76,80 @@ pub(crate) async fn get_swift_bucket_metadata(bucket: &str) -> SwiftStorageResul get_swift_bucket_metadata_from_backend(bucket).await } -pub(crate) async fn set_swift_bucket_metadata(bucket: String, metadata: SwiftBucketMetadata) -> SwiftStorageResult<()> { - set_swift_bucket_metadata_in_backend(bucket, metadata).await +/// Rewrite the bucket's tagging config through the persisting +/// bucket-metadata path. +/// +/// `rewrite` sees the tag set currently persisted on disk (`None` when the +/// bucket has none) and returns the full replacement; an empty tag set +/// clears the config. The read-modify-write runs under the bucket metadata +/// system's write guard — serialized against every other config update — +/// and the result is written to the bucket metadata file before the cache +/// is refreshed, so a Swift metadata POST survives process restarts and +/// disk-truth reloads. Peers are then told to reload, matching what the S3 +/// handlers do after a config write. +/// +/// 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 +/// persisted config, which is reported specifically because the operator has +/// to act on it. +pub(crate) async fn update_swift_bucket_tagging(bucket: String, rewrite: F) -> SwiftResult<()> +where + F: FnOnce(Option<&Tagging>) -> Tagging + Send, +{ + 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 — + // because the rewrite closures treat "no parsed tags" as "no tags". + // Refuse instead: the persisted config is intact, just unreadable. + if !bm.tagging_config_xml.is_empty() && bm.tagging_config.is_none() { + return Err(SwiftStorageError::other(UNREADABLE_TAGGING_SENTINEL)); + } + + let tagging = rewrite(bm.tagging_config.as_ref()); + if tagging.tag_set.is_empty() { + Ok(Vec::new()) + } else { + // The S3 XML serializer, not quick_xml: the metadata loader's + // parse step must be able to round-trip what we persist. + serialize_bucket_config(&tagging) + .map_err(|e| SwiftStorageError::other(format!("failed to serialize bucket tagging: {e}"))) + } + }) + .await; + + if let Err(err) = result { + let unreadable = err.to_string().contains(UNREADABLE_TAGGING_SENTINEL); + tracing::error!( + event = EVENT_SWIFT_BUCKET_TAGGING_UPDATE, + component = LOG_COMPONENT_PROTOCOLS, + subsystem = LOG_SUBSYSTEM_SWIFT_STORAGE, + bucket = %bucket, + error = %err, + reason = if unreadable { "unreadable_persisted_config" } else { "storage_failure" }, + result = "failed", + "swift bucket tagging update failed" + ); + // A Swift-only client has no way to repair this itself, so say what + // happened and name the remedy instead of a bare storage error. + return Err(if unreadable { + SwiftError::Conflict(format!( + "The persisted tagging configuration for container store '{bucket}' cannot be parsed, so metadata cannot be updated without discarding it. Reset it with the S3 DeleteBucketTagging API." + )) + } else { + SwiftError::InternalServerError("Metadata update operation failed".to_string()) + }); + } + + if let Some(notification_sys) = get_global_notification_sys() { + tokio::spawn(async move { + if let Err(err) = notification_sys.load_bucket_metadata(&bucket).await { + tracing::warn!(bucket = %bucket, error = %err, "failed to notify peers after swift bucket tagging update"); + } + }); + } + + Ok(()) } pub(crate) async fn get_swift_bucket_usage() -> SwiftStorageResult>> { diff --git a/crates/protocols/tests/ecstore_test_compat/mod.rs b/crates/protocols/tests/ecstore_test_compat/mod.rs new file mode 100644 index 000000000..ebec36890 --- /dev/null +++ b/crates/protocols/tests/ecstore_test_compat/mod.rs @@ -0,0 +1,25 @@ +// 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. + +//! ECStore facade boundary for the protocols integration tests. +//! +//! The Swift tests need to drive bucket-metadata reloads the way the peer +//! `LoadBucketMetadata` RPC does. Everything they touch from `rustfs_ecstore` +//! is aliased here so the tests themselves hold no raw facade subpaths, the +//! same boundary `crates/protocols/src/swift/storage_api.rs` provides for the +//! Swift implementation. + +pub use rustfs_ecstore::api::bucket::metadata::load_bucket_metadata; +pub use rustfs_ecstore::api::bucket::metadata_sys::{get as get_bucket_metadata, set_bucket_metadata}; +pub use rustfs_ecstore::api::runtime::object_store_handle; diff --git a/crates/protocols/tests/swift_metadata_persistence.rs b/crates/protocols/tests/swift_metadata_persistence.rs new file mode 100644 index 000000000..25a7e42ad --- /dev/null +++ b/crates/protocols/tests/swift_metadata_persistence.rs @@ -0,0 +1,312 @@ +// 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. + +//! Regression tests: a Swift metadata POST must be persisted to the bucket +//! 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. + +#![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_test_utils::TestECStoreEnv; +use serde_json::json; +use sha2::{Digest, Sha256}; + +mod ecstore_test_compat; +use ecstore_test_compat::{get_bucket_metadata, load_bucket_metadata, object_store_handle, set_bucket_metadata}; + +fn keystone_credentials(project_id: &str) -> Credentials { + let mut claims = HashMap::new(); + claims.insert("keystone_project_id".to_string(), json!(project_id)); + claims.insert("keystone_roles".to_string(), json!(["member"])); + + Credentials { + access_key: "keystone:swift-test".to_string(), + claims: Some(claims), + ..Default::default() + } +} + +/// The account-metadata bucket name scheme from `swift::account` +/// (`swift-account-{sha256(account)[0..16]}`), mirrored here so the test can +/// reload that bucket's metadata from disk. +fn account_metadata_bucket_name(account: &str) -> String { + let mut hasher = Sha256::new(); + hasher.update(account.as_bytes()); + let hash = hex::encode(hasher.finalize()); + format!("swift-account-{}", &hash[0..16]) +} + +/// Replace the cached bucket metadata with what is actually on disk — the +/// same thing a peer LoadBucketMetadata notification or the periodic refresh +/// loop does. Before the fix this silently discarded every Swift metadata +/// POST, because those writes only ever touched the cache. +async fn reload_bucket_metadata_from_disk(bucket: &str) { + let store = object_store_handle().expect("test store should be published"); + let bm = load_bucket_metadata(store, bucket) + .await + .expect("bucket metadata should load from disk"); + set_bucket_metadata(bucket.to_string(), bm) + .await + .expect("reloaded metadata should install"); +} + +/// The `swift-meta-*` tags currently persisted for a container, keyed the way +/// a Swift client sees them. `get_container_metadata` would be the natural +/// reader, but it additionally requires a data-usage snapshot for the object +/// count and byte total, which a bare test store has none of — and that is +/// orthogonal to whether the metadata itself was persisted. +async fn persisted_container_metadata(bucket: &str) -> HashMap { + let bm = get_bucket_metadata(bucket).await.expect("bucket metadata should be cached"); + let mut out = HashMap::new(); + if let Some(tagging) = &bm.tagging_config { + for tag in &tagging.tag_set { + if let (Some(key), Some(value)) = (&tag.key, &tag.value) + && let Some(meta_key) = key.strip_prefix("swift-meta-") + { + out.insert(meta_key.to_string(), value.clone()); + } + } + } + out +} + +/// Every scenario that needs a store runs against ONE environment: the Swift +/// handlers resolve the ambient object store and bucket-metadata system, so a +/// second `TestECStoreEnv` in this process would race the first. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn swift_metadata_writes_are_durable() { + let env = TestECStoreEnv::builder().prefix("swift_meta_persist").build().await; + + posts_survive_disk_truth_reload(&env).await; + tag_writers_preserve_each_others_state(&env).await; + versioning_writes_reject_missing_containers().await; +} + +async fn posts_survive_disk_truth_reload(env: &TestECStoreEnv) { + // --- Container metadata POST (X-Container-Meta-*) --- + let project_id = "swiftpersistproj"; + let swift_account = format!("AUTH_{project_id}"); + let credentials = keystone_credentials(project_id); + let swift_container = "photos"; + 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"); + + reload_bucket_metadata_from_disk(&bucket).await; + + let container_meta = persisted_container_metadata(&bucket).await; + assert_eq!( + container_meta.get("color").map(String::as_str), + Some("blue"), + "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"); + + 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!( + !container_meta.contains_key("color"), + "replaced container metadata must not resurrect on reload" + ); + + // --- Container versioning POST (X-Versions-Location) --- + let archive_container = "photos-archive"; + let archive_bucket = ContainerMapper::default().swift_to_s3_bucket(archive_container, project_id); + env.make_bucket(&archive_bucket, false).await; + + container::enable_versioning(&swift_account, swift_container, archive_container, &credentials) + .await + .expect("enable versioning should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + let location = container::get_versions_location(&swift_account, swift_container, &credentials) + .await + .expect("versions location should load"); + assert_eq!( + location.as_deref(), + Some(archive_container), + "versions location must survive a disk-truth metadata reload" + ); + + // --- 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"); + + reload_bucket_metadata_from_disk(&account_metadata_bucket_name(&swift_account)).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"), + "account metadata POST must survive a disk-truth metadata reload" + ); +} + +/// The rewrites all share one tag set, so a closure that ignored the current +/// state would still pass a single-feature test. Drive ACLs, versioning and +/// container metadata over the same container and assert each survives the +/// others — and that clearing one leaves the rest alone. +async fn tag_writers_preserve_each_others_state(env: &TestECStoreEnv) { + let project_id = "swiftcrosstagproj"; + let swift_account = format!("AUTH_{project_id}"); + let credentials = keystone_credentials(project_id); + let container = "shared"; + let archive = "shared-archive"; + let bucket = ContainerMapper::default().swift_to_s3_bucket(container, project_id); + env.make_bucket(&bucket, false).await; + 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) + .await + .expect("container metadata POST should succeed"); + container::enable_versioning(&swift_account, container, archive, &credentials) + .await + .expect("enable versioning should succeed"); + container::set_container_acl(&swift_account, container, Some(".r:*"), Some("AUTH_other"), &credentials) + .await + .expect("set container ACL should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + // All three writers' state coexists after a disk-truth reload. + let meta = persisted_container_metadata(&bucket).await; + assert_eq!(meta.get("color").map(String::as_str), Some("blue")); + assert_eq!( + container::get_versions_location(&swift_account, container, &credentials) + .await + .expect("versions location should load") + .as_deref(), + Some(archive) + ); + let acl = container::get_container_acl(&swift_account, container, &credentials) + .await + .expect("container ACL should load"); + assert!(!acl.read.is_empty(), "read ACL must survive the reload"); + assert!(!acl.write.is_empty(), "write ACL must survive the reload"); + + // Disabling versioning drops only the versioning tag. + container::disable_versioning(&swift_account, container, &credentials) + .await + .expect("disable versioning should succeed"); + + reload_bucket_metadata_from_disk(&bucket).await; + + assert_eq!( + container::get_versions_location(&swift_account, container, &credentials) + .await + .expect("versions location should load"), + None, + "disable_versioning must clear the versioning tag durably" + ); + let meta = persisted_container_metadata(&bucket).await; + assert_eq!( + meta.get("color").map(String::as_str), + Some("blue"), + "disable_versioning must not disturb container metadata" + ); + let acl = container::get_container_acl(&swift_account, container, &credentials) + .await + .expect("container ACL should load"); + assert!(!acl.read.is_empty(), "disable_versioning must not disturb the ACL"); +} + +/// 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 +/// fabricated default as authoritative. +async fn versioning_writes_reject_missing_containers() { + let project_id = "swiftmissingproj"; + let swift_account = format!("AUTH_{project_id}"); + let credentials = keystone_credentials(project_id); + let missing = "no-such-container"; + + let err = container::disable_versioning(&swift_account, missing, &credentials) + .await + .expect_err("disabling versioning on 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(), + "a rejected write must not have cached metadata for a nonexistent container" + ); +} + +/// Account metadata holds the account's TempURL signing key, and it is now +/// durable — so a write for someone else's account would be a persistent, +/// cluster-wide takeover of that account's pre-signed URLs, not a cache blip. +/// The write path must reject both a foreign account and a missing token, +/// while reads stay open for pre-auth TempURL signature validation. +/// +/// Deliberately builds no store: both rejections must happen before the write +/// path resolves storage at all, and a second `TestECStoreEnv` in this process +/// would race the other test over the ambient store handle. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +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 err = account::update_account_metadata(victim_account, &poisoned, &Some(attacker_credentials)) + .await + .expect_err("writing another account's metadata must be rejected"); + assert!( + matches!(err, SwiftError::Forbidden(_)), + "cross-account metadata write must be Forbidden, got {err:?}" + ); + + let err = account::update_account_metadata(victim_account, &poisoned, &None) + .await + .expect_err("anonymous account metadata write must be rejected"); + assert!( + matches!(err, SwiftError::Unauthorized(_)), + "anonymous metadata write must be Unauthorized, got {err:?}" + ); +} diff --git a/crates/utils/src/http/metadata_compat.rs b/crates/utils/src/http/metadata_compat.rs index 9dc6ba175..c95f5b3d7 100644 --- a/crates/utils/src/http/metadata_compat.rs +++ b/crates/utils/src/http/metadata_compat.rs @@ -37,6 +37,7 @@ pub const SUFFIX_CRC: &str = "crc"; pub const SUFFIX_TRANSITION_STATUS: &str = "transition-status"; pub const SUFFIX_TRANSITIONED_OBJECTNAME: &str = "transitioned-object"; pub const SUFFIX_TRANSITIONED_VERSION_ID: &str = "transitioned-versionID"; +pub const SUFFIX_TRANSITIONED_VERSION_STATE: &str = "transitioned-version-state"; pub const SUFFIX_TRANSITION_TIER: &str = "transition-tier"; pub const SUFFIX_TRANSITION_TIER_DESTINATION_ID: &str = "transition-tier-destination-id"; pub const SUFFIX_RESTORE_OPERATION_ID: &str = "restore-operation-id"; diff --git a/rustfs/src/app/lifecycle_transition_api_test.rs b/rustfs/src/app/lifecycle_transition_api_test.rs index 77aa6dcfd..2053ebe5c 100644 --- a/rustfs/src/app/lifecycle_transition_api_test.rs +++ b/rustfs/src/app/lifecycle_transition_api_test.rs @@ -60,7 +60,6 @@ use uuid::Uuid; static GLOBAL_ENV: OnceLock<(Vec, Arc)> = OnceLock::new(); static INIT: Once = Once::new(); const TRANSITION_WAIT_TIMEOUT: Duration = Duration::from_secs(15); -const RESTORE_COPY_BACK_WAIT_TIMEOUT: Duration = Duration::from_secs(60); const ENV_GET_CODEC_STREAMING_ENABLE: &str = "RUSTFS_GET_CODEC_STREAMING_ENABLE"; const ENV_GET_CODEC_STREAMING_ROLLOUT: &str = "RUSTFS_GET_CODEC_STREAMING_ROLLOUT"; const ENV_GET_CODEC_STREAMING_BODY_COMPAT_CONFIRMED: &str = "RUSTFS_GET_CODEC_STREAMING_BODY_COMPAT_CONFIRMED"; @@ -339,45 +338,6 @@ async fn wait_for_transition(ecstore: &Arc, bucket: &str, object: &str, } } -async fn wait_for_restore_completion( - ecstore: &Arc, - backend: &MockWarmBackend, - bucket: &str, - object: &str, - timeout: Duration, -) -> Result { - let deadline = tokio::time::Instant::now() + timeout; - let mut last_state = None; - - loop { - if tokio::time::Instant::now() >= deadline { - let tier_gets = backend.get_count().await; - let op_log = backend.op_log().await; - return Err(format!( - "restore copy-back should complete within {timeout:?}; tier_gets={tier_gets}, op_log={op_log:?}; last observed state: {}", - last_state.unwrap_or_else(|| "no object info observed".to_string()) - )); - } - - match (**ecstore).get_object_info(bucket, object, &ObjectOptions::default()).await { - Ok(info) => { - if !info.restore_ongoing && info.restore_expires.is_some() { - return Ok(info); - } - last_state = Some(format!( - "restore_ongoing={}, restore_expires={:?}, transitioned_status={}", - info.restore_ongoing, info.restore_expires, info.transitioned_object.status - )); - } - Err(err) => { - last_state = Some(format!("get_object_info failed: {err}")); - } - } - - tokio::time::sleep(Duration::from_millis(500)).await; - } -} - // SAFETY: this helper is used only by `#[serial]` tests and runs under the single-threaded Tokio // runtime (`worker_threads = 1`), so no concurrent test can mutate process environment during the // `env::set_var` / `env::remove_var` window. @@ -2097,10 +2057,9 @@ async fn put_bucket_lifecycle_configuration_rejects_zero_day_expiration() { /// POST restore(days=1) is accepted and flips the object to /// `x-amz-restore: ongoing-request="true"` while the mock tier GET barrier /// proves the background copy-back has reached the remote read; a second POST -/// during that window is rejected with 409 `RestoreAlreadyInProgress`; once the -/// copy-back completes the object reports `ongoing-request="false"` with a -/// future expiry-date; and a full GET is then served from the local restored -/// copy (the mock tier records no further `get` calls). +/// during that window is rejected with 409 `RestoreAlreadyInProgress`. +/// Synchronous SetDisks transition tests cover copy-back completion, restore +/// metadata, and local byte-identical reads. /// /// Re-enabled in the serial lane by backlog#1304: the accept path now flips /// the ongoing flag under a short compare-and-set guard and the copy-back @@ -2110,7 +2069,7 @@ async fn put_bucket_lifecycle_configuration_rejects_zero_day_expiration() { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[serial] #[ignore = "global-state ILM integration test: runs serialized in the CI ILM Integration (serial) lane, see ci.yml test-ilm-integration-serial and rustfs/backlog#1148 (ilm-8)"] -async fn restore_object_usecase_reports_ongoing_conflict_and_completion() { +async fn restore_object_usecase_reports_ongoing_conflict() { let (_disk_paths, ecstore) = setup_test_env().await; let usecase = DefaultObjectUsecase::from_global(); @@ -2177,35 +2136,6 @@ async fn restore_object_usecase_reports_ongoing_conflict_and_completion() { ); get_barrier.release(); - - // Completion: ongoing flips to false and a future expiry-date appears. - let completed = - wait_for_restore_completion(&ecstore, &backend, bucket.as_str(), object, RESTORE_COPY_BACK_WAIT_TIMEOUT).await; - let completed = completed.unwrap_or_else(|err| panic!("{err}")); - - let now_secs = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("clock before unix epoch") - .as_secs() as i64; - let expires = completed.restore_expires.expect("completed restore carries an expiry"); - assert!( - expires.unix_timestamp() > now_secs, - "restore expiry-date must be in the future, got {expires}" - ); - assert_eq!( - completed.transitioned_object.status, "complete", - "restore must not clear the transitioned state" - ); - - // The restored copy serves GET locally: no further tier GETs. - let tier_gets_after_restore = backend.get_count().await; - let data = read_object_bytes(&ecstore, bucket.as_str(), object).await; - assert_eq!(data, payload, "restored GET must return the original bytes"); - assert_eq!( - backend.get_count().await, - tier_gets_after_restore, - "GET of a restored object must be served locally, not from the tier" - ); } /// backlog#1304: the restore-accept compare-and-set itself, under real diff --git a/rustfs/src/app/object_usecase.rs b/rustfs/src/app/object_usecase.rs index 6e04bef5c..e77f7c0e6 100644 --- a/rustfs/src/app/object_usecase.rs +++ b/rustfs/src/app/object_usecase.rs @@ -763,7 +763,7 @@ async fn enqueue_transitioned_delete_cleanup( let _activity_guard = DeleteTailActivityGuard::new(DeleteTailStage::Cleanup); let je = if opts.delete_prefix { - tier_sweeper::transitioned_force_delete_journal_entry(&existing.transitioned_object) + tier_sweeper::transitioned_force_delete_journal_entry(&existing.transitioned_object, existing.transition_version_state) } else { let version_id = opts.version_id.as_ref().and_then(|v| Uuid::parse_str(v).ok()); tier_sweeper::transitioned_delete_journal_entry( @@ -771,6 +771,7 @@ async fn enqueue_transitioned_delete_cleanup( opts.versioned, opts.version_suspended, &existing.transitioned_object, + existing.transition_version_state, ) }; let Some(mut je) = je else { @@ -9683,7 +9684,7 @@ mod tests { #[tokio::test] #[serial_test::serial] - async fn transitioned_delete_cleanup_persists_identity_bound_and_legacy_journals() { + async fn transitioned_delete_cleanup_persists_known_state_and_rejects_unknown_state() { let store = crate::app::gating_test_env::shared_gating_ecstore().await; if current_app_context().is_none() { crate::app::runtime_sources::install_test_app_context(Arc::clone(&store)).await; @@ -9703,8 +9704,9 @@ mod tests { current.transitioned_object.tier = "WARM".to_string(); current.transitioned_object.name = "remote/identity-bound".to_string(); current.transitioned_object.version_id = "remote-version".to_string(); + current.transition_version_state = rustfs_filemeta::TransitionVersionState::Exact; - let journal_name = |remote_object: &str, backend_identity: Option<[u8; 32]>| { + let journal_name = |remote_object: &str, backend_identity: Option<[u8; 32]>, version_id_exact: bool| { use sha2::{Digest, Sha256}; let mut hasher = Sha256::new(); @@ -9717,6 +9719,10 @@ mod tests { hasher.update([0]); hasher.update(backend_identity); } + if version_id_exact { + hasher.update([0]); + hasher.update(b"exact-version-id"); + } format!("ilm/tier-delete-journal/{}.json", rustfs_utils::crypto::hex(hasher.finalize().as_slice())) }; @@ -9726,7 +9732,7 @@ mod tests { let mut identity_bound = store .get_object_reader( ".rustfs.sys", - &journal_name("remote/identity-bound", Some(identity)), + &journal_name("remote/identity-bound", Some(identity), true), None, http::HeaderMap::new(), &ObjectOptions::default(), @@ -9739,11 +9745,14 @@ mod tests { .expect("identity-bound journal body should be readable"); let identity_bound: serde_json::Value = serde_json::from_slice(&identity_bound_data).expect("identity-bound journal should decode as JSON"); - assert_eq!(identity_bound["version"], serde_json::json!(2)); + assert_eq!(identity_bound["version"], serde_json::json!(4)); assert_eq!(identity_bound["backend_identity"], serde_json::json!(identity)); + assert_eq!(identity_bound["version_id_exact"], serde_json::json!(true)); + assert_eq!(identity_bound["version_state"], serde_json::json!("exact")); current.user_defined = Arc::new(HashMap::new()); current.transitioned_object.name = "remote/legacy".to_string(); + current.transition_version_state = rustfs_filemeta::TransitionVersionState::Unknown; enqueue_transitioned_delete_cleanup( store.clone(), "bucket", @@ -9755,24 +9764,31 @@ mod tests { Some(¤t), ) .await - .expect("legacy force-delete cleanup should persist a fail-closed v1 journal"); - let mut legacy = store + .expect("unknown force-delete cleanup should fail closed without a journal"); + let legacy_err = match store .get_object_reader( ".rustfs.sys", - &journal_name("remote/legacy", None), + &journal_name("remote/legacy", None, false), None, http::HeaderMap::new(), &ObjectOptions::default(), ) .await - .expect("legacy journal should be readable"); - let mut legacy_data = Vec::new(); - tokio::io::AsyncReadExt::read_to_end(&mut legacy.stream, &mut legacy_data) - .await - .expect("legacy journal body should be readable"); - let legacy: serde_json::Value = serde_json::from_slice(&legacy_data).expect("legacy journal should decode as JSON"); - assert_eq!(legacy["version"], serde_json::json!(1)); - assert_eq!(legacy["backend_identity"], serde_json::Value::Null); + { + Ok(_) => panic!("unknown remote version state must not persist a delete journal"), + Err(err) => err, + }; + assert!( + matches!( + &legacy_err, + StorageError::FileNotFound + | StorageError::ObjectNotFound(_, _) + | StorageError::FileVersionNotFound + | StorageError::VersionNotFound(_, _, _) + | StorageError::VolumeNotFound + ), + "unknown remote version state must leave no journal, got {legacy_err:?}" + ); } async fn put_real_cold_fill_object(store: &Arc, bucket: &str, object: &str, body: &[u8]) -> ObjectInfo { diff --git a/rustfs/src/app/storage_api.rs b/rustfs/src/app/storage_api.rs index 06765ac96..b97477b9a 100644 --- a/rustfs/src/app/storage_api.rs +++ b/rustfs/src/app/storage_api.rs @@ -409,20 +409,24 @@ pub(crate) mod bucket { versioned: bool, suspended: bool, transitioned: &super::super::super::storage_contracts::TransitionedObject, + transition_version_state: rustfs_filemeta::TransitionVersionState, ) -> Option { crate::storage::storage_api::ecstore_bucket::lifecycle::tier_sweeper::transitioned_delete_journal_entry( version_id, versioned, suspended, transitioned, + transition_version_state, ) } pub(crate) fn transitioned_force_delete_journal_entry( transitioned: &super::super::super::storage_contracts::TransitionedObject, + transition_version_state: rustfs_filemeta::TransitionVersionState, ) -> Option { crate::storage::storage_api::ecstore_bucket::lifecycle::tier_sweeper::transitioned_force_delete_journal_entry( transitioned, + transition_version_state, ) } } diff --git a/rustfs/src/storage/rpc/node_service/disk.rs b/rustfs/src/storage/rpc/node_service/disk.rs index ca863d0e5..d5fa44815 100644 --- a/rustfs/src/storage/rpc/node_service/disk.rs +++ b/rustfs/src/storage/rpc/node_service/disk.rs @@ -120,19 +120,6 @@ fn snapshot_lease_ttl(ttl_ms: u64) -> Result { Ok(ttl) } -#[cfg(test)] -mod snapshot_lease_tests { - use super::{SNAPSHOT_LEASE_MAX_TTL, SNAPSHOT_LEASE_MIN_TTL, snapshot_lease_ttl}; - - #[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()); - } -} - fn decode_msgpack_or_json( binary: &[u8], json: &str, @@ -1605,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; @@ -1619,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/scripts/README.md b/scripts/README.md index c88f4b679..1885201f8 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -90,7 +90,7 @@ their issue closes. | `manual_transition_journal_audit.sh` | dev-tool | Journal + metrics + log audit for manual transition jobs | — | | `manual_transition_mixed_rollout_matrix.sh` | dev-tool | Matrix generator for mixed-version rollout phases | — | | `manual_transition_mixed_rollout_runbook.sh` | dev-tool | Reusable mixed-version rollout runbook generator (external run) | — | -| `manual_transition_mixed_version_docker_harness.sh` | dev-tool | Dedicated #1508 Docker harness for old/new manual-transition rollout evidence | `test_manual_transition_runbooks.sh` | +| `manual_transition_mixed_version_docker_harness.sh` | dev-tool | Dedicated #1508 Docker harness for old/new manual-transition rollout evidence with strict/baseline/blocked result classification | `test_manual_transition_runbooks.sh` | | `monitor_manual_transition_ci.sh` | dev-tool | CI workflow/status watcher for manual transition follow-up monitoring | — | | `manual_transition_soak_matrix.sh` | dev-tool | Matrix generator for nightly stress windows | — | | `manual_transition_nightly_stress_runbook.sh` | dev-tool | Nightly stress entrypoint with failure snapshot templates | — | diff --git a/scripts/manual_transition_mixed_version_docker_harness.sh b/scripts/manual_transition_mixed_version_docker_harness.sh index 0937703fc..68a611611 100755 --- a/scripts/manual_transition_mixed_version_docker_harness.sh +++ b/scripts/manual_transition_mixed_version_docker_harness.sh @@ -25,9 +25,13 @@ COLD_IMAGE="${COLD_IMAGE:-${NEW_IMAGE}}" BASE_PORT="${BASE_PORT:-19400}" OUT_DIR="${OUT_DIR:-${PROJECT_ROOT}/target/manual-transition-1508-docker/$(date +%Y%m%dT%H%M%S)}" KEEP_UP=false -ROLLBACK_NEW2_TO_OLD=true +ROLLBACK_PHASE="${ROLLBACK_PHASE:-after-terminal}" +OLD_NODE_PHASE="${OLD_NODE_PHASE:-initial}" WAIT_TIMEOUT_SECS="${WAIT_TIMEOUT_SECS:-180}" POLL_SECONDS="${POLL_SECONDS:-180}" +TRANSITION_WORKERS="${TRANSITION_WORKERS:-2}" +TRANSITION_QUEUE_CAPACITY="${TRANSITION_QUEUE_CAPACITY:-64}" +FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT="${FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT:-false}" HOT_ACCESS_KEY="${HOT_ACCESS_KEY:-mvadmin}" HOT_SECRET_KEY="${HOT_SECRET_KEY:-mvsecret}" @@ -58,18 +62,31 @@ Options: --object-count Non-empty probe object count --tier Remote tier name --keep-up Leave Docker services running - --no-rollback Do not replace node2 with old image after job admission + --rollback-phase Rollback node2 timing: after-terminal, in-flight, none + --old-node-phase Old node1 timing: initial, before-job + --no-rollback Alias for --rollback-phase none -h, --help Show help Environment: PROJECT_NAME OLD_IMAGE NEW_IMAGE COLD_IMAGE BASE_PORT OUT_DIR KEEP_UP HOT_ACCESS_KEY HOT_SECRET_KEY COLD_ACCESS_KEY COLD_SECRET_KEY TIER_NAME TIER_BUCKET TIER_PREFIX JOB_BUCKET JOB_PREFIX OBJECT_COUNT - WAIT_TIMEOUT_SECS POLL_SECONDS AWS_SIGV4_SCOPE + ROLLBACK_PHASE WAIT_TIMEOUT_SECS POLL_SECONDS TRANSITION_WORKERS + TRANSITION_QUEUE_CAPACITY OLD_NODE_PHASE FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT AWS_SIGV4_SCOPE Artifacts: compose.yml, image inspect files, health/readiness logs, API responses, terminal status, old-node readback, container logs, summary.env. + +Result classifications: + strict_mixed_rollout_pass Real old/new images, non-empty completed transition, zero failures + baseline_tiered_storage_pass Same old/new image completed transition; useful baseline, not #1508 closure + blocked_manual_api_not_implemented Manual transition API returned 501 before job admission + blocked_manual_api_unavailable Manual transition API did not return a usable job_id + blocked_cluster_readiness_failed Docker cluster did not reach health/readiness before admission + blocked_empty_scan_or_lifecycle Job completed without lifecycle-matching transition work + blocked_manual_job_preempted_by_lifecycle_queue Lifecycle/immediate transition queued work before the job + strict_mixed_rollout_fail Mixed rollout ran but did not satisfy the strict #1508 gate USAGE } @@ -111,6 +128,28 @@ parse_positive_int() { fi } +validate_rollback_phase() { + case "$ROLLBACK_PHASE" in + after-terminal|in-flight|none) + ;; + *) + log_error "--rollback-phase must be one of: after-terminal, in-flight, none" + exit 1 + ;; + esac +} + +validate_old_node_phase() { + case "$OLD_NODE_PHASE" in + initial|before-job) + ;; + *) + log_error "--old-node-phase must be one of: initial, before-job" + exit 1 + ;; + esac +} + parse_args() { while [[ $# -gt 0 ]]; do case "$1" in @@ -150,8 +189,16 @@ parse_args() { KEEP_UP=true shift ;; + --rollback-phase) + ROLLBACK_PHASE="$(arg_value "$1" "${2:-}")" + shift 2 + ;; + --old-node-phase) + OLD_NODE_PHASE="$(arg_value "$1" "${2:-}")" + shift 2 + ;; --no-rollback) - ROLLBACK_NEW2_TO_OLD=false + ROLLBACK_PHASE=none shift ;; -h|--help) @@ -198,12 +245,18 @@ cleanup() { } write_compose_file() { + local node1_image COMPOSE_FILE="${OUT_DIR}/compose.yml" + node1_image="$OLD_IMAGE" + if [[ "$OLD_NODE_PHASE" == "before-job" ]]; then + node1_image="$NEW_IMAGE" + fi cat >"$COMPOSE_FILE" <"${OUT_DIR}/run-response.json" + curl_hot POST "$(hot_endpoint 2)/rustfs/admin/v3/ilm/transition/run?${query}" \ + -o "${OUT_DIR}/run-response.json" \ + -w "%{http_code}\n" >"${OUT_DIR}/run-response.http_code" || true + response="$(cat "${OUT_DIR}/run-response.json" 2>/dev/null || true)" + run_http_code="$(cat "${OUT_DIR}/run-response.http_code" 2>/dev/null || true)" job_id="$(printf '%s' "$response" | jq -r '.job_id // empty')" if [[ -z "$job_id" ]]; then - log_error "manual transition response omitted job_id" + log_warn "manual transition response omitted job_id, http_code=${run_http_code:-unknown}" return 1 fi printf '%s\n' "$job_id" >"${OUT_DIR}/job-id.txt" @@ -451,19 +516,25 @@ start_transition_job() { replace_node2_with_old_image() { local network network="$(network_name)" - log_info "Replacing node2 with old image ${OLD_IMAGE} for in-flight rollback readback" + log_info "Replacing node2 with old image ${OLD_IMAGE} for ${ROLLBACK_PHASE} rollback readback" compose stop node2 >/dev/null compose rm -f node2 >/dev/null docker run -d \ --name "${PROJECT_NAME}-node2-rollback-old" \ --network "$network" \ --network-alias node2 \ + --user 0:0 \ -p "$((BASE_PORT + 2)):9000" \ -e RUSTFS_ADDRESS=:9000 \ -e RUSTFS_ACCESS_KEY="$HOT_ACCESS_KEY" \ -e RUSTFS_SECRET_KEY="$HOT_SECRET_KEY" \ -e RUSTFS_VOLUMES="$(hot_volumes)" \ -e RUSTFS_SCANNER_ENABLED=false \ + -e RUSTFS_SCANNER_CYCLE=3600 \ + -e RUSTFS_SCANNER_START_DELAY_SECS=3600 \ + -e RUSTFS_MAX_TRANSITION_WORKERS="$TRANSITION_WORKERS" \ + -e RUSTFS_TRANSITION_QUEUE_CAPACITY="$TRANSITION_QUEUE_CAPACITY" \ + -e RUSTFS_TEST_FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT="$FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT" \ -e RUSTFS_UNSAFE_BYPASS_DISK_CHECK=true \ -e RUSTFS_OBS_LOGGER_LEVEL=warn \ -v "${PROJECT_NAME}_node2_data_0:/data/rustfs0" \ @@ -473,6 +544,37 @@ replace_node2_with_old_image() { "$OLD_IMAGE" >/dev/null } +replace_node1_with_old_image() { + local network + network="$(network_name)" + log_info "Replacing node1 with old image ${OLD_IMAGE} before manual transition job" + compose stop node1 >/dev/null + compose rm -f node1 >/dev/null + docker run -d \ + --name "${PROJECT_NAME}-node1-before-job-old" \ + --network "$network" \ + --network-alias node1 \ + --user 0:0 \ + -p "$((BASE_PORT + 1)):9000" \ + -e RUSTFS_ADDRESS=:9000 \ + -e RUSTFS_ACCESS_KEY="$HOT_ACCESS_KEY" \ + -e RUSTFS_SECRET_KEY="$HOT_SECRET_KEY" \ + -e RUSTFS_VOLUMES="$(hot_volumes)" \ + -e RUSTFS_SCANNER_ENABLED=false \ + -e RUSTFS_SCANNER_CYCLE=3600 \ + -e RUSTFS_SCANNER_START_DELAY_SECS=3600 \ + -e RUSTFS_MAX_TRANSITION_WORKERS="$TRANSITION_WORKERS" \ + -e RUSTFS_TRANSITION_QUEUE_CAPACITY="$TRANSITION_QUEUE_CAPACITY" \ + -e RUSTFS_TEST_FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT="$FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT" \ + -e RUSTFS_UNSAFE_BYPASS_DISK_CHECK=true \ + -e RUSTFS_OBS_LOGGER_LEVEL=warn \ + -v "${PROJECT_NAME}_node1_data_0:/data/rustfs0" \ + -v "${PROJECT_NAME}_node1_data_1:/data/rustfs1" \ + -v "${PROJECT_NAME}_node1_data_2:/data/rustfs2" \ + -v "${PROJECT_NAME}_node1_data_3:/data/rustfs3" \ + "$OLD_IMAGE" >/dev/null +} + poll_terminal_status() { local job_id="$1" local status_url status_json terminal_state @@ -512,6 +614,85 @@ head_probe() { -w "%{http_code}\n" >"${OUT_DIR}/head-object.http_code" || true } +image_id() { + local file="$1" + jq -r '.[0].Id // ""' "$file" 2>/dev/null || true +} + +is_compat_readback_code() { + case "$1" in + 200|501) + return 0 + ;; + *) + return 1 + ;; + esac +} + +classify_result() { + local terminal_state="$1" + local transition_completed="$2" + local transition_failed="$3" + local tier_failure="$4" + local old_code="$5" + local rollback_code="$6" + local run_http_code="$7" + local lifecycle_config_found="$8" + local scanned="$9" + local eligible="${10}" + local skipped_already_transitioned="${11}" + local skipped_already_in_flight="${12}" + local old_image_id new_image_id images_are_mixed readback_ok + + if [[ -f "${OUT_DIR}/readiness-failed" ]]; then + printf 'blocked_cluster_readiness_failed\n' + return + fi + + old_image_id="$(image_id "${OUT_DIR}/old-image.inspect.json")" + new_image_id="$(image_id "${OUT_DIR}/new-image.inspect.json")" + images_are_mixed=false + if [[ "$OLD_IMAGE" != "$NEW_IMAGE" && -n "$old_image_id" && -n "$new_image_id" && "$old_image_id" != "$new_image_id" ]]; then + images_are_mixed=true + fi + + readback_ok=false + if is_compat_readback_code "$old_code"; then + if [[ "$ROLLBACK_PHASE" == "none" ]] || is_compat_readback_code "$rollback_code"; then + readback_ok=true + fi + fi + + if [[ "$run_http_code" == "501" ]]; then + printf 'blocked_manual_api_not_implemented\n' + return + fi + if [[ -z "$(cat "${OUT_DIR}/job-id.txt" 2>/dev/null || true)" ]]; then + printf 'blocked_manual_api_unavailable\n' + return + fi + if [[ "$terminal_state" == "completed" && ( "$lifecycle_config_found" != "true" || "$scanned" == "0" || "$eligible" == "0" || "$transition_completed" == "0" ) ]]; then + printf 'blocked_empty_scan_or_lifecycle\n' + return + fi + if [[ "$transition_completed" == "0" && "$eligible" != "0" && "$transition_failed" == "0" && "$tier_failure" == "0" ]]; then + if [[ "$skipped_already_transitioned" != "0" || "$skipped_already_in_flight" != "0" ]]; then + printf 'blocked_manual_job_preempted_by_lifecycle_queue\n' + return + fi + fi + if [[ "$terminal_state" == "completed" && "$transition_completed" != "0" && "$transition_failed" == "0" && "$tier_failure" == "0" ]]; then + if [[ "$images_are_mixed" == "true" && "$readback_ok" == "true" ]]; then + printf 'strict_mixed_rollout_pass\n' + return + fi + printf 'baseline_tiered_storage_pass\n' + return + fi + printf 'strict_mixed_rollout_fail\n' +} + collect_logs() { local service if [[ -z "$COMPOSE_FILE" || ! -f "$COMPOSE_FILE" ]]; then @@ -520,37 +701,63 @@ collect_logs() { for service in cold node1 node2 node3 node4; do compose logs --no-color "$service" >"${OUT_DIR}/${service}.log" 2>/dev/null || true done + docker logs "${PROJECT_NAME}-node1-before-job-old" >"${OUT_DIR}/node1-before-job-old.log" 2>/dev/null || true docker logs "${PROJECT_NAME}-node2-rollback-old" >"${OUT_DIR}/node2-rollback-old.log" 2>/dev/null || true } summarize() { - local terminal_state transition_completed tier_failure transition_failed old_code rollback_code + local terminal_state transition_completed tier_failure transition_failed old_code rollback_code run_http_code lifecycle_config_found scanned eligible skipped_already_transitioned skipped_already_in_flight queue_queued queue_active result_classification terminal_state="$(cat "${OUT_DIR}/terminal-state.txt" 2>/dev/null || true)" transition_completed="$(jq -r '.report.transition_completed // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" tier_failure="$(jq -r '.report.tier_failure // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" transition_failed="$(jq -r '.report.transition_failed // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" old_code="$(cat "${OUT_DIR}/old-node-status.http_code" 2>/dev/null || true)" rollback_code="$(cat "${OUT_DIR}/rollback-node2-status.http_code" 2>/dev/null || true)" + run_http_code="$(cat "${OUT_DIR}/run-response.http_code" 2>/dev/null || true)" + lifecycle_config_found="$(jq -r '.report.lifecycle_config_found // false' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf 'false')" + scanned="$(jq -r '.report.scanned // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + eligible="$(jq -r '.report.eligible // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + skipped_already_transitioned="$(jq -r '.report.skipped_already_transitioned // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + skipped_already_in_flight="$(jq -r '.report.skipped_already_in_flight // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + queue_queued="$(jq -r '.queue_snapshot.queued // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + queue_active="$(jq -r '.queue_snapshot.active // 0' "${OUT_DIR}/status-terminal.json" 2>/dev/null || printf '0')" + result_classification="$(classify_result "$terminal_state" "$transition_completed" "$transition_failed" "$tier_failure" "$old_code" "$rollback_code" "$run_http_code" "$lifecycle_config_found" "$scanned" "$eligible" "$skipped_already_transitioned" "$skipped_already_in_flight")" cat >"${OUT_DIR}/summary.env" <"${OUT_DIR}/cold-image.inspect.json" compose up -d - wait_cluster_ready + if ! wait_cluster_ready; then + printf 'cluster readiness failed before manual transition admission\n' >"${OUT_DIR}/readiness-failed" + collect_logs + summarize + return 1 + fi curl_cold PUT "$(cold_endpoint)/${TIER_BUCKET}" -o "${OUT_DIR}/create-cold-bucket.response" -w "%{http_code}\n" >"${OUT_DIR}/create-cold-bucket.http_code" create_bucket "$(hot_endpoint 2)" "$JOB_BUCKET" "${OUT_DIR}/create-hot-bucket" add_tier - put_lifecycle seed_objects - start_transition_job - if [[ "$ROLLBACK_NEW2_TO_OLD" == "true" ]]; then - replace_node2_with_old_image + put_lifecycle + if [[ "$OLD_NODE_PHASE" == "before-job" ]]; then + replace_node1_with_old_image + wait_http_ok "$(hot_endpoint 1)/health" "node1-old-live" || true + wait_http_ok "$(hot_endpoint 1)/health/ready" "node1-old-ready" || true + fi + if start_transition_job; then + if [[ "$ROLLBACK_PHASE" == "in-flight" ]]; then + replace_node2_with_old_image + fi + poll_terminal_status "$(cat "${OUT_DIR}/job-id.txt")" || true + if [[ "$ROLLBACK_PHASE" == "after-terminal" ]]; then + replace_node2_with_old_image + wait_http_ok "$(hot_endpoint 2)/health" "rollback-node2-live" || true + fi + capture_old_node_readback "$(cat "${OUT_DIR}/job-id.txt")" + head_probe + else + log_warn "Skipping terminal polling because no manual transition job was admitted" fi - poll_terminal_status "$(cat "${OUT_DIR}/job-id.txt")" - capture_old_node_readback "$(cat "${OUT_DIR}/job-id.txt")" - head_probe collect_logs summarize } diff --git a/scripts/test_manual_transition_runbooks.sh b/scripts/test_manual_transition_runbooks.sh index 248a72ef3..9ad09909a 100755 --- a/scripts/test_manual_transition_runbooks.sh +++ b/scripts/test_manual_transition_runbooks.sh @@ -208,7 +208,16 @@ bash "$MIXED_DOCKER_HARNESS" --help >/tmp/manual_transition_mixed_version_docker rg -q "mixed_version_docker_harness" /tmp/manual_transition_mixed_version_docker_harness.help rg -q -- "--old-image" /tmp/manual_transition_mixed_version_docker_harness.help rg -q -- "--new-image" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q -- "--rollback-phase" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q -- "--old-node-phase" /tmp/manual_transition_mixed_version_docker_harness.help rg -q -- "--no-rollback" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "strict_mixed_rollout_pass" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "baseline_tiered_storage_pass" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "blocked_manual_api_not_implemented" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "blocked_cluster_readiness_failed" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "blocked_manual_job_preempted_by_lifecycle_queue" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "OLD_NODE_PHASE" /tmp/manual_transition_mixed_version_docker_harness.help +rg -q "FORCE_IMMEDIATE_TRANSITION_ENQUEUE_TIMEOUT" /tmp/manual_transition_mixed_version_docker_harness.help if bash "$FAILURE_SAMPLES" --endpoint http://127.0.0.1:9000 --sample >/tmp/manual_transition_failure_samples.err 2>&1; then echo "failure samples script should fail when --sample has no value" >&2 exit 1