Refuse a registration that lands on an already-monitored database (#2280)

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

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;
+
+/// <summary>
+/// #2280: <c>add_servers</c> 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.
+///
+/// <para><b>The defect being prevented.</b> 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 <c>server_id</c>s, 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.</para>
+///
+/// <para><b>Why the probe is what makes it possible.</b> 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 <c>add_servers</c> already probes
+/// in-process, so the answer is in hand at exactly the moment the decision has to be made.</para>
+/// </summary>
+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<string> Claimed(params string[] keys) =>
+        new(keys, StringComparer.OrdinalIgnoreCase);
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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));
+    }
+
+    /// <summary>
+    /// THE VALID PAIR THAT MUST STILL BE ALLOWED: a read-only-intent registration alongside a read-write one for
+    /// the same database. <c>read_only_intent</c> 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.
+    /// </summary>
+    [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));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    /// #2218's regression in this method, pinned: the duplicate gate reads <c>engine</c> and <c>port</c>, so it
+    /// keys on the identity the store actually derives.
+    ///
+    /// <para>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.</para>
+    /// </summary>
+    [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));
+    }
+}

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)
 {
     /// <summary>
     /// Rebuilds the gate's-eye view of this target, so a caller can ask which collectors would actually