Per-identifier locking for the blocking queue rate limiter by danieldoglas · Pull Request #2709 · Expensify/Bedrock

danieldoglas · 2026-08-05T15:52:42Z

Details

This PR changes how we handle the blocking commit thread rate limits.

The logic we applied here is:

We have a 180s time window
Each command can spend up to 40s in the blocking queue during that period
Each accountID can spend up to 20s in the blocking queue during that period
If a command or accountID reach their limits, we will block them from going to the blocking queue for 60s.

Every time a command finishes in the blocking queue, we will:

add the time spent in that command in _commandStates, keyed by command name
add the time spent for that account in _accountStates, keyed by accountID

The StateMap object has a mutex that is briefly held while we retrieve an entry from the map.

The entry in the map is an IdentifierState, which also has a mutex, a blockedUntil property (so the checkes when we're inserting the commands to the blocking queue is O(1)) and a list of <startTime, timeSpent> for commands of that identifier, which we'll use to calculate how long they spent in the blocking queue in the last 180s every time a command is finished.

So the max we'll hold a mutex for someone is if they have several commands going to the blocking queue, or if we have the same command going to the blocking queue too much. But if that happens, there's a high chance we will block them either way.

Fixed Issues

For https://github.com/Expensify/Expensify/issues/662853

Tests

Compiles clean; existing unit + cluster tests unaffected (no behavior change yet).

Internal Testing Reminder: when changing bedrock, please compile auth against your new changes

…fier-locking

danieldoglas · 2026-08-07T14:43:15Z

+uint64_t BedrockBlockingCommandQueue::_now() const
 {
-    const string identifier = command->blockingQueueRateLimitIdentifier;
+    return STimeNow();
+}


This is here so we can implement automated tests

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1f8eca1c02

ℹ️ 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".

chatgpt-codex-connector · 2026-08-07T14:46:38Z

+    // Hot path: called by push() and by _dequeue() under the base `_queueMutex`. Keep it O(1) by reading only
+    // the precomputed block deadline. The windowed time is summed in recordExecutionTime, off the blocking thread.
+    const uint64_t now = _now();
+    return _isBlocked(_accountStates, accountID, now) || _isBlocked(_commandStates, commandName, now);


Respect disabled thresholds when checking active blocks

When a control command sets AccountThresholdMs or CommandThresholdMs to 0 after an identifier has already been blocked, new traffic for that account/command is still rejected until the old blockedUntil deadline because this check only consults the cached block state and not the current threshold. That contradicts the new “threshold of 0 disables that dimension” behavior and makes the documented emergency disable/reset path ineffective unless operators also know to send ClearBlocks.

Useful? React with 👍 / 👎.

…only branch, blocked-identifier status, MS naming

tylerkaraszewski · 2026-08-07T22:09:51Z

-                response["previousMaxBlockingQueueTimePerIdentifierMs"] = to_string(previousUS / 1000);
-                SINFO("Setting blocking queue max time per identifier to " << maxTimeMs << "ms");
+        if (command->request.isSet("WindowMS")) {
+            int64_t windowMS = command->request.calc64("WindowMS");


You can collapse the above two lines into just int64_t windowMS = command->request.calc64("WindowMS");

it will end up 0 if it's not set. Similar below.

I don't think we want that. If you pass identifierThresholdMS or commandThresholdMS as 0, that means we're not gonna check for those anymore, which means disabling the rate limit. I think we want to allow disabling it, so having the check if the property exists makes sense.

tylerkaraszewski · 2026-08-07T22:21:04Z

+    // locking each entry to read `blockedUntil`; no code path takes an entry lock before the map mutex, so this
+    // can't deadlock.
+    auto inspect = [now](StateMap& map, size_t& tracked, list<string>& blocked) {
+        lock_guard<decltype(map.mapMutex)> lock(map.mapMutex);


I don't really like this lock wrapping a full walk of the entire map, even more so since it waits for a lock on every single item in the map. It seems like it's only used in Status so maybe performance wise it's not that big of a deal.

However, the two locks are worth considering. Are you sure there's no way to cause a deadlock with this?

I don't see how we would cause a deadlock here... maybe you're seeing something I'm not seeing?

getState only uses the mapMutex, same as clearRateLimits. The other places always use mapMutex, then the state mutex in that order, so I don't think a deadlock would happen.

flodnv · 2026-08-10T12:54:45Z

+void BedrockBlockingCommandQueue::push(unique_ptr<BedrockCommand>&& command)
+{
+    if (isBlocked(command->blockingQueueRateLimitIdentifier, command->request.methodLine)) {
        STHROW("503 Blocking queue rate limited (time)");


NAB: Auth uses 429 Too Many Requests for something very similar, maybe we should standardize these.

flodnv · 2026-08-10T12:57:28Z

@@ -2097,12 +2099,36 @@ void BedrockServer::_control(unique_ptr<BedrockCommand>& command)
    } else if (SIEquals(command->request.methodLine, "SetConflictPageLocks")) {
        _enableConflictPageLocks = command->request.test("enable");
    } else if (SIEquals(command->request.methodLine, "SetBlockingQueueTimeRateLimit")) {


Please add this to Step 4 of https://stackoverflowteams.com/c/expensify/questions/17450/17451#17451

Will do once we approve this and there are not more changes

flodnv · 2026-08-10T13:14:57Z

+            it = state->commands.erase(it);
+            continue;
+        }
+        total += min(it->elapsedTime, windowUS - age);


I don't understand this. Why not total += it->elapsedTime?

added a comment

let me know if it's clearer or not

Co-authored-by: Florent De'Neve <florent@expensify.com>

Takes a `commitCount` and returns the journal hash for that commit. Reads through the DB pool the same way the Status command does, so it doesn't need the sync node. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Covers argument validation, that the returned hash matches what's actually in the journal, and that the command is allowed from a non-localhost source while other control commands aren't. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

We'll verify this one by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Co-authored-by: Florent De'Neve <florent@expensify.com>

…fier-locking

melvin-bot Bot assigned danieldoglas Aug 5, 2026

Base automatically changed from danielsilva-662853-remove-count-rate-limit to main August 6, 2026 19:42

danieldoglas added 4 commits August 6, 2026 16:44

add per-identifier state container for rate limiting

df1e7b6

tighten per-identifier state comments

6476b6a

store finished blocking-queue commands per identifier

4ce9c0f

wip

be0c03a

danieldoglas force-pushed the danielsilva-662853-blocking-queue-per-identifier-locking branch from 462ed60 to be0c03a Compare August 6, 2026 19:44

danieldoglas added 5 commits August 6, 2026 17:03

Prune redundant comments from blocking-queue rate limiter

fcfbc0f

changign logic to return the blocked accounts/commands

4c5c580

whitelisting new log params

5c785d0

adjusting tests

eceeb39

Merge branch 'main' into danielsilva-662853-blocking-queue-per-identi…

1f8eca1

…fier-locking

danieldoglas marked this pull request as ready for review August 7, 2026 14:41

danieldoglas requested review from flodnv and tylerkaraszewski August 7, 2026 14:42

danieldoglas commented Aug 7, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Aug 7, 2026

View reviewed changes

Refine blocking-queue limiter: string dimension, structured log, log-…

bc8f8a1

…only branch, blocked-identifier status, MS naming

tylerkaraszewski requested changes Aug 7, 2026

View reviewed changes

flodnv requested changes Aug 10, 2026

View reviewed changes

danieldoglas and others added 10 commits August 10, 2026 14:03

Apply suggestions from code review

0111cd4

Co-authored-by: Florent De'Neve <florent@expensify.com>

Add a GetCommitHash control command

6310713

Takes a `commitCount` and returns the journal hash for that commit. Reads through the DB pool the same way the Status command does, so it doesn't need the sync node. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Allow GetCommitHash from hosts other than localhost

6d692ac

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Add tests for GetCommitHash

28bc565

Covers argument validation, that the returned hash matches what's actually in the journal, and that the command is allowed from a non-localhost source while other control commands aren't. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Drop GetCommitHashTest

1654f3c

We'll verify this one by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

addressing comments

a526b45

addressing more comments

14159d0

more comments

385bb18

renaming tests from accounts to identifiers

d98d15c

wip

99b647a

danieldoglas added 2 commits August 10, 2026 14:36

wip

3e955cb

fix tests

4cd3548

danieldoglas requested review from flodnv and tylerkaraszewski August 11, 2026 00:27

flodnv reviewed Aug 11, 2026

View reviewed changes

Comment thread BedrockServer.cpp

Comment thread BedrockServer.cpp Outdated

danieldoglas and others added 3 commits August 11, 2026 14:56

Apply suggestion from @flodnv

8a71cd4

Co-authored-by: Florent De'Neve <florent@expensify.com>

fixing comment

ee8b88d

Merge branch 'main' into danielsilva-662853-blocking-queue-per-identi…

8f05d57

…fier-locking

danieldoglas requested a review from flodnv August 11, 2026 17:59

style

ef2db5a

quinthar added the #stability label Aug 11, 2026

tylerkaraszewski approved these changes Aug 11, 2026

View reviewed changes

flodnv approved these changes Aug 12, 2026

View reviewed changes

flodnv merged commit 9b71d8c into main Aug 12, 2026
9 of 11 checks passed

flodnv deleted the danielsilva-662853-blocking-queue-per-identifier-locking branch August 12, 2026 09:53

Uh oh!

Conversation

danieldoglas commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danieldoglas commented Aug 5, 2026 •

edited

Loading