From 43ed152ab609f26f92a9df8b388594430b7150f0 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 3 Sep 2026 05:32:52 -0400 Subject: [PATCH] Fix #2825: defer the Plan Viewer "+"-tab auto-add out of container generation Right-clicking a FinOps High Impact / Expensive Queries row and choosing "View Stored Plan" on a not-yet-opened Plan Viewer crashed the whole Darling Viewer with an unhandled InvalidOperationException: "Cannot call StartAt when content generation is in progress". The shared StandalonePlanViewerController's inner plan TabControl has a trailing "+" sentinel tab whose SelectionChanged handler auto-adds a new empty sub-tab (an ItemCollection.Insert). The first time the Plan Viewer host tab is realized, the inner control's only item is the auto-selected "+" tab, and WPF's container generator coerces that selection WHILE GENERATING; inserting into Items then throws, and nothing catches it. HandleAddTabSelected now defers the insert via Dispatcher.BeginInvoke(Loaded) when only the "+" sentinel is present (the transient that only exists during first-time setup), guarded by a one-shot latch with a re-validate in the deferred callback, and stays synchronous for a genuine steady-state "+" click. The opener adds its own real sub-tab synchronously right after, so the deferred pass finds "+" deselected and no-ops: no duplicate tab, no reordering, no crash. Generator status is unusable here (already ContainersGenerated by the time the handler runs), so the item count is the discriminator. Parity: the fix lives in the shared controller, so Lite gets it too. The deprecated Dashboard keeps its own copy of the plan-viewer code and had the identical bug, so it got the identical guard. Tests: the PlanViewerCapabilityPinTests source-pin (Darling + Lite) gains a probe on the _addTabInsertDeferred latch so the deferral cannot silently regress. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01K6rFZwjKPCzF2CxtxM6Ugp --- CHANGELOG.md | 2 + .../CapabilityPins/planviewer.baseline.txt | 1 + .../PlanViewerCapabilityPinTests.cs | 3 ++ .../CapabilityPins/planviewer.baseline.txt | 1 + Lite.Tests/PlanViewerCapabilityPinTests.cs | 3 ++ .../StandalonePlanViewerController.cs | 48 ++++++++++++++++++- deprecated/Dashboard/MainWindow.PlanViewer.cs | 48 +++++++++++++++++-- deprecated/Dashboard/MainWindow.xaml.cs | 3 ++ 8 files changed, 103 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d29778bd2a..3738f2d541 100644 --- a/CHANGELOG.md +++ b/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 `win-x64` 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 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. - **The `io_latency` baseline no longer times out, and eight other metrics got faster with it** ([#2820]) - `PgBaselineProvider`'s shared `RobustTierScaffold` joined its MAD tiers with an OR'd non-equi predicate PostgreSQL cannot hash, reaching a planner estimate of cost 596,208,924 for a 193-row result and spilling to temp. The tiers are now expanded with `UNION ALL` and joined by equality - identical rows, identical checksum, 23.7s to 4.2s measured on a 476,431-row window. On the dogfood box `io_latency` had been exceeding its command timeout on eight of the last nine days, losing its baseline and silencing its anomaly detection each time. @@ -3143,3 +3144,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#2816]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2816 [#2796]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2796 [#2819]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2819 +[#2825]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2825 diff --git a/Darling/Darling.Tests/CapabilityPins/planviewer.baseline.txt b/Darling/Darling.Tests/CapabilityPins/planviewer.baseline.txt index a2bf96f942..f8ca3bf80f 100644 --- a/Darling/Darling.Tests/CapabilityPins/planviewer.baseline.txt +++ b/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 diff --git a/Darling/Darling.Tests/PlanViewerCapabilityPinTests.cs b/Darling/Darling.Tests/PlanViewerCapabilityPinTests.cs index ff13101674..22b3d8dec6 100644 --- a/Darling/Darling.Tests/PlanViewerCapabilityPinTests.cs +++ b/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] diff --git a/Lite.Tests/CapabilityPins/planviewer.baseline.txt b/Lite.Tests/CapabilityPins/planviewer.baseline.txt index 2d5b8777e3..0a7d2fb333 100644 --- a/Lite.Tests/CapabilityPins/planviewer.baseline.txt +++ b/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 diff --git a/Lite.Tests/PlanViewerCapabilityPinTests.cs b/Lite.Tests/PlanViewerCapabilityPinTests.cs index 55ab4fe157..679e72eaa4 100644 --- a/Lite.Tests/PlanViewerCapabilityPinTests.cs +++ b/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] diff --git a/PerformanceMonitor.Ui/StandalonePlanViewerController.cs b/PerformanceMonitor.Ui/StandalonePlanViewerController.cs index 568389cc02..3bd7c1ec55 100644 --- a/PerformanceMonitor.Ui/StandalonePlanViewerController.cs +++ b/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(); + }; + } + + /// + /// 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. + /// ends in an ItemCollection.Insert, 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). + /// + 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; } - }; + })); } /// @@ -410,6 +455,7 @@ public void Reset() pv.Cleanup(); _planTabControl.Items.Clear(); _initialized = false; + _addTabInsertDeferred = false; } private static bool IsPlanFile(string path) diff --git a/deprecated/Dashboard/MainWindow.PlanViewer.cs b/deprecated/Dashboard/MainWindow.PlanViewer.cs index dba406f114..67991bc3f0 100644 --- a/deprecated/Dashboard/MainWindow.PlanViewer.cs +++ b/deprecated/Dashboard/MainWindow.PlanViewer.cs @@ -24,6 +24,9 @@ public partial class MainWindow : Window private TabControl? _mainPlanTabControl; private Grid? _planViewerContainer; + /* Re-entrancy latch for the "+"-sentinel auto-add (see HandleAddTabSelected / #2825). */ + private bool _addTabInsertDeferred; + private void OpenPlanViewer_Click(object sender, RoutedEventArgs e) { if (_planViewerTab != null && ServerTabControl.Items.Contains(_planViewerTab)) @@ -69,11 +72,8 @@ private void OpenPlanViewerTab() _mainPlanTabControl.SelectionChanged += (_, _) => { - if (_mainPlanTabControl.SelectedItem is TabItem { Tag: string t } && t == PlanAddTabId) - { - var newSub = AddNewEmptyPlanSubTab(); - _mainPlanTabControl.SelectedItem = newSub; - } + if (_mainPlanTabControl?.SelectedItem is TabItem { Tag: string t } && t == PlanAddTabId) + HandleAddTabSelected(); }; var container = new Grid(); @@ -115,6 +115,44 @@ private void OpenPlanViewerTab() Dispatcher.BeginInvoke(DispatcherPriority.Loaded, new Action(() => _planViewerContainer?.Focus())); } + /// + /// The "+" sentinel became the selected tab -- add a fresh empty sub-tab and select it. Mirrors the shared + /// StandalonePlanViewerController's #2825 re-entrancy fix: when the Plan Viewer tab is realized for the + /// first time, WPF re-asserts this selection for the auto-selected "+" tab from INSIDE the inner TabControl's + /// container generator, and 's ItemCollection.Insert mid-generation + /// throws "Cannot call StartAt when content generation is in progress", which is unhandled and crashes the app. + /// Only the "+" tab present means we are in that first-time realization, so defer the insert off the generation + /// pass; the opener () adds a real sub-tab synchronously right after, so the + /// deferred pass finds "+" deselected and no-ops -- no duplicate tab. A real user click (steady state, more than + /// one item) stays synchronous exactly as before. + /// + private void HandleAddTabSelected() + { + if (_mainPlanTabControl == null) return; + + // Steady state: real sub-tab(s) already present => a genuine user click on "+". Safe to mutate now. + if (_mainPlanTabControl.Items.Count > 1) + { + var newSub = AddNewEmptyPlanSubTab(); + _mainPlanTabControl.SelectedItem = newSub; + return; + } + + // Only the "+" sentinel exists => first-time realization, possibly mid-generation. Defer once. + if (_addTabInsertDeferred) return; + _addTabInsertDeferred = true; + Dispatcher.BeginInvoke(DispatcherPriority.Loaded, new Action(() => + { + _addTabInsertDeferred = false; + if (_mainPlanTabControl != null && + _mainPlanTabControl.SelectedItem is TabItem { Tag: string t } && t == PlanAddTabId) + { + var newSub = AddNewEmptyPlanSubTab(); + _mainPlanTabControl.SelectedItem = newSub; + } + })); + } + /// /// Adds a new empty "New Plan" sub-tab to the inner plan TabControl and selects it. /// Returns the newly created sub-tab. diff --git a/deprecated/Dashboard/MainWindow.xaml.cs b/deprecated/Dashboard/MainWindow.xaml.cs index c88bfa965d..a292315290 100644 --- a/deprecated/Dashboard/MainWindow.xaml.cs +++ b/deprecated/Dashboard/MainWindow.xaml.cs @@ -1039,6 +1039,9 @@ private void CloseTab_Click(object sender, RoutedEventArgs e) ServerTabControl.Items.Remove(_planViewerTab); _planViewerTab = null; _mainPlanTabControl = null; + // Re-arm the "+"-sentinel deferral latch for the next open (matches the shared + // controller's Reset(); a fresh _mainPlanTabControl is built by OpenPlanViewerTab). + _addTabInsertDeferred = false; } } else if (_openTabs.TryGetValue(tabId, out var tabToClose))