ref(service): Replace upfront batch reservation with per-op bulk permits#563
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #563 +/- ##
==========================================
+ Coverage 87.55% 87.67% +0.12%
==========================================
Files 93 93
Lines 14969 15113 +144
==========================================
+ Hits 13106 13251 +145
+ Misses 1863 1862 -1
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 21d1d0d. Configure here.
lcian
left a comment
There was a problem hiding this comment.
Looks good to me after addressing comments, feel free to merge and deploy, as we discussed this extensively and tested it in sandbox.
| /// `1s` | ||
| #[serde(with = "humantime_serde")] | ||
| pub concurrency_queue_timeout: Duration, | ||
| pub concurrency_timeout: Duration, |
There was a problem hiding this comment.
This is technically a breaking change. We cannot use serde(alias) here, as this breaks with figment.
|
I've changed the implementation for regular permits now, so that the queue semaphore just covers the wait: it's only acquired while acquiring the actual task permit, and then immediately released. Previously it wrapped the lifetime of the task permit, which broke with bulk requests not holding a queue permit. The new approach comes with different behavior: It isolates waiting bulk tasks (i.e. batch requests, but later maybe background tasks) from queued requests. Also, even with queue configured at I'm a bit torn about this - the previous wrapping approach seemed cleaner to me, although bypassing the queue is the more future-proof behavior. |

The streaming executor previously reserved a bulk window of permits upfront. This PR replaces that model with per-operation permit acquisition through a new bulk concurrency class on the limiter.
Each streaming operation now individually acquires a bulk permit via
acquire_bulk, which waits on two semaphores sequentially:Note that this purposefully does not acquire the queue permit as to not take away queueing slots for regular requests.
Permits are acquired sequentially, and only operations with permit are parallelized. This ensures fairness between concurrent batch requests and prevents a large batch from racing for all permits at once.
The bulk budget sets a safe operating point: below this level, there should be little-to-no performance degradation, leaving room for more tasks to be admitted via the queue before rejection is necessary. In config this is defaulted to 60% of
max_concurrency.See also #562
Ref FS-447