Repository navigation
The other ~124 live-test finally blocks still clean up on the body's connection, so a failing test can still report its teardown's error #1902
Description
Activity
- added a commit that references this issue
on Jul 31, 2026 Batch one merged (#1908): 39 conversions across 24 classes, selected on blast radius - every teardown whose leaked rows change what OTHER tests see (config singletons, edge-trigger watermarks, server registrations), with the singleton config_alert_settings the sharpest case. The bidirectional ratchet lands at Ceiling = 87 and fails on both a new unconverted teardown AND unspent slack, so batches two and three only edit the constant. Both predicted gotchas recurred at scale (ten token-fetching helpers threaded, two half-right hand-rolled connections replaced); one principled RunOwnedAsync at the mute-rule store, which owns its own data source. Batches two and three follow.
- added a commit that references this issue
on Jul 31, 2026 Batch two merged (#1916): the entire Viewer read-path family, 68 conversions across 21 classes, Ceiling 87 -> 19. All 21 classes carried the token gotcha (25 helpers threaded). En route it found and fixed the FOURTH instance of the unparked-policy scheduler race (#1889's fix applied to StuckCompressionJobsSql_NeverRunJob, proven closed both ways in a separate commit) - the find #1888 predicted in writing when it raised the worker slots. Batch three: the remaining 19 Darling engine sites, including the NpgsqlDataSource teardown isolated deliberately.
- added a commit that references this issue
on Jul 31, 2026 Completed by #1920, the third and final batch: 19 remaining teardown sites converted, the conversion ratchet's Ceiling constant deleted, and the detector rewritten as a standing invariant - brace-matched now rather than a fixed 13-line window, which on the way out exposed that the old window could be fooled in both directions by doc comments that merely NAMED the helper (batches one and two counted right in total, wrong in composition on one site each; net effect zero, now moot at ceiling zero). The NpgsqlDataSource family needed a different justification than the rest - fresh-connection-per-call means it was never exposed to the session-reuse half of the defect - but it lacked the masking rule and used an already-cancelled token, so it goes through RunAsync off the connection string like everything else, gaining the explicit search_path the data source connections never set. Hand-opened connections deliberately do NOT count as compliant. The #1873 -> #1896 -> #1902 arc is closed: cleanup that verifies what it removed, fails loudly when it cannot, never masks the failure it was cleaning up after, and an invariant that stops the shape returning.
- added 6 commits that reference this issue
on Jul 31, 2026 - added a commit that references this issue
on Jul 31, 2026 10 remaining items
- added 6 commits that reference this issue
on Sep 5, 2026 - added 7 commits that reference this issue
on Sep 14, 2026
Found by the sweep done for #1896, and deliberately left out of that PR on size grounds rather than principle.
What #1896 fixed, and the shape it left standing
#1896 converted the sixteen
finallyblocks inTimescaleSupportTestsandDarlingSecuritySplitLiveTestsoff the body's connection and ontoLiveStoreCleanup, so a test that fails mid-body reports its own exception instead of whatever its teardown said afterwards.Counting the same shape across the whole
live-postgrescollection turned up 148finallyblocks that do not go throughLiveStoreCleanup. Classified:DELETEof the test's own sentinel/server rows on the body's connection#1776 own-storeexemption)The ~124 live in roughly 55 classes, typically one or two each:
await DeleteTestRowsAsync(connection),CleanServerRowsAsync(dataSource, ...),CleanupAsync(connection, ...).Why it is the same defect
A throw from a
finallyreplaces the exception already in flight, and the connection these run on is the one the body's failure may have closed — so the teardown fails because of the thing it then hides. #1896 demonstrated it end to end: with a body failure and a cleanup failure forced into one test, the pre-change shape reported the cleanup's42883and lost the body's exception entirely.Why it is genuinely lower priority
The blast radius is much smaller. These delete rows keyed to the test's own sentinel server id; they do not drop continuous aggregates, hypertables, retention policies or cluster-wide roles, so an abandoned one leaves a handful of rows rather than an object that changes compose's tier routing or flips the #1784 coverage gate. #1873's residue check does not see them either, precisely because they are rows and not relations — which cuts both ways: nothing is currently watching, but nothing is currently breaking.
What is lost is diagnosis. A failing test in one of these classes can still report the wrong error, which is the expensive-to-notice failure mode: the suite stays red and simply says the wrong reason.
The work
Mechanical, and the pattern is settled —
var bodySucceeded = false;, set it as the last statement of thetry, and wrap the old finally body inLiveStoreCleanup.RunAsync(connectionString, bodySucceeded, ...), taking the cleanup connection and its token. Two details #1896 hit that will recur:TestContext.Current.CancellationTokeninternally must take a token instead, or they re-signal themselves on the cancelled-run pathLiveStoreCleanupdeliberately passesNoneto survive;NpgsqlDataSourcerather than a connection, so the callback shape needs a look per class rather than a blind find-and-replace.Worth splitting across a few PRs by class rather than landing 124 conversions at once, and each batch wants the full gated-live suite rather than a filtered run — the failure semantics being changed are exactly what a filtered run does not exercise.
LiveStoreCleanupTests(added by #1896) already pins the rule itself, so this is application rather than design.