Repository navigation
Fix the late-row fixed-clock test that fails every run since 2026-10-10 (#5661) - #5662
Merged
Merged
Conversation
erikdarlingdata
marked this pull request as ready for review
October 11, 2026 15:17
There was a problem hiding this comment.
Verdict: LGTM
Claims
- VERIFIED — the four fixed-clock cases now build their clock from today's UTC date plus the case's hour/minute, with the date read once —
Darling/Darling.Tests/PlanRegressionBuiltDaysFallbackLiveTests.cs:154, a singleDateTime.UtcNow.Dateread threaded through as oneutcNowanchor intoSeedAndBuildAsync. - VERIFIED — it used to be pinned to
2026-10-07— confirmed by the removed line in the diff (new DateTime(2026, 10, 7, hour, minute, 0, DateTimeKind.Utc)). - VERIFIED — the four inline cases (00:30, 03:59, 04:00, 12:00) are unchanged — same
[InlineData]attributes before and after, lines 149-152. - VERIFIED — the docstring now explains why the date is today's — lines 140-146 add the
now()/#5661 rationale. - VERIFIED — test-only, no product code changed, no changelog entry —
gh pr diff --name-onlyshows exactly one file touched, the test file itself. - VERIFIED — root cause:
trg_plan_regression_daily_late(V168,PgMigrations.cs:815-820) only marks a row whenfirst_execution_timeis within[now()-17d, now()-1d)of the database's real clock. I independently recomputed the pinned-date math: with clock2026-10-07, the late row lands at2026-09-22(hour<4 cases) or2026-09-23(hour>=4 cases); from real-now2026-10-10the reach is[2026-09-23, 2026-10-09), failing the two hour<4 cases; from2026-10-11the reach is[2026-09-24, 2026-10-10), failing all four — matching the PR body's claim exactly. - VERIFIED — "10 skipped (no PostgreSQL)" — the file has exactly 10
[Fact]/[Theory]-case test methods (1+1+1+4+1+1+1), each gated by the sameDARLING_TEST_PGskip. - UNVERIFIED — "
Darling.Testsbuilds with 0 warnings, 0 errors" — runningdotnet buildwas blocked by the sandbox's approval gate in this session; not re-attempted since this is a test-only change with no production-code risk. - UNVERIFIED — "Census, Ratchet and DocCommentHygiene classes: Total 593, Failed 0, Skipped 1" — requires running the local-only test suite, not checkable from a read-only tree.
- UNVERIFIED — the body's audit that no other live test in the repo shares this pinned-date-vs-trigger-
now()failure shape — I spot-checkedPlanRegressionDailyTriggerLiveTests.cs, which already anchors onDateTime.UtcNow.Daterather than a hardcoded far date, consistent with the claim, but did not re-verify every other file the body lists (QueryStoreComposeStampWriterGuardLiveTests,SlowReadsLiveTests, thePgDeadlockRemask*PagingLiveTests, etc.).
Riskiest lines
Darling/Darling.Tests/PlanRegressionBuiltDaysFallbackLiveTests.cs:154— the new anchor computation that replaces the hardcoded date; if it ever drifted from the database's real clock the trigger window math breaks again — pinned by the fourAtAFixedClocktheory cases, run against live PostgreSQL in CI (not in this sandbox).Darling/Darling.Tests/PlanRegressionBuiltDaysFallbackLiveTests.cs:169(unchanged) —PlanRegressionWindowFloor(periodStart.AddDays(-WindowDays)), the value the late row and the trigger window math both key off — pinned byAssert.Contains/Assert.DoesNotContainat lines 172-173, 178-179.Darling/Darling.Tests/PlanRegressionBuiltDaysFallbackLiveTests.cs:346-350(unchanged) —SeedAndBuildAsync's derivation ofnow/start/end/windowDayfrom the single passed-in anchor — pinned byAssert.True(fact.Value > 1)at line 186, which fails if the late row and the built-days window ever desync.
Traps near this diff
- "A test that reads the clock twice measures the CI scheduler... One anchor owns both ends of an interval" (#3676) — does not apply:
DateTime.UtcNow.Dateis read exactly once atPlanRegressionBuiltDaysFallbackLiveTests.cs:154and the resulting value is threaded as the singleutcNowparameter throughRunLateRowAsync→SeedAndBuildAsync; nothing re-reads the C# clock afterward. - "Lite and Darling ship in parity... a fix to one SKU's reader... without the counterpart's is a finding" (#3594) — does not apply:
grep -rln "PlanRegressionWindowFloor\|plan_regression_daily_built\|BuiltDaysFallback" Lite Lite.Testsreturns nothing — this built-days/late-row trigger mechanism is Postgres/Darling-only, with no Lite counterpart to keep in parity.
Divergence (blind read vs PR body)
None — the body's account matches the diff and my independent recomputation of the trigger-window math on every claim graded above.
Reviewer checklist
- Let the CI Darling PostgreSQL job (shard 3) actually run
ADayMarkedByALateRow_DropsOutOfTheBuiltDays_AndIsReadLive_AtAFixedClockagainst live Postgres and confirm all four cases now pass (the author's own test plan already flags this as the only remaining unchecked item).
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
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.
Claude posting for Erik Darling.
Fixes #5661.
Summary
The four fixed-clock cases of
ADayMarkedByALateRow_DropsOutOfTheBuiltDays_AndIsReadLive_AtAFixedClocknow build their clock from today's UTC date plus the case's hour and minute. The date is read once. It used to be the pinned day 2026-10-07. The four cases (00:30, 03:59, 04:00, 12:00) are unchanged. The docstring now says why the date is today's. Test-only: no product code changed, no changelog entry.Root cause
The late-row trigger from migration V168 (
trg_plan_regression_daily_late) marks a row only when itsfirst_execution_timeis 1 to 17 days before the database's ownnow(). With the clock pinned to 2026-10-07 the test's late row lands on 2026-09-22 (cases before 04:00) or 2026-09-23. From 2026-10-10 that is past the 17-day reach for two cases, and from 2026-10-11 for all four. The trigger skips the row, its day stays in the built days, andAssert.DoesNotContainfails. The product code is right. The pinned date was the bug.What I checked
SeedAndBuildAsyncis safe. The seeded rows are inserted with the trigger disabled, so a "future" row cannot trip its bounds.PlanRegressionDaily.BuildDayAsyncuses the passed clock only to stampbuilt_at. The plan regression fact read inPgFactCollectorhas nonow()in it. The late row sits at the window floor plus 10 hours. The floor is 14 or 15 days before today. That is inside the trigger's[today - 17 days, today - 1 day)reach at every hour of the day. A run that crosses UTC midnight still has a full day of margin.now():PlanRegressionDailyTriggerLiveTests,PlanRegressionDailyBuilderLiveTests,QueryStoreComposeStampLiveTests,QueryStoreBackgroundIndexesLiveTests: the trigger-driven checks already seed relative to the real or database clock. Their fixed dates are not windowed by the trigger or bynow(). Those are the 2026-01-01collection_timebase, the 2026-03 seed days and the directly insertedplan_regression_dailyrows. The 2026-03 data is already past the trigger's reach, so it cannot change as the date moves.QueryStoreComposeStampWriterGuardLiveTestspins 2026-10-08 only for an in-process deferral clock, not store SQL.SlowReadsLiveTests,LongQueryTraceRunRowLiveTests,ProcedureStatsFloorAndHistoryLiveTestsand the twoPgDeadlockRemask*PagingLiveTests. Their pinned dates are passed to the code under test as its clock, or are stored values. None is compared with the databasenow().Test plan
Darling.Testsbuilds with 0 warnings, 0 errors.PlanRegressionBuiltDaysFallbackLiveTests: 10 skipped (no PostgreSQL on this machine, as the brief set).