Optimize dev proxy upstream performance#896
Conversation
Replace the verbose `cargo run --manifest-path … --target … -- dev proxy` invocations with the installed `ts` binary (via `cargo install-cli`), so the guide reads the same way as the CLI guide. Note that `ts dev proxy` is macOS-only and phrase the install step as "install or update". Make the quick start a complete first run (`ca install`, then run with `--rewrite-host` and `--launch chrome`), and make `--rewrite-host` the recommended default for dev and staging upstreams. The Host-header guidance now turns on whether the upstream accepts `Host: <FROM>` rather than on the upstream being Trusted Server Compute. Correct several stale or wrong examples, verified against the CLI source: - Firefox `certutil` now uses `-d "sql:…"` and the full CA nickname, matching the auto-import the tool performs. - Add the missing `--connect-timeout` option and note that `[COMMAND]` is `ca`. - Prefer `--basic-auth-file` over inline `--basic-auth` in the staging example. - Clarify the CA key is stored outside the repository only by default, and rename "First run: CA setup" to "The development CA".
Keep `--rewrite-host` in the quick start (it is required for upstreams that reject `Host: <FROM>`), but state its trade-off honestly: against a real Trusted Server adapter, first-party URLs then render on the upstream host, not the production domain. Qualify the "first-party URLs always stay on the production domain" claim accordingly — it holds only when the upstream preserves `X-Forwarded-Host`. Also: - Note that Firefox CA auto-import needs `certutil` (`brew install nss`) and qualify the troubleshooting row that assumed it is always present. - State that `--launch` opens only the first mapped hostname while every mapping is still proxied. - Note that connection options (`--rewrite-host`, auth, `--insecure`, `--upstream-plaintext`) apply to every mapping, not per-rule. - Scope the Safari "only while the proxy runs" claim to a clean exit.
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Adds a bounded, reusable HTTP/1.1 upstream connection manager (plus DNS caching, precomputed upstream identities, and a repeatable benchmark suite) to ts dev proxy. The design is careful and thoroughly tested: the manager is an actor with a bounded ordinary lane + unbounded control lane so lifecycle events (cancel / driver-closed / shutdown) can never deadlock behind a saturated acquire lane, and capacity reconciliation is airtight — every path leads back to release(id) (reservation drop → ConnectFailed, driver-guard drop → DriverClosed, shutdown connector abort → finish error → reservation drop). The AcquireTicket single-terminal-transition invariant closes the resolve/cancel race. No blocking issues found; approving.
Non-blocking
♻️ refactor
- Crypto provider rebuilt per call:
NoVerifier::supported_verify_schemescallsaws_lc_rs::default_provider()(a non-trivial clone) on every invocation — cache in aOnceLock.--insecure-only path, cosmetic. (connect.rs:282)
🌱 seedling
- Non-idempotent 502 on stale reuse: the
send_requestretry is gated on provably-empty idempotent requests, so a POST racing an upstream idle-close returns 502 rather than replaying. This is the correct safe behavior; flagged only so it's a documented trade-off. (upstream/mod.rs:246)
⛏ nitpick
- Doc comments on new
pubitems: several new public items (ProxyMetrics::record_*,PooledResponseBody::new, etc.) lack doc comments. CLAUDE.md asks for one per public item;missing_docsisn't enforced here and it matches surrounding CLI style, so optional.
👍 praise
- Lifecycle & concurrency design: the
can_reusematrix + exhaustive test, DNS miss-coalescing that survives owner cancellation without poisoning the in-flight key, the trailer-frame deferral inPooledResponseBody, and thepriority_shutdown_overtakes_a_saturated_ordinary_lanetest are all excellent. - Security hardening: Basic-auth refused on non-loopback binds, hop-by-hop +
Connection-token stripping,Forwarded/Fastly-SSLstripped and authoritatively re-stamped, strict host validation before PAC / Host-header embedding, and redactedBasicAuthDebug.
CI Status
- fmt: PASS
- clippy (fastly/axum/cloudflare/spin native+wasm): PASS
- rust tests (fastly/axum/cloudflare/spin/CLI/parity): PASS
- js build + vitest + docs/ts prettier: PASS
| Ok(rustls::client::danger::HandshakeSignatureValid::assertion()) | ||
| } | ||
| fn supported_verify_schemes(&self) -> Vec<rustls::SignatureScheme> { | ||
| rustls::crypto::aws_lc_rs::default_provider() |
There was a problem hiding this comment.
♻️ refactor — aws_lc_rs::default_provider() builds a fresh CryptoProvider (cloning every cipher-suite/signature algorithm) on each call, and rustls can invoke supported_verify_schemes() per handshake. Consider caching the scheme list once:
fn supported_verify_schemes(&self) -> Vec<rustls::SignatureScheme> {
static SCHEMES: std::sync::OnceLock<Vec<rustls::SignatureScheme>> = std::sync::OnceLock::new();
SCHEMES
.get_or_init(|| {
rustls::crypto::aws_lc_rs::default_provider()
.signature_verification_algorithms
.supported_schemes()
})
.clone()
}Only reached in --insecure mode (loud-warned), so purely cosmetic — but on-theme for a PR about avoiding handshake cost.
| let response = loop { | ||
| match lease.connection.value.send_request(request).await { | ||
| Ok(response) => break response, | ||
| Err(_error) if lease.reused && !stale_retry && replay_template.is_some() => { |
There was a problem hiding this comment.
🌱 seedling — The stale-connection send_request retry is intentionally gated on replay_template.is_some() (provably-empty GET/HEAD/OPTIONS). A POST that races an upstream idle-close after ready() already passed therefore returns 502 with no replay. This is the correct safe choice — replaying a non-idempotent request could double-submit — noting it only so the trade-off is documented and deliberate rather than read as an oversight.
| } | ||
| } | ||
|
|
||
| fn can_reuse( |
There was a problem hiding this comment.
👍 praise — The can_reuse matrix (response complete AND upload complete AND no close-intent AND driver alive) is exactly the right set of conditions, and counting a complete-but-unpoolable response as a success independent of reuse is the correct semantics. The exhaustive test alongside it makes the invariant hard to regress.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Approved with seven actionable follow-ups noted inline. The pooled HTTP/1 architecture and coverage are strong; the highest-priority changes are preserving request trailers end-to-end and tying connector-capacity release to actual task termination.
| if let Ok(value) = HeaderValue::from_str(&outcome.host_header) { | ||
| headers.insert(hyper::header::HOST, value); | ||
| } | ||
| headers.insert(hyper::header::HOST, outcome.host_header.clone()); |
There was a problem hiding this comment.
🔧 P1 — Preserve request-trailer semantics on the upstream leg
rewrite_headers removes both Trailer and TE through strip_hop_by_hop without regenerating them. RequestUploadBody still forwards trailer frames, but Hyper's HTTP/1 encoder only serializes fields declared by the outgoing Trailer header; otherwise it discards them. A chunked upload can therefore arrive intact except for silently lost trailer fields, and removing TE: trailers prevents compliant origins from knowing the proxy accepts response trailers.
Capture the browser's trailer declarations and TE: trailers before sanitation, then regenerate safe upstream-leg Trailer and TE/Connection: TE metadata. Please add an E2E test proving request trailers reach the origin and a compliant origin can conditionally return response trailers.
| } | ||
| } | ||
|
|
||
| impl Drop for PendingConnection { |
There was a problem hiding this comment.
🔧 P1 — Release connector capacity only after cancellation completes
Dropping PendingConnection only calls AbortHandle::abort(), while the independently dropped Reservation immediately sends ConnectFailed; the manager can then release capacity before Tokio has actually dropped the DNS/TCP/TLS task. Replacement admission or shutdown acknowledgement can occur while the old connector remains live, violating the hard six-per-origin/64-global bound. The current test yields until abort.is_finished() before reacquiring, which masks this ordering.
Transfer reconciliation ownership to an exactly-once guard inside the connector task, and release the reservation only when that task actually exits (disarming the connector guard after successful driver registration). Add a test proving replacement admission and shutdown remain blocked until cancellation completion.
| connection.abort.abort(); | ||
| return; | ||
| } | ||
| if let Some(index) = self.waiters.iter().position(|waiter| { |
There was a problem hiding this comment.
🔧 P2 — Continue handoff after a waiter loses the cancellation race
A waiter can cancel after this position check but before ticket.resolve(). The actor then removes that waiter, stores the returned connection as idle, and never checks the next eligible same-origin waiter. With no further connection event, that waiter can time out after 30 seconds while a reusable connection sits idle for 60 seconds; newer requests may also take it first.
Make handoff loop over matching waiters until delivery succeeds or none remain. If the oneshot send fails, recover the lease payload and retry the next waiter. A deterministic two-waiter cancellation-race test would protect the FIFO invariant.
| let mut lease = loop { | ||
| let acquisition_started = tokio::time::Instant::now(); | ||
| let acquired = | ||
| acquire_for_mode(&self.manager, rule.origin_key().clone(), acquisition_mode) |
There was a problem hiding this comment.
🔧 P2 — Count setup and acquisition failures as failed requests
Acquisition errors here and connector errors at lines 209–212 return before record_request_failed; stale-replacement acquisition/connection has the same omission. forward_request converts these errors to visible 502 responses, but requests_failed remains zero, so the new diagnostics under-report common DNS/TCP/TLS/pool failures. Failed acquisition latency is also omitted.
Ensure every terminal UpstreamClient::send error records exactly one failure—preferably with a scoped outcome guard or centralized error boundary—and record acquisition duration on both success and failure. Add an unreachable-loopback-origin E2E assertion for 502, zero completions, and one failure.
| ) { | ||
| let result = resolve(resolver, &key, deadline, &metrics).await; | ||
| let shared = result.as_ref().map(Arc::clone).map_err(OwnedError::from_io); | ||
| let _ = signal.send(Some(shared.clone())); |
There was a problem hiding this comment.
🔧 P2 — Retain the completed lookup when all original receivers cancel
watch::Sender::send does not retain the value when there are no receivers. A new caller can subscribe to the still-present Loading entry between this failed send and cache-state replacement, then observe channel closure and return Interrupted even though the lookup completed.
| let _ = signal.send(Some(shared.clone())); | |
| signal.send_replace(Some(shared.clone())); |
Please add a multithreaded regression test where every initial waiter cancels and a late subscriber arrives during completion.
|
|
||
| #[tokio::test] | ||
| #[ignore = "manual performance workload"] | ||
| async fn perf_http1_remote_model() { |
There was a problem hiding this comment.
🔧 P2 — Make the named remote benchmark self-validating
This test labels output http1_remote_model_30_30_25, but the 30 ms connect and TLS delays are enabled only when TS_PERF_VARIANT starts with remote_. Under the default pooled variant, the named remote workload silently runs with only the 25 ms response delay, allowing mislabeled benchmark evidence.
Require/assert remote_baseline or remote_pooled for this test, or construct typed remote options directly. Please also document the exact alternating commands used for the recorded remote results.
| pub fn debug_summary(&self) -> String { | ||
| let snapshot = self.snapshot(); | ||
| format!( | ||
| "dev proxy metrics: browser_connections={} initial_heads_parsed={} initial_heads_rejected={} \ |
There was a problem hiding this comment.
🔧 P2 — Include the latency values advertised by the troubleshooting guide
The debug summary prints only sample counts for DNS, TLS, HTTP handshake, request-to-headers, and CA-mint timing. It emits latency buckets only for parsing, TCP connect, pool acquisition, and queue wait. Consequently the guide's claim that this summary can distinguish DNS and TLS latency is not currently true.
Include totals or histogram buckets for every advertised latency phase, or narrow the documentation claim to the phases actually emitted.
Closes #895
Summary
TCP_NODELAYexperiments that did not satisfy their retention gates.--resolve, Basic authentication, hop-by-hop sanitation, and blind-tunnel isolation.Final review corrections
Connection: closeresponses.Performance results
Full measurements and experiment decisions are recorded in
docs/superpowers/implementation-notes/2026-07-10-dev-proxy-performance.md.Verification
cargo fmt --all -- --checkcargo check --testsand Clippy with-D warnings./scripts/test-cli.sh— 147 unit + 26 E2E testsproxy_perfworkloads with one test thread