From 2feebe842b9885d6254901df871353201d4b4c4e Mon Sep 17 00:00:00 2001 From: carodewig <16093297+carodewig@users.noreply.github.com> Date: Mon, 15 Dec 2025 17:05:05 -0500 Subject: [PATCH 1/5] fix: return 429 when rate-limited by load_shed --- apollo-router/src/plugins/traffic_shaping/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apollo-router/src/plugins/traffic_shaping/mod.rs b/apollo-router/src/plugins/traffic_shaping/mod.rs index 0d81426495..76c88bc934 100644 --- a/apollo-router/src/plugins/traffic_shaping/mod.rs +++ b/apollo-router/src/plugins/traffic_shaping/mod.rs @@ -342,7 +342,7 @@ impl PluginPrivate for TrafficShaping { .extension_code("REQUEST_RATE_LIMITED") .build(); Ok(RouterResponse::error_builder() - .status_code(StatusCode::SERVICE_UNAVAILABLE) + .status_code(StatusCode::TOO_MANY_REQUESTS) .error(error) .context(ctx) .build() @@ -416,7 +416,7 @@ impl PluginPrivate for TrafficShaping { .extension_code("REQUEST_RATE_LIMITED") .build(); Ok(SubgraphResponse::error_builder() - .status_code(StatusCode::SERVICE_UNAVAILABLE) + .status_code(StatusCode::TOO_MANY_REQUESTS) .subgraph_name(subgraph_name) .error(error) .context(ctx) @@ -1066,7 +1066,7 @@ mod test { .await .expect("it responded"); - assert_eq!(StatusCode::SERVICE_UNAVAILABLE, response.response.status()); + assert_eq!(StatusCode::TOO_MANY_REQUESTS, response.response.status()); tokio::time::sleep(Duration::from_millis(300)).await; @@ -1197,7 +1197,7 @@ mod test { .call(RouterRequest::fake_builder().build().unwrap()) .await .unwrap(); - assert_eq!(StatusCode::SERVICE_UNAVAILABLE, response.response.status()); + assert_eq!(StatusCode::TOO_MANY_REQUESTS, response.response.status()); let j: serde_json::Value = serde_json::from_slice( &router::body::into_bytes(response.response) .await From 1b55dcb8c1752f1681a84dbf9fd7dd0071b4d81b Mon Sep 17 00:00:00 2001 From: carodewig <16093297+carodewig@users.noreply.github.com> Date: Tue, 16 Dec 2025 10:23:54 -0500 Subject: [PATCH 2/5] refactor: emphasize that we're just modifying response conditionally --- .../src/plugins/traffic_shaping/mod.rs | 131 ++++++++---------- 1 file changed, 59 insertions(+), 72 deletions(-) diff --git a/apollo-router/src/plugins/traffic_shaping/mod.rs b/apollo-router/src/plugins/traffic_shaping/mod.rs index 76c88bc934..6295800f74 100644 --- a/apollo-router/src/plugins/traffic_shaping/mod.rs +++ b/apollo-router/src/plugins/traffic_shaping/mod.rs @@ -262,29 +262,22 @@ impl PluginPrivate for TrafficShaping { } fn router_service(&self, service: router::BoxService) -> router::BoxService { + // NB: consider each triplet (map_future_with_request_data, load_shed, layer) as a unit of + // behavior ServiceBuilder::new() .map_future_with_request_data( |req: &router::Request| req.context.clone(), - move |ctx, future| { - async { - let response: Result = future.await; - match response { - Ok(ok) => Ok(ok), - Err(err) if err.is::() => { - // TODO add metrics - let error = graphql::Error::builder() - .message("Your request has been timed out") - .extension_code("GATEWAY_TIMEOUT") - .build(); - Ok(RouterResponse::error_builder() - .status_code(StatusCode::GATEWAY_TIMEOUT) - .error(error) - .context(ctx) - .build() - .expect("should build overloaded response")) - } - Err(err) => Err(err), - } + move |ctx, future| async { + let response: Result = future.await; + if matches!(response, Err(ref err) if err.is::()) { + Ok(RouterResponse::error_builder() + .status_code(StatusCode::GATEWAY_TIMEOUT) + .error(gateway_timeout_error()) + .context(ctx) + .build() + .expect("should build overloaded response")) + } else { + response } }, ) @@ -298,26 +291,17 @@ impl PluginPrivate for TrafficShaping { )) .map_future_with_request_data( |req: &router::Request| req.context.clone(), - move |ctx, future| { - async { - let response: Result = future.await; - match response { - Ok(ok) => Ok(ok), - Err(err) if err.is::() => { - // TODO add metrics - let error = graphql::Error::builder() - .message("Your request has been concurrency limited") - .extension_code("REQUEST_CONCURRENCY_LIMITED") - .build(); - Ok(RouterResponse::error_builder() - .status_code(StatusCode::SERVICE_UNAVAILABLE) - .error(error) - .context(ctx) - .build() - .expect("should build overloaded response")) - } - Err(err) => Err(err), - } + move |ctx, future| async { + let response: Result = future.await; + if matches!(response, Err(ref err) if err.is::()) { + Ok(RouterResponse::error_builder() + .status_code(StatusCode::SERVICE_UNAVAILABLE) + .error(concurrency_limit_error()) + .context(ctx) + .build() + .expect("should build overloaded response")) + } else { + response } }, ) @@ -330,26 +314,17 @@ impl PluginPrivate for TrafficShaping { })) .map_future_with_request_data( |req: &router::Request| req.context.clone(), - move |ctx, future| { - async { - let response: Result = future.await; - match response { - Ok(ok) => Ok(ok), - Err(err) if err.is::() => { - // TODO add metrics - let error = graphql::Error::builder() - .message("Your request has been rate limited") - .extension_code("REQUEST_RATE_LIMITED") - .build(); - Ok(RouterResponse::error_builder() - .status_code(StatusCode::TOO_MANY_REQUESTS) - .error(error) - .context(ctx) - .build() - .expect("should build overloaded response")) - } - Err(err) => Err(err), - } + move |ctx, future| async { + let response: Result = future.await; + if matches!(response, Err(ref err) if err.is::()) { + Ok(RouterResponse::error_builder() + .status_code(StatusCode::TOO_MANY_REQUESTS) + .error(rate_limit_error()) + .context(ctx) + .build() + .expect("should build overloaded response")) + } else { + response } }, ) @@ -395,34 +370,25 @@ impl PluginPrivate for TrafficShaping { async { let response: Result = future.await; match response { - Ok(ok) => Ok(ok), Err(err) if err.is::() => { // TODO add metrics - let error = graphql::Error::builder() - .message("Your request has been timed out") - .extension_code("GATEWAY_TIMEOUT") - .build(); Ok(SubgraphResponse::error_builder() .status_code(StatusCode::GATEWAY_TIMEOUT) .subgraph_name(subgraph_name) - .error(error) + .error(gateway_timeout_error()) .context(ctx) .build()) } Err(err) if err.is::() => { // TODO add metrics - let error = graphql::Error::builder() - .message("Your request has been rate limited") - .extension_code("REQUEST_RATE_LIMITED") - .build(); Ok(SubgraphResponse::error_builder() .status_code(StatusCode::TOO_MANY_REQUESTS) .subgraph_name(subgraph_name) - .error(error) + .error(rate_limit_error()) .context(ctx) .build()) } - Err(err) => Err(err), + _ => response } } }, @@ -566,6 +532,27 @@ impl TrafficShaping { } } +fn concurrency_limit_error() -> graphql::Error { + graphql::Error::builder() + .message("Your request has been concurrency limited") + .extension_code("REQUEST_CONCURRENCY_LIMITED") + .build() +} + +fn gateway_timeout_error() -> graphql::Error { + graphql::Error::builder() + .message("Your request has been timed out") + .extension_code("GATEWAY_TIMEOUT") + .build() +} + +fn rate_limit_error() -> graphql::Error { + graphql::Error::builder() + .message("Your request has been rate limited") + .extension_code("REQUEST_RATE_LIMITED") + .build() +} + register_private_plugin!("apollo", "traffic_shaping", TrafficShaping); #[cfg(test)] From 44a167b26d55aa0ef4fd8f06341696b58dc8cc63 Mon Sep 17 00:00:00 2001 From: carodewig <16093297+carodewig@users.noreply.github.com> Date: Tue, 16 Dec 2025 10:49:52 -0500 Subject: [PATCH 3/5] test: check behavior with ratelimit and timeout combined --- .../src/plugins/traffic_shaping/mod.rs | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/apollo-router/src/plugins/traffic_shaping/mod.rs b/apollo-router/src/plugins/traffic_shaping/mod.rs index 6295800f74..8ecef81504 100644 --- a/apollo-router/src/plugins/traffic_shaping/mod.rs +++ b/apollo-router/src/plugins/traffic_shaping/mod.rs @@ -575,6 +575,8 @@ mod test { use serde_json_bytes::ByteString; use serde_json_bytes::Value; use serde_json_bytes::json; + use tokio::task::JoinSet; + use tokio::time::sleep; use tower::Service; use super::*; @@ -1247,4 +1249,44 @@ mod test { .expect("our body is valid json"); assert_eq!("Your request has been timed out", j["errors"][0]["message"]); } + + #[tokio::test(flavor = "multi_thread")] + async fn it_raises_different_errors_for_timeouts_and_rate_limits() { + // expected behavior: the first request sent will timeout, the second request will return + // immediately due to the ratelimit + let config: serde_json::Value = serde_yaml::from_str( + r#" + router: + global_rate_limit: + capacity: 1 + interval: 100ms + timeout: 150ms + "#, + ) + .unwrap(); + + let plugin = get_traffic_shaping_plugin(&config).await; + let svc = ServiceBuilder::new() + .service_fn(move |_req: router::Request| async { + sleep(Duration::from_millis(500)).await; + RouterResponse::fake_builder().build() + }) + .boxed(); + + let mut router_service = plugin.router_service(svc); + + let mut tasks = JoinSet::new(); + for _ in 0..2 { + let request = RouterRequest::fake_builder().build().unwrap(); + tasks.spawn(router_service.ready().await.unwrap().call(request)); + } + + let mut results = tasks.join_all().await.into_iter(); + + let response = results.next().unwrap().unwrap().response; + assert_eq!(StatusCode::TOO_MANY_REQUESTS, response.status()); + + let response = results.next().unwrap().unwrap().response; + assert_eq!(StatusCode::GATEWAY_TIMEOUT, response.status()); + } } From 6bf70abb90361ffa3ed029fcc24aebb38d121eeb Mon Sep 17 00:00:00 2001 From: carodewig <16093297+carodewig@users.noreply.github.com> Date: Tue, 16 Dec 2025 11:17:44 -0500 Subject: [PATCH 4/5] test: update integration test --- apollo-router/tests/integration/traffic_shaping.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apollo-router/tests/integration/traffic_shaping.rs b/apollo-router/tests/integration/traffic_shaping.rs index 816f4102db..4060001405 100644 --- a/apollo-router/tests/integration/traffic_shaping.rs +++ b/apollo-router/tests/integration/traffic_shaping.rs @@ -261,12 +261,12 @@ async fn test_router_rate_limit() -> Result<(), BoxError> { assert_yaml_snapshot!(response); let (_, response) = router.execute_default_query().await; - assert_eq!(response.status(), 503); + assert_eq!(response.status(), 429); let response = response.text().await?; assert!(response.contains("REQUEST_RATE_LIMITED")); assert_yaml_snapshot!(response); - router.assert_metrics_contains(r#"http_server_request_duration_seconds_count{error_type="Service Unavailable",http_request_method="POST",http_response_status_code="503""#, None).await; + router.assert_metrics_contains(r#"http_server_request_duration_seconds_count{error_type="Too Many Requests",http_request_method="POST",http_response_status_code="429""#, None).await; router.graceful_shutdown().await; Ok(()) From 104d7744cd992db0f38fbcffeb6234486fc68b27 Mon Sep 17 00:00:00 2001 From: carodewig <16093297+carodewig@users.noreply.github.com> Date: Tue, 16 Dec 2025 11:22:02 -0500 Subject: [PATCH 5/5] doc: create changeset --- .changesets/fix_caroline_fix_rate_limit_status_code.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changesets/fix_caroline_fix_rate_limit_status_code.md diff --git a/.changesets/fix_caroline_fix_rate_limit_status_code.md b/.changesets/fix_caroline_fix_rate_limit_status_code.md new file mode 100644 index 0000000000..58dab5d578 --- /dev/null +++ b/.changesets/fix_caroline_fix_rate_limit_status_code.md @@ -0,0 +1,7 @@ +### Raise 429 rather than 503 when enforcing rate-limit ([PR #8765](https://github.com/apollographql/router/pull/8765)) + +In the release of router 2.0, the rate-limiting error raised was changed from 429 (`TOO_MANY_REQUESTS`) to 503 ( +`SERVICE_UNAVAILABLE`). This change reverts that modification to align with the +router [documentation](https://www.apollographql.com/docs/graphos/routing/errors#429). + +By [@carodewig](https://github.com/carodewig) in https://github.com/apollographql/router/pull/8765