A runtime-precondition miss vocabulary, evaluated at read time (#2546)

[erikdarlingdata]

Closes #2546 — no, deliberately does not close it: see "What I did not wire" below.

The distinction, and why it is not a fourth capability axis

Engine edition (#2511) and engine kind (#2536) are properties of the target, fixed for the life of a connection, and both answer "this collector can never run here". That fixedness is what makes the sweep-and-derive machinery sound.

A precondition is neither. It is mutable — somebody can start the session, issue the GRANT or create the extension while the service is running — and it is actionable, where the other two are final. A gate decided at connect time would go on reporting a precondition after somebody satisfied it: the operator does exactly what the message asked for and nothing changes, with no way to tell why. That is the worst direction to be wrong in.

So this is answered at read time, from what collection already recorded, and re-derived on every call.

The defect it closes: the store already knew, and no read reported it

This turned out not to be speculative machinery for PostgreSQL. Every fact needed is already collected, already classified, and already carries its own remedy — it just had nowhere to go:

• DarlingWorker.PostgresFaultOutcome sorts a denied grant, a missing source object and a disabled feature out of the general ERROR bucket and writes the actionable sentence into collection_log.error_message. The SQLSTATE 42P01 case says CREATE EXTENSION pg_stat_statements in so many words.
• The SQL Server permissions catch does the same through the shared SqlServerPermissionErrors predicate, on both SKUs.
• SESSION_MISSING exists as a distinct status for a capture session that is gone.
• The hourly query_store_health collector has recorded actual_state per database all along.

Meanwhile two Query Store reads were literally guessing in prose — "Query Store may not be enabled on target databases" — which is equally true of a server where it is enabled and the window simply reached past what the raw tier retains. That is the "captured data that no read reports" shape.

What is in this PR

The vocabulary — PerformanceMonitor.Collectors/CollectorRuntimePrecondition.cs: the precondition status word and two message shapes. The remedy text is framed, not authored — a second copy of prose whose whole value is being accurate about one server's answer is the copy that drifts, so the message quotes what the server said. The noun phrase is borrowed from CollectorEngineCapability.CapturePathByCollector rather than restated in a second table.

The two store halves — DarlingRuntimePrecondition (Npgsql) and McpRuntimePrecondition + LocalDataService.RuntimePrecondition (DuckDB). Neither holds any vocabulary, so the SKUs are byte-identical here by construction rather than by pinning. A failed diagnostic read answers null, for the same reason the capability probe does.

Six call sites, five of them on both SKUs, always in the order capability → precondition → the read's own miss:

read	collector	what it can now say
get_deadlocks	deadlocks	the capture session is missing, not "no deadlocks"
get_blocked_process_xml	blocked_process_report	same
get_running_jobs	running_jobs	the login was denied msdb, not "no jobs are running"
get_long_query_completions	long_query_completions	the session is missing, not "enable the opt-in collector" (it is already on)
get_query_store_top	query_store + the health snapshot	Query Store is OFF / READ_ONLY / ERROR on these databases, with the remedy for the state it is actually in
get_pg_top_queries (Darling only)	pg_statement_stats	the stored CREATE EXTENSION, quoted

Invariants respected

• "After the read, never before" is untouched. Both questions are asked on the miss path only, so a server whose recorded state disagrees with its collected rows still gets its data. get_pg_top_queries: make it parse, and stop rounding the PostgreSQL int8 identities (#2554, #2548) #2553 made the THROW path a miss path rather than moving the gate earlier; this follows the same rule.
• No sweep change and no IL-guard change. CollectorEngineCapability is read from, never modified. This asks a different question of the store, not a new question of the gates.
• Permanent outranks fixable. Nothing in the type system enforces that and a ?? chain is one cut-and-paste from reversing it, so a source scan pins it.

What I did not wire, and why

• system_health stopped. The issue names it, and it is not detectable from the store today. SystemHealthEventsCollector joins sys.dm_xe_session_targets to sys.dm_xe_sessions; a stopped session produces zero rows and the collector logs SUCCESS. It never goes through RunXeTolerantAsync, so it cannot raise SESSION_MISSING. The three captures that do classify it — deadlocks, blocked_process_report, long_query_completions — are wired instead. Making the system_health case reportable needs the collector to record session state, which is its own change.
• HasMsdbAccess as a gate. See below.
• Lite has no SESSION_MISSING status. Its runner records a failed XE-session ensure as PERMISSIONS (a denial) or ERROR (anything else), so the SESSION_MISSING branch is wired and correct but currently silent on that SKU. Wired anyway: the day Lite classifies it, every read starts answering without another edit, and wiring only the statuses a SKU writes today is how the two drift apart.

The HasMsdbAccess determination

Checked first, as asked. The framing in #2546 is right about the fact and slightly off about where the wrongness lives.

HasMsdbAccess comes from HAS_DBACCESS(N'msdb'), probed once in DarlingServerConnector.ConnectAsync, and it is a GRANT somebody can change without a restart — so on both of the properties #2546 uses to separate the axes, it belongs in this vocabulary and not on the capability axis.

But it is not currently reported on the capability axis either. TargetsWithEngineEdition deliberately varies it, so AFixableGate_IsNotReportedAsAnEngineGap already asserts job_history / running_jobs / agent_status are not engine gaps on a box edition. The capability answer is correct. The wrongness is two other things:

1. Nothing reported it at all. get_running_jobs answered empty — "No running SQL Agent jobs found" — which is an affirmative claim about the server's Agent, not "we cannot see it". Fixed here for the case where HAS_DBACCESS returns 1 but the job tables deny: the collector runs, is refused, and records the denial, which this read now surfaces.
2. ServerRuntime is cached for the life of the connection. It is dropped only on config change, removal, or a connect/collection failure — so on a healthy server the msdb probe is frozen until the service restarts, and a GRANT genuinely does not take effect. Not fixed here, because the only two honest fixes are both a different lane:
   • Remove HasMsdbAccess from the three AppliesTo gates and let the collectors run and fail into PERMISSIONS. That fixes it completely and needs no sweep change — but it re-introduces the wasted per-cycle query that gating it off deliberately removed, and it is a change to collection dispatch, not to a read vocabulary. Your call, and worth its own issue.
   • Stamp has_msdb_access onto servers (a migration rung + Lite twin + viewer probe) — which would let the read state it positively, but the value would still be connect-frozen, so we would be shipping advice that does not take. Worse than not shipping it.

So: the determination is yes, it belongs in this vocabulary, this issue is the right home for something the SQL Server side already gets slightly wrong, and half of it is fixed here. The remaining half is a gate decision I have deliberately surfaced rather than taken.

Verification

Darling.Tests and Lite.Tests are net10.0-windows and cannot run on macOS, so pure logic and the source scans were verified against the shipped build in a throwaway net10.0 harness — 63 checks, all passing — and every guard was proven red first:

mutation	result
reverse capability/precondition order in McpJobTools	RED: get_running_jobs: precondition asked BEFORE capability
drop the Lite query_store wiring	RED: parity floor + Lite wires get_query_store_top
delete the "any READ_WRITE in scope" guard	RED: any READ_WRITE in scope makes no claim, an unrecognised state makes no claim
delete the ERROR branch (review fix)	RED: all six ERROR checks, including the mixed-scope case that named OffDb while dropping the broken database entirely

Also added, running in CI:

• RuntimePreconditionMissLivePostgresTests — end to end against a live store, both directions, including the property a gate cannot give: the same read on the same server stops saying it the moment collection records a healthy run.
• Four Lite behavioural tests through the real tools against a seeded DuckDB, also both directions.
• Two fragments added to McpMissMessageParityPinTests, floor bumped 9 → 11.

Full solution builds clean on macOS with no new warnings.

🤖 Generated with Claude Code

Review

claude[bot] found one real defect: actual_state = 'ERROR' fell through to the OFF wording, so a database whose Query Store was configured and then broke got "turn it on" — an ALTER already in effect that silently no-ops — and in a mixed scope the sentence named the OFF database while dropping the broken one entirely. Fixed in 1ec0737: three not-collecting states, three remedies, ordered by how badly the OFF wording would mislead. Two tests, proven red first.

[claude]

actual_state = 'ERROR' gets the "not enabled, turn it on" message — which is wrong for that state.

IsCollecting (line 227) deliberately excludes "ERROR" from "collecting" — correct, since Query Store isn't actively recording in that state. But the bucketing right after only splits READ_ONLY out; everything else (OFF, ERROR, null/unrecognised-but-falsy) lands in off and gets:

"Query Store is not enabled on … so there is nothing for this read to return … Turn it on with ALTER DATABASE [name] SET QUERY_STORE = ON …"

ERROR is a documented actual_state_desc value distinct from OFF (per sys.database_query_store_options) — it means Query Store was configured (desired_state is typically still READ_WRITE) but hit an internal error and stopped recording, usually needing investigation/repair (or a disable+re-enable cycle), not a bare SET QUERY_STORE = ON (which the database may already have in effect). Telling an operator to "turn it on" when it's already on and broken sends them down the wrong path, and re-running the suggested ALTER may silently no-op.

The READ_ONLY case got its own branch specifically to avoid this class of mistake (see the comment right above it: "Saying 'Query Store is off' about a database whose desired state is READ_WRITE would send the operator to re-run an ALTER that is already in effect") — ERROR deserves the same treatment but currently falls through to the OFF wording instead. There's also no test coverage for actual_state = "ERROR" in QueryStoreDisabledMessage/QueryStoreCollectingOrUnknown_MakesNoPreconditionClaim (only collection_log's unrelated status = "ERROR" is pinned).

[claude]

Reviewed the diff (C# only — no T-SQL touched). Overall this is well-structured: the vocabulary/store separation keeps the two SKUs byte-identical by construction, the capability→precondition→miss ordering is pinned by a source-scan test, and parity between Lite and Darling looks solid (shared wiring test, bumped pin count, matching instructions text). Verified the DegradedStatus = "PERMISSIONS" claim against DarlingWorker.PostgresFaultOutcome and SqlServerPermissionErrors — all three sub-cases (denied grant, missing object/extension, disabled feature) really do write status PERMISSIONS, so the get_pg_top_queries wiring isn't dead.

One correctness issue found in CollectorRuntimePrecondition.QueryStoreDisabledMessage — left as an inline comment: actual_state = 'ERROR' (a real, documented Query Store state distinct from OFF) gets bucketed with OFF and receives the "not enabled, turn it on with SET QUERY_STORE = ON" message, which is misleading advice for a database that's already configured but hit an internal Query Store error. READ_ONLY got its own branch to avoid exactly this kind of wrong-remedy mistake; ERROR didn't, and there's no test covering it.

No security, SQL-injection, or performance concerns — all new queries are parameterized and read from existing collected data (no new probes against monitored servers).