Migrate integration and provider HTTP types (PR 13)#626
Migrate integration and provider HTTP types (PR 13)#626prk-Jr wants to merge 11 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.
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
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
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
…/edgezero-pr13-integration-provider-type-migration
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of PR13 after the latest "Address PR review" commit (4b27087e) and the PR12 merge (a2036eb5). The async parse_response migration, the dedicated AUCTION_MAX_BODY_BYTES, the README updates, and the testlight body cap all landed cleanly. One blocker remains plus several non-blocking re-raises from prior reviews that haven't been addressed.
Local verification on this commit:
cargo test --workspace— PASS (952 tests, 0 failures)cargo clippy --workspace --all-targets --all-features -- -D warnings— PASScargo build --release --target wasm32-wasip1 -p trusted-server-adapter-fastly— PASScargo fmt --all -- --check— FAIL (4 PR-introduced sites + 11 inherited from PR12 base)- GitHub CI (integration tests workflow) — PASS
GitHub's format.yml / test.yml still only fire on PRs targeting main, so the fmt/clippy/test gates don't run for this PR's stack. The PR description's [x] cargo fmt / [x] cargo clippy / [x] cargo test checkboxes therefore can't be relied on as evidence — see the fmt blocker below.
Blocking
🔧 wrench
-
fmt failures still present in PR-modified files.
cargo fmt --all -- --checkreports diffs at:crates/trusted-server-adapter-fastly/src/main.rs:209and:236crates/trusted-server-core/src/integrations/prebid.rs:1057(the line at ~1066 is too long inside theif letblock)crates/trusted-server-core/src/integrations/registry.rs:673
These four sites did not exist on the PR12 base — PR13 introduced them. (11 additional fmt diffs are inherited from the PR12 base and are not this PR's responsibility, but the project-wide
cargo fmt --all -- --checkdoes fail.) Fix:cargo fmt --all.
Non-blocking
🤔 thinking
- Provider/integration response bodies still unbounded (re-raise; inline at
integrations/mod.rs:84). 7 callsites still drain upstream responses withcollect_bodyinstead of a bounded helper. Provider responses are highest risk (prebid.rs:1160,aps.rs:555,adserver_mock.rs:375). - Stream-read errors classified as
Integration(re-raise; inline atintegrations/mod.rs:93and the bounded variant at:137). Conflates transport failure with integration semantic error; both surface as 5xx-server with the same shape. GTM has a bespoke 502 path that the shared helper doesn't. - Orchestrator backend-name fallback can silently lose bids (inline at
auction/orchestrator.rs:337). Whenpending.backend_name()isNoneand the platform sets a differentPlatformResponse::backend_name, the lookup at line 392 fails and the bid is dropped with a"Received response from unknown backend"warning. Consider alog::warn!when the fallback fires. collect_body_boundedchecks size after reading the chunk (inline atintegrations/mod.rs:143). The effective bound is "≤ max + one_chunk", not strict ≤ max. Acceptable in practice (chunk sizes are small) but worth a comment on the helper.
♻️ refactor
- Auction providers still construct
BackendConfiginline (re-raise; inline atprebid.rs:1134, alsoaps.rs:517-528,adserver_mock.rs:338-348). Diverges fromensure_integration_backendbecause providers need a per-request timeout and the helper hardcodes 15s. AuctionContext.client_infois redundant withservices.client_info()(re-raise; inline atauction/types.rs:107).run_auctiontakesservicesas a separate arg even thoughcontext.servicesexists (inline atauction/orchestrator.rs:64). They must stay in sync; an inconsistent caller breaks the auction silently.- Inconsistent
services.client_infoaccess (inline atdidomi.rs:259). Two sites use thepub(crate)field (didomi.rs:259,auction/formats.rs:145); the rest use the methodservices.client_info(). Standardize on the method.
📝 note
- Lockr/Permutive/Didomi handlers contain unreachable PUT/PATCH branches (inline at
lockr.rs:202). Routes register only GET/POST. StubHttpClient::send_asyncdoes not capture request headers (inline atplatform/test_support.rs:297). Limits header-shape assertions for prebid/aps/adserver_mock tests.
🏕 camp site
testlight::Defaultimpls can be derived (inline attestlight.rs:271).
👍 praise
compat::shim eliminated from integration dispatch —IntegrationProxy::handleis nowhttp::Request<EdgeBody>end-to-end (main.rs:195,registry.rs:266). Removes a non-trivial conversion overhead per request and a class of header-mapping bugs.- Async
parse_responsemigration withPlatformResponseresolves the priorinto_bytes()-on-Body::Streampanic class for every provider (auction/provider.rs:38). The trait docs explicitly note why it's now async — exactly the right context for future implementors. - GTM 413 vs 502 distinction (
google_tag_manager.rs:511-535). Clean separation between client-error and transport-error; this is the pattern the sharedcollect_body*helpers should follow. - EC-ID header-injection guard (
registry.rs:673) plus regression test atregistry.rs:1406. DefensiveHeaderValue::from_strblocks\r/\n/\0injection on a user-influenced ID before it lands in either the request or the response.
CI Status
- fmt: FAIL locally (CI did not run for this commit because workflow triggers don't match the PR's base branch — see header above)
- clippy: PASS locally (CI did not run)
- rust tests: PASS locally (CI did not run; 952 tests)
- wasm32-wasip1 build: PASS locally
- Integration Tests workflow: PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Inline annotations for the prior REQUEST_CHANGES review
Adding the per-line comments now that I've filtered them against the PR diff (the previous submission was body-only because GitHub rejected the batch when 3 of 17 comment anchors fell outside the changed hunks). These reference the same findings as #626 (review) — no new findings, just inline anchoring.
The three findings that couldn't be anchored to a diff line are tagged below for reference:
- ♻️ refactor —
run_auctiontakesservicesseparately even thoughcontext.servicesexists. Anchored as a top-level note rather than inline becauseauction/orchestrator.rs:64(therun_auctionsignature) is not in the diff, but the dual-pass pattern is visible at the response-handling sites (e.g.orchestrator.rs:337). - 📝 note —
StubHttpClient::send_asyncdoes not capture request headers.crates/trusted-server-core/src/platform/test_support.rsis not in this PR's diff scope at all; the asymmetry betweensend(lines 266–280) andsend_async(lines 297–320) limits header-shape assertions for prebid/aps/adserver_mock tests. Worth a follow-up issue. - 🏕 camp site —
testlight::Defaultimpls can be derived.testlight.rs:271-285is outside the changed hunks; deriving#[derive(Default)]would replace both manual impls.
| if request_signing_config.enabled { | ||
| let signer = RequestSigner::from_services(context.services)?; | ||
| let params = | ||
| SigningParams::new(request.id.clone(), request_info.host.clone(), request_info.scheme.clone()); |
There was a problem hiding this comment.
🔧 wrench — cargo fmt --all -- --check fails here (line is too long inside the if let Some(request_signing_config) = ... block). The PR description marks [x] cargo fmt, but a fresh local run on this commit reports diffs at:
crates/trusted-server-adapter-fastly/src/main.rs:209crates/trusted-server-adapter-fastly/src/main.rs:236crates/trusted-server-core/src/integrations/prebid.rs:1057(this site)crates/trusted-server-core/src/integrations/registry.rs:673
(11 additional fmt diffs are inherited from the PR12 base — not this PR's responsibility — but PR13 introduced the 4 above.)
Fix: cargo fmt --all.
| req.set_header(HEADER_X_TS_EC, ec_id.as_str()); | ||
| match HeaderValue::from_str(ec_id) { | ||
| Ok(header_value) => { | ||
| req.headers_mut().insert(HEADER_X_TS_EC.clone(), header_value); |
There was a problem hiding this comment.
🔧 wrench — cargo fmt --all -- --check fails here too (req.headers_mut().insert(HEADER_X_TS_EC.clone(), header_value); needs to wrap). Same root cause as the prebid.rs:1057 site. cargo fmt --all will fix it.
| /// # 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 is still unbounded for upstream responses (re-raise from prior review). Inbound bodies are now correctly capped via collect_body_bounded, but every provider/integration response read still drains an unbounded body to Vec<u8>:
integrations/prebid.rs:1160(Prebid Server response — highest risk: third-party hop)integrations/aps.rs:555(APS response)integrations/adserver_mock.rs:375(Mediator response)integrations/lockr.rs:150(Lockr SDK fetch)integrations/permutive.rs:152(Permutive SDK fetch)integrations/datadome.rs:302(DataDome SDK fetch)integrations/google_tag_manager.rs:549(GTM script)
A misbehaving Prebid Server / mediator can ship a multi-GB body and OOM the worker. Recommend routing all of these through collect_body_bounded with a per-callsite cap (e.g., 1–2 MiB for RTB responses, 16 MiB for SDK fetches), or removing the unbounded variant in favor of a single bounded helper.
| EdgeBody::Stream(mut stream) => { | ||
| let mut body_bytes = Vec::new(); | ||
| while let Some(chunk_result) = stream.next().await { | ||
| let chunk = chunk_result.map_err(|error| { |
There was a problem hiding this comment.
🤔 thinking — Stream-read errors are still wrapped as TrustedServerError::Integration for both collect_body and collect_body_bounded (this site at lines 93–98 and the bounded variant at 137–142). This conflates "transport failed mid-body" with "integration semantic error" — both surface as 5xx-server-error with the same shape.
GTM has a bespoke PayloadSizeError::StreamRead path that returns 502 explicitly (google_tag_manager.rs:528). The shared helper doesn't get that distinction.
Fix: introduce a dedicated variant (e.g. TrustedServerError::UpstreamBodyRead) that maps to 502, or document the 5xx mapping explicitly so callers know what they're returning to the client. Re-raise from prior review.
| message: format!("Failed to read request body: {error}"), | ||
| }) | ||
| })?; | ||
| if body_bytes.len() + chunk.len() > max_bytes { |
There was a problem hiding this comment.
🤔 thinking — The size check happens after chunk is materialized in memory, not before. If a single chunk's len() is greater than max_bytes, that chunk has already been allocated and read before being rejected.
In practice Fastly H2/H3 framing chunks are small (~16 KiB), and the EdgeBody::Once path at line 124 already short-circuits on a single-shot body. So this is a small concern, but worth a comment: the bound here is "≤ max + one_chunk" rather than a hard ≤ max.
|
|
||
| let mut target_req = Request::new(method.clone(), &target_url); | ||
| self.copy_request_headers(&req, &mut target_req); | ||
| let request_body = if matches!(method, Method::POST | Method::PUT | Method::PATCH) { |
There was a problem hiding this comment.
📝 note — Routes register only GET /sdk, GET /api/*, POST /api/* (line 374), but the body guard here matches POST | PUT | PATCH. PUT/PATCH cannot reach the handler, so those branches are dead defensive code.
Same pattern in permutive.rs:210 and didomi.rs:242. Either tighten the match to POST only, or also register PUT/PATCH routes if the upstream API actually needs them.
| }) | ||
| .map(compat::from_fastly_response) | ||
| } | ||
| (m, path) if integration_registry.has_route(&m, path) => integration_registry |
There was a problem hiding this comment.
👍 praise — Clean elimination of the compat::to_fastly_request / compat::from_fastly_response round-trip for integration dispatch. The proxy registry now operates on http::Request<EdgeBody> end-to-end, which removes a non-trivial amount of conversion overhead per request and eliminates an entire class of header-mapping bugs.
| ) -> Result<PlatformPendingRequest, Report<TrustedServerError>>; | ||
|
|
||
| /// Parse the response from the provider into an `AuctionResponse`. | ||
| /// |
There was a problem hiding this comment.
👍 praise — Async parse_response migration with PlatformResponse resolves the prior unit-asserted into_bytes()-on-Body::Stream panic class for every provider. The trait doc comment explicitly explains why it's now async ("so implementations can safely drain streaming response bodies without panicking on the Body::Stream variant") — exactly the right note for future implementors.
|
|
||
| log::debug!("Proxying to upstream: {}", target_url); | ||
|
|
||
| // Handle payload size errors explicitly to return 413 instead of 502 |
There was a problem hiding this comment.
👍 praise — Correct 413 vs 502 distinction. PayloadSizeError::TooLarge → 413 (client error); PayloadSizeError::StreamRead → 502 (transport error). This addresses the prior reviewer's misclassification finding cleanly, and is the model the shared collect_body* helpers should follow.
| // Header injection: Fastly's HeaderValue API rejects values containing \r, \n, or \0, | ||
| // Header injection: HeaderValue::from_str rejects values containing \r, \n, or \0, | ||
| // so a crafted EC ID cannot inject additional request headers. | ||
| if let Ok(ref ec_id) = ec_id_result { |
There was a problem hiding this comment.
👍 praise — Defensive HeaderValue::from_str guard against \r/\n/\0 injection on the EC ID before it lands in the request and the response, with a regression test at line 1406 (handle_proxy_replaces_invalid_ec_request_header_with_matching_response_cookie). Good security posture.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Inline annotations for the prior REQUEST_CHANGES review
Adding the per-line comments now that I've filtered them against the PR diff (the previous submission was body-only because GitHub rejected the batch when 3 of 17 comment anchors fell outside the changed hunks). These reference the same findings as #626 (review) — no new findings, just inline anchoring.
The three findings that couldn't be anchored to a diff line are tagged below for reference:
- ♻️ refactor —
run_auctiontakesservicesseparately even thoughcontext.servicesexists. Anchored as a top-level note rather than inline becauseauction/orchestrator.rs:64(therun_auctionsignature) is not in the diff, but the dual-pass pattern is visible at the response-handling sites (e.g.orchestrator.rs:337). - 📝 note —
StubHttpClient::send_asyncdoes not capture request headers.crates/trusted-server-core/src/platform/test_support.rsis not in this PR's diff scope at all; the asymmetry betweensend(lines 266–280) andsend_async(lines 297–320) limits header-shape assertions for prebid/aps/adserver_mock tests. Worth a follow-up issue. - 🏕 camp site —
testlight::Defaultimpls can be derived.testlight.rs:271-285is outside the changed hunks; deriving#[derive(Default)]would replace both manual impls.
| if request_signing_config.enabled { | ||
| let signer = RequestSigner::from_services(context.services)?; | ||
| let params = | ||
| SigningParams::new(request.id.clone(), request_info.host.clone(), request_info.scheme.clone()); |
There was a problem hiding this comment.
🔧 wrench — cargo fmt --all -- --check fails here (line is too long inside the if let Some(request_signing_config) = ... block). The PR description marks [x] cargo fmt, but a fresh local run on this commit reports diffs at:
crates/trusted-server-adapter-fastly/src/main.rs:209crates/trusted-server-adapter-fastly/src/main.rs:236crates/trusted-server-core/src/integrations/prebid.rs:1057(this site)crates/trusted-server-core/src/integrations/registry.rs:673
(11 additional fmt diffs are inherited from the PR12 base — not this PR's responsibility — but PR13 introduced the 4 above.)
Fix: cargo fmt --all.
| req.set_header(HEADER_X_TS_EC, ec_id.as_str()); | ||
| match HeaderValue::from_str(ec_id) { | ||
| Ok(header_value) => { | ||
| req.headers_mut().insert(HEADER_X_TS_EC.clone(), header_value); |
There was a problem hiding this comment.
🔧 wrench — cargo fmt --all -- --check fails here too (req.headers_mut().insert(HEADER_X_TS_EC.clone(), header_value); needs to wrap). Same root cause as the prebid.rs:1057 site. cargo fmt --all will fix it.
| /// # 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 is still unbounded for upstream responses (re-raise from prior review). Inbound bodies are now correctly capped via collect_body_bounded, but every provider/integration response read still drains an unbounded body to Vec<u8>:
integrations/prebid.rs:1160(Prebid Server response — highest risk: third-party hop)integrations/aps.rs:555(APS response)integrations/adserver_mock.rs:375(Mediator response)integrations/lockr.rs:150(Lockr SDK fetch)integrations/permutive.rs:152(Permutive SDK fetch)integrations/datadome.rs:302(DataDome SDK fetch)integrations/google_tag_manager.rs:549(GTM script)
A misbehaving Prebid Server / mediator can ship a multi-GB body and OOM the worker. Recommend routing all of these through collect_body_bounded with a per-callsite cap (e.g., 1–2 MiB for RTB responses, 16 MiB for SDK fetches), or removing the unbounded variant in favor of a single bounded helper.
| EdgeBody::Stream(mut stream) => { | ||
| let mut body_bytes = Vec::new(); | ||
| while let Some(chunk_result) = stream.next().await { | ||
| let chunk = chunk_result.map_err(|error| { |
There was a problem hiding this comment.
🤔 thinking — Stream-read errors are still wrapped as TrustedServerError::Integration for both collect_body and collect_body_bounded (this site at lines 93–98 and the bounded variant at 137–142). This conflates "transport failed mid-body" with "integration semantic error" — both surface as 5xx-server-error with the same shape.
GTM has a bespoke PayloadSizeError::StreamRead path that returns 502 explicitly (google_tag_manager.rs:528). The shared helper doesn't get that distinction.
Fix: introduce a dedicated variant (e.g. TrustedServerError::UpstreamBodyRead) that maps to 502, or document the 5xx mapping explicitly so callers know what they're returning to the client. Re-raise from prior review.
| message: format!("Failed to read request body: {error}"), | ||
| }) | ||
| })?; | ||
| if body_bytes.len() + chunk.len() > max_bytes { |
There was a problem hiding this comment.
🤔 thinking — The size check happens after chunk is materialized in memory, not before. If a single chunk's len() is greater than max_bytes, that chunk has already been allocated and read before being rejected.
In practice Fastly H2/H3 framing chunks are small (~16 KiB), and the EdgeBody::Once path at line 124 already short-circuits on a single-shot body. So this is a small concern, but worth a comment: the bound here is "≤ max + one_chunk" rather than a hard ≤ max.
| *pbs_req.body_mut() = EdgeBody::from(pbs_body); | ||
|
|
||
| // Send request asynchronously with auction-scoped timeout | ||
| let backend_name = BackendConfig::from_url_with_first_byte_timeout( |
There was a problem hiding this comment.
♻️ refactor — Re-raise from prior review: providers still construct BackendConfig inline while integration proxies route through ensure_integration_backend. Other instances:
integrations/aps.rs:517-528integrations/adserver_mock.rs:338-348
The two paths differ on timeout — providers need context.timeout_ms per request, the proxy helper hardcodes 15s.
Fix: widen ensure_integration_backend to accept a Duration, or split into ensure_proxy_backend(15s) / ensure_provider_backend(timeout). Alternatively, document the divergence explicitly.
| pub settings: &'a Settings, | ||
| pub request: &'a Request, | ||
| pub request: &'a Request<EdgeBody>, | ||
| pub client_info: &'a ClientInfo, |
There was a problem hiding this comment.
♻️ refactor — Re-raise from prior review: client_info is redundant with services.client_info().
AuctionContext now carries both client_info: &'a ClientInfo and services: &'a RuntimeServices, and RuntimeServices::client_info() already exposes the same value. auction/endpoints.rs:105-108 populates both. Two paths to the same data invites drift.
Fix: drop the client_info field; callers use context.services.client_info().
| match provider.request_bids(request, &provider_context) { | ||
| match provider.request_bids(request, &provider_context).await { | ||
| Ok(pending) => { | ||
| let request_backend_name = pending |
There was a problem hiding this comment.
🤔 thinking — pending.backend_name().unwrap_or_else(|| backend_name.clone()) silently falls back to the predicted name. If the runtime returns None for pending.backend_name() and PlatformResponse::backend_name is set to something else by the platform impl, the response handler at line 392 will fail to identify the provider and log "Received response from unknown backend" — meaning that provider's bids are silently dropped.
Worth a log::debug! (or warn!) when the fallback fires, so production has a signal if PlatformPendingRequest::backend_name() ever stops returning the expected value.
| .change_context(Self::error("Failed to build Didomi proxy request"))?; | ||
| self.copy_headers( | ||
| &backend, | ||
| services.client_info.client_ip, |
There was a problem hiding this comment.
♻️ refactor — Inconsistent access: this reaches the pub(crate) field directly (services.client_info.client_ip) while auction/endpoints.rs:72, auction/endpoints.rs:105, and prebid.rs:1378 use the public method (services.client_info()). One other field-access site is at auction/formats.rs:145.
Fix: standardize on the method (services.client_info().client_ip).
|
|
||
| let mut target_req = Request::new(method.clone(), &target_url); | ||
| self.copy_request_headers(&req, &mut target_req); | ||
| let request_body = if matches!(method, Method::POST | Method::PUT | Method::PATCH) { |
There was a problem hiding this comment.
📝 note — Routes register only GET /sdk, GET /api/*, POST /api/* (line 374), but the body guard here matches POST | PUT | PATCH. PUT/PATCH cannot reach the handler, so those branches are dead defensive code.
Same pattern in permutive.rs:210 and didomi.rs:242. Either tighten the match to POST only, or also register PUT/PATCH routes if the upstream API actually needs them.
…/edgezero-pr13-integration-provider-type-migration
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!