fix(ribbon): contain a throwing log sink at both engine-toggle coordinator call sites

TaskMaster.Test/Ribbon/EngineToggleStateCoordinatorTests.ThrowingSink.cs

@@ -0,0 +1,215 @@
+using System;
+using System.Threading.Tasks;
+using FluentAssertions;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Moq;
+
+namespace TaskMaster.Test.Ribbon
+{
+    /// <summary>
+    /// Regression tests for issue #947: a <c>logError</c> sink that throws while a faulted or
+    /// canceled prime is reported must not leave the prime marker registered, must not move the
+    /// report after the clear, and must leave no faulted task behind; a sink that throws while
+    /// the click boundary reports a toggle fault must not escape that boundary. A fifth partial
+    /// of the coordinator fixture, so the private <c>Harness</c> and <c>LoggedError</c> types
+    /// and the fixture constants are reused without adding any harness member. The harness
+    /// invokes <c>OnLogError</c> after it has recorded the error, so a throwing hook both
+    /// records the report and models a throwing sink.
+    /// </summary>
+    public partial class EngineToggleStateCoordinatorTests
+    {
+        #region Issue #947 — a throwing log sink leaves no stale prime marker
+
+        /// <summary>
+        /// Regression for issue #947 and the faulted variant that carries the fail-before
+        /// obligation. Invariant: when the sink throws while a faulted prime is reported, the
+        /// marker is still removed, so a later read starts a new prime. The first prime handle is
+        /// captured before the trigger, because the fixed code clears the marker before that
+        /// handle completes. Without the fix the sink exception skips the clear, the later read
+        /// finds the stale marker and starts nothing, and the activation read is verified once
+        /// rather than twice. No sleep, delay, timer or parallelism attribute.
+        /// </summary>
+        [TestMethod]
+        public async Task GetPressed_WhenLogSinkThrowsOnFaultedPrime_LaterReadStartsNewPrime()
+        {
+            // Arrange
+            var harness = new Harness();
+            var probe = new TaskCompletionSource<bool>();
+            var failure = new InvalidOperationException("configuration load failed");
+            harness
+                .Engines.SetupSequence(x => x.EngineActiveAsync(SpamEngine))
+                .Returns(probe.Task)
+                .Returns(Task.FromResult(true));
+            harness.OnLogError = (_, _) => throw new InvalidOperationException("sink failed");
+            harness.Coordinator.GetPressed(SpamEngine);
+            var firstPrime = harness.Coordinator.GetPrimeTask(SpamEngine);
+
+            // Act
+            probe.SetException(failure);
+            await firstPrime;
+            harness.Coordinator.GetPressed(SpamEngine);
+            var secondPrime = harness.Coordinator.GetPrimeTask(SpamEngine);
+
+            // Assert
+            harness.Engines.Verify(
+                x => x.EngineActiveAsync(SpamEngine),
+                Times.Exactly(2),
+                "a throwing sink leaves no marker behind, so the later read starts a new prime"
+            );
+            secondPrime.Should().NotBeSameAs(firstPrime, "the later read registered a new prime");
+            await secondPrime;
+            harness.Errors.Should().ContainSingle("the sink was invoked once before it threw");
+            harness
+                .Errors[0]
+                .Exception.Should()
+                .BeSameAs(failure, "the sink receives the injected exception unchanged");
+            harness
+                .Coordinator.GetPressed(SpamEngine)
+                .Should()
+                .BeTrue("the new prime read the engine as active and cached that value");
+            harness
+                .Invalidations.Should()
+                .Equal(
+                    new[] { SpamToggleControlId },
+                    "only the successful prime changed state to display"
+                );
+        }
+
+        /// <summary>
+        /// Regression for issue #947, canceled variant: when the sink throws while a canceled
+        /// prime is reported with the synthesized cancellation exception, the marker is still
+        /// removed and a later read starts a new prime. Fails before the fix for the same reason
+        /// as the faulted variant.
+        /// </summary>
+        [TestMethod]
+        public async Task GetPressed_WhenLogSinkThrowsOnCanceledPrime_LaterReadStartsNewPrime()
+        {
+            // Arrange
+            var harness = new Harness();
+            var probe = new TaskCompletionSource<bool>();
+            harness
+                .Engines.SetupSequence(x => x.EngineActiveAsync(SpamEngine))
+                .Returns(probe.Task)
+                .Returns(Task.FromResult(true));
+            harness.OnLogError = (_, _) => throw new InvalidOperationException("sink failed");
+            harness.Coordinator.GetPressed(SpamEngine);
+            var firstPrime = harness.Coordinator.GetPrimeTask(SpamEngine);
+
+            // Act
+            probe.SetCanceled();
+            await firstPrime;
+            harness.Coordinator.GetPressed(SpamEngine);
+            var secondPrime = harness.Coordinator.GetPrimeTask(SpamEngine);
+
+            // Assert
+            harness.Engines.Verify(
+                x => x.EngineActiveAsync(SpamEngine),
+                Times.Exactly(2),
+                "a throwing sink leaves no marker behind, so the later read starts a new prime"
+            );
+            secondPrime.Should().NotBeSameAs(firstPrime, "the later read registered a new prime");
+            await secondPrime;
+            harness.Errors.Should().ContainSingle("the sink was invoked once before it threw");
+            harness
+                .Errors[0]
+                .Exception.Should()
+                .BeAssignableTo<OperationCanceledException>(
+                    "a canceled task carries no exception to unwrap, so one is synthesized"
+                );
+            harness
+                .Coordinator.GetPressed(SpamEngine)
+                .Should()
+                .BeTrue("the new prime read the engine as active and cached that value");
+            harness
+                .Invalidations.Should()
+                .Equal(
+                    new[] { SpamToggleControlId },
+                    "only the successful prime changed state to display"
+                );
+        }
+
+        /// <summary>
+        /// Regression for issue #947, the no-unobserved-fault guarantee. The hook probes the prime
+        /// handle from inside the throwing sink, so report-then-clear is observed under a throwing
+        /// sink as well. The first prime handle, captured before the trigger, must end
+        /// ran-to-completion, and once it has completed the marker must be cleared, which is only
+        /// possible when the sink exception was contained inside the coordinator. Without the fix
+        /// the marker stays registered after the handle completes.
+        /// </summary>
+        [TestMethod]
+        public async Task GetPressed_WhenLogSinkThrows_FirstPrimeCompletesAndMarkerIsCleared()
+        {
+            // Arrange
+            var harness = new Harness();
+            var probe = new TaskCompletionSource<bool>();
+            var failure = new InvalidOperationException("configuration load failed");
+            harness.Engines.Setup(x => x.EngineActiveAsync(SpamEngine)).Returns(probe.Task);
+            harness.Coordinator.GetPressed(SpamEngine);
+            var firstPrime = harness.Coordinator.GetPrimeTask(SpamEngine);
+            Task handleSeenBySink = null;
+            harness.OnLogError = (_, _) =>
+            {
+                handleSeenBySink = harness.Coordinator.GetPrimeTask(SpamEngine);
+                throw new InvalidOperationException("sink failed");
+            };
+
+            // Act
+            probe.SetException(failure);
+            await firstPrime;
+
+            // Assert
+            firstPrime
+                .Status.Should()
+                .Be(TaskStatus.RanToCompletion, "the prime handle never faults");
+            handleSeenBySink
+                .Should()
+                .BeSameAs(firstPrime, "the report is attempted before the marker is cleared");
+            harness.Errors.Should().ContainSingle("the sink was invoked once before it threw");
+            harness
+                .Errors[0]
+                .Exception.Should()
+                .BeSameAs(failure, "the sink receives the injected exception unchanged");
+            harness.Invalidations.Should().BeEmpty("a failed prime leaves nothing to display");
+            harness
+                .Coordinator.GetPrimeTask(SpamEngine)
+                .Should()
+                .BeSameAs(
+                    Task.CompletedTask,
+                    "the sink exception is contained, so the marker is still cleared"
+                );
+        }
+
+        /// <summary>
+        /// Regression for issue #947, the click-boundary call site. Invariant: when the toggle
+        /// faults and the sink then throws, the click boundary still attempts the report and
+        /// does not throw, because its caller is an <c>async void</c> Office handler. Without
+        /// the fix the sink exception escapes the boundary, so the awaited call faults with the
+        /// sink exception.
+        /// </summary>
+        [TestMethod]
+        public async Task HandleToggleClickAsync_WhenLogSinkThrowsOnToggleFault_DoesNotThrowAndAttemptsReport()
+        {
+            // Arrange
+            var harness = new Harness();
+            var failure = new InvalidOperationException("toggle failed");
+            harness.Engines.Setup(x => x.ToggleEngineAsync(SpamEngine)).ThrowsAsync(failure);
+            harness.OnLogError = (_, _) => throw new InvalidOperationException("sink failed");
+
+            // Act
+            Func<Task> act = () => harness.Coordinator.HandleToggleClickAsync(SpamEngine);
+
+            // Assert
+            await act.Should()
+                .NotThrowAsync("the click boundary contains a failure of the sink itself");
+            harness.Errors.Should().ContainSingle("the sink was invoked once before it threw");
+            harness
+                .Errors[0]
+                .Exception.Should()
+                .BeSameAs(failure, "the sink receives the toggle fault unchanged");
+            harness.Invalidations.Should().BeEmpty("a failed toggle changed no state to display");
+            harness.Notifications.Should().BeEmpty("a fault is logged, not surfaced as a notice");
+        }
+
+        #endregion Issue #947 — a throwing log sink leaves no stale prime marker
+    }
+}

TaskMaster.Test/TaskMaster.Test.csproj

@@ -359,6 +359,7 @@
     <Compile Include="Ribbon\EngineToggleStateCoordinatorTests.Race.cs" />
     <Compile Include="Ribbon\EngineToggleStateCoordinatorTests.PrimeFaultOrdering.cs" />
     <Compile Include="Ribbon\EngineToggleStateCoordinatorTests.PrimeRegistration.cs" />
+    <Compile Include="Ribbon\EngineToggleStateCoordinatorTests.ThrowingSink.cs" />
     <Compile Include="Ribbon\EngineTogglePressedStateCacheTests.cs" />
     <Compile Include="Properties\AssemblyInfo.cs" />
   </ItemGroup>

TaskMaster/Ribbon/EngineToggleStateCoordinator.cs

@@ -151,8 +151,8 @@ internal bool GetPressed(string engineName)
         }
 
         /// <summary>
-        /// The toggle-click boundary: the only place in this type that observes a fault with a
-        /// <c>catch</c> clause.
+        /// The toggle-click boundary: the only <c>catch</c> clause in this type that observes an
+        /// engine fault. The other two are sink guards, here and in <see cref="CompletePrime"/>.
         /// </summary>
         /// <param name="engineName">The engine key whose activation setting is being flipped.</param>
         /// <returns>
@@ -164,8 +164,11 @@ internal bool GetPressed(string engineName)
         /// <c>notifyUnavailable</c> message and nothing else is invoked. Otherwise
         /// <see cref="ExecuteToggleAsync"/> runs inside a single boundary <c>try</c>/<c>catch</c>:
         /// a fault is reported through <c>logError</c>, is not rethrown, and does not invalidate.
-        /// This method never throws, because its caller is an <c>async void</c> Office handler
-        /// whose faults would otherwise become unobserved.
+        /// The sink call is itself guarded (issue #947): the sink is the last reporting channel,
+        /// so a failure inside it has nowhere else to go and is discarded deliberately, following
+        /// <c>RibbonCommandBoundary.SafeLog</c>. This method therefore never throws, even when the
+        /// sink throws, because its caller is an <c>async void</c> Office handler whose faults
+        /// would otherwise become unobserved.
         /// </remarks>
         internal async Task HandleToggleClickAsync(string engineName)
         {
@@ -181,7 +184,14 @@ internal async Task HandleToggleClickAsync(string engineName)
             }
             catch (Exception ex)
             {
-                _logError(BuildToggleFailedMessage(engineName), ex);
+                try
+                {
+                    _logError(BuildToggleFailedMessage(engineName), ex);
+                }
+                catch (Exception)
+                {
+                    // Intentionally discarded: see the remarks on this method.
+                }
             }
         }
 
@@ -244,8 +254,9 @@ internal async Task ExecuteToggleAsync(string engineName)
         /// The prime task, or <see cref="Task.CompletedTask"/> when no prime has been started for
         /// the key. The returned task never faults: a prime fault is observed inside the prime
         /// itself and reported through <c>logError</c>. For a key whose prime did not run to
-        /// completion, the marker is cleared only after that report has returned, so a caller that
-        /// receives <see cref="Task.CompletedTask"/> can rely on the fault having been reported.
+        /// completion, the marker is cleared only after that report has returned or thrown, so a
+        /// caller that receives <see cref="Task.CompletedTask"/> can rely on the report having
+        /// been attempted.
         /// </returns>
         internal Task GetPrimeTask(string engineName)
         {
@@ -292,13 +303,15 @@ private void StartPrimeIfNeeded(string engineName, string controlId)
         /// Runs <see cref="ApplyPrimeAsync"/> and attaches the fault observer.
         /// </summary>
         /// <remarks>
-        /// The observer is a continuation rather than a <c>catch</c> clause, so this type keeps
-        /// exactly one <c>catch</c> — the click boundary. Reading
+        /// The observer is a continuation rather than a <c>catch</c> clause. The three
+        /// <c>catch</c> clauses in this type all sit in <see cref="HandleToggleClickAsync"/> and
+        /// <see cref="CompletePrime"/>: the click boundary and the two sink guards. Reading
         /// <see cref="Task.Exception"/> inside <see cref="CompletePrime"/> marks the fault
         /// observed, so no unobserved task remains. The continuation task itself is discarded;
         /// the value a test awaits is the marker, which the continuation completes only through
         /// <c>SetResult</c> in a <c>finally</c> after <see cref="CompletePrime"/> exits, so it
-        /// never faults or cancels.
+        /// never faults or cancels. Because <see cref="CompletePrime"/> also contains a failure
+        /// of the sink, the discarded continuation has no remaining throw source of its own.
         /// </remarks>
         private void StartObservedPrime(
             IAppItemEngines engines,
@@ -352,16 +365,28 @@ string controlId
         /// Observes the outcome of a prime. On any outcome other than ran-to-completion the cache
         /// is left unset — so the key still reports unchecked — the failure is reported through
         /// <c>logError</c>, and only then is the in-flight marker cleared so a later read may
-        /// re-prime.
+        /// re-prime. A failure thrown by the sink itself is contained here, so the marker is
+        /// cleared whether or not the report succeeded.
         /// </summary>
         /// <remarks>
+        /// <para>
         /// The status is tested rather than the exception. A CANCELED task carries a null
         /// <see cref="Task.Exception"/>, so a handler keyed on the exception returned early for a
         /// cancellation: nothing was logged, the cache stayed unset, and the in-flight marker stayed
         /// registered, which blocked any re-prime for the rest of the session. When there is no
         /// exception to unwrap a <see cref="TaskCanceledException"/> is synthesized so the sink
         /// always receives one. The faulted path is unchanged and still reports the unwrapped base
         /// exception.
+        /// </para>
+        /// <para>
+        /// The sink call is guarded (issue #947). The sink is the last reporting channel of this
+        /// type, so a failure inside it has nowhere else to go; letting it escape skipped the clear
+        /// below, which left a stale marker that blocked every later re-prime, and faulted the
+        /// discarded continuation unobserved. The guard follows
+        /// <c>RibbonCommandBoundary.SafeLog</c>. With the sink contained, the continuation in
+        /// <see cref="StartObservedPrime"/> has no remaining throw source of its own, so it
+        /// completes rather than faulting.
+        /// </para>
         /// </remarks>
         private void CompletePrime(Task completed, string engineName)
         {
@@ -375,9 +400,18 @@ private void CompletePrime(Task completed, string engineName)
                 ?? new TaskCanceledException(completed);
 
             // Report-then-clear is load-bearing: the marker stays registered until the report has
-            // returned, so a caller that observes the marker absent — including one that fetched the
-            // prime handle after the fault — is guaranteed the fault has already been reported.
-            _logError(BuildPrimeFailedMessage(engineName), failure);
+            // returned or thrown, so a caller that observes the marker absent — including one that
+            // fetched the prime handle after the fault — is guaranteed the report has already been
+            // attempted.
+            try
+            {
+                _logError(BuildPrimeFailedMessage(engineName), failure);
+            }
+            catch (Exception)
+            {
+                // Intentionally discarded: see the remarks on this method.
+            }
+
             _primeTasks.TryRemove(engineName, out _);
         }