Skip to content

Fix the late-row fixed-clock test that fails every run since 2026-10-10 (#5661) - #5662

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/5661-late-row-test-clock
Oct 11, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/5661-late-row-test-clock

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Fixes #5661.

Summary

The four fixed-clock cases of ADayMarkedByALateRow_DropsOutOfTheBuiltDays_AndIsReadLive_AtAFixedClock now 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 its first_execution_time is 1 to 17 days before the database's own now(). 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, and Assert.DoesNotContain fails. The product code is right. The pinned date was the bug.

What I checked

  • A clock later in the day than the real time (for example 12:00 when the run is at 01:00): SeedAndBuildAsync is safe. The seeded rows are inserted with the trigger disabled, so a "future" row cannot trip its bounds. PlanRegressionDaily.BuildDayAsync uses the passed clock only to stamp built_at. The plan regression fact read in PgFactCollector has no now() 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.
  • Other live tests with pinned calendar dates that reach store SQL reading 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 by now(). Those are the 2026-01-01 collection_time base, the 2026-03 seed days and the directly inserted plan_regression_daily rows. The 2026-03 data is already past the trigger's reach, so it cannot change as the date moves.
    • QueryStoreComposeStampWriterGuardLiveTests pins 2026-10-08 only for an in-process deferral clock, not store SQL.
    • The interval partition, convergence and bounded-read pruning classes, SlowReadsLiveTests, LongQueryTraceRunRowLiveTests, ProcedureStatsFloorAndHistoryLiveTests and the two PgDeadlockRemask*PagingLiveTests. Their pinned dates are passed to the code under test as its clock, or are stored values. None is compared with the database now().
    • The timeout-visibility test in the same file pins 2026-10-07 for a caveat store that takes its time as a parameter.
    • I found no other test with this failure shape.

Test plan

  • Darling.Tests builds with 0 warnings, 0 errors.
  • PlanRegressionBuiltDaysFallbackLiveTests: 10 skipped (no PostgreSQL on this machine, as the brief set).
  • Census, Ratchet and DocCommentHygiene classes: Total 593, Failed 0, Skipped 1.
  • The live result is decided by the CI Darling PostgreSQL job (shard 3).

)

The V168 late-row trigger only marks rows 1 to 17 days before the database's now(), so the pinned date aged out of its reach. The cases now use today's UTC date plus their hour and minute.
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 11, 2026 15:17

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 single DateTime.UtcNow.Date read threaded through as one utcNow anchor into SeedAndBuildAsync.
  • 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-only shows 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 when first_execution_time is within [now()-17d, now()-1d) of the database's real clock. I independently recomputed the pinned-date math: with clock 2026-10-07, the late row lands at 2026-09-22 (hour<4 cases) or 2026-09-23 (hour>=4 cases); from real-now 2026-10-10 the reach is [2026-09-23, 2026-10-09), failing the two hour<4 cases; from 2026-10-11 the 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 same DARLING_TEST_PG skip.
  • UNVERIFIED — "Darling.Tests builds with 0 warnings, 0 errors" — running dotnet build was 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-checked PlanRegressionDailyTriggerLiveTests.cs, which already anchors on DateTime.UtcNow.Date rather than a hardcoded far date, consistent with the claim, but did not re-verify every other file the body lists (QueryStoreComposeStampWriterGuardLiveTests, SlowReadsLiveTests, the PgDeadlockRemask*PagingLiveTests, etc.).

Riskiest lines

  1. 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 four AtAFixedClock theory cases, run against live PostgreSQL in CI (not in this sandbox).
  2. 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 by Assert.Contains/Assert.DoesNotContain at lines 172-173, 178-179.
  3. Darling/Darling.Tests/PlanRegressionBuiltDaysFallbackLiveTests.cs:346-350 (unchanged) — SeedAndBuildAsync's derivation of now/start/end/windowDay from the single passed-in anchor — pinned by Assert.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.Date is read exactly once at PlanRegressionBuiltDaysFallbackLiveTests.cs:154 and the resulting value is threaded as the single utcNow parameter through RunLateRowAsync → 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.Tests returns 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_AtAFixedClock against live Postgres and confirm all four cases now pass (the author's own test plan already flags this as the only remaining unchecked item).

@erikdarlingdata
erikdarlingdata enabled auto-merge (squash) October 11, 2026 15:28
@erikdarlingdata
erikdarlingdata merged commit b5b53e8 into dev Oct 11, 2026
35 of 38 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/5661-late-row-test-clock branch October 11, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant