Repository navigation
ads-client: simplify the http cache and ads store in-memory fallback - #7713
Merged
Merged
Conversation
Move opening a LazyDb (and eagerly locking it) into an `open_db` helper, so `HttpCacheBuilder::open_connection` only holds the fallback policy: try the filesystem db, record the failure, otherwise use a memory db.
Split HttpCacheBuilder's db setup into three pieces: - `open_connection(path)` only opens a db, with no fallback, test or telemetry logic. - `file_db_path()` decides whether there is a filesystem db to try. This holds the cases where a memory db is intended rather than a fallback: no path, unit tests, or a builder that already fell back. - `open_with_memory_fallback()` holds the fallback policy, shared by `build` and `build_for_time_dependent_tests`. Add a test pinning that unit tests always get a memory db.
Follow-up to mozilla#7701: - Do the memory fallback in a single `build_with_store`, shared by `build` and `build_for_time_dependent_tests` instead of duplicating their bodies. - Return `is_memory` from the build instead of storing it on the builder, so `build` takes `&self` again. This also drops the quirk where a second `build()` after a fallback skipped the filesystem db. - Keep the cases where a memory db is intended (no path, unit tests) in `open_file_db`, separate from `open_connection`, which only opens. - `HttpCacheBuilder::new` takes `Option<PathBuf>`, so `builder_memory` no longer needs `None::<String>`. - In `AdsClientInner::new`, say that an invalid cache config also falls back to the in-memory cache, and record build errors the same way in both branches. - Fix `test_bad_path_opens_memory_db` clearing `good_path.db` instead of the `good_path_httpstore.db` it creates, and its copy-pasted message.
This reverts commit 429b511.
Unit tests now ask for a memory db explicitly (`HttpCache::builder_memory()`, or ":memory:" where only a path is accepted) instead of production code silently ignoring the path under `cfg!(test)`. That leaves the builder with a single, linear fallback: open the file db if there is a path, otherwise or on failure open a memory db. `open_db` returns whether the db is in memory instead of storing it on the builder, so `build` takes `&self` again. The fallback is now unit-testable, so the test that pinned the `cfg!(test)` behaviour is replaced by one that opens an unopenable path.
…lder `is_memory()` on HttpCache and AdsStore was only read by tests. Remove it from both, along with the builder state that tracked it. The integration tests now check the real outcome: a valid path creates the db file, and an unopenable path still builds (through the memory fallback). The http cache test now clears the file it actually creates, so that check can't pass on a file left over from a previous run. AdsStoreBuilder gets the same treatment as HttpCacheBuilder: no `cfg!(test)` switch (tests use ":memory:"), `build` takes `&self`, and the fallback is a plain `or_else` in `build` on top of an `open_connection` that only opens. Add a unit test for the fallback.
`build()` succeeding on an unopenable path doesn't prove the fallback ran: without the eager open, a broken file-backed db would also build. Use the db (`clear()`) and check nothing was created at the path instead, which only holds for a working memory db.
… flag `HttpCache::is_memory()` and `AdsStore::is_memory()` now come from the open connection (SQLite reports an empty path for in-memory dbs) instead of a flag threaded through the builders. The builders stay stateless, and the integration tests go back to their original `is_memory()` assertions. Asking the connection is also stricter than the old flag: it tells where the db really lives, so a fallback after the file was created is caught, and a db opened from a ":memory:" path is reported as in memory.
thesuzerain
reviewed
Oct 31, 2026
| .telemetry(Box::new(NoopMozAdsTelemetry)) | ||
| .cache_config(MozAdsCacheConfig { | ||
| db_path: "test_shutdown_is_idempotent".to_string(), | ||
| db_path: ":memory:".to_string(), |
Collaborator
There was a problem hiding this comment.
I think we should reuse the constant we have for this where possible.
thesuzerain
approved these changes
Oct 31, 2026
The http cache and ads store builders each defined their own ":memory:" constant, and tests spelled the literal out. Define it once at the crate root and use it everywhere instead.
Almaju
changed the title
Almaju
enabled auto-merge
November 2, 2026 16:39
Almaju
added a commit
to Almaju/application-services
that referenced
this pull request
Nov 3, 2026
Conflicts with "ads-client: simplify the http cache and ads store in-memory fallback" (mozilla#7713): its new `open_connection` helpers replace the fallback code this branch had changed, and `AdsStore` loses `is_memory`. The helpers get the same explicit `drop` for the lock taken only to open the database.
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
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.
Pull Request checklist
[ci full]to the PR title.