From 86981b934067b4ea1b0e3a8116a8f0d39a455cdc Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sun, 26 Jul 2026 14:39:42 -0400 Subject: [PATCH 1/2] Execute both Availability Group reads against a real Postgres (#991) The two AG store reads shipped in #1692 with no test that runs their SQL. That is the one gap that mattered, because the defect they were fixed for is invisible to a unit test: the grains were briefly read as two statements over a SINGLE command to save a round trip, and PostgreSQL rejects that. 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". Because every AG path is failure-isolated, that would not have crashed anything. It would have logged one error per server per sweep with the entire alert family silently dead - the worst failure mode a monitoring product has, since the thing that is broken is the thing that tells you something is broken. It was caught in review rather than by the suite. Added to the existing DARLING_TEST_PG-gated live section (skipped locally, run by CI's "Darling PostgreSQL tests" job), seeding both collector tables and asserting the things only real SQL can prove: that each query executes at all, that the MAX(collection_time) predicate keeps an older snapshot out, 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 whole thing reaches EvaluateStoreAlertsAsync and fires exactly one alert off freshly seeded rows. The cleanup helper issues one command per statement for the same reason. Darling suite green: 3250 passed, 156 skipped (the new test skips without a store; it is the +1 against the previous 155). Co-Authored-By: Claude Fable 5 --- .../Darling.Tests/DarlingSelfAlertTests.cs | 135 ++++++++++++++++++ 1 file changed, 135 insertions(+) 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) { From bff75d7d528eb85cdd60cd73e4d1c16774a94ce4 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sun, 26 Jul 2026 14:40:46 -0400 Subject: [PATCH 2/2] Add the CHANGELOG entry for #1697 Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) 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