Scope an Azure SQL DB sweep to the database it registered (#2220)

[erikdarlingdata]

Fixes #2220.

The report was right, and the reporter found the mechanism before I did

One real deadlock in one Azure SQL Database produced near-identical alerts on every other monitored database on the same logical server. Azure SQL DB engines are isolated per database, so one database's sessions cannot block another's — and the stored data bore that out: byte-identical deadlock graphs and the same top query, counters within ~1%, under six unrelated server_ids. Not cross-talk. The same rows, collected six times.

Confirmed from source. GetAzureDatabaseListAsync → BuildDatabaseListPlan hops to master:

SELECT name FROM sys.databases WHERE state_desc = N'ONLINE' AND database_id > 0 {exclusion} ORDER BY name;

That returns every online database on the logical server, narrowed only by that registration's ExcludedDatabases — a denylist, not an allowlist. The caller sweeps all of them and stores every row under the one server_id of whichever registration ran the sweep. The single-database behaviour existed only as the fallback for a master-access error, so the safe path was the exceptional one.

On a logical server holding N separately-registered databases: N registrations × N databases = N² collection, with every registration's history contaminated by its siblings'. This is a data-correctness bug, not alert noise.

Why it happened, which is the part worth keeping

Two parts of the product hold incompatible ideas of what an Azure SQL DB registration is, and both are deliberate:

• the enumeration assumes one registration = one logical server — the shape [FEATURE] Review/lower required permission for the Azure server to user database level(no master) #857 was written for;
• identity assumes one registration = one database: server_id hashes host[:database][:RO], so registering two databases on one server is the supported way to get two identities, and the Azure query_store path needs a per-database connection anyway (Query Store collector never collects on Azure SQL Database #1836).

Nothing reconciled them, so the second shape silently behaved like the first.

The rule

A registration that names a database is a registration of that database, and sweeps exactly that one. Only a registration naming none — or naming master, where a catalog-less Azure connection lands — is a registration of the server, and only that one enumerates.

This also subsumes #857's motivating case and improves on it: a login granted access to one user database but not to master has a named database, so it now returns without probing master at all, rather than probing, failing, forming a verdict and falling back.

Extracted to shared code rather than fixed twice

Both runners carried their own private copy of this predicate. A sweep-scoping rule that disagrees between Lite and Darling is the same class of defect as the one being fixed, so it now lives in AzureSweepScope and both suites pin it.

Deliberately not done here

The master-probe throttle (_azureMasterInaccessibleSince / IsMasterProbeThrottled / OnServerReconnected) is now unreachable for any registration that names a database — which is #857's whole population. Removing that machinery is a separate change from a correctness fix a reporter is waiting on; its guard is left stating the precondition this path now satisfies by construction. Happy to do the cleanup as a follow-up.

Verification

Rule checked against compiled code (12/12 in a scratch harness, since net10.0-windows xunit will not run on this machine), including the over-match cases — mastermind, paymaster, master_archive are real databases and are still swept — and that each call returns its own list, since both runners hand it straight to their per-database loop. Full solution builds clean, 0 warnings.

What this does not fix: existing stored data. Any store that has been collecting Azure SQL DB siblings already holds rows under the wrong server_id, and this change stops the contamination rather than unwinding it. Worth deciding separately whether that needs a cleanup path or just a note.

🤖 Generated with Claude Code

[claude]

Reviewed. The core fix in AzureSweepScope (a named-database registration sweeps only that database, never master) is correct, well-tested, and properly shared between Lite and Darling — good use of a single implementation pinned by both suites rather than two copies drifting again.

One correctness regression found in both GetAzureDatabaseListAsync implementations (left as inline comments on each): the throttle-skip for the "no target database" branch got dropped in the refactor. Previously, when a registration has no fallback database (bare connection string or explicit master), the code deliberately bypassed IsMasterProbeThrottled and re-probed master on every cycle, because with no fallback the throttled path can only throw anyway — so skipping it maximized the chance of detecting recovery quickly. The new code checks IsMasterProbeThrottled unconditionally in that branch, so once master access fails once, every cycle for up to 15 minutes now short-circuits straight into a throw via FallbackDatabaseList without ever attempting to reconnect — reintroducing the exact "guaranteed failure without attempting to recover" problem the deleted comment (and the new comment sitting right above the code!) explicitly warns against. Not covered by the new AzureSweepScopeTests (those only test the static predicate, not this method), and no existing test calls GetAzureDatabaseListAsync end-to-end for this path, so it wasn't caught.

Also flagged: the docstrings above both GetAzureDatabaseListAsync methods still describe the pre-fix "always try master first" behavior and should be updated to match the new named-database short-circuit.

No security, SQL injection, or missing-index concerns — parametrized queries are unchanged, and the exclusion filter logic wasn't touched.

[erikdarlingdata]

Both confirmed and fixed in c4c844a0 — and this was a regression I introduced, not a nit, so thank you for catching it.

I read hasFallback && as a redundant condition and dropped it. It was the opposite: in that branch hasFallback is false, so the guard existed to disable the throttle — precisely what the comment I deleted said. "Honouring the throttle would guarantee 15 minutes of failure without ever attempting to recover."

Left unconditional, and with the branch now reachable only when the registration names no database, one master failure would send every subsequent cycle straight into FallbackDatabaseList — which throws immediately without probing — for the whole recheck interval, where it used to retry every cycle. Recovery detection delayed by 15 minutes. Exactly as you described, in both hosts.

Removed the check from that path in both, restoring the old effective behaviour, with the reasoning written where the old comment was so it does not get re-added by the next person who reads the condition the way I did.

One thing I did not do, deliberately. The obvious follow-through is to delete the throttle as newly-dead code, and I checked before reaching for it: Lite's IsMasterProbeThrottled is directly tested by AzureMasterFallbackTests — six assertions carrying #857/#1506 behaviour — so retiring it is its own change with those tests, not a rider on a correctness fix.

Worth recording why it is obsolete now, since it is a different reason than this fix: the throttle's entire purpose was to stop re-probing master for a registration that has a fallback, and after this PR such a registration never probes master at all. So it is unreachable in production either way — but that is a deletion to argue on its own merits.

[claude]

Review summary (high effort)

This is a well-scoped, well-justified fix, and the PR already caught and corrected its own regression in the second commit (the hasFallback && drop). I traced both GetAzureDatabaseListAsync implementations end-to-end against AzureSweepScope, checked Lite/Darling parity, and looked for security and performance regressions.

Correctness / parity: No functional bugs found. The new early-return (AzureSweepScope.OwnDatabaseOrEmpty(targetDb).Count > 0 → return immediately) correctly bypasses master for any database-named registration, matches between Lite and Darling line-for-line (including the restored no-throttle branch from the second commit), and the shared AzureSweepScope type + its mirrored test files in Darling.Tests and Lite.Tests keep the two apps' scoping rule from drifting again. Postgres's GetPostgresDatabaseListAsync is correctly left untouched — it never shared this predicate. This is a genuine performance win too: named-database registrations no longer make a per-cycle round trip to master.

What I did flag (inline, all low/medium severity, no functional-bug findings): a few spots where the fix's code changed but adjacent documentation or a newly-added helper didn't keep up, which is a little ironic given the PR's own postmortem is about a doc/code mental-model mismatch:

• Both GetAzureDatabaseListAsync XML-doc summaries (Darling and Lite) still describe the pre-[BUG] Blocking/Deadlock alerts cross-talk between Azure SQL databases sharing the same logical server (Darling) #2220 "try master first, throttle re-probes" strategy that the fix replaces for the common (named-database) case.
• AzureSweepScope.IsScopedToOneDatabase is unused in production — both call sites inline ownDatabase.Count > 0 instead of calling the predicate whose own doc comment says it exists specifically so "a call site reads as the rule rather than as a length check."
• FallbackDatabaseList's quiet parameter (both apps) is now always false at its only call site, since the second commit removed the throttled branch that used to pass quiet: true — the <param name="quiet"> doc and the if (quiet) branch describe a path that no longer exists.

No SQL injection, secrets, or missing-index concerns; not raising folklore missing-index feedback per instructions.

[erikdarlingdata]

All five addressed in 45773906 — two fixed, one accepted as follow-up with your reasoning.

IsScopedToOneDatabase was dead surface, and you caught the irony: I wrote a named predicate arguing a call site should read as the rule rather than as a length check, then wrote both call sites as ownDatabase.Count > 0 anyway. The reason is structural — both callers need the list to return, so the boolean twin would mean calling twice — so I dropped it rather than contorting the callers to justify it. The tests that existed only for it went with it.

Both stale summaries rewritten. You're right that they re-teach the pre-fix model: "try master enumeration first … and on a master-access error fall back … throttling re-probes per server" is now the exceptional path, not the default. Both now lead with the rule that governs, and Lite's also records that this covers #857's own case better than #857 did — a login with access to one database but not master no longer probes, fails and falls back; it simply never probes.

FallbackDatabaseList's quiet parameter — agreed, and taking it as follow-up on your own reasoning. You're right it's now dead in both hosts (one caller, always the default), and right that it's the same now-unreachable machinery as IsMasterProbeThrottled. They should retire together, alongside the AzureMasterFallbackTests assertions that pin the throttle — six of them carrying #857/#1506 behaviour. Bundling a tested-behaviour deletion into a correctness fix a reporter is waiting on is how the fix gets held up arguing about the deletion.

Full solution still builds clean, 0 warnings.

[claude]

Reviewed the diff (AzureSweepScope.cs, DarlingCollectorRunner.cs, RemoteCollectorService.cs, and the two AzureSweepScopeTests.cs files).

Correctness: the fix itself checks out. AzureSweepScope.OwnDatabaseOrEmpty correctly treats null/empty/master (case-insensitively) as "no database named" and everything else — including database names that merely contain "master" — as a named database, matching the stated rule. Both runners now short-circuit to the single-database list before ever touching master when a database is named, which is exactly what closes the N² sweep described in the PR body; the no-database/logical-server branch is unchanged (it always probed master unconditionally before this PR too, since hasFallback was always false there — so no behavior regression on that path). Traced RunsPerDatabase → target.IsAzureSqlDb to confirm this code path is unreachable for on-prem SQL Server, so there's no risk of accidentally scoping a non-Azure multi-database sweep down to one database.

Lite/Darling parity: good — the predicate was genuinely de-duplicated into the shared AzureSweepScope, both runners' SingleDbOrEmpty now delegate to it, and both suites pin the identical test file. No divergence between the two apps in the functional change.

Two minor nits, left as inline comments where GitHub would let me anchor them:

1. Lite.Tests/AzureSweepScopeTests.cs line 32 — a sentence lost its "between Lite" clause when customized from the Darling version, leaving broken grammar ("A rule that disagrees and Darling is the same class of defect").
2. Not inline-able (outside the diff hunk), so noting here: the field-level doc comments on _azureMasterInaccessibleSince in both Lite/Services/RemoteCollectorService.cs (~line 170-180) and Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs (~line 73-79) are now stale in both apps. They describe the timestamp as existing "so database-scoped collectors fall back to the connection's own catalog instead of re-probing master every cycle," but after this change that's no longer true for the common (named-database) case — GetAzureDatabaseListAsync returns via AzureSweepScope before the throttle is ever consulted. The much better explanation you already wrote lives 700+ lines away in GetAzureDatabaseListAsync's own comment; worth a one-line pointer from the field declaration so a reader who only sees the field summary doesn't come away thinking the throttle still gates the common path.

No security, missing-index, or performance concerns beyond what's already discussed above (and the change is a net performance win — it removes a master round-trip per cycle for every named-database registration).

[erikdarlingdata]

Fixed in aac1c26c — and the irony is earned. The sed I used to customize the Darling header for Lite ate two words. In a PR whose whole point is that Lite and Darling must not disagree, that is the sentence to get right.

[claude]

Reviewed the diff (AzureSweepScope.cs, both GetAzureDatabaseListAsync runners, both AzureSweepScopeTests.cs). This is a clean, well-scoped fix — a few notes:

Correctness / scope check (passed) — GetAzureDatabaseListAsync is only reached when RunsPerDatabase(target) is true, and every collector implementing that returns target.IsAzureSqlDb (DeadlocksCollector, QueryStoreCollector, QueryStatsCollector, IndexObjectStatsCollector, FileIoStatsCollector, BlockedProcessReportCollector, LongQueryCompletionsCollector, ProcedureStatsCollector, PlanCorrectionCollector). So this change is genuinely Azure-SQL-DB-only and can't regress on-prem/multi-database SQL Server sweeps or PostgreSQL (which has its own GetPostgresDatabaseListAsync path in Darling, and no per-database engine at all in Lite). Good.

Throttle removal reasoning checks out — I traced the old hasFallback && IsMasterProbeThrottled(...) guard by hand: hasFallback was only ever true when the registration named a database, and that's exactly the branch that now returns before ever reaching the throttle check. The "no database named" branch never consulted the throttle before either (short-circuited by &&). So IsMasterProbeThrottled/_azureMasterInaccessibleSince really is fully unreachable from production in both apps now — matches what the PR body claims, not just for the named-database case but for both. Worth double-checking that framing in a follow-up if the throttle retirement happens later, since the comments say "unreachable for any registration that names a database" as if the other branch still uses it, but it doesn't.

Parity — AzureSweepScope.OwnDatabaseOrEmpty is shared correctly via PerformanceMonitor.Collectors (both runners already had the using), both SingleDbOrEmpty wrappers now delegate to it identically, and both test suites pin identical cases. No drift found between the Lite and Darling changes.

One gap: no CHANGELOG.md entry. Given this fixes a real data-correctness bug (N² collection contaminating stored history across registrations on the same logical server) and the project's changelog otherwise documents fixes at this level of detail — including "what this does not fix" caveats exactly like the one in this PR's description — it seems worth adding one, particularly the note that existing contaminated data isn't cleaned up by this change. That's operator-facing information that won't be visible to anyone reading only the git history.

No functional bugs, security issues, or performance regressions found. The deliberate deferral of retiring the throttle machinery is reasonable — it's inert now, not actively wrong, and bundling its removal here would mix an orthogonal cleanup into a correctness fix.