From 55c94c523d9792880b68b74c87900d32128a6c67 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Sat, 12 Sep 2026 03:18:46 -0700 Subject: [PATCH 1/2] test(server): the sandbox assertion holds the WRITERS' lock, not just #[serial] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `main` was red at `055cb087` — `test (ubuntu-latest)`, one failure: test test_sandbox::tests::the_session_database_is_not_the_developers ... FAILED expected the per-process sandbox, got /tmp/.tmpMnHK86/data Not a product defect, and not #280's either, though #280's merge is what exposed it. The mechanism: * `crates/biorouter-server/src/test_sandbox.rs` is `#[path]`-included by `tests/session_store_survives_a_relocated_path_root.rs`, so its `tests` module runs INSIDE that binary — alongside `a_relocated_path_root_cannot_move_the_session_store`, whose whole job is to relocate `BIOROUTER_PATH_ROOT` under `env_lock`. * `Paths::data_dir()` re-reads that variable on every call. * The assertion was guarded by `#[serial_test::serial]` — a DIFFERENT mutex from `env_lock` — so the two ran concurrently and it read the relocator's `TempDir`. It was latent from the day that binary landed (#282). #280 changed how many sessions the suite creates (`create_session` 4264 -> 1167 after deleting five spacer bands), which moved the schedule enough to open the window. A scheduling-dependent assertion, not a scheduling-dependent product. The fix is the rule `pinned_path_root` was corrected to obey the same day, in #281: **a reader of a process-global must hold the lock its WRITERS hold, and read the value only after taking it.** So the assertion now takes `env_lock`, pinned to `sandbox_root()`, before calling `Paths::data_dir()`. Pinning to the ctor's own answer rather than to whatever happens to be set makes the claim stronger, not weaker — and it can no longer observe a relocation by construction, rather than merely rarely. `cargo test -p biorouter-server --test session_store_survives_a_relocated_path_root`: 3/3, and **20 consecutive runs, 20 pass / 0 fail**. `cargo fmt --check` clean. --- crates/biorouter-server/src/test_sandbox.rs | 24 +++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/crates/biorouter-server/src/test_sandbox.rs b/crates/biorouter-server/src/test_sandbox.rs index 2a3615d7..c44fb6b0 100644 --- a/crates/biorouter-server/src/test_sandbox.rs +++ b/crates/biorouter-server/src/test_sandbox.rs @@ -120,6 +120,30 @@ mod tests { #[test] #[serial_test::serial] fn the_session_database_is_not_the_developers() { + // ⚠ `#[serial]` is the WRONG mutex for this assertion, and that cost a red + // `main` (`055cb087`, `test (ubuntu-latest)`: + // "expected the per-process sandbox, got /tmp/.tmpMnHK86/data"). + // + // `Paths::data_dir()` re-reads `BIOROUTER_PATH_ROOT` on every call, and + // this file is `#[path]`-included by + // `tests/session_store_survives_a_relocated_path_root.rs`, whose own test + // relocates that variable under **`env_lock`** — a different lock from + // `serial_test`'s. So the two ran concurrently and this read the + // relocator's `TempDir`. It was latent from the day the binary was added + // and surfaced when #280 changed how many sessions the suite creates, + // which moved the schedule: a scheduling-dependent assertion, not a + // scheduling-dependent product. + // + // The rule is the one `pinned_path_root` was fixed to obey the same day: + // a reader of a process-global must hold the lock its WRITERS hold, and + // must read the value only after taking it. Pinning to the sandbox root + // rather than to whatever is there makes the assertion stronger, not + // weaker — it asserts the ctor's answer, under the writers' lock. + let root = super::sandbox_root(); + let _env = env_lock::lock_env([( + "BIOROUTER_PATH_ROOT", + Some(root.to_str().expect("the sandbox root is utf-8")), + )]); let data_dir = Paths::data_dir(); let home = std::env::var("HOME") .or_else(|_| std::env::var("USERPROFILE")) From 6ae00d7f7d4d8f3e7b8515055146b9a0df872d8b Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Sat, 12 Sep 2026 03:36:03 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fixup:=20take=20the=20writers'=20lock=20WIT?= =?UTF-8?q?HOUT=20setting=20it=20=E2=80=94=20my=20first=20attempt=20was=20?= =?UTF-8?q?vacuous?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My own fix was the failure mode #281 found in its Family A: an assertion that passes for the wrong reason. The first commit took `env_lock` **pinned to `sandbox_root()`**, then asserted that `Paths::data_dir()` — which reads the very variable just written — starts with `sandbox_root()`. True by construction. It serialised correctly against the relocator and, in doing so, stopped testing anything: the sibling guard test in this binary deliberately reads its expected root from the ENVIRONMENT rather than from `shared_store_root()` for exactly this reason, and I had just broken that property one test over. An empty variable set acquires the same mutex and changes nothing, so the assertion reads the value the `#[ctor]` installed. It is not circular either: `sandbox_root()` is `temp_dir()` + pid, never the environment. **Falsified, so it cannot pass vacuously:** BIOROUTER_PATH_ROOT=/tmp/notsandbox.epKWb6 cargo test -p biorouter-server \ --test session_store_survives_a_relocated_path_root the_session_database_is_not_the_developers ... FAILED expected the per-process sandbox, got /tmp/notsandbox.epKWb6/data (The sibling `the_session_store_is_pinned_inside_the_sandbox` fails there too, correctly — with an external root the store is pinned outside the sandbox.) Unset: 3/3 pass. --- crates/biorouter-server/src/test_sandbox.rs | 25 ++++++++++----------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/crates/biorouter-server/src/test_sandbox.rs b/crates/biorouter-server/src/test_sandbox.rs index c44fb6b0..6b168643 100644 --- a/crates/biorouter-server/src/test_sandbox.rs +++ b/crates/biorouter-server/src/test_sandbox.rs @@ -129,21 +129,20 @@ mod tests { // `tests/session_store_survives_a_relocated_path_root.rs`, whose own test // relocates that variable under **`env_lock`** — a different lock from // `serial_test`'s. So the two ran concurrently and this read the - // relocator's `TempDir`. It was latent from the day the binary was added - // and surfaced when #280 changed how many sessions the suite creates, - // which moved the schedule: a scheduling-dependent assertion, not a + // relocator's `TempDir`. Latent since that binary landed; #280 changed how + // many sessions the suite creates, which moved the schedule enough to open + // the window. A scheduling-dependent assertion, not a // scheduling-dependent product. // - // The rule is the one `pinned_path_root` was fixed to obey the same day: - // a reader of a process-global must hold the lock its WRITERS hold, and - // must read the value only after taking it. Pinning to the sandbox root - // rather than to whatever is there makes the assertion stronger, not - // weaker — it asserts the ctor's answer, under the writers' lock. - let root = super::sandbox_root(); - let _env = env_lock::lock_env([( - "BIOROUTER_PATH_ROOT", - Some(root.to_str().expect("the sandbox root is utf-8")), - )]); + // So take the WRITERS' lock — the rule `pinned_path_root` was corrected to + // obey — and take it while **setting nothing**. ⚠ Pinning the variable to + // `sandbox_root()` here would serialise correctly and make the assertion + // VACUOUS: `Paths::data_dir()` reads the same variable, so it would be + // asserting the value it had just written. An empty set acquires the mutex + // and leaves the ctor's answer in place, which is what is under test. + // `sandbox_root()` is derived from `temp_dir()` and the pid, never from the + // environment, so comparing against it is not circular either. + let _env = env_lock::lock_env(Vec::<(&str, Option<&str>)>::new()); let data_dir = Paths::data_dir(); let home = std::env::var("HOME") .or_else(|_| std::env::var("USERPROFILE"))