Fix repo serialization with default commit metadata

icechunk-python/src/repository.rs

@@ -553,18 +553,27 @@ impl PyRepository {
     #[pyo3(signature = (metadata))]
     pub fn set_default_commit_metadata(
         &self,
+        py: Python<'_>,
         metadata: Option<PySnapshotProperties>,
     ) -> PyResult<()> {
         let metadata = metadata.map(|m| m.into());
-        pyo3_async_runtimes::tokio::get_runtime().block_on(async move {
-            self.0.set_default_commit_metadata(metadata).await;
+        py.allow_threads(move || {
+            self.0
+                .set_default_commit_metadata(metadata)
+                .map_err(PyIcechunkStoreError::RepositoryError)?;
             Ok(())
         })
     }
 
-    pub fn default_commit_metadata(&self) -> PyResult<Option<PySnapshotProperties>> {
-        pyo3_async_runtimes::tokio::get_runtime().block_on(async move {
-            let metadata = self.0.default_commit_metadata().await;
+    pub fn default_commit_metadata(
+        &self,
+        py: Python<'_>,
+    ) -> PyResult<Option<PySnapshotProperties>> {
+        py.allow_threads(move || {
+            let metadata = self
+                .0
+                .default_commit_metadata()
+                .map_err(PyIcechunkStoreError::RepositoryError)?;
             Ok(metadata.map(|m| m.into()))
         })
     }

icechunk-python/tests/test_pickle.py

@@ -15,7 +15,9 @@
 
 
 def create_local_repo(path: str) -> Repository:
-    return Repository.create(storage=local_filesystem_storage(path))
+    repo = Repository.create(storage=local_filesystem_storage(path))
+    repo.set_default_commit_metadata({"author": "test"})
+    return repo
 
 
 @pytest.fixture(scope="function")
@@ -29,6 +31,8 @@ def test_pickle_repository(tmpdir: Path, tmp_repo: Repository) -> None:
     pickled = pickle.dumps(tmp_repo)
     roundtripped = pickle.loads(pickled)
     assert tmp_repo.list_branches() == roundtripped.list_branches()
+    assert tmp_repo.default_commit_metadata() == roundtripped.default_commit_metadata()
+    assert tmp_repo.default_commit_metadata() == {"author": "test"}
 
     storage = tmp_repo.storage
     assert (

icechunk/src/repository.rs

@@ -2,7 +2,7 @@ use std::{
     collections::{BTreeSet, HashMap, HashSet},
     future::ready,
     ops::RangeBounds,
-    sync::Arc,
+    sync::{Arc, Mutex},
 };
 
 use async_recursion::async_recursion;
@@ -16,7 +16,6 @@ use futures::{
 use regex::bytes::Regex;
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
-use tokio::sync::RwLock;
 use tokio::task::JoinError;
 use tracing::{Instrument, debug, error, instrument, trace};
 
@@ -60,7 +59,8 @@ pub enum RepositoryErrorKind {
     FormatError(IcechunkFormatErrorKind),
     #[error(transparent)]
     Ref(RefErrorKind),
-
+    #[error("failed to obtain mutex lock")]
+    SyncError,
     #[error("snapshot not found: `{id}`")]
     SnapshotNotFound { id: SnapshotId },
     #[error("branch {branch} does not have a snapshots before or at {at}")]
@@ -139,8 +139,7 @@ pub struct Repository {
     asset_manager: Arc<AssetManager>,
     virtual_resolver: Arc<VirtualChunkResolver>,
     virtual_chunk_credentials: HashMap<ContainerName, Credentials>,
-    #[serde(skip)]
-    default_commit_metadata: Arc<RwLock<Option<SnapshotProperties>>>,
+    default_commit_metadata: Arc<Mutex<Option<SnapshotProperties>>>,
 }
 
 impl Repository {
@@ -312,7 +311,7 @@ impl Repository {
             virtual_resolver,
             asset_manager,
             virtual_chunk_credentials,
-            default_commit_metadata: Arc::new(RwLock::new(None)),
+            default_commit_metadata: Arc::new(Mutex::new(None)),
         })
     }
 
@@ -382,17 +381,27 @@ impl Repository {
     }
 
     #[instrument(skip_all)]
-    pub async fn set_default_commit_metadata(
+    pub fn set_default_commit_metadata(
         &self,
         metadata: Option<SnapshotProperties>,
-    ) {
-        let mut guard = self.default_commit_metadata.write().await;
+    ) -> RepositoryResult<()> {
+        let mut guard = self
+            .default_commit_metadata
+            .lock()
+            .map_err(|_| RepositoryErrorKind::SyncError)?;
         *guard = metadata;
+        Ok(())
     }
 
     #[instrument(skip_all)]
-    pub async fn default_commit_metadata(&self) -> Option<SnapshotProperties> {
-        self.default_commit_metadata.read().await.clone()
+    pub fn default_commit_metadata(
+        &self,
+    ) -> RepositoryResult<Option<SnapshotProperties>> {
+        let guard = self
+            .default_commit_metadata
+            .lock()
+            .map_err(|_| RepositoryErrorKind::SyncError)?;
+        Ok(guard.clone())
     }
 
     #[instrument(skip(storage, config))]
@@ -774,7 +783,10 @@ impl Repository {
             self.virtual_resolver.clone(),
             branch.to_string(),
             ref_data.snapshot.clone(),
-            self.default_commit_metadata.read().await.clone(),
+            self.default_commit_metadata
+                .lock()
+                .map_err(|_| RepositoryErrorKind::SyncError)?
+                .clone(),
         );
 
         self.preload_manifests(ref_data.snapshot);

icechunk/src/session.rs

@@ -1983,7 +1983,7 @@ mod tests {
         let mut default_metadata = SnapshotProperties::default();
         default_metadata.insert("author".to_string(), "John Doe".to_string().into());
         default_metadata.insert("project".to_string(), "My Project".to_string().into());
-        repo.set_default_commit_metadata(Some(default_metadata.clone())).await;
+        repo.set_default_commit_metadata(Some(default_metadata.clone()))?;
 
         let mut ds = repo.writable_session("main").await?;
         ds.add_group("/group".try_into().unwrap(), Bytes::new()).await?;