From 7414bc8bf8b8a5114b63e5f8965362119263efaf Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 21:45:31 -0400 Subject: [PATCH] Stop ordinary repository lock acquisition on interruption (#473) --- src/git.rs | 25 ++++++-- src/git/execution_tests.rs | 127 +++++++++++++++++++++++++++++++++++++ src/process.rs | 2 +- 3 files changed, 148 insertions(+), 6 deletions(-) diff --git a/src/git.rs b/src/git.rs index 97c446a18..8b64b2947 100644 --- a/src/git.rs +++ b/src/git.rs @@ -1,6 +1,6 @@ //! Running `git` in a directory. -use std::fs::File; +use std::fs::{File, TryLockError}; use std::path::{Path, PathBuf}; use std::process::{Command, Output}; use std::time::{Duration, Instant}; @@ -88,13 +88,28 @@ impl Git { /// Wait for, then hold until the file returned is dropped, a lock on the /// file `name` in the repository's common git directory, which every Run - /// from one Launch directory shares. + /// from one Launch directory shares. Ordinary acquisition stops for a + /// recorded interruption; completion acquisition keeps waiting. Contention + /// is polled every 100 ms without a deadline, and the file is never removed. pub fn lock(&self, name: &str) -> Result { let path = self.common_dir()?.join(name); + self.interruption.check()?; let file = File::create(&path).with_context(|| format!("can't open {}", path.display()))?; - file.lock() - .with_context(|| format!("can't lock {}", path.display()))?; - Ok(file) + loop { + self.interruption.check()?; + match file.try_lock() { + Ok(()) => { + self.interruption.check()?; + return Ok(file); + } + Err(TryLockError::WouldBlock) => { + std::thread::sleep(Duration::from_millis(100)); + } + Err(TryLockError::Error(error)) => { + return Err(error).with_context(|| format!("can't lock {}", path.display())); + } + } + } } /// Whether origin has the branch `branch`, as origin itself answers. diff --git a/src/git/execution_tests.rs b/src/git/execution_tests.rs index af491966b..24fdd538f 100644 --- a/src/git/execution_tests.rs +++ b/src/git/execution_tests.rs @@ -6,6 +6,128 @@ use crate::interrupt; use crate::test_support::with_recorded_signal; use std::fs; use std::os::unix::fs::PermissionsExt; +use std::sync::mpsc; + +/// Observe whether acquisition finishes while an independent open still +/// holds the lock. Release and join before the caller can assert, even when +/// acquisition ignores interruption or returns unexpectedly early. +fn contended_repository_lock(git: &Git, signal: Option) -> (bool, Result) { + let path = git.common_dir().unwrap().join("thirdshift-test.lock"); + let held = File::create(path).unwrap(); + held.lock().unwrap(); + std::thread::scope(|scope| { + let (finished, received) = mpsc::channel(); + let contender = scope.spawn(move || { + let result = git.lock("thirdshift-test.lock"); + let _ = finished.send(()); + result + }); + std::thread::sleep(Duration::from_millis(200)); + let delivered = signal.map(signal_hook::low_level::raise); + let finished_while_held = received.recv_timeout(Duration::from_secs(1)).is_ok(); + drop(held); + let result = contender.join().unwrap(); + if let Some(delivered) = delivered { + delivered.unwrap(); + } + (finished_while_held, result) + }) +} + +#[test] +fn ordinary_repository_lock_contention_stops_before_the_holder_releases() { + with_recorded_signal( + "git::execution_tests::ordinary_repository_lock_contention_stops_before_the_holder_releases", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + let (finished_while_held, result) = contended_repository_lock(&git, Some(signal)); + + assert!(finished_while_held, "acquisition waited for holder release"); + assert_eq!(result.unwrap_err().to_string(), "interrupted"); + assert!(interrupt::requested()); + let independent = File::open(git.dir().join(".git/thirdshift-test.lock")).unwrap(); + independent.try_lock().unwrap(); + }, + ); +} + +#[test] +fn completion_repository_lock_waits_through_arriving_and_recorded_interruption() { + with_recorded_signal( + "git::execution_tests::completion_repository_lock_waits_through_arriving_and_recorded_interruption", + |signal| { + let (_temp, git) = super::tests::repo_with_origin(); + let completion = git.completion(); + let path = git.common_dir().unwrap().join("thirdshift-test.lock"); + // First deliver a new request during contention, then acquire + // again with that request already recorded. + for signal in [Some(signal), None] { + let (finished_while_held, result) = contended_repository_lock(&completion, signal); + + assert!(!finished_while_held, "completion did not wait for release"); + let acquired = result.unwrap(); + let independent = File::open(&path).unwrap(); + assert!(matches!( + independent.try_lock(), + Err(TryLockError::WouldBlock) + )); + assert!(interrupt::requested()); + assert_eq!( + git.lock("ordinary.lock").unwrap_err().to_string(), + "interrupted" + ); + assert_eq!( + Git::new(git.dir()) + .lock("ordinary.lock") + .unwrap_err() + .to_string(), + "interrupted" + ); + assert_eq!( + git.run(&["rev-parse", "HEAD"]).unwrap_err().to_string(), + "interrupted" + ); + assert!(!path.with_file_name("ordinary.lock").exists()); + drop(acquired); + independent.try_lock().unwrap(); + assert!(path.exists(), "release removed the shared locking identity"); + } + }, + ); +} + +#[test] +fn repository_lock_is_shared_by_worktrees_and_owned_until_drop() { + let (temp, git) = super::tests::repo_with_origin(); + let worktree_path = temp.path().join("worktree"); + git.run(&[ + "worktree", + "add", + "--detach", + worktree_path.to_str().unwrap(), + "HEAD", + ]) + .unwrap(); + let worktree = Git::new(worktree_path); + let path = git.common_dir().unwrap().join("thirdshift-test.lock"); + let acquired = git.lock("thirdshift-test.lock").unwrap(); + let independent = + File::open(worktree.common_dir().unwrap().join("thirdshift-test.lock")).unwrap(); + + assert!(matches!( + independent.try_lock(), + Err(TryLockError::WouldBlock) + )); + drop(acquired); + let acquired = worktree.lock("thirdshift-test.lock").unwrap(); + assert!(matches!( + independent.try_lock(), + Err(TryLockError::WouldBlock) + )); + drop(acquired); + independent.try_lock().unwrap(); + assert!(path.exists(), "release removed the shared locking identity"); +} #[test] fn ordinary_git_refuses_to_spawn_after_interruption() { @@ -19,6 +141,11 @@ fn ordinary_git_refuses_to_spawn_after_interruption() { .unwrap_err(); assert_eq!(error.to_string(), "interrupted"); assert!(!git.dir().join("started").exists()); + assert_eq!( + git.lock("ordinary.lock").unwrap_err().to_string(), + "interrupted" + ); + assert!(!git.dir().join(".git/ordinary.lock").exists()); }, ); } diff --git a/src/process.rs b/src/process.rs index 1d1ecd023..76762bc22 100644 --- a/src/process.rs +++ b/src/process.rs @@ -33,7 +33,7 @@ impl Interruption { matches!(self, Self::Ordinary) && interrupt::requested() } - fn check(self) -> Result<()> { + pub(crate) fn check(self) -> Result<()> { if self.requested() { bail!("interrupted"); }