Skip to content

http-client: make proxy route fallback explicit#31916

Open
bolinfest wants to merge 1 commit into
mainfrom
pr31916
Open

http-client: make proxy route fallback explicit#31916
bolinfest wants to merge 1 commit into
mainfrom
pr31916

Conversation

@bolinfest

@bolinfest bolinfest commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Why

When OS proxy resolution is enabled, an unavailable platform resolver must not silently fall back to reqwest's ambient behavior. That would create a second routing decision and make respect_system_proxy difficult to reason about. Route resolution is also on the request hot path, so cached PAC decisions should not queue behind unrelated lookups or retain signed URLs in process-global state.

What changed

  • Represent environment fallback as an explicit proxy route carrying its captured NO_PROXY policy, or as an explicit direct route when no proxy is configured.
  • Preserve Tungstenite's WSS fallback order (HTTPS_PROXY, then HTTP_PROXY, then ALL_PROXY) when system lookup is unavailable.
  • Consult the URL-keyed system-proxy cache before taking the global macOS/Windows resolver permit, and hash cache keys on every platform.
  • Avoid spawn_blocking on platforms where system resolution is already synchronous and nonblocking.
  • Delegate environment-derived WebSocket routes to Tungstenite's environment-aware dialer so its full NO_PROXY behavior remains intact.

Review guide

  1. Review http-client/src/outbound_proxy.rs for the explicit route state machine, cache ordering, and unavailable-resolver fallback.
  2. Review websocket-client/src/dialer.rs for the environment-route handoff.
  3. The next PR, http-client: add safe route-aware request pool #31917, adds request-level pooling on top of these concrete routes.

Validation

  • Proxy fallback, WSS fallback, hashed-cache, and cached-resolution regression tests in outbound_proxy_tests.rs.
  • A subprocess WebSocket test proves matching NO_PROXY hosts bypass the proxy while nonmatching hosts use it.

Stack created with Sapling. Best reviewed with ReviewStack.

@chatgpt-codex-connector

Copy link
Copy Markdown
Contributor

💡 Codex Review

http = { workspace = true }

P1 Badge Update Bazel lockfile after adding Rust deps

This adds a new direct dependency (and a dev-dependency below), but the commit does not update MODULE.bazel.lock. The repo's Bazel lock check runs bazel mod deps --lockfile_mode=error, so changing Cargo.toml without the generated lockfile will fail CI until just bazel-lock-update is run and the lockfile committed.


"https" => {
proxy_env_value(env, "HTTPS_PROXY").or_else(|| proxy_env_value(env, "ALL_PROXY"))
}

P2 Badge Keep HTTP_PROXY fallback for WSS routes

When system proxy resolution is unavailable, this branch now resolves wss:// destinations as https and only checks HTTPS_PROXY/ALL_PROXY. The previous TransportDefault path delegated to the bundled tokio-tungstenite resolver, which falls back from HTTPS_PROXY to HTTP_PROXY for TLS WebSockets, so users who set only HTTP_PROXY for an HTTP CONNECT proxy will now connect directly and fail in proxy-only networks. Add the same HTTP_PROXY fallback for the WebSocket/HTTPS case or keep delegating to the transport for that case.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant