An edit keeps its server identity, so history stays attached (#2158)

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- **Editing a server's address keeps its identity, so its collected history stays attached** ([#2158]) - the Add/Edit save re-derived `server_id` from host/database/read-only-intent on EVERY save, so fixing a hostname typo wrote a row under a new id and deleted the old one. That left the registry tidy and every `collect.*` row keyed to the old id orphaned with nothing pointing at it, which is why it went unnoticed: nothing looks broken, the server simply reads as though it had never been monitored. An edit now keeps the row's identity and rewrites the address in place; derivation runs only on Add, where there is no history to lose. **Why assigned rather than derived, argued from consequences rather than preference**: three issues pull on this identity and they pull in opposite directions. This one says a config edit must not change it. [#2228] says two different configs that resolve to the same real database must not be two identities - which no config-derived hash can decide, because the configs genuinely differ. [#2218] says two instances on one host need distinguishing, which wants MORE fields in the derivation and so makes this bug strictly worse. Only one shape satisfies all three: identity is allocated once and never recomputed, the derived address is a lookup key rather than the identity, and what the target actually IS comes from the connection. The groundwork was already in - `StoredServerId` made the store authoritative and `DarlingServerConnector` reads `config.ServerId` rather than re-hashing - so the Viewer's save was the last place identity was recomputed. **Two things the fix had to carry with it.** The collision guard used to look a derived id up in the registry, which only works while every row's id equals the hash of its own address; left alone, an edit could point a second registration at an address another server already monitors and the guard would never fire, because the id it looked up belongs to nobody. It now matches on the ADDRESS columns (`IS NOT DISTINCT FROM` for the nullable database, since `=` never matches NULL and every server-scoped registration would otherwise read as "address free") and compares ids afterwards, which is what still lets a rename or a credential change through. And the darling.json reconcile matched on id alone, so after an edit it would report a server that IS monitored as absent and advise re-adding it - wrong advice, on every start, about the one server the operator had just fixed; it now matches id OR name, either-or rather than name-only so a same-named sibling cannot hide a genuinely unmonitored server.
+
 - **The database-state heal tests no longer assume a best-effort maintenance cycle always runs** ([#2266] item 2, root-caused from source) - `RebaselinedByHandDuringAnOutage_HealsOnceTheDatabaseRecovers` failed on a PR whose diff could not reach it and passed on a re-run of the same commit, reporting `ExpectedState = SUSPECT` alongside `StateDesc = ONLINE`. That pair is only reachable one way: `GetDatabaseStateDeviationsAsync` performs its seeding, the [#2189] heal, the [#2203] forget and the prune inside a block that opens the write connection with a **5-second** lock acquisition and, on `TimeoutException`, skips the entire maintenance block while still running the deviation read - which is deliberate and documented, because skipping is the only lossless option when archival holds the lock. That write lock is **static, shared by the whole process**, and xunit runs test classes in parallel, so another class can hold it long enough for a cycle to skip its maintenance. The test was therefore asserting that the heal lands in ONE cycle, which the design does not promise; it now sweeps until the expectation settles, bounded, which is the actual contract. It cannot mask a regression: a genuinely broken heal never settles, every cycle runs, and the caller's own assertion fails on the final result with its own message exactly as before. Ruled out first: the fixture mints a unique temp directory per instance so no two classes share a store file, and the server id is used by no other class - the shared resource is the LOCK, not the data. **Not fixed here, and worth knowing**: that `catch (TimeoutException)` logs nothing, so in production a sustained contention window means baselines quietly stop being seeded and healed with no evidence anywhere, which matters because [#2189] exists precisely because an unhealed baseline inverts the alert permanently. Item 1 of that issue (the sub-second scale-test comparison) is deliberately untouched: every candidate fix needs the jitter distribution, and guessing a threshold is how an intermittent test stops looking broken without becoming correct.
 
 - **`get_top_queries_by_cpu` can rank a procedure's dynamic SQL as ONE statement: `group_by: "host_object"`** ([#2235]) - `query_hash` is a SHAPE hash, so dynamic SQL built with per-value literals fragments one logical statement across as many hashes as there are literal sets. Measured on `prod-pos-use2-apex-01`: **21 hashes** for a single `API.GetInventoryWithLabsV5` `insert #result` statement, whose fragments were **58-65% of the instance's worker_time** in every window sampled - while the hash never entered the 168-hour top 20, and the per-query ranking as a whole accounted for roughly a **tenth** of the box's CPU. A top-N-by-hash list cannot surface that no matter how large N is, and nothing in the output said so. Setting `group_by: "host_object"` collapses every statement of a hosting procedure or function into one row, which is what makes the real consumer rank first; `distinct_query_hashes` reports how many hashes the row rolled up (21, in the reported case) and is the number that explains why the default ranking missed it, with a `rollup_note` saying so in words. `query_hash` and `query_text` in a rolled-up row are one representative fragment, exactly as `query_text` already is when `distinct_texts > 1`. **Ad-hoc statements keep their per-hash grouping in BOTH modes, and that is the load-bearing part**: ad-hoc rows carry `host_object_name = NULL`, so a bare `GROUP BY host_object_name` would pool every unrelated ad-hoc statement in a database into one meaningless row - a worse attribution bug than the one being fixed - and the representative-text lookup would start serving an unrelated statement's text. The grouping key keys those rows on their own `query_hash` instead, identical to the default read. The per-hash grouping ([#2012] stage 2) stays the DEFAULT and is unchanged: two procedures sharing a hash genuinely are different work, which is why that split exists, so this is an additional lens rather than a replacement. Implemented as a sibling SQL const rather than a built clause because Postgres cannot parameterize `GROUP BY` and every read here is a public const so the suite can pin its dialect without a live store; an unrecognised `group_by` is rejected rather than silently falling back, since a caller who asked for a rollup and got a per-hash ranking would read it as "this procedure is not hot", which is the exact wrong conclusion. `group_by` is trailing and optional, so Lite-shaped calls are unaffected, and it is deliberately absent from `get_top_procedures_by_cpu` (already keyed on the object) and `get_query_store_top` (keys on `query_id`, which does not fragment). Pinned by a live-Postgres test that asserts the collapse, the ad-hoc non-pooling, per-row text correctness, and that total CPU and executions are CONSERVED across both groupings - a rollup must redistribute attribution, never invent or lose it.
@@ -2725,6 +2727,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2190]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2190
 [#2186]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2186
 [#2185]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2185
+[#2228]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2228
+[#2218]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2218
 [#2266]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2266
 [#2235]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2235
 [#2165]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2165

Darling/Darling.Tests/ServerIdentitySurvivesAnEditTests.cs

@@ -0,0 +1,185 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Runtime.CompilerServices;
+using PerformanceMonitor.Darling.Service;
+using PerformanceMonitor.Darling.Viewer;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #2158: editing a server's address keeps its identity, so its collected history stays attached to it.
+///
+/// <para><b>The defect.</b> The Add/Edit save re-derived <c>server_id</c> from host/database/read-only-intent on
+/// every save. So an operator fixing a hostname typo produced a row under a NEW id and the old row was deleted —
+/// which left the registry tidy and every <c>collect.*</c> row keyed to the old id orphaned, with nothing
+/// pointing at it. The visible symptom is a server that reads as though it had never been monitored, which is
+/// why it went unnoticed: nothing looks broken, the history is simply gone.</para>
+///
+/// <para><b>Why identity must be assigned rather than derived, argued from consequences.</b> Three issues pull
+/// on this and they pull in opposite directions. #2158 says a config edit must NOT change the identity.
+/// #2228 says two different configs resolving to one real database must not be two identities — which no
+/// config-derived hash can decide, because the configs genuinely differ. #2218 says two instances on one host
+/// need distinguishing, which wants MORE fields in the derivation and therefore makes #2158 strictly worse.
+/// Only one shape satisfies all three: the identity is allocated once and never recomputed, the derived address
+/// is a lookup key rather than the identity, and what the target actually IS comes from the connection. This
+/// file pins the first of those three.</para>
+/// </summary>
+public sealed class ServerIdentitySurvivesAnEditTests
+{
+    /// <summary>
+    /// THE FIX, pinned at the source: the row builder prefers the row's existing identity and only derives one
+    /// when there is none (an Add).
+    ///
+    /// <para>Pinned textually because the alternative is a WPF dialog — reproducing it behaviourally means
+    /// standing up <c>AddServerDialog</c> with a live store and a real edit, which the suite cannot do. A
+    /// re-derivation here is invisible in every other test and silently discards history, so it is worth
+    /// holding at the only level available.</para>
+    /// </summary>
+    [Fact]
+    public void TheEditSaveKeepsTheOriginalIdentity_AndOnlyAddDerivesOne()
+    {
+        var source = ReadDialogSource();
+
+        Assert.Contains(
+            "ServerId = _originalServerId ?? ViewerDataService.ComputeServerId(host, database, readOnlyIntent),",
+            source, StringComparison.Ordinal);
+
+        /* And the delete-the-old-identity step is GONE: with the id preserved there is no second row to clean
+           up, and leaving the delete in would drop the row that was just written. */
+        Assert.DoesNotContain("await _dataService.DeleteMonitoredServerAsync(original);", source, StringComparison.Ordinal);
+    }
+
+    /// <summary>
+    /// The collision guard survived the change and now asks about the ADDRESS.
+    ///
+    /// <para>This is the half that would have been easy to lose. The old guard compared a derived id against
+    /// the registry, which only works while every row's id equals the hash of its own address — precisely the
+    /// invariant this change gives up. Left as it was, an edit could point a second registration at an address
+    /// another server already monitors and the guard would never fire, because the derived id it looked up
+    /// belongs to nobody. That is #2228's shape reached from the registry side.</para>
+    /// </summary>
+    [Fact]
+    public void TheCollisionGuardChecksTheAddress_NotADerivedId()
+    {
+        var source = ReadDialogSource();
+
+        Assert.Contains("GetMonitoredServerByAddressAsync(row.Host, row.Database, row.ReadOnlyIntent)", source, StringComparison.Ordinal);
+        /* Compared by id afterwards, which is what excludes "collided with myself" on an edit that leaves the
+           address alone (a rename, or new credentials). */
+        Assert.Contains("occupant.ServerId != row.ServerId", source, StringComparison.Ordinal);
+        Assert.DoesNotContain("await _dataService.GetMonitoredServerAsync(row.ServerId) is not null", source, StringComparison.Ordinal);
+    }
+
+    /// <summary>
+    /// The address lookup matches a NULL database with <c>IS NOT DISTINCT FROM</c>. A plain <c>=</c> never
+    /// matches NULL in SQL, so every server-scoped registration — the common case — would read as "address
+    /// free" and the guard would pass for all of them.
+    /// </summary>
+    [Fact]
+    public void TheAddressLookupMatchesANullDatabase()
+    {
+        var sql = ViewerDataService.MonitoredServerByAddressSql;
+
+        Assert.Contains("WHERE host = $1", sql, StringComparison.Ordinal);
+        Assert.Contains("database IS NOT DISTINCT FROM $2", sql, StringComparison.Ordinal);
+        Assert.Contains("read_only_intent = $3", sql, StringComparison.Ordinal);
+        /* Secret-free, so a read-only seat gets an answer rather than 42501 on the column it is denied. */
+        Assert.DoesNotContain("encrypted_password", sql, StringComparison.Ordinal);
+    }
+
+    /// <summary>
+    /// The file-vs-store reconcile no longer calls an edited server "not monitored".
+    ///
+    /// <para>That log line gives ADVICE — "add them with the Viewer's Add Server dialog" — so being wrong is
+    /// worse than being silent: after an address edit the file's derived id matches nothing, and an id-only
+    /// comparison would tell the operator to re-add the one server they had just fixed, on every start.</para>
+    /// </summary>
+    [Fact]
+    public void AnEditedServerIsNotReportedAsFileOnly()
+    {
+        var file = new[] { Server("prod-01", host: "prod-01.old.example.com") };
+
+        /* The store row kept its identity through the edit, so its id is NOT the hash of the file's address. */
+        var storeIds = new HashSet<int> { 999_111 };
+        var storeNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "prod-01" };
+
+        Assert.Empty(StoreConfigProvider.ServersOnlyInFile(file, storeIds, storeNames));
+
+        /* Without the name arm this is exactly the wrong answer the old comparison gave. */
+        Assert.Single(StoreConfigProvider.ServersOnlyInFile(file, storeIds, new HashSet<string>()));
+    }
+
+    /// <summary>
+    /// A genuinely removed server is STILL reported. The Viewer's Remove hard-deletes the row, so it is absent
+    /// under both keys — the name arm must not turn this log line off altogether, which is the obvious way to
+    /// over-apply the fix.
+    /// </summary>
+    [Fact]
+    public void ADeletedServerIsStillReportedAsFileOnly()
+    {
+        var file = new[] { Server("gone-01", host: "gone-01.example.com") };
+
+        var missing = StoreConfigProvider.ServersOnlyInFile(
+            file, new HashSet<int> { 999_111 }, new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "someone-else" });
+
+        Assert.Equal(new[] { "gone-01" }, missing);
+    }
+
+    /// <summary>
+    /// Matching is either-or, not name-only. Nothing enforces display-name uniqueness, so a name-only
+    /// comparison would hide a genuinely unmonitored server behind a same-named sibling; and an id match alone
+    /// must still be enough, which is the path every unedited server takes.
+    /// </summary>
+    [Fact]
+    public void AnIdMatchAloneIsEnough_AndNameMatchingDoesNotReplaceIt()
+    {
+        var server = Server("prod-02", host: "prod-02.example.com");
+
+        /* Id present, name absent — the ordinary case for a server nobody has edited. */
+        Assert.Empty(StoreConfigProvider.ServersOnlyInFile(
+            new[] { server }, new HashSet<int> { server.ServerId }, new HashSet<string>()));
+
+        /* Neither present. */
+        Assert.Single(StoreConfigProvider.ServersOnlyInFile(
+            new[] { server }, new HashSet<int>(), new HashSet<string>()));
+    }
+
+    /// <summary>
+    /// Omitting the names keeps the old id-only behaviour, so a caller that has not been updated cannot start
+    /// silently suppressing the warning.
+    /// </summary>
+    [Fact]
+    public void WithNoNamesSuppliedTheComparisonIsIdOnly()
+    {
+        var server = Server("prod-03", host: "prod-03.example.com");
+
+        Assert.Single(StoreConfigProvider.ServersOnlyInFile(new[] { server }, new HashSet<int>()));
+        Assert.Empty(StoreConfigProvider.ServersOnlyInFile(new[] { server }, new HashSet<int> { server.ServerId }));
+    }
+
+    private static MonitoredServer Server(string name, string host) =>
+        new() { Name = name, Host = host, Auth = "integrated" };
+
+    private static string ReadDialogSource([CallerFilePath] string thisFile = "")
+    {
+        var dir = Path.GetDirectoryName(thisFile)!;
+        var relative = Path.Combine("Darling", "PerformanceMonitor.Darling.Viewer", "AddServerDialog.xaml.cs");
+        while (dir is not null && !File.Exists(Path.Combine(dir, relative)))
+        {
+            dir = Path.GetDirectoryName(dir);
+        }
+
+        Assert.NotNull(dir);
+        return File.ReadAllText(Path.Combine(dir!, relative));
+    }
+}