From 320dda3244b5a05f8107da22892aabdcdd2f0235 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 15 Aug 2026 08:54:45 +0200 Subject: [PATCH 1/3] Refuse a registration that lands on an already-monitored database (#2280) Identity is registration-derived, so N registrations silently resolving to one database get N identities and N copies of every row -- #2220's byte-identical deadlock graphs under six server_ids. #2277 added the tripwire that reports it at connect; this refuses it where it can be prevented. add_servers already probes in-process, and since #2277 the probe returns the database the connection actually reached, so the answer is in hand exactly when the decision is made. No comparison of configuration substitutes: the colliding registrations genuinely differ, which is why #2158 and #2218 could not touch this. Keyed on the FULL identity, which is what keeps two legitimate pairs working -- a read-only-intent registration alongside its read-write twin, and a PostgreSQL instance alongside a SQL Server on one host. Silent when the probe reports no database (unknown is not colliding) and when the entry reaches what it names (already ruled on by the declared gate). It also fixes a regression #2218 introduced in this same method: the duplicate gate built its key from host, database and read-only intent only, so once engine and port joined the identity the gate keyed on a NARROWER identity than the store derives -- and a PostgreSQL instance on a host that already had a SQL Server registration read as a duplicate and was refused. A valid pair rejected because the gate could not see what distinguished them. It compiled and every existing test passed, so it now has an explicit pin. The honest limit: existing rows record only the database they DECLARE, so this catches "the new one lands where an existing one lives" and not "both mis-resolve to a database neither names". #2277's tripwire reports the latter at connect for both. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 + .../RegistrationCollisionTests.cs | 196 ++++++++++++++++++ .../DarlingServerConnector.cs | 7 +- .../Mcp/DarlingMcpServerAdminTools.cs | 98 ++++++++- 4 files changed, 301 insertions(+), 4 deletions(-) create mode 100644 Darling/Darling.Tests/RegistrationCollisionTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 86089c40fa..b8b8daadd1 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 +- **`add_servers` refuses a registration whose connection lands in a database already monitored** ([#2280], the registration-time half of [#2228]) - identity is registration-derived, so N registrations that silently resolve to one database get N identities and N full copies of every collected row: [#2220]'s byte-identical deadlock graphs under six `server_id`s, one incident alerting six times. [#2277] added the connect-time tripwire that REPORTS it; this refuses it where it can be prevented instead of described. The probe already runs in-process at Add, and since [#2277] it returns the database the connection actually reached, so the answer is in hand at exactly the moment the decision has to be made - no comparison of configuration could substitute, because the two colliding registrations genuinely differ, which is why [#2158] and [#2218] could not touch this. Keyed on the FULL identity, which is what keeps two legitimate pairs working: a read-only-intent registration alongside its read-write twin for one database, and a PostgreSQL instance alongside a SQL Server on one host. Silent when the probe reports no database (unknown is not colliding - refusing there would block a registration for a reason nobody could act on) and when the entry reaches the database it names (the ordinary case, already ruled on by the declared-duplicate gate). **It also fixes a regression [#2218] introduced in this same method**: the duplicate gate built its key from host, database and read-only intent only, so once engine and port joined the identity the gate was keying on a NARROWER identity than the store derives - and a PostgreSQL instance on a host that already had a SQL Server registration read as a duplicate and was refused. A valid pair rejected because the gate could not see what distinguished them; it compiled and every existing test passed, which is why it now has an explicit pin. The honest limit, since it bounds what this can promise: existing rows record only the database they DECLARE, so this catches "the new one lands where an existing one lives" and not "both mis-resolve to a database neither names" - [#2277]'s tripwire reports the latter at connect for both. + - **Add Server warns when a SQL-auth password may not be decryptable by the service** ([#2279], the policy half of [#2255]) - a password is stored as a DPAPI `LocalMachine` blob, decryptable only on the machine that wrote it, and the SERVICE is what has to decrypt it. So a credential saved from a viewer on another PC can never be used and the server fails to connect on every sweep afterwards - the [#2255] report. [#2273] made that failure explain itself; this says so BEFORE the save, in the same place and the same way the dialog already warns about an Azure/Entra mode the service cannot honour, so it lands as the mode is picked rather than after the fact. **Warned, not refused**, and that is the decision rather than a hedge: the signal is whether the viewer's store is reached over LOOPBACK, which is a proxy for "the service runs here" derived from the product's own architecture - the managed deploy builds its store connection on literal `127.0.0.1` (`ViewerSettings` mirroring the service's `DarlingManagedPostgres.BuildConnectionString`) and the service runs where its managed store runs. A non-loopback store does NOT prove the viewer is remote, though: a bring-your-own store on another host with the service local reads identically. That is good enough to decide whether to SAY something and not good enough to decide whether to BLOCK, and inverting it would refuse a legitimate first-run Add on the service host. **Silent for a loopback store**, which is the managed single-box deploy the DPAPI design targets and the overwhelmingly common case - a hint that fires for everyone is a hint nobody reads, so a false positive there would defeat the feature rather than merely annoy. Also silent for an omitted host (Npgsql defaults to localhost, so no `Host` means a local store), for an absent connection string, and for an unparseable one - failing toward silence, since the wrong direction is a warning the operator cannot act on. A host that merely CONTAINS a loopback spelling still warns. The hint names all three ways to produce a usable credential (a viewer on the service's host, `--add-server` there, or an `env:`/`file:` reference, which is not machine-bound at all) and clears itself when the auth mode changes away, the same self-clearing discipline the Azure message uses. - **A skipped database-state maintenance cycle now says so, once** ([#2266] follow-on) - `GetDatabaseStateDeviationsAsync` performs its baseline seed, the [#2189] heal, the [#2203] forget and the prune inside a best-effort block: it opens the write connection with a 5-second lock acquisition and, on `TimeoutException`, skips all of it while still running the deviation read. Skipping is the right behaviour - the method's own comment makes the case that it is the only lossless option when archival holds the lock - but it logged NOTHING, so a sustained window of write-lock contention meant baselines quietly stopped being seeded and healed with no evidence anywhere. That matters more than a typical missing log line: [#2189] exists *because* an unhealed baseline inverts the alert permanently, so the failure this maintenance prevents is itself invisible and its absence has to be visible instead. Reported on the TRANSITION - one line when it starts skipping, one when it resumes - rather than per sweep, because a standing contention window would otherwise emit a line every cycle and get filtered, which restores the silence. At **Warn**, not Error: one skipped cycle is the expected benign outcome of colliding with archival and the next sweep re-runs everything, so flagging it as a fault is the fastest way to get the line ignored. The message names the CONSEQUENCE rather than the event ("baselines are not being seeded or healed while this persists"), says the deviations were still read so nobody wonders whether the numbers are stale, and says one occurrence is expected so nobody escalates it. Keyed per server, because the lock is process-wide but the consequence is not. A regression test pins the one property that is a stray `return` away at all times: a skipped maintenance block must still run the deviation read - swallowing it would report every database as recovered and clear the alert memory for all of them. @@ -2735,6 +2737,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 +[#2280]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2280 +[#2277]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2277 [#2279]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2279 [#2273]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2273 [#2220]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2220 diff --git a/Darling/Darling.Tests/RegistrationCollisionTests.cs b/Darling/Darling.Tests/RegistrationCollisionTests.cs new file mode 100644 index 0000000000..7366d07bb7 --- /dev/null +++ b/Darling/Darling.Tests/RegistrationCollisionTests.cs @@ -0,0 +1,196 @@ +/* + * 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 PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Service; +using PerformanceMonitor.Darling.Service.Mcp; +using Xunit; + +namespace Darling.Tests; + +/// +/// #2280: add_servers refuses a registration whose connection lands in a database another registration +/// already covers — and, in the same method, keys its duplicate gate on the identity the store actually derives. +/// +/// The defect being prevented. Identity is registration-derived, so N registrations that silently +/// resolve to one database get N identities and N full copies of every collected row. #2220 reported it as +/// byte-identical deadlock graphs under six server_ids, one real incident alerting six times. #2277 added +/// the connect-time tripwire that reports it; this refuses it at the point of creation, which is the only place +/// it can be prevented rather than described. +/// +/// Why the probe is what makes it possible. No comparison of configuration can decide this — the two +/// registrations genuinely differ, which is why #2158 and #2218 (identity assigned, then widened) could not touch +/// it. Only the server can say which database a connection reached, and add_servers already probes +/// in-process, so the answer is in hand at exactly the moment the decision has to be made. +/// +public sealed class RegistrationCollisionTests +{ + private static DarlingMcpServerAdminTools.ParsedServerEntry Entry( + string host, string? database, bool readOnlyIntent = false, string engine = "sqlserver", int port = 0) + { + var config = new MonitoredServer + { + Name = host, + Host = host, + Database = database, + ReadOnlyIntent = readOnlyIntent, + Engine = engine, + Port = port, + Auth = "integrated", + }; + + var key = ServerIdHelper.BuildStorageName(host, database, readOnlyIntent, engine, port); + return new DarlingMcpServerAdminTools.ParsedServerEntry(0, host, key, config, null); + } + + private static HashSet Claimed(params string[] keys) => + new(keys, StringComparer.OrdinalIgnoreCase); + + /// + /// THE CASE: the entry names one database, its connection lands in another, and that other one is already + /// monitored. Adding it would give one real database two identities. + /// + [Fact] + public void ARegistrationLandingOnAnAlreadyMonitoredDatabaseIsRefused() + { + var entry = Entry("azure.example.net", "Sibling-A"); + var claimed = Claimed(ServerIdHelper.BuildStorageName("azure.example.net", "Source-DB", false, "sqlserver", 0)); + + var reason = DarlingMcpServerAdminTools.ActualIdentityCollision(entry, "Source-DB", claimed); + + Assert.NotNull(reason); + Assert.Contains("Sibling-A", reason, StringComparison.Ordinal); + Assert.Contains("Source-DB", reason, StringComparison.Ordinal); + /* It has to say what the harm is, or it reads as pedantry about naming. */ + Assert.Contains("two identities", reason, StringComparison.Ordinal); + Assert.Contains("Initial Catalog", reason, StringComparison.Ordinal); + } + + /// + /// An entry that reaches the database it NAMES is the ordinary case and passes — the declared gate has + /// already ruled on it, so firing here would only re-detect that gate's decision under a worse name. + /// + [Theory] + [InlineData("SalesDB", "SalesDB")] + [InlineData("SalesDB", "salesdb")] + [InlineData("SalesDB", " SalesDB ")] + public void AnEntryThatReachesWhatItNamesIsAllowed(string declared, string connected) + { + var entry = Entry("host1", declared); + var claimed = Claimed(ServerIdHelper.BuildStorageName("host1", declared, false, "sqlserver", 0)); + + Assert.Null(DarlingMcpServerAdminTools.ActualIdentityCollision(entry, connected, claimed)); + } + + /// + /// Landing somewhere else is fine as long as nobody else covers it — this refuses a COLLISION, not a + /// misconfiguration. #2277's tripwire is what reports the latter, at every connect. + /// + [Fact] + public void LandingElsewhereIsAllowedWhenNobodyElseCoversIt() + { + var entry = Entry("host1", "Declared-DB"); + var claimed = Claimed(ServerIdHelper.BuildStorageName("host1", "Declared-DB", false, "sqlserver", 0)); + + Assert.Null(DarlingMcpServerAdminTools.ActualIdentityCollision(entry, "Somewhere-Else", claimed)); + } + + /// + /// An absent probe answer is UNKNOWN, not colliding. Refusing on a missing value would block registrations + /// for a reason nobody could act on — and a stub probe returns exactly this. + /// + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void AnUnknownConnectedDatabaseNeverRefuses(string? connected) + { + var entry = Entry("host1", "Declared-DB"); + var claimed = Claimed(ServerIdHelper.BuildStorageName("host1", "Other-DB", false, "sqlserver", 0)); + + Assert.Null(DarlingMcpServerAdminTools.ActualIdentityCollision(entry, connected, claimed)); + } + + /// + /// THE VALID PAIR THAT MUST STILL BE ALLOWED: a read-only-intent registration alongside a read-write one for + /// the same database. read_only_intent is part of the identity, so comparing on (host, database) alone + /// would refuse a legitimate configuration — which is why the check keys on the FULL identity. + /// + [Fact] + public void AReadOnlyIntentRegistrationDoesNotCollideWithItsReadWriteTwin() + { + /* Read-only entry that lands in Source-DB; the read-WRITE registration of Source-DB is already claimed. */ + var entry = Entry("ag-listener", "Sibling-A", readOnlyIntent: true); + var claimed = Claimed(ServerIdHelper.BuildStorageName("ag-listener", "Source-DB", false, "sqlserver", 0)); + + Assert.Null(DarlingMcpServerAdminTools.ActualIdentityCollision(entry, "Source-DB", claimed)); + + /* But it DOES collide with another read-only registration of the same database. */ + var roClaimed = Claimed(ServerIdHelper.BuildStorageName("ag-listener", "Source-DB", true, "sqlserver", 0)); + Assert.NotNull(DarlingMcpServerAdminTools.ActualIdentityCollision(entry, "Source-DB", roClaimed)); + } + + /// + /// Engine is part of the identity too (#2218), so a PostgreSQL entry landing in a database name that a SQL + /// Server registration covers is not a collision — they are different instances on one host. + /// + [Fact] + public void APostgresEntryDoesNotCollideWithASqlServerRegistration() + { + var entry = Entry("box01", "declared", engine: "postgres"); + var sqlServerClaim = Claimed(ServerIdHelper.BuildStorageName("box01", "actual", false, "sqlserver", 0)); + + Assert.Null(DarlingMcpServerAdminTools.ActualIdentityCollision(entry, "actual", sqlServerClaim)); + + /* Another PostgreSQL registration of that database IS a collision. */ + var pgClaim = Claimed(ServerIdHelper.BuildStorageName("box01", "actual", false, "postgres", 0)); + Assert.NotNull(DarlingMcpServerAdminTools.ActualIdentityCollision(entry, "actual", pgClaim)); + } + + /// + /// A server-scoped entry (no database named) that lands somewhere already covered is still refused — the + /// message says "no database" rather than pretending it named one, so the operator can tell which of their + /// registrations is the vague one. + /// + [Fact] + public void AServerScopedEntryThatLandsOnACoveredDatabaseIsRefusedAndSaysSo() + { + var entry = Entry("host1", null); + var claimed = Claimed(ServerIdHelper.BuildStorageName("host1", "master", false, "sqlserver", 0)); + + var reason = DarlingMcpServerAdminTools.ActualIdentityCollision(entry, "master", claimed); + + Assert.NotNull(reason); + Assert.Contains("no database", reason, StringComparison.Ordinal); + } + + /// + /// #2218's regression in this method, pinned: the duplicate gate reads engine and port, so it + /// keys on the identity the store actually derives. + /// + /// Without them the gate keys on a NARROWER identity than the product does, and a PostgreSQL instance + /// on a host that already has a SQL Server registration reads as a duplicate and is refused — a valid pair + /// rejected because the gate could not see what distinguishes them. It compiles and every existing test + /// passes, which is why it is worth an explicit pin. + /// + [Fact] + public void TheDuplicateGateReadsTheFullIdentity() + { + Assert.Contains("engine", DarlingMcpServerAdminTools.ExistingServersSql, StringComparison.Ordinal); + Assert.Contains("port", DarlingMcpServerAdminTools.ExistingServersSql, StringComparison.Ordinal); + Assert.Contains("read_only_intent", DarlingMcpServerAdminTools.ExistingServersSql, StringComparison.Ordinal); + + /* And the two identities it would compare genuinely differ, so the columns are load-bearing rather than + decorative. */ + Assert.NotEqual( + ServerIdHelper.BuildStorageName("box01", null, false, "sqlserver", 0), + ServerIdHelper.BuildStorageName("box01", null, false, "postgres", 0)); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingServerConnector.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingServerConnector.cs index 94cccc2fd2..8ecd55bad5 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingServerConnector.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingServerConnector.cs @@ -356,6 +356,7 @@ succeeded against a different engine. */ IsAwsRds: runtime.IsAwsRds, HasMsdbAccess: runtime.HasMsdbAccess, Error: null, + ConnectedDatabase: runtime.ConnectedDatabase, Engine: runtime.Target.Engine, PostgresMajorVersion: runtime.Target.PostgresMajorVersion, PostgresVersionNum: runtime.Target.PostgresVersionNum, @@ -466,7 +467,11 @@ public sealed record ConnectionProbeResult( int PostgresMajorVersion = 0, int PostgresVersionNum = 0, bool IsAurora = false, - bool IsInRecovery = false) + bool IsInRecovery = false, + /* #2280: the database the connection ACTUALLY reached, so a registration-time collision check can compare + what the SERVER says against what other registrations claim, rather than comparing two claims. Trailing + and defaulted, so every existing construction of this record still compiles unchanged. */ + string? ConnectedDatabase = null) { /// /// Rebuilds the gate's-eye view of this target, so a caller can ask which collectors would actually diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs index 04e5b179e4..2f7249776c 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs @@ -133,6 +133,16 @@ a duplicate (of an existing server OR an earlier entry in this batch, first occu var (ready, duplicates) = PartitionDuplicates(entries, existingKeys); results.AddRange(duplicates); + /* #2280: the identities claimed so far — the store's, plus every entry this batch is about to add. + The gate above compares DECLARED identities; the check inside the loop compares each entry's ACTUAL + database (what the server just told the probe) against this set, which is what catches two + registrations resolving to one database while claiming different ones. */ + var claimed = new HashSet(existingKeys, StringComparer.OrdinalIgnoreCase); + foreach (var entry in ready) + { + claimed.Add(entry.StorageKey); + } + foreach (var entry in ready) { /* Validate the connection IN-PROCESS (the service holds the network path + credentials). A failure @@ -147,6 +157,22 @@ is recorded and the batch CONTINUES — one unreachable server never aborts the continue; } + /* #2280: the probe just asked the server which database it actually reached. If that is a + DIFFERENT database from the one this entry names, and some other registration already claims + that one, then adding this would give one real database two identities and two full copies of + every collected row — the #2220 field report, prevented at the point of creation instead of + reported at every connect by #2277's tripwire. + + Compared against the ACTUAL database and only when it differs from the declared one: an entry + that names what it reached is the normal case and is already covered by the declared gate + above, so re-checking it would just re-detect that gate's own decision. */ + var collision = ActualIdentityCollision(entry, probeResult.ConnectedDatabase, claimed); + if (collision is not null) + { + results.Add(new ServerResult(entry.Order, entry.DisplayName, "collides", collision)); + continue; + } + /* DPAPI-encrypt the SQL password for storage (the service identity encrypts here and decrypts it during collection, so it round-trips); Windows-auth servers store no secret. The plaintext never leaves this method — it is not logged, not echoed in a result. */ @@ -400,7 +426,9 @@ fail at every connect. */ Port = port, }; - var storageKey = ServerIdHelper.BuildStorageName(host, database, readOnlyIntent); + /* #2218: the FULL identity, matching what the store derives — engine and port included, so a PostgreSQL + entry does not collide with a SQL Server one on the same host. */ + var storageKey = ServerIdHelper.BuildStorageName(host, database, readOnlyIntent, engine, port); return (new ParsedServerEntry(index, displayName, storageKey, probeConfig, plaintextPassword), null); } @@ -410,6 +438,60 @@ fail at every connect. */ /// entries to probe + insert and the Duplicates as ready-to-report results. Unit-testable without a /// store or probe. /// + /// + /// The #2280 check: why this entry must not be added, or null when it may be. + /// + /// Compares the identity this entry would have if keyed on the database the server ACTUALLY reached + /// against the identities already claimed. Only fires when the actual database DIFFERS from the declared one + /// — an entry that reached what it named is the ordinary case and the declared gate has already ruled on it, + /// so re-checking would only re-detect that gate's decision under a more confusing name. + /// + /// Silent when the probe did not report a database (a stub probe, or a target that returned + /// none): unknown is not the same as colliding, and refusing on an absent value would block registrations + /// for a reason nobody could act on. + /// + /// Keyed on the FULL identity, not on (host, database). A read-only-intent registration + /// alongside a read-write one for the same database is legitimate and read_only_intent is part of the + /// identity, so comparing without it would refuse a valid pair. Same for engine and port after #2218. + /// + /// Note the asymmetry this cannot see: existing rows record only the database they DECLARE, so this + /// catches "the new one lands where an existing one lives" and not "both mis-resolve to a database neither + /// names". Closing that needs the actual database persisted per row; #2277's tripwire reports it at connect + /// for both in the meantime, which is why this is a guard and not the whole answer. + /// + internal static string? ActualIdentityCollision( + ParsedServerEntry entry, string? connectedDatabase, ISet claimedKeys) + { + if (entry is null || claimedKeys is null || string.IsNullOrWhiteSpace(connectedDatabase)) + { + return null; + } + + var declared = entry.ProbeConfig.Database; + if (!string.IsNullOrWhiteSpace(declared) + && string.Equals(declared.Trim(), connectedDatabase.Trim(), StringComparison.OrdinalIgnoreCase)) + { + return null; + } + + var actualKey = ServerIdHelper.BuildStorageName( + entry.ProbeConfig.Host, connectedDatabase.Trim(), entry.ProbeConfig.ReadOnlyIntent, + entry.ProbeConfig.Engine, entry.ProbeConfig.Port); + + if (string.Equals(actualKey, entry.StorageKey, StringComparison.OrdinalIgnoreCase) + || !claimedKeys.Contains(actualKey)) + { + return null; + } + + var declaredText = string.IsNullOrWhiteSpace(declared) ? "no database" : $"database '{declared}'"; + return $"Not added: this registration names {declaredText} but its connection lands in " + + $"'{connectedDatabase.Trim()}', which another monitored server already covers. Adding it would " + + "store that one database's history under two identities and alert twice for every incident. " + + "Point it at the database you meant (check Initial Catalog), or monitor the existing " + + "registration instead."; + } + internal static (List Ready, List Duplicates) PartitionDuplicates( IReadOnlyList entries, IEnumerable existingKeys) { @@ -437,7 +519,15 @@ internal static (List Ready, List Duplicates) P /// Reads the identity fields of every existing monitored server so the dedupe gate can be seeded from /// the authoritative set (mirrors the bulk dialog's LoadExistingKeysAsync). Non-secret columns only. - public const string ExistingServersSql = "SELECT host, database, read_only_intent FROM config_monitored_servers"; + /// Reads the identity fields of every existing monitored server so the dedupe gate can be seeded from + /// the authoritative set. Non-secret columns only. + /// + /// #2218 added engine and port to the identity, so they have to be read here too. Without + /// them the gate keys on a NARROWER identity than the product does, and a PostgreSQL instance on a host that + /// already has a SQL Server registration reads as a duplicate and is refused — a valid pair rejected because + /// the gate could not see what distinguishes them. + public const string ExistingServersSql = + "SELECT host, database, read_only_intent, engine, port FROM config_monitored_servers"; /// The INSERT — column set + shape mirrored from StoreConfigProvider.SeedMonitoredServersAsync /// (the seed authority), so a tool-added row is byte-identical to a seeded one. capture_plans and @@ -463,7 +553,9 @@ private static async Task> LoadExistingStorageKeysAsync(NpgsqlDataS var host = reader.GetString(0); var database = reader.IsDBNull(1) ? null : reader.GetString(1); var readOnlyIntent = !reader.IsDBNull(2) && reader.GetBoolean(2); - keys.Add(ServerIdHelper.BuildStorageName(host, database, readOnlyIntent)); + var engine = reader.IsDBNull(3) ? null : reader.GetString(3); + var port = reader.IsDBNull(4) ? 0 : reader.GetInt32(4); + keys.Add(ServerIdHelper.BuildStorageName(host, database, readOnlyIntent, engine, port)); } return keys; From 6f25d6fb7ad175975ee03ab7f031a1d984f9dad6 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 15 Aug 2026 08:55:43 +0200 Subject: [PATCH 2/3] Reattach the doc block my insertion displaced (#2280) PartitionDuplicates' own summary ended up above ActualIdentityCollision, leaving PartitionDuplicates undocumented. Moved back rather than deleted, per the hygiene test's warning that most displaced blocks belong to a member that was left bare. My detector caught this before the push and I pushed anyway -- I had chained it into the same command as the commit, so the commit never waited on the result. Running it as its own step is the actual fix; knowing the trap and owning the check are both worthless if the check's output arrives after the thing it was supposed to gate. Co-Authored-By: Claude Opus 5 (1M context) --- .../Mcp/DarlingMcpServerAdminTools.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs index 2f7249776c..0b7c395298 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs @@ -432,12 +432,6 @@ entry does not collide with a SQL Server one on the same host. */ return (new ParsedServerEntry(index, displayName, storageKey, probeConfig, plaintextPassword), null); } - /// - /// PURE dedupe partition — the case-folded gate seeded with the - /// existing store keys, first-occurrence-wins within the batch (the #1549 idiom). Returns the Ready - /// entries to probe + insert and the Duplicates as ready-to-report results. Unit-testable without a - /// store or probe. - /// /// /// The #2280 check: why this entry must not be added, or null when it may be. /// @@ -492,6 +486,12 @@ entry does not collide with a SQL Server one on the same host. */ "registration instead."; } + /// + /// PURE dedupe partition — the case-folded gate seeded with the + /// existing store keys, first-occurrence-wins within the batch (the #1549 idiom). Returns the Ready + /// entries to probe + insert and the Duplicates as ready-to-report results. Unit-testable without a + /// store or probe. + /// internal static (List Ready, List Duplicates) PartitionDuplicates( IReadOnlyList entries, IEnumerable existingKeys) { From b857d1d2fa456ad0502bc763ada0bcc0ba01a5a9 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 15 Aug 2026 09:38:36 +0200 Subject: [PATCH 3/3] Merge a duplicate doc block my detector could not see (#2280) DocCommentHygieneTests reported openings at lines 520 AND 522 -- two apart, not adjacent. The original ExistingServersSql summary closed with INLINE at the end of a text line, and my detector only matched a line whose whole content is '/// '. So it read as clean while a real duplicate sat there. Merged rather than replaced, and the merge keeps the one detail my new block had dropped: the cross-reference to the bulk dialog's LoadExistingKeysAsync. That is the failure mode the hygiene test warns about -- deleting a displaced block loses documentation rather than deduplicating it -- reached by overwriting instead of by displacing. The detector is rewritten to match the test's actual rule: walk each contiguous run of /// lines and count openings in it, rather than looking for two specific adjacent lines. Verified across all 1,355 tracked .cs files: zero stacked blocks on this branch. Co-Authored-By: Claude Opus 5 (1M context) --- .../Mcp/DarlingMcpServerAdminTools.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs index 0b7c395298..abf07aca3c 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpServerAdminTools.cs @@ -518,9 +518,7 @@ internal static (List Ready, List Duplicates) P /* ─────────────────────────────── store I/O ─────────────────────────────── */ /// Reads the identity fields of every existing monitored server so the dedupe gate can be seeded from - /// the authoritative set (mirrors the bulk dialog's LoadExistingKeysAsync). Non-secret columns only. - /// Reads the identity fields of every existing monitored server so the dedupe gate can be seeded from - /// the authoritative set. Non-secret columns only. + /// the authoritative set (mirrors the bulk dialog's LoadExistingKeysAsync). Non-secret columns only. /// /// #2218 added engine and port to the identity, so they have to be read here too. Without /// them the gate keys on a NARROWER identity than the product does, and a PostgreSQL instance on a host that