Carry engine and port in the server identity, re-keying nothing (#2218)

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- **`server_id` identity now carries engine and port, without re-keying anything that exists** ([#2218]) - the storage name was derived from host, database and read-only intent only, so a SQL Server and a PostgreSQL instance on ONE host collided into a single identity and interleaved their histories, as did two PostgreSQL instances distinguished only by port - both of which [#2213] made first-class configuration. Engine and port are now discriminators. **The interesting constraint is what could NOT change.** Lite derives `server_id` FRESH at runtime from the shared `ServerIdHelper.BuildStorageName`, everywhere, and has no stored-id fallback the way Darling does - so altering what that function returns for an EXISTING server would re-key it in Lite and orphan all of its collected history, silently: the same class of harm as [#2158], arrived at from the other direction. So the new parameters are OPTIONAL and append nothing at their defaults, which keeps Lite's three-argument call byte-identical - verified against a re-statement of the pre-change rule rather than against hand-written strings, so it holds for any input and not just the cases someone thought to list. A SQL Server entry that DOES pass them is unchanged too, which is what lets Darling pass `Engine` and `Port` unconditionally instead of branching: `Engine` folds to no token for SQL Server, and `Port` is a PostgreSQL-only field that stays 0 there because SQL Server carries a non-default port inside the host as `host,1433` and is therefore already discriminated by the host string. Darling's already-registered PostgreSQL targets do not re-key either, for a different reason: their id comes from the store and is only ever derived for an entry with no row yet - which is why [#2158] (identity assigned, never re-derived) was a prerequisite for this rather than a sibling of it. Every spelling of the engine folds to one token (`postgres`, `PostgreSQL`, `pg`, any casing), so a colleague's capitalisation cannot mint a second identity for one instance - a split history nothing downstream could diagnose - and an unrecognised engine appends nothing rather than being interpolated raw, so a typo cannot mint one either. Suffix order is fixed (engine, port, then `:RO`) so two callers supplying the same facts cannot produce two names.
+
 - **A registration connected to a database it does not name now says so** ([#2228]) - identity is registration-derived and was never checked against the connection, so a registration whose Initial Catalog is absent, misspelled or overridden lands in a different database and every collected row is stored under that registration's identity while describing somewhere else - indefinitely, with nothing anywhere saying so. When a sibling registration names that same database, both collect it and its history exists twice under two identities: [#2220]'s report of byte-identical deadlock graphs under six `server_id`s, one real incident alerting six times. Both engine probes now also return the database the connection ACTUALLY reached - `DB_NAME()` on SQL Server, `current_database()` on PostgreSQL - and the worker compares it against the registration on every connect. **Why this has to happen at connect and not in the registry**: [#2158] established that identity is assigned rather than derived, which fixes the re-key class but cannot touch this one, because the two registrations here genuinely DIFFER in configuration - no amount of care in hashing config can tell that they resolve to one database. Only the server can answer what a connection reached. The message names what is at stake (mis-attributed rows, and duplication when a sibling names the same database) and both places the setting lives, because the log line is the whole diagnosis. Logged at ERROR on the TRANSITION rather than per connect: a mismatch is a standing misconfiguration that persists until someone edits the registration, so repeating it every reconnect would bury the one line that matters, which is how a tripwire gets trained past and stops working; the recovery is logged too, so an operator who fixes it has confirmation rather than silence. **Silent in three cases on purpose**, each a false positive that would have made the feature useless: a registration naming NO database is server-scoped by design and meant to land wherever the login defaults, so it would otherwise fire on every correctly-configured server-scoped registration in the fleet; a null probe answer is unknown rather than mismatched; and comparison is case-insensitive, which is what SQL Server database names are. `DB_NAME()` needs no DMV, so it does not reintroduce the VIEW DATABASE STATE dependency [#1535] removed, and both columns are APPENDED because every read in those probes is positional. The registration-time half of the issue - refusing a new registration whose (host, actual database) pair matches an existing one - is not in this: it needs a connection during Add, which is a different surface.
 
 - **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.

Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs

@@ -54,7 +54,8 @@ public sealed class PostgresEngineGateBehaviorTests
 {
     /// <summary>
     /// The HOST is what identity derives from, which is the trap this test tripped over first:
-    /// <c>MonitoredServer.StorageName</c> is <c>BuildStorageName(Host, Database, ReadOnlyIntent)</c> — NOT
+    /// <c>MonitoredServer.StorageName</c> is <c>BuildStorageName(Host, Database, ReadOnlyIntent, Engine, Port)</c>
+    /// (#2218 added the last two) — NOT
     /// <c>Name</c> — and <c>RunAnalyzeNowAsync</c> finds a server by hashing that. A server_id hashed from
     /// anything else simply is not found, and the gate then returns "server not monitored" rather than the
     /// arm under test. Unique hosts so neither case can collide with a real server's analysis_state row.
@@ -74,9 +75,17 @@ depend on test ordering. */
     /// the ONLY thing that can decide whether it is dispatched.</summary>
     private const string SnapshotCollector = "wait_stats";
 
-    /// <summary>Derived through the SAME helper the worker uses, so the test cannot drift from the lookup.</summary>
-    private static int ServerIdFor(string host) =>
-        ServerIdHelper.GetDeterministicHashCode(ServerIdHelper.BuildStorageName(host, null, false));
+    /// <summary>
+    /// Derived through the SAME helper the worker uses, so the test cannot drift from the lookup.
+    ///
+    /// <para>#2218 is that drift happening: the ENGINE joined the derivation, and because this helper hashed
+    /// without it, a PostgreSQL host produced an id the product never uses — so the gate returned "server not
+    /// monitored" rather than the arm under test, which is the same trap the class comment above describes for
+    /// <c>Name</c> vs <c>Host</c>. The engine is defaulted to null so the SQL Server call sites stay unchanged,
+    /// exactly as the shared helper does it.</para>
+    /// </summary>
+    private static int ServerIdFor(string host, string? engine = null) =>
+        ServerIdHelper.GetDeterministicHashCode(ServerIdHelper.BuildStorageName(host, null, false, engine, 0));
 
     [Fact]
     public async Task AnalyzeNow_AgainstAPostgresTarget_WritesTheEngineTombstone_AndDoesNotRunThePass()
@@ -86,7 +95,7 @@ public async Task AnalyzeNow_AgainstAPostgresTarget_WritesTheEngineTombstone_And
             "Set DARLING_TEST_PG to a Postgres connection string to run the analyze_now engine-gate test.");
 
         await using var postgres = NpgsqlDataSource.Create(connectionString!);
-        var serverId = ServerIdFor(PgHost);
+        var serverId = ServerIdFor(PgHost, "postgres");
 
         /* Fabricated worker, the CollectorMemoryKnobTests.SweepGate idiom: the real ctor wants a host's worth
            of dependencies, and the gate under test reads exactly three fields. Reflection because pinning the
@@ -268,7 +277,7 @@ public async Task SnapshotNow_AgainstAPostgresTarget_DispatchesNoSqlServerCollec
             "Set DARLING_TEST_PG to a Postgres connection string to run the snapshot_now engine-gate test.");
 
         await using var postgres = NpgsqlDataSource.Create(connectionString!);
-        var serverId = ServerIdFor(PgSnapshotHost);
+        var serverId = ServerIdFor(PgSnapshotHost, "postgres");
 
         var bodySucceeded = false;
         try
@@ -356,7 +365,7 @@ private static ServerRuntime PostgresRuntime(
         ConnectionString = $"Host={PgHost};Database=postgres;Username=monitor",
         Target = new CollectorTargetInfo { Engine = engine },
         StorageName = PgHost,
-        ServerId = ServerIdFor(PgHost),
+        ServerId = ServerIdFor(PgHost, "postgres"),
     };
 
     private static void SetField(DarlingWorker worker, string name, object value) =>

Darling/Darling.Tests/PostgresTargetConfigTests.cs

@@ -177,8 +177,18 @@ public void DerivesStorageIdentityThroughTheSharedRule()
         server.Database = "segments_horizon";
         server.ReadOnlyIntent = true;
 
+        /* #2218: ":pg" sits between the database and ":RO". A PostgreSQL instance and a SQL Server on one host
+           used to derive ONE server_id and interleave their histories; the engine token is what separates them,
+           and its position is fixed so two callers with the same facts cannot produce two names. */
         Assert.Equal(
-            "segments-multi-1.cluster-x.us-east-1.rds.amazonaws.com:segments_horizon:RO",
+            "segments-multi-1.cluster-x.us-east-1.rds.amazonaws.com:segments_horizon:pg:RO",
+            server.StorageName);
+
+        /* A port appends after the engine when one is set — the second half of #2218, for two PostgreSQL
+           instances on one host. */
+        server.Port = 6432;
+        Assert.Equal(
+            "segments-multi-1.cluster-x.us-east-1.rds.amazonaws.com:segments_horizon:pg:6432:RO",
             server.StorageName);
     }
 

Darling/Darling.Tests/ServerIdentityFromStoreTests.cs

@@ -245,7 +245,11 @@ public async Task TheLoadedServerCarriesTheStoresOwnId_NotAFreshHash()
 
         /* The seed writes the derivation, which is what makes every existing store migration-free. Assert it
            rather than assume it -- if this ever stops holding, the "no data moves" claim stops holding too. */
-        var seededId = Derived("identity-host", "identityDb", readOnlyIntent: true);
+        /* #2218: derived from the SERVER'S OWN StorageName rather than from a re-statement of the rule here.
+           This test previously hashed (host, database, readOnlyIntent) by hand, which silently stopped matching
+           the moment the derivation grew the engine and port the seeded server actually carries — and the
+           failure reads as "the seed wrote the wrong id" rather than "the test's copy of the rule is stale". */
+        var seededId = ServerIdHelper.GetDeterministicHashCode(seeded.StorageName);
         Assert.Equal(seededId, await ReadServerIdByNameAsync(dataSource, "identity-roundtrip", ct));
 
         /* Now the thing production cannot produce yet: re-key the row so the stored id disagrees with the hash

Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs

@@ -1172,11 +1172,20 @@ public sealed class MonitoredServer
     public string DisplayName => string.IsNullOrWhiteSpace(Name) ? Host : Name;
 
     /// <summary>
-    /// The canonical storage identity (host[:database][:RO]) — hashed to server_id via the shared
-    /// ServerIdHelper, so this Darling entry derives the same id Lite would for the same server.
+    /// The canonical storage identity (<c>host[:database][:pg][:port][:RO]</c>) — hashed to server_id via the
+    /// shared ServerIdHelper, so this Darling entry derives the same id Lite would for the same server.
+    ///
+    /// <para>#2218: engine and port are passed so a PostgreSQL instance cannot collide with a SQL Server on the
+    /// same host, and two PostgreSQL instances on one host cannot collide with each other. Both are inert for a
+    /// SQL Server entry and the resulting name is byte-identical to what it was before — <c>Engine</c> folds to
+    /// no token for SQL Server, and <c>Port</c> is a PostgreSQL-only field that stays 0 there, because SQL
+    /// Server carries a non-default port inside <c>Host</c> as <c>host,1433</c> and is therefore already
+    /// discriminated by the host string. That is why no conditional is needed here: the defaults ARE the
+    /// backwards-compatible case.</para>
     /// </summary>
     [JsonIgnore]
-    public string StorageName => PerformanceMonitor.Common.ServerIdHelper.BuildStorageName(Host, Database, ReadOnlyIntent);
+    public string StorageName => PerformanceMonitor.Common.ServerIdHelper.BuildStorageName(
+        Host, Database, ReadOnlyIntent, Engine, Port);
 
     /// <summary>
     /// <c>config_monitored_servers.server_id</c> as READ FROM THE STORE, or null for an entry that has no