mcp: improve http transports error handling and make transport work with any size message #734

alexbumbacea · 2025-12-19T19:51:12Z

We have a use case for messages larger than 1MB, hence making this configurable would help.
In order to discover the source of our problems we had to spend some time investigating, as errors were silently discarded. So we also included changes to improving surfacing errors .

findleyr

Let's discuss this change in #726. I'm not sure we shouldn't just remove the buffering.

mcp/sse.go

mcp/event.go

alexbumbacea · 2026-01-08T08:11:30Z

@findleyr current proposal (no fixed buffer size) approach is inline with most sdks. Can we get this merged?

findleyr · 2026-01-09T04:55:29Z

@alexbumbacea sorry for the delay, will review tomorrow.

mcp/event.go

findleyr · 2026-01-10T03:06:25Z

mcp/event.go

+					yield(Event{}, fmt.Errorf("context done: %w", ctx.Err()))
+					return
+				} else {
+					yield(Event{}, fmt.Errorf("error reading event: %w", err))


I don't think we want to expose these errors in the API: we can just format them with %v.

Not clear why we don't wat to wrap it and instead we just want to print it.

If you wrap an error, then callers can extract the error. That means the error type becomes part of your effective API. Which means you can't change it.

If you just include the error text, you don't have that problem. If later, someone needs to distinguish particular errors, we can design the proper API for them.

findleyr · 2026-01-10T03:34:33Z

mcp/event.go

-	const maxTokenSize = 1 * 1024 * 1024 // 1 MiB max line size
-	scanner.Buffer(nil, maxTokenSize)
+func scanEvents(ctx context.Context, r io.Reader) iter.Seq2[Event, error] {
+	scanner := bufio.NewReader(r)


findleyr · 2026-01-10T03:36:59Z

mcp/event.go

-	scanner := bufio.NewScanner(r)
-	const maxTokenSize = 1 * 1024 * 1024 // 1 MiB max line size
-	scanner.Buffer(nil, maxTokenSize)
+func scanEvents(ctx context.Context, r io.Reader) iter.Seq2[Event, error] {


Why are we adding a ctx parameter? Does not appear to be used?

Used on L117 to stop the iteration on cancellation.

Ack, ok... but is that necessary? Wouldn't a context cancellation close the response body?

findleyr · 2026-01-10T03:42:14Z

mcp/sse.go


-	case data := <-c.incoming:
+	case m := <-c.incoming:
+		if m.err != nil {


Returning an error from Read breaks the connection. I don't think that's actually what we want to happen here, is it?

Given that this method return either an error or a jsonrpc.Message, I think this is an acceptable behaviour.
Considering previous behaviour when there was an error during scan events, parsing the body would stop as the loop over scanEvents would break in case there was an error, but without bubbling up the error.

Sorry, I don't think I sufficiently conveyed the consequences.
The JSON-RPC library expects to operate on a logical stream. Any failure to read from that stream indicates that the logical connection is broken.

So as a consequence of this change, a context cancellation calling a tool will break the entire MCP session.

I think I know the desired behavior: you want to get an error from the CallTool request, but this change doesn't achieve that result.

The jsonrpc2 library doesn't really support this: we'd need to add something like jsonrpc2.Connection.Fail(jsonrpc2.ID, error) to allow this type of layering traversal.

If you'd like to land this PR, I suggest not returning an error here, and I can make the change to achieve what you want.

How is this error different from the error returned a few lines bellow by jsonrpc2.DecodeMessage?

Your assumption is correct, I want to bubble up any errors from lower levels, so that for the caller the problem would be clear enough, but this implementation doesn't seem to be using a jsonrpc2 connection, just the encode/decode part. (this is sse implementation, not jsonrpc).

The jsonrpc2.Connection calls Read, which must have stream semantics: an error from Read breaks the stream.

I think there's an argument to be made that a malformed payload should break the stream: if the server sends something that we can't even parse as a jsonrpc2.Message, then the stream is corrupt.

But if the error is due to a network error or client cancellation, we don't want to terminate the session.

See also #683.

Let's focus on fixing the size limit. We can leave the bubbling up of errors, but should not return an error here. I'll make the change to surface the error to the application layer:it's a subtle change to the jsonrpc2 connection.

jba · 2026-01-10T15:46:14Z

mcp/event.go

+					yield(Event{}, fmt.Errorf("context done: %w", ctx.Err()))
+					return
+				} else {
+					yield(Event{}, fmt.Errorf("error reading event: %w", err))


If you wrap an error, then callers can extract the error. That means the error type becomes part of your effective API. Which means you can't change it.

If you just include the error text, you don't have that problem. If later, someone needs to distinguish particular errors, we can design the proper API for them.

jba · 2026-01-10T15:53:29Z

mcp/event.go

+			return true
+		}
+		for {
+			line, err := scanner.ReadBytes('\n')


Do we think this is a potential DOS attack vector? True, the client is reading from the server, which it presumably trusts in many other ways. But a malicious server could make the client crash by feeding it a large event.

What about a high and configurable upper limit?

it was already discussed in #726 that having this kind of limit would not protect against this in the event the server would present that as a multi "data" event.

Looking into other implementations of the MCP client (other languages and even other written in go) they do not impose such a limitation.

jba · 2026-01-10T15:54:59Z

mcp/event.go

-	scanner := bufio.NewScanner(r)
-	const maxTokenSize = 1 * 1024 * 1024 // 1 MiB max line size
-	scanner.Buffer(nil, maxTokenSize)
+func scanEvents(ctx context.Context, r io.Reader) iter.Seq2[Event, error] {


Used on L117 to stop the iteration on cancellation.

alexbumbacea · 2026-01-13T09:03:39Z

@findleyr all comments have been either replied to or addressed. are there any other concerns?

findleyr · 2026-01-14T03:44:36Z

mcp/event.go

-			if !bytes.Equal(before, dataKey) {
-				flushData()
+			// If we're starting a new event field and we already have an event, emit it first
+			if bytes.Equal(before, eventKey) && !evt.Empty() {


This doesn't look right: the event: field does not need to be the first field in the event. Maybe add a failing test for this.

removed and added tests the scenario you mentioned

findleyr · 2026-01-14T03:45:24Z

mcp/event.go

 			dataBuf *bytes.Buffer // if non-nil, preceding field was also data
 		)
-		flushData := func() {
+		emitEvent := func() bool {


s/emitEvent/yieldEvent

findleyr · 2026-01-14T03:51:35Z

mcp/sse.go


-	case data := <-c.incoming:
+	case m := <-c.incoming:
+		if m.err != nil {


Sorry, I don't think I sufficiently conveyed the consequences.
The JSON-RPC library expects to operate on a logical stream. Any failure to read from that stream indicates that the logical connection is broken.

So as a consequence of this change, a context cancellation calling a tool will break the entire MCP session.

I think I know the desired behavior: you want to get an error from the CallTool request, but this change doesn't achieve that result.

The jsonrpc2 library doesn't really support this: we'd need to add something like jsonrpc2.Connection.Fail(jsonrpc2.ID, error) to allow this type of layering traversal.

If you'd like to land this PR, I suggest not returning an error here, and I can make the change to achieve what you want.

findleyr · 2026-01-14T03:52:17Z

mcp/event.go

-	scanner := bufio.NewScanner(r)
-	const maxTokenSize = 1 * 1024 * 1024 // 1 MiB max line size
-	scanner.Buffer(nil, maxTokenSize)
+func scanEvents(ctx context.Context, r io.Reader) iter.Seq2[Event, error] {


Ack, ok... but is that necessary? Wouldn't a context cancellation close the response body?

mcp/event.go

…igurable

findleyr reviewed Dec 23, 2025

View reviewed changes

mcp/sse.go Outdated Show resolved Hide resolved

mcp/event.go Outdated Show resolved Hide resolved

alexbumbacea force-pushed the handling-large-messages-and-surface-errors branch from 91f93af to ee3d418 Compare December 24, 2025 21:59

alexbumbacea force-pushed the handling-large-messages-and-surface-errors branch from ee3d418 to 85a4340 Compare January 3, 2026 09:48

alexbumbacea changed the title ~~mcp: improve http transports error handling and make buffer size configurable~~ mcp: improve http transports error handling and make transport work with any size message Jan 3, 2026

findleyr reviewed Jan 10, 2026

View reviewed changes

jba reviewed Jan 10, 2026

View reviewed changes

alexbumbacea force-pushed the handling-large-messages-and-surface-errors branch from 85a4340 to 7ba1c9a Compare January 12, 2026 10:44

findleyr reviewed Jan 14, 2026

View reviewed changes

alexbumbacea force-pushed the handling-large-messages-and-surface-errors branch 2 times, most recently from 3577e96 to 377a108 Compare January 14, 2026 07:19

findleyr reviewed Jan 15, 2026

View reviewed changes

mcp/event.go Show resolved Hide resolved

mcp: improve http transports error handling and make buffer size conf…

8f634ce

…igurable

alexbumbacea force-pushed the handling-large-messages-and-surface-errors branch from 377a108 to 8f634ce Compare January 15, 2026 10:28

mcp: improve http transports error handling and make transport work with any size message #734

Are you sure you want to change the base?

mcp: improve http transports error handling and make transport work with any size message #734

Conversation

alexbumbacea commented Dec 19, 2025

Uh oh!

findleyr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexbumbacea commented Jan 8, 2026

Uh oh!

findleyr commented Jan 9, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexbumbacea Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexbumbacea commented Jan 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexbumbacea Jan 12, 2026 •

edited

Loading