Skip to content

feat(command): rate limiting with gcra command#3740

Open
ndyakov wants to merge 14 commits intomasterfrom
ndyakov/gcra-cmd
Open

feat(command): rate limiting with gcra command#3740
ndyakov wants to merge 14 commits intomasterfrom
ndyakov/gcra-cmd

Conversation

@ndyakov
Copy link
Copy Markdown
Member

@ndyakov ndyakov commented Mar 18, 2026

This PR introduces GCRA command for rate limiting.

marked as draft/ work in progress until an image with the command is available for testing.


Note

Medium Risk
Introduces a new command type with custom reply parsing and integrates it into cluster routing/value extraction; mistakes could cause incorrect parsing or behavior in pipelines/cluster mode.

Overview
Adds first-class support for Redis 8.8+ GCRA rate limiting, including a new RateLimitCmdable API (GCRA/GCRAWithArgs), GCRAArgs/GCRAResult types, and a dedicated GCRACmd with explicit 5-element reply decoding.

Wires the new command type into the generic command-value extraction and OSS cluster router value-setting paths, and adds both integration and unit tests plus a standalone example/gcra-rate-limiting module demonstrating basic, burst, and weighted token usage.

Also includes small whitespace/format-only touch-ups in TLS examples/tests and a mock pooler test.

Reviewed by Cursor Bugbot for commit 899776c. Bugbot is set up for automated code reviews on this repo. Configure here.

@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented Mar 18, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

@ndyakov ndyakov force-pushed the ndyakov/gcra-cmd branch 2 times, most recently from e4b3c6d to 289d6e1 Compare March 20, 2026 13:52
@ndyakov ndyakov changed the title WIP: feat(command): rate limiting with gcra command feat(command): rate limiting with gcra command Mar 20, 2026
@ndyakov ndyakov marked this pull request as ready for review March 20, 2026 15:31
Comment thread command.go
ofekshenawa
ofekshenawa previously approved these changes Mar 22, 2026
Copy link
Copy Markdown
Collaborator

@ofekshenawa ofekshenawa left a comment

Choose a reason for hiding this comment

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

LGTM

@ndyakov
Copy link
Copy Markdown
Member Author

ndyakov commented Mar 23, 2026

I would prefer to merge other PRs and release a version before including this one (or any other 8.8 related changes) in master.

use proper image for 8.6
Comment thread Makefile
Comment thread command.go
Comment thread command.go
if err != nil {
return err
}
cmd.val.FullBurstAfter = fullBurstAfter
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ReadInt used for RESP double fields in reply

High Severity

The Redis GCRA command returns reply_after and full_burst_after as RESP doubles (using addReplyDouble on the server side), but readReply uses rd.ReadInt() to parse them. The ReadInt method doesn't handle the RespFloat wire type, so it will always fail with a parse error when reaching the 4th element. Additionally, RetryAfter and FullBurstAfter in GCRAResult are typed int64 but need to be float64 to hold the double values returned by the server.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d443382. Configure here.

Comment thread ratelimit_commands.go
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 899776c. Configure here.

Comment thread ratelimit_commands.go
// Add TOKENS if specified and not default
if args.Tokens != 1 {
cmdArgs = append(cmdArgs, "TOKENS", args.Tokens)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Zero-value Tokens causes unexpected Redis error

High Severity

The condition args.Tokens != 1 treats Go's zero-value (0) for the Tokens field as an explicit user choice, sending TOKENS 0 to Redis which rejects it as "out of range". When a user constructs GCRAArgs without setting Tokens, the field defaults to 0, not the documented default of 1. This means any caller of GCRAWithArgs that omits Tokens will get an unexpected error instead of the documented default behavior.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 899776c. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants