[AC-154] implement fire and forget ads client async

[thesuzerain]

The first initial slice for the plan to let ads-client work quasi-asynchronously: instead of having every single command be synchronous (waiting on a call to fully resolve) this adds a background thread worker to allow surface commands to be fire and forget.

• This adds fire-and-forget prefetch functions that perform the ads fetching in the background (queuing up the task).
• The background worker is still synchronous, but resolving these one at a time (we are not using async functions at the current point)
• Once the background worker resolves, it's going to be put into a local cache that can be queried instantaneously. In a future slice, this cache will use sqlite and have durability.
  • expected behaviour: if it does not exist in the cache when we need it, we don't need to load it.
• All of the functions are new and do not replace old functions- the old synchronous routes can still be used. They call the same function internally (eg: the sync and 'async' versions have the same handling internally when the 'async' version gores through the background worker)

Discussion points:

I've excluded the following from this PR:

• The nimbus work, while I will implement in a separate PR. We do want to use nimbus for the various slices of the async work, but because this adds an entirely new set of surface entrypoints, the branching based on nimbus flags will need to happen on the surface layer. (That being said, future slices will be able to use this, so it will still be included, but I'll do it in a separate PR)
• In an earlier version of this PR, I included a callback ErrorOnlyCallback that would allow the surfaces to receive any errors from the process running in the worker thread. However, in light of the uniffi callbacks issue, it requires a bit of refactoring to ensure these can be passed to the thread and shutdown successfully- possible, but adds a lot to this PR, so I've excluded it for now.
  • As a result, if there are errors in the fire-and-forget functions like RecordClick, the tests currently do not catch them.

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.

[thesuzerain]

One thing I iterated a bit earlier was adding callbacks so we can check for errors in the background processes in integration tests, but it becomes a bit problematic with the new shutdown functionality (to ensure we can immediately shutdown, we need to be able to kill all held callbacks in the process queue), meaning we need to have proper async implementation (tokio can kill held tasks in this way) or have an arena of Arc<> callbacks to shutdown on the shutdown() call.

Pinging is an internal function anyway, so we could also maintain a history of errors that gets returned on a ping (for purposes of tests), but this also adds more a bit more complexity (could separate that to another PR) and wouldn't be easily used by the surfaces.

[Almaju]

Thanks for putting this together! there's a lot of good thinking here. My main concern is scope: this combines FFI, worker/threading, caching, and telemetry changes in one PR, which made some things hard to isolate.

Would it be possible to split this into smaller, independently reviewable pieces? AdsCache as its own PR with unit tests, worker/dispatch logic as another? I also think it's worth discussing splitting the crate into ads-client-core (plain Rust, no uniffi) and ads-client (FFI wrapper), since a few comments below stem from the worker/command code depending directly on FFI types.

[Almaju]

Nit: could we keep the variants alphabetically ordered here? BackgroundWorker would go first, ahead of the Record*/Report*/RequestAds variants. Same applies to BackgroundWorkerError below (PongFailure, WorkerClosed, WorkerFull, WorkerTimedOut).

[Almaju]

I think worker shouldn't be owned by the FFI object here. My understanding is FFI's job should stay limited to uniffi/Arc/Mutex plumbing and breaking-change protection, not orchestrating a background thread. Would it make sense to move worker into AdsClient and use &mut self directly there, instead of going through Arc<Mutex<...>> at this level?

Also noticed worker/command.rs imports MozAdsPlacementRequest and AdsClientApiResult straight from the FFI layer, and does the FFI → domain conversion inside run_command (line 83-84). That conversion feels like it belongs in ffi.rs, not in the worker module.

[thesuzerain]

My intention with this is I really didn't want the fire and forget methods to need to have to wait on the mutex lock. So currently, any mutating call to the AdsClient (including the ones that get ads in the background) gets that lock, and if we keep AdsClientWorkerWrapper inside the Mutex<AdsClient<...>> we need to get a lock to fire-and-forget.

That means that if the background process wants to call request_spoc_ads, and gets a lock on it, any fire and forget commands in the meantime would have to wait.

If we don't want this struct to hold this, we could add a layer of indiscretion for it to work? Something like MozAdsClient -> AdsClientWithWorker (obviously with a different name than that) -> Mutex<AdsClient>

[Almaju]

Could we simplify this to a single requests: Vec<...> instead of three parallel vectors (image_ad_requests, spoc_ad_requests, tile_ad_requests)? Right now the caller has to split requests by ad type manually, and we do three separate empty-checks + dispatches, which undercuts the point of having a unified request type. If we need a per-item count for SPOCs, maybe that's an optional field (or enum) on the request item rather than a separate vector.

[thesuzerain]

Sure. I think most reasonable setup would be an enum here

AdRequest {
   Image{...},
   Spoc{...},
   Tile{...}
}

Though it might be a little more verbose on the surface implementation end

[Almaju]

Question: can this actually fail? It's reading from the in-memory cache and already returns Option, so I don't see a path to Err. If this is intentionally forward-looking (e.g. a future fallible cache backend), a short comment saying so would help, otherwise I'd lean towards dropping AdsClientApiResult here since it implies a failure mode that doesn't exist.

[thesuzerain]

Yeah, I was intending to be a bit futureproof here. I'll add a comment

[Almaju]

I'm not fully convinced by having both record_impression and dispatch_record_impression -> they parse the URL and map errors identically, the only difference is sync vs. queued. From a consumer's perspective these look interchangeable, which I think creates more confusion than it saves. If we want both a sync and a fire-and-forget flavor, could that be an explicit v1/v2 FFI decision rather than two similarly-named methods sitting next to each other?

Separate, bigger topic (feel free to punt): do we still want to expose raw impression URLs to the consumer at all, or should we be moving towards ids instead? 🤔

[thesuzerain]

Makes sense- this actually adds a reasonable usecase for the originally pitched ads-client.async-enabled nimbus flag (for the first slice), for whether the dispatches are immediate or async.

[Almaju]

If this happens, it looks like the client stays broken for the rest of its lifetime, nothing here attempts to restart the worker thread. Since this is an internal implementation detail, I'd prefer we recover from it (respawn) rather than propagate the error to the caller.

[Almaju]

These four enums all share the same four variants. Could we collapse them into a single ClientEvent enum plus a separate Phase (Dispatched/Processed/Failed)? That would also address the naming, CommandDispatchedOperationEvent is quite long for something scoped to this module.

[Almaju]

A few thoughts here:

1. Do we need the StorageType associated type / three separate maps? It looks like this could just be placement_id -> Vec<Ad> uniformly. image/tile would simply be a Vec of length 0 or 1, and the unwrap-to-single-ad already happens at the call site (take_first() in client.rs). That removes the trait indirection and the three near-identical impls.
2. Naming: is "cache" the right word? We already have http_cache as the actual cache (lookup-before-fetch, sqlite-backed, TTL/ETag). This structure is only ever written by the background worker after a prefetch resolves, and read via the query_* methods -> that reads more like a materialized view (CQRS-style) than a cache to me, and having two different "cache" concepts in the same crate risks confusion down the line. Would AdsStore be clearer?
3. Could we use a PlacementId newtype instead of a raw String while we're touching this to make it clearer what the key is?

[thesuzerain]

1. My initial version of it looked something like this but I ended up veering away from it because it felt a bit odd to be storing them as Vec<...> and then running them immediately. Happy to do it however though.

2. I'm happy to rename it to AdsStore. I think the term cache is still appropriate here but I agree with the naming confusion- it is intended to be bundled into the same sqlite database eventually as well.

[Almaju]

Is there a reason behind 1000? Seems like a reasonable starting point, but a short comment on how we landed on it would help future readers. Separately: since dispatch uses try_send, hitting this limit returns WorkerFull synchronously to callers of the fire-and-forget methods (dispatch_record_click, etc.), which seems at odds with these being fire-and-forget in the first place.

[Almaju]

I think this is backwards:

• t=1000: we cache an image ad for placement "a" → stored as ("a", (1000, ad)).
• t=1100: we prefetch an image ad for placement "b" → cache_ads is called with timestamp=1100, so x=1000 for "a"'s existing entry and x=1100 for "b"'s new one.
• retain evaluates "b": 1100 - 1100 = 0 < 300 → kept, correct.
• retain evaluates "a": 1000 - 1100 → this is a panic https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=aaa058d8f0bc8f002ce41e92ddb06bba

Beyond fixing the operand order, I'd suggest we stop doing the age check as raw arithmetic on u64 at three call sites and give it a name instead, something like:

struct CacheEntry<T>{
    inserted_at: Instant,
    value: T,
}

impl<T> CacheEntry<T>{
    fn is_expired(&self, ttl: Duration) -> bool {
        self.inserted_at.elapsed() >= ttl
    }
}