Protect settings files from overwrite, and fix three save bugs

[erikdarlingdata]

What does this PR do?

It fixes four settings-persistence bugs. Each one matched the code as described, so none was skipped.

F1: unreadable or corrupt settings files were overwritten with defaults

Three stores returned defaults when their file failed to load: AppSettingsService, ConnectionStore, and SettingsFile (the MCP port and proxy settings). The next save then wrote those defaults over the user's real file. A new shared helper, SettingsFileStore, gives all three stores one rule:

• A malformed file is moved aside to <file name>.bad-<UTC yyyyMMddHHmmssfff> in the same folder, with a counter added if that name is taken. A file is malformed when its text does not parse, or when it parses to the wrong shape. The store then returns defaults, and saves work normally. The user's text survives in the .bad file. Two readers can find the same broken file at once, for example the UI and the MCP server. The second finds it already moved aside, and that counts as moved aside, not unreadable.
• An unreadable file exists, but the read fails, for example because another process locked it. The store returns defaults and refuses to save over the file. If the move aside itself fails, the file counts as unreadable.
• A missing file gives defaults, and saves are allowed, as before.

SettingsFile also treated valid JSON of the wrong shape, such as an array, as an empty file. That case is now malformed.

A refused save works differently in each store:

• AppSettingsService.Save already ignored write failures, so it skips quietly. Its block lasts until the app closes. MainWindow keeps the settings from its first load and passes them to the Settings window, which saves a copy. After a failed read, those settings are defaults, so a later successful read must not let them be saved. The Settings window checks the block after its save. It shows why nothing was saved and stays open, instead of closing as if it had saved.
• ConnectionStore and SettingsFile.Update throw an IOException that names the file. Callers must handle it, and F7 does that. ConnectionStore.AddOrUpdate and SettingsFile.Update read the file first, and refuse when their own read fails, so a read on another thread cannot let them write. ConnectionStore.Save refuses until a later read of the file succeeds.

ConnectionStore had no test redirect. Its path pointed at the real ~/.planview folder, so a test that saved a connection wrote the developer's own list. It now has RedirectForTestHost, called from the same test-host setup as the other two stores.

The task text mentioned ConnectionStore.Remove. That method does not exist here. ConnectionStore has Load, Save, and AddOrUpdate, and ConnectionDialog is the only caller that saves.

F10: AtomicFile did not flush before the rename

AtomicFile.WriteAllText wrote its .tmp file and renamed it without flushing to disk. After a power loss, the renamed file can be empty. It now writes through a FileStream and calls Flush(flushToDisk: true) before the rename.

The bytes stay the same. A null encoding writes UTF-8 with no BOM. A given encoding writes its own preamble, as File.WriteAllText does. A separate commit fixes one edge that differed: the new default replaced a lone surrogate with U+FFFD, where File.WriteAllText throws. The default now throws too, and a test compares the two.

F7: a refused save crashed the app

ConnectionDialog.Connect_Click and SettingsWindow.Save_Click both let the IOException from F1 escape. Both now catch IOException and UnauthorizedAccessException, show the message, and stay open.

• The connection dialog reports into its existing StatusText. The save goes through a small TrySaveConnection helper, so a test can call it without a live SQL connection.
• The Settings window had no error display, so this adds a text block to the button bar. It stays hidden until a save fails. SaveIntegrations has two write points, the MCP write and ProxySettings.Save, and one try block covers both.

F8: the server-filter panel state was lost on the next MainWindow save

ServerFilterExpander_StateChanged cloned the cached settings, changed the clone, and saved it. AppSettingsService cached the clone, but MainWindow kept its own older reference. The next unrelated MainWindow save, such as opening a plan, wrote the older object back over the change. The handler now changes the shared cached instance in place, as every other settings save in MainWindow does. The new test drives the real grid control and MainWindow. I checked that it fails against the old code.

Which component(s) does this affect?

• Desktop App (PlanViewer.App)
• Core Library (PlanViewer.Core)
• CLI Tool (PlanViewer.Cli)
• SSMS Extension (PlanViewer.Ssms)
• Tests
• Documentation

How was this tested?

There are 21 new tests:

• SettingsFileStoreTests (12): the malformed, missing, and unreadable cases for all three stores, and the array-shaped SettingsFile case. Two more cover a file another reader already moved aside, and two quarantines of the same path.
• AtomicFileTests (5): byte-for-byte comparison with File.WriteAllText for a null encoding, UTF-8 with a BOM, and Encoding.Latin1. Latin1 is a legacy single-byte code page that needs no extra package. One test covers the lone-surrogate edge, and one covers overwriting.
• SaveFailureDisplayTests (3): each dialog shows the refusal message and stays open, including the Settings window when the app settings save is blocked.
• ServerFilterPanelPersistenceTests (1): the F8 regression test.

The unreadable tests lock the file with FileShare.None. A rename onto a locked file fails by itself, so a refusal seen under the lock proves little. The AppSettingsService and ConnectionStore tests therefore release the lock first. They then try the save before any successful read, and the file must stay untouched.

SettingsFile.Update reads first, so its refusal is only visible under the lock. Its test checks the refusal message instead. The ConnectionStore and SettingsFile tests then read again and check that saving works. The AppSettingsService test reads again and checks that saves stay refused, for the old defaults and for the newly read settings. With the blocking flags disabled, all three tests fail. I ran that check.

Five of these tests hold a file open, and they use Assert.SkipUnless(OperatingSystem.IsWindows(), ...). Unix file permissions do not reliably block a same-user read. Ubuntu CI skips those five tests. They ran here on Windows.

The new settings tests, SettingsIntegrationsTests and ServerFilterPanelPersistenceTests share one xunit collection, SettingsFileStore serial, and that collection runs on its own. The tests lock the same redirected files and turn on save blocks that apply to the whole process. While a block is on, a test in another class that saves settings fails for no reason of its own.

Full suite on Windows at 6e7d553: 1059 total, 0 failed, 1057 succeeded, 2 skipped. Both skipped tests were already skipped before this change. They are off-Windows contract tests. The existing save-in-place encoding tests in OpenSaveQueryTests and all of SettingsIntegrationsTests pass. The build has 0 warnings and 0 errors.

I copied the real .planview and PerformanceStudio settings folders aside as a backup. I took it after a few targeted test runs, not before the first one. All test settings files are redirected to a temp folder, and connections.json, settings.json, and appsettings.json had timestamps from before those runs. After the final full run, diff -rq against the backup reported no differences.

Checklist

• I have read the contributing guide
• My code builds with zero warnings (dotnet build -c Release, and the Debug build inside dotnet test)
• All tests pass (dotnet test)
• I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza

[claude]

Reviewed the diff (settings/connection file handling, AtomicFile fsync, dialog error surfacing). No correctness or security blockers. Untrusted plan XML and generated T-SQL aren't touched, and no version files changed, so no Ssms bump is needed.

Minor notes, none blocking:

1. SettingsFileStore.Quarantine: the .bad-<yyyyMMddHHmmss> name has one-second resolution and uses overwrite: false. Two quarantines in the same second, for example the UI thread and the MCP thread both reading a corrupt settings.json, make the second File.Move throw. That reader then reports Unreadable, so a corrupt-but-quarantined file can briefly block saves. Add a random or counter suffix, or treat "source no longer exists" as Malformed.
2. SettingsFile.Read and McpSettings: the MCP thread calls SettingsFile.Read(), which now has a side effect of moving the file aside. Writes go through AtomicFile, so a torn read is unlikely. Even so, a read-only MCP path renaming the user's settings file is surprising. Consider quarantining only on the write path.
3. AppSettingsService._saveBlocked: it is deliberately sticky for the whole session, and the doc explains why. The catch is that Save returns silently, so the user's settings edits are dropped for that session with no UI signal. The Settings dialog, unlike the connection and integrations paths, will close as if it saved. Consider surfacing this.
4. Tests: AtomicFileTests covers encoding parity, but nothing exercises the fsync behavior itself. That is understandable, since it can't be tested portably.

[claude]

Reviewed the diff. I found no correctness, untrusted-input, or security problems, and the change adds no SQL. I did not build the branch or run the tests.

The new failure handling looks consistent:

• SettingsFileStore separates malformed files (moved aside to a .bad- file) from unreadable ones (save refused).
• ConnectionStore.AddOrUpdate and SettingsFile.Update decide from their own read rather than from a shared flag.
• The only callers of the throwing save paths (ConnectionDialog, SettingsWindow) catch IOException and UnauthorizedAccessException.

Minor, non-blocking points:

• ConnectionStore.Save(List<>) throws when _saveBlocked is set, and the flag is a plain static written by both the UI and MCP threads. The value could be stale by a moment, but AddOrUpdate re-checks its own read, so I don't see a way for it to overwrite the file.
• In Save_Click, TimeDisplayHelper.Current is set before the save is known to succeed. When the save is blocked the dialog stays open, but the in-memory time display mode has already changed. This is cosmetic.
• SettingsFileStore.Read only catches IOException and UnauthorizedAccessException around ReadAllText. Any other exception type would propagate. For ConnectionStore and SettingsFile the old code caught all exceptions, so a rarer read failure such as SecurityException would now escape Load.
• The PR adds no NoWarn entries. Every changed path is in PlanViewer.App, so no Blazor link or version bump is needed. Test coverage for the new behaviour is present.

[erikdarlingdata]

Review notes, taken in 6e7d553 unless stated:

1. Fixed. Quarantine names now carry milliseconds. If that name is taken, a counter is added. A reader that finds the file already moved aside by another reader counts it as moved aside, not unreadable.
2. Left as is. All three stores move a broken file aside when they read it, including reads on the MCP server's thread. The user's text is kept in the .bad file, and the next save starts clean.
3. Fixed. The Settings window checks the block after its save. It shows why nothing was saved, keeps its edits, and stays open. Integrations still save, because they live in a different file.
4. No change. A flush to disk cannot be observed portably from a test.

The settings-file test collection now also runs on its own. Its save blocks apply to the whole process. A test in another class that saved settings at that moment failed for no reason of its own.