Window the custom date range in the offset of the server the read names - #2987
Conversation
… tab static GetTimeRange's custom-range branch converted server-local bounds to UTC with ServerTimeHelper.UtcOffsetMinutes, process-wide state written only by the WPF tab paths, while the read around it filtered on a server_id of its own. The offset now arrives as a required parameter, as GetTimeRangeServerLocal's already does, so the branch reads no ambient state and every caller has to name whose offset it means. The alert-badge read is the one that was naming the wrong server. It runs on every tab's own timer outside ServerTab's IsVisible gate, so a background tab's badge was windowed in the selected tab's offset. GetAlertCountsAsync takes the offset of the server it counts for, and RefreshAlertCountsAsync derives its own window in the tab's own UtcOffsetMinutes rather than inheriting the one computed for the sub-tab reads. Both halves of the conversion have to name one server or they stop cancelling: GetCurrentWindow converts the pickers out of the display mode and the read converts back out to UTC. The pair cancels under the UTC and LocalTime display modes and only the read's half applies under ServerTime, the default. GetCurrentWindow therefore takes the offset to convert in, and is given the selected tab's for the sub-tab reads and the tab's own for the badge.
|
Reviewed. This is a clean, mechanical fix and I couldn't find a correctness bug in it. What I checked:
Lite/Darling parity: no drift to flag. Darling's viewer has an analogous-looking static ( No T-SQL in this diff, so the collector/ (Note: I wasn't able to actually compile this branch in this sandbox — both |
GetTimeRange's custom-range branch converted caller-supplied server-local bounds to UTC withServerTimeHelper.UtcOffsetMinutes— process-wide state whose only writers are the WPF tab paths (MainWindow.xaml.cson tab selection,ServerTab.xaml.cson construction) — while the read around it filtered on aserver_idof its own. The two halves of one predicate could name two different servers, and the failure is silent: bounds compared against the wrong clock match fewer or more rows with no error.Part of #2977. Part of #2976, which is the reachable consequence and is subsumed by this.
The change
The offset is a required parameter of
GetTimeRange, exactly asGetTimeRangeServerLocal's already is. The branch reads no ambient state, and the compiler makes every call site name whose offset it means.asOfUtcloses its default in the same signature — an omitted anchor was alreadynull, so this is not a behaviour change, but it puts #2495's anchor at the call site too.LocalDataServiceis not per-server —MainWindowand the MCP host share one instance across every registered server — so an instance field would have been the same defect in a new place. It has to be an argument.The 82 call sites, by the answer given
75 pass
SelectedServerTabUtcOffsetMinutes, a named and documented accessor for the desktop's selected-tab offset. Their server-localfromDate/toDatecan only have come from aServerTab's own toolbar pickers, and every one of those reads sits behind theIsVisiblegate inServerTab.RefreshAllDataAsync, where the tab doing the reading is the selected tab and the two offsets are the same number. Behaviour is unchanged. It is named rather than spelled inline because it is an answer and not a value — it asserts "this window belongs to whichever server the desktop has selected", which is true only for a read the selected tab drives — and because it makes the set auditable:git grep SelectedServerTabUtcOffsetMinutesis now the census of reads that inherit the selected tab's offset, where previously that set was invisible.1 takes the offset of the server it names.
GetAlertCountsAsyncis the badge read, and the one reachable for a non-selected tab: it runs on everyServerTab's own 60-second timer outside theIsVisiblegate, on the tab's own_serverId. It now requires the offset of that server, andServerTab.RefreshAlertCountsAsyncsupplies the tab's ownUtcOffsetMinutes.6 pass
utcOffsetMinutes: 0. They hard-codenull, null, so the converting branch is unreachable and no offset exists to be right or wrong. Stated at the call site rather than defaulted, per the same reasoning that made the parameter required.Nothing outside those 82 was reachable and wrong.
LiteAlertReadAdapter— the headless alert engine, which does run for servers no tab has selected — passes onlyhoursBack, so it never enters the branch.Lite/Mcp/passes nofromDate/toDateto anyGetTimeRangeread at all, which re-confirms #2977's measurement on currentdev; theMcpBlockingToolscomment asking a reader to remember this is left in place, since the parameter now enforces it for a caller who does not read comments.Both halves of the conversion, kept paired
The offset is applied twice per window:
ServerTab.GetCurrentWindowconverts the pickers out of the display mode into server time, andGetTimeRangeconverts back out to UTC. UnderTimeDisplayMode.UTCandLocalTimethose cancel; underServerTime— the default — the first is the identity and only the second applies. So moving one side's offset source without the other breaks the two modes that currently work while appearing to fix the default.GetCurrentWindowtherefore takes the offset to convert in, andServerTimeHelper.DisplayTimeToServerTimegains an explicit-offset overload for it. The sub-tab reads get the selected tab's offset on both sides, as before. The badge gets the tab's own on both sides:RefreshAlertCountsAsyncderives its own window instead of inheriting the one computed at the top ofRefreshAllDataAsync, which also removes the "window computed before the gate, used after it" shape #2976 describes.RevertChartAxesre-pins axes onto the window its plotted data was read over, so it takes the same offset those reads take.Tests
AlertBadgeServerOffsetTestsstates the contract as invariance, not as a pinned window — a pinned timestamp passes against the unfixed code whenever the static happens to hold the right server, which is how this stayed hidden. Two synthetic servers 13.5 hours apart (UTC-8 and UTC+5:30, one a half-hour offset so whole-hour rounding is caught), read under four values of the static including one belonging to neither, each returning its own counts every time, told apart by count shape rather than row totals.Every display mode is a separate case, and each runs the real pair — the production display conversion feeding the production read. That is load-bearing: the mutation that moves only the display conversion onto the static breaks
UTCandLocalTimeand leavesServerTimegreen, so a fixture exercising only the default would not have caught it.Of the four existing fixtures in this area,
CollectionHealthWindowTestsis the one that masks this shape: it readsServerTimeHelper.UtcOffsetMinutesand seeds the bounds it then queries against that same value, so it cannot distinguish a read that used the static from one that used the right server's offset — the two are never made unequal. It is left as it is, because it is a live and useful test of the bounds themselves and this PR does not change the behaviour it pins.AsOfWindowAnchorTestsno longer touches the static (#2967 fixed exactly this pattern there);QueryStoreDedupReadTestsdeliberately avoidsfromDate/toDatefor this reason and says so;AnalysisAsOfAnchorTestsdoes not touch the static.Verification
The solution compiles clean on macOS with
-p:EnableWindowsTargeting=true— 0 errors, with zeroGetTimeRangecall sites left reading the offset implicitly, which is what the required parameter buys and what the compiler asserts. The call-site count reconciles at 82 two independent ways (git grep -oandgit ls-files | grep -c, both positive-controlled).Lite.Teststargetsnet10.0-windowsand cannot run here, so fail-first was demonstrated in a throwawaynet10.0xUnit host staged outside the repo that compiles the real test files by absolute path against the shippedLocalDataService.csandServerTimeHelper.cs, withGetAlertCountsAsyncspliced out of the shipped source by script (re-spliced on every build, so a mutation cannot go stale). Four mutations, one at a time, each asserted applied by anchor count and content hash:GetAlertCountsAsyncignores its parameter and passes the selected-tab static — the "made it compile" anti-fixUTC+LocalTime+ the pairing assertion;ServerTimestays green, as expectedCollectionHealthWindowTestsThe last one is there to show that fixture's green under this change is a live measurement of unchanged behaviour rather than a vacuous pass.
Not verified
The runtime WPF behaviour.
ServerTab,MainWindowand the picker controls need a Windows host, so the pairing insideGetCurrentWindow/RefreshAlertCountsAsyncis verified by construction and by the arithmetic the tests exercise, not by watching two tabs in different zones disagree. The operator-visible half of #2976 was derived from source in the first place and remains so. CI is the arbiter for the xUnit suites as they actually run.