fix the race condition for remote stream close on context cancel - #3591
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR addresses double-release of fasthttp responses in streaming scenarios by adding context-aware connection-state tracking to ChangesStreaming Response Double-Release Prevention
Minor Formatting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/utils/large_response.go`:
- Around line 48-58: The early-return guard that checks
r.ctx.Value(schemas.BifrostContextKeyConnectionClosed) must be re-evaluated
immediately after calling r.cleanup() inside Close() (and the similar
drain+ReleaseResponse locations) because cleanup() can call stopCancellation()
which may set that context key; change the sequence in Close() (and the other
spots referenced) so that after invoking r.cleanup() you re-check
r.ctx.Value(schemas.BifrostContextKeyConnectionClosed) and if it is true skip
calling fasthttp.ReleaseResponse(r.Resp), set r.Resp = nil and return to avoid a
double-close/race.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db97ff45-664d-4491-ac64-be4d0d009b5e
📒 Files selected for processing (3)
core/providers/utils/large_response.gocore/providers/utils/utils.goui/app/workspace/config/feature-flags/layout.tsx
Confidence Score: 5/5The fix is safe to merge; the reordering of cleanup before the connection-closed flag check is the correct sequencing to guarantee the cancellation goroutine has fully settled before The race being addressed is narrow and well-understood: the cleanup closure synchronously waits for the cancellation goroutine ( The Important Files Changed
Reviews (2): Last reviewed commit: "fix the race condition for remote stream..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
32edcc3 to
d888280
Compare
Merge activity
|
## Summary Fixes a nil-dereference panic in fasthttp's `connsCleaner` that occurred when a mid-stream client disconnect caused `LargeResponseReader.Close` to call `ReleaseResponse` on a connection that had already been torn down by `SetupStreamCancellation`. The fix prevents the double-release by checking `BifrostContextKeyConnectionClosed` before draining and releasing the fasthttp response. ## Changes - `LargeResponseReader` now holds a `*schemas.BifrostContext` reference so that `Close()` can inspect `BifrostContextKeyConnectionClosed` before attempting to drain and release the underlying fasthttp response. When the flag is set (indicating the connection was already closed mid-stream), `Close()` skips the drain and release, leaking `r.Resp` to the GC instead — mirroring the existing behavior of `ReleaseStreamingResponse`. - `SetupStreamCancellation` is now wired into `SetupStreamingPassthrough` so that mid-stream client disconnects unblock the transport's `Read` via `CloseWithError` and set `BifrostContextKeyConnectionClosed` before `LargeResponseReader.Close` runs. - In `SetupStreamCancellation`, `BifrostContextKeyConnectionClosed` is now set unconditionally after a close attempt in the `done`+cancelled-context race branch, regardless of whether the close returned an error. Previously the flag was only set on a successful close, meaning a failed close (e.g. against an already-pooled conn) left the flag unset and allowed a second release to proceed. ## Type of change - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] UI (React) ## How to test Simulate a mid-stream client disconnect against a streaming endpoint and confirm the server does not panic with a nil-dereference in fasthttp's `connsCleaner`. Verify that normal stream completion (EOF) still correctly releases the response. ```sh go version go test ./... ``` ## Breaking changes - [x] No ## Security considerations None. This change only affects connection lifecycle management for streaming responses and does not touch auth, secrets, or PII handling. ## 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
2 participants
Summary
Fixes a nil-dereference panic in fasthttp's
connsCleanerthat occurred when a mid-stream client disconnect causedLargeResponseReader.Closeto callReleaseResponseon a connection that had already been torn down bySetupStreamCancellation. The fix prevents the double-release by checkingBifrostContextKeyConnectionClosedbefore draining and releasing the fasthttp response.Changes
LargeResponseReadernow holds a*schemas.BifrostContextreference so thatClose()can inspectBifrostContextKeyConnectionClosedbefore attempting to drain and release the underlying fasthttp response. When the flag is set (indicating the connection was already closed mid-stream),Close()skips the drain and release, leakingr.Respto the GC instead — mirroring the existing behavior ofReleaseStreamingResponse.SetupStreamCancellationis now wired intoSetupStreamingPassthroughso that mid-stream client disconnects unblock the transport'sReadviaCloseWithErrorand setBifrostContextKeyConnectionClosedbeforeLargeResponseReader.Closeruns.SetupStreamCancellation,BifrostContextKeyConnectionClosedis now set unconditionally after a close attempt in thedone+cancelled-context race branch, regardless of whether the close returned an error. Previously the flag was only set on a successful close, meaning a failed close (e.g. against an already-pooled conn) left the flag unset and allowed a second release to proceed.Type of change
Affected areas
How to test
Simulate a mid-stream client disconnect against a streaming endpoint and confirm the server does not panic with a nil-dereference in fasthttp's
connsCleaner. Verify that normal stream completion (EOF) still correctly releases the response.go version go test ./...Breaking changes
Security considerations
None. This change only affects connection lifecycle management for streaming responses and does not touch auth, secrets, or PII handling.
Checklist
docs/contributing/README.mdand followed the guidelines