Cover the reconcile engine gate behaviorally — the seam I said it needed turned out to exist (#2230)

[erikdarlingdata]

#2230's second of three doors. Leaves the issue open at 2/3.

Correcting my own note

When I covered the analyze_now gate, I wrote in that test's doc:

The reconcile and snapshot_now gates observe an ABSENCE (no connection attempted), which needs a counting seam that does not exist yet.

Half wrong. DarlingXeSessions.ReconcileLongQueryCompletionsAsync carries its own engine precondition — "belt to the worker's braces", per its comment — and it's public static. So an ungated call throws where a gated one returns, which an assertion can see with no seam, no live store, and no network. The doc is corrected in place rather than left to mislead the next person who reads it.

The regression, which was measured

Ungated, the method built a SqlConnection from a PostgreSQL connection string:

ArgumentException: Keyword not supported: 'host'.

The caller's catch then skipped the latch assignment, and because LongQueryTraceApplied resets to null on every connect, it retried every sweep, forever — roughly 1,440 warnings/day/server (#2213 round 2).

I confirmed that exception locally against Microsoft.Data.SqlClient before writing the assertion, rather than trusting my memory of the message.

What's pinned

fact	asserts
PG target, enabled: true	returns, no throw
PG target, enabled: false	returns, no throw — the gate sits ahead of the enabled branch, so ungated the false arm would try to DROP a session over the same impossible connection
SQL Server engine, same connection string	throws Keyword not supported: 'host'

That last row is what makes the first two mean anything. runner is passed null on purpose in the PG arms: reaching it would mean the gate didn't fire. And the engine is the only difference between the gated and ungated cases — same host, same string — so the test can't pass because of some incidental change to connection handling.

Verification

Builds clean (0 warnings). The tests themselves can't run on macOS — both test projects are net10.0-windows and need Microsoft.WindowsDesktop.App, which doesn't exist for this platform — so they're CI-verified. What I did verify locally is the pair of facts the assertions rest on: the gate is the method's first statement (read from source), and SqlConnection rejects that string (run locally, output above).

Still open

snapshot_now remains uncovered. It's the one that genuinely does observe an absence with no public precondition of its own, so it needs either a counting seam or a different observable. #2230 stays open and says so.

[claude]

Nit: missing blank line between PostgresRuntime() and SetField — every other member in this file is separated by a blank line (see the rest of the class). Minor, but worth a quick fix for consistency:

Suggested change

	};
	private static void SetField(DarlingWorker worker, string name, object value) =>
	};
	private static void SetField(DarlingWorker worker, string name, object value) =>

[erikdarlingdata]

Fixed in 3b467aca. Cause was mechanical: the block I spliced in ended with a single newline, so the helper's closing brace landed directly against SetField — every other member in the file is separated, as you say.

Also worth recording your async observation, because it's load-bearing for the test and not obvious: new SqlConnection(...) runs synchronously before any await, but since the method is declared async the ArgumentException is captured into the returned Task rather than thrown at the call site. That's exactly why Assert.ThrowsAsync is the right assertion here and a synchronous Assert.Throws would have failed to observe it.

[claude]

Review summary

This PR is test-only (Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs) — no production code, no T-SQL, and no Lite counterpart exists for DarlingXeSessions.ReconcileLongQueryCompletionsAsync (it's a Darling-only worker concern), so there's no Lite/Darling parity concern here.

Verified correctness of the two new tests:

• DarlingXeSessions.ReconcileLongQueryCompletionsAsync (Darling/PerformanceMonitor.Darling.Service/DarlingXeSessions.cs:543-572) does gate on server.Target.Engine != CollectorTargetEngine.SqlServer as its very first statement, before ever touching runner or the enabled branch — confirmed the "returns for both enabled values" claim.
• For the SQL-Server-engine + Postgres-shaped connection-string arm, new SqlConnection(...) is constructed synchronously inside an async Task method before any await; since the method is declared async, that exception is captured into the returned Task rather than thrown to the caller synchronously, so Assert.ThrowsAsync<ArgumentException> correctly observes it. Good non-vacuousness proof (same host/connection string, only the engine flips).
• All required ServerRuntime/MonitoredServer/CollectorTargetInfo properties are populated in the new PostgresRuntime() helper, so this compiles against the required members on ServerRuntime.
• No SQL injection or secrets exposure — the connection strings are synthetic, .invalid hostnames, and no live network/DB call happens on the paths these two new tests exercise.
• The doc-comment update to the class-level summary (replacing the now-incorrect claim that the reconcile gate "needs a counting seam") is accurate and matches the corrected understanding in the PR description.

One minor style nit left as an inline comment: a missing blank line between two methods, inconsistent with the rest of the file's spacing convention. Nothing else stood out — this is a solid, well-reasoned addition to the #2230 gate coverage.

[claude]

Reviewed. This is a test-only addition to Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs (no production code touched), so Lite/Darling parity doesn't apply here in the usual sense — I checked whether Lite's RemoteCollectorService.LongQueryCompletions.cs has the same failure class (building a SqlConnection from an engine-ambiguous connection string), and it doesn't: Lite branches on _serverManager.GetConnectionStatus(server.Id).SqlEngineEdition, which is already resolved before any XE-session code runs, so it isn't exposed to the #2213-style bug this test pins. No parity gap to flag.

Verified the two new tests against the actual implementation in DarlingXeSessions.cs:

• The engine gate (server.Target.Engine != CollectorTargetEngine.SqlServer) at line 549 returns before the Azure/SqlConnection branches, and IsAzureSqlDb defaults to false on a bare CollectorTargetInfo, so PostgresRuntime() correctly avoids the Azure arm in both new tests.
• runner is never touched by either the gated-return path or the on-prem SqlConnection construction, so passing runner: null! is safe and doesn't mask a different failure with an NRE.
• The gate sits ahead of the enabled branch, so [InlineData(true)]/[InlineData(false)] both correctly exercise the same short-circuit — matches the doc comment's claim.
• The SQL-Server-engine arm's connection string (Host=...;Database=postgres;Username=monitor) is genuinely Postgres-shaped, so the thrown ArgumentException traces to the real historical failure rather than an unrelated one — this is the non-vacuity check for the first test, and it does its job.

One very minor, non-blocking observation: ReconcileLongQueryCompletions_SameStringButSqlServerEngine_ThrowsTheFieldFailure pins substrings of Microsoft.Data.SqlClient's exception message ("Keyword not supported", "host"). That's a third-party message, so a future SqlClient version bump could reword it and break this test for reasons unrelated to the gate. Given the PR explicitly wants to pin the exact historical incident text, and the author confirmed the message locally, I'd call this an accepted tradeoff rather than a defect — flagging only for visibility if the assertion ever needs loosening.

No correctness, security, or performance issues found. The doc-comment correction in the class summary (replacing the now-inaccurate "needs a counting seam that does not exist yet" claim) is accurate per the source read above.