download_manager: validate the .part BEFORE promoting -- a corrupt re-download must not replace an existing valid dest - #2830
Conversation
When a re-download of an already-installed model produced corrupt bytes (SHA mismatch), _download promoted the .part onto task.dest BEFORE validation, then deleted task.dest on mismatch -- destroying the existing valid file. The fix validates the .part first and only promotes on success. On mismatch the .part alone is removed and task.dest is left untouched. _validate_download now takes a path parameter so callers can validate either the .part stage file or task.dest, reusing the streamed digest instead of re-reading the file. The torrent path in _download_with_fallback already validates before marking complete: torrent.download() verifies SHA internally and writes directly to task.dest (no promote step), so the promote-before-validate defect never applied there. RED (at cut point): uv run pytest tests/test_download_manager.py::TestDownloadHttp::test_corrupt_redownload_keeps_existing_dest -q AssertionError: a corrupt re-download must not delete the existing file assert False + where False = exists() GREEN (after fix): uv run pytest tests/test_download_manager.py -q 54 passed
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe download manager validates HTTP downloads against their ChangesDownload integrity
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Corrupt HTTP re-downloads now leave an existing valid destination intact, while valid downloads still promote successfully and clean up staging files. No current merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant _download
participant _validate_download
participant Filesystem
_download->>_validate_download: Validate .part file
_validate_download->>Filesystem: Check size and SHA-256
Filesystem-->>_validate_download: Return validation result
_validate_download-->>_download: Return success or SHA256 mismatch
_download->>Filesystem: Replace task.dest after success
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # must never be promoted. Remove only the stage file; | ||
| # task.dest may hold a previously-verified model from an | ||
| # earlier install and is left untouched. | ||
| part.unlink(missing_ok=True) |
There was a problem hiding this comment.
WARNING: part.unlink(missing_ok=True) after validation failure can raise non-FileNotFoundError exceptions (e.g., PermissionError), which would be caught by the outer except Exception handler and mask the actual validation error. The user would see the unlink error instead of "SHA256 mismatch".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -340,13 +347,12 @@ async def _download(self, task: DownloadTask, expected_sha256: str | None = None | |||
| body is on disk, so a failure can never leave a corrupt weight sitting | |||
| at the canonical path where every later "is this model installed?" | |||
There was a problem hiding this comment.
SUGGESTION: await self._validate_download(task, task.dest) is redundant since path defaults to task.dest. Could be simplified to await self._validate_download(task).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash:free · Input: 123.4K · Output: 22.6K · Cached: 199K |
1 participant
CARD TITLE (intent, not commit subject): download_manager: validate the .part BEFORE promoting -- a corrupt re-download must not replace an existing valid dest
Autonomous build of board card tsk-xprnf7.
When a re-download of an already-installed model produced corrupt bytes
(SHA mismatch), _download promoted the .part onto task.dest BEFORE
validation, then deleted task.dest on mismatch -- destroying the
existing valid file. The fix validates the .part first and only
promotes on success. On mismatch the .part alone is removed and
task.dest is left untouched.
_validate_download now takes a path parameter so callers can validate
either the .part stage file or task.dest, reusing the streamed digest
instead of re-reading the file.
The torrent path in _download_with_fallback already validates before
marking complete: torrent.download() verifies SHA internally and
writes directly to task.dest (no promote step), so the
promote-before-validate defect never applied there.
RED (at cut point):
uv run pytest tests/test_download_manager.py::TestDownloadHttp::test_corrupt_redownload_keeps_existing_dest -q
AssertionError: a corrupt re-download must not delete the existing file
assert False
GREEN (after fix):
uv run pytest tests/test_download_manager.py -q
54 passed
Files:
changelog.d/tsk-xprnf7-validate-before-promote.md | 18 ++++++++
tests/test_download_manager.py | 46 ++++++++++++++++++++
tinyagentos/download_manager.py | 51 +++++++++++++----------
3 files changed, 93 insertions(+), 22 deletions(-)
Summary by CodeRabbit