Carry the retention live tests' evidence into their failure messages (Fixes #2818)

[erikdarlingdata]

The defect

Both live retention tests passed logger: null to EnsureRetentionPoliciesAsync, so its per-relation catch — correct for the service, where one bad relation must not abort the other sixteen — discarded every warning message. When the nightly failed once with expected 17, got 0, all seventeen discarded messages were the diagnosis and the bare count was the only survivor. It was written off as a flake because it could not be anything else. Same family as #2801/#2816/#2826: a failure that destroys the one piece of information needed to act on it.

The fix

Test-side only. #2818 assumed no capturing ILogger existed in Darling.Tests; CapturingTestLogger has existed since #2128, added by #1564 for the purge E2Es after the identical null-logger shape. Both tests now create one, pass it to EnsureContinuousAggregatesAsync and every EnsureRetentionPoliciesAsync call (a swallowed CAGG failure is a plausible upstream cause of the same all-relations-throw symptom), and fold Joined into each count assertion — the four Assert.Equal count checks become Assert.True with evidence-carrying messages, the DarlingRetentionTests house pattern. The shared logger's empty-case placeholder generalizes from (no purge log lines) to (no log lines captured); nothing asserts on that literal.

Proven both ways against live TimescaleDB 2.29 (PG17), running the SHIPPED methods

Injected the all-relations-throw shape (migrated store, no hypertables/CAGGs):

• old shape: applied=0 — everything the assertion could say
• new shape: applied=0 plus one captured warning per relation, first line: Warning: Retention policy for query_stats (4 days) failed - … TS001: "query_stats" is not a hypertable or a continuous aggregate

Healthy path unchanged: 69/69 hypertables, 20/20 CAGGs, 17 applied, 17 idempotent, zero warnings captured.

No product behavior change, no migration rung, no schema bump. The next occurrence names its cause in the CI log on the first failure.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy

[claude]

Minor diagnostic-quality nit: retentionLog is a single CapturingTestLogger shared across the CAGG call and all three EnsureRetentionPoliciesAsync passes in this test, and CapturingTestLogger has no reset — Joined just keeps growing. Unlike the simpler first test, this fixture deliberately keeps query_stats HELD for the whole test (line 1335 comment), so every pass logs a LogWarning("... HELD PAUSED ...") for it, plus EnsureRetentionPoliciesAsync always logs an Information summary line even on full success (TimescaleSupport.cs:2286).

By the time settled17's assertion could fire, Joined would contain the accumulated benign HELD/summary lines from created17 and converged as well — the real evidence for whichever pass actually failed gets buried in expected noise from the earlier passes, which cuts against the PR's own goal of a message that "names the actual Postgres error" without a round of triage.

Since CapturingTestLogger has no clear/reset, consider instantiating a fresh one before each EnsureRetentionPoliciesAsync call (as the CAGG-failure-can-explain-it comment in the first test already implies for that call+the immediately following one), so each assertion's Joined reflects only its own pass.

[claude]

Reviewed. This is a well-scoped, test-only fix (Darling.Tests + CHANGELOG) — no product code, no migration, no Lite counterpart needed since TimescaleDB retention/CAGGs are Darling-only (Lite has no equivalent live fixture). Consistent with the established Assert.True(x == N, "...; {log.Joined}") house pattern used elsewhere in the suite (e.g. AsOfWindowAnchorTests, ConfigSeedStatementArityTests, DarlingCustomViewsTests), and CapturingTestLogger is reused rather than reinvented, matching #1564's precedent.

One diagnostic-quality nit left as an inline comment on TimescaleSupportTests.cs: in EnsureRetentionPolicies_ConvergesAnOldHorizon_PreservingScheduledStateAndNextStart_AgainstDevPostgres, the single retentionLog is shared across three sequential EnsureRetentionPoliciesAsync calls plus the CAGG call. Because that test's fixture deliberately keeps one relation (query_stats) HELD for its whole run, every pass logs an expected LogWarning for it (plus EnsureRetentionPoliciesAsync always logs an Information summary on success), so Joined accumulates several passes' worth of benign noise by the time the third assertion could fire — diluting the very evidence-in-the-failure-message goal this PR is going for. Not a correctness bug (the count assertions are unaffected), just a suggestion to give each pass its own logger instance so a failure's message stays isolated to that pass.

No security, performance, or Lite/Darling parity concerns found.

[erikdarlingdata]

Good catch — fixed in 3d1dcd1. Every EnsureRetentionPoliciesAsync/EnsureContinuousAggregatesAsync call now gets its own fresh CapturingTestLogger rather than one shared across all passes in a test. Verified against the exact HELD-fixture shape you described (query_stats held for the whole test, so every pass would otherwise log a benign warning): pass-to-pass Joined line count now stays flat (2-3 lines) instead of accumulating across passes.

[claude]

Reviewed. This is a tightly-scoped, test-only change to Darling.Tests — no issues found.

• Verified the premise: both EnsureRetentionPoliciesAsync and EnsureContinuousAggregatesAsync log an Information summary line even on full success (TimescaleSupport.cs:1670, :2286), which confirms the second commit's "fresh CapturingTestLogger per pass" fix is load-bearing, not cosmetic — a shared logger really would bury a later pass's failure evidence under earlier passes' benign summary/HELD-PAUSED noise.
• CapturingTestLogger is internal sealed in the same Darling.Tests namespace, so no new using was needed, and it already existed (since Job History tab speaks display names #2128) as claimed.
• The Assert.Equal(...) → Assert.True(x == N, message) conversions are semantically identical, just with an evidence-carrying message.
• No Lite/Darling parity concern — TimescaleDB retention policies have no Lite equivalent (Lite has no TimescaleDB/CAGG concept), so this doesn't need a Lite-side counterpart.
• No security surface (test code, no new external input), no perf impact.

One non-blocking observation, not a defect: several other live test files (RollupBackfillLiveTests.cs, QueryStoreCorrectedRollupLiveTests.cs, PayloadDimensionLiveTests.cs, etc.) still pass logger: null to these same two methods. None of them assert on the multi-relation applied count the way the two fixed tests do (they check a single relation's armed/scheduled flag instead), so they don't reproduce the exact "expected 17, got 0" untriageable-flake shape this PR targets — but if a similar all-relations-throw failure ever hits one of those, the same diagnosability gap exists there. Given the PR explicitly frames this as one of a series (#2801/#2816/#2826) of narrowly-scoped fixes, leaving those out of scope here seems intentional rather than missed.

[claude]

Reviewed. This is a test-only change in Darling.Tests (no production code, no T-SQL, no Lite counterpart — TimescaleDB retention is Darling/Postgres-only, confirmed no EnsureRetentionPoliciesAsync/CapturingTestLogger references exist under Lite/).

Checked:

• Correctness: each EnsureRetentionPoliciesAsync call that follows another in the same test now gets its own fresh CapturingTestLogger, consistent with the PR's own fixup commit (3d1dcd1) noting the logger has no reset and EnsureRetentionPoliciesAsync always logs an Information summary even on success — reusing one logger across passes would've buried a later failure under earlier passes' accumulated noise. The one exception (first EnsureContinuousAggregatesAsync + first EnsureRetentionPoliciesAsync sharing retentionLog/createLog) is deliberate per the comments, since a swallowed CAGG failure is a plausible upstream cause of the same symptom.
• Assert.Equal → Assert.True conversions: all read correctly, each folding Joined into the failure message so a future "expected 17, got 0" names the actual per-relation Postgres error instead of just the bare count.
• CapturingTestLogger.Joined placeholder text: generalized from "(no purge log lines)" to "(no log lines captured)" — grepped the test suite, nothing asserts on the old literal.
• CHANGELOG: new [#2818]/[#1564] reference links added correctly; entry accurately describes the change.

No correctness, parity, security, or performance concerns found. LGTM.

[claude]

Review

Test-only change (Darling.Tests + CapturingTestLogger + CHANGELOG), no shipped-code paths touched. Went through it against CONTRIBUTING.md conventions and the correctness/parity/security/perf checklist.

Correctness — Sound. The null → CapturingTestLogger swap and Assert.Equal → Assert.True(... , $"...; {log.Joined}") rewrites match the house pattern already established in DarlingRetentionTests.cs (purgeLog) and PayloadDimensionLiveTests.cs. Each EnsureRetentionPoliciesAsync pass gets its own fresh logger (the commit history shows this was already caught and fixed as a review finding earlier in this same PR — good self-correction, since CapturingTestLogger has no reset and would otherwise bury a later failing pass under an earlier pass's already-explained noise). Sharing one logger across EnsureContinuousAggregatesAsync + the first EnsureRetentionPoliciesAsync call in each test is deliberate and justified (a swallowed CAGG failure is a plausible upstream cause of the same all-relations-throw symptom) rather than an oversight.

Parity — No concern. TimescaleDB retention/CAGG policies are Darling-only (Lite uses DuckDB); there's no Lite counterpart to keep in sync here.

Security — Nothing in scope; test-only, no new external input, no new file/network/process surface.

Performance — N/A; gated live tests behind DARLING_TEST_PG, not part of the hot path.

Minor/non-blocking: the CHANGELOG's new [#2818]/[#1564] reference-link lines are inserted at the top of the link block rather than in the numeric-descending order the surrounding lines mostly follow. Existing file isn't strictly sorted either, so this isn't a real convention violation — just noting it in case the block is meant to stay ordered.

Note: I wasn't able to run dotnet build/dotnet test in this sandboxed review environment (blocked by tool permissions), so I verified the diff by manual read rather than compiling it — the PR description's own live-TimescaleDB verification is the stronger evidence here anyway.

No blocking issues found.