Route from CLUSTER SLOTS, and give endpoints a lifecycle (#3177)
* Resolve servers by any identity a node answers to servers is keyed on exact endpoint equality, so a node held under its address was unreachable by its announced hostname - and a redirect naming it that way became a second ServerEndPoint for one node, doubling connections and splitting backlog and subscription state across the pair (#2826). - _serverIdentities maps secondary names to the one ServerEndPoint, kept separate from the servers table so exact keying is untouched - TryResolveServerEndPoint: exact hit, then identity. Used by the public GetServer, which previously threw for a node known by its other name, and by the internal GetServerEndPoint, which resolves before creating - RegisterServerIdentities records the other names of nodes we already know, and deliberately creates nothing: an unheard-of node is discovery's business, not identity's Autoconfigure now asks for CLUSTER SLOTS *before* CLUSTER NODES. Replies arrive in request order, so the identities are registered before NODES can create anything by address; without it, a node created from a redirect under its hostname was duplicated under its address moments later by its own autoconfigure, which is exactly what the redirect test demonstrated. Costs nothing - the burst remains a single pipeline with no round-trip stall - but the ordering is now load-bearing, and commented as such in both places. Also adds AssertOneEndpointPerNode: an invariant check over the shadow topology - no node-id may be held under two endpoints - asserted from state rather than from a scenario, so it catches duplication however it arose. That matters because two creators still exist structurally: ReconfigureAsync reads CLUSTER NODES independently of autoconfigure, and that window is not deterministically reproducible. * Drive the slot map from CLUSTER SLOTS Autoconfigure asks for CLUSTER SLOTS (ahead of CLUSTER NODES, per the previous commit), and the resulting id-keyed topology now feeds ServerSelectionStrategy where the answering server supplied one; the NODES-derived path remains as the fallback and still drives node relations. The reason this is worth doing rather than merely equivalent: the SLOTS view carries the node-id and both naming forms, so applying it resolves each node through every identity it answers to before considering the form this particular reply happened to use. The NODES path keyed on one endpoint and so created a second ServerEndPoint whenever a node arrived under its other name. Where a node is genuinely new, an address is preferred over a hostname - the address is dialable as-is, whereas a hostname is only usable if it resolves. Tests prove the flip took effect rather than the two views merely agreeing: a slot is migrated such that routing can only be correct if SLOTS is what feeds the map, and a hostname-preferring cluster (where SLOTS names every node by hostname while NODES names them by address) now routes with two endpoints for two nodes rather than four. Also adds the autoconfigure wiring test, since the existing coverage deliberately sourced the reply explicitly. ClusterTopologyShadowUnitTests renamed: it is no longer shadowing anything. Its agreement-with-NODES assertions are kept on purpose - NODES is no longer what routes, but it is still the public admin surface, and disagreement would mean one of the two is wrong. * Discover cluster endpoints from CLUSTER SLOTS, registering the rest inert Reconfigure now asks both topology commands of the connected server, in one pipelined pair rather than trusting the view cached by autoconfigure: re-reading one half while acting on possibly-stale data for the other would be worse than either choice on its own. CLUSTER SLOTS drives discovery, so the nodes that serve traffic are the ones we connect to, resolved through every identity they answer to first so that a node already held under another name is not duplicated. CLUSTER NODES then contributes what SLOTS cannot: a node serving no slots does not appear in the SLOTS reply at all, so those are registered with activate:false - known, in GetEndPoints(), addressable via GetServer, but not dialled, since there is nothing to route to them. Nothing is lost by that: Activate is only GetBridge(create:true) and GetBridge(Message) creates unconditionally, so the first command sent to such a node connects it then. Sentinel already used activate:false for a new primary, so the pattern is established rather than novel. If SLOTS yields nothing usable - a pre-4.0 server, or an error reply - discovery falls back to exactly the previous behaviour, with every NODES endpoint active. Node relations continue to come from NODES regardless: SLOTS conveys replica-of by position, but not the ids and flags that Primary/Replicas resolution reads. ClusterHandshakeNodesAreIgnored passes unchanged, which was the point of the dual source - an empty node stays reachable, it simply is not dialled. New tests cover both directions: a slot-less node is known but reports IsConnected false until used, and a node that owns slots is still connected eagerly. * Add a graceful server-retirement primitive Nothing could previously remove a ServerEndPoint: ServerSnapshot had Add and no counterpart, nothing called servers.Remove, and Dispose tears the bridges down immediately - abandoning anything in flight. Four separate requirements want the same drain-then-close (topology pruning, duplicate merging, and both endpoint handoffs in the maintenance-notification work), so it is built once here, with no policy attached and no caller yet. ServerEndPoint.RetireAsync marks itself unselectable *first*, which bounds the drain by ensuring nothing new arrives, then waits for written-and-awaiting plus backlogged work to clear before disposing. Exceeding the drain timeout is logged with the count abandoned, since that is precisely what someone will be asking about later. ConnectionMultiplexer.RetireServerAsync then forgets it, including every secondary identity that pointed at it - without that, an alias outlives its server and TryResolveServerEndPoint hands back something whose bridges are gone. That method now also refuses disposed servers, restoring a check dropped during the cherry-pick because IsDisposed was not exposed. ServerSnapshot.Remove always allocates a compacted copy and never reuses the array. Add is allowed to write into spare capacity because older readers hold a smaller count and never observe the new slot; removal shifts elements a concurrent reader may be enumerating, so the same trick is unsafe. Tests drive retirement directly: in-flight work completes across it, the server is forgotten along with its aliases, it stops being selected, retiring twice is harmless, and removing the middle of three leaves the others working. * Prune rotated-out endpoints, and merge duplicates, from the topology pass Provenance and generation tracking on ServerEndPoint, then two policies over the retirement primitive. Provenance records how we learned of a server, because "absent from the topology" only means anything if the source that would have listed it actually ran. A configured endpoint is never pruned - it is the seed needed to bootstrap after a full rotation. A sentinel-discovered one is never pruned by *cluster* absence: in a sentinel deployment no cluster topology runs at all, so a single rule would retire the entire deployment. A redirect target is legitimately ahead of the topology, so it is exempt until a topology confirms it, at which point it becomes an ordinary cluster node. Absence is counted in topology generations rather than time, so a quiet client cannot age endpoints out simply by not reconfiguring; three consecutive misses are required, since a single reply is only one node's view. A server is only retired if it is also idle - owns no slots, carries no subscriptions, owes no responses. Duplicate merge falls out of the same pass: resolving each node's identities and finding two distinct servers means one process reached under two names. The survivor is the configured one if either is, else the one matching the form the answering node advertised (Identities is ordered accordingly); the loser's name becomes an alias of the survivor, so a caller still holding it keeps resolving rather than breaking. One deviation from the design notes, recorded in the remarks on OnMissingFromTopology: they proposed also resetting the absence count whenever the server had been *used* since. That is not implementable - the only usage counter is incremented by our own heartbeat pings as well as by callers, so an idle-but-connected server never looks unused, and a first attempt at it silently prevented all pruning. It is also unnecessary, because the cases it was meant to protect are exactly the ones IsIdle already covers. * Dial the advertised identity, and keep configured provenance under ResolveDns Two corrections to the previous commits in this branch. A new node is now dialled by the form the answering node *advertised* rather than by an address in preference. The earlier choice reasoned that an address is dialable as-is - true, but backwards for the deployments this work targets: a certificate validates against a name, and where hostnames are preferred the advertised address may not be routable at all, which is #2826's premise. Identities is already ordered with the advertised form first, so this is a deletion rather than an addition. And a bug found while checking the TLS story: ResolveDns rewrites the multiplexer's working set at startup, replacing configured DnsEndPoints with the addresses they resolved to, while RawConfig keeps the original names. Provenance tested only RawConfig, so with ResolveDns enabled a *configured* endpoint was classified as discovered - and therefore prunable. It now tests both collections. Both cases have regression tests. Also relevant, having checked it rather than assumed: the SslHost derivation hazard from the design notes does not apply here. ConfigurationOptions.SslHost falls back to a value derived from ConfigurationOptions.EndPoints, and nothing in this branch mutates that collection - the multiplexer works on a clone, discovery adds to the server table, and even sentinel's Clear/TryAdd hit the clone. So the derived TLS host cannot flap as endpoints come and go. * Merge origin/main; do not reference ValueTuple from the library The duplicate-merge bookkeeping used List<(ServerEndPoint, ServerEndPoint)>, which puts a ValueTuple reference in the library assembly and so fails SanityChecks.ValueTupleNotReferenced - it would add a facade dependency on the down-level targets. Replaced with a named private struct, which reads better at the use site anyway. That check is what the Ubuntu CI leg was failing on. It never showed locally because every run here had been filtered to the cluster and endpoint classes, so SanityChecks was never in scope - the same shape of mistake as the earlier Environment.TickCount64 break, which the filtered runs also hid. Merge from main was otherwise clean, including the new SER305-SER308 analyzers, which flag nothing here. Full suite on net10.0: 5981 passed, 0 failed. * Require provenance explicitly, and stop lookups from creating servers Provenance was defaulted in two places - and worse, defaulted *differently*: ServerEndPoint's constructor assumed Configured while GetServerEndPoint assumed ClusterTopology. No reader of either call site would guess that, and the factory's default was the dangerous direction, since a new call site that said nothing silently got a prunable server. It is now required at both, and sits second in the parameter list so it reads adjacent to the endpoint it describes. Only eleven call sites needed a value, and choosing each deliberately turned up four that should never have been able to create a server at all: reporting a connection id, inspecting a physical connection for feature flags, and updating node relations for endpoints created moments earlier. Those now resolve rather than create - a lookup able to create is exactly how the "?" phantom endpoint came about. The test-facing IInternalConnectionMultiplexer member keeps working unchanged, so no test churn beyond four sites that deliberately construct. Note what the required parameter does *not* do: it cannot mislabel a configured endpoint, because the constructor still checks both endpoint collections and upgrades to Configured regardless of what a caller passes. The parameter answers "what is this if it turns out to be new", which is why ClusterTopology is the honest value at the discovery sites even though some of the endpoints they see are configured. * Make the TLS harness capable of failing, and cover cluster name identity By default the in-process certificate covers the default endpoint and every registered alias. That is convenient, and it means a TLS identity test *cannot fail*: whatever the client dials or presents, the certificate matches. So the existing coverage proved nothing about the naming change in this branch, where a hostname-preferred cluster is now dialled by name rather than by address. InProcessTestServer.CertificateNames narrows the certificate deliberately, and three tests use it: - a hostname-preferred cluster with a hostname-only certificate connects and round-trips, which is the case this branch's dialling change exists for - reaching the same node by address against that certificate fails, and fails specifically with an authentication error rather than a timeout - without this half the positive test would still pass if the harness lost the ability to detect a name mismatch at all - SslHost still overrides the dialled form for validation: the address is dialled while the name is validated, which is how a single-certificate deployment works today and is precedence that must not change Prompted by thomhurst/Respire#296, which asks for transport address and certificate identity to be separable. This does not implement that - it makes it testable, which was the blocker: an inference about SNI cannot be verified in a harness where every name validates. * Stop the unroutable-redirect tests depending on a race These asserted that a command earns a "MOVED <slot> ?:<port>" redirect, and relied on no topology refresh landing between migrating the slot and issuing the command. That premise was even stated in the helper's comment, and it is not something the test controls. It fails because the target *was* addressable, just not nameable: hostnames are preferred and it announces none, so the endpoint field is "?" - but the complement rule then reports its address in the metadata, and a refresh discovers it there and routes straight to it. No redirect, no exception. Only ever seen on RESP2, which is consistent: that is the protocol with a separate subscription connection, whose completion can trigger the refresh. The target is now unaddressable by every form, not merely un-named, which removes the race by construction. Added a test that asserts the condition CI hit rather than avoiding it: force a refresh between the migrate and the command, and check the target still has not become an endpoint and the redirect still faults. Without that, a future change making such a node discoverable would silently turn every test in this class into a no-op. Reproduced neither in isolation (5 Release runs) nor in a full Release suite locally, so this is inference from the failure mode rather than a repro I watched; the new assertion is what makes it not matter.
M
Marc Gravell committed
efd950171a77f67d64f72b7f87800514d08e25d5
Parent: c59ce26
Committed by GitHub <noreply@github.com>
on 8/24/2026, 3:20:21 PM