fix: no rate limiting if llm api calls #126
Conversation
WalkthroughThe pull request introduces a global API rate limiter using Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RateLimiter
participant Provider
participant API
rect rgb(220, 240, 255)
note over Caller,API: Concurrent generateMessage calls (e.g., 100 goroutines)
end
par Goroutine 1
Caller->>RateLimiter: Wait (Reserve token)
and Goroutine 2
Caller->>RateLimiter: Wait (Queue/Block if needed)
and Goroutine N
Caller->>RateLimiter: Wait (Queue/Block if needed)
end
rect rgb(240, 255, 240)
note over RateLimiter: Tokens released at 5 events/sec
end
loop As tokens become available
RateLimiter->>Provider: Generate (allows through)
Provider->>API: Make API call
API-->>Provider: Response
Provider-->>Caller: Return message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes introduce new concurrency-related logic with a rate limiter pattern, requiring careful verification of initialization, timing correctness, and thread-safety. The test suite adds comprehensive concurrent validation, but reviewers must assess whether the 5 events/sec configuration aligns with provider requirements and whether error handling for rate-limit exhaustion is properly managed. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/cli/createMsg_test.go (1)
20-48: Enhance test to verify actual rate limiting behavior.The test correctly validates that all concurrent calls eventually succeed (rate limiter queues rather than rejects), but it has several limitations:
No timing verification: The test doesn't verify that rate limiting actually occurred. Consider measuring elapsed time and asserting it's approximately
numCalls / 5seconds (≈20 seconds for 100 calls).Slow test: With 5 req/s and 100 calls, this test will take about 20 seconds to complete, which may be too slow for a unit test. Consider reducing to 10-20 calls for faster feedback.
Error handling: Line 34 uses
t.Logffor rate limiter errors, which doesn't immediately fail the test. Errors only reducesuccessCount, which is checked at the end. This is acceptable but consider whether rate limiter errors should fail immediately.Example improvement to verify timing:
func TestGenerateMessageRateLimiter(t *testing.T) { ctx := context.Background() + start := time.Now() var waitGroup sync.WaitGroup successCount := 0 var mu sync.Mutex - // Test sending a number of messages in a short period to check the rate limiter - numCalls := 100 + numCalls := 20 // Reduced for faster test execution waitGroup.Add(numCalls) for i := 0; i < numCalls; i++ { go func() { defer waitGroup.Done() _, err := generateMessage(ctx, FakeProvider{}, "", nil) if err != nil { - t.Logf("rate limiter error: %v", err) + t.Errorf("rate limiter error: %v", err) return } mu.Lock() successCount++ mu.Unlock() }() } waitGroup.Wait() + elapsed := time.Since(start) t.Logf("Successful calls: %d out of %d", successCount, numCalls) + t.Logf("Elapsed time: %v", elapsed) + + // With 5 req/s, expect at least (numCalls-5)/5 seconds (accounting for burst) + expectedMinDuration := time.Duration(numCalls-5) * time.Second / 5 + if elapsed < expectedMinDuration { + t.Errorf("rate limiting may not be working: completed too quickly (elapsed: %v, expected: >%v)", elapsed, expectedMinDuration) + } + if successCount != numCalls { t.Errorf("expected %d successful calls but got %d", numCalls, successCount) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/cli/createMsg.go(3 hunks)cmd/cli/createMsg_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/cli/createMsg_test.go (1)
pkg/types/options.go (1)
GenerationOptions(4-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build Go Binary (ubuntu-latest)
- GitHub Check: Build Go Binary (macos-latest)
- GitHub Check: Build Go Binary (windows-latest)
- GitHub Check: Test (1.22)
- GitHub Check: Test (1.23)
- GitHub Check: Test (1.21)
🔇 Additional comments (4)
cmd/cli/createMsg.go (3)
11-11: LGTM!The imports are appropriate for implementing rate limiting functionality.
Also applies to: 22-22
302-307: LGTM! Correct rate limiter application.The implementation properly waits for rate limiter permission before making the API call and handles context cancellation correctly.
25-27: Clarify the comment and reconsider the rate limit strategy.The review comment identifies valid concerns:
Misleading comment: The phrasing "Burst once every 5 times per second" does not clearly describe that the limiter allows 5 requests per second with burst capacity of 5.
Global limit for all providers: The codebase supports 6 providers (OpenAI, Claude, Gemini, Grok, Groq, Ollama), each with different API rate limits. The global 5 req/s limit applies uniformly without provider awareness.
Unjustified rate choice: No documentation or constant exists explaining why 5 req/s was selected. This appears arbitrary.
Limited effectiveness for stated use case: Within a single
CreateCommitMsginvocation,generateMessageis called sequentially (lines 128 and 191), meaning the per-process rate limiter provides minimal value unless multiple CLI processes run in parallel.The comment should be clarified and the rate limiting strategy reconsidered to either be provider-specific or made configurable.
cmd/cli/createMsg_test.go (1)
11-18: FakeProvider is correctly implemented; both review concerns are unfounded.The verification reveals:
Type compatibility is not an issue.
types.LLMProvideris defined astype LLMProvider string, making the string literal"fake"fully compatible and type-safe.Missing delay is not needed. Rate limiting is implemented in the
generateMessage()function viaapiRateLimiter.Wait(ctx)(called beforeprovider.Generate()), not within the provider mock itself. The test verifies that all 100 concurrent calls succeed by checking theapiRateLimitermechanism—adding delay to the mock would serve no purpose and wouldn't improve test coverage.FakeProvider correctly implements the two required methods of the
Providerinterface and is appropriate for the test.Likely an incorrect or invalid review comment.
DFanso
added
enhancement
2 participants
Description
No rate limiting if LLM API calls when executed this CLI tool.
Type of Change
Related Issue
Fixes #85
Changes Made
Add a rate limitation library in file
createMsg.goand make an unit test file namedcreateMsg_test.gounder the foldercmd/cli/to do the unit test with functiongenerateMessageTesting
Checklist
Screenshots (if applicable)
Additional Notes
For Hacktoberfest Participants
Thank you for your contribution! 🎉
Summary by CodeRabbit
New Features
Tests