diff --git a/crates/radicle-cli/tests/commands.rs b/crates/radicle-cli/tests/commands.rs index ddef19be..4fbc0e24 100644 --- a/crates/radicle-cli/tests/commands.rs +++ b/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", diff --git a/crates/radicle-node/src/worker/fetch.rs b/crates/radicle-node/src/worker/fetch.rs index 159e8afa..a52f255a 100644 --- a/crates/radicle-node/src/worker/fetch.rs +++ b/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, - tmp: tempfile::TempDir, + handle: radicle_fetch::Handle, }, Pull { handle: radicle_fetch::Handle, @@ -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>, ) -> Result { 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)?; - mv(tmp, storage, &rid)?; - (result, true, None) + match radicle_fetch::clone(&mut handle, limit, remote) { + Err(err) => { + 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, diff --git a/crates/radicle/Cargo.toml b/crates/radicle/Cargo.toml index 827f9e91..e50d4605 100644 --- a/crates/radicle/Cargo.toml +++ b/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" } diff --git a/crates/radicle/src/storage/git.rs b/crates/radicle/src/storage/git.rs index cfea5837..00e04c9f 100644 --- a/crates/radicle/src/storage/git.rs +++ b/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 - /// using the [`Repository`]. If it is dropped, any action on the - /// `Repository` will fail. - pub fn lock_repository(&self, rid: RepoId) -> Result<(Repository, TempDir), RepositoryError> { + /// This is used to prevent other processes accessing it during + /// initialization. Usually, callers will want to move the repository + /// to its destination after initialization in the temporary location. + pub fn temporary_repository(&self, rid: RepoId) -> Result { 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() - .prefix(&rid.canonical()) - .suffix(".lock") - .tempdir_in(self.path()) - .map_err(Error::from)?; - Ok((Repository::create(tmp.path(), rid, &self.info)?, tmp)) + TempRepository::new(self.path(), rid, &self.info) } pub fn path(&self) -> &Path { diff --git a/crates/radicle/src/storage/git/temp.rs b/crates/radicle/src/storage/git/temp.rs new file mode 100644 index 00000000..d5e9798c --- /dev/null +++ b/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

(root: P, rid: RepoId, info: &UserInfo) -> Result + where + P: AsRef, + { + 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

(self, to: P) -> io::Result<()> + where + P: AsRef, + { + 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 for TempRepository { + fn as_ref(&self) -> &Repository { + &self.repo + } +}