fix: Added an "acquisition lock" to prevent deadlocks (fine grain locking)

This commit adds the concept of an acquisition lock that is held
exclusively during the fingerprint checking and job preparation.
We release this lock prior to executing jobs in the queue to allow other
cargos to proceed. This lock prevents deadlocks by only allowing a
single Cargo to lock the build unit locks at a time effectively making
this operation a "transaction" guarded by the acquisition lock.

Note that we added a new `.acquisition-lock` instead of reusing `.cargo-lock`
This is needed as we drop the `.acquisition-lock` during the compilation
phase, however `cargo clean` relies on `.cargo-lock` to avoid cleaning
during a build. If we were to change the behavior of `.cargo-lock` to be
unlocked during the compilation phase, it would open us up to having
`cargo clean` potentially corrupt an in progress build.

Author: ranger-ross

src/cargo/core/compiler/build_runner/compilation_files.rs

@@ -444,6 +444,11 @@ impl<'a, 'gctx: 'a> CompilationFiles<'a, 'gctx> {
             .map(Arc::clone)
     }
 
+    /// Returns the path to the acquisition lock, used to avoid multiple Cargos from deadlocking
+    pub fn acquisition_lock(&self) -> &Path {
+        self.host.build_dir().acquisition_lock()
+    }
+
     /// Returns the path where the output for the given unit and `FileType`
     /// should be uplifted to.
     ///

src/cargo/core/compiler/build_runner/mod.rs

@@ -178,6 +178,15 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
         self.check_collisions()?;
         self.compute_metadata_for_doc_units();
 
+        // When -Zfine-grain-locking is enabled, we take an "acquisition lock" exclusively
+        // that we hold while taking other locks. This lock will block other Cargos from also
+        // taking locks possibly causing deadlocks. We drop the acquisition lock before we start
+        // executing jobs allowing other Cargos to compile in parallel if they do not share build
+        // units.
+        if self.bcx.gctx.cli_unstable().fine_grain_locking {
+            self.lock_manager.acquire_acquisition_lock(&self)?;
+        }
+
         // We need to make sure that if there were any previous docs already compiled,
         // they were compiled with the same Rustc version that we're currently using.
         // See the function doc comment for more.
@@ -200,6 +209,11 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
             fingerprint.clear_memoized();
         }
 
+        // Release acquisition lock allowing other Cargos to proceed
+        if self.bcx.gctx.cli_unstable().fine_grain_locking {
+            self.lock_manager.release_acquisition_lock()?;
+        }
+
         // Now that we've figured out everything that we're going to do, do it!
         queue.execute(&mut self)?;
 

src/cargo/core/compiler/layout.rs

@@ -286,6 +286,7 @@ impl Layout {
         let build_dest = build_dest.as_path_unlocked();
         let deps = build_dest.join("deps");
         let artifact = deps.join("artifact");
+        let acquisition_lock = build_dest.join(".acquisition-lock");
 
         let artifact_dir = if must_take_artifact_dir_lock {
             // For now we don't do any more finer-grained locking on the artifact
@@ -323,6 +324,7 @@ impl Layout {
                 fingerprint: build_dest.join(".fingerprint"),
                 examples: build_dest.join("examples"),
                 tmp: build_root.join("tmp"),
+                acquisition_lock,
                 _lock: build_dir_lock,
                 is_new_layout,
             },
@@ -405,6 +407,7 @@ pub struct BuildDirLayout {
     examples: PathBuf,
     /// The directory for temporary data of integration tests and benches
     tmp: PathBuf,
+    acquisition_lock: PathBuf,
     /// The lockfile for a build (`.cargo-lock`). Will be unlocked when this
     /// struct is `drop`ped.
     ///
@@ -505,4 +508,8 @@ impl BuildDirLayout {
         paths::create_dir_all(&self.tmp)?;
         Ok(&self.tmp)
     }
+    // Fetch the acquisition lock path
+    pub fn acquisition_lock(&self) -> &Path {
+        &self.acquisition_lock
+    }
 }

src/cargo/core/compiler/locking.rs

@@ -16,16 +16,52 @@ use tracing::instrument;
 
 /// A struct to store the lock handles for build units during compilation.
 pub struct LockManager {
+    acquisition: RwLock<Option<FileLock>>,
     locks: RwLock<HashMap<LockKey, FileLock>>,
 }
 
 impl LockManager {
     pub fn new() -> Self {
         Self {
+            acquisition: RwLock::new(None),
             locks: RwLock::new(HashMap::new()),
         }
     }
 
+    /// Acquires the acquisition lock required to call [`LockManager::lock`] and [`LockManager::lock_shared`]
+    ///
+    /// This should be called prior to attempting lock build units and should be released prior to
+    /// executing compilation jobs to allow other Cargos to proceed if they do not share any build
+    /// units.
+    #[instrument(skip_all)]
+    pub fn acquire_acquisition_lock(&self, build_runner: &BuildRunner<'_, '_>) -> CargoResult<()> {
+        let path = build_runner.files().acquisition_lock();
+        let fs = Filesystem::new(path.to_path_buf());
+
+        let lock = fs.open_rw_exclusive_create(&path, build_runner.bcx.gctx, "acquisition lock")?;
+
+        let Ok(mut acquisition_lock) = self.acquisition.write() else {
+            bail!("failed to take acquisition write lock");
+        };
+        *acquisition_lock = Some(lock);
+
+        Ok(())
+    }
+
+    /// Releases the acquisition lock, see [`LockManager::acquire_acquisition_lock`]
+    #[instrument(skip_all)]
+    pub fn release_acquisition_lock(&self) -> CargoResult<()> {
+        let Ok(mut acquisition_lock) = self.acquisition.write() else {
+            bail!("failed to take acquisition write lock");
+        };
+        assert!(
+            acquisition_lock.is_some(),
+            "attempted to release acquisition while it was not taken"
+        );
+        *acquisition_lock = None;
+        Ok(())
+    }
+
     /// Takes a shared lock on a given [`Unit`]
     /// This prevents other Cargo instances from compiling (writing) to
     /// this build unit.
@@ -38,6 +74,10 @@ impl LockManager {
         build_runner: &BuildRunner<'_, '_>,
         unit: &Unit,
     ) -> CargoResult<LockKey> {
+        assert!(
+            self.acquisition.read().unwrap().is_some(),
+            "attempted to take shared lock without acquisition lock"
+        );
         let key = LockKey::from_unit(build_runner, unit);
         tracing::Span::current().record("key", key.0.to_str());
 
@@ -60,6 +100,10 @@ impl LockManager {
 
     #[instrument(skip(self))]
     pub fn lock(&self, key: &LockKey) -> CargoResult<()> {
+        assert!(
+            self.acquisition.read().unwrap().is_some(),
+            "attempted to take exclusive lock without acquisition lock"
+        );
         let locks = self.locks.read().unwrap();
         if let Some(lock) = locks.get(&key) {
             lock.file().lock()?;

src/cargo/core/compiler/mod.rs

@@ -250,6 +250,12 @@ fn compile<'gctx>(
                     // the user from running `cargo build` while developing. Generally, only a few
                     // workspace units will be changing in each build and the units will not be
                     // shared between `build` and `check` allowing them to run in parallel.
+                    //
+                    // Also note that we unlock before taking the exclusive lock as not all
+                    // platforms support lock upgrading. This is safe as we hold the acquisition
+                    // lock so we should be the only one operating on the locks so we can assume
+                    // that the lock will not be stolen by another instance.
+                    build_runner.lock_manager.unlock(&lock)?;
                     build_runner.lock_manager.lock(&lock)?;
                     job.after(downgrade_lock_to_shared(lock));
                 }

tests/testsuite/build_dir.rs

@@ -40,6 +40,7 @@ fn binary_with_debug() {
 [ROOT]/foo/build-dir/.rustc_info.json
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo
@@ -95,6 +96,7 @@ fn binary_with_release() {
 [ROOT]/foo/build-dir/.rustc_info.json
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/release/.cargo-lock
+[ROOT]/foo/build-dir/release/.acquisition-lock
 [ROOT]/foo/build-dir/release/build/foo/[HASH]/fingerprint/bin-foo
 [ROOT]/foo/build-dir/release/build/foo/[HASH]/fingerprint/bin-foo.json
 [ROOT]/foo/build-dir/release/build/foo/[HASH]/fingerprint/dep-bin-foo
@@ -205,6 +207,7 @@ fn should_default_to_target() {
 [ROOT]/foo/target/.rustc_info.json
 [ROOT]/foo/target/CACHEDIR.TAG
 [ROOT]/foo/target/debug/.cargo-lock
+[ROOT]/foo/target/debug/.acquisition-lock
 [ROOT]/foo/target/debug/build/foo/[HASH]/fingerprint/bin-foo
 [ROOT]/foo/target/debug/build/foo/[HASH]/fingerprint/bin-foo.json
 [ROOT]/foo/target/debug/build/foo/[HASH]/fingerprint/dep-bin-foo
@@ -234,6 +237,7 @@ fn should_respect_env_var() {
 [ROOT]/foo/build-dir/.rustc_info.json
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo
@@ -279,6 +283,7 @@ fn build_script_should_output_to_build_dir() {
     p.root().join("build-dir").assert_build_dir_layout(str![[r#"
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo.txt
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/build_script_build[..].d
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/build_script_build[..][EXE]
@@ -342,6 +347,7 @@ fn cargo_tmpdir_should_output_to_build_dir() {
     p.root().join("build-dir").assert_build_dir_layout(str![[r#"
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo-[HASH].d
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo.d
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..].d
@@ -402,6 +408,7 @@ fn examples_should_output_to_build_dir_and_uplift_to_target_dir() {
 [ROOT]/foo/build-dir/.rustc_info.json
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-example-foo
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/example-foo
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/example-foo.json
@@ -448,6 +455,7 @@ fn benches_should_output_to_build_dir() {
     p.root().join("build-dir").assert_build_dir_layout(str![[r#"
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo-[HASH].d
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..].d
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo-[HASH][EXE]
@@ -527,6 +535,7 @@ fn cargo_package_should_build_in_build_dir_and_output_to_target_dir() {
     p.root().join("build-dir").assert_build_dir_layout(str![[r#"
 [ROOT]/foo/build-dir/.rustc_info.json
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo
@@ -608,6 +617,7 @@ fn cargo_clean_should_clean_the_target_dir_and_build_dir() {
 [ROOT]/foo/build-dir/.rustc_info.json
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo
@@ -678,6 +688,7 @@ fn cargo_clean_should_remove_correct_files() {
 [ROOT]/foo/build-dir/.rustc_info.json
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/bar/[HASH]/out/bar-[HASH].d
 [ROOT]/foo/build-dir/debug/build/bar/[HASH]/out/libbar-[HASH].rlib
 [ROOT]/foo/build-dir/debug/build/bar/[HASH]/out/libbar-[HASH].rmeta
@@ -705,6 +716,7 @@ fn cargo_clean_should_remove_correct_files() {
 [ROOT]/foo/build-dir/.rustc_info.json
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..][EXE]
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/out/foo[..].d
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo
@@ -841,6 +853,7 @@ fn template_workspace_root() {
 [ROOT]/foo/build-dir/.rustc_info.json
 [ROOT]/foo/build-dir/CACHEDIR.TAG
 [ROOT]/foo/build-dir/debug/.cargo-lock
+[ROOT]/foo/build-dir/debug/.acquisition-lock
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json
 [ROOT]/foo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo
@@ -889,6 +902,7 @@ fn template_cargo_cache_home() {
 [ROOT]/home/.cargo/build-dir/.rustc_info.json
 [ROOT]/home/.cargo/build-dir/CACHEDIR.TAG
 [ROOT]/home/.cargo/build-dir/debug/.cargo-lock
+[ROOT]/home/.cargo/build-dir/debug/.acquisition-lock
 [ROOT]/home/.cargo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo
 [ROOT]/home/.cargo/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json
 [ROOT]/home/.cargo/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo
@@ -951,6 +965,7 @@ fn template_workspace_path_hash() {
 [ROOT]/foo/foo/[HASH]/build-dir/.rustc_info.json
 [ROOT]/foo/foo/[HASH]/build-dir/CACHEDIR.TAG
 [ROOT]/foo/foo/[HASH]/build-dir/debug/.cargo-lock
+[ROOT]/foo/foo/[HASH]/build-dir/debug/.acquisition-lock
 [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo
 [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/bin-foo.json
 [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/dep-bin-foo
@@ -1019,6 +1034,7 @@ fn template_workspace_path_hash_should_handle_symlink() {
 [ROOT]/foo/foo/[HASH]/build-dir/.rustc_info.json
 [ROOT]/foo/foo/[HASH]/build-dir/CACHEDIR.TAG
 [ROOT]/foo/foo/[HASH]/build-dir/debug/.cargo-lock
+[ROOT]/foo/foo/[HASH]/build-dir/debug/.acquisition-lock
 [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/dep-lib-foo
 [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/invoked.timestamp
 [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/lib-foo
@@ -1058,6 +1074,7 @@ fn template_workspace_path_hash_should_handle_symlink() {
 [ROOT]/foo/foo/[HASH]/build-dir/.rustc_info.json
 [ROOT]/foo/foo/[HASH]/build-dir/CACHEDIR.TAG
 [ROOT]/foo/foo/[HASH]/build-dir/debug/.cargo-lock
+[ROOT]/foo/foo/[HASH]/build-dir/debug/.acquisition-lock
 [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/dep-lib-foo
 [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/invoked.timestamp
 [ROOT]/foo/foo/[HASH]/build-dir/debug/build/foo/[HASH]/fingerprint/lib-foo