Never learn a transient state as a database's expected state (#2189)

CHANGELOG.md

@@ -30,6 +30,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **A database observed mid-restore no longer learns RESTORING as its expected state and then alerts forever for being healthy** ([#2189]) - found by dogfooding: the production 52-server box fired 636 Database State alerts in 24 hours from 5 databases, every one of them reading `Expected: RESTORING, Current: ONLINE`. Each had been swept into monitoring during a consolidation while a restore was running, had RESTORING written as its accepted baseline, and had been "deviating" ever since by being healthy - permanently, since the only escape was an operator noticing and re-baselining by hand. Two halves. The seed now refuses to learn the TRANSIENT operational states (RESTORING, RECOVERING) the same way it already refused the integrity ones: a database in the middle of an operation is not a steady state anybody would choose as expected, so it stays pending and silent until it settles into one, and when the restore completes ONLINE is what gets learned. That governs rows that do not exist yet and cannot touch the ones already written, so the second half applies the same rule after the fact - an AUTO-seeded baseline recording a state the seed would refuse to learn is not a baseline anyone chose, and once that database's effective state reaches ONLINE the steady state is learned instead. The already-poisoned rows therefore repair themselves on the next sweep, with no migration and no manual step, and the route that stays open forever is covered too: pressing "reset to current" while a restore is running (or during an outage) records whatever it sees with no state filter at all, and that now un-writes itself. What the repair deliberately does NOT touch is as load-bearing as what it does, because every case here is one where being wrong means silence. It never rewrites a baseline an OPERATOR declared, so #2166's composition contract holds exactly as specified: a database you park at expected OFFLINE stays quiet while parked and still alerts the moment it comes back ONLINE. It never touches an OFFLINE or STANDBY baseline even though the machine inferred those, because both are steady states and leaving one is real news - a STANDBY secondary that turns up truly ONLINE has stopped being a secondary, which means somebody recovered it and log shipping is broken, and an auto-baselined OFFLINE database brought up for an hour of maintenance and re-parked would otherwise come back deviating forever against a baseline it never had. And it matches the EFFECTIVE state rather than `state_desc`, because a standby secondary reports `state_desc = ONLINE` with `is_in_standby` set - matching the raw column would have re-baselined every such secondary and then alerted it forever for being STANDBY, this same bug recreated for the one database family the alert works hardest to keep quiet. A log-shipping secondary restored WITH NORECOVERY, which sits in RESTORING permanently, stays silent forever as before; it now gets there by never being baselined rather than by baselining RESTORING, and an operator who wants deviation coverage on one sets its expected state explicitly. Both SKUs. (This is the seed and baseline logic, not #2166's alerted-state memory, which remains Darling-only until parity lands.)
+
 - **The doc-comment hygiene pin now catches a stacked summary whose first block is never closed** ([#2190]) - `NoMemberCarriesTwoStackedSummaryBlocks` keyed off a `</summary>` immediately followed by a reopening, so it only ever saw a stacked pair when the FIRST block was closed, and two instances were sitting on dev unseen: a duplicated opening tag on `ApplyProcessEnvironment` in the Darling service, and a doc block that #1912's restructure split in `QueryStoreSliceRepairService`, stranding its unclosed head on top of `PromoteRewrittenFileAsync`. The rule now counts `<summary>` OPENINGS inside each contiguous run of `///` lines - a run documents exactly one member, so two openings means two summaries whether or not either is closed, and whether they are written single-line or spread over many. That mixed form is the one a closing-tag matcher cannot see at all, and it is what defeated the first attempt at this fix. Both live instances are repaired: the duplicate tag is deleted, and the stranded head is deleted rather than moved back, because the same restructure had already re-documented `FlushExternalFileCacheAsync` in place with a superset of that one sentence, so moving it would have recreated the exact duplicate this rule exists to forbid. The detector now carries its own self-test as well, pinning the five stacked shapes it must catch and the four legitimate ones it must leave alone, since this was a blind spot in the DETECTOR rather than in anyone's reading of the tree.
 - **The installer refuses an install directory the service could never read** ([#2187], split out of #2185 reported by @jovon44) - a zip extracted to `C:\Users\<someone>\Desktop\PerformanceMonitorDarling-3.2.0\` - a completely reasonable thing to do with a download - installed without a complaint and produced a service that could not work, and nothing told the operator why. The service deliberately runs as the unprivileged virtual account `NT SERVICE\PerformanceMonitor Darling` and never as LocalSystem, because the bundled PostgreSQL refuses to run with administrative privileges; that account is not the installing user, not SYSTEM, and not Administrators, which is approximately everyone a user profile grants anything to. Measured on Windows 11, a directory created under a profile inherits exactly SYSTEM / Administrators / the profile owner - no `BUILTIN\Users`, no Authenticated Users, no CREATOR OWNER - so the service cannot read its own program files, and cannot read back even what it writes there itself; the bundled PostgreSQL's `initdb.exe` died at exit code -1073741515 (`0xC0000135`, STATUS_DLL_NOT_FOUND) before writing a word of output. `install-darling.ps1` now refuses a fresh install anywhere under the machine's profile root (read from `ProfileList\ProfilesDirectory` rather than assumed to be `C:\Users`, since it is relocatable) or on a UNC / mapped-drive path - where a virtual account reaches the network as the computer account rather than as the operator, and cannot see a drive letter belonging to someone else's logon session at all. The refusal names the account, the mechanism, and `C:\PerformanceMonitorDarling` as the fix, and it runs before the pre-flight, the Event Log source, and `sc create`, so a doomed location costs nothing and leaves nothing behind. ACLing the install tree to make a profile path work instead was considered and rejected: it would mean the product silently changing permissions inside somebody's profile. An **upgrade** of a service that already lives there asks rather than refuses, on the same reasoning the script already applies to a re-homed logon account - that location may have been made to work by hand, and refusing outright would strand a deployment that runs today.
 - **The store's compressed plan format is now documented** ([#2171], reported by @argpna) - execution-plan XML has been gzip-compressed in `query_plan_dim.query_plan_gz` since 3.4.0, with `query_plan_xml` nullable, and the release notes never said so. A consumer reading the store directly over SQL therefore got nothing back for anything collected by a current build, with no clue why. The store section of the Darling README now states the format (gzip, magic `1f 8b`), that `query_plan_xml IS NULL` means read the compressed column rather than "no plan", and the three practical ways to get XML back - ask the product via `get_plan_xml`, decompress client-side, or ship a UDF into your own store if your tooling is SQL-only. In-product paths were never affected; this was a contract change for direct SQL consumers that shipped silently.
@@ -2676,3 +2678,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2138]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2138
 [#2190]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2190
 [#2187]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2187
+[#2189]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2189

Darling/Darling.Tests/AlertEngineTests.cs

@@ -1559,9 +1559,63 @@ the only value on hand is the state being alerted ON — so inserting would base
         Assert.DoesNotContain("ON CONFLICT", body, StringComparison.Ordinal);
     }
 
-    private static string ReadStateStoreSource([CallerFilePath] string thisFile = "")
+    [Fact]
+    public void DatabaseState_TheNeverBaselinedList_IsWiderThanTheCriticalOne()
+    {
+        /* #2189. Two lists that look interchangeable and are not, which is exactly why they are worth
+           pinning: one answers "bad enough to page about with no baseline to compare against", the other
+           "must never be LEARNED as this database's normal". A transient state belongs only in the second.
+
+           Collapsing them either way is a shipped bug. Widen the critical list and every restore in progress
+           pages. Narrow the never-baselined list and a database observed mid-restore learns RESTORING as
+           expected, then alerts forever for being ONLINE — the reported bug, 636 fires in 24 hours. */
+        Assert.Contains(DatabaseStateTokens.Suspect, DatabaseStateTokens.CriticalSqlList, StringComparison.Ordinal);
+        Assert.Contains(DatabaseStateTokens.RecoveryPending, DatabaseStateTokens.CriticalSqlList, StringComparison.Ordinal);
+        Assert.Contains(DatabaseStateTokens.Emergency, DatabaseStateTokens.CriticalSqlList, StringComparison.Ordinal);
+
+        /* A pending database in a transient state must stay SILENT, so these must not reach the critical arm. */
+        Assert.DoesNotContain(DatabaseStateTokens.Restoring, DatabaseStateTokens.CriticalSqlList, StringComparison.Ordinal);
+        Assert.DoesNotContain(DatabaseStateTokens.Recovering, DatabaseStateTokens.CriticalSqlList, StringComparison.Ordinal);
+
+        Assert.StartsWith(DatabaseStateTokens.CriticalSqlList, DatabaseStateTokens.NeverBaselinedSqlList, StringComparison.Ordinal);
+        Assert.Contains($"'{DatabaseStateTokens.Restoring}'", DatabaseStateTokens.NeverBaselinedSqlList, StringComparison.Ordinal);
+        Assert.Contains($"'{DatabaseStateTokens.Recovering}'", DatabaseStateTokens.NeverBaselinedSqlList, StringComparison.Ordinal);
+
+        /* STANDBY is synthetic and stable by construction — the whole reason it exists is to give a
+           log-shipping secondary one steady token instead of the RESTORING flicker underneath it. Refusing to
+           learn it would leave every standby secondary permanently pending for no benefit. */
+        Assert.DoesNotContain(DatabaseStateTokens.Standby, DatabaseStateTokens.NeverBaselinedSqlList, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void DatabaseState_BothDarlingSeedSites_ShareTheOneStateList()
+    {
+        /* The viewer's editor seeds and heals baselines with its own copy of this SQL because that project
+           cannot reference the service's. Two hand-kept copies of "what must never be learned" is the drift
+           that lets the editor write a baseline the alert refuses to — silently, and only for operators who
+           happen to open the editor mid-restore. Both sites interpolate the shared constant instead, and
+           BOTH statements use it: the seed to refuse those states, the heal to un-write them (#2189). A copy
+           that widened only one of the two would be the subtler half of the same bug. */
+        var viewer = ReadRepoFile(Path.Combine("Darling", "PerformanceMonitor.Darling.Viewer", "ViewerDataService.DatabaseStates.cs"));
+        var service = ReadRepoFile(Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingAlertReadAdapter.cs"));
+
+        foreach (var source in new[] { viewer, service })
+        {
+            Assert.Contains("NOT IN ({DatabaseStateTokens.NeverBaselinedSqlList})", source, StringComparison.Ordinal);
+            Assert.Contains("expected_state IN ({DatabaseStateTokens.NeverBaselinedSqlList})", source, StringComparison.Ordinal);
+            Assert.DoesNotContain($"NOT IN ('{DatabaseStateTokens.Suspect}'", source, StringComparison.Ordinal);
+
+            /* The heal must never be reachable for a state somebody DECLARED, in either copy. */
+            Assert.Contains("is_user_override = false", source, StringComparison.Ordinal);
+        }
+    }
+
+    private static string ReadStateStoreSource() =>
+        ReadRepoFile(Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "PgAlertStateStore.cs"));
+
+    /// <summary>Reads a repo-relative source file, walking up from this test file to find the repo root.</summary>
+    private static string ReadRepoFile(string relative, [CallerFilePath] string thisFile = "")
     {
-        var relative = Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "PgAlertStateStore.cs");
         var dir = Path.GetDirectoryName(thisFile)!;
         while (dir is not null && !File.Exists(Path.Combine(dir, relative)))
         {