modelcontextprotocol / modelcontextprotocol/go-sdk
sse: Close does not drain incoming; Read can deliver stale messages after Close and POST can return false 202
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 5.1k
- Forks
- 543
- Avg merge
- 1d 17h
- Merged PRs (30d)
- 37
Description
What did you do?
I was reading the HTTP+SSE transport (mcp/sse.go) while evaluating the SDK, and noticed that sseServerConn.Close() closes the done channel but does not drain the buffered incoming channel. incoming has capacity 100 and is documented as intentionally "never closed" (sse.go:126). Both Read and the POST handler (ServeHTTP) use a select over incoming and done, so when a message is buffered and done is closed, the selection is non-deterministic.
I wrote a small reproduction (details below) to confirm.
What did you see?
ReadafterClosereturns stale messages. With one message buffered inincomingbeforeClose, callingReadreturns the buffered message instead ofio.EOFroughly 50% of the time, because Go'sselectpicks randomly among ready cases. I measured 1000/2000 deliveries.POSTafterClosereturns202 Acceptedfor messages that will never be processed. AfterClose,incoming(cap 100) is never drained, so thet.incoming <- msgcase stays ready alongside<-t.done, and the POST handler sometimes returns202. I measured 100/2000 false202responses.
What did you expect to see?
After Close, a Read should return io.EOF (the session is terminated — no post-close message delivery), and a POST should consistently return 400 session closed, since there is no live reader to consume the message. The current behavior gives the client a 202 ack for a message that is silently dropped.
Root cause
sse.go:348-356:
func (s *sseServerConn) Close() error {
s.t.mu.Lock()
defer s.t.mu.Unlock()
if !s.t.closed {
s.t.closed = true
close(s.t.done)
}
return nil
}
Close closes done but leaves buffered messages in incoming. Read (sse.go:307-316):
func (s *sseServerConn) Read(ctx context.Context) (jsonrpc.Message, error) {
select {
case <-ctx.Done():
return nil, ctx.Err()
case msg := <-s.t.incoming:
return msg, nil
case <-s.t.done:
return nil, io.EOF
}
}
When both incoming and done are ready, the choice is random. The POST handler (sse.go:166-171) has the same shape:
select {
case t.incoming <- msg:
w.WriteHeader(http.StatusAccepted)
case <-t.done:
http.Error(w, "session closed", http.StatusBadRequest)
}
Reproduction
Both are deterministic (no -race needed):
// 1. Post-close stale Read delivery (~50%)
w := httptest.NewRecorder()
tr := &SSEServerTransport{Endpoint: "/messages?sessionid=x", Response: w}
conn, _ := tr.Connect(context.Background())
srv := conn.(*sseServerConn)
srv.t.incoming <- &jsonrpc.Request{Method: "ping"}
conn.Close()
// Loop Read 2000x, refilling incoming each time -> ~1000 stale deliveries.
// 2. Post-close false 202 (~5%)
// Close the transport, then POST 2000 messages -> ~100 return 202.
On current main (3d6450f), I measured:
- stale deliveries after
Close: 1000/2000 (~50%, matching the random select) - false
202responses afterClose: 100/2000 (~5%)
Suggested fix
Drain incoming in Close so no buffered message can be observed after close:
if !s.t.closed {
s.t.closed = true
close(s.t.done)
// drain any buffered messages so Read cannot observe them after Close
for {
select {
case <-s.t.incoming:
default:
return
}
}
}
This makes done the only ready case in Read after Close, and forces the POST handler's incoming <- msg to block so the <-t.done case fires and the client gets 400 session closed. An alternative is to check closed in Read/ServeHTTP before selecting on incoming; draining is the smaller change that closes both paths.
I'm happy to send a PR with a regression test, but wanted to file this first and get a read on the approach before working on it (per the contributing guide for changes not related to an existing issue).
Versions
github.com/modelcontextprotocol/go-sdkatmain(3d6450f, 2026-08-21)- Go:
go1.26.1 windows/amd64
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in mcp/sse.go with sseServerConn.Read, Close, and the POST handling in ServeHTTP, then review the existing SSE transport tests. Reproduce the post-close cases described in the issue and add regression coverage. Done means Read consistently returns io.EOF after Close and POST consistently returns 400 session closed instead of acknowledging undeliverable messages.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100