Bind store window bounds instead of comparing naive UTC to now() by erikdarlingdata · Pull Request #2971 · erikdarlingdata/PerformanceMonitor

erikdarlingdata · 2026-09-05T01:24:38Z

Every timestamp column in both stores is timestamp without time zone holding naive UTC — measured, there is not one timestamptz column in either schema. now() is timestamptz. PostgreSQL resolves the mixed comparison by converting the NAIVE side at the store session's TimeZone, which initdb takes from the host OS and which BuildConfAppend does not pin, so a window silently widens west of UTC and inverts to nothing east of it. Nothing raises an error, and every store anyone develops or tests against runs UTC, which is why these read as correct in review.

Measured, running the shipped query strings

timescale/timescaledb:latest-pg17 (17.11) seeded one row per 52 seconds, and DuckDB 1.5.5 (the pinned version) seeded the same way:

	UTC	America/New_York	Pacific/Kiritimati
`collection_time > now() - interval '1 hour'`	70 rows	347	0
same window bound as `$1`	70	70	70
`ForcePlanFailuresSql` before	1 row	1	0 — alert dead
`ForcePlanFailuresSql` after	1	1	1
`BaselineBackfillProbeSql` `need_from` before	01:00	−4h	+14h
`BaselineBackfillProbeSql` `need_from` after	01:00	01:00	01:00
Lite 10-minute freshness floor before (DuckDB)	12 rows	289	0 — alert dead
Lite 10-minute freshness floor after (DuckDB)	12	12	12

The fix, and why parameters rather than `AT TIME ZONE 'UTC'`

Both spellings are correct. Binding a Kind-Unspecified DateTime from DateTime.UtcNow wins on four counts:

It is already the house convention. NaiveUtcNow() exists in four service files — including 79 lines below the offending predicate in DarlingAlertReadAdapter, which was the only windowed read in its own file not using it — and PgReadKindDisciplineTests already enforces the C# bind half of exactly this rule. This PR holds the SQL literal half, which that scan cannot see.
The bound is then the same clock that stamped collection_time (DarlingCollectorRunner writes DateTime.UtcNow through SpecifyKind(..., Unspecified)), so the two sides cannot disagree about what "two hours ago" means.
It is one spelling for both stores. DuckDB's AT TIME ZONE needs ICU; a parameter does not.
It makes the guard's rule absolute — no bare clock in a store-side comparison — rather than "a bare clock, but only in the blessed spelling". The blessed spelling also has a precedence trap: now() AT TIME ZONE 'UTC' - interval '2 hours' is correct only because AT TIME ZONE binds tighter than -, which is something you have to know.

Four predicates now bind their bound. The fifth, DarlingModuleMap.RefreshSql, keeps its bare now(): across UTC-12..UTC+14 its 48-hour window delivers 34–60 hours, which stays above the 24-hour refresh cadence (so no handle is missed between runs) and inside the 4-day raw retention (so the widened end scans nothing dropped). It is waived by arithmetic, not by being harmless; the query's remarks carry the derivation and the two numbers that have to keep holding, and the guard records it as its single exception.

Lite/Services/LocalDataService.WaitStats.cs was not in the original report — a span-level scan of the whole store-side literal corpus found it. It is the long-running-query alert's snapshot-freshness floor, and it fails in the same direction as the Darling alert.

And this one was already diagnosed in the repo. DarlingAlertReadAdapter.LongRunningQueriesSqlTemplate is that same query ported to Postgres, and its doc comment has said all along:

Lite's long-running-query read with the two PG dialect adjustments: DuckDB's bare NOW() - INTERVAL '10 MINUTES' becomes the parameterized naive-UTC $4 (a bare now() is timestamptz — wrong basis against naive-UTC timestamp columns)

So the defect was identified, correctly explained, and fixed on the Darling side at the moment of the port — and the Lite original it was ported from kept it. That is the strongest argument in this PR for a class-level guard rather than four fixes: the knowledge was already here, written down next to the fix, and it still did not propagate. ($4 is also the parameter number Darling's twin uses, so the two now match.)

The guard

StoreSqlClockDisciplineTests holds the class rather than the four instances: no store-side SQL literal may compare a naive collector timestamp column against a bare now() / CURRENT_TIMESTAMP / LOCALTIMESTAMP / now()::timestamp.

Reads literals through CSharpSourceWalker, which gains a StringLiteralBodies entry point rather than a sixth private lexer (The shared source-walker blanks interpolated-string holes, so a call inside an interpolation is invisible to every scan built on it #2913's lesson) — additive, and the mirror image of the existing StripCommentsAndStrings.
Strips SQL comments inside each literal before matching. The repo's SQL discusses now() constantly, including in the waiver this pin allows.
Scrapes its naive-column vocabulary from the DDL's own literals, so a column added by a new rung is covered the day it lands. sample_time is deliberately excluded: it is the one column name whose frame depends on the table (cpu_utilization_stats.sample_time is intentionally the monitored server's local clock per [FEATURE] Add a headless Windows Service collector mode for Lite (gMSA-compatible), with the existing UI as a read-only viewer #1262, memory_pressure_events.sample_time must be UTC), and CollectorTimestampFrameTests already pins both per column — its own remarks record that its first cut was a store-wide "all naive timestamps are UTC" rule that would have forbidden the CPU collector's intentional local clock. A name-keyed scan cannot tell those two apart and must not claim to.
Floors on what it scanned — 563 files, 19,604 literals, 1,626 SQL-shaped, 58 column names — so it cannot pass vacuously. The column floor is the load-bearing one: a scrape returning four names would hide every offender while still reporting thousands of literals.
Accepts bare predicate fragments, not just whole statements — these readers interpolate standalone filter literals (five in LocalDataService.WaitStats alone), so "AND r.collection_time >= NOW() - INTERVAL '10 MINUTES'" has no statement keyword and a statement-only filter walks past the shape most likely to reintroduce this. Proven both ways: that fragment as a mutation is caught with the arm and missed without it. The limit it does not reach is stated in the source — one literal at a time, so a predicate welded from two literals by concatenation is out of scope, and no such split exists today.
Discriminator pinned in both directions against 12 hazard forms and 15 benign ones. The benign set is the part that took the work: SET col = now(), VALUES (CURRENT_TIMESTAMP), alter_job(..., next_start => now()), TimescaleDB's genuinely-timestamptz catalog views, and a clock spelled only in a comment all appear in the corpus and must not be dragged in. One of those controls caught a real bug in the detector's own regex, where the optional ::timestamp ate the first eleven characters of ::timestamptz and made a correct now()::timestamptz AT TIME ZONE 'UTC' read as an offender.

Mutation-tested: reverting each of the four fixes turns it red, and neutering the waiver key exposes the module-map site — so the allowlist is what suppresses that one, not a blind spot in the detector.

Scope boundary, stated rather than allowlisted

The guard flags comparisons and clamps, not writes. Lite has six bare-clock writes (now()::TIMESTAMP into config_database_state_expected.updated_at, server_tags.created_at, and CURRENT_TIMESTAMP into the repair-marker tables). Those store local wall time in naive-UTC columns, which is a different defect shape — and all of them are inert today: none of those columns is read back or compared anywhere, and the one ordering use (ORDER BY attempted_at) is offset-invariant. collector_state.updated_at, which is compared against MAX(collection_time), is written from DateTime.UtcNow and is correct. Widening the guard to the write side would need six allowlist entries that misrepresent them as blessed, plus a data-migration question for existing stores, so it is deliberately out of scope here rather than quietly waived.

Defence in depth

DarlingManagedPostgres gains a v9 conf block pinning timezone = 'UTC'. Verified above: with the session pinned, even the un-fixed bare form returns the right answer. It rides the established versioned-marker mechanism, so existing field stores gain it on their next service-owned start rather than only a fresh initdb; timezone is SIGHUP-context and the append runs before pg_ctl start, so it is effective on that start. It is placed after the v8 hardware check and carries no fingerprint line, preserving v8's documented invariant about the text it reads.

This is a backstop, not the fix: it reaches managed stores only, and a bring-your-own store keeps whatever zone its owner built it with. The predicates are what actually hold.

Verification scope

The Windows-only suites cannot run on macOS. What ran here: all four projects plus Lite.Tests compile with EnableWindowsTargeting; the new guard's real source compiled into a net10.0 harness against the actual CSharpSourceWalker and executed (2/2, plus the five mutations); the shipped ForcePlanFailuresSql and BaselineBackfillProbeSql executed through Npgsql against live PostgreSQL 17.11 under three session zones; DuckDB 1.5.5 probed for the Lite half. Not run locally: the xUnit suites themselves — CI is the arbiter for those.

No CHANGELOG entry in this PR; the entry text is reported to the coordinating session for consolidation.

claude · 2026-09-05T01:31:09Z

Review summary

Reviewed the naive-UTC-vs-now() fix across Darling (Postgres) and Lite (DuckDB). The core change is sound and well-verified:

ForcePlanFailuresSql (both apps), BaselineBackfillProbeSql, and Lite's WaitStats freshness floor now bind their lower bound as a Kind-appropriate parameter computed from the service clock instead of comparing a naive collection_time against a bare now(). Spot-checked the arithmetic and parameter ordering in each ($1/$2/$4 positions line up with Parameters.Add order) — looks correct.
Confirmed by grep that Darling's LongRunningQueriesSqlTemplate was already parameterized this way pre-PR, so the Lite WaitStats fix brings it into parity rather than introducing new drift.
Spot-checked the remaining bare now()/CURRENT_TIMESTAMP occurrences left in both trees: all are either writes (SET/VALUES/DEFAULT, explicitly out of scope per the PR description and inert since unread) or genuine timestamptz columns (e.g. timescaledb_information.chunks.range_end), or already use the now() AT TIME ZONE 'UTC' rescue. Didn't find a comparison the guard/PR missed.
The DarlingModuleMap.RefreshSql waiver's 34–60 hour arithmetic (UTC-12..UTC+14 over a 48h window) checks out against the actual refresh cadence (daily purge + startup call).
The v9 timezone = 'UTC' conf block is appended before pg_ctl start (confirmed in EnsureConfAppended/StartAsync), so the SIGHUP-context setting is live on the same start that writes it, and it's placed after v8 so it can't disturb the hardware-fingerprint staleness check.

One correctness/consistency issue found in both apps (posted inline): in DarlingAlertReadAdapter.cs and Lite/Services/LocalDataService.ForcePlanFailures.cs, the new ForcePlanFailureWindow field's doc comment was stacked directly above the existing ForcePlanFailuresSql summary with no blank line between them, so the two merge into one XML doc comment that the compiler attaches to ForcePlanFailureWindow instead of ForcePlanFailuresSql. Result: the SQL constant loses its documentation entirely (the file's other query constants are all documented) and the TimeSpan field gets a summary that isn't about it, plus a malformed doc block with two sibling <summary> elements. Same mistake in both files — easy parity-preserving fix.

No SQL injection, secrets, or missing-index-DMV concerns found in this diff.

Every timestamp column in both stores is `timestamp without time zone` holding naive UTC; `now()` is `timestamptz`. PostgreSQL resolves the mixed comparison by converting the NAIVE side at the store session's TimeZone, which initdb takes from the host OS, so a window silently widens west of UTC and can invert to nothing east of it. No error is raised and every store anyone develops against runs UTC, which is why this survived review. Measured on timescale/timescaledb:latest-pg17 seeded one row per 52 seconds, running the shipped query strings: `collection_time > now() - interval '1 hour'` returned 70 rows at TimeZone=UTC, 347 at America/New_York and 0 at Pacific/Kiritimati; the bound form returned 70 in all three. Four predicates now bind the bound as a Kind-Unspecified DateTime computed from DateTime.UtcNow — the clock that stamped collection_time, and the convention the rest of these reads already follow (NaiveUtcNow, PgReadKindDisciplineTests): - DarlingAlertReadAdapter.ForcePlanFailuresSql, which returned 0 rows east of UTC, so the forced-plan-failure alert never fired there. - TimescaleSupport.BaselineBackfillProbeSql, whose `now()::timestamp` is LOCALTIMESTAMP: need_from moved +14h at Kiritimati, under-asking for baseline coverage. - Lite's LocalDataService.ForcePlanFailuresSql, the DuckDB twin. - Lite's long-running-query read, whose 10-minute freshness floor lands in the future east of UTC, so that alert returned nothing at all. DarlingModuleMap.RefreshSql keeps its bare now() and states why: across UTC-12..UTC+14 its 48-hour window delivers 34-60 hours, which stays above the 24-hour refresh cadence and inside the 4-day raw retention. It is waived by arithmetic, and the guard records it as the single exception. StoreSqlClockDisciplineTests holds the class: no store-side SQL literal may compare a naive collector timestamp against a bare clock. It reads literals through CSharpSourceWalker, which gains a StringLiteralBodies entry point rather than a sixth private lexer, strips SQL comments before matching, and carries floors on files/literals/columns scanned so it cannot pass vacuously. Its discriminator is pinned in both directions against ten hazard forms and thirteen benign ones, including the writes it deliberately does not claim. DarlingManagedPostgres gains a v9 conf block pinning timezone = 'UTC'. That is defence in depth, not the fix: it reaches managed stores only, and a bring-your-own store keeps whatever zone it was built with.

Three follow-ups from running the suites. DocCommentHygieneTests failed: inserting ForcePlanFailureWindow between ForcePlanFailuresSql's <summary> and the const itself left two stacked summaries on the const and no doc on the window. Both files now declare the window above the query's own doc block, so each member carries one summary. The adapter's measured figures are also corrected to the ones actually reproduced here (70 / 347, not 66 / 343) and now name the zero-row case. The guard's SQL filter accepts predicate FRAGMENTS, not just statements. These readers assemble filters as standalone literals and interpolate them — LocalDataService.WaitStats builds five that way — so "AND r.collection_time >= NOW() - INTERVAL '10 MINUTES'" carries no statement keyword and a statement-only filter skipped exactly the shape most likely to reintroduce this. Proven both ways: that fragment as a mutation is caught with the arm and MISSED without it. The known limit is stated in the source — one literal at a time, so a predicate split across concatenated literals is outside it, and no such split exists today. sample_time is excluded from the naive-UTC vocabulary. It is the one column name in the store whose frame depends on the table: cpu_utilization_stats.sample_time is deliberately the monitored server's LOCAL clock (#1262), while memory_pressure_events.sample_time must be UTC. CollectorTimestampFrameTests pins both per column, and records that its own first cut was a store-wide "all naive timestamps are UTC" rule that would have forbidden the CPU collector's intentional local clock. This scan keys on names, so it cannot tell those two apart and must not claim to.

The previous commit's message describes a fragment arm on the guard's SQL filter and four fragment controls. They are not in it. They were written, verified, and then destroyed: the mutation run that proved the arm load-bearing finished with `git checkout --` on this file to undo its mutation, and the arm was still UNCOMMITTED at that point, so the restore took it too. Only the sample_time exclusion, written afterwards, survived. That message overclaims and cannot be edited now without rewriting a commit a review is running against, so this records it instead. The arm itself: LooksLikeSql accepts a bare predicate FRAGMENT — a literal carrying both a clock and a comparison — as well as a whole statement. These readers assemble filters as standalone literals and interpolate them, five of them in LocalDataService.WaitStats alone, so "AND r.collection_time >= NOW() - INTERVAL '10 MINUTES'" has no statement keyword and the statement-only filter walked straight past the shape most likely to reintroduce this. Controls go from 10/13 to 12/15, covering the fragment in both directions, and the source states the limit the arm does NOT reach: one literal at a time, so a predicate welded together from two literals by concatenation is out of scope, and no such split exists today.

event_time is the second column name whose frame depends on the table, and excluding sample_time alone was a scenario-shaped fix. default_trace_events .event_time is server-LOCAL (the .trc files store local time) while system_health.event_time is UTC — ViewerSystemEventsTests puts it in one line: "system_health.event_time is UTC, default_trace.event_time is local". A name-keyed scan cannot tell those apart, so treating event_time as naive UTC was a claim the repo contradicts in four places. Both now sit in AmbiguousFrameColumns with the evidence for each and the rule for adding a third: a documented split, per table. CollectorTimestampFrameTests remains the authority, and the reason the split belongs there rather than here is that a server-local column compared against a bare clock IS still a defect — just a different one, whose fix is to de-skew by that server's offset rather than to bind naive UTC. Census in the floors comment corrected to what the scan actually reports now: 1,627 SQL-shaped literals and 56 column names.

claude · 2026-09-05T01:43:21Z

Reviewed the diff (Darling + Lite changes, the new StoreSqlClockDisciplineTests guard, CSharpSourceWalker.StringLiteralBodies, and the v9 postgresql.conf timezone pin).

Correctness — checked out clean:

ForcePlanFailuresSql ($2) and the WaitStats freshness floor ($4) bind in the same order the parameters are declared in each SQL string (Npgsql/DuckDB.NET bind positionally by add-order), so $1/$2/$3/$4 line up correctly in both DarlingAlertReadAdapter and LocalDataService.
NaiveUtcNow() - ForcePlanFailureWindow and DateTime.SpecifyKind(DateTime.UtcNow - BaselineRetentionSpan, DateTimeKind.Unspecified) both preserve DateTimeKind.Unspecified through the subtraction, matching the Strip the DateTimeKind before binding a timestamp parameter rule in CONTRIBUTING.md.
TimescaleSupport.BackfillBaselineAggregatesAsync's using/await using restructuring is correct — the parameter is added to probe before ExecuteReaderAsync is called, which is required and wasn't possible under the old nested-using shape.
The DarlingModuleMap.RefreshSql waiver's arithmetic checks out: with the 48h literal window and real UTC offsets (-12..+14), the effective window is 34-60h (west widens, east narrows) — verified against the call site (DarlingWorker.cs, daily purge cadence + startup) and RawRetentionInterval (4 days), so both the "no missed handle" and "no scan of dropped raw rows" claims hold.
v9 postgresql.conf block placement after v8 is consistent with the existing marker-append pattern (independent marker checks, no fingerprint line, appended before pg_ctl start so it's SIGHUP-effective immediately).

Lite/Darling parity — ForcePlanFailureWindow (2h) and the bind approach are duplicated shape-for-shape between Lite/Services/LocalDataService.ForcePlanFailures.cs and Darling/.../DarlingAlertReadAdapter.cs; LatestSnapshotFreshness (10m) in LocalDataService.WaitStats.cs matches the pre-existing Darling LongRunningQueriesSqlTemplate bind. No drift found.

Scope check — grepped both stores for remaining bare now()/CURRENT_TIMESTAMP/LOCALTIMESTAMP outside this diff; everything left is either a write (documented as explicitly out of scope), a TimescaleDB catalog column that's genuinely timestamptz (e.g. timescaledb_information.chunks.range_end in CompressionActivitySql), or the one documented DarlingModuleMap waiver. Nothing else needed the fix.

No security or performance concerns — all binds are parameterized, nothing new touches file/network/process boundaries. Nothing else to flag.

claude · 2026-09-05T01:50:44Z

Reviewed the diff (Darling + Lite clock-discipline fix, the CSharpSourceWalker literal-body extension, and the new StoreSqlClockDisciplineTests guard). Summary of what I checked and didn't find anything wrong with:

Parameter binding correctness: verified positional param order for all four rewritten queries (ForcePlanFailuresSql in both apps, BaselineBackfillProbeSql, and the Lite WaitStats freshness floor). Counts, ordinals ($1..$4), and DateTimeKind handling (Unspecified for Npgsql, plain DateTime.UtcNow for DuckDB, matching each app's existing convention) all line up correctly with what gets added to command.Parameters.
Guard test soundness: manually re-derived the discriminator's regex/predicate-span logic against the current corpus (grepped for collection_time/updated_at/last_seen etc. compared to now()/CURRENT_TIMESTAMP/LOCALTIMESTAMP across both stores, including inside GREATEST/LEAST clamps and the Viewer's write paths). Found no comparison left unfixed outside the one deliberately waived case (DarlingModuleMap.RefreshSql), and no case the scan would flag by mistake (TimescaleDB catalog reads like timescaledb_information.jobs.next_start/chunks.range_end are correctly out of the naive-column vocabulary since they're not scraped from the store's own DDL).
Parity: the two apps' fixes are shape-for-shape (ForcePlanFailuresSql window, long-running-query freshness floor). The Timescale-only fixes (BaselineBackfillProbeSql, the v9 postgresql.conf timezone pin) have no Lite counterpart, which is correct — continuous aggregates and a managed Postgres process don't exist on the DuckDB side.
DarlingManagedPostgres v9 block: placement after v8, no fingerprint line, conf snapshot read once at top of method — consistent with the existing v1-v8 idempotency pattern.

One minor, non-blocking observation: the new window-length constants (ForcePlanFailureWindow = 2h in both apps, LatestSnapshotFreshness = 10m in Lite vs. the equivalent inline AddMinutes(-10) in Darling's LongRunningQueriesSqlTemplate) aren't pinned equal by a test the way BaselineRetentionSpan/BaselineRetentionInterval are. Nothing today would catch one side's window drifting from the other's. Not asking for a change, just flagging since it's the one place in this PR where cross-app value parity relies on eyeballing rather than a test.

No correctness, security, or Lite/Darling parity issues found.

claude Bot reviewed Sep 5, 2026

View reviewed changes

Comment thread Darling/PerformanceMonitor.Darling.Service/DarlingAlertReadAdapter.cs Outdated

claude Bot reviewed Sep 5, 2026

View reviewed changes

Comment thread Lite/Services/LocalDataService.ForcePlanFailures.cs Outdated

erikdarlingdata added 4 commits September 4, 2026 21:43

erikdarlingdata force-pushed the fix/naive-utc-vs-now-window branch from 6949d77 to 040b8f6 Compare September 5, 2026 01:43

erikdarlingdata merged commit d83db8b into dev Sep 5, 2026
8 of 9 checks passed

This was referenced Sep 5, 2026

Record the CHANGELOG entries for twenty merged changes #2980

Merged

Take the default trace's archival-empty cutoff from the server's clock #2985

Merged

erikdarlingdata deleted the fix/naive-utc-vs-now-window branch September 12, 2026 20:30

Navigation Menu

Bind store window bounds instead of comparing naive UTC to now() - #2971

Bind store window bounds instead of comparing naive UTC to now()#2971
erikdarlingdata merged 4 commits into
devfrom
fix/naive-utc-vs-now-window

erikdarlingdata commented Sep 5, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Sep 5, 2026

Uh oh!

claude Bot commented Sep 5, 2026

Uh oh!

claude Bot commented Sep 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

erikdarlingdata commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Measured, running the shipped query strings

The fix, and why parameters rather than AT TIME ZONE 'UTC'

The guard

Scope boundary, stated rather than allowlisted

Defence in depth

Verification scope

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Sep 5, 2026

Review summary

Uh oh!

claude Bot commented Sep 5, 2026

Uh oh!

claude Bot commented Sep 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

erikdarlingdata commented Sep 5, 2026 •

edited

Loading

The fix, and why parameters rather than `AT TIME ZONE 'UTC'`