Radish alpha
h
Radicle Heartwood Protocol & Stack
Radicle
Git (anonymous pull)
Log in to clone via SSH
storage: Repository locking for Windows
◌ CI pending Lorenz Leutgeb committed 9 months ago
commit 4d99a95408828a630587989607bf6ab1f99dfe1f
parent 19a262d3d67ea92a37ceaf18e79a783d8e65ccdd
1 pending (1 total) View logs
2 files changed +84 -17
modified crates/radicle-node/src/worker/fetch.rs
@@ -10,6 +10,7 @@ use radicle::identity::crefs::GetCanonicalRefs as _;
use radicle::prelude::NodeId;
use radicle::prelude::RepoId;
use radicle::storage::git::Repository;
+
use radicle::storage::git::TemporaryPath;
use radicle::storage::refs::RefsAt;
use radicle::storage::{
    ReadRepository, ReadStorage as _, RefUpdate, RemoteRepository, RepositoryError,
@@ -25,7 +26,7 @@ use super::channels::ChannelsFlush;
pub enum Handle {
    Clone {
        handle: radicle_fetch::Handle<ChannelsFlush>,
-
        tmp: tempfile::TempDir,
+
        tmp: TemporaryPath,
    },
    Pull {
        handle: radicle_fetch::Handle<ChannelsFlush>,
@@ -71,9 +72,17 @@ impl Handle {
        let (result, clone, notifs) = match self {
            Self::Clone { mut handle, tmp } => {
                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) => {
+
                        #[cfg(windows)]
+
                        cleanup(tmp.path());
+
                        return Err(err.into());
+
                    }
+
                    Ok(result) => {
+
                        mv(tmp, storage, &rid)?;
+
                        (result, true, None)
+
                    }
+
                }
            }
            Self::Pull {
                mut handle,
@@ -168,7 +177,7 @@ impl Handle {
///
/// # Errors
///   - Will fail if `storage` contains `rid` already.
-
fn mv(tmp: tempfile::TempDir, storage: &Storage, rid: &RepoId) -> Result<(), error::Fetch> {
+
fn mv(tmp: TemporaryPath, storage: &Storage, rid: &RepoId) -> Result<(), error::Fetch> {
    use std::io::{Error, ErrorKind};

    let from = tmp.path();
@@ -178,6 +187,10 @@ fn mv(tmp: tempfile::TempDir, storage: &Storage, rid: &RepoId) -> Result<(), err
        std::fs::rename(from, to)?;
    } else {
        log::warn!(target: "worker", "Refusing to move cloned repository {rid} already exists");
+

+
        #[cfg(windows)]
+
        cleanup(from);
+

        return Err(Error::new(
            ErrorKind::AlreadyExists,
            format!("repository already exists {to:?}"),
@@ -188,6 +201,14 @@ fn mv(tmp: tempfile::TempDir, storage: &Storage, rid: &RepoId) -> Result<(), err
    Ok(())
}

+
#[cfg(windows)]
+
fn cleanup(path: &std::path::Path) {
+
    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}");
+
    }
+
}
+

// Post notifications for the given refs.
fn notify(
    rid: &RepoId,
modified crates/radicle/src/storage/git.rs
@@ -9,7 +9,6 @@ use std::sync::LazyLock;
use std::{fs, io};

use crypto::Verified;
-
use tempfile::TempDir;

use crate::identity::crefs::GetCanonicalRefs as _;
use crate::identity::doc::DocError;
@@ -197,6 +196,12 @@ impl WriteStorage for Storage {
    }
}

+
#[cfg(unix)]
+
pub use tempfile::TempDir as TemporaryPath;
+

+
#[cfg(windows)]
+
pub use std::path::PathBuf as TemporaryPath;
+

impl Storage {
    /// Open a new storage instance and load its inventory.
    pub fn open<P: AsRef<Path>>(path: P, info: UserInfo) -> Result<Self, Error> {
@@ -212,23 +217,64 @@ 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
+
    /// returned by this function.
+
    ///
+
    /// On Unix-like system, upon success, returns a [`TempDir`].
+
    /// Make sure not to only drop it when the directory is not required
+
    /// anymore, as it removes the directory behind the file handle when dropped.
+
    /// Subsequent operations on the [`Repository`] will then fail.
+
    ///
+
    /// On Windows, we cannot keep a handle of the directory, because this
+
    /// prevents it from being moved. So, this returns a plain [`PathBuf`] and
+
    /// callers must take care to remove the directory when they do not need
+
    /// it anymore (probably also in error cases).
+
    pub fn lock_repository(
+
        &self,
+
        rid: RepoId,
+
    ) -> Result<(Repository, TemporaryPath), RepositoryError> {
+
        const RANDOMNESS_LENGTH: usize = 6;
+
        const SUFFIX: &str = ".lock";
+

+
        let prefix = rid.canonical() + ".";
+

        if self.contains(&rid)? {
            return Err(Error::Io(io::Error::new(
                io::ErrorKind::AlreadyExists,
-
                format!("refusing to create '{rid}.lock'"),
+
                format!("refusing to create '{prefix}*{SUFFIX}'"),
            ))
            .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))
+

+
        let (path, lock) = {
+
            #[cfg(unix)]
+
            {
+
                let tmp = tempfile::Builder::new()
+
                    .prefix(&prefix)
+
                    .suffix(SUFFIX)
+
                    .rand_bytes(RANDOMNESS_LENGTH)
+
                    .tempdir_in(self.path())
+
                    .map_err(Error::from)?;
+

+
                (tmp.path().to_path_buf(), tmp)
+
            }
+

+
            #[cfg(windows)]
+
            {
+
                let randomness = std::iter::repeat_with(fastrand::alphanumeric)
+
                    .take(RANDOMNESS_LENGTH)
+
                    .collect::<String>();
+

+
                let mut tmp = self.path().to_path_buf();
+
                tmp.push(format!("{prefix}{randomness}{SUFFIX}"));
+

+
                (tmp.as_path(), tmp)
+
            }
+
        };
+

+
        Ok((Repository::create(path, rid, &self.info)?, lock))
    }

    pub fn path(&self) -> &Path {