Move Lite's data out of the installer's install root (#1832)

[erikdarlingdata]

Summary

Lite's data root was %LOCALAPPDATA%\PerformanceMonitorLite — which is also Velopack's install root (vpk pack -u PerformanceMonitorLite, no --installto). Re-running Setup.exe over an existing install renames that folder aside and deletes it, so an installer-based upgrade destroyed the DuckDB store, the Parquet archive, the logs, and settings.json before any code from the new build ran. In-app updates were always safe — Velopack replaces the current\ subfolder in place — which is why the loss looked random rather than reproducible.

1. New data root. %LOCALAPPDATA%\PerformanceMonitorLite-Data, a literal sibling. Local, not roaming: the DuckDB store must not be dragged across a roaming profile. Every path App derived from the old root moves with it — config\, archive\, logs\, monitor.duckdb (+ .wal, + DuckDB's .tmp scratch dir), and alert_state.json. Those are the only paths anything derives from App.DataDirectory; Environment.SpecialFolder.LocalApplicationData appears exactly twice in Lite/, both in App.xaml.cs.

2. Startup migration (Lite/Services/DataRootMigration.cs), running before anything reads settings or opens the store. Same-volume Directory.Move/File.Move, so a multi-GB store transfers instantly. Nothing already in the new root is overwritten, nothing anywhere is deleted, and a DATA-MOVED.txt signpost is left in the old root.

3. Settings-save tripwire (second defect, same failure). App.LoadAlertSettings read the four webhook secrets out of Credential Manager at the tail of its settings.json parse — behind if (!File.Exists(path)) return;. With settings.json gone (exactly what the wipe produced) the load returned before those reads, so the Settings window's webhook boxes rendered empty while the credentials were still in the store; Save then wrote the blanks back through SaveWebhookUrl, which deletes on blank. The secrets that survived the data loss were destroyed by opening the settings window afterwards. The reads now run first and unconditionally. Save is otherwise unchanged — blank still means delete, which is how you clear a webhook on purpose.

4. Update discoverability. One tray balloon per launch when an update is found, pointing at Help > About and saying that route updates in place and keeps your data. The title-bar suffix was the only signal, and a title bar nobody reads is how installs both stayed on old builds and got "upgraded" by re-downloading Setup.exe. Nothing else about the update UX changed.

5. Docs. README.md now states the real location and carries an explicit warning that on pre-fix versions Setup.exe deletes local data (upgrade in-app or via ZIP). Three other stale references fixed: docs/how-collection-works.md (which also named a database file, pm_lite.duckdb, that has not existed for some time) and the bug-report issue template's log path. The CHANGELOG [Unreleased] entry doubles as the public advisory, including the one-time caveat that reaching this version via Setup.exe still loses data once, because the installer deletes before the migrating code runs.

Two deliberate design points

The migration is an explicit allow-list, not "move the folder contents." The old root also holds Velopack's Update.exe, current\, packages\ and velopack.log — confirmed present in a real install. Moving those would break the installed app and its updater.

The gate is per-artifact rather than the specified all-or-nothing pair. The brief said migrate only when the new root lacks both config\settings.json and monitor.duckdb. That strands data on an interrupted run: if config\ moves and the store then fails (a locked 125 MB file, a killed process), the new root has settings.json, the gate never fires again, and the store sits permanently in the folder the next Setup.exe deletes — the exact failure this PR exists to fix. So each artifact moves only when the target does not exist, and the "both roots complete" case still short-circuits to a single log line with the old root untouched. Every case in the brief behaves as specified; the interrupted run additionally self-heals. Covered by Migrate_FinishesAnInterruptedRun_WithoutOverwritingWhatAlreadyLanded.

Test plan

• dotnet build Lite.Tests -t:Rebuild: Build succeeded, 0 Warning(s), 0 Error(s) (repo enforces zero warnings).
• Full Lite.Tests suite: 1637 passed, 0 failed.
• 7 new DataRootMigrationTests (temp dirs): no legacy root → no-op and the new root is not even created; legacy root holding only updater files → no-op; populated legacy + absent new → everything ours moves with contents verified, updater files intact, old root not deleted, marker written and naming the new path; both roots complete → nothing moves, no marker, and the skip is logged rather than silent; partial legacy → moves only what exists; interrupted run → the store follows while the already-landed config is kept and reported in Kept; self-move guarded.
• 4 new AlertSettingsCredentialLoadTests: all four credential keys are read with settings.json absent and with it corrupt, landing on the statics SettingsWindow binds its boxes to; the settings-present path still parses; the [QUESTION] Connection is broken: need PM restart ? #1506 legacy-plaintext-URL migration still overrides the stored value. The internal overload takes the credential writer as a required parameter specifically so a test cannot silently overwrite the operator's real webhook URL — the tests that must not write pass a FailOnWrite that asserts.
• Mutation red-watch (test project rebuilt each time, then restored):
  • Credential reads put back below the early return → 2 tests red (Collection: [], key never requested).
  • No-overwrite guard removed → Migrate_FinishesAnInterruptedRun... red.
  • Allow-list widened to include current\, packages\, Update.exe, velopack.log → 4 tests red.
• AppAlertSettingsTests joined to a shared collection with the new class: both write the App webhook statics and xUnit runs classes in parallel, so this prevents a flake the new tests would otherwise have introduced.
• Not run: Installer.Tests (DB-touching), per instructions.

🤖 Generated with Claude Code

[claude]

Correctness: a single failed per-artifact move permanently strands data in the directory the next Setup.exe run deletes — the exact failure class this PR exists to prevent.

The "already there → keep, don't overwrite" check here (and the HasCompleteStore short-circuit above at lines 132-141) can't distinguish "this artifact was already fully migrated" from "something else created an empty/partial artifact at the target path after a previous move attempt failed." Once anything exists at target, this loop marks it kept and never retries the source, forever.

Concretely, with monitor.duckdb: if Directory.Move/File.Move for monitor.duckdb fails once (the PR description's own motivating case — "a locked 125 MB file, a killed process"), App.DatabasePath still points at the new root, and DuckDB auto-creates a fresh empty database file there the moment the app opens it this session. On the next launch, HasCompleteStore(newRoot) now finds monitor.duckdb present (the fresh empty one) — so if config/settings.json also happens to be there, the whole function short-circuits to AlreadyMigrated and never looks at the legacy root again. The real multi-GB store with all monitoring history sits untouched in the legacy install directory forever, and the next Setup.exe upgrade deletes it — full data loss from a single transient lock, silently.

A narrower but more likely trigger: if config specifically fails to move, App.xaml.cs unconditionally runs Directory.CreateDirectory(ConfigDirectory) + ConfigSeeder.SeedMissing(...) right after Migrate() returns (lines ~390-399), regardless of migration.Failed. That creates a config folder at the new root this same session, seeded only with the two default files ConfigSeeder knows about (not settings.json). Every future launch then sees config already existing at target and treats it as kept — the user's real settings.json (alert thresholds, SMTP config, etc.) never migrates and is deleted by the next installer run, while the app silently runs on defaults from the seeded stand-in.

None of the DataRootMigrationTests cover "one artifact's move fails while others succeed, then something recreates an empty version of it at the target before the next Migrate() call" — worth a test, and probably worth having Migrate treat a Failed item from a prior attempt as needing a real overwrite-if-target-is-just-the-seeded-default check, or at minimum have HasCompleteStore/the per-item kept check consult something more durable (e.g. the marker file, or a size/content check) than bare existence.

[claude]

Review: #1842 — Move Lite's data out of the installer's install root

Scope check: This PR is correctly Lite-only. Darling has no analogous exposure — its Windows service stores data under %ProgramData%\PerformanceMonitorDarling (never inside a Velopack install root), and its webhook config lives directly in darling.json (DarlingAlertSettings.cs), not behind a Credential-Manager-read-gated-by-settings.json-existence pattern like Lite's. So there's no Lite/Darling parity gap to flag here.

Correctness — one significant issue (inline comment on DataRootMigration.cs)

The per-artifact retry design (moving away from the brief's all-or-nothing gate specifically to avoid stranding data on an interrupted run) has a real hole: once anything exists at the target path for an artifact, the loop's Directory.Exists(target)/File.Exists(target) check treats it as already-migrated and never retries the source — regardless of why something exists there. Two concrete trigger paths:

1. If monitor.duckdb's move fails once (the PR's own cited motivating case — a locked file), DuckDB auto-creates a fresh empty database at the new path the moment the app opens it that session. Next launch, HasCompleteStore sees monitor.duckdb present and (if config/settings.json also landed) short-circuits to AlreadyMigrated — the real multi-GB store is stranded in the legacy root forever and gets deleted by the next Setup.exe run. This is exactly the data-loss class the PR exists to close, surviving in narrower form.
2. If config specifically fails to move, App.xaml.cs unconditionally runs Directory.CreateDirectory(ConfigDirectory) + ConfigSeeder.SeedMissing(...) right after Migrate() returns, regardless of migration.Failed. That plants an empty, then default-seeded, config folder at the target this same session, permanently masking the user's real settings.json from ever migrating.

Full detail and a couple of mitigation ideas are on the inline comment. None of the seven DataRootMigrationTests (including the interrupted-run one) cover "one artifact fails to move while its siblings succeed, and something else populates the target path before the next Migrate() call" — that gap is exactly why this got through the otherwise-thorough test suite and mutation pass described in the PR body.

Everything else looks solid

• Settings-save tripwire fix (App.LoadAlertSettings hoisting the four credential reads above the settings.json early-return): logic checks out — reads now always populate the statics SettingsWindow binds to, the [QUESTION] Connection is broken: need PM restart ? #1506 legacy-plaintext-URL override still writes-then-sets-live-value correctly, and the new internal overload's required writeSecret parameter is a good guard against tests silently hitting the real Credential Manager.
• Migration allow-list vs. Velopack's own files: the explicit allow-list (config, archive, logs, monitor.duckdb.tmp, monitor.duckdb, monitor.duckdb.wal, alert_state.json) correctly excludes Update.exe, current\, packages\, velopack.log — verified against the test fixtures.
• Self-move guard (Path.GetFullPath + TrimEndingDirectorySeparator comparison) is sound for a Windows-only app.
• Ordering in App.xaml.cs: migration runs before the single-instance-owned process reads settings or opens the store, and after the single-instance mutex is acquired, so no cross-process race during the move.
• Tray update notification: CheckForUpdatesOnStartupAsync runs once per launch, so AnnounceUpdate can't double-fire; _trayService?.ShowNotification(...) is called from the UI dispatcher correctly.
• Docs/CHANGELOG: paths and the one-time Setup.exe-upgrade caveat are consistent across README, the issue template, docs/how-collection-works.md, and the CHANGELOG entry.
• No SQL changes in this PR, so the T-SQL style guide / missing-index-DMV concerns don't apply here.

Nice, thorough test coverage and mutation testing overall — the one gap above is a real one worth closing before this ships, given how high the cost of getting it wrong is (silent, permanent loss of exactly the data this PR is trying to protect).