Thread EC KV read through the request and recover orphaned cookies - #885
Thread EC KV read through the request and recover orphaned cookies#885prk-Jr wants to merge 22 commits into
Conversation
Harden the request-scoped EC KV snapshot work (#851) with the test coverage the spec's test strategy called for, plus small cleanups the plan requested. Tests: - ec::kv: snapshot upsert (write-without-read, unchanged-preserves-gen, refresh-once, CAS re-merge, tombstone-rejects, store-fail) and conditional tombstone (CAS conflict, store failure, disappear-on-retry) - ec::mod: generate_if_needed collision retry + exhaustion; default and read-path recovery-ineligibility (non-Fastly adapter contract) - ec::prebid_eids: collect_eid_cookie_updates merge and empty-registry - ec::finalize: NotRead/Failed/tombstone/subresource no-rotate paths and two-ID existing-only withdrawal - ec::pull_sync: request-wide aggregation into one bulk write across concurrency batches, plus no-dispatch for non-present snapshots - publisher: concurrent-vs-eager origin scheduling order and origin-start failure, using recording HTTP/KV collaborators Cleanups: - Extract rewrite_origin_request to remove the duplicated origin-rewrite logic across the concurrent and eager paths - Bind the orphaned EC ID once in recover_orphaned_ec Docs: - Note the fastly-ssl vendor-header layering wart in core scheme detection and the origin-forwarding strip (comments only; behavior unchanged)
A Failed request-scoped snapshot no longer short-circuits tombstone_existing_from_snapshot. A transient read error earlier in the request must not silently drop a consent withdrawal, so a non-authoritative snapshot is re-read (bounded by MAX_CAS_RETRIES) and the row is tombstoned when present. An authoritative Missing snapshot stays a no-op.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Changes requested. The snapshot/recovery design is promising, but I found two high-severity identity/privacy issues and three medium correctness, latency, and resource-usage issues. Details are attached inline.
Address PR review findings: - Gate orphan-recovery eligibility to the publisher fallback after a successful origin start. Named routes, integration proxies, and request-filter short circuits no longer reach EC finalization with recovery authorized, so a blocked or non-publisher response cannot rotate an identity. - Never downgrade an in-request Add-confirmed Present snapshot on a preload refresh miss, and confirm an authoritative miss with a second read (after the origin round trip) before rotating. A single eventually-consistent edge miss can no longer rotate a valid identity; a now-visible row is adopted instead. - Preload the origin-overlapped snapshot with the unfiltered active EC ID while keeping the consent-filtered ID for auction identity, so consent-withdrawn navigations keep the withdrawal CAS off the post-origin latency path. - Resolve the initial usable snapshot outside the CAS retry counter in both partner upsert and conditional tombstoning, so a generation-unavailable or refreshed snapshot keeps all five write attempts. - Defer /auction and page-bids identity-graph reads until a live auction actually runs with a partner registry, avoiding billable KV reads that cannot be consumed. Add tests for four-conflicts-then-fifth-write CAS, transient Add->Missing->Present confirmation, and recovery-eligibility lifecycle across named routes, filter short circuits, and origin-start failures.
…apshot-ec-ttl # Conflicts: # crates/trusted-server-adapter-fastly/src/app.rs # crates/trusted-server-core/src/auction/endpoints.rs # crates/trusted-server-core/src/ec/finalize.rs # crates/trusted-server-core/src/ec/prebid_eids.rs # crates/trusted-server-core/src/ec/pull_sync.rs # crates/trusted-server-core/src/http_util.rs
|
@prk-Jr Please confirm that is still issue with SSAT and ESI |
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Request changes. I found three P1 identity/consent races and two P2 snapshot-contract regressions in the request-scoped EC snapshot implementation. All findings are inline.
Point lookups on edge data stores are eventually consistent, so `Ok(None)` cannot prove a key is absent. Three identity and consent paths treated a single stale read as authoritative. Fastly's KV list API reads the primary data source unless `eventual_consistency()` is requested, and `EcKvStore::count_keys_with_prefix` already rides that path on every adapter. `KvIdentityGraph::key_exists_confirmed` wraps it as a consistency-safe existence check, used wherever absence has real consequences. Withdrawal tombstones: a preloaded `Missing` no longer short-circuits `tombstone_existing_from_snapshot`. It is re-read — on the publisher path the re-read is separated from the preload by the full origin round trip — and a second miss is escalated to the existence check. A proven-absent key stays a no-op so forged cookies still mint nothing; a key the store still lists is tombstoned unconditionally, because no CAS generation survives a missed read and a withdrawal must win; a failed check leaves the withdrawal unresolved rather than silently dropped. EID ingestion: `upsert_partner_ids_from_snapshot` carries the existence proof held by an incoming `Present` snapshot — an `Add`-confirmed create or an earlier authoritative read. A refresh that misses or fails skips the update and logs it instead of returning `Missing`/`Failed`, so a transient miss can no longer suppress the `ts-ec` cookie for a root that was written. Write failures still report `Failed`: those are operation failures, not ambiguous reads. Post-send pull sync: dispatch was authorized entirely by the request snapshot, so a CMP withdrawal completing after the page response was flushed could still send the raw `ec_id` to partners. The live row is now re-read immediately before the first partner request and anything short of a live, consenting entry cancels the dispatch. The read sits behind the snapshot eligibility filter, so a dispatch with nothing to pull still costs no KV operation, and the fresh snapshot feeds the write-back instead of the stale one. Orphan recovery: rotation abandons a year-lived identity graph, so a second point-read miss is no longer enough. It now requires proven absence; a key the store still lists, or an existence check that fails, leaves the identity intact and retries recovery on a later navigation.
`/auction` and `/_ts/page-bids` loaded the identity-graph row into a local snapshot to resolve server-side EIDs, then dropped it. The Fastly adapter passed an unchanged `EcContext` into response finalization, which therefore saw `NotRead` and paid for a second billable lookup whenever `ts-eids` or `sharedId` carried an update — working against this PR's read-reduction goal. Both handlers now take `&mut EcContext` and store the loaded snapshot before returning, so finalization ingests EID updates from the read the endpoint already performed. `ec_id` and the consent context are taken by value in both handlers so the store-back does not conflict with a live borrow. The other adapters own their `EcContext` locally and only needed the binding made mutable. Adds a counting-KV test asserting `/auction` plus finalization performs exactly one lookup and still ingests the `sharedId` update.
The single conflict is in `handle_publisher_request`, where both sides changed what happens between the auction dispatch and the origin fetch. This branch moved origin-request preparation above the auction dispatch so the origin fetch overlaps the EC identity-graph read. Main (#1008) added the shared template cache in the space that move vacated: request-derived cacheability flags, the pre-fetch cache key, and a `lookup_or_reserve` whose hit returns `AssembleTemplate` without ever contacting the origin. Resolution splits main's block along the line the move drew. Everything that reads the request — `request_had_authorization`, `cookie_disqualifies`, the DataDome suppression flags, reader-compression negotiation, `request_can_use_shared_template`, and the cache key itself — moves up beside this branch's origin preparation, because that is now the last point where the request is in hand. `configured_assembly_mode` moves with them since the key depends on it. The URI and Host rewrite stays folded into this branch's `rewrite_origin_request` helper rather than being reinstated inline. The lookup and its hit handling stay where main put them: they need `dispatched_auction`, which the hit hands to the assembler. The two features then contend over one request. A template hit skips the origin entirely, and an in-flight request cannot be recalled, so the concurrent origin start is gated on `template_cache_key.is_none()`. A request eligible for a shared template waits for the lookup instead of speculatively fetching an origin response a hit would discard. Every other request — including all traffic in the default `Inline` assembly mode, where the key is always `None` — keeps the overlap unchanged. The cost is one lost overlap on an ESI-mode cache miss, which is the correct trade against paying for a discarded fetch on every hit. Test fallout from main, resolved in kind rather than by reverting either side: - `handle_publisher_request` gained `edge_header: EdgeCacheHeader` (#860); this branch's three scheduling-probe call sites now pass `SurrogateControl`. - Main renamed the `private, no-store` cache-control value to `no-store, private`. The probe now asserts against `cache_policy::NO_STORE_PRIVATE_CACHE_CONTROL` so it cannot drift again. - Main removed the `queue_cacheable_html_response` fixture along with its own callers. This branch's surviving caller moves to main's replacement, `queue_html_response_with_cache_control`, instead of reviving a duplicate. - `PublisherResponse::AssembleTemplate` is new; the probe's response matches cover it. - Main added an `/auction` test that predates this branch's `&mut EcContext` signature. Verified: fmt, all six clippy gates, core (2269), fastly under Viceroy, axum, cloudflare, spin, parity, CLI, openrtb-codegen, JS (888), and both `scripts/template-cache-local-test.sh` modes — esi 21/21 including a warm hit that skips the origin, inline 8/8.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed 7b71f83331a5506e078537c749d283ff7fc9324a against 616b74051a4ec4147160d882ae8ac77bfe6785dd. Two medium-severity findings are included inline. All reported CI checks pass, and the existing review threads were checked to avoid duplicate feedback.
A `Missing` snapshot recorded earlier in the request short-circuited `upsert_partner_ids_from_snapshot`, so a stale point read on the eventually-consistent identity graph dropped the request's collected partner IDs. `/auction` and `/_ts/page-bids` save their first lookup into `EcContext` and are never recovery eligible, so those routes had no later chance to retry and a live row could miss a newly collected `ts-eids` or `sharedId` value. Revalidate a matching `Missing` once before the updates are dropped; a refresh that still misses keeps the no-create behavior, and an already-failed lookup is still not retried on the hot path. Withdrawal tombstones that exhausted every CAS attempt returned `EcKvSnapshot::Failed` with no log, and finalization discarded the outcome entirely for a non-active cookie ID. The browser cookie is cleared while the KV row can stay live with consent granted, so operators had no signal that withdrawal enforcement failed. Log CAS exhaustion for both the tombstone and the snapshot partner upsert, and warn on every failed withdrawal outcome in finalization.
`main` replaced the template cache's plain `Authorization` presence check with `authorization_disqualifies`, which lets a single unchanged edge-terminated value stay shareable and rejects unmarked, replaced, or repeated values. Every downstream use merged cleanly to the new name, so only the definition conflicted. Take main's gate and keep this branch's rationale for computing it here: the origin send below may start before the auction is dispatched, so this is the last point where the request is still in hand.
|
No objection to the change itself. One sequencing note before it lands. This PR and the provider stack in #1043–#1047 and #1094 both change the Edge Cookie path and overlap on If the stack can't be merged quickly and #885 needs to go first, the alternative we put to Rowena, Jason and Shailley is a release branch model, keeping the current live and next release branches in sync so neither blocks the other. Either order works for us. Flagging it now so whoever merges first knows the other is coming. |
|
Sequencing note on this PR and the provider stack. This PR and the open provider stack (#1043 to #1047, #1084, #1094) rework the same Edge Cookie code. A merge simulation of this PR's head (d414740) against the stack's tip (35584db) conflicts in 8 files, and against #1043 alone, the first PR of the stack, in 6 files, so whichever lands second re-resolves this by hand across the chain:
Reproduce: The request is the one we have made on #940 and #1094. The stack has been open since 19 August, carrying work that has been under review since 2 July as #838, is green on required CI, and its branches are kept rebased close to |
| } | ||
|
|
||
| log::warn!( | ||
| "snapshot partner upsert for '{}': CAS conflict after {MAX_CAS_RETRIES} retries; {} partner updates were not persisted", |
There was a problem hiding this comment.
⛏ nitpick — This string literal contains a run of spaces where a \ line-continuation was evidently intended, so the emitted warning reads …retries; 3 partner updates were not persisted. Neighboring multi-line log messages in this file use the \ continuation.
| "snapshot partner upsert for '{}': CAS conflict after {MAX_CAS_RETRIES} retries; {} partner updates were not persisted", | |
| "snapshot partner upsert for '{}': CAS conflict after {MAX_CAS_RETRIES} retries; \ | |
| {} partner updates were not persisted", |
(compile-verified: cargo fmt --all -- --check and cargo clippy-fastly pass with this applied — pure log-output change)
| // live with consent granted while the browser cookie is cleared. That | ||
| // divergence is only visible to operators if it is logged here. | ||
| log::warn!( | ||
| "withdrawal tombstone for '{}': CAS conflict after {MAX_CAS_RETRIES} retries; the identity-graph row may still be live with consent granted", |
There was a problem hiding this comment.
⛏ nitpick — Same embedded-spaces defect as the partner-upsert warning above. This is the operator-facing message the adjacent comment says exists precisely so the cookie/KV divergence is visible — worth keeping it legible.
| "withdrawal tombstone for '{}': CAS conflict after {MAX_CAS_RETRIES} retries; the identity-graph row may still be live with consent granted", | |
| "withdrawal tombstone for '{}': CAS conflict after {MAX_CAS_RETRIES} retries; the \ | |
| identity-graph row may still be live with consent granted", |
(compile-verified: cargo fmt --all -- --check and cargo clippy-fastly pass with this applied — pure log-output change)
| let Some(request_entry) = context.snapshot.entry_for(context.ec_id()) else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🤔 thinking — This exit narrows pull sync relative to main, and it does so silently. On main, dispatch_pull_sync read KV itself, so any real-browser request (subresources included) could dispatch. Now dispatch requires a Present request snapshot, which only navigations (preload), /auction, /_ts/page-bids, and requests whose EID-cookie ingestion loaded the row produce — a real-browser subresource request with a valid EC and no EID cookies, previously a dispatch opportunity, now exits here with no log at all, while every other ineligibility exit in this function logs. The narrowing itself looks like the intended KV-read saving from the design doc; the silent exit is the actionable part.
| let Some(request_entry) = context.snapshot.entry_for(context.ec_id()) else { | |
| return; | |
| }; | |
| let Some(request_entry) = context.snapshot.entry_for(context.ec_id()) else { | |
| log::debug!( | |
| "Pull sync: skipping dispatch for '{}' because the request captured no usable \ | |
| identity snapshot", | |
| super::log_id(context.ec_id()) | |
| ); | |
| return; | |
| }; |
(compile-verified: cargo fmt --all -- --check and cargo clippy-fastly pass with this applied)
| if ec_context.kv_snapshot().entry_for(&ec_id).is_some() { | ||
| set_ec_cookie_on_response(settings, ec_context, response); | ||
| } else { | ||
| log::warn!("Skipping generated EC cookie because backing row is not authoritative"); |
There was a problem hiding this comment.
🤔 thinking — When EID updates are present and the partner-ID CAS write fails (or exhausts its five attempts), upsert_partner_ids_from_snapshot returns Failed and this branch suppresses the cookie — even though the Add in generate_if_needed already proved the backing row exists. That is asymmetric with keep_proven, which correctly refuses to let a failed read retract the Add-confirmed proof: a failed write doesn’t disprove existence either. Net effect: the user churns to a fresh ID on the next visit and the just-created row becomes KV garbage, with no privacy gain (the row is live either way). Low probability, and the fail-closed choice is documented as deliberate — raising it as a design question, not a bug. A fix would carry the existence proof through the write-failure path for cookie issuance while still logging the enrichment failure.
| message: "Failed to persist generated EC ID to KV identity graph".to_string(), | ||
| })); | ||
| if let Some(graph) = kv { | ||
| match graph.create_if_absent(&ec_id, &entry) { |
There was a problem hiding this comment.
♻️ refactor — This migration (and the pull-sync/finalize ones) leaves several pub entry points with zero production callers — they’re reachable only from their own tests, and no dead-code lint fires because they’re pub in a lib crate. Verified across all workspace crates:
KvIdentityGraph::create_or_revive(ec/kv.rs:362) — last caller was this function, now oncreate_if_absent. (Generation no longer reviving tombstones is a behavior improvement worth keeping — a withdrawn ID stays withdrawn.)KvIdentityGraph::upsert_partner_id(ec/kv.rs:689) — last caller was pull sync’sdrain_pull_batch, now aggregated into the bulk snapshot path.ingest_eid_cookies(ec/prebid_eids.rs:158) plus wrappersingest_prebid_eids(:197) andingest_sharedid_cookie(:322) — finalize now usescollect_eid_cookie_updates+ snapshot upserts.
Apply manually — the deletion spans multiple files and their tests, so it can’t be a one-click suggestion. Removing them (here or in a fast follow-up) makes the snapshot-aware paths the only entry points, so future callers can’t reach for a non-snapshot write path that bypasses the new invariants.
| ) { | ||
| // Snapshot the orphaned ID once so every fail-closed exit binds the failed | ||
| // snapshot to the same key. | ||
| let orphan_id = ec_context.ec_value().unwrap_or_default().to_owned(); |
There was a problem hiding this comment.
⛏ nitpick — unwrap_or_default() can’t fire on the current call path (recovery is only reachable with an active EC value), but if it ever did, every fail-closed exit below would store EcKvSnapshot::Failed { ec_id: "" }, which belongs_to/entry_for then silently never match. A let Some(orphan_id) = ec_context.ec_value().map(str::to_owned) else { return; } (or an expect("should have an active EC during orphan recovery") per house style) would make the invariant explicit.
| /// Supporting adapters must override `wait` with direct single-handle | ||
| /// completion. A pending request marked for streaming must never be passed | ||
| /// to [`select`](Self::select), whose fan-out contract remains buffered. | ||
| fn supports_pending_streaming_responses(&self) -> bool { |
There was a problem hiding this comment.
🌱 seedling — The default wait routes through select, and both implemented selects now reject stream-marked handles — so an adapter that returns true here without overriding wait fails at runtime on every publisher navigation. The doc comment states the requirement, but a future adapter author could still miss it. Not for this PR: consider a shared conformance test (or a debug_assert) tying this capability to an overridden wait.
| Valid partner responses are collected across every HTTP concurrency batch into | ||
| one request-wide set of `PartnerIdUpdate` values and merged in one final bulk | ||
| CAS operation. Draining a network batch never writes KV. When the finalized snapshot still | ||
| has a usable generation, the uncontended case performs one write and no | ||
| additional read. When finalization already wrote the row, pull sync uses its | ||
| updated entry for eligibility, collects responses, then performs one refresh | ||
| lookup to obtain the new generation before its bulk CAS. A conflict rereads the | ||
| latest entry, rejects a tombstone, re-merges every collected update, and retries | ||
| within the existing bound. Pull sync never creates a missing root. |
There was a problem hiding this comment.
📝 note — This paragraph predates the consent-revalidation fix (af56a10f, added for the round-2 “post-send pull sync can disclose an ID after withdrawal” finding): the shipped code always re-reads the live row before the first partner request and cancels on anything short of a live, consenting entry, so “the uncontended case performs one write and no additional read” no longer holds — every dispatching request pays exactly one revalidation read, and the bulk CAS writes from that refreshed snapshot rather than the finalized one. Since this spec ships in this PR and reads as authoritative, the stale sentence misstates exactly the KV-read accounting the PR is about.
| Valid partner responses are collected across every HTTP concurrency batch into | |
| one request-wide set of `PartnerIdUpdate` values and merged in one final bulk | |
| CAS operation. Draining a network batch never writes KV. When the finalized snapshot still | |
| has a usable generation, the uncontended case performs one write and no | |
| additional read. When finalization already wrote the row, pull sync uses its | |
| updated entry for eligibility, collects responses, then performs one refresh | |
| lookup to obtain the new generation before its bulk CAS. A conflict rereads the | |
| latest entry, rejects a tombstone, re-merges every collected update, and retries | |
| within the existing bound. Pull sync never creates a missing root. | |
| Valid partner responses are collected across every HTTP concurrency batch into | |
| one request-wide set of `PartnerIdUpdate` values and merged in one final bulk | |
| CAS operation. Draining a network batch never writes KV. Because pull sync runs | |
| after the response is flushed and discloses the raw EC ID to partners, the live | |
| row is re-read once before the first partner request; anything short of a live, | |
| consenting entry cancels the dispatch, and that refreshed snapshot supplies the | |
| generation for the bulk CAS. A conflict rereads the latest entry, rejects a | |
| tombstone, re-merges every collected update, and retries within the existing | |
| bound. Pull sync never creates a missing root. |
(format-verified: prettier --check passes with this applied)
4 participants
Summary
ts-eccookie (valid cookie, missing KV row) by rotating to a freshly generated, KV-backed EC ID on consent-granted real-browser navigations — fixing the TTL-decay identity loss where quiet long-lived users silently lost their identity graph.Changes
ec/mod.rsEcKvSnapshotrequest-scoped state (NotRead/Missing/Present/Failed), EC-ID binding, recovery-eligibility flag;generate_if_neededuses bounded Add-only creationec/kv.rsload_snapshot,create_if_absent, snapshot-aware bulk upsert, existing-key-only conditional tombstone; CAS/refresh semantics + testsec/finalize.rsec/prebid_eids.rscollect_eid_cookie_updates) from KV persistenceec/pull_sync.rsauction/endpoints.rsplatform/http.rsselectsafeguardsplatform/test_support.rspublisher.rshttp_util.rsfastly-sslscheme-signal layering wart (no behavior change)adapter-fastly/platform.rsPendingRequest::wait(), retain method/stream metadata, preserve HEAD and bodiless-status framing, and reject stream-marked handles from auctionselectadapter-fastly/{app,main}.rsEcRequestState/EcFinalizeState; mutable finalize outcome into post-send pull syncCloses
Closes #851
Test coverage
origin start -> EC KV lookup -> auction dispatch -> origin wait.origin_proxy_errorabandonment outcome.select, HEAD, and1xx/204/205/304bodiless semantics are covered.Verification
cargo test-fastlycargo test-axumcargo test-cloudflarecargo test-spincargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test paritycargo clippy-fastlycargo clippy-axumcargo clippy-cloudflarecargo clippy-cloudflare-wasmcargo clippy-spin-nativecargo clippy-spin-wasmcargo fmt --all -- --checkcd crates/trusted-server-js/lib && node build-all.mjscd crates/trusted-server-js/lib && npx vitest run— 44 files, 829 tests, no type errorscd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatfastly compute servePR #1013 compatibility
PR #1013 does not supersede this change. A synthetic merge found a real conflict in
publisher.rs: the inline pending-origin optimization consumes the origin request before PR #1013's ESI/C2 lookup expects to inspect it. The combined implementation must branch by assembly mode:Inline: retain this PR's origin-first pending flow.Esi: retain the request through the C2 lookup and fetch the origin only on a cache miss.This compatibility work belongs in the branch that combines the two PRs; it is not included in this PR.
Hardening note
N/A — no config-derived regex or pattern compilation touched. Fail-closed invariants covered by tests: KV errors never become authoritative misses (no rotation), tombstones are never revived, cookies are emitted only after a backing row exists, store/CAS failures return a failed snapshot rather than claiming request-local IDs were persisted, and stream-marked pending requests cannot enter the buffered auction-selection path.
Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)