Skip to content

Conversation

tinco
Copy link
Contributor

@tinco tinco commented Aug 12, 2025

The current StreamError implementation wraps whatever happens in a string, throwing away important underlying information. Specifically this prevents us from catching ContextLengthExceeded errors when streaming is enabled.

This PR rewrites the error handling so that the streaming and non-streaming implementations share the same error parsing code and both return OpenAIError's of which StreamError is now a stream specific subset.

Unfortunately this is a breaking change, any users that explicitly match on StreamError will have to be modified to accommodate the extra information that's now returned in the error.

@@ -20,13 +24,35 @@ pub enum OpenAIError {
FileReadError(String),
/// Error on SSE streaming
#[error("stream failed: {0}")]
StreamError(String),
StreamError(StreamError),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Since you always map the inner eventsource error anyway, It would simplify the code quite a bit I think to wrap StreamError(EventSourceError) instead with #[from].

If I'm not mistaken, that would remove map_stream_error instead as we can use Into, remove the intermediate error, and avoid reading out the bytes of the response early.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't because we need to determine what OpenAIError is wrapped inside the EventSourceError::InvalidStatusCode and as a bonus we also map the EventSourceError::Transport to OpenAIError::Reqwest.

Copy link
Owner

@64bit 64bit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this library should unwrap and match another library's error structure - please consider achieveing same outcome with OpenAIError::StreamError(EventSourceError) or better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants