-
Notifications
You must be signed in to change notification settings - Fork 12
Avoid no-op EC KV reads in post-send pull sync #900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: fix/kv-eid-request-snapshot-ec-ttl
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ use super::generation::{generate_ec_id, is_valid_ec_id}; | |||||||||||||
| use super::kv::{CreateIfAbsentOutcome, KvIdentityGraph, apply_partner_id_updates}; | ||||||||||||||
| use super::kv_types::KvEntry; | ||||||||||||||
| use super::prebid_eids::collect_eid_cookie_updates; | ||||||||||||||
| use super::pull_sync_marker::{expire_marker, reconcile_marker}; | ||||||||||||||
| use super::registry::PartnerRegistry; | ||||||||||||||
| use super::{EcKvSnapshot, current_timestamp, log_id}; | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -48,6 +49,7 @@ pub fn ec_finalize_response( | |||||||||||||
| sharedid_cookie: Option<&str>, | ||||||||||||||
| response: &mut Response<EdgeBody>, | ||||||||||||||
| ) { | ||||||||||||||
| ec_context.validate_pull_sync_marker(settings, registry); | ||||||||||||||
| let consent_allows_ec = ec_consent_granted(ec_context.consent()); | ||||||||||||||
| let consent_withdrawn = ec_consent_withdrawn(ec_context.consent()); | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -58,8 +60,12 @@ pub fn ec_finalize_response( | |||||||||||||
| // consent input. | ||||||||||||||
| clear_ec_headers_on_response(response, Some(registry)); | ||||||||||||||
|
|
||||||||||||||
| // Only expire the browser cookie and tombstone the identity-graph row | ||||||||||||||
| // when the request carries an explicit withdrawal signal. | ||||||||||||||
| if consent_withdrawn { | ||||||||||||||
| expire_marker(ec_context.pull_sync_marker_mut(), response); | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+63
to
+65
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 wrench — Withdrawal always emits a marker-expiry
Downstream that matters: Gating on
Suggested change
Verified in an isolated worktree at this head: |
||||||||||||||
|
|
||||||||||||||
| // Only expire the EC cookie and tombstone the identity-graph row when | ||||||||||||||
| // explicit withdrawal accompanies an EC cookie. | ||||||||||||||
| if consent_withdrawn && ec_context.cookie_was_present() { | ||||||||||||||
| expire_ec_cookie(settings, response); | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -117,6 +123,8 @@ pub fn ec_finalize_response( | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| reconcile_pull_sync_marker(settings, registry, ec_context, response); | ||||||||||||||
|
|
||||||||||||||
| // Ordinary returning-user page views no longer refresh the browser | ||||||||||||||
| // cookie, emit the EC header, or update KV TTL. | ||||||||||||||
| return; | ||||||||||||||
|
|
@@ -128,6 +136,7 @@ pub fn ec_finalize_response( | |||||||||||||
| if ec_context.ec_generated() { | ||||||||||||||
| let (Some(graph), Some(ec_id)) = (kv, ec_context.ec_value().map(str::to_owned)) else { | ||||||||||||||
| log::info!("Skipping generated EC response write because KV graph is unavailable"); | ||||||||||||||
| reconcile_pull_sync_marker(settings, registry, ec_context, response); | ||||||||||||||
| return; | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -144,6 +153,26 @@ pub fn ec_finalize_response( | |||||||||||||
| log::warn!("Skipping generated EC cookie because backing row is not authoritative"); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| reconcile_pull_sync_marker(settings, registry, ec_context, response); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| fn reconcile_pull_sync_marker( | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 thinking — Marker issuance re-adds a Same mechanism as the wrench finding above, but inherent to the design rather than a bug. The returning-user path comments that "ordinary returning-user page views no longer refresh the browser cookie" — yet a marker issue or expire makes exactly those responses cookie-bearing, so Bounded to roughly one response per hour per user per host, against one saved KV read per navigation, so it is plausibly a clear win — but the trade is not in the plan's risk table or in |
||||||||||||||
| settings: &Settings, | ||||||||||||||
| registry: &PartnerRegistry, | ||||||||||||||
| ec_context: &mut EcContext, | ||||||||||||||
| response: &mut Response<EdgeBody>, | ||||||||||||||
| ) { | ||||||||||||||
| let ec_id = ec_context.ec_value().map(str::to_owned); | ||||||||||||||
| let snapshot = ec_context.kv_snapshot().clone(); | ||||||||||||||
| reconcile_marker( | ||||||||||||||
| settings, | ||||||||||||||
| registry, | ||||||||||||||
| ec_id.as_deref(), | ||||||||||||||
| &snapshot, | ||||||||||||||
| ec_context.pull_sync_marker_mut(), | ||||||||||||||
| response, | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| fn recover_orphaned_ec( | ||||||||||||||
|
|
@@ -1259,6 +1288,104 @@ mod tests { | |||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| #[test] | ||||||||||||||
| fn finalize_sets_marker_for_complete_pull_partner_snapshot() { | ||||||||||||||
| let settings = create_test_settings(); | ||||||||||||||
| let ec_id = sample_ec_id("compl1"); | ||||||||||||||
| let mut partner = make_partner("ssp.example.com"); | ||||||||||||||
| partner.pull_sync_enabled = true; | ||||||||||||||
| partner.pull_sync_url = Some("https://sync.example.com/pull".to_owned()); | ||||||||||||||
| partner.pull_sync_allowed_domains = vec!["sync.example.com".to_owned()]; | ||||||||||||||
| partner.ts_pull_token = Some(Redacted::new("pull-token".to_owned())); | ||||||||||||||
| let registry = PartnerRegistry::from_config(&[partner]).expect("should build registry"); | ||||||||||||||
| let mut ec_context = make_context( | ||||||||||||||
| Some(&ec_id), | ||||||||||||||
| Some(&ec_id), | ||||||||||||||
| true, | ||||||||||||||
| false, | ||||||||||||||
| Jurisdiction::NonRegulated, | ||||||||||||||
| ); | ||||||||||||||
| let mut entry = live_entry(); | ||||||||||||||
| entry.ids.insert( | ||||||||||||||
| "ssp.example.com".to_owned(), | ||||||||||||||
| crate::ec::kv_types::KvPartnerId { | ||||||||||||||
| uid: "partner-uid".to_owned(), | ||||||||||||||
| }, | ||||||||||||||
| ); | ||||||||||||||
| ec_context.set_kv_snapshot(EcKvSnapshot::Present { | ||||||||||||||
| ec_id, | ||||||||||||||
| entry: Box::new(entry), | ||||||||||||||
| generation: Some(1), | ||||||||||||||
| }); | ||||||||||||||
| let mut response = empty_response(); | ||||||||||||||
|
|
||||||||||||||
| ec_finalize_response( | ||||||||||||||
| &settings, | ||||||||||||||
| &mut ec_context, | ||||||||||||||
| None, | ||||||||||||||
| ®istry, | ||||||||||||||
| None, | ||||||||||||||
| None, | ||||||||||||||
| &mut response, | ||||||||||||||
| ); | ||||||||||||||
|
|
||||||||||||||
| let cookies = response | ||||||||||||||
| .headers() | ||||||||||||||
| .get_all(http::header::SET_COOKIE) | ||||||||||||||
| .iter() | ||||||||||||||
| .filter_map(|value| value.to_str().ok()) | ||||||||||||||
| .collect::<Vec<_>>(); | ||||||||||||||
| assert!( | ||||||||||||||
| cookies | ||||||||||||||
| .iter() | ||||||||||||||
| .any(|cookie| cookie.starts_with("ts-ec-pull-complete=v1.")), | ||||||||||||||
| "complete snapshot should issue the marker" | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| #[test] | ||||||||||||||
| fn explicit_withdrawal_expires_marker_without_ec_cookie() { | ||||||||||||||
| let settings = create_test_settings(); | ||||||||||||||
| let consent = ConsentContext { | ||||||||||||||
| jurisdiction: Jurisdiction::UsState("CA".to_owned()), | ||||||||||||||
| gpc: true, | ||||||||||||||
| source: ConsentSource::Cookie, | ||||||||||||||
| ..Default::default() | ||||||||||||||
| }; | ||||||||||||||
| let mut ec_context = make_context_with_consent(None, None, false, false, consent); | ||||||||||||||
| ec_context.set_pull_sync_marker_for_test( | ||||||||||||||
| crate::ec::pull_sync_marker::PullSyncMarkerState::Invalid, | ||||||||||||||
| ); | ||||||||||||||
| let mut response = empty_response(); | ||||||||||||||
|
|
||||||||||||||
| ec_finalize_response( | ||||||||||||||
| &settings, | ||||||||||||||
| &mut ec_context, | ||||||||||||||
| None, | ||||||||||||||
| &PartnerRegistry::empty(), | ||||||||||||||
| None, | ||||||||||||||
| None, | ||||||||||||||
| &mut response, | ||||||||||||||
| ); | ||||||||||||||
|
|
||||||||||||||
| let cookies = response | ||||||||||||||
| .headers() | ||||||||||||||
| .get_all(http::header::SET_COOKIE) | ||||||||||||||
| .iter() | ||||||||||||||
| .filter_map(|value| value.to_str().ok()) | ||||||||||||||
| .collect::<Vec<_>>(); | ||||||||||||||
| assert!( | ||||||||||||||
| cookies.iter().any(|cookie| { | ||||||||||||||
| cookie.starts_with("ts-ec-pull-complete=;") && cookie.contains("Max-Age=0") | ||||||||||||||
| }), | ||||||||||||||
| "withdrawal should expire the marker independently of EC cookie state" | ||||||||||||||
| ); | ||||||||||||||
| assert!( | ||||||||||||||
| cookies.iter().all(|cookie| !cookie.starts_with("ts-ec=;")), | ||||||||||||||
| "missing EC cookie should not add an EC-cookie expiry" | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| fn live_entry() -> KvEntry { | ||||||||||||||
| let mut entry = KvEntry::tombstone(1000); | ||||||||||||||
| entry.consent.ok = true; | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏ nitpick — The three-state loop asserts the same thing three times.
for reason in ["no partners", "complete snapshot", "unread marker state"]runs three byte-identicalprepare_pull_sync_after_send(None, …)calls. None of the three named states is exercised here — all three are decided insidebuild_pull_sync_context, which this test never calls — so the labels read as coverage that doesn't exist. What the test actually proves (aNoneplan never reaches the graph factory) is worth keeping; the loop is not.The three states themselves are already covered in core by
build_pull_sync_context_skips_empty_registry_and_complete_snapshot.Verified in an isolated worktree at this head:
cargo fmt --all -- --checkandcargo clippy-fastly(which compiles--all-targets, including this test) both pass with this applied.