Fix #588 race cond updating .crates.toml (#645)

Fixed #588

* Add new dep fs-lock v0.1.0 to crates/bin
* Refactor: Impl new type `Manifests` in crates/bin
   for managing manifests.
* Fix #588 race cond updating `.crates.toml`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Cargo.lock

@@ -353,6 +353,7 @@ dependencies = [
  "crates_io_api",
  "dirs",
  "embed-resource",
+ "fs-lock",
  "log",
  "miette",
  "mimalloc",

crates/bin/Cargo.toml

@@ -27,6 +27,7 @@ binstalk-manifests = { path = "../binstalk-manifests", version = "0.1.1" }
 clap = { version = "4.0.32", features = ["derive"] }
 crates_io_api = { version = "0.8.1", default-features = false }
 dirs = "4.0.0"
+fs-lock = { version = "0.1.0", path = "../fs-lock" }
 log = { version = "0.4.17", features = ["std"] }
 miette = "5.5.0"
 mimalloc = { version = "0.1.32", default-features = false, optional = true }

crates/bin/src/entry.rs

@@ -1,4 +1,4 @@
-use std::{collections::BTreeMap, fs, io, path::PathBuf, sync::Arc, time::Duration};
+use std::{fs, path::PathBuf, sync::Arc, time::Duration};
 
 use binstalk::{
     errors::BinstallError,
@@ -11,12 +11,7 @@ use binstalk::{
         Resolver,
     },
 };
-use binstalk_manifests::{
+use binstalk_manifests::cargo_toml_binstall::PkgOverride;
-    binstall_crates_v1::Records as BinstallCratesV1Records,
-    cargo_crates_v1::{CratesToml, CratesTomlParseError},
-    cargo_toml_binstall::PkgOverride,
-    CompactString, Version,
-};
 use crates_io_api::AsyncClient as CratesIoApiClient;
 use log::LevelFilter;
 use miette::{miette, Result, WrapErr};
@@ -26,6 +21,7 @@ use tracing::{debug, error, info, warn};
 use crate::{
     args::{Args, Strategy},
     install_path,
+    manifests::Manifests,
     ui::confirm,
 };
 
@@ -47,12 +43,12 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
         .collect();
 
     // Compute paths
-    let (install_path, cargo_roots, metadata, temp_dir) =
+    let (install_path, mut manifests, temp_dir) =
         compute_paths_and_load_manifests(args.roots, args.install_path)?;
 
     // Remove installed crates
     let mut crate_names =
-        filter_out_installed_crates(args.crate_names, args.force, metadata.as_ref()).peekable();
+        filter_out_installed_crates(args.crate_names, args.force, manifests.as_mut())?.peekable();
 
     if crate_names.peek().is_none() {
         debug!("Nothing to do");
@@ -160,21 +156,8 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
                     .map(|fetch| fetch.install(&binstall_opts))
                     .collect::<Result<Vec<_>, BinstallError>>()?;
 
-                if let Some((mut cargo_binstall_metadata, _)) = metadata {
+                if let Some(manifests) = manifests {
-                    // The cargo manifest path is already created when loading
+                    manifests.update(metadata_vec)?;
-                    // metadata.
-
-                    debug!("Writing .crates.toml");
-                    CratesToml::append_to_path(
-                        cargo_roots.join(".crates.toml"),
-                        metadata_vec.iter(),
-                    )?;
-
-                    debug!("Writing binstall/crates-v1.json");
-                    for metadata in metadata_vec {
-                        cargo_binstall_metadata.replace(metadata);
-                    }
-                    cargo_binstall_metadata.overwrite()?;
                 }
 
                 if no_cleanup {
@@ -205,13 +188,11 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) -
     Ok(())
 }
 
-type Metadata = (BinstallCratesV1Records, BTreeMap<CompactString, Version>);
+/// Return (install_path, manifests, temp_dir)
-
-/// Return (install_path, cargo_roots, metadata, temp_dir)
 fn compute_paths_and_load_manifests(
     roots: Option<PathBuf>,
     install_path: Option<PathBuf>,
-) -> Result<(PathBuf, PathBuf, Option<Metadata>, tempfile::TempDir)> {
+) -> Result<(PathBuf, Option<Manifests>, tempfile::TempDir)> {
     block_in_place(|| {
         // Compute cargo_roots
         let cargo_roots = install_path::get_cargo_roots_path(roots).ok_or_else(|| {
@@ -229,42 +210,9 @@ fn compute_paths_and_load_manifests(
         fs::create_dir_all(&install_path).map_err(BinstallError::Io)?;
         debug!("Using install path: {}", install_path.display());
 
-        // Load metadata
+        // Load manifests
-        let metadata = if !custom_install_path {
+        let manifests = if !custom_install_path {
-            // Read cargo_binstall_metadata
+            Some(Manifests::open_exclusive(&cargo_roots)?)
-            let metadata_dir = cargo_roots.join("binstall");
-            fs::create_dir_all(&metadata_dir).map_err(BinstallError::Io)?;
-            let manifest_path = metadata_dir.join("crates-v1.json");
-
-            debug!(
-                "Reading {} from {}",
-                "cargo_binstall_metadata",
-                manifest_path.display()
-            );
-
-            let cargo_binstall_metadata = BinstallCratesV1Records::load_from_path(&manifest_path)?;
-
-            // Read cargo_install_v1_metadata
-            let manifest_path = cargo_roots.join(".crates.toml");
-
-            debug!(
-                "Reading {} from {}",
-                "cargo_install_v1_metadata",
-                manifest_path.display()
-            );
-
-            let cargo_install_v1_metadata = match CratesToml::load_from_path(&manifest_path) {
-                Ok(metadata) => metadata.collect_into_crates_versions()?,
-                Err(CratesTomlParseError::Io(io_err))
-                    if io_err.kind() == io::ErrorKind::NotFound =>
-                {
-                    // .crates.toml does not exist, create an empty BTreeMap
-                    Default::default()
-                }
-                Err(err) => Err(err)?,
-            };
-
-            Some((cargo_binstall_metadata, cargo_install_v1_metadata))
         } else {
             None
         };
@@ -280,7 +228,7 @@ fn compute_paths_and_load_manifests(
             .map_err(BinstallError::from)
             .wrap_err("Creating a temporary directory failed.")?;
 
-        Ok((install_path, cargo_roots, metadata, temp_dir))
+        Ok((install_path, manifests, temp_dir))
     })
 }
 
@@ -288,18 +236,23 @@ fn compute_paths_and_load_manifests(
 fn filter_out_installed_crates(
     crate_names: Vec<CrateName>,
     force: bool,
-    metadata: Option<&Metadata>,
+    manifests: Option<&mut Manifests>,
-) -> impl Iterator<Item = (CrateName, Option<semver::Version>)> + '_ {
+) -> Result<impl Iterator<Item = (CrateName, Option<semver::Version>)> + '_> {
-    CrateName::dedup(crate_names)
+    let mut installed_crates = manifests
+        .map(Manifests::load_installed_crates)
+        .transpose()?;
+
+    Ok(CrateName::dedup(crate_names)
     .filter_map(move |crate_name| {
         let name = &crate_name.name;
 
-        let curr_version = metadata
+        let curr_version = installed_crates
-            // `cargo-uninstall` can be called to uninstall crates,
+            .as_mut()
-            // but it only updates .crates.toml.
+            // Since crate_name is deduped, every entry of installed_crates
+            // can be visited at most once.
             //
-            // So here we will honour .crates.toml only.
+            // So here we take ownership of the version stored to avoid cloning.
-            .and_then(|metadata| metadata.1.get(name));
+            .and_then(|crates| crates.remove(name));
 
         match (
             force,
@@ -307,7 +260,7 @@ fn filter_out_installed_crates(
             &crate_name.version_req,
         ) {
             (false, Some(curr_version), Some(version_req))
-                if version_req.is_latest_compatible(curr_version) =>
+                if version_req.is_latest_compatible(&curr_version) =>
             {
                 debug!("Bailing out early because we can assume wanted is already installed from metafile");
                 info!("{name} v{curr_version} is already installed, use --force to override");
@@ -316,10 +269,10 @@ fn filter_out_installed_crates(
 
             // The version req is "*" thus a remote upgraded version could exist
             (false, Some(curr_version), None) => {
-                Some((crate_name, Some(curr_version.clone())))
+                Some((crate_name, Some(curr_version)))
             }
 
             _ => Some((crate_name, None)),
         }
-    })
+    }))
 }

crates/bin/src/lib.rs

@@ -3,4 +3,5 @@ pub mod bin_util;
 pub mod entry;
 pub mod install_path;
 pub mod logging;
+pub mod manifests;
 pub mod ui;

crates/bin/src/manifests.rs

@@ -0,0 +1,85 @@
+use std::{collections::BTreeMap, fs, io::Seek, path::Path};
+
+use binstalk::errors::BinstallError;
+use binstalk_manifests::{
+    binstall_crates_v1::Records as BinstallCratesV1Records, cargo_crates_v1::CratesToml,
+    crate_info::CrateInfo, CompactString, Version,
+};
+use fs_lock::FileLock;
+use miette::{Error, Result};
+use tracing::debug;
+
+pub struct Manifests {
+    binstall: BinstallCratesV1Records,
+    cargo_crates_v1: FileLock,
+}
+
+impl Manifests {
+    pub fn open_exclusive(cargo_roots: &Path) -> Result<Self> {
+        // Read cargo_binstall_metadata
+        let metadata_path = cargo_roots.join("binstall/crates-v1.json");
+        fs::create_dir_all(metadata_path.parent().unwrap()).map_err(BinstallError::Io)?;
+
+        debug!(
+            "Reading binstall metadata from {} and obtaining exclusive lock",
+            metadata_path.display()
+        );
+
+        let binstall = BinstallCratesV1Records::load_from_path(&metadata_path)?;
+
+        // Read cargo_install_v1_metadata
+        let manifest_path = cargo_roots.join(".crates.toml");
+
+        debug!(
+            "Obtaining exclusive lock of cargo install v1 metadata in path {}",
+            manifest_path.display()
+        );
+
+        let cargo_crates_v1 = fs::File::options()
+            .read(true)
+            .write(true)
+            .create(true)
+            .open(manifest_path)
+            .and_then(FileLock::new_exclusive)
+            .map_err(BinstallError::Io)?;
+
+        Ok(Self {
+            binstall,
+            cargo_crates_v1,
+        })
+    }
+
+    fn rewind_cargo_crates_v1(&mut self) -> Result<()> {
+        self.cargo_crates_v1
+            .rewind()
+            .map_err(BinstallError::Io)
+            .map_err(Error::from)
+    }
+
+    /// `cargo-uninstall` can be called to uninstall crates,
+    /// but it only updates .crates.toml.
+    ///
+    /// So here we will honour .crates.toml only.
+    pub fn load_installed_crates(&mut self) -> Result<BTreeMap<CompactString, Version>> {
+        self.rewind_cargo_crates_v1()?;
+
+        CratesToml::load_from_reader(&mut self.cargo_crates_v1)
+            .and_then(CratesToml::collect_into_crates_versions)
+            .map_err(Error::from)
+    }
+
+    pub fn update(mut self, metadata_vec: Vec<CrateInfo>) -> Result<()> {
+        self.rewind_cargo_crates_v1()?;
+
+        debug!("Writing .crates.toml");
+        CratesToml::append_to_file(&mut self.cargo_crates_v1, &metadata_vec)?;
+
+        debug!("Writing binstall/crates-v1.json");
+        for metadata in metadata_vec {
+            self.binstall.replace(metadata);
+        }
+        self.binstall.overwrite()?;
+
+        Ok(())
+    }
+}