_connect_via_tcp notification handler reads self._sessions without _sessions_lock

Aperta Adatta ai principianti
#2,673 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
1/5
Tempo stimato
Meno di un'ora
Idoneità per principianti
78/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
python
Ambito
api

Direzione di ricerca

Inizia nel client dell’SDK Python, in _connect_via_tcp intorno alla riga 4749, e confronta il relativo gestore delle notifiche con _connect_via_stdio intorno alla riga 4629. Verifica che il gestore TCP protegga la ricerca in _sessions con _sessions_lock e che gli eventi di sessione, incluse le richieste di autorizzazione e le chiamate agli strumenti, continuino a essere distribuiti; nell’issue non è indicato alcun file di test.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

While reading through the Python SDK client code, I noticed the notification handler inside _connect_via_tcp does a bare self._sessions.get(session_id) without holding self._sessions_lock.

The stdio handler gets this right — it wraps the lookup in with self._sessions_lock: — but the TCP version doesn't. Looks like these two handlers were written separately (or one was copied from the other before the lock was added) and they drifted.

stdio path (_connect_via_stdio, around line 4629):

python

with self._sessions_lock:
session = self._sessions.get(session_id)
TCP path (_connect_via_tcp, around line 4749):

python

session = self._sessions.get(session_id) # no lock
_sessions is mutated from the asyncio event loop thread (create/resume/destroy) and read here from the notification handler that gets scheduled via call_soon_threadsafe from the reader thread. Every other access to _sessions across the file (there are 15+ of them) correctly uses the lock — this is the only one that doesn't.

For what it's worth, the Go SDK's equivalent (handleSessionEvent in client.go) always grabs sessionsMux before touching the sessions map, regardless of transport.

Why it matters
Right now on CPython with the GIL, dict.get() is accidentally atomic so this mostly works by luck. But it's still a logic race — a notification can arrive while a session is mid-registration and get silently 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'll never come.
With free-threaded Python (3.13+ nogil builds), this becomes a real data race on the dict internals — potential segfault territory.
Fix
Pretty straightforward one-liner:

diff

     def handle_notification(method: str, params: dict):
         if method == "session.event":
             session_id = params["sessionId"]
             event_dict = params["event"]
             event = session_event_from_dict(event_dict)
  •            session = self._sessions.get(session_id)
    
  •            with self._sessions_lock:
    
  •                session = self._sessions.get(session_id)
               if session:
                   session._dispatch_event(event)
    

Happy to put up a PR if this looks right.

Lingua principale
Java
Stelle
10.5k
Fork
1.5k
Merge medio
1g 12h
PR unite (30g)
133

Guida per i contributori

Apri la guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di github/copilot-sdk

Tutte le issue di github/copilot-sdk

Issue simili

Altre issue su Java

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.