fix(openai): use second-to-last SSE as fallback when last frame lacks valid usage#5788
fix(openai): use second-to-last SSE as fallback when last frame lacks valid usage#5788duyiliu wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesSSE penultimate-frame usage fallback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 150-152: The fallback in relay-openai.go can forward a penultimate
usage frame even when usage was not requested, because the normal last-frame
suppression via info.ShouldIncludeUsage is not applied here. Update the fallback
around secondLastStreamData and the stream usage handling logic to mirror the
same ShouldIncludeUsage check used for lastStreamData before emitting buffered
usage metadata, so penultimate usage-only frames are only forwarded when
requested.
- Around line 145-148: The error handling in openai relay currently logs the raw
lastStreamData payload when handleLastResponse fails, which can leak sensitive
streamed content. Update the logging in relayOpenAI.go around handleLastResponse
to remove the verbatim payload and instead log safe metadata such as the payload
length, responseId/model context, and the error message. Keep the fix localized
to the error path in the response handling flow.
🪄 Autofix (Beta)
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
Run ID: 5e9ac7fa-a97b-4f24-8d6d-c101f2a2b1ac
📒 Files selected for processing (1)
relay/channel/openai/relay-openai.go
| if err := handleLastResponse(lastStreamData, &responseId, &createAt, &systemFingerprint, &model, &usage, | ||
| &containStreamUsage, info, &shouldSendLastResp); err != nil { | ||
| logger.LogError(c, fmt.Sprintf("error handling last response: %s, lastStreamData: [%s]", err.Error(), lastStreamData)) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging raw streamed payloads on parse errors.
lastStreamData can contain model output/tool data; logging it verbatim on malformed upstream frames can leak sensitive content. Log metadata such as length instead.
Proposed fix
- logger.LogError(c, fmt.Sprintf("error handling last response: %s, lastStreamData: [%s]", err.Error(), lastStreamData))
+ logger.LogError(c, fmt.Sprintf("error handling last response: %s, lastStreamDataLen: %d", err.Error(), len(lastStreamData)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := handleLastResponse(lastStreamData, &responseId, &createAt, &systemFingerprint, &model, &usage, | |
| &containStreamUsage, info, &shouldSendLastResp); err != nil { | |
| logger.LogError(c, fmt.Sprintf("error handling last response: %s, lastStreamData: [%s]", err.Error(), lastStreamData)) | |
| } | |
| if err := handleLastResponse(lastStreamData, &responseId, &createAt, &systemFingerprint, &model, &usage, | |
| &containStreamUsage, info, &shouldSendLastResp); err != nil { | |
| logger.LogError(c, fmt.Sprintf("error handling last response: %s, lastStreamDataLen: %d", err.Error(), len(lastStreamData))) | |
| } |
🤖 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 145 - 148, The error
handling in openai relay currently logs the raw lastStreamData payload when
handleLastResponse fails, which can leak sensitive streamed content. Update the
logging in relayOpenAI.go around handleLastResponse to remove the verbatim
payload and instead log safe metadata such as the payload length,
responseId/model context, and the error message. Keep the fix localized to the
error path in the response handling flow.
… valid usage
Some upstreams emit a malformed trailing frame (e.g. {"choices":[],"cost":"0"})
followed by TCP EOF without [DONE], causing the last frame to fail ValidUsage.
Previously the audio-only second-to-last fallback was skipped for non-audio models,
triggering ResponseText2Usage local counting and dropping cache_tokens.
This change moves the second-to-last usage extraction after handleLastResponse and
gates it on !containStreamUsage, so it acts as a pure fallback for malformed
streams without altering behavior for normal streams (where the last frame has
valid usage and containStreamUsage is already true).
duyiliu
force-pushed
the
fix/openai-stream-usage-fallback
branch
from
b929bbd to
d19f25c
Compare
duyiliu
left a comment
duyiliu
left a comment
There was a problem hiding this comment.
Thanks for the review! Addressed inline below.
Comment 1 (line 152, ShouldIncludeUsage): ✅ Adopted in commit d19f25c9.
Added && info.ShouldIncludeUsage to the fallback condition so the penultimate-frame usage extraction mirrors the suppression behavior in handleLastResponse. Updated the comment to document the intent. Note: the previously-existing audio-only path did not gate on ShouldIncludeUsage either, so this strictly tightens behavior (clients that did not opt in via stream_options.include_usage will no longer have usage metadata "unlocked" by a malformed last frame). Final usage is still recorded for billing via record consume log regardless, which is the actual fix target.
Comment 2 (line 148, raw lastStreamData in error log):
This log line (logger.LogError(c, fmt.Sprintf("... lastStreamData: [%s]", ..., lastStreamData))) exists verbatim on the original main branch at the same location — it is not introduced by this PR. Agree it's a privacy concern, but conflating it with the cache fix would expand the PR's scope and make review harder. Happy to send it as a follow-up PR (or amend here if maintainer prefers a single commit).
Diff of the amend is +1 net line (the new && info.ShouldIncludeUsage plus 2 lines of comment), same relay/channel/openai/relay-openai.go file.
|
Closing this PR — going back to clean upstream 2d5a041 per local decision. The cache fallback fix (second-to-last SSE) was useful for malformed upstream streams where the last frame lacks valid usage, but the maintainer should review independently before adopting. Leaving the diff in place for reference. Thanks @coderabbitai for the review. |
|
Tip For best results, initiate chat on the files or code changes.
Appreciate the note, and thanks for the thoughtful iteration on this. 🐇✨ |
1 participant
Summary
Some upstreams (e.g. opencode-zen for
minimax-m3) emit a malformed trailing SSEframe such as
{"choices":[],"cost":"0"}followed by TCP EOF without adata: [DONE]sentinel. The actual
usage(includingcached_tokens) is carried in thesecond-to-last frame.
relay/channel/openai/relay-openai.goalready has a second-to-last fallback, butit is gated on
isAudioModel, so it is skipped for non-audio models. In thatcase
handleLastResponsecannot extract a validusagefrom the last frame,containStreamUsagestaysfalse, and the code falls back toservice.ResponseText2Usage(...)which performs local token counting and neverfills
PromptTokensDetails.CachedTokens. Result:cache_tokens=0,local_count_tokens=true,end_reason="eof"in the log — a silent billinginaccuracy for any user of such an upstream.
Reproduction
Upstream emits (3 content chunks + 1 usage chunk + 1 malformed
{"choices":[],"cost":"0"}[DONE]). Before this fix the log line shows:After this fix the same upstream response yields:
(Large prompts also work, e.g.
prompt_tokens=160553 cache_tokens=159730.)Change
relay/channel/openai/relay-openai.go(OaiStreamHandler):secondLastStreamData(previously only whenisAudioModel).usageextraction to afterhandleLastResponse.!containStreamUsageinstead ofisAudioModel.The fallback is a pure pass-through: it only fires when the last frame
ValidUsagecheck failed, so behavior for normal streams (where the last framecarries a valid
usageandcontainStreamUsageis alreadytrue) isunchanged.
Compatibility
containStreamUsage=trueafterhandleLastResponse→ fallback is skipped → identical tomain.code already preferred second-to-last; with this change, the last frame is
preferred when it has valid usage, otherwise the second-to-last is used. The
resulting
usageis the same valid one; the only difference is which frameis preferred when both are valid, which is benign.
correctly instead of being lost to local counting.
Test
Local smoke test on the patched binary (default token, channel 102,
opencode.ai/zen/goforminimax-m3):No errors in
service-3002-stderr.logduring the run.Summary by CodeRabbit