From b56aee570c2c13bc9dc7c84ddf42a0e4cd725670 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sun, 23 Aug 2026 13:04:40 -0400 Subject: [PATCH] Pass the SQLite pool directly to the legacy blob import and verification import_legacy_blobs_into and verify_legacy_blobs_in took a &BlobStore and extracted its pool through sqlite_pool_for_legacy_import, an Option that was statically always Some in production (the None arm existed only for the test-only Slate backend). That accessor forced a clippy unnecessary_wraps suppression and two WrongTargetBackend error variants no production caller could ever hit, and the activation path round-tripped a pool it already owned through a BlobStore it had just built. Both functions now take &SqlitePool, deleting the accessor, the suppression, both unreachable variants, and their rejection test. Co-Authored-By: Claude Fable 5 --- lib/apps/fabro-server/src/blob_activation.rs | 4 +- lib/components/fabro-store/src/blob_store.rs | 15 ---- .../fabro-store/src/legacy_blob_import.rs | 88 ++++++------------- 3 files changed, 30 insertions(+), 77 deletions(-) diff --git a/lib/apps/fabro-server/src/blob_activation.rs b/lib/apps/fabro-server/src/blob_activation.rs index e6e4139c9..beda1df7f 100644 --- a/lib/apps/fabro-server/src/blob_activation.rs +++ b/lib/apps/fabro-server/src/blob_activation.rs @@ -181,11 +181,11 @@ pub(crate) async fn activate_blob_storage( }; let import = store - .import_legacy_blobs_into(&blob_store) + .import_legacy_blobs_into(database.pool()) .await .map_err(|source| BlobActivationError::Import(Box::new(source)))?; let verification = store - .verify_legacy_blobs_in(&blob_store) + .verify_legacy_blobs_in(database.pool()) .await .map_err(|source| BlobActivationError::Verification(Box::new(source)))?; validate_live_integrity(database.pool()).await?; diff --git a/lib/components/fabro-store/src/blob_store.rs b/lib/components/fabro-store/src/blob_store.rs index e1c7e60dc..e5d0dc128 100644 --- a/lib/components/fabro-store/src/blob_store.rs +++ b/lib/components/fabro-store/src/blob_store.rs @@ -82,21 +82,6 @@ impl BlobStore { } } - #[cfg_attr( - not(test), - expect( - clippy::unnecessary_wraps, - reason = "the temporary Slate test backend has no SQLite pool" - ) - )] - pub(crate) fn sqlite_pool_for_legacy_import(&self) -> Option<&SqlitePool> { - match &self.backend { - #[cfg(test)] - BlobBackend::Slate(_) => None, - BlobBackend::Sqlite(pool) => Some(pool), - } - } - pub async fn write(&self, bytes: &[u8]) -> Result { match &self.backend { #[cfg(test)] diff --git a/lib/components/fabro-store/src/legacy_blob_import.rs b/lib/components/fabro-store/src/legacy_blob_import.rs index 20f5b87e0..d07692b0e 100644 --- a/lib/components/fabro-store/src/legacy_blob_import.rs +++ b/lib/components/fabro-store/src/legacy_blob_import.rs @@ -10,13 +10,13 @@ use bytes::Bytes; use fabro_types::BlobHash; use futures::TryStreamExt as _; use sqlx::pool::PoolConnection; -use sqlx::{Acquire as _, Sqlite}; +use sqlx::{Acquire as _, Sqlite, SqlitePool}; #[cfg(test)] use tokio::sync::Barrier; use tracing::debug; +use crate::Database; use crate::keys::SlateKey; -use crate::{BlobStore, Database}; const MAX_BATCH_ROWS: usize = 100; const MAX_BATCH_BYTES: u64 = 1024 * 1024; @@ -158,8 +158,6 @@ impl StdError for LegacyBlobVerificationError { #[derive(strum::IntoStaticStr, thiserror::Error)] #[strum(serialize_all = "snake_case")] enum LegacyBlobVerificationFailure { - #[error("the legacy blob verification target is not backed by SQLite")] - WrongTargetBackend, #[error("opening the legacy blob source")] OpenSource(#[source] crate::Error), #[error("opening the legacy blob scan")] @@ -281,9 +279,6 @@ impl StdError for LegacyBlobImportError { #[derive(strum::IntoStaticStr, thiserror::Error)] #[strum(serialize_all = "snake_case")] enum LegacyBlobImportFailure { - #[error("the legacy blob import target is not backed by SQLite")] - WrongTargetBackend, - #[error("opening the legacy blob source")] OpenSource(#[source] crate::Error), @@ -576,10 +571,10 @@ impl Database { /// blob row independently. pub async fn verify_legacy_blobs_in( &self, - target: &BlobStore, + pool: &SqlitePool, ) -> std::result::Result { let mut report = LegacyBlobVerificationReport::default(); - let result = self.run_legacy_blob_verification(target, &mut report).await; + let result = self.run_legacy_blob_verification(pool, &mut report).await; match result { Ok(()) => Ok(report), Err(failure) => Err(LegacyBlobVerificationError { report, failure }), @@ -588,12 +583,9 @@ impl Database { async fn run_legacy_blob_verification( &self, - target: &BlobStore, + pool: &SqlitePool, report: &mut LegacyBlobVerificationReport, ) -> Result<(), LegacyBlobVerificationFailure> { - let pool = target - .sqlite_pool_for_legacy_import() - .ok_or(LegacyBlobVerificationFailure::WrongTargetBackend)?; let source = self .open_db() .await @@ -679,20 +671,20 @@ impl Database { /// cleanup fails. pub async fn import_legacy_blobs_into( &self, - target: &BlobStore, + pool: &SqlitePool, ) -> std::result::Result { - self.import_legacy_blobs_with_controls(target, &ImportControls::default()) + self.import_legacy_blobs_with_controls(pool, &ImportControls::default()) .await } async fn import_legacy_blobs_with_controls( &self, - target: &BlobStore, + pool: &SqlitePool, controls: &ImportControls, ) -> std::result::Result { let mut report = LegacyBlobImportReport::default(); let result = self - .run_legacy_blob_import(target, controls, &mut report) + .run_legacy_blob_import(pool, controls, &mut report) .await; match result { @@ -709,13 +701,10 @@ impl Database { async fn run_legacy_blob_import( &self, - target: &BlobStore, + pool: &SqlitePool, controls: &ImportControls, report: &mut LegacyBlobImportReport, ) -> Result<(), LegacyBlobImportFailure> { - let pool = target - .sqlite_pool_for_legacy_import() - .ok_or(LegacyBlobImportFailure::WrongTargetBackend)?; let mut connection = pool .acquire() .await @@ -1221,7 +1210,7 @@ mod tests { } async fn import(&self) -> TestResult { - Ok(self.source.import_legacy_blobs_into(&self.target).await?) + Ok(self.source.import_legacy_blobs_into(&self.sqlite).await?) } } @@ -1338,7 +1327,7 @@ mod tests { let report = context .source - .verify_legacy_blobs_in(&context.target) + .verify_legacy_blobs_in(&context.sqlite) .await?; assert_eq!(report.source_rows, 1); @@ -1360,7 +1349,7 @@ mod tests { let error = context .source - .verify_legacy_blobs_in(&context.target) + .verify_legacy_blobs_in(&context.sqlite) .await .expect_err("a missing destination row must fail verification"); @@ -1390,7 +1379,7 @@ mod tests { drop(connection); let error = malformed_hash .source - .verify_legacy_blobs_in(&malformed_hash.target) + .verify_legacy_blobs_in(&malformed_hash.sqlite) .await .expect_err("malformed SQLite hashes must fail verification"); assert_eq!(error.report().invalid_target_rows, 1); @@ -1401,7 +1390,7 @@ mod tests { .await?; let error = mismatched_bytes .source - .verify_legacy_blobs_in(&mismatched_bytes.target) + .verify_legacy_blobs_in(&mismatched_bytes.sqlite) .await .expect_err("SQLite bytes must match their hash"); assert_eq!(error.report().invalid_target_rows, 1); @@ -1433,7 +1422,7 @@ mod tests { let error = context .source - .import_legacy_blobs_into(&context.target) + .import_legacy_blobs_into(&context.sqlite) .await .expect_err("noncanonical key should fail import"); @@ -1463,7 +1452,7 @@ mod tests { let error = context .source - .import_legacy_blobs_into(&context.target) + .import_legacy_blobs_into(&context.sqlite) .await .expect_err("digest mismatch should fail import"); @@ -1488,7 +1477,7 @@ mod tests { let error = context .source - .import_legacy_blobs_into(&context.target) + .import_legacy_blobs_into(&context.sqlite) .await .expect_err("invalid row should stop the import"); @@ -1529,7 +1518,7 @@ mod tests { let error = context .source - .import_legacy_blobs_into(&context.target) + .import_legacy_blobs_into(&context.sqlite) .await .expect_err("differing destination row should fail import"); @@ -1557,7 +1546,7 @@ mod tests { let error = context .source - .import_legacy_blobs_into(&context.target) + .import_legacy_blobs_into(&context.sqlite) .await .expect_err("last-row conflict should interrupt import"); @@ -1673,7 +1662,7 @@ mod tests { let error = context .source - .import_legacy_blobs_with_controls(&context.target, &controls) + .import_legacy_blobs_with_controls(&context.sqlite, &controls) .await .expect_err("injected passive checkpoint should fail import"); @@ -1703,7 +1692,7 @@ mod tests { let error = context .source - .import_legacy_blobs_with_controls(&context.target, &controls) + .import_legacy_blobs_with_controls(&context.sqlite, &controls) .await .expect_err("injected source transport failure should stop import"); @@ -1737,7 +1726,7 @@ mod tests { let error = context .source - .import_legacy_blobs_with_controls(&context.target, &controls) + .import_legacy_blobs_with_controls(&context.sqlite, &controls) .await .expect_err("both injected failures should fail import"); @@ -1773,7 +1762,7 @@ mod tests { failure.put_raw(invalid_key, b"invalid").await?; failure .source - .import_legacy_blobs_into(&failure.target) + .import_legacy_blobs_into(&failure.sqlite) .await .expect_err("invalid source should fail import"); assert_eq!(failure.automatic_checkpoint().await?, 41); @@ -1814,11 +1803,11 @@ mod tests { }; let task = tokio::spawn({ let source = source.clone(); - let target = Arc::clone(&target); + let pool = pool.clone(); async move { let mut report = LegacyBlobImportReport::default(); source - .run_legacy_blob_import(&target, &controls, &mut report) + .run_legacy_blob_import(&pool, &controls, &mut report) .await } }); @@ -1848,7 +1837,7 @@ mod tests { let error = context .source - .import_legacy_blobs_with_controls(&context.target, &controls) + .import_legacy_blobs_with_controls(&context.sqlite, &controls) .await .expect_err("injected restoration failure should fail import"); @@ -1864,27 +1853,6 @@ mod tests { Ok(()) } - #[tokio::test] - async fn slate_backed_target_is_rejected_before_scanning() -> TestResult<()> { - let context = TestContext::new().await?; - context.put_blob(b"never-scanned").await?; - let slate_target = BlobStore::from_slate(Arc::new(context.source_db.clone())); - - let error = context - .source - .import_legacy_blobs_into(&slate_target) - .await - .expect_err("Slate target should be rejected"); - - assert_eq!(*error.report(), LegacyBlobImportReport::default()); - assert!(matches!( - &error.failure, - LegacyBlobImportFailure::WrongTargetBackend - )); - assert_eq!(context.destination_rows().await?, 0); - Ok(()) - } - #[tokio::test] async fn logs_and_error_rendering_expose_counts_but_not_row_data() -> TestResult<()> { let context = TestContext::new().await?; @@ -1897,7 +1865,7 @@ mod tests { let error = context .source - .import_legacy_blobs_into(&context.target) + .import_legacy_blobs_into(&context.sqlite) .with_subscriber(capture.clone()) .await .expect_err("invalid key should fail import");