From 1a732770451d61c9910b817bde7a6e30296bccdd Mon Sep 17 00:00:00 2001 From: guglielmoc Date: Wed, 28 Oct 2026 15:25:05 +0000 Subject: [PATCH] refactor: consolidate ErrClientClosing and ErrServerClosing into ErrConnClosing --- internal/jsonrpc2/conn.go | 21 ++++++++++----------- internal/jsonrpc2/conn_test.go | 14 +++++++------- internal/jsonrpc2/wire.go | 9 +++++---- mcp/cmd_test.go | 8 +++----- mcp/shared.go | 2 +- mcp/transport.go | 10 +++++----- mcp/transport_test.go | 19 +++++++++---------- 7 files changed, 40 insertions(+), 43 deletions(-) diff --git a/internal/jsonrpc2/conn.go b/internal/jsonrpc2/conn.go index e91d8e50e..68774fe86 100644 --- a/internal/jsonrpc2/conn.go +++ b/internal/jsonrpc2/conn.go @@ -290,7 +290,7 @@ func (c *Connection) Notify(ctx context.Context, method string, params any) (err // cannot increase once shutdown begins, and allowing outgoing notifications // may permit notifications that will cancel in-flight calls. if len(s.outgoingCalls) == 0 && len(s.incomingByID) == 0 { - err = s.shuttingDown(ErrClientClosing) + err = s.shuttingDown(ErrConnClosing) if err != nil { return } @@ -334,7 +334,7 @@ func (c *Connection) Call(ctx context.Context, method string, params any) *Async } c.updateInFlight(func(s *inFlightState) { - err = s.shuttingDown(ErrClientClosing) + err = s.shuttingDown(ErrConnClosing) if err != nil { return } @@ -539,7 +539,7 @@ func (c *Connection) wait(fromWait bool) error { // // After the start of a Close, notification requests (that lack IDs and do not // receive responses) will continue to be passed to the Preempter, but calls -// with IDs will receive immediate responses with ErrServerClosing, and no new +// with IDs will receive immediate responses with ErrConnClosing, and no new // requests (not even notifications!) will be enqueued to the Handler. func (c *Connection) Close() error { // Stop handling new requests, and interrupt the reader (by closing the @@ -585,16 +585,15 @@ func (c *Connection) readIncoming(ctx context.Context, reader Reader, preempter // Retire any outgoing requests that were still in flight: with the Reader no // longer being processed, they necessarily cannot receive a response. for id, ac := range s.outgoingCalls { - ac.retire(&Response{ID: id, Error: fmt.Errorf("%w: %w", ErrClientClosing, err)}) + ac.retire(&Response{ID: id, Error: fmt.Errorf("%w: %w", ErrConnClosing, err)}) } s.outgoingCalls = nil // Cancel any incoming requests still in flight: with the reader gone we // cannot receive cancellation notifications, and likely cannot write a // response either, so parked handlers have nothing useful left to do. - // The read error (typically io.EOF from the peer disconnecting) is the - // cause, rather than a directional ErrServerClosing/ErrClientClosing - // sentinel: at this layer the connection has no designated end. + // The cause is the read error, typically io.EOF from the peer + // disconnecting. for _, r := range s.incomingByID { r.cancel(err) } @@ -635,7 +634,7 @@ func (c *Connection) acceptRequest(ctx context.Context, msg *Request, preempter // theoretically be handled by the preempter. The preempter could return // ErrAsyncResponse, which would increase the amount of work in flight // when we're trying to ensure that it strictly decreases. - err = s.shuttingDown(ErrServerClosing) + err = s.shuttingDown(ErrConnClosing) } }) if err != nil { @@ -657,7 +656,7 @@ func (c *Connection) acceptRequest(ctx context.Context, msg *Request, preempter // handler — not even notifications. That ensures that if the handler // continues to make progress, it will eventually become idle and // close the connection. - err = s.shuttingDown(ErrServerClosing) + err = s.shuttingDown(ErrConnClosing) if err != nil { return } @@ -797,7 +796,7 @@ func (c *Connection) write(ctx context.Context, msg Message) error { if req, ok := msg.(*Request); ok && !req.IsCall() && s.outgoingNotifications > 0 { return } - err = s.shuttingDown(ErrServerClosing) + err = s.shuttingDown(ErrConnClosing) }) if err == nil { err = c.writer.Write(ctx, msg) @@ -818,7 +817,7 @@ func (c *Connection) write(ctx context.Context, msg Message) error { if s.writeErr == nil { s.writeErr = err for _, r := range s.incomingByID { - r.cancel(fmt.Errorf("%w: %v", ErrServerClosing, err)) + r.cancel(fmt.Errorf("%w: %v", ErrConnClosing, err)) } } }) diff --git a/internal/jsonrpc2/conn_test.go b/internal/jsonrpc2/conn_test.go index 9e77bf0a3..0893953ca 100644 --- a/internal/jsonrpc2/conn_test.go +++ b/internal/jsonrpc2/conn_test.go @@ -88,9 +88,9 @@ func TestUnmarshalableResultSendsError(t *testing.T) { // failure). func TestShuttingDownWrapsWriteError(t *testing.T) { s := &inFlightState{writeErr: io.EOF} - err := s.shuttingDown(ErrServerClosing) - if !errors.Is(err, ErrServerClosing) { - t.Errorf("shuttingDown() error = %v, want it to wrap ErrServerClosing", err) + err := s.shuttingDown(ErrConnClosing) + if !errors.Is(err, ErrConnClosing) { + t.Errorf("shuttingDown() error = %v, want it to wrap ErrConnClosing", err) } if !errors.Is(err, io.EOF) { t.Errorf("shuttingDown() error = %v, want it to wrap io.EOF", err) @@ -99,9 +99,9 @@ func TestShuttingDownWrapsWriteError(t *testing.T) { func TestShuttingDownWrapsReadError(t *testing.T) { s := &inFlightState{readErr: io.EOF} - err := s.shuttingDown(ErrServerClosing) - if !errors.Is(err, ErrServerClosing) { - t.Errorf("shuttingDown() error = %v, want it to wrap ErrServerClosing", err) + err := s.shuttingDown(ErrConnClosing) + if !errors.Is(err, ErrConnClosing) { + t.Errorf("shuttingDown() error = %v, want it to wrap ErrConnClosing", err) } if !errors.Is(err, io.EOF) { t.Errorf("shuttingDown() error = %v, want it to wrap io.EOF", err) @@ -137,7 +137,7 @@ func TestReadErrorRetiresCalls(t *testing.T) { <-w.out fail() err := call.Await(ctx, nil) - for _, want := range []error{ErrClientClosing, readErr} { + for _, want := range []error{ErrConnClosing, readErr} { if !errors.Is(err, want) { t.Errorf("Await() = %v, want an error wrapping %v", err, want) } diff --git a/internal/jsonrpc2/wire.go b/internal/jsonrpc2/wire.go index 499b02637..0d2cd5062 100644 --- a/internal/jsonrpc2/wire.go +++ b/internal/jsonrpc2/wire.go @@ -30,10 +30,11 @@ var ( // ErrUnknown should be used for all non coded errors. ErrUnknown = NewError(-32001, "unknown error") - // ErrServerClosing is returned for calls that arrive while the server is closing. - ErrServerClosing = NewError(-32004, "server is closing") - // ErrClientClosing is a dummy error returned for calls initiated while the client is closing. - ErrClientClosing = NewError(-32003, "client is closing") + // ErrConnClosing is returned for calls and notifications that are sent or + // received while the connection is closing. Calls received while closing + // are answered with it, so it keeps the code that peers already send for + // this condition. + ErrConnClosing = NewError(-32004, "connection is closing") // The following errors have special semantics for MCP transports diff --git a/mcp/cmd_test.go b/mcp/cmd_test.go index 01c171bee..114c2c620 100644 --- a/mcp/cmd_test.go +++ b/mcp/cmd_test.go @@ -94,13 +94,11 @@ func TestStdioCallInterruptedByPeerExit(t *testing.T) { t.Errorf("%s = %v, want an error wrapping %v", name, err, want) } } - for _, closing := range []error{jsonrpc2.ErrClientClosing, jsonrpc2.ErrServerClosing} { - if errors.Is(err, closing) { - t.Errorf("%s = %v, must not wrap %v", name, err, closing) - } + if errors.Is(err, jsonrpc2.ErrConnClosing) { + t.Errorf("%s = %v, must not wrap %v", name, err, jsonrpc2.ErrConnClosing) } var wireErr *jsonrpc2.WireError - if errors.As(err, &wireErr) && (wireErr.Code == -32003 || wireErr.Code == -32004) { + if errors.As(err, &wireErr) && wireErr.Code == -32004 { t.Errorf("%s = %v, exposes internal closing code %d", name, err, wireErr.Code) } } diff --git a/mcp/shared.go b/mcp/shared.go index 386388543..7f5a6abe3 100644 --- a/mcp/shared.go +++ b/mcp/shared.go @@ -161,7 +161,7 @@ func defaultSendingMethodHandler(ctx context.Context, method string, req Request // Notifications don't have results. if strings.HasPrefix(method, "notifications/") { err := req.GetSession().getConn().Notify(ctx, method, params) - if errors.Is(err, jsonrpc2.ErrClientClosing) || errors.Is(err, jsonrpc2.ErrServerClosing) { + if errors.Is(err, jsonrpc2.ErrConnClosing) { if cause := withoutClosingErrors(err); cause != nil { err = fmt.Errorf("%w: %w", ErrConnectionClosed, cause) } else { diff --git a/mcp/transport.go b/mcp/transport.go index 4247bd807..c9695f15e 100644 --- a/mcp/transport.go +++ b/mcp/transport.go @@ -310,7 +310,7 @@ func call(ctx context.Context, conn *jsonrpc2.Connection, method string, params // is nothing to cancel, and the spec permits notifications/cancelled // only for requests believed to still be in progress. return nil - case errors.Is(err, jsonrpc2.ErrClientClosing), errors.Is(err, jsonrpc2.ErrServerClosing): + case errors.Is(err, jsonrpc2.ErrConnClosing): if cause := withoutClosingErrors(err); cause != nil { return fmt.Errorf("%w: calling %q: %w", ErrConnectionClosed, method, cause) } @@ -344,11 +344,11 @@ func call(ctx context.Context, conn *jsonrpc2.Connection, method string, params return nil } -// withoutClosingErrors returns err with jsonrpc2.ErrClientClosing and -// jsonrpc2.ErrServerClosing removed from its tree, or nil if nothing else is -// left. Those errors are internal: wrapping them would expose their codes. +// withoutClosingErrors returns err with jsonrpc2.ErrConnClosing removed from +// its tree, or nil if nothing else is left. That error is internal: wrapping it +// would expose its code. func withoutClosingErrors(err error) error { - if !errors.Is(err, jsonrpc2.ErrClientClosing) && !errors.Is(err, jsonrpc2.ErrServerClosing) { + if !errors.Is(err, jsonrpc2.ErrConnClosing) { return err } switch err := err.(type) { diff --git a/mcp/transport_test.go b/mcp/transport_test.go index c2fb985de..05237728e 100644 --- a/mcp/transport_test.go +++ b/mcp/transport_test.go @@ -108,23 +108,24 @@ type connectionClosedTest struct { func connectionClosedTests() []connectionClosedTest { readErr := errors.New("read failed") wrappedReadErr := fmt.Errorf("reading: %w", readErr) - sameMessageErr := errors.New("client is closing") + sameMessageErr := errors.New("connection is closing") return []connectionClosedTest{ {name: "local close", close: true}, {name: "EOF", readErr: io.EOF, causes: []error{io.EOF}}, {name: "unexpected EOF", readErr: io.ErrUnexpectedEOF, causes: []error{io.ErrUnexpectedEOF}}, {name: "read error", readErr: readErr, causes: []error{readErr}}, {name: "wrapped read error", readErr: wrappedReadErr, causes: []error{wrappedReadErr, readErr}}, - {name: "client closing", readErr: jsonrpc2.NewError(-32003, "client is closing")}, - {name: "server closing", readErr: jsonrpc2.NewError(-32004, "server is closing")}, + {name: "closing", readErr: jsonrpc2.NewError(-32004, "connection is closing")}, + // Older SDK versions send the same code with a different message. + {name: "closing, older message", readErr: jsonrpc2.NewError(-32004, "server is closing")}, { name: "wrapped closing error", - readErr: fmt.Errorf("outer: %w", fmt.Errorf("%w: %w", jsonrpc2.ErrServerClosing, wrappedReadErr)), + readErr: fmt.Errorf("outer: %w", fmt.Errorf("%w: %w", jsonrpc2.ErrConnClosing, wrappedReadErr)), causes: []error{wrappedReadErr, readErr}, }, { name: "joined closing errors", - readErr: errors.Join(jsonrpc2.ErrClientClosing, errors.Join(jsonrpc2.ErrServerClosing, readErr), io.ErrUnexpectedEOF), + readErr: errors.Join(jsonrpc2.ErrConnClosing, errors.Join(jsonrpc2.ErrConnClosing, readErr), io.ErrUnexpectedEOF), causes: []error{readErr, io.ErrUnexpectedEOF}, }, {name: "wire error cause", readErr: jsonrpc2.ErrInvalidParams, causes: []error{jsonrpc2.ErrInvalidParams}}, @@ -139,13 +140,11 @@ func checkConnectionClosedError(t *testing.T, err error, causes ...error) { t.Errorf("error = %v, want an error wrapping %v", err, want) } } - for _, closing := range []error{jsonrpc2.ErrClientClosing, jsonrpc2.ErrServerClosing} { - if errors.Is(err, closing) { - t.Errorf("error = %v, must not wrap %v", err, closing) - } + if errors.Is(err, jsonrpc2.ErrConnClosing) { + t.Errorf("error = %v, must not wrap %v", err, jsonrpc2.ErrConnClosing) } var wireErr *jsonrpc2.WireError - if errors.As(err, &wireErr) && (wireErr.Code == -32003 || wireErr.Code == -32004) { + if errors.As(err, &wireErr) && wireErr.Code == -32004 { t.Errorf("error = %v, exposes internal closing code %d", err, wireErr.Code) } }