Availability Group alerts: failover, replica disconnect, sync fell behind, database suspended (#991) - #1692
Merged
Merged
Conversation
Four store-polled self-alerts over the AG collectors' latest snapshot, evaluated on the existing per-server self-alert sweep: AG Failover (Warning) role_desc changed since the previous sweep AG Replica Disconnected (Critical) connected_state_desc crossed DISCONNECTED AG Replica Reconnected (resolved) the recovery notice for the above AG Sync Fell Behind (Warning) secondary_lag_seconds or redo_queue_size AG Database Suspended (Warning) is_suspended false -> true, with the reason State machines live in DarlingSelfAlertEvaluator behind injectable Func seams, keyed per AG grain (ag+replica, ag+database+replica) rather than per server, so two lagging databases on one host track and recover independently. Every alert still fires under the real server_id as its serverKey, so per-server delivery overrides, mute rules and history correlation are unaffected. First sighting of any replica or database is a silent baseline (the ConnectionAlertPolicy discipline), and a NULL state string is skipped rather than read as a transition - WSFC quorum loss nulls the AG catalog views wholesale, and treating that as an edge would spray alerts across the fleet at the worst possible moment. JudgeAgSync returns three states, not a bool, and that is the load-bearing design decision. secondary_lag_seconds reads 0 - not NULL - while data movement is SUSPENDED, so the seconds trigger has to abstain on a suspended row. If abstaining were a plain "not behind", the caller would read it as recovery: a lagging database that got SUSPENDED, or whose columns went NULL under quorum loss, would have emitted "AG Sync Recovered - has caught up with the primary" in the same sweep that reported it suspended. Only a database this sweep MEASURED as caught up resolves a standing alert. That also makes cross-server resolution structurally impossible rather than something a prefix check has to catch. Store-backed settings (V35 on config.config_alert_settings, all NOT NULL DEFAULT): notify_ag_health (true), ag_lag_alert_seconds (300, clamp 0-86400), ag_redo_queue_alert_kb (0 = off, clamp 0-1073741824). Wired through DarlingConfig, DarlingAlertSettings, StoreConfigProvider, ViewerDataService and the viewer Settings window on the #1674 pattern; read live, so an edit takes effect on the next sweep with no restart. Each grain is read by its own command and gated on its OWN snapshot time. The two were briefly read as two statements over one command to save a round trip; that cannot work. Npgsql only splits multi-statement text into a batch when it parses the SQL for NAMED placeholders - with the POSITIONAL ($1) parameters every read in this file uses, it sends one extended-protocol Parse and PostgreSQL rejects it with "cannot insert multiple commands into a prepared statement". Failure-isolated as that path is, the whole family would have been silently dead with one logged error per server per sweep. Per-grain gating is also correct on its own merits: the AG collectors are scheduled independently, so a healthy replica-grain timestamp must not vouch for stale database-grain rows. The read is skipped entirely when the master switch is off, so an AG-free fleet pays nothing either way. Also fixes four pieces of adjacent drift found while wiring this up: - AlertMetricClassifier.IsResolution recognized only Cleared/Resolved/Restored, so Darling's "Collection Resumed", "Agent Restarted" and "Compression Job Recovered" recoveries have been shipping styled as live actionable alerts in both apps' Alert History grids. Added Resumed/Restarted/Recovered/Reconnected rather than letting the AG recoveries become a fifth blind spot; no actionable metric name in either app contains those words. Both hand-maintained SQL copies of that suffix list (Darling's DailySummarySql, Lite's LocalDataService.DailySummary) are widened to match - they drive the Daily Summary actionable-alert counts in BOTH apps. - Registered all five AG metric names in the shared AlertSeverity map so a renderer reaching it without an explicit severity override does not fall through to INFO-blue (the #1136 gap). - get_alert_settings (MCP) stopped at 36 columns while the store grew to 41, so an MCP client could not see the V33 connection opt-ins at all. Extended to the full 41. ViewerControlPlaneStage3bTests - the parity test that pins the column list against the upsert placeholders, the bind order and the reader ordinals, i.e. the guard for the highest-risk defect class in this plumbing - had drifted the same way; extended, and its placeholder loop now runs off Columns.Length so the literal cannot drift again. - The Darling suite had a real ~1-in-6 intermittent failure, and it was not the AG work: ViewerTimeHelper keeps the display mode and UTC offset in process-wide statics, three test classes must mutate them, and their shared "viewer-time-statics" 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 rendering a timestamp could observe a swapped mode mid-assertion; the victim differed run to run (ViewerSystemEventsTests, then ViewerWave3DisplayTests), which is the shape that reads as "flaky, just re-run it". Added the definition with DisableParallelization, which constrains the three mutators instead of chasing an open-ended set of readers. Verified with 12 consecutive full-suite runs. TestResults/ is gitignored - trx logs from that investigation would otherwise land in commits. The two migration pin tests are re-pinned by identity rather than by ordinal and literal count - the same fact stated twice made every stacked branch collide on them. Suites green: Darling 3194, Lite 1494, Dashboard 768. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
One Added entry for the Availability Group alert family, three Fixed entries for the adjacent drift (the resolution-suffix misclassification in both apps, the MCP alert-settings under-report, and the parallel-collection test flake), plus the link-ref. Also corrects the clause #1688 left behind saying the failover and sync-fell-behind alerts were still follow-ups. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
erikdarlingdata
enabled auto-merge
July 26, 2026 18:15
# Conflicts: # CHANGELOG.md
This was referenced Jul 26, 2026
This was referenced Jul 26, 2026
pull Bot
pushed a commit
to ehtick/PerformanceMonitor
that referenced
this pull request
Jul 29, 2026
…rlingdata#991) The two AG store reads shipped in erikdarlingdata#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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1688, which landed AG collection and explicitly deferred the alerts. This adds the alert family over those two tables.
The alerts
AG Failoverrole_descchanged vs the previous sweep, per ag+replicaAG Replica Disconnectedconnected_state_desctransitions toDISCONNECTEDAG Replica ReconnectedAG Sync Fell Behindsecondary_lag_seconds >= ag_lag_alert_seconds, orredo_queue_size >= ag_redo_queue_alert_kbAG Database Suspendedis_suspendedfalse -> true, detail carriessuspend_reason_descMetric names are webhook automation keys and are consts, not inline literals. All five are registered in the shared
AlertSeveritymap so a history replay that reaches the map without an explicit override does not render INFO-blue.Settings are store-backed on
config.config_alert_settings(migration V35, allNOT NULL DEFAULT):notify_ag_health(true),ag_lag_alert_seconds(300, clamped 0-86400),ag_redo_queue_alert_kb(0 = off, clamped 0-1073741824), wired throughDarlingConfig/DarlingAlertSettings/StoreConfigProvider/ViewerDataService/ the viewer Settings window on the #1674 pattern and read live.Judgment calls
State is keyed per AG grain, but alerts fire under the real
server_id. A server hosts many replicas and databases, so two lagging databases must track and recover independently. Keying the alert that way would have cost the per-server delivery-mode override (#1236) and history correlation, so the grain lives in the alert text instead.JudgeAgSyncreturns three states, not a bool. MS Learn documentssecondary_lag_secondsreading0(not NULL) while data movement is SUSPENDED, so the seconds trigger has to abstain on a suspended row. Abstaining had to become its own answer rather than a plain "not behind", or the caller reads it as recovery: a lagging database that got suspended, or whose columns went NULL under quorum loss, would announce "AG Sync Recovered - has caught up with the primary" in the same sweep that reported it suspended. Only a database a sweep measured as caught up resolves a standing alert. That also makes cross-server resolution structurally impossible rather than something a prefix check has to catch. The redo-queue trigger keeps judging while suspended, because that backlog is real.One statement per command. The two grains were briefly read as two statements over a single command to save a round trip. That cannot work: Npgsql only splits multi-statement text into a batch when it parses the SQL for named placeholders. With the positional (
$1) parameters every read in this file uses, it sends one extended-protocolParseand PostgreSQL rejects it withcannot insert multiple commands into a prepared statement. Because the path is failure-isolated, the whole family would have been silently dead behind one logged error per server per sweep. Split into two commands, each gated on its own snapshot time, which is independently correct: the two AG collectors are scheduled separately, so a healthy replica-grain timestamp must not vouch for stale database-grain rows. The read is skipped entirely when the master switch is off, so an AG-free fleet pays nothing.NULL is never a transition. Under WSFC quorum loss the AG catalog views serve only cached metadata and any column can read NULL. Treating that as an edge would spray alerts across the fleet at the exact moment the cluster is already in trouble.
Adjacent drift fixed in the same change
AlertMetricClassifier.IsResolutionrecognized only Cleared/Resolved/Restored. Darling'sCollection Resumed,Agent RestartedandCompression Job Recoveredrecoveries have been shipping styled as live actionable alerts in both apps' Alert History grids. Widened to include Resumed/Restarted/Recovered/Reconnected rather than letting the AG recoveries become a fifth blind spot; no actionable metric name in either app contains those words. Both hand-maintained SQL copies of that list (DarlingDailySummarySql, LiteLocalDataService.DailySummary) are widened to match, since they drive the Daily Summary actionable-alert counts in both apps.get_alert_settings(MCP) stopped at 36 columns while the store grew to 41, so an MCP client could not see the V33 connection opt-ins at all. Extended to 41.ViewerControlPlaneStage3bTests, the parity test pinning the column list against the upsert placeholders, bind order and reader ordinals (the guard for the riskiest defect class in this plumbing), had drifted the same way; extended, and its placeholder loop now runs offColumns.Lengthso the literal cannot drift again.ViewerTimeHelperkeeps the display mode and UTC offset in process-wide statics; three test classes must mutate them, and their sharedviewer-time-staticscollection had noCollectionDefinition, so it grouped its members without constraining them and xUnit ran it in parallel with every other collection. Any viewer test rendering a timestamp could observe a swapped mode mid-assertion, and the victim differed run to run (ViewerSystemEventsTests, thenViewerWave3DisplayTests) - the shape that reads as "flaky, re-run it". Added the definition withDisableParallelization, constraining the three mutators instead of chasing an open-ended set of readers. Verified with 12 consecutive full-suite runs.TestResults/is now gitignored.The two migration pin tests are re-pinned by identity rather than by ordinal and literal count - the same fact stated twice made every stacked branch collide on them.
Known limits (deliberate, not oversights)
JudgeAgSyncis a pure static and is the obviousPerformanceMonitor.Commontwin ofConnectionAlertPolicywhenever Lite follows.is_localon the replica grain, which the collector does not carry. Consistent with how every other alert in the product reports each server's own view, but worth a follow-up if it proves noisy in the field.AG Replica Disconnectedis a pure edge with no re-fire, matching the connect edge's pre-Connection alerts never fire for a server that is already down, and never re-fire during a standing outage #1659 behavior. The Connection alerts never fire for a server that is already down, and never re-fire during a standing outage #1659 re-fire treatment is the obvious follow-up if a standing AG outage needs re-announcing.AlertMetricClassifier.IsCriticalstill matches only Deadlock/Poison, soAG Replica Disconnectedstyles as a warning row in the history grids, exactly asServer UnreachableandCapture Downalready do. Left alone: changing it would restyle existing alerts, which is a separate call.Testing
dotnet testgreen on all three suites: Darling 3194, Lite 1494, Dashboard 768 (0 failed). No liveInstaller.Testscategories were run. 24 new test methods plus 12 newAlertMetricClassifiercases cover every transition including baselines, recoveries, NULL readings, the suspend/lag interaction, per-database independence, cross-server scoping, live threshold re-reads, andForget.Reviewed by a
code-reviewersubagent before push; its two highest findings (the multi-statement read and the false-recovery path) are the two fixed above.🤖 Generated with Claude Code