Repository navigation
Stop an unpreloaded TimescaleDB masking itself as 'Connection is not open' - #1930
Conversation
…open" CREATE EXTENSION IF NOT EXISTS timescaledb does not merely ERROR when the library is on disk but missing from shared_preload_libraries - it terminates the backend. TryEnableAsync catches that and returns false exactly as its contract says, so the caller reads "carry on in plain-PostgreSQL mode" while the connection it handed over is dead. The live-postgres fixture then ran three more calls on it and every test in the collection died with InvalidOperationException: Connection is not open at RelationsAsync, naming the cause in neither the message nor the stack - the #1794 masking signature in setup rather than teardown. The product was checked FIRST and is safe, verified not assumed: DarlingWorker opens a DEDICATED connection for the TimescaleDB block and gates every subsequent call on the returned flag, so a false return means nothing touches it before disposal. That is why this fix is test-side only. The enable now runs on its own short-lived connection, shaped after #1896's LiveStoreCleanup: the safest connection to risk is one nobody else holds. A second call site had the same shape and a wider blast radius - it discarded the result entirely and kept using the connection for the whole test - and is converted too. Because the product's safety is a property of how the block is written rather than something the compiler holds, both halves are now pinned: one test fails if a call moves out from under the availability flag, another fails if any test-side caller reuses a connection it handed to TryEnableAsync. TryEnableAsync gains the warning its contract was missing. Measured before and after against a deliberately unpreloaded PostgreSQL 18.4: before, the whole collection failed on the masked error; after, the fixture survives and the single failure is its own assertion naming TimescaleDB and the preload expectation, with the other fifteen passing. That good diagnostic already existed - it was simply unreachable behind the masking. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dconn # Conflicts: # CHANGELOG.md
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /* SET, never inherit — the same Npgsql pooling trap LiveStoreCleanup documents: a physical session | ||
| opened before the store's first migration keeps the pre-ALTER search_path for its whole life, and | ||
| the hypertable helpers downstream use unqualified names. */ | ||
| await using (var setPath = new NpgsqlCommand("SET search_path = " + PgSchemaGenerator.SearchPath, connection)) | ||
| { | ||
| await setPath.ExecuteNonQueryAsync(cancellationToken); | ||
| } | ||
|
|
||
| return await TimescaleSupport.TryEnableAsync(connection, null, cancellationToken); |
There was a problem hiding this comment.
Minor nit, not blocking: unlike LiveStoreCleanup, the SET search_path here doesn't appear load-bearing for what this method actually does. TryEnableAsync only runs CREATE EXTENSION IF NOT EXISTS timescaledb and then checks pg_extension (a system catalog, resolved independent of search_path) — it never touches collect/config unqualified names the way the hypertable helpers do. If that's right, the comment ("the hypertable helpers downstream use unqualified names") doesn't quite apply to this probe, since it never calls those helpers. Harmless either way, just flagging in case it was copied from LiveStoreCleanup without re-checking whether it's needed here.
Review summaryVerified this against the diff and the surrounding code ( Correctness — looks right.
Lite/Darling parity — not applicable here. All changed files are Darling-only ( Security. No user input involved; table/column names in the touched code come from compile-time constants, matching the existing Performance. Test-only changes plus a doc-comment addition to One minor, non-blocking nit left as an inline comment on Overall: solid, well-scoped test-infrastructure fix with good verification (build + full suite + live before/after against an unpreloaded PostgreSQL). No blocking issues found. |
Review flagged that the SET looked copied from LiveStoreCleanup without re-checking, since TryEnableAsync only runs CREATE EXTENSION and then reads pg_extension - a system catalog that search_path does not affect. The comment was indeed wrong: it justified the line with the unqualified-name trap, which this probe never reaches. But the line itself is load-bearing, for a reason neither of us had stated. timescaledb's control file is relocatable = false yet, as it says itself, "relocatable during installation" - so CREATE EXTENSION with no SCHEMA clause installs into the FIRST schema on the search path. Verified on PostgreSQL 18.4 + TimescaleDB 2.28.1: on a database with no default search_path, SET search_path = collect, public before CREATE lands the extension in collect, and the live store's own extension is in collect for exactly that reason. So it stays and the comment now says why. Removing it would make which schema the extension lands in depend on MigrateAsync having already set the database default on this database - an unstated ordering dependency, which is the shape #1862 fixed in this very fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| continue; | ||
| } | ||
|
|
||
| if (!line.Contains("Assert.True(", StringComparison.Ordinal)) |
There was a problem hiding this comment.
Minor/nit: this guard only recognizes the safe shape when Assert.True( appears on the same line as TimescaleSupport.TryEnableAsync(. A perfectly safe two-line variant would false-positive as an "offender":
var enabled = await TimescaleSupport.TryEnableAsync(connection, null, ct);
Assert.True(enabled, "...");Since the call line doesn't contain Assert.True(, this would flag it even though the result is checked before the connection is used again. Not a problem for any current call site (none use this shape), but a future contributor hitting this lint might be confused about why an apparently-safe pattern fails it. Worth a comment on the check itself, or loosening the scan to look a line or two ahead/behind, if you want the guard to be self-explanatory when it eventually fires on a new pattern.
ReviewWhat this does: Fixes #1922 — Correctness
Lite/Darling parity
Minor nit (left inline)
OverallWell-scoped and well-evidenced (measured before/after against a deliberately unpreloaded PostgreSQL, watched-red mutation testing on both new guards). No security, performance, or parity issues found. |
Review: the guard only recognized the safe shape when Assert.True appeared
on the SAME line as the call, so this - which is safe - would have been
flagged as an offender:
var enabled = await TimescaleSupport.TryEnableAsync(connection, null, ct);
Assert.True(enabled, "...");
No current call site uses it, but a guard that fires on a safe shape is
worse than no guard: the next person routes around it instead of reading it.
The check now also accepts a capture asserted within the next three lines,
matched against the CAPTURED VARIABLE rather than any nearby Assert.True, so
an unrelated assertion cannot launder an unchecked call. Verified all three
ways: inline assert accepted, two-line capture accepted, and a capture that
is never checked still flagged with its file:line. The failure message names
both accepted shapes so it explains itself when it fires.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReviewOverview. This closes #1922 by fixing a masking bug: on a PostgreSQL backend that has the TimescaleDB library on disk but not in Correctness. I traced this independently rather than taking the PR description at face value:
Parity. No Lite-side counterpart needed — Lite has no TimescaleDB/Postgres dependency at all (it's DuckDB-backed), confirmed via grep across Minor observations (non-blocking):
Overall: thorough, well-verified fix with good regression coverage (both product-side and test-side invariants pinned via source parsing rather than trying to reproduce an environment-dependent crash in CI). No correctness bugs found, no parity gaps, no security concerns. |
| for (var i = 0; i < lines.Length; i++) | ||
| { | ||
| var line = lines[i]; | ||
| if (!line.Contains("TimescaleSupport.TryEnableAsync(", StringComparison.Ordinal)) |
There was a problem hiding this comment.
Nitpick: this guard is a literal substring match on TimescaleSupport.TryEnableAsync( within a single line. A call site that splits the member access or arguments across multiple lines (e.g. TimescaleSupport\n .TryEnableAsync(...)) would silently evade this check instead of failing loudly, since IsCheckedOnTheSpot also operates strictly line-by-line. Given every existing call site in the repo keeps this call on one line, this is low risk today — just flagging the blind spot since the whole point of this test is to make future regressions impossible to miss.
| /* #1922: on its OWN connection. This site discarded the result entirely and then kept using the same | ||
| connection for the whole test, so an unpreloaded TimescaleDB left every statement below failing | ||
| with "Connection is not open" instead of the real cause. */ | ||
| await LiveTimescaleProbe.TryEnableAsync(connectionString!, TestContext.Current.CancellationToken); |
There was a problem hiding this comment.
The return value is still discarded here (as it was before the fix) — confirmed safe because TimescaleSupport.ConvertToHypertablesAsync on the next line is per-table failure-isolated (catches and logs rather than throwing) even if TimescaleDB didn't actually enable, so this doesn't reintroduce the masking bug. Just noting for the reviewer that the discard is intentional/pre-existing behavior being preserved, not something the PR needed to fix — the connection-safety issue is what's being fixed, not the discard itself.
Review: the scan was a literal substring match within a single line, so a call split as `TimescaleSupport\n .TryEnableAsync(...)` would have slipped past silently rather than failing loudly. No such site exists today, but a guard a line break can evade is not a guard - and this test's whole purpose is to make the regression impossible to miss. Detection now runs over the whole file with a whitespace-tolerant pattern, and the safety window is flattened before it is read, so a call OR an assertion wrapped across lines reads the same as the one-line form. The window still starts at the call's own line, so an assertion above it cannot be mistaken for a check of it, and the capture form is still tied to its own variable. Verified: the split-call form is now caught (LivePostgresStoreFixture.cs:121 when introduced deliberately), while both safe shapes and every existing call site still pass. Also confirmed the reviewer's second point rather than relaying it: the discarded result in DarlingDeltaSeederTests is safe, because ConvertToHypertablesAsync catches per table inside its foreach and logs a warning rather than throwing. Pre-existing and intentional; left alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review: Stop an unpreloaded TimescaleDB masking itself as 'Connection is not open'Overview. This is a test-infrastructure fix for #1922:
Correctness. I traced the product path ( Lite/Darling parity. No concern — TimescaleDB is Darling/Postgres-only; Lite has no counterpart surface to drift from. Security. No new input handling, secrets, or SQL-injection surface. All interpolated identifiers in Minor notes (non-blocking), left as inline comments:
Overall this is a well-scoped, well-verified fix — the before/after live repro against an unpreloaded PostgreSQL 18.4 is convincing evidence the actual masking bug is gone. Nice work. |
| /* #1922: on its OWN connection. This site discarded the result entirely and then kept using the same | ||
| connection for the whole test, so an unpreloaded TimescaleDB left every statement below failing | ||
| with "Connection is not open" instead of the real cause. */ | ||
| await LiveTimescaleProbe.TryEnableAsync(connectionString!, TestContext.Current.CancellationToken); |
There was a problem hiding this comment.
Minor inconsistency: this discards LiveTimescaleProbe.TryEnableAsync's result, while the LivePostgresStoreFixture site (line 121) now captures it into TimescaleAvailable. It's not unsafe here — the enable runs on its own connection, so connection below stays healthy regardless of outcome — and if TimescaleDB genuinely isn't enabled, ConvertToHypertablesAsync fails per-table into a swallowed warning (logger is null here) and the test's own Assert.SkipWhen(recentChunk == oldChunk, ...) catches the resulting unpartitioned-table case. So functionally this self-skips rather than masking. Still, capturing the bool and asserting/skipping on it explicitly (like the fixture does) would make the "TimescaleDB wasn't available" case diagnosable directly instead of via an inferred side effect two asserts later.
| assertion wrapped across lines read the same as the one-line form — the blind spot a purely | ||
| line-by-line scan leaves. The window starts at the call's line, so an assertion ABOVE it is not | ||
| mistaken for a check of it. */ | ||
| var window = string.Join(' ', lines.Skip(index).Take(4)); |
There was a problem hiding this comment.
Heads-up on a soundness gap in this heuristic (not blocking, given the PR already frames these as cheap tripwires rather than exhaustive guarantees): the window is "the call's line + the next 3", flattened, with no check that nothing between the capture and the Assert.True touches the connection. e.g.
var enabled = await TimescaleSupport.TryEnableAsync(connection, null, ct);
await SomeOtherCall(connection, ct); // runs on a possibly-dead connection, unchecked
Assert.True(enabled, "...");would pass IsCheckedOnTheSpot (the assert is within 4 lines and references the captured variable) even though the unsafe use happens before the check. Given the discipline this PR establishes (enable-then-immediately-assert), this is unlikely to occur in practice, but worth knowing the guard doesn't strictly prevent it.
… be next
Two more from review, both accepted.
The seeder site still discarded the result. Safe - the enable runs on its own
connection now - but without TimescaleDB the conversion fails per table into
a swallowed warning, the table stays plain, and the test lands on its own
"no per-day chunks to exclude" skip several asserts later. True, but arrived
at by inference, which is the exact thing this PR is about. It now captures
the bool and skips on it by name.
The guard's window also only required the check to appear within a few
lines, so this would have passed:
var enabled = await TimescaleSupport.TryEnableAsync(connection, null, ct);
await SomethingElse(connection, ct); // possibly-dead connection, unchecked
Assert.True(enabled, "...");
"Checked somewhere nearby" is not the property that matters - anything
between the capture and the check is itself running on a connection that may
be dead. The window is now split into statements and the check must be the
very NEXT one, which is the discipline every call site already follows.
Assert.SkipWhen(!x) counts alongside Assert.True(x): both stop before the
connection is used again, and a rig legitimately without TimescaleDB should
skip rather than fail.
Verified all four ways: inline accepted, capture-then-check accepted,
interleaved use rejected, split-across-lines call still caught. The failure
message now states the tightened rule rather than the old looser one.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReviewScoped this as a C#/test-infra change (no What it does: Fixes a masking bug in the Darling live-test rig — Verification performed:
No correctness, security, or performance issues found. The regression-guard tests ( Nice work tracing this down to the actual Postgres backend-termination behavior rather than papering over the downstream symptom. |
1 participant
Closes #1922.
The product path is safe, and that is why this is test-side only
Checked first, as instructed.
DarlingWorker.csis safe for two independent reasons:falsereturn — which is the only outcome when the backend died, since thecatchfires beforeDetectAsyncis reached — means nothing touches that connection again.It is the only product caller; the rest of the repo's
TryEnableAsynccalls are all inDarling.Tests.What was actually exposed
Two test call sites, not the one I filed:
LivePostgresStoreFixture.cs— takes the result, then runs three more calls on the same connection.DarlingDeltaSeederTests.cs— discards the result entirely and keeps using the connection for the whole test. Wider blast radius, same shape.Both now enable on a short-lived connection of their own via
LiveTimescaleProbe, deliberately shaped after #1896'sLiveStoreCleanup— the safest connection to risk is one nobody else is holding. Reopening the caller's would also work, but it makes correctness depend on every call site remembering, and a rule every call site must remember is one a new call site will not.Extension creation is committed DDL, so the caller's connection sees the extension immediately afterwards; it was opened by a backend that already had the library preloaded, since
shared_preload_librariesloads at postmaster start.Measured, not argued
Against a deliberately unpreloaded PostgreSQL 18.4 (same cluster,
shared_preload_librariescommented out, same database recreated):Before — the whole collection dies, cause named nowhere:
After — the fixture survives; 15 passed, 1 skipped, and the single failure is the fixture's own assertion:
Worth noting: that good diagnostic already existed. It was simply unreachable, because the fixture died three calls before anything could assert. The fix does not add a better message so much as let the one that was already written run.
Pinning both halves
The product's safety is a property of how that block is written, not something the compiler holds — move one call out from under the flag and the service acquires exactly the masking the tests just shed. So both directions are pinned in
TimescaleEnableConnectionSafetyTests:DarlingWorker_TouchesTheTimescaleConnectionOnlyBehindTheAvailabilityFlag— extracts the block, and fails if anyTimescaleSupport.*call other than the enable itself appears beforeif (_timescaleAvailable), or if the connection stops being dedicated.NoLiveTest_KeepsUsingAConnectionItHandedToTryEnableAsync— every test-side call must go through the probe or be asserted on the spot.Both are source-parsed, and that is the honest way to hold them: reproducing the crash needs a PostgreSQL misconfigured in one specific way, which no CI rig is and none should be made to be. What actually regresses is a call site written the unsafe way, and that is textual.
The ~15
Assert.True(await TryEnableAsync(...))sites are deliberately still allowed rather than swept. On a dead connection they fail immediately with their own message ("the rig is expected to have TimescaleDB"), which is a true and useful diagnosis rather than a masked one. What is banned is the shape that masks — taking the result and carrying on with the same connection.TryEnableAsyncalso gains the warning its contract was missing: afalsereturn may mean the connection is unusable, andDarlingWorker's gating is called out as load-bearing rather than incidental.Testing
dotnet build PerformanceMonitor.sln -c Debug -t:Rebuild— Build succeeded, 0 Warning(s), 0 Error(s).Run against live PostgreSQL 18.4 + TimescaleDB 2.28.1 (10 skipped, not 214 — the gated live suite genuinely ran). No Installer.Tests.
Watched-red, each mutation reddening only its own guard, with the message naming the exact offender:
TryEnableAsync(connection, …)NoLiveTest_KeepsUsingAConnection…— "…: LivePostgresStoreFixture.cs:121"ConvertToHypertablesAsyncout from under the flag inDarlingWorkerDarlingWorker_TouchesTheTimescaleConnection…— "…: ConvertToHypertablesAsync"Plus the live before/after above, which is the end-to-end version of the same thing.
Deviations
None to file. The issue's option 2 — hardening
TryEnableAsyncto reopen on its failure path — is deliberately not taken: the product proves safe, so it would be a behavior change to shipped code with no exposed caller to justify it. The contract is documented and the invariant pinned instead, which keeps "the product is safe" a fact rather than something that was true once.🤖 Generated with Claude Code