hub: verify content hashes, harden download resume & accounting#19
Merged
Conversation
Correctness/integrity gaps found in a download-logic review: - Content integrity: LFS files (the weights) carry a sha256 in LFS.OID, but the downloader validated size only — a corrupt-but-right-length body, or corruption in a resumed prefix, was silently accepted. Now the finished .part is streamed through sha256 and compared to LFS.OID before it is renamed into place; a mismatch is rejected and removed. Non-LFS files carry a git-blob sha1 (not a content hash) and remain size-checked. - Resume: a 206 whose Content-Range does not start where we asked (a buggy proxy that returns the whole body as "partial") was blindly appended onto the .part. The offset/total are now validated; a wrong range discards the partial and restarts cleanly instead of corrupting or failing the file. - Empty/size-unknown: a size-0 manifest entry disabled every integrity check, so an empty or truncated file was renamed into place as "complete". A size-0 download that returns nothing is now rejected, and a size-0 existing file is re-fetched rather than adopted. - Duplicate tree entries are de-duplicated in WantedFiles, so a manifest listing the same path twice no longer spawns two goroutines racing on one file (and no longer double-counts it so progress can't reach 100%). - A leftover .part beside a completed file is now cleaned up; reported progress is clamped to [0, total] so a pathological pre-existing-file state can't show a negative or >100% bar. Tests cover hash match/mismatch, wrong-Content-Range restart, empty-file rejection, dedup, and validContentRange; each fails against the old code. Assisted-by: Claude:claude-fable-5
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.
Download-logic review follow-up (separate from the settings PR #18). All in
internal/hub, full suite green.Content integrity (the main one)
LFS files — the model weights — carry a sha256 in
LFS.OID, but the downloadervalidated size only. A corrupt-but-right-length body, or corruption in a
resumed prefix (the appended tail was size-checked, the prefix never was), was
silently accepted. The finished
.partis now streamed through sha256 andcompared to
LFS.OIDbefore rename; a mismatch is rejected and removed. Non-LFSfiles carry a git-blob sha1 (not a content hash) and stay size-checked.
Resume hardening
that returns the whole body as "partial") was blindly appended onto the
.part. The offset/total are now validated; a bad range discards the partialand restarts cleanly instead of corrupting or failing the download.
file was renamed in as "complete". A size-0 download returning nothing is now
rejected; a size-0 existing file is re-fetched, not adopted.
Accounting / robustness
WantedFiles— a manifest listingthe same path twice no longer spawns two goroutines racing on one file, nor
double-counts it (which kept progress under 100%).
.partbeside a completed file is cleaned up; reported progress isclamped to
[0, total]so a pathological pre-existing-file state can't render anegative or >100% bar.
Tests cover hash match/mismatch, wrong-Content-Range restart, empty-file
rejection, dedup, and
validContentRange— each fails against the old code.Assisted-by: Claude:claude-fable-5