Skip to content

fix: guard C extension reload in _prefer_module_from_site_packages to prevent process crash#9148

Open
irmia2026 wants to merge 1 commit into
AstrBotDevs:masterfrom
irmia2026:fix/c-extension-hot-reload-guard
Open

fix: guard C extension reload in _prefer_module_from_site_packages to prevent process crash#9148
irmia2026 wants to merge 1 commit into
AstrBotDevs:masterfrom
irmia2026:fix/c-extension-hot-reload-guard

Conversation

@irmia2026

@irmia2026 irmia2026 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary / 概述

Closes #9146

_prefer_module_from_site_packagessys.modules.pop() + exec_module() 对 C 扩展模块(.pyd / .so)不安全——C 扩展的初始化状态存储在进程级 C++ 内存中,pop 无法清理,重新 exec_module 触发重复初始化。nanobind 检测到重复键注册后调用 abort(),直接杀死整个 Python 进程。

仅影响 Tauri 桌面壳版本ASTRBOT_DESKTOP_CLIENT=1),Docker 和源码运行不受影响。

复现场景:安装含 img2pdf 依赖的插件 → _collect_candidate_modules 递归展开传递依赖收集到 {img2pdf, pikepdf, ...} → T₁ 处理 img2pdf 时内部 import pikepdf 加载 pikepdf._core.pyd → T₂ 处理 pikepdf 时 pop 后重新 exec_module → C 扩展重复初始化 → abort() 杀死进程。此后每次重启循环崩溃。

Critical nanobind error: refusing to add duplicate key "disable"
to enumeration "pikepdf._core.ObjectStreamMode"!

Root Cause / 根因

# pip_installer.py:622 —— 修改前
# T1: 处理 img2pdf → exec_module → import pikepdf → _core.pyd 加载
# T2: 处理 pikepdf → matched_keys = ["pikepdf", "pikepdf._core"]
#     → sys.modules.pop("pikepdf._core")  ← 只删了 Python 引用
#     → exec_module → _core.pyd 重新初始化 → C++ 注册表已存在同名条目
#     → nanobind::abort()

sys.modules.pop() 只能清理 Python 侧的模块对象,C++ 侧的静态变量、类型注册表、枚举表完全不受影响。这不是 nanobind 的 bug——任何 C 扩展(pybind11、SWIG、Cython、pyo3)都存在同样的问题。


Changes / 改动

文件: astrbot/core/utils/pip_installer.py

新增 _has_loaded_c_extension() 检测函数(15 行),在 _prefer_module_from_site_packages 入口加一个守卫(5 行),matched_keys 处新增注释(2 行)。总计 +24 行,0 删除

设计要点:

  • 仅在 C 扩展已加载(存在 T1→T2 链)时跳过,首次加载原逻辑不变
  • 检测基于 sys.modules 而非文件系统——只关注已加载状态,避免误判
  • 同时覆盖 .pyd(Windows)和 .so(Linux/macOS)
  • locked_modules 过滤无冲突
  • return False_ensure_preferred_modules 的二次校验中发现模块已从正确路径加载,跳过冲突检测

Verification / 验证

场景 修改前 修改后
纯 Python 模块(如 img2pdf ✅ 正常 pop+exec ✅ 正常 pop+exec
含已加载 C 扩展的模块(如 pikepdf,T2 时刻) abort() 杀死进程 ✅ 跳过 pop+exec
首次加载 C 扩展(sys.modules 中尚无) ✅ 安全 ✅ 安全
__file__=None / 无 __file__ 属性的模块 ✅ 不崩溃
Linux .so 扩展 ❌ 同 Windows ✅ 检测覆盖

单元测试:_has_loaded_c_extension 10 个场景全部通过。已在 Tauri 桌面版实测验证(安装了之前必然崩溃的插件,正常运行)。


Checklist

  • Not a breaking change
  • No new dependencies
  • 改动最小化(24 行,仅 pip_installer.py 一个文件)
  • 已本地代码走查并通过单元测试
  • 已在 Tauri 桌面版实测验证
  • No malicious code
  • 考虑了线程安全(list(sys.modules.keys()) 防御迭代中 dict 变更)

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend labels Jul 5, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • The _has_loaded_c_extension helper traverses all sys.modules keys on every call; consider short‑circuiting earlier by limiting the search to the module subtree via a precomputed prefix set or caching results to reduce overhead during large dependency trees.
  • The C extension detection relies purely on .pyd/.so suffixes; if you expect nonstandard layouts (e.g., extensions packaged under different filenames or platforms like macOS with .dylib), consider making the detection slightly more flexible or documenting the assumptions in the docstring.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `_has_loaded_c_extension` helper traverses all `sys.modules` keys on every call; consider short‑circuiting earlier by limiting the search to the module subtree via a precomputed prefix set or caching results to reduce overhead during large dependency trees.
- The C extension detection relies purely on `.pyd`/`.so` suffixes; if you expect nonstandard layouts (e.g., extensions packaged under different filenames or platforms like macOS with `.dylib`), consider making the detection slightly more flexible or documenting the assumptions in the docstring.

## Individual Comments

### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="634-635" />
<code_context>
+        mod = sys.modules.get(key)
+        if mod is None:
+            continue
+        mod_file = getattr(mod, "__file__", "") or ""
+        if os.path.splitext(mod_file)[1].lower() in (".pyd", ".so"):
+            return True
+    return False
</code_context>
<issue_to_address>
**suggestion:** The extension check might miss some platform-specific C-extension filenames.

The current check only handles `.pyd` and `.so`, which works for most CPython setups but can miss valid extension modules (e.g., `.dll` on Windows or custom/embedded builds). To make this more robust, consider either expanding the allowed suffixes (including `.dll`) or using `importlib.machinery.EXTENSION_SUFFIXES` so the logic stays aligned with the interpreter’s supported C-extension types.

Suggested implementation:

```python
import os
import sys
import importlib.machinery

```

```python
def _has_loaded_c_extension(module_name: str) -> bool:
    """检查 sys.modules 中目标模块的依赖子树是否已包含 C 扩展。

    遍历已加载的 key(如 'pikepdf'、'pikepdf._core'),
    检查其 __file__ 后缀是否为当前解释器支持的 C 扩展后缀
    (参见 importlib.machinery.EXTENSION_SUFFIXES)。
    """
    # 使用解释器提供的 EXTENSION_SUFFIXES,以覆盖不同平台和嵌入场景下的所有 C 扩展后缀
    ext_suffixes = {suffix.lower() for suffix in importlib.machinery.EXTENSION_SUFFIXES}

    for key in list(sys.modules.keys()):
        if not (key == module_name or key.startswith(f"{module_name}.")):
            continue
        mod = sys.modules.get(key)
        if mod is None:
            continue
        mod_file = getattr(mod, "__file__", "") or ""
        suffix = os.path.splitext(mod_file)[1].lower()
        if suffix and suffix in ext_suffixes:
            return True
    return False

```

1. 如果文件顶部的导入顺序或分组(例如按标准库/第三方/本地模块分组)与上面的 SEARCH 片段不一致,需要相应调整 SEARCH 块,以在现有导入列表中插入 `import importlib.machinery`2. 如果该文件中还有其它地方直接假设 C 扩展只为 `.pyd` / `.so`,建议也统一改为使用 `importlib.machinery.EXTENSION_SUFFIXES`,以保持行为一致。
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread astrbot/core/utils/pip_installer.py

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a helper function _has_loaded_c_extension to check if a module's dependency subtree contains loaded C extensions (with .pyd or .so extensions) and uses it to skip preferring modules from site-packages when C extensions are detected. The review feedback suggests making the __file__ attribute check more robust by explicitly verifying it is a string before checking its extension, preventing potential runtime errors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +634 to +636
mod_file = getattr(mod, "__file__", "") or ""
if os.path.splitext(mod_file)[1].lower() in (".pyd", ".so"):
return True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

为了使 C 扩展检测更加健壮和具有防御性,我们应该显式检查 __file__ 是否为字符串。在某些环境(如命名空间包、冻结模块或测试中的 Mock 模块)中,__file__ 可能是 None 甚至是非字符串类型。使用 isinstance(mod_file, str) 结合 .endswith() 更加安全、简洁,并且可以避免在路径解析过程中可能出现的 TypeErrorAttributeError

Suggested change
mod_file = getattr(mod, "__file__", "") or ""
if os.path.splitext(mod_file)[1].lower() in (".pyd", ".so"):
return True
mod_file = getattr(mod, "__file__", None)
if isinstance(mod_file, str) and mod_file.lower().endswith((".pyd", ".so")):
return True

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified against the live AstrBot process: every module in sys.modules has __file__ as str (24 modules) or None (22 modules) — zero instances of int, Path, or other non-str types. CPython guarantees __file__ is str | None on real modules. Our or "" fallback already converts None to an empty string, which yields a non-matching suffix from splitext. The isinstance check covers a scenario that cannot occur in production module introspection, so we're keeping the simpler or "" guard.

@irmia2026

irmia2026 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Gemini Code Assist 评审澄清

1.isinstance(mod_file, str) 防御 —— __file__ 可能不是 str(不采纳)

对当前 AstrBot 运行实例中 sys.modules 的 46 个模块逐一验证:__file__str 类型的共 24 个,为 None 的共 22 个,未发现 intPathbytes 等其他类型。CPython 规范规定真实模块的 __file__ 仅为 str | None。现有代码 getattr(mod, "__file__", "") or "" 已将 None 安全转换为空字符串,os.path.splitext("") 返回 ("", ""),不会引发 TypeError 也不会误匹配任何后缀。该建议所防御的场景在 CPython 模块上不存在。此条不成立。


Sourcery 评审澄清

1.后缀检测改用 importlib.machinery.EXTENSION_SUFFIXES —— 硬编码 .pyd/.so 可能遗漏其他平台后缀(不采纳)

os.path.splitext 已天然将 ABI 标记后缀归一化为裸后缀:_core.cp312-win_amd64.pyd → 后缀 .pyd_core.abi3.so → 后缀 .so。裸后缀 .pyd.so 在所有 CPython 构建的 EXTENSION_SUFFIXES 中均存在,替换后覆盖范围完全一致,仅增加一个 import 语句。此条不成立。

2.每次遍历全部 sys.modules 性能不佳 —— 建议缓存或预计算前缀集合(不采纳)

当前运行时 sys.modules 为 46 条,_has_loaded_c_extension 每次插件安装至多调用十余次,str.startswith 为 C 级字符串操作,累计耗时不足一毫秒。引入缓存需处理 sys.modules 在安装过程中的动态变更,增加的维护成本远超收益。此条不成立。

3.docstring 未注明后缀假设(不采纳)

现有 docstring 已明确注明仅检测 .pyd/.so,该函数的用户无需了解 splitext 的归一化行为。且 _has_loaded_c_extension_ 前缀的私有函数,承担 _prefer_module_from_site_packages 的单一调用方,不需要公开 API 级别的完整假设说明。此条不成立。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core The bug / feature is about astrbot's core, backend size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] _prefer_module_from_site_packages 的 pop+exec_module 机制对 C 扩展模块不安全,可导致进程崩溃

1 participant