diff --git a/src/git_submodules/cli.rs b/src/git_submodules/cli.rs index f7ecd25f..63d78dc5 100644 --- a/src/git_submodules/cli.rs +++ b/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 { + super::find_root(requested).with_context(|| { + format!( + "`zed overtake --git-submodules` requires .gitmodules at or above {}", + requested.display() + ) + }) +} + fn run_cli(args: Vec) -> Result { crate::flags::normalize_global_boolean_environment(&args)?; let cli = match OvertakeCli::try_parse_from(args) { @@ -111,7 +125,19 @@ fn run_cli(args: Vec) -> Result { "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(()) + } } diff --git a/src/main.rs b/src/main.rs index 67f25141..a343a782 100644 --- a/src/main.rs +++ b/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 { diff --git a/src/project_lock.rs b/src/project_lock.rs index 564d05ec..719b8f09 100644 --- a/src/project_lock.rs +++ b/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> = RefCell::new(HashMap::new()); + static HELD_LOCKS: RefCell>> = 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() {} +/// assert_send::(); +/// ``` +/// +/// ```compile_fail +/// fn assert_sync() {} +/// assert_sync::(); +/// ``` +#[must_use = "dropping the operation guard releases one project-ownership handle"] +pub struct OperationGuard { + _ownership: Rc, } /// Canonical descriptor-lock identity for one checkout. @@ -67,28 +71,31 @@ pub fn operation_lock_path(project: &Path) -> Result { 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> { + 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( - project: &Path, - operation: &str, - action: impl FnOnce() -> Result, -) -> Result { +/// 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 { 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( 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( + project: &Path, + operation: &str, + action: impl FnOnce() -> Result, +) -> Result { + 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(()) + } }