Skip to content

Do not let a refused preference restore kill the tray - #6

Merged
matt-edmondson merged 2 commits into
mainfrom
claude/preference-restore-failure
Sep 23, 2026
Merged

matt-edmondson merged 2 commits into
mainfrom
claude/preference-restore-failure

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Found while starting the NoSleep migration onto ktsu.TrayApp 1.1.0 — NoSleep is the first real tool through this path, and it hits the bug.

The bug

Restoring a remembered toggle calls the tool's setter:

foreach (TrayToggleItem toggle in app.Toggles)
{
    if (toggle.PreferenceKey is null || store.Get(toggle.PreferenceKey) is not bool remembered)
    {
        continue;
    }

    toggle.Set(remembered);   // <- bare
}

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 — NoSleep's KeepAwakeController.Enable() throws SleepBlockException or PlatformNotSupportedException in that case.

LoadPreferences runs from RunAsync before the menu is built, so there is nowhere for the refusal to land, and nothing catches it. The exception escapes RunAsync and ends the process before the tray ever appears.

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 tool it is meant to replace: NoSleep today wraps its startup Enable() in catch (Exception ex) when (ex is SleepBlockException or PlatformNotSupportedException), prints the message, and still shows the tray so the user can retry once whatever was missing is installed. A tool ported onto 1.1.0 as-is would lose that.

The fix

Catch per toggle, report to standard error, and hand the first message back for the menu's status line via a new internal TrayMenu.SeedError, which the next successful action clears like any other failure. Two consequences worth stating:

  • The toggles after the one that refused are still restored. One unavailable inhibitor should not also forget the user's other settings.
  • The tray still comes up, saying why. Same destination a click's refusal reaches, because there is nowhere better for it to go.

RunTray takes the message and seeds the menu before the host starts, so the status line is already correct on the first paint rather than after the first refresh.

Verified

  • Unit tests, 103 passing (1 inconclusive by platform). New: a restore where the first toggle's setter throws — LoadPreferences returns the message instead of propagating, and the second toggle is still restored; a clean restore returns null; and a seeded error shows as Demo failed: … in the status line and clears on the next successful action.
  • The real restore path, through a tray. With a status-notifier host on a session bus under xvfb, across two processes:
########## run 1: click Running off ##########
--- before ---
  id=1 label='running for 00:00:00' checked=None enabled=False
  id=3 label='Running' checked=1 enabled=True
--- after ---
  id=6 label='idle' checked=None enabled=False
  id=8 label='Running' checked=0 enabled=True
persisted: {"Toggles":{"running":false}}
########## run 2: the remembered OFF must be restored ##########
--- before ---
  id=1 label='idle' checked=None enabled=False
  id=3 label='Running' checked=0 enabled=True
run 2 exit=0

A fresh process comes up already unchecked with the status line reading idle. The restore path was previously only unit-tested; this is it working through a real com.canonical.dbusmenu menu.

Tagged [patch]. As before, this is left for you to merge — it publishes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01C5iSfFNhum18dnc794cVvy


Generated by Claude Code

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qksp7NcKB2jMJjXuxGS3nP
Comment thread TrayApp/TrayAppBuilder.cs Fixed
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Qksp7NcKB2jMJjXuxGS3nP
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant