From 064e62d2bc73a515db2b5f0bebe730ebbfed8d64 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sun, 26 Jul 2026 18:32:52 -0400 Subject: [PATCH] Prove the Agent-status SQL, not just the decision it feeds #1725's tests all run against constructed AgentStatusRow values. That is the right shape for pinning what a row MEANS, but it leaves the query that produces those rows unproven, and it is not trivial SQL: ever_seen_running is a window aggregate that must see the whole retained partition while the surrounding query collapses to the newest row per server. Both ways that goes wrong return a plausible boolean and pass every model-level test: - Collapse to the newest row, and a server that ran Agent yesterday and stopped today reads "never ran Agent" - so a genuine outage renders neutral. That is the failure the fix exists to prevent, reached from the other side. - Leak the partition, and one real server's history makes every Agent-less container read as a server that runs Agent, putting the red "Stopped" back exactly where #1725 removed it. Six real-DuckDB round-trips cover both, plus newest-row-wins, collection_time driving staleness, and the fresh case not being suppressed. Verified non-vacuous by mutation: dropping PARTITION BY server_id from the shipped query fails two of them. Also pins that the stale window stays longer than the collector's own cadence, read from CollectorScheduleDefaults so it follows a retune. That is a live trap, not a hypothetical - the shared ServerHealthThresholds.StaleThreshold is two minutes derived from the FASTEST collector's one-minute cadence, looks like the obvious thing to unify this with, and would render a healthy Agent as "unknown (stale)" most of the time because agent_status collects every five. Tests only. No production code; the shipped query passes as written. Lite.Tests 1510 passed / 0 failed. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 2 + Lite.Tests/AgentStatusReaderSqlTests.cs | 214 ++++++++++++++++++++++++ 2 files changed, 216 insertions(+) create mode 100644 Lite.Tests/AgentStatusReaderSqlTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index a6197a941c..1d362ca880 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Lite: prove the Agent-status SQL, not just the decision it feeds** ([#1730]) - [#1725]'s tests all run against constructed `AgentStatusRow` values, which is the right shape for pinning what a row MEANS but leaves the query that produces those rows unproven. It is not trivial SQL: `ever_seen_running` is a window aggregate that has to see the whole retained partition while the surrounding query collapses to the newest row per server, and the two ways that goes wrong both return a plausible boolean and both pass every model-level test. If it collapsed to the newest row, a server that ran Agent yesterday and stopped today would read "never ran Agent" and a **genuine outage would render neutral** - the exact failure the fix exists to prevent, arrived at from the other direction. If the partition leaked, one real server's history would make every Agent-less container read as a server that runs Agent, putting the red "Stopped" straight back where [#1725] removed it. Six real-DuckDB round-trips now cover both, plus the newest-row-wins tiebreak, `collection_time` driving staleness, and the fresh case NOT being suppressed. Confirmed non-vacuous by mutation: dropping `PARTITION BY server_id` from the shipped query fails two of them. Also pins that the stale window stays longer than the collector's own cadence, read from `CollectorScheduleDefaults` so it follows a retune - a live trap rather than a hypothetical, since the shared `ServerHealthThresholds.StaleThreshold` is two minutes derived from the FASTEST collector's one-minute cadence, looks like the obvious thing to unify this with, and would render a perfectly healthy Agent as "unknown (stale)" most of the time because `agent_status` collects every five. Tests only; no production code, and the shipped query passes as written. - **Lite: the Job History Agent indicator stops calling absence of signal a problem** ([#1725]) - the display-tier companion to the Agent-alert capability gate, closing the same misleading-by-platform class on the surface a user actually looks at. The header rendered a red **"Agent: Stopped"** in exactly two situations where nothing is wrong. A server where SQL Agent has NEVER been observed running - a container built without it, Express, a Linux-minimal image - is not stopped; nothing stopped, the target simply does not run Agent, and Lite collects in-process so nothing here depends on it. That now reads a neutral **"not present"**. And a STALE snapshot is not evidence Agent is down: a server nobody has collected from in days was rendering its last-known state in red forever. That now reads **"unknown (stale)"**, using the same 30-minute refusal window the headless service's alert path already applies, pinned equal by test so the two surfaces cannot drift into disagreeing about whether a reading is old enough to judge. Red is now reserved for the one case that means something: a FRESH reading of a genuinely stopped Agent on a server that runs one. The fleet roll-up counts only the servers the question applies to, so "2/2 running" no longer becomes "2/5 running, 3 stopped" because three containers were counted as failures. The backing read gained `collection_time` and an ever-seen-running flag to make any of this decidable - it previously returned neither, so the header had no way to tell the three cases apart. Also corrects the collector-schedule description shown in Lite's own UI, which told users `agent_status` "drives the Job History tab header and the Agent Not Running alert"; Lite raises no such alert, and now the text says so. - **Darling Web: a capped time-series panel kept the OLDEST slice of the window and said nothing** ([#1724]) - closes #991's sibling #1687, found live on DARLING01 while building the #1606 demo view. A grouped panel whose buckets x groups product exceeds the 10,000-row hard cap got `ORDER BY bucket LIMIT 10000`, which keeps the EARLIEST rows: a 24-hour wait_stats panel grouped by `wait_type` at minute grain rendered its first ~87 minutes as though that were the whole window, and because the truncated domain never crossed midnight the x-axis showed no dates, so nothing looked wrong. Same silent-truncation class as the retention-routing gaps (#1661/#1664/#1665) - the numbers are right and the WINDOW is wrong, which a monitoring product must say out loud. **Two halves, both needed.** The cap now applies `ORDER BY bucket DESC` inside a subquery and re-sorts the survivors ascending for the renderer, so a truncated chart shows the RECENT end - what a monitoring chart is for - instead of the start of the window. And when a time-series panel comes back sitting exactly on the cap it attaches a notice through the existing #1665 `notice` plumbing ("row cap reached - showing the most recent 10,000 buckets of the window, not the whole of it. Coarsen the time bucket or narrow the group-by..."), rendered by the `noticeStrip` already on every web surface and riding verbatim through MCP. Exactly-at-cap is the only signal available, because group cardinality is a property of the data rather than the spec - so a panel that legitimately produces exactly 10,000 rows is reported as capped, a false positive taken deliberately over the reverse. Retention and row-cap notices are JOINED rather than one winning, since the two truncate a panel from the same end and a long grouped panel on a retention-active store hits both. Ranked and Scalar panels are untouched: a Ranked LIMIT is the caller's own topN, so reaching it is the request being honored, not truncation. The issue's optional third item (a write-time hint) is deliberately not implemented - `MaxBuckets` (5,000) is already below the row cap, so buckets alone can never trip it and any pre-flight warning would have to guess at group cardinality it cannot know. - **Darling: "Agent Not Running" no longer nags servers that never ran SQL Agent** ([#1719]) - a capability gate for the field-noise class Erik hit within minutes of pointing the fleet at real mixed targets. The alert fired the instant a stopped Agent was first seen and re-fired every cooldown forever after, so any target where Agent is off BY DESIGN - a container built without it, Express, a Linux-minimal image - produced an endless stream of identical Critical alerts telling the operator to start a service they deliberately declined. Observed on the AG fixture containers: byte-identical alerts every five minutes on both nodes from first contact. **The framing matters more than the fix.** This alert is a full-Dashboard-era holdover: the Dashboard collected THROUGH SQL Agent jobs, so Agent down meant collection down and firing on sight was correct. Lite and Darling collect in-process and have no Agent dependency at all, so all that survives is a customer-WORKLOAD signal - "the jobs you rely on stopped running" - which is meaningless on a server that runs no Agent jobs. So a stopped Agent now alerts only where Agent has been OBSERVED RUNNING at least once; everywhere else it is permanently silent, and it re-arms by itself the moment Agent genuinely runs. That is the same first-sighting-silent discipline the connection edge three methods away already applied, restored to the one condition that was missing it. The baseline is DERIVED from collected `agent_status` history rather than remembered in process, deliberately: an in-memory flag would be forgotten on restart, which is precisely when a genuinely stopped Agent would go quiet instead of alerting. Failed-job and long-running-job alerts are untouched - real jobs failing on a real Agent are still real. No new configuration: the gate is evidence, not a knob. Lite raises no Agent service-state alert at all, so there was nothing to gate there; three Lite comments and one collector description that claimed otherwise (the schedule UI told users agent_status "drives ... the Agent Not Running alert") are corrected to say the alert is Darling's. @@ -1710,6 +1711,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1716]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1716 [#1710]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1710 [#1725]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1725 +[#1730]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1730 [#1690]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1690 [#1693]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1693 [#1694]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1694 diff --git a/Lite.Tests/AgentStatusReaderSqlTests.cs b/Lite.Tests/AgentStatusReaderSqlTests.cs new file mode 100644 index 0000000000..f53ed75eac --- /dev/null +++ b/Lite.Tests/AgentStatusReaderSqlTests.cs @@ -0,0 +1,214 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor Lite. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Threading.Tasks; +using DuckDB.NET.Data; +using PerformanceMonitor.Collectors; +using PerformanceMonitorLite.Database; +using PerformanceMonitorLite.Services; +using Xunit; + +namespace PerformanceMonitorLite.Tests; + +/// +/// Real-DuckDB round-trips for the two columns #1725 added to +/// LocalDataService.GetAgentStatusAsync: collection_time and the derived +/// ever_seen_running. +/// +/// AgentStatusHeaderHonestyTests pins the DECISION — what a row means once you have it — against +/// constructed rows, which is the right shape for that half. Nothing yet proved the SQL that produces those +/// rows, and it is not trivial SQL: ever_seen_running is a window aggregate that must see the WHOLE +/// retained partition while the surrounding query collapses to the newest row per server. The two ways that +/// goes wrong both return a plausible boolean and both pass every model-level test — it silently reports the +/// newest row's own agent_running (so a server that ran Agent yesterday and stopped today reads "never +/// ran Agent", and a genuine outage renders neutral), or the partition leaks (so an Agent-less container +/// inherits a real server's history and goes back to being called "Stopped"). Each case below fails against +/// one of those. +/// +public sealed class AgentStatusReaderSqlTests : IClassFixture, IDisposable +{ + private readonly DuckDbInitializer _duckDb; + private DuckDBConnection? _seedConn; + private long _nextId = 1; + + public AgentStatusReaderSqlTests(SharedDuckDbFixture fixture) + { + fixture.ResetData(); + _duckDb = fixture.DuckDb; + } + + public void Dispose() => _seedConn?.Dispose(); + + private async Task SeedConnectionAsync() + { + if (_seedConn is null) + { + _seedConn = _duckDb.CreateConnection(); + await _seedConn.OpenAsync(); + } + return _seedConn; + } + + private static DateTime Truncate(DateTime t) => + new(t.Year, t.Month, t.Day, t.Hour, t.Minute, t.Second, DateTimeKind.Unspecified); + + private async Task SeedAsync(int serverId, string serverName, DateTime collectionTimeUtc, bool agentRunning) + { + using var readLock = _duckDb.AcquireReadLock(); + var connection = await SeedConnectionAsync(); + using var cmd = connection.CreateCommand(); + cmd.CommandText = @" +INSERT INTO agent_status + (collection_id, collection_time, server_id, server_name, agent_running, agent_status_desc, agent_startup_desc, next_scheduled_run) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8)"; + cmd.Parameters.Add(new DuckDBParameter { Value = _nextId++ }); + cmd.Parameters.Add(new DuckDBParameter { Value = Truncate(collectionTimeUtc) }); + cmd.Parameters.Add(new DuckDBParameter { Value = serverId }); + cmd.Parameters.Add(new DuckDBParameter { Value = serverName }); + cmd.Parameters.Add(new DuckDBParameter { Value = agentRunning }); + cmd.Parameters.Add(new DuckDBParameter { Value = agentRunning ? "Running" : "Stopped" }); + cmd.Parameters.Add(new DuckDBParameter { Value = "Automatic" }); + cmd.Parameters.Add(new DuckDBParameter { Value = DBNull.Value }); + await cmd.ExecuteNonQueryAsync(); + } + + [Fact] + public async Task EverSeenRunning_ReadsTheHistory_NotJustTheNewestRow() + { + /* Agent ran, then stopped — the case the whole gate turns on. The newest row says false; the retained + history says this server genuinely runs Agent, so it must still render as a real problem. A window + aggregate that collapsed to the newest row would call this "not present" and paint it neutral, + silently swallowing a genuine outage. */ + var service = new LocalDataService(_duckDb); + + await SeedAsync(5101, "REAL", DateTime.UtcNow.AddHours(-4), agentRunning: true); + await SeedAsync(5101, "REAL", DateTime.UtcNow.AddMinutes(-3), agentRunning: false); + + var row = Assert.Single(await service.GetAgentStatusAsync(5101)); + + Assert.False(row.AgentRunning); + Assert.True(row.EverSeenRunning); + Assert.False(row.IsStale); + Assert.True(row.IsAgentProblem); + Assert.Equal("Stopped", row.StatusDisplay); + } + + [Fact] + public async Task EverSeenRunning_IsFalse_WhenNoRetainedRowEverRan() + { + /* A container built without Agent, Express, Azure SQL DB: every retained row is false. */ + var service = new LocalDataService(_duckDb); + + await SeedAsync(5102, "CONTAINER", DateTime.UtcNow.AddHours(-4), agentRunning: false); + await SeedAsync(5102, "CONTAINER", DateTime.UtcNow.AddHours(-1), agentRunning: false); + await SeedAsync(5102, "CONTAINER", DateTime.UtcNow.AddMinutes(-3), agentRunning: false); + + var row = Assert.Single(await service.GetAgentStatusAsync(5102)); + + Assert.False(row.EverSeenRunning); + Assert.False(row.IsAgentProblem); + Assert.Equal("not present", row.StatusDisplay); + } + + [Fact] + public async Task EverSeenRunning_IsPerServer_AndDoesNotLeakAcrossThePartition() + { + /* Without PARTITION BY server_id the aggregate spans every row in the table, and one real server's + history would make every Agent-less target look like it runs Agent — putting the red "Stopped" + straight back on the containers this was meant to stop nagging about. */ + var service = new LocalDataService(_duckDb); + + await SeedAsync(5103, "USES-AGENT", DateTime.UtcNow.AddHours(-2), agentRunning: true); + await SeedAsync(5103, "USES-AGENT", DateTime.UtcNow.AddMinutes(-2), agentRunning: false); + await SeedAsync(5104, "NO-AGENT", DateTime.UtcNow.AddHours(-2), agentRunning: false); + await SeedAsync(5104, "NO-AGENT", DateTime.UtcNow.AddMinutes(-2), agentRunning: false); + + var rows = await service.GetAgentStatusAsync(); + + var usesAgent = Assert.Single(rows, r => r.ServerId == 5103); + var noAgent = Assert.Single(rows, r => r.ServerId == 5104); + + Assert.True(usesAgent.EverSeenRunning); + Assert.True(usesAgent.IsAgentProblem); + + Assert.False(noAgent.EverSeenRunning); + Assert.False(noAgent.IsAgentProblem); + } + + [Fact] + public async Task CollectionTime_IsProjected_AndTheNewestRowWins() + { + var service = new LocalDataService(_duckDb); + + await SeedAsync(5105, "LAGGED", DateTime.UtcNow.AddDays(-2), agentRunning: true); + await SeedAsync(5105, "LAGGED", DateTime.UtcNow.AddHours(-6), agentRunning: false); + + var row = Assert.Single(await service.GetAgentStatusAsync(5105)); + + Assert.NotNull(row.CollectionTime); + Assert.True(row.IsStale); + Assert.False(row.IsAgentProblem); + Assert.Equal("unknown (stale)", row.StatusDisplay); + } + + [Fact] + public async Task FreshReadingIsNotStale_SoTheGateOnlyBitesOnOldData() + { + /* The other side of the staleness gate: a current reading must NOT be suppressed. */ + var service = new LocalDataService(_duckDb); + + await SeedAsync(5106, "FRESH", DateTime.UtcNow.AddMinutes(-1), agentRunning: true); + + var row = Assert.Single(await service.GetAgentStatusAsync(5106)); + + Assert.False(row.IsStale); + Assert.Equal("Running", row.StatusDisplay); + } + + [Fact] + public async Task FleetRead_ReturnsNewestRowPerServer_WithBothColumnsPopulated() + { + var service = new LocalDataService(_duckDb); + + await SeedAsync(5107, "SQL-A", DateTime.UtcNow.AddHours(-3), agentRunning: true); + await SeedAsync(5107, "SQL-A", DateTime.UtcNow.AddMinutes(-1), agentRunning: true); + await SeedAsync(5108, "SQL-B", DateTime.UtcNow.AddMinutes(-1), agentRunning: false); + + var rows = await service.GetAgentStatusAsync(); + + var a = Assert.Single(rows, r => r.ServerId == 5107); + var b = Assert.Single(rows, r => r.ServerId == 5108); + + Assert.True(a.AgentRunning); + Assert.True(a.EverSeenRunning); + Assert.NotNull(a.CollectionTime); + Assert.False(a.IsAgentProblem); + + Assert.False(b.AgentRunning); + Assert.False(b.EverSeenRunning); + Assert.NotNull(b.CollectionTime); + Assert.False(b.IsAgentProblem); + } + + [Fact] + public void StaleWindow_StaysLongerThanTheCollectorsOwnCadence() + { + /* A stale window at or under the collector's cadence would render a perfectly healthy Agent as + "unknown (stale)" most of the time. This is a live trap rather than a hypothetical: the shared + ServerHealthThresholds.StaleThreshold is two minutes, derived from the FASTEST collector's + one-minute cadence, and looks like the obvious thing to unify this with — but agent_status collects + every five, so adopting it would break the header. Pinned against the schedule default so the guard + follows the cadence if it is ever retuned. */ + var cadence = TimeSpan.FromMinutes(CollectorScheduleDefaults.All["agent_status"].FrequencyMinutes); + + Assert.True( + AgentStatusRow.StaleWindow > cadence, + $"StaleWindow ({AgentStatusRow.StaleWindow}) must exceed the agent_status cadence ({cadence})."); + } +}