From 6b6d0d88a11729f18fff1eb182c779fa3d1f2549 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 9 Sep 2026 14:41:38 -0400 Subject: [PATCH 1/2] Scope the de-skew's accuracy claim where the column outlives a DST transition 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. Most of the sixteen describe current state -- a running job, an open transaction, a cleaner that ran seconds ago -- so the timestamp and the 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 fleet is exposed rather than theoretically exposed -- its targets report utc_offset_minutes of -240, which is EDT, a DST-observing zone on summer time -- and sqlserver_start_time on the same row is the bound on how far back it reaches. 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 (a live read showed 2026-08-26 against a 2026-09-09 snapshot), 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 -- and that is a collected-column addition plus a migration rung, which is its own change. 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. Same family as #2993 by a different mechanism: there a zone was captured and discarded, here an offset is captured and over-applied. Both end with a naive timestamp confidently placed in the wrong frame. --- .../Mcp/DarlingObjectStatsReader.cs | 16 ++++++++++++++++ .../Mcp/DarlingPvsReader.cs | 8 ++++++++ 2 files changed, 24 insertions(+) diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingObjectStatsReader.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingObjectStatsReader.cs index dfa3674fe0..e79252f7a3 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingObjectStatsReader.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingObjectStatsReader.cs @@ -187,6 +187,22 @@ public static async Task> GetObjectSizeGrowthAsync( /// keeps it NULL. This read returns NO other timestamp, which is why converting rather than labelling /// matters more here than elsewhere: there is nothing else in the payload for a reader to notice a /// disagreement against. + /// The de-skew is exact only inside the current DST period, and this is the read where that + /// matters most. server_properties.utc_offset_minutes is + /// DATEDIFF(MINUTE, GETUTCDATE(), GETDATE()) — the offset in force AT COLLECTION TIME, one + /// current value. The other de-skewed reads describe current state (a running job, an open transaction, + /// a cleaner that ran seconds ago), so their timestamps and that offset sit on the same side of any + /// transition. These four do not: 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 values predate + /// the most recent transition and come back 60 minutes early — silently, and in the plausible + /// direction. The fleet is exposed rather than theoretically exposed: its targets report + /// utc_offset_minutes = -240, which is EDT, a DST-observing zone on summer time. + /// sqlserver_start_time on the same row is the bound on how far back that can reach. + /// Fixing it properly needs a ZONE rather than an offset — CURRENT_TIMEZONE_ID() + /// (SQL Server 2019+) collected alongside the offset, then AT TIME ZONE at the read boundary, + /// which handles transitions. That is a collected-column addition and a migration rung, so what is + /// carried here is the SCOPE of the claim, in the #2993 shape: this read places a timestamp exactly + /// when it falls inside the current DST period, and within an hour otherwise. /// The alias deliberately does NOT carry a _utc suffix, unlike the other fifteen. This one /// is a projection alias rather than a column, and ConsumedTimestampFrameDisciplineTests reaches /// the payload field through the alias — a suffix here would make the field name and the alias diverge diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingPvsReader.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingPvsReader.cs index 5e04fb402e..cb170b4e42 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingPvsReader.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingPvsReader.cs @@ -31,6 +31,14 @@ namespace PerformanceMonitor.Darling.Service.Mcp; /// exists to answer — the tool's own description sends a caller here "when ADR cleanup looks stuck" — so a /// phantom four-hour stall is the worst place for an unmarked frame. The trend read needs nothing: it /// returns only collection_time. +/// +/// Exact only inside the current DST period. The collected offset is the one in force at +/// collection time, so a cleaner timestamp from before the most recent transition is de-skewed by an +/// offset that was not in force when it was written and lands 60 minutes early. The off-row cleaner runs +/// continuously and is unaffected in practice, but aborted_version_cleaner_* can reach back weeks +/// on a quiet database — a live read showed 2026-08-26 against a 2026-09-09 snapshot — so it can cross a +/// transition. Same limit and same proper fix as DarlingObjectStatsReader records: a collected zone +/// plus AT TIME ZONE, rather than one offset applied to every age of value. /// internal static class DarlingPvsReader { From 8b8095732f781471dbdc0197351dce8b6621db21 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 9 Sep 2026 14:46:43 -0400 Subject: [PATCH 2/2] Argue the DST exposure from the product's supported range, not one estate's composition The note justified itself with a deployment fact -- the fleet's reported offset and its zone. A shipped product's source should not reason from one estate's composition: the exposure is that ANY target in a DST-observing zone has it, and on RDS a non-UTC zone is an ordinary creation-time parameter rather than an exotic setting, which also kills the plausible-sounding assumption that an RDS instance is probably UTC. #2932 already carries the measurement, so the general argument loses nothing and is checkable without a private fact. Same for the version gate: the offset path stays because the product supports servers older than SQL Server 2019, not because any particular fleet does or does not contain one. --- .../Mcp/DarlingObjectStatsReader.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingObjectStatsReader.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingObjectStatsReader.cs index e79252f7a3..4da3a897ba 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingObjectStatsReader.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingObjectStatsReader.cs @@ -195,9 +195,12 @@ public static async Task> GetObjectSizeGrowthAsync( /// transition. These four do not: 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 values predate /// the most recent transition and come back 60 minutes early — silently, and in the plausible - /// direction. The fleet is exposed rather than theoretically exposed: its targets report - /// utc_offset_minutes = -240, which is EDT, a DST-observing zone on summer time. - /// sqlserver_start_time on the same row is the bound on how far back that can reach. + /// direction. This is not a theoretical exposure: any target in a DST-observing zone has it, a target + /// configured to UTC does not, and on AWS RDS the instance takes its time zone from a creation-time + /// parameter — so a non-UTC zone is an ordinary configuration rather than an exotic one, and "it is + /// RDS, so it is probably UTC" is not a safe assumption. #2932 records the measured offset behind the + /// four-hour figure quoted above. sqlserver_start_time on the same row is the bound on how far + /// back the affected values can reach. /// Fixing it properly needs a ZONE rather than an offset — CURRENT_TIMEZONE_ID() /// (SQL Server 2019+) collected alongside the offset, then AT TIME ZONE at the read boundary, /// which handles transitions. That is a collected-column addition and a migration rung, so what is