From f38d7a7ede460a4043a21f578e9345072aadf511 Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Wed, 23 Sep 2026 03:51:55 +0000 Subject: [PATCH 1/2] [patch] Do not let a refused preference restore kill the tray Restoring a remembered toggle calls the tool's setter, and that setter can refuse for exactly the reason a click can: restoring "keep awake: on" asks the tool to take an inhibitor, and the machine may have none to give. The call sat bare in LoadPreferences, outside any handling, and it runs before there is a menu to fail into - so the exception escaped RunAsync and ended the process before the tray ever appeared. That is the one failure mode this library exists to prevent, in the library's own startup path, and it is a regression against the tools it is meant to replace: NoSleep catches SleepBlockException and PlatformNotSupportedException around its startup Enable() and still shows the tray. The refusal is now caught per toggle, written to standard error, and handed back for the menu's status line through TrayMenu.SeedError, which the next successful action clears like any other failure. The toggles after the one that refused are still restored, and the tray still comes up saying why. Verified: the refusal path and the seeded status line have tests; and with a real status-notifier host under xvfb, the demo's remembered toggle still round trips across a restart - click it off, and the next process starts with the entry unchecked and the status line already reading "idle". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qksp7NcKB2jMJjXuxGS3nP --- TrayApp.Test/TrayAppBuilderTests.cs | 63 ++++++++++++++++++++++++++++- TrayApp.Test/TrayMenuTests.cs | 17 ++++++++ TrayApp/Menu/TrayMenu.cs | 10 +++++ TrayApp/TrayAppBuilder.cs | 48 +++++++++++++++++++--- 4 files changed, 131 insertions(+), 7 deletions(-) diff --git a/TrayApp.Test/TrayAppBuilderTests.cs b/TrayApp.Test/TrayAppBuilderTests.cs index c813a53..d563747 100644 --- a/TrayApp.Test/TrayAppBuilderTests.cs +++ b/TrayApp.Test/TrayAppBuilderTests.cs @@ -109,7 +109,7 @@ public async Task LoadPreferences_RestoresOnlyThePersistedToggles() .Build(); using DebouncedPreferenceStore store = new(provider, "tray", Debounce); - TrayAppBuilder.LoadPreferences(app, store); + _ = TrayAppBuilder.LoadPreferences(app, store); Assert.IsTrue(remembered); Assert.IsFalse(forgotten); @@ -137,7 +137,7 @@ public async Task LoadPreferences_ThenOptions_LetsAnExplicitSwitchWin() TrayAppDefinition app = builder.Build(); using DebouncedPreferenceStore store = new(provider, "tray", Debounce); - TrayAppBuilder.LoadPreferences(app, store); + _ = TrayAppBuilder.LoadPreferences(app, store); Assert.IsTrue(running, "the remembered value should be restored first"); Assert.IsTrue(CommandLineParser.TryParse(["--off"], app.Options, out CommandLineOptions options, out _)); @@ -147,6 +147,65 @@ public async Task LoadPreferences_ThenOptions_LetsAnExplicitSwitchWin() Assert.IsFalse(running); } + [TestMethod] + public async Task LoadPreferences_WhenTheToolRefuses_KeepsGoingAndHandsBackTheMessage() + { + InMemoryPersistenceProvider provider = new(); + + using (DebouncedPreferenceStore seed = new(provider, "tray", Debounce)) + { + await seed.LoadAsync().ConfigureAwait(false); + seed.Set("awake", true); + seed.Set("display", true); + await seed.FlushAsync().ConfigureAwait(false); + } + + bool display = false; + + TrayAppDefinition app = TrayAppBuilder.Create("demo") + .Toggle("Keep awake", () => false, _ => throw new InvalidOperationException("no inhibitor here"), persistAs: "awake") + .Toggle("Keep display awake", () => display, value => display = value, persistAs: "display") + .Preferences(provider) + .Build(); + + using DebouncedPreferenceStore store = new(provider, "tray", Debounce); + + // Restoring "on" asks the tool to take an inhibitor, and the machine may have none to give. That + // refusal happens before there is a menu to fail into; letting it escape would end the process + // before the tray appeared. + string? error = TrayAppBuilder.LoadPreferences(app, store); + + Assert.AreEqual("no inhibitor here", error); + + // The toggle after the one that refused is still restored. + Assert.IsTrue(display); + } + + [TestMethod] + public async Task LoadPreferences_WhenNothingRefuses_HandsBackNoMessage() + { + InMemoryPersistenceProvider provider = new(); + + using (DebouncedPreferenceStore seed = new(provider, "tray", Debounce)) + { + await seed.LoadAsync().ConfigureAwait(false); + seed.Set("awake", true); + await seed.FlushAsync().ConfigureAwait(false); + } + + bool awake = false; + + TrayAppDefinition app = TrayAppBuilder.Create("demo") + .Toggle("Keep awake", () => awake, value => awake = value, persistAs: "awake") + .Preferences(provider) + .Build(); + + using DebouncedPreferenceStore store = new(provider, "tray", Debounce); + + Assert.IsNull(TrayAppBuilder.LoadPreferences(app, store)); + Assert.IsTrue(awake); + } + [TestMethod] public void TryApplyOptions_WhenAHandlerRejectsTheValue_ReportsItAsAUsageError() { diff --git a/TrayApp.Test/TrayMenuTests.cs b/TrayApp.Test/TrayMenuTests.cs index 40ef800..446f965 100644 --- a/TrayApp.Test/TrayMenuTests.cs +++ b/TrayApp.Test/TrayMenuTests.cs @@ -122,6 +122,23 @@ public void Activate_WhenTheToolRefuses_KeepsTheMessageAndSurvivesTheRefresh() Assert.AreEqual("idle", menu.StatusItem.Header); } + [TestMethod] + public void SeedError_ShowsInTheStatusLineAndClearsOnTheNextSuccess() + { + TrayMenu menu = BuildMenu(() => "idle"); + + // A refusal while restoring a remembered toggle happens before the menu exists, so the host seeds it. + menu.SeedError("no inhibitor here"); + menu.Refresh(); + + Assert.AreEqual("no inhibitor here", menu.LastError); + Assert.AreEqual("Demo failed: no inhibitor here", menu.StatusItem!.Header); + + Assert.IsTrue(menu.Run(() => { })); + Assert.IsNull(menu.LastError); + Assert.AreEqual("idle", menu.StatusItem.Header); + } + [TestMethod] public void Activate_OnTheQuitItem_RaisesQuitRequested() { diff --git a/TrayApp/Menu/TrayMenu.cs b/TrayApp/Menu/TrayMenu.cs index bf27ed1..df5f3d4 100644 --- a/TrayApp/Menu/TrayMenu.cs +++ b/TrayApp/Menu/TrayMenu.cs @@ -86,6 +86,16 @@ public TrayMenu(string displayName, Func? status, IEnumerable public string? LastError { get; private set; } + /// + /// Records a failure that happened before the menu existed. + /// + /// The message, or to clear it. + /// + /// Restoring a remembered toggle runs the tool's setter before there is a menu to fail into, and the + /// next successful action clears this the same way it clears a failed click. + /// + internal void SeedError(string? message) => LastError = message; + /// /// Re-reads every item's state from the tool. /// diff --git a/TrayApp/TrayAppBuilder.cs b/TrayApp/TrayAppBuilder.cs index 2949473..633651d 100644 --- a/TrayApp/TrayAppBuilder.cs +++ b/TrayApp/TrayAppBuilder.cs @@ -383,6 +383,7 @@ public Task RunAsync(string[] args) bool useTray = UseTray(options); DebouncedPreferenceStore? store = null; + string? restoreError = null; try { @@ -391,7 +392,7 @@ public Task RunAsync(string[] args) if (useTray && app.Persistence is not null) { store = new DebouncedPreferenceStore(app.Persistence, app.PreferenceKey, app.PreferenceDebounce); - LoadPreferences(app, store); + restoreError = LoadPreferences(app, store); } if (!TryApplyOptions(options, out string optionError)) @@ -405,7 +406,7 @@ public Task RunAsync(string[] args) return Task.FromResult(ExitSuccess); } - int exitCode = useTray ? RunTray(app, options, store) : ConsoleRunner.Run(app, options); + int exitCode = useTray ? RunTray(app, options, store, restoreError) : ConsoleRunner.Run(app, options); // FinishAsync owns the store from here: it flushes whatever the run queued and disposes it. Task finishing = FinishAsync(exitCode, store); @@ -456,19 +457,40 @@ private static async Task FinishAsync(int exitCode, DebouncedPreferenceStor return exitCode; } + /// + /// Reads the remembered state and applies it to the toggles that asked to be remembered. + /// + /// The tool being run. + /// The store to read from. + /// The first refusal's message, for the menu's status line, or . /// + /// /// Blocking here is deliberate. The load has to finish before the menu is built, and it has to finish on /// the thread that goes on to start Avalonia - see the remarks on . There is no /// synchronization context in a console application, so there is nothing for this to deadlock against. + /// + /// + /// A setter can refuse here exactly as it can on a click - restoring "keep awake: on" asks the tool to + /// take an inhibitor, and the machine may have none to give. This runs before there is a menu to fail + /// into, so the refusal is caught, reported, and handed back for the status line. Letting it escape would + /// end the process before the tray appeared, which is the one thing this library exists to prevent: the + /// remaining toggles are still restored, and the tray still comes up saying why. + /// /// [SuppressMessage( "Reliability", "CA2007:Consider calling ConfigureAwait on the awaited task", Justification = "Not an await; the result is taken synchronously so that the tray still starts on the process's main thread.")] - internal static void LoadPreferences(TrayAppDefinition app, DebouncedPreferenceStore store) + [SuppressMessage( + "Design", + "CA1031:Do not catch general exception types", + Justification = "The setter belongs to the consuming tool, so there is no exception type to filter on, and a refusal here must not stop the tray from coming up.")] + internal static string? LoadPreferences(TrayAppDefinition app, DebouncedPreferenceStore store) { store.LoadAsync().GetAwaiter().GetResult(); + string? firstError = null; + foreach (TrayToggleItem toggle in app.Toggles) { if (toggle.PreferenceKey is null || store.Get(toggle.PreferenceKey) is not bool remembered) @@ -476,8 +498,18 @@ internal static void LoadPreferences(TrayAppDefinition app, DebouncedPreferenceS continue; } - toggle.Set(remembered); + try + { + toggle.Set(remembered); + } + catch (Exception ex) + { + firstError ??= ex.Message; + Console.Error.WriteLine($"{app.Name}: {ex.Message}"); + } } + + return firstError; } /// @@ -507,9 +539,15 @@ internal static bool TryApplyOptions(CommandLineOptions options, out string erro "Design", "CA1031:Do not catch general exception types", Justification = "Any failure to bring up a tray icon is recoverable by falling back to the console, and the windowing backends do not report those failures through a common exception type.")] - private static int RunTray(TrayAppDefinition app, CommandLineOptions options, DebouncedPreferenceStore? store) + private static int RunTray( + TrayAppDefinition app, + CommandLineOptions options, + DebouncedPreferenceStore? store, + string? restoreError) { TrayMenu menu = new(app.DisplayName, app.Status, app.Items); + menu.SeedError(restoreError); + TrayApplication? host = null; void Remember(TrayToggleItem toggle) From ceec35029a9581b6c928731a6565bf8df1b389db Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Wed, 23 Sep 2026 03:58:42 +0000 Subject: [PATCH 2/2] Never absorb a fatal exception from the tool's own code github-code-quality flagged the new catch in LoadPreferences as a generic catch clause, and it is right: reporting an OutOfMemoryException in a menu and carrying on presents a tray that cannot work and buries the real cause under whatever fails next. Filtering only the flagged line would have been worse than leaving it, though. LoadPreferences and TrayMenu.Run invoke the same thing - the consuming tool's toggle setter - one on a restore and one on a click. Filtering one and not the other makes identical user code behave differently depending on how it was reached. So the rule is stated once, in FatalError, and applied at the three places the library runs the tool's own code: the preference restore, the menu's action path, and the console runner's start action. Fatal means the process is already lost: OutOfMemoryException and AccessViolationException. StackOverflowException is absent because managed code cannot catch it, and InsufficientMemoryException is explicitly not fatal - it means one allocation was refused before being attempted, not that memory is gone. Two catches stay total on purpose. RunTray's is the "fall back, never die" case: the windowing backends report failure as whatever type they felt like, a console fallback always beats exiting, and there is no tool code involved. The preference store's timer callback is async void, where an escaping exception crashes the process rather than reporting anything. Verified: 108 tests (107 passing, 1 inconclusive by platform), including that the menu still absorbs an ordinary refusal and now lets a fatal one through; and the tray still round trips a remembered toggle across a restart. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qksp7NcKB2jMJjXuxGS3nP --- TrayApp.Test/FatalErrorTests.cs | 67 +++++++++++++++++++++++++++++++++ TrayApp/ConsoleRunner.cs | 4 +- TrayApp/FatalError.cs | 38 +++++++++++++++++++ TrayApp/Menu/TrayMenu.cs | 4 +- TrayApp/TrayAppBuilder.cs | 4 +- 5 files changed, 111 insertions(+), 6 deletions(-) create mode 100644 TrayApp.Test/FatalErrorTests.cs create mode 100644 TrayApp/FatalError.cs diff --git a/TrayApp.Test/FatalErrorTests.cs b/TrayApp.Test/FatalErrorTests.cs new file mode 100644 index 0000000..e7d1bc3 --- /dev/null +++ b/TrayApp.Test/FatalErrorTests.cs @@ -0,0 +1,67 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.TrayApp.Test; + +using System; +using System.Diagnostics.CodeAnalysis; +using ktsu.TrayApp; +using ktsu.TrayApp.Menu; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Tests for the line between a failure the library absorbs and one it must not. +/// +[TestClass] +public class FatalErrorTests +{ + [TestMethod] + public void IsNotFatal_ForAnOrdinaryRefusal_IsTrue() + { + Assert.IsTrue(FatalError.IsNotFatal(new InvalidOperationException("no inhibitor here"))); + Assert.IsTrue(FatalError.IsNotFatal(new PlatformNotSupportedException())); + Assert.IsTrue(FatalError.IsNotFatal(new ObjectDisposedException("blocker"))); + } + + [TestMethod] + [SuppressMessage( + "Usage", + "CA2201:Do not raise reserved exception types", + Justification = "These are the reserved types under test; constructing one is how the filter that excludes them is checked, and none of them is raised.")] + public void IsNotFatal_ForAProcessThatIsAlreadyLost_IsFalse() + { + Assert.IsFalse(FatalError.IsNotFatal(new OutOfMemoryException())); + Assert.IsFalse(FatalError.IsNotFatal(new AccessViolationException())); + } + + [TestMethod] + public void IsNotFatal_ForARefusedAllocation_IsTrue() + { + // InsufficientMemoryException derives from OutOfMemoryException but means the opposite: one + // allocation was refused before being attempted, and the process is fine. + Assert.IsTrue(FatalError.IsNotFatal(new InsufficientMemoryException())); + } + + [TestMethod] + [SuppressMessage( + "Usage", + "CA2201:Do not raise reserved exception types", + Justification = "Throwing the reserved type is the behaviour under test: the menu must let it through rather than report it as a refusal.")] + public void Run_DoesNotSwallowAFatalException() + { + TrayMenu menu = new("Demo", () => "idle", []); + + // The same setter reached from a click and from a preference restore must behave the same way, so + // the filter sits on both paths rather than only the one a review flagged. + Assert.ThrowsExactly(() => menu.Run(() => throw new OutOfMemoryException())); + Assert.IsNull(menu.LastError); + } + + [TestMethod] + public void Run_StillAbsorbsAnOrdinaryRefusal() + { + TrayMenu menu = new("Demo", () => "idle", []); + + Assert.IsFalse(menu.Run(() => throw new InvalidOperationException("nope"))); + Assert.AreEqual("nope", menu.LastError); + } +} diff --git a/TrayApp/ConsoleRunner.cs b/TrayApp/ConsoleRunner.cs index 2c814df..e70facc 100644 --- a/TrayApp/ConsoleRunner.cs +++ b/TrayApp/ConsoleRunner.cs @@ -31,7 +31,7 @@ internal static class ConsoleRunner [SuppressMessage( "Design", "CA1031:Do not catch general exception types", - Justification = "The start action belongs to the consuming tool, so there is no exception type to filter on; reporting the refusal and exiting 1 is the contract the exit codes in --help describe.")] + Justification = "The start action belongs to the consuming tool, so there is no exception type to filter on beyond the fatal ones FatalError excludes; reporting the refusal and exiting 1 is the contract the exit codes in --help describe.")] internal static int Run(TrayAppDefinition app, CommandLineOptions options) { Ensure.NotNull(app); @@ -41,7 +41,7 @@ internal static int Run(TrayAppDefinition app, CommandLineOptions options) { app.OnStart?.Invoke(); } - catch (Exception ex) + catch (Exception ex) when (FatalError.IsNotFatal(ex)) { Console.Error.WriteLine($"{app.Name}: {ex.Message}"); return ExitFailure; diff --git a/TrayApp/FatalError.cs b/TrayApp/FatalError.cs new file mode 100644 index 0000000..6569d4e --- /dev/null +++ b/TrayApp/FatalError.cs @@ -0,0 +1,38 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.TrayApp; + +using System; + +/// +/// Tells a failure the library is meant to absorb from one it must not. +/// +/// +/// +/// The library catches broadly on purpose wherever it runs the consuming tool's own code: a toggle setter, +/// a command, a start action. There is no exception type to filter on there - the tool's platform layer +/// throws whatever it throws - and the whole point of this library is that a refusal ends in a message +/// rather than in a dead process. +/// +/// +/// That reasoning stops at an exception which says the process is already lost. Reporting one in a menu and +/// carrying on presents a tray that cannot work, and buries the real cause under whatever fails next. +/// is absent because .NET does not let managed code catch it, and +/// is deliberately not fatal: it means one allocation was refused +/// before being attempted, not that the process is out of memory. +/// +/// +internal static class FatalError +{ + /// + /// Determines whether an exception is one the library may absorb. + /// + /// The exception the tool's code threw. + /// when it may be reported and survived. + internal static bool IsNotFatal(Exception exception) => exception switch + { + InsufficientMemoryException => true, + OutOfMemoryException or AccessViolationException => false, + _ => true, + }; +} diff --git a/TrayApp/Menu/TrayMenu.cs b/TrayApp/Menu/TrayMenu.cs index df5f3d4..023cea3 100644 --- a/TrayApp/Menu/TrayMenu.cs +++ b/TrayApp/Menu/TrayMenu.cs @@ -154,7 +154,7 @@ public bool Activate(TrayMenuItem item) [SuppressMessage( "Design", "CA1031:Do not catch general exception types", - Justification = "The action belongs to the consuming tool, so there is no exception type to filter on, and letting one escape a tray click handler takes down a process the user asked to keep running.")] + Justification = "The action belongs to the consuming tool, so there is no exception type to filter on beyond the fatal ones FatalError excludes, and letting one escape a tray click handler takes down a process the user asked to keep running.")] public bool Run(Action action) { Ensure.NotNull(action); @@ -166,7 +166,7 @@ public bool Run(Action action) action(); LastError = null; } - catch (Exception ex) + catch (Exception ex) when (FatalError.IsNotFatal(ex)) { LastError = ex.Message; Console.Error.WriteLine($"{displayName}: {ex.Message}"); diff --git a/TrayApp/TrayAppBuilder.cs b/TrayApp/TrayAppBuilder.cs index 633651d..0fa5684 100644 --- a/TrayApp/TrayAppBuilder.cs +++ b/TrayApp/TrayAppBuilder.cs @@ -484,7 +484,7 @@ private static async Task FinishAsync(int exitCode, DebouncedPreferenceStor [SuppressMessage( "Design", "CA1031:Do not catch general exception types", - Justification = "The setter belongs to the consuming tool, so there is no exception type to filter on, and a refusal here must not stop the tray from coming up.")] + Justification = "The setter belongs to the consuming tool, so there is no exception type to filter on beyond the fatal ones FatalError excludes, and a refusal here must not stop the tray from coming up.")] internal static string? LoadPreferences(TrayAppDefinition app, DebouncedPreferenceStore store) { store.LoadAsync().GetAwaiter().GetResult(); @@ -502,7 +502,7 @@ private static async Task FinishAsync(int exitCode, DebouncedPreferenceStor { toggle.Set(remembered); } - catch (Exception ex) + catch (Exception ex) when (FatalError.IsNotFatal(ex)) { firstError ??= ex.Message; Console.Error.WriteLine($"{app.Name}: {ex.Message}");