[fix](s3) Keep the response stream usable when an error body overflows the read buffer - #66557
Open
liaoxin01 wants to merge 1 commit into
Open
[fix](s3) Keep the response stream usable when an error body overflows the read buffer#66557liaoxin01 wants to merge 1 commit into
liaoxin01 wants to merge 1 commit into
Conversation
liaoxin01
requested review from
Gabriel39,
gavinchou,
luwei16 and
morningman
as code owners
August 6, 2026 17:43
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
…s the read buffer
### What problem does this PR solve?
Problem Summary:
Reading from object storage fails from time to time with
```
[INTERNAL_ERROR]failed to read from <key>: Failed to flush response stream (eof: 0, bad: 1) code=-1 type=1, request_id=failed to read
```
and succeeds when the same statement is run again. It has been hit by queries
reading a rowset, by compaction, by an outfile export and by the download of an
inverted index, always on an object storage that was answering `429` or `503` at
that moment.
`S3ObjStorageClient::get_object()` hands the buffer of the caller to the SDK as
the response stream of the request, sized exactly like the requested range. The
SDK writes the body of every response into that stream, the body of an error
response included. The XML document of a `429 SlowDown` is a few hundred bytes,
so a small ranged read cannot hold it - the read of the footer of a packed file
asks for 12 bytes. `PreallocatedStreamBuf` does not implement `overflow()`, so
the stream turns bad, the write callback of curl reports a short write and curl
aborts the transfer with `CURLE_WRITE_ERROR`.
The status code of the response is lost from there on:
`CurlHttpClient::MakeRequest()` reads `CURLINFO_RESPONSE_CODE` only when curl
succeeded, so the code stays at `REQUEST_NOT_MADE` (-1), and the flush check at
the end of the same function replaces the retryable `NETWORK_CONNECTION`
classification with `INTERNAL_FAILURE` (1). `S3CustomRetryStrategy::ShouldRetry()`
declines to retry an error classified that way, and so does
`S3FileReader::read_at_impl()`, which retries on `429` alone. A throttling error
the server asked us to retry cancels the statement of the user instead, which is
why running it again works.
This also means the error carries no evidence of what really happened: the code
of the response, the exception name and the request id of the object storage are
all gone by the time the message is built.
The fix lets the response stream grow: the body is written into the buffer of the
caller as long as it fits, which is the case for every successful ranged read and
keeps that path free of copies, and the remainder spills into a buffer of the
stream itself, truncated at 1MB because only error documents are expected to
overflow. The stream never turns bad, so curl completes the transfer, the SDK
records the real status code and parses the error out of the body, and both the
retry of the SDK and the retry of `S3FileReader` on `429` work again.
A server or a proxy answering a ranged read with the whole object overflows the
buffer as well. Such a read is still rejected, by the length check that follows
the request, and now with a message that says so.
Two misleading messages are fixed along the way:
- `request_id=failed to read` is not a request id of the object storage. It is
the string `S3FileReader` appended behind the empty request id of a failure
raised by the client itself. The append is dropped and an empty request id is
printed as `<empty>`.
- The message of a failed read named neither the bucket nor the offset, leaving
`failed to read from :` in the log whenever the key was empty.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `be/test/io/fs/s3_response_stream_test.cpp` covers a body that fits, an
error body overflowing in one write, across writes and character by
character, the truncation of an oversized body, the rewind the SDK does
before parsing an error, and an empty body.
- Not tested end to end against a rate limited object storage.
- Behavior changed: No
- Does this need documentation: No
liaoxin01
force-pushed
the
fix-s3-response-stream-overflow
branch
from
August 7, 2026 00:47
aa1dfd1 to
2c2f27e
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29028 ms |
Contributor
TPC-DS: Total hot run time: 166759 ms |
Contributor
ClickBench: Total hot run time: 23.86 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
Reading from object storage fails from time to time with
and succeeds when the same statement is run again. It has been hit by queries
reading a rowset, by compaction, by an outfile export and by the download of an
inverted index, always on an object storage that was answering
429or503atthat moment.
S3ObjStorageClient::get_object()hands the buffer of the caller to the SDK asthe response stream of the request, sized exactly like the requested range. The
SDK writes the body of every response into that stream, the body of an error
response included. The XML document of a
429 SlowDownis a few hundred bytes,so a small ranged read cannot hold it - the read of the footer of a packed file
asks for 12 bytes.
PreallocatedStreamBufdoes not implementoverflow(), sothe stream turns bad, the write callback of curl reports a short write and curl
aborts the transfer with
CURLE_WRITE_ERROR.The status code of the response is lost from there on:
CurlHttpClient::MakeRequest()readsCURLINFO_RESPONSE_CODEonly when curlsucceeded, so the code stays at
REQUEST_NOT_MADE(-1), and the flush check atthe end of the same function replaces the retryable
NETWORK_CONNECTIONclassification with
INTERNAL_FAILURE(1).S3CustomRetryStrategy::ShouldRetry()declines to retry an error classified that way, and so does
S3FileReader::read_at_impl(), which retries on429alone. A throttling errorthe server asked us to retry cancels the statement of the user instead, which is
why running it again works.
This also means the error carries no evidence of what really happened: the code
of the response, the exception name and the request id of the object storage are
all gone by the time the message is built.
The fix lets the response stream grow: the body is written into the buffer of the
caller as long as it fits, which is the case for every successful ranged read and
keeps that path free of copies, and the remainder spills into a buffer of the
stream itself, truncated at 1MB because only error documents are expected to
overflow. The stream never turns bad, so curl completes the transfer, the SDK
records the real status code and parses the error out of the body, and both the
retry of the SDK and the retry of
S3FileReaderon429work again.A server or a proxy answering a ranged read with the whole object overflows the
buffer as well. Such a read is still rejected, by the length check that follows
the request, and now with a message that says so.
Two misleading messages are fixed along the way:
request_id=failed to readis not a request id of the object storage. It isthe string
S3FileReaderappended behind the empty request id of a failureraised by the client itself. The append is dropped and an empty request id is
printed as
<empty>.failed to read from :in the log whenever the key was empty.Release note
None
Check List (For Author)
be/test/io/fs/s3_response_stream_test.cppcovers a body that fits, anerror body overflowing in one write, across writes and character by
character, the truncation of an oversized body, the rewind the SDK does
before parsing an error, and an empty body.