fix: atomic config writes (no more truncated JSON on crash)#252
fix: atomic config writes (no more truncated JSON on crash)#252erikdarlingdata merged 1 commit intodevfrom
Conversation
Added Services/AtomicFile.WriteAllText which writes to a sibling .tmp and then renames into place. File.Move(src, dst, overwrite: true) maps to MoveFileEx(MOVEFILE_REPLACE_EXISTING) on Windows and rename(2) on Unix — both atomic when source and destination share a filesystem, which is always the case here. Swapped File.WriteAllText for AtomicFile.WriteAllText at every config-save site: - ConnectionStore.Save (saved server list — the worst-case loss) - AppSettingsService.Save (recent/open plans, slicer days-back, etc.) - SqlFormatSettingsService.Save (format options) - AboutWindow.SaveMcpSettings (MCP enable + port) A crash between the .tmp write and the rename leaves the original file intact and a stray .tmp sibling; the next save overwrites .tmp before renaming, so no manual cleanup is needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
erikdarlingdata
left a comment
There was a problem hiding this comment.
What this does
Routes all four per-user config save sites (ConnectionStore, AppSettingsService, SqlFormatSettingsService, AboutWindow.SaveMcpSettings) through a new Services/AtomicFile.WriteAllText helper that writes to path + ".tmp" and then File.Move(..., overwrite: true) into place. Fixes the silent "load catches JsonException, replaces user data with defaults" path that was wiping the saved-server list on a mid-write crash.
What's good
- Targets
dev, notmain. ✅ - Fix is minimal and local — no MVVM creep, no XAML churn, no new brushes, no Avalonia-specific gotchas in play here.
- Correct platform understanding:
File.Move(..., overwrite: true)maps toMoveFileEx(MOVEFILE_REPLACE_EXISTING)on Windows andrename(2)on Unix, both atomic on same-filesystem moves (which is guaranteed here since tmp is a sibling). - Self-healing stray
.tmpstory is correct — the nextFile.WriteAllText(tmp, ...)just overwrites it.
What needs attention
- Durability claim is slightly overstated — see inline on
AtomicFile.cs:25. This fixes process-crash truncation but not power-loss truncation (no fsync on tmp, no fsync on dir). - Stylistic inconsistency at
AboutWindow.axaml.cs:59— qualifies asServices.AtomicFiledespite theusingbeing present; the other three sites don't. - No tests added.
tests/PlanViewer.Core.Tests/hasHtmlExporterTests,PlanAnalyzerTests, etc., but nothing forAtomicFile. The PR description references an out-of-tree local harness — worth landing those four cases (happy path, overwrite, stray-tmp cleanup, crash-sim via pre-created tmp) as anAtomicFileTests.csso the invariants don't quietly regress. NoteAtomicFileisinternal, so eitherInternalsVisibleToor bump topublic. - No PlanAnalyzer rule/scoring changes, so no Dashboard/Lite sync work implied for this PR.
Non-blocking — comments only. PR is already merged; flagging for follow-up if any of the above land as a tidy-up commit.
Generated by Claude Code
|
|
||
| Directory.CreateDirectory(settingsDir); | ||
| File.WriteAllText(settingsFile, json); | ||
| Services.AtomicFile.WriteAllText(settingsFile, json); |
There was a problem hiding this comment.
Style nit: this file already has using PlanViewer.App.Services; at line 18, so the Services. qualifier is redundant. The other three call sites in the PR use the unqualified AtomicFile.WriteAllText(...) — consistent to drop Services. here too.
Generated by Claude Code
| // File.Move with overwrite:true maps to MoveFileEx(MOVEFILE_REPLACE_EXISTING) | ||
| // on Windows and rename(2) on Unix — both atomic when source and destination | ||
| // live on the same filesystem, which is always the case here. | ||
| File.Move(tmp, path, overwrite: true); |
There was a problem hiding this comment.
Scope caveat vs. the PR description: this closes the process-crash truncation hole (good — that's the bug that was actually hitting users), but it does not survive power loss. File.WriteAllText doesn't fsync the tmp, and File.Move doesn't fsync the containing directory, so after a sudden power cut the kernel can replay the rename without the new data being on disk — same truncated/empty JSON on next boot. If durability-across-power-loss is a real goal, you'd need FileStream.Flush(true) on the tmp before the rename (and ideally an fsync on the dir on Unix). If the goal really is just "process crashes don't nuke my server list," the description overstates it — consider dropping "power loss" from the rationale, or add the flush.
Generated by Claude Code
| /// </summary> | ||
| public static void WriteAllText(string path, string contents) | ||
| { | ||
| var tmp = path + ".tmp"; |
There was a problem hiding this comment.
Minor: two processes calling Save concurrently will both write to the same path + ".tmp" and race on the rename. For a single-user desktop app this is effectively unreachable (one instance, UI thread serializes saves), but if anything ever calls this off the UI thread, a Guid-suffixed tmp (path + "." + Guid.NewGuid().ToString("N") + ".tmp") would make it safe. Not blocking given current usage — just flagging.
Generated by Claude Code
Summary
All four per-user config save sites called
File.WriteAllTextdirectly — a process crash (power loss, kill, OOM) mid-write leaves a truncated JSON file, and on the next load the config-loaders catch theJsonExceptionand silently replace the user's data with defaults. TheConnectionStore.Savecase is the worst — entire saved-server list gone.Fix
New helper
Services/AtomicFile.WriteAllText: write to a sibling.tmp, then rename into place withFile.Move(src, dst, overwrite: true), which maps to:MoveFileEx(MOVEFILE_REPLACE_EXISTING)— atomic on NTFS.rename(2)— atomic by spec.Applied at all four save sites:
Services/ConnectionStore.cs— saved server listServices/AppSettingsService.cs— recent/open plans, slicer daysServices/SqlFormatSettingsService.cs— format optionsAboutWindow.axaml.cs— MCP enable + portA crash between the tmp write and the rename leaves the original file intact and a stray
.tmpbehind; the next save overwrites that.tmpbefore renaming, so no manual cleanup is needed.Test plan
dotnet buildon App: 0 errors..tmppopulated but never renamed → target preserves old contents ✅.tmpand writes target ✅File.WriteAllTextinterrupted mid-write does truncate the file (this is the bug the fix avoids) ✅🤖 Generated with Claude Code