ads-client: simplify the http cache and ads store in-memory fallback

components/ads-client/integration-tests/tests/ads_store.rs

@@ -30,15 +30,15 @@ fn test_bad_path_opens_memory_db() {
         .build()
         .expect("Should be able to build a bad path");
     assert!(
-        !good_path.is_memory(),
+        !good_path.is_memory().unwrap(),
         "A valid path should result in a file-based sqlite database"
     );
 
     let bad_path = AdsStoreBuilder::new("this/is/a/test/bad/path")
         .build()
         .expect("Should be able to build a bad path");
     assert!(
-        bad_path.is_memory(),
+        bad_path.is_memory().unwrap(),
         "Invalid path should result in a memory-based sqlite database"
     );
 

components/ads-client/integration-tests/tests/http_cache.rs

@@ -126,15 +126,15 @@ fn test_bad_path_opens_memory_db() {
         .build()
         .expect("Should be able to build a bad path");
     assert!(
-        !good_path.is_memory(),
+        !good_path.is_memory().unwrap(),
         "A valid path should result in a file-based sqlite database"
     );
 
     let bad_path = HttpCache::builder("this/is/a/test/bad/path")
         .build()
         .expect("Should be able to build a bad path");
     assert!(
-        bad_path.is_memory(),
+        bad_path.is_memory().unwrap(),
         "Invalid path should result in a memory-based sqlite database"
     );
 
@@ -148,7 +148,7 @@ fn test_bad_path_opens_memory_db() {
         .build()
         .expect("Should be able to build a memory db");
     assert!(
-        no_path.is_memory(),
+        no_path.is_memory().unwrap(),
         "`builder_memory` should result in a memory-based sqlite database"
     )
 }

components/ads-client/src/ads_store.rs

@@ -12,7 +12,6 @@ use std::path::Path;
 
 pub struct AdsStore {
     holder: AdsStoreHolder,
-    is_memory: bool,
     #[allow(dead_code)]
     max_size: ByteSize,
 }
@@ -32,8 +31,8 @@ impl AdsStore {
         Ok(())
     }
 
-    pub fn is_memory(&self) -> bool {
-        self.is_memory
+    pub fn is_memory(&self) -> Result<bool, open_database::Error> {
+        self.holder.is_memory()
     }
 
     pub fn shutdown_db(&self) {
@@ -45,17 +44,18 @@ impl AdsStore {
 mod tests {
     use super::*;
     use crate::test_utils::get_example_happy_image_ads;
+    use crate::IN_MEMORY_DB_PATH;
 
     #[test]
     fn test_ads_store_creation() {
         // Test that AdsStore can be created successfully with test config
-        let store: Result<AdsStore, _> = AdsStore::builder("test_store.db").build();
+        let store: Result<AdsStore, _> = AdsStore::builder(IN_MEMORY_DB_PATH).build();
         assert!(store.is_ok());
     }
 
     #[test]
     fn test_clear_store() {
-        let store: AdsStore = AdsStore::builder("test_clear.db").build().unwrap();
+        let store: AdsStore = AdsStore::builder(IN_MEMORY_DB_PATH).build().unwrap();
 
         let (placement_id, ad) = get_example_happy_image_ads("mock_billboard_1");
         store.holder.store_ad(&placement_id, ad.clone()).unwrap();
@@ -74,7 +74,7 @@ mod tests {
 
     #[test]
     fn test_invalidate_by_id() {
-        let store: AdsStore = AdsStore::builder("test_invalidate.db").build().unwrap();
+        let store: AdsStore = AdsStore::builder(IN_MEMORY_DB_PATH).build().unwrap();
 
         let (placement_1, ad_1) = get_example_happy_image_ads("mock_billboard_1");
         let (placement_2, ad_2) = get_example_happy_image_ads("mock_billboard_2");

components/ads-client/src/ads_store/builder.rs

@@ -7,6 +7,7 @@ use crate::ads_store::store::AdsStoreHolder;
 use crate::ads_store::AdsStore;
 use crate::bytesize::ByteSize;
 use crate::telemetry;
+use crate::IN_MEMORY_DB_PATH;
 use rusqlite::OpenFlags;
 use sql_support::{open_database, LazyDb};
 use std::path::{Path, PathBuf};
@@ -15,8 +16,6 @@ const DEFAULT_MAX_SIZE: ByteSize = ByteSize::mib(10);
 const MIN_STORE_SIZE: ByteSize = ByteSize::kib(1);
 const MAX_STORE_SIZE: ByteSize = ByteSize::mib(100);
 
-pub const IN_MEMORY_DB_PATH: &'static str = ":memory:";
-
 #[derive(Debug, thiserror::Error)]
 pub enum AdsStoreBuilderError {
     #[error("Database error: {0}")]
@@ -35,72 +34,35 @@ pub enum AdsStoreBuilderError {
 
 pub struct AdsStoreBuilder {
     db_path: PathBuf,
-    // Flag for whether this db is built in-memory
-    // This is only set by an error in `open_connection`, not manually.
-    is_memory: bool,
     max_size: Option<ByteSize>,
 }
 
 impl AdsStoreBuilder {
     pub fn new(db_path: impl Into<PathBuf>) -> Self {
         Self {
             db_path: db_path.into(),
-            is_memory: false,
             max_size: None,
         }
     }
 
-    pub fn build(&mut self) -> Result<AdsStore, AdsStoreBuilderError> {
+    pub fn build(&self) -> Result<AdsStore, AdsStoreBuilderError> {
         self.validate()?;
 
-        let db = self.open_connection()?;
+        // Fall back to a memory db if the file db fails to open.
+        let db = open_connection(&self.db_path)
+            .map_err(AdsStoreBuilderError::from)
+            .inspect_err(telemetry::record_ads_store_error)
+            .or_else(|_| open_connection(Path::new(IN_MEMORY_DB_PATH)))?;
         let holder = AdsStoreHolder::new(db);
         let max_size = self.max_size.unwrap_or(DEFAULT_MAX_SIZE);
-        Ok(AdsStore {
-            holder,
-            is_memory: self.is_memory,
-            max_size,
-        })
+        Ok(AdsStore { holder, max_size })
     }
 
     pub fn max_size(mut self, max_size: ByteSize) -> Self {
         self.max_size = Some(max_size);
         self
     }
 
-    fn open_connection(
-        &mut self,
-    ) -> Result<LazyDb<AdsStoreConnectionInitializer>, AdsStoreBuilderError> {
-        if !cfg!(test) {
-            let db = LazyDb::new(
-                &self.db_path,
-                OpenFlags::default(),
-                AdsStoreConnectionInitializer {},
-            );
-
-            // Attempt to open initial connection, resolving an error if needed.
-            let conn = db.lock();
-            if let Some(e) = conn.err() {
-                telemetry::record_ads_store_error(&AdsStoreBuilderError::from(e));
-            } else {
-                return Ok(db);
-            }
-        }
-
-        // If we cannot instantiate a filesystem db, or cfg!(test) == true, we fall back to a memory db.
-        let memory_db = LazyDb::new(
-            &Path::new(IN_MEMORY_DB_PATH),
-            OpenFlags::default(),
-            AdsStoreConnectionInitializer {},
-        );
-        self.is_memory = true;
-
-        // Attempt to open initial connection, resolving an error if needed.
-        let _ = memory_db.lock()?;
-
-        Ok(memory_db)
-    }
-
     fn validate(&self) -> Result<(), AdsStoreBuilderError> {
         if self.db_path.to_string_lossy().trim().is_empty() {
             return Err(AdsStoreBuilderError::EmptyDbPath);
@@ -120,6 +82,15 @@ impl AdsStoreBuilder {
     }
 }
 
+// Opens the db at `path`, eagerly opening the initial connection so failures surface here.
+fn open_connection(
+    path: &Path,
+) -> Result<LazyDb<AdsStoreConnectionInitializer>, open_database::Error> {
+    let db = LazyDb::new(path, OpenFlags::default(), AdsStoreConnectionInitializer {});
+    let _ = db.lock()?;
+    Ok(db)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -130,17 +101,27 @@ mod tests {
 
     #[test]
     fn test_store_builder_with_defaults() {
-        let mut builder = make_test_builder("test.db");
-        assert_eq!(builder.db_path, PathBuf::from("test.db"));
+        let builder = make_test_builder(IN_MEMORY_DB_PATH);
+        assert_eq!(builder.db_path, PathBuf::from(IN_MEMORY_DB_PATH));
         assert_eq!(builder.max_size, None);
         assert!(builder.build().is_ok());
     }
 
+    #[test]
+    fn test_unopenable_path_falls_back_to_memory_db() {
+        // The fallback records telemetry, so don't race the telemetry tests.
+        let _lock = crate::telemetry::test_lock();
+        let store = make_test_builder("this/is/a/test/bad/path")
+            .build()
+            .unwrap();
+        assert!(store.is_memory().unwrap());
+    }
+
     #[test]
     fn test_cache_builder_valid_custom() {
-        let mut builder = make_test_builder("custom.db").max_size(ByteSize::b(1024));
+        let builder = make_test_builder(IN_MEMORY_DB_PATH).max_size(ByteSize::b(1024));
 
-        assert_eq!(builder.db_path, PathBuf::from("custom.db"));
+        assert_eq!(builder.db_path, PathBuf::from(IN_MEMORY_DB_PATH));
         assert_eq!(builder.max_size, Some(ByteSize::b(1024)));
         assert!(builder.build().is_ok());
     }
@@ -153,7 +134,7 @@ mod tests {
 
     #[test]
     fn test_validation_max_size_too_small() {
-        let result = make_test_builder("test.db")
+        let result = make_test_builder(IN_MEMORY_DB_PATH)
             .max_size(ByteSize::b(512))
             .build();
         assert!(matches!(
@@ -168,7 +149,7 @@ mod tests {
 
     #[test]
     fn test_validation_max_size_too_large() {
-        let result = make_test_builder("test.db")
+        let result = make_test_builder(IN_MEMORY_DB_PATH)
             .max_size(ByteSize::b(2 * 1024 * 1024 * 1024))
             .build();
         assert!(matches!(
@@ -183,10 +164,10 @@ mod tests {
 
     #[test]
     fn test_validation_max_size_boundaries() {
-        let mut builder_min = make_test_builder("test.db").max_size(MIN_STORE_SIZE);
+        let builder_min = make_test_builder(IN_MEMORY_DB_PATH).max_size(MIN_STORE_SIZE);
         assert!(builder_min.build().is_ok());
 
-        let mut builder_max = make_test_builder("test.db").max_size(MAX_STORE_SIZE);
+        let builder_max = make_test_builder(IN_MEMORY_DB_PATH).max_size(MAX_STORE_SIZE);
         assert!(builder_max.build().is_ok());
     }
 }

components/ads-client/src/ads_store/store.rs

@@ -62,6 +62,13 @@ impl AdsStoreHolder {
         self.db.close(true);
     }
 
+    /// Returns whether the db is in memory rather than backed by a file.
+    pub fn is_memory(&self) -> Result<bool, open_database::Error> {
+        let (conn, _) = self.db.lock()?;
+        // SQLite reports an empty path for in-memory dbs.
+        Ok(conn.path() == Some(""))
+    }
+
     /// Returns total size of the cache in bytes.
     pub fn current_total_size_bytes(&self) -> Result<ByteSize, open_database::Error> {
         let (conn, _) = self.db.lock()?;
@@ -194,7 +201,7 @@ mod tests {
     fn create_test_store() -> AdsStoreHolder {
         let initializer = AdsStoreConnectionInitializer {};
         let db = LazyDb::new(
-            Path::new(crate::ads_store::builder::IN_MEMORY_DB_PATH),
+            Path::new(crate::IN_MEMORY_DB_PATH),
             OpenFlags::default(),
             initializer,
         );

components/ads-client/src/client.rs

@@ -473,7 +473,7 @@ mod tests {
         let inner = Arc::new(AdsClientInner {
             #[cfg(feature = "stateful")]
             ads_store: Arc::new(Some(
-                AdsStoreBuilder::new("test_store.db")
+                AdsStoreBuilder::new(crate::IN_MEMORY_DB_PATH)
                     .build()
                     .expect("Simplest AdsStoreBuilder should be constructable"),
             )),
@@ -687,9 +687,7 @@ mod tests {
     #[ignore = "Cache invalidation temporarily disabled - will be re-enabled behind Nimbus experiment"]
     fn test_record_click_invalidates_cache() {
         viaduct_dev::init_backend_dev();
-        let cache = HttpCache::builder("test_record_click_invalidates_cache")
-            .build()
-            .unwrap();
+        let cache = HttpCache::builder_memory().build().unwrap();
         let mars_client = MARSClient::new(Environment::Test, Some(cache));
         let ads_client = new_with_mars_client(mars_client);
 

components/ads-client/src/ffi.rs

@@ -550,7 +550,7 @@ mod tests {
         let builder = Arc::new(MozAdsClientBuilder::new())
             .telemetry(Box::new(NoopMozAdsTelemetry))
             .cache_config(MozAdsCacheConfig {
-                db_path: "test_shutdown_is_idempotent".to_string(),
+                db_path: crate::IN_MEMORY_DB_PATH.to_string(),
                 default_cache_ttl_seconds: None,
                 max_size_mib: None,
             });
@@ -573,7 +573,7 @@ mod tests {
         let builder = Arc::new(MozAdsClientBuilder::new())
             .telemetry(Box::new(NoopMozAdsTelemetry))
             .cache_config(MozAdsCacheConfig {
-                db_path: "test_shutdown_is_idempotent".to_string(),
+                db_path: crate::IN_MEMORY_DB_PATH.to_string(),
                 default_cache_ttl_seconds: None,
                 max_size_mib: None,
             });

components/ads-client/src/http_cache.rs

@@ -47,7 +47,6 @@ pub struct HttpCache {
     default_ttl: Duration,
     max_size: ByteSize,
     store: HttpCacheStore,
-    is_memory: bool,
 }
 
 impl HttpCache {
@@ -59,8 +58,8 @@ impl HttpCache {
         HttpCacheBuilder::new(None::<String>)
     }
 
-    pub fn is_memory(&self) -> bool {
-        self.is_memory
+    pub fn is_memory(&self) -> Result<bool, open_database::Error> {
+        self.store.is_memory()
     }
 
     pub fn clear(&self) -> Result<(), open_database::Error> {
@@ -165,7 +164,7 @@ mod tests {
 
     fn make_cache() -> HttpCache {
         // Our store opens an in-memory cache for tests. So the name is irrelevant.
-        HttpCache::builder("ignored_in_tests.db")
+        HttpCache::builder_memory()
             .default_ttl(Duration::from_secs(60))
             .max_size(ByteSize::mib(1))
             .build()
@@ -174,7 +173,7 @@ mod tests {
 
     fn make_cache_with_ttl(secs: u64) -> HttpCache {
         // In tests our store uses an in-memory DB; filename is irrelevant.
-        HttpCache::builder("ignored_in_tests.db")
+        HttpCache::builder_memory()
             .default_ttl(Duration::from_secs(secs))
             .max_size(ByteSize::mib(4))
             .build_for_time_dependent_tests()
@@ -184,11 +183,11 @@ mod tests {
     #[test]
     fn test_http_cache_creation() {
         // Test that HttpCache can be created successfully with test config
-        let cache: Result<HttpCache, _> = HttpCache::builder("test_cache.db").build();
+        let cache: Result<HttpCache, _> = HttpCache::builder_memory().build();
         assert!(cache.is_ok());
 
         // Test with custom config
-        let cache_with_config: Result<HttpCache, _> = HttpCache::builder("custom_test.db")
+        let cache_with_config: Result<HttpCache, _> = HttpCache::builder_memory()
             .max_size(ByteSize::mib(1))
             .default_ttl(Duration::from_secs(60))
             .build();
@@ -197,7 +196,7 @@ mod tests {
 
     #[test]
     fn test_clear_cache() {
-        let cache: HttpCache = HttpCache::builder("test_clear.db").build().unwrap();
+        let cache: HttpCache = HttpCache::builder_memory().build().unwrap();
 
         // Create a test request and response
         let hash = RequestHash::new(&("Get", "https://example.com/test"));
@@ -501,7 +500,7 @@ mod tests {
 
     #[test]
     fn test_invalidate_by_hash() {
-        let cache: HttpCache = HttpCache::builder("test_invalidate.db").build().unwrap();
+        let cache: HttpCache = HttpCache::builder_memory().build().unwrap();
 
         let hash1 = RequestHash::new(&("Post", "https://example.com/api1"));
         let hash2 = RequestHash::new(&("Post", "https://example.com/api2"));