Repository navigation
Conversation
Once a streaming response commits HTTP 200, every backend error reached
the client as the same frame:
{"error":{"message":"Internal server error",
"type":"internal_server_error","code":500}}
regardless of what actually failed. A caller's own invalid request was
indistinguishable from an engine fault or an overload.
Two layers dropped the classification. `annotated_to_sse_event` wrapped
only `dynamo_err.message()` into `axum::Error`, discarding the typed
`DynamoError`; `openai_stream_error` then took `_error` and ignored it,
hardcoding `SanitizedError::Internal`. The second was not laziness --
after the first, nothing typed was left to read.
Preserve the `DynamoError` through the SSE conversion and classify on it,
with the same precedence the pre-commit path uses in `from_anyhow`: a
rejection forwards its message verbatim as `invalid_request_error` / 400,
overload and unavailable and cancelled get their own slugs, and anything
else stays sanitized so internals never reach the caller. The existing
leak test still passes unchanged -- forwarding is gated on the rejection
classification, not on the error merely being typed.
`code` in the frame stays advisory, since the real status went out with
the 200. `message` and `type` are what a streaming client reads.
This is why `DYN_HTTP_PRE_COMMIT_ERROR_PEEK_MS` was needed to get an
actionable error out of a streaming request: it catches the failure
before commit so the typed path runs. That workaround is no longer the
only way to learn what went wrong.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughStreaming error handling now preserves typed backend errors and serializes classified failures as OpenAI-compatible SSE frames. Invalid arguments retain their messages and return validation metadata. Unknown errors remain sanitized. Tests cover metadata, fallback behavior, and ChangesStreaming error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The change preserves typed backend errors so streaming clients receive appropriate classifications and safe messages while unknown failures remain sanitized; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if let Some(invalid_argument) = find_invalid_argument_in_chain(error) { | ||
| return ( | ||
| ErrorType::Validation, | ||
| stream_error_frame( | ||
| invalid_argument.message(), | ||
| "invalid_request_error", | ||
| axum::http::StatusCode::BAD_REQUEST.as_u16(), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| // Same precedence as `ErrorMessage::from_anyhow`. | ||
| let (sanitized, error_type) = if request_was_rejected(error) { | ||
| (SanitizedError::Overloaded, ErrorType::Overload) | ||
| } else if request_was_unavailable(error) { | ||
| (SanitizedError::Unavailable, ErrorType::Unavailable) | ||
| } else if request_was_cancelled(error) { | ||
| (SanitizedError::Cancelled, ErrorType::Cancelled) | ||
| } else { | ||
| (SanitizedError::Internal, ErrorType::Internal) | ||
| }; |
There was a problem hiding this comment.
🟡 Streaming errors are sorted differently than non-streaming ones, so an overload or unavailable failure can be reported to the caller as a bad request
The streaming error classifier checks the "caller sent something invalid" case first (find_invalid_argument_in_chain at lib/llm/src/http/service/disconnect.rs:215) instead of after the overload and unavailable checks, so a failure that is both an overload/unavailable and carries an invalid-argument cause is labelled a client mistake and its raw text is passed through, unlike the identical failure on the non-streaming path.
Impact: The same backend failure is described differently (and with a different message) depending on whether the client asked for streaming, and internal text can be forwarded in a case where the non-streaming path sanitizes it.
Precedence mismatch with `ErrorMessage::from_anyhow`
The added doc comment states the classification "mirrors the pre-commit path in backend_error_response" and the inline comment claims "Same precedence as ErrorMessage::from_anyhow". ErrorMessage::from_anyhow (lib/llm/src/http/service/openai.rs:418-470) orders the checks: queue rejection → request_was_rejected (529) → request_was_unavailable (503) → find_invalid_argument_in_chain (400, message forwarded) → request_was_cancelled. lib/llm/src/http/service/sglang_generate.rs:107-119 uses the same order.
The new openai_stream_error evaluates find_invalid_argument_in_chain before the rejected/unavailable checks. For an error chain containing both classifications (e.g. an InvalidArgument wrapped under, or wrapping, a ResourceExhausted/Unavailable cause — match_error_chain walks the whole chain), the streaming path reports invalid_request_error/400 with the raw message while the pre-commit path reports the sanitized overload/unavailable frame.
| if let Some(invalid_argument) = find_invalid_argument_in_chain(error) { | |
| return ( | |
| ErrorType::Validation, | |
| stream_error_frame( | |
| invalid_argument.message(), | |
| "invalid_request_error", | |
| axum::http::StatusCode::BAD_REQUEST.as_u16(), | |
| ), | |
| ); | |
| } | |
| // Same precedence as `ErrorMessage::from_anyhow`. | |
| let (sanitized, error_type) = if request_was_rejected(error) { | |
| (SanitizedError::Overloaded, ErrorType::Overload) | |
| } else if request_was_unavailable(error) { | |
| (SanitizedError::Unavailable, ErrorType::Unavailable) | |
| } else if request_was_cancelled(error) { | |
| (SanitizedError::Cancelled, ErrorType::Cancelled) | |
| } else { | |
| (SanitizedError::Internal, ErrorType::Internal) | |
| }; | |
| // Same precedence as `ErrorMessage::from_anyhow`. | |
| let (sanitized, error_type) = if request_was_rejected(error) { | |
| (SanitizedError::Overloaded, ErrorType::Overload) | |
| } else if request_was_unavailable(error) { | |
| (SanitizedError::Unavailable, ErrorType::Unavailable) | |
| } else if let Some(invalid_argument) = find_invalid_argument_in_chain(error) { | |
| return ( | |
| ErrorType::Validation, | |
| stream_error_frame( | |
| invalid_argument.message(), | |
| "invalid_request_error", | |
| axum::http::StatusCode::BAD_REQUEST.as_u16(), | |
| ), | |
| ); | |
| } else if request_was_cancelled(error) { | |
| (SanitizedError::Cancelled, ErrorType::Cancelled) | |
| } else { | |
| (SanitizedError::Internal, ErrorType::Internal) | |
| }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn openai_stream_error(error: &(dyn std::error::Error + 'static)) -> (ErrorType, String) { | ||
| use crate::http::service::metrics::{ | ||
| request_was_cancelled, request_was_rejected, request_was_unavailable, | ||
| }; | ||
| use crate::http::service::openai::find_invalid_argument_in_chain; | ||
|
|
||
| if let Some(invalid_argument) = find_invalid_argument_in_chain(error) { | ||
| return ( | ||
| ErrorType::Validation, | ||
| stream_error_frame( | ||
| invalid_argument.message(), | ||
| "invalid_request_error", | ||
| axum::http::StatusCode::BAD_REQUEST.as_u16(), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔍 Anthropic streaming clients receive an OpenAI-shaped error frame
The Anthropic handler routes its converted stream through monitor_for_disconnects (lib/llm/src/http/service/anthropic.rs:667), which uses openai_stream_error. That was already true before this PR, but the change now means a forwarded invalid-argument message reaches Anthropic clients inside an OpenAI-shaped {"error":{message,type,code}} frame, which does not match Anthropic's error event schema handled elsewhere in that file (lib/llm/src/http/service/anthropic.rs:1030-1060). Widening what is forwarded makes this shape mismatch more visible to callers.
Was this helpful? React with 👍 or 👎 to provide feedback.
github-actions
Bot
added
frontend
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| // `String` here erases `error_type` and left every streamed backend | ||
| // failure — invalid argument, overload, engine fault — looking | ||
| // identical to the caller. | ||
| if let Some(dynamo_err) = annotated.error.filter(|err| !err.message().is_empty()) { |
There was a problem hiding this comment.
Typed backend errors with an empty message are still flattened to an untyped string here, so overload/unavailable/cancelled classifications can be lost on the streaming path. Fix: preserve annotated.error whenever it is present, and let the downstream formatter decide whether the message is safe to expose.
🤖 AI Fix
In lib/llm/src/http/service/metrics.rs, inside annotated_to_sse_event, replace if let Some(dynamo_err) = annotated.error.filter(|err| !err.message().is_empty()) { return Err(axum::Error::new(dynamo_err)); } with if let Some(dynamo_err) = annotated.error { return Err(axum::Error::new(dynamo_err)); }.
This comment has been minimized.
This comment has been minimized.
The first commit forwarded a rejection's message, on the reasoning that `openai_stream_error` ignored its error because nothing typed reached it. That was wrong about history: 97a28ac ("sanitize HTTP error responses to avoid leaking internals") deliberately replaced `err.to_string()` with a static frame, and #11640 only moved that behaviour into a formatter. The exposure is also wider than messages we author. `py_err_to_dynamo` maps any Python `ValueError`/`TypeError` to `Backend(InvalidArgument)`, so forwarding on that classification would carry engine-internal text -- paths, tensor shapes -- to a streaming client. This path cannot tell an authored rejection from an incidental one. Keep the classification, drop the forwarding: add `SanitizedError:: InvalidRequest` so a rejection reports "Invalid request" / `invalid_request_error` / 400. A streaming client can now tell its own bad request from an engine fault, which was the actual gap, without any backend text crossing the boundary. The unary path still forwards on 400. It can afford to -- it is the established protocol contract there and predates this -- but that asymmetry is now stated in the code rather than implied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Automated evidence record — validation incompleteValidation status: incomplete Evidence summary: 2 of 3 checks validated; 1 needs hardware that was not available here. AI review assessment (advisory, not an approval): Validation result: incomplete. Every static, compile, lint, and unit-test check passed. The live single-GPU streaming request could not run: the GPU driver available in this environment reports CUDA Evidence audit: complete — 2 of 3 validated, 1 needs hardware. The command report below comes from recorded runs. Commands and results [2/3 validated · 1 needs hardware]Generated from the commands recorded during this run. Check 1Builds the changed Dynamo source and confirms that Python can import its compiled extension. Result: Passed ( Command: Not shown because the exact command contained private run data. Check 2Checks the changed Rust crates with Result: Passed ( Command: cargo test -p dynamo-llm --lib http::service::disconnect::tests::test_cancelled_classification_survives_an_unset_signal -- --exact --nocaptureCheck 3Starts Dynamo with vLLM on one GPU and sends a real request. Result: Failed ( Command: bash -c 'source /opt/dynamo/venv/bin/activate; python3 -c "import torch; print(\"torch\", torch.__version__, \"built for CUDA\", torch.version.cuda); print(\"cuda available:\", torch.cuda.is_available()); torch.cuda.init()"'Details: The live aggregated streaming smoke could not run: the GPU driver available to this environment reports CUDA 12080 while the installed PyTorch is built for CUDA 13.0, so the vLLM worker aborts in CUDA init with "The NVIDIA driver on your system is too old (found version 12080)" and never registers with the frontend. |
review.md
Assessment: needs_changesThis reviews #13139 — "classify mid-stream SSE Four findings follow. One is a behavioural divergence the code's own comment denies; one is Line numbers below are in the post-image of the PR's own diff. Drift auditThe change was materialized onto current Two benign, non-substantive differences exist and are not findings. Findings1.
|
glamr-agent
left a comment
glamr-agent
left a comment
There was a problem hiding this comment.
Automated AI review — advisory. The inline comments below are the findings. The full review is in a separate comment on this pull request.
| // Same precedence as `ErrorMessage::from_anyhow`. | ||
| let (sanitized, error_type) = if find_invalid_argument_in_chain(error).is_some() { | ||
| (SanitizedError::InvalidRequest, ErrorType::Validation) | ||
| } else if request_was_rejected(error) { | ||
| (SanitizedError::Overloaded, ErrorType::Overload) | ||
| } else if request_was_unavailable(error) { | ||
| (SanitizedError::Unavailable, ErrorType::Unavailable) | ||
| } else if request_was_cancelled(error) { | ||
| (SanitizedError::Cancelled, ErrorType::Cancelled) | ||
| } else { | ||
| (SanitizedError::Internal, ErrorType::Internal) | ||
| }; |
There was a problem hiding this comment.
Blocking — automated AI review (advisory). the comment // Same precedence as ErrorMessage::from_anyhow. is not accurate — this ladder tests find_invalid_argument_in_chain first while ErrorMessage::from_anyhow tests it fourth, so a chain matching both request_was_rejected and find_invalid_argument_in_chain is an overload before the response commits and a 400 after the first token
Evidence: openai.rs:548 orders find_queue_rejection_in_chain (openai.rs:549), request_was_rejected (openai.rs:564), request_was_unavailable (openai.rs:572), find_invalid_argument_in_chain (openai.rs:580), request_was_cancelled (openai.rs:594); anthropic.rs:563-589 and sglang_generate.rs:107-118 use that same order. Both predicates walk the whole chain — find_invalid_argument_in_chain (openai.rs:277) matches InvalidArgument or Backend(InvalidArgument) anywhere in it, request_was_rejected (metrics.rs:41) matches ResourceExhausted or WorkerOverloaded anywhere in it — so order alone decides. Pre-commit that chain is SanitizedError::Overloaded (overload_status_code(), 529 by default, slug service_unavailable, label overload); mid-stream it is now "code": 400, "type": "invalid_request_error", label validation. map_scheduler_error (lib/llm/src/kv_router.rs:451) builds exactly such wrappers. The suggested reorder is safe against test_mid_stream_invalid_argument_reports_category_without_message, which drives a bare Backend(InvalidArgument) for which request_was_rejected is false. The alternative resolution is to keep this order and rewrite the comment to state the intended streaming-specific precedence, including whether dropping the find_queue_rejection_in_chain arm was deliberate.
| // Same precedence as `ErrorMessage::from_anyhow`. | |
| let (sanitized, error_type) = if find_invalid_argument_in_chain(error).is_some() { | |
| (SanitizedError::InvalidRequest, ErrorType::Validation) | |
| } else if request_was_rejected(error) { | |
| (SanitizedError::Overloaded, ErrorType::Overload) | |
| } else if request_was_unavailable(error) { | |
| (SanitizedError::Unavailable, ErrorType::Unavailable) | |
| } else if request_was_cancelled(error) { | |
| (SanitizedError::Cancelled, ErrorType::Cancelled) | |
| } else { | |
| (SanitizedError::Internal, ErrorType::Internal) | |
| }; | |
| // Same precedence as `ErrorMessage::from_anyhow`: a rejection or an | |
| // unavailable worker outranks an invalid argument found deeper in the chain. | |
| let (sanitized, error_type) = if request_was_rejected(error) { | |
| (SanitizedError::Overloaded, ErrorType::Overload) | |
| } else if request_was_unavailable(error) { | |
| (SanitizedError::Unavailable, ErrorType::Unavailable) | |
| } else if find_invalid_argument_in_chain(error).is_some() { | |
| (SanitizedError::InvalidRequest, ErrorType::Validation) | |
| } else if request_was_cancelled(error) { | |
| (SanitizedError::Cancelled, ErrorType::Cancelled) | |
| } else { | |
| (SanitizedError::Internal, ErrorType::Internal) | |
| }; |
| } | ||
|
|
||
| /// The complement: an untyped backend fault stays sanitized. Forwarding | ||
| /// messages is gated on the rejection classification, not on the error | ||
| /// merely being typed. | ||
| #[tokio::test] | ||
| async fn test_mid_stream_unknown_backend_error_stays_sanitized() { | ||
| use dynamo_runtime::error::{BackendError, DynamoError, ErrorType as RuntimeErrorType}; | ||
|
|
||
| let (_metrics, guard, ctx, handle) = setup_test("unknown-model", "req-unknown"); | ||
| let secret = "assertion failed at engine/worker.py:512"; | ||
| let stream = typed_error_stream( | ||
| DynamoError::builder() | ||
| .error_type(RuntimeErrorType::Backend(BackendError::Unknown)) | ||
| .message(secret) | ||
| .build(), | ||
| ); | ||
|
|
||
| let monitored = monitor_for_disconnects_with_timeout(stream, ctx, guard, handle, None); | ||
| let body = collect_sse_body(monitored).await; | ||
| let frame = error_frame(&body); | ||
|
|
||
| assert_eq!(frame["message"], "Internal server error"); | ||
| assert_eq!(frame["type"], "internal_server_error"); | ||
| assert_eq!(frame["code"], 500); | ||
| assert!( | ||
| !body.contains(secret), | ||
| "leaked internal detail. Body:\n{body}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Blocking — automated AI review (advisory). test_mid_stream_unknown_backend_error_stays_sanitized passes unchanged with the production change reverted, so it documents pre-existing behaviour rather than evidencing this change, and should be deleted rather than strengthened
Evidence: It asserts frame["message"] == "Internal server error", frame["type"] == "internal_server_error" and frame["code"] == 500 — exactly what the previous openai_stream_error produced for every error, since it built its body from an unconditional SanitizedError::Internal. Post-change, Backend(BackendError::Unknown) falls to the final else, which is the same SanitizedError::Internal, so all four assertions are byte-identical either way. Confirm it by restoring the body of openai_stream_error to (ErrorType::Internal, …) and running cargo test -p dynamo-llm --lib http::service: this test stays green. Its doc comment is separately inaccurate — "Forwarding messages is gated on the rejection classification" describes a gate that does not exist, since every arm passes a static sanitized.to_string() into stream_error_frame. The suggestion deletes the test; the range starts one line early, at the preceding test's closing brace, so the replacement text is exact.
| } | |
| /// The complement: an untyped backend fault stays sanitized. Forwarding | |
| /// messages is gated on the rejection classification, not on the error | |
| /// merely being typed. | |
| #[tokio::test] | |
| async fn test_mid_stream_unknown_backend_error_stays_sanitized() { | |
| use dynamo_runtime::error::{BackendError, DynamoError, ErrorType as RuntimeErrorType}; | |
| let (_metrics, guard, ctx, handle) = setup_test("unknown-model", "req-unknown"); | |
| let secret = "assertion failed at engine/worker.py:512"; | |
| let stream = typed_error_stream( | |
| DynamoError::builder() | |
| .error_type(RuntimeErrorType::Backend(BackendError::Unknown)) | |
| .message(secret) | |
| .build(), | |
| ); | |
| let monitored = monitor_for_disconnects_with_timeout(stream, ctx, guard, handle, None); | |
| let body = collect_sse_body(monitored).await; | |
| let frame = error_frame(&body); | |
| assert_eq!(frame["message"], "Internal server error"); | |
| assert_eq!(frame["type"], "internal_server_error"); | |
| assert_eq!(frame["code"], 500); | |
| assert!( | |
| !body.contains(secret), | |
| "leaked internal detail. Body:\n{body}" | |
| ); | |
| } | |
| } |
| } | ||
|
|
||
| /// Without a typed error the comment fallback still applies, untyped. | ||
| #[test] | ||
| fn test_annotated_error_event_falls_back_to_comment() { | ||
| let annotated = crate::types::Annotated::<serde_json::Value> { | ||
| data: None, | ||
| id: None, | ||
| event: Some("error".to_string()), | ||
| comment: Some(vec!["worker exploded".to_string()]), | ||
| error: None, | ||
| }; | ||
|
|
||
| let err = annotated_to_sse_event(annotated).expect_err("error event must become Err"); | ||
| assert!(err.to_string().contains("worker exploded")); | ||
| assert!(crate::http::service::openai::find_invalid_argument_in_chain(&err).is_none()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Blocking — automated AI review (advisory). test_annotated_error_event_falls_back_to_comment passes unchanged with the production change reverted, because error: None means it never enters the branch this change adds, and should be deleted
Evidence: The new branch in annotated_to_sse_event is guarded on annotated.error being Some and non-empty, so an Annotated built with error: None exercises only the untouched comment fallback; both assertions — that the message contains worker exploded, and that find_invalid_argument_in_chain finds nothing — held before the change. Its sibling test_annotated_error_event_preserves_typed_error (metrics.rs:2252) is the one that carries the weight and should stay: revert the change and its .expect(...) panics, because the typed error is flattened to a String and find_invalid_argument_in_chain returns None. The suggestion deletes only the comment-fallback test; the range starts one line early, at the preceding test's closing brace, so the replacement text is exact.
| } | |
| /// Without a typed error the comment fallback still applies, untyped. | |
| #[test] | |
| fn test_annotated_error_event_falls_back_to_comment() { | |
| let annotated = crate::types::Annotated::<serde_json::Value> { | |
| data: None, | |
| id: None, | |
| event: Some("error".to_string()), | |
| comment: Some(vec!["worker exploded".to_string()]), | |
| error: None, | |
| }; | |
| let err = annotated_to_sse_event(annotated).expect_err("error event must become Err"); | |
| assert!(err.to_string().contains("worker exploded")); | |
| assert!(crate::http::service::openai::find_invalid_argument_in_chain(&err).is_none()); | |
| } | |
| } |
| } else if request_was_cancelled(error) { | ||
| (SanitizedError::Cancelled, ErrorType::Cancelled) |
There was a problem hiding this comment.
Nit — automated AI review (advisory). ErrorType::Cancelled is now both a formatter verdict and the "dropped without a verdict" sentinel, so a genuine mid-stream cancellation can be silently relabelled on drop
Evidence: disconnect.rs:491 seeds every monitored stream with inflight_guard.mark_error(ErrorType::Cancelled) to mean "no verdict yet", disconnect.rs:520 overwrites it with the formatter's verdict, and SignaledInflightGuard::drop (disconnect.rs:142) re-marks the guard from the stream-error signal precisely when it still reads ErrorType::Cancelled. Before this change the formatter returned ErrorType::Internal unconditionally, so that sentinel was unambiguous. This is latent rather than live: the only site supplying a StreamErrorSignal is the Responses streaming handler (openai.rs:3608, openai.rs:3673), where backend errors are consumed by extract_backend_error_if_present before they can reach error_formatter. A distinct unset state on the guard would remove the ambiguity more cheaply than special-casing this arm.
Assessment: needs_changesThe premise here is right, and so is the shape of the fix. Four things I would want resolved before this merges. Line numbers are in this PR's diff. 1. Both predicates walk the whole chain — Either reorder to match — 2. Its doc comment is also inaccurate independently of that: "Forwarding messages is gated on 3. That leaves exactly two of the four added tests sensitive to the change — 4. I could not build a live request path that hits this today — the only site supplying a One coverage gap. Nothing in the PR drives the change through the streaming handler. Scheduling note, not an objection. #14382 and Requested changes
Everything else checks out. |
|
Cross-linking: I independently filed #14403 for this exact issue before finding this PR (my search terms didn't surface "classify backend errors on the streaming path"). Closing #14403 as a duplicate of this PR, which is the correct and more precisely-located fix. For the record, #14403 carries production evidence for the impact, in case it's useful motivation here: cross-checking Related umbrella: #12899 (couple HTTP errors with metric classification — Anthropic pre-stream handler). |
This branch has not been deployed
3 participants
Overview:
Once a streaming response commits HTTP 200, every backend error reaches the client as the same frame:
{"error":{"message":"Internal server error","type":"internal_server_error","code":500}}regardless of what actually failed. A caller's own invalid request is indistinguishable from an engine fault, an overload, or a cancellation. This recovers the category — and only the category.
Details:
Two layers drop the classification:
annotated_to_sse_event(metrics.rs) wraps onlydynamo_err.message()— aString— intoaxum::Error, discarding the typedDynamoErrorand with iterror_type.openai_stream_error(disconnect.rs) has no type left to read, so it hardcodesSanitizedError::Internal.This preserves the
DynamoErrorthrough the SSE conversion and classifies on it, using the same precedence asErrorMessage::from_anyhow:InvalidArgument/Backend(InvalidArgument)Internal server error/internal_server_error/ 500Invalid request/invalid_request_error/ 400Service temporarily overloaded/service_unavailable/ 529Service temporarily unavailable/service_unavailable/ 503Request cancelled/request_cancelled/ 499Every message stays sanitized — deliberately
An earlier revision of this PR forwarded the rejection's message, reasoning that
openai_stream_errorignored its argument only because nothing typed reached it. That was wrong about the history. 97a28ac (fix(http): sanitize HTTP error responses to avoid leaking internals, #10151) deliberately replacederr.to_string()with a static frame and rewroteassert_fault_contractto assert the raw detail appears nowhere in the body; #11640 only moved that behaviour into a pluggable formatter. Nothing here reintroduces it.Forwarding would also be wider than it sounds:
py_err_to_dynamo(backend.rs:1654) maps any PythonValueError/TypeErrortoBackend(InvalidArgument), so an engine-internalValueErrornaming a path or tensor shape would ride out to the client. This path cannot distinguish an authored rejection from an incidental one, so it forwards neither.Hence
SanitizedError::InvalidRequest: a fixed("Invalid request", invalid_request_error, 400)triple. A streaming client learns the fault was its own — the actual gap — with no backend text crossing the boundary.The unary path still forwards a rejection's message on 400. It can afford to; that is the established protocol contract there and predates this change. The asymmetry is now stated in the code rather than implied.
codein the frame is advisory — the real status went out with the 200 and cannot be revised.Where should the reviewer start?
lib/llm/src/http/service/disconnect.rs—openai_stream_errorand the doc comment stating why nothing is forwarded. ThenSanitizedError::InvalidRequestinerror.rs, and the one substantive line inannotated_to_sse_event(metrics.rs) that keeps the typed error reachable.Validation
test_mid_stream_invalid_argument_reports_category_without_message— aBackend(InvalidArgument)fault whose message isValueError: shape mismatch at /opt/dynamo/engine/worker.py:512yieldsInvalid request/invalid_request_error/ 400, and asserts neitherworker.pynorshape mismatchappears anywhere in the body.test_mid_stream_unknown_backend_error_stays_sanitized—Backend(Unknown)staysInternal server error/ 500 with its detail absent.test_annotated_error_event_preserves_typed_error/..._falls_back_to_comment— the conversion keeps the typed error; the untyped comment fallback is unchanged.test_mid_stream_error_does_not_leak_internal_detailspasses unmodified — the guard that this does not widen what reaches a client.cargo test -p dynamo-llm --lib— 2057 passed, 0 failed.cargo fmt --allandcargo clippy -p dynamo-llm --no-deps --all-targets -- -D warningsclean.Verified end to end
Live frontend + worker over the real request plane (etcd + NATS, no GPU),
stream: true, noDYN_HTTP_PRE_COMMIT_ERROR_PEEK_MS:Non-streaming re-checked on the same build and unchanged — still 400 carrying the backend's full message — confirming the pre-commit path is untouched.
Related Issues
🚫 This PR is NOT linked to an issue:
🤖 Generated with Claude Code