From a83591c417164875d7e105bb0491db832ddf5c28 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 8 Oct 2026 10:21:37 +0700 Subject: [PATCH] fix(test): isolate unit tests from the real data dir; bound vector index lock wait (#587) Unit tests now resolve the CodeCora data root to a process-wide scratch dir unless CODECORA_HOME is set, so index/watch tests no longer open the developer's real global vector index or hang behind another cora process. acquire_file_lock polls try_lock for 15s then errors naming the lock path. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- src/data_dir.rs | 121 +++++++++++++++++++++++--------------------- src/index/vector.rs | 68 ++++++++++++++++++++++--- 2 files changed, 125 insertions(+), 64 deletions(-) diff --git a/src/data_dir.rs b/src/data_dir.rs index 9f802e7..53682b9 100644 --- a/src/data_dir.rs +++ b/src/data_dir.rs @@ -16,15 +16,43 @@ pub const CODECORA_HOME_ENV: &str = "CODECORA_HOME"; /// (not set) → $HOME/.codecora/ /// ``` pub fn codecora_home() -> PathBuf { - if let Ok(home) = std::env::var(CODECORA_HOME_ENV) { - PathBuf::from(home) - } else { - dirs::home_dir() + let override_dir = std::env::var_os(CODECORA_HOME_ENV); + // Unit tests must never read or write the developer's real `~/.codecora` + // (the global vector index there is flock-ed by any running `cora`, #587). + // Without an explicit override they get one process-wide scratch dir. + #[cfg(test)] + if override_dir.is_none() { + return test_home().to_path_buf(); + } + resolve_home(override_dir, dirs::home_dir()) +} + +/// Pure resolution rule behind [`codecora_home`] (no env/FS access). +fn resolve_home(override_dir: Option, home: Option) -> PathBuf { + match override_dir { + Some(dir) => PathBuf::from(dir), + None => home .expect("Cannot determine home directory. Set CODECORA_HOME or HOME.") - .join(".codecora") + .join(".codecora"), } } +/// Process-wide scratch data root for unit tests. One dir per test process +/// matches the process-global vector cache, and needs no env mutation, so +/// parallel tests cannot race on it. It is not removed at process exit; it +/// is small and lives under the OS temp dir. +#[cfg(test)] +fn test_home() -> &'static std::path::Path { + static HOME: std::sync::LazyLock = std::sync::LazyLock::new(|| { + tempfile::Builder::new() + .prefix("cora-test-home-") + .tempdir() + .expect("create test data dir") + .keep() + }); + &HOME +} + /// Returns the data directory for a specific CodeCora product. /// /// ```text @@ -78,68 +106,47 @@ pub fn ensure_data_dir() -> anyhow::Result { #[cfg(test)] mod tests { use super::*; - use std::sync::Mutex; - - // Ensure tests that mutate CODECORA_HOME don't run concurrently. - static ENV_LOCK: Mutex<()> = Mutex::new(()); #[test] - fn test_codecora_home_returns_path() { - let _guard = ENV_LOCK.lock().unwrap(); - unsafe { - std::env::remove_var(CODECORA_HOME_ENV); - } - let path = codecora_home(); - assert!(path.ends_with(".codecora")); + fn resolve_home_uses_override_then_home() { + assert_eq!( + resolve_home(Some("/custom".into()), Some(PathBuf::from("/h"))), + PathBuf::from("/custom") + ); + assert_eq!( + resolve_home(None, Some(PathBuf::from("/h"))), + PathBuf::from("/h/.codecora") + ); } #[test] - fn test_product_data_dir() { - let _guard = ENV_LOCK.lock().unwrap(); - unsafe { - std::env::remove_var(CODECORA_HOME_ENV); + fn unit_tests_never_resolve_into_the_real_home() { + if std::env::var_os(CODECORA_HOME_ENV).is_some() { + return; // explicit override in the developer's shell: honoured + } + let real = dirs::home_dir().unwrap().join(".codecora"); + for p in [ + codecora_home(), + cora_data_dir(), + product_data_dir("x"), + graph_db_path(), + ] { + assert!(!p.starts_with(&real), "{p:?} is under the real home"); + assert!(p.starts_with(test_home()), "{p:?} is not under test home"); } - let path = product_data_dir("cora-code"); - assert!(path.ends_with(".codecora/cora-code")); } #[test] - fn test_graph_db_path() { - let _guard = ENV_LOCK.lock().unwrap(); - unsafe { - std::env::remove_var(CODECORA_HOME_ENV); - } - let path = graph_db_path(); + fn product_dirs_are_nested_under_home() { + assert_eq!( + product_data_dir("cora-code"), + codecora_home().join("cora-code") + ); + assert_eq!(cora_data_dir(), product_data_dir("cora-code")); + let db = graph_db_path(); assert!( - path.ends_with(".codecora/cora-code/cora.db") - || path.ends_with(".codecora/cora-code/graph.db"), - "graph_db_path should end with cora.db (or graph.db on migration failure), got: {path:?}" + db.ends_with("cora.db") || db.ends_with("graph.db"), + "{db:?}" ); } - - #[test] - fn test_cora_data_dir() { - let _guard = ENV_LOCK.lock().unwrap(); - unsafe { - std::env::remove_var(CODECORA_HOME_ENV); - } - let path = cora_data_dir(); - assert!(path.ends_with(".codecora/cora-code")); - // Should not have trailing slash - let s = path.to_string_lossy(); - assert!(!s.ends_with('/')); - } - - #[test] - fn test_env_override() { - let _guard = ENV_LOCK.lock().unwrap(); - unsafe { - std::env::set_var(CODECORA_HOME_ENV, "/tmp/test-codecora"); - } - let path = codecora_home(); - assert_eq!(path, PathBuf::from("/tmp/test-codecora")); - unsafe { - std::env::remove_var(CODECORA_HOME_ENV); - } - } } diff --git a/src/index/vector.rs b/src/index/vector.rs index 8619b34..9d92d8a 100644 --- a/src/index/vector.rs +++ b/src/index/vector.rs @@ -592,21 +592,52 @@ pub fn cosine_distance_to_similarity(distance: f32) -> f32 { (1.0 - distance).clamp(0.0, 1.0) } +/// How long [`acquire_file_lock`] waits for another process to release the +/// index lock before giving up. The lock is held for the lifetime of any +/// `cora` process that loaded the index, so an unbounded wait would hang +/// forever behind a long-running scan (#587). +const FILE_LOCK_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(15); + +/// Poll interval while waiting for a busy lock. +const FILE_LOCK_POLL: std::time::Duration = std::time::Duration::from_millis(50); + fn acquire_file_lock(path: &Path) -> Result { + acquire_file_lock_within(path, FILE_LOCK_TIMEOUT) +} + +/// Take the exclusive lock on `path`, polling until `timeout` elapses. +/// Returns a descriptive error instead of blocking indefinitely. +fn acquire_file_lock_within(path: &Path, timeout: std::time::Duration) -> Result { let file = File::options() .read(true) .write(true) .open(path) .with_context(|| format!("open usearch file for locking: {}", path.display()))?; - if file.try_lock_exclusive().is_ok() { - tracing::debug!("usearch file lock acquired: {}", path.display()); - } else { - tracing::debug!("usearch file lock busy, waiting..."); - file.lock_exclusive() - .context("acquire exclusive file lock on usearch")?; + let start = std::time::Instant::now(); + loop { + match file.try_lock_exclusive() { + Ok(()) => { + tracing::debug!("usearch file lock acquired: {}", path.display()); + return Ok(file); + } + Err(e) if e.kind() == fs2::lock_contended_error().kind() => { + if start.elapsed() >= timeout { + anyhow::bail!( + "timed out after {}s waiting for the vector index lock at {}; \ + another cora process may be holding it", + timeout.as_secs_f32(), + path.display() + ); + } + std::thread::sleep(FILE_LOCK_POLL); + } + Err(e) => { + return Err(e) + .with_context(|| format!("acquire exclusive file lock on {}", path.display())); + } + } } - Ok(file) } fn atomic_write(path: &std::path::Path, data: &[u8]) -> Result<()> { @@ -636,6 +667,29 @@ mod tests { STORE_LOCK.lock().unwrap_or_else(|e| e.into_inner()) } + #[test] + fn locked_index_errors_instead_of_blocking() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("held.usearch"); + std::fs::write(&path, []).unwrap(); + let holder = acquire_file_lock_within(&path, std::time::Duration::from_secs(1)).unwrap(); + + let started = std::time::Instant::now(); + let err = acquire_file_lock_within(&path, std::time::Duration::from_millis(200)) + .expect_err("second lock must time out while the first is held"); + assert!(started.elapsed() < std::time::Duration::from_secs(5)); + let msg = format!("{err:#}"); + assert!( + msg.contains("another cora process may be holding it"), + "{msg}" + ); + assert!(msg.contains("held.usearch"), "{msg}"); + + drop(holder); + acquire_file_lock_within(&path, std::time::Duration::from_secs(1)) + .expect("lock is acquirable once released"); + } + #[test] fn test_empty_search() { let _g = with_store_lock();