fix(transports): drop trailing blank line from SSE heartbeat frame - #5883
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe SSE heartbeat frame changed from ChangesSSE heartbeat compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transports/bifrost-http/lib/streamreader_test.go`:
- Line 6: Update the heartbeat assertion in the stream reader tests to require
the exact frame value ": heartbeat\n" rather than accepting strings with
additional content. Remove the strings import if it is no longer used elsewhere
in the test file.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aace9043-9aeb-4e52-8fdf-ab6c56651096
📒 Files selected for processing (4)
transports/bifrost-http/integrations/router_heartbeat_test.gotransports/bifrost-http/lib/streamreader.gotransports/bifrost-http/lib/streamreader_test.gotransports/changelog.md
The heartbeat's trailing blank line is the SSE event-dispatch signal. Non-conforming decoders (e.g. openai-go ssestream before v3.43.0, fixed in openai/openai-go#621) dispatch an empty event on it, fail to unmarshal the empty payload, and abort the stream with "unexpected end of JSON input" -- so any stream outliving the 1s heartbeat interval died at the first heartbeat for those consumers. A bare comment line accumulates nothing and dispatches nothing in every decoder, while still being a real write on the socket, which is all the disconnect probe and intermediary idle timers need. Fixes maximhq#5874
jeremym-tanium
force-pushed
the
fix/sse-heartbeat-frame
branch
from
August 6, 2026 14:34
b5fb10e to
1f89af6
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 tasks
akshaydeo
approved these changes
Aug 6, 2026
18 tasks
akshaydeo
added a commit
that referenced
this pull request
Aug 7, 2026
## Summary Fixes #5905: SSE heartbeat frames injected mid-line corrupt the `data:` payload on passthrough streams. When Bifrost forwards raw upstream bytes (e.g. via `StreamPassthrough`, which reads into a fixed 4096-byte buffer), chunks are arbitrary TCP slices and are not guaranteed to end on an SSE line boundary. A heartbeat tick landing between two such chunks splices `: heartbeat\n` into the middle of a half-written `data:` line. Per the SSE spec, a comment is only a comment when the colon is the **first character of a line**; spliced mid-line, the `\n` terminates the `data:` line early and the JSON remainder becomes an unrecognized field that decoders silently discard, producing truncated JSON and a `JSONDecodeError` at the client. No heartbeat frame shape can fix this — the reader must refuse to emit a heartbeat unless it is at a line boundary. closes #5905 ## Changes - **`SSEStreamReader`** **now tracks** **`atLineBoundary`**: a mutex-guarded boolean, initialized `true`, updated on every `Send` call based on whether the last byte written was `\n`. The mutex makes the "check position, then write" pair atomic against the concurrent heartbeat goroutine — an atomic flag alone is insufficient because the producer can enqueue a partial line between the check and the write. - **`Send`** **is split into** **`Send`** **(acquires lock) and** **`sendLocked`** **(body)**: `SendHeartbeat` calls `sendLocked` without releasing the lock between the boundary check and the write, closing the race window that caused #5905. - **`SendHeartbeat`** **skips emission when mid-line, but still returns** **`true`**: a skipped heartbeat is not a disconnect. Returning `false` would cause `StartSSEHeartbeat` to invoke `onDisconnect` and cancel a healthy stream. A real disconnect (`closeCh` closed) returns `false` even when mid-line, preserving the proactive disconnect detection from #5010. - **`mcpserver.go`** **switches from a hand-rolled** **`": ping\n\n"`** **byte slice to** **`reader.SendHeartbeat()`**: the old local frame bypassed the line-boundary gate and carried the trailing blank line that #5883 removed (some decoders dispatch it as an empty event). - **Investigate-issue skill updated**: regression tests are now written and confirmed red _before_ the plan is presented to the user, rather than after approval. The checklist, approval gate wording, and Step 7 flow are updated accordingly. A new "Regression Rerun Scope" section (Step 5e) requires coverage-attributed test tiers rather than guessed reruns. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh cd transports/bifrost-http go test ./lib/... -run TestSSEStreamReaderHeartbeat -race -v go test ./lib/... -run TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits -race -count=5 go test ./lib/... -race ``` Key tests added: - `TestSSEStreamReaderHeartbeatNeverSplitsDataLine` — deterministic interleaving: heartbeat tick lands exactly between two chunks of a split `data:` line; asserts structural integrity, byte-for-byte forwarding, and JSON parseability. - `TestSSEStreamReaderHeartbeatStillSentAtLineBoundary` — guards against over-correction by verifying heartbeats are still emitted on the typed paths (where every `Send` ends in `\n\n`). - `TestSSEStreamReaderHeartbeatBoundaryMatrix` — exhaustive matrix of write endings vs. expected `atLineBoundary` state. - `TestSSEStreamReaderWrapperMethodsLeaveBoundary` — all high-level wrapper methods (`SendEvent`, `SendError`, `SendDone`, `SendHeartbeat`) must leave the stream at a boundary. - `TestSSEStreamReaderClosedMidLineReportsDisconnect` — disconnect outranks mid-line skip. - `TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits` — full-fidelity race test with a live `StartSSEHeartbeat` goroutine and co-prime chunk sizes; run under `-race`. - `TestSSEStreamReaderConcurrentProducersWithHeartbeat` — multiple concurrent producers plus a live heartbeat goroutine; asserts no mid-line splice and no deadlock under `-race`. - `TestSSEStreamReaderSkippedHeartbeatIsNotDisconnect` — mid-line skip must return `true`. ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes #5905. Complements #5883 (trailing blank line removal) and #5010 (proactive disconnect detection). ## Security considerations None. This change affects SSE framing only; no auth, secrets, or PII are involved. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
akshaydeo
pushed a commit
that referenced
this pull request
Aug 7, 2026
…5883) The heartbeat's trailing blank line is the SSE event-dispatch signal. Non-conforming decoders (e.g. openai-go ssestream before v3.43.0, fixed in openai/openai-go#621) dispatch an empty event on it, fail to unmarshal the empty payload, and abort the stream with "unexpected end of JSON input" -- so any stream outliving the 1s heartbeat interval died at the first heartbeat for those consumers. A bare comment line accumulates nothing and dispatches nothing in every decoder, while still being a real write on the socket, which is all the disconnect probe and intermediary idle timers need. Fixes #5874
akshaydeo
added a commit
that referenced
this pull request
Aug 7, 2026
## Summary Fixes #5905: SSE heartbeat frames injected mid-line corrupt the `data:` payload on passthrough streams. When Bifrost forwards raw upstream bytes (e.g. via `StreamPassthrough`, which reads into a fixed 4096-byte buffer), chunks are arbitrary TCP slices and are not guaranteed to end on an SSE line boundary. A heartbeat tick landing between two such chunks splices `: heartbeat\n` into the middle of a half-written `data:` line. Per the SSE spec, a comment is only a comment when the colon is the **first character of a line**; spliced mid-line, the `\n` terminates the `data:` line early and the JSON remainder becomes an unrecognized field that decoders silently discard, producing truncated JSON and a `JSONDecodeError` at the client. No heartbeat frame shape can fix this — the reader must refuse to emit a heartbeat unless it is at a line boundary. closes #5905 ## Changes - **`SSEStreamReader`** **now tracks** **`atLineBoundary`**: a mutex-guarded boolean, initialized `true`, updated on every `Send` call based on whether the last byte written was `\n`. The mutex makes the "check position, then write" pair atomic against the concurrent heartbeat goroutine — an atomic flag alone is insufficient because the producer can enqueue a partial line between the check and the write. - **`Send`** **is split into** **`Send`** **(acquires lock) and** **`sendLocked`** **(body)**: `SendHeartbeat` calls `sendLocked` without releasing the lock between the boundary check and the write, closing the race window that caused #5905. - **`SendHeartbeat`** **skips emission when mid-line, but still returns** **`true`**: a skipped heartbeat is not a disconnect. Returning `false` would cause `StartSSEHeartbeat` to invoke `onDisconnect` and cancel a healthy stream. A real disconnect (`closeCh` closed) returns `false` even when mid-line, preserving the proactive disconnect detection from #5010. - **`mcpserver.go`** **switches from a hand-rolled** **`": ping\n\n"`** **byte slice to** **`reader.SendHeartbeat()`**: the old local frame bypassed the line-boundary gate and carried the trailing blank line that #5883 removed (some decoders dispatch it as an empty event). - **Investigate-issue skill updated**: regression tests are now written and confirmed red _before_ the plan is presented to the user, rather than after approval. The checklist, approval gate wording, and Step 7 flow are updated accordingly. A new "Regression Rerun Scope" section (Step 5e) requires coverage-attributed test tiers rather than guessed reruns. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh cd transports/bifrost-http go test ./lib/... -run TestSSEStreamReaderHeartbeat -race -v go test ./lib/... -run TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits -race -count=5 go test ./lib/... -race ``` Key tests added: - `TestSSEStreamReaderHeartbeatNeverSplitsDataLine` — deterministic interleaving: heartbeat tick lands exactly between two chunks of a split `data:` line; asserts structural integrity, byte-for-byte forwarding, and JSON parseability. - `TestSSEStreamReaderHeartbeatStillSentAtLineBoundary` — guards against over-correction by verifying heartbeats are still emitted on the typed paths (where every `Send` ends in `\n\n`). - `TestSSEStreamReaderHeartbeatBoundaryMatrix` — exhaustive matrix of write endings vs. expected `atLineBoundary` state. - `TestSSEStreamReaderWrapperMethodsLeaveBoundary` — all high-level wrapper methods (`SendEvent`, `SendError`, `SendDone`, `SendHeartbeat`) must leave the stream at a boundary. - `TestSSEStreamReaderClosedMidLineReportsDisconnect` — disconnect outranks mid-line skip. - `TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits` — full-fidelity race test with a live `StartSSEHeartbeat` goroutine and co-prime chunk sizes; run under `-race`. - `TestSSEStreamReaderConcurrentProducersWithHeartbeat` — multiple concurrent producers plus a live heartbeat goroutine; asserts no mid-line splice and no deadlock under `-race`. - `TestSSEStreamReaderSkippedHeartbeatIsNotDisconnect` — mid-line skip must return `true`. ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes #5905. Complements #5883 (trailing blank line removal) and #5010 (proactive disconnect detection). ## Security considerations None. This change affects SSE framing only; no auth, secrets, or PII are involved. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
atharvamhaske
pushed a commit
to atharvamhaske/bifrost
that referenced
this pull request
Aug 13, 2026
…aximhq#5883) The heartbeat's trailing blank line is the SSE event-dispatch signal. Non-conforming decoders (e.g. openai-go ssestream before v3.43.0, fixed in openai/openai-go#621) dispatch an empty event on it, fail to unmarshal the empty payload, and abort the stream with "unexpected end of JSON input" -- so any stream outliving the 1s heartbeat interval died at the first heartbeat for those consumers. A bare comment line accumulates nothing and dispatches nothing in every decoder, while still being a real write on the socket, which is all the disconnect probe and intermediary idle timers need. Fixes maximhq#5874
atharvamhaske
pushed a commit
to atharvamhaske/bifrost
that referenced
this pull request
Aug 13, 2026
## Summary Fixes maximhq#5905: SSE heartbeat frames injected mid-line corrupt the `data:` payload on passthrough streams. When Bifrost forwards raw upstream bytes (e.g. via `StreamPassthrough`, which reads into a fixed 4096-byte buffer), chunks are arbitrary TCP slices and are not guaranteed to end on an SSE line boundary. A heartbeat tick landing between two such chunks splices `: heartbeat\n` into the middle of a half-written `data:` line. Per the SSE spec, a comment is only a comment when the colon is the **first character of a line**; spliced mid-line, the `\n` terminates the `data:` line early and the JSON remainder becomes an unrecognized field that decoders silently discard, producing truncated JSON and a `JSONDecodeError` at the client. No heartbeat frame shape can fix this — the reader must refuse to emit a heartbeat unless it is at a line boundary. closes maximhq#5905 ## Changes - **`SSEStreamReader`** **now tracks** **`atLineBoundary`**: a mutex-guarded boolean, initialized `true`, updated on every `Send` call based on whether the last byte written was `\n`. The mutex makes the "check position, then write" pair atomic against the concurrent heartbeat goroutine — an atomic flag alone is insufficient because the producer can enqueue a partial line between the check and the write. - **`Send`** **is split into** **`Send`** **(acquires lock) and** **`sendLocked`** **(body)**: `SendHeartbeat` calls `sendLocked` without releasing the lock between the boundary check and the write, closing the race window that caused maximhq#5905. - **`SendHeartbeat`** **skips emission when mid-line, but still returns** **`true`**: a skipped heartbeat is not a disconnect. Returning `false` would cause `StartSSEHeartbeat` to invoke `onDisconnect` and cancel a healthy stream. A real disconnect (`closeCh` closed) returns `false` even when mid-line, preserving the proactive disconnect detection from maximhq#5010. - **`mcpserver.go`** **switches from a hand-rolled** **`": ping\n\n"`** **byte slice to** **`reader.SendHeartbeat()`**: the old local frame bypassed the line-boundary gate and carried the trailing blank line that maximhq#5883 removed (some decoders dispatch it as an empty event). - **Investigate-issue skill updated**: regression tests are now written and confirmed red _before_ the plan is presented to the user, rather than after approval. The checklist, approval gate wording, and Step 7 flow are updated accordingly. A new "Regression Rerun Scope" section (Step 5e) requires coverage-attributed test tiers rather than guessed reruns. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh cd transports/bifrost-http go test ./lib/... -run TestSSEStreamReaderHeartbeat -race -v go test ./lib/... -run TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits -race -count=5 go test ./lib/... -race ``` Key tests added: - `TestSSEStreamReaderHeartbeatNeverSplitsDataLine` — deterministic interleaving: heartbeat tick lands exactly between two chunks of a split `data:` line; asserts structural integrity, byte-for-byte forwarding, and JSON parseability. - `TestSSEStreamReaderHeartbeatStillSentAtLineBoundary` — guards against over-correction by verifying heartbeats are still emitted on the typed paths (where every `Send` ends in `\n\n`). - `TestSSEStreamReaderHeartbeatBoundaryMatrix` — exhaustive matrix of write endings vs. expected `atLineBoundary` state. - `TestSSEStreamReaderWrapperMethodsLeaveBoundary` — all high-level wrapper methods (`SendEvent`, `SendError`, `SendDone`, `SendHeartbeat`) must leave the stream at a boundary. - `TestSSEStreamReaderClosedMidLineReportsDisconnect` — disconnect outranks mid-line skip. - `TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits` — full-fidelity race test with a live `StartSSEHeartbeat` goroutine and co-prime chunk sizes; run under `-race`. - `TestSSEStreamReaderConcurrentProducersWithHeartbeat` — multiple concurrent producers plus a live heartbeat goroutine; asserts no mid-line splice and no deadlock under `-race`. - `TestSSEStreamReaderSkippedHeartbeatIsNotDisconnect` — mid-line skip must return `true`. ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes maximhq#5905. Complements maximhq#5883 (trailing blank line removal) and maximhq#5010 (proactive disconnect detection). ## Security considerations None. This change affects SSE framing only; no auth, secrets, or PII are involved. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
This was referenced Aug 17, 2026
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
…aximhq#5883) The heartbeat's trailing blank line is the SSE event-dispatch signal. Non-conforming decoders (e.g. openai-go ssestream before v3.43.0, fixed in openai/openai-go#621) dispatch an empty event on it, fail to unmarshal the empty payload, and abort the stream with "unexpected end of JSON input" -- so any stream outliving the 1s heartbeat interval died at the first heartbeat for those consumers. A bare comment line accumulates nothing and dispatches nothing in every decoder, while still being a real write on the socket, which is all the disconnect probe and intermediary idle timers need. Fixes maximhq#5874
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
## Summary Fixes maximhq#5905: SSE heartbeat frames injected mid-line corrupt the `data:` payload on passthrough streams. When Bifrost forwards raw upstream bytes (e.g. via `StreamPassthrough`, which reads into a fixed 4096-byte buffer), chunks are arbitrary TCP slices and are not guaranteed to end on an SSE line boundary. A heartbeat tick landing between two such chunks splices `: heartbeat\n` into the middle of a half-written `data:` line. Per the SSE spec, a comment is only a comment when the colon is the **first character of a line**; spliced mid-line, the `\n` terminates the `data:` line early and the JSON remainder becomes an unrecognized field that decoders silently discard, producing truncated JSON and a `JSONDecodeError` at the client. No heartbeat frame shape can fix this — the reader must refuse to emit a heartbeat unless it is at a line boundary. closes maximhq#5905 ## Changes - **`SSEStreamReader`** **now tracks** **`atLineBoundary`**: a mutex-guarded boolean, initialized `true`, updated on every `Send` call based on whether the last byte written was `\n`. The mutex makes the "check position, then write" pair atomic against the concurrent heartbeat goroutine — an atomic flag alone is insufficient because the producer can enqueue a partial line between the check and the write. - **`Send`** **is split into** **`Send`** **(acquires lock) and** **`sendLocked`** **(body)**: `SendHeartbeat` calls `sendLocked` without releasing the lock between the boundary check and the write, closing the race window that caused maximhq#5905. - **`SendHeartbeat`** **skips emission when mid-line, but still returns** **`true`**: a skipped heartbeat is not a disconnect. Returning `false` would cause `StartSSEHeartbeat` to invoke `onDisconnect` and cancel a healthy stream. A real disconnect (`closeCh` closed) returns `false` even when mid-line, preserving the proactive disconnect detection from maximhq#5010. - **`mcpserver.go`** **switches from a hand-rolled** **`": ping\n\n"`** **byte slice to** **`reader.SendHeartbeat()`**: the old local frame bypassed the line-boundary gate and carried the trailing blank line that maximhq#5883 removed (some decoders dispatch it as an empty event). - **Investigate-issue skill updated**: regression tests are now written and confirmed red _before_ the plan is presented to the user, rather than after approval. The checklist, approval gate wording, and Step 7 flow are updated accordingly. A new "Regression Rerun Scope" section (Step 5e) requires coverage-attributed test tiers rather than guessed reruns. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh cd transports/bifrost-http go test ./lib/... -run TestSSEStreamReaderHeartbeat -race -v go test ./lib/... -run TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits -race -count=5 go test ./lib/... -race ``` Key tests added: - `TestSSEStreamReaderHeartbeatNeverSplitsDataLine` — deterministic interleaving: heartbeat tick lands exactly between two chunks of a split `data:` line; asserts structural integrity, byte-for-byte forwarding, and JSON parseability. - `TestSSEStreamReaderHeartbeatStillSentAtLineBoundary` — guards against over-correction by verifying heartbeats are still emitted on the typed paths (where every `Send` ends in `\n\n`). - `TestSSEStreamReaderHeartbeatBoundaryMatrix` — exhaustive matrix of write endings vs. expected `atLineBoundary` state. - `TestSSEStreamReaderWrapperMethodsLeaveBoundary` — all high-level wrapper methods (`SendEvent`, `SendError`, `SendDone`, `SendHeartbeat`) must leave the stream at a boundary. - `TestSSEStreamReaderClosedMidLineReportsDisconnect` — disconnect outranks mid-line skip. - `TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits` — full-fidelity race test with a live `StartSSEHeartbeat` goroutine and co-prime chunk sizes; run under `-race`. - `TestSSEStreamReaderConcurrentProducersWithHeartbeat` — multiple concurrent producers plus a live heartbeat goroutine; asserts no mid-line splice and no deadlock under `-race`. - `TestSSEStreamReaderSkippedHeartbeatIsNotDisconnect` — mid-line skip must return `true`. ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes maximhq#5905. Complements maximhq#5883 (trailing blank line removal) and maximhq#5010 (proactive disconnect detection). ## Security considerations None. This change affects SSE framing only; no auth, secrets, or PII are involved. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
…aximhq#5883) The heartbeat's trailing blank line is the SSE event-dispatch signal. Non-conforming decoders (e.g. openai-go ssestream before v3.43.0, fixed in openai/openai-go#621) dispatch an empty event on it, fail to unmarshal the empty payload, and abort the stream with "unexpected end of JSON input" -- so any stream outliving the 1s heartbeat interval died at the first heartbeat for those consumers. A bare comment line accumulates nothing and dispatches nothing in every decoder, while still being a real write on the socket, which is all the disconnect probe and intermediary idle timers need. Fixes maximhq#5874
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
## Summary Fixes maximhq#5905: SSE heartbeat frames injected mid-line corrupt the `data:` payload on passthrough streams. When Bifrost forwards raw upstream bytes (e.g. via `StreamPassthrough`, which reads into a fixed 4096-byte buffer), chunks are arbitrary TCP slices and are not guaranteed to end on an SSE line boundary. A heartbeat tick landing between two such chunks splices `: heartbeat\n` into the middle of a half-written `data:` line. Per the SSE spec, a comment is only a comment when the colon is the **first character of a line**; spliced mid-line, the `\n` terminates the `data:` line early and the JSON remainder becomes an unrecognized field that decoders silently discard, producing truncated JSON and a `JSONDecodeError` at the client. No heartbeat frame shape can fix this — the reader must refuse to emit a heartbeat unless it is at a line boundary. closes maximhq#5905 ## Changes - **`SSEStreamReader`** **now tracks** **`atLineBoundary`**: a mutex-guarded boolean, initialized `true`, updated on every `Send` call based on whether the last byte written was `\n`. The mutex makes the "check position, then write" pair atomic against the concurrent heartbeat goroutine — an atomic flag alone is insufficient because the producer can enqueue a partial line between the check and the write. - **`Send`** **is split into** **`Send`** **(acquires lock) and** **`sendLocked`** **(body)**: `SendHeartbeat` calls `sendLocked` without releasing the lock between the boundary check and the write, closing the race window that caused maximhq#5905. - **`SendHeartbeat`** **skips emission when mid-line, but still returns** **`true`**: a skipped heartbeat is not a disconnect. Returning `false` would cause `StartSSEHeartbeat` to invoke `onDisconnect` and cancel a healthy stream. A real disconnect (`closeCh` closed) returns `false` even when mid-line, preserving the proactive disconnect detection from maximhq#5010. - **`mcpserver.go`** **switches from a hand-rolled** **`": ping\n\n"`** **byte slice to** **`reader.SendHeartbeat()`**: the old local frame bypassed the line-boundary gate and carried the trailing blank line that maximhq#5883 removed (some decoders dispatch it as an empty event). - **Investigate-issue skill updated**: regression tests are now written and confirmed red _before_ the plan is presented to the user, rather than after approval. The checklist, approval gate wording, and Step 7 flow are updated accordingly. A new "Regression Rerun Scope" section (Step 5e) requires coverage-attributed test tiers rather than guessed reruns. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh cd transports/bifrost-http go test ./lib/... -run TestSSEStreamReaderHeartbeat -race -v go test ./lib/... -run TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits -race -count=5 go test ./lib/... -race ``` Key tests added: - `TestSSEStreamReaderHeartbeatNeverSplitsDataLine` — deterministic interleaving: heartbeat tick lands exactly between two chunks of a split `data:` line; asserts structural integrity, byte-for-byte forwarding, and JSON parseability. - `TestSSEStreamReaderHeartbeatStillSentAtLineBoundary` — guards against over-correction by verifying heartbeats are still emitted on the typed paths (where every `Send` ends in `\n\n`). - `TestSSEStreamReaderHeartbeatBoundaryMatrix` — exhaustive matrix of write endings vs. expected `atLineBoundary` state. - `TestSSEStreamReaderWrapperMethodsLeaveBoundary` — all high-level wrapper methods (`SendEvent`, `SendError`, `SendDone`, `SendHeartbeat`) must leave the stream at a boundary. - `TestSSEStreamReaderClosedMidLineReportsDisconnect` — disconnect outranks mid-line skip. - `TestSSEStreamReaderHeartbeatRaceAcrossRandomSplits` — full-fidelity race test with a live `StartSSEHeartbeat` goroutine and co-prime chunk sizes; run under `-race`. - `TestSSEStreamReaderConcurrentProducersWithHeartbeat` — multiple concurrent producers plus a live heartbeat goroutine; asserts no mid-line splice and no deadlock under `-race`. - `TestSSEStreamReaderSkippedHeartbeatIsNotDisconnect` — mid-line skip must return `true`. ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes maximhq#5905. Complements maximhq#5883 (trailing blank line removal) and maximhq#5010 (proactive disconnect detection). ## Security considerations None. This change affects SSE framing only; no auth, secrets, or PII are involved. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The SSE heartbeat introduced in #5850 is emitted as
": heartbeat\n\n". The trailing blank line is the SSE event-dispatch signal, and some widely-deployed decoders dispatch an empty event on it: openai-go'sssestream(before v3.43.0, fixed in openai/openai-go#621) falls into its untyped branch, callsjson.Unmarshalon the empty payload, and aborts the stream withunexpected end of JSON input. Net effect: for those consumers, any streaming request outliving the 1s heartbeat interval dies at the first heartbeat. Fast streams are unaffected, which makes this easy to miss.Changes
lib/streamreader.go:sseHeartbeatFramedrops its trailing blank line (": heartbeat\n"). A bare comment line accumulates nothing and dispatches nothing in every decoder — conforming or not — while still being a real write on the socket, which is all the disconnect probe (and intermediary idle timers, per [Feature]: Server-side SSE keepalive (comment heartbeat) to keep long-idle streams alive through intermediaries #5010's original goal) needs. Doc comment explains why the blank line must not come back.lib/streamreader_test.go:TestSSEStreamReaderSendHeartbeatnow pins the frame as a single newline-terminated comment line with no blank line.integrations/router_heartbeat_test.go: asserts the heartbeat appears during idle gaps and never carries a blank line.transports/changelog.md: entry under Fixed + closed-issue link.Type of change
Affected areas
How to test
Expected: all pass.
TestSSEStreamReaderSendHeartbeatfails on the old frame;Test_handleStreamingSSESendsHeartbeatDuringIdleGapconfirms heartbeats still flow during idle gaps.To reproduce the original failure end-to-end: run bifrost-http at
transports/v1.6.8with any streaming provider and issue a streaming request lasting more than ~1s using openai-go < v3.43.0 (e.g. v3.15.0); the stream aborts at the first heartbeat withunexpected end of JSON input. With this fix, the same client streams to completion (verified with both openai-go v3.15.0 and v3.43.0).No new configs or environment variables.
Breaking changes
Related issues
Fixes #5874
Security considerations
None — one byte removed from a server-emitted SSE comment frame.
Checklist
docs/contributing/README.mdand followed the guidelines