Preparation work for smart-client-handoffs - #3174
Merged
Merged
Conversation
Test-only; prerequisite for the endpoint-identity work (#2826) and for reacting to endpoints we did not choose the form of. - node aliases, resolved through TryGetNode, so one node answers to several names; kept in their own map rather than as extra _nodes entries, since EndPointComparer cannot order mixed endpoint types and AddEmptyNode derives the next endpoint's form from an arbitrary key - CLUSTER NODES emits ip:port@cport[,hostname]; the cport was previously string-concatenated as "@1" + port, which is accidentally right for four-digit ports and wrong from 10000 up - per-node cluster-preferred-endpoint-type, with the complement metadata map in CLUSTER SLOTS and the prescribed placeholder endpoint values (null, "", ?); ? is derived from hostname-preferred-but-unannounced rather than being settable, so combinations the real server cannot produce cannot be expressed - gated on a 7.0 server version: below that the preference is inert, the hostname is not emitted, and the SLOTS node block is three elements - CONFIG GET reports the answering node's view of the announce settings, which costs CLUSTER CONFIG GET its LockFree status - AddEmptyNode(EndPoint) overload, since the existing one copies the form of an existing key Verified against a real 8.9.241 cluster at RESP2 and RESP3: the preference belongs to the *answering* node and governs every entry in its reply, while hostname availability is per described node - so asking a hostname-preferring node about a peer with no hostname yields ?. Also tightens the in-process TLS harness: the certificate is built on first use so it can cover aliases registered after construction, and validation no longer forgives a name mismatch on thumbprint alone.
CLUSTER NODES reports "ip:port@cport[,hostname[,aux-field=value]*]" since 4.0, and we discarded everything from the '@' onwards. The text was never lost (ClusterNode.Raw has it) but the parsed view did not offer it, and the hostname is a node's second identity - the thing endpoint reconciliation needs (#2826). Additive on ClusterNode: - ClusterBusPort (int?), null on pre-4.0 lines that report no cport - Hostname (string?), null when unannounced or the slot is empty - AuxFields, preserving unrecognized keys since the set is documented as extensible No named ShardId/HumanNodename properties: neither is observable on a real 8.9 server (cluster-announce-human-nodename does not appear in the reply at all), so naming them would mean shipping API for fields we cannot demonstrate. A named accessor stays additive if that changes. Parsing is lenient by design - an exception here does silent damage to topology, per the warning on the constructor - so an unparseable cport yields null and aux entries without '=' are skipped. Toy server gains SetAuxField, and CLUSTER NODES now handles the positional subtlety that the hostname slot can be empty while aux fields follow ("@17000,,shard-id=abc"). Tests: grammar cases against the parser directly, realistic shapes round tripped through the toy server, and end-to-end against a real cluster with version gates (4.0 for the bus port, 7.0 for hostnames) - including setting cluster-announce-hostname and restoring it. Those run per protocol, so they also cover CLUSTER NODES arriving as a RESP3 verbatim string, which had no end-to-end coverage before.
The second topology view, and a prerequisite for endpoint reconciliation (#2826): CLUSTER SLOTS is the only source that conveys which naming form a deployment prefers, and it carries the node-id, which is the one identity that does not depend on who is answering. Laid out per the HotKeys precedent rather than growing the existing files: ClusterSlots.cs (IServer partial + model), ClusterSlots.ResultProcessor.cs (the parse, on RespReader), ClusterSlots.Server.cs (RedisServer implementation). Model: ClusterSlotsResult.Assignments -> ClusterSlotAssignment{Slots, Primary, Replicas} -> ClusterSlotNode. Deliberately lossless: AnnouncedEndpoint keeps the literal the server sent, Metadata keeps unrecognized keys, and EndPoint is populated only when the reported value is usable as one. The three placeholder endpoint values are encoded rather than merely documented: null ("the server does not know this node's address"), "" ("the node does not know its own") and "?" ("an unknown node") all yield a null EndPoint, since substituting the connection's own address is a caller decision - and is outright wrong for "?". Metadata is walked pairwise rather than by declared length, so the RESP3 map and the RESP2 flat array need no special casing. ExportConfiguration now writes slots.txt alongside nodes.txt, one line per node per range so it greps like nodes.txt; ExportOptions.Cluster covers it (comment updated), and ExportOptions.All is -1 so it is included by default. Rendered from the model rather than dumped as RESP: users do not want to read RESP, CONFIG GET already sets the precedent for rendering a structured reply as flat text, and the rendering is faithful because the model is lossless. Tests: the naming matrix against the toy server (ip/hostname/unknown preference, unannounced hostnames, both address placeholders, pre-7.0 three-element node blocks, migrated slots, node-id stability across forms), plus real-cluster coverage at both protocols for keyspace coverage, replicas, the export, and SLOTS/NODES agreeing on primary node-ids - which is the premise id-keyed reconciliation rests on.
A hostname-preferring node redirecting to a peer that has announced no
hostname reports "MOVED <slot> ?:<port>", and "?" denotes an *unknown*
node - so unlike a missing or empty endpoint it cannot be resolved to the
node that answered. Reachable on any OSS cluster with
cluster-preferred-endpoint-type=hostname and a node whose
cluster-announce-hostname is unset; most plausibly one that had it set via
CONFIG SET without CONFIG REWRITE and was then restarted.
Demonstrated before fixing, and it was worse than a bad message:
Format.TryParseEndPoint accepts "?" as a hostname, so TryResend called
GetServerEndPoint(endpoint) - which defaults to activate:true - and thus
created and dialled a ServerEndPoint for a host literally named "?". The
phantom stayed in GetEndPoints() with a live connect loop, the command sat
in the backlog for the full async timeout, and the caller got a timeout
exception recommending they check connectTimeout, with a link to the
timeouts troubleshooting article.
Now: unusable targets are rejected before any endpoint is created ("?", an
empty host, or port 0 - the last covering the unknown-endpoint form, which
parsed to the nonsense DnsEndPoint(":<port>", 0)); a topology refresh is
still requested, which genuinely recovers, since CLUSTER NODES reports
addresses positionally whatever the preference; and the command faults
immediately with a message naming the cause and quoting what the server
actually sent.
New RedisErrorKind.UnknownRedirectTarget, appended so existing values keep
their numbers, and documented as a client-side classification rather than a
server error code. Included in FaultContext's known-not-applied set
alongside Moved/Ask: the redirect proves the command did not run, so a
retry is a first attempt rather than a repeat, which is what lets WithRetry
recover once the refresh has landed.
Builds an internal id-keyed view of the cluster from CLUSTER SLOTS and records it during autoconfigure, without routing on it. The point is to have an oracle: each subsequent step (slot map, then discovery, then pruning) can be taken with the two views still demonstrably agreeing. Keyed on node-id because it is the only identity in a reply that does not depend on which node answered - names vary with the answering node's preferred endpoint type, so keying on a name means one node can produce two entries. Keyed on the id they merge, and the names become attributes. Identity accumulation is the union of the primary endpoint field and the metadata complement, and the primary is classified by *content* rather than position: a first cut read only the metadata and consequently lost whichever form the answering node happened to prefer, which the tests caught. Tests assert the shadow view agrees with the routing view on primary node-ids and on slot ownership per node - as sorted slot sets, so differing range fragmentation between the two views is not mistaken for disagreement. Covered against the toy server across naming configurations, and against a live six-node cluster whose ownership is heavily fragmented, which is the stronger check on range boundaries.
The ClusterTopology model and SetClusterSlots stay - they are the id-keyed view the routing work needs - but the autoconfigure request is commented out with the reason in place. This PR is scoped to work that cannot destabilise a connection, and asking every server for an extra command on every autoconfigure is a new failure surface on the connect path (an unexpected error reply to an internal call, or a proxy that mangles the command) for no user-visible benefit until routing consumes it. It is enabled in the follow-up, where ordering also matters: the request must precede CLUSTER NODES so identities are known before NODES starts creating servers by address. The topology tests consequently source the reply explicitly via the public ClusterSlotsAsync rather than relying on autoconfigure, so they cover the model and the parser here; the wiring is covered where it is switched on.
PublicAPI.Unshipped.txt conflicted because both sides appended to a sorted file: main added the sentinel credential options and RedisFeatures.Hello, this branch the CLUSTER SLOTS surface, the CLUSTER NODES trailer members and RedisErrorKind.UnknownRedirectTarget. Resolved as the sorted union of both, and the BOM restored - an earlier rewrite of that file here had dropped it. Also, as drive-by cleanup: toys/AotRig did not build, on main as well as here - confirmed against origin/main untouched, so it is not a merge artefact. The rig uses the ordinary string APIs a caller would, and the implicit string-to-RedisValue conversion is [Experimental], so with warnings as errors the project could not compile. NoWarn for that one diagnostic, as RESPite.Benchmark and the toy server already do.
mgravell
marked this pull request as ready for review
August 14, 2026 13:16
The metadata keys come from a fixed vocabulary, so match them over the raw bytes via the usual [AsciiHash] enum rather than materializing a string per entry: ClusterSlotMetadataKey plus a case-insensitive generated TryParse, since the contract renders these keys as IP/Hostname in prose but lower-case in its examples and the casing cannot be relied on. Recognized keys are surfaced as ClusterSlotNode.Ip/Hostname and no longer appear in Metadata, which now holds only what we do not recognize. That is what makes the saving real - keeping them in a string-keyed list would allocate the key anyway - and it matches the ClusterNode.Hostname/AuxFields precedent, where the hostname likewise is not repeated in the collection. Worth more than it first appears: ranges repeat a node whenever its slot ownership is not contiguous, so this is one allocation avoided per key per *range*, not per node - on the live test cluster that is 43 ranges rather than 6 nodes. Toy server gains SetSlotsMetadata so the forward-compatibility path is actually exercised: an unrecognized key must be preserved as declared, and an upper-case known key must still be recognized rather than landing in Metadata.
This was referenced Aug 14, 2026
IList advertises Add/Remove/set that throw at runtime on the collections we actually hand back, which is a lie in the type system. The only argument for it was matching ClusterNode.Slots/Children, and those are shipped so they cannot move; every member changed here is unshipped, so consistency-with-a-wart buys nothing. Changed: ClusterSlotsResult.Assignments, ClusterSlotAssignment.Replicas, ClusterSlotNode.Metadata, ClusterNode.AuxFields. Also unpicks the `x?.AsReadOnly() ?? (IList<T>)Array.Empty<T>()` expressions, which were dense enough to misread, into a plain null check returning `[]`. No allocation difference: an empty collection expression targeting an interface compiles to Array.Empty<T>(), so only a populated list pays for the wrapper.
…leet shape Two unrelated failures, one real bug. Node.NewId created a Random *per call*, and .NET Framework seeds Random from Environment.TickCount - so two nodes constructed within the same tick received identical 40-character ids. Only reachable on the net481 leg, which is why Linux never showed it. The failure was maximally misleading: the shadow topology keys on node-id, so it correctly merged the two lying nodes into one, and the assertion that broke was the comparison against CLUSTER NODES - the *fake* was wrong and the client looked wrong. Now one shared Random on that path, plus an interlocked counter woven into the first eight characters so ids are unique by construction rather than by luck, with a regression test that creates 26 nodes back to back. ClusterSlotsTests.ReplicasAreReported asserted that some assignment reports replicas, which is a statement about the deployment rather than the parser: the Windows CI fleet is smaller than the local compose and has none. Skips in that case instead, so it still exercises the replica parsing wherever replicas exist.
Two CI changes, both for the same reason: the Windows leg failed and the detail was unreachable. dorny/test-reporter defaults to listing every suite and every test, which rendered ~3MB into $GITHUB_STEP_SUMMARY against a 1MB limit - so the summary was dropped wholesale and the actual failures had to be read out of the check-run annotations API. list-suites/list-tests: failed keeps full detail for failures and collapses the passing thousands to counts, which also means a failure is no longer buried in a wall of green. Note this was never the xunit logger's doing: its summary is a fixed counts table, so suppressing passed/skipped there changes nothing. Also uploads the trx (14-day retention). The run had no artifacts at all, and the failure-only report deliberately omits per-test output - which is precisely what a CI-only failure needs. PublicAPI.Unshipped.txt conflicted again. Resolved by taking main's content verbatim and appending ours, rather than merging into sorted order as before: main appends to the end of its own list, so a sorted file interleaves the two and conflicts on every merge. Dropped our now-stale Tunnel.ConnectTransportAsync entry, which #3173 replaced with the TlsOptions overload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Smart -client-handoffs needs prep work:
-MOVEDwith unknown routing: works (as in: fails correctly, and doesn't attempt to route to?) - newRedisErrorKind-MOVEDtargetsMoving to a follow-up more invasive PR:
ConfigurationOptions), which then migrate out of useChecklist