mcp: log out-of-band errors instead of dropping them (#887)

## Summary

Two error sites in the MCP runtime were previously unobservable:

- **`mcp/shared.go` `startKeepalive`** — when a keepalive ping failed,
the session was closed silently with no indication of why.
- **`mcp/transport.go` `connect`** — `jsonrpc2.OnInternalError` printed
via the package-level `log.Printf`, bypassing the `*slog.Logger`
configured on `ServerOptions`/`ClientOptions`.

Both sites now report through the existing `*slog.Logger` that `Server`
and `Client` already guarantee non-nil via `ensureLogger`. The logger is
threaded into the unexported `startKeepalive` and `connect` helpers as a
parameter — **no new public API surface**.

## Context

Per @jba's [suggestion on
#865](https://github.com/modelcontextprotocol/go-sdk/pull/865#issuecomment-4156166474):

> Maybe we can just use the logger in all the places we currently drop
the error. Then it's still reported, but we don't have to add any API.

This PR is that smaller alternative. It addresses the concrete
observability gap in #218 without expanding `ServerOptions`. PR #865
(the `ErrorHandler` callback proposal) remains open pending community
input on whether a structured callback API is also wanted on top of
logging.

`notifySessions` already logs at `Warn` level on delivery errors and is
intentionally left untouched.

## Test plan

- [x] New regression test `TestKeepAliveFailure_Logged` asserts the
keepalive failure produces a log line on the configured logger.
- [x] Verified the test fails when the new `logger.Error(...)` call is
removed (sanity check that it pins the behavior).
- [x] `go test ./mcp/... -count=1` passes.
- [x] `go vet ./...` clean.
- [x] `go build ./...` clean.

Refs #218

---------

Co-authored-by: Maciej Kisiel <mkisiel@google.com>
This commit is contained in:
Ravish Gupta 2026-04-14 07:53:11 -07:00 • committed by GitHub
commit 4c00594be6
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -582,7 +582,10 @@ type keepaliveSession interface {
// startKeepalive starts the keepalive mechanism for a session.
// It assigns the cancel function to the provided cancelPtr and starts a goroutine
// that sends ping messages at the specified interval.
func startKeepalive(session keepaliveSession, interval time.Duration, cancelPtr *context.CancelFunc) {
//
// logger must be non-nil; ping failures (which terminate the keepalive loop and
// close the session) are reported via logger so they are not silently dropped.
func startKeepalive(session keepaliveSession, interval time.Duration, cancelPtr *context.CancelFunc, logger *slog.Logger) {
ctx, cancel := context.WithCancel(context.Background())
// Assign cancel function before starting goroutine to avoid race condition.
// We cannot return it because the caller may need to cancel during the
@ -602,7 +605,9 @@ func startKeepalive(session keepaliveSession, interval time.Duration, cancelPtr
err := session.Ping(pingCtx, nil)
pingCancel()
if err != nil {
// Ping failed, close the session
// 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)
_ = session.Close()
return
}