Abort the install when an upgrade migration fails (#1497)#1498
Merged
erikdarlingdata merged 41 commits intoJul 13, 2026
Conversation
AddServerDialog logged "Continuing with full installation to ensure consistency..." when an upgrade script failed, ran the full install over the partially-upgraded database anyway, and then wrote an installation_history row using _installResult.Success -- which is FilesFailed == 0 for the install files only and excludes upgrade failures entirely. Because GetInstalledVersionAsync reads the most recent installation_status = 'SUCCESS' row and FilterUpgrades only offers hops with ToVersion > current, that SUCCESS row at the target version stranded the failed migration permanently: the server reported as current with the upgrades/* ALTERs never applied, and the hop was never offered again. ExecuteAllUpgradesAsync already documents the contract -- "The caller aborts the whole install when totalFailureCount > 0" -- and the CLI installer honors it (Program.cs). Only the Dashboard's single-server path did not. - AddServerDialog now aborts on upgrade failure without running the install or writing a history row, and folds the upgrade counts into the history write, matching the CLI. Leaving no SUCCESS row is what makes the failed hop get re-offered, and upgrade scripts are idempotent, so re-running after fixing the error resumes cleanly. - FilterUpgrades now throws ArgumentException on a present-but-unparseable version instead of returning an empty list. Empty means "no upgrades needed", which must never be the answer to "I could not read the version you gave me" -- that silent path is what lets a status string like "Unreachable" skip every migration and still report zero failures. - ExecuteAllUpgradesAsync translates that into a reported failure (0, 1, 0) so callers abort through the existing contract instead of hitting an unhandled exception; the CLI's Main has no outer try/catch. Adds seven regression tests to UpgradeOrderingTests covering blank, unparseable, and status-string versions through both ScriptProvider and ExecuteAllUpgradesAsync. Found while reviewing #963, whose bulk-upgrade path mirrors the same defect across N servers at once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three defects found in review of the previous commit.
1. The CLI's abort was nested inside `if (upgradeCount > 0)`, so the new
(0, 1, 0) "discovery failed" return skipped it entirely: the CLI printed
"No pending upgrades found." and ran the whole install anyway. The history
row was still written FAILED (totalFailureCount folds in upgradeFailureCount),
so nothing was stranded, but the install should not have run -- and the
Dashboard aborting while the CLI did not is the exact divergence this branch
set out to remove. The abort now sits outside the upgradeCount block, since
discovery can fail before any hop runs.
2. Version.TryParse("3.1") succeeds with Build == -1, and the Version(int,int,int)
ctor rejects that -- a two-part version threw ArgumentOutOfRangeException
(an ArgumentException, so the new catch would have reported it as an
unreadable version). Pre-existing latent crash; now clamped.
3. GetAppVersion strips a "+metadata" suffix but not "-prerelease". The moment
<InformationalVersion> becomes e.g. "3.2.0-rc1", targetVersion stops parsing
-- which, with the new throw, would hard-abort every upgrade. Rather than fix
the seven call sites that each strip suffixes by hand, FilterUpgrades now
normalizes at the choke point, mirroring SingleInstanceDecision.ParseProductVersion
(re-stated locally to keep Installer.Core dependency-free). Genuine garbage
like "Unreachable" still throws.
Also zeroes the progress bar on the Dashboard abort path, matching the cancel path.
Adds four regression tests: pre-release / build-metadata / combined suffixes on
the target version, and a two-part version.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Aborting on a failed migration removed an accidental self-healing path: an
upgrade script can fail precisely BECAUSE an object it ALTERs is missing or
damaged, and the full install running afterwards used to recreate it. With the
abort in place the only escape hatch left was Clean Install, which drops the
database.
Repair runs the idempotent install scripts WITHOUT running migrations, so the
missing objects come back and the pending upgrade can then be re-run.
Dashboard: "Repair: reinstall objects, skip upgrade scripts" in Advanced
Options, mutually exclusive with the destructive clean install.
The upgrade-abort message now points at it.
CLI: --repair, mutually exclusive with --reinstall (which would drop the
database, leaving nothing to repair).
The load-bearing detail: a repair must NOT record a SUCCESS history row at the
TARGET version. installer_version is what GetInstalledVersionAsync reads back,
and FilterUpgrades only offers hops newer than it -- so stamping the target here
would strand every pending migration, which is the exact bug this branch exists
to fix. Both paths record history at the version the database is still at, so the
upgrade is still offered afterwards.
Dashboard resolves that to `repairFromVersion` (the version being repaired FROM,
null when not repairing) rather than a bare bool, so the history write cannot
record the old version after a clean install even if both boxes were somehow set.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dialog-upgrade-abort # Conflicts: # CHANGELOG.md
Review of the repair commit found the SAME stranding bug still reachable through the Dashboard's front door, plus five smaller issues. 1. HIGH -- AddServerDialog's pre-install discovery called GetInstalledVersionAsync with the soft (null-returning) overload. GetInstalledVersionAsync's own comment names the hazard: "The installer passes throwOnError=true so a transient/ permission error can't be mistaken for 'database absent' and silently trigger a fresh install over an existing database (no upgrades, then logged as SUCCESS -- the #538 hazard). Soft callers (Dashboard version column, adversarial tests) keep the null fallback." But :537 is not the version column -- it is the discovery that decides install-vs-upgrade, the same class of caller as the CLI. A timeout, an OFFLINE/ RESTORING database or a permissions blip came back as null, which reads as "no database": upgrades skipped, install run over the existing database, history stamped SUCCESS at the target version, every pending hop stranded. Exactly the bug this branch exists to kill. Now passes throwOnError: true and drops into a new Connected_StatusUnknown state that hides the install button and explains why. A _databaseStatusUnknown field also hard-blocks InstallOrUpgrade_Click, so the guard is structural rather than depending on a hidden button. 2. Repair-loop trap: InstallComplete disables the expander RepairCheckBox lives in, so after a successful repair the box could not be unticked -- and the success message told the user to do exactly that. Clicking "Upgrade Now" would repair again, forever. Repair is now cleared automatically before the transition. 3. Connected_Current collapsed the install button, so a damaged-object server that was already at the current version had NO non-destructive recovery in the Dashboard, while --repair handled it fine. It now offers Repair; with current == target there are no migrations to skip, so it is a plain idempotent reinstall. 4. CLI put currentVersion (an installer_version value) into installer_info_version on repair. That column records which binary ran, so infoVersion passes through. 5. Dashboard summary report claimed appVersion after a repair, contradicting its own history row. Now uses historyVersion. 6. CLI's hoisted currentVersion reset moved in with its sibling per-iteration resets, so a clean-install iteration can never reuse the prior one's version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…opened Two independent adversarial reviews converged on the same critical defect in the previous commit, plus a stale-state hole neither the abort nor the repair covered. 1. CRITICAL -- Connected_Current is a catch-all, not "installed == target". The routing sends installedVer >= appVer AND the unparseable case into it. On dev that was harmless: the state collapsed the install button, so no action was reachable. The previous commit un-hid the button (labelled "Repair") for all three. A 3.1.0 Dashboard against a server a colleague upgraded to 3.2.0 lands in Connected_Current, reports "up to date", and offers Repair. Clicking it runs the 3.1.0 install scripts over the 3.2.0 schema -- every CREATE OR ALTER proc and view reverts to its older body -- then stamps installation_history SUCCESS at 3.1.0. A silent downgrade: this branch's own bug class, inverted. Connected_Current now means installedVer == appVer, exactly. A newer server and an unparseable version both block instead, via _installBlockedReason (which replaces the _databaseStatusUnknown bool and carries the reason to the UI). 2. CRITICAL -- the install path trusted cached dialog state, not the connection it was about to write to. BuildInstallerConnectionString() reads ServerNameTextBox live, but _installedVersion was only ever assigned in DetectDatabaseStatusAsync. Retype the server box after a Test Connection and click straight through, and the installer writes to server B while reasoning about server A's version -- and since the cached version is what suppresses hops, that runs zero migrations and stamps SUCCESS at the wrong version, stranding server B's upgrades for good. InstallOrUpgrade_Click now re-reads the version against the connection it is about to write to, with throwOnError: true. That also closes the TOCTOU between discovery and install for every state, not just the new one. 3. Connected_Current no longer shows InstallationPanel. That panel holds the "drops existing database" clean-install checkbox, which must never sit behind a button labelled Repair. 4. Sticky mode flags. AdvancedOptionsExpander was re-enabled only on the abort, cancel and fatal paths, so after a completed install it stayed disabled for the dialog's life -- with whatever mode checkboxes were ticked frozen inside it. And Repair was cleared only on the SUCCESS branch. Combined: a failed repair left Repair ticked and un-untickable, and every later click repaired again, skipping migrations. Worse, the form is re-enabled at InstallComplete, so the server box could be retargeted with a ticked checkbox carried onto a server that never consented -- "Clean install" included, which drops the database. The expander is now unfrozen in the transition reset block, and both mode checkboxes are cleared on every completed run and on every fresh detection. 5. NormalizeVersion stripped +metadata but not -prerelease, and formatted a two-part version as "3.1.-1". A 3.2.0-rc1 InformationalVersion therefore failed to parse and sent EVERY server to "up to date". Now strips both suffixes and clamps Build, matching ScriptProvider.ParseVersionCore. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… of the guard Both Round-2 reviewers converged on the same two criticals. The Round-1 fixes were incomplete in exactly the way that mattered. 1. CRITICAL -- the install-time re-probe refreshed _installedVersion but never re-ran the SAFETY VERDICT on it. Only the throw case was handled; the "installed is NEWER than this build" case was still decided by _installBlockedReason, a value computed by the last Test Connection against the server name as it was then. ServerNameTextBox has no TextChanged handler, so retargeting invalidates nothing. That case is invisible to every other guard: FilterUpgrades selects ToVersion > current && ToVersion <= target, so installed > target yields zero hops AND zero failures -- the abort never fires. The install then runs this binary's older scripts over the newer database, reverting every CREATE OR ALTER proc and view, and writes SUCCESS at the LOWER version. No user error needed: test a server, have a colleague upgrade it with a newer build, click Upgrade Now. The classification is now one method, GetInstallBlockReason, called by BOTH the detect path and the install click -- against the connection it is about to write to. It also closes a second leak: repair took repairFromVersion straight from the un-revalidated probe, so a garbage installer_version could be written back verbatim. 2. CRITICAL -- the re-probe put an await in front of the only re-entrancy guard. TransitionToState(Installing) used to be this handler's first statement; it collapses DatabaseStatusPanel, the Border InstallUpgradeButton lives in, and that collapse is the ONLY thing that makes the button unclickable (SetFormEnabled never touches it). Awaiting a network round trip in front of it left the button live for the whole connect timeout -- unbounded for interactive Entra -- and the handler is async void. Two clicks meant two concurrent installs: the second clobbers _installCts, so the first's finally disposes the second's source and nulls the field, leaving Cancel a silent no-op on the run that is actually still going; _installResult is clobbered; two history rows; and with clean install ticked, one DROP racing another's install. Cancel/ESC during the probe was also a no-op because _installCts did not exist yet -- the dialog closed and the install ran on regardless. The UI is now frozen and the CTS armed BEFORE the first await, and the probe takes the token. 3. HIGH -- the CLI had no ahead-of-installer guard at all, and --repair skipped the only thing that validated the recorded version (ExecuteAllUpgradesAsync, hence ParseVersionCore) before writing it straight back as installer_version. An unreadable value got re-persisted, after which every non-repair run aborted forever and only --repair "worked". Both are now checked up front, for repair too. 4. MEDIUM -- a successful repair leaves migrations unapplied by design, but InstallComplete collapses the panel InstallUpgradeButton lives in, so the completion message pointed at a button that was not on screen; the obvious next click was Save & Connect, leaving the half-migrated database this feature exists to escape. ScriptProvider.TryParseVersionCore is now public (null instead of throwing) so the Dashboard and CLI share one parser rather than drifting. 16 new parser tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…all doors Two more reviewers, six more findings. The invariant held throughout -- these are about the repair feature being honest, and about doors left open around it. 1. Repair on a database with pending migrations reports errors, and that is INHERENT, not a bug. Verified against SQL2022: install scripts compile against the CURRENT schema, and ALTER PROCEDURE binds columns at compile time, so install/23_process_blocked_process_xml.sql -- whose body reads collect.blocking_BlockedProcessReport.monitor_loop, a column the 3.0.0-to-3.1.0 migration adds -- fails with Msg 207 "Invalid column name" on a 3.0.0 database. install/02 only creates that table IF OBJECT_ID IS NULL, with no column backfill (contrast the /*Add columns for existing installs*/ pattern it does carry for collect.memory_stats). Nothing is damaged -- a failed CREATE OR ALTER leaves the old body intact, the run records PARTIAL rather than SUCCESS, and the upgrade's own install pass recompiles them once the schema is current. But the completion path GATED the "run the upgrade next" handoff on _installResult.Success, so the user was left staring at "completed with N error(s)" and no next step -- landing precisely the half-migrated database the feature exists to escape. The handoff is no longer gated, and the expected errors are explained rather than presented as failure. 2. Repair now writes NO installation_history row, in both apps. It changes no version, and that table is the version ledger -- echoing back a version we merely READ is how a guess becomes a fact. GetInstalledVersionAsync returns "1.0.0" as the #538 fallback when a database exists with no SUCCESS row, meaning "unknown, try every upgrade"; repair was persisting that sentinel as a SUCCESS row, i.e. materializing the guess as truth -- the same failure mode this branch exists to prevent. Writing nothing leaves the previous row as the version of record, so the pending upgrade is still offered. 3. CheckForUpdatesButton was never disabled by SetFormEnabled, so it stayed live during Installing. Clicking it re-ran detection, which transitions back to a Connected_* state -- re-showing the install button and collapsing the running install's progress panel -- and a second click then started a CONCURRENT install against the same database. The Round-2 comment reasoned that the panel collapse was "the ONLY thing that makes the button unclickable"; the identical sentence was true of this button and was not followed through. One line in SetFormEnabled. 4. The cancel and fatal paths land in Initial, which did not clear the mode checkboxes -- so my own "cleared on EVERY outcome" comment was false. Tick "Clean install (drops existing database)", have the install throw, retype the server box, click again: it DROPS a database that was never consented to. Cleared in Initial as well as InstallComplete. 5. Cancelling during the install-time probe landed in BlockInstall, not the cancel handler: SqlCommand does not surface cancellation as an OperationCanceledException -- it faults with a SqlException ("Operation cancelled by user."). The user's own Cancel was reported as an unknown-database-state block that hid the install button. 6. The repair handoff never appeared for SQL auth: InstallComplete chains straight into MonitoringCredentials, which does not re-show the panel either -- so the auth mode most likely to hit this got the dead end. The summary report also named the database's version where it prints "Installer Version", which identifies the binary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g about repair Both Round-4 reviewers converged again. The two worst both end in a DROPPED DATABASE by different routes. 1. CRITICAL -- concurrent install via an IN-FLIGHT detection. Round 3 disabled CheckForUpdatesButton during Installing, which closed "start a detection during an install". It did not close "a detection that was ALREADY running when the install started": its continuation transitions back to a Connected_* state, which un-collapses DatabaseStatusPanel over the running install, re-shows the install button, re-enables Advanced Options -- and both detection paths then re-enable their buttons in unconditional finally blocks, overwriting the guard. From there: expand Advanced Options mid-install, tick "Perform clean install (drops existing database)", click Upgrade Now -- and CleanInstallAsync DROPS the database out from under the running install. Even without the tick, the second run disposes the first's still-in-use CancellationTokenSource and leaves Cancel pointing at the wrong run. Guarded structurally on _currentState == Installing: the detection bails before touching any UI, both finallys stop re-arming, and the click handler refuses to start a second install. 2. HIGH -- the CLI told the truth to nobody. A --repair on a behind database is DESIGNED to fail some install files (Msg 207: the install scripts compile against the current schema, and ALTER PROCEDURE binds columns at compile time). The Dashboard says so in as many words. The CLI reported it as PartialInstallation, exit code 4, "Installation completed with N error(s). Review errors above" -- so a script gating on %ERRORLEVEL% treats a good repair as failed, and the operator reading "repair failed" reaches for --reinstall, which DROPS the database. That is the precise destructive outcome this feature exists to avoid. The CLI now distinguishes the expected case, prints the same handoff, and exits Success. 3. HIGH -- "Repair completed with expected errors" was asserted for failures that are NOT expected. ExecuteInstallationAsync aborts the whole pass on a critical file (01_/02_/03_), so a repair that died on 02_create_tables.sql reinstalled nothing -- and the log read "[ERROR] Critical installation file failed. Aborting." immediately followed by "[OK] Repair complete... click Upgrade Now". Both surfaces now gate the "expected" wording, the handoff, and the exit code on Patterns.IsCriticalFile. 4. HIGH -- the repair handoff re-showed DatabaseStatusPanel in MonitoringCredentials, which also contains the "Skip, just add server" link -- unreachable there before. SkipInstall_Click flips _currentState to Initial, after which Save_Click silently saves the INSTALLER credentials instead of the monitoring credentials the user just typed. The link is now collapsed in the handoff. 5. GetInstallBlockReason is the pure decision core of both new blocks and had no tests. It is now internal (InternalsVisibleTo Dashboard.Tests already existed) and pinned by 16 cases, including the silent-downgrade case that produces zero hops and zero failures so nothing else catches it. It also now blames the BUILD, not the database, when it is the Dashboard's own version that will not parse -- and tells a user with an unreadable recorded version how to get unstuck. 6. Cancel is no longer armed outside Installing; CHANGELOG named a field that no longer exists and overstated the repair history rule (the Connected_Current "Repair" button has no pending migrations to skip, so it does write a row). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ls for sa Both reviewers again. The two worst were both caused by earlier rounds' fixes. 1. CRITICAL -- the repair handoff silently persisted the INSTALLER account as the monitoring credential. Round 4 put a live "Upgrade Now" button in front of MonitoringCredentials, a state that used to be terminal. Save_Click gated the monitoring-credential read on _currentState == MonitoringCredentials -- and every exit from the install that button starts (abort, cancel, fatal, version block) transitions away from it, without clearing the fields. Golden path, SQL auth: upgrade fails -> tick Repair -> repair succeeds -> handoff -> user unticks "Use same credentials" and types the low-privilege monitoring login -> clicks Upgrade Now -> the migration fails again -> abort -> _currentState is Initial -> Save. The typed login is discarded and the installer account, usually sysadmin, is written to Credential Manager as the ongoing monitoring credential. Now keyed on what the user actually entered, which cannot drift with state. 2. HIGH -- GetInstallBlockReason returned early for "no database" BEFORE validating the app's own version. appVersion is exactly what a fresh install writes to installation_history.installer_version, so a Dashboard with an unparseable InformationalVersion would poison a brand-new server's ledger at birth -- after which both surfaces refuse to touch it ever again, recoverable only by hand-editing the row or a destructive reinstall. The check now runs first, and is pinned. 3. CRITICAL (my Round-4 over-correction) -- repairUsable never checked that a pending upgrade EXISTS. The whole "these file failures are expected" story rests on there being a migration-added column to be missing. Without one, --repair against an up-to-date server reported "Installation completed successfully", printed "20 object(s) could not be compiled because the pending upgrade has not run yet" when there was no pending upgrade, and exited 0. Round 4 traded a false negative for a worse false positive: the %ERRORLEVEL% operator the fix existed to protect now saw SUCCESS with 20 broken procedures. Both apps now gate on installed < target. 4. The CLI's criticalFileFailed guard was provably dead -- a critical file already early-returns CriticalScriptFailed long before the summary -- so it made an over-broad flag look discriminating. Removed; the early return is the real guard. 5. --repair with no readable installation fell through to a FULL fresh install and stamped the target version: a whole new database on a mistyped server, or, where the database exists but its history table does not, a target-version stamp that strands every hop. Both surfaces now refuse. 6. Save_Click had no InstallInProgress guard, so a connection test still in flight when an install starts would fall through to DialogResult/Close() and orphan the install. And the title-bar X bypassed Cancel_Click entirely -- ESC was covered, the window chrome was not -- leaving the install running with the dialog destroyed. Both closed; NormalizeVersion now delegates to TryParseVersionCore rather than keeping a second copy of the parser in sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Repair state
First round where the previous round's fixes all HELD under verification. Six
findings left; two mattered.
1. HIGH -- cancelling a Connected_Current "Repair" put "Perform clean install
(drops existing database)" behind a button still labelled [Repair]. The three
exit handlers did TransitionToState(Initial) and then force-showed
InstallationPanel -- but Initial never resets InstallUpgradeButton.Content or
DatabaseStatusText. So after a cancel (or any transient error, no user action
needed) the dialog read "is up to date... click Repair to reinstall", showed a
[Repair] button, and newly exposed Advanced Options with the clean-install
checkbox. Tick, click, database dropped.
That is the exact invariant the code documents ("InstallationPanel stays
collapsed on purpose: it holds the clean-install checkbox, which must never sit
behind a button labelled Repair"). The exit paths now restore the state the
install was LAUNCHED from, so Connected_Current keeps its panel collapsed and
surfaces the failure in the panel that is actually visible there.
2. The last stranding door, fixed at the root. GetInstalledVersionAsync returned
null for TWO different things: no database, and a database whose
config.installation_history is missing -- which it even LOGS as "old or corrupted
install" before answering "fresh install". Both apps then run the install scripts
over the live schema, skip every migration, and stamp the target version SUCCESS.
Permanently stranded: the precise bug this branch exists to prevent, reachable
without any failed migration at all.
Worse, Round 5's new "nothing to repair" refusals RECOMMENDED that path in as many
words. It now tells the two cases apart by whether the database actually holds
collect objects (verified against SQL2022: a real install has 62), falling back to
the same "1.0.0" sentinel the #538 guard uses -- "unknown, attempt every upgrade",
the safe direction, since every upgrade script is IF NOT EXISTS guarded. An empty
pre-created database still reads as a clean install, so that path is unchanged
(VersionDetectionTests.DatabaseExists_NoHistoryTable_ReturnsNull still holds --
its database has no collect tables).
3. The two most dangerous decisions in the branch were hand-copied into both apps and
pinned by nothing, while the CHANGELOG stated them as a contract. Extracted to
Installer.Core/RepairOutcome and pinned by 13 cases. Getting it wrong is dangerous
in BOTH directions: calling a good repair a failure sends the operator to a
destructive reinstall; calling a broken one a success ships uncompiled procedures
past a CI gate.
4. DetectDatabaseStatusAsync committed _coreServerInfo/_installedVersion to the shared
fields BEFORE its InstallInProgress guards, so a stale detection could clobber the
version a running install was reasoning about. Safe today only by statement ordering
-- one inserted await and it is THE INVARIANT again. Now lands in a local, guards,
then commits.
5. The CLI never validated its OWN version, so an unparseable build version skipped the
downgrade guard and a fresh install would write it straight to installer_version,
bricking a brand-new server. The Dashboard blocks on this; now both do.
6. The "nothing to repair" refusal re-enabled Save in the one state that deliberately
disables it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
…ered two questions
R8a could not break the invariant and confirmed six of seven Round-7 fixes hold.
Both reviewers then found the same two real defects, and both are mine.
1. CRITICAL -- I reopened the clean-install drop bug by fixing something else.
Round 6 rerouted the cancel/fatal/abort handlers away from
TransitionToState(Initial) to RestoreAfterInstall(_preInstallState). Nothing calls
TransitionToState(Initial) any more -- so the mode-flag clearing living in that
case became DEAD CODE, and RestoreAfterInstall never cleared them. It re-enables
the form on top of a still-ticked "Perform clean install (drops existing
database)": cancel a run, retype the server box, click again, and it DROPS a
database that never consented. Verbatim the scenario the now-dead comment was
written to prevent, and the third time this bug class has appeared. Cleared in
RestoreAfterInstall, which is now the single exit path.
2. CRITICAL -- one boolean was answering two different questions, and they disagree
for the unknown sentinel. "May I excuse these file failures?" must answer NO for an
unreadable version (we cannot certify a success we cannot explain). "Is there an
upgrade to run next?" must answer YES for the same input -- the version is unknown,
so every hop may be pending. Reusing FailuresAreExpected for both printed
Repair complete. This server is still at v1.0.0 and no version was recorded.
This server was already at the current version, so there is no upgrade to apply.
two lines apart, for a server with eleven migrations waiting -- and withheld the
"Upgrade Now" handoff that would have applied them, leaving exactly the
half-migrated database this feature exists to escape. And this branch INCREASED the
sentinel's reachability (Round 6's ledger-lost fallback), then broke the messaging
for the state it added. Split into FailuresAreExpected / HasPendingUpgrade /
IsVersionUnknown, pinned by a test that asserts the two questions disagree.
3. "Reinstall Objects" had no nothing-to-reinstall refusal. That state's button is new
(dev collapsed it), the server box stays editable, and the install-time re-read runs
against whatever is in it now -- so retyping to a bare instance made the button
perform a FULL fresh install: new database, jobs, XE sessions, from a button that
promised to reinstall existing objects. The Repair checkbox got that refusal; the
button, whose label makes the same promise, never did.
4. The CLI's InstallGuard switch was non-exhaustive (correct only because a duplicate
check runs earlier -- remove that and it falls through silently). GetInstallBlockReason
was still internal for a test that no longer exists. Two CHANGELOG claims were false:
it named a deleted test file, and claimed both surfaces block a newer database
outright, when the CLI's destructive --reinstall is the deliberate escape hatch.
Known and stated plainly: the ledger-lost probe's only test lives in VersionDetectionTests,
which CI excludes because it hits a real server. The SQL is verified empirically against
SQL2022; the test cannot fail a PR. That limitation is the test project's, not this branch's.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
All four Round-8 fixes hold; R9a could not break the invariant. Ten findings, and
the top three are the same root cause: the dialog restored a state computed for a
DIFFERENT server.
1. CRITICAL -- the restore rendered a stale verdict over a fresh version.
_preInstallState is captured BEFORE the install-time re-read, but the states
render from _installedVersion, which the re-read overwrites. The server box stays
editable, so those two describe different servers whenever it was retyped.
Test Connection on an up-to-date server -> Connected_Current ("Reinstall
Objects"). Retype the box to one four migrations behind. Click. The re-read
correctly finds v2.5.0 and the upgrade runs; a hop fails; the abort fires ->
RestoreAfterInstall -> Connected_Current renders "PerformanceMonitor v2.5.0 is up
to date." over a HALF-UPGRADED database, with Save enabled. The user reads "up to
date" and clicks Save & Connect.
The restore state is now DERIVED from what the re-read actually found
(DeriveConnectedState), which detection now shares. That also keeps
_preInstallState out of InstallComplete/MonitoringCredentials -- reachable through
the repair handoff -- which RestoreAfterInstall would otherwise re-enter with
Advanced Options unfrozen.
2. CRITICAL -- the "nothing to reinstall" refusal guarded "Reinstall Objects" but not
"Upgrade Now", which makes the identical promise. Connected_NeedsUpgrade + retype
to a bare instance + click: _installedVersion is null, so the upgrade block is
skipped and a FULL fresh install lands on a server the user never consented to --
new database, Agent jobs, XE sessions, plus a SUCCESS row -- from a button that
said "Upgrade Now". Both button arms are covered now.
3. HIGH -- that refusal was also the ONE exit path that did not clear
ResetScheduleCheckBox. Tick Repair + "Reset collection schedule" on PROD-A, retype
to a bare server, get refused (Repair cleared, ResetSchedule survives), retype back
to PROD-A, click Install Now -> TRUNCATE TABLE config.collection_schedule. PROD-A's
tuned intervals, gone. Fourth appearance of the sticky-checkbox class; all five
exits now clear all three.
4. Both InstallGuard switches defaulted to "safe to install" -- a new InstallBlock
member would silently become an allow, which is the single failure mode the guard
exists to prevent. Both now refuse instead.
5. The CLI named a FailuresAreExpected result "repairHasPendingUpgrade" -- the exact
one-name-two-questions confusion Round 8 split the API to kill, reintroduced as a
variable name. Renamed. Two comments cited causes that later rounds had already
fixed, and the block-reason doc still said "three cases" when there are four.
CHANGELOG now states plainly that the ledger-lost probe's test cannot fail a PR
(VersionDetectionTests needs a real SQL Server and CI excludes it); the query was
verified by hand against SQL Server 2022.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…entinel out of the version space First fresh-eyes reviewer to come back with no critical: "I could not find a correctness bug in the shipping decision paths... none of these is the 17th stranding bug." Two of its five findings were already fixed by Round 9. Three real ones remain. 1. The single most consequential branch in the installer -- "database exists, no history table": fresh install, or run every migration? -- had NO test that could fail a PR. Its only cover was VersionDetectionTests, which CI excludes because it needs a real SQL Server, AND which hand-copied the SQL rather than calling production. That copy had already drifted (production does USE PerformanceMonitor; the replica did not), so the exact failure mode throwOnError exists for -- an OFFLINE/RESTORING database throwing rather than reading as "absent" -- was never exercised. The DECISION is now a pure function, InstalledVersionClassifier, taking the facts (database exists / history table exists / collect table count / latest SUCCESS row) and returning the version to act on. Pinned by 7 cases in Installer.Tests, which CI actually runs. The SQL stays where it must, but it now only gathers facts -- and the DB test calls the same classifier, so the decision can no longer drift out from under the tests. 2. The unknown sentinel was "1.0.0" -- a real shipped tag. And the "your recorded version is unreadable" message tells operators to correct the SUCCESS row by hand, where "1.0.0" is precisely what someone would type to force a full re-run. They would then be told their version could not be read, and --repair would exit 4 on a server with eleven pending hops. Moved to "0.0.0": no release is 0.0.0, it still sorts below every real version so every hop is still offered, and it is never persisted (repair writes no history row), so this is purely internal. 3. The Dashboard's newer-than-this-build block is a DEAD END: it blocks clean install too (Connected_StatusUnknown collapses the panel the checkbox lives in), and its message only said "Update this Dashboard first" -- useless if the database was written by an unreleased build. It now carries the whole escape route, including the CLI's --reinstall. The CHANGELOG claimed the message already pointed there, and stated the --repair exit-code contract as a binary when the code deliberately has three cases. Both corrected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mer again
Both reviewers found the same critical, and the state-machine lens gave the verdict
I asked it for: NOT sound, needs a structural fix.
"Every critical from rounds 6-9 is one bug: the server box stays editable while
_currentState, _installedVersion, _installBlockedReason, _coreServerInfo,
_preInstallState and five checkboxes all describe a server that may no longer be the
one in the box. That is not bad luck; it is what happens when an invariant is enforced
at N call sites instead of one."
That is exactly right, and it is why this kept reopening. So this round fixes the
class, not the instances.
1. CRITICAL -- the repair handoff arms a live "Upgrade Now" button while the state is
InstallComplete/MonitoringCredentials. The "nothing to reinstall" guard listed only
Connected_NeedsUpgrade and Connected_Current, so from the handoff: retype the server
box to a bare instance, click the big accent button in front of you, and you get a
FULL FRESH INSTALL -- database, tables, procs, SQL Agent jobs, XE sessions, community
dependencies -- plus a SUCCESS row, on a production server that never consented.
Root cause: one field, _preInstallState, was answering two different questions --
"what did the button PROMISE?" and "where should a failure RESTORE to?" -- and was
consumed for the promise 50 lines BEFORE being re-derived for the restore. Split into
_launchState (the promise, fixed at click time, via PromisesExistingInstall which now
covers all four states that can arm a non-"Install Now" button) and _preInstallState
(the restore target, from the facts).
2. HIGH -- _preInstallState is also consumed by the cancel/fatal paths INSIDE that
window, where this server's version has not been read yet and the fields still
describe the last one. Cancel during the re-read and the dialog restored
"PerformanceMonitor v3.1.0 is up to date." under the NEW server's name, Save enabled.
It is now null until the facts are in, and RestoreAfterInstall refuses to render a
verdict it never obtained.
3. THE STRUCTURAL FIX -- the verdict is now STAMPED with the server it was read from,
and TransitionToState refuses to render any Connected_* verdict whose stamp no longer
matches the box, falling back to "the server name changed; click Test Connection".
One check, one place. A stale verdict is now unrenderable no matter which consumer
forgets to re-check -- which is the failure mode that produced three separate
criticals across nine rounds.
Detection also takes an epoch, so two interleaved runs cannot publish the first
server's verdict under the second's name; and the install-time re-read now refreshes
_coreServerInfo too, so the summary report cannot name this server with the previous
one's SQL version and edition.
4. GetAppVersion's version-less fallback was "0.0.0" -- which Round 9b had just made the
unknown sentinel. A build with no version would have written a parseable "0.0.0"
SUCCESS row that every later read takes as "unknown, replay every migration". It now
returns "Unknown", which does not parse, so InstallGuard blocks it -- matching the CLI.
5. The sentinel is no longer rendered as a fact ("PerformanceMonitor v0.0.0 is
installed") on either surface, and SkipInstall_Click -- the one exit that bypasses
TransitionToState -- now clears the destructive checkboxes like every other exit.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
R11b could not break the decision core or the ledger invariant -- it attacked both hard and said so. But it found the Round-10 structural guard was self-satisfying, and it was right. 1. CRITICAL -- the guard could never fail. RecordVerdictFor read ServerNameTextBox.Text AFTER the awaits, so it stamped the verdict with whatever was in the box NOW, not the server we had actually read. VerdictMatchesServerBox then compared the box to itself. Retype the box during a detect -- which is seconds, and unbounded for Entra interactive, with no TextChanged handler and nothing disabling the box -- and the dialog rendered SERVER-A's verdict under SERVER-B's name: "PerformanceMonitor v3.1.0 is up to date." with Save enabled, over a bare instance. The exact lie the guard's own comment says it stops. The stamp is now captured WITH the connection string built from it, before any await. And a TextChanged handler makes a retype supersede an in-flight detection (it publishes nothing) and demotes a now-stale on-screen verdict through the guard. 2. HIGH -- the unsupported-SQL-version branch wrote the panels by hand and never called TransitionToState, so the guard had no hook there at all: even a correct stamp would not have protected it, and it left _currentState pointing at the previous server's state. Routed through BlockInstall. 3. HIGH -- the install-time re-read fetched _coreServerInfo precisely BECAUSE "after a retarget it describes a different box", and then never checked IsSupportedVersion. The 2016+ gate was detect-time only, so retargeting to an older server and clicking through ran 2016+ syntax onto SQL 2014 -- a junk database and a FAILED history row on a server the gate exists to refuse. Re-reading the fact and not using it was the whole hole. Now gated. 4. MEDIUM -- the verdict was stamped with WHO, not WHEN: a clean install DROPS the database, so cancelling mid-run restored "PerformanceMonitor v3.0.0 is installed" over a server whose database had just been dropped, Save enabled. The verdict is now forgotten before a clean install runs. 5. Three files had picked up a UTF-8 BOM (dev has none) -- an editor artifact in files the release tooling reads. Stripped. The CLI also promised "the pending upgrade still needs to run afterwards" for every --repair, including servers already current; now gated on there actually being one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Superseded() tests InstallInProgress, but that flag is only read when the continuation RESUMES. Nothing bumped the epoch when an install STARTED, so a detection that spanned the whole run woke up afterwards, found the install finished, and published its pre-install verdict over the result: "Upgrade Now" returning on a server that had just been upgraded (wiping the SQL-auth monitoring-credentials panel with it), or coming back live while _installBlockedReason was still set, so every click was a "Blocked" dialog. The epoch is the only half of that test that cannot un-fire. Bump it on install start. _installBlockedReason was cleared in the detection's prologue, before the await, while the publish it belongs to is guarded. That makes the clear unconditional and the publish conditional, which is what let an install that blocked itself mid-flight get retroactively un-blocked by a detection that then declined to publish anything. Committed with the verdict now, not ahead of it. The retarget guard only covered the three Connected_* verdict states, but PromisesExistingInstall already knew that the repair handoff arms a live "Upgrade Now" in InstallComplete/MonitoringCredentials too -- the two predicates disagreed about the same question. Retyping the server box there left SQL-A's version claim and a live upgrade button sitting under SQL-B's name: verbatim the lie the guard says it stops. Withdraw the handoff on a retarget. _serverVersion was written nowhere but Test Connection, so retyping the box and clicking Check for Updates rendered the new server's name beside the old server's version. Committed alongside _coreServerInfo, which was already being re-read two lines away. GetAppVersion's "Unknown" fallback sat one branch too late: an assembly with no AssemblyVersion reports 0.0.0.0, so the branch above it returned "0.0.0" -- which IS UnknownVersionSentinel, the exact collision the comment claims to avoid. A version-less build must land on something unparseable so InstallGuard blocks it.
…dialog-upgrade-abort # Conflicts: # CHANGELOG.md
Round 12's handoff withdrawal was one-way. It hid the version claim and the
"Upgrade Now" button when the box was retargeted, but nothing restored them when
the name came back -- so typing one character into the server box and deleting it
destroyed the only button that applies the pending upgrade, while the install log
still said to click it. Repair writes no history row, so the upgrade was still
offered on the next dialog open; nothing on screen said so. The handoff is now
re-rendered rather than painted once: idempotent, reversible, and it explains
itself while withdrawn instead of silently vanishing.
Save_Click carried the same un-firing guard this branch just fixed elsewhere:
InstallInProgress alone, read after an await, is false again once the install
FINISHES. An install started and completed during Save's connection test fell
straight through to DialogResult/Close() -- destroying the dialog as the install
landed, so a SQL-auth user never saw the monitoring-credentials panel and the
INSTALLER's account (routinely sa) was persisted as the ongoing monitoring login.
Guarded with the epoch, which cannot un-fire. The same fix covers a retype during
the test, which would otherwise persist the new name on the strength of a
connection proven against the old one.
skipConnectionTest keyed on _currentState alone, so retyping the box after an
install skipped the connection test entirely and saved an un-contacted server.
The stamp is what makes "we just installed, so this connection is proven" true,
so it now tests the stamp.
BlockInstall is an exit from a RUN, but it left the clean-install / repair /
reset-schedule ticks set -- unreachable today only because the state it lands in
happens to collapse the panel holding them. That is a rendering accident standing
in for a consent check, and it is the same shape as the drop-the-database bug
that has reopened more than any other here. It clears them itself now.
The connection header was built from the LIVE box plus the PREVIOUS server's
version, and rendered in the one connected state the stamp guard does not cover
-- the state a demotion LANDS in. Mid-keystroke it read "Connected to S
(Microsoft SQL Server 2019 ...)": a connection never made, to a half-typed name.
The CLI had the 0.0.0.0 hole the Dashboard just closed: GetName().Version is
never null, so its "?? Unknown" could not fire, and a version-less build would
write 0.0.0.0 into the ledger -- which reads back as the unknown sentinel and
replays every migration forever. Both surfaces write to the same ledger, so the
guard had to exist on both. The comment claiming CLI parity was false; now it
isn't. Also stopped printing the sentinel as a fact ("v0.0.0") at the three sites
that still did -- the CLI contradicted itself inside a single run.
Found by two adversarial reviewers on the Round-12 diff. Neither could break the
ledger invariant, reach a destructive action without consent, or find a stuck UI.
_handoffStatusText was written only under repairCompleted and never cleared. So: repair a server, click the "Upgrade Now" the handoff gives you, let the upgrade SUCCEED -- and the field still held "PerformanceMonitor is still at v3.0.0, the pending upgrade has not been applied". Any later keystroke in the server box re-rendered exactly that, with a live Upgrade Now button, on a server that had just been upgraded to 3.1.0. Introduced by the very commit that made the handoff re-renderable, and it is the same bug class this whole branch exists to kill: a fact outliving the run that produced it. Cleared at run start; the completion block re-sets it only when THIS run was a repair that left an upgrade pending. Found by self-review, not by a reviewer.
…m at the choke point The clean-install / repair / reset-schedule ticks authorize DROP DATABASE, skipping every migration, and TRUNCATE config.collection_schedule -- against the server that was on screen when they were ticked. Keeping that true has been patched into RestoreAfterInstall, then the detection prologue, then InstallComplete, then BlockInstall. Round 13's reviewer found the fourth one already had a sibling that bypassed it: the stale-name demotion rewrites the state IN PLACE and never calls BlockInstall, so a ticked clean install still survived a retarget there -- unreachable today only because the state it lands in happens to collapse the panel holding the checkboxes. That is a rendering accident standing in for a consent check, on the code path that drops a database, and it is the bug that has reopened more times than any other in this file. Cleared in TransitionToState instead -- the one choke point every state change passes through -- for every state except Installing, which is the state that is about to READ them. Verified the checkbox handlers only enforce mutual exclusion and never transition, so ticking cannot untick itself, and that the ticks are read after Installing is entered. Also from that round: An ordinary install/upgrade/reinstall left "Installation completed successfully!" and its log on screen after the box was retyped. Only the repair handoff was withdrawn -- because that was the path the previous round happened to be looking at, while the common case had no withdrawal at all. A user could retarget after an install and click Save believing PerformanceMonitor had been installed on the server now in the box. Withdrawn and restored together now, with the reason stated once. A clean install forgets the verdict on the way in (it is about to drop the database, so a cancel must not restore a verdict describing it) and never re-stamped it on success -- so a completed clean install read as un-addressed: Save re-ran the connection test, which on Entra means a second interactive MFA prompt, and the first keystroke would have withdrawn a success message that was true. Re-stamped with the server the run actually wrote to. The stale-name demotion is now reversible. The verdict was never wrong, only mis-addressed, so typing the name back restores it -- previously one keystroke and a backspace left "Upgrade Now" gone for good, under a notice that the server name had changed, which by then was no longer true. Tracked separately from a REAL block (unsupported version, unreadable version, newer-than-binary), which still cannot be typed away. CLI: a repair with a pending upgrade exits 0 by contract, but it was printing "Installation completed successfully! / All collector stored procedures" over N procedures that demonstrably failed to compile -- contradicted by its own next paragraph. The exit code is the contract for scripts; the banner is for the human. CHANGELOG said the unknown sentinel was 1.0.0. It is 0.0.0 -- and the whole reason it was moved off 1.0.0 is that 1.0.0 is a real shipped version.
Connected_StatusUnknown is where a stale-verdict demotion LANDS, so the guard in
TransitionToState -- which only ever demotes INTO it -- can never cover what is
already sitting there. It is also where a REAL block sits, and a real block is a
fact about one specific server: detect an unsupported SQL 2014 instance, retype the
box to a 2022 one, and the panel still read "SQL Server 2014 is not supported" --
about a server the dialog had never looked at. Verbatim the lie the guard exists to
stop, in the one state the guard structurally cannot reach.
Two things were missing. The verdict was stamped only on the success path, so a
block was an unattributed fact with nothing to compare the box against; it is now
stamped as soon as the connection facts land, which is the point from which every
statement is ABOUT that server. And TextChanged now handles the landing pad: a real
block loses the previous server's specifics on a retarget, while a demotion keeps
its message (which already says exactly this) and its undo.
SkipInstall_Click writes _currentState directly rather than transitioning, and
bumped no epoch -- so a detection still in flight landed afterwards, published its
verdict, and put the install panel and install button straight back on screen,
silently undoing the skip the user had just chosen.
Save was re-enterable, and the second one CRASHES. RunConnectionTestAsync disables
Save for its duration, but Test Connection stays live and its finally re-enables the
form wholesale -- Save included. Click Save, click Test Connection, click Save again:
both complete, the first closes the window, the second sets DialogResult on a CLOSED
window. That is an InvalidOperationException on an async void handler: unhandled,
and it takes the app down. Pre-existing; the epoch guard happened to close every
variant where a detect or install intervenes, but not this one.
And the comment I wrote last round claiming the abandoned save "is never silent"
was false. From Initial -- a fresh dialog, the most common way to reach Save -- a
retype renders nothing, so the guard swallowed the click: fix a typo during the
10-second connection test and Save just... does nothing, while the user believes the
server was added. It says so now.
The stale-server notice was three hand-copied strings that had already drifted
("re-check" vs "check"). One const.
…kes it AFFIRM
CRITICAL, and it was Round 15's own fix. Stamping the verdict as soon as the
connection facts landed -- so that a BLOCK would be an attributed fact rather than
an anonymous one -- put the stamp BEFORE the guard that can still abandon the
publish. The comment waved it through: "re-stamped on the success path with the
same value; harmless." It is only re-stamped if that path is REACHED.
Retype the server box during a detection and the version read is superseded, so it
returns having already moved _verdictServerName and _serverVersion to the NEW server
while _installedVersion and _currentState still describe the OLD one.
VerdictMatchesServerBox() then answers TRUE. The guard did not miss the lie; it
CONFIRMED it -- "PerformanceMonitor v3.1.0 is up to date", Save enabled, over a
server with no PerformanceMonitor database at all. Fifteen rounds of hardening that
guard, and the way in was to make it say yes.
Stale-but-consistent facts the guard catches every time. Half-replaced facts are the
one thing it structurally cannot catch, because the stamp IS the thing every consumer
tests before trusting the other two.
So the three now move as one, through PublishProbedServer, committed only where a
verdict about that server is actually published -- never in the prologue, never
before a guard that can still abandon it. That is the rule this method already stated
at the top of the probe ("Land in a LOCAL, guard, and only then commit to the shared
field") and that Round 15 broke. TestConnection_Click's lone _serverVersion write --
the last place that set one of the three on its own, post-await, with no stamp beside
it -- is gone with it; the detection it calls publishes all three itself.
Also from that round:
The outer catch in DetectDatabaseStatusAsync was the one continuation with no
Superseded() check, so a stale detection whose connection test threw would paint its
error over a running install's screen.
RunConnectionTestAsync blanked StatusText in its finally -- but after a retarget
withdraws the post-run panels, that notice is the ONLY thing left on screen. A failed
Test Connection then left a dialog with no panels and no explanation. It borrows the
label now, and gives it back.
RestoreAfterInstall's comment claimed it was "the only exit path from a run", which
was wrong twice over and pointed the next reader at the wrong mechanism on the code
path that runs DROP DATABASE. The choke point in TransitionToState is the guarantee;
these three lines are belt and braces, and now say so.
…dialog Round 17 taught StatusText to be borrowed and given back, so a retarget's "this may describe a different server" notice would survive a failed connection test. It read the label BEFORE the awaits and wrote it back AFTER them, with no epoch guard -- which is Shape 1, the exact defect this branch has spent nine rounds killing, freshly introduced by the fix for the round before. Retype away, click Test Connection, retype BACK during the connect: the screen correctly restores itself, then the failed test's finally pastes the stale notice over the top of it. The label is only ours to give back if nothing bumped the epoch while we were away. _installedVersion was the FOURTH fact. PublishProbedServer committed three atomically and left it behind at both block sites -- where we either never read the version (unsupported server) or could not (the read threw) -- so a block described the PREVIOUS server's database. Sealed today only because a block lands in a state that happens to collapse both buttons: a rendering accident standing in for an invariant, which is precisely what this file keeps getting caught by. It moves with the other three now, and a block publishes null, because null is the truth there. Close the dialog during a save's connection test -- ESC, Cancel, the X -- and the close lands first, then the test completes and SaveAsync walks on into DialogResult on a window that is gone, which throws. The re-entrancy flag did not cover it: that is one Save, not two. And ESC is far easier to hit than the three precise clicks the flag does cover. Correction: the comment I wrote last round said that throw "takes the app down". It does not -- App.OnDispatcherUnhandledException handles it (App.xaml.cs:173). It is an alarming error box and a silently unsaved server. Overstated, now accurate. SkipInstall_Click was the only epoch-bumper that didn't clear StatusText, so "Checking database status..." sat there for the rest of the dialog's life announcing a check nobody was running. RunConnectionTestAsync's ServerVersion return is gone -- Round 17 made both callers discard it. It used to be assigned straight to _serverVersion, which is one of the four facts that may only be committed as a set. The @@Version round-trip stays as a liveness check; its result is discarded explicitly. And one in the CI-pinned core, which is the only part of this that a test can hold: InstalledVersionClassifier treated a BLANK installer_version as an answer. The column is NOT NULL, so a row hand-edited to '' -- and the "unreadable version" message we show an operator invites exactly that edit -- came back as "" rather than null, and "" is not a version: FilterUpgrades resolves it to ZERO hops, which reads as "already current" and strands every migration. Blank is the absence of an answer. Two tests.
… was the signal CRITICAL, and I introduced it. _dialogClosed guarded DialogResult -- 47 lines AFTER the assignments it needed to guard. In edit mode ServerConnection is not a copy. It is the same object ServerManager holds in its list: MainWindow hands us item.Server directly. So the assignments in SaveAsync mutate the live server IN PLACE. Edit PROD01, retype the box to TEST99, click Save, and press ESC during the 10-second connection test: the dialog closes, ShowDialog returns false, the caller never calls UpdateServer, and the user is entirely satisfied nothing was saved. Then the test lands, SaveAsync walks past InstallInProgress and the epoch, renames the user's monitored server, and hits _dialogClosed -- and returns. Silently. It reaches disk on its own, the next time UpdateLastConnected fires for ANY server. PROD01 becomes TEST99, keeps PROD01's id and credential, and stops being monitored. The part I got wrong is worth naming. Before Round 18, assigning DialogResult to a closed window THREW, and the app surfaced it. The corruption was already there -- what my fix removed was the only evidence of it. A silent bug is not a fixed bug, and "stopped the exception" is not the same as "stopped the damage". The guard belongs where every other guard in this branch already sits: before the facts are committed, not after. One check covers it -- RunConnectionTestAsync is the only await in SaveAsync -- and it also kills the orphaned "Do you still want to save this connection?" box that was popping up with no dialog left behind it. Two smaller ones from the same round: The abandoned-save message blamed "the server details changed" for all four epoch bumpers. Check for Updates and "Skip, just add server" bump it too, so the message accused the user of a change they never made. Worded for what actually happened. TestConnection_Click's failure box read the server box AFTER the await -- the last read-the-box-after-the-await in the file. Retype during the connect and it said "Could not connect to A" about a server that is fine, for a failure that belonged to B.
MAJOR, and mine, from Round 11. RestoreAfterInstall's "we have no verdict to restore" branch returns BEFORE the line that re-shows InstallationPanel, and it gets there through BlockInstall, which lands in a state that COLLAPSES that panel. So the install log -- the only place the actual SQL error text lives -- disappears. And _preInstallState == null overwhelmingly means CLEAN INSTALL, because that is where I null it on purpose. CleanInstallAsync runs before the file loop, so any cancel or failure after that point arrives here with the database ALREADY DROPPED. The user gets one exception message, no log, and no database. The branch eleven lines below has said "show the log either way -- it is the only place the actual SQL error text lives" since it was written. This one just didn't. The reason it gave was false there too: "this server's installed version was not read." It WAS read -- we discarded it on purpose, because we were about to drop the database out from under it. Those are two different failures and they need two different sentences. Also from that round: Round 19 swept the orphaned modals out of SaveAsync and left the identical shape in its siblings. Close the dialog during a Test Connection and its failure box still pops with no dialog behind it, telling the user to "Click Test to try again" on a window that no longer exists. _dialogClosed now lives inside Superseded() as well, because "the dialog is gone" is just another reason a detection's answer is no longer wanted -- and Superseded() is the one question every publish point already asks, so one line covers all of them instead of a guard per caller that the next caller forgets. Two connection tests could overlap: Test Connection stayed live during a Save's test. The second one then borrowed the FIRST one's "Testing connection..." label and faithfully restored it at the end, leaving that message on screen forever with nothing running behind it. (It is also what re-armed Save mid-save and gave the double DialogResult its way in.) Both entry points are disabled for the duration now. And the install path was still writing three of the four coupled facts separately across an await, never writing the fourth. Not exploitable -- the box is frozen and every other handler bails on InstallInProgress -- but "not exploitable because something else happens to be disabled" is enablement standing in for an invariant, and that substitution is precisely what this file keeps getting caught by. It publishes them as a set now, so PublishProbedServer really is the sole writer it claims to be.
Two user-visible bugs found in the last review rounds that the entry set did not cover: a save abandoned with ESC still renamed the live server object in place (the dialog edits the same instance ServerManager holds, not a copy) and it reached disk on its own; and a cancelled or failed CLEAN install hid the install log -- on the one path where DROP DATABASE has already run and the log is the only place the SQL error text lives.
…leaned on it CRITICAL. Round 19 put the _dialogClosed guard after the await and reasoned that nothing could re-enter before the commit. MessageBox.Show pumps a nested Win32 message loop, and the "Do you still want to save this connection?" box sits between that guard and the assignments -- which, in edit mode, rename the user's live monitored server IN PLACE, because ServerConnection is the object ServerManager holds, not a copy. Its owner is whatever GetActiveWindow() returns, and that is NULL when the calling thread does not own the foreground window -- so the box does not necessarily disable the dialog behind it. This file passed an owner to exactly ZERO of its eleven MessageBox calls while four other windows in this app pass one. All eleven now do. But the owner is the smaller half. The real error was mine: a previous reviewer told me the modal disables the dialog, and I wrote that into the comment and let the guard sit where it was. That is "unreachable because something else happens to be disabled" -- the exact substitution I have condemned in this file five times. Do not reason about re-entrancy windows. Guard where the damage is: immediately before the commit, with nothing between. MAJOR -- and it can leave an install that cannot be cancelled by anything, including closing the dialog. The "Nothing to Reinstall" refusal called TransitionToState BEFORE its MessageBox: the transition re-enables the form, puts a live "Install Now" back on screen and clears Installing, so while that box pumped, the InstallInProgress backstop was already false. Click Install Now and a SECOND run starts with its own CancellationTokenSource -- then the first run unwinds into a finally that disposed and nulled _installCts unconditionally, i.e. the second run's token. CancelInstallOnClose is the thing that stops an install when the dialog closes, and it would be cancelling null. Modal first, while the form is still frozen; and the finally now clears the field only if it still owns the token it created. Also: only one server probe at a time (a detection left Test/Save armed, so a connection test could start on top of one, borrow ITS "Checking database status..." label, and restore it forever over a settled screen). A fresh detection now clears the previous run's install log, which was reappearing under a different server's verdict -- PROD01's log and "Installation completed successfully!" sitting under "No PerformanceMonitor database found". The not-connected branch no longer returns silently, which made "Click Test Connection" a dead end that did literally nothing. The summary report names installTimeServerName, the last read-the-box-after-the-await in the file. And a real block's reason survives a retype-and-retype-back, instead of being permanently replaced by "the server name changed" -- which also stops the FIRST keystroke from wiping the one sentence that tells a user their database was dropped.
A block is a fact about a specific server, and the guard attributes it by the stamp. That branch deliberately refuses to move the stamp -- correctly, because it established no facts -- so BlockInstall there would have left the block attributed to the PREVIOUS server, and the next keystroke would have restated it as 'the server name changed', which is not what happened. And the only way to attribute it properly would be to stamp without the facts to go with it, which is precisely the half-replaced state that made the guard AFFIRM a lie. Fixing the dead-end must not cost the invariant. It is a plain report of what failed; the install stays blocked by whatever already blocked it.
…ed one MAJOR, and it is in Installer.Core, which is why eleven rounds of staring at the dialog never found it. CleanInstallAsync drops the three Agent jobs, both XE sessions, and then the DATABASE -- SET SINGLE_USER WITH ROLLBACK IMMEDIATE, DROP DATABASE -- all before a single install file runs. That is the most destructive and least recoverable window in the installer, and it was the one window with no cancellation check in front of it: the first ThrowIfCancellationRequested sits AFTER the clean install. A cancelled SqlCommand faults with SqlException, not OperationCanceledException, so the clean-install catch swallowed the cancel as an ordinary failure and RETURNED NORMALLY. The Dashboard's cancel path never ran. Instead the user was told "Installation completed with 1 error(s)" over a database that may already be gone; the dialog re-stamped the verdict it had deliberately discarded for safety; and because Save skips the connection test once that verdict is stamped, the server could then be saved without ever being reconnected to. Guarded, and surfaced as a cancel. Pinned with tests, because unlike everything in the dialog this lives where a test can hold it -- and they FAIL against the old code, which I checked rather than assumed. The re-stamp was never gated on success at all. Its comment asserted "but the run SUCCEEDED" while the code checked nothing, so a clean install that FAILED restored the verdict it had thrown away. It is now gated -- and it is a PUBLISH, not a lone stamp: moving the stamp alone put it back while _installedVersion still held the PRE-DROP version, so the guard would have gone true over a half-replaced fact set, which is the one shape it structurally cannot catch. A failed clean install now goes where a cancelled one goes: status unknown, log on screen, install blocked. And two regressions I introduced in Round 21: The probe's finally re-armed Save unconditionally -- clobbering Connected_NoDatabase, which turns Save OFF on purpose so the user must click Install Now or take the explicit "Skip, just add server" link. That handed the consent away on the most ordinary path there is: point at a bare instance, click Check for Updates, read "No PerformanceMonitor database found" -- and Save is live, and it is IsDefault, so ENTER commits a server the Dashboard can read nothing from. Save is owned by the state; the finally now only restores what it borrowed. And the install log was being cleared in the detection PROLOGUE, before it knew whether it would publish anything. The not-connected branch publishes nothing by design, so it left InstallComplete on screen with an empty log, a blank status line and a 0% bar -- destroying, on a failed install, the only on-screen copy of the SQL error text. It is discarded WITH the verdict that replaces it now. Same rule as _installBlockedReason, same reason. Also: _reportPath had no per-run reset, so a failed report write left "View Report" pointing at the previous server's file.
…abase regression The first is the most serious defect this review found, and it was not in the file twenty-two rounds were spent on: cancelling a clean install returned NORMALLY -- 'Installation completed with 1 error(s)' over a database DROP DATABASE had already taken -- because a cancelled SqlCommand faults with SqlException, not OperationCanceledException, and the clean-install handler swallowed it. The second is a button-gating regression introduced while fixing an overlapping-probe bug: Save is disabled on Connected_NoDatabase on PURPOSE (Install Now, or the explicit 'Skip, just add server' link), and re-arming it unconditionally handed that consent away on the most ordinary path in the product.
…stall hole Security pass on the SQL and credential paths came back close to clean -- SQL is parameterized, no password reaches the log/report/history/console, TLS defaults to Mandatory with TrustServerCertificate off. One real finding. GenerateSummaryReport built its filename from the user-typed server name and stripped only the backslash, so a name like "x/../../Users/Public/foo" turned Path.Combine into a write OUTSIDE the report directory -- forward slash is a separator, ".." is parent, ":" opens an NTFS alternate data stream. Called by BOTH front ends. Mostly self- inflicted for an interactive operator, but it matters in the CLI's unattended mode where the server list may come from a lower-trust inventory while running under a privileged account. The CLI's own error-log writer already sanitizes the same input with GetInvalidFileNameChars; this shared report writer was the one that was missed. Full invalid-char stripping now, plus a containment assert so a future filename change cannot reintroduce an arbitrary write silently. Pinned with tests that fail on the old code (the forward-slash and ADS cases), verified not assumed. And a parity gap in the exclusivity guard THIS branch introduced: --repair is mutually exclusive with --reinstall, but not with --uninstall -- which is dispatched before any repair logic and, in automated mode, drops the database with no prompt. So "--repair --uninstall" let the destructive mode win silently, on a command line that also asked to destroy nothing. Both destructive companions are rejected now, not just one. Left as a flagged sharp edge, not fixed: a CLI password beginning with "--" is dropped by the positional filter and the run then fails with a misleading "Password is required". Pre-existing, fails safe (no wrong connection, no leak), documented workaround (PM_SQL_PASSWORD). Touching positional-arg parsing to fix it risks the exact class of accident this branch exists to prevent, so it is Erik's call.
…an Round 22 R23a's InstallationService pass found the sibling of the clean-install cancel bug. The file loop's broad catch(Exception) counted a cancelled SqlCommand (which faults as SqlException, not OperationCanceledException) as a file FAILURE and returned Success=false. The loop-top ThrowIfCancellationRequested only re-surfaces a cancel if control reaches the top of the loop again -- which it does NOT for a critical file (break) or the last file (loop ends), so those two cancels fell through to "completed with N error(s)" and the caller's FAILURE path instead of its CANCELLATION path. Same class as the clean-install bug Round 22 fixed; same dedicated catch-when-cancelled pattern applied here, ahead of the general catch, so a cancel is never recorded as a file failure. (I first wrote this as a `when (!IsCancellationRequested)` filter plus a post-loop throw -- which is broken: the filter makes the SqlException skip the catch AND everything after it, so the post-loop throw is never reached and a raw SqlException escapes. The dedicated converting-catch is the correct shape and matches the clean-install branch.) Coverage boundary stated honestly in the test file: the DB-free tests pin the PRE-file windows (a pre-cancelled token trips the guards before any command runs); a cancel that lands INSIDE a running SqlCommand needs a live server and is a click-through item, not a fake unit test. Backed OUT the SET SINGLE_USER / DROP / MULTI_USER / THROW guard I had added for the "failed DROP strands the DB in SINGLE_USER" edge. docs-first-verifier against MS Learn showed it does not deliver: the timeout it targets is a CLIENT CommandTimeout, which fires an attention, and TRY/CATCH is documented not to trap attentions -- and the whole pattern is invalid on Azure SQL MI (SET SINGLE_USER is non-modifiable there, so the batch dies before the TRY). It is a real MINOR edge that needs a C#-side fix across three files; parked as a follow-up rather than shipped as theatre.
…word CLI hint Two parked items from the #1498 review, both installer-safety. 3a -- the clean-install teardown. SET SINGLE_USER WITH ROLLBACK IMMEDIATE is non-modifiable on Azure SQL Managed Instance (EngineEdition 8), so the teardown batch died on that line there before the DROP; gated to box SQL Server now, MI drops directly. And SINGLE_USER + DROP are two autocommitted statements -- a DROP that fails (single-user-slot race) or hits the client timeout/cancel AFTER SINGLE_USER committed left the database stranded in SINGLE_USER, semi-bricked. A server-side TRY/CATCH cannot see a client-attention timeout, so the un-brick is C#-side: best-effort SET MULTI_USER on a fresh connection before re-raising the original error. Applied to InstallationService.CleanInstallAsync and ServerManager's drop path (which now warns if the un-brick itself fails); TestDatabaseHelper gets the MI gate for pattern-correctness. Every SQL construct (EngineEdition 8 = MI, SINGLE_USER/MULTI_USER non-modifiable on MI, DROP supported on MI, DB_ID null-check, bare-comparison of the sql_variant EngineEdition) was verified against MS Learn for both box and MI. Plain SET MULTI_USER, not WITH ROLLBACK IMMEDIATE, deliberately -- it does not force-kill a session that raced into the slot. 3b -- a SQL-auth password beginning with "--" (e.g. "--h7x") is indistinguishable from a flag, so the positional filter dropped it and the run reported "Password is required" for a password that WAS given. The error now adds a precise note -- only when a "--"-token that is not a recognized flag actually appeared -- pointing at PM_SQL_PASSWORD. Parsing is unchanged on purpose: accepting "--"-values positionally is exactly what would let a mistyped flag become a password.
The testability follow-up the multi-round review kept pointing at. The AddServerDialog flow's DialogState enum and its pure predicates lived inside the WPF dialog, which is not CI-wired, so nothing tested them -- and the recurring bug shape was "a predicate over the state that forgot a state" (the destructive-checkbox rule missing an exit; IsServerVerdictState and PromisesExistingInstall disagreeing about the same server). The enum is now Installer.Core.ServerSetupState and the predicates are Installer.Core.ServerSetupPolicy -- IsServerVerdictState, PromisesExistingInstall, StateConsumesModeSelections (the destructive-tick rule), IsInstalling, and DeriveConnectedState (which already called RepairOutcome, also in Installer.Core, so no new dependency). Pinned by a full state x predicate matrix in Installer.Tests: one assertion per predicate per state, plus a guard that the enum still has exactly the eight states the tables cover, plus the two tricky relationships that were themselves bugs when "fixed" to be consistent (the repair-handoff states promise an existing install but are NOT verdict states; Installing is neither). A switch that forgets a state now fails a test instead of only misbehaving in the untested UI. Behavior-preserving: the dialog aliases the enum as DialogState (via `using`), so its 40+ references are unchanged, and calls the extracted predicates at the ~7 sites. Nothing about the logic changed -- all 732 Dashboard tests pass unchanged, and the 33 new matrix tests are additive (Installer.Tests 157 -> 190). CA1707 (underscores in the now-public Connected_* members) is suppressed with justification: the grouping is a deliberate, readable convention carried verbatim from the dialog.
…dialog-upgrade-abort # Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1497.
The bug
AddServerDialoglogged "Upgrade failures detected. Continuing with full installation to ensure consistency..." when anupgrades/*script failed, ran the full install over the partially-upgraded database anyway, and then wrote aninstallation_historyrow using_installResult.Success— which isFilesFailed == 0for the install files only and excludes upgrade failures entirely.That row is permanent damage:
LogInstallationHistoryAsyncwritesinstallation_status(InstallationService.cs:1236).GetInstalledVersionAsyncreadsSELECT TOP 1 installer_version ... WHERE installation_status = 'SUCCESS' ORDER BY installation_date DESC(:964-968).FilterUpgradesonly offers hops.Where(x => x.ToVersion > current)(ScriptProvider.cs).So a failed migration got stamped SUCCESS at the target version, the server then reported as current with the
ALTERs never applied, and the hop was never offered again. Silent, permanent schema drift; the only recovery was a clean install.ExecuteAllUpgradesAsyncalready documents the contract in its own comment:The CLI installer honored it. Only the Dashboard's single-server path did not.
The fix
Dashboard/AddServerDialog.xaml.cs— aborts on upgrade failure without running the install or writing a history row, and folds the upgrade counts into the history write, matching the CLI. Writing noSUCCESSrow is precisely what makes the failed hop get re-offered on the next attempt, and upgrade scripts are written to be idempotent ("each column gets its own guarded ALTER so a re-run after a partial failure resumes cleanly"), so retrying after fixing the error resumes cleanly. The UI restores to the state the run was launched from — notInitial, which would leave "Reinstall Objects" on screen with the clean-install checkbox newly reachable behind it.Installer.Core/ScriptProvider.cs—FilterUpgradesnow throwsArgumentExceptionon a version with no parseable numeric core, instead of returning an empty list. An empty list means "no upgrades needed", which must never be the answer to "I could not read the version you gave me". That silent path is what allows a status string like"Unreachable"to skip every migration while reporting zero failures.null/blank still means a fresh install and still returns empty.Installer.Core/InstallationService.cs—ExecuteAllUpgradesAsynccatches that and returns(0, 1, 0)so callers abort through the existingtotalFailureCount > 0contract rather than hitting an unhandled exception (the CLI'sMainhas no outertry/catch).Installer/Program.cs— the CLI's abort was nested insideif (upgradeCount > 0), so the(0, 1, 0)discovery-failure return skipped it: the CLI printed "No pending upgrades found." and ran the whole install anyway. (The history row was still written FAILED, sincetotalFailureCountfolds inupgradeFailureCount, so nothing was stranded — but the install shouldn't have run, and the Dashboard aborting while the CLI didn't is the exact divergence this branch set out to remove.) The abort now sits outside theupgradeCountblock, because discovery can fail before any hop runs.Two latent bugs the strict parse would have detonated
Both pre-existing, both found in review, both closed here:
Version.TryParse("3.1")succeeds withBuild == -1, andnew Version(3, 1, -1)throwsArgumentOutOfRangeException. Now clamped.GetAppVersionstrips a+metadatasuffix but not-prerelease. The moment<InformationalVersion>becomes3.2.0-rc1,targetVersionstops parsing — which, with the new throw, would abort every Dashboard upgrade. Rather than patch the seven call sites that each hand-strip suffixes,FilterUpgradesnow normalizes at the choke point, mirroringSingleInstanceDecision.ParseProductVersion(re-stated locally to keepInstaller.Coredependency-free). Genuine garbage like"Unreachable"still throws.The same bug, still reachable through the Dashboard's front door
Review of the repair commit found the stranding bug still live by a second route, and this one needed no failed migration at all.
AddServerDialog's pre-install discovery (:537) calledGetInstalledVersionAsyncwith the soft, null-returning overload. That method's own comment names the hazard:But
:537is not the version column. It is the discovery that decides install-vs-upgrade — the same class of caller as the CLI, which correctly passesthrowOnError: true. So a connection timeout, a database leftOFFLINE/RESTORING, or a permissions blip returnednull, read as "no database", and dropped the dialog into the fresh-install path: every migration skipped, install scripts run over the existing database,installation_historystamped SUCCESS at the target version, every pending hop stranded.Discovery now passes
throwOnError: trueand enters a newConnected_StatusUnknownstate that hides the install button and explains why. An_installBlockedReasonguard also hard-blocksInstallOrUpgrade_Click, so the protection is structural rather than depending on a hidden button.The worst bug was not in the file everyone was reviewing
Twenty-two rounds of review went at the dialog, because that is where the findings kept coming from. That was survivorship bias: findings kept coming from the dialog because it was the only place anyone was looking. When a reviewer finally looked one call deeper, it found this in
Installer.Core/InstallationService.cs:CleanInstallAsyncdrops the three SQL Agent jobs, both Extended Events sessions, and then the database itself (SET SINGLE_USER WITH ROLLBACK IMMEDIATE,DROP DATABASE) — before the first install script runs. It was the single most destructive window in the installer, and the only one with no cancellation check in front of it; the firstThrowIfCancellationRequestedsat after it. And because a cancelledSqlCommandfaults withSqlExceptionrather thanOperationCanceledException, the clean-install error handler swallowed the cancel as an ordinary failure and returned normally — so the caller's cancellation path never ran.Click Cancel Install mid-drop and the dialog announced "Installation completed with 1 error(s)" over a database that was already gone, restored the version verdict it had deliberately discarded for safety, and — because Save skips the connection test once that verdict is stamped — offered to save the server without ever reconnecting to it.
Fixed, and pinned with tests that fail against the old code (verified, not assumed) — which is possible precisely because the fix lives in
Installer.Coreand not in the dialog.The bug behind the bugs
Fourteen reviewers could not break the extracted, CI-pinned decision core. They broke the dialog every single round — and the same defect kept coming back wearing different clothes, because the server-name box stays editable during every await (a connection test is ~10s; unbounded for Entra MFA interactive auth):
That is why those four fields now have a single writer, callable only at a guarded publish point, and why the destructive checkboxes are cleared at one choke point (
TransitionToState) instead of at each exit — the previous approach had been patched into four separate exits and still missed a fifth.Four smaller fixes from the same review:
InstallCompletedisables the expanderRepairCheckBoxlives in, so after a successful repair the box couldn't be unticked — and the success message told the user to do exactly that. Clicking "Upgrade Now" would repair again, forever. Repair is now cleared automatically before the transition.Connected_Currenthad no recovery. It collapsed the install button, so a damaged-object server already at the current version had no non-destructive repair in the Dashboard, while--repairhandled it fine. It now offers Repair; withcurrent == targetthere are no migrations to skip, so it's a plain idempotent reinstall.currentVersion(aninstaller_versionvalue) intoinstaller_info_versionon repair. That column records which binary ran, soinfoVersionnow passes through unchanged.appVersionafter a repair, contradicting its own history row. Now useshistoryVersion.Blast radius
_installedVersionis only ever a real database version ornull— never a UI sentinel — so the new throw cannot fire from the Dashboard's normal flow. It fires only on genuinely corrupt version data, where failing loudly is the correct outcome.It is also one of four fields that describe the probed server (
_coreServerInfo,_serverVersion,_verdictServerName,_installedVersion), and after review they have exactly one writer:PublishProbedServer, called only at points that actually publish a verdict, always after the guard that could still abandon the publish. That coupling is load-bearing, not cosmetic — see below.How this was tested
Installer.Tests(non-DB, CI filter): 152 passed, 0 failed. The three decisions that can corrupt the ledger are extracted intoInstaller.Coreas pure functions and pinned there:InstallGuard(is installing safe?),RepairOutcome(are a repair's file failures the expected kind, and is an upgrade still pending?),InstalledVersionClassifier(clean install vs. run every migration).Installer.Testsis CI-wired;Dashboard.Testsis not, so tests placed there could never fail a PR.Dashboard.Tests: 732 passed, 0 failed.--repair --reinstall→ exit 1).Installer.Tests(VersionDetectionTests,IdempotencyTests,AdversarialTests) deliberately not run — they create production-named Agent jobs on the real test server.Sixteen rounds of adversarial review. From round 9 on, every surviving finding was in
AddServerDialog.xaml.cs— the one file with zero test coverage — and the bugs were all one of two shapes: a fact read before anawaitand acted on after it, or a predicate overDialogStatethat didn't enumerate every state. Six independent reviewers failed to break the ledger invariant or reach a destructive action without consent. That asymmetry is the honest summary of this PR: the extracted, CI-pinned core is solid; the dialog is correct only as far as careful reading gets you, which is why the manual pass below is not optional.Still needs a live click-through — I cannot drive WPF clicks
Everything below is in the untested dialog. 1–4 are the data-integrity core. 5–12 are the retarget class — the server-name box stays editable while the verdict, the version, the block reason and the destructive checkboxes all describe whichever server was last checked.
Setup: a test server at an older version, and a deliberately broken
upgrades/{from}-to-{to}/*.sql(reference a missing object).Data integrity
config.installation_history— the version check still reports the old version, so the hop is still offered. (Leaving noSUCCESSrow is exactly what makes it get re-offered.)Retarget (each one was a real bug found in review)
5. Repair a server, get the handoff, then type one character into the server box and delete it. The handoff must withdraw and come back — not vanish permanently.
6. Repair → click the handoff's Upgrade Now → let it succeed → type a character in the server box. It must not re-assert "still at v3.0.0 — the pending upgrade has not been applied".
7. Install on server A → retype the box to B. "Installation completed successfully!" must withdraw, with a notice — not sit there under B's name.
8. Get to "needs upgrade" on A, tick Perform clean install (drops existing database), then retype the box to B and click through. It must not drop B's database. (This was reachable.)
9. On "needs upgrade", type a character and delete it. Upgrade Now must come back (the verdict was mis-addressed, not wrong).
10. Point at an unsupported instance (SQL 2014) → get the block → retype to a 2016+ instance. The panel must not still say "SQL Server 2014 is not supported".
11. Click Save, and while the connection test runs (~10s), edit the server name. Save must say "nothing was saved, click Save again" — not silently do nothing.
12. Click Save → click Test Connection → click Save again. Must not throw (this assigned
DialogResultto an already-closed window; the app survives it, but you get an "An error occurred" box and a server that was silently not saved).The two I would most want a human on:
PROD01). Change the Server Name box to something else (TEST99). Click Save, and while the ~10s connection test is running, press ESC. Now reopen Manage Servers.PROD01must still bePROD01. (It wasn't: the dialog writes into the same objectServerManagerholds, so the abandoned save renamed the live server in place — whileShowDialogreported "cancelled" and nothing on screen said otherwise. It then reached disk on its own the next time any server connected.PROD01came back asTEST99, keptPROD01's id and credentials, and stopped being monitored.)DROP DATABASEruns before the first install script, so at that moment the database is already gone.config.installation_history.SQL auth specifically: after any install, confirm the monitoring-credentials panel still appears. A bug here persisted the installer's account — routinely
sa— as the ongoing monitoring login.CLI equivalent for 1–2:
PerformanceMonitorInstaller.exe <server> ...(aborts), then... --repair. A repair with a pending upgrade exits 0 (its failures are the expected kind) and now prints "Repair completed with N expected error(s)" rather than claiming a clean install.The repair path (the escape hatch the abort needs)
Aborting removes an accidental self-healing path: an upgrade script can fail because an object it
ALTERs is missing or damaged, and the full install running afterwards used to quietly recreate it. With the abort in place, the only escape hatch left would be Clean Install — which drops the database. So both front ends gain a non-destructive repair:--repair, mutually exclusive with--reinstall(which drops the database, leaving nothing to repair).Both run the idempotent install scripts without migrations, so the missing objects come back and the pending upgrade can then be re-run.
The load-bearing detail: a repair that skips pending migrations writes no
installation_historyrow at all.installer_versionis exactly whatGetInstalledVersionAsyncreads back, andFilterUpgradesonly offers hops newer than it — so stamping the target version here would strand every pending migration, i.e. re-create the bug this PR exists to fix. But writing the version we merely read is not safe either:GetInstalledVersionAsyncreturns a0.0.0sentinel meaning "installed, but the version is unreadable — try every upgrade", and persisting that as aSUCCESSrow would turn a guess into a fact. Writing nothing leaves the previous row as the version of record, so the pending upgrade is still offered.That sentinel is also why
RepairOutcomesplits what was one flag into two questions, because they disagree for exactly that value: "may I excuse these file failures?" → no (we cannot certify a success we cannot explain), and "is there an upgrade to run next?" → yes (every hop may be pending). Answering both with one flag told operators "already at the current version, so there is no upgrade to apply" for a server with every migration waiting — and withheld the button that would have applied them.The Dashboard resolves this to
repairFromVersion(the version being repaired from,nullwhen not repairing) rather than a bare bool, so the history write cannot record the old version after a clean install even if both boxes were somehow set.Related
Found while reviewing #963, whose bulk-upgrade path mirrors this same defect across N servers at once. The semantics to mirror there are called out in that review.
🤖 Generated with Claude Code