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) { }