-
Notifications
You must be signed in to change notification settings - Fork 7
Share concurrent request limits across the fleet #975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 8 commits
5cff7e2
07a09c3
389ff68
76ae48a
f783811
6eca5fd
dcb326d
272e2ea
a8d35b9
506dd41
bfe5512
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -131,6 +131,7 @@ pub fn test_config() -> ApiConfig { | |||||||||||||
| ..config::UsageReportingConfig::default() | ||||||||||||||
| }, | ||||||||||||||
| ita: config::ItaAttestationConfig::default(), | ||||||||||||||
| fleet_concurrency: config::FleetConcurrencyConfig::default(), | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -434,6 +435,36 @@ where | |||||||||||||
| (server, database) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// Independent servers sharing one database, standing in for fleet replicas. | ||||||||||||||
| /// Each gets its own services, so agreement has to travel through the database. | ||||||||||||||
| pub async fn setup_test_fleet<F>( | ||||||||||||||
| instances: usize, | ||||||||||||||
| mutate: F, | ||||||||||||||
| ) -> ( | ||||||||||||||
| Vec<axum_test::TestServer>, | ||||||||||||||
| Vec<Arc<inference_providers::mock::MockProvider>>, | ||||||||||||||
| Arc<Database>, | ||||||||||||||
| ) | ||||||||||||||
| where | ||||||||||||||
| F: Fn(&mut config::ApiConfig), | ||||||||||||||
| { | ||||||||||||||
|
Comment on lines
+454
to
+456
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
Suggestion:
Suggested change
|
||||||||||||||
| let infra = setup_test_infrastructure().await; | ||||||||||||||
| let database = infra.database.clone(); | ||||||||||||||
|
|
||||||||||||||
| let mut servers = Vec::with_capacity(instances); | ||||||||||||||
| let mut mocks = Vec::with_capacity(instances); | ||||||||||||||
| for _ in 0..instances { | ||||||||||||||
| let mut config = infra.config.clone(); | ||||||||||||||
| mutate(&mut config); | ||||||||||||||
| let (server, _pool, mock, _router) = | ||||||||||||||
| build_test_server_components(database.clone(), config).await; | ||||||||||||||
| servers.push(server); | ||||||||||||||
| mocks.push(mock); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| (servers, mocks, database) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| pub async fn setup_test_server_with_database() -> (axum_test::TestServer, Arc<Database>) { | ||||||||||||||
| let (server, _, _, database) = setup_test_server_with_pool().await; | ||||||||||||||
| (server, database) | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| use crate::common; | ||
| use inference_providers::mock::ResponseTemplate; | ||
| use std::time::Duration; | ||
| use uuid::Uuid; | ||
|
|
||
| const INSTANCES: usize = 4; | ||
| const LIMIT: i32 = 2; | ||
| const ATTEMPTS: usize = 8; | ||
| const HOLD: Duration = Duration::from_secs(2); | ||
| const MODEL: &str = "Qwen/Qwen3-30B-A3B-Instruct-2507"; | ||
|
|
||
| async fn set_rate_limit(database: &database::Database, organization_id: &str, limit: i32) { | ||
| let organization_id = Uuid::parse_str(organization_id).expect("organization id is a uuid"); | ||
| database | ||
| .pool() | ||
| .get() | ||
| .await | ||
| .expect("database connection") | ||
| .execute( | ||
| "UPDATE organizations SET rate_limit = $1 WHERE id = $2", | ||
| &[&limit, &organization_id], | ||
| ) | ||
| .await | ||
| .expect("rate limit update"); | ||
| } | ||
|
|
||
| async fn live_lease_count(database: &database::Database, organization_id: &str) -> i64 { | ||
| let organization_id = Uuid::parse_str(organization_id).expect("organization id is a uuid"); | ||
| database | ||
| .pool() | ||
| .get() | ||
| .await | ||
| .expect("database connection") | ||
| .query_one( | ||
| "SELECT COUNT(*) FROM concurrency_leases | ||
| WHERE organization_id = $1 AND expires_at > NOW()", | ||
| &[&organization_id], | ||
| ) | ||
| .await | ||
| .expect("lease count") | ||
| .get(0) | ||
| } | ||
|
|
||
| /// Four replicas sharing one database must not admit more than the | ||
| /// organization's limit between them. Without fleet-wide accounting each | ||
| /// replica counts only itself, so this admits INSTANCES * LIMIT. | ||
| #[tokio::test] | ||
| async fn fleet_limit_is_shared_across_replicas() { | ||
| let (servers, mocks, database) = common::setup_test_fleet(INSTANCES, |config| { | ||
| config.fleet_concurrency.mode = config::FleetConcurrencyMode::Enforce; | ||
| }) | ||
| .await; | ||
|
|
||
| for mock in &mocks { | ||
| mock.set_default_response(ResponseTemplate::new("held").with_hold(HOLD)) | ||
| .await; | ||
| } | ||
|
|
||
| let org = common::setup_org_with_credits(&servers[0], 100_000_000_000).await; | ||
| let api_key = common::get_api_key_for_org(&servers[0], org.id.clone()).await; | ||
| set_rate_limit(&database, &org.id, LIMIT).await; | ||
|
|
||
| let attempts = (0..ATTEMPTS).map(|attempt| { | ||
| let server = &servers[attempt % INSTANCES]; | ||
| let api_key = api_key.clone(); | ||
| async move { | ||
| server | ||
| .post("/v1/chat/completions") | ||
| .add_header("Authorization", format!("Bearer {api_key}")) | ||
| .json(&serde_json::json!({ | ||
| "model": MODEL, | ||
| "messages": [{ "role": "user", "content": "hello" }] | ||
| })) | ||
| .await | ||
| .status_code() | ||
| .as_u16() | ||
| } | ||
| }); | ||
|
|
||
| let statuses = futures::future::join_all(attempts).await; | ||
| let admitted = statuses.iter().filter(|status| **status == 200).count(); | ||
| let rejected = statuses.iter().filter(|status| **status == 429).count(); | ||
|
|
||
| assert_eq!( | ||
| admitted, LIMIT as usize, | ||
| "expected the fleet to admit exactly {LIMIT} of {ATTEMPTS}, got {statuses:?}" | ||
| ); | ||
| assert_eq!( | ||
| rejected, | ||
| ATTEMPTS - LIMIT as usize, | ||
| "every attempt over the limit should be rejected, got {statuses:?}" | ||
| ); | ||
| } | ||
|
|
||
| /// A finished request must give its slot back, or the organization stays | ||
| /// locked out until the leases expire. | ||
| #[tokio::test] | ||
| async fn completed_requests_release_their_lease() { | ||
| let (servers, mocks, database) = common::setup_test_fleet(1, |config| { | ||
| config.fleet_concurrency.mode = config::FleetConcurrencyMode::Enforce; | ||
| }) | ||
| .await; | ||
|
|
||
| mocks[0] | ||
| .set_default_response(ResponseTemplate::new("done")) | ||
| .await; | ||
|
|
||
| let org = common::setup_org_with_credits(&servers[0], 100_000_000_000).await; | ||
| let api_key = common::get_api_key_for_org(&servers[0], org.id.clone()).await; | ||
| set_rate_limit(&database, &org.id, LIMIT).await; | ||
|
|
||
| for _ in 0..(ATTEMPTS as i32 + LIMIT) { | ||
| let status = servers[0] | ||
| .post("/v1/chat/completions") | ||
| .add_header("Authorization", format!("Bearer {api_key}")) | ||
| .json(&serde_json::json!({ | ||
| "model": MODEL, | ||
| "messages": [{ "role": "user", "content": "hello" }] | ||
| })) | ||
| .await | ||
| .status_code(); | ||
| assert_eq!( | ||
| status, 200, | ||
| "a released slot should let the next request straight through" | ||
| ); | ||
| } | ||
|
|
||
| tokio::time::sleep(Duration::from_millis(500)).await; | ||
| assert_eq!( | ||
| live_lease_count(&database, &org.id).await, | ||
| 0, | ||
| "leases outlived the requests that took them" | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,16 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| -- One row per in-flight request, so replicas count against a shared limit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| -- rather than one each. Rows outlive a dead replica and are swept on expiry. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CREATE TABLE concurrency_leases ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id UUID PRIMARY KEY, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| organization_id UUID NOT NULL REFERENCES organizations(id) ON DELETE CASCADE, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| model_id UUID NOT NULL REFERENCES models(id) ON DELETE CASCADE, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| instance_id TEXT NOT NULL, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| acquired_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expires_at TIMESTAMPTZ NOT NULL | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The migration lacks a descriptive header comment explaining the table's purpose. Other recent migrations (e.g. V0074) include a brief comment. Adding one here would help future maintainers understand that this table tracks fleet-wide concurrency leases for rate-limit enforcement across replicas. Suggestion:
Suggested change
Comment on lines
+8
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a CHECK constraint (expires_at > acquired_at) as defense-in-depth. The application code always sets expires_at = NOW() + positive TTL, so the invariant holds today, but a database-level constraint would prevent accidental insertion of already-expired leases and make the intent explicit. Suggestion:
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CREATE INDEX idx_concurrency_leases_org_model | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ON concurrency_leases (organization_id, model_id, expires_at); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CREATE INDEX idx_concurrency_leases_expires_at | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ON concurrency_leases (expires_at); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Medium · Do not use a single shadow replica as fleet-wide rollout evidence
Instances left in
offmode never createFleetConcurrency, so they continue using only local counters and write no leases. Consequently, the documented one-instance shadow rollout measures only that instance, and enabling enforce one instance at a time still ignores traffic on off replicas. Require all replicas to write shadow leases before relying on the metric or performing a gradual enforce rollout.