fix: Fixes tick count wrap-around in movement throttle, and eliminates more allocations in NetState (#2603)
## Summary Removes the per-tick allocation in the movement throttle, fixes tick-count wrap-around bugs in the throttle and RTT probe state, and trims per-connection allocations and dead fields in `NetState`. ## Movement throttle - **No more per-tick `List<NetState>` snapshot.** `ProcessAllQueues()` iterates the `HashSet` directly and removes drained or disconnected states in place. `HashSet<T>.Remove` does not invalidate enumerators on .NET Core 3.0+ (verified on 10.0.11); only inserting a *new* member does, and the only `Add` is in the packet handler, which never nests with `Slice()`. The eager `Remove` calls in `RejectAndReset`, `ClearQueue`, and `ProcessMovementQueue` are gone; membership is reconciled once per tick from `_hasQueuedMovements`. - **Debug logging** is now gated solely by the per-connection `NetState.MovementLogging` flag. The global `movementThrottle.debugLogging` setting is removed. - **New settings**: `movementThrottle.maxRttBonus`, `movementThrottle.maxChainGap`, and `movementThrottle.speedHackNotificationCooldown` were fields with no config binding. ## Tick-count wrap-around All comparisons are now in subtraction form and no tick field uses zero as a sentinel: - `now < _nextMovementTime` in the queue drain loop → `now - _nextMovementTime < 0`. - `_lastMovementRecordTime > 0`, `_lastSpeedHackNotification`, `_rttProbeTime > 0`, and `_nextRttProbe == 0` sentinels replaced with `_hasMovementRecord`, `_speedHackNotified`, `_rttProbePending`, and a seeded `_nextRttProbe`. - `_lastQueueDepthCheck` and `_movementWindowStart` are seeded from `Core.TickCount` at construction and on reset instead of zero. User-visible effects of the old code: on hosts with pass-through counters (GCP) movement history never recorded and speed hack detection was silently off; on every host, staff speed hack notifications were suppressed until `Core.TickCount` exceeded the five-minute cooldown. ## NetState - `Instances` returns `HashSet<NetState>` again so engine-internal `foreach` uses the struct enumerator instead of boxing through `IReadOnlySet<T>`. - Removed `_sustainedQueueDepth` (declared and zeroed since #2266, never read), `_lastRtt` (now derived as `LastRtt` from the newest history slot), and `_rttProbeTimestampHiRes` (only fed one debug log line). 20 bytes per connection. - `HuePickers`, `Menus`, and `Trades` are lazily created instead of allocating three lists per connection, including every login-server connection that dies on shard select. `Trades` is released when it empties. All helpers and the `HuePickerResponse` / `MenuResponse` handlers are null-tolerant; the trade cancel loops keep their `i < Count` guards because `SecureTrade.Cancel()` runs virtual item hooks that can re-enter the same list. ## Testing - `dotnet build -c Release` clean. - All MovementThrottle tests pass (27), plus the Trade / Menu / HuePicker / NetState tests (32).
This commit is contained in:
Kamron Batman
GitHub
parent
c9875e7f64
commit
547c2ea0fa
5 changed files with 153 additions and 120 deletions
|
|
@ -50,9 +50,6 @@ public static class MovementThrottle
|
|||
private const int ClientMaxUnackedMovements = 5;
|
||||
private const int MaxQueueWithUnmodifiedClient = ClientMaxUnackedMovements - 1; // 4
|
||||
|
||||
// Debug logging - enable for testing speed hack detection
|
||||
private static bool _debugLogging = false;
|
||||
|
||||
// Track NetStates with queued movements for efficient processing
|
||||
private static readonly HashSet<NetState> _netStatesWithQueuedMovements = new(256);
|
||||
|
||||
|
|
@ -83,15 +80,9 @@ public static class MovementThrottle
|
|||
|
||||
public static void Configure()
|
||||
{
|
||||
_maxCredit = ServerConfiguration.GetOrUpdateSetting(
|
||||
"movementThrottle.maxCredit",
|
||||
_maxCredit
|
||||
);
|
||||
|
||||
_hardQueueLimit = ServerConfiguration.GetOrUpdateSetting(
|
||||
"movementThrottle.hardQueueLimit",
|
||||
_hardQueueLimit
|
||||
);
|
||||
_maxCredit = ServerConfiguration.GetOrUpdateSetting("movementThrottle.maxCredit", _maxCredit);
|
||||
_maxRttBonus = ServerConfiguration.GetOrUpdateSetting("movementThrottle.maxRttBonus", _maxRttBonus);
|
||||
_hardQueueLimit = ServerConfiguration.GetOrUpdateSetting("movementThrottle.hardQueueLimit", _hardQueueLimit);
|
||||
|
||||
_movementHistorySize = ServerConfiguration.GetOrUpdateSetting(
|
||||
"movementThrottle.movementHistorySize",
|
||||
|
|
@ -103,6 +94,13 @@ public static class MovementThrottle
|
|||
_minSamplesForRate
|
||||
);
|
||||
|
||||
_maxChainGap = ServerConfiguration.GetOrUpdateSetting("movementThrottle.maxChainGap", _maxChainGap);
|
||||
|
||||
_speedHackNotificationCooldown = ServerConfiguration.GetOrUpdateSetting(
|
||||
"movementThrottle.speedHackNotificationCooldown",
|
||||
_speedHackNotificationCooldown
|
||||
);
|
||||
|
||||
_suspiciousRateThreshold = (float)ServerConfiguration.GetOrUpdateSetting(
|
||||
"movementThrottle.suspiciousRateThreshold",
|
||||
_suspiciousRateThreshold
|
||||
|
|
@ -112,11 +110,6 @@ public static class MovementThrottle
|
|||
"movementThrottle.definiteRateThreshold",
|
||||
_definiteRateThreshold
|
||||
);
|
||||
|
||||
_debugLogging = ServerConfiguration.GetOrUpdateSetting(
|
||||
"movementThrottle.debugLogging",
|
||||
_debugLogging
|
||||
);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
|
@ -191,15 +184,16 @@ public static class MovementThrottle
|
|||
// Credit can go negative up to -dynamicCredit (debt limit)
|
||||
if (ns._movementCredit - earlyAmount >= -dynamicCredit)
|
||||
{
|
||||
var prevCredit = ns._movementCredit;
|
||||
// Use credit to cover early arrival
|
||||
ns._movementCredit -= earlyAmount;
|
||||
|
||||
if (_debugLogging && ns._movementLogging)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
var prevCredit = ns._movementCredit + earlyAmount;
|
||||
|
||||
logger.Debug(
|
||||
"[Credit] {Name}: delta={Delta}ms early={Early}ms credit={PrevCredit}->{Credit}/{MaxCredit} action=execute",
|
||||
mobile.RawName, delta, earlyAmount, prevCredit, ns._movementCredit, dynamicCredit
|
||||
mobile, delta, earlyAmount, prevCredit, ns._movementCredit, dynamicCredit
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -208,11 +202,11 @@ public static class MovementThrottle
|
|||
return;
|
||||
}
|
||||
|
||||
if (_debugLogging && ns._movementLogging)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
logger.Debug(
|
||||
"[Credit] {Name}: delta={Delta}ms early={Early}ms credit={Credit}/{MaxCredit} EXHAUSTED -> queue",
|
||||
mobile.RawName, delta, earlyAmount, ns._movementCredit, dynamicCredit
|
||||
mobile, delta, earlyAmount, ns._movementCredit, dynamicCredit
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -227,11 +221,11 @@ public static class MovementThrottle
|
|||
var prevCredit = ns._movementCredit;
|
||||
ns._movementCredit = Math.Min(ns._movementCredit + delta, dynamicCredit);
|
||||
|
||||
if (_debugLogging && ns._movementLogging && ns._movementCredit != prevCredit)
|
||||
if (ns._movementLogging && ns._movementCredit != prevCredit)
|
||||
{
|
||||
logger.Debug(
|
||||
"[Credit] {Name}: delta=+{Delta}ms credit={PrevCredit}->{Credit}/{MaxCredit} action=execute",
|
||||
mobile.RawName, delta, prevCredit, ns._movementCredit, dynamicCredit
|
||||
mobile, delta, prevCredit, ns._movementCredit, dynamicCredit
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
@ -247,12 +241,9 @@ public static class MovementThrottle
|
|||
{
|
||||
if (!mobile.Move(dir))
|
||||
{
|
||||
if (_debugLogging && ns._movementLogging)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
logger.Debug(
|
||||
"[Execute] {Name}: Move FAILED dir={Dir} seq={Seq} -> reject+reset",
|
||||
mobile.RawName, dir, seq
|
||||
);
|
||||
logger.Debug("[Execute] {Name}: Move FAILED dir={Dir} seq={Seq} -> reject+reset", mobile, dir, seq);
|
||||
}
|
||||
|
||||
// Movement failed (blocked, paralyzed, frozen, etc.)
|
||||
|
|
@ -260,11 +251,11 @@ public static class MovementThrottle
|
|||
return;
|
||||
}
|
||||
|
||||
if (_debugLogging && ns._movementLogging)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
logger.Debug(
|
||||
"[Execute] {Name}: Move OK dir={Dir} seq={Seq} nextMove={NextMove}ms",
|
||||
mobile.RawName, dir, seq, ns._nextMovementTime - Core.TickCount
|
||||
mobile, dir, seq, ns._nextMovementTime - Core.TickCount
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -304,11 +295,11 @@ public static class MovementThrottle
|
|||
ns._hasQueuedMovements = true;
|
||||
_netStatesWithQueuedMovements.Add(ns);
|
||||
|
||||
if (_debugLogging && ns._movementLogging)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
logger.Debug(
|
||||
"[Queue] {Name}: enqueued dir={Dir} seq={Seq} (depth={Depth})",
|
||||
ns.Mobile?.RawName, dir, seq, ns._movementQueue.Count
|
||||
ns.Mobile, dir, seq, ns._movementQueue.Count
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
@ -320,7 +311,6 @@ public static class MovementThrottle
|
|||
{
|
||||
ns.SendMovementRej(seq, mobile);
|
||||
ns.ResetMovementState();
|
||||
_netStatesWithQueuedMovements.Remove(ns);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
|
@ -333,20 +323,18 @@ public static class MovementThrottle
|
|||
return;
|
||||
}
|
||||
|
||||
// Process each NetState with queued movements
|
||||
// Use a snapshot to avoid modification during iteration
|
||||
var toProcess = new List<NetState>(_netStatesWithQueuedMovements);
|
||||
|
||||
for (var i = 0; i < toProcess.Count; i++)
|
||||
foreach (var ns in _netStatesWithQueuedMovements)
|
||||
{
|
||||
var ns = toProcess[i];
|
||||
if (!ns.Running)
|
||||
if (ns.Running)
|
||||
{
|
||||
_netStatesWithQueuedMovements.Remove(ns);
|
||||
continue;
|
||||
ProcessMovementQueue(ns);
|
||||
if (ns._hasQueuedMovements)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
ProcessMovementQueue(ns);
|
||||
_netStatesWithQueuedMovements.Remove(ns);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -356,6 +344,7 @@ public static class MovementThrottle
|
|||
public static void ProcessMovementQueue(NetState ns)
|
||||
{
|
||||
var mobile = ns.Mobile;
|
||||
|
||||
if (mobile?.Deleted != false)
|
||||
{
|
||||
ClearQueue(ns);
|
||||
|
|
@ -374,7 +363,7 @@ public static class MovementThrottle
|
|||
while (ns._movementQueue?.Count > 0)
|
||||
{
|
||||
// Check if it's time to execute
|
||||
if (now < ns._nextMovementTime)
|
||||
if (now - ns._nextMovementTime < 0)
|
||||
{
|
||||
// Not yet - leave remaining items in queue for next Slice
|
||||
break;
|
||||
|
|
@ -394,11 +383,11 @@ public static class MovementThrottle
|
|||
// Execute the move
|
||||
if (!mobile.Move(movement.Direction))
|
||||
{
|
||||
if (_debugLogging && ns._movementLogging)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
logger.Debug(
|
||||
"[Queue] {Name}: dequeued FAILED dir={Dir} (remaining={Remaining})",
|
||||
mobile.RawName, movement.Direction, remaining
|
||||
mobile, movement.Direction, remaining
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -407,12 +396,12 @@ public static class MovementThrottle
|
|||
return;
|
||||
}
|
||||
|
||||
if (_debugLogging && ns._movementLogging)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
var waited = now - ns._nextMovementTime;
|
||||
logger.Debug(
|
||||
"[Queue] {Name}: dequeued OK dir={Dir} (remaining={Remaining}, waited={Waited}ms)",
|
||||
mobile.RawName, movement.Direction, remaining, waited >= 0 ? waited : 0
|
||||
mobile, movement.Direction, remaining, waited >= 0 ? waited : 0
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -430,10 +419,6 @@ public static class MovementThrottle
|
|||
|
||||
// Update tracking
|
||||
ns._hasQueuedMovements = ns._movementQueue?.Count > 0;
|
||||
if (!ns._hasQueuedMovements)
|
||||
{
|
||||
_netStatesWithQueuedMovements.Remove(ns);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
|
@ -469,7 +454,6 @@ public static class MovementThrottle
|
|||
{
|
||||
ns._movementQueue?.Clear();
|
||||
ns._hasQueuedMovements = false;
|
||||
_netStatesWithQueuedMovements.Remove(ns);
|
||||
}
|
||||
|
||||
// Maximum expected packets per second (mounted running = 100ms = 10/sec, plus tolerance)
|
||||
|
|
@ -484,7 +468,7 @@ public static class MovementThrottle
|
|||
logger.Information(
|
||||
"Movement queue overflow: {Character} ({Account}) | " +
|
||||
"Queue reached hard limit: {Limit} | IP: {IP}",
|
||||
mobile?.RawName ?? "Unknown",
|
||||
mobile,
|
||||
ns.Account?.Username ?? "Unknown",
|
||||
_hardQueueLimit,
|
||||
ns.Address
|
||||
|
|
@ -516,7 +500,7 @@ public static class MovementThrottle
|
|||
private static void RecordMovement(NetState ns, long now, int cost, Direction dir, Mobile mobile)
|
||||
{
|
||||
// Calculate interval since last movement
|
||||
var interval = ns._lastMovementRecordTime > 0
|
||||
var interval = ns._hasMovementRecord
|
||||
? (int)(now - ns._lastMovementRecordTime)
|
||||
: -1; // -1 indicates first movement (no previous time)
|
||||
|
||||
|
|
@ -525,6 +509,7 @@ public static class MovementThrottle
|
|||
if (interval <= 0 || interval > _maxChainGap)
|
||||
{
|
||||
ns._lastMovementRecordTime = now;
|
||||
ns._hasMovementRecord = true;
|
||||
|
||||
// Use RTT to distinguish "stopped moving" vs "lagged"
|
||||
// - Stable low-latency connection with gap >> RTT → player stopped, reset history
|
||||
|
|
@ -544,19 +529,19 @@ public static class MovementThrottle
|
|||
// A large gap followed by a burst of packets = likely lag recovery, not speed hack
|
||||
ns._lastGapDuration = interval;
|
||||
|
||||
if (_debugLogging && mobile?.RawName != null)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
var action = shouldReset ? "history reset" : "history preserved (possible lag)";
|
||||
logger.Debug(
|
||||
"[Movement] {Name}: SKIP recording (gap {Gap}ms > {MaxGap}ms, " +
|
||||
"RTT={RTT}ms stable={Stable} → {Action})",
|
||||
mobile.RawName, interval, _maxChainGap, avgRtt, ns.HasStableConnection, action
|
||||
mobile, interval, _maxChainGap, avgRtt, ns.HasStableConnection, action
|
||||
);
|
||||
}
|
||||
}
|
||||
else if (_debugLogging && mobile?.RawName != null)
|
||||
else if (ns._movementLogging)
|
||||
{
|
||||
logger.Debug("[Movement] {Name}: SKIP recording (first in chain)", mobile.RawName);
|
||||
logger.Debug("[Movement] {Name}: SKIP recording (first in chain)", mobile);
|
||||
}
|
||||
|
||||
return;
|
||||
|
|
@ -572,12 +557,9 @@ public static class MovementThrottle
|
|||
// the next real move's interval artificially short, inflating rate.
|
||||
if (cost == 0)
|
||||
{
|
||||
if (_debugLogging && mobile?.RawName != null)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
logger.Debug(
|
||||
"[Movement] {Name}: SKIP direction-only change (preserves interval measurement)",
|
||||
mobile.RawName
|
||||
);
|
||||
logger.Debug("[Movement] {Name}: SKIP direction-only change (preserves interval measurement)", mobile);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
|
@ -613,15 +595,16 @@ public static class MovementThrottle
|
|||
}
|
||||
|
||||
ns._lastMovementRecordTime = now;
|
||||
ns._hasMovementRecord = true;
|
||||
|
||||
// Debug logging
|
||||
if (_debugLogging && mobile?.RawName != null)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
var historyCount = ns._movementHistoryFull ? _movementHistorySize : ns._movementHistoryIndex;
|
||||
logger.Debug(
|
||||
"[Movement] {Name}: interval={Interval}ms target={Target}ms queue={Queue} " +
|
||||
"flags={Flags} history={History}/{MaxHistory} RTT={RTT}ms",
|
||||
mobile.RawName, interval, cost, record.QueueDepth,
|
||||
mobile, interval, cost, record.QueueDepth,
|
||||
flags, historyCount, _movementHistorySize, ns.AverageRtt
|
||||
);
|
||||
}
|
||||
|
|
@ -814,7 +797,7 @@ public static class MovementThrottle
|
|||
var averageRtt = ns.AverageRtt;
|
||||
|
||||
// Detailed rate breakdown for debugging
|
||||
if (_debugLogging)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
logger.Debug("[MovementAnalysis] Rate={Rate:F3}, Samples={Samples}, RTT={RTT}ms",
|
||||
rate, sampleCount, averageRtt);
|
||||
|
|
@ -977,19 +960,19 @@ public static class MovementThrottle
|
|||
var verdict = AnalyzeMovement(ns, out var rate, out var sampleCount, out var confidence);
|
||||
|
||||
// Debug logging
|
||||
if (_debugLogging && ns.Mobile?.RawName != null)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
var (burstSize, _) = DetectRecentBurst(ns);
|
||||
var probeStatus = ns._rttProbeTime > 0 ? "pending" : "idle";
|
||||
var probeStatus = ns._rttProbePending ? "pending" : "idle";
|
||||
var queueDepth = ns._movementQueue?.Count ?? 0;
|
||||
logger.Debug(
|
||||
"[RateCheck] {Name}: rate={Rate:F3} samples={Samples} verdict={Verdict} " +
|
||||
"confidence={Confidence:P0} queue={Queue} burst={Burst} sustained={Sustained}s",
|
||||
ns.Mobile.RawName, rate, sampleCount, verdict, confidence, queueDepth, burstSize, ns._consecutiveHighRateSeconds
|
||||
ns.Mobile, rate, sampleCount, verdict, confidence, queueDepth, burstSize, ns._consecutiveHighRateSeconds
|
||||
);
|
||||
logger.Debug(
|
||||
" RTT: avg={Avg}ms last={Last}ms var={Var} samples={RttSamples} stable={Stable} probe={Probe}",
|
||||
ns.AverageRtt, ns._lastRtt, ns._rttVariance, ns._rttSampleCount, ns.HasStableConnection, probeStatus
|
||||
ns.AverageRtt, ns.LastRtt, ns._rttVariance, ns._rttSampleCount, ns.HasStableConnection, probeStatus
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -1025,11 +1008,11 @@ public static class MovementThrottle
|
|||
|
||||
if (shouldNotify)
|
||||
{
|
||||
if (_debugLogging)
|
||||
if (ns._movementLogging)
|
||||
{
|
||||
logger.Debug(
|
||||
"[ALERT] {Urgency} - {Name}: rate={Rate:F3} verdict={Verdict} confidence={Confidence:P0}",
|
||||
urgency, ns.Mobile?.RawName, rate, verdict, confidence
|
||||
urgency, ns.Mobile, rate, verdict, confidence
|
||||
);
|
||||
}
|
||||
NotifyStaff(ns, rate, sampleCount, confidence, verdict, urgency);
|
||||
|
|
@ -1054,11 +1037,12 @@ public static class MovementThrottle
|
|||
var now = Core.TickCount;
|
||||
|
||||
// Rate-limit notifications per player
|
||||
if (now - ns._lastSpeedHackNotification < _speedHackNotificationCooldown)
|
||||
if (ns._speedHackNotified && now - ns._lastSpeedHackNotification < _speedHackNotificationCooldown)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
ns._speedHackNotified = true;
|
||||
ns._lastSpeedHackNotification = now;
|
||||
|
||||
var mobile = ns.Mobile;
|
||||
|
|
@ -1070,7 +1054,7 @@ public static class MovementThrottle
|
|||
"PacketRate: {PacketRate}/s (peak: {PeakRate}/s) | RTT: {Rtt}ms (stable: {Stable}) | " +
|
||||
"Sustained: {Sustained}s | Queue: {Queue} | Location: {Location} Map: {Map} | IP: {IP}",
|
||||
urgency,
|
||||
mobile?.RawName ?? "Unknown",
|
||||
mobile,
|
||||
ns.Account?.Username ?? "Unknown",
|
||||
rate,
|
||||
sampleCount,
|
||||
|
|
@ -1138,7 +1122,7 @@ public static class MovementThrottle
|
|||
Verdict = verdict,
|
||||
Confidence = confidence,
|
||||
AverageRtt = ns.AverageRtt,
|
||||
LastRtt = ns._lastRtt,
|
||||
LastRtt = ns.LastRtt,
|
||||
RttVariance = ns._rttVariance,
|
||||
StableConnection = ns.HasStableConnection,
|
||||
RttSampleCount = ns._rttSampleCount,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue