Support resuming from binary caches that don't support ranged requests#445
Merged
Support resuming from binary caches that don't support ranged requests#445
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds per-response state to TransferItem (requestRange flag, bytesReceived counter), resets per-response fields on new HTTP responses/requests, updates sink handling to discard duplicate data across retries, tightens retry gating for range-resume and compressed responses, and conditions transparent decompression and CURLOPT_RESUME_FROM_LARGE on requestRange. ChangesTransfer/resume / per-response state
Sequence Diagram(s)sequenceDiagram
participant Client
participant TransferItem
participant Server
participant Sink
Client->>TransferItem: start request (may set requestRange)
TransferItem->>Server: send HTTP request (with Range if requestRange)
Server-->>TransferItem: HTTP response (status, headers, body)
TransferItem->>TransferItem: parse status line -> reset bytesReceived, acceptRanges, hasContentEncoding
alt hasContentEncoding && dataCallback active
TransferItem->>Client: block retries (no retry)
end
TransferItem->>Sink: finalSink writes bytes
Sink-->>TransferItem: writtenToSink update
TransferItem->>TransferItem: bytesReceived += bytesWritten
alt error / retry needed
TransferItem->>TransferItem: if writtenToSink>0 and acceptRanges set requestRange=true
TransferItem->>Server: retry (with Range if requestRange)
Note right of Server: New response may overlap previous written bytes
TransferItem->>TransferItem: discard/trim duplicate bytes based on bytesReceived vs writtenToSink
TransferItem->>Sink: continue writing remaining bytes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
📝 WalkthroughWalkthroughThe file transfer implementation now tracks bytes received across retry attempts and introduces a request range flag. It prevents duplicate data writing by comparing previous and current received bytes, resets counters on new HTTP responses, and ties decompression behavior to range-request state rather than first-attempt checks. Retry eligibility is tightened to prevent retries when dataCallback meets non-identity Content-Encoding, and range requests are only set on retry if supported. ChangesHTTP Range Request and Deduplication Logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/5 reviews remaining, refill in 35 minutes and 36 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libstore/filetransfer.cc`:
- Around line 185-194: The duplicate-trimming logic incorrectly compares the
response-local bytesReceived (which resets on a resumed 206 response) against
the global writtenToSink, causing valid resumed bytes to be dropped; change the
comparison to work in absolute file-offset space by adding the response's
starting offset to prevReceived (i.e., compute absolutePrev = responseStart +
prevReceived or use the existing resume/expected offset variable) and only trim
data when absolutePrev < writtenToSink, calculating the trim amount as
writtenToSink - absolutePrev and slicing data accordingly; update the block that
references bytesReceived, prevReceived, writtenToSink, and data to use this
absolute-offset logic so ranged (206) retries do not discard bytes.
- Around line 510-513: The condition that sets CURLOPT_ACCEPT_ENCODING is
inverted: currently it enables transparent decompression when requestRange is
true, but the comment and semantics require skipping decompression for range
requests; change the conditional that calls curl_easy_setopt(req,
CURLOPT_ACCEPT_ENCODING, "") to run only when requestRange is false and
request.data is false (i.e., use if (!requestRange && !request.data)) so
byte-range downloads are not decompressed; update the surrounding comment if
needed to reflect this behavior and ensure the check references the same
requestRange and request.data symbols used in the surrounding code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ed76e76-dde5-4967-ab79-cd73d33754ab
📒 Files selected for processing (1)
src/libstore/filetransfer.cc
edolstra
force-pushed
the
resume-unranged-downloads
branch
2 times, most recently
from
08118a0 to
dbd6993
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libstore/filetransfer.cc (1)
772-781:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDifferentiate real range resumes from restart-and-skip retries.
When
acceptRangesis false,requestRangestays false, but this still warns"retrying from offset ...". On the non-ranged-cache path, the next attempt restarts from byte 0 and only skips duplicates locally, so the message is misleading.💡 Proposed fix
if (writtenToSink) { if (acceptRanges) requestRange = true; - warn( - "%s; retrying from offset %d in %d ms (attempt %d/%d)", - exc.message(), - writtenToSink, - ms, - attempt, - fileTransfer.settings.tries); + if (requestRange) + warn( + "%s; retrying from offset %d in %d ms (attempt %d/%d)", + exc.message(), + writtenToSink, + ms, + attempt, + fileTransfer.settings.tries); + else + warn( + "%s; retrying in %d ms and skipping the first %d bytes locally (attempt %d/%d)", + exc.message(), + ms, + writtenToSink, + attempt, + fileTransfer.settings.tries); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libstore/filetransfer.cc` around lines 772 - 781, The warning message is misleading when acceptRanges is false because requestRange stays false but the code logs "retrying from offset X" even though the next attempt restarts from 0 and skips duplicates locally; update the branch that handles writtenToSink to check acceptRanges and emit two different warn messages: when acceptRanges is true keep the existing "retrying from offset %d ..." message and when acceptRanges is false log something like "retrying by restarting from the beginning and skipping %d bytes locally ..." (use writtenToSink and fileTransfer.settings.tries in the message), and ensure this logic is placed around the existing requestRange/acceptRanges handling so the correct message corresponds to requestRange being true or false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libstore/filetransfer.cc`:
- Line 139: The bytesReceived counter (curl_off_t bytesReceived) isn't reset on
each retry, so non-HTTP transfers reuse the previous attempt's value and can
re-send sunk bytes to dataCallback; fix this by resetting bytesReceived = 0 at
the start of every new download attempt (i.e., inside the retry/attempt loop or
immediately before the logic that handles each attempt) so both the
getHTTPStatus() != 0 and getHTTPStatus() == 0 code paths use a fresh counter;
reference the bytesReceived variable, getHTTPStatus(), and dataCallback to
locate where to add the reset.
---
Outside diff comments:
In `@src/libstore/filetransfer.cc`:
- Around line 772-781: The warning message is misleading when acceptRanges is
false because requestRange stays false but the code logs "retrying from offset
X" even though the next attempt restarts from 0 and skips duplicates locally;
update the branch that handles writtenToSink to check acceptRanges and emit two
different warn messages: when acceptRanges is true keep the existing "retrying
from offset %d ..." message and when acceptRanges is false log something like
"retrying by restarting from the beginning and skipping %d bytes locally ..."
(use writtenToSink and fileTransfer.settings.tries in the message), and ensure
this logic is placed around the existing requestRange/acceptRanges handling so
the correct message corresponds to requestRange being true or false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bac9bc7f-7016-41a9-b784-88112531127f
📒 Files selected for processing (1)
src/libstore/filetransfer.cc
We just start over and discard the data we've already received.
edolstra
force-pushed
the
resume-unranged-downloads
branch
from
dbd6993 to
6839d85
Compare
cole-h
approved these changes
May 6, 2026
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
2 participants
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.
Motivation
We just start over and discard the data we've already received.
Context
This fixes resuming NAR downloads from binary caches that don't support ranged requests.
Summary by CodeRabbit