A malformed settings.json silently resets all 88 Lite settings, before logging exists to say so

[erikdarlingdata]

Found while making Lite's sample settings file real (#2418). It is the most serious thing that work turned up and it is in neither issue.

One trailing comma resets all 88 settings, silently

Both settings loaders swallow every exception and fall back to defaults:

catch { /* Use default */ }    // App.xaml.cs, LoadDefaultTimeRange
catch { /* Use defaults */ }   // App.xaml.cs, LoadAlertSettings

LoadAlertSettings wraps the whole JsonDocument.Parse plus all 88 TryGetProperty calls in that one try. So a single malformed character does not cost one setting — the parse throws before any key is read and every setting reverts to its default at once. Alert thresholds, SMTP configuration, webhook enablement, retention, theme, the lot.

And the app structurally cannot tell you

The loaders run before logging exists:

LoadDefaultTimeRange();      <- here
LoadAlertSettings();         <- and here
   ... 13 lines ...
AppLogger.Initialize(logDirectory);

So there is no log line, no dialog, and no way to add one without reordering startup. The user sees a Performance Monitor that has forgotten its configuration, with nothing anywhere explaining why, and the most likely next move is to reconfigure everything by hand — writing the defaults back over the file that still holds the real settings, at which point the original is gone.

This matters more now, not less. #2418 ships a documented reference precisely so people hand-edit settings.json, and hand-editing JSON is how you get a trailing comma. We are about to make the triggering action more common while the failure stays invisible.

What to do

The two halves are separable and the first is most of the value.

Report it. Capture the exception and surface it once logging exists — a deferred diagnostic the loader records and AppLogger.Initialize flushes is enough, and avoids reordering startup. A user who is told "settings.json could not be parsed at line 42, defaults are in use" can fix it in a minute.

Do not overwrite what you could not read. This is the part that turns an annoyance into data loss. If the parse failed, WriteSetting's next whole-document rewrite replaces a file whose contents were never understood. At minimum, copy the unreadable file aside (settings.json.unreadable-<timestamp>) before the first write, so the original is recoverable.

Worth considering, though it is a bigger change: distinguishing "file absent" (a legitimate first run — defaults are correct and silent) from "file present but unparseable" (never correct, always worth saying). The current code cannot tell those apart, which is why the silence looks reasonable in the code and is wrong in practice.

Related: #2418 (the sample that makes hand-editing likelier), #2413 (a key with no UI, so hand-editing is the only path).

[erikdarlingdata]

Fixed on dev by #2428 — and two of this issue's premises were wrong, both in ways worth recording.

Startup did not need reordering. I wrote that there was "no way to add [a log line] without reordering startup". AppLogger.Log enqueues unconditionally and only Flush is gated on Initialize, so a line written from a loader lands in the file a few statements later on its own — DataRootMigration's failure lines above already rely on exactly that. The deferred-diagnostic scaffolding I asked for was unnecessary; the loaders just log, and a static carries the reason to one modal after the main window shows.

The data-loss path was narrower than I described. I claimed the writers start from a fresh JsonObject when the parse fails and overwrite the file. They did not — they threw at JsonNode.Parse and aborted, so the file survived while the save silently did nothing and the window still said "Settings saved." The route to actual loss was the user giving up and deleting the file. The one shape that genuinely overwrote outright was JsonNode.Parse(json) ?? new JsonObject() against a JSON literal null. Both are closed now, but the original framing overstated the mechanism.

What shipped is better than what I asked for in one respect: the guard is a SettingsFileState { Absent, Readable, Unreadable } distinction, WPF-free and logger-free, and all five writers go through it — I had identified two loaders; there were five write sites each rolling its own read in front of a whole-document rewrite. Absent stays completely silent (no log, no dialog, no quarantine), which is the case that must not become noise. Unreadable is never silent, and carries the JSON line and position.

The wiring test is the part that lasts: it pins the category rather than the instance, so a sixth writer built the old way fails the test instead of shipping. That is the distinction #2405 was about, applied here without being asked.

Proved against a faithful transcription of dev's four call shapes: 4 passed / 9 failed on dev, 13 passed / 0 failed on the branch. The four that pass on dev are the absent and readable paths, which were never broken.

Three residues split out rather than folded in: #2433 (four of six Save methods return void and cannot report failure, so "Settings saved." can be a lie — note the two bool ones ARE honoured by the caller), #2434 (Darling's viewer has the same defect and worse — Debug.WriteLine compiles out of Release, and load-then-save means one dropdown click replaces a corrupt file's every setting), and #2431 (the same bare catch in McpSettings.Load silently disables the MCP endpoint).