A settings file that fails to deserialize is deleted outright and replaced with defaults, with no backup left to recover from

[matt-edmondson]

What's wrong

When the file can't be deserialized, AppData<T>.LoadOrCreate does this (AppDataStorage/AppData.cs):

catch (JsonException)
{
    // file was corrupt or could not be deserialized
    // delete and try load a backup
    AppData.FileSystem.File.Delete(newAppData.FilePath);
    return LoadOrCreate(subdirectory, fileName);
}

The comment says it deletes the file and then loads a backup, but in normal operation no backup exists. WriteText removes the .bk file as the last step of every successful save (FileSystem.File.Delete(bkFilePath);). A .bk or .tmp only survives a process that crashed mid-save. So in practice, once the file fails to deserialize, the user's only copy of their data is permanently deleted, and a fresh default instance is saved over the same path.

TryRestoreFrom already archives the recovery candidates it promotes ("Archiving rather than deleting keeps the recovered content for inspection"). The main file gets no such protection.

Repro

Run against the current main source with MockFileSystem:

1. LoadOrCreate(), set Data = "years of user settings", Save(). The directory now holds only test_app_data.json, with no .bk.
2. Make a one-character hand edit that breaks the JSON (a stray ,).
3. LoadOrCreate() returns Data = "". The directory holds only test_app_data.json, which now contains the defaults. No file anywhere contains the original text.

Why it matters

A JsonException is not just a sign of disk corruption. Ordinary things raise it:

• An app update changes the model. For example, it renames or removes an enum member (JsonStringEnumConverter throws on an unknown name) or changes a property's type. The first launch of the new version silently resets every user's settings to defaults, and the old file is gone, so the developer can't write a migration after the fact.
• The user hand-edits a settings file and leaves a trailing comma. All their settings are wiped instead of being kept for them to fix.

In both cases, nothing is logged or thrown, and nothing is left on disk to recover from.

Suggested fix

• Replace File.Delete with a move to an archived name, such as <file>.corrupt.<yyyyMMdd_HHmmss>, deduplicated with the same counter loop TryRestoreFrom uses. The recursive LoadOrCreate still finds the main file missing and falls back to the temp, then the backup, then defaults. The unreadable content stays on disk for the user or a migration to recover.
• Optionally, expose the archive (an event, or a property with the archived path) so an app can tell the user that its settings were reset and where the old file went.
• Correct the comment, since in the common case no backup exists.

Acceptance: after the repro above, the directory contains the defaults file plus an archived copy with the original (unparseable) content. The existing TestLoadOrCreateHandlesCorruptFile and recursive-recovery tests still pass.

[matt-edmondson]

Triage

• Category: Bug
• Priority: High. This silently destroys user data. When a JsonException is thrown, the only copy of the settings file is deleted and overwritten with defaults, and nothing is logged. Common, non-corruption triggers set it off: a model change in an app update, such as a renamed enum member or a changed property type, or a hand edit that leaves a trailing comma. Every consumer of ktsu.AppDataStorage inherits the behaviour.
• Area / suggested owner: AppDataStorage/AppData.cs, in the persistence and recovery path: LoadOrCreate, alongside TryRestoreFrom and WriteText. The fix in the issue is small and self-contained. Archive the file with the same counter loop TryRestoreFrom uses instead of calling File.Delete, and correct the misleading comment.
• Duplicates: None found in the org.
• In progress: No open PR covers this. The only open AppDataStorage PR is Bump the ktsu group with 4 updates #309, a dependency bump.
• Next step: Add the acceptance test from the issue first: after a corrupt load, the directory holds the defaults file plus a .corrupt.<timestamp> archive. Then swap the delete for the move. The optional event or property that reports the archive path can follow in a separate change.

---

Generated by Claude Code