Rate limits and budgets #1366
Rate limits and budgets #1366
Conversation
fix errors
|
Note PR Review SkippedPR review skipped as no relevant changes found due to large diff hunk OR part of a non-reviewable file. 📄Files skipped in review
💡Tips to use MatterAICommand List
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR introduces rate limiting and budgeting features with new cache backends and Redis-based rate limiter. Key areas reviewed include cache backend implementations, rate limiter logic, and initialization logic.
Skipped files
package-lock.json: Skipped file patternsettings.example.json: Skipped file patternwrangler.toml: Skipped file pattern
initializeSettings.ts
Outdated
| const transformIntegrations = (integrations: any) => { | ||
| return integrations.map((integration: any) => { | ||
| return { | ||
| id: '1234567890', //need to do consistent hashing for caching |
There was a problem hiding this comment.
🔴 Security: Hardcoded ID used for consistent hashing. This could lead to cache collision or security issues if the ID is predictable. Consider using a proper consistent hashing mechanism or a secure random ID.
| id: '1234567890', //need to do consistent hashing for caching | |
| id: crypto.randomUUID(), // Use a proper consistent hashing mechanism or secure random ID |
| private createBackend(config: CacheConfig): CacheBackend { | ||
| switch (config.backend) { | ||
| case 'memory': | ||
| return new MemoryCacheBackend(config.maxSize, config.cleanupInterval); | ||
|
|
||
| case 'file': | ||
| return new FileCacheBackend( | ||
| config.dataDir, | ||
| config.fileName, | ||
| config.saveInterval, | ||
| config.cleanupInterval | ||
| ); | ||
|
|
||
| case 'redis': | ||
| if (!config.redisUrl) { | ||
| throw new Error('Redis URL is required for Redis backend'); | ||
| } | ||
| return createRedisBackend(config.redisUrl, { | ||
| ...config.redisOptions, | ||
| dbName: config.dbName || 'cache', | ||
| }); | ||
|
|
||
| case 'cloudflareKV': | ||
| if (!config.kvBindingName || !config.dbName) { | ||
| throw new Error( | ||
| 'Cloudflare KV binding name and db name are required for Cloudflare KV backend' | ||
| ); | ||
| } | ||
| return createCloudflareKVBackend( | ||
| config.env, | ||
| config.kvBindingName, | ||
| config.dbName | ||
| ); | ||
|
|
||
| default: | ||
| throw new Error(`Unsupported cache backend: ${config.backend}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟢 Design: Well-structured cache service with pluggable backends. The use of factory functions for backend creation is a good pattern.
| // For backends that don't support atomic increment, we simulate it | ||
| const current = (await this.get<number>(key, options.namespace)) || 0; | ||
| const newValue = current + delta; | ||
| await this.set(key, newValue, options); | ||
| return newValue; |
There was a problem hiding this comment.
🟡 Performance: The increment method simulates atomic increment for backends that don't support it. This can lead to race conditions. Consider using Redis atomic operations or other backend-specific atomic increment methods if available.
| const RATE_LIMIT_LUA = ` | ||
| local tokensKey = KEYS[1] | ||
| local refillKey = KEYS[2] | ||
| local capacity = tonumber(ARGV[1]) | ||
| local windowSize = tonumber(ARGV[2]) | ||
| local units = tonumber(ARGV[3]) | ||
| local now = tonumber(ARGV[4]) | ||
| local ttl = tonumber(ARGV[5]) | ||
| local consume = tonumber(ARGV[6]) -- 1 = consume, 0 = check only | ||
| -- Reject invalid input | ||
| if units <= 0 or capacity <= 0 or windowSize <= 0 then | ||
| return {0, -1, -1} | ||
| end | ||
| local lastRefill = tonumber(redis.call("GET", refillKey) or "0") | ||
| local tokens = tonumber(redis.call("GET", tokensKey) or "-1") | ||
| local tokensModified = false | ||
| local refillModified = false | ||
| -- Initialization | ||
| if tokens == -1 then | ||
| tokens = capacity | ||
| tokensModified = true | ||
| end | ||
| if lastRefill == 0 then | ||
| lastRefill = now | ||
| refillModified = true | ||
| end | ||
| -- Refill logic | ||
| local elapsed = now - lastRefill | ||
| if elapsed > 0 then | ||
| local rate = capacity / windowSize | ||
| local tokensToAdd = math.floor(elapsed * rate) | ||
| if tokensToAdd > 0 then | ||
| tokens = math.min(tokens + tokensToAdd, capacity) | ||
| lastRefill = now -- simpler and avoids drift | ||
| tokensModified = true | ||
| refillModified = true | ||
| end | ||
| end | ||
| -- Consume logic | ||
| local allowed = 0 | ||
| local waitTime = 0 | ||
| local currentTokens = tokens | ||
| if tokens >= units then | ||
| allowed = 1 | ||
| if consume == 1 then | ||
| tokens = tokens - units | ||
| tokensModified = true | ||
| end | ||
| else | ||
| local needed = units - currentTokens | ||
| local rate = capacity / windowSize | ||
| waitTime = (rate > 0) and math.floor(needed / rate) or -1 | ||
| end | ||
| -- Save changes | ||
| if tokensModified then | ||
| redis.call("SET", tokensKey, tokens, "PX", ttl) | ||
| end | ||
| if refillModified then | ||
| redis.call("SET", refillKey, lastRefill, "PX", ttl) | ||
| end | ||
| return {allowed, waitTime, currentTokens} | ||
| `; |
There was a problem hiding this comment.
🟢 Performance: Efficient use of Lua script for rate limiting logic in Redis. This minimizes round trips and ensures atomicity.
| if (error.message.includes('NOSCRIPT')) { | ||
| // Script not loaded on target node - load it and retry with same SHA | ||
| await this.redis.script('LOAD', RATE_LIMIT_LUA); | ||
| return await this.redis.evalsha(sha, keys.length, ...keys, ...args); | ||
| } |
There was a problem hiding this comment.
🟡 Error Handling: The evalsha retry logic assumes the script will be loaded on retry. Ensure the Redis cluster configuration supports script replication or handle potential failures more gracefully.
| class Logger { | ||
| private config: LoggerConfig; | ||
| private colors = { | ||
| error: '\x1b[31m', // red | ||
| critical: '\x1b[35m', // magenta | ||
| warn: '\x1b[33m', // yellow | ||
| info: '\x1b[36m', // cyan | ||
| debug: '\x1b[37m', // white | ||
| reset: '\x1b[0m', | ||
| }; | ||
|
|
||
| constructor(config: LoggerConfig) { | ||
| this.config = { | ||
| timestamp: true, | ||
| colors: true, | ||
| ...config, | ||
| }; | ||
| } | ||
|
|
||
| private formatMessage(level: string, message: string): string { | ||
| const parts: string[] = []; | ||
|
|
||
| if (this.config.timestamp) { | ||
| parts.push(`[${new Date().toISOString()}]`); | ||
| } | ||
|
|
||
| if (this.config.prefix) { | ||
| parts.push(`[${this.config.prefix}]`); | ||
| } | ||
|
|
||
| parts.push(`[${level.toUpperCase()}]`); | ||
| parts.push(message); | ||
|
|
||
| return parts.join(' '); | ||
| } | ||
|
|
||
| private log(level: LogLevel, levelName: string, message: string, data?: any) { | ||
| if (level > this.config.level) return; | ||
|
|
||
| const formattedMessage = this.formatMessage(levelName, message); | ||
| const color = this.config.colors | ||
| ? this.colors[levelName as keyof typeof this.colors] | ||
| : ''; | ||
| const reset = this.config.colors ? this.colors.reset : ''; | ||
|
|
||
| if (data !== undefined) { | ||
| console.log(`${color}${formattedMessage}${reset}`, data); | ||
| } else { | ||
| console.log(`${color}${formattedMessage}${reset}`); | ||
| } | ||
| } | ||
|
|
||
| error(message: string, error?: Error | any) { | ||
| if (error instanceof Error) { | ||
| this.log(LogLevel.ERROR, 'error', `${message}: ${error.message}`); | ||
| if (this.config.level >= LogLevel.DEBUG) { | ||
| console.error(error.stack); | ||
| } | ||
| } else if (error) { | ||
| this.log(LogLevel.ERROR, 'error', message, error); | ||
| } else { | ||
| this.log(LogLevel.ERROR, 'error', message); | ||
| } | ||
| } | ||
|
|
||
| critical(message: string, data?: any) { | ||
| this.log(LogLevel.CRITICAL, 'critical', message, data); | ||
| } | ||
|
|
||
| warn(message: string, data?: any) { | ||
| this.log(LogLevel.WARN, 'warn', message, data); | ||
| } | ||
|
|
||
| info(message: string, data?: any) { | ||
| this.log(LogLevel.INFO, 'info', message, data); | ||
| } | ||
|
|
||
| debug(message: string, data?: any) { | ||
| this.log(LogLevel.DEBUG, 'debug', message, data); | ||
| } | ||
|
|
||
| createChild(prefix: string): Logger { | ||
| return new Logger({ | ||
| ...this.config, | ||
| prefix: this.config.prefix ? `${this.config.prefix}:${prefix}` : prefix, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Create default logger instance | ||
| const defaultConfig: LoggerConfig = { | ||
| level: process.env.LOG_LEVEL | ||
| ? LogLevel[process.env.LOG_LEVEL.toUpperCase() as keyof typeof LogLevel] || | ||
| LogLevel.ERROR | ||
| : process.env.NODE_ENV === 'production' | ||
| ? LogLevel.ERROR | ||
| : LogLevel.INFO, | ||
| timestamp: process.env.LOG_TIMESTAMP !== 'false', | ||
| colors: | ||
| process.env.LOG_COLORS !== 'false' && process.env.NODE_ENV !== 'production', | ||
| }; | ||
|
|
||
| export const logger = new Logger(defaultConfig); | ||
|
|
||
| // Helper to create a logger for a specific component | ||
| export function createLogger(prefix: string): Logger { | ||
| return logger.createChild(prefix); | ||
| } |
There was a problem hiding this comment.
🟢 Design: Well-designed logger with configurable levels, prefixes, and colors. The use of environment variables for configuration is a good practice.
|
Note PR Review SkippedPR review skipped as no relevant changes found due to large diff hunk OR part of a non-reviewable file. 📄Files skipped in review
💡Tips to use MatterAICommand List
|
|
Note PR Review SkippedPR review skipped as no relevant changes found due to large diff hunk OR part of a non-reviewable file. 📄Files skipped in review
💡Tips to use MatterAICommand List
|
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
|
DescriptionSummary By MatterAI
🔄 What ChangedThis pull request introduces a new mechanism for loading application settings from a local 🔍 Impact of the ChangeThese changes significantly improve the gateway's configurability and robustness. The ability to load settings from 📁 Total Files Changed
🧪 Test AddedN/A 🔒Security VulnerabilitiesPotential for misconfiguration or malicious MotivationTo enable flexible, file-based configuration for self-hosted instances, enhance application resilience with global error handling, and standardize Redis cache initialization. Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
Caution Package Vulnerabilities
Tip Quality Recommendations
Tanka Poem ♫
Sequence DiagramsequenceDiagram
participant Process
participant FS as File System
participant IS as initializeSettings.ts
participant IDX as src/index.ts
participant Redis
participant SS as src/start-server.ts
Note over Process,IS: Application Startup
Process->>IS: Check FETCH_SETTINGS_FROM_FILE env
alt FETCH_SETTINGS_FROM_FILE is 'true'
IS->>FS: readFile('./conf.json', 'utf-8')
FS-->>IS: settingsFile content
IS->>IS: JSON.parse(settingsFile)
IS->>IS: transformIntegrations(settingsFileJson.integrations)
IS-->>Process: settings object
else FETCH_SETTINGS_FROM_FILE is not 'true'
IS-->>Process: undefined
end
Note over IDX: Cache Initialization
IDX->>IDX: getRuntimeKey()
IDX->>Process: Check REDIS_CONNECTION_STRING env
alt runtime is 'node' AND REDIS_CONNECTION_STRING exists
IDX->>Redis: createCacheBackendsRedis(REDIS_CONNECTION_STRING)
Redis-->>IDX: Cache client initialized
end
Note over SS: Global Error Handling
SS->>Process: process.on('uncaughtException', handler)
SS->>Process: process.on('unhandledRejection', handler)
|
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
|
Fix incorrect error logging and improve cache backend consistency |
|
Improved error logging for Cloudflare KV keys operation |
|
Improved error logging in Cloudflare KV stats retrieval |
|
Minor logging improvement for Cloudflare KV cleanup |
initializeSettings.ts
Outdated
| }); | ||
| }; | ||
|
|
||
| let settings: any = undefined; |
There was a problem hiding this comment.
🔴 Type Safety
Issue: Using any type for settings defeats TypeScript's type safety and can lead to runtime errors.
Fix: Define an interface for settings or use a more specific type.
Impact: Improved type safety and developer experience.
initializeSettings.ts
Outdated
| if (settingsFile) { | ||
| settings = {}; |
There was a problem hiding this comment.
🟡 Logic Error
Issue: The condition if (settingsFile) will always be true because import() returns a module object even if the file is empty. This means settings will always be initialized as an empty object regardless of the file content.
Fix: Check for specific properties or use a validation schema to ensure the imported file has the expected structure.
Impact: Prevents incorrect initialization when settings file is empty or malformed.
…nt is greater than the available tokens
| if tokens > 0 then | ||
| tokensModified = true | ||
| end | ||
| tokens = 0 |
There was a problem hiding this comment.
🔴 Logic Error
Issue: The current logic sets tokens = 0 unconditionally after checking if tokens > 0, which means tokens are always reset to 0 even when there aren't enough tokens to satisfy the request. This breaks the rate limiting mechanism.
Fix: Only reset tokens to 0 when they're actually consumed, and ensure proper handling of token availability.
Impact: Correct rate limiting behavior and resource allocation.
|
PR introduces rate limits and budgets. Review focuses on ensuring correct implementation and avoiding regressions. |
|
Code reorganization and import statement changes. No functional changes detected. |
|
Note PR Review SkippedPR review skipped as no relevant changes found due to large diff hunk OR part of a non-reviewable file. 📄Files skipped in review
💡Tips to use MatterAICommand List
|
|
Review of rate limit and budget handling changes |
| getFullKey(key: string, namespace?: string): string { | ||
| return namespace | ||
| ? `${this.dbName}:${namespace}:${key}` | ||
| : `${this.dbName}:default:${key}`; | ||
| } |
There was a problem hiding this comment.
🔴 Security/Correctness Issue
Issue: The getFullKey method hardcodes 'default' as the namespace when none is provided. This could lead to key collisions or unexpected behavior when different parts of the application expect distinct namespaces.
Fix: Consider making the default namespace configurable or requiring explicit namespace specification to avoid ambiguity.
Impact: Prevents potential key collisions and ensures predictable cache key generation.
| async script(mode: 'LOAD' | 'EXISTS', script: string): Promise<unknown> { | ||
| return await this.client.script('LOAD', script); | ||
| } |
There was a problem hiding this comment.
🟡 Correctness Issue
Issue: In the script method, only 'LOAD' operation is handled. When mode is 'EXISTS', the method will still attempt to execute 'LOAD', which is incorrect.
Fix: Handle both 'LOAD' and 'EXISTS' operations properly by checking the mode parameter.
Impact: Ensures correct behavior for both script loading and existence checking operations.
| this.tokensKey = `default:default:${tag}:tokens`; | ||
| this.lastRefillKey = `default:default:${tag}:lastRefill`; |
There was a problem hiding this comment.
🔴 Security/Correctness Issue
Issue: Hardcoded 'default:default:' namespace in rate limiter keys bypasses the RedisCacheBackend's key formatting logic. This creates inconsistency and potential key collisions.
Fix: Use the RedisCacheBackend's getFullKey method to properly format keys with appropriate namespaces.
Impact: Ensures consistent key formatting across the cache system and prevents potential collisions.
…aming settings to conf and handle reloading the conf file during runtime
There was a problem hiding this comment.
Identified a critical stability issue in the new exception handlers that could leave the application in a corrupt state. Also provided suggestions to improve configuration loading robustness and general code quality.
Skipped files
conf.example.json: File hunk diff too large
| process.on('uncaughtException', (err) => { | ||
| console.error('Unhandled exception', err); | ||
| }); | ||
|
|
||
| process.on('unhandledRejection', (err) => { | ||
| console.error('Unhandled rejection', err); | ||
| }); |
There was a problem hiding this comment.
🔴 Stability Issue
Issue: The current implementation for uncaughtException and unhandledRejection only logs the error and allows the process to continue. This is dangerous, as the application is left in an undefined and potentially corrupt state after such an error, which can lead to unpredictable behavior, data loss, or security vulnerabilities.
Fix: The process must be terminated after an uncaught exception or unhandled rejection. The handlers should log the error and then call process.exit(1).
Impact: Prevents the application from running in a compromised state, ensuring stability and reliability in production environments.
| process.on('uncaughtException', (err) => { | |
| console.error('Unhandled exception', err); | |
| }); | |
| process.on('unhandledRejection', (err) => { | |
| console.error('Unhandled rejection', err); | |
| }); | |
| process.on('uncaughtException', (err) => { | |
| console.error('Unhandled exception', err); | |
| process.exit(1); | |
| }); | |
| process.on('unhandledRejection', (err) => { | |
| console.error('Unhandled rejection', err); | |
| process.exit(1); | |
| }); |
| console.log( | ||
| 'WARNING: unable to load settings from your conf.json file', | ||
| error | ||
| ); |
There was a problem hiding this comment.
🔵 Code Quality
Issue: A warning about failing to load configuration is logged using console.log. Semantically, console.warn is the correct function for warning messages.
Fix: Change console.log to console.warn to correctly classify the log message.
Impact: Improves log semantics, which helps in filtering, monitoring, and analyzing logs, especially when using structured logging systems.
| console.log( | |
| 'WARNING: unable to load settings from your conf.json file', | |
| error | |
| ); | |
| console.warn( | |
| 'WARNING: unable to load settings from your conf.json file', | |
| error | |
| ); |
1 participant
No description provided.