refactor(python-sdk): unify the pyqwest connection pools - #1692
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>
… pool open The unified pool is process-global now, so a close reaching it would take down every stack at once: pyqwest pools are closable and every httpx client holds the same cached adapter over one. The adapter forwards neither close() nor the context-manager exit the generated clients expose, which is what the docstrings promise — assert it against a live server for the sync and async pools, covering both a sibling httpx client and the pool the envd RPC stack executes on directly. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 54bb596 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 |
Package ArtifactsBuilt from 3f2884d. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.40.1-cursor-pr-claiming-mechanism-1180.0.tgzCLI ( npm install ./e2b-cli-2.16.3-cursor-pr-claiming-mechanism-1180.0.tgzPython SDK ( pip install ./e2b-2.40.0+cursor.pr.claiming.mechanism.1180-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 refactor of the Python SDK's core HTTP transport/connection-pooling layer with a real behavior change under load (envd RPC and the envd HTTP API now share one HTTP/2 connection's stream budget per sandbox, per the PR's own notes), a human look would still be worthwhile before merging.
What was reviewed: the unified pool cache keying (proxy, idle read bound, HTTP version) across the REST, envd HTTP, envd RPC, and volume stacks; the pyqwest-pool/httpx-adapter split and that closing one httpx client doesn't close the shared pool (get_httpx_transport/adapter close semantics, backed by the new regression test); and the frame-level HTTP/2 tests verifying the health probe reuses the live connection after an RST_STREAM and redials after a dropped TCP connection.
Extended reasoning...
Overview
This PR replaces four independently-cached pyqwest connection pools (control-plane REST, envd HTTP API, envd RPC, volume content API) with one shared pool per (proxy, idle read bound, HTTP version) key, exposed via two accessors (get_pyqwest_transport for connectrpc, get_httpx_transport for httpx clients) in e2b.api.client_sync/client_async. It touches the core transport plumbing for essentially every Python SDK network call: control-plane API requests, envd file transfers, envd RPC (process/filesystem), and volume content. Test infrastructure is substantially reworked to match (a new multi-connection HTTP/2 test server, a shared transport_caches.py reset helper, and a new frame-level test file asserting connection-reuse/redial behavior around RST_STREAM and dropped TCP connections).
Security risks
No new attack surface — no auth, crypto, or permission logic changes. The retry/plain-error-normalization semantics are preserved (connect-only retries, same error mapping), just relocated. The main risk category here is correctness/availability, not security: a bug in pool-key derivation or lock handling could cause connection reuse across incompatible configurations (e.g. wrong proxy or HTTP version), but the cache key and tests appear to guard against that.
Level of scrutiny
This warrants more than a mechanical-change pass. It's a large (16-file), non-trivial refactor of shared, process-global mutable state (pool caches guarded by locks) in a foundational part of the SDK that every sandbox operation depends on. The author's own PR description flags a real behavior change: envd RPC and the envd HTTP API 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 design tradeoff a human maintainer should explicitly sign off on, even though the bug-hunting pass and my own reading didn't surface a defect.
Other factors
Test coverage is thorough — pool-identity tests per stack, HTTP-version keying, proxy-credential isolation, and new frame-level tests specifically pinning the shared-connection/redial behavior for the health-probe path (handle_rpc_exception_with_health). The PR also separately documents (and files as SDK-332, not fixed here) a pre-existing RAM-buffering issue in the retry middleware for streamed uploads — worth a maintainer's awareness but out of scope for this change. Given the combination of scope, shared mutable state, and an acknowledged behavioral change, I'm deferring rather than approving.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
SDK test coverage report
Measured base 02ba746e (the true merge-base) against head 7f5cd88c, with E2B_API_KEY set so the live suites actually ran in both regimes.
Every changed source file sits at 100% statements and 100% branches on both sides, all 62 added executable lines are covered, no file anywhere in the package lost line or branch coverage, and all 14 new tests ran and passed. The package total dips 0.08pp purely because the refactor deletes 42 well-covered statements — the numerator falls faster than the denominator when you remove covered code.
python-sdk
base 02ba746e |
head 7f5cd88c |
|
|---|---|---|
| statements | 69.30% (10201/14720) | 69.22% (10161/14678) |
| branches | 46.10% (1299/2818) | 46.00% (1293/2810) |
| hand-written statements | 83.20% (5246/6305) | 83.13% (5206/6263) |
| generated statements | 58.9% (4955/8415) | 58.9% (4955/8415) |
| tests | 899 passed / 2 failed / 58 skipped | 907 passed / 2 failed / 58 skipped |
Diff coverage of added source lines: 62/62 (100%), nothing uncovered.
| changed file | statements base → head | branches |
|---|---|---|
e2b/api/client_sync/__init__.py |
38/38 → 41/41 (100%) | 4/4 → 4/4 |
e2b/api/client_async/__init__.py |
38/38 → 41/41 (100%) | 4/4 → 4/4 |
e2b/envd/client_sync/__init__.py |
39/39 → 29/29 (100%) | 8/8 → 6/6 |
e2b/envd/client_async/__init__.py |
57/57 → 47/47 (100%) | 12/12 → 10/10 |
e2b/envd/client_shared.py |
42/42 → 42/42 (100%) | 8/8 → 8/8 |
e2b/volume/client_sync/__init__.py |
36/36 → 22/22 (100%) | 4/4 → 2/2 |
e2b/volume/client_async/__init__.py |
36/36 → 22/22 (100%) | 4/4 → 2/2 |
e2b/sandbox_sync/filesystem/filesystem.py |
163/192 (84.9%), unchanged | 47/68, unchanged |
e2b/sandbox_async/filesystem/filesystem.py |
178/214 (83.2%), unchanged | 49/72, unchanged |
The two filesystem.py files only have docstring edits (get_envd_transport → get_transport), so their unchanged numbers are expected.
Unaffected packages, for context
This is a python-only change, and the other two packages measured identical to the same base commit, confirming no cross-package effect: js-sdk 81.96% lines (2159/2634), 82.05% statements, 72.72% branches, 88.82% functions, 611 passed / 1 failed / 33 skipped; cli 17.05% lines (236/1384), 21.72% branches, 109 passed. The cli figure is the usual artifact of its tests invoking the built CLI as a subprocess, which v8 cannot instrument — not untested code.
The three failures (test_firewall_transform_injects_headers in both python flavors, and the js-sdk httpbin sidecar test) are the pre-existing environmental ones: the test org has no httpbin template. All three reproduce identically on base.
Executed vs. asserted
Coverage percentages are the weak part of any report on a refactor like this, so the more useful question is which of the changeset's claims a test would actually catch a revert of. All four unification claims have an assertion behind them:
- REST and envd HTTP share a pool —
test_{sync,async}_envd_and_api_share_one_transport, plusenvd_negotiated is negotiatedin the http-version tests. - envd RPC runs on the shared pool —
test_rpc_clients_run_on_the_shared_poolrecords whatPlainHTTPErrorTransportis handed and asserts it is the cached pool, working aroundpyqwest.SyncClientnot exposing its transport. - The volume content API runs on the shared pool —
test_volume_transports_are_the_shared_sdk_pools, streaming variant included. - The actual payoff: one HTTP/2 connection per sandbox —
test_shared_transport_pool.pycounts accepted TCP connections against a real h2 server: one afterRST_STREAM, two after a TCP drop. This is the strongest test in the PR, because it fails if reqwest stops reusing the connection, not merely if an object identity changes. - The new hazard the PR creates — one
close()reaching a now process-wide pool — is pinned bytest_{sync,async}_closing_one_client_leaves_the_shared_pool_open, which drives real traffic through both the sibling httpx client and the raw pool after closing one client.
The six tests that disappear are all replaced rather than dropped: two renames (get_transport → get_pyqwest_transport in the envd modules), *_envd_transports_keyed_by_streaming folded into *_envd_and_api_share_one_transport, and test_transport_stack_normalizes_plain_errors_and_retries_connects split — its retry half into test_shared_pool_retries_connects, its normalization half already covered independently by the untouched test_envd_plain_http_errors.py.
One incidental strength worth keeping: because e2b/connection_config.py and e2b/volume/connection_config.py each define their own READ_TIMEOUT = 60.0, the new volume identity assertion only passes while the two constants agree, so it now pins them in sync.
Gaps
Two seams left, detailed inline. Neither is a regression this PR introduces, but the first one's blast radius grows now that a single pool serves every stack: nothing asserts that pool_max_idle_per_host, pool_idle_timeout, tls_include_system_certs or follow_redirects=False reach the pyqwest constructor, and test_shared_transport_pool.py proves connection reuse on a hand-built pool rather than the SDK's own, so pool wiring and pool reuse are verified by different tests with nothing joining them end to end.
A lock-ordering regression is covered by accident but reliably: _transport_lock is a non-reentrant threading.Lock, so moving the get_pyqwest_transport call inside it would deadlock, and pytest.ini sets timeout = 30, which turns that into a failure rather than a hung job. Note also that no workflow passes coverage flags and there are no thresholds anywhere in CI, so none of these numbers gate the merge either way.
Sent by Cursor Automation: /coverage SDK Test Coverage Report
| tls_include_system_certs=True, | ||
| proxy=proxy.to_pyqwest() if proxy is not None else None, | ||
| pool_idle_timeout=pool_idle_timeout, | ||
| pool_max_idle_per_host=pool_max_idle_per_host, |
There was a problem hiding this comment.
Coverage gap worth closing while this code is fresh: no test asserts that these pool-tuning kwargs actually reach the pyqwest constructor. test_env_var_parsing.py pins the e2b.api module constants, and test_sync_transports_pass_http_version_to_pyqwest pins http_version — but it captures only kwargs["http_version"] out of a record(**kwargs) hook that already has the whole dict in hand.
That gap predates this PR, but its blast radius grows here. Setting pool_max_idle_per_host=0 (or dropping tls_include_system_certs, or flipping follow_redirects) would leave every identity assertion in test_api_client_transport.py green and every frame-level test in test_shared_transport_pool.py green, while a sandbox redialed on every request — precisely the regression this PR exists to prevent. Since the monkeypatch hook is already there, asserting the rest of the recorded kwargs is a couple of lines.
|
|
||
| def _sync_pool() -> SyncConnectionRetryTransport: | ||
| return SyncConnectionRetryTransport( | ||
| SyncHTTPTransport(http_version=HTTPVersion.HTTP2), |
There was a problem hiding this comment.
This pool is hand-built rather than obtained from get_pyqwest_transport, and the module docstring gives the good reason (the SDK's transports negotiate the version over TLS via ALPN, and this server is plaintext, so the test needs HTTP/2 prior knowledge).
The consequence for coverage is worth naming: connection reuse is proven here on a pool that omits the SDK's actual tuning, while pool wiring is proven by the identity assertions elsewhere, and nothing joins the two end to end. Combined with the untested constructor kwargs, that leaves one narrow seam where a tuning change could keep both halves green and still cost a connection per request. A cheap way to narrow it would be building this pool through get_pyqwest_transport and overriding only http_version, so the rest of the tuning is the real thing.
|
|
||
| try: | ||
| assert get_sync_transport(config) is api_client_sync.get_transport(api_config) | ||
| assert get_sync_streaming_transport(config) is api_client_sync.get_transport( |
There was a problem hiding this comment.
Nice side effect: e2b/connection_config.py and e2b/volume/connection_config.py each declare their own READ_TIMEOUT = 60.0, so this identity assertion holds only while the two agree — changing one alone now fails here rather than silently splitting the streaming pool in two.
That also happens to be the only thing pinning the streaming pool's idle bound at the wiring level. The value itself is never asserted; streaming_transport is not api_transport in test_api_client_transport.py rules out None, and the stall/slow-transfer semantics are covered further down this file with READ_TIMEOUT monkeypatched to 0.3s. That combination is reasonable — just noting there is no sandbox-filesystem equivalent of those stall tests, only the _envd_api_streaming wiring assertion in test_sync_client_lifecycle.py.
There was a problem hiding this comment.
Taste review: pool unification
First, a caveat on scope: TASTE.md was not reachable from this run. https://raw.cursorusercontent.com/e2b/sdk-harness/main/TASTE.md returns 404, neither e2b/sdk-harness nor e2b-dev/sdk-harness resolves with this run's read-only token, and no copy exists in this repo or on the VM. So this is not a line-by-line check against that file — it is a review against the distilled rules from previous runs of this automation, plus verification of the PR's own claims.
On the taste rules there is genuinely little to bite on: this is internal transport plumbing with no user-facing surface change, and the new module-level names (get_pyqwest_transport, get_httpx_transport, TransportKey) match the existing precedent in the same modules (get_transport, get_envd_transport, the retrying_http_transport they replace). Sync and async mirror each other faithfully, comment-for-comment, with only the intended runtime/thread wording differing. So I spent the run on the substance instead, and found one real problem plus two inaccurate rationales.
The new test is flaky, and I can prove why
test_sync_dropped_connection_redials_for_the_health_probe failed on my first full run of tests/*.py (263 passed, 1 failed), then passed roughly a hundred subsequent executions — plain, --numprocesses=4 like CI, cold bytecode, pinned to a starved single CPU. There is no random ordering plugin here, so it is a timing race rather than an order dependency. The failure:
ConnectError: Request failed: error sending request for url
(http://127.0.0.1:45755/process.Process/Connect):
client error (SendRequest): connection error: connection reset
That is the first next(events) raising, so the exception escaped _break_sync_stream's pytest.raises block rather than being the failure the test wants. The cause is in SharedPoolServer._serve: in "drop" mode it sendalls the response head plus the first event and then RSTs the connection immediately, with nothing synchronizing against the client having consumed it. I confirmed the mechanism by mutation — moving the SO_LINGER/return above the sendall makes both drop tests fail with a byte-identical error message. (The naive TCP explanation is not it: a standalone socket probe showed Linux hands buffered data to recv before reporting ECONNRESET. The race resolves inside hyper, whose connection task fails the in-flight request with the connection error instead of dispatching the response head it already has.)
Details and a verified fix are in the inline comment on tests/envd_frame_server.py.
Two rationales that do not match reality
Neither changes behavior, but both are the kind of claim that outlives the diff and gets trusted later:
get_envd_transport's reason for existing is wrong. Its new docstring sayse2b-code-interpreterreaches into it; that package actually callse2b.api.client_{sync,async}.get_transport(config, http2=False), andget_envd_transportappears nowhere in its 119 Python files. After this PR nothing insidee2b/calls it either, so it is dead surface kept on a false premise.tests/transport_caches.pylists template uploads among the stacks drawing from the shared pool. They build their own transport inline intemplate_{sync,async}/build_api.py— as this PR's own description correctly says — soreset_transport_caches()does not in fact "clear them all."
What I verified as accurate
Every number in the description reproduced, and the substantive claims hold up:
| Claim | Result |
|---|---|
uv run pytest tests/*.py -q → 264 passed |
Reproduced (after the flake above) |
uv run pytest tests/shared -q → 128 passed, 1 skipped |
Reproduced exactly |
| format / lint / typecheck clean | Reproduced: ruff check clean, 409 files already formatted, ty check clean |
| Adapters never close the wrapped pool | Confirmed in the pinned pyqwest: neither PyqwestTransport nor AsyncPyqwestTransport defines close/aclose, so both inherit httpx's no-ops |
| The new close tests are not vacuous | Confirmed by mutation: forwarding close/aclose through to the pool makes both fail with exactly RuntimeError: Executing request on already closed transport. Worth knowing that the adapter holds ConnectionRetryTransport, which exposes no close at all — so the pool sits one layer further from an accidental close than the description implies |
| Sharing one retry middleware across the RPC and httpx stacks is safe | SyncRetryTransport/RetryTransport hold only immutable backoff configuration — no per-request or cross-request state |
One thing the description does not mention that I checked because it looked like a silent behavior change: routing the volume transports through get_httpx_transport now passes http_version=None where they previously passed nothing. pyqwest's default for that argument is None, so the negotiated version is unchanged.
Finally, a stale line in the description: it asks that #1659 be closed in favour of this PR, but #1659 is already closed. (I have not acted on that request either way — read-only token, and closing PRs is not this automation's job.)
The refactor itself looks right to me and the disclosed HTTP/2 stream-budget change is the correct thing to flag for a human. The flaky test is the one item I would fix before merge.
Sent by Cursor Automation: /review SDK complies with TASTE.md
|
@cursor check |
|
I addressed the concrete, well-supported issues the review bots raised on this PR. All changes are Python-SDK-only; What I changed (each its own commit, pushed to
I left the PR title/description as-is since the change wasn't requested there and the commit messages are self-describing. Note the acknowledged HTTP/2 stream-budget behavior change under heavy per-sandbox concurrency still warrants a human sign-off before merge.  |
The SharedPoolServer RST'd the TCP connection immediately after writing the response head and first event, with nothing ensuring the client had consumed them. When the RST won, hyper failed the in-flight request with the connection error instead of yielding the first event, so next(events) raised inside the stream helper's assert rather than its pytest.raises block, failing the two 'drop' tests intermittently. Gate the connection teardown on a threading.Event the test sets only after it has read the event written before the fault, so the RST can no longer race the response head. Harmless for the 'reset' tests, which never wait on it. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
The docstring claimed e2b-code-interpreter reaches into get_envd_transport, but that package imports get_transport directly and calls no such name in any of its files; nothing inside the SDK calls get_envd_transport either. Keep it as a backward-compatible alias of get_transport for any external importer of the older public name, and describe it as exactly that. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
reset_transport_caches() only clears the shared REST/envd/volume pools; template uploads build their own non-retrying transport inline and are not cached here, so listing them overstated what the helper resets. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
…nfigs The volume streaming pool and the sandbox-filesystem streaming pool are the same reqwest pool only while e2b.connection_config.READ_TIMEOUT and e2b.volume.connection_config.READ_TIMEOUT agree, since the read bound is part of the transport cache key. Flag the coupling next to both constants so a lone change does not silently split the pool in two. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
The identity assertions prove one pool is reused but not how it was built, so dropping pool_max_idle_per_host, the system CA certs, or follow_redirects=False would leave every identity and frame-level test green while a sandbox redialed on every request or TLS broke through an intercepting proxy. Record the full kwargs handed to the pyqwest transport and assert the tuning, using the streaming pool so read_timeout is pinned too. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
|
/sdk review |
There was a problem hiding this comment.
Taste review: pool unification
TASTE.md came through in full this run — earlier runs on this PR had to fall back on a distilled copy — so this is a genuine pass against the actual document. All four findings from my last review are fixed, three of them exactly as suggested, and I re-ran everything rather than reading the commits.
Verified
| check | result |
|---|---|
uv run pytest tests/*.py -q |
266 passed (the body still says 264) |
uv run pytest tests/shared -q |
128 passed, 1 skipped — as claimed |
make lint / make typecheck / make format |
clean, 409 files left unchanged — as claimed |
tests/test_shared_transport_pool.py × 30 |
30/30 green |
- The drop-test race fix is structural, not a timing tweak. Gating the teardown on
drop_whenmeans the RST can no longer precede the client's read of the first event, which is exactly the mechanism I demonstrated last time by hoisting theSO_LINGERabove thesendall. The 5-second bound on the wait is a good touch — a test that forgets to set the event degrades to the old behaviour instead of hanging into pytest's 30s timeout. - "No user-facing API change" holds.
e2b/__init__.pyis untouched, so nothing enters or leaves__all__, and TASTE's flat-entry-point rule is unaffected. - sync/async parity is clean. I normalised away the idiom (
async def,await, theAsync*/Sync*class names,client_sync↔client_async) and diffed all three mirror pairs. The only differences left are the ones that should be there:make_async_logging_event_hooks, and "thread-safe" vs "loop-independent" in the docstrings. Given this change rewrites six parallel modules, semantic drift between the mirrors was the failure I most expected to find, and it isn't there. - The volume stack genuinely is unconfigured-identical. It previously built its transport without passing
http_versionat all, and now reaches the shared pool viahttp2=True→http_version=None; the pinned pyqwest stub hashttp_version: HTTPVersion | None = Noneas the constructor default, so the two are the same configuration rather than merely similar. - Both outright deletions are safe.
gh search codeturns up no user ofretrying_http_transportanywhere outside this repo (the only other hit is a package-metadata scraper), and nothing importse2b.envd.client_syncat all. So apatchchangeset is defensible on impact — see the second inline comment for why the policy is still worth writing down.
Findings
Three, none of them blocking:
- A wrong version number in the changeset, which lands verbatim in the published CHANGELOG: the
http2restore shipped in@e2b/python-sdk2.39.1, not 2.38.1 (the Python SDK has no 2.38.1 at all). get_envd_transportis now a compatibility alias in everything but its docstring field. TASTE asks Python deprecations to carry:deprecated:with a migration path and a removal horizon; the corrected rationale in this PR is what makes the rule clearly apply.- A one-character naming nit on
TransportKey.
The docstrings throughout this change are unusually good — get_pyqwase… get_pyqwest_transport explaining why read_timeout and http2 have to be cache-key components, and the new coupling notes beside both READ_TIMEOUT constants, are the kind of thing TASTE means by treating docstrings as part of the API.
Sent by Cursor Automation: /review SDK complies with TASTE.md
| carrying the idle `read_timeout`: reqwest's read timer runs during body send and | ||
| TTFB, so on a shared pool it would cut off long uploads. No signature changes — | ||
| `get_transport` and `get_envd_transport` keep the `http2` parameter restored in | ||
| 2.38.1, and the two are now the same pool per key rather than two. |
There was a problem hiding this comment.
The version here is wrong: the http2 parameter came back in @e2b/python-sdk 2.39.1, not 2.38.1. The Python SDK has no 2.38.1 release at all — packages/python-sdk/CHANGELOG.md runs 2.38.0 → 2.39.0 → 2.39.1 → 2.40.0, and #1671's entry sits under the ## 2.39.1 heading. 2.38.1 exists only as an e2b (JS SDK) tag, which is probably where it came from.
Worth correcting because changeset prose is copied verbatim into the published CHANGELOG, and this is precisely the sentence a reader consults to find out which release to pin to keep http2 working.
`get_transport` and `get_envd_transport` keep the `http2` parameter restored in
-2.38.1, and the two are now the same pool per key rather than two.
+2.39.1, and the two are now the same pool per key rather than two.| now share one pool per key, since reqwest pools per host and envd RPC and | ||
| the envd HTTP API hit the same sandbox host anyway. | ||
|
|
||
| Kept only as a backward-compatible alias of :func:`get_transport` for any |
There was a problem hiding this comment.
The corrected rationale is accurate now — and it also settles this function's status: an alias kept solely for external importers of an older name, called nowhere inside the SDK. That is TASTE's definition of a deprecated identifier, so the deprecation rule applies directly: mark it with the Python :deprecated: field including the migration path and, where applicable, the removal horizon. At the moment the migration path exists only as prose ("Prefer get_transport"), so it won't render as deprecated in the docs and nothing dates it.
What makes this more than bookkeeping is that the same PR deletes two equally public-named neighbours outright — retrying_http_transport in this module and get_transport in e2b.envd.client_{sync,async} — with no alias and no note. So this module's compatibility policy is currently decided per name, with nothing recording the reasoning. I checked and both deletions are safe in practice (no external importer of either), so the ask is only to write the policy down where the next refactor will see it. The cautionary precedent is six days old: #1671 shipped in 2.39.1 to restore http2 on these very factories because e2b-code-interpreter imports them, even though #1671's own description reasoned they were internal.
Kept only as a backward-compatible alias of :func:`get_transport` for any
external importer of the older public name; nothing inside the SDK calls it
(``e2b-code-interpreter`` imports :func:`get_transport` directly). Prefer
:func:`get_transport`.
+
+ :deprecated: Use :func:`get_transport` instead; will be removed in the next
+ major version.
"""The async mirror in e2b/api/client_async/__init__.py needs the same line.
|
|
||
|
|
||
| def retrying_http_transport( | ||
| TransportKey = Tuple[Optional[ProxyConfig], Optional[float], bool] |
There was a problem hiding this comment.
Nit, and only a naming one: TransportKey is the key type for _transports and _httpx_transports and is used nowhere else, yet it is the one name in this group without the underscore its neighbours all carry (_transport_lock, _transports, _httpx_transports immediately below). TASTE keeps values the SDK needs but users don't underscore-prefixed, so _TransportKey would match both the rule and the surrounding lines. The docstring explaining why all three components are fixed at construction time is worth keeping either way — that's the part a future reader needs.
Same in the async mirror.
|
@cursor check |
|
Addressed the three non-blocking taste-review findings; all pushed to
|
The http2 parameter came back in @e2b/python-sdk 2.39.1 (#1671), not 2.38.1 — the Python SDK has no 2.38.1 release. Changeset prose lands verbatim in the published CHANGELOG, so the wrong pin would mislead anyone looking for which release keeps http2 working. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
…nsportKey get_envd_transport is now an alias kept solely for external importers of the older name and called nowhere inside the SDK — mark it :deprecated: with the migration path and a next-major removal horizon, matching TASTE and the existing beta_pause precedent. Rename TransportKey to _TransportKey so the cache-key type matches its underscore-prefixed neighbours (_transport_lock, _transports, _httpx_transports); it is module-private and used nowhere else. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
2 participants
Claimed from #1659 on
/sdk claimby the PR's own author (@mishushakov, org member). The original commit is carried over untouched, so authorship and theCo-Authored-Bytrailer are preserved — only PR ownership moves. Please close #1659 in favour of this PR (Closesdoes not auto-close pull requests, and this automation has no write access to do it).Closes SDK-291.
What changes
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, HTTP version), 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.
Added while claiming
One regression test the original was missing (
test_{sync,async}_closing_one_client_leaves_the_shared_pool_openintests/test_api_client_transport.py). The refactor's docstrings promise that "closing an httpx client leaves the pool intact for the other clients on it", and that promise is now load-bearing process-wide rather than per-stack, but nothing asserted it: pyqwest pools are closable (SyncHTTPTransport.close/HTTPTransport.aclose) and every httpx client in the SDK holds the same cached adapter over one. The existing tests all close their clients insidefinallyand then reset the caches, so a close that reached the pool would go unnoticed.The new tests round-trip against the local echo server, then close the control-plane client and assert that both a sibling client (the envd HTTP API) and the pool the envd RPC stack executes on directly still work. Verified in the pinned dependency that
PyqwestTransport/AsyncPyqwestTransportinherit httpx's no-opclose/acloseand never touch the wrapped pool, and confirmed the assertions are not vacuous: forwarding the adapter'sclose()to the pool makes both of them fail withRuntimeError: Executing request on already closed transport.Verification
uv run pytest tests/*.py -q— 264 passed (262 before the added test).uv run pytest tests/shared -q— 128 passed, 1 skipped.pnpm run format,pnpm run lint,pnpm run typecheck— clean.bytesrequest bodies in RAM (RetryingRequestContentaccumulates every chunk into abytearrayto make the body replayable), which is why template context uploads deliberately keep their own non-retrying transport — and why the same buffering applies to volume uploads and envdfiles.writeon the shared retrying pool, a pre-existing issue onmainfiled as SDK-332 rather than something this PR introduces.Notes for review
get_envd_transportsurvives only as an alias ofget_transportbecause external consumers (e2b-code-interpreter) call it; preferget_transportinside the SDK.@e2b/python-sdkpatch); the added test needs none of its own.🤖 Generated with Claude Code