The viewer stops replacing a settings file it could not read (#2434)

[erikdarlingdata]

One click on the time-display dropdown was enough to lose every setting in the Darling viewer's viewer-settings.json, and in a shipped build nothing anywhere recorded that it had happened.

Both of the viewer's per-user stores ended their load in a bare catch whose only output was Debug.WriteLine. That carries [Conditional("DEBUG")] and the compiler removes it from a Release build, so this was not "unlogged" — it was unreportable: no log line, no dialog, not even a trace. And neither Save merged; each serialized its whole in-memory object over the file. OnDisplayModeChanged and the Overview sort selector both do load-then-save, so an unreadable file was loaded as defaults and those defaults were written straight back over it. Lite's pre-#2425 writers at least parsed first and aborted, which left the file intact by accident. The viewer had no such accident.

The premise held up in source, with one correction to make

Everything in the issue checked out: the Debug.WriteLine catch in both stores, the whole-object File.WriteAllText in both Saves, the load-then-save at MainWindow.xaml.cs L1042 and L1086, SettingsWindow.xaml.cs L1509, and ViewerControlPlaneMigration being fed from _appSettingsStore.Load().

The correction is that the issue's third bullet — "ViewerLogger exists, and the Debug.WriteLine comment predates it, worth re-checking rather than trusted" — is exactly right, and the re-check found one thing the issue could not have known. ViewerLogger.Log returns early when the logger has not been initialized, unlike Lite's AppLogger, which enqueues unconditionally. App.OnStartup reads viewer-settings.json for the color theme before ViewerLogger.Initialize(). So simply logging from the store would have dropped the earliest and most useful diagnostic — the very first read of the file — on the floor. Initialize moves ahead of the theme read; it creates a directory, starts a timer and swallows its own failure, and nothing between the two positions depends on the ordering.

There is a third store, and writing the category guard is what found it

The issue names two stores. There are three. ViewerServerStore persists viewer-servers.json — the operator's registry of monitored servers, their display names, favourites, view filters and credential-profile links — through the identical whole-object round trip, with the same bare catch and the same Debug.WriteLine.

Its shape made the loss arrive sooner rather than later. LoadFromDisk began empty on an unreadable registry, so the first favourite toggle serialized an empty list over the file. There is no intermediate state in which anyone could have noticed: the servers were simply not there, and then the file said so.

It is fixed here rather than filed, because a guard listing two of the three stores would be exactly the scenario-shaped fix it was written to prevent. Two things had to move in the guard to take it, and one of them is a trap worth naming:

• ReadObject no longer applies Read's "the root must be an object" rule. That rule exists because a MERGE has nothing to merge into otherwise; the registry is a legitimate JSON array. Borrowing the rule here would have called every healthy registry unreadable and dropped a quarantine copy beside it on every favourite toggle — data loss traded for litter, and litter is how a real quarantine copy gets ignored. Save_LeavesNoCopyBehind_ForAHealthyRegistry_ThoughItsRootIsAnArray is the control for exactly that mistake, because it was one line away.
• PermitReplace became generic, so it asks whether the file can still be read AS the thing about to replace it — the same question the store's own Load asks — rather than by some other standard.

Read and ReadObject now share a ReadText helper instead of the second calling the first, so absent / empty / unparseable is decided in one place for both.

Shared, not mirrored

SettingsFileGuard moved to PerformanceMonitor.Common/Services/. Common is referenced by Lite and the viewer, targets net10.0, and carries no WPF and no logger — which is what the guard was already written to be, a pure function of a path and a clock. The move is a namespace change and nothing else; SettingsWindow.xaml.cs already had the using.

The issue is right that only half of it is common, and that is the half that matters. Lite merges a JsonObject key by key, so RootForWrite hands back a document to merge into. The viewer round-trips a whole object and REPLACES the file, so it has no document to be handed and one question left: whether the write is allowed to destroy what is there now. Two members answer that shape:

• PermitReplace — absent or readable is a plain yes; unreadable is copied aside first and the answer is yes only if the copy exists.
• ReadObject<T> — Read plus the typed deserialize. That deserialize is inside the guard rather than in front of it because it fails on files Read is perfectly happy with: {"DefaultTimeRangeIndex":"not a number"} is a valid JSON object and still unusable, and a classify step that stops at "is this a JSON object" hands the exception straight back to the bare catch it was meant to replace.

Mirroring the class into the viewer would have meant two copies of the quarantine rule, and this issue exists because the first copy already had a sibling nobody had looked at.

What the viewer does now

Both stores read and write through one helper, ViewerSettingsFile. Absent stays completely silent — a first run has nothing to preserve and nothing to explain, and a warning there is how a diagnostic earns being ignored. A present file that cannot be read is reported to ViewerLogger, left exactly where it is, and copied aside as viewer-settings.json.unreadable-<timestamp> before any save replaces it. When even the copy cannot be made, the save is refused and says so: leaving the file alone beats replacing it when the alternative is permanent.

One broken file gets one log line, not one per read. Nothing reads these files once — the theme is applied from viewer-settings.json before the window exists, MainWindow loads it again to seed itself, the Settings window loads it on open and MainWindow re-loads it on close, and the control-plane migration loads it too — so one trailing comma would otherwise account for five identical ERROR lines, which is a poor reward for the one person who went and opened the log. Reported once per (file, problem); keyed on the problem as well as the path, so a file that changes mid-session and breaks in a NEW way is said again. (Sharing one store instance between App and MainWindow was the other way to get here and is the wrong lever — it makes the number of log lines a property of object ownership across startup, so the next caller needing its own read quietly brings it back.)

Save answers with a bool instead of throwing. Three of its four call sites never caught anything, so on a full disk the old contract was a crash — and a save nobody can ask about is precisely how a UI ends up claiming it saved something it did not (#2433, next door). The two handlers that persist on an ordinary click now say when the click did not stick. PersistCollapseState logs instead: a modal every time a sidebar group collapses would be worse than what it reports, and that is the write that most needed the guard behind it, because collapsing a group is a whole-file rewrite of viewer-preferences.json that nobody thinks of as a save. MainWindow raises one dialog at Loaded for a file that was unreadable at startup — somebody looking at an app that has forgotten its configuration should not have to go and find a log to learn that it did.

Verification

Darling.Tests targets net10.0-windows and cannot run on macOS, so the real test file was compiled into a throwaway net10.0 harness with a minimal xUnit shim, against the REAL ViewerAppSettings.cs, ViewerPreferences.cs, ViewerLogger.cs and ViewerSettingsFile.cs from each branch plus a project reference to the real PerformanceMonitor.Common — not a transcription of them. Compiled -c Release on purpose, so Debug.WriteLine is genuinely gone the way it is in a shipped build.

Against dev the test file does not compile at all — 13 CS errors, because LastLoadState, LastLoadProblem, SettingsFileState and a Save that returns anything do not exist there. That absence is the finding, not a measurement, so the pass/fail split below runs the subset that can be expressed against dev's contract, identical bodies on both sides:

	dev's behavior	this branch
quarantine + wiring + "is it reportable at all"	3 passed, 10 failed	13 passed, 0 failed
the full committed test file	does not compile (13 errors)	19 passed, 0 failed

The three that pass on dev are the controls — a readable file, a first run, and a healthy array-rooted registry all leave no quarantine copy — which were never broken and are the first thing a wrong fix breaks. The ten failures are the ones that matter: no copy made before ANY of the three stores replaced an unreadable file, all six source-wiring guards red, and Load_LeavesARecordAShippedBuildCanSee finding nothing whatsoever in either channel a shipped viewer has (the Trace listeners Debug.WriteLine routes to, and the viewer's own log file) — the only line in the log is ViewerLogger's own "Logging initialized".

That last one stays in the harness rather than the committed suite, deliberately. It needs process-wide ViewerLogger and Trace.Listeners, and more to the point CI runs dotnet test in Debug, where Debug.WriteLine still works — so as a standing guard it would pass on unfixed code, which is worse than not having it. The standing guard for the same property is NoViewerSettingsStore_ReportsThroughDebugWriteLine, which is a source scan and is configuration-independent.

ViewerSettingsStoreWiringTests is the category rather than the instance, and it earned its keep before it ever ran in CI — writing it is what turned up the third store. The defect was never that one store's Save was wrong; it was that each store rolled its own read in front of its own whole-file replace, so whether a save could destroy your configuration depended on which store you happened to be in. A fourth written the same way tomorrow fails that test instead of shipping. It keys on _filePath — the store's own file — rather than on File.* in general, because ImportServersFromFile legitimately reads a file the user picked, and a rule that could not tell those apart would either miss the defect or forbid a feature.

One note on the merge

#2435 landed on dev while this was open and made McpSettings.Load take SettingsFileGuard — from Lite/Services, where it lived at the time. The two merge cleanly and do not compile: git has no way to see that a using directive one file over is what the other lane's new call site needs. One using is the whole fix, and nothing about #2435's reasoning changes — it is the same type with the same behavior, one assembly further down. Worth knowing that the PR build catches this, because a local build of the branch alone does not.

Deliberately not in scope

ViewerServerStore's mutators still return void — filed as a residue, then argued out of it by review, correctly: the two settings files say when a save did not land and leaving the registry silent is the inconsistency rather than the compromise, on the file this PR itself calls the costliest to lose.

The answer lives on the store as LastSaveSucceeded rather than in the return types, because two mutators already answer other questions — ToggleFavorite says whether it is favourite now, ImportServersFromFile answers with counts. A caller about to tell the user something happened can ask; one doing incidental cleanup need not. Two ask: the sidebar's pin toggle, whose status line is that click's whole report, and "Imported N server definition(s).", which is a claim about a file made at exactly the moment a refused write matters most. The two SetFavorite(..., false) cleanups on server removal deliberately do not, and say so in place — they remove a pin for a server that no longer exists, so a refused write strands an entry nothing reads rather than losing anything.

ViewerSettings (the viewer's read of the SERVICE's darling.json) is a different file with a different owner and its own diagnostics path (#1954), and it is read-only from the viewer — there is no save that could replace it, which is the whole hazard here. Left alone.

The startup dialog reports the file and the parse position but not which SETTINGS were lost, because the whole-object round trip cannot know: the deserialize failed. Naming them would mean per-property reads, which is the viewer's version of the eighty-eight-try-blocks item #2428 filed for Lite.

Fixes #2434

[claude]

Doc-comment mixup: this new <summary> for WarnSettingNotSaved was inserted between the existing <summary> for OnDisplayModeChanged (the "A server tab's Server/Local/UTC picker changed..." block just above it) and the OnDisplayModeChanged method itself, instead of being placed as its own block before/after that pair.

Net effect: the pre-existing OnDisplayModeChanged summary now dangles with no method directly under it (doc tooling will attach it to WarnSettingNotSaved instead), and OnDisplayModeChanged itself ends up with no doc comment at all.

Suggest moving this WarnSettingNotSaved summary block above the OnDisplayModeChanged summary block (swap their order) so each method keeps its own doc comment.

[claude]

Review summary

Went through the diff (Darling viewer settings-file guard + SettingsFileGuard move to PerformanceMonitor.Common) against CONTRIBUTING.md, Lite/Darling parity, and correctness/security.

Overall: solid. The core fix (three-way Absent/Readable/Unreadable classification, copy-aside-before-replace via PermitReplace, Save returning bool instead of throwing, and routing diagnostics through ViewerLogger instead of Debug.WriteLine) is correct and well-tested (ViewerSettingsFileGuardTests + the wiring/source-scan tests). Verified:

• SettingsFileGuard.ReadObject<T> / PermitReplace correctly reuse Read/Quarantine and match the documented contracts.
• ViewerSettingsFile.Load/Save wire the guard into both stores identically — no drift between ViewerAppSettingsStore and ViewerPreferencesStore.
• ViewerLogger.Initialize() was moved ahead of the first viewer-settings.json read in App.xaml.cs, and base.OnStartup(e) (which triggers StartupUri → MainWindow) runs after that, so the ordering claim in the comment holds — no diagnostic is dropped on the floor.
• The SettingsFileGuard move to PerformanceMonitor.Common is a clean rename (no stale PerformanceMonitorLite.Services references left behind), and both Lite and Darling.Viewer project references already exist so nothing new needs wiring.
• This is a Darling-only functional change layered on shared infrastructure — no Lite/Darling parity drift; Lite's own store already went through the equivalent fix in A malformed settings.json silently resets all 88 Lite settings, before logging exists to say so #2425 and this PR only relocates the shared piece.
• No SQL/T-SQL in this diff, no missing-index suggestions applicable.

One finding posted inline: a doc-comment placement bug in MainWindow.xaml.cs — the new WarnSettingNotSaved XML doc summary got inserted between the pre-existing OnDisplayModeChanged summary and the OnDisplayModeChanged method itself, so OnDisplayModeChanged now has no doc comment and WarnSettingNotSaved's doc reads like it documents OnDisplayModeChanged's neighbor block instead. Cosmetic (doesn't affect behavior), but worth a quick swap.

[claude]

Minor diagnostic-quality nit, not a correctness bug: this move is correct and well-justified by the comment above it — but it has a side effect worth flagging. A few lines down, ThemeManager.Apply(new ViewerAppSettingsStore().Load().ColorTheme) creates a second, independent ViewerAppSettingsStore that reads the same viewer-settings.json MainWindow's own _appSettingsStore will load moments later (MainWindow.xaml.cs constructor, ~L172-183).

ViewerSettingsFile.Load<T> logs a ViewerLogger.Error unconditionally whenever State == Unreadable, with no dedup. So a single corrupt viewer-settings.json now produces two near-identical "could not be read" ERROR log lines at startup — one from this throwaway theme-load instance, one from MainWindow's real store — even though only one dialog is ever shown (ReportUnreadableSettingsFiles only reads MainWindow's own store state). That's a bit self-defeating for a PR whose whole point is making the log trustworthy for someone diagnosing a lost-settings report: two log lines with no indication they refer to the same read.

Not blocking, but worth considering having MainWindow accept the already-loaded settings from App (or the store instance) instead of loading viewer-settings.json a second time, so there's one authoritative read (and one log line) per startup.

[claude]

Reviewed. This PR is entirely C#/WPF (no T-SQL), so the T-SQL style conventions in CONTRIBUTING.md don't apply here.

Design/correctness: The SettingsFileGuard → PerformanceMonitor.Common move and the new ReadObject<T>/PermitReplace pair are sound. ViewerSettingsFile.Load/Save correctly thread SettingsFileState through both viewer stores, the three-state split (Absent/Readable/Unreadable) is preserved end-to-end into LastLoadState/LastLoadProblem, and every Save call site in the viewer (MainWindow.xaml.cs ×2, MainWindow.Tags.cs, SettingsWindow.xaml.cs) now checks the bool return and reports appropriately (dialog for user-initiated saves, log-only for the sidebar collapse write). The quarantine-before-replace logic in PermitReplace is copy-not-move, which is the right call given the write that follows it can itself fail.

Ordering fix verified: ViewerLogger.Initialize() moving ahead of the theme read in App.xaml.cs checks out — ViewerLogger.Log drops anything enqueued before Initialize (confirmed in ViewerLogger.cs), so without this move the earliest possible diagnostic really would vanish.

Parity: No Lite/Darling drift introduced — Lite's merge-based SettingsFileGuard usage (RootForWrite, throw-based refusal, callers already wrapping in try/catch) is untouched aside from the namespace fix from the file move. The viewer's whole-object-replace shape genuinely needs the new ReadObject/PermitReplace members Lite doesn't, as the PR body explains — that's a legitimate asymmetry, not drift.

One minor finding posted inline on App.xaml.cs: moving ViewerLogger.Initialize() earlier is correct, but it means the throwaway ViewerAppSettingsStore used for the theme read and MainWindow's own _appSettingsStore both call Load() against the same viewer-settings.json at startup, so a single unreadable file now logs the same ERROR twice. Not data-affecting, just log noise in the exact area this PR is trying to make trustworthy.

Doc-comment ordering fix (3rd commit) and the new test coverage (ViewerSettingsFileGuardTests, ViewerSettingsStoreWiringTests) both look correct and well-targeted at the actual defect (wiring, not just behavior).

[claude]

Save() now returns whether the write actually reached disk, but every one of its callers in this file discards that bool: AddServer (L155), both UpdateServer overloads (L167, L175), DeleteServer (L185), ImportServersFromFile (L219), ToggleFavorite (L249) and SetFavorite (L292)/SetViewFilterDatabases (L332) all call Save(); and move on regardless of the result.

That means the quarantine-and-refuse path this PR adds (PermitReplace returning Allowed: false — file unreadable and the quarantine copy itself failed) is unreachable from the UI for this store. Concretely:

• AddServerDialog.xaml.cs calls _serverStore.SetFavorite(...) and immediately sets DialogResult = true; Close(); — the dialog reports success even if nothing was written.
• MainWindow.ServerManagement.cs (~L657) shows "Imported {imported} server definition(s)." after ImportServersFromFile unconditionally, with no check that the resulting Save() succeeded.
• The sidebar favorite-star (ToggleFavorite, MainWindow.ServerManagement.cs:261) flips the UI state but a failed underlying save is never surfaced — unlike viewer-settings.json/viewer-preferences.json, which now show a WarnSettingNotSaved dialog on every failed save (MainWindow.xaml.cs L1140/L1188/L1706, MainWindow.Tags.cs L122).

The PR's own description calls this store "the one where losing the file costs the operator the most," so of the three stores it's the one where this gap matters most. Worth threading the bool (or at least logging + a warning like the other two stores get) through the mutators that are reachable from user actions, especially ToggleFavorite/SetFavorite/ImportServersFromFile.

[claude]

Reviewed the settings-persistence fix (#2434). No T-SQL in this PR, so the T-SQL style/parity rules don't apply — this is a Lite/Darling C# parity fix, and it's well-targeted: SettingsFileGuard moved to PerformanceMonitor.Common with the classify-then-quarantine logic shared, while the merge-vs-replace divergence between Lite (RootForWrite, JsonObject merge) and the viewer (PermitReplace/ReadObject<T>, whole-object replace) is explicitly justified in the comments rather than accidental drift. ViewerLogger.Initialize() moving ahead of the first settings read in App.xaml.cs is correct and low-risk (it's a no-op-safe, idempotent call). Test coverage (ViewerSettingsFileGuardTests, plus the source-scanning wiring tests banning Debug.WriteLine and un-guarded File.*(_filePath...)) is solid and should catch regressions.

Left one inline finding: ViewerServerStore.Save()'s new bool return (whether the write actually reached disk) is discarded by every mutator that calls it — AddServer, UpdateServer, DeleteServer, ImportServersFromFile, ToggleFavorite, SetFavorite, SetViewFilterDatabases — so a failed/refused save on the server registry never reaches the UI, unlike the other two stores which now show WarnSettingNotSaved. Given the PR's own framing that the registry is the store where data loss costs the operator the most, this seems worth closing before merge.

No security, SQL injection, or missing-index concerns — this PR doesn't touch T-SQL or the DMV surface at all.

[claude]

Reviewed this PR (C#/WPF, no T-SQL touched, so the CONTRIBUTING.md T-SQL style section doesn't apply here — no missing-index folklore concerns either since there's no SQL in the diff).

Went through the full diff carefully: SettingsFileGuard's move to PerformanceMonitor.Common (all Lite call sites — App.xaml.cs, Mcp/McpSettings.cs, Lite.Tests— updated to the new namespace, no orphaned references to the old PerformanceMonitorLite.Services.SettingsFileGuard), the new ReadObject<T>/PermitReplace<T> pair and their deliberate divergence from Read/RootForWrite (no "root must be an object" rule, since the server registry is a legitimate JSON array — correctly pinned by Save_LeavesNoCopyBehind_ForAHealthyRegistry_ThoughItsRootIsAnArray), the three viewer stores' switch from Debug.WriteLine (compiled out of Release builds) to ViewerLogger, and the ViewerLogger.Initialize() reordering in App.OnStartup ahead of the theme read.

Things I specifically checked and didn't find issues with:

• Field-initializer ordering: MainWindow's constructor reads _serverStore.LastLoadState where _serverStore is a field declared in a different partial file (MainWindow.ServerManagement.cs). This is safe — all field initializers across every partial-class file run before any constructor body executes, regardless of file order.
• LastSaveSucceeded staleness in the import flow (MainWindow.ServerManagement.cs, ImportSettingsButton_Click): confirmed ImportFromStoreAsync never calls _serverStore.Save() (it only reads via GetAllServers()/GetCredential), so _serverStore.LastSaveSucceeded checked after it still reflects the correct ImportServersFromFile save, not a later unrelated write.
• Every Store.Save(...) call site (MainWindow.xaml.cs, MainWindow.Tags.cs, SettingsWindow.xaml.cs) checks the new bool return value — none silently drop a failed save.
• ReadObject<T>'s narrower exception filter (JsonException/NotSupportedException/ArgumentException vs. the old blanket catch) matches JsonSerializer.Deserialize's documented exception surface, and none of the three POCOs (ViewerAppSettings, ViewerPreferences, ViewerServerEntry) use custom converters that could throw outside that set.
• No parity gap: this bug (Darling's viewer silently replaces a corrupt settings file with defaults, on a click #2434) and its fix are Darling-viewer-specific (three JSON stores that don't exist in Lite); Lite's own SettingsFileGuard-based stores already got the A malformed settings.json silently resets all 88 Lite settings, before logging exists to say so #2425 treatment and are unchanged here beyond the namespace move.
• New files (ViewerSettingsFile.cs, ViewerSettingsFileGuardTests.cs) carry copyright headers.

No correctness, security, parity, or performance findings to raise — this is a well-scoped, well-tested fix (including a source-parsing wiring test to guard against a future store reintroducing Debug.WriteLine or bypassing the guard).