CLDSRV-855: Update helpers#6128
Open
tmacro wants to merge 8 commits intoimprovement/CLDSRV-869/account_limitingfrom
Open
CLDSRV-855: Update helpers#6128tmacro wants to merge 8 commits intoimprovement/CLDSRV-869/account_limitingfrom
tmacro wants to merge 8 commits intoimprovement/CLDSRV-869/account_limitingfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review by Claude Code |
❌ 105 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
tcarmet
approved these changes
Apr 1, 2026
lib/Config.js
Outdated
|
|
||
| // 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 (config.rateLimiting?.enabled) { |
Contributor
There was a problem hiding this comment.
Any reason why we don't do something like this now?
Suggested change
| if (config.rateLimiting?.enabled) { | |
| if (this.rateLimiting.enabled) { |
|
LGTM |
| code: errors.SlowDown.message, | ||
| message: errors.SlowDown.description, | ||
| }), | ||
| }).default({ |
Contributor
There was a problem hiding this comment.
The serviceUserArn is required but doesn't appear in the global default value, is it wanted?
Comment on lines
-64
to
-66
| const applyRateLimit = config.rateLimiting?.enabled | ||
| && !rateLimitApiActions.includes(request.apiMethod) // Don't limit any rate limit admin actions | ||
| && !request.isInternalServiceRequest; // Don't limit any calls from internal services |
Contributor
There was a problem hiding this comment.
This seems to be done by requestNeedsRateCheck now, but extractAccountRateLimitConfig doesn't call this function, is it expected?
|
|
||
| function buildRateChecksFromConfig(resourceClass, resourceId, limitConfig) { | ||
| const checks = []; | ||
| if (limitConfig?.RequestsPerSecond?.Limit !== undefined && limitConfig.RequestsPerSecond?.Limit > 0) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if (limitConfig?.RequestsPerSecond?.Limit !== undefined && limitConfig.RequestsPerSecond?.Limit > 0) { | |
| if (limitConfig?.RequestsPerSecond?.Limit > 0) { |
BourgoisMickael
approved these changes
Apr 2, 2026
| @@ -1860,23 +1860,15 @@ class Config extends EventEmitter { | |||
| this.enableVeeamRoute = config.enableVeeamRoute; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
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
| log.debug('Rate limit cleanup completed', { expiredConfigs, expiredBucketOwners }); | ||
| } | ||
|
|
||
| cleanupTimer = setTimeout(() => cleanupJob(log), rateLimitCleanupInterval); |
There was a problem hiding this comment.
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
Review by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.