Define session storage interface #251
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| if session, ok := value.(*sseSession); ok { | ||
| close(session.done) | ||
| } | ||
| s.sessions.Delete(key) | ||
| s.sessions.LoadAndDelete(key) |
There was a problem hiding this comment.
I'm not sure deleting sessions is the right thing to do during shutdown for a horizontally-scaled deployment. Clients may still have an active session connection to another instance of the server.
Even if it's a single instance, it would make sense to persist sessions through restarts IMO.
I'd suggest not deleting here. Even on a single instance with the memory store, the sessions would get garbage collected anyway.
There was a problem hiding this comment.
I think that at least for the SSE transport, there's no way to resume a session so if the SSE connection is closed, the session effectively goes away. Any reconnect would establish a new session.
pottekkat
added
type: enhancement
There was a problem hiding this comment.
This seems like a great overall path forward to me. It probably needs some tests for session store itself.
@tt any interest in rebasing and bringing it current?
Right now,
MCPServerandSSEServerdefine internal fields for storing sessions.The default session storage is implemented with
sync.Mapwhich makes it hard to scale a back-end. With the SSE transport, message requests need to be received by the same process hosting the SSE connection. For this to happen, you need session affinity. Routing layers typically implement this through an HTTP cookie but I don't think available clients broadly honorSet-Cookieheaders.So, the session storage should probably be overridable to allow horizontally scaled deployments.
This pull request is not a complete solution but really a request for comments on how to do this. My first draft is narrowing how
sync.Mapis used to minimize the interface. A second step would then be accepting an option to pass in such an implementation.(The session store will need to also be responsible for creating new sessions and provide a custom implementation of the
ClientSessioninterface.)Alternatives
The
OnRegisterSessionhook potentially allows tracking the session registration and then propagating the existence of this session to other nodes which would register them through(*MCPServer).RegisterSession. The problem is that every server then needs to keep track of every session and there would be a delay where a newly created session is not known to every node. That makes it a nonstarter.Another approach would be to embed a node name into the session ID itself, intercept requests early and do sideways routing to the proper node. There's no technical blocker to doing this but it would lead to some amount of code duplication and unnecessary processing.