Index Analysis connects to the database it analyses on Azure - #2409
Conversation
Azure SQL Database has no cross-database execution, so the Utility DB idea -- install sp_IndexCleanup once and point it at any database on the server -- cannot work there. The proc runs inside whichever database the connection opened, and @database_name asks it to read a different one. @ZedZipDev found the shape by experiment: with Utility DB set to db1, analysing db1 works and analysing db2 reports no valid database. That is the proc's own message and it reads like the database is missing rather than unreachable, which is the part that cost him the time. On Azure the connection now targets the database being ANALYSED rather than the utility database. The proc has to be installed in each database regardless, so pointing at the target is the only shape that can work, and it is what he was already doing manually. Two supporting changes. All Databases is refused on Azure with an explanation rather than half-filling the grid from whichever database the connection happened to open -- enumerating every database is the same cross-database read. And the not-installed branch names the database it checked, because "not installed" against a server with fifty databases is not actionable without saying which one. The Utility DB tooltip says it is ignored on Azure and why, which is what he suggested. Fixes #2407 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var properties = _dataService == null | ||
| ? null | ||
| : await _dataService.GetLatestServerPropertiesAsync(GetSelectedServerId()); | ||
| var isAzureSqlDb = properties?.EngineEdition == 5; |
There was a problem hiding this comment.
Azure detection can silently fail open, reproducing the exact bug this PR fixes.
isAzureSqlDb is derived from _dataService.GetLatestServerPropertiesAsync(...), which reads the server_properties table. That collector is registered with FrequencyMinutes = 0 ("on-load only", Lite/Services/ScheduleManager.cs) and runs ~29th of ~35 collectors, strictly sequentially, inside RunAllCollectorsForServerAsync — which MainWindow.xaml.cs ConnectToServer kicks off after it has already added the server tab to ServerTabControl.Items and made it the selected item. RunIndexAnalysisButton has no gating on initial load completing (it's only disabled while an analysis is in flight), so a user can open the FinOps tab and click "Run Analysis" before server_properties has ever been collected for that server — e.g. right after AddServer(), which doesn't trigger any collection at all.
In that window properties is null, so isAzureSqlDb is false, and the code takes the pre-fix branch: _credentialResolver.GetUtilityConnectionString(server). On Azure SQL Database with a Utility DB configured, that's precisely the "Utility DB set to db1, analysing db2 reports no valid database" bug from #2407 — it just resurfaces on a timing window instead of unconditionally.
_serverManager.GetConnectionStatus(server.Id).SqlEngineEdition == 5 (already used the same way elsewhere, e.g. MainWindow.AlertEngine.cs:84/230, RemoteCollectorService.cs:634) is populated synchronously by the permission-free connectivity-check query and is available far earlier/more reliably — _serverManager is already a field on this class and is even null-checked at the top of this same method. That looks like the more robust signal to key this fix off of.
| var utilityConnectionString = isAzureSqlDb && !string.IsNullOrWhiteSpace(databaseNameEarly) | ||
| ? _credentialResolver.GetConnectionStringForDatabase(server, databaseNameEarly!) | ||
| : _credentialResolver.GetUtilityConnectionString(server); |
There was a problem hiding this comment.
When Azure is correctly detected but the database field is left blank and "All Databases" is unchecked (a reachable, unvalidated combination — nothing requires filling in a name or checking the box), this falls through to _credentialResolver.GetUtilityConnectionString(server), which still honors server.UtilityDatabase on Azure.
That contradicts the updated tooltip in AddServerDialog.xaml ("Ignored on Azure SQL Database … Index Analysis connects to the named database directly"): if a Utility DB is configured, the connection silently opens against that database rather than "the connection database" the tooltip implies, and the analysis runs against the wrong database with no indication to the user of which one was actually checked. Worth either forcing the plain connection string (ignoring UtilityDatabase) whenever isAzureSqlDb is true, or requiring a database name up front on Azure the same way "All Databases" is now refused.
| internal enum EndpointToggleOrigin | ||
| { | ||
| /// <summary>darling.json, because the worker has not published yet (still bootstrapping, or it never | ||
| /// reached the store). PROVISIONAL: the control plane can contradict it within one poll interval.</summary> | ||
| File, | ||
|
|
||
| /// <summary>The store's <c>config.config_service</c> row, published by the worker. Authoritative.</summary> | ||
| ControlPlane, | ||
| } | ||
|
|
||
| /// <summary>The effective (enabled, port) a supervisor acts on, WITH its provenance (#2389). | ||
| /// <see cref="EnabledOverridden"/> / <see cref="PortOverridden"/> are true only when the control plane | ||
| /// supplied a value that DIFFERS from darling.json's -- the reportable disagreement.</summary> | ||
| internal readonly record struct EndpointToggle( | ||
| bool Enabled, int Port, EndpointToggleOrigin Origin, bool EnabledOverridden, bool PortOverridden); | ||
|
|
||
| /// <summary> | ||
| /// PURE: the store row wins whenever the worker has published one -- byte-for-byte the old | ||
| /// <c>published?.Enabled ?? config.Mcp.Enabled</c> pair -- but it also reports WHERE the values came from | ||
| /// and whether they contradict the file, which a null-coalesce structurally cannot. | ||
| /// </summary> | ||
| internal static EndpointToggle ResolveEndpointToggle((bool Enabled, int Port)? published, bool fileEnabled, int filePort) | ||
| => published is null | ||
| ? new EndpointToggle(fileEnabled, filePort, EndpointToggleOrigin.File, false, false) | ||
| : new EndpointToggle( | ||
| published.Value.Enabled, | ||
| published.Value.Port, | ||
| EndpointToggleOrigin.ControlPlane, | ||
| EnabledOverridden: published.Value.Enabled != fileEnabled, | ||
| PortOverridden: published.Value.Port != filePort); | ||
|
|
||
| /// <summary> | ||
| /// PURE: the provenance clause the start line carries, so "Starting ... on http://..." says on whose | ||
| /// authority it is starting -- and admits when it is running on file values the control plane has not | ||
| /// weighed in on yet, which is the line the operator greps and stops reading. | ||
| /// </summary> | ||
| internal static string DescribeToggleOrigin(EndpointToggle toggle) | ||
| => toggle.Origin == EndpointToggleOrigin.ControlPlane | ||
| ? "the control plane (config.config_service)" | ||
| : "darling.json (PROVISIONAL - the control plane has not published yet and may stop or rebind this server)"; | ||
|
|
||
| /// <summary> | ||
| /// PURE: the one-line report of a control-plane override, or null when the two planes agree (or nothing is | ||
| /// published yet, in which case there is nothing to disagree with). <paramref name="section"/> is the | ||
| /// darling.json object name, the config_service column prefix, and the CLI verb suffix at once ("mcp" -> | ||
| /// mcp.enabled / config_service.mcp_enabled / --enable-mcp), which is what keeps the MCP and web wordings | ||
| /// from drifting apart. The file values are passed in rather than re-derived so the message quotes what the | ||
| /// caller actually loaded. | ||
| /// </summary> | ||
| internal static string? DescribeToggleOverride( | ||
| EndpointToggle toggle, string section, string surface, bool fileEnabled, int filePort) | ||
| { | ||
| if (toggle.Origin != EndpointToggleOrigin.ControlPlane || (!toggle.EnabledOverridden && !toggle.PortOverridden)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var fields = new List<string>(2); | ||
| if (toggle.EnabledOverridden) | ||
| { | ||
| fields.Add( | ||
| $"enabled is {(fileEnabled ? "true" : "false")} in darling.json ({section}.enabled) but " | ||
| + $"{(toggle.Enabled ? "true" : "false")} in config.config_service.{section}_enabled"); | ||
| } | ||
|
|
||
| if (toggle.PortOverridden) | ||
| { | ||
| fields.Add($"port is {filePort} in darling.json ({section}.port) but {toggle.Port} in config.config_service.{section}_port"); | ||
| } | ||
|
|
||
| return $"{surface} configuration disagrees across the two planes and the CONTROL PLANE WINS: " | ||
| + string.Join("; ", fields) | ||
| + $". After the first run darling.json's {section}.enabled/{section}.port are only the SEED -- change them with " | ||
| + $"--enable-{section}/--disable-{section} or the Viewer's Settings, or the file values will keep being ignored. " | ||
| + $"The {section}.network block is the OPPOSITE: file-only, restart-only, no store equivalent -- so an exposure " | ||
| + "block in darling.json is live even while the control plane keeps this endpoint off."; | ||
| } | ||
| } |
There was a problem hiding this comment.
Test-coverage gap: ResolveEndpointToggle, DescribeToggleOrigin, and DescribeToggleOverride are all marked PURE in their doc comments and have no unit tests — I couldn't find any references to them under Darling/Darling.Tests. This codebase otherwise tests exactly this shape of pure helper directly (e.g. DarlingEndpointToggleCliTests.cs for ClassifyFirewallPlan/ClassifyAllowFrom, CredentialProfileTests.cs for the Lite Resolve*ConnectionString siblings). Worth pinning at least: ResolveEndpointToggle with published = null vs. a value that agrees/disagrees with the file (checking EnabledOverridden/PortOverridden independently), and DescribeToggleOverride returning null when they agree vs. the exact wording when only one of the two fields disagrees.
Review summaryThis diff bundles three distinct changes: the Azure Index Analysis connection-targeting fix (#2407, Lite), an MCP Correctness (left as inline comments):
Minor: the new pure helpers in Good things noted, no action needed:
|
|
Heads-up on a lane collision, not a review note — this PR is carrying two files that belong to #2394, and they are mine rather than yours.
It is now filed properly as #2410 off Your call how to handle it. Two options that both work:
The only outcome to avoid is both merging with the files in each, which would conflict on Separately, and only because you may want to check the same way: |
|
Heads up —
Those are #2389 (the MCP enabled/network authority split), and they were uncommitted work sitting in the shared checkout when this branch was created — a Your call how to handle it, but the cheap fix is to drop them from this branch: (An amend is safe here — no review has anchored on this diff yet. A plain follow-up commit reverting the three works too if you'd rather not rewrite.) #2411 has the same three files plus their tests, so if this merges first that PR will conflict on them; if you drop them here, #2411 stays clean either way. I'm not touching this branch — flagging it so whichever of us merges second doesn't get surprised. Unrelated to this, |
This branch picked up in-progress work from three other lines of work because the commit that created it staged everything in a shared checkout rather than the paths it had actually edited: the MCP plane-authority resolver (#2389), Lite's MCP alert-settings parity (#2394), and Lite's analysis re-notify cooldown box (#2393, since merged). None of it belonged here. All eight files are restored to their dev state, leaving this branch as the four files #2407 actually touches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Corrected: this branch had picked up in-progress work from three other lines — the MCP plane-authority resolver (#2389/#2411), Lite's MCP alert-settings parity (#2394), and Lite's analysis re-notify cooldown box (#2393, since merged). Entirely my fault, and worth naming the cause rather than just the effect: the commit that created this branch ran All eight files are restored to their dev state and dev is merged in. This PR is now the four files #2407 actually touches: #2411 and the #2394 branch are unaffected — they hold the authoritative copies of what was swept up here, and neither needs to wait on this. Thanks to the #2389 agent for catching it; it would otherwise have merged three half-finished changes under an unrelated title. |
| ? null | ||
| : await _dataService.GetLatestServerPropertiesAsync(GetSelectedServerId()); | ||
| var isAzureSqlDb = properties?.EngineEdition == 5; |
There was a problem hiding this comment.
Azure detection here reads _dataService.GetLatestServerPropertiesAsync(serverId), i.e. the last stored server_properties snapshot in local DuckDB storage. That's a different, weaker signal than the one the PR description says it mirrors.
MainWindow.AlertEngine.cs (and RemoteCollectorService) detect Azure from _serverManager.GetConnectionStatus(server.Id).SqlEngineEdition == 5 — the live connection-status field populated by the lightweight connectivity check, independent of any collection cycle.
server_properties is an "on-load only" collector (Lite/Services/ScheduleManager.cs, FrequencyMinutes = 0). If it hasn't run yet for this server (freshly added server, in-flight/failed on-load collection, or the collector disabled in settings), properties is null here, isAzureSqlDb silently defaults to false, and the code falls through to GetUtilityConnectionString(server) — reproducing the exact cross-database failure from #2407 for the population most likely to hit it (a server just added and not yet fully collected).
Consider using _serverManager.GetConnectionStatus(server.Id).SqlEngineEdition == 5 instead, matching the existing pattern and avoiding the dependency on a prior successful collection cycle.
| if (isAzureSqlDb && allDatabasesEarly) | ||
| { | ||
| /* Enumerating every database from one connection is the same cross-database read, so All | ||
| Databases cannot work on Azure either — and failing per-database would half-fill the grid | ||
| with whichever database the connection happened to open. */ | ||
| IndexAnalysisStatusText.Text = | ||
| "Azure SQL Database cannot analyse across databases — clear \u201CAll Databases\u201D and name one, " | ||
| + "with sp_IndexCleanup installed in it."; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Minor: this early-return path only sets IndexAnalysisStatusText.Text and returns — it doesn't clear IndexAnalysisNotInstalledMessage/IndexAnalysisNoDataMessage or the summary/detail grids the way the "not installed" branch below does. If a user already has analysis results on screen from a prior run, checks "All Databases" and re-runs on Azure, the old grid data stays visible underneath the new error message, which can read as if the error applies to stale-but-still-shown results rather than replacing them.
|
Reviewed the diff (net change: Summary: the fix itself is sound — routing the connection at the target database for Azure, refusing "All Databases" there, and naming the database in the not-installed message all directly address the #2407 report. Parity: checked Darling's Index Analysis ( Findings posted inline:
|
1 participant
@ZedZipDev on #2407, with 50 Azure SQL databases: set Utility DB to db1, Index Analysis for db1 works, Index Analysis for db2 reports no valid database.
His diagnosis is right. Azure SQL Database has no cross-database execution, so the Utility DB idea — install
sp_IndexCleanuponce and point it at any database on the server — cannot work there at all. The proc runs inside whichever database the connection opened;@database_nameasks it to read a different one; Azure refuses. The message he saw is the proc's own, and it reads like the database is missing rather than unreachable, which is the part that cost him the time.On Azure the connection now targets the database being analysed rather than the utility database. The proc must be installed in each database regardless — which is exactly what he found by experiment — so pointing at the target is the only shape that can work. On every other engine the Utility DB behaviour is unchanged.
Two supporting changes:
All Databases is refused on Azure with an explanation, rather than half-filling the grid from whichever database the connection happened to open. Enumerating every database is the same cross-database read, so it cannot work either.
The not-installed branch names the database it checked. "sp_IndexCleanup is not installed" against a server with fifty databases is not actionable without saying which one — and on Azure the answer is now per-database rather than per-server.
The Utility DB tooltip says it is ignored on Azure and why, which is what he suggested.
Azure is detected from the collected
engine_edition(5), the same signalMainWindow.AlertEngineand the Query Store backfill already use — no extra round trip.