Add Bulk Server Upgrades - #815

[mas-mark-y]

Bulk Server Upgrades

This is an attempt to implement a new feature requested in #815

Which component(s) does this affect?

• Full Dashboard
• Lite Dashboard
• Lite Tests
• SQL collection scripts
• CLI Installer
• GUI Installer
• Documentation

How was this tested?

• Check All Versions: Click the button, verify all servers are checked in parallel and versions appear in the grid
• Mixed states: Some servers current, some outdated
• Mixed states: Some servers current, some outdated, some unreachable — unreachable states are currently displayed as "Not Installed"
• Upgrade All: Click upgrade, verify all outdated servers upgrade in parallel
• Progress reporting: Verify per-server progress appears in the log panel
• Partial failure: One server is unreachable during upgrade — verify others still complete and the failure is reported
• Cancellation: Close the window during upgrades — verify no orphaned tasks or half-upgraded servers
• Post-upgrade: After upgrading, "Check All Versions" again — all should show current
• Single server still works: Verify the existing single-server "Check Server Version" button still works as before
• No outdated servers: Click "Check All Versions" when all are current — "Upgrade All" button should not appear
• Authentication types: Test with Windows Auth and SQL Auth
• Authentication types: Test with Windows Auth, SQL Auth, and Entra MFA servers in the mix
• Azure SQL DB: Servers without SQL Agent should upgrade cleanly (the collector skip logic is in the SQL scripts)

Checklist

• I have read the contributing guide
• My code builds with zero warnings (dotnet build -c Debug)
• I have tested my changes against at least one SQL Server version
• I have not introduced any hardcoded credentials or server names

[erikdarlingdata]

Thanks for taking this on — the core idea (Check All Versions probe + parallel InstallationService.ExecuteAllUpgradesAsync + ExecuteInstallationAsync per server) is sound and matches the pattern already used in AddServerDialog.xaml.cs:612 and the CLI installer. Most of what I'm flagging is around the edges.

Process-level blockers

1. Wrong base branch. This project's feature branches always go to dev (and dev merges to main only at release time — apologies, that's not in CONTRIBUTING.md and probably should be). Concrete impact: dev is on .NET 10, main is still .NET 8. The new test project targets net8.0-windows, and Installer/packages.lock.json is bumped from ILLink 8.0.26 → 8.0.27 (an 8.x patch). When dev next merges to main, both conflict and the test project won't build against the .NET 10 codebase. Please rebase onto dev and retarget the PR there.

2. Test project versions don't match the rest of the repo. Dashboard.Tests.csproj uses xunit 2.6.1 + xunit.runner.visualstudio 3.1.5. The existing Lite.Tests and Installer.Tests use xunit.v3 3.2.2 + xunit.runner.visualstudio 3.1.5. Tests do execute (I ran them locally — 2/2 pass), but it's inconsistent. Move to xunit.v3 3.2.2.

3. Tests aren't wired into CI. Dashboard.Tests isn't added to .github/workflows/build.yml — see the existing Run Lite fast tests / Run Installer tests steps for the shape.

4. Solution file noise. PerformanceMonitor.sln has an unrelated edit (VisualStudioVersion = 17.14.36930.0 d17.14 → ... 17.14.36930.0) — looks like VS install drift. Revert.

Correctness

5. ServerVersionInfo proxy drops InstalledVersion change notifications. OnInnerServerPropertyChanged (Models/ServerConnection.cs ~290) forwards DisplayName, ServerName, AuthenticationDisplay, MonthlyCostUsd, LastConnected, IsFavorite — but not InstalledVersion, even though ServerConnection.InstalledVersion does raise PropertyChanged. The code papers over this with a defensive dual-write everywhere (entry.InstalledVersion = ...; entry.Server.InstalledVersion = ...). Cleaner: either add InstalledVersion to the switch and drop the redundant private field, or make ServerVersionInfo.InstalledVersion a pure passthrough getter/setter on Server.InstalledVersion.

   (AuthenticationDisplay is the same shape — read-only computed on ServerConnection, never raises PropertyChanged. The proxy forwarding for it is dead code. Not blocking, just confusing.)

6. Closing the window mid-upgrade leaks running tasks. ManageServersWindow_Closing cancels the CTS and returns; the window dies, but the upgrade tasks keep running and continue posting to a dead Dispatcher via Progress<InstallationProgress>. The PR description already calls out this case as "not tested." Either await completion before closing or guard the Progress handlers (e.g. if (!Dispatcher.HasShutdownStarted) Dispatcher.Invoke(...)).

7. Progress bar oscillates under parallel upgrades. centralProgress writes UpgradeProgressBar.Value = p.ProgressPercent.Value from N concurrent servers reporting their own 0–100. Bar will jump randomly. Switch to a count-based bar (X of N servers complete) or average the per-server percents.

8. Button bar overflows at 1024px width. Summing the left-aligned button widths + 8px margins in ManageServersWindow.xaml:

   • Initial state (Upgrade/Cancel hidden): ~944px
   • During an in-flight upgrade (Upgrade + Cancel both visible): ~1230px

   The window is 1024px wide. Even in the initial state, with the Close button on the right + window chrome, the left-most buttons are clipped. Either grow the default width (~1280) or move the new buttons into a WrapPanel.

9. Dead variables. ManageServersWindow.xaml.cs:643-644 — int successCount = 0; int failCount = 0; declared in UpgradeAll_Click and never used. Triggers CS0219.

10. Stray editorial comments / using:

    • using ModelContextProtocol.Protocol; at the top of ManageServersWindow.xaml.cs — unused, looks like an IDE auto-import.
    • // extract the existing version logic into a helper appears twice (lines 455, 641); GetAppVersion() is the helper, so the comment is stale.
    • // existing version comparison logic follows unchanged... (line 374) is an editor note.
    • // Replace the existing ServerVersionInfo type with this implementation... (Models/ServerConnection.cs:206) — implies a prior ServerVersionInfo; on main there is none. Remove.

11. catch { } in ManageServersWindow_Closing. Swallows everything silently. At minimum, log via the existing AppendUpgradeLog.

12. PerServerUpgradeResult should be sealed (CA1852).

Design / UX

13. EntraMFA + parallel upgrades will pop N stacked auth dialogs. The PR's test plan correctly flags this as untested — at minimum, serialize EntraMFA upgrades (run them after the SQL Auth / Windows Auth batch) or warn the user up front before kicking them off in parallel.

14. Dispatcher.Invoke inside Progress<T> handlers is redundant. Progress<T> already marshals to the captured SynchronizationContext (the UI thread, since the instances are created on the UI thread). The nested Dispatcher.Invoke in centralProgress and the Dispatcher.CheckAccess() path in AppendUpgradeLog aren't wrong, just belt-and-suspenders.

15. Tests cover only the aggregator. UpgradeAggregator.Aggregate is ~25 lines of trivial counting; the orchestration in UpgradeAll_Click (cancellation, partial failure, credential lookup, EntraMFA serialization, dispatcher race on close) is where bugs will live. Not blocking — WPF code-behind is genuinely hard to unit-test — but the coverage is lighter than the PR description suggests.

TL;DR for a follow-up

1. Rebase onto dev and retarget the PR.
2. Dashboard.Tests.csproj → net10.0-windows + xunit.v3 3.2.2; wire into build.yml.
3. Revert the unrelated .sln edit.
4. Drop the stale comments, the unused using, and the two dead variables.
5. Fix the proxy's InstalledVersion forwarding so the dual-write isn't needed.
6. Grow the default window width or WrapPanel the new buttons.
7. Either rate-limit/serialize EntraMFA upgrades or warn before launching them in parallel.

Happy to look again once these are addressed.

[mas-mark-y]

@erikdarlingdata, thank you for a thorough and thoughtful review. I should've mentioned that this is my first WPF project and I am still learning the intricacies of GitHub and git integration in Visual Studio. I will flag this PR as Ready for review when I think it's ready. Again, thanks for your patience :-)

[erikdarlingdata]

Thanks for the updates, @mas-mark-y — here's where things stand against the earlier review, point by point so it's easy to cross-reference. It's still a draft, so no rush — this is just a map of what's left.

Done ✅

• Add check for updates feature #1 Base branch — retargeted to dev, test project targets net10.0-windows7.0. 👍
• Lite overview: show Online/Offline status for servers #2 xunit version — now xunit.v3 3.2.2, matches Lite.Tests/Installer.Tests. 👍

#3 — CI wiring needs three more pieces

The Run Dashboard tests step is there, but as written it can't pass:

1. Missing lock file. CI restores with --locked-mode, which fails when there's no packages.lock.json. Every other project has one. Generate it once and commit it:

   dotnet restore Dashboard.Tests/Dashboard.Tests.csproj --use-lock-file
   

2. No dashboard filter. The step's if: checks steps.filter.outputs.dashboard, but the paths-filter block only defines lite_analysis, installer, and code — so the step only runs on release, never on a PR. Simplest fix: gate it on steps.filter.outputs.code != 'false', the same condition the "Run Lite fast tests" step uses.
3. No build step. Lite.Tests and Installer.Tests each get a dotnet build … --no-restore step before their test step; Dashboard.Tests doesn't, but the test step uses --no-build. Add a matching Build Dashboard.Tests step.

Still open from the original review

• Add possibility to exclude WAITFOR() from Top Queries view #4 — the unrelated PerformanceMonitor.sln edit (VisualStudioVersion = … d17.14 → … 36930.0) is still in the diff; please revert just that line (the new Dashboard.Tests project entries are correct, keep those).
• Lite: collector health status bar shows global errors instead of per-server #5 — ServerVersionInfo still keeps its own _installedVersion field, OnInnerServerPropertyChanged still doesn't forward InstalledVersion, and the entry.InstalledVersion = …; entry.Server.InstalledVersion = … dual-write is now in four places. Forwarding InstalledVersion in the switch (and dropping the private field) removes all four.
• Lite overview: show Online/Offline status #6 — Closing now cancels the CTS 👍, but it doesn't await the tasks, and AppendUpgradeLog / centralProgress still call Dispatcher.Invoke unguarded — an in-flight upgrade will post to a dead dispatcher after the window closes. A Dispatcher.HasShutdownStarted check guards it.
• Lite: per-server collector health in status bar #7 — the progress bar still writes each server's raw ProgressPercent, so it'll jump around under parallel upgrades. Count-based (X of N complete) is steadier.
• Add "Max Duration" column to all query lists (Top Queries, Top Procedures, Top Querystore ...) #8 — width went 960 → 1024, but with Upgrade/Cancel visible the button row is ~1230px wide. Either ~1280 default width or a WrapPanel.
• [FEATURE] Make the application "High DPI" aware #9 — successCount / failCount in UpgradeAll_Click are still declared and unused (CS0219).
• Feature/high dpi awareness #10 — still present: using ModelContextProtocol.Protocol;, both // extract the existing version logic into a helper comments, // existing version comparison logic follows unchanged..., and // Replace the existing ServerVersionInfo type with this implementation.... (Dashboard.Tests.csproj also has a stray <!-- Ensure this path matches your dashboard project file -->.)
• Dashboard: Performance Trends graphs missing legends when no data #11 — catch { } in ManageServersWindow_Closing is still silent.
• [BUG] Wait Stats tab "Select All" doesn't select all wait types #12 — PerServerUpgradeResult is still an unsealed private record.
• Idea: Web-based dashboard for remote monitoring access #13 — EntraMFA servers still go into the same parallel Task.WhenAll as the rest; that's N interactive auth prompts at once. Serialize the EntraMFA ones or warn before launching.

One new thing

LoadServers() now rebuilds _lastVersionCheckResults with NeedsUpgrade = false, but it doesn't hide UpgradeAllButton or reset OutdatedCount. So if you run Check All Versions, then Edit / Toggle Favorite / Remove a server (all call LoadServers()), the "Upgrade N Servers" button stays visible — but clicking it now finds nothing to upgrade, and the confirm dialog shows a stale count. Hiding the button (and zeroing OutdatedCount) inside LoadServers() keeps it consistent.

Nice progress on the core flow — the parallel RunUpgradeAsync orchestration and partial-failure handling read well. Flag it for review whenever you're ready and happy to take another look.