Migrate integration and provider HTTP types (PR 13)#626
Migrate integration and provider HTTP types (PR 13)#626prk-Jr wants to merge 4 commits intofeature/edgezero-pr12-handler-layer-typesfrom
Conversation
prk-Jr
changed the title
prk-Jr
requested review from
ChristianPavilonis and
aram356
and removed request for
aram356
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
PR Review — Migrate integration and provider HTTP types (PR 13)
1 blocking finding, 5 non-blocking suggestions.
Blocking
🔧 Missing Content-Type forwarding for Didomi POST/PUT — see inline comment.
Non-blocking (inline)
- 🤔 Prebid still imports from
fastly::http— last remaininguse fastlyin the integration layer - ♻️ Duplicated body collection helpers (
collect_response_body/collect_body_bytes) - 🌱 Unbounded response body collection (matches pre-migration, but could use a size cap)
- ⛏ Dead
"X-"uppercase branch in custom header copy (http crate lowercases names) - ⛏ Duplicated
backend_name_for_urlhelper across 4 integrations
…/edgezero-pr13-integration-provider-type-migration
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Solid migration of the integration and auction provider layers from Fastly-specific HTTP types to platform-agnostic http crate types. No blocking issues found. CI is passing.
Non-blocking
♻️ refactor
- Duplicated
body_as_readerhelper:proxy.rs:24andpublisher.rs:23define identicalbody_as_readerfunctions. Consider extracting into a shared location (e.g., aninto_reader()method onEdgeBody, or a helper inhttp_util.rs).
📝 note
- PR description file table incomplete:
proxy.rsandpublisher.rshave changes in this diff (thebody_as_readerextraction and test type alias cleanups) but are not listed in the PR body's change table.
📌 out of scope
- Orchestrator tests disabled pending mock
PlatformHttpClient: Several provider integration tests and timeout enforcement paths inorchestrator.rs:751-765are disabled pending a mock forPlatformHttpClient::select(). This is acknowledged in the PR, but the scope of untested paths has grown — a follow-up to add thin mock support forselect()would significantly improve coverage of the deadline enforcement logic.
| /// # Errors | ||
| /// | ||
| /// Returns an error when a streaming body chunk cannot be read. | ||
| pub(crate) async fn collect_body( |
There was a problem hiding this comment.
🤔 thinking — collect_body has no size limit
This reads a streaming body into a Vec<u8> without any size bound. The GTM integration already has its own collect_request_body_bounded for request bodies, but response bodies via collect_body remain unbounded. An upstream returning a very large response would allocate unboundedly.
This pre-dates this PR, but since this PR touches the call sites it's worth a follow-up to add a size cap here too.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
⛏ nitpick — Extra blank line before the closing } on the next line.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
The refactor is clean and the ensure_integration_backend / collect_body helpers extracted in the fix commits are a genuine improvement. The main risks are latent panics and missing POST size bounds that were introduced in the base migration commit. None are new in the fix commits, but this PR is the natural owner of the HTTP-type boundary, so they should land before merge.
Blocking
🔧 wrench
EdgeBody::into_bytes()panics on streaming bodies atauction/endpoints.rs:42— the inbound auction POST body is user-controlled.Body::into_bytes()panics onBody::Stream(_)(unit testinto_bytes_panics_for_streamasserts this). Today Fastly's adapter happens to returnBody::Once, so nothing panics — but that invariant is undocumented and the/auctionhandler is the first publicly-reachable crash if it ever changes. Fix: switch toBody::into_bytes_bounded(max).awaitwith a documented cap (e.g. 256KB).EdgeBody::into_bytes()panics across every migrated provider/proxy — same root cause, see inline comments onprebid.rs:1157,permutive.rs:151,lockr.rs:149,datadome.rs:301,aps.rs:554,adserver_mock.rs:373. Makeparse_responseasync and read bodies withinto_bytes_bounded.- Unbounded inbound POST body forwarding on the datadome / didomi / permutive / lockr proxies — only GTM enforces a cap. Promote the GTM size-bounded reader into
integrations/mod.rsascollect_body_boundedand apply it uniformly. Inline comments ondatadome.rs:367,didomi.rs:245,permutive.rs:211,lockr.rs:203. - Lockr
.expect()on user-controlledOriginheader — reachable worker panic atlockr.rs:266. - GTM stream-read errors mis-classified as
PayloadTooLarge— returns 413 for transport errors atgoogle_tag_manager.rs:313. Add aStreamReadvariant and map to 502.
Non-blocking
🤔 thinking
collect_bodyhas no size cap (integrations/mod.rs:101) — silently drains whole bodies to RAM. Same concern applies to the GTM response-rewrite path atgoogle_tag_manager.rs:486.- Orchestrator
select_result.readyerror path drops provider identity inrun_providers_parallel— whenreadyisErr, the provider identity is lost so noAuctionResponse::error(...)is pushed and the failing backend vanishes silently from the result set. Track a(backend_name, provider_index)pair alongsidefastly_pendingso failures are attributable. - Deadline enforcement relies on every provider honoring
backend_name(effective_timeout)— Phase 1 computesremaining_ms.min(provider.timeout_ms())correctly, but nothing guarantees each provider actually threads that timeout through to the backend config. Not a new regression, but the async migration makes it more load-bearing. Add a unit test that asserts the select loop returns beforedeadline + epsilon.
♻️ refactor
aps.rs/adserver_mock.rsstill buildBackendConfiginline — extend the newensure_integration_backendhelper to cover providers too.
🌱 seedling
- Make
parse_responseasync in the provider trait (auction/provider.rs:48) — needed anyway once theinto_bytes_boundedfix lands; zero cost under#[async_trait(?Send)].
📝 note
- Testlight stale
Content-Lengthtest is narrow (testlight.rs:425) — good unit-level coverage, but the twohandle()rebuild call sites would benefit from an end-to-end assertion.
👍 praise
- Clean helper extraction in
integrations/mod.rs— real dedup across six integrations. - Dropping the dead
|| name_str.starts_with("X-")branch inlockr.rs/permutive.rs—HeaderName::as_str()is always lowercase, so it was unreachable. - Adding
CONTENT_TYPEto didomi's forwarded headers — POST bodies can now be interpreted by upstream.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
|
|
||
| // Parse response | ||
| let body_bytes = response.take_body_bytes(); | ||
| let body_bytes = response.into_body().into_bytes(); |
There was a problem hiding this comment.
🔧 wrench — EdgeBody::into_bytes() panics on Body::Stream(_) (unit test into_bytes_panics_for_stream asserts this). Upstream PBS responses are safe today only because FastlyPlatformHttpClient::fastly_response_to_platform happens to wrap everything in Body::Once. Any future backend or streaming path panics the worker.
Fix: make parse_response async and use into_bytes_bounded(max).await, or have the orchestrator pre-collect the body into Once before invoking parse_response. Apply consistently across all providers (aps, adserver_mock, prebid).
| } | ||
|
|
||
| let sdk_body = permutive_response.take_body_bytes(); | ||
| let sdk_body = permutive_response.into_body().into_bytes(); |
There was a problem hiding this comment.
🔧 wrench — .into_body().into_bytes() panics on streaming bodies. Use the new collect_body helper with a size cap (see the follow-up collect_body cap concern) or Body::into_bytes_bounded(max).await.
| } | ||
|
|
||
| let sdk_body = lockr_response.take_body_bytes(); | ||
| let sdk_body = lockr_response.into_body().into_bytes(); |
There was a problem hiding this comment.
🔧 wrench — .into_body().into_bytes() panics on streaming bodies. Route through a bounded helper and propagate overflow as a proper error instead of panicking.
| .headers() | ||
| .get(header::ACCESS_CONTROL_ALLOW_ORIGIN) | ||
| .cloned(); | ||
| let body = backend_resp.response.into_body().into_bytes(); |
There was a problem hiding this comment.
🔧 wrench — .into_body().into_bytes() panics on streaming bodies. Use Body::into_bytes_bounded(max).await or the shared collect_body_bounded helper.
|
|
||
| // Parse response body | ||
| let body_bytes = response.take_body_bytes(); | ||
| let body_bytes = response.into_body().into_bytes(); |
There was a problem hiding this comment.
🔧 wrench — .into_body().into_bytes() panics on streaming bodies. Make parse_response async and read with into_bytes_bounded.
| Ok(body_bytes) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🤔 thinking — collect_body silently drains any sized body to RAM. The current caller (testlight) reads whole upstream RTB responses; the same concern applies to GTM's response rewrite path. Once you add collect_body_bounded for the POST-forwarding fix, force every caller to pass a max — or at minimum document the invariant loudly here.
| response: fastly::Response, | ||
| response: PlatformResponse, | ||
| response_time_ms: u64, | ||
| ) -> Result<AuctionResponse, Report<TrustedServerError>>; |
There was a problem hiding this comment.
🌱 seedling — parse_response is sync today. Once you land the into_bytes_bounded fix, it'll need to become async anyway. The trait is #[async_trait(?Send)] so the ergonomic cost is zero; worth doing in the same PR so the async boundary matches the new HTTP type boundary.
| @@ -517,29 +525,33 @@ impl AuctionProvider for ApsAuctionProvider { | |||
| ), | |||
| })?; | |||
There was a problem hiding this comment.
♻️ refactor — aps.rs and adserver_mock.rs still construct BackendConfig::from_url_with_first_byte_timeout(...) inline. The new ensure_integration_backend helper in integrations/mod.rs is focused on proxy integrations but the providers do the same thing and would benefit from the same dedup.
| Some("application/json"), | ||
| "should normalize JSON responses to application/json" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📝 note — The stale Content-Length regression test covers rebuild_response in isolation — good, but handle() has two rebuild_response call sites (JSON success and non-JSON fallback). Consider extending handle_uses_platform_http_client_with_http_request to assert end-to-end that a stale upstream Content-Length is dropped by injecting one on the stubbed upstream response.
| integration: integration.to_string(), | ||
| message: "Failed to register backend".to_string(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
👍 praise — Clean extraction of ensure_integration_backend + collect_body. The generic TrustedServerError::Integration { integration, message } plumbing is a real improvement over the per-integration Self::error() wrappers it replaces, and the reuse across datadome/lockr/permutive/testlight/didomi/GTM meaningfully cuts the migration's footprint.
3 participants
Summary
RuntimeServicesHTTP client primitives with asyncrequest_bids,PlatformPendingRequest, andPlatformResponsewhile preserving orchestrator deadline handling.Changes
crates/trusted-server-adapter-fastly/src/main.rsRuntimeServicesinto the PR13 entrypoint.crates/trusted-server-core/src/auction/README.mdPlatformPendingRequest,PlatformResponse, and the platform HTTP client flow.crates/trusted-server-core/src/auction/endpoints.rsRuntimeServicesinto the auction entrypoint context.crates/trusted-server-core/src/auction/orchestrator.rsPlatformPendingRequest/select, and update the remaining PR13 migration comments.crates/trusted-server-core/src/auction/provider.rsAuctionProviderto asyncrequest_bidswithPlatformPendingRequest/PlatformResponseand refresh the trait docs.crates/trusted-server-core/src/auction/types.rsRuntimeServicestoAuctionContext.crates/trusted-server-core/src/integrations/adserver_mock.rscrates/trusted-server-core/src/integrations/aps.rscrates/trusted-server-core/src/integrations/datadome.rscrates/trusted-server-core/src/integrations/didomi.rscrates/trusted-server-core/src/integrations/google_tag_manager.rscrates/trusted-server-core/src/integrations/gpt.rscrates/trusted-server-core/src/integrations/lockr.rscrates/trusted-server-core/src/integrations/permutive.rscrates/trusted-server-core/src/integrations/prebid.rsPlatformResponseresults.crates/trusted-server-core/src/integrations/registry.rsIntegrationProxy/routing types tohttptypes and threadRuntimeServicesthrough proxy dispatch.crates/trusted-server-core/src/integrations/testlight.rsContent-Lengthregression test, and drop stale length headers when rebuilding JSON responses.Closes
Closes #494
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 servecargo doc --no-deps --all-features(passes with pre-existing unrelated rustdoc warnings outside this PR's scope)Checklist
unwrap()in production code — useexpect("should ...")println!