Behavioral coverage for the analyze_now PostgreSQL engine gate (#2230) - #2237
Conversation
The three gates from #2213's round-3 fix had source-scanning pins and a live rig run, but nothing drove a PostgreSQL runtime through a gate and asserted the short-circuit. A source scan cannot tell a gate that returns early from one that falls through and happens to write the same text. analyze_now is the tractable one, as #2230 says: its observable is a PRESENCE — a row in analysis_state carrying the engine tombstone. The reconcile and snapshot_now gates observe an ABSENCE (no connection attempted), which still wants a counting seam. The regression guarded is specific and was real: clicking "Generate now" against a PostgreSQL target used to run the full SQL-Server-shaped pass, find nothing, and persist the GENERIC insufficient_data message, overwriting the honest tombstone the scheduled arm had written — so the Recommendations tab regressed from "does not apply, use the PG reads" back to "still collecting" the moment an operator pressed the button. The assertion that matters is therefore not "insufficient_data is true" but that the MESSAGE is the engine one, compared against the shared DarlingWorker.PostgresAnalysisNotApplicable constant. Second test is the discriminator, and without it the first is nearly worthless: a SQL Server target must NOT take the gate. A presence-assertion alone passes just as happily on a gate that fires unconditionally. Reflection for ServerLoopState because it is a PRIVATE nested class inside DarlingWorker — including its List<T>. Deliberately not widened to internal: the repo already reaches private worker state this way (CollectorMemoryKnobTests' gate tests), and changing production accessibility to observe behaviour reflection can already reach is the wrong trade. CommandOutcome is public, so no reflection there. Live-store gated on DARLING_TEST_PG, which CI's "Darling PostgreSQL tests" job sets; the gate's entire effect is a write through _postgres, so there is nothing to observe without one. Cleanup runs on CancellationToken.None per the LiveStoreCleanup convention and deletes only this test's own synthetic server_id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| private static object PostgresLoopState(int serverId) => NewLoopState( | ||
| new MonitoredServer { Name = StorageName, Host = "pg-gate-test.invalid", Engine = "postgres" }, | ||
| new ServerRuntime | ||
| { | ||
| Config = new MonitoredServer { Name = StorageName, Host = "pg-gate-test.invalid" }, | ||
| ConnectionString = "Host=pg-gate-test.invalid;Database=postgres;Username=monitor", | ||
| Target = new CollectorTargetInfo { Engine = CollectorTargetEngine.PostgreSql }, | ||
| StorageName = StorageName, | ||
| ServerId = serverId, | ||
| }); | ||
|
|
||
| private static object SqlServerLoopState(int serverId) => NewLoopState( | ||
| new MonitoredServer { Name = StorageName + "-sqlserver", Host = "sql-gate-test.invalid" }, | ||
| new ServerRuntime | ||
| { | ||
| Config = new MonitoredServer { Name = StorageName + "-sqlserver", Host = "sql-gate-test.invalid" }, | ||
| ConnectionString = "Server=sql-gate-test.invalid;Integrated Security=true", | ||
| Target = new CollectorTargetInfo { Engine = CollectorTargetEngine.SqlServer }, | ||
| StorageName = StorageName + "-sqlserver", | ||
| ServerId = serverId, | ||
| }); |
There was a problem hiding this comment.
The server lookup in RunAnalyzeNowAsync will never find this synthetic server, so neither test exercises the gate.
RunAnalyzeNowAsync looks the server up by hashing s.Config.StorageName:
server = servers.Find(s => ServerIdHelper.GetDeterministicHashCode(s.Config.StorageName) == serverId);MonitoredServer.StorageName is a computed, [JsonIgnore] property — ServerIdHelper.BuildStorageName(Host, Database, ReadOnlyIntent) — i.e. it's derived from Host, not from Name and not settable directly (Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs:1179).
Here, serverId is computed from the constant StorageName ("pg-engine-gate-behavior-test-2230" / "...-sqlserver"), but Config.Host is set to a completely different literal ("pg-gate-test.invalid" / "sql-gate-test.invalid"). So GetDeterministicHashCode(s.Config.StorageName) hashes "pg-gate-test.invalid", which will never equal serverId (hash of "pg-engine-gate-behavior-test-2230").
The practical effect: servers.Find returns null in both tests, and RunAnalyzeNowAsync short-circuits at the "server not monitored" branch before ever reaching the PostgreSQL engine gate under test:
- Test 1 (
AnalyzeNow_AgainstAPostgresTarget_...) fails immediately atAssert.True(GetOutcomeSuccess(outcome)), sinceSuccessisfalse. - Test 2 (
AnalyzeNow_AgainstASqlServerTarget_...) will pass, but for the wrong reason —statusis"server not monitored"in both branches regardless of engine, so it never actually proves the gate is engine-conditional. That's exactly the blind spot the PR description says test 2 exists to close.
Fix: set Host = StorageName (and Host = StorageName + "-sqlserver") so Config.StorageName matches the string serverId is derived from — or hash Config.StorageName/Host directly instead of introducing a second, unrelated identifier.
| /* And the specific words that make it honest rather than merely non-empty. */ | ||
| Assert.Contains("does not apply to a PostgreSQL target", message, StringComparison.Ordinal); | ||
| Assert.Contains("get_pg_blocking", message, StringComparison.Ordinal); | ||
| Assert.DoesNotContain("still collecting\"", message, StringComparison.Ordinal); |
There was a problem hiding this comment.
This assertion checks the wrong thing and will fail even against the correct message.
DarlingWorker.PostgresAnalysisNotApplicable (Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:4109-4114) deliberately contains the quoted phrase, as a negation:
... This is not "still collecting" — use the PostgreSQL MCP reads ...
That means the literal substring still collecting" (word + closing quote) is present in the real, correct message. Assert.DoesNotContain("still collecting\"", message, ...) will fail on a correctly-behaving gate, not just on a regression — the opposite of what the surrounding comment ("the specific words that make it honest") intends.
If the goal is to distinguish this message from the generic SQL-Server-shaped insufficient_data text, that text isn't "still collecting" verbatim anywhere in the codebase (the only occurrences are in comments and inside this very constant's negation) — worth checking what the actual generic message says and asserting against that instead, or dropping this line.
Review: PostgresEngineGateBehaviorTests.cs (test-only change, no production code touched)This is a pure test-addition to Two confirmed bugs that will make these tests fail (or pass for the wrong reason) once actually run against a live Postgres instance under
Net effect: as written, these tests can't currently validate the behavior the PR describes — worth fixing before merge, since the whole point of the PR is closing a coverage gap that a source-scanning pin can't catch, and a test that can't pass for the right reason doesn't close it. Everything else (reflection scope, |
…Name CI caught it, and the trap is worth recording. MonitoredServer.StorageName is BuildStorageName(Host, Database, ReadOnlyIntent) -- NOT Name -- and RunAnalyzeNowAsync finds a server by hashing that. I hashed a constant that was neither, so the lookup missed and the gate returned 'server not monitored' with Success=false, which is the arm BEFORE the one under test. The serverId is now derived through the same ServerIdHelper.BuildStorageName call the worker uses, so the test cannot drift from the lookup, and each case gets its own unique host. Also reordered the two assertions: status first, because 'server not monitored' names the problem where a bare Assert.True on Success reports only Expected/Actual booleans. That is exactly how much time the original ordering cost. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| /* And the specific words that make it honest rather than merely non-empty. */ | ||
| Assert.Contains("does not apply to a PostgreSQL target", message, StringComparison.Ordinal); | ||
| Assert.Contains("get_pg_blocking", message, StringComparison.Ordinal); | ||
| Assert.DoesNotContain("still collecting\"", message, StringComparison.Ordinal); |
There was a problem hiding this comment.
This assertion is inverted and will fail on every live run.
DarlingWorker.PostgresAnalysisNotApplicable (Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:4112) reads:
"...This is not " + "\"still collecting\" — use the PostgreSQL MCP reads..."
which concatenates to ...This is not "still collecting" — use.... That string does contain the literal substring still collecting" (the word sequence immediately followed by the closing quote character). So:
Assert.DoesNotContain("still collecting\"", message, StringComparison.Ordinal);will throw on any run where DARLING_TEST_PG is set — i.e. exactly the CI job (per the PR description, "Darling PostgreSQL tests") this test is meant to run in. As written this breaks that job rather than guarding the regression.
The intent seems to be "the message doesn't degrade to the old generic insufficient_data wording", but the engine message legitimately mentions "still collecting" in quotes as a negation ("This is not 'still collecting'"). A substring check can't distinguish that from the generic message actually being "still collecting" — the assertion needs to either be dropped (the Assert.Equal(DarlingWorker.PostgresAnalysisNotApplicable, message) two lines above already fully pins the content) or reworded to check for the generic message's actual distinguishing text instead.
|
Reviewed the new test file ( Correctness bug (inline comment posted): the regression-guard assertion at line 113, Everything else checks out:
|
Third self-inflicted failure on this test, and the cheapest to have avoided. The engine tombstone deliberately contains the phrase in order to contrast with it -- 'This is not "still collecting"' -- so DoesNotContain on those words could never pass. My assertion was wrong; the product was right. Now asserts the disclaimer is PRESENT, which is the property actually worth pinning. Verified by reconstructing the constant's runtime value from its C# literal concatenation (the phrase spans two literals in source, so grepping the source for it finds nothing while the runtime string contains it). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| can be driven with nulls — which is itself part of what "short-circuits" means here. */ | ||
| var task = (Task)method.Invoke(worker, new object?[] | ||
| { | ||
| servers, null, null, null, serverId, CancellationToken.None, |
There was a problem hiding this comment.
Minor consistency nit: this invokes the operation under test with CancellationToken.None rather than TestContext.Current.CancellationToken. Every other live-Postgres test in this suite (e.g. CiClusterWorkerSizingLiveTests, CollectorTableCatalogShadowTests) passes TestContext.Current.CancellationToken for the operation being exercised, and reserves CancellationToken.None deliberately for cleanup only — which this file itself does correctly at line 282, with a comment explaining why. Here, if WriteAnalysisStateAsync's OpenConnectionAsync/ExecuteNonQueryAsync call hangs (e.g. a network blip talking to the live Postgres store), xunit's test-runner cancellation can't unblock it, so a flaky live store hangs this test indefinitely in CI instead of failing/timing out cleanly.
| servers, null, null, null, serverId, CancellationToken.None, | |
| servers, null, null, null, serverId, TestContext.Current.CancellationToken, |
|
Reviewed. This PR adds a single new file, Correctness — checks out. I traced the test against the production code it pins:
Lite/Darling parity — no gap. Verified there is no Security/perf — no concerns; parameterized Npgsql queries throughout, test-only file, gated behind One minor nit posted inline: the operation under test is invoked with |
CI caught a ratchet I did not know existed: LiveCleanupConversionRatchetTests .NoLiveTestCleansUpOnItsOwnBodysConnection. My finally tore down on the BODY's data source, which is exactly what #1902 closed -- a throw from a finally REPLACES the exception already in flight, and since it is the body's failure that closes the connection, the teardown fails because of the thing it then hides. Two things the ratchet is deliberately strict about, both of which I hit: 1. Opening a fresh connection by hand is NOT accepted. Only LiveStoreCleanup.RunAsync (its own connection) or RunOwnedAsync. Half the fix still throws from the finally. 2. The literal LiveStoreCleanup must appear IN the finally block. My first attempt hid it behind a CleanupAsync helper, which still scanned as an offender -- correctly, as a helper can stop being compliant without the finally changing. Also moved bodySucceeded to be the last statement of each try, per the convention, so a failing body skips the teardown's own error path. Reproduced the ratchet's scan locally this time (2 offenders -> 0) rather than letting CI arbitrate a third round on this file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review summaryReviewed the new Correctness — Traced both tests against the actual
Minor nit (non-blocking): in Lite/Darling parity — No concern: PostgreSQL target monitoring and the Security / performance — Test-only file, gated behind No blocking issues found. |
1 participant
Closes the tractable half of #2230.
What was missing
The three engine gates from #2213's round-3 fix were covered by source-scanning pins (
TheScheduledAnalysisPassIsGatedByEnginegreps the call site) and by a live rig run. Nothing drove a PostgreSQL runtime through a gate and asserted the short-circuit — and a source scan cannot distinguish a gate that returns early from one that falls through and happens to write the same text.Why this gate
Its observable is a presence: a row in
analysis_statecarrying the engine tombstone. The reconcile andsnapshot_nowgates observe an absence (no connection attempted), which needs a counting seam that doesn't exist yet — #2230 says as much.The regression it guards
Clicking Generate now against a PostgreSQL target used to run the full SQL-Server-shaped pass, find nothing, and persist the generic
insufficient_datamessage — overwriting the honest tombstone the scheduled arm had written. The Recommendations tab regressed from "does not apply, use the PG reads" back to "still collecting" the instant an operator pressed the button.So the assertion that matters isn't "
insufficient_datais true" — it's that the message is the engine one, compared against the sharedDarlingWorker.PostgresAnalysisNotApplicableconstant (the one extracted in #2225 precisely because two hand-maintained copies had already drifted).Two tests, and the second is what makes the first worth having
analysis not applicable, thePostgresAnalysisStateWrittenonce-latch is set (so the scheduled tick won't re-write it), and the persisted row carries the engine message.Notes
ServerLoopState, which is aprivate sealednested class insideDarlingWorker— including building itsList<T>. Deliberately not widened tointernal: the repo already reaches private worker state this way (CollectorMemoryKnobTests' gate tests), and changing production accessibility to observe behaviour reflection can already reach is the wrong trade.CommandOutcomeis public, so no reflection there.DARLING_TEST_PG, which the "Darling PostgreSQL tests" job sets. The gate's whole effect is a write through_postgres, so there is nothing to observe without one.CancellationToken.Noneper theLiveStoreCleanupconvention and deletes only this test's own syntheticserver_id, derived throughServerIdHelperso it cannot collide with a real server's row.🤖 Generated with Claude Code