diff --git a/.github/workflows/apply-blocking-store-process-locks.yml b/.github/workflows/apply-blocking-store-process-locks.yml new file mode 100644 index 00000000..85a39020 --- /dev/null +++ b/.github/workflows/apply-blocking-store-process-locks.yml @@ -0,0 +1,70 @@ +name: blocking store process locks + +on: + pull_request: + paths: + - src/store.rs + - src/install_graph.rs + - src/install_graph/** + - Cargo.toml + - Cargo.lock + - .github/workflows/apply-blocking-store-process-locks.yml + push: + branches: [main] + paths: + - src/store.rs + - src/install_graph.rs + - src/install_graph/** + - Cargo.toml + - Cargo.lock + - .github/workflows/apply-blocking-store-process-locks.yml + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: blocking-store-process-locks-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +env: + CARGO_TERM_COLOR: always + RUST_BACKTRACE: "1" + +jobs: + linux-kernel-lock: + name: Linux process-lock and recursive-cache contracts + runs-on: ubuntu-24.04 + timeout-minutes: 35 + steps: + - name: Check out zed-cli + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 1 + persist-credentials: false + show-progress: false + + - name: Install stable Rust with formatting and lint support + run: | + rustup toolchain install stable --profile minimal --component rustfmt,clippy + rustup default stable + rustc --version --verbose + cargo --version --verbose + + - name: Check formatting + run: cargo fmt --all --check + + - name: Prove a separate process blocks until orderly release + run: cargo test --locked --lib store::tests::install_process_lock_blocks_without_polling_until_owner_release -- --exact --nocapture + + - name: Prove owner termination releases the descriptor lock + run: cargo test --locked --lib store::tests::install_process_lock_is_released_when_owner_process_is_terminated -- --exact --nocapture + + - name: Run all store process-lock tests + run: cargo test --locked --lib store::tests + + - name: Run recursive acquisition and cache-race tests + run: cargo test --locked --lib install_graph::tests + + - name: Deny Rust warnings + run: cargo clippy --locked --all-targets -- -D warnings diff --git a/.github/workflows/recursive-install-windows.yml b/.github/workflows/recursive-install-windows.yml index 8fdb860c..eda09e0f 100644 --- a/.github/workflows/recursive-install-windows.yml +++ b/.github/workflows/recursive-install-windows.yml @@ -79,3 +79,9 @@ jobs: - name: Prove failed downloads leave no cache publication run: cargo test --locked --manifest-path zed-cli/Cargo.toml --lib install_graph::tests::failed_downloads_remove_staging_files_and_never_publish_cache_entries -- --exact --nocapture + + - name: Prove the lower-level store lock blocks across processes + run: cargo test --locked --manifest-path zed-cli/Cargo.toml --lib store::tests::install_process_lock_blocks_without_polling_until_owner_release -- --exact --nocapture + + - name: Prove owner termination releases the lower-level store lock + run: cargo test --locked --manifest-path zed-cli/Cargo.toml --lib store::tests::install_process_lock_is_released_when_owner_process_is_terminated -- --exact --nocapture diff --git a/src/store.rs b/src/store.rs index 018446af..60c03286 100644 --- a/src/store.rs +++ b/src/store.rs @@ -2,7 +2,7 @@ use std::collections::{BTreeMap, BTreeSet}; use std::fs; use std::io::Read; use std::path::{Component, Path, PathBuf}; -use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; use anyhow::{Context, Result, bail}; use flate2::read::GzDecoder; @@ -55,22 +55,18 @@ fn require_build_key(key: &str) -> Result<()> { Ok(()) } -/// An advisory (flock-based) process lock held for the life of the guard. -/// CLI commands are highly concurrent — two `zed install` runs in different -/// terminals, or N parallel CI runners — so store mutations that must not -/// interleave (extracting the same artifact, rewriting refs.json) take a -/// lock first. The OS releases it if the process dies, so a crash can't -/// wedge the store. +/// A descriptor-backed process lock held for the life of the guard. +/// +/// Acquisition uses the operating system's blocking lock primitive directly: +/// `flock`/`fcntl` semantics on Unix and `LockFileEx` semantics on Windows via +/// `fs2`. Contended callers sleep in the kernel and wake when the owner drops +/// the descriptor or exits. There is no retry timer, jitter loop, stale-file +/// reclamation, or userspace polling. pub struct ProcessLock { _file: fs::File, } impl ProcessLock { - /// Three-phase acquisition tuned for CLI latency: - /// spin-wait at 50ms while young (catches sub-second releases with no - /// perceptible lag), then exponential backoff with full jitter so - /// parallel CI runners don't wake in lockstep, and after 3s tell the - /// user what they are waiting on instead of freezing silently. fn acquire(path: &Path, waiting_on: &str) -> Result { if let Some(parent) = path.parent() { fs::create_dir_all(parent)?; @@ -83,52 +79,13 @@ impl ProcessLock { .open(path) .with_context(|| format!("opening lock file {}", path.display()))?; - let timeout = Duration::from_secs( - std::env::var("ZED_PKG_LOCK_TIMEOUT") - .ok() - .and_then(|v| v.parse().ok()) - .unwrap_or(600), - ); - let start = Instant::now(); - let mut backoff_attempt: u32 = 0; - let mut notified = false; - loop { - match file.try_lock_exclusive() { - Ok(()) => return Ok(Self { _file: file }), - Err(_) if start.elapsed() >= timeout => { - bail!( - "timed out after {}s waiting for {waiting_on} (lock {}); \ - another zed process may be stuck — raise ZED_PKG_LOCK_TIMEOUT \ - or investigate", - timeout.as_secs(), - path.display() - ); - } - Err(_) => {} - } - let elapsed = start.elapsed(); - if elapsed >= Duration::from_secs(3) && !notified { - eprintln!("waiting for {waiting_on} (held by another zed process)..."); - notified = true; - } - let sleep = if elapsed < Duration::from_millis(500) { - Duration::from_millis(50) - } else { - backoff_attempt = backoff_attempt.saturating_add(1); - let cap_ms: u64 = 2000; - let exp_ms = 100u64 - .saturating_mul(1u64 << backoff_attempt.min(20)) - .min(cap_ms); - // Full jitter: uniform in [0, exp_ms] without pulling in an - // RNG dependency — subsec_nanos is plenty for de-syncing. - let nanos = SystemTime::now() - .duration_since(UNIX_EPOCH) - .map(|d| d.subsec_nanos() as u64) - .unwrap_or(0); - Duration::from_millis(nanos % (exp_ms + 1)) - }; - std::thread::sleep(sleep); - } + file.lock_exclusive().with_context(|| { + format!( + "waiting for {waiting_on} through operating-system lock {}", + path.display() + ) + })?; + Ok(Self { _file: file }) } } @@ -688,8 +645,222 @@ pub fn human_size(bytes: u64) -> String { #[cfg(test)] mod tests { + use std::process::{Command, Stdio}; + use std::time::Instant; + use super::*; + #[test] + fn install_process_lock_blocks_without_polling_until_owner_release() { + const CHILD_HOME: &str = "ZED_PKG_TEST_STORE_LOCK_CHILD_HOME"; + const CHILD_ATTEMPTING: &str = "ZED_PKG_TEST_STORE_LOCK_CHILD_ATTEMPTING"; + const CHILD_ACQUIRED: &str = "ZED_PKG_TEST_STORE_LOCK_CHILD_ACQUIRED"; + const TEST_NAME: &str = + "store::tests::install_process_lock_blocks_without_polling_until_owner_release"; + + if let Some(home) = std::env::var_os(CHILD_HOME) { + let attempting = PathBuf::from(std::env::var_os(CHILD_ATTEMPTING).unwrap()); + let acquired = PathBuf::from(std::env::var_os(CHILD_ACQUIRED).unwrap()); + fs::write(attempting, b"attempting").unwrap(); + let store = Store::new(Path::new(&home)); + let _waiter = store.install_lock().unwrap(); + fs::write(acquired, b"acquired").unwrap(); + return; + } + + let temp = tempfile::tempdir().unwrap(); + let store = Store::new(temp.path()); + let owner = store.install_lock().unwrap(); + let attempting = temp.path().join("store-lock-waiter-attempting"); + let acquired = temp.path().join("store-lock-waiter-acquired"); + let mut child = Command::new(std::env::current_exe().unwrap()) + .arg(TEST_NAME) + .arg("--exact") + .arg("--nocapture") + .env(CHILD_HOME, temp.path()) + .env(CHILD_ATTEMPTING, &attempting) + .env(CHILD_ACQUIRED, &acquired) + .stdin(Stdio::null()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()) + .spawn() + .unwrap(); + + let attempting_deadline = Instant::now() + Duration::from_secs(5); + while !attempting.is_file() { + if let Some(status) = child.try_wait().unwrap() { + panic!("store-lock waiter exited before attempting acquisition: {status}"); + } + if Instant::now() >= attempting_deadline { + let _ = child.kill(); + let _ = child.wait(); + panic!("store-lock waiter did not reach acquisition"); + } + std::thread::sleep(Duration::from_millis(20)); + } + + std::thread::sleep(Duration::from_millis(200)); + assert!( + !acquired.exists(), + "a separate process acquired install.lock while the owner held it" + ); + assert!( + child.try_wait().unwrap().is_none(), + "store-lock waiter exited before owner release" + ); + + drop(owner); + let release_deadline = Instant::now() + Duration::from_secs(5); + loop { + if let Some(status) = child.try_wait().unwrap() { + assert!(status.success(), "store-lock waiter failed: {status}"); + break; + } + if Instant::now() >= release_deadline { + let _ = child.kill(); + let _ = child.wait(); + panic!("store-lock waiter did not wake after owner release"); + } + std::thread::sleep(Duration::from_millis(20)); + } + assert!( + acquired.is_file(), + "waiter exited without acquiring install.lock" + ); + } + + #[test] + fn install_process_lock_is_released_when_owner_process_is_terminated() { + const CHILD_ROLE: &str = "ZED_PKG_TEST_STORE_LOCK_EXIT_ROLE"; + const CHILD_HOME: &str = "ZED_PKG_TEST_STORE_LOCK_EXIT_HOME"; + const OWNER_ACQUIRED: &str = "ZED_PKG_TEST_STORE_LOCK_OWNER_ACQUIRED"; + const WAITER_ATTEMPTING: &str = "ZED_PKG_TEST_STORE_LOCK_WAITER_ATTEMPTING"; + const WAITER_ACQUIRED: &str = "ZED_PKG_TEST_STORE_LOCK_WAITER_ACQUIRED"; + const TEST_NAME: &str = + "store::tests::install_process_lock_is_released_when_owner_process_is_terminated"; + + if let Some(role) = std::env::var_os(CHILD_ROLE) { + let home = PathBuf::from(std::env::var_os(CHILD_HOME).unwrap()); + let store = Store::new(&home); + match role.to_string_lossy().as_ref() { + "owner" => { + let _owner = store.install_lock().unwrap(); + fs::write(std::env::var_os(OWNER_ACQUIRED).unwrap(), b"acquired").unwrap(); + loop { + std::thread::sleep(Duration::from_secs(60)); + } + } + "waiter" => { + fs::write(std::env::var_os(WAITER_ATTEMPTING).unwrap(), b"attempting").unwrap(); + let _waiter = store.install_lock().unwrap(); + fs::write(std::env::var_os(WAITER_ACQUIRED).unwrap(), b"acquired").unwrap(); + return; + } + other => panic!("unexpected child role: {other}"), + } + } + + let temp = tempfile::tempdir().unwrap(); + let owner_acquired = temp.path().join("store-lock-owner-acquired"); + let waiter_attempting = temp.path().join("store-lock-waiter-attempting-after-exit"); + let waiter_acquired = temp.path().join("store-lock-waiter-acquired-after-exit"); + + let mut owner = Command::new(std::env::current_exe().unwrap()) + .arg(TEST_NAME) + .arg("--exact") + .arg("--nocapture") + .env(CHILD_ROLE, "owner") + .env(CHILD_HOME, temp.path()) + .env(OWNER_ACQUIRED, &owner_acquired) + .stdin(Stdio::null()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()) + .spawn() + .unwrap(); + + let owner_deadline = Instant::now() + Duration::from_secs(5); + while !owner_acquired.is_file() { + if let Some(status) = owner.try_wait().unwrap() { + panic!("store-lock owner exited before acquiring install.lock: {status}"); + } + if Instant::now() >= owner_deadline { + let _ = owner.kill(); + let _ = owner.wait(); + panic!("store-lock owner did not acquire install.lock"); + } + std::thread::sleep(Duration::from_millis(20)); + } + + let mut waiter = Command::new(std::env::current_exe().unwrap()) + .arg(TEST_NAME) + .arg("--exact") + .arg("--nocapture") + .env(CHILD_ROLE, "waiter") + .env(CHILD_HOME, temp.path()) + .env(WAITER_ATTEMPTING, &waiter_attempting) + .env(WAITER_ACQUIRED, &waiter_acquired) + .stdin(Stdio::null()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()) + .spawn() + .unwrap(); + + let attempting_deadline = Instant::now() + Duration::from_secs(5); + while !waiter_attempting.is_file() { + if let Some(status) = waiter.try_wait().unwrap() { + let _ = owner.kill(); + let _ = owner.wait(); + panic!("store-lock waiter exited before attempting acquisition: {status}"); + } + if Instant::now() >= attempting_deadline { + let _ = waiter.kill(); + let _ = waiter.wait(); + let _ = owner.kill(); + let _ = owner.wait(); + panic!("store-lock waiter did not reach acquisition"); + } + std::thread::sleep(Duration::from_millis(20)); + } + + std::thread::sleep(Duration::from_millis(200)); + assert!( + !waiter_acquired.exists(), + "waiter acquired install.lock while the owner process was alive" + ); + assert!( + waiter.try_wait().unwrap().is_none(), + "store-lock waiter exited before owner termination" + ); + + owner.kill().unwrap(); + let owner_status = owner.wait().unwrap(); + assert!( + !owner_status.success(), + "owner process unexpectedly exited successfully instead of being terminated" + ); + + let release_deadline = Instant::now() + Duration::from_secs(5); + loop { + if let Some(status) = waiter.try_wait().unwrap() { + assert!( + status.success(), + "store-lock waiter failed after owner exit: {status}" + ); + break; + } + if Instant::now() >= release_deadline { + let _ = waiter.kill(); + let _ = waiter.wait(); + panic!("store-lock waiter did not wake after owner process termination"); + } + std::thread::sleep(Duration::from_millis(20)); + } + assert!( + waiter_acquired.is_file(), + "waiter exited without acquiring install.lock after owner termination" + ); + } + #[test] fn gc_survives_hostile_max_age() { // A hostile age (`u64::MAX` seconds) would overflow naive epoch math