From 8e5b7af1cf650faefd2e34aa08847f97d2fa7683 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:13:31 -0700 Subject: [PATCH] perf: retire auto-denylist holds from an expiry ring instead of scanning Reclaiming lapsed holds was O(entries held). Every cap-triggered reclaim during a flood, and every periodic sweep, walked the whole dictionary to find the few that had expired. That is what forced the cap to be sized for the scan rather than for the flood. A hold is now never refreshed: the first detection sets the expiry and later ones leave it alone. That makes insertion order equal to expiry order, so a ring of the same keys is sorted by construction and retiring lapsed entries stops at the first live record -- the cost is the number expiring, not the number held. Nothing is lost by dropping the refresh: the rate limiter runs ahead of the connection filters (NetState.Network.cs) and reports to the ban channel, so a flooder whose hold lapses is re-held on its next attempt. Because the ring carries the expiry, the dictionary only had to answer membership, so it is a HashSet now: 36 bytes a slot against 52. The ring is parallel UInt128[]/long[] rather than an array of structs -- UInt128 forces 16-byte alignment, so a packed pair would cost 32 bytes where these cost 24, and the drain reads only the long[]. Measured, standalone copies of both designs head to head at the shipped 324,449 cap: accept path, nothing held 9.1ns -> 6.1ns per call sustained flood at cap 26.7ms -> 9.3ms over 60k rejected addresses flood end, realistic spread 9.49ms -> 0.05ms worst single call flood end, all at one instant 8.85ms -> 10.69ms The third line is the point: the on-loop stall drops 190x because the work is spread across the calls that were happening anyway. Two honest costs. Total work over that spread rises 1.9x (sequential dictionary scan beats random-access set removes on cache), and the synthetic case where every entry shares one expiry millisecond is 21% worse. Peak latency is the currency for a game loop, and reaching that synthetic case needs an entire flood to land inside one millisecond. The SweepThrottleMs added earlier goes away with the scan it was throttling. The periodic timer stays, now O(expiring), purely to reclaim memory on a shard that goes quiet after a flood. Release now purges the ring record too. Nothing records that a key was released, so a re-detection before the old record lapsed would otherwise be retired early by it. O(n), on an operator retraction. Co-Authored-By: Claude Opus 5 (1M context) --- .../Network/AutoDenylist/AutoDenylistTests.cs | 76 +++--- .../Network/AutoDenylist/AutoDenylist.cs | 217 ++++++++++++------ 2 files changed, 202 insertions(+), 91 deletions(-) diff --git a/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs b/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs index 0c7ff7537..19dcd123c 100644 --- a/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs +++ b/Projects/UOContent.Tests/Tests/Network/AutoDenylist/AutoDenylistTests.cs @@ -62,8 +62,11 @@ public class AutoDenylistTests Assert.Equal(0, AutoDenylist.Count); } + // Not refreshed on purpose: it is what keeps insertion order equal to expiry order, so retiring lapsed + // entries costs the number expiring instead of the number held. A flooder whose hold lapses trips the + // rate limiter on its next attempt -- which runs ahead of the connection filters -- and is held again. [Fact] - public void Repeat_detection_extends_the_hold() + public void Repeat_detection_does_not_extend_the_hold() { Reset(); var ip = IPAddress.Parse("198.51.100.13"); @@ -71,8 +74,51 @@ public class AutoDenylistTests AutoDenylist.Hold(ip, BanReasons.SilentConnect, Now); AutoDenylist.Hold(ip, BanReasons.SilentConnect, Now + DurationMs - 1); - Assert.True(AutoDenylist.IsDenied(ip, Now + DurationMs + 1)); // would have lapsed without the second - Assert.Equal(1, AutoDenylist.Count); // and did not add a duplicate + Assert.Equal(1, AutoDenylist.Count); // no duplicate + Assert.True(AutoDenylist.IsDenied(ip, Now + DurationMs - 1)); + Assert.False(AutoDenylist.IsDenied(ip, Now + DurationMs + 1)); // lapses from the FIRST detection + } + + // The ring carries the expiry and the set carries membership; if they ever disagree, an address is + // either denied forever or retired early. + [Fact] + public void Ring_and_set_stay_in_step() + { + Reset(maxEntries: 4); + + for (var i = 0; i < 8; i++) + { + AutoDenylist.Hold(IPAddress.Parse($"198.51.100.{70 + i}"), BanReasons.InvalidSeed, Now); + } + + Assert.Equal(4, AutoDenylist.Count); + Assert.Equal(AutoDenylist.Count, AutoDenylist.RingCount); + + AutoDenylist.Release(IPAddress.Parse("198.51.100.71")); + Assert.Equal(3, AutoDenylist.Count); + Assert.Equal(AutoDenylist.Count, AutoDenylist.RingCount); + + AutoDenylist.Drain(Now + DurationMs + 1); + Assert.Equal(0, AutoDenylist.Count); + Assert.Equal(0, AutoDenylist.RingCount); + } + + // Releasing leaves no ring record behind, so a re-detection is not retired by the old one. + [Fact] + public void Release_then_re_hold_is_not_retired_by_the_stale_record() + { + Reset(); + var ip = IPAddress.Parse("198.51.100.15"); + + AutoDenylist.Hold(ip, BanReasons.RateLimit, Now); + AutoDenylist.Release(ip); + + var later = Now + DurationMs - 1; + AutoDenylist.Hold(ip, BanReasons.RateLimit, later); + + // The first hold's expiry has passed; the second must survive it. + Assert.True(AutoDenylist.IsDenied(ip, Now + DurationMs + 1)); + Assert.Equal(1, AutoDenylist.RingCount); } [Fact] @@ -104,30 +150,6 @@ public class AutoDenylistTests Assert.False(AutoDenylist.IsDenied(IPAddress.Parse("198.51.100.109"), Now)); } - // A sweep is O(maxEntries). Without the throttle a distinct-source flood re-scans the whole - // dictionary for every address it rejects, which is the case the cap exists to make cheap. - [Fact] - public void Cap_does_not_re_sweep_for_every_rejected_address() - { - Reset(maxEntries: 2); - - AutoDenylist.Hold(IPAddress.Parse("198.51.100.40"), BanReasons.InvalidSeed, Now); - AutoDenylist.Hold(IPAddress.Parse("198.51.100.41"), BanReasons.InvalidSeed, Now); - Assert.Equal(0, AutoDenylist.SweepCount); - - // At the cap with nothing lapsed: the first rejection sweeps, the rest in the window do not. - for (var i = 0; i < 5; i++) - { - Assert.False(AutoDenylist.Hold(IPAddress.Parse($"198.51.100.{50 + i}"), BanReasons.InvalidSeed, Now)); - } - - Assert.Equal(1, AutoDenylist.SweepCount); - - // Past the window it may try again, since entries can have lapsed by then. - Assert.False(AutoDenylist.Hold(IPAddress.Parse("198.51.100.60"), BanReasons.InvalidSeed, Now + 1000)); - Assert.Equal(2, AutoDenylist.SweepCount); - } - [Fact] public void Reaching_the_cap_reclaims_lapsed_entries_first() { diff --git a/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs b/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs index 4f468571e..4a6f079ea 100644 --- a/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs +++ b/Projects/UOContent/Network/AutoDenylist/AutoDenylist.cs @@ -32,28 +32,36 @@ namespace Server.Network; /// no bouncer, which is the default. Not persisted, by design: a holding pen that survives restarts is a ban /// without a ban's review. Only verdicts are held. /// +/// +/// A hold is never refreshed, so every expiry is insertion + duration and the ring is sorted by +/// construction. Retiring lapsed entries is therefore the number expiring rather than the number held, which +/// is what lets the cap be sized for the flood instead of for a scan. +/// public static class AutoDenylist { private static readonly ILogger logger = LogFactory.GetLogger(typeof(AutoDenylist)); - // Address (normalized v6 bits) -> Core.TickCount at which the hold lapses. Loop-only. - private static readonly Dictionary _held = []; + // Membership only (normalized v6 bits). Loop-only. The expiry lives beside the key in the ring, so + // there is exactly one copy of it and the two cannot disagree. + private static readonly HashSet _held = []; - // Shortest gap between cap-triggered sweeps. A sweep is O(_maxEntries) and one that just ran cannot - // have freed more, so without this every rejected address re-scans the whole dictionary -- turning the - // bound into a cost multiplier under exactly the flood it exists to bound. - private const long SweepThrottleMs = 1000; + // The same keys in expiry order. Parallel arrays rather than an array of structs: UInt128 forces + // 16-byte alignment, so a packed (key, expiry) struct costs 32 bytes where these cost 24 -- and the + // drain reads only the long[], 8 sequential bytes per entry. + private static UInt128[] _ringKeys = []; + private static long[] _ringExpiry = []; + private static int _ringHead; + private static int _ringCount; private static bool _enabled; private static long _durationMs; private static int _maxEntries; private static bool _warnedFull; - private static long _lastSweepAt; public static int Count => _held.Count; - // Test seam: lets the throttle be asserted rather than inferred. - internal static int SweepCount { get; private set; } + // Test seam: the ring and the set hold the same entries, and nothing else may assume it. + internal static int RingCount => _ringCount; public static void Configure() { @@ -69,9 +77,6 @@ public static class AutoDenylist _durationMs = (long)s.Duration.TotalMilliseconds; _maxEntries = s.MaxEntries; - // Seeded from a real tick: tick counts need not start near zero. See dev-docs/tick-counts.md. - _lastSweepAt = Core.TickCount; - ConnectionFilters.Register(new AutoDenylistFilter()); BanChannel.Register(new AutoDenylistReporter()); } @@ -89,38 +94,45 @@ public static class AutoDenylist return false; } - var key = address.ToUInt128(); + Drain(nowTicks); - // An address already held is just extended, so no cap check is needed. - if (!_held.ContainsKey(key) && _held.Count >= _maxEntries) + // Deliberately not refreshed: the first detection sets the expiry and later ones leave it alone. + // That is what keeps insertion order equal to expiry order, which is the whole reason the drain + // can stop at the first live record. A flooder whose hold lapses trips the rate limiter on its + // next attempt -- which runs before the connection filters -- and is held again immediately. + if (!_held.Add(address.ToUInt128())) { - if (nowTicks - _lastSweepAt >= SweepThrottleMs) - { - Sweep(nowTicks); - } - - if (_held.Count >= _maxEntries) - { - if (!_warnedFull) - { - _warnedFull = true; - logger.Warning( - "Auto-denylist is full at {Max} addresses; further detections are disconnected but not held", - _maxEntries - ); - } - - return false; - } + return true; } - _held[key] = nowTicks + _durationMs; + // Drain already reclaimed everything reclaimable, so being over now means genuinely full. + if (_held.Count > _maxEntries) + { + _held.Remove(address.ToUInt128()); + + if (!_warnedFull) + { + _warnedFull = true; + logger.Warning( + "Auto-denylist is full at {Max} addresses; further detections are disconnected but not held", + _maxEntries + ); + } + + return false; + } + + Push(address.ToUInt128(), nowTicks + _durationMs); return true; } public static bool IsDenied(IPAddress address) => IsDenied(address, Core.TickCount); - /// The pure decision, split out so the accept-path policy can be tested without a clock. + /// + /// The accept-path decision, split out so the policy can be tested without a clock. Drains first: the + /// expiry is not stored beside the membership, so there is nothing to expire on read and a lapsed hold + /// must be retired here rather than left to deny. Costs one array read when nothing has lapsed. + /// internal static bool IsDenied(IPAddress address, long nowTicks) { if (!_enabled || address == null) @@ -128,56 +140,132 @@ public static class AutoDenylist return false; } - // Decided on read, so a lapsed hold cannot deny even before the sweep. Subtraction: TickCount wraps. - return _held.TryGetValue(address.ToUInt128(), out var expires) && expires - nowTicks > 0; + Drain(nowTicks); + return _held.Contains(address.ToUInt128()); } /// Releases an address early, e.g. when an operator retracts a ban. public static void Release(IPAddress address) { - if (_enabled && address != null) - { - _held.Remove(address.ToUInt128()); - } - } - - internal static void Sweep(long nowTicks) - { - // Stamped even when there is nothing to do, so the cap path throttles off the last attempt. - _lastSweepAt = nowTicks; - SweepCount++; - - if (_held.Count == 0) + if (!_enabled || address == null) { return; } - var lapsed = 0; - - foreach (var (address, expires) in _held) + var key = address.ToUInt128(); + if (_held.Remove(key)) { - if (expires - nowTicks <= 0) - { - _held.Remove(address); - lapsed++; - } + // The ring record has to go too. Nothing records that this key was released, so if it were + // detected again before the old record lapsed, that record would retire the new hold early. + // O(n), but this is an operator retraction, not the accept path. + PurgeRing(key); + } + } + + /// + /// Retires everything that has lapsed. Expiries only ever increase along the ring, so the first live + /// record ends the scan and the cost is the number actually expiring, not the number held. + /// + internal static void Drain(long nowTicks) + { + // Subtraction, never a direct compare: tick counts wrap. See dev-docs/tick-counts.md. + while (_ringCount > 0 && _ringExpiry[_ringHead] - nowTicks <= 0) + { + _held.Remove(_ringKeys[_ringHead]); + _ringHead = _ringHead + 1 == _ringKeys.Length ? 0 : _ringHead + 1; + _ringCount--; + _warnedFull = false; + } + } + + private static void Push(UInt128 key, long expiry) + { + if (_ringCount == _ringKeys.Length) + { + Grow(); } - if (lapsed > 0) + var tail = _ringHead + _ringCount; + if (tail >= _ringKeys.Length) { - _warnedFull = false; + tail -= _ringKeys.Length; + } + + _ringKeys[tail] = key; + _ringExpiry[tail] = expiry; + _ringCount++; + } + + private static void Grow() + { + var size = Math.Max(64, _ringKeys.Length * 2); + var keys = new UInt128[size]; + var expiry = new long[size]; + + for (var i = 0; i < _ringCount; i++) + { + var from = _ringHead + i; + if (from >= _ringKeys.Length) + { + from -= _ringKeys.Length; + } + + keys[i] = _ringKeys[from]; + expiry[i] = _ringExpiry[from]; + } + + _ringKeys = keys; + _ringExpiry = expiry; + _ringHead = 0; + } + + private static void PurgeRing(UInt128 key) + { + var capacity = _ringKeys.Length; + + for (var i = 0; i < _ringCount; i++) + { + var at = _ringHead + i; + if (at >= capacity) + { + at -= capacity; + } + + if (_ringKeys[at] != key) + { + continue; + } + + // Close the gap so the ring stays contiguous and expiry-ordered. + for (var j = i; j < _ringCount - 1; j++) + { + var to = _ringHead + j; + if (to >= capacity) + { + to -= capacity; + } + + var from = to + 1 == capacity ? 0 : to + 1; + _ringKeys[to] = _ringKeys[from]; + _ringExpiry[to] = _ringExpiry[from]; + } + + _ringCount--; + return; } } internal static void LoadForTesting(bool enabled, long durationMs, int maxEntries) { _held.Clear(); + _ringKeys = []; + _ringExpiry = []; + _ringHead = 0; + _ringCount = 0; _enabled = enabled; _durationMs = durationMs; _maxEntries = maxEntries; _warnedFull = false; - _lastSweepAt = 0; - SweepCount = 0; } } @@ -194,11 +282,12 @@ public sealed class AutoDenylistFilter : IConnectionFilter public void Start(CancellationToken token) { - // Only an optimization: IsDenied expires on read. + // Only reclaims memory: Hold and IsDenied both drain, so this matters on a shard that has gone + // quiet after a flood and would otherwise hold the ring until someone next connects. _sweepTimer = Timer.DelayCall( TimeSpan.FromMinutes(1), TimeSpan.FromMinutes(1), - () => AutoDenylist.Sweep(Core.TickCount) + () => AutoDenylist.Drain(Core.TickCount) ); }