From d492b1badb16db9661ca48136ff1333f3a65dc3d Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Tue, 26 May 2026 23:41:56 +0200 Subject: [PATCH] fix(test): pin OPENHUMAN_WORKSPACE under TEST_ENV_LOCK in memory documents tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `memory::ops::documents` tests resolve the workspace from the `OPENHUMAN_WORKSPACE` env var (`memory_init` → `current_workspace_dir` → `Config::load_or_init`), but only held `GLOBAL_MEMORY_TEST_LOCK` — not the env-var lock (`TEST_ENV_LOCK`) that `config`/`update`/autonomy tests take when they set or clear `OPENHUMAN_WORKSPACE`. Under `cargo-llvm-cov` timing a sibling test could change the env between a documents test's setup and its `memory_init` call, making `memory_init` fail intermittently (e.g. `envelope_memory_handlers_report_counts_and_statuses`). `ensure_memory_client()` now returns a guard that holds `TEST_ENV_LOCK` and pins `OPENHUMAN_WORKSPACE` to the shared memory workspace for the test's duration; `ensure_shared_memory_client()` returns that workspace path so the env var and the bound client agree. Lock order is `GLOBAL_MEMORY_TEST_LOCK` → `TEST_ENV_LOCK` (no path takes the reverse), so there is no deadlock. `documents` is the only `memory::ops` test module that reads the workspace from the env, so the change is scoped there. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/openhuman/memory/ops/documents.rs | 49 +++++++++++++++++++++++- src/openhuman/memory/ops/test_support.rs | 11 +++++- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/openhuman/memory/ops/documents.rs b/src/openhuman/memory/ops/documents.rs index 9314f81bf3..bb2cca8116 100644 --- a/src/openhuman/memory/ops/documents.rs +++ b/src/openhuman/memory/ops/documents.rs @@ -496,8 +496,53 @@ mod tests { use super::*; - fn ensure_memory_client() { - crate::openhuman::memory::ops::ensure_shared_memory_client(); + /// Pins `OPENHUMAN_WORKSPACE` to the shared memory workspace for a test's + /// duration, holding [`crate::openhuman::config::TEST_ENV_LOCK`] so sibling + /// tests that mutate the env var (e.g. `config::ops`, `update::ops`, + /// autonomy settings) cannot change it mid-run. + /// + /// `documents` tests are the only `memory::ops` tests that resolve the + /// workspace from the env var (`memory_init` → `current_workspace_dir` → + /// `Config::load_or_init`), so without this pin they race those tests and + /// `memory_init` intermittently fails — surfaced under `cargo-llvm-cov` + /// timing. Lock order is `GLOBAL_MEMORY_TEST_LOCK` → `TEST_ENV_LOCK` (the + /// test takes the memory lock first, then this guard takes the env lock); no + /// code path takes them in the opposite order, so there is no deadlock. + struct WorkspaceEnvGuard { + _env_lock: std::sync::MutexGuard<'static, ()>, + previous: Option, + } + + impl WorkspaceEnvGuard { + fn pin(workspace: &std::path::Path) -> Self { + let env_lock = crate::openhuman::config::TEST_ENV_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let previous = std::env::var_os("OPENHUMAN_WORKSPACE"); + std::env::set_var("OPENHUMAN_WORKSPACE", workspace); + Self { + _env_lock: env_lock, + previous, + } + } + } + + impl Drop for WorkspaceEnvGuard { + fn drop(&mut self) { + match self.previous.take() { + Some(value) => std::env::set_var("OPENHUMAN_WORKSPACE", value), + None => std::env::remove_var("OPENHUMAN_WORKSPACE"), + } + } + } + + /// Bind the shared memory client and pin `OPENHUMAN_WORKSPACE` to its + /// workspace for the test (see [`WorkspaceEnvGuard`]). Hold the returned + /// guard for the whole test: `let _env = ensure_memory_client();`. + #[must_use] + fn ensure_memory_client() -> WorkspaceEnvGuard { + let workspace = crate::openhuman::memory::ops::ensure_shared_memory_client(); + WorkspaceEnvGuard::pin(&workspace) } fn unique_namespace(prefix: &str) -> String { diff --git a/src/openhuman/memory/ops/test_support.rs b/src/openhuman/memory/ops/test_support.rs index ce543a93c5..f231a5d59e 100644 --- a/src/openhuman/memory/ops/test_support.rs +++ b/src/openhuman/memory/ops/test_support.rs @@ -10,11 +10,17 @@ use std::path::PathBuf; use std::sync::OnceLock; -/// Binds the process-global memory client to a single shared temp workspace. +/// Binds the process-global memory client to a single shared temp workspace and +/// returns that workspace path. /// /// Safe to call from multiple test threads concurrently — subsequent calls with /// the same workspace path return the existing client without rebinding. -pub(crate) fn ensure_shared_memory_client() { +/// +/// The returned path lets callers whose RPC path *also* resolves the workspace +/// from `OPENHUMAN_WORKSPACE` (notably `memory::ops::documents` via +/// `memory_init` → `current_workspace_dir`) pin the env var to this same path so +/// the env and the bound client agree. See `documents::tests`. +pub(crate) fn ensure_shared_memory_client() -> PathBuf { static WORKSPACE: OnceLock = OnceLock::new(); let workspace = WORKSPACE.get_or_init(|| { let tmp = tempfile::TempDir::new().expect("tempdir"); @@ -25,4 +31,5 @@ pub(crate) fn ensure_shared_memory_client() { }); crate::openhuman::memory::global::init(workspace.clone()) .expect("initialize shared test memory client"); + workspace.clone() }