[api] Resume truncated HTTP GET response bodies - #9271
Conversation
JingsongLi
left a comment
There was a problem hiding this comment.
Production-focused review. I found one data-integrity regression and two compatibility/protocol issues; details are inline.
|
|
||
| InputStream newStream = entity.getContent(); | ||
| verifyReplayedPrefix(newStream, expectedPrefixDigest); | ||
| if (contentLength < 0 && replayedLength >= 0) { |
There was a problem hiding this comment.
[P1] Do not retain the initial Content-Length when the replay length is unknown.
If the initial response declares length L and truncates after a prefix, while the replay is a longer chunked 200, then replayedLength == -1 and this condition leaves the old contentLength unchanged. currentResponseEndExclusive is consequently set to L, so read() returns EOF at L without checking whether the replay response actually ended. The BLOB writer can then commit truncated data instead of failing. I reproduced this with a 100-byte initial response and a 120-byte chunked replay; the stream returned only 100 bytes.
After the prefix has been verified, derive the boundary solely from the replay response, including resetting it to unknown and reading that response to its real EOF, or fail closed.
There was a problem hiding this comment.
Thanks for catching this. Fixed in 64a5ffe: after the replayed prefix is verified, the replay response now becomes authoritative for both contentLength and currentResponseEndExclusive. An unknown-length chunked replay is therefore read to its actual EOF instead of retaining the initial response's stale length. I added testGetAsInputStreamReadsChunkedReplayPastInitialContentLength, which reproduces the 100-byte initial / 120-byte chunked replay case and asserts the full 120 bytes.
| CloseableHttpResponse newResponse = execute(request, uri); | ||
| boolean accepted = false; | ||
| try { | ||
| if (newResponse.getCode() != HttpStatus.SC_OK) { |
There was a problem hiding this comment.
[P2] Preserve compatibility with encoded-only origins.
newBodyGet forces Accept-Encoding: identity, but this status check rejects a legitimate 406 before openContentDecodedResponse() can run. An origin that accepts a normal gzip request but cannot serve identity content worked with the previous transparent-decompression path and now always fails. If the origin ignores identity and returns gzip, the current fallback also discards an already successful response and performs a second GET, which adds request load and can break one-shot download URLs.
Please fall back to the ordinary decoded request when identity negotiation is rejected, or decode the first accepted encoded response and mark that stream as non-resumable.
There was a problem hiding this comment.
Good catch. Fixed in 64a5ffe: transparent decompression remains enabled on the identity-preferred request, so a successful gzip/deflate response is decoded and consumed from that same response and is marked non-resumable; no second GET is issued. If identity negotiation returns HTTP 406, the rejected response is closed and one ordinary decoded GET is attempted. I added one-shot gzip, 406 fallback, and truncated-encoded fail-closed regression tests.
| } | ||
| } | ||
| Header lastModified = response.getFirstHeader(HttpHeaders.LAST_MODIFIED); | ||
| return lastModified == null ? null : lastModified.getValue(); |
There was a problem hiding this comment.
[P2] Only use Last-Modified as If-Range when it is demonstrably strong.
RFC 9110 section 13.1.5 says a client MUST NOT generate a date-valued If-Range unless that date is a strong validator under section 8.8.2.2. This code promotes every Last-Modified value without checking Date or the one-second strength criteria. A compliant server can therefore ignore the range and make recovery fail; a coarse timestamp implementation can accept a same-second update and let the stream splice two representations.
Use only a strong ETag for range continuation unless the date strength can be proven; otherwise use the full-replay and prefix-verification path.
There was a problem hiding this comment.
Agreed. Fixed in 64a5ffe: only a syntactically strong ETag is now eligible for If-Range. Last-Modified-only and weak-ETag responses use the full HTTP 200 replay plus SHA-256 prefix-verification path instead. The tests assert that neither Range nor If-Range is sent for those two cases, while the strong-ETag case still uses validated range continuation.
Purpose
Paimon BLOB descriptors can refer to HTTP resources. A batch job writing an HTTP-backed BLOB observed the following failure after the server had returned HTTP 200:
The response body was closed while it was being consumed, after 89,075 of 990,434 declared bytes had been returned. At that point the caller may already have copied the prefix into the BLOB output. Replaying from byte zero without verification could duplicate data, while swallowing the exception or changing the value to NULL would be unsafe after output has started.
HttpClientUtils#getAsInputStreampreviously executed the GET and returned the raw entity stream. The existing request retry strategy covers request execution and retryable statuses, but the entity is consumed later. AConnectionClosedException,TruncatedChunkException, or premature EOF before a knownContent-Lengthcan therefore occur after request execution has completed and after bytes have already been delivered.This PR wraps HTTP GET bodies in a bounded, integrity-checked resumable stream:
Range: bytes=<position>-withIf-Range, accepts only HTTP 206, and validates theContent-Rangestart and total length, the response length when known, identity encoding, and any returned ETag.Content-Length.This does not change
blob-write-null-on-fetch-failure, public APIs, table options, or the BLOB storage format. Recovery is limited to the two body-truncation exceptions above and premature EOF detectable from a known response length; unrelated body-read failures retain their existing behavior.Related work: #8412 handles configured NULL values for fetch/open failures, and #9181 reduces HTTP requests while writing BLOBs. This PR addresses the separate case where an accepted response body is truncated during lazy consumption.
Tests
Coverage includes strong-ETag Range/If-Range recovery, Last-Modified-only and weak-ETag full replay, no-validator replay and prefix verification, changed-prefix rejection, an unknown-length chunked replay longer than the initial declared length, multiple truncations, bounded ranges, ignored/mismatched ranges, attempt exhaustion with sticky failure, signed-URI sanitization, one-shot gzip decoding, HTTP 406 encoded-only fallback, truncated encoded-body fail-closed behavior, restart-status failure, and close semantics.
Checkstyle, Spotless, and
git diff --checkalso pass.