From 863be239c00ef8c107b3ccaebc3b942a5d2d5980 Mon Sep 17 00:00:00 2001 From: AdaInTheLab Date: Thu, 28 May 2026 22:32:06 -0400 Subject: [PATCH] fix(restart): skip systemctl probe on Windows; route through OS-aware strategy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/server/restart always tried `sudo -n systemctl restart 7daystodie.service` first and waited up to 5 seconds for it to either succeed or fail before falling back to in-game shutdown. On Linux this is correct (systemd Restart=always bounces the service). On Windows it is wrong twice over: - `systemctl` does not exist on Windows, so the probe always fails fast and wastes ~5s plus logs a misleading "systemctl restart failed or not available..." warning on every restart click. - On a live Windows prod box today this entire code path was the trigger for a cascading crash: the fallback in-game shutdown ran concurrent with a stuck main-thread operation and snowballed into a stack overflow. Beyond cosmetic, it was risk-shaped. Fix: introduce Core/OsRestartStrategy.cs as a small pure helper. `OsRestartStrategy.Decide(bool isWindows)` returns one of two kinds: - SystemctlThenInGameShutdown (Linux): unchanged behavior — try systemctl, fall back to in-game shutdown for systemd Restart=always. - InGameShutdownOnly (Windows): skip the systemctl probe entirely, go straight to in-game shutdown. NSSM (the standard Windows service supervisor for 7DTD) ships with `AppExit Restart` as its default and auto-bounces the service when the game process exits, so this is the correct supervised-restart path on Windows. Windows path logs INFO ("Windows install detected; using in-game shutdown + service-supervisor restart") instead of WARNING — the operator should see we *chose* this path, not that we fell back into it. `OsRestartStrategy.DecideForCurrentHost()` reads the host platform via the existing `PlatformHelper.IsWindows` (Environment.OSVersion.Platform, works under both .NET Framework and Mono). The pure `Decide(bool)` overload keeps the decision testable without touching the real OS check — covered by two NUnit tests in KitsuneCommand.Tests/Core/OsRestartStrategyTests.cs. `krestart` console command was checked and does not need the same treatment — it goes through GracefulRestartFeature.TriggerNow, which is pure in-game shutdown and never shells out. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 18 +++++++ .../Core/OsRestartStrategyTests.cs | 33 ++++++++++++ src/KitsuneCommand/Core/OsRestartStrategy.cs | 52 ++++++++++++++++++ .../Web/Controllers/ServerController.cs | 53 ++++++++++++++----- 4 files changed, 143 insertions(+), 13 deletions(-) create mode 100644 src/KitsuneCommand.Tests/Core/OsRestartStrategyTests.cs create mode 100644 src/KitsuneCommand/Core/OsRestartStrategy.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index b321a8a..1581864 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,24 @@ pulls notes from — it's the minimum, the GitHub release page is the maximum. events don't loop back into the logger), and the failure path on that path writes to `Console.Error` instead of `Log.Warning` so the recursion can't restart. +- **Restart Server no longer probes `systemctl` on Windows.** The + `POST /api/server/restart` endpoint always tried + `sudo -n systemctl restart 7daystodie.service` first and waited up to + 5 seconds before falling back to in-game shutdown. On Windows there + is no `systemctl`, so every restart click wasted ~5s and logged a + misleading `systemctl restart failed or not available...` warning. + On a live Windows prod box this code path was the trigger for a + cascading crash — the fallback in-game shutdown ran concurrent with + a stuck main-thread operation and snowballed into a stack overflow. + Fix: new `Core/OsRestartStrategy.cs` picks the path per OS. Windows + goes straight to in-game shutdown and relies on the service + supervisor (NSSM `AppExit Restart`, which is its default) to bounce + 7DTD on game exit; an INFO-level log line announces the chosen path + instead of the warning. Linux behavior is unchanged — systemctl + first, fall back to in-game shutdown for `Restart=always`. Console + command `krestart` already uses the OS-agnostic + `GracefulRestartFeature.TriggerNow` (no shell-out) so it didn't need + to change. ## [2.8.1] - 2026-05-29 diff --git a/src/KitsuneCommand.Tests/Core/OsRestartStrategyTests.cs b/src/KitsuneCommand.Tests/Core/OsRestartStrategyTests.cs new file mode 100644 index 0000000..20c8aeb --- /dev/null +++ b/src/KitsuneCommand.Tests/Core/OsRestartStrategyTests.cs @@ -0,0 +1,33 @@ +using NUnit.Framework; +using KitsuneCommand.Core; + +namespace KitsuneCommand.Tests.Core +{ + /// + /// Covers the pure decision function. The platform-detection probe itself + /// (PlatformHelper.IsWindows / DecideForCurrentHost) isn't unit-tested + /// here because it just reads Environment.OSVersion.Platform — the + /// useful test surface is the strategy mapping, which is pure. + /// + [TestFixture] + public class OsRestartStrategyTests + { + [Test] + public void Decide_OnWindows_SkipsSystemctlProbe() + { + var strategy = OsRestartStrategy.Decide(isWindows: true); + + Assert.That(strategy, Is.EqualTo(OsRestartStrategy.Kind.InGameShutdownOnly), + "Windows must skip systemctl — NSSM handles the bounce on game exit."); + } + + [Test] + public void Decide_OnLinux_TriesSystemctlFirst() + { + var strategy = OsRestartStrategy.Decide(isWindows: false); + + Assert.That(strategy, Is.EqualTo(OsRestartStrategy.Kind.SystemctlThenInGameShutdown), + "Linux must keep trying systemctl first; in-game shutdown is the fallback."); + } + } +} diff --git a/src/KitsuneCommand/Core/OsRestartStrategy.cs b/src/KitsuneCommand/Core/OsRestartStrategy.cs new file mode 100644 index 0000000..091a2cf --- /dev/null +++ b/src/KitsuneCommand/Core/OsRestartStrategy.cs @@ -0,0 +1,52 @@ +namespace KitsuneCommand.Core +{ + /// + /// Picks how the Restart Server endpoint should bounce the 7DTD service for + /// the current host OS. Two paths exist: + /// + /// - Linux: try sudo -n systemctl restart 7daystodie.service first + /// (works if scripts/linux-updater/install-linux-updater.sh has + /// been run to add the sudoers entry), then fall back to in-game + /// shutdown and rely on systemd Restart=always. + /// - Windows: there is no systemctl. NSSM (the standard service + /// supervisor for 7DTD on Windows) ships with AppExit Restart + /// as its default, so an in-game shutdown is sufficient — NSSM + /// auto-bounces the service when the game process exits. Probing + /// systemctl on Windows wastes ~5s on every restart click and + /// was the trigger for a cascading crash on a live prod box (the + /// fallback shutdown ran concurrent with a stuck main-thread op and + /// snowballed into a stack overflow). + /// + /// Kept as a pure function over isWindows so tests don't need a + /// real OS check or any DI plumbing. + /// + public static class OsRestartStrategy + { + public enum Kind + { + /// Try systemctl first, then fall back to in-game shutdown. + SystemctlThenInGameShutdown, + + /// Skip the systemctl probe entirely; rely on the service + /// supervisor (NSSM AppExit Restart) to bounce the game + /// after the in-game shutdown exits the process. + InGameShutdownOnly, + } + + /// + /// Pick a strategy based on whether the host is Windows. + /// + public static Kind Decide(bool isWindows) + { + return isWindows ? Kind.InGameShutdownOnly : Kind.SystemctlThenInGameShutdown; + } + + /// + /// Convenience: decide based on the running host (uses ). + /// + public static Kind DecideForCurrentHost() + { + return Decide(PlatformHelper.IsWindows); + } + } +} diff --git a/src/KitsuneCommand/Web/Controllers/ServerController.cs b/src/KitsuneCommand/Web/Controllers/ServerController.cs index 6f35b18..efa9cd9 100644 --- a/src/KitsuneCommand/Web/Controllers/ServerController.cs +++ b/src/KitsuneCommand/Web/Controllers/ServerController.cs @@ -218,13 +218,21 @@ public IHttpActionResult Shutdown([FromBody] ShutdownRequest request) } /// - /// Restart the server. Two-step: - /// 1. Try `sudo -n systemctl restart 7daystodie.service` (non-interactive). - /// Works if install-linux-updater.sh has been run (adds the sudoers entry). - /// 2. If systemctl fails, fall back to in-game shutdown with a short delay. - /// This relies on systemd having `Restart=always` configured to bounce it. + /// Restart the server. OS-aware: + /// - Linux: try `sudo -n systemctl restart 7daystodie.service` first + /// (works if install-linux-updater.sh has been run to add the + /// sudoers entry). If that fails, fall back to in-game shutdown + /// and rely on systemd `Restart=always` to bounce the service. + /// - Windows: skip the systemctl probe entirely — `systemctl` + /// doesn't exist on Windows, so probing it just wastes ~5s and + /// produces a misleading "systemctl failed" warning. Go straight + /// to in-game shutdown; NSSM (the standard Windows service + /// supervisor for 7DTD) ships with `AppExit Restart` as its + /// default and auto-bounces the service when the game exits. /// - /// If neither path works, the server stays down - tell the admin to run the installer. + /// The strategy decision is delegated to + /// so it stays testable and shareable with any other restart entry + /// point (e.g. krestart) we add later. /// [HttpPost] [Route("restart")] @@ -236,15 +244,29 @@ public IHttpActionResult Restart([FromBody] RestartRequest request) if (!System.Text.RegularExpressions.Regex.IsMatch(serviceName, @"^[a-zA-Z0-9._-]+$")) return BadRequest("Invalid service name."); - // Try systemctl first (Linux path). - if (TryStart("sudo", $"-n systemctl restart {serviceName}", out var stderr, 5000)) + var strategy = OsRestartStrategy.DecideForCurrentHost(); + + if (strategy == OsRestartStrategy.Kind.SystemctlThenInGameShutdown) { - return Ok(ApiResponse.Ok($"Restart triggered via systemctl ({serviceName}). Server bouncing.")); - } + // Try systemctl first (Linux path). + if (TryStart("sudo", $"-n systemctl restart {serviceName}", out var stderr, 5000)) + { + return Ok(ApiResponse.Ok($"Restart triggered via systemctl ({serviceName}). Server bouncing.")); + } - global::Log.Warning($"[KitsuneCommand] systemctl restart failed or not available ({stderr}). Falling back to in-game shutdown."); + global::Log.Warning($"[KitsuneCommand] systemctl restart failed or not available ({stderr}). Falling back to in-game shutdown."); + } + else + { + // Windows: NSSM (AppExit Restart) handles the bounce after the + // game process exits. INFO log so the operator sees we *chose* + // this path rather than failed into it. + global::Log.Out("[KitsuneCommand] Windows install detected; using in-game shutdown + service-supervisor restart (NSSM AppExit Restart)."); + } - // Fallback: in-game shutdown with short delay, rely on systemd Restart=always. + // Common path: in-game shutdown with short delay. On Linux this is + // the fallback after systemctl failed; on Windows it's the only + // path we take. ModEntry.MainThreadContext.Post(_ => { try @@ -253,10 +275,15 @@ public IHttpActionResult Restart([FromBody] RestartRequest request) } catch (Exception ex) { - global::Log.Error($"[KitsuneCommand] Fallback shutdown command failed: {ex.Message}"); + global::Log.Error($"[KitsuneCommand] In-game shutdown command failed: {ex.Message}"); } }, null); + if (strategy == OsRestartStrategy.Kind.InGameShutdownOnly) + { + return Ok(ApiResponse.Ok("Restart requested via in-game shutdown (5s delay). The Windows service supervisor (NSSM) will auto-bounce 7DTD when the game exits, provided AppExit Restart is configured (the NSSM default).")); + } + return Ok(ApiResponse.Ok("Restart requested via in-game shutdown (5s delay). If systemd Restart=always is not set, server will stay down - run scripts/linux-updater/install-linux-updater.sh to configure.")); }