Maintenance notifications: act on MOVING, and test it against a real deployment (rest of D6, and D9's dedicated testing)

src/StackExchange.Redis/ConnectionMultiplexer.Events.cs

@@ -89,6 +89,17 @@ private void OnEndpointChanged(EndPoint endpoint, EventHandler<EndPointEventArgs
     /// </summary>
     public event EventHandler<ServerMaintenanceEvent>? ServerMaintenanceEvent;
 
+    /// <summary>
+    /// How host names are resolved during a maintenance handoff.
+    /// </summary>
+    /// <remarks>
+    /// A seam rather than a call to <see cref="System.Net.Dns"/> directly, because no in-process fake can move a
+    /// DNS record: the handoff's whole decision turns on the answer *changing*, and the only way to test that
+    /// deterministically is to supply the answers. Defaults to real DNS.
+    /// </remarks>
+    internal Func<string, System.Threading.CancellationToken, System.Threading.Tasks.Task<System.Net.IPAddress[]>> AddressResolver { get; set; }
+        = Maintenance.AdvertisedAddressProbe.DefaultResolveAsync;
+
     // recently-raised (type, sequence) pairs, so one logical event raises one event however many nodes told
     // us. Small and fixed: the copies arrive within milliseconds of each other, so a handful of slots covers
     // any realistic proxy count even with other notifications interleaved.

src/StackExchange.Redis/Enums/ConnectionFailureType.cs

@@ -68,5 +68,22 @@ public enum ConnectionFailureType
         /// </summary>
         [Experimental(Experiments.GeoRedundantFailover, UrlFormat = Experiments.UrlFormat)]
         CircuitBreaker,
+
+        /// <summary>
+        /// The connection was replaced deliberately, to move off an endpoint the server announced it is
+        /// retiring.
+        /// </summary>
+        /// <remarks>
+        /// Not a fault: the connection was working, and we chose to replace it while a replacement address was
+        /// known rather than wait to be disconnected. Reported here because the alternative is worse - the
+        /// replacement raises <see cref="ConnectionMultiplexer.ConnectionRestored"/>, so without this a consumer
+        /// tracking connection state sees a restore with no matching failure, and no reason for the churn.
+        /// <para>
+        /// Consumers alerting on <see cref="ConnectionMultiplexer.ConnectionFailed"/> should filter this out:
+        /// it is expected during planned maintenance and says nothing is wrong. <see cref="CircuitBreaker"/> is
+        /// the precedent for a deliberate client action being reported this way.
+        /// </para>
+        /// </remarks>
+        MaintenanceHandoff,
     }
 }

src/StackExchange.Redis/Maintenance/AdvertisedAddressProbe.cs

@@ -30,8 +30,10 @@ namespace StackExchange.Redis.Maintenance;
 /// </para>
 /// <para>
 /// The rule is "take any address that is not the one being retired". Note it is deliberately *not* "prefer an
-/// address that has newly appeared", even though `MOVING` is emitted precisely when the address set gains a
-/// member (established across nine observations: it is silent whenever the set only loses members). A live
+/// address that has newly appeared", even though a <c>MOVING</c> implies one exists - it is emitted when the
+/// connection's own proxy *leaves* the endpoint's address set **and** the set gains a member (established
+/// across thirteen observations: a pure reduction takes the proxy away without announcing anything, and a pure
+/// widening adds addresses while leaving the connection's proxy in place, which is equally silent). A live
 /// sibling is at least as good a target as a newly joined node and is available *now*, whereas the new node
 /// only becomes visible when the record updates - which can be after the socket has already closed. Preferring
 /// the newcomer would mean waiting for it, and waiting is the thing to avoid. The one case where it would pay
@@ -56,6 +58,16 @@ namespace StackExchange.Redis.Maintenance;
 /// </remarks>
 internal static class AdvertisedAddressProbe
 {
+    /// <summary>
+    /// Ordinary DNS, which is what everything but a test uses.
+    /// </summary>
+    internal static Task<IPAddress[]> DefaultResolveAsync(string host, CancellationToken cancellationToken) =>
+#if NET
+        Dns.GetHostAddressesAsync(host, cancellationToken);
+#else
+        Dns.GetHostAddressesAsync(host);
+#endif
+
     /// <summary>
     /// Polls DNS until it stops naming <paramref name="retiring"/>, or until the window runs out.
     /// </summary>

src/StackExchange.Redis/Maintenance/MaintenanceHandoff.cs

@@ -0,0 +1,133 @@
+using System;
+using System.Net;
+using System.Threading;
+using System.Threading.Tasks;
+
+namespace StackExchange.Redis.Maintenance;
+
+/// <summary>
+/// What to do about a <c>MOVING</c> notification.
+/// </summary>
+internal enum HandoffAction
+{
+    /// <summary>Nothing useful is available; let the server close the socket and reconnect then.</summary>
+    None,
+
+    /// <summary>Drop our connections so they re-establish against the replacement address.</summary>
+    Recycle,
+
+    /// <summary>The replacement is a different endpoint; the topology has to be re-read to find it.</summary>
+    Reconfigure,
+}
+
+/// <summary>
+/// The outcome of deciding how to hand off, and why - the reason is logged either way.
+/// </summary>
+internal readonly struct HandoffDecision(HandoffAction action, EndPoint? target, string reason)
+{
+    public HandoffAction Action { get; } = action;
+
+    public EndPoint? Target { get; } = target;
+
+    public string Reason { get; } = reason;
+
+    public override string ToString() => Target is null ? $"{Action}: {Reason}" : $"{Action} -> {Target}: {Reason}";
+}
+
+/// <summary>
+/// Decides what a <c>MOVING</c> notification means for this connection, and where to go.
+/// </summary>
+/// <remarks>
+/// Separated from the acting so that it can be tested exhaustively without a server: the caller supplies the
+/// endpoint, the address it is currently on, the announced window and a resolver, and gets back an action.
+/// <para>
+/// The dispatch turns on the *form* of the endpoint, which is the thing that decides whether a handoff is even
+/// possible - and the answer corrects an earlier assumption that <c>MOVING</c> should reuse the endpoint
+/// retirement path:
+/// </para>
+/// <list type="bullet">
+/// <item>
+/// A hostname endpoint with no named successor - the case every observed <c>MOVING</c> has been - keeps its
+/// <see cref="ServerEndPoint"/>: only the address behind the name moves. Retiring the endpoint would delete our
+/// only way of reaching the deployment. So the action is to recycle the connections once DNS has moved, which
+/// re-resolves them.
+/// </item>
+/// <item>
+/// An address endpoint with a named successor is genuinely a different endpoint, so the topology has to be
+/// re-read. Never observed: eleven routes have all carried an explicit null, including cases where the server
+/// had already chosen the replacement node. Treat this branch as code that must exist rather than code that is
+/// exercised.
+/// </item>
+/// <item>
+/// An address endpoint with no successor has nothing to re-resolve and nowhere named to go. Doing nothing is
+/// correct: the socket closes, the reconnect happens, and the relaxed window covers it.
+/// </item>
+/// </list>
+/// </remarks>
+internal static class MaintenanceHandoff
+{
+    internal static async Task<HandoffDecision> DecideAsync(
+        EndPoint endpoint,
+        EndPoint? successor,
+        IPAddress? currentAddress,
+        TimeSpan window,
+        TimeSpan pollInterval,
+        Func<string, CancellationToken, Task<IPAddress[]>> resolve,
+        Action<string>? log = null,
+        CancellationToken cancellationToken = default)
+    {
+        if (successor is not null)
+        {
+            // Nothing is asserted about *which* endpoint: a successor may be an address we have never seen, so
+            // finding it is a topology question rather than a DNS one.
+            return new HandoffDecision(HandoffAction.Reconfigure, successor, "the server named a replacement endpoint");
+        }
+
+        if (endpoint is not DnsEndPoint dns)
+        {
+            return new HandoffDecision(
+                HandoffAction.None,
+                null,
+                $"{Format.ToString(endpoint)} is an address, not a name, and no replacement was named: nothing to re-resolve");
+        }
+
+        if (currentAddress is null)
+        {
+            // Without knowing where we are, "has it moved" is unanswerable - and guessing would mean recycling
+            // onto whatever DNS says right now, which for the first several seconds is the address being retired.
+            return new HandoffDecision(HandoffAction.None, null, "the address of the current connection is unknown");
+        }
+
+        var replacement = await AdvertisedAddressProbe.ProbeAsync(
+            dns, currentAddress, window, pollInterval, resolve, log, cancellationToken).ForAwait();
+
+        return replacement is null
+            ? new HandoffDecision(
+                HandoffAction.None,
+                null,
+                $"{dns.Host} still resolved only to {currentAddress} when the window expired")
+            : new HandoffDecision(
+                HandoffAction.Recycle,
+                replacement,
+                $"{dns.Host} now resolves to {replacement}");
+    }
+
+    /// <summary>
+    /// How long to wait before probing, so a fleet does not resolve in lockstep.
+    /// </summary>
+    /// <remarks>
+    /// A *fraction* of the announced window rather than a fixed delay, which is the difference from the refresh
+    /// jitter elsewhere. Windows are not always generous - the shard notifications have been measured announcing
+    /// two seconds - so a flat one-second jitter could spend half of one, and on a short window the right amount
+    /// of jitter is almost none. Capped as well as scaled, because a long window does not justify a long wait
+    /// when DNS has been seen moving after four seconds.
+    /// </remarks>
+    internal static TimeSpan GetJitter(TimeSpan window, Random random)
+    {
+        if (window <= TimeSpan.Zero) return TimeSpan.Zero;
+
+        var tenth = window.TotalMilliseconds / 10;
+        var capped = Math.Min(tenth, 1000);
+        return TimeSpan.FromMilliseconds(random.Next(0, (int)Math.Max(capped, 1)));
+    }
+}

src/StackExchange.Redis/PhysicalBridge.cs

@@ -1794,6 +1794,33 @@ internal void SimulateConnectionFailure(SimulatedFailureType failureType)
             physical?.SimulateConnectionFailure(failureType);
         }
 
+        /// <summary>
+        /// Drops the current connection so that a fresh one is established.
+        /// </summary>
+        /// <remarks>
+        /// Deliberately just a dispose: that routes through <c>RecordConnectionFailed</c> to
+        /// <see cref="OnDisconnected"/>, which reconnects immediately by itself, so there is no new lifecycle
+        /// here to get wrong. Used by the <c>MOVING</c> handoff, where the point is to choose *when* the
+        /// connection is replaced - after the replacement address is known - rather than waiting for the server
+        /// to close it and re-resolving to whatever DNS says at that moment, which has been measured as still
+        /// naming the node being retired.
+        /// </remarks>
+        internal bool RecycleConnection(string reason)
+        {
+            var current = physical;
+            if (current is null) return false;
+
+            Multiplexer.Trace($"recycling {ConnectionType} connection: {reason}", ToString());
+
+            // Report *before* tearing down, and in this order for a reason: RecordConnectionFailed only raises
+            // the public event while the pipe is still live ("if *we* didn't burn the pipe: flag it"), and
+            // Dispose runs Shutdown first - which is exactly why an ordinary dispose is silent. Recording first
+            // also performs the disconnect, so the Dispose below is cleanup.
+            current.RecordConnectionFailed(ConnectionFailureType.MaintenanceHandoff);
+            current.Dispose();
+            return true;
+        }
+
         internal RedisCommand? GetActiveMessage() => Volatile.Read(ref _activeMessage)?.Command;
     }
 }

src/StackExchange.Redis/PhysicalConnection.Maintenance.cs

@@ -130,7 +130,21 @@ private OutOfBandResult OnMaintenanceNotification(ConnectionMultiplexer muxer, P
         {
             if (IsWindowOpening(type))
             {
-                server.OnMaintenanceWindowOpened(type, sequenceId, time);
+                var isNew = server.OnMaintenanceWindowOpened(type, sequenceId, time);
+
+                // ...and MOVING alone means "this endpoint is going away", which is worth acting on rather than
+                // waiting to be disconnected.
+                //
+                // Only when the notification is *new*, and that is load-bearing rather than tidy. A server
+                // re-sends MOVING to a connection that opts in while the window is still open - measured, and
+                // the handoff replaces the connection, so acting on the repeat is a feedback loop: recycle,
+                // reconnect, get told again, recycle. It produced twelve recycles from one event on a real
+                // deployment before this guard. The per-server sequence dedup already knew it was a repeat; the
+                // handoff simply was not asking.
+                if (isNew && type == MaintenanceNotificationType.Moving)
+                {
+                    server.OnMovingAnnounced(time, newEndPoint, this);
+                }
             }
             else if (IsWindowClosing(type))
             {

src/StackExchange.Redis/PublicAPI/PublicAPI.Unshipped.txt

@@ -175,3 +175,4 @@ override StackExchange.Redis.Configuration.DefaultOptionsProvider.ToString() ->
 [SER010]StackExchange.Redis.Maintenance.ClusterSlotMigration.Target.get -> System.Net.EndPoint?
 [SER010]StackExchange.Redis.Maintenance.PushMaintenanceEvent.SlotMigrations.get -> System.Collections.Generic.IReadOnlyList<StackExchange.Redis.Maintenance.ClusterSlotMigration>!
 [SER007]StackExchange.Redis.Availability.HealthCheckContext.ProbeFlags.get -> StackExchange.Redis.CommandFlags
+StackExchange.Redis.ConnectionFailureType.MaintenanceHandoff = 12 -> StackExchange.Redis.ConnectionFailureType