Skip to content

Commit 6f534f5

Browse files
Merge pull request erikdarlingdata#1131 from erikdarlingdata/fix/pretag-review
Fix erikdarlingdata#1128 review defects before v3.0.0 tag
2 parents 56552b4 + 7d9375e commit 6f534f5

5 files changed

Lines changed: 176 additions & 28 deletions

File tree

Lite.Tests/AlertBadgeAckTests.cs

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
using System;
2+
using PerformanceMonitorLite.Services;
3+
using Xunit;
4+
5+
namespace PerformanceMonitorLite.Tests;
6+
7+
/// <summary>
8+
/// Guards the Lite server-tab badge acknowledgement logic for the low-disk (#754) and
9+
/// failed-Agent-job (#749) conditions added in #1128. Those conditions are plain booleans with
10+
/// no event timestamp, so the timestamp-based ack clear in
11+
/// <see cref="AlertStateService.UpdateAlertCounts"/> never fires for them. The review fix added
12+
/// <see cref="AlertStateService.ClearAcknowledgementForNewCondition"/> — the false-&gt;true
13+
/// transition hook MainWindow calls so a freshly-breaching disk or a brand-new failed job
14+
/// re-lights an acknowledged badge, matching the Dashboard's re-show behaviour.
15+
///
16+
/// Each test uses a unique server id so the shared (CWD-relative) alert_state.json the service
17+
/// persists to cannot leak state between tests; assertions read in-memory state, so they hold
18+
/// even when the best-effort file write is unavailable.
19+
/// </summary>
20+
public class AlertBadgeAckTests
21+
{
22+
private static string NewServerId() => "badge-test-" + Guid.NewGuid().ToString("N");
23+
24+
[Fact]
25+
public void StandingLowDisk_AfterAck_StaysSuppressed_UntilNewConditionClearsIt()
26+
{
27+
var svc = new AlertStateService();
28+
var server = NewServerId();
29+
30+
/* A standing low-disk breach lights the badge... */
31+
Assert.True(svc.UpdateAlertCounts(server, 0, 0, hasLowDisk: true, hasFailedJob: false, latestEventTimeUtc: null));
32+
33+
/* ...the user dismisses it: the badge stays suppressed even though the breach is still
34+
standing — with no event timestamp, UpdateAlertCounts can never auto-clear the ack. */
35+
svc.AcknowledgeAlert(server);
36+
Assert.False(svc.UpdateAlertCounts(server, 0, 0, hasLowDisk: true, hasFailedJob: false, latestEventTimeUtc: null));
37+
38+
/* A fresh false->true transition (re-breach / new job) clears the ack and re-lights. */
39+
var fired = false;
40+
svc.SuppressionStateChanged += (_, _) => fired = true;
41+
svc.ClearAcknowledgementForNewCondition(server);
42+
Assert.True(fired);
43+
Assert.True(svc.UpdateAlertCounts(server, 0, 0, hasLowDisk: true, hasFailedJob: false, latestEventTimeUtc: null));
44+
}
45+
46+
[Fact]
47+
public void FailedJob_AfterAck_StaysSuppressed_UntilNewConditionClearsIt()
48+
{
49+
var svc = new AlertStateService();
50+
var server = NewServerId();
51+
52+
Assert.True(svc.UpdateAlertCounts(server, 0, 0, hasLowDisk: false, hasFailedJob: true, latestEventTimeUtc: null));
53+
54+
svc.AcknowledgeAlert(server);
55+
Assert.False(svc.UpdateAlertCounts(server, 0, 0, hasLowDisk: false, hasFailedJob: true, latestEventTimeUtc: null));
56+
57+
svc.ClearAcknowledgementForNewCondition(server);
58+
Assert.True(svc.UpdateAlertCounts(server, 0, 0, hasLowDisk: false, hasFailedJob: true, latestEventTimeUtc: null));
59+
}
60+
61+
[Fact]
62+
public void ClearAcknowledgementForNewCondition_WhenNothingAcknowledged_DoesNotFireEvent()
63+
{
64+
var svc = new AlertStateService();
65+
var server = NewServerId();
66+
67+
var fired = false;
68+
svc.SuppressionStateChanged += (_, _) => fired = true;
69+
70+
/* No prior ack for this server -> nothing to clear -> no event, no save churn. */
71+
svc.ClearAcknowledgementForNewCondition(server);
72+
Assert.False(fired);
73+
74+
/* And a standing condition still lights the badge (it was never suppressed). */
75+
Assert.True(svc.UpdateAlertCounts(server, 0, 0, hasLowDisk: true, hasFailedJob: false, latestEventTimeUtc: null));
76+
}
77+
78+
[Fact]
79+
public void SilencedServer_NewCondition_StaysSuppressed()
80+
{
81+
/* ClearAcknowledgementForNewCondition only clears the *ack*; a fully-silenced server must
82+
stay dark even when a new disk/job condition appears. */
83+
var svc = new AlertStateService();
84+
var server = NewServerId();
85+
86+
svc.SilenceServer(server);
87+
svc.ClearAcknowledgementForNewCondition(server);
88+
Assert.False(svc.UpdateAlertCounts(server, 0, 0, hasLowDisk: true, hasFailedJob: true, latestEventTimeUtc: null));
89+
}
90+
}

Lite/MainWindow.xaml.cs

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1027,6 +1027,12 @@ private void CloseServerTab(string serverId)
10271027
/* Clean up alert state for this server */
10281028
_alertStateService.RemoveServerState(serverId);
10291029

1030+
/* #1128 review fix: drop the per-server badge state so a stale low-disk / failed-job flag
1031+
doesn't flash on reopen, and the dicts don't grow with tab churn. */
1032+
_badgeLowDisk.Remove(serverId);
1033+
_badgeFailedJob.Remove(serverId);
1034+
_lastBadgeCounts.Remove(serverId);
1035+
10301036
// Show empty state if no tabs open
10311037
if (_openServerTabs.Count == 0)
10321038
{
@@ -1542,6 +1548,15 @@ private async void CheckPerformanceAlerts(ServerSummaryItem summary)
15421548
failed-job conditions (#754/#749); null when the server isn't in the list. */
15431549
var badgeServer = _serverManager.GetAllServers().FirstOrDefault(s =>
15441550
RemoteCollectorService.GetDeterministicHashCode(RemoteCollectorService.GetServerNameForStorage(s)).ToString() == key);
1551+
1552+
/* #1128 review fix: snapshot the prior badge flags, recompute them as locals through the
1553+
sweep, and write them ONCE at the end — so a disabled feature / offline server clears a
1554+
stale badge (not just the active branch), and a false->true transition clears the ack. */
1555+
bool prevBadgeLowDisk = badgeServer != null && _badgeLowDisk.TryGetValue(badgeServer.Id, out var _pBadgeLd) && _pBadgeLd;
1556+
bool prevBadgeFailedJob = badgeServer != null && _badgeFailedJob.TryGetValue(badgeServer.Id, out var _pBadgeFj) && _pBadgeFj;
1557+
bool curBadgeLowDisk = false;
1558+
bool curBadgeFailedJob = false;
1559+
15451560
var alertCooldown = TimeSpan.FromMinutes(App.AlertCooldownMinutes);
15461561

15471562
/* Skip popup/email alerts if user has acknowledged or silenced this server */
@@ -1965,12 +1980,9 @@ await _emailAlertService.TrySendAlertEmailAsync(
19651980
var volumes = await Task.Run(() => _dataService.GetVolumeFreeSpaceAsync(summary.ServerId));
19661981
var breached = GetBreachedVolumes(volumes);
19671982

1968-
/* Drive the server tab badge — a breached volume is a standing condition (#754). */
1969-
if (badgeServer != null)
1970-
{
1971-
_badgeLowDisk[badgeServer.Id] = breached.Count > 0;
1972-
RefreshServerBadgeExtras(badgeServer.Id);
1973-
}
1983+
/* Drive the server tab badge — a breached volume is a standing condition (#754).
1984+
Recorded as a local; the flags are written once at the end of the sweep (#1128 review). */
1985+
curBadgeLowDisk = breached.Count > 0;
19741986

19751987
if (breached.Count > 0)
19761988
{
@@ -2137,12 +2149,9 @@ dedups so the same failure never re-fires. */
21372149
off the UI thread; MFA serialization / throttle / retry handled inside). */
21382150
var failedJobs = await _collectorService.GetRecentlyFailedJobsAsync(server, App.AlertFailedJobLookbackMinutes);
21392151

2140-
/* Drive the server tab badge — a failure in the lookback window (#749). */
2141-
if (badgeServer != null)
2142-
{
2143-
_badgeFailedJob[badgeServer.Id] = failedJobs.Count > 0;
2144-
RefreshServerBadgeExtras(badgeServer.Id);
2145-
}
2152+
/* Drive the server tab badge — a failure in the lookback window (#749). Recorded
2153+
as a local; written once at the end of the sweep (#1128 review). */
2154+
curBadgeFailedJob = failedJobs.Count > 0;
21462155

21472156
if (failedJobs.Count > 0)
21482157
{
@@ -2195,6 +2204,21 @@ await _emailAlertService.TrySendAlertEmailAsync(
21952204
AppLogger.Error("Alerts", $"Failed to check failed jobs for {summary.DisplayName}: {ex.Message}");
21962205
}
21972206
}
2207+
2208+
/* #1128 review fix: write the badge's low-disk / failed-job flags ONCE per sweep from the
2209+
values computed above. Doing it here (not only inside the feature-enabled / online / msdb
2210+
branches) means a disabled feature or an offline server clears a previously-lit badge
2211+
instead of leaving it stale. A false->true transition is a genuinely new condition, so it
2212+
clears any acknowledgement — matching the Dashboard, whose IsWorseThanBaseline re-shows on
2213+
a new disk/job condition. RefreshServerBadgeExtras re-renders once (no-op without a tab). */
2214+
if (badgeServer != null)
2215+
{
2216+
_badgeLowDisk[badgeServer.Id] = curBadgeLowDisk;
2217+
_badgeFailedJob[badgeServer.Id] = curBadgeFailedJob;
2218+
if ((curBadgeLowDisk && !prevBadgeLowDisk) || (curBadgeFailedJob && !prevBadgeFailedJob))
2219+
_alertStateService.ClearAcknowledgementForNewCondition(badgeServer.Id);
2220+
RefreshServerBadgeExtras(badgeServer.Id);
2221+
}
21982222
}
21992223

22002224
private static string TruncateText(string text, int maxLength = 300)

Lite/Services/AlertStateService.cs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,24 @@ public void AcknowledgeAlert(string serverId)
9696
SuppressionStateChanged?.Invoke(this, EventArgs.Empty);
9797
}
9898

99+
/// <summary>
100+
/// Clears a server's acknowledgement when a genuinely new badge condition (a fresh low-disk
101+
/// breach or failed Agent job) appears. Those booleans have no event timestamp of their own,
102+
/// so the timestamp-based clear in <see cref="UpdateAlertCounts"/> never fires for them; the
103+
/// caller detects the false-&gt;true transition and calls this so the badge re-lights —
104+
/// matching the Dashboard's re-show on a new disk/job condition (#1128 review).
105+
/// </summary>
106+
public void ClearAcknowledgementForNewCondition(string serverId)
107+
{
108+
bool changed;
109+
lock (_lock)
110+
{
111+
changed = _acknowledgedAlerts.Remove(serverId);
112+
if (changed) Save();
113+
}
114+
if (changed) SuppressionStateChanged?.Invoke(this, EventArgs.Empty);
115+
}
116+
99117
/// <summary>
100118
/// Silences a server entirely (no badges until unsilenced). Persisted across restarts.
101119
/// </summary>

install/25_process_deadlock_xml.sql

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ BEGIN
132132
BEGIN
133133
SELECT
134134
@start_date = MIN(dx.event_time),
135-
@end_date = DATEADD(SECOND, 1, MAX(dx.event_time))
135+
@end_date = MAX(dx.event_time)
136136
FROM collect.deadlock_xml AS dx
137137
WHERE dx.is_processed = 0
138138
AND dx.event_time IS NOT NULL
@@ -153,7 +153,11 @@ BEGIN
153153
*/
154154
SELECT
155155
@start_date_local = DATEADD(MINUTE, @utc_offset_minutes, @start_date),
156-
@end_date_local = DATEADD(MINUTE, @utc_offset_minutes, @end_date);
156+
/* +1s on the PARSER's local upper bound (not on @end_date itself) so sp_BlitzLock
157+
includes events at exactly @end_date, while the mark below uses the un-padded UTC
158+
@end_date — so a deadlock inserted concurrently in that extra second is left for the
159+
next run rather than marked unparsed. Mirrors process_blocked_process_xml. */
160+
@end_date_local = DATEADD(SECOND, 1, DATEADD(MINUTE, @utc_offset_minutes, @end_date));
157161

158162
IF @debug = 1
159163
BEGIN
@@ -244,10 +248,11 @@ BEGIN
244248
here: the XACT_STATE() = -1 check and the CATCH block both roll back
245249
without marking, so a real parse failure still retries next run. Raw
246250
XML is retained (is_processed = 1, not deleted); data-retention
247-
handles cleanup. The +1s pad on @end_date above guarantees the parse
248-
window covers every unprocessed event, so we never mark a row
249-
sp_BlitzLock did not get to see. event_time is UTC, matching
250-
@start_date / @end_date.
251+
handles cleanup. The +1s pad on @end_date_local (the parser's local bound,
252+
set above) guarantees sp_BlitzLock sees every event up to and including
253+
@end_date, while this mark uses the un-padded UTC @end_date so a row inserted
254+
concurrently after @end_date is left for the next run. event_time is UTC,
255+
matching @start_date / @end_date.
251256
*/
252257
IF @rows_parsed = 0 AND @debug = 1
253258
BEGIN
@@ -285,18 +290,22 @@ BEGIN
285290
VALUES
286291
(
287292
N'process_deadlock_xml',
288-
CASE WHEN @rows_available = 0 THEN N'SUCCESS'
289-
WHEN @rows_parsed > 0 THEN N'SUCCESS'
290-
ELSE N'NO_RESULTS'
291-
END,
293+
/*
294+
A clean parse run is SUCCESS even when sp_BlitzLock produced 0 parsed
295+
deadlocks: the events were processed and marked (above), they simply held
296+
no reconstructable deadlock graph (un-parseable-by-design). Genuine failures
297+
take the CATCH path and log ERROR. This ends the perpetual NO_RESULTS this
298+
collector used to emit and mirrors process_blocked_process_xml exactly
299+
(previously this proc still logged NO_RESULTS + "left unprocessed for retry"
300+
after it had already marked the rows processed — a false signal).
301+
Tradeoff: a silent sp_BlitzLock failure that returns 0 rows with no error and
302+
a committable transaction is indistinguishable from "nothing to parse", so
303+
those events are marked processed — an accepted cost of ending the retry loop.
304+
*/
305+
N'SUCCESS',
292306
@rows_available,
293307
DATEDIFF(MILLISECOND, @start_time, SYSDATETIME()),
294-
CASE WHEN @rows_available > 0 AND @rows_parsed = 0
295-
THEN N'sp_BlitzLock returned 0 parsed results for '
296-
+ CAST(@rows_available AS nvarchar(20))
297-
+ N' XML events - rows left unprocessed for retry'
298-
ELSE NULL
299-
END
308+
NULL
300309
);
301310

302311
IF @debug = 1

install/53_collect_server_properties.sql

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,13 @@ BEGIN
319319
CONCAT
320320
(
321321
CONVERT(nvarchar(128), SERVERPROPERTY(N'Edition')), N'|',
322+
/* Include the Azure tier inputs that drive the normalized edition /
323+
service_objective columns. SERVERPROPERTY('Edition') is the constant
324+
'SQL Azure' on Azure SQL DB, so without these a pure tier/SLO change with
325+
no vCore/memory delta would not change the hash and the collector would
326+
SKIP, never recording the new tier. NULL (on-prem) concats as empty. */
327+
CONVERT(nvarchar(128), DATABASEPROPERTYEX(DB_NAME(), N'Edition')), N'|',
328+
CONVERT(nvarchar(128), DATABASEPROPERTYEX(DB_NAME(), N'ServiceObjective')), N'|',
322329
CONVERT(nvarchar(128), SERVERPROPERTY(N'ProductVersion')), N'|',
323330
CONVERT(nvarchar(128), SERVERPROPERTY(N'ProductLevel')), N'|',
324331
@engine_edition, N'|',

0 commit comments

Comments
 (0)