Skip to content

Release plans and cancel running work when a tab closes - #606

Merged
erikdarlingdata merged 4 commits into
devfrom
fix/tab-close-cleanup
Sep 28, 2026
Merged

erikdarlingdata merged 4 commits into
devfrom
fix/tab-close-cleanup

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

What does this PR do?

This PR fixes three review findings about closing tabs in the desktop app. It has one commit per fix, plus a fourth commit for a related Escape case that you can drop on its own. Two premises in the review were partly wrong, and the last section of this list says where.

E2: a closed tab left its plan registered

Every plan viewer registers its plan with PlanSessionManager, and only PlanViewerControl.Clear() removed it. The plan then stayed in memory, and the MCP list_plans tool kept listing it, until the app exited.

  • Every window-level close ends in TryCloseTabAsync: the close button, middle-click, Ctrl+W, and the context menu's Close, Close Other Tabs and Close All Tabs. It now releases the content of the tab it removed. A plan tab clears its viewer. A query session releases each document it holds.
  • Closing a detached window for good does the same. Detach and re-dock do not, because the content is moving and is still on screen. Shutdown does not either, because the process is about to exit.
  • The release reads the tab content at close time. A tab whose spinner was replaced by a plan is released as the plan viewer.

E4: closing a tab cancelled nothing

  • A Query Store grid now has CancelFetch. It cancels the fetch and the database check. It also refuses to start the first fetch that the constructor posted, if the tab is already gone. It cancels and never disposes, because OnWaitStatsCollapsedChanged reads _fetchCts.Token when it runs, and Token on a disposed source throws. The History document disposes its source. The grid must not, and the code comment says why.
  • ReleaseDocument now says what each document kind gives up. CloseDocument and the close button from CreateSubTab both call it, so History's own onClose is gone. AddQueryStoreDocument replaces two copies of the same ten lines.
  • A loading tab remembers its run. Closing the tab cancels the run. Closing a whole query session also cancels its current run and its database metadata fetch.
  • The window-level Actual Plan tab does the same through AddLoadingTab.
  • A run can finish after it was cancelled, with its tab already closed. That run no longer builds a viewer. A viewer built for a closed tab registers a plan that nothing unregisters.
  • RemoveDocument ignores a tab that is not in the strip. A cancelled run removes its tab a second time after the close did.

E8: Escape on one tab cancelled another tab's run

  • Each run now takes its own source from BeginRun, and the loading tab's Cancel button and Escape handler close over it. BeginRun no longer disposes the source it replaces. Those handlers can fire after a newer run starts, and Cancel on a disposed source throws. This is the existing "cancelled, never disposed" rule.
  • The session's own Escape handler cancelled the current run for every Escape it heard. It now cancels only when the editor is showing or when the showing tab owns the current run. This is a judgment call, so it is a separate commit. Escape in the editor and on the loading tab work as before.

Where the review was wrong or incomplete

  • E2 named the plan sub-tabs in a query session. Those already released their plan, because every sub-tab close door goes through CloseDocument, which calls Clear(). The leaks were the window-level closes, closing a whole session, and closing a detached window.
  • E8 named MainWindow.PlanViewer.cs. It already closes over a local source, so it needed no change.
  • The brief warned about a viewer whose Clear() also runs on reload. No path does that. LoadPlan does not call Clear(), and every load goes into a new viewer. Unregistering an id that is already gone does nothing, so a second release is harmless.

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 new class is TabCloseCleanupTests, with 17 headless tests. They need no server. The manager is shared by the whole process. So each plan test reads the session id of the viewer it opened and asks the manager about that id alone. Tabs are closed through their own close buttons, so the real close paths run.

  • E2: closing a plan tab, closing a session with two plans, closing a detached plan window, and closing a tab whose spinner became a plan. Two more tests pin what worked before: closing one plan in a session, and detach then re-dock keeping the plan listed.
  • E4: closing a session cancels its running capture. Closing a loading tab cancels its run. Closing a Query Store grid cancels its fetch and check and does not dispose them. Closing a History document still cancels its fetch. Closing the window-level loading tab cancels its run.
  • E8: Escape and the Cancel button on an older tab do not cancel a newer run. Escape on a finished plan does not cancel a running capture. Escape on a loading tab, in the editor, and elsewhere while the loading tab is showing still cancel it.

I switched each fix off and confirmed that its tests fail. With the releases and the per-run handlers off, 13 of the 14 tests then in the class failed. The one that passed checks that Escape on a loading tab still cancels its own run. With the session Escape check off, its one test failed.

Full suite, run once at the end: total 1143, failed 0, succeeded 1113, skipped 30. Of the 30 skipped tests, 28 are macOS keychain tests. The other 2 pin contracts for setups this run does not have. The build has 0 warnings.

Not done

  • A Get Actual Plan tab that is detached to its own window while the query runs still loses its plan when the query ends. The result is written to the tab that left the strip. Which window shows the result is a design question, so I left it.
  • The Query Store Overview is unchanged. It already cancels its loads when it leaves the visual tree.
  • GetActualPlanFromFile needs a connection dialog, so no test runs it. The tests cover the AddLoadingTab call that it now uses.

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 4 commits September 28, 2026 17:31
A plan viewer registers its plan with PlanSessionManager when it loads, and
only PlanViewerControl.Clear() took it back out. The docs in a query
session's own strip already called it, but every window-level close did
not: closing a plan tab, closing a query session (which left every plan
viewer in it registered) and closing a detached plan window all removed the
tab and nothing else. The plan then stayed in memory, and in the MCP
list_plans answer, until the app exited.

TryCloseTabAsync is the one place every window-level close ends (the X,
middle-click, Ctrl+W, and the context menu's Close, Close Other Tabs and
Close All Tabs), so it now releases the content of the tab it removed. The
detached window's real close does the same. Detach and re-dock move the
content and do not release it. The release reads the tab's content at close
time, so a tab whose spinner was replaced by a plan releases the plan.

Tests register nothing by count: each one reads the session id of the
viewer it opened and asks the manager about that id alone.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
The loading tab a query or plan capture opens has a Cancel button and an
Escape handler. Both cancelled the session's current run, read from the
_executionCts field when the key was pressed, and both stay attached after
the run ends: a failed capture leaves its tab on screen, still focusable.
Escape pressed on that tab later cancelled whatever run was newer, on a
different tab.

Each run now takes its own source from BeginRun and the handlers close over
it, so they can only ever cancel the run their tab was opened for.

BeginRun no longer disposes the source it replaces. Those handlers hold the
older source and can fire after a newer run has started, and Cancel on a
disposed source throws. This is the codebase's written rule, cancelled and
never disposed, already applied to FetchDatabaseMetadataAsync and the
Overview's refresh.

MainWindow.PlanViewer.cs (Get Actual Plan from a file) already closes over a
local source, so it needed no change.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
A tab close removed the tab and cancelled nothing, so work kept running on
the server for a tab nobody could see, and a query or plan capture runs with
no timeout.

- A Query Store grid document is now released when it closes. The grid gets
  CancelFetch, which cancels its fetch and its database check and refuses to
  start the first fetch it posted at construction if the tab is already
  gone. It cancels and never disposes: the wait-stats expander reads
  _fetchCts.Token fresh when it runs, and Token on a disposed source throws.
- One ReleaseDocument now says what each document kind gives up (plan viewer
  unregisters, grid and History cancel their fetch, a loading tab cancels the
  run it shows). CloseDocument and the ✕ built by CreateSubTab both go through
  it, so History's own onClose is gone. AddQueryStoreDocument is the one place
  a grid document is built; the two call sites had the same ten lines.
- A loading tab remembers its run, so closing the tab cancels it. Closing a
  whole query session also cancels its current run and its database
  metadata fetch. The window-level Get Actual Plan tab does the same through
  AddLoadingTab.
- A run whose result arrives after it was cancelled no longer builds a viewer.
  Its tab may already be closed, and a viewer built for it would register a
  plan that nothing is left to unregister.
- RemoveDocument ignores a document that is not in the strip, since a
  cancelled run now removes its tab a second time after the close did.

The Overview already cancels its own loads when it detaches from the visual
tree, so it is unchanged.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
The session's own KeyDown handler cancelled the current run for every Escape
it heard, and it hears every Escape in the session. A keystroke on a
finished plan, a Query Store grid or the Overview reached past its own tab
and stopped a capture running somewhere else, which is the same defect the
loading tab's own handlers had.

Escape now cancels the current run only where that run is: in the editor,
which is where it was started from, or on the tab that is showing it.
Escape in the editor and on the loading tab behave as before.

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:47
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed the non-test changes. I found nothing blocking.

  • Cancel without dispose: the cancel-never-dispose handling of the run and Query Store fetch sources is consistent. It avoids ObjectDisposedException from late Cancel and Token calls.
  • Cancel placement: ReleaseTabContent runs only after ConfirmCloseAsync succeeds and the tab is removed, so a refused close cancels nothing.
  • Detach/re-dock and shutdown: these paths correctly skip the release.
  • No T-SQL, SqlParameter, csproj, version or NoWarn changes.

Minor points, none blocking:

  • ReleaseTabContent's DockPanel case matches any DockPanel and clears any PlanViewerControl child. That is fine today, but it is coupled to the toolbar layout, so a new wrapper type would silently skip the release.
  • I did not run the tests here. The PR reports 17 headless tests and a zero-warning build.

@erikdarlingdata

erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner Author

The DockPanel match is the same one the open-file path uses to find an existing plan tab (MainWindow.FileOps.cs, the existing.Content is DockPanel lookup), so a new wrapper would have to change both. ClosingAPlanTabTakesItsPlanOffTheMcpSessionList opens a plan file through the real path and fails if that tab stops releasing its viewer. A pasted plan tab is built by the same CreatePlanTabContent, but no test pastes one.

@erikdarlingdata
erikdarlingdata merged commit b78ed21 into dev Sep 28, 2026
4 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/tab-close-cleanup branch September 28, 2026 21:53
@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