From 39a24a567851a63e93df1d5023487da58f668143 Mon Sep 17 00:00:00 2001 From: Ross Sullivan Date: Wed, 25 Feb 2026 23:18:30 +0900 Subject: [PATCH 1/2] fix: Simplify fine grain locking to lock before job --- .../core/compiler/job_queue/job_state.rs | 4 --- src/cargo/core/compiler/mod.rs | 26 +++++++------------ 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/cargo/core/compiler/job_queue/job_state.rs b/src/cargo/core/compiler/job_queue/job_state.rs index 06a672b611d..ae5cc32875d 100644 --- a/src/cargo/core/compiler/job_queue/job_state.rs +++ b/src/cargo/core/compiler/job_queue/job_state.rs @@ -147,10 +147,6 @@ impl<'a, 'gctx> JobState<'a, 'gctx> { .push(Message::Finish(self.id, Artifact::Metadata, Ok(()))); } - pub fn lock_exclusive(&self, lock: &LockKey) -> CargoResult<()> { - self.lock_manager.lock(lock) - } - pub fn downgrade_to_shared(&self, lock: &LockKey) -> CargoResult<()> { self.lock_manager.downgrade_to_shared(lock) } diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 014a2781c79..f280c7ceae8 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -237,19 +237,20 @@ fn compile<'gctx>( work.then(link_targets(build_runner, unit, true)?) }); - // If -Zfine-grain-locking is enabled, we wrap the job with an upgrade to exclusive + // If -Zfine-grain-locking is enabled, we take an exclusive // lock before starting, then downgrade to a shared lock after the job is finished. if build_runner.bcx.gctx.cli_unstable().fine_grain_locking && job.freshness().is_dirty() { if let Some(lock) = lock { - // Here we unlock the current shared lock to avoid deadlocking with other cargo - // processes. Then we configure our compile job to take an exclusive lock - // before starting. Once we are done compiling (including both rmeta and rlib) - // we downgrade to a shared lock to allow other cargo's to read the build unit. - // We will hold this shared lock for the remainder of compilation to prevent - // other cargo from re-compiling while we are still using the unit. - build_runner.lock_manager.unlock(&lock)?; - job.before(prebuild_lock_exclusive(lock.clone())); + // We take an exclusive lock before scheduling the job to keep our locking model + // simple. While this does mean we could potentially be waiting for another job + // when this could begin immediately, it reduces the risk of deadlock. + // + // We are also optimizing for avoiding rust-analyzer's `cargo check` preventing + // 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. + build_runner.lock_manager.lock(&lock)?; job.after(downgrade_lock_to_shared(lock)); } } @@ -615,13 +616,6 @@ fn verbose_if_simple_exit_code(err: Error) -> Error { } } -fn prebuild_lock_exclusive(lock: LockKey) -> Work { - Work::new(move |state| { - state.lock_exclusive(&lock)?; - Ok(()) - }) -} - fn downgrade_lock_to_shared(lock: LockKey) -> Work { Work::new(move |state| { state.downgrade_to_shared(&lock)?; From e6af9a21df7a61cd4bcaa49e29b164fe7e9af0c6 Mon Sep 17 00:00:00 2001 From: Ross Sullivan Date: Sat, 28 Feb 2026 16:08:09 +0900 Subject: [PATCH 2/2] 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. --- .../build_runner/compilation_files.rs | 5 +++ src/cargo/core/compiler/build_runner/mod.rs | 14 ++++++ src/cargo/core/compiler/layout.rs | 7 +++ src/cargo/core/compiler/locking.rs | 44 +++++++++++++++++++ src/cargo/core/compiler/mod.rs | 6 +++ tests/testsuite/build_dir.rs | 17 +++++++ 6 files changed, 93 insertions(+) diff --git a/src/cargo/core/compiler/build_runner/compilation_files.rs b/src/cargo/core/compiler/build_runner/compilation_files.rs index 46329f947cd..e2df81d316b 100644 --- a/src/cargo/core/compiler/build_runner/compilation_files.rs +++ b/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. /// diff --git a/src/cargo/core/compiler/build_runner/mod.rs b/src/cargo/core/compiler/build_runner/mod.rs index 957fecf55eb..a65cb72fecb 100644 --- a/src/cargo/core/compiler/build_runner/mod.rs +++ b/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)?; diff --git a/src/cargo/core/compiler/layout.rs b/src/cargo/core/compiler/layout.rs index bfc5608fe13..1aa4ca3b764 100644 --- a/src/cargo/core/compiler/layout.rs +++ b/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 + } } diff --git a/src/cargo/core/compiler/locking.rs b/src/cargo/core/compiler/locking.rs index ea9f98632a1..57657b0865b 100644 --- a/src/cargo/core/compiler/locking.rs +++ b/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>, locks: RwLock>, } 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 { + 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()?; diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index f280c7ceae8..cfad7099bd4 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/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)); } diff --git a/tests/testsuite/build_dir.rs b/tests/testsuite/build_dir.rs index 92f17e7b5dd..d654d3befc1 100644 --- a/tests/testsuite/build_dir.rs +++ b/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