Fix duplicate Pub/Sub subscription restoration after Sentinel failover by vkyow · Pull Request #3337 · seredis/StackExchange.Redis

vkyow · 2026-11-06T04:31:03Z

During Sentinel failover, connection establishment and reconfiguration can both try to restore the same single-node subscription before either SUBSCRIBE/PSUBSCRIBE acknowledgement arrives. Both physical subscriptions can survive, even though the client records only one owner. A single publication can then reach the callback twice through different Redis nodes.

This change serializes restoration through the acknowledgement, including internal fire-and-forget requests. Replies are associated with their restoration attempt, late obsolete registrations are cleaned up, and unsubscribe is coordinated with pending restoration. Delivery is accepted only from the physical connection that owns the subscription. It preserves the existing server-selection policy and multi-node subscription behavior, with no public API or dependency changes.

A second commit moves the existing Sentinel reconfiguration gate ahead of primary discovery. This prevents reconnect-event storms from starting parallel blocking discovery queries before checking that another reconfiguration is already running.

Validation

Tested against upstream main at 9d17974fcade6c89b29b233030fe281e75f9ab1c:

The initial deterministic acknowledgement-barrier regression fails all 8 cases on unmodified source. The completed restoration and Sentinel coalescing suites pass 26/26 cases under RESP2 and RESP3.
Existing unit tests: 2,034 passed, no failures or skips. In-process Pub/Sub tests: 36 passed, 12 existing skips.
All five shipped target frameworks build with the upstream analyzers. Runtime HA tests used .NET 10.
Isolated Docker tests used Redis and Sentinel 8.0.3, with both three-pair and five-pair topologies (one primary, remaining Redis replicas, and one Sentinel per pair). Each topology included three fresh clusters with ten primary switches, plus additional restart, outage, authentication and reconnection scenarios.
Across 104 accepted recovery events, the slowest recovery was 16.78 seconds after an independently confirmed primary switch. Traffic continued for at least two minutes after each recovery. All 251,734 expected post-recovery-ready deliveries arrived once, with no missing ready deliveries or lingering duplicate business subscriptions.
Unmodified 3.4.31 controls reproduced duplicate inbound frames from different Redis nodes for publication IDs sent exactly once. The reproduction and supporting evidence are attached to Duplicate Pub/Sub messages after Redis Sentinel failover #3336.

Pub/Sub messages can still be lost during the failover transition; this does not add replay or an exactly-once guarantee. Already queued handlers may finish after unsubscribe. The Docker results are local validation, not a production-load benchmark.

To run the focused regressions:

dotnet test --project tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj -f net10.0 --filter-class StackExchange.Redis.Tests.SubscriptionRestorationTests
dotnet test --project tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj -f net10.0 --filter-class StackExchange.Redis.Tests.SentinelSwitchCoalescingTests

Checklist

I fully and freely contribute this code in accordance with the project license (and am legally able to do so)
I take responsibility for this contribution's quality and correctness, including any portions produced with AI assistance (see CONTRIBUTING.md).

mgravell · 2026-11-09T13:09:22Z

looking shortly

mgravell

Thanks for this — it's a substantial, well-aimed piece of work, and the core idea is right: having a subscription own a single connection and filtering deliveries against it genuinely kills the duplicate-delivery class, which is a real problem that has bitten people after sentinel failover. Several of the delicate parts hold up under scrutiny too: the finally in SwitchPrimary releases the reconfigure claim correctly, the cleanup-forwarding chain is acyclic, and subscribe acks are matched inline on the read thread, so the ordering assumptions work. The new test coverage is very welcome.

That said, this reworks some genuinely delicate machinery, and I have four correctness-level concerns plus some smaller ones — all inline below. The four that matter most:

A SwitchPrimary that loses the reconfigure-claim race no longer records sentinel's reported primary, which disables the "refused switch" recovery leg.
A sync Unsubscribe that times out on the operation lock leaks the server-side subscription permanently.
BeginRestore clears the current connection before the replacement SUBSCRIBE is acknowledged, turning the old duplicate-delivery window into a silent message-loss window.
The subscribe-ack SetResult is no longer unconditional, so a throw in the bookkeeping can leave the operation lock held.

None of these undermine the approach — they're all fixable within the design you've built. Happy to discuss any of them, and genuinely grateful for the effort that went into chasing this race down.

mgravell · 2026-11-11T09:31:44Z

+                                        ?? GetConfiguredPrimaryForService(serviceName)
+                                        ?? throw new RedisConnectionException(ConnectionFailureType.UnableToConnect, CommandFlags.None, $"Sentinel: Failed connecting to switch primary for service: {serviceName}");
+
+            connection.currentSentinelPrimaryEndPoint = newPrimaryEndPoint;


[correctness] Before this change, currentSentinelPrimaryEndPoint was recorded regardless of whether we went on to reconfigure; now a SwitchPrimary that loses the TryClaimReconfigure race returns early (line ~731) without recording what sentinel told us. That disables the "refused switch" recovery leg in IsSentinelPrimaryDemoted — and since the competing reconfigure that won the race may be the one whose reconnect timer has already been disposed, the worst case is a failover whose recovery stalls until something else prods the multiplexer.

Recording sentinel's answer is just bookkeeping, not reconfiguration, so it's safe outside the claim: when reportedPrimary is non-null, consider stamping connection.currentSentinelPrimaryEndPoint before (or instead of) the early return on a lost claim.

I've moved the reported-primary assignment ahead of the competing-switch check. The discovery result is also recorded before trying to claim reconfiguration, so a refused switch still leaves the demotion/recovery check with Sentinel's answer. Added BusyReconfigurationStillRecordsTheReportedPrimary to cover the lost-claim case; it passes.

mgravell · 2026-11-11T09:31:44Z

        try
        {
+            // Get new primary - try twice (unless the caller already holds sentinel's answer)
+            EndPoint newPrimaryEndPoint = reportedPrimary


Primary discovery (up to two sequential blocking sentinel queries) now runs while holding the reconfigure claim — previously it ran before the claim was taken. During exactly the scenario this PR targets (a failover storm with a slow or partitioned sentinel), that can hold the claim for up to 2× the blocking-query timeout, refusing every other reconfiguration attempt in the meantime.

The "claim first so a lost race touches nothing" comment above is about the endpoint rewrites, which these queries aren't — they touch nothing. Moving discovery back in front of TryClaimReconfigure keeps the claim window tight, and would also resolve the currentSentinelPrimaryEndPoint recording issue in the other comment.

Discovery now runs before taking the general reconfigure claim. I kept a separate per-connection switch flag to coalesce concurrent discovery attempts, so a slow Sentinel query doesn't hold up unrelated reconfiguration. DiscoveryDoesNotHoldTheReconfigurationClaim and SlowDiscoveryCoalescesSwitchesWithoutHoldingTheReconfigurationClaim both pass, including the blocked-query case.

mgravell · 2026-11-11T09:31:44Z

+                UnsubscribeCoreAsync(subscriber, channel, flags, null, internalCall).ObserveErrors();
+                return true;
+            }
+            Enter(subscriber, false);


[correctness] Enter(subscriber, false) throws on an _operationLock timeout (e.g. while a restore is in flight). By the time we get here the subscriber side has already removed the subscription and completed it, so the throw means: no UNSUBSCRIBE is ever sent and no CleanupRequest is queued — the server keeps publishing to a client that now filters every delivery, for the lifetime of the connection. It also makes UnsubscribeAll abort mid-loop, stranding the remaining channels.

Since _unsubscribed is already set at this point, the lock isn't protecting much here — on timeout, falling back to queueing a fire-and-forget unsubscribe/cleanup (rather than throwing) would keep the server-side state converging.

On a lock timeout, unsubscribe now queues cleanup for the current and pending routes and lets the unsubscribe loop continue. Delivery is already disabled for that owner, and the queued cleanup runs once the in-flight operation releases the lock. Added timeout and UnsubscribeAll regression tests for literal channels and patterns under RESP2/RESP3; they pass.

mgravell · 2026-11-11T09:31:44Z

+                RemoveIncorrectRouting(subscriber, channel, flags, internalCall);
+                if (IsConnectedAny()) return null;
+                _currentServer = null;
+                Volatile.Write(ref _currentConnection, null);


[behavior change worth making deliberate] Clearing _currentConnection here — before the replacement SUBSCRIBE has been acknowledged — means any messages still arriving from a live old connection are dropped by the Accepts filter for the whole restore window. Pre-PR those messages were delivered (possibly duplicated); this trades duplicates for silent loss, and the window is wider than the old duplicate window because it spans the full subscribe round-trip.

Pub/sub is best-effort, so the trade may well be acceptable — but it should be a documented decision rather than a side effect (the docs/PubSubOrder.md addition would be a natural home). Related: IsConnectedAny() (checked just above) and Accepts treat a null connection differently, so the two can disagree about "connected" mid-restore.

I've changed the handover to keep a live, acknowledged route accepting messages until the replacement SUBSCRIBE is acknowledged. The ACK switches the accepted physical connection, then queues cleanup of the previous endpoint. IsConnectedAny and Accepts now agree on the confirmed route. The delayed-ACK regression passes for RESP2/RESP3, and I updated PubSubOrder.md to describe the handover and its limits. Pub/Sub can still lose messages during failover.

mgravell · 2026-11-11T09:31:44Z

                                        break;
                                }
                            }
+                            SetResult(message, true);


[correctness] SetResult(message, true) used to run unconditionally once the count parsed; it now runs after the _onSubscribed/AddEndpoint bookkeeping. If anything in that block throws (_onSubscribed ends up in subscription state transitions, and failures there surface via OnInternalError, which invokes user handlers), the SUBSCRIBE message is never completed — and on the restore path that leaves _operationLock held until the timeout sweep rescues it.

Completing the message first, or wrapping the bookkeeping so SetResult always runs, keeps the ack path unconditional the way it was.

The bookkeeping is now wrapped so that SetResult(message, true) always runs in finally; bookkeeping failures still go through the internal-error path. I added SubscribeAcknowledgementCompletesWhenBookkeepingThrows to exercise this. The ACK completes and the test passes for both protocols.

mgravell · 2026-11-11T09:31:44Z

        {
-            var server = _currentServer;
-            if (server is not null)
+            bool fireAndForget = (flags & CommandFlags.FireAndForget) != 0;


fireAndForget is computed from the per-call flags only, where the old path also honored the flags the subscription was created with. A subscription created with CommandFlags.FireAndForget can now block for up to the sync timeout when a later Subscribe call on the same channel passes CommandFlags.None — a subtle behavioral regression for callers relying on F&F never blocking. OR-ing in the subscription's own Flags restores the old contract.

The fire-and-forget checks now use Flags | flags in the subscribe and unsubscribe paths. This preserves the subscription's original flags when a later call supplies CommandFlags.None. Added sync/async restoration coverage with an operation already in flight; the tests pass.

mgravell · 2026-11-11T09:31:44Z

+        {
+            Volatile.Write(ref _unsubscribed, true);
+            var task = UnsubscribeCoreAsync(subscriber, channel, flags, asyncState, internalCall);
+            if ((flags & CommandFlags.FireAndForget) == 0) return task;


The non-F&F branch returns the compiler-generated async task directly, so Task.AsyncState is null rather than the caller's asyncState — inconsistent with the F&F branch two lines down (CompletedTask<bool>.FromResult(true, asyncState)) and with the library's convention of flowing async-state onto returned tasks.

The returned unsubscribe task now comes from a completion source carrying the caller's asyncState. The wrapper also forwards the operation's result, failure or cancellation. UnsubscribeTaskPreservesTheCallersAsyncState passes for RESP2 and RESP3.

mgravell · 2026-11-11T09:31:44Z

-        /// Evaluates state and if we're not currently connected, clears the server reference.
-        /// </summary>
-        internal override void RemoveDisconnectedEndpoints()
+        private void DrainCleanup()


DrainCleanup runs inside _stateLock (via QueueCleanup) and from there issues network writes (ExecuteAsyncImpl) and takes other subscriptions' _stateLock (single.QueueCleanup(request) on the forwarding branch) — frequently on the read thread. I traced the forwarding and it's acyclic today (it only forwards to the currently-active subscription for the channel), so this isn't a live deadlock — but it's a lock-ordering hazard one future edit away from being one. Collecting the requests under the lock and executing them after release would make it structurally safe.

Cleanup requests are now collected under _stateLock, then dispatched or forwarded after releasing it. The operation lock still serializes cleanup writes with restoration, without nesting subscription state locks. I added tests for cleanup arriving after an owner replacement and for repeated replacement of the same channel; both pass. Late acknowledgements also leave the replacement owner subscribed.

mgravell · 2026-11-11T09:31:44Z

@@ -263,7 +267,9 @@ internal void ResubscribeToServer(Subscription sub, in RedisChannel channel, Ser
                    // we'll *try* for a simple resubscribe, following any -MOVED etc, but if that fails: fall back
                    // to full reconfigure; importantly, note that we've already recorded the disconnect
                    var message = sub.GetSubscriptionMessage(channel, SubscriptionAction.Subscribe, CommandFlags.None, false);


Minor: on the SingleNodeSubscription branch the message built here is never used — EnsureSubscribedToServerAsync constructs its own — so it's a dead allocation (including the tracked-reply box). Building it only on the ExecuteAsync branch avoids that.

Moved message construction into the multi-node execution branch. The single-node branch now uses only the message created by its own restore operation. The Pub/Sub and restoration regression suites pass with this change.

mgravell · 2026-11-11T09:31:44Z

            ChannelMessageQueue? queues = null;
            if (subscriptions.TryGetValue(subscription, out Subscription? sub))
            {
+                if (source is not null && sub is SingleNodeSubscription single && !single.Accepts(source)) return;


Nit, take or leave: this type-test/downcast sits on the hot delivery path, and it bakes OnMessage's knowledge of which subscription flavors filter. A virtual bool Accepts(PhysicalConnection) on Subscription (base: true; overridden in SingleNodeSubscription) expresses the invariant polymorphically and keeps this call site agnostic as subscription types evolve.

I've taken this suggestion: Subscription.Accepts(PhysicalConnection) defaults to true, and SingleNodeSubscription overrides it with the physical-connection ownership check. The delivery path now just calls the predicate. The reconnect regression and in-process Pub/Sub tests pass.

Preserve Sentinel observations without holding the reconfigure claim during discovery. Keep the confirmed delivery route until the replacement subscribe acknowledgement, and defer unsubscribe cleanup when restoration is in flight. Complete acknowledgements even when bookkeeping fails, preserve AsyncState and subscription flags, and move cleanup dispatch outside subscription state locks. Recover sharded restores backlogged on retired nodes and add regression tests. Validated with Redis/Sentinel 8.0.3 in isolated local Docker clusters using RESP2 and RESP3: 70 controlled switches and six additional fault scenarios. No duplicate deliveries were observed with this revision; failover message loss is still possible. Focused regression tests and Windows/Linux unit suites passed.

vkyow · 2026-11-11T14:58:03Z

Thanks for the review. I've pushed 65fdeb7 and replied to each thread with the changes and the relevant regression tests.

The update keeps a confirmed delivery route alive until the replacement subscription ACK, fixes deferred unsubscribe cleanup, and moves Sentinel discovery outside the general reconfigure claim. It also preserves the existing task state and subscription flags. While checking the retirement test, I fixed a sharded restore that could remain backlogged on a retired node and hold up later restoration.

I repeated the local tests against Redis and Sentinel 8.0.3, using a standalone client harness with one business subscription per protocol and no application framework or message deduplication:

RESP2 and RESP3, with authenticated clusters of three Redis/Sentinel pairs and five pairs. Three fresh clusters of each size ran ten switches each. Literal, sequential-queue and fault profiles brought the total to 70 controlled primary switches plus six additional fault scenarios.
The extra faults covered a replica restart, a paused primary, Sentinel restarts, repeated client-connection kills, and a primary outage with two of five Sentinels paused. Both 10,000/60,000 ms and 30,000/60,000 ms Sentinel timing profiles were exercised. Active traffic continued for at least two minutes after each recovery.
The revised client published 100,280 numbered messages. Packet captures showed no repeated group/message IDs, and all 184,316 expected post-ready consumer deliveries arrived exactly once. The longest recovery measured from confirmation of the new primary was 16.22 seconds. Each stable snapshot showed one business subscription route per consumer; the sequential queue stayed ordered.
Released 3.4.31 reproduced duplicate delivery in all three fresh control clusters, with each message ID published once. This HA control comparison is against the released package, not the previous PR commit.

There was still message loss during failover: 10,714 missed consumer deliveries for acknowledged publications across the two protocol consumers, all outside the post-ready windows. There were also nine failed publication attempts, including six IDs seen on the wire without a publication acknowledgement. The harness records those separately and does not retry them. This does not establish zero-loss delivery.

The 59 focused review/ownership/Sentinel/retirement cases pass. The review regressions exposed 30 failures when run against the previous PR code. Windows and Linux unit suites each passed 2,034 tests; the additional local Pub/Sub, Scan and Transaction checks also passed. All five shipping target frameworks built and packed locally, and the harness loaded the exact DLL from that local package. The previously reported Scan/Transaction CI failures did not reproduce in the local fixtures on either unchanged upstream or this revision.

All test clusters used internal Docker networks with no published ports, and all task-owned containers and networks have been removed. Detailed logs, per-message accounting and packet captures are retained locally. This is local validation; the new GitHub CI run still needs to be checked.

vkyow added 2 commits November 6, 2026 12:22

Fix concurrent single-node Pub/Sub subscription restoration

7de1db3

Coalesce Sentinel discovery before starting reconfiguration

f43e18d

mgravell reviewed Nov 11, 2026

View reviewed changes

Conversation

vkyow commented Nov 6, 2026

Validation

Checklist

Uh oh!

mgravell commented Nov 9, 2026

Uh oh!

mgravell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkyow commented Nov 11, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants