Merge pull request #13027 from gitbutlerapp/per-repo-locking

fix: release global mutex before blocking on per-repo lock

Author: Byron

crates/but-core/src/sync.rs

@@ -81,10 +81,15 @@ pub fn try_exclusive_inter_process_access(
 /// Note that this **in-process** locking works only under the assumption that no two instances of
 /// GitButler are able to read or write the same repository.
 pub fn exclusive_repo_access(git_dir: impl Into<PathBuf>) -> RepoExclusiveGuard {
-    let mut map = WORKTREE_LOCKS.lock();
-    let git_dir = git_dir.into();
+    let lock = {
+        let mut map = WORKTREE_LOCKS.lock();
+        let git_dir = git_dir.into();
+        Arc::clone(map.entry(git_dir).or_default())
+    };
+    // The global `WORKTREE_LOCKS` mutex is released before blocking on the per-repo RwLock,
+    // so contention on one repo cannot block access to unrelated repos.
     RepoExclusiveGuard {
-        inner: map.entry(git_dir).or_default().write_arc().into(),
+        inner: lock.write_arc().into(),
         perm: RepoExclusive(()),
     }
 }
@@ -98,9 +103,14 @@ pub fn exclusive_repo_access(git_dir: impl Into<PathBuf>) -> RepoExclusiveGuard
 /// alive in the caller that owns the lock, and pass [`RepoShared`] further down instead via
 /// [`RepoSharedGuard::read_permission()`].
 pub fn shared_repo_access(git_dir: impl Into<PathBuf>) -> RepoSharedGuard {
-    let mut map = WORKTREE_LOCKS.lock();
-    let git_dir = git_dir.into();
-    RepoSharedGuard(Some(map.entry(git_dir).or_default().read_arc()))
+    let lock = {
+        let mut map = WORKTREE_LOCKS.lock();
+        let git_dir = git_dir.into();
+        Arc::clone(map.entry(git_dir).or_default())
+    };
+    // The global `WORKTREE_LOCKS` mutex is released before blocking on the per-repo RwLock,
+    // so contention on one repo cannot block access to unrelated repos.
+    RepoSharedGuard(Some(lock.read_arc()))
 }
 
 /// Owns an *exclusive* in-process repository lock and releases it on drop.

crates/but-core/tests/core/sync.rs

@@ -1,5 +1,6 @@
 use but_core::sync::{LockScope, try_exclusive_inter_process_access};
 use but_testsupport::gix_testtools;
+use std::time::Duration;
 
 #[test]
 fn exclusive_lock_prevents_second_acquisition() -> anyhow::Result<()> {
@@ -103,3 +104,54 @@ fn default_lock_scope_is_all_operations() {
     let default_scope = LockScope::default();
     assert!(matches!(default_scope, LockScope::AllOperations));
 }
+
+/// Regression test: contention on one repo must not block access to an unrelated repo.
+///
+/// Before the fix, `shared_repo_access` / `exclusive_repo_access` held the global
+/// `WORKTREE_LOCKS` mutex while blocking on the per-repo RwLock. This meant a thread
+/// waiting for repo "A"'s RwLock would prevent any other thread from acquiring a lock
+/// on repo "B", turning per-repo contention into a process-wide bottleneck.
+#[test]
+fn repo_lock_contention_does_not_block_unrelated_repos() {
+    // Thread 1: hold exclusive access to repo "A".
+    let exclusive_a = but_core::sync::exclusive_repo_access("/test/repo-a");
+
+    // Thread 2: try to acquire shared access to repo "A".
+    // This will block because thread 1 holds the exclusive lock.
+    let (tx_ready, rx_ready) = std::sync::mpsc::channel();
+    let blocked_thread = std::thread::spawn(move || {
+        // Signal that we're about to enter the lock acquisition path.
+        tx_ready.send(()).unwrap();
+        // This blocks until exclusive_a is dropped.
+        let _shared_a = but_core::sync::shared_repo_access("/test/repo-a");
+    });
+
+    // Wait until thread 2 has signalled it is about to call shared_repo_access.
+    rx_ready.recv().unwrap();
+    // Yield repeatedly so the OS scheduler runs thread 2 into the blocking
+    // lock path. Unlike a fixed sleep this adapts to scheduler pressure and
+    // avoids a hard-coded timing assumption.
+    for _ in 0..100 {
+        std::thread::yield_now();
+    }
+
+    // Thread 3 (this thread): access a completely unrelated repo "B".
+    // With the bug, this would deadlock because thread 2 holds the global mutex
+    // while waiting for repo A's RwLock.
+    let (tx, rx) = std::sync::mpsc::channel();
+    let probe = std::thread::spawn(move || {
+        let _shared_b = but_core::sync::shared_repo_access("/test/repo-b");
+        tx.send(()).ok();
+    });
+
+    // If repo "B" access completes within 2 seconds, the global mutex is not held
+    // across RwLock acquisition — the bug is fixed.
+    rx.recv_timeout(Duration::from_secs(2))
+        .expect("accessing an unrelated repo should not be blocked by contention on another repo");
+    probe.join().unwrap();
+
+    // Release the exclusive lock so thread 2 can complete, then join to
+    // surface any panics that occurred inside it.
+    drop(exclusive_a);
+    blocked_thread.join().unwrap();
+}