refactor(python-sdk): unify the pyqwest connection pools - #1659
refactor(python-sdk): unify the pyqwest connection pools#1659mishushakov wants to merge 1 commit into
Conversation
Every persistent HTTP stack in the SDK now draws its connection pool from `e2b.api.client_sync`/`client_async`, keyed on (proxy, idle read bound), instead of caching four of its own: the control-plane REST API, the envd HTTP API, the envd RPC clients, and the volume content API. reqwest pools per host internally, so one pool serves the API host and every per-sandbox host without interference — and because envd RPC and the envd HTTP API hit the same host, an active sandbox needs a single HTTP/2 connection instead of one per stack. Two accessors expose it: `get_pyqwest_transport` hands connectrpc the pool behind the connect-only retries, and `get_httpx_transport` hands the generated httpx clients the `PyqwestTransport` adapter over that same pool. Layers above stay per-consumer, as the design calls for: `PlainHTTPErrorTransport` is now a stateless per-client wrapper rather than a cached transport, so Connect-error normalization stays RPC-only. Streamed downloads keep a pool of their own — the only one carrying the idle `read_timeout`, since reqwest's read timer runs during body send and TTFB and would otherwise cut off long uploads. Sharing puts the sandbox health probe on the connection the failed RPC was using, so `tests/test_shared_transport_pool.py` pins that at the frame level with a new multi-connection HTTP/2 server serving both routes on one pool: an RST_STREAM kills only the stream and the probe reuses the same connection, while a dropped TCP connection makes reqwest redial. Both paths still answer, so `handle_rpc_exception_with_health` keeps telling a wedged connection apart from a dead sandbox. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: ad9eb51 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 Envd RPC drops its own transport cache: Tests add Reviewed by Cursor Bugbot for commit ad9eb51. Bugbot is set up for automated code reviews on this repo. Configure here. |
Package ArtifactsBuilt from 19f7310. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.38.4-python-sdk-unify-pyqwest-connection-pools-once-all-http-sdk-291.0.tgzCLI ( npm install ./e2b-cli-2.16.2-python-sdk-unify-pyqwest-connection-pools-once-all-http-sdk-291.0.tgzPython SDK ( pip install ./e2b-2.38.0+python.sdk.unify.pyqwest.connection.pools.once.all.http.sdk.291-py3-none-any.whl |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large, cross-cutting refactor of the Python SDK's connection-pooling internals that changes runtime sharing behavior across the REST, envd RPC, envd HTTP, and volume stacks, a human look would still be worthwhile.
What was reviewed:
- Cache-key correctness for the unified transport dicts (proxy, idle read bound) across sync/async and all four consumers (API, envd HTTP, envd RPC, volume).
- The two-step lock pattern in
get_httpx_transport(resolve pool, then check/populate the httpx-adapter cache) — not a race since each dict access is independently locked and the operations are idempotent. - New frame-level HTTP/2 tests (
test_shared_transport_pool.py) validating that a health probe still succeeds after an RPC-stream reset (shared connection reuse) and after a full connection drop (redial).
Extended reasoning...
Overview
This PR consolidates four previously-separate pyqwest connection pools (control-plane REST API, envd HTTP API, envd RPC clients, and volume content API) into one process-global pool per (proxy, idle read bound) key, shared via get_pyqwest_transport/get_httpx_transport in e2b.api.client_sync/client_async. It removes per-stack transport caches from e2b/envd/client_async, e2b/envd/client_sync, and e2b/volume/client_*, replacing them with calls into the shared accessors. PlainHTTPErrorTransport becomes a stateless per-client wrapper instead of being cached alongside the pool. Streamed downloads keep a dedicated pool carrying the idle read timeout. Tests are substantially reworked: a new transport_caches.py helper resets all caches, and a new test_shared_transport_pool.py adds a real multi-connection plaintext HTTP/2 server to verify that RPC and envd HTTP now share one HTTP/2 connection per sandbox, and that health probes still succeed whether the shared connection survives (RST_STREAM) or is dropped entirely.
Security risks
None identified. No auth, crypto, or permission logic changes — this is purely about how many TCP/HTTP2 connections are opened and reused. Proxy credentials and headers remain part of the cache key (verified via existing tests), so proxy configs aren't cross-contaminated between differently-configured clients.
Level of scrutiny
This warrants a higher bar than a typical patch: it rewires the connection-pooling layer underneath every HTTP and RPC call the SDK makes (sandbox creation, file I/O, process RPC, volume access). The PR description itself flags a real behavioral change — RPC and envd HTTP now multiplex on one HTTP/2 connection and share its concurrent-stream budget, which could matter under heavy per-sandbox concurrency. That's the kind of tradeoff a maintainer with production traffic context should sign off on, even though the mechanics look correct and are well covered by tests.
Other factors
The test suite is extensive and specifically targets the riskiest part of the change (shared-connection health probing after a broken RPC stream) with a real socket-level HTTP/2 server rather than mocks, which gives good confidence in the specific scenario called out in the description. The PR also transparently documents a pre-existing, unrelated bug it deliberately does not fix (retry-buffering of streamed upload bodies), filed separately as SDK-332 — that's out of scope here and not a reason to block this PR.
1 participant
Every persistent HTTP stack in the Python SDK — control-plane REST, the envd HTTP API, the envd RPC clients, and the volume content API — now draws its connection pool from
e2b.api.client_sync/client_asynckeyed on(proxy, idle read bound), instead of each caching one of its own; reqwest pools per host internally, so one pool serves the API host and every per-sandbox host without interference, and because envd RPC and the envd HTTP API hit the same host an active sandbox needs a single HTTP/2 connection instead of one per stack. Two accessors expose it (get_pyqwest_transportfor connectrpc,get_httpx_transportfor the generated httpx clients) while per-layer concerns stay above the pool, soPlainHTTPErrorTransportbecomes a stateless per-client wrapper and Connect-error normalization stays RPC-only. Streamed downloads keep a pool of their own — the only one carrying the idleread_timeout, since reqwest's read timer runs during body send and TTFB and would otherwise cut off long uploads.Sharing puts the sandbox health probe on the connection the failed RPC was using, so
tests/test_shared_transport_pool.pypins that at the frame level with a new multi-connection HTTP/2 server serving both routes on one pool: anRST_STREAMkills only the stream and the probe reuses the same connection (which is also the proof the pool is genuinely shared), while a dropped TCP connection makes reqwest redial — both still answer, sohandle_rpc_exception_with_healthkeeps telling a wedged connection apart from a dead sandbox.No user-facing API change, so there are no usage examples to add — the public surface, timeouts, retry policy, and proxy handling are all unchanged, and JS has no counterpart since pyqwest pools are Python-only.
Closes SDK-291.
Notes for review
bytesrequest body in RAM to make it replayable (verified — a 16 MiB streamed PUT buffered 16 MiB), so joining the shared retrying pool would buffer whole build contexts. That's a pre-existing bug onmainfor volume uploads and envdfiles.write— filed as SDK-332 with the repro and two verified fixes; note the obvious fix doesn't work, because gating retries on the body type would silently strip connect retries from every envd RPC (connectrpc hands pyqwest a generator even for unary calls).🤖 Generated with Claude Code