[erikdarlingdata]

Closes #2512.

What the gate cost

public override bool AppliesTo(CollectorTargetInfo target) => !target.IsAzureSqlDb;

Every Azure SQL Database target — General Purpose and Hyperscale — collected no tempdb data at all. No get_tempdb_trend, no tempdb space alerting, nothing in the viewer's tempdb surfaces, nothing for the analysis pass's TEMPDB_USAGE fact to score. On a platform where tempdb contention is common and the customer cannot go and look at the box themselves.

Its doc comment said the first result set's three-part tempdb.sys.dm_db_file_space_usage reference could not be served there, so the collector "could only ever fail". That premise was checkable.

The live evidence

The collector's SQL, verbatim, both result sets, over an Entra token (2026-08-22):

tier	result set 1	result set 2
GP_S_Gen5_2 (EngineEdition 5, 12.0.2000.8)	user 5.44 MB, internal 1.81 MB, version store 0.00 MB, total 7.25 MB, unallocated 54.19 MB	session 74, 0.13 MB, 1 session over threshold
HS_S_Gen5_2	user 1.88 MB, internal 0.00, version store 0.00, total 1.88 MB, unallocated 60.69 MB	session 1, 0.00 MB, 0 over threshold

SELECT COUNT(*) FROM tempdb.sys.dm_db_file_space_usage returns 4 on both. No Azure-specific query variant is needed, and both result sets bind — so the one row this collector promises is a whole row on Azure SQL DB, which is what the old comment's "half a row is a different contract" objection was about.

And the figures describe something rather than merely returning, which was the open question. Allocating ~57 MB into a #temp table on GP_S_Gen5_2:

step	user_mb	unallocated_mb
before	1.88	60.69
after ~57 MB	59.75 (+57.87)	2.69 (−58.00)

with sys.dm_db_session_space_usage attributing 59.25 MB to the session that did it. The counters track actual allocation; the session view attributes it correctly.

Worth more here than on a box. The tempdb ceiling on Azure SQL Database is governed by the service tier: you cannot add files and cannot grow past the cap. On a box "tempdb is filling" means "go look at the disk". Here it means "you are approaching a limit you cannot raise without changing service objective".

What each red test was really pinning

CollectorGateSurfacePinTests.TempDbStats_AppliesTo_SkipsOnlyAzureSqlDb — the only test that goes red, and it was pinning two different claims in one Assert.False:

1. That the DMV cannot be served on Azure SQL DB. False, per above. Flipped.
2. That a login might not be able to READ it. True — but that is a property of the login, not of the tier, so it cannot be decided by a gate. It moved to the fault classifier (below).

The other half of that test — Assert.True(...AppliesTo(AzureMi)) — never changed, and it is why the test survives rather than being deleted. Its original comment says outright that asserting both directions is what stops an "anything Azure" gate creeping in. That risk now runs the other way: this must not be re-narrowed later on the strength of the stale comment, so both directions are still asserted, and the test now also asserts the composed CollectorCatalog.AppliesTo and the by-name surface Lite dispatches through — neither of which AppliesTo alone can see.

Nothing else was pinning the gate. AzureSweepScopeTests, CollectorRunnerConnectionEngineTests and DarlingWorkerTests don't reference this collector at all; DarlingCliCommandsTests.ToTargetInfo_RoundTripsTheFactsTheGateReads asserts all SQL Server collectors are off at a PostgreSQL target, which is the engine half of the composed gate and is unaffected — verified by asserting the same thing directly against the real build.

Coordination with #2511

The composer's capability map is derived, not transcribed — DarlingWebEndpoints.BuildAppliesToNode calls collector.AppliesTo(s_azureSqlDbTarget) per measure — so the three tempdb_* measures flip to azureSqlDb: true with no edit here, and DarlingComposeTests.Catalog_Measures_CarryAppliesTo_ReflectingTheCollectorGate doesn't name tempdb. If #2511's not_collected mapping is derived the same way it will flow through automatically; if it goes red on this branch, that is the signal the map was transcribed.

What I verified about payload composition

The issue listed what it had not checked. All of it, executed against the real build rather than reasoned about:

• The composed gate, both halves: CollectorCatalog.AppliesTo(def, azureSqlDb) → true, (def, azureMi) → true, (def, postgres) → false (the engine half still holds). Plus CollectorCatalog.AppliesTo("tempdb_stats", azure), the by-name surface Lite dispatches through.
• RunsPerDatabase(azure) → false, and BuildEnumerationQuery → null. So on Azure this takes the plain single-connection path, not the per-database loop file_io_stats and index_object_stats take. Per [BUG] Blocking/Deadlock alerts cross-talk between Azure SQL databases sharing the same logical server (Darling) #2220 a registration naming a database is scoped to that database, so this is one tempdb snapshot per registration, not N.
• Query text is byte-identical on Azure and on-prem — no variant, as the measurement says none is needed.
• Column typing. sys.dm_db_session_space_usage.session_id is smallint, so the driver hands back a short; the definition reads it through Convert.ToInt32(GetValue(0)) precisely for that. Fed a real short, ReadAsync widens it and WritePayload emits int for top_session_id INTEGER. The existing parity pin feeds an int and can never see this. total_sessions is COUNT_BIG → long → bigint.
• Payload order and arity: 8 values, PayloadColumns order, CLR types decimal ×5, long, int, decimal, matching the generated DDL (numeric(18,2) ×5, bigint, integer, numeric(18,2)).

It composes cleanly. Nothing had to be forced.

The permissions half — what actually makes this safe

The #2150 field report was 11x consecutive with error 262, "VIEW DATABASE PERFORMANCE STATE permission denied in database 'tempdb'". That is real, and removing the gate without handling it would have regressed a fixed bug — 262 was classified Unclassified, which means ERROR, every cycle, forever, which is exactly the collection-health pollution the gate was created to stop.

262 is now a permission denial in both SKUs, so such a login degrades to a non-fatal PERMISSIONS skip with an explanatory Azure sentence, the same treatment error 300 already gets (#1631) — while every properly-permissioned Azure target collects.

Those numbers lived in three hand-maintained places and had already drifted: 916 was in SqlServerTargetProvider.Classify and in neither runner's catch, so the engine seam whose stated purpose is that "the runner and the provider cannot disagree about what an error means" disagreed. All three now read one shared SqlServerPermissionErrors.IsPermissionDenied. That is also the only way to pin them — SqlException cannot be constructed in a test, so a shared predicate is the difference between a tested rule and three unverifiable literals. Note the behaviour change: Lite and Darling's runners gain 916 → PERMISSIONS. That is the correct classification per CollectorTargetFault's own definition, and it is called out here rather than smuggled in.

Thresholds that need attention — filed as #2515

unallocated_mb is free space in the tempdb files AS CURRENTLY ALLOCATED, not headroom to the tier ceiling. Azure creates them small — 4 files, ~62 MB total on both 2-vCore tiers measured — and autogrows toward the cap. So total_reserved / (total_reserved + unallocated), which is exactly what TempDbSpaceInfo.UsedPercent computes and what the tempdb Space alert fires on at a default of 80%, measures distance to the next autogrow, not to the ceiling.

Run the alert's own arithmetic on the measured numbers:

	UsedPercent
idle	3.0%
after one ~57 MB #temp table	95.7%

One ordinary temp table puts an Azure SQL DB target over the default with 60 MB at stake. That would fire on the first busy minute of every Azure target this PR enables, which is the "worse than the gap" case. The alert needs a size floor — the shape ScorePlanCacheBloat already uses ("a high % on a trivially small footprint is noise").

I did not retune it here. The floor value is a policy call that suppresses alerts existing on-prem and RDS targets get today, and I cannot measure what those customers rely on from here — note that AlertEngineTests.TempDb_FiresAtThreshold_AndResolutionCarriesTheCurrentPercent uses a fixture totalling exactly 1000 MB, so a 1 GB floor flips an existing pin. #2515 carries the mechanism, the three options, and the measurement that should decide (the distribution of total tempdb size at the moment UsedPercent >= 80 across collected history). If you want the floor in this PR instead, say so and I will add it.

Second threshold, failing the other way and needing no change: FactScorer.ScoreTempDbVersionStore bars at 1000/2000/5000 MB are unreachable while tempdb sits at ~62 MB, so that arm scores 0. It is self-consistent — a >1 GB version store implies a >1 GB tempdb — and produces no false alerts. Worth knowing because RCSI is on by default on Azure SQL Database, which makes the version store the tempdb consumer most likely to matter there; the space-fraction arm is the only cover until tempdb grows past a gigabyte. Noted in #2515, not acted on.

get_tempdb_trend (both the MCP read and the viewer's), TempDbSpaceSql, and the analysis facts needed no change: they are column reads with no absolute-MB assumptions, and the numeric(18,2) → double precision / bigint / integer typing is already what an Azure row produces.

Testing

• CollectorGateSurfacePinTests.TempDbStats_AppliesTo_EveryTarget_IncludingAzureSqlDb — flipped Azure, kept MI, added the composed and by-name surfaces.
• TempDbStatsCollectorDefinitionTests.AzureSqlDb_MeasuredValues_ComposeThroughToThePayload — the composition run above, using the values GP_S_Gen5_2 actually returned and a real short session id.
• SqlServerPermissionErrorsTests — the six permission numbers, six non-permission numbers that must stay loud, and the 262 hint on/off Azure.

Test projects are net10.0-windows and cannot run on macOS, so every assertion above was additionally executed against the real build through a throwaway net10.0 harness first. CI is the arbiter for the suites themselves.

🤖 Generated with Claude Code

[claude]

Correctness: DatabaseScopedHint assumes every 262 is about tempdb, but the check has no way to know that.

For(int sqlErrorNumber, bool isAzureSqlDb) dispatches on the bare error number alone — it never inspects ex.Message to see which database the denial actually named. Since SqlServerPermissionErrors.IsPermissionDenied now includes 262 for every collector (both Darling's worker catch and Lite's RemoteCollectorService catch call AzureDmvPermissionHint.For(ex.Number, isAzureSqlDb) unconditionally — see DarlingWorker.cs:4500 and RemoteCollectorService.cs:639), any collector that raises SQL error 262 on Azure SQL DB gets this exact tempdb-specific text appended, e.g. "Reach tempdb's space DMVs through server-level state access instead", even when the denial named a completely different database.

Today tempdb_stats is the only cross-database read that can plausibly raise 262, so this is silent for now. But the per-database loop collectors (index_object_stats, file_io_stats, etc.) run against arbitrary user databases on Azure SQL DB and could plausibly raise a genuine "permission denied in database '<userdb>'" 262 for an unrelated reason — and they'd get the same wrong, tempdb-only guidance appended to their error message.

Worth either checking the database name embedded in ex.Message before applying DatabaseScopedHint, or scoping this hint to only fire for the tempdb_stats collector specifically, rather than keying purely off the SQL error number.

[claude]

Reviewed the tempdb/Azure SQL DB gate removal and the permission-error consolidation. The core change is solid — the composed gate derivation (DarlingWebEndpoints.BuildAppliesToNode) confirms the viewer/composer surfaces pick this up automatically with no separate edit needed, and SqlServerTargetProvider.Classify / Darling's worker catch / Lite's catch now agree via the shared SqlServerPermissionErrors.IsPermissionDenied predicate. Left one inline correctness comment on AzureDmvPermissionHint.cs.

Two more observations that don't have a changed line to anchor to:

PerformanceMonitor.Collectors/CollectorTargetFault.cs:31 — the Permissions enum member's doc comment still reads SQL Server: 229, 297, 300, 8189, but that set is now stale: SqlServerTargetProvider.Classify delegates to SqlServerPermissionErrors.IsPermissionDenied, which also includes 262 and 916. Small thing, but this PR's whole rationale is eliminating exactly this kind of hand-maintained-list drift, so it's worth closing the loop here too.

Consolidation is incomplete — two more hand-rolled copies of the same permission-number pattern remain, un-migrated:

• Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:3508 (GetRecentlyFailedJobsAsync's FailedJobsQuery catch): ex.Number is 229 or 297 or 300 or 916
• Lite/Services/RemoteCollectorService.RunningJobs.cs:64: the identical ex.Number is 229 or 297 or 300 or 916

These two are consistent with each other (Lite/Darling parity is fine), but both are now out of step with the new SqlServerPermissionErrors predicate (missing 262 and 8189 — arguably correctly, since neither applies to an msdb job-table read, but that's an assumption a future reader has to rediscover rather than see stated). Given the PR's stated goal was fixing exactly this "lived in three hand-maintained places and had already drifted" problem, it's worth either migrating these two to the shared predicate (with a narrower method/overload if the set genuinely needs to differ) or leaving a comment explaining why they're deliberately separate.

There's also Lite/Services/RemoteCollectorService.cs:616 ((sqlError.Number == 229 || sqlError.Number == 297 || sqlError.Number == 300) in the XeSessionEnsureException catch) — Darling has no equivalent code path here (no XeSessionEnsureException type on that side), so this isn't a parity gap, just a fourth hand-rolled literal set that could drift from the shared predicate over time.

None of these are blocking — the core fix (gate removal + permission classification) is well-verified against live measurements per the PR description.

[erikdarlingdata]

Review finding fixed — c4c3197

claude[bot] caught a real one on AzureDmvPermissionHint, and it was the exact thing I hedged on in wording instead of solving in code: the hint dispatched on the error number alone, so any 262 on an Azure SQL DB target got the tempdb sentence appended — including a denial that named someone's user database.

That is worse than appending nothing. "Reach tempdb's space DMVs through server-level state access" sends the reader after a ##MS_ServerStateReader## membership that would not have helped, for a denial where the raw error already names the database and the grant genuinely is the fix.

That difference is now stated where the two arms are, because it is the reason they are shaped differently:

• 300 is server-scoped. On Azure SQL Database it can only ever be the service objective, so the number settles it.
• 262 names a DATABASE, and the advice inverts on which one — in tempdb there is no grant to issue, in a user database there is. The number cannot settle it, so it reads the message.

Both call sites now pass ex.Message, and the arm matches the quoted identifier that error 262's format string interpolates (permission denied in database '%.*ls'). Quoted rather than bare, because tempdb is a reserved name and no user database can collide with 'tempdb' in that position — while a bare match would read a database called reporting_tempdb_stage as tempdb. Case-insensitive, because the quoted name follows the server's collation and not this code's.

A null or absent message stays silent rather than guessing. That direction is deliberate: a call site that forgets the message loses a helpful sentence instead of gaining a wrong one.

Four new pins cover it — the tempdb message, a user-database message, the reporting_tempdb_stage look-alike, null/empty, and the three casings.

First commit's CI

All six checks passed on 6c3867a (mergeable=MERGEABLE, mergeStateStatus=CLEAN), including the Windows build job where the flipped pin and the two new test classes run. This push re-runs them.

#2511 / PR #2517 — composes, one assertion to reconcile

Checked rather than assumed: their CollectorEngineCapability.IsCollectedOnEngineEdition sweeps TargetsWithEngineEdition through CollectorCatalog.AppliesTo. The mapping is genuinely derived — there is no enumerated gap list — so this gate change flows through it automatically and I edited nothing of theirs. tempdb_stats simply drops out of the derived Azure set, taking their count 13 → 12.

The one place that needs reconciling is a pin, not the map: Darling.Tests/EngineCapabilityMissTests.cs:480-481 and Lite.Tests/EngineCapabilityMissTests.cs:103 assert get_tempdb_trend answers not_collected on an Azure server. After both land the correct answer is unavailable again, because the collector really does run and the absence really is worth looking into. Raised on #2517 with the options and left as their call rather than edited from here — landing a change that reds their open PR and editing their file to match would be forcing the outcome twice over.

Worth noting for its own sake: that not_collected branch was about to tell Azure callers tempdb is a permanent capability gap, which the measurements in #2512 show is false. Their own argument — a confident permanent-gap message is worse than a vague one when the gap is not permanent — lands on their own PR here.

[claude]

The doc comment above frames this as fixing exactly this kind of drift ("it already WAS three literals, and they had already drifted... deriving all three from this set is how it cannot"), but the consolidation is incomplete. Three more sites still hardcode their own copy of the permission-number list instead of calling IsPermissionDenied:

• Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:3508 — catch (SqlException ex) when (ex.Number is 229 or 297 or 300 or 916) (in GetRecentlyFailedJobsAsync)
• Lite/Services/RemoteCollectorService.RunningJobs.cs:64 — the same literal set, in Lite's equivalent method
• Lite/Services/RemoteCollectorService.cs:616 — status = (sqlError.Number == 229 || sqlError.Number == 297 || sqlError.Number == 300) in the XE-session-missing catch

None of these picked up 262 or 8189, and none of them will pick up whatever number gets added to SqlServerPermissionErrors next — they're still hand-maintained lists that can drift, which is the exact failure mode this PR set out to close. Worth routing them through SqlServerPermissionErrors.IsPermissionDenied too (or filing a fast-follow), otherwise the "one shared predicate" claim in the PR description isn't quite accurate.

[claude]

Review summary

Reviewed the diff (Azure SQL DB gate removal on TempDbStatsCollector, the new SqlServerPermissionErrors shared predicate, and the 262/tempdb arm of AzureDmvPermissionHint).

Overall: the core change is well-supported (live measurements cited in the doc comments/PR body) and Lite/Darling parity is maintained for everything actually touched — both catch sites (DarlingWorker.cs, RemoteCollectorService.cs) now call the same SqlServerPermissionErrors.IsPermissionDenied and both pass ex.Message through to AzureDmvPermissionHint.For for the 262 database-name check. SqlServerTargetProvider.Classify picks up 262 too, so Darling's fault classification agrees with both runners' catch filters. The NamesTempDb quoted-identifier match ('tempdb', case-insensitive) correctly avoids false-positiving on a database like reporting_tempdb_stage, and the test coverage for it is thorough. No T-SQL changed (the tempdb_stats query text is untouched — still OPTION(RECOMPILE), AS on aliases, column_name = expression), so the style guide doesn't come into play here.

One gap worth a look: left an inline comment on SqlServerPermissionErrors.cs — the PR's stated goal is eliminating exactly the kind of hand-maintained-list drift that let 916 diverge across three places, but three other sites still hardcode their own 229/297/300/916 list instead of calling the new shared predicate:

• Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:3508 (GetRecentlyFailedJobsAsync)
• Lite/Services/RemoteCollectorService.RunningJobs.cs:64 (Lite's equivalent)
• Lite/Services/RemoteCollectorService.cs:616 (XE-session-missing catch)

These aren't broken by this PR and Lite/Darling stay in parity with each other at those sites, but they're the same drift risk this PR calls out, just left unconsolidated. Not a blocker, but worth a fast-follow or a note in the PR description that it's deliberately out of scope.

No correctness, security, or missing-index concerns found beyond that.

[erikdarlingdata]

Second review finding fixed — 3743fa0

Fair catch, and the sharper of the two: the doc comment claimed the drift was closed while three more hand-written copies were still sitting there, which made the claim less true than before rather than more.

I took the census properly this time instead of fixing the instances the first pass happened to touch:

site	229	262	297	300	916	8189
SqlServerTargetProvider.Classify	✓		✓	✓	✓	✓
DarlingWorker collector catch	✓		✓	✓		✓
Lite RunCollectorAsync collector catch	✓		✓	✓		✓
DarlingWorker failed-jobs msdb read	✓		✓	✓	✓
Lite failed-jobs msdb read	✓		✓	✓	✓
Lite XE-session-ensure catch	✓		✓	✓

Six lists, four distinct sets, one question. Not one of them had 262. All six now ask SqlServerPermissionErrors.IsPermissionDenied.

One set is right for all of them because they all answer the same question — did the login get told no? — even though they act on it differently: the collector catches record PERMISSIONS instead of ERROR, the failed-jobs reads return an empty list instead of failing the alert cycle, the XE catch marks the capture unavailable. What each one does with the answer is its own business; what counts as a denial is not. That is now stated on the type.

Widening is correct at each site, not merely tolerable, and I checked rather than assumed:

• failed-jobs msdb reads gain 262 and 8189. A 262 naming msdb is precisely the least-privilege case those catches exist for, and it used to fall through to the warning arm. 8189 is sys.traces and cannot arise from an msdb read at all.
• the XE-session catch gains 262, 916 and 8189. 8189 is inert there for the same reason; 262 and 916 both mean the login was denied where it asked, which is exactly the PERMISSIONS that arm already records for 229/297/300.

This is the second instance of one category in this PR — a rule stated in one place and transcribed into others — so it was worth taking the whole census rather than fixing the three the review named and leaving a seventh to be found later.

[claude]

Reviewed. This is a clean, well-tested fix — a few notes, no blockers found.

What it does: Removes the !target.IsAzureSqlDb gate on TempDbStatsCollector.AppliesTo, backed by live measurements showing both result sets bind and return real, moving data on Azure SQL DB (GP and Hyperscale). The actual #2150 field failure (error 262, VIEW DATABASE PERFORMANCE STATE permission denied in database 'tempdb') is correctly re-routed to the fault classifier instead of the gate, via a new shared SqlServerPermissionErrors.IsPermissionDenied predicate that replaces six independently-drifted copies of the error-number list across Darling's worker, its target provider, and Lite's collector/XE/failed-jobs catches.

Parity: Solid. TempDbStatsCollector and SqlServerPermissionErrors both live in the shared PerformanceMonitor.Collectors project, so Lite and Darling get the gate removal and the classification fix from one source rather than two synchronized edits. Grepped for any leftover Azure-specific tempdb gating in Darling's dispatch table and Lite's schedule/schema config — found none.

AzureDmvPermissionHint.For message-sniffing for 262 (PerformanceMonitor.Common/AzureDmvPermissionHint.cs): matching on the quoted 'tempdb' substring in the driver message to decide whether to attach the tempdb-specific hint is a reasonable approach given 262 is database-scoped and the number alone doesn't say which database. The reasoning for why a bare-word match would misfire (reporting_tempdb_stage) and why the quoted-identifier match can't is sound, and it's covered by tests including the "message doesn't name tempdb" and casing-variance cases.

One pre-existing thing adjacent to this change, not a defect in it: DarlingWorker.IsPermissionError (used only by the ad-hoc execute_actual_plan/fetch_active_queries commands, not the collection cycle) is a separate, differently-scoped permission-number list (229, 230, 262, 297, 300, 301, 916, 33044 plus a message-text fallback) that wasn't folded into the new shared predicate. That's plausibly fine since it serves a different question (arbitrary user-object access during an ad-hoc command vs. collector DMV access), but worth a conscious call — if it was meant to be swept up too, it's the one hand-maintained list this PR's "six places" count didn't reach.

No SQL files changed, no correctness issues found in the widened permission set (each catch site's own comments correctly reason through why the widening is safe there), and the new tests exercise the real edge cases (short→int widening for top_session_id, per-database vs. single-connection fan-out, casing variance in the 262 message).

[erikdarlingdata]

Reviewed and ready — deliberately not merging yet.

This PR is correct and I want it. The hold is the consequence it documents: enabling Azure tempdb collection while UsedPercent still divides by the currently allocated file size arms the tempdb Space alert on every Azure target, and it fires on the first busy minute. Shipping collection that immediately pages people is worse than the gap it closes, which is exactly the judgment this PR made in refusing to retune the threshold itself.

Since then the question got settled rather than deferred. Probing the same GP_S_Gen5_2 live: tempdb is 4 ROWS files of 16 MB, each with max_size = 2,097,152 pages = 16 GB, so the real ceiling is 65,536 MB. The 57 MB #temp that reads 95.7% full against the allocation is 0.09% full against the cap. So the alert is not mistuned — it is measuring distance to the next autogrow, and it is nearly right on a box only because a pre-sized tempdb has already grown to its ceiling.

#2515 now has that evidence and is being fixed as a denominator correction (SUM(max_size) over ROWS files where it is a real value; today's behaviour where growth is unlimited). This merges immediately after it.

Two things in here I want to note separately, because they are worth more than the gate:

• Error 262 was classified Unclassified, so a permission denial logged an ERROR every cycle, forever. That is precisely the log pollution the gate was supposed to prevent — the gate was hiding a bug rather than avoiding one.
• That error-number set lived in six hand-written places that had drifted three ways. Now one shared predicate, which is also the only way to pin it, since SqlException cannot be constructed in a test.

And thank you for checking #2511's map rather than assuming: confirming it is genuinely derived from CollectorCatalog.AppliesTo is what makes this gate change flow through automatically instead of needing a hand-edit on their branch.

[erikdarlingdata]

#2517 merged first (f5a365eb), so this is now CONFLICTING and needs a dev merge. When you take it, two lines here will go red — flagging them with the exact patch so it costs you nothing.

EngineCapabilityReadWiringTests.EveryWiredRead_NamesARealCollectorThatIsGatedOffOnAzureSqlDb asserts that every read which asks the engine-capability question names a collector that is actually gated off on Azure SQL Database. Once you remove TempDbStatsCollector.AppliesTo's !IsAzureSqlDb, get_tempdb_trend's question becomes dead code and the pin says so. That is the pin working — it exists to stop a stale permanent-gap claim outliving its gate — but it means the un-wiring has to land with the gate change.

Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs (~line 396):

             if (rows.Count == 0)
-                return await DarlingEngineCapability.NotCollectedStatusAsync(postgres, resolved.ServerId, resolved.ServerName, "tempdb_stats")
-                    ?? McpHelpers.Status("unavailable", "No TempDB data available.");
+                return McpHelpers.Status("unavailable", "No TempDB data available.");

Lite/Mcp/McpTempDbTools.cs (~line 31):

             {
-                return await McpEngineCapability.NotCollectedStatusAsync(dataService, resolved.ServerId, resolved.ServerName, "tempdb_stats")
-                    ?? McpHelpers.Status("unavailable", "No TempDB data available.");
+                return McpHelpers.Status("unavailable", "No TempDB data available.");
             }

Nothing else of #2511's needs touching: the derivation is by-sweep, so tempdb_stats simply drops out of the Azure gap set on its own, and CapturePathByCollector["tempdb_stats"]… is the one other thing — EveryCapturePathEntry_NamesARealCollectorThatIsActuallyGatedSomewhere asserts every prose entry describes a collector that is still gated. So also drop this line from PerformanceMonitor.Collectors/CollectorEngineCapability.cs:

-            ["tempdb_stats"] = "the tempdb space-usage snapshot",

Three lines total. My test files no longer reference get_tempdb_trend at all (I swapped them to get_default_trace_events in 5f83a19 precisely so they would not be in your way), so nothing else in #2511's suite moves.

Why in your PR rather than a follow-up from me, having offered the other way round earlier: mine merged first, so splitting it would leave dev red between your merge and my follow-up, and a red dev is worse than either of us carrying three lines. If you would rather not, say so and I will land them the moment yours goes in — but the coherent change is one PR that opens the gate and retires the claim that depended on it.

And to close the loop on my earlier note: you were right that the gate's stated reason was false, and #2511 was about to convert that into a message telling Azure callers a working DMV "is not collected for this server, and never will". Good catch, and it is a better illustration of #2511's own thesis than anything in my PR body.

[claude]

Reviewed the diff (14 files, tempdb_stats gate removal + shared permission-error classification).

Verdict: looks solid, no correctness or parity issues found.

What I checked:

• The core claim is verified by the diff itself: TempDbStatsCollector.AppliesTo going from !target.IsAzureSqlDb to unconditional true is backed by the query being target-independent (BuildQuery doesn't branch), and CollectorGateSurfacePinTests now asserts both the raw gate and the composed CollectorCatalog.AppliesTo surface (both by-instance and by-name), so Darling and Lite can't drift on which collectors run where.
• SqlServerPermissionErrors.IsPermissionDenied correctly consolidates what were 6 independently-drifted literals (SqlServerTargetProvider.Classify, both DarlingWorker catches, both Lite RemoteCollectorService catches, plus the failed-jobs reads) into one predicate, and all production call sites were updated in parity — I grep'd for every remaining ex.Number is/== permission-style literal and found none left over.
• AzureDmvPermissionHint.For 262 handling: matches the quoted 'tempdb' substring case-insensitively, which is safe since tempdb is a reserved system database name (no user database can produce that exact quoted token), and correctly declines to guess when no message is supplied — verified this against SqlServerPermissionErrorsTests, including the case where a user database name contains "tempdb" as a substring (reporting_tempdb_stage), which correctly does not match.
• Dead-code removal is actually dead: removing the tempdb_stats entry from CollectorEngineCapability.CapturePathByCollector and the now-redundant NotCollectedStatusAsync calls in both Lite/Mcp/McpTempDbTools.cs and Darling/.../DarlingMcpDataTools.cs are consistent — IsCollectedOnEngineEdition derives from the (now-true) AppliesTo gate, so that call would return null anyway. There's also a pre-existing guard test (Darling.Tests/EngineCapabilityMissTests.cs) that already anticipated this exact PR and would fail if the dictionary entry weren't removed.
• Lite/Darling parity: every behavior change (916/262 added to the general collector-error classification, the XE-ensure catch, the MCP tempdb tools) was made in both apps in the same commit. No one-sided edits found.
• Scope discipline: the PR explicitly declines to retune the tempdb Space alert's 80% threshold (filed as The tempdb Space alert's percentage has no size floor, and on Azure SQL Database that makes it fire on one temp table #2515 instead) despite having the measurement showing it would false-positive on Azure SQL DB immediately — that's the right call for a PR whose stated purpose is the gate/classification fix, not alert tuning.

No T-SQL was touched in this PR (all changes are C#), so the CONTRIBUTING T-SQL style rules don't apply here. C# changes are well-commented per the "comments explain WHY" convention and don't need further nitpicking.

Nothing to flag for correctness, security, or parity.

[erikdarlingdata]

#2515 is implemented in #2521, so the caveat this PR carries is now resolved rather than deferred: the ratio did not need a size floor, it needed a different denominator. tempdb_stats collects max_size_mb = SUM(max_size) over tempdb's ROWS files, and TempDbSpaceInfo.UsedPercent divides by that where there is a real cap. On GP_S_Gen5_2 the #temp measurement recorded here — 59.75 MB reserved, 2.69 MB unallocated — goes from 95.7% against the allocation to 0.09% against the 65,536 MB cap, so it no longer clears the 80% default. Where any data file is unlimited (-1) there is no ceiling and the allocation stays the denominator, which is today's behaviour, so nothing an unlimited-growth on-prem or RDS target reports moves.

This PR is safe to ship, and it should go after #2521 — or at least not before it, since until #2521 lands, enabling Azure collection arms the alert this PR's own doc comment warns about.

Three things to expect when the two meet, from a trial merge of this branch into fix/2515-tempdb-ceiling-denominator. Only the first is a git conflict; the other two merge clean and are wrong afterwards:

1. TempDbStatsCollector.cs class summary — a genuine conflict, both sides rewrote it. Keep this PR's "applies to every SQL Server target" sentence and Measure tempdb space against its ceiling, not its current allocation (#2515) #2521's ceiling paragraph.
2. AppliesTo's doc comment merges cleanly and is then out of date. The unallocated_mb paragraph says the ratio "needs a size floor before it can safely carry the tempdb Space alert — filed as The tempdb Space alert's percentage has no size floor, and on Azure SQL Database that makes it fire on one temp table #2515". It got a denominator instead, and that paragraph should say so.
3. Lite.Tests/TempDbStatsCollectorDefinitionTests.AzureSqlDb_MeasuredValues_ComposeThroughToThePayload merges cleanly and then does not compile. It builds a five-column first result set and an eight-argument Row; Measure tempdb space against its ceiling, not its current allocation (#2515) #2521 appends max_size_mb to both, so it needs a sixth column and a ninth Row/writer value. Worth knowing before the merge rather than from a red build after it.

(Plus the usual CHANGELOG link-definition block — both branches add [#2512]: and [#2515]:; keep one copy of each.)

[claude]

Nice fix widening this catch to the shared predicate — but the XML doc summary directly above FetchFailedJobsAsync (around line 3465) is now stale: it still says "a login that cannot SELECT the msdb job tables raises SqlException 229/297/300/916". After this change the set is the full SqlServerPermissionErrors list (229/262/297/300/916/8189). Same drift in PerformanceMonitor.Alerting/FailedJobsQuery.cs's class doc, which says Darling's runner degrades "the same exception" as the Dashboard's literal 229/297/300/916 check — no longer true now that Darling routes through the wider shared set. Given this whole PR is about a doc comment's premise going unchecked, worth tidying these two along with it.

[claude]

CollectorTargetFault.cs's doc comment on the Permissions member still says "SQL Server: 229, 297, 300, 8189" — it was never updated to match the 6-number SqlServerPermissionErrors set this Classify method (and the two runner catches) now use, so it's missing 262 and 916. Small thing, but it's the canonical description of what this fault class means and is now inaccurate.

[claude]

Reviewed. This is a well-evidenced fix and the parity/derivation story checks out — verified:

• TempDbStatsCollector.AppliesTo and the composed CollectorCatalog.AppliesTo/by-name surface all flip together, and CollectorGateSurfacePinTests asserts both the flipped Azure SQL DB case and the unchanged MI case, plus the composed surface Lite/Darling actually dispatch through.
• CollectorEngineCapability.CapturePathByCollector correctly drops the tempdb_stats entry (and Lite.Tests/EngineCapabilityMissTests.cs deliberately avoids using tempdb as its example, for exactly the reason stated — good call).
• The DarlingWebEndpoints.BuildAppliesToNode composer map is genuinely derived from AppliesTo, not hand-transcribed, so the tempdb_* measures pick up azureSqlDb: true with no edit needed — confirmed by reading MeasureCatalog.cs/DarlingWebEndpoints.cs.
• SqlServerPermissionErrors is consumed identically by SqlServerTargetProvider.Classify, both DarlingWorker.cs catches, and both RemoteCollectorService.cs catches — no site was missed, and no other AzureDmvPermissionHint.For(...) call site in the tree was left on the old 2-arg behavior in a way that matters (the two in DatabaseSizeCollectorDefinitionTests.cs never exercise 262, so the new optional param doesn't affect them).
• The Lite/Darling MCP get_tempdb_trend fallback removal is mirrored in both McpTempDbTools.cs and DarlingMcpDataTools.cs.
• NamesTempDb's quoted-string match on ex.Message is a reasonable way to disambiguate a database-scoped 262 without a false-positive risk (tempdb is a reserved name, so no user database can produce 'tempdb' in that position).

Left two inline nits — both doc-comment drift, not runtime bugs: CollectorTargetFault.cs's "SQL Server: 229, 297, 300, 8189" doc for the Permissions fault is now missing 262/916, and DarlingWorker.cs's FetchFailedJobsAsync summary (plus FailedJobsQuery.cs's class doc) still describes the old, narrower number set. Slightly ironic given the PR's whole premise is "a comment's claim went unchecked," but low severity — none of these drive behavior, they just describe it inaccurately now.

No correctness, security, or Lite/Darling parity issues found beyond that.

[claude]

Review: #2516 (tempdb on Azure SQL Database)

Went through the diff against CONTRIBUTING.md's style rules and the Lite/Darling parity requirement. Overall this is a well-verified, well-tested change - the gate removal is backed by live measurements, the permission-number consolidation removes a real drift (916 was in the classifier but neither runner catch; the two failed-jobs reads and Lite's XE-session catch didn't have 8189/916/262 in various combinations), and Lite/Darling stay in lockstep across DarlingWorker.cs and RemoteCollectorService, SqlServerTargetProvider.Classify and Lite's catches, and both GetTempDbTrend MCP tools dropping the now-always-null NotCollectedStatusAsync fallback together.

One class of finding, not blocking: this PR's whole point is eliminating hand-maintained copies of the permission-error-number set (SqlServerPermissionErrors.cs's own doc says the set "already WAS six literals... drifted in three different directions"). Three doc comments elsewhere still spell out the old, narrower literal lists and are now stale relative to the new shared IsPermissionDenied set (229, 262, 297, 300, 916, 8189). None are on lines this PR touched, so I can't leave these inline, but worth a follow-up touch-up:

• PerformanceMonitor.Collectors/CollectorTargetFault.cs line 31: Permissions's doc still says "SQL Server: 229, 297, 300, 8189." (missing 262, 916). Since SqlServerTargetProvider.Classify now derives this from SqlServerPermissionErrors.IsPermissionDenied, this doc could just reference that method instead of repeating the list, so it can't drift again.
• Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs line 3465: FetchFailedJobsAsync's doc says the catch degrades on "SqlException 229/297/300/916"; the catch a few lines below it now filters on the wider shared set.
• PerformanceMonitor.Alerting/FailedJobsQuery.cs line 33: says the (deprecated) Dashboard and "the future Darling engine" both degrade on "SqlException 229/297/300/916". Still accurate for the deprecated Dashboard, but Darling is shipped now and uses the wider set, so this reads as "all three hosts agree" when they no longer do.

Nothing else stood out as a correctness, security, or parity problem - AzureDmvPermissionHint.For's new errorMessage parameter is threaded through both of its call sites (Darling and Lite), the 262/tempdb-name matching is quote-anchored so it can't false-positive on a user database merely containing "tempdb" in its name, and the new max_size_mb subquery's three-part tempdb.sys.database_files reference falls under the same permission-classification catch as the rest of the batch even though it wasn't separately called out in the live-verification measurements.