Skip to content

fix: replace never-resetting byte counter with sliding window in rate limiter #3881

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
berkelmali wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: replace never-resetting byte counter with sliding window in rate limiter #3881

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/server/ClientMsgRateLimiter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import { ClientID } from "../core/Schemas";
const INTENTS_PER_SECOND = 10;
const INTENTS_PER_MINUTE = 150;
const MAX_INTENT_SIZE = 2000;
const TOTAL_BYTES = 2 * 1024 * 1024; // 2MB per client
const TOTAL_BYTES = 2 * 1024 * 1024; // 2MB per client per window
const BYTE_WINDOW_MS = 60_000; // Sliding window of 60 seconds
export type RateLimitResult = "ok" | "limit" | "kick";

interface ClientBucket {
perSecond: RateLimiter;
perMinute: RateLimiter;
byteEvents: Array<{ at: number; bytes: number }>;
totalBytes: number;
}

Expand All @@ -18,6 +20,19 @@ export class ClientMsgRateLimiter {

check(clientID: ClientID, type: string, bytes: number): RateLimitResult {
const bucket = this.getOrCreate(clientID);

// Rolling-window byte accounting: evict events older than BYTE_WINDOW_MS
// so throughput is measured over a true sliding window instead of
// accumulating bytes for the entire game duration (which would
// false-kick legitimate long-running clients).
const now = Date.now();
const cutoff = now - BYTE_WINDOW_MS;
while (bucket.byteEvents.length > 0 && bucket.byteEvents[0].at < cutoff) {
const evicted = bucket.byteEvents.shift()!;
bucket.totalBytes -= evicted.bytes;
}
Comment on lines +30 to +33
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Array.shift() in the hot path can cause CPU spikes under spam traffic.

This loop reindexes the whole array on every eviction, so worst-case cost grows fast with many tiny messages. In a rate-limiter path, that can be used as a DoS vector.

Suggested local refactor (head index queue, no shift) 
 interface ClientBucket {
   perSecond: RateLimiter;
   perMinute: RateLimiter;
   byteEvents: Array<{ at: number; bytes: number }>;
+  byteEventsHead: number;
   totalBytes: number;
 }

 // in check()
- while (bucket.byteEvents.length > 0 && bucket.byteEvents[0].at < cutoff) {
-   const evicted = bucket.byteEvents.shift()!;
+ while (
+   bucket.byteEventsHead < bucket.byteEvents.length &&
+   bucket.byteEvents[bucket.byteEventsHead].at < cutoff
+ ) {
+   const evicted = bucket.byteEvents[bucket.byteEventsHead++]!;
    bucket.totalBytes -= evicted.bytes;
  }

+ // periodic compact to keep memory bounded
+ if (bucket.byteEventsHead > 1024 && bucket.byteEventsHead * 2 > bucket.byteEvents.length) {
+   bucket.byteEvents = bucket.byteEvents.slice(bucket.byteEventsHead);
+   bucket.byteEventsHead = 0;
+ }

  bucket.byteEvents.push({ at: now, bytes });

 // in getOrCreate()
   const bucket = {
     perSecond: new RateLimiter({ tokensPerInterval: INTENTS_PER_SECOND, interval: "second" }),
     perMinute: new RateLimiter({ tokensPerInterval: INTENTS_PER_MINUTE, interval: "minute" }),
     byteEvents: [],
+    byteEventsHead: 0,
     totalBytes: 0,
   };

Also applies to: 35-35, 74-74

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/ClientMsgRateLimiter.ts` around lines 30 - 33, The loop that
evicts old events uses bucket.byteEvents.shift() which reindexes the array and
causes CPU spikes under high traffic; update the data structure in
ClientMsgRateLimiter to avoid shifting by implementing a head index or ring
buffer for bucket.byteEvents (and the similar occurrences around the code that
manipulate bucket.byteEvents at the other noted sites), removing uses of
Array.shift() and instead incrementing a head pointer and decrementing
bucket.totalBytes when evicting, and ensure any checks (length/emptiness and
iteration) account for the head pointer so logic in the functions/methods that
reference bucket.byteEvents continues to work correctly.


bucket.byteEvents.push({ at: now, bytes });
bucket.totalBytes += bytes;

if (bucket.totalBytes >= TOTAL_BYTES) return "kick";
Expand Down Expand Up @@ -56,6 +71,7 @@ export class ClientMsgRateLimiter {
tokensPerInterval: INTENTS_PER_MINUTE,
interval: "minute",
}),
byteEvents: [],
totalBytes: 0,
};
this.buckets.set(clientID, bucket);
Expand Down
Loading