From 5655e3fc896e6306a23f437e7b386dd17dcdc106 Mon Sep 17 00:00:00 2001 From: Anon Date: Mon, 27 Jul 2026 17:12:03 +0200 Subject: [PATCH] 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. --- .../McClientConnectionFailureTests.cs | 22 +++-- .../RestartCoordinatorTests.cs | 84 +++++++++++++++++++ MinecraftClient/ChatBots/AutoRelog.cs | 6 +- MinecraftClient/ConnectionAttemptLifecycle.cs | 4 +- MinecraftClient/McClient.cs | 1 + MinecraftClient/Program.cs | 38 ++++++--- MinecraftClient/RestartCoordinator.cs | 34 ++++++-- 7 files changed, 164 insertions(+), 25 deletions(-) diff --git a/MinecraftClient.Tests/McClientConnectionFailureTests.cs b/MinecraftClient.Tests/McClientConnectionFailureTests.cs index 7291e1cc..3789e531 100644 --- a/MinecraftClient.Tests/McClientConnectionFailureTests.cs +++ b/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.False(route.TryActivate(7, () => activations++)); + Assert.True(route.TryActivate(7, 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(11, () => { })); + Assert.True(route.TryActivate(10, 10, () => { })); + Assert.True(route.TryActivate(11, 11, () => { })); Assert.False(route.TryDeactivate(10, () => deactivations++)); Assert.Equal(11, route.OwnerAttempt); diff --git a/MinecraftClient.Tests/RestartCoordinatorTests.cs b/MinecraftClient.Tests/RestartCoordinatorTests.cs index 62e35e61..f9c9edef 100644 --- a/MinecraftClient.Tests/RestartCoordinatorTests.cs +++ b/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(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() { diff --git a/MinecraftClient/ChatBots/AutoRelog.cs b/MinecraftClient/ChatBots/AutoRelog.cs index 92333db2..3a07f7f4 100644 --- a/MinecraftClient/ChatBots/AutoRelog.cs +++ b/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; diff --git a/MinecraftClient/ConnectionAttemptLifecycle.cs b/MinecraftClient/ConnectionAttemptLifecycle.cs index de3689f0..360ae013 100644 --- a/MinecraftClient/ConnectionAttemptLifecycle.cs +++ b/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; diff --git a/MinecraftClient/McClient.cs b/MinecraftClient/McClient.cs index 92e4ab73..11d860cd 100644 --- a/MinecraftClient/McClient.cs +++ b/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; diff --git a/MinecraftClient/Program.cs b/MinecraftClient/Program.cs index 874733ba..dcf5daaf 100644 --- a/MinecraftClient/Program.cs +++ b/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( - sourceConnectionAttempt, - delay, - keepAccountAndServerSettings, - settingsSnapshot, - replaceUntilCommit)); - if (scheduled) - BeginOfflinePrompt(sourceConnectionAttempt); + bool scheduled = restartCoordinator.TrySchedule( + new RestartRequest( + sourceConnectionAttempt, + delay, + keepAccountAndServerSettings, + settingsSnapshot, + replaceUntilCommit, + 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() diff --git a/MinecraftClient/RestartCoordinator.cs b/MinecraftClient/RestartCoordinator.cs index af3eb8c8..00a75994 100644 --- a/MinecraftClient/RestartCoordinator.cs +++ b/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? 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)) - return true; + try + { + 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)