Verify what live-test cleanup removed, and fail the run when it did not (#1873)

[erikdarlingdata]

Closes #1873.

Stacked on #1874. That PR is not merged yet and its LivePostgresStoreFixture is the anchor this work hangs on, so its branch is merged into this one. Until #1874 lands, the diff below shows both; once it does, this collapses to its own three commits. Its branch moved twice while this was in progress and both moves are merged in, including the compression-policy parking that touches TimescaleSupportTests alongside these edits.

The mechanism, reproduced rather than inferred

#1873 could name the mechanism but had no live instance. There is one now. Against PostgreSQL 18.4 + TimescaleDB 2.28.1, a DROP MATERIALIZED VIEW concurrent with a refresh_continuous_aggregate of the same aggregate fails and leaves the aggregate standing, every time it collides — seven collisions, seven survivals. It arrives as two different SQLSTATEs from the identical setup, depending on which phase of the refresh is in flight:

• 40P01 deadlock detected — the drop is chosen as the victim while deleting from continuous_aggs_materialization_invalidation_log, which the refresh wants a lock on.
• XX000 tuple concurrently deleted — the refresh's own catalog maintenance beats it to a row.

That second one decided the design. A SQLSTATE allow-list is the shape this obviously wants, and it is the wrong one: XX000 is internal_error, the class nobody would put on a retry list, yet it is what an ordinary refresh collision produces. So LiveCleanupBatch retries any failure and asks the catalog whether the object is actually gone. The postcondition is the only thing that has to be true, so nothing has to be right about which errors count — and the reverse case comes free: DROP OWNED BY has no IF EXISTS form and raises 42704 on every pre-test call in the security-split class, where a probe that finds no such role simply reports success.

Each aggregate's refresh policy comes off before its drop, which bounds the collision to the one run already executing rather than one the scheduler keeps relaunching (#1788); the retry outlasts that.

Where the alarm rings, and why not in the test

These cleanups live in finally blocks, and a throw from a finally replaces the body's in-flight exception — the masking #1794 exists to prevent. So the helper never throws. It records what it could not remove, with the owning test's name, and LivePostgresStoreFixture compares the store's relations against the set captured when the run began and fails the run at collection teardown, after the last test has reported.

Measured on xUnit v3 3.2.2 rather than assumed, because the whole fix rests on it: a throw from a collection fixture's DisposeAsync surfaces as [Test Collection Cleanup Failure (live-postgres)] and exits non-zero, while the tests themselves still pass. That is the correct attribution — the run is broken, not any one test — and it is why this beats a hard assert in a test's own teardown. One limit worth stating: at --verbosity normal, which is what CI uses, the console shows the banner and the collection name but not the message; the full residue list is in the trx and at detailed verbosity. Console.WriteLine from DisposeAsync was measured too and reaches nothing, so it is not an alternative.

Both halves accuse independently. An armed retention policy that could not be removed leaves no relation behind and will still drop another test's chunks on its own schedule.

The second bug, which the check found on its own

The first full run after the check landed came back with a real accusation, precisely attributed:

continuous aggregate collect.query_store_stats_interval_hourly survived 7 removal attempts
  — PostgresException (2BP01): cannot drop view query_store_stats_interval_daily
    because other objects depend on it
  statement: DROP MATERIALIZED VIEW IF EXISTS collect.query_store_stats_interval_hourly CASCADE
  test: TimescaleSupportTests.EndToEnd_RetentionPoliciesActuallyApply_AndAreCreatedPaused_AgainstDevPostgres

query_store_stats_interval_hourly → interval_daily → day_grain_daily (#1849, #1869) is three levels deep, and TimescaleDB refuses to cascade through a dependent aggregate that has dependents of its own. Retrying the root can never succeed, because what has to change is a different relation. The drop now sweeps the whole set per round, so the leaf goes in the round the root fails — to any depth, without the helper knowing the hierarchy.

This was the cause of a failure nobody had connected to it. RunComposedPanel_OldWindow_AgainstPlainPostgres_RunsCleanOnRaw asserts a 10-day window lands on RAW, which only holds while no rollup exists. On a reused database the stranded aggregate changed the tier routing and failed it — exactly the consequence #1873 predicts in the abstract. The same rig on the branch without this change failed by the third run; with it, six consecutive runs on one database are clean.

Audit

Every DB-touching swallow in the project, five in total. The three that target the shared store are converted:

Site	What it left behind
PayloadDimensionLiveTests.TryExecAsync	aggregates, retention policies — reported to a console line nobody reads on a passing test
TimescaleSupportTests.TryExecAsync	aggregates, retention policies, throwaway hypertables — empty catch, fully silent
DarlingSecuritySplitLiveTests.DropTestRolesAsync	three CLUSTER-wide roles, which outlive even a DROP DATABASE

Both TryExecAsync helpers are deleted rather than fixed, so the shape cannot come back by copy. The two remaining swallows are own-store (ScratchPostgres, RollupBackfillLiveTests) and drop their whole database.

TimescaleSupportTests was the sharpest of the three: unlike its sibling it calls EnsureContinuousAggregatesAsync directly, so its finally raced an armed, immediately-firing scheduler and reported success either way.

One catalog reading corrected

The refresh-policy probe keys on the aggregate's own schema and name, measured rather than assumed. timescaledb_information.jobs reports a refresh policy's hypertable_schema/hypertable_name as the user-facing view, not the materialization hypertable the job writes. Joining continuous_aggregates.materialization_hypertable_name to it — the reading the column names invite, and the one written first here — matches nothing, so the probe answers "gone" while the policy is still attached: a verification that always passes. All four probes were exercised against a live 2.28.1 store in both directions before being relied on.

The fixture also establishes collect.module_map

No migration creates it and the service does (DarlingWorker's maintenance sweep), so it belongs with CREATE EXTENSION rather than with the hypertables and aggregates #1874 deliberately leaves to the tests. Without it the first full run read product infrastructure as test residue: DarlingModuleMapTests creates it through the product and correctly deletes only its rows. CREATE TABLE IF NOT EXISTS, so that test's own call still returns true and still covers the path.

Test plan

PostgreSQL 18.4 + TimescaleDB 2.28.1 from the repo's pg-runtime.zip, throwaway cluster on port 5881.

• Two full gated-live runs on brand-new databases: 3886 passed, 0 failed, 9 skipped, exit code 0, zero collection-cleanup failures. Exit codes read from dotnet test itself, not through a pipe.
• Six consecutive runs against one reused database, all clean — the scenario the ticket is actually about. The branch without this change failed the compose test by its third run on the same rig.
• Both mutation checks on the residue alarm. A test that leaks a relation and passes: the run fails, naming collect.leaked_on_purpose_1873, while the leaking test still reports as passed. A ledger entry with nothing leaked: the run fails, naming the statement and the test. Restored and green.
• Mutation check on the hierarchy fix: with per-object retry restored, the three-level seam test fails with cagg_1873_raw_hourly as residue. It is three levels deep because the two-level version passed under mutation — plain CASCADE handles that one.
• Live collision, four forced rounds against a 20-million-row aggregate with a refresh executing: residue 0 and the aggregate gone every round, one round taking 1268 ms against 30-70 ms uncontended. The losing-drop-then-winning-retry sequence itself was proven in the psql harness (XX000 on attempt 1, success on attempt 2), which is the same algorithm.
• Lite suite 1743/0. Both suites -t:Rebuild, 0 warnings, 0 errors. Installer.Tests not run.

One infrastructure note, since it produced a scary intermediate number: a run reporting 489 failures was the PostgreSQL server dying (could not reserve shared memory region, the Windows fork issue) when a background task was killed, not a code regression. Re-run after restarting the cluster: 3886/0.

Filed, not fixed

• Sixteen live-test finally blocks still clean up on the body's connection and throw, so a failing test can report the cleanup's error instead of its own #1896 — sixteen live-test finally blocks (10 in TimescaleSupportTests, 6 in DarlingSecuritySplitLiveTests) still clean up on the body's connection and throw from the finally, so a failing test can report the cleanup's error instead of its own. That is the Two live-Postgres test classes fail 'Connection is not open' on a reused database (lifecycle, not parallelism) #1794 shape, not the Live tests can strand a continuous aggregate in the shared DARLING_TEST_PG store, and the best-effort DROP swallows the reason #1873 swallow, and LiveStoreCleanup already solves it at ~20 other call sites. The residue check here catches the consequence — anything they abandon is named at teardown — so what remains is the masking, a diagnosis cost rather than a correctness one. Sixteen tests' failure semantics wants its own PR.

Test-suite only — no shipped code changed.

[claude]

The retry backoff can throw out of a finally — the exact #1794 shape this class exists to prevent.

The class-level doc says "It never throws, deliberately" because finally-block cleanup must not replace an in-flight test failure. ExecuteAsync/SurvivesAsync are both wrapped in try/catch here, but the backoff delay is not:

if (attempt < _maxAttempts)
{
    await Task.Delay(BackoffFor(attempt), ct);   // <-- unguarded
}

If ct is cancelled while this delay is pending, Task.Delay throws TaskCanceledException, which propagates straight out of RemoveAsync/RemoveRetentionPolicyAsync/DropRolesAsync/RemoveRefreshPolicyAsync.

That's not a theoretical ct: TimescaleSupportTests.cs's finally block (around line 1156) and DarlingSecuritySplitLiveTests.DropTestRolesAsync both call into this batch with ct = TestContext.Current.CancellationToken directly, not through LiveStoreCleanup.RunAsync (which deliberately swaps in CancellationToken.None for exactly this reason — see its own doc comment). If a test in either of those classes times out or the run is aborted mid-cleanup, the live per-test token fires, the backoff throws from the finally, and it replaces the real test failure with a TaskCanceledException from cleanup — reproducing the masking bug #1873/#1794 were filed to kill, in the code written to kill it.

Same issue at line 221 in DropContinuousAggregatesAsync's round-level Task.Delay(BackoffFor(round), ct).

Suggest wrapping both delays in try/catch (swallow OperationCanceledException and fall through to Record), or passing CancellationToken.None to the delay specifically, matching the invariant the rest of the class already upholds.

[claude]

Review summary

Scope. Test-suite only (Darling.Tests), no shipped product code. This is Darling/Postgres-Timescale-specific infrastructure (continuous aggregates, retention policies, cluster-wide roles) with no analog in Lite.Tests — I checked and found no parity drift to flag; Lite has nothing comparable to touch.

What it does. Replaces best-effort, fully-silent TryExecAsync cleanup helpers in three live-Postgres test classes with LiveCleanupBatch, which retries a removal until a catalog probe confirms the object is actually gone (rather than trusting "no exception" as success), and reports anything it can't remove to a run-wide ledger that LivePostgresStoreFixture.DisposeAsync cross-checks against a relation-set diff at collection teardown — failing the run, not any individual test, so a losing race against refresh_continuous_aggregate/background jobs can no longer strand debris in the shared store silently. Design rationale (retry-on-any-failure + postcondition probe rather than a SQLSTATE allow-list, sweeping the whole aggregate set per round to handle multi-level hierarchies CASCADE can't, reporting from the collection fixture instead of the test's own finally to avoid replacing an in-flight exception) is well-reasoned and thoroughly justified in both the PR description and the code's own doc comments.

Correctness. One real defect, left as an inline comment on LiveCleanupBatch.cs: the backoff Task.Delay(BackoffFor(attempt), ct) calls in RemoveAsync (line 149) and DropContinuousAggregatesAsync (line 221) are not wrapped in try/catch, unlike every other operation in this class. If the caller's CancellationToken fires mid-delay, the delay throws and escapes the "never throws" helper — and two call sites (TimescaleSupportTests's finally block, DarlingSecuritySplitLiveTests.DropTestRolesAsync) pass TestContext.Current.CancellationToken straight into this batch from inside a finally, rather than routing through LiveStoreCleanup.RunAsync's CancellationToken.None swap. On a test timeout or aborted run, that reproduces the exact finally-replaces-the-real-failure masking (#1794) this PR was written to eliminate.

Everything else I checked came back clean:

• Multi-statement DROP OWNED BY; DROP ROLE IF EXISTS ... batching in DropTestRolesAsync — a mid-batch failure aborts the rest of that statement string (implicit-transaction semantics), but this is pre-existing behavior carried over unchanged from the swallow it replaces, and the postcondition probe correctly catches anything left behind on retry.
• DarlingModuleMap.EnsureTableAsync ordering in InitializeAsync (established before the baseline snapshot, so it doesn't read as test residue) — correct, and the reasoning in the doc comment matches what RelationsAsync's schema/relkind filter actually captures.
• xUnit collection semantics — live-postgres classes serialize against each other (no DisableParallelization needed, confirmed via LivePostgresCollection's own doc comment), so the concurrent-refresh collision this PR handles is against TimescaleDB's background job scheduler, not test parallelism.
• No SQL-injection-relevant input: all interpolated identifiers (view/table/relation names) are test-authored constants, not external input, consistent with the rest of the test suite's existing style.
• No production/shipped-code or Lite-side changes.

Overall: solid, well-tested change (the PR description documents live collision repro + mutation testing on the residue/hierarchy logic). The one finding above is worth a fix before merge since it undermines the specific invariant the PR is built around, but it's narrow and easy to patch.

[claude]

Review

Went through the diff in detail (LiveCleanupBatch, the LivePostgresStoreFixture residue check, and the three converted call sites). This is a well-scoped, test-suite-only change and the reasoning is sound:

• Retry-on-any-failure + postcondition probe is the right design given that XX000 (a generic internal_error) is one of the two real SQLSTATEs the collision produces — a SQLSTATE allow-list genuinely couldn't have covered it.
• Sweeping the whole aggregate set per round (DropContinuousAggregatesAsync) correctly handles arbitrary-depth hierarchies: traced through the 3-level test case (Hourly → Daily → DayGrain offered root-first) and the round-robin removal converges in 3 rounds as claimed, well inside the 7-attempt budget.
• Reporting residue at collection-fixture DisposeAsync instead of from the tests' own finally blocks correctly avoids the finally-replaces-in-flight-exception masking (Two live-Postgres test classes fail 'Connection is not open' on a reused database (lifecycle, not parallelism) #1794) — moving the alarm to a place that can't lie about a test's own result is the right call.
• DROP OWNED BY role statements remain atomic (simple-query multi-statement implicit transaction), so the "already gone → 42704 → probe says gone → success" path can't leave the batch in a partially-dropped state on retry.
• Traced RelationsAsync's baseline-diff logic and BuildResidueReport's four cases (clean / leaked-only / ledger-only / both) — all consistent with the seam tests in LiveStoreResidueReportTests.

Lite/Darling parity: not applicable here — Lite.Tests has no live-Postgres/Timescale test infrastructure at all (Lite targets SQL Server), so there's no counterpart that could have drifted out of sync.

Security: the SQL built via string interpolation in LiveCleanupBatch (view/table/role names) isn't parameterized, but every value passed in comes from compile-time constants or TimescaleSupport.RollupViews/RawTierCoverage — no external input reaches these strings, and it matches the pattern the TryExecAsync helpers it replaces already used. Not a regression.

Two very minor, non-blocking nits left as inline comments (backoff comment vs. actual math, and a fully-silent swallow with no diagnostic). Nothing here blocks merging as far as I can tell.

[claude]

Minor: the actual max backoff is ~7s, not ~9s — attempts 1-6 delay 200,400,800,1600,2000,2000ms (attempt 7 never delays since it's the last), summing to 7000ms. Not load-bearing, just a doc nit.

[claude]

This remove_continuous_aggregate_policy failure is fully silent (no Console.WriteLine, unlike the give-up path in Record). Since it's best-effort by design and the following DROP is what's actually verified, that's probably fine — but if this call ever fails for a real reason (permissions, not "already gone"), there's currently no diagnostic trail pointing at it specifically, only at the eventual drop failure it may cause. Worth a one-line Console.WriteLine if this ever needs debugging in practice; not blocking.