From a816918a68c4c5e553d97c60ac3fc763c02cecc1 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 15 Aug 2026 05:48:03 +0200 Subject: [PATCH] An edit keeps its server identity, so 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. The registry stayed tidy and every collect.* row keyed to the old id was orphaned with nothing pointing at it -- which is why nobody noticed: the server just reads as though it was never 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. Assigned rather than derived, argued from consequences: #2158 says a config edit must not change the identity; #2228 says two 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 makes #2158 strictly worse. One shape satisfies all three -- allocate once, never recompute, treat the derived address as a lookup key, and get what the target actually IS from the connection. StoredServerId and the connector's config.ServerId already did their half; the Viewer's save was the last site. Two things the fix had to carry. The collision guard compared a DERIVED id against the registry, which only holds 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 the address columns (IS NOT DISTINCT FROM for the nullable database; = never matches NULL, so every server-scoped registration would read as "free") and compares ids afterwards, which still lets a rename or re-credential through. And the darling.json reconcile matched on id alone, so after an edit it would call a monitored server absent and advise re-adding it -- wrong advice every start about the server just fixed. Now id OR name, either-or so a same-named sibling cannot hide a genuinely unmonitored one. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 + .../ServerIdentitySurvivesAnEditTests.cs | 185 ++++++++++++++++++ .../StoreConfigProvider.cs | 47 +++-- .../AddServerDialog.xaml.cs | 34 ++-- .../ViewerDataService.MonitoredServers.cs | 44 +++++ 5 files changed, 290 insertions(+), 24 deletions(-) create mode 100644 Darling/Darling.Tests/ServerIdentitySurvivesAnEditTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index cba78927d7..cde1e21148 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Darling/Darling.Tests/ServerIdentitySurvivesAnEditTests.cs b/Darling/Darling.Tests/ServerIdentitySurvivesAnEditTests.cs new file mode 100644 index 0000000000..450d23298d --- /dev/null +++ b/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; + +/// +/// #2158: editing a server's address keeps its identity, so its collected history stays attached to it. +/// +/// The defect. The Add/Edit save re-derived server_id 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 collect.* 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. +/// +/// Why identity must be assigned rather than derived, argued from consequences. 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. +/// +public sealed class ServerIdentitySurvivesAnEditTests +{ + /// + /// 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). + /// + /// Pinned textually because the alternative is a WPF dialog — reproducing it behaviourally means + /// standing up AddServerDialog 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. + /// + [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); + } + + /// + /// The collision guard survived the change and now asks about the ADDRESS. + /// + /// 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. + /// + [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); + } + + /// + /// The address lookup matches a NULL database with IS NOT DISTINCT FROM. A plain = 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. + /// + [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); + } + + /// + /// The file-vs-store reconcile no longer calls an edited server "not monitored". + /// + /// 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. + /// + [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 { 999_111 }; + var storeNames = new HashSet(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())); + } + + /// + /// 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. + /// + [Fact] + public void ADeletedServerIsStillReportedAsFileOnly() + { + var file = new[] { Server("gone-01", host: "gone-01.example.com") }; + + var missing = StoreConfigProvider.ServersOnlyInFile( + file, new HashSet { 999_111 }, new HashSet(StringComparer.OrdinalIgnoreCase) { "someone-else" }); + + Assert.Equal(new[] { "gone-01" }, missing); + } + + /// + /// 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. + /// + [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 { server.ServerId }, new HashSet())); + + /* Neither present. */ + Assert.Single(StoreConfigProvider.ServersOnlyInFile( + new[] { server }, new HashSet(), new HashSet())); + } + + /// + /// Omitting the names keeps the old id-only behaviour, so a caller that has not been updated cannot start + /// silently suppressing the warning. + /// + [Fact] + public void WithNoNamesSuppliedTheComparisonIsIdOnly() + { + var server = Server("prod-03", host: "prod-03.example.com"); + + Assert.Single(StoreConfigProvider.ServersOnlyInFile(new[] { server }, new HashSet())); + Assert.Empty(StoreConfigProvider.ServersOnlyInFile(new[] { server }, new HashSet { 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)); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs b/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs index 7bfcc22a81..8d81a53634 100644 --- a/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs +++ b/Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs @@ -141,24 +141,33 @@ operator to discover it. */ /// outputs that were each correct about different things and no way to see the disagreement: config edit, /// service restart, support round trip. /// - /// Compared on server_id, not name — that is the identity the collectors and the registry - /// actually key on, so a file entry whose host or read-only intent differs is correctly reported as - /// absent even if a same-named row exists. Reported BY name, because that is what the operator typed. + /// Compared on server_id OR name (#2158). It used to be id alone, on the grounds that the id + /// is what the collectors key on — correct while every row's id equalled the hash of its own address, and + /// wrong the moment an edit began PRESERVING a row's identity so a re-addressed server keeps its history. + /// After such an edit the file's derived id matches nothing, and an id-only comparison would report a + /// server that IS monitored as absent, then advise re-adding it — wrong advice, on every start, about the + /// one server the operator had just fixed. The name arm covers that; a genuinely removed server is gone + /// from the store under both keys, so the Viewer-Remove case still reports exactly as before. /// private async Task WarnAboutFileOnlyServersAsync( NpgsqlConnection connection, DarlingConfig config, CancellationToken ct) { var storeIds = new HashSet(); - using (var command = new NpgsqlCommand("SELECT server_id FROM config_monitored_servers", connection)) + var storeNames = new HashSet(StringComparer.OrdinalIgnoreCase); + using (var command = new NpgsqlCommand("SELECT server_id, name FROM config_monitored_servers", connection)) await using (var reader = await command.ExecuteReaderAsync(ct)) { while (await reader.ReadAsync(ct)) { storeIds.Add(reader.GetInt32(0)); + if (!reader.IsDBNull(1)) + { + storeNames.Add(reader.GetString(1)); + } } } - var fileOnly = ServersOnlyInFile(config.Servers, storeIds); + var fileOnly = ServersOnlyInFile(config.Servers, storeIds, storeNames); if (fileOnly.Count == 0) { return; @@ -190,7 +199,7 @@ edit that silences it. */ /// comparison is testable without a store — the log line above is the only part that needs one. /// internal static IReadOnlyList ServersOnlyInFile( - IEnumerable fileServers, ISet storeServerIds) + IEnumerable fileServers, ISet storeServerIds, ISet? storeNames = null) { var missing = new List(); if (fileServers is null) @@ -200,14 +209,28 @@ internal static IReadOnlyList ServersOnlyInFile( foreach (var server in fileServers) { - /* A file entry has no StoredServerId, so ServerId here IS the derivation — which is what this - comparison needs: it is asking "would the id this file entry describes be in the store". Once - identity stops being derivable (#2218) that question stops being answerable this way and has - to move onto the observed-identity fingerprint; noted on #2228 rather than pre-solved here. */ - if (!storeServerIds.Contains(server.ServerId)) + /* A file entry has no StoredServerId, so ServerId here IS the derivation: "would the id this file + entry describes be in the store". That was the whole test until #2158 made an edit preserve its + identity — a re-addressed server keeps its own id so its history stays attached, which means the + file's derived id no longer matches it and the id arm alone now reports a monitored server as + absent. The NAME arm answers the question the log actually asks, "is this file entry represented + in the store at all", and it is the operator-facing key: the display name is what they typed and + what the Viewer shows, and an edit does not change it. + + Deliberately either-or rather than name-only. Two different file entries can share a display + name (nothing enforces uniqueness), so name-only would hide a genuinely unmonitored server + behind a same-named sibling; and the id arm still resolves the common case exactly. */ + if (storeServerIds.Contains(server.ServerId)) + { + continue; + } + + if (storeNames is not null && storeNames.Contains(server.DisplayName)) { - missing.Add(server.DisplayName); + continue; } + + missing.Add(server.DisplayName); } return missing; diff --git a/Darling/PerformanceMonitor.Darling.Viewer/AddServerDialog.xaml.cs b/Darling/PerformanceMonitor.Darling.Viewer/AddServerDialog.xaml.cs index 1aff2f54f2..c3f4b011e3 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/AddServerDialog.xaml.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/AddServerDialog.xaml.cs @@ -348,7 +348,13 @@ authoring convenience the store needs no table for. */ return new MonitoredServerRow { - ServerId = ViewerDataService.ComputeServerId(host, database, readOnlyIntent), + /* #2158: an EDIT keeps the row's identity; only an Add derives one. Changing a server's address + does not make it a different server — it is the same monitored instance — and every collect.* + row is keyed by this id, so re-deriving it abandons the whole of that server's history. The old + shape wrote a row under the new hash and deleted the old one, which left the REGISTRY tidy and + the history orphaned with nothing pointing at it: the failure looked like a server that had + never been monitored. Derivation now runs only where there is no history to lose. */ + ServerId = _originalServerId ?? ViewerDataService.ComputeServerId(host, database, readOnlyIntent), Name = displayName, Host = host, Database = database, @@ -397,24 +403,28 @@ private async void SaveButton_Click(object sender, RoutedEventArgs e) return; } - /* Refuse to silently overwrite a DIFFERENT existing server that shares this identity — the upsert's - ON CONFLICT DO UPDATE would clobber its excluded databases / capture override. Covers Add and an - edit that re-points host/database/read-only-intent onto another server's identity. */ - if (_originalServerId != row.ServerId - && await _dataService.GetMonitoredServerAsync(row.ServerId) is not null) + /* Refuse to point this definition at an address another server already monitors: on Add the + upsert's ON CONFLICT DO UPDATE would clobber that row's excluded databases / capture override, + and on Edit it would leave two registrations collecting the same real instance under two + identities — #2228's shape, arrived at from the registry side. + + #2158: checked against the ADDRESS rather than against a derived id. Now that an edit preserves + its identity, a row's server_id no longer has to equal the hash of its own address, so the old + id-based lookup would miss exactly the row it exists to protect. Comparing ids afterwards is + what excludes "collided with myself" — an edit that leaves the address alone, or that only + renames or re-credentials the server. */ + var occupant = await _dataService.GetMonitoredServerByAddressAsync(row.Host, row.Database, row.ReadOnlyIntent); + if (occupant is not null && occupant.ServerId != row.ServerId) { StatusText.Text = "A server with this address (and database / read-only intent) is already monitored. Edit it from Manage Servers instead."; SaveButton.IsEnabled = true; return; } - /* Write the NEW row first, THEN drop the old identity on an edit that moved it — if the delete - fails we leave a recoverable duplicate rather than losing the definition entirely. */ + /* One row, one identity, in place — no delete. The upsert's ON CONFLICT (server_id) arm rewrites + the address on the row that already owns this id, so the server's collected history stays + attached to it. */ await _dataService.UpsertMonitoredServerAsync(row); - if (_originalServerId is int original && original != row.ServerId) - { - await _dataService.DeleteMonitoredServerAsync(original); - } /* Favorites are viewer-local (the service never reads them) — keyed by the server address. */ _serverStore.SetFavorite(row.Host, FavoriteCheckBox.IsChecked == true); diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.MonitoredServers.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.MonitoredServers.cs index aea0fd3546..51ebbd46ab 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.MonitoredServers.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.MonitoredServers.cs @@ -131,6 +131,32 @@ FROM config_monitored_servers FROM config_monitored_servers WHERE server_id = $1"; + /// + /// One configured server by its ADDRESS — host, database and read-only intent (#2158). The collision check + /// the Add/Edit save runs before writing. + /// + /// Why by address and not by derived id. The guard used to look the address's + /// hash up by server_id, which only works while every row's id still + /// equals the hash of its own address. Once an edit PRESERVES a row's identity — which is the point of + /// #2158, so a re-addressed server keeps its collected history — that stops being true, and a hash lookup + /// would miss the very row it is meant to protect: two registrations would end up pointing at one real + /// instance, which is #2228's shape. Matching the address columns asks the question the guard actually + /// means. + /// + /// IS NOT DISTINCT FROM for database because it is nullable and NULL = NULL is unknown + /// in SQL: a plain = would never match the server-scoped registrations (the common case), so every + /// one of them would read as "address free". Secret-free projection — the caller only needs to know whether + /// a row exists and which id it has, so this runs for a read-only seat too. + /// + public const string MonitoredServerByAddressSql = @" +SELECT server_id, name, host, database, auth, username, encrypt_mode, + trust_server_certificate, read_only_intent, multi_subnet_failover, excluded_databases, + monthly_cost_usd, capture_plans, is_enabled, created_at, alert_delivery_mode_override +FROM config_monitored_servers +WHERE host = $1 +AND database IS NOT DISTINCT FROM $2 +AND read_only_intent = $3"; + /// Row count — the migrate-in / reconcile "is the config-server set seeded yet?" guard. public const string MonitoredServersCountSql = "SELECT COUNT(*) FROM config_monitored_servers"; @@ -262,6 +288,24 @@ public async Task> GetMonitoredServersAsync(Cancellatio return await reader.ReadAsync(cancellationToken) ? ReadMonitoredServerRow(reader) : null; } + /// + /// The server already registered at this address, or null when the address is free (#2158) — what the + /// Add/Edit save checks before writing, so one real instance cannot end up under two identities. + /// + /// Secret-free by design: the caller compares ids and shows a message, so there is no reason to + /// read the DPAPI blob, and skipping it means a read-only seat gets the same answer instead of 42501. + /// + public async Task GetMonitoredServerByAddressAsync( + string host, string? database, bool readOnlyIntent, CancellationToken cancellationToken = default) + { + await using var command = _dataSource.CreateCommand(MonitoredServerByAddressSql); + command.Parameters.Add(new NpgsqlParameter { TypedValue = host }); + command.Parameters.Add(new NpgsqlParameter { Value = (object?)database ?? DBNull.Value }); + command.Parameters.Add(new NpgsqlParameter { TypedValue = readOnlyIntent }); + await using var reader = await command.ExecuteReaderAsync(cancellationToken); + return await reader.ReadAsync(cancellationToken) ? ReadMonitoredServerRowNoSecret(reader) : null; + } + /// How many servers the config-server registry holds (the migrate-in / reconcile guard). public async Task GetMonitoredServerCountAsync(CancellationToken cancellationToken = default) {