fix: acquire _sessions_lock in TCP notification handler - #2674
Open
ArshVermaGit wants to merge 2 commits into
Open
ArshVermaGit wants to merge 2 commits into
ArshVermaGit wants to merge 2 commits into
Conversation
The notification handler inside _connect_via_tcp was reading self._sessions.get(session_id) without holding _sessions_lock, while the identical handler in _connect_via_stdio correctly wraps the lookup in 'with self._sessions_lock:'. _sessions is mutated from the event loop thread during session create/resume/destroy and read here from the notification path scheduled via call_soon_threadsafe. Every other access (15+ sites) uses the lock — this was the only one that didn't. Without the lock, a notification arriving during session registration can see an inconsistent dict state, silently drop the event, and leave the session hanging forever (permission requests, tool calls, and MCP OAuth all flow through _dispatch_event). On free-threaded Python 3.13+ (PEP 703), the unprotected read becomes a hard data race on dict internals.
Author
|
Hey @edburns , @stephentoub etc etc maintainers, would you mind taking a look at this when you get a chance? It's a small one-liner fix for a missing _sessions_lock in the Python SDK's TCP notification handler. The stdio handler already does this correctly, this one just got missed. Details are in the PR description. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The notification handler inside _connect_via_tcp reads self._sessions.get(session_id) without holding _sessions_lock, while the identical handler in _connect_via_stdio correctly wraps the lookup with with self._sessions_lock:.
Every other access to self._sessions across the file (15+ call sites) uses the lock — this was the only one that didn't. Looks like the two handlers were written separately and drifted over time.
What can go wrong
A session.event notification arriving while a session is mid-registration can hit an inconsistent dict state and silently get dropped. Since _dispatch_event handles permission requests, tool calls, and MCP OAuth, a dropped event means the session hangs forever waiting for a response that never comes.
On free-threaded Python 3.13+ (PEP 703), the unprotected concurrent read becomes a real data race on dict internals.
Change
One-liner — wrap the dict.get() in the TCP notification handler with the same lock the stdio handler already uses.
Resolves #2673