mcp: don't close session when keepalive ping returns method-not-found (#900)
Since ping is optional per the MCP spec, a peer that doesn't implement it will respond with a -32601 (method not found) error. Previously, startKeepalive treated all ping errors identically, closing the session even when the peer was perfectly healthy. Now, a method-not-found response stops the keepalive goroutine without closing the session. Fixes #897
This commit is contained in:
parent
4c00594be6
commit
b37e497f0a
2 changed files with 61 additions and 0 deletions
|
|
@ -25,6 +25,7 @@ import (
|
|||
|
||||
"github.com/google/go-cmp/cmp"
|
||||
"github.com/google/jsonschema-go/jsonschema"
|
||||
"github.com/modelcontextprotocol/go-sdk/internal/jsonrpc2"
|
||||
"github.com/modelcontextprotocol/go-sdk/jsonrpc"
|
||||
)
|
||||
|
||||
|
|
@ -938,6 +939,61 @@ func TestKeepAlive(t *testing.T) {
|
|||
})
|
||||
}
|
||||
|
||||
func TestKeepAliveMethodNotFound(t *testing.T) {
|
||||
synctest.Test(t, func(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
ct, st := NewInMemoryTransports()
|
||||
|
||||
// Server that rejects ping with method-not-found, simulating a
|
||||
// server that does not implement the optional ping method.
|
||||
s := NewServer(testImpl, nil)
|
||||
AddTool(s, greetTool(), sayHi)
|
||||
s.AddReceivingMiddleware(func(next MethodHandler) MethodHandler {
|
||||
return func(ctx context.Context, method string, req Request) (Result, error) {
|
||||
if method == "ping" {
|
||||
return nil, jsonrpc2.ErrMethodNotFound
|
||||
}
|
||||
return next(ctx, method, req)
|
||||
}
|
||||
})
|
||||
ss, err := s.Connect(ctx, st, nil)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
defer ss.Close()
|
||||
|
||||
clientOpts := &ClientOptions{
|
||||
KeepAlive: 50 * time.Millisecond,
|
||||
}
|
||||
c := NewClient(testImpl, clientOpts)
|
||||
cs, err := c.Connect(ctx, ct, nil)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
defer cs.Close()
|
||||
|
||||
// Advance past several keepalive cycles.
|
||||
time.Sleep(200 * time.Millisecond)
|
||||
|
||||
// The session should still be alive despite the server not
|
||||
// supporting ping.
|
||||
result, err := cs.CallTool(ctx, &CallToolParams{
|
||||
Name: "greet",
|
||||
Arguments: map[string]any{"Name": "user"},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("call failed after keepalive with method-not-found: %v", err)
|
||||
}
|
||||
if len(result.Content) == 0 {
|
||||
t.Fatal("expected content in result")
|
||||
}
|
||||
if textContent, ok := result.Content[0].(*TextContent); !ok || textContent.Text != "hi user" {
|
||||
t.Fatalf("unexpected result: %v", result.Content[0])
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestElicitationUnsupportedMethod(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
ct, st := NewInMemoryTransports()
|
||||
|
|
|
|||
|
|
@ -14,6 +14,7 @@ package mcp
|
|||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"net/http"
|
||||
|
|
@ -605,6 +606,10 @@ func startKeepalive(session keepaliveSession, interval time.Duration, cancelPtr
|
|||
err := session.Ping(pingCtx, nil)
|
||||
pingCancel()
|
||||
if err != nil {
|
||||
if errors.Is(err, jsonrpc2.ErrMethodNotFound) {
|
||||
// Peer doesn't support ping, stop the keepalive process.
|
||||
return
|
||||
}
|
||||
// Ping failed; log it before closing the session so the
|
||||
// failure is observable to operators. See #218.
|
||||
logger.Error("keepalive ping failed; closing session", "error", err)
|
||||
|
|
|
|||
Loading…
Reference in a new issue