diff --git a/CHANGELOG.md b/CHANGELOG.md index 49a284d4..9e520066 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Darling: the Availability Group store reads are now executed against a real Postgres in CI** ([#1697]) - the two AG reads added in [#1692] had no test that ran their SQL, and that was the one gap that mattered: the defect they were corrected for during review is invisible to a unit test. Both grains were briefly read as two statements over a SINGLE command to save a round trip, which PostgreSQL rejects - Npgsql only splits multi-statement text into a batch when it parses the SQL for NAMED placeholders, so with the positional (`$1`) parameters those reads use it sends one extended-protocol `Parse` and the server answers `cannot insert multiple commands into a prepared statement`. Every AG path is failure-isolated, so it would not have crashed anything; it would have logged one error per server per sweep with the whole alert family silently dead, which is the worst failure mode a monitoring product has - the thing that is broken is the thing that tells you something is broken. The new `DARLING_TEST_PG`-gated test seeds both collector tables and asserts what only real SQL can prove: that each query executes, that the newest-snapshot predicate excludes an older one, that a row with a NULL identity column is dropped rather than keyed under a placeholder, that NULL lag and suspend-reason columns round-trip, and that the path reaches the sweep entry point and fires exactly one alert off freshly seeded rows. + - **Lite + Darling: recovery notices were being styled and counted as live alerts in Alert History and the Daily Summary** ([#1692]) - `AlertMetricClassifier.IsResolution` is the shared source of truth for "this row is good news, not something to act on", and it recognized only the `Cleared` / `Resolved` / `Restored` suffixes. Darling's self-alert recoveries have been emitting `Collection Resumed`, `Agent Restarted` and `Compression Job Recovered` - written by the very same recovery path as the recognized `Capture Restored` - and every one of them landed in BOTH apps' Alert History grids styled as an actionable alert, and was counted as one in the Daily Summary's per-day alert totals. This is the same drift #1225 fixed one layer up, recurring one layer down. Widened the suffix set to `Resumed` / `Restarted` / `Recovered` / `Reconnected` (no actionable metric name in either app contains those words, so nothing real turns green), and widened both hand-maintained SQL copies of that list - Darling's `DailySummarySql` and Lite's `LocalDataService.DailySummary` - which is where the miscount came from. - **Darling: the MCP `get_alert_settings` tool under-reported the store by five columns** ([#1692]) - its SELECT stopped at 36 columns while `config_alert_settings` grew to 41, so an MCP client could not see the [#1674] connection opt-ins at all, and would not have seen the new Availability Group knobs either. Extended to the full 41. The parity test that pins the alert-settings column list against the upsert's placeholders, the bind order and the reader ordinals - the guard for the highest-risk defect class in that plumbing, since a mismatch only fails against a live Postgres - had drifted to 36 the same way; it now covers all 41 and drives its placeholder loop off the list length so the literal cannot drift again. - **Darling test suite: a genuinely intermittent failure, roughly one full-suite run in six** ([#1692]) - `ViewerTimeHelper` keeps the timestamp display mode and the active server's UTC offset in process-wide statics, and three test classes have to mutate them to exercise the production code that reads them. They share a `viewer-time-statics` collection and each restores the previous value in a `finally`, which makes them safe against each other - but the collection had **no `CollectionDefinition`**, so it grouped its members without constraining them and xUnit ran it in parallel with every other collection. Any viewer test that renders a timestamp could observe a swapped display mode mid-assertion, and the victim differed run to run (`ViewerSystemEventsTests`, then `ViewerWave3DisplayTests`) - precisely the shape that reads as "flaky test, just re-run it". Added the definition with `DisableParallelization`, constraining the three mutators rather than chasing an open-ended set of readers; verified with 12 consecutive full-suite runs. @@ -1661,5 +1663,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1680]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1680 [#1688]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1688 [#1692]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1692 +[#1697]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1697 [#1690]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1690 [#1693]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1693 diff --git a/Darling/Darling.Tests/DarlingSelfAlertTests.cs b/Darling/Darling.Tests/DarlingSelfAlertTests.cs index 7dbb2c94..2f87c0c6 100644 --- a/Darling/Darling.Tests/DarlingSelfAlertTests.cs +++ b/Darling/Darling.Tests/DarlingSelfAlertTests.cs @@ -1658,6 +1658,141 @@ the 45-minute-old success reads as stale against the seeded rows. */ } } + /// + /// EXECUTES both Availability Group reads against a real Postgres (#991). This exists because the two + /// grains were briefly read as two statements over a SINGLE command to save a round trip, and that is + /// rejected by PostgreSQL: Npgsql only splits multi-statement text into a batch when it parses the SQL for + /// NAMED placeholders, so with the positional ($1) parameters these reads use it sends one + /// extended-protocol Parse and the server answers "cannot insert multiple commands into a prepared + /// statement". Every AG path is failure-isolated, so that defect did not crash anything — it would have + /// logged one error per server per sweep with the whole alert family silently dead. No unit test can catch + /// that shape; only running the SQL can. Also pins the freshness signal, the newest-snapshot filter, and + /// the NULL-identity drop. + /// + [Fact] + public async Task LiveStoreReads_ExecuteBothAgQueries_AndReturnTheNewestSnapshot() + { + 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 AG store reads."); + + var ct = Ct; + using var connection = new NpgsqlConnection(connectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + await DeleteLiveAgRowsAsync(connection, ct); + + await using var postgres = NpgsqlDataSource.Create(connectionString!); + try + { + var utcNow = DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified); + var older = utcNow.AddMinutes(-5); + + /* An older snapshot that must be filtered out by the MAX(collection_time) predicate, a current one + that must come back, and a current row with a NULL identity column that must be dropped. */ + await InsertAgReplicaAsync(connection, ct, older, "AG1", "NODE1", "PRIMARY", "CONNECTED"); + await InsertAgReplicaAsync(connection, ct, utcNow, "AG1", "NODE1", "SECONDARY", "CONNECTED"); + await InsertAgReplicaAsync(connection, ct, utcNow, "AG1", "NODE2", "PRIMARY", "DISCONNECTED"); + await InsertAgReplicaAsync(connection, ct, utcNow, null, "NODE3", "SECONDARY", "CONNECTED"); + + var (replicaTime, replicas) = + await DarlingSelfAlertEvaluator.ReadLatestAgReplicaStatesAsync(postgres, LiveServerId, ct); + + Assert.NotNull(replicaTime); + Assert.Equal(2, replicas.Count); /* the un-keyable NULL row is dropped */ + Assert.DoesNotContain(replicas, r => r.RoleDesc == "PRIMARY" && r.ReplicaServerName == "NODE1"); + Assert.Contains(replicas, r => r.ReplicaServerName == "NODE2" && r.ConnectedStateDesc == "DISCONNECTED"); + + await InsertAgDatabaseAsync(connection, ct, older, "AG1", "Sales", "NODE2", 10, 10, false, null); + await InsertAgDatabaseAsync(connection, ct, utcNow, "AG1", "Sales", "NODE2", 900, 4096, false, null); + await InsertAgDatabaseAsync(connection, ct, utcNow, "AG1", "Orders", "NODE2", null, null, true, "SUSPEND_FROM_USER"); + + var (databaseTime, databases) = + await DarlingSelfAlertEvaluator.ReadLatestAgDatabaseReplicaStatesAsync(postgres, LiveServerId, ct); + + Assert.NotNull(databaseTime); + Assert.Equal(2, databases.Count); + var sales = Assert.Single(databases, d => d.DatabaseName == "Sales"); + Assert.Equal(900, sales.SecondaryLagSeconds); + Assert.Equal(4096, sales.RedoQueueSizeKb); + var orders = Assert.Single(databases, d => d.DatabaseName == "Orders"); + Assert.True(orders.IsSuspended); + Assert.Null(orders.SecondaryLagSeconds); + Assert.Equal("SUSPEND_FROM_USER", orders.SuspendReasonDesc); + + /* End to end through the sweep entry point, against rows that are genuinely fresh: the disconnect + and the suspend are first sightings (silent baselines), and the 900-second lag is past the + 300-second default, so exactly one alert lands. */ + var h = new Harness { Now = DateTime.UtcNow }; + var evaluator = h.Build(); + await evaluator.EvaluateStoreAlertsAsync(postgres, LiveServerId, Name, connected: true, ct); + + Assert.Contains(h.Deliverer.Outcomes, o => o.MetricName == "AG Sync Fell Behind"); + Assert.DoesNotContain(h.Deliverer.Outcomes, o => o.MetricName == "AG Replica Disconnected"); + } + finally + { + await DeleteLiveAgRowsAsync(connection, ct); + await DeleteLiveRowsAsync(connection, ct); + } + } + + private static async Task InsertAgReplicaAsync( + NpgsqlConnection connection, CancellationToken ct, DateTime time, + string? agName, string replicaServerName, string roleDesc, string connectedStateDesc) + { + using var command = new NpgsqlCommand(@" +INSERT INTO ag_replica_states (collection_id, collection_time, server_id, server_name, ag_name, + replica_server_name, role_desc, operational_state_desc, connected_state_desc, recovery_health_desc, + synchronization_health_desc, availability_mode_desc, failover_mode_desc, endpoint_url) +VALUES (0, $1, $2, $3, $4, $5, $6, 'ONLINE', $7, 'ONLINE', 'HEALTHY', 'SYNCHRONOUS_COMMIT', 'AUTOMATIC', NULL)", connection); + command.Parameters.AddWithValue(time); + command.Parameters.AddWithValue(LiveServerId); + command.Parameters.AddWithValue(Name); + command.Parameters.AddWithValue((object?)agName ?? DBNull.Value); + command.Parameters.AddWithValue(replicaServerName); + command.Parameters.AddWithValue(roleDesc); + command.Parameters.AddWithValue(connectedStateDesc); + await command.ExecuteNonQueryAsync(ct); + } + + private static async Task InsertAgDatabaseAsync( + NpgsqlConnection connection, CancellationToken ct, DateTime time, + string agName, string databaseName, string replicaServerName, + long? secondaryLagSeconds, long? redoQueueSize, bool isSuspended, string? suspendReason) + { + using var command = new NpgsqlCommand(@" +INSERT INTO ag_database_replica_states (collection_id, collection_time, server_id, server_name, ag_name, + database_name, replica_server_name, is_local, synchronization_state_desc, last_hardened_lsn, + last_commit_lsn, log_send_queue_size, redo_queue_size, log_send_rate, redo_rate, is_suspended, + suspend_reason_desc, availability_mode_desc, secondary_lag_seconds) +VALUES (0, $1, $2, $3, $4, $5, $6, FALSE, 'SYNCHRONIZING', NULL, NULL, 0, $7, 0, 0, $8, $9, + 'SYNCHRONOUS_COMMIT', $10)", connection); + command.Parameters.AddWithValue(time); + command.Parameters.AddWithValue(LiveServerId); + command.Parameters.AddWithValue(Name); + command.Parameters.AddWithValue(agName); + command.Parameters.AddWithValue(databaseName); + command.Parameters.AddWithValue(replicaServerName); + command.Parameters.AddWithValue((object?)redoQueueSize ?? DBNull.Value); + command.Parameters.AddWithValue(isSuspended); + command.Parameters.AddWithValue((object?)suspendReason ?? DBNull.Value); + command.Parameters.AddWithValue((object?)secondaryLagSeconds ?? DBNull.Value); + await command.ExecuteNonQueryAsync(ct); + } + + /// One command per statement, for the same reason the reads under test are: multi-statement text + /// is a trap worth not re-laying even in cleanup. + private static async Task DeleteLiveAgRowsAsync(NpgsqlConnection connection, CancellationToken ct) + { + var id = LiveServerId.ToString(CultureInfo.InvariantCulture); + foreach (var table in new[] { "ag_replica_states", "ag_database_replica_states" }) + { + using var cleanup = new NpgsqlCommand($"DELETE FROM {table} WHERE server_id = {id}", connection); + await cleanup.ExecuteNonQueryAsync(ct); + } + } + private static async Task InsertLogAsync( NpgsqlConnection connection, CancellationToken ct, long logId, string collector, DateTime time, string status) {