Cover the snapshot_now engine gate behaviorally — the last of #2230's three

[erikdarlingdata]

Closes #2230.

The third dispatch loop, and the one that got neither engine gate in #2213's first round. #2237 covered analyze_now, #2245 covered the reconcile early-return; this is snapshot_now.

The regression is phantom SUCCESS, not a crash

Dispatched SQL Server collectors do not fail loudly against a PostgreSQL target — their own AppliesTo early-returns, yields zero rows, and RunOneAsync then writes SUCCESS to collection_log. One operator snapshot produced ~40 rows saying collection worked, on an engine where those collectors cannot mean anything. That reads as health, which is strictly worse than an error.

Neither of the other two tricks was needed

analyze_now asserts a presence in analysis_state; the reconcile gate exploits a public precondition that throws when ungated. snapshot_now needs neither: the loop reports collectorsRun in its outcome JSON, and every run it makes writes itself to collection_log. The gate's effect is a count and a set of rows.

One collector, two engines

Schedule overrides disable everything except wait_stats — SQL-Server-only, dispatched through the same loop — so the two arms differ in the engine and nothing else: same collector, same overrides, same store. One dispatch decision instead of 49, which also makes the SQL Server arm cheap enough to be the non-vacuity proof rather than a second slow test. The overrides go through the shipped StoreConfigProvider.ResolveSchedule, which the loop consults, rather than reaching past it.

arm	collectorsRun	collection_log
PostgreSQL target	0	empty
SQL Server target, same overrides	1	wait_stats

Why absence rather than "not SUCCESS": a gate that dispatched and then failed would also avoid SUCCESS, while having connected to a PostgreSQL host as SQL Server. The row must not exist at all.

Why the SQL Server arm is load-bearing: without it, the gate assertion passes just as well against a snapshot that stopped dispatching anything for some unrelated reason — "ran zero collectors" is also what a broken loop reports.

Notes

• Real DarlingCollectorRunner, not a stand-in: a fake would have to reimplement the dispatch the test exists to observe, and the collection_log write that carries the assertion happens inside the real path.
• Unresolvable hosts (.invalid) with a one-second connect timeout, so the dispatched collector fails immediately. The status is not asserted — whether the attempt lands ERROR or a zero-row SUCCESS depends on how far it gets before the connection dies, and the fact under test is that it was dispatched at all.
• Its own synthetic hosts, separate from the analyze_now cases, because it asserts on EMPTINESS — sharing a server_id would make that depend on test ordering.
• Cleanup through LiveStoreCleanup on its own connection, per 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.

Live-store gated on DARLING_TEST_PG, which the "Darling PostgreSQL tests" job sets. Test-only — no production code touched. Builds clean (0 warnings) with -p:EnableWindowsTargeting=true; I cannot run net10.0-windows xunit locally, so CI is the only place the pass is observable — watching it there rather than claiming it here.

🤖 Generated with Claude Code

[claude]

Reviewed. This is a test-only change (Darling.Tests), adding the third of the three snapshot_now engine-gate arms described in #2230/#2213. I traced both new tests against the actual production path (DarlingWorker.RunSnapshotAsync, CollectorCatalog.EngineMatches/AppliesTo, RunOneAsync, DarlingObservability.LogCollectionAsync) and the test's claims hold up:

• The gate check in RunSnapshotAsync (!CollectorCatalog.EngineMatches(...) || (Engine == PostgreSql && !AppliesTo(...))) does drop wait_stats (SQL-Server-only per WaitStatsCollector's default TargetEngine) before dispatch when the target is PostgreSQL, and lets it through for a SQL Server target — matching the collectorsRun == 0 / == 1 assertions.
• RunOneAsync writes a collection_log row (SUCCESS or ERROR, always something) for every dispatched collector regardless of outcome, so asserting on row presence/absence rather than status is the right call — and matches the file's own stated rationale.
• The ServerIdFor/MonitoredServer.StorageName/ServerLoopState reflection plumbing (NewLoopState, SetField, GetUninitializedObject) only touches fields RunSnapshotAsync/RunOneAsync actually read (_serversLock, _logger, _postgres, _scheduleOverrides), consistent with the existing CollectorMemoryKnobTests idiom already used elsewhere in this file.
• ScheduleOverride/StoreConfigProvider.ResolveSchedule argument order and per-server-override precedence match, so SingleEnabledCollectorOverrides genuinely isolates dispatch to one collector.
• Cleanup goes through LiveStoreCleanup on its own connection (per 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), and uses hosts (PgSnapshotHost/SqlSnapshotHost) distinct from the existing analyze_now test hosts, avoiding the ordering hazard the file's own comments call out.

Lite/Darling parity: not applicable here — Lite only ever monitors SQL Server targets (no CollectorTargetEngine.PostgreSql runtime path in Lite), so there's no Lite-side engine gate for this to have a counterpart of.

Security: no concerns — parameterized queries throughout ($1 placeholders), synthetic .invalid hostnames, connection string pulled from DARLING_TEST_PG env var (existing pattern).

One minor, non-blocking note left as an inline comment on the SQL Server arm's connection string (DNS-dependent .invalid host + Connect Timeout=1 vs. the 127.0.0.1;Port=1 pattern used elsewhere in the suite for the same "fail fast" need) — worth a quick sanity check for CI flakiness, not a correctness issue.

No other findings.

[erikdarlingdata]

Taken in c2ad1ca5 — you're right, and it is the house idiom: ViewerControlPlaneStage3bTests.cs:599 plus the DeadStore constants in three MCP test files all use Host=127.0.0.1;Port=1.

The thing worth naming is that the two hosts had been conflated. Identity must stay the synthetic .invalid name — it is what ServerIdFor hashes, what the servers.Find lookup matches, and what the collection_log rows key on, so it has to be something no real server can collide with. The connection should go somewhere that refuses immediately. Those are different requirements and they now have different values, with the reasoning in the doc comment.

Also switched the SQL Server arm off integrated auth to SQL auth: the failure under test is the connect, and integrated auth on a Linux runner fails for a platform reason instead — same red, different cause, and asserting on it would have been asserting on the wrong thing.

Re-running CI. For the record, the previous run did execute both tests rather than skipping them — SnapshotNow_AgainstASqlServerTarget_DispatchesTheSameCollector in 1 s and SnapshotNow_AgainstAPostgresTarget_DispatchesNoSqlServerCollector in 149 ms, which is the timing split the design predicts: the SQL arm pays for a connect attempt and the PostgreSQL arm dispatches nothing at all.

[claude]

Reviewed. This is a test-only change (Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs), adding the two snapshot_now engine-gate arms that #2237/#2245 left uncovered. No production code changes.

Correctness — verified against the current dispatch path, not just the PR's own claims:

• RunSnapshotAsync (DarlingWorker.cs:3340) does gate on CollectorCatalog.EngineMatches(name, runtime.Target) plus the PostgreSQL-only AppliesTo check, exactly as described, and reports collectorsRun in the outcome JSON — the test's premises hold.
• wait_stats really is SQL-Server-only: WaitStatsCollector.Name => "wait_stats" vs. the distinct PgWaitStatsCollector.Name => "pg_wait_stats", so EngineMatches("wait_stats", postgresTarget) is false and the PG arm's Assert.Equal(0, collectorsRun) is meaningful, not vacuous.
• ScheduleOverride's positional constructor (ServerId, CollectorName, FrequencyMinutes, RetentionDays, Enabled) in StoreConfigProvider.cs:891 matches SingleEnabledCollectorOverrides's call shape, and ResolveSchedule matches on o.ServerId == serverId for the per-server override — the "single dispatch decision" framing is accurate.
• The -1-on-missing-collectorsRun sentinel in InvokeSnapshotAsync means a "server not connected"/"not monitored" failure outcome would already fail the PG arm's Assert.Equal(0, ...), so success doesn't need a separate assertion there — the SQL-Server arm does assert it directly.
• collection_log (V2/V23 in PgMigrations.cs) has no FK to a servers row, so the synthetic, unregistered server_ids used here are safe, consistent with the existing analysis_state arms in the same file.
• The bare collection_log/analysis_state table names resolve through the database-level search_path = collect, config, public (PgSchemaGenerator.SearchPath), same as the pre-existing tests in this file — nothing new here.

Parity: Lite has no multi-engine dispatch (grep shows a single incidental CollectorTargetEngine hit, in schema-generation code, nothing behavioral) — the engine gate and this test are Darling-only by design, so there's no Lite counterpart to keep in sync.

Security: synthetic .invalid hosts for identity and 127.0.0.1-with-no-listener / bogus SQL auth (User ID=x;Password=x) for the actual connection attempt — no real secrets, matches the existing unreachable-endpoint idiom used elsewhere in the suite (ViewerControlPlaneStage3bTests, MCP DeadStore).

No bugs, parity drift, or security issues found. Nothing blocking.