From 265c16aa7b9596d445d7aab6e687f0e6853af337 Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 3 Sep 2026 02:17:38 +0530 Subject: [PATCH 1/2] Serialise the tests that install process-global host seams (#130) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Module (own workspace)` fails on main roughly one run in three, always on the same assertion, and independently of what the PR changed. `tinymemory_core`'s seams are `static`s owned by the process, and three tests in this crate's single test binary install or read them: `install_wires_every_seam_this_module_can_supply` (through `install_seams`), `manual_override_outranks_a_paused_gate_and_is_bounded` and `override_member_opens_a_window_that_outranks_a_paused_gate`. libtest runs them on parallel threads, so one test's `set_scheduler_gate` or `set_manual_override` lands between another's write and its assertion. The observed failure is the first read after `set_scheduler_gate`, which is what that produces. `HostSeamsRestore` looks like it should already cover this and does not: restoring what a test found fixes ordering, so a later test cannot inherit an earlier one's gate, but says nothing about two tests running at once. All three now hold a shared lock for their whole body. It is tokio's rather than the standard library's because two of the three are `#[tokio::test]` and hold it across `.await`, which is what `clippy::await_holding_lock` exists to prevent with a `std` guard. The lock's own exclusivity is pinned by a test, because everything here depends on it and a refactor could quietly make it a no-op — a second `OnceLock`, a guard dropped at the end of its statement — with nothing else noticing. The suite would simply go back to being flaky elsewhere. --- crates/tinymemory-module/src/host_test.rs | 4 + crates/tinymemory-module/src/lib.rs | 2 + crates/tinymemory-module/src/seam_lock.rs | 86 ++++++++++++++++++++ crates/tinymemory-module/src/service/test.rs | 1 + 4 files changed, 93 insertions(+) create mode 100644 crates/tinymemory-module/src/seam_lock.rs diff --git a/crates/tinymemory-module/src/host_test.rs b/crates/tinymemory-module/src/host_test.rs index 6b665060..ff6bea99 100644 --- a/crates/tinymemory-module/src/host_test.rs +++ b/crates/tinymemory-module/src/host_test.rs @@ -255,6 +255,9 @@ async fn runtime_callbacks_and_spacy_cross_the_bus_with_their_full_payloads() { /// one's `HostSeamsRestore` had already put the globals back. #[tokio::test] async fn install_wires_every_seam_this_module_can_supply() { + // Taken before the capture, so the state this restores on drop is the + // state no other test can be moving underneath it. See `seam_lock`. + let _seams = crate::seam_lock::hold_global_seams_async().await; let _restore = HostSeamsRestore::capture(); let (connection, _callbacks) = bus_with_runtime_host().await; @@ -389,6 +392,7 @@ async fn store_policy_wakes_sleepers_only_on_resume() { fn manual_override_outranks_a_paused_gate_and_is_bounded() { use tinymemory_core::scheduler_gate as core_gate; use tinymemory_core::scheduler_gate::{PauseReason, Policy}; + let _seams = crate::seam_lock::hold_global_seams(); core_gate::clear_manual_override(); let gate = gate_for_test(); gate.store_policy(Policy::Paused { diff --git a/crates/tinymemory-module/src/lib.rs b/crates/tinymemory-module/src/lib.rs index ae8d76c1..b56ef58d 100644 --- a/crates/tinymemory-module/src/lib.rs +++ b/crates/tinymemory-module/src/lib.rs @@ -69,6 +69,8 @@ pub mod config_loader; pub mod embedding; mod host; mod provider; +#[cfg(test)] +mod seam_lock; mod service; pub use chat::{CHAT_HOST_BUS_NAME, CHAT_HOST_INTERFACE, CHAT_HOST_OBJECT_PATH}; diff --git a/crates/tinymemory-module/src/seam_lock.rs b/crates/tinymemory-module/src/seam_lock.rs new file mode 100644 index 00000000..5d3b6349 --- /dev/null +++ b/crates/tinymemory-module/src/seam_lock.rs @@ -0,0 +1,86 @@ +//! Serialises the tests that reach `tinymemory_core`'s process-global seams. +//! +//! The seams — event sink, error reporter, NLP host, scheduler gate and +//! shutdown host — are `static`s owned by the process, not by whoever installs +//! them. `libtest` runs a binary's tests on parallel threads, so two tests that +//! each install a seam are racing: one's `set_scheduler_gate` or +//! `set_manual_override` lands between the other's write and its assertion, and +//! the assertion then fails for reasons that have nothing to do with the test +//! that reported it. +//! +//! [`crate::host_test::HostSeamsRestore`] does not solve this on its own, and it +//! is worth saying why, because it looks as though it should. It restores what a +//! test found, which fixes *ordering* — a later test cannot inherit an earlier +//! one's gate. It says nothing about two tests running at the same time. +//! +//! A test that installs or reads a global seam must hold this for its whole +//! body, taken before it captures anything. See issue #130. +//! +//! The lock is `tokio`'s rather than the standard library's for one reason: two +//! of the three callers are `#[tokio::test]` and hold it across `.await`, which +//! is exactly what `clippy::await_holding_lock` exists to stop you doing with a +//! `std` guard. Hence the pair of accessors below — the sync one is not an +//! alternative to the async one, it is for the caller that has no runtime. + +use std::sync::OnceLock; + +use tokio::sync::{Mutex, MutexGuard}; + +static SEAMS: OnceLock> = OnceLock::new(); + +fn seams() -> &'static Mutex<()> { + SEAMS.get_or_init(|| Mutex::new(())) +} + +/// Blocks the current thread until no other test holds the global seams. +/// +/// # Panics +/// +/// Panics if called from inside a tokio runtime — use [`hold_global_seams_async`] +/// there. That is `blocking_lock`'s own rule, and it is the right failure: a +/// blocking wait on a runtime thread is a deadlock waiting for the right +/// scheduling. +pub(crate) fn hold_global_seams() -> MutexGuard<'static, ()> { + seams().blocking_lock() +} + +/// The same lock, awaited, for a test that runs on a tokio runtime. +pub(crate) async fn hold_global_seams_async() -> MutexGuard<'static, ()> { + seams().lock().await +} + +/// The lock is actually exclusive. +/// +/// Worth pinning, because everything above only helps if this holds, and a +/// refactor could quietly make it a no-op — a second `OnceLock`, a guard +/// dropped at the end of its own statement rather than the test body — without +/// any test noticing. The failure it guards against is invisible by nature: +/// the suite would simply go back to being flaky somewhere else. +#[test] +fn only_one_thread_holds_the_seams_at_a_time() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + static INSIDE: AtomicUsize = AtomicUsize::new(0); + static PEAK: AtomicUsize = AtomicUsize::new(0); + + std::thread::scope(|scope| { + for _ in 0..8 { + scope.spawn(|| { + for _ in 0..200 { + let _seams = hold_global_seams(); + let now = INSIDE.fetch_add(1, Ordering::SeqCst) + 1; + PEAK.fetch_max(now, Ordering::SeqCst); + std::thread::yield_now(); + INSIDE.fetch_sub(1, Ordering::SeqCst); + } + }); + } + }); + + assert_eq!( + PEAK.load(Ordering::SeqCst), + 1, + "two threads held the global seams at once, so serialising the tests \ + that install them buys nothing" + ); +} diff --git a/crates/tinymemory-module/src/service/test.rs b/crates/tinymemory-module/src/service/test.rs index 6a56ed8e..dc6bc17e 100644 --- a/crates/tinymemory-module/src/service/test.rs +++ b/crates/tinymemory-module/src/service/test.rs @@ -1017,6 +1017,7 @@ async fn override_member_opens_a_window_that_outranks_a_paused_gate() { } } + let _seams = crate::seam_lock::hold_global_seams_async().await; gate::clear_manual_override(); gate::set_scheduler_gate(std::sync::Arc::new(PausedGate)); assert!(matches!(gate::current_policy(), Policy::Paused { .. })); From e71f32dc2b0e86d9d5395315528520b922c5fb67 Mon Sep 17 00:00:00 2001 From: Ghost Scripter Date: Thu, 3 Sep 2026 02:30:18 +0530 Subject: [PATCH 2/2] Move the seam-lock test to a sibling file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review point: module-local tests do not belong in the implementation file. Correct, and the crate is consistent about it — `provider.rs`, `chat.rs`, `config.rs` and `host.rs` each declare `#[cfg(test)] #[path = "_test.rs"] mod test;`. Kept to that flat pattern rather than the `seam_lock/test.rs` the review suggested: a directory is what `service/` uses because it is already a directory, and creating one here for a single file would break the convention every other flat module in the crate follows. --- crates/tinymemory-module/src/seam_lock.rs | 38 ++---------------- .../tinymemory-module/src/seam_lock_test.rs | 40 +++++++++++++++++++ 2 files changed, 43 insertions(+), 35 deletions(-) create mode 100644 crates/tinymemory-module/src/seam_lock_test.rs diff --git a/crates/tinymemory-module/src/seam_lock.rs b/crates/tinymemory-module/src/seam_lock.rs index 5d3b6349..f96f2e2f 100644 --- a/crates/tinymemory-module/src/seam_lock.rs +++ b/crates/tinymemory-module/src/seam_lock.rs @@ -49,38 +49,6 @@ pub(crate) async fn hold_global_seams_async() -> MutexGuard<'static, ()> { seams().lock().await } -/// The lock is actually exclusive. -/// -/// Worth pinning, because everything above only helps if this holds, and a -/// refactor could quietly make it a no-op — a second `OnceLock`, a guard -/// dropped at the end of its own statement rather than the test body — without -/// any test noticing. The failure it guards against is invisible by nature: -/// the suite would simply go back to being flaky somewhere else. -#[test] -fn only_one_thread_holds_the_seams_at_a_time() { - use std::sync::atomic::{AtomicUsize, Ordering}; - - static INSIDE: AtomicUsize = AtomicUsize::new(0); - static PEAK: AtomicUsize = AtomicUsize::new(0); - - std::thread::scope(|scope| { - for _ in 0..8 { - scope.spawn(|| { - for _ in 0..200 { - let _seams = hold_global_seams(); - let now = INSIDE.fetch_add(1, Ordering::SeqCst) + 1; - PEAK.fetch_max(now, Ordering::SeqCst); - std::thread::yield_now(); - INSIDE.fetch_sub(1, Ordering::SeqCst); - } - }); - } - }); - - assert_eq!( - PEAK.load(Ordering::SeqCst), - 1, - "two threads held the global seams at once, so serialising the tests \ - that install them buys nothing" - ); -} +#[cfg(test)] +#[path = "seam_lock_test.rs"] +mod test; diff --git a/crates/tinymemory-module/src/seam_lock_test.rs b/crates/tinymemory-module/src/seam_lock_test.rs new file mode 100644 index 00000000..41278bfd --- /dev/null +++ b/crates/tinymemory-module/src/seam_lock_test.rs @@ -0,0 +1,40 @@ +//! Tests for the global-seam test lock. +//! +//! One test, and it is about the lock rather than about any seam: everything +//! [`super`] claims rests on the lock actually being exclusive. + +/// The lock is actually exclusive. +/// +/// Worth pinning, because everything in [`super`] only helps if this holds, and a +/// refactor could quietly make it a no-op — a second `OnceLock`, a guard +/// dropped at the end of its own statement rather than the test body — without +/// any test noticing. The failure it guards against is invisible by nature: +/// the suite would simply go back to being flaky somewhere else. +#[test] +fn only_one_thread_holds_the_seams_at_a_time() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + static INSIDE: AtomicUsize = AtomicUsize::new(0); + static PEAK: AtomicUsize = AtomicUsize::new(0); + + std::thread::scope(|scope| { + for _ in 0..8 { + scope.spawn(|| { + for _ in 0..200 { + let _seams = super::hold_global_seams(); + let now = INSIDE.fetch_add(1, Ordering::SeqCst) + 1; + PEAK.fetch_max(now, Ordering::SeqCst); + std::thread::yield_now(); + INSIDE.fetch_sub(1, Ordering::SeqCst); + } + }); + } + }); + + assert_eq!( + PEAK.load(Ordering::SeqCst), + 1, + "two threads held the global seams at once, so serialising the tests \ + that install them buys nothing" + ); +}