privacy: return distinct status codes instead of a blanket 502 - #1012
privacy: return distinct status codes instead of a blanket 502#1012lloydmak99 wants to merge 2 commits into
Conversation
Every failure of /v1/privacy/{classify,redact} reached the client as a
generic 502, so a caller could not tell "split this input" from "back off
and retry" from "the vendor is down". Reported in #987 against the live
endpoint, where an over-limit request and an outage were indistinguishable
by status code alone.
InferenceProviderPool::privacy_classify ended by collapsing last_error --
including a typed PrivacyClassifyError::HttpError { status_code } -- into
PrivacyClassifyError::RequestFailed(String). try_privacy_classify's match
on status_code could therefore only ever reach its RequestFailed arm,
hardcoded to ProviderError { status_code: 502 }. The 429, 503 and
passthrough arms were unreachable dead code.
That collapse existed for a good reason: a misbehaving PII filter that
echoes its input would route customer PII into logs and client-visible
errors. So the status code is preserved WITHOUT the upstream body -- the
message is synthesized from the status alone ("PII detector returned HTTP
{code}"), never taken from the response. A unit test feeds a sentinel
containing an email and an SSN as the upstream body and asserts neither
reaches the caller.
- pool returns the typed error with a synthesized message
- explicit 413 arm in try_privacy_classify
- handlers split 4xx (invalid_request_error, warn) from 5xx
(server_error, error) rather than labelling everything server_error
- Retry-After: 1 on 429 and 503
- privacy_redact carried the identical bug and gets the same treatment
Tests cover upstream 429 -> 429 with Retry-After, upstream 413 -> 413, and
upstream 500 -> 502 with no upstream body in the response.
Fixes #987
ReviewGood diagnosis — the
|
Review · Status🟩 CompletedIronLoop completed the review and posted it to GitHub. ResultRun detailsAutomatic trigger · attempt 1 of 3 · completed in 4m 37s |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review · Summary
Found one distinct error-propagation issue in the privacy-provider fallback path.
Findings: 🟠 Medium 1
Code-specific findings are attached to the diff.
Validation
- ✅ Captured CI — Captured PR evidence reports passing build, unit, integration, E2E, lint, and dependency-security checks.
Review details
- Run:
13b87035-a47c-4d2d-b75f-a8c8319a9e6e - Attempts: 1
| } | ||
| inference_providers::PrivacyClassifyError::RequestFailed(_) => { | ||
| "PII detector unreachable".to_string() | ||
| Err(match last_error { |
There was a problem hiding this comment.
🟠 Medium · Fallback errors can erase the propagated client status
This selects only the last provider failure. With multiple fallbacks, a primary can correctly reject an oversized request with 413, then a later provider can fail with RequestFailed (for example, an unsupported privacy-classify backend); the final error becomes 502 instead. Provider ordering is rotated, so the same request can alternate between 413 and 502. Preserve or short-circuit non-retryable client errors before a later fallback overwrites them.
There was a problem hiding this comment.
Fixed in a0f35ea — good catch, and the rotation detail is what makes it nasty: the same request alternating 413/502 depending on provider order is far harder to diagnose than a consistent wrong answer.
Non-retryable client errors now short-circuit before any later provider runs:
inference_providers::PrivacyClassifyError::HttpError { status_code, .. }
if (400..=499).contains(&status_code) && !matches!(status_code, 408 | 429) =>
{
return Err(inference_providers::PrivacyClassifyError::HttpError {
status_code,
message: format!("PII detector returned HTTP {status_code}"),
});
}408 and 429 are excluded because those genuinely are worth retrying elsewhere; everything else in 4xx means the request is wrong and another backend will not fix it. Sanitization is preserved on the early-return path, so the upstream body still cannot escape.
Added privacy_classify_non_retryable_client_error_short_circuits_later_providers: two providers, the first returning 413 and the second RequestFailed, asserting the caller sees 413 and that the second is never consulted.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #[tokio::test] | ||
| async fn test_privacy_redact_upstream_429_returns_rate_limit_with_retry_after() { |
There was a problem hiding this comment.
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 (privacy_redact) is a parallel copy of the classify handler with independently maintained error-mapping logic — not a shared function call. For the 4xx client-error branch specifically, both routes pass the message field directly to the client (e.g., line ~6712: (http_status, "invalid_request_error", message, false)). This currently relies on the pool layer having pre-sanitized the message to "PII detector returned HTTP {status_code}".
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:
| #[tokio::test] | |
| async fn test_privacy_redact_upstream_429_returns_rate_limit_with_retry_after() { | |
| // Add analogous tests for upstream 413 and 500: | |
| #[tokio::test] | |
| async fn test_privacy_redact_upstream_413_returns_invalid_request() { | |
| // ... same setup pattern ... | |
| mock_provider | |
| .set_privacy_classify_error_override(Some( | |
| inference_providers::PrivacyClassifyError::HttpError { | |
| status_code: 413, | |
| message: UPSTREAM_BODY_SENTINEL.to_string(), | |
| }, | |
| )) | |
| .await; | |
| // ... call /v1/privacy/redact ... | |
| assert_eq!(response.status_code(), 413); | |
| let error: ErrorResponse = response.json(); | |
| assert_eq!(error.error.r#type, "invalid_request_error"); | |
| assert_eq!(error.error.message, "PII detector returned HTTP 413"); | |
| assert!(!error.error.message.contains(UPSTREAM_BODY_SENTINEL)); | |
| } | |
| #[tokio::test] | |
| async fn test_privacy_redact_upstream_500_returns_502_without_upstream_body() { | |
| // ... same setup pattern ... | |
| mock_provider | |
| .set_privacy_classify_error_override(Some( | |
| inference_providers::PrivacyClassifyError::HttpError { | |
| status_code: 500, | |
| message: UPSTREAM_BODY_SENTINEL.to_string(), | |
| }, | |
| )) | |
| .await; | |
| // ... call /v1/privacy/redact ... | |
| assert_eq!(response.status_code(), 502); | |
| let error: ErrorResponse = response.json(); | |
| assert_eq!(error.error.r#type, "server_error"); | |
| assert!(!error.error.message.contains(UPSTREAM_BODY_SENTINEL)); | |
| } |
There was a problem hiding this comment.
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 ServiceOverloaded arm was diverging from classify (see the thread above).
api privacy suite is now 29 passing, up from 23.
| 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.
The privacy_redact handler maps ServiceOverloaded to StatusCode::SERVICE_UNAVAILABLE (503), while privacy_classify (line 6415) and every other handler in this file (audio transcription at 4921, rerank at 5723, embeddings at 6082, score at 7255) use crate::routes::common::status_overloaded() which returns 429 (TOO_MANY_REQUESTS). This means the same CompletionError::ServiceOverloaded variant produces different HTTP status codes depending on which endpoint caught it — 503 for redact vs 429 everywhere else.
Additionally, the error message differs: "The service is temporarily overloaded..." here vs "All inference backends are overloaded..." in privacy_classify and all other handlers.
This divergence will confuse API consumers and complicate client-side retry logic that keys off status codes or error types. Use status_overloaded() and the canonical message for consistency.
Suggestion:
| StatusCode::SERVICE_UNAVAILABLE, | |
| "service_overloaded", | |
| "The service is temporarily overloaded. Please retry with exponential backoff.".to_string(), | |
| true, | |
| crate::routes::common::status_overloaded(), | |
| "service_overloaded", | |
| "All inference backends are overloaded. Please retry with exponential backoff.".to_string(), | |
| true, |
There was a problem hiding this comment.
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 retry_after_middleware only stamps 429s, redact returning 503 here meant ServiceOverloaded on that endpoint got no Retry-After at all — which is part of why the hand-rolled header looked necessary in the first place. Making redact match status_overloaded() closed the gap properly and let the local header go entirely.
Added test_privacy_redact_upstream_503_returns_overloaded_with_default_retry_after to pin both the status and the middleware default.
| if retry_after { | ||
| response | ||
| .headers_mut() | ||
| .insert(header::RETRY_AFTER, header::HeaderValue::from_static("1")); |
There was a problem hiding this comment.
The Retry-After header is hardcoded to "1" second for all retryable conditions (RateLimitExceeded and ServiceOverloaded) in both privacy_classify and privacy_redact. This is more aggressive than the existing retry_after_middleware default of 2 seconds (DEFAULT_RETRY_AFTER_SECS), and it contradicts the error body messaging which advises "exponential backoff." A fixed 1-second hint for ServiceOverloaded (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:
| .insert(header::RETRY_AFTER, header::HeaderValue::from_static("1")); | |
| // Consider: header::HeaderValue::from(crate::middleware::retry_after::DEFAULT_RETRY_AFTER_SECS) | |
| // or reuse the middleware default rather than hardcoding a shorter 1s value. |
There was a problem hiding this comment.
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 about ServiceOverloaded specifically 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 the retry_after bool are gone; the 4-tuples are back to 3-tuples and the middleware does its job. The e2e assertions now expect "2".
| 413 => ports::CompletionError::ProviderError { | ||
| status_code: 413, | ||
| message, | ||
| }, |
There was a problem hiding this comment.
The new explicit 413 match arm is functionally identical to the other catch-all below it — both produce ProviderError { status_code: <code>, message }. Before this change, 413 would have naturally fallen through to other with the same result. This arm adds no behavioral difference and is dead code in the sense that removing it would not change any behavior.
If the intent is to document that 413 is an expected status from the PII detector, consider adding a comment instead of a redundant match arm, or consolidate it with the other arm.
There was a problem hiding this comment.
Fixed in a0f35ea — removed. You are right that it is behaviourally identical to the other arm and that its only real effect was to make 413 look special-cased when it is not.
test_privacy_classify_upstream_413_returns_invalid_request still passes without it, which is the point.
Six findings from review, all valid. 1. The hand-rolled `Retry-After: 1` was not filling a gap -- it was overriding one. `retry_after_middleware` (lib.rs:1451) already stamps `Retry-After: 2` on any 429 without one, and its header comment names the exact cases this change hand-rolled. Every `retry_after: true` site resolved to 429, so the effect was to advertise half the backoff of every other 429 in the API while the body said "exponential backoff". The bool and both header inserts are gone. The one genuine gap was privacy_redact's ServiceOverloaded arm, which returned 503 where classify and every other handler return status_overloaded() (429); it now matches, so the middleware covers it. 2. `http_status.is_client_error()` accepted any 4xx, so an upstream 404 (misrouted backend) or 407 (misconfigured egress proxy) was reported to the caller as invalid_request_error -- our infra fault blamed on the client -- and logged at warn, dropping out of 5xx error-rate alerting. The else branch also passed non-4xx through verbatim, so a 3xx would have emitted e.g. 302 with a JSON error body and no Location. Both handlers now use the existing classify_provider_error, which masks 401/403/407, routes 404 to not_found_error, and falls back to a generic for anything non-4xx. It is parameterized on the fallback so the embeddings caller keeps its exact previous strings. 3. The 413 arm in try_privacy_classify produced exactly what the `other` arm below it produced, and was the only divergence from three otherwise-identical sibling blocks. Removed; the 413 e2e test still passes. 4. privacy_redact had only the 429 test while classify had 429/413/500 -- and 413/500 are the two that exercise body sanitization, on a handler that is a parallel copy with independently maintained mapping. Both are mirrored, plus a 503 case asserting the middleware's default Retry-After. 5. The pool kept only last_error, so with multiple providers a correct 413 from provider A could be overwritten by a transport failure from provider B and surface as 502. Since provider order rotates, the same request could alternate 413/502. Non-retryable client errors (4xx except 408/429) now short-circuit before later providers run, with sanitization preserved on that path. 6. The handlers forward `message` on the 4xx branch, which is only safe because the pool rewrites it at its single exit point. That load-bearing invariant is now documented where it is relied upon. cargo test -p api privacy: 29 passed, 0 failed cargo test -p services privacy_classify: 2 passed classify_provider_error unit tests: 10 passed (embeddings unchanged) cargo clippy --workspace: 0 warnings
|
All three blocking items plus both minors are addressed in a0f35ea. Thanks — items 1 and 2 were both cases where I had reasoned from the diff in isolation rather than from what the router already does. 1. 2. 3. The 413 arm is gone. Identical to Minors, both taken. Redact now has the 413 and 500 cases (and a 503 one). And the handler now documents that forwarding Also fixed the separate ironloop finding on the same file: with multiple providers, a correct 413 from provider A could be overwritten by provider B's transport failure and surface as 502 — alternating with rotation. Non-retryable 4xx (excluding 408/429) now short-circuit. One thing worth flagging for re-review: fixing these grew the diff rather than shrinking it (+498), almost entirely from |
1 participant
Closes #987 (status-code half).
Problem
Every failure of
/v1/privacy/{classify,redact}reached the client as a generic 502, so a caller could not distinguish "split this input" from "back off and retry" from "the vendor is down".InferenceProviderPool::privacy_classifyended by collapsinglast_error— including a typedPrivacyClassifyError::HttpError { status_code }— intoPrivacyClassifyError::RequestFailed(String).try_privacy_classify'smatch status_codecould therefore only ever reach itsRequestFailedarm, hardcoded toProviderError { status_code: 502 }.The 429, 503 and passthrough arms were unreachable dead code.
The constraint that shaped the fix
That collapse existed for a good reason: a misbehaving PII filter that echoes its input would route customer PII into logs and client-visible errors.
So the status code is preserved without the upstream body — the message is synthesized from the status alone (
"PII detector returned HTTP {code}"), never taken from the response. A unit test feeds a sentinel containing an email and an SSN as the upstream body and asserts neither reaches the caller.Changes
413arm intry_privacy_classifyinvalid_request_error, warn) from 5xx (server_error, error) rather than labelling everythingserver_errorRetry-After: 1on 429 and 503privacy_redactcarried the identical bug and gets the same treatmentVerification
cargo build --workspacecleancargo test -p api privacy— 23 passed, 0 failed (19 pre-existing + 4 new)cargo clippy --workspace— 0 warningsRetry-After; upstream 413 → 413; upstream 500 → 502 with no upstream bodyprivacy_classify_preserves_http_status_and_discards_response_bodyNote for reviewers: these e2e tests need Postgres,
DEV=true, andBRAVE_SEARCH_PRO_API_KEY/AUTH_ENCODING_KEY/AUTH_ADMIN_DOMAINSset, or the whole suite fails at bootstrap and looks like a regression.Not included
The
context_length: 512metadata onopenai/privacy-filter(the model card says 128,000) and the 256 KB body cap are tracked separately — see #987.