From 05aafceff758c01b7065c23809d35ae2730a03ee Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sun, 26 Jul 2026 18:53:20 -0400 Subject: [PATCH 1/2] Report an AG failover once, and re-fire a standing replica disconnect (#1696) Items 2 and 3 of #1696, in one migration because they ship together. CROSS-NODE DE-DUPLICATION. Every replica in an Availability Group is visible from every node, so a fully-monitored 3-node AG collected the same replica's state three times and reported one failover THREE times - once per monitored server. `is_local` is added to ag_replica_states (from the DMV, not inferred: matching a monitored server's name against replica_server_name is unreliable across instance names, aliases and listeners), and one server now judges each AG. Which server is chosen is not arbitrary. A SECONDARY's sys.dm_hadr_* carries only its OWN row - measured on the live fixture - so only the primary sees the whole group. Vantage is therefore ranked None < Remote < Local < LocalPrimary, the best available wins, and a secondary yields to the primary as soon as the primary is monitored. Ties keep the incumbent so authority cannot oscillate. The AG edge state drops serverId from its key as part of this. An AG is one object however many of its nodes we watch, so whoever is authoritative reads and writes the SAME state - which is what lets authority move without re-baselining and losing an alert, or double-firing one. That also changes what Forget means: it releases the departing server's CLAIM so a survivor can take over, and deliberately keeps the group's edge state, because another node may still be watching it and dropping the state would swallow the next failover. A NULL is_local reads as "unknown", never as "not local". Rows collected before this migration genuinely do not know, and de-duplicating on a false negative would drop a real alert; a snapshot with no known-local row is treated as un-de-duplicable and every row is kept, which is the direction that keeps alerts rather than losing them. DISCONNECT RE-FIRE. "AG Replica Disconnected" was a pure edge, so a replica that stayed disconnected for a week announced it once. ag_disconnect_refire_minutes (V37, NOT NULL DEFAULT 0 = off, clamped 0-1440) re-announces on the #1659 pattern: same metric name, because webhook automation keyed on it is exactly what a re-fire exists to re-trigger; stamped on DELIVERY only, so an alert suppressed by the master switch cannot consume the window; cleared on reconnect so a later outage starts a fresh episode. Default off means nothing starts re-alerting on upgrade. Both columns ride migration V37, claimed after sweeping every worktree on disk and every remote branch - dev was at 36 and nothing claimed 37. Two pin tests needed real thought rather than renumbering: - PgSchemaGeneratorTests: ag_replica_states is now the same two-migration story the database grain already was - V34 created 10 columns, V37 appends is_local, and only their sum equals the generator's output. Reused the existing TruncatedSchema pattern. Its "V34 was not widened in place" sweep is deliberately NOT copied for this grain: the exact-block assertion above it is strictly stronger, and a substring sweep would false-positive on the DATABASE grain's own legitimate is_local column. - The two AG tests whose premise this supersedes are rewritten rather than renumbered, because per-server scoping and Forget-drops-state are no longer the intended behavior. They now pin the new contract: a fully-monitored AG reports once, and Forget hands over cleanly. The Lite golden DuckDB schema gains is_local too - the generator drives Lite's tables, so a new collector column has to appear there or fresh and upgraded stores diverge. Suites green: Darling 3290, Lite 1534, Dashboard 768; Installer builds. Co-Authored-By: Claude Fable 5 --- .../DarlingObservabilityTests.cs | 4 +- .../Darling.Tests/DarlingSelfAlertTests.cs | 189 ++++++++++++++---- .../Darling.Tests/PgSchemaGeneratorTests.cs | 30 ++- .../ViewerControlPlaneStage3bTests.cs | 3 +- .../Darling.Tests/ViewerDataServiceTests.cs | 2 +- .../DarlingAlertSettings.cs | 5 + .../DarlingConfig.cs | 6 + .../DarlingSelfAlertEvaluator.cs | 149 +++++++++++--- .../DarlingWorker.cs | 3 +- .../Mcp/DarlingAlertReader.cs | 9 +- .../StoreConfigProvider.cs | 13 +- .../PgMigrations.cs | 27 +++ .../StorageVersion.cs | 2 +- .../SettingsWindow.xaml | 8 + .../SettingsWindow.xaml.cs | 4 + .../ViewerDataService.AlertSettings.cs | 17 +- .../ViewerDataService.cs | 12 +- Lite.Tests/AgCollectorDefinitionTests.cs | 35 +++- Lite.Tests/GoldenCollectorSchema.cs | 3 +- .../AgReplicaStatesCollector.cs | 19 +- PerformanceMonitor.Common/AgAlertPolicy.cs | 68 +++++++ deprecated/Installer/packages.lock.json | 6 +- 22 files changed, 513 insertions(+), 101 deletions(-) diff --git a/Darling/Darling.Tests/DarlingObservabilityTests.cs b/Darling/Darling.Tests/DarlingObservabilityTests.cs index 0e7eef06e4..e4777e05ba 100644 --- a/Darling/Darling.Tests/DarlingObservabilityTests.cs +++ b/Darling/Darling.Tests/DarlingObservabilityTests.cs @@ -76,8 +76,8 @@ public void MigrationScripts_AreRegisteredInAscendingOrder_V34AgCollectors_V36Ag Assert.Equal(33, PgMigrations.Scripts[32].Version); /* The newest migration is asserted by identity rather than by ordinal: this ladder is walked by every stacked branch at once, and a positional pin turns each addition into a conflict for the next. */ - Assert.Equal(36, PgMigrations.Scripts[^1].Version); - Assert.Equal(36, StorageVersion.SchemaVersion); + Assert.Equal(37, PgMigrations.Scripts[^1].Version); + Assert.Equal(37, StorageVersion.SchemaVersion); /* V34 (#991) creates the two Availability Group collector tables. Schema-qualified collect.* and CREATE TABLE IF NOT EXISTS, per the file's additive-create idiom (V29): a no-op on a fresh store diff --git a/Darling/Darling.Tests/DarlingSelfAlertTests.cs b/Darling/Darling.Tests/DarlingSelfAlertTests.cs index 2b4b5962eb..a28476efec 100644 --- a/Darling/Darling.Tests/DarlingSelfAlertTests.cs +++ b/Darling/Darling.Tests/DarlingSelfAlertTests.cs @@ -152,6 +152,9 @@ private sealed class Harness public int AgLagAlertSeconds { get; set; } = 300; public long AgRedoQueueAlertKb { get; set; } + /// #1696 (V37): AG disconnect re-fire minutes. Default 0 = off, the shipped behavior. + public int AgDisconnectRefireMinutes { get; set; } + public DateTime Now { get; set; } = new(2026, 7, 1, 12, 0, 0, DateTimeKind.Utc); /// #1681: captures what the evaluator writes to the service log, so the firing/recovery pair @@ -167,7 +170,8 @@ private sealed class Harness connectionRefireMinutes: () => ConnectionRefireMinutes, notifyAgHealth: () => NotifyAgHealth, agLagAlertSeconds: () => AgLagAlertSeconds, - agRedoQueueAlertKb: () => AgRedoQueueAlertKb); + agRedoQueueAlertKb: () => AgRedoQueueAlertKb, + agDisconnectRefireMinutes: () => AgDisconnectRefireMinutes); } /* ---------------- #991 Availability Group fixtures ---------------- */ @@ -177,8 +181,9 @@ private sealed class Harness private const string Db = "Sales"; private static AgReplicaReading ReplicaRow( - string? role = "SECONDARY", string? connected = "CONNECTED", string replica = Replica, string ag = Ag) => - new(ag, replica, role, connected); + string? role = "SECONDARY", string? connected = "CONNECTED", string replica = Replica, string ag = Ag, + bool? isLocal = null) => + new(ag, replica, role, connected, isLocal); private static AgDatabaseReading DatabaseRow( long? lagSeconds = 0, @@ -1443,28 +1448,28 @@ public async Task AgSyncFellBehind_BothTriggersOff_NeitherFiresNorResolves() } [Fact] - public async Task AgSyncFellBehind_RecoverySweep_IsScopedToTheServerBeingEvaluated() + public async Task AgSyncFellBehind_OneAgIsJudgedByOneServer_SoAFullyMonitoredAgReportsItOnce() { var h = new Harness(); var e = h.Build(); - const int otherServerId = 515151; + const int nodeA = 100001; + const int nodeB = 100002; - /* Two servers each have a lagging database. */ - await e.ApplyAgDatabaseHealthAsync(ServerId, Name, new[] { DatabaseRow(lagSeconds: 600) }, Ct); - await e.ApplyAgDatabaseHealthAsync(otherServerId, "OTHER-SRV", new[] { DatabaseRow(lagSeconds: 600) }, Ct); - Assert.Equal(2, h.Deliverer.Outcomes.Count); + /* #1696: both nodes are monitored and BOTH see the same AG database, because every replica is + visible from every node. Only one may judge it, or a 3-node AG reports one problem three times. */ + await e.ApplyAgDatabaseHealthAsync(nodeA, "NODE-A", new[] { DatabaseRow(lagSeconds: 600) }, Ct); + await e.ApplyAgDatabaseHealthAsync(nodeB, "NODE-B", new[] { DatabaseRow(lagSeconds: 600) }, Ct); - /* The FIRST server catches up. Its snapshot says nothing about the second server, so a recovery sweep - that walked every tracked key would silently resolve the other server's live alert. */ - await e.ApplyAgDatabaseHealthAsync(ServerId, Name, new[] { DatabaseRow(lagSeconds: 0) }, Ct); - var resolution = Assert.Single(h.History.Records); - Assert.Equal(Key, resolution.ServerId); + var fired = Assert.Single(h.Deliverer.Outcomes); + Assert.Equal("AG Sync Fell Behind", fired.MetricName); - /* Proof the other server's state survived: it is still inside its cooldown, so it stays quiet rather - than re-firing as a fresh episode. */ - await e.ApplyAgDatabaseHealthAsync(otherServerId, "OTHER-SRV", new[] { DatabaseRow(lagSeconds: 600) }, Ct); - Assert.Equal(2, h.Deliverer.Outcomes.Count); + /* The recovery is announced once too, by the same authoritative node. */ + await e.ApplyAgDatabaseHealthAsync(nodeA, "NODE-A", new[] { DatabaseRow(lagSeconds: 0) }, Ct); + await e.ApplyAgDatabaseHealthAsync(nodeB, "NODE-B", new[] { DatabaseRow(lagSeconds: 0) }, Ct); + + var resolution = Assert.Single(h.History.Records); + Assert.Equal("AG Sync Recovered", resolution.MetricName); } /* ---------------- #991: database suspended ---------------- */ @@ -1521,6 +1526,119 @@ public async Task AgDatabaseSuspended_AlreadySuspendedAtFirstSighting_IsASilentB Assert.Equal("no reason reported", fired.CurrentValue); } + /* ---------------- #1696: fleet de-dup + disconnect re-fire ---------------- */ + + [Fact] + public async Task AgFailover_FullyMonitoredThreeNodeAg_ReportsTheFailoverOnce() + { + var h = new Harness(); + var e = h.Build(); + + /* Every replica is visible from EVERY node, so all three monitored servers report the same two + replica rows. Before #1696 that meant one failover paged three times. */ + var beforeFailover = new[] + { + ReplicaRow(role: "PRIMARY", replica: "NODE1"), + ReplicaRow(role: "SECONDARY", replica: "NODE2"), + }; + var afterFailover = new[] + { + ReplicaRow(role: "SECONDARY", replica: "NODE1"), + ReplicaRow(role: "PRIMARY", replica: "NODE2"), + }; + + foreach (var serverId in new[] { 100001, 100002, 100003 }) + { + await e.ApplyAgReplicaHealthAsync(serverId, "NODE" + serverId, beforeFailover, Ct); + } + + Assert.Empty(h.Deliverer.Outcomes); + + foreach (var serverId in new[] { 100001, 100002, 100003 }) + { + await e.ApplyAgReplicaHealthAsync(serverId, "NODE" + serverId, afterFailover, Ct); + } + + /* Two replicas changed role, so two alerts — NOT six. */ + Assert.Equal(2, h.Deliverer.Outcomes.Count); + Assert.All(h.Deliverer.Outcomes, o => Assert.Equal("AG Failover", o.MetricName)); + } + + [Fact] + public async Task AgAuthority_PrimaryTakesOverFromASecondary_BecauseOnlyThePrimarySeesTheWholeGroup() + { + var h = new Harness(); + var e = h.Build(); + + /* A monitored SECONDARY claims the AG first — its sys.dm_hadr_* is a one-row self-view. */ + await e.ApplyAgReplicaHealthAsync( + 200001, "SEC", new[] { ReplicaRow(role: "SECONDARY", replica: "NODE2", isLocal: true) }, Ct); + + /* The PRIMARY is then monitored. Its vantage is strictly better, so it takes over and its view of + NODE1 is judged — which the secondary could never have supplied. */ + await e.ApplyAgReplicaHealthAsync(200002, "PRI", new[] + { + ReplicaRow(role: "PRIMARY", replica: "NODE1", isLocal: true), + ReplicaRow(role: "SECONDARY", replica: "NODE2"), + }, Ct); + + Assert.Empty(h.Deliverer.Outcomes); + + await e.ApplyAgReplicaHealthAsync(200002, "PRI", new[] + { + ReplicaRow(role: "SECONDARY", replica: "NODE1", isLocal: true), + ReplicaRow(role: "SECONDARY", replica: "NODE2"), + }, Ct); + + var fired = Assert.Single(h.Deliverer.Outcomes); + Assert.Equal("AG Failover", fired.MetricName); + } + + [Fact] + public async Task AgReplicaDisconnected_RefireOff_IsAPureEdge() + { + var h = new Harness(); + var e = h.Build(); + + await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(connected: "CONNECTED") }, Ct); + await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(connected: "DISCONNECTED") }, Ct); + Assert.Single(h.Deliverer.Outcomes); + + /* Default is off, so a replica down for a week still announces exactly once. */ + h.Now = h.Now.AddHours(8); + await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(connected: "DISCONNECTED") }, Ct); + Assert.Single(h.Deliverer.Outcomes); + } + + [Fact] + public async Task AgReplicaDisconnected_Refire_ReAnnouncesUnderTheSameMetricName_ThenReconnectClearsTheClock() + { + var h = new Harness { AgDisconnectRefireMinutes = 10 }; + var e = h.Build(); + + await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(connected: "CONNECTED") }, Ct); + await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(connected: "DISCONNECTED") }, Ct); + Assert.Single(h.Deliverer.Outcomes); + + /* Inside the window: quiet. */ + h.Now = h.Now.AddMinutes(5); + await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(connected: "DISCONNECTED") }, Ct); + Assert.Single(h.Deliverer.Outcomes); + + /* Past it: re-announce under the SAME metric name, so webhook automation keyed on it re-triggers. */ + h.Now = h.Now.AddMinutes(6); + await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(connected: "DISCONNECTED") }, Ct); + Assert.Equal(2, h.Deliverer.Outcomes.Count); + Assert.Equal("AG Replica Disconnected", h.Deliverer.Outcomes[1].MetricName); + Assert.Contains("STILL disconnected", h.Deliverer.Outcomes[1].ShortMessage, StringComparison.Ordinal); + + /* Reconnect clears the clock, so a later outage starts a fresh episode rather than re-firing late. */ + h.Now = h.Now.AddMinutes(1); + await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(connected: "CONNECTED") }, Ct); + Assert.Equal(3, h.Deliverer.Outcomes.Count); + Assert.Equal("AG Replica Reconnected", h.Deliverer.Outcomes[2].MetricName); + } + /* ---------------- #991: gating and forget ---------------- */ [Fact] @@ -1560,30 +1678,33 @@ public async Task AgAlerts_ThresholdsAreReadLive_SoAStoreEditTakesEffectOnTheNex } [Fact] - public async Task AgAlerts_Forget_DropsCompositeKeyedState_SoAReAddedServerReBaselines() + public async Task AgAlerts_Forget_ReleasesTheAgClaim_ButKeepsTheGroupsEdgeState() { var h = new Harness(); var e = h.Build(); - /* Establish a role baseline and a standing sync-behind alert. */ - await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(role: "PRIMARY") }, Ct); - await e.ApplyAgDatabaseHealthAsync(ServerId, Name, new[] { DatabaseRow(lagSeconds: 600) }, Ct); - Assert.Single(h.Deliverer.Outcomes); + const int nodeA = 100001; + const int nodeB = 100002; - /* Removed from the monitored set. AG state is keyed by a COMPOSITE, so the per-server TryRemove that - clears the other conditions cannot reach it — Forget has to sweep the prefix. */ - e.Forget(ServerId); + /* NODE-A claims the AG and establishes a role baseline; NODE-B sees the same AG but defers. */ + await e.ApplyAgReplicaHealthAsync(nodeA, "NODE-A", new[] { ReplicaRow(role: "PRIMARY") }, Ct); + await e.ApplyAgReplicaHealthAsync(nodeB, "NODE-B", new[] { ReplicaRow(role: "PRIMARY") }, Ct); + Assert.Empty(h.Deliverer.Outcomes); - /* Re-added: the role is a fresh baseline (no phantom failover) ... */ - await e.ApplyAgReplicaHealthAsync(ServerId, Name, new[] { ReplicaRow(role: "SECONDARY") }, Ct); - Assert.Single(h.Deliverer.Outcomes); + /* NODE-A is removed from monitoring. Its CLAIM is released so a survivor can take over, but the + AG's edge state is deliberately kept: the group still exists and NODE-B is still watching it. + Dropping the state here would re-baseline a live group and swallow the next failover. */ + e.Forget(nodeA); - /* ... and the sync-behind episode starts over rather than sitting inside the dropped cooldown stamp. */ - await e.ApplyAgDatabaseHealthAsync(ServerId, Name, new[] { DatabaseRow(lagSeconds: 600) }, Ct); - Assert.Equal(2, h.Deliverer.Outcomes.Count); + /* NODE-B takes over and sees the role change against the state NODE-A left behind — so the failover + is reported exactly once, by the new owner, with the correct previous role. */ + await e.ApplyAgReplicaHealthAsync(nodeB, "NODE-B", new[] { ReplicaRow(role: "SECONDARY") }, Ct); - /* Nothing lingered to resolve, so no stray recovery row was written for the forgotten episode. */ - Assert.Empty(h.History.Records); + var fired = Assert.Single(h.Deliverer.Outcomes); + Assert.Equal("AG Failover", fired.MetricName); + Assert.Equal("PRIMARY", fired.ThresholdValue); + Assert.Equal("SECONDARY", fired.CurrentValue); + Assert.Equal(nodeB.ToString(System.Globalization.CultureInfo.InvariantCulture), fired.ServerKey); } [Fact] diff --git a/Darling/Darling.Tests/PgSchemaGeneratorTests.cs b/Darling/Darling.Tests/PgSchemaGeneratorTests.cs index 044370c819..31056b8d42 100644 --- a/Darling/Darling.Tests/PgSchemaGeneratorTests.cs +++ b/Darling/Darling.Tests/PgSchemaGeneratorTests.cs @@ -427,8 +427,36 @@ static string CollectQualified(ICollectorSchemaInfo schema) upgraded store's physical shape differ from a fresh one. */ var v34 = Lf(PgMigrations.Scripts.Single(m => m.Version == 34).Sql); - Assert.Contains(CollectQualified(AgReplicaStatesCollector.Instance), v34, StringComparison.Ordinal); + /* The REPLICA-grain table is now the same two-migration story as the database grain below: V34 + created its first 10 payload columns and V37 (#1696) appended is_local, so an upgraded store's + shape is V34 + V37 and only their sum equals the generator's current output. */ + var replicaColumns = AgReplicaStatesCollector.Instance.PayloadColumns; + const int V34ReplicaColumnCount = 10; + + Assert.Contains( + CollectQualified(new TruncatedSchema(AgReplicaStatesCollector.Instance, V34ReplicaColumnCount)), + v34, + StringComparison.Ordinal); Assert.Contains("CREATE INDEX IF NOT EXISTS idx_ag_replica_states_time ON collect.ag_replica_states(server_id, collection_time);", v34, StringComparison.Ordinal); + + var v37 = Lf(PgMigrations.Scripts.Single(m => m.Version == 37).Sql); + + foreach (var column in replicaColumns.Skip(V34ReplicaColumnCount)) + { + var generatedType = Lf(PgSchemaGenerator.CreateTable(new TruncatedSchema(AgReplicaStatesCollector.Instance, replicaColumns.Count))) + .Split('\n') + .Single(l => l.TrimStart().StartsWith(column.Name + " ", StringComparison.Ordinal)) + .Trim() + .TrimEnd(','); + + Assert.Contains($"ADD COLUMN IF NOT EXISTS {generatedType}", v37, StringComparison.Ordinal); + } + + /* No "V34 was not widened in place" sweep for this grain, unlike the database one below: the + TruncatedSchema assertion above already matches V34's ag_replica_states block EXACTLY, which is a + strictly stronger statement than any name-absence check. A substring sweep would also be wrong + here — is_local is a real V34 column on the DATABASE grain, so searching the whole migration body + for it finds the other table's legitimate column and fails. */ Assert.Contains("CREATE INDEX IF NOT EXISTS idx_ag_database_replica_states_time ON collect.ag_database_replica_states(server_id, collection_time);", v34, StringComparison.Ordinal); /* The database-grain table is the one case where a single migration is NOT the whole story: V34 diff --git a/Darling/Darling.Tests/ViewerControlPlaneStage3bTests.cs b/Darling/Darling.Tests/ViewerControlPlaneStage3bTests.cs index d35bfc222e..06315cfba0 100644 --- a/Darling/Darling.Tests/ViewerControlPlaneStage3bTests.cs +++ b/Darling/Darling.Tests/ViewerControlPlaneStage3bTests.cs @@ -27,7 +27,7 @@ namespace Darling.Tests; /// public sealed class ViewerAlertSettingsSqlTests { - /* The 41 AlertsConfig + AnalysisConfig columns the service reads (StoreConfigProvider.ReadAlertSettingsAsync) + /* The 42 AlertsConfig + AnalysisConfig columns the service reads (StoreConfigProvider.ReadAlertSettingsAsync) — delivery_mode/per_event_max appended in V18 (#1141/#1236), the six long-running-query read knobs + the connection-change notify toggle in V20, the two connection opt-ins in V33 (#1659), and the three Availability Group knobs in V35 (#991). @@ -50,6 +50,7 @@ parallel sequences (this list, the upsert's $N placeholders, the bind order, and "long_running_query_exclude_misc_waits", "long_running_query_exclude_cdc", "notify_connection_changes", "notify_connection_down_at_startup", "connection_refire_minutes", "notify_ag_health", "ag_lag_alert_seconds", "ag_redo_queue_alert_kb", + "ag_disconnect_refire_minutes", }; [Fact] diff --git a/Darling/Darling.Tests/ViewerDataServiceTests.cs b/Darling/Darling.Tests/ViewerDataServiceTests.cs index 3e17e5d833..75972ce9e5 100644 --- a/Darling/Darling.Tests/ViewerDataServiceTests.cs +++ b/Darling/Darling.Tests/ViewerDataServiceTests.cs @@ -470,7 +470,7 @@ public void RequiredStoreSchemaVersion_TracksTheBuildSchemaVersion_AndTheProbeCo the connect-time gate refuse to open the viewer against a perfectly healthy store. */ Assert.Equal( ViewerDataService.RequiredStoreSchemaVersion, - ViewerDataService.MapProbedSchemaVersion(true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true)); + ViewerDataService.MapProbedSchemaVersion(true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true, true)); } } diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingAlertSettings.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingAlertSettings.cs index 7af1cecb71..d6218164a9 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingAlertSettings.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingAlertSettings.cs @@ -104,6 +104,11 @@ public DarlingAlertSettings(DarlingConfig config) /// it is indistinguishable from off, and a negative would fire on every healthy row. public long AgRedoQueueAlertKb => Math.Clamp(_config.Alerts.AgRedoQueueAlertKb, 0L, 1073741824L); + /// #1696 (V37), read live: re-announce a still-disconnected AG replica every N minutes + /// (0 = off). Clamped 0–1440 like the sibling connection re-fire, so a hand-edited row can drive + /// neither a per-sweep spam loop nor a never-fires interval. + public int AgDisconnectRefireMinutes => Math.Clamp(_config.Alerts.AgDisconnectRefireMinutes, 0, 1440); + /// "sql" → SqlProcess; anything else (incl. Lite's default "total") → TotalServer. public CpuAlertMode CpuAlertMode => string.Equals(_config.Alerts.CpuMode, "sql", StringComparison.OrdinalIgnoreCase) diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs index cd623317ac..5334e8037d 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs @@ -340,6 +340,12 @@ public sealed class AlertsConfig [JsonPropertyName("agRedoQueueAlertKb")] public long AgRedoQueueAlertKb { get; set; } + /// #1696 (V37): re-announce a still-disconnected AG replica every N minutes (0 = off, the + /// default). "AG Replica Disconnected" was a pure edge, so a replica down for a week announced it once. + /// Re-fires deliver under the SAME metric name so webhook automation keyed on it re-triggers. + [JsonPropertyName("agDisconnectRefireMinutes")] + public int AgDisconnectRefireMinutes { get; set; } + [JsonPropertyName("cpuEnabled")] public bool CpuEnabled { get; set; } = true; diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs index 40d72fb729..a0ee0c0347 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs @@ -117,6 +117,10 @@ the Func seam also keeps the test fakes — which implement only IAlertEngineSet /// master switch gates all four AG conditions; the two thresholds are already clamped by /// , so a hand-edited store row cannot drive a nonsense window. private readonly Func _notifyAgHealth; + + /// #1696 (V37) re-fire interval for "AG Replica Disconnected", read live like the other AG + /// seams. 0 = off, the shipped default. + private readonly Func _agDisconnectRefireMinutes; private readonly Func _agLagAlertSeconds; private readonly Func _agRedoQueueAlertKb; @@ -185,6 +189,19 @@ sibling condition — the restart replay protection is the deliverer's history-s private readonly ConcurrentDictionary _lastAgSyncBehindAlert = new(StringComparer.Ordinal); private readonly ConcurrentDictionary _agDatabaseSuspended = new(StringComparer.Ordinal); + /// Which monitored server currently judges each Availability Group, and how good its view is + /// (#1696). Every replica is visible from every node, so a fully-monitored 3-node AG previously reported + /// one failover THREE times. One server owns each AG; a server with a strictly better vantage takes over + /// (a secondary yielding to the primary, whose view is the only complete one). Fleet-level, so it is + /// deliberately NOT dropped by the way the per-server state is — see there. + private readonly ConcurrentDictionary _agAuthority = + new(StringComparer.Ordinal); + + /// When "AG Replica Disconnected" last DELIVERED per ag+replica — the #1659 re-fire clock + /// (V37). Stamped on delivery only, so a decision suppressed by the master switch cannot consume the + /// window; cleared on reconnect. + private readonly ConcurrentDictionary _lastAgDisconnectAlert = new(StringComparer.Ordinal); + /* The AG alert metric names and every pure AG decision now live in the shared PerformanceMonitor.Common.AgAlertPolicy, so Lite fires the SAME names off the SAME rules (#1696) — the ConnectionAlertPolicy discipline. This evaluator keeps only the edge STATE and the delivery; the @@ -225,7 +242,8 @@ public DarlingSelfAlertEvaluator( Func? connectionRefireMinutes = null, Func? notifyAgHealth = null, Func? agLagAlertSeconds = null, - Func? agRedoQueueAlertKb = null) + Func? agRedoQueueAlertKb = null, + Func? agDisconnectRefireMinutes = null) { _settings = settings ?? throw new ArgumentNullException(nameof(settings)); _deliverer = deliverer ?? throw new ArgumentNullException(nameof(deliverer)); @@ -241,6 +259,7 @@ public DarlingSelfAlertEvaluator( _notifyAgHealth = notifyAgHealth ?? (() => true); _agLagAlertSeconds = agLagAlertSeconds ?? (() => 300); _agRedoQueueAlertKb = agRedoQueueAlertKb ?? (() => 0); + _agDisconnectRefireMinutes = agDisconnectRefireMinutes ?? (() => 0); } private enum ConnectionState @@ -713,7 +732,18 @@ internal async Task ApplyAgReplicaHealthAsync( foreach (var replica in replicas) { - var key = AgReplicaKey(serverId, replica.AgName, replica.ReplicaServerName); + /* #1696 fleet-side de-dup: one monitored server judges each AG, so a fully-monitored 3-node AG + reports a failover ONCE instead of once per node that can see it. A server with a strictly + better vantage takes over — a secondary yielding to the primary, whose view is the only + complete one. The state key drops serverId for the same reason: whoever is authoritative + reads and writes the SAME edge state, so authority moving cannot re-baseline and lose an + alert, and cannot double-fire one either. */ + if (!IsAuthoritativeFor(serverId, replica.AgName, replicas)) + { + continue; + } + + var key = AgReplicaKey(replica.AgName, replica.ReplicaServerName); /* --- 1. Failover: the role changed since we last looked. --- */ if (!string.IsNullOrEmpty(replica.RoleDesc)) @@ -745,7 +775,20 @@ await FireAsync( var connection = AgAlertPolicy.DecideConnection(previousState, replica.ConnectedStateDesc); _agReplicaConnectedState[key] = replica.ConnectedStateDesc; - if (connection == AgConnectionDecision.Disconnected) + /* #1696 (V37): "AG Replica Disconnected" was a pure edge, so a replica that stayed + disconnected for a week announced it ONCE. The #1659 treatment: re-announce every N + minutes while it is still down (0 = off, the shipped default, so nothing starts + re-alerting on upgrade). Re-fires deliver under the SAME metric name, because webhook + automation keyed on it is exactly what a re-fire exists to re-trigger. */ + bool stillDisconnected = + connection == AgConnectionDecision.None + && AgAlertPolicy.IsDisconnected(replica.ConnectedStateDesc) + && _agDisconnectRefireMinutes() is int refire + && refire > 0 + && (!_lastAgDisconnectAlert.TryGetValue(key, out var lastDown) + || _utcNow() - lastDown >= TimeSpan.FromMinutes(refire)); + + if (connection == AgConnectionDecision.Disconnected || stillDisconnected) { await FireAsync( Key(serverId), serverName, AgReplicaDisconnectedMetric, replica.ConnectedStateDesc, "CONNECTED", @@ -756,8 +799,14 @@ await FireAsync( "automatic-failover partner. Check the replica's SQL Server service, the availability endpoint " + "(TCP 5022 by default) and its firewall rule, the WSFC quorum, and the network between the nodes.", severity: AlertSeverityLevel.Critical, - shortMessage: $"{replica.ReplicaServerName} in AG {replica.AgName} is disconnected from the primary", + shortMessage: stillDisconnected + ? $"{replica.ReplicaServerName} in AG {replica.AgName} is STILL disconnected from the primary" + : $"{replica.ReplicaServerName} in AG {replica.AgName} is disconnected from the primary", cancellationToken); + + /* Stamped on DELIVERY, never on the decision: an alert suppressed by the master switch + must not consume the re-fire window (the #1659 discipline). */ + _lastAgDisconnectAlert[key] = _utcNow(); } else if (connection == AgConnectionDecision.Reconnected) { @@ -770,6 +819,7 @@ await FireAsync( "send and redo queues until they drain before you count it as a failover target again.", severity: null, shortMessage: $"{replica.ReplicaServerName} in AG {replica.AgName} reconnected", cancellationToken); + _lastAgDisconnectAlert.TryRemove(key, out _); } } } @@ -810,7 +860,14 @@ internal async Task ApplyAgDatabaseHealthAsync( foreach (var database in databases) { - var key = AgDatabaseKey(serverId, database.AgName, database.DatabaseName, database.ReplicaServerName); + /* Same #1696 de-dup as the replica grain: without it a fully-monitored 3-node AG reports the + same database's lag once per node. */ + if (!IsAuthoritativeFor(serverId, database.AgName)) + { + continue; + } + + var key = AgDatabaseKey(database.AgName, database.DatabaseName, database.ReplicaServerName); /* --- 3. Data movement suspended (edge). --- */ if (database.IsSuspended is bool suspended) @@ -899,13 +956,47 @@ await RecordResolutionAsync(new AlertResolution( /// The prefix every AG state key for one server starts with — the scope for /// and for the per-server recovery sweep. - private static string AgServerPrefix(int serverId) => Key(serverId) + AgKeySeparator; + /* AG edge state is keyed by the AG GRAIN ALONE, deliberately without the serverId (#1696). An AG is one + object no matter how many of its nodes we monitor, so whichever server is authoritative reads and + writes the same state — which is what makes authority able to move (a secondary yielding to the + primary) without re-baselining and losing an alert, or double-firing one. */ + private static string AgReplicaKey(string agName, string replicaServerName) => + agName + AgKeySeparator + replicaServerName; - private static string AgReplicaKey(int serverId, string agName, string replicaServerName) => - AgServerPrefix(serverId) + agName + AgKeySeparator + replicaServerName; + private static string AgDatabaseKey(string agName, string databaseName, string replicaServerName) => + agName + AgKeySeparator + databaseName + AgKeySeparator + replicaServerName; - private static string AgDatabaseKey(int serverId, string agName, string databaseName, string replicaServerName) => - AgServerPrefix(serverId) + agName + AgKeySeparator + databaseName + AgKeySeparator + replicaServerName; + /// + /// Whether is the server that judges right now. + /// The first server to report an AG claims it; a server with a STRICTLY better vantage takes over, so a + /// secondary's one-row self-view yields to the primary's complete one as soon as the primary is + /// monitored. Ties keep the incumbent, so authority does not oscillate between equally-placed nodes. + /// + private bool IsAuthoritativeFor(int serverId, string agName, IReadOnlyList snapshot) + { + var vantage = AgAlertPolicy.ClassifyVantage(snapshot, agName); + var claimed = _agAuthority.AddOrUpdate( + agName, + _ => (serverId, vantage), + (_, current) => current.ServerId == serverId + ? (serverId, vantage) + : (vantage > current.Vantage ? (serverId, vantage) : current)); + + return claimed.ServerId == serverId; + } + + /// + /// The database-grain check. The replica grain runs first in the same sweep and has normally already + /// decided who owns this AG, so this defers to the incumbent rather than re-classifying. When nothing has + /// claimed the AG — the replica-grain snapshot was missing or stale while the database one is fresh — the + /// caller claims it at the weakest vantage, so the group is still judged by SOMEBODY rather than by + /// nobody. Judging once from a poor vantage beats silence; that is the direction that keeps alerts. + /// + private bool IsAuthoritativeFor(int serverId, string agName) + { + var claimed = _agAuthority.GetOrAdd(agName, _ => (serverId, AgVantage.Remote)); + return claimed.ServerId == serverId; + } /// Renders a database-grain AG key back into prose for a recovery message. The key is built here /// and never escaped, so this is a straight positional split; an unexpected shape degrades to the raw key @@ -913,8 +1004,8 @@ private static string AgDatabaseKey(int serverId, string agName, string database private static string DescribeAgDatabaseKey(string key) { var parts = key.Split(AgKeySeparator); - return parts.Length == 4 - ? $"database {parts[2]} in AG {parts[1]} on replica {parts[3]}" + return parts.Length == 3 + ? $"database {parts[1]} in AG {parts[0]} on replica {parts[2]}" : key; } @@ -1194,25 +1285,20 @@ public void Forget(int serverId) _connectionState.TryRemove(key, out _); _hasBeenOnline.TryRemove(key, out _); - /* AG state is keyed by a COMPOSITE (serverId + AG grain), so an exact-key TryRemove like the - per-server conditions above cannot reach it — sweep this server's key prefix instead. Missing this - is how a removed-then-re-added server would inherit a stale role and page a phantom failover. - ConcurrentDictionary.Keys hands back a snapshot, so removing while enumerating it is safe. */ - var agPrefix = AgServerPrefix(serverId); - ForgetAgKeys(_agReplicaRole, agPrefix); - ForgetAgKeys(_agReplicaConnectedState, agPrefix); - ForgetAgKeys(_activeAgSyncBehind, agPrefix); - ForgetAgKeys(_lastAgSyncBehindAlert, agPrefix); - ForgetAgKeys(_agDatabaseSuspended, agPrefix); - } + /* AG state is keyed by the AG GRAIN, not by server (#1696), so there is deliberately nothing here to + drop: an Availability Group outlives any one of its monitored nodes, and another node may still be + watching it. Dropping the edge state on removal would re-baseline a group that is still monitored + and silently swallow the next failover. - private static void ForgetAgKeys(ConcurrentDictionary state, string prefix) - { - foreach (var key in state.Keys) + What DOES belong to the departing server is its claim to judge an AG. Releasing it lets a + surviving node take over on its next sweep; the edge state it inherits is the same state, so the + handover neither re-baselines nor double-fires. If the removed server was the group's only + monitor, the state simply goes quiet — no snapshots arrive, so nothing fires. */ + foreach (var entry in _agAuthority) { - if (key.StartsWith(prefix, StringComparison.Ordinal)) + if (entry.Value.ServerId == serverId) { - state.TryRemove(key, out _); + _agAuthority.TryRemove(entry.Key, out _); } } } @@ -1385,7 +1471,7 @@ ORDER BY collection_time DESC await using var connection = await postgres.OpenConnectionAsync(cancellationToken); using var command = new NpgsqlCommand(@" -SELECT ag_name, replica_server_name, role_desc, connected_state_desc, collection_time +SELECT ag_name, replica_server_name, role_desc, connected_state_desc, is_local, collection_time FROM ag_replica_states WHERE server_id = $1 AND collection_time = (SELECT MAX(collection_time) FROM ag_replica_states WHERE server_id = $1) @@ -1395,7 +1481,7 @@ FROM ag_replica_states await using var reader = await command.ExecuteReaderAsync(cancellationToken); while (await reader.ReadAsync(cancellationToken)) { - newest = Newest(newest, reader, 4); + newest = Newest(newest, reader, 5); if (reader.IsDBNull(0) || reader.IsDBNull(1)) { continue; @@ -1405,7 +1491,8 @@ FROM ag_replica_states AgName: reader.GetString(0), ReplicaServerName: reader.GetString(1), RoleDesc: reader.IsDBNull(2) ? null : reader.GetString(2), - ConnectedStateDesc: reader.IsDBNull(3) ? null : reader.GetString(3))); + ConnectedStateDesc: reader.IsDBNull(3) ? null : reader.GetString(3), + IsLocal: reader.IsDBNull(4) ? null : reader.GetBoolean(4))); } return (newest, replicas); diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs index a469510c5b..8c605b095f 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs @@ -803,7 +803,8 @@ through the same by-reference alertSettings seam a store reload hot-swaps. */ without a restart (and the clamps live on the settings properties, not here). */ notifyAgHealth: () => alertSettings.NotifyAgHealth, agLagAlertSeconds: () => alertSettings.AgLagAlertSeconds, - agRedoQueueAlertKb: () => alertSettings.AgRedoQueueAlertKb); + agRedoQueueAlertKb: () => alertSettings.AgRedoQueueAlertKb, + agDisconnectRefireMinutes: () => alertSettings.AgDisconnectRefireMinutes); /* Phase-5 analysis slice AN3: the analysis pipeline's shared pieces, constructed once. The plan fetcher resolves a finding's serverId to the CONNECTED runtime's connection diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingAlertReader.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingAlertReader.cs index c34d794fe9..9004ddb7d5 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingAlertReader.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingAlertReader.cs @@ -131,7 +131,8 @@ public sealed record AlertSettingsReadRow( int ConnectionRefireMinutes, bool NotifyAgHealth, int AgLagAlertSeconds, - long AgRedoQueueAlertKb); + long AgRedoQueueAlertKb, + int AgDisconnectRefireMinutes); /// The single global alert-settings row (id=1) — the viewer's AlertSettingsSelectSql. The /// 41 columns are read in the SAME order the service reads them (StoreConfigProvider). This had @@ -149,7 +150,8 @@ public sealed record AlertSettingsReadRow( long_running_query_exclude_wait_for, long_running_query_exclude_backups, long_running_query_exclude_misc_waits, long_running_query_exclude_cdc, notify_connection_changes, notify_connection_down_at_startup, connection_refire_minutes, - notify_ag_health, ag_lag_alert_seconds, ag_redo_queue_alert_kb + notify_ag_health, ag_lag_alert_seconds, ag_redo_queue_alert_kb, + ag_disconnect_refire_minutes FROM config_alert_settings WHERE id = 1"; @@ -178,6 +180,7 @@ FROM config_alert_settings reader.GetBoolean(35), /* V33 (#1659) at 36-37, V35 (#991) at 38-40. */ reader.GetBoolean(36), reader.GetInt32(37), - reader.GetBoolean(38), reader.GetInt32(39), reader.GetInt64(40)); + reader.GetBoolean(38), reader.GetInt32(39), reader.GetInt64(40), + reader.GetInt32(41)); } } diff --git a/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs b/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs index a90802f4dd..0e4fdae8a2 100644 --- a/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs +++ b/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs @@ -165,9 +165,11 @@ INSERT INTO config_alert_settings ( long_running_query_exclude_wait_for, long_running_query_exclude_backups, long_running_query_exclude_misc_waits, long_running_query_exclude_cdc, notify_connection_changes, notify_connection_down_at_startup, connection_refire_minutes, - notify_ag_health, ag_lag_alert_seconds, ag_redo_queue_alert_kb, modified_at) + notify_ag_health, ag_lag_alert_seconds, ag_redo_queue_alert_kb, + ag_disconnect_refire_minutes, modified_at) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, - $22, $23, $24, $25, $26, $27, $28, $29, $30, $31, $32, $33, $34, $35, $36, $37, $38, $39, $40, $41, $42) + $22, $23, $24, $25, $26, $27, $28, $29, $30, $31, $32, $33, $34, $35, $36, $37, $38, $39, $40, $41, $42, + $43) ON CONFLICT (id) DO NOTHING", connection); command.Parameters.AddWithValue(a.Enabled); command.Parameters.AddWithValue(a.CpuEnabled); @@ -215,6 +217,8 @@ INSERT INTO config_alert_settings ( command.Parameters.AddWithValue(a.NotifyAgHealth); command.Parameters.AddWithValue(a.AgLagAlertSeconds); command.Parameters.AddWithValue(a.AgRedoQueueAlertKb); + /* V37 #1696: AG disconnect re-fire. */ + command.Parameters.AddWithValue(a.AgDisconnectRefireMinutes); command.Parameters.AddWithValue(now); await command.ExecuteNonQueryAsync(ct); } @@ -359,7 +363,8 @@ backfilled at read time (BuildServerFromRow's bootstrap merge). */ long_running_query_exclude_wait_for, long_running_query_exclude_backups, long_running_query_exclude_misc_waits, long_running_query_exclude_cdc, notify_connection_changes, notify_connection_down_at_startup, connection_refire_minutes, - notify_ag_health, ag_lag_alert_seconds, ag_redo_queue_alert_kb + notify_ag_health, ag_lag_alert_seconds, ag_redo_queue_alert_kb, + ag_disconnect_refire_minutes FROM config_alert_settings WHERE id = 1", connection); using var reader = await command.ExecuteReaderAsync(ct); if (!await reader.ReadAsync(ct)) @@ -415,6 +420,8 @@ here without the columns present. */ NotifyAgHealth = reader.GetBoolean(38), AgLagAlertSeconds = reader.GetInt32(39), AgRedoQueueAlertKb = reader.GetInt64(40), + /* #1696 AG disconnect re-fire appended (V37) at ordinal 41. */ + AgDisconnectRefireMinutes = reader.GetInt32(41), }; var analysis = new AnalysisConfig { diff --git a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs index cf071a9deb..23f6b3913c 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs @@ -80,6 +80,7 @@ public Migration(int version, string name, string sql) new Migration(34, "availability-group-collectors", V34Sql), new Migration(35, "availability-group-alerts", V35Sql), new Migration(36, "ag-latency-columns", V36Sql), + new Migration(37, "ag-local-replica-and-disconnect-refire", V37Sql), }; /// @@ -586,6 +587,32 @@ ALTER TABLE collect.ag_database_replica_states ADD COLUMN IF NOT EXISTS est_redo_completion_time_min double precision, ADD COLUMN IF NOT EXISTS est_send_drain_time_min double precision;"; + /// + /// V37 — two additive columns for #1696, in ONE migration because they ship together. + /// + /// ag_replica_states.is_local marks the row describing the replica the collector was + /// connected to. Every replica in an AG is visible from every node, so a fully-monitored 3-node AG + /// collects the same replica's state three times and previously reported one failover three times. The + /// alert path uses this to pick one node's view. NULLABLE, unlike the settings column below: rows + /// collected before this migration genuinely do not know, and a NULL must read as "unknown" rather than + /// as "not local" — de-duplicating on a false negative would drop a real alert. The de-dup treats a + /// snapshot with no known-local row as un-de-duplicable and keeps every row, which is the safe direction. + /// + /// config_alert_settings.ag_disconnect_refire_minutes is the #1659 re-fire treatment for + /// "AG Replica Disconnected", which until now was a pure edge: a replica that stayed disconnected for a + /// week announced it once. NOT NULL DEFAULT 0 = off, so the shipped behavior is byte-for-byte the old + /// edge-only one and nothing starts re-alerting on upgrade. + /// + /// ADD COLUMN IF NOT EXISTS throughout, per the file's additive idiom; config.-qualified because the + /// migrate session runs under search_path = collect, config, public. + /// + private const string V37Sql = @" +ALTER TABLE collect.ag_replica_states + ADD COLUMN IF NOT EXISTS is_local boolean; + +ALTER TABLE config.config_alert_settings + ADD COLUMN IF NOT EXISTS ag_disconnect_refire_minutes integer NOT NULL DEFAULT 0;"; + /// /// V9 — the FinOps copy-parity fields that were user-input config or previously live-only: /// server_properties gains the three inventory columns the shared ServerPropertiesCollector now diff --git a/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs b/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs index 7bf326553b..f6db18fc6a 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs @@ -16,5 +16,5 @@ namespace PerformanceMonitor.Darling.Storage; /// public static class StorageVersion { - public const int SchemaVersion = 36; + public const int SchemaVersion = 37; } diff --git a/Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml b/Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml index bbbd58c86d..4232fb0fa1 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml +++ b/Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml @@ -264,6 +264,14 @@ + + + + + diff --git a/Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml.cs b/Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml.cs index 89b28089a1..8c1611ae36 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/SettingsWindow.xaml.cs @@ -664,6 +664,7 @@ private void SeedAlertControlsFrom(AlertSettingsRow r) NotifyAgHealthCheckBox.IsChecked = r.NotifyAgHealth; AgLagAlertSecondsBox.Text = r.AgLagAlertSeconds.ToString(CultureInfo.InvariantCulture); AgRedoQueueAlertKbBox.Text = r.AgRedoQueueAlertKb.ToString(CultureInfo.InvariantCulture); + AgDisconnectRefireMinutesBox.Text = r.AgDisconnectRefireMinutes.ToString(CultureInfo.InvariantCulture); AlertCpuCheckBox.IsChecked = r.CpuEnabled; AlertCpuThresholdBox.Text = r.CpuThresholdPercent.ToString(CultureInfo.InvariantCulture); AlertCpuModeBox.SelectedIndex = ViewerDataService.MapCpuModeFromStore(r.CpuMode) == "SqlOnly" ? 1 : 0; @@ -723,6 +724,8 @@ private AlertSettingsRow BuildAlertRowFromControls(List errors) ? Math.Clamp(agLag, 0, 86400) : 300, AgRedoQueueAlertKb = long.TryParse(AgRedoQueueAlertKbBox.Text, out var agRedo) ? Math.Clamp(agRedo, 0L, 1073741824L) : 0L, + AgDisconnectRefireMinutes = int.TryParse(AgDisconnectRefireMinutesBox.Text, out var agRefire) + ? Math.Clamp(agRefire, 0, 1440) : 0, CpuEnabled = AlertCpuCheckBox.IsChecked == true, CpuMode = ViewerDataService.MapCpuModeToStore((AlertCpuModeBox.SelectedItem as ComboBoxItem)?.Tag?.ToString() ?? "Total"), BlockingEnabled = AlertBlockingCheckBox.IsChecked == true, @@ -892,6 +895,7 @@ private void UpdateAlertControlStates() NotifyAgHealthCheckBox.IsEnabled = enabled; AgLagAlertSecondsBox.IsEnabled = enabled; AgRedoQueueAlertKbBox.IsEnabled = enabled; + AgDisconnectRefireMinutesBox.IsEnabled = enabled; AlertCpuCheckBox.IsEnabled = enabled; AlertCpuThresholdBox.IsEnabled = enabled; AlertCpuModeBox.IsEnabled = enabled; diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.AlertSettings.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.AlertSettings.cs index 802b8f0304..a777241b8e 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.AlertSettings.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.AlertSettings.cs @@ -39,7 +39,7 @@ namespace PerformanceMonitor.Darling.Viewer; /// public sealed partial class ViewerDataService { - /* The 41 AlertsConfig + AnalysisConfig columns in the SAME order the service reads them + /* The 42 AlertsConfig + AnalysisConfig columns in the SAME order the service reads them (StoreConfigProvider.ReadAlertSettingsAsync), so the parity test pins one list against both ends. delivery_mode/per_event_max (V18, #1141) then the six long-running-query read knobs + the connection-change notify toggle (V20) are appended so the existing ordinals stay pinned. */ @@ -55,7 +55,8 @@ notify toggle (V20) are appended so the existing ordinals stay pinned. */ "long_running_query_exclude_wait_for, long_running_query_exclude_backups, " + "long_running_query_exclude_misc_waits, long_running_query_exclude_cdc, notify_connection_changes, " + "notify_connection_down_at_startup, connection_refire_minutes, " + - "notify_ag_health, ag_lag_alert_seconds, ag_redo_queue_alert_kb"; + "notify_ag_health, ag_lag_alert_seconds, ag_redo_queue_alert_kb, " + + "ag_disconnect_refire_minutes"; /// The single global alert-settings row (id=1), for the Settings window prefill + the migrate-in /// defaults check. Column order matches . @@ -64,11 +65,11 @@ notify toggle (V20) are appended so the existing ordinals stay pinned. */ /// Upserts the single global alert-settings row (Settings window Save). ON CONFLICT rewrites every /// column and bumps modified_at (and, via the V17 statement trigger, config_version — the - /// service reloads on its next sweep). $1..$41 bind the columns in order. + /// service reloads on its next sweep). $1..$42 bind the columns in order. public const string AlertSettingsUpsertSql = @" INSERT INTO config_alert_settings (id, " + AlertSettingsColumns + @", modified_at) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, - $23, $24, $25, $26, $27, $28, $29, $30, $31, $32, $33, $34, $35, $36, $37, $38, $39, $40, $41, + $23, $24, $25, $26, $27, $28, $29, $30, $31, $32, $33, $34, $35, $36, $37, $38, $39, $40, $41, $42, (now() AT TIME ZONE 'UTC')) ON CONFLICT (id) DO UPDATE SET enabled = EXCLUDED.enabled, @@ -112,6 +113,7 @@ ON CONFLICT (id) DO UPDATE SET notify_ag_health = EXCLUDED.notify_ag_health, ag_lag_alert_seconds = EXCLUDED.ag_lag_alert_seconds, ag_redo_queue_alert_kb = EXCLUDED.ag_redo_queue_alert_kb, + ag_disconnect_refire_minutes = EXCLUDED.ag_disconnect_refire_minutes, modified_at = (now() AT TIME ZONE 'UTC')"; /// The two cpu_mode values the service honors (it compares case-insensitively against @@ -182,6 +184,7 @@ private static void BindAlertSettings(NpgsqlCommand command, AlertSettingsRow r) command.Parameters.Add(new NpgsqlParameter { TypedValue = r.NotifyAgHealth }); // $39 (#991, V35) command.Parameters.Add(new NpgsqlParameter { TypedValue = r.AgLagAlertSeconds }); // $40 (#991, V35) command.Parameters.Add(new NpgsqlParameter { TypedValue = r.AgRedoQueueAlertKb }); // $41 (#991, V35) + command.Parameters.Add(new NpgsqlParameter { TypedValue = r.AgDisconnectRefireMinutes }); // $42 (#1696, V37) } private static AlertSettingsRow ReadAlertSettingsRow(NpgsqlDataReader reader) => new() @@ -229,6 +232,8 @@ private static void BindAlertSettings(NpgsqlCommand command, AlertSettingsRow r) NotifyAgHealth = reader.GetBoolean(38), AgLagAlertSeconds = reader.GetInt32(39), AgRedoQueueAlertKb = reader.GetInt64(40), + /* #1696 AG disconnect re-fire appended (V37) at ordinal 41. */ + AgDisconnectRefireMinutes = reader.GetInt32(41), }; /// Maps the Settings window's CPU-mode combo tag ("Total"/"SqlOnly") to the store value. @@ -274,6 +279,9 @@ public sealed class AlertSettingsRow /// #991 (V35): "AG Sync Fell Behind" redo-queue trigger in KB (0 = off). public long AgRedoQueueAlertKb { get; set; } + /// #1696 (V37): re-announce a still-disconnected AG replica every N minutes (0 = off). + public int AgDisconnectRefireMinutes { get; set; } + public bool CpuEnabled { get; set; } = true; public int CpuThresholdPercent { get; set; } = 80; @@ -349,6 +357,7 @@ public bool ValueEquals(AlertSettingsRow other) && NotifyAgHealth == other.NotifyAgHealth && AgLagAlertSeconds == other.AgLagAlertSeconds && AgRedoQueueAlertKb == other.AgRedoQueueAlertKb + && AgDisconnectRefireMinutes == other.AgDisconnectRefireMinutes && CpuEnabled == other.CpuEnabled && CpuThresholdPercent == other.CpuThresholdPercent && string.Equals(CpuMode, other.CpuMode, StringComparison.OrdinalIgnoreCase) diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs index 8c451aaf75..ffd1fa818e 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs @@ -397,6 +397,7 @@ OR NOT EXISTS (SELECT 1 FROM pg_extension WHERE extname = 'timescaledb') EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'config_alert_settings' AND column_name = 'notify_connection_down_at_startup'), EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'ag_database_replica_states'), EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'config_alert_settings' AND column_name = 'notify_ag_health'), + EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'config_alert_settings' AND column_name = 'ag_disconnect_refire_minutes'), EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'ag_database_replica_states' AND column_name = 'est_send_drain_time_min')"; /// The store schema version this viewer build requires — the highest migration it knows @@ -418,7 +419,7 @@ OR NOT EXISTS (SELECT 1 FROM pg_extension WHERE extname = 'timescaledb') await using var reader = await command.ExecuteReaderAsync(cancellationToken); if (await reader.ReadAsync(cancellationToken)) { - return MapProbedSchemaVersion(reader.GetBoolean(0), reader.GetBoolean(1), reader.GetBoolean(2), reader.GetBoolean(3), reader.GetBoolean(4), reader.GetBoolean(5), reader.GetBoolean(6), reader.GetBoolean(7), reader.GetBoolean(8), reader.GetBoolean(9), reader.GetBoolean(10), reader.GetBoolean(11), reader.GetBoolean(12), reader.GetBoolean(13), reader.GetBoolean(14), reader.GetBoolean(15), reader.GetBoolean(16), reader.GetBoolean(17), reader.GetBoolean(18), reader.GetBoolean(19)); + return MapProbedSchemaVersion(reader.GetBoolean(0), reader.GetBoolean(1), reader.GetBoolean(2), reader.GetBoolean(3), reader.GetBoolean(4), reader.GetBoolean(5), reader.GetBoolean(6), reader.GetBoolean(7), reader.GetBoolean(8), reader.GetBoolean(9), reader.GetBoolean(10), reader.GetBoolean(11), reader.GetBoolean(12), reader.GetBoolean(13), reader.GetBoolean(14), reader.GetBoolean(15), reader.GetBoolean(16), reader.GetBoolean(17), reader.GetBoolean(18), reader.GetBoolean(19), reader.GetBoolean(20)); } return null; @@ -443,8 +444,15 @@ OR NOT EXISTS (SELECT 1 FROM pg_extension WHERE extname = 'timescaledb') /// is unit-tested without a live store; any schema bump past the newest arm trips the pinning test that keeps /// this in step with . /// - internal static int MapProbedSchemaVersion(bool hasConfigControlPlane, bool hasAlertDeliveryOverride, bool hasAnalysisState, bool hasAlertTuningKnobs, bool hasDefaultTraceEvents, bool hasIndexObjectStatsLatestIndex, bool hasCollectionLogHypertableOrPlainPg, bool hasJobHistory, bool hasAgentStatus, bool hasGenericWebhook, bool hasDeadlocksDatabaseName, bool hasQueryStoreReplicaRole, bool hasLongQueryCompletions, bool hasWebDashboardConfig, bool hasCustomViews, bool hasServerTags, bool hasConnectionRefireKnobs = false, bool hasAgCollectors = false, bool hasAgAlertKnobs = false, bool hasAgLatencyColumns = false) + internal static int MapProbedSchemaVersion(bool hasConfigControlPlane, bool hasAlertDeliveryOverride, bool hasAnalysisState, bool hasAlertTuningKnobs, bool hasDefaultTraceEvents, bool hasIndexObjectStatsLatestIndex, bool hasCollectionLogHypertableOrPlainPg, bool hasJobHistory, bool hasAgentStatus, bool hasGenericWebhook, bool hasDeadlocksDatabaseName, bool hasQueryStoreReplicaRole, bool hasLongQueryCompletions, bool hasWebDashboardConfig, bool hasCustomViews, bool hasServerTags, bool hasConnectionRefireKnobs = false, bool hasAgCollectors = false, bool hasAgAlertKnobs = false, bool hasAgLatencyColumns = false, bool hasAgDisconnectRefire = false) { + /* V37 (#1696 AG disconnect re-fire): engine-agnostic column-existence sentinel, newest-first arm. + config_alert_settings.ag_disconnect_refire_minutes exists only at V37 or later. */ + if (hasAgDisconnectRefire) + { + return 37; + } + /* V36 (#991 addendum, AG latency columns): engine-agnostic COLUMN-existence sentinel — V36 only widens the V34 table, so the table-existence arm below cannot distinguish the two. Newest-first. */ if (hasAgLatencyColumns) diff --git a/Lite.Tests/AgCollectorDefinitionTests.cs b/Lite.Tests/AgCollectorDefinitionTests.cs index b6f3a3cb2b..deb7636ae9 100644 --- a/Lite.Tests/AgCollectorDefinitionTests.cs +++ b/Lite.Tests/AgCollectorDefinitionTests.cs @@ -47,7 +47,8 @@ public sealed class AgCollectorDefinitionTests synchronization_health_desc = ars.synchronization_health_desc, availability_mode_desc = ar.availability_mode_desc, failover_mode_desc = ar.failover_mode_desc, - endpoint_url = ar.endpoint_url + endpoint_url = ar.endpoint_url, + is_local = ars.is_local FROM sys.availability_replicas AS ar JOIN sys.availability_groups AS ag ON ar.group_id = ag.group_id @@ -182,11 +183,19 @@ public void ReplicaPayloadColumns_AreInAppendOrder() "availability_mode_desc", "failover_mode_desc", "endpoint_url", + /* #1696: appended LAST, so an upgraded store's ALTER lands it in the same physical position + a fresh generated table puts it - which is what keeps the two provenances comparable. */ + "is_local", }, names); - Assert.All(AgReplicaStatesCollector.Instance.PayloadColumns, + /* Every state string is a Varchar; is_local is the one Boolean, and typing it as text would make the + migration's ALTER disagree with the generated fresh shape. */ + Assert.All(AgReplicaStatesCollector.Instance.PayloadColumns.Where(c => c.Name != "is_local"), c => Assert.Equal(CollectorColumnType.Varchar, c.Type)); + Assert.Equal( + CollectorColumnType.Boolean, + AgReplicaStatesCollector.Instance.PayloadColumns.Single(c => c.Name == "is_local").Type); } [Fact] @@ -267,8 +276,8 @@ whole collection cycle rather than one column. */ public async Task ReplicaReadAsync_MapsColumns() { using var reader = new FakeCollectorDataReader( - new object[] { "AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", "TCP://NODE1.corp:5022" }, - new object[] { "AG1", "NODE2", "SECONDARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", "TCP://NODE2.corp:5022" }); + new object[] { "AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", "TCP://NODE1.corp:5022", true }, + new object[] { "AG1", "NODE2", "SECONDARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", "TCP://NODE2.corp:5022", false }); var context = CollectorTestContext.Make(new RecordingCollectorDeltaCalculator()); @@ -276,10 +285,14 @@ public async Task ReplicaReadAsync_MapsColumns() Assert.Equal(2, rows.Count); Assert.Equal( - new AgReplicaStatesCollector.Row("AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", "TCP://NODE1.corp:5022"), + new AgReplicaStatesCollector.Row("AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", "TCP://NODE1.corp:5022", true), rows[0]); Assert.Equal("NODE2", rows[1].ReplicaServerName); Assert.Equal("SECONDARY", rows[1].RoleDesc); + /* #1696: is_local marks which row is the connected replica's own, so the alert path can pick ONE + node's view of an AG that every node can see. */ + Assert.True(rows[0].IsLocal); + Assert.False(rows[1].IsLocal); } [Fact] @@ -289,8 +302,8 @@ public async Task ReplicaReadAsync_ToleratesNulls() state sys.availability_replicas serves only locally cached metadata — so every column can be null. A quorum-loss read must produce a row, not an InvalidCastException. */ using var reader = new FakeCollectorDataReader( - new object[] { "AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", DBNull.Value }, - new object[] { DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value }); + new object[] { "AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", DBNull.Value, DBNull.Value }, + new object[] { DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value, DBNull.Value }); var context = CollectorTestContext.Make(new RecordingCollectorDeltaCalculator()); @@ -298,6 +311,10 @@ state sys.availability_replicas serves only locally cached metadata — so every Assert.Equal(2, rows.Count); Assert.Null(rows[0].EndpointUrl); + /* #1696: is_local must read NULL under quorum loss, never false — the de-dup treats unknown as + "cannot rank this vantage" and keeps judging, whereas a false would silently demote a real one. */ + Assert.Null(rows[0].IsLocal); + Assert.Null(rows[1].IsLocal); Assert.Equal("AG1", rows[0].AgName); Assert.Equal(default(AgReplicaStatesCollector.Row), rows[1]); } @@ -416,12 +433,12 @@ public void WritePayload_EmitsPayloadOrder_AndTakesNoDeltas() var replicaWriter = new RecordingCollectorRowWriter(); AgReplicaStatesCollector.Instance.WritePayload( - new AgReplicaStatesCollector.Row("AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", null), + new AgReplicaStatesCollector.Row("AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", null, true), replicaWriter, context); Assert.Equal( - new object?[] { "AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", null }, + new object?[] { "AG1", "NODE1", "PRIMARY", "ONLINE", "CONNECTED", "ONLINE", "HEALTHY", "SYNCHRONOUS_COMMIT", "AUTOMATIC", null, true }, replicaWriter.Values); /* Modeled on a real measured sample from the Docker AG fixture: a SUSPEND_FROM_USER replica 62 s diff --git a/Lite.Tests/GoldenCollectorSchema.cs b/Lite.Tests/GoldenCollectorSchema.cs index d63afdb9a1..67efc2301f 100644 --- a/Lite.Tests/GoldenCollectorSchema.cs +++ b/Lite.Tests/GoldenCollectorSchema.cs @@ -859,7 +859,8 @@ next_scheduled_run TIMESTAMP synchronization_health_desc VARCHAR, availability_mode_desc VARCHAR, failover_mode_desc VARCHAR, - endpoint_url VARCHAR + endpoint_url VARCHAR, + is_local BOOLEAN )", ["ag_database_replica_states"] = @"CREATE TABLE IF NOT EXISTS ag_database_replica_states ( collection_id BIGINT PRIMARY KEY, diff --git a/PerformanceMonitor.Collectors/AgReplicaStatesCollector.cs b/PerformanceMonitor.Collectors/AgReplicaStatesCollector.cs index f698abf60a..c75bbc4b11 100644 --- a/PerformanceMonitor.Collectors/AgReplicaStatesCollector.cs +++ b/PerformanceMonitor.Collectors/AgReplicaStatesCollector.cs @@ -28,6 +28,12 @@ namespace PerformanceMonitor.Collectors; /// exist on every non-Azure-SQL-DB instance whether or not Always On is enabled, so an AG-less /// server returns an empty set rather than erroring, and the host records SUCCESS/0 rows. /// +/// is_local (#1696) marks the row describing the replica this collector is connected to. Every +/// replica in an AG is visible from every node, so a fully-monitored 3-node AG collects the same replica's +/// state three times; the alert path uses this flag to pick ONE node's view and report a failover once +/// instead of three times. It comes from the DMV rather than being inferred, because matching the monitored +/// server's name against replica_server_name is not reliable (instance names, aliases, listeners). +/// /// WHERE COALESCE(ag.is_distributed, 0) = 0 keeps distributed-AG container rows out; /// the member AGs themselves still appear. Drilling into a DAG's remote members /// (sys.fn_hadr_distributed_ag_replica) needs a connection per member and is out of scope. @@ -65,7 +71,8 @@ public readonly record struct Row( string? SynchronizationHealthDesc, string? AvailabilityModeDesc, string? FailoverModeDesc, - string? EndpointUrl); + string? EndpointUrl, + bool? IsLocal); private const string QueryText = @" SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; @@ -80,7 +87,8 @@ public readonly record struct Row( synchronization_health_desc = ars.synchronization_health_desc, availability_mode_desc = ar.availability_mode_desc, failover_mode_desc = ar.failover_mode_desc, - endpoint_url = ar.endpoint_url + endpoint_url = ar.endpoint_url, + is_local = ars.is_local FROM sys.availability_replicas AS ar JOIN sys.availability_groups AS ag ON ar.group_id = ag.group_id @@ -122,6 +130,7 @@ ORDER BY new CollectorColumn("availability_mode_desc", CollectorColumnType.Varchar), new CollectorColumn("failover_mode_desc", CollectorColumnType.Varchar), new CollectorColumn("endpoint_url", CollectorColumnType.Varchar), + new CollectorColumn("is_local", CollectorColumnType.Boolean), }; public override async ValueTask> ReadAsync(DbDataReader reader, CollectorContext context, CancellationToken cancellationToken) @@ -140,7 +149,8 @@ public override async ValueTask> ReadAsync(DbDataReader reader, Collec SynchronizationHealthDesc: reader.IsDBNull(6) ? null : reader.GetString(6), AvailabilityModeDesc: reader.IsDBNull(7) ? null : reader.GetString(7), FailoverModeDesc: reader.IsDBNull(8) ? null : reader.GetString(8), - EndpointUrl: reader.IsDBNull(9) ? null : reader.GetString(9))); + EndpointUrl: reader.IsDBNull(9) ? null : reader.GetString(9), + IsLocal: reader.IsDBNull(10) ? null : reader.GetBoolean(10))); } return rows; @@ -158,6 +168,7 @@ public override void WritePayload(Row row, ICollectorRowWriter writer, Collector .Value(row.SynchronizationHealthDesc) /* synchronization_health_desc VARCHAR */ .Value(row.AvailabilityModeDesc) /* availability_mode_desc VARCHAR */ .Value(row.FailoverModeDesc) /* failover_mode_desc VARCHAR */ - .Value(row.EndpointUrl); /* endpoint_url VARCHAR */ + .Value(row.EndpointUrl) /* endpoint_url VARCHAR */ + .Value(row.IsLocal); /* is_local BOOLEAN */ } } diff --git a/PerformanceMonitor.Common/AgAlertPolicy.cs b/PerformanceMonitor.Common/AgAlertPolicy.cs index 36e169ca96..e5f9e00d51 100644 --- a/PerformanceMonitor.Common/AgAlertPolicy.cs +++ b/PerformanceMonitor.Common/AgAlertPolicy.cs @@ -7,6 +7,7 @@ */ using System; +using System.Collections.Generic; using System.Globalization; namespace PerformanceMonitor.Common; @@ -80,6 +81,26 @@ public enum AgSuspensionDecision Resumed, } +/// How good one monitored server's view of a given Availability Group is (#1696). Every replica in +/// an AG is visible from every node, so a fully-monitored 3-node AG yields three views of the same replica — +/// and judging all three reports one failover three times. Ranked so the best available view wins. +public enum AgVantage +{ + /// This server's snapshot says nothing about the AG. + None, + + /// The AG appears, but no row is flagged local — either a pre-#1696 store whose rows predate + /// the is_local column, or a genuinely remote-only view. Usable, but the weakest evidence. + Remote, + + /// This server is one of the AG's replicas, but not its primary. + Local, + + /// This server is the AG's PRIMARY. The best vantage by a distance: a SECONDARY's + /// sys.dm_hadr_* carries only its OWN row, so only the primary sees the whole group. + LocalPrimary, +} + /// /// The single definition of when an Availability Group alert fires, shared by Darling's headless self-alert /// evaluator and Lite's alert path (#991/#1696) — the discipline: pure @@ -137,6 +158,53 @@ public static class AgAlertPolicy public static bool IsDisconnected(string? connectedStateDesc) => string.Equals(connectedStateDesc, DisconnectedState, StringComparison.OrdinalIgnoreCase); + /// + /// How good this snapshot's view of is (#1696), for choosing ONE monitored + /// server to judge each Availability Group from. + /// + /// Without this, a fully-monitored 3-node AG reports every failover three times — once per node + /// that can see it. Preferring the PRIMARY's view is not just tie-breaking: measured on a live AG, a + /// SECONDARY's sys.dm_hadr_* returns only that replica's OWN row, so only the primary actually + /// sees the whole group. A secondary-only vantage is a one-row self-view. + /// + /// A row whose IsLocal is NULL counts as , never as "not + /// local": rows collected before the column existed genuinely do not know, and treating unknown as + /// not-local would let a real vantage lose to nothing and drop the alert entirely. The safe direction is + /// to keep judging. + /// + public static AgVantage ClassifyVantage(IReadOnlyList? snapshot, string agName) + { + if (snapshot is null) + { + return AgVantage.None; + } + + var best = AgVantage.None; + foreach (var replica in snapshot) + { + if (!string.Equals(replica.AgName, agName, StringComparison.Ordinal)) + { + continue; + } + + if (replica.IsLocal == true) + { + if (string.Equals(replica.RoleDesc, "PRIMARY", StringComparison.OrdinalIgnoreCase)) + { + return AgVantage.LocalPrimary; + } + + best = AgVantage.Local; + } + else if (best == AgVantage.None) + { + best = AgVantage.Remote; + } + } + + return best; + } + /// /// Whether a replica's role changed since the previous sweep — the "AG Failover" trigger. Any change /// counts, not just PRIMARY↔SECONDARY: a move into RESOLVING is a failover in progress and is worth diff --git a/deprecated/Installer/packages.lock.json b/deprecated/Installer/packages.lock.json index af333d919e..39ad2a4295 100644 --- a/deprecated/Installer/packages.lock.json +++ b/deprecated/Installer/packages.lock.json @@ -22,9 +22,9 @@ }, "Microsoft.NET.ILLink.Tasks": { "type": "Direct", - "requested": "[10.0.8, )", - "resolved": "10.0.8", - "contentHash": "dVbSXGIFNR5nZcv2tOLoWI+a9T4jtFd77IYjuND+QVe360qWgAF7H0WtoopYhRw/+SgpGUTyrkrh+65+ClNnfw==" + "requested": "[10.0.10, )", + "resolved": "10.0.10", + "contentHash": "f5VCIE7AJpd5YvzNTeMGVzQIgyE9tX+AreTYwQF+REbu+DZo/2Ae+jNSwhPEYrVz6RRkd7y8ubXjk6Nn6Ka+Cg==" }, "Azure.Core": { "type": "Transitive", From f6d5a78ed9c9c097da8f37e955be59951978a7bb Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sun, 26 Jul 2026 18:57:23 -0400 Subject: [PATCH 2/2] Add the CHANGELOG entry for #1734 Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d70b3853f6..97d017b096 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Darling: an AG failover is reported once, not once per monitored node, and a standing replica disconnect can re-alert** ([#1734]) - completes #1696. **Cross-node de-duplication:** every replica in an Availability Group is visible from every node, so a fully-monitored 3-node AG collected the same replica three times and reported one failover THREE times. `ag_replica_states` gains `is_local` (read from the DMV rather than inferred - matching a monitored server against `replica_server_name` is unreliable across instance names, aliases and listeners) and one server now judges each AG. Which one is not arbitrary: a SECONDARY's `sys.dm_hadr_*` carries only its OWN row, measured on the live fixture, so only the primary sees the whole group - vantage is ranked None < Remote < Local < LocalPrimary, the best available wins, a secondary yields to the primary as soon as the primary is monitored, and ties keep the incumbent so authority cannot oscillate. The AG edge state drops the server id from its key as part of this, because an AG is ONE object however many of its nodes are watched; that is what lets authority move without re-baselining and losing an alert, or double-firing one. Removing a server now releases its CLAIM rather than dropping the group's state, since another node may still be watching it. A NULL `is_local` reads as "unknown" and never as "not local" - rows predating the migration genuinely do not know, and de-duplicating on a false negative would drop a real alert. **Disconnect re-fire:** "AG Replica Disconnected" was a pure edge, so a replica disconnected for a week announced it once; `ag_disconnect_refire_minutes` (default 0 = off, clamped 0-1440) re-announces on the [#1674] pattern - same metric name so webhook automation keyed on it re-triggers, stamped on DELIVERY only so a suppressed alert cannot consume the window, and cleared on reconnect. Both columns ride store migration V37. + - **Lite gets the Availability Group alerts, off a shared policy** ([#1726]) - Lite has collected both AG grains since [#1688] but could not tell you a replica had failed over, disconnected, fallen behind, or had data movement suspended; only Darling could. Lite now raises the same four conditions under the SAME metric names, so a webhook keyed on `AG Failover` matches whichever app sent it. The rules are shared rather than reimplemented: a new `PerformanceMonitor.Common.AgAlertPolicy` holds the readings, the metric-name consts and every pure decision, on the `ConnectionAlertPolicy` pattern - each app owns only its own edge STATE and its own delivery, which is the part that is genuinely app-shaped. Darling was refactored onto it with no behavior change (its whole suite passes untouched, which is why the lift was done as its own step). Lite reads the latest snapshot of each grain from DuckDB, each freshness-gated on its OWN collection time because the two AG collectors are scheduled independently and a stale database-grain snapshot must not be vouched for by a healthy replica-grain one; the evaluator is WPF-free so it pins directly, and delivery goes through the same mute-check and send path every other Lite alert uses, inheriting muting, silencing, the combined history row and the email/webhook fan-out. Three settings mirror Darling's V35 knobs (`notify_ag_health` on, `ag_lag_alert_seconds` 300, `ag_redo_queue_alert_kb` 0 = off), clamped to the same ranges on load AND on save so the stored value and the effective value cannot disagree. Server removal drops the AG state, or a remove-then-re-add would compare the new first sighting against the old role and page a phantom failover. Every rule the earlier AG work paid for carries over: a first sighting is a silent baseline, NULL is never a transition, and a suspended row may raise an alarm but may never clear one - including that a suspended secondary drifting past the threshold still fires. Lite's collector-coverage ratchet went red on this exactly as designed - both AG tables were allow-listed as collect-only, and adding a reader forced the entries out - so that allow-list is now empty. - **Availability Groups tab in the WPF viewer** ([#1722]) - the AG topology on the client's primary surface, completing #991's reader half. The desktop twin of the web dashboard's Availability Groups page: one card per AG with per-replica chips (role, connected / operational state, synchronization health, availability + failover mode, endpoint on hover) over a database grid carrying synchronization state, log-send and redo queue sizes, send / redo rates, secondary lag, derived drain estimates, and the suspend reason when data movement has stopped. Every severity is computed in the reader and the XAML only binds the brush it produced, so the tab cannot drift from the web page's verdict. **One card per (reporting server, AG), deliberately not merged** - the same rule the web page follows, and the live AG fixture shows exactly why: the primary reports both replicas and names the primary, while the secondary reports ONE replica and no primary at all, because `sys.dm_hadr_availability_replica_states` returns only local information when queried on a secondary. Collapsing those two views would let the blind one overwrite the complete one, so the header states groups, reporting servers and views as three separate counts rather than leaving a reader to wonder why one AG name appears twice. The tab ships hidden and reveals itself once a sweep finds AG rows (Always On is opt-in; most fleets would otherwise carry a permanently empty tab), converging without a restart and never vanishing again mid-look if a later sweep reads zero. The reader is a COPY of the service's `DarlingAgReader` rather than a reference - the viewer has no ProjectReference to the headless service - so the banding rules are duplicated deliberately and pinned by tests on both sides to keep them honest; the viewer's copy reads bare table names (search_path resolves `collect`) and joins the enabled registry so a disabled server's AGs leave with it. This also retires the last two entries in the viewer-coverage ratchet: `ag_replica_states` and `ag_database_replica_states` were carried as tracked debt when #991 shipped collection-only, and the pin that tracked them is the same pin that now certifies the surface exists. Lite gets no AG tab in this pass - it is a single-server app and the fleet view is Darling's job. @@ -1715,6 +1717,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1716]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1716 [#1710]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1710 [#1726]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1726 +[#1734]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1734 [#1725]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1725 [#1730]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1730 [#1727]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1727