Reduce EID KV write conflicts during page loads - #1157
ChristianPavilonis wants to merge 3 commits into
Conversation
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.
a5aad79 to
c225900
Compare
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
c896537 to
1cc78b3
Compare
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!