-
Notifications
You must be signed in to change notification settings - Fork 255
CLDSRV-855: Update helpers #6128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bf3a451
75266b5
4f3352e
185e209
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1860,23 +1860,15 @@ class Config extends EventEmitter { | |
| this.enableVeeamRoute = config.enableVeeamRoute; | ||
| } | ||
|
|
||
| this.rateLimiting = { | ||
| enabled: false, | ||
| bucket: { | ||
| configCacheTTL: constants.rateLimitDefaultConfigCacheTTL, | ||
| }, | ||
| }; | ||
| // Parse and validate all rate limiting configuration | ||
| this.rateLimiting = parseRateLimitConfig(config.rateLimiting); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Backward compatibility issue: |
||
|
|
||
| if (config.rateLimiting?.enabled) { | ||
| // rate limiting uses the same localCache config defined for S3 to avoid | ||
| // config duplication. | ||
| // Rate limiting uses the same localCache config defined for S3 to avoid config duplication. | ||
| // If rate limiting is enabled check to make sure it is also configured. | ||
| if (this.rateLimiting.enabled) { | ||
| assert(this.localCache, 'localCache must be defined when rate limiting is enabled'); | ||
|
|
||
| // Parse and validate all rate limiting configuration | ||
| this.rateLimiting = parseRateLimitConfig(config.rateLimiting); | ||
| } | ||
|
|
||
|
|
||
| if (config.capabilities) { | ||
| if (config.capabilities.locationTypes) { | ||
| this.supportedLocationTypes = new Set(config.capabilities.locationTypes); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,34 @@ | ||
| const { expireCachedConfigs } = require('./cache'); | ||
| const { expireCachedConfigs, expireCachedBucketOwners } = require('./cache'); | ||
| const { rateLimitCleanupInterval } = require('../../../../constants'); | ||
|
|
||
| let cleanupInterval = null; | ||
| let cleanupTimer = null; | ||
|
|
||
| function cleanupJob(log, options = {}) { | ||
| const expiredConfigs = expireCachedConfigs(); | ||
| const expiredBucketOwners = expireCachedBucketOwners(); | ||
| if (expiredConfigs > 0 || expiredBucketOwners > 0) { | ||
| log.debug('Rate limit cleanup completed', { expiredConfigs, expiredBucketOwners }); | ||
| } | ||
|
|
||
| cleanupTimer = setTimeout(() => cleanupJob(log, options), rateLimitCleanupInterval); | ||
| if (!options.skipUnref) { | ||
| cleanupTimer.unref(); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switching from |
||
|
|
||
|
|
||
| /** | ||
| * Start periodic cleanup of expired rate limit counters and cached configs | ||
| * | ||
| * Runs every 10 seconds to: | ||
| * - Remove expired GCRA counters (where emptyAt <= now) | ||
| * - Remove expired cached rate limit configs (where TTL expired) | ||
| * - Remove expired cached bucket owners (where TTL expired) | ||
| * | ||
| * This prevents memory leaks from accumulating expired entries. | ||
| */ | ||
| function startCleanupJob(log, options = {}) { | ||
| if (cleanupInterval) { | ||
| if (cleanupTimer) { | ||
| log.warn('Rate limit cleanup job already running'); | ||
| return; | ||
| } | ||
|
|
@@ -22,18 +37,12 @@ function startCleanupJob(log, options = {}) { | |
| interval: rateLimitCleanupInterval, | ||
| }); | ||
|
|
||
| cleanupInterval = setInterval(() => { | ||
| const now = Date.now(); | ||
| const expiredConfigs = expireCachedConfigs(now); | ||
| if (expiredConfigs > 0) { | ||
| log.debug('Rate limit cleanup completed', { expiredConfigs }); | ||
| } | ||
| }, rateLimitCleanupInterval); | ||
| cleanupTimer = setTimeout(() => cleanupJob(log, options), rateLimitCleanupInterval); | ||
|
|
||
| // Prevent cleanup job from keeping process alive | ||
| // Skip unref() in test environment to work with sinon fake timers | ||
| if (!options.skipUnref) { | ||
| cleanupInterval.unref(); | ||
| cleanupTimer.unref(); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -42,9 +51,9 @@ function startCleanupJob(log, options = {}) { | |
| * Used for graceful shutdown or testing | ||
| */ | ||
| function stopCleanupJob(log) { | ||
| if (cleanupInterval) { | ||
| clearInterval(cleanupInterval); | ||
| cleanupInterval = null; | ||
| if (cleanupTimer !== null) { | ||
| clearTimeout(cleanupTimer); | ||
| cleanupTimer = null; | ||
| if (log) { | ||
| log.info('Stopped rate limit cleanup job'); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ const Joi = require('@hapi/joi'); | |
| const { errors, ArsenalError } = require('arsenal'); | ||
|
|
||
| const { rateLimitDefaultConfigCacheTTL, rateLimitDefaultBurstCapacity } = require('../../../../constants'); | ||
| const { calculateInterval } = require('./gcra'); | ||
|
|
||
| /** | ||
| * Full Rate Limiting Configuration Example | ||
|
|
@@ -191,6 +190,24 @@ const rateLimitConfigSchema = Joi.object({ | |
| code: errors.SlowDown.message, | ||
| message: errors.SlowDown.description, | ||
| }), | ||
| }).default({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| enabled: false, | ||
| nodes: 1, | ||
| tokenBucketBufferSize: 50, | ||
| tokenBucketRefillThreshold: 20, | ||
| bucket: { | ||
| configCacheTTL: rateLimitDefaultConfigCacheTTL, | ||
| defaultBurstCapacity: rateLimitDefaultBurstCapacity, | ||
| }, | ||
| account: { | ||
| configCacheTTL: rateLimitDefaultConfigCacheTTL, | ||
| defaultBurstCapacity: rateLimitDefaultBurstCapacity, | ||
| }, | ||
| error: { | ||
| statusCode: 503, | ||
| code: errors.SlowDown.message, | ||
| message: errors.SlowDown.description, | ||
| }, | ||
| }); | ||
|
|
||
| /** | ||
|
|
@@ -203,10 +220,10 @@ const rateLimitConfigSchema = Joi.object({ | |
| * @returns {RateLimitClassConfig} Transformed rate limit config | ||
| */ | ||
| function transformClassConfig(resourceClass, validatedCfg, nodes) { | ||
| const transformed = { | ||
| defaultConfig: undefined, | ||
| configCacheTTL: validatedCfg.configCacheTTL, | ||
| defaultBurstCapacity: validatedCfg.defaultBurstCapacity, | ||
| const defaultConfig = { | ||
| RequestsPerSecond: { | ||
| BurstCapacity: validatedCfg.defaultBurstCapacity, | ||
| }, | ||
| }; | ||
|
|
||
| if (validatedCfg.defaultConfig?.requestsPerSecond) { | ||
|
|
@@ -223,23 +240,18 @@ function transformClassConfig(resourceClass, validatedCfg, nodes) { | |
| ); | ||
| } | ||
|
|
||
| // Use provided burstCapacity or fall back to default | ||
| const effectiveBurstCapacity = burstCapacity || transformed.defaultBurstCapacity; | ||
|
|
||
| // Calculate per-node interval using distributed architecture | ||
| const interval = calculateInterval(limit, nodes); | ||
|
|
||
| // Store both the original limit and the calculated values | ||
| transformed.defaultConfig = { | ||
| limit, | ||
| requestsPerSecond: { | ||
| interval, | ||
| bucketSize: effectiveBurstCapacity * 1000, | ||
| }, | ||
| defaultConfig.RequestsPerSecond = { | ||
| Limit: limit, | ||
| BurstCapacity: burstCapacity || validatedCfg.defaultBurstCapacity, | ||
| }; | ||
| } | ||
|
|
||
| return transformed; | ||
| return { | ||
| defaultConfig, | ||
| configCacheTTL: validatedCfg.configCacheTTL, | ||
| defaultBurstCapacity: validatedCfg.defaultBurstCapacity, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseRateLimitConfigis now called unconditionally, butserviceUserArnisJoi.string().required()in the schema. If a user has{ rateLimiting: { enabled: false } }withoutserviceUserArn, this will now throw. Previously it worked becauseparseRateLimitConfigwas only called whenenabled: true.Fix: in config.js line 178, make serviceUserArn conditionally required:
serviceUserArn: Joi.string().when('enabled', { is: true, then: Joi.required() })— Claude Code