From 66f6b47b0edab335e0385a3a7f6f0d4a77da3b92 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:24:08 +0200 Subject: [PATCH 1/2] Stop learning a mid-restore state as a database's expected one (#2189) The baseline seeder refused to learn SUSPECT, RECOVERY_PENDING and EMERGENCY, but not RESTORING or RECOVERING, which are equally transient. A database swept while a restore was running therefore had RESTORING recorded as its ACCEPTED state, and every healthy observation afterwards counted as a deviation - permanently, with no exit but an operator noticing and re-baselining by hand. Found by dogfooding rather than reasoning: 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 observed at the wrong moment. The seed guard's own doc says onboarding mid-outage must not learn the bad state as expected; a restore in progress is that same situation and the list just did not include it. Both transient states are now excluded from seeding, so the database stays pending until it settles and the seeder learns what it settles into. Rows already poisoned are repaired automatically, because the seed cannot do it - it is insert-if-absent by design, so it will never correct an existing row. A maintenance statement runs beside the existing seed and prune and drops any auto-baseline naming a transient state. Deliberately NOT a migration rung: this way it is self-healing for any future path that poisons a baseline, needs no schema change, and is idempotent. Ordering is load-bearing: the repair runs BEFORE the seed, so the correct steady state is re-learned in the same cycle. Repairing afterwards would leave the database with no baseline until the next pass. Two things it must not touch, both pinned: is_user_override rows, because an operator who deliberately expects RESTORING means it, and STANDBY, because a log-shipping secondary lives there permanently and only superficially resembles the transient states. Lite gets the identical change - same exclusion list, same repair before its seed - since both SKUs seed baselines the same way and a fix in one would leave the other generating the same flood. Tested live rather than in the harness, because the statement IS the behaviour: seeds six baselines covering guessed-transient, steady, STANDBY and operator-override, asserts exactly three rows deleted and which three survive, and asserts a second pass deletes zero since this runs on every evaluation of every server. A second test pins that the seed excludes every state the repair deletes - if those lists ever disagree the pair oscillates, writing and deleting the same row forever. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 2 + .../TransientBaselineRepairLiveTests.cs | 152 ++++++++++++++++++ .../DarlingAlertReadAdapter.cs | 40 ++++- .../LocalDataService.DatabaseStates.cs | 23 ++- 4 files changed, 214 insertions(+), 3 deletions(-) create mode 100644 Darling/Darling.Tests/TransientBaselineRepairLiveTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 479de2335c..fd557142fe 100644 --- a/CHANGELOG.md +++ b/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 `` 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 `` 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. diff --git a/Darling/Darling.Tests/TransientBaselineRepairLiveTests.cs b/Darling/Darling.Tests/TransientBaselineRepairLiveTests.cs new file mode 100644 index 0000000000..69599dd51a --- /dev/null +++ b/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; + +/// +/// #2189 gated-live contract for . +/// +/// 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. +/// +/// 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 DARLING_TEST_PG, on the serialized +/// "live-postgres" collection, against a negative sentinel server_id, cleaning up in finally. +/// +[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 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); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingAlertReadAdapter.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingAlertReadAdapter.cs index a901d54ff5..b76cdf20c8 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingAlertReadAdapter.cs +++ b/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"; + /// + /// #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. + /// + /// 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. + /// + /// 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. + /// + /// is_user_override = false 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. + /// + 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')"; + /// /// 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> GetDatabaseStatesAsync( var items = new List(); 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); diff --git a/Lite/Services/LocalDataService.DatabaseStates.cs b/Lite/Services/LocalDataService.DatabaseStates.cs index 3b57c3eb7c..2bbfae480a 100644 --- a/Lite/Services/LocalDataService.DatabaseStates.cs +++ b/Lite/Services/LocalDataService.DatabaseStates.cs @@ -36,8 +36,27 @@ public async Task> GetDatabaseStateDeviationsAsync(int s { using var connection = await OpenConnectionAsync(); + /* #2189, BEFORE the seed: drop any auto-baseline that recorded a TRANSIENT state, so the seed below + re-learns the real steady state in the SAME cycle. A database observed mid-restore used to get + RESTORING written as its expected state and then deviated forever by being healthy — 636 alerts in + 24 hours from 5 databases on the production fleet. The seed is insert-if-absent, so it can never + correct a row itself; this is what makes the fix self-healing for baselines already written. + is_user_override = false is load-bearing: an operator who deliberately expects RESTORING (a + permanently log-shipped target) means it. */ + using (var repair = connection.CreateCommand()) + { + repair.CommandText = @" +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')"; + repair.Parameters.Add(new DuckDBParameter { Value = serverId }); + await repair.ExecuteNonQueryAsync(); + } + /* Seed missing baselines from the latest snapshot (insert-if-absent; effective state; non-critical - only — a critical first observation stays pending and alerts via the no-baseline arm below). */ + and non-transient only — a critical or mid-operation first observation stays pending, and the + critical ones alert via the no-baseline arm below). */ using (var seed = connection.CreateCommand()) { seed.CommandText = $@" @@ -47,7 +66,7 @@ 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 {EffectiveStateSql} NOT IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY') +AND {EffectiveStateSql} NOT IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY', 'RESTORING', 'RECOVERING') AND NOT EXISTS ( SELECT 1 FROM config_database_state_expected e WHERE e.server_id = $1 AND e.database_name = ds.database_name From eb5955fee304329f5f3862b833f8cd6e6cce1e82 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:12:57 +0200 Subject: [PATCH 2/2] Close the #2189 fix in the Viewer too, plus changelog link and Lite tests Three review findings, and the first was a real hole rather than polish. The Viewer holds a THIRD copy of the seed rule (ViewerDataService.DatabaseStates.cs), and I had updated only the service adapter and Lite. Its own doc says it mirrors the service's seed "so the editor and the alert agree on what 'expected' means" - so the previous commit broke that invariant. It matters because the editor is a WRITE path: opening the database-state override screen while a database was mid-restore would baseline RESTORING again and reproduce the exact alert storm this PR fixes, just via the editor instead of the sweep. It also displayed a poisoned baseline it was capable of repairing, to the one person most likely to assume it had been repaired. Same exclusion list there now, and the repair runs before its seed for the same reason it does in the service. Grepped for other copies while I was in there: these three were all of them. The CHANGELOG entry referenced [#2189] with no link definition - the reference block jumped #2138 to #2190, so it would have rendered unresolved. Added. Lite had no test for either behaviour. Three added to DatabaseStateExpectedStoreTests, which runs against a real in-process DuckDB with no live gate: a mid-restore first observation stays pending and then baselines what it settles into; an already-poisoned guessed baseline is repaired and re-seeded in ONE call (the ordering claim, not just the outcome); and an operator override on a transient state survives the repair. The third needed a direct-insert helper, because SetDatabaseStateExpectedAsync always stamps is_user_override = true and so cannot express "the product guessed this" - which is the exact distinction the repair keys on. Lite.Tests is net10.0-windows and does not run on this machine, so those three are CI-verified only; the Darling live test and every affected project build clean locally. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + .../ViewerDataService.DatabaseStates.cs | 27 ++++++- Lite.Tests/DatabaseStateExpectedStoreTests.cs | 81 +++++++++++++++++++ 3 files changed, 106 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd557142fe..6b56b89796 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2675,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 diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.DatabaseStates.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.DatabaseStates.cs index 7d9e47ee70..444b8cd079 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.DatabaseStates.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.DatabaseStates.cs @@ -27,8 +27,13 @@ namespace PerformanceMonitor.Darling.Viewer; public sealed partial class ViewerDataService { /* Matches the service alert read's seed: effective state (STANDBY for a log-shipping secondary, else - state_desc), and a critical effective state is NOT baselined so it stays pending — otherwise opening - the editor mid-outage would baseline SUSPECT and silence the alert. */ + state_desc), and neither a critical NOR a transient effective state is baselined so it stays pending — + otherwise opening the editor mid-outage would baseline SUSPECT and silence the alert, and opening it + mid-restore would baseline RESTORING and make the database alert forever once it recovered (#2189). + + This list must stay identical to DarlingAlertReadAdapter's. It is a third copy of the same rule (the + service, Lite, and here), and the editor is a WRITE path — a seed that disagrees with the alert's does + not merely display something different, it poisons the baseline the alert then reads. */ private const string DatabaseStateSeedSql = @" INSERT INTO config.database_state_expected (server_id, database_name, expected_state, is_user_override, updated_at) SELECT $1, ds.database_name, CASE WHEN ds.is_in_standby THEN 'STANDBY' ELSE ds.state_desc END, false, (now() AT TIME ZONE 'UTC') @@ -36,9 +41,19 @@ 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"; + /* #2189, run BEFORE the seed for the same reason the service does it: drop an auto-baseline that recorded + a transient state so the seed re-learns the real one in the same call. Without this the editor would + keep DISPLAYING a poisoned "Expected: RESTORING" it is capable of fixing, and an operator looking at + the override screen is exactly the person who would reasonably assume it had been fixed. */ + private const string DatabaseStateRepairTransientSql = @" +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')"; + private const string DatabaseStateExpectationsSql = @" WITH latest AS ( SELECT ds.database_name, CASE WHEN ds.is_in_standby THEN 'STANDBY' ELSE ds.state_desc END AS eff @@ -77,6 +92,12 @@ public async Task> GetDatabaseStateExpectationsAs { if (!IsReadOnly) { + await using (var repair = _dataSource.CreateCommand(DatabaseStateRepairTransientSql)) + { + repair.Parameters.Add(new NpgsqlParameter { TypedValue = serverId }); + await repair.ExecuteNonQueryAsync(cancellationToken); + } + await using var seed = _dataSource.CreateCommand(DatabaseStateSeedSql); seed.Parameters.Add(new NpgsqlParameter { TypedValue = serverId }); await seed.ExecuteNonQueryAsync(cancellationToken); diff --git a/Lite.Tests/DatabaseStateExpectedStoreTests.cs b/Lite.Tests/DatabaseStateExpectedStoreTests.cs index 79ddb699d0..2c6a8377a7 100644 --- a/Lite.Tests/DatabaseStateExpectedStoreTests.cs +++ b/Lite.Tests/DatabaseStateExpectedStoreTests.cs @@ -77,6 +77,26 @@ INSERT INTO database_states (collection_id, collection_time, server_id, server_n } } + /// + /// Writes a baseline row DIRECTLY, which is the only way to reproduce a row the OLD seeder guessed + /// (#2189). SetDatabaseStateExpectedAsync always stamps is_user_override = true, so it + /// cannot express "the product guessed this" — and that distinction is the whole thing the repair keys on. + /// + private async Task SetExpectedAsync(string database, string expectedState, bool userOverride) + { + using var readLock = _duckDb.AcquireReadLock(); + var connection = await SeedConnectionAsync(); + using var cmd = connection.CreateCommand(); + cmd.CommandText = @" +INSERT INTO config_database_state_expected (server_id, database_name, expected_state, is_user_override, updated_at) +VALUES ($1, $2, $3, $4, now()::TIMESTAMP)"; + cmd.Parameters.Add(new DuckDBParameter { Value = ServerId }); + cmd.Parameters.Add(new DuckDBParameter { Value = database }); + cmd.Parameters.Add(new DuckDBParameter { Value = expectedState }); + cmd.Parameters.Add(new DuckDBParameter { Value = userOverride }); + await cmd.ExecuteNonQueryAsync(); + } + private static readonly DateTime T0 = new(2026, 8, 1, 9, 0, 0, DateTimeKind.Unspecified); [Fact] @@ -219,6 +239,67 @@ public async Task PendingCritical_AutoAcceptsBaseline_WhenItRecoversToNonCritica Assert.False(row.IsUserOverride); } + [Fact] + public async Task MidRestoreFirstObservation_IsNotBaselined_ThenBaselinesWhatItSettlesInto() + { + // #2189: RESTORING is as transient as RECOVERY_PENDING, and baselining it inverts the alert forever — + // the database then "deviates" by being healthy. Observed on the production fleet as 636 alerts in 24 + // hours from 5 databases reading "Expected: RESTORING, Current: ONLINE". + await SeedSnapshotAsync(T0, ("Restoring", "RESTORING", false)); + var service = new LocalDataService(_duckDb); + + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + Assert.Equal("", (await service.GetDatabaseStateExpectationsAsync(ServerId)) + .Single(r => r.DatabaseName == "Restoring").ExpectedState); // pending, NOT baselined RESTORING + + // Restore finishes. Now there is a real steady state to learn. + await SeedSnapshotAsync(T0.AddMinutes(1), ("Restoring", "ONLINE", false)); + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + + var row = Assert.Single(await service.GetDatabaseStateExpectationsAsync(ServerId)); + Assert.Equal("ONLINE", row.ExpectedState); + Assert.False(row.IsUserOverride); + } + + [Fact] + public async Task AlreadyPoisonedRestoringBaseline_IsRepairedAndReseeded_InOneCall() + { + // The seed is insert-if-absent, so it can never correct a row written before the #2189 fix. The repair + // statement runs BEFORE the seed precisely so a poisoned baseline is dropped and the real steady state + // re-learned in the SAME call, rather than leaving the database with no baseline for a cycle. + await SeedSnapshotAsync(T0, ("Restored", "ONLINE", false)); + var service = new LocalDataService(_duckDb); + + // Write the poisoned baseline the way the old seeder would have: guessed, not an operator override. + await SetExpectedAsync("Restored", "RESTORING", userOverride: false); + Assert.Equal("RESTORING", (await service.GetDatabaseStateExpectationsAsync(ServerId)) + .Single().ExpectedState); + + // Reading deviations runs repair-then-seed. Healthy database, so it must end up ONLINE and silent. + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + + var row = Assert.Single(await service.GetDatabaseStateExpectationsAsync(ServerId)); + Assert.Equal("ONLINE", row.ExpectedState); + Assert.False(row.IsUserOverride); + } + + [Fact] + public async Task OperatorOverrideOnATransientState_SurvivesTheRepair() + { + // Somebody who deliberately sets a database's expectation to RESTORING — a permanently log-shipped + // target, say — means it. The repair is scoped to guessed baselines only, and this is the assertion + // that keeps it that way. + await SeedSnapshotAsync(T0, ("Deliberate", "RESTORING", false)); + var service = new LocalDataService(_duckDb); + await SetExpectedAsync("Deliberate", "RESTORING", userOverride: true); + + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + + var row = Assert.Single(await service.GetDatabaseStateExpectationsAsync(ServerId)); + Assert.Equal("RESTORING", row.ExpectedState); + Assert.True(row.IsUserOverride); + } + [Fact] public async Task StandbySecondary_BaselinesAsStandby_AndDoesNotChurnOnLogRestores() {