Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error when receing an SSE response that does not contain any SSE event #3479

Merged
merged 2 commits into from Mar 20, 2024

Conversation

philipp-spiess
Copy link
Member

We have observed cases where the response of the SSE endpoint did not contain any SSE events (and was thus malformed). This PR makes it so that in this case we still yield an error and log the actual body in verbose mode.

Test plan

  • Apply a change to the sg server like this
diff --git a/internal/completions/httpapi/handler.go b/internal/completions/httpapi/handler.go
index 50fb45bcd77..afc1080a63a 100644
--- a/internal/completions/httpapi/handler.go
+++ b/internal/completions/httpapi/handler.go
@@ -204,6 +204,8 @@ func newSwitchingResponseHandler(logger log.Logger, db database.DB, feature type
        nonStreamer := newNonStreamingResponseHandler(logger, db, feature)
        streamer := newStreamingResponseHandler(logger, db, feature)
        return func(ctx context.Context, requestParams types.CompletionRequestParameters, cc types.CompletionsClient, w http.ResponseWriter, userStore database.UserStore, test guardrails.AttributionTest) {
+               w.Write([]byte("oh this is malformed huh? too bad"))
+               return
                if requestParams.IsStream(feature) {
                        streamer(ctx, requestParams, cc, w, userStore, test)
                } else {
  • Observe that we see an error now instead of a hanging request
Screenshot 2024-03-20 at 16 04 11

@philipp-spiess philipp-spiess requested a review from a team March 20, 2024 15:06
@philipp-spiess philipp-spiess enabled auto-merge (squash) March 20, 2024 15:10
Copy link
Member

@slimsag slimsag left a comment

Choose a reason for hiding this comment

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

🤦 🙏

@philipp-spiess philipp-spiess merged commit 81b1726 into main Mar 20, 2024
18 of 19 checks passed
@philipp-spiess philipp-spiess deleted the ps/error-on-invalid-sse-payload branch March 20, 2024 15:26
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.

None yet

4 participants