From 21421dce78d8ecac7d8ca99b3a687fb919e0374e Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sun, 23 Aug 2026 13:01:13 -0400 Subject: [PATCH] Isolate the shared test blob store between tests test_blob_store was a process-wide OnceLock singleton over one in-memory SQLite connection, so content-addressed rows written by one test were visible to every other test in the same process. nextest's process-per-test model masked the bleed, but plain cargo test failed (8/24 in fabro-workflow-version) because negative existence assertions became order-dependent. test_blob_store now builds a fresh isolated in-memory store per call, and test_database gives every database its own blob authority. Reopen-style tests that model one durable blob authority across several store handles use the new test_blob_store_at, which keeps the blob table in a SQLite file beside the store directory, plus test_database_with_blobs to share it explicitly. Co-Authored-By: Claude Fable 5 --- .../fabro-store/src/test_support/mod.rs | 116 +++++++++++++----- .../fabro-workflow/src/test_support.rs | 9 +- .../fabro-workflow/tests/it/integration.rs | 13 +- 3 files changed, 100 insertions(+), 38 deletions(-) diff --git a/lib/components/fabro-store/src/test_support/mod.rs b/lib/components/fabro-store/src/test_support/mod.rs index 2e1dd9623..a8a3ee9b7 100644 --- a/lib/components/fabro-store/src/test_support/mod.rs +++ b/lib/components/fabro-store/src/test_support/mod.rs @@ -1,7 +1,5 @@ -#[cfg(test)] -use std::path::Path; -use std::path::PathBuf; -use std::sync::{Arc, OnceLock}; +use std::path::{Path, PathBuf}; +use std::sync::Arc; use std::time::Duration; use fabro_types::{BlobHash, RunId}; @@ -13,47 +11,85 @@ use crate::{AuthSessionStore, RunSummaryStore}; use crate::keys::SlateKey; use crate::{BlobStore, Database, Result}; -/// Returns the process-wide SQLite blob authority used by Slate-backed tests. +/// Returns an isolated SQLite blob authority backed by its own in-memory +/// database. /// -/// Production has one blob authority shared by every run handle. Keeping the -/// same shape in test processes also lets helpers reopen the Slate run store -/// without silently switching to an empty blob database. The CAS key is the -/// content hash, so sharing rows across otherwise isolated tests is safe. +/// Every call creates a fresh blob table, so tests never observe rows written +/// by other tests in the same process. Reopen-style tests that model one +/// process-wide blob authority across several store handles should call this +/// once and share the result through [`test_database_with_blobs`]. /// /// The pool connects lazily so synchronous fixture builders can remain /// synchronous. Its single connection installs the production blob schema on /// first use. #[must_use] pub fn test_blob_store() -> Arc { - static STORE: OnceLock> = OnceLock::new(); + let options = SqliteConnectOptions::new() + .filename(":memory:") + .foreign_keys(true); + let pool = SqlitePoolOptions::new() + .max_connections(1) + // A single in-memory test connection never needs reaping. Disabling + // both timers also keeps this lazy fixture constructible from sync + // tests, where SQLx has no Tokio runtime for maintenance tasks. + .max_lifetime(None) + .idle_timeout(None) + .after_connect(|connection, _metadata| { + Box::pin(async move { + sqlx::query(fabro_db::BLOBS_MIGRATION_SQL) + .execute(&mut *connection) + .await?; + Ok(()) + }) + }) + .connect_lazy_with(options); + Arc::new(BlobStore::new(pool)) +} - Arc::clone(STORE.get_or_init(|| { - let options = SqliteConnectOptions::new() - .filename(":memory:") - .foreign_keys(true); - let pool = SqlitePoolOptions::new() - .max_connections(1) - // A single in-memory test connection never needs reaping. - // Disabling both timers also keeps this lazy fixture constructible - // from sync tests, where SQLx has no Tokio runtime for maintenance - // tasks. - .max_lifetime(None) - .idle_timeout(None) - .after_connect(|connection, _metadata| { - Box::pin(async move { +/// Returns the SQLite file backing [`test_blob_store_at`] for `store_dir`. +#[must_use] +pub fn test_blob_store_path(store_dir: &Path) -> PathBuf { + fabro_db::append_to_path(store_dir, "-blobs.sqlite3") +} + +/// Returns a durable SQLite blob authority stored beside `store_dir`. +/// +/// Handles created for the same directory share one blob database file, so +/// reopen-style tests observe blobs across store handles the way production +/// handles share the process-wide blob authority. Tests that reuse a +/// directory must delete [`test_blob_store_path`] (and its `-wal`/`-shm` +/// siblings) when they reset the directory itself. +#[must_use] +pub fn test_blob_store_at(store_dir: &Path) -> Arc { + let options = SqliteConnectOptions::new() + .filename(test_blob_store_path(store_dir)) + .create_if_missing(true) + .foreign_keys(true); + let pool = SqlitePoolOptions::new() + .max_connections(1) + .max_lifetime(None) + .idle_timeout(None) + .after_connect(|connection, _metadata| { + Box::pin(async move { + let installed: bool = sqlx::query_scalar( + "SELECT EXISTS(SELECT 1 FROM sqlite_master \ + WHERE type = 'table' AND name = 'blobs')", + ) + .fetch_one(&mut *connection) + .await?; + if !installed { sqlx::query(fabro_db::BLOBS_MIGRATION_SQL) .execute(&mut *connection) .await?; - Ok(()) - }) + } + Ok(()) }) - .connect_lazy_with(options); - Arc::new(BlobStore::new(pool)) - })) + }) + .connect_lazy_with(options); + Arc::new(BlobStore::new(pool)) } -/// Builds a Slate-backed run database using the process-wide test blob -/// authority and its signed SQLite blob table. +/// Builds a Slate-backed run database with its own isolated blob authority. #[must_use] pub fn test_database( object_store: Arc, @@ -61,7 +97,7 @@ pub fn test_database( flush_interval: Duration, cache_path: Option, ) -> Database { - Database::new( + test_database_with_blobs( object_store, base_prefix, flush_interval, @@ -70,6 +106,22 @@ pub fn test_database( ) } +/// Builds a Slate-backed run database sharing an explicit blob authority. +/// +/// Use this for reopen-style tests where two store handles must observe the +/// same signed SQLite blob table, mirroring the one blob authority a +/// production process shares across every run handle. +#[must_use] +pub fn test_database_with_blobs( + object_store: Arc, + base_prefix: impl Into, + flush_interval: Duration, + cache_path: Option, + blobs: Arc, +) -> Database { + Database::new(object_store, base_prefix, flush_interval, cache_path, blobs) +} + /// Seeds one canonical row in the legacy SlateDB blob keyspace. pub async fn put_legacy_blob(database: &Database, bytes: &[u8]) -> Result { let hash = BlobHash::new(bytes); diff --git a/lib/components/fabro-workflow/src/test_support.rs b/lib/components/fabro-workflow/src/test_support.rs index 4be59d052..08f657ff1 100644 --- a/lib/components/fabro-workflow/src/test_support.rs +++ b/lib/components/fabro-workflow/src/test_support.rs @@ -174,8 +174,14 @@ async fn initialized( std::fs::create_dir_all(&run_options.run_dir).expect("failed to create run dir"); let store_dir = test_store_dir(&run_options.run_dir); let _ = std::fs::remove_dir_all(&store_dir); + let blob_store_path = store_test_support::test_blob_store_path(&store_dir); + for suffix in ["", "-wal", "-shm"] { + let mut sibling = blob_store_path.clone().into_os_string(); + sibling.push(suffix); + let _ = std::fs::remove_file(sibling); + } std::fs::create_dir_all(&store_dir).expect("failed to create local test run store dir"); - let store = Arc::new(store_test_support::test_database( + let store = Arc::new(store_test_support::test_database_with_blobs( Arc::new( LocalFileSystem::new_with_prefix(&store_dir) .expect("failed to create local test run store"), @@ -183,6 +189,7 @@ async fn initialized( "", Duration::from_millis(1), None, + store_test_support::test_blob_store_at(&store_dir), )); let inner_store = store .create_run(&run_options.run_id) diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index 45bb4c6b3..b72b3cdf8 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -117,12 +117,13 @@ fn load_run_checkpoint(run_dir: &Path) -> Result