fix: Bedrock stream silently drops exception/error frames#128
fix: Bedrock stream silently drops exception/error frames#128Mingye-Lu wants to merge 2 commits into
Conversation
parse_stream_event_payload tried to deserialize every event-stream payload as StreamEvent and silently dropped it via .ok() on failure. AWS's vnd.amazon.eventstream framing signals mid-stream errors (throttling, validation, internal errors) via :message-type / :exception-type headers, which parse_event_frame skipped over entirely without inspecting -- so mid-stream Bedrock errors were discarded and the caller just saw the stream end early. parse_event_frame now also parses and returns the header block (name/value pairs, supporting all vnd.amazon.eventstream header value types). drain_frames checks each frame's headers via the new event_frame_error: if :message-type is "exception" or "error", it builds an ApiError::Api carrying the :exception-type/:error-code and a message extracted from the JSON payload (or :error-message header), with retryable set for the known-transient exception types (throttling/serviceUnavailable/internalServer/modelTimeout). Adds tests for: header parsing, a non-error frame producing no error, a throttlingException frame surfacing as a retryable error with the right type/message, and a validationException frame surfacing as non-retryable.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review — retrying, previous review attempt hit the Codex usage-limit error. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 436e3ed6f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
|
|
||
| Some(ApiError::Api { | ||
| status: reqwest::StatusCode::INTERNAL_SERVER_ERROR, |
There was a problem hiding this comment.
When a Bedrock stream emits a non-500 exception such as validationException or throttlingException, this hard-coded status makes ApiError::Display report api returned 500 (...) even though the event-stream exception represents a 400/429 class failure. That gives users and any status-aware handling the wrong failure class; please map the Bedrock exception type to its documented status instead of always using INTERNAL_SERVER_ERROR.
Useful? React with 👍 / 👎.
| let retryable = matches!( | ||
| exception_type.as_deref(), | ||
| Some( | ||
| "throttlingException" | ||
| | "modelTimeoutException" | ||
| | "serviceUnavailableException" | ||
| | "internalServerException" | ||
| ) |
There was a problem hiding this comment.
When Bedrock emits modelStreamErrorException mid-stream, the service is telling us the stream failed after starting and the request should be retried, but this allow-list leaves it with retryable: false. In contexts that consult ApiError::is_retryable() this newly surfaced transient failure will be handled as terminal, unlike the other transient Bedrock stream errors listed here.
Useful? React with 👍 / 👎.
…xception as retryable - Add bedrock_exception_status() to map Bedrock :exception-type headers to appropriate HTTP status codes (400/429/504/503/500) - Replace hard-coded INTERNAL_SERVER_ERROR in event_frame_error() with the new mapping function - Add modelStreamErrorException to the retryable exception set - Add unit test for the full status mapping
2 participants
Summary
parse_stream_event_payloadtried to deserialize every Bedrock event-stream payload asStreamEventand silently dropped it (.ok()) on failure. AWS'svnd.amazon.eventstreamframing signals mid-stream errors (throttling, validation, internal errors) via:message-type/:exception-typeheaders, whichparse_event_frameskipped over entirely without inspecting — so mid-stream Bedrock errors were discarded and the caller just saw the stream end early.parse_event_framenow also parses and returns the header block (name/value pairs, covering allvnd.amazon.eventstreamheader value types).drain_frameschecks each frame's headers via the newevent_frame_error: if:message-typeis"exception"or"error", it builds anApiError::Apicarrying the:exception-type/:error-codeand a message extracted from the JSON payload (or:error-messageheader), withretryableset for the known-transient exception types (throttling/serviceUnavailable/internalServer/modelTimeout).Test plan
test_parse_event_stream_headers_string_values— header block parsingtest_event_frame_error_none_for_normal_frame— non-error frames are untouchedtest_event_frame_error_surfaces_exception_frame—throttlingExceptionframe surfaces as a retryableApiErrorwith correct type/messagetest_event_frame_error_surfaces_validation_exception_as_non_retryable—validationExceptionsurfaces as non-retryablecargo test -p api(194 passed)cargo fmt --checkcargo clippy -p api --all-targets -- -D warnings