From 3991bcc653e8f779079bd147fead51bc40a83c6e Mon Sep 17 00:00:00 2001 From: gamer147 Date: Tue, 2 Jun 2026 13:06:08 -0400 Subject: [PATCH] feat(battle-node): bound InboundTracker with watermark-guarded sliding window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit Md11 (part 1 of 2). Replace unbounded HashSet _seen with a WindowSize=256 ring (HashSet + Queue, LRU eviction). The stale-below-window guard (pubSeq <= HighWaterMark - WindowSize) prevents window eviction from re-admitting old seqs as novel — the load-bearing invariant. pubSeq is client-monotonic and SIO retransmit horizons are seconds-scale, so 256 covers realistic retries by a wide margin. HighWaterMark semantics preserved (Gungnir still reports it). Tests: 5 new InboundTrackerTests covering below-window guard, evicted-seq rejection, within-window dedup after eviction, memory bound, and watermark monotonicity. Co-Authored-By: Claude Opus 4.7 --- .../Reliability/InboundTracker.cs | 45 ++++++++++- .../Reliability/InboundTrackerTests.cs | 75 +++++++++++++++++++ 2 files changed, 116 insertions(+), 4 deletions(-) diff --git a/SVSim.BattleNode/Reliability/InboundTracker.cs b/SVSim.BattleNode/Reliability/InboundTracker.cs index 199e412..3011681 100644 --- a/SVSim.BattleNode/Reliability/InboundTracker.cs +++ b/SVSim.BattleNode/Reliability/InboundTracker.cs @@ -4,17 +4,54 @@ namespace SVSim.BattleNode.Reliability; /// Per-session inbound-emit ledger. Dedupes the client's pubSeq so we never dispatch /// a retransmitted emit twice; ack-echo (via SIO callback) is the caller's job. /// +/// +/// State is bounded: the ledger keeps the most recent +/// pubSeqs in an LRU ring. Seqs below HighWaterMark - WindowSize are +/// treated as stale-below-window and rejected without recording — this is what +/// prevents window eviction from re-admitting an old seq as novel. The pubSeq is +/// client-assigned monotonically; the bound is sized well above the realistic +/// Socket.IO retransmit horizon, so legitimate retransmits always fall inside. +/// public sealed class InboundTracker { - private readonly HashSet _seen = new(); + /// Sliding-window size. Anything below HighWaterMark - WindowSize is dropped. + public const int WindowSize = 256; + + private readonly HashSet _seen = new(WindowSize); + private readonly Queue _order = new(WindowSize); /// Highest pubSeq observed so far. Reported via Gungnir for diagnostics. public long HighWaterMark { get; private set; } - /// Record an incoming pubSeq. Returns true if the caller should dispatch the envelope, false on duplicate. + /// Record an incoming pubSeq. Returns true if the caller should dispatch the envelope, false on duplicate or stale-below-window. public bool Observe(long pubSeq) { - if (pubSeq > HighWaterMark) HighWaterMark = pubSeq; - return _seen.Add(pubSeq); + // Stale-below-window guard. Required AFTER HighWaterMark is past the window, + // otherwise an evicted ring entry would re-admit as novel. + if (HighWaterMark > 0 && pubSeq <= HighWaterMark - WindowSize) + return false; + + if (pubSeq > HighWaterMark) + { + HighWaterMark = pubSeq; + Record(pubSeq); + return true; + } + + if (_seen.Contains(pubSeq)) + return false; + Record(pubSeq); + return true; + } + + private void Record(long pubSeq) + { + if (_order.Count >= WindowSize) + { + var evicted = _order.Dequeue(); + _seen.Remove(evicted); + } + _order.Enqueue(pubSeq); + _seen.Add(pubSeq); } } diff --git a/SVSim.UnitTests/BattleNode/Reliability/InboundTrackerTests.cs b/SVSim.UnitTests/BattleNode/Reliability/InboundTrackerTests.cs index d811786..7892ac9 100644 --- a/SVSim.UnitTests/BattleNode/Reliability/InboundTrackerTests.cs +++ b/SVSim.UnitTests/BattleNode/Reliability/InboundTrackerTests.cs @@ -38,4 +38,79 @@ public class InboundTrackerTests t.Observe(5); Assert.That(t.HighWaterMark, Is.EqualTo(5)); } + + [Test] + public void Observe_returns_false_for_pubSeq_below_high_water_minus_window() + { + // Drive watermark past WindowSize, then re-observe seq=1. + // The watermark guard rejects it without consulting ring membership. + var t = new InboundTracker(); + for (long i = 1; i <= InboundTracker.WindowSize + 10; i++) t.Observe(i); + + Assert.That(t.Observe(pubSeq: 1), Is.False); + } + + [Test] + public void Evicted_seq_stays_rejected_by_watermark() + { + // Load-bearing invariant: even after seq=1 is evicted from the ring, + // re-observing it must still return false because the watermark guard + // catches `1 <= 257 - 256`. Otherwise window eviction would admit + // an old seq as novel. + var t = new InboundTracker(); + for (long i = 1; i <= InboundTracker.WindowSize; i++) t.Observe(i); // ring: {1..256} + t.Observe(InboundTracker.WindowSize + 1); // evicts 1; watermark=257 + + Assert.That(t.Observe(pubSeq: 1), Is.False); + } + + [Test] + public void Within_window_dedup_still_fires_after_eviction() + { + var t = new InboundTracker(); + for (long i = 1; i <= InboundTracker.WindowSize; i++) t.Observe(i); // ring: {1..256}, watermark=256 + t.Observe(pubSeq: 300); // ring: {45..256, 300}, watermark=300 + + // 200 is in the ring → dedup catches it. + Assert.That(t.Observe(pubSeq: 200), Is.False, "Within-window dedup still works after eviction."); + + // 44 was evicted, but is below HighWaterMark - WindowSize = 44 → stale guard. + Assert.That(t.Observe(pubSeq: 44), Is.False, "Below-window guard rejects evicted seqs."); + } + + [Test] + public void Memory_bound_stays_constant_after_many_observes() + { + // The dedup structures must not grow without bound. Internals are accessible + // via the existing InternalsVisibleTo("SVSim.UnitTests"). Use reflection so + // the test doesn't force the field-access modifier choice in production. + var t = new InboundTracker(); + for (long i = 1; i <= 10_000; i++) t.Observe(i); + + var seenField = typeof(InboundTracker).GetField("_seen", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); + var orderField = typeof(InboundTracker).GetField("_order", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); + Assert.That(seenField, Is.Not.Null); + Assert.That(orderField, Is.Not.Null); + + var seen = (System.Collections.Generic.IReadOnlyCollection)seenField!.GetValue(t)!; + var order = (System.Collections.Generic.IReadOnlyCollection)orderField!.GetValue(t)!; + Assert.That(seen.Count, Is.LessThanOrEqualTo(InboundTracker.WindowSize), + $"_seen must stay <= WindowSize ({InboundTracker.WindowSize})."); + Assert.That(order.Count, Is.LessThanOrEqualTo(InboundTracker.WindowSize), + $"_order must stay <= WindowSize ({InboundTracker.WindowSize})."); + } + + [Test] + public void HighWaterMark_is_not_moved_backward_by_eviction() + { + var t = new InboundTracker(); + for (long i = 1; i <= InboundTracker.WindowSize; i++) t.Observe(i); + var beforeEviction = t.HighWaterMark; + t.Observe(InboundTracker.WindowSize + 1); // eviction fires here + + Assert.That(t.HighWaterMark, Is.GreaterThanOrEqualTo(beforeEviction)); + Assert.That(t.HighWaterMark, Is.EqualTo(InboundTracker.WindowSize + 1)); + } }