fix(client): make readSSE context-aware to prevent goroutine leaks and HTTP/2 hangs#780
Merged
ezynda3 merged 7 commits intomark3labs:mainfrom Apr 4, 2026
Merged
Conversation
On HTTP/2, resp.Body.Close() blocks in a select waiting for stream cleanup (cs.donec) or context cancellation (cs.ctx.Done()). When cc.wmu is contended, cs.donec may never close, making ctx.Done() the only exit path. The previous defer ordering (LIFO) ran Close() before cancel(), so ctx.Done() never fired — causing an indefinite hang when the MCP server leaves the SSE stream open after sending its response (allowed by the spec: SHOULD, not MUST). Fix: call cancel() before resp.Body.Close() in all three locations: - SendRequest - SendNotification - sendResponseToServer Includes a deterministic reproduction test using a mock transport that simulates the HTTP/2 Close() blocking behavior. Fixes mark3labs#768 Branch-Creation-Time: 2026-03-27T18:18:02+0000
…in test - Return descriptive error when resp is nil instead of wrapping a nil err - Add done channel to blockingSSEReader so goroutines are cleanly unblocked when h2BodySimulator.Close() is called, preventing leaks on test panics
- Delete path/to/your/file.go (placeholder that caused typecheck lint failure) - Delete revert_commit.md (stray file) - Remove unused t *testing.T field from mockH2Transport struct - Add defer transport.Close() after Start() in regression test
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
WalkthroughReorders context cancellation and HTTP response body closing in StreamableHTTP to avoid HTTP/2 hangs; modifies SSE reader to close the reader on context cancellation to unblock blocked reads. Adds deterministic tests simulating HTTP/2 body-close hang and multiple SSE cancellation scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
samkeet
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/transport/streamable_http_test.go (1)
1330-1331: GlobalretryIntervalmutation may cause test interference.Setting
retryInterval = 10 * time.Millisecondmodifies a package-level variable. If tests run in parallel (t.Parallel()), this could cause flaky behavior. The same mutation exists at line 749 inTestContinuousListening.Consider using
t.Cleanupto restore the original value, or pass the interval via a constructor option:♻️ Suggested fix: Restore retryInterval after test
func TestSendRequestSSEStreamStaysOpenWithContinuousListening(t *testing.T) { + origRetryInterval := retryInterval retryInterval = 10 * time.Millisecond + t.Cleanup(func() { retryInterval = origRetryInterval }) + serverURL, closeServer := startMockStatelessSSEServer()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/streamable_http_test.go` around lines 1330 - 1331, The test mutates the package-level retryInterval causing cross-test interference; in TestSendRequestSSEStreamStaysOpenWithContinuousListening (and similarly TestContinuousListening) capture the original value at start, set retryInterval = 10*time.Millisecond for the test, and register t.Cleanup to restore the original retryInterval when the test finishes (or refactor the code to accept an injected retry interval via constructor/option and use that instead of the global). Ensure you reference and update the global symbol retryInterval and the test functions TestSendRequestSSEStreamStaysOpenWithContinuousListening / TestContinuousListening when applying the cleanup or injection fix.client/transport/streamable_http.go (1)
517-523: Goroutine may outlivereadSSEif reader returns EOF before context cancellation.When
readSSEreturns due to EOF (line 543), the goroutine spawned at line 520 continues waiting onctx.Done(). While this is typically cleaned up when the caller's context is eventually cancelled, it creates an unnecessary lingering goroutine between EOF and context cancellation.Consider using a local done channel to signal the goroutine to exit when
readSSEreturns:♻️ Optional: Signal goroutine to exit on readSSE return
func (c *StreamableHTTP) readSSE(ctx context.Context, reader io.ReadCloser, handler func(event, data string)) { + // done signals the close-goroutine to exit when readSSE returns normally. + done := make(chan struct{}) + defer close(done) + // Close the reader when context is cancelled to interrupt blocking reads. // This ensures ReadString returns immediately with an error instead of // blocking indefinitely when the SSE stream is open but idle. go func() { - <-ctx.Done() - reader.Close() + select { + case <-ctx.Done(): + reader.Close() + case <-done: + // readSSE returned normally; no need to close (caller handles it) + } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/streamable_http.go` around lines 517 - 523, The goroutine that closes reader waits only on ctx.Done() and can leak after readSSE returns (e.g., on EOF); modify the logic in readSSE to create a local done channel (e.g., done := make(chan struct{})), have the goroutine select between ctx.Done() and done to decide to close reader, and close(done) right before readSSE returns so the goroutine exits promptly; reference the readSSE function, the reader variable, and ctx.Done() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/transport/streamable_http.go`:
- Around line 686-688: The comment above the deferred resp.Body.Close() is stale
— it says "Cancel the context before closing the body" although this function
doesn't own a cancel function; update the comment to accurately describe
behavior (e.g., "Close response body; context cancellation is the caller's
responsibility" or similar) and remove the reference to cancel(), while
optionally noting similarity to SendRequest/SendNotification but not implying
ownership of ctx; locate the defer containing resp.Body.Close() in this file and
replace the misleading comment accordingly.
---
Nitpick comments:
In `@client/transport/streamable_http_test.go`:
- Around line 1330-1331: The test mutates the package-level retryInterval
causing cross-test interference; in
TestSendRequestSSEStreamStaysOpenWithContinuousListening (and similarly
TestContinuousListening) capture the original value at start, set retryInterval
= 10*time.Millisecond for the test, and register t.Cleanup to restore the
original retryInterval when the test finishes (or refactor the code to accept an
injected retry interval via constructor/option and use that instead of the
global). Ensure you reference and update the global symbol retryInterval and the
test functions TestSendRequestSSEStreamStaysOpenWithContinuousListening /
TestContinuousListening when applying the cleanup or injection fix.
In `@client/transport/streamable_http.go`:
- Around line 517-523: The goroutine that closes reader waits only on ctx.Done()
and can leak after readSSE returns (e.g., on EOF); modify the logic in readSSE
to create a local done channel (e.g., done := make(chan struct{})), have the
goroutine select between ctx.Done() and done to decide to close reader, and
close(done) right before readSSE returns so the goroutine exits promptly;
reference the readSSE function, the reader variable, and ctx.Done() when making
the change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d65d0675-33db-4117-b90f-3411a74a3832
📒 Files selected for processing (3)
client/transport/streamable_http.goclient/transport/streamable_http_bodyclose_test.goclient/transport/streamable_http_test.go
readSSE uses bufio.ReadString which is blocking I/O that does not respect
context cancellation. When an SSE stream is open but idle (e.g. stateless
MCP servers that never send data on GET connections), ReadString blocks
indefinitely. The select{case <-ctx.Done()} check only runs between reads,
not during a blocked read.
Fix: spawn a goroutine that closes the reader when ctx is cancelled. This
causes ReadString to return immediately with an error, allowing readSSE
to exit promptly.
Also fix createGETConnectionToServer to use the same cancel-before-close
pattern as SendRequest and SendNotification, preventing HTTP/2 body drain
hangs on shutdown.
Branch-Creation-Time: 2026-04-01T23:12:16+0000
samkeet
force-pushed
the
fix/context-aware-readsse
branch
from
19725ac to
2e100d6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Resolves #779
readSSEinstreamable_http.gousesbufio.ReadStringwhich is blocking I/O that does not respect Go context cancellation. When an SSE stream is open but idle (e.g. stateless MCP servers that never send data on GET connections, or servers usingtext/event-streamresponses that keep the HTTP stream open after sending a single event),ReadStringblocks indefinitely. The existingselect { case <-ctx.Done() }check only runs between reads, never during a blocked read.This causes:
ReadStringresp.Body.Close()on HTTP/2 tries to drain the stream, blocking indefinitelyClose()cannot cleanly shut downlistenForeverGET connectionsWithContinuousListening: The GET SSE stream is always open, making this the primary hang pathAdditionally,
createGETConnectionToServerhaddefer resp.Body.Close()without cancelling the context first, which can cause HTTP/2 body drain hangs on shutdown.Fix
Close the reader when the context is cancelled, which causes
ReadStringto return immediately with an I/O error:The read loop then checks
ctx.Err()to distinguish context cancellation from real I/O errors. The ineffectiveselect { case <-ctx.Done() }wrapper aroundReadStringis removed since the goroutine approach actually interrupts the blocking call.Also fixed
createGETConnectionToServerto cancel the context before closing the body, matching the pattern used inSendRequestandSendNotification(as established in PR #769).Test
Includes two new tests:
TestReadSSEContextCancellation— deterministic unit test usingio.Pipethat verifiesreadSSEexits promptly when the context is cancelled whileReadStringis blocked. Without the fix this hangs indefinitely; with the fix it passes in <50ms.TestSendRequestSSEStreamStaysOpen— integration test using a mock stateless SSE server that responds withtext/event-stream, sends a single event, but keeps the stream open. Verifies that multipleSendRequestcalls complete without hanging.All existing tests pass with
-race.Dependencies
Summary by CodeRabbit
Release Notes