Migrate handler layer to HTTP types (PR 12)#624
Migrate handler layer to HTTP types (PR 12)#624prk-Jr wants to merge 12 commits intofeature/edgezero-pr11-utility-layer-migration-v2from
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Clean migration of the handler layer from fastly::Request/fastly::Response to http::Request<EdgeBody>/http::Response<EdgeBody>. The conversion boundary is pushed to the adapter entry/exit and the still-unmigrated integration trait boundary. Well-tested, CI passes.
Non-blocking
♻️ refactor
- Duplicate error-to-response functions:
http_error_response(new, returnsHttpResponse) andto_error_response(existing, returnsFastlyResponse) implement identical logic with different return types. Expected migration scaffolding — track for cleanup in PR 15 when Fastly types are fully removed. (main.rs:255-267/error.rs:10)
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
Solid mechanical migration of the handler layer from Fastly request/response types to http crate types. The conversion boundary is cleanly pushed to the adapter entry point, with proper compat shims for the still-unmigrated integration/provider boundary.
Non-blocking
🤔 thinking
platform_response_to_fastlyduplicated into orchestrator.rs: Copy-pasted fromproxy.rsrather than shared viacompat.rs. Two separate HTTP→Fastly response paths risk divergence (this copy usesset_headerwhich drops multi-value headers;compat::to_fastly_responsedoesn't). (orchestrator.rs:15)- sync→async
handle_publisher_request: Public API signature change — only caller usesblock_onso it works, but worth documenting. (publisher.rs:305)
♻️ refactor
Cursor::new(body.into_bytes())repeated 4 times: Could extract a smallbody_as_readerhelper to centralize body materialization. (proxy.rs:175,publisher.rs:228,246,263)
🌱 seedling
DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUThardcoded at 15s: Good to make it explicit, but may need to be configurable per deployment. (publisher.rs:22)
📝 note
- Integration proxy double-conversion: GTM, GPT, testlight each wrap
proxy_requestwithfrom_fastly_request/to_fastly_response, copying bodies twice. Expected for the PR 13 boundary.
⛏ nitpick
- Inconsistent test type alias style:
proxy.rstests useHttpMethod/HttpStatusCodeprefixes;publisher.rstests don't. The unprefixed style is cleaner.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
Mechanically sound migration of the handler layer from fastly::{Request,Response} to http::{Request,Response}<EdgeBody>. The adapter entry/exit is now the only fastly conversion site for handlers, and the migration guard test is extended to lock this in. Overall high quality — a handful of concrete concerns to address before merge, mostly around silent failure modes that the type migration makes easier to miss.
Blocking
🔧 wrench
- Silent
EdgeBody::Streamdrop —compat.rs:132-134andauction/orchestrator.rs:29-32log a warning and drop the body on theStreamvariant. No test pins this behavior;debug_assert!only fires in debug. If a streaming body ever reaches this boundary, the client gets an empty 200. - Unbounded request body →
String—proxy.rs:889andproxy.rs:1003doString::from_utf8(req.into_body().into_bytes().to_vec())with no size cap. OOM/DoS risk on hostile POST. - Silent JSON serialization fallback —
proxy.rs:1156returns{}onserde_json::to_stringfailure instead of propagating an error. - Geo lookup moved before auth —
main.rs:95-110runs the geo lookup on every request, including ones that will immediately 401. Confirm this is intentional.
❓ question
- Integration trait still fastly-based —
main.rs:182-196pays a fullhttp → fastly → httpround-trip per integration request. Deferred to PR 13? If so, please leave a// TODO(PR13)marker. to_fastly_request_refdrops body —auction/endpoints.rs:90call site has no comment explaining why the empty body is safe. Please add one line so this invariant doesn't regress.
Non-blocking
🤔 thinking
- Multi-valued
Set-Cookiesurvival throughrebuild_response_with_body(proxy.rs:138) — add a test, also considermem::taketo avoid the header clone. - Invalid
settings.response_headersare silently warn-skipped at request time (main.rs:250-262); CLAUDE.md prefers startup-time validation.
♻️ refactor
migration_guards.rsuses substring matching on"fastly::Request"— prefer a\bfastly::(Request|Response|http::(Method|StatusCode))\bregex to avoid false positives on future string literals or doc comments.
🌱 seedling
- End-to-end streaming (
EdgeBody::Stream) is effectively unreachable today. Worth a follow-up to preserve streaming semantics through publisher fetch → rewrite → response once PR 13 lands.
📝 note
publisher.rs:325vspublisher.rs:361— inconsistentservices.client_info()vsservices.client_info.client_ipaccess idiom.
👍 praise
- Tight adapter boundary in
main.rs:89-120: onefrom_fastly_request/to_fastly_responsepair framingroute_request. Very clean. insert_geo_headerwarn-and-continue is the right shape for derived geo data.- Migration guard extended to cover the handler layer — exactly the right regression gate for this refactor.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
(All three GitHub Actions checks — browser integration tests, integration tests, prepare integration artifacts — are green.)
publisher.rs:325vspublisher.rs:361— inconsistentservices.client_info()(method) vsservices.client_info.client_ip(field) access idiom. Please pick one.
Brings in PR11 review-finding fixes (compat shim additions, testlight integration changes, migration guard comments). Conflicts resolved by keeping PR12's http-native handler layer throughout — all compat shim insertions from PR11 are superseded by the http type migration.
aram356
left a comment
There was a problem hiding this comment.
Summary
Second-round review. The PR addresses all four blocking findings and both questions from the prior review — unbounded body → String in sign/rebuild, silent JSON serialization fallback, geo lookup before auth, silent EdgeBody::Stream drop (now test-pinned in compat.rs), TODO(PR13) marker for the integration trait, and the body-drop comment on to_fastly_request_ref. CI is green.
Two new blocking issues — public POST endpoints /verify-signature and /auction still accept unbounded bodies — are the only security-material gaps in this round. The rest are follow-ups and polish.
Blocking
🔧 wrench
- Unbounded body on
/verify-signature(crates/trusted-server-core/src/request_signing/endpoints.rs:89) — public endpoint materializes the full request into memory with no cap. See inline. - Unbounded body on
/auction(crates/trusted-server-core/src/auction/endpoints.rs:43) — public endpoint, same pattern. See inline.
Non-blocking
🤔 thinking
platform_response_to_fastlystill duplicated (crates/trusted-server-core/src/auction/orchestrator.rs:19-40vscrates/trusted-server-core/src/compat.rs:119-138) — the prior review flagged this. Now consistent onappend_headerand documented as PR 15 removal, which is an improvement, butplatform_resp.responseis alreadyhttp::Response<EdgeBody>and can be passed tocompat::to_fastly_responsedirectly. See inline.- Admin endpoints also lack body-size caps (
crates/trusted-server-core/src/request_signing/endpoints.rs:170, 270) — basic-auth-protected so attack surface is narrow, but defence-in-depth is cheap onceRequestTooLargeexists. See inline. EdgeBody::Streamsilent drop in release (crates/trusted-server-core/src/compat.rs:74-77, 132-135andcrates/trusted-server-core/src/auction/orchestrator.rs:29-32) — tests atcompat.rs:584-629pin the drop-to-empty behaviour, anddebug_assert!catches it in debug, but in release a future streaming backend would silently return a 200 with an empty body. Acceptable transient debt given PR 15 removal scope — please carry this into the PR 15 plan so streaming isn't silently lost when the boundary moves.
♻️ refactor
- No 413 regression tests for the new size-capped endpoints (
crates/trusted-server-core/src/proxy.rs:884-891and:1007-1014) — the 65536-byte cap is a security control with no negative test. GTM has the pattern worth copying (crates/trusted-server-core/src/integrations/google_tag_manager.rs:1100-1137). See inline.
📝 note
response_headersvalidation uses.expect()in the hot path (crates/trusted-server-adapter-fastly/src/main.rs:249-255). Correct today becauseSettings::prepare_runtimevalidates at load (crates/trusted-server-core/src/settings.rs:486-498), butroute_requestkeeps a defensive fallback for the handler regex with the same "validated at load time" invariant (main.rs:132-138). Consider matching that pattern — warn-skip instead of panic — for symmetry and one less panic site in the wasm binary.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
…tion - Add VERIFY_MAX_BODY_BYTES (4096) cap to handle_verify_signature - Add AUCTION_MAX_BODY_BYTES (65536) cap to handle_auction - Add ADMIN_MAX_BODY_BYTES (4096) cap to handle_rotate_key and handle_deactivate_key - Delete platform_response_to_fastly from orchestrator; both call sites now use crate::compat::to_fastly_response directly - Add sign_rejects_oversized_body and rebuild_rejects_oversized_body regression tests asserting 413 on oversized POST bodies
aram356
left a comment
There was a problem hiding this comment.
Summary
Round 3 review. The PR cleanly addresses every blocking finding from the prior two rounds: /verify-signature and /auction now enforce body-size caps (4 KiB and 64 KiB), platform_response_to_fastly was deleted from auction/orchestrator.rs (both call sites use crate::compat::to_fastly_response directly), and /admin/keys/{rotate,deactivate} got matching 4 KiB caps. The expect() in finalize_response (main.rs:251-253) is now safe because Settings::prepare_runtime validates response_headers at startup (settings.rs:486-498).
One new blocking finding: to_fastly_request_ref (introduced in PR 12) silently collapses multi-value headers, which is a latent correctness regression in the provider path. See inline.
CI is green; local cargo test --workspace (814 passed), cargo clippy --workspace --all-targets --all-features -- -D warnings, and cargo fmt --all -- --check all pass.
Blocking
🔧 wrench
to_fastly_request_refclobbers multi-value headers: see inline atcompat.rs:90.
Non-blocking
🤔 thinking
VERIFY_MAX_BODY_BYTES = 4096may be tight for legitimate signed payloads (request_signing/endpoints.rs:78). After base64 signature (~88 bytes for Ed25519),kid, JSON wrapper, and quoting/escaping, the usablepayloadbudget is ~3 KiB. Plausibly fine for typical OpenRTB-shaped payloads but can clash with publishers signing larger blobs (e.g. embedded events). Consider a brief// safety:comment explaining the chosen limit, or making it config-derived alongside the existing GTMmax_beacon_body_sizepattern.
📝 note
- Body-size caps fire after the body is already materialized into memory (
compat.rs:40-43).compat::from_fastly_requestcallsreq.take_body_bytes()at the adapter entry, so the entire body is buffered before any handler-level cap runs. The new caps protect parse allocations but not the receive allocation. This matches the compat shim's semantics today and is a PR-15 concern, not PR-12 — worth pinning in the PR 15 plan that streaming-aware caps need to land with the boundary removal so the protection isn't quietly downgraded when the shim is replaced.
♻️ refactor
-
No 413 regression tests for
/verify-signature,/auction,/admin/keys/{rotate,deactivate}(request_signing/endpoints.rs:93,183,292,auction/endpoints.rs:45). This commit addedsign_rejects_oversized_bodyandrebuild_rejects_oversized_body(proxy.rs:2358-2395) — exactly what the previous review asked for. The same pattern wasn't applied to the four endpoints whose caps are introduced in this commit. Without negative tests, a future commit that drops any cap won't be caught. Same shape as the existing proxy tests — five lines per endpoint. -
Body-size-cap pattern duplicated 6× across handlers.
if body.len() > MAX { return Err(Report::new(RequestTooLarge { format!("X payload {} exceeds limit of {}", body.len(), MAX) })); }appears atproxy.rs:884,proxy.rs:1007,auction/endpoints.rs:45, and three times inrequest_signing/endpoints.rs(93, 183, 292). A small helper (fn enforce_max_body_size(bytes: &[u8], limit: usize, what: &'static str) -> Result<(), Report<TrustedServerError>>) inhttp_utilwould drop ~50 lines and centralize the message format. Defer if you'd rather land in a follow-up.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
…feature/edgezero-pr12-handler-layer-types
…feature/edgezero-pr12-handler-layer-types
aram356
left a comment
There was a problem hiding this comment.
Summary
Round-4 review. Round-3's blocking finding (to_fastly_request_ref clobbering multi-value headers) is resolved at compat.rs:95 with a regression test at compat.rs:478. CI is green.
Two new concerns this round: (1) the round-3 ♻️ ask for 413 regression tests on the four endpoints whose caps were introduced in this PR is still unaddressed — only the proxy endpoints got sign_rejects_oversized_body/rebuild_rejects_oversized_body. (2) Two doc-vs-code mismatches in compat.rs — the # Panics blocks describe a panic site that doesn't exist (URL parsing has a silent fallback to /), while the real panic site (header construction) is undocumented.
Blocking
🔧 wrench
- No 413 regression tests for
/verify-signature,/admin/keys/{rotate,deactivate}(request_signing/endpoints.rs:103, :254, :381). Round-3 explicitly asked. See inline. - No 413 regression test for
/auction; file has no#[cfg(test)]module (auction/endpoints.rs:45). See inline.
Non-blocking
🤔 thinking
# Panicsdoc claims URL-parse panic that cannot occur (compat.rs:42-44, compat.rs:56-58). Doc/code mismatch. See inline.- Silent URL fallback to
/masks malformed URIs (compat.rs:18-21).build_http_requestusesunwrap_or_else(|_| http::Uri::from_static("/")). Realistically unreachable given Fastly pre-validation, but produces silent misrouting (catch-all hits the publisher origin at main.rs:215) if it ever fires. Either document why the fallback is reachable-but-safe, or assert the invariant withexpect("should parse Fastly-validated URL").
♻️ refactor
65536literal duplicated 4× in proxy.rs (proxy.rs:884, :887, :1007, :1010) — the only handler in this PR not using named constants. See inline.String::from_utf8(body_bytes.to_vec())allocates twice (proxy.rs:892, :1015). See inline.- Body-size cap pattern duplicated 6× (carry-over from round 3). Same
if body.len() > MAX { return Err(RequestTooLarge { format!(...) }) }shape at proxy.rs:884, proxy.rs:1007, auction/endpoints.rs:45, and three times in request_signing/endpoints.rs (103, 254, 381). A smallenforce_max_body_size(bytes, limit, what)helper inhttp_utilwould centralize the message format and remove ~50 lines.
🌱 seedling
DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUT = 15sstill hardcoded (publisher.rs:21). Carry-over from round 1 — worth a config knob during PR-15 boundary cleanup.- Body-size caps still fire after
req.take_body_bytes()materializes (compat.rs:46). Carry-over from round 3 — caps protect parse allocations but not the receive allocation. PR-15 streaming-aware shim should land alongside boundary removal so the protection isn't quietly downgraded.
📝 note
platform_response_to_fastlyis now the only diverging conversion (compat.rs:245-264). ReturnsErron streaming bodies instead ofto_fastly_response's silent-drop. The asymmetry is correct for orchestrator use, but worth pinning in the PR-15 plan so streaming closes consistently.
CI Status
- browser integration tests: PASS
- integration tests: PASS
- prepare integration artifacts: PASS
…feature/edgezero-pr12-handler-layer-types
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of the handler-layer HTTP-types migration. The migration is structurally clean, but two concerns block merge: (1) the publisher streaming dispatch added in #562 is regressed — PublisherResponse::Stream now buffers the entire pipeline output instead of writing chunks via stream_to_client(), and (2) cargo test --workspace fails on the current head because the debug_assert! in compat::to_fastly_response (introduced in PR 11 and inherited here) contradicts the test that exercises the documented silent-drop fallback. CI did not catch (2) because PR 624's base branch causes test.yml and format.yml to skip.
Blocking
🔧 wrench
-
Streaming regression on the publisher fallback path. Before this PR,
Ok(PublisherResponse::Stream { … })inadapter-fastly/src/main.rscalledresponse.stream_to_client()and wrote chunks viastream_publisher_body. After this PR,resolve_publisher_responsematerializes the entire pipeline output into aVec<u8>, setsContent-Length, replaces the body, and the adapter sends viacompat::to_fastly_response(response).send_to_client(). The whole publisher response is now buffered in WASM memory.Observable: the
PublisherResponse::Streamenum variant becomes equivalent toPublisherResponse::Buffereduntil streaming is restored. TTFB is delayed by the full pipeline pass, and WASM memory pressure scales with body size.Structural cause:
compat::to_fastly_responsecannot move anEdgeBody::Streaminto afastly::StreamingBody— there is no streaming bridge across the compat shim. Two reasonable directions:- Preferred: Keep the streaming dispatch in the adapter. Have
route_requestreturnResult<HandlerOutcome, …>whereHandlerOutcome::Streaming(…)carries the unbuffered body+params, andmain()opensstream_to_client()on the converted Fastly response and callsstream_publisher_bodyagainst it. Honest "no behavior regressions" version of PR 12. - Alternative: Block the streaming arm and route everything through
BufferedProcesseduntil PR 15 introduces a streaming body bridge — but explicitly call this out as a known regression in the PR description and add a tracking issue.
No test today asserts streaming behavior. The publisher tests (
stream_publisher_body_*) all useVec<u8>outputs and don't distinguish streamed-to-client from buffered-then-sent. So the regression is entirely silent. - Preferred: Keep the streaming dispatch in the adapter. Have
-
cargo test --workspacefails:compat::tests::to_fastly_response_with_streaming_body_produces_empty_bodypanics on adebug_assert!that contradicts the test.In
crates/trusted-server-core/src/compat.rs(line ~138), the function assertsmatches!(&body, EdgeBody::Once(_)). The test below it (line ~676) constructs anEdgeBody::Streamand asserts the body comes out empty — so the test panics incargo testdebug builds.Both the
debug_assert!and the test were introduced in PR 11 (commit76df6f8), so PR 624 inherits the contradiction rather than introducing it — but it materializes here because (a) the PR's test plan checkbox[x] cargo test --workspacedoes not pass on the current head, and (b) when this stack lands onmainand a downstream PR opens, the standardtest.ymlworkflow will fail. The PR 11 review can fix it upstream, or this PR can apply the fix.Fix (preferred — the warn-and-empty fallback is documented behavior):
pub fn to_fastly_response(resp: http::Response<EdgeBody>) -> fastly::Response { // … build headers … match body { EdgeBody::Once(bytes) => { if !bytes.is_empty() { fastly_resp.set_body(bytes.to_vec()); } } EdgeBody::Stream(_) => { log::warn!("streaming body in compat::to_fastly_response; body will be empty"); } } fastly_resp }
Alternative: keep the
debug_assert!and gate the test on#[cfg(not(debug_assertions))]. But the warn-log already covers misuse and the documented fallback path should be exercised by tests.
Non-blocking
🤔 thinking
- Silent body drop on
EdgeBody::Streamincompat. Bothto_fastly_requestandto_fastly_responselog a warning and emit an empty body when handed a streamingEdgeBody. Combined with the streaming-dispatch removal above, this means a future caller that tries to stream through the compat boundary will silently see truncated bodies. Promote the misuse to aResultreturn (the Fastly adapter'sedge_request_to_fastlyalready does this), or add a comment listing the audited non-streaming call sites so the constraint is verifiable. Not in PR 12's diff but the streaming-regression fix will reintroduce the constraint.
♻️ refactor
bytes.to_vec()copies the full body across the compat boundary.compat.rs:144-146andcompat.rs:81-84memcpy the body. For large publisher responses this is unavoidable O(N) at the shim. Worth a one-line comment that this cost goes away with PR 15 so future readers don't try to optimize inside the shim. (Not in PR 12's diff.)
🌱 seedling
- Sync between
route_requestadmin routes andSettings::ADMIN_ENDPOINTSis comment-only.main.rs:166carries a "Keep in sync with Settings::ADMIN_ENDPOINTS" comment. A test that compares the constant against the routes registered inroute_requestwould prevent future drift. Follow-up issue.
📝 note
- CI workflow gate gap.
.github/workflows/test.ymland.github/workflows/format.ymltrigger only onpull_request: branches: [main]. PR 624 targets the PR 11 feature branch, so cargo test, fmt, and clippy never ran on this PR. Onlyintegration-tests.ymlran (and passed). This is how the failingcompattest stayed invisible. Out of scope for PR 12 to fix — but the consequence (a regression that will only surface when this lands on main and a downstream PR opens) is in scope. Either drop thebranches: [main]filter for the EdgeZero stack PRs, orworkflow_dispatchthe test workflow against each stack head.
CI Status
- fmt: PASS (locally)
- clippy: PASS (locally)
- rust tests: FAIL locally (
compat::tests::to_fastly_response_with_streaming_body_produces_empty_body); not run on CI for this PR - integration tests: PASS (CI)
- browser integration tests: PASS (CI)
| log::warn!("geo lookup failed: {e}"); | ||
| None | ||
| }) | ||
| }; |
There was a problem hiding this comment.
🤔 thinking — 401 responses no longer carry geo headers — silent behavior change.
Previously geo_info was looked up before routing and applied via finalize_response to every path including auth early-returns, so 401s had geo headers. Now geo_info = if status == UNAUTHORIZED { None } else { … } skips the lookup on 401.
This may be intentional (avoid leaking geo to unauthenticated callers) but it's a silent change. Add a one-line comment to make the rationale explicit:
// Skip geo lookup for 401s so geo headers are not exposed to unauthenticated
// callers. Authenticated routes do their own lookups for consent context.
let geo_info = if response.status() == StatusCode::UNAUTHORIZED { … };| })) | ||
| }), | ||
| (m, path) if integration_registry.has_route(&m, path) => { | ||
| // TODO(PR13): migrate integration trait to http types here |
There was a problem hiding this comment.
📝 note — TODO(PR13) is captured. The bridge through compat::to_fastly_request / compat::from_fastly_response is correct for the scope of this PR. No action here — confirming the deferred work is tracked.
| .unwrap_or_else(|e| { | ||
| log::warn!("geo lookup failed: {e}"); | ||
| None | ||
| }); |
There was a problem hiding this comment.
🤔 thinking — Geo lookup is now duplicated.
The handler does its own lookup here for the consent context, and the adapter does another lookup at response-finalization time for header injection (main.rs finalize_response). Lookup is cheap (in-memory on Fastly) so this is more of a 📌 out of scope note — a follow-up could thread the same Option<GeoInfo> through both. Same pattern in auction/endpoints.rs:75-81.
| use fastly::geo::{geo_lookup, Geo}; | ||
| use fastly::{Request, Response}; | ||
| use edgezero_core::body::Body as EdgeBody; | ||
| use fastly::geo::Geo; |
There was a problem hiding this comment.
📝 note — fastly::geo::Geo import remains in core after this migration. The geo_from_fastly field-mapping helper still lives here, so crates/trusted-server-core retains a Fastly type leak after PR 12. Predates this PR and is acknowledged out of scope, but worth tracking for PR 13 / PR 15 since geo.rs is one of the migrated files.
| let auction_status = match auction_result { | ||
| Ok(resp) => resp.status(), | ||
| Err(ref e) => e.current_context().status_code(), | ||
| }; |
There was a problem hiding this comment.
♻️ refactor — Dedup the match Result { Ok(r) => r.status(), Err(e) => e.current_context().status_code() } block.
It appears twice in this file (auction + publisher cases). A small helper at the top of the test module would clean this up:
fn status_of(
result: &Result<HttpResponse, Report<TrustedServerError>>,
) -> StatusCode {
match result {
Ok(resp) => resp.status(),
Err(e) => e.current_context().status_code(),
}
}Not a blocker — duplication is small.
| params, | ||
| } => { | ||
| let mut output = Vec::new(); | ||
| stream_publisher_body(body, &mut output, ¶ms, settings, integration_registry)?; |
There was a problem hiding this comment.
🔧 wrench — Streaming regression: this arm now buffers the full pipeline output instead of streaming chunks to the client.
Before this PR, the adapter handled Ok(PublisherResponse::Stream { … }) by calling response.stream_to_client() and writing chunks via stream_publisher_body against the StreamingBody — the streaming pipeline added in #562 (commit 5ff3e676) emitted bytes as the upstream body was processed.
After this PR, resolve_publisher_response materializes the entire pipeline output into a Vec<u8>, sets Content-Length, replaces the body, and the adapter sends via compat::to_fastly_response(response).send_to_client(). The whole publisher response is now buffered in WASM memory:
PublisherResponse::Streambecomes observably equivalent toPublisherResponse::Buffered— the variant no longer means "stream".- TTFB is delayed by the full pipeline pass rather than starting at first chunk.
- WASM memory pressure scales with body size for every publisher response.
Structural cause: compat::to_fastly_response cannot move an EdgeBody::Stream into a fastly::StreamingBody — there is no streaming bridge across the compat shim, so the only way to honor the variant is to streaming-dispatch from the adapter itself before going through compat.
Fix (preferred) — keep the streaming dispatch in the adapter:
// Have route_request return Result<HandlerOutcome, …> where HandlerOutcome
// distinguishes streamed vs buffered responses, then in main():
match outcome {
HandlerOutcome::Streaming { mut response, body, params } => {
finalize_response(&settings, geo_info.as_ref(), &mut response);
let mut fastly_resp = compat::to_fastly_response_skeleton(response);
let mut sb = fastly_resp.stream_to_client();
if let Err(e) = stream_publisher_body(body, &mut sb, ¶ms, &settings, &integration_registry) {
log::error!("Streaming processing failed: {e:?}");
drop(sb);
} else if let Err(e) = sb.finish() {
log::error!("Failed to finish streaming body: {e}");
}
}
HandlerOutcome::Buffered(response) => {
compat::to_fastly_response(response).send_to_client();
}
}Alternative — fold this arm into BufferedProcessed until PR 15 introduces a streaming body bridge, and call it out as a known regression in the PR description with a tracking issue.
No test today asserts streaming behavior — the publisher tests (stream_publisher_body_*) all use Vec<u8> outputs and don't distinguish streamed-to-client from buffered-then-sent. So the regression is entirely silent until you measure TTFB or memory.
Summary
httprequest/response types so core handlers no longer depend on Fastly request/response APIs.Changes
crates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-core/src/auction/endpoints.rs/auctionhandler tohttptypes and rebuild a Fastly request only at the provider-facingAuctionContextboundary.crates/trusted-server-core/src/auction/formats.rscrates/trusted-server-core/src/auction/orchestrator.rscrates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/geo.rscrates/trusted-server-core/src/integrations/google_tag_manager.rscrates/trusted-server-core/src/integrations/gpt.rscrates/trusted-server-core/src/integrations/testlight.rscrates/trusted-server-core/src/migration_guards.rscrates/trusted-server-core/src/proxy.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/request_signing/endpoints.rsCloses
Closes #493
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest run(not run; no JS/TS files changed)cd 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 ...")tracingmacros (notprintln!)