Stop learning a mid-restore state as a database's expected one (#2189)

CHANGELOG.md

@@ -31,6 +31,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - **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.
+- **A database observed mid-restore no longer alerts forever for being healthy** ([#2189]) - the baseline seeder refused to learn SUSPECT, RECOVERY_PENDING and EMERGENCY as a database's expected state, but not RESTORING or RECOVERING, which are just as transient. A database swept while a restore was in progress therefore had RESTORING recorded as its ACCEPTED state, and every healthy observation afterwards counted as a deviation - permanently, with no way out but an operator noticing and re-baselining by hand. Found by dogfooding: the 52-server production monitor fired **636 Database State alerts in 24 hours from 5 databases**, every one reading "Expected: RESTORING, Current: ONLINE", all of them consolidation restores caught at the wrong moment. Both transient states are now excluded from seeding, so such a database stays pending until it settles and the seeder learns the state it actually settles into. Baselines already poisoned are repaired automatically rather than needing a migration or a manual fix: a maintenance statement running beside the existing seed and prune drops any auto-baseline naming a transient state, and because it runs BEFORE the seed the correct steady state is re-learned in the same cycle. Operator overrides are never touched - somebody who deliberately expects a database to sit in RESTORING means it - and STANDBY is deliberately left alone, since a log-shipping secondary lives there permanently and only resembles the transient states.
+
 - **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.
 - **tempdb no longer reports more than 100% used in FinOps Database Sizes** ([#2169], reported by @CatastropheOps) - the used percentage divided in-database usage by the size recorded in `sys.master_files`, which is the size set at configuration time and does not track autogrowth for tempdb. A tempdb that had grown was therefore measured against its startup size and rendered above 100%. The per-database probe now captures the file's current size in the same round trip it already makes for space-used, and the payload prefers it, so both halves of the ratio come from one snapshot; a database whose probe fails still falls back to the old source rather than vanishing from the grid. Affects the on-prem, RDS, and Managed Instance path - the Azure SQL Database path already read both numbers in-database.
 - **Backing out of Custom Range no longer strands an open calendar - both apps** ([#2154], reported in #2153) - a DatePicker's calendar dropdown is a popup living outside the visual tree's visibility, so collapsing the pickers when the user switched back to a preset range left an already-open calendar floating on screen; the dropdowns now close explicitly alongside the collapse, in Lite's ServerTab and the Darling Viewer's twin alike.
@@ -2673,4 +2675,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2166]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2166
 [#2167]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2167
 [#2138]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2138
+[#2189]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2189
 [#2190]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2190

Darling/Darling.Tests/TransientBaselineRepairLiveTests.cs

@@ -0,0 +1,152 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System;
+using System.Globalization;
+using System.Threading;
+using System.Threading.Tasks;
+using Npgsql;
+using PerformanceMonitor.Darling.Service;
+using PerformanceMonitor.Darling.Storage;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #2189 gated-live contract for <see cref="DarlingAlertReadAdapter.RepairTransientDatabaseStateBaselineSql"/>.
+///
+/// <para>The bug: the seed excluded only SUSPECT / RECOVERY_PENDING / EMERGENCY, so a database observed
+/// mid-restore had RESTORING recorded as its ACCEPTED baseline and then deviated permanently by being
+/// healthy. Measured on the production fleet: 636 alerts in 24 hours from 5 databases, every one reading
+/// "Expected: RESTORING, Current: ONLINE". Excluding those states from the seed stops new ones; it cannot
+/// repair rows already written, because the seed is insert-if-absent by design.</para>
+///
+/// <para>Live rather than harness-level because the whole statement IS the behaviour — there is no C# to
+/// exercise. Runs against a real Postgres gated on <c>DARLING_TEST_PG</c>, on the serialized
+/// "live-postgres" collection, against a negative sentinel server_id, cleaning up in finally.</para>
+/// </summary>
+[Collection("live-postgres")]
+public sealed class TransientBaselineRepairLiveTests
+{
+    private const int LiveServerId = -915759;
+
+    [Fact]
+    public async Task Repair_DropsGuessedTransientBaselines_KeepsSteadyStatesAndOperatorIntent()
+    {
+        var connectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG");
+        Assert.SkipWhen(string.IsNullOrEmpty(connectionString),
+            "Set DARLING_TEST_PG to a Postgres connection string to run the live transient-baseline repair.");
+
+        var ct = TestContext.Current.CancellationToken;
+        using var connection = new NpgsqlConnection(connectionString);
+        await connection.OpenAsync(ct);
+        await PgMigrations.MigrateAsync(connection, ct);
+        await DeleteLiveRowsAsync(connection, ct);
+
+        var bodySucceeded = false;
+        try
+        {
+            /* The bug's signature: a guessed baseline naming a state nobody would choose as "expected". */
+            await ExpectedAsync(connection, ct, "MidRestore", "RESTORING", userOverride: false);
+            await ExpectedAsync(connection, ct, "MidRecovery", "RECOVERING", userOverride: false);
+            await ExpectedAsync(connection, ct, "FirstSeenSuspect", "SUSPECT", userOverride: false);
+
+            /* Steady states a baseline SHOULD hold. STANDBY is the one worth pinning explicitly: a
+               log-shipping secondary sits there permanently, so it is a legitimate expectation and must not
+               be swept along with the transient states it superficially resembles. */
+            await ExpectedAsync(connection, ct, "Healthy", "ONLINE", userOverride: false);
+            await ExpectedAsync(connection, ct, "LogShipped", "STANDBY", userOverride: false);
+
+            /* Operator intent, even when it names a transient state: somebody who deliberately expects a
+               database to sit in RESTORING means it, and their row must survive. */
+            await ExpectedAsync(connection, ct, "DeliberatelyRestoring", "RESTORING", userOverride: true);
+
+            using (var repair = new NpgsqlCommand(DarlingAlertReadAdapter.RepairTransientDatabaseStateBaselineSql, connection))
+            {
+                repair.Parameters.AddWithValue(LiveServerId);
+                Assert.Equal(3, await repair.ExecuteNonQueryAsync(ct));
+            }
+
+            Assert.Null(await ExpectedStateAsync(connection, ct, "MidRestore"));
+            Assert.Null(await ExpectedStateAsync(connection, ct, "MidRecovery"));
+            Assert.Null(await ExpectedStateAsync(connection, ct, "FirstSeenSuspect"));
+
+            Assert.Equal("ONLINE", await ExpectedStateAsync(connection, ct, "Healthy"));
+            Assert.Equal("STANDBY", await ExpectedStateAsync(connection, ct, "LogShipped"));
+            Assert.Equal("RESTORING", await ExpectedStateAsync(connection, ct, "DeliberatelyRestoring"));
+
+            /* Idempotent: it runs on every evaluation of every server, so a second pass over an already
+               repaired store must change nothing. */
+            using (var again = new NpgsqlCommand(DarlingAlertReadAdapter.RepairTransientDatabaseStateBaselineSql, connection))
+            {
+                again.Parameters.AddWithValue(LiveServerId);
+                Assert.Equal(0, await again.ExecuteNonQueryAsync(ct));
+            }
+
+            bodySucceeded = true;
+        }
+        finally
+        {
+            await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, async (cleanup, cleanupCt) =>
+                await DeleteLiveRowsAsync(cleanup, cleanupCt));
+        }
+    }
+
+    [Fact]
+    public void Seed_ExcludesEveryStateTheRepairDeletes()
+    {
+        /* The two statements have to agree, or the pair oscillates: the seed writes a baseline the repair
+           deletes on the next evaluation, forever, churning the table and re-announcing the database. This
+           pins that the seed's exclusion list covers everything the repair treats as unfit. */
+        foreach (var state in new[] { "SUSPECT", "RECOVERY_PENDING", "EMERGENCY", "RESTORING", "RECOVERING" })
+        {
+            Assert.Contains($"'{state}'", DarlingAlertReadAdapter.RepairTransientDatabaseStateBaselineSql, StringComparison.Ordinal);
+            Assert.Contains($"'{state}'", DarlingAlertReadAdapter.SeedDatabaseStateExpectedSql, StringComparison.Ordinal);
+        }
+
+        /* And the inverse: a steady state must appear in NEITHER list. STANDBY is the trap. */
+        Assert.DoesNotContain("'STANDBY'", DarlingAlertReadAdapter.RepairTransientDatabaseStateBaselineSql, StringComparison.Ordinal);
+        Assert.DoesNotContain("'ONLINE'", DarlingAlertReadAdapter.RepairTransientDatabaseStateBaselineSql, StringComparison.Ordinal);
+
+        /* Operator intent is the repair's only exemption, so its absence would be silent data loss. */
+        Assert.Contains("is_user_override = false", DarlingAlertReadAdapter.RepairTransientDatabaseStateBaselineSql, StringComparison.Ordinal);
+    }
+
+    private static async Task ExpectedAsync(
+        NpgsqlConnection connection, CancellationToken ct, string database, string expected, bool userOverride)
+    {
+        using var command = new NpgsqlCommand(@"
+INSERT INTO config.database_state_expected (server_id, database_name, expected_state, is_user_override, updated_at)
+VALUES ($1, $2, $3, $4, (now() AT TIME ZONE 'UTC'))", connection);
+        command.Parameters.AddWithValue(LiveServerId);
+        command.Parameters.AddWithValue(database);
+        command.Parameters.AddWithValue(expected);
+        command.Parameters.AddWithValue(userOverride);
+        await command.ExecuteNonQueryAsync(ct);
+    }
+
+    private static async Task<string?> ExpectedStateAsync(
+        NpgsqlConnection connection, CancellationToken ct, string database)
+    {
+        using var command = new NpgsqlCommand(
+            "SELECT expected_state FROM config.database_state_expected WHERE server_id = $1 AND database_name = $2",
+            connection);
+        command.Parameters.AddWithValue(LiveServerId);
+        command.Parameters.AddWithValue(database);
+        var value = await command.ExecuteScalarAsync(ct);
+        return value is DBNull or null ? null : (string)value;
+    }
+
+    private static async Task DeleteLiveRowsAsync(NpgsqlConnection connection, CancellationToken ct)
+    {
+        using var cleanup = new NpgsqlCommand(
+            $"DELETE FROM config.database_state_expected WHERE server_id = {LiveServerId.ToString(CultureInfo.InvariantCulture)};",
+            connection);
+        await cleanup.ExecuteNonQueryAsync(ct);
+    }
+}

Darling/PerformanceMonitor.Darling.Service/DarlingAlertReadAdapter.cs

@@ -680,9 +680,37 @@ FROM database_states ds
 WHERE ds.server_id = $1
 AND   ds.collection_time = (SELECT MAX(collection_time) FROM database_states WHERE server_id = $1)
 AND   ds.state_desc IS NOT NULL
-AND   (CASE WHEN ds.is_in_standby THEN 'STANDBY' ELSE ds.state_desc END) NOT IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY')
+AND   (CASE WHEN ds.is_in_standby THEN 'STANDBY' ELSE ds.state_desc END) NOT IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY', 'RESTORING', 'RECOVERING')
 ON CONFLICT (server_id, database_name) DO NOTHING";
 
+    /// <summary>
+    /// #2189: removes an auto-baseline that recorded a TRANSIENT state as expected, so the seed above can
+    /// re-learn the database's real steady state on its next pass.
+    ///
+    /// <para>The seed used to exclude only SUSPECT / RECOVERY_PENDING / EMERGENCY, so a database observed
+    /// mid-restore had RESTORING written as its accepted baseline — and then deviated permanently by being
+    /// healthy. Measured on the production fleet: 636 alerts in 24 hours from 5 databases, every one reading
+    /// "Expected: RESTORING, Current: ONLINE", all of them consolidation restores that had been observed at
+    /// the wrong moment. The seed fix stops new ones; this repairs the rows already written, which the seed
+    /// cannot do because it is insert-if-absent by design.</para>
+    ///
+    /// <para>Runs beside the seed and prune every evaluation rather than as a migration rung: it is
+    /// self-healing (a baseline poisoned by any future path gets corrected on the next cycle), it needs no
+    /// schema change, and it is idempotent — once no auto-baseline names a transient state it deletes
+    /// nothing. Deleting rather than updating is deliberate: the correct steady state is not knowable here
+    /// (the database may still be restoring), and an absent baseline is exactly the "pending" condition the
+    /// seed already handles correctly on the next observation.</para>
+    ///
+    /// <para><c>is_user_override = false</c> is load-bearing. An operator who deliberately sets a database's
+    /// expected state to RESTORING — a permanently log-shipped target, say — means it, and their intent must
+    /// survive. Only baselines this code guessed are eligible. $1 server_id.</para>
+    /// </summary>
+    public const string RepairTransientDatabaseStateBaselineSql = @"
+DELETE FROM config.database_state_expected
+WHERE server_id = $1
+AND   is_user_override = false
+AND   expected_state IN ('RESTORING', 'RECOVERING', 'SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY')";
+
     /// <summary>
     /// Tidies auto-baselines for databases no longer in the newest snapshot (dropped/renamed); user
     /// overrides are kept. $1 server_id.
@@ -784,6 +812,16 @@ public async Task<List<DatabaseStateInfo>> GetDatabaseStatesAsync(
         var items = new List<DatabaseStateInfo>();
         await using var connection = await _postgres.OpenConnectionAsync(cancellationToken);
 
+        /* BEFORE the seed, deliberately (#2189): dropping a transient-state baseline first lets the seed
+           re-learn the real steady state in the SAME cycle. Repairing afterwards would leave the database
+           with no baseline until the next pass, and the seed is insert-if-absent so it can never correct a
+           row itself. */
+        using (var repair = new NpgsqlCommand(RepairTransientDatabaseStateBaselineSql, connection))
+        {
+            repair.Parameters.AddWithValue(serverId);
+            await repair.ExecuteNonQueryAsync(cancellationToken);
+        }
+
         using (var seed = new NpgsqlCommand(SeedDatabaseStateExpectedSql, connection))
         {
             seed.Parameters.AddWithValue(serverId);