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) <noreply@anthropic.com>

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")).

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;

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
     }
 
     /// <summary>
-    /// 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.
     /// </summary>
     private static void Disable(IConnectionFilter filter, Exception e)
     {

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")).

Projects/UOContent.Tests/Tests/Network/Bans/CrowdSecReporterTests.cs

@@ -161,24 +161,11 @@ public class CrowdSecReporterTests
     }
 
     /// <summary>
-    /// 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&lt;ValueTask&gt; 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.
     /// </summary>
-    /// <remarks>
-    /// <see cref="CrowdSecReporter.Start"/> hands the drain loop to <see cref="Task.Run(Func{Task})"/>,
-    /// and there is no <c>Task.Run(Func&lt;ValueTask&gt;)</c> overload — a ValueTask-returning drain loop
-    /// binds to <c>Task.Run&lt;TResult&gt;(Func&lt;TResult&gt;)</c> and yields a <c>Task&lt;ValueTask&gt;</c>
-    /// that completes at the first suspending await rather than when the loop exits, silently upcast by
-    /// the <see cref="Task"/> field. That turned <see cref="CrowdSecReporter.Stop"/>'s drain-exited
-    /// handshake into a no-op and let the shutdown flush read the <c>SingleReader</c> channel while the
-    /// drain loop was still reading it. Every other test here reaches Stop() without a Start(), so
-    /// nothing covered it.
-    /// <para>
-    /// Racing the drain against a delay is what separates the two shapes: with an empty queue the loop
-    /// parks on <c>WaitToReadAsync</c> forever, so a correctly unwrapped task cannot win that race, while
-    /// the <c>Task&lt;ValueTask&gt;</c> completed in ~0ms. A slow pool can only make this under-detect,
-    /// never fail spuriously.
-    /// </para>
-    /// </remarks>
     [Fact]
     public async Task Start_DrainTaskSpansLoopLifetime_NotJustTheFirstAwait()
     {

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) }
             ]
         };
 

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);

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
         {

Projects/UOContent/Misc/CrowdSec/CrowdSecReporter.cs

@@ -69,10 +69,7 @@ public sealed class CrowdSecReporter : IBanReporter
     /// </summary>
     public int SendFailureCount => _sendFailures;
 
-    /// <summary>
-    /// The drain loop's task, so tests can observe its lifetime. It must stay incomplete until the loop
-    /// actually exits — see the note on <see cref="DrainLoop"/> for the shape that silently broke that.
-    /// </summary>
+    /// <summary>The drain loop's task, so tests can assert it stays alive until the loop exits.</summary>
     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
     }
 
     /// <summary>
-    /// 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: <see cref="Stop"/> runs where
+    /// <c>SynchronizationContext.Current</c> is the <c>EventLoopContext</c>, and a captured continuation
+    /// would be posted to a queue nothing pumps any more. <see cref="Task.Run(Func{Task})"/> keeps the
+    /// chain on the pool; the bounded wait caps a wedged send at a few seconds of shutdown.
     /// </summary>
-    /// <remarks>
-    /// 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 <em>on the loop thread</em>.
-    /// <see cref="Stop"/> runs on the main thread, where <see cref="SynchronizationContext.Current"/> is
-    /// the <c>EventLoopContext</c>; 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
-    /// <see cref="Task.Run(Func{Task})"/> 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
-    /// <c>ConfigureAwait(false)</c>. The bounded <see cref="Task.Wait(TimeSpan)"/> is the backstop behind
-    /// the flush's own budget: a wedged send costs a few seconds of shutdown, never the process.
-    /// </remarks>
     private void FlushRemainingOnStop()
     {
         if (_queue == null || _client == null)
@@ -187,12 +170,10 @@ public sealed class CrowdSecReporter : IBanReporter
     }
 
     /// <summary>
-    /// 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
-    /// <see cref="BuildAlerts"/> 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 <see cref="CrowdSecSettings.ManualBanDuration"/> 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
+    /// <see cref="CrowdSecSettings.ManualBanDuration"/>.
     /// </summary>
     private async Task FlushRemainingOnStopAsync(List<ReportItem> reports, List<ReportItem> 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<ValueTask>) overload, so a ValueTask-returning lambda binds to Task.Run<TResult> and
-    // yields a Task<ValueTask> 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<ValueTask> overload, so a
+    // ValueTask would bind to Task.Run<TResult> and yield a Task<ValueTask> that completes at the first
+    // await rather than when the loop exits.
     private async Task DrainLoop(CancellationToken token)
     {
         var reader = _queue.Reader;

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));

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<FirewallEntryRecord>(_entries.Count);
 

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