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 15 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.
| } if snapshot_id == ec_id => snapshot, | ||
| EcKvSnapshot::Missing { | ||
| ec_id: ref snapshot_id, | ||
| } if snapshot_id == ec_id => return snapshot, |
There was a problem hiding this comment.
🔧 P1 — A preloaded miss can drop an explicit withdrawal
A publisher preload can observe Missing at an eventually-consistent Fastly POP. This return treats that stale result as final, so finalization deletes the browser cookie but never writes the authoritative tombstone; the live row can remain consent.ok = true and accept later identity/EID operations.
Please revalidate a preloaded Missing after the origin round trip and tombstone only when that confirmation is Present. That preserves existing-key-only behavior for forged cookies. Add a Missing -> Present withdrawal test.
| | EcKvSnapshot::Failed { | ||
| ec_id: ref snapshot_id, | ||
| } if snapshot_id == ec_id => return snapshot, | ||
| _ => self.load_snapshot(ec_id), |
There was a problem hiding this comment.
🔧 P1 — EID ingestion re-downgrades a successful Add
generate_if_needed records a successful Add as Present { generation: None }. When ts-eids or sharedId has an update, this branch refreshes that proven snapshot; a transient miss/failure then reaches finalization as Missing/Failed, which suppresses the new ts-ec cookie despite the successfully persisted root. The publisher preload guard does not cover this finalization refresh.
Retain explicit Add-confirmation provenance and do not replace it with a best-effort EID-write refresh failure. The update can be skipped/logged, but cookie issuance should continue from the successful Add. Please add an Add-success + lookup-miss test with a valid EID update.
| ); | ||
| return; | ||
| } | ||
| let Some(kv_entry) = context.snapshot.entry_for(context.ec_id()) else { |
There was a problem hiding this comment.
🔧 P1 — Post-send pull sync can disclose an ID after withdrawal
This authorizes post-send partner calls from the earlier request snapshot without checking whether a concurrent request has since tombstoned the EC. After the page response reaches the browser, a CMP withdrawal can complete before this callback dispatches pull sync, yet the stale live snapshot still sends the raw ec_id to partners.
Revalidate live, non-tombstoned consent immediately before dispatching post-send pulls (through a consistency-safe revocation check). Add a concurrency regression test that tombstones after snapshot capture and asserts no pull request is sent.
| // EC and both KV and partner stores are available. Gate the read on a | ||
| // present registry: without one, `resolve_auction_eids` yields no | ||
| // server-side EIDs, so the snapshot would be an unused billable KV read. | ||
| let auction_kv_snapshot = match (kv, ec_id, registry) { |
There was a problem hiding this comment.
🔧 P2 — Endpoint snapshots are not shared with finalization
This snapshot is local, while Fastly passes an unchanged immutable EcContext into response finalization. If ts-eids/sharedId has updates, finalization sees NotRead and performs a second billable lookup. The same issue exists in the page-bids local snapshot at publisher.rs:4117.
Thread a mutable EcContext through these handlers and store the loaded snapshot before returning (or otherwise return it to the Fastly caller). Add a counting-KV test covering endpoint EID resolution plus finalization.
| let merged = graph.upsert_partner_ids_from_snapshot(ec_id, updates, confirmed); | ||
| ec_context.set_kv_snapshot(merged); | ||
| } | ||
| EcKvSnapshot::Missing { .. } => { |
There was a problem hiding this comment.
🔧 P2 — Two eventually-consistent misses are still treated as authoritative absence
The confirmation read reduces the chance of a false rotation but cannot prove absence on an eventually-consistent store. If both reads are stale, this creates a replacement ID and abandons a still-live identity graph, fragmenting its accumulated EIDs.
Do not rotate unless the backend can provide a consistency-safe absence proof; otherwise fail closed and retry recovery on a later navigation. Add a two-miss test with the original row becoming visible afterward.
3 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!)