fix: clear the stream close claim between attempts so streaming retries and fallbacks work

[fus3r]

Summary

Fixes #4788. Since v1.6.0, any streaming retry or fallback that follows a provider error embedded in an HTTP 200 SSE stream was dead on arrival: the new attempt's stream failed every read with stream closed before delivering a single chunk, and the request ultimately failed with the primary error. The reporter hit this with an OpenAI-compatible primary falling back to DeepSeek's Anthropic-compatible endpoint, but the bug is not specific to either provider.

When a stream dies through an SSE-embedded error, CheckFirstStreamChunkForError requalifies it as an attempt failure so retries and fallbacks can run. By then the dead stream's teardown has already run ReleaseStreamingResponse, which since #4678 claims the connection_closed flag on the request's shared context (an atomic guard against double-releasing the fasthttp body stream). That claim is scoped to the response it released, but the flag stayed set on the context. Nothing cleared it between attempts, so the idle-timeout reader of the next attempt's stream treated its own fresh stream as already closed and every read returned stream closed. On v1.5.16 the guard was read-only, which is why the same setup worked there.

Changes

• executeRequestWithRetries clears BifrostContextKeyConnectionClosed when it requalifies a first-chunk error, right after the dead stream's teardown is known to be complete (<-drainDone). This covers both the in-loop retry and the fallback path, which re-enters executeRequestWithRetries per attempt. Clearing is race-free at that point: the drain only completes after the provider goroutine's deferred ReleaseStreamingResponse/CloseStream have run, and the cancellation watcher and idle timer are stopped before that.
• Regression tests in core/streamfallback_test.go cover both paths against local httptest servers: an OpenAI-compatible primary that returns HTTP 200 with an SSE error event, with an Anthropic fallback that streams normally, and a single-provider retry where the first attempt fails the same way. Both fail before the fix (the fallback test fails with the primary error after the fallback stream dies, the retry test fails with Error reading stream: stream closed) and pass with it.
• Changelog entry.

Cancellation semantics are unchanged: a genuinely cancelled request surfaces RequestCancelled, which neither retries nor falls back, and non-streaming requests never take this branch.

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

cd core
go test . -run 'TestStreamFallbackAfterFirstChunkError|TestStreamRetryAfterFirstChunkError' -v

Both tests fail on dev without this change and pass with it. go build ./... passes and the package tests (go test .) are green, including under the race detector. The pre-existing TestResponsesMessageToolCallArguments/real_tool_search_call_frames_from_openai failure in core/schemas fails on dev without this change as well.

Screenshots/Recordings

N/A

Breaking changes

• Yes
• No

Related issues

Closes #4788

Security considerations

None. The flag only coordinates stream teardown ownership within a single request.

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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7f7807b4-9ac8-4f41-8e4f-689ee1fc97b1

📥 Commits

Reviewing files that changed from the base of the PR and between 4b0a641 and 523671a.

📒 Files selected for processing (1)

• core/utils.go

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • Fixed streaming retries and fallback attempts so a prior “connection closed” state no longer leaks into the next attempt, preventing premature termination (including cases involving early SSE provider errors).
  • Improved recovery after an early streaming/SSE error so subsequent retry/fallback attempts can complete successfully.

• Tests

  • Added regression tests covering fallback and retry behavior when the stream errors on the first chunk, asserting correct retry counts and successful streamed output.

Walkthrough

The fix clears the streaming connection-closed context flag after a failed first-chunk attempt is drained, so subsequent retry or fallback streams start cleanly. Regression tests and a changelog entry were added.

Changes

Streaming retry/fallback fix

Layer / File(s)	Summary
Clear connection-closed flag on retry after stream error core/bifrost.go, core/utils.go, core/changelog.md	executeRequestWithRetries and fallback context cleanup both clear schemas.BifrostContextKeyConnectionClosed after a failed stream attempt; the changelog records the fix (closes #4788).
Regression tests for retry and fallback after first-chunk error core/streamfallback_test.go	Adds SSE test handlers, a stream-draining helper, a test client factory, and two tests validating fallback-once and retry-once behavior after an SSE-embedded first-chunk error.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• maximhq/bifrost#3522: Introduces the BifrostContextKeyConnectionClosed flag and related streaming cleanup behavior that this change clears.
• maximhq/bifrost#3582: Adjusts when ReleaseStreamingResponse sets and checks the same connection-closed flag during teardown.
• maximhq/bifrost#3733: Changes handling of the same flag around stream closure and read behavior.

Suggested reviewers: danpiths, akshaydeo

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main fix for clearing the stream-close flag between streaming attempts.
Description check	✅ Passed	The description follows the template and includes the required summary, changes, testing, related issue, and checklist sections.
Linked Issues check	✅ Passed	The changes and tests address #4788 by clearing the shared close flag so fallback and retry streams complete successfully.
Out of Scope Changes check	✅ Passed	The PR changes are limited to the bug fix, regression tests, and a changelog entry, with no unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Confidence Score: 5/5

Safe to merge — the change is a targeted, race-free clear of a per-attempt flag after its owning goroutines have fully exited.

The fix is minimal and surgical: one ClearValue call in bifrost.go (gated behind drainDone, after all dead-stream goroutines have exited) and one in clearCtxForFallback for the cross-provider path. ClearValue is mutex-protected, BifrostContextKeyConnectionClosed is not a reserved key so the clear is never silently dropped, and setting the value to nil correctly resets both the GetAndSetValue atomic claim and the isClosed() read path. Two new httptest-based regression tests cover both the retry and fallback branches deterministically. No streaming teardown invariants, fasthttp acquire/release patterns, or plugin hook ordering are disturbed.

No files require special attention.

Important Files Changed

Filename	Overview
core/bifrost.go	Adds a single ClearValue call for BifrostContextKeyConnectionClosed after drainDone confirms teardown of the failed stream; clear is race-safe because all dead-stream goroutines have exited by then.
core/utils.go	Adds BifrostContextKeyConnectionClosed to clearCtxForFallback alongside other per-attempt context keys; handles the cross-provider fallback path that bypasses the in-loop clear in bifrost.go.
core/streamfallback_test.go	New regression test file with two deterministic httptest-based tests: one for the cross-provider fallback path and one for the in-loop retry path, using SSE error events to trigger the exact bug scenario.
core/changelog.md	Prepends the changelog entry for the streaming retry/fallback bug fix.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant R as executeRequestWithRetries
    participant P as Provider (goroutine)
    participant C as CheckFirstStreamChunkForError
    participant CTX as BifrostContext

    Note over R,CTX: Attempt N — primary fails via SSE-embedded error

    R->>P: requestHandler(currentKey) → streamChan
    P->>CTX: (stream teardown) ReleaseStreamingResponse → GetAndSetValue(connection_closed, true)
    R->>C: CheckFirstStreamChunkForError(ctx, streamChan)
    C-->>R: "firstChunkErr != nil, drainDone chan"

    R->>R: drainDone (all dead-stream goroutines done)
    R->>CTX: ClearValue(connection_closed) NEW FIX
    Note over R,CTX: Flag is nil — next attempt starts clean

    alt In-loop retry
        R->>P: requestHandler(currentKey) → fresh streamChan
        P-->>R: stream reads succeed
    else Cross-provider fallback
        R-->>R: return bifrostError to fallback loop
        R->>CTX: clearCtxForFallback() ClearValue(connection_closed) also added
        R->>P: executeRequestWithRetries(fallback provider)
        P-->>R: stream reads succeed
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant R as executeRequestWithRetries
    participant P as Provider (goroutine)
    participant C as CheckFirstStreamChunkForError
    participant CTX as BifrostContext

    Note over R,CTX: Attempt N — primary fails via SSE-embedded error

    R->>P: requestHandler(currentKey) → streamChan
    P->>CTX: (stream teardown) ReleaseStreamingResponse → GetAndSetValue(connection_closed, true)
    R->>C: CheckFirstStreamChunkForError(ctx, streamChan)
    C-->>R: "firstChunkErr != nil, drainDone chan"

    R->>R: drainDone (all dead-stream goroutines done)
    R->>CTX: ClearValue(connection_closed) NEW FIX
    Note over R,CTX: Flag is nil — next attempt starts clean

    alt In-loop retry
        R->>P: requestHandler(currentKey) → fresh streamChan
        P-->>R: stream reads succeed
    else Cross-provider fallback
        R-->>R: return bifrostError to fallback loop
        R->>CTX: clearCtxForFallback() ClearValue(connection_closed) also added
        R->>P: executeRequestWithRetries(fallback provider)
        P-->>R: stream reads succeed
    end

Loading

_{Reviews (3): Last reviewed commit: "Merge branch 'dev' into fix-stream-close..." | Re-trigger Greptile}

[TejasGhatte]

Hey the fix is at correct place can you also add it in function clearCtxForFallback, then its good to merge

[fus3r]

Done, added the clear to clearCtxForFallback as well.

[CLAassistant]

All committers have signed the CLA.