diff --git a/CHANGELOG.md b/CHANGELOG.md index c723887caf..c7e1330520 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Dashboard: the add/edit-server state machine and its predicates move to `Installer.Core`, where CI can test them** ([#1498]) — the `AddServerDialog` flow's state enum (`DialogState`) and its pure decisions — is-this-a-verdict-state, does-this-button-promise-an-existing-install, which-state-consumes-the-destructive-checkboxes, and which-connected-state-the-facts-imply — lived inside the WPF dialog, which is not wired into CI, so nothing pinned them. Every bug in that flow that survived review came in one of two shapes, and one was "a predicate over the state that silently forgot a state" (the destructive-checkbox rule missing an exit; two predicates disagreeing about the same server). The enum is now `Installer.Core.ServerSetupState` and the predicates are `Installer.Core.ServerSetupPolicy`, pinned by a full state×predicate matrix in `Installer.Tests` (CI-wired) that asserts every predicate for every state — so a switch that forgets a state fails a test instead of only misbehaving in the UI. **Behavior-preserving:** the dialog aliases the enum as `DialogState` and calls the extracted predicates, no logic changed; all 732 Dashboard tests pass unchanged. This is the testability follow-up the multi-round review of this PR kept pointing at. - **Darling + Lite: the `system_health` significance predicates are collapsed into one shared copy in `PerformanceMonitor.Common` (Tier-1 parity groundwork)** ([#1507]) — sp_HealthParser's per-category "significant / warning" predicates (WARNING scheduler status, severity >= 19 severe errors excluding the benign reset numbers, RESOURCE_MEMPHYSICAL_LOW memory-conditions/broker notifications, the `pendingTasks >= 10` CPU-task gate, the IO_SUBSYSTEM WARNING gate, the always-on 500 ms significant-wait predicate with its 90-entry idle-wait ignore list, and the ungated memory-node-OOM) were **hand-duplicated** in the Darling viewer's `SystemEventSignificance` and the headless MCP host's `DarlingSystemHealthSignificance`, and the two copies had **ALREADY drifted** — the service copy was missing the significant-wait predicate, its wait-duration constant, and the entire idle-wait ignore list. They now live once in `PerformanceMonitor.Common.SystemHealthSignificance` (the same one-source pattern `DefaultTraceEventSignificance` already uses); the viewer facade `SystemEventSignificance` **delegates** (keeping its full public surface, so viewer call sites and tests are untouched) and the service twin is **deleted** with its seven MCP-tool call sites repointed to the shared class. **Behavior-preserving** — the existing anti-drift equivalence tests and fixture round-trips stay green (84 passed, 0 failed), now pinning the shared class and guarding that the viewer facade still delegates instead of growing a third copy. Groundwork for the Lite System Events tab, so Lite consumes the shared predicates rather than becoming that third hand-maintained copy. Scoped to Common + Darling — the deprecated Full Dashboard (whose significance is server-side sp_HealthParser) is untouched. Darling + Common (Lite<->Darling parity groundwork) - **Lite and Darling: the triplicated baseline analysis MODEL is collapsed into one shared copy in `PerformanceMonitor.Analysis` (Tier 0 parity collapse)** ([#1503]) — the last Tier-0 forcing-function item. The anomaly brain's baseline MODEL was carried VERBATIM in three places (Lite `BaselineProvider`/`AnomalyDetector`, Darling `PgBaselineProvider`/`PgAnomalyDetector`, Dashboard `SqlServer*`) — a drift risk the code itself flagged in `AnomalyGate`. The shared pieces: the `BaselineBucket` model (mean/stddev/sample-count/distinct-days + `EffectiveStdDev`/`IsTrustworthy` and the six tier-gate floors), the `BaselineTier` enum, the store-agnostic `BaselineMath` (tier selection with hysteresis, hour-only/flat pooling, pooled variance, the bounded-metric dispersion floor, and the rolling-window/collapse/restore thresholds), the ~20 anomaly-detector tuning constants in `AnomalyThresholds` (deviation/ratio thresholds, the #1486 magnitude floors, the low-quality-baseline absolute-fallback bars, the sigma cap, the object-growth thresholds), and the `MetricNames` cache keys. Verified byte-identical between Lite and Darling FIRST, then extracted to shared types that BOTH active apps reference; each provider keeps only the genuinely dialect-specific part — its per-metric DuckDB/Postgres baseline SQL + parameter binding. **Behavior-preserving:** not one threshold, constant, or line of logic changed — the full Lite and Darling baseline/anomaly suites pass unchanged (the Lite tier-collapse integration tests and the Darling QUALIFY-rewrite / `EffectiveStdDev` pins exercise the extracted math). Scoped to the two ACTIVE apps: the deprecated Full Dashboard keeps its OWN frozen copy untouched (a bug on a frozen app is unacceptable risk), and the shared types live in the `PerformanceMonitor.Analysis.Baselines` sub-namespace precisely so a plain `using PerformanceMonitor.Analysis;` in the Dashboard's files can't collide with its own same-named `BaselineBucket`/`BaselineTier` (CS0104) — the Dashboard compiles unchanged. Adds a mirror-image FORCING-FUNCTION pin on each side (`SharedBaselineModelPinTests` in Lite.Tests and Darling.Tests) asserting each app binds the model to the shared assembly and holds NO per-app copy, so Lite and Darling can't silently re-fork it again. Lite + Darling (Lite<->Darling parity collapse) - **Lite: the 35 collector-table DuckDB schemas are now GENERATED from the shared `CollectorCatalog` (Tier 0 parity collapse) instead of hand-written** ([#1502]) — Lite used to hand-write 35 `CREATE TABLE` constants + 33 index constants in `Schema.cs`, plus TWO parallel hand-maintained archive lists (`DuckDbInitializer.ArchivableTables`, `ArchiveService.ArchivableTables`) — the documented "three registrations" drift trap — while Darling GENERATES the identical collector schema from `CollectorCatalog` and pins it column-for-column. That asymmetry was the single largest place Lite's store could silently diverge from Darling's. A new `DuckDbSchemaGenerator` (the DuckDB twin of Darling's `PgSchemaGenerator`) now walks `CollectorCatalog.All` and emits each collector's `CREATE TABLE`/`CREATE INDEX`; `Schema.cs` keeps only the 7 non-collector tables (registry / schedule / log / alert-coordination), and both archive lists derive from the catalog. A collector column added in one place now flows to Lite, Darling, AND the archive/purge machinery automatically. **This is a storage-schema change gated on a hard equivalence proof, not a refactor taken on faith:** the generated DDL is demonstrated byte-for-byte STORAGE-equivalent to the pre-generation hand-written tables for all 35 tables (`DuckDbSchemaEquivalenceTests` executes both the frozen pre-change DDL and the freshly generated DDL against DuckDB and compares `PRAGMA table_info` row-for-row — identical columns, DuckDB types, order, NOT NULL, DEFAULT, PRIMARY KEY — plus matching indexes). The two things the engine-neutral catalog does not carry but Lite's DuckDB schema has always enforced — the prefix `PRIMARY KEY` and NOT NULL on the always-populated key/name/count payload columns, plus the lone `query_snapshots.is_cdc_capture DEFAULT false` (Darling deliberately drops both for its hypertable/COPY path) — are reproduced exactly by a small, pinned, Lite-local constraint overlay, and Lite's historical irregular index names (`idx_cpu_time`, `idx_query_store_time`, the composite `idx_index_object_stats_object`, `memory_pressure_events` on `sample_time`, …) are preserved verbatim so an existing store's `CREATE INDEX IF NOT EXISTS` matches rather than adding a redundant index. **Existing DuckDB stores are unaffected**: every statement is `CREATE ... IF NOT EXISTS` (a no-op on an already-created table), the 45-step migration ladder and `CurrentSchemaVersion` are untouched, so the generator only shapes FRESH stores — and it shapes them identically to before. Adds a FORCING-FUNCTION pin (`DuckDbSchemaGeneratorTests`) asserting every generated table's columns + types equal the catalog definition, so a catalog change must flow to Lite and no hand-edit can silently diverge. Full Lite.Tests green (1125 tests incl. every collector round-trip, which exercises the positional appender against the generated schema). Lite (Lite<->Darling parity groundwork) @@ -72,6 +73,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Clean install is Azure SQL Managed Instance-safe, and a failed drop no longer leaves the database bricked** ([#1498]) — the clean-install teardown ran `ALTER DATABASE ... SET SINGLE_USER WITH ROLLBACK IMMEDIATE` before `DROP DATABASE` to evict other connections. `SINGLE_USER` is non-modifiable on Azure SQL Managed Instance (`SERVERPROPERTY('EngineEdition') = 8`), so the whole teardown batch died on that line there; it is now gated to box SQL Server (MI drops directly, relying on there being no other sessions, which an installer teardown normally guarantees). And because `SINGLE_USER` and `DROP` are two separate autocommitted statements, a `DROP` that failed (another session racing the single-user slot) or hit the client command-timeout/cancel *after* `SINGLE_USER` committed left the database online but stranded in `SINGLE_USER` — semi-bricked. The teardown now restores `MULTI_USER` best-effort on a fresh connection before re-raising the original error (a client-attention timeout is invisible to a server-side `TRY/CATCH`, so this has to be C#-side); the Dashboard's server-drop path warns if that restore itself fails. Applies to `InstallationService.CleanInstallAsync` and the Dashboard's "remove server" drop; the SQL constructs were verified against Microsoft Learn for both box SQL Server and Managed Instance. +- **CLI installer: a password beginning with `--` gets a clear message instead of a misleading "password required"** ([#1498]) — a value like `--h7x` passed as the SQL-auth password is indistinguishable from a flag, so the positional-argument filter dropped it and the run then reported *"Password is required"* — for a password that *was* supplied. The error now adds a targeted note (only when a `--`-token that is not a recognized flag actually appeared) explaining that such a password must go through `PM_SQL_PASSWORD`. The parsing is unchanged — deliberately, since accepting `--`-prefixed values positionally is what would make a mistyped flag become a password. +- **Dashboard: a failed upgrade migration no longer stamps the database as successfully upgraded** ([#1498]) — `AddServerDialog` logged *"Upgrade failures detected. Continuing with full installation to ensure consistency..."* when an `upgrades/*` script failed, ran the full install over the partially-upgraded database anyway, and then wrote an `installation_history` row using `InstallationResult.Success` — which is `FilesFailed == 0` for the **install files only** and excludes upgrade failures entirely. That row was permanent damage: version detection reads the most recent `installation_status = 'SUCCESS'` row, and upgrade discovery only offers hops newer than the detected version, so the failed migration was stamped SUCCESS at the target version, the server then reported as current with the `ALTER`s never applied, and the hop was **never offered again** — silent schema drift recoverable only by a clean install. `ExecuteAllUpgradesAsync` already documented the contract ("*the caller aborts the whole install when totalFailureCount > 0*") and the CLI installer honored it; only the Dashboard's single-server path did not. The dialog now aborts on upgrade failure without running the install or writing a history row — leaving no `SUCCESS` row is exactly what makes the hop get re-offered, and upgrade scripts are written with guarded `ALTER`s so a re-run after fixing the error resumes cleanly — and folds the upgrade counts into the history write, matching the CLI. Relatedly, `ScriptProvider.FilterUpgrades` now throws on a version that is present but has no parseable numeric core instead of silently returning "no upgrades needed" (an empty result must never be the answer to "I could not read the version you gave me" — that path is what let a status string like `"Unreachable"` skip every migration while reporting zero failures), and `ExecuteAllUpgradesAsync` reports that as a failure so callers abort through the existing contract rather than hitting an unhandled exception. The **CLI**'s abort was itself nested inside `if (upgradeCount > 0)`, so a discovery failure (which reports zero hops) slipped past it — the CLI printed *"No pending upgrades found."* and ran the whole install; the abort now sits outside that block. Two latent version-parsing bugs the strict parse would have detonated are closed at the same choke point: `Version.TryParse("3.1")` succeeds with `Build == -1`, which the `Version` ctor rejects (now clamped), and `GetAppVersion` strips a `+metadata` suffix but not `-prerelease` — so a `3.2.0-rc1` `` would have hard-aborted every upgrade. `FilterUpgrades` now normalizes SemVer suffixes at the choke point rather than relying on the seven call sites that each hand-strip them, mirroring `SingleInstanceDecision.ParseProductVersion`. The two decisions that can corrupt the ledger — "is installing safe?" (`InstallGuard`) and "are a repair's file failures the expected kind?" (`RepairOutcome`) — are extracted into `Installer.Core` and shared by both surfaces rather than hand-copied, and pinned by regression cases in `Installer.Tests`, which is CI-wired (`Dashboard.Tests` is not, so tests placed there could never fail a PR). One caveat worth recording: the ledger-lost probe is SQL, so its only test lives in `VersionDetectionTests`, which CI excludes because it needs a real SQL Server — that query was verified by hand against SQL Server 2022, but it cannot fail a PR. Found while reviewing [#963]. +- **Dashboard: a failed version check no longer reinstalls over an existing database and reports it as up to date** ([#1498]) — `AddServerDialog`'s pre-install discovery called `GetInstalledVersionAsync` with the *soft*, null-returning overload. That method'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)", and only *soft callers* (the version column) keep the null fallback. But this call is not the version column — it is the discovery that decides install-vs-upgrade, the same class of caller as the CLI. So a connection timeout, a database left `OFFLINE`/`RESTORING`, or a permissions blip came back as `null`, read as "no database", and dropped the dialog into the fresh-install path: every migration skipped, install scripts run over the existing database, and `installation_history` stamped `SUCCESS` at the target version — stranding every pending hop exactly as above. Discovery now fails loudly and the dialog enters a new "status unknown" state that hides the install button and explains why; an `_installBlockedReason` guard also hard-blocks the install path, so the protection is structural rather than depending on a hidden button. The version is re-read — and the safety verdict re-run — against the connection the installer is about to *write* to, since the server box stays editable after a connection test. +- **A non-destructive repair path, so aborting on a failed migration doesn't leave you stuck** ([#1498]) — an upgrade script can fail *because* an object it `ALTER`s is missing or damaged, and before the abort above, the full install running afterwards would quietly recreate it. With the abort in place the only remaining escape hatch was Clean Install, which drops the database. **Dashboard** gains a "Repair: reinstall objects, skip upgrade scripts" option in Advanced Options (mutually exclusive with clean install; the upgrade-abort message now points at it), and the **CLI installer** gains `--repair` (mutually exclusive with `--reinstall`). Both run the idempotent install scripts *without* running migrations, so missing objects come back and the pending upgrade can then be re-run. Critically, a repair *that skips pending migrations* writes **no** `installation_history` row: 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 the `0.0.0` sentinel as a fallback meaning "unknown, try every upgrade", and persisting that as a `SUCCESS` row would make the guess true). Writing nothing leaves the previous row as the version of record, so the pending upgrade is still offered. Note that a repair against a database with pending migrations is *expected* to report errors on a few procedures: the install scripts compile against the current schema (`install/23_process_blocked_process_xml.sql` reads a column the `3.0.0-to-3.1.0` migration adds) and `ALTER PROCEDURE` binds columns at compile time, so those objects cannot compile until the upgrade runs. A failed `CREATE OR ALTER` leaves the old body intact, nothing is damaged, and the upgrade recompiles them — so the "run the upgrade next" handoff is deliberately not gated on a clean repair. +- **`--repair` exit-code contract** ([#1498]) — a repair with a *pending upgrade* exits **0** even with failed files, because those failures are the expected compile errors described above; reporting them as a failure sent operators reaching for `--reinstall`, which drops the database. A repair with **no** pending upgrade has no such excuse, so a failure there still exits non-zero. A third case: when the server's recorded version cannot be read at all, the failures cannot be excused (exit non-zero) but an upgrade *is* still offered — those are two different questions, and answering both with one flag told operators "already at the current version, so there is no upgrade to apply" for a server with every hop pending. `--repair` also now refuses when there is no installation to repair, rather than silently performing a full fresh install and stamping the target version on a mistyped server. +- **Dashboard: retyping the server name no longer leaves the previous server's answers on screen** ([#1498]) — the Add/Edit Server box stays editable while the connection status, installed version, safety verdict and the Advanced Options checkboxes all describe whichever server was last *checked*. Nothing re-read the box, so every one of those could end up describing a different server than the one named on screen, and one of them is destructive: a ticked "clean install" (which runs `DROP DATABASE PerformanceMonitor` and drops the Agent jobs and Extended Events sessions) survived a retarget and could fire against a server that was never consented to, with no confirmation. Each verdict is now stamped with the server it was actually read from, and the dialog refuses to render one whose stamp no longer matches the box — a single structural check, rather than a re-check remembered at each of the dozen places that display or act on it. Related, from the same root: a connection test still in flight when an install started could wake up afterwards and republish its *pre-install* verdict over the finished result (restoring "Upgrade Now" on a server that had just been upgraded); **Save** could close the dialog the instant an install landed, which under SQL authentication skipped the monitoring-credentials prompt entirely and persisted the *installer's* account — routinely `sa` — as the ongoing monitoring login; and **Save** would skip the connection test altogether after an install, so a name typed into the box afterwards was saved without ever being contacted. +- **Cancelling an install is reported as a cancellation, not a failure — at every stage** ([#1498]) — a cancelled `SqlCommand` faults with `SqlException`, not `OperationCanceledException`, so two windows in the installer were counting a cancel as an ordinary failure and returning a normal result instead of surfacing the cancel: the clean-install teardown (before any install file runs), and the install file loop itself (a cancel during a *critical* file, which aborts the loop, or during the *last* file, which ends it — neither returns to the loop-top cancellation check). Both now convert a cancel into `OperationCanceledException` so the caller takes its cancellation path rather than announcing *"Installation completed with N error(s)."* +- **Cancelling a clean install no longer reports success over a database it just dropped** ([#1498]) — a clean install drops 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. That was the single most destructive window in the installer, and it was the only one with no cancellation check in front of it — the first `ThrowIfCancellationRequested` sat *after* it. Worse, a cancelled `SqlCommand` faults with `SqlException`, not `OperationCanceledException`, so the clean-install error handler swallowed the cancel as an ordinary failure and returned **normally**: the caller's cancellation path never ran. Clicking **Cancel Install** mid-drop produced *"Installation completed with 1 error(s)"* over a database that was already gone, restored the version verdict the dialog had deliberately discarded for safety, and — because **Save** skips the connection test once that verdict is stamped — let the server be saved without ever being reconnected to. A cancel is now surfaced as a cancel, and a clean install that *fails* is treated the same way: no history row, no verdict, the install log left on screen, and installing blocked until the server is re-checked. Pinned by regression tests, which is possible because the fix lives in `Installer.Core` rather than in the dialog. +- **Dashboard: "Save" is no longer enabled on a server with no PerformanceMonitor database** ([#1498]) — the `Connected_NoDatabase` state disables Save deliberately: the intended paths are **Install Now**, or the explicit *"Skip, just add server"* link (which re-enables Save itself). A button-gating change made while fixing an unrelated overlapping-probe bug re-enabled Save unconditionally after every status check, which handed that consent away on the most ordinary path in the product — point at a bare instance, click Check for Updates, read *"No PerformanceMonitor database found"*, and Save is live. It is also the default button, so **Enter** would commit a server the Dashboard can read nothing from. +- **Dashboard: cancelling an edit no longer silently renames the server you were editing** ([#1498]) — the Add/Edit Server dialog writes its fields straight into the `ServerConnection` object it was handed, and in edit mode that object *is* the one `ServerManager` holds — not a copy. Clicking **Save** and then closing the dialog (ESC, Cancel, the X) while the connection test was still running let the test finish and complete the save anyway: the dialog was gone, `ShowDialog` had already returned "cancelled", and the caller never persisted anything — but the live object had already been mutated in place, and it reached disk on its own the next time any server's `last_connected` was written. A `PROD01` retargeted to `TEST99` and abandoned would come back as `TEST99` under `PROD01`'s id and credentials, and `PROD01` would stop being monitored, with nothing on screen ever having said so. +- **Dashboard: a failed or cancelled clean install no longer hides the install log** ([#1498]) — the clean-install path deliberately discards the pre-install version (the database is about to be dropped, so a verdict describing it would be a lie), and the "no verdict to restore" branch that handles that returned *before* the line that re-shows the installation panel — which the surrounding code had collapsed. The install log is the only place the actual SQL error text appears, and `DROP DATABASE` runs *before* the install scripts, so a cancel or failure at any point afterwards left the user with a dropped database, a single exception message, and no log. That branch also told them "this server's installed version was not read", which on that path is false — it was read, and deliberately discarded. +- **Both installers now refuse to run over a database that is NEWER than the binary** ([#1498]) — an older Dashboard or CLI installing over a server someone upgraded with a newer build would run its own older scripts, reverting every `CREATE OR ALTER` procedure and view to the older definitions, and then record the **lower** version as `SUCCESS`. Because upgrade discovery only offers hops *newer* than the current version, that case yields zero hops and zero failures, so no abort caught it. Both surfaces now hard-block such a server, as they do a version that will not parse at all. In the Dashboard everything is blocked, clean install included, so the block message carries the whole escape route: update the Dashboard, or use the CLI installer's destructive `--reinstall` (which drops the database first, so there is nothing left to downgrade). - **Lite and Darling: the collector gate surface is collapsed to ONE shared layer, so Darling stops attempting collectors it can't run (no-msdb / Azure SQL DB / pre-2016) every cycle** ([#1500]) — the direct sequel to the [#1492] RDS fix, closing the whole gating-drift class it exposed. Lite gated collectors in TWO places — the shared `ICollectorSchemaInfo.AppliesTo` (which Darling also honors) AND its own host-side `IsCollectorSupported` — while Darling gated ONLY on `AppliesTo`, so every gate Lite kept solely in the second layer was silently ignored by Darling: on a monitored login without msdb access Darling attempted `running_jobs`/`job_history`/`agent_status` every cycle (each failing 229/916 and polluting collection-health), on Azure SQL DB it attempted `server_config`/`trace_flags`, and neither SKU version-gated `query_stats`/`query_store` off a pre-2016 box in the shared layer. **The second layer is gone.** `CollectorTargetInfo` gains a `HasMsdbAccess` flag (default true = assume access, matching the probe's NULL semantics), populated in BOTH hosts from the `HAS_DBACCESS(N'msdb')` value each already probed — Darling had it on `ServerRuntime` but never wired it into the gate (the exact missing link), Lite reads it from `ServerConnectionStatus`. Each gate moved into the correct collector's shared `AppliesTo`, matching Lite's exact prior condition: `ServerConfigCollector`/`TraceFlagsCollector` → `!IsAzureSqlDb`; `QueryStatsCollector`/`QueryStoreCollector` → `SqlMajorVersion == 0 || >= 13 || IsAzureSqlDb || IsAzureManagedInstance`; `RunningJobsCollector`/`AgentStatusCollector` add `&& HasMsdbAccess` atop their existing `!IsAzureSqlDb && !IsAwsRds`, and `JobHistoryCollector` adds `&& HasMsdbAccess` atop `!IsAzureSqlDb` (still NOT RDS-gated — it never touches `syssessions`). `AppliesTo` is hoisted from `ICollectorDefinition` up to the non-generic `ICollectorSchemaInfo` so `CollectorCatalog.AppliesTo(name, target)` can evaluate the gate by name without the row type — the ONE surface both SKUs consult. Lite's `IsCollectorSupported` is DELETED; `RunCollectorAsync` now calls `CollectorCatalog.AppliesTo` pre-dispatch, preserving its clean SKIPPED log (a genuine skip with no `collection_log` row, vs. the SUCCESS/0-rows a gated collector would otherwise record) — behavior-identical for Lite (same gates, relocated), and INTENTIONALLY corrective for Darling (it now skips these targets). The gate CONDITION lives ONLY in each definition's `AppliesTo`, so the two hosts can't drift on it again. Covered by extended per-collector `AppliesTo` tests (the msdb / config / version dimensions) plus a new `CollectorGateSurfacePinTests` that pins every moved gate's truth table (Azure SQL DB / AWS RDS / no-msdb / pre-2016) AND asserts `CollectorCatalog.AppliesTo(name, target)` agrees with each definition's own `AppliesTo` for every catalog collector across every target — the proof both SKUs now gate identically off one surface. Lite + Darling - **Darling viewer: the collector-schedule presets now include `job_history`, `agent_status`, and `default_trace_events`, matching Lite** ([#1495]) — the viewer's Aggressive / Balanced / Low-Impact preset tables (`CollectorSchedulePresets.Presets`, duplicated byte-for-byte from Lite's `ScheduleManager.s_presets`) were missing these three collectors in **all three** presets, so applying a preset in the Darling schedule editor left them pinned at their code-default cadence instead of the preset's — a drift flagged during the [#1494] review. Each preset now carries the three at Lite's exact interval (Aggressive 2 min, Balanced 5 min, Low-Impact 15 min); retention is untouched (it lives in the shared `CollectorScheduleDefaults`, which already carried all three, so `BuildDefaultSchedule` was already correct — only the preset-apply path drifted). The class docstring promised "a Darling.Tests pin asserts they cannot drift from Lite's intervals," but no membership pin actually existed — the existing `Presets_OnlyReferenceKnownCollectors` only checks the reverse direction (no *unknown* collectors), which is why the missing three went uncaught. A new `Presets_CoverTheSamePinnedCollectorSet` test mirrors Lite's own `SchedulePresets_CoverTheSamePinnedCollectorSet`, pinning the full 27-collector membership of every preset so a future collector added to one SKU but not mirrored into all three Darling presets fails here instead of shipping silently. Darling - **Lite: the `v_job_history` and `v_default_trace_events` archive views no longer double-count rows after a 512 MB emergency reset** ([#1492]) — both views `UNION` the hot table with the parquet archive, but `ArchiveService.ArchiveAllAndResetAsync` archives all hot data to parquet AND wipes `collection_log`, so the next cycle re-collects recent history into the hot store while parquet still holds it. The local surrogate prefix id is a per-process counter (`CollectionIdGenerator`), so the re-collected rows get brand-new ids and the plain `UNION ALL` showed each logical event twice. `CreateArchiveViewsAsync` now dedups each on its SQL-Server-side natural key via `QUALIFY ROW_NUMBER() OVER (PARTITION BY ORDER BY collection_time DESC) = 1`, keeping the newest-collected (hot) copy — `job_history` on `(server_id, instance_id)` (the `sysjobhistory` identity that survives `sp_purge_jobhistory`), `default_trace_events` on `(server_id, event_time, event_sequence)` (the trace `EventSequence`, paired with `event_time` so it stays distinct across the server restarts that reset it). Other archivable tables are unaffected — normal archival keeps hot and parquet disjoint, so only these two re-collect over still-archived rows. Covered by a new `ArchiveViewDedupTests` that stages the archive→reset→re-collect sequence and asserts each natural key appears exactly once with the hot copy winning. Lite @@ -287,6 +301,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1501]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1501 [#1500]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1500 [#1499]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1499 +[#1498]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1498 +[#963]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/963 [#1495]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1495 [#1494]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1494 [#1493]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1493 diff --git a/Dashboard/AddServerDialog.xaml b/Dashboard/AddServerDialog.xaml index 5f9a2afef6..84f74c4621 100644 --- a/Dashboard/AddServerDialog.xaml +++ b/Dashboard/AddServerDialog.xaml @@ -34,6 +34,7 @@ @@ -218,6 +219,12 @@ + "2.4.1" */ return $"{version.Major}.{version.Minor}.{version.Build}"; } - return "0.0.0"; + return "Unknown"; } /// /// Normalize a version string to 3-part for comparison (e.g., "2.4.1.0" -> "2.4.1"). /// - private static string NormalizeVersion(string version) - { - if (Version.TryParse(version, out var parsed)) - { - return $"{parsed.Major}.{parsed.Minor}.{parsed.Build}"; - } - return version; - } + /* + Reduce a version to its three-part numeric core, stripping any SemVer build (+sha) or + pre-release (-rc1) suffix. Without the strip, a "3.2.0-rc1" InformationalVersion fails to + parse, and the install/upgrade routing below then treats EVERY server as up to date. The + Build clamp matters for the same reason: TryParse("3.1") succeeds with Build == -1, which + would otherwise format as "3.1.-1" and fail to re-parse. Matches ScriptProvider.ParseVersionCore. + */ + private static string NormalizeVersion(string version) => + ScriptProvider.TryParseVersionCore(version)?.ToString() ?? version; private bool ValidateInputs() { if (string.IsNullOrWhiteSpace(ServerNameTextBox.Text)) { MessageBox.Show( + this, "Please enter a server name or address.", "Validation Error", MessageBoxButton.OK, @@ -359,6 +414,7 @@ private bool ValidateInputs() if (SqlAuthRadio.IsChecked == true && string.IsNullOrWhiteSpace(UsernameTextBox.Text)) { MessageBox.Show( + this, "Please enter a username for SQL Server authentication.", "Validation Error", MessageBoxButton.OK, @@ -372,6 +428,7 @@ private bool ValidateInputs() if (string.IsNullOrWhiteSpace(ServicePrincipalClientIdBox.Text)) { MessageBox.Show( + this, "Please enter the Application (Client) ID for service principal authentication.", "Validation Error", MessageBoxButton.OK, @@ -383,6 +440,7 @@ private bool ValidateInputs() if (string.IsNullOrEmpty(ServicePrincipalSecretBox.Password)) { MessageBox.Show( + this, "Please enter the client secret for service principal authentication.", "Validation Error", MessageBoxButton.OK, @@ -395,10 +453,38 @@ private bool ValidateInputs() return true; } - private async System.Threading.Tasks.Task<(bool Connected, string? ErrorMessage, bool MfaCancelled, string? ServerVersion)> RunConnectionTestAsync(Button triggerButton) + private async System.Threading.Tasks.Task<(bool Connected, string? ErrorMessage, bool MfaCancelled)> RunConnectionTestAsync(Button triggerButton) { + /* + Disable BOTH entry points, not just the one that was clicked. Test Connection stayed live during + a SAVE's test, so two of these could overlap -- and then the second one borrowed the FIRST one's + "Testing connection..." label and dutifully 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; _saveInProgress catches that, but two concurrent connection + tests are nonsense in the first place.) One test at a time; the finally re-arms both. + */ triggerButton.IsEnabled = false; SaveButton.IsEnabled = false; + TestConnectionButton.IsEnabled = false; + CheckForUpdatesButton.IsEnabled = false; + + /* + StatusText may already be carrying something the user still needs -- most importantly the + "the server name changed, what is on screen may describe a different server" notice, which is + the ONLY thing on screen after a retarget withdraws the post-run panels. Blanking it in the + finally below left a dialog with no panels and no explanation whenever the test then failed. + Borrow it, and give it back. + + Epoch-stamped, because the borrow spans the awaits and the give-back happens after them -- the + same read-before / write-after shape this whole branch exists to kill, and giving it back + unconditionally resurrected a notice that had already stopped being true. (Retype away, click + Test Connection, retype BACK during the connect: the screen correctly restores itself, then the + failed test's finally pastes the stale "server name changed" notice back over it.) If anything + bumped the epoch while we were away, the label is no longer ours to restore. + */ + int statusEpoch = _detectEpoch; + string priorStatus = StatusText.Text; + var priorStatusVisibility = StatusText.Visibility; StatusText.Text = EntraMfaAuthRadio.IsChecked == true ? "Testing connection — please complete authentication in the popup window..." @@ -408,7 +494,6 @@ private bool ValidateInputs() bool connected = false; string? errorMessage = null; bool mfaCancelled = false; - string? serverVersion = null; try { /* Connect to master (not PerformanceMonitor) so the test succeeds @@ -418,9 +503,16 @@ happens after the connection test in DetectDatabaseStatusAsync() */ builder.InitialCatalog = "master"; await using var connection = new SqlConnection(builder.ConnectionString); await connection.OpenAsync(); + + /* + Kept as a round-trip liveness check -- Open() alone can be satisfied from the pool -- but the + version it returns is deliberately DISCARDED. It used to be handed back and assigned straight + to _serverVersion, which is one of the four facts that may only be committed together, by + PublishProbedServer. DetectDatabaseStatusAsync re-probes and publishes them as a set. + */ using var cmd = new SqlCommand("SELECT @@VERSION", connection); - var version = await cmd.ExecuteScalarAsync() as string; - serverVersion = version?.Split('\n')[0]?.Trim(); + _ = await cmd.ExecuteScalarAsync(); + connected = true; } catch (Exception ex) @@ -432,13 +524,28 @@ happens after the connection test in DetectDatabaseStatusAsync() */ } finally { - triggerButton.IsEnabled = true; - SaveButton.IsEnabled = true; - StatusText.Text = string.Empty; - StatusText.Visibility = System.Windows.Visibility.Collapsed; + /* Do not re-arm over a running install: SetFormEnabled(false) disabled these deliberately. */ + if (!InstallInProgress) + { + triggerButton.IsEnabled = true; + SaveButton.IsEnabled = true; + TestConnectionButton.IsEnabled = true; + CheckForUpdatesButton.IsEnabled = true; + + /* + Give back what we borrowed -- but only if it is still ours. A retype or a newer detection + during the await means the label has moved on, and restoring it would be pasting a stale + notice over a fresher one. On success the detection repaints it anyway. + */ + if (statusEpoch == _detectEpoch) + { + StatusText.Text = priorStatus; + StatusText.Visibility = priorStatusVisibility; + } + } } - return (connected, errorMessage, mfaCancelled, serverVersion); + return (connected, errorMessage, mfaCancelled); } private async void CheckForUpdates_Click(object sender, RoutedEventArgs e) @@ -454,8 +561,12 @@ private async void CheckForUpdates_Click(object sender, RoutedEventArgs e) } finally { - CheckForUpdatesButton.IsEnabled = true; CheckForUpdatesButton.Content = "Check for Updates"; + /* Do not re-arm over a running install: SetFormEnabled(false) disabled this deliberately. */ + if (!InstallInProgress) + { + CheckForUpdatesButton.IsEnabled = true; + } } } @@ -463,18 +574,42 @@ private async void TestConnection_Click(object sender, RoutedEventArgs e) { if (!ValidateInputs()) return; - var (connected, errorMessage, mfaCancelled, serverVersion) = await RunConnectionTestAsync(TestConnectionButton); + /* + The server this test is actually about, captured with the connection string built from it. The + box is editable for the whole ~10s test (unbounded on Entra MFA), and the failure message below + used to re-read it AFTER the await -- so retyping during the connect produced "Could not connect + to A" about a server that is fine, for a failure that belonged to B. The last read-the-box-after- + the-await in this file. + */ + string testedServerName = ServerNameTextBox.Text.Trim(); + + var (connected, errorMessage, mfaCancelled) = await RunConnectionTestAsync(TestConnectionButton); + + /* + The dialog was closed while this test ran. Round 19 swept exactly this out of SaveAsync and left + the same shape sitting in its two siblings: without it, the failure and MFA-cancel boxes below + pop up with no dialog behind them -- one of them helpfully telling the user to "Click Test to try + again" on a window that no longer exists -- and the success path runs a full two-round-trip + detection against a destroyed one. + */ + if (_dialogClosed) return; if (connected) { - _serverVersion = serverVersion; - - /* Show connection + database status inline instead of a popup */ + /* + Deliberately does NOT write _serverVersion. It is one of the four facts that may only ever be + committed together, by PublishProbedServer -- and this write was the last place that set one + of them on its own, post-await, with no stamp beside it. The box is editable during that + await, so it could stamp the PREVIOUS server's engine version into place while the verdict + and the state still described someone else. DetectDatabaseStatusAsync re-probes and publishes + them as a set, which is where it belongs. + */ await DetectDatabaseStatusAsync(); } else if (mfaCancelled) { MessageBox.Show( + this, "Authentication was cancelled. Click Test to try again.", "Authentication Cancelled", MessageBoxButton.OK, @@ -485,7 +620,8 @@ private async void TestConnection_Click(object sender, RoutedEventArgs e) { var detail = errorMessage != null ? $"\n\nError: {errorMessage}" : string.Empty; MessageBox.Show( - $"Could not connect to {ServerNameTextBox.Text}.{detail}\n\nPlease check:\n" + + this, + $"Could not connect to {testedServerName}.{detail}\n\nPlease check:\n" + "• Server name/address is correct\n" + "• Server is accessible from this machine\n" + "• Firewall allows SQL Server connections\n" + @@ -498,80 +634,791 @@ private async void TestConnection_Click(object sender, RoutedEventArgs e) } } + /* + True once an install has started. Detection runs are async and can still be in flight when the + user starts an install, and their continuation would otherwise transition back to a Connected_* + state -- un-collapsing the panel over a RUNNING install, re-showing the install button, and + re-enabling Advanced Options. From there a second click starts a concurrent install (and a + newly-reachable "Clean install" tick drops the database out from under the first one). + Disabling the buttons is not enough: the detection paths re-enable them in their finally blocks. + */ + private bool InstallInProgress => ServerSetupPolicy.IsInstalling(_currentState); + + /* + Two detections can be in flight at once (Test Connection re-arms its button in a finally before + the detection it triggered has finished, and Check for Updates disables only its own). They then + race to write _installedVersion / _coreServerInfo / _currentState, and a late-landing one renders + the FIRST server's verdict under the SECOND server's name. Each run takes an epoch; a superseded + continuation discards itself instead of publishing a verdict nobody asked for. + */ + private int _detectEpoch; + + /* + TWO different things, deliberately two fields. Conflating them is what let a button reading + "Upgrade Now" perform a full fresh install on a retargeted bare server: + + _launchState -- where the click came FROM, i.e. what the button PROMISED. Fixed at click + time and never re-derived. Only meaningful for "did the user ask us to act + on an existing installation?". + _preInstallState -- where a failure should RESTORE to, i.e. what the FACTS say. Always a + Connected_* state, and refreshed from the install-time re-read, because the + server box stays editable and the facts may belong to a different server. + + One field served both, was consumed for the promise BEFORE being re-derived for the restore, and + so answered the promise question with a value that could be InstallComplete -- exactly the state + the repair handoff puts a live "Upgrade Now" button in. + */ + private DialogState _launchState = DialogState.Initial; + + /* Null until THIS server's version has actually been read. See RestoreAfterInstall. */ + private DialogState? _preInstallState; + + /* + Which of the two reasons nulled _preInstallState: a clean install throwing the verdict away on + purpose, or the run ending before the version was ever read. They get different words, and reset + per run -- a flag that outlives its run is how the repair handoff ended up describing an upgrade + that had already been applied. + */ + private bool _verdictDiscardedByCleanInstall; + + /* + THE structural guard. Every critical found in rounds 6-10 was one bug wearing different clothes: + the server box stays editable, so _installedVersion / _coreServerInfo / _currentState can all + describe a server that is no longer the one in the box -- and some consumer renders or acts on + them anyway. Nine rounds of patching each consumer in turn left the next one uncovered. + + So the verdict is stamped with the server it was read FROM, and TransitionToState refuses to + render a Connected_* verdict whose stamp no longer matches the box. One check, one place, + structurally uncheatable: a stale verdict cannot be displayed, whoever forgets to re-check. + */ + private string? _verdictServerName; + + private void RecordVerdictFor(string serverName) => _verdictServerName = serverName.Trim(); + + /* + The probed server's three facts are committed TOGETHER, and only where a verdict about it is + actually published -- never in the prologue, and never before a guard that can still abandon the + publish. + + They have to move as one because the stamp is what every consumer tests before rendering the other + two. Hoisting the stamp to the top of the probe (so that a BLOCK would be attributed) looked + harmless -- "re-stamped on the success path with the same value" -- but it is only re-stamped if + that path is REACHED. A detection superseded at the version read returned having already moved the + stamp and the engine version to the new server, while _installedVersion and _currentState still + described the old one. VerdictMatchesServerBox() then answered TRUE, so the guard did not merely + miss the lie, it AFFIRMED it: "PerformanceMonitor v3.1.0 is up to date", with Save enabled, over a + server that had no PerformanceMonitor database at all. + + A half-replaced set of facts is the one thing the guard cannot catch. Stale-but-consistent it + catches every time. + + _installedVersion is in here for that reason and no other. It is the FOURTH fact, and leaving it + behind at the two block sites -- where we either never read it (unsupported version) or could not + (the read threw) -- left them describing the PREVIOUS server. That is sealed today only because a + block lands in Connected_StatusUnknown, which happens to collapse both buttons: a rendering + accident standing in for an invariant, which is the exact thing this file has been bitten by over + and over. A block passes null, because null is the truth there: we do not know. + */ + private void PublishProbedServer( + ServerInfo info, + string serverVersion, + string serverName, + string? installedVersion) + { + _coreServerInfo = info; + _serverVersion = serverVersion; + _installedVersion = installedVersion; + RecordVerdictFor(serverName); + } + + /* + The previous run's install log is discarded WITH the verdict that replaces it, never ahead of it. + + Clearing it in the detection's prologue looked equivalent and was not: a probe that publishes NOTHING + -- superseded, or the not-connected branch that deliberately refuses to move the four facts -- left + the screen still in InstallComplete with the panel visible, now showing a blank log, an empty status + line and a 0% bar. On a FAILED install that destroys the only on-screen copy of the SQL error text, + and nothing repopulates it: RenderPostRunClaims only shows and hides the panel. + + Same rule as _installBlockedReason, for the same reason: state that describes a verdict is committed + with the verdict. Called only from the detection -- the install path clears the log itself, at the + top of the run, and must not have it wiped from under the run it is recording. + */ + private void DiscardPreviousRunLog() + { + InstallLogTextBox.Clear(); + InstallStatusText.Text = string.Empty; + InstallProgressBar.Value = 0; + } + + /* + The repair handoff's version claim, kept so the handoff can be RE-RENDERED rather than painted + once. Painting it once made withdrawing it one-way: a retarget correctly hid the claim and the + "Upgrade Now" button, but typing the server name back -- or typing one character and deleting it -- + never brought them back, so the only button that applies the pending upgrade was gone for good + 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. + */ + private string? _handoffStatusText; + + /* + The verdict state a stale-name demotion came FROM, so retyping the name back can undo it. Set only + by that demotion and cleared by every other transition, so a REAL block -- unsupported SQL version, + unreadable version, database newer than the binary -- can never be typed away. + */ + private DialogState? _demotedFromState; + + /* + Idempotent, and safe to call on every keystroke. No-ops unless a repair actually left an upgrade + pending -- for an ordinary install, upgrade or reinstall there is no handoff to render. The reason + the panels went away is stated once, by RenderPostRunClaims; this just shows or hides them. + MonitoringCredsPanel is a separate grid row and is deliberately untouched: the SQL-auth user still + needs it either way. + */ + private void RenderRepairHandoff(bool addressed) + { + if (_handoffStatusText == null) return; + + if (!addressed) + { + DatabaseStatusPanel.Visibility = Visibility.Collapsed; + InstallUpgradeButton.Visibility = Visibility.Collapsed; + return; + } + + ConnectionInfoText.Text = $"Connected to {_verdictServerName}"; + DatabaseStatusText.Text = _handoffStatusText; + DatabaseStatusPanel.Visibility = Visibility.Visible; + InstallUpgradeButton.Content = "Upgrade Now"; + InstallUpgradeButton.Visibility = Visibility.Visible; + + /* + That Border also holds the "Skip, just add server" link, which would otherwise become clickable + for the first time in MonitoringCredentials -- an abandon-the-upgrade action sitting inches from + the Upgrade Now button we just put there. + */ + SkipInstallText.Visibility = Visibility.Collapsed; + } + + /* + Editing the server name invalidates everything on screen: the verdict, the connection info, and any + detection still in flight all describe the server that WAS in the box. Bumping the epoch makes an + in-flight detection discard itself instead of publishing a verdict for the old server under the new + name, and re-entering the current state lets the stamp guard demote a now-stale verdict to + "re-check me". Without this the box could be retyped mid-detect and the guard would never notice. + */ + private void ServerNameTextBox_TextChanged(object sender, TextChangedEventArgs e) + { + if (InstallInProgress) return; + + /* + Any detection in flight was probing the PREVIOUS name -- discard it. It painted "Checking + database status..." before it awaited, and its Superseded() early-outs return without clearing + that, so the label would sit there indefinitely announcing a check nobody is running any more. + */ + _detectEpoch++; + StatusText.Text = string.Empty; + StatusText.Visibility = Visibility.Collapsed; + + /* + The completion states are not verdict states, so the guard below never sees them -- but they + assert plenty about a specific server: "Installation completed successfully!", the install log, + and (after a repair) a live "Upgrade Now" with a version claim. All of it describes the server + the run went to. Withdraw it when the box no longer names that server, and RESTORE it when the + box names it again: typing one character and deleting it must not permanently destroy the only + button that applies a pending upgrade, nor permanently hide the result of an install that did + happen. + */ + if (_currentState is DialogState.InstallComplete or DialogState.MonitoringCredentials) + { + RenderPostRunClaims(); + return; + } + + if (_currentState == DialogState.Connected_StatusUnknown) + { + /* + Connected_StatusUnknown is where a demotion LANDS, so the guard in TransitionToState -- which + only demotes -- can never cover it. It is also where a real block sits, and a real block is a + fact about a specific server: "SQL Server 2014 is not supported" is not true of the instance + now in the box, and we have not looked at it. + */ + if (VerdictMatchesServerBox()) + { + /* + Undo a stale-name demotion when the name comes back. The verdict was never WRONG, only + mis-addressed. _demotedFromState is set ONLY by that demotion, so a real block -- an + unsupported version, an unreadable version, a database newer than the binary -- cannot be + typed away here. + */ + if (_demotedFromState != null) + { + DialogState restored = _demotedFromState.Value; + _installBlockedReason = null; + _serverBlockReason = null; + TransitionToState(restored); + return; + } + + /* + A real block, about the server now back in the box. Put its ACTUAL reason back. A retype + and a retype-back had swapped it for the generic notice below and never swapped it + return -- so "SQL Server 2014 is not supported" became "the server name changed", which by + then was not even true, and only Test Connection could get the real answer back. + */ + if (_serverBlockReason != null && _installBlockedReason != _serverBlockReason) + { + _installBlockedReason = _serverBlockReason; + DatabaseStatusText.Text = _serverBlockReason; + } + + return; + } + + /* + The box names someone else. A demotion's message already says exactly that, and its + _demotedFromState must survive so typing the name back can still undo it -- so only a REAL + block needs restating, and it must lose the previous server's specifics. + + _verdictServerName != null is what makes this correct rather than merely plausible. A clean + install deliberately forgets the stamp (it is about to drop the database), so + VerdictMatchesServerBox() is false for the rest of that dialog's life -- and without this + test, the FIRST keystroke in the box, including one that changes nothing, replaced "a clean + install was requested, which drops the database" with "the server name changed after this + server's status was checked". That is the one sentence telling the user their database is + gone, and it was being overwritten with a sentence that is not even true. No stamp means + nothing to compare the box against, so there is nothing to restate. + */ + if (_demotedFromState == null && + _installBlockedReason != null && + _verdictServerName != null) + { + _installBlockedReason = StaleServerNotice; + DatabaseStatusText.Text = StaleServerNotice; + } + + return; + } + + if (ServerSetupPolicy.IsServerVerdictState(_currentState) && !VerdictMatchesServerBox()) + { + /* Re-entering the state lets the stamp guard demote it. */ + TransitionToState(_currentState); + } + } + + /* + Everything a completed run leaves on screen describes the server the run actually went to: the + install log and its "Installation completed successfully!" headline, the View Report link, and after + a repair the "Upgrade Now" handoff with its version claim. Retyping the box left all of it standing + under the new name -- so a user could retarget after an install and click Save believing + PerformanceMonitor had been installed on the server now in the box. + + Only the repair handoff was withdrawn before, because that was the path the previous round happened + to be looking at; the ordinary install, upgrade and reinstall are the COMMON case and had no + withdrawal at all. Withdraw all of it together, restore all of it together, and say once why it went + away -- idempotent, so it is safe to call on every keystroke. + */ + private void RenderPostRunClaims() + { + bool addressed = VerdictMatchesServerBox(); + + InstallationPanel.Visibility = addressed ? Visibility.Visible : Visibility.Collapsed; + + if (_reportPath != null) + { + ViewReportButton.Visibility = addressed ? Visibility.Visible : Visibility.Collapsed; + } + + RenderRepairHandoff(addressed); + + if (addressed) + { + StatusText.Text = string.Empty; + StatusText.Visibility = Visibility.Collapsed; + return; + } + + /* The panels are gone; without this the dialog would simply look empty for no stated reason. */ + StatusText.Text = StaleServerNotice; + StatusText.Visibility = Visibility.Visible; + } + + private bool VerdictMatchesServerBox() => + _verdictServerName != null && + string.Equals(_verdictServerName, ServerNameTextBox.Text.Trim(), StringComparison.OrdinalIgnoreCase); + + /* + Return to the state the install was launched from -- NOT Initial. + + Initial does not reset InstallUpgradeButton.Content or DatabaseStatusText, and the exit paths + used to force-show InstallationPanel on top of it. Launched from Connected_Current that produced + a genuinely dangerous screen: the button still read "Repair", the text still read "is up to + date", and Advanced Options -- holding "Perform clean install (drops existing database)" -- was + now visible behind it. Ticking that and clicking [Repair] dropped the database. Connected_Current + keeps its options panel collapsed precisely so that cannot happen, so restoring it honestly is + the fix; there, the failure is surfaced in the status panel that IS visible. + */ + private void RestoreAfterInstall(string message) + { + /* + REDUNDANT, and deliberately kept. TransitionToState clears these on every non-Installing + transition, and every path out of here goes through it -- that choke point is the guarantee, + not this. An earlier comment here claimed this was "the only exit path from a run", which was + wrong twice over (BlockInstall and the stale-name demotion are exits too, and the demotion + never reaches either) and would have sent the next reader hunting the wrong mechanism on the + code path that runs DROP DATABASE. Belt and braces on that path is worth three lines. + */ + RepairCheckBox.IsChecked = false; + CleanInstallCheckBox.IsChecked = false; + ResetScheduleCheckBox.IsChecked = false; + + /* + We have no verdict to restore, for one of two reasons -- and they need DIFFERENT words. Either + the run ended before this server's version was read (a cancel or a connect failure during the + install-time re-read), or a clean install deliberately discarded it because it was about to drop + the database. Telling a user whose database we just dropped that "the installed version was not + read" is simply false, and it is the wrong thing to be reading at that moment. + + Either way, do not restore a Connected_* state: that renders the previous server's verdict under + this one's name. Say we do not know. + */ + if (_preInstallState == null) + { + string reason = + _verdictDiscardedByCleanInstall + ? "A clean install was requested, which drops the database, so this server's status " + + "is unknown until it is re-checked." + : "This server's installed version was not read, so its status is unknown."; + + BlockInstall($"{message}\n\n{reason} Click Test Connection to check it."); + + /* + And SHOW THE LOG. BlockInstall lands in Connected_StatusUnknown, which collapses + InstallationPanel -- so this branch used to return having hidden InstallLogTextBox, which is + the only place the actual SQL error text lives. On the clean-install path that is the worst + possible moment to hide it: the database has already been dropped (CleanInstallAsync runs + before the file loop, so any cancel or failure after that point arrives here with the + database gone), and the user was left with a single exception message to work out why. The + branch below has said "show the log either way" since it was written; this one just didn't. + + Advanced Options stays disabled: the install button is collapsed in this state anyway, so + the options are not actionable, and the ticks were cleared on the way in. + */ + InstallationPanel.Visibility = Visibility.Visible; + InstallStatusText.Text = message; + AdvancedOptionsExpander.IsEnabled = false; + return; + } + + TransitionToState(_preInstallState.Value); + + DatabaseStatusPanel.Visibility = Visibility.Visible; + InstallationPanel.Visibility = Visibility.Visible; + InstallStatusText.Text = message; + + /* + Show the log either way -- it is the only place the actual SQL error text lives, and hiding it + leaves the user one exception message to work with. But when the run was launched from + Connected_Current the button still reads "Reinstall Objects" and the clean-install checkbox + must stay out of reach, so re-show the panel with Advanced Options DISABLED rather than + collapsed: WPF propagates IsEnabled to children, so the checkbox cannot be ticked. + */ + AdvancedOptionsExpander.IsEnabled = _preInstallState.Value != DialogState.Connected_Current; + } + private async System.Threading.Tasks.Task DetectDatabaseStatusAsync() { + if (InstallInProgress) return; + + /* + One probe at a time. A detection left Test Connection and Save armed, so a connection test could + start on top of a running detection -- and that test captured the DETECTION's "Checking database + status..." label as the thing to restore when it finished, then faithfully put it back over a + screen where no detection was running. The epoch does not catch it: a detection bumps the epoch + when it STARTS, not when it finishes, so the borrow still looked current. If the test then failed + it showed a MessageBox and never repainted, and the label stayed there for good. + + The finally re-arms only if an install did not start meanwhile -- SetFormEnabled(false) disabled + them deliberately in that case, and every non-Installing state re-arms them on the way out. + */ + DialogState stateBeforeProbe = _currentState; + bool saveWasEnabled = SaveButton.IsEnabled; + + TestConnectionButton.IsEnabled = false; + SaveButton.IsEnabled = false; + CheckForUpdatesButton.IsEnabled = false; + + try + { + await DetectDatabaseStatusCoreAsync(); + } + finally + { + if (!InstallInProgress) + { + /* Nothing owns these two: they are always available whenever the form is live. */ + TestConnectionButton.IsEnabled = true; + CheckForUpdatesButton.IsEnabled = true; + + /* + Save is OWNED BY THE STATE, and this finally must not overrule it. Connected_NoDatabase + turns Save OFF on purpose -- the user has to click Install Now, or take the explicit + "Skip, just add server" link, which is the CONSENTED way to add a server with no + PerformanceMonitor database (SkipInstall_Click re-enables Save and relabels it). Blindly + re-arming it here handed that consent away on the most ordinary path in the dialog: 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. + + If the probe published a verdict, the state it landed in has already set Save correctly. + Only put back what we borrowed when the probe changed nothing -- superseded, or one that + deliberately published nothing. + */ + if (_currentState == stateBeforeProbe) + { + SaveButton.IsEnabled = saveWasEnabled; + } + } + } + } + + private async System.Threading.Tasks.Task DetectDatabaseStatusCoreAsync() + { + int epoch = ++_detectEpoch; + + /* + _dialogClosed belongs in here, not at the call sites. "The dialog is gone" is just another reason + this detection's answer is no longer wanted, and Superseded() is already the one question every + publish point in this method asks before committing anything -- so putting it here covers all of + them at once, including the second DB round-trip, instead of a guard per caller that the next + caller forgets. + */ + bool Superseded() => epoch != _detectEpoch || InstallInProgress || _dialogClosed; + try { StatusText.Text = "Checking database status..."; StatusText.Visibility = Visibility.Visible; + /* + A fresh detection means a fresh server/state, so the destructive and mode-changing + options start clear. Without this they persist across a retarget of the server box. + */ + RepairCheckBox.IsChecked = false; + CleanInstallCheckBox.IsChecked = false; + /* Sticky and INVISIBLE behind a collapsed panel otherwise -- it TRUNCATEs config.collection_schedule, + so a tick left over from another server would wipe this one's tuned intervals with no consent. */ + ResetScheduleCheckBox.IsChecked = false; + + /* + Capture the server name HERE, alongside the connection string built from it -- not after + the awaits below. The box stays editable throughout, so re-reading it later would stamp + the verdict with whatever is in the box NOW rather than the server we actually read, and + the guard would then be comparing the box to itself: it could never fail. + */ + string probedServerName = ServerNameTextBox.Text; string installerConnStr = BuildInstallerConnectionString(); string appVersion = GetAppVersion(); - /* Test connection via Installer.Core to get ServerInfo */ - _coreServerInfo = await InstallationService.TestConnectionAsync(installerConnStr); + /* + Land in a LOCAL, guard, and only then commit to the shared field. An install may have + started while this was in flight (against a different server -- the box stays editable), + and it reads _coreServerInfo. Guarding after the field write would already have clobbered + it with the previous server's ServerInfo. + */ + var probedServerInfo = await InstallationService.TestConnectionAsync(installerConnStr); - if (_coreServerInfo == null || !_coreServerInfo.IsConnected) + /* An install started, or a newer detection superseded us: publish nothing. */ + if (Superseded()) return; + + if (probedServerInfo == null || !probedServerInfo.IsConnected) { - StatusText.Text = string.Empty; - StatusText.Visibility = Visibility.Collapsed; + /* + Publish NOTHING -- deliberately. We established no facts, so the previous server's four + facts and its stamp stay together and stale, which the guard catches. Stamping this + server's NAME here without the facts to go with it is the half-replaced state the guard + structurally cannot catch, and it is what made it affirm a lie once already. + + But SAY something. This used to blank the label and return, which turned the one + instruction on screen after a demotion -- "Click Test Connection to check the server now + in the box" -- into a dead end: the installer-side probe can fail where the plain + connection test just succeeded (different connection string, different database), and + the user clicked, and nothing happened. No message, no state change. Forever. + + A STATUS LINE, not a BlockInstall. A block is a fact about a specific server, and the + guard attributes it by the stamp -- which we just refused to move, correctly. Blocking + here would leave the block attributed to the PREVIOUS server, so the next keystroke would + restate it as "the server name changed", which is not what happened. And the only way to + attribute it would be to stamp without the facts, which is the half-replaced state that + made the guard affirm a lie in the first place. So: no new claim about any server, just + a plain report of what failed. The install stays blocked by whatever already blocked it. + */ + string detail = + string.IsNullOrWhiteSpace(probedServerInfo?.ErrorMessage) + ? "." + : $": {probedServerInfo.ErrorMessage}"; + + StatusText.Text = $"Could not read this server's PerformanceMonitor status{detail}"; + StatusText.Visibility = Visibility.Visible; return; } - if (!_coreServerInfo.IsSupportedVersion) + string probedServerVersion = probedServerInfo.SqlServerVersion.Split('\n')[0].Trim(); + + if (!probedServerInfo.IsSupportedVersion) { - string serverName = ServerNameTextBox.Text; - ConnectionInfoText.Text = _serverVersion != null - ? $"Connected to {serverName} ({_serverVersion})" - : $"Connected to {serverName}"; - DatabaseStatusText.Text = $"Warning: {_coreServerInfo.ProductMajorVersionName} is not supported. SQL Server 2016+ is required."; - DatabaseStatusPanel.Visibility = Visibility.Visible; - InstallUpgradeButton.Visibility = Visibility.Collapsed; - SkipInstallText.Visibility = Visibility.Collapsed; + /* + A publish: "SQL Server 2014 is not supported" is a fact ABOUT this server, so it is + stamped with it -- otherwise retyping past an unsupported 2014 instance to a 2022 one + left the panel still saying 2014, with no stamp for the guard to catch it on. Safe to + commit here: no await stands between the guard above and this line, so the publish + cannot be abandoned after the fields are written. + + Routed through BlockInstall rather than hand-poked. Writing the panels directly here + bypassed the stale-verdict guard entirely and left _currentState pointing at whatever + the previous server's state was -- so an unsupported server could be described using + the previous one's facts, under this one's name. + */ + /* The verdict that replaces the previous run's log arrives WITH it, not before it. */ + DiscardPreviousRunLog(); + + PublishProbedServer( + probedServerInfo, + probedServerVersion, + probedServerName, + /* Never read on this path -- we bail before the version probe. */ + installedVersion: null); + StatusText.Text = string.Empty; StatusText.Visibility = Visibility.Collapsed; + BlockInstall( + $"{probedServerInfo.ProductMajorVersionName} is not supported.\n\n" + + "PerformanceMonitor requires SQL Server 2016 or later."); return; } - /* Check installed version */ - _installedVersion = await InstallationService.GetInstalledVersionAsync(installerConnStr); - - if (_installedVersion == null) + /* + Check installed version. throwOnError matters: with the soft overload a transient + SqlException -- a timeout, the database OFFLINE/RESTORING, a permissions blip -- comes + back as null, which is indistinguishable from "no database". That drops us into the + fresh-install path, which skips every migration, reinstalls over the existing + database, and then stamps installation_history SUCCESS at the target version -- + stranding every pending hop permanently. The CLI passes throwOnError: true for + exactly this reason; the Dashboard's install path must too. + */ + try { - TransitionToState(DialogState.Connected_NoDatabase); + /* Local first, guard, then commit -- a running install reads _installedVersion. */ + var probedVersion = await InstallationService.GetInstalledVersionAsync( + installerConnStr, + throwOnError: true); + + /* An install started, or a newer detection superseded us: publish nothing. */ + if (Superseded()) return; + + /* + Cleared HERE, not before the await. Clearing it in the prologue makes the clear + unconditional while the publish it belongs to stays guarded: an install that blocked + itself mid-flight (a cancel, a fatal) would be retroactively un-blocked by a detection + that then declines to publish anything -- leaving a live button whose every click is a + "Blocked" dialog. State that describes a verdict is committed with the verdict. + */ + _installBlockedReason = null; + _serverBlockReason = null; + + /* The verdict that replaces the previous run's log arrives WITH it, not before it. */ + DiscardPreviousRunLog(); + + PublishProbedServer( + probedServerInfo, + probedServerVersion, + probedServerName, + probedVersion); } - else + catch (Exception ex) { - string normalizedInstalled = NormalizeVersion(_installedVersion); - string normalizedApp = NormalizeVersion(appVersion); + if (Superseded()) return; + + /* + A publish: the block names THIS server's failure, so it is stamped with it -- and the + installed version is null, because that is exactly what just failed to be read. Carrying + the previous server's version through here is how a block ends up describing the wrong + database. + */ + /* The verdict that replaces the previous run's log arrives WITH it, not before it. */ + DiscardPreviousRunLog(); + + PublishProbedServer( + probedServerInfo, + probedServerVersion, + probedServerName, + installedVersion: null); + + BlockInstall( + $"Could not determine the installed PerformanceMonitor version: {ex.Message}\n\n" + + "Install and upgrade are blocked until this resolves. Proceeding could reinstall over an " + + "existing database, skip its pending migrations, and record it as up to date."); + return; + } - if (Version.TryParse(normalizedInstalled, out var installedVer) && - Version.TryParse(normalizedApp, out var appVer)) - { - if (installedVer < appVer) - { - TransitionToState(DialogState.Connected_NeedsUpgrade); - } - else - { - TransitionToState(DialogState.Connected_Current); - } - } - else - { - TransitionToState(DialogState.Connected_Current); - } + string? blockReason = GetInstallBlockReason(_installedVersion, appVersion); + if (blockReason != null) + { + BlockInstall(blockReason); + return; } + + TransitionToState(ServerSetupPolicy.DeriveConnectedState(_installedVersion, appVersion)); } catch (Exception ex) { + /* + The one continuation in this method that was not guarded. A stale detection whose connection + test throws would paint its error over a running install's screen, or over a newer + detection's result -- reporting a failure against a server nobody is looking at any more. + */ + if (Superseded()) return; + StatusText.Text = $"Could not check database status: {ex.Message}"; StatusText.Visibility = Visibility.Visible; } } + /* Record why installing is unsafe and drop into the state that hides the install button. */ + private void BlockInstall(string reason) + { + _installBlockedReason = reason; + + /* Kept so a retarget can swap in the generic notice and a retarget BACK can restore the truth. */ + _serverBlockReason = reason; + + /* + The destructive/mode ticks are cleared by TransitionToState, for every state but Installing -- + deliberately NOT here. Clearing them at each exit is what kept failing: the stale-name demotion + rewrites the state in place and never reaches this method, so a clear written here would have + missed it exactly as the three before it did. + */ + TransitionToState(DialogState.Connected_StatusUnknown); + } + + /* + Decide whether installing against this installed version is safe, and if not, why. Returns null + when it is safe (including "no database", which is a fresh install). + + Both the Test Connection path and the install click run this. It cannot be computed once and + cached: the server box stays editable and BuildInstallerConnectionString() rebuilds the + connection string live, so a verdict from the last detection may belong to a different server. + Refreshing the version alone is not enough either -- the "installed is NEWER than this build" + case produces zero hops and zero failures, so nothing downstream catches it, and the install + would silently revert a newer database to this binary's older object definitions and record + the lower version as SUCCESS. + */ + private static string? GetInstallBlockReason(string? installedVersion, string appVersion) + { + /* + The DECISION lives in Installer.Core/InstallGuard, shared with the CLI and pinned by tests + that actually run in CI. Only the wording is ours. Two of these cases are invisible to every + other guard -- they produce zero upgrade hops AND zero failures, so the migration-failure + abort never fires and nothing downstream notices. + */ + switch (InstallGuard.Check(installedVersion, appVersion)) + { + case InstallBlock.UnreadableBuildVersion: + return $"This Dashboard reports its own version as '{appVersion}', which is not a valid version.\n\n" + + "Install and upgrade are blocked: without a comparable version we cannot tell which " + + "migrations still need to run, and a fresh install would record that value as the " + + "server's version. This is a build problem, not a problem with the server."; + + case InstallBlock.UnreadableInstalledVersion: + return $"Could not interpret the installed PerformanceMonitor version ('{installedVersion}').\n\n" + + "Install and upgrade are blocked: without a comparable version we cannot tell which " + + "migrations still need to run. Correct the most recent SUCCESS row in " + + "PerformanceMonitor.config.installation_history, or rebuild the database with the " + + "CLI installer's --reinstall (destructive)."; + + case InstallBlock.InstalledIsNewerThanBuild: + /* + Everything is blocked here, clean install included -- Connected_StatusUnknown collapses + the panel the checkbox lives in. So this message has to carry the whole escape route, or + the user is simply stuck. + */ + return $"PerformanceMonitor v{NormalizeVersion(installedVersion!)} is installed, which is newer than this " + + $"Dashboard (v{NormalizeVersion(appVersion)}).\n\n" + + "Install, upgrade and repair are blocked: running the older installer would revert this " + + "server's objects to the older definitions and record it at the lower version.\n\n" + + "Update this Dashboard to v" + NormalizeVersion(installedVersion!) + " or later. If you " + + "genuinely need to move this server BACK to an older version, the CLI installer's " + + "--reinstall does that (it drops the database first, so there is nothing left to downgrade)."; + + case InstallBlock.None: + return null; + + default: + /* + Never default to "safe to install". A new InstallBlock member silently becoming an + allow is the one failure mode this whole guard exists to prevent. + */ + throw new NotSupportedException( + $"Unhandled {nameof(InstallBlock)}: {InstallGuard.Check(installedVersion, appVersion)}"); + } + } + private void TransitionToState(DialogState newState) { + /* + Refuse to render a verdict that belongs to a different server. The states below assert facts + about "this server" -- its installed version, whether it needs an upgrade, whether it is up to + date -- and every one of those facts came from a read of some server. If the box has since + been retyped, rendering them puts the OLD server's verdict under the NEW server's name: + "PerformanceMonitor v3.1.0 is up to date" over a database four migrations behind, with Save + enabled. That exact lie has been reached three different ways across nine review rounds, so it + is stopped here, once, rather than at each consumer. + */ + if (ServerSetupPolicy.IsServerVerdictState(newState) && !VerdictMatchesServerBox()) + { + /* + Remembered so the demotion can be UNDONE. The verdict was never wrong -- it was + mis-addressed -- so if the box comes back to the name it was read from, it is valid again. + Null on every other transition, which is what keeps a REAL block (unsupported version, + unreadable version, newer-than-binary) from being undone by typing. + */ + _demotedFromState = newState; + + _installBlockedReason = StaleServerNotice; + newState = DialogState.Connected_StatusUnknown; + } + else + { + _demotedFromState = null; + } + + /* + Consent for a destructive option is per-run and per-SERVER. These three ticks authorize + DROP DATABASE (clean install), skipping the migrations (repair), and TRUNCATE + config.collection_schedule (reset schedule) -- against the server that was on screen when they + were ticked. ANY transition other than starting the run invalidates that: the run ended, or the + verdict was demoted because the box now names someone else. + + Cleared HERE, at the one choke point every state change passes through, because clearing it at + each exit is precisely what kept failing. It was patched into RestoreAfterInstall, then the + detection prologue, then BlockInstall -- and the stale-name demotion, which rewrites newState in + place a dozen lines above and never calls BlockInstall at all, still slipped past every one of + them. That path is unreachable today only because the state it lands in happens to collapse the + panel holding these checkboxes: a rendering accident standing in for a consent check, on the + code path that drops a database. + + Installing is the sole exception -- the run is about to READ these ticks. That "sole exception" + is StateConsumesModeSelections, in ServerSetupPolicy, pinned by a test over every state so a new + state cannot silently join the exception and carry a live tick onto a server. + */ + if (!ServerSetupPolicy.StateConsumesModeSelections(newState)) + { + CleanInstallCheckBox.IsChecked = false; + RepairCheckBox.IsChecked = false; + ResetScheduleCheckBox.IsChecked = false; + } + _currentState = newState; string appVersion = GetAppVersion(); @@ -580,17 +1427,38 @@ private void TransitionToState(DialogState newState) InstallationPanel.Visibility = Visibility.Collapsed; MonitoringCredsPanel.Visibility = Visibility.Collapsed; ViewReportButton.Visibility = Visibility.Collapsed; + /* + Unfreeze Advanced Options. The install states below re-disable it immediately, but without + this it was only ever re-enabled on the abort/cancel/fatal paths -- so after a completed + install it stayed disabled for the rest of the dialog's life, stranding whatever mode + checkboxes were ticked inside it with no way to untick them. + */ + AdvancedOptionsExpander.IsEnabled = true; + /* Only the Installing state arms Cancel; otherwise it paints "Cancelling..." over nothing. */ + CancelInstallButton.IsEnabled = false; StatusText.Text = string.Empty; StatusText.Visibility = Visibility.Collapsed; ConnectionInfoText.Text = string.Empty; InstallUpgradeButton.Visibility = Visibility.Visible; SkipInstallText.Visibility = Visibility.Visible; - /* Build the connection header shown for all connected states */ - string serverName = ServerNameTextBox.Text; - string connectionHeader = _serverVersion != null - ? $"Connected to {serverName} ({_serverVersion})" - : $"Connected to {serverName}"; + /* + Built from the STAMPED server, never the live box, and only when the stamp still matches it. + Reading the box here rendered "Connected to S (Microsoft SQL Server 2019 ...)" mid-keystroke: + a connection that was never made, to a half-typed name, carrying the PREVIOUS server's engine + version. The three verdict states below reach this line only with a matching stamp (the guard + demotes them otherwise), but Connected_StatusUnknown is not a verdict state -- it is where a + demotion LANDS -- so it was never covered. Asserting "Connected to" a server we did not + connect to is the same lie the guard exists to stop; say nothing instead. + */ + string connectionHeader = string.Empty; + + if (VerdictMatchesServerBox()) + { + connectionHeader = _serverVersion != null + ? $"Connected to {_verdictServerName} ({_serverVersion})" + : $"Connected to {_verdictServerName}"; + } switch (newState) { @@ -601,27 +1469,65 @@ private void TransitionToState(DialogState newState) InstallUpgradeButton.Content = "Install Now"; DatabaseStatusPanel.Visibility = Visibility.Visible; InstallationPanel.Visibility = Visibility.Visible; + /* Reachable from Installing (the "nothing to repair" refusal), which disabled the form. */ + SetFormEnabled(true); SaveButton.IsEnabled = false; break; case DialogState.Connected_NeedsUpgrade: - string normalizedInstalled = NormalizeVersion(_installedVersion!); + string normalizedInstalled = NormalizeVersion(_installedVersion ?? "unknown"); ConnectionInfoText.Text = connectionHeader; - DatabaseStatusText.Text = $"PerformanceMonitor v{normalizedInstalled} is installed. " + - $"v{appVersion} is available — click Upgrade Now to apply the update."; + /* Never state the sentinel as a fact: it means "installed, version unreadable". */ + DatabaseStatusText.Text = RepairOutcome.IsVersionUnknown(_installedVersion) + ? "PerformanceMonitor is installed, but its recorded version could not be read, so it is " + + $"unknown which migrations have run. Click Upgrade Now to attempt every upgrade and bring it to v{appVersion}." + : $"PerformanceMonitor v{normalizedInstalled} is installed. " + + $"v{appVersion} is available — click Upgrade Now to apply the update."; InstallUpgradeButton.Content = "Upgrade Now"; DatabaseStatusPanel.Visibility = Visibility.Visible; InstallationPanel.Visibility = Visibility.Visible; + /* Reachable from Installing (every upgrade abort restores here), which disabled the form. */ + SetFormEnabled(true); SaveButton.IsEnabled = true; break; case DialogState.Connected_Current: - string normalizedCurrent = NormalizeVersion(_installedVersion!); + string normalizedCurrent = NormalizeVersion(_installedVersion ?? "unknown"); ConnectionInfoText.Text = connectionHeader; - DatabaseStatusText.Text = $"PerformanceMonitor v{normalizedCurrent} is up to date."; + DatabaseStatusText.Text = $"PerformanceMonitor v{normalizedCurrent} is up to date. " + + "If objects are missing or damaged, click Reinstall Objects to restore them."; + /* + Repair stays reachable at the current version: the install scripts are idempotent, + so re-running them restores missing objects. This state now means installed == + target exactly, so there are no migrations to skip and this is a plain reinstall at + the same version. The CLI can already do this with --repair; without it the + Dashboard had no non-destructive recovery for a healthy-version-but-damaged database. + + InstallationPanel stays collapsed on purpose: it holds the "drops existing database" + clean-install checkbox, which must never sit behind this button. + + Named "Reinstall Objects", NOT "Repair": the Repair CHECKBOX means something different + (skip the migrations, write no history row), and it is deliberately unreachable here. + This button runs the ordinary install path -- with installed == target there are no + migrations to skip, so it is a same-version REINSTALL and correctly records one. + */ + InstallUpgradeButton.Content = "Reinstall Objects"; + SkipInstallText.Visibility = Visibility.Collapsed; + DatabaseStatusPanel.Visibility = Visibility.Visible; + /* Reachable from Installing (a failed reinstall restores here), which disabled the form. */ + SetFormEnabled(true); + SaveButton.IsEnabled = true; + break; + + case DialogState.Connected_StatusUnknown: + ConnectionInfoText.Text = connectionHeader; + DatabaseStatusText.Text = _installBlockedReason ?? "The database status could not be determined."; InstallUpgradeButton.Visibility = Visibility.Collapsed; SkipInstallText.Visibility = Visibility.Collapsed; DatabaseStatusPanel.Visibility = Visibility.Visible; + /* Reachable from Installing (the install-time re-check blocks), which disabled the form. */ + CancelInstallButton.IsEnabled = false; + SetFormEnabled(true); SaveButton.IsEnabled = true; break; @@ -635,6 +1541,17 @@ private void TransitionToState(DialogState newState) case DialogState.InstallComplete: InstallationPanel.Visibility = Visibility.Visible; + /* + Clear both mode checkboxes once a run completes, on EVERY outcome. They are sticky + otherwise: the form is re-enabled here, so the user can retype the server box and + click again -- carrying "Repair" (skip every migration) or, worse, "Clean install" + (drop the database) onto a server that was never consented to. + */ + RepairCheckBox.IsChecked = false; + CleanInstallCheckBox.IsChecked = false; + /* Sticky and INVISIBLE behind a collapsed panel otherwise -- it TRUNCATEs config.collection_schedule, + so a tick left over from another server would wipe this one's tuned intervals with no consent. */ + ResetScheduleCheckBox.IsChecked = false; AdvancedOptionsExpander.IsEnabled = false; CancelInstallButton.IsEnabled = false; SetFormEnabled(true); @@ -668,6 +1585,18 @@ private void TransitionToState(DialogState newState) case DialogState.Initial: default: + /* + The cancel and fatal-error paths land here, and both re-enable the form and re-show + the panels -- so a ticked "Clean install (drops existing database)" would survive a + cancelled run, and the server box could then be retargeted and clicked, dropping a + database that was never consented to. Clear the mode flags here as well as at + InstallComplete, so every exit from a run clears them. + */ + RepairCheckBox.IsChecked = false; + CleanInstallCheckBox.IsChecked = false; + /* Sticky and INVISIBLE behind a collapsed panel otherwise -- it TRUNCATEs config.collection_schedule, + so a tick left over from another server would wipe this one's tuned intervals with no consent. */ + ResetScheduleCheckBox.IsChecked = false; SetFormEnabled(true); SaveButton.IsEnabled = true; SaveButton.Content = "Save"; @@ -677,20 +1606,268 @@ private void TransitionToState(DialogState newState) private async void InstallOrUpgrade_Click(object sender, RoutedEventArgs e) { + /* + Never install when the detected state said it is unsafe: we cannot tell an absent database + from an existing one we simply could not read, and guessing "absent" reinstalls over it with + the migrations skipped and then records it as up to date. + */ + /* Structural backstop: never start a second install over a running one. */ + if (InstallInProgress) return; + + /* + Permanently supersede any detection still in flight. Superseded() also tests InstallInProgress, + but that is only read when the continuation RESUMES -- so a detection that spans the whole run + wakes up AFTER it, sees the install finished, and publishes its pre-install verdict over the + result: the "Upgrade Now" button returns on a server that was just upgraded, or comes back live + while _installBlockedReason is still set, so every click is a "Blocked" box. The epoch is the + only part of that test which cannot un-fire. + */ + _detectEpoch++; + + /* + Every run invalidates the previous run's handoff. Without this, the handoff text outlived the + repair that produced it: repair, then click the "Upgrade Now" it hands you, and the upgrade + succeeds -- but this 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 that, with a + live Upgrade Now button, on a server that had just been upgraded. The completion block below + re-sets it only when THIS run was a repair that left an upgrade pending. + */ + _handoffStatusText = null; + + if (_installBlockedReason != null) + { + MessageBox.Show( + this,_installBlockedReason, "Install Blocked", MessageBoxButton.OK, MessageBoxImage.Warning); + return; + } + + /* + Freeze the UI and arm cancellation BEFORE the first await. TransitionToState(Installing) + collapses DatabaseStatusPanel, which is the Border InstallUpgradeButton lives in -- and that + collapse is the ONLY thing that makes the button unclickable, since SetFormEnabled never + touches it. Awaiting the version re-read in front of it would leave the button live for the + whole connect timeout (unbounded for interactive Entra auth), and this handler is async void: + a second click starts a second concurrent install that clobbers _installCts and + _installResult, races two clean installs, and leaves Cancel pointing at the wrong run. + Creating the CTS first also means Cancel and ESC work DURING the probe rather than no-oping + on a null and letting the install continue after the dialog has closed. + */ + /* + The promise is fixed here and never re-derived. The restore target starts from the best facts + we currently have -- and is refreshed from the install-time re-read below -- but it is a + Connected_* state from the outset, so a cancel DURING the re-read can never restore into + InstallComplete and unfreeze the clean-install checkbox the repair handoff had frozen. + */ + _launchState = _currentState; + _preInstallState = null; + + /* Per-run, like _handoffStatusText. A stale one would misexplain the NEXT run's failure. */ + _verdictDiscardedByCleanInstall = false; + + /* + Per-run too, and it was the one that got missed. GenerateSummaryReport is inside a try whose + catch only logs -- so if it throws on THIS run (long path, disk full, permissions), _reportPath + still holds the LAST run's file, for what may be a different server, and InstallComplete shows + "View Report" because it only tests for null. RenderPostRunClaims will even bring the link back + on a retype-and-back. + */ + _reportPath = null; + TransitionToState(DialogState.Installing); InstallLogTextBox.Clear(); InstallProgressBar.Value = 0; - InstallStatusText.Text = "Preparing installation..."; + InstallStatusText.Text = "Checking installed version..."; - _installCts = new CancellationTokenSource(); - var cancellationToken = _installCts.Token; + _installCts?.Dispose(); + + /* + Held locally as well as in the field, so the finally can tear down the token IT created and + nothing else. Disposing the field unconditionally meant a run that unwound LATE could dispose + and null a token belonging to a run that had started since -- leaving that one uncancellable by + the Cancel button, by Cancel_Click, and by CancelInstallOnClose, so closing the dialog would no + longer stop an install running against the database. + */ + var installCts = new CancellationTokenSource(); + _installCts = installCts; + var cancellationToken = installCts.Token; try { var provider = ScriptProvider.FromEmbeddedResources(); + /* Captured with the connection string built from it -- the box is frozen here, but stamp + what we read, never what the box happens to say later. */ + string installTimeServerName = ServerNameTextBox.Text; string installerConnStr = BuildInstallerConnectionString(); string appVersion = GetAppVersion(); + + /* + Re-read the installed version against the connection we are about to WRITE to, and + re-run the safety verdict on the result. The cached _installedVersion and + _installBlockedReason come from the last Test Connection, but the server box stays + editable and the connection string is rebuilt live -- so retyping it and clicking + straight through would install into a different server while still reasoning about the + previous one. Refreshing the version alone is not enough: the "installed is NEWER than + this build" verdict produces zero hops and zero failures, so nothing downstream catches + it, and the install would silently downgrade the server and record the lower version. + */ + try + { + /* + Refresh the server facts too, not just the version. The cached ones came from the last + Test Connection, and after a retarget they describe a different box -- so the summary + report would name THIS server with the previous one's SQL version and edition. + + Landed in LOCALS and published as a SET, exactly as the detect path does. This method + used to write _coreServerInfo, then await, then write _installedVersion, then stamp -- + three of the four coupled facts, committed separately across an await, and never the + fourth (_serverVersion kept whatever the last detection left). It is not exploitable, + because the box is frozen for the whole run 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 is the exact substitution this file + has been bitten by over and over. Publish them together and it is true by construction. + */ + var installTimeInfo = await InstallationService.TestConnectionAsync( + installerConnStr, + cancellationToken: cancellationToken); + + /* + And ACT on those facts. The SQL-2016+ gate was detect-time only, so retargeting the box + to an older server and clicking through installed 2016+ syntax onto it -- creating a + junk database and a FAILED history row on a server the gate exists to refuse. Re-reading + the fact and then not using it was the whole hole. + */ + if (installTimeInfo is not { IsConnected: true }) + { + throw new InvalidOperationException("Could not connect to this server."); + } + + if (!installTimeInfo.IsSupportedVersion) + { + throw new InvalidOperationException( + $"{installTimeInfo.ProductMajorVersionName} is not supported. PerformanceMonitor requires SQL Server 2016 or later."); + } + + string? installTimeVersion = await InstallationService.GetInstalledVersionAsync( + installerConnStr, + throwOnError: true, + cancellationToken: cancellationToken); + + /* Stamped with the server we just read -- this is the connection we will write to. */ + PublishProbedServer( + installTimeInfo, + installTimeInfo.SqlServerVersion.Split('\n')[0].Trim(), + installTimeServerName, + installTimeVersion); + } + catch (Exception) when (cancellationToken.IsCancellationRequested) + { + /* + SqlCommand does NOT surface cancellation as an OperationCanceledException: it cancels + the in-flight command and the task faults with a SqlException ("Operation cancelled by + user."). Without this, the user's own Cancel would be reported as an + unknown-database-state block, hiding the install button until they re-tested. + */ + throw new OperationCanceledException(cancellationToken); + } + catch (Exception ex) + { + await Dispatcher.InvokeAsync(() => BlockInstall( + $"Could not determine the installed PerformanceMonitor version on this server: {ex.Message}\n\n" + + "Install and upgrade are blocked: proceeding could reinstall over an existing database " + + "and skip its pending migrations.")); + return; + } + + /* + Refuse to reinstall what is not there. Falling through would silently run a FULL fresh + install and stamp the target version -- a whole new database, Agent jobs and XE sessions on + a mistyped server. Both buttons that promise to act on an EXISTING install are covered: + "Upgrade Now" (Connected_NeedsUpgrade) and "Reinstall Objects" (Connected_Current), plus the + Repair checkbox. The server box stays editable and the re-read above runs against whatever + is in it NOW, so a retarget to a bare instance reaches all three. + */ + bool promisedAnExistingInstall = ServerSetupPolicy.PromisesExistingInstall(_launchState); + + if ((RepairCheckBox.IsChecked == true || promisedAnExistingInstall) && + CleanInstallCheckBox.IsChecked != true && + _installedVersion == null) + { + /* + The button arms matter as much as the checkbox: those states' buttons say "Upgrade Now" + and "Reinstall Objects", the server box stays editable, and the install-time re-read runs + against whatever is in it now. Retype it to a bare instance and the button would + silently perform a FULL fresh install -- new database, jobs, XE sessions -- from a + button that promised to reinstall existing objects. + */ + await Dispatcher.InvokeAsync(() => + { + /* Every other exit clears all three; this one must too. ResetSchedule TRUNCATEs + config.collection_schedule, so a tick surviving a retarget wipes the wrong server. */ + RepairCheckBox.IsChecked = false; + CleanInstallCheckBox.IsChecked = false; + ResetScheduleCheckBox.IsChecked = false; + InstallStatusText.Text = string.Empty; + + /* + The box goes up BEFORE the state is unfrozen, and the order is load-bearing. + + MessageBox.Show pumps a nested message loop. TransitionToState(Connected_NoDatabase) + re-enables the form and puts a live "Install Now" back on screen, and it clears + Installing -- so with the transition first, the user could click Install Now while + this box was still standing, and the InstallInProgress backstop was already false and + would not stop them. That starts a SECOND install, which allocates its own + CancellationTokenSource -- and then this run unwinds into its finally, which used to + dispose and null _installCts unconditionally: the second install's token. It would + then be uncancellable by anything, including CLOSING THE DIALOG, which is the exact + hazard CancelInstallOnClose exists to prevent. + + Modal first, while the form is still frozen. The transition happens after it is + dismissed, when nothing can be clicked underneath. + */ + MessageBox.Show( + this, + "There is no existing PerformanceMonitor installation on this server to reinstall.\n\n" + + "Repair, Upgrade and Reinstall Objects restore or update the objects of an EXISTING " + + "installation; none of them creates one. Click Install Now to perform a fresh install instead.", + "Nothing to Reinstall", + MessageBoxButton.OK, + MessageBoxImage.Information); + + TransitionToState(DialogState.Connected_NoDatabase); + }); + return; + } + + string? blockReason = GetInstallBlockReason(_installedVersion, appVersion); + if (blockReason != null) + { + await Dispatcher.InvokeAsync(() => BlockInstall(blockReason)); + return; + } + + /* + Re-derive where a failure should land us from what we just READ, not from where we came + from. Those differ whenever the server box was retyped: restoring the old state would + render the old server's verdict over the new server's version -- "PerformanceMonitor v2.5.0 + is up to date" on a database four migrations behind, with Save enabled. It also keeps + _preInstallState out of InstallComplete/MonitoringCredentials (reachable via the repair + handoff), which RestoreAfterInstall would otherwise re-enter with Advanced Options unfrozen. + */ + _preInstallState = ServerSetupPolicy.DeriveConnectedState(_installedVersion, appVersion); + + InstallStatusText.Text = "Preparing installation..."; + bool cleanInstall = CleanInstallCheckBox.IsChecked == true; + + /* + Repair only means anything for an existing installation we are not about to drop. + Resolving it to the version we repair FROM (rather than a bare bool) keeps the + history write below from ever recording the old version after a clean install. + */ + string? repairFromVersion = RepairCheckBox.IsChecked == true && !cleanInstall + ? _installedVersion + : null; + bool resetSchedule = ResetScheduleCheckBox.IsChecked == true; bool runValidation = ValidationCheckBox.IsChecked == true; bool installDeps = InstallDepsCheckBox.IsChecked == true; @@ -720,11 +1897,34 @@ private async void InstallOrUpgrade_Click(object sender, RoutedEventArgs e) }); /* Run upgrades if applicable (existing database, not clean install) */ - if (!cleanInstall && _installedVersion != null) - { - AppendInstallLog($"Checking for upgrades from v{NormalizeVersion(_installedVersion)} to v{appVersion}...", "Info"); + int upgradeSuccess = 0; + int upgradeFailure = 0; - var (upgradeSuccess, upgradeFailure, upgradeCount) = await InstallationService.ExecuteAllUpgradesAsync( + if (repairFromVersion != null) + { + /* + Repair reinstalls the schema objects (install scripts are idempotent) without + running migrations, so a hop that failed on a missing or damaged object can be + recovered without dropping the database. The pending upgrade runs afterwards. + */ + AppendInstallLog( + "Repair mode: skipping upgrade scripts. Objects will be reinstalled at their current definitions; the pending upgrade still needs to run afterwards.", + "Warning"); + } + else if (!cleanInstall && _installedVersion != null) + { + /* + The sentinel parses, so interpolating it logged "Checking for upgrades from v0.0.0" -- + stating the "I could not read this server's version" guess as a fact. Every other + consumer in this file guards it with IsVersionUnknown; this line did not. + */ + AppendInstallLog( + RepairOutcome.IsVersionUnknown(_installedVersion) + ? $"This server's recorded version could not be read. Attempting every upgrade up to v{appVersion}..." + : $"Checking for upgrades from v{NormalizeVersion(_installedVersion)} to v{appVersion}...", + "Info"); + + var upgradeResult = await InstallationService.ExecuteAllUpgradesAsync( provider, installerConnStr, _installedVersion, @@ -732,17 +1932,59 @@ private async void InstallOrUpgrade_Click(object sender, RoutedEventArgs e) progress, cancellationToken); - if (upgradeCount > 0) + upgradeSuccess = upgradeResult.totalSuccessCount; + upgradeFailure = upgradeResult.totalFailureCount; + + if (upgradeResult.upgradeCount > 0) { AppendInstallLog($"Upgrades complete: {upgradeSuccess} succeeded, {upgradeFailure} failed", upgradeFailure == 0 ? "Success" : "Warning"); } + /* + Abort when an upgrade script fails, matching the CLI installer. + Reinstalling over a partially-upgraded database compounds the damage, and + recording a successful install at the target version would strand the failed + migration permanently: version detection reads the most recent SUCCESS row, so + the hop would never be offered again. Writing no history row leaves the server + at its current version, and upgrade scripts are idempotent, so re-running after + fixing the error resumes cleanly. + */ if (upgradeFailure > 0) { - AppendInstallLog("Upgrade failures detected. Continuing with full installation to ensure consistency...", "Warning"); + await Dispatcher.InvokeAsync(() => + { + InstallProgressBar.Value = 0; + AppendInstallLog( + $"Installation aborted: {upgradeFailure} upgrade script(s) failed. Upgrade scripts must succeed before installation can proceed.", + "Error"); + AppendInstallLog( + "Fix the errors above and run the upgrade again. The server remains at its current version.", + "Info"); + AppendInstallLog( + "If the failure is a missing or damaged object, tick 'Repair' in Advanced Options to reinstall the schema objects without running migrations, then run the upgrade again.", + "Info"); + RestoreAfterInstall($"Upgrade aborted: {upgradeFailure} upgrade script(s) failed."); + }); + + return; } } + /* + A repair reinstalls objects without running migrations, so it must NOT record the + target version -- that would strand every pending hop, which is exactly the bug the + abort above exists to prevent. + + So a repair writes NO history row at all. It does not change the version, and + installation_history is the version ledger -- writing back a version we merely READ is + how a guess becomes a fact. Concretely: GetInstalledVersionAsync returns the unknown sentinel as a + #538 fallback when the database exists but has no SUCCESS row, meaning "unknown, try + every upgrade". Echoing that back as a SUCCESS row would persist the guess as truth. + Writing nothing leaves the previous row as the version of record, which is exactly + right -- the pending upgrade is still offered afterwards. + */ + bool isRepair = repairFromVersion != null; + /* Run main installation */ AppendInstallLog("Starting main installation...", "Info"); @@ -758,6 +2000,26 @@ private async void InstallOrUpgrade_Click(object sender, RoutedEventArgs e) }; } + if (cleanInstall) + { + /* + A clean install DROPS the database. From here the version we read is about to stop being + true, so a cancel or a failure must not restore a verdict describing it -- that renders + "PerformanceMonitor v3.0.0 is installed" over a server whose database no longer exists, + with Save enabled. Forget the verdict; RestoreAfterInstall will say "status unknown". + */ + _preInstallState = null; + _verdictServerName = null; + + /* + Remember WHY we forgot it. Both causes of a null _preInstallState land in the same + branch of RestoreAfterInstall, which told every one of them "this server's installed + version was not read" -- which on THIS path is simply false. It was read; we discarded + it on purpose, because we are about to drop the database out from under it. + */ + _verdictDiscardedByCleanInstall = true; + } + _installResult = await InstallationService.ExecuteInstallationAsync( installerConnStr, provider, @@ -767,24 +2029,62 @@ private async void InstallOrUpgrade_Click(object sender, RoutedEventArgs e) preValidationAction, cancellationToken); - /* Log installation history */ - try + /* + A clean install that failed is not "completed with N error(s)". CleanInstallAsync drops the + Agent jobs, the XE sessions and then the DATABASE before a single install file runs -- so if + it failed, the database may be gone, half-gone, or stuck in SINGLE_USER, and we do not know + which. ExecuteInstallationAsync returns early in that case with no files attempted. + + Route it exactly where a cancel goes: no history row, no verdict, the log left on screen, and + install blocked until the server is re-checked. The completion path below would instead have + announced a completion, restored the discarded verdict, and armed a Save that skips the + connection test -- on a server whose database we may have just destroyed. + */ + if (cleanInstall && !_installResult.Success && _installResult.FilesSucceeded == 0) { - AppendInstallLog("Recording installation history...", "Info"); - await InstallationService.LogInstallationHistoryAsync( - installerConnStr, - appVersion, - appVersion, - _installResult.StartTime, - _installResult.FilesSucceeded, - _installResult.FilesFailed, - _installResult.Success, - progress); - AppendInstallLog("Installation history recorded", "Success"); + string cleanInstallError = + _installResult.Errors.Count > 0 + ? _installResult.Errors[0].ErrorMessage + : "the database could not be recreated."; + + await Dispatcher.InvokeAsync(() => + RestoreAfterInstall($"Clean install failed: {cleanInstallError}")); + return; } - catch (Exception ex) + + /* Log installation history -- but never for a repair, which changes no version (see above). */ + if (isRepair) + { + AppendInstallLog( + "Repair does not change the recorded version, so no installation history row was written.", + "Info"); + } + else { - AppendInstallLog($"Could not record installation history: {ex.Message}", "Warning"); + try + { + AppendInstallLog("Recording installation history...", "Info"); + + /* + Fold the upgrade script counts in. InstallationResult only covers the install + files, so passing it alone would under-report files_executed and could record a + SUCCESS at the target version even when a migration had failed. + */ + await InstallationService.LogInstallationHistoryAsync( + installerConnStr, + appVersion, + appVersion, + _installResult.StartTime, + upgradeSuccess + _installResult.FilesSucceeded, + upgradeFailure + _installResult.FilesFailed, + upgradeFailure == 0 && _installResult.Success, + progress); + AppendInstallLog("Installation history recorded", "Success"); + } + catch (Exception ex) + { + AppendInstallLog($"Could not record installation history: {ex.Message}", "Warning"); + } } /* Run validation if requested */ @@ -809,8 +2109,16 @@ await InstallationService.LogInstallationHistoryAsync( /* Generate summary report */ try { + /* This parameter is printed as "Installer Version" -- it names the BINARY that ran, not the database. */ + /* + installTimeServerName, not the live box. This was the last read-the-box-after-the-await + in the file: the report names the server the install actually went to, and it is written + after every await in the run. Un-exploitable today only because the box is disabled for + the duration -- which is enablement standing in for an invariant, and the report would + have carried the wrong server's name the moment that stopped being true. + */ _reportPath = InstallationService.GenerateSummaryReport( - ServerNameTextBox.Text.Trim(), + installTimeServerName.Trim(), _coreServerInfo?.SqlServerVersion ?? "", _coreServerInfo?.SqlServerEdition ?? "", appVersion, @@ -822,11 +2130,82 @@ await InstallationService.LogInstallationHistoryAsync( AppendInstallLog($"Could not generate report: {ex.Message}", "Warning"); } + /* + Did a CRITICAL file (01_/02_/03_) fail? ExecuteInstallationAsync aborts the whole pass on + those, so the repair reinstalled almost nothing. That is NOT the expected Msg 207 outcome + below, and must not be dressed up as one. + */ + bool criticalFileFailed = _installResult.Errors.Any(e => Patterns.IsCriticalFile(e.FileName)); + + /* Shared with the CLI so the two cannot drift -- see RepairOutcome. */ + /* + THREE different questions, and they disagree for the unknown sentinel. Answering all of + them with one flag told the operator "already at the current version, so there is no + upgrade to apply" for a server with every hop pending, and withheld the button that would + have applied them. + */ + bool failuresExcused = RepairOutcome.FailuresAreExpected( + isRepair, repairFromVersion, appVersion, criticalFileFailed); + bool hasPendingUpgrade = RepairOutcome.HasPendingUpgrade(repairFromVersion, appVersion); + bool versionUnknown = RepairOutcome.IsVersionUnknown(repairFromVersion); + + /* The repair ran and was not aborted by a critical file, so its handoff is still owed. */ + bool repairCompleted = isRepair && !criticalFileFailed; + /* Update final status */ await Dispatcher.InvokeAsync(() => { InstallProgressBar.Value = 100; - if (_installResult.Success) + if (repairCompleted) + { + /* + A repair on a database with pending migrations is EXPECTED to report file errors, + and that is not a failed repair. The install scripts compile against the CURRENT + schema -- e.g. install/23_process_blocked_process_xml.sql reads + collect.blocking_BlockedProcessReport.monitor_loop, a column the 3.0.0-to-3.1.0 + migration adds -- and ALTER PROCEDURE binds columns at compile time, so those + procedures cannot compile until the upgrade runs (Msg 207, "Invalid column name"). + A failed CREATE OR ALTER leaves the old body intact, so nothing is damaged; the + upgrade's own install pass recompiles them once the schema is current. + + So the completion block is NOT gated on _installResult.Success: gating it would + leave the user staring at "completed with N error(s)" and no next step, which is + how the half-migrated database this feature exists to escape gets left behind. + */ + InstallStatusText.Text = + _installResult.Success ? "Repair completed." + : failuresExcused ? $"Repair completed with {_installResult.FilesFailed} expected error(s)." + : $"Repair completed with {_installResult.FilesFailed} error(s)."; + + if (versionUnknown) + { + AppendInstallLog( + "Repair complete. No version was recorded, and this server's recorded version could not be read -- " + + "it is unknown which migrations have run. Click Upgrade Now to attempt every upgrade, then re-check.", + "Warning"); + } + else if (hasPendingUpgrade) + { + AppendInstallLog( + $"Repair complete. The server is still at v{NormalizeVersion(repairFromVersion!)} and no version was recorded -- click Upgrade Now to apply the pending migrations.", + "Success"); + + if (!_installResult.Success && failuresExcused) + { + AppendInstallLog( + $"{_installResult.FilesFailed} object(s) could not be compiled because the pending upgrade has not run yet. " + + "This is expected -- they reference columns the upgrade adds, and the upgrade will recompile them.", + "Info"); + } + } + else + { + AppendInstallLog( + "Repair complete. This server was already at the current version, so no version was recorded and there is no upgrade to apply.", + "Success"); + } + } + else if (_installResult.Success) { InstallStatusText.Text = "Installation completed successfully!"; AppendInstallLog("Installation completed successfully!", "Success"); @@ -837,38 +2216,100 @@ await Dispatcher.InvokeAsync(() => AppendInstallLog($"Installation completed with {_installResult.FilesFailed} error(s).", "Error"); } + /* + Re-publish the four facts for the server the run actually went to. 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 if the run SUCCEEDED we now know exactly which server this + screen describes: the one we just wrote to. Without this the stamp stays null and + everything keyed on it treats a completed clean install as un-addressed: Save re-runs the + connection test (a second interactive MFA prompt on Entra), and the first keystroke in the + server box withdraws a success message that was perfectly true. + + GATED on success, which it was not. The comment used to assert "but the run SUCCEEDED" + while the code never checked -- so a clean install that FAILED restored the very verdict + it had thrown away for safety, over a database DROP DATABASE may have already taken. And + skipConnectionTest keys on this stamp: Save would then have persisted that server without + ever reconnecting to it. A failed run knows nothing. Leave the stamp null and make Save + prove the connection. + + A PUBLISH, not a lone re-stamp. Moving the stamp by itself put it back while + _installedVersion still held the version read BEFORE the run -- the pre-drop version, on + the clean-install path -- so VerdictMatchesServerBox() went true over a half-replaced fact + set, which is the one shape the guard structurally cannot catch. It is unexploitable today + only because nothing reads _installedVersion from InstallComplete, and reader-absence + standing in for an invariant is exactly the substitution that produced the worst bug in + this file. The database now holds appVersion -- except after a repair, which changes no + version at all. + */ + if (_installResult.Success && _coreServerInfo != null) + { + PublishProbedServer( + _coreServerInfo, + _serverVersion ?? string.Empty, + installTimeServerName, + isRepair ? _installedVersion : appVersion); + } + TransitionToState(DialogState.InstallComplete); + + /* + A successful repair leaves the migrations UNAPPLIED by design, so the user has to run + the upgrade next. InstallComplete collapses DatabaseStatusPanel, which is the Border + InstallUpgradeButton lives in -- so without re-showing it, the completion message + points at a button that is not on screen and the obvious next click is Save & Connect, + leaving exactly the half-migrated database this feature exists to escape. + + MonitoringCredentials is included: InstallComplete chains straight into it for SQL + auth, and that state does not re-show the panel either -- so without it, every SQL-auth + user (the ones most likely to hit this) would get the dead end. The three panels live in + separate grid rows and coexist fine. + + Not gated on _installResult.Success, for the reason above: a repair with the expected + compile errors still needs the handoff. It IS gated on no critical file having failed, + because that repair reinstalled nothing -- and on there actually being an upgrade to + hand off TO, which for the unknown sentinel means yes (every hop may be pending). + */ + if (repairCompleted && hasPendingUpgrade && + (_currentState == DialogState.InstallComplete || + _currentState == DialogState.MonitoringCredentials)) + { + _handoffStatusText = versionUnknown + ? "Objects were reinstalled. This server's recorded version could not be read, so it is " + + "unknown which migrations have run. Click Upgrade Now to attempt every upgrade." + : $"Objects were reinstalled. PerformanceMonitor is still at v{NormalizeVersion(repairFromVersion!)} " + + "-- the pending upgrade has not been applied. Click Upgrade Now to apply it."; + + /* Addressed by construction: the box was frozen for the run and just re-stamped. */ + RenderRepairHandoff(addressed: true); + } }); } catch (OperationCanceledException) { Dispatcher.Invoke(() => { - InstallStatusText.Text = "Installation cancelled."; AppendInstallLog("Installation was cancelled by user.", "Warning"); InstallProgressBar.Value = 0; - TransitionToState(DialogState.Initial); - DatabaseStatusPanel.Visibility = Visibility.Visible; - InstallationPanel.Visibility = Visibility.Visible; - AdvancedOptionsExpander.IsEnabled = true; + RestoreAfterInstall("Installation cancelled."); }); } catch (Exception ex) { Dispatcher.Invoke(() => { - InstallStatusText.Text = $"Installation failed: {ex.Message}"; AppendInstallLog($"Fatal error: {ex.Message}", "Error"); - TransitionToState(DialogState.Initial); - DatabaseStatusPanel.Visibility = Visibility.Visible; - InstallationPanel.Visibility = Visibility.Visible; - AdvancedOptionsExpander.IsEnabled = true; + RestoreAfterInstall($"Installation failed: {ex.Message}"); }); } finally { - _installCts?.Dispose(); - _installCts = null; + /* Only clear the field if it still points at OUR token -- see where it was created. */ + if (ReferenceEquals(_installCts, installCts)) + { + _installCts = null; + } + + installCts.Dispose(); } } @@ -879,6 +2320,14 @@ private void CancelInstall_Click(object sender, RoutedEventArgs e) InstallStatusText.Text = "Cancelling..."; } + /* A clean install drops and recreates the database, so there is nothing left to repair. */ + private void CleanInstallCheckBox_Checked(object sender, RoutedEventArgs e) => + RepairCheckBox.IsChecked = false; + + /* Repair is the non-destructive recovery path, so it cannot mean "drop the database". */ + private void RepairCheckBox_Checked(object sender, RoutedEventArgs e) => + CleanInstallCheckBox.IsChecked = false; + private void AppendInstallLog(string message, string status) { if (string.IsNullOrEmpty(message)) @@ -904,6 +2353,26 @@ private void AppendInstallLog(string message, string status) private void SkipInstall_Click(object sender, MouseButtonEventArgs e) { + /* + Supersede any detection in flight. "Skip, just add server" is live while one is running, and + this method writes _currentState directly rather than transitioning -- so the detection landed + afterwards, published its verdict, and put the install panel and the install button straight + back on screen, silently undoing the skip the user had just chosen. + + Clearing StatusText is part of superseding, not decoration: the detection painted "Checking + database status..." before it awaited, and its Superseded() early-outs return without clearing + it. Every other epoch-bumper does this; this one didn't, so the label sat there for the rest of + the dialog's life announcing a check nobody was running. + */ + _detectEpoch++; + StatusText.Text = string.Empty; + StatusText.Visibility = Visibility.Collapsed; + + /* This bypasses TransitionToState, so it is the one exit that never cleared these. */ + RepairCheckBox.IsChecked = false; + CleanInstallCheckBox.IsChecked = false; + ResetScheduleCheckBox.IsChecked = false; + DatabaseStatusPanel.Visibility = Visibility.Collapsed; InstallationPanel.Visibility = Visibility.Collapsed; SaveButton.IsEnabled = true; @@ -926,6 +2395,7 @@ private void ViewReport_Click(object sender, RoutedEventArgs e) catch (Exception ex) { MessageBox.Show( + this, $"Could not open report: {ex.Message}", "Error", MessageBoxButton.OK, @@ -969,26 +2439,130 @@ private void SetFormEnabled(bool enabled) AlertDeliveryOverrideComboBox.IsEnabled = enabled; DescriptionTextBox.IsEnabled = enabled; TestConnectionButton.IsEnabled = enabled; + /* + Check for Updates re-runs DetectDatabaseStatusAsync, which transitions back to a Connected_* + state -- re-showing the install button and collapsing the running install's progress panel. + Leaving it live during Installing let a second click start a concurrent install against the + same database, clobbering _installResult and leaving Cancel pointing at the wrong run. + */ + CheckForUpdatesButton.IsEnabled = enabled; SaveButton.IsEnabled = enabled; } private async void Save_Click(object sender, RoutedEventArgs e) + { + /* + Save is re-enterable. RunConnectionTestAsync(SaveButton) disables Save for the duration -- but + Test Connection stays live, and ITS finally re-enables Save (it re-enables the form wholesale, + not just the button it took). Click Save, click Test Connection, click Save again: both tests + complete, the first sets DialogResult and closes the window, and the second assigns DialogResult + to a CLOSED window, which throws. + + It does NOT take the app down -- App.OnDispatcherUnhandledException handles it (App.xaml.cs:173) + -- so an earlier version of this comment overstated it. What the user gets is an alarming + "An error occurred" box and a server that was silently not saved. Pre-existing; the epoch guard + below happens to close every variant where a detect or install intervenes, but not this one. + (The close-during-save variant is handled separately, by _dialogClosed.) + */ + if (_saveInProgress) return; + _saveInProgress = true; + + try + { + await SaveAsync(); + } + finally + { + _saveInProgress = false; + } + } + + private bool _saveInProgress; + + private async Task SaveAsync() { if (!ValidateInputs()) return; - /* If we just finished installing, skip re-testing the connection */ - bool skipConnectionTest = _currentState == DialogState.InstallComplete || - _currentState == DialogState.MonitoringCredentials; + /* + "We just installed, so the connection is proven" is only true if the box still names the server + we installed to. Keyed on _currentState alone, retyping the box after an install skipped the + test entirely: the new name was persisted un-contacted, and under SQL auth the INSTALLER's + credentials -- routinely sa -- were saved as the ongoing monitoring account for a server nobody + had ever connected to. The stamp is what makes the claim true, so test the stamp. + */ + bool skipConnectionTest = + (_currentState == DialogState.InstallComplete || + _currentState == DialogState.MonitoringCredentials) && + VerdictMatchesServerBox(); if (!skipConnectionTest) { - var (connected, errorMessage, mfaCancelled, _) = await RunConnectionTestAsync(SaveButton); + /* + An install can start AND FINISH while this connection test is in flight (InstallUpgradeButton + stays hit-testable -- SetFormEnabled does not touch it). InstallInProgress alone is the + half of that test that un-fires: it is only read when this continuation resumes, so a run + that completed in the meantime reads as "no install", and we fall through to DialogResult + and Close() -- destroying the dialog the instant the install lands, before the SQL-auth user + is ever shown the monitoring-credentials panel, and persisting the installer account in its + place. The epoch cannot un-fire; InstallOrUpgrade_Click bumps it on every start. + + It also covers the retype: TextChanged bumps the epoch too, so a box edited while this test + was in flight abandons the save rather than persisting the new name on the strength of a + connection proven against the old one. + */ + int epoch = _detectEpoch; + + var (connected, errorMessage, mfaCancelled) = await RunConnectionTestAsync(SaveButton); + + if (InstallInProgress) return; + + /* + The dialog was CLOSED while this test was running -- ESC, Cancel, the X. Bail here, before + anything is written, because in edit mode ServerConnection is not a copy: it is the very + object ServerManager holds in its list (MainWindow hands us item.Server directly). The + assignments below mutate it IN PLACE, so a save the user cancelled still renames their + monitored server -- and it reaches disk on its own, the next time UpdateLastConnected fires + for ANY server. PROD01 quietly becomes TEST99, keeps PROD01's id and credential, and PROD01 + stops being monitored. + + This guard used to sit 47 lines lower, next to the DialogResult write. That stopped the + THROW and not the WRITE -- and the throw was the only thing the user ever saw, so moving it + down there turned a loud bug into a silent one. Guard before you commit the facts; it is the + same rule the detection path already follows. + + One check covers everything: RunConnectionTestAsync is the only await in this method (the + skipConnectionTest path has none), so this is the only window in which _dialogClosed can + flip. It also kills the orphaned "Do you still want to save this connection?" box below, + which was popping up with no dialog left behind it. + */ + if (_dialogClosed) return; + + if (epoch != _detectEpoch) + { + /* + SAY SO. An earlier version of this comment claimed every epoch bump has a visible cause, + so the abandoned save could not be silent. That was wrong: from Initial -- a fresh dialog, + which is the most common way to reach Save -- a retype renders nothing at all, and this + return then swallowed the click. The user fixes a typo while the 10-second connection test + is running, clicks nothing else, and believes the server was added. + + Worded for all four bumpers, not just the retype. Check for Updates and "Skip, just add + server" bump the epoch too, and blaming "the server details changed" for a change the + user never made is its own small lie. + */ + StatusText.Text = + "Nothing was saved -- something changed while the connection was being tested. " + + "Click Save again."; + StatusText.Visibility = Visibility.Visible; + return; + } if (!connected) { if (mfaCancelled) { MessageBox.Show( + this, "Authentication was cancelled. Click Save to try again, or Cancel to abort.", "Authentication Cancelled", MessageBoxButton.OK, @@ -999,6 +2573,7 @@ private async void Save_Click(object sender, RoutedEventArgs e) var detail = errorMessage != null ? $"\n\nError: {errorMessage}" : string.Empty; var result = MessageBox.Show( + this, $"Could not connect to {ServerNameTextBox.Text}.{detail}\n\n" + "Do you still want to save this connection?", "Connection Failed", @@ -1050,9 +2625,17 @@ private async void Save_Click(object sender, RoutedEventArgs e) { authenticationType = AuthenticationTypes.SqlServer; - /* Use monitoring credentials if provided */ - if (_currentState == DialogState.MonitoringCredentials && - UseSameCredsCheckBox.IsChecked == false && + /* + Use the monitoring credentials if the user actually entered them. + + Deliberately NOT gated on _currentState: the repair handoff puts a live "Upgrade Now" + button in front of MonitoringCredentials, which used to be terminal. Any exit from the + install that button starts (abort, cancel, fatal, version block) transitions away, and + the typed monitoring login would then be silently discarded and the INSTALLER account -- + typically sysadmin -- persisted as the ongoing monitoring credential instead. Keying on + what was entered rather than on a transient state cannot drift that way. + */ + if (UseSameCredsCheckBox.IsChecked == false && !string.IsNullOrWhiteSpace(MonitorUsernameTextBox.Text)) { Username = MonitorUsernameTextBox.Text.Trim(); @@ -1070,6 +2653,32 @@ private async void Save_Click(object sender, RoutedEventArgs e) ? ServerNameTextBox.Text.Trim() : DisplayNameTextBox.Text.Trim(); + /* + THE guard, immediately before the commit, with nothing between. + + There is one further up, straight after the await, and it is not enough -- because "await is the + only place this method yields" is FALSE. MessageBox.Show pumps a nested Win32 message loop, and + the "Do you still want to save this connection?" box above sits between that guard and these + assignments. + + Every box in this file now passes `this` as its owner, so Win32 disables the dialog while one is + up -- which is what closes the concrete route. This guard stays anyway, and not out of caution: + a dispatcher work item or an application shutdown can still close a window during a nested pump, + and an already-closed window has no HWND to own the next box with. The whole reason the previous + fix was wrong is that it reasoned about which re-entrancy windows exist instead of guarding where + the damage happens. Do not repeat that by deleting this. + + And these are not writes to a copy. In edit mode ServerConnection IS the object ServerManager + holds -- MainWindow hands us item.Server -- so they rename the user's live, monitored server in + place, and it reaches disk on its own the next time UpdateLastConnected fires for anyone. + + The previous fix put the guard after the await and reasoned that nothing could re-enter before + the commit. That reasoning leaned on a modal box happening to disable the window, which is the + same "unreachable because something else happens to be disabled" substitution this file has been + bitten by over and over. Do not reason about re-entrancy windows. Guard where the damage is. + */ + if (_dialogClosed) return; + if (_isEditMode) { ServerConnection.DisplayName = displayName; @@ -1113,6 +2722,12 @@ private async void Save_Click(object sender, RoutedEventArgs e) }; } + /* + The user closed the dialog while this save's connection test was still running -- ESC, Cancel, + the X, Alt+F4. The window is gone; assigning DialogResult to it throws. Nothing to save to. + */ + if (_dialogClosed) return; + DialogResult = true; Close(); } diff --git a/Dashboard/Services/ServerManager.cs b/Dashboard/Services/ServerManager.cs index eee419a353..fd57a8b070 100644 --- a/Dashboard/Services/ServerManager.cs +++ b/Dashboard/Services/ServerManager.cs @@ -272,15 +272,35 @@ IF EXISTS (SELECT 1 FROM msdb.dbo.sysjobs WHERE name = N'PerformanceMonitor - Hu jobCmd.CommandTimeout = 30; await jobCmd.ExecuteNonQueryAsync(); - // Close active connections before dropping + // Close active connections before dropping. + // EngineEdition 8 = Azure SQL Managed Instance, where SINGLE_USER is non-modifiable (the batch + // would die on that line); gate it to box SQL Server and let MI DROP directly. Mirrors + // InstallationService.CleanInstallAsync. using var killCmd = new SqlCommand(@" IF DB_ID('PerformanceMonitor') IS NOT NULL BEGIN - ALTER DATABASE [PerformanceMonitor] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; + IF SERVERPROPERTY('EngineEdition') <> 8 + ALTER DATABASE [PerformanceMonitor] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; DROP DATABASE [PerformanceMonitor]; END", connection); killCmd.CommandTimeout = 30; - await killCmd.ExecuteNonQueryAsync(); + try + { + await killCmd.ExecuteNonQueryAsync(); + } + catch + { + // A DROP that fails or times out after SINGLE_USER committed strands the database + // semi-bricked. Un-brick best-effort on a fresh connection, then re-raise the original error. + connection.Close(); + bool restored = await InstallationService.TryRestoreDatabaseAccessAsync(builder.ConnectionString); + if (!restored) + { + Logger.Warning($"Drop failed on '{server.DisplayName}' and the database may be left in " + + "SINGLE_USER mode; re-run the clean install to recover, or ALTER DATABASE ... SET MULTI_USER manually."); + } + throw; + } Logger.Info($"Dropped PerformanceMonitor database and Agent jobs on '{server.DisplayName}'"); } diff --git a/Installer.Core/InstallGuard.cs b/Installer.Core/InstallGuard.cs new file mode 100644 index 0000000000..bec503ec99 --- /dev/null +++ b/Installer.Core/InstallGuard.cs @@ -0,0 +1,74 @@ +namespace Installer.Core; + +/// Why installing against a server would be unsafe. +public enum InstallBlock +{ + /// Safe to install. + None, + + /// + /// This binary's own version will not parse. It is what a FRESH install writes to + /// installation_history.installer_version, so letting it through poisons a brand-new server's + /// ledger at birth — after which every surface refuses to touch that server again. + /// + UnreadableBuildVersion, + + /// + /// The version recorded on the server will not parse, so we cannot tell which migrations still apply. + /// + UnreadableInstalledVersion, + + /// + /// The server is on a NEWER build than this binary. Installing would run our older scripts over it — + /// reverting every CREATE OR ALTER procedure and view to their older definitions — and then + /// record the LOWER version as SUCCESS. A silent downgrade. + /// + InstalledIsNewerThanBuild, +} + +/// +/// The decision behind both installers' pre-install blocks. +/// +/// Two of these cases are invisible to every other guard: they produce ZERO upgrade hops AND ZERO +/// failures, so the migration-failure abort never fires and nothing downstream notices. That is what +/// makes them dangerous, and why the decision lives here — shared and pinned — rather than hand-copied +/// into a WPF code-behind and a CLI Main. +/// +public static class InstallGuard +{ + /// The version recorded on the server; null when nothing is installed. + /// The version of the binary about to run. + public static InstallBlock Check(string? installedVersion, string? buildVersion) + { + var build = ScriptProvider.TryParseVersionCore(buildVersion); + + /* + Checked first, and independently of whether anything is installed: a fresh install WRITES this + value, so an unreadable one is fatal even on an empty server. + */ + if (build == null) + { + return InstallBlock.UnreadableBuildVersion; + } + + if (installedVersion == null) + { + /* Nothing installed: a fresh install is safe. */ + return InstallBlock.None; + } + + var installed = ScriptProvider.TryParseVersionCore(installedVersion); + + if (installed == null) + { + return InstallBlock.UnreadableInstalledVersion; + } + + if (installed > build) + { + return InstallBlock.InstalledIsNewerThanBuild; + } + + return InstallBlock.None; + } +} diff --git a/Installer.Core/InstallationService.cs b/Installer.Core/InstallationService.cs index b7ffd2dcc6..750094e4ec 100644 --- a/Installer.Core/InstallationService.cs +++ b/Installer.Core/InstallationService.cs @@ -20,6 +20,24 @@ namespace Installer.Core; /// public static class InstallationService { + /// + /// Returned by when the database is clearly a + /// PerformanceMonitor install but its recorded version cannot be read — no SUCCESS row, or no + /// history table at all. It is a GUESS meaning "unknown, attempt every upgrade", chosen because it + /// sorts below every real version so FilterUpgrades offers all of them: the safe direction. + /// + /// It is NOT a fact, and callers that DECIDE something from a version must not treat it as one — + /// it compares less than any target, so anything keyed on "is an upgrade pending?" would answer + /// yes unconditionally. See . + /// + /// Deliberately OUTSIDE the real version space. It used to be "1.0.0", which is a shipped tag — and + /// the "your recorded version is unreadable" message tells operators to correct the SUCCESS row by + /// hand, where "1.0.0" is exactly the value someone would type to force a full re-run. They would + /// then be told their version could not be read. No release is 0.0.0, and it still sorts below every + /// real version, so upgrade discovery still offers every hop. + /// + public const string UnknownVersionSentinel = "0.0.0"; + private static readonly char[] NewLineChars = ['\r', '\n']; /// @@ -260,13 +278,40 @@ IF EXISTS (SELECT 1 FROM sys.dm_xe_sessions WHERE name = N'PerformanceMonitor_De IF EXISTS (SELECT 1 FROM sys.databases WHERE name = N'PerformanceMonitor') BEGIN - ALTER DATABASE PerformanceMonitor SET SINGLE_USER WITH ROLLBACK IMMEDIATE; + /* + SINGLE_USER evicts other connections so the DROP can proceed -- but it is non-modifiable on Azure SQL + Managed Instance (EngineEdition 8), where the whole batch would die on this line before the DROP. MI + has no force-disconnect; an installer teardown relies on there being no other sessions, which is + normally true. So gate the eviction to box SQL Server and let MI DROP directly. + */ + IF SERVERPROPERTY('EngineEdition') <> 8 + ALTER DATABASE PerformanceMonitor SET SINGLE_USER WITH ROLLBACK IMMEDIATE; DROP DATABASE PerformanceMonitor; END;"; using var command = new SqlCommand(cleanupSql, connection); command.CommandTimeout = ShortTimeoutSeconds; - await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); + try + { + await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); + } + catch + { + /* + SET SINGLE_USER and DROP are separate autocommitted statements. If the DROP then fails (another + session racing into the single-user slot, Msg 3702) or the client CommandTimeout / a cancel + fires an attention AFTER SINGLE_USER committed, the database is left online but stranded in + SINGLE_USER -- semi-bricked. Un-brick before propagating. It must run on a FRESH connection: a + client-attention timeout is invisible to a server-side TRY/CATCH, so this cannot be a + BEGIN CATCH in the batch above. Close this connection first to release the single-user slot the + failed run may still hold, so the restore is not blocked by it. The bare throw preserves the + original outcome -- a cancel's OperationCanceledException (converted upstream) or the failure's + SqlException. + */ + connection.Close(); + await TryRestoreDatabaseAccessAsync(connectionString, progress).ConfigureAwait(false); + throw; + } progress?.Report(new InstallationProgress { @@ -275,6 +320,55 @@ IF EXISTS (SELECT 1 FROM sys.databases WHERE name = N'PerformanceMonitor') }); } + /// + /// Best-effort recovery for a clean install whose DROP DATABASE failed or timed out after + /// SET SINGLE_USER had already committed, leaving the database stranded in SINGLE_USER. Puts it back to + /// MULTI_USER so it is usable rather than semi-bricked. + /// + /// Swallows its own errors -- it must never mask the failure that triggered it -- and runs uncancellably + /// on a fresh master connection, so it still executes after a client-attention timeout or a cancel + /// (both of which a server-side TRY/CATCH cannot see). No-op on Azure SQL Managed Instance, where + /// SINGLE_USER cannot be set in the first place, and a no-op when the database no longer exists (the + /// DROP actually succeeded) or is already MULTI_USER (SET MULTI_USER over MULTI_USER is harmless). + /// + /// + /// true if the restore ran without error (either it set MULTI_USER, or it was a no-op because the drop + /// actually succeeded / this is Managed Instance) — i.e. the database is not left stranded as far as we + /// can tell. false if the restore attempt itself failed, so the caller can warn that the database may + /// still be in SINGLE_USER. Never throws. + /// + public static async Task TryRestoreDatabaseAccessAsync( + string connectionString, + IProgress? progress = null) + { + try + { + var builder = new SqlConnectionStringBuilder(connectionString) { InitialCatalog = "master" }; + using var connection = new SqlConnection(builder.ConnectionString); + await connection.OpenAsync().ConfigureAwait(false); + + /* + Plain SET MULTI_USER, deliberately NOT WITH ROLLBACK IMMEDIATE. In the one race where another + session has grabbed the single-user slot, this blocks on the exclusive lock (up to the timeout) + and then fails best-effort rather than force-killing whoever raced in. The common case -- our own + timed-out session in master context, nobody in the slot -- is restored cleanly. + */ + using var cmd = new SqlCommand( + @"IF SERVERPROPERTY('EngineEdition') <> 8 AND DB_ID(N'PerformanceMonitor') IS NOT NULL + ALTER DATABASE PerformanceMonitor SET MULTI_USER;", + connection); + cmd.CommandTimeout = ShortTimeoutSeconds; + await cmd.ExecuteNonQueryAsync().ConfigureAwait(false); + return true; + } + catch (Exception ex) + { + /* Best-effort: the original failure is what the caller must see, never this one. */ + LogDebug(progress, $"TryRestoreDatabaseAccessAsync: MULTI_USER restore failed (ignored) — {ex.Message}"); + return false; + } + } + /// /// Perform complete uninstall (remove database, jobs, XE sessions, and traces). /// @@ -379,6 +473,9 @@ public static async Task ExecuteInstallationAsync( StartTime = DateTime.Now }; + /* Cancelled before we have touched anything: a plain cancel, not a failure. */ + cancellationToken.ThrowIfCancellationRequested(); + /*Perform clean install if requested*/ if (cleanInstall) { @@ -386,6 +483,23 @@ public static async Task ExecuteInstallationAsync( { await CleanInstallAsync(connectionString, progress, cancellationToken).ConfigureAwait(false); } + catch (Exception) when (cancellationToken.IsCancellationRequested) + { + /* + A CANCEL, surfaced as one. SqlCommand does not fault with OperationCanceledException when its + token trips -- it throws SqlException -- so the catch below used to swallow a cancelled clean + install as an ordinary "failure" and return NORMALLY. The caller's cancellation path never + ran. What the user got instead was "Installation completed with 1 error(s)" over a database + that CleanInstallAsync may have already dropped (it drops the Agent jobs, the XE sessions, + then SET SINGLE_USER WITH ROLLBACK IMMEDIATE and DROP DATABASE, all before a single install + file runs) -- and the dialog then re-stamped the verdict it had deliberately discarded and + armed a Save that skips the connection test entirely. + + The first ThrowIfCancellationRequested in this method is AFTER the clean install, which is + exactly the window where cancelling is most destructive and least recoverable. + */ + throw new OperationCanceledException(cancellationToken); + } catch (Exception ex) { progress?.Report(new InstallationProgress @@ -527,6 +641,21 @@ Files execute without transaction wrapping because many contain DDL. result.FilesSucceeded++; } + catch (Exception) when (cancellationToken.IsCancellationRequested) + { + /* + A cancel, surfaced as one -- the same pattern as the clean-install branch above, and here for + the same reason. A cancelled SqlCommand faults with SqlException, not + OperationCanceledException, so the general catch below counted a Cancel mid-file as a file + FAILURE and the run returned Success=false. The loop-top ThrowIfCancellationRequested only + re-surfaces the cancel if control reaches the top of the loop again -- which it does NOT for a + critical file (it `break`s) or for the LAST file (the loop just ends), so those two cancels + fell through to "completed with N error(s)" and the caller's FAILURE path instead of its + CANCELLATION path. Converting here, ahead of the general catch, keeps the failure accounting + honest: a cancel is not a file failure, and it is not recorded as one. + */ + throw new OperationCanceledException(cancellationToken); + } catch (Exception ex) { LogDebug(progress, $" {fileName}: FAILED — {ex.GetType().Name}: {ex.Message}"); @@ -836,6 +965,23 @@ public static async Task RunTroubleshootingAsync( /// Generate installation summary report file. /// /// Directory to write the report. Null defaults to user profile. + /* + Every character Windows forbids in a filename -> underscore, including the separators and ".." pieces + that a path-traversal string is built from. Mirrors Program.SanitizeFilename; Installer.Core cannot + reach that private CLI helper, and this is the shared writer both front ends call. + */ + private static string SanitizeForFileName(string value) + { + char[] invalid = Path.GetInvalidFileNameChars(); + var sb = new StringBuilder(value.Length); + foreach (char c in value) + { + sb.Append(Array.IndexOf(invalid, c) >= 0 ? '_' : c); + } + + return sb.ToString(); + } + public static string GenerateSummaryReport( string serverName, string sqlServerVersion, @@ -850,10 +996,34 @@ public static string GenerateSummaryReport( var duration = result.EndTime - result.StartTime; string timestamp = result.StartTime.ToString("yyyyMMdd_HHmmss"); - string fileName = $"PerformanceMonitor_Install_{serverName.Replace("\\", "_", StringComparison.Ordinal)}_{timestamp}.txt"; + + /* + Strip EVERY invalid filename char, not just the backslash. The server name is user-typed, and a + value like "x/../../Users/Public/foo" turned this Path.Combine into a write OUTSIDE reportDir -- + forward slash is a separator, ".." is parent, ":" opens an NTFS alternate data stream, all of which + the old .Replace("\\","_") let straight through. The "PerformanceMonitor_Install_" prefix does not + contain it; ".." just escapes past it. The CLI's own error-log writer already sanitizes the same + input with GetInvalidFileNameChars (Program.SanitizeFilename); this report writer -- called by BOTH + the CLI and the Dashboard -- was the one that was missed. + */ + string fileName = $"PerformanceMonitor_Install_{SanitizeForFileName(serverName)}_{timestamp}.txt"; string reportDir = outputDirectory ?? Environment.GetFolderPath(Environment.SpecialFolder.UserProfile); string reportPath = Path.Combine(reportDir, fileName); + /* + Belt and braces: even sanitized, refuse to write outside the intended directory. A future change to + the filename builder cannot turn this into an arbitrary-write without tripping here first. + */ + string fullReportDir = Path.GetFullPath(reportDir); + string fullReportPath = Path.GetFullPath(reportPath); + if (!fullReportPath.StartsWith( + fullReportDir.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar, + StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException( + "Refusing to write the installation report outside the report directory."); + } + var sb = new StringBuilder(); sb.AppendLine("================================================================================"); @@ -945,47 +1115,81 @@ FROM sys.databases WHERE name = N'PerformanceMonitor';", connection); var dbExists = await dbCheckCmd.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false); - if (dbExists == null || dbExists == DBNull.Value) + bool databaseExists = dbExists != null && dbExists != DBNull.Value; + + if (!databaseExists) { LogDebug(progress, "GetInstalledVersionAsync: database does not exist → clean install"); - return null; + return InstalledVersionClassifier.Classify(false, false, 0L, null); } - LogDebug(progress, "GetInstalledVersionAsync: database exists, checking installation_history table"); /*Check if installation_history table exists*/ using var tableCheckCmd = new SqlCommand(@" USE PerformanceMonitor; SELECT OBJECT_ID(N'config.installation_history', N'U');", connection); - var tableExists = await tableCheckCmd.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false); - if (tableExists == null || tableExists == DBNull.Value) + var tableOid = await tableCheckCmd.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false); + bool historyTableExists = tableOid != null && tableOid != DBNull.Value; + + long collectTableCount = 0L; + if (!historyTableExists) { - LogDebug(progress, "GetInstalledVersionAsync: installation_history table does not exist → old or corrupted install"); - return null; + /* + No ledger. Count the collect objects so the classifier can tell a PerformanceMonitor + database that LOST its history apart from an empty database someone pre-created for us. + Answering "no version" for the first is the stranding bug this whole path exists to + prevent; answering "installed" for the second would run migrations against tables that do + not exist. See InstalledVersionClassifier. + */ + using var collectCheckCmd = new SqlCommand(@" + SELECT + COUNT_BIG(*) + FROM PerformanceMonitor.sys.tables AS t + INNER JOIN PerformanceMonitor.sys.schemas AS s + ON s.schema_id = t.schema_id + WHERE s.name = N'collect';", connection); + + var collectTables = await collectCheckCmd.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false); + collectTableCount = + collectTables == null || collectTables == DBNull.Value + ? 0L + : Convert.ToInt64(collectTables); } - /*Get most recent successful installation version*/ - using var versionCmd = new SqlCommand(@" - SELECT TOP 1 installer_version - FROM PerformanceMonitor.config.installation_history - WHERE installation_status = 'SUCCESS' - ORDER BY installation_date DESC;", connection); - - var version = await versionCmd.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false); - if (version != null && version != DBNull.Value) + string? latestSuccessVersion = null; + if (historyTableExists) { - LogDebug(progress, $"GetInstalledVersionAsync: found installed version {version}"); - return version.ToString(); + /*Get most recent successful installation version*/ + using var versionCmd = new SqlCommand(@" + SELECT TOP (1) + installer_version + FROM PerformanceMonitor.config.installation_history + WHERE installation_status = 'SUCCESS' + ORDER BY installation_date DESC;", connection); + + var version = await versionCmd.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false); + if (version != null && version != DBNull.Value) + { + latestSuccessVersion = version.ToString(); + } } /* - Fallback: database and history table exist but no SUCCESS rows. - This can happen if a prior install didn't write history (#538/#539). - Return "1.0.0" so all idempotent upgrade scripts are attempted - rather than treating this as a fresh install (which would drop the database). + The DECISION lives in InstalledVersionClassifier so it can be pinned by tests CI actually + runs -- this method needs a live SQL Server, and that suite is excluded from CI. */ - LogDebug(progress, "GetInstalledVersionAsync: no SUCCESS rows — fallback to 1.0.0 (#538 guard)"); - return "1.0.0"; + string? resolved = InstalledVersionClassifier.Classify( + databaseExists, + historyTableExists, + collectTableCount, + latestSuccessVersion); + + LogDebug( + progress, + $"GetInstalledVersionAsync: dbExists={databaseExists}, historyTable={historyTableExists}, " + + $"collectTables={collectTableCount}, successRow={latestSuccessVersion ?? "(none)"} → {resolved ?? "(clean install)"}"); + + return resolved; } catch (SqlException ex) { @@ -1126,8 +1330,27 @@ existing database (no upgrades, then logged as SUCCESS — the #538 hazard). Sof int totalSuccessCount = 0; int totalFailureCount = 0; - var upgrades = provider.GetApplicableUpgrades(currentVersion, targetVersion, - warning => progress?.Report(new InstallationProgress { Message = warning, Status = "Warning" })); + List upgrades; + try + { + upgrades = provider.GetApplicableUpgrades(currentVersion, targetVersion, + warning => progress?.Report(new InstallationProgress { Message = warning, Status = "Warning" })); + } + catch (ArgumentException ex) + { + /* + Report as a failure rather than letting it surface as "no upgrades needed". Callers abort + when totalFailureCount > 0, so this keeps an unreadable version from silently skipping + every migration and then stamping the database as current. + */ + progress?.Report(new InstallationProgress + { + Message = $"Cannot determine which upgrades to apply: {ex.Message}", + Status = "Error" + }); + + return (0, 1, 0); + } if (upgrades.Count == 0) { diff --git a/Installer.Core/InstalledVersionClassifier.cs b/Installer.Core/InstalledVersionClassifier.cs new file mode 100644 index 0000000000..848300d073 --- /dev/null +++ b/Installer.Core/InstalledVersionClassifier.cs @@ -0,0 +1,76 @@ +namespace Installer.Core; + +/// +/// Turns what we can SEE on a server into the version we act on. This is the single most consequential +/// decision in the installer: it is what separates "nothing is installed, do a clean install" from +/// "something is installed, work out which migrations still need to run". +/// +/// Getting it wrong in one direction strands migrations forever. A PerformanceMonitor database whose +/// ledger was lost — someone dropped config.installation_history, or a restore left it behind — +/// used to answer "no version" and therefore read as a FRESH INSTALL: the install scripts ran over the +/// live schema, every migration was skipped, and the target version was stamped SUCCESS. Version +/// detection reads back the most recent SUCCESS row, so those hops were never offered again. +/// +/// Getting it wrong in the other direction breaks a legitimate install: an EMPTY database that someone +/// pre-created (commonly to control the data/log file paths) genuinely has nothing installed, and +/// attempting migrations against tables that do not exist would fail. +/// +/// Split out of so it can be pinned by unit +/// tests that CI actually runs — the SQL that feeds it needs a live SQL Server, and that test suite is +/// excluded from CI. +/// +public static class InstalledVersionClassifier +{ + /// The PerformanceMonitor database exists on the server. + /// config.installation_history exists. + /// + /// How many tables live in the collect schema. The discriminator between a PerformanceMonitor + /// database that lost its ledger and an empty database someone pre-created for us. + /// + /// + /// installer_version of the most recent SUCCESS row, or null when there is none. + /// + /// + /// null when nothing is installed (do a clean install), a real version when we know it, or + /// when something IS installed but its + /// version cannot be read — meaning "attempt every upgrade", the safe direction, since every + /// upgrade script is IF NOT EXISTS-guarded and replays cleanly. + /// + public static string? Classify( + bool databaseExists, + bool historyTableExists, + long collectTableCount, + string? latestSuccessVersion) + { + if (!databaseExists) + { + /* Nothing there at all. */ + return null; + } + + if (!historyTableExists) + { + /* + No ledger. If the database holds collect objects it is a PerformanceMonitor database that lost + its history -- answering null here is the stranding bug above. If it holds none, it is an empty + database someone pre-created, and a clean install is right. + */ + return collectTableCount > 0 ? InstallationService.UnknownVersionSentinel : null; + } + + /* + The ledger exists. A SUCCESS row is the answer; without one we know something was installed but + not what, so fall back to "attempt every upgrade" rather than treating it as a fresh install and + dropping the database (#538). + + Blank counts as absent, not as an answer. installer_version is NOT NULL, so a row hand-edited to '' + -- and the block message we show an operator literally invites them to edit that row -- came back as + "" rather than null, and "" is not a version: FilterUpgrades turns it into ZERO hops, which reads as + "nothing to do" and strands every migration. InstallGuard rejects "" on every path today, so this is + the second lock on the same door, and the ledger is exactly where a second lock is worth having. + */ + return string.IsNullOrWhiteSpace(latestSuccessVersion) + ? InstallationService.UnknownVersionSentinel + : latestSuccessVersion; + } +} diff --git a/Installer.Core/RepairOutcome.cs b/Installer.Core/RepairOutcome.cs new file mode 100644 index 0000000000..8c2f18ec8f --- /dev/null +++ b/Installer.Core/RepairOutcome.cs @@ -0,0 +1,103 @@ +namespace Installer.Core; + +/// +/// Decides whether a repair's failed install files are the EXPECTED kind. +/// +/// A repair runs the install scripts without the migrations. Those scripts compile against the CURRENT +/// schema, and ALTER PROCEDURE binds columns at compile time -- e.g. +/// install/23_process_blocked_process_xml.sql reads +/// collect.blocking_BlockedProcessReport.monitor_loop, a column the 3.0.0-to-3.1.0 migration adds. +/// So on a database with a PENDING upgrade, some procedures simply cannot compile until it runs +/// (Msg 207, "Invalid column name"). A failed CREATE OR ALTER leaves the old body intact, nothing +/// is damaged, and the upgrade's own install pass recompiles them. +/// +/// Both conditions matter, and getting either wrong is dangerous in a different direction: +/// +/// +/// Treating those failures as a FAILURE sends the operator reaching for a destructive reinstall, +/// and makes a %ERRORLEVEL% gate reject a good repair. +/// Treating them as EXPECTED when there is NO pending upgrade reports SUCCESS over genuinely +/// broken objects, and blames a migration that does not exist. +/// +/// +/// Shared so the Dashboard and the CLI cannot drift: this is the contract the CHANGELOG states. +/// +public static class RepairOutcome +{ + /// + /// True when a repair's install-file failures are the expected uncompilable-until-upgraded kind, and + /// the run should therefore be reported as a success with a "now run the upgrade" handoff. + /// + /// A repair actually ran against an existing installation. + /// The version recorded on the server. + /// This binary's version. + /// + /// A critical file (01_/02_/03_) failed, which aborts the whole pass -- so the repair reinstalled + /// nothing and genuinely failed, whatever the version situation. + /// + public static bool FailuresAreExpected( + bool repairRan, + string? installedVersion, + string? targetVersion, + bool criticalFileFailed) + { + if (!repairRan || criticalFileFailed) + { + return false; + } + + /* + The "unknown" sentinel is a GUESS, not a version -- GetInstalledVersionAsync returns it when the + database is clearly installed but its recorded version cannot be read. It sorts below every real + version, so trusting it here would answer "yes, an upgrade is pending" unconditionally, and every + REAL repair failure on such a server would be reported as expected and exit 0. Concretely: a + schema-current 3.1.0 server whose history rows are all FAILED, with four genuinely broken + procedures, would pass a %ERRORLEVEL% gate while telling the operator to ignore the errors. + */ + if (string.Equals(installedVersion?.Trim(), InstallationService.UnknownVersionSentinel, StringComparison.Ordinal)) + { + return false; + } + + var installed = ScriptProvider.TryParseVersionCore(installedVersion); + var target = ScriptProvider.TryParseVersionCore(targetVersion); + + /* No comparable versions means no basis for the "expected" story. */ + if (installed == null || target == null) + { + return false; + } + + /* The expected failures only exist when there is a migration still to run. */ + return installed < target; + } + + /// + /// True when there is an upgrade to run AFTER this repair — i.e. what the "now run the upgrade" + /// handoff should key on. + /// + /// This is a DIFFERENT question from , and the difference is 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 one boolean + /// for both told the operator "already at the current version, so there is no upgrade to apply" for + /// a server with eleven pending hops, and withheld the handoff button that would have applied them. + /// + public static bool HasPendingUpgrade(string? installedVersion, string? targetVersion) + { + var installed = ScriptProvider.TryParseVersionCore(installedVersion); + var target = ScriptProvider.TryParseVersionCore(targetVersion); + + return installed != null && target != null && installed < target; + } + + /// + /// True when the recorded version could not be read, so it is unknown which migrations have run. + /// Callers must say so rather than asserting the server is current. + /// + public static bool IsVersionUnknown(string? installedVersion) => + string.Equals( + installedVersion?.Trim(), + InstallationService.UnknownVersionSentinel, + StringComparison.Ordinal); +} diff --git a/Installer.Core/ScriptProvider.cs b/Installer.Core/ScriptProvider.cs index bc117235ff..7b893c7da6 100644 --- a/Installer.Core/ScriptProvider.cs +++ b/Installer.Core/ScriptProvider.cs @@ -80,21 +80,21 @@ public abstract Task ReadUpgradeScriptAsync( /// /// Core upgrade-discovery logic shared by both providers. /// + /// + /// Thrown when a version is present but has no parseable numeric core. An empty result means + /// "no upgrades needed", which must never be the answer to "I could not read the version you gave me". + /// protected static List FilterUpgrades( IEnumerable candidates, string? currentVersion, string targetVersion) { - if (currentVersion == null) + /* No recorded version at all means a fresh install: there is nothing to upgrade from. */ + if (string.IsNullOrWhiteSpace(currentVersion)) return []; - if (!Version.TryParse(currentVersion, out var currentRaw)) - return []; - var current = new Version(currentRaw.Major, currentRaw.Minor, currentRaw.Build); - - if (!Version.TryParse(targetVersion, out var targetRaw)) - return []; - var target = new Version(targetRaw.Major, targetRaw.Minor, targetRaw.Build); + var current = ParseVersionCore(currentVersion, nameof(currentVersion)); + var target = ParseVersionCore(targetVersion, nameof(targetVersion)); return candidates .Where(x => x.FromVersion != null && x.ToVersion != null) @@ -104,6 +104,50 @@ protected static List FilterUpgrades( .ToList(); } + /// + /// Parses a version to its three-part numeric core, stripping any SemVer build (+sha) or + /// pre-release (-rc1) suffix. Returns null when there is no parseable core, rather than + /// throwing — for callers that need to *decide* something about a version rather than use it. + /// Mirrors SingleInstanceDecision.ParseProductVersion, re-stated here to keep + /// Installer.Core dependency-free. + /// + public static Version? TryParseVersionCore(string? version) + { + if (string.IsNullOrWhiteSpace(version)) + return null; + + string core = version.Trim(); + + int plus = core.IndexOf('+', StringComparison.Ordinal); + if (plus >= 0) + core = core[..plus]; + + int dash = core.IndexOf('-', StringComparison.Ordinal); + if (dash >= 0) + core = core[..dash]; + + if (!Version.TryParse(core, out var parsed)) + return null; + + /* TryParse leaves Build at -1 for a two-part version like "3.1", which the Version ctor rejects. */ + return new Version(parsed.Major, parsed.Minor, Math.Max(parsed.Build, 0)); + } + + private static Version ParseVersionCore(string version, string paramName) + { + /* + A version that is present but has no numeric core is a caller bug -- a status string like + "Unreachable" reaching us instead of a version. Returning an empty list would run zero + migrations and report zero failures, and the caller would then stamp installation_history as + SUCCESS at the target version. Version detection only reads the most recent SUCCESS row, so + every skipped hop would be stranded permanently. Fail loudly instead. + */ + return TryParseVersionCore(version) + ?? throw new ArgumentException( + $"'{version}' is not a valid version. Upgrade discovery needs a version, not a status string.", + paramName); + } + /// /// Parses an upgrade folder name like "1.2.0-to-1.3.0" into an UpgradeInfo. /// Returns null if the name doesn't match the expected pattern. diff --git a/Installer.Core/ServerSetupPolicy.cs b/Installer.Core/ServerSetupPolicy.cs new file mode 100644 index 0000000000..e1418053e8 --- /dev/null +++ b/Installer.Core/ServerSetupPolicy.cs @@ -0,0 +1,80 @@ +namespace Installer.Core; + +/// +/// The pure decisions of the add/edit-server flow, extracted out of the (untested) WPF dialog so CI can +/// pin them. Every method here is a total function of (plus, for +/// , two version strings) with no UI dependency — which is exactly what +/// lets ServerSetupPolicyTests assert the full state×predicate matrix. +/// +/// The bug class this closes: a predicate over the state that silently omits a state. It happened more than +/// once — listing three states while +/// listed five and the two disagreeing about the same server; the destructive-checkbox rule missing an exit. +/// A switch that forgets a state compiles fine and fails only in the UI; a matrix test over the enum does not. +/// +public static class ServerSetupPolicy +{ + /// + /// Does this state assert a discovered fact ABOUT the server currently on screen — its installed + /// version, whether it needs an upgrade, whether it is current? Those verdicts must not be rendered + /// once the server-name box no longer matches the server they were read from. This is the set the + /// stale-verdict guard demotes. + /// + public static bool IsServerVerdictState(ServerSetupState state) => + state is ServerSetupState.Connected_NoDatabase + or ServerSetupState.Connected_NeedsUpgrade + or ServerSetupState.Connected_Current; + + /// + /// Did the button the user actually clicked promise to act on an EXISTING installation? Only + /// says "Install Now"; every other launchable state + /// promises otherwise — including and + /// , where the repair handoff deliberately puts a + /// live "Upgrade Now" in front of the user. Those two are NOT verdict states, so they are the pair + /// and this predicate legitimately disagree on — a real bug came from + /// making them agree. + /// + public static bool PromisesExistingInstall(ServerSetupState launchState) => launchState switch + { + ServerSetupState.Connected_NeedsUpgrade => true, // "Upgrade Now" + ServerSetupState.Connected_Current => true, // "Reinstall Objects" + ServerSetupState.InstallComplete => true, // repair handoff: "Upgrade Now" + ServerSetupState.MonitoringCredentials => true, // repair handoff, SQL auth: "Upgrade Now" + _ => false, // Connected_NoDatabase: "Install Now"; and the non-launchable states + }; + + /// + /// The ONE state that consumes the destructive checkboxes (clean install → DROP DATABASE, repair, reset + /// schedule → TRUNCATE): , which is about to read them. Every + /// other transition must clear them, because consent for a destructive option is per-run and per-server + /// and a tick must never survive onto a server it was not set for. Keeping this as a single predicate — + /// rather than clearing the ticks at each exit — is what stops the next exit from being the one that + /// forgets. It reopened as a bug more than once when it was enforced exit-by-exit. + /// + public static bool StateConsumesModeSelections(ServerSetupState state) => + state == ServerSetupState.Installing; + + /// True only while a run is in progress. The backstop against starting a second install. + public static bool IsInstalling(ServerSetupState state) => + state == ServerSetupState.Installing; + + /// + /// Which connected state the FACTS imply. Used by detection and — crucially — re-derived from the + /// install-time re-read before the install begins, never restored from "the state we came from" (the + /// box stays editable, so the state we came from may describe a different server). + /// + /// null means no database (fresh install). Otherwise the choice is + /// pending-upgrade vs current, decided by so the unknown + /// sentinel (which sorts below every real version) correctly reads as "an upgrade is pending". + /// + public static ServerSetupState DeriveConnectedState(string? installedVersion, string appVersion) + { + if (installedVersion == null) + { + return ServerSetupState.Connected_NoDatabase; + } + + return RepairOutcome.HasPendingUpgrade(installedVersion, appVersion) + ? ServerSetupState.Connected_NeedsUpgrade + : ServerSetupState.Connected_Current; + } +} diff --git a/Installer.Core/ServerSetupState.cs b/Installer.Core/ServerSetupState.cs new file mode 100644 index 0000000000..1409cb049f --- /dev/null +++ b/Installer.Core/ServerSetupState.cs @@ -0,0 +1,49 @@ +using System.Diagnostics.CodeAnalysis; + +namespace Installer.Core; + +/// +/// The state of the "add / edit a monitored server" flow: connect to a server, discover whether +/// PerformanceMonitor is installed on it, and install or upgrade. The Dashboard's AddServerDialog aliases +/// this as DialogState and drives its UI from it. +/// +/// This enum and the predicates over it () live here, in Installer.Core, +/// for one reason: they are pure decisions, and Installer.Core is CI-tested while the WPF dialog is not. +/// Every bug in this flow that survived review came in one of two shapes, and one of them was "a predicate +/// over the state that failed to enumerate every state" — a class the matrix tests over this enum make +/// impossible to reintroduce silently. +/// +[SuppressMessage("Naming", "CA1707:Identifiers should not contain underscores", + Justification = "Connected_* is a deliberate, readable grouping convention for this flow's states; it " + + "carried over verbatim from the dialog's original private enum and reads better than " + + "the un-underscored form.")] +public enum ServerSetupState +{ + /// Nothing checked yet — a fresh dialog, or one whose fields were just edited. + Initial, + + /// Connected; no PerformanceMonitor database present. The offer is "Install Now" (or Skip). + Connected_NoDatabase, + + /// Connected; PerformanceMonitor installed but behind this build. The offer is "Upgrade Now". + Connected_NeedsUpgrade, + + /// Connected; PerformanceMonitor installed and current. The offer is "Reinstall Objects". + Connected_Current, + + /// + /// Connected, but the installed status could not be trusted — a failed re-read, an unsupported or + /// unreadable version, a database newer than the binary, or a verdict demoted because the server-name + /// box changed after it was checked. Installing is blocked; the install button is hidden. + /// + Connected_StatusUnknown, + + /// An install or upgrade is running. The ONLY state that consumes the destructive checkboxes. + Installing, + + /// A run finished. May carry a repair handoff ("Upgrade Now") for the still-pending upgrade. + InstallComplete, + + /// Post-install, SQL auth: capturing the ongoing monitoring credentials. May also carry the handoff. + MonitoringCredentials +} diff --git a/Installer.Tests/CleanInstallCancellationTests.cs b/Installer.Tests/CleanInstallCancellationTests.cs new file mode 100644 index 0000000000..c20b3b99c6 --- /dev/null +++ b/Installer.Tests/CleanInstallCancellationTests.cs @@ -0,0 +1,73 @@ +using Installer.Core; + +namespace Installer.Tests; + +/// +/// Pins the one thing a cancelled clean install must never do: come back looking like a completed run. +/// +/// CleanInstallAsync drops the three Agent jobs, both Extended Events sessions, and then the DATABASE +/// (SET SINGLE_USER WITH ROLLBACK IMMEDIATE, DROP DATABASE) -- all of it BEFORE a single install file +/// runs. Cancelling in that window is the most destructive and least recoverable moment in the whole +/// installer, and it was the one window with no cancellation check in front of it. +/// +/// 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. The user was told "Installation completed with 1 error(s)" over a database that may already +/// have been dropped; the dialog restored 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. +/// +/// These need no database precisely because the guard must fire BEFORE anything is contacted -- the +/// connection string points at a host that must never be reached. If the guard regresses, the test stops +/// seeing OperationCanceledException and starts seeing a SQL error or a timeout, which is exactly the +/// failure it exists to catch. +/// +public class CleanInstallCancellationTests +{ + /* Deliberately unreachable. Contacting it at all is the bug. */ + private const string UnreachableServer = + "Server=this-host-must-never-be-contacted.invalid;Database=master;" + + "User Id=nobody;Password=nobody;Connect Timeout=1;TrustServerCertificate=true"; + + [Fact] + public async Task CancelledBeforeStart_CleanInstall_Cancels_AndNeverContactsTheServer() + { + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await Assert.ThrowsAnyAsync(() => + InstallationService.ExecuteInstallationAsync( + UnreachableServer, + ScriptProvider.FromEmbeddedResources(), + cleanInstall: true, + cancellationToken: cts.Token)); + } + + [Fact] + public async Task CancelledBeforeStart_OrdinaryInstall_CancelsToo() + { + /* The guard sits ahead of the clean-install branch, so it covers both shapes of run. */ + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + await Assert.ThrowsAnyAsync(() => + InstallationService.ExecuteInstallationAsync( + UnreachableServer, + ScriptProvider.FromEmbeddedResources(), + cleanInstall: false, + cancellationToken: cts.Token)); + } + + /* + COVERAGE BOUNDARY, stated honestly. These pin the PRE-file windows -- a token already cancelled trips + the guard at the top of ExecuteInstallationAsync (and the loop-top guard) before any command runs, so + they never exercise a cancel that lands INSIDE a running SqlCommand. + + That third window -- a cancel mid-file, which faults as SqlException rather than OperationCanceledException + and used to be miscounted as a file failure -- is handled by a dedicated `catch when + (cancellationToken.IsCancellationRequested)` that mirrors the clean-install branch. It can only be + exercised end to end against a live server (a connection that opens, then a cancel during execution), + which these DB-free tests must not do. It is covered by click-through item: cancel a normal install while + a file is executing and confirm the dialog shows "cancelled", not "completed with N error(s)". + */ +} diff --git a/Installer.Tests/Helpers/TestDatabaseHelper.cs b/Installer.Tests/Helpers/TestDatabaseHelper.cs index 05bdb64949..281e3ff934 100644 --- a/Installer.Tests/Helpers/TestDatabaseHelper.cs +++ b/Installer.Tests/Helpers/TestDatabaseHelper.cs @@ -32,10 +32,13 @@ public static async Task DropTestDatabaseAsync() using var connection = new SqlConnection(GetConnectionString()); await connection.OpenAsync(TestContext.Current.CancellationToken); + // EngineEdition 8 = Azure SQL Managed Instance, where SINGLE_USER is non-modifiable. These tests + // target box SQL Server, but the gate keeps the pattern correct so it is safe to copy from. using var cmd = new SqlCommand($@" IF DB_ID(N'{TestDatabaseName}') IS NOT NULL BEGIN - ALTER DATABASE [{TestDatabaseName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; + IF SERVERPROPERTY('EngineEdition') <> 8 + ALTER DATABASE [{TestDatabaseName}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE; DROP DATABASE [{TestDatabaseName}]; END;", connection); await cmd.ExecuteNonQueryAsync(TestContext.Current.CancellationToken); diff --git a/Installer.Tests/InstallGuardTests.cs b/Installer.Tests/InstallGuardTests.cs new file mode 100644 index 0000000000..82cad2da2a --- /dev/null +++ b/Installer.Tests/InstallGuardTests.cs @@ -0,0 +1,78 @@ +using Installer.Core; + +namespace Installer.Tests; + +/// +/// Pins InstallGuard, the decision behind both installers' pre-install blocks. +/// +/// Both of the worst defects found while building this feature lived here, and BOTH are invisible to +/// every other guard: a version we cannot compare, or a server NEWER than this build, produces ZERO +/// upgrade hops and ZERO failures -- so the migration-failure abort never fires and nothing downstream +/// notices. The install then runs this binary's older scripts over the newer database, reverting every +/// CREATE OR ALTER procedure and view, and records the LOWER version as SUCCESS. Version detection reads +/// back the most recent SUCCESS row, so that silently strands every migration in between. +/// +/// Lives in Installer.Tests, not Dashboard.Tests, because Dashboard.Tests is not wired into CI -- tests +/// that cannot fail a PR are decoration. +/// +public class InstallGuardTests +{ + [Fact] + public void NoDatabase_IsAllowed() + { + /* Nothing installed: a fresh install is safe. */ + Assert.Equal(InstallBlock.None, InstallGuard.Check(null, "3.1.0")); + } + + [Theory] + [InlineData("3.1.0", "3.1.0")] + [InlineData("3.0.0", "3.1.0")] + [InlineData("3.1.0.0", "3.1.0")] + [InlineData("2.9", "3.1.0")] + [InlineData("3.0.0", "3.1.0+abc123")] + [InlineData("3.0.0", "3.2.0-rc1")] + public void SameOrOlderThanThisBuild_IsAllowed(string installed, string build) + { + Assert.Equal(InstallBlock.None, InstallGuard.Check(installed, build)); + } + + [Theory] + [InlineData("3.2.0", "3.1.0")] + [InlineData("3.1.1", "3.1.0")] + [InlineData("4.0.0", "3.1.0.0")] + [InlineData("3.2.0", "3.1.0+abc123")] + public void NewerThanThisBuild_IsBlocked(string installed, string build) + { + /* The silent downgrade: older scripts over a newer schema, then the LOWER version recorded. */ + Assert.Equal(InstallBlock.InstalledIsNewerThanBuild, InstallGuard.Check(installed, build)); + } + + [Theory] + [InlineData("Unreachable")] + [InlineData("Not installed")] + [InlineData("")] + [InlineData(" ")] + public void UnreadableInstalledVersion_IsBlocked(string installed) + { + Assert.Equal(InstallBlock.UnreadableInstalledVersion, InstallGuard.Check(installed, "3.1.0")); + } + + [Fact] + public void UnreadableBuildVersion_IsBlocked_AndBlamesTheBuild() + { + /* Nothing the user does to the database fixes a malformed InformationalVersion. */ + Assert.Equal(InstallBlock.UnreadableBuildVersion, InstallGuard.Check("3.0.0", "not-a-version")); + } + + [Fact] + public void UnreadableBuildVersion_IsBlocked_EvenOnAFreshServer() + { + /* + Regression: the build-version check used to sit BELOW the no-database early return, so a fresh + server sailed through -- and the build version is exactly what a fresh install writes to + installation_history.installer_version. That poisoned the ledger at birth, after which every + surface refuses to touch the server ever again. + */ + Assert.Equal(InstallBlock.UnreadableBuildVersion, InstallGuard.Check(null, "not-a-version")); + } +} diff --git a/Installer.Tests/InstalledVersionClassifierTests.cs b/Installer.Tests/InstalledVersionClassifierTests.cs new file mode 100644 index 0000000000..64bdbc7799 --- /dev/null +++ b/Installer.Tests/InstalledVersionClassifierTests.cs @@ -0,0 +1,114 @@ +using Installer.Core; + +namespace Installer.Tests; + +/// +/// Pins InstalledVersionClassifier — the decision that separates "nothing is installed, do a clean +/// install" from "something is installed, work out which migrations still need to run". +/// +/// It exists as a pure function precisely so these can run in CI. The SQL that feeds it needs a live +/// SQL Server, and that suite (VersionDetectionTests) is excluded from the CI filter — so before this +/// split, the single most consequential branch in the installer had no test that could fail a PR. +/// +public class InstalledVersionClassifierTests +{ + [Fact] + public void NoDatabase_IsACleanInstall() + { + Assert.Null(InstalledVersionClassifier.Classify( + databaseExists: false, historyTableExists: false, collectTableCount: 0, latestSuccessVersion: null)); + } + + [Fact] + public void EmptyPreCreatedDatabase_IsACleanInstall() + { + /* + Someone created the database themselves (commonly to control the data/log file paths) and then ran + the installer. Nothing is installed. Answering "installed" here would attempt migrations against + tables that do not exist, and every one of them would fail. + */ + Assert.Null(InstalledVersionClassifier.Classify( + databaseExists: true, historyTableExists: false, collectTableCount: 0, latestSuccessVersion: null)); + } + + [Fact] + public void LedgerLostButCollectObjectsPresent_IsUnknown_NotAFreshInstall() + { + /* + THE STRANDING BUG. A PerformanceMonitor database whose config.installation_history was dropped. + Answering "clean install" made the install scripts run over the LIVE schema, skip every migration, + and stamp the target version SUCCESS -- and since version detection reads back the most recent + SUCCESS row, those hops were never offered again. Permanently stranded, with no failed migration + anywhere in sight. + */ + Assert.Equal( + InstallationService.UnknownVersionSentinel, + InstalledVersionClassifier.Classify( + databaseExists: true, historyTableExists: false, collectTableCount: 62, latestSuccessVersion: null)); + } + + [Fact] + public void LedgerPresentWithSuccessRow_IsThatVersion() + { + Assert.Equal( + "3.0.0", + InstalledVersionClassifier.Classify( + databaseExists: true, historyTableExists: true, collectTableCount: 62, latestSuccessVersion: "3.0.0")); + } + + [Fact] + public void LedgerPresentButNoSuccessRow_IsUnknown_Regression538() + { + /* + #538: a prior install that never wrote a SUCCESS row. Treating it as a fresh install would drop + the database. "Unknown" means attempt every upgrade, which is safe -- every upgrade script is + IF NOT EXISTS-guarded and replays cleanly. + */ + Assert.Equal( + InstallationService.UnknownVersionSentinel, + InstalledVersionClassifier.Classify( + databaseExists: true, historyTableExists: true, collectTableCount: 62, latestSuccessVersion: null)); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void LedgerPresentButVersionIsBlank_IsUnknown_NotAnAnswer(string blank) + { + /* + installer_version is NOT NULL, so a row hand-edited to '' comes back as "" rather than null -- and + the "unreadable version" message we show an operator invites exactly that edit. "" is not a version: + FilterUpgrades resolves it to ZERO applicable hops, which reads as "already current" and strands + every pending migration, silently and permanently. Blank is absence of an answer, not an answer. + + InstallGuard rejects "" on every path today, so this is the second lock on the same door. The ledger + is where a second lock earns its keep. + */ + Assert.Equal( + InstallationService.UnknownVersionSentinel, + InstalledVersionClassifier.Classify( + databaseExists: true, historyTableExists: true, collectTableCount: 62, latestSuccessVersion: blank)); + } + + [Fact] + public void TheSentinelSortsBelowEveryRealVersion_SoEveryHopIsOffered() + { + /* Its whole job. If it ever stopped sorting below a real version, hops would be skipped silently. */ + var sentinel = ScriptProvider.TryParseVersionCore(InstallationService.UnknownVersionSentinel); + + Assert.NotNull(sentinel); + Assert.True(sentinel < ScriptProvider.TryParseVersionCore("1.2.0")); + Assert.True(sentinel < ScriptProvider.TryParseVersionCore("3.1.0")); + } + + [Fact] + public void TheSentinelIsNotARealShippedVersion() + { + /* + It used to be "1.0.0" -- a shipped tag. The "your recorded version is unreadable" message tells + operators to correct the SUCCESS row by hand, and "1.0.0" is exactly what someone would type to + force a full re-run; they would then be told their version could not be read. + */ + Assert.Equal("0.0.0", InstallationService.UnknownVersionSentinel); + } +} diff --git a/Installer.Tests/RepairOutcomeTests.cs b/Installer.Tests/RepairOutcomeTests.cs new file mode 100644 index 0000000000..adae8b0c60 --- /dev/null +++ b/Installer.Tests/RepairOutcomeTests.cs @@ -0,0 +1,132 @@ +using Installer.Core; + +namespace Installer.Tests; + +/// +/// Pins RepairOutcome, which both the Dashboard and the CLI depend on and which the CHANGELOG states as +/// a contract: a repair with a pending upgrade exits 0 even with failed files; a repair without one does +/// not. Getting it wrong is dangerous in BOTH directions -- reporting a good repair as a failure sends +/// the operator reaching for a destructive reinstall, and reporting a broken one as a success ships +/// genuinely uncompiled procedures past a CI gate. +/// +public class RepairOutcomeTests +{ + [Fact] + public void PendingUpgrade_FailuresAreExpected() + { + // The install scripts cannot compile against a schema the pending migration has not touched yet. + Assert.True(RepairOutcome.FailuresAreExpected( + repairRan: true, installedVersion: "3.0.0", targetVersion: "3.1.0", criticalFileFailed: false)); + } + + [Fact] + public void NoPendingUpgrade_FailuresAreReal() + { + // Nothing to blame: there is no migration-added column to be missing, so these are real errors. + Assert.False(RepairOutcome.FailuresAreExpected( + repairRan: true, installedVersion: "3.1.0", targetVersion: "3.1.0", criticalFileFailed: false)); + } + + [Fact] + public void ServerNewerThanBinary_FailuresAreReal() + { + Assert.False(RepairOutcome.FailuresAreExpected( + repairRan: true, installedVersion: "3.2.0", targetVersion: "3.1.0", criticalFileFailed: false)); + } + + [Fact] + public void CriticalFileFailed_IsNeverExpected() + { + // A critical file aborts the whole pass, so the repair reinstalled nothing and really did fail -- + // regardless of whether an upgrade is pending. + Assert.False(RepairOutcome.FailuresAreExpected( + repairRan: true, installedVersion: "3.0.0", targetVersion: "3.1.0", criticalFileFailed: true)); + } + + [Fact] + public void NotARepair_IsNeverExpected() + { + Assert.False(RepairOutcome.FailuresAreExpected( + repairRan: false, installedVersion: "3.0.0", targetVersion: "3.1.0", criticalFileFailed: false)); + } + + [Theory] + [InlineData(null, "3.1.0")] + [InlineData("3.0.0", null)] + [InlineData("Unreachable", "3.1.0")] + [InlineData("3.0.0", "not-a-version")] + public void UncomparableVersions_AreNeverExpected(string? installed, string? target) + { + // No comparable versions means no basis for the "expected failure" story. + Assert.False(RepairOutcome.FailuresAreExpected( + repairRan: true, installedVersion: installed, targetVersion: target, criticalFileFailed: false)); + } + + [Fact] + public void UnknownVersionSentinel_IsNeverExpected() + { + /* + The sentinel is GetInstalledVersionAsync's guess for "installed, but I cannot read the version" -- not + a fact. It sorts below every real version, so trusting it would answer "an upgrade is pending" + unconditionally and report every REAL repair failure as expected, exiting 0. A schema-current + 3.1.0 server whose history rows are all FAILED, with genuinely broken procedures, must not pass. + */ + Assert.False(RepairOutcome.FailuresAreExpected( + repairRan: true, + installedVersion: InstallationService.UnknownVersionSentinel, + targetVersion: "3.1.0", + criticalFileFailed: false)); + } + + [Fact] + public void UnknownSentinel_AnswersTheTwoQuestionsDIFFERENTLY() + { + /* + The whole point of splitting them. For an unreadable version: + - "may I excuse these file failures?" -> NO. We cannot certify a success we cannot explain. + - "is there an upgrade to run next?" -> YES. Every hop may be pending. + + Answering both with one flag printed "already at the current version, so there is no upgrade to + apply" for a server with eleven migrations waiting, and withheld the button that would have + applied them. + */ + const string sentinel = InstallationService.UnknownVersionSentinel; + + Assert.False(RepairOutcome.FailuresAreExpected( + repairRan: true, installedVersion: sentinel, targetVersion: "3.1.0", criticalFileFailed: false)); + + Assert.True(RepairOutcome.HasPendingUpgrade(sentinel, "3.1.0")); + Assert.True(RepairOutcome.IsVersionUnknown(sentinel)); + } + + [Theory] + [InlineData("3.1.0", "3.1.0", false)] + [InlineData("3.0.0", "3.1.0", true)] + [InlineData("3.2.0", "3.1.0", false)] + [InlineData("Unreachable", "3.1.0", false)] + [InlineData(null, "3.1.0", false)] + public void HasPendingUpgrade_KeysOnlyOnVersionOrder(string? installed, string target, bool expected) + { + Assert.Equal(expected, RepairOutcome.HasPendingUpgrade(installed, target)); + } + + [Theory] + [InlineData("3.1.0")] + [InlineData("Unreachable")] + [InlineData(null)] + public void IsVersionUnknown_OnlyTheSentinel(string? installed) + { + Assert.False(RepairOutcome.IsVersionUnknown(installed)); + } + + [Theory] + [InlineData("3.0.0", "3.1.0.0")] + [InlineData("2.9", "3.1.0")] + [InlineData("3.0.0", "3.1.0+abc123")] + [InlineData("3.0.0-rc1", "3.1.0")] + public void SemVerSuffixesAndPartLengths_StillCompare(string installed, string target) + { + Assert.True(RepairOutcome.FailuresAreExpected( + repairRan: true, installedVersion: installed, targetVersion: target, criticalFileFailed: false)); + } +} diff --git a/Installer.Tests/ServerSetupPolicyTests.cs b/Installer.Tests/ServerSetupPolicyTests.cs new file mode 100644 index 0000000000..1bfc8bf1fa --- /dev/null +++ b/Installer.Tests/ServerSetupPolicyTests.cs @@ -0,0 +1,154 @@ +using Installer.Core; + +namespace Installer.Tests; + +/// +/// The state×predicate matrix for the add/edit-server flow. This is the test that pays for extracting +/// ServerSetupState / ServerSetupPolicy out of the (un-CI-tested) WPF dialog: every predicate is asserted +/// for EVERY state, so a predicate that silently omits a state — the exact bug that recurred in the dialog +/// (IsServerVerdictState listing three states while PromisesExistingInstall listed five; the destructive- +/// checkbox rule missing an exit) — now fails a test instead of only misbehaving in the UI. +/// +/// Each predicate has one [InlineData] per enum member, and a guard test asserts the enum still has exactly +/// the eight members these tables were written against, so adding a ninth forces this file to be updated. +/// +public class ServerSetupPolicyTests +{ + [Fact] + public void TheEnumHasExactlyTheEightStatesTheseTablesCover() + { + // If this fails, a state was added or removed: extend every [Theory] below to cover it, on purpose. + Assert.Equal(8, Enum.GetValues().Length); + } + + // ---- IsServerVerdictState: states that assert a discovered fact about the server on screen ---- + + [Theory] + [InlineData(ServerSetupState.Connected_NoDatabase, true)] + [InlineData(ServerSetupState.Connected_NeedsUpgrade, true)] + [InlineData(ServerSetupState.Connected_Current, true)] + [InlineData(ServerSetupState.Initial, false)] + [InlineData(ServerSetupState.Connected_StatusUnknown, false)] // where a demotion LANDS -- not a verdict + [InlineData(ServerSetupState.Installing, false)] + [InlineData(ServerSetupState.InstallComplete, false)] + [InlineData(ServerSetupState.MonitoringCredentials, false)] + public void IsServerVerdictState_IsExactlyTheThreeConnectedVerdicts(ServerSetupState state, bool expected) + { + Assert.Equal(expected, ServerSetupPolicy.IsServerVerdictState(state)); + } + + // ---- PromisesExistingInstall: did the launched button act on an EXISTING install? ---- + + [Theory] + [InlineData(ServerSetupState.Connected_NeedsUpgrade, true)] // "Upgrade Now" + [InlineData(ServerSetupState.Connected_Current, true)] // "Reinstall Objects" + [InlineData(ServerSetupState.InstallComplete, true)] // repair handoff: "Upgrade Now" + [InlineData(ServerSetupState.MonitoringCredentials, true)] // repair handoff, SQL auth: "Upgrade Now" + [InlineData(ServerSetupState.Connected_NoDatabase, false)] // "Install Now" + [InlineData(ServerSetupState.Initial, false)] + [InlineData(ServerSetupState.Connected_StatusUnknown, false)] + [InlineData(ServerSetupState.Installing, false)] + public void PromisesExistingInstall_IncludesTheTwoRepairHandoffStates(ServerSetupState state, bool expected) + { + Assert.Equal(expected, ServerSetupPolicy.PromisesExistingInstall(state)); + } + + // ---- StateConsumesModeSelections: the ONLY state allowed to keep the destructive checkboxes ---- + + [Theory] + [InlineData(ServerSetupState.Installing, true)] + [InlineData(ServerSetupState.Initial, false)] + [InlineData(ServerSetupState.Connected_NoDatabase, false)] + [InlineData(ServerSetupState.Connected_NeedsUpgrade, false)] + [InlineData(ServerSetupState.Connected_Current, false)] + [InlineData(ServerSetupState.Connected_StatusUnknown, false)] + [InlineData(ServerSetupState.InstallComplete, false)] + [InlineData(ServerSetupState.MonitoringCredentials, false)] + public void StateConsumesModeSelections_IsInstallingAndNothingElse(ServerSetupState state, bool expected) + { + // Every state but Installing must clear clean-install (DROP DATABASE) / repair / reset-schedule + // (TRUNCATE). A regression here is how a live destructive tick survived onto a server that never + // consented -- the bug that reopened repeatedly when the rule was enforced exit-by-exit. + Assert.Equal(expected, ServerSetupPolicy.StateConsumesModeSelections(state)); + } + + [Fact] + public void IsInstalling_MatchesTheInstallingState() + { + foreach (var state in Enum.GetValues()) + { + Assert.Equal(state == ServerSetupState.Installing, ServerSetupPolicy.IsInstalling(state)); + } + } + + // ---- The relationships between the predicates, pinned because getting them "consistent" was a bug ---- + + [Fact] + public void RepairHandoffStates_PromiseAnExistingInstall_ButAreNotVerdictStates() + { + // The deliberate disagreement. InstallComplete / MonitoringCredentials carry a live "Upgrade Now" + // (so they promise an existing install) but assert no fresh verdict about the box (so they are not + // verdict states). A round of review "fixed" this by making them agree and reintroduced a bug. + foreach (var handoff in new[] { ServerSetupState.InstallComplete, ServerSetupState.MonitoringCredentials }) + { + Assert.True(ServerSetupPolicy.PromisesExistingInstall(handoff)); + Assert.False(ServerSetupPolicy.IsServerVerdictState(handoff)); + } + } + + [Fact] + public void Installing_IsNeitherAVerdictNorAPromiseState() + { + // It consumes the ticks; it must not also be treated as a server verdict or a launched promise. + Assert.True(ServerSetupPolicy.StateConsumesModeSelections(ServerSetupState.Installing)); + Assert.False(ServerSetupPolicy.IsServerVerdictState(ServerSetupState.Installing)); + Assert.False(ServerSetupPolicy.PromisesExistingInstall(ServerSetupState.Installing)); + } + + // ---- DeriveConnectedState: which connected state the FACTS imply ---- + + [Fact] + public void DeriveConnectedState_NullVersion_IsNoDatabase() + { + Assert.Equal( + ServerSetupState.Connected_NoDatabase, + ServerSetupPolicy.DeriveConnectedState(installedVersion: null, appVersion: "3.1.0")); + } + + [Fact] + public void DeriveConnectedState_OlderVersion_NeedsUpgrade() + { + Assert.Equal( + ServerSetupState.Connected_NeedsUpgrade, + ServerSetupPolicy.DeriveConnectedState("3.0.0", "3.1.0")); + } + + [Fact] + public void DeriveConnectedState_SameVersion_IsCurrent() + { + Assert.Equal( + ServerSetupState.Connected_Current, + ServerSetupPolicy.DeriveConnectedState("3.1.0", "3.1.0")); + } + + [Fact] + public void DeriveConnectedState_UnknownSentinel_NeedsUpgrade() + { + // The sentinel sorts below every real version, so "unknown installed version" correctly reads as + // "an upgrade is pending" -- attempt every hop, the safe direction. + Assert.Equal( + ServerSetupState.Connected_NeedsUpgrade, + ServerSetupPolicy.DeriveConnectedState(InstallationService.UnknownVersionSentinel, "3.1.0")); + } + + [Fact] + public void DeriveConnectedState_NewerThanBinary_IsCurrent_BecauseBlockingItIsInstallGuardsJob() + { + // Not "NeedsUpgrade" -- there is no hop to a lower version. Classifying a newer-than-binary database + // as Current is correct HERE; refusing to install over it is a separate decision (InstallGuard), + // made before this state is ever rendered. DeriveConnectedState only answers "is an upgrade pending". + Assert.Equal( + ServerSetupState.Connected_Current, + ServerSetupPolicy.DeriveConnectedState("3.2.0", "3.1.0")); + } +} diff --git a/Installer.Tests/SummaryReportPathTests.cs b/Installer.Tests/SummaryReportPathTests.cs new file mode 100644 index 0000000000..d39661f137 --- /dev/null +++ b/Installer.Tests/SummaryReportPathTests.cs @@ -0,0 +1,56 @@ +using Installer.Core; +using Installer.Core.Models; + +namespace Installer.Tests; + +/// +/// GenerateSummaryReport builds a filename out of the user-typed server name, and both front ends call it. +/// It used to strip only the backslash, so a server name like "x/../../foo" turned Path.Combine into a +/// write OUTSIDE the report directory -- forward slash is a separator and ".." is parent. These pin that +/// the report always lands inside the directory it was given, whatever the server name contains. +/// +public class SummaryReportPathTests +{ + private static InstallationResult AResult() => new() + { + Success = true, + FilesSucceeded = 1, + StartTime = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc), + EndTime = new DateTime(2026, 1, 1, 0, 0, 1, DateTimeKind.Utc) + }; + + [Theory] + [InlineData(@"x/../../../../Windows/Temp/pwn")] // forward-slash traversal + [InlineData(@"..\..\..\..\Users\Public\pwn")] // backslash traversal + [InlineData(@"SQL01:evil")] // NTFS alternate data stream + [InlineData(@"a/b/c")] // plain separators + [InlineData(@"SQL01\INSTANCE")] // ordinary, legitimate instance name + public void Report_AlwaysLandsInsideTheGivenDirectory(string serverName) + { + string dir = Path.Combine(Path.GetTempPath(), "pm_report_test_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(dir); + try + { + string written = InstallationService.GenerateSummaryReport( + serverName, "v", "Edition", "3.1.0", AResult(), dir); + + string fullDir = Path.GetFullPath(dir); + string fullWritten = Path.GetFullPath(written); + + Assert.StartsWith( + fullDir.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar, + fullWritten, + StringComparison.OrdinalIgnoreCase); + + // And it is directly in the directory -- one segment, no traversal survived. + Assert.Equal(fullDir.TrimEnd(Path.DirectorySeparatorChar), + Path.GetDirectoryName(fullWritten)!.TrimEnd(Path.DirectorySeparatorChar)); + + Assert.True(File.Exists(written)); + } + finally + { + try { Directory.Delete(dir, recursive: true); } catch { /* best effort */ } + } + } +} diff --git a/Installer.Tests/UpgradeOrderingTests.cs b/Installer.Tests/UpgradeOrderingTests.cs index 1346dfe1d2..66d9b887f0 100644 --- a/Installer.Tests/UpgradeOrderingTests.cs +++ b/Installer.Tests/UpgradeOrderingTests.cs @@ -116,6 +116,145 @@ public void NullCurrentVersion_ReturnsEmpty() Assert.Empty(upgrades); } + [Theory] + [InlineData("")] + [InlineData(" ")] + public void BlankCurrentVersion_ReturnsEmpty(string currentVersion) + { + using var dir = new TempDirectoryBuilder() + .WithUpgrade("2.0.0", "2.1.0", "01_columns.sql"); + + var upgrades = ScriptProvider.FromDirectory(dir.RootPath).GetApplicableUpgrades(currentVersion, "2.2.0"); + + Assert.Empty(upgrades); + } + + [Theory] + [InlineData("Unreachable")] + [InlineData("Not installed")] + [InlineData("2.x")] + public void UnparseableCurrentVersion_Throws(string currentVersion) + { + using var dir = new TempDirectoryBuilder() + .WithUpgrade("2.0.0", "2.1.0", "01_columns.sql"); + + var provider = ScriptProvider.FromDirectory(dir.RootPath); + + // An empty result means "no upgrades needed". If a status string silently produced one, + // every migration would be skipped and the caller would still stamp the DB as current. + var ex = Assert.Throws( + () => provider.GetApplicableUpgrades(currentVersion, "2.2.0")); + + Assert.Equal("currentVersion", ex.ParamName); + } + + [Theory] + [InlineData("2.2.0-rc1")] + [InlineData("2.2.0+abc123")] + [InlineData("2.2.0-rc1+abc123")] + public void SemVerSuffixOnTargetVersion_IsStripped(string targetVersion) + { + using var dir = new TempDirectoryBuilder() + .WithUpgrade("2.0.0", "2.1.0", "01_columns.sql") + .WithUpgrade("2.1.0", "2.2.0", "01_compress.sql"); + + // A pre-release must not make every upgrade abort. Without the + // suffix strip, "2.2.0-rc1" fails to parse and throws. + var upgrades = ScriptProvider.FromDirectory(dir.RootPath).GetApplicableUpgrades("2.0.0", targetVersion); + + Assert.Equal(2, upgrades.Count); + } + + [Fact] + public void TwoPartVersion_IsTreatedAsThreePart() + { + using var dir = new TempDirectoryBuilder() + .WithUpgrade("2.0.0", "2.1.0", "01_columns.sql") + .WithUpgrade("2.1.0", "2.2.0", "01_compress.sql"); + + // Version.TryParse("2.0") leaves Build == -1, which the Version(int,int,int) ctor rejects. + var upgrades = ScriptProvider.FromDirectory(dir.RootPath).GetApplicableUpgrades("2.0", "2.2"); + + Assert.Equal(2, upgrades.Count); + } + + [Fact] + public void UnparseableTargetVersion_Throws() + { + using var dir = new TempDirectoryBuilder() + .WithUpgrade("2.0.0", "2.1.0", "01_columns.sql"); + + var provider = ScriptProvider.FromDirectory(dir.RootPath); + + var ex = Assert.Throws( + () => provider.GetApplicableUpgrades("2.0.0", "not-a-version")); + + Assert.Equal("targetVersion", ex.ParamName); + } + + [Theory] + [InlineData("3.1.0", 3, 1, 0)] + [InlineData("3.1.0.4", 3, 1, 0)] + [InlineData("3.1", 3, 1, 0)] + [InlineData("3.2.0-rc1", 3, 2, 0)] + [InlineData("3.2.0+abc123", 3, 2, 0)] + [InlineData("3.2.0-rc1+abc123", 3, 2, 0)] + [InlineData(" 3.2.0 ", 3, 2, 0)] + public void TryParseVersionCore_ParsesRealVersions(string input, int major, int minor, int build) + { + Assert.Equal(new Version(major, minor, build), ScriptProvider.TryParseVersionCore(input)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("Unreachable")] + [InlineData("Not installed")] + [InlineData("v1.2.3")] + [InlineData("-rc1")] + [InlineData("+sha")] + public void TryParseVersionCore_ReturnsNullRatherThanThrowing(string? input) + { + // Callers that need to DECIDE something about a version (is it newer than me? is it + // readable?) must get null, not an exception -- unlike FilterUpgrades, which throws. + Assert.Null(ScriptProvider.TryParseVersionCore(input)); + } + + [Fact] + public void TryParseVersionCore_OrdersNewerAboveOlder() + { + // This comparison is what stops an older installer from running over a newer database and + // recording the lower version as SUCCESS -- a silent downgrade that produces zero hops and + // zero failures, so nothing else catches it. + var installed = ScriptProvider.TryParseVersionCore("3.2.0"); + var binary = ScriptProvider.TryParseVersionCore("3.1.0.0"); + + Assert.True(installed > binary); + } + + [Fact] + public async Task ExecuteAllUpgrades_UnreadableVersion_ReportsFailureRatherThanSkipping() + { + using var dir = new TempDirectoryBuilder() + .WithUpgrade("2.0.0", "2.1.0", "01_columns.sql"); + + // Callers abort when totalFailureCount > 0. Returning (0, 0, 0) here would read as + // "nothing to upgrade", and the caller would go on to stamp the database as current at + // the target version -- stranding every skipped hop. The connection string is never used: + // discovery fails before any database work. + var (successCount, failureCount, upgradeCount) = await InstallationService.ExecuteAllUpgradesAsync( + ScriptProvider.FromDirectory(dir.RootPath), + "Server=unused;Database=unused;", + "Unreachable", + "2.2.0", + cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(0, successCount); + Assert.Equal(1, failureCount); + Assert.Equal(0, upgradeCount); + } + [Fact] public void OrderedByFromVersion() { diff --git a/Installer.Tests/VersionDetectionTests.cs b/Installer.Tests/VersionDetectionTests.cs index 12837ac3cf..241ecc1411 100644 --- a/Installer.Tests/VersionDetectionTests.cs +++ b/Installer.Tests/VersionDetectionTests.cs @@ -1,3 +1,4 @@ +using Installer.Core; using Installer.Tests.Helpers; using Microsoft.Data.SqlClient; @@ -42,26 +43,57 @@ public async Task DatabaseExists_WithSuccessRow_ReturnsVersion() } [Fact] - public async Task DatabaseExists_NoHistoryTable_ReturnsNull() + public async Task DatabaseExists_NoHistoryTable_NoCollectObjects_ReturnsNull() { - // Create database but don't create the installation_history table + // An EMPTY database (someone pre-created it, e.g. to control the file paths). Nothing is + // installed, so a clean install is correct -- attempting migrations here would fail against + // tables that do not exist. await TestDatabaseHelper.CreateTestDatabaseAsync(); var version = await GetInstalledVersionFromTestDbAsync(); Assert.Null(version); } + [Fact] + public async Task DatabaseExists_NoHistoryTable_WithCollectObjects_ReturnsFallback() + { + // A PerformanceMonitor database whose LEDGER WAS LOST (someone dropped config.installation_history). + // Answering null here would read as "fresh install": the install scripts run over the live schema, + // every migration is skipped, and the target version is stamped SUCCESS -- stranding them + // permanently. It must fall back to the "attempt every upgrade" sentinel instead. + await TestDatabaseHelper.CreateTestDatabaseAsync(); + + using (var connection = new SqlConnection(TestDatabaseHelper.GetConnectionString())) + { + await connection.OpenAsync(TestContext.Current.CancellationToken); + using var cmd = new SqlCommand(@" + USE PerformanceMonitor_Test; + IF SCHEMA_ID(N'collect') IS NULL + BEGIN + EXECUTE(N'CREATE SCHEMA collect;'); + END; + IF OBJECT_ID(N'collect.wait_stats', N'U') IS NULL + BEGIN + CREATE TABLE collect.wait_stats (id integer NOT NULL); + END;", connection); + await cmd.ExecuteNonQueryAsync(TestContext.Current.CancellationToken); + } + + var version = await GetInstalledVersionFromTestDbAsync(); + Assert.Equal(InstallationService.UnknownVersionSentinel, version); + } + [Fact] public async Task DatabaseExists_EmptyHistoryTable_ReturnsFallback_Regression538() { // This is the #538 regression test. // When installation_history exists but has NO SUCCESS rows, - // the installer must return "1.0.0" (not null), so it attempts + // the installer must return the unknown sentinel (not null), so it attempts // upgrades rather than treating the existing database as a fresh install. await TestDatabaseHelper.CreateInstallationWithNoSuccessRowsAsync(); var version = await GetInstalledVersionFromTestDbAsync(); - Assert.Equal("1.0.0", version); + Assert.Equal(InstallationService.UnknownVersionSentinel, version); } [Fact] @@ -71,7 +103,7 @@ public async Task DatabaseExists_OnlyFailedRows_ReturnsFallback() await TestDatabaseHelper.CreateInstallationWithOnlyFailedRowsAsync(); var version = await GetInstalledVersionFromTestDbAsync(); - Assert.Equal("1.0.0", version); + Assert.Equal(InstallationService.UnknownVersionSentinel, version); } [Fact] @@ -96,8 +128,13 @@ INSERT INTO config.installation_history } /// - /// Replicates the same SQL logic as InstallationService.GetInstalledVersionAsync - /// but queries PerformanceMonitor_Test instead of the hardcoded PerformanceMonitor. + /// Gathers the same FACTS as InstallationService.GetInstalledVersionAsync against the test database + /// (production hardcodes the "PerformanceMonitor" name), then hands them to the SAME decision — + /// InstalledVersionClassifier — that production uses. + /// + /// The decision is deliberately not re-implemented here. It used to be, and the copy had already + /// drifted: only the queries are test-local now, so a change to what the facts MEAN cannot pass this + /// suite while breaking production. /// private static async Task GetInstalledVersionFromTestDbAsync() { @@ -108,32 +145,66 @@ INSERT INTO config.installation_history using var connection = new SqlConnection(TestDatabaseHelper.GetConnectionString()); await connection.OpenAsync(TestContext.Current.CancellationToken); - // Check if database exists + // Does the database exist? using var dbCheckCmd = new SqlCommand($@" SELECT database_id FROM sys.databases WHERE name = N'{testDbName}';", connection); var dbExists = await dbCheckCmd.ExecuteScalarAsync(TestContext.Current.CancellationToken); - if (dbExists == null || dbExists == DBNull.Value) - return null; + bool databaseExists = dbExists != null && dbExists != DBNull.Value; + + if (!databaseExists) + { + return InstalledVersionClassifier.Classify(false, false, 0L, null); + } - // Check if installation_history table exists + // Does config.installation_history exist? using var tableCheckCmd = new SqlCommand($@" - SELECT OBJECT_ID(N'{testDbName}.config.installation_history', N'U');", connection); - var tableExists = await tableCheckCmd.ExecuteScalarAsync(TestContext.Current.CancellationToken); - if (tableExists == null || tableExists == DBNull.Value) - return null; - - // Get most recent successful version - using var versionCmd = new SqlCommand($@" - SELECT TOP 1 installer_version - FROM {testDbName}.config.installation_history - WHERE installation_status = 'SUCCESS' - ORDER BY installation_date DESC;", connection); - var version = await versionCmd.ExecuteScalarAsync(TestContext.Current.CancellationToken); - if (version != null && version != DBNull.Value) - return version.ToString(); - - // Fallback: database + table exist but no SUCCESS rows → return "1.0.0" - return "1.0.0"; + USE {testDbName}; + SELECT OBJECT_ID(N'config.installation_history', N'U');", connection); + var tableOid = await tableCheckCmd.ExecuteScalarAsync(TestContext.Current.CancellationToken); + bool historyTableExists = tableOid != null && tableOid != DBNull.Value; + + // How many collect objects? (Tells a ledger-lost install apart from an empty database.) + long collectTableCount = 0L; + if (!historyTableExists) + { + using var collectCheckCmd = new SqlCommand($@" + SELECT + COUNT_BIG(*) + FROM {testDbName}.sys.tables AS t + INNER JOIN {testDbName}.sys.schemas AS s + ON s.schema_id = t.schema_id + WHERE s.name = N'collect';", connection); + + var collectTables = await collectCheckCmd.ExecuteScalarAsync(TestContext.Current.CancellationToken); + collectTableCount = + collectTables == null || collectTables == DBNull.Value + ? 0L + : Convert.ToInt64(collectTables); + } + + // Most recent SUCCESS row, if any. + string? latestSuccessVersion = null; + if (historyTableExists) + { + using var versionCmd = new SqlCommand($@" + SELECT TOP (1) + installer_version + FROM {testDbName}.config.installation_history + WHERE installation_status = 'SUCCESS' + ORDER BY installation_date DESC;", connection); + + var version = await versionCmd.ExecuteScalarAsync(TestContext.Current.CancellationToken); + if (version != null && version != DBNull.Value) + { + latestSuccessVersion = version.ToString(); + } + } + + return InstalledVersionClassifier.Classify( + databaseExists, + historyTableExists, + collectTableCount, + latestSuccessVersion); } catch { diff --git a/Installer/Program.cs b/Installer/Program.cs index eec2ec5377..dfe0b36536 100644 --- a/Installer/Program.cs +++ b/Installer/Program.cs @@ -20,9 +20,33 @@ namespace PerformanceMonitorInstaller { class Program { + /* + The unknown sentinel PARSES, so interpolating it printed "Existing installation detected: v0.0.0" + -- a guess stated as a fact, and one the same run then contradicts a few lines later with "this + server's recorded version could not be read". It is not a version; do not print it as one. + */ + static string DescribeInstalledVersion(string? version) => + RepairOutcome.IsVersionUnknown(version) + ? "Existing installation detected, but its recorded version could not be read." + : $"Existing installation detected: v{version}"; + static async Task Main(string[] args) { - var version = Assembly.GetExecutingAssembly().GetName().Version?.ToString() ?? "Unknown"; + /* + GetName().Version is never null for a loaded assembly, so the old "?? Unknown" could not fire: + a build with no AssemblyVersion reports 0.0.0.0, which PARSES -- InstallGuard waves it through, + and a fresh install writes it into installation_history as the version of record. It then reads + back as version 0.0.0, which is UnknownVersionSentinel, so every later run takes that server's + real version as unreadable and replays every migration. A version-less build must land on + something UNPARSEABLE so InstallGuard blocks it (UnreadableBuildVersion). Same hole, same fix, + as the Dashboard's GetAppVersion -- the two surfaces write to the same ledger. + */ + var asmVersion = Assembly.GetExecutingAssembly().GetName().Version; + var version = + asmVersion != null && (asmVersion.Major | asmVersion.Minor | asmVersion.Build) != 0 + ? asmVersion.ToString() + : "Unknown"; + var infoVersion = Assembly.GetExecutingAssembly() .GetCustomAttribute()?.InformationalVersion ?? version; @@ -44,6 +68,9 @@ static async Task Main(string[] args) Options: --reinstall Drop existing database and perform clean install + --repair Reinstall schema objects without running upgrade scripts. Use when an + upgrade fails on a missing or damaged object; non-destructive, and the + pending upgrade still needs to run afterwards --encrypt=X Connection encryption: mandatory (default), optional, strict --trust-cert Trust server certificate without validation (default: require valid cert) --data-path DIR Server-side directory for the data (.mdf) file (first install only) @@ -64,6 +91,7 @@ static async Task Main(string[] args) Console.WriteLine("Options:"); Console.WriteLine(" -h, --help Show this help message"); Console.WriteLine(" --reinstall Drop existing database and perform clean install"); + Console.WriteLine(" --repair Reinstall schema objects, skipping upgrade scripts (non-destructive)"); Console.WriteLine(" --uninstall Remove database, Agent jobs, and XE sessions"); Console.WriteLine(" --reset-schedule Reset collection schedule to recommended defaults"); Console.WriteLine(" --troubleshoot Run installation diagnostics (99_installer_troubleshooting.sql)"); @@ -96,7 +124,26 @@ static async Task Main(string[] args) bool automatedMode = args.Length > 0; bool reinstallMode = args.Any(a => a.Equals("--reinstall", StringComparison.OrdinalIgnoreCase)); + bool repairMode = args.Any(a => a.Equals("--repair", StringComparison.OrdinalIgnoreCase)); bool uninstallMode = args.Any(a => a.Equals("--uninstall", StringComparison.OrdinalIgnoreCase)); + + /* + --repair means "restore the objects, destroy nothing". Pairing it with a mode that drops the + database is a contradiction, and the destructive mode would otherwise win silently: --uninstall + is dispatched further down BEFORE any repair logic, and in automated mode it skips its own + confirmation. So both destructive companions are rejected here, not just --reinstall -- the + --uninstall gap was the parity hole in only guarding one of them. + */ + if (repairMode && reinstallMode) + { + WriteError("--repair and --reinstall are mutually exclusive: --reinstall drops the database, leaving nothing to repair."); + return (int)InstallationResultCode.InvalidArguments; + } + if (repairMode && uninstallMode) + { + WriteError("--repair and --uninstall are mutually exclusive: --uninstall drops the database, leaving nothing to repair."); + return (int)InstallationResultCode.InvalidArguments; + } bool resetSchedule = args.Any(a => a.Equals("--reset-schedule", StringComparison.OrdinalIgnoreCase)); bool troubleshootMode = args.Any(a => a.Equals("--troubleshoot", StringComparison.OrdinalIgnoreCase)); bool trustCert = args.Any(a => a.Equals("--trust-cert", StringComparison.OrdinalIgnoreCase)); @@ -318,6 +365,20 @@ a client id (--managed-identity= or --managed-identity ) = user-assigned { Console.WriteLine("Error: Password is required for SQL Server Authentication."); Console.WriteLine("Provide password as third argument or set PM_SQL_PASSWORD environment variable."); + + /* + The common trap: a password that begins with "--" (e.g. "--h7!x") is indistinguishable + from a flag, so the positional filter above dropped it and we landed here reporting + "no password" for a password that WAS supplied. Only say so when a "--"-token that is + not a recognized flag actually appeared -- otherwise this is just a forgotten password. + */ + if (HasUnrecognizedDoubleDashArg(args)) + { + Console.WriteLine(); + Console.WriteLine("Note: an argument beginning with \"--\" was treated as a flag and ignored. A password"); + Console.WriteLine(" cannot be passed on the command line if it starts with \"--\"; set PM_SQL_PASSWORD."); + } + return (int)InstallationResultCode.InvalidArguments; } @@ -612,6 +673,8 @@ Main installation loop - allows retry on failure bool installationSuccessful = false; bool retry; DateTime installationStartTime = DateTime.Now; + /* Declared out here so the history write below can tell what version the database is still at. */ + string? currentVersion = null; do { retry = false; @@ -622,6 +685,8 @@ Main installation loop - allows retry on failure installationErrors.Clear(); installationSuccessful = false; installationStartTime = DateTime.Now; + /* Reset with its siblings so a clean-install iteration can never reuse the prior one's version. */ + currentVersion = null; /* Ask about clean install (automated mode preserves database unless --reinstall flag is used) @@ -648,6 +713,26 @@ Ask about clean install (automated mode preserves database unless --reinstall fl dropExisting = cleanInstall?.Trim().Equals("Y", StringComparison.OrdinalIgnoreCase) ?? false; } + /* + Validate this binary's OWN version BEFORE the clean-install branch, not inside the upgrade + one. It is what a FRESH install writes to installation_history.installer_version, so + letting an unparseable value through poisons a brand-new server's ledger at birth -- after + which both this installer and the Dashboard refuse to touch it ever again. --reinstall IS + a fresh install, so scoping this to the upgrade path missed the exact case it names. + */ + if (ScriptProvider.TryParseVersionCore(version) == null) + { + Console.WriteLine(); + WriteError($"This installer reports its own version as '{version}', which is not a valid version."); + Console.WriteLine("Aborting: an install would record that value as the server's version."); + Console.WriteLine("This is a build problem, not a problem with the server."); + if (!automatedMode) + { + WaitForExit(); + } + return (int)InstallationResultCode.VersionCheckFailed; + } + if (dropExisting) { Console.WriteLine(); @@ -682,7 +767,6 @@ Ask about clean install (automated mode preserves database unless --reinstall fl /* Upgrade mode - check for existing installation and apply upgrades */ - string? currentVersion = null; try { currentVersion = await InstallationService.GetInstalledVersionAsync(connectionString, throwOnError: true).ConfigureAwait(false); @@ -714,10 +798,100 @@ Ask about clean install (automated mode preserves database unless --reinstall fl return (int)InstallationResultCode.VersionCheckFailed; } - if (currentVersion != null) + /* + Same decision the Dashboard makes -- shared via InstallGuard so the two cannot drift, and + pinned by tests that actually run in CI. --repair skips ExecuteAllUpgradesAsync, which is + the only thing that would otherwise parse the recorded version before writing it straight + back, and the newer-than-us case produces zero hops AND zero failures, so nothing + downstream catches it. Repair is no exception -- it runs the same install scripts. + */ + switch (InstallGuard.Check(currentVersion, version)) + { + case InstallBlock.UnreadableBuildVersion: + /* Also checked above, before the clean-install branch. Handled here so the switch + is exhaustive and cannot silently fall through if that check ever moves. */ + Console.WriteLine(); + WriteError($"This installer reports its own version as '{version}', which is not a valid version."); + Console.WriteLine("Aborting: an install would record that value as the server's version."); + if (!automatedMode) + { + WaitForExit(); + } + return (int)InstallationResultCode.VersionCheckFailed; + + case InstallBlock.UnreadableInstalledVersion: + Console.WriteLine(); + WriteError($"The version recorded on this server ('{currentVersion}') is not a valid version."); + Console.WriteLine("Aborting: without a comparable version we cannot tell which migrations still need to run."); + if (!automatedMode) + { + WaitForExit(); + } + return (int)InstallationResultCode.VersionCheckFailed; + + case InstallBlock.InstalledIsNewerThanBuild: + Console.WriteLine(); + WriteError($"Installed version v{currentVersion} is newer than this installer (v{version})."); + Console.WriteLine("Aborting: running an older installer over a newer database would revert its objects to"); + Console.WriteLine("the older definitions and record it at the lower version. Use a matching or newer installer."); + if (!automatedMode) + { + WaitForExit(); + } + return (int)InstallationResultCode.VersionCheckFailed; + + case InstallBlock.None: + break; + + default: + /* + Never default to "safe to install". A new InstallBlock member silently becoming an + allow is the one failure mode this whole guard exists to prevent. + */ + Console.WriteLine(); + WriteError($"Unhandled install-guard result. Aborting rather than assuming it is safe."); + if (!automatedMode) + { + WaitForExit(); + } + return (int)InstallationResultCode.VersionCheckFailed; + } + + /* + Refuse to repair what is not there. Falling through would run a FULL fresh install and + stamp the target version -- a whole new database and Agent jobs on a mistyped server, and, + if the database exists but its history table does not, a target-version stamp that strands + every migration in between. An operator who typed --repair asked for the opposite. + */ + if (repairMode && currentVersion == null) { Console.WriteLine(); - Console.WriteLine($"Existing installation detected: v{currentVersion}"); + WriteError("--repair found no existing PerformanceMonitor installation to repair on this server."); + Console.WriteLine("Repair reinstalls the objects of an existing installation; it will not create one."); + Console.WriteLine("Re-run without --repair to install, or check the server name."); + if (!automatedMode) + { + WaitForExit(); + } + return (int)InstallationResultCode.VersionCheckFailed; + } + + if (currentVersion != null && repairMode) + { + /* + Repair reinstalls the schema objects (install scripts are idempotent) without + running migrations, so a hop that failed on a missing or damaged object can be + recovered without dropping the database. The pending upgrade runs afterwards. + */ + Console.WriteLine(); + Console.WriteLine(DescribeInstalledVersion(currentVersion)); + Console.WriteLine("Repair mode: skipping upgrade scripts. Objects will be reinstalled at their"); + Console.WriteLine("current definitions; the pending upgrade still needs to run afterwards."); + } + else if (currentVersion != null) + { + Console.WriteLine(); + Console.WriteLine(DescribeInstalledVersion(currentVersion)); Console.WriteLine("Checking for applicable upgrades..."); var (upgSuccessCount, upgFailureCount, upgradeCount) = @@ -735,26 +909,31 @@ await InstallationService.ExecuteAllUpgradesAsync( { Console.WriteLine(); Console.WriteLine($"Upgrades complete: {upgradeSuccessCount} succeeded, {upgradeFailureCount} failed"); - - /*Abort if any upgrade scripts failed -- proceeding would reinstall over a partially-upgraded database*/ - if (upgradeFailureCount > 0) - { - Console.WriteLine(); - Console.WriteLine("================================================================================"); - WriteError("Installation aborted: upgrade scripts must succeed before installation can proceed."); - Console.WriteLine("Fix the errors above and re-run the installer."); - Console.WriteLine("================================================================================"); - if (!automatedMode) - { - WaitForExit(); - } - return (int)InstallationResultCode.UpgradesFailed; - } } - else + else if (upgradeFailureCount == 0) { Console.WriteLine("No pending upgrades found."); } + + /* + Abort if any upgrade failed -- proceeding would reinstall over a partially-upgraded + database. Checked outside the upgradeCount block because discovery itself can fail + before any hop runs (an unreadable installed version reports a failure with an + upgrade count of zero), and that must not read as "no pending upgrades". + */ + if (upgradeFailureCount > 0) + { + Console.WriteLine(); + Console.WriteLine("================================================================================"); + WriteError("Installation aborted: upgrade scripts must succeed before installation can proceed."); + Console.WriteLine("Fix the errors above and re-run the installer."); + Console.WriteLine("================================================================================"); + if (!automatedMode) + { + WaitForExit(); + } + return (int)InstallationResultCode.UpgradesFailed; + } } else { @@ -964,26 +1143,74 @@ Calculate totals and determine success */ totalSuccessCount = upgradeSuccessCount + installSuccessCount; totalFailureCount = upgradeFailureCount + installFailureCount; - installationSuccessful = totalFailureCount == 0; + + /* + A repair on a database with pending migrations is EXPECTED to fail some install files, and + that is not a failed repair. The install scripts compile against the CURRENT schema -- e.g. + install/23_process_blocked_process_xml.sql reads collect.blocking_BlockedProcessReport.monitor_loop, + a column the 3.0.0-to-3.1.0 migration adds -- and ALTER PROCEDURE binds columns at compile + time, so those procedures cannot compile until the upgrade runs (Msg 207). A failed + CREATE OR ALTER leaves the old body intact, so nothing is damaged, and the upgrade's own + install pass recompiles them. + + Reporting that as PartialInstallation (exit 4) is actively dangerous: a script gating on + %ERRORLEVEL% treats a good repair as a failure, and the operator reading "repair failed" + reaches for --reinstall, which DROPS the database -- the exact destructive outcome this + feature exists to avoid. A critical file (01_/02_/03_) failing is different: that aborts the + whole pass, so the repair reinstalled nothing and really did fail. + */ + bool repairRan = repairMode && currentVersion != null; + + /* + Shared with the Dashboard so the two cannot drift -- see RepairOutcome. A critical file + failing already returned CriticalScriptFailed above, so it cannot reach here; pass false. + */ + bool repairFailuresExcused = RepairOutcome.FailuresAreExpected( + repairRan, + currentVersion, + version, + criticalFileFailed: false); + + installationSuccessful = totalFailureCount == 0 || repairFailuresExcused; /* Log installation history to database */ - try + /* + A repair reinstalls objects without running migrations, so it must NOT record the target + version -- that would strand every pending hop, which is exactly what the upgrade abort + above exists to prevent. + + It writes NO history row at all. A repair changes no version, and installation_history is + the version ledger -- echoing back a version we merely READ is how a guess becomes a fact. + Concretely: GetInstalledVersionAsync returns the unknown sentinel as a #538 fallback when the database + exists but has no SUCCESS row, meaning "unknown, try every upgrade". Persisting that as a + SUCCESS row would turn the guess into truth. Writing nothing leaves the previous row as the + version of record, so the pending upgrade is still offered afterwards. + */ + if (repairMode && currentVersion != null) { - await InstallationService.LogInstallationHistoryAsync( - connectionString, - version, - infoVersion, - installationStartTime, - totalSuccessCount, - totalFailureCount, - installationSuccessful - ).ConfigureAwait(false); + Console.WriteLine(); + Console.WriteLine("Repair does not change the recorded version, so no installation history row was written."); } - catch (Exception ex) + else { - Console.WriteLine($"Warning: Could not log installation history: {ex.Message}"); + try + { + await InstallationService.LogInstallationHistoryAsync( + connectionString, + version, + infoVersion, + installationStartTime, + totalSuccessCount, + totalFailureCount, + installationSuccessful + ).ConfigureAwait(false); + } + catch (Exception ex) + { + Console.WriteLine($"Warning: Could not log installation history: {ex.Message}"); + } } Console.WriteLine(); @@ -991,7 +1218,25 @@ await InstallationService.LogInstallationHistoryAsync( Console.WriteLine("Installation Summary"); Console.WriteLine("================================================================================"); - if (installationSuccessful) + if (repairFailuresExcused && totalFailureCount > 0) + { + /* + A repair with a pending upgrade exits 0 -- but it did NOT install cleanly, and saying + "Installation completed successfully! / All collector stored procedures" over N procedures + that demonstrably failed to compile is a lie the very next paragraph contradicts. The exit + code is the contract for scripts; the banner is for the human, and the human needs the truth. + + Gated on there BEING failures: repairFailuresExcused only means "if it failed, that is + expected", so a clean repair is still a clean install and gets the ordinary banner. + */ + WriteWarning($"Repair completed with {totalFailureCount} expected error(s)."); + Console.WriteLine(); + Console.WriteLine("Those errors are expected: the install scripts compile against the CURRENT"); + Console.WriteLine("schema, and this server's pending upgrade has not run yet, so a few procedures"); + Console.WriteLine("cannot compile until it does. A failed CREATE OR ALTER leaves the previous"); + Console.WriteLine("definition intact -- nothing was damaged, and the upgrade recompiles them."); + } + else if (installationSuccessful) { WriteSuccess("Installation completed successfully!"); Console.WriteLine(); @@ -1017,6 +1262,58 @@ await InstallationService.LogInstallationHistoryAsync( Console.WriteLine("Review errors above and check PerformanceMonitor.config.collection_log for details."); } + /* + The repair handoff. Without it the operator is told "N error(s), review errors above" with no + next step and reaches for --reinstall, which drops the database. + */ + if (repairRan) + { + /* + Keyed on HasPendingUpgrade, NOT on FailuresAreExpected. They answer different questions, + and they disagree for the unknown sentinel: an unreadable version cannot excuse a file + failure, but it absolutely can have every hop pending. Reusing the one flag printed + "already at the current version, so there is no upgrade to apply" two lines under + "still at v1.0.0" -- for a server with eleven migrations waiting. + */ + bool versionUnknown = RepairOutcome.IsVersionUnknown(currentVersion); + bool pendingUpgrade = RepairOutcome.HasPendingUpgrade(currentVersion, version); + + Console.WriteLine(); + Console.WriteLine("================================================================================"); + + if (versionUnknown) + { + Console.WriteLine("Repair complete. No version was recorded."); + Console.WriteLine("This server's recorded version could not be read, so it is unknown which migrations"); + Console.WriteLine("have run."); + if (totalFailureCount > 0) + { + Console.WriteLine($"{totalFailureCount} object(s) failed. Some may simply be waiting on a migration."); + } + Console.WriteLine(); + Console.WriteLine("Next: re-run WITHOUT --repair to attempt every upgrade, then re-check."); + } + else if (pendingUpgrade) + { + Console.WriteLine($"Repair complete. This server is still at v{currentVersion} and no version was recorded."); + if (totalFailureCount > 0) + { + Console.WriteLine($"{totalFailureCount} object(s) could not be compiled because the pending upgrade has not"); + Console.WriteLine("run yet. This is expected -- they reference columns the upgrade adds, and the"); + Console.WriteLine("upgrade will recompile them."); + } + Console.WriteLine(); + Console.WriteLine("Next: re-run WITHOUT --repair to apply the pending upgrade."); + } + else + { + Console.WriteLine($"Repair complete. This server is at v{currentVersion} and no version was recorded."); + Console.WriteLine("It was already at the current version, so there is no upgrade to apply."); + } + + Console.WriteLine("================================================================================"); + } + /* Ask if user wants to retry or exit (skip in automated mode) */ @@ -1339,6 +1636,40 @@ private static string SanitizeFilename(string input) return string.Concat(input.Select(c => invalid.Contains(c) ? '_' : c)); } + /* + The flags the positional-argument filter knows to skip. Kept only to DIAGNOSE the dropped-password + trap: a "--"-token that is not one of these was almost certainly a value the filter silently ate. + If a new flag is added and not listed here, the only cost is a spurious hint in an error path -- so + this is a diagnostic aid, not a parsing authority (the actual flag handling matches each flag by + name inline in Main). + */ + private static readonly string[] RecognizedFlags = + { + "--data-path", "--encrypt", "--entra", "--help", "--log-path", "--managed-identity", + "--reinstall", "--repair", "--reset-schedule", "--service-principal", "--troubleshoot", + "--trust-cert", "--uninstall" + }; + + private static bool HasUnrecognizedDoubleDashArg(string[] args) + { + foreach (string arg in args) + { + if (!arg.StartsWith("--", StringComparison.Ordinal)) + { + continue; + } + + /* Compare the flag name only, so the --flag=value form is matched too. */ + string flagName = arg.Split('=')[0]; + if (!RecognizedFlags.Contains(flagName, StringComparer.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; + } + /* Wait for user input before exiting (prevents window from closing) Used for fatal errors where retry doesn't make sense