Parallel downloads rebased

[dmatlock171]

OK, this was a lot harder then I expected. Chardonay is ticky. Please please double check the work. Very rewarding but I am tired of tesitng both standard and chardonay. :-)

Continues #1885 by @SirBiggin, rebased onto current master (bb0ece9e). His two commits are
unsquashed and still authored by him — the concurrent queue loop and the multi-active TrackedQueue
are his work and remain the substance of this change. We're both added to LibationContributor.cs.

Opened as a new PR rather than force-pushed into #1885, per @rmcrackan's request.

The original four review points

1. Avalonia parity — Chardonnay's queue panel has the same controls as Classic, and Cancel All
   cancels every active item rather than just Current.
2. Abort and disk-full — both paths now cancel in-flight downloads, not just the pending queue.
3. Safer default — MaxConcurrentDownloads defaults to 3 with a numeric control and a hard cap
   of 10. The concern was real: above 3 reproduced Content License denied, which surfaces as
   ordinary download failures.
4. Installer script — dropped; the diff against master is empty for that file.

The daily download limit

818510d1 landed after this branch was written, inside the exact while (Queue.MoveNext()) loop
this change replaces, so rebasing alone left it subtly wrong rather than merely conflicted. The gate
now runs in the dispatch loop, between taking a book off the queue and starting its task — same
semantics, while books already in flight carry on. Three defects fell out of that:

• RequeueLast deferred a book with ClearCurrent(), which drops Active[0]. With three in flight
  that's somebody else's download, so deferring the second active book evicted the first.
• A book cancelled at the gate stayed on the active list — the sequential loop retired it on the next
  MoveNext(), and there is no next MoveNext().
• CancelAllAsync existed in two forms after the rebase; unified into one that both sets
  cancelAllRequested (how a paused queue learns to stop) and cancels every active book.

The TrackedQueue defects

Both of the ones you reproduced are fixed, and both are pinned down by tests in
Source/_Tests/LibationUiBase.Tests/TrackedQueueTests.cs:

• MarkCompleted now raises a Move for the book whose display position actually changed. With one
  book at a time the indices are equal and nothing is raised, so the sequential path is unchanged.
• GetAllItems returns a snapshot taken under the lock instead of a lazy Concat evaluated outside
  it. That's the source of the crash setLiberatedVisibleMenuItem was patched for at the symptom,
  and it makes AnyOtherQueuedBookAllowed's Queue.Any(...) safe. GetActive() was added for the
  same reason; the speed-limit and Cancel All paths use it.

Plus the three from your diff read: faulted book tasks are observed and logged rather than dropped;
the bad-book dialog is serialised on the session so concurrent failures can't stack modals racing
over the same override; and machine capability no longer overwrites the stored concurrency setting.

That last one needed fixing twice. Bounding the persisted value was only half of it — both spinners
took their maximum from the same machine-derived number and are two-way, so a control handed a value
above its maximum coerces the display down and writes it back. The bound is now machine capability
or the stored value, whichever is higher. The trade-off is an asymmetry: on a machine smaller than
the stored value you can lower the setting but not raise it. The alternative — bound by the flat hard
limit, apply machine capability only at the point of use — avoids that but lets someone pick 10 on a
two-core box and silently get 2. I went with the first because it's what you asked for; happy to
switch.

Tested

Against a real library, both UIs: parallel downloads, out-of-order completion, Cancel All mid-run,
the limit pause and cancel-while-paused, the Plus-only deferral with owned titles still downloading,
concurrency persistence across restarts (including with DOTNET_PROCESSOR_COUNT faked low), and the
bad-book dialog under five simultaneous failures.

Layout, and two things to decide

The queue settings panel needed rework in both UIs — WinForms positioned everything absolutely and
mixed left- and right-anchored controls, and Chardonnay's single row of four groups pushed Clear
Finished off the edge. Both are now two-row layouts with labels right-aligned in a shared column.
Worth noting the WinForms narrow-width overlap predates this PR — the DL Limit label has always
slid onto Cancel All the same way — so this fixes more than it caused. Both layout commits are last
on the branch and drop cleanly if you'd rather they went separately.

Unrelated, but found while testing: Chardonnay's ProcessBookControl is pinned to 87px and splits
*,16 between the title stack and the status line. The stack isn't clipped, so a title wrapping to
three lines paints over the status text. Reproducible on master with any long title — the daily-limit
status just makes it obvious, being a long string where you'd normally read "Queued". Left alone
here since this PR doesn't otherwise touch that file, but worth an issue.

[rmcrackan]

Buckle up, it's going to be a big one. As you well know, this is an ambitious one that you took a bite of.

I assume most people are using AI help these days so I made this thorough and (hopefully) consumable by humans and AI. Thanks for the honest write-up — it made this much faster to review, and the substance holds up: the dispatch loop, the multi-active TrackedQueue, and the daily-limit rework are all soundly done. I built the branch, ran the tests, ran two end-to-end sessions in Chardonnay, and wrote a small harness against a verbatim copy of TrackedQueue to stress the parts I couldn't drive from the UI. LibationAvalonia builds clean (only the expected NU1903/SourceLink noise) and all 172 tests in LibationUiBase.Tests pass in 1.7s, including your nine new ones.

Three things I'd want fixed before merge, then some decisions and nits. One of them is in the area you asked to have double-checked. There's an implementation-notes appendix at the end with reproduction steps and a couple of traps, in case you hand any of this to an assistant.

1. Abort and disk-full throw Queue empty. and take the queue loop down

This is review point #2 turning back on itself. Reproduced on the first attempt: 5 books via the debug "Simulate bad book failures" item at At once = 3, dialog answered Ignore, Ignore, Abort. The moment the third book returned FailedAbort:

18:54:01.385 [INF] Completed processing: '[DEMO003] 03 Green | PDF done | purchased' result: "FailedAbort"
18:54:01.401 [ERR] A book failed to process and did not report a result
System.AggregateException: One or more errors occurred. (Queue empty.)
 ---> System.InvalidOperationException: Queue empty.
   at System.Collections.Generic.Queue`1.Dequeue()
   at LibationUiBase.ProcessQueue.ProcessBookViewModel.get_CurrentProcessable() ... line 111
   at LibationUiBase.ProcessQueue.ProcessBookViewModel.CancelAsync() ... line 277
   at LibationUiBase.ProcessQueue.ProcessQueueViewModel.CancelAllAsync(ProcessBookViewModel except) ... line 611

CurrentProcessable is _currentProcessable ??= Processes.Dequeue().Invoke(). A book that has run its last step has a null field and an empty Processes, so the getter throws — and the catch in CancelAsync throws again by interpolating CurrentProcessable.Name. This isn't a narrow race: every book sitting in the bad-book dialog is in that state, because ProcessOneAsync reaches GetFailureActionAsync from its finally after the processable loop has drained. So abort with anything else in flight hits it reliably.

It doesn't stop at one faulted book task. The abort branch's await Task.WhenAll(activeTasks) rethrows, QueueLoop exits through its outer catch, and Completed processing queue, the final Queue_CompletedCountChanged, and ProgressBarVisible = false never run — the queue dies quietly with the progress bar still on screen, and the remaining books finish unsupervised (mine completed 40s and 65s after the loop had gone). Master never called CancelAsync from the loop at all (FailedAbort just did ClearQueue()), and the Cancel All button can't be clicked while a modal dialog is up, so this arrived with the new cancellation behaviour.

Three small changes cover it: have CancelAsync test the _currentProcessable field instead of the instantiating property, keep CurrentProcessable out of its catch message, and isolate each cancellation in CancelAllAsync so one failure can't abandon the rest of the list. Worth noting the lazy ??= also mutates a non-thread-safe Queue<T> from the cancelling thread while the book's own loop reads it, which the field-based check removes too.

2. TrackedQueue computes notification indices under the lock and raises them outside it

MarkCompleted snapshots oldIndex/newIndex inside the lock, then raises CompletedCountChanged and the Move after releasing it. A second book can complete in that gap, so two Move events can arrive in an order no index-based consumer can replay. The gap isn't tiny, because MarkCompleted always raises CompletedCountChanged first and the view model's handler runs two LINQ passes over Completed before the Move goes out.

Forcing the interleaving turns a queue whose real order is [B, C, A] into a bound list holding [B, A, B] — a duplicated row and a lost one. Unforced, with four books completing together, about 1 run in 50 ends with the bound list disagreeing with the queue. That is precisely the "rows show the wrong book" symptom the Move was added to fix, so it's worth closing rather than leaving as a rare cosmetic slip.

This matters for Chardonnay specifically, since ProcessQueueControl.axaml binds ItemsSource straight to the queue and Avalonia applies Move/Remove by index; WinForms is immune because VirtualFlowControl.Items_CollectionChanged discards the args and re-reads through RefreshDisplay. The minimal fix is to make delivery order match mutation order: append the event args to a pending list while the lock is held, then drain that list outside the lock under a separate dispatch lock, so whoever drains first delivers everything in the order it happened. Marshalling to the UI thread on top of that is a reasonable hardening, but please read the appendix first — the obvious way to do it deadlocks.

On that last point, in fairness to the current code: I did verify that off-thread notifications work today. I forced out-of-order completion (apply-to-all Ignore, At once = 3) so three Move events were raised from thread-pool threads, and the display ended in exactly the logged completion order with no exception and a clean Completed processing queue. So this is a correctness-under-race and contract issue, not a crash you're currently shipping. Related, and cheap while you're in there: MarkCompleted on an item that isn't in _active still appends it to Completed, so Count changes with no CollectionChanged at all — a guard or a throw would be worth having.

3. Chardonnay now clips "Cancel All" down to "Ca"

You asked whether the layout commits should go separately. Keep them, but they're not finished: the pane is a fixed 400px SplitView and the settings row is still *,Auto,Auto with four groups in it, so stacking the spinners made Clear Finished fit by collapsing the star column instead. On master, at identical window geometry, all three controls render in full; on this branch the left button reads "Ca". Stacking Cancel All and Clear Finished as well, or giving the spinners their own full-width row, would settle it at that width.

While there: the Auto-scroll checkbox is inert in Chardonnay. Only WinForms' Book_ProcessStart reads AutoScrollQueue; Avalonia's still uses the old "scroll only if the previous item is visible" logic and never consults the setting, so the control matches Classic but the behaviour doesn't. Either wire it up or drop the checkbox from Chardonnay.

Behaviour decisions worth a second look

WinForms auto-scroll changed more than the description implies. Master scrolled the new item into view only when the previous one was visible, with a comment explaining that this lets users scroll the queue without being interrupted; it now pins Completed.Count to the top on every start, so someone reading further down gets pulled back each time a book begins. Avalonia keeps the gentler behaviour, so the two UIs disagree about what auto-scroll means.

Abort no longer aborts, from the user's point of view. Because a dialog-level Abort doesn't set the session override and CancelAsync can't interrupt a book waiting on a dialog, every book already in flight still asks — I answered five dialogs in a run where I chose Abort on the third. Treating Abort as a session-wide override would make it mean what it says once several books are running. The DialogGate itself works exactly as intended: never more than one dialog on screen across the whole run.

On the concurrency bound you flagged: I'd take the alternative you offered. As shipped, the two UIs already disagree — Avalonia binds Maximum to MaxAllowedConcurrentDownloads and ratchets down, while WinForms sets concurrencyNum.Maximum once in the constructor and never handles nameof(MaxAllowedConcurrentDownloads), so within a session you can lower and re-raise. And the shipped combination still has the property you were trying to avoid: a stored 8 on a two-core box displays 8 while EffectiveConcurrentDownloads runs 2. Bounding by the flat hard limit and applying machine capability at the point of use gives one rule for both UIs; a tooltip or a "(2 on this machine)" hint would close the remaining gap cheaply.

Two smaller ones. cancelAllRequested stays set until the next QueueLoop starts, so books queued while the loop drains cancellations get cancelled at the gate — a window master closed almost immediately but which now lasts as long as N cancellations. And SpeedLimit = nextBook.Configuration.DownloadSpeedLimit / 1024m / 1024 was dropped from the loop without a mention; presumably deliberate now that several books run at once, but TrackedQueue.Current's doc comment still cites the speed-limit display as its reason to exist.

Nits

Current, Active, ClearCurrent() and MoveNext() are dead outside the #if DEBUG design-mode block in ProcessQueueControl.axaml.cs, so the "legacy / kept for compatibility" labels point at callers that no longer exist — worth deleting instead of documenting. CancelAllAsync carries two stacked <summary> blocks and two <param name="except"> tags from an edit that wasn't cleaned up. abortCts is only ever Cancel() plus IsCancellationRequested, so a volatile bool would say what it means; as written it suggests cancellation reaches the book tasks, which it doesn't. var _resultLock is a local named like a field. TrackedQueue[T].cs and ProcessQueueViewModel.cs lost their UTF-8 BOMs, and the WinForms designer diff carries a lot of // to // churn that pads the review. The redundant Configuration write in the MaxConcurrentDownloads setter is harmless — SetNonString returns early on equal values.

TrackedQueueTests is a good addition and reads well, but all the new risk lives in the dispatch loop — the capacity cap, the enqueue signal, the abort drain — and none of it is covered. Making book processing injectable would let the loop be tested with fake books that complete on command, which is also how the reordering in point 2 would be caught. And since docs/features/ has a page per user-facing feature, including the daily download limit this interacts with, parallel downloads probably deserves one — especially the finding that above 3 you start seeing Content License denied, and that books already in flight keep going when the limit pauses the queue.

Last thing, confirming your aside: the ProcessBookControl status line does get painted over — the status text is clipped on every row in my runs, on master too. Agreed it belongs in its own issue.

Appendix: implementation notes

Reproducing issue 1. Debug build, a library with at least five visible books (dotnet run Scripts/seed-demo-library.cs fills one), error handling left at "Ask each time", At once = 3. Settings > "Simulate bad book failures (test dialog)..." > Yes, then answer Ignore, Ignore, Abort. Grep the log for Queue empty.; the tell that the loop died rather than finished is the absence of Completed processing queue and the progress bar still being visible.

Reproducing issue 2's setup. Same start, but on the first dialog tick "apply to all" and choose Ignore, so the remaining books fail with no further prompts and race freely. Compare the order of Completed processing: lines in the log against the on-screen row order; they should match exactly. Running it a handful of times is the honest test, since the reorder window is small — roughly one run in fifty at four concurrent completions in my harness.

Traps in the issue-2 fix. Do not raise the events, or call a blocking Invoke, while lockObject is held: the UI thread reads Count, IndexOf and the indexer (Avalonia's binding and WinForms' DoVirtualScroll both do), all of which take that same lock, so a book thread holding the lock and waiting on the UI thread deadlocks. SynchronizeInvoker exposes a posting BeginInvoke alongside the blocking Invoke; only the posting form is safe from inside the lock, and only the posting form preserves order. Note also that TrackedQueue is not a ReactiveObject, so it has no invoker today — it needs one injected, or the view model has to own delivery. And keep delivery synchronous when there is no invoker: TrackedQueueTests asserts on moves immediately after calling MarkCompleted, and the "sequential path raises nothing" property (oldIndex == newIndex) is load-bearing — all 172 tests should still pass unchanged.

Two things not to chase. Queue_CompletedCountChanged enumerating the live Completed list looks like the GetAllItems bug, but Enumerable.Count(predicate) over a List<T> does not version-check on .NET 10 (span fast path): 0 throws in 200 forced races here, versus 200/200 for a plain foreach over the same list. Worth snapshotting for the stale-count reason, but it is not a crash. Likewise, off-thread Move events into Avalonia's ItemsControl do not throw today — I verified three of them applying correctly — so please don't rewrite the UI layer on the theory that they do.

Scope. The layout fix needs eyes on it at the pane's fixed 400px, not reasoning about Auto columns. And if you add loop-level tests, a seam that lets a fake book complete on command is enough; a wholesale refactor of QueueLoop would cost more review than it buys in this PR.

[dmatlock171]

Thank you — that's the most useful review I've had on anything. The appendix especially; the
repro steps and the two "don't chase these" notes saved a round on their own.

I am going to work these blockers locally first and check them out and I will do i full push at the end and I will let you know when to look. I epxect a number of incremental fixes and I don't want to cause you to get pummled with a lot of individutal notificaitons.

So, All three reproduce. Taking them in order. More later, but here is the full plan,

1. Queue empty. on abort and disk full

Confirmed, and your diagnosis is exactly right — it isn't a race. CurrentProcessable is
_currentProcessable ??= Processes.Dequeue().Invoke(), so any book past its last step throws on
read, and every book sitting in the bad-book dialog is in that state because ProcessOneAsync
reaches GetFailureActionAsync from its finally. The catch in CancelAsync then throws again
interpolating CurrentProcessable.Name, which is why it escapes at all.

Doing the three changes you describe: CancelAsync tests the _currentProcessable field rather
than the instantiating property, the catch message stops touching it, and CancelAllAsync isolates
each cancellation so one failure can't abandon the rest. The point about ??= mutating a
non-thread-safe Queue<T> from the cancelling thread is a good catch — the field test removes that
too.

This one is squarely mine. I added the cancel-from-the-loop path for review point #2 and never
tested abort with anything else in flight.

2. TrackedQueue notification ordering

Confirmed. Indices are snapshotted under lockObject and the events go out after it's released,
so two completions in that window can be delivered in an order no index-based consumer can replay.
And you're right that the window isn't tight, since CompletedCountChanged runs the view model's
two LINQ passes over Completed before the Move is raised.

Taking your fix: append the args to a pending list while the lock is held, then drain under a
separate dispatch lock so whoever drains first delivers everything in mutation order. I'll heed the
trap — posting BeginInvoke only, never a blocking Invoke from inside lockObject, and delivery
stays synchronous when there's no invoker so the existing tests keep asserting immediately after
MarkCompleted. TrackedQueue isn't a ReactiveObject, so I'll inject the invoker rather than
have it reach for one.

Adding the guard for MarkCompleted on an item that isn't in _active as well — appending to
Completed with no CollectionChanged at all is worse than the reorder.

3. Chardonnay clipping "Cancel All"

Mine, and you're right that stacking the spinners only made Clear Finished fit by collapsing the
star column. Giving the spinners their own full-width row rather than stacking the buttons — at a
fixed 400px that's the version with room to spare, and I'll check it at that width rather than
reason about it.

The inert Auto-scroll checkbox is a straight miss on my part against review point #1 — I checked
that the control existed in both UIs and not that it did anything in one of them. AutoScrollQueue
is only read in LibationWinForms/ProcessQueue/ProcessQueueControl.cs; Avalonia never consults it.
Which way it gets fixed depends on the next question.

Decisions

Concurrency bound — switching to the alternative, as you asked: bound the control by the flat
hard limit and apply machine capability at the point of use. One rule for both UIs, and it removes
the WinForms/Avalonia disagreement about whether you can re-raise within a session. Adding the
"(2 on this machine)" hint; that's the part that actually closes the gap.

Auto-scroll — I'd like your call here. I changed the WinForms behaviour without saying so,
which I shouldn't have; master's "scroll only if the previous item was visible" exists for a
documented reason and pinning Completed.Count to the top on every start does interrupt someone
reading further down. My instinct is to restore master's behaviour and wire Chardonnay's checkbox
to gate that same logic in both UIs, so auto-scroll means one thing. But you know what people
expect from that checkbox better than I do — if you'd rather it pin to the active download, I'll
do that in both instead.

Abort — agreed, and it's what point #2 was for. A dialog-level Abort will set the session
override so books already in flight stop asking. Glad the DialogGate itself held up.

The two smaller ones — cancelAllRequested staying set while the loop drains cancellations is
a real window now that it's N cancellations wide; I'll clear it at the end of the drain rather than
at the next QueueLoop. And dropping SpeedLimit from the loop was deliberate but undocumented —
I'll say so in the commit and fix TrackedQueue.Current's doc comment, which now cites a reason
that no longer applies.

Tests and docs

Fair that the new risk is all in the dispatch loop and none of it is covered. Making book
processing injectable so a fake book can complete on command is a small enough seam, and as you say
it's also how the reordering in #2 gets caught — I'll add it there rather than refactor QueueLoop.

Writing the docs/features/ page, including the Content License denied finding above 3 and the
fact that books already in flight keep going when the daily limit pauses the queue.

Nits

All taken: deleting Current, Active, ClearCurrent() and MoveNext() rather than documenting
them, cleaning up the doubled <summary>/<param> on CancelAllAsync, abortCts to a volatile bool, renaming _resultLock, restoring the BOMs, and reverting the // churn in the designer
diff.

[rmcrackan]

Your plan looks right, and batching it into one push at the end is fine by me - no need to drip-feed.

One correction before you start: don't clear cancelAllRequested at the end of the drain. That flag is what a queue parked in WaitForDailyLimitAsync uses to learn it should stop, and the gate only re-reads it every 15 seconds. Press Cancel All during a limit pause with nothing in flight, and the drain finishes and clears the flag well before the gate wakes up - it sees false and resumes the book you just cancelled. Scope it to the run instead: clear it in QueueLoop's finally, or swap it for a per-run CancellationTokenSource that Cancel All cancels and a new run recreates. That closes the window you're after without opening that one.

On #2, keeping delivery synchronous when there's no invoker is fine, but it means the tests cover inline delivery while the app runs the posted path - the one with the ordering bug. Add a fake invoker that queues actions so at least one test drives concurrent MarkCompleted through the real path. And post always, never inline, even from the UI thread; otherwise a UI-thread mutation can jump ahead of events already posted from a book thread.

Auto-scroll: your instinct is right. Restore master's behavior and let the checkbox gate that same logic in both UIs. Note that leaves VirtualFlowControl.ScrollToTop dead - delete it.

Last small thing: with Abort as a session override, every in-flight book will run the whole abort path. Short-circuit the inherited ones to cancelled rather than having each re-enter CancelAllAsync.

Nothing else from me - the rest of your plan looks solid.