From 3aff9b77f8d967d034be09ef883a01e7bdbba17d Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:06:10 -0400 Subject: [PATCH] Never learn a transient state as a database's expected state (#2189) The database-state alert baselines each database's first observed state as its accepted normal. A database swept into monitoring mid-restore therefore learned RESTORING as expected and deviated forever by being ONLINE: 636 alerts in 24 hours from 5 databases on the 52-server production fleet, with no escape but an operator re-baselining by hand. Two halves, both stores. The seed's refusal list widens from the integrity states to include the transient ones (RESTORING, RECOVERING) via a shared DatabaseStateTokens constant, so a database mid-operation stays pending and silent until it settles into a state worth learning. That governs rows that do not exist yet, so the second half applies the same rule after the fact: an AUTO-seeded baseline recording a state the seed would refuse is not a baseline anyone chose, and once the database's effective state reaches ONLINE the steady state is learned instead. Already-poisoned rows repair themselves on the next sweep with no migration, and "reset to current" pressed mid-restore -- which records whatever it sees with no filter, and always will -- now un-writes itself. What the heal does NOT touch is as load-bearing as what it does, since being wrong here means silence: - A user override. #2166's composition contract depends on it: a database parked at expected OFFLINE stays quiet while parked and still alerts when it comes back ONLINE. - An OFFLINE or STANDBY baseline, even though both are inferred. They are steady states, and leaving one is real news -- a STANDBY secondary that turns up truly ONLINE has stopped being a secondary, so log shipping is broken, and healing it would swap that alert for silence and then fire when the operator FIXED it. An auto-OFFLINE database brought up for an hour and re-parked would come back deviating forever. - The raw state_desc. A standby secondary reports state_desc = ONLINE with is_in_standby set, so matching that column would re-baseline every log-shipping secondary and alert it forever for being STANDBY. A NORECOVERY secondary, permanently RESTORING, stays silent as before; it now gets there by never being baselined rather than by baselining RESTORING. Tested watched-red across the transition matrix: onboard-during-restore then ONLINE, permanent RESTORING secondary, operator override wins, ONLINE to OFFLINE still fires, standby not healed, re-park stays quiet. The seed and heal are pinned live against real Postgres, and the two Darling copies of the SQL are pinned to share the one state list. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 3 + Darling/Darling.Tests/AlertEngineTests.cs | 58 ++++- .../DatabaseStateAlertMemoryLiveTests.cs | 195 +++++++++++++- .../DarlingAlertReadAdapter.cs | 87 ++++++- .../PgAlertStateStore.cs | 4 +- .../ViewerDataService.DatabaseStates.cs | 38 ++- Lite.Tests/DatabaseStateExpectedStoreTests.cs | 244 ++++++++++++++++++ .../LocalDataService.DatabaseStates.cs | 63 ++++- .../DatabaseStateTokens.cs | 33 +++ .../IAlertReadAdapter.cs | 17 +- 10 files changed, 701 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9ff31fbaf..3ecb819bec 100644 --- a/CHANGELOG.md +++ b/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 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. @@ -2672,3 +2674,4 @@ 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 diff --git a/Darling/Darling.Tests/AlertEngineTests.cs b/Darling/Darling.Tests/AlertEngineTests.cs index 62e9899bbc..a5b165887f 100644 --- a/Darling/Darling.Tests/AlertEngineTests.cs +++ b/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")); + + /// Reads a repo-relative source file, walking up from this test file to find the repo root. + 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))) { diff --git a/Darling/Darling.Tests/DatabaseStateAlertMemoryLiveTests.cs b/Darling/Darling.Tests/DatabaseStateAlertMemoryLiveTests.cs index 94725ed307..4ad271c43f 100644 --- a/Darling/Darling.Tests/DatabaseStateAlertMemoryLiveTests.cs +++ b/Darling/Darling.Tests/DatabaseStateAlertMemoryLiveTests.cs @@ -18,14 +18,18 @@ namespace Darling.Tests; /// -/// #2166 gated-live contract for , -/// the store-derived half of the database-state edge trigger. +/// Gated-live contracts for the maintenance statements the database-state alert runs against the store +/// beside its deviation read — the parts of this alert whose correctness lives in SQL rather than in the +/// engine: (#2166, the +/// store-derived half of the edge trigger), +/// and (#2189, what a baseline is +/// allowed to be learned from and what un-learns a stale one). /// -/// Why this has to be a LIVE test rather than a harness one: the bug it guards against was precisely -/// that the clear depended on the engine's in-memory active set, so a restart between an alert and the -/// recovery left last_alerted_state sticky and swallowed the next episode forever. Any test that -/// drives the engine can only prove the path a running process takes — the restart gap is invisible to it by -/// construction. This asks the store the same question the statement does, with nothing in memory at all. +/// Why these have to be LIVE rather than harness tests: each guards a bug that only a store can +/// exhibit. #2166's clear depended on the engine's in-memory active set, so a restart between an alert and +/// the recovery left last_alerted_state sticky forever — a test that drives the engine can only prove +/// the path a running process takes, and the restart gap is invisible to it by construction. #2189's pair +/// decide what rows EXIST, which no amount of engine stubbing observes. /// /// 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 — the house pattern. @@ -109,6 +113,160 @@ await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, async (cleanup } } + [Fact] + public async Task Seed_RefusesToLearnATransientState_SoAMidRestoreOnboardingStaysPending() + { + /* #2189: the seed's exclusion list is what decides a database's accepted normal FOREVER, and it used + to exclude only the integrity states. A database observed mid-restore therefore learned RESTORING + as expected and deviated by being healthy ever after. RESTORING and RECOVERING are databases in + the middle of an operation, not steady states, and are now refused the same way SUSPECT is. */ + 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 database-state seed pins."); + + 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 + { + var newest = new DateTime(2026, 08, 11, 10, 0, 0, DateTimeKind.Unspecified); + + await StateAsync(connection, ct, "MidRestore", "RESTORING", standby: false, at: newest); + await StateAsync(connection, ct, "ComingUp", "RECOVERING", standby: false, at: newest); + await StateAsync(connection, ct, "Corrupt", "SUSPECT", standby: false, at: newest); + await StateAsync(connection, ct, "Healthy", "ONLINE", standby: false, at: newest); + await StateAsync(connection, ct, "Parked", "OFFLINE", standby: false, at: newest); + /* A standby secondary's effective state is the synthetic STANDBY, which IS stable by construction + and so is exactly the kind of state worth learning — the raw RESTORING underneath it is not. */ + await StateAsync(connection, ct, "LogShipped", "RESTORING", standby: true, at: newest); + + using (var seed = new NpgsqlCommand(DarlingAlertReadAdapter.SeedDatabaseStateExpectedSql, connection)) + { + seed.Parameters.AddWithValue(LiveServerId); + await seed.ExecuteNonQueryAsync(ct); + } + + Assert.Null(await ExpectedStateAsync(connection, ct, "MidRestore")); + Assert.Null(await ExpectedStateAsync(connection, ct, "ComingUp")); + Assert.Null(await ExpectedStateAsync(connection, ct, "Corrupt")); + + Assert.Equal("ONLINE", await ExpectedStateAsync(connection, ct, "Healthy")); + Assert.Equal("OFFLINE", await ExpectedStateAsync(connection, ct, "Parked")); + Assert.Equal("STANDBY", await ExpectedStateAsync(connection, ct, "LogShipped")); + + bodySucceeded = true; + } + finally + { + await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, async (cleanup, cleanupCt) => + await DeleteLiveRowsAsync(cleanup, cleanupCt)); + } + } + + [Fact] + public async Task Heal_RelearnsOnline_OnlyForInferredBaselinesTheSeedWouldHaveRefused() + { + /* #2189's other half: the widened seed governs rows that do not exist yet, and cannot touch the ones + already written — five databases on the reporting fleet were baselined RESTORING and then alerted + ~127 times each in 24 hours for being ONLINE. The heal applies the seed's own rule after the fact: + a baseline recording a state the seed would REFUSE to learn is not a baseline anyone chose, so once + the database is demonstrably healthy the steady state is learned instead. + + Every row below the first is one that must NOT be healed. The failure mode on this side is silence, + and there are more ways to cause it than to fix it. */ + 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 database-state baseline heal."); + + 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 + { + var newest = new DateTime(2026, 08, 11, 11, 0, 0, DateTimeKind.Unspecified); + + /* The reported row: baselined mid-restore, restore finished, now permanently "deviating" by being + healthy. Healed — and its alerted-state memory goes with the baseline it described. */ + await StateAsync(connection, ct, "Poisoned", "ONLINE", standby: false, at: newest); + await ExpectedAsync(connection, ct, "Poisoned", expected: "RESTORING", lastAlerted: "ONLINE"); + + /* An operator's declaration. #2166's composition contract says a database parked at expected + OFFLINE stays quiet while parked and still alerts the moment it comes back ONLINE — which only + works if the heal leaves overrides alone. Rewriting this to ONLINE would silently delete the + operator's intent AND the alert they set it up to get. */ + await StateAsync(connection, ct, "Parked", "ONLINE", standby: false, at: newest); + await ExpectedAsync(connection, ct, "Parked", expected: "OFFLINE", lastAlerted: "", isOverride: true); + + /* The trap: a standby secondary reports state_desc = 'ONLINE' with is_in_standby set. Matching the + RAW column would re-baseline every log-shipping secondary from STANDBY to ONLINE and then alert + it forever for being STANDBY — #2189 recreated for the family #1986 works hardest to keep quiet. */ + await StateAsync(connection, ct, "LogShipped", "ONLINE", standby: true, at: newest); + await ExpectedAsync(connection, ct, "LogShipped", expected: "STANDBY", lastAlerted: ""); + + /* Not ONLINE yet: a database that moved from one un-settled state to another has not settled, so + its illegitimate baseline stays exactly as illegitimate as it was. */ + await StateAsync(connection, ct, "StillDown", "OFFLINE", standby: false, at: newest); + await ExpectedAsync(connection, ct, "StillDown", expected: "RESTORING", lastAlerted: ""); + + /* A STANDBY secondary that turns up TRULY online (is_in_standby now 0) has been recovered out of + standby: log shipping is broken, and that deviation is the alert's entire job. Healing it would + replace the alert with silence and then fire when the operator put standby BACK. STANDBY is a + steady state the seed learns on purpose, so it is not on the heal's list. */ + await StateAsync(connection, ct, "RecoveredSecondary", "ONLINE", standby: false, at: newest); + await ExpectedAsync(connection, ct, "RecoveredSecondary", expected: "STANDBY", lastAlerted: ""); + + /* Same reasoning for the other steady state. An auto-baselined OFFLINE database brought up for an + hour of maintenance must not have its baseline rewritten, or re-parking it leaves it deviating + forever against a baseline it never had - this bug, inverted, by the fix for it. */ + await StateAsync(connection, ct, "WasParked", "ONLINE", standby: false, at: newest); + await ExpectedAsync(connection, ct, "WasParked", expected: "OFFLINE", lastAlerted: ""); + + /* The sentinel. Not on the list either, so un-ignoring a database by accident cannot happen even + if some future path writes "(ignore)" without the override flag. */ + await StateAsync(connection, ct, "Silenced", "ONLINE", standby: false, at: newest); + await ExpectedAsync(connection, ct, "Silenced", expected: "(ignore)", lastAlerted: ""); + + using (var heal = new NpgsqlCommand(DarlingAlertReadAdapter.HealDatabaseStateBaselineToOnlineSql, connection)) + { + heal.Parameters.AddWithValue(LiveServerId); + Assert.Equal(1, await heal.ExecuteNonQueryAsync(ct)); + } + + Assert.Equal("ONLINE", await ExpectedStateAsync(connection, ct, "Poisoned")); + Assert.Null(await MemoryAsync(connection, ct, "Poisoned")); + + Assert.Equal("OFFLINE", await ExpectedStateAsync(connection, ct, "Parked")); + Assert.Equal("STANDBY", await ExpectedStateAsync(connection, ct, "LogShipped")); + Assert.Equal("RESTORING", await ExpectedStateAsync(connection, ct, "StillDown")); + Assert.Equal("STANDBY", await ExpectedStateAsync(connection, ct, "RecoveredSecondary")); + Assert.Equal("OFFLINE", await ExpectedStateAsync(connection, ct, "WasParked")); + Assert.Equal("(ignore)", await ExpectedStateAsync(connection, ct, "Silenced")); + + /* Idempotent, which matters because this runs on EVERY evaluation of every server: a store with + nothing left to heal must cost zero writes, not rewrite the same rows and churn updated_at. */ + using (var again = new NpgsqlCommand(DarlingAlertReadAdapter.HealDatabaseStateBaselineToOnlineSql, 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)); + } + } + private static async Task StateAsync( NpgsqlConnection connection, CancellationToken ct, string database, string stateDesc, bool standby, DateTime at) { @@ -124,19 +282,38 @@ INSERT INTO collect.database_states (collection_id, collection_time, server_id, await command.ExecuteNonQueryAsync(ct); } + /// + /// Plants an expectation row. is the load-bearing one for #2189: it is the + /// only thing separating a baseline the machine INFERRED (heal-able) from one an operator DECLARED + /// (never second-guessed), so a test that cannot set it cannot tell the two apart. + /// private static async Task ExpectedAsync( - NpgsqlConnection connection, CancellationToken ct, string database, string expected, string lastAlerted) + NpgsqlConnection connection, CancellationToken ct, string database, string expected, string lastAlerted, + bool isOverride = false) { using var command = new NpgsqlCommand(@" INSERT INTO config.database_state_expected (server_id, database_name, expected_state, is_user_override, updated_at, last_alerted_state, last_alerted_at) -VALUES ($1, $2, $3, false, (now() AT TIME ZONE 'UTC'), $4, (now() AT TIME ZONE 'UTC'))", connection); +VALUES ($1, $2, $3, $5, (now() AT TIME ZONE 'UTC'), $4, (now() AT TIME ZONE 'UTC'))", connection); command.Parameters.AddWithValue(LiveServerId); command.Parameters.AddWithValue(database); command.Parameters.AddWithValue(expected); command.Parameters.AddWithValue(lastAlerted); + command.Parameters.AddWithValue(isOverride); await command.ExecuteNonQueryAsync(ct); } + /// The stored expected state, or null when no row exists (a database still pending a baseline). + 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 MemoryAsync(NpgsqlConnection connection, CancellationToken ct, string database) { using var command = new NpgsqlCommand( diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingAlertReadAdapter.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingAlertReadAdapter.cs index a901d54ff5..d48c2f1668 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingAlertReadAdapter.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingAlertReadAdapter.cs @@ -666,23 +666,82 @@ public async Task GetAnomalousJobsAsync( /// /// Seeds a first-observation baseline for any database in the latest snapshot that has none - /// (insert-if-absent; never overwrites an existing baseline or user override). A CRITICAL first - /// observation (SUSPECT / RECOVERY_PENDING / EMERGENCY) is deliberately NOT baselined: onboarding a - /// server mid-outage must not learn the bad state as expected — such a database stays pending (no - /// row) and the deviation read alerts on it until it recovers or an operator sets an expected state. + /// (insert-if-absent; never overwrites an existing baseline or user override). A first observation in + /// an integrity or transient state is deliberately NOT baselined + /// (): onboarding a server mid-outage or + /// mid-restore must not learn that state as expected — such a database stays pending (no row) until it + /// settles into a steady state, and the deviation read alerts meanwhile only if the state is critical. /// config schema qualified explicitly; database_states resolves to collect through the search_path. /// $1 server_id. /// - public const string SeedDatabaseStateExpectedSql = @" + public const string SeedDatabaseStateExpectedSql = $@" 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') 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 ({DatabaseStateTokens.NeverBaselinedSqlList}) ON CONFLICT (server_id, database_name) DO NOTHING"; + /// + /// #2189: re-learns an ILLEGITIMATE inferred baseline as ONLINE once the database reaches ONLINE. The + /// rule is the seed's own, applied after the fact — an expectation recording a state the seed would + /// refuse to learn () is not a baseline at all, + /// it is a snapshot of a database mid-something, and the moment that database is demonstrably healthy + /// the honest move is to learn the steady state rather than page about the improvement forever. + /// + /// This is what heals the rows the old seed already poisoned, which the widened exclusion above + /// cannot: a database baselined RESTORING mid-restore deviated by being healthy forever, and the only + /// escape was an operator noticing and re-baselining by hand. It also covers the route that is still + /// open and always will be — "reset to current" pressed during a restore, or during an outage, records + /// whatever it sees with no state filter at all, and this un-writes it on the next sweep. + /// + /// Two gates, and both matter more than they look. + /// + /// is_user_override = false: an operator who declared an expected state MEANT it, and + /// #2166's composition contract depends on that — a database parked at expected OFFLINE stays silent + /// while parked and still alerts the moment it comes back ONLINE. Only the machine's own inference is + /// second-guessed, never the operator's. + /// + /// The state list, which is deliberately NOT "anything that is not ONLINE". OFFLINE and STANDBY + /// are steady states the seed is happy to learn, and leaving one is real news that must still fire. + /// A STANDBY secondary that turns up ONLINE has stopped being a secondary — somebody recovered it, log + /// shipping is broken, and healing it would replace that alert with silence and then fire the moment + /// the operator FIXED it. An auto-baselined OFFLINE database brought up for an hour and re-parked would + /// come back deviating forever against a baseline it never had. Both are the reported bug's own shape, + /// which is why the heal only ever touches states that were never a legitimate baseline. + /// + /// Reads the EFFECTIVE state, not state_desc — the same CASE as the seed and the deviation + /// read. That is load-bearing rather than cosmetic: a standby log-shipping secondary reports + /// state_desc = 'ONLINE' with is_in_standby set, so matching on the raw column would + /// re-baseline every such secondary to ONLINE and then alert it forever for being STANDBY — the very + /// bug being fixed, re-created for the one database family #1986 went out of its way to keep quiet. + /// + /// The alerted-state memory is dropped with the baseline it described (#2166). A memory saying + /// "the operator was told about ONLINE" only meant anything against the stale expectation; carried + /// past it, it would judge the next episode against an announcement about a baseline that no longer + /// exists. Clearing is the safe direction — it can cost an extra alert, never a missed one. $1 + /// server_id. + /// + public const string HealDatabaseStateBaselineToOnlineSql = $@" +UPDATE config.database_state_expected e +SET expected_state = 'ONLINE', + updated_at = (now() AT TIME ZONE 'UTC'), + last_alerted_state = NULL, + last_alerted_at = NULL +WHERE e.server_id = $1 +AND e.is_user_override = false +AND e.expected_state IN ({DatabaseStateTokens.NeverBaselinedSqlList}) +AND EXISTS ( + SELECT 1 + 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.database_name = e.database_name + AND (CASE WHEN ds.is_in_standby THEN 'STANDBY' ELSE ds.state_desc END) = 'ONLINE' +)"; + /// /// Tidies auto-baselines for databases no longer in the newest snapshot (dropped/renamed); user /// overrides are kept. $1 server_id. @@ -743,7 +802,7 @@ FROM database_states ds /// sentinel; each row carries current + expected (expected is empty for a pending row). Lite's DuckDB /// read ported to Postgres. $1 server_id. /// - public const string DatabaseStateDeviationsSql = @" + public const string DatabaseStateDeviationsSql = $@" WITH newest AS ( SELECT MAX(collection_time) AS t FROM database_states WHERE server_id = $1 ), @@ -769,8 +828,8 @@ LEFT JOIN config.database_state_expected e ON e.server_id = $1 AND e.database_name = l.database_name WHERE (e.expected_state IS NULL - AND l.eff IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY') - AND p.eff IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY')) + AND l.eff IN ({DatabaseStateTokens.CriticalSqlList}) + AND p.eff IN ({DatabaseStateTokens.CriticalSqlList})) OR (e.expected_state IS NOT NULL AND e.expected_state <> '(ignore)' AND l.eff IS DISTINCT FROM e.expected_state AND p.eff IS DISTINCT FROM e.expected_state) @@ -790,6 +849,16 @@ public async Task> GetDatabaseStatesAsync( await seed.ExecuteNonQueryAsync(cancellationToken); } + /* Beside the seed because it is the same job from the other end (#2189): the seed learns a baseline + for a database that has none, this un-learns one the database has since outgrown. Both run before + the read, so a poisoned expectation is corrected on the cycle that notices it rather than firing + once more first. */ + using (var heal = new NpgsqlCommand(HealDatabaseStateBaselineToOnlineSql, connection)) + { + heal.Parameters.AddWithValue(serverId); + await heal.ExecuteNonQueryAsync(cancellationToken); + } + using (var prune = new NpgsqlCommand(PruneDatabaseStateExpectedSql, connection)) { prune.Parameters.AddWithValue(serverId); diff --git a/Darling/PerformanceMonitor.Darling.Service/PgAlertStateStore.cs b/Darling/PerformanceMonitor.Darling.Service/PgAlertStateStore.cs index 4ec1a2a30c..ddb479d379 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PgAlertStateStore.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PgAlertStateStore.cs @@ -171,8 +171,8 @@ ON CONFLICT (server_id, metric_name) DO UPDATE SET /// the only value available is the state being alerted ON — so a database first observed SUSPECT would /// get SUSPECT written as its accepted baseline. It would then stop deviating, drop out of the deviation /// query, be read as RECOVERED (firing a false "resolved" on a still-corrupt database), and never alert - /// again. The seed logic refuses to baseline SUSPECT/RECOVERY_PENDING/EMERGENCY for exactly this reason; - /// this write must not do behind its back what it declines to do in front. + /// again. The seed logic refuses to baseline any of + /// for exactly this reason; this write must not do behind its back what it declines to do in front. /// /// Nothing is lost by skipping the no-row case: a database with no baseline row was first observed /// in an integrity state, and those states are never edge-suppressed (RepeatsAreNoise is false for them), diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.DatabaseStates.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.DatabaseStates.cs index 7d9e47ee70..ac0b9edeb1 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.DatabaseStates.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.DatabaseStates.cs @@ -27,18 +27,44 @@ 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. */ - private const string DatabaseStateSeedSql = @" + state_desc), and an integrity or transient effective state is NOT baselined so it stays pending — + otherwise opening the editor mid-outage would baseline SUSPECT and silence the alert, or opening it + mid-restore would baseline RESTORING and invert it (#2189). The state list is shared with the service + rather than spelled twice: this project cannot reference the service's, and two hand-kept copies of + "what must never be learned" is the drift that would let the editor seed what the alert refuses to. */ + 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') 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 ({DatabaseStateTokens.NeverBaselinedSqlList}) ON CONFLICT (server_id, database_name) DO NOTHING"; + /* The service adapter's #2189 heal, run for the same reason the seed is: the editor must not SHOW a + stale auto-baseline the alert has already stopped honouring, and a viewer seat pointed at a store + whose service is down would otherwise display — and keep — an expectation the database outgrew. + Auto-baselines only, and only ones recording a state the seed would refuse to learn: a user override + is the operator's declaration, and OFFLINE/STANDBY are steady states whose departure is real news. */ + private const string DatabaseStateHealToOnlineSql = $@" +UPDATE config.database_state_expected e +SET expected_state = 'ONLINE', + updated_at = (now() AT TIME ZONE 'UTC'), + last_alerted_state = NULL, + last_alerted_at = NULL +WHERE e.server_id = $1 +AND e.is_user_override = false +AND e.expected_state IN ({DatabaseStateTokens.NeverBaselinedSqlList}) +AND EXISTS ( + SELECT 1 + 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.database_name = e.database_name + AND (CASE WHEN ds.is_in_standby THEN 'STANDBY' ELSE ds.state_desc END) = 'ONLINE' +)"; + 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 @@ -80,6 +106,10 @@ public async Task> GetDatabaseStateExpectationsAs await using var seed = _dataSource.CreateCommand(DatabaseStateSeedSql); seed.Parameters.Add(new NpgsqlParameter { TypedValue = serverId }); await seed.ExecuteNonQueryAsync(cancellationToken); + + await using var heal = _dataSource.CreateCommand(DatabaseStateHealToOnlineSql); + heal.Parameters.Add(new NpgsqlParameter { TypedValue = serverId }); + await heal.ExecuteNonQueryAsync(cancellationToken); } var rows = new List(); diff --git a/Lite.Tests/DatabaseStateExpectedStoreTests.cs b/Lite.Tests/DatabaseStateExpectedStoreTests.cs index 79ddb699d0..15a6ea363c 100644 --- a/Lite.Tests/DatabaseStateExpectedStoreTests.cs +++ b/Lite.Tests/DatabaseStateExpectedStoreTests.cs @@ -79,6 +79,25 @@ INSERT INTO database_states (collection_id, collection_time, server_id, server_n private static readonly DateTime T0 = new(2026, 8, 1, 9, 0, 0, DateTimeKind.Unspecified); + /// + /// Writes an expectation row directly, bypassing the service's writers — the only way to reproduce a + /// row the OLD seed wrote (#2189), since no current code path can produce one any more. + /// + private async Task SeedExpectedAsync(string database, string expected, bool isOverride) + { + 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) +SELECT $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 = expected }); + cmd.Parameters.Add(new DuckDBParameter { Value = isOverride }); + await cmd.ExecuteNonQueryAsync(); + } + [Fact] public async Task FirstObservation_SeedsBaseline_AndReportsNoDeviation() { @@ -219,6 +238,231 @@ public async Task PendingCritical_AutoAcceptsBaseline_WhenItRecoversToNonCritica Assert.False(row.IsUserOverride); } + /* ---------------- #2189: transient states are never learned, and ONLINE un-learns a stale one ---------------- */ + + [Fact] + public async Task OnboardedMidRestore_NeverLearnsRestoring_AndIsSilentBeforeAndAfterTheRestoreCompletes() + { + /* #2189, the reported bug, end to end. A database swept into monitoring during a consolidation is + mid-restore, not in a steady state anybody chose — so nothing is learned while it restores, and + when the restore finishes the STEADY state is what gets learned. The old seed learned RESTORING + here and the database then "deviated" by being healthy, forever. */ + var service = new LocalDataService(_duckDb); + + await SeedSnapshotAsync(T0, ("App", "RESTORING", false)); + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + await SeedSnapshotAsync(T0.AddMinutes(1), ("App", "RESTORING", false)); + await SeedSnapshotAsync(T0.AddMinutes(2), ("App", "RESTORING", false)); + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); // a restore in progress is not news + + var pending = Assert.Single(await service.GetDatabaseStateExpectationsAsync(ServerId)); + Assert.Equal("", pending.ExpectedState); + + await SeedSnapshotAsync(T0.AddMinutes(3), ("App", "ONLINE", false)); + await SeedSnapshotAsync(T0.AddMinutes(4), ("App", "ONLINE", false)); + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + + var settled = Assert.Single(await service.GetDatabaseStateExpectationsAsync(ServerId)); + Assert.Equal("ONLINE", settled.ExpectedState); + Assert.False(settled.IsUserOverride); + } + + [Fact] + public async Task TransientStates_AreNeverLearnedAsABaseline_RestoringAndRecovering() + { + /* Both halves of "mid-something": RESTORING is a restore in flight, RECOVERING a database still + coming up. Neither is a steady state, and the states around them are unaffected — a healthy + database still baselines on its first observation. */ + await SeedSnapshotAsync(T0, + ("Restoring", "RESTORING", false), ("Recovering", "RECOVERING", false), ("Healthy", "ONLINE", false)); + var service = new LocalDataService(_duckDb); + await service.GetDatabaseStateDeviationsAsync(ServerId); + + var rows = await service.GetDatabaseStateExpectationsAsync(ServerId); + Assert.Equal("", rows.Single(r => r.DatabaseName == "Restoring").ExpectedState); + Assert.Equal("", rows.Single(r => r.DatabaseName == "Recovering").ExpectedState); + Assert.Equal("ONLINE", rows.Single(r => r.DatabaseName == "Healthy").ExpectedState); + } + + [Fact] + public async Task PoisonedRestoringBaseline_HealsToOnline_RatherThanAlertingForeverOnBeingHealthy() + { + /* The other half of #2189, and the half the widened seed cannot reach: rows that were ALREADY + written. Five databases on the reporting fleet sat like this — baselined RESTORING during a + consolidation, then ~127 identical alerts each in 24 hours for the crime of being ONLINE. No + current code path can write this row any more, so it is planted directly. */ + await SeedExpectedAsync("pecan", "RESTORING", isOverride: false); + await SeedSnapshotAsync(T0, ("pecan", "ONLINE", false)); + await SeedSnapshotAsync(T0.AddMinutes(1), ("pecan", "ONLINE", false)); + var service = new LocalDataService(_duckDb); + + 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 RebaselinedByHandMidRestore_StillHealsOnceTheRestoreCompletes() + { + /* The seed is not the only way into a transient baseline: "reset to current" pressed while a restore + is running writes exactly the same poisoned row, and always will. The heal is what makes that a + self-correcting mistake instead of a trap armed for the next operator. */ + await SeedSnapshotAsync(T0, ("App", "RESTORING", false)); + var service = new LocalDataService(_duckDb); + await service.ResetDatabaseStateExpectedToCurrentAsync(ServerId, "App"); + Assert.Equal("RESTORING", (await service.GetDatabaseStateExpectationsAsync(ServerId)).Single().ExpectedState); + + await SeedSnapshotAsync(T0.AddMinutes(1), ("App", "ONLINE", false)); + await SeedSnapshotAsync(T0.AddMinutes(2), ("App", "ONLINE", false)); + + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + Assert.Equal("ONLINE", (await service.GetDatabaseStateExpectationsAsync(ServerId)).Single().ExpectedState); + } + + [Fact] + public async Task RebaselinedByHandDuringAnOutage_HealsOnceTheDatabaseRecovers() + { + /* The seed is not the only writer of inferred baselines, and never will be: "reset to current" + records whatever it sees with NO state filter, so pressing it during an outage writes SUSPECT as + the accepted normal. That silences the database while it is corrupt (the operator's own doing) and + then, without this, makes it deviate by RECOVERING. Same shape as the restore case, integrity + flavour — which is why the heal keys off the seed's refusal list rather than RESTORING alone. */ + await SeedSnapshotAsync(T0, ("Payments", "SUSPECT", false)); + var service = new LocalDataService(_duckDb); + await service.ResetDatabaseStateExpectedToCurrentAsync(ServerId, "Payments"); + var planted = Assert.Single(await service.GetDatabaseStateExpectationsAsync(ServerId)); + Assert.Equal("SUSPECT", planted.ExpectedState); + Assert.False(planted.IsUserOverride); + + await SeedSnapshotAsync(T0.AddMinutes(1), ("Payments", "ONLINE", false)); + await SeedSnapshotAsync(T0.AddMinutes(2), ("Payments", "ONLINE", false)); + + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + Assert.Equal("ONLINE", (await service.GetDatabaseStateExpectationsAsync(ServerId)).Single().ExpectedState); + } + + [Fact] + public async Task AutoBaselinedOffline_IsNeverHealed_SoParkingItAgainStaysQuiet() + { + /* The heal is deliberately NOT "ONLINE overwrites anything the machine inferred". OFFLINE is a steady + state the seed is happy to learn, so it is a legitimate baseline, and rewriting it on the first + ONLINE sighting would be this bug inverted: bring a parked database up for an hour of maintenance, + re-park it, and it now deviates forever against a baseline it never had - which in Lite, with no + persisted alerted-state memory to edge-trigger against, means an alert every cooldown for good. + + Coming UP still alerts, because that is a real departure from the accepted normal. Going back to it + is silence, and the baseline is the same one it started with. */ + await SeedSnapshotAsync(T0, ("Parked", "OFFLINE", false)); + var service = new LocalDataService(_duckDb); + await service.GetDatabaseStateDeviationsAsync(ServerId); // learns OFFLINE + + await SeedSnapshotAsync(T0.AddMinutes(1), ("Parked", "ONLINE", false)); + await SeedSnapshotAsync(T0.AddMinutes(2), ("Parked", "ONLINE", false)); + var up = Assert.Single(await service.GetDatabaseStateDeviationsAsync(ServerId)); + Assert.Equal("ONLINE", up.StateDesc); + Assert.Equal("OFFLINE", up.ExpectedState); + + await SeedSnapshotAsync(T0.AddMinutes(3), ("Parked", "OFFLINE", false)); + await SeedSnapshotAsync(T0.AddMinutes(4), ("Parked", "OFFLINE", false)); + + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + var row = Assert.Single(await service.GetDatabaseStateExpectationsAsync(ServerId)); + Assert.Equal("OFFLINE", row.ExpectedState); + Assert.False(row.IsUserOverride); + } + + [Fact] + public async Task StandbySecondaryRecoveredOutOfStandby_StillAlerts_BecauseItHasStoppedBeingASecondary() + { + /* The other state the heal must not touch, and the sharper of the two. A STANDBY secondary that turns + up truly ONLINE (is_in_standby now 0) has been RECOVERED - log shipping is broken and that is + exactly what this alert exists to say. Healing it would swap that alert for silence and then fire + when the operator re-established standby, announcing the repair instead of the break. */ + await SeedSnapshotAsync(T0, ("LogShip", "ONLINE", true)); + var service = new LocalDataService(_duckDb); + await service.GetDatabaseStateDeviationsAsync(ServerId); // learns STANDBY + + await SeedSnapshotAsync(T0.AddMinutes(1), ("LogShip", "ONLINE", false)); + await SeedSnapshotAsync(T0.AddMinutes(2), ("LogShip", "ONLINE", false)); + + var fired = Assert.Single(await service.GetDatabaseStateDeviationsAsync(ServerId)); + Assert.Equal("ONLINE", fired.StateDesc); + Assert.Equal("STANDBY", fired.ExpectedState); + Assert.Equal("STANDBY", (await service.GetDatabaseStateExpectationsAsync(ServerId)).Single().ExpectedState); + } + + [Fact] + public async Task OperatorParkedOffline_StillAlertsWhenTheDatabaseComesBackOnline() + { + /* #2166's composition contract, which the heal must not eat. The heal second-guesses only what the + machine inferred; an operator who DECLARED an expected state meant it, so a parked database coming + back ONLINE is a deviation from a real intent and still fires — and the override survives the sweep + rather than being quietly rewritten to ONLINE underneath them. */ + await SeedSnapshotAsync(T0, ("Parked", "OFFLINE", false)); + var service = new LocalDataService(_duckDb); + await service.SetDatabaseStateExpectedAsync(ServerId, "Parked", PerformanceMonitor.Alerting.DatabaseStateTokens.Offline); + + await SeedSnapshotAsync(T0.AddMinutes(1), ("Parked", "ONLINE", false)); + await SeedSnapshotAsync(T0.AddMinutes(2), ("Parked", "ONLINE", false)); + + var fired = Assert.Single(await service.GetDatabaseStateDeviationsAsync(ServerId)); + Assert.Equal("ONLINE", fired.StateDesc); + Assert.Equal("OFFLINE", fired.ExpectedState); + + var row = Assert.Single(await service.GetDatabaseStateExpectationsAsync(ServerId)); + Assert.True(row.IsUserOverride); + Assert.Equal("OFFLINE", row.ExpectedState); + } + + [Fact] + public async Task NorecoveryLogShippingSecondary_StaysQuietForever_AndCanBeOptedIntoCoverageByHand() + { + /* The #1986 property that must survive #2189: a log-shipping secondary restored WITH NORECOVERY sits + in RESTORING permanently (is_in_standby is 0 — only a read-only STANDBY secondary sets that), and + must never page. It now gets there by staying PENDING rather than by learning RESTORING, which is + silent for the same reason: the no-baseline arm only alerts on the integrity states. + + The cost is that a pending database has no baseline to deviate FROM, so the operator who wants + deviation coverage on a permanent secondary declares it — and that being an override is the point, + since it is a genuine choice about a database only they can classify. */ + var service = new LocalDataService(_duckDb); + for (int minute = 0; minute < 6; minute++) + { + await SeedSnapshotAsync(T0.AddMinutes(minute), ("Secondary", "RESTORING", false)); + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + } + + Assert.Equal("", (await service.GetDatabaseStateExpectationsAsync(ServerId)).Single().ExpectedState); + + await service.SetDatabaseStateExpectedAsync(ServerId, "Secondary", PerformanceMonitor.Alerting.DatabaseStateTokens.Restoring); + await SeedSnapshotAsync(T0.AddMinutes(6), ("Secondary", "OFFLINE", false)); + await SeedSnapshotAsync(T0.AddMinutes(7), ("Secondary", "OFFLINE", false)); + + var fired = Assert.Single(await service.GetDatabaseStateDeviationsAsync(ServerId)); + Assert.Equal("OFFLINE", fired.StateDesc); + Assert.Equal("RESTORING", fired.ExpectedState); + } + + [Fact] + public async Task StandbySecondary_IsNeverHealedToOnline_BecauseItsEffectiveStateIsStandby() + { + /* The trap inside the heal. A standby secondary reports state_desc = 'ONLINE' with is_in_standby set, + so a heal written against the RAW column would re-baseline every log-shipping secondary from + STANDBY to ONLINE and then alert it forever for being STANDBY — #2189 recreated for exactly the + database family #1986 works hardest to keep quiet. Matching the EFFECTIVE state is what prevents it. */ + await SeedSnapshotAsync(T0, ("LogShip", "ONLINE", true)); + var service = new LocalDataService(_duckDb); + await service.GetDatabaseStateDeviationsAsync(ServerId); // baselines STANDBY + + await SeedSnapshotAsync(T0.AddMinutes(1), ("LogShip", "ONLINE", true)); + await SeedSnapshotAsync(T0.AddMinutes(2), ("LogShip", "RESTORING", true)); + + Assert.Empty(await service.GetDatabaseStateDeviationsAsync(ServerId)); + Assert.Equal("STANDBY", (await service.GetDatabaseStateExpectationsAsync(ServerId)).Single().ExpectedState); + } + [Fact] public async Task StandbySecondary_BaselinesAsStandby_AndDoesNotChurnOnLogRestores() { diff --git a/Lite/Services/LocalDataService.DatabaseStates.cs b/Lite/Services/LocalDataService.DatabaseStates.cs index 3b57c3eb7c..9266f636f3 100644 --- a/Lite/Services/LocalDataService.DatabaseStates.cs +++ b/Lite/Services/LocalDataService.DatabaseStates.cs @@ -26,18 +26,23 @@ public partial class LocalDataService /// database-state alert. Fires only when the deviation is present in the TWO most recent collections /// (so a restart's RECOVERY_PENDING / RECOVERING transients — and a standby secondary's per-restore /// RESTORING flicker — don't page unless the condition actually sticks). First AUTO-SEEDS a baseline - /// (the effective state) for any database in the newest snapshot that has none — EXCEPT a critical - /// effective state (SUSPECT / RECOVERY_PENDING / EMERGENCY), which is left pending so onboarding a - /// server mid-outage doesn't learn the bad state as expected. Also tidies auto-baselines for databases - /// that have dropped off the newest snapshot (user overrides are preserved). The base table always - /// holds the newest snapshots (archival only moves older rows to parquet), so it is queried directly. + /// (the effective state) for any database in the newest snapshot that has none — EXCEPT an integrity or + /// transient effective state (), which is left + /// pending so onboarding a server mid-outage or mid-restore doesn't learn that state as expected. Then + /// HEALS an auto-baseline that nonetheless records one of those states — written by an older build, or + /// by re-baselining a database by hand while it was mid-something — to ONLINE once the database reaches + /// ONLINE, so it stops deviating by being healthy (#2189); a user override, and an OFFLINE or STANDBY + /// baseline, are never touched. Also tidies auto-baselines for databases that have dropped off the + /// newest snapshot (user overrides are preserved). The base table always holds the newest snapshots + /// (archival only moves older rows to parquet), so it is queried directly. /// public async Task> GetDatabaseStateDeviationsAsync(int serverId) { using var connection = await OpenConnectionAsync(); - /* 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). */ + /* Seed missing baselines from the latest snapshot (insert-if-absent; effective state). An integrity + or transient state is never learned: a critical first observation stays pending and alerts via the + no-baseline arm below, and a transient one stays pending SILENTLY until it settles. */ using (var seed = connection.CreateCommand()) { seed.CommandText = $@" @@ -47,7 +52,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 ({DatabaseStateTokens.NeverBaselinedSqlList}) AND NOT EXISTS ( SELECT 1 FROM config_database_state_expected e WHERE e.server_id = $1 AND e.database_name = ds.database_name @@ -56,6 +61,44 @@ SELECT 1 FROM config_database_state_expected e await seed.ExecuteNonQueryAsync(); } + /* #2189: re-learn an ILLEGITIMATE inferred baseline as ONLINE once the database reaches ONLINE — the + seed's own rule applied after the fact. An expectation recording a state the seed would refuse to + learn is not a baseline, it is a snapshot of a database mid-something, and left alone it inverts + the alert permanently. The widened seed above cannot fix that on its own because it only governs + rows that do not exist yet; this heals the ones already written, by the old seed or by "reset to + current" pressed during a restore or an outage (that path records whatever it sees, no filter). + + Two gates. is_user_override = false: an operator who declared an expected state meant it, and a + database parked at expected OFFLINE must still alert when it comes back ONLINE. And the state list + is NOT "anything that is not ONLINE" — OFFLINE and STANDBY are steady states worth learning, and + leaving one is real news: a STANDBY secondary that turns up ONLINE has stopped being a secondary + (somebody recovered it, log shipping is broken), and an auto-OFFLINE database brought up for an + hour and re-parked would come back deviating forever. Both are this bug's own shape. + + The EFFECTIVE state is what is matched, never state_desc — a standby secondary reports + state_desc = 'ONLINE' with is_in_standby set, so matching the raw column would re-baseline every + log-shipping secondary to ONLINE and then alert it forever for being STANDBY. Uncorrelated + IN (...), like the prune below. */ + using (var heal = connection.CreateCommand()) + { + heal.CommandText = $@" +UPDATE config_database_state_expected +SET expected_state = 'ONLINE', + updated_at = now()::TIMESTAMP +WHERE server_id = $1 +AND is_user_override = false +AND expected_state IN ({DatabaseStateTokens.NeverBaselinedSqlList}) +AND database_name IN ( + SELECT ds.database_name + 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 {EffectiveStateSql} = 'ONLINE' +)"; + heal.Parameters.Add(new DuckDBParameter { Value = serverId }); + await heal.ExecuteNonQueryAsync(); + } + /* Tidy auto-baselines for databases no longer in the newest snapshot (dropped/renamed). User overrides are kept — an operator's intent shouldn't vanish because a database is briefly gone. */ using (var prune = connection.CreateCommand()) @@ -100,8 +143,8 @@ LEFT JOIN config_database_state_expected e ON e.server_id = $1 AND e.database_name = l.database_name WHERE (e.expected_state IS NULL - AND l.eff IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY') - AND p.eff IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY')) + AND l.eff IN ({DatabaseStateTokens.CriticalSqlList}) + AND p.eff IN ({DatabaseStateTokens.CriticalSqlList})) OR (e.expected_state IS NOT NULL AND e.expected_state <> '(ignore)' AND l.eff IS DISTINCT FROM e.expected_state AND p.eff IS DISTINCT FROM e.expected_state) diff --git a/PerformanceMonitor.Alerting/DatabaseStateTokens.cs b/PerformanceMonitor.Alerting/DatabaseStateTokens.cs index 55ac03062a..a6736b5282 100644 --- a/PerformanceMonitor.Alerting/DatabaseStateTokens.cs +++ b/PerformanceMonitor.Alerting/DatabaseStateTokens.cs @@ -48,6 +48,39 @@ public static class DatabaseStateTokens /// The metric name every database-state alert fires under (the AlertSeverity map key). public const string MetricName = "Database State"; + /// + /// The integrity-failure states as a SQL IN-list literal, for the arm of both stores' deviation reads + /// that alerts a database with no baseline yet. Composed from the constants above so the + /// one-spelling-each rule this class exists for reaches the SQL too, where a typo would not fail to + /// compile — it would silently never match. + /// + /// This list is deliberately NARROWER than and must stay that + /// way. It answers "which states are bad enough to page about with no baseline to compare against", + /// where the other answers "which states must never be LEARNED as normal". A transient state belongs + /// only in the second: a database sitting in RESTORING has nothing wrong with it, so widening this list + /// to match would page for every restore in progress. + /// + public const string CriticalSqlList = "'" + Suspect + "', '" + RecoveryPending + "', '" + Emergency + "'"; + + /// + /// The effective states a first observation must never be baselined from, as a SQL IN-list literal — + /// the integrity states plus the transient operational ones (#2189). A database in one of these is + /// mid-something, not in a steady state anybody would choose as "expected", and learning one inverts + /// the alert permanently: a database observed mid-restore learned RESTORING as its baseline and then + /// deviated by being HEALTHY, forever (636 alerts in 24 hours from 5 databases on one fleet). + /// + /// A database in one of these states simply stays pending — no row — until it settles into + /// something the seed will learn. That keeps a NORECOVERY log-shipping secondary permanently quiet + /// (pending plus a non-critical state alerts nothing) while still covering it for the integrity states + /// through the pending arm; an operator who wants deviation coverage for one sets the expected state + /// explicitly, which is an override and therefore honoured over anything inferred. + /// + /// Note STANDBY is absent on purpose. It is synthetic and stable by construction — the whole + /// reason exists is to give a log-shipping secondary one steady token instead of + /// the RESTORING flicker underneath it — so it is exactly the kind of state worth learning. + /// + public const string NeverBaselinedSqlList = CriticalSqlList + ", '" + Restoring + "', '" + Recovering + "'"; + /// /// The fixed severity for a given state_desc: CRITICAL for the integrity-failure states /// (SUSPECT / RECOVERY_PENDING / EMERGENCY), WARNING for everything else. Case-insensitive. diff --git a/PerformanceMonitor.Alerting/IAlertReadAdapter.cs b/PerformanceMonitor.Alerting/IAlertReadAdapter.cs index 0f3d7afda9..2e4b754ef5 100644 --- a/PerformanceMonitor.Alerting/IAlertReadAdapter.cs +++ b/PerformanceMonitor.Alerting/IAlertReadAdapter.cs @@ -156,12 +156,19 @@ Task GetAnomalousJobsAsync( /// returns the rows where current != expected in both samples and expected is not the /// sentinel, each carrying both the current and expected state. /// - /// CONTRACT — the read also AUTO-SEEDS and PRUNES: any database in the latest snapshot with no + /// CONTRACT — the read also AUTO-SEEDS, HEALS and PRUNES: any database in the latest snapshot with no /// expected-state row yet gets its current effective state recorded as the first-observation baseline, - /// EXCEPT a critical effective state (SUSPECT / RECOVERY_PENDING / EMERGENCY), which is left pending so - /// it alerts rather than learning the bad state as expected. Auto-baselines for databases that have - /// dropped off the newest snapshot are pruned (user overrides preserved). Seeding is idempotent - /// (insert-if-absent) and never overwrites a user override or an existing baseline. + /// EXCEPT the states in — the integrity ones + /// (which stay pending so they alert rather than learning the bad state as expected) and the transient + /// ones (RESTORING / RECOVERING, which stay pending SILENTLY until the database settles, so onboarding + /// mid-restore cannot learn a state nobody chose). An AUTO-seeded baseline that nonetheless records one + /// of those states — written by an older build, or by re-baselining a database by hand while it was + /// mid-something — is HEALED to ONLINE once the database's effective state reaches ONLINE, since such a + /// row is not a baseline anyone chose and would otherwise make the database deviate by being healthy + /// (#2189). The heal never touches a user override, and never touches an OFFLINE or STANDBY baseline: + /// those are steady states, and departing one is a real deviation that must still fire. Auto-baselines + /// for databases that have dropped off the newest snapshot are pruned (user overrides preserved). + /// Seeding is idempotent (insert-if-absent) and never overwrites a user override or an existing baseline. /// /// /// Empty when the store has no snapshot for this server. Unlike the anomalous-jobs read this is