Skip to content

Fix drill-down database switching and stale database-check races - #602

Merged
erikdarlingdata merged 7 commits into
devfrom
fix/drilldown-database
Sep 28, 2026
Merged

erikdarlingdata merged 7 commits into
devfrom
fix/drilldown-database

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

What does this PR do?

This PR fixes four bugs in how a query session keeps its toolbar database picker, its Query Store grids, and its Overview in step. Each fix has its own commit and its own headless tests.

Drill-down from the Overview did not switch the toolbar database (E1)

Drilling down from the Overview into a database opened a Query Store tab for it. The handler also wrote the database into _selectedDatabase and _connectionString directly. It never changed DatabaseBox. The toolbar picker kept naming the old database while the session ran queries in the new one.

The handler now selects the database in the picker, using the same match that the connect code uses. That runs Database_SelectionChanged exactly as a user's own pick does, which sets both fields and refreshes the metadata. The two direct assignments are deleted. If the picker does not list the database, the session stays on its current database. The Query Store tab still opens.

A plan opened from a Query Store grid now remembers that grid's database. Each grid has its own database picker, and it can differ from the toolbar's. Get Actual Plan runs such a plan in the grid's database, and the confirmation text names it, for example "The query will execute in [DbName] with SET STATISTICS XML ON". Every other plan tab keeps using the toolbar database, including plans opened from History. The plan XML's own database context is not used. The tab that Get Actual Plan creates keeps the same source database, so a second run stays in the same place.

Overlapping Query Store enabled checks on the grid picker (E6)

If the user picked two databases in the grid's picker before the first CheckEnabledAsync returned, both checks ran to the end. The one that finished last won, even when it belonged to a database the user had already left. A new pick now cancels the previous check. The older check returns without changing the grid, so only the newest pick can apply its result. The early return for re-selecting the current database stays ahead of the cancel, because the picker's own revert fires the handler again.

Overlapping database metadata fetches on the toolbar picker (E7)

FetchDatabaseMetadataAsync had the same problem. Every pick starts a fetch, and nothing stopped an older fetch from landing after a newer one. The older fetch then overwrote _serverMetadata.Database with the wrong database's rows. A new pick now cancels the older fetch, and the older fetch drops its result. The token also goes down to ServerMetadataService.FetchDatabaseMetadataAsync, which already accepted one.

A reconnect had the same gap. A fetch that started before the reconnect used the previous server's connection string. If it landed after FetchServerMetadataAsync replaced _serverMetadata, it wrote the old server's rows into the new server's metadata. The connect block now cancels the pending fetch at the point where the connection changes.

The Overview stayed empty after a tab switch (E3)

Switching to another top-level session tab detaches the whole session from the visual tree. The Overview's detach handler cancels its load, and nothing restarted the load on the way back. A view that the user left mid-load stayed empty for the rest of the session.

The detach handler now records whether a load or refresh was running. The next attach restarts the load. LoadAsync keeps the time range that the user chose, so the reload uses the same range. A load that had already finished, failed, or been cancelled some other way is not repeated.

One premise in the original report did not hold. Switching between the Editor, Overview, and Documents segments does not cancel anything. ApplySurface only toggles IsVisible, so that switch hides the Overview and never detaches it. The fix targets the top-level tab switch, which is the only path that detaches it.

The control tracks running work as the token source of the current load or refresh, not as a flag. Loading the slicer raises RangeChanged. Its handler cancels the load and runs the slow metrics phase on a new token. The cancelled load reaches its finally while that phase is only starting. A flag cleared there reads "idle" during the phase where a tab switch is most likely. Each run now clears the claim only if the claim is still its own.

The E6 and E7 tokens are cancelled, not disposed

The first versions of the E6 and E7 fixes disposed the older token source when a newer pick replaced it. That was a mistake. The older check is still awaiting, and it reads its own token again when it wakes. CancellationTokenSource.Token throws ObjectDisposedException on a disposed source. In the grid, that read also sits inside the catch block. An exception there escapes an async void handler.

Commit 79813c5 removes the Dispose() calls, which matches the rule that the Overview already follows. The E6 and E7 tests now read Token on the superseded source, and they fail against the disposing code.

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

How was this tested?

The tests build sessions and grids against a pretend server at 127.0.0.1 port 1, where nothing listens, so no test can reach a real server. No plan files are involved. I tested on Windows only. The new tests use no Windows-only code.

  • Nine new headless tests: DrillDownDatabaseTests (3), QueryStoreDatabaseCheckRaceTests (2), DatabaseMetadataFetchRaceTests (1), and OverviewReattachReloadTests (3).
  • Two of the DrillDownDatabaseTests cover the drill-down. The third checks which database and connection string Get Actual Plan resolves for a grid plan and for a toolbar plan. It does not execute a query.
  • I broke the E3 fix three ways to prove the tests catch it. Clearing the claim unconditionally failed the refresh test. Using _cts != null as the "running" check failed the not-mid-load test. Removing the reload on attach failed the other two tests.
  • The E6 and E7 tests failed against the disposing code before the fix in 79813c5.
  • dotnet test passes at the last commit: 1047 tests, 1045 passed, 0 failed, 2 skipped. The two skipped tests only run off Windows.
  • dotnet build gives 0 warnings and 0 errors in both Debug and Release.

No headless test covers the reconnect cancel in the E7 section. The connect block sits behind the connection dialog. The test harness notes the same limit for InvalidateOverview.

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (dotnet build -c Debug)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza

erikdarlingdata and others added 6 commits September 28, 2026 16:15
The Overview's DrillDownRequested handler set _selectedDatabase and
_connectionString directly, so Execute and Get Actual Plan ran in the
drilled database while DatabaseBox kept showing the old one. Route the
drill-down through the picker instead (TrySelectDrilledDatabase), so
Database_SelectionChanged does the real work exactly as a user's own
pick would; a database missing from the picker leaves the session
alone and still opens the Query Store tab.

Also: a plan opened from a Query Store grid now remembers that grid's
own database (independent of the toolbar's), and Get Actual Plan runs
such a plan back against it via a new ResolveExecutionTarget helper,
instead of silently switching it to the toolbar's database. The
confirmation dialog now names the database the query will run in.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
…ker (E6)

QsDatabase_SelectionChanged had no way to tell a superseded check from
the newest one, so picking a second database before the first one's
CheckEnabledAsync landed let whichever finished last write _database/
_connectionString, even for a database the user had already clicked
past. Cancel the previous check's token when a new pick starts, thread
it into CheckEnabledAsync, and bail out silently if this call turns
out to be the one that got cancelled — same shape as the Overview's
load-generation guard.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
FetchDatabaseMetadataAsync had no way to tell a superseded fetch from
the newest one, so picking a second database before the first fetch
landed let whichever finished last overwrite _serverMetadata.Database,
even for a database already clicked past. Same fix as E6: cancel the
previous fetch's token when a new pick starts, thread it into
ServerMetadataService.FetchDatabaseMetadataAsync, and bail out
silently if this call turns out to be the one that got cancelled.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
Switching to another top-level session tab detaches the whole session
from the visual tree, and the Overview's detach handler cancels its
load. Nothing restarted it on the way back, so a view switched away
from mid-fetch stayed empty for the rest of the session.

The detach now records whether work was actually running, and the next
attach restarts the load (LoadAsync keeps the user's time range). A
load that had already finished, failed, or was cancelled some other
way is not repeated.

"Running" is tracked as the token source of the current load or
refresh, cleared only by the run that owns it, not as a flag: loading
the slicer raises RangeChanged, whose handler cancels the load and runs
the slow metrics phase on a token of its own, so the cancelled load
reaches its finally while that phase is only starting. A flag cleared
there would read "idle" for exactly the window a switch is likeliest.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
…e (E6, E7)

The E6 and E7 fixes disposed the older token source when a newer pick
replaced it. The older check or fetch is still awaiting and reads its
own token again when it wakes, and CancellationTokenSource.Token throws
ObjectDisposedException once the source is disposed. In the Query Store
grid that read sits in a catch block too, so the exception would escape
an async void handler.

Cancel the older source and leave it alone, which is the rule the
Overview already follows. The tests now read Token on the superseded
source and fail if it was disposed.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
…(E7)

A fetch started before a reconnect was built from the previous server's
connection string. If it landed after FetchServerMetadataAsync replaced
_serverMetadata, it wrote the old server's database rows into the new
server's metadata. Cancel it where the connection changes. The fetch
the connect block makes itself is the newest one and owns the result.

No headless test drives this line: the connect block sits behind the
connection dialog, the same limit the harness notes for
InvalidateOverview.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 28, 2026 21:00
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Review: no blocking issues found. The drill-down now goes through the picker, which is correct, and the execution target resolves via GetConnectionString. The database name in the confirmation dialog is display-only. Tests cover both paths.

Two low-severity notes on QuerySessionControl.Execution.cs / Plans.cs:

  1. GetActualPlan_Click still returns early when _selectedDatabase == null, even though ResolveExecutionTarget can now use the viewer's SourceDatabase. A Query Store plan therefore can't run Get Actual Plan if the toolbar has no database selected. Dropping that check and relying on the connectionString == null check below it would fix this.
  2. AddPlanTab still sets viewer.ConnectionString = _connectionString and SetConnectionStatus(..., _selectedDatabase) for plans that have a SourceDatabase. Metadata lookups and the status label will point at the toolbar's database, which is inconsistent with the new execution target.

🤖 Generated with Claude Code

A plan opened from a Query Store grid kept the toolbar's connection
string and status label, so Show Indexes and Show Table Definition looked
in the toolbar's database. ConnectViewer now sets the source database,
connection string and label together, for AddPlanTab and for the tab a
Get Actual Plan capture lands in.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed the diff. I found nothing blocking.

  • The CancellationToken overloads used for FetchDatabaseMetadataAsync and CheckEnabledAsync exist.
  • The cancel-without-dispose pattern is consistent across the new race guards.
  • ResolveExecutionTarget and TrySelectDrilledDatabase are covered by tests.
  • The PR adds no generated SQL, no version bump and no new NoWarn, so none of the T-SQL, version or warning conventions apply.

Minor: the Get Actual Plan confirmation now interpolates [{database}] into dialog text. That is display only, so it is safe. A database name containing ] will show unescaped, which is cosmetic.

@erikdarlingdata
erikdarlingdata merged commit 20bb9d9 into dev Sep 28, 2026
3 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/drilldown-database branch September 28, 2026 21:12
@erikdarlingdata erikdarlingdata mentioned this pull request Sep 29, 2026
2 of 8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant