fix(python-sdk): use thread-local API transports#1399
Conversation
🦋 Changeset detectedLatest commit: 1ae0150 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 Tests clear thread-local caches instead of Reviewed by Cursor Bugbot for commit 1ae0150. Bugbot is set up for automated code reviews on this repo. Configure here. |
Package ArtifactsBuilt from 4bb8f50. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.28.1-threaded-api-calls-on-python-have-http2-collisions-eng-4272.0.tgzCLI ( npm install ./e2b-cli-2.10.5-threaded-api-calls-on-python-have-http2-collisions-eng-4272.0.tgzPython SDK ( pip install ./e2b-2.26.0+threaded.api.calls.on.python.have.http2.collisions.eng.4272-py3-none-any.whl |
There was a problem hiding this comment.
LGTM — thread-local sync API transports mirror the existing envd pattern; the dead else branch flagged inline is a nit.
Extended reasoning...
Overview
The PR makes the sync Python SDK's API HTTP transport cache thread-local by switching TransportWithLogger._instances (a shared class-level dict) to TransportWithLogger._thread_local = threading.local(), mirroring the pattern already used for EnvdTransportWithLogger. The change touches three files: a changeset markdown, packages/python-sdk/e2b/api/client_sync/__init__.py (the transport caching logic), and packages/python-sdk/tests/test_api_client_transport.py (helper extraction and a new regression test).
Security risks
None meaningful. This narrows visibility of cached transports rather than widening it — moving from process-global to thread-local actually reduces the chance of accidental state sharing across thread boundaries. The fix addresses HTTP/2 connection-state collisions between threads (the Linear ticket ENG-4272 / PR title), which is itself a correctness/safety improvement.
Level of scrutiny
Low. This is a small, mechanical change copying an existing, already-reviewed pattern (envd transports) onto an adjacent class. The transformation is straightforward (read-from / write-to a threading.local() attribute via getattr with a fresh-dict default), and the test suite was extended with a direct regression test (test_sync_api_transport_cache_reuses_within_thread_and_isolates_across_threads) that the author confirms fails on main and passes on this branch.
Other factors
The only finding from the bug hunting pass is a nit: a defensive else branch in the new reset_sync_api_transports test helper is unreachable (the class attribute always exists) and references the now-removed _instances attribute. It has no behavioral impact — it's dead code in a test helper — and was likely intended as migration scaffolding. The inline comment already conveys the suggested cleanup, so the author can collapse it in a follow-up commit at their discretion.
5ed1ed8 to
8b6baf3
Compare
makes the sync python python API http transport cache thread-local to handle unsafe usage of the shared transport under pressure (e.g. concurrent template builds). uses the same logic that we were using for envd.
test
test_sync_api_transport_cache_reuses_within_thread_and_isolates_across_threadsfails on main, passes on branch.