From 99fc33ad47bd55a0d8da670fdab407decd50b9fc Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sat, 25 Jul 2026 11:30:52 -0700 Subject: [PATCH] fix(firewall): use Core.Now on the game thread; seed the test clock Firewall.LoadFrom and ToSettings run on the game loop (Configure sweep, the maintenance timer, Save on shutdown) but read DateTime.UtcNow. ToSettings was the one that mattered: it derives each persisted expiry as now + (expiresAtTick - nowTicks) while nowTicks came from Core.TickCount, so pairing a fresh wall clock with the loop's tick baked the loop's lag into every saved TTL. Core.Now and Core.TickCount are refreshed together at the top of each iteration, so taking both keeps the operands on one instant. Left DateTime.UtcNow in CrowdSecAlertClient and the reporter's flush/drain paths, which run on the pool and have no loop clock to read. Neither test fixture seeded Core._now, so Core.Now was DateTime.MinValue for the whole test host -- MinValue.AddHours(-1) throws, and any code correctly reading the game-thread clock computed nonsense. Seed it as Main.cs does. Also fixes a dangling collection reference: the firewall tests moved into UOContent.Tests still declared [Collection("Sequential Server Tests")], which is only defined in Server.Tests. xUnit matched no fixture and silently skipped the bootstrap for those tests. Comment pass over the branch: drop development narration and tighten what stays. Co-Authored-By: Claude Opus 5 (1M context) --- .../Fixtures/TestServerInitializer.cs | 5 ++ .../Server/Network/Bans/BanConfiguration.cs | 3 +- Projects/Server/Network/ConnectionFilters.cs | 10 ++-- .../Fixtures/TestServerInitializer.cs | 6 ++- .../Network/Bans/CrowdSecReporterTests.cs | 21 ++------ .../Firewall/FirewallPersistenceTests.cs | 5 +- .../Tests/Network/Firewall/FirewallTests.cs | 2 +- .../Misc/Blocklist/BlocklistFilter.cs | 11 ++-- .../Misc/CrowdSec/CrowdSecReporter.cs | 53 ++++++------------- .../Misc/Firewall/CidrFirewallEntry.cs | 2 +- Projects/UOContent/Misc/Firewall/Firewall.cs | 8 ++- .../Misc/Firewall/FirewallConnectionFilter.cs | 6 +-- 12 files changed, 53 insertions(+), 79 deletions(-) diff --git a/Projects/Server.Tests/Fixtures/TestServerInitializer.cs b/Projects/Server.Tests/Fixtures/TestServerInitializer.cs index 3902478a2..845efb115 100644 --- a/Projects/Server.Tests/Fixtures/TestServerInitializer.cs +++ b/Projects/Server.Tests/Fixtures/TestServerInitializer.cs @@ -1,3 +1,4 @@ +using System; using System.IO; using System.Reflection; using System.Threading; @@ -78,6 +79,10 @@ internal static class TestServerInitializer Core.LoopContext = new EventLoopContext(); Core.Expansion = Expansion.EJ; + // Seed the loop clock as Main.cs does before the Configure sweep; otherwise Core.Now is + // DateTime.MinValue for the whole test host. + Core._now = DateTime.UtcNow; + // Timer wheel must exist before NetState.Configure(), which schedules a recurring // sweep via Timer.DelayCall (matches production ordering in Main.cs: Timer.Init runs // before AssemblyHandler.Invoke("Configure")). diff --git a/Projects/Server/Network/Bans/BanConfiguration.cs b/Projects/Server/Network/Bans/BanConfiguration.cs index b1160f54e..2a2af40cf 100644 --- a/Projects/Server/Network/Bans/BanConfiguration.cs +++ b/Projects/Server/Network/Bans/BanConfiguration.cs @@ -33,8 +33,7 @@ public static class BanConfiguration public static void Configure() { - // Reached by the Configure sweep, which is the only caller. Idempotent anyway, so a second call - // cannot re-deserialize or re-write the template over an operator's edits. + // Idempotent: a second call must not re-deserialize or overwrite an operator's edits. if (Settings != null) { return; diff --git a/Projects/Server/Network/ConnectionFilters.cs b/Projects/Server/Network/ConnectionFilters.cs index 6d3113d63..c0bb0b2fb 100644 --- a/Projects/Server/Network/ConnectionFilters.cs +++ b/Projects/Server/Network/ConnectionFilters.cs @@ -75,8 +75,7 @@ public static class ConnectionFilters var filters = _filters; for (var i = 0; i < filters.Length; i++) { - // Try/catch costs nothing when nothing throws, and this is the one path where a faulty - // third-party filter would otherwise take down the accept loop for every connection. + // A faulty filter must not take down the accept loop for every connection. try { if (filters[i].ShouldDeny(address)) @@ -96,10 +95,9 @@ public static class ConnectionFilters } /// - /// Drops a filter that threw on the accept path. A filter that faults once will fault for every - /// subsequent connection, so leaving it registered means an exception and a log line per accept — - /// exactly the amplification an attacker wants. Failing open here is deliberate: a broken filter - /// must not be able to deny every connection either. + /// Drops a filter that threw on the accept path: one that faults once faults for every subsequent + /// connection, costing an exception and a log line per accept. Failing open is deliberate — a broken + /// filter must not be able to deny every connection either. /// private static void Disable(IConnectionFilter filter, Exception e) { diff --git a/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs index dad2411ce..640c05837 100644 --- a/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs +++ b/Projects/UOContent.Tests/Fixtures/TestServerInitializer.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.IO; using System.Reflection; using System.Threading; @@ -62,6 +62,10 @@ internal static class TestServerInitializer SkillsInfo.Configure(); + // Seed the loop clock as Main.cs does before the Configure sweep; otherwise Core.Now is + // DateTime.MinValue for the whole test host. + Core._now = DateTime.UtcNow; + // Timer wheel must exist before NetState.Configure(), which schedules a recurring // sweep via Timer.DelayCall (matches production ordering in Main.cs: Timer.Init runs // before AssemblyHandler.Invoke("Configure")). diff --git a/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecReporterTests.cs b/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecReporterTests.cs index c6b83706f..a8daf3c44 100644 --- a/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecReporterTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecReporterTests.cs @@ -161,24 +161,11 @@ public class CrowdSecReporterTests } /// - /// Regression for the two shapes that made the drain task lie about the loop's lifetime. + /// The drain task must track the loop's lifetime, not just its first await — a ValueTask-returning + /// drain loop passed to Task.Run yields a Task<ValueTask> that completes immediately, which makes + /// Stop()'s drain-exited handshake a no-op. With an empty queue the loop parks on WaitToReadAsync, + /// so a correctly unwrapped task cannot win this race; a slow pool only under-detects. /// - /// - /// hands the drain loop to , - /// and there is no Task.Run(Func<ValueTask>) overload — a ValueTask-returning drain loop - /// binds to Task.Run<TResult>(Func<TResult>) and yields a Task<ValueTask> - /// that completes at the first suspending await rather than when the loop exits, silently upcast by - /// the field. That turned 's drain-exited - /// handshake into a no-op and let the shutdown flush read the SingleReader channel while the - /// drain loop was still reading it. Every other test here reaches Stop() without a Start(), so - /// nothing covered it. - /// - /// Racing the drain against a delay is what separates the two shapes: with an empty queue the loop - /// parks on WaitToReadAsync forever, so a correctly unwrapped task cannot win that race, while - /// the Task<ValueTask> completed in ~0ms. A slow pool can only make this under-detect, - /// never fail spuriously. - /// - /// [Fact] public async Task Start_DrainTaskSpansLoopLifetime_NotJustTheFirstAwait() { diff --git a/Projects/UOContent.Tests/Tests/Network/Firewall/FirewallPersistenceTests.cs b/Projects/UOContent.Tests/Tests/Network/Firewall/FirewallPersistenceTests.cs index f925cd5b4..01691e223 100644 --- a/Projects/UOContent.Tests/Tests/Network/Firewall/FirewallPersistenceTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/Firewall/FirewallPersistenceTests.cs @@ -20,7 +20,7 @@ using Xunit; namespace Server.Tests.Network.Firewall; -[Collection("Sequential Server Tests")] +[Collection("Sequential UOContent Tests")] public class FirewallPersistenceTests { [Fact] @@ -47,7 +47,8 @@ public class FirewallPersistenceTests { Entries = [ - new FirewallEntryRecord { Value = "9.9.9.9", Expires = DateTime.UtcNow.AddHours(-1) } + // Core.Now, matching the clock LoadFrom compares against. + new FirewallEntryRecord { Value = "9.9.9.9", Expires = Core.Now.AddHours(-1) } ] }; diff --git a/Projects/UOContent.Tests/Tests/Network/Firewall/FirewallTests.cs b/Projects/UOContent.Tests/Tests/Network/Firewall/FirewallTests.cs index ca4c245d0..fabf6d4ba 100644 --- a/Projects/UOContent.Tests/Tests/Network/Firewall/FirewallTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/Firewall/FirewallTests.cs @@ -20,7 +20,7 @@ using Xunit; namespace Server.Tests.Network.Firewall; -[Collection("Sequential Server Tests")] +[Collection("Sequential UOContent Tests")] public class FirewallTests { private static IPAddress Ip(string s) => IPAddress.Parse(s); diff --git a/Projects/UOContent/Misc/Blocklist/BlocklistFilter.cs b/Projects/UOContent/Misc/Blocklist/BlocklistFilter.cs index 2be16b08b..aeca58477 100644 --- a/Projects/UOContent/Misc/Blocklist/BlocklistFilter.cs +++ b/Projects/UOContent/Misc/Blocklist/BlocklistFilter.cs @@ -99,8 +99,8 @@ public sealed class BlocklistFilter : IConnectionFilter _cts = CancellationTokenSource.CreateLinkedTokenSource(token); - // A missing file is the shipped default, not an error: the gate stays inert and the poll picks the - // list up whenever the generator first writes it. No restart needed. + // A missing file is the shipped default, not an error: the gate stays inert until the poll picks + // up whatever the generator first writes. No restart needed. if (File.Exists(_path)) { Reload(); // synchronous prime; empty on failure (fail-open) @@ -233,10 +233,9 @@ public sealed class BlocklistFilter : IConnectionFilter private void Reload() { - // Capture the mtime/header BEFORE Load() so the markers describe the version we're about to - // parse, not whatever the producer may have atomically swapped in mid-parse. If a swap happens - // mid-parse, the markers describe the old-or-equal version, so the next poll detects the change - // and reloads again -- this errs toward reloading and never skips a version. + // Capture the mtime/header BEFORE Load() so the markers describe the version being parsed, not + // one the producer swapped in mid-parse. Stale markers only cost an extra reload next poll; + // capturing after could skip a version entirely. var writeUtc = default(DateTime); try { diff --git a/Projects/UOContent/Misc/CrowdSec/CrowdSecReporter.cs b/Projects/UOContent/Misc/CrowdSec/CrowdSecReporter.cs index e09713241..306d4ab53 100644 --- a/Projects/UOContent/Misc/CrowdSec/CrowdSecReporter.cs +++ b/Projects/UOContent/Misc/CrowdSec/CrowdSecReporter.cs @@ -69,10 +69,7 @@ public sealed class CrowdSecReporter : IBanReporter /// public int SendFailureCount => _sendFailures; - /// - /// The drain loop's task, so tests can observe its lifetime. It must stay incomplete until the loop - /// actually exits — see the note on for the shape that silently broke that. - /// + /// The drain loop's task, so tests can assert it stays alive until the loop exits. internal Task DrainTaskForTesting => _drainTask; public static void Configure() @@ -104,28 +101,21 @@ public sealed class CrowdSecReporter : IBanReporter { _cts?.Cancel(); - // Main.HandleClosed cancels ClosingTokenSource BEFORE calling BanChannel.Stop(), so by the time - // we get here the drain loop has almost always already observed cancellation and is unwinding. - // Wait a short bounded grace period for it to actually EXIT before we touch the SingleReader channel - // ourselves — flushing while the drain is still a live reader would be unsafe. + // The flush below reads a SingleReader channel, so wait for the drain to actually exit first. var drainExited = true; try { - // Task.Wait(timeout) returns false only on timeout (task still running); true when completed; - // throws when the task faulted/cancelled (also completed). So drainExited is false only while - // the drain is genuinely still alive. + // Wait(timeout) is false only on timeout; a throw means faulted/cancelled, which is still exited. drainExited = _drainTask == null || _drainTask.Wait(TimeSpan.FromSeconds(2)); } catch { - // A faulted/cancelled wait means the drain task has completed — it is no longer reading the - // channel, so the flush below is safe. + // Ignored: a faulted wait means the drain has completed and released the channel. } _cts?.Dispose(); _cts = null; - // Only read the SingleReader channel once the drain has provably stopped reading it. if (drainExited) { FlushRemainingOnStop(); @@ -136,19 +126,12 @@ public sealed class CrowdSecReporter : IBanReporter } /// - /// Best-effort bounded flush of whatever contribution items are still queued at shutdown. + /// Best-effort bounded flush of whatever is still queued at shutdown. Blocking is correct here — the + /// loop has stopped ticking — but must not happen on the loop thread: runs where + /// SynchronizationContext.Current is the EventLoopContext, and a captured continuation + /// would be posted to a queue nothing pumps any more. keeps the + /// chain on the pool; the bounded wait caps a wedged send at a few seconds of shutdown. /// - /// - /// Shutdown is the one place where blocking is the right answer — the loop has stopped ticking, so - /// there is no later tick to resume on — but it must not block on the loop thread. - /// runs on the main thread, where is - /// the EventLoopContext; an await that captured it would post its continuation to a queue - /// nothing pumps any more, and we would wait here forever. Running the flush through - /// puts the whole chain on the pool, where there is no context to - /// capture, so correctness does not depend on every await in the client remembering - /// ConfigureAwait(false). The bounded is the backstop behind - /// the flush's own budget: a wedged send costs a few seconds of shutdown, never the process. - /// private void FlushRemainingOnStop() { if (_queue == null || _client == null) @@ -187,12 +170,10 @@ public sealed class CrowdSecReporter : IBanReporter } /// - /// Runs on a fresh, short-lived token — NOT the drain loop's (already-cancelled) token — since a - /// cancelled token would make the send fail immediately. Pending reports are deduped via - /// and sent as a single batch; pending retracts are issued as individual - /// (deduped) DELETEs so an admin's explicit unban still propagates on a clean shutdown instead of - /// lingering until elapses. One shared short budget - /// bounds the whole flush, and any leftover retracts still self-heal via that duration. + /// Uses a fresh token, not the drain loop's already-cancelled one, which would fail every send + /// immediately. Reports go as one deduped batch; retracts go as individual DELETEs so an admin's + /// unban propagates on a clean shutdown. Leftovers self-heal via + /// . /// private async Task FlushRemainingOnStopAsync(List reports, List retracts) { @@ -263,11 +244,9 @@ public sealed class CrowdSecReporter : IBanReporter SingleReader = true }); - // Returns Task, not ValueTask, precisely because Start() hands it to Task.Run: there is no - // Task.Run(Func) overload, so a ValueTask-returning lambda binds to Task.Run and - // yields a Task that completes at the FIRST await instead of when the loop exits. That - // would make Stop()'s drain-exited handshake a no-op and let the flush race this loop on a - // SingleReader channel. A loop awaited once has nothing to gain from ValueTask anyway. + // Must return Task: Start() passes this to Task.Run, which has no Func overload, so a + // ValueTask would bind to Task.Run and yield a Task that completes at the first + // await rather than when the loop exits. private async Task DrainLoop(CancellationToken token) { var reader = _queue.Reader; diff --git a/Projects/UOContent/Misc/Firewall/CidrFirewallEntry.cs b/Projects/UOContent/Misc/Firewall/CidrFirewallEntry.cs index 2016853df..06527ab71 100644 --- a/Projects/UOContent/Misc/Firewall/CidrFirewallEntry.cs +++ b/Projects/UOContent/Misc/Firewall/CidrFirewallEntry.cs @@ -26,7 +26,7 @@ public class CidrFirewallEntry : BaseFirewallEntry public CidrFirewallEntry(string ipAddressOrCidr) { - // Core owns the CIDR -> normalized range parse (IPAddressUtility); this used to be a private copy. + // Core owns the CIDR -> normalized range parse. if (!IPAddressUtility.TryParseCidrRange(ipAddressOrCidr, out var min, out var max)) { throw new ArgumentException("Invalid IP address or CIDR.", nameof(ipAddressOrCidr)); diff --git a/Projects/UOContent/Misc/Firewall/Firewall.cs b/Projects/UOContent/Misc/Firewall/Firewall.cs index 9636dedcf..075b47ec3 100644 --- a/Projects/UOContent/Misc/Firewall/Firewall.cs +++ b/Projects/UOContent/Misc/Firewall/Firewall.cs @@ -279,7 +279,8 @@ public static class Firewall return; } - var now = DateTime.UtcNow; + // Core.Now: this runs on the game loop, via the Configure sweep. + var now = Core.Now; var records = settings.Entries; for (var i = 0; i < records.Length; i++) { @@ -307,7 +308,10 @@ public static class Firewall internal static FirewallSettings ToSettings() { - var now = DateTime.UtcNow; + // expires is derived below as now + (expiresAtTick - nowTicks), so both operands must come from + // the same instant. Core.Now and Core.TickCount are refreshed together each loop iteration; a + // fresh DateTime.UtcNow here would bake the loop's lag into every persisted expiry. + var now = Core.Now; var nowTicks = Core.TickCount; var list = new List(_entries.Count); diff --git a/Projects/UOContent/Misc/Firewall/FirewallConnectionFilter.cs b/Projects/UOContent/Misc/Firewall/FirewallConnectionFilter.cs index 7695eba48..570e33f19 100644 --- a/Projects/UOContent/Misc/Firewall/FirewallConnectionFilter.cs +++ b/Projects/UOContent/Misc/Firewall/FirewallConnectionFilter.cs @@ -35,14 +35,12 @@ internal sealed class FirewallConnectionFilter : IConnectionFilter public string Name => "firewall"; - // Firewall.Configure() owns loading/persistence and does the registering, so there is nothing to do - // here; Register() calling this back is harmless. + // Firewall.Configure() owns loading and registration. public void Register() { } - // Nothing to hydrate in the background: the set is loaded synchronously at Configure and maintained - // by a main-loop timer. + // Nothing to hydrate: the set loads at Configure and is maintained by a main-loop timer. public void Start(CancellationToken token) { }