diff --git a/Darling/Darling.Tests/AnalysisFindingsTabTests.cs b/Darling/Darling.Tests/AnalysisFindingsTabTests.cs index 063c759039..13a858e110 100644 --- a/Darling/Darling.Tests/AnalysisFindingsTabTests.cs +++ b/Darling/Darling.Tests/AnalysisFindingsTabTests.cs @@ -140,6 +140,17 @@ public void TheReadCapNote_AndThePageNote_AreBothShown() Assert.Contains("PAGE-NOTE", text, StringComparison.Ordinal); } + [Fact] + public void TheSecondaryReplicaNote_ShowsWithFindingsAndWithNone_AndIsAbsentWhenTheReadCarriesNone() + { + var r = Run("replicaNote"); + Assert.Contains("REPLICA-NOTE", r.GetProperty("withFindings").GetString(), StringComparison.Ordinal); + Assert.Contains("REPLICA-NOTE", r.GetProperty("noFindings").GetString(), StringComparison.Ordinal); + Assert.DoesNotContain("REPLICA-NOTE", r.GetProperty("without").GetString(), StringComparison.Ordinal); + Assert.DoesNotContain("REPLICA-NOTE", r.GetProperty("nullNote").GetString(), StringComparison.Ordinal); + Assert.DoesNotContain("null", r.GetProperty("nullNote").GetString(), StringComparison.Ordinal); + } + [Fact] public void TheTabsAbortSignal_ReachesTheRead() { diff --git a/Darling/Darling.Tests/DarlingMcpFinOpsRecommendationsToolsTests.cs b/Darling/Darling.Tests/DarlingMcpFinOpsRecommendationsToolsTests.cs index 88a11f0e22..92f25297b2 100644 --- a/Darling/Darling.Tests/DarlingMcpFinOpsRecommendationsToolsTests.cs +++ b/Darling/Darling.Tests/DarlingMcpFinOpsRecommendationsToolsTests.cs @@ -88,7 +88,7 @@ public void Envelope_WithNoMonthlyCost_NullsTheCostAndSaysWhy() { var env = Json(DarlingMcpFinOpsRecommendationsTools.Envelope("alpha", 0m, ["Storage tier"], [Rec(null, "B"), Rec(1m, "A")])); Assert.Equal( - new[] { "server", "monthly_cost_usd", "cost_reason", "recommendation_count", "skipped_checks", "recommendations" }, + new[] { "server", "monthly_cost_usd", "cost_reason", "recommendation_count", "skipped_checks", "secondary_replica_note", "recommendations" }, env.EnumerateObject().Select(p => p.Name).ToArray()); Assert.Equal("alpha", env.GetProperty("server").GetString()); Assert.Equal(JsonValueKind.Null, env.GetProperty("monthly_cost_usd").ValueKind); @@ -98,6 +98,24 @@ public void Envelope_WithNoMonthlyCost_NullsTheCostAndSaysWhy() Assert.Equal(new[] { "B", "A" }, env.GetProperty("recommendations").EnumerateArray().Select(e => e.GetProperty("category").GetString()).ToArray()); } + [Fact] + public void Envelope_WithASecondaryNote_WritesItBetweenSkippedChecksAndTheRows_AndWithoutOneWritesNull() + { + var note = AgReplicaScope.SkippedNote(2); + var with = Json(DarlingMcpFinOpsRecommendationsTools.Envelope("alpha", 1000m, [], [Rec(1m, "A")], note)); + Assert.Equal( + new[] { "server", "monthly_cost_usd", "cost_reason", "recommendation_count", "skipped_checks", "secondary_replica_note", "recommendations" }, + with.EnumerateObject().Select(p => p.Name).ToArray()); + Assert.Equal(note, with.GetProperty("secondary_replica_note").GetString()); + + /* #5558: one convention across every MCP note: the key is always there, null when nothing was skipped. */ + var without = Json(DarlingMcpFinOpsRecommendationsTools.Envelope("alpha", 1000m, [], [Rec(1m, "A")], null)); + Assert.Equal(JsonValueKind.Null, without.GetProperty("secondary_replica_note").ValueKind); + Assert.Equal( + new[] { "server", "monthly_cost_usd", "cost_reason", "recommendation_count", "skipped_checks", "secondary_replica_note", "recommendations" }, + without.EnumerateObject().Select(p => p.Name).ToArray()); + } + [Fact] public void Envelope_WithAMonthlyCost_CarriesItAndNoReason() { diff --git a/Darling/Darling.Tests/FinOpsRecommendationsReadsLiveTests.cs b/Darling/Darling.Tests/FinOpsRecommendationsReadsLiveTests.cs index 4054ce1289..79d9f4b3eb 100644 --- a/Darling/Darling.Tests/FinOpsRecommendationsReadsLiveTests.cs +++ b/Darling/Darling.Tests/FinOpsRecommendationsReadsLiveTests.cs @@ -332,7 +332,7 @@ public async Task Composer_ReportsAFailedCheck_ToTheHook_AndKeepsTheRest() { Assert.NotNull(ex); failed.Add(label); - }, ct); + }, cancellationToken: ct); Assert.Equal(new[] { "Maintenance window" }, failed.ToArray()); Func key = r => $"{r.Severity}|{r.Category}|{r.Finding}|{r.Detail}|{r.EstMonthlySavings}"; @@ -404,7 +404,7 @@ public async Task Composer_DoesNotReportTheVmCpuFallback_WhileTheVmRowsStillEmit var failed = new System.Collections.Generic.List(); var after = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( - dataSource, idA, 1000m, TimeoutSeconds, (label, _) => failed.Add(label), ct); + dataSource, idA, 1000m, TimeoutSeconds, (label, _) => failed.Add(label), cancellationToken: ct); Assert.DoesNotContain("VM right-sizing", failed); Assert.Contains(after, r => r.Category == "Hardware" && r.Finding.StartsWith("CPU:", StringComparison.Ordinal)); diff --git a/Darling/Darling.Tests/FinOpsRecommendationsSecondaryReplicaLiveTests.cs b/Darling/Darling.Tests/FinOpsRecommendationsSecondaryReplicaLiveTests.cs new file mode 100644 index 0000000000..19bd10a64a --- /dev/null +++ b/Darling/Darling.Tests/FinOpsRecommendationsSecondaryReplicaLiveTests.cs @@ -0,0 +1,199 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Service.Mcp; +using PerformanceMonitor.Darling.Storage; +using PerformanceMonitor.Darling.Storage.FinOps; +using PerformanceMonitor.Darling.Viewer; +using Xunit; + +namespace Darling.Tests; + +/* #1776 own-store: every test seeds its own scratch database, so nothing here shares rows with another test. */ +/// +/// #5558: the FinOps Recommendations list leaves the per-database findings of a database this node holds only as a +/// secondary copy in an Availability Group to the primary. Over the golden store, server A has an idle database +/// (ArchiveA), two dev/test names (app_dev_a, qa1_a), two TDE databases (SalesA, OrdersA), a 2 GB uncompressed index in +/// SalesA, and a low-latency database (OrdersA). Each test seeds the stored Availability Group snapshots for server A, +/// reads through the storage composer, the viewer and the MCP tool, and compares with the same read over no snapshots. +/// +public sealed class FinOpsRecommendationsSecondaryReplicaLiveTests +{ + private const int TimeoutSeconds = 30; + private const string BaseSkip = "Set DARLING_TEST_PG to a Postgres connection string to run the live FinOps secondary replica test."; + + private static int ServerId => FinOpsRecommendationsGoldenLiveTests.ServerIdA; + + private static async Task SeedStoreAsync(string baseConnectionString, CancellationToken ct) + { + var scratch = await ScratchPostgres.CreateAsync(baseConnectionString, ct); + await using var connection = new NpgsqlConnection(scratch.ConnectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + await FinOpsRecommendationsGoldenLiveTests.SeedAsync(connection, ct); + return scratch; + } + + /// One AG whose local replica has , holding ; snapshots taken ago. + private static async Task SeedAgAsync(string connectionString, string localRole, TimeSpan age, CancellationToken ct, params string[] databases) + { + await using var connection = new NpgsqlConnection(connectionString); + await connection.OpenAsync(ct); + var at = DateTime.SpecifyKind(DateTime.UtcNow - age, DateTimeKind.Unspecified); + await DarlingMcpTestData.ExecAsync(connection, ct, + "INSERT INTO ag_replica_states (collection_id, collection_time, server_id, server_name, ag_name, replica_server_name, role_desc, is_local) VALUES ($1,$2,$3,$4,'AG1','NODE1',$5,TRUE)", + CollectionIdGenerator.Next(), at, ServerId, FinOpsRecommendationsGoldenLiveTests.ServerNameA, localRole); + foreach (var database in databases) + await DarlingMcpTestData.ExecAsync(connection, ct, + "INSERT INTO ag_database_replica_states (collection_id, collection_time, server_id, server_name, ag_name, database_name, replica_server_name, is_local) VALUES ($1,$2,$3,$4,'AG1',$5,'NODE1',TRUE)", + CollectionIdGenerator.Next(), at, ServerId, FinOpsRecommendationsGoldenLiveTests.ServerNameA, database); + } + + private static FinOpsRecommendation? Find(IEnumerable rows, string findingContains) => + rows.FirstOrDefault(r => r.Finding.Contains(findingContains, StringComparison.Ordinal)); + + [Fact] + public async Task ASecondaryCopy_IsDroppedFromIdleDevTestTdeAndCompression_ButKeptInLowIoLatency() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var dataSource = NpgsqlDataSource.Create(scratch.ConnectionString); + + var baseline = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync(dataSource, ServerId, 1000m, TimeoutSeconds, cancellationToken: ct); + Assert.NotNull(Find(baseline, "idle database")); + Assert.Contains("2 possible dev/test", Find(baseline, "possible dev/test")!.Finding, StringComparison.Ordinal); + Assert.Contains("SalesA", Find(baseline, "TDE in use")!.Detail, StringComparison.Ordinal); + Assert.Contains("OrdersA", Find(baseline, "TDE in use")!.Detail, StringComparison.Ordinal); + Assert.NotNull(Find(baseline, "uncompressed object")); + Assert.Contains("OrdersA", Find(baseline, "low IO latency")!.Detail, StringComparison.Ordinal); + + /* ArchiveA (idle), app_dev_a (dev/test) and SalesA (TDE and the 2 GB index) are secondary copies. Names are matched + exactly (#5558 round 2): the set and the stored names both come from sys.databases.name, so the casing is shared. */ + var secondary = new HashSet(StringComparer.Ordinal) { "ArchiveA", "app_dev_a", "SalesA" }; + var rows = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( + dataSource, ServerId, 1000m, TimeoutSeconds, secondaryDatabases: secondary, cancellationToken: ct); + + Assert.Null(Find(rows, "idle database")); + Assert.Contains("1 possible dev/test", Find(rows, "possible dev/test")!.Finding, StringComparison.Ordinal); + Assert.DoesNotContain("app_dev_a", Find(rows, "possible dev/test")!.Detail, StringComparison.Ordinal); + var tde = Find(rows, "TDE in use")!; + Assert.Contains("OrdersA", tde.Detail, StringComparison.Ordinal); + Assert.DoesNotContain("SalesA", tde.Detail, StringComparison.Ordinal); + Assert.Null(Find(rows, "uncompressed object")); + /* Low IO latency is unchanged, and so is every finding that is not about one database. */ + Assert.Equal(Find(baseline, "low IO latency")!.Detail, Find(rows, "low IO latency")!.Detail); + static bool PerDatabase(FinOpsRecommendation r) => + r.Finding.Contains("idle database", StringComparison.Ordinal) || r.Finding.Contains("possible dev/test", StringComparison.Ordinal) + || r.Finding.Contains("TDE in use", StringComparison.Ordinal) || r.Finding.Contains("uncompressed object", StringComparison.Ordinal); + Assert.Equal( + baseline.Where(r => !PerDatabase(r)).Select(r => (r.Finding, r.Detail)).ToArray(), + rows.Where(r => !PerDatabase(r)).Select(r => (r.Finding, r.Detail)).ToArray()); + } + + [Fact] + public async Task EveryTdeDatabaseASecondary_SaysNothingAboutTdeAndNeverClaimsNoEnterpriseFeatures() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var dataSource = NpgsqlDataSource.Create(scratch.ConnectionString); + + var rows = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( + dataSource, ServerId, 1000m, TimeoutSeconds, secondaryDatabases: new[] { "SalesA", "OrdersA" }, cancellationToken: ct); + + Assert.DoesNotContain(rows, r => r.Category == "Licensing"); + Assert.NotNull(Find(rows, "low IO latency")); + } + + [Fact] + public async Task NullOrEmptySet_ChangesNothing() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var dataSource = NpgsqlDataSource.Create(scratch.ConnectionString); + + var baseline = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync(dataSource, ServerId, 1000m, TimeoutSeconds, cancellationToken: ct); + var empty = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( + dataSource, ServerId, 1000m, TimeoutSeconds, secondaryDatabases: new HashSet(), cancellationToken: ct); + + Assert.Equal(baseline.Select(r => (r.Finding, r.Detail)).ToArray(), empty.Select(r => (r.Finding, r.Detail)).ToArray()); + } + + [Fact] + public async Task TheViewerAndTheMcpTool_ReadTheStoredSnapshots_AndFailOpenWhenTheyAreStaleOrTheNodeIsPrimary() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var dataSource = NpgsqlDataSource.Create(scratch.ConnectionString); + await using var viewer = new ViewerDataService(scratch.ConnectionString); + + /* No snapshots: no note, and the MCP payload carries the field as null. */ + var (plainRows, plainNote) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + Assert.Null(plainNote); + var plainRead = await FinOpsAsync(dataSource, ct); + Assert.Equal(System.Text.Json.JsonValueKind.Null, plainRead.GetProperty("secondary_replica_note").ValueKind); + + /* A stale secondary snapshot (a collector that stopped hours ago) vouches for nothing. */ + await SeedAgAsync(scratch.ConnectionString, "SECONDARY", TimeSpan.FromHours(6), ct, "ArchiveA"); + var (staleRows, staleNote) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + Assert.Null(staleNote); + Assert.Equal(plainRows.Select(r => r.Finding).ToArray(), staleRows.Select(r => r.Finding).ToArray()); + + /* A fresh secondary snapshot of ArchiveA drops its idle finding on both surfaces and writes the note. */ + await SeedAgAsync(scratch.ConnectionString, "SECONDARY", TimeSpan.FromMinutes(2), ct, "ArchiveA"); + var (rows, note) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + Assert.Equal(AgReplicaScope.SkippedNote(1), note); + Assert.DoesNotContain(rows, r => r.Finding.Contains("idle database", StringComparison.Ordinal)); + Assert.Contains(plainRows, r => r.Finding.Contains("idle database", StringComparison.Ordinal)); + + var payload = await FinOpsAsync(dataSource, ct); + Assert.Equal(AgReplicaScope.SkippedNote(1), payload.GetProperty("secondary_replica_note").GetString()); + Assert.DoesNotContain(payload.GetProperty("recommendations").EnumerateArray(), + r => r.GetProperty("finding").GetString()!.Contains("idle database", StringComparison.Ordinal)); + } + + [Fact] + public async Task OnThePrimary_NothingIsSkipped_AndThereIsNoNote() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var viewer = new ViewerDataService(scratch.ConnectionString); + + var (plainRows, _) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + await SeedAgAsync(scratch.ConnectionString, "PRIMARY", TimeSpan.FromMinutes(2), ct, "ArchiveA", "app_dev_a", "SalesA"); + var (rows, note) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + + Assert.Null(note); + Assert.Equal(plainRows.Select(r => (r.Finding, r.Detail)).ToArray(), rows.Select(r => (r.Finding, r.Detail)).ToArray()); + } + + private static async Task FinOpsAsync(NpgsqlDataSource dataSource, CancellationToken ct) + { + var json = await DarlingMcpFinOpsRecommendationsTools.GetFinOpsRecommendations( + dataSource, FinOpsRecommendationsGoldenLiveTests.ServerNameA, ct); + return JsonDocument.Parse(json).RootElement.Clone(); + } +} diff --git a/Darling/Darling.Tests/LiteAnalysisServiceCancellationSourceTests.cs b/Darling/Darling.Tests/LiteAnalysisServiceCancellationSourceTests.cs index 5fb5d43b6c..7d2d9a4532 100644 --- a/Darling/Darling.Tests/LiteAnalysisServiceCancellationSourceTests.cs +++ b/Darling/Darling.Tests/LiteAnalysisServiceCancellationSourceTests.cs @@ -39,7 +39,7 @@ public sealed class LiteAnalysisServiceCancellationSourceTests private const string CollectConfigAuditFactsAsyncSignature = "public async Task> CollectConfigAuditFactsAsync("; private const string ComparePeriodsAsyncSignature = - "public async Task<(List BaselineFacts, List ComparisonFacts, WindowCoverage? BaselineCoverage, WindowCoverage? ComparisonCoverage, IReadOnlyDictionary Dispersion)> ComparePeriodsAsync("; + "public async Task<(List BaselineFacts, List ComparisonFacts, WindowCoverage? BaselineCoverage, WindowCoverage? ComparisonCoverage, IReadOnlyDictionary Dispersion, IReadOnlySet? BaselineSecondaries, IReadOnlySet? ComparisonSecondaries)> ComparePeriodsAsync("; private const string LookUpDispersionAsyncSignature = "private async Task> LookUpDispersionAsync("; // The next declaration after LookUpDispersionAsync in file order (#4203's four end here); anchors the diff --git a/Darling/Darling.Tests/McpPayloadContractCensusTests.cs b/Darling/Darling.Tests/McpPayloadContractCensusTests.cs index ed02846798..f630364f20 100644 --- a/Darling/Darling.Tests/McpPayloadContractCensusTests.cs +++ b/Darling/Darling.Tests/McpPayloadContractCensusTests.cs @@ -2305,7 +2305,7 @@ public static readonly (string File, string Member, int Limit)[] StatementTermin [ ("DarlingFinOpsRecommendationsReader.cs", "(file scope)", 10), ("LocalDataService.Blocking.cs", "GetBlockingPairRowsAsync", 5000), - ("LocalDataService.FinOps.Recommendations.cs", "GetRecommendationsAsync", 10), + ("LocalDataService.FinOps.Recommendations.cs", "GetRecommendationsWithNoteAsync", 10), ("LocalDataService.PlanCache.cs", "GetPlanCacheSnapshotAsync", 30), ("LocalDataService.RunningJobs.cs", "GetAnomalousJobsAsync", 5), ("LocalDataService.WaitStats.cs", "GetAllQuerySnapshotsInRangeAsync", 2000), diff --git a/Darling/Darling.Tests/SecondaryReplicaScopeLiveTests.cs b/Darling/Darling.Tests/SecondaryReplicaScopeLiveTests.cs new file mode 100644 index 0000000000..6c5a060d2c --- /dev/null +++ b/Darling/Darling.Tests/SecondaryReplicaScopeLiveTests.cs @@ -0,0 +1,533 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Analysis; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Analysis; +using PerformanceMonitor.Darling.Service.Mcp; +using PerformanceMonitor.Darling.Storage; +using Xunit; + +namespace Darling.Tests; + +/* #1776 own-store: this fact mints its own scratch database through ScratchPostgres for every test and never touches + another test's rows, so it is deliberately NOT [Collection("live-postgres")]. */ + +/// +/// #5558 on a seeded PostgreSQL store, the twin of Lite's SecondaryReplicaFactTests: the replicated facts drop the +/// database this node holds as a secondary copy and the node-local ones keep it. Three databases: SecDb (the local +/// copy of group AG1 is SECONDARY), PrimDb (the local copy of group AG2 is PRIMARY) and StandDb (in no group). +/// Skipped when DARLING_TEST_PG is not set. +/// +public sealed class SecondaryReplicaScopeLiveTests +{ + private const int ServerId = -558_201; + private const string ServerName = "darling-secondary-replica-scope"; + private static readonly string[] Databases = ["SecDb", "PrimDb", "StandDb"]; + + private static string? BaseConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + + private static DateTime Now() => DarlingMcpTestData.TruncateToSeconds(DateTime.UtcNow); + + /// Runs against a fresh migrated scratch store with the test server registered, then + /// drops the store whether the body passed or not. + private static async Task WithStoreAsync(Func body) + { + var baseConnectionString = BaseConnectionString; + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), "Set DARLING_TEST_PG to run the live secondary replica scope tests."); + + var ct = TestContext.Current.CancellationToken; + var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + var bodySucceeded = false; + try + { + await using var connection = new NpgsqlConnection(scratch.ConnectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + await DarlingMcpTestData.RegisterServerAsync(connection, ServerId, ServerName, ct); + + await using var postgres = NpgsqlDataSource.Create(scratch.ConnectionString); + await body(connection, postgres, ct); + bodySucceeded = true; + } + finally + { + await LiveStoreCleanup.RunAsync(scratch.ConnectionString, bodySucceeded, async (_, _) => await Task.CompletedTask); + await scratch.DisposeAsync(); + } + } + + private static Task ExecAsync(NpgsqlConnection connection, CancellationToken ct, string sql, params object?[] values) => + DarlingMcpTestData.ExecAsync(connection, ct, sql, values); + + /// AG1: this node (NODE1) is for it and holds SecDb; AG2: this node is PRIMARY and + /// holds PrimDb. NODE2 is the other replica of AG1 and holds SecDb too (not local). + private static async Task SeedAgAsync(NpgsqlConnection c, CancellationToken ct, DateTime at, string localRole = "SECONDARY") + { + const string Replica = "INSERT INTO ag_replica_states (collection_id, collection_time, server_id, server_name, ag_name, replica_server_name, role_desc, is_local) VALUES ($1,$2,$3,$4,$5,$6,$7,$8)"; + var when = DarlingMcpTestData.Naive(at); + await ExecAsync(c, ct, Replica, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG1", "NODE1", localRole, true); + await ExecAsync(c, ct, Replica, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG1", "NODE2", "PRIMARY", false); + await ExecAsync(c, ct, Replica, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG2", "NODE1", "PRIMARY", true); + const string Db = "INSERT INTO ag_database_replica_states (collection_id, collection_time, server_id, server_name, ag_name, database_name, replica_server_name, is_local) VALUES ($1,$2,$3,$4,$5,$6,$7,$8)"; + await ExecAsync(c, ct, Db, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG1", "SecDb", "NODE1", true); + await ExecAsync(c, ct, Db, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG1", "SecDb", "NODE2", false); + await ExecAsync(c, ct, Db, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG2", "PrimDb", "NODE1", true); + } + + /// One sys.databases capture for each of the three databases: auto_shrink on and RCSI off, so every one is a + /// DB_CONFIG offender, plus one 20 GB percent-growth data file each for FILE_AUTOGROWTH_PERCENT. + private static async Task SeedDatabasesAsync(NpgsqlConnection c, CancellationToken ct, DateTime end) + { + foreach (var db in Databases) + { + await ExecAsync(c, ct, @" +INSERT INTO database_config + (config_id, capture_time, server_id, server_name, database_name, + state_desc, compatibility_level, collation_name, recovery_model, is_read_only, + is_auto_close_on, is_auto_shrink_on, is_auto_create_stats_on, is_auto_update_stats_on, + is_auto_update_stats_async_on, is_read_committed_snapshot_on, snapshot_isolation_state, + is_parameterization_forced, is_query_store_on, is_encrypted, is_trustworthy_on, is_db_chaining_on, + is_broker_enabled, is_cdc_enabled, is_mixed_page_allocation_on, log_reuse_wait_desc, page_verify_option, + target_recovery_time_seconds, delayed_durability, is_accelerated_database_recovery_on, + is_memory_optimized_enabled, is_optimized_locking_on) +VALUES ($1, $2, $3, $4, $5, + 'ONLINE', 160, 'SQL_Latin1_General_CP1_CI_AS', 'FULL', FALSE, + FALSE, TRUE, TRUE, TRUE, + FALSE, FALSE, 'OFF', + FALSE, TRUE, FALSE, FALSE, FALSE, + FALSE, FALSE, FALSE, 'NOTHING', 'CHECKSUM', + 60, 'DISABLED', FALSE, + FALSE, FALSE)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(end.AddHours(-1)), ServerId, ServerName, db); + + await ExecAsync(c, ct, @" +INSERT INTO database_size_stats + (collection_id, collection_time, server_id, server_name, database_name, database_id, file_id, file_type_desc, file_name, + physical_name, total_size_mb, used_size_mb, is_percent_growth, growth_pct) +VALUES ($1, $2, $3, $4, $5, 7, 1, 'ROWS', $6, 'D:\data.mdf', 20480, 10000, TRUE, 10)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(end.AddMinutes(-30)), ServerId, ServerName, db, db + "_data"); + } + } + + private static AnalysisContext Context(DateTime end) => new() + { + ServerId = ServerId, + ServerName = ServerName, + TimeRangeStart = DarlingMcpTestData.Naive(end.AddHours(-4)), + TimeRangeEnd = DarlingMcpTestData.Naive(end), + ServerUtcOffset = TimeSpan.Zero, + }; + + private static async Task<(List Facts, AnalysisContext Context)> FactsAsync(NpgsqlDataSource postgres, DateTime end, CancellationToken ct) + { + var context = Context(end); + context.CancellationToken = ct; + await PgSecondaryReplicaScope.EnsureAsync(postgres, context, logger: null); + var facts = (await new PgFactCollector(postgres).CollectFactsAsync(context)).ToList(); + return (facts, context); + } + + private static Fact One(List facts, string key) => Assert.Single(facts, f => f.Key == key); + + private static async Task DrillDownJsonAsync(NpgsqlDataSource postgres, AnalysisContext context, string rootFactKey, double severity = 0.3) + { + var finding = new AnalysisFinding + { + ServerId = ServerId, RootFactKey = rootFactKey, StoryPath = rootFactKey, PathKeys = [rootFactKey], Severity = severity, + }; + await new PgDrillDownCollector(postgres).EnrichFindingsAsync([finding], context); + return JsonSerializer.Serialize(finding.DrillDown); + } + + [Fact] + public async Task OnASecondary_DbConfigAndAutogrowthDropTheSecondaryDatabase_TheDrillDownsAgree_AndNodeLocalFactsKeepIt() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedDatabasesAsync(c, ct, end); + await SeedAgAsync(c, ct, end.AddMinutes(-1)); + + var (facts, context) = await FactsAsync(postgres, end, ct); + + Assert.Equal(["SecDb"], context.SecondaryReplicaDatabases!.ToArray()); + + var config = One(facts, "DB_CONFIG"); + Assert.Equal(2.0, config.Value); + Assert.Equal(2.0, config.Metadata["database_count"]); + Assert.Equal(2.0, config.Metadata["auto_shrink_on_count"]); + Assert.Equal(2.0, config.Metadata["rcsi_off_count"]); + + var autogrowth = One(facts, "FILE_AUTOGROWTH_PERCENT"); + Assert.Equal(2.0, autogrowth.Value); + Assert.Equal(2.0, autogrowth.Metadata["database_count"]); + + /* The drill-downs list the same databases the facts counted. */ + var configJson = await DrillDownJsonAsync(postgres, context, "DB_CONFIG"); + Assert.Contains("PrimDb", configJson); + Assert.Contains("StandDb", configJson); + Assert.DoesNotContain("SecDb", configJson); + + var growthJson = await DrillDownJsonAsync(postgres, context, "FILE_AUTOGROWTH_PERCENT"); + Assert.Contains("PrimDb", growthJson); + Assert.Contains("StandDb", growthJson); + Assert.DoesNotContain("SecDb", growthJson); + }); + } + + [Fact] + public async Task OnThePrimary_OrWithNoOrStaleAgRows_NothingIsSkipped() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedDatabasesAsync(c, ct, end); + + /* No AG rows at all. */ + var (noRows, noRowsContext) = await FactsAsync(postgres, end, ct); + Assert.Empty(noRowsContext.SecondaryReplicaDatabases!); + Assert.Equal(3.0, One(noRows, "DB_CONFIG").Metadata["database_count"]); + Assert.Equal(3.0, One(noRows, "FILE_AUTOGROWTH_PERCENT").Metadata["database_count"]); + + /* Rows exist, but the newest is three hours old: stale, so the role vouches for nothing. */ + await SeedAgAsync(c, ct, end.AddHours(-3)); + var (stale, staleContext) = await FactsAsync(postgres, end, ct); + Assert.Empty(staleContext.SecondaryReplicaDatabases!); + Assert.Equal(3.0, One(stale, "DB_CONFIG").Metadata["database_count"]); + + /* A fresh snapshot where the local role for the group is PRIMARY. */ + await SeedAgAsync(c, ct, end.AddMinutes(-1), localRole: "PRIMARY"); + var (primary, primaryContext) = await FactsAsync(postgres, end, ct); + Assert.Empty(primaryContext.SecondaryReplicaDatabases!); + Assert.Equal(3.0, One(primary, "DB_CONFIG").Metadata["database_count"]); + }); + } + + [Fact] + public async Task AnAsOfRead_UsesTheRoleAtItsOwnEnd_NotTheCurrentOne_AndTheNoteFollows() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var now = Now(); + /* Primary three hours ago, secondary a minute ago (a failover in between). */ + await SeedAgAsync(c, ct, now.AddHours(-3).AddMinutes(-1), localRole: "PRIMARY"); + await SeedAgAsync(c, ct, now.AddMinutes(-1)); + + var current = await PgSecondaryReplicaScope.ReadAsync(postgres, ServerId, now, logger: null, ct); + Assert.Equal(["SecDb"], current.ToArray()); + Assert.Equal(AgReplicaScope.SkippedNote(1), await PgSecondaryReplicaScope.NoteAsync(postgres, ServerId, logger: null, ct)); + + var earlier = await PgSecondaryReplicaScope.ReadAsync(postgres, ServerId, now.AddHours(-3), logger: null, ct); + Assert.Empty(earlier); + Assert.Null(await PgSecondaryReplicaScope.NoteAsync(postgres, ServerId, logger: null, ct, asOfUtc: now.AddHours(-3))); + + /* An anchor before any snapshot knows nothing, so it skips nothing. */ + Assert.Empty(await PgSecondaryReplicaScope.ReadAsync(postgres, ServerId, now.AddDays(-30), logger: null, ct)); + }); + } + + /// + /// #5558 round 2 (L1): compare_analysis and get_analysis_facts build secondary_replica_note from the set their own context + /// filtered with, not from a second read of the role. The baseline window ends before the only snapshot (nothing known, + /// nothing skipped); the comparison window ends after it (SecDb skipped). Each returned set is its own. + /// + [Fact] + public async Task CompareAndFactsReads_CarryTheSetTheirContextFilteredWith_SoTheNoteCannotDisagree() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedAgAsync(c, ct, end.AddMinutes(-1)); + var service = new DarlingAnalysisService(postgres); + + var (_, _, _, _, _, baselineSecondaries, comparisonSecondaries) = await service.ComparePeriodsAsync( + ServerId, ServerName, DarlingMcpTestData.Naive(end.AddHours(-30)), DarlingMcpTestData.Naive(end.AddHours(-26)), + DarlingMcpTestData.Naive(end.AddHours(-4)), DarlingMcpTestData.Naive(end), ct); + Assert.Empty(baselineSecondaries!); + Assert.Null(AgReplicaScope.SkippedNote(baselineSecondaries)); + Assert.Equal(["SecDb"], comparisonSecondaries!.ToArray()); + Assert.Equal(AgReplicaScope.SkippedNote(1), AgReplicaScope.SkippedNote(comparisonSecondaries)); + + var (_, _, caveats) = await service.CollectAndScoreFactsAsync(ServerId, ServerName, 4, DarlingMcpTestData.Naive(end), ct); + Assert.Equal(["SecDb"], caveats.SecondaryReplicaDatabases!.ToArray()); + }); + } + + private static Task SeedRegressionAsync(NpgsqlConnection c, CancellationToken ct, DateTime end, string db, long planId, string hash, + long avgCpu, DateTime firstExec, DateTime lastExec, long queryId = 101) => + ExecAsync(c, ct, @" +INSERT INTO query_store_stats + (collection_id, collection_time, server_id, server_name, database_name, query_id, plan_id, execution_type_desc, + first_execution_time, last_execution_time, query_text, query_hash, execution_count, avg_cpu_time_us, + avg_duration_us, query_plan_hash, is_forced_plan, force_failure_count, runtime_stats_interval_id) +VALUES ($1,$2,$3,$4,$5,$11,$6,'Regular',$7,$8,'SELECT 1','0xQH',100,$9,$9,$10,FALSE,0,$6)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(end), ServerId, ServerName, db, planId, + DarlingMcpTestData.Naive(firstExec), DarlingMcpTestData.Naive(lastExec), avgCpu, hash, queryId); + + [Fact] + public async Task PlanRegression_DropsTheSecondaryDatabase() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedAgAsync(c, ct, end.AddMinutes(-1)); + foreach (var db in new[] { "SecDb", "StandDb" }) + { + await SeedRegressionAsync(c, ct, end, db, planId: 1, hash: "0xGOOD", avgCpu: 100_000, firstExec: end.AddDays(-6), lastExec: end.AddDays(-5)); + await SeedRegressionAsync(c, ct, end, db, planId: 2, hash: "0xBAD", avgCpu: 1_200_000, firstExec: end.AddDays(-1), lastExec: end); + } + + var (facts, context) = await FactsAsync(postgres, end, ct); + + Assert.Equal(1.0, One(facts, "PLAN_REGRESSION").Metadata["offender_count"]); + Assert.NotEmpty(context.PlanRegressionOffenders!); + Assert.All(context.PlanRegressionOffenders!, o => Assert.Equal("StandDb", o.DatabaseName)); + }); + } + + /// + /// L1 (round 1): the regressed-queries drill-down filters the secondary database in its source CTE, BEFORE its LIMIT 5, as + /// Lite's twin does. The pass where the fact did not run has no offender list, so every query is read; when the secondary + /// database holds the five worst regressions, a filter applied after the LIMIT would leave nothing for the primary's own. + /// + [Fact] + public async Task TheRegressedQueriesDrillDown_FiltersTheSecondaryDatabaseBeforeItsLimit() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedAgAsync(c, ct, end.AddMinutes(-1)); + for (var q = 0; q < 5; q++) + { + var queryId = 201 + q; + await SeedRegressionAsync(c, ct, end, "SecDb", planId: queryId * 10 + 1, hash: "0xGOOD", avgCpu: 100_000, firstExec: end.AddDays(-6), lastExec: end.AddDays(-5), queryId: queryId); + await SeedRegressionAsync(c, ct, end, "SecDb", planId: queryId * 10 + 2, hash: "0xBAD", avgCpu: 5_000_000, firstExec: end.AddDays(-1), lastExec: end, queryId: queryId); + } + foreach (var queryId in new long[] { 101, 102 }) + { + await SeedRegressionAsync(c, ct, end, "StandDb", planId: queryId * 10 + 1, hash: "0xGOOD", avgCpu: 100_000, firstExec: end.AddDays(-6), lastExec: end.AddDays(-5), queryId: queryId); + await SeedRegressionAsync(c, ct, end, "StandDb", planId: queryId * 10 + 2, hash: "0xBAD", avgCpu: 1_200_000, firstExec: end.AddDays(-1), lastExec: end, queryId: queryId); + } + + /* No fact ran, so PlanRegressionOffenders is null; the set itself is resolved as a pass would. */ + var context = Context(end); + context.CancellationToken = ct; + await PgSecondaryReplicaScope.EnsureAsync(postgres, context, logger: null); + Assert.Null(context.PlanRegressionOffenders); + + var json = await DrillDownJsonAsync(postgres, context, "PLAN_REGRESSION", severity: 0.6); + using var doc = JsonDocument.Parse(json); + var rows = doc.RootElement.GetProperty("regressed_queries").EnumerateArray().ToList(); + Assert.Equal(2, rows.Count); + Assert.All(rows, r => Assert.Equal("StandDb", r.GetProperty("database").GetString())); + }); + } + + private static Task SeedObjectAsync(NpgsqlConnection c, CancellationToken ct, DateTime at, string db, decimal reservedMb) => + ExecAsync(c, ct, @" +INSERT INTO index_object_stats + (collection_id, collection_time, server_id, server_name, database_name, database_id, schema_name, object_id, table_name, + index_id, index_name, reserved_mb, row_lock_wait_in_ms, index_lock_promotion_count) +VALUES ($1,$2,$3,$4,$5,7,'dbo',100,'Big',1,'PK_Big',$6,0,0)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(at), ServerId, ServerName, db, reservedMb); + + [Fact] + public async Task ObjectGrowth_DropsTheSecondaryDatabase() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedAgAsync(c, ct, end.AddMinutes(-1)); + foreach (var (db, grownMb) in new[] { ("SecDb", 900m), ("StandDb", 500m) }) + { + await SeedObjectAsync(c, ct, end.AddDays(-1), db, 100m); + await SeedObjectAsync(c, ct, end.AddHours(-1), db, 100m + grownMb); + } + + /* The baseline gate: anomaly detection sits out a server with no history to compare against. */ + await ExecAsync(c, ct, + "INSERT INTO cpu_utilization_stats (collection_id, collection_time, server_id, server_name, sample_time, sqlserver_cpu_utilization, other_process_cpu_utilization) VALUES ($1, $2, $3, $4, $2, 10, 5)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(end.AddDays(-2)), ServerId, ServerName); + + var context = Context(end); + context.CancellationToken = ct; + await PgSecondaryReplicaScope.EnsureAsync(postgres, context, logger: null); + var detector = new PgAnomalyDetector(postgres, new PgBaselineProvider(postgres)); + var anomalies = await detector.DetectAnomaliesAsync(context); + + var growth = Assert.Single(anomalies, f => f.Key == "ANOMALY_OBJECT_GROWTH"); + Assert.Equal("StandDb", growth.DatabaseName); + }); + } + + /// + /// get_analysis_findings carries the shared one-sentence note beside a non-empty list and inside the empty answer when + /// this node holds a secondary copy. Lite's twin is in McpAnalysisFindingsCommandTests. + /// + [Fact] + public async Task GetAnalysisFindings_CarriesTheSecondaryReplicaNote_OnlyWhenThisNodeHoldsASecondaryCopy() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var now = Now(); + var service = new DarlingAnalysisService(postgres); + + /* Nothing stored, no AG rows: the plain empty answer, no note. */ + using (var plain = JsonDocument.Parse(await DarlingMcpTools.GetAnalysisFindings(service, postgres, ServerName))) + { + Assert.Equal("empty", plain.RootElement.GetProperty("status").GetString()); + Assert.DoesNotContain("secondary copy", plain.RootElement.GetProperty("message").GetString(), StringComparison.Ordinal); + } + + /* A secondary copy, still no findings: the empty answer names the skipped database. */ + await SeedAgAsync(c, ct, now.AddMinutes(-1)); + using (var empty = JsonDocument.Parse(await DarlingMcpTools.GetAnalysisFindings(service, postgres, ServerName))) + { + Assert.Contains(AgReplicaScope.SkippedNote(1)!, empty.RootElement.GetProperty("message").GetString(), StringComparison.Ordinal); + } + + /* With a finding stored the field rides on the payload. */ + var t = now.AddMinutes(-10); + await ExecAsync(c, ct, @" +INSERT INTO analysis_findings + (finding_id, analysis_time, server_id, server_name, database_name, time_range_start, time_range_end, severity, confidence, + category, story_path, story_path_hash, story_text, root_fact_key, root_fact_value, leaf_fact_key, leaf_fact_value, fact_count, + incident_id, remediation_action_json, drill_down_json) +VALUES ($1, $2, $3, $4, 'StandDb', $5, $2, 0.72, 0.55, 'config', 'DB_CONFIG', 'AG5558_HASH', '', 'DB_CONFIG', 2, 'DB_CONFIG', 2, 1, + 'AG5558_INCIDENT', NULL, NULL)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(t), ServerId, ServerName, DarlingMcpTestData.Naive(t.AddHours(-4))); + + using (var withNote = JsonDocument.Parse(await DarlingMcpTools.GetAnalysisFindings(service, postgres, ServerName))) + { + Assert.Equal(AgReplicaScope.SkippedNote(1), withNote.RootElement.GetProperty("secondary_replica_note").GetString()); + } + }); + } + + private static Task PlantWaitAsync(NpgsqlConnection c, CancellationToken ct, DateTime at, long waitMs) => ExecAsync(c, ct, @" +INSERT INTO wait_stats + (collection_id, collection_time, server_id, server_name, wait_type, delta_waiting_tasks, delta_wait_time_ms) +VALUES ($1, $2, $3, $4, 'AN5558_WAIT', 50, $5)", CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(at), ServerId, ServerName, waitMs); + + /// The note from wherever the answer's shape puts it: the root of a data-bearing answer, or the hints of a miss. + private static string? NoteOf(string json) + { + using var doc = JsonDocument.Parse(json); + var root = doc.RootElement; + var carrier = root.TryGetProperty("secondary_replica_note", out _) ? root : root.GetProperty("hints"); + var note = carrier.GetProperty("secondary_replica_note"); + return note.ValueKind == JsonValueKind.Null ? null : note.GetString(); + } + + /// + /// M4 (round 1): analyze_server carries the same note get_analysis_findings does, under the same field name, always present + /// and null when nothing is skipped; the service keeps the sentence the pass used. Lite's twin is in + /// SecondaryReplicaMcpAnalysisTests. + /// + [Fact] + public async Task AnalyzeServer_CarriesTheNote_OnlyWhenThisNodeHoldsASecondaryCopy() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var service = new DarlingAnalysisService(postgres) { MinimumDataHours = 0 }; + + Assert.Null(NoteOf(await DarlingMcpTools.AnalyzeServer(service, postgres, ServerName, 4))); + Assert.Null(service.LastSecondaryReplicaNote); + + await SeedAgAsync(c, ct, Now().AddMinutes(-1)); + var answer = await DarlingMcpTools.AnalyzeServer(service, postgres, ServerName, 4); + Assert.Equal(AgReplicaScope.SkippedNote(1), NoteOf(answer)); + Assert.Equal(AgReplicaScope.SkippedNote(1), service.LastSecondaryReplicaNote); + + using var doc = JsonDocument.Parse(answer); + if (doc.RootElement.GetProperty("status").GetString() == "empty") + Assert.Contains(AgReplicaScope.SkippedNote(1)!, doc.RootElement.GetProperty("message").GetString(), StringComparison.Ordinal); + }); + } + + /// + /// Round 2: get_analysis_facts returns facts the secondary filter already trimmed, so it carries the same note, in + /// every envelope it can answer with, always present and null when nothing is skipped. Lite's twin is in + /// SecondaryReplicaMcpAnalysisTests. + /// + [Fact] + public async Task GetAnalysisFacts_CarriesTheNote_OnlyWhenThisNodeHoldsASecondaryCopy() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var service = new DarlingAnalysisService(postgres) { MinimumDataHours = 0 }; + + /* Nothing collected, no AG rows: an unavailable answer, the field there and null. */ + Assert.Null(NoteOf(await DarlingMcpTools.GetAnalysisFacts(service, postgres, ServerName, 4, cancellationToken: ct))); + + /* A secondary copy, still nothing collected: the sentence is in the field and in the prose. */ + await SeedAgAsync(c, ct, Now().AddMinutes(-1)); + var bare = await DarlingMcpTools.GetAnalysisFacts(service, postgres, ServerName, 4, cancellationToken: ct); + Assert.Equal(AgReplicaScope.SkippedNote(1), NoteOf(bare)); + using (var bareDoc = JsonDocument.Parse(bare)) + Assert.Contains(AgReplicaScope.SkippedNote(1)!, bareDoc.RootElement.GetProperty("message").GetString(), StringComparison.Ordinal); + + /* Facts in the window: the facts payload carries it at the root. */ + for (var m = 5; m <= 55; m += 10) await PlantWaitAsync(c, ct, Now().AddMinutes(-m), 150_000L); + var withFacts = await DarlingMcpTools.GetAnalysisFacts(service, postgres, ServerName, 4, cancellationToken: ct); + using var factsDoc = JsonDocument.Parse(withFacts); + Assert.True(factsDoc.RootElement.TryGetProperty("facts", out _), "expected the facts payload, got: " + withFacts[..Math.Min(withFacts.Length, 200)]); + Assert.Equal(AgReplicaScope.SkippedNote(1), factsDoc.RootElement.GetProperty("secondary_replica_note").GetString()); + }); + } + + [Fact] + public async Task GetAnalysisFacts_OnAPrimary_CarriesANullNote() + { + await WithStoreAsync(async (c, postgres, ct) => + { + await SeedAgAsync(c, ct, Now().AddMinutes(-1), localRole: "PRIMARY"); + for (var m = 5; m <= 55; m += 10) await PlantWaitAsync(c, ct, Now().AddMinutes(-m), 150_000L); + var answer = await DarlingMcpTools.GetAnalysisFacts( + new DarlingAnalysisService(postgres) { MinimumDataHours = 0 }, postgres, ServerName, 4, cancellationToken: ct); + Assert.Null(NoteOf(answer)); + }); + } + + /// M4 (round 1): compare_analysis carries one note per window, each from the role at that window's own end. + [Fact] + public async Task CompareAnalysis_CarriesOneNotePerWindow_EachFromItsOwnRole() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var now = Now(); + var baselineEnd = now.AddHours(-24); + + /* Secondary when the baseline window ended, primary after a failover by the time the comparison window ends. */ + await SeedAgAsync(c, ct, baselineEnd.AddMinutes(-1)); + await SeedAgAsync(c, ct, now.AddMinutes(-1), localRole: "PRIMARY"); + await PlantWaitAsync(c, ct, baselineEnd.AddHours(-1), 500_000L); + await PlantWaitAsync(c, ct, now.AddHours(-1), 900_000L); + + var answer = await DarlingMcpTools.CompareAnalysis( + new DarlingAnalysisService(postgres) { MinimumDataHours = 0 }, postgres, ServerName, 4, 28, cancellationToken: ct); + + using var doc = JsonDocument.Parse(answer); + var root = doc.RootElement; + var (baseline, comparison) = root.TryGetProperty("baseline", out var b) + ? (b.GetProperty("secondary_replica_note"), root.GetProperty("comparison").GetProperty("secondary_replica_note")) + : (root.GetProperty("hints").GetProperty("baseline_secondary_replica_note"), root.GetProperty("hints").GetProperty("comparison_secondary_replica_note")); + Assert.Equal(AgReplicaScope.SkippedNote(1), baseline.GetString()); + Assert.Equal(JsonValueKind.Null, comparison.ValueKind); + }); + } +} diff --git a/Darling/Darling.Tests/SecondaryReplicaScopeTests.cs b/Darling/Darling.Tests/SecondaryReplicaScopeTests.cs new file mode 100644 index 0000000000..99bc1a4b94 --- /dev/null +++ b/Darling/Darling.Tests/SecondaryReplicaScopeTests.cs @@ -0,0 +1,277 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Analysis; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Analysis; +using PerformanceMonitor.Darling.Viewer; +using Xunit; + +namespace Darling.Tests; + +/// +/// #5558: 's predicate builder and its fail-open read, with no database. The seeded +/// twins of Lite's fact tests are in . +/// +[Trait("Reads", "Lite")] +public sealed class PgSecondaryReplicaScopeTests +{ + private static AnalysisContext ContextWith(params string[] secondaries) => new() + { + SecondaryReplicaDatabases = new HashSet(secondaries, StringComparer.Ordinal), + }; + + [Fact] + public void Apply_WithNothingToSkip_JustRemovesTheMarker_AndAddsNoParameter() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t WHERE a = $1 /*SEC*/ ORDER BY 1"); + cmd.Parameters.AddWithValue(1); + + PgSecondaryReplicaScope.Apply(cmd, ContextWith(), "DB_CONFIG", "database_name"); + + Assert.DoesNotContain("/*SEC*/", cmd.CommandText, StringComparison.Ordinal); + Assert.Equal("SELECT 1 FROM t WHERE a = $1 ORDER BY 1", cmd.CommandText); + Assert.Single(cmd.Parameters); + } + + [Fact] + public void Apply_WithASecondary_BindsTheNamesAsTheyAreAsOneArray_NumberedAfterTheExistingParameters() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t WHERE a = $1 /*SEC*/ ORDER BY 1"); + cmd.Parameters.AddWithValue(1); + + PgSecondaryReplicaScope.Apply(cmd, ContextWith("SecDb", "OtherDb"), "DB_CONFIG", "database_name"); + + Assert.Contains("AND (database_name IS NULL OR database_name <> ALL($2::text[]))", cmd.CommandText, StringComparison.Ordinal); + Assert.Equal(2, cmd.Parameters.Count); + var names = Assert.IsType(cmd.Parameters[1].Value); + Assert.Equal(["OtherDb", "SecDb"], names.OrderBy(n => n, StringComparer.Ordinal).ToArray()); + } + + [Fact] + public void Apply_MatchesTheNameExactly_ASecondarySalesDbDoesNotHideASeparateSalesdb() + { + /* #5558 round 2: on a case-sensitive server collation, SalesDb (a secondary copy) and salesdb (a standalone + database) are two databases. The set carries only the secondary's exact name and the predicate compares + it as is, so the standalone one keeps its rows. */ + using var cmd = new NpgsqlCommand("SELECT 1 FROM t WHERE a = $1 /*SEC*/"); + cmd.Parameters.AddWithValue(1); + + PgSecondaryReplicaScope.Apply(cmd, ContextWith("SalesDb"), "DB_CONFIG", "database_name"); + + Assert.Equal(["SalesDb"], Assert.IsType(cmd.Parameters[1].Value)); + Assert.DoesNotContain("lower(", cmd.CommandText, StringComparison.Ordinal); + } + + /// + /// #5558 round 2 (L4): the regressed-queries drill-down has two SQL variants, the raw one and the interval-table one, and + /// the live drill-down test runs whichever the seeded store picks. Each must carry the marker exactly once and before its + /// LIMIT 5; a variant that lost it would leave Apply binding a parameter nothing references and the filter + /// would silently not run there. + /// + [Theory] + [InlineData("raw")] + [InlineData("table")] + public void EachRegressedQueriesVariant_CarriesTheMarkerExactlyOnce_BeforeItsLimit(string variant) + { + var sql = variant == "raw" ? PgDrillDownCollector.RegressedQueriesSql : PgDrillDownCollector.RegressedQueriesTableSql; + + var first = sql.IndexOf(PgSecondaryReplicaScope.Marker, StringComparison.Ordinal); + Assert.True(first >= 0, variant + " variant lost the secondary-replica marker"); + Assert.Equal(-1, sql.IndexOf(PgSecondaryReplicaScope.Marker, first + PgSecondaryReplicaScope.Marker.Length, StringComparison.Ordinal)); + + var limit = sql.IndexOf("LIMIT 5", first, StringComparison.Ordinal); + Assert.True(limit > first, variant + " variant: the marker must come before its LIMIT 5, so the filter runs before the cut"); + } + + [Fact] + public void Apply_UsesTheKeywordItIsGiven_ForAClauseThatOpensTheWhere() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t /*SEC*/ GROUP BY 1"); + PgSecondaryReplicaScope.Apply(cmd, ContextWith("SecDb"), "PLAN_REGRESSION", "database_name", "WHERE"); + Assert.Contains(" WHERE (database_name IS NULL OR database_name <> ALL($1::text[]))", cmd.CommandText, StringComparison.Ordinal); + } + + [Fact] + public void Apply_LeavesANodeLocalKeyUnfiltered_EvenWithASecondarySet() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t WHERE a = $1 /*SEC*/"); + cmd.Parameters.AddWithValue(1); + + PgSecondaryReplicaScope.Apply(cmd, ContextWith("SecDb"), "MISSING_INDEX", "database_name"); + + Assert.DoesNotContain("/*SEC*/", cmd.CommandText, StringComparison.Ordinal); + Assert.DoesNotContain("<> ALL(", cmd.CommandText, StringComparison.Ordinal); + Assert.Single(cmd.Parameters); + } + + [Fact] + public void Apply_WithNoSetOnTheContext_SkipsNothing() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t WHERE a = $1 /*SEC*/"); + cmd.Parameters.AddWithValue(1); + + PgSecondaryReplicaScope.Apply(cmd, new AnalysisContext(), "DB_CONFIG", "database_name"); + + Assert.DoesNotContain("<> ALL(", cmd.CommandText, StringComparison.Ordinal); + Assert.Single(cmd.Parameters); + } + + [Fact] + public async Task ReadAsync_FailsOpen_WhenTheStoreCannotBeReached() + { + /* A port nothing listens on: the connection fails at once, and the answer is the empty set, not a throw. */ + await using var unreachable = NpgsqlDataSource.Create( + "Host=127.0.0.1;Port=1;Username=x;Database=x;Timeout=2;Pooling=false"); + + var set = await PgSecondaryReplicaScope.ReadAsync(unreachable, 1, DateTime.UtcNow, logger: null, CancellationToken.None); + Assert.Empty(set); + + Assert.Null(await PgSecondaryReplicaScope.NoteAsync(unreachable, 1, logger: null, CancellationToken.None)); + } + + [Fact] + public async Task EnsureAsync_FillsTheSetOnce_AndLeavesAnAlreadyResolvedSetAlone() + { + await using var unreachable = NpgsqlDataSource.Create( + "Host=127.0.0.1;Port=1;Username=x;Database=x;Timeout=2;Pooling=false"); + + var context = new AnalysisContext { ServerId = 1, TimeRangeEnd = DateTime.UtcNow }; + await PgSecondaryReplicaScope.EnsureAsync(unreachable, context, logger: null); + Assert.NotNull(context.SecondaryReplicaDatabases); + Assert.Empty(context.SecondaryReplicaDatabases!); + + var resolved = new HashSet(StringComparer.Ordinal) { "SecDb" }; + var already = new AnalysisContext { ServerId = 1, SecondaryReplicaDatabases = resolved }; + await PgSecondaryReplicaScope.EnsureAsync(unreachable, already, logger: null); + Assert.Same(resolved, already.SecondaryReplicaDatabases); + } + + [Fact] + public void NoneSkipped_IsEmpty() + { + Assert.Empty(PgSecondaryReplicaScope.NoneSkipped); + } +} + +/// +/// #5558: every new AnalysisContext in the product code resolves the secondary replica set, either in its own +/// initializer or through EnsureAsync on the variable it is assigned to. A context built without it leaves +/// null, which skips nothing, so the replicated findings of a +/// secondary copy would quietly come back for that one pass. A pass that reads only node-local facts says so by setting the +/// empty set on purpose. Scans Lite and Darling; test projects build contexts freely and are not scanned. +/// +[Trait("Reads", "Lite")] +public sealed class AnalysisContextSecondaryScopeSourceScanTests +{ + private static string RepoRoot([CallerFilePath] string thisFile = "") + { + var dir = Path.GetDirectoryName(thisFile)!; + while (dir is not null && !File.Exists(Path.Combine(dir, "Directory.Build.props"))) + dir = Path.GetDirectoryName(dir); + Assert.False(dir is null, "could not locate the repo root from the test source path"); + return dir!; + } + + private static IEnumerable ProductSources(string root) + { + foreach (var top in new[] { "Lite", "Darling" }) + { + foreach (var file in Directory.EnumerateFiles(Path.Combine(root, top), "*.cs", SearchOption.AllDirectories)) + { + var relative = Path.GetRelativePath(root, file).Replace('\\', '/'); + if (relative.Contains("/bin/") || relative.Contains("/obj/") || relative.Contains(".Tests/")) continue; + yield return file; + } + } + } + + [Fact] + public void EveryAnalysisContextInTheProduct_ResolvesTheSecondaryReplicaSet() + { + var root = RepoRoot(); + var construction = new Regex(@"(?:var\s+(?\w+)\s*=\s*)?new\s+AnalysisContext\b", RegexOptions.CultureInvariant); + var missing = new List(); + var sites = 0; + + /* The end of the member a construction sits in: the next closing brace at member indentation (4 spaces). + Each site is judged on its own text, so a second context in a file that already ensures a variable of the + same name cannot borrow the first one's call. */ + var memberEnd = new Regex(@"\n \}[ \t]*\n", RegexOptions.CultureInvariant); + + foreach (var file in ProductSources(root)) + { + var text = File.ReadAllText(file).Replace("\r\n", "\n"); + foreach (Match m in construction.Matches(text)) + { + sites++; + var end = memberEnd.Match(text, m.Index); + var limit = end.Success ? end.Index : text.Length; + var member = text[m.Index..limit]; + /* The initializer's closing brace is searched only up to the member's end: an inline construction + such as Foo(new AnalysisContext { ... }); closes with "})", and an unbounded search would run on into + a LATER construction's initializer and borrow its SecondaryReplicaDatabases setting (#5558 round 2). */ + var close = text.IndexOf("};", m.Index, limit - m.Index, StringComparison.Ordinal); + var initializer = close < 0 ? member : text[m.Index..close]; + var name = m.Groups["name"].Value; + var inInitializer = initializer.Contains("SecondaryReplicaDatabases", StringComparison.Ordinal); + var ensured = name.Length > 0 && Regex.IsMatch(member, @"SecondaryReplicaScope\.EnsureAsync\([^)]*\b" + Regex.Escape(name) + @"\b"); + + /* A context handed straight to AnalyzeAsync(context) is resolved by that overload, provided the overload + itself ensures the set it receives (checked on the same file's declaration, so the hand-off cannot + be a loophole). */ + if (!inInitializer && !ensured && name.Length > 0 && Regex.IsMatch(member, @"\bAnalyzeAsync\(\s*" + Regex.Escape(name) + @"\s*\)")) + { + var overload = Regex.Match(text, @"AnalyzeAsync\(AnalysisContext (?

\w+)\)"); + if (overload.Success) + { + var overloadEnd = memberEnd.Match(text, overload.Index); + var body = overloadEnd.Success ? text[overload.Index..overloadEnd.Index] : text[overload.Index..]; + ensured = Regex.IsMatch(body, @"SecondaryReplicaScope\.EnsureAsync\([^)]*\b" + overload.Groups["p"].Value + @"\b"); + } + } + if (!inInitializer && !ensured) + { + var line = text[..m.Index].Count(c => c == '\n') + 1; + missing.Add(Path.GetRelativePath(root, file).Replace('\\', '/') + ":" + line); + } + } + } + + Assert.True(sites >= 10, "the scan found only " + sites + " construction sites, so its pattern no longer matches the code"); + Assert.True(missing.Count == 0, + "an AnalysisContext that never resolves SecondaryReplicaDatabases would bring back a secondary copy's findings: " + string.Join(", ", missing)); + } +} + +///

#5558: the viewer Recommendations view model carries the note only where a list or the all-clear shows. +[Trait("Reads", "Lite")] +public sealed class RecommendationsReplicaNoteTests +{ + [Fact] + public void TheNote_IsCarriedOnTheEmptyAndLoadedStates_AndNullWhenNotSet() + { + var empty = RecommendationsViewModel.FromFindings(Array.Empty(), "SQL2022"); + Assert.Null(empty.ReplicaNote); + + var note = AgReplicaScope.SkippedNote(2); + Assert.Equal(note, empty.WithReplicaNote(note).ReplicaNote); + Assert.Same(empty, empty.WithReplicaNote(" ")); + Assert.Null(empty.ReplicaNote); + Assert.Equal(RecommendationsState.Empty, empty.State); + } +} diff --git a/Darling/Darling.Tests/TsqlConventionGuardTests.cs b/Darling/Darling.Tests/TsqlConventionGuardTests.cs index 2c1061b3b2..c341d49ca9 100644 --- a/Darling/Darling.Tests/TsqlConventionGuardTests.cs +++ b/Darling/Darling.Tests/TsqlConventionGuardTests.cs @@ -1564,6 +1564,10 @@ Lite twin (LocalDataService.FinOps.IndexObjects.cs, below) derives the same four on dev's 291e15f8 run); the buffer drill-down's `Round` LEFT on the same run — #3799 reshaped it into a block the walk reads whole — so its #3804 line comes off here. */ "Darling/PerformanceMonitor.Darling.Analysis/PgTargetFactCollector.Plans.cs Ratio", + /* #5558: the secondary-replica rule's expression-bodied `IsFresh` (`snapshotUtc is { } at && at <= windowEndUtc && ...`), + the same expression-shaped member as the lines above. What its range strands is the pattern variable and the + comparison, no T-SQL, no tempdb label and no SQL literal a census reads. ARRIVED with #5558. */ + "PerformanceMonitor.Common/AgReplicaScope.cs IsFresh", ]; /* ───────────────────────── the resolver, pinned on arranged source ───────────────────────── */ diff --git a/Darling/Darling.Tests/analysis-findings-harness.mjs b/Darling/Darling.Tests/analysis-findings-harness.mjs index ecae30c8f4..d8d98b0dc0 100644 --- a/Darling/Darling.Tests/analysis-findings-harness.mjs +++ b/Darling/Darling.Tests/analysis-findings-harness.mjs @@ -121,6 +121,15 @@ try { const root = await run("srv-a", { findings: sample, truncated: true, truncation_note: "READ-CAP-NOTE", findings_truncated: true, findings_truncated_note: "PAGE-NOTE" }); return { text: root.textContent }; }, + replicaNote: async () => { + /* #5558: the secondary-copy note shows with a list and with an empty chain list, and is absent when the read carries none. */ + const note = "REPLICA-NOTE"; + const withFindings = await run("srv-a", { findings: sample, secondary_replica_note: note }); + const noFindings = await run("srv-a", { findings: [], secondary_replica_note: note }); + const without = await run("srv-a", { findings: sample }); + const nullNote = await run("srv-a", { findings: sample, secondary_replica_note: null }); + return { withFindings: withFindings.textContent, noFindings: noFindings.textContent, without: without.textContent, nullNote: nullNote.textContent }; + }, signal: async () => { const ac = new AbortController(); await run("srv-a", null, { hours: 24, label: "x", signal: ac.signal }); diff --git a/Darling/PerformanceMonitor.Darling.Analysis/DarlingAnalysisService.cs b/Darling/PerformanceMonitor.Darling.Analysis/DarlingAnalysisService.cs index fd71a934e6..336678f13a 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/DarlingAnalysisService.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/DarlingAnalysisService.cs @@ -221,6 +221,15 @@ internal sealed record AnalysisEngineSet( /// See : of those facts, how many scored above zero. public int? LastFactsScored { get; private set; } + /// + /// #5558: the one-sentence note naming how many databases the last pass left to the primary replica because this + /// node holds only a secondary copy of them in an availability group, or null when none were skipped (and when + /// the pass never reached the set). Carried out like , because a short or empty + /// findings list cannot say it: without the note, "nothing found" reads as a clean bill of health for databases + /// the pass never looked at. + /// + public string? LastSecondaryReplicaNote { get; private set; } + /// /// How the last pass ended EARLY, or null when it ran through (#2430). Set inside the pass's own /// catch, so here means a genuine fault: the pass reached the @@ -266,6 +275,26 @@ internal sealed record AnalysisEngineSet( } } + /// + /// #5558: the databases this node holds only as a secondary Availability Group copy, as of the pass's window end, + /// so the replicated facts skip them. Every context this service builds goes through here. Fails open: any unknown + /// returns an empty set. + /// + internal async Task> SecondaryScopeForAsync(int serverId, DateTime windowEndUtc, CancellationToken cancellationToken) + { + try + { + return await PgSecondaryReplicaScope.ReadAsync(_postgres, serverId, windowEndUtc, _logger, cancellationToken); + } + catch (Exception ex) when (AnalysisShutdown.IsExpectedAbandon(ex, cancellationToken)) + { + /* This runs in the context initializer, ahead of the pass's own try. An abandoned budget must not escape the + wrapper from here: an empty set lets the pass reach its first checkpoint and take the normal abandon + path (and the read-only wrappers fail on their next store read, as #4203 requires). */ + return PgSecondaryReplicaScope.NoneSkipped; + } + } + /// The store, read as whatever role this data source connects as. /// Optional; the SQL Server drill-down's cached-plan fetch. /// Optional. @@ -342,6 +371,7 @@ public async Task> AnalyzeAsync( { ServerId = serverId, SeparatelyMonitoredDatabases = await ScopeForAsync(serverId, cancellationToken), + SecondaryReplicaDatabases = await SecondaryScopeForAsync(serverId, timeRangeEnd, cancellationToken), /* #5558 */ ServerName = serverName, TimeRangeStart = timeRangeStart, TimeRangeEnd = timeRangeEnd, @@ -377,6 +407,7 @@ public async Task> AnalyzeAsync(AnalysisContext context) LastFactCount = null; LastFactsScored = null; EndedEarlyAs = null; + LastSecondaryReplicaNote = AgReplicaScope.SkippedNote(context.SecondaryReplicaDatabases); /* #5558 */ try { @@ -766,6 +797,7 @@ letting the halves drift. */ { ServerId = serverId, SeparatelyMonitoredDatabases = await ScopeForAsync(serverId, cancellationToken), + SecondaryReplicaDatabases = await SecondaryScopeForAsync(serverId, timeRangeEnd, cancellationToken), /* #5558 */ ServerName = serverName, TimeRangeStart = timeRangeStart, TimeRangeEnd = timeRangeEnd, @@ -824,6 +856,8 @@ public async Task> CollectConfigAuditFactsAsync( var context = new AnalysisContext { ServerId = serverId, + /* #5558: audit_config reads server settings only, all node-local, so nothing is skipped and no AG read is paid for. */ + SecondaryReplicaDatabases = PgSecondaryReplicaScope.NoneSkipped, ServerName = serverName, TimeRangeStart = timeRangeEnd.AddHours(-1), TimeRangeEnd = timeRangeEnd, @@ -876,8 +910,13 @@ context.CancellationToken is the caller's own token. */ /// that fails must not cost the caller the comparison it was only meant to refine, so it degrades /// to an empty map and every key takes the absolute rule — the never-blind fallback the anomaly /// gate follows. + /// + /// #5558: the last two elements are the secondary-replica sets the two windows' contexts filtered with (null + /// when collection threw), each as of its own window end. compare_analysis builds its per-window + /// secondary_replica_note from them rather than reading the role a second time, so a note cannot name a skip + /// the facts did not make. /// - public async Task<(List BaselineFacts, List ComparisonFacts, WindowCoverage? BaselineCoverage, WindowCoverage? ComparisonCoverage, IReadOnlyDictionary Dispersion)> ComparePeriodsAsync( + public async Task<(List BaselineFacts, List ComparisonFacts, WindowCoverage? BaselineCoverage, WindowCoverage? ComparisonCoverage, IReadOnlyDictionary Dispersion, IReadOnlySet? BaselineSecondaries, IReadOnlySet? ComparisonSecondaries)> ComparePeriodsAsync( int serverId, string serverName, DateTime baselineStart, DateTime baselineEnd, DateTime comparisonStart, DateTime comparisonEnd, @@ -888,6 +927,8 @@ context.CancellationToken is the caller's own token. */ { ServerId = serverId, SeparatelyMonitoredDatabases = separatelyMonitored, + /* #5558: per window, not shared: the windows can straddle a failover and each uses the role at its own end. */ + SecondaryReplicaDatabases = await SecondaryScopeForAsync(serverId, baselineEnd, cancellationToken), ServerName = serverName, TimeRangeStart = baselineStart, TimeRangeEnd = baselineEnd, @@ -898,6 +939,7 @@ context.CancellationToken is the caller's own token. */ { ServerId = serverId, SeparatelyMonitoredDatabases = separatelyMonitored, + SecondaryReplicaDatabases = await SecondaryScopeForAsync(serverId, comparisonEnd, cancellationToken), /* #5558 */ ServerName = serverName, TimeRangeStart = comparisonStart, TimeRangeEnd = comparisonEnd, @@ -915,7 +957,8 @@ context.CancellationToken is the caller's own token. */ var dispersion = await LookUpDispersionAsync(engine, serverId, serverName, baselineFacts, comparisonFacts, comparisonStart); - return (baselineFacts, comparisonFacts, baselineContext.Coverage, comparisonContext.Coverage, dispersion); + return (baselineFacts, comparisonFacts, baselineContext.Coverage, comparisonContext.Coverage, dispersion, + baselineContext.SecondaryReplicaDatabases, comparisonContext.SecondaryReplicaDatabases); } catch (Exception ex) when (ex is not OperationCanceledException) { @@ -925,7 +968,7 @@ not be swallowed into an empty result and logged as a fault — this read has no caller's own token, the same reasoning CollectConfigAuditFactsAsync's catch already states (#4203). */ _logger?.LogError("[DarlingAnalysisService] Period comparison failed for {Server}: {Message}", serverName, ex.Message); - return ([], [], null, null, new Dictionary()); + return ([], [], null, null, new Dictionary(), null, null); } } @@ -1299,7 +1342,7 @@ private async Task AttributeConfigChangesAsync(AnalysisEngineSet engine, Analysi var windows = ConfigChangeAttribution.WindowsFor(anchorTime, context.TimeRangeEnd); context.CancellationToken.ThrowIfCancellationRequested(); - var (before, after, beforeCoverage, afterCoverage, dispersion) = await ComparePeriodsAsync( + var (before, after, beforeCoverage, afterCoverage, dispersion, _, _) = await ComparePeriodsAsync( context.ServerId, context.ServerName, windows.BeforeStart, windows.BeforeEnd, windows.AfterStart, windows.AfterEnd); diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgAnomalyDetector.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgAnomalyDetector.cs index e10dd03ee3..d2f3bba7b9 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgAnomalyDetector.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgAnomalyDetector.cs @@ -473,6 +473,7 @@ prv AS (SELECT database_name, object_id, SUM(reserved_mb) mb WHERE (SELECT t FROM latest) <> (SELECT t FROM prior) AND cur.mb - prv.mb >= $2 AND (CASE WHEN prv.mb > 0 THEN (cur.mb - prv.mb) * 100.0 / prv.mb ELSE 0 END) >= $3 +/*SEC*/ ORDER BY growth_mb DESC LIMIT 1"; public const string ObjectContentionSql = @" @@ -512,6 +513,9 @@ private async Task DetectObjectStatsAnomalies(AnalysisContext context, List(); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Queries.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Queries.cs index 99492d7197..7698a1f499 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Queries.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Queries.cs @@ -819,6 +819,7 @@ FROM v_query_store_stats -- matched independently, which admits every pairing of the listed databases and query ids: a superset -- of the offenders, never a subset. AND ($5::text[] IS NULL OR (database_name = ANY($5::text[]) AND query_id = ANY($6::bigint[]))) + /*SEC*/ ), plan_dedup AS ( @@ -877,6 +878,7 @@ FROM query_store_interval_latest AND collection_time >= $7 AND first_execution_time >= $7 AND ($5::text[] IS NULL OR (database_name = ANY($5::text[]) AND query_id = ANY($6::bigint[]))) + /*SEC*/ GROUP BY database_name, query_id, replica_role, query_plan_hash HAVING SUM(execution_count) >= 25 ), @@ -1043,6 +1045,12 @@ test on each row (#4821). */ cmd.Parameters.AddWithValue(windowStart.AddDays(-1)); } + /* #5558: the same databases the PLAN_REGRESSION fact skipped, for the pass where the fact did not run and the + offender list above is NULL. The predicate sits in the source CTE, ahead of the LIMIT 5, so the five rows are + the worst five of the databases this node reports (Lite's twin does the same); a filter after the LIMIT could + leave fewer. Bound last, so its parameter number follows every parameter above. */ + PgSecondaryReplicaScope.Apply(cmd, context, "PLAN_REGRESSION", "database_name"); + ServerClock? clock = null; var items = new List(); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Storage.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Storage.cs index f711b3c4c8..e3075d6780 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Storage.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Storage.cs @@ -77,6 +77,7 @@ FROM latest AND is_percent_growth = true AND total_size_mb >= 10240 AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb') +/*SEC*/ ORDER BY total_size_mb DESC LIMIT 50"; @@ -99,6 +100,8 @@ private async Task CollectAutogrowthPercentFiles(AnalysisFinding finding, Analys cmd.Parameters.AddWithValue(context.ServerId); cmd.Parameters.AddWithValue(AsNaive(context.LatestValueStartFor(DatabaseSizeStatsCollector.Instance.Name))); cmd.Parameters.AddWithValue(AsNaive(context.TimeRangeEnd)); + /* #5558: the same databases the fact skipped. */ + PgSecondaryReplicaScope.Apply(cmd, context, "FILE_AUTOGROWTH_PERCENT", "database_name"); var items = new List(); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Config.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Config.cs index 740395bc47..ac0da0b6d6 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Config.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Config.cs @@ -160,6 +160,7 @@ WITH latest AS ( FROM database_config WHERE server_id = $1 AND capture_time = (SELECT MAX(capture_time) FROM database_config WHERE server_id = $1) + /*SEC*/ ) SELECT COUNT(*) AS database_count, @@ -187,6 +188,9 @@ private async Task CollectDatabaseConfigFactsAsync(AnalysisContext context, List using var cmd = new NpgsqlCommand(DatabaseConfigSql, connection) { CommandTimeout = FactCommandTimeoutSeconds }; cmd.Parameters.AddWithValue(context.ServerId); + /* #5558: a database this node holds only as a secondary copy is the primary's to report (and drops out of + every count here, database_count included). */ + PgSecondaryReplicaScope.Apply(cmd, context, "DB_CONFIG", "database_name"); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); if (!await reader.ReadAsync(context.CancellationToken)) return; diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs index e65c2ffa26..7bc51543a4 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs @@ -565,6 +565,7 @@ internal static DateTime PlanRegressionLiveFloor(DateTime windowFloor, IReadOnly bool_or(is_forced_plan) AS is_forced_plan, MAX(force_failure_count) AS force_failure_count FROM plan_agg + /*SEC*/ GROUP BY database_name, query_id, replica_role, query_plan_hash HAVING SUM(execs) >= 25 ), @@ -743,6 +744,11 @@ not swallowed. */ } } + /* #5558: Query Store on a secondary copy is the primary's content, and rows from the days this node was + primary stay in the window, so the read skips those databases. The marker sits in the suffix all three + variants share, so they cannot drift; bound after every other parameter. */ + PgSecondaryReplicaScope.Apply(cmd, context, "PLAN_REGRESSION", "database_name", "WHERE"); + var offenderCount = 0; var worstFactor = 0.0; var worstQueryId = 0L; diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Storage.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Storage.cs index d0243b5dab..da5a723db0 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Storage.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Storage.cs @@ -351,7 +351,8 @@ FROM latest WHERE rn = 1 AND is_percent_growth = true AND total_size_mb >= 10240 -AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb')"; +AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb') +/*SEC*/"; /// /// Collects the percent-autogrowth-on-large-files config fact (WS3): data/log files set @@ -372,6 +373,8 @@ private async Task CollectFileAutogrowthFactsAsync(AnalysisContext context, List cmd.Parameters.AddWithValue(context.ServerId); cmd.Parameters.AddWithValue(AsNaive(context.LatestValueStartFor(DatabaseSizeStatsCollector.Instance.Name))); cmd.Parameters.AddWithValue(AsNaive(context.TimeRangeEnd)); + /* #5558: the file growth setting replicates from the primary; a secondary copy is the primary's to report. */ + PgSecondaryReplicaScope.Apply(cmd, context, "FILE_AUTOGROWTH_PERCENT", "database_name"); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); if (!await reader.ReadAsync(context.CancellationToken)) return; diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs new file mode 100644 index 0000000000..d982f930bf --- /dev/null +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs @@ -0,0 +1,150 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Npgsql; +using PerformanceMonitor.Analysis; +using PerformanceMonitor.Common; + +namespace PerformanceMonitor.Darling.Analysis; + +/// +/// #5558, Darling's half of Lite's SecondaryReplicaScope: resolves +/// from the store's own ag_replica_states and +/// ag_database_replica_states snapshots at or before the pass's window end, and builds the predicate the +/// replicated facts add. The decision is 's; this class only reads rows. Every failure +/// is "skip nothing". +/// +public static class PgSecondaryReplicaScope +{ + /// The marker a replicated read puts in its SQL where the secondary-database predicate goes. + public const string Marker = "/*SEC*/"; + + /// The set a pass that reads only node-local facts carries: nothing is skipped, and no AG read is paid for. + public static IReadOnlySet NoneSkipped { get; } = new HashSet(StringComparer.Ordinal); + + /// Fills the set once per context. Never throws except an abandonment: a read fault logs and leaves the pass unfiltered. + public static async Task EnsureAsync(NpgsqlDataSource postgres, AnalysisContext context, Microsoft.Extensions.Logging.ILogger? logger) + { + if (context.SecondaryReplicaDatabases is not null) return; + context.SecondaryReplicaDatabases = await ReadAsync(postgres, context.ServerId, context.TimeRangeEnd, logger, context.CancellationToken); + } + + /// The secondary databases as of ; empty on every failure or unknown. + public static async Task> ReadAsync( + NpgsqlDataSource postgres, int serverId, DateTime asOfUtc, + Microsoft.Extensions.Logging.ILogger? logger, CancellationToken cancellationToken) + { + try + { + await using var connection = await postgres.OpenConnectionAsync(cancellationToken); + var asOf = DateTime.SpecifyKind(asOfUtc, DateTimeKind.Unspecified); + + var replicaTime = await NewestAsync(connection, "ag_replica_states", serverId, asOf, cancellationToken); + var databaseTime = await NewestAsync(connection, "ag_database_replica_states", serverId, asOf, cancellationToken); + if (!AgReplicaScope.IsFresh(replicaTime, asOfUtc) || !AgReplicaScope.IsFresh(databaseTime, asOfUtc)) + return new HashSet(StringComparer.Ordinal); + + var replicas = new List(); + using (var cmd = new NpgsqlCommand( + "SELECT ag_name, replica_server_name, role_desc, is_local FROM ag_replica_states WHERE server_id = $1 AND collection_time = $2", connection) + { CommandTimeout = DarlingAnalysisService.AnalysisCommandTimeoutSeconds }) + { + cmd.Parameters.AddWithValue(serverId); + cmd.Parameters.AddWithValue(DateTime.SpecifyKind(replicaTime!.Value, DateTimeKind.Unspecified)); + using var reader = await cmd.ExecuteReaderAsync(cancellationToken); + while (await reader.ReadAsync(cancellationToken)) + { + if (reader.IsDBNull(0)) continue; + replicas.Add(new AgReplicaReading( + AgName: reader.GetString(0), + ReplicaServerName: reader.IsDBNull(1) ? "" : reader.GetString(1), + RoleDesc: reader.IsDBNull(2) ? null : reader.GetString(2), + ConnectedStateDesc: null, + IsLocal: reader.IsDBNull(3) ? null : reader.GetBoolean(3))); + } + } + + var databases = new List(); + using (var cmd = new NpgsqlCommand( + "SELECT ag_name, database_name, is_local FROM ag_database_replica_states WHERE server_id = $1 AND collection_time = $2", connection) + { CommandTimeout = DarlingAnalysisService.AnalysisCommandTimeoutSeconds }) + { + cmd.Parameters.AddWithValue(serverId); + cmd.Parameters.AddWithValue(DateTime.SpecifyKind(databaseTime!.Value, DateTimeKind.Unspecified)); + using var reader = await cmd.ExecuteReaderAsync(cancellationToken); + while (await reader.ReadAsync(cancellationToken)) + { + if (reader.IsDBNull(0) || reader.IsDBNull(1)) continue; + databases.Add(new AgDatabaseMembership( + reader.GetString(0), reader.GetString(1), reader.IsDBNull(2) ? null : reader.GetBoolean(2))); + } + } + + return AgReplicaScope.SecondaryDatabases(replicas, databases); + } + catch (Exception ex) when (!AnalysisShutdown.IsExpectedAbandon(ex, cancellationToken)) + { + logger?.LogWarning(ex, "Availability Group role lookup failed for server {ServerId}; no database is skipped this pass", serverId); + return new HashSet(StringComparer.Ordinal); + } + } + + private static async Task NewestAsync( + NpgsqlConnection connection, string table, int serverId, DateTime asOf, CancellationToken cancellationToken) + { + /* The table name is a constant of this class, never input. */ + /* The lower bound is the freshness limit: an older snapshot is stale and yields the empty set anyway + (AgReplicaScope.IsFresh), so the bound changes no answer but lets the hypertable exclude old chunks. */ + using var cmd = new NpgsqlCommand( + "SELECT MAX(collection_time) FROM " + table + + " WHERE server_id = $1 AND collection_time <= $2 AND collection_time >= $3", connection) + { CommandTimeout = DarlingAnalysisService.AnalysisCommandTimeoutSeconds }; + cmd.Parameters.AddWithValue(serverId); + cmd.Parameters.AddWithValue(asOf); + cmd.Parameters.AddWithValue(asOf - AgReplicaScope.SnapshotFreshness); + var value = await cmd.ExecuteScalarAsync(cancellationToken); + return value is DateTime at ? DateTime.SpecifyKind(at, DateTimeKind.Utc) : null; + } + + /// The note for the viewer, web and MCP, or null when nothing is skipped. The role is the current one, + /// or the role at for an AsOf read of stored findings. + public static async Task NoteAsync( + NpgsqlDataSource postgres, int serverId, Microsoft.Extensions.Logging.ILogger? logger, CancellationToken cancellationToken, + DateTime? asOfUtc = null) + { + var set = await ReadAsync(postgres, serverId, asOfUtc ?? DateTime.UtcNow, logger, cancellationToken); + return AgReplicaScope.SkippedNote(set); + } + + /// + /// Replaces in the command's text with the predicate that drops the skipped databases of + /// (), binding the names (exact casing) as one text[] + /// parameter numbered after the parameters already added. is AND for a clause that + /// extends a WHERE and WHERE for one that opens it. With nothing to skip the marker just disappears: the SQL is + /// the same as before and no parameter is added. Call it after every other parameter is bound. + /// + public static void Apply(NpgsqlCommand cmd, AnalysisContext context, string factKey, string column, string keyword = "AND") + { + var names = FactReplicaScope.SecondariesFor(context, factKey); + if (names.Length == 0) + { + cmd.CommandText = cmd.CommandText.Replace(Marker, string.Empty, StringComparison.Ordinal); + return; + } + + var index = cmd.Parameters.Count + 1; + cmd.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlTypes.NpgsqlDbType.Array | NpgsqlTypes.NpgsqlDbType.Text, Value = names }); + cmd.CommandText = cmd.CommandText.Replace( + Marker, $" {keyword} ({column} IS NULL OR {column} <> ALL(${index}::text[]))", StringComparison.Ordinal); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs index 5104e1b02b..b95cc46f6f 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs @@ -10641,6 +10641,8 @@ surviving findings (the STORAGE identity, same as every collected row), the dril ServerName = runtime.StorageName, TimeRangeStart = latest.AddMinutes(-SameStatementPileupDetector.BaselineLookbackMinutes), TimeRangeEnd = latest, + /* #5558: the same-statement pileup is a node-local finding (queries that piled up on THIS node), so there is nothing to skip. */ + SecondaryReplicaDatabases = PerformanceMonitor.Darling.Analysis.PgSecondaryReplicaScope.NoneSkipped, CancellationToken = stoppingToken, ShutdownToken = stoppingToken, }; diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpFinOpsRecommendationsTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpFinOpsRecommendationsTools.cs index 92fbe4a6c3..332e86db7e 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpFinOpsRecommendationsTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpFinOpsRecommendationsTools.cs @@ -17,6 +17,7 @@ using ModelContextProtocol.Server; using Npgsql; using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Analysis; using PerformanceMonitor.Darling.Storage.FinOps; namespace PerformanceMonitor.Darling.Service.Mcp; @@ -30,7 +31,7 @@ namespace PerformanceMonitor.Darling.Service.Mcp; public sealed class DarlingMcpFinOpsRecommendationsTools { private const string RecommendationsGuide = - " est_savings_usd_month is a rough monthly USD estimate rounded to cents; null when the check gives none or no monthly cost is set (monthly_cost_usd null, cost_reason 'monthly cost not set'). severity and confidence are High, Medium or Low. finding and detail are English text with numbers in invariant format (1,234.5; a percent reads '20 %'). skipped_checks names each check whose read failed: its rows are absent, not clean. An empty recommendations list with no skipped_checks means no check found anything."; + " est_savings_usd_month is a rough monthly USD estimate rounded to cents; null when the check gives none or no monthly cost is set (monthly_cost_usd null, cost_reason 'monthly cost not set'). severity and confidence are High, Medium or Low. finding and detail are English text with numbers in invariant format (1,234.5; a percent reads '20 %'). skipped_checks names each check whose read failed: its rows are absent, not clean. An empty recommendations list with no skipped_checks means no check found anything. secondary_replica_note is null unless this server holds a secondary copy of some database in an availability group, and then names how many were left out: idle, dev/test, TDE and compression findings leave those databases out (the primary reports them), and the low IO latency finding keeps them."; [McpServerTool(Name = "get_finops_recommendations"), Description( "FinOps recommendations for one server: the cost and right-sizing findings the desktop Recommendations tab shows, High severity first. Mixed fixed windows ending now (CPU 24 hours and 7 days; memory, jobs and file I/O 7 days; edition and database facts from the latest snapshot); UTC; no hours_back, limit or as_of. At most about 21 rows. <>" + RecommendationsGuide)] @@ -47,6 +48,11 @@ public static async Task GetFinOpsRecommendations( var monthly = await FinOpsUtilizationFigures.GetMonthlyCostUsdAsync( postgres, resolved.ServerId, McpCommandDeadlines.ReadSeconds, cancellationToken); + /* #5558: databases this node holds only as an Availability Group secondary copy are left to the primary's findings. + Fails open (stale or missing snapshots, standalone, Azure SQL Database skip nothing). */ + var secondary = await PgSecondaryReplicaScope.ReadAsync( + postgres, resolved.ServerId, DateTime.UtcNow, logger: null, cancellationToken); + /* The composer swallows every failed check; the hook names them. A cancelled request is not a failed check. */ var skipped = new List(); var rows = await InInvariantCultureAsync(() => DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( @@ -56,10 +62,11 @@ public static async Task GetFinOpsRecommendations( if (ex is OperationCanceledException && cancellationToken.IsCancellationRequested) return; if (!skipped.Contains(label, StringComparer.Ordinal)) skipped.Add(label); }, - cancellationToken)); + secondaryDatabases: secondary, cancellationToken: cancellationToken)); cancellationToken.ThrowIfCancellationRequested(); - return JsonSerializer.Serialize(Envelope(resolved.ServerName, monthly, skipped, rows), McpHelpers.JsonOptions); + return JsonSerializer.Serialize( + Envelope(resolved.ServerName, monthly, skipped, rows, AgReplicaScope.SkippedNote(secondary)), McpHelpers.JsonOptions); } catch (Exception ex) when (ex is not OperationCanceledException) { @@ -97,7 +104,8 @@ internal static async Task InInvariantCultureAsync(Func> body) }; /// The response envelope. Rows keep the composer's order. - internal static object Envelope(string server, decimal monthly, IReadOnlyList skipped, IReadOnlyList rows) + internal static object Envelope(string server, decimal monthly, IReadOnlyList skipped, IReadOnlyList rows, + string? secondaryNote = null) { var hasCost = monthly > 0m; return new @@ -107,6 +115,9 @@ internal static object Envelope(string server, decimal monthly, IReadOnlyList GetAnalysisFacts( var (facts, coverage, collection) = await analysisService.CollectAndScoreFactsAsync( resolved.ServerId, resolved.ServerName, hours_back, asOfUtc: anchor, cancellationToken: cancellationToken); + /* #5558: databases left to the primary because this node holds only a secondary copy of them in an + availability group, as of the window end; the facts below were trimmed of them. Built from the set the + read's own context filtered with (carried on the returned collection state), not from a second read, so + the note cannot disagree with what was skipped. Null when none, and then the field is simply null. */ + var secondaryReplicaNote = AgReplicaScope.SkippedNote(collection.SecondaryReplicaDatabases); + /* #3691: null on a clean read, and then every envelope below is byte-for-byte what it was; when a family failed, the sentence is appended to the message and collection_caveats to the payload. */ var collectionCaveat = collection.Describe(); @@ -343,12 +357,11 @@ public static async Task GetAnalysisFacts( #3691: a pass in which every family FAILED lands here too, and used to be told apart from "no data" only by the service log; the caveat now says which families were not read. */ const string noFacts = "No facts collected. The collector may not have run yet, or no data exists in the requested time range."; - return collectionCaveat is null - ? McpHelpers.Status("unavailable", noFacts) - : McpHelpers.Status( - "unavailable", - $"{noFacts} COLLECTION CAVEAT: {collectionCaveat}", - collection.Attach(new { coverage = coverage?.ToPayload() }, McpHelpers.JsonOptions)); + var noFactsMessage = secondaryReplicaNote is null ? noFacts : $"{noFacts} {secondaryReplicaNote}"; + return McpHelpers.Status( + "unavailable", + collectionCaveat is null ? noFactsMessage : $"{noFactsMessage} COLLECTION CAVEAT: {collectionCaveat}", + collection.Attach(new { secondary_replica_note = secondaryReplicaNote, coverage = coverage?.ToPayload() }, McpHelpers.JsonOptions)); } if (coverage is null || !coverage.IsObserved) @@ -365,7 +378,7 @@ after configuration alone knows audit_config still has it. */ $"{facts.Count} point-in-time fact(s) — configuration and current state — could still be read; audit_config reports those. " + "Check get_collection_health to see when collectors last succeeded and why they stopped." + (collectionCaveat is null ? string.Empty : $" COLLECTION CAVEAT: {collectionCaveat}"), - collection.Attach(new { coverage = coverage?.ToPayload() }, McpHelpers.JsonOptions)); + collection.Attach(new { secondary_replica_note = secondaryReplicaNote, coverage = coverage?.ToPayload() }, McpHelpers.JsonOptions)); } var filtered = facts.AsEnumerable(); @@ -417,6 +430,7 @@ after configuration alone knows audit_config still has it. */ total_facts = facts.Count, shown = result.Count, filters = new { source, min_severity }, + secondary_replica_note = secondaryReplicaNote, /* #5558: null when nothing was left to the primary */ /* #3538 A2: null at full coverage; below the partial bar it says what share of the window the fractions and rates were divided over, because a 25%-of-observed-time wait on a quarter-collected window is a different claim from 25% of four hours. #3691: the @@ -467,12 +481,18 @@ silently change what the two windows are relative to each other. */ var baselineEnd = windowEnd.AddHours(-baseline_hours_back + hours_back); var baselineStart = windowEnd.AddHours(-baseline_hours_back); - var (baselineFacts, comparisonFacts, baselineCoverage, comparisonCoverage, dispersion) = await analysisService.ComparePeriodsAsync( + var (baselineFacts, comparisonFacts, baselineCoverage, comparisonCoverage, dispersion, baselineSecondaries, comparisonSecondaries) = await analysisService.ComparePeriodsAsync( resolved.ServerId, resolved.ServerName, baselineStart, baselineEnd, comparisonStart, comparisonEnd, cancellationToken); + /* #5558: per window, because each window uses the role at its own end and the two can straddle a + failover. Built from the sets the windows' contexts filtered with (returned beside the coverages), not + from a second read, so the note names exactly what the comparison skipped. */ + var baselineSecondaryNote = AgReplicaScope.SkippedNote(baselineSecondaries); + var comparisonSecondaryNote = AgReplicaScope.SkippedNote(comparisonSecondaries); + /* The COLLECTION_GAP context fact (#3538 A2) is an observation of the COLLECTOR, not of the server, and it is reported through the coverage blocks and caveat below. Left in the comparison it would count as a "stable" (or, on one side, a "new") entry in the summary @@ -523,7 +543,9 @@ in hand are the whole answer. it observed and found idle — is the difference between "check collection" and "the server was quiet", and only the coverage can tell them apart. */ baseline_coverage = baselineCoverage?.ToPayload(), - comparison_coverage = comparisonCoverage?.ToPayload() + comparison_coverage = comparisonCoverage?.ToPayload(), + baseline_secondary_replica_note = baselineSecondaryNote, + comparison_secondary_replica_note = comparisonSecondaryNote }); } @@ -580,14 +602,16 @@ description is not in front of the reader when the numbers are. */ start = baselineStart.ToString("o"), end = baselineEnd.ToString("o"), fact_count = baselineServerFacts.Count, - coverage = baselineCoverage?.ToPayload() + coverage = baselineCoverage?.ToPayload(), + secondary_replica_note = baselineSecondaryNote }, comparison = new { start = comparisonStart.ToString("o"), end = comparisonEnd.ToString("o"), fact_count = comparisonServerFacts.Count, - coverage = comparisonCoverage?.ToPayload() + coverage = comparisonCoverage?.ToPayload(), + secondary_replica_note = comparisonSecondaryNote }, summary = comparison.SummaryPayload(), families = comparison.Families.Select(f => f.ToPayload()).ToList(), @@ -1128,11 +1152,16 @@ exactly when the window held an occurrence the newest-first LIMIT dropped. This resolved.ServerId, hours_back, FindingOccurrences.WindowCoveringLimit + 1, asOfUtc: anchor, cancellationToken: cancellationToken); var (findings, truncated) = McpHelpers.BoundPage(fetched, FindingOccurrences.WindowCoveringLimit); + /* #5558: databases skipped because this node holds only a secondary copy of them in an availability group + (their per-database findings are the primary's). Null when none; the same sentence on every surface. */ + var secondaryReplicaNote = await PgSecondaryReplicaScope.NoteAsync(postgres, resolved.ServerId, null, cancellationToken, anchor); + if (findings.Count == 0) { return McpHelpers.Status( "empty", - "No findings in the requested time range. Run analyze_server to generate new findings."); + "No findings in the requested time range. Run analyze_server to generate new findings." + + (secondaryReplicaNote is null ? "" : " " + secondaryReplicaNote)); } /* #2000: collapse to one entry per (story_path_hash, incident_id). Measured 27.9x @@ -1180,6 +1209,7 @@ than the fleet mean (~15/server/24h after collapse), and unlike the raw-read cap finding_count = groups.Count, total_finding_count = totalFindingCount, total_occurrences = findings.Count, + secondary_replica_note = secondaryReplicaNote, // No silent caps: a read the window-covering limit CUT has had its OLDEST rows dropped by // the store's newest-first LIMIT, so occurrence stats may under-report — say so instead of // letting first_seen quietly lie. truncated is the #3594 flag (observed above); the note diff --git a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/analysis-findings.js b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/analysis-findings.js index 3469d3caa0..0310aaa089 100644 --- a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/analysis-findings.js +++ b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/analysis-findings.js @@ -179,9 +179,12 @@ export function renderFindings(server, res) { if (res.kind !== "data") return []; const data = res.data || {}; const groups = groupFindings(data.findings); - if (!groups.length) return [keptWindowStrip(res), emptyStrip("No findings in this window.")]; + /* #5558: databases left out because this node holds only a secondary copy of them in an availability group. */ + const replicaNote = typeof data.secondary_replica_note === "string" && data.secondary_replica_note ? noticeStrip(data.secondary_replica_note) : null; + if (!groups.length) return [keptWindowStrip(res), replicaNote, emptyStrip("No findings in this window.")]; return [ keptWindowStrip(res), + replicaNote, typeof data.truncation_note === "string" && data.truncation_note ? noticeStrip(data.truncation_note) : null, typeof data.findings_truncated_note === "string" && data.findings_truncated_note ? noticeStrip(data.findings_truncated_note) : null, ...groups.map((g) => groupNode(server, g)), diff --git a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/finops/recommendations.js b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/finops/recommendations.js index c4af2da50d..c7655f3c6e 100644 --- a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/finops/recommendations.js +++ b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/finops/recommendations.js @@ -52,6 +52,8 @@ export const tab = { const skipped = data.skipped_checks || []; const parts = [noticeStrip(noticeText(rows.length))]; if (data.monthly_cost_usd == null) parts.push(noticeStrip("Monthly cost not set for this server, so savings estimates are blank.")); + /* Databases this server holds only as an availability group secondary copy are left to the primary's findings (#5558). */ + if (data.secondary_replica_note) parts.push(noticeStrip(data.secondary_replica_note)); if (skipped.length) parts.push(noticeStrip("Some checks could not run, so their findings are missing (not clean): " + skipped.join(", ") + ".")); parts.push(VIZ.table({ rows }, { rowsKey: "rows", columns: COLUMNS, emptyText: "No recommendations: nothing needs attention, or not enough has been collected yet." })); mount(body, parts); diff --git a/Darling/PerformanceMonitor.Darling.Storage/FinOps/DarlingFinOpsRecommendationsReader.cs b/Darling/PerformanceMonitor.Darling.Storage/FinOps/DarlingFinOpsRecommendationsReader.cs index 7083320abc..80fe5bf60c 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/FinOps/DarlingFinOpsRecommendationsReader.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/FinOps/DarlingFinOpsRecommendationsReader.cs @@ -356,9 +356,17 @@ public static async Task> GetDatabaseEncryptionFact /// empty tab. is the per-server budget (0 → findings emit with no savings /// estimate, mirroring Lite's monthlyCost > 0 ? … : null). /// + /// Optional (#5558). The databases this node holds only as a secondary copy in an + /// Availability Group, as PgSecondaryReplicaScope.ReadAsync resolves them (that class lives in the Analysis project, + /// which depends on this one, so the caller resolves the set). The idle, dev/test, TDE-blocker and compression rules drop + /// them, because the primary reports the same finding for the same replicated database; the low IO latency rule keeps them, + /// since a replica's own storage is that replica's own cost. Null or empty skips nothing, which is how a stale snapshot, a + /// standalone server and Azure SQL Database arrive. The edition note still follows the instance-level collected role + /// (server_properties.ag_replica_role): an edition is one decision for the whole group, not a finding about a database. /// Optional. Invoked once per failed check with that check's label and the exception. /// It is not invoked for the VM check's inner CPU fallback, which degrades to the 24-hour window and still emits. - public static async Task> GetRecommendationsAsync(NpgsqlDataSource dataSource, int serverId, decimal monthlyCost, int commandTimeoutSeconds, Action? onCheckFailed = null, CancellationToken cancellationToken = default) + public static async Task> GetRecommendationsAsync(NpgsqlDataSource dataSource, int serverId, decimal monthlyCost, int commandTimeoutSeconds, Action? onCheckFailed = null, + IReadOnlyCollection? secondaryDatabases = null, CancellationToken cancellationToken = default) { var recommendations = new List(); var memoryCutoff = DateTime.SpecifyKind(DateTime.UtcNow.AddDays(-7), DateTimeKind.Unspecified); @@ -373,15 +381,23 @@ public static async Task> GetRecommendationsAsync(Npg // TDE is only the deciding factor on pre-2019 Enterprise — read the config snapshot just then. var tdeDbNames = new List(); + var tdeSecondaryOnly = false; if (isEnterprise && f.MajorVersion < 15) { - tdeDbNames = FinOpsRecommendationFigures.SelectTdeDatabaseNames( + var tdeAllNames = FinOpsRecommendationFigures.SelectTdeDatabaseNames( await GetDatabaseEncryptionFactsAsync(dataSource, serverId, commandTimeoutSeconds, cancellationToken)); + /* #5558: a secondary copy's TDE is the primary's to report. When every TDE database here is a secondary copy, + "no Enterprise-only features" would be false and the blocker is the primary's, so the audit says nothing + (a secondary-role instance keeps its own edition note, which never reads this list). */ + tdeDbNames = AgReplicaScope.WithoutSecondaries(tdeAllNames, secondaryDatabases); + tdeSecondaryOnly = tdeAllNames.Count > 0 && tdeDbNames.Count == 0 + && !string.Equals(f.AgReplicaRole.Trim(), "Secondary", StringComparison.OrdinalIgnoreCase); } - recommendations.AddRange( - FinOpsRecommendationFigures.EditionAudit( - f.Edition, f.MajorVersion, f.CpuCount, tdeDbNames, monthlyCost, f.AgReplicaRole, f.IsHadrEnabled)); + if (!tdeSecondaryOnly) + recommendations.AddRange( + FinOpsRecommendationFigures.EditionAudit( + f.Edition, f.MajorVersion, f.CpuCount, tdeDbNames, monthlyCost, f.AgReplicaRole, f.IsHadrEnabled)); } } catch (Exception ex) @@ -458,7 +474,8 @@ and cannot be resized on its own. util.PhysicalMemoryMb is the database's own me try { var indexes = await DarlingFinOpsIndexAnalysisReader.GetIndexCleanupInputsAsync(dataSource, serverId, commandTimeoutSeconds, cancellationToken); - var rec = FinOpsRecommendationFigures.Compression(indexes); + var rec = FinOpsRecommendationFigures.Compression( + indexes.Where(i => !AgReplicaScope.IsSkipped(secondaryDatabases, i.DatabaseName))); if (rec != null) { recommendations.Add(rec); @@ -477,7 +494,8 @@ and cannot be resized on its own. util.PhysicalMemoryMb is the database's own me enrolled hours ago, or one that was not collected for a stretch, has not been watched long enough to call any database idle. */ var idleDbs = (await DarlingFinOpsOptimizationReader.GetIdleDatabaseReadAsync( - dataSource, serverId, DateTime.UtcNow.AddDays(-7), commandTimeoutSeconds, cancellationToken)).Rows; + dataSource, serverId, DateTime.UtcNow.AddDays(-7), commandTimeoutSeconds, cancellationToken)).Rows + .Where(d => !AgReplicaScope.IsSkipped(secondaryDatabases, d.DatabaseName)).ToList(); if (idleDbs.Count > 0) { var allocatedTotalMb = 0m; @@ -504,7 +522,8 @@ database idle. */ try { var devDbs = FinOpsRecommendationFigures.MatchDevTestDatabases( - (await GetDatabaseEncryptionFactsAsync(dataSource, serverId, commandTimeoutSeconds, cancellationToken)).Select(r => r.DatabaseName)); + (await GetDatabaseEncryptionFactsAsync(dataSource, serverId, commandTimeoutSeconds, cancellationToken)) + .Select(r => r.DatabaseName).Where(name => !AgReplicaScope.IsSkipped(secondaryDatabases, name))); var devTestRecommendation = FinOpsRecommendationFigures.DevTest(devDbs); if (devTestRecommendation != null) recommendations.Add(devTestRecommendation); diff --git a/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.Recommendations.cs b/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.Recommendations.cs index 8edaf19f4f..84cba63551 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.Recommendations.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.Recommendations.cs @@ -25,8 +25,11 @@ public partial class FinOpsTab /// Reads the collected recommendations for this server and repaints the grid + count indicator. private async Task LoadFinOpsRecommendationsAsync() { - var data = await _dataService.GetRecommendationsAsync(_server.ServerId, _server.MonthlyCostUsd); + var (data, skippedNote) = await _dataService.GetRecommendationsWithNoteAsync(_server.ServerId, _server.MonthlyCostUsd); + /* #5558: databases this server holds only as an Availability Group secondary copy are left to the primary's findings. */ + FinOpsRecommendationsSecondaryNote.Text = skippedNote ?? ""; + FinOpsRecommendationsSecondaryNote.Visibility = skippedNote is null ? Visibility.Collapsed : Visibility.Visible; _finopsRecommendationsFilterMgr!.UpdateData(data); FinOpsRecommendationsNoDataMessage.Visibility = data.Count == 0 ? Visibility.Visible : Visibility.Collapsed; FinOpsRecommendationsCountIndicator.Text = data.Count > 0 diff --git a/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.xaml b/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.xaml index 1627e6f189..04dce43efd 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.xaml +++ b/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.xaml @@ -1166,6 +1166,7 @@ + @@ -1177,7 +1178,9 @@ - + + +