feat: Adds the background worker for fire-and-forget ads-client by thesuzerain · Pull Request #7597 · mozilla/application-services

thesuzerain · 2026-09-10T19:36:04Z

This adds the background worker part of the first layer of the fire-and-forget ads client.

This is still missing:

The FFI interface
RecordImpression, etc (this is only the ad requests)
The integration tests (which are written and worked in the original version of this PR, but used the FFI functions- I'm happy though to revamp all these tests if we want them in this PR)
Telemetry
Request bulking (eg: getting multiple of the requests in one send)

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

…into AC-154-ads-client-background-worker

thesuzerain · 2026-09-11T16:32:35Z

+
+// Command dispatch enum for passing different instructions to the background worker thread.
+// `RequestImageAds`, `RequestSpocAds`, `RequestTileAds` are prefetch mechanisms that query and load data into the local cache.
+pub enum DispatchCommand {


My intention is to have this changed internally to DispatchCommand::RequestAds and we can do a whole bunch of ad requests in one MARS call, but the MARS part isn't really set up for that yet.

So when I do the FFI layer, I'm going to have it be one request that splits out into these multiple commands, then come back and have a new MARS option for bulk requests.

thesuzerain · 2026-09-11T16:44:16Z

+    pub fn store_ads(&self, ads: HashMap<PlacementId, StorableAd>) -> Result<(), FetchAdsError> {
+        for (placement_id, ad) in ads {
+            self.holder.store_ad(&placement_id, ad)?;
+        }


I think its possible to also rewrite the sql queries to batch insert these, but not for this PR.

Almaju · 2026-09-11T21:26:53Z

+### Ads-Client
+
+- Adds a background worker to allow for future fire-and-forget logic.
+


I would say, let's no put anything in "What's changed" yet because right now it's still dev only. This can be misleading if someone wanted to try it. If we wanted to be extra clean we could even flag all the stateful code under some cargo flag. I think we could argue not to do it because we have a strong test suite and we mostly manage the vendoring end to end right now but that's on the table I think.

Almaju · 2026-09-11T21:28:23Z

+        ohttp: bool,
+        flags: HashMap<String, bool>,
+        blocks: Vec<String>,
+    },


Do you think it would be a lot of work on the MARS side to be able to just accept one request variant with Vec<MozAdsPlacementRequestWithCount> (count default to 1 if not provided for example)? I feel the 3 variants create a lot of code. I am not against doing this in future cleanup PR though if you prefer.

See my other comment- I definitely want to do this but I think it makes more sense to do in a separate PR as it will require MARS changes.

Almaju · 2026-09-11T21:31:08Z

+        } else {
+            Err(FetchAdsError::SqliteShutdown)
+        }
+    }


I think we should not use the word "cache" because it's not really a cache, it's more like a store / view / aggregate / projection / query model. (like CQRS pattern)
Also I am not sure why this would belong to client.rs which should be essentially the public API.

Good point, this is a vestige from the old version.

Almaju · 2026-09-11T21:33:38Z

+        } else {
+            Err(FetchAdsError::SqliteShutdown)
+        }
+    }


Looking at this, I feel it would probably be better not to use StorableAd but instead do something like:

pub fn query_spoc() -> Option<Spoc> pub fn query_tile() -> Option<Spoc> etc.

Then on the store side, have a generic parameter T: Deserialize.
I don't think we want to send errors because what would the client do with them? Instead, if there is an error, we should send it through telemetry, reschedule a command request and return None.

So, my initial plan for it was to do that on the next layer. eg: query_tile that calls get_stored_ad with an extraction mechanism. But I'm happy to move that to this layer instead. I'm not sure though if it makes sense to immediately reschedule a command request on that though given more than just the placement_id is needed, right?

I'll replace this with what my idea was.

Almaju · 2026-09-11T21:36:51Z

+pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT: usize = 10000;
+pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker";
+
+pub struct AdsClientWorkerWrapper {


Why not just BackgroundWorker?
AdsClient -> we are already in the ads-client crate so not sure we need that prefix
Wrapper -> when I see this, I expect to see some AdsClientWorker and something like:

pub struct AdsClientWorkerWrapper(Arc<Mutex<AdsClientWorker>>);

at least in a uniffi environment.

My logic was that it wasn't the thread itself, but I agree with your point.

Almaju · 2026-09-11T21:37:09Z

+pub const ADS_CLIENT_WORKER_THREAD_NAME: &str = "ads-client.worker";
+
+pub struct AdsClientWorkerWrapper {
+    _worker_thread: Option<JoinHandle<()>>,


Why the _ prefix?

Mostly because the JoinHandle is never actually read, it just needs to be held somewhere so it isn't dropped. _ prevents clippy + linting processes from identifying it as dead code because it's never referenced

Almaju · 2026-09-11T21:38:27Z

+pub mod command;
+
+// This is a somewhat arbitrary default value that is overridable.
+pub const ADS_CLIENT_WORKER_CHANNEL_BUFFER_SIZE_DEFAULT: usize = 10000;


I need to dig that, I am not sure how buffer size work but I would maybe explain in the comment why we think "10000" is a good idea. Also, if it's in bytes, we could maybe use the Byte struct directly. I never feel safe with unit conversion when using raw integers.

Almaju · 2026-09-11T21:39:30Z

+pub fn build_worker_thread(
+    inner_client: MozAdsClientInner,
+    max_channel_size: Option<usize>,
+) -> Option<(SyncSender<DispatchCommand>, JoinHandle<()>)> {


Given the signature, this very much looks like a constructor method.

Almaju · 2026-09-11T21:40:38Z

+// Spawn worker thread from a reference to the client, returning a synchronous channel transmitter to the thread, and its JoinHandle.
+// Returns None if thread fails to build.
+pub fn build_worker_thread(
+    inner_client: MozAdsClientInner,


Hmmm, this is coupling. I don't think the background thread should know about MozAdsClient stuff. It should be solely responsible for managing the background thread. The client should be responsible of the orchestration.

This kind of goes back to the problem of the client being locked with almost every function. Unless we rewrite a lot of things, the MozAdsClient layer gets a lock on the inner AdsClient with every single ads fetch etc.

We need to be able to send commands to the worker thread fire-and-forget, which means we can't have that be inside the Mutex<AdsClient<...>>, or whenever it is locked for any reason (eg: running a synchronous command or if one is running in the background).

We can maybe make it so that there is a second MARSClient separately held by the worker thread and clone some logic but I'm not sure how that would work with synchronous behaviour.

FWIW, MozAdsClientInner is also not inherently knowing about the MozAdsClient, but it does allow for it to do similar things as the MozAdsClient.
pub type MozAdsClientInner = Arc<Mutex<AdsClient<MozAdsTelemetryWrapper>>>;

Almaju · 2026-09-11T21:42:47Z

-    inner: Mutex<AdsClient<MozAdsTelemetryWrapper>>,
+    inner: MozAdsClientInner,
    shutdown_references: ShutdownReferences<MozAdsTelemetryWrapper>,
+    _worker: AdsClientWorkerWrapper,


I really don't like that we start adding business logic to the FFI object. It should be minimal and only be responsible of FFI concerns, not owning anything -> that's the role of the core client.rs. I understand that this is probably because of the shutdown requirement and it seemed okay as a quick fix for HNT but I would prefer if we can discuss a better long term solution for this one. 🤔

See my above comment about the fire-and-forget- we need to refactor these locks IMO if so. I'm not sure how easy that would be. 'ts not for the shutdown requirement but its for a similar reason as the shutdown requirement

thesuzerain added 9 commits September 10, 2026 12:27

feat: Adds the background worker

34415fc

fix: removes the FFI layer

791f0a6

feat: fixes, updates

ef7a00a

fix: removed ping

b187cd4

fix: pong removal

0e4c7d5

fix: adds shutdown

59fa831

fix: buffer

93f7d7d

fix: changelog

231d4cd

Merge branch 'main' of https://github.com/mozilla/application-services …

2e932a1

…into AC-154-ads-client-background-worker

thesuzerain commented Sep 11, 2026

View reviewed changes

thesuzerain requested review from Almaju and jonesetc September 11, 2026 16:36

thesuzerain marked this pull request as ready for review September 11, 2026 16:43

thesuzerain requested a review from a team as a code owner September 11, 2026 16:43

thesuzerain commented Sep 11, 2026

View reviewed changes

Almaju reviewed Sep 11, 2026

View reviewed changes

fix: Revisions

df0fcac

		### Ads-Client

		- Adds a background worker to allow for future fire-and-forget logic.

Uh oh!

Conversation

thesuzerain commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesuzerain Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thesuzerain Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thesuzerain commented Sep 10, 2026 •

edited

Loading

thesuzerain Sep 11, 2026 •

edited

Loading

thesuzerain Sep 11, 2026 •

edited

Loading