fix(python-sdk): stop buffering streamed request bodies on the retrying transports - #1701
fix(python-sdk): stop buffering streamed request bodies on the retrying transports#1701mishushakov wants to merge 3 commits into
Conversation
…ng transports pyqwest's retry middleware keeps a request replayable, and for a body that is not already bytes it does that by growing a copy of it as the body is sent. A streamed upload was therefore sent incrementally and mirrored whole in RAM, so peak memory scaled with file size for files.write of a file-like object, volume.write_file and template context uploads (SDK-332). Declare pyqwest's unbuffered retry mode on the shared retrying transports instead: a streamed body is replayed only while nothing has been read from it, which is all the connect-only policy needs, since a connect error means the request was never written. bytes bodies keep their retries either way. Releases whose retry middleware has no RetryMode keep the buffered behavior they ship, where going unbuffered would cost the connect retries instead. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 945624c 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 d5b0dcb. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.41.1-cursor-unbuffered-retry-streamed-bodies-f12f.0.tgzCLI ( npm install ./e2b-cli-2.16.3-cursor-unbuffered-retry-streamed-bodies-f12f.0.tgzPython SDK ( pip install ./e2b-2.41.0+cursor.unbuffered.retry.streamed.bodies.f12f-py3-none-any.whl |
Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
![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: b98324ee0a
ℹ️ 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".
| # chunk of a unary RPC body, up to a whole small-chunked upload) and neither | ||
| # buffered nor rewindable. | ||
| _retry_mode = getattr(retry_middleware, "RetryMode", None) | ||
| unbuffered_retries: Any = True if _retry_mode is None else _retry_mode.UNBUFFERED |
There was a problem hiding this comment.
When the SDK is installed with its currently supported and locked pyqwest 0.9.0, RetryMode is absent, so this branch returns True and preserves the original full-body buffering; the new memory tests are skipped in exactly that configuration. Because packages/python-sdk/pyproject.toml still allows 0.9.0 and uv.lock still selects it, publishing this patch does not actually fix large streamed uploads for default or existing installations. Raise the pyqwest lower bound to the release containing RetryMode and refresh the lockfile before shipping the changeset.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
SDK test coverage — #1701
Base is the true merge-base 666241d47, not the 15bd48b73 the trigger quoted (that is origin/main's tip, one commit ahead). Python-only PR, E2B_API_KEY present so the live suites really ran.
The numbers
| Base | Head | |
|---|---|---|
| statements | 69.44% (10307/14844) | 69.46% (10315/14851) |
| branches | 46.67% (1332/2854) | 46.71% (1333/2854) |
| hand-written | 83.55% (5271/6309) | 83.58% (5279/6316) |
| generated | 59.00% (5036/8535) | unchanged |
| tests | 934 pass / 2 fail / 57 skip | 936 / 2 / 63 |
Diff coverage 7/7 added executable statements (100%). No file lost line or branch coverage, none added or removed. Both transports go 41/41 → 43/43 lines with branches steady at 4/4. The 2 failures are the pre-existing environmental test_firewall_transform_injects_headers pair (the test org has no httpbin template), identical on base and head. Test inventory: +8, 0 removed, 0 status changes.
Why that 100% means nothing here
Those +8 tests are 2 passed and 6 skipped. pyqwest.middleware.retry has no RetryMode in 0.9.0 — which uv.lock pins, pyproject.toml caps (>=0.9.0,<0.10), CI installs via uv sync --locked, and which is the newest release on PyPI. No published pyqwest exposes RetryMode at all, so unbuffered_retries is True and @unbuffered_only skips every test that pins the unbuffered half.
And on 0.9.0 the base class's should_retry_request already returns True (verified directly). So the new override returns precisely the value the middleware used before this PR.
Mutation testing — revert a piece of the change, re-run test_retry_streamed_bodies.py + test_envd_retry_transport.py:
| mutant | result |
|---|---|
delete should_retry_request from both transports (the entire behavioural change) |
survived — 2 passed / 6 skipped, identical to head |
hard-code unbuffered_retries = True, deleting the RetryMode lookup |
survived |
no-RetryMode fallback returns False |
caught (9 failures) |
override returns False |
caught (7 failures) |
The two caught mutants are caught by the pre-existing test_envd_retry_transport.py, and all they prove is that connect retries still happen. Nothing in this PR's 244-line test file can tell head apart from base on any installable pyqwest. That is the coverage-side corroboration of Codex's P1 thread, arrived at independently — and worth adding that "raise the lower bound" is not yet actionable, because there is no pyqwest release to raise it to.
This is also the cleanest example yet of a construct coverage cannot see: the decision lives in a ternary, and coverage.py does not branch on ternaries, so e2b/api/__init__.py gained 3 covered statements and zero new branches. The dead arm is invisible.
What does check out
- The fix reaches everything the changeset claims. Since #1692 all four stacks (control-plane REST, envd HTTP, envd RPC, volume content) draw from the single
ConnectionRetryTransport, sofiles.write,volume.write_fileand template contexts are genuinely covered by one override. - The premise is real. I checked that the httpx adapter does not flatten bodies: a generator body and an
httpxmultipart file both arrive at the pyqwest transport as agenerator, notbytes. So there really is a body for the replay buffer to mirror. - The two tests that do run are correct and offline, and the
bytes-body retries stay pinned bytest_envd_retry_transport.py.
Three inline suggestions below, each with a test I ran against both head and the mutant it is meant to kill.
Sent by Cursor Automation: /coverage SDK Test Coverage Report
| # so a connect error surfaced with the body already started (measured: the one | ||
| # chunk of a unary RPC body, up to a whole small-chunked upload) and neither | ||
| # buffered nor rewindable. | ||
| _retry_mode = getattr(retry_middleware, "RetryMode", None) |
There was a problem hiding this comment.
The one line that decides the whole feature is the one line no test can reach.
_retry_mode is None on every installable pyqwest, so the else arm never evaluates — and because coverage.py does not branch on ternaries, this file shows +3 covered statements and 0 new branches. Nothing reports the dead arm.
Making the derivation a function makes both arms testable today, without waiting on a pyqwest that has RetryMode:
def _resolve_unbuffered_retries(middleware: Any) -> Any:
retry_mode = getattr(middleware, "RetryMode", None)
return True if retry_mode is None else retry_mode.UNBUFFERED
unbuffered_retries: Any = _resolve_unbuffered_retries(retry_middleware)with:
from types import SimpleNamespace
from e2b.api import _resolve_unbuffered_retries
def test_unbuffered_mode_is_taken_from_pyqwest_when_available():
mode = object()
middleware = SimpleNamespace(RetryMode=SimpleNamespace(UNBUFFERED=mode))
assert _resolve_unbuffered_retries(middleware) is mode
def test_retry_policy_falls_back_to_buffered_without_retry_mode():
assert _resolve_unbuffered_retries(SimpleNamespace()) is TrueVerified: both pass on head with the refactor, and the first one fails on the mutant that drops the RetryMode lookup — the mutant that survives the current suite. It is the only way to pin the forward-compatible arm before a pyqwest exists that exercises it.
| Streamed bodies are retried without being buffered, so an upload is not | ||
| mirrored in memory as it is sent (see ``unbuffered_retries``).""" | ||
|
|
||
| def should_retry_request(self, request: SyncRequest) -> Any: |
There was a problem hiding this comment.
This override is entirely unpinned. Deleting it here and in client_async leaves the suite at 2 passed / 6 skipped — identical to head — because on pyqwest 0.9.0 the inherited should_retry_request already returns True.
So an accidental removal (or a refactor that drops it while collapsing the two transports) is silent today, and only surfaces as a memory regression once pyqwest ships RetryMode. Cheap guard:
@pytest.mark.parametrize(
"transport", [SyncConnectionRetryTransport, ConnectionRetryTransport]
)
def test_transports_declare_the_retry_mode(transport):
# Inheriting pyqwest's default silently restores the buffered replay.
assert "should_retry_request" in transport.__dict__
assert transport.should_retry_request(None, None) is unbuffered_retriesVerified: passes on head, and fails for both transports when the override is removed. The __dict__ check is what does the work — asserting only the return value cannot catch it while unbuffered_retries is True.
| """16 MiB in 256 KiB chunks: large enough that a mirrored copy dwarfs the | ||
| per-chunk allocations, small enough to stay quick.""" | ||
|
|
||
| unbuffered_only = pytest.mark.skipif( |
There was a problem hiding this comment.
This guard skips 6 of the 8 tests in every environment that can currently exist, including CI (uv sync --locked → pyqwest 0.9.0). Locally: 2 passed, 6 skipped.
That is a defensible way to write forward-compatible tests, but as it stands the file's headline assertions — the tracemalloc peak bound, the no-replay-after-read behaviour, the real-transport pulled == [] checks — have never executed anywhere, so they are unreviewed code rather than passing tests. Two things worth doing:
- Make the skip loud rather than silent. A module-level
pytestmarkwhose reason names the pyqwest version, or a single always-running test asserting which regime is active, means that the day pyqwest shipsRetryModenobody has to notice these tests quietly switching on. Today a reader of green CI sees 8 collected tests with no signal that 6 are inert. - Consider exercising the transport through its real wiring. Every test builds a
pyqwest.Request/SyncRequestby hand, but every call site the changeset names (files.write,volume.write_file, template contexts) goes through thePyqwestTransporthttpx adapter. I confirmed that adapter passes both a generator body and anhttpxmultipart file down as agenerator, so the layer is sound — but nothing in the suite proves it, and that join between "the transport declares the mode" and "an upload is not mirrored" is what the fix actually depends on.
Same pattern as the inflight work in #1695, where every test of the new wrapper used a stub fetcher and never a response from the real dispatcher.
There was a problem hiding this comment.
TASTE.md review
Against TASTE this lands clean. The change is one hook on two mirrored classes, sync and async are byte-identical modulo the Sync prefix, the policy is a named module constant rather than an inline literal at the call site, and nothing new reaches e2b/__init__.py (it still imports only ApiClient, client from .api). Your numbers reproduce exactly: 421 passed, 7 skipped, ruff check and ruff format --check clean over 414 files, ty check clean. packages/js-sdk/src has no retry middleware at all, so "JS has no counterpart" holds as an absence, not just an assertion.
The load-bearing premise is true, and I verified the part the change can't verify itself. Everything rests on the pre-existing claim that pyqwest raises the builtin ConnectionError only before the request was written — if a mid-body drop surfaced as ConnectionError, UNBUFFERED would forfeit a retry the SDK performs today. It doesn't. None of pyqwest's error classes (ReadError, WriteError, RemoteProtocolError) subclass ConnectionError, and a socket server that resets mid-body raises, 3 runs out of 3 with 3-6 chunks already written:
pyqwest.WriteError: Request failed: error sending request for url (...): client error (SendRequest): error writing a body to connection: Connection reset by peer (os error 104)
isinstance(e, ConnectionError) = False
should_retry_response already declines that, so nothing is traded away and no E2B_ opt-out knob is warranted here. I also checked the upstream surface you code against blind: pyqwest#219 is merged, RetryMode really is re-exported from pyqwest.middleware.retry.__all__, should_retry_request really is typed -> bool | RetryMode, normalize_retry_mode(True) really is BUFFERED, and unbuffered_stream is gated on not isinstance(content, bytes), so "bytes bodies keep their retries either way" is exactly right.
The tests have real teeth — once RetryMode exists. Upstream's retry middleware is pure Python, so I overlaid the post-#219 version onto the pinned 0.9.0 wheel: 6 of your 8 pass, and removing the should_retry_request override makes all 6 fail, with the buffered peak at 17,337,488 B = 16.53 MiB, matching your claimed 16.5 MiB. The other 2 (real transport, refused port) fail there, pulling 2 chunks in sync and the whole body in async — an independent reproduction of your 0.9.0 table and confirmation that #218 is genuinely required rather than nice to have. On the pinned release CI runs 2 passed, 6 skipped; deleting the whole source change leaves that byte-identical, which is inherent (on 0.9.0 your True is the base default), while flipping the fallback to False is caught by the two un-gated tests. So the fallback's behavior is pinned; its presence can't be.
Three findings, none blocking
- The changeset promises a fix for template build contexts that this PR doesn't deliver, and changeset prose is copied verbatim into
packages/python-sdk/CHANGELOG.md, so it ships to users. Your own "Notes for review" says the opposite. Inline, along with the two source copies of the same sentence. ConnectionRetryTransport's docstring asserts the unbuffered behavior unconditionally, but onpyqwest==0.9.0— the only releasepyqwest>=0.9.0,<0.10admits, and whatuv.lockpins — streamed bodies are still mirrored. The module comment states the fallback carefully; the docstring, which is whathelp()renders, reads as a promise. TASTE makes docstrings part of the API. Inline.- It ships inert, and the pin may not let it activate on its own. Latest pyqwest on PyPI is 0.9.0 (Aug 10); #218 and #219 merged Aug 12 and 14 with no release since. #219 also changes upstream's default policy (POST becomes
UNBUFFERED), which reads like a 0.10.0 change — and<0.10excludes it, so "bump the pin" is likely a cap change rather than a floor bump that happens automatically. That's your stated follow-up and it's the right one; the thing worth reconsidering is shipping the changeset now, since it announces a memory fix the release it lands in does not contain.
Nits: the naming inversion on the constant and its public name among underscore-prefixed internals (inline); the skip markers key on the policy value rather than on the capability, so a wrong value turns tests into skips (inline); and the verification section says "The five that pin the unbuffered half skip" when there are six @unbuffered_only markers over 8 tests, which the next bullet gets right.
The comment block above the constant in e2b/api/__init__.py is the best thing in the diff — it records why the fallback is True and why a connect-only policy still needed #218, which is precisely the reasoning that would otherwise be lost the moment the pin moves.
Sent by Cursor Automation: /check SDK complies with TASTE.md
| Stop mirroring streamed request bodies in memory while they are being sent. The | ||
| shared retrying transports now declare pyqwest's unbuffered retry policy, so a | ||
| streamed upload — `files.write` of a file-like object, `volume.write_file`, | ||
| template build contexts — is no longer copied into a replay buffer as it goes to |
There was a problem hiding this comment.
Template build contexts were never on this path, so this clause promises something the PR doesn't deliver — and changeset prose is copied verbatim into packages/python-sdk/CHANGELOG.md, so this is the one artifact here that reaches users.
template_sync/build_api.py::upload_file (and its async mirror) builds its own transport inline, with no ConnectionRetryTransport anywhere in it:
transport=PyqwestTransport(
SyncHTTPTransport(
tls_include_system_certs=True,
proxy=(...),
follow_redirects=False,
)
),No retry middleware means no replay buffer, and it has been that way since #1603 — git log -S 'PyqwestTransport(' -- packages/python-sdk/e2b/template_sync/build_api.py returns only that commit. Your own "Notes for review" agrees: "template context uploads still build their own inline transport ... specifically to stay off the retrying stack."
The other two entries check out, for what it's worth: files.write hands the file object straight through as content=, volume.write_file passes IO[bytes] / Iterator[bytes], and the pyqwest httpx adapter turns anything that isn't an httpx.ByteStream into an iterator, which is exactly what RetryingRequestContent mirrors.
Suggest dropping the template clause here and in the two source copies (e2b/api/__init__.py:96 and tests/test_retry_streamed_bodies.py:8), or recasting it as the follow-up your Notes section already describes.
| # isn't already `bytes` it does that by growing a copy of it in memory as it is | ||
| # sent. A streamed upload therefore went to the wire incrementally *and* was | ||
| # mirrored whole in RAM, so peak memory scaled with file size for | ||
| # `files.write`, `volume.write_file` and template context uploads even though |
There was a problem hiding this comment.
Same claim as in the changeset: template context uploads never went through this middleware, so they were never mirrored by it. Since this comment is the place a future reader will look to understand why the constant exists, it's worth being exact about which two paths were affected — files.write of a file-like object and volume.write_file — and mentioning template uploads only as the stack that is deliberately off the retrying transport.
| # chunk of a unary RPC body, up to a whole small-chunked upload) and neither | ||
| # buffered nor rewindable. | ||
| _retry_mode = getattr(retry_middleware, "RetryMode", None) | ||
| unbuffered_retries: Any = True if _retry_mode is None else _retry_mode.UNBUFFERED |
There was a problem hiding this comment.
Two naming nits here, plus one thing worth defending in a comment.
unbuffered_retries = True means buffered. The name reads as a boolean flag, but the value is a policy, and its truthy spelling carries the opposite of what the name says. The clearest symptom is in this PR's own test file:
unbuffered_only = pytest.mark.skipif(
unbuffered_retries is True,
reason="pyqwest without RetryMode buffers streamed bodies to replay them",
)"Skip when unbuffered_retries is True" gating the unbuffered-only tests. TASTE prefers an enum over a boolean for options that select a behavior, and here you already have upstream's enum whenever it exists — the constant just needs a name that admits it isn't a flag (_retry_request_mode / _retry_request_policy), with the test gate derived from _retry_mode is None instead.
It's also the only public-named module attribute among internals in this family. _retry_mode is on the line above, and _TransportKey / _transport_lock / _transports / _httpx_transports in client_{sync,async} are all prefixed — _TransportKey was renamed for exactly this reason in the last round on #1692. Only the two transports and one test read this, and e2b/__init__.py imports just ApiClient, client from .api, so _-prefixing costs nothing. The unprefixed neighbours (connection_retries, pool_idle_timeout, pool_max_idle_per_host) each mirror a documented E2B_* knob; this one doesn't.
The Any here and on the override is forced, and that deserves a one-line comment — it currently reads like a shortcut. I tried the tighter Union[bool, Enum], which needs no import from the unpinned release, and ty rejects it:
error[invalid-method-override]
--> e2b/api/client_sync/__init__.py
::: .venv/.../pyqwest/middleware/retry/_sync.py:160:9
160 | def should_retry_request(self, request: SyncRequest) -> bool:
info: This violates the Liskov Substitution Principle
The pinned base declares -> bool, so Any really is the only spelling that typechecks until the pin moves. Worth saying so in the comment block so nobody tightens it and reverts.
| for idempotent methods.""" | ||
| for idempotent methods. | ||
|
|
||
| Streamed bodies are retried without being buffered, so an upload is not |
There was a problem hiding this comment.
This states the new behavior unconditionally, but it isn't true of the release the SDK pins. On pyqwest==0.9.0 — the only release pyqwest>=0.9.0,<0.10 admits, and what's in uv.lock — unbuffered_retries is True, so a streamed body is still mirrored in memory as it is sent. I confirmed the constant evaluates to True in a fresh uv sync.
The comment block beside the constant gets the fallback exactly right; this docstring, which is what help(ConnectionRetryTransport) renders and the one a reader of this class actually sees, reads as a promise. TASTE treats docstrings as part of the API, so it's worth conditioning it the way the comment already does — something like "...on a pyqwest whose retry middleware exposes RetryMode; earlier releases keep that release's buffered behavior."
Same paragraph in client_async/__init__.py:38.
| per-chunk allocations, small enough to stay quick.""" | ||
|
|
||
| unbuffered_only = pytest.mark.skipif( | ||
| unbuffered_retries is True, |
There was a problem hiding this comment.
Keying the skip on the policy value rather than on the capability makes a wrong value silently become a skip. skipif(_retry_mode is None, ...) says the same thing and stays correct if the constant is ever spelled differently.
Related gap worth closing while you're here: nothing pins the fallback branch, which is the only branch CI executes today. An inverse buffered_only test does, and I verified it behaves correctly in all three states — passing on the pinned release, failing under a mutated fallback, skipping once RetryMode exists:
buffered_only = pytest.mark.skipif(
_retry_mode is not None,
reason="pyqwest with RetryMode replays streamed bodies only while unread",
)
@buffered_only
def test_buffered_streamed_body_is_replayed_after_a_mid_body_failure():
inner = MidBodyFailing(failures=1) # reads a chunk, then raises ConnectionError
pulled: List[int] = []
request = SyncRequest("PUT", "http://sandbox.test/upload", content=_chunks(pulled))
response = _retrying_sync(inner).execute_sync(request)
assert response.status == 200
assert inner.attempts == 2
# Replayed from the buffer: the first chunk was sent twice.
assert inner.received == CHUNK_SIZE + BODY_SIZEThat is the exact inverse of test_sync_streamed_body_is_not_replayed_once_it_was_read, so the pair documents both policies and CI always runs one of them. With the gate written as unbuffered_retries is not True instead, mutating the fallback to False turned it into a skip rather than a failure — which is the same value-vs-capability trap as above.
|
@cursor check |
|
I worked through the review findings and pushed them as one commit; CI is now fully green, including the staging jobs that flaked before. The overclaims are fixed. Template build contexts were never mirrored by the retry middleware — both The naming inversion is gone. The coverage review's real finding was the important one. On pyqwest 0.9.0 the inherited hook already returns One finding I did not act on, because I don't think it's mine to decide: both reviewers and Codex flagged that this ships inert and suggested either raising the pyqwest floor or holding the changeset back. The floor can't move — 0.9.0 is still the newest release on PyPI, and #218/#219 are unreleased — so the changeset now says plainly that the fix is inert until the dependency allows a pyqwest with  |
Review found three overclaims and a gap: - template context uploads were never mirrored by the retry middleware, since they build their own transport without it, so drop them from the changeset, the module comment and the test docstring; - the transports' docstrings promised unbuffered streaming unconditionally, which is not true of the pinned pyqwest, so condition them the way the module comment already does; - the policy constant read as a boolean flag while holding a policy, and spelled its buffered value `True`, so rename it to _retry_request_policy and prefix it like the other internals in this family. The gap: on the pinned pyqwest the inherited hook already returns `True`, so nothing failed if the override or the RetryMode lookup were removed. Resolving the policy is now a function, and the tests pin both of its arms, the presence of both overrides, which regime is live, the buffered replay the fallback relies on, and the streamed body shape the httpx adapter hands the middleware. Co-authored-by: Mish Ushakov <mishushakov@users.noreply.github.com>
2 participants
pyqwest's retry middleware keeps a request replayable, and for a body that isn't already
bytesit does that by growing a copy of the body as it is sent. A streamed upload therefore went to the wire incrementally and was mirrored whole in RAM, so peak memory scaled with file size forfiles.writeof a file-like object and forvolume.write_file, even though nothing in the SDK buffers (SDK-332). Template build contexts were never affected — they build their own transport with no retry middleware — and an earlier revision of this description, the changeset and the module comment all wrongly claimed otherwise; that is fixed.The copy is what makes retries work, so pyqwest#219 added a way to decline it:
RetryMode.UNBUFFEREDreplays a streamed body only while nothing has been read from it. That is exactly what the SDK's connect-only policy needs —ConnectionRetryTransportretries the builtinConnectionError, which pyqwest raises only before the request was written — so the shared retrying transports (sync and async) now declare it as their per-request policy.bytesbodies, which is every unary RPC payload and every in-memory write, are replayable as they are and keep their retries either way.Read this before reviewing the diff: the fix is dormant on the pinned pyqwest.
RetryModelanded upstream after 0.9.0 and PyPI's latest pyqwest is 0.9.0, so there is no release to pin to yet and neitherpyproject.tomlnoruv.lockcan move in this PR. The policy is therefore resolved frompyqwest.middleware.retryat import: withRetryModethe transports go unbuffered, without it they keep the buffered behavior that release ships. Merging this alone does not change what apip install e2buser gets; bumping the pin to the release carrying #218 and #219 is what activates it, and needs no further SDK change. The alternative — widening the pin to<0.11now — would adopt an unreleased, untested pyqwest minor sight unseen, which is not how the SDK has taken pyqwest 0.8 and 0.9. SDK-332 stays open until the pin moves, and the changeset says the fix is inert until then.Why the unbuffered policy is safe now and was not before
The earlier reading of this ticket was that a connect-only policy provably never needs the copy, and SDK-332 corrected it: on pyqwest 0.9.0 reqwest read ahead into its body channel while connecting, so a connect error surfaced with the body already started (1–2 chunks in sync, up to the whole body in async) and a replay would have sent a truncated request. That is why the fallback keeps buffering rather than going unbuffered everywhere: on 0.9.0 the choice is the copy or the connect retries, and connectrpc hands pyqwest a generator even for unary RPCs, so dropping the retries would hit every envd RPC.
pyqwest#218 closed that gap — the body stream is started when hyper first polls it, not while connecting — so with it the SDK gets both. Measured against a refused port, chunks pulled from the body before
ConnectionError, 3 runs each:No user-facing API change, so there are no usage examples to add: same public surface, same options, same timeouts, and connect retries behave as before. JS has no counterpart — undici streams request bodies without buffering and the retry middleware is pyqwest-only.
Verification
Against pyqwest built from its
main(i.e. with #218 + #219), traced peak allocations for a genuinely streamed upload of a 64 MiB file object, with the full body confirmed at the far end each time:files.write, syncfiles.write, asyncfiles.writemultipart (use_octet_stream=False)volume.write_file(32 MiB)tests/test_retry_streamed_bodies.py(new, 21 tests) is split so that CI exercises whichever policy is installed rather than skipping the file wholesale — 13 pass / 8 skip on the pinned pyqwest, 19 pass / 2 skip onmain:PyqwestTransport/AsyncPyqwestTransporthttpx adapter, acontent=generator and a multipart file object both reach the middleware as a streamed (non-bytes) body — the join the transport-level tests would miss if the adapter ever flattened a body.Mutation-checked on the pinned pyqwest, where the review found two mutants surviving: deleting both
should_retry_requestoverrides now fails 2 tests, and deleting theRetryModelookup fails 1 (both previously passed, since 0.9.0's inherited hook also returnsTrue); a fallback ofFalsefails 11.main— so adopting that release does not disturb the rest of the SDK either.main: the fullfiles/suites sync and async (123 tests, in-memory, streamed octet-stream and multipart writes), andtests/sync+tests/asyncminus the template build suites — 378 passed, with the same twotest_firewall_transform_injects_headersfailures the pinned pyqwest produces on that account (itshttpbintest template is missing, unrelated).rufflint/format andtytypecheck are green. No JS or TS files touched.template_{sync,async}/test_build.py::test_build_templatetiming out at 180s, which reproduces on unrelated PRs against staging right now (e.g. feat(sdk): refresh the MCP server schema from Docker's MCP catalog #1700) and touches no code in this diff.Notes for review
e2b.api._retry_request_policy, resolved by_resolve_retry_request_policyso both of its arms are testable on any pyqwest. It is_-prefixed like the other internals in this family (the unprefixed neighbours each mirror a documentedE2B_*knob; this one doesn't) and named for the policy it holds rather than as a flag, since its buffered value isTrue.Anyon the constant and the overrides is forced, and the comment now says so: the pinned release declaresshould_retry_requestas-> bool, so a union that admitsRetryModefailsty's Liskov check until the pin moves.bytesbody, so it stays on the replayable path.