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

[erikdarlingdata]

Closes #2258 — without the tombstone table or the migration rung it proposed, because the fact both would record already exists.

The ambiguity

A server named in darling.json but absent from the registry had two indistinguishable causes:

1. added to the file after the first seed and never registered — the [QUESTION] Add new server to monitoring #2252 field report, where the operator expects monitoring and isn't getting it
2. registered once and then removed via the Viewer, with the file left alone — a correct state

So the reconciliation reported at Information and worded itself for both. Honest, but it could neither warn about the first nor stay calm about the second, and a naive fix that warned about both would tell the operator to re-add a server they'd deliberately dropped, at every startup forever.

No tombstone needed — collect.servers already is one

The issue proposed a config_removed_servers table or an is_removed flag, plus a rung, and noted it wasn't worth a rung on its own. It doesn't need one:

• collect.servers — the observed registry — gets a row upserted on every successful connect
• the Viewer's Remove deletes only from config_monitored_servers, the desired config
• nothing purges the observed registry, because it's a registry rather than a time series (verified against the retention purge list, not assumed)

So "was this server ever really monitored" is answerable today, for free, with no schema change and — more importantly — no second piece of state that could disagree with the first.

The is_removed flag was worth rejecting explicitly rather than merely not choosing. is_enabled = FALSE already means "registered but paused", so a second flag makes (is_enabled, is_removed) a four-state space where two combinations are meaningless, and every existing reader of that table would have to learn the new flag or silently start including removed servers. That's the same seam failure #2280 had to fix in the dedupe gate — a widened concept that old call sites never learned about. I'd rather not manufacture another one for a cosmetic gain.

What it reports now

• Warning — "listed in darling.json, NOT monitored and never have been", naming them and saying that file edits don't register a server and a restart won't change it. This is the case where something the operator wants isn't happening.
• Information — "were monitored and have since been removed. That is expected." Advises nothing, and notes their collected history is kept, because "removed" reads as "deleted" to most people.

Two lines with different severities rather than one blended list, because a single list forces the sharper cause to inherit the calmer wording.

The limits, documented on the method

Stated rather than left to be discovered, since they bound what the log may claim:

• A server registered but never successfully connected to has no observed row, so it reports as never-monitored. That's the right answer to "is this being monitored?" even though it's the wrong answer to "was it ever registered?" — and the operator's question is the former.
• A rebuilt store has no observed rows at all, so everything reads as never-monitored until it connects once. That degrades to a warning rather than to silence, which is the safe direction for a fresh store where the file genuinely is the declared intent.
• Matched on either name, for the reason ServersOnlyInFile already gives: identity drift predating [BUG] Editing a server's Host/Database/ReadOnlyIntent re-keys its runtime server_id — registry PK orphaned, collected history split #2158 makes the id the least reliable of the three keys, and a miss here warns rather than going quiet.

Testing

SplitByEverMonitored is pure, so the truth table is ordinary tests, and I executed it locally — all green: both causes in one pass with order preserved, case-insensitivity, the empty-registry (fresh store) case landing in the warning arm, nothing-to-report, and null tolerance on a startup diagnostic path.

The two log arms and their severities are pinned at the source, including that the removed arm advises nothing and does say the history is kept.

Full solution builds, 0 warnings; doc-comment detector clean (run as its own gate).

🤖 Generated with Claude Code

[claude]

Review summary

Reviewed the diff (CHANGELOG.md, Darling/PerformanceMonitor.Darling.Service/StoreConfigProvider.cs, Darling/Darling.Tests/FileOnlyServerCauseSplitTests.cs). No blocking issues found.

Correctness — SplitByEverMonitored is a small, pure function with well-chosen edge-case tests (null file list, null observed set, case-insensitive matching, empty registry, both causes in one pass, and a source-pinning test on the log wording). I traced the surrounding claims against the actual write paths:

• collect.servers is confirmed to get a row on every successful connect (DarlingObservability.UpsertServerAsync/UpsertServerSql), and a removed server's observed row is flipped to is_enabled = FALSE rather than deleted (SyncEnabledStatesSql / the deleted-server mirror) — so the "the observed registry already is the tombstone" premise holds.
• storeIds/storeNames (used to compute fileOnly) are read from config_monitored_servers with no is_enabled filter, so a merely-paused server is correctly never treated as file-only in the first place — only a truly-removed-from-config-table server reaches the new split.
• The new SELECT display_name, server_name FROM collect.servers query is unconditionally schema-qualified (collect.servers), unlike the pre-existing unqualified config_monitored_servers reads elsewhere in the file that rely on search_path — a nice small robustness improvement, not a regression.
• Everything here runs once per service start (only reached when config_monitored_servers is non-empty) inside the existing try/catch in SeedIfEmptyAsync, so a store hiccup degrades to a warning log rather than blocking startup, consistent with the file's existing failure-isolation posture.

Lite/Darling parity — no drift. Lite has no equivalent to this feature: it has a single servers.json that is the authoritative server list (ServerManager.cs), not a separate file-vs-database duality the way Darling has darling.json vs. config_monitored_servers/collect.servers. Lite's DuckDB servers table exists in schema but is unused/vestigial, and Lite.Tests/CrossAppMcpToolInventoryPinTests.cs already documents that server-registry concepts here are "Darling-ONLY by architecture." Nothing to port.

Security — no new attack surface. No user input reaches the new query (it's a fixed SELECT against a fixed table), and it's read-only.

Performance — negligible: one extra SELECT against a registry-sized table, once at startup.

Nothing else stood out.