Skip to content

Commit 793876d

Browse files
committed
fix: remove non-EoS body chunk ack that breaks FDS mode for multi-chunk requests
Signed-off-by: Noy Itzikowitz <nitzikow@redhat.com>
1 parent c35395a commit 793876d

2 files changed

Lines changed: 185 additions & 9 deletions

File tree

pkg/handlers/request_test.go

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -886,6 +886,191 @@ func buildStreamingResponse(bodyBytes []byte, setHeaders map[string]string, remo
886886
}
887887
}
888888

889+
// === stripStreamOptions Tests ===
890+
891+
func TestStripStreamOptions(t *testing.T) {
892+
tests := []struct {
893+
name string
894+
input string
895+
wantBody map[string]any
896+
}{
897+
{
898+
name: "removes top-level stream_options key",
899+
input: `{"model":"claude-opus-4-8","messages":[{"role":"user","content":"hi"}],"max_tokens":5,"stream":true,"stream_options":{"include_usage":true}}`,
900+
wantBody: map[string]any{
901+
"model": "claude-opus-4-8",
902+
"messages": []any{map[string]any{"role": "user", "content": "hi"}},
903+
"max_tokens": float64(5),
904+
"stream": true,
905+
},
906+
},
907+
{
908+
name: "no stream_options — body unchanged",
909+
input: `{"model":"claude-opus-4-8","messages":[{"role":"user","content":"hi"}],"max_tokens":5}`,
910+
wantBody: map[string]any{
911+
"model": "claude-opus-4-8",
912+
"messages": []any{map[string]any{"role": "user", "content": "hi"}},
913+
"max_tokens": float64(5),
914+
},
915+
},
916+
{
917+
name: "stream_options text inside message string is preserved",
918+
input: `{"model":"claude-opus-4-8","messages":[{"role":"user","content":"please fix stream_options handling"}],"max_tokens":5,"stream_options":{"include_usage":true}}`,
919+
wantBody: map[string]any{
920+
"model": "claude-opus-4-8",
921+
"messages": []any{map[string]any{"role": "user", "content": "please fix stream_options handling"}},
922+
"max_tokens": float64(5),
923+
},
924+
},
925+
{
926+
name: "only stream_options text inside string — no top-level key to remove",
927+
input: `{"model":"test","messages":[{"role":"user","content":"the stream_options field should not appear"}],"max_tokens":5}`,
928+
wantBody: map[string]any{
929+
"model": "test",
930+
"messages": []any{map[string]any{"role": "user", "content": "the stream_options field should not appear"}},
931+
"max_tokens": float64(5),
932+
},
933+
},
934+
{
935+
name: "stream_options with empty object",
936+
input: `{"model":"test","stream_options":{}}`,
937+
wantBody: map[string]any{
938+
"model": "test",
939+
},
940+
},
941+
{
942+
name: "large body with stream_options",
943+
input: `{"model":"claude-opus-4-8","messages":[{"role":"user","content":"` + strings.Repeat("A", 50000) + `"}],"max_tokens":5,"stream":true,"stream_options":{"include_usage":true}}`,
944+
wantBody: map[string]any{
945+
"model": "claude-opus-4-8",
946+
"messages": []any{map[string]any{"role": "user", "content": strings.Repeat("A", 50000)}},
947+
"max_tokens": float64(5),
948+
"stream": true,
949+
},
950+
},
951+
}
952+
953+
for _, tc := range tests {
954+
t.Run(tc.name, func(t *testing.T) {
955+
result := stripStreamOptions([]byte(tc.input))
956+
957+
var got map[string]any
958+
if err := json.Unmarshal(result, &got); err != nil {
959+
t.Fatalf("failed to unmarshal result: %v", err)
960+
}
961+
962+
if _, exists := got["stream_options"]; exists {
963+
t.Error("stream_options key still present after stripping")
964+
}
965+
966+
wantBytes, _ := json.Marshal(tc.wantBody)
967+
gotBytes, _ := json.Marshal(got)
968+
if string(wantBytes) != string(gotBytes) {
969+
t.Errorf("body mismatch:\nwant: %s\ngot: %s", wantBytes, gotBytes)
970+
}
971+
})
972+
}
973+
974+
t.Run("invalid JSON returns original bytes", func(t *testing.T) {
975+
input := []byte(`not json at all`)
976+
result := stripStreamOptions(input)
977+
if string(result) != string(input) {
978+
t.Errorf("expected original bytes for invalid JSON, got: %s", result)
979+
}
980+
})
981+
982+
t.Run("empty body returns original bytes", func(t *testing.T) {
983+
result := stripStreamOptions([]byte{})
984+
if len(result) != 0 {
985+
t.Errorf("expected empty result for empty input, got: %s", result)
986+
}
987+
})
988+
}
989+
990+
func TestHandleRequestBody_StripsStreamOptions(t *testing.T) {
991+
metrics.Register()
992+
ctx := logutil.NewTestLoggerIntoContext(context.Background())
993+
994+
tests := []struct {
995+
name string
996+
body string
997+
wantBody map[string]any
998+
}{
999+
{
1000+
name: "small body with stream_options is stripped before plugin processing",
1001+
body: `{"model":"claude-opus-4-8","messages":[{"role":"user","content":"hi"}],"max_tokens":5,"stream":true,"stream_options":{"include_usage":true}}`,
1002+
wantBody: map[string]any{
1003+
"model": "claude-opus-4-8",
1004+
"messages": []any{map[string]any{"role": "user", "content": "hi"}},
1005+
"max_tokens": float64(5),
1006+
"stream": true,
1007+
},
1008+
},
1009+
{
1010+
name: "large body with stream_options (simulates multi-chunk Claude Code request)",
1011+
body: `{"model":"claude-opus-4-8","messages":[{"role":"user","content":"` + strings.Repeat("X", 100000) + `"}],"max_tokens":5,"stream":true,"stream_options":{"include_usage":true}}`,
1012+
wantBody: map[string]any{
1013+
"model": "claude-opus-4-8",
1014+
"messages": []any{map[string]any{"role": "user", "content": strings.Repeat("X", 100000)}},
1015+
"max_tokens": float64(5),
1016+
"stream": true,
1017+
},
1018+
},
1019+
{
1020+
name: "body without stream_options passes through unchanged",
1021+
body: `{"model":"claude-opus-4-8","messages":[{"role":"user","content":"hi"}],"max_tokens":5}`,
1022+
wantBody: map[string]any{
1023+
"model": "claude-opus-4-8",
1024+
"messages": []any{map[string]any{"role": "user", "content": "hi"}},
1025+
"max_tokens": float64(5),
1026+
},
1027+
},
1028+
}
1029+
1030+
for _, tc := range tests {
1031+
t.Run(tc.name, func(t *testing.T) {
1032+
profiles := newTestProfiles()
1033+
server := newServerForTest(profiles)
1034+
reqCtx := &RequestContext{
1035+
CycleState: plugin.NewCycleState(),
1036+
Request: requesthandling.NewInferenceRequest(),
1037+
}
1038+
1039+
resp, err := server.HandleRequestBody(ctx, reqCtx, []byte(tc.body))
1040+
if err != nil {
1041+
t.Fatalf("HandleRequestBody returned unexpected error: %v", err)
1042+
}
1043+
1044+
// The parsed body in reqCtx should NOT have stream_options
1045+
if _, exists := reqCtx.Request.Body["stream_options"]; exists {
1046+
t.Error("stream_options still present in parsed request body after HandleRequestBody")
1047+
}
1048+
1049+
// Verify the body in the StreamedBodyResponse doesn't contain stream_options as a JSON key
1050+
for _, r := range resp {
1051+
if rb, ok := r.Response.(*extProcPb.ProcessingResponse_RequestBody); ok {
1052+
if rb.RequestBody != nil && rb.RequestBody.Response != nil {
1053+
if bm := rb.RequestBody.Response.BodyMutation; bm != nil {
1054+
if sr, ok := bm.Mutation.(*extProcPb.BodyMutation_StreamedResponse); ok {
1055+
var bodyMap map[string]any
1056+
if err := json.Unmarshal(sr.StreamedResponse.Body, &bodyMap); err == nil {
1057+
if _, exists := bodyMap["stream_options"]; exists {
1058+
t.Errorf("stream_options found in StreamedBodyResponse body chunk")
1059+
}
1060+
}
1061+
}
1062+
}
1063+
}
1064+
}
1065+
}
1066+
1067+
if len(resp) < 2 {
1068+
t.Fatalf("expected at least 2 responses (headers + body), got %d", len(resp))
1069+
}
1070+
})
1071+
}
1072+
}
1073+
8891074
// === Request Body Tests (body mutations) ===
8901075

8911076
type bodyMutatingPlugin struct {

pkg/handlers/server.go

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -140,15 +140,6 @@ func (s *Server) Process(srv extProcPb.ExternalProcessor_ProcessServer) error {
140140
loggerVerbose.Info("Incoming request body chunk", "EoS", v.RequestBody.EndOfStream, "len", len(v.RequestBody.Body))
141141
requestBody = append(requestBody, v.RequestBody.Body...)
142142
if !v.RequestBody.EndOfStream {
143-
// In STREAMED mode, ack each chunk so envoy sends the next one.
144-
// In FDS mode, this is harmless (envoy doesn't wait for ack).
145-
if err := srv.Send(&extProcPb.ProcessingResponse{
146-
Response: &extProcPb.ProcessingResponse_RequestBody{
147-
RequestBody: &extProcPb.BodyResponse{},
148-
},
149-
}); err != nil {
150-
return status.Errorf(codes.Unknown, "failed to ack request body chunk: %v", err)
151-
}
152143
continue
153144
}
154145
responses, err = s.HandleRequestBody(ctx, reqCtx, requestBody)

0 commit comments

Comments
 (0)