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).
|
@ChristianPavilonis please assign issue for this PR |
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Adds a signed, host-only browser marker that lets an eligible navigation skip the EC identity-graph read when pull-sync completeness is the only reason to inspect the row, and returns from post-send pull sync before the graph is constructed when there is nothing to pull. The design is sound and the skip is well fenced: I traced each path rather than only reading the diff.
4 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. All four were applied and verified in an isolated worktree at963c331(cargo fmt --all -- --check,cargo clippy-fastly,cargo check-fastly, andcargo test -p trusted-server-core --lib ec::— 334 passed), individually and as a batch, with a byte-exact pre/post drift check.
What I verified beyond the diff
marker_validimplies a non-empty registry.partner_set_fingerprintreturnsNonefor an empty pull set, so a valid marker requires at least one pull-enabled partner, soauction_needs_row == should_run_auction. A marker skip can therefore never suppress a read the auction needs.- A generated EC never carries a valid marker.
generate_if_neededandreplace_with_generatedboth callinvalidate_for_replaced_ec(), so the generated-cookie path atec/finalize.rs:150always has a real snapshot. No phantom-cookie regression fromentry_for(&ec_id).is_none(). - The EID gate matches its consumer exactly.
collect_eid_cookie_updatesreads onlyts-eidsandsharedId— the same two constantseid_cookie_may_need_persistencechecks — andupsert_partner_ids_from_snapshotshort-circuits on empty updates, so aNotReadsnapshot costs no hidden read in finalize. - No shared-cache leak from the new
Set-Cookie.enforce_set_cookie_cache_privacyruns inapply_terminal_response_effects, i.e. afterec_finalize_response, and the template cache stores reader-neutral bytes rather than response headers. - The
expect()inreconcile_markercannot fire. The!belongs_to(ec_id)early return plus thePresentmatch arm together guaranteeentry_forisSome. reconcile_markerruns exactly once per finalize call. Every branch either returns before it or falls through to a single call, so no response can pick up a duplicate markerSet-Cookie.- Only the Fastly adapter calls
build_pull_sync_context; the Cloudflare, Spin, and Axum adapters are unaffected by the signature change.
Non-blocking
♻️ refactor
build_pull_sync_contextdoc no longer describes its return contract — see inline atcrates/trusted-server-core/src/ec/pull_sync.rs:60entry_is_pull_completedoc omits the consent and empty-registry conditions — see inline atcrates/trusted-server-core/src/ec/pull_sync.rs:285
🤔 thinking
- No test for the two "steady state implies no
Set-Cookie" invariants — see inline atcrates/trusted-server-core/src/ec/pull_sync_marker.rs:580 should_preload_ecis double-duty and its safety argument is implicit — see inline atcrates/trusted-server-core/src/publisher.rs:4411
Cross-cutting / body-level findings
-
🤔
prepare_pull_sync_after_send's test asserts a property that already held —crates/trusted-server-adapter-fastly/src/main.rs:553.pull_sync_noop_states_skip_post_send_graph_factoryproves that aNoneplan does not build the graph, but the pre-PR shape (if is_real_browser && let Some(context) = build_pull_sync_context(..)wrappingrun_pull_sync_after_send, which built the graph inside) already guaranteed that. The behaviour that actually changed —build_pull_sync_contextreturningNonefor a complete row or an empty pull registry — is covered bybuild_pull_sync_context_skips_empty_registry_and_complete_snapshot. The case this helper uniquely adds and does not cover isSome(context)with a failinggraph_factory. Worth either covering that arm or noting that the helper exists purely to make the ordering testable. -
🌱 Allocation churn on the finalize path —
sorted_pull_partner_domainsclones everysource_domainString, and a singlereconcile_markerwalks it twice on the issue path (once directly, once throughcreate_marker->partner_set_fingerprint), withentry_is_pull_completecallingpull_enabled_partners()again. That is three to four partner-set traversals per response in Wasm, where CLAUDE.md asks to minimise allocations. Small in absolute terms and it does not belong in this PR, but a cached fingerprint onPartnerRegistrywould remove all of it. Follow-up. -
📝 Orphan-recovery detection is deferred by up to
MARKER_MAX_AGE_SECS— a marker-skipped navigation never observes aMissingsnapshot, sorecovery_eligiblecannot fire until the marker expires. This is explicitly accepted in the plan doc anddocs/guide/edge-cookies.md; recording it here so the tradeoff is visible on the PR thread rather than only in the plan. -
👍 The crypto hygiene here is genuinely good. Key separation through
MARKER_KEY_LABELrather than usingec.passphraseas the HMAC key directly; length-prefixed domain hashing inpartner_set_fingerprint, which removes the concatenation ambiguity a naive join would introduce; constant-timeverify_slicefor the tag; a both-sided expiry window (expires_at <= nowandexpires_at > now + MARKER_MAX_AGE_SECS) so a forged far-future expiry is rejected; andRedactedplus a hand-writtenDebugso the marker cannot leak into logs.EcSnapshotPreloadInputnaming all four "another consumer needs the row" guards is the right call too — it turns an implicit condition into something reviewable.
CI Status
cargo fmt: PASScargo test: PASScargo test (axum native): PASScargo test (cross-adapter parity): PASScargo test (ts CLI, native): PASScargo check (cloudflare native + wasm32-unknown-unknown): PASScargo check/build/test (spin native + wasm32-wasip1): PASSintegration tests: PASSintegration tests (Fastly EC lifecycle): PASSbrowser integration tests: PASSprepare integration artifacts: PASSvitest: PASSformat-typescript: PASSformat-docs: PASS
No check reported as required by branch protection on this branch; all 14 reported checks pass.
| @@ -59,8 +59,11 @@ struct PullSyncResponse { | |||
| /// | |||
| /// Returns `None` when consent denies EC or there is no active EC ID. | |||
There was a problem hiding this comment.
♻️ refactor — This is a pub function and the contract widened in this PR: it now also returns None when the registry has no pull-enabled partners, when the request snapshot has no entry for the active EC ID (NotRead / Missing / Failed), when the entry's consent is not ok, and when the entry is already pull-complete. The current line describes only two of those.
| /// Returns `None` when consent denies EC or there is no active EC ID. | |
| /// Returns `None` when there are no pull-enabled partners, when consent denies | |
| /// EC, when there is no valid active EC ID, or when the request snapshot holds | |
| /// no consented row that is still missing at least one pull-partner UID. |
(verified in a scratch worktree: cargo fmt --all -- --check and cargo check-fastly clean)
| } | ||
| } | ||
|
|
||
| /// Returns whether a live entry contains every pull-enabled partner ID. |
There was a problem hiding this comment.
♻️ refactor — The body also requires entry.consent.ok and a non-empty pull-partner set, so a consent-revoked entry and an empty registry both return false. The empty-registry case in particular is load-bearing — it is what stops reconcile_marker from issuing a marker for a configuration with no pull partners — so it is worth stating.
| /// Returns whether a live entry contains every pull-enabled partner ID. | |
| /// Returns whether a consented live entry holds a UID for every pull-enabled | |
| /// partner. Always `false` when the registry has no pull-enabled partners. |
(verified in a scratch worktree: cargo fmt --all -- --check and cargo check-fastly clean)
| assert_expired(&state, &response); | ||
| } |
There was a problem hiding this comment.
🤔 thinking — docs/guide/edge-cookies.md states that "A valid marker is not refreshed on each request, so this cost is limited to responses that establish or clear marker state in exchange for avoiding later KV reads." That is a cache-privacy claim: every marker write forces private, max-age=0 through enforce_set_cookie_cache_privacy and strips the edge-cache headers. The only thing enforcing it is the if !state.is_valid() guard on line 139, and no test covers it.
The mirror case matters just as much: a user whose row will never complete (a partner that never returns a UID) sits at Absent with an incomplete Present snapshot on every navigation, and expire_if_present must stay a no-op there or every one of those responses becomes uncacheable.
Both are one test:
| assert_expired(&state, &response); | |
| } | |
| assert_expired(&state, &response); | |
| } | |
| #[test] | |
| fn steady_states_leave_the_marker_cookie_untouched() { | |
| let (settings, registry) = settings_and_registry(&["a.example.com"]); | |
| let complete = live_snapshot(EC_ID, &["a.example.com"]); | |
| let incomplete = live_snapshot(EC_ID, &[]); | |
| let cases = [ | |
| (PullSyncMarkerState::Valid { expires_at: 4_600 }, &complete), | |
| (PullSyncMarkerState::Absent, &incomplete), | |
| ]; | |
| for (mut state, snapshot) in cases { | |
| let mut response = empty_response(); | |
| reconcile_marker( | |
| &settings, | |
| ®istry, | |
| Some(EC_ID), | |
| snapshot, | |
| &mut state, | |
| &mut response, | |
| ); | |
| assert!( | |
| marker_cookies(&response).is_empty(), | |
| "a steady marker state should not add a Set-Cookie header" | |
| ); | |
| } | |
| } |
(verified in a scratch worktree: cargo fmt --all -- --check, cargo clippy-fastly, and cargo test -p trusted-server-core --lib ec:: all clean — 334 passed, including the new case)
| let eid_cookie_may_need_persistence = cookie_jar | ||
| .as_ref() | ||
| .is_some_and(|jar| jar.get(COOKIE_TS_EIDS).is_some() || jar.get(COOKIE_SHAREDID).is_some()); | ||
| let should_preload_ec = should_preload_ec_snapshot(&EcSnapshotPreloadInput { |
There was a problem hiding this comment.
🤔 thinking — should_preload_ec does two jobs: it decides the KV read, and it also gates the concurrent origin send_async on line 4434. Before this PR the two were the same question. Now a marker skip flips both.
I convinced myself it is currently latency-neutral, but only through a three-step chain: marker_valid requires partner_set_fingerprint to be Some, which requires a non-empty pull set, which makes registry.is_empty() false, which makes auction_needs_row == should_run_auction — so marker_can_skip implies !should_run_auction, so neither the dispatch_auction await on line 4538 nor the template-cache lookup on line 4637 (already excluded by the template_cache_key.is_none() guard on the concurrent path) sits between here and the origin send on line 4724.
That is a real invariant, but nothing in the code states it. If auction_needs_row is ever loosened — say to skip the read for an auction with no server-side EIDs — the origin fetch would silently move behind auction dispatch, and the parity suite would not catch it. Worth naming so the next editor sees the coupling:
| let should_preload_ec = should_preload_ec_snapshot(&EcSnapshotPreloadInput { | |
| // Also gates the concurrent origin `send_async` below. A marker-driven skip | |
| // stays latency-neutral only because `marker_valid` implies a non-empty | |
| // registry, so `auction_needs_row == should_run_auction` and a skip means no | |
| // auction dispatch runs between here and the origin send. | |
| let should_preload_ec = should_preload_ec_snapshot(&EcSnapshotPreloadInput { |
(verified in a scratch worktree: cargo fmt --all -- --check and cargo check-fastly clean)
963c331 to
4e9a01e
Compare
3 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!)