fix(openai): normalize chat stream termination - #6769
Conversation
WalkthroughThe OpenAI streaming handler now tracks per-choice finish reasons and classifies stream termination. It finalizes responses only for completed streams. Incomplete EOF and timeout cases preserve partial output, record errors, and omit usage and ChangesOpenAI stream completion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ZHOUKAILIAN
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relay/channel/openai/relay-openai.go (1)
137-139: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTrack completion for each choice.
hasOpenAIStreamFinishReasonsetssawFinishReasonwhen any choice has a finish reason. EOF then becomes a successful stream even if another choice has no finish reason.Track started and finished choice indexes. Treat EOF as complete only when every observed choice has a non-empty finish reason. Add a deterministic two-choice EOF regression test where one choice is finished and the other is partial.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/channel/openai/relay-openai.go` around lines 137 - 139, Update the OpenAI stream completion logic around hasOpenAIStreamFinishReason to track observed choice indexes and which of them have non-empty finish reasons, rather than using the single sawFinishReason flag. Treat EOF as successful only when every started choice is finished, and add a deterministic two-choice EOF regression test covering one finished choice and one partial choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@relay/channel/openai/relay-openai.go`:
- Around line 137-139: Update the OpenAI stream completion logic around
hasOpenAIStreamFinishReason to track observed choice indexes and which of them
have non-empty finish reasons, rather than using the single sawFinishReason
flag. Treat EOF as successful only when every started choice is finished, and
add a deterministic two-choice EOF regression test covering one finished choice
and one partial choice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2aaff35-01e3-40ec-a741-d1d7d2198f5e
📒 Files selected for processing (2)
relay/channel/openai/relay-openai.gorelay/channel/openai/relay_openai_stream_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relay/channel/openai/relay-openai.go (1)
259-264: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFlush the buffered choices payload on incomplete streams.
The callback forwards a payload only when the next SSE event arrives. If EOF or timeout follows the final choices payload, Lines 259-264 return without forwarding it. This drops final partial content, including the
"partial"content in the multi-choice test.
relay/channel/openai/relay-openai.go#L259-L264: Before returning, forward the buffered payload when it contains choices. Do not forward usage, callHandleFinalResponse, or emit[DONE].relay/channel/openai/relay_openai_stream_test.go#L262-L264: Assert that"partial"is present. Do not reject the existing"finish_reason":"stop"for choice 0. Assert that no additional synthesized terminal choice is emitted and that[DONE]is absent.As per coding guidelines, backend tests must protect real behavior, API contracts, billing/accounting invariants, compatibility, or regression paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/channel/openai/relay-openai.go` around lines 259 - 264, The incomplete-stream return path in relay/channel/openai/relay-openai.go lines 259-264 must flush the buffered choices payload before returning: forward it only when choices are present, without forwarding usage, calling HandleFinalResponse, or emitting [DONE]. Update relay/channel/openai/relay_openai_stream_test.go lines 262-264 to assert partial content is preserved, retain choice 0’s existing finish_reason stop assertion, and verify no synthesized terminal choice or [DONE] is emitted.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@relay/channel/openai/relay-openai.go`:
- Around line 198-204: Update the synthesizedFinishResponse construction so it
occurs after handleLastResponse has parsed the final payload, or populate the
required metadata beforehand. Ensure Id, Created, Model, and SystemFingerprint
on the synthesized terminal chunk match the preceding completion chunks rather
than using zero values.
---
Outside diff comments:
In `@relay/channel/openai/relay-openai.go`:
- Around line 259-264: The incomplete-stream return path in
relay/channel/openai/relay-openai.go lines 259-264 must flush the buffered
choices payload before returning: forward it only when choices are present,
without forwarding usage, calling HandleFinalResponse, or emitting [DONE].
Update relay/channel/openai/relay_openai_stream_test.go lines 262-264 to assert
partial content is preserved, retain choice 0’s existing finish_reason stop
assertion, and verify no synthesized terminal choice or [DONE] is emitted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 145e614e-0432-403b-838a-0844976c9742
📒 Files selected for processing (2)
relay/channel/openai/relay-openai.gorelay/channel/openai/relay_openai_stream_test.go
| synthesizedFinishResponse = &dto.ChatCompletionsStreamResponse{ | ||
| Id: responseId, | ||
| Object: "chat.completion.chunk", | ||
| Created: createAt, | ||
| Model: model, | ||
| } | ||
| synthesizedFinishResponse.SetSystemFingerprint(systemFingerprint) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Populate synthesized chunk metadata after parsing the last payload.
Lines 198-204 construct synthesizedFinishResponse before Lines 221-225 call handleLastResponse. Therefore, Id, Created, and SystemFingerprint use zero values in the synthesized terminal chunk.
Move the synthesis block after handleLastResponse, or parse this metadata before constructing the response. The synthesized chunk must identify the same completion as preceding chunks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@relay/channel/openai/relay-openai.go` around lines 198 - 204, Update the
synthesizedFinishResponse construction so it occurs after handleLastResponse has
parsed the final payload, or populate the required metadata beforehand. Ensure
Id, Created, Model, and SystemFingerprint on the synthesized terminal chunk
match the preceding completion chunks rather than using zero values.
1 participant
变更描述 / Description
修复 OpenAI Chat Completions 流结束信号不完整时的兼容处理。
[DONE]、但没有非空finish_reason时，补发终止 chunk：普通文本使用stop，工具调用使用tool_calls，再发送 usage 和[DONE]。finish_reason、但省略[DONE]时，保留兼容收尾行为。finish_reason和[DONE]时，不伪造成功结尾，不发送潜在残缺末块、usage 或[DONE]，并记录流异常状态。这样可以兼容严格要求
finish_reason的客户端（包括 Pi），同时避免把真正截断的响应误判成完整对话。本 PR 由 AI 辅助完成，并已由提交者人工检查代码、测试和变更范围。
Type of change
Related Issue
Checklist
[DONE]存在时补终止原因，EOF/timeout 不合成成功信号。Proof of Work
make test通过go test ./relay/... -count=1通过go test -race ./relay/channel/openai -run TestOaiStreamHandler -count=1通过GOWORK=off go vet ./...通过GOWORK=off go build ./...通过GOWORK=off go vet ./...和GOWORK=off go build ./...通过新增回归覆盖：
finish_reason收尾[DONE]缺少finish_reason时补stop[DONE]后存在 usage-only chunk 时先补终止 chunkfinish_reason时补tool_callsSummary by CodeRabbit
Bug Fixes
Tests