diff --git a/crates/trusted-server-core/src/ec/finalize.rs b/crates/trusted-server-core/src/ec/finalize.rs index 1c8d5ee9a..5a718f3a5 100644 --- a/crates/trusted-server-core/src/ec/finalize.rs +++ b/crates/trusted-server-core/src/ec/finalize.rs @@ -1288,6 +1288,151 @@ mod tests { ); } + #[test] + fn finalize_withdrawal_tombstones_both_present_ids_once() { + let settings = create_test_settings(); + let active_ec = sample_ec_id("activ3"); + let cookie_ec = sample_ec_id("cook3e"); + let consent = ConsentContext { + jurisdiction: Jurisdiction::UsState("CA".to_owned()), + gpc: true, + source: ConsentSource::Cookie, + ..Default::default() + }; + let mut ec_context = + make_context_with_consent(Some(&active_ec), Some(&cookie_ec), true, false, consent); + let graph = KvIdentityGraph::in_memory("test_store"); + graph + .create( + &active_ec, + &KvEntry::minimal("active.example.com", "active-uid", 1_000), + ) + .expect("should seed active row"); + graph + .create( + &cookie_ec, + &KvEntry::minimal("cookie.example.com", "cookie-uid", 1_000), + ) + .expect("should seed cookie row"); + ec_context.set_kv_snapshot(graph.load_snapshot(&active_ec)); + let mut response = empty_response(); + + ec_finalize_response( + &settings, + &mut ec_context, + Some(&graph), + &PartnerRegistry::empty(), + None, + None, + &mut response, + ); + + let (active_tombstone, active_generation) = graph + .get(&active_ec) + .expect("should read active row") + .expect("should retain active tombstone"); + let (cookie_tombstone, cookie_generation) = graph + .get(&cookie_ec) + .expect("should read cookie row") + .expect("should retain cookie tombstone"); + assert!( + !active_tombstone.consent.ok, + "active row should be withdrawn" + ); + assert!( + active_tombstone.ids.is_empty(), + "active IDs should be cleared" + ); + assert!( + !cookie_tombstone.consent.ok, + "cookie row should be withdrawn" + ); + assert!( + cookie_tombstone.ids.is_empty(), + "cookie IDs should be cleared" + ); + + let mut repeated_response = empty_response(); + ec_finalize_response( + &settings, + &mut ec_context, + Some(&graph), + &PartnerRegistry::empty(), + None, + None, + &mut repeated_response, + ); + + assert_eq!( + graph + .get(&active_ec) + .expect("should read active row") + .expect("should retain active tombstone") + .1, + active_generation, + "repeated finalization should not rewrite active tombstone" + ); + assert_eq!( + graph + .get(&cookie_ec) + .expect("should read cookie row") + .expect("should retain cookie tombstone") + .1, + cookie_generation, + "repeated finalization should not rewrite cookie tombstone" + ); + } + + #[test] + fn finalize_withdrawal_keeps_cookie_deletion_on_kv_failure() { + let settings = create_test_settings(); + let ec_id = sample_ec_id("failw1"); + let consent = ConsentContext { + jurisdiction: Jurisdiction::UsState("CA".to_owned()), + gpc: true, + source: ConsentSource::Cookie, + ..Default::default() + }; + let mut ec_context = + make_context_with_consent(Some(&ec_id), Some(&ec_id), true, false, consent); + let graph = KvIdentityGraph::failing("unavailable-store"); + let mut response = empty_response(); + + ec_finalize_response( + &settings, + &mut ec_context, + Some(&graph), + &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::>(); + assert_eq!( + response.status(), + 200, + "KV failure should not change response status" + ); + assert!( + cookies + .iter() + .any(|cookie| { cookie.starts_with("ts-ec=;") && cookie.contains("Max-Age=0") }), + "KV failure should not prevent EC cookie deletion" + ); + assert!( + cookies.iter().any(|cookie| { + cookie.starts_with("ts-ec-pull-complete=;") && cookie.contains("Max-Age=0") + }), + "KV failure should not prevent marker deletion" + ); + } + #[test] fn finalize_sets_marker_for_complete_pull_partner_snapshot() { let settings = create_test_settings(); diff --git a/crates/trusted-server-core/src/ec/kv.rs b/crates/trusted-server-core/src/ec/kv.rs index 89decf1c6..58fc5b0c8 100644 --- a/crates/trusted-server-core/src/ec/kv.rs +++ b/crates/trusted-server-core/src/ec/kv.rs @@ -39,6 +39,9 @@ const ENTRY_TTL: Duration = Duration::from_secs(365 * 24 * 60 * 60); /// TTL for withdrawal tombstones (24 hours). const TOMBSTONE_TTL: Duration = Duration::from_secs(24 * 60 * 60); +/// Namespace for completion markers written after a withdrawal tombstone. +const WITHDRAWAL_MARKER_PREFIX: &str = "__ts_ec_withdrawal_complete__:"; + /// Outcome of an [`KvIdentityGraph::upsert_partner_id_if_exists`] call. /// /// Like [`KvIdentityGraph::upsert_partner_id`], this method fails closed when @@ -367,6 +370,10 @@ impl KvIdentityGraph { // Serialize once and reuse across the fast path and CAS loop. let (body, meta_str) = Self::serialize_entry(entry, self.store_name())?; + // Completion markers belong to withdrawn generations. Remove any + // marker before this key can become live again. + self.clear_withdrawal_marker(ec_id)?; + // Try create first — fast path for new entries. if self.write_entry(ec_id, &body, &meta_str, ENTRY_TTL, EcKvWriteMode::Add)? == EcKvWriteOutcome::Written @@ -398,6 +405,10 @@ impl KvIdentityGraph { let mut current_gen = generation; for attempt in 0..MAX_CAS_RETRIES { + // A completion marker belongs to the tombstone generation. Remove + // it before making this key live so a later withdrawal cannot be + // suppressed by stale fallback state. + self.clear_withdrawal_marker(ec_id)?; match self.write_entry( ec_id, &body, @@ -831,20 +842,70 @@ impl KvIdentityGraph { ))) } - /// Writes a withdrawal tombstone for consent enforcement. - /// - /// Overwrites the entry with `consent.ok = false`, empty partner IDs, - /// and a 24-hour TTL. Uses unconditional overwrite (no CAS) since the - /// entry is being withdrawn regardless of concurrent state. + fn withdrawal_marker_key(ec_id: &str) -> String { + format!("{WITHDRAWAL_MARKER_PREFIX}{ec_id}") + } + + fn withdrawal_marker_exists(&self, ec_id: &str) -> Result> { + let marker_key = Self::withdrawal_marker_key(ec_id); + Ok(self.store.count_keys_with_prefix(&marker_key, 1)? > 0) + } + + fn write_withdrawal_marker(&self, ec_id: &str) -> Result<(), Report> { + let marker_key = Self::withdrawal_marker_key(ec_id); + match self.store.insert( + &marker_key, + EcKvWrite { + body: "1", + metadata: "{}", + ttl: TOMBSTONE_TTL, + mode: EcKvWriteMode::Add, + }, + )? { + EcKvWriteOutcome::Written | EcKvWriteOutcome::PreconditionFailed => Ok(()), + } + } + + fn clear_withdrawal_marker(&self, ec_id: &str) -> Result<(), Report> { + if !self.withdrawal_marker_exists(ec_id)? { + return Ok(()); + } + + let marker_key = Self::withdrawal_marker_key(ec_id); + match self.store.delete(&marker_key) { + Ok(()) => Ok(()), + Err(delete_err) => match self.withdrawal_marker_exists(ec_id) { + // Another request removed the marker first. + Ok(false) => Ok(()), + Ok(true) | Err(_) => Err(delete_err), + }, + } + } + + fn record_withdrawal_completion(&self, ec_id: &str) { + if let Err(err) = self.write_withdrawal_marker(ec_id) { + // The root is already tombstoned. Preserve that successful privacy + // write even if the cost-control marker cannot be recorded. + log::warn!( + "withdrawal completion marker failed for '{}': {err:?}", + log_id(ec_id) + ); + } + } + + /// Writes a withdrawal tombstone after the caller confirms the key exists. /// - /// The tombstone preserves consent enforcement for batch sync clients - /// (`POST /_ts/api/v1/batch-sync`) during the 24-hour revocation window. + /// This unconditional fallback is reserved for an eventually consistent + /// point read that misses a key still visible from the primary data source. + /// A successful write records a same-TTL completion marker so repeated + /// stale misses do not overwrite the root or refresh its tombstone TTL. + /// Normal and repeated withdrawals use + /// [`Self::tombstone_existing_from_snapshot`] so an authoritative tombstone + /// remains a no-op and retains its original TTL. /// /// # Errors /// - /// Returns [`TrustedServerError::KvStore`] on store error. Callers on - /// the browser path should log at `error` level and continue — cookie - /// deletion is the primary enforcement mechanism. + /// Returns [`TrustedServerError::KvStore`] when serialization or storage fails. pub fn write_withdrawal_tombstone( &self, ec_id: &str, @@ -859,7 +920,10 @@ impl KvIdentityGraph { TOMBSTONE_TTL, EcKvWriteMode::Overwrite, ) { - Ok(_) => Ok(()), + Ok(_) => { + self.record_withdrawal_completion(ec_id); + Ok(()) + } Err(report) => Err(report.change_context(TrustedServerError::KvStore { store_name: self.store_name().to_owned(), message: format!("Failed to write tombstone for key '{ec_id}'"), @@ -893,14 +957,34 @@ impl KvIdentityGraph { /// /// A proven-absent key is a no-op: there is nothing to withdraw, and a /// forged cookie must not mint a row. A key that provably exists is - /// tombstoned unconditionally — no CAS generation is available after a - /// missed read, and a withdrawal must win over any concurrent write. An - /// existence check that itself fails leaves the withdrawal unresolved - /// rather than silently dropped. + /// tombstoned unconditionally when no completion marker exists because no + /// CAS generation is available after a missed read. The marker is written + /// only after a successful root tombstone, so it can safely suppress later + /// stale-miss overwrites. Marker lookup failure falls back to the privacy + /// write. An existence check that itself fails leaves the withdrawal + /// unresolved rather than silently dropped. fn tombstone_unproven_missing(&self, ec_id: &str, missing: EcKvSnapshot) -> EcKvSnapshot { match self.key_exists_confirmed(ec_id) { Ok(false) => missing, Ok(true) => { + match self.withdrawal_marker_exists(ec_id) { + Ok(true) => { + log::debug!( + "withdrawal tombstone for '{}': completion marker already exists", + log_id(ec_id) + ); + return missing; + } + Ok(false) => {} + Err(err) => { + // Marker failure must not weaken withdrawal. Fall back + // to the existing unconditional privacy write. + log::warn!( + "withdrawal completion marker lookup failed for '{}': {err:?}", + log_id(ec_id) + ); + } + } log::warn!( "withdrawal tombstone for '{}': point read missed a row the store still \ lists; writing an unconditional tombstone", @@ -961,6 +1045,11 @@ impl KvIdentityGraph { snapshot: EcKvSnapshot, ) -> EcKvSnapshot { let mut current = match snapshot { + EcKvSnapshot::Present { + ec_id: ref snapshot_id, + ref entry, + .. + } if snapshot_id == ec_id && !entry.consent.ok => return snapshot, EcKvSnapshot::Present { ec_id: ref snapshot_id, generation: Some(_), @@ -971,6 +1060,11 @@ impl KvIdentityGraph { for _attempt in 0..MAX_CAS_RETRIES { let generation = match current { + EcKvSnapshot::Present { + ec_id: ref snapshot_id, + ref entry, + .. + } if snapshot_id == ec_id && !entry.consent.ok => return current, EcKvSnapshot::Present { ec_id: ref snapshot_id, generation: Some(generation), @@ -1006,6 +1100,7 @@ impl KvIdentityGraph { EcKvWriteMode::IfGenerationMatch(generation), ) { Ok(EcKvWriteOutcome::Written) => { + self.record_withdrawal_completion(ec_id); return EcKvSnapshot::Present { ec_id: ec_id.to_owned(), entry: Box::new(tombstone), @@ -1139,10 +1234,10 @@ impl KvIdentityGraph { Ok(Some(cluster_size)) } - /// Hard-deletes the entry. + /// Hard-deletes the entry and any withdrawal completion marker. /// - /// Reserved for the IAB data deletion framework (deferred). For consent - /// withdrawal, use [`write_withdrawal_tombstone`](Self::write_withdrawal_tombstone). + /// Reserved for the IAB data deletion framework (deferred). Consent + /// withdrawal uses the snapshot-aware conditional tombstone path instead. /// /// # Errors /// @@ -1150,7 +1245,8 @@ impl KvIdentityGraph { pub fn delete(&self, ec_id: &str) -> Result<(), Report> { // The backend's delete already attaches store context, so propagate // without re-wrapping the same message. - self.store.delete(ec_id) + self.store.delete(ec_id)?; + self.clear_withdrawal_marker(ec_id) } } @@ -1298,6 +1394,17 @@ mod tests { entry } + fn concurrent_live_entry() -> KvEntry { + let mut entry = live_entry(); + entry.ids.insert( + "concurrent.example.com".to_owned(), + crate::ec::kv_types::KvPartnerId { + uid: "concurrent-uid".to_owned(), + }, + ); + entry + } + // ----------------------------------------------------------------------- // CAS-conflict injection tests // ----------------------------------------------------------------------- @@ -1313,6 +1420,7 @@ mod tests { inner: InMemoryEcKv, conflicts_remaining: std::sync::Mutex, revive_on_conflict: bool, + partner_update_on_conflict: bool, } impl ConflictInjectingEcKv { @@ -1321,6 +1429,16 @@ mod tests { inner: InMemoryEcKv::new("conflict-store"), conflicts_remaining: std::sync::Mutex::new(conflicts), revive_on_conflict, + partner_update_on_conflict: false, + } + } + + fn with_partner_update_on_conflict(conflicts: u32) -> Self { + Self { + inner: InMemoryEcKv::new("partner-conflict-store"), + conflicts_remaining: std::sync::Mutex::new(conflicts), + revive_on_conflict: true, + partner_update_on_conflict: true, } } @@ -1368,8 +1486,13 @@ mod tests { if self.revive_on_conflict { // Simulate a concurrent writer reviving the entry // between this writer's read and its CAS write. + let concurrent_entry = if self.partner_update_on_conflict { + concurrent_live_entry() + } else { + live_entry() + }; let (body, meta) = KvIdentityGraph::serialize_entry( - &live_entry(), + &concurrent_entry, self.inner.store_name(), ) .expect("should serialize concurrent live entry"); @@ -1642,12 +1765,19 @@ mod tests { } #[test] - fn create_or_revive_revives_tombstone() { + fn create_or_revive_clears_withdrawal_marker() { let kv = KvIdentityGraph::in_memory("test_store"); let ec_id = format!("{}.ABC123", "a".repeat(64)); + kv.create(&ec_id, &live_entry()) + .expect("should create live entry"); + let snapshot = kv.load_snapshot(&ec_id); + kv.tombstone_existing_from_snapshot(&ec_id, snapshot); + assert!( + kv.withdrawal_marker_exists(&ec_id) + .expect("should read withdrawal marker"), + "withdrawal should record completion" + ); - kv.create(&ec_id, &KvEntry::tombstone(1000)) - .expect("should create tombstone"); kv.create_or_revive(&ec_id, &live_entry()) .expect("should revive tombstone"); @@ -1656,6 +1786,30 @@ mod tests { .expect("should read entry back") .expect("should find revived entry"); assert!(loaded.consent.ok, "should be live after revive"); + assert!( + !kv.withdrawal_marker_exists(&ec_id) + .expect("should read withdrawal marker"), + "revival should clear stale withdrawal completion" + ); + } + + #[test] + fn delete_removes_withdrawal_marker() { + let kv = KvIdentityGraph::in_memory("test_store"); + let ec_id = format!("{}.ABC123", "a".repeat(64)); + kv.create(&ec_id, &live_entry()) + .expect("should create live entry"); + let snapshot = kv.load_snapshot(&ec_id); + kv.tombstone_existing_from_snapshot(&ec_id, snapshot); + + kv.delete(&ec_id).expect("should delete entry and marker"); + + assert!(kv.get(&ec_id).expect("should read store").is_none()); + assert!( + !kv.withdrawal_marker_exists(&ec_id) + .expect("should read withdrawal marker"), + "hard delete should remove withdrawal completion" + ); } #[test] @@ -1741,22 +1895,6 @@ mod tests { assert!(kv.get(&ec_id).expect("should read store").is_none()); } - #[test] - fn write_withdrawal_tombstone_overwrites_live_entry() { - let kv = KvIdentityGraph::in_memory("test_store"); - let ec_id = format!("{}.ABC123", "a".repeat(64)); - kv.create(&ec_id, &live_entry()).expect("should create"); - - kv.write_withdrawal_tombstone(&ec_id) - .expect("should write tombstone"); - - let (loaded, _) = kv - .get(&ec_id) - .expect("should read entry back") - .expect("should find tombstone entry"); - assert!(!loaded.consent.ok, "should be withdrawn after tombstone"); - } - #[test] fn tombstone_existing_from_snapshot_never_creates_missing_key() { let kv = KvIdentityGraph::in_memory("test_store"); @@ -1800,6 +1938,175 @@ mod tests { // Snapshot-aware mutation stores and tests // ----------------------------------------------------------------------- + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + struct RecordedEcKvInsert { + mode: EcKvWriteMode, + ttl: Duration, + } + + #[derive(Default)] + struct RecordedEcKvOperations { + lookups: std::sync::atomic::AtomicUsize, + inserts: std::sync::Mutex>, + } + + impl RecordedEcKvOperations { + fn reset(&self) { + self.lookups.store(0, std::sync::atomic::Ordering::Relaxed); + self.inserts + .lock() + .expect("should lock recorded inserts") + .clear(); + } + + fn lookup_count(&self) -> usize { + self.lookups.load(std::sync::atomic::Ordering::Relaxed) + } + + fn inserts(&self) -> Vec { + self.inserts + .lock() + .expect("should lock recorded inserts") + .clone() + } + } + + /// In-memory store that records every backend operation before delegation. + struct RecordingEcKv { + inner: InMemoryEcKv, + operations: Arc, + stale_lookups_remaining: std::sync::Mutex, + } + + impl RecordingEcKv { + fn new(operations: Arc) -> Self { + Self::with_stale_lookups(operations, 0) + } + + fn with_stale_lookups(operations: Arc, stale_lookups: u32) -> Self { + Self { + inner: InMemoryEcKv::new("recording-store"), + operations, + stale_lookups_remaining: std::sync::Mutex::new(stale_lookups), + } + } + } + + impl EcKvStore for RecordingEcKv { + fn store_name(&self) -> &str { + self.inner.store_name() + } + + fn lookup(&self, key: &str) -> Result, Report> { + self.operations + .lookups + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let mut stale_lookups = self + .stale_lookups_remaining + .lock() + .expect("should lock stale lookup counter"); + if *stale_lookups > 0 { + *stale_lookups -= 1; + return Ok(None); + } + self.inner.lookup(key) + } + + fn insert( + &self, + key: &str, + write: EcKvWrite<'_>, + ) -> Result> { + self.operations + .inserts + .lock() + .expect("should lock recorded inserts") + .push(RecordedEcKvInsert { + mode: write.mode, + ttl: write.ttl, + }); + self.inner.insert(key, write) + } + + fn count_keys_with_prefix( + &self, + prefix: &str, + limit: u32, + ) -> Result> { + self.inner.count_keys_with_prefix(prefix, limit) + } + + fn delete(&self, key: &str) -> Result<(), Report> { + self.inner.delete(key) + } + } + + /// Store whose completion-marker operations fail while root operations work. + struct MarkerFailingEcKv { + inner: InMemoryEcKv, + stale_lookups_remaining: std::sync::Mutex, + } + + impl MarkerFailingEcKv { + fn new(stale_lookups: u32) -> Self { + Self { + inner: InMemoryEcKv::new("marker-failing-store"), + stale_lookups_remaining: std::sync::Mutex::new(stale_lookups), + } + } + + fn marker_error(&self, operation: &str) -> Report { + Report::new(TrustedServerError::KvStore { + store_name: self.inner.store_name().to_owned(), + message: format!("completion marker {operation} failed"), + }) + } + } + + impl EcKvStore for MarkerFailingEcKv { + fn store_name(&self) -> &str { + self.inner.store_name() + } + + fn lookup(&self, key: &str) -> Result, Report> { + let mut stale_lookups = self + .stale_lookups_remaining + .lock() + .expect("should lock stale lookup counter"); + if *stale_lookups > 0 { + *stale_lookups -= 1; + return Ok(None); + } + self.inner.lookup(key) + } + + fn insert( + &self, + key: &str, + write: EcKvWrite<'_>, + ) -> Result> { + if key.starts_with(WITHDRAWAL_MARKER_PREFIX) { + return Err(self.marker_error("write")); + } + self.inner.insert(key, write) + } + + fn count_keys_with_prefix( + &self, + prefix: &str, + limit: u32, + ) -> Result> { + if prefix.starts_with(WITHDRAWAL_MARKER_PREFIX) { + return Err(self.marker_error("lookup")); + } + self.inner.count_keys_with_prefix(prefix, limit) + } + + fn delete(&self, key: &str) -> Result<(), Report> { + self.inner.delete(key) + } + } + /// [`EcKvStore`] whose reads succeed but every write fails, simulating a /// store that becomes unwritable mid-request. struct WriteFailingEcKv { @@ -2176,6 +2483,301 @@ mod tests { ); } + #[test] + fn tombstone_existing_from_snapshot_skips_backend_for_authoritative_tombstone() { + for generation in [Some(7), None] { + let operations = Arc::new(RecordedEcKvOperations::default()); + let graph = KvIdentityGraph::new(RecordingEcKv::new(Arc::clone(&operations))); + let ec_id = snapshot_ec_id(); + let snapshot = EcKvSnapshot::Present { + ec_id: ec_id.clone(), + entry: Box::new(KvEntry::tombstone(1_000)), + generation, + }; + + let outcome = graph.tombstone_existing_from_snapshot(&ec_id, snapshot.clone()); + + assert_eq!( + outcome, snapshot, + "should preserve authoritative tombstone state" + ); + assert_eq!( + operations.lookup_count(), + 0, + "should not reread a tombstone" + ); + assert!( + operations.inserts().is_empty(), + "should not attempt to rewrite a tombstone" + ); + } + } + + #[test] + fn tombstone_existing_from_snapshot_repeated_request_preserves_first_write() { + let operations = Arc::new(RecordedEcKvOperations::default()); + let graph = KvIdentityGraph::new(RecordingEcKv::new(Arc::clone(&operations))); + let ec_id = snapshot_ec_id(); + graph + .create(&ec_id, &live_entry()) + .expect("should seed live row"); + let live_snapshot = graph.load_snapshot(&ec_id); + operations.reset(); + + graph.tombstone_existing_from_snapshot(&ec_id, live_snapshot); + + assert_eq!( + operations.lookup_count(), + 0, + "usable generation should avoid a read" + ); + assert_eq!( + operations.inserts(), + vec![ + RecordedEcKvInsert { + mode: EcKvWriteMode::IfGenerationMatch(1), + ttl: TOMBSTONE_TTL, + }, + RecordedEcKvInsert { + mode: EcKvWriteMode::Add, + ttl: TOMBSTONE_TTL, + }, + ], + "first withdrawal should write the root and its completion marker" + ); + let first_snapshot = graph.load_snapshot(&ec_id); + let (first_entry, first_generation) = match &first_snapshot { + EcKvSnapshot::Present { + entry, generation, .. + } => (entry.as_ref().clone(), *generation), + other => panic!("should load first tombstone, got {other:?}"), + }; + operations.reset(); + + let second_outcome = graph.tombstone_existing_from_snapshot(&ec_id, first_snapshot); + + assert_eq!( + operations.lookup_count(), + 0, + "repeated withdrawal should not reread" + ); + assert!( + operations.inserts().is_empty(), + "repeated withdrawal should not refresh the tombstone TTL" + ); + assert_eq!( + second_outcome.generation_for(&ec_id), + first_generation, + "repeated withdrawal should preserve the stored generation" + ); + assert_eq!( + second_outcome + .entry_for(&ec_id) + .map(|entry| entry.consent.updated), + Some(first_entry.consent.updated), + "repeated withdrawal should preserve the first tombstone timestamp" + ); + } + + #[test] + fn tombstone_existing_from_repeated_stale_miss_preserves_first_write() { + let operations = Arc::new(RecordedEcKvOperations::default()); + let graph = KvIdentityGraph::new(RecordingEcKv::with_stale_lookups( + Arc::clone(&operations), + 2, + )); + let ec_id = snapshot_ec_id(); + graph + .create(&ec_id, &live_entry()) + .expect("should seed live row"); + operations.reset(); + + let first_outcome = graph.tombstone_existing_from_snapshot( + &ec_id, + EcKvSnapshot::Missing { + ec_id: ec_id.clone(), + }, + ); + let first_updated = first_outcome + .entry_for(&ec_id) + .expect("should return first tombstone") + .consent + .updated; + operations.reset(); + + graph.tombstone_existing_from_snapshot( + &ec_id, + EcKvSnapshot::Missing { + ec_id: ec_id.clone(), + }, + ); + + assert!( + operations.inserts().is_empty(), + "a repeated stale miss should not rewrite the completed tombstone" + ); + let (stored, generation) = graph + .get(&ec_id) + .expect("should read stored tombstone") + .expect("should preserve tombstone"); + assert_eq!( + generation, 2, + "only the first withdrawal should advance the root generation" + ); + assert_eq!( + stored.consent.updated, first_updated, + "repeated withdrawal should preserve the first tombstone timestamp" + ); + } + + #[test] + fn tombstone_existing_from_stale_parallel_snapshot_stops_after_conflict() { + let operations = Arc::new(RecordedEcKvOperations::default()); + let graph = KvIdentityGraph::new(RecordingEcKv::new(Arc::clone(&operations))); + let ec_id = snapshot_ec_id(); + graph + .create(&ec_id, &live_entry()) + .expect("should seed live row"); + let stale_snapshot = graph.load_snapshot(&ec_id); + operations.reset(); + + let first_outcome = graph.tombstone_existing_from_snapshot(&ec_id, stale_snapshot.clone()); + let second_outcome = graph.tombstone_existing_from_snapshot(&ec_id, stale_snapshot); + + assert_eq!( + operations.inserts(), + vec![ + RecordedEcKvInsert { + mode: EcKvWriteMode::IfGenerationMatch(1), + ttl: TOMBSTONE_TTL, + }, + RecordedEcKvInsert { + mode: EcKvWriteMode::Add, + ttl: TOMBSTONE_TTL, + }, + RecordedEcKvInsert { + mode: EcKvWriteMode::IfGenerationMatch(1), + ttl: TOMBSTONE_TTL, + }, + ], + "parallel loser should attempt stale CAS once and never replace the winner" + ); + assert_eq!( + operations.lookup_count(), + 1, + "parallel loser should reread exactly once after its conflict" + ); + assert_eq!( + second_outcome.generation_for(&ec_id), + Some(2), + "parallel loser should return the winner's stored generation" + ); + assert_eq!( + second_outcome + .entry_for(&ec_id) + .map(|entry| entry.consent.updated), + first_outcome + .entry_for(&ec_id) + .map(|entry| entry.consent.updated), + "parallel loser should preserve the winner's tombstone" + ); + } + + #[test] + fn tombstone_existing_from_snapshot_succeeds_without_backend_for_tombstone() { + let graph = KvIdentityGraph::failing("unavailable-store"); + let ec_id = snapshot_ec_id(); + let snapshot = EcKvSnapshot::Present { + ec_id: ec_id.clone(), + entry: Box::new(KvEntry::tombstone(1_000)), + generation: Some(3), + }; + + let outcome = graph.tombstone_existing_from_snapshot(&ec_id, snapshot.clone()); + + assert_eq!( + outcome, snapshot, + "authoritative tombstone should not touch unavailable backend" + ); + } + + #[test] + fn tombstone_existing_from_snapshot_non_authoritative_states_reread_live_row() { + let ec_id = snapshot_ec_id(); + let states = [ + EcKvSnapshot::NotRead, + EcKvSnapshot::Failed { + ec_id: ec_id.clone(), + }, + EcKvSnapshot::Present { + ec_id: ec_id.clone(), + entry: Box::new(live_entry()), + generation: None, + }, + EcKvSnapshot::Present { + ec_id: "different-ec-id".to_owned(), + entry: Box::new(KvEntry::tombstone(1_000)), + generation: Some(9), + }, + ]; + + for state in states { + let operations = Arc::new(RecordedEcKvOperations::default()); + let graph = KvIdentityGraph::new(RecordingEcKv::new(Arc::clone(&operations))); + graph + .create(&ec_id, &live_entry()) + .expect("should seed live row"); + operations.reset(); + + let outcome = graph.tombstone_existing_from_snapshot(&ec_id, state); + + assert_eq!( + operations.lookup_count(), + 1, + "state should force one reread" + ); + assert_eq!( + operations.inserts().len(), + 2, + "live reread should write the root and completion marker" + ); + assert!( + outcome + .entry_for(&ec_id) + .is_some_and(|entry| !entry.consent.ok), + "reread live row should be tombstoned" + ); + } + } + + #[test] + fn tombstone_stale_miss_still_writes_when_marker_operations_fail() { + let graph = KvIdentityGraph::new(MarkerFailingEcKv::new(1)); + let ec_id = snapshot_ec_id(); + graph + .create(&ec_id, &live_entry()) + .expect("should seed live row"); + + let outcome = graph.tombstone_existing_from_snapshot( + &ec_id, + EcKvSnapshot::Missing { + ec_id: ec_id.clone(), + }, + ); + + assert!( + outcome + .entry_for(&ec_id) + .is_some_and(|entry| !entry.consent.ok), + "marker failures must not suppress the withdrawal write" + ); + let (stored, _) = graph + .get(&ec_id) + .expect("should read stored row") + .expect("should preserve the root"); + assert!(!stored.consent.ok, "root should remain tombstoned"); + } + #[test] fn tombstone_existing_from_snapshot_retries_cas_conflict() { let graph = KvIdentityGraph::new(ConflictInjectingEcKv::new(1, false)); @@ -2243,6 +2845,49 @@ mod tests { ); } + #[test] + fn tombstone_existing_from_snapshot_overrides_concurrent_live_update() { + let graph = KvIdentityGraph::new(ConflictInjectingEcKv::with_partner_update_on_conflict(1)); + let ec_id = snapshot_ec_id(); + graph + .create(&ec_id, &live_entry()) + .expect("should seed live row"); + let snapshot = graph.load_snapshot(&ec_id); + + let outcome = graph.tombstone_existing_from_snapshot(&ec_id, snapshot); + + let entry = outcome + .entry_for(&ec_id) + .expect("should return persisted tombstone"); + assert!(!entry.consent.ok, "withdrawal should win after retry"); + assert!( + entry.ids.is_empty(), + "withdrawal should clear concurrent partner IDs" + ); + } + + #[test] + fn upsert_partner_id_rejects_tombstone() { + let graph = KvIdentityGraph::in_memory("test-store"); + let ec_id = snapshot_ec_id(); + graph + .create(&ec_id, &KvEntry::tombstone(1_000)) + .expect("should seed tombstone"); + + let result = graph.upsert_partner_id(&ec_id, "ssp.example.com", "uid-1"); + + assert!(result.is_err(), "public upsert should reject a tombstone"); + let (stored, _) = graph + .get(&ec_id) + .expect("should read store") + .expect("should preserve tombstone"); + assert!(!stored.consent.ok, "entry should remain withdrawn"); + assert!( + stored.ids.is_empty(), + "upsert should not repopulate partner IDs" + ); + } + #[test] fn tombstone_existing_from_snapshot_store_failure_returns_failed() { let graph = KvIdentityGraph::new(WriteFailingEcKv::new()); diff --git a/docs/guide/edge-cookies.md b/docs/guide/edge-cookies.md index 5b85dd436..5f71efba8 100644 --- a/docs/guide/edge-cookies.md +++ b/docs/guide/edge-cookies.md @@ -124,7 +124,7 @@ flowchart TD - **Non-regulated**: EC always allowed. - **Unknown**: Fail-closed when jurisdiction cannot be determined. -The `ec_identity_store` KV store is the only EC lifecycle store. It holds identity graph state, source-domain keyed partner UIDs, a minimal consent snapshot used for EC entry metadata, and withdrawal tombstones. Consent interpretation for each request remains based on the live request signals listed above. +The `ec_identity_store` KV store is the only EC lifecycle store. It holds identity graph state, source-domain keyed partner UIDs, a minimal consent snapshot used for EC entry metadata, withdrawal tombstones, and same-TTL completion markers that prevent stale point-read misses from rewriting completed tombstones. Consent interpretation for each request remains based on the live request signals listed above. ## Partner Sync Channels diff --git a/docs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.md b/docs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.md new file mode 100644 index 000000000..ec424e0f7 --- /dev/null +++ b/docs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.md @@ -0,0 +1,350 @@ +# Issue #881: Idempotent EC Withdrawal Tombstones Plan + +- **Date:** 2026-07-13 +- **Status:** Implemented and verified +- **Issue:** [#881 — Make EC withdrawal tombstoning idempotent across request bursts](https://github.com/IABTechLab/trusted-server/issues/881) +- **Stack base:** [Draft PR #900 — Avoid no-op EC KV reads in post-send pull sync](https://github.com/IABTechLab/trusted-server/pull/900) +- **Underlying dependency:** [PR #885 — Request-scoped EC KV snapshot and orphan recovery](https://github.com/IABTechLab/trusted-server/pull/885) + +## Goal + +Make explicit EC withdrawal idempotent across repeated and concurrent requests +without weakening the existing-key-only privacy invariant introduced by PR +#885. The first successful withdrawal of a live row writes a CAS-protected +24-hour tombstone and a same-TTL completion marker. A request that already has +authoritative tombstone state returns without reading or writing KV. When an +eventually consistent point read instead misses the existing tombstone, the +strongly read completion marker prevents an unconditional replacement write, +so neither path refreshes the tombstone's entry timestamp or TTL. + +Browser-cookie deletion remains synchronous and best-effort KV failure must +never block the response. + +## Clarified Semantics + +- An authoritative missing row is a no-op. A valid-looking but unverified + browser cookie must never create a KV root. +- A matching authoritative tombstone snapshot is returned unchanged before any + lookup, serialization, or write, regardless of whether its generation is + available. +- A matching live snapshot with a generation uses one conditional write. +- A live snapshot without a generation, a failed/not-read snapshot, or a + snapshot for another EC ID rereads the requested row before deciding. +- CAS conflicts reread and retry. If another withdrawal has already written a + tombstone, retry ends without another write. If a concurrent live update or + re-consent changed the generation first, withdrawal retries against that live + row and tombstones it. +- A later re-consent may legitimately win when it linearizes after a completed + or no-op withdrawal. Idempotency does not impose global withdrawal priority. +- Repeated withdrawal preserves the original tombstone expiration because it + performs no second root write. A completion-marker insert is attempted only + after the first successful tombstone write. +- A repeated stale point-read miss uses the strongly consistent completion + marker to avoid another unconditional root write. +- Completion-marker failure never suppresses the privacy write, and revival or + hard deletion clears the marker before the key can become live again. +- When cookie and active EC IDs differ, every valid existing row is withdrawn + independently; missing or malformed IDs are never created. +- `ts-ec` and the pull-completeness marker are expired before best-effort KV + work. Store failure is logged/swallowed by finalization. + +## Non-Goals + +- Do not recreate missing roots from browser cookies. +- Do not add a separate deduplication store, cross-request lock, or withdrawal + cookie. The stale-miss completion marker lives in the existing EC store. +- Do not restrict withdrawal handling to document navigations. +- Do not change the 24-hour tombstone duration or root-entry KV schema. +- Do not change EC generation, browser pull-marker behavior, batch sync, pull sync, or + partner-upsert semantics beyond preserving tombstone rejection. +- Do not make withdrawal dominate a re-consent that occurs after withdrawal's + linearization point. +- Do not rewrite archival specs that describe the superseded unconditional + helper. + +## Current Behavior + +`KvIdentityGraph::tombstone_existing_from_snapshot` already preserves PR #885's +existing-key-only and CAS behavior, but it writes a fresh tombstone whenever the +snapshot is `Present`, including when that entry is already a tombstone. Parallel +withdrawals therefore converge safely but still perform a redundant CAS write, +and repeated requests reset the tombstone's 24-hour TTL. + +The unconditional `write_withdrawal_tombstone` helper remains necessary when +point reads miss a root that the strong list still sees. Without durable +completion state, repeated stale misses bypass the `Present` fast path and keep +rewriting that root. + +Finalization already: + +- expires browser state before KV work; +- collects both valid cookie and active EC IDs; +- uses the carried snapshot only for its matching ID; +- independently resolves the other ID; +- logs/swallows KV failures. + +The implementation should therefore remain concentrated in the core KV method, +with finalization changes limited to acceptance-level integration tests unless a +test exposes a defect. + +## Proposed Design + +### 1. Add an authoritative tombstone fast path + +At the beginning of each `tombstone_existing_from_snapshot` retry iteration, +match a `Present` snapshot only when its `ec_id` equals the requested ID. + +- If `entry.consent.ok == false`, return the exact snapshot immediately. +- Perform this check before requiring a generation or constructing a new + `KvEntry::tombstone`. +- Preserve the snapshot's entry timestamp and generation exactly. + +Then retain the existing state machine: + +| Initial/refreshed state | Action | +| ----------------------------------------- | ------------------------------------- | +| Matching tombstone | Return unchanged; zero backend work | +| Matching live entry + generation | CAS-write a 24-hour tombstone | +| Matching live entry without generation | Reread | +| Refreshed `Missing` | Prove absence or use guarded fallback | +| `Failed`, `NotRead`, or wrong-ID snapshot | Reread requested ID | +| CAS precondition failure | Reread and retry | +| Row disappears during retry | Return `Missing` | +| Store failure or retry exhaustion | Return ID-bound `Failed` | + +A successful tombstone remains `consent.ok = false`, has empty partner IDs, and +uses `TOMBSTONE_TTL`. + +Each successful root tombstone also creates an add-only completion marker in the +same EC store with `TOMBSTONE_TTL`. The marker namespace cannot collide with EC +IDs and is excluded from hash-prefix cluster counts. If a later point read +misses but the strong root-existence check and marker check both succeed, +withdrawal returns without rewriting the root. Marker read or write failures +fall back to the root privacy write. Same-key revival and hard deletion remove +the marker. + +### 2. Contain the unconditional fallback + +Keep `write_withdrawal_tombstone` only for the case where eventually consistent +point reads miss a root that the strong list still sees. Record completion after +that write so another stale miss cannot refresh the root. The marker is +best-effort after a successful root write; marker failure is logged and cannot +turn a completed privacy write into a reported failure. + +Update hard deletion and same-key revival to remove completion state. Historical +design documents may remain unchanged. + +### 3. Prove operation-level idempotency + +Use a focused recording backend around the existing in-memory store. It must +count every lookup and insert attempt, including inserts that return a CAS +precondition failure, and record each write's mode and TTL. Tests must show: + +- a supplied matching tombstone returns with zero lookups and zero insert + attempts; +- generation-unavailable tombstone state also performs no backend operation; +- the first live withdrawal performs one `IfGenerationMatch` root insert and + one add-only completion-marker insert with `TOMBSTONE_TTL`, while a repeated + authoritative tombstone causes zero additional insert attempts and leaves + stored root generation and `consent.updated` unchanged; +- two consecutive stale point-read misses cause one unconditional root write; + the second request observes the strong completion marker and leaves the root + generation and `consent.updated` unchanged; +- two stale live snapshots model parallel requests: the first writes the + tombstone; the second conflicts, rereads the tombstone, and performs no + replacement write; +- a supplied tombstone succeeds even against an always-failing backend, proving + no hidden operation; +- live state with generation avoids an initial read and uses one CAS write; +- missing state remains a no-op; +- `NotRead`, failed, generation-unavailable, and wrong-ID states reread the + requested row before applying the documented write/no-create behavior. + +The recording backend's first-write TTL assertion plus zero additional insert +attempts on repetition is the authoritative proof that the original tombstone +TTL was not refreshed. + +### 4. Preserve race ordering and tombstone authority + +Extend conflict tests to model a concurrent live update/re-consent that changes +the generation before withdrawal's first CAS. Withdrawal must reread the live +row and eventually write the tombstone, clearing any partner IDs. + +Retain the existing batch-sync conditional-upsert and snapshot bulk-upsert +tombstone tests, and add focused coverage for the public single-partner upsert +path so all live enrichment APIs are proven unable to repopulate tombstones: + +- `upsert_partner_id_if_exists` rejects tombstones; +- `upsert_partner_id` returns an error and leaves tombstone IDs empty; +- snapshot bulk upsert cannot repopulate a tombstone; +- a disappeared row is not recreated; +- store errors return failed state rather than claiming persistence. + +### 5. Verify finalization behavior + +Retain the existing finalization coverage for malformed/absent IDs and a +present active ID plus missing secondary ID. Add only the missing integration +cases: + +- differing valid cookie and active EC IDs are both tombstoned when both rows + exist; +- repeated finalization preserves existing tombstone generations; +- a failing KV graph still returns the response and emits both applicable + browser-cookie expiration headers. + +Production finalization should not change unless these tests expose a defect. + +## File Map + +### Modify + +- `crates/trusted-server-core/src/ec/kv.rs` + - Add the matching-tombstone no-op branch. + - Gate the stale-miss fallback with a same-store completion marker. + - Clear completion markers on same-key revival and hard deletion. + - Update withdrawal documentation. + - Add operation-count, repetition, stale-read, and concurrency tests. +- `crates/trusted-server-core/src/ec/finalize.rs` + - Add two-ID, repeated-withdrawal, and KV-failure integration coverage. + +### Add + +- `docs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.md` + - Record the reviewed design and verification contract. + +No dependency, configuration, adapter, JavaScript, or public wire-format change +is expected. The EC store gains an internal completion-marker key namespace. + +## Implementation Tasks + +### Task 1 — Establish failing idempotency tests + +- [x] Add a recording withdrawal backend that counts lookups and every insert + attempt and captures write mode/TTL. +- [x] Add a supplied-tombstone test proving zero lookups and zero inserts. +- [x] Add repeated and stale-parallel snapshot tests proving the first insert is + `IfGenerationMatch` with `TOMBSTONE_TTL`, then no further insert occurs and + generation/`consent.updated` remain unchanged. +- [x] Add live-generation, unavailable-generation, `NotRead`, failed, wrong-ID, + and missing state tests. +- [x] Run `cargo test-fastly tombstone_existing_from_snapshot` and confirm the + new repeated/no-backend tests fail before implementation. + +### Task 2 — Implement the no-op branch and guard the fallback + +- [x] Return a matching tombstone snapshot before generation lookup, + serialization, or write. +- [x] Keep live/missing/failed/mismatched/CAS behavior unchanged. +- [x] Record completion after successful tombstone writes. +- [x] Suppress repeated stale-miss overwrites when the completion marker exists. +- [x] Clear completion state on same-key revival and hard deletion. +- [x] Run focused KV tests until green. + +### Task 3 — Cover concurrent state changes + +- [x] Model another withdrawal winning between read and CAS; prove the loser + rereads and stops without replacing the tombstone. +- [x] Model a concurrent live update/re-consent winning before CAS; prove + withdrawal retries and tombstones the refreshed row. +- [x] Assert final tombstones contain no partner IDs. +- [x] Add direct `upsert_partner_id` tombstone rejection coverage and re-run the + existing conditional and snapshot-bulk rejection tests. + +### Task 4 — Verify finalization + +- [x] Add a test with both differing valid IDs present and assert both become + tombstones. +- [x] Retain the existing missing and invalid ID tests unchanged as regression + coverage. +- [x] Add repeated-finalization generation-stability coverage. +- [x] Add a failing-store test proving EC and marker cookie deletion survives KV + failure. +- [x] Run focused withdrawal/finalization tests. + +### Task 5 — Review and full verification + +- [x] Run independent correctness/concurrency and test-quality reviews. +- [x] Apply only fixes required by issue scope. +- [x] Mark this plan implemented only after all checks below pass. + +## Acceptance Mapping + +| Issue requirement | Planned evidence | +| ------------------------------------------------ | ----------------------------------------------------------------------------- | +| First withdrawal establishes a 24-hour tombstone | Live-entry CAS test and existing `TOMBSTONE_TTL` assertion | +| Repeated requests avoid overwrite writes | Present and stale-miss operation counts plus unchanged root generation/time | +| Concurrent withdrawal cannot restore IDs | Stale-snapshot and concurrent-live-update conflict tests | +| Both differing valid IDs are withdrawn | Finalization test with both rows seeded | +| Missing/unverified IDs create no root | Existing-key-only and invalid-ID tests | +| KV failure remains best-effort | Finalization response/cookie test with failing graph | +| Late partner updates cannot repopulate | Conditional, public single, and snapshot-bulk upsert rejection tests | +| Original TTL is not refreshed | First insert records `TOMBSTONE_TTL`; repetition records zero further inserts | + +## Verification Contract + +Run focused checks during implementation: + +```bash +cargo test-fastly tombstone_existing_from_snapshot +cargo test-fastly withdrawal +cargo test-fastly upsert_partner_id_if_exists_rejects_tombstone +cargo test-fastly upsert_partner_id_rejects_tombstone +cargo test-fastly snapshot_upsert_rejects_tombstone +``` + +Before committing and opening the draft PR, run: + +```bash +cargo fmt --all -- --check +cargo test-fastly +cargo test-axum +cargo test-cloudflare +cargo test-spin +cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity +cargo clippy-fastly +cargo clippy-axum +cargo clippy-cloudflare +cargo clippy-cloudflare-wasm +cargo clippy-spin-native +cargo clippy-spin-wasm +cd crates/trusted-server-js/lib && npx vitest run +cd crates/trusted-server-js/lib && npm run format +cd docs && npm run format +cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1 +git diff --check +``` + +## Definition of Done + +- The first live-row withdrawal writes one CAS-protected 24-hour tombstone. +- Repeated and concurrent withdrawals observing that tombstone perform no + replacement write and do not refresh its expiration. +- Repeated stale point-read misses use the completion marker and do not rewrite + the root tombstone. +- Missing IDs remain absent; unconditional overwrite remains confined to the + strongly confirmed stale-miss fallback. +- Concurrent live updates before successful withdrawal are tombstoned on retry. +- Later partner writes cannot repopulate tombstones. +- Both valid differing IDs are handled independently. +- Browser-cookie deletion remains independent of KV success. +- Focused tests, independent review, and every applicable repository gate pass. + +## Risks and Mitigations + +- **Snapshot binding:** The no-op branch must require the snapshot ID to match + the requested EC ID; a tombstone for another ID cannot suppress withdrawal. +- **Linearization:** Returning an observed tombstone linearizes withdrawal at + that observation. A later re-consent may legitimately win. +- **TTL visibility:** Record the first root and completion-marker TTLs and every + subsequent insert attempt at the wrapper boundary; stable root + generation/timestamp alone is not sufficient evidence. +- **Stale-miss completion:** Write the marker only after the root tombstone + succeeds. Marker failures must fall back to the privacy write, while revival + and hard deletion must remove stale completion state. +- **Fallback containment:** Keep unconditional overwrite behind strong root + existence and completion-marker checks so no other path can refresh a + completed tombstone. +- **Conflict-test realism:** Inject actual generation changes and persisted + state, not endless synthetic precondition failures. +- **Stack dependency:** Reconcile changes if PR #885 or draft PR #900 modifies + snapshot/finalization contracts before this stack lands.