fix(cli): prevent HTTP errors from corrupting resumed downloads - #1179
Merged
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
sridipbasu
force-pushed
the
fix/range-resume-http-error
branch
from
August 18, 2026 13:40
80f97f3 to
dec6ab5
Compare
Contributor
|
/gcbrun |
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.
Summary
While looking into the download resume flow, I found that an HTTP error during a
Rangerequest could end up overwriting the existing partial download.The resume logic was treating any response other than
200or206as the fallback case used for416 Range Not Satisfiable. So if the storage request returned something like403or5xx, the CLI would make another request without theRangeheader and then write that response to the file without checking its status.This could replace a valid partial file with the error response. The resume marker was also left behind, so a later run could treat the error response as a valid partial download and append the remaining bytes. Since the final file size could still be correct, the existing size check would not catch the corruption.
This is particularly relevant for long-running downloads where a resume or storage error is more likely to occur.
Fix
I restricted the full-download fallback to
416 Range Not Satisfiableand added status checking for the fallback request.For other HTTP errors such as
403,404, or5xx, the error is now raised instead of modifying the existing partial file.The existing
HTTPErrorhandling already retries the appropriate errors, so this keeps the change small and fits into the current download flow.I also added a regression test to make sure a failed range-resume request does not overwrite the existing partial file or leave it in a corrupted state.
Tests
Added a regression test covering a
403response during a range-resume request.Ran:
pytest tests/unit/test_download_resume.py -qpytest tests/unit -qblack --check .mypyThe regression test was also checked against the old implementation and failed before the fix, then passed with the fix.
All 1319 unit tests pass.
mypyreports the same 7 pre-existing errors as before, with no new errors from the changed code.I came across this while working with the RSNA Knee Abnormality Detection competition. The dataset is quite large and took a significant amount of time to download, which made me look more closely at how the Kaggle CLI handles interrupted and resumed downloads.
While going through that flow, I noticed this error-handling path and was able to reproduce the corruption locally.