Fix bug in expiration that creates a commit loop

icechunk/src/ops/gc.rs

@@ -415,14 +415,27 @@ pub async fn expire_ref(
     released.remove(&root);
 
     let editable_snap = asset_manager.fetch_snapshot(&editable_snap).await?;
-    let parent_id = editable_snap.parent_id();
-    if editable_snap.id() == root || Some(&root) == parent_id.as_ref() {
+
+    let old_parent_id = editable_snap.parent_id();
+    if editable_snap.id() == root    // only root can be expired
+        || Some(&root) == old_parent_id.as_ref()
+        // we never found an expirable snap
+        || root == snap_id
+    {
         // Either the reference is the root, or it is pointing to the root as first parent
         // Nothing to do
         return Ok(ExpireRefResult::NothingToDo);
     }
 
     let root = asset_manager.fetch_snapshot(&root).await?;
+    // we don't want to create loops, so:
+    // we never edit the root of a tree
+    assert!(editable_snap.parent_id().is_some());
+    // and, we only set a root as parent
+    assert!(root.parent_id().is_none());
+
+    assert!(editable_snap.flushed_at()? >= older_than);
+
     // TODO: add properties to the snapshot that tell us it was history edited
     let new_snapshot = Arc::new(root.adopt(&editable_snap)?);
     asset_manager.write_snapshot(new_snapshot).await?;

icechunk/tests/test_gc.rs

@@ -3,7 +3,7 @@
 use std::{collections::HashMap, sync::Arc};
 
 use bytes::Bytes;
-use chrono::{DateTime, Utc};
+use chrono::{DateTime, TimeDelta, Utc};
 use futures::{StreamExt, TryStreamExt};
 use icechunk::{
     Repository, RepositoryConfig, Storage,
@@ -431,6 +431,77 @@ pub async fn test_expire_ref() -> Result<(), Box<dyn std::error::Error>> {
     Ok(())
 }
 
+#[tokio::test]
+pub async fn test_expire_ref_with_odd_timestamp() -> Result<(), Box<dyn std::error::Error>>
+{
+    let storage: Arc<dyn Storage + Send + Sync> = new_in_memory_storage().await?;
+    let storage_settings = storage.default_settings();
+    let repo = Repository::create(None, Arc::clone(&storage), HashMap::new()).await?;
+
+    let mut session = repo.writable_session("main").await?;
+
+    let user_data = Bytes::new();
+    session.add_group(Path::root(), user_data.clone()).await?;
+    session.commit("first", None).await?;
+    let mut session = repo.writable_session("main").await?;
+    session.add_group("/a".try_into().unwrap(), user_data.clone()).await?;
+    session.commit("second", None).await?;
+    let mut session = repo.writable_session("main").await?;
+    session.add_group("/b".try_into().unwrap(), user_data.clone()).await?;
+    session.commit("third", None).await?;
+
+    let asset_manager = Arc::new(AssetManager::new_no_cache(
+        storage.clone(),
+        storage_settings.clone(),
+        1,
+    ));
+    match expire_ref(
+        storage.as_ref(),
+        &storage_settings,
+        asset_manager.clone(),
+        &Ref::Branch("main".to_string()),
+        Utc::now() - TimeDelta::days(10),
+    )
+    .await?
+    {
+        ExpireRefResult::NothingToDo => {}
+        _ => panic!(),
+    }
+
+    // create another repo to avoid caching issues
+    let repo = Repository::open(None, Arc::clone(&storage), HashMap::new()).await?;
+    assert_eq!(
+        branch_commit_messages(&repo, "main").await,
+        Vec::from(["third", "second", "first", "Repository initialized"])
+    );
+
+    let asset_manager = Arc::new(AssetManager::new_no_cache(
+        storage.clone(),
+        storage_settings.clone(),
+        1,
+    ));
+    match expire_ref(
+        storage.as_ref(),
+        &storage_settings,
+        asset_manager.clone(),
+        &Ref::Branch("main".to_string()),
+        Utc::now() + TimeDelta::days(10),
+    )
+    .await?
+    {
+        ExpireRefResult::RefIsExpired => {}
+        _ => panic!(),
+    }
+
+    // create another repo to avoid caching issues
+    let repo = Repository::open(None, Arc::clone(&storage), HashMap::new()).await?;
+    assert_eq!(
+        branch_commit_messages(&repo, "main").await,
+        Vec::from(["third", "second", "first", "Repository initialized"])
+    );
+    Ok(())
+}
+
 #[tokio::test]
 /// In this test, we set up a repo as in the design document for expiration.
 ///