diff --git a/icechunk/src/ops/gc.rs b/icechunk/src/ops/gc.rs index d83261607..109b8662f 100644 --- a/icechunk/src/ops/gc.rs +++ b/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?; diff --git a/icechunk/tests/test_gc.rs b/icechunk/tests/test_gc.rs index f132b8b67..e1700037f 100644 --- a/icechunk/tests/test_gc.rs +++ b/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> { Ok(()) } +#[tokio::test] +pub async fn test_expire_ref_with_odd_timestamp() -> Result<(), Box> +{ + let storage: Arc = 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. ///