Repository navigation
mcp: prevent a failed request from breaking a stateless streamable client - #1353
jeongukjae wants to merge 1 commit into
Conversation
c3da297 to
8547b3e
Compare
…ient One failed call broke the streamable client connection, even with stateless, even after the server recovered. Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
8547b3e to
d80f8d3
Compare
chrikrah
left a comment
There was a problem hiding this comment.
@jeongukjae your repro breaks the connection in 5 of 6 scenarios with mcp/streamable.go and internal/jsonrpc2/conn.go at the merge base, and in 0 of 6 on your tree. I would merge the fix once the #1118 question below is settled.
$ go run ./tmp/main # your <details> program, go1.25.3, tree at d80f8d3
0 of 6 scenarios broke the connection
$ go run ./tmp/main # both source files at 3c08147 (merge base)
5 of 6 scenarios broke the connection
$ go test ./mcp/ ./internal/jsonrpc2/ && go vet ./mcp/ ./internal/jsonrpc2/ # d80f8d3
ok github.com/modelcontextprotocol/go-sdk/mcp 32.303s
ok github.com/modelcontextprotocol/go-sdk/internal/jsonrpc2 0.004s
$ sed -i 's/if forCall != nil \&\& c.SessionID() == "" {/if false \&\& forCall != nil {/' mcp/streamable.go # d80f8d3, 7 matches
$ # plus the content-type NonFatal return removed
$ go vet ./mcp/ && go test -count=1 ./mcp/
ok github.com/modelcontextprotocol/go-sdk/mcp 32.425s
$ go run ./tmp/main # same tree
2 of 6 scenarios broke the connection
non-blocking: the table only reaches the Write path. The seven guarded branches in handleJSON, handleSSE and processStream and the content-type return have no test. The HTML login page and the JSON-body timeout from your repro would each fit as a row.
non-blocking: NonFatal sits beside ErrRejected and does not replace it. On #683 findleyr proposed a flaky mode in jsonrpc2 that removes ErrRejected.
@maciej-kisiel these six scenarios answer your April question on #683 about concrete errors. @guglielmo-san the PR inverts three rows you added in #1118 (noprotocolerrorbody=1, plain-text 400, bare 404). Is a bare 404 on a 2026-07-28 connection still meant to end it?
Hi all,
I noticed one failed request permanently broke the streamable client connection. The later call failed with connection closed, or client is closing. I strongly believe 1) that the request for stateless client should be independent and 2) #723 fixed all 429 + 5xx cases to make connection broken, but still there's still some gaps like timeout, 4xx, and others.
So what I'm trying to patch in this pr is only making stateless streamable client not to be broken due to previous request. This doesn't change any public interface.
Partially address #683
Tested with