diff --git a/crates/biorouter-mcp/src/knowledge/service.rs b/crates/biorouter-mcp/src/knowledge/service.rs index d6bd4d816..19f1d0f5e 100644 --- a/crates/biorouter-mcp/src/knowledge/service.rs +++ b/crates/biorouter-mcp/src/knowledge/service.rs @@ -3030,6 +3030,30 @@ impl KnowledgeService { Ok(staged) } + /// Is a base publication for `id` staged under this root **right now**? + /// + /// A base is built in a sibling directory (`.creating--`, or + /// `.importing-…`) and moved into place with one rename, so `/` + /// appears only at the very END of the transaction. The two observables + /// therefore say opposite things, and mistaking one for the other is easy: + /// + /// * `paths::kb_root(root, id).exists()` — the publication has **finished** + /// and the base is installed. + /// * `publication_in_progress(id)` — a publication holds the root lock + /// right now and the base is **not installed yet**. + /// + /// This is the one a concurrent caller can wait on to be *inside* another + /// creation rather than after it, and it deliberately takes no lock: the + /// whole point is to observe while somebody else holds it. It costs one + /// `read_dir` of the knowledge root and never waits on a subprocess, so + /// waiting for it does not become a bet on how long `git init` takes — + /// which is what waiting for the installed base is. + pub fn publication_in_progress(&self, id: &str) -> bool { + self.staged_publication_paths_unlocked() + .map(|staged| staged.iter().any(|(staged_id, _)| staged_id == id)) + .unwrap_or(false) + } + fn session_primary_references_unlocked(&self, id: &str) -> Result { let dir = paths::primary_kb_sessions_dir(self.root()); if !dir.exists() { @@ -5002,6 +5026,88 @@ mod tests { assert_eq!(bases[0].name, "MS Patient Analysis"); } + /// The two observables a concurrent caller can wait on mean OPPOSITE + /// things, and a test elsewhere in this repo waited on the wrong one for + /// three weeks. + /// + /// `create_base` builds the base in `.creating--` and publishes + /// it with a single rename, so `/` exists only once the whole + /// transaction — `git init`, the initial commit, the graph cache, the + /// classification stamp, the registry row — is done and the root lock is + /// about to be released. A caller that waits for the base *directory* has + /// therefore waited for the creation to FINISH, and anything it does next + /// runs after the window, not inside it. + /// `KnowledgeService::publication_in_progress` is the marker that means + /// inside, and it is true from the first write of the transaction. + /// + /// Both halves are asserted from within the transaction itself, through the + /// creation's own checkpoints, because that is the only place the claim is + /// observable. + #[test] + fn a_creation_is_visible_as_staged_long_before_its_base_directory_exists() { + let (_dir, svc) = svc(); + let kb_root = paths::kb_root(svc.root(), "midflight"); + let mut seen: Vec<(CreateCheckpoint, bool, bool)> = Vec::new(); + svc.create_base_as_with_checkpoint( + CreateBaseSpec { + id: "midflight", + name: "midflight", + color: None, + format: KbFormat::default(), + }, + false, + &crate::knowledge::affiliation::CallerAffiliation::Unstated, + |checkpoint| { + seen.push(( + checkpoint, + svc.publication_in_progress("midflight"), + kb_root.exists(), + )); + Ok(()) + }, + ) + .unwrap(); + + let before_publication = [ + CreateCheckpoint::Files, + CreateCheckpoint::Repository, + CreateCheckpoint::GraphCache, + CreateCheckpoint::Classification, + CreateCheckpoint::Registry, + ]; + for checkpoint in before_publication { + let (_, staged, installed) = seen + .iter() + .copied() + .find(|(seen, _, _)| *seen == checkpoint) + .unwrap_or_else(|| panic!("the creation must reach {checkpoint:?}")); + assert!( + staged, + "at {checkpoint:?} the creation holds the root lock and must read as staged, \ + or nothing can wait for the window it is in" + ); + assert!( + !installed, + "at {checkpoint:?} the base directory must NOT exist yet — a caller waiting for \ + it would be waiting for this whole transaction to finish, subprocesses included" + ); + } + + let (_, staged, installed) = seen + .iter() + .copied() + .find(|(seen, _, _)| *seen == CreateCheckpoint::Published) + .expect("the creation must reach Published"); + assert!(!staged, "publication consumes the staging directory"); + assert!( + installed, + "publication is what makes the base directory exist" + ); + + assert!(!svc.publication_in_progress("midflight")); + assert!(kb_root.exists()); + } + // ----------------------------------------------------------------------- // Stage 3: the scaffold, per profile (DR-6, DR-23, requirement E) // ----------------------------------------------------------------------- diff --git a/crates/biorouter-server/src/routes/agent.rs b/crates/biorouter-server/src/routes/agent.rs index 12166477f..0d96e8e8e 100644 --- a/crates/biorouter-server/src/routes/agent.rs +++ b/crates/biorouter-server/src/routes/agent.rs @@ -6039,12 +6039,41 @@ mod knowledge_selection_tests { /// so a base created in that window was in nobody's hidden list and joined /// a workflow session the workflow had never mentioned. /// - /// The race is staged deterministically: the creator takes the root lock - /// first (`create_base` holds it for the whole git init), and the apply - /// starts a few ms later. `list_bases` takes no lock, so the old code read - /// its inventory straight through the creator's lock and missed `gamma`; - /// the locked write blocks until the base is fully installed and then hides - /// it like any other undeclared base. + /// The race is staged on an OBSERVABLE, never on the clock, and the + /// observable has to be the right one. `create_base` builds the base in + /// `.creating--` and publishes it with a single rename, so the + /// base's own directory appears only at the END of the transaction — + /// `KnowledgeService::publication_in_progress` is what means "the creator + /// holds the root lock and the base is not installed yet", which is the + /// window the apply must survive. Listing the bases takes no lock, so the + /// old code read its inventory straight through the creator's lock and + /// missed the new base; the locked write blocks until it is fully installed + /// and then hides it like any other undeclared base. + /// + /// ⚠ Both previous stagings were wrong, in opposite directions, and the + /// second looked like a fix for the first: + /// + /// * A fixed 10 ms sleep staged the window correctly but bet the creator + /// thread would be scheduled within it, which it is not on a loaded + /// Windows runner. + /// * Waiting for the base DIRECTORY to exist waits for the publishing + /// rename — i.e. for the creation to *finish*. That un-staged the race + /// entirely (with the base already installed, the buggy unlocked + /// inventory would have seen it too, so the test could no longer fail on + /// the regression it is named for) and made the 5 s budget a bet on + /// `git init` plus a commit plus a graph derive returning in time. It + /// lost that bet on `main` on 2026-09-12 and failed a PR with no Rust + /// changes the same day. + /// + /// So: no budget on anything legitimate. The stage loop ends when the + /// creation becomes observable (proceed) or when the creator thread finishes + /// (the window was missed — stage a fresh one, and never assert into an + /// unstaged run). Both conditions are events, so being starved for a whole + /// second only makes this slower, never red. The one clock left is + /// [`WEDGE`]: a creation that neither publishes nor returns is not a slow + /// runner, it is a held root lock, and that must be a sentence rather than + /// a hung job. `publication_in_progress`'s two halves are pinned in + /// `knowledge::service`, from inside the transaction. #[test] fn applying_a_workflow_hides_a_base_that_lands_mid_call() { let dir = tempfile::tempdir().unwrap(); @@ -6064,23 +6093,53 @@ mod knowledge_selection_tests { visible: ids(&["alpha"]), }); - let creator = { - let svc = Arc::clone(&svc); - std::thread::spawn(move || svc.create_base("gamma", "gamma", None).unwrap()) - }; - // Wait for an observable write that happens only after `create_base` - // has taken the root lock. A fixed sleep lost this race on slower - // Windows runners: the apply could finish before the creator thread was - // scheduled, making `gamma` legitimately visible when it landed later. - let gamma_root = biorouter_mcp::knowledge::paths::kb_root(svc.root(), "gamma"); - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); - while !gamma_root.exists() { - assert!( - std::time::Instant::now() < deadline, - "the creator must begin writing gamma while the test is waiting" - ); - std::thread::sleep(std::time::Duration::from_millis(1)); + // Not a race budget: 100x the slowest whole `create_base` anyone has + // measured, and only reachable when the creation can neither finish nor + // publish — i.e. something else is holding the knowledge root lock. + const WEDGE: std::time::Duration = std::time::Duration::from_secs(600); + + let mut staged = None; + for attempt in 0..8 { + let id = format!("gamma{attempt}"); + let creator = { + let svc = Arc::clone(&svc); + let id = id.clone(); + std::thread::spawn(move || svc.create_base(&id, &id, None).unwrap()) + }; + let wedged_at = std::time::Instant::now() + WEDGE; + let mut inside = false; + while !creator.is_finished() { + if svc.publication_in_progress(&id) { + inside = true; + break; + } + assert!( + std::time::Instant::now() < wedged_at, + "creating {id} neither published nor returned in {WEDGE:?}: the knowledge \ + root lock is held by something that is not this creation" + ); + // Cheap polling, not a budget: the loop's exits are still the two + // events above. A bare `yield_now` here spun one core hot for the + // few ms the staging takes and issued thousands of `read_dir`s — + // measurable next door, because ~660 other tests in this binary + // share a session store and some of them are sensitive to load. + std::thread::sleep(std::time::Duration::from_micros(200)); + } + if inside { + staged = Some(creator); + break; + } + // The whole creation landed before this thread looked even once. + // Nothing is asserted against that run: it would be the vacuous + // pass this staging exists to stop. Its base stays installed and + // undeclared, so the assertion below covers it too. + creator.join().unwrap(); } + let creator = staged.expect( + "every creation finished before the test could observe it mid-flight, so the \ + mid-call window was never staged", + ); + apply_workflow_knowledge_selection(&svc, "s1", &workflow).unwrap(); creator.join().unwrap(); diff --git a/crates/biorouter/src/agents/extension_manager_extension.rs b/crates/biorouter/src/agents/extension_manager_extension.rs index 493dda31d..7cb6f0320 100644 --- a/crates/biorouter/src/agents/extension_manager_extension.rs +++ b/crates/biorouter/src/agents/extension_manager_extension.rs @@ -4000,9 +4000,18 @@ mod tests { } } + /// Hold `BIOROUTER_PATH_ROOT` still for the duration of a test, so the + /// fixture below and the uninstall path under test resolve + /// `extensions_root()` to the same directory every time either asks. + /// + /// ⚠ It pins the **sandbox** root, not the variable's current value. The + /// difference is the whole bug: reading the variable to decide what to pin + /// happens before the lock is taken, so the value read is whichever of this + /// binary's ~30 relocating tests is holding `env_lock` at that instant — + /// and by the time the pin acquires the lock, that test has finished and + /// deleted its `TempDir`. See `crate::test_sandbox::pin_sandbox_path_root`. fn pinned_path_root() -> env_lock::EnvGuard<'static> { - let current = std::env::var("BIOROUTER_PATH_ROOT").ok(); - env_lock::lock_env([("BIOROUTER_PATH_ROOT", current.as_deref())]) + crate::test_sandbox::pin_sandbox_path_root() } async fn install_deletion_fixture(registry_id: &str, label: &str) -> DeletionFixture { diff --git a/crates/biorouter/src/security/global_memory.rs b/crates/biorouter/src/security/global_memory.rs index f9e77f89a..a2bb7b273 100644 --- a/crates/biorouter/src/security/global_memory.rs +++ b/crates/biorouter/src/security/global_memory.rs @@ -1203,11 +1203,18 @@ record_result(all);"#; /// /// The writers were never the problem and adding a lock to them would not /// help — `env_lock` serialises only the tasks that ASK for it, and the - /// reader here never did. Pinning to the variable's *current* value is - /// deliberate: the point is to hold the lock, not to change the root. + /// reader here never did. + /// + /// ⚠ It pins the **sandbox** root rather than the variable's current value, + /// and the version that pinned "current" was this same bug one level down: + /// the read that decides what to pin happens *before* the lock is acquired, + /// so when a relocating test holds the lock at that moment it is that + /// test's `TempDir` that gets pinned — restored and deleted a moment later, + /// while this test spends its whole life resolving under it. Holding the + /// lock was always the point; reading the environment to decide what to + /// hold was the hole. See `crate::test_sandbox::pin_sandbox_path_root`. fn pinned_store_root() -> env_lock::EnvGuard<'static> { - let current = std::env::var("BIOROUTER_PATH_ROOT").ok(); - env_lock::lock_env([("BIOROUTER_PATH_ROOT", current.as_deref())]) + crate::test_sandbox::pin_sandbox_path_root() } fn store_path_spellings() -> Vec { diff --git a/crates/biorouter/src/test_sandbox.rs b/crates/biorouter/src/test_sandbox.rs index 5e3c1a418..4799f1ee4 100644 --- a/crates/biorouter/src/test_sandbox.rs +++ b/crates/biorouter/src/test_sandbox.rs @@ -16,6 +16,19 @@ //! an editor's test runner emits. This makes the sandbox the default and lets //! the gate keep choosing its own root. +/// This binary's sandbox root, recorded before any test could move it. +/// +/// ⚠ **`std::env::var("BIOROUTER_PATH_ROOT")` is NOT a way to ask what the +/// sandbox root is, at any point after `main` starts.** Around thirty tests in +/// this binary legitimately point that variable at a `TempDir` of their own +/// under `env_lock` and put it back (`logging`, `managed`, `providers::utils`, +/// `session::diagnostics`, `agents::skills_extension`, `agents::agent`, +/// `execution::manager`, `knowledge::conversation_ingest`, …), so a read taken +/// at test time answers "whichever test is relocating it at this instant", +/// which is a different directory on every run and is deleted moments later. +/// This cell is the only stable answer, and it is why it exists. +static SANDBOX_ROOT: std::sync::OnceLock = std::sync::OnceLock::new(); + /// Run before `main`, because the placement is the whole point. /// /// `Config::global()`, `SessionManager::instance()` and `AGENT_MANAGER` are all @@ -40,6 +53,12 @@ /// whatever the *next* test was asserting. So the ctor also freezes the store's /// root here, which costs one environment read and a `PathBuf` — no pool, no /// disk, no runtime. +/// +/// ⚠ …and freezing one singleton is only half of THAT. A test that resolves a +/// path itself, rather than through a frozen cell, still needs to know which +/// root is its own — so whichever value ends up in effect is also recorded in +/// [`SANDBOX_ROOT`] here. This is the one moment in the process's life at which +/// reading the variable answers that question, because no test has run yet. #[ctor::ctor] fn sandbox_config_root_for_the_lib_test_binary() { if std::env::var_os("BIOROUTER_PATH_ROOT").is_none() { @@ -50,9 +69,65 @@ fn sandbox_config_root_for_the_lib_test_binary() { static ROOT: std::sync::OnceLock = std::sync::OnceLock::new(); let _ = ROOT.set(root); } + // Both branches, and after the set: an outer root is recorded as-is, and one + // we minted is recorded as the value we just installed. + if let Some(root) = std::env::var_os("BIOROUTER_PATH_ROOT") { + record_sandbox_root(&root); + } let _ = crate::session::session_manager::SessionManager::shared_store_root(); } +fn record_sandbox_root(root: &std::ffi::OsStr) { + // A root that is not valid UTF-8 cannot be pinned through `env_lock` + // (its API is `&str`), so it is left unrecorded rather than recorded + // lossily — a mangled root would be pinned as a *different* directory. + // `sandbox_path_root` then says so instead of silently relocating writes. + if let Some(root) = root.to_str() { + let _ = SANDBOX_ROOT.set(root.to_owned()); + } +} + +/// The config/data root every test in this binary resolves under, unless a test +/// is deliberately relocating it. +pub(crate) fn sandbox_path_root() -> &'static str { + SANDBOX_ROOT + .get() + .expect( + "the sandbox root was recorded before main; an absent value means either the ctor \ + did not run or BIOROUTER_PATH_ROOT is not valid UTF-8 and cannot be pinned", + ) + .as_str() +} + +/// Hold `BIOROUTER_PATH_ROOT` **at the sandbox root** for as long as the guard +/// lives, so a test whose subject resolves paths from that variable resolves +/// the same root every time it asks. +/// +/// ⚠ **Pin the recorded root, never the variable's current value.** Two call +/// sites used to open with +/// +/// ```ignore +/// let current = std::env::var("BIOROUTER_PATH_ROOT").ok(); +/// env_lock::lock_env([("BIOROUTER_PATH_ROOT", current.as_deref())]) +/// ``` +/// +/// whose stated intent — "hold the lock, do not change the root" — is exactly +/// right and which does the opposite whenever it matters. The read happens +/// *before* the lock is acquired, so if any of the ~30 relocating tests holds +/// the lock at that instant, `current` is **that test's `TempDir`**. The pin +/// then blocks, the relocator finishes, its guard restores the sandbox root and +/// its `TempDir` is deleted — and the pin wakes up and installs the deleted +/// directory as this test's root for the whole test. Everything the test then +/// resolves (`Config::global()`, `extensions_root()`, the global memory store) +/// points outside its own sandbox at a path whose parent may be gone, which is +/// how `a_removal_prunes_the_extension_from_every_stored_session_roster` came +/// to fail a full-suite run inside `create_dir_all` with `EINVAL` while passing +/// 3/3 in isolation. A writer's lock cannot protect an unlocked reader — this +/// helper's job is to have no unlocked read to protect. +pub(crate) fn pin_sandbox_path_root() -> env_lock::EnvGuard<'static> { + env_lock::lock_env([("BIOROUTER_PATH_ROOT", Some(sandbox_path_root()))]) +} + #[cfg(test)] mod tests { /// Nothing in this binary may resolve to the developer's live configuration. @@ -63,17 +138,146 @@ mod tests { /// still pass if something had reached `Config::global()` before the ctor. #[test] fn the_lib_test_binary_config_root_is_sandboxed() { - let root = std::env::var("BIOROUTER_PATH_ROOT") - .expect("BIOROUTER_PATH_ROOT must be sandboxed before any test in this binary runs"); + // The recorded root, not `std::env::var` — a read taken here answers + // "whichever test is relocating the variable right now", and this + // assertion would then blame the sandbox for a sibling's `TempDir`. + let root = super::sandbox_path_root(); let path = crate::config::Config::global().path(); assert!( - path.starts_with(&root), + path.starts_with(root), "Config::global() resolved to {path}, outside the sandbox at {root}. \ Something reached Config::global() before the sandbox was installed, so \ config writes from this binary land in the developer's real config." ); } + /// The pin must ignore the live variable, and this is the situation in which + /// they differ — the one that made `pinned_path_root` install another test's + /// soon-to-be-deleted `TempDir`. + /// + /// Staged with a thread that holds `env_lock` (so it is holding a foreign + /// root installed) and is released by a channel, not a sleep: every step is + /// an event, so a starved runner makes this slower and never red. Nothing + /// else in the binary can write the environment while that lock is held, + /// which is also what makes the bare `std::env::var` read below safe here. + /// + /// Before the fix, the pin's source was exactly that `std::env::var` read, + /// so the assertion below could not hold. + #[test] + fn the_pin_source_ignores_a_root_another_test_has_installed() { + let sandbox = super::sandbox_path_root(); + let foreign = tempfile::TempDir::new().expect("a foreign root to install"); + let foreign_value = foreign + .path() + .to_str() + .expect("a UTF-8 temp path") + .to_owned(); + + let (installed_tx, installed_rx) = std::sync::mpsc::channel(); + let (release_tx, release_rx) = std::sync::mpsc::channel(); + let held = foreign_value.clone(); + let holder = std::thread::spawn(move || { + let _guard = env_lock::lock_env([("BIOROUTER_PATH_ROOT", Some(held.as_str()))]); + installed_tx.send(()).expect("the test is still waiting"); + // Hold the foreign root — and the environment lock — until released. + let _ = release_rx.recv(); + }); + installed_rx.recv().expect("the holder installs its root"); + + let live = std::env::var("BIOROUTER_PATH_ROOT").ok(); + let pinned = super::sandbox_path_root(); + release_tx.send(()).expect("the holder is still parked"); + holder.join().expect("the holder exits cleanly"); + + assert_eq!( + live.as_deref(), + Some(foreign_value.as_str()), + "precondition: the holder's root really is the live value right now, so a pin \ + that reads the environment would install a directory it does not own" + ); + assert_eq!( + pinned, sandbox, + "the pin followed another test's root instead of this binary's sandbox" + ); + } + + /// Nothing in this crate may ask the environment where the sandbox root is. + /// + /// This is the shared state itself rather than either symptom: both flakes + /// this guard comes from — a pin that installed another test's `TempDir`, + /// and a sandbox assertion that compared a frozen path against a sibling's + /// root — were an unlocked read of a variable ~30 tests in this binary + /// relocate. Adding a lock to the writers cannot close that; not reading + /// can. The resolver is the one place that must read it, because production + /// honours the relocation, and this module is where the answer is recorded. + #[test] + fn only_the_resolver_and_the_sandbox_read_the_path_root_variable() { + let crate_src = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + assert!( + crate_src.is_dir(), + "the audit walks {}; if that path is wrong it passes for the wrong reason", + crate_src.display() + ); + let needle = concat!("BIOROUTER_", "PATH_ROOT"); + let allowed = ["config/paths.rs", "test_sandbox.rs"]; + + let mut offenders: Vec = Vec::new(); + let mut scanned = 0usize; + for entry in walkdir::WalkDir::new(&crate_src) { + let entry = entry.expect("the audit must not silently skip an unreadable directory"); + let path = entry.path(); + if path.extension().and_then(|ext| ext.to_str()) != Some("rs") { + continue; + } + let rel = path + .strip_prefix(&crate_src) + .unwrap() + .to_string_lossy() + .replace('\\', "/"); + scanned += 1; + if allowed.contains(&rel.as_str()) { + continue; + } + let body = std::fs::read_to_string(path).expect("a readable source file"); + for (number, line) in body.lines().enumerate() { + let code = line.trim_start(); + // Prose about the variable is fine; a read of it is not. + if code.starts_with("//") { + continue; + } + if code.contains(needle) && (code.contains("env::var") || code.contains("var_os")) { + offenders.push(format!("{rel}:{}", number + 1)); + } + } + } + assert!( + scanned > 100, + "only {scanned} files scanned — the walk found nothing to audit" + ); + assert!( + offenders.is_empty(), + "these read BIOROUTER_PATH_ROOT from the environment: {offenders:?}. After main \ + starts, that answers 'whichever test is relocating it at this instant', not 'where \ + is the sandbox'. Ask `test_sandbox::sandbox_path_root()`, or take \ + `test_sandbox::pin_sandbox_path_root()` if the subject resolves paths itself. \ + Setting it is still fine under `env_lock`." + ); + } + + /// …and the guard the helper hands out really does install that root, so a + /// subject resolving `BIOROUTER_PATH_ROOT` under it lands in the sandbox. + #[test] + fn the_pin_installs_the_sandbox_root_for_its_lifetime() { + let _pin = super::pin_sandbox_path_root(); + assert_eq!( + std::env::var("BIOROUTER_PATH_ROOT").as_deref(), + Ok(super::sandbox_path_root()) + ); + assert!(crate::config::paths::Paths::config_dir().starts_with(super::sandbox_path_root())); + assert!(crate::extension_install::brxt::extensions_root() + .starts_with(super::sandbox_path_root())); + } + /// The same claim for the session store, and it needs its own test because /// the two singletons are frozen by different calls. /// @@ -82,13 +286,18 @@ mod tests { /// `shared_store_root()` is the frozen value, and it is the one that decides /// whether this binary's `sessions.db` can end up inside a `TempDir` a test /// deletes underneath it. + /// + /// ⚠ The expected root is the **recorded** one, not `std::env::var`. It + /// arrived from #282 reading the variable live, which is the hazard the rest + /// of this module exists to close: a sibling relocating the root while this + /// test runs would make it compare a frozen store path against that + /// sibling's `TempDir` and fail, blaming the store for a harness race. #[test] fn the_lib_test_binary_session_store_is_sandboxed() { - let root = std::env::var("BIOROUTER_PATH_ROOT") - .expect("BIOROUTER_PATH_ROOT must be sandboxed before any test in this binary runs"); + let root = super::sandbox_path_root(); let pinned = crate::session::session_manager::SessionManager::shared_store_root(); assert!( - pinned.starts_with(&root), + pinned.starts_with(root), "the process session store is pinned at {}, outside the sandbox at {root}. \ Something resolved it before the ctor did, so a test that relocates \ BIOROUTER_PATH_ROOT can move this binary's sessions.db into a TempDir it \