Repository navigation
fix(downloads): drain sibling range requests after failure - #2643
rudycelekli wants to merge 2 commits into
Conversation
Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com>
Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com>
|
All contributors on this pull request have signed the VoiceStudio CLA. Thank you! |
|
[Medium risk] Fixes parallel download failure handling to prevent partial writes. The PR appears safe to merge based on the reviewed changes. SummaryThe PR cancels and drains sibling range requests before a segmented download returns a failure.
Reviews (1) · Last reviewed commit: "docs: link range-drain regression report..." |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughSegmented downloads now cancel and drain pending range tasks when gathering fails, then re-raise the failure. An HTTP integration test covers failure and success cases. The troubleshooting guide and changelog describe the behavior. ChangesSegmented download failure cleanup
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The segmented-download cleanup is ready to merge after normal checks; no concrete outstanding issue was identified. 🚥 Pre-merge checks | ✅ 7 | ❌ 1 | ❓ 1❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (7 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (2 skipped: 2 unsupported.) Full details: Cross-Platform Default ParityExplanation The PR changes default download behavior:
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
|
recheck |
|
Thanks for the report and the work on this, @rudycelekli. The problem in #2642 is fixed on |
Summary
When one parallel range request fails, asyncio.gather propagates its exception while siblings may still write to the partial file or invoke progress callbacks. The caller can receive failure while those writers remain active.
Changes
Type
Testing
Hosted CI audit at 2026-10-06T12:35:20.584679+00:00: no failing latest checks; required CLA passes. All latest reported checks completed successfully. No upstream merge performed.
A real HTTPX client and threaded loopback range server reproduce the live sibling after a refused range. The native success control reassembles the exact eight MiB payload, reports exact progress and removes the partial file. Existing range-contract, resume and changelog modules also pass.
Regression against unchanged main production source: 1 failed, 12 passed. After the fix: 49 passed across range/downloader/resume/changelog modules.
Python 3.13 with
HF_HUB_OFFLINE=1and an empty Hugging Face cache. No model download or inference calls.Diff check and Python compilation pass.
Full backend/Electron suites were not run locally; focused tests alone do not establish hosted security or platform gates. Hosted results are reported separately below.
Checklist
smoke-matrixCI — not run locallyCloses #2642
The downloader now cancels and drains sibling range tasks before returning a download failure, preventing further writes or progress callbacks. This addresses failures where
asyncio.gatherraised while sibling requests continued running. Focused tests pass, but full backend, Electron, hosted security, and platform checks were not run.