Repository navigation
Conversation
Fixes the one failure the main merge exposed, and it was a window rather than a leak. **Diagnosed by instrumentation rather than guessed at**, after two wrong theories. Tracing both delivery entry points showed every delivery arriving on THIS core - no bridge deliveries at all, which killed the "both cores subscribed" idea. Tracing the out-of-band path by socket then showed subscription sockets at ALL THREE cluster nodes receiving. A non-sharded PUBLISH in a cluster is broadcast to every node, so each of those delivers while the publishing node counts only its own subscriber: twenty deliveries for ten publishes, with a reported count of one. **The cause is the in-flight window D2.5 created on purpose.** The endpoint is recorded only on CONFIRMATION, so a subscribe that never landed stays retryable - but `EnsureSubscribedToServer` guards with `if (IsConnectedAny()) return 0`, and in that window the answer is "not live". Every ensure arriving in it starts ANOTHER subscribe, and for an unrouted channel `SelectServer` deliberately spreads, so each lands on a different node. This core's establish-time re-subscribe then feeds the loop, which is how it reached three nodes rather than two. The merge did not cause this. #3254's role repair sets `IsReplica`, which on this branch publishes the role and asks for a reconfigure, so topology applies now cascade - and that opened the window often enough to see. It was latent before, and would surface on any cluster that reconfigures while a subscribe is in flight. The fix is one clause - `IsConnectedAny() || HasSendInFlight` - and it is the same conflation the ownership field had to split earlier, in the other direction: **"not live yet" and "nobody is doing anything about it" are different questions.** `IsHeldByNewCoreOn` already consulted the in-flight endpoint for the ping gate; this is the other reader that needed it. Both flags: back to the `ReconnectRetryPolicy` pair plus `ClusterTests.MovedProfiling`, 11,247 passing. Default 11,244 with the known timing rotator. Three consecutive clean runs of the cluster, pub/sub, keyspace and Issue1101 families. CI build clean.
First piece of the gate on removing the subscription bridge, and a direct port of the shipped core's answer to #3154: `PhysicalBridge.WriteMessageInsideLock` hands a subscriber-mode message to `ServerEndPoint.TryRerouteToSubscriptionBridge` when the connection it is about to go out on turned out to be RESP2. `RespEndpointExecutor.RerouteSubscription` is the same question in the same place. Why it has to be the write rather than the compose: whether a subscription shares the ordinary connection depends on the NEGOTIATED protocol, and a command can be composed while RESP3 is expected and written after a reconnect has settled on RESP2 - at which point writing it there puts the connection into subscriber mode and it refuses every ordinary command afterwards. Inert while the bridge still exists, which is how it was verified rather than assumed. **And removing the compose-time decline on top of it was tried, and does not work yet - for a reason that corrects my earlier framing.** With the decline gone, `Resp3DowngradeTests` fails on TIMING, not correctness: nothing is poisoned, but rerouting at the write has to DIAL the subscription socket first, and the in-process server's transcript shows `PUBLISH => :0` landing before the re-subscribe arrives. The shipped core does not pay that because it dials its subscription bridge the moment a downgrade is detected - `OnFullyEstablished`'s `else if (SupportsSubscriptions && Protocol > Resp2) Activate(Subscription)`. So the two mechanisms do DIFFERENT jobs and both are wanted: choosing the right socket when the protocol is already known is the fast path, and the reroute is the safety net for the window where compose-time knowledge was wrong. The decline was never a crutch for the reroute's absence, which is what I had written down before measuring it. Next piece, now specified: dial this core's subscription socket proactively when a connection establishes below the expected protocol, mirroring that `Activate`. Then the decline can go, and then the bridge. Both flags: the `ReconnectRetryPolicy` pair plus three rotators - `MovedProfiling` and two `Resp3HandshakeTests` cases that pass 192/192 in isolation - 11,245 passing. Default 11,244 with the known cache-sweep timing rotator. CI build clean.
… we hold one A connection that comes up BELOW the protocol it expected has subscriptions about to be re-placed onto a socket that does not exist yet, and a re-place that has to dial first loses the race against whatever the caller does next. The shipped core reaches the same conclusion in the same place, so dial it here on the establish. Guarded on actually holding a subscription for that endpoint, which is the correctness of it rather than a refinement: unconditionally, an endpoint whose subscriptions belong to the shipped bridge gets a second socket and the channel is subscribed twice. Also forget subscription records naming a replaced socket: a fresh socket carries nothing, so a record naming it reads as "already subscribed" and suppresses the re-ensure.
Removes the compose-time decline that kept RESP3-shared subscriptions on the shipped bridge - the gate on the subscription bridge, since while it stood the bridge had to exist to place them. Four pieces make it safe and each was measured by removing it again; the fifth had to be built, because the ordering the shipped core gets by accident (both bridges reconnect on one heartbeat) has to be arranged here, where the subscription socket is dialled lazily and the need for one is known only after the ordinary connection is warm. SubscriptionsSettling is that happens-before: an endpoint observed to downgrade holds an entry until its subscriptions are back on the wire, and publish waits on it, bounded. It has to be released by the re-place rather than by the dial - a socket already opened once returns from the connect long before the replacement runs its establish hook. Also retires the now-dead WouldSubscribeOnItsOwnSocket, narrows the downgrade test's mixed-traffic classifier to data commands (handshake probes on a dedicated subscription socket precede subscriber mode and are legitimate), and makes the RESP2 separate-pubsub assertion ask the server which socket is subscribed rather than assuming the bridge's.
A ServerEndPoint no longer builds a subscription PhysicalBridge at any protocol under the flag: the subscription leg is the other core's, which dials and owns its own socket, and with the share-the-connection decline retired every subscription goes there. All four construction sites now answer with the interactive bridge or decline. Stopping at "don't activate it" does not work and is worth the note in the design file: IsSelectable creates the bridge it asks about and then requires it to be connected, so an unactivated one makes every subscription command unselectable - no server is chosen, the send reads as "nothing to do yet", and the subscribe silently does nothing. 70 failures, nearly all a publish reporting no subscribers. Not needing it and not building it are one change. Five tests were counting this core's sockets or asking for two distinct connection ids; each was already written as "RESP3 shares one connection" and now says "or the engine flag". The sixth was a real gap: IsSubscriberConnected ignored a disabled SUBSCRIBE, which the shipped path got free because such a bridge never connects.
Under RESP2 the shipped core subscribes the configuration channel as the last step of the subscription bridge's handshake, so removing that bridge removed the client's only way of hearing that the topology moved. This core's subscription socket now does it in the same position - and, crucially, gets dialled at activation rather than lazily: the only subscriber is the library itself on that socket's own establish, so a client that only calls ReplicaOfAsync never touches this core and nothing would ever connect. Dialled on the same KnowOrAssumeResp3 condition the shipped core uses before activating its bridge, so a wrong guess costs a spare socket exactly as it did there. Also replaces the tuple state object in the dial, which referenced System.ValueTuple and failed SanityChecks.ValueTupleNotReferenced. The connect gate stays open for now: making IsSubscriberConnected consult this core's subscription leg was measured at 39 failures and a ten-minute run - the same connect-wait hang as before - so ConnectUsesSingleSocket waits briefly for the count instead, keeping the upper bound it exists to protect. Recorded in the design notes.
The share-the-connection decline was, in a cluster, also keeping key-routed channels on the shipped path - so retiring it exposed three things that had never had to work here. 1. Re-resolve the landed endpoint only for SSUBSCRIBE. A key-routed channel has a slot, so the slot's owner can always be computed, but a plain SUBSCRIBE is not routed by it and never answers -MOVED: the subscription really is on whichever node it was sent to. SubscribeToWrongServerAsync(sharded: false) subscribes via a deliberately wrong server and requires it to still be there. 2. Act on an UNSOLICITED unsubscribe push. When a slot migrates away the node drops its shard channels and says so; there is no command of ours to match that to, so matching alone left the client believing it was subscribed on a node that had stopped delivering. Read from a copy of the reader, since the matching layer parses the frame from the start. 3. Scope the publish barrier to the downgrade re-place. The configuration-channel dial runs at activation, once per node, so in a cluster every publish waited on six of them and any one slow to establish held the lot - a five-second publish against the shipped path's eight hundred milliseconds.
…re does Replaces the ad-hoc forget-and-re-ensure with the shipped routine itself. An unsolicited SUNSUBSCRIBE - the node dropping its shard channels as a slot leaves - now calls ResubscribeToServer via the OUTGOING node, which is the only one known to have the new route, and returns Handled rather than leaving it to matching. "Unsolicited" is told apart by there being no send in flight, which is what the shipped core asks of its own outstanding commands. ResubscribeToServer now sends via the owning core, which under the flag is not a refinement: the shipped send resolves to the interactive bridge once there is no subscription bridge, so under RESP2 it would put the connection carrying ordinary commands into subscriber mode. Two further gaps this exposed: - Rebind never passed forSubscriptionEndpoint, so TryFollowRedirect fell back to the ordinary executor and a redirected sharded subscribe put the target's ordinary connection into subscriber mode - the next publish there answered "only (P|S)SUBSCRIBE ... allowed in this context". The rule was written in TryFollowRedirect; it just had nothing to apply. - ForgetSubscriptionsOn now skips a subscription with a send in flight. Such a subscribe is very often why the socket is being established at all, and its record is about to be confirmed rather than stale; forgetting it let the re-ensure place a key-routed channel on the slot owner instead of the server the caller explicitly chose.
… weakly from pushes ActivateServer dials this core's subscription socket where it would have activated the shipped subscription bridge, on the same KnowOrAssumeResp3 condition, because the configuration channel is subscribed by connecting rather than by anyone asking: a client that only calls ReplicaOfAsync never touches this core, so nothing would ever connect and the broadcast reached nobody. The establish-hook dial keeps its TryResp3 condition - dropping it made a plain RESP2 client dial a second time, which RespSubscriptionConnectionTests counts. OnPush now holds the multiplexer weakly. A subscription connection holds a standing read, so the socket is rooted while open and anything strong reaching out of it keeps the multiplexer alive; a push arriving after the multiplexer is gone has nowhere to go anyway. This is right but not sufficient - the leak predates this change and is written up in design notes 9g with what to chase next, along with the full before/after suite numbers and the measurement showing that withdrawing the dial trades two failures for six worse ones.
A connection runs a read loop for as long as it is open, so its pending read keeps the connection reachable - and every strong reference out of a connection then keeps the whole deployment alive. A caller who abandons a multiplexer without disposing it never got it collected. The shipped core already says how this is meant to work: PhysicalConnection reaches its bridge through a WeakReference and calls the property BridgeCouldBeNull to say so. This core's connection needed the same on all three of its paths out - OnPush, Server, and the redirect router - and the router was the one that mattered, being a lambda over an instance method; the other two alone left the multiplexer rooted. Each can go null while the connection is open, which every caller already handled: none of them is known until the endpoint is resolved, so null was always possible, and "nobody is modelling this any more" gets the same treatment as "not yet known". GarbageCollectionTests.MuxerIsCollected now passes under both flags.
…annel "Nothing has been sent, so nothing has dialled" stopped being true once this core dials at activation for the configuration channel, so the test asserted its own precondition rather than the mapping it exists to measure. Run with that channel off instead, which restores the precondition and leaves the property alone. Also clears SlotMapIsDrivenByTheSlotsView in the same batch - that one passes alone and its in-batch failure reports a MOVED from a real server, so it was the in-process server sharing port 6379 with the docker primary, not a routing fault. Flagged full-suite failures are back to the baseline count of 5, with the subscription bridge gone, the configuration channel carried, the multiplexer no longer rooted, and two of the baseline five fixed. Numbers and membership in design notes 9g.
Adds Subscription.OnSubscribedViaNewCore, the missing half of OnSubscribedViaBridge: ownership was otherwise only ever set by TrySendViaNewCore, so a caller that places a subscription on this core's connection itself had no way to say so, and the registry asked the wrong core whether it was live. The heartbeat reads that as a subscription nobody is connected for and subscribes it again, which the server reports as two subscribers to one channel. ASubscriptionOnTheNewCoreReceivesDeliveries also has to use the multiplexer's own core rather than the test fixture's: the registry asks server.Multiplexer.NewCore, so two instances can never agree about ownership. The fixture's core is for asserting about commands, where there is no shared registry to disagree with. With the flag off the shipped bridge owns subscriptions and will always contest a hand-placed one, so the test now skips there, stating why. It had been failing on the shipped run for that reason; the shipped suite is now fully green.
A send to a node that has stopped accepting writes never completes, so the endpoint it recorded was never cleared and HasSendInFlight answered yes for ever - after which every EnsureSubscribedToServer returned "nothing to do" and the subscription could not be re-placed at all. Bounded by the configured timeout. Bounded by TIME rather than by asking whether the connection is live, which was tried and is wrong: a subscription socket that is still dialling is not live either, so liveness cannot tell "not up yet" from "never coming", and treating the first as abandoned reopens the duplication window the guard exists to close. Found while diagnosing RetirementUnderMaintenanceTests, which it does NOT fix: that one is EndpointForSlot having no reachability filter where the shipped selector passes allowDisconnected: false, so this core keeps aiming a slot at an owner it cannot reach. Written up in design notes 9h as the next step, along with the causes ruled out.
EndpointForSlot asked the topology who owns a slot and picked by role, with nothing asking whether the chosen node could be reached - where the shipped selector asks it in its signature, as allowDisconnected: false. So a slot whose owner had gone away kept being chosen. "Known down", not "not connected": this core dials lazily, so an endpoint nobody has needed yet is unconnected and perfectly usable, and barring it would route every first command away from the node that owns the slot. An endpoint counts as down only once it has been dialled AND has a connect fault to show for it - which is the distinction ServerEndPoint.PublishSelectable already relies on from the other side when it excludes DidNotRespond. Either socket counts. Asking only about the ordinary executor missed the case that matters: a node this core has only ever subscribed on has no ordinary executor, which is exactly the shape of a sharded channel's node going away. And Settle now records NOTHING when a redirect points at a node known to be down. The re-resolution reads the slot map, and a -MOVED updates that map whether or not the resend it caused ever landed, so a redirect to a departed node was recorded as the subscription's home - the one answer that is certainly wrong, since it is neither where we aimed nor anywhere reachable. Unplaced is the truth and is what lets the next ensure retry.
RetirementUnderMaintenanceTests guards against vacuity by requiring the refusing node to be accumulating our own probe traffic. Under the engine flag that state does not arise: this core will not aim anything at a node it has dialled and found unreachable, so the node is simply idle rather than busy-with-probes-but-idle-of-caller-work. Verified before changing it by relaxing only that precondition - the whole rest of the test passes, so retirement itself was never in question. The flagged path therefore asserts that nothing is queued there at all, which is a property worth pinning, rather than skipping; everything after that point still has to hold either way. Design notes 9i records the three parts of the routing fix and why the first two were each insufficient alone.
…oken Three consecutive full runs of the same build: shipped fully green, flagged five to seven failures with only ReconnectRetryPolicyUnitTests in common. The rest rotates almost entirely, and most of it is timing - intervals, timeouts, connect failures, a profiled redirect. Naming it as its own problem because it is one: the stable count is two and the observed count is five to seven, so quoting either alone would misrepresent it. The likely driver is three sockets per node under RESP2 while both cores exist, which makes it the interactive bridge's removal to fix rather than any individual test.
The wedge in 9b-xiii was seen a third time, and this time it produced no failure list at all: a full flagged run exceeded its thirty-minute timeout and was killed, where runs of the same build either side finished in about three minutes. Worth recording because CI would report that as a timeout rather than as this. Also rules out the obvious suspicion about 9j's rotation: these runs follow hours of cluster tests that migrate slots without migrating them back, so the shared docker cluster was checked directly - every node reports cluster_state:ok with all 16384 slots assigned and ok. The topology is intact, so the rotation belongs to the client.
…what is missing An earlier note recorded the gate as measured closed. Re-measuring - gating the interactive handshake's AutoConfigureAsync on !NewCoreEngine and running the full flagged suite - gives 26 failures, not baseline. The failures cluster by belief, which is the useful part: the new core's handshake does not publish the product variant and version, the tie-breaker, CLUSTER NODES as distinct from CLUSTER SLOTS, or the SET replica probe used when HELLO cannot report the role. Each is named in design notes 9k with the tests that catch it, and with the order to close them in. The handshake already asks INFO server; it just discards everything except redis_version, so the product variant is the cheapest of the four and the largest cluster of failures.
DiscoverProductAsync reads INFO server and derives the product and its version, reusing AutoConfigureInfoField and its generated TryParse so the field names keep one spelling. It is not a cosmetic label: ServerEndPoint decides whether a cluster supports multiple databases from it, so a client that never learns the product applies the wrong rule. From INFO rather than HELLO, matching the shipped core: HELLO's server field would be free but names the product without its product version, and the version is half the belief. The variant is decided across the whole reply, because redis_version and valkey_version can both appear in either order. Placed in DiscoverServerConfigAsync, already the interactive-only home for beliefs nothing else has described, so the handshake transcript is untouched - several tests assert it exactly. Gated on an empty recorded product version, the same "has anyone said?" test the database count uses. With the shipped auto-configure gated off this closes ProductVariantUnitTests (7 of 26). Design notes 9l records the rest, including why closing the other three gaps individually will not close the gate: the next failure is not a missing belief but a missing connection.
…erdict Wired the whole coupled step as an experiment - shipped core stops dialling, this core dials at connect via ConnectMode.Discover, IsConnected and IsSelectable answer from this core, shipped auto-configure gated off - and measured it: a 311-test family went from ~20s to 6m30s with 36 failures, spaced ~15s apart. The 15s is ConnectImplAsync deciding the multiplexer never connected: ReconfigureAsync(first: true) returns false and connect throws UnableToConnect after its retries. Every failure in the list is that one exception, so nothing else about the coupled move is measurable yet. One hypothesis eliminated and deliberately not landed: completing OnConnectedAsync's pending connection monitors from this core's establish, which is the obvious candidate and changed the measurement not at all - same failures, same duration, same spacing. Dropped rather than committed, because landing a plausible-but-unverified change is exactly how the "gate measured closed" error in 9k happened. Experiment patch kept in the scratchpad; design notes 9m records it.
ReconfigureAsync proves an endpoint is reachable by sending a tracer down the "already connected, show me" path of OnConnectedAsync, and WriteDirectAsync puts that on the shipped bridge - so under the engine flag it proves the wrong connection, and against the step that stops that bridge dialling it is written to a connection that will never carry it and never completes. ConnectFailTimeoutTests.NoticesConnectFail is exactly the test that catches it. Declines unless the other core actually has the endpoint connected, so it cannot report "unreachable" about a server the bridge is talking to perfectly well. Measured clean in both default configurations: shipped fully green, flagged within its usual rotation band. Design notes 9n records this alongside the two hypotheses that looked just as good and fixed nothing - completing the pending connection monitors from this core, and dialling this core before the connect verdict - both dropped rather than committed. The ~15s per connect that the coupled move still costs remains unexplained, and the next step is to read the connect log rather than reason about it.
…ath is 82% of the rest "How much still travels as a Message" had been answered by counting source sites, which flatters the position badly - RedisDatabase.cs holds 395 of them and under both flags not one runs. Measured instead, with a counter on the Message constructor over the same 2,282 tests: 79,114 messages shipped against 9,772 under both flags, and zero data commands in the flagged run. No SET, HSET, LPUSH or ZADD at all. Of the 9,772: the connect path is 8,050 (82%), the tie-breaker GET 862, SENTINEL 432, SUBSCRIBE/PSUBSCRIBE 403 (built to route, never sent), SCAN 25. So there is one structural problem left between here and a single core - the connect path, which is where 9m/9n is stuck - rather than a long tail. Counter reverted rather than kept; design notes 9o says how to re-run it.
Reading the connect log, as 9n said to, named it immediately: the pending connection monitor is handed out by OnConnectedAsync and never completed, so every endpoint runs out three ConnectTimeouts. 9n's first hypothesis had the mechanism right. It failed because it and the second hypothesis were two halves of one change and I tested them separately - the hook had no connection to fire from, the ordering had nobody to complete the waiter. Together they still failed, and that exposed the real finding: muxerEndpoints=0. The multiplexer has no endpoints until ReconfigureAsync resolves them, so any dial before it iterates an empty list and does nothing. The dial belongs inside that method's per-endpoint loop, beside ActivateServer. With that, connect goes from 15,081ms to 72ms, the log shows the monitor completed by this core, and the shipped interactive bridge never connects at all. The 311-test family goes from 36 failures in 6m30s to 24 in 50s, and not one remaining failure is a connect failure - they are the belief gaps already itemised in 9k, with the tie-breaker the largest. Not landed: the enabling dial is not inert by default - it would add a socket per endpoint while the shipped bridges still dial - so it lands with the switch that stops them. Patch kept in the scratchpad; design notes 9p.
The handshake took the server type from HELLO's mode field and from CLUSTER INFO, which leaves two cases unanswered: a server with no HELLO - exactly the case that needs another source - and a product that spells the field server_mode rather than redis_mode. ValkeyUnitTests is built on both at once and was read as Standalone. Read in the same INFO server pass that already derives the product, using ServerTypeMetadata.TryParse and accepting both spellings, which is the shipped processor's rule rather than a guess. Guarded the way Publish guards it: a sentinel or a proxy is not something an INFO section gets to overrule. Against the coupled move this takes the 311-test family from 24 failures to 18, clearing all four ClusterTopologyUnitTests and both ValkeyUnitTests - so those were never the CLUSTER NODES gap they had been attributed to in 9k/9p, they were this. Measured clean in both default configurations.
…shake AUTH was awaited with no catch, so a server that refused it failed the whole handshake - where the shipped handshake writes AUTH fire-and-forget, cannot read the reply at all, and therefore continues while recording the suspicion via SetAuthSuspect. Commands then fail individually with the server's own words, which is a far better diagnostic than a connection that never exists. The case that made it matter is not a wrong password but ONE configuration spanning servers with different requirements: a config carrying the secure server's password also reaches the plain one, which answers "Client sent AUTH, but no password is set". Awaiting that reply took a perfectly good endpoint out of the deployment. Also reads the tie-breaker, which is the deciding vote in the primary election - without it every endpoint logs "had no tiebreaker set" and then "No primaries detected". Re-read on every handshake rather than cached, because a tie-breaker is mutable state and changing it is how an operator moves the election. Database 0 named explicitly: a tie-breaker is a key, the asking context may name no database, and GET is refused before it reaches a socket - which is what RespNewCoreDiscoveryTests caught, in both configurations. Against the coupled move these take the 311-test family from 18 failures in 50s to 6 in 11s, clearing all ten MultiPrimaryTests.TestMultiWithTiebreak and both ConfigTests.MutableOptions - so MutableOptions' "could not connect" was this, not a separate structural problem.
A role is not a preference, it is a routing fact - the client refuses writes to a replica on the strength of it - and unlearned, every server looks like a primary. MultiPrimaryTests.CannotFlushReplica connects to the replica, looks for the server where IsReplica is true, and finds none. From INFO replication, which is the only source always available. ROLE is better where it applies, since a primary lists its replicas and one reply describes both sides, but it is asked only when the topology says roles could change a decision - which with a single configured endpoint they cannot, and that is exactly this case. HELLO carries a role only under RESP3. master_host and master_port come from the same section, so PrimaryEndPoint is read with it. Re-read per handshake like the tie-breaker rather than cached like the product: a failover changes this. Takes the coupled family from 6 failures to 5; design notes 9q records the whole 24 → 5 run and corrects two wrong attributions - CLUSTER NODES turns out not to be needed by anything measured, so the 9k itemisation over-counted by a row.
Four pieces, each needed by the coupled move and each correct on its own terms. KEEP-ALIVE. The heartbeat reached this core's executors but only ever expired backlog and pending - it sent nothing. An idle connection is a connection being timed out: servers close one after their own 'timeout' (the very setting this core reads during discovery) and the network between does it sooner. Two schedules, as the shipped bridge has: every heartbeat when consistency checks are on, since noticing a dropped stream promptly is their whole purpose, and otherwise once the connection has been quiet for WriteEverySeconds. Fire-and-forget, because a heartbeat that waits for a server that has stopped answering is a heartbeat that has stopped beating for everything else. OPERATION COUNT. ServerEndPoint.OperationCount summed bridges, so it read zero for a server this core had been talking to all along - which is also how a test asks whether the heartbeat is doing anything at all. CONNECTION ID. The handshake now asks CLIENT ID, and GetConnectionId answers from whichever core holds the connection. It is the only handle on a connection from outside - CLIENT LIST names it, CLIENT KILL takes it - and it was null for the connection actually carrying the commands. The subscription leg falls back to the ordinary connection when it IS the ordinary connection, which under RESP3 it is; answering null there would describe a connection that does not exist rather than the one carrying the deliveries. Together with the earlier handshake work this takes the coupled 311-test family from 36 failures in 6m30s to 0 in 4s. Measured clean in both default configurations.
The 311-test family is fully green under the coupled move - 36 failures in 6m30s to 0 in 4s - but it was not representative, and the full suite says 126. Recording that plainly, because optimising against a slice made the position look much better than it is. Grouped, it is one piece rather than a scatter: maintenance notifications are 70 of the 126 across six test classes, and they are a coherent feature - the opt-in is sent only by the shipped handshake, and the pushes, relaxation windows, retention and replay all hang off a connection that has opted in. This core neither opts in nor would recognise the pushes. That reframes the census in 9o: the connect path was 82% of remaining Message traffic by volume, but by test weight maintenance is the largest thing still on the shipped bridge. One run of this build was killed at the 25-minute timeout where neighbours took 3.5 minutes, so 126 is from a run that completed rather than a stable average.
The opt-in is the feature's gate: the windows, retention, replay and migration handling all hang off a connection that has asked, and one that has not asked is never told. With this core carrying the commands and only the shipped bridge opting in, the client was being warned on a connection it no longer used. Sends CLIENT MAINT_NOTIFICATIONS ON [moving-endpoint-type <t>] and reads the reply, which is why it is asked from here rather than written blind - "agreed" and "declined, because X" are different facts. Both decisions are borrowed rather than restated: the shipped ShouldRequestMaintenanceNotifications and MaintenanceMovingEndpointTypeLiteral gained overloads this core can call, and the shipped callers delegate to them. moving-endpoint-type needed real plumbing rather than an approximation. For Auto the type is derived from the address actually reached and whether the connection is encrypted - not the endpoint dialled, which is usually a name - and only whoever built the transport knows either, with encryption sometimes the tunnel's decision. RespTransportFactory.ConnectAsync now reports both. Guessing internal where it is external sends a client to an address it cannot reach when a shard moves. Fixes ~11 maintenance failures; 58 remain and all need the push path. Design notes 9s records its shape: lift PhysicalConnection.OnMaintenanceNotification's ~270 lines of deliberately tolerant parsing out so both dispatchers call it, rather than writing a second copy of rules that exist for go-redis compatibility.
9s said the 58 remaining failures wanted the push path and that the move was to lift the parse rather than copy it. It was a smaller refactor than it looked: eight of the eleven members were already static, and the three that touched 'this' wanted exactly four facts - the server, whether the connection is established, the address it reached, and a trace sink. So OnMaintenanceNotification becomes ReadMaintenanceNotification taking those, the instance method is a four-line wrapper, and RespPushDispatch calls the same routine. The ~270 lines of deliberately tolerant parsing are untouched: no second spelling of the optional sequence id, the movable time, SMIGRATED's nested triplets or the explicit nulls. It returns void rather than OutOfBandResult, which is private to PhysicalConnection and a different enum from the new core's: a maintenance notification is never a reply, so the frame is consumed either way and there is nothing to signal. OnMovingAnnounced took a PhysicalConnection and used it for one thing - the address the announcing connection reached - so it now takes the address. That is the third time this session that 'the address we actually reached' turned out to be the real parameter hiding behind a connection. Maintenance failures 58 to 17. Shipped suite fully green.
The miss harness's keys and cache, but each read picks its key from a Zipf distribution (s = 0.99, a seeded table of 4M draws, so every arm sees the same sequence), and every mode now reports the hit rate over the measured window. Where admission's two effects meet: one extra miss before a hot key is cached, against the long tail no longer evicting it. 64 async callers: OnFirstMiss 1.05-1.07M ops/s 9.8us CPU/op 72.4% hits OnRepeatedMiss 1.58-1.61M 7.1-7.2us 76.6% hits no cache 553k 19.6us
…call; two follow-ups
…orrected It can write - it caches the computed cardinality back into the HyperLogLog's header - but that is the data structure's bookkeeping, not the command's meaning: re-running it gives the same answer, and a cache hit that skips it skips nothing observable. Refreshing the header signals the key modified, so the PFCOUNT that refreshes it invalidates its own cached reply and the next one is cached: one extra miss, never a wrong answer. The code already classified it this way; the design notes (6.9) said caching it was wrong. CommandCategoryTests.PfCountIsARead pins the decision.
Measured on a skewed workload (Zipf, s = 0.99) it raised the hit rate as well as throughput (72.4% -> 76.6%, 1.06M -> 1.6M ops/s), and on an all-miss workload it removed the cache's overhead entirely; it loses only where every request is read exactly twice. So it becomes the default - as CacheAdmission.Default, resolving to OnRepeatedMiss, on the CacheTrackingMode pattern: zero means "the library's choice", so that choice can move without overriding anyone who named a rule. OnFirstMiss and OnRepeatedMiss renumber to 1 and 2 (unshipped). The doorkeeper is now sized from the most entries the budget could hold (MaxBytes / EntryOverheadBytes) rather than assuming ~1 KiB per entry, which gave small replies a window several times too short. The cache-mechanics tests (invalidation, eviction, budgets, sweeps - not admission) now name OnFirstMiss explicitly, since they fill and then expect a hit. Docs: the quick start shows a reply cached on its second read; the Admission section explains the rule, the measurement, the cost and how to opt out; "Is it working?" warns that a check reading twice is testing OnFirstMiss.
…stops routing Every enumerator - SSCAN/HSCAN/ZSCAN, the emulated key SCAN, vector-set range paging - checked the WithCancellation token between pages but sent each page with no token, from when every executor refused a cancellable one. So cancelling took effect only at the next page boundary: a slow page ran to completion after the caller had given up, and against a server that never answered, the enumeration never ended. RespScan.ForSend now hands the token to the send wherever the executor can act on it; elsewhere the between-pages check still stops it. CancellingMidPageAbandonsThePageInFlight pins it (and fails, timing out, without the change). RespMultiplexerExecutor.CanCancel answered by resolving a route - server selection and the endpoint's lock - on every call made with a cancellable token, the same shape as the Transactional regression fixed in c537026; its routes all reach executors that can cancel, so it is now constant. That also stops a cancellable call to a multiplexer with no reachable endpoint being refused as "not supported", rather than failing for the reason it would.
…dependent, so checked before changing
… as a repeat
RespFest's skewed cells showed the default admission behaving exactly as
OnFirstMiss (Zipf 72.1% vs 72.5%; all-miss rows identical). The cause was the
byte hash behind RespRequest, RedisKey, RedisChannel and RespKeyTable: a 32-bit
multiply-by-33 over words folded in half, which gave 200,000 framed requests for
unpadded keys (GET rf:cbig:{i}) ~42,000 hashes between them. The doorkeeper then
took its probe from the low bits of a bare multiply, so those landed on ~4,500
bits of a million: the filter was full from the start and admitted every miss.
The local harness used zero-padded keys, which happened to hash well.
Now a word at a time with one 64-bit multiply each, then MurmurHash3's
finalizer: ideal distribution over a dozen key shapes, as keys and as requests,
at the same cost on a short request. The doorkeeper also runs its hash through a
full avalanche before probing, so a weak hash cannot collapse it again.
On RespFest's key names, OnRepeatedMiss: Zipf 72.4% -> 75.1% hits; all-miss
324k -> 610k ops/s, GC pause 22% -> 2%. Hit path: the stub single-key loop
measures 70-97ns across builds of any change to this function (one returning
the old value measured the same), so layout; 100k keys +4-7ns.
The hit-scaling harness names OnFirstMiss (it measures hits, not admission) and
uses unpadded keys, as RespFest does.
Counting only what it recorded, a keyspace smaller than the window (131,072 at 32 MiB) was never forgotten: once each key had missed once, nothing new arrived to trigger a clear, every later miss read as a repeat, and the default admission became OnFirstMiss for good. RespFest's Zipf cells replay a 1M-draw table of ~120,000 distinct keys, so showed exactly that: 72% either way at 64 callers. Modelled (design/admitsim.py): that stream 72.5% -> 76.1%, independent draws 75.3% -> 76.1%. Measured with the harness, which now draws as it goes rather than replaying its own 4M table: default 76.4%/76.0% at 1/64 callers vs 72.4%/ 72.5% OnFirstMiss; 1.45M vs 1.07M ops/s at 64. All-miss unchanged (596k ops/s).
…backlog count again
…rejected; three stale descriptions corrected
…or, dedicated pub/sub under RESP3, connection-attempt events
…ct for dedicated); the cache is opt-in, not on by default
… cannot serve a replaced value A connection group resolves the active member's cache per command, so a write made while B was active stamped only B's cache. A's server hears of it only by replication, and A's tracking connection is told only then - so failing back could answer from A's entry with the value the caller had just replaced, for up to the replication lag: read-your-own-writes broken by a switch. Each member's cache now holds its peers (RespClientCache.SetPeers, relinked by the group on construction, add and remove), and a local write stamps them all. A then misses and asks its server, which may still be behind - that is replication, and an uncached read sees the same; the cache is never staler than the member it answers for. Server invalidations and flushes stay per member. Flushing on every switch was rejected: with equal weights a group selects by latency, so the active member can change on any health pass. The ledger's second worry - a silent inactive member - needs no code: group members force HeartbeatConsistencyChecks, so every endpoint is pinged each heartbeat, and a half-open tracking connection trips the dead-socket rule and flushes that member's cache, exactly as for an active one. Tests: GroupCacheTests (in-process servers that never replicate; both write paths, the IDatabase forwarding and the context; both fail without the fix) and the peer relinking on Remove. ActiveActiveCacheScenarioTests in the fault-injector tier, against real replication - compiled, not yet run.
…ts async twin does Found porting NRedisStack, whose command factories hand back frames: SendAsync<T>(ref frame) took an optional handler (the inbuilt one for T when omitted), while Send<T>(ref frame) demanded a non-null handler and a token. The synchronous overload now takes both as optional, matching the interpolated Send<T> - flags stay required, as they are there. Unshipped API, so no compatibility cost.
…not just timeouts
…gRPC's call token - in Basics.md and AsyncTimeouts.md
11 tasks
…ce 2026-10-09); the PR-body-only backlog items (cache metrics, AppendFormatted, .NET 11) recorded here
…d drop the Logging.Abstractions reference where it is in the shared framework (NU1510) (#3276)
…r hops - DedicatedThreads recovers it
…lt now resolves to it (Broadcast when Prefixes are given, since PREFIX needs BCAST)
…y flag, the entry queue spares flagged entries once, and flagged entries are copied forward out of an evicted slab Measured in-test against exact LRU and FIFO at the same capacity (Zipf s=0.99: LRU 66%, FIFO 62%, now 67%; hot set + scan: LRU 74.5%, FIFO 56%, now 74.5%), and in RespFest's cache league before/after (zipf hit rate 76% -> 79%; hit, churn and sad-path cells within spread). The hit path gains one conditional field write per entry per eviction cycle.
…te(RedisKey|span) and InvalidateAll(), acting as local writes (peers stamped, no grace period), plus the cache's counters
…OptOut, TrackingOptIn, TrackingByPrefix) (#3297) * client cache: opt-in/opt-out by mode and by flag - CommandFlags.CacheOptIn/CacheOptOut (two bits, both-set reserved) replace NoClientCache; Tracking negotiates OPTOUT, new TrackingOptIn negotiates OPTIN, new TrackingByPrefix (OPTIN, Prefixes applied client-side, flags win) is what Prefixes now select by default; CLIENT CACHING YES|NO travels as a per-request prefix frame written as one unit with the request (RESPite IRespMessage.Prefix + discard sink); a transaction that reads is sent behind CACHING NO under OPTOUT * tests: netfx expectations for the CacheOptIn/CacheOptOut formatting cases (the second zero-valued name, PreferMaster, is appended to every non-zero value on .NET Framework)
This branch was successfully deployed
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
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.
What 4.0 is
A new core, with the old one deleted.
Message,ResultProcessor,PhysicalBridge,PhysicalConnectionandRedisDatabaseare gone. Commands are rendered by an interpolated-string RESPwriter and sent through executors over the RESPite transport. Reconnect, backlog, resubscribe, redirects,
maintenance events, sentinel and logging (shipped event ids preserved) were ported and checked against
the old core before it was deleted.
IDatabasestays, served by the new core. Existing code keeps compiling and working; seedocs/LegacyApi.md. The old API is not deprecated.A grouped, async-first command surface alongside it:
Commands are extension members on group types (
Strings,Hashes,Keys, ...), so adding one - ours or athird party's - is never a binary break to an interface. Replies can be leased rather than allocated, and
every call takes a
CancellationToken.Highlights
buffers, and replies are parsed on the caller's continuation rather than by the reader. The core's IO
loop now only frames bytes in and out, so one slow or large reply no longer holds up everyone else's.
Instead of building
object[]argument arrays forExecute/ExecuteAsyncand picking through aRedisResult, a library writesctx.SendAsync<T>($"FT.SEARCH {index} {query}")with a typed replyhandler, getting the same zero-copy formatting, cluster slot routing, key prefixing and caching as the
built-in commands, and can hang its own command groups off the same targets as extension members.
ConfigurationOptions.ClientCache, RESP3, opt-in) driven by the library itself(
CLIENT TRACKING), keyed by the rendered frame, with invalidation that cannot race the send; in a connectiongroup, our own writes reach every member's cache. See
docs/ClientSideCaching.md.SER014):db.BeginBatch()/db.BeginTransaction()return structs that are ordinary keyspace targets, with explicit
ExecuteAsync, discard-on-dispose, andconditions via the existing
Conditiontype.Blocking()contexts send and wait on the calling thread, so a groupmethod returns an already-completed
ValueTask; the synchronousIDatabasemethods are built on it, andso can a library's.
DedicatedThreadsconnections connect and handshake synchronously, so on Linux theirsockets never need a pool thread either.
Blocking()for sync forms,IDatabaseAsync.AsTask(...)for
Taskforms (async state and observed faults), andcontext.Multiplexerfor connection-level settingssuch as
AddLibraryNameSuffix. Seedocs/Extending.md.CLUSTER SLOTS, batches split per slot,MOVED/ASKhandled in the executor.Compatibility
4.0.N-alpha.[Experimental]IDs (SER011,SER012,SER014; seedocs/exp/).IDatabasefamily are deliberate and expected in a major version; removals andsignature changes to shipped members are not.
Status
The full suite runs on Ubuntu (net10.0) and Windows (net10.0, net481); CI is green at the branch head (3cf425d).
Latest prerelease: 4.0.127-alpha (2026-10-12). Intermittent failures are listed under Watch, with detail in the ledger.
Pending
Mirrors
design/v4-ledger.md(last updated 2026-10-13), which has the detail; ticked here as items close.Decisions
SingleReadLoop/SEREDIS_SINGLEREADLOOPopts outDedicatedThreadsconnections connect and run TLS synchronously, so they never need a pool thread (dbb38f8)RespBatch/RespTransactionstay structs?SharedSubscriptionConnectionopts in)Work
ActiveActiveCacheScenarioTestsagainst a real Active-Active deployment - written and compiled, never executed (needs the fault-injector environment)Subscription._subscribed/IsSubscribed/AnySubscribed; ~30ConnectionsIfCreated?.null-tolerances; topology flowing both ways betweenRespTopologyandServerSelectionStrategyConnectAsyncis called without the token (default mode only)RetryDatabase.ExecuteAsyncandRespRetryExecutormainby merging (ongoing; the final landing must be a real merge)design/client-side-cache.md(378a103)PFCOUNTis cacheable and retryable - its write is internal bookkeeping (d59d4e2)Send<T>over a pre-built frame defaults its handler, as the async twin does (97ec53a; from the NRedisStack port)Watch (flakes, not yet reproducible)
GetServerByKeyMemoization(RESP3, Windows net481) - speculative fix d45eadf; not seen in the 60 CI runs since 2026-10-09RespCacheChurnTests- 2026-10-09, and again 2026-10-10 (Ubuntu) and 2026-10-12 (Windows net10.0); take a dump mid-stall next timeBacklogTests.TotalOutstandingIncludesBacklogQueueandSentinelTests.AbortOnConnectFailFalseRecoversOnceSentinelsAppear- intermittent, pre-existingQueuedResultTests.RetryTransactionFireAndForgetSharesOneCompletedTask(once)MaintenanceOptInServerTests.SequenceIdsAdvanceAndCanBeRepeatedfails when its class runs aloneCancellationTests.WithCancellation_CancelledToken_ThrowsOperationCanceledException- a race in the test (Task.WaitAsyncreturns an already-completed task as-is); should cancel through the API's own tokenTouchIdleTime,RespAggregateTiming,ADeferredWalkNeedsNoStorageAtAll, and seven single CI occurrences on 2026-10-10..12 (listed in the ledger)Backlog (after the alpha)
__keyspace@<db>__:*; needsnotify-keyspace-events, no RESP3 requirement) and explicit pub/sub (an application-defined channel the writer publishes to), for deployments without usableCLIENT TRACKING(proxies, older servers, some managed offerings)CanWritePreamble,RespKeyTablenode lifetime, the parked builder-inlining patchValueTasksurface (non-pooledasync ValueTaskonly; low priority)AppendFormatted(long)/(double)overloads (NRedisStack; needs an overload-resolution sweep)TlsBufferSession); run the suite and RespFest on 11 early