From ffd2786fc9908bda38a84fefd5b4770a76145330 Mon Sep 17 00:00:00 2001 From: Jordan Date: Fri, 16 Jun 2023 04:36:52 -0400 Subject: [PATCH 01/12] Only test topic limits + 1 in tests Fixes #1540 --- .../Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs b/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs index d560bcc9bf..b9278aec6d 100644 --- a/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs +++ b/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs @@ -530,10 +530,11 @@ namespace Tgstation.Server.Tests.Live.Instance }; var json = JsonConvert.SerializeObject(baseTopic, DMApiConstants.SerializerSettings); - var topicString = $"tgs_integration_test_tactics3={TopicClient.SanitizeString(json)}"; - var baseSize = topicString.Length; - var wrappingSize = baseSize; + var baseSize = (int)(DMApiConstants.MaximumTopicRequestLength - 1); + + var topicString = $"tgs_integration_test_tactics3={TopicClient.SanitizeString(json)}"; + var wrappingSize = topicString.Length; while (!cancellationToken.IsCancellationRequested) { @@ -584,7 +585,7 @@ namespace Tgstation.Server.Tests.Live.Instance System.Console.WriteLine("TEST: Receiving Topic tests topics..."); // Receive - baseSize = 1; + baseSize = (int)(DMApiConstants.MaximumTopicResponseLength - 1); nextPow = 0; lastSize = 0; while (!cancellationToken.IsCancellationRequested) From 3fa6adc37fbe257cda7de47287256f2e1edd96dd Mon Sep 17 00:00:00 2001 From: Jordan Date: Fri, 16 Jun 2023 04:37:21 -0400 Subject: [PATCH 02/12] Regression test for #1550 --- tests/DMAPI/LongRunning/Test.dm | 15 ++++ .../Live/Instance/WatchdogTest.cs | 86 ++++++++++++++----- 2 files changed, 81 insertions(+), 20 deletions(-) diff --git a/tests/DMAPI/LongRunning/Test.dm b/tests/DMAPI/LongRunning/Test.dm index b81b7f3f42..ef653c1efc 100644 --- a/tests/DMAPI/LongRunning/Test.dm +++ b/tests/DMAPI/LongRunning/Test.dm @@ -128,6 +128,12 @@ var/run_bridge_test var/list/channels = TgsChatChannelInfo() return "[length(channels)]" + var/legalize_nuclear_bombs = data["shadow_wizard_money_gang"] + if(legalize_nuclear_bombs) + text2file("I expect this to remain here for a while", "kajigger.txt") + kajigger_test = TRUE + return "we love casting spells" + TgsChatBroadcast(new /datum/tgs_message_content("Recieved non-tgs topic: `[T]`")) return "feck" @@ -147,10 +153,18 @@ var/run_bridge_test world.TgsChatBroadcast(new /datum/tgs_message_content("2/3 queued detached chat messages")) world.TgsChatBroadcast(new /datum/tgs_message_content("3/3 queued detached chat messages")) +var/kajigger_test = FALSE + /world/Reboot(reason) TgsChatBroadcast("World Rebooting") + + if(kajigger_test && !fexists("kajigger.txt")) + FailTest("TGS STOLE MY KAJIGGER (#1548 regression)") + TgsReboot() + ..() + /datum/tgs_event_handler/impl/HandleEvent(event_code, ...) set waitfor = FALSE @@ -175,6 +189,7 @@ var/run_bridge_test world.log << "Done sleep, calling Reboot" world.Reboot() + /datum/tgs_chat_command/embeds_test name = "embeds_test" help_text = "dumps an embed" diff --git a/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs b/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs index b9278aec6d..4e59572eac 100644 --- a/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs +++ b/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs @@ -102,6 +102,8 @@ namespace Tgstation.Server.Tests.Live.Instance { await StartAndLeaveRunning(cancellationToken); + await RegressionTest1550(cancellationToken); + var deleteJobTask = TestDeleteByondInstallErrorCasesAndQueing(cancellationToken); SessionController.LogTopicRequests = false; @@ -127,6 +129,54 @@ namespace Tgstation.Server.Tests.Live.Instance await WaitForJob(restartJob, 15, false, null, cancellationToken); } + async ValueTask RegressionTest1550(CancellationToken cancellationToken) + { + // we need to cycle deployments twice because TGS holds the initial deployment + var job = await DeployTestDme("LongRunning/long_running_test", DreamDaemonSecurity.Trusted, true, cancellationToken); + var currentStatus = await instanceClient.DreamDaemon.Read(cancellationToken); + + Assert.AreEqual(WatchdogStatus.Online, currentStatus.Status); + Assert.IsNotNull(currentStatus.StagedCompileJob); + var expectedStaged = currentStatus.StagedCompileJob; + Assert.AreNotEqual(expectedStaged.Id, currentStatus.ActiveCompileJob.Id); + + await TellWorldToReboot(cancellationToken); + + currentStatus = await instanceClient.DreamDaemon.Read(cancellationToken); + Assert.AreEqual(expectedStaged.Id, currentStatus.ActiveCompileJob.Id); + + await Task.Delay(TimeSpan.FromSeconds(3), cancellationToken); + + var topicRequestResult = await TopicClient.SendTopic( + IPAddress.Loopback, + $"shadow_wizard_money_gang=1", + TestLiveServer.DDPort, + cancellationToken); + + Assert.IsNotNull(topicRequestResult); + Assert.AreEqual("we love casting spells", topicRequestResult.StringData); + + await DeployTestDme("LongRunning/long_running_test", DreamDaemonSecurity.Trusted, true, cancellationToken); + + currentStatus = await instanceClient.DreamDaemon.Read(cancellationToken); + + Assert.AreEqual(WatchdogStatus.Online, currentStatus.Status); + Assert.IsNotNull(currentStatus.StagedCompileJob); + Assert.AreEqual(expectedStaged.Id, currentStatus.ActiveCompileJob.Id); + expectedStaged = currentStatus.StagedCompileJob; + Assert.AreNotEqual(expectedStaged.Id, currentStatus.ActiveCompileJob.Id); + + await TellWorldToReboot(cancellationToken); + + currentStatus = await instanceClient.DreamDaemon.Read(cancellationToken); + Assert.AreEqual(WatchdogStatus.Online, currentStatus.Status); + Assert.IsNull(currentStatus.StagedCompileJob); + Assert.AreEqual(expectedStaged.Id, currentStatus.ActiveCompileJob.Id); + + await CheckDMApiFail(currentStatus.ActiveCompileJob, cancellationToken, false); + await CheckDMApiFail(expectedStaged, cancellationToken, false); + } + async Task TestDeleteByondInstallErrorCasesAndQueing(CancellationToken cancellationToken) { var testCustomVersion = new Version(ByondTest.TestVersion.Major, ByondTest.TestVersion.Minor, 1); @@ -950,32 +1000,28 @@ namespace Tgstation.Server.Tests.Live.Instance public async Task TellWorldToReboot(CancellationToken cancellationToken) { var daemonStatus = await instanceClient.DreamDaemon.Read(cancellationToken); + Assert.IsNotNull(daemonStatus.StagedCompileJob); var initialCompileJob = daemonStatus.ActiveCompileJob; - try - { - System.Console.WriteLine("TEST: Sending world reboot topic..."); - var result = await TopicClient.SendTopic(IPAddress.Loopback, "tgs_integration_test_special_tactics=1", TestLiveServer.DDPort, cancellationToken); - Assert.AreEqual("ack", result.StringData); + System.Console.WriteLine("TEST: Sending world reboot topic..."); + var result = await TopicClient.SendTopic(IPAddress.Loopback, "tgs_integration_test_special_tactics=1", TestLiveServer.DDPort, cancellationToken); + Assert.AreEqual("ack", result.StringData); - using (var tempCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken)) - using (tempCts.Token.Register(() => System.Console.WriteLine("TEST ERROR: Timeout in TellWorldToReboot!"))) + using var tempCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + var tempToken = tempCts.Token; + using (tempToken.Register(() => System.Console.WriteLine("TEST ERROR: Timeout in TellWorldToReboot!"))) + { + tempCts.CancelAfter(TimeSpan.FromMinutes(2)); + + do { - tempCts.CancelAfter(TimeSpan.FromMinutes(2)); - var tempToken = tempCts.Token; - - do - { - await Task.Delay(TimeSpan.FromSeconds(1), tempToken); - daemonStatus = await instanceClient.DreamDaemon.Read(tempToken); - } - while (initialCompileJob.Id == daemonStatus.ActiveCompileJob.Id); + await Task.Delay(TimeSpan.FromSeconds(1), tempToken); + daemonStatus = await instanceClient.DreamDaemon.Read(tempToken); } + while (initialCompileJob.Id == daemonStatus.ActiveCompileJob.Id); } - catch (OperationCanceledException) - { - throw; - } + + await Task.Delay(TimeSpan.FromSeconds(3), cancellationToken); return daemonStatus; } From f542f2b9031339d2a5c767322510010d2fb6ece2 Mon Sep 17 00:00:00 2001 From: Jordan Date: Fri, 16 Jun 2023 04:37:28 -0400 Subject: [PATCH 03/12] Fixes #1550 --- .../Deployment/SwappableDmbProvider.cs | 13 +++++ .../Components/Watchdog/WindowsWatchdog.cs | 56 +++++++++++++------ 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/Tgstation.Server.Host/Components/Deployment/SwappableDmbProvider.cs b/src/Tgstation.Server.Host/Components/Deployment/SwappableDmbProvider.cs index a794135d13..b406267d0f 100644 --- a/src/Tgstation.Server.Host/Components/Deployment/SwappableDmbProvider.cs +++ b/src/Tgstation.Server.Host/Components/Deployment/SwappableDmbProvider.cs @@ -26,6 +26,11 @@ namespace Tgstation.Server.Host.Components.Deployment /// public CompileJob CompileJob => baseProvider.CompileJob; + /// + /// If has been run. + /// + public bool Swapped => swapped != 0; + /// /// The we are swapping for. /// @@ -41,6 +46,11 @@ namespace Tgstation.Server.Host.Components.Deployment /// readonly ISymlinkFactory symlinkFactory; + /// + /// Backing field for . + /// + volatile int swapped; + /// /// Initializes a new instance of the class. /// @@ -67,6 +77,9 @@ namespace Tgstation.Server.Host.Components.Deployment /// A representing the running operation. public async Task MakeActive(CancellationToken cancellationToken) { + if (Interlocked.Exchange(ref swapped, 1) != 0) + throw new InvalidOperationException("Already swapped!"); + // Note this comment from TGS3: // These next two lines should be atomic but this is the best we can do await ioManager.DeleteDirectory(LiveGameDirectory, cancellationToken); diff --git a/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs b/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs index e797127e3b..2f80de7f9c 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs @@ -125,6 +125,9 @@ namespace Tgstation.Server.Host.Components.Watchdog { var updateTask = BeforeApplyDmb(pendingSwappable.CompileJob, cancellationToken); Logger.LogTrace("Replacing activeSwappable with pendingSwappable..."); + if (!pendingSwappable.Swapped) + await PerformDmbSwap(pendingSwappable, cancellationToken); + Server.ReplaceDmbProvider(pendingSwappable); ActiveSwappable = pendingSwappable; pendingSwappable = null; @@ -173,23 +176,14 @@ namespace Tgstation.Server.Host.Components.Watchdog } SwappableDmbProvider windowsProvider = null; - bool suspended = false; try { windowsProvider = new SwappableDmbProvider(compileJobProvider, GameIOManager, symlinkFactory); - - Logger.LogDebug("Swapping to compile job {0}...", windowsProvider.CompileJob.Id); - try + if (ActiveCompileJob.DMApiVersion == null) { - Server.Suspend(); - suspended = true; + Logger.LogWarning("Active compile job has no DMAPI! Commencing immediate .dmb swap. Note this behavior is known to be buggy in some DM code contexts. See https://github.com/tgstation/tgstation-server/issues/1550"); + await PerformDmbSwap(windowsProvider, cancellationToken); } - catch (Exception ex) - { - Logger.LogWarning(ex, "Exception while suspending server!"); - } - - await windowsProvider.MakeActive(cancellationToken); } catch (Exception ex) { @@ -199,10 +193,6 @@ namespace Tgstation.Server.Host.Components.Watchdog throw; } - // Let this throw hard if it fails - if (suspended) - Server.Resume(); - pendingSwappable?.Dispose(); pendingSwappable = windowsProvider; } @@ -260,5 +250,39 @@ namespace Tgstation.Server.Host.Components.Watchdog Logger.LogTrace("Symlinking compile job..."); return ActiveSwappable.MakeActive(cancellationToken); } + + /// + /// Suspends the and calls on a . + /// + /// The to activate. + /// The for the operation. + /// A representing the running operation. + async ValueTask PerformDmbSwap(SwappableDmbProvider newProvider, CancellationToken cancellationToken) + { + Logger.LogDebug("Swapping to compile job {id}...", newProvider.CompileJob.Id); + + var suspended = false; + var server = Server; + try + { + server.Suspend(); + suspended = true; + } + catch (Exception ex) + { + Logger.LogWarning(ex, "Exception while suspending server!"); + } + + try + { + await newProvider.MakeActive(cancellationToken); + } + finally + { + // Let this throw hard if it fails + if (suspended) + server.Resume(); + } + } } } From 8a674ae3c12f32e4653d1f61e330a97ebad4b3a0 Mon Sep 17 00:00:00 2001 From: Jordan Date: Fri, 16 Jun 2023 12:21:12 -0400 Subject: [PATCH 04/12] Minor PosixWatchdog comment --- src/Tgstation.Server.Host/Components/Watchdog/PosixWatchdog.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tgstation.Server.Host/Components/Watchdog/PosixWatchdog.cs b/src/Tgstation.Server.Host/Components/Watchdog/PosixWatchdog.cs index 9eacfc2f17..69c5bcfb31 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/PosixWatchdog.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/PosixWatchdog.cs @@ -84,7 +84,7 @@ namespace Tgstation.Server.Host.Components.Watchdog } /// - protected override Task ApplyInitialDmb(CancellationToken cancellationToken) => Task.CompletedTask; + protected override Task ApplyInitialDmb(CancellationToken cancellationToken) => Task.CompletedTask; // not necessary to hold initial .dmb on Linux because of based inode deletes /// protected override async Task InitialLink(CancellationToken cancellationToken) From 975fb984186f807c2ae511c7cfb132e1b52fe6b7 Mon Sep 17 00:00:00 2001 From: Jordan Date: Fri, 16 Jun 2023 14:14:48 -0400 Subject: [PATCH 05/12] Fixes #1548 --- .../Components/Session/ISessionController.cs | 8 +- .../Components/Session/SessionController.cs | 46 ++++---- .../Components/Watchdog/BasicWatchdog.cs | 2 + .../Watchdog/MonitorActivationReason.cs | 6 + .../Components/Watchdog/WatchdogBase.cs | 7 +- .../Components/Watchdog/WindowsWatchdog.cs | 109 +++++++++++++++++- 6 files changed, 153 insertions(+), 25 deletions(-) diff --git a/src/Tgstation.Server.Host/Components/Session/ISessionController.cs b/src/Tgstation.Server.Host/Components/Session/ISessionController.cs index 21b45dc434..5e3e9de16b 100644 --- a/src/Tgstation.Server.Host/Components/Session/ISessionController.cs +++ b/src/Tgstation.Server.Host/Components/Session/ISessionController.cs @@ -54,6 +54,11 @@ namespace Tgstation.Server.Host.Components.Session /// RebootState RebootState { get; } + /// + /// A that completes when the server calls /world/TgsNew(). + /// + Task OnStartup { get; } + /// /// A that completes when the server calls /world/TgsReboot(). /// @@ -113,6 +118,7 @@ namespace Tgstation.Server.Host.Components.Session /// Replace the in use with a given , disposing the old one. /// /// The new . - void ReplaceDmbProvider(IDmbProvider newProvider); + /// An to be disposed once certain that the original is no longer in use. + IDisposable ReplaceDmbProvider(IDmbProvider newProvider); } } diff --git a/src/Tgstation.Server.Host/Components/Session/SessionController.cs b/src/Tgstation.Server.Host/Components/Session/SessionController.cs index b7978ff051..a1d7e8db9b 100644 --- a/src/Tgstation.Server.Host/Components/Session/SessionController.cs +++ b/src/Tgstation.Server.Host/Components/Session/SessionController.cs @@ -69,6 +69,9 @@ namespace Tgstation.Server.Host.Components.Session /// public Task Lifetime { get; } + /// + public Task OnStartup => startupTcs.Task; + /// public Task OnReboot => rebootTcs.Task; @@ -143,20 +146,30 @@ namespace Tgstation.Server.Host.Components.Session /// TaskCompletionSource portAssignmentTcs; + /// + /// The that completes when DD sends a valid startup bridge request. + /// + volatile TaskCompletionSource startupTcs; + + /// + /// The that completes when DD tells us about a reboot. + /// + volatile TaskCompletionSource rebootTcs; + + /// + /// The that completes when DD tells us it's primed. + /// + volatile TaskCompletionSource primeTcs; + /// /// The port to assign DreamDaemon when it queries for it. /// ushort? nextPort; /// - /// The that completes when DD tells us about a reboot. + /// The for the . /// - TaskCompletionSource rebootTcs; - - /// - /// The that completes when DD tells us it's primed. - /// - TaskCompletionSource primeTcs; + ApiValidationStatus apiValidationStatus; /// /// If we know DreamDaemon currently has it's port closed. @@ -168,11 +181,6 @@ namespace Tgstation.Server.Host.Components.Session /// bool disposed; - /// - /// The for the . - /// - ApiValidationStatus apiValidationStatus; - /// /// If should be kept alive instead. /// @@ -234,6 +242,7 @@ namespace Tgstation.Server.Host.Components.Session apiValidationStatus = ApiValidationStatus.NeverValidated; released = false; + startupTcs = new TaskCompletionSource(); rebootTcs = new TaskCompletionSource(); primeTcs = new TaskCompletionSource(); @@ -500,11 +509,11 @@ namespace Tgstation.Server.Host.Components.Session public void Resume() => process.Resume(); /// - public void ReplaceDmbProvider(IDmbProvider dmbProvider) + public IDisposable ReplaceDmbProvider(IDmbProvider dmbProvider) { var oldDmb = ReattachInformation.Dmb; ReattachInformation.Dmb = dmbProvider ?? throw new ArgumentNullException(nameof(dmbProvider)); - oldDmb.Dispose(); + return oldDmb; } /// @@ -639,9 +648,7 @@ namespace Tgstation.Server.Host.Components.Session parsedChannels); break; case BridgeCommandType.Prime: - var oldPrimeTcs = primeTcs; - primeTcs = new TaskCompletionSource(); - oldPrimeTcs.SetResult(); + Interlocked.Exchange(ref primeTcs, new TaskCompletionSource()).SetResult(); break; case BridgeCommandType.Kill: Logger.LogInformation("Bridge requested process termination!"); @@ -722,6 +729,7 @@ namespace Tgstation.Server.Host.Components.Session // Load custom commands chatTrackingContext.CustomCommands = parameters.CustomCommands; + Interlocked.Exchange(ref startupTcs, new TaskCompletionSource()).SetResult(); break; case BridgeCommandType.Reboot: if (ClosePortOnReboot) @@ -731,9 +739,7 @@ namespace Tgstation.Server.Host.Components.Session portClosedForReboot = true; } - var oldRebootTcs = rebootTcs; - rebootTcs = new TaskCompletionSource(); - oldRebootTcs.SetResult(); + Interlocked.Exchange(ref rebootTcs, new TaskCompletionSource()).SetResult(); break; case BridgeCommandType.Chunk: return await ProcessChunk(ProcessBridgeCommand, BridgeError, parameters.Chunk, cancellationToken); diff --git a/src/Tgstation.Server.Host/Components/Watchdog/BasicWatchdog.cs b/src/Tgstation.Server.Host/Components/Watchdog/BasicWatchdog.cs index f7bc152132..f42b708489 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/BasicWatchdog.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/BasicWatchdog.cs @@ -170,6 +170,8 @@ namespace Tgstation.Server.Host.Components.Watchdog case MonitorActivationReason.ActiveServerPrimed: await HandleEvent(EventType.WorldPrime, Enumerable.Empty(), false, cancellationToken); break; + case MonitorActivationReason.ActiveServerStartup: + break; // unused in BasicWatchdog case MonitorActivationReason.Heartbeat: default: throw new InvalidOperationException($"Invalid activation reason: {reason}"); diff --git a/src/Tgstation.Server.Host/Components/Watchdog/MonitorActivationReason.cs b/src/Tgstation.Server.Host/Components/Watchdog/MonitorActivationReason.cs index eaa4b4e441..e3f2a5d48a 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/MonitorActivationReason.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/MonitorActivationReason.cs @@ -34,5 +34,11 @@ /// Server primed. /// ActiveServerPrimed, + + /// + /// Server started. + /// + /// The monitor misses the first startup of a session. + ActiveServerStartup, } } diff --git a/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs b/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs index 08ba1c8e2d..126f36628e 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs @@ -796,6 +796,7 @@ namespace Tgstation.Server.Host.Components.Watchdog MonitorAction nextAction = MonitorAction.Continue; Task activeServerLifetime = null, activeServerReboot = null, + activeServerStartup = null, serverPrimed = null, activeLaunchParametersChanged = null, newDmbAvailable = null; @@ -825,12 +826,14 @@ namespace Tgstation.Server.Host.Components.Watchdog TryUpdateTask(ref activeServerLifetime, () => controller.Lifetime); TryUpdateTask(ref activeServerReboot, () => controller.OnReboot); TryUpdateTask(ref serverPrimed, () => controller.OnPrime); + TryUpdateTask(ref activeServerStartup, () => controller.OnStartup); } else { activeServerLifetime = controller.Lifetime; activeServerReboot = controller.OnReboot; serverPrimed = controller.OnPrime; + activeServerStartup = controller.OnStartup; lastController = controller; } @@ -862,6 +865,7 @@ namespace Tgstation.Server.Host.Components.Watchdog var toWaitOn = Task.WhenAny( activeServerLifetime, activeServerReboot, + activeServerStartup, heartbeat, newDmbAvailable, cancelTcs.Task, @@ -908,7 +912,8 @@ namespace Tgstation.Server.Host.Components.Watchdog || CheckActivationReason(ref newDmbAvailable, MonitorActivationReason.NewDmbAvailable) || CheckActivationReason(ref activeLaunchParametersChanged, MonitorActivationReason.ActiveLaunchParametersUpdated) || CheckActivationReason(ref heartbeat, MonitorActivationReason.Heartbeat) - || CheckActivationReason(ref serverPrimed, MonitorActivationReason.ActiveServerPrimed); + || CheckActivationReason(ref serverPrimed, MonitorActivationReason.ActiveServerPrimed) + || CheckActivationReason(ref activeServerStartup, MonitorActivationReason.ActiveServerStartup); UpdateMonitoredTasks(); diff --git a/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs b/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs index 2f80de7f9c..04175fde30 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs @@ -1,4 +1,6 @@ using System; +using System.Collections.Generic; +using System.Diagnostics; using System.Threading; using System.Threading.Tasks; @@ -37,11 +39,21 @@ namespace Tgstation.Server.Host.Components.Watchdog /// readonly ISymlinkFactory symlinkFactory; + /// + /// of s that are waiting to clean up old deployments. + /// + readonly List deploymentCleanupTasks; + /// /// The active for . /// SwappableDmbProvider pendingSwappable; + /// + /// The representing the cleanup of an unused . + /// + volatile TaskCompletionSource deploymentCleanupGate; + /// /// Initializes a new instance of the class. /// @@ -98,11 +110,16 @@ namespace Tgstation.Server.Host.Components.Watchdog { GameIOManager = gameIOManager ?? throw new ArgumentNullException(nameof(gameIOManager)); this.symlinkFactory = symlinkFactory ?? throw new ArgumentNullException(nameof(symlinkFactory)); + + deploymentCleanupTasks = new List(); } catch { // Async dispose is for if we have controllers running, not the case here - DisposeAsync().AsTask().GetAwaiter().GetResult(); + var disposeTask = DisposeAsync(); + Debug.Assert(disposeTask.IsCompleted, "This should always be true during construction!"); + disposeTask.GetAwaiter().GetResult(); + throw; } } @@ -116,6 +133,8 @@ namespace Tgstation.Server.Host.Components.Watchdog ActiveSwappable = null; pendingSwappable?.Dispose(); pendingSwappable = null; + + await DrainDeploymentCleanupTasks(true); } /// @@ -124,11 +143,55 @@ namespace Tgstation.Server.Host.Components.Watchdog if (pendingSwappable != null) { var updateTask = BeforeApplyDmb(pendingSwappable.CompileJob, cancellationToken); - Logger.LogTrace("Replacing activeSwappable with pendingSwappable..."); + if (!pendingSwappable.Swapped) await PerformDmbSwap(pendingSwappable, cancellationToken); - Server.ReplaceDmbProvider(pendingSwappable); + var currentCompileJobId = Server.ReattachInformation.Dmb.CompileJob.Id; + + await DrainDeploymentCleanupTasks(false); + + IDisposable lingeringDeployment; + var localDeploymentCleanupGate = new TaskCompletionSource(); + async Task CleanupLingeringDeployment() + { + var lingeringDeploymentExpirySeconds = ActiveLaunchParameters.StartupTimeout.Value; + Logger.LogDebug( + "Holding old deployment {compileJobId} for up to {expiry} seconds...", + currentCompileJobId, + lingeringDeploymentExpirySeconds); + + var timeout = AsyncDelayer.Delay(TimeSpan.FromSeconds(lingeringDeploymentExpirySeconds), cancellationToken); + + var completedTask = await Task.WhenAny( + localDeploymentCleanupGate.Task, + timeout); + + var timedOut = completedTask == timeout; + Logger.Log( + timedOut + ? LogLevel.Warning + : LogLevel.Trace, + "Releasing old deployment {compileJobId}{afterTimeout}", + timedOut + ? " due to timeout!" + : "..."); + + lingeringDeployment.Dispose(); + } + + var oldDeploymentCleanupGate = Interlocked.Exchange(ref deploymentCleanupGate, localDeploymentCleanupGate); + oldDeploymentCleanupGate?.TrySetResult(); + + Logger.LogTrace("Replacing activeSwappable with pendingSwappable..."); + + lock (deploymentCleanupTasks) + { + lingeringDeployment = Server.ReplaceDmbProvider(pendingSwappable); + deploymentCleanupTasks.Add( + CleanupLingeringDeployment()); + } + ActiveSwappable = pendingSwappable; pendingSwappable = null; @@ -240,6 +303,16 @@ namespace Tgstation.Server.Host.Components.Watchdog await base.SessionStartupPersist(cancellationToken); } + /// + protected override async Task HandleMonitorWakeup(MonitorActivationReason reason, CancellationToken cancellationToken) + { + var result = await base.HandleMonitorWakeup(reason, cancellationToken); + if (reason == MonitorActivationReason.ActiveServerStartup) + await DrainDeploymentCleanupTasks(false); + + return result; + } + /// /// Create the initial link to the live game directory using . /// @@ -284,5 +357,35 @@ namespace Tgstation.Server.Host.Components.Watchdog server.Resume(); } } + + /// + /// Asynchronously drain . + /// + /// If , all s will be ed. Otherwise, only s with set will be ed. + /// A representing the running operation. + Task DrainDeploymentCleanupTasks(bool blocking) + { + Logger.LogTrace("DrainDeploymentCleanupTasks..."); + var localDeploymentCleanupGate = Interlocked.Exchange(ref deploymentCleanupGate, null); + localDeploymentCleanupGate?.TrySetResult(); + + List localDeploymentCleanupTasks; + lock (deploymentCleanupTasks) + { + var totalActiveTasks = deploymentCleanupTasks.Count; + localDeploymentCleanupTasks = new List(totalActiveTasks); + for (var i = totalActiveTasks - 1; i >= 0; --i) + { + var currentTask = deploymentCleanupTasks[i]; + if (!blocking && !currentTask.IsCompleted) + continue; + + localDeploymentCleanupTasks.Add(currentTask); + deploymentCleanupTasks.RemoveAt(i); + } + } + + return Task.WhenAll(localDeploymentCleanupTasks); + } } } From efd0936a6710adba1d28dafb5e27cd448f941214 Mon Sep 17 00:00:00 2001 From: Jordan Date: Fri, 16 Jun 2023 14:29:40 -0400 Subject: [PATCH 06/12] Always log DreamDaemon output under test --- .../Session/SessionControllerFactory.cs | 41 ++++++++++++------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/src/Tgstation.Server.Host/Components/Session/SessionControllerFactory.cs b/src/Tgstation.Server.Host/Components/Session/SessionControllerFactory.cs index 586af39cd4..f9921dc324 100644 --- a/src/Tgstation.Server.Host/Components/Session/SessionControllerFactory.cs +++ b/src/Tgstation.Server.Host/Components/Session/SessionControllerFactory.cs @@ -286,6 +286,7 @@ namespace Tgstation.Server.Host.Components.Session await CheckPagerIsNotRunning(cancellationToken); string outputFilePath = null; + var preserveLogFile = true; if (launchParameters.LogOutput.Value) { var now = DateTimeOffset.UtcNow; @@ -299,7 +300,10 @@ namespace Tgstation.Server.Host.Components.Session logger.LogInformation("Logging DreamDaemon output to {path}...", outputFilePath); } else if (!byondLock.SupportsCli) + { outputFilePath = gameIOManager.ConcatPath(dmbProvider.Directory, $"{Guid.NewGuid()}.dd.log"); + preserveLogFile = false; + } var accessIdentifier = cryptographySuite.GetSecureString(); @@ -352,9 +356,12 @@ namespace Tgstation.Server.Host.Components.Session assemblyInformationProvider, asyncDelayer, loggerFactory.CreateLogger(), - () => !launchParameters.LogOutput.Value - ? LogDDOutput(process, outputFilePath, byondLock.SupportsCli, default) // DCT: None available - : Task.CompletedTask, + () => LogDDOutput( + process, + outputFilePath, + byondLock.SupportsCli, + preserveLogFile, + default), // DCT: None available launchParameters.StartupTimeout, false, apiValidate); @@ -562,18 +569,20 @@ namespace Tgstation.Server.Host.Components.Session /// Attempts to log DreamDaemon output. /// /// The DreamDaemon . - /// The path to the DreamDaemon log file. Will be deleted. + /// The path to the DreamDaemon log file. Will be deleted if is . /// If DreamDaemon was launched with CLI capabilities. + /// If , will be deleted. /// The for the operation. /// A representing the running operation. - async Task LogDDOutput(IProcess process, string outputFilePath, bool cliSupported, CancellationToken cancellationToken) + async Task LogDDOutput(IProcess process, string outputFilePath, bool cliSupported, bool preserveFile, CancellationToken cancellationToken) { try { - string ddOutput; + string ddOutput = null; if (cliSupported) ddOutput = await process.GetCombinedOutput(cancellationToken); - else + + if (ddOutput == null) try { var dreamDaemonLogBytes = await gameIOManager.ReadAllBytes( @@ -584,14 +593,16 @@ namespace Tgstation.Server.Host.Components.Session } finally { - try - { - await gameIOManager.DeleteFile(outputFilePath, cancellationToken); - } - catch (Exception ex) - { - logger.LogWarning(ex, "Failed to delete DreamDaemon log file {outputFilePath}!", outputFilePath); - } + if (!preserveFile) + try + { + logger.LogTrace("Deleting temporary log file {path}...", outputFilePath); + await gameIOManager.DeleteFile(outputFilePath, cancellationToken); + } + catch (Exception ex) + { + logger.LogWarning(ex, "Failed to delete DreamDaemon log file {outputFilePath}!", outputFilePath); + } } logger.LogTrace( From f1f0a350d6cfd5f127ecffe688b94ff2f5bba55a Mon Sep 17 00:00:00 2001 From: Jordan Date: Fri, 16 Jun 2023 17:35:08 -0400 Subject: [PATCH 07/12] PosixWatchdog doesn't need hard links - Added ISymlinkFactory.SymlinkedDirectoriesAreFiles because they are files on Linux. - Removed crazy-ass hard link junk from PosixWatchdog. - Added symlink unit tests. --- .../Deployment/SwappableDmbProvider.cs | 8 +- .../Components/Watchdog/PosixWatchdog.cs | 81 +------------- .../Components/Watchdog/WindowsWatchdog.cs | 2 +- .../IO/ISymlinkFactory.cs | 6 + .../IO/PosixSymlinkFactory.cs | 3 + .../IO/WindowsSymlinkFactory.cs | 3 + .../System/TestSymlinkFactory.cs | 105 ++++++++++++++++++ 7 files changed, 127 insertions(+), 81 deletions(-) create mode 100644 tests/Tgstation.Server.Host.Tests/System/TestSymlinkFactory.cs diff --git a/src/Tgstation.Server.Host/Components/Deployment/SwappableDmbProvider.cs b/src/Tgstation.Server.Host/Components/Deployment/SwappableDmbProvider.cs index b406267d0f..e496453222 100644 --- a/src/Tgstation.Server.Host/Components/Deployment/SwappableDmbProvider.cs +++ b/src/Tgstation.Server.Host/Components/Deployment/SwappableDmbProvider.cs @@ -80,9 +80,11 @@ namespace Tgstation.Server.Host.Components.Deployment if (Interlocked.Exchange(ref swapped, 1) != 0) throw new InvalidOperationException("Already swapped!"); - // Note this comment from TGS3: - // These next two lines should be atomic but this is the best we can do - await ioManager.DeleteDirectory(LiveGameDirectory, cancellationToken); + if (symlinkFactory.SymlinkedDirectoriesAreDeletedAsFiles) + await ioManager.DeleteFile(LiveGameDirectory, cancellationToken); + else + await ioManager.DeleteDirectory(LiveGameDirectory, cancellationToken); + await symlinkFactory.CreateSymbolicLink( ioManager.ResolvePath(baseProvider.Directory), ioManager.ResolvePath(LiveGameDirectory), diff --git a/src/Tgstation.Server.Host/Components/Watchdog/PosixWatchdog.cs b/src/Tgstation.Server.Host/Components/Watchdog/PosixWatchdog.cs index 69c5bcfb31..044ef3f1ef 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/PosixWatchdog.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/PosixWatchdog.cs @@ -1,5 +1,4 @@ -using System; -using System.Threading; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -22,11 +21,6 @@ namespace Tgstation.Server.Host.Components.Watchdog /// sealed class PosixWatchdog : WindowsWatchdog { - /// - /// If the swappable game directory is currently a rename of the compile job. - /// - IDmbProvider hardLinkedDmb; - /// /// Initializes a new instance of the class. /// @@ -84,77 +78,10 @@ namespace Tgstation.Server.Host.Components.Watchdog } /// - protected override Task ApplyInitialDmb(CancellationToken cancellationToken) => Task.CompletedTask; // not necessary to hold initial .dmb on Linux because of based inode deletes - - /// - protected override async Task InitialLink(CancellationToken cancellationToken) + protected override Task ApplyInitialDmb(CancellationToken cancellationToken) { - // The logic to check for an active live directory is in SwappableDmbProvider, so we just do it again here for safety - Logger.LogTrace("Hard linking compile job..."); - - // Symlinks are counted as a file on linux?? - if (await GameIOManager.DirectoryExists(ActiveSwappable.Directory, cancellationToken)) - await GameIOManager.DeleteDirectory(ActiveSwappable.Directory, cancellationToken); - else - await GameIOManager.DeleteFile(ActiveSwappable.Directory, cancellationToken); - - // Instead of symlinking to begin with we actually rename the directory - await GameIOManager.MoveDirectory( - ActiveSwappable.CompileJob.DirectoryName.ToString(), - ActiveSwappable.Directory, - cancellationToken); - - hardLinkedDmb = ActiveSwappable; - } - - /// - protected override async Task InitController(Task chatTask, ReattachInformation reattachInfo, CancellationToken cancellationToken) - { - var suspended = false; - try - { - await base.InitController(chatTask, reattachInfo, cancellationToken); - } - finally - { - // Then we move it back and apply the symlink - if (hardLinkedDmb != null) - { - try - { - Logger.LogTrace("Unhardlinking compile job..."); - Server?.Suspend(); - suspended = true; - var hardLink = hardLinkedDmb.Directory; - var originalPosition = hardLinkedDmb.CompileJob.DirectoryName.ToString(); - await GameIOManager.MoveDirectory( - hardLink, - originalPosition, - default); - } - catch (Exception ex) - { - Logger.LogError( - ex, - "Failed to un-hard link compile job #{compileJobId} ({compileJobDirectory})", - hardLinkedDmb.CompileJob.Id, - hardLinkedDmb.CompileJob.DirectoryName); - } - - hardLinkedDmb = null; - } - } - - if (reattachInfo != null) - { - Logger.LogTrace("Skipping symlink due to reattach"); - return; - } - - Logger.LogTrace("Symlinking compile job..."); - await ActiveSwappable.MakeActive(cancellationToken); - if (suspended) - Server.Resume(); + // not necessary to hold initial .dmb on Linux because of based inode deletes + return Task.CompletedTask; } } } diff --git a/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs b/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs index 04175fde30..3a37cf69ec 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs @@ -318,7 +318,7 @@ namespace Tgstation.Server.Host.Components.Watchdog /// /// The for the operation. /// A representing the running operation. - protected virtual Task InitialLink(CancellationToken cancellationToken) + Task InitialLink(CancellationToken cancellationToken) { Logger.LogTrace("Symlinking compile job..."); return ActiveSwappable.MakeActive(cancellationToken); diff --git a/src/Tgstation.Server.Host/IO/ISymlinkFactory.cs b/src/Tgstation.Server.Host/IO/ISymlinkFactory.cs index c762395fca..1adbd99d97 100644 --- a/src/Tgstation.Server.Host/IO/ISymlinkFactory.cs +++ b/src/Tgstation.Server.Host/IO/ISymlinkFactory.cs @@ -8,6 +8,12 @@ namespace Tgstation.Server.Host.IO /// interface ISymlinkFactory { + /// + /// If directory symlinks must be deleted as files would in the current environment. + /// + /// This is because Linux symlinked directories must be deleted with . + bool SymlinkedDirectoriesAreDeletedAsFiles { get; } + /// /// Create a symbolic link. /// diff --git a/src/Tgstation.Server.Host/IO/PosixSymlinkFactory.cs b/src/Tgstation.Server.Host/IO/PosixSymlinkFactory.cs index 4644080a10..c3c1abcc32 100644 --- a/src/Tgstation.Server.Host/IO/PosixSymlinkFactory.cs +++ b/src/Tgstation.Server.Host/IO/PosixSymlinkFactory.cs @@ -12,6 +12,9 @@ namespace Tgstation.Server.Host.IO /// sealed class PosixSymlinkFactory : ISymlinkFactory { + /// + public bool SymlinkedDirectoriesAreDeletedAsFiles => true; + /// public Task CreateSymbolicLink(string targetPath, string linkPath, CancellationToken cancellationToken) => Task.Factory.StartNew( () => diff --git a/src/Tgstation.Server.Host/IO/WindowsSymlinkFactory.cs b/src/Tgstation.Server.Host/IO/WindowsSymlinkFactory.cs index 2beddb0581..b2ced648e0 100644 --- a/src/Tgstation.Server.Host/IO/WindowsSymlinkFactory.cs +++ b/src/Tgstation.Server.Host/IO/WindowsSymlinkFactory.cs @@ -13,6 +13,9 @@ namespace Tgstation.Server.Host.IO /// sealed class WindowsSymlinkFactory : ISymlinkFactory { + /// + public bool SymlinkedDirectoriesAreDeletedAsFiles => false; + /// public Task CreateSymbolicLink(string targetPath, string linkPath, CancellationToken cancellationToken) => Task.Factory.StartNew( () => diff --git a/tests/Tgstation.Server.Host.Tests/System/TestSymlinkFactory.cs b/tests/Tgstation.Server.Host.Tests/System/TestSymlinkFactory.cs new file mode 100644 index 0000000000..8e488f11b0 --- /dev/null +++ b/tests/Tgstation.Server.Host.Tests/System/TestSymlinkFactory.cs @@ -0,0 +1,105 @@ +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Tgstation.Server.Host.IO; + +namespace Tgstation.Server.Host.System.Tests +{ + [TestClass] + public sealed class TestSymlinkFactory + { + readonly ISymlinkFactory factory = new PlatformIdentifier().IsWindows + ? new WindowsSymlinkFactory() + : new PosixSymlinkFactory(); + + [TestMethod] + public async Task TestSymlinks() + { + var cancellationToken = CancellationToken.None; + var cwd = Path.GetTempFileName(); + File.Delete(cwd); + Directory.CreateDirectory(cwd); + try + { + var realDir = Path.Combine(cwd, "RealDir"); + var symDir = Path.Combine(cwd, "SymDir"); + var realFile = Path.Combine(cwd, "RealFile.txt"); + var symFile = Path.Combine(realDir, "RealFile.txt"); + + var subRealFile = Path.Combine(realDir, "test.txt"); + var subSymFile = Path.Combine(symDir, "test.txt"); + + try + { + await factory.CreateSymbolicLink(subRealFile, subSymFile, cancellationToken); + Assert.Fail("Expected Exception!"); + } + catch + { + } + + Directory.CreateDirectory(realDir); + Directory.CreateDirectory(symDir); + + await File.WriteAllBytesAsync(realFile, Array.Empty(), cancellationToken); + await File.WriteAllBytesAsync(symFile, Array.Empty(), cancellationToken); + + try + { + await factory.CreateSymbolicLink(realFile, symFile, cancellationToken); + Assert.Fail("Expected Exception!"); + } + catch + { + } + + Directory.Delete(symDir); + File.Delete(symFile); + + await factory.CreateSymbolicLink(realFile, symFile, cancellationToken); + Assert.IsTrue(File.Exists(symFile)); + Assert.IsFalse(Directory.Exists(symFile)); + + await File.WriteAllTextAsync(realFile, "test", cancellationToken); + var symFileContents = await File.ReadAllTextAsync(symFile, cancellationToken); + Assert.AreEqual("test", symFileContents); + File.Delete(symFile); + File.Delete(realFile); + + try + { + await factory.CreateSymbolicLink(realDir, symDir, cancellationToken); + + Assert.IsFalse(File.Exists(symDir)); + Assert.IsTrue(Directory.Exists(symDir)); + + await File.WriteAllTextAsync(subRealFile, "test", cancellationToken); + Assert.IsTrue(File.Exists(subSymFile)); + + File.Delete(subSymFile); + Assert.IsFalse(File.Exists(subRealFile)); + } + finally + { + if (factory.SymlinkedDirectoriesAreDeletedAsFiles) + File.Delete(symDir); + else + Directory.Delete(symDir); + } + + if (factory.SymlinkedDirectoriesAreDeletedAsFiles) + Assert.IsFalse(File.Exists(symDir)); + else + Assert.IsFalse(Directory.Exists(symDir)); + } + finally + { + Directory.Delete(cwd, true); + } + } + } +} From b0558808c75094489ffd3cbf0ac25241f139c815 Mon Sep 17 00:00:00 2001 From: Jordan Date: Fri, 16 Jun 2023 17:35:31 -0400 Subject: [PATCH 08/12] Add missing `.csproj`s to Dockerfile --- build/Dockerfile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/build/Dockerfile b/build/Dockerfile index c5baa2f3ff..759ba84cb1 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -43,6 +43,8 @@ COPY tgstation-server.sln ./ COPY src/Tgstation.Server.Host.Console/Tgstation.Server.Host.Console.csproj src/Tgstation.Server.Host.Console/ COPY src/Tgstation.Server.Host.Watchdog/Tgstation.Server.Host.Watchdog.csproj src/Tgstation.Server.Host.Watchdog/ COPY src/Tgstation.Server.Api/Tgstation.Server.Api.csproj src/Tgstation.Server.Api/ +COPY src/Tgstation.Server.Common/Tgstation.Server.Common.csproj src/Tgstation.Server.Common/ +COPY src/Tgstation.Server.Host.Common/Tgstation.Server.Host.Common.csproj src/Tgstation.Server.Host.Common/ RUN dotnet restore -nowarn:MSB3202,nu1503 -p:RestoreUseSkipNonexistentTargets=false From f9167033adb9e4c8a114577bea24b9230e0445ec Mon Sep 17 00:00:00 2001 From: Jordan Date: Fri, 16 Jun 2023 14:42:41 -0400 Subject: [PATCH 09/12] Version bump to 5.12.6 --- build/Version.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build/Version.props b/build/Version.props index f4ebd0f5a2..f3666b8777 100644 --- a/build/Version.props +++ b/build/Version.props @@ -3,7 +3,7 @@ - 5.12.5 + 5.12.6 4.6.0 9.10.2 10.4.1 From 57e5409fde004b01aea378b8212b52b7255e6760 Mon Sep 17 00:00:00 2001 From: Jordan Date: Sat, 17 Jun 2023 12:34:26 -0400 Subject: [PATCH 10/12] Fix UnixIOException usage --- src/Tgstation.Server.Host/System/PosixProcessFeatures.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Tgstation.Server.Host/System/PosixProcessFeatures.cs b/src/Tgstation.Server.Host/System/PosixProcessFeatures.cs index 184ce2b55e..5f55d3f1e1 100644 --- a/src/Tgstation.Server.Host/System/PosixProcessFeatures.cs +++ b/src/Tgstation.Server.Host/System/PosixProcessFeatures.cs @@ -48,7 +48,7 @@ namespace Tgstation.Server.Host.System { var result = Syscall.kill(process.Id, Signum.SIGCONT); if (result != 0) - throw new UnixIOException(result); + throw new UnixIOException(Stdlib.GetLastError()); } /// @@ -56,7 +56,7 @@ namespace Tgstation.Server.Host.System { var result = Syscall.kill(process.Id, Signum.SIGSTOP); if (result != 0) - throw new UnixIOException(result); + throw new UnixIOException(Stdlib.GetLastError()); } /// From aedd35fce69dcea60282d4eea2323f363af65753 Mon Sep 17 00:00:00 2001 From: Jordan Date: Sat, 17 Jun 2023 12:34:54 -0400 Subject: [PATCH 11/12] Inconclusive rather than success in POSIX test --- tests/Tgstation.Server.Host.Tests/IO/TestPostWriteHandler.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/Tgstation.Server.Host.Tests/IO/TestPostWriteHandler.cs b/tests/Tgstation.Server.Host.Tests/IO/TestPostWriteHandler.cs index 8c6e1cbaef..71d3c3fb0b 100644 --- a/tests/Tgstation.Server.Host.Tests/IO/TestPostWriteHandler.cs +++ b/tests/Tgstation.Server.Host.Tests/IO/TestPostWriteHandler.cs @@ -70,7 +70,8 @@ namespace Tgstation.Server.Host.IO.Tests public void TestThrowsOnUnix() { if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - return; + Assert.Inconclusive("POSIX only test."); + var postWriteHandler = new PosixPostWriteHandler(Mock.Of>()); var tmpFile = Path.GetTempFileName(); File.Delete(tmpFile); From 2e93a3f98f63109e9da0fcf47386443f9b17ba88 Mon Sep 17 00:00:00 2001 From: Jordan Date: Sat, 17 Jun 2023 14:06:16 -0400 Subject: [PATCH 12/12] Fix WindowsWatchdog swapping race condition --- .../Components/Session/ISessionController.cs | 5 +++ .../Components/Session/SessionController.cs | 27 ++++++++++++--- .../Components/Watchdog/BasicWatchdog.cs | 34 +++++++++++-------- .../Components/Watchdog/WindowsWatchdog.cs | 13 +++++++ .../Live/Instance/WatchdogTest.cs | 2 +- 5 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/Tgstation.Server.Host/Components/Session/ISessionController.cs b/src/Tgstation.Server.Host/Components/Session/ISessionController.cs index 5e3e9de16b..d9f9fb9f26 100644 --- a/src/Tgstation.Server.Host/Components/Session/ISessionController.cs +++ b/src/Tgstation.Server.Host/Components/Session/ISessionController.cs @@ -49,6 +49,11 @@ namespace Tgstation.Server.Host.Components.Session /// bool ClosePortOnReboot { get; set; } + /// + /// If the is currently processing a bridge request from TgsReboot(). + /// + bool ProcessingRebootBridgeRequest { get; } + /// /// The current . /// diff --git a/src/Tgstation.Server.Host/Components/Session/SessionController.cs b/src/Tgstation.Server.Host/Components/Session/SessionController.cs index a1d7e8db9b..8b120bec72 100644 --- a/src/Tgstation.Server.Host/Components/Session/SessionController.cs +++ b/src/Tgstation.Server.Host/Components/Session/SessionController.cs @@ -81,6 +81,9 @@ namespace Tgstation.Server.Host.Components.Session /// public bool DMApiAvailable => ReattachInformation.Dmb.CompileJob.DMApiVersion?.Major == DMApiConstants.InteropVersion.Major; + /// + public bool ProcessingRebootBridgeRequest => rebootBridgeRequestsProcessing > 0; + /// /// The up to date . /// @@ -161,6 +164,11 @@ namespace Tgstation.Server.Host.Components.Session /// volatile TaskCompletionSource primeTcs; + /// + /// The number of currently active calls to from TgsReboot(). + /// + volatile uint rebootBridgeRequestsProcessing; + /// /// The port to assign DreamDaemon when it queries for it. /// @@ -732,14 +740,23 @@ namespace Tgstation.Server.Host.Components.Session Interlocked.Exchange(ref startupTcs, new TaskCompletionSource()).SetResult(); break; case BridgeCommandType.Reboot: - if (ClosePortOnReboot) + Interlocked.Increment(ref rebootBridgeRequestsProcessing); + try { - chatTrackingContext.Active = false; - response.NewPort = 0; - portClosedForReboot = true; + if (ClosePortOnReboot) + { + chatTrackingContext.Active = false; + response.NewPort = 0; + portClosedForReboot = true; + } + + Interlocked.Exchange(ref rebootTcs, new TaskCompletionSource()).SetResult(); + } + finally + { + Interlocked.Decrement(ref rebootBridgeRequestsProcessing); } - Interlocked.Exchange(ref rebootTcs, new TaskCompletionSource()).SetResult(); break; case BridgeCommandType.Chunk: return await ProcessChunk(ProcessBridgeCommand, BridgeError, parameters.Chunk, cancellationToken); diff --git a/src/Tgstation.Server.Host/Components/Watchdog/BasicWatchdog.cs b/src/Tgstation.Server.Host/Components/Watchdog/BasicWatchdog.cs index f42b708489..e61eb734f6 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/BasicWatchdog.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/BasicWatchdog.cs @@ -143,21 +143,27 @@ namespace Tgstation.Server.Host.Components.Watchdog gracefulRebootRequired = false; Server.ResetRebootState(); - await HandleEvent(EventType.WorldReboot, Enumerable.Empty(), false, cancellationToken); - - switch (rebootState) + var eventTask = HandleEvent(EventType.WorldReboot, Enumerable.Empty(), false, cancellationToken); + try { - case Session.RebootState.Normal: - return await HandleNormalReboot(cancellationToken); - case Session.RebootState.Restart: - return MonitorAction.Restart; - case Session.RebootState.Shutdown: - // graceful shutdown time - Chat.QueueWatchdogMessage( - "Active server rebooted! Shutting down due to graceful termination request..."); - return MonitorAction.Exit; - default: - throw new InvalidOperationException($"Invalid reboot state: {rebootState}"); + switch (rebootState) + { + case Session.RebootState.Normal: + return await HandleNormalReboot(cancellationToken); + case Session.RebootState.Restart: + return MonitorAction.Restart; + case Session.RebootState.Shutdown: + // graceful shutdown time + Chat.QueueWatchdogMessage( + "Active server rebooted! Shutting down due to graceful termination request..."); + return MonitorAction.Exit; + default: + throw new InvalidOperationException($"Invalid reboot state: {rebootState}"); + } + } + finally + { + await eventTask; } case MonitorActivationReason.ActiveLaunchParametersUpdated: diff --git a/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs b/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs index 3a37cf69ec..fcf1dac080 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/WindowsWatchdog.cs @@ -145,7 +145,20 @@ namespace Tgstation.Server.Host.Components.Watchdog var updateTask = BeforeApplyDmb(pendingSwappable.CompileJob, cancellationToken); if (!pendingSwappable.Swapped) + { + // IMPORTANT: THE SESSIONCONTROLLER SHOULD STILL BE PROCESSING THE BRIDGE REQUEST SO WE KNOW DD IS SLEEPING + // OTHERWISE, IT COULD RETURN TO /world/Reboot() TOO EARLY AND LOAD THE WRONG .DMB + if (!Server.ProcessingRebootBridgeRequest) + { + // integration test logging will catch this + Logger.LogError( + "The reboot bridge request completed before the watchdog could suspend the server! This can lead to buggy DreamDaemon behaviour and should be reported! To ensure stability, we will need to hard reboot the server"); + await updateTask; + return MonitorAction.Restart; + } + await PerformDmbSwap(pendingSwappable, cancellationToken); + } var currentCompileJobId = Server.ReattachInformation.Dmb.CompileJob.Id; diff --git a/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs b/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs index 4e59572eac..2392ebb480 100644 --- a/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs +++ b/tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs @@ -132,7 +132,7 @@ namespace Tgstation.Server.Tests.Live.Instance async ValueTask RegressionTest1550(CancellationToken cancellationToken) { // we need to cycle deployments twice because TGS holds the initial deployment - var job = await DeployTestDme("LongRunning/long_running_test", DreamDaemonSecurity.Trusted, true, cancellationToken); + await DeployTestDme("LongRunning/long_running_test", DreamDaemonSecurity.Trusted, true, cancellationToken); var currentStatus = await instanceClient.DreamDaemon.Read(cancellationToken); Assert.AreEqual(WatchdogStatus.Online, currentStatus.Status);