fix(quickfiler): await a counted queue drain before metrics and cleanup (#633)

[drmoisan]

fix(quickfiler): await a counted queue drain before metrics and cleanup

Summary

• FilerQueue gains public Task WhenDrainedAsync(), a counted, awaitable quiesce backed by a monitor-guarded outstanding-work counter and a lazily created TaskCompletionSource<bool> drain signal.
• BackGroundMoveAsync now awaits that drain after the batch move and before both the WriteMetrics dispatch and the CleanupBackground() dispatch, so the ordering the code previously only assumed is now enforced by control flow.
• The Enqueue/ConsumeAsync start-stop handshake is repaired. The one-shot guard start gate is replaced by a running flag set and cleared inside the same critical section that performs the queue add and the TryTake failure, which closes the orphaned-item window.
• An internal Func<FilerQueueItem, Task> ItemProcessor seam replaces the hard-coded per-item call, making the concurrency assertions testable without a live Outlook COM path.
• The two subsumed await _parent.FilerQueue.Consumer; statements are removed, and BackGroundMoveAsync's early-return guard gains a _parent null clause for the post-Cleanup() state.

Why

The batch-move path treated the undo stack as populated by the time the move completed, but the push happens later on a queue worker. MoveMailAsync only enqueues the filer and then returns await Task.CompletedTask, so awaiting it conveyed nothing about the filing work. BackGroundMoveAsync then proceeded straight to WriteMetrics and CleanupBackground() with no statement between them that observed the queue at all.

The defect was latent rather than active: the entries land eventually and are serialized, so undo worked in the observed configuration. The cost was the absent ordering constraint. A future caller reading the stack immediately after a batch move would have seen an incomplete stack with no diagnostic.

The handshake repair is a precondition rather than an opportunistic refactor. Consumer is not a lifetime task — it completes whenever a worker observes a momentarily empty queue — and both Enqueue overloads performed Queue.Add before reading the guard while the worker exited its TryTake loop before reinstalling one. A barrier layered over that handshake would have reported "drained" while an item was stranded. Adding a barrier that reads as a guarantee but is not one is worse than the present state, in which the ordering constraint is at least honestly unexpressed.

What Changed

Core fix (2 production files)

• QuickFiler/Controllers/FilerQueue.cs — outstanding-work counter, drain signal, WhenDrainedAsync(), ItemProcessor seam, monitor-protected running flag replacing the one-shot guard. The counter decrement is in a finally, so a throwing item still decrements. Consumer is retained with its type, accessibility, and completed-task default.
• QuickFiler/Controllers/QfcFormController.EventHandlers.cs — awaits the drain before the two dispatches, adds the _parent guard clause, deletes the two subsumed Consumer awaits.

Tests (4 files)

• QuickFiler.Test/Controllers/FilerQueueTests.cs — extended with the queue-level drain cases; the class comment recording the deliberate Enqueue/ConsumeAsync exclusion is corrected, since that exclusion no longer holds.
• QuickFiler.Test/Controllers/QfcFormControllerUndoHandoffTests.cs — new; the ordering tests.
• QuickFiler.Test/Controllers/QfcItemController.SeamFactoryTests.cs — reconciled off the private guard field the repair removes, which it previously reached by reflection.
• QuickFiler.Test/QuickFiler.Test.csproj — one Compile Include entry, since the project uses explicit compile items.

Architecture / How It Fits Together

Enqueue takes the monitor, increments the counter, adds to the BlockingCollection, and decides whether to start a worker — all in one critical section, so the decision is atomic with respect to the worker's loop exit. ConsumeAsync drains via TryTake, invokes ItemProcessor inside the existing try/catch, and decrements in a finally; when the counter reaches zero it completes and clears the drain signal. WhenDrainedAsync() returns an already-completed task when nothing is outstanding, and otherwise the signal's task.

BackGroundMoveAsync awaits _groups.MoveEmailsAsync(...), then _parent.FilerQueue.WhenDrainedAsync(), then dispatches metrics and cleanup in that unchanged order. Because MoveEmailsAsync awaits each group's MoveMailAsync sequentially and each enqueues synchronously, the outstanding count observed at that point is an exact upper bound on the batch — which is what makes the counted barrier correct rather than heuristic.

The barrier is awaited off the UI thread, before both UiThread.Dispatcher.InvokeAsync calls, and the monitor is never held across an await.

Verification

Completed

Full C# toolchain, one uninterrupted pass, in order:

Gate	Result
dotnet tool run csharpier check .	exit 0, 1566 files checked, 0 unformatted
msbuild /t:Rebuild ... /p:EnableNETAnalyzers=true /p:EnforceCodeStyleInBuild=true	exit 0, 0 errors, 0 Skipping target "CoreCompile"
msbuild /t:Rebuild ... /p:TreatWarningsAsErrors=true	exit 0, 0 errors, 0 CS0518, 0 CS86xx, 0 skips
Invoke-MSTestWithCoverage.ps1	exit 0, 6924 total, 6924 passed, 0 failed

Both MSBuild gates were run with /t:Rebuild and file logging, and the zero Skipping target "CoreCompile" count is recorded so the gates are demonstrably non-vacuous rather than warm no-ops.

Coverage, first-party filtered denominator, identical nine-package set before and after: line 85.32 percent to 85.39 percent, branch 79.32 percent to 79.40 percent. FilerQueue.cs per-file coverage 36.73 percent to 100.00 percent. 138 changed lines, 0 uncovered.

Fail-before evidence is a genuine failing run for the barrier defect, recorded at evidence/regression-testing/fail-before-run.2026-09-01T10-46.md with the passing counterpart at pass-after-run.2026-09-01T10-55.md. The queue-level drain suite and the orphan-window regression cannot compile against the pre-fix tree, so they are covered by the dossier at evidence/regression-testing/fail-before-exception.2026-09-01T10-48.md rather than by a failing run.

All twenty acceptance criteria in spec.md pass. Feature review returned zero blocking findings and re-derived the coverage figures, the formatter result, and the CoreCompile counts from primary sources rather than reading the recorded values.

Recommended

• Live Outlook session: batch-move several emails, then immediately open the undo dialog and confirm every moved item is present.
• Confirm the metrics written for the session are unchanged in shape and content against a pre-change run.

Backward Compatibility / Migration Notes

Additive on the public surface. WhenDrainedAsync() is new; ItemProcessor is internal and reachable from tests through the existing InternalsVisibleTo. Consumer keeps its declaration, accessibility, and Task.CompletedTask default, and both Enqueue overloads keep their signatures and exception behaviour — including the synchronous ArgumentNullException in the caller's frame that QfcItemController.MoveMailAsync wraps.

The only user-observable difference is timing: the batch-move task now completes after the batch has been filed rather than after it has been enqueued. That wait occurs after the next group is loaded and displayed, and ButtonOK_Click is async void and has already yielded, so the message loop is not blocked.

No configuration key, no persisted state, no schema change. Rollback is a revert of the commits.

Risks and Mitigations

• A leaked count would leave the drain permanently incomplete and hang the batch-move path, which is worse than the defect being fixed. Mitigated by putting the decrement in a finally and by the regression ItemProcessor_ThatThrows_StillDecrementsAndDrainCompletes. See the first follow-up below for a residual case.
• The barrier never completes because no worker was started. This is the pre-existing orphan window; the handshake repair closes it and Enqueue_AfterPreviousBatchDrained_ProcessesSecondBatch pins it.
• UI-thread deadlock. The filing path does not require the UI thread; the loop runs inside Task.Run, and the barrier is awaited before any dispatcher operation is in flight.
• A perceived pause after confirming a batch move. Bounded by filing work the code already performed. No numeric latency budget is asserted, because no timing telemetry exists in the repository from which to derive one.

Review Guide

1. QuickFiler/Controllers/FilerQueue.cs — the critical sections are the substance of the change. Check that the counter, the queue add, and the start/stop decision are all under one monitor and that the monitor is never held across an await.
2. QuickFiler/Controllers/QfcFormController.EventHandlers.cs — small, and the placement of the awaited drain relative to the two dispatches is the whole point.
3. QuickFiler.Test/Controllers/QfcFormControllerUndoHandoffTests.cs — the determinism argument is worth reading closely. Ordering is established by a probe operation posted at equal priority to a pinned dispatcher, not by elapsed time; there is no sleep, delay, or polling anywhere in the added tests.
4. The remaining docs/ paths are evidence and audit artifacts, mechanical and safe to skim.

Follow-ups

These were found during review and are deliberately not addressed here, because the change is bounded by a footprint acceptance criterion that permits only the two production files, QuickFiler.Test/, and docs/. They will be filed separately.

• FilerQueue can leak the consumer-running flag (Major). The flag is cleared only on the normal loop-exit path. The catch handler's own body sits outside the try/catch/finally, so an exception raised inside the diagnostic — for example Helpers.First() on an empty list, which the FilerQueueItem constructor permits — escapes the worker loop and leaves the flag set. Enqueue((FilerQueueItem)null) is a second route. Neither is reachable from the single production call site today, but the consequence post-fix is a hang rather than a delay. Suggested fix: a try/finally around the loop clearing the flag under the monitor, a null-and-empty-safe diagnostic, and a null guard on the item overload.
• ConsumeAsync is public and now participates in an invariant it does not establish; an external call could reopen the window this change closes. The sibling queue in TaskVisualization declares the same method internal. Sealing it would be a breaking change, so the minimum step is a documented warning.
• The barrier is queue-wide rather than per-batch, which is correct under the current single-producer topology but would change meaning if a second producer appeared. It also takes no CancellationToken and has no upper bound.
• DASLFilterParserTests needs [DoNotParallelize]. It uses Console.SetOut and races other tests; the repository already applies that attribute to PrettyPrint_Tests for the identical hazard. Out of footprint here.
• QfcFormController.EventHandlers.cs per-file coverage is 49.41 percent, below the modified-file floor. Pre-existing at 45.38 percent, improved by 4 points, with zero uncovered changed lines; the remainder is untouched Outlook-interop and WinForms handler code. Belongs in a dedicated coverage-uplift issue.
• The same latent handshake window exists in TaskVisualization/FlagChangeTrainingQueue.cs, a different type with a different consumer, recorded as a non-goal in the spec.

GitHub Auto-close

• Closes Bug: qfc-unsynchronized-undo-handoff-after-batch-move #633