Repository navigation
Conversation
…vers (#3336) During a failover, OnFullyEstablished (per reconnecting connection) and ReconfigureAsync both restore subscriptions fire-and-forget; between sending (P)SUBSCRIBE and its acknowledgement a SingleNodeSubscription recorded no server, so a concurrent restore would subscribe again - often on a different node. The later ack then overwrote the recorded endpoint while the earlier registration stayed live on its server, delivering every message twice until that connection happened to drop. Two layers: - claim before send: a pending-claim token (CAS) makes competing restores back off while an attempt is in flight; the subscribe message is sent without fire-and-forget (callers that asked for F&F just don't await it) so every completion - ack, error reply, connection failure, timeout - observably releases the claim, keeping retry liveness. - reconcile at ack: TryAdoptEndpoint rejects an ack when the subscription is already confirmed on a different connected server, and the processor sends a fire-and-forget unsubscribe pinned to the stray node - mopping up anything the claim cannot prevent (e.g. a timed-out attempt whose ack arrives late). Regression tests run the reporter's overlap scenario (held first ack, second restore must not subscribe elsewhere) plus the stray-ack cleanup, per protocol, on the in-process test server.
This branch has not been 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.
Fixes #3336.
During a Sentinel failover,
ServerEndPoint.OnFullyEstablished(per reconnecting connection) andConnectionMultiplexer.ReconfigureAsyncboth restore subscriptions fire-and-forget. ASingleNodeSubscriptiononly records its server when the (P)SUBSCRIBE acknowledgement is processed, so between send and ack a concurrent restore saw the subscription as disconnected and subscribed again — often on a different node (sometimes a replica, depending on selectability mid-failover). The later ack overwrote the recorded endpoint while the earlier registration stayed live on its server, and since message dispatch is by channel only, every publish was delivered twice until that connection happened to drop. (Nice diagnosis by the reporter — confirmed accurate end to end, including their in-process repro test, which failed 8/8 against main.)Two layers:
RemoveDisconnectedEndpoints.AddEndpointbecomesTryAdoptEndpoint, which rejects an ack when the subscription is already confirmed on a different connected server; the processor then sends a fire-and-forget unsubscribe pinned to the stray node. This mops up anything the claim cannot prevent (e.g. a timed-out attempt whose ack arrives late).Multi-node (keyspace) subscriptions always adopt — they want every eligible node — so their behavior is unchanged.
Tests: the reporter's overlap scenario (first ack held, second restore must not subscribe elsewhere) plus the stray-ack cleanup, literal/pattern × normal/F&F × RESP2/RESP3, on the in-process test server. Full suite green locally apart from the known
SortedSetScanLargeZSCAN environment failures (fixed on the 8.12 branch).The reporter also mentioned a separate reconfiguration-gate change for slow concurrent Sentinel discovery; that is intentionally not part of this PR.