Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request improves the handling of incomplete streams by refining the criteria for a completed aggregated response and ensuring that streams ending without a terminal event are correctly marked as failed. Feedback suggests improving observability by logging aggregation errors and reporting the specific error cause when updating the request status.
| if aggErr == nil && meta.ID != "" && len(responseBody) > 0 && isCompletedAggregatedOutboundResponse(meta) { | ||
| log.Debug(ctx, "Stream has valid complete response without terminal event, treating as completed") | ||
| ts.state.StreamCompleted = true | ||
| } |
There was a problem hiding this comment.
If AggregateStreamChunks fails, the error aggErr is currently ignored without any logging. Adding a debug log here would help diagnose why a stream couldn't be treated as completed via aggregation. Also, note that the function name isCompletedAggregatedOutboundResponse is slightly confusing for an inbound stream; consider a more generic name like isCompletedAggregatedResponse in a future refactor.
if aggErr != nil {
log.Debug(ctx, "Failed to aggregate stream chunks", log.Cause(aggErr))
} else if meta.ID != "" && len(responseBody) > 0 && isCompletedAggregatedOutboundResponse(meta) {
log.Debug(ctx, "Stream has valid complete response without terminal event, treating as completed")
ts.state.StreamCompleted = true
}| if ts.request != nil { | ||
| persistCtx := context.WithoutCancel(ctx) | ||
|
|
||
| errToReport := errors.New("stream ended without terminal event or completed response") |
There was a problem hiding this comment.
When the stream ends without a terminal event, it is more informative to report the actual aggregation error (aggErr) if it exists, rather than a generic error message. This ensures that if the stream was corrupted or malformed, the specific error is captured in the request status.
errToReport := aggErr
if errToReport == nil {
errToReport = errors.New("stream ended without terminal event or completed response")
}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Uh oh!
There was an error while loading. Please reload this page.