Skip to content

mcp: add ErrorHandler to ServerOptions#865

Open
ravyg wants to merge 3 commits intomodelcontextprotocol:mainfrom
ravyg:feat/218-error-handler
Open

mcp: add ErrorHandler to ServerOptions#865
ravyg wants to merge 3 commits intomodelcontextprotocol:mainfrom
ravyg:feat/218-error-handler

Conversation

@ravyg
Copy link
Copy Markdown
Contributor

@ravyg ravyg commented Mar 27, 2026

Add an optional ErrorHandler callback to ServerOptions for receiving
out-of-band errors that occur during server operation but are not
associated with a specific request.

Error sources wired to the handler:

  • Keepalive ping failures (previously silently dropped)
  • Notification delivery errors (previously logged as warnings)
  • Internal JSON-RPC protocol errors (previously printed via log.Printf)

When ErrorHandler is nil, existing behavior is preserved — errors are
logged at the appropriate level.

Thanks to @findleyr and @jba for the design discussion on this issue.

Fixes #218

Add an optional ErrorHandler callback to ServerOptions for receiving
out-of-band errors that occur during server operation. These include
keepalive ping failures, notification delivery errors, and internal
JSON-RPC protocol errors.

When ErrorHandler is nil, errors continue to be logged at the
appropriate level using the configured Logger.

Fixes modelcontextprotocol#218
@maciej-kisiel
Copy link
Copy Markdown
Contributor

The issue this is addressing is waiting for community feedback. We're not planning to merge it before there is interest in it.

@jba
Copy link
Copy Markdown
Contributor

jba commented Mar 30, 2026

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.

ravyg added a commit to ravyg/go-sdk that referenced this pull request Apr 9, 2026
Two errors that occur outside the request/response path were previously
unobservable:

  - Keepalive ping failures (mcp/shared.go startKeepalive) silently
    closed the session with no record of why.
  - jsonrpc2 internal errors (mcp/transport.go connect) were printed
    via the global log.Printf, bypassing the configured slog.Logger.

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

Includes a regression test (TestKeepAliveFailure_Logged) that asserts
the keepalive failure produces a log line on the configured logger,
and was verified to fail when the new log call is removed.

Per @jba's suggestion on modelcontextprotocol#865, this is the smaller no-new-API
alternative to the ErrorHandler callback proposal. modelcontextprotocol#865 remains open
pending community input on whether a structured callback is also
wanted.

Refs modelcontextprotocol#218
@ravyg
Copy link
Copy Markdown
Contributor Author

ravyg commented Apr 9, 2026

Thanks @jba and @maciej-kisiel for the feedback. Splitting this into two PRs based on @jba's suggestion above:

ravyg added a commit to ravyg/go-sdk that referenced this pull request Apr 10, 2026
Two errors that occur outside the request/response path were previously
unobservable:

  - Keepalive ping failures (mcp/shared.go startKeepalive) silently
    closed the session with no record of why.
  - jsonrpc2 internal errors (mcp/transport.go connect) were printed
    via the global log.Printf, bypassing the configured slog.Logger.

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

Includes a regression test (TestKeepAliveFailure_Logged) that asserts
the keepalive failure produces a log line on the configured logger,
and was verified to fail when the new log call is removed.

Per @jba's suggestion on modelcontextprotocol#865, this is the smaller no-new-API
alternative to the ErrorHandler callback proposal. modelcontextprotocol#865 remains open
pending community input on whether a structured callback is also
wanted.

Refs modelcontextprotocol#218
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: add ErrorHandler to ServerOptions

3 participants