diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index d018c2cc2..c9c0ca89f 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -45,8 +45,8 @@ pub mod bucket { pub use crate::bucket::lifecycle::bucket_lifecycle_ops::{ ExpiryState, LifecycleOps, RestoreRequestOps, TransitionState, TransitionedObject, apply_expiry_rule, apply_transition_rule, enqueue_expiry_for_existing_objects, enqueue_transition_for_existing_objects, - enqueue_transition_immediate, get_global_expiry_state, get_global_transition_state, init_background_expiry, - post_restore_opts, run_stale_multipart_upload_cleanup_once, validate_transition_tier, + enqueue_transition_immediate, expire_transitioned_object, get_global_expiry_state, get_global_transition_state, + init_background_expiry, post_restore_opts, run_stale_multipart_upload_cleanup_once, validate_transition_tier, }; } diff --git a/crates/scanner/tests/lifecycle_integration_test.rs b/crates/scanner/tests/lifecycle_integration_test.rs index eef3691fc..fb73d12c7 100644 --- a/crates/scanner/tests/lifecycle_integration_test.rs +++ b/crates/scanner/tests/lifecycle_integration_test.rs @@ -43,11 +43,12 @@ mod storage_api; use storage_api::lifecycle::{ BUCKET_LIFECYCLE_CONFIG, BucketOperations, BucketOptions, BucketVersioningSys, CompletePart, DiskAPI as _, DiskOption, - ECStore, Endpoint, EndpointServerPools, Endpoints, ListOperations as _, MakeBucketOptions, MultipartOperations as _, - ObjectIO as _, ObjectOperations as _, PoolEndpoints, ReadCloser, ReaderImpl, STORAGE_FORMAT_FILE, ScannerWarmBackend, - TierConfig, TierMinIO, TierType, TransitionOptions, WarmBackendGetOpts, build_transition_put_options, - enqueue_transition_for_existing_objects, get_bucket_metadata, get_global_tier_config_mgr, init_background_expiry, - init_bucket_metadata_sys, init_local_disks, new_disk, path2_bucket_object_with_base_path, update_bucket_metadata, + ECStore, EcstoreError, Endpoint, EndpointServerPools, Endpoints, IlmAction, LcEvent, LcEventSrc, ListOperations as _, + MakeBucketOptions, MultipartOperations as _, ObjectIO as _, ObjectOperations as _, PoolEndpoints, ReadCloser, ReaderImpl, + STORAGE_FORMAT_FILE, ScannerWarmBackend, TierConfig, TierMinIO, TierType, TransitionOptions, WarmBackendGetOpts, + build_transition_put_options, enqueue_transition_for_existing_objects, expire_transitioned_object, get_bucket_metadata, + get_global_tier_config_mgr, init_background_expiry, init_bucket_metadata_sys, init_local_disks, is_err_object_not_found, + is_err_version_not_found, new_disk, path2_bucket_object_with_base_path, update_bucket_metadata, }; static GLOBAL_ENV: OnceLock<(Vec, Arc)> = OnceLock::new(); @@ -603,6 +604,23 @@ struct MockStoredObject { #[derive(Clone, Default)] struct MockWarmBackend { objects: Arc>>, + // Ordered log of remote-tier mutating operations observed by this backend + // (e.g. `remove:`). Used by the expire/GET race regression test to + // assert that `expire_transitioned_object` never issues a synchronous + // remote-tier removal (the #3491 local-first ordering contract). + ops_log: Arc>>, +} + +#[allow(dead_code)] +impl MockWarmBackend { + async fn remote_remove_count(&self) -> usize { + self.ops_log + .lock() + .await + .iter() + .filter(|op| op.starts_with("remove:")) + .count() + } } impl MockWarmBackend { @@ -682,6 +700,7 @@ impl ScannerWarmBackend for MockWarmBackend { } async fn remove(&self, object: &str, _rv: &str) -> Result<(), std::io::Error> { + self.ops_log.lock().await.push(format!("remove:{object}")); self.objects.lock().await.remove(object); Ok(()) } @@ -767,6 +786,169 @@ where mod serial_tests { use super::*; + /// Regression for rustfs#3491 (backlog#1148 ilm-2): the expire/GET race on a + /// transitioned (tiered) object. + /// + /// Before #3491, `expire_transitioned_object` removed the remote tier + /// version **before** deleting local metadata. A GET that raced into the + /// window between those two steps read local metadata still pointing at a + /// remote version that was already gone, so the tier fetch failed and the + /// client saw a spurious, user-visible error (`NoSuchVersion`). #3491 + /// flipped the ordering: local metadata is deleted **first** (the object + /// becomes atomically unreachable) and remote-tier cleanup is deferred to + /// persisted free-version recovery -- so no live local metadata ever points + /// at an already-removed remote version. + /// + /// This test pins the FIXED contract two complementary ways, both + /// revert-proof (reverting to remote-first ordering turns them red): + /// + /// 1. Ordering (deterministic): immediately after + /// `expire_transitioned_object` returns, the remote tier object is still + /// present and the mock recorded **zero** remote `remove` calls -- + /// proving the local delete happened with no synchronous remote removal + /// (local-first). Remote-first ordering loses the object and records a + /// `remove`. + /// 2. Concurrent GET (user-visible): a tight GET loop runs concurrently + /// with the expiry; every observation must be either a full, correct + /// body (GET won) or a clean object/version-not-found (expiry won). A + /// tier-fetch failure -- the #3491 symptom -- is never tolerated. + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + #[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-2)"] + async fn test_expire_transitioned_object_never_races_concurrent_get() { + let (_disk_paths, ecstore) = setup_isolated_test_env(false).await; + + let tier_name = format!("COLDTIER{}", &Uuid::new_v4().simple().to_string()[..8]).to_uppercase(); + let backend = register_mock_tier(&tier_name).await; + + let bucket_name = format!("test-expire-get-race-{}", &Uuid::new_v4().simple().to_string()[..8]); + let object_name = "test/race-object.bin"; + // Multi-block payload so the transitioned GET streams from the remote + // tier rather than serving an inline fast-path. + let payload: Vec = (0..64 * 1024).map(|i| (i % 251) as u8).collect(); + + create_test_bucket(&ecstore, bucket_name.as_str()).await; + set_bucket_lifecycle_transition_with_tier(bucket_name.as_str(), &tier_name) + .await + .expect("Failed to set lifecycle configuration"); + + upload_test_object(&ecstore, bucket_name.as_str(), object_name, &payload).await; + enqueue_transition_for_existing_objects(ecstore.clone(), bucket_name.as_str()) + .await + .expect("Failed to enqueue transition for existing objects"); + + let transitioned = wait_for_transition(&ecstore, bucket_name.as_str(), object_name, TRANSITION_WAIT_TIMEOUT) + .await + .expect("object should transition before expiry"); + let remote_object = transitioned.transitioned_object.name.clone(); + assert!( + backend.objects.lock().await.contains_key(&remote_object), + "transitioned remote object should exist in the mock tier before expiry" + ); + assert_eq!( + backend.remote_remove_count().await, + 0, + "no remote-tier removal should have happened before expiry" + ); + + // The ObjectInfo the scanner would hand to the expiry action. + let oi = ecstore + .get_object_info(bucket_name.as_str(), object_name, &ObjectOptions::default()) + .await + .expect("Failed to load transitioned object info"); + + // Concurrent GET loop: hammer GET while the expiry runs. Every outcome + // must be a full correct body or a clean not-found -- never a tier-fetch + // failure. + let get_store = ecstore.clone(); + let get_bucket = bucket_name.clone(); + let get_object = object_name.to_string(); + let expected = payload.clone(); + let get_loop = tokio::spawn(async move { + let mut saw_full_body = 0usize; + let mut saw_not_found = 0usize; + for _ in 0..400 { + match get_store + .get_object_reader( + get_bucket.as_str(), + get_object.as_str(), + None, + http::HeaderMap::new(), + &ObjectOptions::default(), + ) + .await + { + Ok(mut reader) => { + let mut data = Vec::new(); + match reader.stream.read_to_end(&mut data).await { + Ok(_) => { + assert_eq!( + data, expected, + "a successful GET during expiry must return the complete, correct body" + ); + saw_full_body += 1; + } + Err(err) => { + panic!("GET during expiry streamed a truncated/failed body (expire/GET race regression): {err:?}") + } + } + } + Err(err) => { + let ec: &EcstoreError = &err; + assert!( + is_err_object_not_found(ec) || is_err_version_not_found(ec), + "GET during expiry may only fail with a clean object/version-not-found (expiry won \ + the race); a tier-fetch failure is the #3491 regression: {err:?}" + ); + saw_not_found += 1; + } + } + tokio::task::yield_now().await; + } + (saw_full_body, saw_not_found) + }); + + // Run the exact expiry action the scanner drives for a transitioned + // current version. + let lc_event = LcEvent { + action: IlmAction::DeleteAction, + ..Default::default() + }; + expire_transitioned_object(ecstore.clone(), &oi, &lc_event, &LcEventSrc::Scanner) + .await + .expect("expire_transitioned_object should succeed"); + + // --- Ordering contract (deterministic revert-proof) ---------------- + // #3491 defers remote cleanup to free-version recovery, so immediately + // after expiry the remote object is still present and NO synchronous + // remote `remove` was issued. Reverting to remote-first ordering makes + // both assertions fail. + assert_eq!( + backend.remote_remove_count().await, + 0, + "expire_transitioned_object must NOT issue a synchronous remote-tier removal (local-first \ + ordering, #3491); remote cleanup is deferred to free-version recovery" + ); + assert!( + backend.objects.lock().await.contains_key(&remote_object), + "remote tier object must still exist immediately after expiry (deferred cleanup, #3491)" + ); + + // Local metadata is gone: the object is atomically unreachable. + assert!( + wait_for_object_absence(&ecstore, bucket_name.as_str(), object_name, Duration::from_secs(5)).await, + "local metadata for the expired transitioned object should be gone" + ); + + // Drain the concurrent GET loop; its internal asserts already guarantee + // no #3491-style tier-fetch failure was ever observed. + let (saw_full_body, saw_not_found) = get_loop.await.expect("concurrent GET loop task panicked"); + assert!( + saw_full_body + saw_not_found > 0, + "the concurrent GET loop should have observed at least one GET outcome" + ); + } + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] #[serial] #[ignore = "FAILING on main: excluded from the serial ILM lane pending a fix, see rustfs/backlog#1148 (ilm-1 partial)"] diff --git a/crates/scanner/tests/storage_api/mod.rs b/crates/scanner/tests/storage_api/mod.rs index 106ecd9da..2a59dffb9 100644 --- a/crates/scanner/tests/storage_api/mod.rs +++ b/crates/scanner/tests/storage_api/mod.rs @@ -13,8 +13,9 @@ // limitations under the License. pub(crate) use rustfs_ecstore::api::bucket::lifecycle::{ - bucket_lifecycle_ops::{enqueue_transition_for_existing_objects, init_background_expiry}, - lifecycle::TransitionOptions, + bucket_lifecycle_audit::LcEventSrc, + bucket_lifecycle_ops::{enqueue_transition_for_existing_objects, expire_transitioned_object, init_background_expiry}, + lifecycle::{Event as LcEvent, IlmAction, TransitionOptions}, }; pub(crate) use rustfs_ecstore::api::bucket::metadata::BUCKET_LIFECYCLE_CONFIG; pub(crate) use rustfs_ecstore::api::bucket::metadata_sys::{ @@ -24,6 +25,7 @@ pub(crate) use rustfs_ecstore::api::bucket::versioning_sys::BucketVersioningSys; pub(crate) use rustfs_ecstore::api::capacity::path2_bucket_object_with_base_path; pub(crate) use rustfs_ecstore::api::client::transition_api::{ReadCloser, ReaderImpl}; pub(crate) use rustfs_ecstore::api::disk::{DiskAPI, DiskOption, STORAGE_FORMAT_FILE, endpoint::Endpoint, new_disk}; +pub(crate) use rustfs_ecstore::api::error::{Error as EcstoreError, is_err_object_not_found, is_err_version_not_found}; pub(crate) use rustfs_ecstore::api::layout::{EndpointServerPools, Endpoints, PoolEndpoints}; pub(crate) use rustfs_ecstore::api::runtime::global_tier_config_mgr as get_global_tier_config_mgr; pub(crate) use rustfs_ecstore::api::storage::{ECStore, init_local_disks}; @@ -40,10 +42,11 @@ pub(crate) mod lifecycle { }; pub(crate) use super::{ - BUCKET_LIFECYCLE_CONFIG, BucketVersioningSys, DiskAPI, DiskOption, ECStore, Endpoint, EndpointServerPools, Endpoints, - PoolEndpoints, ReadCloser, ReaderImpl, STORAGE_FORMAT_FILE, ScannerWarmBackend, TierConfig, TierMinIO, TierType, - TransitionOptions, WarmBackendGetOpts, build_transition_put_options, enqueue_transition_for_existing_objects, - get_bucket_metadata, get_global_tier_config_mgr, init_background_expiry, init_bucket_metadata_sys, init_local_disks, + BUCKET_LIFECYCLE_CONFIG, BucketVersioningSys, DiskAPI, DiskOption, ECStore, EcstoreError, Endpoint, EndpointServerPools, + Endpoints, IlmAction, LcEvent, LcEventSrc, PoolEndpoints, ReadCloser, ReaderImpl, STORAGE_FORMAT_FILE, + ScannerWarmBackend, TierConfig, TierMinIO, TierType, TransitionOptions, WarmBackendGetOpts, build_transition_put_options, + enqueue_transition_for_existing_objects, expire_transitioned_object, get_bucket_metadata, get_global_tier_config_mgr, + init_background_expiry, init_bucket_metadata_sys, init_local_disks, is_err_object_not_found, is_err_version_not_found, new_disk, path2_bucket_object_with_base_path, update_bucket_metadata, }; } diff --git a/docs/operations/tier-ilm-debugging.md b/docs/operations/tier-ilm-debugging.md index 575b57c95..897cf1dc3 100644 --- a/docs/operations/tier-ilm-debugging.md +++ b/docs/operations/tier-ilm-debugging.md @@ -83,7 +83,11 @@ was transitioned to a non-versioned tier bucket and no versionId must be sent. remote-tier cleanup is driven by persisted free-version recovery (`crates/ecstore/src/bucket/lifecycle/tier_free_version_recovery.rs`). **Invariant — keep local-first ordering**: never remove a remote tier - version while live local metadata still points at it. + version while live local metadata still points at it. Regression test: + `serial_tests::test_expire_transitioned_object_never_races_concurrent_get` + in `crates/scanner/tests/lifecycle_integration_test.rs` (runs in the CI + ILM Integration serial lane) pins both the local-first ordering and the + "concurrent GET never sees `NoSuchVersion`" contract. - Nil-UUID versionId sent to tier (`NoSuchVersion`): reading code used `Uuid::from_slice(..).unwrap_or_default()`, converting an empty metadata value into `Uuid::nil()`. Fixed by the `and_then`/`filter` pattern above.