-
Notifications
You must be signed in to change notification settings - Fork 7
privacy: return distinct status codes instead of a blanket 502 #1012
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
Changes from 1 commit
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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6375,38 +6375,47 @@ pub async fn privacy_classify( | |||||||||||||||||
| .unwrap_or_else(|_| StatusCode::INTERNAL_SERVER_ERROR.into_response()) | ||||||||||||||||||
| } | ||||||||||||||||||
| Err(e) => { | ||||||||||||||||||
| let (status_code, error_type, message) = match e { | ||||||||||||||||||
| let (status_code, error_type, message, retry_after) = match e { | ||||||||||||||||||
| services::completions::ports::CompletionError::RateLimitExceeded(msg) => { | ||||||||||||||||||
| tracing::warn!("Concurrent request limit exceeded for privacy classify"); | ||||||||||||||||||
| (StatusCode::TOO_MANY_REQUESTS, "rate_limit_error", msg) | ||||||||||||||||||
| (StatusCode::TOO_MANY_REQUESTS, "rate_limit_error", msg, true) | ||||||||||||||||||
| } | ||||||||||||||||||
| services::completions::ports::CompletionError::ProviderError { | ||||||||||||||||||
| status_code, | ||||||||||||||||||
| message, | ||||||||||||||||||
| } => { | ||||||||||||||||||
| tracing::error!( | ||||||||||||||||||
| upstream_status = status_code, | ||||||||||||||||||
| detail = %message, | ||||||||||||||||||
| "Privacy classify provider error" | ||||||||||||||||||
| ); | ||||||||||||||||||
| let http_status = StatusCode::from_u16(status_code) | ||||||||||||||||||
| .unwrap_or(StatusCode::INTERNAL_SERVER_ERROR); | ||||||||||||||||||
| ( | ||||||||||||||||||
| http_status, | ||||||||||||||||||
| "server_error", | ||||||||||||||||||
| "Privacy classify request failed. Please try again later.".to_string(), | ||||||||||||||||||
| ) | ||||||||||||||||||
| if http_status.is_client_error() { | ||||||||||||||||||
| tracing::warn!( | ||||||||||||||||||
| upstream_status = status_code, | ||||||||||||||||||
| "Privacy classify provider error" | ||||||||||||||||||
| ); | ||||||||||||||||||
| (http_status, "invalid_request_error", message, false) | ||||||||||||||||||
| } else { | ||||||||||||||||||
| tracing::error!( | ||||||||||||||||||
| upstream_status = status_code, | ||||||||||||||||||
| "Privacy classify provider error" | ||||||||||||||||||
| ); | ||||||||||||||||||
| ( | ||||||||||||||||||
| http_status, | ||||||||||||||||||
| "server_error", | ||||||||||||||||||
| "Privacy classify request failed. Please try again later.".to_string(), | ||||||||||||||||||
| false, | ||||||||||||||||||
| ) | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| services::completions::ports::CompletionError::InvalidModel(msg) => { | ||||||||||||||||||
| tracing::warn!("Privacy classify model not found"); | ||||||||||||||||||
| (StatusCode::NOT_FOUND, "not_found_error", msg) | ||||||||||||||||||
| (StatusCode::NOT_FOUND, "not_found_error", msg, false) | ||||||||||||||||||
| } | ||||||||||||||||||
| services::completions::ports::CompletionError::ServiceOverloaded(_) => { | ||||||||||||||||||
| tracing::warn!("Privacy classify service overloaded"); | ||||||||||||||||||
| ( | ||||||||||||||||||
| crate::routes::common::status_overloaded(), | ||||||||||||||||||
| "service_overloaded", | ||||||||||||||||||
| "All inference backends are overloaded. Please retry with exponential backoff.".to_string(), | ||||||||||||||||||
| true, | ||||||||||||||||||
| ) | ||||||||||||||||||
| } | ||||||||||||||||||
| _ => { | ||||||||||||||||||
|
|
@@ -6415,15 +6424,22 @@ pub async fn privacy_classify( | |||||||||||||||||
| StatusCode::INTERNAL_SERVER_ERROR, | ||||||||||||||||||
| "server_error", | ||||||||||||||||||
| "Privacy classify request failed".to_string(), | ||||||||||||||||||
| false, | ||||||||||||||||||
| ) | ||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
| ( | ||||||||||||||||||
| let mut response = ( | ||||||||||||||||||
| status_code, | ||||||||||||||||||
| ResponseJson(ErrorResponse::new(message, error_type.to_string())), | ||||||||||||||||||
| ) | ||||||||||||||||||
| .into_response() | ||||||||||||||||||
| .into_response(); | ||||||||||||||||||
| if retry_after { | ||||||||||||||||||
| response | ||||||||||||||||||
| .headers_mut() | ||||||||||||||||||
| .insert(header::RETRY_AFTER, header::HeaderValue::from_static("1")); | ||||||||||||||||||
| } | ||||||||||||||||||
| response | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
@@ -6677,38 +6693,47 @@ pub async fn privacy_redact( | |||||||||||||||||
| { | ||||||||||||||||||
| Ok(b) => b, | ||||||||||||||||||
| Err(e) => { | ||||||||||||||||||
| let (status_code, error_type, message) = match e { | ||||||||||||||||||
| let (status_code, error_type, message, retry_after) = match e { | ||||||||||||||||||
| services::completions::ports::CompletionError::RateLimitExceeded(msg) => { | ||||||||||||||||||
| tracing::warn!("Concurrent request limit exceeded for privacy redact"); | ||||||||||||||||||
| (StatusCode::TOO_MANY_REQUESTS, "rate_limit_error", msg) | ||||||||||||||||||
| (StatusCode::TOO_MANY_REQUESTS, "rate_limit_error", msg, true) | ||||||||||||||||||
| } | ||||||||||||||||||
| services::completions::ports::CompletionError::ProviderError { | ||||||||||||||||||
| status_code, | ||||||||||||||||||
| message, | ||||||||||||||||||
| } => { | ||||||||||||||||||
| tracing::error!( | ||||||||||||||||||
| upstream_status = status_code, | ||||||||||||||||||
| detail = %message, | ||||||||||||||||||
| "Privacy redact provider error" | ||||||||||||||||||
| ); | ||||||||||||||||||
| let http_status = StatusCode::from_u16(status_code) | ||||||||||||||||||
| .unwrap_or(StatusCode::INTERNAL_SERVER_ERROR); | ||||||||||||||||||
| ( | ||||||||||||||||||
| http_status, | ||||||||||||||||||
| "server_error", | ||||||||||||||||||
| "Privacy redact request failed. Please try again later.".to_string(), | ||||||||||||||||||
| ) | ||||||||||||||||||
| if http_status.is_client_error() { | ||||||||||||||||||
| tracing::warn!( | ||||||||||||||||||
| upstream_status = status_code, | ||||||||||||||||||
| "Privacy redact provider error" | ||||||||||||||||||
| ); | ||||||||||||||||||
| (http_status, "invalid_request_error", message, false) | ||||||||||||||||||
| } else { | ||||||||||||||||||
| tracing::error!( | ||||||||||||||||||
| upstream_status = status_code, | ||||||||||||||||||
| "Privacy redact provider error" | ||||||||||||||||||
| ); | ||||||||||||||||||
| ( | ||||||||||||||||||
| http_status, | ||||||||||||||||||
| "server_error", | ||||||||||||||||||
| "Privacy redact request failed. Please try again later.".to_string(), | ||||||||||||||||||
| false, | ||||||||||||||||||
| ) | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| services::completions::ports::CompletionError::InvalidModel(msg) => { | ||||||||||||||||||
| tracing::warn!("Privacy redact model not found"); | ||||||||||||||||||
| (StatusCode::NOT_FOUND, "not_found_error", msg) | ||||||||||||||||||
| (StatusCode::NOT_FOUND, "not_found_error", msg, false) | ||||||||||||||||||
| } | ||||||||||||||||||
| services::completions::ports::CompletionError::ServiceOverloaded(_) => { | ||||||||||||||||||
| tracing::warn!("Privacy redact service overloaded"); | ||||||||||||||||||
| ( | ||||||||||||||||||
| StatusCode::SERVICE_UNAVAILABLE, | ||||||||||||||||||
| "service_overloaded", | ||||||||||||||||||
| "The service is temporarily overloaded. Please retry with exponential backoff.".to_string(), | ||||||||||||||||||
| true, | ||||||||||||||||||
|
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 Additionally, the error message differs: "The service is temporarily overloaded..." here vs "All inference backends are overloaded..." in This divergence will confuse API consumers and complicate client-side retry logic that keys off status codes or error types. Use Suggestion:
Suggested change
Contributor
Author
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. Fixed in a0f35ea, adopting your suggestion verbatim. This turned out to matter more than a consistency nit: it was also the one real gap in the middleware coverage. Since Added |
||||||||||||||||||
| ) | ||||||||||||||||||
| } | ||||||||||||||||||
| _ => { | ||||||||||||||||||
|
|
@@ -6717,14 +6742,21 @@ pub async fn privacy_redact( | |||||||||||||||||
| StatusCode::INTERNAL_SERVER_ERROR, | ||||||||||||||||||
| "server_error", | ||||||||||||||||||
| "Privacy redact request failed".to_string(), | ||||||||||||||||||
| false, | ||||||||||||||||||
| ) | ||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
| return ( | ||||||||||||||||||
| let mut response = ( | ||||||||||||||||||
| status_code, | ||||||||||||||||||
| ResponseJson(ErrorResponse::new(message, error_type.to_string())), | ||||||||||||||||||
| ) | ||||||||||||||||||
| .into_response(); | ||||||||||||||||||
| if retry_after { | ||||||||||||||||||
| response | ||||||||||||||||||
| .headers_mut() | ||||||||||||||||||
| .insert(header::RETRY_AFTER, header::HeaderValue::from_static("1")); | ||||||||||||||||||
| } | ||||||||||||||||||
| return response; | ||||||||||||||||||
| } | ||||||||||||||||||
| }; | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::common::*; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use api::models::{BatchUpdateModelApiRequest, ErrorResponse}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use axum::http::header::RETRY_AFTER; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async fn setup_privacy_filter_model(server: &axum_test::TestServer) -> String { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut batch = BatchUpdateModelApiRequest::new(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -377,3 +378,47 @@ async fn test_privacy_redact_costs_deducted() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Privacy redact should bill 10 tokens × 1_000_000 = 10_000_000", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[tokio::test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async fn test_privacy_redact_upstream_429_returns_rate_limit_with_retry_after() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+382
to
+383
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. Coverage gap: the redact test file mirrors the classify tests but only covers the upstream 429 scenario. The classify file also tests 413 (→ invalid_request_error with the pool-sanitized message passed through) and 500 (→ 502 server_error with a generic message). The redact route handler ( Without 413/500 redact tests, a future change to the redact handler (e.g., accidentally interpolating the raw upstream message, or diverging from the classify path) could leak the upstream body without any test catching it. Adding the same 413 and 500 test cases here would close that gap and keep the two parallel handler copies in lockstep. Suggestion:
Suggested change
Contributor
Author
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. Fixed in a0f35ea — added both, following your sketch. Your reasoning is the part I want to acknowledge: the two handlers are parallel copies with independently maintained mapping, and 413/500 are precisely the cases that exercise sanitization. Without them a future divergence in the redact handler could leak an upstream body with nothing failing. Both new tests carry the PII sentinel assertions. While in there I also added a 503 case, since the redact api privacy suite is now 29 passing, up from 23. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Given: an upstream privacy provider that rejects the request with a | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // body containing data that must never leave the provider boundary. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const UPSTREAM_BODY_SENTINEL: &str = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "UPSTREAM_PRIVACY_BODY_SENTINEL::alice@example.com::123-45-6789"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let (server, _pool, mock_provider, _db) = setup_test_server_with_pool().await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setup_privacy_filter_model(&server).await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let org = setup_org_with_credits(&server, 10_000_000_000i64).await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let api_key = get_api_key_for_org(&server, org.id).await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mock_provider | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .set_privacy_classify_error_override(Some( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| inference_providers::PrivacyClassifyError::HttpError { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status_code: 429, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| message: UPSTREAM_BODY_SENTINEL.to_string(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // When: a client calls the real redact route. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let response = server | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .post("/v1/privacy/redact") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .add_header("Authorization", format!("Bearer {api_key}")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .add_header("User-Agent", MOCK_USER_AGENT) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .json(&serde_json::json!({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "model": "openai/privacy-filter", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "input": "Redact this text" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Then: retry semantics are explicit and the upstream body is absent. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(response.status_code(), 429); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .headers() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .get(RETRY_AFTER) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .and_then(|value| value.to_str().ok()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Some("1") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let error: ErrorResponse = response.json(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(error.error.r#type, "rate_limit_error"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert!(!error.error.message.contains(UPSTREAM_BODY_SENTINEL)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
The
Retry-Afterheader is hardcoded to"1"second for all retryable conditions (RateLimitExceeded and ServiceOverloaded) in bothprivacy_classifyandprivacy_redact. This is more aggressive than the existingretry_after_middlewaredefault of 2 seconds (DEFAULT_RETRY_AFTER_SECS), and it contradicts the error body messaging which advises "exponential backoff." A fixed 1-second hint forServiceOverloaded(where all backends are exhausted) is particularly risky — it may encourage clients to retry too quickly and worsen the overload.Consider using a more conservative value (e.g., the middleware's 2s default) or, for
ServiceOverloaded, omitting the explicit header to let clients honor the "exponential backoff" prose guidance rather than anchoring on a fixed 1s.Suggestion:
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.
Fixed in a0f35ea — you and the other reviewer both landed on this, and you were right that it was worse than a style issue.
I had not noticed
retry_after_middleware(lib.rs:1451,DEFAULT_RETRY_AFTER_SECS = 2) already existed. So the header was not filling a gap, it was overriding one — halving the advertised backoff on these two endpoints only, while the body text says "exponential backoff". Your point aboutServiceOverloadedspecifically is the sharpest version of it: anchoring clients to 1s when all backends are exhausted is exactly when you least want it.Both
headers_mut().insert(...)blocks and theretry_afterbool are gone; the 4-tuples are back to 3-tuples and the middleware does its job. The e2e assertions now expect"2".