Render every desktop timestamp through the renderer for its own clock frame (#3207) - #3228
Closed
erikdarlingdata wants to merge 2 commits into
Closed
Render every desktop timestamp through the renderer for its own clock frame (#3207)#3228erikdarlingdata wants to merge 2 commits into
erikdarlingdata wants to merge 2 commits into
Conversation
… frame Twenty-four sites across both SKUs rendered a timestamp through the renderer built for the OTHER frame. Both directions were live and both are silent: a server-local value through ForDisplay/FormatServerTime subtracts the offset a second time and displays four hours EARLY on the fleet's measured -240, while a naive-UTC value through FormatServerClock displays four hours LATE. Lite had no raw renderer at all, which is the root of sixteen of them: its FormatServerTime is its ForDisplay -- it adds the offset and names its parameter utcTime -- so there was no way to say "this value is already on the server's clock". ServerTimeHelper.FormatServerClock supplies that, and unlike Darling's namesake it routes through ConvertForDisplay, so these columns honour the user's Server/Local/UTC choice like every other Lite timestamp. Darling gains the opposite helper for the same reason: five query_store_stats sites had no way to say "this column is UTC" and reached for the raw renderer, so ViewerDataService.FormatStoredUtc is now the named counterpart to FormatServerClock. Query Store returns datetimeoffset and QueryStoreCollector normalises through DateTimeOffset.UtcDateTime, so those five are UTC and the sixth Darling site -- running_jobs.start_time, the msdb Agent clock -- goes the other way, off ForDisplay and onto FormatServerClock. Two sites are outside #3220's census and were found by hand, because its scan structurally cannot see them: the deadlock graph's lasttranstarted has no store column of that name, and Lite's running-jobs StartTimeLocal used DateTime.ToLocalTime(), which is not a tracked renderer -- that one ignored the display-mode setting entirely as well as treating an msdb-local value as UTC. Three doc comments stated the wrong frame and are corrected, including the one in ViewerDataService.QuerySnapshots that called Lite's FormatServerTime a "raw server-clock" renderer and asserted the two SKUs agreed. They did not, and that claim is how the Lite side stayed hidden. FormatStoredUtc is registered in the guard's Renderers map rather than left out: an unlisted renderer is invisible to that scan, so omitting it would have dropped five sites out of the census as the price of fixing them. The seventeen DesktopRenderFrameMismatch rows come out and DesktopRenderMismatchSites goes to 0 -- that scan flags only DISAGREEMENT, so a corrected site leaves the found-set and a row for it would fail set equality from the other side. Zero is the strongest form of the pin, and a regression is still caught: it would be found and not be in the inventory.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| [ | ||
| ("ForDisplay", ClockFrame.Utc), | ||
| ("FormatServerTime", ClockFrame.Utc), | ||
| ("FormatStoredUtc", ClockFrame.Utc), |
There was a problem hiding this comment.
Adding FormatStoredUtc makes Renderers.Length 4, but the pin at line ~1418 (RenderCallMechanics or nearby test, not touched by this diff) still reads:
/* And the two renderer families are distinguished, not merged: three names, two expectations. */
Assert.Equal(3, Renderers.Length);That assertion — and the "three names" comment above it — predate this PR and weren't updated for the new entry. As written it will fail at runtime (Renderers.Length is now 4, not 3), which goes red on the Windows "build" / "Darling PostgreSQL tests" CI jobs (this project compiles on Linux but only runs there per CONTRIBUTING.md, so the Linux build job won't catch it). Needs Assert.Equal(4, Renderers.Length) and the comment updated to "four names, two expectations".
|
Reviewed the diff (Lite The production logic checks out: verified independently that Left one inline comment: the new No other correctness, parity, or security issues found in this diff. |
…renderer Two CI failures, both mine, both about the guard rather than the fix. AssertMatchesInventory asserts the found set is NOT empty. That is the right floor for the MCP fact, whose scan can only report sites it found -- an empty result there cannot be told apart from a scan that stopped matching. It is the one assertion a fully-fixed render tree cannot satisfy, and I had zeroed DesktopRenderMismatchSites without noticing the helper forbids it. The render fact does not need that floor, because it already carries a stronger one: judged >= 30 counts the sites the scan LOOKED AT rather than the sites that failed, and its own comment says why -- "a matcher that stopped recognising the renderers would otherwise report an empty offender set over 459 files". So this fact now spells its set equality inline and tolerates the empty set that is now correct. The ratchet is unchanged where it matters: a reintroduced mismatch lands in found, is absent from the inventory, and reds. Renderers went from three names to four, so the count pin follows, plus an expectation pin for FormatStoredUtc and one asserting FormatServerClock is still the only member on the server-local side -- the property that makes "three take UTC, one takes the server's clock" checkable rather than a comment.
|
Reviewed the diff in detail — this is a careful, well-scoped fix and I didn't find correctness or parity bugs. Verified independently:
One thing worth a follow-up issue rather than a blocker here: the PR's own doc comment in 🤖 Generated with Claude Code |
erikdarlingdata
added a commit
that referenced
this pull request
Sep 9, 2026
…mpleteness two ways FormatStoredUtc is the counterpart to FormatServerClock, ported from #3228 with its doc comment: named for the frame it takes, because picking the wrong one of the pair is silent - both return a plausible timestamp and they differ by the server's whole offset. The five query_store_stats sites take it instead of an inline ternary, and it is REGISTERED, which is what keeps those sites inside the judged population; the scan keys on the renderer's name. The map's completeness had no criterion, which is the class the guard itself exists to catch. Two checks now cover it from different directions. The alias derivation pins every static formatter over a DateTime that reaches a registered renderer under another name - the shape that hid four plan_correction stamps - at set equality. That criterion is a SHAPE and a shape can be evaded, so a second check asks nothing about signatures: no identifier may be applied to a census timestamp column unless it is a registered renderer, a declared alias, or declared as not rendering, with each exemption carrying what it actually is. An unregistered INSTANCE method handed a census column is invisible to the first and caught by the second.
Owner
Author
|
Closing in favour of #3227, which covers strictly more. Both PRs were opened for #3207 within six minutes of each other — #3227 at 17:43, this one at 17:49 — and I opened this one without checking whether a PR already existed. That check is the cheap one and I skipped it. #3227 is the more complete fix and now carries everything this branch had:
So there is nothing left to port and no reason to keep two PRs open against one issue. Branch left in place rather than deleted, in case any of the reasoning here is worth lifting. Two things from this branch's CI worth having on #3227's radar: |
erikdarlingdata
added a commit
that referenced
this pull request
Sep 9, 2026
…#3207, #3221) (#3227) * Pair every desktop clock render with its column's frame, in both SKUs (#3207, #3221) Twenty-six render sites, both directions. A server-local column through ForDisplay / FormatServerTime adds the collected offset a second time and displays four hours early on the fleet's measured -240; a naive-UTC column through FormatServerClock renders raw and displays four hours late. Each site now takes the renderer that matches its column's frame, read off #3220's catalog-derived census rather than by hand. Lite had no server-clock renderer at all, so its server-local columns had nowhere correct to go: ServerTimeHelper.FormatServerClock is that renderer, built on the existing ConvertForDisplay, whose input contract is already the server's clock. That keeps exactly one add of the offset on the naive-UTC path and none on this one, and honours the Server / Local / UTC preference the rest of the app honours. Three doc comments asserted the wrong frame outright and are why one wrong site became several; a fourth claimed cross-SKU parity with a renderer that does not behave the way it said. All four state the frame the code now uses. The census's seventeen DesktopRenderFrameMismatch rows are deleted with the sites they described. Its render scan could not see a renderer reached through a one-hop wrapper, which is how four server-local plan_correction stamps in the Darling viewer stayed out of the census; the wrapper set is now derived and pinned at set equality, and no wrapper may be handed a column every census table frames the other way. Two class summaries in ViewerDataService.FinOps.cs claimed "localized in read" for columns their own reads take verbatim, with the read's comment stating the opposite and giving the reason. Same defect one file over, so the summaries now defer to the reads. * State the frame on Lite's two undocumented job-history wall-clock getters RunTimeLocal carries a stated justification for showing run_datetime as-is; its two siblings in the same file showed the same frame the same way with nothing said at the property. LastSuccessfulRun is MAX(run_datetime) over the successful step-0 rows and had nothing said at either level; NextScheduledRunLocal had the frame in its class summary only. Both now state it where a reader checks, and neither behaviour changes. * Move the mode-blindness pin to the class that owns the viewer-time statics The pin flips ViewerTimeHelper.CurrentDisplayMode, and only [Collection("viewer-time-statics")] serializes that against the three other classes that flip it - ViewerQueriesTests is in no such collection, so the pin raced them. It also introduced a non-store finally into a file that carries a [Collection("live-postgres")] class, which LiveCleanupConversionRatchetTests reports as a teardown not routed through LiveStoreCleanup; that is the failure both Windows jobs reported, and the ratchet is right about the file even though the block was restoring statics rather than store state. The pin now sits in ViewerTimeHelperTests, in the right collection, and gains a control: the naive-UTC renderer in the same viewer must MOVE with the mode, so the mode-blind assertion is about this renderer rather than about a preference nothing honours. * Name the Darling viewer's UTC renderer, and pin the renderer map's completeness two ways FormatStoredUtc is the counterpart to FormatServerClock, ported from #3228 with its doc comment: named for the frame it takes, because picking the wrong one of the pair is silent - both return a plausible timestamp and they differ by the server's whole offset. The five query_store_stats sites take it instead of an inline ternary, and it is REGISTERED, which is what keeps those sites inside the judged population; the scan keys on the renderer's name. The map's completeness had no criterion, which is the class the guard itself exists to catch. Two checks now cover it from different directions. The alias derivation pins every static formatter over a DateTime that reaches a registered renderer under another name - the shape that hid four plan_correction stamps - at set equality. That criterion is a SHAPE and a shape can be evaded, so a second check asks nothing about signatures: no identifier may be applied to a census timestamp column unless it is a registered renderer, a declared alias, or declared as not rendering, with each exemption carrying what it actually is. An unregistered INSTANCE method handed a census column is invisible to the first and caught by the second. * Make the Darling server-clock renderer honour the display mode, so the frame fix costs no preference FormatServerClock emitted the server's own clock in all three modes, so moving running_jobs.start_time onto it fixed the frame and dropped the preference: a user who selected UTC read server time with nothing saying so, at a site that had respected the choice. It composes out of ConvertToDisplay's existing arms rather than adding any. The naive-UTC twin of a server-clock value is serverLocal - offset, so ConvertServerClockToDisplay is one subtraction in front of the conversion already there and every arm - the machine-local one in particular - is reused. Verified as arithmetic before it was written: a server-clock value converted for display equals the same instant's naive-UTC value converted for display, in every mode, which is the composition rather than a restatement of any arm. The pair is now symmetric across the SKUs. Darling's ConvertToDisplay takes naive UTC, so its server-clock conversion subtracts the offset first; Lite's ConvertForDisplay takes the server's clock, so its naive-UTC renderer adds it. One offset step between the frames, either way round, and both SKUs honour the preference. Both modes are pinned: the same server-local value must render differently under UTC and Server mode, by exactly the offset, at the fleet's measured -240. That assertion is what the old renderer could not support - with no mode input it was untestable on this axis, which is how a raw render shipped. Seven doc comments that said "renders raw" now say what the code does. Lite is untouched. ServerTimeHelper already honours the mode; whether its two job-history getters should is a separate product question and stays one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3207.
Twenty-four desktop render sites across both SKUs rendered a timestamp through the renderer built for the other clock frame. Both directions were live and both are silent — the wrong renderer returns a plausible timestamp, off by the server's whole offset:
ForDisplay/FormatServerTimesubtracts the offset a second time and displays 4 hours early on the fleet's measured −240;FormatServerClockdisplays 4 hours late.Root cause: each SKU was missing one half of the pair
Lite had no raw renderer at all.
ServerTimeHelper.FormatServerTimeis Lite'sForDisplay— it adds the offset unconditionally and names its parameterutcTime— so there was no way to say "this value is already on the server's clock". Sixteen sites therefore pushed server-local DMV and XML values through it.ServerTimeHelper.FormatServerClocksupplies the missing half, and unlike Darling's namesake it routes throughConvertForDisplay, so these columns now honour the user's Server/Local/UTC choice like every other Lite timestamp rather than ignoring it.Darling had no way to say "this column is UTC" in a display property. Five
query_store_statssites reached for the raw renderer instead.ViewerDataService.FormatStoredUtcis now the named counterpart toFormatServerClock— Query Store returnsdatetimeoffsetandQueryStoreCollectornormalises throughDateTimeOffset.UtcDateTime, so those five are UTC. The sixth Darling site goes the other way:running_jobs.start_timeis the msdb Agent's clock (RunningJobsCollectorshipsja.start_execution_dateverbatim and measures duration againstGETDATE()), so it moves offForDisplayontoFormatServerClock.Inlining
ViewerTimeHelper.ForDisplay(x).ToString(...)would behave identically; a named method beside its opposite is what makes the choice reviewable, which is the whole failure mode here.Two sites #3220's census structurally cannot see
Found by hand, and worth naming because they show the guard's reach rather than a gap in its care:
lasttranstarted(ViewerDataService.Deadlock.cs) — parsed from XML intoDeadlockProcessInfo, with no store column of that name for the frame register to resolve.DeadlockTimeLocaltwo lines above is the XE@timestampand correctly stays onForDisplay: two frames, two renderers, one class.RunningJobRow.StartTimeLocal, which usedDateTime.ToLocalTime()— not a renderer the guard'sRenderCalltracks. That one carried two defects: it treated an msdb-local value as UTC, and it rendered in the Lite host machine's zone regardless of the display-mode setting every other timestamp honours.Three doc comments stated the wrong frame
Corrected with the code, including the one that hid the Lite half.
ViewerDataService.QuerySnapshotscalled Lite'sFormatServerTimea "raw server-clock" renderer and asserted the two SKUs agreed. They did not — Lite was double-skewing while the comment said otherwise. They agree now, and the comment says why it used to be wrong rather than being quietly deleted.Guard changes, and why each direction differs from #3206's
FormatStoredUtcis registered in theRenderersmap rather than left out. An unlisted renderer is invisible to that scan, so omitting it would have dropped five sites out of the census as the price of fixing them.The seventeen
DesktopRenderFrameMismatchrows come out, andDesktopRenderMismatchSitesgoes to 0. That is the opposite of what #3206 did with its MCP rows, and the difference is in the discriminator: the render scan flags only a disagreement between column frame and renderer, so a corrected site genuinely leaves the found-set and a row for it would fail set equality from the other side. The MCP scan keys on a payload field NAME, which survives the fix — which is why those rows had to be relabelledDeSkewedAtReadinstead. Zero is the strongest form of this pin, and a regression is still caught: it would be found by the scan and not be in the inventory.Verification
Darling.TestsandLite.Testsarenet10.0-windowsand cannot execute on macOS, so CI is the arbiter for the suite. Run here: all four affected projects build with 0 errors, every changed file keeps CRLF with no bare LF, and the two existingFormatServerClockbehaviour pins (ViewerQueriesTests.FormatServerClock_ShowsRawServerWallClock_EmptyForNullandViewerQueriesRestTests.QuerySnapshotRow_TranStartTimeLocal_RendersRawServerClock_...) pin columns this change does not touch —query_stats/procedure_statsandquery_snapshots.tran_start_time— so they still hold.I have not run the render scan itself; its frame register is derived from collector provenance at runtime and my ability to replay that faithfully is the thing that has been wrong twice on the sibling PR, so I am not claiming it here.
Merge order
This shares
ConsumedTimestampFrameDisciplineTests.cswith #3212, which edits the MCP rows and two different constants. Whichever lands second needs a small mechanical conflict resolution in that one file — different regions, no overlapping rows. #3212 first is the easier order, since its inventory edit is the larger of the two.