Repository navigation
Conversation
db9e915 to
bdd12a8
Compare
bdd12a8 to
9f1e764
Compare
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
This PR moves pull sync into after-response identity collection and adds a bounded, JavaScript-readable mirror of provider-written bidstream UIDs. One startup-validation issue needs addressing; there is also an opportunity to avoid a redundant KV read on steady-state navigations.
The inline comment is prose-only: the proposed fix and regression coverage touch lines outside the diff, so there are no one-click suggestions.
Blocking
🔧 wrench
- Reject malformed pull tokens during startup — see inline at
crates/trusted-server-identity-providers/src/pull_sync.rs:59-60.
Cross-cutting / body-level findings
♻️ Avoid the follow-up KV read when all lookups were skipped
Introduced by crates/trusted-server-identity-providers/src/lib.rs:169-182; affected helper: crates/trusted-server-core/src/ec/after_response.rs:76-79.
When every pull-sync source already holds a UID, dispatch produces Skipped(NotDue) results. Those results are nonempty, so the follow-up helper rereads KV despite having nothing to persist. Moving existing pull-sync partners into this path adds an avoidable read on every eligible returning navigation.
Import LookupStatus and SkipReason alongside LookupResults, then replace the helper's empty-results guard with:
if lookups.iter().all(|lookup| {
lookup.entries.is_empty()
&& lookup.cleared.is_empty()
&& lookup.status != LookupStatus::Skipped(SkipReason::OptedOut)
}) {
return AfterResponseOutcome::Unchanged;
}This preserves writes that remember provider opt-outs.
Apply manually — the helper, imports, and regression test are outside the diff.
Verification
- A focused scratch reproduction confirmed that a newline-containing pull token passes runtime config validation but prevents construction of the coordinator for both an invalid and an otherwise valid partner.
- A counting-store scratch reproduction confirmed that all-
NotDueresults still cause one KV read in the follow-up helper. - Both reproductions passed. Scratch changes were removed and the worktree was restored to the reviewed head.
- The proposed fixes have not been compiled or run through the full local gate.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- trace runtime and browser acceptance: PASS
- cargo fmt: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- format-docs: PASS
- worker-deadlines (spin): PASS
- worker-deadlines (cloudflare): PASS
- ts dev lint domains (changed lines): PASS
- cargo test: PASS
- format-typescript: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- HTML EOF runtime (Fastly): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- CLAUDE.md symlink guard: PASS
- docs-proposal scripts: PASS
- CI Node pin guard: PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- prepare integration artifacts: PASS
- vitest: PASS
No required checks were reported by the required-check query.
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR moves pull sync onto the after-response provider path and adds a JavaScript-readable mirror of provider-written bidstream UIDs. The move keeps the live-row consent re-check and the bot gate. Before the mirror ships, it needs an operator opt-in and a per-vendor consent check. The after-response path also adds KV reads that the legacy dispatcher avoided.
3 of the 10 inline comments carry a one-click GitHub
suggestion. Use Commit suggestion, or Add suggestion to batch to apply several at once. The other comments describe the fix in prose, because those changes span files or touch lines outside the diff.
Blocking
🔧 wrench
- Make the EID mirror opt-in, separate from
bidstream_enabled: see inline atcrates/trusted-server-core/src/ec/finalize.rs:90 - Withhold mirrored UIDs whose provider lacks TCF vendor consent: see inline at
crates/trusted-server-core/src/ec/mirror.rs:77 - Skip the after-response row read when nothing is due: see inline at
crates/trusted-server-adapter-fastly/src/app.rs:453 - Cover the
enabled = falsebypass of the double-claim check (suggestion): see inline atcrates/trusted-server-core/src/config.rs:1455
❓ question
- Should pull-sync sources keep accepting browser UIDs?: see inline at
crates/trusted-server-core/src/ec/registry.rs:274
Non-blocking
♻️ refactor
- Delete
pull_enabled_partners; nothing calls it now: see inline atcrates/trusted-server-core/src/ec/registry.rs:358 - Keep one definition of "runs pull sync": see inline at
crates/trusted-server-core/src/ec/registry.rs:395
🤔 thinking
- The pull-sync cap is logged as
skipped_rate_limited: see inline atcrates/trusted-server-core/src/ec/coordinator/mod.rs:578
⛏ nitpick
- Describe where pull sync runs now (suggestion): see inline at
docs/guide/edge-cookies.md:268 - The
Nonecase cannot happen (suggestion): see inline atcrates/trusted-server-identity-providers/src/lib.rs:71
Cross-cutting / body-level findings
- 📝 "Behaviour changes from
main" is missing items. In addition to the four already listed:- Browser
ts-eidsandsharedIdUIDs are no longer stored for pull-sync sources. See the question atregistry.rs:274. - Pull sync runs only on Fetch Metadata navigations that get a page-load permit. It no longer runs after
/auctionresponses, on navigations without Fetch Metadata, or on prefetches. - Each pull used to have a 15-second first-byte timeout. Pull sync now shares the 2-second
identity_after_response_budget_mswith every after-response provider, so a partner slower than that is never filled. pull_sync_concurrency = 0used to mean one request at a time (.max(1), with every partner still pulled). It now means no limit.- A steady-state returning navigation makes up to two more KV reads after the response. See the inline comment at
app.rs:453and the open thread onpull_sync.rs:60.
- Browser
- 📝 Earlier findings are still open. ChristianPavilonis's two findings apply unchanged at this head, and that thread has no reply yet:
- 🔧 malformed pull tokens (
pull_sync.rs:60) - ♻️ the follow-up read when every result is
NotDue
- 🔧 malformed pull tokens (
Verification
- Scratch tests at this head did three things:
- reproduced the browser-ingest change (
trusted-server-identity-providers, native target) - reproduced the mirror's vendor-consent gap (
ec/mirror.rs,wasm32-wasip1under Viceroy) - confirmed that the double-claim bypass works
- reproduced the browser-ingest change (
- I applied each suggestion to a scratch copy, first alone and then together with the others:
- The config test passed rustfmt, the new test itself, all six adapter clippy aliases,
test-fastly,test-axum,test-cloudflare,test-spinand the parity suite. - The doc comment passed rustfmt and
clippy-fastly. - The guide line passed prettier.
- The scratch changes were then discarded.
- The config test passed rustfmt, the new test itself, all six adapter clippy aliases,
- The prose fixes (mirror opt-in, vendor filter,
any_due,runs_pull_sync) have not been compiled.
CI Status
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CI Node pin guard: PASS
- CLAUDE.md symlink guard: PASS
- docs-proposal scripts: PASS
- format-docs: PASS
- format-typescript: PASS
- HTML EOF runtime (Fastly): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- trace runtime and browser acceptance: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- worker-deadlines (spin): PASS
No required checks are reported for this branch.
9f1e764 to
15eb284
Compare
|
@aram356 @ChristianPavilonis review-body points, addressed in 7360a0e (rebased onto #1487 at f844a52):
|
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
This round addresses every earlier finding: the mirror is opt-in per partner and honours TCF vendor consent, browser ingest still fills pull-sync sources, the after-response phase makes no KV read when nothing is due, malformed pull tokens fail at startup, and the concurrency cap logs as skipped_concurrency_cap with hourly rotation. HTTPS and allowlist checks for pull-sync URLs, the mirror's cookie encoding and __Host- attributes, the bot gate, the fail-closed limiter and the double-claim rejection also look right. Three non-blocking points remain.
Non-blocking
🤔 thinking
- Retiring the completeness marker brings back a pre-response KV read — see below.
♻️ refactor
provider_ownedhas no production reader after the ingest split — see inline atcrates/trusted-server-core/src/ec/registry.rs:276-283
⛏ nitpick
- Unused
logdependency — see inline atcrates/trusted-server-identity-providers/Cargo.toml:20
Cross-cutting / body-level findings
- 🤔 Retiring the completeness marker brings back a pre-response KV read. On the base branch, a valid
ts-ec-pull-completemarker let a permitted navigation skip the row preload (should_preload_ec_snapshot'smarker_can_skip) when no auction, browser EID cookie or withdrawal needed the row. With the marker gone,should_preload_ec_snapshot(publisher.rs:3466-3473) returnseligiblefor every permitted navigation. Deployments with pull-sync partners therefore read KV on steady-state returning navigations that have no auction and nots-eids/sharedIdcookie, where they previously read nothing for up to an hour. In the default inline mode the read overlaps the origin fetch, so the cost is mostly KV operations; with template-cache keys the origin waits for it. Dropping the marker also removes its periodicSet-Cookie, which is a fair trade. Could you add this to "Behaviour changes frommain", or confirm it is intended? The guide says the marker is retired but not that the read it avoided is back.
CI Status
All 29 check runs on 15eb284f4 passed.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
This PR moves pull sync into after-response identity collection and adds an opt-in, bounded mirror cookie for provider-written bidstream UIDs. Two correctness issues remain: clearing an existing mirror after vendor-consent withdrawal, and preserving the default partner configuration schema during mixed-version deployment or rollback.
Both inline comments carry a one-click GitHub
suggestion. Use Commit suggestion, or Add suggestion to batch, to apply the proposed replacements.
Blocking
🔧 wrench
- Clear an existing mirror when vendor consent is withdrawn (suggestion) — see inline at
crates/trusted-server-core/src/ec/finalize.rs:99–114. - Omit default
mirror_enabledfrom serialized configuration (suggestion) — see inline atcrates/trusted-server-core/src/settings.rs:453.
Review scope and verification
- Reviewed the stacked delta against
feat/s2s-eid-collection-coreat head15eb284f495c5fb9554a04b178804a4776d63965. - Coverage limitation: large unchanged portions of
settings.rsandpublisher.rs, especially their tests, were not exhaustively examined end to end. This is not an exhaustive whole-file review. Full reads were completed for Fastlyapp.rs,main.rs,config.rs, and the deleted completeness-marker implementation. - Each suggestion was applied and verified independently against the clean reviewed head: rustfmt; all eight clippy aliases; Fastly, Axum and Cloudflare check aliases; Fastly tests with the repository-pinned Viceroy 0.21.0 (including the reusable-sandbox variant), Axum, Cloudflare and Spin tests; and cross-adapter parity tests.
- An out-of-tree native public-API harness exercised the mirror replacement with a real encoded cookie: vendor withdrawal on a non-navigation, withdrawal with an unavailable navigation snapshot, unchanged consent controls, no-cookie handling, and retaining another consented vendor on a readable navigation.
- The serialization replacement was checked with omitted, explicit-false and string-false inputs against the base partner key set; explicit
truesurvived serialization and deserialization. - Byte-exact pre/post-verification patch comparisons passed for both suggestions. Scratch edits were discarded and the review worktree was restored to the reviewed head. No commits or pushes were made.
- No manual browser or deployed-service validation was performed locally.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- trace runtime and browser acceptance: PASS
- integration tests: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- format-typescript: PASS (required)
- cargo test: PASS (required)
- CI Node pin guard: PASS
- HTML EOF runtime (Fastly): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo fmt: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- Analyze (actions): PASS
- Analyze (rust): PASS
- worker-deadlines (spin): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- cargo test (axum native): PASS
- CLAUDE.md symlink guard: PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- prepare integration artifacts: PASS
- ts dev lint domains (changed lines): PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
This round fixes all eleven of my round-1 findings, and the rebase onto #1487's startup-built coordinators is clean. I found no new code defects. What still blocks are two documentation gaps: the design spec now describes the behaviour this PR replaced, and the CHANGELOG has no entry, including for one breaking change.
2 of the 4 inline comments carry a one-click GitHub
suggestion. Use Commit suggestion, or Add suggestion to batch to apply both at once. The other two describe changes outside the diff.
Blocking
🔧 wrench
- Update the design spec to match this PR: body-level, below
- Add CHANGELOG entries, including one Breaking item: body-level, below
Non-blocking
♻️ refactor
- Carry
runs_pull_syncontoPartnerConfigto simplify the double-claim check: see inline atcrates/trusted-server-core/src/config.rs:384
🤔 thinking
- Reject
mirror_enabledwithoutbidstream_enabled: see inline atcrates/trusted-server-core/src/settings.rs:454
⛏ nitpick
- Give
holds_live_uidits doc line back (suggestion): see inline atcrates/trusted-server-core/src/ec/coordinator/mod.rs:294-306 - Assert the specific token error (suggestion): see inline at
crates/trusted-server-core/src/config.rs:1502-1506
Cross-cutting / body-level findings
-
🔧 Update the design spec to match this PR.
docs/superpowers/specs/2026-10-30-s2s-eid-collection-design.mdwas added in #1487 (889159cd6) and is not onmainyet. It describes this PR as PR5 and PR6, but the behaviour changed in this round:- D6 (line 81) and §10 Contents (lines 1407-1408): say the mirror holds every
bidstream_enabledsource. It now needsmirror_enabledas well, and it leaves out UIDs whose owning provider's TCF vendor consent is denied. - §10 Set and Expired (lines 1412-1418): say the cookie is set on every consented navigation, and expired on any response without EID consent and on any navigation whose mirror would be empty. The code instead sets it only when the value changes, expires only a cookie the request carried, and leaves it untouched when the row cannot be read.
- PR5 row (line 1529) and acceptance scenario 21 (lines 1597-1600): say browser ingest skips pull-sync sources and batch sync rejects them, and that a browser
ts-eidsUID and a batch record are "ignored or rejected". Both now keep filling the source, and pull sync fills it only when it holds no UID. - PR6 row (line 1530),
IdentityCoordinator::mirror(line 1059) and theapply_cookiescall (line 1228): describe the mirror as a coordinator method. The code builds it withec::mirror::build_mirror(row, consent, partners, now), called fromec_finalize_response.
Since the spec lands with this stack, please update these sections in this PR, so reviewers and later work read the design that ships.
- D6 (line 81) and §10 Contents (lines 1407-1408): say the mirror holds every
-
🔧 Add CHANGELOG entries, including one Breaking item.
[Unreleased]carries #1487's provider entry but nothing for this PR's user-visible changes.- Breaking. On
main, a resolvedts_pull_tokenthat cannot form a header value made the old dispatcher's.expect("should build pull sync request")panic after the page was sent, so only pull sync failed. It now fails config validation, and the service does not start. A secret value with a trailing newline is the likely way to hit this. - A possible wording, under
### Changed:
- Pull sync now runs as a `pull-sync` identity provider built from the existing `[[ec.partners]]` settings, after the response on top-level publisher navigations from recognized browsers, within `ec.identity_after_response_budget_ms` shared with other after-response providers. It no longer runs after `/auction` responses, on prefetches or on navigations without Fetch Metadata, and a partner slower than the budget is not filled, where each pull used to wait up to 15 seconds. `ec.pull_sync_concurrency` now caps the partners pulled per navigation (`0` means no limit; it used to mean one at a time), partner order rotates hourly, and a partner past the cap logs `skipped_concurrency_cap`. The `ts-ec-pull-complete` marker is retired and leftover cookies are expired. `kind = "pull-sync"` in `[[ec.identity_providers]]` is rejected, as is a configured provider that claims a pull-sync partner's source. - Added `mirror_enabled` on `[[ec.partners]]` (default `false`). With `bidstream_enabled`, the partner's provider-written UIDs are mirrored to the JavaScript-readable `__Host-ts-eids-mirror` cookie, leaving out UIDs whose owning provider lacks TCF vendor consent. See [EID Mirror Cookie](docs/guide/edge-cookies.md#eid-mirror-cookie). - **Breaking:** For a partner with pull sync enabled, a resolved `ts_pull_token` that cannot form an `Authorization: Bearer` header, such as one ending in a newline, now fails config validation and stops startup. Before, only that partner's pull sync failed, after the page was sent. Check pull-token secrets before deploying.
- Breaking. On
-
📝 Earlier findings.
- Mine: all eleven round-1 findings are addressed at this head. I checked each in the code. The browser-ingest repro now stores the browser UID for a pull-sync source, and the vendor-consent and bypass cases have tests.
- Still open from other reviewers at this head:
- ChristianPavilonis's two 🔧: the mirror after a vendor withdrawal (
finalize.rs:114), and serializing a defaultmirror_enabled(settings.rs:453). - prk-Jr's ♻️ on
provider_owned(registry.rs:283). - prk-Jr's ⛏ on the unused
logdependency. - prk-Jr's 🤔 on the preload read that the retired marker used to skip.
- ChristianPavilonis's two 🔧: the mirror after a vendor withdrawal (
Verification
- At this head, the round-1 browser-ingest scratch test passes with the expected values: the UID is stored both with and without pull sync.
- I applied each suggestion to a scratch copy, first alone and then together:
- The assertion change passed rustfmt, its test, all six adapter clippy aliases,
test-fastly,test-axum,test-cloudflare,test-spinand the parity suite. - The doc-line move passed rustfmt and
clippy-fastly. - The scratch changes were then discarded.
- The assertion change passed rustfmt, its test, all six adapter clippy aliases,
- The prose proposals (
runs_pull_synconPartnerConfig, rejectingmirror_enabledwithout bidstream) have not been compiled.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): 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) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CI Node pin guard: PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- HTML EOF runtime (Fastly): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- trace runtime and browser acceptance: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- worker-deadlines (spin): PASS
15eb284 to
29de3d4
Compare
|
aram356
left a comment
There was a problem hiding this comment.
Summary
This round addresses every earlier finding from all three reviewers, and the rebase onto #1487's latest head is clean. One defect remains, in the vendor-withdrawal expiry added this round: for a visitor who denies one mirrored vendor, the mirror is cleared after every page view and set again on the next navigation.
1 of the 2 inline comments carries a one-click GitHub
suggestion; the other describes a fix that spans two files.
Blocking
🔧 wrench
- Expire the mirror only when it holds a denied vendor's UIDs: see inline at
crates/trusted-server-core/src/ec/finalize.rs:144
Non-blocking
⛏ nitpick
- Drop "pull sync skips them" from the provider entry (suggestion): see inline at
CHANGELOG.md:24
Cross-cutting / body-level findings
- 📝 Earlier findings, checked at this head.
- Mine from round 2: all fixed. These were the spec drift, the CHANGELOG (including the Breaking pull-token entry),
runs_pull_synconPartnerConfig, rejectingmirror_enabledwithoutbidstream_enabled(at startup and atts config push, both tested), theholds_live_uiddoc line, and the exact token assertion. - ChristianPavilonis's two:
- The vendor-withdrawal expiry is in place; the 🔧 above is about its scope.
- A default
mirror_enabledis no longer serialized, with round-trip tests.
- prk-Jr's three:
provider_ownedagain means only an enabled configured provider.- The unused
logdependency is gone. - The pre-response row read is documented in the guide, the CHANGELOG and the description.
- Mine from round 2: all fixed. These were the spec drift, the CHANGELOG (including the Breaking pull-token entry),
Verification
- A scratch test at this head reproduces the cycle with two mirrored partners, one whose vendor is granted and one whose vendor is denied: navigation sets the mirror, a non-navigation expires it, and the next navigation sets it again.
- I applied the proposed fix in a scratch copy. It compiled and passed rustfmt and all 30 mirror and finalize tests (
wasm32-wasip1under Viceroy), and the cycle stopped. It has not been through the full adapter gate. The scratch changes were discarded. - The
CHANGELOG.mdsuggestion passes the repository's prettier check, and only ", pull sync skips them" changes. - The rebase onto #1487's split vendor gate holds:
keep_vendor_gatewrites the sameowner_tcf_vendor_idthe mirror reads, so a disabled provider's stored UIDs stay gated there as they are ineids.rsandidentify.rs.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): 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) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CI Node pin guard: PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- HTML EOF runtime (Fastly): PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- release scripts: PASS
- trace runtime and browser acceptance: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- worker-deadlines (cloudflare): PASS
- worker-deadlines (spin): PASS
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
29de3d4 to
d1440d6
Compare
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
d1440d6 to
6ffb613
Compare
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…he cap Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…rror exposure Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Pull sync now runs as a provider, so its UIDs are capped at MAX_STORED_UID_LENGTH like every provider write after the base branch split the ingest and stored limits. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Mirror: add a per-partner mirror_enabled opt-in, off by default, so a partner's UIDs can stay in the bidstream without reaching page scripts; leave out UIDs whose owning provider's TCF vendor consent is denied, as live_eids does for the bidstream. Pull sync: restore browser ts-eids and sharedId ingest for sources owned only by pull sync (the batch-sync split now covers browser ingest too, as ingest_rejected); gate after-response lookups on the real-browser check in prepare_after_response_lookups; skip the after-response row read when the request snapshot has nothing due (ProviderCoordinator::any_due) and the follow-up re-read when no result has anything to persist; log the per-navigation cap as skipped_concurrency_cap. Config: reject at startup a ts_pull_token that cannot form an Authorization header; define EcPartner::runs_pull_sync once and use it for ownership, the double-claim check and provider building; remove the unused pull_enabled_partners; cover the disabled-provider bypass of the double-claim check. Docs: describe where pull sync runs now, the mirror opt-in and vendor consent. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Mirror: expire a carried mirror where it cannot be rebuilt (non-navigations, or a navigation without a readable row) when the request denies TCF vendor consent to a mirrored partner's owning provider; reject mirror_enabled without bidstream_enabled at startup and at config push; omit a default mirror_enabled when serializing partner config so older binaries still load the blob. Registry: restore provider_owned to its base meaning (an enabled configured provider claims the source) and drop the duplicate ingest_rejected flag; record runs_pull_sync on PartnerConfig so the double-claim check is a single lookup. Cleanup: drop the unused log dependency of the identity-providers crate, restore holds_live_uid's doc line, assert the exact pull-token error. Docs: align the design spec with the mirror and pull sync as shipped, add CHANGELOG entries including the breaking pull-token validation, and note the row read the retired marker used to skip. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Coordinator tests that dispatch a lookup now use a live row, since a missing row skips every provider; restore the WriteFailingEcKv test import. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
A carried mirror was expired whenever any mirrored partner's owning vendor was denied, so a visitor who denied one of two mirrored vendors had the mirror set on each navigation and expired on the next non-navigation. Expire it only when it holds a source whose owning vendor is denied, or when its value does not decode. Drop the stale pull-sync clause from the provider CHANGELOG entry and align the guide and spec wording. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
… inputs The vendors base makes a provider endpoint optional, names each provider in its build inputs, passes request cookies and the clock to lookups and records opt-out memory in lookup results. Require an endpoint for pull sync and update the pull-sync tests and fixtures to match. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
… multi-vendor mirror gate Pull sync runs for existing deployments with no config change, but as a provider it stored UIDs up to 1024 bytes, which an older build rejects with the whole row on rollback. Ignore pull-sync UIDs over 512 bytes, as pull sync did before. The mirror is withheld while any TCF vendor gating its source is denied, including a disabled provider's. Test that case and align the docs, spec and changelog, which still named a single owning vendor. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ec1a6a2 to
d3780ba
Compare
Reverse the provider stack to #1487, #1488, #1489 and refresh the moved heads. Split the EdgeZero release so Axum serving does not wait for the record contract (R1b), add the ClientInfo contract (EZI-3) and JA4 (IABTechLab/edgezero#283) that ID-7 needs on Axum, and move the Axum side of providers, abuse protection and SharedID write-back into A10 so Fastly no longer waits on the Axum chain. Drop blockers that code at the PR heads shows are already met (#1189, #1556, #1141 before A1), move four-adapter and decision-gated issues out of the MVP, and align priorities and sizes. The MVP is now waves 0 to 9.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Pull sync now uses the after-response provider coordinator, and provider-owned IDs can be mirrored into an opt-in JavaScript-readable cookie. Three privacy/correctness issues remain: skipped provider-cookie opt-outs, missing GDPR-region mirror restrictions, and missed mirror expiry with split Cookie headers.
Two inline comments carry one-click GitHub
suggestionblocks. Use Commit suggestion or Add suggestion to batch to apply them. The remaining comment describes a coordinated multi-file fix that must be applied manually.
Blocking
🔧 wrench
- Process vendor-cookie opt-outs even when sources are held — see inline at
crates/trusted-server-core/src/ec/coordinator/mod.rs:321–330. - Apply GDPR-region exclusions to the mirror — see inline at
crates/trusted-server-core/src/ec/mirror.rs:67–71. - Capture the mirror from every Cookie header field — see inline at
crates/trusted-server-core/src/ec/mod.rs:217–220.
Verification
The four focused regression probes reproduced the findings on the reviewed head. Each approved suggestion (#1 and #3) was then applied alone to the original PR head and passed:
cargo fmt --all -- --check.- All eight clippy aliases: Fastly, Axum, Cloudflare native/WASM, Spin native/WASM, CLI and codegen.
cargo check-fastly && cargo check-axum && cargo check-cloudflare.cargo test-fastly && cargo test-fastly-reuse && cargo test-axum && cargo test-cloudflare && cargo test-spin.- The cross-adapter parity suite (29 tests).
- Its targeted regression probe: provider-cookie opt-out processing for #1, and combined/split Cookie-field mirror expiry for #3.
WASM tests used the repository-pinned Viceroy 0.21.0. Byte-exact pre/post verification patches matched for both suggestions, and all scratch changes were discarded. The multi-file proposal in #2 was not applied or scratch-verified.
Scope
Compared the stacked delta against feat/s2s-eid-vendors at d3780ba13dd598836bb9992145d170a19bdb124b. Whole-file inspection covered 24 of 36 changed/deleted files. The other 12 received diff/context inspection only, so this is not an exhaustive whole-file review.
Files without complete whole-file inspection
CHANGELOG.mdcrates/trusted-server-core/src/auction/endpoints.rscrates/trusted-server-core/src/config.rscrates/trusted-server-core/src/ec/admin.rscrates/trusted-server-core/src/ec/kv.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/settings.rsdocs/guide/configuration.mddocs/guide/ec-setup-guide.mddocs/guide/edge-cookies.mddocs/superpowers/specs/2026-10-30-s2s-eid-collection-design.mdtrusted-server.example.toml
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- cargo test (cross-adapter parity): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- trace runtime and browser acceptance: PASS
- browser integration tests: PASS
- vitest: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- Analyze (javascript-typescript): PASS
- cargo test (axum native): PASS
- cargo test: PASS (required)
- ts dev lint domains (changed lines): PASS
- CLAUDE.md symlink guard: PASS
- cargo fmt: PASS (required)
- HTML EOF runtime (Fastly): PASS
- format-typescript: PASS (required)
- CI Node pin guard: PASS
- docs-proposal scripts: PASS
- release scripts: PASS
- worker-deadlines (cloudflare): PASS
- format-docs: PASS (required)
- worker-deadlines (spin): PASS
- prepare integration artifacts: PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS
| /// Whether any provider has a due source in `entry`, ignoring consent and | ||
| /// rate limits, so a caller can skip a row read that cannot lead to a | ||
| /// lookup. `None` (no row known) counts every source as due. | ||
| #[must_use] | ||
| pub fn any_due(&self, entry: Option<&KvEntry>, now: UnixSeconds) -> bool { | ||
| self.providers.iter().any(|registered| { | ||
| !entry.is_some_and(|entry| entry.opted_out(®istered.id, now)) | ||
| && !Self::due_sources(registered, entry, now).0.is_empty() | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔧 wrench — Process vendor-cookie opt-outs even when sources are held
A configured provider can have work even when none of its sources is due for a lookup: dispatch reads its first-party cookies before the due check and also clears region-excluded sources. With a sole after-response SharedID provider whose source already holds a live UID, this returns false and run_after_response_lookups exits before noticing _pubcid_optout. The stored ID is therefore not removed; changed cookie IDs and cookie opt-back-in are skipped too.
Keep this optimization for pull sync, which only fills missing sources, but let configured providers process cookie and regional state on each permitted navigation.
| /// Whether any provider has a due source in `entry`, ignoring consent and | |
| /// rate limits, so a caller can skip a row read that cannot lead to a | |
| /// lookup. `None` (no row known) counts every source as due. | |
| #[must_use] | |
| pub fn any_due(&self, entry: Option<&KvEntry>, now: UnixSeconds) -> bool { | |
| self.providers.iter().any(|registered| { | |
| !entry.is_some_and(|entry| entry.opted_out(®istered.id, now)) | |
| && !Self::due_sources(registered, entry, now).0.is_empty() | |
| }) | |
| } | |
| /// Whether any provider may need after-response processing. | |
| /// | |
| /// Configured providers must still run to observe cookie changes, | |
| /// opt-outs and regional exclusions even when no source is due. | |
| /// Only pull sync can skip the row read when every source is held. | |
| #[must_use] | |
| pub fn any_due(&self, entry: Option<&KvEntry>, now: UnixSeconds) -> bool { | |
| self.providers.iter().any(|registered| { | |
| !registered.policy.pull_sync | |
| || (!entry.is_some_and(|entry| entry.opted_out(®istered.id, now)) | |
| && !Self::due_sources(registered, entry, now).0.is_empty()) | |
| }) | |
| } |
| for partner in partners.in_config_order().filter(|partner| { | ||
| partner.bidstream_enabled | ||
| && partner.mirror_enabled | ||
| && !partner.owner_vendor_consent_denied(consent) | ||
| }) { |
There was a problem hiding this comment.
🔧 wrench — Apply GDPR-region exclusions to the mirror
This filter omits partner.excluded_for(consent), which KvEntry::live_eids applies to bidstream egress. For example, a stored UID2 ID from a non-GDPR visit can still be mirrored to page scripts on a GDPR visit with Purposes 1 and 4 granted. Disabling its provider does not remove the registry's regional restriction, so relying on the coordinator to clear the row is insufficient. A regression probe produces zero bidstream EIDs but a populated mirror from the same inputs.
The carried-cookie path in ec/finalize.rs:152–156 also checks only vendor consent: a non-navigation response leaves an existing mirror containing the region-excluded source intact.
Proposed fix: apply the regional restriction both when building the mirror and when deciding whether a carried mirror must expire.
In ec/mirror.rs:
partner.bidstream_enabled
&& partner.mirror_enabled
&& !partner.owner_vendor_consent_denied(consent)
&& !partner.excluded_for(consent)In ec/finalize.rs:
let denied = |partner: &PartnerConfig| {
partner.bidstream_enabled
&& partner.mirror_enabled
&& (partner.owner_vendor_consent_denied(ec_context.consent())
|| partner.excluded_for(ec_context.consent()))
};Cover both new-mirror construction and carried-mirror expiry with regression tests.
Apply manually — this requires coordinated changes in two files and cannot be auto-applied as one suggestion.
| let mirror_cookie = jar | ||
| .as_ref() | ||
| .and_then(|j| j.get(COOKIE_TS_EIDS_MIRROR)) | ||
| .map(|cookie| cookie.value().to_owned()); |
There was a problem hiding this comment.
🔧 wrench — Capture the mirror from every Cookie header field
handle_request_cookies reads only headers.get(COOKIE), so this jar contains only the first field. Browsers may split cookies across multiple header fields, and the pinned Fastly adapter preserves those fields. If __Host-ts-eids-mirror is in a later field, finalization treats it as absent and emits no expiry after consent withdrawal, leaving its IDs readable by page scripts.
The combined-header regression control expires the cookie; the equivalent split-header request does not. Read every Cookie field when capturing the mirror.
| let mirror_cookie = jar | |
| .as_ref() | |
| .and_then(|j| j.get(COOKIE_TS_EIDS_MIRROR)) | |
| .map(|cookie| cookie.value().to_owned()); | |
| let mirror_cookie = req | |
| .headers() | |
| .get_all(http::header::COOKIE) | |
| .iter() | |
| .filter_map(|value| value.to_str().ok()) | |
| .flat_map(cookie::Cookie::split_parse) | |
| .filter_map(Result::ok) | |
| .find(|cookie| cookie.name() == COOKIE_TS_EIDS_MIRROR) | |
| .map(|cookie| cookie.value().to_owned()); |
|
CREATOR: From the server-side identity plan (#1559, |
Summary
__Host-ts-eids-mirrorcookie so publisher-side code can read them, on a best-effort basis. The mirror is opt-in per partner (mirror_enabled, default off) and leaves out UIDs whose owning provider's TCF vendor consent is denied.Stacked on #1487; review only the commits on top of it.
Changes
trusted-server-identity-providers/src/pull_sync.rs; removes coreec/pull_sync.rs/ec/pull_sync_marker.rsand the old post-send pull sync path in the Fastly adapter. Pull-sync providers are built withcollect = "after_response", so they only run for a real browser's EC whose consented row already existsec/mirror.rs;ec/finalize.rs,ec/cookies.rs,constants.rswrite the__Host-ts-eids-mirrorcookieapi_tokenkeeps batch syncSet-Cookie, so returning visitors' pages stay cacheablekind = "pull-sync"in[[ec.identity_providers]]is rejected; pull sync comes only from partner config with its token and rate limit__Host-ts-eids-mirrorjoins the identity cookies the template cache key refusesdocs/guide/edge-cookies.mdandconfiguration.mdMAX_STORED_UID_LENGTHfor provider writesmirror_enabledopt-in and vendor-consent filter for the mirror; browser ingest restored for pull-sync sources; real-browser gate inprepare_after_response_lookups; no after-response KV read when nothing is due (ProviderCoordinator::any_due) and no follow-up re-read when nothing is to persist;skipped_concurrency_cap; startup rejection of ats_pull_tokenthat cannot form anAuthorizationheader;EcPartner::runs_pull_sync;pull_enabled_partnersremovedBehaviour changes from
mainidentity_after_response_budget_ms(shared with other after-response providers), and only for an EC whose consented row already exists. Each pull used to have a 15-second first-byte timeout; a partner slower than the budget is now never filled./auctionresponses, on navigations without Fetch Metadata, or on prefetches.ts-eidsandsharedIdUIDs still fill a pull-sync partner's source; pull sync only fills a source that holds no UID.pull_sync_concurrencyis a cap per navigation. It used to be a batch width with every eligible partner pulled; partners now rotate hourly so each gets turns under the cap, and partners past it logskipped_concurrency_cap.0used to mean one request at a time (.max(1), every partner still pulled); it now means no limit.pull_sync_rate_limitis enforced per minute with a minimum of one on Fastly, so the default of 10 per hour behaves like 60.bidstream_enabledandmirror_enabledthey reach the script-readable mirror cookie. Previously they lived only in KV. The mirror is off by default.Set-Cookieis gone.ts_pull_tokenthat cannot form anAuthorization: Bearerheader now fails config validation instead of failing the provider build.Closes
Part of #1246 and #1245; the provider implementations that close them come in follow-up PRs.
Spec
Spec: reviewed separately; not included in this PR.
Test plan
cargo test-fastly && cargo test-fastly-reuse && cargo test-axum && cargo test-cloudflare && cargo test-spincargo fmt --all -- --checkscripts/lint-wasm-clock.sh./scripts/test-cli.sh, domain lint against the base branch, CI Node pin guard, docs-proposal scriptsfastly compute serveChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)Closes #1567