adds EOF handler support to all providers - #5567
Conversation
|
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 (9)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughStreaming handlers across seven providers now detect premature SSE termination, emit structured truncation errors, and avoid duplicate error or finalization handling. Anthropic adds terminal-event tracking, cached-usage normalization, and coverage for truncated and complete streams. ChangesProvider stream termination
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UpstreamSSE
participant StreamingHandler
participant SendStreamTruncatedError
participant Client
UpstreamSSE->>StreamingHandler: SSE events or premature body end
StreamingHandler->>Client: Forward converted stream chunks
StreamingHandler->>SendStreamTruncatedError: Report missing terminal event
SendStreamTruncatedError->>Client: Send structured truncation error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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" Comment |
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 7 seconds. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 57 minutes. |
![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
🧹 Nitpick comments (2)
core/providers/replicate/replicate.go (1)
765-772: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd Replicate-specific truncation coverage.
The listed tests cover Anthropic, while these five guards change Replicate stream semantics. Add deterministic, table-driven tests for EOF before output, EOF mid-stream, and valid
doneevents—covering text/chat plus at least one Responses/image path—and assert exactly one truncation error with no synthetic terminal chunk. Run provider tests withmake test-core.Based on the PR objective, the intended contract is distinguishable truncation without duplicate reporting. As per coding guidelines, behavior changes require deterministic/table-driven coverage and provider-level tests should use
make test-core.Also applies to: 1130-1136, 1711-1717, 2162-2168, 2556-2562
🤖 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 `@core/providers/replicate/replicate.go` around lines 765 - 772, Add deterministic, table-driven Replicate provider tests covering EOF before output, EOF mid-stream, and valid done events across text/chat plus at least one Responses or image path. Assert truncated streams emit exactly one truncation error and no synthetic terminal chunk, while valid done events remain successful. Cover each affected stream guard and run the provider suite with make test-core.Source: Coding guidelines
core/providers/anthropic/streamtruncation_test.go (1)
136-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding equivalent coverage for
HandleAnthropicResponsesStream.These tests only exercise
ChatCompletionStream; the Responses-stream truncation path (anthropic.golines ~1732-1737) has the same detection logic and the flagged usage-normalization gap, but no direct regression test. The existing helpers (anthropicSSEServer,collectTruncationChunks,assertAnthropicTruncationError) could be reused with a Responses-stream request/provider call.🤖 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 `@core/providers/anthropic/streamtruncation_test.go` around lines 136 - 211, Add equivalent truncation regression tests for HandleAnthropicResponsesStream, covering pre-first-byte truncation, mid-stream truncation, and a complete message_stop stream. Reuse anthropicSSEServer, collectTruncationChunks, and assertAnthropicTruncationError, and invoke the Responses-stream request path directly so detection and usage normalization are both exercised.
🤖 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 `@core/providers/anthropic/anthropic.go`:
- Around line 1177-1192: Before each truncated-stream call to
providerUtils.SendStreamTruncatedError, invoke the applicable normalizeUsage()
or normalizeBilledUsage() for the chat and responses streaming paths. Update
both the sawTerminalEvent/fallthrough path and the deferred ctx.Err() path as
needed so cached read/write tokens are folded into billed usage before billing,
while preserving the existing idempotent normalization guards.
In `@core/providers/openai/openai.go`:
- Around line 3692-3693: The non-EOF ReadDataLine error paths must prevent the
later truncation guard from sending a duplicate stream error. Update the
preceding error handling for both core/providers/openai/openai.go:3692-3693 and
core/providers/openai/openai.go:5183-5184 to mark the stream ended or track that
an error was already sent, preserving single-error behavior in both the standard
and image-edit handlers.
---
Nitpick comments:
In `@core/providers/anthropic/streamtruncation_test.go`:
- Around line 136-211: Add equivalent truncation regression tests for
HandleAnthropicResponsesStream, covering pre-first-byte truncation, mid-stream
truncation, and a complete message_stop stream. Reuse anthropicSSEServer,
collectTruncationChunks, and assertAnthropicTruncationError, and invoke the
Responses-stream request path directly so detection and usage normalization are
both exercised.
In `@core/providers/replicate/replicate.go`:
- Around line 765-772: Add deterministic, table-driven Replicate provider tests
covering EOF before output, EOF mid-stream, and valid done events across
text/chat plus at least one Responses or image path. Assert truncated streams
emit exactly one truncation error and no synthetic terminal chunk, while valid
done events remain successful. Cover each affected stream guard and run the
provider suite with make test-core.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 664ec189-3e07-4c59-8485-f2be81937698
📒 Files selected for processing (8)
core/providers/anthropic/anthropic.gocore/providers/anthropic/streamtruncation_test.gocore/providers/azure/azure.gocore/providers/cohere/cohere.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/vllm/vllm.go
52453d7 to
d0c6d7d
Compare
5fab498 to
ac5af2e
Compare
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 `@core/providers/anthropic/streamtruncation_test.go`:
- Around line 172-183: Strengthen the truncation tests around
collectTruncationChunks: in the mid-stream case, require at least one pre-error
chunk containing the expected “partial answer” content before asserting the
terminal Anthropic truncation error; in the complete case, assert the final
chunk is the synthesized terminal-only chunk by requiring its expected usage or
finish metadata, rather than merely checking for a non-nil response.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f6307a20-6095-48d7-95a5-40a57d500209
📒 Files selected for processing (9)
core/providers/anthropic/anthropic.gocore/providers/anthropic/streamtruncation_test.gocore/providers/azure/azure.gocore/providers/cohere/cohere.gocore/providers/mistral/mistral.gocore/providers/openai/openai.gocore/providers/openai/streamtruncation_test.gocore/providers/replicate/replicate.gocore/providers/vllm/vllm.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/providers/mistral/mistral.go
- core/providers/azure/azure.go
- core/providers/vllm/vllm.go
- core/providers/cohere/cohere.go
- core/providers/replicate/replicate.go
- core/providers/anthropic/anthropic.go
- core/providers/openai/openai.go
d0c6d7d to
195b962
Compare
Merge activity
|
The base branch was changed.
195b962 to
359de69
Compare
3 participants
Summary
Streaming providers that die mid-response close the connection with a plain
io.EOF, which is indistinguishable from a healthy close at the transport layer. Previously, Bifrost would silently synthesize a terminal chunk and close the channel, presenting a truncated stream to the caller as if it had completed normally. This PR detects the missing terminal event (e.g.,message_stopfor Anthropic,[DONE]for OpenAI-compatible providers,donefor Replicate) and surfaces a502truncation error on the stream instead, allowing retry logic to act on it.Changes
message_stopwas received. If the loop falls through without it, emit a truncation error instead of the synthesized final chunk. Error paths nowreturnrather thanbreakto prevent the post-loop check from double-reporting.return-instead-of-breakfix on error paths, plus a post-loop truncation check since Cohere's terminal event is the only completion signal.SendStreamTruncatedErrorcall so the caller can observe the truncation.BifrostContextKeyStreamEndIndicatorso already-reported read errors stay quiet.SSEStreamEndedOnMarkerto avoid false positives when[DONE]was received.done-event contract.SSEStreamEndedOnMarker.Type of change
Affected areas
How to test
The three new Anthropic truncation tests spin up an in-process
httptest.Serverthat either drops the connection mid-stream (panic(http.ErrAbortHandler)) or completes normally, and assert that:502truncation error.502truncation error as the final chunk.Breaking changes
Related issues
Closes #5546
Security considerations
None. The change only affects error reporting on the stream channel; no secrets or PII are involved.
Checklist
docs/contributing/README.mdand followed the guidelines