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?

mcp/sse.go

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

mcp/sse.go

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

alexbumbacea · 2026-01-20T09:41:18Z

@findleyr is there anything else?

…igurable

alexbumbacea · 2026-01-27T07:40:50Z

@findleyr can we get this merged?

findleyr · 2026-01-27T15:00:53Z

@alexbumbacea very sorry, I thought I had merged this. Total oversight on my part.

@maciej-kisiel we may want to cherry-pick this into the release, or do a second prerelease.

mcp/sse.go

findleyr · 2026-01-27T15:08:00Z

mcp/sse.go


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


Sorry, but returning (nil, nil) here is not correct. Previously the code would simply not receive the message: now it's reading a nil message, which is almost certain to cause a nil pointer exception.

I think we should simply not surface this error to the JSON-RPC layer.

removed the return here, just a TODO comment to have track of the idea that it needs to be handled

This is still not right: we'd proceed to process the nil message.

not surfacing the error at all, would require even more extensive refactoring. the best I can think of it's to wrap the error in a jsonrpc.Response

findleyr · 2026-01-27T15:16:09Z

mcp/streamable.go

+			// Network errors during reading should trigger reconnection, not permanent failure.
+			// Return from processStream so handleSSE can attempt to reconnect.
+			c.logger.Debug(fmt.Sprintf("%s: stream read error (will attempt reconnect): %v", requestSummary, err))
+			return lastEventID, reconnectDelay, false


FWIW, I think we actually need to be more subtle than this: a malformed event is a hard error, and different from a reader error.

did some adaptaptation. let me know WDYT

mcp/event.go

findleyr · 2026-01-27T16:09:55Z

Apologies again for this getting lost in the weeds: I think there are a few things that need to be addressed before merging.

mcp/event.go

findleyr · 2026-01-28T15:56:23Z

mcp/sse.go


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


This is still not right: we'd proceed to process the nil message.

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

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

alexbumbacea force-pushed the handling-large-messages-and-surface-errors branch from 8f634ce to 9fc710b Compare January 20, 2026 13:42

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

9792dd9

…igurable

alexbumbacea force-pushed the handling-large-messages-and-surface-errors branch from 9fc710b to 9792dd9 Compare January 20, 2026 13:44

alexbumbacea mentioned this pull request Jan 20, 2026

Proposal: make maxTokenSize of client transport SSE event handling configurable #726

Open

findleyr reviewed Jan 27, 2026

View reviewed changes

maciej-kisiel mentioned this pull request Jan 28, 2026

Client doesn't accept SSE events with empty data (SEP-1699) #768

Open

alexbumbacea force-pushed the handling-large-messages-and-surface-errors branch from b434d37 to da1ac52 Compare January 28, 2026 11:14

alexbumbacea requested a review from findleyr January 28, 2026 11:15

findleyr reviewed Jan 28, 2026

View reviewed changes

incorporate PR feedback

d534f57

alexbumbacea force-pushed the handling-large-messages-and-surface-errors branch from da1ac52 to d534f57 Compare January 28, 2026 17:04

alexbumbacea requested a review from findleyr January 28, 2026 17:06

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexbumbacea commented Jan 20, 2026

Uh oh!

alexbumbacea commented Jan 27, 2026

Uh oh!

findleyr commented Jan 27, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexbumbacea Jan 28, 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!

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!

findleyr commented Jan 27, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexbumbacea Jan 12, 2026 •

edited

Loading

alexbumbacea Jan 28, 2026 •

edited

Loading