github / github/copilot-sdk

_connect_via_tcp notification handler reads self._sessions without _sessions_lock

Đang mở Phù hợp với người mới
#2,673 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Java
Star
10.5k
Fork
1.5k
Merge trung bình
1 ngày 11 giờ
Pull request đã merge (30 ngày)
128

Mô tả

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.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Bắt đầu trong client Python SDK tại _connect_via_tcp quanh dòng 4749 và so sánh trình xử lý thông báo của nó với _connect_via_stdio quanh dòng 4629. Xác nhận rằng trình xử lý TCP bảo vệ việc tra cứu _sessions bằng _sessions_lock và các sự kiện phiên, bao gồm yêu cầu cấp quyền và lệnh gọi công cụ, vẫn được dispatch; issue không nêu tên tệp kiểm thử nào.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
python
Lĩnh vực
api
Loại issue
Lỗi
Độ khó
1/5
Thời gian dự kiến
Dưới một giờ
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
78/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.