[erikdarlingdata]

The crash (#2825)

FinOps tab -> High Impact -> right-click the top query -> "View Stored Plan" took the whole Darling Viewer down:

InvalidOperationException: Cannot call StartAt when content generation is in progress.
  at System.Windows.Controls.ItemCollection.Insert(...)
  at PerformanceMonitor.Ui.StandalonePlanViewerController.AddNewEmptyPlanSubTab()
  at PerformanceMonitor.Ui.StandalonePlanViewerController.<EnsureInitialized>b__4_0(...)   // SelectionChanged
  at System.Windows.Controls.TabControl.OnGeneratorStatusChanged(...)                       // during generation

Root cause (a classic WPF re-entrancy)

The shared StandalonePlanViewerController owns an inner plan TabControl with a trailing "+" sentinel tab. Its SelectionChanged handler auto-adds a fresh empty sub-tab whenever the "+" tab becomes selected, by calling AddNewEmptyPlanSubTab(), which ends in an ItemCollection.Insert.

OpenStoredPlanInPlanViewer calls EnsureInitialized() (which adds the "+" tab, and WPF auto-selects it because it is the only item), then sets MainWindowPlanViewerTab.IsSelected = true. That first-time selection realizes the host tab's content, so the inner TabControl generates its containers for the first time. When the generator finishes it fires OnGeneratorStatusChanged, which coerces the inner selection to the already-selected "+" tab and raises SelectionChanged while the generator is still in progress. The handler then does Items.Insert(...) mid-generation, ItemContainerGenerator.StartAt throws, nothing catches it, and the app dies.

The OpenPlanViewerButton_Click path (sidebar "Open Plan Viewer") has the same cold-realization shape and the same exposure; the reporter just hit it via FinOps.

The fix (shared, minimal, behavior-preserving)

In PerformanceMonitor.Ui/StandalonePlanViewerController.cs, the SelectionChanged handler now calls a new HandleAddTabSelected():

• Steady state (Items.Count > 1, real sub-tabs already present): a genuine user click on "+". The control is fully realized and idle, so the insert is done synchronously exactly as before - no dispatcher hop, no visual flash, no behavior change.
• Only the "+" sentinel present (Items.Count == 1): we are in first-time setup / realization (the state the mid-generation coercion happens in). Never insert now - defer via _planTabControl.Dispatcher.BeginInvoke(DispatcherPriority.Loaded, ...) so it runs after the generation/layout pass unwinds, guarded by a one-shot _addTabInsertDeferred latch so a burst of SelectionChanged notifications can't queue multiple inserts. The deferred callback re-validates _initialized and that "+" is STILL selected before adding.

Why the item count is the discriminator (not the generator status)

By the time the handler runs, the generator has already flipped Status to ContainersGenerated (it is set inside the generator's Dispose, before SelectionChanged is raised), so GeneratorStatus cannot distinguish the dangerous coercion from a safe click. But the count can: "+" is only ever left as the selected item with no real sub-tab during first-time setup - every steady-state "+" selection (a user click) happens when real sub-tabs already exist, with the control realized and idle. So Count == 1 && "+" selected is exactly the unsafe transient, and Count > 1 && "+" selected is exactly a safe click.

Why this preserves behavior (no duplicate tab, no reorder)

Every opener - FinOps "View Plan", "Open Plan Viewer", and the close-of-last-sub-tab handler - adds and selects a real sub-tab synchronously right after triggering the "+" transient. So by the time the deferred pass runs, "+" is no longer selected and the deferred add no-ops. The plan / empty sub-tab the opener created is the one that renders. New sub-tabs still insert before the "+" tab (unchanged AddNewEmptyPlanSubTab logic), so the "+" stays last. The genuine user-click path is untouched.

The deferral priority is Loaded, matching the only Dispatcher.BeginInvoke priority already used in this codebase (including the plan viewer's own focus deferral). The re-validate in the deferred callback covers teardown: Reset() clears _initialized (and the latch), so a deferral that survives a close does nothing.

Parity

• Lite: the fix is in the shared StandalonePlanViewerController, which Lite consumes via Lite/MainWindow.PlanViewer.cs (OpenPlanViewerButton_Click). Lite's AddMultipleServersDialog only borrows the controller's unique-label idiom in a comment; it does not host the plan TabControl, so it has no exposure. Lite is covered by the same one change.
• Deprecated Dashboard: deprecated/Dashboard/MainWindow.PlanViewer.cs keeps its own copy of the plan-viewer code (a local AddNewEmptyPlanSubTab + SelectionChanged handler, not the shared controller) and had the identical bug (OpenPlanViewerTab selects the host tab before adding the first sub-tab). It got the identical guard, adapted to its nullable _mainPlanTabControl (the close path nulls it, and the deferred callback guards != null). Per the never-scope-down-a-Dashboard-parity-fix rule.

Tests

A full WPF UI crash repro is not automatable headless, and the existing PlanViewerCapabilityPinTests are deliberately source-scan-only (no WPF / assembly load). So the guard is pinned there: both Darling.Tests and Lite.Tests get a new reentrancy-defer-add probe on the _addTabInsertDeferred latch (present in the shared controller both suites scan) plus its baseline line, so reverting the deferral to a synchronous insert trips the pin.

Verification

• dotnet build succeeded for PerformanceMonitor.Darling.Viewer, PerformanceMonitorLite, and the deprecated Dashboard (Debug): 0 errors, no new warnings (the one surfaced warning, CS8848 in DataGridRowMarks.cs, is pre-existing and unrelated).
• The pin-scan logic was replicated headlessly and passes for both suites (all baseline probes matched, new probe matched, floors satisfied). The dotnet test runner itself is unusable in this environment due to an SDK testhost manifest bug (testhost.dll missing from testhost.deps.json, version 18.6.0-release-26377-103) - unrelated to this change; the test projects compile.
• Behavior-preservation reasoning above was traced through all three opener paths plus the close-last-sub-tab and multi-file-open/drop paths.

Review

A general code-reviewer traced all four implementations (Darling opener, Lite opener, Dashboard opener, close-last-sub-tab) and confirmed: no new re-entrancy, no double-add (every opener selects a real sub-tab synchronously before the deferred callback runs, so it always no-ops), the "+" stays last, Loaded is the correct priority, and teardown is safe. Two follow-ups came out of it:

• Fixed in this PR: the Dashboard's close path did not clear the new _addTabInsertDeferred latch the way the shared Reset() does - a parity nit in this change, now mirrored.
• Filed as Plan Viewer inner TabControl SelectionChanged handler accumulates across open/close cycles #2828 (pre-existing, out of scope): the inner TabControl's SelectionChanged handler is subscribed per-init and never unsubscribed, so it accumulates one handler per open/close cycle. It exists on main independent of this fix and is benign after this change (the _addTabInsertDeferred latch and the self-correcting selection collapse duplicate invocations to a single add - no crash, no double tab). Folding a subscription-lifecycle refactor into a high-severity crash fix would widen the blast radius in exactly the re-entrancy-sensitive area this PR is meant to make safe, so it is tracked separately.

Closes #2825

🤖 Generated with Claude Code

https://claude.ai/code/session_01K6rFZwjKPCzF2CxtxM6Ugp

[claude]

Reviewed the diff (C#/WPF only — no T-SQL in this PR, so the collector/style conventions don't apply here).

Fix logic checks out. Traced all four openers (Darling's OpenStoredPlanInPlanViewer/OpenPlanViewerButton_Click, Lite's OpenPlanViewerButton_Click, the Dashboard's OpenPlanViewerTab) plus the close-last-sub-tab handlers in both the shared controller and the deprecated Dashboard copy:

• Every opener calls AddNewEmptyPlanSubTab() synchronously right after EnsureInitialized()/tab selection, which reselects away from "+" before the deferred Loaded-priority callback ever runs — so the no-op/no-duplicate reasoning in the PR description holds.
• The close-last-sub-tab path also checks out: Items.Remove synchronously re-selects "+" with Count == 1, which now defers instead of inserting mid-callback, and the handler's own explicit if (Items.Count == 1) AddNewEmptyPlanSubTab(); immediately supplies the real tab, so the later deferred pass finds "+" deselected and no-ops.
• Items.Count > 1 as the "steady-state click" discriminator is sound given the invariant that the surface never rests with only "+" present outside the first-realization transient (the close handler always backfills a real sub-tab).

Parity is solid. The shared-controller fix covers Lite for free; the deprecated Dashboard's independent copy got an identical guard including the _addTabInsertDeferred reset on close (MainWindow.xaml.cs); both Darling.Tests and Lite.Tests PlanViewerCapabilityPinTests gained matching reentrancy-defer-add pins against _addTabInsertDeferred, with matching baseline entries. CHANGELOG entry present and consistent with the fix.

One theoretical (not practically reachable) edge case, noted but not blocking: the deferred BeginInvoke(DispatcherPriority.Loaded, ...) callback validates _initialized but not "which open session" it belongs to. If Reset() followed by a fresh EnsureInitialized() could happen within the same dispatcher pass as the still-pending deferred callback, a stale callback from a prior session could fire against the new session's "+" selection. In practice this can't happen through the UI: Reset()/reopen only ever run from separate button-click (Input-priority) dispatches, and Loaded is a higher priority than Input, so the deferred callback always drains before the next click is even processed. Flagging only for completeness — no action needed.

No security, T-SQL, or Lite/Darling drift concerns found beyond what's noted above.