From 4786463ad3c095600d29ff09ee5c7338927d99d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=94=90=E5=B0=8F=E9=B8=AD?= Date: Sat, 15 Aug 2026 19:42:27 +0800 Subject: [PATCH] fix(site-replication): close the second ILM review round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Missed-delete repair: a deleted expiry state now travels through bootstrap/repair as an explicit timestamped lc-config delete item (lifecycle_expiry_statement distinguishes deletion — whole-config write time advanced past the created backfill — from never-configured buckets and from transition-only configs without an expiry axis, which say nothing). A peer that missed the live delete converges on repair; the receiver's staleness guard protects newer peer state. - Strict tombstone recognition: only a well-delimited zero-rule document maps to delete semantics; truncated or foreign payloads that fail the strict deserializer are rejected instead of being treated as a delete that erases local expiry rules. - Staleness fallback axis narrowed: the whole-config write time is used only for deleted or legacy-with-expiry state. A present transition-only config without an expiry axis compares at epoch — its whole-config time moves on transition edits and must not shadow or block independent peer expiry updates and same-timestamp repairs. --- rustfs/src/admin/handlers/site_replication.rs | 303 +++++++++++++++--- 1 file changed, 252 insertions(+), 51 deletions(-) diff --git a/rustfs/src/admin/handlers/site_replication.rs b/rustfs/src/admin/handlers/site_replication.rs index 9d2eb8c52..d9e18618c 100644 --- a/rustfs/src/admin/handlers/site_replication.rs +++ b/rustfs/src/admin/handlers/site_replication.rs @@ -2323,20 +2323,31 @@ fn append_bootstrap_bucket_items( }, )?; if replicate_ilm_expiry { - append_bootstrap_bucket_item( - &mut plan.bucket_items, - bucket, - "lc-config", - bucket.expiry_lc_config.clone(), - bucket.expiry_lc_config_updated_at, - |item, value| { - item.expiry_lc_config = Some(value); - // `updated_at` here is the entry's expiry axis (see the - // SRBucketInfo construction), not the wall clock. - item.expiry_updated_at = item.updated_at; - Ok(()) - }, - )?; + if bucket.expiry_lc_config.is_some() { + append_bootstrap_bucket_item( + &mut plan.bucket_items, + bucket, + "lc-config", + bucket.expiry_lc_config.clone(), + bucket.expiry_lc_config_updated_at, + |item, value| { + item.expiry_lc_config = Some(value); + // `updated_at` here is the entry's expiry axis (see the + // SRBucketInfo construction), not the wall clock. + item.expiry_updated_at = item.updated_at; + Ok(()) + }, + )?; + } else if bucket.expiry_lc_config_updated_at.is_some() { + // Expiry rules were removed at this axis (lifecycle_expiry_statement): + // an explicit timestamped delete item, so a peer that missed the + // live delete converges on bootstrap/repair instead of keeping + // stale expiry rules. The receiver's staleness guard protects a + // peer whose expiry state is newer. + let mut item = bootstrap_bucket_meta_item(bucket, "lc-config", bucket.expiry_lc_config_updated_at); + item.expiry_updated_at = item.updated_at; + plan.bucket_items.push(item); + } } append_bootstrap_bucket_item( &mut plan.bucket_items, @@ -4334,9 +4345,11 @@ async fn build_sr_info(state: &SiteReplicationState, local_peer: &PeerInfo) -> S // Expiry subset only: this entry feeds both the bootstrap/repair // plan (peers must not receive transition rules) and cross-site // consistency views (transition rules are site-local and would - // read as false mismatches). - entry.expiry_lc_config = - lifecycle_expiry_subset_xml(&metadata.lifecycle_config_xml).and_then(|data| raw_config_to_base64(&data)); + // read as false mismatches). A deleted expiry state is a `None` + // value with the deletion's axis so repair can converge peers + // that missed the live delete. + let expiry_statement = lifecycle_expiry_statement(&metadata); + entry.expiry_lc_config = expiry_statement.as_ref().and_then(|(subset, _)| subset.clone()); entry.cors_config = raw_config_to_base64(&metadata.cors_config_xml); entry.policy_updated_at = maybe_time(metadata.policy_config_updated_at); entry.tag_config_updated_at = maybe_time(metadata.tagging_config_updated_at); @@ -4348,11 +4361,8 @@ async fn build_sr_info(state: &SiteReplicationState, local_peer: &PeerInfo) -> S // The expiry axis, not the whole-config write time: local // transition-only edits inflate the latter, and a repair item // stamped with it could out-rank a newer real expiry edit on a - // third site. Legacy configs without the axis fall back to the - // whole-config time (bounded staleness, same as the receiver). - entry.expiry_lc_config_updated_at = lifecycle_expiry_updated_at(&metadata.lifecycle_config_xml) - .map(Some) - .unwrap_or_else(|| maybe_time(metadata.lifecycle_config_updated_at)); + // third site. + entry.expiry_lc_config_updated_at = expiry_statement.map(|(_, axis)| axis); entry.cors_config_updated_at = maybe_time(metadata.cors_config_updated_at); entry.replication_targets_online = Some(site_replication_targets_online(&bucket.name, &metadata.replication_config_xml).await); @@ -7036,13 +7046,95 @@ fn lifecycle_expiry_updated_at(raw: &[u8]) -> Option { .map(OffsetDateTime::from) } -/// MinIO emits a zero-rule `…` -/// document for its delete tombstone / transition-only states; the s3s -/// deserializer rejects a missing `` outright. Such a document is the -/// "no expiry rules here" statement and must map to delete semantics rather -/// than an error. -fn lifecycle_document_has_rules(raw: &[u8]) -> bool { - raw.windows(b", + whole_config_axis: OffsetDateTime, +) -> OffsetDateTime { + match local { + Some(config) => match config.expiry_updated_at.clone() { + Some(axis) => OffsetDateTime::from(axis), + None if config.rules.iter().any(lifecycle_rule_has_expiry) => whole_config_axis, + None => OffsetDateTime::UNIX_EPOCH, + }, + None => whole_config_axis, + } +} + +/// Recognize MinIO's zero-rule lifecycle tombstone (its delete / +/// transition-only state marshals `` with no `` +/// child, which the strict s3s deserializer rejects). Only a well-delimited +/// document qualifies as the "no expiry rules here" statement; truncated or +/// otherwise malformed payloads are rejected rather than treated as a delete +/// that would erase local expiry rules. +fn is_zero_rule_lifecycle_tombstone(raw: &[u8]) -> bool { + let Ok(text) = std::str::from_utf8(raw) else { + return false; + }; + let mut text = text.trim(); + if let Some(rest) = text.strip_prefix("") else { + return false; + }; + text = rest[end + 2..].trim_start(); + } + let Some(rest) = text.strip_prefix("") { + return tail.trim().is_empty(); + } + // The root open tag may carry attributes (xmlns); it must close, and the + // document must end with the matching close tag. + let Some(open_end) = rest.find('>') else { + return false; + }; + let Some(body) = rest[open_end + 1..].trim_end().strip_suffix("") else { + return false; + }; + !body.contains(" Option<(Option, OffsetDateTime)> { + if metadata.lifecycle_config_xml.is_empty() { + // Deleted vs never configured: the whole-config write time survives + // deletion in bucket metadata and strictly exceeds the created-time + // backfill only after a real write. + return (metadata.lifecycle_config_updated_at > metadata.created).then_some((None, metadata.lifecycle_config_updated_at)); + } + let axis = lifecycle_expiry_updated_at(&metadata.lifecycle_config_xml); + match lifecycle_expiry_subset_xml(&metadata.lifecycle_config_xml) { + Some(subset) => { + // Legacy documents predate the axis field; their whole-config + // write time bounds the last expiry edit. + let axis = axis.unwrap_or(metadata.lifecycle_config_updated_at); + Some((raw_config_to_base64(&subset), axis)) + } + // Transition-only config: with an expiry axis the site once had + // expiry rules and properly removed them — the delete travels at + // that axis. Without one there is nothing to say (a delete stamped + // off the whole-config time would let a local transition edit erase + // newer peer expiry state). + None => axis.map(|axis| (None, axis)), + } } fn replication_rule_deployment_id(rule: &ReplicationRule) -> Option { @@ -8194,8 +8286,10 @@ async fn apply_bucket_meta_item(item: SRBucketMeta) -> S3Result<()> { Ok(config) => Some(config), // MinIO's delete tombstone / transition-only state is a // zero-rule document the strict deserializer rejects; it - // means "no expiry rules here" (delete semantics). - Err(_) if !lifecycle_document_has_rules(&data) => None, + // means "no expiry rules here" (delete semantics). Any + // other malformed payload is rejected — treating it as a + // delete would let a bad payload erase local expiry rules. + Err(_) if is_zero_rule_lifecycle_tombstone(&data) => None, Err(e) => return Err(s3_error!(InvalidRequest, "invalid lifecycle config: {e}")), } } @@ -8206,21 +8300,11 @@ async fn apply_bucket_meta_item(item: SRBucketMeta) -> S3Result<()> { Err(StorageError::ConfigNotFound) => None, Err(err) => return Err(ApiError::from(err).into()), }; - // Staleness axis: the config's expiry_updated_at when present. When - // the config is absent (deleted) or predates the axis field, fall - // back to the whole-config write time — it survives deletion in - // bucket metadata and is the deletion's lower bound, so a delayed - // stale broadcast cannot resurrect deleted expiry rules. let whole_config_axis = metadata_sys::get(&item.bucket) .await .map(|bucket_meta| bucket_meta.lifecycle_config_updated_at) .unwrap_or(OffsetDateTime::UNIX_EPOCH); - let local_expiry_updated_at = local - .as_ref() - .and_then(|config| config.expiry_updated_at.clone()) - .map(OffsetDateTime::from) - .unwrap_or(whole_config_axis); - if is_stale_update(local_expiry_updated_at, incoming_updated_at) { + if is_stale_update(local_lifecycle_staleness_axis(local.as_ref(), whole_config_axis), incoming_updated_at) { return Ok(()); } let local_absent = local.is_none(); @@ -12913,6 +12997,87 @@ mod tests { assert!(!plan.bucket_items.iter().any(|item| item.r#type == "lc-config")); } + /// A deleted expiry state (entry value None, axis set) must travel as an + /// explicit timestamped delete item — a peer that missed the live delete + /// otherwise keeps stale expiry rules through every repair (review + /// finding). + #[test] + fn test_site_replication_bootstrap_plan_emits_timestamped_lifecycle_delete() { + let deleted_at = OffsetDateTime::from_unix_timestamp(1_700_000_000).expect("timestamp"); + let mut info = SRInfo::default(); + info.state.peers.insert( + "remote-dep".to_string(), + PeerInfo { + replicate_ilm_expiry: true, + ..peer("remote", "https://remote.example.com") + }, + ); + info.buckets.insert( + "photos".to_string(), + SRBucketInfo { + bucket: "photos".to_string(), + expiry_lc_config: None, + expiry_lc_config_updated_at: Some(deleted_at), + api_version: Some(SITE_REPL_API_VERSION.to_string()), + ..Default::default() + }, + ); + + let plan = site_replication_bootstrap_plan(&info).expect("bootstrap plan should build"); + + let item = plan + .bucket_items + .iter() + .find(|item| item.r#type == "lc-config") + .expect("a deleted expiry state must produce an lc-config delete item"); + assert!(item.expiry_lc_config.is_none(), "delete items carry no config body"); + assert_eq!(item.expiry_updated_at, Some(deleted_at)); + assert_eq!(item.updated_at, Some(deleted_at)); + } + + /// What each local lifecycle state contributes to the SRInfo entry: + /// deletions are timestamped statements, never-configured buckets and + /// transition-only configs without an expiry axis say nothing. + #[test] + fn test_lifecycle_expiry_statement_matrix() { + let created = OffsetDateTime::from_unix_timestamp(1_600_000_000).expect("timestamp"); + let mut meta = crate::admin::storage_api::bucket::metadata::BucketMetadata::new("photos"); + meta.created = created; + // Never configured: load backfills the write time to `created`. + meta.lifecycle_config_updated_at = created; + assert!(lifecycle_expiry_statement(&meta).is_none()); + + // Deleted: the write time survives deletion and exceeds creation. + let deleted_at = created + time::Duration::seconds(100); + meta.lifecycle_config_updated_at = deleted_at; + let (subset, axis) = lifecycle_expiry_statement(&meta).expect("deletion is a statement"); + assert!(subset.is_none()); + assert_eq!(axis, deleted_at); + + // Present with expiry rules and the axis: subset + axis travel. + let expiry_axis = created + time::Duration::seconds(50); + let mut config = lc_config(vec![lc_rule("e1", Some(7), None)]); + config.expiry_updated_at = Some(s3s::dto::Timestamp::from(expiry_axis)); + meta.lifecycle_config_xml = serialize(&config).expect("serialize config"); + let (subset, axis) = lifecycle_expiry_statement(&meta).expect("expiry config is a statement"); + assert!(subset.is_some()); + assert_eq!(axis.unix_timestamp(), expiry_axis.unix_timestamp()); + + // Transition-only without an axis: nothing to say (a delete stamped + // off the whole-config time would erase newer peer expiry state). + meta.lifecycle_config_xml = serialize(&lc_config(vec![lc_rule("t1", None, Some(30))])).expect("serialize config"); + assert!(lifecycle_expiry_statement(&meta).is_none()); + + // Transition-only WITH an axis: expiry rules were properly removed — + // the delete travels at that axis. + let mut transition_only = lc_config(vec![lc_rule("t1", None, Some(30))]); + transition_only.expiry_updated_at = Some(s3s::dto::Timestamp::from(expiry_axis)); + meta.lifecycle_config_xml = serialize(&transition_only).expect("serialize config"); + let (subset, axis) = lifecycle_expiry_statement(&meta).expect("removed expiry state is a statement"); + assert!(subset.is_none()); + assert_eq!(axis.unix_timestamp(), expiry_axis.unix_timestamp()); + } + #[test] fn test_site_replication_repair_request_is_strict_and_requires_explicit_mode() { assert!(serde_json::from_str::(r#"{"mode":"dry-run"}"#).is_ok()); @@ -14839,20 +15004,56 @@ mod tests { ); } - /// MinIO emits a zero-rule lifecycle document (delete tombstone / - /// transition-only state); the strict deserializer rejects it, so the - /// apply path must recognize it as delete semantics via the rule sniff. + /// Only a well-delimited zero-rule `` document is + /// the delete statement; truncated or foreign payloads must be rejected, + /// not treated as a delete that erases local expiry rules. #[test] - fn test_lifecycle_document_has_rules_sniff() { - assert!(!lifecycle_document_has_rules( + fn test_zero_rule_lifecycle_tombstone_recognition() { + assert!(is_zero_rule_lifecycle_tombstone( b"2026-01-01T00:00:00Z" )); - assert!(lifecycle_document_has_rules( + assert!(is_zero_rule_lifecycle_tombstone( + b"\n" + )); + assert!(is_zero_rule_lifecycle_tombstone(b"")); + + // Documents with rules are not tombstones (they must parse strictly). + assert!(!is_zero_rule_lifecycle_tombstone( b"x" )); - assert!(lifecycle_document_has_rules( - b"" - )); + // Truncated / malformed / foreign payloads are rejected. + assert!(!is_zero_rule_lifecycle_tombstone(b"")); + assert!(!is_zero_rule_lifecycle_tombstone(b"")); + assert!(!is_zero_rule_lifecycle_tombstone(b"garbage")); + assert!(!is_zero_rule_lifecycle_tombstone(b"")); + assert!(!is_zero_rule_lifecycle_tombstone(b"")); + } + + /// The staleness axis an incoming lc-config item must beat: the expiry + /// axis when present; the whole-config write time only for deleted or + /// legacy-with-expiry state; epoch for a transition-only config (its + /// whole-config time moves on transition edits and must not shadow + /// independent peer expiry updates — review finding). + #[test] + fn test_local_lifecycle_staleness_axis_selection() { + let whole = OffsetDateTime::from_unix_timestamp(1_700_000_000).expect("timestamp"); + let axis_ts = OffsetDateTime::from_unix_timestamp(1_600_000_000).expect("timestamp"); + + let mut with_axis = lc_config(vec![lc_rule("e1", Some(7), None)]); + with_axis.expiry_updated_at = Some(s3s::dto::Timestamp::from(axis_ts)); + assert_eq!(local_lifecycle_staleness_axis(Some(&with_axis), whole), axis_ts); + + let legacy_with_expiry = lc_config(vec![lc_rule("e1", Some(7), None)]); + assert_eq!(local_lifecycle_staleness_axis(Some(&legacy_with_expiry), whole), whole); + + let transition_only = lc_config(vec![lc_rule("t1", None, Some(30))]); + assert_eq!( + local_lifecycle_staleness_axis(Some(&transition_only), whole), + OffsetDateTime::UNIX_EPOCH, + "a transition-only config has no expiry state to protect" + ); + + assert_eq!(local_lifecycle_staleness_axis(None, whole), whole, "deletion lower bound"); } /// Sender-side filter: only the expiry subset leaves this site. MinIO