diff --git a/crates/khive-pack-git/docs/api/cache.md b/crates/khive-pack-git/docs/api/cache.md index d0d29e5a2..4b2f041e2 100644 --- a/crates/khive-pack-git/docs/api/cache.md +++ b/crates/khive-pack-git/docs/api/cache.md @@ -34,6 +34,16 @@ Config is env-var driven today (`KHIVE_GIT_DIGEST_CACHE_MAX_REPOS`, `KHIVE_GIT_DIGEST_CACHE_MAX_BYTES`, `KHIVE_GIT_DIGEST_CLONE_MAX_BYTES`, `KHIVE_GIT_DIGEST_SCRATCH_ROOT`) rather than a `[git]` TOML section. +`ensure_clone`/`refetch_clone`/`reclone` each check a slot's state and then +mutate it based on that check. A per-`cache_key` advisory `slot_lock` (issue +#805) is held for the full span of each of those functions, so two calls +racing the same slot can never interleave their check-and-mutate sequences. +Calls against distinct keys run their clone/fetch work concurrently; only +the short cache-wide `evict_lru` pass is serialized (by a separate +process-wide `EVICTION_LOCK`), and that pass defers — rather than blocks on +— any candidate whose per-slot lock is currently held. See `slot_lock` and +`evict_lru` below. + ## `CacheError::UnsafeToReplace` A repair operation (refetch/reclone) would have to touch a path that does @@ -79,11 +89,16 @@ Re-checks `is_owned_entry` immediately before fetching (issue #765 follow-up PR #788): the gap between `ensure_clone`'s own ownership check and this repair running — project resolution and GitHub ingestion happen in between — is wide enough for the slot to go markerless or be replaced, -so this function cannot rely on the caller having checked recently. There -is no same-key serialization for cache mutation in this crate today (a -concurrent `ensure_clone`/`reclone` racing this same slot is not otherwise -excluded) — this re-check narrows the adoption bug but does not close a -true concurrent-writer race. +so this function cannot rely on the caller having checked recently. +Concurrent same-key mutation is separately excluded by `slot_lock` (issue +#805, see below): a per-`cache_key` advisory lock held across the whole +check-and-mutate span of `ensure_clone`/`refetch_clone`/`reclone`, so two +calls racing the same slot cannot interleave. That lock does not close the +staleness window this re-check addresses — it is held only for a single +call's own span, not across the caller's earlier `ensure_clone` (released +before project resolution and lengthy GitHub ingestion run) — so the slot +can still go markerless in that intra-request interval, and the re-check is +what narrows it. The over-cap cleanup path routes through the same ownership-guarded `remove_owned_entry` `reclone` uses, rather than a raw `remove_dir_all` — a @@ -99,6 +114,24 @@ Refuses via `CacheError::UnsafeToReplace` when the existing path does not prove itself an owned cache slot — the same ownership guard `evict_lru` uses. +## `slot_lock` + +Returns the per-`cache_key` advisory `Mutex` from a process-global registry +(issue #805), creating it on first use. `ensure_clone`, `refetch_clone`, and +`reclone` each hold this lock for their whole check-and-mutate span, so two +calls racing the same slot cannot interleave a check against another call's +mutation. The lock is advisory and same-process only: it serializes this +crate's own cache mutations, not an external process touching the scratch +root. It is deliberately *not* held across a caller's separate `ensure_clone` +and later `refetch_clone` calls within one request — that intra-request +staleness window is what `refetch_clone`'s pre-fetch ownership re-check +narrows instead. + +`evict_lru` takes each candidate slot's lock with `try_lock` rather than +blocking, so a cache-wide eviction pass never waits on an in-flight same-key +mutation. How a deferred candidate is nonetheless brought back under the +caps is covered under `evict_lru` below. + ## `install_fresh_clone` Shared staging-clone-then-move path for both a first-time `ensure_clone` @@ -237,6 +270,36 @@ legitimately delete it between the `read_dir` listing and the `dir_size` call, so that vanish is tolerated by skipping the entry rather than aborting the whole pass. +A candidate whose `slot_lock` is currently held (an in-flight mutation on +that key) is deferred: `evict_lru` takes each candidate lock with `try_lock` +and skips a `WouldBlock` rather than waiting, so an eviction pass never +blocks on a concurrent clone/fetch (and, holding `EVICTION_LOCK` while a +mutation may hold a slot lock and be about to wait on `EVICTION_LOCK`, must +not). A deferred candidate is therefore *not* counted in this pass — so this +pass alone can return with the caps still exceeded. The guarantee that the +caps are nonetheless restored is `enforce_caps` (below): every mutation runs +a cap pass on its own exit, so the last of a set of concurrent mutations to +release its lock sees the others unlocked and enforces the caps over the +settled set. + +`evict_lru` and `enforce_caps` share one core (`evict_to_caps`) that differs +only in whether a `keep` slot is protected. + +## `enforce_caps` + +The keep-less cap pass (`evict_to_caps` with no protected slot). Run after a +cache mutation releases its slot lock on a FAILURE path (issue #960). On +success a mutation already ran `evict_lru` under its lock, protecting the +slot it returns; a *failed* `ensure_clone`/`refetch_clone`/`reclone` returns +before that pass, and a concurrent eviction may have deferred this slot while +its lock was held — leaving the caps exceeded with nothing scheduled to +correct them. `finish_mutation` runs `enforce_caps` once the lock is free (no +slot is protected because a failed mutation has no slot to return), so the +deferred candidate is finally accounted for. It acquires only `EVICTION_LOCK` +and `try_lock`s candidates — never held while a slot lock is held — so it +cannot deadlock with a success-path `evict_lru`. Best-effort: a failure here +is logged, and the mutation's own error is what propagates. + ## `ENV_MUTEX` `scratch_root()` reads process-global env vars; serialize any in-crate diff --git a/crates/khive-pack-git/src/cache.rs b/crates/khive-pack-git/src/cache.rs index 15bd0369e..0793faaa2 100644 --- a/crates/khive-pack-git/src/cache.rs +++ b/crates/khive-pack-git/src/cache.rs @@ -6,13 +6,16 @@ //! `KHIVE_GIT_DIGEST_CLONE_MAX_BYTES`, `KHIVE_GIT_DIGEST_SCRATCH_ROOT`) //! evicts least-recently-used clones once the cache exceeds its repo-count //! or total-byte limit; a per-clone size cap rejects an oversized -//! clone/fetch before it enters the addressable cache slot. See -//! crates/khive-pack-git/docs/api/cache.md for the full design rationale -//! (ownership-proof eviction, staging-then-move installation, per-clone cap -//! enforcement). +//! clone/fetch before it enters the addressable cache slot. A per-`cache_key` +//! advisory `slot_lock` (issue #805) serializes each slot's check-and-mutate +//! span. See crates/khive-pack-git/docs/api/cache.md for the full design +//! rationale (ownership-proof eviction, staging-then-move installation, +//! per-clone cap enforcement, slot serialization). +use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::process::Command; +use std::sync::{Arc, Mutex}; use std::time::SystemTime; use uuid::Uuid; @@ -102,6 +105,79 @@ fn clone_max_bytes() -> u64 { env_u64("KHIVE_GIT_DIGEST_CLONE_MAX_BYTES", DEFAULT_CLONE_MAX_BYTES) } +/// Per-cache-slot advisory locks, keyed by `cache_key` (issue #805): each of +/// `ensure_clone`, `refetch_clone`, and `reclone` is a check-then-mutate +/// sequence (does `is_owned_entry`/existence hold, act on the result), and +/// nothing previously ordered two such sequences racing the *same* slot -- +/// `refetch_clone`'s own doc comment used to admit this. Holding this slot's +/// lock for the full span of one of those functions serializes same-key +/// mutation while leaving distinct keys free to run concurrently: each +/// `cache_key` gets its own `Mutex` entry here, so locking one slot never +/// blocks a caller operating on a different slot. `SlotLock::drop` removes +/// an entry once the final live handle releases it, keeping the registry +/// bounded by active slot operations rather than process-lifetime history. +static SLOT_LOCKS: std::sync::LazyLock>>>> = + std::sync::LazyLock::new(|| Mutex::new(HashMap::new())); + +struct SlotLock { + key: String, + mutex: Arc>, +} + +impl std::ops::Deref for SlotLock { + type Target = Mutex<()>; + + fn deref(&self) -> &Self::Target { + &self.mutex + } +} + +impl Drop for SlotLock { + fn drop(&mut self) { + let mut locks = SLOT_LOCKS + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + // The registry and this handle are the final two owners only when no + // waiter or guard can still reference this mutex. + let is_final_handle = Arc::strong_count(&self.mutex) == 2; + let is_registered = locks + .get(&self.key) + .is_some_and(|mutex| Arc::ptr_eq(mutex, &self.mutex)); + if is_final_handle && is_registered { + locks.remove(&self.key); + let live_entries = locks.len(); + if locks.capacity() > live_entries.saturating_mul(4) { + locks.shrink_to(live_entries); + } + } + } +} + +/// Eviction passes are serialized so the last overlapping slot mutation to +/// reach eviction observes every earlier successful operation that has +/// released its slot lock and can restore the configured caps. Callers +/// already hold their own slot lock; eviction only probes candidate locks +/// with `try_lock`, so this ordering cannot deadlock with another mutation +/// waiting here. +static EVICTION_LOCK: Mutex<()> = Mutex::new(()); + +/// Get-or-create the advisory lock for cache slot `key`. Callers hold the +/// returned lock for the entire check-and-mutate span of their operation on +/// that slot (see `SLOT_LOCKS`). The handle's drop check runs while holding +/// the registry mutex, so a concurrent lookup either increments the same +/// `Arc` first or observes the entry only after its final handle is gone. +fn slot_lock(key: &str) -> SlotLock { + let mut locks = SLOT_LOCKS + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let key = key.to_string(); + let mutex = locks + .entry(key.clone()) + .or_insert_with(|| Arc::new(Mutex::new(()))) + .clone(); + SlotLock { key, mutex } +} + /// Ensure a local clone of `canonical_url` exists and is up to date; returns /// the repo's local path. /// @@ -117,8 +193,37 @@ fn clone_max_bytes() -> u64 { /// the staging-then-move and ownership-guard rationale. pub fn ensure_clone(canonical_url: &str) -> Result { let root = scratch_root(); - std::fs::create_dir_all(&root)?; + let outcome = ensure_clone_locked(&root, canonical_url); + finish_mutation(&root, &outcome); + outcome +} + +/// Bring the cache caps back within limits after a mutation whose slot lock +/// has just been released. A successful `ensure_clone`/`refetch_clone`/ +/// `reclone` already ran `evict_lru` under its lock (protecting the slot it +/// returns), so nothing is needed on success. A FAILED mutation skipped that +/// pass, and a concurrent eviction may have deferred this slot while its lock +/// was held — leaving the caps exceeded with nothing scheduled to correct them +/// (#960). Enforce them now that the lock is free. Best-effort: the mutation's +/// own error is the one propagated, so a secondary eviction failure is logged, +/// not surfaced. +fn finish_mutation(root: &Path, outcome: &Result) { + if outcome.is_ok() { + return; + } + if let Err(evict_err) = enforce_caps(root) { + tracing::warn!( + error = %evict_err, + "cap enforcement after a failed cache mutation did not complete" + ); + } +} + +fn ensure_clone_locked(root: &Path, canonical_url: &str) -> Result { + std::fs::create_dir_all(root)?; let key = cache_key(canonical_url); + let lock = slot_lock(&key); + let _guard = lock.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); let repo_dir = root.join(&key); let cap = clone_max_bytes(); @@ -128,21 +233,21 @@ pub fn ensure_clone(canonical_url: &str) -> Result { } fetch(&repo_dir)?; // `repo_dir` was just fetched into and its ownership already - // confirmed above; it vanishing here is a real problem (e.g. a - // racing, non-serialized `ensure_clone`/`reclone` on the same key -- - // see `refetch_clone`'s doc comment), not a maybe-absent slot, so - // propagate rather than swallow. + // confirmed above; it vanishing here is a real problem (`slot_lock` + // excludes a concurrent `ensure_clone`/`refetch_clone`/`reclone` on + // this same key, so nothing else in this crate should be touching + // it), not a maybe-absent slot, so propagate rather than swallow. let size = dir_size(&repo_dir)?; if size > cap { - remove_owned_entry(&root, &repo_dir)?; + remove_owned_entry(root, &repo_dir)?; return Err(CacheError::CloneTooLarge { bytes: size, cap }); } touch(&repo_dir)?; } else { - install_fresh_clone(canonical_url, &root, &repo_dir, cap)?; + install_fresh_clone(canonical_url, root, &repo_dir, cap)?; } - evict_lru(&root, &repo_dir)?; + evict_lru(root, &repo_dir)?; Ok(repo_dir) } @@ -151,7 +256,15 @@ pub fn ensure_clone(canonical_url: &str) -> Result { /// crates/khive-pack-git/docs/api/cache.md#refetch_clone. pub(crate) fn refetch_clone(canonical_url: &str) -> Result { let root = scratch_root(); + let outcome = refetch_clone_locked(&root, canonical_url); + finish_mutation(&root, &outcome); + outcome +} + +fn refetch_clone_locked(root: &Path, canonical_url: &str) -> Result { let key = cache_key(canonical_url); + let lock = slot_lock(&key); + let _guard = lock.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); let repo_dir = root.join(&key); if !repo_dir.join(".git").exists() { return Err(CacheError::Git(format!( @@ -172,12 +285,12 @@ pub(crate) fn refetch_clone(canonical_url: &str) -> Result if size > cap { // Ownership-guarded removal, not a raw `remove_dir_all` — see // crates/khive-pack-git/docs/api/cache.md#refetch_clone. - remove_owned_entry(&root, &repo_dir)?; + remove_owned_entry(root, &repo_dir)?; return Err(CacheError::CloneTooLarge { bytes: size, cap }); } touch(&repo_dir)?; - evict_lru(&root, &repo_dir)?; + evict_lru(root, &repo_dir)?; Ok(repo_dir) } @@ -186,15 +299,23 @@ pub(crate) fn refetch_clone(canonical_url: &str) -> Result /// crates/khive-pack-git/docs/api/cache.md#reclone. pub(crate) fn reclone(canonical_url: &str) -> Result { let root = scratch_root(); - std::fs::create_dir_all(&root)?; + let outcome = reclone_locked(&root, canonical_url); + finish_mutation(&root, &outcome); + outcome +} + +fn reclone_locked(root: &Path, canonical_url: &str) -> Result { + std::fs::create_dir_all(root)?; let key = cache_key(canonical_url); + let lock = slot_lock(&key); + let _guard = lock.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); let repo_dir = root.join(&key); let cap = clone_max_bytes(); - remove_owned_entry(&root, &repo_dir)?; - install_fresh_clone(canonical_url, &root, &repo_dir, cap)?; + remove_owned_entry(root, &repo_dir)?; + install_fresh_clone(canonical_url, root, &repo_dir, cap)?; - evict_lru(&root, &repo_dir)?; + evict_lru(root, &repo_dir)?; Ok(repo_dir) } @@ -394,6 +515,13 @@ fn dir_size(path: &Path) -> Result { Ok(total) } +fn is_cache_key_name(name: &str) -> bool { + name.len() == 16 + && name + .chars() + .all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase()) +} + /// Whether `path` is a directory `ensure_clone` could plausibly have /// created: a 16-lowercase-hex `cache_key`-shaped real directory (not a /// symlink) containing both a `.git` entry and the `.khive-last-used` @@ -403,11 +531,7 @@ fn is_owned_entry(path: &Path) -> bool { Some(n) => n, None => return false, }; - if name.len() != 16 - || !name - .chars() - .all(|c| c.is_ascii_hexdigit() && !c.is_ascii_uppercase()) - { + if !is_cache_key_name(name) { return false; } match std::fs::symlink_metadata(path) { @@ -423,7 +547,32 @@ fn is_owned_entry(path: &Path) -> bool { /// error); a listed candidate entry vanishing mid-walk IS tolerated /// (skipped). See crates/khive-pack-git/docs/api/cache.md#evict_lru. fn evict_lru(root: &Path, keep: &Path) -> Result<(), CacheError> { - let mut entries: Vec<(PathBuf, SystemTime, u64)> = Vec::new(); + evict_to_caps(root, Some(keep)) +} + +/// Enforce the cache caps with no protected slot: evict least-recently-used +/// owned clones until both caps hold, treating every owned slot as a +/// candidate. Run after a cache mutation releases its slot lock on a FAILURE +/// path (#960). A failed `ensure_clone`/`refetch_clone`/`reclone` skips the +/// success-path `evict_lru`, and a concurrent eviction may have deferred this +/// slot (its lock was held) — so without this pass the caps can stay exceeded +/// with nothing scheduled to correct them. See +/// crates/khive-pack-git/docs/api/cache.md#enforce_caps. +fn enforce_caps(root: &Path) -> Result<(), CacheError> { + evict_to_caps(root, None) +} + +/// Shared eviction core. `keep = Some(slot)` protects that slot from eviction +/// and requires it to still exist (its vanishing propagates as an error); +/// `keep = None` protects nothing. Holds `EVICTION_LOCK` for the whole pass +/// and takes each candidate's `slot_lock` with `try_lock`, deferring (skipping) +/// a candidate whose lock is currently held rather than blocking on it — the +/// deferred candidate's own mutation runs its own tail pass once it settles. +fn evict_to_caps(root: &Path, keep: Option<&Path>) -> Result<(), CacheError> { + let _eviction_guard = EVICTION_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let mut entries: Vec<(PathBuf, String, SystemTime, u64)> = Vec::new(); let read_dir = std::fs::read_dir(root).map_err(|e| io_err("evict_lru: read_dir root", root, e))?; for entry in read_dir { @@ -436,7 +585,23 @@ fn evict_lru(root: &Path, keep: &Path) -> Result<(), CacheError> { Err(e) => return Err(io_err("evict_lru: read_dir entry", root, e)), }; let p = entry.path(); - if !p.is_dir() || p == keep || !is_owned_entry(&p) { + if keep == Some(p.as_path()) { + continue; + } + let Some(key) = p.file_name().and_then(|name| name.to_str()) else { + continue; + }; + if !is_cache_key_name(key) || !is_owned_entry(&p) { + continue; + } + let key = key.to_string(); + let lock = slot_lock(&key); + let _candidate_guard = match lock.try_lock() { + Ok(guard) => guard, + Err(std::sync::TryLockError::Poisoned(poisoned)) => poisoned.into_inner(), + Err(std::sync::TryLockError::WouldBlock) => continue, + }; + if !p.is_dir() || !is_owned_entry(&p) { continue; } let mtime = std::fs::metadata(p.join(MARKER_FILE)) @@ -450,23 +615,44 @@ fn evict_lru(root: &Path, keep: &Path) -> Result<(), CacheError> { Err(CacheError::Io(e)) if e.kind() == std::io::ErrorKind::NotFound => continue, Err(e) => return Err(e), }; - entries.push((p, mtime, size)); + entries.push((p, key, mtime, size)); } - entries.sort_by_key(|(_, mtime, _)| *mtime); + entries.sort_by_key(|(_, _, mtime, _)| *mtime); - let keep_size = dir_size(keep)?; - let mut total: u64 = entries.iter().map(|(_, _, s)| s).sum::() + keep_size; - let mut count = entries.len() + 1; + let (keep_size, keep_count) = match keep { + Some(keep) => (dir_size(keep)?, 1), + None => (0, 0), + }; + let mut total: u64 = entries.iter().map(|(_, _, _, s)| s).sum::() + keep_size; + let mut count = entries.len() + keep_count; let cap_repos = max_repos(); let cap_bytes = max_total_bytes(); - for (path, _, size) in entries { + for (path, key, _, measured_size) in entries { + if count <= cap_repos && total <= cap_bytes { + break; + } + let lock = slot_lock(&key); + let _candidate_guard = match lock.try_lock() { + Ok(guard) => guard, + Err(std::sync::TryLockError::Poisoned(poisoned)) => poisoned.into_inner(), + Err(std::sync::TryLockError::WouldBlock) => continue, + }; + if !is_owned_entry(&path) { + count = count.saturating_sub(1); + total = total.saturating_sub(measured_size); + continue; + } + let current_size = dir_size(&path)?; + total = total + .saturating_sub(measured_size) + .saturating_add(current_size); if count <= cap_repos && total <= cap_bytes { break; } - let _ = std::fs::remove_dir_all(&path); + remove_owned_entry(root, &path)?; count -= 1; - total = total.saturating_sub(size); + total = total.saturating_sub(current_size); } Ok(()) } @@ -492,6 +678,20 @@ mod tests { p } + fn slot_lock_registry_len() -> usize { + SLOT_LOCKS + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .len() + } + + fn slot_lock_registry_capacity() -> usize { + SLOT_LOCKS + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .capacity() + } + /// A `git clone` failure must not leave a `.staging-` dir behind. #[test] fn ensure_clone_cleans_up_staging_dir_on_clone_failure() { @@ -544,6 +744,77 @@ mod tests { std::env::remove_var("KHIVE_GIT_DIGEST_CACHE_MAX_BYTES"); } + /// Issue #960: a cache mutation that FAILS must still leave the caps + /// enforced. A failed `refetch_clone` returns before its success-path + /// `evict_lru`, and under concurrency a sibling eviction pass can defer + /// this slot (its lock is held) — so without a post-release cap pass the + /// caps stay exceeded with nothing scheduled to correct them. + /// `finish_mutation` runs `enforce_caps` once the lock is free. This pins + /// the settled-state invariant the concurrent case also relies on: two + /// owned slots over a repo cap of 1, a failed refetch of one, and + /// afterward exactly one owned slot remains. + #[test] + fn a_failed_mutation_enforces_caps_over_the_settled_set() { + let _guard = ENV_MUTEX.blocking_lock(); + let dir = tempfile::tempdir().expect("tempdir"); + std::env::set_var("KHIVE_GIT_DIGEST_SCRATCH_ROOT", dir.path()); + std::env::set_var("KHIVE_GIT_DIGEST_CACHE_MAX_REPOS", "1"); + std::env::set_var("KHIVE_GIT_DIGEST_CACHE_MAX_BYTES", "1000000000"); + + let root = dir.path(); + // Two owned slots present, one over the repo cap of 1. The slot we + // will fail to refetch is the newer one; the older sibling is the LRU + // eviction victim, showing the failed mutation enforced the cap over a + // slot it was not itself operating on. + let url_victim = "https://example.com/lru-victim.git"; + let url_target = "https://example.com/refetch-target.git"; + let key_victim = cache_key(url_victim); + let key_target = cache_key(url_target); + assert_ne!( + key_victim, key_target, + "distinct urls must map to distinct slots" + ); + + let victim = make_owned_entry(root, &key_victim, true); + // Ensure a real mtime gap so `victim` is unambiguously the LRU. + std::thread::sleep(std::time::Duration::from_millis(20)); + let target = make_owned_entry(root, &key_target, true); + + // `target`'s `.git` is an empty directory, not a real repository, so + // `git fetch --refetch` fails deterministically with no network. The + // mutation therefore returns Err before its own eviction pass. + let result = refetch_clone(url_target); + assert!( + result.is_err(), + "refetch of a slot with no valid git repo must fail: {result:?}" + ); + + // The failed mutation must nonetheless have enforced the caps. + let owned: Vec<_> = std::fs::read_dir(root) + .expect("read scratch root") + .filter_map(|e| e.ok()) + .map(|e| e.path()) + .filter(|p| is_owned_entry(p)) + .collect(); + assert_eq!( + owned.len(), + 1, + "a failed mutation must leave the repo cap enforced, found: {owned:?}" + ); + assert!( + target.exists(), + "the newer (refetched) slot must survive as the non-LRU entry" + ); + assert!( + !victim.exists(), + "the older sibling must be evicted to satisfy the repo cap" + ); + + std::env::remove_var("KHIVE_GIT_DIGEST_SCRATCH_ROOT"); + std::env::remove_var("KHIVE_GIT_DIGEST_CACHE_MAX_REPOS"); + std::env::remove_var("KHIVE_GIT_DIGEST_CACHE_MAX_BYTES"); + } + #[test] fn evict_lru_only_touches_children_of_root() { let _guard = ENV_MUTEX.blocking_lock(); @@ -593,6 +864,27 @@ mod tests { std::env::remove_var("KHIVE_GIT_DIGEST_CACHE_MAX_BYTES"); } + #[test] + fn evict_lru_does_not_grow_registry_for_unrelated_scratch_root_children() { + let _guard = ENV_MUTEX.blocking_lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path().join("scratch-root"); + std::fs::create_dir_all(&root).unwrap(); + let kept = make_owned_entry(&root, "4444444444444444", true); + + for index in 0..32 { + std::fs::create_dir_all(root.join(format!("operator-data-{index}"))).unwrap(); + } + + let baseline = slot_lock_registry_len(); + evict_lru(&root, &kept).expect("evict"); + assert_eq!( + slot_lock_registry_len(), + baseline, + "unrelated scratch-root children must not allocate slot locks" + ); + } + #[test] fn evict_lru_never_removes_an_owned_looking_dir_missing_the_marker() { let _guard = ENV_MUTEX.blocking_lock(); @@ -607,12 +899,18 @@ mod tests { let no_marker = make_owned_entry(&root, "5555555555555555", false); let kept = make_owned_entry(&root, "6666666666666666", true); + let baseline = slot_lock_registry_len(); evict_lru(&root, &kept).expect("evict"); assert!( no_marker.exists(), "an owned-looking directory without the marker must survive eviction" ); + assert_eq!( + slot_lock_registry_len(), + baseline, + "an unowned cache-shaped child must not allocate a slot lock" + ); std::env::remove_var("KHIVE_GIT_DIGEST_CACHE_MAX_REPOS"); std::env::remove_var("KHIVE_GIT_DIGEST_CACHE_MAX_BYTES"); @@ -1095,4 +1393,170 @@ mod tests { std::env::remove_var("KHIVE_GIT_DIGEST_SCRATCH_ROOT"); } + + // ── issue #805: same-key mutation serialization ──────────────────────── + + /// `slot_lock` must serialize a *repeated* lookup of the same cache key + /// (both calls return handles to the same underlying `Mutex`) while + /// leaving a distinct key completely unaffected -- the acceptance + /// criterion from issue #805 ("serialize per-key without serializing + /// distinct keys"). + #[test] + fn slot_lock_serializes_same_key_but_not_distinct_keys() { + let _env_guard = ENV_MUTEX.blocking_lock(); + let key_a = "abcdef0123456789"; + let key_b = "fedcba9876543210"; + + let lock_a1 = slot_lock(key_a); + let guard = lock_a1.lock().expect("lock key_a"); + + let lock_a2 = slot_lock(key_a); + assert!( + lock_a2.try_lock().is_err(), + "a second lookup of the same cache key must observe the first as held" + ); + + let lock_b = slot_lock(key_b); + assert!( + lock_b.try_lock().is_ok(), + "locking a distinct cache key must never be blocked by another key's held lock" + ); + + drop(guard); + drop(lock_a1); + + let guard = lock_a2.lock().expect("re-lock key_a"); + let lock_a3 = slot_lock(key_a); + assert!( + lock_a3.try_lock().is_err(), + "dropping one handle must not replace the lock while another handle still exists" + ); + drop(guard); + } + + #[test] + fn released_distinct_slot_locks_do_not_grow_the_registry() { + let _env_guard = ENV_MUTEX.blocking_lock(); + let baseline = slot_lock_registry_len(); + let baseline_capacity = slot_lock_registry_capacity(); + let locks: Vec<_> = (0..64) + .map(|index| slot_lock(&format!("released-distinct-key-{index}"))) + .collect(); + + assert_eq!( + slot_lock_registry_len(), + baseline + locks.len(), + "live handles must remain registered" + ); + drop(locks); + assert_eq!( + slot_lock_registry_len(), + baseline, + "released handles must remove idle registry entries" + ); + assert!( + slot_lock_registry_capacity() <= baseline_capacity, + "released handles must not retain registry capacity above its baseline" + ); + } + + /// An eviction pass for one key must not delete another key while that + /// key is inside its slot-locked mutation span. The active thread models + /// the interval in which `ensure_clone` is blocked in `git fetch`; before + /// eviction consulted candidate locks, the count cap deleted `active` + /// despite its guard and the operation resumed over a missing slot. + #[test] + fn eviction_defers_a_candidate_with_an_active_slot_mutation() { + let _guard = ENV_MUTEX.blocking_lock(); + let dir = tempfile::tempdir().expect("tempdir"); + std::env::set_var("KHIVE_GIT_DIGEST_CACHE_MAX_REPOS", "1"); + std::env::set_var("KHIVE_GIT_DIGEST_CACHE_MAX_BYTES", "1000000000"); + + let root = dir.path(); + let active_key = "aaaaaaaaaaaaaaaa"; + let active = make_owned_entry(root, active_key, true); + std::thread::sleep(std::time::Duration::from_millis(20)); + let keep = make_owned_entry(root, "bbbbbbbbbbbbbbbb", true); + + let active_lock = slot_lock(active_key); + let (started_tx, started_rx) = std::sync::mpsc::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + let active_for_thread = active.clone(); + let handle = std::thread::spawn(move || { + let _active_guard = active_lock.lock().expect("lock active slot"); + started_tx.send(()).expect("signal active mutation"); + release_rx.recv().expect("release active mutation"); + assert!( + active_for_thread.exists(), + "an active slot must still exist when its mutation resumes" + ); + std::fs::write(active_for_thread.join("mutation-complete"), b"") + .expect("complete active mutation"); + }); + + started_rx.recv().expect("wait for active mutation"); + evict_lru(root, &keep).expect("evict around active slot"); + assert!(active.exists(), "eviction must defer the active candidate"); + release_tx.send(()).expect("release active mutation"); + handle.join().expect("active mutation thread"); + assert!(active.join("mutation-complete").exists()); + + std::env::remove_var("KHIVE_GIT_DIGEST_CACHE_MAX_REPOS"); + std::env::remove_var("KHIVE_GIT_DIGEST_CACHE_MAX_BYTES"); + } + + /// The concrete regression issue #805 describes: before `slot_lock`, + /// concurrent `ensure_clone` calls for the same never-before-cached URL + /// could both observe an absent slot and both proceed to + /// `install_fresh_clone`, racing `std::fs::rename` onto the same + /// `//` path -- the loser's rename fails because the + /// winner already populated a non-empty directory there. With same-key + /// mutation serialized, the loser instead waits, observes the slot the + /// winner installed, and takes the existing-slot (`fetch`) path -- every + /// concurrent call succeeds and resolves to the same slot. + #[test] + fn concurrent_ensure_clone_on_same_key_never_races_the_slot() { + let _guard = ENV_MUTEX.blocking_lock(); + let scratch = tempfile::tempdir().expect("tempdir"); + std::env::set_var("KHIVE_GIT_DIGEST_SCRATCH_ROOT", scratch.path()); + + let origin_dir = tempfile::tempdir().expect("tempdir"); + init_origin_with_one_commit(origin_dir.path()); + let canonical = origin_dir.path().to_str().unwrap().to_string(); + + const CONCURRENCY: usize = 6; + let barrier = std::sync::Arc::new(std::sync::Barrier::new(CONCURRENCY)); + let handles: Vec<_> = (0..CONCURRENCY) + .map(|_| { + let canonical = canonical.clone(); + let barrier = barrier.clone(); + std::thread::spawn(move || { + barrier.wait(); + ensure_clone(&canonical) + }) + }) + .collect(); + + let results: Vec<_> = handles + .into_iter() + .map(|h| h.join().expect("ensure_clone thread panicked")) + .collect(); + + for result in &results { + assert!( + result.is_ok(), + "concurrent ensure_clone calls on the same key must never race the slot: {result:?}" + ); + } + let first = results[0].as_ref().unwrap(); + for result in &results[1..] { + assert_eq!( + result.as_ref().unwrap(), + first, + "every concurrent call must resolve to the same cache slot" + ); + } + + std::env::remove_var("KHIVE_GIT_DIGEST_SCRATCH_ROOT"); + } }