Skip to content

Multithreaded parallel downloads with auto-scroll and queue UI controls - #1885

Closed
SirBiggin wants to merge 5 commits into
rmcrackan:masterfrom
SirBiggin:master
Closed

Multithreaded parallel downloads with auto-scroll and queue UI controls#1885
SirBiggin wants to merge 5 commits into
rmcrackan:masterfrom
SirBiggin:master

Conversation

@SirBiggin

@SirBiggin SirBiggin commented Jun 21, 2026

Copy link
Copy Markdown

Summary

  • Parallel downloads: Download and DeDRM multiple audiobooks simultaneously using a configurable thread pool (defaults to processor count). Uses a HashSet<Task> loop allowing dynamic concurrency changes mid-run.
  • Auto-scroll: The download queue automatically scrolls to keep active downloads in view via a new ScrollToTop method on VirtualFlowControl.
  • UI controls: Added two checkboxes to the Process Queue panel — Auto-scroll and Parallel downloads — alongside the existing Cancel All and DL Limit controls.
  • Thread-safety fix: Fixed InvalidOperationException crash in setLiberatedVisibleMenuItem caused by enumerating GetVisible() on a background thread while parallel downloads modified the collection. Collection is now snapshotted on the UI thread before being passed to Task.Run.
  • TrackedQueue multi-active: Replaced single Current item with an _active list to track multiple simultaneous downloads, while keeping the backward-compatible Current property.
  • Settings: Added AutoScrollQueue (bool) and MaxConcurrentDownloads (int) persistent settings.

Test plan

  • Start multiple downloads and confirm they run in parallel
  • Toggle Parallel downloads checkbox off mid-run and confirm it serializes
  • Toggle Auto-scroll and confirm queue scrolls/stops scrolling to active items
  • Confirm no InvalidOperationException crash during parallel downloads
  • Confirm Cancel All cancels all active downloads
  • Confirm settings persist across restarts

SirBiggin and others added 5 commits June 16, 2026 10:23
@SirBiggin
Added Multi Threading for downloads and Auto Scroll to view current d…
69d79f0 
…ownloads
@SirBiggin
Merge branch 'master' of https://github.com/rmcrackan/Libation
3dfa443 
# Conflicts:
#	Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs
@SirBiggin
Merge remote-tracking branch 'upstream/master'
cd0154a
@SirBiggin @claude
Fix thread-safety crash in setLiberatedVisibleMenuItem during paralle…
444d467 
…l downloads

Snapshot GetVisible() on the UI thread before handing off to Task.Run,
preventing InvalidOperationException when parallel downloads modify the
collection concurrently. Also fix installer script to use 64-bit dotnet.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@SirBiggin
Merge remote-tracking branch 'upstream/master'
5be38bd
rmcrackan
rmcrackan requested changes Jul 25, 2026
View reviewed changes

@rmcrackan rmcrackan left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it has taken me so long to get back to this.

Thanks for this - the concurrent queue loop and multi-active TrackedQueue are a solid foundation. A few gaps before this is safe to merge:

  1. Avalonia parity - Parallelism is on by default in shared UI code, but Avalonia has no toggle and Cancel All still only cancels Current (see ProcessQueueControl.axaml.cs).
  2. Abort / disk-full - Please also cancel in-flight active downloads, not just clear the pending queue.
  3. Safer defaults - Defaulting to ProcessorCount is aggressive given Audible license throttling; prefer opt-in or a small default, plus a way to set the limit.
  4. Installer script - Please drop the hardcoded dotnet.exe path; it looks unrelated to this feature.

Also worth rebasing on current master when you get a chance.

Comment thread Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs
SpeedLimit = Configuration.Instance.DownloadSpeedLimit / 1024m / 1024;
MaxConcurrentDownloads = Configuration.Instance.MaxConcurrentDownloads;
AutoScrollQueue = Configuration.Instance.AutoScrollQueue;
MultiThreadEnabled = true;

@rmcrackan rmcrackan Jul 25, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this lives in shared UI code, Avalonia gets parallel downloads with no control to turn them off. Could we default to off (or persist the setting) and add matching Avalonia UI?

Comment thread Source/LibationUiBase/ProcessQueue/ProcessQueueViewModel.cs

if (result == ProcessBookResult.FailedAbort)
{
abortCts.Cancel();

@rmcrackan rmcrackan Jul 25, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClearQueue() stops new work, but other active downloads keep running. Could we cancel all Active items here (same as Cancel All)? Same for the disk-full path just below.

Comment thread Source/LibationFileManager/Configuration.PersistentSettings.cs
{
var value = GetNonString(defaultValue: 0);
// Treat 0 or 1 as "use default" — 1 may have been written by an earlier bug.
return value <= 1 ? Environment.ProcessorCount : value;

@rmcrackan rmcrackan Jul 25, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaulting to ProcessorCount may trigger Audible license denials on larger machines. A small default (e.g. 2-3) and a numeric control would be safer than an on/off checkbox alone.

Comment thread Scripts/Windows/Build-WindowsInstaller.ps1
dotnet publish 'LoadByOS/WindowsConfigApp/WindowsConfigApp.csproj' @publishArgs
dotnet publish 'LibationCli/LibationCli.csproj' @publishArgs
dotnet publish "Hangover$Ui/Hangover$Ui.csproj" @publishArgs
& "C:\Program Files\dotnet\dotnet.exe" publish "Libation$Ui/Libation$Ui.csproj" @publishArgs

@rmcrackan rmcrackan Jul 25, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind leaving this out of the PR? Hardcoding C:\Program Files\dotnet\dotnet.exe is machine-specific and unrelated to parallel downloads.

@SirBiggin

SirBiggin commented Jul 25, 2026 via email •
edited by rmcrackan
Loading

Copy link
Copy Markdown
Author

@dmatlock171

dmatlock171 commented Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown

@SirBiggin — I'd like to pick this up and finish it, if you're alright with that.

I've been working on Libation's Avalonia UI on a fork, which means the Avalonia parity item in rmcrackan's review is more or less what I've been doing anyway.

The hard part here is already done. The concurrent queue loop and the multi-active TrackedQueue are the substance of this, and they hold up — I've rebased both of your commits onto current master (one conflict, in ProcessQueueViewModel.cs, where upstream added the Widevine guidance dialog to the old sequential loop; I moved it into your per-book task under the same lock you used for the license and disk-full messages). Your commits are unsquashed and still authored by you. The installer script change is dropped, and I'm working through the other three points.

Two ways to land it, and I'm fine with either:

  1. I open a PR against SirBiggin/Libation-Mulithreaded:master. You merge it, this PR updates in place, and it stays yours with your name on it.
  2. I open a new PR that continues this one, links back here, and credits you as the original author.

I'd prefer the first — it's your work and it should read that way. But if you've moved on, say so and I'll take the second route. Either way there's no obligation to review anything.

@SirBiggin

SirBiggin commented Aug 13, 2026

Copy link
Copy Markdown
Author

That's actually very kind of you, I'm happy to go route 1. just Gatta find some time this week to sink my teeth into it. This is my first PR so I'm still a little new at this

@dmatlock171

dmatlock171 commented Aug 13, 2026 via email

Copy link
Copy Markdown

@dmatlock171

dmatlock171 commented Aug 13, 2026

Copy link
Copy Markdown

Path 1 it is! Awesome.

I rebased your two commits onto current master, then tested what happens if you merge that into your branch the normal way. It merges cleanly with no conflicts — but it quietly brings back the hardcoded C:\Program Files\dotnet\dotnet.exe in the installer script, the one rmcrackan asked to drop. Git does that because your branch has the change and the common ancestor doesn't, so the merge keeps it. Easy to miss,

The way around it is for your branch to become the rebased version rather than merge with it. If you add me as a collaborator on Libation-Mulithreaded (Settings → Collaborators → Add people), I can push it to your master directly. That's a force-push, so the branch history gets replaced — but both your commits are still there and still authored by you, just replayed on top of current master. #1885 updates in place and there's nothing for you to resolve.

I'd rather hand you something finished than something half-done, so I'll work through rmcrackan's other three points first: the safer concurrency default with a numeric control, cancelling in-flight downloads on abort and disk-full, and the Avalonia UI. I'll comment here when it's ready so you can look it over before anything touches your fork. No rush on your end.

Let me know.

Cheers,

-D

@dmatlock171

dmatlock171 commented Aug 16, 2026

Copy link
Copy Markdown

@SirBiggin @rmcrackan — I've finished the changes from the review, and I'd like to know how you'd both prefer this lands.

The concurrency work here is the substance of it and it holds up. I've kept both of @SirBiggin's commits unsquashed and still authored by him, rebased onto current master. All four review points are addressed:

  1. Avalonia parity. Chardonnay's queue panel now has the same controls as Classic — an "At once" numeric limit and the auto-scroll toggle. Cancel All cancels every active item rather than just Current.
  2. Abort and disk-full. Both paths now cancel in-flight downloads instead of only clearing the pending queue.
  3. Safer default. MaxConcurrentDownloads defaults to 3, with a numeric control rather than an on/off checkbox, and a hard cap of 10. I can confirm the concern was real: pushing concurrency above 3 reproduced Content License denied errors, which surface as ordinary download failures.
  4. Installer script. Dropped — the diff against master is now empty for that file.

Also rebased on current master as asked, plus three fixes found while testing: the thread-safety crash in setLiberatedVisibleMenuItem, free slots not being filled when books are queued after the loop starts, and overlapping controls in the WinForms queue panel.

On how to land it. My preference is still that this stays @SirBiggin's PR, since it's his work — if you add me as a collaborator on Libation-Mulithreaded I can push the rebased branch and #1885 updates in place with nothing for you to resolve. Worth noting it has to be a push rather than a merge: merging brings the installer change back, because your branch has it and the merge base predates it, which would look like the review was ignored.

If you'd rather not, or you've moved on, that's completely fine — say so, or say nothing, and I'll open a separate PR that links back here with you credited as the original author. Either way there's no obligation to review anything.

@rmcrackan — happy to go whichever way is less work for you.

@rmcrackan

rmcrackan commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Reviewing now. Sorry for the additional merge conflicts I caused today.

@SirBiggin

SirBiggin commented Aug 16, 2026

Copy link
Copy Markdown
Author

hey guys, finally got some time to work on this, im pulling everything now, give me a couple of hours to catch up.

Panning on working on resolving the conflicts and pull in @dmatlock171's PR

@rmcrackan

rmcrackan commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Plz don't start yet. I'm wrapping up my write up which could substantially affect your work.

@rmcrackan

rmcrackan commented Aug 16, 2026

Copy link
Copy Markdown
Owner

@SirBiggin @dmatlock171 - thanks to you both.

Let me answer the "whichever is less work for you" question first, because I think the plan you have both agreed to is the riskier one, and for a reason that isn't obvious.

Please open a new PR against master rather than force-pushing into SirBiggin's fork. I'm happy to give you both credit. In fact, please do add both github users into Source/LibationUiBase/LibationContributor.cs

What Path 1 costs is real. A force-push here replaces every commit on the branch, so #1885 would become a substantially different change wearing the old number, my July review would be attached to code that no longer exists, and there'd be no diff anywhere showing what changed. I'd also be seeing the new content for the first time only after it had already replaced the old. That's a bad review surface for a change to the download loop.

So: new PR, both authors' commits preserved, rebased on current master. Your parallel-downloads branch already looks like exactly the right scope - if you push that and open it as-is, there is nothing to strip out. Two things I would check first: whether the Avalonia queue panel still lays out correctly without 6a135e0e, since that reflow stays behind on tipple, and whether you would rather c69fe2e3 went in here or as its own small fix. SirBiggin, if you're happy with that, nothing is needed from you at all - no collaborator access, nothing to resolve.

Now the part that matters more than the process question.

The rebase is already stale, and the conflict is a design one. parallel-downloads is rooted on 79142567, which was master on Aug 9. We're over 100 commits past that now. In particular 818510d1 (Aug 14) added the opt-in daily download limit, and it lives inside the exact while (Queue.MoveNext()) loop this PR replaces. It's sequential by construction and it collides in concrete ways:

  • RequeueLast(book) defers a book with Queue.ClearCurrent() + re-enqueue. Under the new TrackedQueue, ClearCurrent() drops _active.FirstOrDefault() - so deferring the second of three active books silently evicts the first one instead.
  • AnyOtherQueuedBookAllowed does Queue.Any(...), which is an unlocked enumeration (more on that below).
  • CancelAllAsync still only cancels Queue.Current, and it sets the cancelAllRequested flag the daily-limit wait watches - so ba3bf914's rewrite of it will need to keep that.

This isn't a merge conflict, it's "how does the limit gate behave when N books are in flight", and I think it's the biggest piece of work left.

Two defects I can reproduce. I compiled TrackedQueue[T].cs into a standalone harness - it has no dependencies beyond System, which is also why it's easy to test. Note that the file is byte-identical between this PR's head and 6d74d28e, so both of these are still live on the follow-up branch. Happy to share the harness.

  1. Out-of-order completion renumbers rows and never says so. MarkCompleted moves a book from _active to _completed and the display index comes off Completed.Count, so when the second of two active books finishes first, the two swap rows. Only CompletedCountChanged fires, never CollectionChanged, so a bound list keeps painting the old order. Four queued, two active, B finishes first: [0]=A, [1]=B becomes [0]=B, [1]=A with zero notifications. Out-of-order completion is the normal case here, so expect rows showing the wrong book's progress.

  2. Enumerating the queue during concurrent mutation throws. GetAllItems builds a lazy Concat inside the lock and returns it, so the enumeration happens unlocked - any foreach or LINQ over the queue while a book task mutates it throws InvalidOperationException: Collection was modified. This one is mine, it predates the PR, and on master it's unreachable because everything mutates on the UI thread. This change makes it reachable. It's also the same crash the PR fixes in setLiberatedVisibleMenuItem, treated at the symptom rather than the source.

Three more from reading the diff, all still open at 6d74d28e:

  • Faulted book tasks vanish. activeTasks.RemoveWhere(t => t.IsCompleted) drops faulted tasks unobserved, before the closing WhenAll could rethrow. ProcessOneAsync can throw out of its finally via GetFailureActionAsync; today that lands in the QueueLoop catch and gets logged. It should still get logged.
  • The bad-book dialog doesn't survive concurrency. The license and Widevine dialogs are guarded to fire once per run, but ShowRetryDialogAsync is per-book and its "apply to all remaining books" answer lands in a shared BadBookSessionContext. Three books failing at once means three stacked modals racing to set the same override.
  • Tests. Source/_Tests/LibationUiBase.Tests already exists and TrackedQueue<T> is a pure data structure. Multi-active ordering and notifications should be pinned down there.

On the follow-up commits themselves - they're good, and I don't want that lost in the list above. 6d74d28e is the one I'd single out: books queued just after the loop starts used to sit idle until an in-flight download happened to finish, and the fix captures the signal before inspecting the queue and swaps in a fresh TaskCompletionSource via Interlocked.Exchange, so a wakeup landing between the dequeue and the wait isn't lost. That's the right pattern and it's easy to get wrong. ba3bf914 also fixes an Avalonia bug that predates this PR entirely, and c69fe2e3 catches the panel overlap the new checkbox introduced. All four of my original points are genuinely addressed, and the commit messages explain intent instead of restating the diff - much appreciated.

One thing to reconsider in 47a31379: MaxAllowedConcurrentDownloads becomes Math.Clamp(Environment.ProcessorCount, 1, 10) and both the getter and the setter clamp to it. A user who picks 8, then opens the same config on a smaller machine or in a container, reads back 2 - and has 2 written over their stored 8. Machine capability belongs in the spinner's bounds, not in the persisted value. Also worth confirming at runtime that the Avalonia Minimum="{Binding MinConcurrentDownloads}" bindings resolve, since those are static properties on the view model.

And a small note on point 3 from my original review - thank you for actually reproducing the Content License denied errors above concurrency 3 rather than taking my word for it. A conservative default plus a hard cap is exactly right.

@SirBiggin - your loop and the multi-active TrackedQueue are still the substance of this, and that hasn't changed.

@dmatlock171 - I'd rather have this in a reviewable PR than in the right place on the org chart, so take the new-PR route and I'll review it properly.

@SirBiggin

SirBiggin commented Aug 16, 2026

Copy link
Copy Markdown
Author

i kinda agree with this, im just a novice fucking around with some code i find cool trying to make it helpful for others.

if this is legitimately going to be used i would want @rmcrackan to have another PR as long as he's willing to credit me for the work ive done so far. i do think that he needs to be the one reviewing @dmatlock171 changes going into the repo eventually.

with that being said, do i need to rebase my PR and make sure it has the most recent master?

@dmatlock171

dmatlock171 commented Aug 16, 2026 via email

Copy link
Copy Markdown

@dmatlock171

dmatlock171 commented Aug 16, 2026 via email

Copy link
Copy Markdown

@dmatlock171

dmatlock171 commented Aug 16, 2026 via email

Copy link
Copy Markdown

@SirBiggin

SirBiggin commented Aug 16, 2026

Copy link
Copy Markdown
Author

hey, i was content not getting credit for my work when @rmcrackan made that reddit post about stepping back from dev work from the project literally days after i made the PR. i thought for sure my PR was going to get shot down. i'll take credit if its being given out, but i just wanted to add a cool feature

@dmatlock171

dmatlock171 commented Aug 16, 2026 via email

Copy link
Copy Markdown

@dmatlock171 dmatlock171 mentioned this pull request Aug 17, 2026
Open
@dmatlock171

dmatlock171 commented Aug 17, 2026

Copy link
Copy Markdown

@SirBiggin @rmcrackan — opened as #1958, rebased onto current master with both of SirBiggin's commits preserved and still authored by him.

All four points from the July review are addressed, plus what came out of the writeup: the daily download limit collision (818510d assumed the sequential loop, so RequeueLast was evicting the wrong active book), both TrackedQueue defects, and the three from reading the diff. TrackedQueue tests are in Source/_Tests/LibationUiBase.Tests. We're both added to LibationContributor.cs.

Tested end to end against a real library: parallel downloads, out-of-order completion, Cancel All mid-run, the limit pause and resume, and the Plus-only deferral.

@SirBiggin — nothing needed from you, no collaborator access. The loop and the multi-active queue are still the substance of this.

Some other minor notes I put in the PR about the queue window, etc.

@rmcrackan

rmcrackan commented Aug 17, 2026

Copy link
Copy Markdown
Owner

Thanks y'all. I'll be reviewing the other one when I can get a break at work. I was up all night addressing other Libation issues and questions so I'm a bit burned-out right now.

I'm going to close this PR (#1885) and we'll focus on the new one (#1958). You're welcome to continue discussions in either.

@rmcrackan rmcrackan closed this Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rmcrackan rmcrackan rmcrackan requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@SirBiggin @dmatlock171 @rmcrackan