Add PlatformHttpClient and PlatformBackend traits (EdgeZero PR6)#581
Merged
Add PlatformHttpClient and PlatformBackend traits (EdgeZero PR6)#581
Conversation
Rename crates/common → crates/trusted-server-core and crates/fastly → crates/trusted-server-adapter-fastly following the EdgeZero naming convention. Add EdgeZero workspace dependencies pinned to rev 170b74b. Update all references across docs, CI workflows, scripts, agent files, and configuration.
Introduces trusted-server-core::platform with PlatformConfigStore, PlatformSecretStore, PlatformKvStore, PlatformBackend, PlatformHttpClient, and PlatformGeo traits alongside ClientInfo, PlatformError, and RuntimeServices. Wires the Fastly adapter implementations and threads RuntimeServices into route_request. Moves GeoInfo to platform/types as platform-neutral data and adds geo_from_fastly for field mapping.
…o-pr2-platform-traits
- Defer KV store opening: replace early error return with a local UnavailableKvStore fallback so routes that do not need synthetic ID access succeed when the KV store is missing or temporarily unavailable - Use ConfigStore::try_open + try_get and SecretStore::try_get throughout FastlyPlatformConfigStore and FastlyPlatformSecretStore to honour the Result contract instead of panicking on open/lookup failure - Encapsulate RuntimeServices service fields as pub(crate) with public getter methods (config_store, secret_store, backend, http_client, geo) and a pub new() constructor; adapter updated to use new() - Reference #487 in FastlyPlatformHttpClient stub (PR 6 implements it) - Remove unused KvPage re-export from platform/mod.rs - Use super::KvHandle shorthand in RuntimeServices::kv_handle()
- Split fastly_storage.rs into storage/{config_store,secret_store,api_client,mod}.rs
- Add PlatformConfigStore read path via FastlyPlatformConfigStore::get using ConfigStore::try_open/try_get
- Add PlatformError::NotImplemented variant; stub write methods on FastlyPlatformConfigStore and FastlyPlatformSecretStore
- Add StoreName/StoreId newtypes with From<String>, From<&str>, AsRef<str>
- Add UnavailableKvStore to core platform module
- Add RuntimeServicesBuilder replacing 7-arg constructor
- Migrate get_active_jwks and handle_trusted_server_discovery to use &RuntimeServices
- Update call sites in signing.rs, rotation.rs, main.rs
- Add success-path test for handle_trusted_server_discovery using StubJwksConfigStore
- Fix test_parse_cookies_to_jar_empty typo (was emtpy)
- Make StoreName and StoreId inner fields private; From/AsRef provide all needed construction and access - Add #[deprecated] to GeoInfo::from_request with #[allow(deprecated)] at the three legacy call sites to track migration progress - Enumerate the six platform traits in the platform module doc comment - Extract backend_config_from_spec helper to remove duplicate BackendConfig construction in predict_name and ensure - Replace .into_iter().collect() with .to_vec() on secret plaintext bytes - Remove unused bytes dependency from trusted-server-adapter-fastly - Add comment on SecretStore::open clarifying it already returns Result (unlike ConfigStore::open which panics)
…zero-pr3-config-store
…o-pr4-secret-store
…o-pr6-backend-http-client
Collaborator
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
PR #581 Review: Add PlatformHttpClient and PlatformBackend traits
Overall: Well-structured PR with strong abstractions and thorough test coverage. The previous review round's findings were all addressed. Two new findings warrant attention before merge (header handling and the select() ordering assumption); the rest are improvements for follow-ups.
Praise
PlatformPendingRequestdowncast API returningErr(self)to preserve backend metadata is a thoughtful design!Sendrationale documentation onPlatformPendingRequestclearly explains the wasm32 constraintStubHttpClientfan-out test infrastructure withStubPendingResponseis clean and comprehensiveRuntimeServicesBuilderwithexpect("should ...")messages follows project conventions perfectly
Collaborator
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Good PR — the platform abstraction layer is well-designed with thoughtful type erasure, thorough test coverage, and clean RuntimeServices threading. A few items to address before merge:
P0 — Stale doc comment: orchestrator.rs run_providers_parallel doc (line 244) still references fastly::http::request::select() but the implementation now uses services.http_client().select(). (This line is not in the diff, so noting here instead of inline.)
P1 — Silent body truncation in release builds: The debug_assert! + log::warn! pattern for Body::Stream in edge_request_to_fastly, platform_response_to_fastly, and fastly_response_to_platform is a no-op in release. If a streaming body reaches these paths, requests/responses are sent with empty bodies. Recommend upgrading to log::error! or returning Result.
P1 — Downcast failure drops all pending requests: In FastlyPlatformHttpClient::select(), if any single PlatformPendingRequest fails downcast, the entire function errors and all remaining in-flight requests are lost. Consider adding the backend name to the error message for diagnostics.
P2 — Pre-existing: rebuild_response_with_body still uses set_header (drops multi-value headers like Set-Cookie), while the new conversion functions correctly use append_header.
ChristianPavilonis
approved these changes
Apr 8, 2026
aram356
requested changes
Apr 10, 2026
Collaborator
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Implements PlatformHttpClient with send/send_async/select and threads RuntimeServices through all proxy-layer handlers. Good test coverage for the new platform abstractions, but the allowed_domains removal from ProxyRequestConfig introduces a correctness regression for integration proxies.
Blocking
🔧 wrench
- Integration proxies blocked by first-party allowed_domains: Removing
allowed_domainsfromProxyRequestConfigmeans GTM, testlight, and other integration proxies now havesettings.proxy.allowed_domainsenforced on their upstream requests. Previously they operated in open mode (&[]). In production this will block integrations from contacting their upstream domains (crates/trusted-server-core/src/proxy.rs:582,:702)
Non-blocking
♻️ refactor
- Header forwarding test removed without replacement: The
header_copy_copies_curated_settest was deleted butproxy_request_calls_platform_http_client_sendusescopy_request_headers: false, leaving the header forwarding path untested (crates/trusted-server-core/src/proxy.rs:1977)
🤔 thinking
select()used for single-request mediator wait: Wrapping a single pending request inselect(vec![...])adds unnecessary complexity vs a dedicatedwait()method (crates/trusted-server-core/src/auction/orchestrator.rs:161)
⛏ nitpick
PROXY_FORWARD_HEADERSconst vs inlineAccept-Encoding: The 5 forwarded headers are in a const array but Accept-Encoding override is added separately inline — minor inconsistency vs the old unifiedcopy_proxy_forward_headersfunction
CI Status
- fmt: PASS
- clippy: PASS
- cargo test: PASS
- vitest: PASS
- integration tests: PASS
- CodeQL: PASS
aram356
reviewed
Apr 14, 2026
Collaborator
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
All previously-blocking findings from the prior review rounds are resolved and covered by new tests. The trait design (async_trait(?Send) + Box<dyn Any> type-erased pending requests) is sound for WASM. No correctness blockers remain — only follow-up quality items.
Non-blocking
🤔 thinking
- Orchestrator
select()fan-out has no end-to-end stub-driven test.AuctionOrchestrator::run_providers_parallelis only covered bytest_no_providers_configured. Root cause:AuctionProvider::request_bidsstill hands back a concretefastly::http::request::PendingRequest, soStubHttpClientcan't be injected without a provider-level abstraction. Ships the deadline branch, backend-name correlation, unknown-backend warning, and remaining-drop logic without direct coverage. (crates/trusted-server-core/src/auction/orchestrator.rs:379-460) PlatformPendingRequestmetadata lost onselect()rewrap. Remaining requests are rebuilt withPlatformPendingRequest::new(pending)— no.with_backend_name()(crates/trusted-server-adapter-fastly/src/platform.rs:308-311). Any future caller inspectingpending.backend_name()on a remaining request (e.g. the "dropping N remaining" log atorchestrator.rs:454) will seeNone. Consider re-deriving viapending.get_backend_name().map(str::to_string)on the rewrap.StubHttpClient::selectis strictly FIFO. Always pops index 0, which is a less faithful double than Fastly's unorderedselect(). Tests built on it can accidentally depend on "first pushed = first ready," masking reordering bugs. Either document explicitly on the stub or randomize which item is marked ready. (crates/trusted-server-core/src/platform/test_support.rs:250-278)IntegrationProxy: Send + Syncwith?Sendfutures. Consistent for single-threaded WASM, but a one-line rationale mirroring the excellent!Senddoc block onPlatformPendingRequestwould save future readers time. (crates/trusted-server-core/src/integrations/registry.rs:247-248)
♻️ refactor
servicesparameter proliferation. Four proxy entry points now take(settings, req, config, services). As more platform services thread through in PR7+, consider absorbingservicesintoProxyRequestConfigor a lightweightProxyContext<'a>. Next PR, not this one. (crates/trusted-server-core/src/proxy.rs:447-451)PROXY_FORWARD_HEADERSconst vs. inlineAccept-Encodingoverride. The 5 forwarded headers live in a const array butAccept-Encodingis appended separately atproxy.rs:634-637, splitting header curation across two sites. Pulling the override into the same helper would keep curation in one place.
⛏ nitpick
unwrap_or("")fallback inselect()backend-name is now logged (good), but the subsequentbackend_to_provider.remove("")in the orchestrator will still silently drop the response. Preferable: returnPlatformResponse { backend_name: None, .. }and let the orchestrator's existing unknown-backend branch handle it with its more specific log message. (crates/trusted-server-adapter-fastly/src/platform.rs:315-321)- Brief doc comment on the
StreamingResponseHttpClienthelper intest_support.rsclarifying what it models, since it's the only!Send-body test surface.
👍 praise
PlatformPendingRequest::downcastreturningErr(self)— preserves backend metadata on downcast failure. Thoughtful API.- Exemplary
!Sendrationale doc block onPlatformPendingRequest— please keep that pattern on future platform traits. - Default
wait()method on the trait cleanly eliminates the "single-item select" awkwardness flagged in the previous round. StubHttpClientnow supportssend_async/selectwith per-call header capture — a real fan-out test surface, not just a stub-that-errors.proxy_request_returns_error_for_streaming_platform_response_bodyproving the release-mode behavior of the new error path is exactly the right response to the previousdebug_assert!concern.- Zero new Cargo dependencies — the entire abstraction lands with no supply-chain cost.
Resolved from prior rounds
All P0/P1 items from the prior aram356 and ChristianPavilonis rounds are verified resolved on bbef5223:
backend_namecorrelation now re-derived fromfastly::Response::get_backend_name(), positional zip removed.- Duplicated
platform_response_to_fastlyconsolidated aspub(crate)inproxy.rsand reused fromorchestrator.rs. Body::Streamtruncation viadebug_assert!now returnsPlatformError::HttpClient/TrustedServerError::Proxyin release, covered by new tests on both sides of the boundary.set_headermulti-value drop replaced withappend_headereverywhere; regression testrebuild_response_with_body_preserves_multiple_set_cookie_headersadded.allowed_domainsregression fixed; field restored and threaded throughhandle_first_party_proxy.- Curated-header forwarding test re-added as
proxy_request_forwards_curated_headers_when_copy_request_headers_is_true.
CI Status
- cargo fmt: PASS
- cargo clippy / Analyze (rust / wasm32-wasip1): PASS
- cargo test: PASS
- vitest: PASS
- integration + browser integration: PASS
- CodeQL: PASS
- format-docs / format-typescript: PASS
aram356
approved these changes
Apr 14, 2026
* Add PR7 design spec for geo lookup + client info extract-once
Documents the call site migration plan: five Fastly SDK extraction
points in trusted-server-core replaced by RuntimeServices::client_info
reads, following Phase 1 injection pattern from the EdgeZero migration design.
* Fix spec review issues in PR7 design doc
- Correct erroneous claim about generate_synthetic_id being called twice
via DeviceInfo; it is called once (line 91 for fresh_id), DeviceInfo.ip
is a separate req.get_client_ip_addr() call fixed independently
- Add before/after snippet for handle_publisher_request call site in main.rs
- Add noop_services import instruction for http_util.rs test module
- Clarify _services rename (drop underscore, not add new param) in didomi.rs
- Clarify nextjs #[allow(deprecated)] annotations are out of scope (different function)
* Update PR7 spec to address all five agent review findings
- Change RequestInfo::from_request signature to &ClientInfo (not
&RuntimeServices) so prebid can call it with context.client_info
- Scope SDK-call acceptance criteria to active non-deprecated code only
- List all six AuctionContext construction sites including two production
sites in orchestrator.rs and three test helpers in orchestrator/prebid
- Add explicit warn-and-continue pattern for publisher.rs geo lookup
- Correct testing table: formats.rs and endpoints.rs have no test modules;
add orchestrator.rs and prebid.rs test helper update rows
* Add PR7 implementation plan and address plan review findings
Plan covers 6 tasks in compilation-safe order: AuctionContext struct change
first, then from_request signature, then synthetic.rs cascade, then publisher
geo, then didomi. Includes two new copy_headers unit tests (Some/None).
Spec fixes: clarify injection pattern exceptions for &ClientInfo and
Option<IpAddr>; reword acceptance criterion to reflect that provider-layer
reads flow through AuctionContext.client_info.
* Fix three plan review findings and two open questions
- Finding 1 (High): Add missing publisher.rs test call site at line ~695
for get_or_generate_synthetic_id — was omitted from Task 3 Step 6
- Finding 2 (Medium): Remove crate::geo::GeoInfo import from endpoints.rs
rather than replacing it — type is not used by name after the change,
keeping any import fails clippy -D warnings
- Finding 3 (Low): Replace interactive git add -p in Task 6 with explicit
file staging instruction
- Open Q1: Add Task 2 step to update stale handle_publisher_request
signature in auction/README.md
- Open Q2: Add Task 2 step to update from_request doc comment to reflect
ClientInfo-based TLS detection instead of Fastly SDK calls
* Broaden two low-severity doc cleanup steps in PR7 plan
- Step 7: cover all four stale Fastly-SDK-specific locations in
http_util.rs (SPOOFABLE_FORWARDED_HEADERS doc, RequestInfo struct doc,
from_request doc, detect_request_scheme doc)
- Step 8: replace the whole routing snippet in auction/README.md, not
just the one handle_publisher_request line — handle_auction and
integration_registry.handle_proxy are also stale in that snippet
* Fix two remaining low findings in PR7 plan
- Add missing Location 2 (RequestInfo.scheme field doc, line ~67) to
Step 7; renumber subsequent locations 3-5
- Replace &runtime_services with runtime_services in Step 5 and README
snippet — runtime_services is already &RuntimeServices in route_request
* Fix count drift in Step 7: four → five locations
* Add client_info field to AuctionContext and fix all construction sites
* Change RequestInfo::from_request to take &ClientInfo, thread services into handle_publisher_request
* Add Task 2 follow-up coverage and README route fixes
* Add services param to generate_synthetic_id, remove Fastly IP/geo calls in formats and endpoints
* Revert premature publisher geo change from Task 3
* Replace deprecated GeoInfo::from_request in publisher.rs with services.geo().lookup()
* Remove Fastly IP extraction from Didomi copy_headers, use ClientInfo instead
* Move IpAddr import to test module level in didomi.rs
* Apply rustfmt formatting to didomi.rs, publisher.rs, and synthetic.rs
Fix multi-line function call style in didomi.rs, line-break wrapping in
publisher.rs test, and import ordering in synthetic.rs test module.
* Add test coverage for generate_synthetic_id with concrete client IP
Adds noop_services_with_client_ip helper to test_support and a new
test that verifies the client_ip path through generate_synthetic_id
by asserting the HMAC differs when the IP changes.
* Align geo lookup warn log format with codebase convention ({e} not {e:?})
* Apply Prettier formatting to PR7 plan and spec docs
* Verify content rewriting pipeline is platform-agnostic (PR 8) (#600)
* Document content rewriting as platform-agnostic in platform module
* Document html_processor as platform-agnostic
* Document streaming_processor as platform-agnostic
* Fix unresolved doc link: replace EdgeRequest with edgezero_core::http::Request
- Fix intra-doc link syntax and restore missing blank line in `html_processor`
- Replace opaque PR number references with descriptive context labels
- Move HTTP-type coupling caveat from `platform` module down to `publisher.rs`
- Convert `StreamingPipeline::process` plain-text generics to an intra-doc link
… in proxy requests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PlatformHttpClienttrait withsend(),send_async(), andselect()for auction fan-out — a superset of EdgeZero'sProxyClientthat covers the async/select paths not yet in EdgeZero upstreamPlatformBackendtrait withpredict_name()andensure()to decouple backend registration from Fastly-specific APIsRuntimeServicesthrough all proxy-layer handlers (IntegrationProxy::handle, endpoint handlers,proxy_request) so the HTTP client and backend are reachable without global stateChanges
platform/http.rs(new)PlatformHttpClienttrait +PlatformHttpRequest,PlatformResponse,PlatformPendingRequest,PlatformSelectResulttypesplatform/backend.rs(new)PlatformBackendtrait +PlatformBackendSpecplatform/types.rsRuntimeServicesextended withhttp_clientandbackendfieldsplatform/test_support.rsStubHttpClient,StubBackend,build_services_with_http_clienttest helpersintegrations/registry.rsIntegrationProxy::handle+IntegrationRegistry::handle_proxyaccept&RuntimeServices_servicesthroughIntegrationProxy::handleproxy.rsservices.http_client().send(); addProxyRequestHeadersstruct to stay under 7-arg limit; addproxy_request_calls_platform_http_client_sendtestauction/orchestrator.rsservicesthrough auction handlerplatform.rs(Fastly adapter)FastlyPlatformHttpClientimpl; documentBody::Streamlimitation with warning logmain.rs(Fastly adapter)runtime_servicesto all route handlersCloses
Closes #487
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd 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 — useexpect("should ...")logmacros (notprintln!)Notes for reviewer
This branch carries PRs 2–6 cumulatively (crate rename, platform traits, config store, secret store, HTTP client). The focused diff for this PR is the latest commit (
571656c) plus the PR6-specific commits. ThePlatformHttpClient::send_asyncandselectmethods intentionally returnErr(Unsupported)in the Fastly adapter for now — the fan-out path inorchestrator.rsstill usesfastly::http::request::selectdirectly and will be migrated in a follow-up once EdgeZero adds upstream fan-out support (issues #147–148).