Fix ref delete during ref expiration (#873)

Author: paraseba

icechunk/src/ops/gc.rs

@@ -344,7 +344,9 @@ async fn gc_transaction_logs(
 
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum ExpireRefResult {
-    NothingToDo,
+    NothingToDo {
+        ref_is_expired: bool,
+    },
     Done {
         released_snapshots: HashSet<SnapshotId>,
         edited_snapshot: SnapshotId,
@@ -427,7 +429,7 @@ pub async fn expire_ref(
     {
         // Either the reference is the root, or it is pointing to the root as first parent
         // Nothing to do
-        return Ok(ExpireRefResult::NothingToDo);
+        return Ok(ExpireRefResult::NothingToDo { ref_is_expired });
     }
 
     let root = asset_manager.fetch_snapshot(&root).await?;
@@ -501,44 +503,41 @@ pub async fn expire(
             }
         })
         .try_fold(ExpireResult::default(), |mut result, (r, ref_result)| async move {
-            match ref_result {
+            let ref_is_expired = match ref_result {
                 ExpireRefResult::Done {
                     released_snapshots,
                     edited_snapshot,
                     ref_is_expired,
                 } => {
                     result.released_snapshots.extend(released_snapshots.into_iter());
                     result.edited_snapshots.insert(edited_snapshot);
-                    if ref_is_expired {
-                        match &r {
-                            Ref::Tag(name) => {
-                                if expired_tags == ExpiredRefAction::Delete {
-                                    delete_tag(storage, storage_settings, name.as_str())
-                                        .await
-                                        .map_err(GCError::Ref)?;
-                                    result.deleted_refs.insert(r);
-                                }
-                            }
-                            Ref::Branch(name) => {
-                                if expired_branches == ExpiredRefAction::Delete
-                                    && name != Ref::DEFAULT_BRANCH
-                                {
-                                    delete_branch(
-                                        storage,
-                                        storage_settings,
-                                        name.as_str(),
-                                    )
-                                    .await
-                                    .map_err(GCError::Ref)?;
-                                    result.deleted_refs.insert(r);
-                                }
-                            }
+                    ref_is_expired
+                }
+                ExpireRefResult::NothingToDo { ref_is_expired } => ref_is_expired,
+            };
+            if ref_is_expired {
+                match &r {
+                    Ref::Tag(name) => {
+                        if expired_tags == ExpiredRefAction::Delete {
+                            delete_tag(storage, storage_settings, name.as_str())
+                                .await
+                                .map_err(GCError::Ref)?;
+                            result.deleted_refs.insert(r);
+                        }
+                    }
+                    Ref::Branch(name) => {
+                        if expired_branches == ExpiredRefAction::Delete
+                            && name != Ref::DEFAULT_BRANCH
+                        {
+                            delete_branch(storage, storage_settings, name.as_str())
+                                .await
+                                .map_err(GCError::Ref)?;
+                            result.deleted_refs.insert(r);
                         }
                     }
-                    Ok(result)
                 }
-                ExpireRefResult::NothingToDo => Ok(result),
             }
+            Ok(result)
         })
         .await
 }

icechunk/tests/test_gc.rs

@@ -293,7 +293,7 @@ pub async fn test_expire_ref() -> Result<(), Box<dyn std::error::Error>> {
     )
     .await?
     {
-        ExpireRefResult::NothingToDo => {
+        ExpireRefResult::NothingToDo { .. } => {
             panic!()
         }
         ExpireRefResult::Done { released_snapshots, ref_is_expired, .. } => {
@@ -330,7 +330,7 @@ pub async fn test_expire_ref() -> Result<(), Box<dyn std::error::Error>> {
     )
     .await?
     {
-        ExpireRefResult::NothingToDo => panic!(),
+        ExpireRefResult::NothingToDo { .. } => panic!(),
         ExpireRefResult::Done { released_snapshots, ref_is_expired, .. } => {
             assert!(!ref_is_expired);
             assert_eq!(released_snapshots.len(), 4);
@@ -365,7 +365,7 @@ pub async fn test_expire_ref() -> Result<(), Box<dyn std::error::Error>> {
     )
     .await?
     {
-        ExpireRefResult::NothingToDo => panic!(),
+        ExpireRefResult::NothingToDo { .. } => panic!(),
         ExpireRefResult::Done { released_snapshots, ref_is_expired, .. } => {
             assert!(!ref_is_expired);
             assert_eq!(released_snapshots.len(), 5);
@@ -400,7 +400,7 @@ pub async fn test_expire_ref() -> Result<(), Box<dyn std::error::Error>> {
     )
     .await?
     {
-        ExpireRefResult::NothingToDo => panic!(),
+        ExpireRefResult::NothingToDo { .. } => panic!(),
         ExpireRefResult::Done { released_snapshots, ref_is_expired, .. } => {
             assert!(!ref_is_expired);
             assert_eq!(released_snapshots.len(), 5);
@@ -462,7 +462,9 @@ pub async fn test_expire_ref_with_odd_timestamps()
     )
     .await?
     {
-        ExpireRefResult::NothingToDo => {}
+        ExpireRefResult::NothingToDo { ref_is_expired } => {
+            assert!(!ref_is_expired);
+        }
         _ => panic!(),
     }