Add trusted-server-adapter-axum native dev server (PR 16)#643
Open
prk-Jr wants to merge 48 commits into
Open
Conversation
Move trusted-server-adapter-axum from workspace exclude to members list. Remove the global `target = "wasm32-wasip1"` build override from .cargo/config.toml (which forced the axum crate out of the workspace) and pass --target wasm32-wasip1 explicitly only for Fastly CI commands. Delete the now-redundant crate-local .cargo/config.toml. Update CI test-rust job to exclude the axum crate and pass the explicit target; test-axum job runs from the workspace root with -p flag. Add .edgezero/ to .gitignore to exclude the local KV store file.
Register AxumDevServer alongside FastlyViceroy in RUNTIME_ENVIRONMENTS so the full framework x runtime scenario matrix (WordPress, Next.js) runs against both platforms. AxumDevServer spawns the native trusted-server-axum binary (no WASM or Viceroy), binds to the fixed port 8787 (baked into axum.toml at compile time), and polls for any HTTP response as readiness (root returns 403 in test env). Binary path defaults to target/debug/trusted-server-axum, overridable via AXUM_BINARY_PATH. Settings are baked in at build time via TRUSTED_SERVER__* env vars, same as Fastly. The integration-tests.sh script now builds both the WASM and the native Axum binary with test-specific overrides (origin=127.0.0.1:8888). Add test_wordpress_axum and test_nextjs_axum individual test functions. Ignore .edgezero/ at workspace root (local KV store from the dev server).
- README: update Quick Start and Development commands for both runtimes - getting-started: add Axum as Option A (no Fastly CLI needed) - architecture: add trusted-server-adapter-axum to Core Components; add Runtime Targets table - testing: fix cargo test commands; add Axum adapter section; split Local Server Testing into Axum vs Fastly; restore clippy step in CI/CD workflow example alongside new test-axum job
The integration test matrix includes AxumDevServer which requires the native trusted-server-axum binary. Add build-axum step to the shared setup action, package the binary alongside the WASM artifact, and pass AXUM_BINARY_PATH to the integration test run step.
prk-Jr
requested review from
ChristianPavilonis and
aram356
and removed request for
aram356
aram356
requested changes
Apr 20, 2026
aram356
left a comment
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Introduces a native Axum dev-server adapter that runs the full trusted-server pipeline locally without Fastly Compute or Viceroy, promotes the crate to a workspace member, and extends the integration-test matrix to cover both runtimes. Requesting changes for a handful of small but concrete issues: unused deps, stale lockfile, doc/code drift, and a middleware bypass on the startup-error path.
Blocking
🔧 wrench
- Unused direct dependencies:
serde_jsonandtrusted-server-jsdeclared incrates/trusted-server-adapter-axum/Cargo.tomlbut never referenced insrc/ortests/. See inline. - Stale crate-local
Cargo.lock: ~100 KB file that cargo now silently ignores (crate is a workspace member). Delete it to avoid lockfile drift. CLAUDE.mdcomment contradicts the PR: says "excluded from workspace" at line 14 while the PR makes it a workspace member. See inline.- Log text references a non-existent
NoopKvStore: the code usesUnavailableKvStore(src/platform.rs:367vs :379). See inline. - Startup-error router bypasses middleware:
startup_error_router()has noFinalizeResponseMiddleware/AuthMiddleware, breaking the "every response has X-Geo-Info-Available" invariant and bypassing operator headers + basic auth on startup-failure responses. See inline onsrc/app.rs:134.
Non-blocking
🤔 thinking
send_async/selectdiverges from Fastly (eager execution changes error-surface timing and ordering). Trade-off is documented, but consider a breadcrumb log or realtokio::spawnfan-out. See inline.Body::Streamoutbound body silently truncated to empty on axum. See inline.- Env-var namespace collisions possible due to
-/./_normalization. See inline. - Fixed port 8787 baked at compile time can TIME_WAIT-flake across sequential integration tests. See inline.
♻️ refactor
reqwest::Clientrebuilt per request — defeats connection pool. Move to sharedArc<AxumPlatformHttpClient>inAppState. See inline.- Env-var tests not isolated — use
temp-env(already a workspace dep). See inline. bytes::Bytes→Vec<u8>round-trip on request/response bodies is wasted allocation. See inline.tests/routes.rsuses.unwrap()instead of.expect("should ...")— CLAUDE.md applies to test code. See inline.
🌱 seedling
- No
/healthendpoint —AxumDevServer::health_check_path()returns/healthbut the app never registers it; tests work around withwait_for_any_response. See inline.
⛏ nitpick
- Crate-local
.gitignoreis redundant with the workspace.gitignore. See inline. build_per_request_servicesis a no-op wrapper aroundbuild_runtime_services. See inline.
📝 note
test-axumCI job runs withoutTRUSTED_SERVER__*env vars — every request goes through the startup-error path. Smoke tests still pass, but not a great signal. See inline.
CI Status
- fmt: PASS (verified locally,
cargo fmt --all -- --check) - clippy (axum crate, host target): PASS with zero warnings
cargo test -p trusted-server-adapter-axum: 18 tests PASS- GitHub CI on
75fe0d01: prepare artifacts + integration tests + browser tests all PASS
… bypass, refactors
Blocking:
- Remove unused serde_json and trusted-server-js from Cargo.toml
- Delete crate-local Cargo.lock (now silently ignored as workspace member)
- Fix CLAUDE.md workspace layout comment (drop "excluded from workspace")
- Fix log message naming NoopKvStore → UnavailableKvStore in build_runtime_services
- Wrap startup_error_router with FinalizeResponseMiddleware(Settings::default()) so
startup-error responses carry X-Geo-Info-Available and operator response_headers
Refactor:
- Move AxumPlatformHttpClient to AppState; share Arc across requests via
build_runtime_services(ctx, Arc::clone(&state.http_client)) to preserve
the reqwest connection pool
- Remove build_per_request_services no-op wrapper; inline at call sites
- Use temp_env::with_var in config/secret store tests for proper isolation
- Replace .unwrap() with .expect("should ...") throughout test code per CLAUDE.md
Nitpick:
- Delete redundant crate-local .gitignore (covered by workspace .gitignore)
…ce breadcrumbs Port fix: - main.rs reads PORT env var at startup; when set, uses AxumDevServer::with_config with a dynamic SocketAddr instead of the run_app default (hardcoded 8787) - Integration test spawner (axum.rs) now calls find_available_port() and passes PORT=<port> to the child process, matching the Fastly env's dynamic-port pattern - Fallback to AXUM_DEFAULT_PORT=8787 if find_available_port fails (offline runner) Divergence breadcrumbs: - send_async: debug log noting that execution is eager and errors surface immediately, not at select() time as they do on Fastly - select: debug log noting that index 0 is popped unconditionally (sequential, not first-to-complete) — any fan-out ordering tests should use the Fastly runtime
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Nice addition overall — the Axum adapter is headed in a useful direction. I found a few correctness gaps in the platform shim plus a couple of documentation mismatches around what the Axum runtime can currently support.
Fix EdgeZero HTTPS scheme detection regression: capture tls_protocol and tls_cipher from the raw FastlyRequest before dispatch_with_config_handle consumes it, inject as trusted internal headers x-ts-tls-protocol / x-ts-tls-cipher (added to INTERNAL_HEADERS), and read them in build_per_request_services to populate ClientInfo. This restores parity with the legacy path where RequestInfo::from_request detects HTTPS via ClientInfo TLS fields rather than falling back to the default "http". Skip the from_fastly_response -> to_fastly_response round-trip for middleware-finalized responses in edgezero_main. Checking x-ts-finalized on the FastlyResponse directly and calling send_to_client() without the intermediate take_body_bytes() call avoids re-materializing the full response body in WASM heap on the normal registered-route path. Update integration-guide.md and pull_request_template.md to use the target-specific cargo aliases (cargo test-fastly / cargo test-axum, cargo clippy-fastly / cargo clippy-axum) instead of the removed --workspace variants.
aram356
requested changes
May 28, 2026
aram356
left a comment
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Re-review of PR16 (Axum dev-server adapter) at head a6e1073b. Most prior findings from earlier reviews are resolved; CI is green. This re-review finds one new blocking security issue introduced by the most recent commit (TLS-header trust contract not enforced) plus several non-blocking concerns.
Note: PR base is
feature/edgezero-pr15-remove-fastly-core, notmain. PR15 must merge first; ordering should be confirmed before merge of this PR.
Blocking
🔧 wrench
- Spoofable
x-ts-tls-protocol/x-ts-tls-cipheron the EdgeZero entry path — see inline at crates/trusted-server-adapter-fastly/src/main.rs:178. The trust contract documented at L173-176 is not enforced; the spoofable-headers strip list does not includex-ts-tls-*, and the trusted-sourceset_headeris conditional on the Fastly SDK returningSome. Consequence:detect_request_schemecan be spoofed tohttpsover plain HTTP, affecting cookieSecure, URL rewriting, and any scheme-gated handler.
Non-blocking
🤔 thinking
reqwest::Clientrebuilt per request regresses a previously-resolved finding — see inline at crates/trusted-server-adapter-axum/src/platform.rs:503.health_check_path()returns/healthbut no route is registered — see inline at crates/integration-tests/tests/environments/axum.rs:70.
♻️ refactor
- Stateless platform shims allocated
Arc::new(...)per request — see inline at platform.rs:497. - Two divergent code paths in
main.rs::main()— see inline at main.rs:16.
⛏ nitpick
- Logger init failure silently swallowed in
main()— see inline at main.rs:9. PORTparse error falls back instead of failing fast — see inline at main.rs:43.
🌱 seedling
- Operator-header validation is per-request — see inline at middleware.rs:105.
📌 out of scope
- Consolidate known dev-server limitations in
crates/trusted-server-adapter-axum/src/lib.rs. The PR author's responses to prior reviews acknowledge several intentional Axum-vs-Fastly divergences (eagersend_async, sequentialselectordering, truncatedBody::Streamoutbound, no KV/geo, env-var key collision surface). Today they're scattered acrossplatform.rs,app.rs, andmiddleware.rsdoc comments — a single "Known limitations" section inlib.rswould make them discoverable. Worth a follow-up issue rather than this PR.
CI Status
- fmt: PASS
- clippy (fastly + axum): PASS
- rust tests (Fastly via Viceroy + Axum native): PASS
- integration tests: PASS
- browser integration tests: PASS
All 3 PR checks green at head a6e1073b.
Blocking: - Strip x-ts-tls-protocol and x-ts-tls-cipher from the inbound request before conditionally re-setting them from the Fastly SDK in edgezero_main; without this, a plain-HTTP client injecting X-TS-TLS-Protocol spoofs the scheme detection path, affecting cookie Secure and URL rewriting - Add regression test in app.rs asserting tls_protocol/tls_cipher are None when the trusted headers are absent after the strip+set logic Non-blocking: - Register GET /health -> 200 ok on TrustedServerApp so health_check_path() is satisfied and the shared wait_for_ready helper can be used - Promote stateless shims (AxumPlatformConfigStore, AxumPlatformSecretStore, UnavailableKvStore, AxumPlatformBackend, AxumPlatformGeo) to OnceLock statics in build_runtime_services so Arc::clone is used per request instead of allocating a fresh Arc each time - Unify the two divergent main() branches into a single code path using AxumDevServer::with_config; log logger init failure with eprintln! instead of silently swallowing it - Exit non-zero on PORT parse error instead of falling back silently to axum.toml default — the fallback surprises tooling expecting the server at the explicitly requested port
Collaborator
Author
|
All round-2 findings resolved in commit 746993c. Blocking — fixed
Non-blocking — addressed
Deferred
CI: |
Conflicts resolved: - app.rs (2 doc conflicts): PR16 wins on the module-level and build_per_request_services docs — they correctly describe the x-ts-tls-* header injection mechanism added in this PR - app.rs (test import): merge both — PR16's build_per_request_services and PR15's build_state_from_settings both needed in the test module - main.rs (edgezero_main finalization): PR15 wins — take_finalize_sentinel + inlined resolve_geo_for_response + apply_finalize_headers replaces the older apply_entry_point_finalize approach - main.rs (function): PR15 wins — take_finalize_sentinel function replaces apply_entry_point_finalize - main.rs (BoundedWriter + resolve_publisher_response_buffered): PR15 wins — add these structures needed for the EdgeZero buffered publisher path - main.rs (test): PR15 wins — add take_finalize_sentinel_strips_sentinel test - publisher.rs (doc): PR16 wins — buffer_publisher_response description is correct for that function; PR15 doc was misattributed to stream_publisher_body
The merge conflict resolution took PR15's take_finalize_sentinel code expecting an HttpResponse but left the fastly_response variable unconverted. Add the missing let mut response = compat::from_fastly_response(fastly_response) before the sentinel check.
- Remove BoundedWriter and resolve_publisher_response_buffered from main.rs; they were copied in by the merge but belong in app.rs for the EdgeZero path; the legacy path uses stream_publisher_body directly - Remove the now-unused std::io::Write import - Fix let mut fastly_response to let fastly_response (no longer mutated)
aram356
requested changes
Jun 3, 2026
aram356
left a comment
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Round-4 re-review of the Axum dev-server adapter, scoped to the PR's own diff against its base feature/edgezero-pr15-remove-fastly-core. All blocking findings from the previous three rounds are resolved (deps, lockfile, middleware bypass, fan-out semantics, Body::Stream buffering, env-var rename, fmt/clippy failures, CLAUDE.md/AGENTS.md/agent-doc drift, wasm clippy coverage, fixed-port flakiness, /health registration, handler boilerplate). Verified locally: cargo fmt --all -- --check passes, clippy -p trusted-server-adapter-axum is clean, and the 19 axum tests pass.
Requesting changes for one regression introduced by this PR: the removal of the publisher buffered-body size cap.
Blocking
🔧 wrench
- Unbounded publisher buffer / dead
max_buffered_body_bytes: deletingresolve_publisher_response_buffered(and itsBoundedWriter) in favor of the new unboundedbuffer_publisher_responsedropssettings.publisher.max_buffered_body_bytesenforcement on the Fastly EdgeZero path. The setting is still defined, defaulted to 16 MiB, and documented as preventing Wasm-heap OOM, but is now read nowhere. See inline oncrates/trusted-server-core/src/publisher.rs:418.
Non-blocking
📌 out of scope
- Per-request
reqwest::Client(crates/trusted-server-adapter-axum/src/platform.rs:521): the no-pooling revert is deliberate, but its comment cites a Next.js "poisoned connection after a truncated POST." The empty-body branch (if !body_bytes.is_empty()) skippingContent-Length: 0is a likely latent cause worth a follow-up issue rather than masking with per-request clients.
📝 note
- Weak happy-path coverage: the
test-axumCI job runs withoutTRUSTED_SERVER__*env vars andtests/routes.rsonly asserts non-404/non-5xx, so requests flow throughstartup_error_routerand 200/302 paths are never exercised. Pre-existing; deferred.
⛏ nitpick
Cargo.toml:83:reqwestadded out of alphabetical order (sits beforeregex).crates/trusted-server-adapter-axum/Cargo.toml:33-35:edgezero-adapter-axum,edgezero-core, andreqwestare re-declared in[dev-dependencies]though already normal dependencies with no added features — redundant.
CI Status (verified locally on macOS host)
- fmt: PASS (
cargo fmt --all -- --check) - clippy (axum, native): PASS (zero warnings)
cargo test -p trusted-server-adapter-axum: PASS (19 tests)- GitHub CI (integration tests, browser integration tests): PASS
- Note:
Run Tests/Run Formatworkflows only trigger on PRs tomain, so fmt/clippy/test-fastly/test-axum have not run on this PR's feature-branch target; they were verified locally instead.
ChristianPavilonis
requested changes
Jun 3, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Automated review by Yesman.
I found two blocking issues that should be fixed before merge: the Axum async select path can mislabel remaining backend requests after the first completion, and the shared publisher buffering helper drops the configured max-buffer guard that protected the Fastly EdgeZero path from unbounded buffering. Inline comments include details.
Pick up PR15 commits since the first merge (cedea8b — remove stale fastly::Body reference from platform/mod.rs docs). Conflicts resolved: - Cargo.lock: auto-resolved, take union - integration-tests/tests/integration.rs: keep both Axum tests (PR16) and EC lifecycle test (PR15) - trusted-server-adapter-fastly/src/main.rs: keep PR16 TLS strip/inject headers AND PR15 into_core_request + oneshot dispatch Post-merge compilation fixes for trusted-server-adapter-axum: - app.rs: update handle_proxy call to use ProxyDispatchInput struct (PR15 bundled 5 args into struct to stay within 7-arg cap) - app.rs: update handle_auction call with new kv/registry/ec_context args added in PR15 (pass None/default for Axum dev server) - platform.rs: add failed_backend_name: None to PlatformSelectResult initializer (new field added in PR15)
The PR15 merge reintroduced fastly into trusted-server-core through ec/kv.rs, ec/rate_limiter.rs, and an orphaned backend.rs, breaking native linking for the Axum dev server. - Delete orphaned core backend.rs (adapter already owns its copy) - Keep the RateLimiter trait in core; move FastlyRateLimiter and RATE_COUNTER_NAME to the Fastly adapter - Add the EcKvStore primitive trait (lookup with generation marker, conditional insert, prefix count, delete) in ec/kv_backend.rs - Rewrite KvIdentityGraph on top of EcKvStore so CAS retry loops, tombstone semantics, and validation stay in core - Implement FastlyEcKvStore in the adapter mapping onto the Fastly KV Store API - Add in-memory and failing test backends; cover create/revive, upsert, and tombstone paths natively
get_settings() now rejects the placeholder secrets baked into trusted-server.toml (hardened in PR14/15) instead of only logging warnings, so adapter tests that loaded baked settings began failing. - Build test settings from explicit TOML with non-placeholder values in the Axum middleware tests and the Fastly app tests - Add TrustedServerApp::routes_with_settings() so the Axum route integration tests drive the real router without the baked settings - Accept the designed 501 Not Implemented for admin key routes on the dev server; only an unhandled 500 fails the test
- Restore the publisher buffered-body size cap: reinstate BoundedWriter (now in core so both adapters share it) and enforce settings.publisher.max_buffered_body_bytes in buffer_publisher_response, with unit tests covering the cap - Fix backend-name misattribution in AxumPlatformHttpClient::select: futures::future::select_all uses swap_remove and makes no ordering guarantee for remaining futures, so positional index reconstruction could pair later auction responses with the wrong bidder. Each AxumPendingHandle now implements Future and resolves to its own backend name - Reorder reqwest after regex in workspace Cargo.toml - Drop redundant dev-dependency re-declarations (edgezero-adapter-axum, edgezero-core, reqwest) in the Axum adapter manifest
Removing the fastly dependency from trusted-server-core dropped its transitive wasm dependencies from the integration-tests dependency graph, leaving the crate's standalone Cargo.lock stale and failing the --locked check in check-integration-dependency-versions.sh.
The 'Verify Fastly WASM release build' step builds for wasm32-wasip1 but the job's toolchain setup never installed that target, failing with: can't find crate for core.
aram356
requested changes
Jun 12, 2026
aram356
left a comment
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Re-review of the Axum native dev-server adapter against its base (feature/edgezero-pr15-remove-fastly-core). Most prior blocking items are now genuinely fixed (verified): the select_all success-path backend-name misattribution, the unbounded publisher buffer / dead max_buffered_body_bytes cap, TLS-header spoofing strip, /health route, PORT exit-on-error, the clippy split covering both targets, and the SYNTHETIC__SECRET_KEY → EDGE_COOKIE__SECRET_KEY rename. The new trait extractions (EcKvStore, RateLimiter) that remove fastly from core are well-executed and faithful to the original semantics.
Requesting changes for two items: a correctness regression on the auction error path (the counterpart of the select_all fix was left incomplete) and an incomplete migration of the workspace-command aliases into the live dev-tooling files.
Blocking
🔧 wrench
-
Axum auction: failed providers silently vanish —
crates/trusted-server-adapter-axum/src/platform.rs:484returnsfailed_backend_name: Noneon the error path, so a failed provider is never attributed and noBidStatus::Erroris recorded (it disappears via the orchestrator's "backend not identified" branch). The Fastly adapter returnsSome(failed_name). Cross-platform parity regression. See inline. -
Live dev-tooling commands broken by the global-target removal — removing
[build] target = wasm32-wasip1plus making axum a workspace member means barecargo {build,test,clippy} --workspacenow compiles the wasm-onlyfastlycrate for the host and fails.CLAUDE.md,AGENTS.md,README.md, and three agent docs were migrated to the*-fastly/*-axumaliases, but four live tooling files were missed (same root cause):.claude/commands/check-ci.md:4-5— the/check-cicommand (cargo clippy/test --workspace).claude/commands/verify.md:3,6,7— the/verifycommand (cargo build/clippy/test --workspace).claude/commands/test-all.md:4— the/test-allcommand (cargo test --workspace).claude/agents/build-validator.md:14,26(cargo build/clippy --workspace)
A contributor running
/check-cior/verifynow hits the exact failureAGENTS.mdwarns against. Update each tocargo test-fastly && cargo test-axum/cargo clippy-fastly && cargo clippy-axum.
Non-blocking
🤔 thinking
- Dropped
remainingauction handles detach, not abort —platform.rs:453; dev-only, up to 30s of wasted background work per dropped bidder. See inline. - Unbounded upstream response buffering —
platform.rs:348(andsend_asyncat ~413); no cap, no log on the response side. See inline. - Redundant double error-wrapping in KV paths —
ec/kv.rs:663and:762re-wrap with the same message the backend already attached. See inline.
♻️ refactor
- Dead
ifover discarded bool —proxy.rs:370;apply_image_passthrough_metadatareturns aboolboth call sites ignore. See inline.
🌱 seedling
- CAS-conflict retry branches untested —
ec/kv.rs:331; no test injects a generation conflict, so the retry / concurrent-revive / exhaustion paths are uncovered. See inline. - Per-request
reqwest::Clientkeeps the pool disabled —platform.rs:280; documented Next.js-regression workaround, follow-up issue acknowledged. See inline.
⛏ nitpick
- Bare version strings for dev-deps —
crates/trusted-server-adapter-axum/Cargo.toml:31,34(axum/tower) vs the workspace-pinning convention. See inline. - Weak route-test assertions / no auction fan-out test —
tests/routes.rs:118. See inline.
📝 note
- Misplaced WASM-verify step —
.github/workflows/test.yml:77; lives intest-axum, only compiles becauserust-toolchain.tomlpins the target. See inline. - CI never exercised fmt/clippy/test on this PR —
test.ymlandformat.ymlare gatedpull_request: branches: [main]; this PR targets a feature branch, so onlyintegration-tests.ymlran (the green checks). The fmt/clippy/test-fastly/test-axum gates will run for the first time on retarget tomain. CLAUDE.mdbarecargo checkunder Testing & Quality now fails at the workspace root; scope it with-por an alias.
CI Status
- integration tests / browser tests / prepare artifacts: PASS (on
4a2e1134) - fmt / clippy / test-fastly / test-axum: not run on this PR (workflows gated to PRs targeting
main); clippy-split config verified correct locally
Reconciles PR15's EC parity and review fixes with PR16's trait extractions, preferring the PR16 implementation where both sides changed the same code: - maybe_identity_graph / require_identity_graph keep PR16's KvIdentityGraph::new(FastlyEcKvStore::new(...)) construction (now pub(crate) for app.rs), and app.rs EC dispatch uses the core buffer_publisher_response instead of re-adding the adapter-local BoundedWriter copy that PR16 had already moved into core - core buffer_publisher_response adapted to the plain usize max_buffered_body_bytes from PR15 (the unbounded None arm is unrepresentable) - EcFinalizeState carries a use_finalize_kv flag instead of the KvIdentityGraph itself: the graph now wraps a non-Sync dyn EcKvStore and cannot ride in response extensions, so edgezero_main rebuilds it from settings when the flag is set - FastlyRateLimiter import moved to the adapter-local module per PR16's rate-limiter extraction - Test helpers merged: PR16's TLS-header tests and minimal_state plus PR15's test_router and absolute-URI empty_request - integration-tests lock keeps PR16's post-fastly-removal graph; shared dependency versions verified with the check script
Blocking:
- Attribute failed auction providers on the Axum select error path:
failed_backend_name now carries the backend whose request failed,
matching the Fastly adapter, so the orchestrator removes the provider
and records BidStatus::Error instead of dropping it through the
"backend not identified" branch. Adds a select-level regression test
(request to a closed port) asserting the attribution
- Migrate the remaining live dev-tooling files to the per-target
aliases: /check-ci, /verify, /test-all, and the build-validator agent
now use cargo {build,clippy,test}-{fastly,axum}. New build-* and
check-* aliases added to .cargo/config.toml; CLAUDE.md's bare
cargo build / cargo check examples updated to the aliases
Non-blocking:
- Abort dropped auction tasks: AxumPendingHandle aborts its JoinHandle
on drop so deadline-dropped bidders stop instead of running up to the
30s transport timeout in the background
- Log buffered upstream response sizes on both the send and send_async
paths so a large upstream is visible in dev instead of silently
growing the heap
- Drop the redundant double error-wrapping in count_hash_prefix_keys
and delete — the backend already attaches store context
- apply_image_passthrough_metadata returns () instead of a bool both
call sites discarded
- axum and tower dev-dependencies moved to workspace pins
- Admin route tests assert the fixed 501 contract instead of != 404,
and CAS-conflict paths are now covered: a conflict-injecting EcKvStore
test backend exercises the retry-then-succeed, concurrent-revive
short-circuit, and retry-exhaustion arms of create_or_revive
Collaborator
Author
|
Re the review-summary items without inline threads (all in f7e8846 unless noted):
|
ChristianPavilonis
approved these changes
Jun 12, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
I completed the review and found one low-severity gap (P3, behavior parity) plus previously validated no P0/P1/P2 blockers.
- [P3] (named_routes): routes are not mirrored in Axum (, , and related canonical variants). These routes currently route into generic fallback behavior rather than the handler logic used on Fastly.
Recommendation: consider adding canonical route entries in Axum’s route table (e.g., as aliases with same handlers), so production parity remains consistent and consumers hitting documented API paths get expected responses even in dev/runtime testing mode.
ChristianPavilonis
approved these changes
Jun 12, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
I completed the review and found one low-severity gap (P3, behavior parity) plus previously validated no P0/P1/P2 blockers.
- [P3] crates/trusted-server-adapter-axum/src/app.rs (named_routes):
_tsroutes are not mirrored in Axum (/_ts/admin/keys/*,/_ts/api/v1/identify,/_ts/api/v1/batch-syncand related canonical variants). These routes currently route into generic fallback behavior rather than the handler logic used on Fastly.
Recommendation: add _ts canonical route entries in Axum’s route table (as aliases to existing handlers where behavior is shared), so production-like API paths remain consistent in Axum test/runtime mode.
aram356
approved these changes
Jun 15, 2026
aram356
left a comment
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Round-6 re-review of the Axum native dev-server adapter. Every blocking and non-blocking finding from my previous review (f7e88462, "round-5") is fixed and independently verified — I read each diff and ran the new core CAS tests locally (4 passed). Approving.
Verified fixes:
- 🔧 Auction
failed_backend_nameis now attributed on the error path (backend_namecloned into the success.map,ready.as_ref().err().map(...)for the failure), with a newselect_attributes_failed_backend_nameregression test — the exact guard that was missing. - 🔧
/check-ci,/verify,/test-all, andbuild-validatormigrated to the*-fastly/*-axumaliases; newcheck-*/build-*aliases added;CLAUDE.mdcargo checkscoped. AxumPendingHandlenowDrops withhandle.abort(); both buffering pathslog::debug!the upstream size; KVdelete/countno longer double-wrap;apply_image_passthrough_metadatareturns();axum/towermoved to workspace deps; admin route tests assert== 501;test-axumtoolchain declareswasm32-wasip1.- New
ConflictInjectingEcKv+ three tests cover the CAS retry / concurrent-revive / exhaustion branches.
A fresh full-diff pass found no blocking issues. The Fastly KV → EcKvStore mapping is faithful and fastly is genuinely removed from core. A few minor, non-blocking items below.
Non-blocking
⛏ nitpick (inline)
- Byte-slice panic risk on non-ASCII prefix —
crates/trusted-server-adapter-fastly/src/ec_kv.rs:122. [workspace.dependencies]ordering —Cargo.toml:87.
📝 note / observation (adjacent to, or just outside, this PR's diff)
- Consent-store fail-closed comment is inaccurate —
crates/trusted-server-adapter-fastly/src/app.rs:171(runtime_services_for_consent_route). The 503-on-misconfigured-consent-store is a reasonable hardening, but the doc comment claims it "matches the legacyroute_requestbehavior" — legacy (main.rs:417) buildsruntime_serviceswithUnavailableKvStoreand never opens the named consent store, so it never fails closed. This is a new divergence on the EdgeZero path (gated off by default), not parity. Suggest rewording to "intentional hardening over legacy" and confirming the 503 is intended. (This sits on a context line just above the changed hunk, so it's body-level rather than inline.) get_metadatatransfers the full entry body —crates/trusted-server-core/src/ec/kv.rs:217.lookup()always callstake_body_bytes()(ec_kv.rs:67), so a metadata-only read downloads the body to discard it. The method has no production callers today, so this is a latent inefficiency rather than a regression — a dedicatedlookup_metadataprimitive would avoid it if it ever lands on a hot path.- Duplicated
DEFAULT_FIRST_BYTE_TIMEOUT = 15s— defined independently incrates/trusted-server-adapter-fastly/src/backend.rs:37andcrates/trusted-server-core/src/platform/mod.rs:57(the former is not touched by this PR). Consider exposing the core const and reusing it in the adapter to avoid drift.
CI Status
- integration tests / browser tests / prepare integration artifacts: PASS (on
f7e88462) - new core CAS-conflict tests: PASS locally (
cargo test -p trusted-server-core ec::kv, 4 tests) - fmt / clippy / test-fastly / test-axum: still gated to PRs targeting
main; clippy-split + new aliases verified correct locally
| store_name: self.store_name.clone(), | ||
| message: format!( | ||
| "Failed to list keys with prefix '{}'", | ||
| &prefix[..prefix.len().min(8)], |
Collaborator
There was a problem hiding this comment.
⛏ nitpick — &prefix[..prefix.len().min(8)] slices by byte index; a non-ASCII prefix whose 8th byte falls mid-codepoint would panic. Safe today (callers pass hex EC-hash prefixes), but the trait method takes an arbitrary &str. Prefer prefix.get(..8).unwrap_or(prefix) or prefix.chars().take(8).collect::<String>().
| regex = "1.12.3" | ||
| reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] } | ||
| serde = { version = "1.0", features = ["derive"] } | ||
| simple_logger = "5" |
Collaborator
There was a problem hiding this comment.
⛏ nitpick — simple_logger is inserted between serde and serde_json, breaking the otherwise-alphabetical ordering of [workspace.dependencies]. Move it after serde_json for consistency.
* Replace std::time::Instant with web_time::Instant in auction orchestrator
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.
* Add trusted-server-adapter-cloudflare crate with host-target smoke tests
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.
* Add CI job for Cloudflare adapter and update CLAUDE.md
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.
* Fix Cloudflare entry point: use worker 0.7 and pass manifest_src to run_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.
* Add CloudflareHttpClient, build.sh, wrangler DX, and review fixes
- 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
* Wire Cloudflare platform stores via edgezero handles
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
* Add Cloudflare Workers to CI integration tests
- 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
* Resolve PR review findings in Cloudflare adapter
- 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
* Exclude Cloudflare adapter from wasm32-wasip1 test job
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.
* Resolve PR review findings
* Resolve PR review findings
* Add alias for clouflare tests
* Fix lint error
* Resolve PR review findings
Blocking:
- Fix cargo fmt failure in integration-tests cloudflare.rs (long import, struct init)
- Replace `use worker::*` with explicit imports in adapter lib.rs
- Return generic 500 body from top-level dispatch error; log detail server-side
- Fix workerd process-group leak: spawn wrangler as group leader, killpg on drop
- Use find_available_port() for wrangler dev instead of hardcoded 8787
- Reject multi-provider fan-out in select() with PlatformError::HttpClient
instead of a noisy warn; per-provider timeout is now enforced by failing
loudly rather than silently degrading to sum(latencies)
- Fix clippy::type_complexity in axum platform.rs with ResponseTriplet alias
- Fix docs prettier formatting
Non-blocking:
- Return Err from execute() on Body::Stream outbound instead of silent empty body
- Assert unreachable! on Body::Stream in send_async (execute always returns Once)
- Extract shared dispatch() helper from get_fallback/post_fallback duplicates
- Rename auth test to auth_middleware_runs_in_chain_for_protected_routes
- Update stale #[ignore] reasons for Cloudflare integration tests
* Fix cargo clippy fastly
* Resolve PR 644 round-4 review findings
Blocking:
- Extract reject_multi_provider_fanout(len) as a target-agnostic free function
gated #[cfg(any(target_arch = \"wasm32\", test))]; add 4 unit tests covering
len=0 (pass), len=1 (pass), len=2 (reject), len=5 (reject with count in msg)
- Inline-confirm Secret::to_string() returns the raw JsValue string with no
wrapping — verified against worker-rs src/env.rs Display impl
Non-blocking:
- from_utf8_lossy -> std::str::from_utf8 with a hard error on non-UTF-8
outbound header values; lossy conversion silently mutated bytes
- to_ascii_lowercase() -> eq_ignore_ascii_case() for content-encoding /
transfer-encoding header checks — no allocation, same semantics
- Remove to_ascii_uppercase() on Method::from; worker 0.7 uppercases internally
- Strip Transfer-Encoding before setting Content-Length in buffered publisher
response (defense-in-depth; Workers likely strips it anyway)
- build.sh: gate cargo install with command -v to skip reinstall when warm
- wrangler.toml: add comment above placeholder KV ID explaining local vs remote
- wrangler.toml: add comment explaining compatibility_date and when to bump it
* Fix Method::from — worker 0.7 implements From<String> not From<&str>
The prior commit used Method::from(method.as_str()) based on reviewer note
that From<&str> is case-insensitive, but From<&str> is not implemented at all
in worker 0.7 — only From<String>. http::Method::to_string() already returns
uppercase so the to_ascii_uppercase() allocation is removed without changing
semantics.
* Add compile_error! for cloudflare feature on non-wasm32 targets
Enables the cloudflare feature on a native target pulls worker which
requires wasm-bindgen and produces cryptic linker errors. The guard
catches it immediately with a clear message pointing to the correct
--target wasm32-unknown-unknown flag.
* Collapse 13 handler closures into make_handler factory in app.rs
Extract make_handler<F, Fut>(state, f) -> impl Fn(RequestContext) -> BoxedHandlerFuture
that owns the build_per_request_services + ctx.into_request() boilerplate.
The routing table now reads as a flat list of (METHOD, PATH, handler-expr) triples
instead of 10 named handler variables each spanning 7-9 lines. ~120 lines removed.
* Address Cloudflare adapter review findings and fix axum CI wasm target
CI:
- Install the wasm32-wasip1 target in the test-axum job — the
'Verify Fastly WASM release build' step added by PR16 builds for
that target but the toolchain setup never installed it
Blocking findings:
- Strip stale Content-Length from decoded Workers fetch responses and
set it from the decoded body length. The origin value describes the
compressed payload while the Workers runtime auto-decompresses, so
pass-through responses could be sent with a truncating length
- Reject multi-provider auction fan-out before any request launches:
add PlatformHttpClient::supports_concurrent_fanout() (default true),
report false from CloudflareHttpClient whose send_async executes
eagerly, and validate in the orchestrator before the launch loop.
Previously the select()-time rejection fired only after every
provider request had already run sequentially and spent the auction
budget. The select()-time check remains as defense-in-depth.
Regression test asserts rejection happens with zero requests sent
Non-blocking findings:
- Outbound request headers use Headers::append instead of set so
duplicate header names forward every value, matching the response path
- Replace the unreachable! panic in send_async with a typed
PlatformError so an edgezero behavior change cannot panic a Worker
- Replace bare unwrap() with expect("should ...") in platform tests
per the testing conventions
* Extend per-target build/check aliases to the Cloudflare adapter
The build-fastly/check-fastly aliases merged from PR16 predate the
Cloudflare adapter and did not exclude it, so they compiled the
wasm32-unknown-unknown crate for wasm32-wasip1. Add the exclusion
(matching test-fastly/clippy-fastly), add a build-cloudflare alias
mirroring check-cloudflare, and update every dev-tooling doc that lists
the per-target commands: /check-ci, /verify, /test-all, the
build-validator agent, CLAUDE.md, AGENTS.md, and README.md now include
the cloudflare clippy/test/build/check steps alongside fastly and axum.
All aliases verified locally (check-fastly, check-axum,
check-cloudflare, build-cloudflare).
* Resolve PR17 round-5 review findings
- EC current_timestamp(): use web_time::SystemTime instead of std::time so
the EC path does not panic on wasm32-unknown-unknown (Cloudflare Workers),
matching the rest of core (proxy, orchestrator, signing).
- Cloudflare router: proxy the publisher fallback for all 7 methods
(GET/POST/HEAD/OPTIONS/PUT/PATCH/DELETE) via publisher_fallback_methods(),
mirroring the Axum/Fastly adapters; tsjs stays GET-only. Applies to both
the main and startup-error routers.
- build.sh: export .env.cloudflare.dev overrides with set -a so worker-build
sees them.
- rust-toolchain.toml: install wasm32-unknown-unknown so documented
cargo build-cloudflare works on a clean checkout.
- CI: run cargo clippy-cloudflare in the format workflow.
- ec_kv.rs: use prefix.get(..8).unwrap_or(prefix) for the error message so a non-ASCII prefix whose 8th byte falls mid-codepoint cannot panic the slice. - Cargo.toml: move simple_logger after serde_json to restore alphabetical ordering of [workspace.dependencies]. - app.rs: correct the consent-route doc comment — the fail-closed 503 is intentional hardening over legacy route_request (which uses UnavailableKvStore and never opens the consent store), not behavioral parity.
Brings PR15's main-merge progress (multi-backend asset proxy, S3 SigV4 signing, image optimizer primitive, DataDome server-side, settings/docs) up into the PR16 axum dev-server branch. Conflict resolutions: - fastly/app.rs: keep captured tls_protocol/tls_cipher (PR16) plus ..ClientInfo::default() for PR15's new device-signal fields; single publisher import incl. buffer_publisher_response + BoundedWriter. - fastly/main.rs: keep both the trusted x-ts-tls-* header injection (PR16) and derive_device_signals (PR15); drop the now-orphaned fastly-local resolve_publisher_response_buffered in favor of core buffer_publisher_response. - core/proxy.rs: keep PR16's origin_response_metadata/ apply_image_passthrough_metadata helper refactor (superset of PR15's inline image/pixel logic) and restore PR15 #754 IMAGE_FALLBACK_CONTENT_TYPE; union imports; web_time for EC clock, std SystemTime only at the chrono-bound s3 sign_headers call. - core/publisher.rs: dedupe the doubly-merged BoundedWriter, keeping the pub version the adapters import. - core/platform/test_support.rs: merge both StubHttpClient field sets (concurrent_fanout + image-optimizer/stream/method/uri stubs). - core/Cargo.toml: restore the wasm32-unknown-unknown uuid/getrandom js block the auto-merge dropped (required for the Cloudflare build). - axum/cloudflare platform.rs: ..ClientInfo::default() and PlatformBackendSpec.host_header_override for the new fields. - migration_guards.rs: ec/kv.rs and ec/rate_limiter.rs are now fully Fastly-free (required for the wasm32-unknown-unknown core build), so move them into checked_sources and drop the obsolete deferred-EC allowlist. - docs/getting-started.md: template version vars (PR15 #760) + Fastly-optional framing (PR16 axum dev server).
The PR15→PR16 merge restored the wasm32-unknown-unknown getrandom/uuid `js` feature block in trusted-server-core, which adds getrandom (with js-sys and wasm-bindgen) to core's dependency graph. The integration-tests crate is a separate workspace with its own lockfile that did not yet record this, so the --locked integration build failed. Add the missing entries only (no version churn); the shared getrandom 0.2.17 entry now matches the root lockfile.
…tly-core' into feature/edgezero-pr16-axum-dev-server # Conflicts: # crates/trusted-server-adapter-fastly/src/app.rs # crates/trusted-server-adapter-fastly/src/main.rs # crates/trusted-server-core/Cargo.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
trusted-server-adapter-axumas a native (non-WASM) dev server so the full trusted-server pipeline can be run and tested locally without Fastly Compute or Viceroytarget = "wasm32-wasip1"override from.cargo/config.toml; Fastly-specific commands now pass--target wasm32-wasip1explicitlyChanges
crates/trusted-server-adapter-axum/src/platform.rsPlatformConfigStore,PlatformSecretStore,PlatformBackend,PlatformGeo,PlatformHttpClient— env-var-backed implementationscrates/trusted-server-adapter-axum/src/middleware.rsFinalizeResponseMiddleware+AuthMiddleware— mirrors Fastly adapter, always emitsX-Geo-Info-Available: falsecrates/trusted-server-adapter-axum/src/app.rsTrustedServerAppimplementingHookswith all 11 routes wiredcrates/trusted-server-adapter-axum/src/main.rs+axum.tomlcrates/trusted-server-adapter-axum/tests/routes.rsEdgeZeroAxumService(no live TCP server)crates/integration-tests/tests/environments/axum.rsAxumDevServerruntime environment added to the matrixcrates/integration-tests/tests/environments/mod.rsAxumDevServeralongsideFastlyViceroycrates/integration-tests/tests/integration.rstest_wordpress_axum+test_nextjs_axumindividual test functionsscripts/integration-tests.sh.cargo/config.tomltarget = "wasm32-wasip1"; keep only the viceroy runnerCargo.tomltrusted-server-adapter-axumfrom[exclude]to[members]crates/trusted-server-adapter-axum/Cargo.toml.github/workflows/test.ymltest-axumCI job;test-rustnow passes--target wasm32-wasip1explicitlyCLAUDE.md.gitignore(root + adapter).edgezero/(local KV store created by dev server)Closes
Closes #497
Test plan
cargo test --workspace(Fastly/WASM crates via Viceroy)cargo test -p trusted-server-adapter-axum(8 route + middleware tests)cargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest run(282 tests)test_wordpress_fastly,test_nextjs_fastly,test_wordpress_axum,test_nextjs_axumall passcargo run -p trusted-server-adapter-axumstarts on port 8787Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)