feat(DEN-2038): complete checkout-local mutation ownership

src/git_submodules/cli.rs

@@ -1,5 +1,6 @@
 use std::env;
 use std::ffi::{OsStr, OsString};
+use std::path::{Path, PathBuf};
 
 use anyhow::{Context, Result, bail};
 use clap::{Args, Parser, Subcommand};
@@ -90,6 +91,19 @@ fn print_root_help() -> Result<()> {
     Ok(())
 }
 
+/// Resolve the authority root before acquiring checkout-local mutation
+/// ownership. `overtake` supports invocation below the superproject, so locking
+/// the raw current directory would create a second `.zed/operation.lock` and
+/// allow a root-level lifecycle command to mutate the same checkout concurrently.
+fn takeover_project_root(requested: &Path) -> Result<PathBuf> {
+    super::find_root(requested).with_context(|| {
+        format!(
+            "`zed overtake --git-submodules` requires .gitmodules at or above {}",
+            requested.display()
+        )
+    })
+}
+
 fn run_cli(args: Vec<OsString>) -> Result<i32> {
     crate::flags::normalize_global_boolean_environment(&args)?;
     let cli = match OvertakeCli::try_parse_from(args) {
@@ -111,7 +125,19 @@ fn run_cli(args: Vec<OsString>) -> Result<i32> {
                     "no takeover source selected; pass `--git-submodules` or set ZED_PKG_GIT_SUBMODULES=1"
                 );
             }
-            let report = super::overtake(&cwd, &cfg)?;
+            let project = takeover_project_root(&cwd)?;
+            let report = crate::project_lock::with_lock(
+                &project,
+                "adopt Git submodules into Zed package graph",
+                || {
+                    // Modular takeover dispatch bypasses the ordinary `run`
+                    // entrypoint. Recover under the already-owned project lock
+                    // before reading `.gitmodules` or executing Git transport,
+                    // not only later when the first transaction begins.
+                    crate::transaction::recover_pending(&project)?;
+                    super::overtake(&project, &cfg)
+                },
+            )?;
             println!(
                 "overtook {} Git submodule package(s) in {}",
                 report.adopted,
@@ -198,9 +224,12 @@ fn global_option_takes_value(token: &str) -> bool {
 
 #[cfg(test)]
 mod tests {
+    use std::fs;
+
+    use anyhow::Result;
     use clap::Parser;
 
-    use super::OvertakeCli;
+    use super::{OvertakeCli, takeover_project_root};
 
     #[test]
     fn takeover_accepts_the_global_switch_before_and_after_the_command() {
@@ -216,4 +245,19 @@ mod tests {
             OvertakeCli::try_parse_from(["zed", "overtake", "--git-submodules=false"]).unwrap();
         assert!(!cli.globals.git_submodules);
     }
+
+    #[test]
+    fn nested_takeover_uses_the_superproject_operation_lock() -> Result<()> {
+        let root = tempfile::tempdir()?;
+        fs::write(root.path().join(".gitmodules"), b"")?;
+        let nested = root.path().join("packages/client/src");
+        fs::create_dir_all(&nested)?;
+
+        let selected = takeover_project_root(&nested)?;
+        assert_eq!(
+            crate::project_lock::operation_lock_path(&selected)?,
+            crate::project_lock::operation_lock_path(root.path())?
+        );
+        Ok(())
+    }
 }

src/main.rs

@@ -173,12 +173,15 @@ fn run(cli: Cli) -> anyhow::Result<()> {
     let git_submodules = cli.globals.git_submodules;
     let cwd = std::env::current_dir()?;
     if cwd.join(zed_cli::transaction::STAGING_DIR).is_dir() {
-        // Every live project transaction already owns this kernel-backed
-        // install lock. Recover under the same lock so a concurrent process
-        // cannot mistake an in-flight rollback journal for an abandoned one.
-        let store = Store::new(&cfg.home);
-        let _recovery_lock = store.install_lock()?;
-        zed_cli::transaction::recover_pending(&cwd)?;
+        // Recovery mutates the checkout and must use the same canonical
+        // descriptor-lock boundary as every new lifecycle mutation. Unlike the
+        // old Store install lock, this also coordinates processes using
+        // different ZED_PKG_HOME values without serializing unrelated projects.
+        zed_cli::project_lock::with_lock(
+            &cwd,
+            "recover interrupted Zed project transaction",
+            || zed_cli::transaction::recover_pending(&cwd).map(|_| ()),
+        )?;
     }
     match cli.cmd {
         Cmd::Init { org, name } => ops::init(&cwd, org, name, cfg.interactive),
@@ -195,21 +198,50 @@ fn run(cli: Cli) -> anyhow::Result<()> {
             allow_ecosystem_mismatch,
         } => {
             if git_submodules {
-                submodules::sync(&cwd)?;
+                // Git synchronization mutates the submodule worktrees and must
+                // share one descriptor lifetime with manifest/lock resolution,
+                // materialization, adapter wiring, and Git-lock finalization.
+                // Resolve the superproject first so nested invocations and root
+                // invocations converge on one checkout-local ownership path.
+                let project = submodules::find_root(&cwd).unwrap_or_else(|| cwd.clone());
+                let operation = if frozen {
+                    "synchronize Git submodules and restore frozen Zed dependency graph"
+                } else {
+                    "synchronize Git submodules and install Zed dependency graph"
+                };
+                let _guard = zed_cli::project_lock::acquire(&project, operation)?;
+                // Close the journal-created-between-startup-and-lock window and
+                // recover a superproject journal when invoked below the root.
+                zed_cli::transaction::recover_pending(&project)?;
+                submodules::sync(&project)?;
+                managed_install::install(
+                    &project,
+                    &cfg,
+                    &specs,
+                    frozen,
+                    install_mode,
+                    adapter,
+                    allow_build,
+                    target.as_deref(),
+                    allow_no_manifest,
+                    allow_ecosystem_mismatch,
+                )
+                .map(|_| ())
+            } else {
+                managed_install::install(
+                    &cwd,
+                    &cfg,
+                    &specs,
+                    frozen,
+                    install_mode,
+                    adapter,
+                    allow_build,
+                    target.as_deref(),
+                    allow_no_manifest,
+                    allow_ecosystem_mismatch,
+                )
+                .map(|_| ())
             }
-            managed_install::install(
-                &cwd,
-                &cfg,
-                &specs,
-                frozen,
-                install_mode,
-                adapter,
-                allow_build,
-                target.as_deref(),
-                allow_no_manifest,
-                allow_ecosystem_mismatch,
-            )
-            .map(|_| ())
         }
         Cmd::Uninstall { specs } => ops::uninstall(&cwd, &cfg, &specs),
         Cmd::Env { cmd } => match cmd {

src/project_lock.rs

@@ -5,55 +5,59 @@
 //! generated adapter wiring, Git-submodule projections, and transaction
 //! recovery. The lock lives inside the canonical project directory so two Zed
 //! processes using different `ZED_PKG_HOME` values still coordinate.
+//! Git-submodule synchronization, installer finalization, and transaction
+//! recovery all use this same authority root.
 
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::fs;
 use std::path::{Path, PathBuf};
+use std::rc::{Rc, Weak};
 
 use anyhow::{Context, Result};
-use zed_lock::{LockClass, LockManager, LockRequest};
+use zed_lock::{LockClass, LockGuard, LockManager, LockRequest};
 
 pub const OPERATION_LOCK_RELATIVE_PATH: &str = ".zed/operation.lock";
 
+// One thread's reentrant ownership cache.
+//
+// The map stores weak references so it never extends descriptor ownership.
+// Stale entries are removed lazily on the next acquisition of that canonical
+// checkout. Every live `OperationGuard` holds a strong reference to the same
+// `OwnedLock`, so nested handles may be dropped in any order without releasing
+// the kernel lock before the final handle.
 thread_local! {
-    /// Same-thread facade composition is intentionally reentrant. For example,
-    /// the CLI may hold the checkout lock around Git-submodule synchronization
-    /// and then call the public installer facade, which must reuse that exact
-    /// ownership rather than deadlock on a second descriptor request.
-    static HELD_LOCKS: RefCell<HashMap<PathBuf, usize>> = RefCell::new(HashMap::new());
+    static HELD_LOCKS: RefCell<HashMap<PathBuf, Weak<OwnedLock>>> = RefCell::new(HashMap::new());
 }
 
-struct HeldMarker {
-    path: PathBuf,
+struct OwnedLock {
+    _guard: LockGuard,
 }
 
-impl HeldMarker {
-    fn enter(path: PathBuf) -> Self {
-        HELD_LOCKS.with(|held| {
-            *held.borrow_mut().entry(path.clone()).or_insert(0) += 1;
-        });
-        Self { path }
-    }
-}
-
-impl Drop for HeldMarker {
-    fn drop(&mut self) {
-        HELD_LOCKS.with(|held| {
-            let mut held = held.borrow_mut();
-            let remove = match held.get_mut(&self.path) {
-                Some(count) if *count > 1 => {
-                    *count -= 1;
-                    false
-                }
-                Some(_) => true,
-                None => false,
-            };
-            if remove {
-                held.remove(&self.path);
-            }
-        });
-    }
+/// Owned checkout-local mutation authority.
+///
+/// A nested same-thread acquisition clones the strong reference to the outer
+/// descriptor ownership. The operating-system lock therefore remains held until
+/// the final nested or outer handle is dropped, regardless of drop order.
+/// Independent threads and processes always contend through `zed-lock`.
+///
+/// Reentrancy bookkeeping is thread-local, so this guard is intentionally
+/// neither [`Send`] nor [`Sync`]. It must be dropped on the thread that acquired
+/// it; moving it would otherwise detach the ownership cache from the originating
+/// thread. The `Rc` field enforces both type-level constraints.
+///
+/// ```compile_fail
+/// fn assert_send<T: Send>() {}
+/// assert_send::<zed_cli::project_lock::OperationGuard>();
+/// ```
+///
+/// ```compile_fail
+/// fn assert_sync<T: Sync>() {}
+/// assert_sync::<zed_cli::project_lock::OperationGuard>();
+/// ```
+#[must_use = "dropping the operation guard releases one project-ownership handle"]
+pub struct OperationGuard {
+    _ownership: Rc<OwnedLock>,
 }
 
 /// Canonical descriptor-lock identity for one checkout.
@@ -67,28 +71,31 @@ pub fn operation_lock_path(project: &Path) -> Result<PathBuf> {
     Ok(canonical.join(OPERATION_LOCK_RELATIVE_PATH))
 }
 
-fn is_held_on_this_thread(path: &Path) -> bool {
-    HELD_LOCKS.with(|held| held.borrow().contains_key(path))
+fn ownership_on_this_thread(path: &Path) -> Option<Rc<OwnedLock>> {
+    HELD_LOCKS.with(|held| {
+        let mut held = held.borrow_mut();
+        let ownership = held.get(path).and_then(Weak::upgrade);
+        if ownership.is_none() {
+            held.remove(path);
+        }
+        ownership
+    })
 }
 
-/// Run a complete project mutation while owning the checkout-local lock.
+/// Acquire checkout-local mutation ownership and return an RAII guard.
 ///
-/// Nested calls on the same thread and canonical checkout reuse the outer
-/// ownership. Independent threads and processes still go through `zed-lock`,
-/// whose descriptor lock is the sole local ownership authority and is released
-/// automatically if the process exits.
-pub fn with_lock<T>(
-    project: &Path,
-    operation: &str,
-    action: impl FnOnce() -> Result<T>,
-) -> Result<T> {
+/// This form is useful when callers must span multiple library calls under one
+/// project mutation boundary. Nested calls on the same thread share the outer
+/// descriptor ownership rather than issuing a second kernel lock request.
+pub fn acquire(project: &Path, operation: &str) -> Result<OperationGuard> {
     let path = operation_lock_path(project)?;
-    if is_held_on_this_thread(&path) {
-        let _marker = HeldMarker::enter(path);
-        return action();
+    if let Some(ownership) = ownership_on_this_thread(&path) {
+        return Ok(OperationGuard {
+            _ownership: ownership,
+        });
     }
 
-    let _guard = LockManager::global()
+    let guard = LockManager::global()
         .acquire_blocking(
             LockRequest::exclusive(&path)
                 .operation(operation)
@@ -100,15 +107,39 @@ pub fn with_lock<T>(
                 path.display()
             )
         })?;
-    let _marker = HeldMarker::enter(path);
+    let ownership = Rc::new(OwnedLock { _guard: guard });
+    HELD_LOCKS.with(|held| {
+        let previous = held.borrow_mut().insert(path, Rc::downgrade(&ownership));
+        debug_assert!(previous.and_then(|entry| entry.upgrade()).is_none());
+    });
+    Ok(OperationGuard {
+        _ownership: ownership,
+    })
+}
+
+/// Run a complete project mutation while owning the checkout-local lock.
+///
+/// Nested calls on the same thread and canonical checkout reuse the outer
+/// ownership. Independent threads and processes still go through `zed-lock`,
+/// whose descriptor lock is the sole local ownership authority and is released
+/// automatically if the process exits.
+pub fn with_lock<T>(
+    project: &Path,
+    operation: &str,
+    action: impl FnOnce() -> Result<T>,
+) -> Result<T> {
+    let _guard = acquire(project, operation)?;
     action()
 }
 
 #[cfg(test)]
 mod tests {
+    use std::rc::Rc;
+
     use anyhow::Result;
+    use zed_lock::{LockClass, LockManager, LockRequest};
 
-    use super::{OPERATION_LOCK_RELATIVE_PATH, operation_lock_path, with_lock};
+    use super::{OPERATION_LOCK_RELATIVE_PATH, acquire, operation_lock_path, with_lock};
 
     #[test]
     fn canonical_aliases_share_one_lock_identity() -> Result<()> {
@@ -135,4 +166,45 @@ mod tests {
         assert!(operation_lock_path(project.path())?.is_file());
         Ok(())
     }
+
+    #[test]
+    fn explicit_guard_allows_nested_facade_reuse() -> Result<()> {
+        let project = tempfile::tempdir()?;
+        let _guard = acquire(project.path(), "multi-step mutation")?;
+        with_lock(project.path(), "nested facade", || Ok(()))?;
+        assert!(operation_lock_path(project.path())?.is_file());
+        Ok(())
+    }
+
+    #[test]
+    fn nested_guards_keep_descriptor_ownership_until_the_last_drop() -> Result<()> {
+        let project = tempfile::tempdir()?;
+        let path = operation_lock_path(project.path())?;
+        let outer = acquire(project.path(), "outer mutation")?;
+        let inner = acquire(project.path(), "inner mutation")?;
+        assert!(Rc::ptr_eq(&outer._ownership, &inner._ownership));
+        assert_eq!(Rc::strong_count(&inner._ownership), 2);
+
+        // Public guards are independent values and callers can drop them out of
+        // lexical order. The inner handle must continue owning the descriptor.
+        drop(outer);
+        assert_eq!(Rc::strong_count(&inner._ownership), 1);
+        let contender = LockManager::global().try_acquire(
+            LockRequest::exclusive(&path)
+                .operation("same-process contention probe")
+                .class(LockClass::ProjectMutation)
+                .queue_same_process(),
+        )?;
+        assert!(contender.is_none());
+
+        drop(inner);
+        let acquired = LockManager::global().try_acquire(
+            LockRequest::exclusive(&path)
+                .operation("post-release probe")
+                .class(LockClass::ProjectMutation)
+                .queue_same_process(),
+        )?;
+        assert!(acquired.is_some());
+        Ok(())
+    }
 }