Skip to content

Window the custom date range in the offset of the server the read names - #2987

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/2977-explicit-offset-parameter
Sep 5, 2026
Merged

Window the custom date range in the offset of the server the read names#2987
erikdarlingdata merged 2 commits into
devfrom
fix/2977-explicit-offset-parameter

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

GetTimeRange's custom-range branch converted caller-supplied server-local bounds to UTC with ServerTimeHelper.UtcOffsetMinutes — process-wide state whose only writers are the WPF tab paths (MainWindow.xaml.cs on tab selection, ServerTab.xaml.cs on construction) — while the read around it filtered on a server_id of 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 as GetTimeRangeServerLocal's already is. The branch reads no ambient state, and the compiler makes every call site name whose offset it means. asOfUtc loses its default in the same signature — an omitted anchor was already null, so this is not a behaviour change, but it puts #2495's anchor at the call site too.

LocalDataService is not per-server — MainWindow and 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-local fromDate/toDate can only have come from a ServerTab's own toolbar pickers, and every one of those reads sits behind the IsVisible gate in ServerTab.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 SelectedServerTabUtcOffsetMinutes is 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. GetAlertCountsAsync is the badge read, and the one reachable for a non-selected tab: it runs on every ServerTab's own 60-second timer outside the IsVisible gate, on the tab's own _serverId. It now requires the offset of that server, and ServerTab.RefreshAlertCountsAsync supplies the tab's own UtcOffsetMinutes.

6 pass utcOffsetMinutes: 0. They hard-code null, 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 only hoursBack, so it never enters the branch. Lite/Mcp/ passes no fromDate/toDate to any GetTimeRange read at all, which re-confirms #2977's measurement on current dev; the McpBlockingTools comment 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.GetCurrentWindow converts the pickers out of the display mode into server time, and GetTimeRange converts back out to UTC. Under TimeDisplayMode.UTC and LocalTime those cancel; under ServerTime — 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.

GetCurrentWindow therefore takes the offset to convert in, and ServerTimeHelper.DisplayTimeToServerTime gains 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: RefreshAlertCountsAsync derives its own window instead of inheriting the one computed at the top of RefreshAllDataAsync, which also removes the "window computed before the gate, used after it" shape #2976 describes. RevertChartAxes re-pins axes onto the window its plotted data was read over, so it takes the same offset those reads take.

Tests

AlertBadgeServerOffsetTests states 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 UTC and LocalTime and leaves ServerTime green, so a fixture exercising only the default would not have caught it.

Of the four existing fixtures in this area, CollectionHealthWindowTests is the one that masks this shape: it reads ServerTimeHelper.UtcOffsetMinutes and 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. AsOfWindowAnchorTests no longer touches the static (#2967 fixed exactly this pattern there); QueryStoreDedupReadTests deliberately avoids fromDate/toDate for this reason and says so; AnalysisAsOfAnchorTests does not touch the static.

Verification

The solution compiles clean on macOS with -p:EnableWindowsTargeting=true — 0 errors, with zero GetTimeRange call 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 -o and git ls-files | grep -c, both positive-controlled).

Lite.Tests targets net10.0-windows and cannot run here, so fail-first was demonstrated in a throwaway net10.0 xUnit host staged outside the repo that compiles the real test files by absolute path against the shipped LocalDataService.cs and ServerTimeHelper.cs, with GetAlertCountsAsync spliced 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:

mutation caught by
custom branch reads the static again all 3 display modes
GetAlertCountsAsync ignores its parameter and passes the selected-tab static — the "made it compile" anti-fix all 3 display modes
explicit-offset display conversion reads the static — one side of the pair only UTC + LocalTime + the pairing assertion; ServerTime stays green, as expected
a group-1 site's offset replaced by a fixed wrong value pre-existing CollectionHealthWindowTests

The 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, MainWindow and the picker controls need a Windows host, so the pairing inside GetCurrentWindow/RefreshAlertCountsAsync is 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.

… 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.
@claude

claude Bot commented Sep 5, 2026

Copy link
Copy Markdown

Reviewed. This is a clean, mechanical fix and I couldn't find a correctness bug in it.

What I checked:

  • Traced all 82 GetTimeRange(...) call sites after the signature change (offset went from an implicit default read off ServerTimeHelper.UtcOffsetMinutes to a required parameter) — every call site was updated, and the split between SelectedServerTabUtcOffsetMinutes (75 call sites, all reads that are gated behind the IsVisible check where the reading tab is the selected tab), the one GetAlertCountsAsync call site (which now takes the named server's own UtcOffsetMinutes, correct since it runs on every tab's own background timer regardless of visibility), and the 6 utcOffsetMinutes: 0 sites (all have fromDate/toDate hardcoded to null, so the offset-converting branch in GetTimeRange is provably unreachable there) all check out.
  • Verified Lite/Mcp/*.cs tool call sites never pass fromDate/toDate into these reads (they use asOfUtc instead, per an explicit comment in McpBlockingTools.cs explaining why), so the MCP host sharing one LocalDataService instance across every registered server can't hit the ambient-offset branch this PR removes.
  • Confirmed LiteAlertReadAdapter (the headless alert engine, which does run for non-selected servers) calls GetRecentDeadlocksAsync/GetRecentBlockedProcessReportsAsync with fromDate/toDate left at their null defaults, so it was never reachable by the bug and needs no change here, matching the PR description.
  • New test file (Lite.Tests/AlertBadgeServerOffsetTests.cs) asserts an invariance across a battery of desktop-static values rather than pinning one expected timestamp, which is the right shape of test for a "silently reads the wrong ambient value" defect — a pinned-value test would have passed against the old buggy code too whenever the static happened to hold the right value.

Lite/Darling parity: no drift to flag. Darling's viewer has an analogous-looking static (ViewerTimeHelper.UtcOffsetMinutes), but its alert badge path (MainWindow.AlertBadges.csDarlingAlertReadAdapter.Window(hoursBack)) is UTC/hoursBack-only with no offset conversion at all, so it was never exposed to this bug class and doesn't need a matching fix.

No T-SQL in this diff, so the collector/OPTION(RECOMPILE)/alias conventions don't apply here.

(Note: I wasn't able to actually compile this branch in this sandbox — both Lite and Lite.Tests target net10.0-windows, and dotnet build here requires interactive approval I didn't have. Everything above is from static tracing of every call site, not a build/test run.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant