diff --git a/docs/how-it-works/architecture.mdx b/docs/how-it-works/architecture.mdx index 36e278c4..21c6df74 100644 --- a/docs/how-it-works/architecture.mdx +++ b/docs/how-it-works/architecture.mdx @@ -26,6 +26,8 @@ check remote (via daemon) ── hit → restore via reflink → done ↓ miss acquire per-key build lock ↓ +re-check local store ── peer committed → restore via reflink → done + ↓ still absent run rustc ↓ store output files in content-addressed blobs @@ -35,7 +37,7 @@ send upload job to daemon (async) release lock ``` -The lock prevents duplicate compilation when cargo spawns multiple parallel rustc processes for the same crate (which can happen in certain workspace configurations). A process that loses the lock waits for the winner's result and restores from the cache instead of compiling. +The lock prevents duplicate compilation when cargo spawns multiple parallel rustc processes for the same crate (which can happen in certain workspace configurations). A process that loses the lock waits for the winner's result and restores from the cache instead of compiling. A process that acquires the lock re-checks the store before compiling, covering a peer that committed between the initial lookup and lock acquisition. Build outcomes are separated into three cacheable cases: diff --git a/src/store.rs b/src/store.rs index dddc4e75..66ec02ed 100644 --- a/src/store.rs +++ b/src/store.rs @@ -616,6 +616,16 @@ pub struct KeyLock { path: PathBuf, } +/// Result of claiming responsibility for a cache miss. +pub enum BuildClaim { + /// This process owns the key and may compile it. + Acquired(KeyLock), + /// A peer committed the key after the caller's cache lookup. + Committed(Box), + /// Another process currently owns the key. + Contended, +} + /// A fully-written key lock waiting to be published at its canonical path. /// Keeping preparation separate from publication prevents contenders from /// observing an empty lock file and mistaking an in-progress owner for stale. @@ -1316,6 +1326,25 @@ impl Store { self.try_acquire_lock(self.entry_dir(cache_key).with_extension("lock")) } + /// Claim a cache miss, re-checking the store after acquiring the key lock. + /// + /// The re-check closes the window where a peer can commit and release its + /// lock between this process's cache lookup and lock acquisition. + pub fn claim_build(&self, cache_key: &str) -> Result { + let Some(lock) = self.try_lock(cache_key)? else { + return Ok(BuildClaim::Contended); + }; + match self.get(cache_key)? { + Some(meta) if meta.files.is_empty() => { + tracing::warn!("cache entry {cache_key} has no files, evicting before build"); + self.remove_entry(cache_key)?; + Ok(BuildClaim::Acquired(lock)) + } + Some(meta) => Ok(BuildClaim::Committed(Box::new(meta))), + None => Ok(BuildClaim::Acquired(lock)), + } + } + /// Acquire the cross-process GC lock so concurrent GC drivers — a manual /// `kache gc`, the daemon's periodic sweep, `maybe_evict_after_upload`, or a /// second daemon — don't double-scan and contend. Returns `None` if another @@ -4583,19 +4612,114 @@ mod tests { let config = test_config(dir.path()); let store = Store::open(&config).unwrap(); - let lock1 = store.try_lock("testkey").unwrap(); - assert!(lock1.is_some()); + let lock1 = match store.claim_build("testkey").unwrap() { + BuildClaim::Acquired(lock) => lock, + BuildClaim::Committed(_) | BuildClaim::Contended => { + panic!("first build claim should acquire the key") + } + }; - // Second lock attempt should fail - let lock2 = store.try_lock("testkey").unwrap(); - assert!(lock2.is_none()); + assert!(matches!( + store.claim_build("testkey").unwrap(), + BuildClaim::Contended + )); - // Drop first lock drop(lock1); - // Now should succeed - let lock3 = store.try_lock("testkey").unwrap(); - assert!(lock3.is_some()); + assert!(matches!( + store.claim_build("testkey").unwrap(), + BuildClaim::Acquired(_) + )); + } + + #[test] + fn claim_build_rechecks_entry_after_acquiring_lock() { + let dir = tempfile::tempdir().unwrap(); + let config = test_config(dir.path()); + let peer = Store::open(&config).unwrap(); + let waiter = Store::open(&config).unwrap(); + let cache_key = "committed_during_claim_race"; + + let peer_lock = match peer.claim_build(cache_key).unwrap() { + BuildClaim::Acquired(lock) => lock, + BuildClaim::Committed(_) | BuildClaim::Contended => { + panic!("peer should acquire the initial build claim") + } + }; + assert!(matches!( + waiter.claim_build(cache_key).unwrap(), + BuildClaim::Contended + )); + + let output = dir.path().join("lib.rlib"); + fs::write(&output, b"peer output").unwrap(); + peer.put( + cache_key, + "peer", + &["rlib".to_string()], + &[], + "host", + "dev", + &[(output, "lib.rlib".to_string())], + "", + "", + ) + .unwrap(); + drop(peer_lock); + + match waiter.claim_build(cache_key).unwrap() { + BuildClaim::Committed(meta) => assert_eq!(meta.cache_key, cache_key), + BuildClaim::Acquired(_) => panic!("committed entry must prevent a duplicate compile"), + BuildClaim::Contended => panic!("peer already released the build lock"), + } + assert!( + waiter.try_lock(cache_key).unwrap().is_some(), + "serving the committed entry must release the claim" + ); + } + + #[test] + fn claim_build_evicts_empty_committed_entry() { + let dir = tempfile::tempdir().unwrap(); + let config = test_config(dir.path()); + let store = Store::open(&config).unwrap(); + let cache_key = "empty_committed_entry"; + let entry_dir = store.entry_dir(cache_key); + fs::create_dir_all(&entry_dir).unwrap(); + let meta = EntryMeta { + cache_key: cache_key.to_string(), + crate_name: "empty".to_string(), + crate_types: vec!["rlib".to_string()], + files: vec![], + stdout: String::new(), + stderr: String::new(), + features: vec![], + target: "host".to_string(), + profile: "dev".to_string(), + compile_time_ms: 0, + emit_kinds: Vec::new(), + }; + fs::write( + entry_dir.join("meta.json"), + serde_json::to_string_pretty(&meta).unwrap(), + ) + .unwrap(); + store + .db + .execute( + "INSERT INTO entries (cache_key, crate_name, size, committed) VALUES (?1, ?2, 0, 1)", + params![cache_key, "empty"], + ) + .unwrap(); + + match store.claim_build(cache_key).unwrap() { + BuildClaim::Acquired(_) => {} + BuildClaim::Committed(_) => panic!("empty entry must not be served"), + BuildClaim::Contended => panic!("no peer owns the build lock"), + } + assert!(store.get(cache_key).unwrap().is_none()); + assert!(!entry_dir.exists()); + assert!(store.try_lock(cache_key).unwrap().is_some()); } #[test] diff --git a/src/wrapper.rs b/src/wrapper.rs index 58acaf29..c7098444 100644 --- a/src/wrapper.rs +++ b/src/wrapper.rs @@ -14,7 +14,7 @@ use crate::compiler::{ use crate::config::Config; use crate::events::{self, BuildEvent, EventResult}; use crate::link; -use crate::store::{Store, StorePutResult}; +use crate::store::{BuildClaim, Store, StorePutResult}; /// Check whether progress lines should be printed to stderr. /// @@ -1321,12 +1321,13 @@ pub fn run(config: &Config, wrapper_args: &[String]) -> Result { } } - // 3. Cache miss — try to acquire build lock - let lock = match store.try_lock(&cache_key) { - Ok(Some(lock)) => lock, + // 3. Cache miss — claim the key, then re-check under the build lock. + let (lock, committed) = match store.claim_build(&cache_key) { + Ok(BuildClaim::Acquired(lock)) => (Some(lock), None), + Ok(BuildClaim::Committed(meta)) => (None, Some(*meta)), Err(e) => { tracing::warn!( - "acquiring build lock for {} failed: {} — recompiling", + "claiming build for {} failed: {} — recompiling", crate_name, e ); @@ -1336,81 +1337,80 @@ pub fn run(config: &Config, wrapper_args: &[String]) -> Result { crate_name, &event_root, start, - format!("build lock unavailable: {e}"), + format!("build claim failed: {e}"), ); } - Ok(None) => { + Ok(BuildClaim::Contended) => { // Another process is building this key — wait for it tracing::debug!("waiting for {} to be built by another process", crate_name); - if store.wait_for_committed(&cache_key).unwrap_or(false) { - // It's now available - if let Ok(Some(meta)) = store.get(&cache_key) { - let restore_start = std::time::Instant::now(); - if let Err(e) = restore_from_cache( - config, - &compiler, - &BlobSource::Store(&store), - &args, - &meta, - ) { - tracing::warn!( - "restoring cache hit for {} failed: {} — recompiling", - crate_name, - e - ); - return passthrough_with_event( - config, - &args, - crate_name, - &event_root, - start, - format!("restore failed: {e}"), - ); - } - let restore_ms = restore_start.elapsed().as_millis() as u64; - let elapsed = start.elapsed().as_millis() as u64; - let size: u64 = meta.files.iter().map(|f| f.size).sum(); - log_event_with_hash_stats( - config, - &event_root, - crate_name, - EventResult::LocalHit, - elapsed, - meta.compile_time_ms, - size, - &cache_key, - key_ms, - key_hash_stats, - lookup_ms, - restore_ms, - 0, - ); - // Replay the original compiler diagnostics, exactly as - // the other hit sites do — otherwise the process that - // loses the build-lock race silently swallows the - // cached warnings/notes. - if !meta.stdout.is_empty() { - print!("{}", meta.stdout); - } - if !meta.stderr.is_empty() { - eprint!("{}", meta.stderr); - } - clean_incremental_dir(config, &args); - return Ok(0); - } - } - // If waiting failed, fall through to compile - tracing::warn!("wait for {} failed, compiling ourselves", crate_name); - // Compile without caching + let committed = store + .wait_for_committed(&cache_key) + .unwrap_or(false) + .then(|| store.get(&cache_key).ok().flatten()) + .flatten(); + (None, committed) + } + }; + + if let Some(meta) = committed { + let restore_start = std::time::Instant::now(); + if let Err(e) = + restore_from_cache(config, &compiler, &BlobSource::Store(&store), &args, &meta) + { + tracing::warn!( + "restoring cache hit for {} failed: {} — recompiling", + crate_name, + e + ); return passthrough_with_event( config, &args, crate_name, &event_root, start, - "build lock wait failed", + format!("restore failed: {e}"), ); } + let restore_ms = restore_start.elapsed().as_millis() as u64; + let elapsed = start.elapsed().as_millis() as u64; + let size: u64 = meta.files.iter().map(|f| f.size).sum(); + log_event_with_hash_stats( + config, + &event_root, + crate_name, + EventResult::LocalHit, + elapsed, + meta.compile_time_ms, + size, + &cache_key, + key_ms, + key_hash_stats, + lookup_ms, + restore_ms, + 0, + ); + // Replay the original compiler diagnostics, exactly as the other hit + // sites do, so a coalesced compile does not swallow warnings or notes. + if !meta.stdout.is_empty() { + print!("{}", meta.stdout); + } + if !meta.stderr.is_empty() { + eprint!("{}", meta.stderr); + } + clean_incremental_dir(config, &args); + return Ok(0); + } + + let Some(lock) = lock else { + tracing::warn!("wait for {} failed, compiling ourselves", crate_name); + return passthrough_with_event( + config, + &args, + crate_name, + &event_root, + start, + "build lock wait failed", + ); }; // 4. Compile