test: two CI flakes that were both unlocked reads of shared state (one test had also stopped testing its regression)

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-<id>-<uuid>`, or
+    /// `.importing-…`) and moved into place with one rename, so `<root>/<id>`
+    /// 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<bool> {
         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-<id>-<uuid>` and publishes
+    /// it with a single rename, so `<root>/<id>` 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)
     // -----------------------------------------------------------------------

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-<id>-<uuid>` 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();
 

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 {

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<String> {