Trace-flag display reads anchor on the collector's newest successful run (#3999)

Darling/Darling.Tests/ViewerConfigurationTests.cs

@@ -404,6 +404,70 @@ await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, async (cleanup
         }
     }
 
+    /// <summary>
+    /// #3999: the all-off case. A capture that finds every flag off writes ZERO rows to <c>trace_flags</c>
+    /// (<c>DBCC TRACESTATUS(-1)</c> only ever lists flags that are ON), so the newest ROW can be older than
+    /// the newest SUCCESSFUL run. Before this fix, <c>capture_time = MAX(capture_time)</c> could not see that
+    /// the collector had run again and fell back to the stale ON row - reporting a flag enabled days after it
+    /// (and every other flag) was turned off. Seeded here at the exact shape: one old ON row, then a newer
+    /// SUCCESS in collection_log with no corresponding trace_flags row at all.
+    /// </summary>
+    [Fact]
+    public async Task TraceFlags_AllOffSinceNewerSuccessfulRun_ReadsNoFlags_AgainstDevPostgres()
+    {
+        var connectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG");
+        Assert.SkipWhen(string.IsNullOrEmpty(connectionString),
+            "Set DARLING_TEST_PG to a Postgres connection string to run the live trace-flags all-off test.");
+
+        using var connection = new NpgsqlConnection(connectionString);
+        await connection.OpenAsync(TestContext.Current.CancellationToken);
+        await PgMigrations.MigrateAsync(connection, TestContext.Current.CancellationToken);
+        await DeleteRowsAsync(connection, "trace_flags", TraceFlagsServerId, TestContext.Current.CancellationToken);
+        await DeleteRowsAsync(connection, "collection_log", TraceFlagsServerId, TestContext.Current.CancellationToken);
+
+        await using var viewer = new ViewerDataService(connectionString!);
+
+        var bodySucceeded = false;
+        try
+        {
+            var flagWasOnAt = TruncateToSeconds(DateTime.UtcNow.AddDays(-2));
+            var laterSuccessfulRunFoundNothingAt = flagWasOnAt.AddDays(1);
+
+            await InsertTraceFlagAsync(connection, flagWasOnAt, 1117, status: true, isGlobal: true, isSession: false);
+            await InsertCollectionLogSuccessAsync(connection, laterSuccessfulRunFoundNothingAt);
+
+            /* The control: without the newer collection_log SUCCESS, the same row still reads (proven by
+               TraceFlags_LatestCaptureWins_OrderedByFlag_AgainstDevPostgres above with no collection_log row
+               at all — the COALESCE floor covers that case). This test is the case that row's coverage
+               cannot reach: a genuinely newer successful capture that stored nothing. */
+            var rows = await viewer.GetLatestTraceFlagsAsync(TraceFlagsServerId);
+
+            Assert.Empty(rows);
+
+            bodySucceeded = true;
+        }
+        finally
+        {
+            await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, async (cleanup, cleanupCt) =>
+            {
+                await DeleteRowsAsync(cleanup, "trace_flags", TraceFlagsServerId, cleanupCt);
+                await DeleteRowsAsync(cleanup, "collection_log", TraceFlagsServerId, cleanupCt);
+            });
+        }
+    }
+
+    private static async Task InsertCollectionLogSuccessAsync(NpgsqlConnection connection, DateTime collectionTimeUtc)
+    {
+        using var command = new NpgsqlCommand(
+            "INSERT INTO collection_log (log_id, server_id, server_name, collector_name, collection_time, status) " +
+            "VALUES ($1, $2, $3, 'trace_flags', $4, 'SUCCESS')", connection);
+        command.Parameters.AddWithValue(CollectionIdGenerator.Next());
+        command.Parameters.AddWithValue(TraceFlagsServerId);
+        command.Parameters.AddWithValue("viewer-trace-flags-e2e");
+        command.Parameters.AddWithValue(DateTime.SpecifyKind(collectionTimeUtc, DateTimeKind.Unspecified));
+        await command.ExecuteNonQueryAsync(TestContext.Current.CancellationToken);
+    }
+
     private static async Task InsertServerConfigAsync(
         NpgsqlConnection connection, DateTime captureTimeUtc,
         string configurationName, long valueConfigured, long valueInUse, bool isDynamic, bool isAdvanced)

Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingCurrentConfigReader.cs

@@ -161,12 +161,29 @@ public sealed record TraceFlagReadRow(int TraceFlag, bool Status, bool IsGlobal,
 
     /// <summary>Latest trace-flags snapshot for one server — the viewer's <c>TraceFlagsSql</c> plus the trailing
     /// <c>capture_time</c> (#3541 A10). $1 server_id. A row exists only while a flag is enabled, so an empty
-    /// result means no active flags at the last capture — and, having no row, no stamp either.</summary>
+    /// result means no active flags at the last capture — and, having no row, no stamp either.
+    ///
+    /// <para><b>#3999: anchored on the collector's newest SUCCESSFUL run, not the newest row.</b> A capture
+    /// that finds every flag off writes ZERO rows (<c>DBCC TRACESTATUS(-1)</c> only ever lists flags that are
+    /// ON), so plain <c>capture_time = MAX(capture_time)</c> cannot see that capture at all and silently falls
+    /// back to an older capture that still had a flag on — reporting a flag enabled days or weeks after it (and
+    /// every other flag) was turned off. The second <c>AND</c> below closes that: it compares the newest row's
+    /// own timestamp against the newest SUCCESS this collector logged in <c>v_collection_log</c>, and if that
+    /// successful run is NEWER than the newest row, the run that ran most recently found nothing on, so the
+    /// whole predicate goes false and the read reports no flags — exact, rather than a stale fallback. The
+    /// <c>COALESCE</c> floor only matters when collection_log's retention has aged past this collector's
+    /// oldest trace_flags row (or nothing has run yet), and defaults to the pre-fix reading rather than
+    /// wrongly suppressing a real row it cannot corroborate.</para>
+    /// </summary>
     public const string TraceFlagsSql = """
         SELECT trace_flag, status, is_global, is_session, capture_time
         FROM v_trace_flags
         WHERE server_id = $1
         AND   capture_time = (SELECT MAX(capture_time) FROM v_trace_flags WHERE server_id = $1)
+        AND   (SELECT MAX(capture_time) FROM v_trace_flags WHERE server_id = $1) >= COALESCE(
+                  (SELECT MAX(collection_time) FROM v_collection_log
+                   WHERE server_id = $1 AND collector_name = 'trace_flags' AND status = 'SUCCESS'),
+                  TIMESTAMP '1900-01-01')
         ORDER BY trace_flag
         """;
 

Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.Config.cs

@@ -78,11 +78,25 @@ FROM v_query_store_health
         ORDER BY database_name
         """;
 
+    /// <summary>
+    /// #3999: anchored on the trace_flags collector's newest SUCCESSFUL run, not the newest row. A capture
+    /// that finds every flag off writes ZERO rows (<c>DBCC TRACESTATUS(-1)</c> only lists flags that are ON),
+    /// so a plain <c>capture_time = MAX(capture_time)</c> falls back to an older capture that still had a
+    /// flag on. The second <c>AND</c> compares the newest row's timestamp against the newest SUCCESS this
+    /// collector logged in <c>v_collection_log</c>; if that run is newer, it found nothing on, and the whole
+    /// predicate goes false so the read reports no flags rather than a stale one. The <c>COALESCE</c> floor
+    /// only matters once collection_log's retention has aged past this collector's oldest trace_flags row,
+    /// and defaults to the pre-fix reading rather than wrongly suppressing a row it cannot corroborate.
+    /// </summary>
     public const string TraceFlagsSql = """
         SELECT trace_flag, status, is_global, is_session
         FROM v_trace_flags
         WHERE server_id = $1
         AND   capture_time = (SELECT MAX(capture_time) FROM v_trace_flags WHERE server_id = $1)
+        AND   (SELECT MAX(capture_time) FROM v_trace_flags WHERE server_id = $1) >= COALESCE(
+                  (SELECT MAX(collection_time) FROM v_collection_log
+                   WHERE server_id = $1 AND collector_name = 'trace_flags' AND status = 'SUCCESS'),
+                  TIMESTAMP '1900-01-01')
         ORDER BY trace_flag
         """;
 

Lite/Services/LocalDataService.Config.cs

@@ -217,6 +217,15 @@ FROM v_database_scoped_config
 
     /// <summary>
     /// Gets the latest trace flags snapshot.
+    ///
+    /// <para>#3999: anchored on the trace_flags collector's newest SUCCESSFUL run, not the newest row - the
+    /// Darling twin of this read (<c>DarlingCurrentConfigReader.TraceFlagsSql</c> /
+    /// <c>ViewerDataService.Config.TraceFlagsSql</c>) carries the same fix, and the reasoning there applies
+    /// verbatim: a capture that finds every flag off writes ZERO rows, so a plain
+    /// <c>capture_time = MAX(capture_time)</c> falls back to an older capture that still had a flag on. The
+    /// second <c>AND</c> compares the newest row's timestamp against the newest SUCCESS this collector
+    /// logged in <c>v_collection_log</c>; if that run is newer, it found nothing on, and the whole predicate
+    /// goes false so the read reports no flags rather than a stale one.</para>
     /// </summary>
     public async Task<List<TraceFlagRow>> GetLatestTraceFlagsAsync(int serverId)
     {
@@ -227,6 +236,10 @@ public async Task<List<TraceFlagRow>> GetLatestTraceFlagsAsync(int serverId)
 FROM v_trace_flags
 WHERE server_id = $1
 AND   capture_time = (SELECT MAX(capture_time) FROM v_trace_flags WHERE server_id = $1)
+AND   (SELECT MAX(capture_time) FROM v_trace_flags WHERE server_id = $1) >= COALESCE(
+          (SELECT MAX(collection_time) FROM v_collection_log
+           WHERE server_id = $1 AND collector_name = 'trace_flags' AND status = 'SUCCESS'),
+          TIMESTAMP '1900-01-01')
 ORDER BY trace_flag";
 
         command.Parameters.Add(new DuckDBParameter { Value = serverId });