Add Bulk Server Upgrades - #815#963
Conversation
…g and bulk-upgrade UI; remove duplicate type
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
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
-
Wrong base branch. This project's feature branches always go to
dev(anddevmerges tomainonly at release time — apologies, that's not in CONTRIBUTING.md and probably should be). Concrete impact:devis on .NET 10,mainis still .NET 8. The new test project targetsnet8.0-windows, andInstaller/packages.lock.jsonis bumped from ILLink 8.0.26 → 8.0.27 (an 8.x patch). Whendevnext merges tomain, both conflict and the test project won't build against the .NET 10 codebase. Please rebase ontodevand retarget the PR there. -
Test project versions don't match the rest of the repo.
Dashboard.Tests.csprojusesxunit 2.6.1+xunit.runner.visualstudio 3.1.5. The existingLite.TestsandInstaller.Testsusexunit.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 toxunit.v3 3.2.2. -
Tests aren't wired into CI.
Dashboard.Testsisn't added to.github/workflows/build.yml— see the existingRun Lite fast tests/Run Installer testssteps for the shape. -
Solution file noise.
PerformanceMonitor.slnhas an unrelated edit (VisualStudioVersion = 17.14.36930.0 d17.14→... 17.14.36930.0) — looks like VS install drift. Revert.
Correctness
-
ServerVersionInfoproxy dropsInstalledVersionchange notifications.OnInnerServerPropertyChanged(Models/ServerConnection.cs ~290) forwardsDisplayName, ServerName, AuthenticationDisplay, MonthlyCostUsd, LastConnected, IsFavorite— but notInstalledVersion, even thoughServerConnection.InstalledVersiondoes raise PropertyChanged. The code papers over this with a defensive dual-write everywhere (entry.InstalledVersion = ...; entry.Server.InstalledVersion = ...). Cleaner: either addInstalledVersionto the switch and drop the redundant private field, or makeServerVersionInfo.InstalledVersiona pure passthrough getter/setter onServer.InstalledVersion.(
AuthenticationDisplayis the same shape — read-only computed onServerConnection, never raises PropertyChanged. The proxy forwarding for it is dead code. Not blocking, just confusing.) -
Closing the window mid-upgrade leaks running tasks.
ManageServersWindow_Closingcancels the CTS and returns; the window dies, but the upgrade tasks keep running and continue posting to a dead Dispatcher viaProgress<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(...)). -
Progress bar oscillates under parallel upgrades.
centralProgresswritesUpgradeProgressBar.Value = p.ProgressPercent.Valuefrom 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. -
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. -
Dead variables.
ManageServersWindow.xaml.cs:643-644—int successCount = 0; int failCount = 0;declared inUpgradeAll_Clickand never used. Triggers CS0219. -
Stray editorial comments / using:
using ModelContextProtocol.Protocol;at the top ofManageServersWindow.xaml.cs— unused, looks like an IDE auto-import.// extract the existing version logic into a helperappears 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 priorServerVersionInfo; onmainthere is none. Remove.
-
catch { }inManageServersWindow_Closing. Swallows everything silently. At minimum, log via the existingAppendUpgradeLog. -
PerServerUpgradeResultshould besealed(CA1852).
Design / UX
-
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.
-
Dispatcher.InvokeinsideProgress<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 nestedDispatcher.InvokeincentralProgressand theDispatcher.CheckAccess()path inAppendUpgradeLogaren't wrong, just belt-and-suspenders. -
Tests cover only the aggregator.
UpgradeAggregator.Aggregateis ~25 lines of trivial counting; the orchestration inUpgradeAll_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
- Rebase onto
devand retarget the PR. Dashboard.Tests.csproj→net10.0-windows+xunit.v3 3.2.2; wire intobuild.yml.- Revert the unrelated
.slnedit. - Drop the stale comments, the unused
using, and the two dead variables. - Fix the proxy's
InstalledVersionforwarding so the dual-write isn't needed. - Grow the default window width or
WrapPanelthe new buttons. - Either rate-limit/serialize EntraMFA upgrades or warn before launching them in parallel.
Happy to look again once these are addressed.
…g and bulk-upgrade UI; remove duplicate type
…g and bulk-upgrade UI; remove duplicate type
…nInfo, add version probing and bulk-upgrade UI; remove duplicate type
|
@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 :-) |
|
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 ✅
#3 — CI wiring needs three more piecesThe
Still open from the original review
One new thing
Nice progress on the core flow — the parallel |
…, ensure tests run on release and PR
… from the original PR review
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 → The core flow reads well — parallel 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
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:
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. |
…g and bulk-upgrade UI; remove duplicate type
…g and bulk-upgrade UI; remove duplicate type
…nInfo, add version probing and bulk-upgrade UI; remove duplicate type
…, ensure tests run on release and PR
… from the original PR review
…g and bulk-upgrade UI; remove duplicate type
…g and bulk-upgrade UI; remove duplicate type
…g and bulk-upgrade UI; remove duplicate type
… from the original PR review
…/mas-mark-y/PerformanceMonitor into feature/815-bulk-server-upgrades
Round 4 — the three blockers are still openThanks 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 blocking1. Merge conflict — and it's grown. The branch still conflicts with The 2. #13 — EntraMFA + parallel. Still unaddressed. Every 3. Stale "Upgrade All" button. Still there. 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:
Path to mergeCleanest 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 Thanks for sticking with this — the core flow is solid, it's the edges that are left. |
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
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
usingdirectives (...Models/...Serviceseach appear twice) → CS0105 warnings. UpgradeAll_Clickdereferences_lastVersionCheckResultsbefore 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, butCheckForUpdates_Clickre-implements the same version-stripping inline. - Tests: only the trivial aggregator (counts + string formatting) is covered. The version-comparison /
NeedsUpgradelogic and the failure aggregation are extractable and worth testing — that's where the risk actually is. - Missing trailing newline on the two new files.
|
Sorry to bother you, @erikdarlingdata, Re: 2. Event-subscription leak (ServerVersionInfo) - Dashboard/Models/ServerConnection.cs line 215 has |
Round 6 — the line-ending fix paid off immediatelyThanks @mas-mark-y. And it paid off on the first honest read: there's a data-integrity bug in the upgrade path that all five previous rounds missed, because none of us could actually see the code. It's below — and a chunk of it is our fault, not yours. Everything here is verified against ✅ Fixed this round
Your question about #2 (the event-subscription leak)
Good question, because that line genuinely looks like it covers it. It doesn't, for two reasons. 1. It never runs. It sits in the
On a brand-new wrapper 2. The publisher outlives the subscriber — for the whole app session.
So every Don't reach for 🔴 The big one: a bulk upgrade can skip every migration and still stamp the DB as upgradedThree defects compose into this. 1. An unparseable version silently means "no upgrades needed." if (currentVersion == null) return [];
if (!Version.TryParse(currentVersion, out var currentRaw)) return [];So if 2. get => _server!.InstalledVersion;
set { if (_server!.InstalledVersion == value) return; _server!.InstalledVersion = value; ... }…while
There's an async variant too: the ctor's fire-and-forget 3. The history row is stamped SUCCESS from install-file results only. await InstallationService.LogInstallationHistoryAsync(
installerConnectionString, targetVersion, targetVersion,
installResult.StartTime,
installResult.FilesSucceeded, // install files only - excludes upgrade scripts
installResult.FilesFailed, // ditto
installResult.Success, // ditto <-- this is the bug
progress);
Why it's permanent. So once a skipped or failed hop is recorded SUCCESS at the target version, it is never offered again. The database sits permanently at the new version number with the Defect 3 is on us, not you.
The CLI honors it ( I've filed #1497 and I'm fixing
And for the leak: give 🔴 Also blockingMerge conflict — trivial now, but there's a landmine behind it. Down to one file (
"Upgrade All" is dead after any run. Closing the window orphans the installers — and allows a second concurrent run. Bulk skips every post-upgrade refresh the single-server path does. Credential-less servers vanish from the run and from the denominator. #6 EntraMFA — I do need the serialization. You did add the confirm-dialog warning, and that was one of the two options I offered in Round 4, so moving the bar without saying so was on me. Saying it plainly now: N concurrent Also, Four new build warnings. The build is otherwise green (0 errors):
CS8629 is latent, not live — every current reporter in The tests assert on a string the app never shows. 🧹 Still on me — don't touch these
Where this leaves usStraight with you: this is a bigger lift than Round 5 implied, and part of that is because our own single-server path had the bug you copied. I don't want it to feel like the goalposts keep moving, so that list above is the whole thing — I'm not adding to it. Yours: merge Mine: #1497, and everything under 🧹. If any of that is more than you want to take on, say the word and I'll finish it on a branch off yours with your commits intact. You've put real work into this and I'd much rather land it than let it rot. |
Round 6's fixes all held under verification, and the invariant survived. Seven more findings; three mattered a lot. 1. CRITICAL -- RestoreAfterInstall left the form permanently disabled on the exact path this PR exists to add. TransitionToState(Installing) calls SetFormEnabled(false); Round 6 replaced the exit paths' TransitionToState(Initial) (which re-enables) with a restore to the pre-install state -- and Connected_NeedsUpgrade and Connected_Current never call SetFormEnabled(true). I had added it to Connected_NoDatabase and Connected_StatusUnknown and missed the only two states the restore actually lands in. So EVERY upgrade abort left the server box, all credential fields, Test Connection and Check for Updates dead for the life of the dialog -- and if the failure was a login problem, the field you need to fix is the one you cannot touch. 2. HIGH -- ResetScheduleCheckBox was never cleared anywhere, and it TRUNCATEs config.collection_schedule. Same bug class Round 6 closed for the clean-install box, on the third checkbox behind the same collapsed panel: tick it for SERVER-A, retype the box to SERVER-B, and it wipes B's tuned intervals from an invisible tick with no consent. Cleared with the other mode flags. 3. HIGH -- RepairOutcome trusted the "1.0.0" UNKNOWN sentinel as a real version. It is GetInstalledVersionAsync's guess for "installed, but I cannot read the version", and it sorts below every real version -- so "is an upgrade pending?" answered yes unconditionally, and every REAL repair failure on such a server was reported as expected and exited 0. A schema-current 3.1.0 server whose history rows are all FAILED, with four genuinely broken procedures, would have passed a %ERRORLEVEL% gate while telling the operator to ignore the errors. The sentinel is now a named constant and explicitly rejected. 4. My InstallBlockReasonTests could never fail a PR: Dashboard.Tests is not wired into CI at all (749 tests that never run; #963 is what fixes that). Tests that cannot fail a PR are decoration -- so the DECISION moved to Installer.Core/InstallGuard, which IS CI-wired, and the CLI's three hand-copied guards now share it. Only the wording is per-surface now. 5. VersionDetectionTests hand-replicates GetInstalledVersionAsync's SQL, and Round 6 changed the method without touching the replica -- so the test file named for this exact bug class was certifying the PRE-change behavior while the riskiest line in the diff had zero coverage. Replica updated; both shapes now pinned (empty database -> null, ledger-lost database -> sentinel). Compiled, not run: these hit a real server. 6. The CLI's self-version check sat inside the upgrade branch, so --reinstall -- a FRESH install, the exact case its own comment cites -- skipped it. Hoisted. 7. The Connected_Current button labelled "Repair" does NOT take the repair path (the checkbox is unreachable there, so it writes a history row). Renamed "Reinstall Objects"; "Repair" now names one thing. And RestoreAfterInstall no longer hides the install log -- it re-shows the panel with Advanced Options DISABLED, since WPF propagates IsEnabled to children, so the clean-install box stays untickable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Bulk Server Upgrades
This is an attempt to implement a new feature requested in #815
Which component(s) does this affect?
How was this tested?
Checklist
dotnet build -c Debug)