fix(quickfiler): resolve issue #726 code-correctness sweep (6 of 8 findings)

QuickFiler.Test/Controllers/EfcViewerTests.cs

@@ -119,6 +119,24 @@ public void ClaimsAltChord_WithBareAltAndHandler_ReturnsTrue()
                 .BeTrue("bare Alt is the chord the keyboard dialog services");
         }
 
+        // Issue #726 finding 8: ClaimsAltChord's guard is `keyCode == Keys.Menu || keyCode ==
+        // Keys.None` -- the bare-Alt test above only exercises the Keys.None disjunct (Keys.Alt
+        // carries no key-code bits). Keys.Menu | Keys.Alt is the VK_MENU keystroke shape (the Alt
+        // key itself pressed as a key) and exercises the other disjunct; without this test the
+        // Keys.Menu arm of the predicate was deletable without failing any existing test.
+        [TestMethod]
+        public void ClaimsAltChord_WithMenuKeyAndAlt_ReturnsTrue()
+        {
+            var handler = new Mock<IQfcKeyboardHandler>();
+
+            EfcViewer
+                .ClaimsAltChord(handler.Object, Keys.Menu | Keys.Alt)
+                .Should()
+                .BeTrue(
+                    "Keys.Menu | Keys.Alt is the VK_MENU keystroke shape of the bare Alt chord"
+                );
+        }
+
         [TestMethod]
         public void ClaimsAltChord_WithAltF_ReturnsFalse()
         {

QuickFiler.Test/Controllers/FilerQueueTests.cs

@@ -97,9 +97,9 @@ private static TaskCompletionSource<bool> NewGate() =>
             new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
 
         /// <summary>
-        /// Enqueues one item. Every enqueued item carries a real helper because the preserved
-        /// worker <c>catch</c> block calls <c>item.Helpers.First()</c>; an empty list would raise
-        /// inside the catch, escape the worker loop, and leave the drain permanently incomplete.
+        /// Enqueues one item carrying a real helper, for tests that are not themselves about the
+        /// empty-helpers diagnostic path fixed under issue #726 (see
+        /// <see cref="ConsumeAsync_ItemWithEmptyHelpersThrows_ConsumerRecoversForLaterItems"/>).
         /// </summary>
         private static void EnqueueOne(FilerQueue queue) =>
             queue.Enqueue(new EmailFiler(), OneHelper());
@@ -354,5 +354,76 @@ public async Task ItemProcessor_ThatThrows_StillDecrementsAndDrainCompletes()
             drain.IsCompleted.Should().BeTrue("the throwing item still decrements the counter");
             invocations.Should().Be(2, "the worker loop continues past the failing item");
         }
+
+        [TestMethod]
+        public void Enqueue_NullItem_ThrowsArgumentNullException()
+        {
+            // Arrange
+            var queue = new FilerQueue();
+
+            // Act
+            Action act = () => queue.Enqueue((FilerQueueItem)null);
+
+            // Assert
+            act.Should()
+                .Throw<ArgumentNullException>(
+                    "issue #726 finding 1: a null item must never reach the queue -- it previously "
+                        + "left the outstanding counter permanently incremented, since nothing was "
+                        + "actually queued to decrement it"
+                );
+        }
+
+        /// <summary>
+        /// Regression test for issue #726 finding 1. Before the fix, an item constructed with an
+        /// empty (not null) helpers list -- which <see cref="FilerQueueItem"/>'s constructor
+        /// permits -- would cause <c>item.Helpers.First()</c> inside the worker's diagnostic catch
+        /// handler to throw <see cref="InvalidOperationException"/>. That exception escaped the
+        /// catch itself, unwound the entire worker loop, and left the consumer-running flag
+        /// permanently set with no <c>try</c>/<c>finally</c> to clear it -- so no later Enqueue
+        /// call would ever start a new worker again, permanently hanging the background mover.
+        /// </summary>
+        [TestMethod]
+        public async Task ConsumeAsync_ItemWithEmptyHelpersThrows_ConsumerRecoversForLaterItems()
+        {
+            // Arrange
+            var queue = new FilerQueue();
+            var emptyHelpersItem = new FilerQueueItem(new EmailFiler(), new List<MailItemHelper>());
+            TaskCompletionSource<bool> secondProcessed = NewGate();
+            int invocations = 0;
+            queue.ItemProcessor = item =>
+            {
+                int index = Interlocked.Increment(ref invocations) - 1;
+                if (index == 0)
+                {
+                    // Provoked by an EMPTY (not null) Helpers list, exactly like the diagnostic
+                    // path's own item.Helpers.First() would throw before the fix.
+                    throw new InvalidOperationException(
+                        "processing fails for the empty-helpers item"
+                    );
+                }
+
+                secondProcessed.TrySetResult(true);
+                return Task.CompletedTask;
+            };
+
+            // Act: enqueue the poisoning item, wait for the queue to fully drain (proving the
+            // worker did not hang), then enqueue a second, normal item and confirm a NEW worker
+            // starts and processes it -- which only happens if the consumer-running flag was
+            // actually cleared rather than left stuck by the first item's escaping exception.
+            queue.Enqueue(emptyHelpersItem);
+            await queue.WhenDrainedAsync();
+            EnqueueOne(queue);
+            await secondProcessed.Task;
+            await queue.WhenDrainedAsync();
+
+            // Assert
+            invocations
+                .Should()
+                .Be(
+                    2,
+                    "the consumer-running flag must clear on the exceptional exit path so a later "
+                        + "Enqueue can start a fresh worker, not just on the normal empty-queue exit"
+                );
+        }
     }
 }

QuickFiler/Controllers/EfcFormController.cs

@@ -128,6 +128,33 @@ internal EfcFormController InitializeDataFields(EfcDataModel dataModel)
         internal System.Action<string, System.Exception> BoundaryErrorSink { get; set; } =
             (message, exception) => logger.Error(message, exception);
 
+        /// <summary>
+        /// Issue #726 finding 5: invokes <see cref="BoundaryErrorSink"/> defensively so a null or
+        /// throwing sink delegate cannot silently reinstate the unobserved-fault behavior this
+        /// boundary exists to prevent -- these call sites all sit in an <c>async void</c> handler's
+        /// catch block, where an escaping exception would crash the process rather than merely fail
+        /// to log.
+        /// </summary>
+        private void TryReportBoundaryFault(string message, System.Exception exception)
+        {
+            var sink = BoundaryErrorSink;
+            if (sink is null)
+            {
+                logger.Error(message, exception);
+                return;
+            }
+
+            try
+            {
+                sink(message, exception);
+            }
+            catch (System.Exception sinkException)
+            {
+                logger.Error($"{message} (and the error sink itself threw)", sinkException);
+                logger.Error(message, exception);
+            }
+        }
+
         private IApplicationGlobals _globals;
         private System.Action _parentCleanup;
         private EfcDataModel _dataModel;
@@ -453,7 +480,7 @@ internal async Task ButtonCancelClickAsync()
             }
             catch (System.Exception ex)
             {
-                BoundaryErrorSink(ex.Message, ex);
+                TryReportBoundaryFault(ex.Message, ex);
             }
         }
 
@@ -470,7 +497,7 @@ internal async Task ButtonOkClickAsync()
             }
             catch (System.Exception ex)
             {
-                BoundaryErrorSink(ex.Message, ex);
+                TryReportBoundaryFault(ex.Message, ex);
             }
         }
 
@@ -488,7 +515,7 @@ internal async Task ButtonRefreshClickAsync()
             }
             catch (System.Exception ex)
             {
-                BoundaryErrorSink(ex.Message, ex);
+                TryReportBoundaryFault(ex.Message, ex);
             }
         }
 
@@ -550,7 +577,7 @@ await _dataModel.MoveToFolderAsync(
             }
             catch (System.Exception ex)
             {
-                BoundaryErrorSink(ex.Message, ex);
+                TryReportBoundaryFault(ex.Message, ex);
             }
         }
 
@@ -565,7 +592,7 @@ internal async Task ButtonDeleteClickAsync()
             }
             catch (System.Exception ex)
             {
-                BoundaryErrorSink(ex.Message, ex);
+                TryReportBoundaryFault(ex.Message, ex);
             }
         }
 
@@ -1135,7 +1162,7 @@ public async Task PopulateFolderCombobox(object folderList = null)
             }
             catch (System.Exception ex)
             {
-                BoundaryErrorSink(ex.Message, ex);
+                TryReportBoundaryFault(ex.Message, ex);
             }
         }
 

QuickFiler/Controllers/EfcItemController.WebViewFaultBoundary.cs

@@ -0,0 +1,67 @@
+using System;
+using System.Threading.Tasks;
+
+namespace QuickFiler.Controllers
+{
+    internal partial class EfcItemController
+    {
+        /// <summary>
+        /// Issue #726 finding 4: fault-boundary sink for <see cref="InitializeWebViewGuardedAsync"/>,
+        /// mirroring <c>QfcItemController.WebViewInitializationErrorSink</c>. Named distinctly so no
+        /// shared contract with the QFC sink or with <see cref="EfcFormController.BoundaryErrorSink"/>
+        /// is implied.
+        /// </summary>
+        internal Action<string, Exception> WebViewInitializationErrorSink { get; set; } =
+            (message, exception) => logger.Error(message, exception);
+
+        /// <summary>
+        /// Issue #726 finding 4: fault boundary for <see cref="InitializeWebViewAsync"/>. Both
+        /// production call sites previously discarded the task returned by
+        /// <c>Task.Run(() =&gt; InitializeWebViewAsync())</c>, so a fault there was never observed --
+        /// under .NET Framework 4.5+, a discarded faulted task is silently finalized with no
+        /// diagnostic. This member contains the fault instead of returning it: the task it returns
+        /// never transitions to Faulted.
+        /// </summary>
+        internal async Task InitializeWebViewGuardedAsync()
+        {
+            try
+            {
+                await InitializeWebViewAsync();
+            }
+            catch (OperationCanceledException)
+            {
+                // Cooperative cancellation during teardown is expected and is not a fault.
+            }
+            catch (Exception ex)
+            {
+                // Issue #726 finding 5: guard against a null or throwing sink delegate so a
+                // misconfigured sink cannot silently reinstate the unobserved-fault behavior this
+                // boundary exists to prevent.
+                TryReportWebViewInitializationFault(ex);
+            }
+        }
+
+        private void TryReportWebViewInitializationFault(Exception ex)
+        {
+            var sink = WebViewInitializationErrorSink;
+            if (sink is null)
+            {
+                logger.Error("WebView2 initialization failed.", ex);
+                return;
+            }
+
+            try
+            {
+                sink("WebView2 initialization failed.", ex);
+            }
+            catch (Exception sinkException)
+            {
+                logger.Error(
+                    "WebView2 initialization failed, and the error sink itself threw.",
+                    sinkException
+                );
+                logger.Error("Original WebView2 initialization failure.", ex);
+            }
+        }
+    }
+}

QuickFiler/Controllers/EfcItemController.cs

@@ -23,7 +23,7 @@
 namespace QuickFiler.Controllers
 {
     [ExcludeFromCodeCoverage]
-    internal class EfcItemController : IItemControler
+    internal partial class EfcItemController : IItemControler
     {
         #region Constructors and Initializers
 
@@ -94,7 +94,9 @@ public EfcItemController InitializeDataFields(EfcDataModel dataModel)
             PopulateControls(dataModel);
             PopulateConversation();
             WireEvents();
-            Task.Run(() => InitializeWebViewAsync());
+            // Issue #726 finding 4: routed through the guarded wrapper so a fault is logged
+            // instead of silently finalized away as an unobserved discarded-task exception.
+            _ = InitializeWebViewGuardedAsync();
             return this;
         }
 
@@ -150,7 +152,9 @@ private void Initialize(bool async)
             _itemPositionTips.Toggle(Enums.ToggleState.Off, shareColumn: true);
 
             WireEvents();
-            Task.Run(() => InitializeWebViewAsync());
+            // Issue #726 finding 4: routed through the guarded wrapper so a fault is logged
+            // instead of silently finalized away as an unobserved discarded-task exception.
+            _ = InitializeWebViewGuardedAsync();
         }
 
         private static readonly log4net.ILog logger = log4net.LogManager.GetLogger(