Fix restart publication races (#3186)

Prepare offline routing before restart requests become observable, reject stale route owners, and wait for source-client cleanup before executing automatic retries.

Add deterministic publication-order, cleanup-gate, and stale-route regression tests.

MinecraftClient.Tests/McClientConnectionFailureTests.cs

@@ -55,8 +55,8 @@ public sealed class McClientConnectionFailureTests
         int activations = 0;
         int deactivations = 0;
 
-        Assert.True(route.TryActivate(7, () => activations++));
+        Assert.True(route.TryActivate(7, 7, () => activations++));
-        Assert.False(route.TryActivate(7, () => activations++));
+        Assert.False(route.TryActivate(7, 7, () => activations++));
         Assert.True(route.TryTransfer(7, 8));
         Assert.False(route.TryDeactivate(7, () => deactivations++));
         Assert.Equal(8, route.OwnerAttempt);
@@ -73,20 +73,32 @@ public sealed class McClientConnectionFailureTests
         var route = new AttemptOwnedRoute();
         int activations = 0;
 
-        Assert.True(route.TryActivate(0, () => activations++));
+        Assert.True(route.TryActivate(0, 0, () => activations++));
 
         Assert.Equal(1, activations);
         Assert.Equal(0, route.OwnerAttempt);
     }
 
+    [Fact]
+    public void OlderAttemptCannotTakeAnEmptyOfflineRoute()
+    {
+        var route = new AttemptOwnedRoute();
+        int activations = 0;
+
+        Assert.False(route.TryActivate(4, 5, () => activations++));
+
+        Assert.Equal(0, activations);
+        Assert.Equal(-1, route.OwnerAttempt);
+    }
+
     [Fact]
     public void StaleCleanupCannotClearNewerOfflineRoute()
     {
         var route = new AttemptOwnedRoute();
         int deactivations = 0;
 
-        Assert.True(route.TryActivate(10, () => { }));
+        Assert.True(route.TryActivate(10, 10, () => { }));
-        Assert.True(route.TryActivate(11, () => { }));
+        Assert.True(route.TryActivate(11, 11, () => { }));
         Assert.False(route.TryDeactivate(10, () => deactivations++));
 
         Assert.Equal(11, route.OwnerAttempt);

MinecraftClient.Tests/RestartCoordinatorTests.cs

@@ -2,6 +2,90 @@ namespace MinecraftClient.Tests;
 
 public sealed class RestartCoordinatorTests
 {
+    [Fact]
+    public async Task PreparationCompletesBeforeRequestCanExecute()
+    {
+        var completed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        bool prepared = false;
+
+        RestartCoordinator coordinator = null!;
+        coordinator = new RestartCoordinator(
+            (request, cancellationToken) =>
+            {
+                Assert.True(Volatile.Read(ref prepared));
+                Assert.True(coordinator.TryBeginCommit(request, out _));
+                completed.SetResult();
+                return Task.CompletedTask;
+            },
+            exception => throw new Xunit.Sdk.XunitException(exception.ToString()));
+        using var cleanup = coordinator;
+
+        Assert.True(coordinator.TrySchedule(
+            new RestartRequest(1, TimeSpan.Zero, true),
+            () =>
+            {
+                Volatile.Write(ref prepared, true);
+                return true;
+            }));
+        await completed.Task.WaitAsync(TimeSpan.FromSeconds(5));
+    }
+
+    [Fact]
+    public async Task RejectedPreparationDoesNotPublishOrAdvanceAttempt()
+    {
+        var completed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        int executions = 0;
+
+        RestartCoordinator coordinator = null!;
+        coordinator = new RestartCoordinator(
+            (request, cancellationToken) =>
+            {
+                Interlocked.Increment(ref executions);
+                Assert.True(coordinator.TryBeginCommit(request, out _));
+                completed.SetResult();
+                return Task.CompletedTask;
+            },
+            exception => throw new Xunit.Sdk.XunitException(exception.ToString()));
+        using var cleanup = coordinator;
+
+        Assert.False(coordinator.TrySchedule(
+            new RestartRequest(2, TimeSpan.Zero, true),
+            () => false));
+        Assert.False(coordinator.HasScheduledRestart(2));
+
+        Assert.True(coordinator.TrySchedule(new RestartRequest(2, TimeSpan.Zero, true)));
+        await completed.Task.WaitAsync(TimeSpan.FromSeconds(5));
+
+        Assert.Equal(1, executions);
+    }
+
+    [Fact]
+    public async Task FaultedSourceCleanupPreventsRestartExecution()
+    {
+        var failureReported = new TaskCompletionSource<Exception>(TaskCreationOptions.RunContinuationsAsynchronously);
+        int executions = 0;
+
+        using var coordinator = new RestartCoordinator(
+            (_, _) =>
+            {
+                Interlocked.Increment(ref executions);
+                return Task.CompletedTask;
+            },
+            exception => failureReported.SetResult(exception));
+
+        var cleanupFailure = new InvalidOperationException("cleanup failed");
+        Assert.True(coordinator.TrySchedule(new RestartRequest(
+            3,
+            TimeSpan.Zero,
+            true,
+            SourceCleanupCompletion: Task.FromException(cleanupFailure))));
+
+        Exception reportedException = await failureReported.Task.WaitAsync(TimeSpan.FromSeconds(5));
+
+        Assert.Same(cleanupFailure, reportedException);
+        Assert.Equal(0, executions);
+    }
+
     [Fact]
     public async Task AutomaticSameAttemptIsCoalescedWhileQueued()
     {

MinecraftClient/ChatBots/AutoRelog.cs

@@ -170,7 +170,11 @@ namespace MinecraftClient.ChatBots
 
             McClient.ReconnectionAttemptsLeft = retriesLeft;
             long connectionAttempt = sourceConnectionAttempt ?? Handler.ConnectionAttempt;
-            if (Program.TryRestart(connectionAttempt, TimeSpan.FromSeconds(delay), true))
+            if (Program.TryRestart(
+                    connectionAttempt,
+                    TimeSpan.FromSeconds(delay),
+                    keepAccountAndServerSettings: true,
+                    sourceCleanupCompletion: sourceConnectionAttempt.HasValue ? null : Handler.DisconnectCompletion))
             {
                 LogToConsole(string.Format(Translations.bot_autoRelog_wait_with_retries, delay, retriesDisplay));
                 return true;

MinecraftClient/ConnectionAttemptLifecycle.cs

@@ -47,13 +47,13 @@ namespace MinecraftClient
             }
         }
 
-        internal bool TryActivate(long connectionAttempt, Action activate)
+        internal bool TryActivate(long connectionAttempt, long currentConnectionAttempt, Action activate)
         {
             ArgumentNullException.ThrowIfNull(activate);
 
             lock (stateLock)
             {
-                if (ownerAttempt >= connectionAttempt)
+                if (connectionAttempt < currentConnectionAttempt || ownerAttempt >= connectionAttempt)
                     return false;
 
                 ownerAttempt = connectionAttempt;

MinecraftClient/McClient.cs

@@ -238,6 +238,7 @@ namespace MinecraftClient
         public ILogger Log;
         public DialogManager Dialogs { get; }
         internal long ConnectionAttempt { get; }
+        internal Task DisconnectCompletion => disconnectCompletion.Task;
 
         private static IMinecraftComHandler? instance;
         public static IMinecraftComHandler? Instance => instance;

MinecraftClient/Program.cs

@@ -1095,11 +1095,15 @@ namespace MinecraftClient
             long sourceConnectionAttempt,
             TimeSpan delay,
             bool keepAccountAndServerSettings = false,
-            bool replaceUntilCommit = false)
+            bool replaceUntilCommit = false,
+            Task? sourceCleanupCompletion = null)
         {
             if (Volatile.Read(ref exitOnFailurePending) != 0)
                 return false;
 
+            if (sourceConnectionAttempt != CurrentConnectionAttempt)
+                return false;
+
             if (delay < TimeSpan.Zero)
                 delay = TimeSpan.Zero;
 
@@ -1107,14 +1111,15 @@ namespace MinecraftClient
                 ? CaptureRestartSettings()
                 : null;
 
-            bool scheduled = restartCoordinator.TrySchedule(new RestartRequest(
+            bool scheduled = restartCoordinator.TrySchedule(
-                sourceConnectionAttempt,
+                new RestartRequest(
-                delay,
+                    sourceConnectionAttempt,
-                keepAccountAndServerSettings,
+                    delay,
-                settingsSnapshot,
+                    keepAccountAndServerSettings,
-                replaceUntilCommit));
+                    settingsSnapshot,
-            if (scheduled)
+                    replaceUntilCommit,
-                BeginOfflinePrompt(sourceConnectionAttempt);
+                    sourceCleanupCompletion),
+                () => BeginOfflinePrompt(sourceConnectionAttempt));
             return scheduled;
         }
 
@@ -1265,9 +1270,13 @@ namespace MinecraftClient
             }
         }
 
-        private static void BeginOfflinePrompt(long connectionAttempt)
+        private static bool BeginOfflinePrompt(long connectionAttempt)
         {
-            offlinePromptRoute.TryActivate(connectionAttempt, () =>
+            long currentConnectionAttempt = CurrentConnectionAttempt;
+            if (connectionAttempt != currentConnectionAttempt)
+                return false;
+
+            if (offlinePromptRoute.TryActivate(connectionAttempt, currentConnectionAttempt, () =>
             {
                 ConsoleInputRouter.RouteOffline(HandleOfflineCommand);
                 ConsoleIO.WriteLine(string.Empty);
@@ -1276,7 +1285,12 @@ namespace MinecraftClient
                     ConsoleIO.WriteLineFormatted(string.Format(Translations.mcc_use_quit_to_exit, Config.Main.Advanced.InternalCmdChar.ToLogString()));
                 else
                     ConsoleIO.WriteLineFormatted(Translations.mcc_press_exit, acceptnewlines: true);
-            });
+            }))
+            {
+                return true;
+            }
+
+            return offlinePromptRoute.OwnerAttempt == connectionAttempt;
         }
 
         private static void EndOfflinePrompt()

MinecraftClient/RestartCoordinator.cs

@@ -12,6 +12,7 @@ namespace MinecraftClient
         bool KeepAccountAndServerSettings,
         RestartSettingsSnapshot? SettingsSnapshot = null,
         bool ReplaceUntilCommit = false,
+        Task? SourceCleanupCompletion = null,
         long RequestId = 0);
 
     internal readonly record struct RestartSettingsSnapshot(
@@ -67,7 +68,7 @@ namespace MinecraftClient
                 return !stopped && pendingAttempts.ContainsKey(connectionAttempt);
         }
 
-        internal bool TrySchedule(RestartRequest request)
+        internal bool TrySchedule(RestartRequest request, Func<bool>? beforePublish = null)
         {
             lock (stateLock)
             {
@@ -79,7 +80,11 @@ namespace MinecraftClient
                     if (pendingRequest.State != RestartRequestState.Replaceable || !request.ReplaceUntilCommit)
                         return false;
 
-                    request = request with { RequestId = pendingRequest.RequestId };
+                    request = request with
+                    {
+                        RequestId = pendingRequest.RequestId,
+                        SourceCleanupCompletion = pendingRequest.Request.SourceCleanupCompletion,
+                    };
                     pendingAttempts[request.ConnectionAttempt] = pendingRequest with { Request = request };
                     return true;
                 }
@@ -87,14 +92,30 @@ namespace MinecraftClient
                 if (request.ConnectionAttempt <= highestScheduledAttempt)
                     return false;
 
-                highestScheduledAttempt = Math.Max(highestScheduledAttempt, request.ConnectionAttempt);
                 request = request with { RequestId = ++nextRequestId };
                 pendingAttempts[request.ConnectionAttempt] = new PendingRestart(
                     request.RequestId,
                     request,
                     RestartRequestState.Replaceable);
-                if (requests.Writer.TryWrite(request))
+                try
-                    return true;
+                {
+                    if (beforePublish is not null && !beforePublish())
+                    {
+                        pendingAttempts.Remove(request.ConnectionAttempt);
+                        return false;
+                    }
+
+                    if (requests.Writer.TryWrite(request))
+                    {
+                        highestScheduledAttempt = Math.Max(highestScheduledAttempt, request.ConnectionAttempt);
+                        return true;
+                    }
+                }
+                catch
+                {
+                    pendingAttempts.Remove(request.ConnectionAttempt);
+                    throw;
+                }
 
                 pendingAttempts.Remove(request.ConnectionAttempt);
                 return false;
@@ -152,6 +173,9 @@ namespace MinecraftClient
 
                     try
                     {
+                        if (request.SourceCleanupCompletion is Task sourceCleanupCompletion)
+                            await sourceCleanupCompletion.WaitAsync(shutdown.Token).ConfigureAwait(false);
+
                         await restart(request, shutdown.Token).ConfigureAwait(false);
                     }
                     catch (OperationCanceledException) when (shutdown.IsCancellationRequested)