Tell 'never registered' from 'deliberately removed' (#2258)

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- **The darling.json reconciliation now tells "never registered" from "deliberately removed"** ([#2258]) - a server named in the file but absent from the registry had two indistinguishable causes: added to the file after the first seed and never registered (the [#2252] field report - the operator expects monitoring and is not getting it), or registered once and then removed via the Viewer, which is a CORRECT state. So the line reported at Information and worded itself for both, which was honest but could neither warn about the first nor stay calm about the second. It is now two lines: a WARNING naming the servers that were never monitored and what to do about it, and an Information line for the ones that were removed on purpose which advises nothing and notes their collected history is kept. **No tombstone table and no migration rung, which is the point.** The issue proposed a `config_removed_servers` table or an `is_removed` flag; the fact both would record already exists - `collect.servers`, the OBSERVED registry, gets a row on every successful connect, the Viewer's Remove deletes only from `config_monitored_servers` (the DESIRED config), and nothing purges the observed registry because it is a registry rather than a time series. So the distinction is answerable today, with no schema change and no second piece of state that could disagree with the first. The `is_removed` flag was worth rejecting explicitly: `is_enabled = FALSE` already means "registered but paused", so a second flag makes `(is_enabled, is_removed)` a four-state space with two meaningless combinations, and every existing reader of that table would have to learn the new flag or silently start including removed servers - the same seam failure [#2280] had to fix in the dedupe gate. **The limits are stated on the method rather than left to be discovered**: a server registered but never successfully connected to reads as never-monitored, which is the right answer to "is this being monitored?" even though it is the wrong answer to "was it ever registered?"; and a rebuilt store has no observed rows, so everything reads as never-monitored until it connects once - degrading to a warning rather than to silence, which is the safe direction for a fresh store where the file genuinely is the intent.
+
 - **PostgreSQL statement text is stored, so `get_pg_top_queries` returns something readable** ([#2219]) - `pg_statement_stats` identified queries by `queryid` and stored no text, because `aurora_stat_statements`' `showtext` is a real per-collection cost and normalized text is highly repetitive. But `queryid` is **not stable across a major version upgrade**, so afterwards the stored history joined to nothing readable: a list of integers that used to be your slowest queries, and unrecoverable - the live view no longer holds the old ids, and anything else on the instance may have reset `pg_stat_statements` out from under us (pganalyze's collector calls `pg_stat_statements_reset()` on a size budget). V73 adds `collect.pg_statement_text`, one row per `(server_id, queryid)`, refreshed hourly and joined into the read. **Text is INLINE rather than a `query_text_dim` digest, which reverses what V64's comment promised, deliberately.** The dimension route is blocked and expensive to unblock - V38 is GENERATED from `PayloadDimensions.All`, so registering the fact table makes V38 `ALTER` a table it has not created yet on every upgraded store, and it would break V64's own ladder diff - but the deciding reason is that the dimension needs the GC liveness interlock `QueryStorePlanMap` documents at length, whose failure mode is **silently missing text**. Inline cannot dangle. The cost is cross-server dedup: one row per server per queryid rather than one per distinct text, a few hundred MB on the measured 52-server fleet against a store whose Query Store plan XML alone was 43 GB. Paying that to make a silent-loss mode impossible is the trade. **Idempotent by construction**, which is what makes the cadence a free choice: every refresh upserts the same rows, so re-fetching costs one statement and no growth - there is no "which queryids do I have" bookkeeping to get wrong and no watermark to corrupt. Due-ness is asked of the STORE rather than remembered in the service, so a restart cannot re-fetch the fleet and two hosts cannot disagree about when text was last written. `first_seen` is preserved on conflict because it is the one fact that survives a major-version re-key and cannot be reconstructed; `query_text` advances. Hung off the statement-stats collector's success rather than given its own loop - it is meaningless without those rows and must never run against a server whose stats collection is failing - and best-effort, because unreadable text is a degraded read while a failed collection is lost data. The prune margin makes text **outlive** the statistics referencing it, the opposite direction from the plan map's, because the asymmetry is the other way round: text kept past its facts is dead bytes, facts kept past their text is the list-of-integers failure this table exists to fix. Not a hypertable and not a collector table, exactly like V72's plan map - a bespoke upsert path, pruned on `last_seen`. `query_text` is **null**, never an empty string, when nothing has been captured for a queryid yet, so a caller cannot read a blank as the query.
 
 - **`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.
@@ -2739,6 +2741,7 @@ 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
+[#2258]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2258
 [#2219]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2219
 [#2280]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2280
 [#2277]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2277

Darling/Darling.Tests/FileOnlyServerCauseSplitTests.cs

@@ -0,0 +1,181 @@
+/*
+ * 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 Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #2258: telling "never registered" from "deliberately removed" in the darling.json reconciliation.
+///
+/// <para><b>The ambiguity.</b> A server named in the file but absent from the registry has two indistinguishable
+/// causes: it was added to the file after the first seed and never registered (the #2252 field report — the
+/// operator expects monitoring and is not getting it), or it was registered once and then removed via the Viewer,
+/// which is a CORRECT state. The reconciliation therefore reported at Information and worded itself for both —
+/// honest, but it could neither warn about the first nor stay calm about the second.</para>
+///
+/// <para><b>No tombstone table was needed, which is the interesting part.</b> #2258 proposed a
+/// <c>config_removed_servers</c> table or an <c>is_removed</c> flag, plus a migration rung. But the fact it wanted
+/// is already recorded: <c>collect.servers</c> — the OBSERVED registry — gets a row on every successful connect,
+/// the Viewer's Remove deletes only from <c>config_monitored_servers</c> (the DESIRED config), and nothing purges
+/// the observed registry because it is a registry rather than a time series. So the distinction is answerable
+/// today, with no schema change and no second piece of state that could disagree with the first.</para>
+///
+/// <para>An <c>is_removed</c> flag was worth rejecting explicitly rather than merely not choosing:
+/// <c>is_enabled = FALSE</c> already means "registered but paused", so a second flag makes
+/// <c>(is_enabled, is_removed)</c> a four-state space with two meaningless combinations, and every existing reader
+/// of that table would have to learn the new flag or silently start including removed servers — the same seam
+/// failure #2280 had to fix in the dedupe gate.</para>
+/// </summary>
+public sealed class FileOnlyServerCauseSplitTests
+{
+    private static HashSet<string> Observed(params string[] names) =>
+        new(names, StringComparer.OrdinalIgnoreCase);
+
+    /// <summary>
+    /// THE FIELD REPORT (#2252): in the file, never observed. That is the case where something the operator wants
+    /// is not happening, so it must be the one that warns.
+    /// </summary>
+    [Fact]
+    public void AServerNeverObservedIsReportedAsNeverRegistered()
+    {
+        var (never, removed) = StoreConfigProvider.SplitByEverMonitored(
+            new[] { "new-server" }, Observed("some-other-server"));
+
+        Assert.Equal(new[] { "new-server" }, never);
+        Assert.Empty(removed);
+    }
+
+    /// <summary>
+    /// THE CORRECT STATE: in the file, and the observed registry remembers it. The operator removed it on purpose
+    /// and left the file alone — advising them to re-add it would be wrong advice, repeated at every startup
+    /// forever, which is exactly what the old single message risked.
+    /// </summary>
+    [Fact]
+    public void AServerTheStoreHasObservedIsReportedAsDeliberatelyRemoved()
+    {
+        var (never, removed) = StoreConfigProvider.SplitByEverMonitored(
+            new[] { "retired-server" }, Observed("retired-server"));
+
+        Assert.Empty(never);
+        Assert.Equal(new[] { "retired-server" }, removed);
+    }
+
+    /// <summary>
+    /// Both causes in one reconciliation, which is the normal case on a fleet that has been running a while —
+    /// and the reason the two are reported as separate lines with different severities rather than one blended
+    /// list. A single list would force the sharper cause to inherit the calmer wording.
+    /// </summary>
+    [Fact]
+    public void BothCausesAreSeparatedInOnePass()
+    {
+        var (never, removed) = StoreConfigProvider.SplitByEverMonitored(
+            new[] { "new-a", "retired-b", "new-c", "retired-d" },
+            Observed("retired-b", "retired-d", "unrelated"));
+
+        Assert.Equal(new[] { "new-a", "new-c" }, never);
+        Assert.Equal(new[] { "retired-b", "retired-d" }, removed);
+    }
+
+    /// <summary>
+    /// Matched case-insensitively, because the observed registry's names come from a different write path than
+    /// the file's and neither normalizes case. A case difference is not a different server, and treating it as
+    /// one would warn about a deliberately-removed server forever.
+    /// </summary>
+    [Theory]
+    [InlineData("Retired-Server", "retired-server")]
+    [InlineData("retired-server", "RETIRED-SERVER")]
+    public void NamesMatchCaseInsensitively(string fileName, string observedName)
+    {
+        var (never, removed) = StoreConfigProvider.SplitByEverMonitored(
+            new[] { fileName }, Observed(observedName));
+
+        Assert.Empty(never);
+        Assert.Single(removed);
+    }
+
+    /// <summary>
+    /// An EMPTY observed registry sends everything to the warning arm. That is the fresh-store case — nothing has
+    /// connected yet — and it is the safe direction: on a store where the file genuinely is the declared intent,
+    /// a warning that these are not yet monitored is correct, whereas silence would hide it.
+    /// </summary>
+    [Fact]
+    public void AnEmptyObservedRegistryWarnsRatherThanGoingQuiet()
+    {
+        var (never, removed) = StoreConfigProvider.SplitByEverMonitored(
+            new[] { "a", "b" }, Observed());
+
+        Assert.Equal(new[] { "a", "b" }, never);
+        Assert.Empty(removed);
+    }
+
+    /// <summary>Nothing file-only means nothing to report, in either arm.</summary>
+    [Fact]
+    public void NoFileOnlyServersProducesNothing()
+    {
+        var (never, removed) = StoreConfigProvider.SplitByEverMonitored(
+            Array.Empty<string>(), Observed("anything"));
+
+        Assert.Empty(never);
+        Assert.Empty(removed);
+    }
+
+    /// <summary>Null inputs are not a crash on a startup diagnostic path.</summary>
+    [Fact]
+    public void NullInputsAreTolerated()
+    {
+        var (never, removed) = StoreConfigProvider.SplitByEverMonitored(null!, Observed("x"));
+        Assert.Empty(never);
+        Assert.Empty(removed);
+
+        var (never2, removed2) = StoreConfigProvider.SplitByEverMonitored(new[] { "a" }, null!);
+        Assert.Equal(new[] { "a" }, never2);
+        Assert.Empty(removed2);
+    }
+
+    /// <summary>
+    /// The two arms carry the severities their causes deserve, and the observed registry is what they read —
+    /// pinned at the source because the log calls need a live store to exercise.
+    ///
+    /// <para>The severity split IS the feature: the old single Information line could not warn about a server
+    /// the operator wanted monitored, and a naive fix that warned about both would have told them to re-add a
+    /// server they deliberately dropped, at every startup.</para>
+    /// </summary>
+    [Fact]
+    public void TheWarningAndTheInformationArmsAreSplitByCause()
+    {
+        var source = ReadProviderSource();
+
+        Assert.Contains("SELECT display_name, server_name FROM collect.servers", source, StringComparison.Ordinal);
+        Assert.Contains("NOT monitored and never have been", source, StringComparison.Ordinal);
+        Assert.Contains("LogWarning(", source, StringComparison.Ordinal);
+        Assert.Contains("were monitored and have since been removed", source, StringComparison.Ordinal);
+        /* The removed arm must not advise anything — it is a correct state. */
+        Assert.Contains("That is expected", source, StringComparison.Ordinal);
+        /* And it must still say their history is kept, since "removed" reads as "deleted" to most people. */
+        Assert.Contains("collected history is", source, StringComparison.Ordinal);
+    }
+
+    private static string ReadProviderSource([CallerFilePath] string thisFile = "")
+    {
+        var dir = Path.GetDirectoryName(thisFile)!;
+        var relative = Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "StoreConfigProvider.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));
+    }
+}