Conversation
maxisbey
left a comment
maxisbey
left a comment
There was a problem hiding this comment.
Most of my comments are about making session_id required for anything task related. All the Tasks related objects we have are only used when you use the default task related method handlers, and so I think it's fair we enforce a session_id.
The only time session_id would be None is when you've set the server to be in stateless mode, but in this situation Tasks doesn't work anyway.
🏠 Remote-Dev: homespace
🏠 Remote-Dev: homespace
src/mcp/client/_memory.py
Outdated
|
|
||
| async with anyio.create_task_group() as tg: | ||
| # Start server in background | ||
| memory_session_id = uuid.uuid4().hex |
There was a problem hiding this comment.
It's a bit weird to be forced to create a session_id before running the server, does stdio does that as well?
There was a problem hiding this comment.
stdio doesn't have session IDs either. After thinking through it I think the problem is we actually shouldn't require session_ids for the default TaskStore, and instead no session ID means no task isolation.
Instead the default task support should only allow None session_ids if the server isn't in stateless mode. I think I was wrong before in assuming that a None session ID meant the server is in stateless mode, but in reality it could just mean we're in a different transport.
maxisbey
added
bug
Previously, session_id was required (str) throughout the task store interface, which forced sessionless transports like stdio and memory to fabricate UUIDs before running tasks. This was awkward because those transports are single-client by architecture and have no session concept. The policy is now enforced at the handler layer instead of the store layer: default task handlers reject requests when the server is in stateless mode (where tasks cannot survive across requests), and pass session_id through as-is otherwise. A None session_id simply means no session-scoped isolation, which is correct for single-client transports. Isolation in InMemoryTaskStore uses strict equality: None only matches None, so tasks created by a sessionless transport are not visible to session-scoped transports and vice versa, preventing cross-transport leaks when a process serves multiple transports from one store. Changes: - TaskStore, InMemoryTaskStore, TaskContext, helpers: session_id is now str | None - ServerSession: expose stateless property - Default task handlers: reject stateless mode, pass session_id as-is - run_task() / ServerTaskContext: accept None session_id, reject stateless mode - Memory transport: revert fabricated UUID session_id - New tests for None-session isolation (strict equality, no cross-transport leaks) :house: Remote-Dev: homespace
3 participants
Ensures that session IDs are used for tasks, avoiding cross-polination.