Repository navigation
fix: make spectator rate limits atomic (#750) - #789
abhinav-phi wants to merge 1 commit into
Conversation
CheckReactionRateLimit and CheckQuestionRateLimit read the counter and RecordReaction and RecordQuestion incremented it in a separate call, so two connections using the same spectator hash could both pass the check before either recorded. Increment and set the expiry in one Lua script and return whether the action is within the limit.
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Atomic limiter operation and validation backend/internal/debate/rate_limiter.go, backend/internal/debate/rate_limiter_test.go, backend/go.mod |
The limiter adds atomic increment-and-expire handling and injectable Redis client support. Tests cover concurrent reaction attempts and question-window expiry. The module adds miniredis and gopher-lua dependencies. |
WebSocket question and reaction calls backend/websocket/debate_spectator.go |
Question and reaction handlers use the combined limiter operations and return when an operation errors or denies the request. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: rixitgithub
Merge Risk: 
Minimal · up to a7896
-
No actionable merge-blocking risk is identified; the change is mergeable after normal checks.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Full details: Linked Issues check
-
Explanation
Issue
#750requires atomic enforcement for question and reaction limits across connections that share a spectator hash.rate_limiter.gonow uses one Redis Lua operation for each action. The operation increments the counter, sets expiry on the first increment, and returns the new count.debate_spectator.gousesAllowQuestionandAllowReactionwithout a separate record call. The concurrency test confirms that exactly the configured reaction limit succeeds. The expiry test confirms question-window reset behavior.
Full details: Out of Scope Changes check
-
Explanation
The changes remain within Issue
#750. The Redis client constructor supports isolated rate-limiter tests. Theminiredisandgopher-luadependencies support those tests. The caller updates, Lua operation, and automated tests directly support atomic spectator rate limiting. No unrelated change is shown.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Link your account with GitcordThanks for opening this PR, @abhinav-phi! To receive Discord notifications and contributor tracking for this organization:
Once linked, Gitcord can notify you about reviews, merges, and more. — Posted by Gitcord |
Fixes #750
CheckReactionRateLimit/CheckQuestionRateLimitread the Redis counter, andRecordReaction/RecordQuestionincremented it in a separate call. Two connections sharing one spectator hash could both read a count below the limit and then both record, so the limit could be exceeded.The check and the record are now one Lua script:
INCR, setPEXPIREon the first increment, and return the new count.AllowReaction/AllowQuestioncall it and report whether the action is within the limit. The callers inwebsocket/debate_spectator.gouse the single call.Tests, with
miniredisas a test only dependency:AllowReactioncalls allow exactlyMaxReactions.Summary by CodeRabbit