Establish the live-postgres store in a collection fixture so the gated-live suite stops depending on test order (#1862)

[erikdarlingdata]

Closes #1846. Closes #1850.

#1846 — a dash where there was never a number

The alert-history current_value / threshold_value columns are NOT NULL doubles, and a family of alerts has no measurement to put in them: their value is a role (PRIMARY), a connection state (DISCONNECTED), a suspend reason, a failure reason, or the literal resolved. Producers hand the history store a display STRING, the store's parser yields 0 for text carrying no digit, and both grids rendered that sentinel as 0.00 — a number the alert never made. The read side now renders an em dash.

Write side untouched. The column stays NOT NULL, 0 stays the stored sentinel. Rows already collected display correctly with no migration and no re-collection.

The list is 19 metrics, not the 9 the issue named

Enumerated from the fire sites rather than the report, as asked. Nine were known — the seven AG metrics plus Server Unreachable and Server Restored. The other ten are every resolution notice: DarlingSelfAlertEvaluator.BuildResolutionRecord hardcodes CurrentValueText: "resolved" and ThresholdValueText: "" with both numerics null, for BOTH Darling's own self-alert recoveries (Collection Resumed, Capture Restored, Agent Restarted, Store Disk Pressure Resolved, Compression Job Recovered) and the shared engine's resolution callback (CPU Resolved, Blocking Cleared, Blocking Wait Cleared, Deadlocks Cleared, Poison Waits Cleared, Long-Running Queries Cleared, tempdb Space Resolved, Volume Free Space Resolved, Long-Running Jobs Cleared). Those reach alert history only in Darling — Lite's resolution callback is toast-only — so Darling's grid carried most of the 0.00 rows.

Resolutions match through the existing shared AlertMetricClassifier.IsResolution rather than being listed by name, so a future recovery metric renders correctly without an edit. The predicate lives in PerformanceMonitor.Common beside it rather than being hand-mirrored into the two formatters, which is the drift that class was created to stop.

The 0-gate is load-bearing, not bookkeeping

AlertValueParser.ParseOrDefault scans to the first digit anywhere, not to a leading number. So several of these do not reliably store 0: AG Sync Fell Behind spells its lag seconds into its prose, and any of them can pick a digit out of an object name (SQL01, Sales2024). Those rows keep rendering whatever was parsed, and a producer that starts supplying a real numeric is shown rather than hidden.

Three Darling self-alerts are deliberately excluded for the same reason (Collection Stopped, Store Runtime Upgrade, Compression Job Stuck — filed as #1881), and Store Disk Pressure most of all: its parsed value is percent-free, where a genuine 0 means a full volume. A measurement that happens to be zero is not a missing value — Blocking Detected at 0 still reads 0, pinned separately in both suites.

#1850 — the narrower key was dropping a replica, not de-duplicating

Four analysis-layer dedups predate #1841 tier 1 and kept (database_name, query_id, plan_id, runtime_stats_interval_id, first_execution_time) with no replica_role. sys.query_store_runtime_stats is keyed by (plan_id, interval, execution_type, replica_group), and on a SQL Server 2022+ AG with Query Store for secondary replicas enabled the primary holds ONE shared Query Store carrying every replica's rows — so two rows differing only in replica_role are distinct legitimate work, and the rn = 1 filter was DISCARDING one. An under-count, worse than the double-count the dedup exists to fix: a double-count is visible in the number, a dropped row is silent. #1845 used exactly this reasoning for the read-side key.

Split, per the issue's option 3. replica_role is carried through the downstream grouping and the ranking, so a regression is measured WITHIN a replica (this replica's current plan against the best plan this replica has run, never a cross-replica comparison of two different workloads), and it is exposed in both drill-down row shapes. Adding it to the dedup alone would have made totals complete while blending primary and secondary workload into one indistinguishable number.

The joins use IS NOT DISTINCT FROM, never =

This is the whole risk of the change. replica_role is NULL on every standalone server, every non-AG server and everything below SQL Server 2022, and NULL = NULL is UNKNOWN — an equi-join would have matched nothing and silently disabled plan-regression detection for the overwhelming majority of installs. Darling's drill-down also moved off JOIN ... USING, which is an equi-join. Both suites carry a NULL-replica arm for exactly that case.

Fact-shape consequences

• Facts (PgFactCollector / DuckDbFactCollector): no shape change. Fact.Metadata is Dictionary<string, double> and cannot hold a replica dimension; the numbers simply become per-replica-correct. offender_count now counts two distinct regressions where it counted one.
• Drill-downs: replica_role added LAST in the row so existing reader ordinals are untouched. It flows to the MCP payload and to email/webhook/toast detail with no code change (AnalysisNotificationService.FlattenInto enumerates every property). The drill-down is documented ephemeral and never persisted, so nothing has a pinned schema to break.
• Force-plan remediation — the one consumer that could not take per-replica rows. FactRemediation.ExtractPlanRegressionTargets reads fixed keys, so two rows for one query would have rendered two sp_query_store_force_plan calls naming the SAME query with DIFFERENT plan ids: mutually exclusive instructions where whichever the operator runs last silently wins. Fixed mechanically — keep the worst per (database, query_id), which the regression_factor DESC order already provides. Which replica should drive the recommendation is a product question, filed as Force-plan recommendation is replica-blind now that plan regression splits by replica_role #1882.

Testing

Watched RED first, on both stores, with the same seed shape: two replicas running the same two plans over the same two intervals, the secondary always collected one second later so the old partition drops the PRIMARY specifically.

	old key	truth
offender_count	1	2
worst_regression_factor	3	12
drill-down rows	1	2

The factor is the point: dropping the primary did not merely lose a row, it reported the secondary's much milder regression as the server's worst. The remediation dedup was watched red separately (2 targets for one query).

• Lite: 1792 passed / 0 failed, real DuckDB.
• Darling: 3906 passed / 0 failed / 9 skipped (the SQL-Server-requiring live tests), against a real PostgreSQL 18.4 + TimescaleDB.
• Solution-wide -t:Rebuild: 0 warnings, 0 errors.
• The Darling live test was verified against a FRESH database, not just a rerun — the first version passed on rerun and failed on first run, which is the pooled-session search_path trap LiveStoreCleanup documents. It now SETs the path explicitly rather than inheriting it.

One test-side fix that is a real fix rather than an accommodation: the Darling FROM/JOIN drift guard scanned SQL comments, so English prose about a join ("an equi-join here would…") parsed as a relation named here, and it read the FROM inside IS NOT DISTINCT FROM as one too. It now strips line comments and that operator before scanning, so the assertion itself stays exact.

Deferred, filed before this PR

• Alert history stores a semantically-wrong number for metrics whose display text merely contains a digit #1881 — alert history stores a semantically-wrong number for metrics whose text merely contains a digit (PG major version as a "current value", two units under one metric name).
• Force-plan recommendation is replica-blind now that plan regression splits by replica_role #1882 — force-plan recommendation is replica-blind; needs the SQL Server 2022+ plan-forcing semantics confirmed against MS Learn before anyone picks a rule.

🤖 Generated with Claude Code

---

Follow-up: a fourth masked bug, found by this PR's own CI (#1889)

The first push turned the darling-pg leg red on TimescaleSupportTests.EndToEnd_CompressionRun_OneUncompressibleChunkDoesNotBlockTheRest_AgainstDevPostgres (Expected: 1 / Actual: 0). It is pre-existing on dev, not caused by anything here: the same test failed on run 30588924852 at commit 7e40c850, which is an ancestor of this branch's base, with Expected: 2 / Actual: 3.

Root cause. add_compression_policy creates its job SCHEDULED with no initial_start, and TimescaleDB launches it within a second or two (the #1788 behaviour). Three tests added the policy AFTER inserting eligible chunks, so that background run had chunks to compress and competed with the deterministic foreground run_job they are built around. The background session carries no lock_timeout — the default is wait-forever — so in the isolation test it queued behind the ACCESS EXCLUSIVE lock the test takes on the middle chunk and compressed it the instant the test rolled its blocker back, landing directly on the assertions. Both CI failures are the two arms of that one race: 3 compressed if it beat the first assertion, a torn 2-then-0 if the chunk flipped between the two reads.

Evidence. Replaying the test's exact two-session sequence in SQL:

ordering	counts after the blocker rolls back
policy added after the chunks (pre-fix)	3 compressed / 0 uncompressed, stable — dev's failure exactly
policy added and parked before any chunk (post-fix)	2 compressed / 1 uncompressed, stable across every sample

Separately: adding the policy to a hypertable holding three eligible chunks and then doing nothing at all compressed all three within six seconds, run_job never called, total_runs = 1.

Fix. The three tests that insert chunks now add the policy first, while there is nothing to compress, and park its job (scheduled => false, next_start => 'infinity') before any row is inserted. The ordering is the load-bearing half — parking a job that has already launched does not recall the run in flight. Same idiom the file already used for the #1760 sentinel probe, and the same lever PayloadDimensionLiveTests.EnsureAggregatesWithoutPoliciesAsync pulls against this behaviour on the aggregate side. run_job still executes a parked job, verified live.

Why it never reproduced locally, and why that matters. Whether the job launches depends on a free background-worker slot, and the test cluster runs max_worker_processes = 8 against TimescaleDB's default max_background_workers = 16, so launches routinely fail ("failed to start a background worker"). Twenty consecutive local runs passed for that reason alone. Raising max_worker_processes on the same rig made the background run fire every time — and all the verification below was then re-run under that raised setting, which is strictly more race-prone than CI. Filed as #1888, because the product's own DarlingManagedPostgres.BuildConfAppend sizes these correctly and CI diverges from it.

Re-verified after the fix, all with worker slots raised so the background job launches: the isolation test 10/10; TimescaleSupportTests solo on a fresh database; three more consecutive full gated-live runs on fresh databases at 3853 passed / 5 skipped / 0 failed.

[claude]

Review

Scope: test-infrastructure only (Darling.Tests + CHANGELOG.md), no shipped code. Confirmed Lite has no PostgreSQL/TimescaleDB dependency at all (Lite.Tests has zero real references), so there's no Lite/Darling parity drift to flag here.

Correctness — verified against the surrounding code, not just the PR description

• Root cause and fix are real. TimescaleSupport.TryEnableAsync/PgMigrations.MigrateAsync are both idempotent (checked CreateHypertableSql, AddCompressionPolicySql, etc. — if_not_exists throughout), so the new LivePostgresStoreFixture.InitializeAsync running once per collection ahead of ~60 classes that also call these themselves is safe and matches the documented "costs nothing, no-op on an already-established store" claim.
• Migrate-before-enable ordering is correct against PgMigrations.cs — V23's IF EXISTS (SELECT 1 FROM pg_extension …) guard does depend on this order, so reversing it would silently change what TimescaleSupportTests covers, as the PR claims.
• ConvertToHypertablesAsync moved into EnsureAggregatesWithoutPoliciesAsync (PayloadDimensionLiveTests.cs) is safe: the method is failure-isolated per table and uses if_not_exists, so the two callers that already convert before invoking the helper now do it twice — redundant but a cheap idempotent no-op DDL call in test code, not worth a change.
• DarlingAnomalyBaselineTests existence-vs-count fix checks out: EnsureBaselineFallbackViewsAsync's return value is documented as "how many the call created" (TimescaleSupport.cs), which does under-count on a store where a sibling's aggregate is already standing — asserting existence via to_regclass instead is the right fix, and the 9-tuple BaselineAggregates array confirms the count invariant is pinned elsewhere.
• The hygiene-rule regex fix is real, not just reasoned about. I greped every real [Collection("live-postgres")] usage plus every prose mention in the project: all real attribute applications open their line, and every prose/doc-comment mention (including the new fixture's own 19-lines-above-declaration mention that motivated this fix) does not. The new CarriesLiveCollectionAttribute line-anchored check is correctly scoped and doesn't introduce new false negatives.
• Reflection-based fixture exemption (LiveCollectionFixtureTypeName()) correctly ties the hygiene-test exemption to whatever type is actually wired via ICollectionFixture<> on the [CollectionDefinition("live-postgres")] — can't be spoofed by a comment, degrades to exempting nobody if unwired, matching the "mutation-checked" claim in the description.
• xUnit mechanics check out: IAsyncLifetime.InitializeAsync/DisposeAsync return ValueTask in xunit.v3 (3.2.2, per the csproj), matching LivePostgresStoreFixture's signatures; collection-fixture constructor injection for LivePostgresStoreFixtureTests is standard xUnit v3 behavior.

Minor, non-blocking observations

• LivePostgresStoreFixture.InitializeAsync uses CancellationToken.None with no fixture-level timeout — a hung migration/connection blocks the whole collection until the runner's own timeout kicks in. This is explicitly called out as a deliberate tradeoff in the doc comment, so not a defect, just noting it as the one place a stuck live rig would be diagnosed less directly than before.
• The redundant ConvertToHypertablesAsync call noted above (two of the three call sites now convert twice) is harmless in test code given the idempotency, so no action needed.

Overall

No correctness bugs found. The fix is well-targeted at the actual root cause (order-dependent, database-persistent side effects with no collection-level setup), the two "masked" bugs it also fixes are genuine and independently verifiable against the referenced code, and the guard tests (LivePostgresStoreFixtureTests, the hygiene-rule reflection change) are structurally sound rather than just asserting behavior that would pass by accident. Nothing here touches Lite or any T-SQL, so no parity or missing-index concerns apply.

[claude]

Nit: this one-line comment ("Runs the policy body NOW instead of waiting out its schedule. job_id is INTEGER (#1586).") describes RunPolicyAsync, not the newly-inserted AddCompressionPolicyParkedAsync it now sits directly above. Since AddCompressionPolicyParkedAsync's doc comment was added between this line and RunPolicyAsync's declaration, the old comment is now floating in the wrong place and reads as documentation for the wrong method. Move it down to sit directly above private static async Task RunPolicyAsync(...) (line 1051).

[claude]

Review: test-suite-only PR, Darling/Postgres side

This PR only touches Darling/Darling.Tests/* (plus CHANGELOG) — no shipped product code changed, confirmed against the diff. Since the bugs being fixed are TimescaleDB/PostgreSQL-specific (extension enablement ordering, compression-job scheduling races), there's no Lite counterpart to keep in parity here; Lite uses DuckDB and has no equivalent collection fixture or compression-policy test surface. No Lite/Darling drift.

What it does

• Adds LivePostgresStoreFixture (ICollectionFixture<>) wired via a new LivePostgresCollection definition, so the live-postgres xUnit collection migrates the store and enables TimescaleDB once, before any member class runs — closing an order-dependent flake where "the store is established" was an emergent property of which class the runner scheduled first.
• Adds a seam test (LivePostgresStoreFixtureTests) that asserts the fixture actually ran, structurally pinning the wiring (deleting/unhooking the fixture breaks constructor injection loudly).
• Tightens LivePostgresCollectionHygieneTests: the attribute-presence check now requires the [Collection("live-postgres")] literal to open a line (previously a bare substring search over the doc-comment lookback window, which the new fixture's own doc comments would have tripped, since they mention the attribute in prose). Verified: all ~79 existing real usages are alone on their own line, so this tightening doesn't create false positives. The new fixture class is exempted from the "must carry [Collection]" rule via reflection off [CollectionDefinition]/ICollectionFixture<> rather than a third prose marker — a nice touch, since it makes the exemption track the actual wiring instead of being fakeable by pasting a comment.
• Fixes a masked ordering bug in PayloadDimensionLiveTests.EnsureAggregatesWithoutPoliciesAsync: adds the missing ConvertToHypertablesAsync call before EnsureContinuousAggregatesAsync (continuous aggregates can't be built over a heap). Confirmed the other 3 of 4 call sites already convert on their own, so this is a real gap-fill, not a redundant add (harmless anyway — idempotent if_not_exists).
• Fixes DarlingAnomalyBaselineTests to assert existence of all nine baseline relations instead of a created-count of nine — correct, since EnsureBaselineFallbackViewsAsync legitimately returns fewer than nine when a sibling test's aggregate is already standing (verified against the actual implementation in TimescaleSupport.cs).
• Fixes a genuine, CI-caught race in three TimescaleSupportTests compression tests: add_compression_policy launches its job within ~1-2s with no initial_start, so tests that inserted chunks before adding the policy could get raced by a background compression run with no lock_timeout. The fix (add the policy first while parked, before any chunk exists) is well-reasoned and the ordering rationale (parking doesn't recall an in-flight run) is correctly called out as load-bearing.

Correctness spot-checks

• All new/changed calls against TimescaleSupport (ConvertToHypertablesAsync, EnsureContinuousAggregatesAsync, BaselineAggregates, BaselineRelationExistsSql, TryEnableAsync) match the real signatures and documented failure-isolation behavior in TimescaleSupport.cs — nothing invented.
• LivePostgresStoreFixture.InitializeAsync correctly no-ops when DARLING_TEST_PG is unset (ungated run) and lets a real migration/extension failure throw rather than swallowing it, which is the right behavior for a fixture that every collection member depends on.
• The stricter CarriesLiveCollectionAttribute line-start check is a real improvement — the old substring check would have been fooled by any doc comment quoting the attribute (which the PR's own new fixture doc comment does, 19 lines above its declaration).

One nit (posted inline)

• TimescaleSupportTests.cs: the pre-existing one-line comment /* Runs the policy body NOW instead of waiting out its schedule. job_id is INTEGER (#1586). */ describes RunPolicyAsync, but the new AddCompressionPolicyParkedAsync method (with its own large doc comment) was inserted between that comment and RunPolicyAsync's declaration, so the old comment now floats above the wrong method.

No security concerns (test-only code; the SQL string interpolation into alter_job/probe queries uses only hardcoded test constants, never external input). No missing-index-DMV suggestions, per instructions. Overall this is a well-verified fix (CI-driven RED-first repro, cross-checked against actual API bodies) for a real and previously-hard-to-diagnose class of test flakiness.