fix: return closed non-nil channel for empty streams instead of (nil, nil) - #5556
Conversation
… nil) When a provider's stream closed before the first chunk, CheckFirstStreamChunkForError returned a nil channel with a nil error and executeRequestWithRetries assigned it straight into the public *StreamRequest result. Integrators that range/receive on the returned channel then block forever, because a receive from a nil channel never returns. Substitute a closed, non-nil channel at the call site: range exits immediately, a receive yields (nil, false), and the implicit contract 'nil error implies a usable channel' holds. The empty case keeps the synchronous span-completion path (isStreamChan stays false): the provider goroutine is already gone, so a deferred span would never be completed — this also preserves the existing span behavior for the large-payload passthrough placeholder, which is a closed channel by design. CheckFirstStreamChunkForError is unchanged; its nil return is now genuinely consumed by the caller as the empty-stream signal.
|
|
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughEmpty provider streams now return a closed, non-nil channel instead of ChangesEmpty stream handling
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
… nil) (maximhq#5556) When a provider's stream closed before the first chunk, CheckFirstStreamChunkForError returned a nil channel with a nil error and executeRequestWithRetries assigned it straight into the public *StreamRequest result. Integrators that range/receive on the returned channel then block forever, because a receive from a nil channel never returns. Substitute a closed, non-nil channel at the call site: range exits immediately, a receive yields (nil, false), and the implicit contract 'nil error implies a usable channel' holds. The empty case keeps the synchronous span-completion path (isStreamChan stays false): the provider goroutine is already gone, so a deferred span would never be completed — this also preserves the existing span behavior for the large-payload passthrough placeholder, which is a closed channel by design. CheckFirstStreamChunkForError is unchanged; its nil return is now genuinely consumed by the caller as the empty-stream signal.
… nil) (maximhq#5556) When a provider's stream closed before the first chunk, CheckFirstStreamChunkForError returned a nil channel with a nil error and executeRequestWithRetries assigned it straight into the public *StreamRequest result. Integrators that range/receive on the returned channel then block forever, because a receive from a nil channel never returns. Substitute a closed, non-nil channel at the call site: range exits immediately, a receive yields (nil, false), and the implicit contract 'nil error implies a usable channel' holds. The empty case keeps the synchronous span-completion path (isStreamChan stays false): the provider goroutine is already gone, so a deferred span would never be completed — this also preserves the existing span behavior for the large-payload passthrough placeholder, which is a closed channel by design. CheckFirstStreamChunkForError is unchanged; its nil return is now genuinely consumed by the caller as the empty-stream signal.
Summary
Fixes the public streaming API returning
(nil, nil)for empty streams. When a provider's chunk channel closes before the first chunk,CheckFirstStreamChunkForErrorreturns a nil channel with a nil error, andexecuteRequestWithRetriesassigned that straight into the public*StreamRequestresult — so integrators received a nil channel with no error and blocked forever on the receive (a nil-channel receive never returns).Closes #5555.
Changes
core/bifrost.go(executeRequestWithRetries): when the checked stream comes back nil (empty stream), substitute a closed, non-nil channel as the result. Zero-chunk semantics are preserved —rangeexits immediately, a receive yields(nil, false)— while the implicit contract "nil error ⇒ usable channel" now holds.CheckFirstStreamChunkForError, and the empty case keepsisStreamChan == false, for two reasons:SetupStreamingPassthroughproviders intentionally return an already-closed placeholder channel; changing the helper's return would have silently moved every passthrough request onto the deferred-span path with no completer.CheckFirstStreamChunkForErroritself is unchanged — its nil return is now genuinely consumed by the caller as the empty-stream signal, matching its doc comment.Behavior note (intended): through the HTTP transport, an empty stream previously hit the
stream == nilguard and produced an error response; it now returns a well-formed SSE stream with zero data events, which is the truthful representation of "the provider ended the stream without chunks".Type of change
Affected areas
How to test
TestExecuteRequestWithRetries_EmptyStreamReturnsClosedChannelis a caller-level regression test: it fails on the previous behavior (nil channel) and passes with this change. ExistingTestCheckFirstStreamChunk_*tests are untouched and still green.Breaking changes
Related issues
Closes #5555
Security considerations
None. One extra channel allocation only on the empty-stream path.