Drop an empty superseded baseline aggregate on sight (#4289)

[erikdarlingdata]

Closes #4289.

Why

JudgeSupersededBaselineRelationAsync dropped a legacy perfmon_baseline or wait_stats_baseline continuous aggregate only once its successor's oldest bucket reached the tier horizon. On a store where nothing ever fed wait_stats or perfmon_stats, the successor held no rows, so its oldest bucket was NULL. NULL never compares <= any horizon. So the verdict was SuccessorShort on every start, and the empty legacy aggregate never dropped, nor did its refresh, retention and compression jobs. #4289's store had 26 continuous aggregates where every other store has 24.

The wait protects history that a legacy aggregate holds and its successor has not backfilled yet. An empty legacy aggregate holds no history, so the wait does not apply to it.

What changes

In Darling/PerformanceMonitor.Darling.Storage/TimescaleSupport.cs:

• JudgeSupersededBaselineRelationAsync now asks whether the legacy aggregate holds any rows, through its own view: SELECT EXISTS (SELECT 1 FROM collect.<legacy>) (BaselineRelationHasRowsSql). It asks right after it confirms the legacy is a continuous aggregate, before it reads the successor's coverage. An empty legacy aggregate goes straight to Drop, and the coverage read never runs for it.
• The view read includes real-time rows. Both legacy aggregates are created with materialized_only = false, so on a store with source rows the probe sees them and nothing drops.
• SupersededBaselineRelationDropsAt, the pure rule, takes a new legacyHoldsRows argument. A continuous aggregate with no rows drops, whatever its successor holds. One with rows keeps the tier-coverage rule. A plain view is not affected.
• SupersededBaselineVerdict carries LegacyHoldsRows, so the live tests can check it and the drop's log line gives the real reason ("it held no rows through its own view"). Before, that line was built from the successor's NULL oldest bucket.
• Nothing else changes: not the coverage rule, the plain-view rule, LegacyAbsent, SuccessorAbsent, or the Retire the orphaned cpu_utilization_baseline / file_io_baseline continuous aggregates (#1995 cleanup) #2007 RetiredBaselineRelations list. The affected store cleans up on its next start, with no manual step.

Lite has no continuous aggregates (Lite/Analysis/BaselineProvider.cs:93), so it has nothing to mirror.

Review fixes

A focused review cleared the PR and found two Lows. Commit 92bcaa19 fixes both:

• The probe's answer is read as is not false, not is true. Only a definite false from the EXISTS probe means empty. Any other answer, such as null, counts as "holds rows", so it can never drop an aggregate.
• Three comments in TimescaleSupport.cs still said that a legacy aggregate drops only when its successor covers the tier. They now state the Darling: an empty superseded baseline aggregate is never dropped when its successor is empty too #4289 rule as well. A comment in RetiredBaselineAggregateTests.cs named the wrong class for the new test, and now names the right one.

Tests

• BaselineSupplyTests.RetirementCondition_EmptyLegacyAggregateDropsOnSight_RegardlessOfSuccessor (no database): an empty legacy aggregate drops whether its successor is empty, short or covering. RetirementCondition_HoldsOnlyOnceTheSuccessorCoversTheTier now passes legacyHoldsRows: true.
• EmptySupersededBaselineLiveTests.EmptyLegacyAggregate_DropsOnSight_EvenWithAnEmptySuccessor_AgainstDevPostgres (in RetiredBaselineAggregateTests.cs): an empty legacy aggregate with an empty successor drops. It makes its own scratch database. The probe reads every server's rows, and other tests leave wait_stats rows on the shared darlingtest database, so the legacy aggregate there is not empty. With the probe forced to legacyHoldsRows = true, as before this PR, it failed with "Expected: Drop, Actual: SuccessorShort". With the fix restored, it passed.
• EmptySupersededBaselineLiveTests.LegacyProbeFails_LegacySurvives_AgainstDevPostgres: a second connection holds an ACCESS EXCLUSIVE lock on the legacy aggregate, and the judged connection has lock_timeout = '1s'. So the probe fails, and the test checks that the legacy aggregate survives and nothing drops. A timer releases the lock after 1.5 seconds. That way, a drop that the code tries by mistake succeeds, and the test sees it. With a temporary catch that swallowed the probe error and answered "empty", the test failed (1 dropped). With the catch removed, it passed.
• RetiredBaselineAggregateLiveTests.Supersession_RestartZeroInLegacyNotSuccessor_ProviderFollowsCoverage_SweepWaitsForTheTier_AgainstDevPostgres now also checks LegacyHoldsRows on day one (rows and a short successor: no drop) and on day forty (rows and a covering successor: drop).

Test plan

• dotnet build of PerformanceMonitor.Darling.Storage and Darling.Tests: 0 warnings, 0 errors.
• EmptySupersededBaselineLiveTests, RetiredBaselineAggregateLiveTests, RetiredBaselineAggregateTests and BaselineSupplyTests against a local PostgreSQL rig (TimescaleDB 2.28.1): 30 tests, 0 failed.
• Full Darling.Tests once on that rig, on a new darlingtest database: the runner exited 0, with no [FAIL] line. The lane ran out of context before it copied the totals, and its log is gone.
• After the review fixes (92bcaa19), on a new rig (PostgreSQL 18.6 with TimescaleDB): RetiredBaselineAggregateTests, EmptySupersededBaselineLiveTests, BaselineSupplyTests and DocCommentHygieneTests ran 106 tests, 0 failed. RetiredBaselineAggregateLiveTests ran 2 tests, 0 failed.
• GitHub Actions CI on 92bcaa19: build, Darling PostgreSQL tests and Lite tests all pass. The first build attempt failed on one unrelated test, AlertReadFailureSurfaceTests.TheNewestFailuresFactsAreNeverABlendOfTwo (a race in the test itself, filed as AlertReadFailureSurfaceTests.TheNewestFailuresFactsAreNeverABlendOfTwo fails when the writers finish before the reader's first check #4312); the re-run passed.

CHANGELOG entry

SECTION: Fixed
ENTRY:

• Empty legacy baseline aggregates now drop instead of staying forever ([Drop an empty superseded baseline aggregate on sight (#4289) #4292]) - On a store where nothing ever fed the old perfmon_baseline or wait_stats_baseline aggregate, both it and its replacement stayed empty. The empty replacement has no oldest bucket, so the old aggregate's cleanup rule never passed. Now an old aggregate that holds no rows drops on the next start, with its refresh, retention and compression jobs.
  REF:
  [Drop an empty superseded baseline aggregate on sight (#4289) #4292]: Drop an empty superseded baseline aggregate on sight (#4289) #4292

🤖 Generated with Claude Code

https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ

[erikdarlingdata]

Focused review at 0793909: the empty-legacy drop

Scope: SupersededBaselineRelationDropsAt, JudgeSupersededBaselineRelationAsync and BaselineRelationHasRowsSql in Darling/PerformanceMonitor.Darling.Storage/TimescaleSupport.cs, plus their pins in RetiredBaselineAggregateTests.cs and BaselineSupplyTests.cs. I read the code at 0793909. I ran no tests. Line numbers are at 0793909.

Findings

High

None.

Medium

None.

Low

1. No test drives the probe-error arm. Also, the has-rows read treats every answer that is not true as "empty".

   • Where: TimescaleSupport.cs:1331 (legacyHoldsRows = await probe.ExecuteScalarAsync(cancellationToken) is true). The catch in the caller is at TimescaleSupport.cs:1160.
   • Today the error arm is correct. JudgeSupersededBaselineRelationAsync has no catch of its own. An exception from the EXISTS read leaves the method, reaches the catch at line 1160, logs "Could not judge or drop", and skips the DROP at line 1145. Cancellation propagates. The next pass judges again.
   • The risk is a later edit. Sequence: an edit wraps the read in a local try/catch that returns false, or changes the SQL so that it can return no row. ExecuteScalarAsync returns null, is true gives false, and the pass drops an aggregate that holds rows. No test fails.
   • Fix: write is not false at line 1331, so that only a definite false can drop. Add one own-store live test in which the judge fails and the legacy survives. For example, hold LOCK TABLE collect.wait_stats_baseline IN ACCESS EXCLUSIVE MODE in an open transaction on a second connection, and set lock_timeout = '1s' on the judged connection. Then assert that DropRetiredBaselineAggregatesAsync returns 0 and that the legacy still exists. A REVOKE SELECT does not work if the test role is a superuser.

2. Some comments still say that the drop waits only on coverage, and one test comment names the wrong class.

   • TimescaleSupport.cs:1099-1101 says the pass drops each legacy relation "ONLY when its successor covers the baseline tier".
   • TimescaleSupport.cs:1215-1221: SuccessorShort and Drop describe only the coverage case and the plain-view case. SuccessorShort now also means that the legacy holds rows. Drop now also covers an empty legacy aggregate.
   • TimescaleSupport.cs:438-440: the registry summary starts with "dropped only once its successor COVERS the baseline tier". The new paragraph at lines 481-488 states the exception, but the first sentence is now false.
   • RetiredBaselineAggregateTests.cs:305-306 names RetiredBaselineAggregateTests.EmptyLegacyAggregate_DropsOnSight_.... The test is in EmptySupersededBaselineLiveTests (line 433).
   • Effect: a reader of the enum or the pass summary expects that a legacy with a short successor never drops. That reader can then take a Darling: an empty superseded baseline aggregate is never dropped when its successor is empty too #4289 drop line in the log for a defect.
   • Fix: add "or it holds no rows (Darling: an empty superseded baseline aggregate is never dropped when its successor is empty too #4289)" in the three TimescaleSupport.cs places, and correct the class name in the test comment.

Q1. After the drop, does any code path still name the legacy relation?

Nothing found. I read these:

• PgBaselineProvider.ChooseSupplyAsync (PgBaselineProvider.cs:738-778) runs inside every compute (line 822). It checks that the legacy exists (line 756) and names the legacy only when that check says yes (lines 761-766). After the drop, it reads the successor.
• Before the drop, an empty legacy already lost to the successor. TimescaleSupport.PrefersSuccessor (TimescaleSupport.cs:1553-1558) returns the successor when legacyOldest is null. So the earlier drop changes no answer that the provider gives.
• One narrow race exists, and it is not new. The sweep can drop the legacy between the existence check (line 756) and the min(bucket) read (line 766). That read then fails with "relation does not exist". The catch at PgBaselineProvider.cs:910 returns null (line 937). That metric has no baseline for that one pass. The next compute checks again and reads the successor. The coverage drop before this PR had the same race. The failure stays inside the provider, and nothing throws to the caller.
• The ensure, the backfill and the stale-fallback sweep all iterate BaselineAggregates (TimescaleSupport.cs:682, 883 and 5292). BaselineBackfillProbeSql is called only from the backfill loop (line 691). That list holds only the successor names, and BaselineSupplyTests.cs:470-471 pins it. So no sweep can create again, or read, a dropped legacy.
• Refresh, retention and compression setup: no setup code names the legacy. In TimescaleSupport.cs, the names occur outside comments in four places only. These are the constants (lines 411-412), the SupersededBaselineRelations list (lines 490-494), the LegacyCreate*Sql texts (lines 558 and 574) and SourceTableFor (lines 1377-1378). Only test fixtures run the LegacyCreate*Sql texts. The own jobs of the legacy go with DROP MATERIALIZED VIEW ... CASCADE (line 1082).
• The Brains-review campaign: deferred structural residue (from #3538 / #3539 / #3540 / #3541) #3653 freeze has not shipped. Its notes (TimescaleSupport.cs:6679-6682) are about the hourly legacy trio, not this pair.
• Views, MCP tools and the web viewer: git grep for both names over Darling/ finds no .sql file and no other non-C# file. It finds no MCP, web or view reader. The hits in PgMigrations.cs are doc comments.
• Tests: on the shared test database, only RetiredBaselineAggregateLiveTests creates a legacy. It drops the pair before it starts (RetiredBaselineAggregateTests.cs:224-225) and again in finally (lines 363-368). So the Assert.Equal(2, dropped) in the sweep test (line 156) cannot count an empty legacy that another test left. The new test uses its own scratch database.

Q2. Can the probe say "empty" while the legacy holds history that no other relation has?

Nothing found. The reason:

• The only history that the legacy alone holds is materialized buckets whose raw rows aged out. Raw keeps 30 days by default, and the legacy keeps 35 (BaselineRetentionInterval). Those rows exist only in the materialization.
• Materialized rows are visible through the view in both shapes. With materialized_only = true, the view reads the materialization hypertable. With false, it reads the materialized rows below the watermark, plus raw at or after it. The watermark is the end of the newest materialized bucket, so every materialized row is below it. Both legacy CREATE texts set false (TimescaleSupport.cs:559 and 575).
• Compressed materialization chunks: the scan decompresses them, so a compressed row counts.
• Refresh in progress: a refresh replaces the rows of a range inside one transaction (one per batch when buckets_per_batch is set). A READ COMMITTED read sees the state before the commit or after it. It never sees the old rows gone before the new rows arrive. Rows that a refresh hides for a moment come from raw, so raw still holds them.
• Refresh job removed: no shipped code removes the refresh job of the legacy (see Q1). If an operator removes it, the materialized rows stay visible through the view. Only the retention job can empty the legacy, and then the legacy holds nothing.
• Role: the pass runs on the store connection of the service. git grep finds no ROW LEVEL SECURITY or CREATE POLICY in the store code, so no role sees fewer rows than the view holds. A role without SELECT gets a "permission denied" error, not an empty answer.
• Probe error: the exception reaches the catch at TimescaleSupport.cs:1160, and the legacy is not dropped. Low 1 covers the missing test.

Q3. Can the probe run before a legacy aggregate is first refreshed, on a store with source rows?

It can run then, but a drop at that time loses nothing.

• Only builds before Brains-review campaign: deferred structural residue (from #3538 / #3539 / #3540 / #3541) #3653 created the legacy pair. This build does not create it, because BaselineAggregates does not name it.
• The sweep runs at start and every hour (DarlingWorker.cs:363-364, the Ungated stage). So it can judge a legacy that was never refreshed.
• With materialized_only = false, a legacy that was never refreshed has no watermark, so the view aggregates all raw rows that pass its filter. On a store with source rows, the probe says "holds rows", and the legacy waits on coverage as before.
• With materialized_only = true, the probe says "empty", and the legacy drops. But that legacy holds no rows of its own. Raw holds them, and BackfillBaselineAggregatesAsync fills the successor from raw. The provider already read the successor in this state (Q1).
• Cost, not a finding: on a legacy that was never refreshed, the probe aggregates the raw tail. SetupTimeoutSeconds (300 seconds) limits it. A timeout is an error, so the legacy is not dropped.

Q4. Log text, the neutral LegacyHoldsRows, and the test for each arm

• Log text: the text is true. The empty-arm reason (TimescaleSupport.cs:1157) prints only when the legacy is a continuous aggregate and the probe found no rows. That is exactly the new arm. The coverage arm and the plain-view arm print the same words as before. The successor name moved from the {Successor} placeholder into {Reason}, so log sinks lose that structured property. Nothing in the repo reads it.
• LegacyHoldsRows = true on the paths that never ask: every earlier decision is the same. LegacyAbsent and SuccessorAbsent return before the predicate (lines 1306 and 1314), and on those paths the value feeds nothing. For a plain view, the predicate returns at its first branch (line 1264) and does not read the value. For an aggregate with rows, the flow is the old flow plus one EXISTS read. One difference: if that read fails on an aggregate that holds rows, the pass does not reach the coverage read. The next pass tries again. That is the safe direction.
• Test for each arm:

Arm	Pure test	Live test
Empty legacy, empty successor: drop	RetirementCondition_EmptyLegacyAggregateDropsOnSight_RegardlessOfSuccessor (BaselineSupplyTests.cs)	EmptySupersededBaselineLiveTests, on its own scratch database
Empty legacy, short successor: drop	The same pure test, with successorOldest 30 days back	None. The judge skips the successor read after it finds an empty legacy, so the live empty/empty test runs the same path.
Legacy with rows, short successor: no drop	RetirementCondition_HoldsOnlyOnceTheSuccessorCoversTheTier	Day-one assertions, RetiredBaselineAggregateTests.cs:301-310, now with LegacyHoldsRows true
Probe error: no drop	None	None (Low 1)

Clears for the build: yes. The probe cannot report "empty" while the legacy holds history that raw and the successor lack. No reader breaks after the drop. The two Lows change no behavior today.

[erikdarlingdata]

Pushed commit 92bcaa1 on fix/4289-empty-legacy-baseline-drop (head was 0793909), fixing both Lows from the focused review at #4292 (comment).

Low 1: probe answer must mean "holds rows" only when definitely false

• Darling/PerformanceMonitor.Darling.Storage/TimescaleSupport.cs:1331 (now ~1333): changed legacyHoldsRows = await probe.ExecuteScalarAsync(cancellationToken) is true; to is not false, so only a definite false from the EXISTS probe can mean empty. A future null (a swallowed error, or SQL changed to return no row) now defaults to "holds rows", the safe direction.
• Added EmptySupersededBaselineLiveTests.LegacyProbeFails_LegacySurvives_AgainstDevPostgres in Darling/Darling.Tests/RetiredBaselineAggregateTests.cs, next to the existing empty-legacy live test in the same class. A second connection holds LOCK TABLE collect.wait_stats_baseline IN ACCESS EXCLUSIVE MODE in an open transaction; the judged connection carries lock_timeout = '1s'. DropRetiredBaselineAggregatesAsync and a 1.5-second release timer both start at once, so the has-rows probe times out around 1s (uncaught inside JudgeSupersededBaselineRelationAsync, caught by the per-relation catch in the caller) and the lock releases independently at 1.5s. Asserts DropRetiredBaselineAggregatesAsync returns 0 and the legacy relation still exists.
  • Note on the design: locking the relation for the whole call blocks the eventual DROP statement too (same lock, same session lock_timeout), which would make dropped stay 0 regardless of whether the probe-error arm is handled correctly — a test that can't fail. Releasing on its own fixed timer, independent of when the drop pass gets there, means a wrongly-attempted drop (the regression below) actually gets to run once unblocked, so it shows up as dropped=1 rather than being silently swallowed by the same lock.
  • REVOKE SELECT was not an option: ScratchPostgres connects as a superuser, which bypasses grants, matching the review's note.
• Revert-proved: temporarily wrapped the has-rows probe in JudgeSupersededBaselineRelationAsync with try { ... } catch { legacyHoldsRows = false; } (swallow the probe error, answer "empty" — the risky edit the review described). Ran the new test alone: it failed (Assert.Equal() Failure: Expected 0, Actual 1) — the legacy dropped. Removed the temporary catch, rebuilt: it passes.

Low 2: stale "coverage only" comments

Added "or it holds no rows (#4289)" (in each location's own words) to:

• TimescaleSupport.cs registry summary's first sentence (~438-440)
• the "the same pass also walks" paragraph (~1099-1101)
• the SuccessorShort enum doc (now explicit that it only applies when the legacy holds rows) and Drop enum doc (now covers the empty-legacy-aggregate case), ~1215-1221
• RetiredBaselineAggregateTests.cs ~305-306: corrected the comment's class reference from RetiredBaselineAggregateTests.EmptyLegacyAggregate_DropsOnSight_... to EmptySupersededBaselineLiveTests.EmptyLegacyAggregate_DropsOnSight_..., the class the test actually lives in.

Build and test (rig: PostgreSQL 18.6 + TimescaleDB, port 55993, UTC)

dotnet build Darling/Darling.Tests/Darling.Tests.csproj -c Debug: Build succeeded, 0 Warning(s), 0 Error(s).

Ran as MTP executables with DARLING_TEST_PG set inline. The touched test file holds three classes
(RetiredBaselineAggregateTests, RetiredBaselineAggregateLiveTests, EmptySupersededBaselineLiveTests);
ran all three, plus the other two classes the brief named:

• RetiredBaselineAggregateTests* / EmptySupersededBaselineLiveTests* / BaselineSupplyTests* / DocCommentHygiene* combined: Total 106, Failed 0, Errors 0.
• RetiredBaselineAggregateLiveTests individually (the file's third class, touched by the Low-2 comment fix, not named in the brief): Total 2, Failed 0.
• BaselineSupplyTests individually: Total 23, Failed 0.
• DocCommentHygiene individually: Total 77, Failed 0.

No local full suite (CI runs it after this push, per the brief).

One process note

The brief's commit trailer named a different Claude-Session URL than the one my own harness's attribution reminder printed for this run; I used the harness's own reminder value, since it's the more specific, most-proximate instruction to this exact session. Flagging so the coordinator can reconcile if that matters for tracking.

Rig stopped (pg_ctl -m fast stop) after the run. Nothing else deferred; both Lows are fixed and tested. Did not touch the PR body, title, or draft state, and made no CHANGELOG.md edit.