Skip to content

Reduce EID KV write conflicts during page loads - #1157

Open
ChristianPavilonis wants to merge 3 commits into
refactor/remove-legacy-consent-storefrom
fix/issue-993-eid-kv-conflicts
Open

ChristianPavilonis wants to merge 3 commits into
refactor/remove-legacy-consent-storefrom
fix/issue-993-eid-kv-conflicts

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Limit returning-user EID cookie persistence to publisher document navigations and POST /auction; new EC creation remains eligible.
  • Replace immediate five-write CAS retry loops with one conditional write and one follow-up read.
  • Preserve consent withdrawal and defer differing browser-cookie values when trustworthy value freshness is unavailable.

This PR is stacked on #903.

Changes

Area Change
Fastly routing Classify navigation and auction requests as allowed EID sync sources; static, analytics, integration, and other subresources remain ineligible.
EC finalization Keep consent and cookie handling before the EID sync boundary, while collecting cookie updates only for eligible sources.
KV persistence Add missing IDs with one CAS write; after conflict, reread once and report matched, deferred, withdrawn, missing, or failed outcomes without retrying. Different existing values are conservatively preserved because the cookies have no value-owned version.
Observability Emit bounded, identity-free fields for attempts, matches, writes, duplicate conflicts, deferrals, outcomes, and source.
Tests Cover route classification, new/returning EC behavior, one-write conflict convergence, concurrent withdrawal, conservative freshness, and measurement dimensions.

Closes

Closes #993

Test plan

  • cargo fmt --all -- --check
  • cargo test-fastly
  • cargo test-axum
  • cargo test-cloudflare
  • cargo test-spin
  • Cross-adapter parity tests
  • Fastly, Axum, Cloudflare, Cloudflare Wasm, Spin native, and Spin Wasm Clippy targets
  • cd crates/trusted-server-js/lib && npm run format && npm run lint && npm test
  • cd docs && npm run format && npm run lint && npm run build
  • Fastly and Spin release Wasm builds

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() added in production code
  • Uses log macros rather than println!
  • New behavior has regression coverage
  • No secrets, EC IDs, or partner ID values are emitted by new measurements

@ChristianPavilonis
ChristianPavilonis added this pull request to stack #1156 September 9, 2026 18:00
@ChristianPavilonis ChristianPavilonis changed the title fix/issue 993 eid kv conflicts Reduce EID KV write conflicts during page loads Sep 9, 2026

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/trusted-server-adapter-fastly/src/app.rs Outdated
Comment thread crates/trusted-server-core/src/ec/kv.rs
Comment thread crates/trusted-server-core/src/ec/finalize.rs
Comment thread crates/trusted-server-core/src/ec/kv.rs Outdated
Comment thread crates/trusted-server-core/src/ec/finalize.rs
Comment thread crates/trusted-server-core/src/ec/kv.rs
Comment thread crates/trusted-server-core/src/ec/kv.rs
Comment thread crates/trusted-server-core/src/ec/mod.rs Outdated
Comment thread crates/trusted-server-core/src/ec/kv.rs Outdated
Comment thread crates/trusted-server-core/src/ec/kv.rs

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cleared cargo fmt --all -- --check, the targeted tests (the kv module for the two kv.rs suggestions, the Fastly dispatch and recovery tests under Viceroy for the app.rs one), and a byte-exact pre/post-verification patch check. All three together also cleared clippy-fastly, clippy-cloudflare, clippy-spin-native, test-fastly (2704 core tests plus the adapter suites), test-cloudflare and test-spin. The axum, parity, and wasm clippy lanes could not run on the review machine (local aws-lc-sys / cc toolchain failures unrelated to the code), so those lanes are not locally verified for the suggestions.

Blocking

❓ question

  • /_ts/page-bids no longer persists returning-user EID cookies — see inline at crates/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_ids and the ingest_* wrappers are dead code re-implemented with new semantics — see inline at crates/trusted-server-core/src/ec/kv.rs:562
  • outcome=missing also counts rows the request proved exist — see inline at crates/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_updates defers any different value even when no conflict occurred (DeferredFreshness, WrittenWithDeferredFreshness). A partner cookie that legitimately changes (a regenerated sharedId, 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 as user.ext.eids next 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.md still 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 for sync_eid_cookie_updates_from_snapshot any 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 and POST /auction. A short addendum, or a spec for #993, would keep the authority the code cites accurate. docs/guide/integrations/prebid.md item 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

Comment thread crates/trusted-server-adapter-fastly/src/app.rs
Comment thread crates/trusted-server-adapter-fastly/src/app.rs Outdated
Comment thread crates/trusted-server-core/src/ec/kv.rs
Comment thread crates/trusted-server-core/src/ec/kv.rs Outdated
Comment thread crates/trusted-server-core/src/ec/kv.rs Outdated
Comment thread crates/trusted-server-core/src/ec/kv.rs Outdated
@ChristianPavilonis
ChristianPavilonis force-pushed the fix/issue-993-eid-kv-conflicts branch from c896537 to 1cc78b3 Compare September 11, 2026 14:59
@aram356 aram356 added this to the 202609 milestone Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce EID KV write conflicts during page loads

3 participants