fix: resume interrupted file downloads with byte-range retries - #3764
fix: resume interrupted file downloads with byte-range retries#3764CacinieP wants to merge 10 commits into
Conversation
Downloading large Batch API result files (>200-300 MB) can fail because long-lived single connections are cut off mid-body; retries restart the download from the first byte, so they can never make progress when the transfer dies at a deterministic size or duration threshold. GET retries now resume an interrupted body with Range requests when the server advertises Accept-Ranges: bytes: - the first attempt behaves exactly as before (full download); - if the body read dies mid-transfer with a retryable error, later attempts request only the missing bytes (206 appends, a 200 from a range-ignoring server replaces the buffer); - a 416 whose Content-Range total matches the bytes already received means the download completed and only the terminating chunks went missing, so the response is assembled from what we have; - servers without Accept-Ranges: bytes keep today's full-restart behaviour, and streamed responses are untouched. Fixes openai#2959
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 0f1f691280
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if resuming and response.status_code == 206: | ||
| # partial content; append the missing tail to what we already have | ||
| accumulate = True |
There was a problem hiding this comment.
When the resource changes between the interrupted attempt and the retry, this accepts any 206 and appends it without checking the prior/current ETag or that Content-Range begins at len(partial). A same-sized replacement therefore produces a successful 200 containing the old prefix and new suffix; the 416 total check does not protect this case. Preserve a strong validator and use If-Range, and validate the returned range before appending.
Useful? React with 👍 / 👎.
| for chunk in response.iter_bytes(): | ||
| partial.extend(chunk) |
There was a problem hiding this comment.
When an origin returns Content-Encoding: gzip or br together with Accept-Ranges, iter_bytes() yields decoded bytes, so len(partial) is not an offset in the encoded representation to which HTTP byte ranges apply. The retry consequently requests the wrong offset, and an encoded slice generally cannot be decoded independently, leaving compressed large downloads unrecoverable or incorrectly assembled. Force identity encoding for resumable requests or accumulate raw representation bytes and decode only after reassembly; the async aiter_bytes() path has the same issue.
Useful? React with 👍 / 👎.
|
|
||
| for chunk in response.iter_bytes(): | ||
| partial.extend(chunk) | ||
| return _reassembled_download_response(request, response, bytes(partial)) |
There was a problem hiding this comment.
On every successful non-streamed GET advertising byte ranges, including transfers that never fail, bytes(partial) copies the entire accumulated bytearray, while partial_download remains referenced until request() finishes response processing. For the targeted 200–300 MB downloads this retains roughly two complete body buffers and can cause avoidable OOMs; release or transfer the mutable buffer promptly and avoid the full-size copy on the normal success path.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
| if resumable_attempt: | ||
| response = self._read_resumable_body(request, response, partial_download) |
There was a problem hiding this comment.
Because resumable attempts now call the HTTP client with stream=True, a timeout or protocol error raised while _read_resumable_body() iterates the body occurs after Client.send() has returned and therefore bypasses the client's exception cleanup. HTTPX leaves both the response and its stream open when iter_bytes() raises, and these exception handlers retry or raise without closing response, so repeated interrupted downloads can leak transport resources; close/aclose the failed response before retrying or propagating. The async path has the same problem.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
| if total is not None and total == len(partial): | ||
| # the earlier attempt already received every byte; only the | ||
| # terminating chunks went missing | ||
| return _reassembled_download_response(request, response, bytes(partial)) |
There was a problem hiding this comment.
When the first attempt delivered every byte and the retry receives 416, this rebuilds the successful response from the 416 response headers rather than the original representation headers. A normal 416 need only provide Content-Range, so the returned full body can lose Content-Type, Content-Disposition, ETag, and other metadata that callers of files.content() previously received. Retain the initial response headers and use them for this completion path.
Useful? React with 👍 / 👎.
| return httpx2.Response( | ||
| 200, |
There was a problem hiding this comment.
When a caller uses the supported legacy httpx.Client, the response passed here is a legacy httpx.Response, but reconstruction always creates an httpx2.Response. Thus a range-capable GET requested with cast_to=httpx.Response returns an object of the wrong runtime class after this change, despite the base client explicitly accepting both response types. Construct the replacement using the incoming response's implementation or otherwise preserve the configured client's response type.
Useful? React with 👍 / 👎.
| 200, | ||
| headers=headers, | ||
| content=content, | ||
| request=request, |
There was a problem hiding this comment.
For a range-capable GET that follows a redirect, response.request identifies the final redirected request, but reconstruction replaces it with the originally built request passed into this helper. The returned raw/binary response therefore reports the wrong URL and request headers, and its request is inconsistent with the preserved redirect history. Use response.request when constructing the assembled response; this also preserves request replacements made by authentication flows.
Useful? React with 👍 / 👎.
…anup Addresses the Codex review findings on the resume logic: - send If-Range with the interrupted response's strong validator (ETag, falling back to Last-Modified) so a changed resource is re-downloaded in full instead of being spliced from two different versions - only resume identity-encoded bodies: iter_bytes() yields decoded bytes while byte ranges address the encoded representation, so gzip/br downloads restart from scratch instead of resuming at a wrong offset - verify a 206's Content-Range starts at len(partial); a misaligned or missing range drops the partial state and restarts - close the response (aclose on the async path) when a body read fails mid-stream instead of leaking the connection across retries - rebuild the completed response from the interrupted attempt's headers and its final (post-redirect, post-auth) request, keeping the original Content-Type/Content-Disposition on the 416 completion path and the response class of whichever httpx implementation served the request - release the accumulated bytearray as soon as the body is reassembled so a 200-300 MB download doesn't hold two full buffers
|
Thanks for the thorough review — all seven findings are addressed in 222e2df:
Happy to iterate further if anything still looks off. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 222e2dfc24
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| validator = _resume_validator(representation) | ||
| if validator is not None: | ||
| request.headers["If-Range"] = validator |
There was a problem hiding this comment.
When the interrupted response has no strong validator—such as no ETag or the weak ETag explicitly covered by the new test—this still sends Range without If-Range and accepts an aligned 206. If the resource changes between attempts, the returned body silently combines the old prefix with the new suffix. Fresh evidence beyond the earlier comment is that the updated code makes the validator optional and test_weak_etag_is_not_used_as_validator confirms this unguarded path; restart from byte zero unless a genuinely strong validator is available.
Useful? React with 👍 / 👎.
| if not accumulate: | ||
| response.read() | ||
| return response |
There was a problem hiding this comment.
When a non-streamed GET does not advertise byte ranges, or uses non-identity encoding, resumable_attempt still sends it with stream=True and this unguarded response.read() can raise while leaving the response stream open. Fresh evidence beyond the earlier cleanup comment is that only the accumulating iter_bytes() branch was wrapped; this fallback and its async counterpart at line 1856 still bypass cleanup, so repeated interrupted ordinary or compressed GETs can leak transport resources. Wrap these reads with the same close-on-failure handling.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
| return response_cls( | ||
| 200, | ||
| headers=headers, | ||
| content=content, |
There was a problem hiding this comment.
For every successful 200 GET that advertises byte ranges—even when no retry occurs—this constructs a fresh response around a plain in-memory byte stream without copying or setting the original response's elapsed timing. Consequently, callers using a raw response or cast_to=httpx.Response now get a RuntimeError when accessing .elapsed, although the body has been fully read and closed. Preserve the served response's elapsed value on the replacement response.
Useful? React with 👍 / 👎.
| start = _range_start_bytes(response) | ||
| if start is not None and start == len(partial.data): | ||
| # partial content; append the missing tail to what we already have | ||
| accumulate = True |
There was a problem hiding this comment.
When an origin or intermediary returns a cleanly framed 206 whose Content-Range begins at the requested offset but ends before the advertised total, this start-only check accepts the segment and immediately finishes the download. Because the shorter segment can terminate normally with a matching Content-Length, no transport error triggers another retry, and the SDK returns a truncated body as a successful 200. Parse the range end and total and continue or reject unless the assembled bytes reach the complete representation length; the async branch has the same issue.
AGENTS.md reference: AGENTS.md:L114-L121
Useful? React with 👍 / 👎.
|
@codex address that feedback Please address all four findings from the latest review on |
|
Note To use Codex here, create an environment for this repo. |
…leteness Second round of Codex review hardening: - only resume when the interrupted response carries a strong validator (strong ETag or Last-Modified); without one the download restarts from byte zero instead of risking a silent splice of two resource versions - accept a 206 only when its Content-Range covers the representation total: a satisfying-but-short segment keeps its bytes and the next attempt resumes from the new offset, so intermediaries that truncate ranged responses can no longer produce a truncated 'successful' body - wrap the non-accumulating response.read()/aread() fallback with the same close-on-failure handling as the streaming path - copy the serving response's elapsed timing onto the rebuilt response so raw-response callers keep a working .elapsed
|
Thanks — the second round is addressed in a39c2e6:
One note on the strict validator gate: origins that serve |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a39c2e6758
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| last_modified = str(response.headers.get("last-modified", "")).strip() | ||
| return last_modified or None |
There was a problem hiding this comment.
When a range-capable response lacks a strong ETag but includes an ordinary Last-Modified, this treats the date as strong without checking whether it meets the strong-validator timing requirements. If the resource changes within the header's timestamp granularity, the retry's If-Range can still receive a 206, causing the old prefix and new suffix to be silently combined. Fresh evidence beyond the earlier validator comment is that _resume_validator() now unconditionally falls back to any nonempty Last-Modified; restart unless the date can actually be established as strong, or require a strong ETag.
Useful? React with 👍 / 👎.
| bounds = _content_range_bounds(response) | ||
| if bounds is not None and resuming and bounds[2] is not None and bounds[2] != len(partial.data): |
There was a problem hiding this comment.
When a resumed server returns a valid bounded segment such as Content-Range: bytes 4096-4999/*, bounds[2] is None, so this completion check is skipped and the helper immediately rebuilds a successful 200 containing only the prefix plus that segment even though more representation bytes may remain. Fresh evidence beyond the earlier segment-completeness comment is that the updated parser explicitly supports *, but only known totals trigger another retry. Continue from the parsed end or restart rather than treating an unknown total as complete; the async branch has the same defect.
AGENTS.md reference: AGENTS.md:L114-L121
Useful? React with 👍 / 👎.
| except Exception: | ||
| # the stream is left open when the body read fails; release the | ||
| # connection before the retry loop takes over | ||
| await response.aclose() |
There was a problem hiding this comment.
When a caller cancels a task while aread() or aiter_bytes() is awaiting the next chunk, the cancellation exception inherits from BaseException, so these except Exception cleanup handlers are bypassed. Because resumable GETs are now sent with stream=True, the HTTP client no longer performs the normal non-streamed read cleanup, leaving the response and transport connection open after cancellation; catch cancellation in a BaseException/finally cleanup path (shielding aclose() as needed). The fallback read at line 1907 has the same defect.
AGENTS.md reference: AGENTS.md:L118-L119
Useful? React with 👍 / 👎.
| value = str(response.headers.get("content-range", "")) | ||
| if not value.startswith("bytes "): | ||
| return None |
There was a problem hiding this comment.
When a compliant origin serializes the range unit with different casing, such as Content-Range: Bytes 4096-10239/10240, this exact lowercase prefix check rejects the successful resumed response even though HTTP range-unit names are case-insensitive. In the deterministic-cutoff scenario this discards the received prefix, forces the next attempt to restart in full, and can exhaust all retries despite the origin serving the requested remainder correctly. Normalize the range unit before parsing; _range_total_bytes() has the same issue for 416 responses, and both async paths share these helpers.
Useful? React with 👍 / 👎.
Third round of review hardening: - only a strong ETag qualifies as a resume validator; a Last-Modified date can hide sub-second changes and is no longer used, so responses without a strong ETag restart instead of resuming - a 206 whose Content-Range reports an unknown total (bytes x-y/*) can never prove completeness: the partial state is dropped and the next attempt downloads the full body instead of returning a possibly truncated one - async read cleanup now catches BaseException so a cancelled task still closes the streamed response (CancelledError bypasses 'except Exception'); the sync paths treat KeyboardInterrupt the same - Content-Range and the 416 total parser normalise the range unit case-insensitively, per RFC 9110 range-unit comparison
|
Third round addressed in 6d469e3:
Full local suite for the touched areas: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d469e3c6e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| content = bytes(partial.data) | ||
| partial.clear() # release the mutable buffer before response processing |
There was a problem hiding this comment.
For the targeted 200–300 MB range-capable downloads, bytes(partial.data) allocates a second full-size buffer while the original bytearray is still live, so peak memory remains roughly twice the payload and can still trigger the OOM described by the earlier finding. Fresh evidence is that the new partial.clear() occurs only after this allocation; use buffering that can be transferred or spooled without a full-size duplicate.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
| except BaseException: | ||
| # the stream is left open when the body read fails or the task is | ||
| # cancelled; release the connection before propagating | ||
| await response.aclose() | ||
| raise |
There was a problem hiding this comment.
When a body read is cancelled inside an AnyIO or Trio cancel scope, catching BaseException does not make this cleanup reliable: level-triggered cancellation can immediately interrupt await response.aclose(), leaving the transport stream open. Fresh evidence beyond the earlier cancellation comment is that the new handler awaits aclose() without shielding it, while the added test merely raises asyncio.CancelledError from the stream and therefore has no active cancellation scope.
AGENTS.md reference: AGENTS.md:L118-L119
Useful? React with 👍 / 👎.
| rebuilt = response_cls( | ||
| 200, | ||
| headers=headers, | ||
| content=content, |
There was a problem hiding this comment.
When a supported custom HTTP client uses a non-UTF-8 default_encoding, a successful range-capable text response without a charset is reconstructed directly with the response class's UTF-8 default rather than the encoding attached by the client; an encoding selected by a response hook is similarly lost. Consequently, raw-response or cast_to=httpx.Response callers can now get incorrectly decoded .text, so copy the served response's encoding state into the rebuilt response.
Useful? React with 👍 / 👎.
| base_url = "http://localhost:7123/v1" | ||
| api_key = "test-api-key" | ||
|
|
||
| DATA = b"".join(bytes([i % 256]) * 64 for i in range(160)) # 10240 bytes |
There was a problem hiding this comment.
The shared fixture is only 10 KB, so these tests do not exercise the 200–300 MB download behavior this change targets and cannot detect payload-size-dependent memory regressions such as excessive buffering. The repository guidance explicitly requires large synthetic in-memory public-entrypoint probes and says their high memory use must not be reduced; add sequential large sync/async coverage rather than relying solely on this miniature fixture.
AGENTS.md reference: AGENTS.md:L123-L128
Useful? React with 👍 / 👎.
Fourth round of review hardening: - async cleanup now closes the response inside an anyio shielded cancel scope, so a level-triggered cancellation (anyio/trio scopes, repeated cancels) cannot interrupt aclose itself; a real asyncio task cancellation test replaces the manual CancelledError raise - the rebuilt response copies default_encoding and the effective encoding from the interrupted response, keeping .text identical for clients configured with a non-UTF-8 default_encoding or hook-set encoding - the accumulated buffer is handed off rather than emptied in place, dropping the last reference to the mutable copy as soon as the immutable content exists (peak memory matches the stock Response.read() join, which also transiently holds two copies) - large-payload regression probes per the repository's large-payload contract: a 32 MiB+ body is generated in memory, interrupted mid-transfer, and resumed through the public files.content() entrypoint, sync and async, sequentially in one test
|
Fourth round addressed in ce670da:
Local verification: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce670da94d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for chunk in response.iter_bytes(): | ||
| partial.data.extend(chunk) |
There was a problem hiding this comment.
When a caller's custom HTTP client has a response event hook that retains the response for post-request inspection, iter_bytes() consumes and closes that object without assigning its .content, while _finish_download() returns a different response. Before this change, the non-streaming send() populated the same hook-visible response before returning; now any successful validator-bearing, range-capable GET leaves the retained response raising ResponseNotRead for .content or .text. Preserve the original response identity or populate its content during reassembly; the async path has the same regression.
Useful? React with 👍 / 👎.
| # the request as it was actually sent (post-redirect, post-auth) | ||
| request=representation.request, |
There was a problem hiding this comment.
When a download actually resumes, representation still refers to the initial interrupted 200, so the rebuilt raw response reports that attempt's request rather than the final 206 request that completed the transfer; callers consequently see headers without Range, and rotating redirects can leave the response reporting the wrong final URL and history. Fresh evidence beyond the earlier comment is that the updated helper receives the final response only through elapsed_from, while this line still unconditionally selects representation.request; pass the completing response's request metadata as well.
Useful? React with 👍 / 👎.
| def _accepts_byte_ranges(response: httpx2.Response) -> bool: | ||
| # https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Accept-Ranges | ||
| value = str(response.headers.get("accept-ranges", "")) | ||
| return value.strip().lower() == "bytes" |
There was a problem hiding this comment.
When an origin advertises more than one supported range unit, such as Accept-Ranges: bytes, example-unit, this exact equality check rejects byte-range support even though the header's standard list syntax permits that value. A deterministically interrupted download from such an origin therefore restarts from byte zero on every attempt and can exhaust all retries despite the server supporting the requested byte ranges. Parse the comma-separated range units and test whether bytes is present; the shared helper affects both sync and async requests.
Useful? React with 👍 / 👎.
…ability Fifth round of review hardening: - Accept-Ranges is parsed as a comma-separated unit list, so origins advertising 'bytes, other-unit' are recognised as byte-range capable - the reassembled response now carries the request, extensions and redirect history of the attempt that completed the transfer (the resumed ranged request), keeping headers/encoding from the original representation; the 416-completion path does the same - the consumed completing response gets its content populated, so callers that retained it (e.g. via a response event hook) can still read .content/.text instead of hitting ResponseNotRead
|
Fifth round addressed in 8e3952e:
Local verification: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e3952e9a5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # the consumed response stays reachable to callers that retained it (for | ||
| # example through a response event hook), so populate its content too | ||
| # instead of leaving it unreadable | ||
| elapsed_from._content = content |
There was a problem hiding this comment.
When a response hook retains the completing 206 response (or the 416 used for end-of-body completion), this assignment replaces its content with the entire assembled representation but leaves its status and headers unchanged. After the request finishes, the retained response can therefore report 206 with a Content-Range and suffix-sized Content-Length while exposing the full body, which can break hook-based caching, metrics, or post-request inspection; either normalize that response to the rebuilt 200 metadata or avoid attaching mismatched content.
Useful? React with 👍 / 👎.
| rebuilt.default_encoding = representation.default_encoding | ||
| effective_encoding = representation.encoding | ||
| if effective_encoding is not None: | ||
| rebuilt.encoding = effective_encoding |
There was a problem hiding this comment.
When an actually resumed response has no charset and the custom HTTP client uses HTTPX's supported callable default_encoding, representation is the initial interrupted response and still has no _content; resolving representation.encoding therefore falls back to UTF-8 rather than invoking the detector, and that fallback is then forced onto the rebuilt response. Fresh evidence in the updated encoding fix is that it reads the effective encoding from the unread interrupted response, so non-UTF-8 raw responses still decode incorrectly via .text; preserve an explicitly set encoding only, otherwise let the callable run against the assembled content.
Useful? React with 👍 / 👎.
Sixth round of review hardening: - the consumed response is left with exactly the body its own exchange delivered (a 206 keeps its suffix-sized body with matching status and Content-Range), matching what a non-streaming send() would have left for a hook-retained reference, instead of the previously attached assembled body that disagreed with its metadata; the 416 completion path now reads its small body for the same parity - only an encoding pinned explicitly on the interrupted response is copied verbatim; a client-level default_encoding (including a callable detector) is carried over and left to resolve against the assembled body, so detectors see the full content instead of a UTF-8 fallback from the unread interrupted response
|
Sixth round addressed in 81cebf8:
Local verification: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81cebf8e70
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bounds = _content_range_bounds(response) | ||
| if resuming and bounds is not None and (bounds[2] is None or bounds[2] != len(partial.data)): |
There was a problem hiding this comment.
When a resumed response claims Content-Range: bytes 4096-8191/10240 but delivers 6144 bytes, the assembled length equals the advertised total, so this condition accepts the response even though the body length contradicts the claimed range end. Fresh evidence beyond the earlier completeness comment is that the new condition compares only the total with the assembled length while the parsed end remains unused. Validate that end >= start, that the delivered length is exactly end - start + 1, and that a completed open-ended request reaches total - 1; the async branch has the same issue.
Useful? React with 👍 / 👎.
| if resumable_attempt and partial_download: | ||
| log.debug("Resuming interrupted download from byte %i", len(partial_download.data)) | ||
| request.headers["Range"] = f"bytes={len(partial_download.data)}-" |
There was a problem hiding this comment.
When a provider or subclass uses _prepare_request to sign or otherwise finalize the complete request headers, a resumed attempt runs that hook first and only then appends Range and If-Range. The resulting authorization can be stale or omit the body-selecting headers, causing resumed downloads to fail against verifiers that canonicalize the received header set. Build the resume headers before _prepare_request, or rerun preparation after adding them; the async ordering has the same defect and needs corresponding authentication coverage.
AGENTS.md reference: AGENTS.md:L41-L45
Useful? React with 👍 / 👎.
| response = self._send_request( | ||
| request, | ||
| stream=stream or self._should_stream_response_body(request=request), | ||
| stream=stream or self._should_stream_response_body(request=request) or resumable_attempt, | ||
| **kwargs, |
There was a problem hiding this comment.
When a configured provider's normalize_response callback inspects .content, .text, or .json() for a non-streamed GET, passing stream=True here makes the callback receive an unread response and HTTPX raises ResponseNotRead; previously the underlying send completed the body read before _send_request invoked normalization. _read_resumable_body runs only after that callback returns, so it cannot repair the regression, and a callback exception can also leave the streamed response open. Arrange for resumable reading to occur before provider normalization while retaining normalization before status handling; the async callback path has the same issue.
Useful? React with 👍 / 👎.
…zation Seventh round of review hardening: - Range and If-Range are added before _prepare_request runs, so request signing or header finalisation always covers the final header set - a 206 whose delivered body length disagrees with its Content-Range extent is rejected and the download restarts from scratch - resumable bodies are consumed inside _send_request via a body_reader hook before provider response normalisation, restoring the non-streaming contract where normalize_response sees a read response (the reassembled response the pipeline consumes); the OpenAI client overrides pass the hook through on both sync and async paths
|
Seventh round addressed in 124bbef:
Local verification: |
|
Hi maintainers 👋 — a polite check-in on this PR, which fixes #2959 (large Batch API result file downloads dying mid-transfer, because retries restart the download from byte 0 and can never get past a deterministic cutoff). The implementation is complete with sync/async coverage and a dedicated test suite, and every finding from the seven automated review rounds has been addressed. Two things I'd be grateful for:
No rush at all — happy to adapt to any feedback. Thanks for taking a look! |
Follows the shape of openai#3712/openai#3713 (polling helpers in SDK-owned files): the byte-range resume helpers, state object, and body readers now live in the handwritten src/openai/lib/_resumable.py, leaving only the retry-loop integration (~120 lines) in the template-owned _base_client.py. No behavior change; the tests are unchanged apart from imports.
|
Structural update (7c9daf1): following the SDK-owned-helper shape from #3712/#3713, the resume helpers, partial-download state, and body readers moved into the handwritten |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c9daf1263
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for chunk in response.iter_bytes(): | ||
| partial.data.extend(chunk) | ||
| delivered.extend(chunk) |
There was a problem hiding this comment.
For a successful validator-bearing, range-capable 200 response, every chunk is accumulated in both partial.data and delivered; afterward bytes(delivered) creates a third full-body buffer and finish_download() creates a fourth from partial.data while the first three remain live. Fresh evidence beyond the earlier memory findings is this new hook-visibility buffer, which raises peak memory to roughly four times the payload even when no retry occurs, so a targeted 200–300 MB download can transiently consume over 1 GB and be killed; preserve hook readability without duplicating the entire body or use transferable/spooled buffering.
AGENTS.md reference: AGENTS.md:L114-L119
Useful? React with 👍 / 👎.
| headers = [ | ||
| (key, value) | ||
| for key, value in representation.headers.raw | ||
| if key.decode("latin-1").lower() not in ("content-range", "content-length", "transfer-encoding") |
There was a problem hiding this comment.
When a resumed request completes with a different x-request-id, the rebuilt response copies all headers from the initial interrupted response even though its request, history, extensions, and elapsed time come from the completing attempt. Consequently APIResponse.request_id, raw-response callers, and the request-ID debug log report the failed first attempt's ID rather than the final exchange, which can send support and tracing investigations to the wrong request; copy attempt-scoped headers such as x-request-id from elapsed_from while retaining representation metadata from the original response.
Useful? React with 👍 / 👎.
|
Hi — a closing note from the author, for whenever a human has a moment. First, some honesty about the noise below: nearly all the review activity on this PR has been the automated Codex reviewer, and my replies to it were correspondingly mechanical. That history is what it is; the code underneath is stable, and apart from one memory-footprint commit I'll push tomorrow (already written and verified locally — it collapses the accumulation buffer so peak memory matches the stock What the change is, plainly: retries for large non-streamed GET downloads currently restart from byte 0, so a transfer that dies at a fixed size or duration threshold can never finish — that's #2959. This PR makes retries resume instead, sending The complete diff accounting (current head
Commit-by-commit (10)
Three questions I'd genuinely value answers to, whichever way they go:
After tomorrow's commit I'll go quiet. Thanks for reading this far. |
The interrupted-then-resumed flow accumulated every chunk twice — once in the shared partial buffer and once in a per-response `delivered` buffer — and then copied each again (`bytes(delivered)` for the response body and `bytes(buffer)` for the reassembled one), holding up to four full-body copies of a 200-300 MB Batch API result at once. Chunks now accumulate exactly once, into the partial buffer. The response body is assigned after assembly: a fresh 200 restart shares the assembled body zero-copy, and a 206 keeps a suffix-sized copy of only the bytes it delivered. Peak memory for a successful range-capable download drops from ~4.2x to ~2.1x the body size (measured with tracemalloc), matching what a plain `Response.read()` holds. Also: the reassembled response now reports the completing attempt's `x-request-id`, so the header agrees with the request, extensions, and elapsed timing already taken from that attempt.
|
Both findings from the last review round are addressed in 836e3a4.
No behaviour change otherwise — all 31 tests in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 836e3a4ac2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| headers.extend( | ||
| (key, value) for key, value in elapsed_from.headers.raw if key.decode("latin-1").lower() == "x-request-id" | ||
| ) |
There was a problem hiding this comment.
When an interrupted GET resumes, the completing request can return updated x-ratelimit-* or retry-after values, but the rebuilt response retains those headers from the first, failed exchange. Raw-response callers therefore see stale quota state and may issue requests based on an incorrect remaining limit. Fresh evidence beyond the earlier request-ID finding is that this code now overlays only x-request-id; preserve other attempt-scoped response headers from elapsed_from as well.
Useful? React with 👍 / 👎.
1 participant
Maintainer summary (30s TL;DR)
Non-streamed
GETfile downloads (e.g.client.files.content()) now resume automatically with byte-range requests when the origin supports it, fixing #2959. Servers withoutAccept-Ranges: bytes, streamed responses, and non-GET requests keep today's behavior byte-for-byte. Seven rounds of automated review drove the edge cases below; each is covered by a focused test:ETag(If-Range); weak ETags /Last-Modified/ no validator restart from byte 0test_resume_sends_if_range_with_strong_etag,test_weak_etag_is_not_used_as_validator,test_last_modified_alone_is_not_a_validatorContent-Encoding: identityresumes; gzip/br restart cleanlytest_encoded_bodies_are_not_resumed416whoseContent-Rangetotal matches the received bytes completes without a re-downloadtest_completes_download_that_was_cut_at_the_very_end206send - start + 1; short segments resume from the new offset; unknown*totals restarttest_short_partial_segments_resume_until_complete,test_content_range_body_mismatch_restarts206not starting at the requested offset restarts;Accept-Ranges: bytes, other-unitis recognizedtest_restarts_when_content_range_does_not_match_offset,test_accept_ranges_with_multiple_unitsaclose()underanyio.CancelScope(shield=True); failed streams are closed before retryingtest_real_task_cancellation_closes_the_responsebody_readerbeforenormalize_response; retained responses keep their own exchange's bodytest_provider_normalization_sees_read_response,test_completing_response_stays_readabletest_large_resumable_downloadAs noted in the comments, I'm happy to move the helpers out of
_base_client.pyinto SDK-owned files underlib/(cf. #3712/#3713) or otherwise reshape the patch — whatever minimizes template intrusion.Fixes #2959
Problem
Downloading large Batch API result files (the report is about >200–300 MB
.jsonloutputs) fails withhttpcore.RemoteProtocolError: peer closed connection without sending complete message body. The cutoff reproduces with plainrequestsagainst the same URL, so it is the long-lived single connection that dies, not the library — but the SDK's retries currently make the situation impossible to recover from:client.files.content()reads the body inside the retry loop, and each retry restarts the download from byte 0. When a transfer is cut at a deterministic size/duration threshold, every retry attempt dies at (roughly) the same point andmax_retriesjust repeats the failure. The reporter's workaround — a manual chunkedRangedownload with backoff — confirms the origin serves byte ranges happily.The previous attempt in #2985 looped
iter_bytes()in 1 MB chunks insideLegacyAPIResponse.content, which doesn't change the network behaviour (httpx already reads incrementally) and broke re-accessing.contenton a consumed response.Change
GET retries in
_base_client.py(sync + async) now resume an interrupted response body instead of restarting it, when the server allows it:iter_bytes()so whatever arrives before a transport error is kept;Range: bytes=<received>-; a206appends the missing tail to the partial body, a200from a range-ignoring server replaces the buffer;416whoseContent-Range: bytes */<total>total equals the bytes already received means the previous attempt actually received everything and only the terminating chunks went missing — the response is assembled from what we have;416(mismatched total, e.g. the object changed between attempts) clears the partial buffer and raises aReadErrorso the normal retry machinery restarts from scratch;content-range/content-length, which is re-derived), status200.Scope is deliberately conservative:
Accept-Ranges: bytes; JSON endpoints and every other origin keep the current full-restart behaviour (verified bytest_restarts_from_scratch_when_server_has_no_rangesandtest_plain_get_is_unaffected);stream=True,with_streaming_response) are untouched;Tests
tests/test_resumable_downloads.pyuses a stateful mock transport whose first response dies mid-body, covering sync + async resume, no-accept-rangesfallback, range-ignored servers, the cut-at-the-very-end416case, exhausted retries, and the untouched plain-JSON GET path.ruff check,ruff format, andmypypass on the changed files;tests/test_client.py,tests/test_httpx2.py,tests/test_legacy_response.py, andtests/test_resumable_downloads.pypass together (235 passed, 2 skipped).Notes for reviewers
_base_client.py, so it will show up in the custom-code report; happy to restructure (e.g. move the helpers into a separate handwritten module) if that helps the budget, or to coordinate a template-level change if cross-SDK consistency is preferred.Range, and only for GETs that already received anAccept-Ranges: bytesresponse from the same origin.