fix(python-sdk): enable TCP keepalive on API and envd HTTP transports - #1608
fix(python-sdk): enable TCP keepalive on API and envd HTTP transports#1608wj-e2b wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 361015d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
PR SummaryMedium Risk Overview New keepalive sync/async transports apply those socket options on direct connects and wrap httpcore’s network backend so proxy connections still get keepalive (httpcore 1.0.x otherwise drops
Adds an explicit Reviewed by Cursor Bugbot for commit 361015d. Bugbot is set up for automated code reviews on this repo. Configure here. |
Package ArtifactsBuilt from c53af43. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.36.1-wj-keepalive.0.tgzCLI ( npm install ./e2b-cli-2.15.2-wj-keepalive.0.tgzPython SDK ( pip install ./e2b-2.35.0+wj.keepalive-py3-none-any.whl |
![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: 3b1bd9b553
ℹ️ 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".
| import logging | ||
| import os | ||
| import re | ||
| import socket |
There was a problem hiding this comment.
This commit changes packages/python-sdk, but the diff does not add or update any .changeset/*.md entry, so the release tooling has no patch note/version bump for the transport fix and consumers may not receive it in the next package release. Please add a changeset for the Python SDK change.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether this Python-only change violates the CLAUDE.md JS/Python SDK parity rule — the JS SDK's HTTP layer (undici/fetch-based, in packages/js-sdk/src/api/http2.ts) has no equivalent socket-level transport abstraction to mirror this httpx/httpcore-specific keepalive plumbing into, so this isn't a parity gap.
Extended reasoning...
Checked packages/js-sdk/src/api for an equivalent HTTP transport layer. The JS SDK uses fetch/undici rather than a pluggable httpx-style transport with socket_options, so there's no direct place to apply the same TCP keepalive socket options — the Python-only scope here is a platform-API mismatch, not a parity violation of the CLAUDE.md JS/Python sync rule.
| pool._network_backend = _TCPKeepaliveAsyncNetworkBackend(pool._network_backend) | ||
|
|
||
|
|
||
| class _TCPKeepaliveTransport(httpx.BaseTransport, httpx.AsyncBaseTransport): | ||
| """Dual sync/async transport for directly constructed public clients.""" | ||
|
|
||
| def __init__(self, **kwargs): | ||
| self._sync_transport = _TCPKeepaliveHTTPTransport(**kwargs) | ||
| self._async_transport = _TCPKeepaliveAsyncHTTPTransport(**kwargs) | ||
|
|
||
| def handle_request(self, request: httpx.Request) -> httpx.Response: | ||
| return self._sync_transport.handle_request(request) | ||
|
|
||
| async def handle_async_request(self, request: httpx.Request) -> httpx.Response: | ||
| return await self._async_transport.handle_async_request(request) | ||
|
|
||
| def close(self) -> None: | ||
| self._sync_transport.close() | ||
|
|
||
| async def aclose(self) -> None: | ||
| await self._async_transport.aclose() | ||
|
|
||
|
|
||
| # Any transport type produced by the factory passed to | ||
| # _build_env_proxy_mounts; keeps the returned mount dict precisely typed for | ||
| # both sync and async httpx clients. | ||
| _TransportT = TypeVar("_TransportT", bound=Union[BaseTransport, AsyncBaseTransport]) |
There was a problem hiding this comment.
🟡 This PR adds TCP keepalive to the API and envd transports in e2b/api/__init__.py, but the near-identical TransportWithLogger/AsyncTransportWithLogger classes in e2b/volume/client_sync/__init__.py and e2b/volume/client_async/__init__.py still build plain httpx.HTTPTransport/httpx.AsyncHTTPTransport with no socket_options. Since Volume Content API connections carry file upload/download traffic — exactly the long-lived-connection case keepalive is meant to protect — those sockets remain unprotected from idle NAT/load-balancer drops; consider having them subclass the new _TCPKeepaliveHTTPTransport/_TCPKeepaliveAsyncHTTPTransport classes as a follow-up.
Extended reasoning...
What the gap is: This PR introduces _TCPKeepaliveHTTPTransport and _TCPKeepaliveAsyncHTTPTransport in packages/python-sdk/e2b/api/__init__.py, and rewires the API/envd TransportWithLogger classes in e2b/api/client_sync/__init__.py and e2b/api/client_async/__init__.py to inherit from them instead of the plain httpx transports. That gives every API and envd connection SO_KEEPALIVE (plus a 60s TCP_KEEPIDLE/TCP_KEEPALIVE probe delay where supported), applied even through proxy connections via the custom _TCPKeepaliveNetworkBackend/_TCPKeepaliveAsyncNetworkBackend wrappers.
However, the Volume Content API has its own near-duplicate transport classes that were not touched: packages/python-sdk/e2b/volume/client_sync/__init__.py:58 defines class TransportWithLogger(httpx.HTTPTransport), and packages/python-sdk/e2b/volume/client_async/__init__.py:59 defines class AsyncTransportWithLogger(httpx.AsyncHTTPTransport). Both are constructed with only limits/proxy/retries (no socket_options), and both files already from e2b.api import connection_retries, make_logging_event_hooks (or the async equivalent) — proving they share code with the exact module this PR modifies, yet they were left on the plain httpx base classes.
Why it matters: Volume file upload/download is precisely the long-lived, potentially-idle connection scenario that TCP keepalive is meant to protect against, per this PR's own stated rationale (idle NAT/load-balancer drops). Both volume transports use the same pooling Limits with keepalive_expiry=300, so pooled connections can sit idle for up to 300 seconds — long enough for a NAT/LB to silently drop the socket — with no keepalive probes to detect or prevent that. After this PR merges, API and envd traffic is protected but Volume Content API traffic (uploads/downloads) is not, creating an inconsistency within the same codebase.
Why existing code doesn't prevent it: The volume transports are structurally independent classes — they don't inherit from or share a base with e2b.api.client_sync.TransportWithLogger/e2b.api.client_async.AsyncTransportWithLogger, so the PR's change to those classes has no effect on volume connections. The only coupling is the shared imports of connection_retries and the logging-hook helpers from e2b.api, neither of which carries the new keepalive behavior.
Concrete walk-through: (1) A user calls a Volume upload/download API, which reaches e2b/volume/client_sync/__init__.py:get_api_client. (2) That calls get_transport(config), which constructs TransportWithLogger(limits=limits, proxy=config.proxy, retries=connection_retries) — a plain httpx.HTTPTransport. (3) The underlying httpcore connection pool has keepalive_expiry=300, so an idle connection can be reused for up to 5 minutes without any TCP-level keepalive probes. (4) If a NAT device or load balancer between the client and the Volume Content API silently drops that idle connection before it's reused (a common occurrence well within a 300s window), the next request over that pooled connection will hang or fail with a connection-reset error — the exact failure mode this PR's keepalive change is meant to eliminate for API/envd traffic.
Suggested fix: Have packages/python-sdk/e2b/volume/client_sync/__init__.py's TransportWithLogger subclass e2b.api._TCPKeepaliveHTTPTransport instead of httpx.HTTPTransport, and similarly have packages/python-sdk/e2b/volume/client_async/__init__.py's AsyncTransportWithLogger subclass e2b.api._TCPKeepaliveAsyncHTTPTransport, mirroring exactly what was done for the API/envd transports in this PR.
On severity: This is not a regression — Volume connections had no keepalive before this PR either, so behavior for Volume traffic is unchanged. The PR's stated scope was explicitly "API and envd HTTP transports," and it does not touch the volume/ directory at all. I'm flagging this as a nit / worthwhile follow-up rather than a blocking issue, since merging as-is doesn't introduce new breakage — it just leaves an inconsistency where Volume traffic remains as exposed to idle-connection drops as it always was, right after a sibling code path fixed the identical problem.
| from typing import Callable, Optional, Protocol, Union | ||
| from typing import Any, Callable, Iterable, Optional, Protocol, TypeVar, Union, cast | ||
|
|
||
| import httpcore |
There was a problem hiding this comment.
🟡 This PR modifies packages/python-sdk (TCP keepalive on API/envd transports) but adds no .changeset entry, which CLAUDE.md explicitly requires for changes to packages/python-sdk. The changeset-bot comment on the PR already flags 'No Changeset found' — merging as-is won't trigger a version bump/release for @e2b/python-sdk despite the behavioral change.
Extended reasoning...
Bug
CLAUDE.md states explicitly: "Generate a changeset when updating packages/cli, packages/js-sdk, packages/python-sdk with pnpm changeset in the repository root." This PR touches three files under packages/python-sdk/e2b/api/ (__init__.py, client_async/__init__.py, client_sync/__init__.py) with a real behavioral change — enabling TCP keepalive socket options and rewiring the HTTP transports/proxy-mount logic — yet no changeset file accompanies the change.
Where this shows up
The repo already surfaces this: the changeset-bot[bot] comment on the PR timeline reads "No Changeset found... Merging this PR will not cause a version bump for any packages... This PR includes no changesets", and even links a suggested stub with "@e2b/python-sdk": patch. Checking .changeset/ in the repo confirms only two files exist (olive-pugs-refuse.md, tidy-moons-shout.md), and both were added by the prior, unrelated JS PR (#1607, commit b511953) targeting the e2b (JS) package — neither references @e2b/python-sdk or this PR's commit (3b1bd9b).
Why nothing else catches this
There is no CI gate in this repo that blocks merge on a missing changeset — the changeset-bot comment is advisory only, and nothing in the PR pipeline enforces the CLAUDE.md rule mechanically. It relies on the author (or reviewer) remembering to run pnpm changeset before merge.
Impact
@e2b/python-sdk participates in changeset-driven versioning (it is not in the changeset config.json ignore list) and is not on the shared ignore path with e2b (JS). Without a changeset, the next release run won't bump the python-sdk's version, so the keepalive fix (which addresses real long-lived-connection idle-drop behavior) will silently sit unreleased on main until someone notices and files it manually. Users depending on published e2b Python SDK versions won't receive the fix until that gap is caught.
Proof / how to reproduce
- Run
git log --oneline -1on this PR's tip — commit3b1bd9b— andgit show --stat 3b1bd9b: no file under.changeset/appears in the diff. - List
.changeset/*.mdin the repo: onlyolive-pugs-refuse.mdandtidy-moons-shout.mdexist, both predating this PR and both scoped to"e2b"(the JS package), not"@e2b/python-sdk". - Read the
changeset-bot[bot]PR comment: it independently confirms "No Changeset found" and even suggests the exact fix — a new file with frontmatter"@e2b/python-sdk": patch. - Cross-reference CLAUDE.md's explicit instruction naming
packages/python-sdkas one of the three packages requiring a changeset on update.
Fix
Run pnpm changeset in the repo root and select @e2b/python-sdk with a patch bump (this is a backward-compatible behavioral fix, not a breaking change), describing the keepalive change, before merging.
| # NOTE: `_pool` / `_network_backend` are httpx/httpcore private API. | ||
| # HTTPcore 1.0.x drops `socket_options` when it builds proxy | ||
| # connections (HTTPProxy/SOCKSProxy), so wrap the backend to force | ||
| # the keepalive options at actual TCP connect time. Verified against | ||
| # the `httpcore>=1.0.5,<2` pin in pyproject.toml — re-check on bumps. | ||
| pool = cast(Any, self._pool) | ||
| pool._network_backend = _TCPKeepaliveNetworkBackend(pool._network_backend) | ||
|
|
||
|
|
||
| class _TCPKeepaliveAsyncHTTPTransport(httpx.AsyncHTTPTransport): | ||
| """Async HTTPX transport that also covers HTTPcore proxy sockets.""" | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| kwargs["socket_options"] = _TCP_KEEPALIVE_SOCKET_OPTIONS | ||
| super().__init__(*args, **kwargs) | ||
| # NOTE: `_pool` / `_network_backend` are httpx/httpcore private API — | ||
| # see _TCPKeepaliveHTTPTransport for the rationale and version pins. |
There was a problem hiding this comment.
🟡 This PR imports httpcore directly and subclasses httpcore.NetworkBackend/AsyncNetworkBackend, but httpcore isn't declared as a direct dependency in pyproject.toml (only httpx>=0.27.0,<1.0.0 is pinned there) — it's only pulled in transitively via httpx. The inline comment on _TCPKeepaliveHTTPTransport also claims this was 'verified against the httpcore>=1.0.5,<2 pin in pyproject.toml,' but no such pin exists, which will mislead future maintainers checking compatibility on httpx/httpcore bumps. Consider adding an explicit httpcore dependency pin and correcting the comment.
Extended reasoning...
What the bug is: packages/python-sdk/e2b/api/__init__.py adds import httpcore and defines _TCPKeepaliveNetworkBackend(httpcore.NetworkBackend) / _TCPKeepaliveAsyncNetworkBackend(httpcore.AsyncNetworkBackend), referencing httpcore.NetworkStream / httpcore.AsyncNetworkStream as return types. This is a genuine new direct usage of httpcore's API surface (specifically its network-backend abstraction, which is not part of httpx's public/stable API contract). Despite this, packages/python-sdk/pyproject.toml's dependencies list only contains httpx>=0.27.0,<1.0.0 — there is no httpcore entry at all.
Where it manifests: Lines ~195-196 and ~205 of the diff contain comments like:
# ... Verified against the httpcore>=1.0.5,<2 pin in pyproject.toml — re-check on bumps.
This comment asserts a specific version constraint (httpcore>=1.0.5,<2) exists in pyproject.toml as the safety net for this private-API usage. I checked pyproject.toml directly (dependencies block, lines 9-22) and confirmed no such pin is present — httpcore doesn't appear anywhere in the dependency list. The comment refers to a constraint that simply isn't there.
Why existing code doesn't prevent it: Today this doesn't break anything at runtime, because every supported version of httpx<1.0.0 transitively requires httpcore 1.x, so the import always resolves and the NetworkBackend/AsyncNetworkBackend classes exist with a stable-enough shape across httpcore 1.x releases. But that safety is accidental/inherited from httpx's own transitive pin, not an explicit guarantee this package controls. If a future httpx release changes its httpcore requirement (e.g., loosens it further or, in a hypothetical httpx 1.x, drops the hard httpcore dependency), there is nothing in this package's own dependency metadata to catch that at install/resolution time — uv/pip would only know about the httpx constraint, not about this code's specific reliance on httpcore.NetworkBackend's current shape.
Impact: Two distinct issues, both low severity but real:
- Dependency-hygiene fragility — code that directly imports and subclasses a package's semi-private API should declare that package as a direct dependency so version bumps are caught by the resolver/lockfile diff, rather than relying on it being transitively present.
- A factually incorrect comment — it tells future maintainers "check the httpcore pin in pyproject.toml when bumping httpx," but there's no such pin to check, which could cause someone to skip verifying compatibility because they believe a safety net exists that doesn't.
Step-by-step proof:
- Open
packages/python-sdk/pyproject.toml,dependencies = [...]block (lines 9-22). - Confirm entries:
python-dateutil,wcmatch,protobuf-py,httpx>=0.27.0,<1.0.0,h2,attrs,packaging,typing-extensions,dockerfile-parse,rich,connectrpc,pyqwest. Nohttpcoreentry exists. - Open
packages/python-sdk/e2b/api/__init__.py, noteimport httpcore(new in this PR) and the_TCPKeepaliveNetworkBackend(httpcore.NetworkBackend)/_TCPKeepaliveAsyncNetworkBackend(httpcore.AsyncNetworkBackend)classes — direct use of httpcore's API. - Note the comment in
_TCPKeepaliveHTTPTransport.__init__: "Verified against thehttpcore>=1.0.5,<2pin in pyproject.toml — re-check on bumps." Searchpyproject.tomlforhttpcore— zero matches. The referenced pin does not exist. - Conclusion: the direct
httpcoreusage is undeclared, and the comment pointing maintainers to a version pin as the trigger for re-verification is inaccurate, since no such pin is present to trigger off of.
Fix: Add an explicit httpcore>=1.0.5,<2 (or whatever range was actually verified) entry to pyproject.toml's dependencies, and update the comment to either reference the newly-added pin correctly, or state plainly that no pin currently exists and compatibility should be manually re-checked whenever httpx's own httpcore requirement changes.
Severity: This is a nit, not a blocker. It causes no runtime failure today — httpx's own dependency constraints guarantee a compatible httpcore 1.x is always present — so nothing breaks by merging as-is. It's a dependency-hygiene / documentation-accuracy cleanup, appropriate as a follow-up rather than something that should delay this PR.
Add keepalive socket options (SO_KEEPALIVE plus TCP_KEEPIDLE / macOS TCP_KEEPALIVE with a 60s initial delay where supported) and keepalive httpx transport subclasses in e2b.api, and wire them into the sync and async API, envd, and proxy transports. On Windows and platforms without a keepalive-idle constant, only SO_KEEPALIVE is set. Custom network backends force the socket options at TCP connect time, since httpcore 1.0.x drops socket_options when constructing proxy connections. Because an explicit transport disables httpx trust_env proxy handling, a shared _build_env_proxy_mounts helper rebuilds the HTTP_PROXY/HTTPS_PROXY/NO_PROXY mounts for ApiClient. Also drop a dead verify parameter from _create_transport.
2 participants
Add keepalive socket options (SO_KEEPALIVE plus TCP_KEEPIDLE / macOS TCP_KEEPALIVE with a 60s initial delay where supported) and keepalive httpx transport subclasses in e2b.api, and wire them into the sync and async API, envd, and proxy transports. On Windows and platforms without a keepalive-idle constant, only SO_KEEPALIVE is set.
Custom network backends force the socket options at TCP connect time, since httpcore 1.0.x drops socket_options when constructing proxy connections. Because an explicit transport disables httpx trust_env proxy handling, a shared _build_env_proxy_mounts helper rebuilds the HTTP_PROXY/HTTPS_PROXY/NO_PROXY mounts for ApiClient. Also drop a dead verify parameter from _create_transport.