diff --git a/internal/jsonrpc2/conn.go b/internal/jsonrpc2/conn.go index 4994c63b..c7c08ecd 100644 --- a/internal/jsonrpc2/conn.go +++ b/internal/jsonrpc2/conn.go @@ -681,6 +681,13 @@ func (c *Connection) handleAsync() { releaser := &releaser{ch: make(chan struct{})} ctx := context.WithValue(req.ctx, asyncKey, releaser) go func() { + defer func() { + if r := recover(); r != nil { + // A panicking handler must not take down the connection or the + // process; reply with an internal error instead. + c.processResult(c.handler, req, nil, fmt.Errorf("%w: %v", ErrPanic, r)) + } + }() defer releaser.release(true) result, err := c.handler.Handle(ctx, req.Request) c.processResult(c.handler, req, result, err) @@ -694,6 +701,11 @@ func (c *Connection) processResult(from any, req *incomingRequest, result any, e if nomethodnotfoundcodeinerror != "1" && (errors.Is(err, ErrNotHandled) || errors.Is(err, ErrMethodNotFound)) { err = fmt.Errorf("%w: %q", ErrMethodNotFound, req.Method) } + if errors.Is(err, ErrPanic) { + // A panic recovered from the handler is reported as an internal error + // rather than a bare error, so the wire error carries code -32603. + err = fmt.Errorf("%w: %v", ErrInternal, err) + } if result != nil && err != nil { c.internalErrorf("%#v returned a non-nil result with a non-nil error for %s:\n%v\n%#v", from, req.Method, err, result) diff --git a/internal/jsonrpc2/jsonrpc2.go b/internal/jsonrpc2/jsonrpc2.go index 76a51161..3426106a 100644 --- a/internal/jsonrpc2/jsonrpc2.go +++ b/internal/jsonrpc2/jsonrpc2.go @@ -19,6 +19,13 @@ import ( // ErrMethodNotFound. var ErrNotHandled = errors.New("JSON RPC not handled") +// ErrPanic is returned when a Handler panics while handling a request. +// +// If a Handler panics, the server replies with ErrInternal instead of +// terminating the connection, so that a single misbehaving request cannot +// take down the whole server. +var ErrPanic = errors.New("JSON RPC panic") + // Preempter handles messages on a connection before they are queued to the main // handler. // Primarily this is used for cancel handlers or notifications for which out of diff --git a/mcp/error_test.go b/mcp/error_test.go index 2b68d75e..3c6164a3 100644 --- a/mcp/error_test.go +++ b/mcp/error_test.go @@ -15,6 +15,42 @@ import ( "github.com/modelcontextprotocol/go-sdk/jsonrpc" ) +// TestServerToolPanic verifies that a panicking tool handler is contained: +// the client receives an internal error and the connection remains usable. +func TestServerToolPanic(t *testing.T) { + ctx := context.Background() + + cs, _, _ := basicConnection(t, func(s *Server) { + AddTool(s, &Tool{Name: "boom", Description: "panics"}, func(context.Context, *CallToolRequest, map[string]any) (*CallToolResult, any, error) { + var slice []int + _ = slice[42] // panic: index out of range + return nil, nil, nil + }) + AddTool(s, &Tool{Name: "ok", Description: "works"}, func(context.Context, *CallToolRequest, map[string]any) (*CallToolResult, any, error) { + return &CallToolResult{Content: []Content{&TextContent{Text: "fine"}}}, nil, nil + }) + }) + + // The panicking tool must produce an internal error rather than + // killing the server process. + _, err := cs.CallTool(ctx, &CallToolParams{Name: "boom", Arguments: map[string]any{}}) + if err == nil { + t.Fatal("CallTool(boom) = nil error, want internal error") + } + var rpcErr *jsonrpc.Error + if !errors.As(err, &rpcErr) { + t.Fatalf("CallTool(boom) error = %v, want *jsonrpc.Error", err) + } + if rpcErr.Code != jsonrpc.CodeInternalError { + t.Errorf("CallTool(boom) error code = %d, want %d", rpcErr.Code, jsonrpc.CodeInternalError) + } + + // The connection must survive the panic: a subsequent call succeeds. + if _, err := cs.CallTool(ctx, &CallToolParams{Name: "ok", Arguments: map[string]any{}}); err != nil { + t.Errorf("CallTool(ok) after panic = %v, want nil", err) + } +} + // TestServerErrors validates that the server returns appropriate error codes // for various invalid requests. func TestServerErrors(t *testing.T) {