fix(ilm): delete historical null versions by exact identity (#7109)

crates/lifecycle/src/core.rs

@@ -481,7 +481,7 @@ impl Lifecycle for BucketLifecycleConfiguration {
             return Event::default();
         };
 
-        if obj.delete_marker || !(obj.is_latest || obj.version_id.is_none_or(|v| v.is_nil())) {
+        if obj.delete_marker || !obj.is_latest {
             return Event::default();
         }
 
@@ -676,10 +676,11 @@ impl Lifecycle for BucketLifecycleConfiguration {
                     obj.is_latest,
                     obj.delete_marker,
                     obj.version_id,
-                    (obj.is_latest || obj.version_id.is_none_or(|v| v.is_nil())) && !obj.delete_marker
+                    obj.is_latest && !obj.delete_marker
                 );
-                // Allow expiration for latest objects OR non-versioned objects (empty version_id)
-                if (obj.is_latest || obj.version_id.is_none_or(|v| v.is_nil())) && !obj.delete_marker {
+                // Current-version expiration is selected by the authoritative
+                // latest flag. An explicit null version can also be historical.
+                if obj.is_latest && !obj.delete_marker {
                     debug!("eval_inner: entering expiration check");
                     if let Some(ref expiration) = rule.expiration {
                         if let Some(ref date) = expiration.date {
@@ -1043,6 +1044,26 @@ impl ObjectOpts {
     }
 }
 
+/// Returns whether an expiry action has enough identity to target the object
+/// it was evaluated against. A nil UUID is an explicit S3 null-version
+/// identity; only an absent version ID is ambiguous for an exact-version
+/// action.
+pub fn expiration_action_has_valid_target(
+    action: IlmAction,
+    version_id: Option<Uuid>,
+    is_latest: bool,
+    delete_marker: bool,
+) -> bool {
+    match action {
+        IlmAction::DeleteAction | IlmAction::DeleteRestoredAction => is_latest && !delete_marker,
+        IlmAction::DeleteVersionAction => version_id.is_some() && (!is_latest || delete_marker),
+        IlmAction::DeleteRestoredVersionAction => version_id.is_some() && !is_latest && !delete_marker,
+        IlmAction::DeleteAllVersionsAction => is_latest && !delete_marker,
+        IlmAction::DelMarkerDeleteAllVersionsAction => is_latest && delete_marker,
+        _ => true,
+    }
+}
+
 /// Total-order rank for lifecycle actions used to break `due` ties.
 ///
 /// Delete-type actions rank before every other action so that, when two events
@@ -4008,6 +4029,76 @@ mod tests {
         assert_eq!(event.action, IlmAction::DeleteAction);
     }
 
+    #[tokio::test]
+    async fn current_expiration_requires_latest_even_for_null_version_identity() {
+        let base_time = datetime!(2025-01-01 00:00:00 UTC);
+        let lc = BucketLifecycleConfiguration {
+            expiry_updated_at: None,
+            rules: vec![enabled_rule(
+                Some(LifecycleExpiration {
+                    days: Some(1),
+                    ..Default::default()
+                }),
+                None,
+                Some("expire-current"),
+            )],
+        };
+        let now = base_time + Duration::days(2);
+
+        let unversioned_current = ObjectOpts {
+            name: "object".to_string(),
+            mod_time: Some(base_time),
+            is_latest: true,
+            version_id: None,
+            ..Default::default()
+        };
+        assert_eq!(
+            lc.eval_inner(&unversioned_current, now, 0).await.action,
+            IlmAction::DeleteAction,
+            "a truly unversioned current object must remain eligible"
+        );
+        assert_eq!(lc.predict_expiration(&unversioned_current).await.action, IlmAction::DeleteAction);
+        assert!(expiration_action_has_valid_target(
+            IlmAction::DeleteAction,
+            unversioned_current.version_id,
+            unversioned_current.is_latest,
+            unversioned_current.delete_marker,
+        ));
+        assert!(!expiration_action_has_valid_target(
+            IlmAction::DeleteVersionAction,
+            unversioned_current.version_id,
+            unversioned_current.is_latest,
+            unversioned_current.delete_marker,
+        ));
+
+        let historical_null = ObjectOpts {
+            name: "object".to_string(),
+            mod_time: Some(base_time),
+            version_id: Some(Uuid::nil()),
+            is_latest: false,
+            versioned: true,
+            ..Default::default()
+        };
+        assert_eq!(
+            lc.eval_inner(&historical_null, now, 0).await.action,
+            IlmAction::NoneAction,
+            "an explicit null identity does not make a historical version current"
+        );
+        assert_eq!(lc.predict_expiration(&historical_null).await.action, IlmAction::NoneAction);
+        assert!(!expiration_action_has_valid_target(
+            IlmAction::DeleteAction,
+            historical_null.version_id,
+            historical_null.is_latest,
+            historical_null.delete_marker,
+        ));
+        assert!(expiration_action_has_valid_target(
+            IlmAction::DeleteVersionAction,
+            historical_null.version_id,
+            historical_null.is_latest,
+            historical_null.delete_marker,
+        ));
+    }
+
     /// backlog#1148 ilm-8: once a restored copy's expiry passes, the evaluator
     /// must emit `DeleteRestoredAction` for the current version (and the
     /// `...Version` variant for a noncurrent one) so the scanner removes only

crates/lifecycle/src/evaluator.rs

@@ -22,7 +22,7 @@ use rustfs_replication::ReplicationStatusType;
 use rustfs_scanner_metrics::metrics::IlmAction;
 
 use crate::object_lock;
-use crate::{Event, Lifecycle, ObjectOpts};
+use crate::{Event, Lifecycle, ObjectOpts, expiration_action_has_valid_target};
 
 const LOG_COMPONENT_ECSTORE: &str = "ecstore";
 const LOG_SUBSYSTEM_LIFECYCLE: &str = "lifecycle";
@@ -80,6 +80,9 @@ impl Evaluator {
         'top_loop: {
             for (i, obj) in objs.iter().enumerate() {
                 let mut event = self.policy.eval_inner(obj, now, newer_noncurrent_versions).await;
+                if !expiration_action_has_valid_target(event.action, obj.version_id, obj.is_latest, obj.delete_marker) {
+                    event = Event::default();
+                }
                 if lifecycle_action_waits_for_replication(event.action) && self.is_pending_replication(obj) {
                     event = Event::default();
                 }
@@ -116,16 +119,10 @@ impl Evaluator {
                     IlmAction::DeleteAction
                     | IlmAction::DeleteRestoredAction
                     | IlmAction::DeleteVersionAction
-                    | IlmAction::DeleteRestoredVersionAction => {
-                        // Defensive code, should never happen
-                        if matches!(event.action, IlmAction::DeleteVersionAction | IlmAction::DeleteRestoredVersionAction)
-                            && obj.version_id.is_none_or(|v| v.is_nil())
-                        {
-                            event.action = IlmAction::NoneAction;
-                        }
-                        if self.is_object_locked(obj) {
-                            event = Event::default();
-                        }
+                    | IlmAction::DeleteRestoredVersionAction
+                        if self.is_object_locked(obj) =>
+                    {
+                        event = Event::default();
                     }
                     _ => {}
                 }
@@ -336,6 +333,30 @@ mod tests {
         })
     }
 
+    fn current_and_noncurrent_expiration_lifecycle() -> Arc<BucketLifecycleConfiguration> {
+        Arc::new(BucketLifecycleConfiguration {
+            expiry_updated_at: None,
+            rules: vec![LifecycleRule {
+                status: ExpirationStatus::from_static(ExpirationStatus::ENABLED),
+                expiration: Some(LifecycleExpiration {
+                    days: Some(30),
+                    ..Default::default()
+                }),
+                abort_incomplete_multipart_upload: None,
+                del_marker_expiration: None,
+                filter: None,
+                id: Some("expire-current-and-noncurrent".to_string()),
+                noncurrent_version_expiration: Some(NoncurrentVersionExpiration {
+                    noncurrent_days: Some(1),
+                    newer_noncurrent_versions: None,
+                }),
+                noncurrent_version_transitions: None,
+                prefix: None,
+                transitions: None,
+            }],
+        })
+    }
+
     fn object_opts(replication_status: ReplicationStatusType, version_purge_status: VersionPurgeStatusType) -> ObjectOpts {
         ObjectOpts {
             name: "logs/object".to_string(),
@@ -541,6 +562,68 @@ mod tests {
         assert_eq!(events[0].action, IlmAction::DeleteVersionAction);
     }
 
+    #[tokio::test]
+    async fn evaluator_treats_explicit_null_as_an_exact_noncurrent_identity() {
+        let lifecycle = current_and_noncurrent_expiration_lifecycle();
+        let now = OffsetDateTime::now_utc();
+        let version_group =
+            |version_id: Option<Uuid>, replication_status: ReplicationStatusType, user_defined: HashMap<String, String>| {
+                vec![
+                    ObjectOpts {
+                        name: "logs/object".to_string(),
+                        mod_time: Some(now),
+                        version_id: Some(Uuid::new_v4()),
+                        is_latest: true,
+                        num_versions: 2,
+                        replication_status: ReplicationStatusType::Completed,
+                        ..Default::default()
+                    },
+                    ObjectOpts {
+                        name: "logs/object".to_string(),
+                        mod_time: Some(now - time::Duration::days(40)),
+                        successor_mod_time: Some(now - time::Duration::days(3)),
+                        version_id,
+                        is_latest: false,
+                        num_versions: 2,
+                        versioned: true,
+                        replication_status,
+                        user_defined,
+                        ..Default::default()
+                    },
+                ]
+            };
+
+        let events = Evaluator::new(lifecycle.clone())
+            .eval(&version_group(Some(Uuid::nil()), ReplicationStatusType::Completed, HashMap::new()))
+            .await
+            .expect("explicit null-version lifecycle evaluation should succeed");
+        assert_eq!(events[0].action, IlmAction::NoneAction);
+        assert_eq!(events[1].action, IlmAction::DeleteVersionAction);
+
+        let events = Evaluator::new(lifecycle.clone())
+            .eval(&version_group(None, ReplicationStatusType::Completed, HashMap::new()))
+            .await
+            .expect("missing historical identity should fail closed without aborting evaluation");
+        assert_eq!(events[1].action, IlmAction::NoneAction);
+
+        let events = Evaluator::new(lifecycle.clone())
+            .eval(&version_group(Some(Uuid::nil()), ReplicationStatusType::Pending, HashMap::new()))
+            .await
+            .expect("pending null-version replication should fail closed without aborting evaluation");
+        assert_eq!(events[1].action, IlmAction::NoneAction);
+
+        let events = Evaluator::new(lifecycle)
+            .with_lock_retention(Some(lock_enabled_without_default_retention()))
+            .eval(&version_group(
+                Some(Uuid::nil()),
+                ReplicationStatusType::Completed,
+                HashMap::from([(X_AMZ_OBJECT_LOCK_LEGAL_HOLD.as_str().to_string(), "ON".to_string())]),
+            ))
+            .await
+            .expect("locked null-version lifecycle evaluation should fail closed without aborting evaluation");
+        assert_eq!(events[1].action, IlmAction::NoneAction);
+    }
+
     #[tokio::test]
     async fn evaluator_skips_transition_while_replication_pending() {
         let evaluator = Evaluator::new(latest_transition_lifecycle());