Add trusted-server-adapter-cloudflare crate (PR 17)#644
Add trusted-server-adapter-cloudflare crate (PR 17)#644prk-Jr wants to merge 16 commits intofeature/edgezero-pr16-axum-dev-serverfrom
Conversation
…ator wasm32-unknown-unknown (Cloudflare Workers) does not support std::time::Instant — it panics at runtime. web_time::Instant is a zero-cost drop-in on native and JS-backed on WASM.
Implements the Cloudflare Workers adapter following the same pattern as trusted-server-adapter-axum: TrustedServerApp implements the Hooks trait, platform services use noop stubs on native (CI-compilable), and the #[event(fetch)] entry point delegates to edgezero_adapter_cloudflare::run_app. Also adds UnavailableHttpClient to trusted-server-core platform module, parallel to the existing UnavailableKvStore.
CI: test-cloudflare job checks native host compile, wasm32-unknown-unknown compile (with cloudflare feature), and runs host-target unit tests. CLAUDE.md: add cloudflare crate to workspace layout and build commands.
…un_app The rev (38198f9) of edgezero used in this workspace requires worker 0.7 (not 0.6) and run_app() takes a manifest_src: &str as first argument. Updated Cargo.toml and lib.rs accordingly.
- Implement CloudflareHttpClient (wasm32 only) using worker::Fetch for real
outbound proxy requests; strip content-encoding/transfer-encoding headers
since the Workers runtime auto-decompresses responses
- Add build.sh with cd-to-SCRIPT_DIR guard so worker-build always runs from
the correct crate root regardless of invocation directory
- Switch wrangler dev task to use --cwd from workspace root (same DX as Fastly)
- Add js-sys to workspace dependencies; reference it via { workspace = true }
- Fix #[ignore] messages on Cloudflare integration tests
- Replace std::time::{SystemTime,UNIX_EPOCH} with web_time in test code for
signing.rs and proxy.rs (consistency with production paths)
- Add NoopConfigStore/NoopSecretStore TODO comment tracking the gap
- Add extract_client_ip unit tests (parses cf-connecting-ip, absent, invalid)
- Remove empty crate_compiles_on_host_target test
- Add CloudflareHttpClient timeout doc noting Workers CPU-budget tradeoff
Replace direct worker::Env store construction with edgezero handles already injected by run_app, reducing #[cfg(target_arch = "wasm32")] blocks from 5 to 2. - ConfigStoreHandleAdapter: bridges ctx.config_store() to PlatformConfigStore — reuses the already-parsed TRUSTED_SERVER_CONFIG JSON handle rather than re-parsing it on every request - KvHandleAdapter: bridges ctx.kv_handle() to PlatformKvStore — reuses the env.kv() handle opened by run_app rather than opening a new one - CloudflareGeo: moved outside #[cfg]; reads cf-ipcountry and related headers via ctx.request().headers() which needs no platform import - CloudflareSecretStoreAdapter: kept under #[cfg] — env.secret() is synchronous at the JS level but PlatformSecretStore::get_bytes is sync while SecretHandle::get_bytes is async; direct env access is required - Add .dev.vars to .gitignore (wrangler convention for local secrets) - Add bytes workspace dep for KvStore impl
…ezero-pr17-cloudflare-adapter
- Implement CloudflareWorkers::spawn() to start wrangler dev; in CI uses wrangler.ci.toml (no build step, uses pre-built bundle); locally uses wrangler.toml (triggers build.sh rebuild) - Add wrangler.ci.toml: no [build] section so wrangler dev skips the worker-build step when the bundle is pre-built as a CI artifact - Add build-cloudflare input to setup-integration-test-env: adds wasm32-unknown-unknown target and runs build.sh with integration test env vars - In prepare-artifacts: enable build-cloudflare and upload build/ dir as artifact - In integration-tests: restore CF build dir, install wrangler, set CLOUDFLARE_WRANGLER_DIR, and remove --skip flags for cloudflare tests
- Add GeoInfo happy-path test: build_geo_lookup_returns_some_with_populated_country verifies country, city, continent, latitude, and longitude are correctly populated when Cloudflare headers are present - Simplify CloudflareSecretStoreAdapter::get_bytes: collapse brittle JsError string-matching guards into a single error arm with contextual message - Document sequential fan-out latency in CloudflareHttpClient: explain sum(DSP_i) vs max(DSP_i) implication and why true parallelism is blocked by the ?Send bound on PlatformHttpClient - Fix stale wrangler.toml comment: update to reflect ConfigStoreHandleAdapter wiring rather than the now-fallback NoopConfigStore - Extend CI triggers to feature branches: format.yml and test.yml now run fmt/clippy/tests on PRs targeting feature/** so stacking PRs are gated - Fix fmt violations caught during pre-review verification: platform.rs two-line Err wrapping and plan doc Prettier formatting
The adapter's tokio dev-dependency uses rt-multi-thread which is not supported on wasm32. It is already tested in the dedicated test-cloudflare job; exclude it from the workspace wasm test to avoid the compile error.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Adds the Cloudflare adapter, runtime wiring, and CI coverage. CI is green, but I found two correctness issues that should be addressed before merge, plus two smaller follow-ups.
| } | ||
| }; | ||
|
|
||
| RouterService::builder() |
There was a problem hiding this comment.
🔧 wrench — Cloudflare routes skip the auth/finalize middleware chain.
Both existing adapters wrap their route tables with FinalizeResponseMiddleware and AuthMiddleware, but this router is built without either layer. That means Cloudflare no longer enforces per-path basic auth and it also skips the standard response finalization headers on every response.
Suggestion:
Port the Cloudflare adapter to use the same middleware chain as Axum/Fastly and add at least one middleware/auth test so this cannot regress silently.
| request: PlatformHttpRequest, | ||
| ) -> Result<PlatformPendingRequest, Report<PlatformError>> { | ||
| let backend_name = request.backend_name.clone(); | ||
| let response = self.execute(request).await?; |
There was a problem hiding this comment.
🔧 wrench — send_async() serializes auctions and does not enforce the request deadline.
send_async() eagerly awaits self.execute(request).await, so the auction orchestrator launches Cloudflare providers one by one instead of handing back true in-flight requests. execute() also waits on worker::Fetch::send().await without any timeout/abort path, so a slow upstream can block past the remaining auction budget.
Suggestion:
Make PlatformPendingRequest hold an actual in-flight fetch plus timeout/abort state and have select() resolve the ready request. If that is out of scope here, it would be safer to reject unsupported multi-provider fan-out on Cloudflare than to silently degrade auction behavior.
|
|
||
| impl PlatformBackend for NoopBackend { | ||
| fn predict_name(&self, spec: &PlatformBackendSpec) -> Result<String, Report<PlatformError>> { | ||
| Ok(format!("{}_{}", spec.scheme, spec.host)) |
There was a problem hiding this comment.
🤔 thinking — backend names are too coarse for reliable response correlation.
This only includes {scheme}_{host}, but the rest of the system treats port / timeout / certificate mode as part of backend identity. If two concurrent requests share a host but differ on those fields, the orchestrator's backend-to-provider map can collide and misattribute a response.
Suggestion:
Mirror the Axum/Fastly naming scheme and include at least port and timeout in the generated name.
| } | ||
|
|
||
| #[test] | ||
| #[ignore = "requires Docker and a running `wrangler dev` instance; see environments/cloudflare.rs"] |
There was a problem hiding this comment.
📝 note — this ignore message is stale now that CloudflareWorkers::spawn() starts wrangler dev itself.
The test no longer requires a separately running wrangler dev instance, so this text will mislead anyone trying to run the ignored test locally.
Suggestion:
Update the ignore reason to describe the real prerequisites instead (Docker, wrangler installed, and a prebuilt Cloudflare bundle).
Resolved conflicts: - .cargo/config.toml: extend test-fastly alias to also exclude trusted-server-adapter-cloudflare - .github/workflows/test.yml: use cargo test-fastly alias in CI (from PR16) and keep test-cloudflare job (from PR17) - CLAUDE.md: keep Cloudflare workspace entry and cargo test-axum alias; merge both adapters' commands - crates/trusted-server-core/src/proxy.rs: use #[tokio::test] + std::time pattern (consistent with PR16 tests) - Cargo.lock: regenerated to reflect merged workspace state
Brings in the default-members fix: restricts workspace default-members to `trusted-server-adapter-fastly` so Viceroy can locate the binary via `cargo run --bin` during `cargo test-fastly`.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds the Cloudflare Workers adapter, with web_time migration in core and a new CI job. The middleware-bypass and backend-naming concerns from the prior review are addressed and have regression tests, but the second prior 🔧 (sequential fan-out + missing per-request deadline) is mitigated only by documentation and a noisy log warning — the underlying behavior is unchanged. CI has three required gates failing.
Blocking
🔧 wrench
- CI is red on three required gates —
cargo fmt --all -- --check,format-docs, andintegration testsare failing on the latest run. The integration-tests failure is especially concerning because this PR is the one adding theCloudflareWorkersruntime to that matrix and the new env-var/wrangler wiring (integration-tests.yml:74-105). Either the new Cloudflare integration test broke, or it isn't actually running and another regression slipped in. Runcargo fmt --all,cd docs && npm run format, and reproduce the integration-tests failure locally withCLOUDFLARE_WRANGLER_DIRset; address whatever surfaces. Don't merge with red required checks. - Per-provider auction timeout is silently dropped on Cloudflare (
crates/trusted-server-adapter-cloudflare/src/platform.rs:289-315) — see inline. select()warning firesn-1times per auction → log noise on every multi-provider request (crates/trusted-server-adapter-cloudflare/src/platform.rs:329-337) — see inline.
Non-blocking
🤔 thinking
send_asyncalways materializes the body, then re-serializes it forselect(platform.rs:289-315) — see inline.Body::Streamsilently dropped in outbound request body (platform.rs:228-235) — see inline.
♻️ refactor
get_fallback/post_fallbacknear-duplicates (app.rs:307-363) — see inline.auth_middleware_rejects_with_401_when_credentials_requireddoesn't actually test 401 (tests/routes.rs:43-75) — see inline.
📌 out of scope
wrangler.tomlships a placeholder KV id (wrangler.toml:11) —id = "REPLACE_WITH_YOUR_KV_NAMESPACE_ID"is fine forwrangler dev --localbut will fail at deploy. Either call this out ingetting-started.mdnext to the deploy instructions, or use akv_namespaces[].preview_idso dev still works while the prod id stays empty/required. Follow-up issue is fine.
📝 note
worker::Env::clone()per request (platform.rs:444-451) — see inline.
Prior-review status
- ✅ Cloudflare routes skip middleware → fixed (new
src/middleware.rs+ regression tests intests/routes.rs:23-75). ⚠️ send_asyncserializes / no deadline → only documentation added; the underlying serialization and missing per-request timeout remain (see blocking findings above).- ✅ Backend names too coarse → fixed (
platform.rs:62-77includes scheme, host, port, timeout_ms, cert flag). - ✅ Stale ignore message → fixed (
integration.rs:139,147).
CI Status
- cargo fmt: FAIL
- format-docs: FAIL
- integration tests: FAIL
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test: PASS
- cargo test (axum native): PASS
- vitest: PASS
- format-typescript: PASS
- browser integration tests: PASS
| request: PlatformHttpRequest, | ||
| ) -> Result<PlatformPendingRequest, Report<PlatformError>> { | ||
| let backend_name = request.backend_name.clone(); | ||
| let response = self.execute(request).await?; |
There was a problem hiding this comment.
🔧 wrench — Per-provider auction timeout is silently dropped on Cloudflare.
The orchestrator computes effective_timeout = remaining_ms.min(provider.timeout_ms()) per provider (orchestrator.rs:294) and bakes it into the backend name so Phase 1 can pick a budget. On Fastly, first_byte_timeout is enforced by the host. On Cloudflare, CloudflareHttpClient::execute calls Fetch::Request(...).send().await with no timeout/abort at all — the only ceiling is the Workers global CPU limit (~30s on paid plans).
This is the prior review's #2 concern. The PR addresses latency-blowup awareness (doc + a log::warn! in select()) but does not address the correctness half: a single slow upstream blocks the whole worker invocation and ignores the auction budget. With sequential send_async, an unhealthy DSP at position 0 prevents any other DSP from even being attempted. With Workers' default 30s wall-clock and a typical 300 ms auction budget, that's a 100× budget overrun.
Fix (any of):
- Wire an
AbortControllerper fetch:
use worker::{AbortController, RequestInit};
let aborter = AbortController::default();
init.with_signal(Some(&aborter.signal()));
// schedule `aborter.abort()` after `request.first_byte_timeout` via setTimeoutThis is the proper fix; worker::AbortController exists in worker 0.7.
- If timeout-aware fan-out is genuinely out of scope for PR-17, fail loud rather than degrade silently: detect
pending_requests.len() >= 2insend_async(not inselect) and returnPlatformError::HttpClientwith "multi-provider fan-out is not supported on Cloudflare; configure a single provider or use Fastly." This matches what the prior reviewer suggested ("safer to reject … than to silently degrade").
The current log::warn! runs n-1 times per auction (see separate finding) and a slow provider can still blow the worker's wall-clock budget regardless of any TS-level timeout config.
| degrade to sequential latency. Use the Fastly adapter for parallel \ | ||
| DSP fan-out.", | ||
| pending_requests.len() | ||
| ); |
There was a problem hiding this comment.
🔧 wrench — select() warning fires n-1 times per auction → log noise on every multi-provider request.
The orchestrator calls select() once per pending request in a loop (orchestrator.rs:381-388). The if pending_requests.len() >= 2 check is true for the first n-1 calls of every auction. With 5 providers at 1k QPS that's 4,000 warnings/sec, all identical, before any real diagnostic value. This is exactly the kind of thing that gets a log alert page-flooded and then silenced, defeating the purpose.
Fix: Move the warning to send_async and gate it on a OnceCell/AtomicBool so it fires once per worker instance, or downgrade to log::debug! and emit one warning per auction from app.rs. Better: combine with the timeout finding above and turn it into a single Err(...) if multi-provider mode is intentionally unsupported.
| let body_bytes = match response.response.into_body() { | ||
| edgezero_core::body::Body::Once(bytes) => bytes.to_vec(), | ||
| edgezero_core::body::Body::Stream(_) => vec![], | ||
| }; |
There was a problem hiding this comment.
🤔 thinking — send_async always materializes the body, then re-serializes it for select.
execute() already calls resp.bytes().await, so the body is Body::Once. send_async then deconstructs that into (status, headers, body_bytes) only to have select() rebuild a Response. The Body::Stream(_) => vec![] branch at line 306 is unreachable in this code path but silently lossy if the upstream contract ever changes. Either pass the assembled PlatformResponse straight through (skip the deconstruction) or assert unreachable!() on the Stream variant with a useful message.
| "CloudflareHttpClient: Body::Stream is not supported; \ | ||
| outbound request body will be empty" | ||
| ); | ||
| vec![] |
There was a problem hiding this comment.
🤔 thinking — Body::Stream is silently dropped in outbound request body too.
CloudflareHttpClient::execute warns then sends an empty body when the outbound request is Body::Stream. For auction/DSP traffic the body is bid JSON in Body::Once, so this never trips today. But for /first-party/proxy POSTs of large bodies, a streamed inbound request body would be dropped on the floor and the upstream would see an empty POST. A 4xx-with-context error would be safer than producing a malformed request.
|
|
||
| Ok(result.unwrap_or_else(|e| http_error(&e))) | ||
| } | ||
| }; |
There was a problem hiding this comment.
♻️ refactor — get_fallback and post_fallback are near-duplicates.
Two ~30-line closures differ only in whether the /static/tsjs= branch exists. Extract the common dispatch(method, path, ...) helper used by both. Cuts ~30 lines and makes the routing intent obvious. Same pattern is in the Axum adapter — could be done across both. (Also: let path = req.uri().path().to_string() allocates every request when &str would do; minor.)
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn auth_middleware_rejects_with_401_when_credentials_required() { |
There was a problem hiding this comment.
♻️ refactor — auth_middleware_rejects_with_401_when_credentials_required doesn't actually test 401.
The test name says "rejects with 401" but the body comment admits it can't actually verify that (settings may not have basic_auth) and only asserts x-geo-info-available and status != 404. That's a middleware-wiring smoke test, not an auth test. Either rename to middleware_chain_runs_for_auction_endpoint (matches what's tested) or stand up a settings fixture with basic_auth configured and assert status == 401 (matches the name).
| let secret_store: Arc<dyn PlatformSecretStore> = | ||
| edgezero_adapter_cloudflare::CloudflareRequestContext::get(ctx.request()) | ||
| .map(|cf_ctx| { | ||
| Arc::new(CloudflareSecretStoreAdapter { |
There was a problem hiding this comment.
📝 note — every request clones the JS-backed Env to construct CloudflareSecretStoreAdapter. worker::Env clones are Rc<…> under the hood so it's cheap, but worth noting if the secret store ends up in a hot path. Not action-required.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds the trusted-server-adapter-cloudflare crate (edgezero handle adapters for Config/KV/secrets, CloudflareHttpClient, geo extraction from cf-* headers), an integration test environment, and a web_time migration in core. Crate scaffolding and middleware wiring are sound after the prior two review rounds, but CI is red on three jobs (one introduced by this PR) and several correctness/quality concerns from the earlier reviews remain open.
Note: the PR base is feature/edgezero-pr16-axum-dev-server, not main. Two of the four CI failures listed below are inherited from the base — but they still block merge to main.
Blocking
🔧 wrench
cargo fmtfails oncrates/integration-tests/tests/environments/cloudflare.rs— introduced by this PR. Theintegration-testscrate is in the workspaceexcludelist, socargo fmt --allfrom the repo root passes; the CI rustfmt action checks every Rust file. See inline comment.use worker::*;violatesCLAUDE.md—crates/trusted-server-adapter-cloudflare/src/lib.rs:7. See inline.- Integration tests fail with orphaned
workerdprocesses —crates/integration-tests/tests/environments/cloudflare.rs:104-109only kills thewranglerparent;workerdgrandchildren leak. Likely root cause of thetest_all_combinations/test_nextjs_axumfailures observed in CI. See inline. - Per-provider auction timeout silently dropped on Cloudflare (still open from prior review) —
platform.rs:277. See inline. select()warning logsn-1times per multi-provider auction (still open from prior review) —platform.rs:337. See inline.- CI:
clippy::type_complexityfailure —crates/trusted-server-adapter-axum/src/platform.rs:194. Inherited from PR-16 base; cannot merge tomainuntil fixed in the base chain. Suggested:type ResponseTriplet = (u16, Vec<(String, Vec<u8>)>, Vec<u8>);. - CI:
format-docsfailure —docs/guide/architecture.mdanddocs/guide/getting-started.md. Inherited from PR-15 (6d1184d6). Same merge-to-main concern.
Non-blocking
🤔 thinking
send_asynceagerly awaits → multi-provider auctions degrade tosum(latencies)— see inline atplatform.rs:315.Body::Streamsilently dropped on outbound — see inline atplatform.rs:235.- Top-level
worker dispatch error: {e}may leak internals to clients — see inline atlib.rs:20. - Eager full-response buffering with no cap — see inline at
app.rs:96. - Confirm
worker::Error::Displaydoes not leak the secret value — see inline atplatform.rs:391. - Hardcoded port 8787 conflicts with local
wrangler dev— see inline atcloudflare.rs:23.
♻️ refactor
get_fallbackandpost_fallbackare near-duplicates — see inline atapp.rs:363.auth_middleware_rejects_with_401_when_credentials_requireddoesn't actually test 401 — see inline attests/routes.rs:75.
📝 note
- Stale
#[ignore]reason on Cloudflare integration tests — see inline atintegration.rs:139. - The PR base also has red CI (
format-docs,clippy::type_complexity). Recommend rebasing on the merged base before re-requesting review so CI status reflects only this PR's deltas.
⛏ nitpick
Method::from(... .to_ascii_uppercase())— see inline atplatform.rs:216.wrangler.tomlshipsid = "REPLACE_WITH_YOUR_KV_NAMESPACE_ID"—wrangler devrefuses to start until edited. Worth a one-line note inCLAUDE.mdor atopbuild.sh.- The
cloudflarefeature in this crate'sCargo.tomlis effectively redundant —[target.'cfg(target_arch = "wasm32")'.dependencies]already pullsworkerand the adapter feature unconditionally on wasm32, so--features cloudflareonly matters forcargo checkon native. The doc comment names the use case but a one-line "why it isn't default" note would help future readers.
CI Status
- cargo fmt: FAIL (introduced by this PR —
crates/integration-tests/tests/environments/cloudflare.rs) - clippy: FAIL (inherited from base — axum
type_complexity) - rust tests (fastly, axum, cloudflare): PASS
- vitest: PASS
- format-docs: FAIL (inherited from base)
- format-typescript: PASS
- integration tests: FAIL (
test_all_combinations,test_nextjs_axum— likely the workerd cleanup issue) - browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
| @@ -0,0 +1,109 @@ | |||
| use crate::common::runtime::{RuntimeEnvironment, RuntimeProcess, RuntimeProcessHandle, TestError, TestResult}; | |||
There was a problem hiding this comment.
🔧 wrench — cargo fmt fails here (introduced by this PR).
crates/integration-tests/ is in the workspace exclude list, so cargo fmt --all from the repo root passes; the CI rustfmt action checks every Rust file. Reproduces locally with cd crates/integration-tests && cargo fmt --all -- --check. Two diffs in this file — line 1 (long single-line import) and around line 76 (single-line struct init).
Fix:
cd crates/integration-tests && cargo fmt|
|
||
| /// Port wrangler dev binds to. Matches the Axum port; both run sequentially | ||
| /// under `--test-threads=1` so the port is never double-allocated. | ||
| const CLOUDFLARE_PORT: u16 = 8787; |
There was a problem hiding this comment.
🤔 thinking — Hardcoded port 8787 conflicts with local wrangler dev.
The comment claims "Matches the Axum port; both run sequentially under --test-threads=1", but axum.rs:32 actually does super::find_available_port().unwrap_or(AXUM_DEFAULT_PORT) — it prefers a dynamic port. A developer with a wrangler session already on 8787 (the wrangler default) cannot run integration tests. Use find_available_port() and pass --port from the result the same way Axum does.
The comment is also factually misleading — it implies the two runtimes share a fixed port, but Axum's port is dynamic.
| impl Drop for CloudflareHandle { | ||
| fn drop(&mut self) { | ||
| let _ = self.child.kill(); | ||
| let _ = self.child.wait(); |
There was a problem hiding this comment.
🔧 wrench — Process-group leak: wrangler dev spawns workerd as a grandchild, and kill() on the parent does not reap it.
The CI integration tests job is currently failing on test_all_combinations and test_nextjs_axum with connection closed before message completed, and the runner explicitly logs Terminate orphan process: pid (...) (workerd) during cleanup. The orphaned workerd likely holds resources (port, fds, memory) that destabilise the next runtime in the matrix (axum runs after cloudflare).
Fix: spawn wrangler dev as a process-group leader and signal the whole group on drop. Sketch:
use std::os::unix::process::CommandExt;
let mut child = Command::new("wrangler")
.args([...])
.process_group(0)
.spawn()?;
impl Drop for CloudflareHandle {
fn drop(&mut self) {
unsafe {
libc::killpg(self.child.id() as i32, libc::SIGTERM);
}
let _ = self.child.wait();
}
}On Windows, Job Object is the equivalent. This is the most likely root cause of the integration-test CI failure on this PR.
| } | ||
|
|
||
| #[test] | ||
| #[ignore = "requires Docker, wrangler CLI installed, and a prebuilt Cloudflare Workers bundle (run build.sh first)"] |
There was a problem hiding this comment.
📝 note — #[ignore] reason still partially stale.
Christian flagged this on 2026-04-24 because CloudflareWorkers::spawn() now starts wrangler dev itself. The bundle still has to be pre-built, but the message could clarify that wrangler is launched by the test:
#[ignore = "requires Docker, the `wrangler` CLI in $PATH, and a prebuilt Cloudflare Workers bundle (run build.sh first); the test starts `wrangler dev` automatically"]
Applies to both test_wordpress_cloudflare and test_nextjs_cloudflare.
| pub mod platform; | ||
|
|
||
| #[cfg(target_arch = "wasm32")] | ||
| use worker::*; |
There was a problem hiding this comment.
🔧 wrench — Wildcard import violates CLAUDE.md.
From CLAUDE.md → "What NOT to Do": Do not use wildcard imports (except use super::* in test modules). Bring in only what is actually used:
#[cfg(target_arch = "wasm32")]
use worker::{event, Context, Env, Request, Response, Result};| // Warn when the orchestrator submitted more than one provider: send_async() | ||
| // executes eagerly, so those requests already ran sequentially rather than | ||
| // in parallel. Total auction latency is sum(DSP_i) instead of max(DSP_i). | ||
| if pending_requests.len() >= 2 { |
There was a problem hiding this comment.
🔧 wrench — select() warning fires n - 1 times per multi-provider auction (still open from prior review).
The orchestrator calls select() in a loop, once per pending request (orchestrator.rs:381-388). The if pending_requests.len() >= 2 check is true for the first n - 1 calls of every auction, so a 5-DSP auction logs the same Use the Fastly adapter for parallel DSP fan-out. warning 4 times per request. At any reasonable QPS this drowns the logs.
Fix: move the warning to send_async so it fires once per launch, or count it once at the call site (app.rs) after building pending_requests.
| _store_name: &StoreName, | ||
| key: &str, | ||
| ) -> Result<Vec<u8>, Report<PlatformError>> { | ||
| match self.env.secret(key) { |
There was a problem hiding this comment.
🤔 thinking — Confirm that worker::Error::Display does not include the secret value before round-tripping.
attach(format!("secret lookup failed for key {key}: {err}")) includes the key name (low-risk) and the error chain. If the underlying JS error string ever contains the secret value (some bindings include the bound value in error messages), this would leak it through error-stack's Debug rendering into logs. Worth a quick audit — if confirmed safe, a comment in this file would be a useful note for the next reader.
| body, | ||
| params, | ||
| } => { | ||
| let mut output = Vec::new(); |
There was a problem hiding this comment.
🤔 thinking — Eager full-response buffering with no cap.
let mut output = Vec::new(); followed by stream_publisher_body writing the entire upstream response into RAM. A multi-MB origin response is held in WASM heap (Workers default 128 MB). Acknowledged in the doc comment, but worth a Vec::with_capacity(...) hint when Content-Length is known, plus a follow-up issue to stream through Workers' TransformStream for true zero-buffer pass-through.
|
|
||
| // POST /{*rest} — integration proxy or publisher origin fallback | ||
| let s = Arc::clone(&state); | ||
| let post_fallback = move |ctx: RequestContext| { |
There was a problem hiding this comment.
♻️ refactor — get_fallback and post_fallback are near-duplicates (still open from prior review).
Two ~30-line closures (this one and get_fallback above) differ only in whether the /static/tsjs= branch exists (GET only). Extract a dispatch(method, path, ...) helper and reuse:
async fn dispatch(
method: &Method,
path: &str,
state: &AppState,
services: &RuntimeServices,
req: Request,
allow_tsjs: bool,
) -> Result<Response, Report<TrustedServerError>> {
if allow_tsjs && path.starts_with("/static/tsjs=") {
return handle_tsjs_dynamic(&req, &state.registry);
}
if state.registry.has_route(method, path) { ... }
handle_publisher_request(...).await
.and_then(|pr| resolve_publisher_response(pr, &state.settings, &state.registry))
}The same pattern exists in the Axum adapter, so consolidating both around a shared helper would be even better.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn auth_middleware_rejects_with_401_when_credentials_required() { |
There was a problem hiding this comment.
♻️ refactor — auth_middleware_rejects_with_401_when_credentials_required doesn't actually test 401 (still open from prior review).
The test name and comment promise a 401 assertion, but the body comment admits it can't (basic_auth not configured in CI settings) and the only assertions are header present and status != 404. Two viable fixes:
- Construct a
Settingswithbasic_authpopulated (mirror whatapply_finalize_headerstests do inmiddleware.rs) andassert_eq!(resp.status(), 401). - Rename the test to reflect what it really verifies — e.g.
auth_middleware_runs_in_chain_for_protected_routes— and keep the existing assertions.
Either is fine; keeping the current name is misleading.
3 participants
Summary
Changes
Closes
Closes #498
Test plan
Checklist
Known deferred items