Repository navigation
AC-179 - Send the context id deletion request from the ads client over OHTTP - #7630
copyrighthero wants to merge 3 commits into
Conversation
b26e84a to
e2a747c
Compare
copyrighthero
changed the title
f8d3493 to
3be5fbb
Compare
…timeout Adds a DELETE /v1/delete_user request builder to the MARS client so the ads client can send the context id deletion itself instead of relying on the context_id component's direct request. The request goes through MARSTransport with a 5 s timeout on the relay request and, when asked, the ads-client OHTTP channel; gateway key retrieval keeps viaduct's own timeout. When OHTTP is asked for and the channel is not configured the request fails with OhttpChannelNotConfigured rather than being sent directly. Without OHTTP the helper sends a direct request; the OHTTP-only gate lives in the ads client's rotation path. MARSTransport gains fire_with_timeout, and client_for now takes the ClientSettings to use instead of always using the default.
…r OHTTP The embedded ContextIDComponent used to send DELETE /v1/delete_user to MARS directly whenever the context id rotated, outside the OHTTP channel the ads client uses for ad requests. The ads client now disables the component's request (running_in_test_automation = true, as Firefox already does for its JS-side component) and implements ContextIdCallback itself with a small queue: rotated ids are collected and flushed right after each ad fetch, with the OHTTP setting of the request_ads call that drains them. When that call did not use OHTTP the deletion is skipped and logged. Failures are logged and not retried. Consumers that do not configure an OHTTP channel for the ads client therefore no longer send the deletion request after this change. The context id still rotates on the same schedule. The usage docs now note that the ohttp request option also applies to this deletion request. This also replaces the cfg!(test) that was passed as running_in_test_automation, a compile-time constant answering a runtime question, which meant release builds always sent the component's request.
…e_user The contract test sends the same DELETE /v1/delete_user the ads client sends after a rotation, over the production relay and gateway, prints the status, and checks that MARS answers 200. Because it writes to production and CI runs this crate with --ignored, it returns without sending unless ADS_CLIENT_PROD_DELETE_USER_CONTRACT is set; it is meant to be run by hand with --nocapture and the observed status pasted into the PR.
3be5fbb to
744e05e
Compare
| /// writes to production when `ADS_CLIENT_PROD_DELETE_USER_CONTRACT` is set. | ||
| #[test] | ||
| #[ignore = "writes to production MARS: run manually with ADS_CLIENT_PROD_DELETE_USER_CONTRACT=1 -- --ignored --nocapture"] | ||
| fn test_contract_delete_user_ohttp_prod() { |
There was a problem hiding this comment.
Just so I understand, as I'm a bit confused with this test:
-
ADS_CLIENT_PROD_DELETE_USER_CONTRACTis an env variable that should be set only for tests, to run this test- is this correct? (can we have 'test' in the name, if so?) -
If our fear is having a DELETE key in a test that contacts prod, can we do it against staging instead?
-
I see that we've rebuilt with viaduct directly rather than use MARS client. I get that the client is private, but I don't really understand what this integration test is testing, then. If it's just making an HTTP request and checking the result, and doesn't use any adsclient code then it's just ostensibly a test for the actual MARS server and not for AdsClient. So even if this test can't work because of
mars.rsprivacy, I don't think this test tests anything on our end.
As for the privacy issue, we could create an AdsClient function that gets routed down to MARSClient's delete_user without making such a function available over FFI, right? It would be a method specifically for integration tests which isn't ideal but if we want such a test, it could be fine.
| self.0.lock().is_empty() | ||
| } | ||
|
|
||
| pub fn take_all(&self) -> Vec<String> { |
There was a problem hiding this comment.
nit: I think drain is more common nomenclature for this pattern, but take_all is fine IMO
There was a problem hiding this comment.
Thank you! Makes sense, I'll make the adjustments!
| continue; | ||
| } | ||
| if let Err(e) = self.client.delete_user(&old_context_id, true) { | ||
| error_support::warn!("Context id deletion request failed: {e}"); |
There was a problem hiding this comment.
I don't think long term we are going to be using error_support::error! macros- we have in a couple places for now, but are looking at using telemetry for errors like this long term. Can we fire a telemetry event here instead? (+ should we fire one for the ohttp skipped ones too?)
There was a problem hiding this comment.
This is a great call out! Thanks! Will adjust!
| /// Best-effort: sends each retired id's deletion over OHTTP, or skips it when | ||
| /// the ad request did not use OHTTP. Failures are logged and not retried. | ||
| fn flush_context_id_deletions(&self, retired_context_ids: Vec<String>, ohttp: bool) { | ||
| for old_context_id in retired_context_ids { |
There was a problem hiding this comment.
Do we need to call this function at all if ohttp is false? It seems nothing happens except for posting Skipping context id deletion len(retired_context_ids) times. Do we anticipate having behaviour later on ohttp=false? I think it's also OK if you are just looking to force anyone who calls this to be conscious of the ohttp rule, but even then, do we want to iterate over this log message n times?
| )?; | ||
| ); | ||
| // Flush even if the fetch failed: the ids have already left the queue. | ||
| self.flush_context_id_deletions(retired_context_ids, ohttp); |
There was a problem hiding this comment.
Another thought: what is the reasonable worst case for this? Are we ever expecting to have to send many flushes at once?
Send the
DELETE /v1/delete_userrequest for a retired context id from the ads client, over the same OHTTP channel as the ad request, instead of as a direct request from the embeddedcontext_idcomponent.Changes
MARSClient::delete_user(context_id, ohttp):DELETE /v1/delete_userwith{"context_id": ...}throughMARSTransport, 5 s timeout on the relay request, over theads-clientOHTTP channel when asked. If OHTTP is asked for and the channel is not configured it fails withOhttpChannelNotConfigured; there is no fallback to a direct request.DISABLE_CONTEXT_ID_COMPONENT_DELETION(true) asrunning_in_test_automation, the same thing Firefox does for its JS-side component inbrowser/modules/ContextId.sys.mjs. This replaces thecfg!(test)passed there before, which only disabled it in test builds.ContextIdDeletionQueueas theContextIdCallback: rotated ids are queued, andrequest_adstakes them before its ad fetch and sends their deletions right after it, with that call's OHTTP setting. The flush runs whether or not the fetch succeeded. Without OHTTP the deletion is skipped and logged; failures are logged and not retried.ads-clientstop sending the deletion request after this lands. The context id still rotates on the same schedule. Configuring the channel and passingohttp: trueon ad requests makes it resume.ohttprow ofMozAdsRequestOptionsin the Kotlin, Swift and JavaScript usage docs to say it also applies to the deletion request.ADS_CLIENT_PROD_DELETE_USER_CONTRACT=1 -- --ignored --nocapture). Ran it by hand: HTTP 200 observed 2026-10-14 through the production relay and gateway. It returns without sending unless the env var is set, so CI's--ignoredrun does not touch production.No changes to
ffi.rs, the binding APIs, orcomponents/context_id/.Testing
test_delete_user_sends_json_delete), fail-closed without an OHTTP channel,test_rotation_without_ohttp_skips_delete, failures only warn.f4970b3baab8) with this revision (744e05eb6) vendored, rotation interval patched to 3 minutes for the test,toolkit.rust-components.logging.crates=viaduct:trace,ads_client:info:unifiedAds.ohttp.enabled=true: on rotation theDELETE /v1/delete_userwas delivered over OHTTP through Firefox's own viaduct/necko backend: relay 200, inner HTTP 200 (3 bytes), 119 ms end to end.unifiedAds.ohttp.enabled=false: the next rotation logged18:50:04.420 ads_client::client: Skipping context id deletion request: the ad request did not use OHTTP.discoverystream.ohttp.relayURL=https://mozilla-ohttp.fastly-edge.com/,discoverystream.ohttp.configURL=https://prod.ohttp-gateway.prod.webservices.mozgcp.net/ohttp-configs.ads-clientOHTTP channel configured there, expected to take the skip path); relay or MARS failure injection; NGWAF behaviour. Context ids lost on process restart are not deleted; pre-existing behaviour, unchanged by this PR.For the reviewer
The flush is synchronous inside
request_ads, so once per rotation one ad request can take longer (one relay request with a 5 s timeout, plus a 10 s gateway key fetch if the key cache is cold). Measured locally: the DELETE added about 119 ms to that one ad request in an opt build. If the worker direction in #7597 lands first it seems natural to move it there; happy to rebase either way. Two spots worth a close look:get_context_id()andtake_all()are separate critical sections, so an overlapping call can drain an id a different call rotated. Since #7647, requests with relevance scores skipget_context_id(), so they never rotate the id, but they still flush any ids already queued. And scope: this changes which path carries the deletion, not when it is sent; it does not address correlation by request timing. The component's own request path will be removed in a follow-up. Thank you so much for taking a look!!Pull Request checklist