Avoid no-op EC KV reads in post-send pull sync - #900
Conversation
0532bba to
41544e8
Compare
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Solid, well-scoped implementation of the #880 contract: the marker protocol is cryptographically careful, the preload gate enumerates its consumers explicitly instead of guessing, and the fallbacks all fail toward the KV path. One blocking issue: the withdrawal path emits the marker-expiry Set-Cookie unconditionally, which downgrades cache privacy for an entire traffic class that previously had no Set-Cookie at all.
2 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. Both were applied in an isolated worktree at this head and verified withcargo fmt --all -- --check,cargo clippy-fastly,cargo check-axum,cargo check-cloudflare, and the nativetrusted-server-coreec::+ publisher marker tests (332 and 28 passing respectively), with a byte-exact pre/post drift check. The remaining comments describe the fix in prose because it spans two files.
Blocking
🔧 wrench
- Withdrawal always emits a marker-expiry
Set-Cookie, downgrading cache privacy for all withdrawn traffic — see inline atcrates/trusted-server-core/src/ec/finalize.rs:63
Non-blocking
🤔 thinking
- Marker issuance re-adds a
Set-Cookieto responses PR #885 deliberately made cookie-free — see inline atcrates/trusted-server-core/src/ec/finalize.rs:160
♻️ refactor
- Completeness duplicates dispatch eligibility; the two can drift — see inline at
crates/trusted-server-core/src/ec/pull_sync_marker.rs:170
⛏ nitpick
- The three-state loop asserts the same thing three times — see inline at
crates/trusted-server-adapter-fastly/src/main.rs:553
👍 praise
- Marker crypto and framing — see inline at
crates/trusted-server-core/src/ec/pull_sync_marker.rs:299
Cross-cutting / body-level findings
- 📝 Verified as correct, for the record — several things that look risky on first read hold up: the
expectinreconcile_markeris unreachable because thebelongs_tocheck precedes thePresentmatch andentry_forignoresgeneration; the partner-set fingerprint is consistent across the validate and issue sites because every caller builds the registry fromPartnerRegistry::from_config(&settings.ec.partners);upsert_partner_ids_from_snapshotreturns early on empty updates, so a marker-valid navigation really does perform zero reads rather than moving the read later; stored KV EIDs reach only the auction path, whichauction_needs_rowforces the read for; tombstones are excluded viaconsent.ok; and per-partnerpull_sync_ttl_secis not wired into eligibility, so presence-only completeness matches dispatch today (see the ♻️ finding for the drift risk that creates).
CI Status
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
All 14 reported checks pass. gh pr checks --required returned no names for this PR, so no check is annotated as branch-protection-required (the base branch is fix/kv-eid-request-snapshot-ec-ttl, not main).
| if consent_withdrawn { | ||
| expire_marker(ec_context.pull_sync_marker_mut(), response); | ||
| } |
There was a problem hiding this comment.
🔧 wrench — Withdrawal always emits a marker-expiry Set-Cookie, downgrading cache privacy for all withdrawn traffic.
expire_marker has no presence guard — it appends the cookie whatever the state, including Absent. So every explicitly-withdrawn request (GPC=true, US-state opt-out) now returns Set-Cookie: ts-ec-pull-complete=; …Max-Age=0, even for the common case of a withdrawn visitor who has no EC cookie and never had a marker. On the base branch this path emitted no Set-Cookie at all, because expire_ec_cookie is gated on cookie_was_present().
Downstream that matters: apply_terminal_response_effects (crates/trusted-server-adapter-fastly/src/main.rs:418) runs enforce_set_cookie_cache_privacy after finalize, and for any cookie-bearing response that strips every edge-cache header and forces Cache-Control: private, max-age=0. template_cache_ttl (crates/trusted-server-core/src/publisher.rs:6168) also bypasses the shared template cache on Set-Cookie. Withdrawn requests never run the ad stack, so they never get with_cache_bypass — those responses were shareable before this change and are not after it, for an entire traffic class, as a side effect of a KV-read optimization.
Gating on was_present() keeps the plan's guarantee intact: it reads only the request cookie, never KV, so withdrawal still clears the marker with no KV dependency — exactly how ts-ec expiry is already gated on cookie_was_present(). explicit_withdrawal_expires_marker_without_ec_cookie still passes, since it seeds PullSyncMarkerState::Invalid (present).
| if consent_withdrawn { | |
| expire_marker(ec_context.pull_sync_marker_mut(), response); | |
| } | |
| if consent_withdrawn && ec_context.pull_sync_marker().was_present() { | |
| expire_marker(ec_context.pull_sync_marker_mut(), response); | |
| } |
Verified in an isolated worktree at this head: cargo fmt --all -- --check, cargo clippy-fastly, cargo check-axum, cargo check-cloudflare, and 332 native trusted-server-core ec:: tests all pass with this applied.
| reconcile_pull_sync_marker(settings, registry, ec_context, response); | ||
| } | ||
|
|
||
| fn reconcile_pull_sync_marker( |
There was a problem hiding this comment.
🤔 thinking — Marker issuance re-adds a Set-Cookie to responses PR #885 deliberately made cookie-free.
Same mechanism as the wrench finding above, but inherent to the design rather than a bug. The returning-user path comments that "ordinary returning-user page views no longer refresh the browser cookie" — yet a marker issue or expire makes exactly those responses cookie-bearing, so enforce_set_cookie_cache_privacy downgrades them to private, max-age=0 with edge-cache headers stripped, and Esi-mode responses stop being eligible for template-cache storage.
Bounded to roughly one response per hour per user per host, against one saved KV read per navigation, so it is plausibly a clear win — but the trade is not in the plan's risk table or in docs/guide/edge-cookies.md, and the two effects land in different budgets (KV operations vs. edge cache hit rate). Worth stating explicitly so it isn't rediscovered from a cache-hit-rate graph later.
| fn entry_has_all_pull_partner_ids(entry: &KvEntry, pull_partners: &[String]) -> bool { | ||
| entry.consent.ok | ||
| && pull_partners | ||
| .iter() | ||
| .all(|source_domain| entry.ids.contains_key(source_domain)) | ||
| } |
There was a problem hiding this comment.
♻️ refactor — Completeness duplicates dispatch eligibility; the two can drift.
entry_has_all_pull_partner_ids reimplements the inverse of is_partner_pull_eligible (crates/trusted-server-core/src/ec/pull_sync.rs:286-290, which is just entry.ids.get(&partner.source_domain).is_none()). They agree today only because eligibility ignores pull_sync_ttl_sec. If per-partner TTL refresh is ever wired into eligibility, completeness silently over-claims, and the marker then suppresses both the snapshot read and pull sync for up to an hour — the failure would be invisible, because both sides would still "agree" in their own tests.
Deriving one from the other makes that drift impossible:
// pull_sync.rs
pub(crate) fn is_partner_pull_eligible(partner: &PartnerConfig, kv_entry: Option<&KvEntry>) -> bool
// pull_sync_marker.rs
fn entry_has_all_pull_partner_ids(entry: &KvEntry, registry: &PartnerRegistry) -> bool {
entry.consent.ok
&& !registry
.pull_enabled_partners()
.iter()
.any(|partner| is_partner_pull_eligible(partner, Some(entry)))
}Apply manually — can't be auto-applied as a suggestion because it changes is_partner_pull_eligible's visibility in pull_sync.rs and replaces the domain-slice parameter threaded through reconcile_marker, so it spans two files.
| #[test] | ||
| fn pull_sync_noop_states_skip_post_send_graph_factory() { | ||
| for reason in ["no partners", "complete snapshot", "unread marker state"] { | ||
| let calls = std::cell::Cell::new(0); | ||
| let result = prepare_pull_sync_after_send(None, || { | ||
| calls.set(calls.get() + 1); | ||
| Err(Report::new(TrustedServerError::KvStore { | ||
| store_name: "unexpected".to_owned(), | ||
| message: "graph factory should not run".to_owned(), | ||
| })) | ||
| }); | ||
| assert!(result.is_none(), "{reason} preparation should return none"); | ||
| assert_eq!( | ||
| calls.get(), | ||
| 0, | ||
| "{reason} should not invoke the graph factory" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
⛏ nitpick — The three-state loop asserts the same thing three times.
for reason in ["no partners", "complete snapshot", "unread marker state"] runs three byte-identical prepare_pull_sync_after_send(None, …) calls. None of the three named states is exercised here — all three are decided inside build_pull_sync_context, which this test never calls — so the labels read as coverage that doesn't exist. What the test actually proves (a None plan never reaches the graph factory) is worth keeping; the loop is not.
| #[test] | |
| fn pull_sync_noop_states_skip_post_send_graph_factory() { | |
| for reason in ["no partners", "complete snapshot", "unread marker state"] { | |
| let calls = std::cell::Cell::new(0); | |
| let result = prepare_pull_sync_after_send(None, || { | |
| calls.set(calls.get() + 1); | |
| Err(Report::new(TrustedServerError::KvStore { | |
| store_name: "unexpected".to_owned(), | |
| message: "graph factory should not run".to_owned(), | |
| })) | |
| }); | |
| assert!(result.is_none(), "{reason} preparation should return none"); | |
| assert_eq!( | |
| calls.get(), | |
| 0, | |
| "{reason} should not invoke the graph factory" | |
| ); | |
| } | |
| } | |
| #[test] | |
| fn pull_sync_noop_states_skip_post_send_graph_factory() { | |
| let calls = std::cell::Cell::new(0); | |
| let result = prepare_pull_sync_after_send(None, || { | |
| calls.set(calls.get() + 1); | |
| Err(Report::new(TrustedServerError::KvStore { | |
| store_name: "unexpected".to_owned(), | |
| message: "graph factory should not run".to_owned(), | |
| })) | |
| }); | |
| assert!( | |
| result.is_none(), | |
| "a skipped pull-sync plan should return none" | |
| ); | |
| assert_eq!(calls.get(), 0, "should not invoke the graph factory"); | |
| } |
The three states themselves are already covered in core by build_pull_sync_context_skips_empty_registry_and_complete_snapshot.
Verified in an isolated worktree at this head: cargo fmt --all -- --check and cargo clippy-fastly (which compiles --all-targets, including this test) both pass with this applied.
| Some(expires_at) | ||
| } | ||
|
|
||
| fn marker_key(settings: &Settings) -> [u8; 32] { |
There was a problem hiding this comment.
👍 praise — Marker crypto and framing.
The details here are the ones that usually get skipped: labelled key derivation off ec.passphrase rather than using the passphrase directly as the MAC key; a NUL-delimited payload that puts the version and the fingerprint inside the MAC input, so no field can be shifted into another; constant-time verify_slice; a length cap applied before any parsing; a host-only cookie, deliberately tighter than ts-ec's Domain=.<publisher>; and Redacted plus a hand-written Debug so the value can't leak through a log line. No UID or EC ID in the cookie itself. The order-independent, set-sensitive fingerprint test covering add/remove/enable/disable is the right shape of test for this.
2 participants
Summary
This PR is stacked on #885. The zero-operation guarantee is scoped to KV work caused solely by pull sync; other identity lifecycle consumers may still require the shared EC snapshot.
Changes
crates/trusted-server-core/src/ec/pull_sync_marker.rscrates/trusted-server-core/src/ec/{mod,finalize,pull_sync}.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-adapter-fastly/src/main.rsdocs/guide/edge-cookies.mddocs/superpowers/plans/2026-07-13-issue-880-no-op-pull-sync-kv-reads.mdCloses
Closes #880
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve— not runcargo test-cloudflare,cargo test-spin, cross-adapter parity, and all Cloudflare/Spin native + WASM clippy targetsChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)