Reduce EID KV write conflicts during page loads - #1157
ChristianPavilonis wants to merge 3 commits into
Conversation
ChristianPavilonis
changed the title
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Reviewed at a5aad79 against refactor/remove-legacy-consent-store (merge-base aee5bf6, no drift) in an isolated worktree. Read all six changed files end to end, and scratch-verified the two code suggestions below against the PR head.
The core design holds up. Replacing the five-attempt CAS loop with one conditional write plus one conflict read removes an unbounded write amplifier from the hot path, and the source classification is assigned after pre-route filters — the challenged-request tests prove a DataDome short circuit leaves the request unclassified. The /auction and navigation sources both writing the same cookie-derived updates means the loser of a concurrent page-load conflict lands on ConflictMatched rather than dropping data, which is the property that makes "never retry" safe.
No blocking findings. Approving. Ten comments below, ranked: one refactor I would land before merge, one dropped error, two behavioral changes worth an explicit confirmation, and the rest small.
Cross-cutting
Adapter scope is correct, and I checked. EcFinalizeState exists only in the Fastly adapter and ec_finalize_response has exactly one production call site (main.rs:308, via the popped extension), so there is no second router and no Cloudflare/Spin/Axum path silently losing returning-user EID sync. Worth stating because a set_eid_sync_source that only one adapter calls is the shape a parity gap usually takes.
POST /_ts/page-bids is no longer an EID sync source. The test this PR renamed used to name page-bids explicitly alongside /auction as a route that saves its first lookup with no later retry. Dropping it is consistent with the PR's goal and navigation covers the same page load, but the old comment made it a deliberate inclusion, so the removal reads as intentional-or-oversight from the diff alone. A line in the PR description would settle it.
Recovery path still receives its updates. The updates slice is now empty whenever the source is unclassified, and it is also what confirm_then_recover_orphaned_ec forwards into a recovered row. That is safe only because recovery_eligible implies a navigation, which always classifies — the same coupling the first comment is about. Worth keeping in mind if either gate moves.
CI Status
All 19 checks PASS, including every gate CLAUDE.md treats as a PR gate: cargo fmt, cargo test, cargo test (axum native), cargo test (cross-adapter parity), cargo test (ts CLI, native), spin native + wasm32-wasip1, cloudflare native + wasm32-unknown-unknown, integration tests, Fastly EC lifecycle, browser integration tests, vitest, format-typescript, format-docs, Analyze (rust), and CodeQL.
Independently re-ran on the PR head: cargo test -p trusted-server-core --lib ec:: → 382 passed, and app::tests → 41 passed.
| let is_publisher_navigation = is_navigation_request(&req); | ||
| if is_publisher_navigation { | ||
| ec.ec_context.set_eid_sync_source(EidSyncSource::Navigation); | ||
| } | ||
| if ec.is_real_browser | ||
| && is_publisher_navigation |
There was a problem hiding this comment.
♻️ refactor — is_publisher_navigation lost its ec.is_real_browser term so it can serve sync-source classification, but it is still the value passed to set_recovery_eligible at line 865, whose comment there still reads "Authorize orphan recovery only for real-browser document navigations". Bot navigations are now recovery-eligible.
This is inert today, but only through two invariants a long way from here: non-browsers get kv_graph = None (line 450), and the withdrawn-consent case that does keep a finalize KV graph returns from ec_finalize_response before the returning-user branch that reads recovery_eligible(). Nothing in the suite pins it either — no test asserts a non-browser navigation is recovery-ineligible.
Splitting the two concepts keeps the gate where the comment says it is, and the new classification test still passes because it deliberately does not depend on the browser gate.
| let is_publisher_navigation = is_navigation_request(&req); | |
| if is_publisher_navigation { | |
| ec.ec_context.set_eid_sync_source(EidSyncSource::Navigation); | |
| } | |
| if ec.is_real_browser | |
| && is_publisher_navigation | |
| let is_publisher_navigation = is_navigation_request(&req); | |
| if is_publisher_navigation { | |
| ec.ec_context.set_eid_sync_source(EidSyncSource::Navigation); | |
| } | |
| let is_browser_navigation = ec.is_real_browser && is_publisher_navigation; | |
| if is_browser_navigation |
Line 865 then becomes ec.ec_context.set_recovery_eligible(is_browser_navigation); — outside this diff, so it needs a manual edit alongside the suggestion.
Verified on the PR head in a scratch worktree: cargo fmt --all -- --check clean, cargo clippy-fastly clean, app::tests 41/41 pass.
| // Browser EID cookies carry no value-owned sequence or timestamp. | ||
| // A different value therefore has unknown freshness and must not | ||
| // replace the stored value. | ||
| Some(_) => { | ||
| deferred = true; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🤔 thinking — deferring a different value is the right call for concurrency, but the deferral is permanent rather than conflict-scoped, and that has a consequence worth stating explicitly.
Partner-owned paths still overwrite: pull sync goes through upsert_partner_ids_from_snapshot and partner push through upsert_partner_id_if_exists, both of which replace differing values. So any pull- or push-enabled partner self-heals after a rotation.
A cookie-only partner does not. _sharedid, or any Prebid userId module matched by the registry but not configured for pull/push, pins to the first UID observed for that row and keeps it for the row's whole TTL — and that stale UID keeps being attached to bid requests. Before this PR, the next request with a changed cookie corrected it.
Not asking for a fix here: inventing a staleness threshold in this PR would be a magic constant with no grounding. But a tracked follow-up for "cookie values need a freshness source" would be worth opening, and the trade-off is worth a line in the module docs so the next reader does not treat the stored UID as current.
| if let (Some(graph), Some(ec_id)) = (kv, ec_context.ec_value().map(str::to_owned)) { | ||
| let updates = collect_eid_cookie_updates(eids_cookie, sharedid_cookie, registry); | ||
| let snapshot = graph.upsert_partner_ids_from_snapshot( | ||
| &ec_id, | ||
| &updates, | ||
| ec_context.kv_snapshot().clone(), | ||
| ); | ||
| ec_context.set_kv_snapshot(snapshot); | ||
| let source = ec_context.eid_sync_source(); | ||
| let updates = source | ||
| .map(|_| collect_eid_cookie_updates(eids_cookie, sharedid_cookie, registry)) | ||
| .unwrap_or_default(); | ||
| if let Some(source) = source { | ||
| sync_eid_cookie_updates(graph, ec_context, &ec_id, &updates, source); | ||
| } |
There was a problem hiding this comment.
🤔 thinking — a side effect worth confirming as intended: this branch is where the returning-user KV read used to happen unconditionally, so it is also what fed reconcile_pull_sync_marker a real snapshot.
With subresources now skipping the sync entirely, their kv_snapshot() stays NotRead at finalization where it previously became Present/Missing. reconcile_marker treats NotRead as "no information" and only expires an already-Invalid marker, so KV-driven marker reconciliation is now navigation/auction-only.
Consent withdrawal is unaffected — it expires the marker on the independent early path in this function, on any request. The residual gap is a row that becomes pull-incomplete or missing: a Valid marker now survives on subresource responses until the next navigation or POST /auction. Given /auction fires on most page loads the practical window is small, but this is a disclosure-window signal, so worth an explicit yes/no rather than an inference.
| Err(_err) => { | ||
| log::warn!("EID cookie sync write failed"); |
There was a problem hiding this comment.
🔧 wrench — the store error is bound to _err and dropped, so a failed EID write is now undiagnosable: the log line is a fixed string with no cause, no store name, and no ec-id prefix. The path this replaced logged {err:?} alongside log_id(ec_id), so including the report is an established pattern here and does not widen identity exposure.
| Err(_err) => { | |
| log::warn!("EID cookie sync write failed"); | |
| Err(err) => { | |
| log::warn!("EID cookie sync write failed: {err:?}"); |
Verified on the PR head: cargo fmt --all -- --check and cargo clippy-fastly both clean with this applied.
| fn record_eid_sync_terminal(source: EidSyncSource, outcome: EidCookieSyncOutcome) { | ||
| let measurement = EidSyncMeasurement::new(source, outcome); | ||
| log::info!( | ||
| "EID sync measurement: source={} outcome={} attempted=1 already_matched={} written={} \ | ||
| conflict_duplicate={} deferred={}", | ||
| measurement.source, | ||
| measurement.outcome, | ||
| measurement.already_matched, | ||
| measurement.written, | ||
| measurement.conflict_duplicate, | ||
| measurement.deferred, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🤔 thinking — three things about this measurement, all pointing the same direction.
It cannot show the reduction it exists for. sync_eid_cookie_updates returns before record_eid_sync_terminal whenever updates.is_empty(), and the unclassified-source case never calls it at all. So the suppressed subresource syncs — the actual behavior this PR ships — emit nothing, and there is no denominator. A source=none / suppressed count, or a single line at the classification boundary, would make the write reduction observable in production instead of inferred.
EidSyncMeasurement is a one-hot re-encoding of outcome. All four u8 fields are pure functions of the single outcome value already being logged, and eid_sync_measurement_dimensions_are_bounded_and_identity_free mostly re-asserts that mapping rather than any behavior. If a metrics pipeline needs the fan-out it belongs there; if not, logging source and outcome alone carries the same information with no struct and no test to keep in sync. The Display bounds assertion in that test is genuinely valuable and worth keeping either way.
Level. This is info! on every navigation and every /auction that carries EID cookies, i.e. roughly per page load per user. debug! for AlreadyMatched/Written with info!/warn! reserved for the deferred and failed outcomes would keep the anomalies visible without the steady-state volume.
| if updates.is_empty() { | ||
| return (snapshot, EidCookieSyncOutcome::AlreadyMatched); | ||
| } |
There was a problem hiding this comment.
⛏ nitpick — unreachable, and misreporting if it were reached. Both callers pre-check for empty updates (upsert_partner_ids at line 568, sync_eid_cookie_updates in finalize.rs), and AlreadyMatched claims the stored values were verified to match when nothing was compared. Either drop the branch or debug_assert!(!updates.is_empty()) so the invariant is stated rather than papered over.
| fn partner_id_updates_match(entry: &KvEntry, updates: &[PartnerIdUpdate]) -> bool { | ||
| updates.iter().all(|update| { | ||
| entry | ||
| .ids | ||
| .get(&update.partner_id) | ||
| .is_some_and(|existing| existing.uid == update.uid) | ||
| }) | ||
| } |
There was a problem hiding this comment.
⛏ nitpick — asymmetry with apply_cookie_partner_id_updates directly above: that function collapses duplicate partner ids through a BTreeMap (last value wins), this one iterates the raw slice and requires every entry to match. Given ("p","a"), ("p","b") the write applies b and this check can never return true, turning a genuine ConflictMatched into DeferredConflict.
Inert in practice — collect_eid_cookie_updates runs dedupe_partner_updates — but sync_eid_cookie_updates_from_snapshot is pub(crate) and takes an arbitrary slice, so the dedupe is a caller-side invariant that is not stated anywhere. Collapsing here too (or documenting the precondition) makes the two halves agree.
|
|
||
| /// Bounded request classifications that may persist browser EID cookies. | ||
| /// | ||
| /// Adapters assign a source only after pre-route filters allow dispatch. |
There was a problem hiding this comment.
⛏ nitpick — "Adapters assign a source" is true for Navigation and Auction but not NewEc, which is assigned inside ec_finalize_response, not by any adapter. Worth naming since NewEc is a public variant that adapters specifically should not set.
| /// Adapters assign a source only after pre-route filters allow dispatch. | |
| /// A source is assigned only after pre-route filters allow dispatch: adapters | |
| /// classify navigation and auction requests, and `NewEc` is assigned during EC | |
| /// finalization. |
| /// Atomically merges browser partner IDs into an existing entry. | ||
| /// | ||
| /// Uses one read-modify-write operation for all updates so request-local | ||
| /// EID cookie ingestion does not perform a KV read per matched partner. | ||
| /// Duplicate partner IDs are collapsed with the last value winning. | ||
| /// This compatibility entry point has no request-start observation, so it | ||
| /// can add missing IDs but cannot replace different values. It performs at | ||
| /// most one conditional write and one follow-up read on conflict. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns [`TrustedServerError::KvStore`] on store error, missing root | ||
| /// entry, withdrawn root entry, or CAS exhaustion after | ||
| /// [`MAX_CAS_RETRIES`] attempts. | ||
| /// Returns [`TrustedServerError::KvStore`] on store failure, a missing root, | ||
| /// or a withdrawn root. | ||
| pub(crate) fn upsert_partner_ids( |
There was a problem hiding this comment.
🌱 seedling — worth a follow-up rather than this PR: after the rewrite this "compatibility entry point" has no production callers left.
Its only caller is ingest_eid_cookies_with_writer, reached from ingest_prebid_eids and ingest_sharedid_cookie, and neither of those is called anywhere outside prebid_eids.rs itself. upsert_partner_id (singular) is likewise tests-only now. So upsert_partner_ids, the PartnerIdBulkWriter trait, and both pub fn ingest_* entry points exist to keep a Result-shaped API alive for tests, while carrying real cost: they are a second way to reach the cookie-sync semantics whose error mapping has to be maintained in parallel.
If it is genuinely reachable through something I missed, a doc line naming that caller would help. Otherwise deleting the chain and porting its tests onto sync_eid_cookie_updates_from_snapshot would remove the duplicate surface.
| /// Store that replaces the row during the first EID CAS write and records | ||
| /// the request's reads and conditional writes. | ||
| struct EidConflictEcKv { |
There was a problem hiding this comment.
👍 praise — this is the right test double for the claim being made. A PR whose headline is "one conditional write, one follow-up read" needs the read and write counts asserted, not just the final row state, and counting them in the store is the only place that can prove it. with_follow_up_miss covering the conflict-then-vanish case, and KvEntry::tombstone proving a concurrent withdrawal survives a conflict, are both cases that would have been easy to leave untested.
Two other things worth calling out while I am here: dropping log_id(ec_id) from the upsert_partner_ids error strings tightens identity exposure in logs, and gating collect_eid_cookie_updates behind the source check means subresources skip the base64 decode entirely rather than decoding and discarding.
a5aad79 to
c225900
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Reviewed head c225900 against base 2c5ba81 (refactor/remove-legacy-consent-store; the merge base is the base tip, so the PR is current) in an isolated worktree, with runtime probes against the head. The one-write-one-read conflict path, the source gating, and the identity-free measurement are well built and well tested. Requesting changes on one question about scope: /_ts/page-bids silently stops persisting returning-user EIDs, which matters for SPA publishers. Everything else is non-blocking. No repository files were modified by this review.
3 of the inline comments below carry a one-click
suggestion. Each was applied alone to a scratch worktree at this head and clearedcargo fmt --all -- --check, the targeted tests (the kv module for the twokv.rssuggestions, the Fastly dispatch and recovery tests under Viceroy for theapp.rsone), and a byte-exact pre/post-verification patch check. All three together also clearedclippy-fastly,clippy-cloudflare,clippy-spin-native,test-fastly(2704 core tests plus the adapter suites),test-cloudflareandtest-spin. The axum, parity, and wasm clippy lanes could not run on the review machine (localaws-lc-sys/cctoolchain failures unrelated to the code), so those lanes are not locally verified for the suggestions.
Blocking
❓ question
/_ts/page-bidsno longer persists returning-user EID cookies — see inline atcrates/trusted-server-adapter-fastly/src/app.rs:648
Non-blocking
♻️ refactor
- Orphan-recovery eligibility lost its browser gate (suggestion) — see inline at
crates/trusted-server-adapter-fastly/src/app.rs:821 - A conflict follow-up miss returns the generation the CAS just rejected (suggestion) — see inline at
crates/trusted-server-core/src/ec/kv.rs:697 - The write-failure log drops the error (suggestion) — see inline at
crates/trusted-server-core/src/ec/kv.rs:711
🏕 camp site / ⛏ nitpick
upsert_partner_idsand theingest_*wrappers are dead code re-implemented with new semantics — see inline atcrates/trusted-server-core/src/ec/kv.rs:562outcome=missingalso counts rows the request proved exist — see inline atcrates/trusted-server-core/src/ec/kv.rs:637
Cross-cutting / body-level findings
- 🤔 Deferral is permanent for browser-owned values. #993 scopes "do not overwrite a different value" to a request that lost a CAS conflict. This PR applies it to every write:
apply_cookie_partner_id_updatesdefers any different value even when no conflict occurred (DeferredFreshness,WrittenWithDeferredFreshness). A partner cookie that legitimately changes (a regeneratedsharedId, a user-ID module re-issuing an ID) can therefore never replace the stored value through this path, and the stale stored value keeps being forwarded asuser.ext.eidsnext to the new one from the auction payload. "Deferred to a later navigation or auction" means "never" until a freshness rule exists, because every later request sees the same different value. The PR body states the conservative choice, which is fine, but it is worth saying explicitly that deferred values do not converge on their own. - 📌 Follow-up: the freshness rule. #993's "Prevent older values from replacing newer ones" already sketches storing the time each partner ID was observed. That follow-up is now what turns "deferred" back into "eventually persisted"; suggest filing it before this merges so the permanent-deferral window is tracked.
- 📝 Docs and spec drift.
docs/superpowers/specs/2026-07-10-kv-eid-request-snapshot-ec-recovery-design.mdstill describes the cookie path as "rereads only on CAS conflict … an exhausted CAS retry returns a failed snapshot" and lists the invariant "CAS conflicts re-merge rather than overwrite concurrent data"; neither holds forsync_eid_cookie_updates_from_snapshotany more (one write, no re-merge, different values deferred).docs/guide/edge-cookies.md's sequence ("Next request with ts-eids → Decode cookie and upsert matched partner UIDs") now holds only for document navigations andPOST /auction. A short addendum, or a spec for #993, would keep the authority the code cites accurate.docs/guide/integrations/prebid.mditem 5 ("still ingested after the response") stays true for/auction. - 🌱 Measurement channel. The measurement is one
log::info!line per navigation, auction, and new EC. Fine for the first read-out, but the counters #993 asks for (attempts, matches, writes, duplicate conflicts, deferrals, by source) are aggregate questions; the auction telemetry pipeline already carries per-request dimensions to the warehouse, so consider emitting there once the log-based read-out has confirmed the shape.
CI Status
All 19 reported checks PASS at this head. Required checks are marked.
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (javascript-typescript): PASS
- Analyze (rust): PASS
- CodeQL: PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
| NamedRouteHandler::SetTester => handle_set_tester(&state.settings), | ||
| NamedRouteHandler::ClearTester => handle_clear_tester(&state.settings), | ||
| NamedRouteHandler::Auction => { | ||
| ec.ec_context.set_eid_sync_source(EidSyncSource::Auction); |
There was a problem hiding this comment.
❓ question — /_ts/page-bids no longer persists returning-user EID cookies.
Before this PR every finalized response that had a KV graph ran the returning-user upsert, and the test comment this PR deletes named /auction and /_ts/page-bids together as the named routes that "save their first lookup into the context". The PageBids arm below (line 666) sets no source, so for a returning user ec_finalize_response now skips sync_eid_cookie_updates there entirely.
On SPA publishers (the SSR + page-bids setup) a route change never navigates and never calls POST /auction; page-bids is the only per-view request that carries the browser's current ts-eids / sharedId. With this PR those users' IDs sync only on hard reloads.
Is the narrowing to navigation + /auction deliberate (it matches #993's list verbatim), or was page-bids overlooked? If deliberate, the PR body and the edge-cookies guide should say so. If not:
NamedRouteHandler::PageBids => {
ec.ec_context.set_eid_sync_source(EidSyncSource::Auction);or a dedicated EidSyncSource::PageBids variant so the measurement can tell the two apart. Apply manually — the PageBids arm is outside the diff hunks, so this cannot be a suggestion.
| let is_publisher_navigation = is_navigation_request(&req); | ||
| if is_publisher_navigation { | ||
| ec.ec_context.set_eid_sync_source(EidSyncSource::Navigation); | ||
| } | ||
| if ec.is_real_browser | ||
| && is_publisher_navigation |
There was a problem hiding this comment.
♻️ refactor — Orphan-recovery eligibility lost its browser gate.
is_publisher_navigation used to mean real browser and document navigation. It still feeds set_recovery_eligible at line 851, under the comment "Authorize orphan recovery only for real-browser document navigations", but it is now just is_navigation_request(&req), so a non-browser navigation whose origin start succeeds becomes recovery-eligible.
Unreachable today: build_ec_request_state withholds the finalize KV graph from non-browser requests (except withdrawals, which return before the returning-user branch), so recovery cannot run. It is defense in depth, and the comment is now wrong. Keeping the source classification browser-independent (as your test asserts) while restoring the gate on the recovery flag:
| let is_publisher_navigation = is_navigation_request(&req); | |
| if is_publisher_navigation { | |
| ec.ec_context.set_eid_sync_source(EidSyncSource::Navigation); | |
| } | |
| if ec.is_real_browser | |
| && is_publisher_navigation | |
| let is_navigation = is_navigation_request(&req); | |
| if is_navigation { | |
| ec.ec_context.set_eid_sync_source(EidSyncSource::Navigation); | |
| } | |
| let is_publisher_navigation = ec.is_real_browser && is_navigation; | |
| if is_publisher_navigation |
No unit test can pin this: under Viceroy the publisher origin never connects, so set_recovery_eligible is never reached in tests (a probe asserting the flag on a browser-less navigation passes on this head for that reason alone). Verified in isolation on a scratch worktree at this head: cargo fmt --all -- --check clean, the dispatch and recovery-eligibility tests pass under Viceroy, no post-verification drift.
| let Some(refreshed_entry) = refreshed.entry_for(ec_id) else { | ||
| let kept = Self::keep_proven(ec_id, refreshed, Some(¤t)); | ||
| return (kept, EidCookieSyncOutcome::DeferredConflict); | ||
| }; |
There was a problem hiding this comment.
♻️ refactor — A conflict whose follow-up read misses returns a snapshot carrying the generation the CAS just rejected.
Self::keep_proven(ec_id, refreshed, Some(¤t)) hands back current unchanged, and current is Present { generation: Some(g) } where g is exactly the generation the conditional write failed on. Runtime-confirmed: adding
assert_eq!(snapshot.generation_for(&ec_id), None);to eid_cookie_sync_keeps_live_proof_when_conflict_follow_up_misses fails on this head.
Harmless today, since pull sync re-reads the live row before its bulk CAS and withdrawal runs earlier in finalize. But upsert_partner_ids_from_snapshot treats Present { generation: Some } as CAS input it may use without a read, so a snapshot that leaves this function should never advertise a generation known to be stale. Keep the existence proof, drop the generation:
| let Some(refreshed_entry) = refreshed.entry_for(ec_id) else { | |
| let kept = Self::keep_proven(ec_id, refreshed, Some(¤t)); | |
| return (kept, EidCookieSyncOutcome::DeferredConflict); | |
| }; | |
| let Some(refreshed_entry) = refreshed.entry_for(ec_id) else { | |
| // The failed CAS proved `current`'s generation stale; keep | |
| // only its existence proof so no later write reuses it. | |
| let proof = match current { | |
| EcKvSnapshot::Present { | |
| ec_id: proven_id, | |
| entry, | |
| .. | |
| } => EcKvSnapshot::Present { | |
| ec_id: proven_id, | |
| entry, | |
| generation: None, | |
| }, | |
| other => other, | |
| }; | |
| let kept = Self::keep_proven(ec_id, refreshed, Some(&proof)); | |
| return (kept, EidCookieSyncOutcome::DeferredConflict); | |
| }; |
Worth adding the assertion above to that test as well (it depends on this change, so it is not posted as its own suggestion). Verified in isolation on a scratch worktree at this head: cargo fmt --all -- --check clean, the 120 ec::kv tests pass and the probe assertion passes with the change, no post-verification drift.
| Err(_err) => { | ||
| log::warn!("EID cookie sync write failed"); |
There was a problem hiding this comment.
♻️ refactor — The write-failure log drops the error.
Every other store failure in this module logs the report (load_snapshot, the snapshot upsert, pull sync), and a_store_error_never_carries_the_whole_identifier already pins that a report never contains the full EC ID, so the identity-free logging goal does not require dropping the cause. As written, a KV outage on this path shows up as a bare "write failed" with no status or reason to correlate with the platform.
| Err(_err) => { | |
| log::warn!("EID cookie sync write failed"); | |
| Err(err) => { | |
| log::warn!("EID cookie sync write failed: {err:?}"); |
Verified in isolation on a scratch worktree at this head: cargo fmt --all -- --check clean, the 120 ec::kv tests pass, no post-verification drift.
| /// [`MAX_CAS_RETRIES`] attempts. | ||
| /// Returns [`TrustedServerError::KvStore`] on store failure, a missing root, | ||
| /// or a withdrawn root. | ||
| pub(crate) fn upsert_partner_ids( |
There was a problem hiding this comment.
🏕 camp site — upsert_partner_ids and the ingest_* wrappers are dead code re-implemented with new semantics.
ingest_eid_cookies, ingest_prebid_eids and ingest_sharedid_cookie in prebid_eids.rs have no callers anywhere in the workspace (finalize uses collect_eid_cookie_updates plus the snapshot path; the only other mentions are a comment in admin.rs and the old SSC spec). This method exists only for them and for the PartnerIdBulkWriter test seam. The PR rebuilt it on sync_eid_cookie_updates_from_snapshot and now returns Ok(()) for DeferredConflict and DeferredFreshness, reporting success when nothing was written, and its Failed arm rebuilds a kv_error without the underlying report.
Deleting the three wrappers, the trait, this method and their tests is smaller than maintaining a compat mapping whose semantics now differ from the name. Apply manually — spans kv.rs and prebid_eids.rs.
| EcKvSnapshot::Missing { .. } => { | ||
| let kept = Self::keep_proven(ec_id, current, proven.as_ref()); | ||
| return (kept, EidCookieSyncOutcome::Missing); |
There was a problem hiding this comment.
⛏ nitpick — outcome=missing also counts rows the request already proved exist.
When the request-start snapshot was Present for this EC ID but the refresh here misses (edge KV point reads are eventually consistent), keep_proven returns the proof and the row is not treated as missing anywhere else, yet the outcome reported to the measurement is Missing. The "missing" counter #993 asks for therefore mixes genuinely absent rows with stale reads on live rows, and for NewEc the read right after create_if_absent is exactly where a stale miss is most likely.
A distinct outcome (say DeferredStaleRead), or at least mapping this case to deferred=1 in EidSyncMeasurement, would keep the read-out honest. Prose only — it needs a new variant, its Display, and the measurement mapping in finalize.rs.
c896537 to
1cc78b3
Compare
3 participants
Summary
POST /auction; new EC creation remains eligible.This PR is stacked on #903.
Changes
Closes
Closes #993
Test plan
cargo fmt --all -- --checkcargo test-fastlycargo test-axumcargo test-cloudflarecargo test-spincd crates/trusted-server-js/lib && npm run format && npm run lint && npm testcd docs && npm run format && npm run lint && npm run buildChecklist
CLAUDE.mdconventionsunwrap()added in production codelogmacros rather thanprintln!