Add blocking queue rate limit - #2555
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a2a67d343
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 836f8a0f3a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
This is my first bedrock PR ever, let me know if anything needs to be changed! I realize that a lot of the comments are a mixture of "what is happening" and "why is it happening", but it made it a lot more clear to me. Happy to remove them if they are unnecessary to people more familiar with the codebase |
|
I see this code uses |
…and control params
…ingQueueRateLimit
|
Okay this is ready for review again! The merge conflicts were due to a pretty big structure change, so things had to be moved around. I disabled the actual throw and left the log so that we can test before actually enabling |
|
UPdated with suggestions, but tests are failing now (we just don't see it here because we disabled the tests since we're only logging) |
flodnv
left a comment
flodnv
left a comment
There was a problem hiding this comment.
Thanks for this, I'm eager to see it in action!
|
Okay we should be good to go here! |
flodnv
left a comment
flodnv
left a comment
There was a problem hiding this comment.
All yours @tylerkaraszewski
| } | ||
|
|
||
| count++; | ||
| if (count > maxPerIdentifier) { |
There was a problem hiding this comment.
Wait what, this if (count > maxPerIdentifier) { is the same as line 36 😅
There was a problem hiding this comment.
lol oops, looks like copy/pasta to me, or maybe a relic of when there were 2 different identifiers. PR to de-duplicate here #2615
There was a problem hiding this comment.
I think I need another change there, please hold
There was a problem hiding this comment.
Okay it's ready now, there was another error in the same spot
5 participants
Details
This PR blocks users from adding to the blocking queue if they already have too many commands in the queue. This prevents a single user from being able to DOS us (intentionally or on accident).
Once the queue is empty for 30 seconds, we reset the block list and allow those users to continue using the product per usual
Fixed Issues
https://github.com/Expensify/Expensify/issues/568969
Tests
Added automated tests
Internal Testing Reminder: when changing bedrock, please compile auth against your new changes