storage: Rewrite temporary repositories for clones

On Windows, all attempts to clone repositories failed with

    Fetch failed for rad:… from z6Mk…: Access is denied. (os error 5)

The reason is that, other than Unix-like systems, it forbids that
directories that are in use are moved.

To improve the situation, take back control over what is moved and
removed exactly by implementing `cleanup` instead of relying
on `impl Drop for tempfile::TempDir`.

A new type `TempRepository` is introduced to capture a repository that can be
used temporarily, and either cleaned up on failure, or moved on success.
This `TempRepository` can be constructed using `Storage::temporary_repository` –
renamed from `Storage::lock_repository` since it is only creating a temporary
resource and not locking it.

In the future (once Rust 1.89 is a little less cutting edge and more
widely available), we may opt for actual locking via
`std::fs::File::lock`.

crates/radicle-cli/tests/commands.rs

@@ -1375,7 +1375,7 @@ fn rad_clone_partial_fail() {
     eve.connect(&bob);
     eve.routes_to(&[(acme, carol), (acme, bob.id), (acme, alice.id)]);
     bob.storage.repository(acme).unwrap().remove().unwrap(); // Cause the fetch from Bob to fail.
-    bob.storage.lock_repository(acme).ok(); // Prevent repo from being re-fetched.
+    bob.storage.temporary_repository(acme).ok(); // Prevent repo from being re-fetched.
 
     test(
         "examples/rad-clone-partial-fail.md",

crates/radicle-node/src/worker/fetch.rs

@@ -1,5 +1,6 @@
 use radicle::identity::doc::CanonicalRefsError;
 use radicle::identity::CanonicalRefs;
+use radicle::storage::git::TempRepository;
 pub(crate) use radicle_protocol::worker::fetch::error;
 
 use std::collections::BTreeSet;
@@ -27,8 +28,7 @@ use super::channels::ChannelsFlush;
 
 pub enum Handle {
     Clone {
-        handle: radicle_fetch::Handle<Repository, ChannelsFlush>,
+        handle: radicle_fetch::Handle<TempRepository, ChannelsFlush>,
-        tmp: tempfile::TempDir,
     },
     Pull {
         handle: radicle_fetch::Handle<Repository, ChannelsFlush>,
@@ -55,9 +55,9 @@ impl Handle {
                 notifications,
             })
         } else {
-            let (repo, tmp) = storage.lock_repository(rid)?;
+            let repo = storage.temporary_repository(rid)?;
             let handle = radicle_fetch::Handle::new(local, repo, follow, blocked, channels)?;
-            Ok(Handle::Clone { handle, tmp })
+            Ok(Handle::Clone { handle })
         }
     }
 
@@ -72,11 +72,18 @@ impl Handle {
         refs_at: Option<Vec<RefsAt>>,
     ) -> Result<FetchResult, error::Fetch> {
         let (result, clone, notifs) = match self {
-            Self::Clone { mut handle, tmp } => {
+            Self::Clone { mut handle } => {
                 log::debug!(target: "worker", "{} cloning from {remote}", handle.local());
-                let result = radicle_fetch::clone(&mut handle, limit, remote)?;
+                match radicle_fetch::clone(&mut handle, limit, remote) {
-                mv(tmp, storage, &rid)?;
+                    Err(err) => {
-                (result, true, None)
+                        handle.into_inner().cleanup();
+                        return Err(err.into());
+                    }
+                    Ok(result) => {
+                        handle.into_inner().mv(storage.path_of(&rid))?;
+                        (result, true, None)
+                    }
+                }
             }
             Self::Pull {
                 mut handle,
@@ -164,35 +171,6 @@ impl Handle {
     }
 }
 
-/// In the case of cloning, we have performed the fetch into a
-/// temporary directory -- ensuring that no concurrent operations
-/// see an empty repository.
-///
-/// At the end of the clone, we perform a rename of the temporary
-/// directory to the storage repository.
-///
-/// # Errors
-///   - Will fail if `storage` contains `rid` already.
-fn mv(tmp: tempfile::TempDir, storage: &Storage, rid: &RepoId) -> Result<(), error::Fetch> {
-    use std::io::{Error, ErrorKind};
-
-    let from = tmp.path();
-    let to = storage.path_of(rid);
-
-    if !to.exists() {
-        std::fs::rename(from, to)?;
-    } else {
-        log::warn!(target: "worker", "Refusing to move cloned repository {rid} already exists");
-        return Err(Error::new(
-            ErrorKind::AlreadyExists,
-            format!("repository already exists {to:?}"),
-        )
-        .into());
-    }
-
-    Ok(())
-}
-
 // Post notifications for the given refs.
 fn notify(
     rid: &RepoId,

crates/radicle/Cargo.toml

@@ -11,7 +11,7 @@ rust-version.workspace = true
 
 [features]
 default = []
-test = ["qcheck", "radicle-crypto/test"]
+test = ["tempfile", "qcheck", "radicle-crypto/test"]
 logger = ["colored", "chrono"]
 
 [dependencies]
@@ -42,7 +42,7 @@ serde_json = { workspace = true, features = ["preserve_order"] }
 serde-untagged = "0.1.7"
 siphasher = "1.0.0"
 sqlite = { workspace = true, features = ["bundled"] }
-tempfile = { workspace = true }
+tempfile = { workspace = true, optional = true }
 thiserror = { workspace = true }
 unicode-normalization = { version = "0.1" }
 

crates/radicle/src/storage/git.rs

@@ -2,6 +2,9 @@
 pub mod cob;
 pub mod transport;
 
+pub mod temp;
+pub use temp::TempRepository;
+
 use std::collections::{BTreeMap, BTreeSet, HashMap};
 use std::ops::{Deref, DerefMut};
 use std::path::{Path, PathBuf};
@@ -9,7 +12,6 @@ use std::sync::LazyLock;
 use std::{fs, io};
 
 use crypto::Verified;
-use tempfile::TempDir;
 
 use crate::git::canonical::Quorum;
 use crate::identity::crefs::GetCanonicalRefs as _;
@@ -121,9 +123,9 @@ impl ReadStorage for Storage {
             if path.file_name().to_string_lossy().starts_with('.') {
                 continue;
             }
-            // Skip lock files.
+            // Skip temporary repositories
             if let Some(ext) = path.path().extension() {
-                if ext == "lock" {
+                if ext == TempRepository::EXT {
                     continue;
                 }
             }
@@ -213,23 +215,18 @@ impl Storage {
 
     /// Create a [`Repository`] in a temporary directory.
     ///
-    /// N.b. it is important to keep the [`TempDir`] in scope while
+    /// This is used to prevent other processes accessing it during
-    /// using the [`Repository`]. If it is dropped, any action on the
+    /// initialization. Usually, callers will want to move the repository
-    /// `Repository` will fail.
+    /// to its destination after initialization in the temporary location.
-    pub fn lock_repository(&self, rid: RepoId) -> Result<(Repository, TempDir), RepositoryError> {
+    pub fn temporary_repository(&self, rid: RepoId) -> Result<TempRepository, RepositoryError> {
         if self.contains(&rid)? {
             return Err(Error::Io(io::Error::new(
                 io::ErrorKind::AlreadyExists,
-                format!("refusing to create '{rid}.lock'"),
+                format!("refusing to create temporary repository for {rid}"),
             ))
             .into());
         }
-        let tmp = tempfile::Builder::new()
+        TempRepository::new(self.path(), rid, &self.info)
-            .prefix(&rid.canonical())
-            .suffix(".lock")
-            .tempdir_in(self.path())
-            .map_err(Error::from)?;
-        Ok((Repository::create(tmp.path(), rid, &self.info)?, tmp))
     }
 
     pub fn path(&self) -> &Path {

crates/radicle/src/storage/git/temp.rs

@@ -0,0 +1,87 @@
+use std::io;
+use std::path::{Path, PathBuf};
+
+use crate::prelude::RepoId;
+
+use super::{Repository, RepositoryError, UserInfo};
+
+/// A [`Repository`] that is created for temporary operations, such as cloning.
+///
+/// When the `TempRepository` is no longer needed, then call one of destructors:
+///
+///   - [`TempRepository::cleanup`]: remove the repository directory
+///   - [`TempRepository::mv`]: move the repository directory to a final
+///     destination and remove the old directory
+///
+/// [`TempRepository`] implements [`AsRef`] so that the [`Repository`] can be
+/// used in places where a [`Repository`] is needed.
+pub struct TempRepository {
+    repo: Repository,
+    path: PathBuf,
+}
+
+impl TempRepository {
+    /// Extension used for the directory
+    pub(crate) const EXT: &str = "tmp";
+    const RANDOMNESS_LENGTH: usize = 6;
+
+    pub(super) fn new<P>(root: P, rid: RepoId, info: &UserInfo) -> Result<Self, RepositoryError>
+    where
+        P: AsRef<Path>,
+    {
+        let random: String = std::iter::repeat_with(fastrand::alphanumeric)
+            .take(Self::RANDOMNESS_LENGTH)
+            .collect();
+        let path = root
+            .as_ref()
+            .join(format!("{rid}.{random}"))
+            .with_extension(Self::EXT);
+        let repo = Repository::create(&path, rid, info)?;
+        Ok(Self { repo, path })
+    }
+
+    /// Clean up the temporary directory of the repository.
+    ///
+    /// Note that the repository is dropped first to ensure that there are no
+    /// handles to the repository, before removing the directory.
+    pub fn cleanup(self) {
+        let path = self.path.clone();
+        drop(self.repo);
+        Self::remove(&path)
+    }
+
+    /// Move the temporary directory of the repository to the new path.
+    ///
+    /// If `to` already exists, then the temporary directory is removed, and the
+    /// repository is not moved.
+    ///
+    /// Note that the repository is dropped first to ensure that there are no
+    /// handles to the repository, before removing the directory.
+    pub fn mv<P>(self, to: P) -> io::Result<()>
+    where
+        P: AsRef<Path>,
+    {
+        let to = to.as_ref();
+        let rid = self.repo.id;
+        let path = self.path.clone();
+        drop(self.repo);
+        if to.exists() {
+            log::warn!(target: "radicle", "Refusing to move from temporary directory '{}' because destination {rid} already exists. Removing the temporary directory.", self.path.display());
+            Self::remove(&path);
+        }
+        std::fs::rename(path, to)
+    }
+
+    fn remove(path: &PathBuf) {
+        if let Err(err) = std::fs::remove_dir_all(path) {
+            let path = path.display();
+            log::error!(target: "worker", "Failed to remove temporary directory '{path}': {err}");
+        }
+    }
+}
+
+impl AsRef<Repository> for TempRepository {
+    fn as_ref(&self) -> &Repository {
+        &self.repo
+    }
+}