Migrate integration and provider HTTP types (PR 13)#626
Migrate integration and provider HTTP types (PR 13)#626prk-Jr wants to merge 8 commits intofeature/edgezero-pr12-handler-layer-typesfrom
Conversation
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
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.
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
Conflicts resolved by keeping PR13's http-native integration layer throughout — all compat shim insertions from PR12 are superseded by this branch's completed integration type migration.
- Add collect_body_bounded helper with INTEGRATION_MAX_BODY_BYTES (256 KiB) cap to prevent unbounded memory use on streaming bodies - Replace all into_bytes() calls (panic on Body::Stream) with collect_body or collect_body_bounded across integrations and auction endpoint - Make AuctionProvider::parse_response async so implementations can safely drain response bodies without panicking on the Stream variant - Add .await to both parse_response call sites in the orchestrator - Cap inbound request bodies in lockr, permutive, datadome, and didomi proxy handlers using collect_body_bounded before forwarding upstream - Fix lockr Origin header: replace expect() with a warn-and-skip fallback so an invalid user-supplied origin cannot crash the edge worker - Add PayloadSizeError::StreamRead variant to google_tag_manager and return 502 (not 413) when a stream transport error occurs while reading the body - Remove extra blank line before closing brace in google_tag_manager impl block
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of the EdgeZero PR13 integration/provider HTTP type migration after the latest "Address review findings" commit. The async parse_response migration and the new collect_body / collect_body_bounded helpers landed cleanly and resolved the prior into_bytes()-on-Stream panic class across all providers. Two blockers remain plus one CI/process question that affects whether the existing test plan can be trusted.
Blocking
🔧 wrench
-
cargo fmt --all -- --checkfails locally in 7+ files. The PR description's[x] cargo fmtcheckbox is incorrect — see the CI question below for why this wasn't caught. Affected files:crates/trusted-server-core/src/auction/endpoints.rs:39-47crates/trusted-server-core/src/auction/orchestrator.rs:12-14, 402-405crates/trusted-server-core/src/integrations/google_tag_manager.rs:16-25, 39-45crates/trusted-server-core/src/integrations/lockr.rs:282-285crates/trusted-server-core/src/integrations/permutive.rs:208-219, 278-281crates/trusted-server-core/src/integrations/prebid.rs:5-13, 1154-1162crates/trusted-server-core/src/integrations/testlight.rs:11-16
Fix: run
cargo fmt --all. -
Testlight POST body is unbounded (inline at
integrations/testlight.rs:181).
❓ question
-
CI gates aren't enforced for this PR.
.github/workflows/format.ymland.github/workflows/test.ymlonly fire onpull_request: branches: [main]. PR 626's base isfeature/edgezero-pr12-handler-layer-types, so on commit94ec9477only "Integration Tests" ran — fmt, clippy, andcargo test --workspacedid not. Combined with the fmt failure above, every stacked EdgeZero PR ships with broken formatting and an unverified test plan. Was this intentional, or should the workflow triggers be widened (or the gates duplicated under the integration-tests umbrella)? Until that's resolved, the PR description's CI checkboxes can't be relied on as evidence.Verified locally on this commit:
cargo test --workspacepasses (821 tests),cargo clippy --workspace --all-targets --all-features -- -D warningspasses,cargo build --release --target wasm32-wasip1passes,cargo fmt --all -- --checkfails.
Non-blocking
🤔 thinking
-
Unbounded response-body reads via
collect_body(inline atintegrations/mod.rs:93for stream-read error fidelity; cross-cutting concern listed here). The bounded variant exists for inbound bodies, but every upstream response site still drains an unbounded body to aVec<u8>:integrations/prebid.rs:1157— Prebid Server response (highest risk: third-party hop, large payloads)integrations/aps.rs:555— APS responseintegrations/adserver_mock.rs:375— Mediator responseintegrations/lockr.rs:150— Lockr SDK fetchintegrations/permutive.rs:152— Permutive SDK fetchintegrations/datadome.rs:302— DataDome SDK fetchintegrations/google_tag_manager.rs:491— GTM script
Provider responses are the highest risk — a misbehaving Prebid Server / mediator can ship a multi-GB body and OOM the worker. Recommend routing all of these through
collect_body_boundedwith a per-callsite cap, or removing the unbounded variant in favor of a single bounded helper.
♻️ refactor
-
Auction providers still construct
BackendConfiginline while integration proxies route throughensure_integration_backend. Reraising the prior-review concern with current locations:integrations/aps.rs:517-528integrations/adserver_mock.rs:338-348integrations/prebid.rs:1131-1135
The two paths differ on timeout (providers need a configurable per-request timeout; the proxy helper hardcodes 15s). Consider widening
ensure_integration_backendto accept a timeout and reusing it from the providers, or at least documenting the divergence.
CI Status
- fmt: FAIL locally (CI did not run for this commit — see ❓ question above)
- clippy: PASS locally (CI did not run)
- rust tests: PASS locally — 821 tests (CI did not run)
- wasm32-wasip1 build: PASS locally
- Integration Tests workflow: PASS (only workflow that ran on
94ec9477)
… compat conflicts
…EADME - Run cargo fmt --all (7+ files had formatting failures) - Cap testlight POST body with collect_body_bounded + INTEGRATION_MAX_BODY_BYTES - Use dedicated AUCTION_MAX_BODY_BYTES (256 KiB) for /auction instead of INTEGRATION_MAX_BODY_BYTES - Update auction/README.md: parse_response is now async, parallel execution via select() is live
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!