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

[erikdarlingdata]

Closes #2280 — the registration-time half of #2228, whose connect-time tripwire shipped in #2277. It also fixes a regression #2218 introduced in this same method; that half is arguably the more urgent one.

The collision check

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_ids, one incident alerting six times. #2277 reports that at connect; this refuses it at the point of creation, which is the only place it can be prevented rather than described.

add_servers already probes in-process, and since #2277 the probe 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: that's precisely why #2158 and #2218 (identity assigned, then widened) couldn't touch this defect.

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
• a PostgreSQL instance alongside a SQL Server on one host

Silent when the probe reports no database (unknown is not colliding — refusing there blocks a registration for a reason nobody can act on) and when the entry reaches the database it names (the ordinary case, already ruled on by the declared-duplicate gate).

The regression this also fixes

The duplicate gate built its key from host, database and read-only intent only:

var storageKey = ServerIdHelper.BuildStorageName(host, database, readOnlyIntent);   // 3-arg

Once #2218 added engine and port to the identity, that gate was keying on a narrower identity than the store derives — so 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 couldn't see what distinguished them.

It compiled, and every existing test passed. ExistingServersSql didn't even select the columns, so there was nothing to notice. It now reads engine, port and keys on the full identity, with an explicit pin — and the pin asserts the two identities genuinely differ, so the columns are load-bearing rather than decorative.

This is the seam lesson from the repo's own skill doc, arriving on schedule: my #2218 change was correct in itself and every call site that re-derived identity was individually tested, but a call site that never learned the concept had widened was silently wrong.

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." 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 rather than the whole answer. Documented on the method rather than left for someone to discover.

Also not in scope, deliberately, and recorded on the issue: the file seed should keep the tripwire only. A server silently unmonitored because of a collision at service start is worse than one collected twice with a loud log — and refusing there would mean a startup-time decision nobody is present to see.

Testing

ActualIdentityCollision is pure, so the whole truth table is ordinary tests, and I executed it locally — all green, including the two valid pairs that must not be refused (RO-vs-RW, PostgreSQL-vs-SQL Server) and the port variant. Those got the most attention because a false refusal here blocks legitimate work, which is worse than the duplicate it prevents.

Full solution builds, 0 warnings.

One process note. My doc-comment detector caught a displaced block before the push and I pushed anyway — I'd chained the detector into the same command as the commit, so the commit never waited on its result. Fixed in a follow-up commit here (the block was moved back to PartitionDuplicates, not deleted, since it was that method's own summary). Running the detector as its own step is the real fix; knowing a trap and owning a checker are both worthless if the output arrives after the thing it was meant to gate.

🤖 Generated with Claude Code

[claude]

claimed is seeded once from the store plus every ready entry's declared identity, before any probing happens, and is never updated with an entry's actual (connected) database as entries are processed in the loop below.

That closes the case where entry B's declared database matches entry A's already-claimed declared identity (or an existing store row) — but it doesn't close the case where two new entries in the same batch both declare different, non-colliding database names yet both actually resolve (per their own probe) to one real database that neither of them names. Each entry's probeResult.ConnectedDatabase is known at the moment it's probed, but that fact is never folded back into claimed, so the second entry's ActualIdentityCollision check can't see the first entry's actual landing spot — only its declared one.

This is a narrower variant of the "honest limit" the PR documents for pre-existing rows (which genuinely lack a persisted actual-database column), but for entries within the same add_servers call the actual database is available in-memory as each one is probed — it's just not being tracked. Given this is precisely the #2220 shape (N registrations silently resolving to one database), and add_servers explicitly supports bulk onboarding in one call, this seems worth closing by adding each successfully-processed entry's actual-database key to claimed after it clears the check (in addition to the pre-seeded declared keys).

[claude]

New "collides" status, but Aggregate (further down, unchanged by this PR) only counts failed as status is "connection_failed" or "invalid" — "collides" falls into none of added/skipped/failed. A batch containing a collision will report added + skipped + failed < results.Count: the collided entry is present in results but silently excluded from every top-line tally, so a caller that only reads the summary counts (rather than walking results) won't notice it happened at all — which undercuts the point of refusing it instead of just describing it.

None of the existing tests catch this: RegistrationCollisionTests only exercises the pure ActualIdentityCollision function directly, and the live AddServersAsync test that does assert the added/skipped/failed sum (DarlingMcpServerAdminToolsTests.cs:319-325) has no collision case in its fixture. Worth folding "collides" into failed (or giving it its own counter).

Separately, the tool's [Description] above (lines ~90-96) still enumerates only "added"|"duplicate"|"connection_failed"|"invalid" as possible status values — "collides" isn't documented there either, so the MCP client has no way to know this outcome exists.

[claude]

Reviewed the diff. This is a well-scoped, well-tested fix for the #2280/#2218 collision gate — the ServerIdHelper.BuildStorageName regression pin (TheDuplicateGateReadsTheFullIdentity) and the pure ActualIdentityCollision test suite cover the stated cases thoroughly (RO/RW twin, Postgres-vs-SQL Server, unknown-database silence, server-scoped entries). No Lite parity issue: add_servers/remove_server are confirmed Darling-only by architecture (CrossAppMcpToolInventoryPinTests), and BuildStorageName itself (the shared identity helper both SKUs use) is untouched by this PR.

Left two inline comments on DarlingMcpServerAdminTools.cs:

1. Aggregate drops the new "collides" status from its tallies — failed only counts connection_failed/invalid, so a collision is present in results but invisible in added/skipped/failed, and the tool's own [Description] doesn't document "collides" as a possible status either. No existing test exercises the full AddServersAsync → Aggregate path with a collision, which is how this slipped through.
2. Intra-batch collision gap — claimed is seeded from declared identities only, before any probing, and never updated with each entry's actual connected database as the batch is processed. Two new entries in the same add_servers call that declare different names but both actually resolve to a third, undeclared database won't be caught, even though each one's actual database is known at the moment it's probed. This is a narrower case than the documented "existing rows only record what they declare" limitation, since the in-batch data is available and just isn't being folded back into the claimed set.

Neither is a security issue; both are correctness gaps in the collision-detection feature this PR adds.

[claude]

claimed is seeded once from every ready entry's declared identity, before any of them are known to actually connect or survive the collision check — so a batch-mate that never ends up in the store can still cause a false-positive refusal.

Concretely: entry B declares host:Foo but has a bad password (connection_failed — never inserted); entry A declares host:Bar but its probe reports it actually connects to Foo. B's declared key host:Foo was added to claimed up front (line 143), so when A is checked with ActualIdentityCollision, A's actual key matches B's declared key in claimed and A gets refused with "collides" — even though B never occupies that identity in the store (it failed to connect). The same thing happens if B itself gets rejected by this same collision check rather than failing to connect: its declared key still lingers in claimed for every entry processed after it.

The set is built from what the batch merely proposes (ready), not from what will actually be persisted. Since the loop already processes entries sequentially and inserts as it goes, claimed could instead be updated with each entry's key only after it succeeds (or the pre-seeded declared keys could be pruned when an entry is later determined not to be inserted), so a later entry is never blocked by an earlier one's identity unless that earlier one actually took it.

This is a narrow window — needs a coincidental name collision within one batch plus a failing/rejected sibling — but it's a real false refusal of an otherwise-valid registration, and none of the new tests exercise it (they all call ActualIdentityCollision directly with a hand-built claimed set, never through AddServersAsync's real seeding).

[claude]

New "collides" status isn't wired into the aggregate counts or the tool's documented contract.

Aggregate() (further down in this file) only buckets "added", "duplicate", and "connection_failed"/"invalid" into added/skipped/failed:

added = ordered.Count(r => r.Status == "added"),
skipped = ordered.Count(r => r.Status == "duplicate"),
failed = ordered.Count(r => r.Status is "connection_failed" or "invalid"),

"collides" falls into none of them, so a batch with a collision silently produces added + skipped + failed < results.Length — e.g. 3 entries in, one collides, and the summary reads added: 2, skipped: 0, failed: 0 even though only 2 of 3 servers actually landed in the store. An MCP client (or an LLM) that trusts the top-level counts rather than walking results[] will believe the batch fully succeeded.

The tool's Description above (~line 95) has the same gap: the documented status enum is still "added"|"duplicate"|"connection_failed"|"invalid" — "collides" isn't listed.

Suggest folding "collides" into failed (semantically it is one — the server wasn't added) and adding it to the Description enum. Worth a regression test through AddServersAsync end-to-end, since the existing RegistrationCollisionTests only exercise the pure ActualIdentityCollision function directly and never check the aggregate counts a real collision produces.

[claude]

Reviewed. Left two inline findings on DarlingMcpServerAdminTools.cs, both in the new #2280 registration-collision path:

1. The new "collides" status isn't counted by Aggregate() — added/skipped/failed only bucket the pre-existing statuses, so a batch containing a collision under-reports its total in the summary (results[] has it, the top-level counts don't). The tool's Description enum of possible statuses also doesn't mention "collides".
2. The claimed identity set used for the collision check is seeded from every ready entry's declared identity up front, before probing — so an entry that later fails to connect (or is itself rejected as a collision) still "occupies" its declared identity for every entry checked after it in the same batch, which can produce a false-positive refusal of an otherwise-valid registration. Narrow window (needs a same-batch coincidence), but real, and untested — the new tests only call ActualIdentityCollision directly with a hand-built claimed set, never through the real AddServersAsync pipeline.

Everything else looks solid: the collision-check logic itself (ActualIdentityCollision), the full-identity fix to the duplicate gate (engine/port), and the ConnectedDatabase plumbing through ConnectionProbeResult are all correct and well-covered. No Lite/Darling parity issue — this feature is inherent to Darling's central multi-server store and has no Lite analogue (Lite derives identity fresh per-connection with no cross-registration store to collide against). No SQL injection surface (constant SQL text, parameterized inserts elsewhere), no secret handling changes.