Repository navigation
De-skew the sixteen server-local timestamps MCP reads return, in both SKUs (#3206) - #3212
Conversation
running_jobs.start_time, the six blocked-process transaction/batch stamps, index_object_stats' last_user_access, the four PVS cleaner times and the four plan-correction lifecycle times are all the monitored server's local wall clock in the store, and every one was stamped into an MCP payload beside naive-UTC fields on the same object. Darling de-skews in SQL by the collected server_properties.utc_offset_minutes, copying DarlingDefaultTraceReader's single-row COALESCE CTE. Lite de-skews the row in C# at the MCP projection: DuckDB has no make_interval, and the underlying LocalDataService reads are shared with the WPF grids, which have their own frame defect on a different subset of columns. Payload field names are unchanged; only the frame is. No read's window, snapshot subquery or ordering touches an affected column, so no row selection moves and there is no stored-data migration. McpPayloadClockFrameDisciplineTests pins the boundary the three existing clock guards could not reach: CreationTimeClockFrameDisciplineTests has the right discriminator but only Analysis roots, StoreSqlClockDisciplineTests has the right roots but hunts clock functions and disclaims projections, and CollectorTimestampFrameTests says what a frame is without asking who consumes it.
ReviewWent through the full diff (16-field de-skew across Sign convention — Darling's Ordinal safety — the CTE additions don't shift any No orphaned name-based consumer of the renamed aliases — grepped the whole repo for Cross-join row safety — every new Lite/Darling parity — the Nothing else stood out — no SQL injection surface (all params bound positionally), no missing-index folklore, no correctness issue in the 🤖 Generated with Claude Code |
…laration count read as a field count Both were in the new guard, not the product. EveryLiteToolSite_... required `Property?.AddMinutes(-utcOffsetMinutes)`, but nullability is a property of the ROW TYPE and not of this convention: RunningJobRow.StartTime is a non-nullable DateTime and ships without the `?.`, while the other fifteen are DateTime? and ship with it. The assertion was therefore vacuous for the one field that cannot carry it. The matcher now makes the null-conditional optional and is pinned in both directions on both forms, plus a prefix-collision case. TheTwoSkus_DeSkewTheSameNumberOfFields compared Darling's per-READ column declarations (18) against Lite's payload-field count (16). get_blocking is served by two reads and the always-on DMV fallback arm re-serves blocked_/blocking_last_tran_started -- the two columns a DMV snapshot has -- so the sums were never going to agree. It now compares DISTINCT payload fields and pins the overlap at exactly 2, so a third read quietly re-serving a field has to be a deliberate edit. The local replay harness has grown the Lite scan and the arity check it lacked, which is why neither of these surfaced before CI: it only ever replayed the Darling facts. Both halves are now proven red without the fix -- 41 assertions on the Darling constants, 37 on origin/dev's Lite sources.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed the diff ( Correctness: Sign convention matches the established Lite/Darling parity: All 16 fields are covered on both sides (confirmed by reading the actual tool files, not just trusting the test). The asymmetry — Darling de-skews in SQL, Lite de-skews in C# at the MCP projection layer rather than in No row-selection changes: confirmed every touched read still windows/snapshots/orders on a naive-UTC column ( Security: no new user input paths, all params still bound via One minor style nit posted inline: three of the five new Postgres cross joins ( Otherwise this is a clean, well-tested fix — the guard test file is unusually thorough (positive controls on its own regexes, a floor-and-ceiling census, an explicit "must NOT acquire the de-skew" list) and the PR description's own account of the two bugs CI caught in the guard's first cut checks out against the current code. |
…nit2008) Nineteen new analyzer warnings, all in the new guard. The two message-carrying assertions are deliberately left as Assert.True/False: the analyzer does not flag them, and Assert.Matches takes no message, so converting them would drop the diagnostic that names the collector evidence.
CONTRIBUTING.md's T-SQL style says table aliases always use AS, and the other three reads this change touches already do. Aliasing v_index_object_stats AS ios, v_pvs_stats AS pvs and plan_correction AS pc removes a 2-vs-3 split introduced inside one diff, and gives each snapshot self-subquery its own alias (latest) so the correlated and outer scopes read apart. The guard tracks the SHIPPED expression for its one derived column, so last_user_access's declared source moves with the qualification -- which is why it is declared rather than derived from the alias. Verified unchanged against live PostgreSQL 17.11 on the same fixture: the same de-skewed values, NULLs preserved, the enablement-only row still dropped, and the unused-first ordering intact.
|
Reviewed the diff against Scope check — no T-SQL collector files are touched by this PR (only Darling's Postgres MCP reads and Lite's C# MCP projection layer), so the T-SQL style rules ( Correctness
Lite/Darling parity — traced all five affected tools ( Security — all Postgres access uses positional parameters ( Minor, non-blocking observation: in the five Lite MCP tool sites ( No blocking issues found. |
Review summaryWent through the full diff (five Darling MCP readers + tools, five Lite MCP tools, and the new
One trivial, non-blocking nit: 🤖 Generated with Claude Code |
This reverts commit 7e69666.
… one unaliased cross-join form Two separate corrections. get_running_jobs' Description asserted start_time is "the same frame as collection_time and as every other timestamp this server returns". The second clause is false by this PR's own census -- five tools are fixed here, six still return server-local values -- and a tool Description ships in the MCP schema every client reads, so it reaches an agent even where the instruction text does not. This is the class the PR exists to remove, restated inside it. Now scoped to the two fields on this payload, matching what Lite's same-named tool already said. The reader's row doc carried the same universal and is scoped too. All six reads now cross join svr WITHOUT aliasing the main table, which is what they did before the aliasing attempt. Full qualification made IndexUsageSql's filter textually diverge from IndexUsageMatchCountSql's, and IndexUsageTruncationTests.TheCountAndTheRowsShareTheirFilter checks those two statements agree BY TEXTUAL IDENTITY -- the count has no cross join, so it was never aliased. Relaxing that pin to accept divergent text would remove its ability to catch a real divergence later, and the count read applying a different filter than the rows is exactly what #2636 was about. svr contributes only offset_minutes and is always referenced as svr.offset_minutes, so nothing is ambiguous unaliased. The AS-on-every-table style question is #3217.
|
Reviewed. This is a solid, well-tested fix and I didn't find correctness, Lite/Darling parity, or security issues. Checked specifically:
One minor, non-blocking observation: |
…ed census #3220's guard landed on dev while this branch was open, and its inventory was written against the unrepaired state. Two of its facts fail here, both because this PR changes what the scan finds rather than because anything regressed. Thirty rows move McpPayloadUnmarked -> DeSkewedAtRead, keeping each row's original reasoning and suffixing the disposition -- relabelled rather than deleted, which is the convention its own DeSkewedAtRead summary asks for and what #3202's two rows already do: the payload field still carries the column's name while the value is now naive UTC, so the scan still finds the site. The two last_user_access rows come OUT instead. That payload field was only ever reachable through the projection-alias hop, and the read's alias is now last_user_access_utc, so RenamedServerLocalProjections yields the _utc name and the payload's own field name is no longer derivable. A row for a site the scan cannot see would break set equality from the other side. The alias pin follows the read. One row stays McpPayloadUnmarked and should: Lite/Mcp/McpPlanCacheSchedulerTools.cs's oldest_plan_create_time. It is a Lite-only surface -- Darling's plan-cache tool does not emit it -- and #3206's census missed it because that census enumerated Darling's emissions and intersected with Lite, which by construction cannot see a Lite-only tool. Left unrepaired here deliberately: it belongs to a different collector and a tool this PR does not otherwise touch. #3206 is corrected to say so. The class doc's "Nothing here is repaired by this change" is no longer true of the MCP half and now says which half is which.
|
Reviewed the diff (all Darling reader/tool files, Lite MCP tool files, and both test files). Correctness: Traced every one of the 16 de-skewed fields end to end — SQL alias → column ordinal → C# reader → JSON payload key — for Row-selection safety: Checked every touched query's Blocking merge: Lite/Darling parity: Both SKUs convert the same 16 fields (verified the arithmetic behind Minor/non-blocking: No SQL-injection, secrets, or missing-index concerns — all reads are parameterized, and no new folklore index advice was added. Nothing else stood out; the new |
…ad of dropping two Removing the two last_user_access rows was the wrong remedy, and the reason is in the guard's own words: "without this the label is a way to delete a site from the census by asserting it is fine." Relabelling without evidence deletes a site by asserting it is fine; removing it deletes the site without even asserting. The obstacle was that the read's alias was last_user_access_utc while the payload field stayed last_user_access, and the census reaches that payload field THROUGH the SQL alias. Flipping the alias's inherited frame to UTC does not fix that -- RenamedServerLocalProjections drops a non-ServerLocal alias entirely, so the site would still vanish from the register. So the read's alias goes back to last_user_access and keeps its de-skew. The payload field and the alias coincide again, the site stays visible, and the alias pin needs no change from what dev already had. This one alias deliberately carries no _utc suffix, and that is now the stronger arrangement: a suffix is a claim in a name that nothing checks, while EveryDeSkewedAtReadSite_CarriesItsConversionInTheReaderItDependsOn pins the conversion text itself. That fact's evidence table therefore grows from 2 rows to 34, one per DeSkewedAtRead row, each naming the reader it depends on and the conversion as shipped -- Darling's SQL de-skew and Lite's C# AddMinutes, including the one non-nullable StartTime that carries no null-conditional. The table was generated from the inventory and every conversion asserted present in its own file before being written, so it is derived from the shipped text rather than transcribed. McpPayloadUnmarkedSites 33 -> 1, DeSkewedAtReadSites 2 -> 34. The 22 render sites are untouched.
|
Reviewed the diff (all 18 files, both product code and the new/updated test guards) against CONTRIBUTING.md and for Lite/Darling parity, correctness, security, and perf. Correctness of the de-skew direction: verified Ordinal safety: for every touched reader ( Lite/Darling parity: Darling de-skews in SQL, Lite de-skews in C# at the MCP projection ( Security: all reads stay parameterized ( Row-selection safety: every touched read still windows/snapshots/orders on the naive-UTC No correctness, parity, or security issues found. The one intentionally-deferred item ( Nit (non-blocking): computing |
…uffix The suffix was the whole cause of both remaining failures, and reverting it on one alias only fixed one sixteenth of it. RenamedServerLocalProjections reads a projection alias's SOURCE columns and cannot see the conversion, so every *_utc alias over an unambiguously server-local column was reported as inheriting a server-local frame. That took the pinned list from one entry to sixteen and pulled alias sites into the found-set that the inventory does not carry. Dropping the suffix makes the alias equal to the column name, which that helper explicitly skips (byColumn.ContainsKey(alias)), so every site routes through the ordinary column path where the frame check applies and the register answers correctly. Its pin needs no edit: it matches what dev already had. The suffix is not missed. It was a claim in a name with nothing checking it, whereas EveryDeSkewedAtReadSite_CarriesItsConversionInTheReaderItDependsOn pins the conversion text in the reader it depends on -- now realigned to the shipped aliases for all fifteen. #3202's event_time_utc keeps its suffix and is untouched: event_time spans five tables and two frames, so the helper declines it and the collision never arises there. Not doing the other half deliberately: teaching that helper about de-skews would now be unexercised, because no projection reaches the code path once every alias matches its column. It wants a live case to be pinned against. Verified on live PostgreSQL 17.11 that all six reads still parse and execute; an alias name cannot move a computed value, and the value arithmetic was verified in detail earlier in this branch.
Review: de-skew of server-local MCP timestamps (#3206)Reviewed the production code changes (both SKUs) against Correctness
Lite/Darling parity
Security / perf
I didn't find correctness bugs, parity drift, or security/perf regressions in the diff. The one thing outside my ability to verify here is the actual CI run (couldn't fetch check-run status in this environment) — worth confirming the Darling PostgreSQL live-Postgres job and the Windows |
… the prose is not read as SQL Both remaining failures came from a tool Description, not from any SQL. ConsumedTimestampFrameDisciplineTests.ProjectionAlias is ([^\r\n]*?)\s+AS\s+([a-z][a-z0-9_]*) under IgnoreCase, so it matches any " as <word>". get_running_jobs' Description said "start_time is UTC, the same frame as the collection_time on this payload" -- which reads as a projection whose expression names start_time and whose alias is "the", with Lite's variant yielding "collection_time". Both were reported as renamed server-local projections, taking the pinned list to three entries and pulling alias sites into the found-set the inventory does not carry. This is a string LITERAL, so WithoutComments does not touch it and should not: the SQL under test lives in literals too. The prose is the thing to change, and "matching" says it without the word the regex keys on. Replaying the helper over both SKUs' Mcp/ trees now returns exactly [(last_user_access, index_object_stats)], which is the pinned value. Not changed: that regex reading prose at all is a latent looseness rather than this PR's defect -- four comment lines in these same readers already matched it on dev without being reportable. Applying the file's own WithoutComments in that helper would close the comment half and is provably a no-op today, so it wants its owner's call rather than a drive-by from here.
|
Reviewed the diff (Darling readers/tools, Lite MCP tools, and the new Correctness: The de-skew direction ( No row-selection changes: confirmed every affected read still windows/snapshots/orders on the naive-UTC Lite/Darling parity: functionally in parity (same 16 fields de-skewed on both SKUs, same Nothing blocking found. The guard/test work here is unusually thorough (live-Postgres verification across three offsets including a non-whole-hour one, and the guard is proven red against |
Both new UTC clauses dropped their trailing comparison on the Lite side, so the two SKUs' descriptions stopped being word-for-word identical. Lite ships all three tools referenced - get_collection_log in McpHealthTools, list_servers in McpDiscoveryTools, get_query_store_regressions in McpQueryTools - so the comparisons are equally valid here. A tool Description reaches an MCP caller where instruction text does not, which is this change's own argument for saying the whole thing.
ReviewWent through all 18 changed files (both Darling readers/tools and their Lite counterparts) plus the two test files. Summary: I did not find correctness, parity, or security issues. What I checked specifically:
Minor, non-blocking observation: in each of the five Lite MCP tools ( The self-referential test coverage ( |
…ansition (#3231) server_properties.utc_offset_minutes is DATEDIFF(MINUTE, GETUTCDATE(), GETDATE()) -- the offset in force at COLLECTION time, one current value. Subtracting it from a stored timestamp is exact only while both sit on the same side of a DST transition, and #3212 shipped sixteen de-skewed fields without saying so on the two reads where that can be false. Thirteen of the sixteen describe current state -- a running job, an open transaction, a cleaner that ran seconds ago -- so their timestamps and that offset are always in the same period. get_index_usage is the exception and the worst case: sys.dm_db_index_usage_stats persists since the instance restarted, which on a stable production box is routinely months, so a large share of last_user_access values predate the most recent transition and come back 60 minutes early. Silently, and in the plausible direction. The exposure is general rather than particular: any target in a DST-observing zone has it and a UTC-configured target does not, and on AWS RDS the instance takes its time zone from a creation-time parameter, so a non-UTC zone is ordinary configuration rather than an exotic setting -- which also kills the plausible-sounding assumption that an RDS instance is probably UTC. #2932 carries the measurement behind the four-hour figure. sqlserver_start_time on the same row bounds how far back the affected values reach. get_pvs_stats is a tier less severe and gets the same note: the off-row cleaner runs continuously, but aborted_version_cleaner_* can reach back weeks on a quiet database, so it can cross a transition. Documented rather than fixed, deliberately. The proper fix is a ZONE instead of an offset -- CURRENT_TIMEZONE_ID() collected alongside utc_offset_minutes, then AT TIME ZONE at the read boundary, which handles transitions for a timestamp of any age -- and that is a collected column plus a migration rung, which is its own change. There is no backfill: the collector ships the DMV value verbatim and the offset is subtracted in the projection, so nothing stored is wrong today. The offset path stays as the fallback regardless, because CURRENT_TIMEZONE_ID() is SQL Server 2019+ and the product supports older servers. What this carries is the #2993 shape: the scope of the claim, stated where the de-skew happens, so the next reader knows the frame is exact inside the current DST period and within an hour outside it.
1 participant
Fixes #3206.
Sixteen fields across five MCP tools returned the monitored server's LOCAL wall clock, unmarked, beside naive-UTC fields on the same JSON object.
ToString("o")on aDateTimeKind.Unspecifiedvalue renders no offset suffix, so the two frames serialize identically and nothing in the payload distinguishes them — andserver_properties.utc_offset_minutesis exposed by no MCP tool, so a caller could not correct for it either.default_trace_events.event_timewas the same class and was fixed by #3202. This is that PR's deliberately-deferred sibling list.What changed
get_running_jobsstart_timeRunningJobsCollectorshipsja.start_execution_date, the msdb Agent clock, and computescurrent_duration_secondsagainstGETDATE()on the next lineget_blocking/ Lite'sget_blocked_process_reportsblocked_/blocking_×last_tran_started,last_batch_started,last_batch_completedlasttranstarted/lastbatchstarted/lastbatchcompleted, andsys.dm_tran_active_transactions.transaction_begin_timeon the DMV-fallback armget_index_usagelast_user_accessGREATESTof foursys.dm_db_index_usage_statscolumns shipped verbatimget_pvs_statssys.dm_tran_persistent_version_store_statsshipped verbatimget_plan_correctionsvalid_since,last_refresh,execute_action_initiated_time,revert_action_initiated_timesys.dm_db_tuning_recommendationsshipped verbatimPayload field names are unchanged; only the frame is. That follows #3202: the C# row property gains a
Utcsuffix and the SQL projects a_utcalias, while the JSON key a caller keys on stays put.The stored frame stays local. Collector dedup and duration arithmetic are local-vs-local, and
CollectorTimestampFrameTestspins several of these columns that way; converting at the READ boundary is what the repo has settled on acrossViewerDataService.SystemEvents,ViewerDataService.JobHistory,DarlingDataReader.CpuUtilizationSql, #2992 and #3202.Darling de-skews in SQL, copying
DarlingDefaultTraceReader's single-rowCOALESCEoffset CTE (?? 0when a server has not completed its firstserver_propertiescycle, so a first-cycle server still gets its rows). Lite de-skews the row in C# at the MCP projection instead, for two reasons: DuckDB has nomake_interval, and the underlyingLocalDataServicereads are shared with the WPF grids, which render throughServerTimeHelperand are wrong on a different subset of columns under #3207 — de-skewing in the data service would fix this surface by breaking that one. Lite takes the offset fromMcpServerLocalWindow.OffsetForAsync, never fromServerTimeHelper's process-wide static, for the reason #2967 established.No row selection moves. Every affected read windows, snapshots and orders on a naive-UTC column, so the offset can only change values — which is why there is no stored-data migration and no schema change. That is asserted, not assumed (
TheDeSkew_TouchesNoWindowBound_NoSnapshotSubquery_AndNoOrdering).Measurement
The offset is exactly −240 minutes and it is fleet-wide: #2932 measured
cpu_utilization_stats.sample_timeat exactly 4 h behind the same collector run'scollection_timeon 42 of 42 servers.Confirmed end-to-end at the read boundary on live
get_pvs_statspayloads, three servers across both production stores. The off-row version cleaner runs continuously, soas_of − cleaner_timeshould be near zero:as_of(UTC)offrow_version_cleaner_start_timeas returned2026-09-09T12:49:10.3942026-09-09T08:49:01.7002026-09-09T12:44:35.0092026-09-09T08:44:34.4262026-09-09T12:59:24.1432026-09-09T08:58:42.763The sub-second-to-41-second residual after subtracting 240 minutes is what rules out "the cleaner really did last run four hours ago". On server-z all four ADR-enabled databases read as 4 h stale — and
get_pvs_stats' own description sends a caller there "when ADR cleanup looks stuck", so the phantom stall landed exactly on the question the tool exists to answer.Verification
Darling.TestsandLite.Teststargetnet10.0-windowsand cannot execute on macOS, so CI is the arbiter for the xUnit run. What was actually run here:PREPAREd successfully, then executed against a seeded fixture:server_propertiesrow at −300 present: every stamp came back local + 240, proving the CTE takes the newest offset and not just any.COALESCE … 0fallback, and proof the cross join drops nothing.GREATESTrow, the absentblocking_last_batch_completed, the non-ADR database, the un-reverted recommendation).plan_correction's enablement-only row (recommendation_name IS NULL) is still dropped, so thesvrcross join did not disturb the filter.origin/dev'sPvsStatsLatestSql, on the identical fixture, returns07:59:59beside a12:00:00collection_time — a 4 h 00 m 01 s phantom stall where the post-fix read returns11:59:59, one second.origin/dev's six SQL constants: 41 failing assertions without the change, all green with it. That replay caught a real defect in the guard's first cut — it derived the expected expression from the column alias, which silently asserted nothing forlast_user_access, the one derived (GREATEST) column in the set. The guard now carries the de-skewed expression per column.The guard, and the gap it closes
McpPayloadClockFrameDisciplineTestspins the payload boundary none of the three existing clock guards could reach:CreationTimeClockFrameDisciplineTestshas the right discriminator shape, but its roots are onlyDarling/PerformanceMonitor.Darling.AnalysisandLite/Analysis.StoreSqlClockDisciplineTestshas the right roots —Service/Mcp/is in its globs — but hunts bare clock functions, and explicitly disclaims projections at its lines 43-47.CollectorTimestampFrameTestshas the right per-column reasoning but reads collector source only: it says what a frame IS and never who consumes it.The gap is the diagonal, and it is what would have caught #3198, #2991 and all sixteen of these at once. Every fact is scoped to a (read, column) pair with the collector evidence named, because the frame genuinely cannot be keyed on the column name —
StoreSqlClockDisciplineTestsmaintainsAmbiguousFrameColumnsfor exactly that reason. Both directions are pinned:AlreadyUtcReadsnames the four sibling reads that must NOT acquire the de-skew, since applying it to a value that is already UTC is the same defect with the sign flipped. The desktop half of that mistake is #3207.The cross-SKU arity check is deliberately a count and not a name map: the two SKUs legitimately spell the same column differently (snake_case SQL alias vs PascalCase row property), so a map would be either a second source of truth or a tautology. What must not drift is that both cover sixteen — a field de-skewed on one SKU and not the other is the one-sided port #2992 found nothing guarding against.
Instruction text
devis already correct here and needs no edit from this PR. #3202 narrowed its own sentence before merging —DarlingMcpInstructions.csnow reads "windowed onevent_time, which this tool returns in UTC", scoped to the tool it describes rather than to the surface.An earlier revision of this description claimed
devstill carried the un-narrowed "like every timestamp on every read here — is UTC" and that this PR would make it true. That quoted a pre-merge revision I had read before #3202 landed and never re-read afterwards; both halves of the claim were wrong, and it is corrected here rather than silently dropped.The same universal recurred inside this PR's own diff and is fixed in cc14760:
get_running_jobs'Descriptionassertedstart_timewas "the same frame ascollection_timeand as every other timestamp this server returns". A toolDescriptionships in the MCP schema every client reads, so it reaches an agent even where the instruction text does not — and the clause is false by this PR's own census, which finds sixteen fields across five tools fixed here and six tools still returning server-local values. Every frame sentence this PR adds is now scoped to the tool it sits on.description-driftpasses either way, because the claim was externally false rather than internally inconsistent.Not in scope, filed separately: #3207 (the desktop renderers, wrong in opposite directions in both SKUs) and the render-projection half of #3208.