Remove fastly dependency from trusted-server-core (PR 15)#635
Conversation
BackendConfig uses fastly::backend::Backend directly, making it incompatible with the platform-portability goal. This commit: - Copies backend.rs verbatim into trusted-server-adapter-fastly, updating the one crate-internal import path - Adds url dependency to the adapter Cargo.toml - Rewires platform.rs and management_api.rs to use crate::backend - Removes pub mod backend from trusted-server-core/lib.rs - Migrates aps.rs, adserver_mock.rs, and prebid.rs off the deleted BackendConfig, using context.services.backend().ensure() for registration and a new predict_backend_name_for_url free function in integrations/mod.rs for stateless name prediction cargo check --workspace passes with zero errors.
All callers were migrated to predict_backend_name_for_url in core's integrations module. Remove the now-unused method and update the parse_origin doc comment that referenced it.
Remove crates/trusted-server-core/src/storage/ entirely (config_store.rs, secret_store.rs, mod.rs). All call sites migrated to PlatformConfigStore / PlatformSecretStore traits. Also drop the now-broken intra-doc links in trusted-server-adapter-fastly/src/platform.rs that referenced the deleted types.
Replace direct fastly::kv_store::KVStore usage in consent/kv.rs with a platform-neutral ConsentKvOps trait. The trait has four sync methods (load_entry, save_entry_with_ttl, fingerprint_unchanged, delete_entry) keeping the consent pipeline synchronous. - consent/kv.rs: add ConsentKvOps trait; replace load_consent_from_kv / save_consent_to_kv / delete_consent_from_kv with load_consent / save_consent (trait-based); remove open_store / fingerprint_unchanged private fns; drop ConsentKvMetadata / metadata_from_context (metadata API was Fastly-specific; fingerprint now lives in the body fp field); add StubKvOps + integration tests - consent/mod.rs: add kv_ops field to ConsentPipelineInput; update try_kv_fallback and try_kv_write to consume kv_ops instead of store_name; update all struct literal sites - publisher.rs: add kv_ops: Option<&dyn ConsentKvOps> parameter to handle_publisher_request; wire it into ConsentPipelineInput and use it for the SSC-revocation delete - adapter/platform.rs: add FastlyConsentKvStore wrapping fastly::kv_store::KVStore implementing ConsentKvOps via the sync API - adapter/app.rs + main.rs: open FastlyConsentKvStore from settings.consent.consent_store and pass as kv_ops to handle_publisher_request
Remove `fingerprint_unchanged` from `ConsentKvOps` trait so the common case (consent unchanged) costs one KV read instead of two. `save_consent` now loads the entry once and compares the fingerprint inline. Extract `open_consent_kv` helper in `app.rs` to deduplicate the two identical KV-open blocks. Replace bare `unwrap()` with descriptive `expect` messages in consent KV tests. Run `cargo fmt --all` to fix all whitespace issues.
Resolved five conflicts: - Deleted compat.rs (PR15 goal: remove Fastly from core) - integrations/mod.rs: kept both PR15's ensure_integration_backend_with_timeout / predict_backend_name_for_url and PR14's INTEGRATION_MAX_BODY_BYTES constant - integrations/adserver_mock.rs, aps.rs, prebid.rs: merged collect_body import (PR14) with ensure_integration_backend_with_timeout / predict_backend_name_for_url imports (PR15) into single use statements
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR delivers its stated goal — trusted-server-core is now fully fastly-free and the consent pipeline is abstracted behind a minimal ConsentKvOps trait. Local verification passes (fmt, clippy -D warnings, cargo test --workspace, wasm32-wasip1 release build).
Three concerns merit changes before merge: a diagnostic regression in predict_backend_name_for_url, duplication of the security-adjacent SPOOFABLE_FORWARDED_HEADERS list, and a test-coverage regression in the relocated adapter compat.rs. The remaining items are non-blocking observations and follow-up candidates.
Note: this PR targets
feature/edgezero-pr14-entry-point-dual-path, notmain; review scope is the 31-file PR-15-specific delta (+2230/-2165).
Blocking
🔧 wrench
- Diagnostic regression in
predict_backend_name_for_url— inline comment atcrates/trusted-server-core/src/integrations/mod.rs:135 SPOOFABLE_FORWARDED_HEADERSduplicated across core and adapter — inline comment atcrates/trusted-server-adapter-fastly/src/compat.rs:18- Adapter
compat.rshas zero tests after relocation (14 security-relevant tests dropped) — inline comment atcrates/trusted-server-adapter-fastly/src/compat.rs:82
Non-blocking
🤔 thinking
- KV errors logged at
debuglevel as "lookup miss" — inline comment atcrates/trusted-server-adapter-fastly/src/platform.rs:420 - Backend-name compute logic duplicated between
predict_backend_name_for_url(core) andBackendConfig::compute_name(adapter) — inline comment atcrates/trusted-server-core/src/integrations/mod.rs:130
🌱 seedling
- Redundant KV read on fallback+save path — inline comment at
crates/trusted-server-core/src/consent/kv.rs:267 - Hardcoded
certificate_check: trueinensure_integration_backend(integrations/mod.rs:69) andensure_integration_backend_with_timeout(integrations/mod.rs:114). No regression vs. pre-PR behaviour, but the siblingpredict_backend_name_for_urldoes takecertificate_check: bool— asymmetric. If any integration ever needs a self-signed upstream (test/on-prem), these helpers would need to grow that parameter.
📌 out of scope
migration_guards.rsnot extended — migration_guards.rs:27-46 only covers 12 files and a narrow regex (Request|Response|http::Method|StatusCode|mime::APPLICATION_JSON). With core'sfastlydep now removed, this guard should either broaden to\bfastly::across all core files, be replaced by a Cargo-metadata dependency-presence test (more robust, no false negatives), or be deleted as structurally redundant. Follow-up issue.FastlyConsentKvStore::opensilent failure — inline comment atcrates/trusted-server-adapter-fastly/src/platform.rs:409
📝 note
- Stale "PR 15" forward references:
- adapter-fastly/src/app.rs:150: "will be removed when
legacy_mainis deleted in PR 15". This is PR 15 andlegacy_mainwas not deleted. - adapter-fastly/src/main.rs:113: "
compat.rswill be deleted in PR 15 once this legacy path is retired". This is PR 15;compat.rswas moved (not deleted) and the legacy path was not retired.
Update comments to reference the actual future PR number or drop the promise.
- adapter-fastly/src/app.rs:150: "will be removed when
⛏ nitpick
- Unnecessary
u16type suffix — inline comment atcrates/trusted-server-core/src/integrations/mod.rs:140
CI Status
- fmt: PASS
- clippy (
-D warnings): PASS - rust tests (
cargo test --workspace): PASS (800+ tests) - wasm32-wasip1 release build: PASS
- PR-reported checks (browser integration, integration tests, prepare integration artifacts): PASS
…mpat tests - predict_backend_name_for_url now returns Result<String, Report<TrustedServerError>> instead of Option<String>; call sites use inspect_err(...).ok() to preserve the Option<String> trait interface while logging the actual failure reason - Promote SPOOFABLE_FORWARDED_HEADERS to pub in http_util; replace inline copy in compat.rs with a use import — single source of truth for the strip list - Add sanitize_fastly_forwarded_headers_strips_spoofable test (all 4 entries + Host preserved) and to_fastly_response_with_streaming_body_produces_empty_body test (pins silent-drop behaviour) to compat.rs - Change Consent KV non-ItemNotFound error log from debug "lookup miss" to warn "lookup failed" for operational visibility - Drop stale "will be removed in PR 15" forward references from app.rs and main.rs - Drop unnecessary u16 type suffixes on port literal defaults
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
- Submitted reviewer-approved findings for consent/KV handling, backend naming consistency, and Fastly wiring.
- Verdict: REQUEST_CHANGES (includes a P1 finding).
Findings moved to review body
The following approved findings were included in the review body because GitHub could not resolve their requested inline locations in the current diff:
-
🤔 Consent bootstrap logic is now duplicated across auction and publisher paths (
crates/trusted-server-core/src/auction/endpoints.rs:61)
Both handlers now repeat the same sequence: parse cookies, generate/read synthetic ID, geo lookup with identical error handling, and build ConsentPipelineInput with kv_ops. This duplication is likely to drift as the consent pipeline evolves; this PR already had to touch both call sites just to thread the new abstraction through.
Suggestion: Extract a small shared helper that prepares request-scoped consent state once and returns the ConsentContext plus any reused intermediates such as synthetic_id, geo, and cookie_jar. -
⛏ Public docs/comments still lag the new kv_ops contract (
crates/trusted-server-core/src/auction/endpoints.rs:21)
The new kv_ops parameter materially changes how consent persistence is enabled, but the public docs still describe these handlers mostly in their pre-PR form, and one consent comment still says fallback requires consent_store rather than kv_ops.
Suggestion: Update the affected doc comments to explain that consent KV fallback/write-through occurs only when a concrete kv_ops implementation is supplied, and adjust the stale internal comment in consent/mod.rs.
|
Fixed the two findings GitHub moved into the review body in 3a53f33.
Fresh verification on this commit:
|
…ro-pr15-remove-fastly-core
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Approving this PR. The main refactor looks solid: trusted-server-core is decoupled from Fastly and the adapter now owns the Fastly-specific backend, KV, and compatibility wiring.
I left 2 non-blocking follow-up comments inline for logging/test coverage.
Incorporates the round-3 review fixes from PR14 (E2E tests, finalize-header coverage for unregistered methods, allowed_domains documentation) and the broader PR14 refactoring (EC ID rename from synthetic_id, kv_ops removal from handle_auction, runtime_services_for_consent_route consent KV approach, async tokio tests in proxy/orchestrator, PublisherResponse streaming). Conflict resolutions: - Take PR14's refactored core files (endpoints, orchestrator, consent, publisher, proxy, platform/mod, testlight) - Keep PR15's deletion of core/compat.rs and storage/mod.rs (Fastly code stays in adapter, not core) - Add from_fastly_response to adapter compat.rs (needed by EdgeZero path for entry-point finalize-header wrap introduced in PR14) - Remove obsolete FastlyConsentKvStore / open_consent_kv / ConsentKvOps references from platform.rs and app.rs (trait removed in PR14) - Fix orchestrator backend_name call to pass services (PR15 trait signature) - Add minimal backend.rs and storage/mod.rs stubs to core for DEFAULT_FIRST_BYTE_TIMEOUT and storage::kv_store module path - Add tokio as dev-dependency to trusted-server-core for async tests
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of PR 15 after the most recent commits (Apr 27). The core refactor goal is achieved — trusted-server-core no longer imports fastly, and the migration guard test (migration_guards.rs) durably enforces the invariant. Build, clippy, fmt, and the full Rust test suite (878 tests) pass locally; GitHub CI is green.
However, the merge with PR14 removed the ConsentKvOps / FastlyConsentKvStore abstraction without replacing the wiring on the EdgeZero entry path, leaving a privacy-sensitive gap: when edgezero_enabled = true and consent_store is configured, consent revocation deletes silently fail. The PR description still describes the pre-merge design and is now misleading.
Requesting changes to address (or explicitly accept and document) the EdgeZero consent-KV gap and to update the PR description before merge.
Blocking
❓ question
Consent KV is not opened on the EdgeZero entry path — crates/trusted-server-adapter-fastly/src/app.rs:92
build_state() constructs kv_store = Arc::new(UnavailableKvStore) unconditionally, and build_per_request_services (lines 111–127) hands that same store to every handler. The runtime_services_for_consent_route helper in crates/trusted-server-adapter-fastly/src/main.rs:307-323 (which actually opens the configured consent.consent_store) is only called from the legacy route_request.
Consequence: when edgezero_enabled = true and [consent] consent_store = "..." is configured, every services.kv_store() call in core returns KvError::Unavailable. In particular, the revocation delete in crates/trusted-server-core/src/publisher.rs:765 silently fails (Failed to delete consent KV entry … Unavailable) — the EC cookie expires, but the persisted consent data is not removed. KV-backed consent fallback also never engages on the EdgeZero path. This is a privacy/compliance concern (right-to-erasure).
This appears to be a regression from the merge resolution that removed FastlyConsentKvStore / open_consent_kv after merging PR14. Either (a) the EdgeZero path needs an equivalent of runtime_services_for_consent_route (open the consent store inside build_per_request_services or wrap the auction/publisher handlers), or (b) the EdgeZero path should fail loudly at startup when consent.consent_store is configured but cannot be opened.
Was this intentional, or did it slip through the merge?
🔧 wrench
PR description is stale and contradicts the merged code — The body still claims:
- "Introduces a sync
ConsentKvOpstrait in core" - "
FastlyConsentKvStorein the adapter implements the trait" - "Threads
kv_ops: Option<&dyn ConsentKvOps>throughConsentPipelineInput"
None of these exist after the PR14 merge — the actual code uses PlatformKvStore directly via RuntimeServices, and the consent KV is opened only on the legacy path. The "Changes" table for consent/kv.rs, consent/mod.rs, app.rs, main.rs, and platform.rs describes work that was reverted. Reviewers and git log consumers will be misled. Please update the PR description to reflect the post-PR14-merge reality before merging.
Non-blocking
🤔 thinking
UnavailableKvStore swallows configuration errors silently on the EdgeZero path — Combined with the question above, configuring consent_store = "missing-store" on the EdgeZero path produces no startup error and only per-request warn! lines. The legacy path correctly fails the auction/publisher routes with 503 (verified by route_tests.rs:184). Worth validating consent-store availability at startup so misconfiguration cannot silently disable revocation.
⛏ nitpick
Pre-existing: core is on edition = "2021" — CLAUDE.md mandates 2024 edition. Not introduced by this PR; flagged only for awareness/follow-up.
CI Status
- fmt: PASS (local + GitHub)
- clippy: PASS (local,
-D warnings) - rust tests: PASS (878 tests, local)
- GitHub CI: PASS (browser tests, integration tests, prepare integration artifacts)
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of PR 15 against HEAD 725ccdc2. Local verification passes (fmt, clippy -D warnings, 878 + 45 unit tests across the changed crates) and GitHub CI is green.
The core refactor goal is achieved — trusted-server-core no longer imports fastly and the migration guard test enforces the invariant. However, the previously flagged EdgeZero consent-KV regression (Apr 30 review) is still present at the same line, and the PR description still describes the pre-PR14-merge design that was reverted. Requesting changes to address (or explicitly accept and document) those before merge.
Blocking
🔧 wrench
-
EdgeZero path silently disables consent KV (privacy/compliance regression) —
crates/trusted-server-adapter-fastly/src/app.rs:92(line outside this PR's diff hunk, so flagging at body level).build_state()hardcodesArc::new(UnavailableKvStore), andbuild_per_request_services(lines 111–127) hands that exact store to every route. There is no analogue ofruntime_services_for_consent_route(main.rs:307-323) on this path.Concrete effect when
edgezero_enabled = trueAND[consent] consent_store = "..."is set:crates/trusted-server-core/src/publisher.rs:765callsdelete_consent_from_kv(services.kv_store(), cookie_ec_id), which hitsUnavailableKvStore.delete()→KvError::Unavailable.storage/kv_store.rs:323-332swallows the error (warn-log only). The EC cookie expires but the persisted consent is not removed — right-to-erasure broken.crates/trusted-server-core/src/auction/endpoints.rs:83-87andpublisher.rs:508-512passSome(services.kv_store())to the consent pipeline, but it's the unavailable store, so consent persistence and KV-backed consent fallback never engage.
Compare with the legacy path (correct):
route_tests.rs:184-260verifies that a missingconsent.consent_storeproduces a fail-closed 503 on/auctionand the publisher fallback viaroute_request. EdgeZero has no equivalent and silently degrades. Same blocking finding as the Apr 30 review, at the same line, still present at HEAD725ccdc2.Fix options:
- Open
consent.consent_storeinsidebuild_state()(or inbuild_per_request_services) usingplatform::open_kv_store, and either (a) fail startup if configured-but-unavailable or (b) wrap the consent-touching handlers (/auction, fallback) so they return 503 like the legacy path. - Mirror
runtime_services_for_consent_routeat the EdgeZero handler boundaries.
Either way, please add a counterpart of the existing
configured_missing_consent_store_only_breaks_consent_routestest that drivesTrustedServerApp::routes()so the regression cannot slip through another merge. -
PR description is stale and contradicts the merged code — the body still claims the PR introduces a sync
ConsentKvOpstrait, aFastlyConsentKvStoreadapter implementation, and threadskv_ops: Option<&dyn ConsentKvOps>throughConsentPipelineInput. None of those exist after the PR14 merge — the actual code usesPlatformKvStoredirectly viaRuntimeServices. The "Changes" table forconsent/kv.rs,consent/mod.rs,app.rs,main.rs, andplatform.rsdescribes work that was reverted. Please update the description to reflect the post-PR14-merge reality before merging so reviewers andgit logconsumers aren't misled.
Non-blocking
🤔 thinking
- No EdgeZero-side test for consent-store wiring —
route_tests.rs:184 configured_missing_consent_store_only_breaks_consent_routesonly exercises the legacyroute_request. There is no analogous test that constructsTrustedServerApp::routes()withconsent.consent_storeconfigured. Combined with the blocking finding above, the regression slipped through the PR14 merge with no failing test to catch it. - Both crates still on
edition = "2021"— CLAUDE.md mandates 2024 edition. Pre-existing across the repo, not introduced by this PR; flagged for awareness/follow-up.
📌 out of scope
UnavailableKvStoremakes consent-store misconfiguration invisible at startup — even after the blocking finding is addressed, configured-but-unopenable stores will only surface as per-requestwarn!lines unlessbuild_state()validates store availability at startup. Worth a follow-up to fail loudly whenconsent.consent_storeis set but cannot be opened.
CI Status
- fmt: PASS (local)
- clippy: PASS (local,
-D warnings) - rust tests: PASS (878 + 45 in changed crates)
- GitHub CI: PASS (browser tests, integration tests, prepare integration artifacts)
|
Addressed the body-level blocking findings from the Apr 30 and May 6 reviews in 50dc61f.
Fresh verification before commit:
|
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR cleanly removes the fastly crate dependency from trusted-server-core, relocates the compat/backend layer into the adapter, migrates four integrations (aps, prebid, adserver_mock, sourcepoint) off BackendConfig to the platform-trait helpers, and restores the fail-closed consent KV wiring on the EdgeZero path for /auction and publisher fallback only. CI is green and local fmt/clippy/cargo test --workspace/wasm32-wasip1 build all pass.
Requesting changes on the items below — none are correctness-breaking today, but two of them (the select empty-backend-name fallback and the predict/ensure parameter asymmetry) sit on the bid-correlation path and can silently misbehave if upstream conditions shift.
Blocking
🤔 thinking — empty backend name silently propagates from select
crates/trusted-server-adapter-fastly/src/platform.rs:300-307 — when fastly_resp.get_backend_name() returns None, the code logs a warning and falls back to "". The PlatformResponse then carries backend_name == "", and AuctionOrchestrator correlates responses by exact string match against provider.backend_name(...). A bid response with "" will silently fail correlation — only the warning log signals the issue to operators.
Consider returning PlatformError::HttpClient here instead, so the orchestrator gets an explicit error frame for that bidder rather than a phantom non-match:
let ready = match result {
Ok(fastly_resp) => match fastly_resp.get_backend_name() {
Some(backend_name) => fastly_response_to_platform(fastly_resp, backend_name.to_string()),
None => Err(Report::new(PlatformError::HttpClient)
.attach("select: response has no backend name; correlation impossible")),
},
Err(e) => Err(Report::new(PlatformError::HttpClient)
.attach(format!("fastly select error: {e}"))),
};(Section unchanged in this PR, so flagging in body rather than inline — fix in this PR or open a follow-up.)
🌱 seedling — no compile-time guard that services.kv_store() is the consent KV
crates/trusted-server-core/src/publisher.rs:764-766 — correctness relies on the caller having threaded runtime_services_for_consent_route(...) before handle_publisher_request. Today the wiring is right (legacy route_request and EdgeZero dispatch_fallback's publisher branch both do it), but there's no compile-time guard. If a future entry point forgets, services.kv_store() will be UnavailableKvStore and delete will succeed-as-noop, leaving stale consent KV entries after a user revokes.
A typed ConsentKvStore(Arc<dyn PlatformKvStore>) newtype constructed only by runtime_services_for_consent_route would make the wiring mistake a compile error. Worth doing while this surface is fresh.
Non-blocking
♻️ refactor — duplicate enable_ssl().sni_hostname(self.host) in BackendConfig::ensure
crates/trusted-server-adapter-fastly/src/backend.rs:177-188 — enable_ssl().sni_hostname(self.host) is called once unconditionally, then a second time inside the if self.certificate_check branch before .check_certificate(host). Idempotent on the builder, so functionally fine, but easy to simplify on the file's move into the adapter:
builder = builder.enable_ssl().sni_hostname(self.host);
if self.certificate_check {
builder = builder.check_certificate(self.host);
} else {
log::warn!(
"INSECURE: certificate check disabled for backend: {}",
backend_name
);
}
log::info!("enable ssl for backend: {}", backend_name);Pre-existing in the deleted core copy; the relocation is the natural moment to clean it up. (Section unchanged in the rename diff, so flagging in body rather than inline.)
📝 note — AppState.kv_store is now always UnavailableKvStore
crates/trusted-server-adapter-fastly/src/app.rs:103 and the matching build_per_request_services site — per-route consent stores are opened on demand via runtime_services_for_consent_route, and no other consumer reads services.kv_store(). The field is now a placeholder, but its name reads as if it were the primary KV. Consider either inlining Arc::new(UnavailableKvStore) at the two construction sites, or renaming the field to default_kv_store to signal that it's only the fallback when no consent route override applies.
📝 note — three AuctionProvider::backend_name impls replicate identical bodies
aps.rs, prebid.rs, adserver_mock.rs all paste essentially the same body wrapping predict_integration_backend_name(&self.config.{endpoint,server_url}, …). A default method on AuctionProvider parameterised by (endpoint: &str, integration_id: &'static str) would remove the boilerplate; doing so would also make the dead certificate_check parameter (see the inline ♻️ refactor on integrations/mod.rs) impossible to misuse.
⛏ nitpick — from_fastly_request panics on URL parse failure
crates/trusted-server-adapter-fastly/src/compat.rs:11-28 — the deleted crates/trusted-server-core/src/compat.rs parsed the URL with a / fallback and warning log; this version .expects. Fastly delivers validated URLs in practice, and the # Panics doc is honest, so this is defensible — but the previous "log + continue" was strictly more defensive on a path that processes every inbound request. Preference call.
CI Status
- fmt: PASS
- clippy: PASS (no warnings)
- rust tests: PASS (950 + 54 + 21 + 3, doctests included)
- wasm32-wasip1 release build: PASS
- GitHub Actions: PASS (browser-integration, integration, prepare-integration-artifacts)
migration_guards.rsinvariant (nofastly::Request/Response/...in migrated core modules): PASS- Verified zero
use ... fastlyimports remain anywhere undercrates/trusted-server-core/src/
Blocking: - select(): return Err(PlatformError::HttpClient) when get_backend_name() returns None instead of silently propagating "" and losing bid correlation - Remove dead certificate_check: bool from predict_integration_backend_name; hardcode true inside to match ensure_integration_backend_with_timeout so predict/ensure cannot silently diverge and break orchestrator correlation Non-blocking: - Fix duplicate enable_ssl().sni_hostname() in BackendConfig::ensure; call once unconditionally, then add check_certificate() conditionally - Add debug_assert!(false) in to_fastly_response Stream arm to catch caller misuse in debug production builds; gated with #[cfg(not(test))] so the behavior-documentation test still passes - Add integration-route assertion to edgezero_missing_consent_store test: GET /integrations/datadome/tags.js must not return 503 when consent KV is misconfigured, locking in that integration proxy bypasses consent gating - Rename AppState.kv_store -> default_kv_store to signal it is only the fallback when no per-route consent KV override applies
Conflicts resolved: - app.rs: keep PR15's default_kv_store field rename; use PR14's build_state_from_settings in the test helper - compat.rs (modify/delete): accept PR15 deletion — file was relocated to the adapter crate as part of removing fastly from core - adserver_mock.rs, aps.rs, prebid.rs: PR15 wins — use ensure_integration_backend_with_timeout for auction-scoped timeouts and keep predict_integration_backend_name import - sourcepoint.rs: merge both sides — keep ensure_integration_backend (PR15, CDN proxy backend) and collect_response_bounded (PR14, streaming response fix); add missing None timeout arg - proxy.rs: PR15 wins on import; add enforce_max_body_size from PR14 (DEFAULT_FIRST_BYTE_TIMEOUT already available via platform module) - mod.rs: use PR15's integration_backend_spec helper in ensure_integration_backend body; honor PR14's Option<Duration> param by passing it through to the helper
|
All round-1 findings resolved in commit 72ddc42. Blocking — fixed
Non-blocking — addressed
Deferred (acknowledged)
CI: |
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of PR 15 (remove fastly from trusted-server-core), reviewed against its actual base feature/edgezero-pr14-entry-point-dual-path to isolate this PR's 32-file changeset. Round-1 findings are addressed (select() None→Err, predict/ensure unified via integration_backend_spec, integration-route consent-bypass test, default_kv_store rename). One requested change remains: the tokio test-migration is half-done and leaves the codebase with two inconsistent async-test conventions. The remaining items are non-blocking.
Blocking
🔧 wrench
- Incomplete tokio removal: GTM tests converted to
block_on(~672 lines) butproxy.rsstill uses#[tokio::test]andtokioremains a dev-dependency — finish the conversion + drop the dep, or revert the churn. (Cargo.toml:79, google_tag_manager.rs, proxy.rs:1520)
Non-blocking
🤔 thinking
from_fastly_requestnow panics on unparseable URL: changed from"/"fallback to.expect(); confirm intentional. (adapter/compat.rs:15)
🌱 seedling
- Speculative
getrandom/uuidjsdeps forwasm32-unknown-unknown, a target no CI job builds. (core/Cargo.toml:56)
⛏ nitpick
- Stale
fastly::doc-comment references missed by the cleanup commit:proxy.rs:1485,proxy.rs:2386(fastly::Response),platform/mod.rs:28(fastly::Body).streaming_processor.rswas updated to "any platform body type"; these were missed.
CI Status
The PR's GitHub CI ran only the 3 integration-test jobs (it targets a feature branch, so the standard gates didn't trigger). Verified locally against the PR head:
- fmt: PASS
- clippy (
--all-targets --all-features -D warnings): PASS - rust tests: PASS (core 950, adapter 54)
- wasm32-wasip1 release build: PASS
Migrates ec/auth.rs, ec/batch_sync.rs, and ec/identify.rs from fastly::Request/Response to http::Request<EdgeBody>/Response<EdgeBody>. Updates the adapter entry point (main.rs) to extract DeviceSignals before the fastly→http conversion and pass them as a new parameter to route_request. Fixes adapter and core tests broken by the merge: - Replace [edge_cookie] secret_key with [ec] passphrase in all test TOML fixtures (app.rs, route_tests.rs, google_tag_manager.rs) - Update route_tests.rs to match PR14 semantics: rename test to routes_use_request_local_consent, remove stale consent-store 503 assertions, use /_ts/admin path - Switch dispatch_auth_rejected_401_carries_finalize_headers to use routes_for_state with test settings so handler regex covers /admin path - Remove unused from_fastly_response from compat.rs - Fix mut variable, orphaned doc comment, and missing # Panics sections flagged by clippy - Mark get_or_generate_ec_id as #[cfg(test)] (test-only wrapper)
Convert all remaining #[tokio::test] tests to futures::executor::block_on across proxy.rs, publisher.rs, auction/endpoints.rs, auction/orchestrator.rs, and integrations/testlight.rs. Drop tokio from [dev-dependencies] in trusted-server-core/Cargo.toml — tokio is no longer referenced anywhere in the crate. Also fix two stale doc comments in proxy.rs that still referenced fastly::Response and platform_response_to_fastly.
|
Addressing the findings from round-4 review: 🔧 Blocking — Incomplete tokio removalFixed in The conversion was broader than just
🤔 Non-blocking —
|
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of PR 15 against HEAD cedea8b1, diffed against its actual base feature/edgezero-pr14-entry-point-dual-path. The previously blocking item — incomplete tokio removal — is resolved: 0 #[tokio::test] remain, tokio is gone from core's manifest, and the GTM conversion is lossless (9→9 tests, 14→14 assertions, 10→10 awaits). Stale fastly:: doc references are fixed.
This pass surfaced four new issues, two blocking. The core refactor mechanics (compat/backend relocation, platform-abstraction threading, consent-route fail-closed 503 wiring, deterministic backend naming) are sound and well-covered — those are not repeated here.
Blocking
🔧 wrench
from_fastly_requestpanics on an unparseable URL — regressed from a graceful"/"fallback to.expect(), on the default legacy path. (adapter/compat.rs:15)- Duplicate, IPv6-divergent EC-ID generator —
edge_cookie.rsnow carries its own HMAC generator whose/64normalization disagrees with canonicalec::generation::normalize_ip; same passphrase ⇒ IPv4 IDs match but IPv6 IDs diverge, minting non-correlating identities. (edge_cookie.rs:39vsec/generation.rs:35) - Consent-KV read-fallback / write-on-change advertised but unwired —
ConsentPipelineInput.{ec_id,kv_store}andload_consent_from_kv/save_consent_to_kvare dead in production;build_consent_contextignores the fields. (consent/mod.rs:82,storage/kv_store.rs)
❓ question
- PR goal unmet:
trusted-server-corestill depends onfastly—core/Cargo.toml:25still declares it, kept alive byec/kv.rs(fastly::kv_store) andec/rate_limiter.rs(fastly::erl). The changes-table claim that core'sfastlydependency is removed is currently false — is the EC KV/ERL migration deferred?
Non-blocking
🏕 camp site
- Orphaned
crates/trusted-server-core/src/backend.rs—lib.rsdroppedpub mod backend;, but the ~16 KB file is still tracked, no longer compiled, and still carriesuse fastly::backend::Backend. Should begit rm'd alongside thelib.rschange.
📝 note
- Duplicated
DEFAULT_FIRST_BYTE_TIMEOUT— added atplatform/mod.rs:58while an identicalpub(crate)const remains in the orphanedbackend.rs:37. Collapses to one definition once the dead file above is removed. - Removed
lgtm[rust/cleartext-logging]suppressions inpublisher.rs— if CodeQL/lgtm still runs in CI, confirm dropping these doesn't re-introduce cleartext-logging alerts on the affected debug logs.
CI Status
The PR's GitHub CI ran only the 3 integration-test jobs (it targets a feature branch, so the standard fmt/clippy/test gates didn't trigger). All 3 PASS.
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
| let uri: Uri = req | ||
| .get_url_str() | ||
| .parse() | ||
| .expect("should parse fastly request URL as URI"); |
There was a problem hiding this comment.
🔧 wrench — Availability regression on the default path. The base version of this code (in core compat.rs) parsed the URL with a graceful fallback and documented "Does not panic in practice":
let uri: http::Uri = req.get_url_str().parse().unwrap_or_else(|_| {
log::warn!("Failed to parse request URL '{}'; falling back to '/'", req.get_url_str());
http::Uri::from_static("/")
});This PR changed it to .expect(...). from_fastly_request runs on the legacy path (main.rs:340), which is the default (is_edgezero_enabled returns false when the flag is absent). A URL that Fastly accepts but http::Uri rejects now panics the entire Wasm instance instead of degrading — the EdgeZero path (into_core_request) correctly returns a Result and is handled with a 500. (Flagged as 🤔 in the Apr-30/Jun-3 reviews — escalating to blocking now that it's confirmed on the hot path.)
Fix — restore the non-panicking fallback and add a regression test feeding a URL that http::Uri rejects:
let uri: Uri = req.get_url_str().parse().unwrap_or_else(|_| {
log::warn!("failed to parse fastly request URL '{}'; falling back to '/'", req.get_url_str());
Uri::from_static("/")
});and update the # Panics doc accordingly.
| let segments = ipv6.segments(); | ||
| // Keep only the first 4 segments (64 bits) for /64 prefix | ||
| format!( | ||
| "{:x}:{:x}:{:x}:{:x}::", |
There was a problem hiding this comment.
🔧 wrench — Duplicate, IPv6-divergent EC-ID generator. This PR rewrites edge_cookie.rs (base delegated generation to crate::ec::generation) to carry its own HMAC generator and wires it into the publisher (publisher.rs:249/485). Both generators hash normalize_ip(client_ip) with the same settings.ec.passphrase via HMAC-SHA256, but the /64 normalization disagrees:
- here:
format!("{:x}:{:x}:{:x}:{:x}::", ...)→2001:db8:85a3:0:: - canonical
ec::generation::normalize_ip:format!("{:04x}{:04x}{:04x}{:04x}", ...)→20010db885a30000
IPv4 IDs coincide; IPv6 IDs diverge, so the same IPv6 client receives two non-correlating 64-hex identity prefixes depending on which path runs. The canonical module documents its format as "a stable contract — EC hashes stored in KV depend on it" and warns that a mismatched normalization "would create duplicate identity graph entries."
Fix — don't duplicate. Delegate to the canonical implementation (both are pub(crate) in the same crate):
use crate::ec::generation::{generate_ec_id as generate_canonical_ec_id, normalize_ip};and call the canonical normalize_ip/generate_ec_id so there is a single normalization + HMAC path.
| /// When set along with `kv_store`, enables: | ||
| /// - **Read fallback**: loads consent from KV when cookies are absent. | ||
| /// - **Write-on-change**: persists cookie-sourced consent to KV. | ||
| pub ec_id: Option<&'a str>, |
There was a problem hiding this comment.
🔧 wrench — Advertised but unwired (dead code). These fields promise "Read fallback: loads consent from KV when cookies are absent" and "Write-on-change: persists cookie-sourced consent to KV", and the publisher populates them (publisher.rs:504-505). But build_consent_context — the only consumer of ConsentPipelineInput — never reads ec_id or kv_store (its own doc admits "does not load persisted consent from KV and does not persist consent to KV"). Consequently load_consent_from_kv and save_consent_to_kv are called only by their own unit tests — dead in production. The PR description lists "ConsentPipelineInput accepts an optional PlatformKvStore reference for KV fallback/write-through" as a delivered change; it isn't.
Fix — either wire the read-fallback/write-on-change into build_consent_context (call load_consent_from_kv when signals are empty and ec_id+kv_store are present; save_consent_to_kv after building a cookie-sourced context) with pipeline-level tests, or drop the unused fields/functions/docs so the API doesn't advertise behavior that doesn't exist. (The revocation delete path in apply_ec_headers is separately wired and correctly tested — that part is fine.)
|
|
||
| let fp = consent_fingerprint(ctx); | ||
|
|
||
| if fingerprint_unchanged(store, ec_id, &fp) { |
There was a problem hiding this comment.
🤔 thinking — fingerprint_unchanged does a full get_bytes + JSON deserialize, then save does a put — two blocking KV round-trips per consent change on the hot path. Correct as written, but once the read-fallback wiring exists (see the ec_id/kv_store finding), the entry loaded during read-fallback can be reused here instead of a second fetch. Not blocking.
| // Uses context.timeout_ms (auction-scoped) rather than the 15 s fixed | ||
| // timeout in ensure_integration_backend, which is for proxy endpoints. | ||
| // Send request asynchronously with auction-scoped timeout | ||
| let backend_name = ensure_integration_backend_with_timeout( |
There was a problem hiding this comment.
⛏ nitpick — Inconsistent error attribution. aps and adserver_mock wrap an ensure_integration_backend_with_timeout failure in TrustedServerError::Auction with a descriptive "Failed to resolve backend for ... endpoint: {endpoint}", but prebid propagates the raw Integration error via bare ?. For uniform auction error attribution, apply the same .change_context(TrustedServerError::Auction { ... }) wrapper here.
| ); | ||
|
|
||
| response.append_header(header::SET_COOKIE, create_ec_cookie(settings, ec_id)); | ||
| if let Ok(val) = HeaderValue::from_str(&create_ec_cookie(settings, ec_id)) { |
There was a problem hiding this comment.
⛏ nitpick — The migration replaces the infallible append_header with if let Ok(val) = HeaderValue::from_str(...), which silently no-ops the Set-Cookie on failure. Unreachable in practice (is_safe_cookie_value + the debug_assert already gate the value, and format_set_cookie emits only controlled bytes), so this is safe — but add a log::warn! on the else branch for defense-in-depth symmetry with the rejection logging above.
| // the hard upper bound until a per-request timeout API is available. | ||
| let response = match pending.pending.wait() { | ||
| let select_result = | ||
| match futures::executor::block_on(services.http_client().select(vec![pending.pending])) |
There was a problem hiding this comment.
♻️ refactor — This inline block_on(services.http_client().select(vec![pending.pending])) + .ready exactly duplicates the PlatformHttpClient::wait() default method (platform/http.rs). Replacing it with services.http_client().wait(pending.pending) (still block_on-wrapped) is clearer and behavior-preserving — concurrency is unchanged since all requests are dispatched up front via send_async.
3 participants
Summary
fastlycrate imports fromtrusted-server-core, keeping the shared crate platform-agnostic and enforcing that invariant withmigration_guards.rstrusted-server-adapter-fastlyRuntimeServices,PlatformBackendSpec,PlatformKvStore) for core request handling instead of Fastly SDK types/auctionand publisher fallback routes, matching the legacy path's fail-closed503behavior when the store is unavailableChanges
crates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/storage/crates/trusted-server-core/src/geo.rscrates/trusted-server-core/src/storage/kv_store.rsPlatformKvStorerather than Fastly KV typescrates/trusted-server-core/src/consent/mod.rsConsentPipelineInputaccepts an optionalPlatformKvStorereference for KV fallback/write-throughcrates/trusted-server-core/src/platform/crates/trusted-server-core/src/integrations/mod.rscrates/trusted-server-core/src/integrations/{aps,adserver_mock,prebid}.rscrates/trusted-server-core/src/publisher.rsRuntimeServices/PlatformKvStorefor consent KV fallback and revocation deletioncrates/trusted-server-core/Cargo.tomlfastlydependency from corecrates/trusted-server-adapter-fastly/src/compat.rscrates/trusted-server-adapter-fastly/src/backend.rscrates/trusted-server-adapter-fastly/src/platform.rsopen_kv_storeforPlatformKvStorecrates/trusted-server-adapter-fastly/src/app.rscrates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-adapter-fastly/Cargo.tomlCloses
Closes #496
Test plan
cargo test -p trusted-server-adapter-fastly edgezero_missing_consent_store_breaks_only_consent_routescargo test -p trusted-server-adapter-fastly configured_missing_consent_store_only_breaks_consent_routescargo test -p trusted-server-core --libcargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspacecd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code; usesexpect("should ...")where neededlogmacros rather thanprintln!