cpu_scheduler_stats no longer stores or reads ambiguous same-instant snapshots (#3936)

Darling/Darling.Tests/CollectionTimeClockTests.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 PerformanceMonitor.Collectors;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #3936: <see cref="CollectionTimeClock"/> is the write-side half of the fix — the reader-side half
+/// (<c>collection_id DESC</c> tiebreaks) is pinned per-reader in <c>DarlingFleetReaderTests</c>,
+/// <c>DarlingMcpPlanCacheSchedulerToolsTests</c>, <c>ViewerCpuSchedulerTests</c>,
+/// <c>ViewerW2aTests</c> and <c>PgFactCollectorTests</c> (Lite's twins in <c>LocalDataServiceTests</c> /
+/// <c>DuckDbFactCollectorTests</c>-adjacent coverage). Every (server, collector) key here is a distinct,
+/// never-reused sentinel — the clock's backing dictionary is process-static and never cleared, so two tests
+/// sharing a key would observe each other's stamps.
+/// </summary>
+public sealed class CollectionTimeClockTests
+{
+    [Fact]
+    public void NextStrictlyAfter_ReturnsTheInputUnchanged_OnTheFirstCallForAKey()
+    {
+        var now = new DateTime(2026, 9, 23, 6, 0, 0, DateTimeKind.Utc);
+        var result = CollectionTimeClock.NextStrictlyAfter(-393601, "cpu_scheduler_stats", now);
+        Assert.Equal(now, result);
+    }
+
+    [Fact]
+    public void NextStrictlyAfter_ReturnsTheInputUnchanged_WhenItIsAlreadyLaterThanTheLastStamp()
+    {
+        const int serverId = -393602;
+        const string collector = "cpu_scheduler_stats";
+        var first = new DateTime(2026, 9, 23, 6, 0, 0, DateTimeKind.Utc);
+        var second = first.AddMinutes(1);
+
+        CollectionTimeClock.NextStrictlyAfter(serverId, collector, first);
+        var result = CollectionTimeClock.NextStrictlyAfter(serverId, collector, second);
+
+        Assert.Equal(second, result);
+    }
+
+    /// <summary>
+    /// The reported shape (#3936): a retried run, an orphaned config-reload body, or plain clock-resolution
+    /// coincidence hands the SAME instant to two runs of the SAME (server, collector) pair. Without the nudge
+    /// both would stamp their row with an IDENTICAL collection_time, making "the newest reading" ambiguous.
+    /// </summary>
+    [Fact]
+    public void NextStrictlyAfter_NudgesForwardByOneStoredMicrosecond_WhenItWouldCollideWithTheLastStamp()
+    {
+        const int serverId = -393603;
+        const string collector = "cpu_scheduler_stats";
+        var collidingInstant = new DateTime(2026, 9, 23, 6, 0, 0, DateTimeKind.Utc);
+
+        var firstRun = CollectionTimeClock.NextStrictlyAfter(serverId, collector, collidingInstant);
+        var secondRun = CollectionTimeClock.NextStrictlyAfter(serverId, collector, collidingInstant);
+
+        Assert.Equal(collidingInstant, firstRun);
+        Assert.Equal(collidingInstant.AddMicroseconds(1), secondRun);
+        Assert.True(secondRun > firstRun, "two runs stamped with the same instant must not store the same collection_time");
+    }
+
+    [Fact]
+    public void NextStrictlyAfter_KeepsNudgingForward_AcrossRepeatedCollisions()
+    {
+        const int serverId = -393604;
+        const string collector = "cpu_scheduler_stats";
+        var collidingInstant = new DateTime(2026, 9, 23, 6, 0, 0, DateTimeKind.Utc);
+
+        var run1 = CollectionTimeClock.NextStrictlyAfter(serverId, collector, collidingInstant);
+        var run2 = CollectionTimeClock.NextStrictlyAfter(serverId, collector, collidingInstant);
+        var run3 = CollectionTimeClock.NextStrictlyAfter(serverId, collector, collidingInstant);
+
+        Assert.Equal(collidingInstant, run1);
+        Assert.Equal(collidingInstant.AddMicroseconds(1), run2);
+        Assert.Equal(collidingInstant.AddMicroseconds(2), run3);
+    }
+
+    /// <summary>A clock that appears to move backwards (NTP step, VM migration) must not un-advance the
+    /// stamp either — the same "strictly after the last one" guarantee, from the other direction.</summary>
+    [Fact]
+    public void NextStrictlyAfter_NudgesForward_WhenTheClockAppearsToGoBackwards()
+    {
+        const int serverId = -393605;
+        const string collector = "cpu_scheduler_stats";
+        var later = new DateTime(2026, 9, 23, 6, 0, 0, DateTimeKind.Utc);
+        var earlier = later.AddMinutes(-5);
+
+        CollectionTimeClock.NextStrictlyAfter(serverId, collector, later);
+        var result = CollectionTimeClock.NextStrictlyAfter(serverId, collector, earlier);
+
+        Assert.Equal(later.AddMicroseconds(1), result);
+    }
+
+    [Fact]
+    public void NextStrictlyAfter_TracksEachServerCollectorPairIndependently()
+    {
+        var now = new DateTime(2026, 9, 23, 6, 0, 0, DateTimeKind.Utc);
+
+        /* Same instant, two different keys — neither the server id nor the collector name may leak into
+           the other's history. */
+        var serverA = CollectionTimeClock.NextStrictlyAfter(-393606, "cpu_scheduler_stats", now);
+        var serverB = CollectionTimeClock.NextStrictlyAfter(-393607, "cpu_scheduler_stats", now);
+        var collectorA = CollectionTimeClock.NextStrictlyAfter(-393608, "cpu_scheduler_stats", now);
+        var collectorB = CollectionTimeClock.NextStrictlyAfter(-393608, "memory_stats", now);
+
+        Assert.Equal(now, serverA);
+        Assert.Equal(now, serverB);
+        Assert.Equal(now, collectorA);
+        Assert.Equal(now, collectorB);
+    }
+
+    /// <summary>
+    /// #3936: PostgreSQL and DuckDB keep collection_time to the microsecond and truncate below it, so two runs
+    /// whose instants differ only in ticks under one microsecond would still store ONE collection_time. The clock
+    /// floors to the stored microsecond first, so the second run steps a whole microsecond past the first.
+    /// </summary>
+    [Fact]
+    public void NextStrictlyAfter_SeparatesTwoInstantsThatShareAStoredMicrosecond()
+    {
+        const int serverId = -393611;
+        const string collector = "cpu_scheduler_stats";
+        var microsecond = new DateTime(2026, 9, 23, 6, 0, 0, DateTimeKind.Utc);
+        var firstRun = CollectionTimeClock.NextStrictlyAfter(serverId, collector, microsecond.AddTicks(3));
+        var secondRun = CollectionTimeClock.NextStrictlyAfter(serverId, collector, microsecond.AddTicks(7));
+
+        Assert.Equal(microsecond, firstRun);
+        Assert.Equal(microsecond.AddMicroseconds(1), secondRun);
+        Assert.Equal(0, firstRun.Ticks % TimeSpan.TicksPerMicrosecond);
+        Assert.Equal(0, secondRun.Ticks % TimeSpan.TicksPerMicrosecond);
+    }
+
+    [Theory]
+    [InlineData(DateTimeKind.Utc)]
+    [InlineData(DateTimeKind.Unspecified)]
+    public void NextStrictlyAfter_PreservesTheSuppliedKind(DateTimeKind kind)
+    {
+        var serverId = kind == DateTimeKind.Utc ? -393609 : -393610;
+        var now = new DateTime(2026, 9, 23, 6, 0, 0, kind);
+
+        var result = CollectionTimeClock.NextStrictlyAfter(serverId, "cpu_scheduler_stats", now);
+
+        Assert.Equal(kind, result.Kind);
+    }
+}

Darling/Darling.Tests/DarlingFleetReaderTests.cs

@@ -95,7 +95,7 @@ every other edition (box) and null (no signal) leave both flags false so the com
     [Theory]
     [InlineData(nameof(DarlingFleetReader.FleetCpuSql), "FROM v_cpu_utilization_stats", "ORDER BY collection_time DESC, sample_time DESC")]
     [InlineData(nameof(DarlingFleetReader.FleetMemorySql), "FROM v_memory_stats", "ORDER BY collection_time DESC")]
-    [InlineData(nameof(DarlingFleetReader.FleetThreadsSql), "FROM v_cpu_scheduler_stats", "ORDER BY collection_time DESC")]
+    [InlineData(nameof(DarlingFleetReader.FleetThreadsSql), "FROM v_cpu_scheduler_stats", "ORDER BY collection_time DESC, collection_id DESC")]
     [InlineData(nameof(DarlingFleetReader.FleetMemoryPressureSql), "FROM v_memory_grant_stats", "ORDER BY collection_time DESC")]
     public void LatestSnapshotReads_AreOnePerServerProbeFromTheRegistry(string constName, string source, string ordering)
     {

Darling/Darling.Tests/DarlingMcpPlanCacheSchedulerToolsTests.cs

@@ -289,6 +289,60 @@ await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, async (cleanup, cleanupCt) =
         }
     }
 
+    /// <summary>
+    /// #3936 regression: two cpu_scheduler_stats snapshots planted under ONE collection_time (a run-overlap or
+    /// clock-resolution collision, reproduced live in the field on DARLING01 — see the issue) must still read
+    /// deterministically as the NEWER of the two, not whichever the plan happens to return first. The lower
+    /// collection_id row (NORMAL) is inserted AFTER the higher one (CRITICAL) specifically so a physical/
+    /// insertion-order coincidence cannot make this pass without the collection_id DESC tiebreak actually
+    /// doing the work — reverting CpuSchedulerPressureSql's tiebreak turns this CRITICAL assertion into NORMAL.
+    /// </summary>
+    [Fact]
+    public async Task GetCpuSchedulerPressure_TwoSnapshotsShareOneCollectionTime_ReadsTheNewerByCollectionId()
+    {
+        var cs = ConnectionString;
+        Assert.SkipWhen(string.IsNullOrEmpty(cs), "Set DARLING_TEST_PG to a Postgres connection string to run the live plan-cache/scheduler-tools test.");
+
+        var ct = TestContext.Current.CancellationToken;
+        using var connection = new NpgsqlConnection(cs);
+        await connection.OpenAsync(ct);
+        await PgMigrations.MigrateAsync(connection, ct);
+        await DeleteRowsAsync(connection, ct);
+        await using var postgres = NpgsqlDataSource.Create(cs!);
+
+        var bodySucceeded = false;
+        try
+        {
+            await DarlingMcpTestData.RegisterServerAsync(connection, ServerId, ServerName, ct);
+            var tiedCollectionTime = DarlingMcpTestData.TruncateToSeconds(DateTime.UtcNow).AddMinutes(-2);
+
+            const string insertSql = @"INSERT INTO cpu_scheduler_stats (collection_id, collection_time, server_id, server_name, max_workers_count, scheduler_count, cpu_count, total_runnable_tasks_count, total_work_queue_count, total_current_workers_count, avg_runnable_tasks_count, total_active_request_count, total_queued_request_count, total_blocked_task_count, total_active_parallel_thread_count, runnable_percent, worker_thread_exhaustion_warning, runnable_tasks_warning, blocked_tasks_warning, queued_requests_warning, total_physical_memory_kb, available_physical_memory_kb, physical_memory_pressure_warning, total_node_count, nodes_online_count, offline_cpu_count, offline_cpu_warning)
+VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24,$25,$26,$27)";
+
+            /* The NEWER snapshot (higher collection_id), inserted FIRST: runnable tasks 60 > 50 -> CRITICAL. */
+            await DarlingMcpTestData.ExecAsync(connection, ct, insertSql,
+                9002L, tiedCollectionTime, ServerId, ServerName, 512, 8, 8, 60, 5L, 100, 7.5m, 40, 12, 2, 20L, 12.5m, false, true, false, true, 65536000L, 32768000L, false, 1, 1, 0, false);
+
+            /* The OLDER snapshot (lower collection_id) under the SAME collection_time, inserted SECOND (i.e.
+               physically last) so a reader that trusted insertion/scan order instead of collection_id would
+               land on THIS one: runnable tasks 5, no warnings -> NORMAL. */
+            await DarlingMcpTestData.ExecAsync(connection, ct, insertSql,
+                9001L, tiedCollectionTime, ServerId, ServerName, 512, 8, 8, 5, 0L, 10, 0.5m, 0, 0, 0, 0L, 0m, false, false, false, false, 65536000L, 60000000L, false, 1, 1, 0, false);
+
+            var cpu = await DarlingMcpPlanCacheSchedulerTools.GetCpuSchedulerPressure(postgres, ServerName);
+            DarlingMcpTestData.AssertEnvelope(cpu, ServerName, "pressure_level");
+            Assert.Contains("CRITICAL - High runnable task queue", cpu, StringComparison.Ordinal);
+            Assert.DoesNotContain("\"NORMAL\"", cpu, StringComparison.Ordinal);
+
+            bodySucceeded = true;
+        }
+        finally
+        {
+            await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, async (cleanup, cleanupCt) =>
+                await DeleteRowsAsync(cleanup, cleanupCt));
+        }
+    }
+
     private static async Task DeleteRowsAsync(NpgsqlConnection connection, System.Threading.CancellationToken ct, bool keepServer = false)
     {
         var sql = string.Join(" ", new[] { "plan_cache_stats", "cpu_scheduler_stats" }

Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Resources.cs

@@ -71,13 +71,20 @@ private async Task CollectMemoryFactsAsync(AnalysisContext context, List<Fact> f
         }
     }
 
+    /// <summary>#3936: the tiebreak is <c>collection_id DESC</c>, not a second <c>collection_time</c>. A
+    /// run-overlap or clock-resolution collision can store two DIFFERENT snapshots under one
+    /// <c>collection_time</c> — <c>collection_id</c> is the per-process monotonic counter every row already
+    /// carries, so it orders two same-instant rows the same way on every read instead of a bare
+    /// <c>LIMIT 1</c> returning either one depending on physical row order, which would otherwise make the
+    /// RUNNABLE_TASKS fact (and the THREADPOOL amplifier reading its warning flag) flip between two answers
+    /// across analysis passes with no change in the server.</summary>
     public const string RunnableTaskStatsSql = @"
 SELECT total_runnable_tasks_count, runnable_tasks_warning
 FROM cpu_scheduler_stats
 WHERE server_id = $1
 AND   collection_time >= $2
 AND   collection_time <= $3
-ORDER BY collection_time DESC
+ORDER BY collection_time DESC, collection_id DESC
 LIMIT 1";
 
     /// <summary>

Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs

@@ -916,7 +916,12 @@ public async Task<CollectorRunResult> RunAsync<TRow>(
         ServerRuntime server,
         CancellationToken cancellationToken)
     {
-        var collectionTime = DateTime.UtcNow;
+        /* #3936: nudged forward (by, in practice, a handful of ticks) rather than a bare DateTime.UtcNow
+           when it would collide with this SAME (server, collector) pair's last stamp — see
+           CollectionTimeClock's own remarks for the run-overlap and clock-resolution cases that used to
+           store two different snapshots under one collection_time, making "the newest reading" ambiguous
+           for every reader downstream. */
+        var collectionTime = CollectionTimeClock.NextStrictlyAfter(server.ServerId, definition.Name, DateTime.UtcNow);
 
         /* Some collectors don't exist on some targets (e.g. ring buffers on Azure SQL DB) —
            skip the cycle entirely, matching Lite. CollectorCatalog.AppliesTo composes the

Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingFleetReader.cs

@@ -301,7 +301,14 @@ WHERE s.is_enabled
 
     /// <summary>Latest worker-thread pressure per server (newest cpu_scheduler_stats snapshot). $ none. The
     /// per-server probe <see cref="FleetCpuSql"/> describes (#3895): 37.6 ms plus 46.0 ms of planning on
-    /// DARLING01 as a <c>DISTINCT ON</c> over every retained row.</summary>
+    /// DARLING01 as a <c>DISTINCT ON</c> over every retained row.
+    ///
+    /// <para>#3936: the tiebreak is <c>collection_id DESC</c>, not a second <c>collection_time</c>. A
+    /// run-overlap or clock-resolution collision can store two DIFFERENT snapshots under one
+    /// <c>collection_time</c> for one server — <c>collection_id</c> is the per-process monotonic counter
+    /// every row already carries (<see cref="PerformanceMonitor.Collectors.CollectionIdGenerator"/>), so it
+    /// is the one column guaranteed to order two same-instant rows the same way on every read, instead of a
+    /// bare <c>LIMIT 1</c> returning either one depending on physical row order.</para></summary>
     public const string FleetThreadsSql = $@"
 SELECT
     s.server_id,
@@ -319,7 +326,7 @@ CROSS JOIN LATERAL
         total_work_queue_count
     FROM v_cpu_scheduler_stats
     WHERE server_id = s.server_id
-    ORDER BY collection_time DESC
+    ORDER BY collection_time DESC, collection_id DESC
     LIMIT 1
 ) AS latest
 WHERE s.is_enabled

Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingPlanCacheSchedulerReader.cs

@@ -143,6 +143,12 @@ public static (string Level, string Recommendation) ClassifyPlanCacheBloat(long
     /// bound for the newest snapshot, not an aggregate — the tool's <c>hours_back</c> description says exactly
     /// that — and the anchor makes "what did the scheduler look like at 03:00 Tuesday" answerable.
     /// $1 server_id, $2 window start, $3 window end (naive UTC).
+    ///
+    /// <para>#3936: the tiebreak is <c>collection_id DESC</c>, not a second <c>collection_time</c>. A
+    /// run-overlap or clock-resolution collision can store two DIFFERENT snapshots under one
+    /// <c>collection_time</c> — <c>collection_id</c> is the per-process monotonic counter every row already
+    /// carries, so it orders two same-instant rows the same way on every read instead of a bare
+    /// <c>LIMIT 1</c> returning either one depending on physical row order.</para>
     /// </summary>
     public const string CpuSchedulerPressureSql = """
         SELECT
@@ -164,7 +170,7 @@ FROM v_cpu_scheduler_stats
         WHERE server_id = $1
         AND   collection_time >= $2
         AND   collection_time <= $3
-        ORDER BY collection_time DESC
+        ORDER BY collection_time DESC, collection_id DESC
         LIMIT 1
         """;
 

Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.CpuScheduler.cs

@@ -74,6 +74,11 @@ public sealed partial class ViewerDataService
     /// over the window, from the <c>v_cpu_scheduler_stats</c> passthrough view. A point-in-time collector
     /// (single row per collection), so the three counts plot directly — no delta normalization.
     /// $1 server_id, $2 window start, $3 window end (all naive UTC).
+    ///
+    /// <para>#3936: <c>collection_id</c> is a secondary sort, not a filter — a same-instant collision (rare,
+    /// see <see cref="PerformanceMonitor.Collectors.CollectionTimeClock"/>) still plots both real snapshots,
+    /// just in a deterministic left-to-right order instead of whatever physical row order the plan happens
+    /// to return them in.</para>
     /// </summary>
     public const string CpuSchedulerTrendSql = """
         SELECT
@@ -85,12 +90,18 @@ FROM v_cpu_scheduler_stats
         WHERE server_id = $1
         AND   collection_time >= $2
         AND   collection_time <= $3
-        ORDER BY collection_time
+        ORDER BY collection_time, collection_id
         """;
 
     /// <summary>
     /// The CPU Scheduler latest-snapshot read: the single most recent cpu_scheduler_stats row in the
     /// window (all pressure columns), for the metric grid. $1 server_id, $2 start, $3 end (naive UTC).
+    ///
+    /// <para>#3936: the tiebreak is <c>collection_id DESC</c>, not a second <c>collection_time</c>. A
+    /// run-overlap or clock-resolution collision can store two DIFFERENT snapshots under one
+    /// <c>collection_time</c> — <c>collection_id</c> is the per-process monotonic counter every row already
+    /// carries, so it orders two same-instant rows the same way on every read instead of a bare
+    /// <c>LIMIT 1</c> returning either one depending on physical row order.</para>
     /// </summary>
     public const string CpuSchedulerSnapshotSql = """
         SELECT
@@ -125,7 +136,7 @@ FROM v_cpu_scheduler_stats
         WHERE server_id = $1
         AND   collection_time >= $2
         AND   collection_time <= $3
-        ORDER BY collection_time DESC
+        ORDER BY collection_time DESC, collection_id DESC
         LIMIT 1
         """;