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.

[erikdarlingdata]

Round 3 — almost there 🎯

Thanks for grinding through the round-2 list, @mas-mark-y — most of it is genuinely done. Confirmed resolved: #1 (base branch → dev, net10.0-windows7.0), #2 (xunit.v3 3.2.2), #3 (CI: lock file + code != 'false' gate + Build Dashboard.Tests step all present), #4 (.sln back to just the test-project entries), #5 (proxy _installedVersion field dropped + InstalledVersion now forwarded in the switch), #7 (per-server progress averaging), #9 (dead successCount/failCount gone), #10 (stale comments + unused using gone), #11 (no more silent catch), #12 (sealed record).

The core flow reads well — parallel RunUpgradeAsync, partial-failure aggregation, and the cancellation wiring all match the existing AddServerDialog install path. I verified the integration seams too (BuildConnectionString, GetCredential, AuthenticationTypes.*, and the double-targetVersion in LogInstallationHistoryAsync is correct — both params are the installed version; the previous version is derived internally).

I split what's left into two buckets: a short list to fix before merge, and a longer list of polish that we'll clean up on our side after it merges — you don't need to touch those.

🔴 Before merge

1. Merge conflict. The PR is currently conflicting with dev (it's moved since you branched) — real conflicts in build.yml, ManageServersWindow.xaml, ManageServersWindow.xaml.cs, and Models/ServerConnection.cs. Please merge/rebase dev in and re-test.
2. Idea: Web-based dashboard for remote monitoring access #13 — EntraMFA + parallel. EntraMFA servers still go into the same parallel Task.WhenAll, so a mixed batch pops N interactive MFA prompts at once and the confirm dialog doesn't warn. Either run the EntraMFA servers serially after the Windows/SQL batch, or warn the user before launching.
3. Stale "Upgrade All" button. LoadServers() rebuilds the list with NeedsUpgrade = false but doesn't hide UpgradeAllButton or reset OutdatedCount. So after Check All Versions → Edit / Toggle Favorite / Remove (all call LoadServers()), the "Upgrade N Servers" button stays visible with a stale count and now upgrades nothing. Hiding the button + zeroing OutdatedCount inside LoadServers() covers it.

That's the whole blocker list.

🧹 Post-merge cleanup (on us — no action needed from you)

Recording these so we don't lose them; we'll fold them into a maintainer follow-up after merge:

• Add "Max Duration" column to all query lists (Top Queries, Top Procedures, Top Querystore ...) #8 (residual): at 1280px the button row still clips ~70px during an active upgrade — with Upgrade All + Cancel both visible the row is ~1230px, but column 0 only has ~1150px after the 16px margins + the 80px Close button. A WrapPanel (or hiding low-priority buttons mid-upgrade) is the durable fix.
• Lite overview: show Online/Offline status #6 (residual): AppendUpgradeLog got the Dispatcher.HasShutdownStarted guard but centralProgress didn't, and Closing cancels without awaiting in-flight tasks.
• Lite: per-server collector health in status bar #7 (residual): the progress average divides by totalServersToUpgrade (the NeedsUpgrade count) instead of upgradeTargets.Count, so the bar stops short if a server fails credential prep.
• Lite: collector health status bar shows global errors instead of per-server #5 (residual): now that the proxy passes through, the entry.InstalledVersion = x; entry.Server.InstalledVersion = x; dual-writes are no-ops, and the foreach in CheckAllVersions_Click re-writing Server.InstalledVersion is dead code.
• new ServerVersionInfo { ... } depends on Server being initialized before InstalledVersion (the setter dereferences _server) — fine today, fragile if the initializer order ever changes.
• The grid isn't refreshed after a successful bulk upgrade (still shows old versions until you re-run Check All Versions).
• Exclude WAITFOR queries from top queries views #14: the nested Dispatcher.Invoke inside the Progress<T> handlers is redundant (they already marshal to the UI thread).
• Nit: "An unexpected error while closing the window." → "...error occurred while...".

Once the three blockers are in and the conflict's resolved, flag it for review and we'll get it merged. Nice work sticking with this.

[erikdarlingdata]

Round 4 — the three blockers are still open

Thanks for the latest push, @mas-mark-y. I can see the work that went in — the progress-bar averaging, the window-width bump, and the close handler. Those are the post-merge polish items I'd marked "on us," and I appreciate you taking a swing at them. The catch is that the three things I'd flagged as 🔴 before-merge blockers are all still open, so the PR isn't mergeable yet. Point by point so it's easy to cross-reference.

🔴 Still blocking

1. Merge conflict — and it's grown. The branch still conflicts with dev (mergeable: CONFLICTING). dev has moved ~500 commits past where you branched, and the conflict set is now 7 files, up from 4:

.github/workflows/build.yml
.gitignore                               (new)
Dashboard.Tests/Dashboard.Tests.csproj   (new, add/add)
Dashboard/ManageServersWindow.xaml
Dashboard/ManageServersWindow.xaml.cs
Dashboard/Models/ServerConnection.cs
PerformanceMonitor.sln                   (new)

The Merge branch 'feature/815-bulk-server-upgrades'… commit merged your fork's copy of this branch into itself (cleaning up a rebase divergence) — it didn't pull in dev. This one gates everything else, so it's worth doing first: merge the latest dev in, resolve, and re-test.

2. #13 — EntraMFA + parallel. Still unaddressed. Every NeedsUpgrade server (EntraMFA included) goes into the one parallel Task.WhenAll, and the confirm dialog still reads "All servers will be upgraded in parallel" with no MFA warning — so a mixed batch still pops N interactive prompts at once. Either run the EntraMFA servers serially after the Windows/SQL batch, or warn in the confirm dialog before launching.

3. Stale "Upgrade All" button. Still there. LoadServers() rebuilds the list with NeedsUpgrade = false but doesn't hide UpgradeAllButton or zero OutdatedCount — so after Check All Versions → Edit / Toggle Favorite / Remove, the "Upgrade N Servers" button stays visible with a stale count and now upgrades nothing. Two lines inside LoadServers() cover it:

UpgradeAllButton.Visibility = Visibility.Collapsed;
OutdatedCount = 0;

🟡 The polish you did land (with one residual each)

These were on my "we'll clean up after merge" list, so no obligation — but since you took them on, here's where they stand:

• Lite: per-server collector health in status bar #7 (oscillation): the per-server ConcurrentDictionary + averaging genuinely fixes the bar jumping backwards as servers report out of order — nice. One thing left: it divides by totalServersToUpgrade (the NeedsUpgrade count) instead of upgradeTargets.Count, so if a server fails credential prep the bar caps just short of 100%. (The completion line right below already uses upgradeTargets.Count, so the two denominators now disagree.)
• Add "Max Duration" column to all query lists (Top Queries, Top Procedures, Top Querystore ...) #8 (button row): Width="1280" fixes the idle overflow. During an active upgrade, though — when Upgrade All + Cancel are both visible — the row is ~1230px but column 0 only has ~1168px after the 16px margins and the 80px Close button, so it still clips ~60px. The horizontal StackPanel won't compress; a WrapPanel is the durable fix.
• Dashboard: Performance Trends graphs missing legends when no data #11 (close handler): the catch (Exception) does the job. Tiny thing: the message reads "An unexpected error while closing the window." — still missing the "occurred" (→ "An unexpected error occurred while closing the window.").

Path to merge

Cleanest split: you take 1, 2, and 3 (the blockers), and leave the #7/#8 residuals to me — they were on my side of the line anyway, and I'd rather you not round-trip on them. Once dev is merged in and the EntraMFA + stale-button fixes are in, flag it and I'll get it merged.

Thanks for sticking with this — the core flow is solid, it's the edges that are left.

[erikdarlingdata]

Thanks for the iterations on this. The bones are solid — cancellation is wired correctly (CTS + Closing handler + Cancel button), AppendUpgradeLog marshals to the dispatcher properly, and version probing is parallel with sensible "Unreachable" handling. A handful of things to address before this is mergeable, worst-first.

1. Line-ending churn is masking the real diff

build.yml, ServerConnection.cs, and ManageServersWindow.xaml.cs are re-emitted in their entirety (CRLF/LF). ServerConnection.cs shows the whole class removed and re-added identically — the only real change is the appended ServerVersionInfo class. ManageServersWindow.xaml.cs is @@ -1,502 +1,1034 @@, so ~530 lines of actual new code are buried inside a full-file rewrite. This is almost certainly why issues are surviving review rounds — the real changes aren't visible. Please renormalize against the repo's CRLF .gitattributes:

git add --renormalize .
git commit -m "Normalize line endings"

That should collapse these down to just the real changes.

2. Event-subscription leak (ServerVersionInfo)

The Server setter does _server.PropertyChanged += OnInnerServerPropertyChanged with no matching -= anywhere. LoadServers() (called on every Add/Edit/Remove/Toggle-Favorite) and CheckAllVersions_Click both build fresh ServerVersionInfo wrappers over the long-lived ServerConnection instances. Each rebuild subscribes a new wrapper that the ServerConnection then keeps alive forever — the old wrappers never get collected, and this accumulates for the window's lifetime. Make ServerVersionInfo IDisposable (unsubscribe in Dispose) and dispose the previous list before replacing it — or drop the subscription entirely, since the code already sets entry.InstalledVersion directly in the paths that matter.

3. The bulk summary never reports failed servers

UpgradeAggregator returns "N servers upgraded (X steps succeeded, Y steps failed)" — ServerFailCount is computed but left out of the string, and UpgradeProgressText only shows "X of N upgraded." For a bulk production schema upgrade, the count of servers that failed is the single most important thing to surface; right now it lives only in a log line. (Minor: "steps succeeded" is hard-coded plural, so a single step reads "1 steps succeeded.")

4. UpgradeAggregator input semantics

AggregationInput carries both UpgradesSucceeded/Failed and StepsSucceeded/Failed, and Aggregate simply sums them. Two distinct concepts (migrations vs. install files) collapse into one "steps" total, which makes the summary ambiguous. Either keep them separate in the output, or document why they're merged.

5. Design call: parallel-all upgrades + partial-failure posture

UpgradeAll_Click runs every outdated server's ExecuteAllUpgradesAsync + ExecuteInstallationAsync concurrently via Task.WhenAll, and a per-server partial failure is recorded Success=false while the rest continue — no sequencing or stop-on-failure at the bulk layer. For production schema changes, a bad migration would hit all servers at once rather than failing on one and halting. This is worth a deliberate decision: parallel-all vs. sequential / stop-on-first-failure, and whether half-upgraded servers need a clearer surfaced state than a log line.

6. Parallel Entra MFA = concurrent interactive auth prompts (confirmed)

The confirm dialog calls out that Entra MFA servers upgrade "in parallel." InstallationService.BuildConnectionString sets Authentication = SqlAuthenticationMethod.ActiveDirectoryInteractive when useEntraAuth is true, so upgrading N MFA servers in parallel will launch N simultaneous interactive auth prompts — which the interactive flow doesn't handle well (it expects a single foreground request). MFA servers should be serialized (at least for the initial auth) rather than fired off together in Task.WhenAll.

7. Smaller items

• Duplicate using directives (...Models / ...Services each appear twice) → CS0105 warnings.
• UpgradeAll_Click dereferences _lastVersionCheckResults before its own null-check a few lines later — harmless today (the ctor always sets it) but contradictory.
• Stale grid after upgrade: no re-probe/refresh, so the Installed Version column still shows pre-upgrade values until the user clicks "Check All Versions" again.
• DRY: GetAppVersion() exists, but CheckForUpdates_Click re-implements the same version-stripping inline.
• Tests: only the trivial aggregator (counts + string formatting) is covered. The version-comparison / NeedsUpgrade logic and the failure aggregation are extractable and worth testing — that's where the risk actually is.
• Missing trailing newline on the two new files.