From 9aa4f0fe9ba4dc780921baeafba85c3db54ea773 Mon Sep 17 00:00:00 2001 From: Christian Date: Mon, 13 Jul 2026 14:25:17 -0500 Subject: [PATCH] Make EC withdrawal tombstones idempotent --- crates/trusted-server-core/src/ec/finalize.rs | 145 ++++++ crates/trusted-server-core/src/ec/kv.rs | 428 +++++++++++++++--- ...ue-881-idempotent-withdrawal-tombstones.md | 319 +++++++++++++ 3 files changed, 837 insertions(+), 55 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.md diff --git a/crates/trusted-server-core/src/ec/finalize.rs b/crates/trusted-server-core/src/ec/finalize.rs index 33b8fbe2..3e0f4513 100644 --- a/crates/trusted-server-core/src/ec/finalize.rs +++ b/crates/trusted-server-core/src/ec/finalize.rs @@ -1011,6 +1011,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 76fcd7fb..667bd0d6 100644 --- a/crates/trusted-server-core/src/ec/kv.rs +++ b/crates/trusted-server-core/src/ec/kv.rs @@ -732,42 +732,6 @@ 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. - /// - /// The tombstone preserves consent enforcement for batch sync clients - /// (`POST /_ts/api/v1/batch-sync`) during the 24-hour revocation window. - /// - /// # 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. - pub fn write_withdrawal_tombstone( - &self, - ec_id: &str, - ) -> Result<(), Report> { - let entry = KvEntry::tombstone(current_timestamp()); - let (body, meta_str) = Self::serialize_entry(&entry, self.store_name())?; - - match self.write_entry( - ec_id, - &body, - &meta_str, - TOMBSTONE_TTL, - EcKvWriteMode::Overwrite, - ) { - Ok(_) => 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}'"), - })), - } - } - /// Writes a tombstone only when an authoritative row already exists. /// /// An authoritative `Missing` snapshot is a no-op (nothing to withdraw). A @@ -782,6 +746,11 @@ impl KvIdentityGraph { let mut current = snapshot; 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), @@ -937,8 +906,8 @@ impl KvIdentityGraph { /// Hard-deletes the entry. /// - /// 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 /// @@ -1062,6 +1031,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 // ----------------------------------------------------------------------- @@ -1077,6 +1057,7 @@ mod tests { inner: InMemoryEcKv, conflicts_remaining: std::sync::Mutex, revive_on_conflict: bool, + partner_update_on_conflict: bool, } impl ConflictInjectingEcKv { @@ -1085,6 +1066,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, } } @@ -1132,8 +1123,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"); @@ -1505,22 +1501,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"); @@ -1608,6 +1588,95 @@ mod 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, + } + + impl RecordingEcKv { + fn new(operations: Arc) -> Self { + Self { + inner: InMemoryEcKv::new("recording-store"), + operations, + } + } + } + + 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); + 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) + } + } + /// [`EcKvStore`] whose reads succeed but every write fails, simulating a /// store that becomes unwritable mid-request. struct WriteFailingEcKv { @@ -1868,6 +1937,212 @@ 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, + }], + "first withdrawal should perform one conditional tombstone write" + ); + 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_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::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(), + 1, + "live reread should write once" + ); + assert!( + outcome + .entry_for(&ec_id) + .is_some_and(|entry| !entry.consent.ok), + "reread live row should be tombstoned" + ); + } + } + #[test] fn tombstone_existing_from_snapshot_retries_cas_conflict() { let graph = KvIdentityGraph::new(ConflictInjectingEcKv::new(1, false)); @@ -1885,6 +2160,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/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 00000000..9dfc6db8 --- /dev/null +++ b/docs/superpowers/plans/2026-07-13-issue-881-idempotent-withdrawal-tombstones.md @@ -0,0 +1,319 @@ +# 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. A request that already has authoritative tombstone state +returns without reading or writing KV, so it cannot refresh 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 write. +- 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 cross-request lock, deduplication store, or withdrawal cookie. +- Do not restrict withdrawal handling to document navigations. +- Do not change the 24-hour tombstone duration or KV schema. +- Do not change EC generation, 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 older public `write_withdrawal_tombstone` unconditional-overwrite helper is +now dead in production but remains available and could bypass the conditional +path in future code. + +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 | +| Matching `Missing` | Return unchanged; never create | +| `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`. + +### 2. Remove the unconditional bypass + +Delete the now-unused public `write_withdrawal_tombstone` method and its obsolete +overwrite test. Update `KvIdentityGraph::delete` documentation so it describes +the snapshot-aware conditional withdrawal path without linking to the removed +API. + +Repository search must confirm there is no live Rust caller before removal. +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 exactly one `IfGenerationMatch` insert + with `TOMBSTONE_TTL`, while the second causes zero additional insert attempts + and leaves stored 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. + - Remove the unconditional overwrite helper. + - Update withdrawal documentation. + - Add operation-count, repetition, 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. + +## 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 remove the bypass + +- [x] Return a matching tombstone snapshot before generation lookup, + serialization, or write. +- [x] Keep live/missing/failed/mismatched/CAS behavior unchanged. +- [x] Remove `write_withdrawal_tombstone` and update the `delete` documentation. +- [x] Search for remaining Rust references to the removed helper. +- [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 | Operation counts plus unchanged generation and `consent.updated` | +| 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. +- Missing IDs remain absent; the unconditional overwrite API no longer exists. +- 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 insert's TTL and every subsequent insert + attempt at the wrapper boundary; stable generation/timestamp alone is not + sufficient evidence. +- **Dead API removal:** Compile and repository-search after deletion to catch any + hidden caller. +- **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.