fix: stop a view-model's background load losing an unexpected failure - #2343
Merged
Merged
Conversation
ViewModelBase.RunInitAsync is the boundary that catches exceptions from every
constructor's fire-and-forget init, and its own summary promised that exceptions
"are caught and logged instead of becoming unobserved task exceptions". It caught
six types. Everything else escaped, and escaping was not merely unlogged, it was
unobservable:
- nothing in production awaits InitializationComplete, so the fault stays on
the task;
- App wires TaskScheduler.UnobservedTaskException, but that event is raised
from a faulted task's FINALIZER;
- the task is rooted by InitializationComplete on a view model the nav table
caches for the process lifetime, so it is never finalized and the handler
never runs.
The result was neither a crash nor a log line. The tab stayed half-loaded, and a
NullReferenceException from DebloaterService.ParsePackages had been thrown on
every launch with nothing reporting it (#2258). It surfaced only because an
unrelated test was changed to await the init, which is the shape of the problem:
the sole way to notice was to await a task production never awaits.
The boundary now ends in catch (Exception), logs at Fatal with the caller name
and the fault's type, and rethrows when a debugger is attached. That is the third
site in this codebase where catching Exception is the right answer rather than a
tolerated one, alongside the three global handlers in App — a fire-and-forget
boundary whose entire purpose is to stop a background fault disappearing. The
five specific handlers stay: they describe an operation failing, which a tab can
be half-useful after, and they log at Error rather than Fatal.
Two decisions worth stating, because both were made against the obvious option:
Debugger.IsAttached, not #if DEBUG. Every build this project produces is Release
— CI builds Release in all nine of its build steps, and the only binary anyone
runs is the published one. A #if DEBUG branch would compile into nothing that
ever executes, and no test could reach it. A debugger is the accurate spelling of
"a developer is watching", and it leaves both branches compiled and asserted. The
default is pinned by a test, because getting it wrong is asymmetric: rethrowing
always would let an unforeseen fault in any of the 40 init paths take down a
released build, which is worse than the silent half-load being fixed here.
InitializationFault, not a log assertion. The issue asked for a test that faults
an init and asserts the log received it. Reading what Serilog wrote means
assigning the global Log.Logger that every test in the run shares, which
LogServiceLogDirTests spells out as the thing not to do. Recording the fault on
the view model is observable without touching process-wide state and is the
stronger assertion: it says WHICH exception was caught, not that a line was
written. It is also where a visible error state would read from, if the tab is
later made to say so itself.
18 tests, red ritual on five mutations, each red for the right reason and green
after restore: the final catch removed (9 red — the seven unforeseen types, the
synchronous throw, and the rethrow test's second assertion, since nothing
recorded the fault); rethrowing unconditionally (8); the default flipped to
always-rethrow (1); cancellation recorded as a fault (1); an expected fault
rethrown as well (1).
Unit suite 5792 passed, 0 failed. All four projects build with 0 warnings.
Closes #2258
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ViewModelBase.RunInitAsyncis the boundary that catches exceptions from every constructor's fire-and-forget init, and its own summary promised that exceptions "are caught and logged instead of becoming unobserved task exceptions that could crash the application". It caught six types. Everything else escaped, and escaping was not merely unlogged, it was unobservable:InitializationComplete, so the fault stays on the task;AppwiresTaskScheduler.UnobservedTaskException, but that event is raised from a faulted task's finalizer;InitializationCompleteon a view model the nav table caches for the process lifetime, so it is never finalized and that handler never runs.Neither a crash nor a log line. The tab stayed half-loaded, indistinguishable from a tab with nothing to show — and a
NullReferenceExceptionfromDebloaterService.ParsePackageshad been thrown on every launch with nothing reporting it. It surfaced only because an unrelated test was changed to await the init, which is the shape of the whole problem: the sole way to notice was to await a task production never awaits. 40 view models fire an init this way.What changed
The boundary now ends in
catch (Exception), logs at Fatal with the caller name and the fault's type, and rethrows when a debugger is attached — Option C from the issue. This is the fourth site in the codebase where catchingExceptionunfiltered is the right answer rather than a tolerated one, the other three being the global handlers inApp: a fire-and-forget boundary whose entire purpose is to stop a background fault disappearing. The five specific handlers stay and keep logging at Error — they describe an operation failing, which a tab can be half-useful after; arriving at the final net means an assumption the code makes about itself was wrong.Two decisions made against the obvious option
Debugger.IsAttached, not#if DEBUG. Option C as written says "in debug builds". Every build this project produces is Release: CI builds Release in all nine of its build steps, and the only binary anyone runs is the published one. A#if DEBUGbranch would compile into nothing that ever executes, and no test could reach it — so the machinery Option C is paying for would be dead code. A debugger is the accurate spelling of "a developer is watching", it never fires in CI or in a release, and it leaves both branches compiled and asserted.The default is pinned by its own test, because getting it wrong is asymmetric: rethrowing always would let an unforeseen fault in any of the 40 init paths take down a released build, which is worse than the silent half-load this fixes.
One limitation, stated rather than implied: a rethrow re-faults the same rooted task, so it does not by itself crash anything. It breaks a debugger configured to break on thrown exceptions. The load-bearing half of this fix is the Fatal log and the recorded fault; the rethrow is the developer-facing extra Option C asked for.
InitializationFault, not a log assertion. The issue asked for a test that faults an init and asserts the log received it. Reading what Serilog wrote means assigning the globalLog.Loggerthat every test in the run shares, whichLogServiceLogDirTestsspells out as the thing not to do. A newpublic Exception? InitializationFaultis observable without touching process-wide state, and is the stronger assertion: it says which exception was caught, not that a line was written. Cancellation is deliberately not recorded — a tab closing mid-load is the expected path, and recording it would make the property true for almost every tab on shutdown.It is also where a visible error state would read from if the tab is later made to say so itself. Not in this PR.
Verification
18 new tests. Red ritual on five mutations of the fix, each rebuilt before running (a stale binary reads as a real failure), each restored from saved bytes:
catchremoved (the original defect)WithRethrowOn's second assertion — the exception still reaches the awaiter but nothing recorded itTheDefaultDecision_RethrowsOnlyUnderADebuggerCancellation_IsNotRecordedAsAFaultAnExpectedFault_IsNotRethrownEvenWhenUnexpectedOnesAreThe 9 in the first row is worth reading: I predicted 8 and the suite found a ninth angle I had not counted.
dotnet build -c Release, 0 errors, 0 warnings.dotnet format --verify-no-changeson app and tests: clean.InitializationComplete; none changed behaviour.Closes #2258