Skip to content

model-provider: route model discovery through HTTP client factory#31361

Merged
bolinfest merged 1 commit into
mainfrom
pr31361
Jul 9, 2026
Merged

model-provider: route model discovery through HTTP client factory#31361
bolinfest merged 1 commit into
mainfrom
pr31361

Conversation

@bolinfest

@bolinfest bolinfest commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Why

Model discovery is a startup-critical request path, but /models still constructed a default reqwest client directly. As a result, features.respect_system_proxy could cover Responses traffic while model catalog refreshes bypassed the same OS proxy policy.

The proxy policy must come from the Config for the operation that actually triggers model discovery. Thread start and resume can apply configuration overrides, so capturing an HttpClientFactory when the process-wide models manager is constructed is not sufficient.

What changed

  • Require an HttpClientFactory on model-manager operations that may fetch the remote catalog, and propagate the current request/session config through every call site.
  • Construct the exact /models?client_version=... URL once and use it both to resolve the outbound route and to execute the request.
  • Resolve synchronous PAC/WinHTTP APIs on Tokio's blocking pool, with a bounded client-build permit and single-flight cache misses.
  • Include proxy resolution and the HTTP request in the existing five-second models refresh timeout.
  • Keep static model catalogs functionally unchanged; they accept the mandatory factory but do not perform network I/O.
  • Add focused tests for request-time policy propagation, exact-URL transport selection, and the single-flight resolver invariant.

Review guide

  1. models-manager/src/manager.rs makes the factory a request-time dependency and forwards it to ModelsEndpointClient.
  2. model-provider/src/models_endpoint.rs builds the exact models URL, selects the route-aware transport, and applies one timeout to resolution plus request execution.
  3. login/src/auth/default_client.rs moves the synchronous client build off the async runtime; http-client/src/outbound_proxy.rs makes cache misses single-flight.
  4. The core, app-server, and CLI edits are mechanical propagation from the Config that owns each operation.

Validation

  • cargo check --tests -p codex-http-client -p codex-login -p codex-api -p codex-models-manager -p codex-model-provider -p codex-core -p codex-app-server
  • just test -p codex-http-client -p codex-api -p codex-models-manager -p codex-model-provider
  • just test -p codex-app-server models_refresh_worker

Follow-up

Other direct reqwest consumers are migrated in the subsequent PRs in this stack.


Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest changed the base branch from main to pr31342 July 7, 2026 04:48
@bolinfest bolinfest force-pushed the pr31361 branch 2 times, most recently from 11a2bda to 1c63ae4 Compare July 7, 2026 17:35
@bolinfest bolinfest changed the base branch from pr31342 to pr31442 July 7, 2026 17:35
bolinfest added a commit that referenced this pull request Jul 7, 2026
## Why

#31335 lets HTTP callers obtain proxy-aware clients from
`HttpClientFactory`, but a non-HTTP transport such as WebSockets also
needs two pieces of policy owned by `codex-http-client`: a concrete
route decision for its destination and the same custom-CA-aware rustls
trust configuration used by HTTPS.

Keeping these prerequisites in the shared abstraction means the
dependent Responses WebSocket change (#31441) cannot independently
reinterpret `features.respect_system_proxy`, PAC results, or enterprise
CA settings.

## What changed

- Add a redaction-safe `OutboundProxyRoute` with explicit
transport-default, direct, and concrete-proxy outcomes.
- Add `HttpClientFactory::resolve_proxy_route()` so transports can
resolve a destination through the already-selected outbound proxy
policy.
- Resolve `ws://` and `wss://` URLs through their HTTP equivalents so
system and PAC rules apply consistently.
- Add an always-returned rustls config builder that starts from native
roots and layers in any configured Codex custom CA bundle. The existing
optional builder remains available to callers that can delegate the
default configuration to their transport.
- Continue redacting proxy URLs from `Debug` output because they may
contain credentials.

## Review guide

1. `http-client/src/outbound_proxy.rs` defines the transport-neutral
route result and WebSocket URL normalization.
2. `http-client/src/custom_ca.rs` factors the native-root/custom-CA
construction so callers that perform TLS themselves can always obtain a
config.
3. `http-client/src/outbound_proxy_tests.rs` verifies WebSocket
normalization and legacy transport-default behavior.

## Test plan

- `just test -p codex-http-client outbound_proxy`
- `just test -p codex-http-client custom_ca`

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31342).
* #31431
* #31363
* #31362
* #31361
* #31442
* #31441
* __->__ #31342
@bolinfest bolinfest changed the base branch from pr31442 to pr31441 July 8, 2026 20:12
@bolinfest bolinfest requested a review from viyatb-oai July 8, 2026 20:19
@bolinfest bolinfest marked this pull request as ready for review July 8, 2026 20:19
@bolinfest bolinfest requested a review from a team as a code owner July 8, 2026 20:19
bolinfest added a commit that referenced this pull request Jul 8, 2026
## Why

Responses WebSockets are the normal lower-latency transport for
WebSocket-capable providers. They must not bypass an OS-selected proxy
when `features.respect_system_proxy` is enabled, but disabling
WebSockets whenever the feature is enabled would impose a substantial
performance penalty.

Merged PR #31622 introduced the reusable proxy-aware WebSocket
transport. This PR makes the Responses API its first consumer so the
existing fast path uses the same effective proxy and trust policy as
HTTP.

## What changed

- Register `codex-websocket-client` as a workspace dependency and use it
from `codex-api`.
- Feed the shared crate’s route-independent `WebSocketConnection` into
the existing Responses message pump.
- Require a configured `HttpClientFactory` for normal Responses
WebSocket connections and the CLI doctor probe, so neither path can open
a connection without consulting the effective proxy policy.
- Pass the session factory from `core` and the effective configuration
factory from `doctor`.
- Add an end-to-end Responses test that enables `RespectSystemProxy`,
asserts the resolved policy, completes a turn over WebSocket, and
verifies the connection and request counts.
- Keep the existing Responses protocol handling, ping/pong pump, and
session-scoped HTTP fallback unchanged.

The DNS, proxy, TLS, custom-CA, and Happy Eyeballs implementation and
its transport tests live in merged PR #31622. This PR deliberately
contains only the Responses integration and does not duplicate that
transport code.

## Review guide

1. `codex-rs/codex-api/src/endpoint/responses_websocket.rs` constructs
the shared connector and adapts its uniform stream to the existing pump.
2. `codex-rs/core/src/client.rs` supplies the session-scoped factory for
production Responses connections.
3. `codex-rs/cli/src/doctor.rs` supplies the effective configuration
factory to the handshake probe.
4. `codex-rs/core/tests/suite/client_websockets.rs` covers the
enabled-feature path end to end.

## Test plan

- `cargo check --tests -p codex-api -p codex-core -p codex-cli`
- `just test -p codex-api`
- `just test -p codex-core
responses_websocket_streams_with_system_proxy_feature`
- `cargo shear`
- `just bazel-lock-check`


---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31441).
* #31637
* #31431
* #31363
* #31362
* #31361
* __->__ #31441
Base automatically changed from pr31441 to main July 8, 2026 21:06
@bolinfest bolinfest requested a review from anp-oai July 8, 2026 21:46
Comment thread codex-rs/core/src/thread_manager.rs Outdated
Comment thread codex-rs/model-provider/src/models_endpoint.rs Outdated
Comment thread codex-rs/model-provider/src/models_endpoint.rs Outdated
Comment thread codex-rs/model-provider/src/models_endpoint.rs Outdated

@viyatb-oai viyatb-oai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One cancellation-safety issue remains in the bounded PAC client build.

Comment on lines +277 to +286
let _permit = ROUTE_AWARE_CLIENT_BUILD_PERMIT
.acquire()
.await
.map_err(std::io::Error::other)?;
tokio::task::spawn_blocking(move || {
build_default_reqwest_client_for_route(&http_client_factory, &request_url, route_class)
.map_err(std::io::Error::from)
})
.await
.map_err(std::io::Error::other)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the permit into the blocking closure? The outer five-second timeout cancels this future and drops _permit, but Tokio keeps an already-started spawn_blocking task running. A later refresh can then acquire the permit and enqueue another PAC/WinHTTP lookup, so repeated timeouts defeat the intended bound.

Suggested change
let _permit = ROUTE_AWARE_CLIENT_BUILD_PERMIT
.acquire()
.await
.map_err(std::io::Error::other)?;
tokio::task::spawn_blocking(move || {
build_default_reqwest_client_for_route(&http_client_factory, &request_url, route_class)
.map_err(std::io::Error::from)
})
.await
.map_err(std::io::Error::other)?
let permit = ROUTE_AWARE_CLIENT_BUILD_PERMIT
.acquire()
.await
.map_err(std::io::Error::other)?;
tokio::task::spawn_blocking(move || {
let _permit = permit;
build_default_reqwest_client_for_route(&http_client_factory, &request_url, route_class)
.map_err(std::io::Error::from)
})
.await
.map_err(std::io::Error::other)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 130fc9e36209. The acquired semaphore permit is now moved into the spawn_blocking closure, so cancelling or timing out the outer future cannot release the permit while the already-started PAC/WinHTTP lookup is still running.

@bolinfest bolinfest requested a review from viyatb-oai July 8, 2026 23:08
Comment thread codex-rs/http-client/src/outbound_proxy.rs Outdated
Comment thread codex-rs/model-provider/src/provider.rs Outdated
Comment thread codex-rs/models-manager/src/manager_tests.rs Outdated
@bolinfest bolinfest enabled auto-merge (squash) July 9, 2026 01:58
@bolinfest bolinfest merged commit 5892c7b into main Jul 9, 2026
107 of 131 checks passed
@bolinfest bolinfest deleted the pr31361 branch July 9, 2026 02:32
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants