Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 5 additions & 13 deletions lib/Config.js
Original file line number Diff line number Diff line change
Expand Up @@ -1860,23 +1860,15 @@ class Config extends EventEmitter {
this.enableVeeamRoute = config.enableVeeamRoute;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseRateLimitConfig is now called unconditionally, but serviceUserArn is Joi.string().required() in the schema. If a user has { rateLimiting: { enabled: false } } without serviceUserArn, this will now throw. Previously it worked because parseRateLimitConfig was only called when enabled: true.

Fix: in config.js line 178, make serviceUserArn conditionally required:
serviceUserArn: Joi.string().when('enabled', { is: true, then: Joi.required() })

— Claude Code

this.rateLimiting = {
enabled: false,
bucket: {
configCacheTTL: constants.rateLimitDefaultConfigCacheTTL,
},
};
// Parse and validate all rate limiting configuration
this.rateLimiting = parseRateLimitConfig(config.rateLimiting);

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);
Expand Down
12 changes: 12 additions & 0 deletions lib/api/apiUtils/rateLimit/cache.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
const configCache = new Map();
const bucketOwnerCache = new Map();

const namespace = {
bucket: 'bkt',
account: 'acc',
};

function cacheSet(cache, key, value, ttl) {
Expand Down Expand Up @@ -54,13 +56,23 @@ const setCachedConfig = formatKeyDecorator(cacheSet.bind(null, configCache));
const deleteCachedConfig = formatKeyDecorator(cacheDelete.bind(null, configCache));
const expireCachedConfigs = cacheExpire.bind(null, configCache);

const getCachedBucketOwner = cacheGet.bind(null, bucketOwnerCache);
const setCachedBucketOwner = cacheSet.bind(null, bucketOwnerCache);
const deleteCachedBucketOwner = cacheDelete.bind(null, bucketOwnerCache);
const expireCachedBucketOwners = cacheExpire.bind(null, bucketOwnerCache);

module.exports = {
namespace,
setCachedConfig,
getCachedConfig,
expireCachedConfigs,
deleteCachedConfig,
setCachedBucketOwner,
getCachedBucketOwner,
deleteCachedBucketOwner,
expireCachedBucketOwners,
// Do not access directly
// Used only for tests
configCache,
bucketOwnerCache,
};
37 changes: 23 additions & 14 deletions lib/api/apiUtils/rateLimit/cleanup.js
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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanupJob recursively calls itself without passing options, so skipUnref is lost after the first run. In production this is harmless (options is always empty), but in tests with skipUnref: true the second and subsequent timeouts will call unref() unexpectedly.

Pass options through: setTimeout(() => cleanupJob(log, options), rateLimitCleanupInterval)

— Claude Code

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;
}
Expand All @@ -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();
}
}

Expand All @@ -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');
}
Expand Down
48 changes: 30 additions & 18 deletions lib/api/apiUtils/rateLimit/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -191,6 +190,24 @@ const rateLimitConfigSchema = Joi.object({
code: errors.SlowDown.message,
message: errors.SlowDown.description,
}),
}).default({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The serviceUserArn is required but doesn't appear in the global default value, is it wanted?

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,
},
});

/**
Expand All @@ -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) {
Expand All @@ -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,
};
}

/**
Expand Down
Loading
Loading