fix(druid): sniff HTML error body before JSON parse (second leg) - #20152
fix(druid): sniff HTML error body before JSON parse (second leg)#20152shoemoney wants to merge 3 commits into
Conversation
6a28d18 to
17b5863
Compare
|
Same as #20151 ? |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
Reviewed 1 of 1 changed files.
Validation: focused git diff --check passed. Builds and tests were not run.
This is an automated review by Codex GPT-5.6-Luna(max)
| preview = preview.substring(0, Math.min(preview.length(), 256)); | ||
| msg = StringUtils.format("%s: %s", msg, preview); | ||
| } | ||
| throw QueryCapacityExceededException.withErrorMessageAndResolvedHost(msg); |
There was a problem hiding this comment.
[P1] Handler exceptions complete the Netty future with null
With production NettyHttpClient, this throw occurs before the handler response is assigned. Netty then completes the future successfully with null and closes the channel, so JsonParserIterator treats the result as a possible scatter-gather limit and raises ResourceLimitExceededException. The intended capacity/interruption error is therefore lost. Propagate the exception through the future or return an error-bearing response, and test with the real Netty client.
| break; | ||
| } | ||
| } | ||
| if (statusCode == 429 || statusCode == 503) { |
There was a problem hiding this comment.
[P2] All 503 responses are mapped to capacity errors
This branch depends only on the status code, so a JSON 503 service-unavailable response is replaced with QueryCapacityExceededException, which maps to HTTP 429 and discards structured error details. Restrict 503 conversion to confirmed HTML/non-JSON responses or preserve the existing JSON error path; add 503 JSON and HTML coverage.
|
Done in d1f40ca: NettyHttpClient now completes the future with the thrown exception instead of null, and the 503-to-capacity-exceeded mapping in DirectDruidClient is now scoped to confirmed HTML/non-JSON bodies so a 503 with a proper JSON error body still takes the normal JSON error path. |
I think you can put these changes in one PR as these changes are similar and in same class. I'm worrying about repeated code in different paths |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
The updated implementation resolves the prior status/error-propagation concerns, but chunked responses can still bypass the new prefix check because the initial response body may be empty and later chunks are not classified with persistent state.
Reviewed 1 of 2 changed files with findings; all 2 changed files were reviewed.
Validation: focused git diff --check passed. Builds and tests were not run.
This is an automated review by Codex GPT-5.6-Luna(max)
| final ChannelBuffer contentBuffer = response.getContent(); | ||
| boolean isHtmlContentType = contentType != null && StringUtils.toLowerCase(contentType).contains("text/html"); | ||
| boolean isHtmlBody = false; | ||
| if (contentBuffer.readableBytes() > 0) { |
There was a problem hiding this comment.
[P2] Chunked HTML can bypass prefix sniffing
handleResponse inspects only the initial response buffer, but later chunked bytes reach handleChunk without persistent classification. A chunked 503 HTML response whose first buffer is empty or lacks the identifying prefix can therefore reach JsonParserIterator and fail as a JSON parse error instead of producing the intended capacity error. Carry the sniff state across chunks or buffer and classify the prefix before handing the stream to the parser.
Fix verified RED->GREEN. Broker masks 429/503 HTML as JsonParseException 0x3c second leg at DirectDruidClient.java:242 variant
… 503 capacity mapping to HTML bodies NettyHttpClient completed the future with null instead of the thrown exception when a response handler threw, discarding the intended QueryCapacityExceededException/QueryInterruptedException from the HTML sniff. DirectDruidClient also converted every 503 to a capacity error even when the body was a legitimate JSON error; that conversion is now scoped to confirmed HTML/non-JSON 503 bodies.
The previous commit scoped the 503-to-capacity-exceeded conversion to confirmed HTML/non-JSON bodies but left 429 unconditional. A 429 is also DruidException.Category.CAPACITY_EXCEEDED's real HTTP status, so a data server's genuine JSON CAPACITY_EXCEEDED error (e.g. failing to reserve a virtual-storage bundle) was being replaced with a generic templated message, losing the original error text. QueryVirtualStorageTest caught this in CI.
d1f40ca to
8b737a3
Compare
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 2 of 2 changed files.
Validation: focused git diff --check passed. Builds and tests were not run.
This is an automated review by Codex GPT-5.6-Luna(max)
| final ChannelBuffer contentBuffer = response.getContent(); | ||
| boolean isHtmlContentType = contentType != null && StringUtils.toLowerCase(contentType).contains("text/html"); | ||
| boolean isHtmlBody = false; | ||
| if (contentBuffer.readableBytes() > 0) { |
There was a problem hiding this comment.
[P2] Chunked HTML responses can bypass classification
When a chunked response has an empty initial buffer, this check leaves isHtmlBody false. Subsequent handleChunk calls only enqueue bytes and do not retain or inspect the response prefix, so a later chunk beginning with HTML is fed to JsonParserIterator and becomes a generic JSON parse/interruption error. Persist the classification across chunks or buffer the prefix before enqueueing, and add an empty-initial-chunk test.
2 participants
Broker masks 429/503 as HTML and DirectDruidClient throws JsonParseException 0x3c.
This is the second leg at DirectDruidClient.java handleResponse variant. Same fix as first leg but for the second code path that was missed.
Fix checks status code and Content-Type, sniffs body for leading < vs { or [, and throws QueryCapacityExceededException or QueryInterruptedException before JSON parse. Prevents HTML being fed to the JSON parser when the broker is throttling.
Test evidence: DirectDruidClientTest 12 tests passed RED and GREEN. Fix verified RED to GREEN, no failures, formatter kept to one file.