fix: enable TCP keepalive on default httpx transports to prevent NAT idle-timeout drops (#3269)#3503
fix: enable TCP keepalive on default httpx transports to prevent NAT idle-timeout drops (#3269)#3503zaid646 wants to merge 4 commits into
Conversation
…idle-timeout drops (openai#3269) Non-streaming API calls (completions.create, Responses API with medium/high reasoning) hang indefinitely behind NAT gateways because the default httpx transport has no TCP keepalive. Server-side generation routinely takes 300-700s, exceeding NAT idle timeouts (~120-350s), and the dead connection is never detected. This adds SO_KEEPALIVE with TCP_KEEPIDLE=60s, TCP_KEEPINTVL=60s, and TCP_KEEPCNT=5 on both _DefaultHttpxClient and _DefaultAsyncHttpxClient, using runtime detection of the socket_options parameter (httpx>=0.25.0) to avoid bumping the dependency floor. Custom transports supplied via http_client= are never overridden.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44c7df4cfc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if "transport" not in kwargs: | ||
| kwargs["transport"] = _build_keepalive_transport(httpx.HTTPTransport, kwargs) |
There was a problem hiding this comment.
For users relying on HTTP_PROXY/HTTPS_PROXY with the default trust_env=True, this unconditional injected transport prevents httpx from loading environment proxy mounts because httpx only enables env proxy discovery when transport is None. As a result DefaultHttpxClient()/OpenAI() bypasses required corporate proxies or fails to connect, and the existing test_proxy_environment_variables expectation that _mounts contains the env proxy will no longer hold; the async default transport path has the same issue.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Note
To use Codex here, create a Codex account and connect to github.
Explicitly passing a transport to httpx bypasses env proxy detection (HTTP_PROXY/HTTPS_PROXY/ALL_PROXY). This adds a proxy-var gate that skips keepalive transport injection when proxy env vars are detected and trust_env hasn't been explicitly disabled.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ad65fe086
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ) | ||
|
|
||
| def test_default_transport_has_tcp_keepalive(self) -> None: | ||
| client = OpenAI(base_url=base_url, api_key=api_key) |
There was a problem hiding this comment.
This test depends on the runner having no HTTP_PROXY/HTTPS_PROXY/ALL_PROXY variables. In an environment where any of those are already set, _should_inject_keepalive_transport() deliberately skips injecting the transport, so socket_options will not include SO_KEEPALIVE and the assertion below fails; the async copy has the same issue. Clear the proxy env vars in these keepalive-positive tests or instantiate with trust_env=False.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d219cc78fa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| httpx.HTTPTransport.__init__ | ||
| ).parameters | ||
|
|
||
| _TRANSPORT_PASSTHROUGH_KEYS = ("verify", "cert", "trust_env", "http1", "http2", "limits") |
There was a problem hiding this comment.
When callers use the documented DefaultHttpxClient(proxy=...) path, _should_inject_keepalive_transport still adds a direct transport, but proxy is not forwarded here and remains in kwargs; httpx routes those requests through a separate proxy transport that it builds from the proxy kwarg, so the injected transport with socket_options is bypassed and proxied calls still have no TCP keepalive. This leaves the idle-timeout hang unfixed for explicit proxy users; construct the proxy transport with the same socket options or handle the proxy kwarg consistently.
Useful? React with 👍 / 👎.
|
@codex review |
1 participant
Fixes #3269
Non-streaming API calls hang indefinitely behind NAT gateways because the default httpx transport has no TCP keepalive. Server-side generation routinely takes 300-700s, exceeding NAT idle timeouts (~120-350s).
Changes: