feat(server): per-intent-type rate limits for social/diplomatic intents#4658
feat(server): per-intent-type rate limits for social/diplomatic intents#4658iiamlewis wants to merge 2 commits into
Conversation
The existing ClientMsgRateLimiter enforces a single global budget
(10/sec, 150/min) across all intent types, so a scripted quick-chat
loop or emoji/embargo spam stays under the ceiling by consuming the
shared budget in short bursts.
Add tighter per-intent-type token buckets for social/diplomatic actions
that no human issues in rapid succession (quick_chat, emoji,
allianceRequest, embargo, embargo_all), layered under the global limit.
The intent sub-type is now passed through from GameServer. Combat and
economy intents (attack, boat, move_warship, build_unit, ...) keep only
the global limit, so legitimate high-tempo play is unaffected.
Exceeding a per-type cap drops the single message ("limit"), never
kicks, so a rare lag burst costs at most one dropped social action.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPer-client message limiting now also applies configured caps to individual intent types. The server forwards intent classifications to the limiter, which maintains isolated per-client and per-intent buckets. Tests cover caps, fallback behavior, and isolation. ChangesPer-intent rate limiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GameServer
participant ClientMsgRateLimiter
participant RateLimiter
GameServer->>ClientMsgRateLimiter: check(clientID, messageType, bytes, intentType)
ClientMsgRateLimiter->>RateLimiter: remove per-intent tokens
RateLimiter-->>ClientMsgRateLimiter: return token result
ClientMsgRateLimiter-->>GameServer: return rate-limit result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/ClientMsgRateLimiter.ts (1)
70-92: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftFix the limit check order and safely access the limits object.
The current logic checks the global limit before the specific intent type limit. This means that even if a spam message is blocked by the specific limit, it already took a token from the global bucket. This leaves less budget for normal actions, which goes against the goal of this pull request. The per-type limit must be checked first so that spam is rejected before it reaches the global bucket.
Also, directly accessing object properties using a user-provided string like
intentTypecan crash the app if the string is"constructor"or"__proto__". We should useObject.prototype.hasOwnProperty.callto safely check if the property actually exists on the object.🐛 Proposed fix
- if ( - !bucket.perSecond.tryRemoveTokens(1) || - !bucket.perMinute.tryRemoveTokens(1) - ) { - return "limit"; - } // Tighter per-type cap for spammy social/diplomatic intents. if (intentType !== undefined) { - const limits = PER_INTENT_LIMITS[intentType]; - if (limits !== undefined) { + if (Object.prototype.hasOwnProperty.call(PER_INTENT_LIMITS, intentType)) { + const limits = PER_INTENT_LIMITS[intentType]; const typeBucket = this.getOrCreateTypeBucket( bucket, intentType, limits, ); if ( !typeBucket.perSecond.tryRemoveTokens(1) || !typeBucket.perMinute.tryRemoveTokens(1) ) { return "limit"; } } } + + if ( + !bucket.perSecond.tryRemoveTokens(1) || + !bucket.perMinute.tryRemoveTokens(1) + ) { + return "limit"; + }🤖 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 70 - 92, In the rate-checking flow, update the intent-specific limit handling before the global per-second and per-minute checks so rejected spam does not consume global tokens. In the intent lookup within the visible limiter method, use Object.prototype.hasOwnProperty.call on PER_INTENT_LIMITS before reading the user-provided intentType, while preserving the existing type-bucket rejection behavior.
🧹 Nitpick comments (1)
tests/server/ClientMsgRateLimiter.test.ts (1)
57-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test to prove spam does not exhaust the global bucket.
The current test only sends 4 messages, which does not reach the global limit of 10 messages per second. Consider adding a new test that explicitly sends 10 or more spam messages to ensure that the rejected spam does not consume the global tokens.
🧪 Proposed test addition
// A different intent type still has global budget left. expect(limiter.check(CLIENT_A, "intent", SMALL, "attack")).toBe("ok"); }); + + it("spamming a throttled type does not exhaust the global bucket", () => { + const limiter = new ClientMsgRateLimiter(); + // Send 10 quick_chat intents to exceed both limits. + for (let i = 0; i < 10; i++) { + limiter.check(CLIENT_A, "intent", SMALL, "quick_chat"); + } + // We should still have global tokens left because rejected spam does not consume them. + expect(limiter.check(CLIENT_A, "intent", SMALL, "attack")).toBe("ok"); + });🤖 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 `@tests/server/ClientMsgRateLimiter.test.ts` around lines 57 - 68, Add a test alongside the existing ClientMsgRateLimiter tests that sends at least 10 spam messages through check, verifies they are rejected without consuming global capacity, and then confirms a different intent type still returns "ok". Keep the existing per-type throttling test unchanged.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@src/server/ClientMsgRateLimiter.ts`:
- Around line 70-92: In the rate-checking flow, update the intent-specific limit
handling before the global per-second and per-minute checks so rejected spam
does not consume global tokens. In the intent lookup within the visible limiter
method, use Object.prototype.hasOwnProperty.call on PER_INTENT_LIMITS before
reading the user-provided intentType, while preserving the existing type-bucket
rejection behavior.
---
Nitpick comments:
In `@tests/server/ClientMsgRateLimiter.test.ts`:
- Around line 57-68: Add a test alongside the existing ClientMsgRateLimiter
tests that sends at least 10 spam messages through check, verifies they are
rejected without consuming global capacity, and then confirms a different intent
type still returns "ok". Keep the existing per-type throttling test unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a05cf65-7bbf-4780-987d-c0e7b2d72e3e
📒 Files selected for processing (3)
src/server/ClientMsgRateLimiter.tssrc/server/GameServer.tstests/server/ClientMsgRateLimiter.test.ts
1 participant
Problem
ClientMsgRateLimiterenforces a single global budget across all intent types — 10/sec and 150/min per client. Public cheat scripts abuse this: a quick-chat spam loop (fires onequick_chatper recipient every ~100ms), emoji spam, and repeatedembargo_alltoggles all stay under the global ceiling by sharing the budget in short bursts, while still flooding other players.Simply lowering the global limit would penalise legitimate high-tempo play (placing several attacks, boats, or warship moves in quick succession).
Fix
Add per-intent-type token buckets layered under the global limit, for social/diplomatic actions no human issues in rapid succession:
quick_chatemojiallianceRequestembargoembargo_allGameServer(clientMsg.intent.type); the newcheck()param is optional, so non-intent callers and existing tests are unchanged.attack,boat,move_warship,build_unit, …) keep only the global limit — no impact on competitive micro."limit"(drop the one message), never"kick", so a rare lag burst costs at most one dropped social action rather than a disconnect.Tests
Extended
tests/server/ClientMsgRateLimiter.test.ts:14/14 rate-limiter tests pass; server suite green.
🤖 Generated with Claude Code