Skip to content

Conversation

@CodeWithKyrian
Copy link
Contributor

This PR fixes a critical issue where JSON-RPC error responses were missing message IDs, causing clients to timeout waiting for responses.

Problem:
When errors occurred (method not found, invalid params, etc.), the error responses were sent without the original message ID. This caused clients to wait indefinitely for a response that would never come, leading to timeouts.

Solution:

  • Modified Handler.php to pass message IDs to all error responses using the existing Error factory methods
  • Updated encodeResponse() to handle JSON encoding errors gracefully with fallback error responses
  • Removed unnecessary try-catch in Server.php since encodeResponse() no longer throws exceptions

@CodeWithKyrian CodeWithKyrian force-pushed the fix/jsonrpc-error-message-ids branch from 659b0f6 to a29c126 Compare September 21, 2025 14:30
@chr-hertel
Copy link
Member

I like that you cleaned up the exception handling - missed that one 👍

I wonder tho if we could ease the Handler with just passing the $message to the error and the error checks for $message instance of Request (it's only than that we should attach an ID, right?) - wdyt?

@CodeWithKyrian
Copy link
Contributor Author

Oh sure. That's even better. We shouldn't send a response for notifications, even if there's an error

@chr-hertel chr-hertel added the Server Issues & PRs related to the Server component label Sep 21, 2025
@chr-hertel chr-hertel changed the title Fix: ensure JSON-RPC error responses include message IDs [Server] Fix: ensure JSON-RPC error responses include message IDs Sep 21, 2025
@CodeWithKyrian
Copy link
Contributor Author

Oh wait. Seems I misunderstood what you meant @chr-hertel . You're suggesting we modify those factory methods in the Error class itself to accept Message instances instead of the id, yeah?

@chr-hertel
Copy link
Member

What got me thinking was the $messageId handling and the method_exists - maybe there's a different way to make that cleaner, but handing over the message to the error was just one idea to try

@CodeWithKyrian
Copy link
Contributor Author

Yeah, I get it. The method_exists check is clunky. I think instead of passing the whole message to Error (which already expects just a message string), we can check if $message instanceof Request in the Handler and extract the ID there, otherwise use 0.

Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CodeWithKyrian!

@chr-hertel chr-hertel merged commit 32b8b2e into modelcontextprotocol:main Sep 24, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Server Issues & PRs related to the Server component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants