github / github/copilot-sdk

_connect_via_tcp notification handler reads self._sessions without _sessions_lock

未关闭 适合新手
#2,673 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看
主要语言
Java
星标
10.5k
派生
1.5k
平均合并
1 天 14 小时
30 天内合并 PR
129

描述

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.

贡献指南

打开贡献指南

调研方向

从 Python SDK 客户端中约第 4749 行的 _connect_via_tcp 开始,将其通知处理程序与约第 4629 行的 _connect_via_stdio 进行比较。确认 TCP 处理程序使用 _sessions_lock 保护对 _sessions 的查找,并且会话事件(包括权限请求和工具调用)仍会被分发;issue 中没有指定测试文件。

由索引模型根据 Issue 内容生成。

评估

技术栈
python
领域
api
Issue 类型
缺陷
难度
1/5
预计耗时
1 小时以内
活跃度
活跃
描述清晰度
描述清楚
新手友好度
78/100

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。