From 08ea2b7764810a5a261dc24206f399d4b2c4a5e9 Mon Sep 17 00:00:00 2001 From: Jordan Date: Sun, 18 Jun 2023 13:24:59 -0400 Subject: [PATCH 1/3] Prevent reboot bridge request completing too soon --- .../Components/Session/ISessionController.cs | 5 ++++ .../Components/Session/SessionController.cs | 28 +++++++++++++++++++ .../Components/Watchdog/WatchdogBase.cs | 8 ++++++ 3 files changed, 41 insertions(+) diff --git a/src/Tgstation.Server.Host/Components/Session/ISessionController.cs b/src/Tgstation.Server.Host/Components/Session/ISessionController.cs index d9f9fb9f26..3ded84429a 100644 --- a/src/Tgstation.Server.Host/Components/Session/ISessionController.cs +++ b/src/Tgstation.Server.Host/Components/Session/ISessionController.cs @@ -69,6 +69,11 @@ namespace Tgstation.Server.Host.Components.Session /// Task OnReboot { get; } + /// + /// A that must complete before a TgsReboot() bridge request can complete. + /// + Task RebootGate { set; } + /// /// A that completes when the server calls /world/TgsInitializationComplete(). /// diff --git a/src/Tgstation.Server.Host/Components/Session/SessionController.cs b/src/Tgstation.Server.Host/Components/Session/SessionController.cs index 8b120bec72..36ef827390 100644 --- a/src/Tgstation.Server.Host/Components/Session/SessionController.cs +++ b/src/Tgstation.Server.Host/Components/Session/SessionController.cs @@ -75,6 +75,26 @@ namespace Tgstation.Server.Host.Components.Session /// public Task OnReboot => rebootTcs.Task; + /// + public Task RebootGate + { + get => rebootGate; + set + { + var tcs = new TaskCompletionSource(); + Task toAwait = null; + async Task Wrap() + { + await tcs.Task; + await toAwait; + await value; + } + + toAwait = Interlocked.Exchange(ref rebootGate, Wrap()); + tcs.SetResult(); + } + } + /// public Task OnPrime => primeTcs.Task; @@ -164,6 +184,11 @@ namespace Tgstation.Server.Host.Components.Session /// volatile TaskCompletionSource primeTcs; + /// + /// Backing field for . + /// + volatile Task rebootGate; + /// /// The number of currently active calls to from TgsReboot(). /// @@ -254,6 +279,8 @@ namespace Tgstation.Server.Host.Components.Session rebootTcs = new TaskCompletionSource(); primeTcs = new TaskCompletionSource(); + rebootGate = Task.CompletedTask; + // Run this asynchronously because we want to try to avoid any effects sending topics to the server while the initial bridge request is processing // It MAY be the source of a DD crash. See this gist https://gist.github.com/Cyberboss/7776bbeff3a957d76affe0eae95c9f14 // Worth further investigation as to if that sequence of events is a reliable crash vector and opening a BYOND bug if it is @@ -751,6 +778,7 @@ namespace Tgstation.Server.Host.Components.Session } Interlocked.Exchange(ref rebootTcs, new TaskCompletionSource()).SetResult(); + await RebootGate; } finally { diff --git a/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs b/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs index 126f36628e..559e6a11a5 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs @@ -804,6 +804,8 @@ namespace Tgstation.Server.Host.Components.Watchdog var ranInitialDmbCheck = false; for (ulong iteration = 1; nextAction != MonitorAction.Exit; ++iteration) using (LogContext.PushProperty(SerilogContextHelper.WatchdogMonitorIterationContextProperty, iteration)) + { + TaskCompletionSource nextMonitorWakeupTcs = new TaskCompletionSource(); try { Logger.LogTrace("Iteration {iteration} of monitor loop", iteration); @@ -821,6 +823,7 @@ namespace Tgstation.Server.Host.Components.Watchdog oldTask = newTaskFactory(); } + controller.RebootGate = nextMonitorWakeupTcs.Task; if (lastController == controller) { TryUpdateTask(ref activeServerLifetime, () => controller.Lifetime); @@ -967,6 +970,11 @@ namespace Tgstation.Server.Host.Components.Watchdog nextAction = MonitorAction.Continue; } } + finally + { + nextMonitorWakeupTcs.SetResult(); + } + } } catch (OperationCanceledException) { From 7218702bd9441b2acc80648fc649e7e14588608d Mon Sep 17 00:00:00 2001 From: Jordan Date: Sun, 18 Jun 2023 18:50:37 -0400 Subject: [PATCH 2/3] Fix issues with renaming instances and setting auto-update intervals --- .../ComponentInterfacingController.cs | 7 ++++-- .../Controllers/InstanceController.cs | 24 +++++++++++-------- .../Live/InstanceManagerTest.cs | 7 ++++++ 3 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/Tgstation.Server.Host/Controllers/ComponentInterfacingController.cs b/src/Tgstation.Server.Host/Controllers/ComponentInterfacingController.cs index 46874a4142..09c74b5252 100644 --- a/src/Tgstation.Server.Host/Controllers/ComponentInterfacingController.cs +++ b/src/Tgstation.Server.Host/Controllers/ComponentInterfacingController.cs @@ -115,14 +115,17 @@ namespace Tgstation.Server.Host.Controllers /// Run a given with the relevant . /// /// A accepting the and returning a with the . + /// The to grab. If , will be used. /// A resulting in the that should be returned. /// The context of should be as small as possible so as to avoid race conditions. This function can return a if the requested instance was offline. - protected async Task WithComponentInstance(Func> action) + protected async Task WithComponentInstance(Func> action, Models.Instance instance = null) { if (action == null) throw new ArgumentNullException(nameof(action)); - using var instanceReference = instanceManager.GetInstanceReference(Instance); + instance ??= Instance; + + using var instanceReference = instanceManager.GetInstanceReference(instance); using (LogContext.PushProperty(SerilogContextHelper.InstanceReferenceContextProperty, instanceReference.Uid)) { if (instanceReference == null) diff --git a/src/Tgstation.Server.Host/Controllers/InstanceController.cs b/src/Tgstation.Server.Host/Controllers/InstanceController.cs index 2af7714f42..4206c04fac 100644 --- a/src/Tgstation.Server.Host/Controllers/InstanceController.cs +++ b/src/Tgstation.Server.Host/Controllers/InstanceController.cs @@ -429,11 +429,13 @@ namespace Tgstation.Server.Host.Controllers if (renamed) { // ignoring retval because we don't care if it's offline - await WithComponentInstance(async componentInstance => - { - await componentInstance.InstanceRenamed(originalModel.Name, cancellationToken); - return null; - }); + await WithComponentInstance( + async componentInstance => + { + await componentInstance.InstanceRenamed(originalModel.Name, cancellationToken); + return null; + }, + originalModel); } var oldAutoStart = originalModel.DreamDaemonSettings.AutoStart; @@ -491,11 +493,13 @@ namespace Tgstation.Server.Host.Controllers if (model.AutoUpdateInterval.HasValue && oldAutoUpdateInterval != model.AutoUpdateInterval) { // ignoring retval because we don't care if it's offline - await WithComponentInstance(async componentInstance => - { - await componentInstance.SetAutoUpdateInterval(model.AutoUpdateInterval.Value); - return null; - }); + await WithComponentInstance( + async componentInstance => + { + await componentInstance.SetAutoUpdateInterval(model.AutoUpdateInterval.Value); + return null; + }, + originalModel); } await CheckAccessible(api, cancellationToken); diff --git a/tests/Tgstation.Server.Tests/Live/InstanceManagerTest.cs b/tests/Tgstation.Server.Tests/Live/InstanceManagerTest.cs index 4dcefae0bc..b25ebe0efc 100644 --- a/tests/Tgstation.Server.Tests/Live/InstanceManagerTest.cs +++ b/tests/Tgstation.Server.Tests/Live/InstanceManagerTest.cs @@ -163,6 +163,13 @@ namespace Tgstation.Server.Tests.Live Assert.AreEqual(ConfigurationType.HostWrite, firstTest.ConfigurationType); Assert.IsTrue(Directory.Exists(firstTest.Path)); + // regression check + await instanceManagerClient.Update(new InstanceUpdateRequest + { + Id = firstTest.Id, + AutoUpdateInterval = 9999, + }, cancellationToken); + //can't move online instance await ApiAssert.ThrowsException(() => instanceManagerClient.Update(new InstanceUpdateRequest { From d169f4b6530a8de4188a2391d6e29ee977be54c5 Mon Sep 17 00:00:00 2001 From: Jordan Date: Sun, 18 Jun 2023 22:44:56 -0400 Subject: [PATCH 3/3] Another BetterWin32Errors possibility --- tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs b/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs index 2392ebb480..62aa017dac 100644 --- a/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs +++ b/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs @@ -306,7 +306,8 @@ namespace Tgstation.Server.Tests.Live.Instance || job.ExceptionDetails.Contains("BetterWin32Errors.Win32Exception: E_HANDLE: The handle is invalid.") || job.ExceptionDetails.Contains("BetterWin32Errors.Win32Exception: 3489660936: Unknown error (0xd0000008)") || job.ExceptionDetails.Contains("System.InvalidOperationException: No process is associated with this object.") - || job.ExceptionDetails.Contains("BetterWin32Errors.Win32Exception: 2147942424: The program issued a command but the command length is incorrect.")))) + || job.ExceptionDetails.Contains("BetterWin32Errors.Win32Exception: 2147942424: The program issued a command but the command length is incorrect.") + || job.ExceptionDetails.Contains("BetterWin32Errors.Win32Exception: 2147942699: Only part of a ReadProcessMemory or WriteProcessMemory request was completed.")))) break; var restartJob = await instanceClient.DreamDaemon.Restart(cancellationToken);