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

[Broccolito]

Two CI flake families, measured, both independent of the SQLite busy_timeout
contention and the subagent_handle::HANDLES deadlock being fixed elsewhere.
Neither turned out to be a production bug — both are test-isolation defects,
and Family A's test had also stopped testing the regression it is named
for, which is the more important finding of the two.

Family A — applying_a_workflow_hides_a_base_that_lands_mid_call

Measured before this branch

• On main itself: run 34679956636 (head 9096d371), job 103516636542,
  test (windows-latest) — test result: FAILED. 608 passed; 1 failed,
  panicked at crates\biorouter-server\src\routes\agent.rs:5697:13: the creator must begin writing gamma while the test is waiting.
• Earlier the same day on PR Desktop: the Browse modals match a search word by word, ranked like the model's search (F5) #255, which has zero Rust changes, while six
  other PRs passed the same job against the same main.

What the race is

Nothing — the test stopped staging one on 2026-08-21.

create_base builds a base in .creating-<id>-<uuid> and publishes it with a
single rename, so <root>/<id> appears at the very end of the
transaction, after git init, the initial commit, the graph cache, the
classification stamp and the registry row. The barrier waited for exactly that
directory, which means:

1. The 5 s budget was a bet on a whole create_base returning, subprocesses
   included, on a Windows runner concurrently running 608 other tests. That is
   the assertion that fired: the staging barrier, not the invariant.
2. The window was already over when the apply started. With the base
   installed, the pre-fix unlocked inventory would have seen it too — so the
   test could no longer fail on its own regression.

Point 2, measured by reintroducing the pre-fix inversion in
workflow::runtime::apply_knowledge_selection (list unlocked → invert →
set_selection with a hidden list):

barrier	result against the reintroduced bug
old — wait for kb_root(root, "gamma").exists()	passes, 5/5 (vacuous)
new — wait for publication_in_progress("gamma0")	fails on attempt 0: left: ["alpha", "gamma0"], right: ["alpha"]

History: 76952512 ("stabilize Windows knowledge selection race test")
replaced a 10 ms sleep — which staged the window correctly but bet on the
creator thread being scheduled inside it — with a wait for the opposite event.
One flake was traded for a false pass plus a slower flake.

Not a production bug

KnowledgeService::set_visible_kbs takes the root lock and inverts the
caller's visible set against the inventory read inside that lock; the
caller has no way to hand it a stale snapshot. The nondeterminism was entirely
in the harness.

Fix

• KnowledgeService::publication_in_progress(id) — one read_dir, no lock, no
  subprocess — is the observable that means "a publication holds the root lock
  right now and the base is not installed yet".
• The stage loop now ends on an event either way: the creation becomes
  observable (proceed), or the creator thread finishes (the window was missed —
  stage a fresh one rather than assert into an unstaged run). A starved runner
  makes the test slower, never red.
• The one clock left is a 600 s wedge detector — ~100× the slowest whole
  create_base anyone has measured, reachable only when a creation can neither
  publish nor return, i.e. the root lock is held by something else — and it says
  that instead of hanging the job.
• a_creation_is_visible_as_staged_long_before_its_base_directory_exists
  (biorouter-mcp) pins both halves from inside the transaction via the
  creation's own checkpoints, which is the only place the claim is observable.

Family B — a_removal_prunes_the_extension_from_every_stored_session_roster

Measured before this branch

Full -p biorouter --lib suite on a merged integration tree: 4008 passed; 1 failed, panicked at crates/biorouter/src/agents/extension_manager_extension.rs:4266:79: called Result::unwrap() on an Err value: Os { code: 22, kind: InvalidInput, message: "Invalid argument" } — i.e. install_sideloaded_fixture's
create_dir_all(install_dir.join("skills").join(&skill_slug)). The same test
passes 3/3 in isolation.

The shared state, and who writes it

extensions_root() → Paths::config_dir() → Paths::get_dir reads
BIOROUTER_PATH_ROOT on every call, uncached — deliberately, because
production honours the relocation. test_sandbox's #[ctor] sets it to a
TempDir before main, and 29 further sites in the same binary then
legitimately relocate it to a TempDir of their own under env_lock:
logging, managed (×3), providers::utils (×5), session::diagnostics (×7),
agents::skills_extension (×6), agents::agent (×3), agents::reply_parts,
execution::manager (×2), extension_install::claim,
knowledge::conversation_ingest (×2).

The bug: the pin read the variable before taking the lock

fn pinned_path_root() -> env_lock::EnvGuard<'static> {
    let current = std::env::var("BIOROUTER_PATH_ROOT").ok();   // unlocked
    env_lock::lock_env([("BIOROUTER_PATH_ROOT", current.as_deref())])
}

Both call sites (agents::extension_manager_extension,
security::global_memory) documented the intent as "hold the lock, do not
change the root", and both did the opposite exactly when it mattered:

1. A relocator holds env_lock with its own TempDir.
2. The pin's unlocked read returns that root, then blocks on the lock.
3. The relocator finishes: its guard restores the sandbox root, and its
   TempDir is deleted.
4. The pin wakes and installs the deleted directory as this test's root for
   the rest of the test.

So the fixture — and Config::global(), extensions_root(), the global memory
store — resolve outside the test's own sandbox, under a path whose owner has
just removed it. the_lib_test_binary_config_root_is_sandboxed was exposed the
same way (it compared a frozen Config::global() path against a live read of
the variable).

A writer's lock cannot protect an unlocked reader. The fix is to have no
unlocked read: test_sandbox records the root in effect before main — the
one moment at which reading the variable answers the right question — and
pin_sandbox_path_root() pins that.

On the EINVAL

The isolation defect is proven and fixed; the specific errno is not
reproduced, and I would rather say so than invent a mechanism. Ruled out by
measurement on this machine (APFS):

candidate path	actual errno
invalid UTF-8 name	92 EILSEQ (and std::env::var cannot yield one)
component > 255 bytes / total > 1024	63 ENAMETOOLONG
create_dir_all inside a tree being rm -rf'd (3087 creates)	0 failures
under a nonexistent /var/folders/<hash>	13 EACCES
/-rooted (blank-home fallback)	30 EROFS
relative .config/biorouter/... (blank-home fallback)	succeeds

What is certain is that the fixture was writing somewhere it does not own. If
it recurs after this change, the root is provably the sandbox, which narrows it
to something other than the environment.

Fix, with three tests that fail before and pass after

• the_pin_source_ignores_a_root_another_test_has_installed — a thread holds
  env_lock with a foreign root and is released by a channel, not a sleep
  (every step is an event, so a starved runner makes it slower, never red), and
  the test asserts the pin source is still the sandbox. Against the pre-fix
  source it fails naming both temp directories:
  left: "…/.tmpejKell", right: "…/.tmpEPxyAn".
• the_pin_installs_the_sandbox_root_for_its_lifetime — the guard really moves
  Paths::config_dir() and extensions_root() into the sandbox.
• only_the_resolver_and_the_sandbox_read_the_path_root_variable — walks
  crates/biorouter/src and permits the read only in the resolver and in
  test_sandbox. Verified non-vacuous: re-adding the old idiom makes it fail
  naming agents/extension_manager_extension.rs:4014. Setting the variable
  under env_lock stays fine; reading it to find out where the sandbox is does
  not.

No #[serial], no retry of an assertion, no widened race budget, no narrowed
assertion, nothing #[ignore]d.

Verification

Five consecutive runs of each affected suite, sequentially (concurrent cargo
runs in one worktree produce phantom failures), on this branch rebased onto
35a67843, with BIOROUTER_DISABLE_KEYRING=true. 15/15 exit 0, zero failures.

run	-p biorouter --lib	-p biorouter-server --lib --bins	-p biorouter-mcp --lib
1	4016 passed, 0 failed — 54 s	660 + 645 passed, 0 failed — 125 s	rc=0 — 127 s *
2	4016 passed, 0 failed — 65 s	660 + 645 passed, 0 failed — 26 s	1686 passed, 0 failed — 100 s
3	4016 passed, 0 failed — 46 s	660 + 645 passed, 0 failed — 33 s	1686 passed, 0 failed — 91 s
4	4016 passed, 0 failed — 48 s	660 + 645 passed, 0 failed — 26 s	1686 passed, 0 failed — 101 s
5	4016 passed, 0 failed — 70 s	660 + 645 passed, 0 failed — 26 s	1686 passed, 0 failed — 95 s

* run 1's biorouter-mcp log was deleted by a sibling process before the
counts could be read from it; the run itself exited 0, and runs 2-5 give the
count. Wall times include the build for the first run of each suite.

Plus, on the same tree: the rewritten race test 10x on its own (0.06-0.13 s
every time, so the window is staged on the first attempt and the restage path is
not being leaned on), and the four test_sandbox tests plus
a_removal_prunes_the_extension_from_every_stored_session_roster together.

• cargo fmt --all -- --check: clean.
• ./scripts/clippy-lint.sh: clean — -D warnings, the too_many_lines
  baseline and the banned-TLS check all pass. (Worth recording: the same script
  was RED on 9096d371, the commit this work started from, in files this branch
  does not touch — clippy::result_large_err in routes/reply.rs,
  clippy::string_slice in commands/agent.rs and two too_many_lines
  baseline violations. All four were fixed upstream in the 121 commits that
  landed while this branch was being written. CI would not have caught them:
  rust.yml runs clippy without -D warnings, "informational until warnings
  are burned down".)

🤖 Generated with Claude Code