streamreader locks to avoid concurrent stream writes - #5927
Merged
akshaydeo merged 1 commit intoAug 7, 2026
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR makes SSE heartbeats line-safe during concurrent raw stream writes. It adds boundary, shutdown, byte-preservation, JSON, and concurrency tests. It also expands the issue investigation workflow with scoped reruns and red-to-green bug validation. ChangesSSE heartbeat safety
Investigation workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPServer
participant RawProducer
participant SSEStreamReader
participant Client
RawProducer->>SSEStreamReader: Send arbitrary stream chunk
MCPServer->>SSEStreamReader: SendHeartbeat()
SSEStreamReader->>SSEStreamReader: Check closure and line boundary
SSEStreamReader->>Client: Send heartbeat only at a safe boundary
SSEStreamReader-->>MCPServer: Return delivery status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
akshaydeo
marked this pull request as ready for review
August 6, 2026 21:46
coderabbitai
Bot
requested review from
Pratham-Mishra04,
TejasGhatte and
roroghost17
August 6, 2026 21:47
![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: 4
🧹 Nitpick comments (1)
.claude/skills/investigate-issue/SKILL.md (1)
457-457: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRequire the local Vitest executable.
When
vitestis not installed locally,npx vitestcan download a mutable package. Use the repository's local executable and fail if it is unavailable.Proposed change
-cd ui && npx vitest related <changed-file> --run +cd ui && npx --no-install vitest related <changed-file> --run🤖 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 @.claude/skills/investigate-issue/SKILL.md at line 457, Update the test command in the investigate-issue instructions to invoke the repository-local Vitest executable directly instead of using npx, so execution fails when Vitest is not installed locally.Source: Linters/SAST tools
🤖 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 @.claude/skills/investigate-issue/SKILL.md:
- Around line 488-489: Update the failure-output guidance in the
investigate-issue workflow to require redacting credentials, authorization
headers, and sensitive response bodies before including test output in reports.
Preserve the raw verbatim output only in a restricted artifact, and ensure the
report’s redacted transcript retains the command, assertion, and relevant stack
trace.
- Around line 433-439: Update “Step 2 -- Find candidate tests” to enumerate
every Test* function across all _test.go files in the changed file’s package and
each identified caller package, rather than relying on single-file grep or
direct TargetSymbol references. Use per-test coverage to classify this complete
test list, including tests that reach the target through helpers or wrappers.
- Around line 18-19: Update the TDD validation guidance in investigate-issue to
use the applicable Makefile target whenever one exists for both failing (red)
and passing (green) tests. Allow raw go test only for the Step 5e coverage
command, while preserving the requirement to include verbatim test output for
bug reports.
- Around line 441-453: Update Step 3’s coverage-checking procedure to inspect
execution counts for the changed line ranges rather than relying on aggregate
output from go tool cover -func. Have the investigator parse each test’s raw
cover-<TestName>.out profile, or use an equivalent per-test line-coverage tool,
and require rerunning tests whose profiles show non-zero counts on the changed
segments.
---
Nitpick comments:
In @.claude/skills/investigate-issue/SKILL.md:
- Line 457: Update the test command in the investigate-issue instructions to
invoke the repository-local Vitest executable directly instead of using npx, so
execution fails when Vitest is not installed locally.
🪄 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: fa09b014-6d22-4122-8db3-983e7f8ee682
📒 Files selected for processing (4)
.claude/skills/investigate-issue/SKILL.mdtransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/lib/streamreader.gotransports/bifrost-http/lib/streamreader_test.go
akshaydeo
force-pushed
the
08-06-streamreader_locks_to_avoid_concurrent_stream_writes
branch
from
August 6, 2026 22:36
bfcad71 to
3404e4f
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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.claude/skills/investigate-issue/SKILL.md:
- Around line 666-669: Update the approval section in the investigation skill so
the “failing test confirmed red” statement is rendered only for Bug issues,
consistent with the red-before-green guidance. Provide Feature and Docs reports
with an accurate issue-type-specific equivalent before the “Implement the fix
now?” approval question, without asserting a failing test for those types.
- Around line 487-490: Update the UI test command in the “For UI changes”
guidance to invoke the locked local Vitest executable via
./node_modules/.bin/vitest or npm exec --offline, preserving the related
<changed-file> --run arguments and avoiding unpinned downloads.
In `@transports/bifrost-http/lib/streamreader_test.go`:
- Around line 265-296: Make both stress tests deterministically prove heartbeat
execution before shutdown: in transports/bifrost-http/lib/streamreader_test.go
lines 265-296, coordinate the producer with the StartSSEHeartbeat callback so
SendHeartbeat runs while raw chunks are forwarded; in lines 338-367, record
callback execution and require at least one invocation before StopSSEHeartbeat.
Remove reliance on timing-only sleeps while preserving the existing corruption
checks.
🪄 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: 3cfb0fd2-a10d-4a8c-aa0e-46efcd50dc38
📒 Files selected for processing (4)
.claude/skills/investigate-issue/SKILL.mdtransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/lib/streamreader.gotransports/bifrost-http/lib/streamreader_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/lib/streamreader.go
- transports/bifrost-http/handlers/mcpserver.go
akshaydeo
force-pushed
the
08-06-streamreader_locks_to_avoid_concurrent_stream_writes
branch
from
August 7, 2026 00:16
3404e4f to
231a381
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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.claude/skills/investigate-issue/SKILL.md:
- Around line 685-702: Update the Bug Todo List in the Step 7 workflow so the
Step 5f “Write failing test” task is marked complete before approval. Make the
implementation fix from Step 7c the first pending task, without instructing the
agent to repeat or modify the already-confirmed regression test.
- Around line 475-479: Update the coverage-report awk command in the
investigate-issue instructions to use the changed-file path directly, removing
the appended “.go” from the file variable while preserving the existing
filtering logic.
In `@transports/bifrost-http/lib/streamreader_test.go`:
- Around line 363-395: Update the heartbeat test around heartbeat, firstBeat,
and the producer WaitGroup so producers remain active while waiting for a
callback that occurs after their event loops begin. Add a deterministic
post-start synchronization signal from the heartbeat callback, wait for that
signal before stopping the heartbeat, and assert it completed; do not rely only
on beats.Load() or the initial firstBeat notification.
🪄 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: f46c04db-d6ca-42cc-b1d2-ca38ea814c7f
📒 Files selected for processing (4)
.claude/skills/investigate-issue/SKILL.mdtransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/lib/streamreader.gotransports/bifrost-http/lib/streamreader_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/handlers/mcpserver.go
- transports/bifrost-http/lib/streamreader.go
akshaydeo
force-pushed
the
08-06-streamreader_locks_to_avoid_concurrent_stream_writes
branch
from
August 7, 2026 00:43
231a381 to
731c2fd
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. |
Contributor
Author
Merge activity
|
akshaydeo
deleted the
08-06-streamreader_locks_to_avoid_concurrent_stream_writes
branch
August 7, 2026 00:52
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
## 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
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
1 participant
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
Fixes #5905: SSE heartbeat frames injected mid-line corrupt the
data:payload on passthrough streams. When Bifrost forwards raw upstream bytes (e.g. viaStreamPassthrough, 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\ninto the middle of a half-writtendata: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\nterminates thedata:line early and the JSON remainder becomes an unrecognized field that decoders silently discard, producing truncated JSON and aJSONDecodeErrorat 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
SSEStreamReadernow tracksatLineBoundary: a mutex-guarded boolean, initializedtrue, updated on everySendcall 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.Sendis split intoSend(acquires lock) andsendLocked(body):SendHeartbeatcallssendLockedwithout releasing the lock between the boundary check and the write, closing the race window that caused [Bug] v1.6.8 raw passthrough heartbeat can split SSE data lines and corrupt JSON #5905.SendHeartbeatskips emission when mid-line, but still returnstrue: a skipped heartbeat is not a disconnect. Returningfalsewould causeStartSSEHeartbeatto invokeonDisconnectand cancel a healthy stream. A real disconnect (closeChclosed) returnsfalseeven when mid-line, preserving the proactive disconnect detection from [Feature]: Server-side SSE keepalive (comment heartbeat) to keep long-idle streams alive through intermediaries #5010.mcpserver.goswitches from a hand-rolled": ping\n\n"byte slice toreader.SendHeartbeat(): the old local frame bypassed the line-boundary gate and carried the trailing blank line that fix(transports): drop trailing blank line from SSE heartbeat frame #5883 removed (some decoders dispatch it as an empty event).Type of change
Affected areas
How to test
Key tests added:
TestSSEStreamReaderHeartbeatNeverSplitsDataLine— deterministic interleaving: heartbeat tick lands exactly between two chunks of a splitdata: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 everySendends in\n\n).TestSSEStreamReaderHeartbeatBoundaryMatrix— exhaustive matrix of write endings vs. expectedatLineBoundarystate.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 liveStartSSEHeartbeatgoroutine 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 returntrue.Breaking changes
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
docs/contributing/README.mdand followed the guidelines