Quantize auction transport timeouts to stabilize Fastly backend names#865
Quantize auction transport timeouts to stabilize Fastly backend names#865prk-Jr wants to merge 7 commits into
Conversation
Fastly dynamic backend names embed the first-byte and between-bytes timeouts so a registration can never be silently reused with a different transport configuration. Deriving those timeouts from the remaining wall-clock auction budget minted a new backend name on nearly every request, defeating cross-request TCP/TLS connection reuse (Fastly pools connections per backend name) and accumulating registrations toward the per-service dynamic backend limit. Compute the effective transport timeout from the configured provider timeout verbatim when the budget allows, floor the budget-bound value to 250ms buckets otherwise, and pass sub-quantum remainders through exactly so publishers with sub-250ms configured budgets keep launching. The quantized value feeds both the backend name and the registered configuration, so they cannot diverge. Rounding down never extends a transport cap past the auction deadline, which the mediator and dispatched-collect paths rely on to bound the </body> hold. Also add a Fastly platform test pinning predict_name == ensure for the same spec, since the orchestrator maps responses back to providers by predicted backend name. Fixes #847
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed the timeout quantization change against the PR's actual main base. The arithmetic is internally consistent, but the current implementation has correctness and operational risks that should be addressed before merge. Detailed findings are attached inline.
Resolve the PR review by making transport-timeout canonicalization a platform capability and hardening auction backend-name correlation. - Move quantization behind PlatformBackend::canonicalize_transport_timeout_ms. Fastly floors budget-derived timeouts to a 250ms quantum with a bounded sub-quantum ladder [200,150,100,50]; other adapters use the exact remaining budget so bidder deadlines (Prebid tmax, APS timeout) are not shortened where no connection-pooling benefit exists. - Bound sub-quantum backend-name cardinality: exact 1-249ms values no longer pass through, capping the budget-derived names a single origin can mint toward the per-service dynamic backend limit. - Add a provider discriminator to PlatformBackendSpec, folded into every adapter's backend name, so two providers sharing one origin no longer collide on the response-correlation key. Reject a duplicate backend_to_provider insertion with an attributed launch failure instead of silently overwriting and misattributing a response. - Make the orchestrator call-site tests deterministic: record predicted and registered transport timeouts separately and assert exact equality via a controllable platform backend, and enumerate the sub-quantum ladder to assert a bounded name cardinality. - Correct the timeout-semantics comments that overstated absolute-deadline enforcement; the Fastly connect/first-byte/between-bytes timeouts bound connection, first-byte, and inactivity, not total response time. A true absolute deadline carried through the platform HTTP API remains follow-up work (#849).
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed current HEAD 5512d850 against main. The Fastly-only canonicalization and provider-discriminator wiring are directionally correct, but two high-risk backend identity/cardinality issues and two medium reliability/compatibility issues remain; details are inline.
Documentation
The PR description no longer describes HEAD: it still claims exact sub-quantum passthrough and a two-file change, while HEAD uses a [200, 150, 100, 50] ladder, skips values below 50 ms, adds a platform trait capability and discriminator, and changes 15 files. Please refresh the Summary, Changes table, and test plan.
Address the auction transport-timeout review findings: - Derive dynamic backend names from a bounded readable prefix plus a SHA-256 digest of the complete backend spec, so distinct specs never alias to one name. Name equality now implies spec equality, making NameInUse reuse provably safe, and the name is bounded to Fastly's 255-char limit regardless of host or discriminator length. - Bound budget-derived transport-timeout cardinality with a globally finite ladder: keep the 250ms quantum through 2000ms, then snap larger budgets to a small fixed set of coarse rungs so a large configured ceiling can no longer mint hundreds of dynamic backends. - Reject duplicate configured providers at startup, and check the predicted backend name before request_bids fires the outbound send, retaining the post-launch check as defense against a provider that resolves to an unexpected name.
…sport-timeouts # Conflicts: # crates/trusted-server-adapter-fastly/src/backend.rs # crates/trusted-server-core/src/auction/orchestrator.rs
The backend name now carries a SHA-256 digest suffix, so the three platform predict_name tests that pinned the exact/ending name string no longer hold. Assert the readable body via starts_with/contains instead. These were missed earlier because the local run filtered the platform suite too narrowly; CI aborted on the first panic.
aram356
left a comment
There was a problem hiding this comment.
Summary
Reviewed HEAD 77f3521 against main. All nine findings from the two earlier review rounds are verified fixed on this HEAD: the digest-based backend naming makes name equality imply spec equality (and I confirmed the only Backend::builder call in the Fastly adapter flows through it), the discriminator keeps same-origin providers on distinct names on all four adapters, cardinality is globally bounded by the sub-quantum and coarse ladders, duplicate providers are rejected at startup with a pre-launch guard on both paths, canonicalization is Fastly-only via the platform trait, and the timeout wiring tests are deterministic. Remaining items are one documentation blocker and a handful of non-blocking suggestions, inline.
Blocking
🔧 wrench
- Stale PR description: the description still describes the pre-review design — "exact passthrough for sub-quantum remainders so publishers with sub-250ms configured budgets keep launching" was removed in 5512d85 (budget-derived sub-quantum values now snap to the
[200, 150, 100, 50]ladder; only configured sub-250ms constants pass through). It also predates most of what the PR now contains: digest-based backend naming, provider discriminators, the coarse ladder, duplicate-provider startup rejection, and the platform-capability split. The changes table still places the quantization helpers inorchestrator.rs, but they live in the Fastly adapter now. Please rewrite the Summary/Changes to describe HEAD.
Non-blocking
⛏ nitpick
ensure()doc drift: the doc comment still says the name is "derived from the scheme, host, port, certificate setting,first_byte_timeout, andbetween_bytes_timeout" — no digest, no discriminator, no Host override (crates/trusted-server-adapter-fastly/src/backend.rs:328-334).compute_namecarries the accurate description; this paragraph should defer to it instead of restating a stale subset.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests (fastly/axum/cloudflare/spin + parity + CLI): PASS
- js tests: PASS
- integration/browser tests: PASS
- Reject a mediator that is also listed in [auction].providers at startup; the overlap would fire the same provider twice per auction and its demand already flows through the mediation response - Assert in both cardinality enumeration tests that canonical values never exceed the remaining budget (the mediator </body> hold bound invariant) - Document the accepted worst-case rounding haircut on the coarse ladder - Write the spec digest hex into one pre-sized buffer instead of allocating per byte, and defer the ensure() doc to compute_name
Summary
PlatformBackend::canonicalize_transport_timeout_msdefaults to the exact budget-bound value (remaining.min(configured)) because adapters that neither register nor name-key backends gain nothing from rounding and must not shorten bidder deadlines. Only the Fastly adapter overrides it with quantization.[200, 150, 100, 50]ladder (below the smallest rung the value canonicalizes to 0 and callers skip the launch as budget-exhausted); (3) at or above the ceiling, values floor to a fixed 8-rung coarse ladder (2,000 … 60,000ms, clamping above the top rung) so a large configured ceiling cannot mint hundreds of buckets. Rounding down never extends a transport cap past the remaining budget — the mediator and dispatched-collect</body>hold bounds rely on this, and the exhaustive enumeration tests assert it for every reachable input.backend_<readable>_<digest>where the digest is the first 128 bits of a SHA-256 over an unambiguous encoding of the complete spec (scheme, host, port, certificate setting, Host override, provider discriminator, and both transport timeouts). This makesNameInUsereuse provably safe and prevents first-registration-wins poisoning where a later request with a tighter timeout would inherit an earlier registration's value. The readable prefix is a lossy, bounded slug carried only for logs, and the whole name is bounded to Fastly's 255-char limit.[auction].providersand a mediator that is also listed as a provider (both would fire duplicate outbound requests per auction; a mediator's own demand already flows through its mediation response). The parallel and dispatch launch paths additionally skip a duplicate predicted backend name before sending, as defense in depth.predict_name == ensureis pinned with a Viceroy test, since the orchestrator maps SSP responses back to providers by predicted backend name.Changes
crates/trusted-server-adapter-fastly/src/backend.rspredict_nameconvenience wrappercrates/trusted-server-adapter-fastly/src/platform.rscanonicalize_transport_timeout_ms; quantum, sub-quantum ladder, and coarse ladder constants with rationale (including the accepted worst-case rounding haircut); exhaustive cardinality + budget-bound tests;predict_name == ensureand discriminator testscrates/trusted-server-core/src/platform/traits.rs,types.rs,test_support.rscanonicalize_transport_timeout_mstrait method with exact-passthrough default;PlatformBackendSpec.discriminatorfieldcrates/trusted-server-core/src/auction/orchestrator.rscrates/trusted-server-core/src/integrations/mod.rscrates/trusted-server-adapter-{axum,cloudflare,spin}/src/platform.rsCloses
Closes #847
Test plan
cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spincargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo fmt --all -- --checkcargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity./scripts/test-cli.shcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-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 ...")logmacros (notprintln!)