fix(router): flush SSE/multipart response head before first message - #3052
Conversation
Subscription responses set the text/event-stream (or multipart) headers but never flushed them, so Go buffered the response head until the first subgraph message. Clients blocked establishing the connection and could time out when no message was streamed within their idle window. Flush the head as soon as the subscription is established.
WalkthroughThe change adds an immediate ChangesImmediate Flush for Streaming Subscriptions
Estimated code review effort: 1 (Trivial) | ~5 minutes Related issues: Suggested reviewers: none identified 🐰 A flush, a hop, a stream held tight, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/core/subscription_response_writer_test.go (1)
130-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a multipart counterpart test.
The fix flushes for both
UseSseandUseMultipart, but the test only covers SSE. Add a symmetric subtest for multipart to guard against regressions in that path too.♻️ Suggested additional subtest
t.Run("flushes the SSE response head before any message is written", func(t *testing.T) { recorder := httptest.NewRecorder() req := httptest.NewRequest(http.MethodPost, "/graphql", nil) req.Header.Set("Accept", sseMimeType) _, _, ok := GetSubscriptionResponseWriter(resolve.NewContext(context.Background()), req, recorder, false) require.True(t, ok) assert.Equal(t, sseMimeType, recorder.Header().Get("Content-Type")) assert.True(t, recorder.Flushed, "expected the SSE response head to be flushed before any message is written") }) + + t.Run("flushes the multipart response head before any message is written", func(t *testing.T) { + recorder := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/graphql", nil) + req.Header.Set("Accept", multipartMimeType) + + _, _, ok := GetSubscriptionResponseWriter(resolve.NewContext(context.Background()), req, recorder, false) + require.True(t, ok) + + assert.True(t, recorder.Flushed, "expected the multipart response head to be flushed before any message is written") + })(Adjust
multipartMimeType/negotiation to match how multipart mode is actually detected inNegotiateSubscriptionParams.)🤖 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 `@router/core/subscription_response_writer_test.go` around lines 130 - 146, The current test in TestGetSubscriptionResponseWriter only verifies the SSE flush path, but the response writer also flushes for multipart subscriptions. Add a symmetric subtest alongside the existing SSE case that exercises the multipart negotiation path used by GetSubscriptionResponseWriter and asserts the Content-Type is multipart and the recorder is flushed before any message is written; use the same test helpers and negotiation behavior already used by NegotiateSubscriptionParams so both UseSse and UseMultipart are covered.
🤖 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.
Nitpick comments:
In `@router/core/subscription_response_writer_test.go`:
- Around line 130-146: The current test in TestGetSubscriptionResponseWriter
only verifies the SSE flush path, but the response writer also flushes for
multipart subscriptions. Add a symmetric subtest alongside the existing SSE case
that exercises the multipart negotiation path used by
GetSubscriptionResponseWriter and asserts the Content-Type is multipart and the
recorder is flushed before any message is written; use the same test helpers and
negotiation behavior already used by NegotiateSubscriptionParams so both UseSse
and UseMultipart are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5309390-787d-4cfd-845c-539b00a701a1
📒 Files selected for processing (2)
router/core/subscription_response_writer.gorouter/core/subscription_response_writer_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3052 +/- ##
==========================================
+ Coverage 61.28% 61.48% +0.20%
==========================================
Files 261 261
Lines 30604 30605 +1
==========================================
+ Hits 18757 18819 +62
+ Misses 10324 10254 -70
- Partials 1523 1532 +9
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Content-Typeheaders but never flushed themFixes #1981.
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.