Skip to content

fix done marker for error frame - #5568

Merged
akshaydeo merged 3 commits into
devfrom
07-27-adds_stream_done_marker_for_inference_interaface
Jul 27, 2026
Merged

fix done marker for error frame#5568
akshaydeo merged 3 commits into
devfrom
07-27-adds_stream_done_marker_for_inference_interaface

Conversation

@akshaydeo

@akshaydeo akshaydeo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a bug where a data: [DONE] marker was emitted after an error frame in a streaming response, causing clients keyed on that marker to interpret a failed or truncated stream as a clean, successful completion. This was the root ambiguity behind truncated upstream streams appearing successful (issue #5546).

Changes

  • Introduced a sawErrorChunk boolean in handleStreamingResponse that is set when a chunk carrying a BifrostError is written to the wire. If this flag is set, the [DONE] marker is suppressed at the end of the stream, since the error frame already serves as a terminal signal.
  • The flag is checked after marshalling to avoid suppressing the marker for chunks that never actually reach the client.
  • runCompleter errors are intentionally not factored into this decision — those reflect post-processing or plugin failures, not an incomplete stream, so [DONE] remains accurate in that case.
  • Added streamdonemarker_test.go with two regression tests: one asserting [DONE] is absent after an error frame, and one guarding against over-suppression by confirming clean streams still receive the marker.

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

go test ./transports/bifrost-http/handlers/...

TestStreamingResponseSkipsDoneAfterErrorChunk verifies that a stream ending with an error frame does not emit [DONE].
TestStreamingResponseSendsDoneOnCleanStream verifies that a clean stream still receives [DONE].

Breaking changes

  • Yes
  • No

Related issues

Closes #5546

Security considerations

None.

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Streaming responses now track Bifrost error chunks and omit the terminal [DONE] marker after an error. New in-process SSE tests verify both error-stream suppression and clean-stream completion behavior.

Changes

SSE termination handling

Layer / File(s) Summary
Track streamed error chunks
transports/bifrost-http/handlers/inference.go
handleStreamingResponse records error-containing chunks and requires a clean stream before emitting [DONE].
Validate terminal marker behavior
transports/bifrost-http/handlers/streamdonemarker_test.go
In-process SSE tests verify error content is delivered without [DONE], while clean content streams include [DONE].

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: tejasghatte, bearts, roroghost17

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code matches #5546 by stopping [DONE] after a serialized error frame while preserving clean-stream completion.
Out of Scope Changes check ✅ Passed The changes stay focused on the SSE done-marker bug fix and related regression tests.
Title check ✅ Passed The title is concise and clearly describes the main fix to suppress the DONE marker after error frames.
Description check ✅ Passed The PR description follows the template well, covering summary, changes, testing, impact, related issue, security, and checklist.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 07-27-adds_stream_done_marker_for_inference_interaface

Comment @coderabbitai help to get the list of available commands.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

akshaydeo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

@akshaydeo
akshaydeo marked this pull request as ready for review July 27, 2026 03:13
@akshaydeo akshaydeo changed the title adds stream done marker for inference interaface fix done marker for error frame Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 27, 2026
@akshaydeo
akshaydeo force-pushed the 07-27-adds_eof_handler_support_to_all_providers branch from 52453d7 to d0c6d7d Compare July 27, 2026 06:52
@akshaydeo
akshaydeo force-pushed the 07-27-adds_stream_done_marker_for_inference_interaface branch from 7f02a81 to 85057f8 Compare July 27, 2026 06:52
@akshaydeo
akshaydeo force-pushed the 07-27-adds_eof_handler_support_to_all_providers branch from d0c6d7d to 195b962 Compare July 27, 2026 07:17
@akshaydeo
akshaydeo force-pushed the 07-27-adds_stream_done_marker_for_inference_interaface branch from 85057f8 to 328c763 Compare July 27, 2026 07:17

akshaydeo commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

@akshaydeo
akshaydeo changed the base branch from 07-27-adds_eof_handler_support_to_all_providers to graphite-base/5568 July 27, 2026 09:26
@akshaydeo
akshaydeo changed the base branch from graphite-base/5568 to dev July 27, 2026 09:46
@akshaydeo
akshaydeo dismissed stale reviews from Pratham-Mishra04 and coderabbitai[bot] July 27, 2026 09:46

The base branch was changed.

@akshaydeo
akshaydeo merged commit c32b8d5 into dev Jul 27, 2026
10 of 11 checks passed
@akshaydeo
akshaydeo deleted the 07-27-adds_stream_done_marker_for_inference_interaface branch July 27, 2026 09:47
@coderabbitai coderabbitai Bot mentioned this pull request Aug 4, 2026
8 tasks
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.

[Bug]: Upstream SSE stream death swallowed into a clean [DONE] — dead streams appear successful (v1.5.16 through v1.6.5)

3 participants