-
Notifications
You must be signed in to change notification settings - Fork 254
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
base: improvement/CLDSRV-869/account_limiting
Are you sure you want to change the base?
Changes from all commits
a0d525e
7e90884
dc81a10
6cd9bba
71304fa
54d0611
cc11c7f
b0b9212
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 |
|---|---|---|
| @@ -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), rateLimitCleanupInterval); | ||
tmacro marked this conversation as resolved.
Show resolved
Hide resolved
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.
|
||
| if (!options.skipUnref) { | ||
| cleanupTimer.unref(); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * 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