Skip to content

fix(downloads): drain sibling range requests after failure - #2643

Closed
rudycelekli wants to merge 2 commits into
debpalash:mainfrom
rudycelekli:fix/voice-download-range-task-drain-20261006
Closed

rudycelekli wants to merge 2 commits into
debpalash:mainfrom
rudycelekli:fix/voice-download-range-task-drain-20261006

Conversation

@rudycelekli

@rudycelekli rudycelekli commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Cancel and drain every owned sibling task before releasing the client and re-raising the original exception.
  • Add focused regressions and update the relevant documentation and changelog.

Type

  • 🐛 Bug fix

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=1 and 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

  • I've tested this locally
  • Every commit author has signed the CLA — registered signature and required CLA status verified
  • I've updated relevant documentation
  • No local machine paths, logs, or personal env details in this PR
  • Maintained version files are in sync — no version bump
  • Runtime regression fixture still loads green on smoke-matrix CI — not run locally

Closes #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.gather raised while sibling requests continued running. Focused tests pass, but full backend, Electron, hosted security, and platform checks were not run.

Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com>
Signed-off-by: Rudy Celekli <47457359+rudycelekli@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

All contributors on this pull request have signed the VoiceStudio CLA. Thank you!

@greptile-apps

greptile-apps Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Medium risk] Fixes parallel download failure handling to prevent partial writes.

The PR appears safe to merge based on the reviewed changes.

Summary

The PR cancels and drains sibling range requests before a segmented download returns a failure.

  • Adds a loopback HTTP regression test for failure and successful reassembly.
  • Updates troubleshooting guidance and the changelog.

Reviews (1) · Last reviewed commit: "docs: link range-drain regression report..."

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: debpalash/VoiceStudio/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1c4c52f5-0235-4161-ad7d-cb199d7901c1
📥 Commits

Reviewing files that changed from the base of the PR and between 286f46b and fa71f71.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • backend/services/segmented_download.py
  • docs/install/troubleshooting.md
  • tests/test_fdl_segmented_download.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

Segmented 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.

Changes

Segmented download failure cleanup

Layer / File(s) Summary
Cancel and drain sibling range tasks
backend/services/segmented_download.py, tests/test_fdl_segmented_download.py, docs/install/troubleshooting.md, CHANGELOG.md
The downloader cancels and awaits segment tasks when gathering raises. The integration test checks failure and success behavior. The troubleshooting guide and changelog describe the cleanup.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: debpalash, marreiradigital

Merge Risk: ⚪ Minimal · up to fa71f

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
Cross-Platform Default Parity ❓ Inconclusive The PR changes default download behavior: segmented_download now cancels and drains sibling tasks on failure (backend/services/segmented_download.py:215–226), and the caller enables segmented downlo… Run the new regression on macOS, Windows, and Linux, or otherwise provide evidence that this failure path behaves identically on all three. If it diverges, fix the divergence or move the behavior behind an explicit opt-in.
✅ Passed checks (7 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue [#2642] requires sibling range writers and progress callbacks to stop before failure returns. segmented_download now cancels and drains every segment task before propagating the original excep…
Out of Scope Changes check ✅ Passed All changes support issue [#2642]. The test covers failure and success paths, and the changelog and troubleshooting updates document the same behavior.
I18n Completeness (21 Locales) ✅ Passed The check is not applicable to this pull request. The reviewed diff changes only CHANGELOG.md, backend/services/segmented_download.py, docs/install/troubleshooting.md, and `tests/test_fdl_segmen…
Local-First Guarantee ✅ Passed The PR adds no new product network calls, cloud requirements, accounts, or API keys. The production change only creates tasks for the existing range requests and cancels and drains them on failure (ba…
Backward Compatibility ✅ Passed The PR changes only the segmented downloader, its tests, the changelog, and troubleshooting documentation. It adds no database schema or migration changes and does not alter voices, projects, settings…
Title check ✅ Passed The title follows the required conventional-commit format with a scope, and the description references issue #2642.
Description check ✅ Passed The description includes the required Summary, Changes, Type, Testing, and Checklist sections. It identifies the runtime regression fixture as not run locally and reports testing limits.
Full details: Docstring Coverage

Explanation

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 Parity

Explanation

The PR changes default download behavior: segmented_download now cancels and drains sibling tasks on failure (backend/services/segmented_download.py:215–226), and the caller enables segmented downloads by default (backend/api/routers/setup/download.py:170–180). The new native HTTPX regression is in the Linux test suite; the cross-platform smoke matrix runs a narrower test set, and the PR reports that platform matrix validation was not run. No platform divergence is established, but parity across macOS, Windows, and Linux is unverified.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@rudycelekli

Copy link
Copy Markdown
Contributor Author

recheck

@debpalash

Copy link
Copy Markdown
Owner

Thanks for the report and the work on this, @rudycelekli. The problem in #2642 is fixed on main by 5dc54750 (merged in #2673), so I'm closing this pull request as already fixed. It will ship in v0.5.7. Reporter credit stays in the changelog.

@debpalash debpalash closed this Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Failed parallel range downloads leave sibling writers running

2 participants