fix(tiering): make rejected upload cleanup durable (#5059)

* fix(tiering): make rejected upload cleanup durable * fix(tiering): close transition upload cancellation gap * test(tiering): cover failed upload without candidate * test(tiering): synchronize cancelled cleanup recovery * test(tiering): stabilize cancelled cleanup recovery Prefer cancellation when the tier delete journal recovery worker is racing an immediate tick, and build the cancelled-cleanup regression store with an already-cancelled token so production recovery cannot consume the test journal. Co-Authored-By: heihutu <heihutu@gmail.com> --------- Co-authored-by: houseme <housemecn@gmail.com> Co-authored-by: heihutu <heihutu@gmail.com>
2026-08-31 09:18:28 +00:00 · 2026-07-20 20:54:32 +08:00
parent 35f3599992
commit 28fdcc87be
14 changed files with 1914 additions and 104 deletions
@@ -107,7 +107,6 @@ pub type ExpiryOpType = Box<dyn ExpiryOp + Send + Sync + 'static>;

 static XXHASH_SEED: u64 = 0;
 static TIER_FREE_VERSION_RECOVERY_STARTED: OnceLock<()> = OnceLock::new();
-static TIER_DELETE_JOURNAL_RECOVERY_STARTED: OnceLock<()> = OnceLock::new();

 pub const AMZ_OBJECT_TAGGING: &str = "X-Amz-Tagging";
 pub const AMZ_TAG_COUNT: &str = "x-amz-tagging-count";
@@ -419,6 +418,7 @@ async fn delete_free_version_remote_object(
        &oi.transitioned_object.tier,
        identity,
        tier_config_mgr,
+        false,
    )
    .await?;
    Ok(())
@@ -1504,12 +1504,20 @@ fn spawn_tier_free_version_recovery_once(api: Arc<ECStore>) {
 }

 fn spawn_tier_delete_journal_recovery_once(api: Arc<ECStore>) {
-    if TIER_DELETE_JOURNAL_RECOVERY_STARTED.set(()).is_err() {
+    let Some(cancel_token) = api.ctx.background_cancel_token() else {
+        error!(
+            event = EVENT_LIFECYCLE_WORKER_STATE,
+            component = LOG_COMPONENT_ECSTORE,
+            subsystem = LOG_SUBSYSTEM_LIFECYCLE,
+            store_id = %api.id,
+            "Tier delete journal recovery was not started because the store shutdown token is unavailable"
+        );
+        return;
+    };
+    if !api.ctx.mark_tier_delete_journal_recovery_started(api.id) {
        return;
    }
-
    tokio::spawn(async move {
-        let cancel_token = runtime_sources::background_services_cancel_token().unwrap_or_default();
        run_tier_delete_journal_recovery_loop(api, cancel_token).await;
    });
 }
@@ -1989,7 +1997,7 @@ fn transitioned_cleanup_tuple(oi: &ObjectInfo) -> Result<(&str, &str, &str), std
    if transitioned.status != lifecycle::TRANSITION_COMPLETE {
        return Err(std::io::Error::other("transitioned object cleanup tuple is not complete"));
    }
-    if transitioned.name.is_empty() || transitioned.version_id.is_empty() || transitioned.tier.is_empty() {
+    if transitioned.name.is_empty() || transitioned.tier.is_empty() {
        return Err(std::io::Error::other("transitioned object cleanup tuple is incomplete"));
    }
    Ok((&transitioned.name, &transitioned.version_id, &transitioned.tier))
@@ -3576,6 +3584,7 @@ mod tests {
            version_id: "remote-version".to_string(),
            tier_name: "WARM".to_string(),
            backend_identity: Some([1; 32]),
+            version_id_exact: false,
        };

        let err = state
@@ -3664,6 +3673,7 @@ mod tests {
            version_id: "remote-version".to_string(),
            tier_name: "WARM".to_string(),
            backend_identity: Some([1; 32]),
+            version_id_exact: false,
        };

        state
@@ -3814,7 +3824,7 @@ mod tests {
    }

    #[test]
-    fn transitioned_cleanup_tuple_requires_remote_name_version_and_tier() {
+    fn transitioned_cleanup_tuple_preserves_versioned_remote() {
        let mut oi = ObjectInfo::default();
        oi.transitioned_object.status = crate::bucket::lifecycle::lifecycle::TRANSITION_COMPLETE.to_string();
        oi.transitioned_object.name = "remote/object".to_string();
@@ -3827,15 +3837,29 @@ mod tests {
    }

    #[test]
-    fn transitioned_cleanup_tuple_rejects_missing_remote_version() {
+    fn transitioned_cleanup_tuple_accepts_unversioned_remote() {
        let mut oi = ObjectInfo::default();
        oi.transitioned_object.status = crate::bucket::lifecycle::lifecycle::TRANSITION_COMPLETE.to_string();
        oi.transitioned_object.name = "remote/object".to_string();
        oi.transitioned_object.tier = "WARM".to_string();

-        let err = transitioned_cleanup_tuple(&oi).expect_err("missing version must be rejected");
+        let tuple = transitioned_cleanup_tuple(&oi).expect("an empty remote version identifies an unversioned tier bucket");

-        assert!(err.to_string().contains("cleanup tuple is incomplete"));
+        assert_eq!(tuple, ("remote/object", "", "WARM"));
+    }
+
+    #[test]
+    fn transitioned_cleanup_tuple_rejects_missing_remote_name_or_tier() {
+        for (name, tier) in [("", "WARM"), ("remote/object", "")] {
+            let mut oi = ObjectInfo::default();
+            oi.transitioned_object.status = crate::bucket::lifecycle::lifecycle::TRANSITION_COMPLETE.to_string();
+            oi.transitioned_object.name = name.to_string();
+            oi.transitioned_object.tier = tier.to_string();
+
+            let err = transitioned_cleanup_tuple(&oi).expect_err("remote name and tier must remain required");
+
+            assert!(err.to_string().contains("cleanup tuple is incomplete"));
+        }
    }

    #[test]
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-use std::sync::Arc;
+use std::{future::Future, sync::Arc, time::Duration};

 use serde::{Deserialize, Serialize};
 use sha2::{Digest, Sha256};
@@ -38,8 +38,11 @@ const LOG_SUBSYSTEM_LIFECYCLE: &str = "lifecycle";
 const EVENT_LIFECYCLE_TIER_DELETE_JOURNAL: &str = "lifecycle_tier_delete_journal";

 pub const DEFAULT_TIER_DELETE_JOURNAL_RECOVERY_LIMIT: usize = 1_000;
+const TIER_DELETE_JOURNAL_RECOVERY_INTERVAL: Duration = Duration::from_secs(60);
+const TIER_DELETE_JOURNAL_RECOVERY_TIMEOUT: Duration = Duration::from_secs(300);
 const TIER_DELETE_JOURNAL_VERSION: u8 = 2;
-const TIER_DELETE_JOURNAL_PREFIX: &str = "ilm/tier-delete-journal/";
+const TIER_DELETE_JOURNAL_EXACT_VERSION: u8 = 3;
+pub(crate) const TIER_DELETE_JOURNAL_PREFIX: &str = "ilm/tier-delete-journal/";

 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 #[serde(deny_unknown_fields)]
@@ -50,12 +53,16 @@ struct PersistedTierDeleteJournalEntry {
    tier_name: String,
    #[serde(default)]
    backend_identity: Option<[u8; 32]>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    version_id_exact: Option<bool>,
 }

 impl PersistedTierDeleteJournalEntry {
    fn from_jentry(je: &Jentry) -> Self {
        Self {
-            version: if je.backend_identity.is_some() {
+            version: if je.version_id_exact {
+                TIER_DELETE_JOURNAL_EXACT_VERSION
+            } else if je.backend_identity.is_some() {
                TIER_DELETE_JOURNAL_VERSION
            } else {
                1
@@ -64,6 +71,7 @@ impl PersistedTierDeleteJournalEntry {
            version_id: je.version_id.clone(),
            tier_name: je.tier_name.clone(),
            backend_identity: je.backend_identity,
+            version_id_exact: je.version_id_exact.then_some(true),
        }
    }

@@ -76,12 +84,32 @@ impl PersistedTierDeleteJournalEntry {
        if self.obj_name.is_empty() || self.tier_name.is_empty() {
            return Err(Error::other("tier delete journal entry is incomplete"));
        }
-        let backend_identity = match self.version {
-            1 => None,
-            TIER_DELETE_JOURNAL_VERSION => Some(
-                self.backend_identity
-                    .ok_or_else(|| Error::other("tier delete journal v2 entry is missing its backend identity"))?,
+        if self.version != TIER_DELETE_JOURNAL_EXACT_VERSION && self.version_id_exact.unwrap_or(false) {
+            return Err(Error::other(
+                "legacy tier delete journal entry has an unsupported exact version constraint",
+            ));
+        }
+        let (backend_identity, version_id_exact) = match self.version {
+            1 => (None, false),
+            TIER_DELETE_JOURNAL_VERSION => (
+                Some(
+                    self.backend_identity
+                        .ok_or_else(|| Error::other("tier delete journal v2 entry is missing its backend identity"))?,
+                ),
+                false,
            ),
+            TIER_DELETE_JOURNAL_EXACT_VERSION => {
+                if self.version_id.is_empty() || self.version_id_exact != Some(true) {
+                    return Err(Error::other("tier delete journal v3 entry is missing its exact version constraint"));
+                }
+                (
+                    Some(
+                        self.backend_identity
+                            .ok_or_else(|| Error::other("tier delete journal v3 entry is missing its backend identity"))?,
+                    ),
+                    true,
+                )
+            }
            version => return Err(Error::other(format!("unsupported tier delete journal version {version}"))),
        };
        Ok(Jentry {
@@ -89,6 +117,7 @@ impl PersistedTierDeleteJournalEntry {
            version_id: self.version_id,
            tier_name: self.tier_name,
            backend_identity,
+            version_id_exact,
        })
    }
 }
@@ -113,6 +142,10 @@ pub(crate) fn tier_delete_journal_object_name(je: &Jentry) -> String {
        hasher.update([0]);
        hasher.update(backend_identity);
    }
+    if je.version_id_exact {
+        hasher.update([0]);
+        hasher.update(b"exact-version-id");
+    }
    format!(
        "{TIER_DELETE_JOURNAL_PREFIX}{}.json",
        rustfs_utils::crypto::hex(hasher.finalize().as_slice())
@@ -185,6 +218,7 @@ pub async fn process_tier_delete_journal_entry(api: Arc<ECStore>, je: &Jentry) -
        &je.tier_name,
        backend_identity,
        &api.tier_config_mgr(),
+        je.version_id_exact,
    )
    .await?;
    remove_tier_delete_journal_entry(api, je).await
@@ -294,16 +328,33 @@ pub async fn recover_tier_delete_journal_entries(
 }

 pub async fn run_tier_delete_journal_recovery_loop(api: Arc<ECStore>, cancel_token: CancellationToken) {
-    let mut interval = tokio::time::interval(std::time::Duration::from_secs(60));
+    let mut interval = tokio::time::interval(TIER_DELETE_JOURNAL_RECOVERY_INTERVAL);
+    interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay);
    let mut marker: Option<String> = None;

    loop {
+        #[cfg(test)]
        tokio::select! {
+            biased;
            _ = cancel_token.cancelled() => return,
-            _ = interval.tick() => {}
+            _ = interval.tick() => {},
+            _ = api.ctx.wait_for_tier_delete_journal_recovery() => {},
+        }
+        #[cfg(not(test))]
+        tokio::select! {
+            biased;
+            _ = cancel_token.cancelled() => return,
+            _ = interval.tick() => {},
        }

-        match recover_tier_delete_journal_entries(api.clone(), DEFAULT_TIER_DELETE_JOURNAL_RECOVERY_LIMIT, marker.clone()).await {
+        let recovery =
+            recover_tier_delete_journal_entries(api.clone(), DEFAULT_TIER_DELETE_JOURNAL_RECOVERY_LIMIT, marker.clone());
+        let Some(result) =
+            await_tier_delete_journal_recovery(&cancel_token, TIER_DELETE_JOURNAL_RECOVERY_TIMEOUT, recovery).await
+        else {
+            return;
+        };
+        match result {
            Ok(stats) => {
                marker = stats.next_marker;
                debug!(
@@ -332,13 +383,36 @@ pub async fn run_tier_delete_journal_recovery_loop(api: Arc<ECStore>, cancel_tok
    }
 }

+async fn await_tier_delete_journal_recovery<T, F>(
+    cancel_token: &CancellationToken,
+    timeout: Duration,
+    recovery: F,
+) -> Option<Result<T>>
+where
+    F: Future<Output = Result<T>>,
+{
+    tokio::select! {
+        _ = cancel_token.cancelled() => None,
+        result = tokio::time::timeout(timeout, recovery) => Some(match result {
+            Ok(result) => result,
+            Err(_) => Err(Error::other(format!(
+                "tier delete journal recovery timed out after {} seconds",
+                timeout.as_secs()
+            ))),
+        }),
+    }
+}
+
 #[cfg(test)]
 mod tests {
    use super::{
-        decode_tier_delete_journal_entry, encode_tier_delete_journal_entry, record_tier_delete_journal_backend_identity,
-        tier_delete_journal_object_name,
+        TIER_DELETE_JOURNAL_EXACT_VERSION, await_tier_delete_journal_recovery, decode_tier_delete_journal_entry,
+        encode_tier_delete_journal_entry, record_tier_delete_journal_backend_identity, tier_delete_journal_object_name,
    };
    use crate::bucket::lifecycle::tier_sweeper::Jentry;
+    use crate::error::Result;
+    use std::time::Duration;
+    use tokio_util::sync::CancellationToken;

    fn journal_entry() -> Jentry {
        Jentry {
@@ -346,6 +420,7 @@ mod tests {
            version_id: "remote-version".to_string(),
            tier_name: "WARM".to_string(),
            backend_identity: Some([7; 32]),
+            version_id_exact: false,
        }
    }

@@ -360,6 +435,82 @@ mod tests {
        assert_eq!(decoded.version_id, je.version_id);
        assert_eq!(decoded.tier_name, je.tier_name);
        assert_eq!(decoded.backend_identity, je.backend_identity);
+        assert_eq!(decoded.version_id_exact, je.version_id_exact);
+    }
+
+    #[test]
+    fn tier_delete_journal_roundtrips_exact_put_response_constraint() {
+        let mut exact = journal_entry();
+        exact.version_id = uuid::Uuid::nil().to_string();
+        exact.version_id_exact = true;
+        let mut normalized = exact.clone();
+        normalized.version_id_exact = false;
+
+        let encoded = encode_tier_delete_journal_entry(&exact).expect("exact journal entry should encode");
+        let persisted: serde_json::Value = serde_json::from_slice(&encoded).expect("exact journal JSON should decode");
+        let decoded = decode_tier_delete_journal_entry(&encoded).expect("exact journal entry should decode");
+
+        assert_eq!(persisted["version"], TIER_DELETE_JOURNAL_EXACT_VERSION);
+        assert_eq!(persisted["version_id_exact"], true);
+        assert!(decoded.version_id_exact);
+        assert_ne!(tier_delete_journal_object_name(&exact), tier_delete_journal_object_name(&normalized));
+    }
+
+    #[test]
+    fn tier_delete_journal_rejects_invalid_exact_version_constraints() {
+        let identity = vec![7_u8; 32];
+        let invalid = [
+            serde_json::json!({
+                "version": 1,
+                "obj_name": "remote/object",
+                "version_id": "exact-version",
+                "tier_name": "WARM",
+                "version_id_exact": true,
+            }),
+            serde_json::json!({
+                "version": 2,
+                "obj_name": "remote/object",
+                "version_id": "exact-version",
+                "tier_name": "WARM",
+                "backend_identity": identity,
+                "version_id_exact": true,
+            }),
+            serde_json::json!({
+                "version": TIER_DELETE_JOURNAL_EXACT_VERSION,
+                "obj_name": "remote/object",
+                "version_id": "",
+                "tier_name": "WARM",
+                "backend_identity": identity,
+                "version_id_exact": true,
+            }),
+            serde_json::json!({
+                "version": TIER_DELETE_JOURNAL_EXACT_VERSION,
+                "obj_name": "remote/object",
+                "version_id": "exact-version",
+                "tier_name": "WARM",
+                "backend_identity": identity,
+            }),
+            serde_json::json!({
+                "version": TIER_DELETE_JOURNAL_EXACT_VERSION,
+                "obj_name": "remote/object",
+                "version_id": "exact-version",
+                "tier_name": "WARM",
+                "backend_identity": identity,
+                "version_id_exact": false,
+            }),
+            serde_json::json!({
+                "version": TIER_DELETE_JOURNAL_EXACT_VERSION,
+                "obj_name": "remote/object",
+                "version_id": "exact-version",
+                "tier_name": "WARM",
+                "version_id_exact": true,
+            }),
+        ];
+
+        for persisted in invalid {
+            let encoded = serde_json::to_vec(&persisted).expect("invalid journal fixture should encode");
+            decode_tier_delete_journal_entry(&encoded).expect_err("invalid exact journal constraint must fail closed");
+        }
    }

    #[test]
@@ -476,4 +627,29 @@ mod tests {

        assert!(err.to_string().contains("decode tier delete journal failed"));
    }
+
+    #[tokio::test]
+    async fn tier_delete_journal_recovery_has_a_hard_outer_timeout() {
+        let result = await_tier_delete_journal_recovery(
+            &CancellationToken::new(),
+            Duration::from_millis(10),
+            std::future::pending::<Result<()>>(),
+        )
+        .await
+        .expect("an elapsed timeout should return a recovery error")
+        .expect_err("a permanently pending recovery must time out");
+
+        assert!(result.to_string().contains("recovery timed out"), "{result}");
+    }
+
+    #[tokio::test]
+    async fn tier_delete_journal_recovery_drops_in_flight_work_on_shutdown() {
+        let cancel = CancellationToken::new();
+        cancel.cancel();
+
+        let result =
+            await_tier_delete_journal_recovery(&cancel, Duration::from_secs(30), std::future::pending::<Result<()>>()).await;
+
+        assert!(result.is_none(), "shutdown must cancel the in-flight recovery future");
+    }
 }
@@ -249,6 +249,7 @@ impl ObjSweeper {
                version_id: self.transition_version_id.clone(),
                tier_name: self.transition_tier.clone(),
                backend_identity: None,
+                version_id_exact: false,
            });
        }
        None
@@ -284,6 +285,7 @@ pub struct Jentry {
    pub(crate) version_id: String,
    pub(crate) tier_name: String,
    pub(crate) backend_identity: Option<TierDestinationId>,
+    pub(crate) version_id_exact: bool,
 }

 impl ExpiryOp for Jentry {
@@ -328,13 +330,14 @@ async fn delete_object_from_remote_tier_raw_with_manager(
    let lease = TierConfigMgr::acquire_operation_lease(&tier_config_mgr, tier_name)
        .await
        .map_err(std::io::Error::other)?;
-    delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, &lease).await
+    delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, &lease, false).await
 }

 async fn delete_object_from_remote_tier_raw_with_lease(
    obj_name: &str,
    rv_id: &str,
    lease: &TierOperationLease,
+    version_id_exact: bool,
 ) -> Result<(), std::io::Error> {
    if remote_delete_breaker_is_open(Instant::now()).await {
        metrics::counter!(METRIC_DELETE_REMOTE_BREAKER_TOTAL).increment(1);
@@ -347,7 +350,11 @@ async fn delete_object_from_remote_tier_raw_with_lease(
        .map_err(|_| std::io::Error::other(ERR_REMOTE_DELETE_LIMITER_CLOSED))?;
    let _inflight = RemoteDeleteInflightGuard::new();

-    lease.remove(obj_name, rv_id).await
+    if version_id_exact {
+        lease.remove_exact(obj_name, rv_id).await
+    } else {
+        lease.remove(obj_name, rv_id).await
+    }
 }

 #[cfg(test)]
@@ -388,19 +395,21 @@ pub(crate) async fn delete_object_from_remote_tier_idempotent_with_manager_and_i
    tier_name: &str,
    backend_identity: TierDestinationId,
    tier_config_mgr: &Arc<tokio::sync::RwLock<TierConfigMgr>>,
+    version_id_exact: bool,
 ) -> Result<RemoteTierDeleteOutcome, std::io::Error> {
    let lease = TierConfigMgr::acquire_operation_lease_for_backend_identity(tier_config_mgr, tier_name, backend_identity)
        .await
        .map_err(std::io::Error::other)?;
-    delete_object_from_remote_tier_with_lease_idempotent(obj_name, rv_id, &lease).await
+    delete_object_from_remote_tier_with_lease_idempotent(obj_name, rv_id, &lease, version_id_exact).await
 }

 pub(crate) async fn delete_object_from_remote_tier_with_lease_idempotent(
    obj_name: &str,
    rv_id: &str,
    lease: &TierOperationLease,
+    version_id_exact: bool,
 ) -> Result<RemoteTierDeleteOutcome, std::io::Error> {
-    match delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, lease).await {
+    match delete_object_from_remote_tier_raw_with_lease(obj_name, rv_id, lease, version_id_exact).await {
        Ok(()) => Ok(RemoteTierDeleteOutcome::Deleted),
        Err(err) if is_remote_tier_not_found_error(&err) => Ok(RemoteTierDeleteOutcome::AlreadyRemoved),
        Err(err) => {
@@ -450,6 +459,7 @@ pub fn transitioned_force_delete_journal_entry(transitioned: &TransitionedObject
        version_id: transitioned.version_id.clone(),
        tier_name: transitioned.tier.clone(),
        backend_identity: None,
+        version_id_exact: false,
    })
 }

@@ -574,6 +584,7 @@ mod test {
            "WARM",
            mismatched,
            &manager,
+            false,
        )
        .await
        .expect_err("journal recovery must fail closed when the tier name was rebound");
@@ -581,6 +592,33 @@ mod test {
        assert!(err.to_string().contains("identity no longer matches"));
    }

+    #[cfg(feature = "test-util")]
+    #[tokio::test]
+    async fn journal_delete_dispatches_an_exact_version_constraint() {
+        let manager = crate::services::tier::tier::TierConfigMgr::new();
+        let backend = crate::services::tier::test_util::register_mock_tier(&manager, "WARM").await;
+        let lease = crate::services::tier::tier::TierConfigMgr::acquire_operation_lease(&manager, "WARM")
+            .await
+            .expect("test tier lease should be available");
+        let identity = lease.backend_identity();
+        drop(lease);
+
+        let outcome = delete_object_from_remote_tier_idempotent_with_manager_and_identity(
+            "remote/object",
+            "exact-version",
+            "WARM",
+            identity,
+            &manager,
+            true,
+        )
+        .await
+        .expect("an exact journal delete should reach the backend");
+
+        assert_eq!(outcome, RemoteTierDeleteOutcome::Deleted);
+        assert_eq!(backend.exact_remove_count(), 1);
+        assert_eq!(backend.remove_count().await, 1);
+    }
+
    #[test]
    fn breaker_opens_at_threshold_and_recovers_after_window() {
        let mut breaker = RemoteDeleteBreaker::new(3, Duration::from_secs(30));