main is red: the sandbox assertion holds the writers' lock, not just #[serial] - #286
Merged
Merged
Conversation
… #[serial]
`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.
…as vacuous 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.
Broccolito
added a commit
that referenced
this pull request
Sep 12, 2026
Broccolito
added a commit
that referenced
this pull request
Sep 12, 2026
Broccolito
added a commit
that referenced
this pull request
Sep 12, 2026
Textually clean — zero conflicts. The "ctor conflict" the previous pass was resolving had already been settled in this branch's own earlier merges of #282 (ccdd9b2) and #280 (055cb08); what was left was an unpushed branch, not an unfinished resolution. GitHub reported DIRTY because the remote tip (24a6d65) was 23 commits behind the local branch. The one thing worth writing down is that #286 and this branch are the SAME rule at two layers, not two patches, and the difference between them is not a matter of taste: * A subject that resolves `BIOROUTER_PATH_ROOT` **live** and needs it held still takes `env_lock` and installs the RECORDED sandbox root (`pin_sandbox_path_root`) — the two call sites this branch corrects. * A subject that resolves it live where the live value IS the assertion takes `env_lock` with an EMPTY set — #286's `the_session_database_is_not _the_developers`. Pinning there would assert the value it had just written, which #286 shipped once and corrected. * A subject that is already FROZEN takes no lock at all and compares against the recorded root. `Config::global()` is a `OnceCell<Config>` and `SHARED_STORE_ROOT` a `LazyLock<PathBuf>` forced by the ctor (#282), so neither re-reads the variable and there is nothing for a lock to serialise — pinning them would be #286's vacuity in this crate. All three say the same thing: after main starts, the live variable answers "whichever of ~30 relocating tests holds it right now", so the only stable answer is one recorded before any test ran. `only_the_resolver_and_the_sandbox_read_the_path_root_variable` enforces that mechanically for `crates/biorouter/src`. Nothing in this branch was made redundant by #280, #282 or #286, so nothing was deleted. #280's five removed collision defences stay removed: verified that `seeded_target` carries no band counter and `reserve_child_session_ids` survives only in the doc comment recording its deletion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mainis red at055cb087—test (ubuntu-latest), exactly one failure:Not a product defect, and not #280's — though #280's merge is what exposed it.
The mechanism
crates/biorouter-server/src/test_sandbox.rsis#[path]-included bytests/session_store_survives_a_relocated_path_root.rs, so itstestsmodule runs inside that binary — alongsidea_relocated_path_root_cannot_move_the_session_store, whose entire job is to relocateBIOROUTER_PATH_ROOTunderenv_lock.Paths::data_dir()re-reads that variable on every call — the file's own doc comment says so, two tests further down.#[serial_test::serial], a different mutex fromenv_lock. So the two ran concurrently and the assertion read the relocator'sTempDir. The panic message names it exactly:/tmp/.tmpMnHK86/data.It has been latent since that binary landed in #282. #280 then deleted five hand-rolled session-id spacer bands, taking
create_sessioncalls from 4264 to 1167 in one lib-test run — which moved the schedule enough to open the window. A scheduling-dependent assertion, not a scheduling-dependent product.The fix
The same rule #281 corrected
pinned_path_rootto obey on the same day: a reader of a process-global must hold the lock its writers hold, and read the value only after taking it.So the assertion takes
env_lock— pinned tosandbox_root()— before callingPaths::data_dir(). Pinning to the ctor's own answer rather than to whatever happens to be set makes the claim stronger: it asserts the value the ctor installed, under the writers' lock. And it can no longer observe a relocation by construction, rather than merely rarely, which is the difference between this and a longer timeout.Verified
cargo test -p biorouter-server --test session_store_survives_a_relocated_path_root: 3/3, all three tests including the relocator.cargo fmt --checkclean.🤖 Generated with Claude Code