fix: honor MaxBatchURLs and handle unannounced batch response lengths - #2296
Open
0xrlawrence wants to merge 1 commit into
Open
fix: honor MaxBatchURLs and handle unannounced batch response lengths#22960xrlawrence wants to merge 1 commit into
0xrlawrence wants to merge 1 commit into
Conversation
parseBatchURLs appended before checking the limit, so an input with more than MaxBatchURLs entries returned MaxBatchURLs+1 URLs. Move the bound check to the top of the loop and add a table test covering the limit. getBatchFromDataService used resp.ContentLength directly as the read bound. A server that does not announce a length (chunked transfer encoding) reports -1, which made the LimitedReader budget 0 bytes and caused every such response to be rejected as oversized. Fall back to the configured MaxBatchSize in that case so the read stays bounded either way. A maxRetries below 1 would also skip the request loop entirely and leave resp nil for the deref that follows, so clamp it to at least one attempt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Description
Two independent correctness problems in the operator's batch download path, plus a small guard.
parseBatchURLsreturns one more URL thanMaxBatchURLs. The loop appends and then checkslen(urls) > MaxBatchURLs, so it breaks only after the sixth entry is already in the slice. WithMaxBatchURLs = 5, an input carrying seven URLs returns six. Verified against the current implementation:Moving the bound check to the top of the loop fixes it, and a table test now pins the limit.
Servers that do not announce a length are always rejected.
resp.ContentLengthis-1for chunked transfer encoding. That value passes the> MaxBatchSizecheck, then makes theio.LimitedReaderbudget-1 + 1 = 0bytes, soreader.N <= 0fires and the batch is refused as oversized. The read now falls back toMaxBatchSizewhen no length is announced, which keeps it bounded either way.maxRetries < 1would nil-deref. A non-positive retry count skips the request loop entirely, leavingrespnil for theresp.Bodydereference below. Clamped to at least one attempt.The first hunk also converts a stray space-indented comment to a tab so the file is
gofmt-clean.Type of change
Checklist
testnet, everything else tostagingVerified with
go vet ./operator/pkg/andgofmtonstaging. The package's own tests need the Rust FFI shared libraries to link, so theparseBatchURLsbehaviour above was confirmed by running the old and new implementations side by side.