Fix #2825: Plan Viewer crash from mutating the tab collection during WPF container generation

CHANGELOG.md

@@ -104,6 +104,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Lite's portable ZIP is self-contained, which HALVED it** ([#2501]) - `Publish Lite` is now `-r win-x64 --self-contained` in both `build.yml` and `nightly.yml`, so neither Lite artifact has a .NET prerequisite any more and the failure [#2489] documented stops existing: a tester who unzips onto a stock Windows Server no longer meets the .NET host's bare `You must install .NET to run this application` before a line of our code runs. **The size went the opposite way from what bundling a runtime suggests.** The old publish was RID-agnostic, so it copied every platform its packages ship - **537 MB of `runtimes\` on a 565 MB tree** (osx 130, linux-x64 116, linux-arm64 70, win-arm64 56, then win-x86, musl, loongarch64 and riscv64), of which only the **52 MB `win-x64`** folder could ever load on Windows. `DuckDB.NET.Bindings.Full` is most of it, SkiaSharp and SqlClient behind it. Dropping ~485 MB of unloadable native payload beats the cost of bundling .NET, WPF and ASP.NET Core by roughly two to one: measured on one commit and one SDK, **565 MB tree / 212.7 MB zipped becomes 277 MB / 114.2 MB**. It matters most for the **nightly** ZIP, which is the UAT download and is not offered as a `Setup.exe` at all. **A RID-specific publish needed two more files than the flag.** `Lite/packages.lock.json` had only a `net10.0-windows7.0` target, and a RID restore adds `net10.0-windows7.0/win-x64` to it - after which the `dotnet restore --locked-mode` that BOTH workflows run before the publish fails `NU1004: the project's runtime identifiers have changed`, because locked mode compares the PROJECT's RID set (empty) against the lock file's (win-x64). Reproduced locally; that is a red CI run on every PR, not the future `--no-restore` trap it was filed as. The fix is `<RuntimeIdentifiers>win-x64</RuntimeIdentifiers>` in `PerformanceMonitorLite.csproj`, so the project itself asks for that graph and one committed lock file satisfies the RID-less locked-mode restore and the RID publish alike; `RuntimeIdentifiers` (plural) sets no RID on the build, so a plain `dotnet build` stays RID-agnostic and `Lite.Tests` is untouched. **SignPath needed nothing** - the `Lite` artifact-configuration slug already receives both shapes today, and the signed re-zip reads `signed/Lite/*`, inheriting whatever shape `publish/Lite` has. Auto-update is unaffected; the ZIP is not a Velopack channel. `LiteRuntimePrerequisiteDocsTests` went red on the flag alone (3 of its 7 facts) and was rewritten to state every claim BOTH ways round: [#2499]'s version asserted only that the docs DID name the runtimes, so two of its facts stayed green while the prose went stale. It now also derives the lock file's RID coverage from the `-r` flags in the workflows, and every new assertion was proven red with its fix reverted.
 
 ### Fixed
+- **FinOps "View Stored Plan" no longer crashes the whole Darling Viewer** ([#2825]) - right-clicking a High Impact or Expensive Queries row and choosing "View Stored Plan" on a not-yet-opened Plan Viewer took the entire app down with an unhandled InvalidOperationException, "Cannot call StartAt when content generation is in progress". Opening the Plan Viewer tab for the first time realizes its inner tab strip, and WPF re-raises that strip's SelectionChanged for the auto-selected "+" sentinel from INSIDE the container generator - the shared StandalonePlanViewerController answered by inserting a fresh sub-tab right then, and mutating a TabControl's Items mid-generation throws. The "+"-tab auto-add now defers off the generation pass while only the sentinel is present (the transient that exists solely during first-time setup) and stays synchronous for a genuine user click, so the opener's own sub-tab is the one that renders: no duplicate tab, no reordering, no crash. The same controller fix covers Lite, and the deprecated Dashboard's separate copy of the plan-viewer code got the identical guard.
 - **The `query_store` fetch split lines now report the probe's INPUT size, not just the ids it attempted** ([#2823]) - `probe:` is the store half of `plan_fetch`/`text_fetch`, and it scales with the number of references handed to the touch/probe round trip. The line beside it logged `PerItemPlanIdsAttempted`, which is incremented only inside the TARGET chunk loop and therefore counts what came back MISSING - a different quantity by orders of magnitude on a healthy database. A pass that probes 847 references and owes nothing logged `0 ids` next to 517ms of real store work. Measured on production the probe costs ~0.61ms per reference, dead linear (78 references 49ms, 272 166ms, 847 517ms), so dividing probe time by the attempted-id count produced a number with no physical meaning - and it was divided that way twice: [#2819] read a 673ms cost on `ids=0` passes as pure `OpenConnectionAsync` and derived a phantom ~140x production-vs-benchmark gap, then [#2822] was expected to drive that floor to zero and briefly read as a failed fix when it did not. It was not: the floor fell 544ms to 397ms and the 147ms delta matches that box's measured 139ms connect cost, so [#2822] removed exactly the acquisition it claimed. What remained was never acquisition. Ruled out with evidence rather than argument - the reconnect path logged zero reopens all day, 500 server-side no-op round trips took 1.475ms total, and the store sat at 19 connections with 1 active and 0 waiting. Both fetches now stamp their own probe input size and the split line carries it as `N probed`, pinned by IL reachability so a refactor cannot drop one side and leave that denominator wrong again.
 - **The Query Store plan and text fetches stop opening their own store connections** ([#2819]) - [#2811]'s split named `probe:` "the store connection open PLUS the touch/probe round trip" and declined to separate them, reasoning that doing so "would name a connection pool rather than a cost". It was a connection pool. The probe SQL runs in **0.39ms** with nothing stale and 37ms with 40 stale ids - every access an index scan, and none of its four tables a hypertable, so [#2796]'s missing-chunk-pruning shape is ruled out by fact rather than resemblance - while the phase itself measured **673-6,663ms**, and a cycle with zero ids, which issues no SQL whatsoever, still cost 673ms. That floor is acquisition. Each of the 114 (server, database) pairs was opening a SECOND and THIRD store connection per item, on top of the one the collector body already holds and documents as "one pooled store connection for the whole body" - roughly 228 acquisitions per cycle against a `MaxPoolSize` of 24. Worse, `await using` scoped the private connection to the whole method, so a pool slot was held across the SQL Server target fetch - measured at **104,799ms** on one database, one slot of twenty-four doing nothing store-side for a hundred seconds. Both fetches now BORROW the caller's connection, which is safe for a structural reason rather than a hopeful one: the enumerated driver reads an item and then awaits its write, so that connection is provably idle across exactly the window these fetches occupy, and neither the probe (autocommit) nor the writer (which opens and commits its own transaction) spans the target fetch with one. The borrowed connection is the collector BODY's, reused for every database in the sweep, so a store fault inside a fetch would leave it broken for every subsequent item - and because the fetches swallow their own exceptions and return a batch, `writeBatch` (which runs OUTSIDE the driver's per-item catch and is documented to propagate, "storage failure is systemic") would then fail the runtime-stats write and abort the REST OF THE SWEEP, which private connections made structurally impossible. So the fetches now reopen the borrowed connection on the way out of their catch: a transient fault (a cancelled command, a dropped socket) recovers and the caller's write proceeds as if the fetch had owned its connection, while a genuinely unreachable store fails the reopen too and is left to propagate - which is the correct handling of a systemic store failure and unchanged from before. `MaxPoolSize` is deliberately NOT raised: every pooled connection is a postgres.exe PROCESS and this store logged 8 "could not reserve shared memory region" retries the same day, so the fix is fewer acquisitions, not a bigger pool - and the multiplier that matters is concurrency, not acquisitions: per database the fetches ACQUIRED three connections (body + plan + text), but they acquire sequentially, so peak CONCURRENT holds per swept server were body + one in-flight fetch = 2. `MaxPoolSize` bounds concurrency, so 2x the sweep width is the number that had to fit - 8 at the 4-wide default, 32 at the 16-wide `ClampConcurrentSweeps` limit, over this bound. Borrowing puts peak concurrent demand back at the sweep width itself, where even a 16-wide sweep fits. Pinned by IL reachability rather than by a duration assertion, which would be flaky when honest and vacuous when loose: the two fetch state machines must contain zero calls to `OpenConnectionAsync`, with positive controls that resolve through the same MemberReference machinery so a scanner that has stopped resolving tokens fails the controls too instead of reporting the acquisition absent for the wrong reason.
 - **Web dashboard: a sparse trend chart spanned only its data burst, so an old incident read as current** ([#2802]) - `renderLineChart` set its x-axis domain from the first and last DATA point, not the requested window. A dense series (CPU, memory) fills the window so it looked right, but a sparse discrete-event series (Blocking Events, Deadlocks) only has rows inside a short burst, so a chart titled "last 24 hours" zoomed its axis to that ~30-minute burst and dropped the calendar date from the tick labels - a burst ~19 hours before the view time read as if it were happening now. The renderer now takes an optional window (windowStart/windowEnd, UTC-epoch ms) and spans the axis over THAT window when given, so the points plot at their true position and the date survives; the window end is the query-time now, never the last data point, so an old burst can no longer slide to the right edge. The server tabs and the shared line renderer pass the range selector's hours ending now, and a composed panel passes the same window it fetched over (a brush-zoom, then the #2788 per-panel pin, then the view scope). No window passed leaves the chart byte-for-byte unchanged, a lone point still renders as a dot at its true x, and a brush-zoom still wins. Pinned with a source-scan test - there is no JS execution harness for the chart module.
@@ -1457,6 +1458,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Failed SQL Agent job alert** ([#749]) in both apps — complements the existing job-duration alerts with a **Failed Agent Job** alert (default on) that issues a live `msdb.dbo.sysjobhistory` query at alert-check time for job-outcome rows (`step_id = 0`, `run_status = 0`) that failed within a configurable look-back window (default 60 minutes). The read degrades gracefully when the login lacks msdb / `SQLAgentReaderRole` access (returns empty, never faults the alert cycle) and is skipped entirely on Azure SQL DB, which has no SQL Agent
 - **Installer: optional custom data/log file locations** ([#768]) — two optional CLI flags, `--data-path` and `--log-path` (both `--flag VALUE` and `--flag=VALUE` forms accepted), place the `PerformanceMonitor` database's `.mdf`/`.ldf` on specific server-side volumes at install time; an omitted flag falls back to the instance default path as before. The paths apply only on first creation (the create block is guarded by `IF DB_ID(N'PerformanceMonitor') IS NULL`), and Azure SQL Managed Instance ignores them. The path is validated and escaped (control characters and the dangerous filename characters are rejected; single quotes are doubled in both the C# injection layer and the dynamic `CREATE DATABASE`) because a data-file `FILENAME` literal cannot be parameterized
 
+[#2825]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2825
 [#2820]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2820
 [#2802]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2802
 [#2814]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2814

Darling/Darling.Tests/CapabilityPins/planviewer.baseline.txt

@@ -13,6 +13,7 @@ multi-file-open
 open-file-button
 open-file-dialog
 paste-button
+reentrancy-defer-add
 render-loadplan
 shared-plan-viewer-control
 unique-sub-tab-label

Darling/Darling.Tests/PlanViewerCapabilityPinTests.cs

@@ -65,6 +65,9 @@ private static readonly (string Name, string Pattern)[] Probes =
         ("entry-close",                "MainWindowPlanViewerClose_Click(object sender"),
         // Darling-only: the FinOps "View Plan" store->viewer entry
         ("entry-stored-plan",          "OpenStoredPlanInPlanViewer("),
+        // Re-entrancy guard for the "+"-sentinel auto-add (#2825): the deferral latch must stay present so the
+        // FinOps "View Stored Plan" crash (Items.Insert mid container-generation) cannot silently regress.
+        ("reentrancy-defer-add",       "_addTabInsertDeferred"),
     };
 
     [Fact]

Lite.Tests/CapabilityPins/planviewer.baseline.txt

@@ -12,6 +12,7 @@ multi-file-open
 open-file-button
 open-file-dialog
 paste-button
+reentrancy-defer-add
 render-loadplan
 shared-plan-viewer-control
 unique-sub-tab-label

Lite.Tests/PlanViewerCapabilityPinTests.cs

@@ -68,6 +68,9 @@ private static readonly (string Name, string Pattern)[] Probes =
         // Per-app thin entry-method definitions
         ("entry-open",                 "OpenPlanViewerButton_Click(object sender"),
         ("entry-close",                "MainWindowPlanViewerClose_Click(object sender"),
+        // Re-entrancy guard for the "+"-sentinel auto-add (#2825): the deferral latch must stay present so the
+        // FinOps "View Stored Plan" crash (Items.Insert mid container-generation) cannot silently regress.
+        ("reentrancy-defer-add",       "_addTabInsertDeferred"),
     };
 
     [Fact]

PerformanceMonitor.Ui/StandalonePlanViewerController.cs

@@ -12,6 +12,7 @@
 using System.Windows;
 using System.Windows.Controls;
 using System.Windows.Input;
+using System.Windows.Threading;
 
 namespace PerformanceMonitor.Ui;
 
@@ -38,6 +39,11 @@ plan captured with either app's tab layout is indistinguishable. */
     private readonly TabControl _planTabControl;
     private bool _initialized;
 
+    /* Re-entrancy latch for the "+"-sentinel auto-add (see HandleAddTabSelected / #2825): at most ONE deferred
+       add is queued at a time while only the "+" tab is present, so a burst of generation-time SelectionChanged
+       notifications cannot pile up multiple inserts. Cleared by Reset() so re-opening the surface re-arms it. */
+    private bool _addTabInsertDeferred;
+
     public StandalonePlanViewerController(TabControl planTabControl)
     {
         _planTabControl = planTabControl ?? throw new ArgumentNullException(nameof(planTabControl));
@@ -69,11 +75,50 @@ public void EnsureInitialized()
         _planTabControl.SelectionChanged += (_, _) =>
         {
             if (_planTabControl.SelectedItem is TabItem { Tag: string t } && t == PlanAddTabId)
+                HandleAddTabSelected();
+        };
+    }
+
+    /// <summary>
+    /// The "+" sentinel became the selected tab. In steady state that is a user clicking "+", so add a fresh
+    /// empty sub-tab and select it. The trap (#2825): WPF ALSO re-asserts this selection from INSIDE the inner
+    /// TabControl's container generator the first time the Plan Viewer host tab is realized -- the "+" tab is then
+    /// the sole item and is auto-selected, and the generator coerces that selection while it is still running.
+    /// <see cref="AddNewEmptyPlanSubTab"/> ends in an <c>ItemCollection.Insert</c>, and inserting mid-generation
+    /// throws "Cannot call StartAt when content generation is in progress"; unhandled, it kills the whole app.
+    /// The generator status is no help (it has already flipped to ContainersGenerated by the time this handler
+    /// runs), but the item count is: only the "+" sentinel present means we are in first-time setup/realization,
+    /// never a steady-state click, so we defer the insert to the dispatcher to run after the generation/layout
+    /// pass unwinds. Every opener (FinOps "View Plan", "Open Plan Viewer", close-of-last-sub-tab) adds and selects
+    /// a REAL sub-tab synchronously right after triggering this, so the deferred pass finds "+" no longer selected
+    /// and no-ops -- no duplicate tab, no reordering. When real sub-tabs already exist the control is fully
+    /// realized and idle, so the click path stays synchronous exactly as before (no dispatcher hop, no flash).
+    /// </summary>
+    private void HandleAddTabSelected()
+    {
+        // Steady state: real sub-tab(s) already present => a genuine user click on "+". Safe to mutate now.
+        if (_planTabControl.Items.Count > 1)
+        {
+            var newSub = AddNewEmptyPlanSubTab();
+            _planTabControl.SelectedItem = newSub;
+            return;
+        }
+
+        // Only the "+" sentinel exists => first-time setup, possibly mid-generation. Never insert now; defer once.
+        if (_addTabInsertDeferred) return;
+        _addTabInsertDeferred = true;
+        _planTabControl.Dispatcher.BeginInvoke(DispatcherPriority.Loaded, new Action(() =>
+        {
+            _addTabInsertDeferred = false;
+            // Re-validate: the surface may have been torn down (Reset), and the opener has usually already
+            // selected a real sub-tab. Only add when "+" is STILL the lone selection that needs a companion.
+            if (_initialized &&
+                _planTabControl.SelectedItem is TabItem { Tag: string t } && t == PlanAddTabId)
             {
                 var newSub = AddNewEmptyPlanSubTab();
                 _planTabControl.SelectedItem = newSub;
             }
-        };
+        }));
     }
 
     /// <summary>
@@ -410,6 +455,7 @@ public void Reset()
                 pv.Cleanup();
         _planTabControl.Items.Clear();
         _initialized = false;
+        _addTabInsertDeferred = false;
     }
 
     private static bool IsPlanFile(string path)