diff --git a/.github/workflows/rustfs-heal-test.yml b/.github/workflows/rustfs-heal-test.yml index 995cc9b23..666c83b4c 100644 --- a/.github/workflows/rustfs-heal-test.yml +++ b/.github/workflows/rustfs-heal-test.yml @@ -54,9 +54,6 @@ env: jobs: heal-test: runs-on: smoke-testing - # Requirement: a failing suite must not fail the workflow; failures - # are filed to rustfs/backlog and the chain continues. - continue-on-error: true timeout-minutes: 480 # Standalone manual run, or one link of the nightly functional chain # (storage -> heal -> pool). Pool expansion no longer re-runs heal. diff --git a/.github/workflows/rustfs-kms-test.yml b/.github/workflows/rustfs-kms-test.yml index 642c4b5e7..8647c9268 100644 --- a/.github/workflows/rustfs-kms-test.yml +++ b/.github/workflows/rustfs-kms-test.yml @@ -49,7 +49,6 @@ env: jobs: kms-test: runs-on: smoke-testing - continue-on-error: true timeout-minutes: 420 if: ${{ github.event_name == 'workflow_dispatch' || github.event_name == 'repository_dispatch' }} steps: @@ -109,7 +108,6 @@ jobs: - name: Run KMS suite id: test - continue-on-error: true env: LOG_FILE: /tmp/rustfs-kms.log run: | diff --git a/.github/workflows/rustfs-performance-test.yml b/.github/workflows/rustfs-performance-test.yml index 6d960052c..ff3e2978d 100644 --- a/.github/workflows/rustfs-performance-test.yml +++ b/.github/workflows/rustfs-performance-test.yml @@ -84,9 +84,6 @@ env: jobs: performance-test: runs-on: pf-testing - # Requirement: a failing benchmark must not fail the workflow; - # failures are filed to rustfs/backlog. - continue-on-error: true timeout-minutes: 900 # Run on manual dispatch, or when the nightly build completed successfully. # Skipped when nightly failed. diff --git a/.github/workflows/rustfs-pool-expand-test.yml b/.github/workflows/rustfs-pool-expand-test.yml index d7002c446..3c9836b2d 100644 --- a/.github/workflows/rustfs-pool-expand-test.yml +++ b/.github/workflows/rustfs-pool-expand-test.yml @@ -76,9 +76,6 @@ jobs: pool-expansion-test: name: Pool expansion / decommission test runs-on: smoke-testing - # Requirement: a failing suite must not fail the workflow; failures - # are filed to rustfs/backlog and the chain continues. - continue-on-error: true timeout-minutes: 360 if: ${{ github.event_name == 'workflow_dispatch' || github.event_name == 'repository_dispatch' }} env: diff --git a/.github/workflows/rustfs-replication-test.yml b/.github/workflows/rustfs-replication-test.yml index b17864f72..74c57b6d5 100644 --- a/.github/workflows/rustfs-replication-test.yml +++ b/.github/workflows/rustfs-replication-test.yml @@ -62,9 +62,6 @@ env: jobs: replication-test: runs-on: smoke-testing - # A failed replication run must not break the chain or the workflow: the - # failure is reported to rustfs/backlog instead (see the issue step). - continue-on-error: true timeout-minutes: 360 if: ${{ github.event_name == 'workflow_dispatch' || github.event_name == 'repository_dispatch' }} steps: @@ -116,7 +113,6 @@ jobs: - name: Run replication suite id: test - continue-on-error: true env: LOG_FILE: /tmp/rustfs-replication.log run: | diff --git a/.github/workflows/rustfs-s3-compat-test.yml b/.github/workflows/rustfs-s3-compat-test.yml index d3fff002b..80856194c 100644 --- a/.github/workflows/rustfs-s3-compat-test.yml +++ b/.github/workflows/rustfs-s3-compat-test.yml @@ -37,7 +37,6 @@ env: jobs: s3-compat-test: runs-on: smoke-testing - continue-on-error: true timeout-minutes: 360 if: ${{ github.event_name == 'workflow_dispatch' || github.event_name == 'repository_dispatch' }} steps: @@ -88,7 +87,6 @@ jobs: - name: Run S3 compatibility suite id: test - continue-on-error: true env: LOG_FILE: /tmp/rustfs-s3-compat.log run: | diff --git a/.github/workflows/rustfs-storage-test.yml b/.github/workflows/rustfs-storage-test.yml index 1dceda80d..767f734dc 100644 --- a/.github/workflows/rustfs-storage-test.yml +++ b/.github/workflows/rustfs-storage-test.yml @@ -46,7 +46,6 @@ env: jobs: storage-test: runs-on: smoke-testing - continue-on-error: true timeout-minutes: 360 if: ${{ github.event_name == 'workflow_dispatch' || github.event_name == 'repository_dispatch' }} steps: @@ -97,7 +96,6 @@ jobs: - name: Run storage engine suite id: test - continue-on-error: true env: LOG_FILE: /tmp/rustfs-storage.log run: | diff --git a/.github/workflows/rustfs-tier-test.yml b/.github/workflows/rustfs-tier-test.yml index 4ac80e609..5d9a2c1d7 100644 --- a/.github/workflows/rustfs-tier-test.yml +++ b/.github/workflows/rustfs-tier-test.yml @@ -61,9 +61,6 @@ env: jobs: tier-test: runs-on: smoke-testing - # Requirement: a failing suite must not fail the workflow; failures - # are filed to rustfs/backlog and the chain continues. - continue-on-error: true timeout-minutes: 420 if: ${{ github.event_name == 'workflow_dispatch' || github.event_name == 'repository_dispatch' }} steps: diff --git a/.github/workflows/rustfs-upgrade-test.yml b/.github/workflows/rustfs-upgrade-test.yml index 0c8c72cd0..0b4c19af1 100644 --- a/.github/workflows/rustfs-upgrade-test.yml +++ b/.github/workflows/rustfs-upgrade-test.yml @@ -79,7 +79,6 @@ env: jobs: upgrade-test: runs-on: smoke-testing - continue-on-error: true timeout-minutes: 420 if: ${{ github.event_name == 'workflow_dispatch' || github.event_name == 'repository_dispatch' }} steps: @@ -142,7 +141,6 @@ jobs: - name: Run upgrade compatibility suite id: test - continue-on-error: true env: LOG_FILE: /tmp/rustfs-upgrade.log GH_TOKEN: ${{ secrets.PF_TESTING_GH_TOKEN }} diff --git a/crates/config/README.md b/crates/config/README.md index c450c05b0..2313a41ec 100644 --- a/crates/config/README.md +++ b/crates/config/README.md @@ -130,6 +130,21 @@ Scanner cycle budget controls: - timeout returns S3 `SlowDown`, so clients should use normal SDK retry handling. - this is not a fdatasync or group-commit switch. Track fdatasync batching separately with `rustfs_s3_put_object_rename_fdatasync_batch_files`. +## Remote tier timeout environment variables + +- `RUSTFS_TIER_REMOTE_CONNECT_TIMEOUT_SECS` + - remote tier TCP connect timeout. + - default is `10`. + - must be positive; zero fails tier client initialization, while an invalid integer is logged and falls back to the default. +- `RUSTFS_TIER_REMOTE_REQUEST_TIMEOUT_SECS` + - remote tier request timeout through response headers. + - default is `86400` so large transition uploads keep a production-safe budget. + - must be positive; zero fails tier client initialization, while an invalid integer is logged and falls back to the default. Very large values are accepted and act as a correspondingly long budget. +- `RUSTFS_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS` + - maximum idle time between remote tier response-body chunks. + - default is `60`; the timer resets only when non-empty body data keeps progressing. + - must be positive; zero fails tier client initialization, while an invalid integer is logged and falls back to the default. + ## Drive timeout environment variables - `RUSTFS_DRIVE_METADATA_TIMEOUT_SECS` diff --git a/crates/config/src/constants/object.rs b/crates/config/src/constants/object.rs index 9af74f2a3..7ec459afd 100644 --- a/crates/config/src/constants/object.rs +++ b/crates/config/src/constants/object.rs @@ -137,6 +137,28 @@ pub const DEFAULT_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED: bool = false; const _: () = assert!(!DEFAULT_TIER_REMOTE_VERSION_STATE_WRITE); const _: () = assert!(!DEFAULT_TIER_REMOTE_VERSION_STATE_FLEET_CONFIRMED); +/// Environment variable for remote tier TCP connect timeout in seconds. +pub const ENV_TIER_REMOTE_CONNECT_TIMEOUT_SECS: &str = "RUSTFS_TIER_REMOTE_CONNECT_TIMEOUT_SECS"; +/// Default remote tier TCP connect timeout in seconds. +pub const DEFAULT_TIER_REMOTE_CONNECT_TIMEOUT_SECS: u64 = 10; + +/// Environment variable for the remote tier request timeout in seconds. +/// +/// This bounds upload/download request progress through response headers. The +/// default is intentionally large so multi-TiB transition uploads keep their +/// previous production budget while black-hole remotes no longer wait forever. +pub const ENV_TIER_REMOTE_REQUEST_TIMEOUT_SECS: &str = "RUSTFS_TIER_REMOTE_REQUEST_TIMEOUT_SECS"; +/// Default remote tier request timeout in seconds. +pub const DEFAULT_TIER_REMOTE_REQUEST_TIMEOUT_SECS: u64 = 24 * 60 * 60; + +/// Environment variable for remote tier response-body idle timeout in seconds. +/// +/// The timer is re-armed on every non-empty response-body chunk, so slow but +/// progressing remotes can continue while silent response bodies are cancelled. +pub const ENV_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS: &str = "RUSTFS_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS"; +/// Default remote tier response-body idle timeout in seconds. +pub const DEFAULT_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS: u64 = 60; + /// Request the object-transaction fencing contract used by storage-owned /// cleanup receipts and lock-window optimizations. /// @@ -812,6 +834,16 @@ mod remote_version_state_tests { ); } + #[test] + fn remote_tier_timeout_env_names_are_stable() { + assert_eq!(super::ENV_TIER_REMOTE_CONNECT_TIMEOUT_SECS, "RUSTFS_TIER_REMOTE_CONNECT_TIMEOUT_SECS"); + assert_eq!(super::ENV_TIER_REMOTE_REQUEST_TIMEOUT_SECS, "RUSTFS_TIER_REMOTE_REQUEST_TIMEOUT_SECS"); + assert_eq!( + super::ENV_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS, + "RUSTFS_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS" + ); + } + #[test] fn data_movement_part_checksum_gate_uses_stable_environment_names() { assert_eq!(super::ENV_DATA_MOVEMENT_PART_CHECKSUMS_WRITE, "RUSTFS_DATA_MOVEMENT_PART_CHECKSUMS_WRITE"); diff --git a/crates/ecstore/src/api/mod.rs b/crates/ecstore/src/api/mod.rs index 9e6fd34be..fd66c897a 100644 --- a/crates/ecstore/src/api/mod.rs +++ b/crates/ecstore/src/api/mod.rs @@ -562,6 +562,12 @@ pub mod set_disk { pub mod test_util { pub use crate::bucket::quota::reservation::fail_next_quota_ledger_save_for_test; pub use crate::set_disk::{MultipartCommitBarrier, MultipartCommitPause, PutObjectCommitBarrier, PutObjectCommitPause}; + + /// Keep a namespace commit pending until the returned owner is dropped. + #[must_use] + pub fn hold_namespace_commit(store: &crate::store::ECStore) -> impl Send + Sync { + store.ctx.begin_namespace_commit() + } } } diff --git a/crates/ecstore/src/bucket/on_demand_migration/backfill.rs b/crates/ecstore/src/bucket/on_demand_migration/backfill.rs index ddcbce8da..3a0ccfd69 100644 --- a/crates/ecstore/src/bucket/on_demand_migration/backfill.rs +++ b/crates/ecstore/src/bucket/on_demand_migration/backfill.rs @@ -25,8 +25,8 @@ //! [`BACKFILL_SAVE_INTERVAL`], and at every page end, with an `If-Match` //! compare-and-set so a concurrent cancel or takeover is never overwritten. //! - The `continuation_token` only advances once every pull queued from the -//! page before it has reported back, so a crash re-lists at most one page -//! (already-present keys are then skipped, never re-pulled). +//! page before it has succeeded. After a failure it stays at that page, +//! so crash recovery cannot skip failed pulls (existing keys are skipped). //! - The owner holds a lease of [`BACKFILL_LEASE`] renewed by every save. The //! recovery loop ([`run_backfill_recovery_loop`]) scans the buckets this //! node has an ODM state for every [`BACKFILL_RECOVERY_INTERVAL`] and takes @@ -367,9 +367,8 @@ pub struct LocalBackfillObject { pub source_etag: Option, } -/// Receiver of one queued pull's report; `None` when the pull was coalesced -/// into one already running. -pub type PullReport = Option>; +/// Shared report of a new or coalesced pull; absent only when not admitted. +pub type PullReport = Option; /// Everything the job needs from its bucket, so the loop can run against a /// mock in unit tests. Production: [`BucketBackfillContext`]. @@ -1191,9 +1190,11 @@ impl Job { } async fn main_loop(&mut self) -> Result<(), Stop> { + let mut cursor = self.checkpoint.continuation_token.clone(); + let failed_at_resume = self.checkpoint.failed; loop { self.check_cancel()?; - let page = self.list_page().await?; + let page = self.list_page(cursor.as_deref()).await?; for object in &page.objects { self.check_cancel()?; self.checkpoint.listed += 1; @@ -1205,10 +1206,13 @@ impl Job { self.drain_ready(); self.tick(false).await?; } - // Only advance the cursor once every pull of this page reported - // back, so a takeover re-lists at most this page. + // A persisted cursor certifies successful work, not just listing + // progress. Keep it at the first failed page for crash recovery. self.drain_all().await?; - self.checkpoint.continuation_token = page.next_continuation_token.clone(); + cursor = page.next_continuation_token; + if self.checkpoint.failed == failed_at_resume { + self.checkpoint.continuation_token = cursor.clone(); + } self.tick(true).await?; if !page.is_truncated { return Ok(()); @@ -1223,7 +1227,7 @@ impl Job { } } - async fn list_page(&mut self) -> Result { + async fn list_page(&mut self, cursor: Option<&str>) -> Result { let mut attempt = 0; loop { while !self.context.source_available() { @@ -1231,7 +1235,7 @@ impl Job { self.tick(false).await?; } let prefix = self.checkpoint.prefix.clone(); - let token = self.checkpoint.continuation_token.clone(); + let token = cursor.map(str::to_string); match self .context .list_page(prefix.as_deref(), token.as_deref(), BACKFILL_LIST_PAGE_SIZE) @@ -1305,9 +1309,10 @@ impl Job { } loop { match self.context.enqueue(key) { - (EnqueueOutcome::Enqueued, report) => { + (EnqueueOutcome::Enqueued | EnqueueOutcome::Coalesced, report) => { self.checkpoint.enqueued += 1; - if let Some(rx) = report { + let rx = report.ok_or(Stop::Unavailable)?; + { let key = key.to_string(); self.outstanding.push(Box::pin(async move { (key, rx.await) })); } @@ -1322,11 +1327,6 @@ impl Job { ); return Ok(()); } - (EnqueueOutcome::Coalesced, _) => { - // Someone else pulls it; its result is not ours to count. - self.checkpoint.enqueued += 1; - return Ok(()); - } (EnqueueOutcome::QueueFull, _) => { // Wait, never drop: one completion frees a slot. if self.outstanding.is_empty() { @@ -1640,6 +1640,7 @@ mod tests { queue_capacity: usize, pending: Mutex)>>, fail_keys: HashSet, + coalesced: bool, auto_complete: AtomicBool, cancel: CancellationToken, config_updated_at: Mutex>, @@ -1667,6 +1668,7 @@ mod tests { queue_capacity: usize::MAX, pending: Mutex::new(Vec::new()), fail_keys: HashSet::new(), + coalesced: false, auto_complete: AtomicBool::new(true), cancel: CancellationToken::new(), config_updated_at: Mutex::new(Some(ts(1_700_000_000))), @@ -1746,7 +1748,12 @@ mod tests { } else { self.pending.lock().push((key.to_string(), tx)); } - (EnqueueOutcome::Enqueued, Some(rx)) + let outcome = if self.coalesced { + EnqueueOutcome::Coalesced + } else { + EnqueueOutcome::Enqueued + }; + (outcome, Some(futures::FutureExt::shared(rx))) } fn cancel_token(&self) -> CancellationToken { @@ -1912,7 +1919,7 @@ mod tests { #[tokio::test] async fn failed_pulls_are_counted_hashed_and_finish_with_failures() { let bucket = "backfill-failed"; - let mut context = MockContext::new(5, 1000); + let mut context = MockContext::new(5, 2); Arc::get_mut(&mut context) .expect("unshared") .fail_keys @@ -1927,12 +1934,52 @@ mod tests { .checkpoint; assert_eq!(cp.state, BackfillState::CompletedWithFailures); assert_eq!((cp.pulled, cp.failed), (4, 1)); + assert_eq!(cp.continuation_token.as_deref(), Some("2"), "retain the first failed page for recovery"); assert_eq!(cp.failed_keys, vec![key_hash("k/00002")]); let last = cp.last_error.expect("last error"); assert_eq!(last.class, "local_write"); assert_eq!(last.key_hash.as_deref(), Some(key_hash("k/00002").as_str())); } + #[tokio::test] + async fn coalesced_pulls_block_the_checkpoint_and_report_failures() { + let bucket = "backfill-coalesced"; + let mut context = MockContext::new(1, 1); + { + let ctx = Arc::get_mut(&mut context).expect("unshared"); + ctx.coalesced = true; + ctx.auto_complete = AtomicBool::new(false); + ctx.fail_keys.insert("k/00000".to_string()); + } + let (_dirs, store, runner) = runner_with("node-a", bucket, Arc::clone(&context)).await; + runner.start(bucket, BackfillRequest::default()).await.expect("start"); + tokio::time::timeout(Duration::from_secs(10), async { + while context.pending.lock().is_empty() { + tokio::task::yield_now().await; + } + }) + .await + .expect("job enqueued"); + assert!(runner.is_running_locally(bucket), "coalescing is not completion"); + let cp = read_checkpoint(&store, bucket) + .await + .expect("read") + .expect("checkpoint") + .checkpoint; + assert!(cp.state.is_active()); + assert!(cp.continuation_token.is_none()); + context.complete_pending(); + runner.wait_until_idle(bucket).await; + let cp = read_checkpoint(&store, bucket) + .await + .expect("read") + .expect("checkpoint") + .checkpoint; + assert_eq!(cp.state, BackfillState::CompletedWithFailures); + assert_eq!((cp.enqueued, cp.pulled, cp.failed), (1, 0, 1)); + assert_eq!(cp.failed_keys, vec![key_hash("k/00000")]); + } + #[tokio::test] async fn listing_failure_marks_the_job_failed_with_the_error_class() { let bucket = "backfill-list-error"; @@ -2145,6 +2192,68 @@ mod tests { assert_eq!(runner.recover_once().await.taken_over, 0, "a finished job is not recovered"); } + #[tokio::test] + async fn recovery_advances_past_historical_failures_but_pins_new_failures() { + let bucket = "backfill-takeover-failed"; + let mut context = MockContext::new(8, 2); + { + let ctx = Arc::get_mut(&mut context).expect("unshared"); + ctx.auto_complete = AtomicBool::new(false); + ctx.fail_keys.insert("k/00004".to_string()); + } + let (_dirs, store, runner) = runner_with("node-b", bucket, Arc::clone(&context)).await; + let crashed_at = OffsetDateTime::now_utc() - Duration::from_secs(300); + let mut crashed = BackfillCheckpoint::new(&BackfillRequest::default(), ts(1_700_000_000), "node-a", crashed_at); + crashed.continuation_token = Some("2".to_string()); + crashed.failed = 1; + crashed.record_failure("local_write", Some("k/00002"), crashed_at); + write_checkpoint(&store, bucket, &crashed, None) + .await + .expect("seed failed page with an expired lease"); + + assert_eq!(runner.recover_once().await.taken_over, 1); + for (page_start, durable_token, failures) in [(2, "2", 1), (4, "4", 1), (6, "4", 2)] { + tokio::time::timeout(Duration::from_secs(10), async { + loop { + if context.pending.lock().len() == 2 { + break; + } + tokio::task::yield_now().await; + } + }) + .await + .expect("resumed page enqueued before its reports complete"); + assert_eq!( + context.pending.lock().iter().map(|(key, _)| key.clone()).collect::>(), + vec![format!("k/{page_start:05}"), format!("k/{:05}", page_start + 1)] + ); + let cp = read_checkpoint(&store, bucket) + .await + .expect("read persisted page boundary") + .expect("checkpoint") + .checkpoint; + assert_eq!(cp.job_id, crashed.job_id); + assert_eq!(cp.owner.as_ref().map(|owner| owner.node.as_str()), Some("node-b")); + assert_eq!(cp.continuation_token.as_deref(), Some(durable_token)); + assert_eq!(cp.failed, failures); + context.complete_pending(); + } + runner.wait_until_idle(bucket).await; + let cp = read_checkpoint(&store, bucket) + .await + .expect("read completed checkpoint") + .expect("checkpoint") + .checkpoint; + assert_eq!(cp.state, BackfillState::CompletedWithFailures); + assert_eq!((cp.pulled, cp.failed), (5, 2)); + assert_eq!(cp.continuation_token.as_deref(), Some("4")); + assert_eq!(cp.failed_keys, vec![key_hash("k/00002"), key_hash("k/00004")]); + assert_eq!( + context.list_requests.lock().as_slice(), + &[Some("2".to_string()), Some("4".to_string()), Some("6".to_string())] + ); + } + #[tokio::test] async fn recovery_cancels_a_job_whose_config_changed_and_reclaims_own_node_jobs() { let bucket = "backfill-recovery-config"; diff --git a/crates/ecstore/src/bucket/on_demand_migration/list_through.rs b/crates/ecstore/src/bucket/on_demand_migration/list_through.rs index a2ee6fea3..dd5a6a236 100644 --- a/crates/ecstore/src/bucket/on_demand_migration/list_through.rs +++ b/crates/ecstore/src/bucket/on_demand_migration/list_through.rs @@ -37,6 +37,9 @@ pub const MAX_LIST_NO_PROGRESS_PAGES: u8 = 16; /// listing's own marker, so the decoder needs a positive signal before it /// treats an opaque token as a merged one. const LIST_THROUGH_TOKEN_TAG: &str = "odm-list"; +// Object keys cannot contain NUL (bucket::utils::is_valid_object_prefix), +// so this framing cannot collide with a local key used as an opaque marker. +const LIST_THROUGH_TOKEN_PREFIX: &str = "\0odm-list:"; /// Pages fetched per side per request: the first page, plus at most one refill /// when the first one was mostly consumed by the previous page. Two pages of @@ -91,8 +94,7 @@ pub struct MergePick { } /// The continuation-token envelope. Opaque to clients: it is serialized as -/// JSON and then base64-encoded by the same helper that encodes a plain local -/// marker, so the wire shape is `base64(json)`. +/// framed JSON and then base64-encoded by the same helper as a local marker. /// /// A `null` cursor with `done = false` means "list that side from the start"; /// `done = true` means the side is finished and must not be listed again. @@ -139,7 +141,7 @@ impl ListThroughToken { pub fn encode(&self) -> String { // The envelope is built here from owned strings, so serialization // cannot fail; the fallback keeps the signature infallible. - serde_json::to_string(self).unwrap_or_default() + format!("{LIST_THROUGH_TOKEN_PREFIX}{}", serde_json::to_string(self).unwrap_or_default()) } } @@ -163,21 +165,18 @@ pub enum ListThroughTokenError { /// Classifies an already base64-decoded continuation token. /// -/// Only a JSON object carrying the envelope marker is read as a merged token; +/// Only a framed JSON object is read as a merged token; /// anything else is a local marker, so a bucket that turns `list_through` off /// keeps paginating with the tokens it handed out. A token that *is* an /// envelope but was tampered with (unknown version, unknown field, truncated /// JSON) is an error, never a silent fallback. pub fn decode_continuation_token(decoded: &str) -> Result { - if !decoded.starts_with('{') { - return Ok(ListThroughCursor::Local(decoded.to_string())); - } - let Ok(value) = serde_json::from_str::(decoded) else { - // Not JSON at all: an object key may legitimately start with '{'. + let Some(payload) = decoded.strip_prefix(LIST_THROUGH_TOKEN_PREFIX) else { return Ok(ListThroughCursor::Local(decoded.to_string())); }; + let value = serde_json::from_str::(payload).map_err(|_| ListThroughTokenError::Malformed)?; if value.get("t").and_then(serde_json::Value::as_str) != Some(LIST_THROUGH_TOKEN_TAG) { - return Ok(ListThroughCursor::Local(decoded.to_string())); + return Err(ListThroughTokenError::Malformed); } match value.get("v").and_then(serde_json::Value::as_u64) { Some(version) if version == u64::from(LIST_THROUGH_TOKEN_VERSION) => { @@ -1052,9 +1051,9 @@ mod tests { assert_eq!(decode_continuation_token(&extra), Err(ListThroughTokenError::Malformed)); let truncated = &encoded[..encoded.len() - 3]; - assert_eq!(decode_continuation_token(truncated), Ok(ListThroughCursor::Local(truncated.to_string()))); + assert_eq!(decode_continuation_token(truncated), Err(ListThroughTokenError::Malformed)); - let no_version = "{\"t\":\"odm-list\"}"; + let no_version = "\0odm-list:{\"t\":\"odm-list\"}"; assert_eq!(decode_continuation_token(no_version), Err(ListThroughTokenError::Malformed)); } @@ -1311,6 +1310,13 @@ mod tests { #[test] fn a_plain_local_marker_stays_local() { + for marker in [ + r#"{"t":"odm-list","v":1}"#, + r#"{"t":"odm-list","v":2,"local_done":true}"#, + r#"{"t":"odm-list"}"#, + ] { + assert_eq!(decode_continuation_token(marker), Ok(ListThroughCursor::Local(marker.to_string()))); + } assert_eq!( decode_continuation_token("photos/2024/01.jpg"), Ok(ListThroughCursor::Local("photos/2024/01.jpg".to_string())) diff --git a/crates/ecstore/src/bucket/on_demand_migration/pull.rs b/crates/ecstore/src/bucket/on_demand_migration/pull.rs index 60f7145a2..df3dd6462 100644 --- a/crates/ecstore/src/bucket/on_demand_migration/pull.rs +++ b/crates/ecstore/src/bucket/on_demand_migration/pull.rs @@ -46,10 +46,10 @@ use super::stats::{PullFailureReason, PullPath}; use super::sys::{BucketOdmState, OnDemandMigrationSys, PullError, PullOutcome, PullSlot}; use async_trait::async_trait; use bytes::Bytes; -use futures::{Stream, StreamExt}; +use futures::{FutureExt, Stream, StreamExt, future::Shared}; use parking_lot::Mutex; use rand::RngExt; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::fmt; use std::io; use std::pin::Pin; @@ -133,6 +133,8 @@ pub enum QueuedPullOutcome { Failed(PullError), } +pub type QueuedPullReport = Shared>; + /// Result of [`PullQueue::enqueue`]. #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub enum EnqueueOutcome { @@ -251,6 +253,7 @@ pub struct WriteBackRequest { pub preserve_etag: bool, /// `policy.emit_events`. pub emit_events: bool, + pub respect_delete_marker: bool, /// Source tags to copy (`policy.copy_tags`), `None` to skip. pub tags: Option>, } @@ -266,6 +269,7 @@ impl WriteBackRequest { pulled_at: OffsetDateTime::now_utc(), preserve_etag: config.policy.preserve_etag, emit_events: config.policy.emit_events, + respect_delete_marker: config.policy.respect_local_delete_marker, tags, } } @@ -830,7 +834,7 @@ pub struct PullQueue { bucket: String, tx: mpsc::Sender, /// Keys queued or running; the job removes its key when it ends. - pending: Mutex>, + pending: Mutex>, capacity: usize, cancel: CancellationToken, stats: Arc, @@ -869,7 +873,7 @@ impl PullQueue { let queue = Arc::new(Self { bucket: state.bucket().to_string(), tx, - pending: Mutex::new(HashSet::new()), + pending: Mutex::new(HashMap::new()), capacity, cancel: state.cancel_token(), stats: Arc::clone(state.stats()), @@ -903,29 +907,24 @@ impl PullQueue { self.enqueue_with_report(key, reason).0 } - /// [`Self::enqueue`] that also hands back the job's report channel when - /// a new job was queued (`Coalesced` pulls report to their first - /// requester only). - pub fn enqueue_with_report( - &self, - key: &str, - reason: PullReason, - ) -> (EnqueueOutcome, Option>) { + /// [`Self::enqueue`] with a shared report, including for coalesced pulls. + pub fn enqueue_with_report(&self, key: &str, reason: PullReason) -> (EnqueueOutcome, Option) { if self.cancel.is_cancelled() { return (EnqueueOutcome::Unavailable, None); } let mut pending = self.pending.lock(); - if pending.contains(key) { - return (EnqueueOutcome::Coalesced, None); + if let Some(report) = pending.get(key) { + return (EnqueueOutcome::Coalesced, Some(report.clone())); } let (report_tx, report_rx) = oneshot::channel(); + let report_rx = report_rx.shared(); match self.tx.try_send(PullJob { key: key.to_string(), reason, report: Some(report_tx), }) { Ok(()) => { - pending.insert(key.to_string()); + pending.insert(key.to_string(), report_rx.clone()); (EnqueueOutcome::Enqueued, Some(report_rx)) } Err(TrySendError::Full(_)) => { @@ -1072,7 +1071,7 @@ impl BucketOdmState { self: &Arc, key: &str, reason: PullReason, - ) -> (EnqueueOutcome, Option>) { + ) -> (EnqueueOutcome, Option) { match self.pull_queue() { Some(queue) => queue.enqueue_with_report(key, reason), None => (EnqueueOutcome::Unavailable, None), @@ -1399,13 +1398,21 @@ mod tests { assert_eq!(queue.capacity(), 1024); let mut outcomes = HashMap::new(); + let mut shared_report = None; for _ in 0..100 { - *outcomes.entry(queue.enqueue("a", PullReason::RangeGet)).or_insert(0) += 1; + let (outcome, report) = queue.enqueue_with_report("a", PullReason::RangeGet); + *outcomes.entry(outcome).or_insert(0) += 1; + shared_report = report; } assert_eq!(outcomes.get(&EnqueueOutcome::Enqueued), Some(&1)); assert_eq!(outcomes.get(&EnqueueOutcome::Coalesced), Some(&99)); assert_eq!(queue.pending_keys(), 1); + assert_eq!( + shared_report.expect("coalesced report").await, + Ok(QueuedPullOutcome::Stored { size: 1000 }) + ); + wait_until("first pull to finish", || queue.pending_keys() == 0).await; assert_eq!(source.head_calls.load(Ordering::SeqCst), 1); assert_eq!(source.get_calls.load(Ordering::SeqCst), 1); @@ -1438,6 +1445,23 @@ mod tests { assert_eq!(queue.enqueue("a", PullReason::RangeGet), EnqueueOutcome::Unavailable); } + #[tokio::test] + async fn coalesced_enqueues_share_failure_reports() { + let sys = OnDemandMigrationSys::new(); + let state = enabled_state(&sys, &config()).await; + let source = MockSource::with_object("missing", 1000, BodyKind::Bytes(body_bytes(1000))); + let queue = PullQueue::start(Arc::clone(&state), source, Arc::new(MockWriteBack::default())); + let (first, first_report) = queue.enqueue_with_report("absent", PullReason::RangeGet); + let (second, second_report) = queue.enqueue_with_report("absent", PullReason::Backfill); + assert_eq!(first, EnqueueOutcome::Enqueued); + assert_eq!(second, EnqueueOutcome::Coalesced); + let (first, second) = tokio::join!(first_report.expect("leader report"), second_report.expect("coalesced report")); + assert_eq!(first, second); + assert!(matches!(first, Ok(QueuedPullOutcome::Failed(_)))); + sys.remove(BUCKET); + queue.wait_until_stopped().await; + } + #[tokio::test] async fn queue_full_is_reported_and_cancel_drains_without_leaking_tasks() { let sys = OnDemandMigrationSys::new(); @@ -1467,7 +1491,8 @@ mod tests { wait_until("dispatcher to wait for a slot", || state.stats().queue_depth() == 1).await; assert_eq!(queue.enqueue("c", PullReason::LargeObject), EnqueueOutcome::Enqueued); assert_eq!(queue.enqueue("d", PullReason::LargeObject), EnqueueOutcome::QueueFull); - assert_eq!(queue.enqueue("c", PullReason::LargeObject), EnqueueOutcome::Coalesced); + let (coalesced, canceled_report) = queue.enqueue_with_report("c", PullReason::LargeObject); + assert_eq!(coalesced, EnqueueOutcome::Coalesced); assert_eq!(queue.pending_keys(), 3); assert_eq!(failures(&state).get("queue_full"), Some(&1)); assert!(!queue.is_stopped()); @@ -1477,6 +1502,12 @@ mod tests { .await .expect("dispatcher and in-flight job must exit after cancel"); assert!(queue.is_stopped()); + assert!( + tokio::time::timeout(Duration::from_secs(5), canceled_report.expect("coalesced cancellation report")) + .await + .expect("cancellation closes the report") + .is_err() + ); assert_eq!(queue.pending_keys(), 0); assert_eq!(state.inflight_keys(), 0); assert_eq!(state.stats().inflight_pulls(), 0); diff --git a/crates/ecstore/src/bucket/on_demand_migration/source_client.rs b/crates/ecstore/src/bucket/on_demand_migration/source_client.rs index f06301036..a97cf5a0a 100644 --- a/crates/ecstore/src/bucket/on_demand_migration/source_client.rs +++ b/crates/ecstore/src/bucket/on_demand_migration/source_client.rs @@ -153,8 +153,8 @@ pub struct SourceClientSpec { /// Wire requests one logical source call may cost. The pull pipeline and /// the backfill job own the retry budget (`pull.rs` `PULL_MAX_RETRIES`, /// `backfill.rs` `LIST_MAX_RETRIES`) and the breaker counts logical calls, - /// so ODM declares [`RemoteS3RetryPolicy::Disabled`] and keeps one counted - /// failure equal to one request against a struggling source. + /// so ODM declares [`RemoteS3RetryPolicy::Disabled`]. An ambiguous HEAD + /// 404 additionally probes the bucket before declaring a key absent. pub retry: RemoteS3RetryPolicy, /// Bytes per second the pull pipeline may consume from this source; /// `None` means unlimited. Enforced by the consumer, not by this client. @@ -262,7 +262,7 @@ const THROTTLE_CODES: &[&str] = &[ "TooManyRequests", "RequestThrottled", ]; -const NOT_FOUND_CODES: &[&str] = &["NoSuchKey", "NotFound", "NoSuchBucket", "NoSuchVersion"]; +const NOT_FOUND_CODES: &[&str] = &["NoSuchKey"]; const ACCESS_DENIED_CODES: &[&str] = &[ "AccessDenied", "InvalidAccessKeyId", @@ -285,7 +285,6 @@ fn classify_status(status: u16, code: Option<&str>, message: String) -> SourceEr } } match status { - 404 => SourceError::NotFound, 401 | 403 => SourceError::AccessDenied, 429 | 503 => SourceError::Throttled, 500..=599 => SourceError::ServerError(status), @@ -631,8 +630,7 @@ impl SourceClient { } /// `config` must come from [`SourceClientSpec::endpoint_spec`], which is - /// where the retry policy that keeps one logical call equal to one wire - /// request is declared. + /// where the policy disabling SDK-level retries is declared. fn from_config_builder(config: aws_sdk_s3::config::Builder, endpoint: String, spec: &SourceClientSpec) -> Self { let client = S3Client::from_conf(config.interceptor(SourceProxyMarkerInterceptor::new()).build()); Self { @@ -754,15 +752,16 @@ impl SourceClient { #[async_trait::async_trait] impl SourceBackend for S3SourceBackend { async fn head(&self, key: &str) -> Result { - let output = self - .client - .head_object() - .bucket(&self.bucket) - .key(key) - .send() - .await - .map_err(classify_sdk_error)?; - source_head_from_head_output(output) + match self.client.head_object().bucket(&self.bucket).key(key).send().await { + Ok(output) => source_head_from_head_output(output), + Err(err) if err.raw_response().is_some_and(|response| response.status().as_u16() == 404) => { + // HEAD has no error body: a missing bucket must not poison + // the per-key negative cache as though only the key was absent. + self.probe().await?; + Err(SourceError::NotFound) + } + Err(err) => Err(classify_sdk_error(err)), + } } /// Streams the object; `range` is passed through as an HTTP `Range` @@ -809,8 +808,8 @@ impl SourceBackend for S3SourceBackend { .contents .unwrap_or_default() .into_iter() - .filter_map(s3_source_object) - .collect(); + .map(s3_source_object) + .collect::, _>>()?; let common_prefixes = output .common_prefixes .unwrap_or_default() @@ -849,14 +848,20 @@ impl SourceBackend for S3SourceBackend { } } -fn s3_source_object(object: SdkObject) -> Option { - let key = object.key?; +fn s3_source_object(object: SdkObject) -> Result { + let key = object + .key + .ok_or_else(|| SourceError::Other("source listing object has no key".to_string()))?; + let size = object + .size + .and_then(|size| u64::try_from(size).ok()) + .ok_or_else(|| SourceError::Other("source listing object has no valid size".to_string()))?; let etag = normalize_etag(object.e_tag); let is_multipart_etag = etag.as_deref().is_some_and(is_multipart_etag); - Some(SourceObject { + Ok(SourceObject { key, etag, - size: object.size.and_then(|size| u64::try_from(size).ok()).unwrap_or(0), + size, last_modified: system_time(object.last_modified), storage_class: object.storage_class.map(|class| class.as_str().to_string()), is_multipart_etag, @@ -1489,7 +1494,10 @@ mod tests { #[tokio::test] async fn source_error_classification_covers_every_class() { let cases: Vec<(Scripted, &str, bool)> = vec![ - (status(404, ""), "not_found", false), + (status(404, ""), "other", false), + (status(404, "NoSuchKey"), "not_found", false), + (status(404, "NoSuchBucket"), "other", false), + (status(404, "NoSuchVersion"), "other", false), (status(403, ACCESS_DENIED_BODY), "access_denied", false), (status(401, ""), "access_denied", false), (status(429, ""), "throttled", true), @@ -1512,14 +1520,35 @@ mod tests { } } - // HEAD carries no error body, so the classification must work from the - // status alone as well. - let (client, _) = scripted_client(&spec(None), vec![status(404, "")]).await; + let (client, requests) = scripted_client(&spec(None), vec![status(404, ""), status(200, "")]).await; assert!(matches!(client.head_object("missing").await, Err(SourceError::NotFound))); + assert_eq!(recorded(&requests).len(), 2, "ambiguous HEAD 404 must check the bucket"); + let (client, _) = scripted_client(&spec(None), vec![status(404, ""), status(404, "")]).await; + assert!(matches!(client.head_object("missing").await, Err(SourceError::Other(_)))); + let (client, _) = scripted_client(&spec(None), vec![status(404, ""), status(403, "")]).await; + assert!(matches!(client.head_object("missing").await, Err(SourceError::AccessDenied))); let (client, _) = scripted_client(&spec(None), vec![status(403, "")]).await; assert!(matches!(client.head_object("secret").await, Err(SourceError::AccessDenied))); } + #[test] + fn source_listing_rejects_missing_and_negative_sizes() { + for size in [None, Some(-1)] { + let object = SdkObject::builder().key("key").set_size(size).build(); + assert!(matches!(s3_source_object(object), Err(SourceError::Other(_)))); + } + assert!(matches!( + s3_source_object(SdkObject::builder().size(0).build()), + Err(SourceError::Other(_)) + )); + assert_eq!( + s3_source_object(SdkObject::builder().key("empty").size(0).build()) + .expect("empty object") + .size, + 0 + ); + } + #[tokio::test] async fn source_client_debug_redacts_credentials() { let (client, _) = scripted_client(&spec(Some("data/")), Vec::new()).await; diff --git a/crates/ecstore/src/object_api/types.rs b/crates/ecstore/src/object_api/types.rs index 701596cbf..81e61ca82 100644 --- a/crates/ecstore/src/object_api/types.rs +++ b/crates/ecstore/src/object_api/types.rs @@ -956,6 +956,9 @@ pub struct ObjectOptions { pub preserve_etag: Option, pub metadata_chg: bool, pub http_preconditions: Option, + /// Internal create-only writes may also preserve an acknowledged deletion. + /// Evaluated with `http_preconditions` under the namespace commit lock. + pub preserve_delete_marker: bool, pub delete_replication: Option, pub delete_replication_config_snapshot: Option>, diff --git a/crates/ecstore/src/runtime/instance.rs b/crates/ecstore/src/runtime/instance.rs index ad65354ed..fd4007698 100644 --- a/crates/ecstore/src/runtime/instance.rs +++ b/crates/ecstore/src/runtime/instance.rs @@ -78,6 +78,21 @@ pub(crate) struct ScannerPublicationLeaseEntry { pub(crate) _operation_guard: OwnedRwLockReadGuard<()>, } +pub(crate) struct NamespaceCommitGuard { + ctx: Arc, + counted: bool, +} + +impl Drop for NamespaceCommitGuard { + fn drop(&mut self) { + if self.counted { + // Publish the new generation before a zero-pending publication probe. + self.ctx.advance_namespace_commit_generation(); + self.ctx.namespace_commits.fetch_sub(1, Ordering::AcqRel); + } + } +} + /// Runtime state owned by a single `ECStore` instance. /// /// This is intentionally minimal in the first migration slice; subsequent @@ -209,9 +224,13 @@ pub struct InstanceContext { /// Last storage-owned movement snapshot observed under the operation /// gate. SetDisks cache writers fail closed until ECStore refreshes it. scanner_publication_state: AtomicU8, + namespace_commits: AtomicU64, + namespace_commit_generation: AtomicU64, /// Resolves object-encryption material at the application boundary. object_encryption_resolver: OnceLock>, tier_delete_journal_recovery_stores: std::sync::Mutex>, + #[cfg(test)] + suppress_tier_delete_journal_recovery: bool, transition_transaction_recovery_stores: std::sync::Mutex>, tier_delete_journal_recovery_wakeup: tokio::sync::Notify, } @@ -256,8 +275,12 @@ impl InstanceContext { data_movement_generation_exhausted: AtomicBool::new(false), data_movement_generation_notify: Arc::new(Notify::new()), scanner_publication_state: AtomicU8::new(SCANNER_PUBLICATION_STATE_UNKNOWN), + namespace_commits: AtomicU64::new(0), + namespace_commit_generation: AtomicU64::new(0), object_encryption_resolver: OnceLock::new(), tier_delete_journal_recovery_stores: std::sync::Mutex::new(HashSet::new()), + #[cfg(test)] + suppress_tier_delete_journal_recovery: false, transition_transaction_recovery_stores: std::sync::Mutex::new(HashSet::new()), tier_delete_journal_recovery_wakeup: tokio::sync::Notify::new(), } @@ -385,6 +408,36 @@ impl InstanceContext { && self.scanner_publication_state.load(Ordering::Acquire) == SCANNER_PUBLICATION_STATE_ALLOWED } + pub(crate) fn begin_namespace_commit(self: &Arc) -> Arc { + let counted = self + .namespace_commits + .fetch_update(Ordering::AcqRel, Ordering::Acquire, |count| count.checked_add(1)) + .is_ok(); + if counted { + self.advance_namespace_commit_generation(); + } else { + self.namespace_commit_generation.store(u64::MAX, Ordering::Release); + } + Arc::new(NamespaceCommitGuard { + ctx: Arc::clone(self), + counted, + }) + } + + fn advance_namespace_commit_generation(&self) { + let _ = self + .namespace_commit_generation + .fetch_update(Ordering::AcqRel, Ordering::Acquire, |generation| Some(generation.saturating_add(1))); + } + + pub(crate) fn namespace_commit_generation(&self) -> u64 { + self.namespace_commit_generation.load(Ordering::Acquire) + } + + pub(crate) fn namespace_commits_pending(&self) -> bool { + self.namespace_commits.load(Ordering::Acquire) != 0 || self.namespace_commit_generation() == u64::MAX + } + pub(crate) fn set_scanner_publication_state(&self, blocked: bool) { self.scanner_publication_state.store( if blocked { @@ -640,12 +693,21 @@ impl InstanceContext { } pub(crate) fn mark_tier_delete_journal_recovery_started(&self, store_id: Uuid) -> bool { + #[cfg(test)] + if self.suppress_tier_delete_journal_recovery { + return false; + } self.tier_delete_journal_recovery_stores .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) .insert(store_id) } + #[cfg(test)] + pub(crate) fn suppress_tier_delete_journal_recovery_for_test(&mut self) { + self.suppress_tier_delete_journal_recovery = true; + } + pub(crate) fn mark_transition_transaction_recovery_started(&self, store_id: Uuid) -> bool { self.transition_transaction_recovery_stores .lock() @@ -756,6 +818,50 @@ pub fn bootstrap_ctx() -> Arc { mod tests { use super::*; + #[test] + fn namespace_commit_guards_are_instance_local_and_count_until_last_owner() { + let first = Arc::new(InstanceContext::new()); + let other = Arc::new(InstanceContext::new()); + first.set_scanner_publication_state(false); + other.set_scanner_publication_state(false); + assert!(first.scanner_publication_state_allowed()); + let one = first.begin_namespace_commit(); + let shared_owner = Arc::clone(&one); + let two = first.begin_namespace_commit(); + assert!(first.namespace_commits_pending()); + assert!(first.scanner_publication_state_allowed(), "pending writes must not block scan admission"); + assert_eq!(first.namespace_commit_generation(), 2); + assert!(!other.namespace_commits_pending()); + assert_eq!(other.namespace_commit_generation(), 0); + assert!(other.scanner_publication_state_allowed()); + drop(one); + assert_eq!(first.namespace_commit_generation(), 2); + drop(shared_owner); + assert!(first.namespace_commits_pending()); + assert_eq!(first.namespace_commit_generation(), 3); + drop(two); + assert!(!first.namespace_commits_pending()); + assert_eq!(first.namespace_commit_generation(), 4); + assert!(first.scanner_publication_state_allowed()); + } + + #[test] + fn namespace_commit_counter_exhaustion_keeps_publication_blocked() { + for (count, generation) in [(0, u64::MAX - 1), (u64::MAX, 0)] { + let ctx = Arc::new(InstanceContext::new()); + ctx.set_scanner_publication_state(false); + ctx.namespace_commits.store(count, Ordering::Release); + ctx.namespace_commit_generation.store(generation, Ordering::Release); + let guard = ctx.begin_namespace_commit(); + assert!(ctx.namespace_commits_pending()); + assert_eq!(ctx.namespace_commit_generation(), u64::MAX); + drop(guard); + assert!(ctx.namespace_commits_pending()); + assert_eq!(ctx.namespace_commit_generation(), u64::MAX); + assert_eq!(ctx.namespace_commits.load(Ordering::Acquire), count); + } + } + // The SetupType inputs must derive the exact (is_erasure, // is_dist_erasure, is_erasure_sd) triples that the original three // process-global erasure bools produced via update_erasure_type(). @@ -1073,6 +1179,12 @@ mod tests { assert!(!ctx_a.mark_tier_delete_journal_recovery_started(store_a)); assert!(ctx_a.mark_tier_delete_journal_recovery_started(store_b)); assert!(ctx_b.mark_tier_delete_journal_recovery_started(store_a)); + + let mut manual_ctx = InstanceContext::new(); + manual_ctx.suppress_tier_delete_journal_recovery_for_test(); + assert!(!manual_ctx.mark_tier_delete_journal_recovery_started(store_a)); + assert!(!manual_ctx.mark_tier_delete_journal_recovery_started(store_b)); + assert!(ctx_b.mark_tier_delete_journal_recovery_started(store_b)); } #[test] diff --git a/crates/ecstore/src/services/tier/tier.rs b/crates/ecstore/src/services/tier/tier.rs index af24423fa..5bb35f956 100644 --- a/crates/ecstore/src/services/tier/tier.rs +++ b/crates/ecstore/src/services/tier/tier.rs @@ -3541,7 +3541,7 @@ impl TierConfigMgr { // Get tier configuration and create new driver let tier_config = self.tiers.get(tier_name).ok_or_else(|| ERR_TIER_NOT_FOUND.clone())?; - let driver = new_warm_backend(tier_config, false).await?; + let driver = construct_warm_backend(tier_config).await?; self.replace_driver(tier_name, driver)?; Ok(self @@ -4486,6 +4486,11 @@ impl TierConfigMgr { let committed_coordinator_intent = committed_tier_mutation_intent(coordinator_intent.as_ref(), &committed_config_etag) .map_err(TierConfigUpdateError::Save)?; + // Persist Committed before notifying refresh; a Prepared disk record + // would restore the prepared block and invalidate our publish allowance. + let coordinator_commit = + commit_coordinator_tier_mutation_intent(api.clone(), coordinator_intent.as_ref(), &committed_config_etag) + .await; if let Some(intent) = committed_coordinator_intent.as_ref() { TierConfigMgr::apply_committed_mutation_intent_block(&handle, intent) .await @@ -4496,9 +4501,9 @@ impl TierConfigMgr { .map_err(TierConfigUpdateError::Publish)?, ); } - commit_coordinator_tier_mutation_intent(api.clone(), coordinator_intent.as_ref(), &committed_config_etag) - .await - .map_err(TierConfigUpdateError::Save)?; + // Config is already saved: retain the committed fence and wake recovery + // even when the coordinator commit failed or its outcome is unknown. + coordinator_commit.map_err(TierConfigUpdateError::Save)?; if coordinated_config_update { drop(update.take()); drop(config_lock.take()); @@ -10603,6 +10608,11 @@ mod tests { .expect_err("coordinator committed-state CAS failure must be observable"); assert!(matches!(err, TierConfigUpdateError::Save(_))); assert!(manager.read().await.tiers.contains_key("COLD-A")); + assert!(TierConfigMgr::has_committed_mutation_block(&manager).await); + let refresh = TierConfigMgr::mutation_refresh_notifier(&manager).await; + tokio::time::timeout(Duration::from_secs(1), refresh.notified()) + .await + .expect("failed coordinator commit must notify recovery after saving config"); let blocked = match TierConfigMgr::acquire_operation_lease(&manager, "COLD-A").await { Ok(_) => panic!("failed coordinator commit CAS must retain the local committed fence"), Err(err) => err, @@ -14329,6 +14339,12 @@ mod tests { after_commit: bool, } + #[derive(Debug, Default)] + struct CasCoordinatorCommitBarrier { + arrived: tokio::sync::Notify, + release: tokio::sync::Notify, + } + #[derive(Debug)] struct CasConfigStore { objects: tokio::sync::Mutex, String)>>, @@ -14341,6 +14357,7 @@ mod tests { fail_delete_prefix: tokio::sync::Mutex>, delete_log: tokio::sync::Mutex>, list_barrier: tokio::sync::Mutex>>, + coordinator_commit_barrier: tokio::sync::Mutex>>, intent_list_calls: AtomicUsize, fail_reference_walk: AtomicBool, reference_walk_send_count: AtomicUsize, @@ -14363,6 +14380,7 @@ mod tests { fail_delete_prefix: tokio::sync::Mutex::new(None), delete_log: tokio::sync::Mutex::new(Vec::new()), list_barrier: tokio::sync::Mutex::new(None), + coordinator_commit_barrier: tokio::sync::Mutex::new(None), intent_list_calls: AtomicUsize::new(0), fail_reference_walk: AtomicBool::new(false), reference_walk_send_count: AtomicUsize::new(0), @@ -14554,6 +14572,19 @@ mod tests { } let mut payload = Vec::new(); tokio::io::AsyncReadExt::read_to_end(&mut data.stream, &mut payload).await?; + if object.starts_with(crate::services::tier::tier_mutation_intent::TIER_COORDINATOR_MUTATION_INTENT_RECORD_PREFIX) + && opts + .http_preconditions + .as_ref() + .and_then(HTTPPreconditions::if_match_value) + .is_some() + { + let barrier = self.coordinator_commit_barrier.lock().await.take(); + if let Some(barrier) = barrier { + barrier.arrived.notify_one(); + barrier.release.notified().await; + } + } let race_rewrite = if opts .http_preconditions .as_ref() @@ -15651,14 +15682,7 @@ mod tests { ); } - #[tokio::test] - async fn force_remove_and_save_bypasses_lifecycle_only_reference() { - // rustfs/rustfs#6832: reproduces the admin RemoveTier path (not just the lower-level - // reference-proof function) for a tier with zero transitioned objects but a lifecycle - // rule still pointing at it — the exact shape of - // `test_manual_transition_async_tier_failure_reports_terminal_partial` in e2e_test, - // which force-removes a tier a lifecycle rule still references to simulate a - // decommissioned backend. + async fn assert_lifecycle_only_reference_obeys_force(clear: bool, force: bool) { let store = Arc::new(CasConfigStore::default()); let tier = build_rustfs_tier("COLD-A"); let mut persisted = empty_mgr(); @@ -15699,22 +15723,55 @@ mod tests { let manager = TierConfigMgr::new(); manager.write().await.tiers.insert("COLD-A".to_string(), tier); - TierConfigMgr::remove_and_save_with(&manager, store.clone(), "COLD-A", true) - .await - .expect("force remove must bypass a lifecycle-config-only reference"); + let mutation = if clear { + TierCandidateMutation::Clear(force) + } else { + TierCandidateMutation::Remove("COLD-A".to_string(), force) + }; + let result = TIER_DRIVER_TEST_FACTORY + .scope( + healthy_driver_factory(), + TierConfigMgr::update_candidate_with_config_lock(&manager, store.clone(), mutation), + ) + .await; + if force { + result.expect("force mutation must bypass a lifecycle-config-only reference"); + } else { + let err = result.expect_err("non-force mutation must reject a lifecycle-only reference"); + let TierConfigUpdateError::Publish(err) = err else { + panic!("non-force mutation must fail during reference proof: {err:?}"); + }; + assert_eq!(err.code, ERR_TIER_BACKEND_IN_USE.code); + assert!(err.message.contains("move-current"), "{err}"); + } - assert!(!manager.read().await.tiers.contains_key("COLD-A")); - assert!( - !load_tier_config_for_update(store) + assert_eq!(manager.read().await.tiers.contains_key("COLD-A"), !force); + assert_eq!( + load_tier_config_for_update(store) .await .expect("config should still reload") .0 .tiers .contains_key("COLD-A"), - "force removal must persist the empty candidate" + !force, + "persisted state must match the force mutation result" ); } + #[tokio::test] + async fn remove_with_config_lock_obeys_force_for_lifecycle_only_reference() { + for force in [false, true] { + assert_lifecycle_only_reference_obeys_force(false, force).await; + } + } + + #[tokio::test] + async fn clear_with_config_lock_obeys_force_for_lifecycle_only_reference() { + for force in [false, true] { + assert_lifecycle_only_reference_obeys_force(true, force).await; + } + } + #[tokio::test] async fn zero_reference_proof_blocks_clear_before_config_save() { let store = Arc::new(CasConfigStore::default()); @@ -17255,6 +17312,98 @@ mod tests { assert_ne!(manager_a.read().await.empty(), manager_b.read().await.empty()); } + async fn assert_coordinator_commit_refresh_succeeds(mutation: TierCandidateMutation) { + let adding = matches!(mutation, TierCandidateMutation::Add(..)); + let manager = TierConfigMgr::new(); + let store = Arc::new(CasConfigStore::default()); + if !adding { + let mut persisted = empty_mgr(); + persisted.tiers.insert("COLD-A".to_string(), build_rustfs_tier("COLD-A")); + persisted + .save_tiering_config_if_current(store.clone(), None) + .await + .expect("existing tier fixture should persist"); + let mut guard = manager.write().await; + install_lease_backend(&mut guard, "COLD-A", LeaseTestBackend::ready("old")); + } + let barrier = Arc::new(CasCoordinatorCommitBarrier::default()); + *store.coordinator_commit_barrier.lock().await = Some(barrier.clone()); + let update_manager = manager.clone(); + let update_store = store.clone(); + let update = tokio::spawn(async move { + TIER_DRIVER_TEST_FACTORY + .scope( + healthy_driver_factory(), + TIER_MUTATION_TEST_PEERS.scope( + Vec::new(), + TierConfigMgr::update_candidate_with_config_lock(&update_manager, update_store, mutation), + ), + ) + .await + }); + tokio::time::timeout(Duration::from_secs(5), barrier.arrived.notified()) + .await + .expect("mutation should reach coordinator commit after saving config"); + assert_eq!( + load_tier_config_for_update(store.clone()) + .await + .expect("saved config should be readable before coordinator commit") + .0 + .tiers + .contains_key("COLD-A"), + adding + ); + assert_eq!( + TierConfigMgr::load_coordinator_mutation_intents(store.clone()) + .await + .expect("coordinator intent should remain readable")[0] + .state, + TierMutationIntentState::Prepared + ); + + let lock_requests = lock_unpoisoned(&store.lock_requests).len(); + // Also exercise an independently scheduled refresh while the durable + // coordinator record is still Prepared, before its commit notification. + TierConfigMgr::request_committed_mutation_refresh(&manager).await; + TIER_MUTATION_TEST_PEERS + .scope(Vec::new(), async { + let worker = TierConfigMgr::refresh_tier_config_handle_with(manager.clone(), store.clone()); + tokio::pin!(worker); + tokio::time::timeout(Duration::from_secs(5), async { + while lock_unpoisoned(&store.lock_requests).len() == lock_requests { + tokio::select! { + _ = &mut worker => panic!("refresh worker must remain available"), + _ = tokio::task::yield_now() => {} + } + } + }) + .await + .expect("refresh should reconcile the Prepared record before waiting for the config lock"); + barrier.release.notify_one(); + let result = tokio::time::timeout(Duration::from_secs(5), async { + tokio::select! { + _ = &mut worker => panic!("refresh worker must remain available"), + result = update => result.expect("tier mutation task should join"), + } + }) + .await + .expect("tier mutation should finish with refresh running"); + result.expect("saved tier mutation must publish successfully on the first attempt"); + }) + .await; + assert_eq!(manager.read().await.tiers.contains_key("COLD-A"), adding); + } + + #[tokio::test] + async fn tier_add_succeeds_with_refresh_during_coordinator_commit() { + assert_coordinator_commit_refresh_succeeds(TierCandidateMutation::Add(build_rustfs_tier("COLD-A"), true)).await; + } + + #[tokio::test] + async fn tier_remove_succeeds_with_refresh_during_coordinator_commit() { + assert_coordinator_commit_refresh_succeeds(TierCandidateMutation::Remove("COLD-A".to_string(), true)).await; + } + async fn committed_refresh_fixture(fail_cleanup: bool) -> (Arc>, Arc, uuid::Uuid) { let manager = TierConfigMgr::new(); { diff --git a/crates/ecstore/src/services/tier/warm_backend.rs b/crates/ecstore/src/services/tier/warm_backend.rs index b5cf4ab38..22134d744 100644 --- a/crates/ecstore/src/services/tier/warm_backend.rs +++ b/crates/ecstore/src/services/tier/warm_backend.rs @@ -37,7 +37,7 @@ use crate::services::tier::{ use bytes::Bytes; use http::StatusCode; use rustfs_s3_client::credentials::{Credentials, SignatureType, Static, Value}; -use rustfs_s3_client::transition_api::{BucketLookupType, Options, TransitionClient, TransitionCore}; +use rustfs_s3_client::transition_api::{BucketLookupType, Options, TransitionClient, TransitionClientTimeouts, TransitionCore}; use rustfs_s3_client::{ admin_handler_utils::AdminError, api_error_response::to_error_response, @@ -320,6 +320,27 @@ pub(crate) fn endpoint_authority(url: &url::Url) -> Result Duration { + Duration::from_secs(rustfs_utils::get_env_u64(env_key, default_secs)) +} + +pub(crate) fn transition_client_timeouts_from_env() -> TransitionClientTimeouts { + TransitionClientTimeouts::new( + transition_timeout_from_env( + rustfs_config::ENV_TIER_REMOTE_CONNECT_TIMEOUT_SECS, + rustfs_config::DEFAULT_TIER_REMOTE_CONNECT_TIMEOUT_SECS, + ), + transition_timeout_from_env( + rustfs_config::ENV_TIER_REMOTE_REQUEST_TIMEOUT_SECS, + rustfs_config::DEFAULT_TIER_REMOTE_REQUEST_TIMEOUT_SECS, + ), + transition_timeout_from_env( + rustfs_config::ENV_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS, + rustfs_config::DEFAULT_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS, + ), + ) +} + /// Build the [`WarmBackendS3`] shared by the S3-compatible warm backend providers. /// /// Credential, bucket, and endpoint validation run in this order because the @@ -350,6 +371,7 @@ pub(crate) async fn new_s3_compatible_warm_backend( signer_type: SignatureType::SignatureV4, ..Default::default() })); + let timeouts = transition_client_timeouts_from_env(); let opts = Options { creds, secure: u.scheme() == "https", @@ -362,7 +384,7 @@ pub(crate) async fn new_s3_compatible_warm_backend( // Run the SSRF guard after the host-presence check so a host-less endpoint // keeps this constructor's stable error text. (params.validate_endpoint)(&u).map_err(|err| std::io::Error::other(format!("tier endpoint is not allowed: {err}")))?; - let client = TransitionClient::new(&endpoint, opts, params.provider_tag).await?; + let client = TransitionClient::new_with_timeouts(&endpoint, opts, params.provider_tag, timeouts).await?; let client = Arc::new(client); let core = TransitionCore(Arc::clone(&client)); diff --git a/crates/ecstore/src/services/tier/warm_backend_s3.rs b/crates/ecstore/src/services/tier/warm_backend_s3.rs index b830ea7f2..268f28596 100644 --- a/crates/ecstore/src/services/tier/warm_backend_s3.rs +++ b/crates/ecstore/src/services/tier/warm_backend_s3.rs @@ -26,7 +26,7 @@ use crate::services::tier::{ tier_config::TierS3, warm_backend::{ TransitionCandidateIdentity, TransitionCandidateProbe, TransitionCandidateReconciler, WarmBackend, WarmBackendGetOpts, - build_transition_put_options, endpoint_authority, + build_transition_put_options, endpoint_authority, transition_client_timeouts_from_env, }, }; use http::HeaderMap; @@ -139,6 +139,7 @@ impl WarmBackendS3 { } else { return Err(std::io::Error::other("insufficient parameters for S3 backend authentication")); } + let timeouts = transition_client_timeouts_from_env(); let opts = Options { creds, secure: u.scheme() == "https", @@ -147,7 +148,7 @@ impl WarmBackendS3 { ..Default::default() }; let endpoint = endpoint_authority(&u)?; - let client = TransitionClient::new(&endpoint, opts, tier_type).await?; + let client = TransitionClient::new_with_timeouts(&endpoint, opts, tier_type, timeouts).await?; let client = Arc::new(client); let core = TransitionCore(Arc::clone(&client)); diff --git a/crates/ecstore/src/set_disk/core/io_primitives.rs b/crates/ecstore/src/set_disk/core/io_primitives.rs index 18b04c781..14da6fc97 100644 --- a/crates/ecstore/src/set_disk/core/io_primitives.rs +++ b/crates/ecstore/src/set_disk/core/io_primitives.rs @@ -3558,6 +3558,11 @@ impl RenameRollbackReceipt { } } +struct RenameRollbackOwnership { + receipt: Option, + namespace_commit_guard: Option>, +} + async fn inspect_incomplete_rename_rollback( disks: &[Option], bucket: &str, @@ -3604,8 +3609,12 @@ async fn rollback_failed_rename( dispatch_states: &[RenameDispatchState], rollback_dirs: &[Option], dst: (&str, &str), - receipt: Option, + ownership: RenameRollbackOwnership, ) { + let RenameRollbackOwnership { + receipt, + namespace_commit_guard, + } = ownership; let owned_disks = disks.to_vec(); let owned_errs = errs.to_vec(); let owned_dispatch_states = dispatch_states.to_vec(); @@ -3651,7 +3660,9 @@ async fn rollback_failed_rename( let fi = std::mem::take(&mut file_infos[disk_index]); let bucket = bucket.to_string(); let object = object.to_string(); + let disk_namespace_commit_guard = namespace_commit_guard.clone(); let task = tokio::spawn(async move { + let _namespace_commit_guard = disk_namespace_commit_guard; #[allow(clippy::let_unit_value)] let _task_guard = SetDisks::rename_fanout_task_guard(&object); SetDisks::rename_fanout_barrier(&object, disk_index, rename_fanout_barrier_phase::ROLLBACK).await; @@ -3672,6 +3683,9 @@ async fn rollback_failed_rename( }); tasks.push(async move { (disk_index, task.await) }); } + #[cfg(test)] + rollback_fault_injection::after_undo_dispatch(object); + let _namespace_commit_guard = namespace_commit_guard; for (disk_index, result) in join_all(tasks).await { outcomes[disk_index].outcome = rename_rollback_task_outcome(result); } @@ -3778,6 +3792,7 @@ pub(in crate::set_disk) struct RenameDataFenceOptions<'a> { write_quorum: usize, scanner_publication_lease_tokens: Option<&'a HashMap>, scanner_publication_commit_scope: Option, + namespace_commit_guard: Option>, rollback_receipt: Option, } @@ -3790,6 +3805,7 @@ impl<'a> RenameDataFenceOptions<'a> { write_quorum, scanner_publication_lease_tokens, scanner_publication_commit_scope: None, + namespace_commit_guard: None, rollback_receipt: None, } } @@ -3806,6 +3822,14 @@ impl<'a> RenameDataFenceOptions<'a> { self.scanner_publication_commit_scope = scanner_publication_commit_scope; self } + + pub(in crate::set_disk) fn with_namespace_commit_guard( + mut self, + namespace_commit_guard: Option>, + ) -> Self { + self.namespace_commit_guard = namespace_commit_guard; + self + } } #[allow(dead_code, reason = "asserted by this file's tests (backlog#1823)")] @@ -4164,6 +4188,7 @@ impl SetDisks { write_quorum, scanner_publication_lease_tokens, scanner_publication_commit_scope: _scanner_publication_commit_scope, + namespace_commit_guard, rollback_receipt, } = fence_options; if let Some(file_info) = disks @@ -4210,7 +4235,9 @@ impl SetDisks { let dst_object = fanout_dst_object.clone(); let file_info = file_info.clone(); let successful_rename_completion_rank = successful_rename_completion_rank.clone(); + let namespace_commit_guard = namespace_commit_guard.clone(); tasks.spawn(async move { + let _namespace_commit_guard = namespace_commit_guard; let mut dispatch_state = RenameDispatchState::NotDispatched; let result = std::panic::AssertUnwindSafe(async { #[allow(clippy::let_unit_value)] @@ -4372,7 +4399,10 @@ impl SetDisks { &dispatch_states, &data_dirs, (&fanout_dst_bucket, &fanout_dst_object), - rollback_receipt, + RenameRollbackOwnership { + receipt: rollback_receipt, + namespace_commit_guard, + }, ) .await; if let Some(commit_tx) = commit_tx.take() { @@ -4528,6 +4558,7 @@ impl SetDisks { write_quorum, scanner_publication_lease_tokens, scanner_publication_commit_scope, + namespace_commit_guard, rollback_receipt, } = fence_options; if let Some(file_info) = disks @@ -4561,6 +4592,7 @@ impl SetDisks { let fanout_dst_bucket = dst_bucket.clone(); let fanout_dst_object = dst_object.clone(); let fanout_publication_scope = scanner_publication_commit_scope.clone(); + let fanout_namespace_commit_guard = namespace_commit_guard.clone(); // Keep one coordinator task so a cancelled caller cannot drop partially // completed disk mutations. Per-disk futures stay ordered in `join_all`, // preserving slot-indexed quorum and convergence accounting without a @@ -4569,6 +4601,7 @@ impl SetDisks { // Keep the storage-owned movement permit attached to the actual // fan-out owner, even if the caller future is cancelled. let _fanout_publication_scope = fanout_publication_scope; + let _namespace_commit_guard = fanout_namespace_commit_guard; let successful_rename_completion_rank = rustfs_io_metrics::put_stage_metrics_enabled().then(|| Arc::new(AtomicUsize::new(0))); let futures = fanout_disks @@ -4790,7 +4823,10 @@ impl SetDisks { &dispatch_states, &data_dirs, (&dst_bucket, &dst_object), - rollback_receipt, + RenameRollbackOwnership { + receipt: rollback_receipt, + namespace_commit_guard, + }, ) .await; return Err(ret_err); @@ -6503,9 +6539,9 @@ impl SetDisks { match oi { Ok(oi) => { // Ordinary writes may proceed past a top-level delete marker; - // data movement must not replace an acknowledged deletion. + // data movement and guarded internal writes must preserve it. if oi.delete_marker { - return opts.data_movement.then_some(StorageError::PreconditionFailed); + return (opts.data_movement || opts.preserve_delete_marker).then_some(StorageError::PreconditionFailed); } let if_none_match = http_preconditions.if_none_match_value().map(str::to_owned); let if_match = http_preconditions.if_match_value().map(str::to_owned); @@ -6754,6 +6790,7 @@ pub(in crate::set_disk) mod rollback_fault_injection { VolumeNotFoundAfterRename, PanicAfterRename, CoordinatorPanic, + RollbackCoordinatorPanic, } fn registry() -> &'static Mutex> { @@ -6816,6 +6853,17 @@ pub(in crate::set_disk) mod rollback_fault_injection { panic!("injected rename coordinator panic"); } } + + pub(super) fn after_undo_dispatch(object: &str) { + let fault = registry() + .lock() + .expect("rollback registry should not poison") + .get(object) + .copied(); + if matches!(fault, Some((_, Fault::RollbackCoordinatorPanic))) { + panic!("injected rollback coordinator panic"); + } + } } /// Test-only per-disk call counters for the metadata fan-out (backlog#1325, @@ -6977,7 +7025,7 @@ pub(crate) mod rename_fanout_barrier { use tokio::sync::Notify; pub use super::rename_fanout_barrier_phase::{ - CLEANUP as PHASE_CLEANUP, READ_VERSION as PHASE_READ_VERSION, RENAME as PHASE_RENAME, + CLEANUP as PHASE_CLEANUP, READ_VERSION as PHASE_READ_VERSION, RENAME as PHASE_RENAME, ROLLBACK as PHASE_ROLLBACK, }; /// One armed barrier: the fan-out task matching `(disk_index, phase)` pauses. @@ -10814,79 +10862,177 @@ mod tests { #[tokio::test] #[serial_test::serial(capacity_dirty_scope)] async fn rename_rollback_incomplete_receipt_waits_for_undo_barrier() { - for cancel_caller in [false, true] { - let bucket = "rename-rollback-barrier"; - let object = if cancel_caller { - "rollback-barrier-cancelled" - } else { - "rollback-barrier-object" - }; - let (dirs, disks) = call_counter_local_disks(bucket, 4).await; - prepare_rename_source_dirs(&dirs, &disks, "source").await; - let mut old = metadata_test_fileinfo(object); - old.mod_time = Some(OffsetDateTime::now_utc()); - old.data = Some(Bytes::from_static(b"old-inline-body")); - old.set_inline_data(); - old.metadata.insert("etag".to_string(), "old-etag".to_string()); - for disk in disks.iter().flatten() { - disk.write_metadata(bucket, bucket, object, old.clone()) - .await - .expect("old metadata should be staged"); - } - let _rename_fault = rename_fault_injection::fail_rename_on(object, &[2, 3]); - let _undo_fault = rollback_fault_injection::arm(object, 0, rollback_fault_injection::Fault::Io); - let barrier = rename_fanout_barrier::arm(object, 0, rename_fanout_barrier_phase::ROLLBACK); - let receipt = RenameRollbackReceipt::default(); - let mut rename = Box::pin(SetDisks::rename_data_owned_with_fence( - &disks, - (RUSTFS_META_TMP_BUCKET, "source"), - rename_commit_fileinfos(object, 4, "new-etag"), - (bucket, object), - false, - RenameDataFenceOptions::new(3, None).with_rollback_receipt(receipt.clone()), - )); - tokio::time::timeout(BARRIER_PAUSE_GUARD, async { - tokio::select! { - () = barrier.wait_until_paused() => {} - _ = rename.as_mut() => panic!("rename returned before the armed rollback barrier"), + temp_env::async_with_vars([(ENV_RUSTFS_PUT_RENAME_EARLY_ACK_ENABLE, Some("true"))], async { + for (allow_early_ack, cancel_caller, object) in [ + (false, false, "rollback-barrier-object"), + (false, true, "rollback-barrier-cancelled"), + (true, false, "rollback-barrier-early-object"), + (true, true, "rollback-barrier-early-cancelled"), + ] { + let ctx = Arc::new(crate::runtime::instance::InstanceContext::new()); + let bucket = "rename-rollback-barrier"; + let (dirs, disks) = call_counter_local_disks(bucket, 4).await; + prepare_rename_source_dirs(&dirs, &disks, "source").await; + let mut old = metadata_test_fileinfo(object); + old.mod_time = Some(OffsetDateTime::now_utc()); + old.data = Some(Bytes::from_static(b"old-inline-body")); + old.set_inline_data(); + old.metadata.insert("etag".to_string(), "old-etag".to_string()); + for disk in disks.iter().flatten() { + disk.write_metadata(bucket, bucket, object, old.clone()) + .await + .expect("old metadata should be staged"); } - }) - .await - .expect("undo must reach its disk barrier"); - assert!(receipt.0.get().is_none(), "pending undo must not be recorded as success"); - if cancel_caller { - drop(rename); + let _rename_fault = rename_fault_injection::fail_rename_on(object, &[2, 3]); + let _undo_fault = rollback_fault_injection::arm(object, 0, rollback_fault_injection::Fault::Io); + let barrier = rename_fanout_barrier::arm(object, 0, rename_fanout_barrier_phase::ROLLBACK); + let receipt = RenameRollbackReceipt::default(); + let mut rename = Box::pin(SetDisks::rename_data_owned_with_fence( + &disks, + (RUSTFS_META_TMP_BUCKET, "source"), + rename_commit_fileinfos(object, 4, "new-etag"), + (bucket, object), + allow_early_ack, + RenameDataFenceOptions::new(3, None) + .with_rollback_receipt(receipt.clone()) + .with_namespace_commit_guard(Some(ctx.begin_namespace_commit())), + )); + tokio::time::timeout(BARRIER_PAUSE_GUARD, async { + tokio::select! { + () = barrier.wait_until_paused() => {} + _ = rename.as_mut() => panic!("rename returned before the armed rollback barrier"), + } + }) + .await + .expect("undo must reach its disk barrier"); + assert!(receipt.0.get().is_none(), "pending undo must not be recorded as success"); + assert!(ctx.namespace_commits_pending()); + assert_eq!(ctx.namespace_commit_generation(), 1); + if cancel_caller { + drop(rename); + assert!(ctx.namespace_commits_pending(), "caller cancellation must not retire pending undo work"); + assert_eq!(ctx.namespace_commit_generation(), 1); + barrier.release(); + tokio::time::timeout(BARRIER_PAUSE_GUARD, async { + while receipt.0.get().is_none() || ctx.namespace_commits_pending() { + tokio::task::yield_now().await; + } + }) + .await + .expect("cancelled caller must not cancel rollback accounting"); + } else { + barrier.release(); + assert!(rename.await.is_err()); + } + assert!( + !ctx.namespace_commits_pending(), + "the completed rollback must release its namespace ownership" + ); + assert_eq!(ctx.namespace_commit_generation(), 2); + assert!(receipt.is_incomplete(), "drained undo failure must survive in the receipt"); + for dir in dirs.iter().skip(1) { + let reopened = reopen_local_disk(dir).await; + let restored = reopened + .read_version( + "", + bucket, + object, + "", + &ReadOptions { + read_data: true, + ..Default::default() + }, + ) + .await + .expect("old version must remain readable after caller cancellation"); + assert_eq!(restored.data.as_deref(), Some(b"old-inline-body".as_slice())); + } + } + }) + .await; + } + + #[tokio::test] + #[serial_test::serial(capacity_dirty_scope)] + async fn rename_rollback_children_keep_namespace_ownership_after_coordinator_panic() { + temp_env::async_with_vars([(ENV_RUSTFS_PUT_RENAME_EARLY_ACK_ENABLE, Some("true"))], async { + for (allow_early_ack, object) in [ + (false, "rollback-coordinator-panic"), + (true, "rollback-coordinator-panic-early"), + ] { + let ctx = Arc::new(crate::runtime::instance::InstanceContext::new()); + let bucket = "rename-rollback-coordinator-panic"; + let (dirs, disks) = call_counter_local_disks(bucket, 4).await; + prepare_rename_source_dirs(&dirs, &disks, "source").await; + let mut old = metadata_test_fileinfo(object); + old.mod_time = Some(OffsetDateTime::now_utc()); + old.data = Some(Bytes::from_static(b"old-inline-body")); + old.set_inline_data(); + old.metadata.insert("etag".to_string(), "old-etag".to_string()); + for disk in disks.iter().flatten() { + disk.write_metadata(bucket, bucket, object, old.clone()) + .await + .expect("old metadata should be staged"); + } + let _rename_fault = rename_fault_injection::fail_rename_on(object, &[2, 3]); + let _rollback_fault = + rollback_fault_injection::arm(object, 0, rollback_fault_injection::Fault::RollbackCoordinatorPanic); + let barrier = rename_fanout_barrier::arm(object, 0, rename_fanout_barrier_phase::ROLLBACK); + let receipt = RenameRollbackReceipt::default(); + let result = tokio::time::timeout( + BARRIER_PAUSE_GUARD, + SetDisks::rename_data_owned_with_fence( + &disks, + (RUSTFS_META_TMP_BUCKET, "source"), + rename_commit_fileinfos(object, 4, "new-etag"), + (bucket, object), + allow_early_ack, + RenameDataFenceOptions::new(3, None) + .with_rollback_receipt(receipt.clone()) + .with_namespace_commit_guard(Some(ctx.begin_namespace_commit())), + ), + ) + .await + .expect("coordinator failure must return without waiting for detached undo tasks"); + assert!(result.is_err()); + tokio::time::timeout(BARRIER_PAUSE_GUARD, barrier.wait_until_paused()) + .await + .expect("detached undo must reach its disk barrier"); + assert!( + receipt.is_incomplete(), + "coordinator failure must preserve indeterminate recovery evidence" + ); + assert!(ctx.namespace_commits_pending(), "the paused child must retain namespace ownership"); + assert_eq!(ctx.namespace_commit_generation(), 1); barrier.release(); tokio::time::timeout(BARRIER_PAUSE_GUARD, async { - while receipt.0.get().is_none() { + while ctx.namespace_commits_pending() { tokio::task::yield_now().await; } }) .await - .expect("cancelled caller must not cancel rollback accounting"); - } else { - barrier.release(); - assert!(rename.await.is_err()); + .expect("completed undo children must release their namespace ownership"); + assert_eq!(ctx.namespace_commit_generation(), 2); + for dir in &dirs { + let reopened = reopen_local_disk(dir).await; + let restored = reopened + .read_version( + "", + bucket, + object, + "", + &ReadOptions { + read_data: true, + ..Default::default() + }, + ) + .await + .expect("old version must remain readable after rollback coordinator failure"); + assert_eq!(restored.data.as_deref(), Some(b"old-inline-body".as_slice())); + } } - assert!(receipt.is_incomplete(), "drained undo failure must survive in the receipt"); - for dir in dirs.iter().skip(1) { - let reopened = reopen_local_disk(dir).await; - let restored = reopened - .read_version( - "", - bucket, - object, - "", - &ReadOptions { - read_data: true, - ..Default::default() - }, - ) - .await - .expect("old version must remain readable after caller cancellation"); - assert_eq!(restored.data.as_deref(), Some(b"old-inline-body".as_slice())); - } - } + }) + .await; } #[tokio::test] @@ -11001,9 +11147,35 @@ mod tests { let mut file_infos = rename_commit_fileinfos(object, DISKS, "fresh-rollback-etag"); file_infos[3] = FileInfo::default(); - SetDisks::rename_data(&disks, RUSTFS_META_TMP_BUCKET, "source", &file_infos, bucket, object, 4) + let ctx = Arc::new(crate::runtime::instance::InstanceContext::new()); + ctx.set_scanner_publication_state(false); + let barrier = rename_fanout_barrier::arm(object, 0, rename_fanout_barrier::PHASE_ROLLBACK); + let rename = SetDisks::rename_data_owned_with_fence( + &disks, + (RUSTFS_META_TMP_BUCKET, "source"), + file_infos, + (bucket, object), + false, + RenameDataFenceOptions::new(4, None).with_namespace_commit_guard(Some(ctx.begin_namespace_commit())), + ); + let control = async { + barrier.wait_until_paused().await; + assert!(ctx.namespace_commits_pending(), "rollback must retain namespace publication ownership"); + assert!(ctx.scanner_publication_state_allowed(), "rollback must not disable namespace walks"); + assert_eq!(ctx.namespace_commit_generation(), 1); + barrier.release(); + }; + let (result, ()) = tokio::time::timeout(BARRIER_PAUSE_GUARD, async { tokio::join!(rename, control) }) .await - .expect_err("three successful disks must fail a strict write quorum of four"); + .expect("rename rollback must reach its barrier and finish after release"); + assert_eq!( + result.err(), + Some(DiskError::ErasureWriteQuorum), + "three successful disks must fail a strict write quorum of four" + ); + assert!(!ctx.namespace_commits_pending()); + assert!(ctx.scanner_publication_state_allowed()); + assert_eq!(ctx.namespace_commit_generation(), 2); for (idx, dir) in dirs.iter().enumerate() { let reopened = reopen_local_disk(dir).await; diff --git a/crates/ecstore/src/set_disk/ops/heal.rs b/crates/ecstore/src/set_disk/ops/heal.rs index 3d4624b4e..c7285b9b8 100644 --- a/crates/ecstore/src/set_disk/ops/heal.rs +++ b/crates/ecstore/src/set_disk/ops/heal.rs @@ -2490,9 +2490,9 @@ impl crate::storage_api_contracts::heal::HealOperations for SetDisks { return Ok((result, err.map(|e| e.into()))); } - let disks = self.disks.read().await; - - let disks = disks.clone(); + // The inner heal and missing-object report read the registry again; + // release this snapshot guard before a topology writer can queue between reads. + let disks = self.get_disks_internal().await; let (_, errs) = Self::read_all_fileinfo(&disks, "", bucket, object, version_id, false, false, false) .await .map_err(|e| to_object_err(e.into(), vec![bucket, object]))?; @@ -3419,6 +3419,366 @@ mod heal_result_report_tests { assert_eq!(unformatted, DiskError::UnformattedDisk); } + #[derive(Clone, Copy)] + enum InventoryWriterHealCase { + Existing, + Missing, + MissingVersion, + } + + async fn assert_heal_object_inventory_writer(case: InventoryWriterHealCase) { + use crate::set_disk::core::io_primitives::disk_call_counters; + use std::time::Duration; + use tokio::io::AsyncReadExt; + + let (_temp_dirs, disks, set) = hermetic_set_disks_isolated(4).await; + let bucket = "heal-inventory-writer-bucket"; + let object = match case { + InventoryWriterHealCase::Existing => "heal-inventory-writer-existing", + InventoryWriterHealCase::Missing => "heal-inventory-writer-missing", + InventoryWriterHealCase::MissingVersion => "heal-inventory-writer-missing-version", + }; + set.make_bucket( + bucket, + &MakeBucketOptions { + versioning_enabled: true, + ..Default::default() + }, + ) + .await + .expect("heal fixture bucket should be created"); + let body = vec![0x67; 64 * 1024]; + let stored_version = Uuid::new_v4(); + let stored_version_string = stored_version.to_string(); + let published = if matches!(case, InventoryWriterHealCase::Missing) { + None + } else { + let mut reader = PutObjReader::from_vec(body.clone()); + let info = set + .put_object( + bucket, + object, + &mut reader, + &ObjectOptions { + no_lock: true, + versioned: true, + version_id: Some(stored_version_string.clone()), + ..Default::default() + }, + ) + .await + .expect("full-fanout PUT should seed the heal fixture"); + for disk in &disks { + let metadata = disk + .read_version("", bucket, object, &stored_version_string, &ReadOptions::default()) + .await + .expect("the seeded version must be present on every disk"); + assert_eq!(metadata.version_id, Some(stored_version)); + assert_eq!(metadata.size, i64::try_from(body.len()).expect("fixture size should fit i64")); + } + Some(info) + }; + let requested_version = match case { + InventoryWriterHealCase::Existing => stored_version_string.clone(), + InventoryWriterHealCase::Missing => String::new(), + InventoryWriterHealCase::MissingVersion => Uuid::new_v4().to_string(), + }; + let opts = HealOpts { + no_lock: true, + ..Default::default() + }; + let calls = disk_call_counters::observe(object); + let read_gate = set.disks.read().await; + // UFCS selects the trait's outer precheck, not the same-named inherent heal. + let heal = ::heal_object( + set.as_ref(), + bucket, + object, + &requested_version, + &opts, + ); + tokio::pin!(heal); + assert!(matches!( + futures::poll!(tokio::task::unconstrained(heal.as_mut())), + std::task::Poll::Pending + )); + // These tests use the current-thread runtime: full-wait metadata tasks + // have been spawned, but cannot run during the single unconstrained poll. + assert_eq!(calls.total(disk_call_counters::KIND_READ_VERSION), 0); + let writer = set.disks.write(); + tokio::pin!(writer); + assert!(matches!( + futures::poll!(tokio::task::unconstrained(writer.as_mut())), + std::task::Poll::Pending + )); + assert!(set.disks.try_read().is_err(), "the writer must already block new inventory readers"); + tokio::time::timeout(Duration::from_secs(5), async { + while calls.total(disk_call_counters::KIND_READ_VERSION) < 4 { + tokio::task::yield_now().await; + } + }) + .await + .expect("the suspended trait heal must have started the real metadata fanout"); + for disk_index in 0..4 { + assert_eq!(calls.for_disk(disk_call_counters::KIND_READ_VERSION, disk_index), 1); + } + drop(read_gate); + + let (_, outcome) = + tokio::time::timeout(Duration::from_secs(5), async { tokio::join!(async { drop(writer.await) }, heal) }) + .await + .expect("trait heal must not deadlock its nested inventory read with the queued writer"); + let (result, error) = outcome.expect("heal should report the object's outcome"); + match case { + InventoryWriterHealCase::Existing => assert!(error.is_none(), "existing object heal failed: {error:?}"), + InventoryWriterHealCase::Missing => assert!(matches!(error, Some(Error::FileNotFound))), + InventoryWriterHealCase::MissingVersion => assert!(matches!(error, Some(Error::FileVersionNotFound))), + } + assert_eq!(result.bucket, bucket); + assert_eq!(result.object, object); + assert_eq!(result.version_id, requested_version); + assert_eq!(result.disk_count, 4); + assert_eq!(result.before.drives.len(), 4); + assert_eq!(result.after.drives.len(), 4); + for disk_index in 0..4 { + let endpoint = set.set_endpoints[disk_index].to_string(); + assert_eq!(result.before.drives[disk_index].endpoint, endpoint); + assert_eq!(result.after.drives[disk_index].endpoint, endpoint); + } + if let Some(published) = published { + tokio::time::timeout(Duration::from_secs(10), async { + let mut reader = set + .get_object_reader( + bucket, + object, + None, + Default::default(), + &ObjectOptions { + versioned: true, + version_id: Some(stored_version_string), + ..Default::default() + }, + ) + .await + .expect("the stored version must remain readable after heal"); + assert_eq!(reader.object_info.etag, published.etag); + assert_eq!(reader.object_info.version_id, Some(stored_version)); + let mut observed_body = Vec::new(); + reader + .stream + .read_to_end(&mut observed_body) + .await + .expect("stored body should stream"); + assert_eq!(observed_body, body); + }) + .await + .expect("GET must finish after the inventory writer and heal"); + } + } + + #[tokio::test] + async fn heal_object_inventory_writer_existing() { + assert_heal_object_inventory_writer(InventoryWriterHealCase::Existing).await; + } + + #[tokio::test] + async fn heal_object_inventory_writer_missing() { + assert_heal_object_inventory_writer(InventoryWriterHealCase::Missing).await; + } + + #[tokio::test] + async fn heal_object_inventory_writer_missing_version() { + assert_heal_object_inventory_writer(InventoryWriterHealCase::MissingVersion).await; + } + + #[tokio::test] + #[serial_test::serial] + async fn heal_object_with_queued_disk_renewal() { + use crate::layout::endpoints::SetupType; + use crate::runtime::instance::InstanceContext; + use crate::set_disk::core::io_primitives::disk_call_counters; + use std::collections::HashMap; + use std::future::Future; + use std::task::Poll; + use std::time::Duration; + use tokio::io::AsyncReadExt; + + // renew_disk still registers local disks on the ambient context. Match + // the default serial group used by its other setup/registry fixtures, + // and restore only this temporary endpoint, including on a failed join. + struct RenewDiskTestState { + ctx: Arc, + was_dist_erasure: bool, + map: Arc>>>, + endpoint: String, + previous_disk: Option>, + } + + impl Drop for RenewDiskTestState { + fn drop(&mut self) { + let ctx = self.ctx.clone(); + let was_dist_erasure = self.was_dist_erasure; + let map = self.map.clone(); + let endpoint = self.endpoint.clone(); + let previous_disk = self.previous_disk.take(); + let handle = tokio::runtime::Handle::current(); + std::thread::spawn(move || { + handle.block_on(async move { + let mut map = map.write().await; + match previous_disk { + Some(disk) => { + map.insert(endpoint, disk); + } + None => { + map.remove(&endpoint); + } + } + drop(map); + if was_dist_erasure { + ctx.update_erasure_type(SetupType::DistErasure).await; + } + }); + }) + .join() + .expect("renew fixture state restoration should finish"); + } + } + + let (_temp_dirs, disks, set) = hermetic_set_disks_isolated(4).await; + let endpoint = set.set_endpoints[0].clone(); + let ctx = crate::runtime::global::current_ctx(); + let map = ctx.local_disk_map(); + let restore = RenewDiskTestState { + ctx: ctx.clone(), + was_dist_erasure: ctx.is_dist_erasure().await, + map: map.clone(), + endpoint: endpoint.to_string(), + previous_disk: map.read().await.get(&endpoint.to_string()).cloned(), + }; + // Only distributed erasure needs an override to avoid the ambient slot array. + if restore.was_dist_erasure { + ctx.update_erasure_type(SetupType::Erasure).await; + } + + let bucket = "heal-disk-renewal-bucket"; + let object = "heal-disk-renewal-object"; + set.make_bucket(bucket, &MakeBucketOptions::default()) + .await + .expect("renew fixture bucket should be created"); + let body = vec![0x73; 64 * 1024]; + let mut reader = PutObjReader::from_vec(body.clone()); + let published = set + .put_object( + bucket, + object, + &mut reader, + &ObjectOptions { + no_lock: true, + ..Default::default() + }, + ) + .await + .expect("full-fanout PUT should seed the renewal fixture"); + for disk in &disks { + let metadata = disk + .read_version("", bucket, object, "", &ReadOptions::default()) + .await + .expect("the seeded object must be present on every disk"); + assert_eq!(metadata.size, i64::try_from(body.len()).expect("fixture size should fit i64")); + } + + let opts = HealOpts { + no_lock: true, + ..Default::default() + }; + let calls = disk_call_counters::observe(object); + let read_gate = set.disks.read().await; + let heal = ::heal_object( + set.as_ref(), + bucket, + object, + "", + &opts, + ); + tokio::pin!(heal); + assert!(matches!(futures::poll!(tokio::task::unconstrained(heal.as_mut())), Poll::Pending)); + assert_eq!(calls.total(disk_call_counters::KIND_READ_VERSION), 0); + + let renew = set.renew_disk(&endpoint); + tokio::pin!(renew); + tokio::time::timeout( + Duration::from_secs(5), + futures::future::poll_fn(|cx| { + assert!( + std::pin::pin!(tokio::task::unconstrained(renew.as_mut())) + .poll(cx) + .is_pending(), + "renewal must reach its inventory write before returning" + ); + if set.disks.try_read().is_err() { + Poll::Ready(()) + } else { + Poll::Pending + } + }), + ) + .await + .expect("real renewal must queue its topology writer behind the read gate"); + let registered = map + .read() + .await + .get(&endpoint.to_string()) + .cloned() + .flatten() + .expect("renewal must register the connected disk before its inventory write"); + assert!(!Arc::ptr_eq(®istered, &disks[0]), "renewal must construct a new disk handle"); + tokio::time::timeout(Duration::from_secs(5), async { + while calls.total(disk_call_counters::KIND_READ_VERSION) < 4 { + tokio::task::yield_now().await; + } + }) + .await + .expect("the suspended trait heal must have started the real metadata fanout"); + for disk_index in 0..4 { + assert_eq!(calls.for_disk(disk_call_counters::KIND_READ_VERSION, disk_index), 1); + } + drop(read_gate); + + let (_, outcome) = tokio::time::timeout(Duration::from_secs(5), async { tokio::join!(renew, heal) }) + .await + .expect("trait heal and real disk renewal must finish without a nested inventory read deadlock"); + let (report, error) = outcome.expect("heal should report the existing object"); + assert!(error.is_none(), "existing object heal failed after renewal: {error:?}"); + assert_eq!(report.bucket, bucket); + assert_eq!(report.object, object); + assert_eq!(report.disk_count, 4); + let renewed = set.get_disks_internal().await[0] + .clone() + .expect("the renewed slot must remain online"); + assert!(Arc::ptr_eq(&renewed, ®istered), "the set must publish the newly connected handle"); + assert_eq!(renewed.endpoint(), endpoint); + let format = load_format_erasure(&renewed, false) + .await + .expect("renewed disk format should remain readable"); + assert_eq!(format.erasure.this, set.format.erasure.sets[0][0]); + tokio::time::timeout(Duration::from_secs(10), async { + let mut reader = set + .get_object_reader(bucket, object, None, Default::default(), &ObjectOptions::default()) + .await + .expect("the object must remain readable after renewal and heal"); + assert_eq!(reader.object_info.etag, published.etag); + let mut observed_body = Vec::new(); + reader + .stream + .read_to_end(&mut observed_body) + .await + .expect("stored body should stream"); + assert_eq!(observed_body, body); + }) + .await + .expect("GET must finish after renewal and heal"); + } + // Regression for #955: an offline disk must contribute exactly one drive // record. Before the fix the offline branch fell through and pushed a second // (Corrupt) record for the same disk, so `before/after.drives` grew to diff --git a/crates/ecstore/src/set_disk/ops/multipart.rs b/crates/ecstore/src/set_disk/ops/multipart.rs index aaf6204b5..4aefdb04c 100644 --- a/crates/ecstore/src/set_disk/ops/multipart.rs +++ b/crates/ecstore/src/set_disk/ops/multipart.rs @@ -4050,6 +4050,7 @@ mod tests { let _ = drain_global_dirty_scopes(); let rename_barrier = rename_fanout_barrier::arm(object, 0, rename_fanout_barrier::PHASE_RENAME); + let rename_tasks = rename_fanout_barrier::observe_tasks(object); let complete_store = Arc::clone(&set_disks); let mut complete = tokio::spawn(async move { let mut opts = ObjectOptions::default(); @@ -4061,16 +4062,6 @@ mod tests { tokio::time::timeout(Duration::from_secs(30), rename_barrier.wait_until_paused()) .await .expect("multipart completion should pause one tail disk during rename"); - assert!( - tokio::time::timeout(Duration::from_millis(100), &mut complete).await.is_err(), - "multipart completion must not publish success while a tail rename is still paused" - ); - - let initial = drain_global_dirty_scopes().into_iter().collect::>(); - assert!( - initial.is_empty(), - "capacity must not be marked as committed before the full multipart rename finishes" - ); let abort_store = Arc::clone(&set_disks); let abort = tokio::spawn(async move { @@ -4079,21 +4070,46 @@ mod tests { .await }); signaling.wait_for_attempts(2).await; - assert!(!abort.is_finished(), "the in-flight completion must retain the multipart upload guard"); - let retained_staging = futures::future::join_all( - disk_stores - .iter() - .map(|disk| disk.read_all(RUSTFS_META_MULTIPART_BUCKET, &staged_part)), - ) + // A paused rename does not establish that the other disks reached quorum. + let retained_staging = tokio::time::timeout(Duration::from_secs(30), async { + loop { + let mut retained = 0; + for result in futures::future::join_all( + disk_stores + .iter() + .map(|disk| disk.read_all(RUSTFS_META_MULTIPART_BUCKET, &staged_part)), + ) + .await + { + match result { + Ok(_) => retained += 1, + Err(DiskError::FileNotFound) => {} + Err(error) => panic!("staged rename source lookup failed: {error}"), + } + } + if retained <= 1 && rename_tasks.running() == 1 { + break retained; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) .await - .into_iter() - .filter(|result| result.is_ok()) - .count(); + .expect("unpaused multipart renames should finish before the tail is released"); assert_eq!( retained_staging, 1, "only the paused tail disk should still retain the multipart rename source" ); + assert!( + tokio::time::timeout(Duration::from_millis(100), &mut complete).await.is_err(), + "multipart completion must not publish success while a tail rename is still paused" + ); + let initial = drain_global_dirty_scopes().into_iter().collect::>(); + assert!( + initial.is_empty(), + "capacity must not be marked as committed before the full multipart rename finishes" + ); + assert!(!abort.is_finished(), "the in-flight completion must retain the multipart upload guard"); signaling.set_target(rustfs_lock::ObjectKey::new(bucket, object)); let object_attempt = signaling.attempts.load(Ordering::Acquire) + 1; diff --git a/crates/ecstore/src/set_disk/ops/object.rs b/crates/ecstore/src/set_disk/ops/object.rs index c76daf707..3e09515d2 100644 --- a/crates/ecstore/src/set_disk/ops/object.rs +++ b/crates/ecstore/src/set_disk/ops/object.rs @@ -4459,7 +4459,10 @@ impl SetDisks { commit_scanner_publication_lease_tokens.as_ref(), ) .with_publication_scope(commit_scanner_publication_scope.clone()) - .with_rollback_receipt(commit_rollback_receipt.clone()), + .with_rollback_receipt(commit_rollback_receipt.clone()) + .with_namespace_commit_guard( + (!is_meta_bucketname(&commit_bucket)).then(|| commit_set.ctx.begin_namespace_commit()), + ), ) .await; if let Some(scope) = commit_scanner_publication_scope.as_ref() { diff --git a/crates/ecstore/src/store/bucket.rs b/crates/ecstore/src/store/bucket.rs index 036585d0e..210118cda 100644 --- a/crates/ecstore/src/store/bucket.rs +++ b/crates/ecstore/src/store/bucket.rs @@ -1059,6 +1059,7 @@ mod tests { use crate::storage_api_contracts::{ bucket::{BucketOperations as _, BucketOptions, DeleteBucketOptions, MakeBucketOptions, SRBucketDeleteOp}, list::ListOperations as _, + namespace::NamespaceLocking as _, object::{ObjectIO as _, ObjectOperations as _}, }; use crate::store::{ECStore, init_local_disks_with_instance_ctx}; @@ -1486,10 +1487,19 @@ mod tests { .put_object(bucket, object, &mut reader, &ObjectOptions::default()) .await .expect("object should be written"); + let lock = ecstore.pools[0].disk_set[0] + .new_ns_lock(bucket, object) + .await + .expect("fixture namespace lock should be created"); + drop( + lock.get_write_lock(Duration::from_secs(30)) + .await + .expect("fixture rename tail should finish before checking its generation"), + ); assert_eq!( ecstore.scanner_namespace_mutation_generation(), - generation_before_put.saturating_add(1), - "successful object creation should advance scanner namespace activity" + generation_before_put.saturating_add(3), + "successful object creation must observe the logical mutation and both fanout boundaries" ); ecstore .get_object_info(bucket, object, &ObjectOptions::default()) diff --git a/crates/ecstore/src/store/init.rs b/crates/ecstore/src/store/init.rs index d0b426b68..63d2b5fdd 100644 --- a/crates/ecstore/src/store/init.rs +++ b/crates/ecstore/src/store/init.rs @@ -787,6 +787,12 @@ impl ECStore { pub fn single_pool(&self) -> bool { self.pools.len() == 1 } + + /// The set-local create-only check is atomic only when every object + /// mutation uses that same, enabled namespace lock domain. + pub fn supports_atomic_create_only_write_back(&self) -> bool { + !self.ctx.lock_manager().is_disabled() && self.pools.len() == 1 && self.pools[0].disk_set.len() == 1 + } } #[cfg(test)] @@ -2127,7 +2133,7 @@ mod tests { .iter() .map(|&drives_per_set| (1, drives_per_set)) .collect::>(); - build_isolated_test_store_with_layout(temp_dir, cmd_line, &pool_layouts, shutdown).await + build_isolated_test_store_with_layout(temp_dir, cmd_line, &pool_layouts, shutdown, None).await } async fn build_isolated_test_store_with_layout( @@ -2135,6 +2141,7 @@ mod tests { cmd_line: &str, pool_layouts: &[(usize, usize)], shutdown: CancellationToken, + instance_ctx: Option>, ) -> ( Arc, Arc, @@ -2167,7 +2174,7 @@ mod tests { let endpoint_pools = EndpointServerPools(pools); crate::services::notification_sys::install_cross_pool_fence_fleet_proof_for_test(); - let instance_ctx = Arc::new(crate::runtime::instance::InstanceContext::new()); + let instance_ctx = instance_ctx.unwrap_or_else(|| Arc::new(crate::runtime::instance::InstanceContext::new())); crate::store::init_local_disks_with_instance_ctx(&instance_ctx, endpoint_pools.clone()) .await .expect("register local disks into the fresh context"); @@ -2535,6 +2542,348 @@ mod tests { shutdown.cancel(); } + #[cfg(feature = "test-util")] + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + #[serial_test::serial(storage_class_env)] + async fn early_ack_put_tails_block_scanner_publication_until_all_renames_finish() { + use crate::storage_api_contracts::namespace::NamespaceLocking as _; + + let temp_dir = tempfile::tempdir().expect("create scanner PUT tail store dir"); + let (ctx, store, shutdown) = + without_storage_class_env(build_isolated_test_store(temp_dir.path(), "scanner-put-tails", &[4])).await; + crate::bucket::metadata_sys::init_bucket_metadata_sys(Arc::clone(&store), Vec::new()).await; + let bucket = format!("scanner-put-tails-{}", Uuid::new_v4()); + store + .make_bucket(&bucket, &MakeBucketOptions::default()) + .await + .expect("create scanner PUT tail bucket"); + let set = &store.pools[0].disk_set[0]; + let objects = [("scanner-tail-a", vec![0xA1; 273]), ("scanner-tail-b", vec![0xB2; 379])]; + + temp_env::async_with_vars([(crate::set_disk::ENV_RUSTFS_PUT_RENAME_EARLY_ACK_ENABLE, Some("true"))], async { + let (active, blocked, movement_generation) = store.scanner_data_movement_activity().await; + assert!(!active && !blocked); + assert!(ctx.scanner_publication_state_allowed(), "the set admission cache should start allowed"); + let (old_lease, _) = store + .acquire_scanner_publication_lease(movement_generation, crate::runtime::instance::SCANNER_PUBLICATION_LEASE_TTL) + .await + .expect("publication lease should be admitted before either PUT starts"); + + let barriers: Vec<_> = objects + .iter() + .map(|(object, _)| { + crate::set_disk::rename_fanout_barrier::arm(object, 0, crate::set_disk::rename_fanout_barrier::PHASE_RENAME) + }) + .collect(); + let trackers: Vec<_> = objects + .iter() + .map(|(object, _)| crate::set_disk::rename_fanout_barrier::observe_tasks(object)) + .collect(); + let puts: Vec<_> = objects + .iter() + .map(|(object, body)| { + let put_store = Arc::clone(&store); + let put_bucket = bucket.clone(); + let object = *object; + let body = body.clone(); + tokio::spawn(async move { + let mut reader = PutObjReader::from_vec(body); + put_store + .put_object(&put_bucket, object, &mut reader, &ObjectOptions::default()) + .await + }) + }) + .collect(); + let committed = tokio::time::timeout(Duration::from_secs(30), async { + for barrier in &barriers { + barrier.wait_until_paused().await; + } + let mut committed = Vec::with_capacity(puts.len()); + for put in puts { + committed.push( + put.await + .expect("early-ACK PUT task should join while its tail is paused") + .expect("root PUT should return after quorum without waiting for its tail"), + ); + } + committed + }) + .await + .expect("both root PUTs must quorum-ACK while their tail disks remain paused"); + + assert!(trackers.iter().all(|tracker| tracker.running() >= 1)); + assert!(ctx.namespace_commits_pending()); + assert!( + ctx.scanner_publication_state_allowed(), + "pending PUT tails must not disable scanner namespace walks" + ); + let (active, blocked, observed_movement_generation) = store.scanner_data_movement_activity().await; + assert!(!active, "ordinary PUT tails are not decommission or rebalance work"); + assert!(!blocked, "ordinary PUT tails must not block the movement-only scan baseline"); + assert_eq!(observed_movement_generation, movement_generation); + assert!(store.scanner_data_usage_publication_blocked().await); + assert!(store.scanner_data_usage_publication_admission_guard().await.is_some()); + assert!(set.scanner_data_usage_publication_admission_guard().await.is_some()); + for error in [ + store + .acquire_scanner_publication_lease( + movement_generation, + crate::runtime::instance::SCANNER_PUBLICATION_LEASE_TTL, + ) + .await + .expect_err("a new remote publication lease must reject pending PUT tails"), + store + .validate_scanner_publication_lease(old_lease, movement_generation) + .await + .expect_err("an existing remote lease must not bypass pending PUT tails"), + store + .acquire_scanner_publication_lease_guard(old_lease) + .await + .expect_err("target-side publication admission must reject pending PUT tails"), + ] { + assert!( + error.to_string().contains("blocked"), + "publication must fail because of active tails: {error}" + ); + } + store.release_scanner_publication_lease(old_lease).await; + + for (index, barrier) in barriers.iter().enumerate() { + let commit_generation = ctx.namespace_commit_generation(); + let namespace_generation = store.scanner_namespace_mutation_generation(); + barrier.release(); + tokio::time::timeout(Duration::from_secs(30), async { + while trackers[index].running() != 0 || ctx.namespace_commit_generation() <= commit_generation { + tokio::task::yield_now().await; + } + if index + 1 == barriers.len() { + while ctx.namespace_commits_pending() { + tokio::task::yield_now().await; + } + } + }) + .await + .expect("released tail must drain and publish its terminal namespace generation"); + assert!(store.scanner_namespace_mutation_generation() > namespace_generation); + let pending = index + 1 < barriers.len(); + assert_eq!(ctx.namespace_commits_pending(), pending); + assert_eq!(store.scanner_data_usage_publication_blocked().await, pending); + assert!(!store.scanner_data_movement_activity().await.1); + assert!(store.scanner_data_usage_publication_admission_guard().await.is_some()); + assert!(set.scanner_data_usage_publication_admission_guard().await.is_some()); + } + + let (lease, generation) = store + .acquire_scanner_publication_lease(movement_generation, crate::runtime::instance::SCANNER_PUBLICATION_LEASE_TTL) + .await + .expect("remote publication lease should resume after both tails drain"); + store + .validate_scanner_publication_lease(lease, generation) + .await + .expect("a resumed remote publication lease should validate"); + drop( + store + .acquire_scanner_publication_lease_guard(lease) + .await + .expect("target-side publication admission should resume after both tails drain"), + ); + assert!(store.release_scanner_publication_lease(lease).await); + + let disks = set.disk_inventory().await; + assert_eq!(disks.len(), 4); + for ((object, body), committed) in objects.iter().zip(&committed) { + let logical_size = i64::try_from(body.len()).expect("fixture payload size should fit i64"); + let etag = committed.etag.as_ref().expect("root PUT should return a committed ETag"); + for (disk_index, disk) in disks.iter().enumerate() { + let file_info = disk + .as_ref() + .expect("every fixture disk should remain online") + .read_version( + "", + &bucket, + object, + "", + &crate::disk::ReadOptions { + read_data: true, + ..Default::default() + }, + ) + .await + .unwrap_or_else(|err| panic!("disk {disk_index} should publish {object} after its tail finishes: {err}")); + assert_eq!(file_info.size, logical_size); + assert_eq!(file_info.metadata.get(http::header::ETAG.as_str()), Some(etag)); + assert!( + file_info.inline_data(), + "small fixture payloads should have an inline shard on every disk" + ); + let inline_data = file_info.data.as_ref().expect("every disk should retain its inline shard"); + let erasure = crate::erasure::coding::Erasure::try_new_with_options( + file_info.erasure.data_blocks, + file_info.erasure.parity_blocks, + file_info.erasure.block_size, + file_info.uses_legacy_checksum, + ) + .expect("persisted erasure geometry should be valid"); + let shard_size = + usize::try_from(erasure.shard_file_size(logical_size)).expect("fixture shard size should fit usize"); + crate::erasure::coding::bitrot_verify( + Cursor::new(inline_data.clone()), + inline_data.len(), + shard_size, + rustfs_utils::HashAlgorithm::HighwayHash256S, + erasure.shard_size(), + ) + .await + .unwrap_or_else(|err| panic!("disk {disk_index} should retain a complete valid shard for {object}: {err}")); + } + let mut reader = store + .get_object_reader(&bucket, object, None, HeaderMap::new(), &ObjectOptions::default()) + .await + .expect("fully drained PUT should be readable"); + let mut actual = Vec::new(); + reader.stream.read_to_end(&mut actual).await.expect("PUT body should drain"); + assert_eq!(&actual, body); + } + + let generation_before_internal_put = ctx.namespace_commit_generation(); + let internal_object = "scanner-tail-regression/internal-metadata"; + let internal_body = b"scanner metadata must not invalidate its own publication"; + let mut internal_reader = PutObjReader::from_vec(internal_body.to_vec()); + store + .put_object(RUSTFS_META_BUCKET, internal_object, &mut internal_reader, &ObjectOptions::default()) + .await + .expect("internal metadata PUT should commit without scanner self-invalidation"); + let internal_lock = set + .new_ns_lock(RUSTFS_META_BUCKET, internal_object) + .await + .expect("internal metadata tail lock should be available"); + drop( + internal_lock + .get_write_lock(Duration::from_secs(30)) + .await + .expect("internal metadata tail should drain"), + ); + assert_eq!(ctx.namespace_commit_generation(), generation_before_internal_put); + assert!(!ctx.namespace_commits_pending()); + assert!(store.scanner_data_usage_publication_admission_guard().await.is_some()); + assert!(set.scanner_data_usage_publication_admission_guard().await.is_some()); + let mut internal_reader = store + .get_object_reader(RUSTFS_META_BUCKET, internal_object, None, HeaderMap::new(), &ObjectOptions::default()) + .await + .expect("internal metadata should remain readable"); + let mut actual = Vec::new(); + internal_reader + .stream + .read_to_end(&mut actual) + .await + .expect("internal metadata body should drain"); + assert_eq!(actual, internal_body); + }) + .await; + shutdown.cancel(); + } + + #[cfg(feature = "test-util")] + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + #[serial_test::serial(storage_class_env)] + async fn cancelled_early_ack_put_keeps_scanner_publication_blocked_until_tail_finishes() { + let temp_dir = tempfile::tempdir().expect("create cancelled scanner PUT tail store dir"); + let (ctx, store, shutdown) = + without_storage_class_env(build_isolated_test_store(temp_dir.path(), "scanner-cancelled-put-tail", &[4])).await; + crate::bucket::metadata_sys::init_bucket_metadata_sys(Arc::clone(&store), Vec::new()).await; + let bucket = format!("scanner-cancelled-put-tail-{}", Uuid::new_v4()); + let object = "scanner-cancelled-tail"; + let body = vec![0xC3; 273]; + store + .make_bucket(&bucket, &MakeBucketOptions::default()) + .await + .expect("create cancelled scanner PUT tail bucket"); + + temp_env::async_with_vars([(crate::set_disk::ENV_RUSTFS_PUT_RENAME_EARLY_ACK_ENABLE, Some("true"))], async { + let tracker = crate::set_disk::rename_fanout_barrier::observe_tasks(object); + let tail = + crate::set_disk::rename_fanout_barrier::arm(object, 0, crate::set_disk::rename_fanout_barrier::PHASE_RENAME); + let quorum = crate::set_disk::PutObjectCommitBarrier::install( + &bucket, + object, + crate::set_disk::PutObjectCommitPause::AfterRenameQuorum, + ); + let handoff = crate::set_disk::PutObjectCommitBarrier::install( + &bucket, + object, + crate::set_disk::PutObjectCommitPause::AfterRenameHandoff, + ); + let put_store = Arc::clone(&store); + let put_bucket = bucket.clone(); + let put_body = body.clone(); + let put = tokio::spawn(async move { + let mut reader = PutObjReader::from_vec(put_body); + put_store + .put_object(&put_bucket, object, &mut reader, &ObjectOptions::default()) + .await + }); + tokio::time::timeout(Duration::from_secs(30), tail.wait_until_paused()) + .await + .expect("cancelled PUT should pause one disk before rename"); + quorum.wait_until_paused().await; + put.abort(); + assert!( + put.await + .expect_err("caller should be cancelled after rename quorum") + .is_cancelled() + ); + quorum.release(); + handoff.wait_until_paused().await; + assert!(tracker.running() >= 1); + assert!(ctx.namespace_commits_pending()); + assert!(!store.scanner_data_movement_activity().await.1); + assert!(store.scanner_data_usage_publication_blocked().await); + assert!(store.scanner_data_usage_publication_admission_guard().await.is_some()); + assert!( + store.pools[0].disk_set[0] + .scanner_data_usage_publication_admission_guard() + .await + .is_some() + ); + let generation = store.scanner_namespace_mutation_generation(); + + handoff.release(); + tail.release(); + tokio::time::timeout(Duration::from_secs(30), async { + while tracker.running() != 0 || ctx.namespace_commits_pending() { + tokio::task::yield_now().await; + } + }) + .await + .expect("cancelled request's detached fanout must release scanner admission after finishing"); + assert!(store.scanner_namespace_mutation_generation() > generation); + assert!(!store.scanner_data_usage_publication_blocked().await); + assert!(store.scanner_data_usage_publication_admission_guard().await.is_some()); + for (disk_index, disk) in store.pools[0].disk_set[0].disk_inventory().await.iter().enumerate() { + let file_info = disk + .as_ref() + .expect("cancelled PUT fixture disk should remain online") + .read_version("", &bucket, object, "", &crate::disk::ReadOptions::default()) + .await + .unwrap_or_else(|err| panic!("cancelled PUT must still publish on disk {disk_index}: {err}")); + assert_eq!(file_info.size, i64::try_from(body.len()).expect("fixture body size should fit i64")); + } + let mut reader = store + .get_object_reader(&bucket, object, None, HeaderMap::new(), &ObjectOptions::default()) + .await + .expect("a cancelled caller must not discard its quorum-committed object"); + let mut actual = Vec::new(); + reader + .stream + .read_to_end(&mut actual) + .await + .expect("cancelled PUT body should drain"); + assert_eq!(actual, body); + }) + .await; + shutdown.cancel(); + } + #[cfg(feature = "test-util")] #[test] #[serial_test::serial(storage_class_env)] @@ -2986,8 +3335,9 @@ mod tests { ) -> crate::core::pools::DecommissionTestFaultDecision { let target_bucket = bucket.to_string(); let target_object = object.to_string(); - Arc::new(move |stage, bucket, object, _attempt, succeeded| { + Arc::new(move |stage, bucket, object, attempt, succeeded| { if !succeeded + || attempt >= crate::core::pools::DECOMMISSION_VERSION_COPY_ATTEMPTS || stage != DECOMMISSION_TEST_FAULT_STAGE_MIGRATE_OBJECT || bucket != target_bucket || object != target_object @@ -2997,6 +3347,7 @@ mod tests { // Entry retries reset the local attempt; real copy errors can skip // successful attempts. Only injected faults spend this global budget. + // A real failure may consume an attempt, so preserve the final chance. faults .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |faults| { (faults < crate::core::pools::DECOMMISSION_VERSION_COPY_ATTEMPTS.saturating_sub(1)) @@ -5018,6 +5369,7 @@ mod tests { "decommission-delete-fence", &[(2, 4), (1, 4)], CancellationToken::new(), + None, )) .await; crate::bucket::metadata_sys::init_bucket_metadata_sys(store.clone(), Vec::new()).await; @@ -5149,7 +5501,15 @@ mod tests { #[test] fn decommission_retry_fault_budget_counts_successes_across_attempt_changes() { - for attempts in [[1, 2, 3], [1, 1, 2], [1, 3, 3]] { + let cases: &[&[(usize, bool, bool)]] = &[ + &[(1, true, true), (2, true, true), (3, true, false)], + &[(1, true, true), (1, true, true), (2, true, false)], + &[(1, true, true), (3, true, false), (3, true, false)], + &[(1, true, true), (2, false, false), (1, true, true), (2, true, false)], + &[(1, true, true), (2, false, false), (3, true, false)], + &[(3, true, false), (4, true, false)], + ]; + for case in cases { let faults = Arc::new(AtomicUsize::new(0)); let hook = decommission_retry_fault_hook("bucket", "object", Arc::clone(&faults)); @@ -5163,14 +5523,16 @@ mod tests { } assert_eq!(faults.load(Ordering::SeqCst), 0, "unrelated or failed copies must not consume faults"); - for (index, attempt) in attempts.into_iter().enumerate() { + let mut expected_faults = 0; + for &(attempt, succeeded, expected) in *case { assert_eq!( - hook(DECOMMISSION_TEST_FAULT_STAGE_MIGRATE_OBJECT, "bucket", "object", attempt, true), - index < 2, - "attempts={attempts:?}, index={index}" + hook(DECOMMISSION_TEST_FAULT_STAGE_MIGRATE_OBJECT, "bucket", "object", attempt, succeeded), + expected, + "fault plan {case:?} at attempt {attempt}" ); + expected_faults += usize::from(expected); + assert_eq!(faults.load(Ordering::SeqCst), expected_faults); } - assert_eq!(faults.load(Ordering::SeqCst), 2, "attempts={attempts:?}"); } } @@ -5306,6 +5668,15 @@ mod tests { changed_result.expect("SourceChanged entry retry must converge"); other_result.expect("other bucket entry must continue through ordinary copy retries"); + assert_eq!( + store.pool_meta.read().await.pools[0] + .decommission + .as_ref() + .expect("decommission progress should remain available") + .items_decommission_failed, + 0, + "entry completion must not hide an exhausted copy failure" + ); assert!(!rx.is_cancelled(), "entry-level SourceChanged must not cancel the shared worker token"); assert_eq!(mutation_calls.load(Ordering::SeqCst), 2, "entry must be re-listed after SourceChanged"); assert_eq!(ordinary_faults.load(Ordering::SeqCst), 2, "ordinary copy must consume the retry budget"); @@ -5934,6 +6305,7 @@ mod tests { "reverse-decommission-fixed-target", &[(1, 4), (1, 4)], CancellationToken::new(), + None, )) .await; crate::bucket::metadata_sys::init_bucket_metadata_sys(store.clone(), Vec::new()).await; @@ -6355,6 +6727,7 @@ mod tests { "multi-set-decommission-source-cleanup", &[(2, 4)], CancellationToken::new(), + None, )) .await; crate::bucket::metadata_sys::init_bucket_metadata_sys(store.clone(), Vec::new()).await; @@ -8870,18 +9243,17 @@ mod tests { const MANIFEST_COUNT: usize = 10; let temp_dir = tempfile::tempdir().expect("create fast manifest pass recovery store dir"); - let (ctx, store, _shutdown) = - without_storage_class_env(build_isolated_test_store(temp_dir.path(), "tier-delete-fast-manifest-pass", &[4])).await; + let mut instance_ctx = crate::runtime::instance::InstanceContext::new(); + instance_ctx.suppress_tier_delete_journal_recovery_for_test(); + let (ctx, store, shutdown) = without_storage_class_env(build_isolated_test_store_with_layout( + temp_dir.path(), + "tier-delete-fast-manifest-pass", + &[(1, 4)], + CancellationToken::new(), + Some(Arc::new(instance_ctx)), + )) + .await; crate::bucket::metadata_sys::init_bucket_metadata_sys(store.clone(), Vec::new()).await; - let bucket = "tier-delete-fast-manifest-pass-bucket"; - store - .make_bucket(bucket, &MakeBucketOptions::default()) - .await - .expect("fast manifest pass bucket should be created"); - let incarnation = store - .bucket_incarnation_id(bucket) - .await - .expect("fast manifest pass bucket incarnation should resolve"); let tier_name = "FAST-MANIFEST-PASS"; let backend = register_mock_tier(&ctx.tier_config_mgr(), tier_name).await; let backend_identity = TierConfigMgr::acquire_operation_lease(&ctx.tier_config_mgr(), tier_name) @@ -8889,9 +9261,19 @@ mod tests { .expect("fast manifest pass tier lease should resolve") .backend_identity(); for index in 0..MANIFEST_COUNT { + // Pagination must not depend on same-bucket lock wait deadlines. + let bucket = format!("tier-delete-fast-manifest-pass-{index}"); + store + .make_bucket(&bucket, &MakeBucketOptions::default()) + .await + .expect("fast manifest pass bucket should be created"); + let incarnation = store + .bucket_incarnation_id(&bucket) + .await + .expect("fast manifest pass bucket incarnation should resolve"); install_aborting_dispatch_fixture( store.clone(), - bucket, + &bucket, incarnation, &format!("manifest-page-{index:06}/"), tier_name, @@ -8922,12 +9304,78 @@ mod tests { "one production pass must cross the default eight-manifest page limit" ); assert_eq!(stats.manifests.scanned, MANIFEST_COUNT); - assert_eq!(stats.manifests.deleted, MANIFEST_COUNT); - assert_eq!(stats.manifests.failed, 0); + assert_eq!(stats.manifests.deleted, MANIFEST_COUNT, "full recovery result: {stats:?}"); + assert_eq!(stats.manifests.failed, 0, "full recovery result: {stats:?}"); assert_eq!(manifest_marker, None); assert_eq!(tier_delete_dispatch_manifest_count(store.clone()).await, 0); assert_eq!(tier_delete_journal_count(store).await, 0); assert_eq!(backend.remove_count().await, 0, "rollback recovery must not call the remote tier"); + shutdown.cancel(); + } + + #[cfg(feature = "test-util")] + #[tokio::test] + #[serial_test::serial(storage_class_env)] + async fn tier_delete_manual_pass_retains_manifest_owned_by_startup_recovery() { + let temp_dir = tempfile::tempdir().expect("create automatic recovery ownership store dir"); + let (ctx, store, shutdown) = + without_storage_class_env(build_isolated_test_store(temp_dir.path(), "tier-delete-auto-owner", &[4])).await; + crate::bucket::metadata_sys::init_bucket_metadata_sys(store.clone(), Vec::new()).await; + let bucket = "tier-delete-auto-owner-bucket"; + store + .make_bucket(bucket, &MakeBucketOptions::default()) + .await + .expect("automatic recovery bucket should be created"); + let incarnation = store.bucket_incarnation_id(bucket).await.expect("bucket incarnation"); + let tier_name = "AUTO-OWNER"; + let backend = register_mock_tier(&ctx.tier_config_mgr(), tier_name).await; + let identity = TierConfigMgr::acquire_operation_lease(&ctx.tier_config_mgr(), tier_name) + .await + .expect("automatic recovery tier lease") + .backend_identity(); + + // The automatic worker must not observe a partially installed fixture. + let lifecycle_guard = store + .acquire_bucket_lifecycle_write_lock(bucket) + .await + .expect("fixture lifecycle lock"); + let (manifest_name, entries) = + install_aborting_dispatch_fixture(store.clone(), bucket, incarnation, "auto-owner/", tier_name, identity, 1).await; + let journal_name = tier_delete_journal_object_name(&entries[0]); + let hook = TierDeleteDispatchRollbackTestHook::install_slow_delete(&journal_name, &journal_name); + drop(lifecycle_guard); + ctx.wake_tier_delete_journal_recovery(); + tokio::time::timeout(Duration::from_secs(30), hook.wait_until_delete_paused()) + .await + .expect("startup recovery should own the manifest before a manual pass"); + assert!(tier_delete_dispatch_manifest_recovery_inflight_for_test(&store, &manifest_name)); + + let stats = recover_tier_delete_dispatch_manifests(store.clone(), 8, None) + .await + .expect("manual recovery scan"); + assert_eq!(stats.scanned, 1, "{stats:?}"); + assert_eq!(stats.retained, 1, "{stats:?}"); + assert_eq!(stats.deleted, 0, "{stats:?}"); + assert_eq!(stats.failed, 0, "{stats:?}"); + assert_eq!(tier_delete_dispatch_manifest_count(store.clone()).await, 1); + assert_eq!(tier_delete_journal_count(store.clone()).await, 1); + + hook.release_delete(); + tokio::time::timeout(Duration::from_secs(30), async { + loop { + let manifest_gone = matches!(com::read_config(store.clone(), &manifest_name).await, Err(Error::ConfigNotFound)); + if manifest_gone && !tier_delete_dispatch_manifest_recovery_inflight_for_test(&store, &manifest_name) { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("automatic recovery should converge without a manual retry"); + assert_eq!(tier_delete_dispatch_manifest_count(store.clone()).await, 0); + assert_eq!(tier_delete_journal_count(store).await, 0); + assert_eq!(backend.remove_count().await, 0, "rollback must not delete from the remote tier"); + shutdown.cancel(); } #[cfg(feature = "test-util")] @@ -10302,8 +10750,17 @@ mod tests { const JOURNAL_COUNT: usize = 40; let temp_dir = tempfile::tempdir().expect("create rollback retry store dir"); - let (ctx, store, _shutdown) = - without_storage_class_env(build_isolated_test_store(temp_dir.path(), "dispatch-rollback-retry", &[4])).await; + // Manual retries must own progress between fault removal and the next attempt. + let mut instance_ctx = crate::runtime::instance::InstanceContext::new(); + instance_ctx.suppress_tier_delete_journal_recovery_for_test(); + let (ctx, store, shutdown) = without_storage_class_env(build_isolated_test_store_with_layout( + temp_dir.path(), + "dispatch-rollback-retry", + &[(1, 4)], + CancellationToken::new(), + Some(Arc::new(instance_ctx)), + )) + .await; crate::bucket::metadata_sys::init_bucket_metadata_sys(store.clone(), Vec::new()).await; let bucket = "dispatch-rollback-retry-bucket"; store @@ -10377,6 +10834,7 @@ mod tests { assert_eq!(tier_delete_dispatch_manifest_count(store.clone()).await, 0); assert_eq!(backend.remove_count().await, 0, "rollback retries must never call the remote tier"); + shutdown.cancel(); } #[cfg(feature = "test-util")] @@ -13204,6 +13662,7 @@ mod tests { "partial-set-prefix-delete", &[(2, 4)], CancellationToken::new(), + None, )) .await; crate::bucket::metadata_sys::init_bucket_metadata_sys(store.clone(), Vec::new()).await; @@ -16576,6 +17035,7 @@ mod tests { "prepared-directory-recovery", &[(2, 4)], shutdown, + None, )) .await; crate::bucket::metadata_sys::init_bucket_metadata_sys(store.clone(), Vec::new()).await; @@ -17228,6 +17688,38 @@ mod tests { .expect("test thread should complete"); } + #[cfg(feature = "test-util")] + #[tokio::test] + #[serial_test::serial(storage_class_env)] + async fn odm_write_back_requires_one_set_and_enabled_namespace_locking() { + for (layout, locking, supported) in [ + (&[(1, 4)][..], true, true), + (&[(1, 4), (1, 4)][..], true, false), + (&[(2, 4)][..], true, false), + (&[(1, 4)][..], false, false), + ] { + temp_env::async_with_vars([("RUSTFS_LOCK_ENABLED", Some(if locking { "true" } else { "false" }))], async { + let dir = tempfile::tempdir().expect("isolated topology"); + let shutdown = CancellationToken::new(); + let (_ctx, store, _) = without_storage_class_env(build_isolated_test_store_with_layout( + dir.path(), + "odm-topology", + layout, + shutdown.clone(), + None, + )) + .await; + assert_eq!( + store.supports_atomic_create_only_write_back(), + supported, + "layout={layout:?}, locking={locking}" + ); + shutdown.cancel(); + }) + .await; + } + } + #[cfg(feature = "test-util")] #[tokio::test] #[serial_test::serial(storage_class_env)] diff --git a/crates/ecstore/src/store/mod.rs b/crates/ecstore/src/store/mod.rs index 8a4579e1b..b2f2965f6 100644 --- a/crates/ecstore/src/store/mod.rs +++ b/crates/ecstore/src/store/mod.rs @@ -848,7 +848,7 @@ impl ECStore { } pub fn scanner_namespace_mutation_generation(&self) -> u64 { - list_objects::scanner_namespace_mutation_generation() + list_objects::scanner_namespace_mutation_generation().saturating_add(self.ctx.namespace_commit_generation()) } pub async fn scanner_data_movement_active(&self) -> bool { @@ -857,7 +857,7 @@ impl ECStore { } /// Return the storage-owned movement state and generation as one - /// authenticated activity snapshot. The read lock is acquired before + /// authenticated activity snapshot. The read lock is acquired before /// the state locks (cancelers, pool metadata, then rebalance metadata), /// matching the transition writer order and preventing a terminal state /// from being reported with the preceding generation. @@ -886,11 +886,12 @@ impl ECStore { /// Returns whether scanner metadata may still be hidden by a local /// data-movement state. Terminal failed/canceled decommission entries /// remain suspended until an operator clears or retries them, so they are - /// a publication barrier even after the worker has stopped. + /// a publication barrier even after the worker has stopped. Active PUT + /// rename fanouts also defer publication, including post-ACK tails. pub async fn scanner_data_usage_publication_blocked(&self) -> bool { let operation_gate = self.ctx.data_movement_operation_gate(); let _operation_guard = operation_gate.read_owned().await; - self.scanner_data_usage_publication_snapshot_blocked().await + self.scanner_data_usage_publication_snapshot_blocked().await || self.ctx.namespace_commits_pending() } pub async fn scanner_data_movement_pause_status(&self) -> ScannerDataMovementPauseStatus { @@ -1070,7 +1071,7 @@ impl ECStore { { return Err(Error::other("scanner publication lease generation is stale")); } - if self.scanner_data_movement_snapshot_locked().await.1 { + if self.scanner_data_movement_snapshot_locked().await.1 || self.ctx.namespace_commits_pending() { return Err(Error::other("scanner publication lease is blocked by data movement")); } @@ -1109,7 +1110,7 @@ impl ECStore { { return Err(Error::other("scanner publication lease generation is stale")); } - if self.scanner_data_movement_snapshot_locked().await.1 { + if self.scanner_data_movement_snapshot_locked().await.1 || self.ctx.namespace_commits_pending() { return Err(Error::other("scanner publication lease is blocked by data movement")); } if !self.ctx.scanner_publication_lease_is_active(token).await { @@ -1129,7 +1130,7 @@ impl ECStore { if self.ctx.data_movement_generation_exhausted() || self.ctx.data_movement_operation_epoch_exhausted() { return Err(Error::other("scanner publication lease generation is exhausted")); } - if self.scanner_data_movement_snapshot_locked().await.1 { + if self.scanner_data_movement_snapshot_locked().await.1 || self.ctx.namespace_commits_pending() { return Err(Error::other("scanner publication lease is blocked by data movement")); } let Some(lease_generation) = self.ctx.scanner_publication_lease_generation(token).await else { diff --git a/crates/heal/src/heal/mrf_queue.rs b/crates/heal/src/heal/mrf_queue.rs index 026653d03..901064823 100644 --- a/crates/heal/src/heal/mrf_queue.rs +++ b/crates/heal/src/heal/mrf_queue.rs @@ -45,6 +45,10 @@ use uuid::Uuid; use crate::heal::task::{HealOptions, HealPriority, HealRequest, HealType}; +/// Read-only inspection of committed MRF checkpoints. The legacy consumer +/// remains unchanged until ownership-aware replay is deployed. +pub mod snapshot; + /// Journal location inside the metadata bucket, following the resume-state /// layout. pub(crate) const MRF_JOURNAL_PATH: &str = "buckets/.heal/mrf/journal.bin"; diff --git a/crates/heal/src/heal/mrf_queue/snapshot.rs b/crates/heal/src/heal/mrf_queue/snapshot.rs new file mode 100644 index 000000000..e8d51ed9e --- /dev/null +++ b/crates/heal/src/heal/mrf_queue/snapshot.rs @@ -0,0 +1,681 @@ +// Copyright 2026 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. + +//! Reader-first support for owner-local MRF checkpoints. +//! +//! Each of two slots has a payload and a commit manifest. The manifest binds +//! the writer identity, persistent sequence, length and whole-payload digest. +//! Replacing the inactive slot must leave the previous committed slot intact. +//! Production publication and reclamation are deliberately not enabled here. +//! An unreadable commit path cannot prove that only legacy data exists. This +//! explicit inspection API fails closed and never mutates recovery anchors. +//! It is not wired into the legacy consumer: that transition requires the +//! ownership-aware replay and producer handoff before writer activation. +//! One surviving committed replica supports process restart recovery only; +//! this reader does not establish a replication quorum or a power-loss policy. + +use super::{MRF_JOURNAL_PATH, MRF_SCOPED_JOURNAL_PATH, decode_journal}; +use crate::heal::RUSTFS_META_BUCKET; +use crate::heal::storage_api::owner::{EcstoreDiskAPI, EcstoreDiskError, EcstoreDiskStore}; +use sha2::{Digest, Sha256}; +use std::collections::HashMap; +use tokio::io::AsyncReadExt; +use uuid::Uuid; + +// Root-level control files avoid requiring a new directory before the first +// atomic commit. They remain inside the storage owner's metadata volume. +const PAYLOAD_PATHS: [&str; 2] = [".heal-mrf-snapshot.0.bin", ".heal-mrf-snapshot.1.bin"]; +const MANIFEST_PATHS: [&str; 2] = [".heal-mrf-commit.0.bin", ".heal-mrf-commit.1.bin"]; +const MAGIC: &[u8; 8] = b"RFMRFC01"; +const MANIFEST_LEN: usize = 8 + 1 + 16 + 8 + 8 + 32 + 32; +const VERSION: u8 = 1; + +#[derive(Debug, thiserror::Error)] +pub enum SnapshotError { + #[error("MRF checkpoint has an invalid or incomplete commit record")] + Corrupt, + #[error("MRF checkpoint format is unsupported")] + Unsupported, + #[error("MRF checkpoint exceeds the configured byte limit")] + TooLarge, + #[error("MRF checkpoint replicas disagree at the same sequence")] + Conflict, + #[error("MRF checkpoint storage is unavailable")] + Disk(#[source] EcstoreDiskError), + #[error("MRF checkpoint body could not be read")] + Read(#[source] std::io::Error), +} + +#[derive(Debug, PartialEq, Eq)] +struct Manifest { + owner: Uuid, + sequence: u64, + payload_len: usize, + payload_digest: [u8; 32], +} + +impl Manifest { + fn decode(bytes: &[u8], limit: usize) -> Result { + if bytes.len() != MANIFEST_LEN || &bytes[..8] != MAGIC { + return Err(SnapshotError::Corrupt); + } + if bytes[8] != VERSION { + return Err(SnapshotError::Unsupported); + } + let signed = MANIFEST_LEN - 32; + let checksum: [u8; 32] = Sha256::digest(&bytes[..signed]).into(); + if checksum != bytes[signed..] { + return Err(SnapshotError::Corrupt); + } + let owner = Uuid::from_slice(&bytes[9..25]).map_err(|_| SnapshotError::Corrupt)?; + let sequence = u64::from_le_bytes(bytes[25..33].try_into().map_err(|_| SnapshotError::Corrupt)?); + let payload_len = u64::from_le_bytes(bytes[33..41].try_into().map_err(|_| SnapshotError::Corrupt)?); + let payload_len = usize::try_from(payload_len).map_err(|_| SnapshotError::TooLarge)?; + if owner.is_nil() || sequence == 0 || sequence == u64::MAX { + return Err(SnapshotError::Corrupt); + } + if payload_len > limit { + return Err(SnapshotError::TooLarge); + } + Ok(Self { + owner, + sequence, + payload_len, + payload_digest: bytes[41..73].try_into().map_err(|_| SnapshotError::Corrupt)?, + }) + } +} + +#[derive(Debug)] +pub struct CommittedSnapshot { + manifest: Manifest, + payload: Vec, +} + +impl CommittedSnapshot { + /// Persistent single-writer sequence, not a process UUID ordering. + pub fn sequence(&self) -> u64 { + self.manifest.sequence + } + + /// Identity recorded by the committed checkpoint's writer. + pub fn owner(&self) -> Uuid { + self.manifest.owner + } + + /// Complete, checksum-validated record bytes. Inspection does not consume + /// these records or acknowledge completion to any producer. + pub fn payload(&self) -> &[u8] { + &self.payload + } + + fn decode(manifest: &[u8], payload: Vec, limit: usize) -> Result { + let manifest = Manifest::decode(manifest, limit)?; + let checksum: [u8; 32] = Sha256::digest(&payload).into(); + if payload.len() != manifest.payload_len || checksum != manifest.payload_digest { + return Err(SnapshotError::Corrupt); + } + if decode_journal(&payload).1 != 0 { + return Err(SnapshotError::Corrupt); + } + Ok(Self { manifest, payload }) + } +} + +#[derive(Debug)] +pub enum RecoverySnapshot { + /// An intact legacy snapshot, without a comparable commit sequence. + Legacy(Vec), + /// A committed checkpoint requiring ownership-aware replay before use. + Committed(CommittedSnapshot), +} + +async fn read_bounded(disk: &EcstoreDiskStore, path: &str, limit: usize) -> Result>, SnapshotError> { + let reader = match EcstoreDiskAPI::read_file(disk.as_ref(), RUSTFS_META_BUCKET, path).await { + Ok(reader) => reader, + Err(EcstoreDiskError::FileNotFound | EcstoreDiskError::VolumeNotFound) => return Ok(None), + Err(error) => return Err(SnapshotError::Disk(error)), + }; + let maximum = limit.checked_add(1).ok_or(SnapshotError::TooLarge)?; + let maximum = u64::try_from(maximum).map_err(|_| SnapshotError::TooLarge)?; + let mut bytes = Vec::new(); + reader + .take(maximum) + .read_to_end(&mut bytes) + .await + .map_err(SnapshotError::Read)?; + if bytes.len() > limit { + return Err(SnapshotError::TooLarge); + } + Ok(Some(bytes)) +} + +fn select_snapshot(selected: &mut Option, candidate: CommittedSnapshot) -> Result<(), SnapshotError> { + if let Some(current) = selected { + if current.manifest.sequence == candidate.manifest.sequence + && (current.manifest != candidate.manifest || current.payload != candidate.payload) + { + return Err(SnapshotError::Conflict); + } + if current.manifest.sequence >= candidate.manifest.sequence { + return Ok(()); + } + } + *selected = Some(candidate); + Ok(()) +} + +async fn read_committed(disks: &[EcstoreDiskStore], limit: usize) -> Result, SnapshotError> { + let mut selected = None; + let mut damaged = None; + let mut identities = HashMap::new(); + for disk in disks { + for (manifest_path, payload_path) in MANIFEST_PATHS.into_iter().zip(PAYLOAD_PATHS) { + let candidate = async { + let Some(manifest) = read_bounded(disk, manifest_path, MANIFEST_LEN).await? else { + return Ok(None); + }; + let header = Manifest::decode(&manifest, limit)?; + let payload = read_bounded(disk, payload_path, header.payload_len) + .await? + .ok_or(SnapshotError::Corrupt)?; + CommittedSnapshot::decode(&manifest, payload, limit).map(Some) + } + .await; + match candidate { + Ok(Some(candidate)) => { + let identity = ( + candidate.manifest.owner, + candidate.manifest.payload_len, + candidate.manifest.payload_digest, + ); + if identities + .insert(candidate.manifest.sequence, identity) + .is_some_and(|previous| previous != identity) + { + return Err(SnapshotError::Conflict); + } + select_snapshot(&mut selected, candidate)?; + } + Ok(None) => {} + // A future committed format may supersede all readable slots. + Err(SnapshotError::Unsupported) => return Err(SnapshotError::Unsupported), + Err(error) => damaged = Some(error), + } + } + } + match (selected, damaged) { + (Some(snapshot), _) => Ok(Some(snapshot)), + (None, Some(error)) => Err(error), + (None, None) => Ok(None), + } +} + +async fn read_legacy(disks: &[EcstoreDiskStore], path: &str, limit: usize) -> Result>, SnapshotError> { + let mut selected = None; + let mut incomplete: Option> = None; + for disk in disks { + match read_bounded(disk, path, limit).await { + Ok(Some(payload)) if decode_journal(&payload).1 == 0 => { + if selected.as_ref().is_some_and(|current| *current != payload) { + // Legacy snapshots have no sequence. There is no evidence + // that the first, longest or nonempty replica is newest. + return Err(SnapshotError::Conflict); + } + selected = Some(payload); + } + Ok(Some(payload)) => { + if let Some(previous) = &incomplete { + if previous.starts_with(&payload) { + continue; + } + if !payload.starts_with(previous) { + return Err(SnapshotError::Corrupt); + } + } + incomplete = Some(payload); + } + Ok(None) => {} + Err(error) => return Err(error), + } + } + if let Some(prefix) = incomplete + && !selected.as_ref().is_some_and(|payload| payload.starts_with(&prefix)) + { + // In particular, an empty O_TRUNC replica cannot supersede another + // replica containing intact records followed by a torn tail. + return Err(SnapshotError::Corrupt); + } + Ok(selected) +} + +/// Inspect local MRF checkpoints without replaying, acknowledging or deleting. +/// +/// `max_bytes` bounds each payload read. Every local replica is examined and +/// ambiguous identities, unavailable proof or unsupported formats return a +/// typed error. This API must not authorize a writer without the separate +/// ownership and mixed-version activation checks. +pub async fn inspect_local_recovery_snapshot(max_bytes: usize) -> Result, SnapshotError> { + read_recovery_snapshot(&super::journal_disks().await, max_bytes).await +} + +async fn read_recovery_snapshot(disks: &[EcstoreDiskStore], limit: usize) -> Result, SnapshotError> { + if let Some(snapshot) = read_committed(disks, limit).await? { + return Ok(Some(RecoverySnapshot::Committed(snapshot))); + } + // RUSTFS_COMPAT_TODO(backlog-2263): inspect retained legacy MRF journals. Remove after all supported upgrade and rollback readers understand committed snapshots and retained journals have migrated. + if let Some(payload) = read_legacy(disks, MRF_SCOPED_JOURNAL_PATH, limit).await? { + return Ok(Some(RecoverySnapshot::Legacy(payload))); + } + Ok(read_legacy(disks, MRF_JOURNAL_PATH, limit) + .await? + .map(RecoverySnapshot::Legacy)) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::heal::mrf_queue::encode_intent; + use crate::heal::storage_api::owner::{EcstoreConditionalFileUpdate, EcstoreDiskBytes}; + use crate::heal::{DiskOption, Endpoint, new_disk}; + use rustfs_common::mrf_channel::{MrfIntent, MrfKind, MrfScope}; + use std::sync::Arc; + use tempfile::TempDir; + + fn payload(object: &str) -> Vec { + let intent = MrfIntent { + bucket: Arc::from("bucket"), + object: Arc::from(object), + version_id: None, + kind: MrfKind::PartialWrite, + scope: None, + lease: None, + enqueued_at_ms: 1234, + attempts: 0, + }; + let mut bytes = Vec::new(); + assert!(encode_intent(&intent, &mut bytes), "fixture must encode a full record"); + bytes + } + + fn manifest(owner: Uuid, sequence: u64, payload: &[u8]) -> Vec { + let mut bytes = Vec::with_capacity(MANIFEST_LEN); + bytes.extend_from_slice(MAGIC); + bytes.push(VERSION); + bytes.extend_from_slice(owner.as_bytes()); + bytes.extend_from_slice(&sequence.to_le_bytes()); + bytes.extend_from_slice(&u64::try_from(payload.len()).expect("fixture length fits").to_le_bytes()); + bytes.extend_from_slice(&Sha256::digest(payload)); + bytes.extend_from_slice(&Sha256::digest(&bytes)); + bytes + } + + async fn disk(root: &TempDir, name: &str) -> EcstoreDiskStore { + let path = root.path().join(name); + std::fs::create_dir_all(&path).expect("create disk directory"); + let endpoint = Endpoint::try_from(path.to_string_lossy().as_ref()).expect("valid disk endpoint"); + let disk = new_disk( + &endpoint, + &DiskOption { + cleanup: false, + health_check: false, + }, + ) + .await + .expect("open disk"); + let result = EcstoreDiskAPI::make_volume(disk.as_ref(), RUSTFS_META_BUCKET).await; + assert!( + matches!(result, Ok(()) | Err(EcstoreDiskError::VolumeExists)), + "metadata volume: {result:?}" + ); + disk + } + + // Exercise the existing storage owner's atomic CAS primitive. No production + // caller publishes this format until ownership-aware replay is available. + async fn install(disk: &EcstoreDiskStore, path: &str, bytes: &[u8]) { + let expected = EcstoreDiskAPI::read_all(disk.as_ref(), RUSTFS_META_BUCKET, path).await.ok(); + let result = EcstoreDiskAPI::compare_and_update_file( + disk.as_ref(), + RUSTFS_META_BUCKET, + path, + expected, + Some(EcstoreDiskBytes::copy_from_slice(bytes)), + ) + .await + .expect("atomic snapshot slot write"); + assert_eq!(result, EcstoreConditionalFileUpdate::Updated); + } + + async fn commit(disk: &EcstoreDiskStore, slot: usize, owner: Uuid, sequence: u64, bytes: &[u8]) { + install(disk, PAYLOAD_PATHS[slot], bytes).await; + install(disk, MANIFEST_PATHS[slot], &manifest(owner, sequence, bytes)).await; + } + + #[test] + fn manifest_validates_identity_sequence_length_and_digest() { + let bytes = payload("object"); + let owner = Uuid::new_v4(); + assert!(CommittedSnapshot::decode(&manifest(owner, 1, &bytes), bytes.clone(), bytes.len()).is_ok()); + for (owner, sequence) in [(Uuid::nil(), 1), (owner, 0), (owner, u64::MAX)] { + assert!(matches!( + Manifest::decode(&manifest(owner, sequence, &bytes), bytes.len()), + Err(SnapshotError::Corrupt) + )); + } + assert!(matches!( + Manifest::decode(&manifest(owner, 1, &bytes), bytes.len() - 1), + Err(SnapshotError::TooLarge) + )); + let mut corrupt = manifest(owner, 1, &bytes); + corrupt[25] ^= 1; + assert!(matches!(Manifest::decode(&corrupt, bytes.len()), Err(SnapshotError::Corrupt))); + let mut unsupported = manifest(owner, 1, &bytes); + unsupported[8] = 2; + assert!(matches!(Manifest::decode(&unsupported, bytes.len()), Err(SnapshotError::Unsupported))); + } + + #[test] + fn whole_payload_integrity_is_required_even_with_a_valid_manifest() { + let bytes = payload("object"); + let owner = Uuid::new_v4(); + let header = manifest(owner, 1, &bytes); + assert!(matches!( + CommittedSnapshot::decode(&header, bytes[..bytes.len() - 1].to_vec(), bytes.len()), + Err(SnapshotError::Corrupt) + )); + let invalid = b"not an MRF record".to_vec(); + assert!(matches!( + CommittedSnapshot::decode(&manifest(owner, 2, &invalid), invalid, bytes.len()), + Err(SnapshotError::Corrupt) + )); + } + + #[tokio::test] + async fn newest_complete_replica_wins_in_both_disk_orders() { + let root = TempDir::new().expect("test directory"); + let first = disk(&root, "first").await; + let second = disk(&root, "second").await; + let owner = Uuid::new_v4(); + commit(&first, 0, owner, 1, &payload("old")).await; + commit(&second, 1, owner, 2, &payload("new")).await; + for disks in [vec![first.clone(), second.clone()], vec![second.clone(), first.clone()]] { + let recovered = read_committed(&disks, 4096) + .await + .expect("read replicas") + .expect("committed snapshot"); + assert_eq!(recovered.manifest.sequence, 2); + assert_eq!(recovered.payload, payload("new")); + } + } + + #[tokio::test] + async fn divergent_commits_at_same_sequence_fail_closed() { + let root = TempDir::new().expect("test directory"); + let first = disk(&root, "first").await; + let second = disk(&root, "second").await; + let owner = Uuid::new_v4(); + commit(&first, 0, owner, 7, &payload("a")).await; + commit(&second, 1, owner, 7, &payload("b")).await; + assert!(matches!(read_committed(&[first, second], 4096).await, Err(SnapshotError::Conflict))); + } + + #[tokio::test] + async fn newer_slot_does_not_hide_a_conflicting_commit_history() { + let root = TempDir::new().expect("test directory"); + let first = disk(&root, "first").await; + let second = disk(&root, "second").await; + let owner = Uuid::new_v4(); + commit(&first, 0, owner, 8, &payload("newest")).await; + commit(&first, 1, owner, 7, &payload("a")).await; + commit(&second, 1, owner, 7, &payload("b")).await; + assert!(matches!(read_committed(&[first, second], 4096).await, Err(SnapshotError::Conflict))); + } + + #[tokio::test] + async fn uncommitted_or_torn_successor_preserves_previous_slot() { + let root = TempDir::new().expect("test directory"); + let disk = disk(&root, "disk").await; + let owner = Uuid::new_v4(); + let old = payload("old"); + let next = payload("next"); + commit(&disk, 0, owner, 1, &old).await; + install(&disk, PAYLOAD_PATHS[1], &next).await; + let recovered = read_committed(std::slice::from_ref(&disk), 4096) + .await + .expect("staged payload is not a commit") + .expect("old snapshot"); + assert_eq!(recovered.payload, old); + install(&disk, MANIFEST_PATHS[1], &manifest(owner, 2, &next)[..20]).await; + let recovered = read_committed(std::slice::from_ref(&disk), 4096) + .await + .expect("torn manifest preserves old slot") + .expect("old snapshot"); + assert_eq!(recovered.manifest.sequence, 1); + install(&disk, MANIFEST_PATHS[1], &manifest(owner, 2, &next)).await; + install(&disk, PAYLOAD_PATHS[1], b"torn").await; + let recovered = read_committed(&[disk], 4096) + .await + .expect("torn payload preserves old slot") + .expect("old snapshot"); + assert_eq!(recovered.manifest.sequence, 1); + } + + #[tokio::test] + async fn stale_manifest_cas_cannot_replace_committed_anchor() { + let root = TempDir::new().expect("test directory"); + let disk = disk(&root, "disk").await; + let owner = Uuid::new_v4(); + let bytes = payload("object"); + commit(&disk, 0, owner, 1, &bytes).await; + let result = EcstoreDiskAPI::compare_and_update_file( + disk.as_ref(), + RUSTFS_META_BUCKET, + MANIFEST_PATHS[0], + None, + Some(manifest(owner, 2, &bytes).into()), + ) + .await + .expect("CAS call"); + assert_eq!(result, EcstoreConditionalFileUpdate::Mismatch); + let recovered = read_committed(&[disk], 4096) + .await + .expect("read old anchor") + .expect("snapshot"); + assert_eq!(recovered.manifest.sequence, 1); + } + + #[tokio::test] + async fn legacy_import_requires_complete_consistent_replicas() { + let root = TempDir::new().expect("test directory"); + let first = disk(&root, "first").await; + let second = disk(&root, "second").await; + let bytes = payload("object"); + for (disk, data) in [(&first, &bytes[..bytes.len() - 1]), (&second, bytes.as_slice())] { + EcstoreDiskAPI::write_all( + disk.as_ref(), + RUSTFS_META_BUCKET, + MRF_SCOPED_JOURNAL_PATH, + EcstoreDiskBytes::copy_from_slice(data), + ) + .await + .expect("legacy fixture"); + } + let disks = [first.clone(), second]; + assert!( + matches!(read_recovery_snapshot(&disks, 4096).await.expect("intact legacy replica"), Some(RecoverySnapshot::Legacy(data)) if data == bytes) + ); + EcstoreDiskAPI::write_all(first.as_ref(), RUSTFS_META_BUCKET, MRF_SCOPED_JOURNAL_PATH, payload("different").into()) + .await + .expect("divergent fixture"); + assert!(matches!(read_recovery_snapshot(&disks, 4096).await, Err(SnapshotError::Conflict))); + } + + #[tokio::test] + async fn committed_inspection_leaves_payload_and_manifest_unchanged() { + let root = TempDir::new().expect("test directory"); + let disk = disk(&root, "disk").await; + let owner = Uuid::new_v4(); + let bytes = payload("object"); + commit(&disk, 0, owner, 3, &bytes).await; + assert!(matches!( + read_recovery_snapshot(std::slice::from_ref(&disk), 4096) + .await + .expect("new snapshot"), + Some(RecoverySnapshot::Committed(_)) + )); + assert_eq!( + EcstoreDiskAPI::read_all(disk.as_ref(), RUSTFS_META_BUCKET, MANIFEST_PATHS[0]) + .await + .expect("manifest retained") + .as_ref(), + manifest(owner, 3, &bytes) + ); + assert_eq!( + EcstoreDiskAPI::read_all(disk.as_ref(), RUSTFS_META_BUCKET, PAYLOAD_PATHS[0]) + .await + .expect("payload retained") + .as_ref(), + bytes + ); + } + + #[tokio::test] + async fn legacy_inspection_rejects_complete_subsets_and_scope_ambiguity() { + let scoped = |set_index| { + let intent = MrfIntent { + bucket: Arc::from("bucket"), + object: Arc::from("a"), + version_id: None, + kind: MrfKind::PartialWrite, + scope: Some(MrfScope { + pool_index: 0, + set_index, + }), + lease: None, + enqueued_at_ms: 1234, + attempts: 0, + }; + let mut bytes = Vec::new(); + assert!(encode_intent(&intent, &mut bytes), "scoped fixture must encode"); + bytes + }; + let mut superset = payload("a"); + superset.extend_from_slice(&payload("b")); + for (case, first_bytes, second_bytes) in [ + ("complete-subset", payload("a"), superset), + ("different-set", scoped(1), scoped(2)), + ("unknown-scope", payload("a"), scoped(1)), + ] { + let root = TempDir::new().expect("test directory"); + let first = disk(&root, "first").await; + let second = disk(&root, "second").await; + for (disk, bytes) in [(&first, &first_bytes), (&second, &second_bytes)] { + assert_eq!(decode_journal(bytes).1, 0, "{case}: complete fixture"); + EcstoreDiskAPI::write_all( + disk.as_ref(), + RUSTFS_META_BUCKET, + MRF_SCOPED_JOURNAL_PATH, + EcstoreDiskBytes::copy_from_slice(bytes), + ) + .await + .expect("write legacy replica"); + } + for disks in [vec![first.clone(), second.clone()], vec![second.clone(), first.clone()]] { + assert!( + matches!(read_recovery_snapshot(&disks, 4096).await, Err(SnapshotError::Conflict)), + "{case}: neither replica order proves a latest snapshot" + ); + } + for (disk, bytes) in [(&first, &first_bytes), (&second, &second_bytes)] { + assert_eq!( + EcstoreDiskAPI::read_all(disk.as_ref(), RUSTFS_META_BUCKET, MRF_SCOPED_JOURNAL_PATH) + .await + .expect("legacy evidence retained") + .as_ref(), + bytes.as_slice(), + "{case}: inspection must preserve both source replicas" + ); + } + } + } + + #[tokio::test] + async fn oversized_or_corrupt_scoped_snapshot_never_falls_back_to_legacy() { + let root = TempDir::new().expect("test directory"); + let disk = disk(&root, "disk").await; + EcstoreDiskAPI::write_all(disk.as_ref(), RUSTFS_META_BUCKET, MRF_SCOPED_JOURNAL_PATH, vec![0; 1025].into()) + .await + .expect("oversized fixture"); + EcstoreDiskAPI::write_all(disk.as_ref(), RUSTFS_META_BUCKET, MRF_JOURNAL_PATH, payload("old").into()) + .await + .expect("legacy fixture"); + assert!(matches!( + read_recovery_snapshot(std::slice::from_ref(&disk), 1024).await, + Err(SnapshotError::TooLarge) + )); + assert!(matches!(read_recovery_snapshot(&[disk], 2048).await, Err(SnapshotError::Corrupt))); + } + + #[tokio::test] + async fn empty_legacy_replica_cannot_erase_records_in_a_torn_replica() { + let root = TempDir::new().expect("test directory"); + let first = disk(&root, "first").await; + let second = disk(&root, "second").await; + let mut incomplete = payload("durable-object"); + incomplete.extend_from_slice(b"torn"); + EcstoreDiskAPI::write_all(first.as_ref(), RUSTFS_META_BUCKET, MRF_SCOPED_JOURNAL_PATH, Vec::new().into()) + .await + .expect("empty truncated replica"); + EcstoreDiskAPI::write_all(second.as_ref(), RUSTFS_META_BUCKET, MRF_SCOPED_JOURNAL_PATH, incomplete.clone().into()) + .await + .expect("records and torn tail"); + for disks in [vec![first.clone(), second.clone()], vec![second.clone(), first.clone()]] { + assert!(matches!(read_recovery_snapshot(&disks, 4096).await, Err(SnapshotError::Corrupt))); + } + assert_eq!( + EcstoreDiskAPI::read_all(second.as_ref(), RUSTFS_META_BUCKET, MRF_SCOPED_JOURNAL_PATH) + .await + .expect("recovery anchor preserved") + .as_ref(), + incomplete + ); + } + + #[tokio::test] + async fn unreadable_commit_record_never_implies_legacy_only() { + let root = TempDir::new().expect("test directory"); + let disk = disk(&root, "disk").await; + let legacy = payload("old"); + EcstoreDiskAPI::write_all(disk.as_ref(), RUSTFS_META_BUCKET, MRF_JOURNAL_PATH, legacy.clone().into()) + .await + .expect("legacy fixture"); + // Opening a directory as a record either fails at open or at read, + // depending on the platform. Neither outcome proves absence. + std::fs::create_dir(root.path().join("disk").join(RUSTFS_META_BUCKET).join(MANIFEST_PATHS[0])) + .expect("unreadable manifest fixture"); + let recovered = read_recovery_snapshot(std::slice::from_ref(&disk), 4096).await; + assert!( + matches!(recovered, Err(SnapshotError::Disk(_) | SnapshotError::Read(_))), + "must preserve unavailable proof: {recovered:?}" + ); + assert_eq!( + EcstoreDiskAPI::read_all(disk.as_ref(), RUSTFS_META_BUCKET, MRF_JOURNAL_PATH) + .await + .expect("legacy remains") + .as_ref(), + legacy + ); + } +} diff --git a/crates/heal/tests/heal_b5_versioned_regression_test.rs b/crates/heal/tests/heal_b5_versioned_regression_test.rs index 81cfdc79a..7b291f3db 100644 --- a/crates/heal/tests/heal_b5_versioned_regression_test.rs +++ b/crates/heal/tests/heal_b5_versioned_regression_test.rs @@ -44,7 +44,9 @@ use walkdir::WalkDir; mod storage_api; -use storage_api::integration::{BucketOperations, ECStore, MakeBucketOptions, ObjectIO as _, ObjectOperations as _}; +use storage_api::integration::{ + BucketOperations, ECStore, MakeBucketOptions, NamespaceLocking as _, ObjectIO as _, ObjectOperations as _, +}; /// 256 KiB + change: large enough to be stored as non-inline erasure shards /// (so each data version materializes as an on-disk `part.*` file we can assert @@ -106,6 +108,7 @@ async fn put_versioned(ecstore: &Arc, bucket: &str, object: &str, data: .put_object(bucket, object, &mut reader, &opts) .await .expect("versioned put_object failed"); + wait_for_put_tail(ecstore, bucket, object).await; info.version_id .map(|u| u.to_string()) .expect("versioned put must return a version id") @@ -117,6 +120,7 @@ async fn put_unversioned(ecstore: &Arc, bucket: &str, object: &str, dat .put_object(bucket, object, &mut reader, &ObjectOptions::default()) .await .expect("unversioned put_object failed"); + wait_for_put_tail(ecstore, bucket, object).await; } /// Create a delete-marker as the latest version (versioned:true, no version_id) @@ -160,20 +164,16 @@ fn xl_meta_path(obj_dir: &Path) -> PathBuf { obj_dir.join("xl.meta") } -async fn wait_for_two_version_copies(disks: &[PathBuf], bucket: &str, object: &str) { - tokio::time::timeout(Duration::from_secs(5), async { - loop { - if disks.iter().all(|disk| { - let object_dir = object_dir(disk, bucket, object); - xl_meta_path(&object_dir).exists() && count_part_files(&object_dir) >= 2 - }) { - break; - } - tokio::time::sleep(Duration::from_millis(10)).await; - } - }) - .await - .expect("PUT rename tails must converge before wiping the versioned fixture"); +async fn wait_for_put_tail(ecstore: &Arc, bucket: &str, object: &str) { + // Shards and xl.meta can exist before the detached PUT owner finishes. + let lock = ecstore + .new_ns_lock(bucket, object) + .await + .expect("fixture namespace lock should be created"); + let _settled = lock + .get_write_lock(Duration::from_secs(30)) + .await + .expect("PUT rename tail must finish before inspecting or wiping the fixture"); } fn recreate_heal_opts() -> HealOpts { @@ -305,7 +305,13 @@ mod serial_tests { let data_v2 = versioned_test_data(20); let v1 = put_versioned(&ecstore, bucket, object, &data_v1).await; // OLD, non-latest let v2 = put_versioned(&ecstore, bucket, object, &data_v2).await; // latest - wait_for_two_version_copies(&disk_paths, bucket, object).await; + assert!( + disk_paths.iter().all(|disk| { + let dir = object_dir(disk, bucket, object); + xl_meta_path(&dir).exists() && count_part_files(&dir) >= 2 + }), + "both versions must exist on every disk before wiping the fixture" + ); // ── Pre-wipe: prove the fixture actually has 2 versions on disk[0] ── let obj_dir0 = object_dir(&disk_paths[0], bucket, object); diff --git a/crates/heal/tests/storage_api.rs b/crates/heal/tests/storage_api.rs index d224f4dfc..341834f57 100644 --- a/crates/heal/tests/storage_api.rs +++ b/crates/heal/tests/storage_api.rs @@ -23,6 +23,7 @@ pub(crate) mod integration { pub(crate) use rustfs_ecstore::api::storage::ECStore; pub(crate) use rustfs_storage_api::BucketOperations; pub(crate) use rustfs_storage_api::MakeBucketOptions; + pub(crate) use rustfs_storage_api::NamespaceLocking; pub(crate) use rustfs_storage_api::ObjectIO; pub(crate) use rustfs_storage_api::ObjectOperations; } diff --git a/crates/lifecycle/src/core.rs b/crates/lifecycle/src/core.rs index fc959b92a..bfa2b93c1 100644 --- a/crates/lifecycle/src/core.rs +++ b/crates/lifecycle/src/core.rs @@ -43,6 +43,10 @@ const ERR_LIFECYCLE_BUCKET_LOCKED: &str = "ExpiredObjectAllVersions element and DelMarkerExpiration action cannot be used on an object locked bucket"; const ERR_LIFECYCLE_TOO_MANY_RULES: &str = "Lifecycle configuration should have at most 1000 rules"; const ERR_LIFECYCLE_INVALID_EXPIRATION_DAYS: &str = "'Days' for Expiration action must be a positive integer"; +const ERR_LIFECYCLE_EXPIRATION_DAYS_DATE_CONFLICT: &str = "Expiration cannot specify both Days and Date"; +const ERR_LIFECYCLE_MULTIPLE_TRANSITIONS: &str = "Only one Transition action per lifecycle rule is supported"; +const ERR_LIFECYCLE_MULTIPLE_NONCURRENT_TRANSITIONS: &str = + "Only one NoncurrentVersionTransition action per lifecycle rule is supported"; const ERR_LIFECYCLE_INVALID_NONCURRENT_EXPIRATION_DAYS: &str = "'NoncurrentDays' for NoncurrentVersionExpiration action must be a positive integer"; const ERR_LIFECYCLE_INVALID_ABORT_INCOMPLETE_MPU_DAYS: &str = @@ -361,6 +365,12 @@ impl Lifecycle for BucketLifecycleConfiguration { { return Err(std::io::Error::other(ERR_LIFECYCLE_INVALID_EXPIRED_OBJECT_ALL_VERSIONS)); } + if expiration.days.is_some() && expiration.date.is_some() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + ERR_LIFECYCLE_EXPIRATION_DAYS_DATE_CONFLICT, + )); + } if let Some(expiration_date) = &expiration.date { let date = OffsetDateTime::from(expiration_date.clone()); if date.hour() != 0 || date.minute() != 0 || date.second() != 0 || date.nanosecond() != 0 { @@ -394,11 +404,20 @@ impl Lifecycle for BucketLifecycleConfiguration { } } if let Some(transitions) = &r.transitions { + if transitions.len() > 1 { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, ERR_LIFECYCLE_MULTIPLE_TRANSITIONS)); + } for transition in transitions { TransitionOps::validate(transition)?; } } if let Some(noncurrent_transitions) = &r.noncurrent_version_transitions { + if noncurrent_transitions.len() > 1 { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + ERR_LIFECYCLE_MULTIPLE_NONCURRENT_TRANSITIONS, + )); + } for transition in noncurrent_transitions { NoncurrentVersionTransitionOps::validate(transition)?; } @@ -473,6 +492,8 @@ impl Lifecycle for BucketLifecycleConfiguration { } async fn eval(&self, obj: &ObjectOpts) -> Event { + // A single-object lookup cannot prove how many newer historical versions + // survive. Count-dependent actions wait for the complete-group evaluator. self.eval_inner(obj, OffsetDateTime::now_utc(), 0).await } @@ -536,23 +557,8 @@ impl Lifecycle for BucketLifecycleConfiguration { return Event::default(); }; - if let Some(restore_expires) = obj.restore_expires - && restore_expires.unix_timestamp() != 0 - && now.unix_timestamp() > restore_expires.unix_timestamp() - { - let mut action = IlmAction::DeleteRestoredAction; - if !obj.is_latest { - action = IlmAction::DeleteRestoredVersionAction; - } - - events.push(Event { - action, - due: Some(now), - rule_id: "".into(), - noncurrent_days: 0, - newer_noncurrent_versions: 0, - storage_class: "".into(), - }); + if let Some(event) = obj.restored_copy_expiry(now) { + events.push(event); } if let Some(ref lc_rules) = self.filter_rules(obj).await { @@ -611,17 +617,12 @@ impl Lifecycle for BucketLifecycleConfiguration { continue; } - if !obj.is_latest - && let Some(ref noncurrent_version_expiration) = rule.noncurrent_version_expiration - && let Some(retain_newer_noncurrent_versions) = noncurrent_version_expiration.newer_noncurrent_versions - && newer_noncurrent_versions < usize::try_from(retain_newer_noncurrent_versions).unwrap_or(usize::MAX) - { - continue; - } - if !obj.is_latest && let Some(ref noncurrent_version_expiration) = rule.noncurrent_version_expiration && let Some(noncurrent_days) = noncurrent_version_expiration.noncurrent_days + && noncurrent_version_expiration + .newer_noncurrent_versions + .is_none_or(|retain| usize::try_from(retain).is_ok_and(|retain| newer_noncurrent_versions >= retain)) { if let Some(successor_mod_time) = obj.successor_mod_time { let expected_expiry = expected_expiry_time(successor_mod_time, noncurrent_days); @@ -651,7 +652,11 @@ impl Lifecycle for BucketLifecycleConfiguration { && let Some(noncurrent_version_transition) = rule .noncurrent_version_transitions .as_ref() + .filter(|transitions| transitions.len() == 1) .and_then(|transitions| transitions.first()) + && noncurrent_version_transition + .newer_noncurrent_versions + .is_none_or(|retain| usize::try_from(retain).is_ok_and(|retain| newer_noncurrent_versions >= retain)) && let Some(storage_class) = noncurrent_version_transition.storage_class.as_ref() && !storage_class.as_str().is_empty() && !obj.delete_marker @@ -735,7 +740,11 @@ impl Lifecycle for BucketLifecycleConfiguration { } if obj.transition_status != TRANSITION_COMPLETE - && let Some(transition) = rule.transitions.as_ref().and_then(|transitions| transitions.first()) + && let Some(transition) = rule + .transitions + .as_ref() + .filter(|transitions| transitions.len() == 1) + .and_then(|transitions| transitions.first()) && let Some(storage_class) = transition.storage_class.as_ref() && !storage_class.as_str().is_empty() { @@ -758,18 +767,15 @@ impl Lifecycle for BucketLifecycleConfiguration { } if !events.is_empty() { - // Select the winning event using a strict total order (MinIO semantics): - // the earliest `due` wins, and ties break toward delete-type actions. A - // missing `due` is treated as UNIX_EPOCH. This replaces a hand-written - // `sort_by` comparator that was not a strict weak ordering (it could return - // `Ordering::Less` for both `(a, b)` and `(b, a)`), which panics on the - // repository toolchain and did not deterministically pick the earliest event. + // Eligible expiration takes precedence over transition, even when a + // failed transition has an earlier deadline. Within each action class, + // prefer the earliest deadline using a deterministic total order. let event = events .iter() .min_by_key(|event| { ( - event.due.unwrap_or(OffsetDateTime::UNIX_EPOCH).unix_timestamp(), ilm_action_priority_rank(&event.action), + event.due.unwrap_or(OffsetDateTime::UNIX_EPOCH).unix_timestamp(), ) }) .cloned() @@ -1042,6 +1048,27 @@ impl ObjectOpts { pub fn expired_object_deletemarker(&self) -> bool { self.delete_marker && self.is_latest && self.num_versions == 1 } + + pub(crate) fn restored_copy_expiry(&self, now: OffsetDateTime) -> Option { + let restore_expires = self.restore_expires?; + // Restore metadata alone does not prove that a durable remote copy exists. + if self.transition_status != TRANSITION_COMPLETE + || restore_expires.unix_timestamp() == 0 + || now.unix_timestamp() <= restore_expires.unix_timestamp() + { + return None; + } + let action = if self.is_latest { + IlmAction::DeleteRestoredAction + } else { + IlmAction::DeleteRestoredVersionAction + }; + expiration_action_has_valid_target(action, self.version_id, self.is_latest, self.delete_marker).then(|| Event { + action, + due: Some(now), + ..Default::default() + }) + } } /// Returns whether an expiry action has enough identity to target the object @@ -1064,11 +1091,8 @@ pub fn expiration_action_has_valid_target( } } -/// Total-order rank for lifecycle actions used to break `due` ties. -/// -/// Delete-type actions rank before every other action so that, when two events -/// share the same `due`, a delete wins (MinIO semantics). The concrete numeric -/// values only matter relative to each other. +/// Eligible logical expiration takes precedence over transition and restore-copy +/// cleanup. Deadlines break ties within an action class. fn ilm_action_priority_rank(action: &IlmAction) -> u8 { match action { IlmAction::DeleteAllVersionsAction @@ -4159,6 +4183,392 @@ mod tests { assert_eq!(event.action, IlmAction::NoneAction); } + mod adversarial_regressions { + use super::*; + use s3s::dto::NoncurrentVersionExpiration; + + fn run(test: impl std::future::Future) { + with_default_ilm_process_time(|| { + tokio::runtime::Builder::new_current_thread() + .build() + .expect("lifecycle regression runtime should build") + .block_on(test); + }); + } + + fn noncurrent_object() -> ObjectOpts { + ObjectOpts { + name: "logs/object".to_string(), + mod_time: Some(datetime!(2020-01-01 00:00:00 UTC)), + successor_mod_time: Some(datetime!(2020-01-02 00:00:00 UTC)), + version_id: Some(Uuid::from_u128(1)), + size: 1024 * 1024, + ..Default::default() + } + } + + #[test] + #[serial] + fn noncurrent_transition_retains_the_requested_newer_versions() { + run(async { + let mut rule = enabled_rule(None, None, Some("retain-two-hot-versions")); + rule.filter = Some(LifecycleRuleFilter::default()); + rule.noncurrent_version_transitions = Some(vec![NoncurrentVersionTransition { + noncurrent_days: Some(1), + newer_noncurrent_versions: Some(2), + storage_class: Some(TransitionStorageClass::from_static("WARM")), + }]); + let lc = Arc::new(BucketLifecycleConfiguration { + rules: vec![rule], + expiry_updated_at: None, + }); + lc.validate(&ObjectLockConfiguration::default()) + .await + .expect("valid noncurrent transition policy"); + let objects = (0..4) + .map(|index| ObjectOpts { + mod_time: Some(datetime!(2020-01-05 00:00:00 UTC) - Duration::days(index)), + successor_mod_time: (index > 0).then_some(datetime!(2020-01-06 00:00:00 UTC) - Duration::days(index)), + version_id: Some(Uuid::from_u128(u128::try_from(index + 1).expect("small version index"))), + is_latest: index == 0, + num_versions: 4, + ..noncurrent_object() + }) + .collect::>(); + let actions = crate::Evaluator::new(lc) + .eval(&objects) + .await + .expect("complete version chain should evaluate") + .into_iter() + .map(|event| event.action) + .collect::>(); + assert_eq!( + actions, + [ + IlmAction::NoneAction, + IlmAction::NoneAction, + IlmAction::NoneAction, + IlmAction::TransitionVersionAction + ], + "the two newest noncurrent versions must remain in their current storage class" + ); + }); + } + + #[test] + #[serial] + fn noncurrent_transition_checks_count_age_and_single_object_context() { + run(async { + let mut rule = enabled_rule(None, None, Some("retain-two")); + rule.filter = Some(LifecycleRuleFilter::default()); + rule.noncurrent_version_transitions = Some(vec![NoncurrentVersionTransition { + noncurrent_days: Some(3), + newer_noncurrent_versions: Some(2), + storage_class: Some(TransitionStorageClass::from_static("WARM")), + }]); + let mut lc = BucketLifecycleConfiguration { + rules: vec![rule], + expiry_updated_at: None, + }; + lc.validate(&ObjectLockConfiguration::default()) + .await + .expect("valid counted transition"); + let object = noncurrent_object(); + let now = datetime!(2020-01-10 00:00:00 UTC); + for (newer, expected) in [ + (0, IlmAction::NoneAction), + (1, IlmAction::NoneAction), + (2, IlmAction::TransitionVersionAction), + (3, IlmAction::TransitionVersionAction), + ] { + assert_eq!(lc.eval_inner(&object, now, newer).await.action, expected, "newer count: {newer}"); + } + assert_eq!( + lc.eval_inner(&object, datetime!(2020-01-04 00:00:00 UTC), 2).await.action, + IlmAction::NoneAction, + "the retention count does not replace the age condition" + ); + assert_eq!( + lc.eval(&object).await.action, + IlmAction::NoneAction, + "a single-object lookup must not assume a complete version history" + ); + for retain in [None, Some(0), Some(-1), Some(i32::MAX)] { + lc.rules[0] + .noncurrent_version_transitions + .as_mut() + .expect("transition exists")[0] + .newer_noncurrent_versions = retain; + let expected = if matches!(retain, None | Some(0)) { + IlmAction::TransitionVersionAction + } else { + IlmAction::NoneAction + }; + assert_eq!(lc.eval_inner(&object, now, 2).await.action, expected, "retention: {retain:?}"); + } + }); + } + + #[test] + #[serial] + fn noncurrent_expiration_and_transition_have_independent_retention_counts() { + run(async { + let mut rule = enabled_rule(None, None, Some("independent-counts")); + rule.filter = Some(LifecycleRuleFilter::default()); + rule.noncurrent_version_expiration = Some(NoncurrentVersionExpiration { + noncurrent_days: Some(90), + newer_noncurrent_versions: Some(4), + }); + rule.noncurrent_version_transitions = Some(vec![NoncurrentVersionTransition { + noncurrent_days: Some(30), + newer_noncurrent_versions: Some(2), + storage_class: Some(TransitionStorageClass::from_static("WARM")), + }]); + let lc = BucketLifecycleConfiguration { + rules: vec![rule], + expiry_updated_at: None, + }; + lc.validate(&ObjectLockConfiguration::default()) + .await + .expect("valid independent retention limits"); + let object = noncurrent_object(); + let now = datetime!(2020-05-01 00:00:00 UTC); + for (newer, expected) in [ + (1, IlmAction::NoneAction), + (2, IlmAction::TransitionVersionAction), + (3, IlmAction::TransitionVersionAction), + (4, IlmAction::DeleteVersionAction), + ] { + assert_eq!(lc.eval_inner(&object, now, newer).await.action, expected, "newer count: {newer}"); + } + }); + } + + #[test] + #[serial] + fn expiration_retention_does_not_skip_an_independent_transition() { + run(async { + let mut rule = enabled_rule(None, None, Some("transition-then-expire")); + rule.filter = Some(LifecycleRuleFilter::default()); + rule.noncurrent_version_transitions = Some(vec![NoncurrentVersionTransition { + noncurrent_days: Some(1), + newer_noncurrent_versions: None, + storage_class: Some(TransitionStorageClass::from_static("WARM")), + }]); + let mut lc = BucketLifecycleConfiguration { + rules: vec![rule], + expiry_updated_at: None, + }; + let object = noncurrent_object(); + let now = datetime!(2020-01-10 00:00:00 UTC); + let transition_only = lc.eval_inner(&object, now, 0).await; + assert_eq!(transition_only.action, IlmAction::TransitionVersionAction); + + lc.rules[0].noncurrent_version_expiration = Some(NoncurrentVersionExpiration { + noncurrent_days: Some(90), + newer_noncurrent_versions: Some(2), + }); + lc.validate(&ObjectLockConfiguration::default()) + .await + .expect("valid combined policy"); + let combined = lc.eval_inner(&object, now, 0).await; + assert_eq!(combined.action, transition_only.action, "retention limits expiration, not transition"); + assert_eq!(combined.storage_class, transition_only.storage_class); + }); + } + + #[test] + #[serial] + fn current_transition_rejects_multiple_stages_in_any_order() { + run(async { + let mut rule = enabled_rule(None, None, Some("two-current-transitions")); + rule.transitions = Some(vec![ + Transition { + date: Some(datetime!(2020-03-01 00:00:00 UTC).into()), + days: None, + storage_class: Some(TransitionStorageClass::from_static("COLD")), + }, + Transition { + date: Some(datetime!(2020-01-03 00:00:00 UTC).into()), + days: None, + storage_class: Some(TransitionStorageClass::from_static("WARM")), + }, + ]); + let mut lc = BucketLifecycleConfiguration { + rules: vec![rule], + expiry_updated_at: None, + }; + let object = ObjectOpts { + is_latest: true, + ..noncurrent_object() + }; + let now = datetime!(2020-01-10 00:00:00 UTC); + for status in [ExpirationStatus::ENABLED, ExpirationStatus::DISABLED] { + lc.rules[0].status = ExpirationStatus::from_static(status); + for _ in 0..2 { + let err = lc + .validate(&ObjectLockConfiguration::default()) + .await + .expect_err("multiple transition stages must be rejected"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!(err.to_string(), ERR_LIFECYCLE_MULTIPLE_TRANSITIONS); + assert_eq!( + lc.eval_inner(&object, now, 0).await.action, + IlmAction::NoneAction, + "legacy multi-stage configurations must not silently execute their first stage" + ); + lc.rules[0] + .transitions + .as_mut() + .expect("transition array is present") + .reverse(); + } + } + lc.rules[0] + .transitions + .as_mut() + .expect("transition array is present") + .remove(0); + lc.rules[0].status = ExpirationStatus::from_static(ExpirationStatus::ENABLED); + lc.validate(&ObjectLockConfiguration::default()) + .await + .expect("one stage is supported"); + let event = lc.eval_inner(&object, now, 0).await; + assert_eq!(event.action, IlmAction::TransitionAction); + assert_eq!(event.storage_class, "WARM"); + }); + } + + #[test] + #[serial] + fn noncurrent_transition_rejects_multiple_stages_in_any_order() { + run(async { + let mut rule = enabled_rule(None, None, Some("two-noncurrent-transitions")); + rule.noncurrent_version_transitions = Some(vec![ + NoncurrentVersionTransition { + noncurrent_days: Some(30), + newer_noncurrent_versions: None, + storage_class: Some(TransitionStorageClass::from_static("COLD")), + }, + NoncurrentVersionTransition { + noncurrent_days: Some(1), + newer_noncurrent_versions: None, + storage_class: Some(TransitionStorageClass::from_static("WARM")), + }, + ]); + let mut lc = BucketLifecycleConfiguration { + rules: vec![rule], + expiry_updated_at: None, + }; + let object = noncurrent_object(); + let now = datetime!(2020-01-10 00:00:00 UTC); + for status in [ExpirationStatus::ENABLED, ExpirationStatus::DISABLED] { + lc.rules[0].status = ExpirationStatus::from_static(status); + for _ in 0..2 { + let err = lc + .validate(&ObjectLockConfiguration::default()) + .await + .expect_err("multiple noncurrent transition stages must be rejected"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!(err.to_string(), ERR_LIFECYCLE_MULTIPLE_NONCURRENT_TRANSITIONS); + assert_eq!( + lc.eval_inner(&object, now, 0).await.action, + IlmAction::NoneAction, + "legacy multi-stage configurations must not silently execute their first stage" + ); + lc.rules[0] + .noncurrent_version_transitions + .as_mut() + .expect("transition array is present") + .reverse(); + } + } + lc.rules[0] + .noncurrent_version_transitions + .as_mut() + .expect("transition array is present") + .remove(0); + lc.rules[0].status = ExpirationStatus::from_static(ExpirationStatus::ENABLED); + lc.validate(&ObjectLockConfiguration::default()) + .await + .expect("one stage is supported"); + let event = lc.eval_inner(&object, now, 0).await; + assert_eq!(event.action, IlmAction::TransitionVersionAction); + assert_eq!(event.storage_class, "WARM"); + }); + } + + #[test] + #[serial] + fn expiration_rejects_simultaneous_days_and_date() { + run(async { + let mut lc = BucketLifecycleConfiguration { + rules: vec![enabled_rule( + Some(LifecycleExpiration { + days: Some(1), + ..Default::default() + }), + None, + Some("ambiguous-expiry"), + )], + expiry_updated_at: None, + }; + lc.validate(&ObjectLockConfiguration::default()) + .await + .expect("a single Days expiration is valid"); + lc.rules[0].expiration.as_mut().expect("expiration is present").date = + Some(datetime!(2099-01-01 00:00:00 UTC).into()); + let err = lc + .validate(&ObjectLockConfiguration::default()) + .await + .expect_err("Days and Date are mutually exclusive; accepting both silently overrides Days"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!(err.to_string(), ERR_LIFECYCLE_EXPIRATION_DAYS_DATE_CONFLICT); + }); + } + + #[test] + #[serial] + fn overdue_transition_does_not_starve_permanent_expiration() { + run(async { + let mut rule = enabled_rule( + Some(LifecycleExpiration { + days: Some(90), + ..Default::default() + }), + None, + Some("archive-then-delete"), + ); + rule.transitions = Some(vec![Transition { + days: Some(30), + date: None, + storage_class: Some(TransitionStorageClass::from_static("WARM")), + }]); + let lc = BucketLifecycleConfiguration { + rules: vec![rule], + expiry_updated_at: None, + }; + lc.validate(&ObjectLockConfiguration::default()) + .await + .expect("valid transition and expiration policy"); + let object = ObjectOpts { + is_latest: true, + version_id: None, + transition_status: TRANSITION_PENDING.to_string(), + ..noncurrent_object() + }; + let before_expiration = lc.eval_inner(&object, datetime!(2020-02-15 00:00:00 UTC), 0).await; + assert_eq!(before_expiration.action, IlmAction::TransitionAction); + let overdue = lc.eval_inner(&object, datetime!(2020-05-01 00:00:00 UTC), 0).await; + assert_eq!( + overdue.action, + IlmAction::DeleteAction, + "an unavailable tier must not prevent permanent expiration indefinitely" + ); + }); + } + } + /// Property-based tests for the rule evaluator (backlog#1148 ilm-14, /// follow-up to backlog#1030 / rustfs#4455). /// @@ -4169,7 +4579,7 @@ mod tests { /// /// * `eval_inner` never panics and is deterministic for a fixed input; /// * the winning event matches an independently recomputed candidate set: - /// earliest `due` wins, ties break toward delete-class actions (the + /// eligible expiration wins over transition, then earliest `due` wins (the /// `min_by_key` selection that replaced the rustfs#4455 comparator); /// * `expected_expiry_time` is monotonically non-decreasing in `days` and /// always lands on the processing boundary, both at production defaults @@ -4458,8 +4868,8 @@ mod tests { /// consider for a live current version under `selection`-shaped rules /// (expiration and first-transition only, no filters): expiration /// fires when `now >= due`, transition when `now > due` and the object - /// has not already transitioned. Selection semantics under test: - /// earliest due wins, ties prefer delete-class. + /// has not already transitioned. Eligible expiration wins over transition; + /// the earliest deadline wins within the selected action class. fn oracle_candidates(lc: &BucketLifecycleConfiguration, obj: &ObjectOpts, now: OffsetDateTime) -> Vec { let mod_time = obj.mod_time.expect("selection strategy always sets mod_time"); let mut candidates = Vec::new(); @@ -4548,8 +4958,8 @@ mod tests { /// Differential test of winner selection (the rustfs#4455 fix): /// for a live current version under randomized expiration and /// transition rules, `eval_inner`'s winner must carry the - /// minimum `(due, rank)` of the independently recomputed - /// candidate set — earliest due wins, ties prefer delete-class — + /// earliest expiration from the independently recomputed candidate + /// set, or the earliest transition when no expiration is eligible, /// and must be `NoneAction` exactly when that set is empty. #[test] #[serial] @@ -4578,7 +4988,13 @@ mod tests { // Oracle and evaluator must observe the same (pinned) time env. let (event, expected) = with_production_time_env(|| { - let expected = oracle_candidates(&lc, &obj, now).into_iter().min(); + let candidates = oracle_candidates(&lc, &obj, now); + let expected = candidates + .iter() + .filter(|(_, rank)| *rank == 0) + .min() + .copied() + .or_else(|| candidates.into_iter().min()); let rt = tokio::runtime::Builder::new_current_thread() .enable_all() .build() diff --git a/crates/lifecycle/src/evaluator.rs b/crates/lifecycle/src/evaluator.rs index 80bd61de6..2da4ae76c 100644 --- a/crates/lifecycle/src/evaluator.rs +++ b/crates/lifecycle/src/evaluator.rs @@ -116,13 +116,10 @@ impl Evaluator { break 'top_loop; } } - IlmAction::DeleteAction - | IlmAction::DeleteRestoredAction - | IlmAction::DeleteVersionAction - | IlmAction::DeleteRestoredVersionAction - if self.is_object_locked(obj) => - { - event = Event::default(); + // Restore expiry removes only the temporary local copy; the + // retained logical version and its remote data remain intact. + IlmAction::DeleteAction | IlmAction::DeleteVersionAction if self.is_object_locked(obj) => { + event = obj.restored_copy_expiry(now).unwrap_or_default(); } _ => {} } @@ -206,6 +203,95 @@ mod tests { use super::*; use rustfs_replication::{ReplicationStatusType, VersionPurgeStatusType}; + + #[tokio::test] + async fn adversarial_restore_expiry_survives_legal_hold() { + let mut policy = (*latest_expiration_lifecycle()).clone(); + policy.rules[0].status = ExpirationStatus::from_static(ExpirationStatus::DISABLED); + let policy = Arc::new(policy); + policy + .validate(&lock_enabled_without_default_retention()) + .await + .expect("valid disabled lifecycle rule"); + let mut objects = [true, false].map(|is_latest| ObjectOpts { + is_latest, + num_versions: 2, + mod_time: Some( + OffsetDateTime::from_unix_timestamp(if is_latest { 1_200_000 } else { 1_000_000 }) + .expect("fixed version timestamp"), + ), + successor_mod_time: (!is_latest) + .then(|| OffsetDateTime::from_unix_timestamp(1_200_000).expect("fixed successor timestamp")), + transition_status: crate::TRANSITION_COMPLETE.to_string(), + restore_expires: Some(OffsetDateTime::from_unix_timestamp(2_000_000).expect("fixed expired restore timestamp")), + ..current_object_opts(ReplicationStatusType::Completed) + }); + let evaluator = Evaluator::new(policy).with_lock_retention(Some(lock_enabled_without_default_retention())); + let expected = [IlmAction::DeleteRestoredAction, IlmAction::DeleteRestoredVersionAction]; + let unlocked = evaluator + .eval(&objects) + .await + .expect("unlocked restored versions should evaluate"); + assert_eq!(unlocked.iter().map(|event| event.action).collect::>(), expected); + + for object in &mut objects { + object + .user_defined + .insert(X_AMZ_OBJECT_LOCK_LEGAL_HOLD.as_str().to_string(), "ON".to_string()); + } + let locked = evaluator + .eval(&objects) + .await + .expect("locked restored versions should evaluate"); + assert_eq!( + locked.iter().map(|event| event.action).collect::>(), + expected, + "expiring a restored local copy preserves the retained logical version and remote object" + ); + + let mut expiring_policy = (*latest_expiration_lifecycle()).clone(); + expiring_policy.rules[0].noncurrent_version_expiration = Some(NoncurrentVersionExpiration { + noncurrent_days: Some(1), + newer_noncurrent_versions: None, + }); + let expiring_evaluator = + Evaluator::new(Arc::new(expiring_policy)).with_lock_retention(Some(lock_enabled_without_default_retention())); + let locked = expiring_evaluator + .eval(&objects) + .await + .expect("locked expired versions should evaluate"); + assert_eq!( + locked.iter().map(|event| event.action).collect::>(), + expected, + "blocked logical expiration must still allow an eligible restore-copy cleanup" + ); + + for status in [ReplicationStatusType::Pending, ReplicationStatusType::Failed] { + for object in &mut objects { + object.replication_status = status.clone(); + } + for evaluator in [&evaluator, &expiring_evaluator] { + let events = evaluator.eval(&objects).await.expect("pending replication should evaluate"); + assert!(events.iter().all(|event| event.action == IlmAction::NoneAction)); + } + } + for object in &mut objects { + object.replication_status = ReplicationStatusType::Completed; + } + for transition_status in ["", crate::TRANSITION_PENDING, "unknown"] { + for object in &mut objects { + object.transition_status = transition_status.to_string(); + } + for evaluator in [&evaluator, &expiring_evaluator] { + let events = evaluator.eval(&objects).await.expect("incomplete transition should evaluate"); + assert!( + events.iter().all(|event| event.action == IlmAction::NoneAction), + "restore metadata cannot authorize cleanup without a completed transition" + ); + } + } + } + fn expired_marker_lifecycle() -> Arc { Arc::new(BucketLifecycleConfiguration { expiry_updated_at: None, diff --git a/crates/s3-client/src/api_get_object.rs b/crates/s3-client/src/api_get_object.rs index 872c1a90d..6cbe2fc02 100644 --- a/crates/s3-client/src/api_get_object.rs +++ b/crates/s3-client/src/api_get_object.rs @@ -120,18 +120,10 @@ impl TransitionClient { let h = resp.headers().clone(); - let mut body = resp.into_body(); let body_vec = if let Some(limit) = max_response_bytes { - collect_response_body(body, limit).await? + self.collect_response_body(resp.into_body(), limit).await? } else { - let mut body_vec = Vec::new(); - while let Some(frame) = body.frame().await { - let frame = frame.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; - if let Some(data) = frame.data_ref() { - body_vec.extend_from_slice(data); - } - } - body_vec + self.collect_response_body_unbounded(resp.into_body()).await? }; Ok((object_stat, h, BufReader::new(Cursor::new(body_vec)))) } @@ -143,7 +135,7 @@ mod bounded_response_tests { use crate::{ api_get_options::GetObjectOptions, credentials::{Credentials, SignatureType, Static, Value}, - transition_api::{BucketLookupType, Options, TransitionClient, collect_response_body}, + transition_api::{BucketLookupType, Options, TransitionClient, TransitionClientTimeouts, collect_response_body}, }; use http_body_util::Full; use hyper::body::Bytes; @@ -175,7 +167,31 @@ mod bounded_response_tests { assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); } - async fn bounded_get_fixture(body: &'static [u8]) -> Option<(TransitionClient, tokio::task::JoinHandle)> { + fn test_options() -> Options { + Options { + creds: Credentials::new(Static(Value { + access_key_id: "access-key".to_string(), + secret_access_key: "secret-key".to_string(), + signer_type: SignatureType::SignatureV4, + ..Default::default() + })), + region: "us-east-1".to_string(), + bucket_lookup: BucketLookupType::BucketLookupPath, + max_retries: 1, + ..Default::default() + } + } + + async fn client_for_endpoint(endpoint: &str, timeouts: TransitionClientTimeouts) -> TransitionClient { + TransitionClient::new_with_timeouts(endpoint, test_options(), "", timeouts) + .await + .expect("fixture client should build") + } + + async fn bounded_get_fixture_with_timeouts( + body: &'static [u8], + timeouts: TransitionClientTimeouts, + ) -> Option<(TransitionClient, tokio::task::JoinHandle)> { let listener = match TcpListener::bind("127.0.0.1:0").await { Ok(listener) => listener, Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return None, @@ -209,27 +225,14 @@ mod bounded_response_tests { stream.write_all(body).await.expect("fixture should write response body"); request }); - let client = TransitionClient::new( - &endpoint, - Options { - creds: Credentials::new(Static(Value { - access_key_id: "access-key".to_string(), - secret_access_key: "secret-key".to_string(), - signer_type: SignatureType::SignatureV4, - ..Default::default() - })), - region: "us-east-1".to_string(), - bucket_lookup: BucketLookupType::BucketLookupPath, - max_retries: 1, - ..Default::default() - }, - "", - ) - .await - .expect("fixture client should build"); + let client = client_for_endpoint(&endpoint, timeouts).await; Some((client, request)) } + async fn bounded_get_fixture(body: &'static [u8]) -> Option<(TransitionClient, tokio::task::JoinHandle)> { + bounded_get_fixture_with_timeouts(body, TransitionClientTimeouts::default()).await + } + #[tokio::test] async fn real_transport_accepts_the_exact_closed_range_length() { let Some((client, request)) = bounded_get_fixture(b"RustFS!").await else { @@ -292,24 +295,7 @@ mod bounded_response_tests { .local_addr() .expect("listener local address should be available") .to_string(); - let client = TransitionClient::new( - &endpoint, - Options { - creds: Credentials::new(Static(Value { - access_key_id: "access-key".to_string(), - secret_access_key: "secret-key".to_string(), - signer_type: SignatureType::SignatureV4, - ..Default::default() - })), - region: "us-east-1".to_string(), - bucket_lookup: BucketLookupType::BucketLookupPath, - max_retries: 1, - ..Default::default() - }, - "", - ) - .await - .expect("fixture client should build"); + let client = client_for_endpoint(&endpoint, TransitionClientTimeouts::default()).await; let mut opts = GetObjectOptions::default(); opts.headers .insert("range".to_string(), "bytes=0-18446744073709551615".to_string()); @@ -326,6 +312,176 @@ mod bounded_response_tests { .is_err() ); } + + #[tokio::test] + async fn connection_refused_returns_without_waiting_for_the_request_timeout() { + let listener = match TcpListener::bind("127.0.0.1:0").await { + Ok(listener) => listener, + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return, + Err(err) => panic!("test listener should bind: {err}"), + }; + let endpoint = listener + .local_addr() + .expect("listener local address should be available") + .to_string(); + drop(listener); + + let client = client_for_endpoint( + &endpoint, + TransitionClientTimeouts::new(Duration::from_secs(1), Duration::from_secs(5), Duration::from_secs(1)), + ) + .await; + let mut opts = GetObjectOptions::default(); + opts.set_range(0, 6).expect("the probe range should be valid"); + + let result = tokio::time::timeout(Duration::from_secs(2), client.get_object_inner("bucket", "probe", &opts)) + .await + .expect("connection refused should return before the broader request timeout"); + + assert!(result.is_err(), "connection refused must fail instead of hanging"); + } + + #[tokio::test] + async fn response_header_stall_returns_timed_out() { + let listener = match TcpListener::bind("127.0.0.1:0").await { + Ok(listener) => listener, + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return, + Err(err) => panic!("test listener should bind: {err}"), + }; + let endpoint = listener + .local_addr() + .expect("listener local address should be available") + .to_string(); + let fixture = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("fixture should accept one GET"); + let mut request = Vec::new(); + let mut buffer = [0; 1024]; + loop { + let read = stream.read(&mut buffer).await.expect("fixture should read request headers"); + assert_ne!(read, 0, "connection closed before request headers were received"); + request.extend_from_slice(&buffer[..read]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + break; + } + } + tokio::time::sleep(Duration::from_millis(200)).await; + }); + let client = client_for_endpoint( + &endpoint, + TransitionClientTimeouts::new(Duration::from_secs(1), Duration::from_millis(50), Duration::from_secs(1)), + ) + .await; + let mut opts = GetObjectOptions::default(); + opts.set_range(0, 6).expect("the probe range should be valid"); + + let err = client + .get_object_inner("bucket", "probe", &opts) + .await + .expect_err("response header stalls must be bounded"); + + assert_eq!(err.kind(), std::io::ErrorKind::TimedOut); + fixture.await.expect("fixture should join"); + } + + #[tokio::test] + async fn response_body_idle_stall_returns_timed_out() { + let listener = match TcpListener::bind("127.0.0.1:0").await { + Ok(listener) => listener, + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return, + Err(err) => panic!("test listener should bind: {err}"), + }; + let endpoint = listener + .local_addr() + .expect("listener local address should be available") + .to_string(); + let fixture = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("fixture should accept one GET"); + let mut request = Vec::new(); + let mut buffer = [0; 1024]; + loop { + let read = stream.read(&mut buffer).await.expect("fixture should read request headers"); + assert_ne!(read, 0, "connection closed before request headers were received"); + request.extend_from_slice(&buffer[..read]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + break; + } + } + stream + .write_all(b"HTTP/1.1 206 Partial Content\r\nContent-Length: 7\r\nConnection: close\r\n\r\nRu") + .await + .expect("fixture should write the first body chunk"); + tokio::time::sleep(Duration::from_millis(200)).await; + }); + let client = client_for_endpoint( + &endpoint, + TransitionClientTimeouts::new(Duration::from_secs(1), Duration::from_secs(1), Duration::from_millis(50)), + ) + .await; + let mut opts = GetObjectOptions::default(); + opts.set_range(0, 6).expect("the probe range should be valid"); + + let err = client + .get_object_inner("bucket", "probe", &opts) + .await + .expect_err("body stalls after partial progress must be bounded"); + + assert_eq!(err.kind(), std::io::ErrorKind::TimedOut); + fixture.await.expect("fixture should join"); + } + + #[tokio::test] + async fn response_body_idle_timer_resets_on_progress() { + let listener = match TcpListener::bind("127.0.0.1:0").await { + Ok(listener) => listener, + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return, + Err(err) => panic!("test listener should bind: {err}"), + }; + let endpoint = listener + .local_addr() + .expect("listener local address should be available") + .to_string(); + let fixture = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("fixture should accept one GET"); + let mut request = Vec::new(); + let mut buffer = [0; 1024]; + loop { + let read = stream.read(&mut buffer).await.expect("fixture should read request headers"); + assert_ne!(read, 0, "connection closed before request headers were received"); + request.extend_from_slice(&buffer[..read]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + break; + } + } + stream + .write_all(b"HTTP/1.1 206 Partial Content\r\nContent-Length: 7\r\nConnection: close\r\n\r\n") + .await + .expect("fixture should write response headers"); + for byte in b"RustFS!" { + stream.write_all(&[*byte]).await.expect("fixture should write body progress"); + tokio::time::sleep(Duration::from_millis(20)).await; + } + }); + let client = client_for_endpoint( + &endpoint, + TransitionClientTimeouts::new(Duration::from_millis(10), Duration::from_secs(1), Duration::from_millis(100)), + ) + .await; + let mut opts = GetObjectOptions::default(); + opts.set_range(0, 6).expect("the probe range should be valid"); + + let (_, _, mut reader) = client + .get_object_inner("bucket", "probe", &opts) + .await + .expect("continuous body progress must not be killed by the idle timer"); + let mut body = Vec::new(); + reader + .read_to_end(&mut body) + .await + .expect("bounded response should be readable"); + + assert_eq!(body, b"RustFS!"); + fixture.await.expect("fixture should join"); + } } #[derive(Default)] diff --git a/crates/s3-client/src/api_list.rs b/crates/s3-client/src/api_list.rs index dab74cf84..0bdebbf5c 100644 --- a/crates/s3-client/src/api_list.rs +++ b/crates/s3-client/src/api_list.rs @@ -27,7 +27,6 @@ use crate::{ transition_api::{ReaderImpl, RequestMetadata, TransitionClient, collect_response_body}, }; use http::{HeaderMap, StatusCode}; -use http_body_util::BodyExt; use hyper::body::Body; use hyper::body::Bytes; use rustfs_config::MAX_S3_CLIENT_RESPONSE_SIZE; @@ -124,14 +123,9 @@ impl TransitionClient { } //let mut list_bucket_result = ListBucketV2Result::default(); - let mut body_vec = Vec::new(); - let mut body = resp.into_body(); - while let Some(frame) = body.frame().await { - let frame = frame.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; - if let Some(data) = frame.data_ref() { - body_vec.extend_from_slice(data); - } - } + let body_vec = self + .collect_response_body(resp.into_body(), MAX_S3_CLIENT_RESPONSE_SIZE) + .await?; let mut list_bucket_result = match quick_xml::de::from_str::(&String::from_utf8_lossy(&body_vec)) { Ok(result) => result, Err(err) => { @@ -214,7 +208,9 @@ impl TransitionClient { let resp_status = resp.status(); let headers = resp.headers().clone(); - let body = collect_response_body(resp.into_body(), MAX_S3_CLIENT_RESPONSE_SIZE).await?; + let body = self + .collect_response_body(resp.into_body(), MAX_S3_CLIENT_RESPONSE_SIZE) + .await?; if resp_status != StatusCode::OK { return Err(std::io::Error::other(http_resp_to_error_response( resp_status, @@ -428,6 +424,30 @@ fn decode_s3_name(name: &str, encoding_type: &str) -> Result Options { + Options { + creds: Credentials::new(Static(Value { + access_key_id: "access-key".to_string(), + secret_access_key: "secret-key".to_string(), + signer_type: SignatureType::SignatureV4, + ..Default::default() + })), + region: "us-east-1".to_string(), + bucket_lookup: BucketLookupType::BucketLookupPath, + max_retries: 1, + ..Default::default() + } + } #[test] fn list_versions_xml_preserves_versions_and_delete_markers() { @@ -525,4 +545,56 @@ mod tests { assert_eq!(parsed.common_prefixes.len(), 1); assert_eq!(parsed.common_prefixes[0].prefix, "subdir/"); } + + #[tokio::test] + async fn list_objects_v2_body_stall_returns_timed_out() { + let listener = match TcpListener::bind("127.0.0.1:0").await { + Ok(listener) => listener, + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return, + Err(err) => panic!("test listener should bind: {err}"), + }; + let endpoint = listener + .local_addr() + .expect("listener local address should be available") + .to_string(); + let fixture = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("fixture should accept one list request"); + let mut request = Vec::new(); + let mut buffer = [0; 1024]; + loop { + let read = stream.read(&mut buffer).await.expect("fixture should read request headers"); + assert_ne!(read, 0, "connection closed before request headers were received"); + request.extend_from_slice(&buffer[..read]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + break; + } + } + stream + .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 512\r\nConnection: close\r\n\r\nwarm") + .await + .expect("fixture should write a partial list response"); + tokio::time::sleep(Duration::from_millis(200)).await; + }); + let client = TransitionClient::new_with_timeouts( + &endpoint, + timeout_test_options(), + "", + TransitionClientTimeouts::new(Duration::from_secs(1), Duration::from_secs(1), Duration::from_millis(50)), + ) + .await + .expect("fixture client should build"); + client + .bucket_loc_cache + .lock() + .expect("location cache should lock") + .set("bucket", "us-east-1"); + + let err = client + .list_objects_v2_query("bucket", "", "", false, false, "", "", 1, HeaderMap::new()) + .await + .expect_err("a stalled ListObjectsV2 body must be bounded"); + + assert_eq!(err.kind(), std::io::ErrorKind::TimedOut); + fixture.await.expect("fixture should join"); + } } diff --git a/crates/s3-client/src/api_put_object_multipart.rs b/crates/s3-client/src/api_put_object_multipart.rs index d7d1ba7a1..8262a6598 100644 --- a/crates/s3-client/src/api_put_object_multipart.rs +++ b/crates/s3-client/src/api_put_object_multipart.rs @@ -18,7 +18,6 @@ #![allow(clippy::all)] use http::{HeaderMap, HeaderName, StatusCode}; -use http_body_util::BodyExt; use hyper::body::Bytes; use s3s::S3ErrorCode; use std::collections::HashMap; @@ -247,14 +246,9 @@ impl TransitionClient { // Parse the CreateMultipartUpload response for the UploadId. Returning a // default (empty) result here made every multipart transition fail at the // first UploadPart with "UploadID cannot be empty" (rustfs/rustfs#4811). - let mut body_vec = Vec::new(); - let mut body = resp.into_body(); - while let Some(frame) = body.frame().await { - let frame = frame.map_err(|e| std::io::Error::other(e.to_string()))?; - if let Some(data) = frame.data_ref() { - body_vec.extend_from_slice(data); - } - } + let body_vec = self + .collect_response_body(resp.into_body(), rustfs_config::MAX_S3_CLIENT_RESPONSE_SIZE) + .await?; let initiate_multipart_upload_result = quick_xml::de::from_str::(&String::from_utf8_lossy(&body_vec)) .map_err(|e| std::io::Error::other(format!("failed to parse CreateMultipartUpload response: {e}")))?; diff --git a/crates/s3-client/src/api_remove.rs b/crates/s3-client/src/api_remove.rs index 5e063e14f..4a552cfcd 100644 --- a/crates/s3-client/src/api_remove.rs +++ b/crates/s3-client/src/api_remove.rs @@ -19,7 +19,6 @@ #![allow(clippy::all)] use http::{HeaderMap, HeaderValue, Method, StatusCode}; -use http_body_util::BodyExt; use hyper::body::Body; use hyper::body::Bytes; use rustfs_utils::HashAlgorithm; @@ -351,14 +350,9 @@ impl TransitionClient { ) .await?; - let mut body_vec = Vec::new(); - let mut body = resp.into_body(); - while let Some(frame) = body.frame().await { - let frame = frame.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; - if let Some(data) = frame.data_ref() { - body_vec.extend_from_slice(data); - } - } + let body_vec = self + .collect_response_body(resp.into_body(), rustfs_config::MAX_S3_CLIENT_RESPONSE_SIZE) + .await?; process_remove_multi_objects_response( ReaderImpl::Body(Bytes::from(body_vec)), bucket_name, diff --git a/crates/s3-client/src/api_stat.rs b/crates/s3-client/src/api_stat.rs index bac59c8ed..9386a209e 100644 --- a/crates/s3-client/src/api_stat.rs +++ b/crates/s3-client/src/api_stat.rs @@ -19,7 +19,6 @@ #![allow(clippy::all)] use http::{HeaderMap, HeaderValue, StatusCode}; -use http_body_util::BodyExt; use hyper::body::Body; use hyper::body::Bytes; use rustfs_utils::EMPTY_STRING_SHA256_HASH; @@ -119,14 +118,9 @@ impl TransitionClient { let resp_status = resp.status(); let h = resp.headers().clone(); - let mut body_vec = Vec::new(); - let mut body = resp.into_body(); - while let Some(frame) = body.frame().await { - let frame = frame.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; - if let Some(data) = frame.data_ref() { - body_vec.extend_from_slice(data); - } - } + let body_vec = self + .collect_response_body(resp.into_body(), rustfs_config::MAX_S3_CLIENT_RESPONSE_SIZE) + .await?; let resperr = http_resp_to_error_response(resp_status, &h, body_vec, bucket_name, ""); warn!("bucket exists, resperr: {:?}", resperr); @@ -170,11 +164,13 @@ impl TransitionClient { let resp_status = resp.status(); let h = resp.headers().clone(); - let body_vec = collect_response_body(resp.into_body(), rustfs_config::MAX_S3_CLIENT_RESPONSE_SIZE).await?; + let body_vec = self + .collect_response_body(resp.into_body(), rustfs_config::MAX_S3_CLIENT_RESPONSE_SIZE) + .await?; parse_bucket_versioning_response(resp_status, &h, body_vec, bucket_name) } - Err(err) => Err(std::io::Error::other(err)), + Err(err) => Err(err), } } @@ -274,8 +270,14 @@ impl TransitionClient { #[cfg(test)] mod tests { use super::parse_bucket_versioning_response; + use crate::{ + credentials::{Credentials, SignatureType, Static, Value}, + transition_api::{BucketLookupType, Options, TransitionClient, TransitionClientTimeouts}, + }; use http::{HeaderMap, StatusCode}; use s3s::dto::BucketVersioningStatus; + use std::time::Duration; + use tokio::{io::AsyncReadExt, net::TcpListener}; #[test] fn parses_bucket_versioning_statuses_mfa_delete_and_unversioned_state() { @@ -338,4 +340,63 @@ mod tests { assert_eq!(strict_err.kind(), std::io::ErrorKind::InvalidData); } } + + #[tokio::test] + async fn get_bucket_versioning_preserves_request_timeout_kind() { + let listener = match TcpListener::bind("127.0.0.1:0").await { + Ok(listener) => listener, + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return, + Err(err) => panic!("test listener should bind: {err}"), + }; + let endpoint = listener + .local_addr() + .expect("listener local address should be available") + .to_string(); + let fixture = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("fixture should accept one versioning request"); + let mut request = Vec::new(); + let mut buffer = [0; 1024]; + loop { + let read = stream.read(&mut buffer).await.expect("fixture should read request headers"); + assert_ne!(read, 0, "connection closed before request headers were received"); + request.extend_from_slice(&buffer[..read]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + break; + } + } + tokio::time::sleep(Duration::from_millis(200)).await; + }); + let client = TransitionClient::new_with_timeouts( + &endpoint, + Options { + creds: Credentials::new(Static(Value { + access_key_id: "access-key".to_string(), + secret_access_key: "secret-key".to_string(), + signer_type: SignatureType::SignatureV4, + ..Default::default() + })), + region: "us-east-1".to_string(), + bucket_lookup: BucketLookupType::BucketLookupPath, + max_retries: 1, + ..Default::default() + }, + "", + TransitionClientTimeouts::new(Duration::from_secs(1), Duration::from_millis(50), Duration::from_secs(1)), + ) + .await + .expect("fixture client should build"); + client + .bucket_loc_cache + .lock() + .expect("location cache should lock") + .set("bucket", "us-east-1"); + + let err = client + .get_bucket_versioning("bucket") + .await + .expect_err("a stalled versioning request must time out"); + + assert_eq!(err.kind(), std::io::ErrorKind::TimedOut); + fixture.await.expect("fixture should join"); + } } diff --git a/crates/s3-client/src/bucket_cache.rs b/crates/s3-client/src/bucket_cache.rs index c891d65a0..c3429da19 100644 --- a/crates/s3-client/src/bucket_cache.rs +++ b/crates/s3-client/src/bucket_cache.rs @@ -26,7 +26,6 @@ use crate::{ transition_api::{CreateBucketConfiguration, LocationConstraint, TransitionClient}, }; use http::Request; -use http_body_util::BodyExt; use hyper::StatusCode; use hyper::body::Body; use hyper::body::Bytes; @@ -86,7 +85,7 @@ impl TransitionClient { let req = self.get_bucket_location_request(bucket_name)?; let mut resp = self.doit(req).await?; - location = process_bucket_location_response(resp, bucket_name, &self.tier_type).await?; + location = process_bucket_location_response(self, resp, bucket_name, &self.tier_type).await?; { if let Ok(mut bucket_loc_cache) = self.bucket_loc_cache.lock() { bucket_loc_cache.set(bucket_name, &location); @@ -198,6 +197,7 @@ impl TransitionClient { } async fn process_bucket_location_response( + client: &TransitionClient, mut resp: http::Response, bucket_name: &str, tier_type: &str, @@ -237,14 +237,9 @@ async fn process_bucket_location_response( } //} - let mut body_vec = Vec::new(); - let mut body = resp.into_body(); - while let Some(frame) = body.frame().await { - let frame = frame.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; - if let Some(data) = frame.data_ref() { - body_vec.extend_from_slice(data); - } - } + let body_vec = client + .collect_response_body(resp.into_body(), MAX_S3_CLIENT_RESPONSE_SIZE) + .await?; let mut location = "".to_string(); if tier_type == "huaweicloud" { if let Ok(body_str) = String::from_utf8(body_vec) { diff --git a/crates/s3-client/src/transition_api.rs b/crates/s3-client/src/transition_api.rs index 6f45c76e2..bd3c62fb5 100644 --- a/crates/s3-client/src/transition_api.rs +++ b/crates/s3-client/src/transition_api.rs @@ -41,7 +41,7 @@ use http::{ request::{Builder, Request}, }; use http_body::Body; -use http_body_util::{BodyExt, LengthLimitError, Limited}; +use http_body_util::BodyExt; use hyper::body::Bytes; use hyper::body::Incoming; use hyper_rustls::{ConfigBuilderExt, HttpsConnector}; @@ -67,10 +67,12 @@ use s3s::dto::Owner; use s3s::dto::ReplicationStatus; use serde::{Deserialize, Serialize}; use sha2::Sha256; +use std::error::Error as StdError; use std::io::Cursor; use std::pin::Pin; use std::sync::atomic::{AtomicI32, Ordering}; use std::task::{Context, Poll}; +use std::time::Duration as StdDuration; use std::{ collections::HashMap, sync::{Arc, Mutex}, @@ -79,28 +81,108 @@ use time::Duration; use time::OffsetDateTime; use tokio::io::BufReader; use tokio::io::{AsyncRead, AsyncReadExt}; -use tracing::{debug, error, warn}; +use tracing::{debug, error, trace, warn}; use url::{Url, form_urlencoded}; use uuid::Uuid; const C_USER_AGENT: &str = "RustFS (linux; x86)"; pub const MAX_S3_ERROR_RESPONSE_SIZE: usize = 64 * 1024; +const EVENT_TIER_REMOTE_TRANSPORT: &str = "tier_remote_transport"; +const LOG_COMPONENT_S3_CLIENT: &str = "s3_client"; +const LOG_SUBSYSTEM_TIER: &str = "tier"; const SUCCESS_STATUS: [StatusCode; 3] = [StatusCode::OK, StatusCode::NO_CONTENT, StatusCode::PARTIAL_CONTENT]; +fn response_body_exceeds_limit_error() -> std::io::Error { + std::io::Error::new(std::io::ErrorKind::InvalidData, "remote tier response body exceeds limit") +} + +fn remote_tier_timeout_error(message: &'static str) -> std::io::Error { + std::io::Error::new(std::io::ErrorKind::TimedOut, message) +} + +fn source_chain_has_io_kind(error: &(dyn StdError + 'static), kind: std::io::ErrorKind) -> bool { + let mut current = Some(error); + while let Some(error) = current { + if error + .downcast_ref::() + .is_some_and(|io_error| io_error.kind() == kind) + { + return true; + } + current = error.source(); + } + false +} + +fn transition_transport_error(err: hyper_util::client::legacy::Error) -> std::io::Error { + if source_chain_has_io_kind(&err, std::io::ErrorKind::TimedOut) { + return remote_tier_timeout_error("remote tier connection timed out"); + } + std::io::Error::other(err) +} + +async fn next_response_body_data( + mut body: Pin<&mut B>, + idle_timeout: Option, +) -> Result, std::io::Error> +where + B: Body, + B::Error: Into>, +{ + let next_nonempty_data = async { + loop { + let Some(frame) = std::future::poll_fn(|cx| body.as_mut().poll_frame(cx)).await else { + return Ok(None); + }; + let frame = frame.map_err(std::io::Error::other)?; + let Ok(data) = frame.into_data() else { + continue; + }; + if !data.is_empty() { + return Ok(Some(data)); + } + } + }; + + if let Some(idle_timeout) = idle_timeout { + tokio::time::timeout(idle_timeout, next_nonempty_data) + .await + .map_err(|_| remote_tier_timeout_error("remote tier response body stalled"))? + } else { + next_nonempty_data.await + } +} + +async fn collect_response_body_inner( + body: B, + limit: Option, + idle_timeout: Option, +) -> Result, std::io::Error> +where + B: Body, + B::Error: Into>, +{ + let mut body_vec = Vec::new(); + let mut body = std::pin::pin!(body); + while let Some(data) = next_response_body_data(body.as_mut(), idle_timeout).await? { + let Some(new_len) = body_vec.len().checked_add(data.len()) else { + return Err(response_body_exceeds_limit_error()); + }; + if limit.is_some_and(|limit| new_len > limit) { + return Err(response_body_exceeds_limit_error()); + } + body_vec.extend_from_slice(&data); + } + Ok(body_vec) +} + pub async fn collect_response_body(body: B, limit: usize) -> Result, std::io::Error> where B: Body, - B::Error: Into>, + B::Error: Into>, { - let body = Limited::new(body, limit).collect().await.map_err(|err| { - if err.is::() { - std::io::Error::new(std::io::ErrorKind::InvalidData, "remote tier response body exceeds limit") - } else { - std::io::Error::other(err) - } - })?; - Ok(body.to_bytes().to_vec()) + collect_response_body_inner(body, Some(limit), None).await } const C_UNKNOWN: i32 = -1; @@ -196,6 +278,62 @@ pub struct TransitionClient { pub trailing_header_support: bool, pub max_retries: i64, pub tier_type: String, + pub timeouts: TransitionClientTimeouts, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct TransitionClientTimeouts { + pub connect_timeout: StdDuration, + pub request_timeout: StdDuration, + pub response_body_idle_timeout: StdDuration, +} + +impl TransitionClientTimeouts { + pub const fn new( + connect_timeout: StdDuration, + request_timeout: StdDuration, + response_body_idle_timeout: StdDuration, + ) -> Self { + Self { + connect_timeout, + request_timeout, + response_body_idle_timeout, + } + } + + fn validate(self) -> Result { + if self.connect_timeout.is_zero() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "remote tier connect timeout must be greater than zero", + )); + } + if self.request_timeout.is_zero() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "remote tier request timeout must be greater than zero", + )); + } + if self.response_body_idle_timeout.is_zero() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "remote tier response body idle timeout must be greater than zero", + )); + } + Ok(self) + } +} + +impl Default for TransitionClientTimeouts { + fn default() -> Self { + Self { + connect_timeout: StdDuration::from_secs(rustfs_config::DEFAULT_TIER_REMOTE_CONNECT_TIMEOUT_SECS), + request_timeout: StdDuration::from_secs(rustfs_config::DEFAULT_TIER_REMOTE_REQUEST_TIMEOUT_SECS), + response_body_idle_timeout: StdDuration::from_secs( + rustfs_config::DEFAULT_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS, + ), + } + } } #[derive(Debug, Default)] @@ -288,12 +426,28 @@ async fn build_tls_config() -> Result { impl TransitionClient { pub async fn new(endpoint: &str, opts: Options, tier_type: &str) -> Result { - let client = Self::private_new(endpoint, opts, tier_type).await?; - - Ok(client) + Self::private_new(endpoint, opts, tier_type, TransitionClientTimeouts::default()).await } - async fn private_new(endpoint: &str, opts: Options, tier_type: &str) -> Result { + /// Builds a transition client with explicit transport timeout budgets. + /// + /// [`Self::new`] keeps the historical constructor surface and uses the + /// production defaults from [`TransitionClientTimeouts::default`]. + pub async fn new_with_timeouts( + endpoint: &str, + opts: Options, + tier_type: &str, + timeouts: TransitionClientTimeouts, + ) -> Result { + Self::private_new(endpoint, opts, tier_type, timeouts).await + } + + async fn private_new( + endpoint: &str, + opts: Options, + tier_type: &str, + timeouts: TransitionClientTimeouts, + ) -> Result { if rustls::crypto::CryptoProvider::get_default().is_none() { // No default provider is set yet; try to install aws-lc-rs. // `install_default` can only fail if another thread races us and installs a provider @@ -306,15 +460,19 @@ impl TransitionClient { } let endpoint_url = get_endpoint_url(endpoint, opts.secure)?; + let timeouts = timeouts.validate()?; let tls = build_tls_config().await?; + let mut http = HttpConnector::new(); + http.enforce_http(false); + http.set_connect_timeout(Some(timeouts.connect_timeout)); let https = hyper_rustls::HttpsConnectorBuilder::new() .with_tls_config(tls) .https_or_http() .enable_http1() .enable_http2() - .build(); + .wrap_connector(http); let http_client = Client::builder(TokioExecutor::new()).build(https); let mut client = TransitionClient { @@ -337,6 +495,7 @@ impl TransitionClient { trailing_header_support: opts.trailing_headers, max_retries: opts.max_retries, tier_type: tier_type.to_string(), + timeouts, }; { @@ -501,29 +660,43 @@ impl TransitionClient { } pub async fn doit(&self, req: Request) -> Result, std::io::Error> { - let req_method; - let req_uri; - let resp; let http_client = self.http_client.clone(); - { - req_method = req.method().clone(); - req_uri = req.uri().clone(); - - debug!("endpoint_url: {}", self.endpoint_url.as_str().to_string()); - resp = http_client.request(req); - } - let resp = resp.await; - debug!("http_client url: {} {}", req_method, req_uri); - if let Err(err) = resp { - error!("http_client call error: {:?}", err); - return Err(std::io::Error::other(err)); - } - + let req_method = req.method().clone(); + let resp = tokio::time::timeout(self.timeouts.request_timeout, http_client.request(req)).await; let resp = match resp { - Ok(r) => r, - Err(_) => return Err(std::io::Error::other("Unexpected error in response")), + Ok(Ok(resp)) => resp, + Ok(Err(err)) => { + let err = transition_transport_error(err); + error!( + event = EVENT_TIER_REMOTE_TRANSPORT, + component = LOG_COMPONENT_S3_CLIENT, + subsystem = LOG_SUBSYSTEM_TIER, + method = %req_method, + error_kind = ?err.kind(), + "remote tier request failed" + ); + return Err(err); + } + Err(_) => { + warn!( + event = EVENT_TIER_REMOTE_TRANSPORT, + component = LOG_COMPONENT_S3_CLIENT, + subsystem = LOG_SUBSYSTEM_TIER, + method = %req_method, + timeout_ms = self.timeouts.request_timeout.as_millis(), + "remote tier request timed out before response headers" + ); + return Err(remote_tier_timeout_error("remote tier request timed out before response headers")); + } }; - debug!(status = %resp.status(), "remote tier response received"); + trace!( + event = EVENT_TIER_REMOTE_TRANSPORT, + component = LOG_COMPONENT_S3_CLIENT, + subsystem = LOG_SUBSYSTEM_TIER, + method = %req_method, + status = %resp.status(), + "remote tier response received" + ); //let b = resp.body_mut().store_all_unlimited().await.unwrap().to_vec(); //debug!("http_resp_body: {}", String::from_utf8(b).unwrap()); @@ -537,7 +710,15 @@ impl TransitionClient { .and_then(|value| value.to_str().ok()) .unwrap_or_default() .to_string(); - warn!(status = %status, request_id, "remote tier request rejected"); + warn!( + event = EVENT_TIER_REMOTE_TRANSPORT, + component = LOG_COMPONENT_S3_CLIENT, + subsystem = LOG_SUBSYSTEM_TIER, + method = %req_method, + status = %status, + request_id, + "remote tier request rejected" + ); } Ok(resp) } @@ -581,7 +762,9 @@ impl TransitionClient { let resp_status = resp.status(); let h = resp.headers().clone(); - let body_vec = collect_response_body(resp.into_body(), MAX_S3_ERROR_RESPONSE_SIZE).await?; + let body_vec = self + .collect_response_body(resp.into_body(), MAX_S3_ERROR_RESPONSE_SIZE) + .await?; let parsed_error = http_resp_to_error_response(resp_status, &h, body_vec, &metadata.bucket_name, &metadata.object_name); let routing_region = parsed_error.region; @@ -635,6 +818,22 @@ impl TransitionClient { Err(std::io::Error::other("remote tier request did not produce a response")) } + pub async fn collect_response_body(&self, body: B, limit: usize) -> Result, std::io::Error> + where + B: Body, + B::Error: Into>, + { + collect_response_body_inner(body, Some(limit), Some(self.timeouts.response_body_idle_timeout)).await + } + + pub async fn collect_response_body_unbounded(&self, body: B) -> Result, std::io::Error> + where + B: Body, + B::Error: Into>, + { + collect_response_body_inner(body, None, Some(self.timeouts.response_body_idle_timeout)).await + } + async fn new_request( &self, method: &http::Method, @@ -1504,12 +1703,17 @@ pub struct CreateBucketConfiguration { mod tests { use super::{ MAX_S3_CLIENT_RESPONSE_SIZE, MAX_S3_ERROR_RESPONSE_SIZE, SignatureType, build_tls_config, collect_response_body, - signer_error_to_io_error, to_object_info_for_provider, validate_header_values, with_rustls_init_guard, + collect_response_body_inner, signer_error_to_io_error, to_object_info_for_provider, validate_header_values, + with_rustls_init_guard, }; use crate::provider_versions::{BucketVersioningState, ProviderVersionCapabilities, RemoteVersion}; - use http::{HeaderMap, HeaderValue}; - use http_body_util::Full; + use futures::stream; + use http::{HeaderMap, HeaderValue, Request}; + use http_body::Frame; + use http_body_util::{Full, StreamBody}; use hyper::body::Bytes; + use std::time::Duration as StdDuration; + use tokio::net::TcpListener; use uuid::Uuid; #[tokio::test] @@ -1540,6 +1744,77 @@ mod tests { assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); } + #[tokio::test] + async fn empty_data_frames_do_not_reset_the_body_idle_timeout() { + let frames = stream::unfold((), |_| async { + tokio::time::sleep(StdDuration::from_millis(10)).await; + Some((Ok::<_, std::io::Error>(Frame::data(Bytes::new())), ())) + }); + let body = StreamBody::new(Box::pin(frames)); + + let err = tokio::time::timeout( + StdDuration::from_millis(200), + collect_response_body_inner(body, Some(1), Some(StdDuration::from_millis(50))), + ) + .await + .expect("the collector should enforce its own body idle timeout") + .expect_err("empty frames must not count as body progress"); + + assert_eq!(err.kind(), std::io::ErrorKind::TimedOut); + } + + #[tokio::test] + async fn public_body_collector_accepts_non_unpin_bodies() { + let body = StreamBody::new(stream::once(async { Ok::<_, std::io::Error>(Frame::data(Bytes::from_static(b"ok"))) })); + + let collected = collect_response_body(body, 2) + .await + .expect("the public collector should pin non-Unpin bodies internally"); + + assert_eq!(collected, b"ok"); + } + + #[tokio::test] + async fn https_endpoints_reach_the_transport_connector() { + let listener = match TcpListener::bind("127.0.0.1:0").await { + Ok(listener) => listener, + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return, + Err(err) => panic!("test listener should bind: {err}"), + }; + let endpoint = listener + .local_addr() + .expect("listener local address should be available") + .to_string(); + let accepted = tokio::spawn(async move { + let (stream, _) = tokio::time::timeout(StdDuration::from_secs(1), listener.accept()) + .await + .expect("HTTPS connector should reach the TCP listener") + .expect("fixture should accept the HTTPS connection"); + drop(stream); + }); + let client = super::TransitionClient::new_with_timeouts( + &endpoint, + super::Options { + secure: true, + ..Default::default() + }, + "", + super::TransitionClientTimeouts::new(StdDuration::from_secs(1), StdDuration::from_secs(1), StdDuration::from_secs(1)), + ) + .await + .expect("fixture client should build"); + let request = Request::builder() + .uri(format!("https://{endpoint}/")) + .body(s3s::Body::empty()) + .expect("fixture request should build"); + + client + .doit(request) + .await + .expect_err("the fixture closes before completing the TLS handshake"); + accepted.await.expect("fixture should join"); + } + #[test] fn rustls_guard_converts_panics_to_io_errors() { let err = with_rustls_init_guard(|| -> Result<(), std::io::Error> { panic!("missing provider") }) @@ -1573,6 +1848,18 @@ mod tests { assert!(outcome.is_ok(), "provider install guard must not panic when a provider is already set"); } + #[test] + fn transition_timeouts_reject_zero_budgets() { + for timeouts in [ + super::TransitionClientTimeouts::new(StdDuration::ZERO, StdDuration::from_secs(1), StdDuration::from_secs(1)), + super::TransitionClientTimeouts::new(StdDuration::from_secs(1), StdDuration::ZERO, StdDuration::from_secs(1)), + super::TransitionClientTimeouts::new(StdDuration::from_secs(1), StdDuration::from_secs(1), StdDuration::ZERO), + ] { + let err = timeouts.validate().expect_err("zero timeout budgets must fail closed"); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + } + } + #[test] fn validate_header_values_returns_header_name_for_non_utf8_values() { let mut headers = HeaderMap::new(); diff --git a/crates/scanner/src/data_usage_define.rs b/crates/scanner/src/data_usage_define.rs index 44230f9c0..470fa1935 100644 --- a/crates/scanner/src/data_usage_define.rs +++ b/crates/scanner/src/data_usage_define.rs @@ -197,7 +197,7 @@ pub(crate) async fn read_config_revision(store: Arc, path } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct DataUsageCacheRevisions { main: DataUsageCacheRevision, backup: Option, @@ -594,6 +594,10 @@ pub struct DataUsageCacheInfo { pub lkg_leader_epoch: Option, #[serde(default)] pub lkg_scan_plan_digest: Option, + /// Activity-sensitive identity for same-cycle set snapshot reuse. The + /// structural plan remains reusable across ordinary bucket writes. + #[serde(default)] + pub scan_execution_digest: Option, } impl Serialize for DataUsageCacheInfo { @@ -614,7 +618,8 @@ impl Serialize for DataUsageCacheInfo { + usize::from(self.lkg_next_cycle.is_some()) + usize::from(self.lkg_last_update.is_some()) + usize::from(self.lkg_leader_epoch.is_some()) - + usize::from(self.lkg_scan_plan_digest.is_some()); + + usize::from(self.lkg_scan_plan_digest.is_some()) + + usize::from(self.scan_execution_digest.is_some()); let mut state = serializer.serialize_map(Some(field_count))?; state.serialize_entry("name", &self.name)?; state.serialize_entry("next_cycle", &self.next_cycle)?; @@ -665,6 +670,9 @@ impl Serialize for DataUsageCacheInfo { if let Some(scan_plan_digest) = self.lkg_scan_plan_digest { state.serialize_entry("lkg_scan_plan_digest", &scan_plan_digest)?; } + if let Some(scan_execution_digest) = self.scan_execution_digest { + state.serialize_entry("scan_execution_digest", &scan_execution_digest)?; + } state.end() } } diff --git a/crates/scanner/src/data_usage_define/tests.rs b/crates/scanner/src/data_usage_define/tests.rs index dc38b9822..7f6dfd30d 100644 --- a/crates/scanner/src/data_usage_define/tests.rs +++ b/crates/scanner/src/data_usage_define/tests.rs @@ -1092,6 +1092,7 @@ fn test_data_usage_cache_info_deserialize_defaults_scan_resume_after() { assert!(decoded.source.is_none()); assert!(!decoded.snapshot_complete); assert!(decoded.scan_plan_digest.is_none()); + assert!(decoded.scan_execution_digest.is_none()); assert_eq!(decoded.cache_key_format, 0); } @@ -1134,6 +1135,7 @@ fn test_data_usage_cache_info_unmarshal_old_msgpack_defaults_scan_resume_after() assert!(decoded.source.is_none()); assert!(!decoded.snapshot_complete); assert!(decoded.scan_plan_digest.is_none()); + assert!(decoded.scan_execution_digest.is_none()); assert_eq!(decoded.cache_key_format, 0); } @@ -1170,6 +1172,7 @@ fn test_new_data_usage_cache_msgpack_round_trips_and_supports_old_reader() { source: Some(DataUsageCacheSource::new(1, 2)), snapshot_complete: true, scan_plan_digest: Some(TEST_PLAN_DIGEST), + scan_execution_digest: Some(DataUsageScanPlanDigest([42; 32])), cache_key_format: DATA_USAGE_CACHE_KEY_FORMAT, ..Default::default() }, @@ -1189,6 +1192,7 @@ fn test_new_data_usage_cache_msgpack_round_trips_and_supports_old_reader() { assert_eq!(current.info.source, Some(DataUsageCacheSource::new(1, 2))); assert!(current.info.snapshot_complete); assert_eq!(current.info.scan_plan_digest, Some(TEST_PLAN_DIGEST)); + assert_eq!(current.info.scan_execution_digest, Some(DataUsageScanPlanDigest([42; 32]))); assert_eq!(current.info.cache_key_format, DATA_USAGE_CACHE_KEY_FORMAT); assert_eq!(current.find("bucket").map(|entry| entry.objects), Some(3)); diff --git a/crates/scanner/src/scanner.rs b/crates/scanner/src/scanner.rs index 18970ac37..e57a6cc1a 100644 --- a/crates/scanner/src/scanner.rs +++ b/crates/scanner/src/scanner.rs @@ -1623,7 +1623,7 @@ where // Refresh the storage-owned movement snapshot before reading background // heal state. A missing heal object yields an in-memory default; do not // let that default influence a cycle while publication is blocked. - if storeapi.scanner_data_usage_publication_blocked().await { + if storeapi.scanner_data_movement_pause_status().await.paused { mark_scan_cycle_idle(cycle_info, &mut cycle_metrics_guard).await; return ScannerCycleOutcome::Deferred(ScannerCycleDeferReason::DataMovement); } @@ -1826,6 +1826,19 @@ where let publication_defer_reason = publication_defer_reason .or(remote_lease_defer_reason) .or(remote_lease_fence_defer_reason); + // A PUT tail can finish between the walk and lease acquisition without + // changing the movement epoch accepted by those leases. Re-prove the + // namespace baseline only after every peer has granted publication. + let post_lease_activity_defer_reason = if publication_defer_reason.is_none() + && remote_publication_leases.is_some() + && let Ok(result) = &scan_result + && result.status == ScannerCycleStatus::Complete + { + scanner_post_lease_activity_defer_reason(result.activity_digest(), probe_scanner_activity(storeapi.as_ref(), true).await) + } else { + None + }; + let publication_defer_reason = publication_defer_reason.or(post_lease_activity_defer_reason); // Include reasons discovered while acquiring or validating remote leases. let publication_deferred = publication_defer_reason.is_some(); let budget_elapsed = cycle_budget.budget_elapsed() && !ctx.is_cancelled(); @@ -3250,6 +3263,21 @@ where } } +fn scanner_post_lease_activity_defer_reason( + expected_digest: Option<[u8; 32]>, + activity: Result, +) -> Option { + match activity { + Ok(snapshot) + if scanner_activity_allows_usage_publication(&snapshot) + && expected_digest == Some(scanner_activity_snapshot_digest(&snapshot)) => + { + None + } + Ok(_) | Err(_) => Some(ScannerCycleDeferReason::ActivityBaselineUnavailable), + } +} + #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum ScannerCyclePreCommitOutcome { RecoverCacheCycle(u64), @@ -3438,12 +3466,11 @@ use cycle_state::*; use leadership::*; use usage_store::*; -pub(crate) use activity::scanner_activity_snapshot_digest; pub use activity::scanner_topology_digest; pub(crate) use activity::{ ScannerActivitySnapshot, ScannerDirtyUsageAcknowledgement, probe_scanner_activity, scanner_activity_allows_usage_publication, - scanner_activity_dirty_usage_state_for_host, scanner_activity_publication_lease_targets, scanner_activity_structural_digest, - scanner_dirty_usage_acknowledgements, + scanner_activity_dirty_usage_state_for_host, scanner_activity_publication_lease_targets, scanner_activity_snapshot_digest, + scanner_activity_structural_digest, scanner_dirty_usage_acknowledgements, }; pub(crate) use activity::{ScannerCycleOutcome, scanner_cycle_outcome_with_pending_maintenance}; pub use backlog::{ diff --git a/crates/scanner/src/scanner/tests.rs b/crates/scanner/src/scanner/tests.rs index 0d1f64c1b..dbbbdca62 100644 --- a/crates/scanner/src/scanner/tests.rs +++ b/crates/scanner/src/scanner/tests.rs @@ -15,7 +15,8 @@ use super::heal_info::{classify_background_heal_read_error, decode_background_heal_info}; use super::*; use crate::EcstoreResult; -use crate::storage_api::scan::BucketOperations as _; +use crate::storage_api::owner::ecstore_hold_namespace_commit; +use crate::storage_api::scan::{BucketOperations as _, ObjectIO as _}; use crate::{ DATA_USAGE_BLOOM_RECOVERY_PATH, DATA_USAGE_CACHE_KEY_FORMAT, DATA_USAGE_CACHE_NAME, DATA_USAGE_ROOT, DataUsageCachePrepareOutcome, DataUsageCacheSource, DataUsageEntry, DataUsageScanPlanDigest, Endpoint, EndpointServerPools, @@ -1165,6 +1166,116 @@ async fn run_data_scanner_cycle_publishes_activity_for_owner_lifetime() { global_metrics().set_cycle(None).await; } +#[tokio::test] +#[serial] +async fn coordinator_walks_during_pending_put_without_persisting_or_acknowledging_usage() { + crate::scanner_io::clear_dirty_usage_buckets_for_tests(); + let (_temp_dir, store) = setup_scanner_cycle_store().await; + let bucket = format!("scanner-coordinator-pending-{}", Uuid::new_v4().simple()); + store + .make_bucket(&bucket, &crate::storage_api::scan::MakeBucketOptions::default()) + .await + .expect("fixture bucket should be created"); + let mut reader = PutObjReader::from_vec(b"first".to_vec()); + store.pools[0].disk_set[0] + .put_object( + &bucket, + "object", + &mut reader, + &ObjectOptions { + no_lock: true, + ..Default::default() + }, + ) + .await + .expect("fixture object should finish its rename fanout"); + crate::scanner_io::record_dirty_usage_bucket(&bucket); + let dirty_before = crate::scanner_io::dirty_usage_buckets_for_tests(); + let baseline = read_config(store.clone(), DATA_USAGE_OBJ_NAME_PATH.as_str()) + .await + .expect("fixture usage baseline should be readable"); + let pending = ecstore_hold_namespace_commit(store.as_ref()); + let ctx = CancellationToken::new(); + let budget = ScannerCycleBudget::new_with_progress_tracking(&ctx, ScannerCycleBudgetConfig::default()); + let mut cycle_info = CurrentCycle { + next: 1, + ..Default::default() + }; + let mut revision = DataUsageCacheRevision::Missing; + let outcome = tokio::time::timeout( + Duration::from_secs(30), + run_data_scanner_cycle_with_budget(&ctx, &store, &mut cycle_info, &mut revision, 1, Arc::clone(&budget), true), + ) + .await + .expect("the coordinator must finish its namespace walk while a PUT is pending"); + assert_eq!(budget.progress().0, 1, "the coordinator must reach actual object traversal"); + assert_eq!(outcome, ScannerCycleOutcome::Deferred(ScannerCycleDeferReason::DataMovement)); + assert_eq!(cycle_info.next, 1, "a rejected publication must not advance the cycle"); + assert_eq!(revision, DataUsageCacheRevision::Missing); + assert_eq!(crate::scanner_io::dirty_usage_buckets_for_tests(), dirty_before); + assert_eq!( + read_config(store.clone(), DATA_USAGE_OBJ_NAME_PATH.as_str()) + .await + .expect("the prior authoritative usage must remain readable"), + baseline, + "the pending candidate must not replace the authoritative baseline" + ); + + let committed_body = b"committed-after-walk"; + let mut reader = PutObjReader::from_vec(committed_body.to_vec()); + store.pools[0].disk_set[0] + .put_object( + &bucket, + "object", + &mut reader, + &ObjectOptions { + no_lock: true, + ..Default::default() + }, + ) + .await + .expect("the pending tail must change the physical object before it drains"); + assert_eq!(crate::scanner_io::dirty_usage_buckets_for_tests(), dirty_before); + drop(pending); + let retry_budget = ScannerCycleBudget::new_with_progress_tracking(&ctx, ScannerCycleBudgetConfig::default()); + let outcome = tokio::time::timeout( + Duration::from_secs(30), + run_data_scanner_cycle_with_budget(&ctx, &store, &mut cycle_info, &mut revision, 1, Arc::clone(&retry_budget), true), + ) + .await + .expect("the same cycle must converge after the pending PUT drains"); + assert_eq!( + retry_budget.progress().0, + 1, + "the same-cycle retry must not reuse the pre-tail bucket cache" + ); + assert!(matches!( + outcome, + ScannerCycleOutcome::Completed | ScannerCycleOutcome::CompletedWithPendingMaintenance + )); + assert_eq!(cycle_info.next, 2); + assert!(!crate::scanner_io::dirty_usage_buckets_for_tests().contains_key(&bucket)); + let usage = read_config(store.clone(), DATA_USAGE_OBJ_NAME_PATH.as_str()) + .await + .expect("the converged usage should be persisted"); + let usage: DataUsageInfo = serde_json::from_slice(&usage).expect("the persisted usage should decode"); + assert_eq!(usage.usage_snapshot_converged, Some(true)); + assert_eq!(usage.scanner_cycle, Some(1)); + assert_eq!(usage.objects_total_count, 1); + assert_eq!( + usage.objects_total_size, + u64::try_from(committed_body.len()).expect("fixture body length") + ); + let bucket_usage = usage + .buckets_usage + .get(&bucket) + .expect("the scanned bucket should be published"); + assert_eq!(bucket_usage.objects_count, 1); + assert_eq!(bucket_usage.size, u64::try_from(committed_body.len()).expect("fixture body length")); + global_metrics().set_cycle(None).await; + crate::scanner_io::clear_dirty_usage_buckets_for_tests(); +} + #[tokio::test] #[serial] async fn test_finalize_partial_scan_cycle_advances_and_persists_counter() { @@ -8642,6 +8753,66 @@ fn scoped_scan_remote_dirty_coverage_invalidates_local_bucket_current() { )); } +#[test] +fn post_lease_activity_proof_rejects_a_put_tail_that_finished_before_lease_acquisition() { + let before = BTreeMap::from([("node-2".to_string(), scanner_node_activity("epoch-a", 7, 3))]); + let expected_digest = Some(scanner_activity_snapshot_digest(&before)); + assert_eq!(scanner_post_lease_activity_defer_reason(expected_digest, Ok(before.clone())), None); + + let mut after = before.clone(); + after + .get_mut("node-2") + .expect("writer should be present") + .namespace_generation += 1; + assert_eq!( + before["node-2"].movement_generation, after["node-2"].movement_generation, + "the existing movement-only lease remains valid after a PUT tail drains" + ); + assert!(scanner_activity_allows_usage_publication(&after)); + let reason = scanner_post_lease_activity_defer_reason(expected_digest, Ok(after)); + assert_eq!(reason, Some(ScannerCycleDeferReason::ActivityBaselineUnavailable)); + + let result = ScannerCycleResult::new(ScannerCycleStatus::Complete, None).with_remote_dirty_usage_acknowledgements(vec![ + ScannerDirtyUsageAcknowledgement { + host: "node-2".to_string(), + instance_id: "epoch-a".to_string(), + generation: 5, + }, + ]); + let (outcome, _, acknowledgements) = finalize_scanner_cycle_result( + result, + DataUsagePersistOutcome::Deferred(reason.expect("changed namespace should defer publication")), + ); + assert_eq!( + outcome, + ScannerCycleOutcome::Deferred(ScannerCycleDeferReason::ActivityBaselineUnavailable) + ); + assert!( + acknowledgements.is_empty(), + "a rejected publication must not acknowledge the peer's dirty usage" + ); +} + +#[test] +fn post_lease_activity_proof_requires_a_complete_matching_baseline() { + let before = BTreeMap::from([("node-2".to_string(), scanner_node_activity("epoch-a", 7, 3))]); + let digest = scanner_activity_snapshot_digest(&before); + let mut blocked = before.clone(); + blocked.get_mut("node-2").expect("peer should be present").publication_blocked = true; + let blocked_digest = scanner_activity_snapshot_digest(&blocked); + for (expected, observed) in [ + (None, Ok(before)), + (Some(digest), Err("peer is unavailable".to_string())), + (Some(digest), Ok(BTreeMap::new())), + (Some(blocked_digest), Ok(blocked)), + ] { + assert_eq!( + scanner_post_lease_activity_defer_reason(expected, observed), + Some(ScannerCycleDeferReason::ActivityBaselineUnavailable) + ); + } +} + #[test] fn scanner_activity_snapshot_digest_fences_storage_topology() { let first = BTreeMap::from([("node-2".to_string(), scanner_node_activity("epoch-a", 7, 3))]); diff --git a/crates/scanner/src/scanner_io.rs b/crates/scanner/src/scanner_io.rs index d1cc7a073..09e959a3b 100644 --- a/crates/scanner/src/scanner_io.rs +++ b/crates/scanner/src/scanner_io.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::data_usage_define::DATA_USAGE_CACHE_KEY_FORMAT; +use crate::data_usage_define::{DATA_USAGE_CACHE_KEY_FORMAT, DataUsageCacheRevisions}; use crate::scanner_budget::ScannerCycleBudget; use crate::scanner_folder::{ScannerItem, scan_data_folder}; use crate::sleeper::SCANNER_SLEEPER; @@ -286,6 +286,8 @@ pub struct ScannerBucketScanPlan { /// Includes mutation generations even when the set planner uses a structural digest. bucket_coverage_digest: DataUsageScanPlanDigest, requires_full_scan: bool, + // Cache work must invalidate on namespace completion even when its scoped baseline remains reusable. + execution_digest: DataUsageScanPlanDigest, leader_epoch: u64, tier_registry_generation: u64, /// Epoch captured once for the whole scanner cycle. `None` is retained @@ -518,9 +520,12 @@ async fn scanner_cycle_activity_status( where S: ScannerStorage, { + // Read the pending-commit barrier before sampling its completion generation. + // A tail that drains during this await must invalidate the earlier baseline. + let publication_blocked = store.scanner_data_usage_publication_blocked().await; match crate::scanner::probe_scanner_activity(store, distributed).await { Ok(after) => { - let status = if after == *before { + let status = if !publication_blocked && after == *before { ScannerCycleActivityStatus::Unchanged } else { ScannerCycleActivityStatus::Changed @@ -855,6 +860,7 @@ fn scanner_activity_preflight( pub(crate) struct ScannerCycleResult { pub(crate) status: ScannerCycleStatus, publication_epoch: Option, + activity_digest: Option<[u8; 32]>, observational_snapshot_published: bool, dirty_usage_clear: Option, remote_dirty_usage_acknowledgements: Vec, @@ -869,6 +875,7 @@ impl ScannerCycleResult { Self { status, publication_epoch: None, + activity_digest: None, observational_snapshot_published: false, dirty_usage_clear, remote_dirty_usage_acknowledgements: Vec::new(), @@ -888,6 +895,15 @@ impl ScannerCycleResult { self.publication_epoch } + fn with_activity_digest(mut self, activity_digest: [u8; 32]) -> Self { + self.activity_digest = Some(activity_digest); + self + } + + pub(crate) fn activity_digest(&self) -> Option<[u8; 32]> { + self.activity_digest + } + pub(crate) fn with_observational_snapshot_published(mut self, published: bool) -> Self { self.observational_snapshot_published = published; self diff --git a/crates/scanner/src/scanner_io/cache.rs b/crates/scanner/src/scanner_io/cache.rs index 8b13c558a..5e47128b8 100644 --- a/crates/scanner/src/scanner_io/cache.rs +++ b/crates/scanner/src/scanner_io/cache.rs @@ -682,11 +682,13 @@ pub(super) async fn persist_and_publish_cache_snapshot( store: Arc, updates: &mpsc::Sender, mut cache_snapshot: DataUsageCache, + initial_revisions: Option<&DataUsageCacheRevisions>, cache_cycle_floor: &AtomicU64, expected_publication_epoch: u64, ) -> Option { let source = cache_snapshot.info.source?; let coverage_digest = cache_snapshot.info.scan_coverage_digest?; + let execution_digest = cache_snapshot.info.scan_execution_digest?; let guard = match acquire_scanner_cache_locks(store.as_ref(), DATA_USAGE_CACHE_NAME, source).await { Ok(guard) => guard, Err(err) => { @@ -752,6 +754,7 @@ pub(super) async fn persist_and_publish_cache_snapshot( return None; } if persisted.info.scan_coverage_digest == Some(coverage_digest) + && persisted.info.scan_execution_digest == Some(execution_digest) && matches!( current_cache_root_entry_with_generation( &persisted, @@ -767,6 +770,20 @@ pub(super) async fn persist_and_publish_cache_snapshot( { cache_snapshot = persisted; } else { + // A later execution may have completed while this scan was walking. + // Only replace the cache revision from which this scan started. + if initial_revisions != Some(&revisions) { + warn!( + target: "rustfs::scanner::io", + event = EVENT_SCANNER_CACHE_PERSIST_STATE, + component = LOG_COMPONENT_SCANNER, + subsystem = LOG_SUBSYSTEM_IO, + state = "scan_baseline_revision_changed", + cache_name = DATA_USAGE_CACHE_NAME, + "Scanner skipped set snapshot without an unchanged baseline revision" + ); + return None; + } if guard.is_lock_lost() { error!( target: "rustfs::scanner::io", diff --git a/crates/scanner/src/scanner_io/io_cache.rs b/crates/scanner/src/scanner_io/io_cache.rs index bfd59ea95..fcae372ca 100644 --- a/crates/scanner/src/scanner_io/io_cache.rs +++ b/crates/scanner/src/scanner_io/io_cache.rs @@ -117,6 +117,7 @@ impl ScannerIOCache for SetDisks { digest: scan_plan_digest, bucket_coverage_digest, requires_full_scan, + execution_digest, leader_epoch, tier_registry_generation, publication_epoch, @@ -138,20 +139,24 @@ impl ScannerIOCache for SetDisks { .ok_or_else(|| StorageError::other("scanner cache publication is blocked by data movement"))?, }; let mut old_cache = DataUsageCache::default(); - if let Err(e) = old_cache.load(self.clone(), DATA_USAGE_CACHE_NAME).await { - warn!( - target: "rustfs::scanner::io", - event = EVENT_SCANNER_CACHE_PERSIST_STATE, - component = LOG_COMPONENT_SCANNER, - subsystem = LOG_SUBSYSTEM_IO, - pool = self.pool_index, - set = self.set_index, - cache_name = DATA_USAGE_CACHE_NAME, - state = "old_cache_load_failed", - error = %e, - "Scanner old data usage cache load failed; rebuilding from bucket caches" - ); - } + let initial_revisions = match old_cache.load_with_revisions(self.clone(), DATA_USAGE_CACHE_NAME).await { + Ok(revisions) => Some(revisions), + Err(e) => { + warn!( + target: "rustfs::scanner::io", + event = EVENT_SCANNER_CACHE_PERSIST_STATE, + component = LOG_COMPONENT_SCANNER, + subsystem = LOG_SUBSYSTEM_IO, + pool = self.pool_index, + set = self.set_index, + cache_name = DATA_USAGE_CACHE_NAME, + state = "old_cache_load_failed", + error = %e, + "Scanner old data usage cache load failed; rebuilding from bucket caches" + ); + None + } + }; let scoped_scan = prepare_scoped_set_scan( &old_cache, &buckets, @@ -198,6 +203,7 @@ impl ScannerIOCache for SetDisks { }; cache.info.last_update = Some(now); cache.info.snapshot_complete = true; + cache.info.scan_execution_digest = Some(execution_digest); cache.info.lkg_snapshot_complete = false; cache.info.lkg_next_cycle = None; cache.info.lkg_last_update = None; @@ -211,6 +217,7 @@ impl ScannerIOCache for SetDisks { self, &updates, cache, + initial_revisions.as_ref(), cache_cycle_floor.as_ref(), expected_publication_epoch, ) @@ -1391,6 +1398,7 @@ impl ScannerIOCache for SetDisks { cache.info.next_cycle = want_cycle; cache.info.last_update.get_or_insert_with(SystemTime::now); cache.info.snapshot_complete = true; + cache.info.scan_execution_digest = Some(execution_digest); cache.info.lkg_snapshot_complete = false; cache.info.lkg_next_cycle = None; cache.info.lkg_last_update = None; @@ -1402,6 +1410,7 @@ impl ScannerIOCache for SetDisks { self.clone(), &updates, cache_snapshot, + initial_revisions.as_ref(), cache_cycle_floor.as_ref(), expected_publication_epoch, ) diff --git a/crates/scanner/src/scanner_io/io_cycle.rs b/crates/scanner/src/scanner_io/io_cycle.rs index 5cb452d6d..da571d1ad 100644 --- a/crates/scanner/src/scanner_io/io_cycle.rs +++ b/crates/scanner/src/scanner_io/io_cycle.rs @@ -194,7 +194,7 @@ where // canceled decommission remains suspended after its worker exits, so // starting a scan in that state could build a snapshot that cannot be // routed to the authoritative metadata object. - if store.scanner_data_usage_publication_blocked().await { + if store.scanner_data_movement_pause_status().await.paused { debug!( target: "rustfs::scanner::io", event = EVENT_SCANNER_SET_STATE, @@ -278,8 +278,9 @@ where let structural_scan_plan_digest = scanner_bucket_plan_digest(&all_buckets, crate::scanner::scanner_activity_structural_digest(&activity_before)); let scan_plan_digest = scanner_bucket_work_digest(structural_scan_plan_digest, scan_mode, requires_full_scan); - let bucket_coverage_digest = - scanner_bucket_plan_digest(&all_buckets, crate::scanner::scanner_activity_snapshot_digest(&activity_before)); + let activity_digest = crate::scanner::scanner_activity_snapshot_digest(&activity_before); + let bucket_coverage_digest = scanner_bucket_plan_digest(&all_buckets, activity_digest); + let execution_digest = scanner_bucket_work_digest(bucket_coverage_digest, scan_mode, requires_full_scan); let dirty_usage_snapshot = Arc::new(snapshot_dirty_usage_buckets(&all_buckets, dirty_generation_before_bucket_list)); let scan_scope = resolve_scanner_bucket_scan_scope( store, @@ -349,6 +350,7 @@ where }; return Ok(ScannerCycleResult::new(status, dirty_usage_clear) .with_publication_epoch(publication_epoch) + .with_activity_digest(activity_digest) .with_observational_snapshot_published(observational_snapshot_published) .with_remote_publication_lease_targets(remote_publication_lease_targets) .with_remote_dirty_usage_acknowledgements(remote_dirty_usage_acknowledgements)); @@ -435,6 +437,7 @@ where digest: structural_scan_plan_digest, bucket_coverage_digest, requires_full_scan, + execution_digest, leader_epoch, tier_registry_generation, publication_epoch, @@ -632,6 +635,7 @@ where }; Ok(ScannerCycleResult::new(cycle_status, dirty_usage_clear) .with_publication_epoch(publication_epoch) + .with_activity_digest(activity_digest) .with_observational_snapshot_published(observational_snapshot_published) .with_remote_publication_lease_targets(remote_publication_lease_targets) .with_remote_dirty_usage_acknowledgements(remote_dirty_usage_acknowledgements) diff --git a/crates/scanner/src/scanner_io/tests.rs b/crates/scanner/src/scanner_io/tests.rs index e5a868875..19e0fc457 100644 --- a/crates/scanner/src/scanner_io/tests.rs +++ b/crates/scanner/src/scanner_io/tests.rs @@ -20,6 +20,7 @@ use crate::scanner_folder::ScannerItem; use crate::storage_api::EcstoreScannerPeerDirtyUsageSnapshot; use crate::storage_api::owner::{ EcstorePoolDecommissionInfo, EcstoreRebalStatus, EcstoreRebalanceInfo, EcstoreRebalanceMeta, EcstoreRebalanceStats, + ecstore_hold_namespace_commit, }; use crate::storage_api::scan::{BucketOperations as _, DeleteBucketOptions, MakeBucketOptions, ObjectIO as _}; use crate::{ @@ -583,6 +584,16 @@ async fn multi_pool_scanner_cycle_publishes_combined_usage() { .put_object(&bucket, object, &mut reader, &ScannerObjectOptions::default()) .await .expect("object should be written to its selected pool"); + + // Quorum ACK can precede tail publication on the disk chosen to scan. + let lock = store.pools[pool_index].disk_set[0] + .new_ns_lock(&bucket, object) + .await + .expect("fixture namespace lock should be created"); + let _settled = lock + .get_write_lock(Duration::from_secs(30)) + .await + .expect("fixture rename tail should finish before the usage scan"); } let ctx = CancellationToken::new(); @@ -602,7 +613,7 @@ async fn multi_pool_scanner_cycle_publishes_combined_usage() { .buckets_usage .get(&bucket) .expect("combined bucket usage should be present"); - assert_eq!(bucket_usage.objects_count, 2); + assert_eq!(bucket_usage.objects_count, 2, "{usage:?}"); assert_eq!(bucket_usage.size, 11); assert_eq!(usage.objects_total_count, 2); assert_eq!(usage.objects_total_size, 11); @@ -612,6 +623,102 @@ async fn multi_pool_scanner_cycle_publishes_combined_usage() { ); } +#[tokio::test] +#[serial] +async fn pending_put_commit_keeps_scanner_walk_live_without_authoritative_usage() { + let (_temp_dir, store) = setup_two_pool_scanner_store().await; + let bucket = format!("scanner-pending-put-{}", Uuid::new_v4().simple()); + store + .make_bucket(&bucket, &MakeBucketOptions::default()) + .await + .expect("bucket should be created across both pools"); + for (pool_index, (object, body)) in [("pool-a", b"first".as_slice()), ("pool-b", b"second".as_slice())] + .into_iter() + .enumerate() + { + let mut reader = ScannerPutObjReader::from_vec(body.to_vec()); + store.pools[pool_index].disk_set[0] + .put_object( + &bucket, + object, + &mut reader, + &ScannerObjectOptions { + no_lock: true, + ..Default::default() + }, + ) + .await + .expect("fixture objects must finish their rename fanouts before scanning"); + } + + let mut pending = Some(ecstore_hold_namespace_commit(store.as_ref())); + let mut previous_activity_digest = None; + let mut structural_plan_digest = None; + for (cycle, converged) in [(1, false), (2, true)] { + if converged { + drop(pending.take()); + } + assert_eq!(store.scanner_data_usage_publication_blocked().await, !converged); + assert!(!store.scanner_data_movement_pause_status().await.paused); + let activity = crate::scanner::probe_scanner_activity(store.as_ref(), false) + .await + .expect("the fixture activity should be observable"); + let activity_digest = crate::scanner::scanner_activity_snapshot_digest(&activity); + if let Some(previous) = previous_activity_digest.replace(activity_digest) { + assert_ne!(previous, activity_digest, "draining a namespace commit must change the publication proof"); + } + let ctx = CancellationToken::new(); + let budget = ScannerCycleBudget::new_with_progress_tracking(&ctx, ScannerCycleBudgetConfig::default()); + let (updates, mut receiver) = mpsc::channel(1); + let result = tokio::time::timeout( + Duration::from_secs(30), + ScannerIOCycle::nsscanner_with_status( + store.as_ref(), + ctx, + Arc::clone(&budget), + updates, + cycle, + 1, + HealScanMode::Normal, + ), + ) + .await + .expect("namespace scanning must finish while a PUT commit is pending") + .expect("namespace scanning must remain available during a pending PUT commit"); + assert_eq!(result.activity_digest(), Some(activity_digest)); + if !converged { + assert_eq!(budget.progress().0, 2, "the pending commit must not suppress actual object traversal"); + } + assert_eq!( + result.status, + if converged { + ScannerCycleStatus::Complete + } else { + ScannerCycleStatus::Superseded + } + ); + let usage = receiver + .recv() + .await + .expect("the completed walk should produce a usage candidate"); + assert_eq!(usage.usage_snapshot_converged, Some(converged)); + assert_eq!(usage.scanner_cycle, Some(cycle)); + assert_eq!(usage.objects_total_count, 2); + assert_eq!(usage.objects_total_size, 11); + assert_eq!(usage.usage_snapshot_set_states.len(), 2); + for state in &usage.usage_snapshot_set_states { + let digest = state + .scan_plan_digest + .expect("each set must retain its structural cache identity"); + assert_eq!(*structural_plan_digest.get_or_insert(digest), digest); + } + let bucket_usage = usage.buckets_usage.get(&bucket).expect("the walked bucket must be present"); + assert_eq!(bucket_usage.objects_count, 2); + assert_eq!(bucket_usage.size, 11); + assert!(receiver.recv().await.is_none(), "each walk must emit exactly one terminal candidate"); + } +} + #[tokio::test] #[serial] async fn multi_pool_scanner_cycle_zero_fills_bucket_absent_from_first_pool() { @@ -627,6 +734,16 @@ async fn multi_pool_scanner_cycle_zero_fills_bucket_absent_from_first_pool() { .put_object(&bucket, "pool-b", &mut reader, &ScannerObjectOptions::default()) .await .expect("object should be written only to the second pool"); + { + let lock = store.pools[1].disk_set[0] + .new_ns_lock(&bucket, "pool-b") + .await + .expect("fixture namespace lock should be created"); + let _settled = lock + .get_write_lock(Duration::from_secs(30)) + .await + .expect("fixture rename tail should finish before the usage scan"); + } store.pools[0] .delete_bucket(&bucket, &DeleteBucketOptions::default()) .await @@ -1016,6 +1133,7 @@ fn complete_set_usage_cache(buckets: &[(&str, usize)], scan_plan_digest: DataUsa source: Some(DataUsageCacheSource::new(1, 2)), snapshot_complete: true, scan_plan_digest: Some(scan_plan_digest), + scan_coverage_digest: Some(scan_plan_digest), cache_key_format: DATA_USAGE_CACHE_KEY_FORMAT, tier_registry_generation: Some(13), ..Default::default() @@ -1037,6 +1155,126 @@ fn complete_set_usage_cache(buckets: &[(&str, usize)], scan_plan_digest: DataUsa cache } +#[tokio::test] +#[serial] +async fn set_snapshot_reuse_requires_execution_identity_and_fences_stale_writers() { + let (_temp_dir, store) = setup_two_pool_scanner_store().await; + let set = Arc::clone(&store.pools[0].disk_set[0]); + let epoch = scanner_publication_epoch(Arc::clone(&set)).await.expect("idle set admission"); + let mut legacy = complete_set_usage_cache(&[("photos", 5)], DataUsageScanPlanDigest([1; 32])); + legacy.info.source = Some(DataUsageCacheSource::new(0, 0)); + legacy + .save(Arc::clone(&set), DATA_USAGE_CACHE_NAME) + .await + .expect("seed legacy set cache"); + let mut persisted = DataUsageCache::default(); + let initial = persisted + .load_with_revisions(Arc::clone(&set), DATA_USAGE_CACHE_NAME) + .await + .expect("capture the shared starting revision"); + let mut fresh = legacy.clone(); + fresh.info.scan_execution_digest = Some(DataUsageScanPlanDigest([2; 32])); + fresh.replace( + "photos", + DATA_USAGE_ROOT, + DataUsageEntry { + size: 20, + objects: 1, + ..Default::default() + }, + ); + let cycle_floor = AtomicU64::new(fresh.info.next_cycle); + let (tx, mut rx) = mpsc::channel(1); + assert!( + persist_and_publish_cache_snapshot(Arc::clone(&set), &tx, fresh.clone(), Some(&initial), &cycle_floor, epoch) + .await + .is_some(), + "a legacy cache without execution identity must be refreshed" + ); + let published = rx.try_recv().expect("fresh snapshot should be forwarded"); + assert_eq!(published.find("photos").expect("published bucket").size, 20); + assert_eq!(published.info.scan_execution_digest, fresh.info.scan_execution_digest); + let current = persisted + .load_with_revisions(Arc::clone(&set), DATA_USAGE_CACHE_NAME) + .await + .expect("capture the current revision for the unidentified execution"); + + let mut stale = legacy.clone(); + stale.info.scan_execution_digest = Some(DataUsageScanPlanDigest([3; 32])); + for (candidate, revisions) in [(stale, &initial), (legacy, ¤t)] { + assert!( + persist_and_publish_cache_snapshot(Arc::clone(&set), &tx, candidate, Some(revisions), &cycle_floor, epoch) + .await + .is_none(), + "a stale or unidentified execution must not replace the newer snapshot" + ); + assert!(matches!(rx.try_recv(), Err(mpsc::error::TryRecvError::Empty))); + } + fresh.info.scan_execution_digest = Some(DataUsageScanPlanDigest([4; 32])); + assert!( + persist_and_publish_cache_snapshot(Arc::clone(&set), &tx, fresh.clone(), None, &cycle_floor, epoch) + .await + .is_none(), + "an unreadable starting revision must not authorize an overwrite" + ); + + fresh.info.scan_execution_digest = published.info.scan_execution_digest; + fresh.replace("photos", DATA_USAGE_ROOT, DataUsageEntry::default()); + assert!( + persist_and_publish_cache_snapshot(Arc::clone(&set), &tx, fresh, Some(&initial), &cycle_floor, epoch) + .await + .is_some(), + "an overlapping identical execution must reuse the completed snapshot" + ); + assert_eq!( + rx.try_recv() + .expect("reused snapshot") + .find("photos") + .expect("reused bucket") + .size, + 20 + ); + persisted + .load(Arc::clone(&set), DATA_USAGE_CACHE_NAME) + .await + .expect("read the final durable set cache"); + assert_eq!(persisted.find("photos").expect("durable bucket").size, 20); + assert_eq!(persisted.info.scan_execution_digest, published.info.scan_execution_digest); + + let ctx = CancellationToken::new(); + let empty_execution = DataUsageScanPlanDigest([5; 32]); + set.nsscanner_cache( + ctx.clone(), + ScannerCycleBudget::new(&ctx, ScannerCycleBudgetConfig::default()), + ScannerBucketScanPlan { + buckets: Vec::new(), + all_buckets: Arc::new(Vec::new()), + scope: ScannerBucketScanScope::default(), + digest: DataUsageScanPlanDigest([6; 32]), + bucket_coverage_digest: DataUsageScanPlanDigest([6; 32]), + requires_full_scan: false, + execution_digest: empty_execution, + leader_epoch: 11, + tier_registry_generation: 13, + publication_epoch: Some(epoch), + dirty_usage_buckets: Arc::new(HashMap::new()), + bucket_failures: ScannerBucketFailureState::default(), + pending_maintenance_work: Arc::new(AtomicBool::new(false)), + cache_cycle_floor: Arc::new(AtomicU64::new(8)), + }, + tx, + 8, + HealScanMode::Normal, + ) + .await + .expect("empty set scope should replace its prior nonempty cache"); + let empty = rx.try_recv().expect("empty set snapshot should be published"); + assert_eq!(empty.info.scan_execution_digest, Some(empty_execution)); + assert!(empty.info.snapshot_complete); + let root = empty.checked_flatten(DATA_USAGE_ROOT).expect("complete empty root"); + assert_eq!((root.size, root.objects), (0, 0)); +} + fn complete_usage_baseline( source: DataUsageCacheSource, scan_plan_digest: DataUsageScanPlanDigest, diff --git a/crates/scanner/src/storage_api.rs b/crates/scanner/src/storage_api.rs index 7b2cda493..a1981df30 100644 --- a/crates/scanner/src/storage_api.rs +++ b/crates/scanner/src/storage_api.rs @@ -127,6 +127,9 @@ pub(crate) use rustfs_lifecycle::{ use rustfs_storage_api as storage_contracts; pub(crate) mod owner { + #[cfg(test)] + pub(crate) use rustfs_ecstore::api::set_disk::test_util::hold_namespace_commit as ecstore_hold_namespace_commit; + pub(crate) use super::storage_contracts::{ HTTPPreconditions, HTTPRangeSpec, NS_SCANNER_PROTOCOL_VERSION, ObjectIO, ObjectOperations, ObjectToDelete, }; diff --git a/docs/architecture/compat-cleanup-register.md b/docs/architecture/compat-cleanup-register.md index 924193b6e..52c50e042 100644 --- a/docs/architecture/compat-cleanup-register.md +++ b/docs/architecture/compat-cleanup-register.md @@ -11,6 +11,7 @@ ## Open Items +- `backlog-2263` legacy heal MRF inspection: retained per-record journals remain readable while committed-snapshot ownership and writer activation are staged. Remove legacy import only after all supported direct-upgrade and rollback readers understand committed snapshots and migration tooling confirms that no retained or restorable legacy journal requires it. This does not enable a new writer or change the automatic legacy consumer. - `backlog-1337` legacy restore orphan recovery: releases that predate the restore worker-lock marker can leave a valid operation-id and `ongoing-request="true"` after cancellation or process failure, with no durable liveness proof. New servers allow an exact, non-nil legacy generation to be superseded only when its consistently parsed request date is at least 24 hours old. Remove the clock-based legacy fallback after the minimum supported direct-upgrade release writes the v1 worker-lock marker on every restore and operators have resolved every retained pre-v1 ongoing generation. - `backlog-2133-tier-delete-chunk-parent` bounded tier-delete dispatch compatibility: prefixes at or below the legacy manifest limit keep the byte-compatible v1 single-manifest protocol, while larger prefixes place a chunk-parent sentinel at the original deterministic root path and use operation-scoped child manifests. Older binaries reject the sentinel and child paths, preserving the v6 sole-owner downgrade fence instead of starting a competing local delete. Remove the v1 reader and fail-closed mixed-version sentinel only after every supported rollback release validates the parent/child protocol and migration tooling confirms that no retained v1 dispatch manifest remains. - `tokio-tar-extension-limits` bounded archive parser hardening: Snowball extraction depends on precedence-resolved MinIO PAX metadata; per-entry and cumulative extension limits; a physical-entry limit; cancellation-safe parsing and ownership of large streamed members; fused streams after errors; and compatibility with minio-go streams that omit the two-block terminator. Swift bulk extraction also uses the same fork. Keep the reviewed pin while the Snowball path is prototyped against tar-codec/tar-framing. Remove it only after a released API exposes the effective allowed vendor records, RustFS provides a cancellation-safe handoff for borrowed member payloads, footerless input is accepted solely when authenticated request framing proves EOF immediately after a complete member, the existing resource-limit, cancellation, error-fuse, and real minio-go fixtures pass against the replacement, and Swift no longer depends on the fork. diff --git a/docs/architecture/scanner-usage-publication.md b/docs/architecture/scanner-usage-publication.md index 4b4791b1f..0665fc473 100644 --- a/docs/architecture/scanner-usage-publication.md +++ b/docs/architecture/scanner-usage-publication.md @@ -23,6 +23,30 @@ therefore has three identities: If any identity changes before commit, the result is a candidate for retry or observation, not an authoritative baseline. +Ordinary PUT rename fanouts also track instance-scoped in-flight work. A quorum +ACK does not release it: the actual disk tasks retain ownership until their +rename work ends, including when the request caller is cancelled. Scan admission +remains movement-only so sustained PUTs do not stop namespace walks and +scanner-driven lifecycle discovery. The post-walk local publication check and +remote publication leases reject pending fanouts. Begin/end namespace generations +invalidate scans and cached plans across the fanout; after acquiring remote +leases, the coordinator rechecks the full activity digest before publishing an +authoritative aggregate. This catches a tail that finishes between the scan's +last probe and lease acquisition. + +This adds no namespace or movement lock. An already-verified older snapshot may +still precede a newly started write. Sustained or stalled PUT tails can delay +authoritative usage publication, which resumes through the existing retry +schedule rather than a new immediate-wakeup protocol. Intermediate per-set and +prefix cache readers retain their existing approximate-cache semantics. A +prolonged pending tail with no generation changes can also delay cycle advancement +and fresh rescans of already-current caches; this is not a guarantee of lifecycle +progress under indefinitely stalled storage I/O. + +This PUT-tail protection requires every writer node to be upgraded. It does not +prove that a failed tail replica has healed, and it does not extend the same +in-flight tracking to multipart or other namespace mutation paths. + ## Fences The protocol uses separate fences because they exclude different stale inputs. @@ -33,7 +57,7 @@ They must not be collapsed unless the replacement proves the same exclusions. | Scanner leadership claim | scanner | competing scanner leaders and stale cycle writers | | Storage publication epoch | ECStore | usage computed across rebalance, decommission, or other data-movement generations | | Publication lease | scanner peers through ECStore-facing activity probes | remote dirty-usage or maintenance state that has not acknowledged the candidate | -| CAS revision | backing config object store | lost updates to `.usage.v2.json`, `.usage.json`, or cycle-state objects | +| CAS revision | backing config object store | lost updates to usage snapshots, scanner caches, or cycle-state objects | | Per-set freshness | scanner aggregation | a merged usage snapshot that combines stale and current set results | | Tier registry generation | scanner tier accounting | bytes classified against a different warm-tier registry | | Usage floor identity | scanner publication and ECStore quota fallback | empty or legacy values becoming plausible authoritative quota input | @@ -42,6 +66,28 @@ A reader that cannot prove the required fence for its surface must fail closed or use the documented observed path below. It must not synthesize an empty usage snapshot for a missing or corrupt authoritative object. +## Cache Execution Identity + +The structural scan-plan digest can remain stable across ordinary bucket writes +so a scoped scan can retain unaffected baseline buckets. It is not sufficient +proof for reusing a completed result within the same cycle. Bucket work uses an +execution digest combining the structural plan and the full activity snapshot, +with the bucket's dirty generation included in its cache identity. Completed set +caches carry the same execution digest separately from their structural plan. +The persisted set-root fast path requires equal execution identities as well as +the existing source, cycle, leader, tier, and cache-structure checks. + +A set scan also captures its starting cache revisions. When the persisted +execution differs, replacement requires those revisions to remain unchanged; +otherwise a slow scan could overwrite a newer completed result. The existing +cache lock, conditional save, and movement admission still fence the commit. + +The optional `scan_execution_digest` field is appended to the map-encoded cache +metadata. Legacy caches remain readable but cannot satisfy same-cycle set-root +reuse without this identity. Older readers can ignore the added map key, but +older writers do not enforce its fence; readability is not a mixed-version +publication-safety guarantee. + ## Persisted Objects The persisted objects are part of the compatibility contract. Removing one diff --git a/docs/operations/on-demand-migration.md b/docs/operations/on-demand-migration.md index eb7cde57f..8155c6f11 100644 --- a/docs/operations/on-demand-migration.md +++ b/docs/operations/on-demand-migration.md @@ -89,9 +89,11 @@ Setting `"enabled": false` in the config has the same read-path effect as deleti The status endpoint reports **the node that answered the request**. Counters, queue depth and breaker state are per-node runtime state, so in a distributed deployment query every node; the saved configuration and `updated_at` are cluster-wide. -### Backfill (ships with ODM-12) +### Backfill -Read-through only migrates what clients touch. The background backfill job walks the source listing and pulls the remainder, with a persisted checkpoint (`.rustfs.sys/buckets//on-demand-migration-backfill.json`), a single-owner lease, resume after restart, and `POST .../{bucket}/backfill?op=start|cancel` plus `GET .../{bucket}/backfill` admin routes. That slice (rustfs/backlog#2159) is not part of the build this page was written against: the shape above is the agreed design, and the exact request/response bodies must be re-checked against `docs/architecture/admin-route-action-snapshot.md` once it lands. +Backfill waits for the result of every pull, including a pull already queued by an online request. A failed or cancelled shared pull is counted as a failure, never as successful migration. The persisted continuation cursor stays at the first failed page; a takeover replays from there and skips objects already present locally. `completed_with_failures` is not a cutover-ready state. + +Read-through only migrates what clients touch. The background backfill job walks the source listing and pulls the remainder, with a persisted checkpoint (`.rustfs.sys/buckets//on-demand-migration-backfill.json`), a single-owner lease, resume after restart, and `POST .../{bucket}/backfill?op=start|cancel` plus `GET .../{bucket}/backfill` admin routes. See `docs/architecture/admin-route-action-snapshot.md` for the route contract. ## Configuration reference @@ -129,7 +131,7 @@ The persisted blob is `on-demand-migration.json` in the bucket's metadata. Unkno | `policy.source_timeout.idle_ms` | integer | `30000` | `100..=600000`; enforced per body chunk on both the background pump and the inline tee | | `policy.bandwidth_limit_bytes_per_sec` | integer \| null | `null` | When set, at least `65536` | -Values that are **not** configurable: the breaker opens after 5 consecutive counted failures inside a 30 s window, stays open for 30 s and then admits one probe (`breaker.rs`); the negative cache holds at most 100 000 keys per bucket with LRU eviction (`negative_cache.rs`); a background pull retries a retryable source failure at most 3 times with 1 s / 4 s / 16 s base delays plus up to 25 % jitter (`pull.rs`). The SDK's own retry policy is disabled on the source client, so one logical source call is exactly one wire request and the retry budget above is the only one. +Values that are **not** configurable: the breaker opens after 5 consecutive counted failures inside a 30 s window, stays open for 30 s and then admits one probe (`breaker.rs`); the negative cache holds at most 100 000 keys per bucket with LRU eviction (`negative_cache.rs`); a background pull retries a retryable source failure at most 3 times with 1 s / 4 s / 16 s base delays plus up to 25 % jitter (`pull.rs`). The SDK's own retry policy is disabled on the source client. Each SDK operation makes one wire request; an ambiguous HEAD 404 additionally probes the bucket, within the same configured first-byte budget. Validation also rejects two shapes outright: a source whose endpoint and bucket name **this** bucket on this deployment (`SelfReference`), and a source that matches one of the bucket's own replication targets (`ReplicationLoop`) — that pairing would amplify a write-back into a loop. @@ -160,6 +162,14 @@ No write, delete, ACL or versioning permission is required or used. Scope the po Behaviour a client can observe. The "Test" column names the case that pins it: `*_test.rs` files live under `crates/e2e_test/src/on_demand_migration/`, and the unit tests live next to the code in `rustfs/src/app/object/get.rs`, `head.rs` and `shared.rs`. +ODM merged continuation tokens use a NUL-prefixed JSON envelope inside the existing base64 encoding. NUL is not valid in a local object key, so a legitimate JSON-shaped key can never be mistaken for a merged cursor. Upgrade every node before using list-through, and restart any in-progress ODM listing issued by an older build: its unframed JSON tokens cannot be distinguished from legitimate local keys. Ordinary local listing tokens remain unchanged. Tokens issued by this build can still resume the local side after list-through is disabled. + +Source `HEAD` responses with status 404 require a successful bucket probe before being negative-cached. The source credential therefore needs permission for `HeadBucket` (S3 `ListBucket`); a prefix-restricted ListBucket policy can deny that probe, in which case the response is a source failure rather than a cached miss. A missing/inaccessible source bucket, a missing source version, or an ambiguous GET 404 is not proof that the requested key is absent. Conditional GET validators are checked against the actual source GET metadata as well as the advisory HEAD; a missing required validator fails with 424. Source LIST entries without a key or a non-negative size fail the page rather than fabricating an empty object. + +Write-back currently requires namespace locking enabled and exactly one pool with one erasure set. Other topologies fail write-back explicitly as `unsupported`: source reads remain available, but backfill cannot complete successfully or certify cutover. This restriction avoids relying on a set-local condition across distinct pool or lock domains; it does not restrict ordinary S3 writes. Full cross-pool migration requires a globally fenced commit protocol. + +On the supported topology, write-back uses a create-only check under the local storage commit lock for both single-part PUT and multipart completion. A client write that commits while ODM is reading the source is preserved. With `respect_local_delete_marker=true`, a concurrent versioned deletion is preserved too. An explicit `respect_local_delete_marker=false` still permits revival; an unversioned deletion has no tombstone and therefore cannot be distinguished from a key that has never existed locally. + | Situation | Behaviour | Test | |---|---|---| | GET miss, object at or below `inline_max_bytes` | One source GET, teed: the client streams while the same bytes are written locally. Later reads are local and carry no source marker | `get_basic_test.rs::get_miss_pulls_inline_and_serves_locally_afterwards`, `get.rs::odm_get_inline_streams_to_client_and_commits_the_same_bytes` | @@ -229,7 +239,7 @@ Five provenance keys are written on every pulled object under both internal pref | Concurrency limit | Local write amplification | `max_concurrent_pulls` permits shared by inline and background pulls | | Bounded queue | Unbounded memory on a burst | `pull_queue_capacity` waiting jobs; overflow is counted as `queue_full` and never fails a client response | | Bandwidth limit | Source and network saturation | `bandwidth_limit_bytes_per_sec` (minimum 64 KiB/s) on the source client | -| Retry budget | Transient source blips | Background pulls retry a retryable failure up to 3 times (1 s / 4 s / 16 s plus jitter). Inline pulls never retry: the bytes are already on their way to the client. The SDK retry policy on the source client is disabled (`RemoteS3RetryPolicy::Disabled`), so this is the only retry budget and one logical source call is exactly one wire request — replication targets keep the SDK's three attempts, declared on their own spec | +| Retry budget | Transient source blips | Background pulls retry a retryable failure up to 3 times (1 s / 4 s / 16 s plus jitter). Inline pulls never retry: the bytes are already on their way to the client. The SDK retry policy is disabled (`RemoteS3RetryPolicy::Disabled`); HEAD 404 also requires one bucket probe. Replication targets keep their separately declared three SDK attempts | | Idle timeout | A source that answers and then goes quiet mid-body | `source_timeout.idle_ms` per body chunk on both paths. The budget measures the source read, upstream of the inline tee, so a slow client is never mistaken for an idle source; when it fires the client stream ends in an error and the write-back is discarded | | Anti-loop marker | Migration chains between RustFS/MinIO deployments | Every source request carries `x-rustfs-source-proxy-request` and `x-minio-source-proxy-request`; a request carrying it is always answered locally | | Outbound endpoint policy | SSRF | See [outbound-connection-policy.md](outbound-connection-policy.md) | diff --git a/docs/operations/replication-outbound-transport.md b/docs/operations/replication-outbound-transport.md index 8ff7f285a..f3149e21c 100644 --- a/docs/operations/replication-outbound-transport.md +++ b/docs/operations/replication-outbound-transport.md @@ -29,6 +29,18 @@ Both knobs are read by the RustFS process that owns the replication target, at client build time; restart the server after changing them. +### Remote tier transport timeouts + +Remote tier S3-compatible clients use separate transport budgets. These settings do not change bucket or site replication clients. + +| Variable | Default | Meaning | +| --- | --- | --- | +| `RUSTFS_TIER_REMOTE_CONNECT_TIMEOUT_SECS` | `10` | Maximum time to establish the remote tier TCP connection. | +| `RUSTFS_TIER_REMOTE_REQUEST_TIMEOUT_SECS` | `86400` | Maximum time for a remote tier request to reach response headers. The long default preserves large transition-upload headroom. | +| `RUSTFS_TIER_REMOTE_RESPONSE_BODY_IDLE_TIMEOUT_SECS` | `60` | Maximum time without a non-empty response-body chunk. Empty HTTP/2 frames do not count as progress. | + +All three values must be positive integers. Zero fails tier client initialization instead of silently disabling the boundary. An invalid integer is logged and falls back to the default; very large values are accepted and provide a correspondingly long effective budget. The values are read when the tier client is built; recreate or reload the tier configuration after changing them. + ## Before changing any of this Follow the SOP in `docs/postmortems/2026-09-03-replication-checksum-default-regression.md`: inventory the target-side rules the current default satisfies, run the outbound target matrix, and document any new knob here in the same PR. diff --git a/docs/operations/tier-ilm-debugging.md b/docs/operations/tier-ilm-debugging.md index 3b4a5d3a6..d5da40af3 100644 --- a/docs/operations/tier-ilm-debugging.md +++ b/docs/operations/tier-ilm-debugging.md @@ -22,6 +22,16 @@ | `FileMeta` / `FileInfo` / version metadata | `crates/filemeta/src/` | | Dual-key internal metadata helpers (`insert_bytes` / `get_bytes`) | `crates/utils/src/http/metadata_compat.rs` | +## Lifecycle rule limits and evaluation + +Each lifecycle rule supports at most one `Transition` and one `NoncurrentVersionTransition`. A version can make one initial transition; chaining additional tiers after it reaches `complete` is not supported. Splitting stages across overlapping rules does not enable a transition chain. `PutBucketLifecycleConfiguration` rejects multiple entries in either transition array with `InvalidArgument`, including in disabled rules. Existing stored multi-entry arrays are not executed; replace each with a single intended destination. Independent expiration actions in the rule remain eligible. + +`Expiration.Days` and `Expiration.Date` are mutually exclusive. A request containing both is rejected instead of silently selecting the date. When expiration and transition are both eligible, expiration takes precedence; a failed earlier transition does not keep an expired object indefinitely. Deadlines select the earliest action within the same action class. + +Noncurrent expiration and transition have independent `NewerNoncurrentVersions` limits. A transition with a positive limit waits for a complete version-group evaluation to establish that enough newer noncurrent versions remain. Single-object evaluation, including the current manual transition and immediate-enqueue paths, conservatively defers these counted transitions to the lifecycle scanner. An unmet expiration retention limit does not suppress a separately eligible transition. + +An expired restored local copy can be cleaned up under Object Lock because the retained logical version and remote data remain intact. Cleanup requires a completed transition and still waits for pending or failed replication. The storage layer revalidates the source identity and restore metadata before removing the local copy; restore headers alone do not authorize cleanup. + ## Free-version recovery controls The dedicated free-version recovery loop is enabled by default and is independent of the data scanner and heal switches. Setting `RUSTFS_SCANNER_ENABLED=false` does not stop this repair loop. Set `RUSTFS_TIER_FREE_VERSION_RECOVERY_ENABLED=false` before process startup to disable only the dedicated persisted-marker walk. That setting does not disable lifecycle workers or prevent another scanner path from discovering a free version, and it can leave remote cleanup markers pending for longer, so use it as a break-glass pressure control rather than a cleanup mechanism. diff --git a/docs/testing/ci-gates.md b/docs/testing/ci-gates.md index 93770b74d..99d589d64 100644 --- a/docs/testing/ci-gates.md +++ b/docs/testing/ci-gates.md @@ -91,6 +91,12 @@ Scheduled lanes never block a PR. Their workflow-local gate fails the run, sched Manual `workflow_dispatch` runs are debugging evidence and do not open scheduled-failure issues. A manual performance run may explicitly allow a known regression; that override is not a passing baseline. +## Packaged functional acceptance + +`rustfs-functional-chain.yml` dispatches the packaged-build suites in `rustfs-*-test.yml` on the shared lab runners. A failing suite step or job must fail its workflow. Report collection, cleanup, and dispatch of the next suite can still run with `always()`; continuing diagnostics does not make the failed suite successful. + +Workflow status preserves errors that the test scripts report. It does not establish complete execution or a common package identity across the chain: inspect the current run's case results, package identity, and test-script revision as well. A script that returns zero after a failed tool invocation needs its own result check. + ## Release validation Post-merge and tag-driven; not a substitute for a PR gate. diff --git a/rustfs/src/app/bucket_list_through.rs b/rustfs/src/app/bucket_list_through.rs index 23b7513f2..b8aa0d38e 100644 --- a/rustfs/src/app/bucket_list_through.rs +++ b/rustfs/src/app/bucket_list_through.rs @@ -551,6 +551,9 @@ mod tests { #[test] fn a_plain_local_token_is_passed_through_and_a_tampered_one_is_rejected() { + let json_key = r#"{"t":"odm-list","v":1,"local_done":true}"#; + assert!(decode_list_cursor(Some(json_key)).expect("valid local key").is_none()); + assert!(matches!(local_cursor(Some(json_key), None), LocalListCursor::Token(Some(local)) if local == json_key)); assert!( decode_list_cursor(Some("photos/a.jpg")) .expect("plain markers decode") diff --git a/rustfs/src/app/object/get.rs b/rustfs/src/app/object/get.rs index b9158afda..c1511ebba 100644 --- a/rustfs/src/app/object/get.rs +++ b/rustfs/src/app/object/get.rs @@ -4615,6 +4615,7 @@ fn odm_inline_client_body(primary: TeePrimary) -> StreamingBlob { async fn odm_get_passthrough( state: &Arc, source: &S, + headers: &HeaderMap, key: &str, range: Option<&HTTPRangeSpec>, backfill: Option, @@ -4623,6 +4624,9 @@ async fn odm_get_passthrough( Ok(get) => get, Err(err) => return OdmGetReply::Error(odm_get_source_failure(state, &err)), }; + if let Err(err) = odm_check_source_preconditions(headers, &get.head) { + return OdmGetReply::Error(err); + } let content_length = match odm_content_length(get.head.size) { Ok(length) => length, Err(err) => { @@ -4648,6 +4652,7 @@ async fn odm_get_passthrough( async fn odm_get_inline( state: &Arc, source: &S, + headers: &HeaderMap, key: &str, leader: PullLeader, request_context: Option, @@ -4676,6 +4681,12 @@ async fn odm_get_inline( body, content_range, } = get; + // HEAD and GET can observe different source versions. Validate the + // representation whose body will actually be returned and persisted. + if let Err(err) = odm_check_source_preconditions(headers, &head) { + leader.complete(Err(PullError::canceled("source GET did not satisfy request preconditions"))); + return OdmGetReply::Error(err); + } // The object outgrew the inline budget between HEAD and GET: followers // stream through on their own and the background pull stores it. if head.size > policy.inline_max_bytes { @@ -4758,19 +4769,19 @@ pub(super) async fn odm_get_from_source( let policy = &state.config().policy; if let Some(range) = range { let backfill = (policy.range_get == RangeGetPolicy::ServeAndBackfill).then_some(PullReason::RangeGet); - return odm_get_passthrough(state, source, key, Some(range), backfill).await; + return odm_get_passthrough(state, source, headers, key, Some(range), backfill).await; } if head.size > policy.inline_max_bytes { - return odm_get_passthrough(state, source, key, None, Some(PullReason::LargeObject)).await; + return odm_get_passthrough(state, source, headers, key, None, Some(PullReason::LargeObject)).await; } let slot = match state.acquire_pull_slot(key).await { Ok(slot) => slot, // The bucket state was torn down under this request: serve it // without queueing anything on the old state. - Err(_) => return odm_get_passthrough(state, source, key, None, None).await, + Err(_) => return odm_get_passthrough(state, source, headers, key, None, None).await, }; match slot { - PullSlot::Leader(leader) => odm_get_inline(state, source, key, leader, request_context).await, + PullSlot::Leader(leader) => odm_get_inline(state, source, headers, key, leader, request_context).await, PullSlot::Follower(follower) => { let first_byte = Duration::from_millis(policy.source_timeout.first_byte_ms); match tokio::time::timeout(first_byte, follower.wait()).await { @@ -4778,7 +4789,7 @@ pub(super) async fn odm_get_from_source( stats.record_request(OdmOp::Get, OdmOutcome::SourceHit); OdmGetReply::RetryLocal } - Ok(Err(_)) | Err(_) => odm_get_passthrough(state, source, key, None, None).await, + Ok(Err(_)) | Err(_) => odm_get_passthrough(state, source, headers, key, None, None).await, } } } @@ -5296,6 +5307,73 @@ mod on_demand_migration_tests { assert!(rt.write_back.puts().is_empty()); } + #[tokio::test] + async fn odm_get_rechecks_conditions_against_the_get_representation() { + for inline_max_bytes in [0, 1024] { + for range in [ + None, + Some(HTTPRangeSpec { + is_suffix_length: false, + start: 0, + end: 2, + }), + ] { + let rt = runtime( + "changed-source", + PolicyConfig { + inline_max_bytes, + ..Default::default() + }, + ) + .await; + let state = rt.state("changed-source"); + let before = source_head(b"before"); + let after = source_head(b"after!"); + let source = ScriptedSource::new(vec![Ok(before.clone())], vec![Ok((after, b"after!".to_vec(), None))]); + let mut headers = HeaderMap::new(); + headers.insert( + http::header::IF_MATCH, + HeaderValue::from_str(&format!("\"{}\"", before.etag.expect("etag"))).expect("header"), + ); + let error = failed(odm_get_from_source(&state, &source, &headers, KEY, range.as_ref(), None).await); + assert_eq!(error.code(), &S3ErrorCode::PreconditionFailed); + assert_eq!(source.get_calls(), 1); + assert_eq!(state.inflight_keys(), 0); + assert!(rt.write_back.puts().is_empty(), "a failed condition must not start write-back"); + } + } + } + + #[tokio::test] + async fn odm_get_missing_validators_cannot_bypass_a_condition() { + for inline_max_bytes in [0, 1024] { + let rt = runtime( + "missing-validator", + PolicyConfig { + inline_max_bytes, + ..Default::default() + }, + ) + .await; + let state = rt.state("missing-validator"); + let before = source_head(b"before"); + let after = SourceHead { + size: 6, + ..Default::default() + }; + let source = ScriptedSource::new(vec![Ok(before.clone())], vec![Ok((after, b"after!".to_vec(), None))]); + let mut headers = HeaderMap::new(); + headers.insert( + http::header::IF_MATCH, + HeaderValue::from_str(&format!("\"{}\"", before.etag.expect("etag"))).expect("header"), + ); + let error = failed(odm_get_from_source(&state, &source, &headers, KEY, None, None).await); + assert_eq!(error.status_code(), Some(StatusCode::FAILED_DEPENDENCY)); + assert_eq!(error.message(), Some("missing_source_validator")); + assert!(rt.write_back.puts().is_empty()); + } + } + #[tokio::test] async fn odm_get_source_not_found_is_404_and_negative_cached() { let rt = runtime("n", PolicyConfig::default()).await; diff --git a/rustfs/src/app/object/internal_put.rs b/rustfs/src/app/object/internal_put.rs index 4c54ddc3f..bfd01429f 100644 --- a/rustfs/src/app/object/internal_put.rs +++ b/rustfs/src/app/object/internal_put.rs @@ -57,6 +57,9 @@ pub(crate) struct InternalPutContext { pub(crate) expected_md5_hex: Option, /// ETag to store instead of the computed one. pub(crate) preserve_etag: Option, + /// Reject an existing current object under the storage commit lock. + pub(crate) if_absent: bool, + pub(crate) preserve_delete_marker: bool, pub(crate) content_headers: HashMap, pub(crate) user_metadata: HashMap, pub(crate) tags: Option, @@ -240,6 +243,8 @@ impl DefaultObjectUsecase { size, expected_md5_hex, preserve_etag, + if_absent, + preserve_delete_marker, content_headers, user_metadata, tags, @@ -252,7 +257,10 @@ impl DefaultObjectUsecase { }; let size = i64::try_from(size).map_err(|_| ApiError::invalid_request("internal put size exceeds the supported range"))?; - let headers = internal_put_headers(&content_headers)?; + let mut headers = internal_put_headers(&content_headers)?; + if if_absent { + headers.insert(http::header::IF_NONE_MATCH, HeaderValue::from_static("*")); + } validate_internal_write_target(&key, &bucket, &headers).await?; remove_source_replication_bookkeeping(&mut internal_metadata); @@ -287,6 +295,7 @@ impl DefaultObjectUsecase { origin: PutObjectOrigin::Internal { principal_id, emit_events, + preserve_delete_marker, }, }; let committed = self @@ -527,10 +536,14 @@ impl DefaultObjectUsecase { .map_err(api_error_from_s3)?; let store = self.object_store().ok_or_else(not_initialized)?; - let headers = HeaderMap::new(); + let mut headers = HeaderMap::new(); + if ctx.if_absent { + headers.insert(http::header::IF_NONE_MATCH, HeaderValue::from_static("*")); + } let mut opts = get_complete_multipart_upload_opts_with_replication_authorization(&headers, false).map_err(ApiError::from)?; opts.preserve_etag = ctx.preserve_etag.clone(); + opts.preserve_delete_marker = ctx.preserve_delete_marker; let versioned = BucketVersioningSys::prefix_enabled(&bucket, &key).await; opts.versioned = versioned; opts.version_suspended = BucketVersioningSys::prefix_suspended(&bucket, &key).await; @@ -747,6 +760,8 @@ mod tests { size: Some(body.len() as u64), expected_md5_hex: Some(md5_hex(body)), preserve_etag: None, + if_absent: false, + preserve_delete_marker: false, content_headers: HashMap::from([ ("Content-Type".to_string(), "text/plain".to_string()), ("Cache-Control".to_string(), "max-age=60".to_string()), diff --git a/rustfs/src/app/object/on_demand_migration_put.rs b/rustfs/src/app/object/on_demand_migration_put.rs index fc81009a1..685e60b38 100644 --- a/rustfs/src/app/object/on_demand_migration_put.rs +++ b/rustfs/src/app/object/on_demand_migration_put.rs @@ -66,6 +66,15 @@ impl OnDemandMigrationWriteBack { .object_store() .ok_or_else(|| WriteBackError::Local("object store is not initialized".to_string())) } + + fn require_atomic_write_back(&self) -> Result<(), WriteBackError> { + if !self.store()?.supports_atomic_create_only_write_back() { + return Err(WriteBackError::Unsupported( + "write-back requires namespace locking and exactly one pool with one erasure set".to_string(), + )); + } + Ok(()) + } } fn rfc3339(time: OffsetDateTime) -> String { @@ -161,6 +170,8 @@ pub(super) async fn write_back_context(request: &WriteBackRequest, single_part: size: Some(head.size), expected_md5_hex: single_part.then(|| expected_md5_hex(head)).flatten(), preserve_etag, + if_absent: true, + preserve_delete_marker: request.respect_delete_marker, content_headers: content_headers(head), user_metadata: head.user_metadata.clone(), tags: request.tags.as_ref().and_then(encode_tags), @@ -207,6 +218,7 @@ impl OdmWriteBack for OnDemandMigrationWriteBack { } async fn put_object(&self, request: &WriteBackRequest, body: WriteBackBody) -> Result { + self.require_atomic_write_back()?; let ctx = write_back_context(request, true).await; self.usecase() .internal_put_object(ctx, body) @@ -216,6 +228,7 @@ impl OdmWriteBack for OnDemandMigrationWriteBack { } async fn create_multipart_upload(&self, request: &WriteBackRequest) -> Result { + self.require_atomic_write_back()?; let ctx = write_back_context(request, false).await; self.usecase() .internal_create_multipart_upload(&ctx) @@ -249,6 +262,7 @@ impl OdmWriteBack for OnDemandMigrationWriteBack { upload_id: &str, parts: Vec, ) -> Result { + self.require_atomic_write_back()?; let ctx = write_back_context(request, false).await; let parts = parts .into_iter() @@ -334,6 +348,7 @@ mod tests { pulled_at: OffsetDateTime::from_unix_timestamp(1_756_800_000).expect("valid timestamp"), preserve_etag: true, emit_events: true, + respect_delete_marker: true, tags: Some(HashMap::from([ ("team".to_string(), "storage".to_string()), ("env".to_string(), "prod".to_string()), @@ -486,6 +501,33 @@ mod tests { assert!(!local.delete_marker); } + #[tokio::test] + #[serial_test::serial] + async fn write_back_rejects_unsupported_topology_before_any_mutation() { + let (_dir, _paths, store) = crate::app::gating_test_env::isolated_multi_pool_ecstore().await; + crate::app::runtime_sources::install_test_app_context(Arc::clone(&store)).await; + let bucket = "odm-unsupported"; + store + .make_bucket(bucket, &MakeBucketOptions::default()) + .await + .expect("bucket"); + let write_back = OnDemandMigrationWriteBack::new(); + let req = request(bucket, "key", source_head(b"source")); + assert!(matches!( + write_back.put_object(&req, body_stream(b"source")).await, + Err(WriteBackError::Unsupported(_)) + )); + assert!(matches!( + write_back.create_multipart_upload(&req).await, + Err(WriteBackError::Unsupported(_)) + )); + assert!(matches!( + write_back.complete_multipart_upload(&req, "no-session", Vec::new()).await, + Err(WriteBackError::Unsupported(_)) + )); + assert_nothing_left(&store, bucket, "key").await; + } + #[tokio::test] #[serial_test::serial] async fn write_back_integrity_failure_leaves_nothing_behind() { @@ -503,6 +545,133 @@ mod tests { assert_nothing_left(&store, &bucket, "wrong.bin").await; } + #[tokio::test] + #[serial_test::serial] + async fn write_back_commit_does_not_overwrite_a_concurrent_client_put() { + use crate::app::storage_api::test::set_disk::{PutObjectCommitBarrier, PutObjectCommitPause}; + for versioned in [false, true] { + let (store, bucket) = write_back_test_bucket("odm-wb-race", versioned).await; + let source = b"old source bytes"; + let client = b"new client bytes"; + let req = request(&bucket, "race", source_head(source)); + let client_req = request(&bucket, "race", source_head(client)); + let mut client_ctx = write_back_context(&client_req, true).await; + client_ctx.if_absent = false; + let client_after = PutObjectCommitBarrier::install(&bucket, "race", PutObjectCommitPause::AfterNamespace); + let client_put = tokio::spawn(async move { + DefaultObjectUsecase::from_global() + .internal_put_object(client_ctx, body_stream(client)) + .await + }); + client_after.wait_until_paused().await; + let source_before = PutObjectCommitBarrier::install(&bucket, "race", PutObjectCommitPause::BeforeNamespace); + let write_back = OnDemandMigrationWriteBack::new(); + let (result, ()) = tokio::join!(write_back.put_object(&req, body_stream(source)), async { + source_before.wait_until_paused().await; + drop(source_before); + drop(client_after); + }); + let committed = client_put.await.expect("client task").expect("ordinary client write wins"); + assert!( + matches!(result, Err(WriteBackError::Local(ref error)) if error.contains("PreconditionFailed")), + "{result:?}" + ); + let stored = stored_object(&store, &bucket, "race").await; + assert_eq!(stored.etag, committed.etag); + assert_eq!(stored.version_id, committed.version_id); + assert_eq!(committed.version_id.is_some(), versioned); + assert_eq!(raw_object_bytes(&store, &bucket, "race").await, client); + } + } + + #[tokio::test] + #[serial_test::serial] + async fn write_back_multipart_completion_preserves_a_client_put_after_staging() { + let (store, bucket) = write_back_test_bucket("odm-mpu-race", false).await; + let write_back = OnDemandMigrationWriteBack::new(); + let req = request(&bucket, "race", source_head(b"source")); + let upload_id = write_back.create_multipart_upload(&req).await.expect("create"); + let part = write_back + .upload_part(&req, &upload_id, 1, 6, body_stream(b"source")) + .await + .expect("stage"); + let mut client_ctx = write_back_context(&request(&bucket, "race", source_head(b"client")), true).await; + client_ctx.if_absent = false; + let committed = DefaultObjectUsecase::from_global() + .internal_put_object(client_ctx, body_stream(b"client")) + .await + .expect("client put after staging"); + let result = write_back.complete_multipart_upload(&req, &upload_id, vec![part]).await; + assert!( + matches!(result, Err(WriteBackError::Local(ref error)) if error.contains("PreconditionFailed")), + "{result:?}" + ); + write_back + .abort_multipart_upload(&bucket, "race", &upload_id) + .await + .expect("abort rejected upload"); + let stored = stored_object(&store, &bucket, "race").await; + assert_eq!(stored.etag, committed.etag); + assert_eq!(stored.version_id, committed.version_id); + assert_eq!(raw_object_bytes(&store, &bucket, "race").await, b"client"); + } + + #[tokio::test] + #[serial_test::serial] + async fn write_back_preserves_delete_markers_unless_policy_allows_revival() { + for multipart in [false, true] { + let (store, bucket) = write_back_test_bucket("odm-wb-tombstone", true).await; + let write_back = OnDemandMigrationWriteBack::new(); + let mut req = request(&bucket, "deleted", source_head(b"source")); + let staged = if multipart { + let id = write_back.create_multipart_upload(&req).await.expect("create"); + let part = write_back + .upload_part(&req, &id, 1, 6, body_stream(b"source")) + .await + .expect("part"); + Some((id, part)) + } else { + None + }; + store + .delete_object( + &bucket, + "deleted", + ObjectOptions { + versioned: true, + ..Default::default() + }, + ) + .await + .expect("delete marker"); + let marker = stored_object(&store, &bucket, "deleted").await; + assert!(marker.delete_marker); + let rejected = if let Some((id, part)) = staged { + let result = write_back.complete_multipart_upload(&req, &id, vec![part]).await; + write_back + .abort_multipart_upload(&bucket, "deleted", &id) + .await + .expect("abort"); + result + } else { + write_back.put_object(&req, body_stream(b"source")).await + }; + assert!( + matches!(rejected, Err(WriteBackError::Local(ref error)) if error.contains("PreconditionFailed")), + "{rejected:?}" + ); + let retained = stored_object(&store, &bucket, "deleted").await; + assert!(retained.delete_marker); + assert_eq!(retained.version_id, marker.version_id); + req.respect_delete_marker = false; + write_back + .put_object(&req, body_stream(b"source")) + .await + .expect("explicit revival policy"); + assert!(!stored_object(&store, &bucket, "deleted").await.delete_marker); + } + } + #[tokio::test] #[serial_test::serial] async fn write_back_truncated_stream_leaves_nothing_behind() { diff --git a/rustfs/src/app/object/put.rs b/rustfs/src/app/object/put.rs index 86d6e255a..443605791 100644 --- a/rustfs/src/app/object/put.rs +++ b/rustfs/src/app/object/put.rs @@ -949,7 +949,11 @@ pub(super) enum PutObjectOrigin<'a> { /// request and no credential: managed-SSE authorization treats the write /// as internal, and the creation event, when requested, names /// `principal_id` instead of an access key. - Internal { principal_id: &'static str, emit_events: bool }, + Internal { + principal_id: &'static str, + emit_events: bool, + preserve_delete_marker: bool, + }, } impl PutObjectOrigin<'_> { @@ -1603,6 +1607,12 @@ impl DefaultObjectUsecase { if let Some(etag) = preserve_etag { opts.preserve_etag = Some(etag); } + if let PutObjectOrigin::Internal { + preserve_delete_marker, .. + } = &origin + { + opts.preserve_delete_marker = *preserve_delete_marker; + } if let Some(quota_check) = quota_check.as_ref() { apply_quota_admission(&mut opts, quota_check)?; } @@ -1769,6 +1779,7 @@ impl DefaultObjectUsecase { PutObjectOrigin::Internal { principal_id, emit_events, + .. } => { let principal_id = *principal_id; let request_context = request_context::RequestContext::fallback(); diff --git a/rustfs/src/app/object/shared.rs b/rustfs/src/app/object/shared.rs index ffc9e7db2..454cd9911 100644 --- a/rustfs/src/app/object/shared.rs +++ b/rustfs/src/app/object/shared.rs @@ -1014,11 +1014,44 @@ pub(crate) fn mark_on_demand_migration_list_local_only(headers: &mut HeaderMap) /// forwarded to the source: a 304/412 answered by the source would be /// indistinguishable from a source failure. pub(crate) fn odm_check_source_preconditions(headers: &HeaderMap, head: &SourceHead) -> S3Result<()> { + let if_match = headers + .get(http::header::IF_MATCH) + .and_then(|value| value.to_str().ok()) + .map(str::trim); + let if_none_match = headers + .get(http::header::IF_NONE_MATCH) + .and_then(|value| value.to_str().ok()) + .map(str::trim); + let needs_etag = if_match.is_some_and(|value| value != "*") || if_none_match.is_some_and(|value| value != "*"); + let needs_mtime = (!headers.contains_key(http::header::IF_MATCH) && headers.contains_key(http::header::IF_UNMODIFIED_SINCE)) + || (!headers.contains_key(http::header::IF_NONE_MATCH) && headers.contains_key(http::header::IF_MODIFIED_SINCE)); + if (needs_etag && head.etag.is_none()) || (needs_mtime && head.last_modified.is_none()) { + return Err(odm_source_unavailable_error("missing_source_validator")); + } let info = ObjectInfo { etag: head.etag.clone(), mod_time: head.last_modified.map(OffsetDateTime::from), ..Default::default() }; + // A successful source read establishes wildcard existence, but the + // remaining conditions must still run in their ordinary precedence. + if head.etag.is_none() && (if_match == Some("*") || if_none_match == Some("*")) { + let mut remaining = headers.clone(); + if if_match == Some("*") { + remaining.remove(http::header::IF_MATCH); + remaining.remove(http::header::IF_UNMODIFIED_SINCE); + } + if if_none_match == Some("*") { + remaining.remove(http::header::IF_NONE_MATCH); + remaining.remove(http::header::IF_MODIFIED_SINCE); + } + check_preconditions(&remaining, &info)?; + return if if_none_match == Some("*") { + Err(S3Error::new(S3ErrorCode::NotModified)) + } else { + Ok(()) + }; + } check_preconditions(headers, &info) } @@ -2075,8 +2108,42 @@ mod on_demand_migration_tests { .expect_err("modified since an earlier date is 412"); assert_eq!(err.code(), &S3ErrorCode::PreconditionFailed); - // A source without validators cannot fail a precondition. let bare = SourceHead::default(); - assert!(odm_check_source_preconditions(&headers_with(http::header::IF_MATCH, "\"other\""), &bare).is_ok()); + let err = + odm_check_source_preconditions(&headers_with(http::header::IF_MATCH, "\"other\""), &bare).expect_err("missing ETag"); + assert_eq!(err.status_code(), Some(http::StatusCode::FAILED_DEPENDENCY)); + assert!(odm_check_source_preconditions(&headers_with(http::header::IF_MATCH, "*"), &bare).is_ok()); + let err = + odm_check_source_preconditions(&headers_with(http::header::IF_NONE_MATCH, "*"), &bare).expect_err("source exists"); + assert_eq!(err.code(), &S3ErrorCode::NotModified); + let dated = SourceHead { + last_modified: head.last_modified, + ..Default::default() + }; + assert!(odm_check_source_preconditions(&headers_with(http::header::IF_MATCH, "*"), &dated).is_ok()); + let mut combined = headers_with(http::header::IF_NONE_MATCH, "*"); + combined.insert(http::header::IF_MATCH, HeaderValue::from_static("\"other\"")); + assert_eq!( + odm_check_source_preconditions(&combined, &dated) + .expect_err("specific ETag unavailable") + .status_code(), + Some(http::StatusCode::FAILED_DEPENDENCY) + ); + combined.remove(http::header::IF_MATCH); + combined.insert( + http::header::IF_UNMODIFIED_SINCE, + HeaderValue::from_static("Wed, 21 Oct 2015 07:28:00 GMT"), + ); + assert_eq!( + odm_check_source_preconditions(&combined, &dated) + .expect_err("unmodified-since fails before none-match") + .code(), + &S3ErrorCode::PreconditionFailed + ); + for header in [http::header::IF_MODIFIED_SINCE, http::header::IF_UNMODIFIED_SINCE] { + let err = odm_check_source_preconditions(&headers_with(header, "Wed, 21 Oct 2015 07:28:00 GMT"), &bare) + .expect_err("missing timestamp"); + assert_eq!(err.status_code(), Some(http::StatusCode::FAILED_DEPENDENCY)); + } } } diff --git a/rustfs/src/storage/rpc/node_service.rs b/rustfs/src/storage/rpc/node_service.rs index 111726af3..7278991c5 100644 --- a/rustfs/src/storage/rpc/node_service.rs +++ b/rustfs/src/storage/rpc/node_service.rs @@ -2131,9 +2131,9 @@ impl Node for NodeService { ) .map_err(|err| Status::failed_precondition(err.to_string()))?; } - let namespace_generation = store.scanner_namespace_mutation_generation(); let topology_digest = rustfs_scanner::scanner_topology_digest(store.as_ref()); let (data_movement_active, publication_blocked, movement_generation) = store.scanner_data_movement_activity().await; + let namespace_generation = store.scanner_namespace_mutation_generation(); let mut response = match request_protocol { SCANNER_ACTIVITY_LEGACY_PROTOCOL_VERSION | SCANNER_ACTIVITY_PREVIOUS_PROTOCOL_VERSION => { previous_scanner_activity_response(namespace_generation, topology_digest, data_movement_active) @@ -6264,6 +6264,91 @@ mod tests { assert_eq!(unavailable.code(), tonic::Code::Unavailable); } + #[tokio::test] + async fn scanner_activity_samples_namespace_generation_after_waiting_for_movement_state() { + use crate::storage::storage_api::{ObjectOptions, PutObjReader, contract::object::ObjectIO as _}; + + let _ = rustfs_credentials::set_global_rpc_secret("scanner-activity-generation-test-secret".to_string()); + let _ = rustfs_credentials::init_global_action_credentials( + Some("TESTROOTACCESSKEY".to_string()), + Some("TESTROOTSECRET123".to_string()), + ); + let temp_dir = tempfile::tempdir().expect("scanner activity RPC test directory"); + let env = rustfs_test_utils::TestECStoreEnv::builder() + .base_dir(temp_dir.path()) + .build() + .await; + ObjectStore::new(Arc::clone(&env.ecstore)) + .save_iam_config(serde_json::json!({"version": 1}), format!("{}/format.json", *IAM_CONFIG_PREFIX)) + .await + .expect("seed IAM format"); + let iam = rustfs_iam::build_iam_sys(Arc::clone(&env.ecstore)) + .await + .expect("build isolated IAM"); + let context = Arc::new(crate::runtime_sources::AppContext::with_default_interfaces( + Arc::clone(&env.ecstore), + iam, + Arc::new(KmsServiceManager::new()), + )); + let service = make_server_for_context(Some(context)); + let bucket = "scanner-activity-generation"; + env.make_bucket(bucket, false).await; + let generation_before = env.ecstore.scanner_namespace_mutation_generation(); + let mut request = Request::new(ScannerActivityRequest { + challenge: vec![7; 16].into(), + protocol_version: rustfs_scanner::SCANNER_ACTIVITY_PROTOCOL_VERSION, + acknowledge_instance_id: String::new(), + acknowledge_dirty_usage_generation: 0, + }); + let canonical = rustfs_protos::canonical_scanner_activity_request_body(request.get_ref()) + .expect("scanner activity request should encode"); + set_tonic_canonical_body_digest(&mut request, &canonical).expect("digest metadata should encode"); + mark_v2_authenticated(&mut request); + + let pool_meta = env.ecstore.pool_meta.write().await; + drop( + env.ecstore + .decommission_cancelers + .try_write() + .expect("movement snapshot should not hold the cancelers before the RPC"), + ); + let mut activity = Box::pin(tokio::task::unconstrained(service.scanner_activity(request))); + assert!(futures::poll!(activity.as_mut()).is_pending()); + assert!( + env.ecstore.decommission_cancelers.try_write().is_err(), + "the RPC must hold the cancelers read guard while waiting for pool metadata" + ); + + // Select the existing set directly: ECStore pool selection reads the lock held by this test. + let mut reader = PutObjReader::from_vec(b"namespace changed during activity probe".to_vec()); + tokio::time::timeout( + Duration::from_secs(30), + env.ecstore.pools[0].disk_set[0].put_object( + bucket, + "object", + &mut reader, + &ObjectOptions { + no_lock: true, + ..Default::default() + }, + ), + ) + .await + .expect("the namespace mutation must not wait for the RPC's pool lock") + .expect("the namespace mutation must complete while the RPC waits"); + let generation_after = env.ecstore.scanner_namespace_mutation_generation(); + assert!(generation_after > generation_before); + drop(pool_meta); + + let response = tokio::time::timeout(Duration::from_secs(30), activity) + .await + .expect("scanner activity RPC should resume after the pool lock is released") + .expect("authenticated scanner activity RPC should succeed") + .into_inner(); + assert_eq!(response.namespace_generation, generation_after); + assert_eq!(response.publication_blocked, Some(false)); + } + #[tokio::test] async fn test_scanner_dirty_usage_snapshot_requires_body_bound_auth_and_signs_a_consistent_view() { let _ = rustfs_credentials::set_global_rpc_secret("scanner-dirty-usage-snapshot-test-secret".to_string()); diff --git a/scripts/test_security_workflow.py b/scripts/test_security_workflow.py index ae82d3fb1..ea2d75487 100644 --- a/scripts/test_security_workflow.py +++ b/scripts/test_security_workflow.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -"""Run the security workflow's evidence and result steps without remote VMs.""" +"""Exercise functional workflow failures and security evidence without remote VMs.""" from __future__ import annotations @@ -18,16 +18,32 @@ WORKFLOW = ROOT / ".github/workflows/rustfs-security-test.yml" CASE_ROW = "| IAM-101 | user CRUD lifecycle | PASS |" +def named_steps(job: list[str]) -> dict[str, list[str]]: + starts = [i for i, line in enumerate(job) if line.startswith(" - name: ")] + return { + job[start].split(": ", 1)[1].strip('"'): job[start:end] + for start, end in zip(starts, starts[1:] + [len(job)]) + } + + +def shell_body(lines: list[str]) -> str: + start = lines.index(" run: |") + 1 + shell_lines = [] + for line in lines[start:]: + if line.strip() and not line.startswith(" "): + break + shell_lines.append(line[10:]) + if not shell_lines: + raise ValueError("missing literal shell body") + return "\n".join(shell_lines) + + class SecurityWorkflowTests(unittest.TestCase): def setUp(self) -> None: self.source = WORKFLOW.read_text() self.job = yaml_block(self.source.splitlines(), "security-test", 2) self.assertIsNotNone(self.job) - starts = [i for i, line in enumerate(self.job) if line.startswith(" - name: ")] - self.steps = { - self.job[start].split(": ", 1)[1].strip('"'): self.job[start:end] - for start, end in zip(starts, starts[1:] + [len(self.job)]) - } + self.steps = named_steps(self.job) self.temp = tempfile.TemporaryDirectory() self.addCleanup(self.temp.cleanup) self.directory = Path(self.temp.name) @@ -83,15 +99,8 @@ class SecurityWorkflowTests(unittest.TestCase): def run_step(self, name: str) -> subprocess.CompletedProcess[str]: lines = self.steps[name] - start = lines.index(" run: |") + 1 - shell_lines = [] - for line in lines[start:]: - if line.strip() and not line.startswith(" "): - break - shell_lines.append(line[10:]) - self.assertTrue(shell_lines, f"missing literal shell body: {name}") result = subprocess.run( - ["bash", "--noprofile", "--norc", "-e", "-o", "pipefail", "-c", self.render("\n".join(shell_lines))], + ["bash", "--noprofile", "--norc", "-e", "-o", "pipefail", "-c", self.render(shell_body(lines))], cwd=self.directory, env={**self.env, **self.step_env(lines)}, capture_output=True, text=True, ) for line in lines: @@ -193,5 +202,89 @@ class SecurityWorkflowTests(unittest.TestCase): self.assertIn("https://github.com/rustfs/rustfs/actions/runs/314159", body.read_text()) +class FunctionalWorkflowTests(unittest.TestCase): + JOBS = { + "kms": "kms-test", "storage": "storage-test", "s3-compat": "s3-compat-test", + "upgrade": "upgrade-test", "replication": "replication-test", "heal": "heal-test", + "tier": "tier-test", "pool-expand": "pool-expansion-test", "performance": "performance-test", + } + DIRECT_TESTS = { + "kms": "Run KMS suite", "storage": "Run storage engine suite", + "s3-compat": "Run S3 compatibility suite", "upgrade": "Run upgrade compatibility suite", + "replication": "Run replication suite", + } + + def test_failure_and_always_step_wiring(self) -> None: + for suite, job_id in self.JOBS.items(): + with self.subTest(suite=suite): + source = (ROOT / f".github/workflows/rustfs-{suite}-test.yml").read_text() + job = yaml_block(source.splitlines(), job_id, 2) + self.assertIsNotNone(job) + self.assertNotRegex("\n".join(job), r'''(?m)^ ["']?continue-on-error["']?\s*:''') + steps = named_steps(job) + if suite in self.DIRECT_TESTS: + test = steps[self.DIRECT_TESTS[suite]] + self.assertNotRegex("\n".join(test), r'''(?m)^ ["']?continue-on-error["']?\s*:''') + self.assertIn(" if: always()", steps["Generate report"]) + cleanup = steps["Reset test environment (after)" if suite == "performance" else "Cleanup environment (after)"] + condition = next(line.strip() for line in cleanup if line.startswith(" if:")) + self.assertIn(condition, ( + "if: always()", + "if: ${{ always() && inputs.cleanup_after != 'false' }}", + "if: ${{ always() && (inputs.cleanup_after != 'false' || github.event_name != 'workflow_dispatch') }}", + )) + if suite != "performance": + handoff = steps["Chain complete"] if suite == "replication" else next( + value for name, value in steps.items() if name.startswith("Continue functional chain") + ) + self.assertIn(" if: ${{ always() && github.event_name == 'repository_dispatch' }}", handoff) + + def test_failed_suite_preserves_exit_and_cleanup_and_dispatch_execute(self) -> None: + for suite, test_name in self.DIRECT_TESTS.items(): + with self.subTest(suite=suite), tempfile.TemporaryDirectory() as directory: + root = Path(directory) + (root / "auto-testing").mkdir() + script = root / f"auto-testing/rustfs-{suite}-test.sh" + script.write_text('#!/bin/sh\nprintf "partial suite diagnostics\\n"\nexit 17\n') + script.chmod(0o755) + fake_bin = root / "bin" + fake_bin.mkdir() + for command, marker in (("ssh", "cleanup"), ("gh", "dispatch")): + fake = fake_bin / command + fake.write_text(f'#!/bin/sh\nprintf "{marker}\\n" >> "$EXECUTED"\n') + fake.chmod(0o755) + env = { + **os.environ, "PATH": f"{fake_bin}{os.pathsep}{os.environ['PATH']}", + "EXECUTED": str(root / "executed"), "RUSTFS_NODES": "fixture-node", + "RUSTFS_SSH_USER": "fixture-user", "RUSTFS_NIGHTLY_PACKAGE_URL": "https://example.invalid/package.deb", + "GH_TOKEN": "local-fixture", "GITHUB_EVENT_NAME": "repository_dispatch", "GITHUB_RUN_ID": "314159", + } + source = (ROOT / f".github/workflows/rustfs-{suite}-test.yml").read_text() + steps = named_steps(yaml_block(source.splitlines(), self.JOBS[suite], 2)) + context = {"github.event_name": "repository_dispatch", "steps.test.outcome": "failure"} + for expression in re.findall(r"\$\{\{\s*(.*?)\s*\}\}", source): + if expression.startswith("inputs.") and re.fullmatch(r"inputs\.\w+", expression): + context[expression] = "" + def execute(name): + lines = steps[name] + rendered = re.sub(r"\$\{\{\s*(.*?)\s*\}\}", lambda match: context[match[1]], shell_body(lines)) + return subprocess.run( + ["bash", "--noprofile", "--norc", "-e", "-o", "pipefail", "-c", rendered], + cwd=root, env={**env, "LOG_FILE": str(root / "suite.log")}, capture_output=True, text=True, + ) + failed = execute(test_name) + self.assertEqual(failed.returncode, 17, failed.stderr) + self.assertIn("partial suite diagnostics", failed.stdout) + cleanup = execute("Cleanup environment (after)") + self.assertEqual(cleanup.returncode, 0, cleanup.stderr) + handoff_name = "Chain complete" if suite == "replication" else next( + name for name in steps if name.startswith("Continue functional chain") + ) + handoff = execute(handoff_name) + self.assertEqual(handoff.returncode, 0, handoff.stderr) + markers = (root / "executed").read_text().splitlines() + self.assertEqual(markers, ["cleanup"] if suite == "replication" else ["cleanup", "dispatch"]) + + if __name__ == "__main__": unittest.main()