Fix #17: Add rate limit to the Core class #19
Conversation
WalkthroughThis pull request introduces a new configuration mechanism to manage request delays when interacting with the Anthropic API. The changes span across multiple files, adding an environment variable Changes
Sequence DiagramsequenceDiagram
participant Client as LLMClient
participant API as Anthropic API
Client->>Client: Check last request timestamp
alt Time since last request < min delay
Client-->>Client: Wait remaining time
end
Client->>API: Send request
API-->>Client: Receive response
Client->>Client: Update last request timestamp
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/core/src/core/env.ts (1)
15-18: Consider adding input validation for ANTHROPIC_MIN_DELAY_MS.The implementation looks good, but consider adding validation to ensure the delay is non-negative and reasonable (e.g., not exceeding 60000ms).
ANTHROPIC_MIN_DELAY_MS: z .string() .optional() - .transform((val: string | undefined) => (val ? parseInt(val, 10) : 0)), + .transform((val: string | undefined) => (val ? parseInt(val, 10) : 0)) + .refine((val) => val >= 0 && val <= 60000, { + message: "ANTHROPIC_MIN_DELAY_MS must be between 0 and 60000ms" + }),.env.example (1)
16-17: Enhance documentation for ANTHROPIC_MIN_DELAY_MS.The comment is helpful but could be more specific about the configuration details.
-# Optional: Set a minimum delay between Anthropic requests to prevent rate limits (eg 2000 and increase until resolved) +# Optional: Minimum delay between Anthropic API requests in milliseconds +# - Default: 0 (no delay) +# - Recommended range: 1000-5000ms +# - Increase if you encounter rate limits +# - Note: Higher values will impact response times
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.env.example(1 hunks)packages/core/src/core/env.ts(1 hunks)packages/core/src/core/llm-client.ts(5 hunks)
🔇 Additional comments (1)
packages/core/src/core/llm-client.ts (1)
26-26: LGTM: Config handling is well-implemented.Good implementation of the config with proper fallback to env variable when not explicitly provided in the config.
Also applies to: 63-67
| private lastRequestTimestamp = 0; | ||
| private logger: Logger; |
There was a problem hiding this comment.
Potential race condition in rate limiting implementation.
The current implementation of rate limiting might not work correctly in concurrent scenarios. The lastRequestTimestamp could be modified by another request between the delay check and the timestamp update.
Consider using a mutex or moving the timestamp update into the enforceMinDelay method:
private async enforceMinDelay(): Promise<void> {
if (this.config.minTimeBetweenRequestsMs > 0) {
const now = Date.now();
const elapsed = now - this.lastRequestTimestamp;
const remaining = this.config.minTimeBetweenRequestsMs - elapsed;
if (remaining > 0) {
this.logger.debug(
"LLMClient.enforceMinDelay",
`Waiting ${remaining} ms to respect minTimeBetweenRequestsMs`,
{
lastRequestTimestamp: this.lastRequestTimestamp,
now,
}
);
await setTimeout(remaining);
}
+ this.lastRequestTimestamp = Date.now();
}
}And remove the timestamp update from the complete method:
public async complete(prompt: string): Promise<LLMResponse> {
await this.enforceMinDelay();
let lastError: Error | null = null;
for (let attempt = 1; attempt <= this.config.maxRetries; attempt++) {
try {
const result = await this.executeCompletion(prompt);
- this.lastRequestTimestamp = Date.now();
return result;Also applies to: 128-145
| // Pull from config if set, otherwise from env if not explicitly provided: | ||
| minTimeBetweenRequestsMs: | ||
| typeof config.minTimeBetweenRequestsMs === "number" | ||
| ? config.minTimeBetweenRequestsMs | ||
| : env.ANTHROPIC_MIN_DELAY_MS, // fallback to env value |
There was a problem hiding this comment.
default should be an amount - i'm not sure we need to have this as an ENV var. It should just be a config when you make your dream chain
|
10-4. Will reopen after fetching and updating |
feat(core/env, llm-client): add optional ANTHROPIC_MIN_DELAY_MS env and minTimeBetweenRequestsMs config
Summary by CodeRabbit
New Features
Improvements