From a2e79071938342f663048d9ce250d0db1d067d2f Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Fri, 16 Oct 2026 10:26:40 +0700 Subject: [PATCH] fix: serialize concurrent clones & recover from leftover dirs --- CHANGELOG.md | 3 ++ crates/signed_git/src/cache.rs | 36 +++++++++++++++--- crates/signed_git/src/tests.rs | 67 ++++++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3019471..9cc8023 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,11 +26,14 @@ - Restructure the core crate into focused modules for announcements, pull requests, git events, inbox, and upstream state - Publish nostr events directly through the client with relay acceptance required, removing the grasp push helper and its per-server retry loop so failures surface immediately - Poll the repository sync drift check every 180 seconds instead of 60 +- Track sync status per view instead of globally - Merge the `PixelAvatar` into the `Avatar` component. Seed user pixel avatars from the public key instead of the display name, so avatars stay stable when a user renames ### Fixed - Fix the repository view refreshing its mirror from the network while a push is in progress, so the refresh no longer races the push +- Fix opening a repository failing with a `destination already exists` error when two loads raced to clone its mirror, clones are now serialized and later loads reuse the finished clone +- Fix a repository never opening again after an interrupted clone left an unusable mirror directory behind, which is now removed and re-cloned - Fix signing in with a `bunker://` URI silently generating a new identity instead of erroring when its master key cannot be parsed ### Removed diff --git a/crates/signed_git/src/cache.rs b/crates/signed_git/src/cache.rs index dfb40fa..5b5c51d 100644 --- a/crates/signed_git/src/cache.rs +++ b/crates/signed_git/src/cache.rs @@ -1,6 +1,8 @@ +use std::collections::HashMap; use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex, PoisonError}; -use anyhow::{Context, Result}; +use anyhow::Result; use signed_core::{Announcement, RepoAddr}; use crate::repo::Repo; @@ -8,12 +10,16 @@ use crate::repo::Repo; #[derive(Debug, Clone)] pub struct GitCache { root: PathBuf, + clone_locks: Arc>>>>, } impl GitCache { /// Creates a cache rooted at `root`. pub fn new(root: PathBuf) -> Self { - Self { root } + Self { + root, + clone_locks: Arc::default(), + } } /// Returns the cache root directory. @@ -38,20 +44,40 @@ impl GitCache { } } + /// Returns the clone lock for `path`, creating it on first use. + fn clone_lock(&self, path: &Path) -> Arc> { + let mut locks = self + .clone_locks + .lock() + .unwrap_or_else(PoisonError::into_inner); + Arc::clone(locks.entry(path.to_path_buf()).or_default()) + } + /// Returns the cached repository, cloning from one of `clone_urls` on first use. pub fn ensure_clone>(&self, addr: &RepoAddr, clone_urls: &[U]) -> Result { let path = self.repo_path(addr); - if let Some(repo) = self.open(addr)? { + // Lock the clone lock for this path to prevent concurrent clones. + let clone_lock = self.clone_lock(&path); + let _clone_guard = clone_lock.lock().unwrap_or_else(PoisonError::into_inner); + + // Refreshes the cached repository if it is already cloned, or clones it from one of `clone_urls` otherwise. + if let Ok(Some(repo)) = self.open(addr) { if let Err(error) = repo.fetch() { log::warn!("failed to refresh the cached repository: {error:#}"); } return Ok(repo); } + // Create the parent directory if it does not exist. if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent) - .with_context(|| format!("failed to create {}", parent.display()))?; + std::fs::create_dir_all(parent)?; + } + + // Remove the cached clone if it exists but is unusable. + if path.exists() { + log::warn!("removing unusable cached clone at {}", path.display()); + std::fs::remove_dir_all(&path)?; } Repo::clone(clone_urls, &path) diff --git a/crates/signed_git/src/tests.rs b/crates/signed_git/src/tests.rs index 8e4a8d0..0805cdf 100644 --- a/crates/signed_git/src/tests.rs +++ b/crates/signed_git/src/tests.rs @@ -1,6 +1,9 @@ use std::path::{Path, PathBuf}; use std::process::Command; +use nostr::prelude::Keys; +use signed_core::RepoAddr; + use super::*; #[test] @@ -11,6 +14,70 @@ fn blocks_parent_components() { assert_eq!(GitCache::sanitize_path_component("a/../b"), "a_.._b"); } +#[test] +fn concurrent_ensure_clone_succeeds_for_every_caller() { + let server = tempfile::tempdir().expect("tempdir"); + let server_repo = bare_server(server.path(), "npub1test", "repo"); + + let (work_dir, work_repo) = fixture(&[("a.txt", b"one")]); + commit_all(&work_repo, "initial"); + let work_dir = work_dir.path(); + + let base_url = format!("file://{}", server.path().display()); + Repo::open(work_dir) + .expect("open") + .push_all(&base_url, "npub1test", "repo") + .expect("push"); + + let cache_dir = tempfile::tempdir().expect("tempdir"); + let cache = GitCache::new(cache_dir.path().to_path_buf()); + let addr = RepoAddr::new(Keys::generate().public_key(), "repo"); + let clone_url = format!("file://{}", server_repo.display()); + + std::thread::scope(|scope| { + for _ in 0..4 { + scope.spawn(|| { + cache + .ensure_clone(&addr, &[&clone_url]) + .expect("ensure_clone"); + }); + } + }); + + assert!(cache.open(&addr).expect("open").is_some()); +} + +#[test] +fn ensure_clone_recovers_from_a_leftover_directory() { + let server = tempfile::tempdir().expect("tempdir"); + let server_repo = bare_server(server.path(), "npub1test", "repo"); + + let (work_dir, work_repo) = fixture(&[("a.txt", b"one")]); + commit_all(&work_repo, "initial"); + let work_dir = work_dir.path(); + + let base_url = format!("file://{}", server.path().display()); + Repo::open(work_dir) + .expect("open") + .push_all(&base_url, "npub1test", "repo") + .expect("push"); + + let cache_dir = tempfile::tempdir().expect("tempdir"); + let cache = GitCache::new(cache_dir.path().to_path_buf()); + let addr = RepoAddr::new(Keys::generate().public_key(), "repo"); + let clone_url = format!("file://{}", server_repo.display()); + + // Simulate the remains of an interrupted clone. + let leftover = cache.repo_path(&addr); + std::fs::create_dir_all(&leftover).expect("mkdir"); + + cache + .ensure_clone(&addr, &[&clone_url]) + .expect("ensure_clone"); + + assert!(cache.open(&addr).expect("open").is_some()); +} + #[test] fn root_commit_reports_the_first_ancestor() { let (dir, repo) = fixture(&[("a.txt", b"one")]);