fix: cancel context before closing body to prevent HTTP/2 hang#769
Merged
ezynda3 merged 6 commits intomark3labs:mainfrom Apr 4, 2026
Merged
fix: cancel context before closing body to prevent HTTP/2 hang#769ezynda3 merged 6 commits intomark3labs:mainfrom
ezynda3 merged 6 commits intomark3labs:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe changes address an HTTP/2 stream cleanup hang by reordering context cancellation relative to response body closure. In 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)
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 |
![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 (1)
client/transport/streamable_http_bodyclose_test.go (1)
88-96: Potential goroutine leak:select {}blocks indefinitely.The empty
select {}at line 91 blocks the goroutine forever with no escape path. While this simulates the intended SSE stream behavior, ifh2BodySimulator.Close()is never called (e.g., due to a test panic), the goroutine will leak.This is acceptable for a focused regression test, but consider adding a context or timeout for safer cleanup if this test grows in scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/streamable_http_bodyclose_test.go` around lines 88 - 96, The empty select in blockingSSEReader.Read blocks forever and can leak a goroutine; add a cancellable signal (e.g., a done chan struct{} or a context.Context field on blockingSSEReader) and have the test harness (h2BodySimulator.Close or equivalent) close/cancel it; then change Read (in blockingSSEReader.Read) to block on that channel/context (select { case <-r.done: return 0, io.EOF; case <-...: }) instead of select {} so the reader can be unblocked and the goroutine cleaned up when Close is called.
🤖 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 287-291: The nil-response branch currently returns the surrounding
err which is guaranteed to be nil there; update the resp==nil handling so it
cancels the context and returns a proper non-nil error (e.g., "failed to send
request: no response received" or similar) instead of fmt.Errorf("failed to send
request: %w", err); locate the resp check and cancel() call in
streamable_http.go and replace the return to return a descriptive fmt.Errorf,
ensuring callers never receive (nil, nil).
---
Nitpick comments:
In `@client/transport/streamable_http_bodyclose_test.go`:
- Around line 88-96: The empty select in blockingSSEReader.Read blocks forever
and can leak a goroutine; add a cancellable signal (e.g., a done chan struct{}
or a context.Context field on blockingSSEReader) and have the test harness
(h2BodySimulator.Close or equivalent) close/cancel it; then change Read (in
blockingSSEReader.Read) to block on that channel/context (select { case
<-r.done: return 0, io.EOF; case <-...: }) instead of select {} so the reader
can be unblocked and the goroutine cleaned up when Close is called.
🪄 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: a962a104-2b85-4903-96bd-f61108590bf9
📒 Files selected for processing (2)
client/transport/streamable_http.goclient/transport/streamable_http_bodyclose_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
client/transport/streamable_http_bodyclose_test.go (2)
110-112: Unusedt *testing.Tfield.The
tfield inmockH2Transportis never used withinRoundTrip. Consider removing it or using it for diagnostic logging if needed.♻️ Suggested fix
type mockH2Transport struct { - t *testing.T }And update the instantiation at line 168:
mockTransport := &mockH2Transport{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/streamable_http_bodyclose_test.go` around lines 110 - 112, The struct mockH2Transport contains an unused field t *testing.T; remove that field from the type definition and update any instantiations (e.g., mockTransport := &mockH2Transport{}) so they no longer attempt to set t; verify RoundTrip (method on mockH2Transport) compiles and uses no testing.T reference after the field is removed.
167-207: Consider addingtransport.Close()cleanup.The test creates a
StreamableHTTPtransport but doesn't close it after the test completes. While this may not cause issues in this specific test since the mock transport doesn't hold real resources, adding cleanup would be more robust and consistent with production usage patterns.♻️ Suggested fix
require.NoError(t, err) require.NoError(t, transport.Start(context.Background())) + defer transport.Close() done := make(chan struct{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/streamable_http_bodyclose_test.go` around lines 167 - 207, The test TestStreamableHTTP_CloseBlocksBeforeCancel creates a StreamableHTTP via NewStreamableHTTP but never calls transport.Close(); update the test to ensure transport is cleaned up by calling transport.Close() (preferably via defer right after Start succeeds) to mirror real usage and release any resources; refer to the transport variable returned by NewStreamableHTTP (and the StreamableHTTP type) and add a defer transport.Close() immediately after require.NoError(t, transport.Start(...)) so the transport is always closed even on test failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/transport/streamable_http_bodyclose_test.go`:
- Around line 110-112: The struct mockH2Transport contains an unused field t
*testing.T; remove that field from the type definition and update any
instantiations (e.g., mockTransport := &mockH2Transport{}) so they no longer
attempt to set t; verify RoundTrip (method on mockH2Transport) compiles and uses
no testing.T reference after the field is removed.
- Around line 167-207: The test TestStreamableHTTP_CloseBlocksBeforeCancel
creates a StreamableHTTP via NewStreamableHTTP but never calls
transport.Close(); update the test to ensure transport is cleaned up by calling
transport.Close() (preferably via defer right after Start succeeds) to mirror
real usage and release any resources; refer to the transport variable returned
by NewStreamableHTTP (and the StreamableHTTP type) and add a defer
transport.Close() immediately after require.NoError(t, transport.Start(...)) so
the transport is always closed even on test failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 80679271-b2b2-4722-8983-7b9a8fa10bfc
📒 Files selected for processing (2)
client/transport/streamable_http.goclient/transport/streamable_http_bodyclose_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/transport/streamable_http.go
samkeet
added a commit
to samkeet/mcp-go
that referenced
this pull request
Mar 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
path/to/your/file.go (1)
7-7: Prefer a sentinel error (and wrap only when propagating underlying errors).Returning
fmt.Errorf("some error")directly makeserrors.Ischecks impossible for callers. Define a package-level sentinel and return it here.Suggested refactor
+var ErrSomeFunction = errors.New("some error") + func SomeFunction() error { ... if someCondition { return nil } - return fmt.Errorf("some error") + return ErrSomeFunction }As per coding guidelines, return sentinel errors and use wrapped errors for contextual propagation (
fmt.Errorf("context: %w", err)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@path/to/your/file.go` at line 7, Define a package-level sentinel error (e.g., var ErrSome = errors.New("some error")) and replace the direct fmt.Errorf("some error") return with returning ErrSome from the function; only use fmt.Errorf("context: %w", err) to wrap when propagating an underlying error, and reference ErrSome in callers with errors.Is to detect this condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@path/to/your/file.go`:
- Around line 1-2: Add proper GoDoc comments for exported functions by adding
comments that start with the function name (e.g., "SomeFunction ..." and
"SomeOtherFunction ...") immediately above their declarations, remove the
unnecessary inline comment inside SomeFunction, and ensure SomeFunction returns
a nil error explicitly (return nil) so the function signature is satisfied;
update the comment text to be concise and descriptive and place it directly
above the corresponding function declarations.
- Around line 3-4: Replace the placeholder ellipses (...) with real code or
remove them so the file compiles, define or replace the undefined someCondition
used in the if statement (or change the if to a valid condition) so it no longer
references an unknown identifier, add the "fmt" import to the file so fmt.Errorf
is available, and update the GoDoc comments for the exported functions
SomeFunction and SomeOtherFunction to start with the function names (e.g., "//
SomeFunction ..." and "// SomeOtherFunction ..."); also remove the unnecessary
inline comment that was on line 5.
---
Nitpick comments:
In `@path/to/your/file.go`:
- Line 7: Define a package-level sentinel error (e.g., var ErrSome =
errors.New("some error")) and replace the direct fmt.Errorf("some error") return
with returning ErrSome from the function; only use fmt.Errorf("context: %w",
err) to wrap when propagating an underlying error, and reference ErrSome in
callers with errors.Is to detect this condition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
|
@samkeet please fix linting errors and tests and address any coderabbit comments. |
ezynda3
added
the
status: needs submitter response
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
samkeet
added a commit
to samkeet/mcp-go
that referenced
this pull request
Apr 1, 2026
samkeet
force-pushed
the
fix/http2-close-before-cancel
branch
from
8fe631d to
4b3d0bd
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/transport/streamable_http_bodyclose_test.go (1)
138-138: Consider handling thejson.Marshalerror.While marshaling a simple map is unlikely to fail, silently ignoring the error is not idiomatic. In test code, you can either check the error or use a fatal assertion.
♻️ Suggested fix
- data, _ := json.Marshal(rpcResp) + data, err := json.Marshal(rpcResp) + if err != nil { + return nil, fmt.Errorf("failed to marshal JSON-RPC response: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/transport/streamable_http_bodyclose_test.go` at line 138, Handle the possible error returned by json.Marshal(rpcResp) instead of ignoring it: capture the error from json.Marshal (when creating variable data from rpcResp) and in the test assert/fail on error (e.g., call t.Fatalf or use your test helper like require.NoError(t, err)) so failures in marshaling surface rather than being silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/transport/streamable_http_bodyclose_test.go`:
- Line 138: Handle the possible error returned by json.Marshal(rpcResp) instead
of ignoring it: capture the error from json.Marshal (when creating variable data
from rpcResp) and in the test assert/fail on error (e.g., call t.Fatalf or use
your test helper like require.NoError(t, err)) so failures in marshaling surface
rather than being silently ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b345dd6e-cde7-4a55-9ce8-e992f660dd2b
📒 Files selected for processing (2)
client/transport/streamable_http.goclient/transport/streamable_http_bodyclose_test.go
✅ Files skipped from review due to trivial changes (1)
- client/transport/streamable_http.go
Contributor
Author
|
@ezynda3 Resolved the comments from CodeRabbit. Ready for review again. Thank you! |
This was referenced Apr 2, 2026
Closed
samkeet
force-pushed
the
fix/http2-close-before-cancel
branch
from
5bb71d2 to
7d14161
Compare
ezynda3
pushed a commit
that referenced
this pull request
Apr 4, 2026
…d HTTP/2 hangs (#780) * fix: cancel context before closing body to prevent HTTP/2 hang 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 #768 Branch-Creation-Time: 2026-03-27T18:18:02+0000 * fix: address review feedback — nil-response error and goroutine leak 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 * Add bug fixes and docstring updates from PR #769 * Revert commit 1a93b65 * fix: address coderabbit review — remove stray files and clean up test - 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 * fix: handle json.Marshal error in mockH2Transport.RoundTrip * fix: make readSSE context-aware to prevent HTTP/2 hangs 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
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
SendRequest,SendNotification, andsendResponseToServercan hang indefinitely on HTTP/2 when the MCP server leaves the SSE stream open after sending its JSON-RPC response.The root cause is Go's LIFO defer ordering:
resp.Body.Close()runs beforecancel(). On HTTP/2,Close()blocks in aselectwaiting forcs.donec(stream cleanup) orcs.ctx.Done()(context cancellation). Whencc.wmuis contended,cs.donecmay never close — andctx.Done()hasn't fired becausecancel()is still waiting its turn.Fix
Call
cancel()beforeresp.Body.Close()so the context is already canceled whenClose()reaches the blocking select:Applied in 3 locations:
SendRequest,SendNotification,sendResponseToServer.Early-return paths call
cancel()explicitly before returning.Test
Includes
TestStreamableHTTP_CloseBlocksBeforeCancel— a deterministic reproduction using a mock transport that simulates the HTTP/2Close()blocking behavior. Without the fix it hangs for 5s then fails; with the fix it passes in <5ms.All existing tests pass with
-race.Fixes #768
Summary by CodeRabbit