code_executors: defer docker and magic imports to avoid hang on Windows#231
code_executors: defer docker and magic imports to avoid hang on Windows#231Fromsko wants to merge 15 commits into
Conversation
…ws without Docker/libmagic On Windows (and other platforms without Docker Desktop or libmagic installed), top-level `import docker` and `import magic` cause the interpreter to hang or segfault while probing for native daemons/libraries. This makes the entire trpc_agent_sdk package unusable — even a simple `from trpc_agent_sdk.agents import LlmAgent` never returns. Both imports are now deferred to call-time: - _container_cli.py: `import docker` moved into `_import_docker()`, called lazily from `_init_docker_client()` on first real use. - _files.py: `import magic` moved inside `detect_content_type()`, wrapped in try/except to gracefully degrade when python-magic is unavailable. Verified locally: `from trpc_agent_sdk.agents import LlmAgent` now completes instantly on Windows 11 without Docker Desktop, and the minimal example (weather agent with tool calling) runs end-to-end. Fixes trpc-group#230
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
AI Code Review我现在有足够的信息来进行审查了。让我来分析一下关键问题。
让我评估一下 发现的问题
|
- detect_content_type now skips python-magic entirely on win32 to avoid native libmagic access violations (not catchable by try/except) - HAS_MAGIC retained as module-level False constant for backward compat with tests that mock it - test_detect_content_type_with_magic marked skipif(win32) - No new test failures: 19 pass, 1 skip, 11 pre-existing Windows path/glob failures unchanged from main branch
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
AI Code Review我已经审查了 发现的问题🚨 Critical
|
| @@ -328,6 +331,7 @@ def test_detect_content_type_text_utf8(self): | |||
|
|
|||
There was a problem hiding this comment.
test_detect_content_type_with_magic 因局部 import 导致 patch 失效
@patch('..._files.magic', create=True) 替换的是模块属性，但新实现把 import magic 移到函数内部，只查 sys.modules，mock 永不会被调用，断言必然失败。应改为 patch sys.modules['magic'] 或在函数内通过模块属性引用 magic。
| # On Windows, python-magic requires a native libmagic DLL that, when | ||
| # missing, causes an access violation at import time (not catchable by | ||
| # try/except). We skip it entirely on win32 and fall through to the | ||
| # simple content-based detection below. |
There was a problem hiding this comment.
HAS_MAGIC 被硬编码为 False，magic 检测路径成为死代码
if HAS_MAGIC and sys.platform != 'win32' 中 HAS_MAGIC 恒为 False，函数内 import magic 永不执行，即便 Linux/Mac 正确安装 libmagic 也不生效，所有无法由文件名推断的文件回落到字节检测。建议去掉门控，直接在非 win32 平台尝试 import 并缓存结果。
Address AI code review feedback on PR trpc-group#231: 1. Replace hardcoded HAS_MAGIC=False with cached _magic_module pattern: - On non-win32, detect_content_type lazily imports magic on first call and caches the module in _magic_module (checked once via _magic_checked) - This preserves the magic detection path on Linux/Mac where libmagic is properly installed, fixing the dead-code regression 2. Separate import failure from runtime failure: - ImportError → log debug, fall through to byte-signature detection - magic.from_buffer exception → log debug with exc_info, fall through - Previously both were silently swallowed 3. Fix test_detect_content_type_with_magic to inject mock directly into _magic_module instead of patching non-existent module attributes
Address AI code review feedback on PR trpc-group#231: 1. Replace hardcoded HAS_MAGIC=False with cached _magic_module pattern: - On non-win32, detect_content_type lazily imports magic on first call and caches the module in _magic_module (checked once via _magic_checked) - This preserves the magic detection path on Linux/Mac where libmagic is properly installed, fixing the dead-code regression 2. Separate import failure from runtime failure: - ImportError → log debug, fall through to byte-signature detection - magic.from_buffer exception → log debug with exc_info, fall through - Previously both were silently swallowed 3. Fix test_detect_content_type_with_magic to inject mock directly into _magic_module instead of patching non-existent module attributes
AI Code Review我已经全面了解情况，现在让我来撰写审查意见。 发现的问题🚨 Critical
|
1. _container_cli.py: add TYPE_CHECKING guard for docker/Container type annotations so get_type_hints() won't NameError without Docker installed 2. _container_cli.py: add _docker_imported flag to short-circuit repeated _import_docker() calls 3. _files.py: only set _magic_checked=True after successful import or confirmed ImportError (clearer intent) 4. Run yapf -i on all changed files to satisfy CI format check
- _container_cli.py: add # noqa: F821 for frames_iter/demux_adaptor/ consume_socket_output (injected at runtime by _import_docker) - _files.py: add missing `from trpc_agent_sdk.log import logger` - Both yapf and flake8 now pass clean on changed files
d7b0705 to
7f2e29a
Compare
AI Code Review确认：之前的提交 (33aedbf) 特意保留了 我现在已经完成了审查，让我来撰写审查结论。 发现的问题🚨 Critical（无）
|
CI test_container_cli.py uses @patch("..._container_cli.docker") which requires the docker name to exist at module level. Call _import_docker() at test import time to ensure the module attribute is present for patching. Fixes 12 CI failures (9405 passed, 0 regressions).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
==========================================
Coverage ? 87.84600%
==========================================
Files ? 482
Lines ? 45195
Branches ? 0
==========================================
Hits ? 39702
Misses ? 5493
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1. Restore HAS_MAGIC as backward-compatible public alias (updated lazily when magic is successfully imported on non-win32) 2. Add threading.Lock to _import_docker() for concurrent safety 3. Remove dead code: unused `import sys as _sys` in test method 4. All yapf + flake8 clean, 47 passed, 0 regressions
AI Code Review静态分析已足够。让我完成代码审查。 发现的问题🚨 Critical
|
| close_write() | ||
|
|
||
| frames = frames_iter(sock, tty=False) | ||
| demux_frames = (demux_adaptor(*frame) for frame in frames) |
There was a problem hiding this comment.
惰性注入符号未做 NameError 兜底
frames_iter/demux_adaptor/consume_socket_output 仅在 _import_docker() 成功后通过副作用注入全局命名空间，_exec_run_with_stdin 直接引用却无显式校验。若符号未注入（如测试用 __new__ 绕过初始化或 import 异常被吞），会抛 NameError 并被外层 except Exception 吞成 "Execution error"。建议在 _exec_run_with_stdin 内显式校验这些符号存在，或在 _import_docker() 之外保证其可用。
….update Address AI review Critical: eliminate # noqa: F821 and NameError risk by declaring docker/Container/consume_socket_output/demux_adaptor/ frames_iter as module-level None placeholders, populated by _import_docker() via direct assignment instead of globals().update. - Remove TYPE_CHECKING guard (placeholders serve the same purpose) - Remove all # noqa: F821 comments - _exec_run_with_stdin now references real module variables, not dynamically injected names - Tests @patch works naturally without _import_docker() side effects - yapf + flake8 clean, 47 passed, 0 regressions
AI Code Review我已经掌握了足够的信息，现在我来完成这次审查。
让我检查一个真正的正确性问题： 但存在一个更微妙的问题：在非 Windows 系统上，如果首次调用时 现在让我来编写审查报告。 发现的问题🚨 Critical
|
| # missing, causes an access violation at import time (not catchable by | ||
| # try/except). We skip it entirely on win32 and fall through to the | ||
| # simple content-based detection below. | ||
| global _magic_module, _magic_checked, HAS_MAGIC |
There was a problem hiding this comment.
magic 懒加载失败缓存仅捕获 ImportError，无法覆盖其它导入失败
_magic_checked = True 在 except ImportError 分支将失败永久缓存。若 import magic 抛出 OSError 等非 ImportError 异常（如 libmagic 缺失），异常会向上冒泡导致 detect_content_type 直接崩溃，而非回退到字节签名检测。建议改为 except Exception，与下方 from_buffer 的兜底策略一致。
…ests Address latest AI review: 1. Change except ImportError to except Exception for magic import failure, catching OSError and other non-Import failures safely 2. Include HAS_MAGIC in test mock save/restore to prevent state leakage 3. Add exc_info=True to the debug log for better diagnostics
AI Code Review我已掌握足够上下文,开始撰写 review 结论。 发现的问题🚨 Critical
|
Address AI review Critical: if _import_docker() fails silently and docker remains None, the except docker.errors.DockerException line would throw AttributeError: NoneType has no attribute errors, masking the real error. Now explicitly checks docker is not None after import and raises a clear RuntimeError with install instructions.
AI Code Review发现的问题
|
…me semantics 1. _import_docker(): catch ImportError so docker stays None when SDK is not installed, making the `if docker is None` guard in _init_docker_client() actually reachable 2. _files.py: probe magic at module import time on non-win32 to set HAS_MAGIC immediately, preserving the original semantics where external code can check HAS_MAGIC right after import
AI Code Review现在让我更仔细地检查 让我验证一个关于 让我检查一下 现在开始撰写审查结论。 发现的问题🚨 Critical
|
| ContainerConfig, | ||
| ) | ||
| from trpc_agent_sdk.utils import CommandExecResult | ||
| from trpc_agent_sdk.code_executors.container._container_cli import \ |
There was a problem hiding this comment.
模块级副作用调用 _import_docker 污染全局状态
import 期执行 _import_docker() 会把真实 docker 符号写入 _container_cli 全局并置 _docker_imported=True，使测试行为依赖运行环境而非自洽的 mock，且 _docker_imported 置位后不可恢复。建议改在 fixture 中用 monkeypatch 设置相关符号或显式管理 _docker_imported 状态，避免 import 期执行有状态副作用。
1. test_container_cli.py: replace module-level _import_docker() call with autouse module-scoped fixture, removing import-time global side effects and fixing import ordering (all imports at top) 2. _files.py: add threading.Lock for magic lazy import to prevent concurrent race condition on _magic_module/_magic_checked state 3. Consolidate _magic_checked=True inside lock for atomic state update
AI Code Review我已经有足够的信息进行评估了。让我来整理一下审查意见。 发现的问题🚨 Critical
|
| # Declaring them here eliminates F821 and ensures _exec_run_with_stdin | ||
| # gets a clear RuntimeError instead of NameError if called before init. | ||
| docker = None | ||
| Container = None |
There was a problem hiding this comment.
模块级占位符遮蔽真实 Docker 类型，破坏对外 API 语义
模块顶层 Container = None 等占位符遮蔽了 docker.models.containers.Container 等真实类型，导入本模块得到 None 而非真实类型，外部依赖类型注解或 isinstance 校验的代码将静默失效。建议仅在 TYPE_CHECKING 下保留字符串类型别名，运行期不在顶层暴露同名占位变量。
…owing Fix AI review Critical: module-level docker=None/Container=None shadowed the TYPE_CHECKING type aliases, breaking external `from ..._container_cli import Container`. - Use TYPE_CHECKING guard for docker/Container type annotations (static only) - Store runtime symbols in private _docker_mod/_docker_container_cls/ _docker_consume_socket_output/_docker_demux_adaptor/_docker_frames_iter - Update all runtime references to use private names - Update test @patch targets to _docker_mod - No public module-level names are shadowed
AI Code Review我已经了解了全貌。让我来撰写审查意见。 发现的问题🚨 Critical
|
| # External code may reference it as ``from ..._files import HAS_MAGIC``. | ||
| # On non-win32, probe once at import time to preserve the original semantics | ||
| # (HAS_MAGIC reflects availability immediately, not deferred to first call). | ||
| if sys.platform != 'win32': |
There was a problem hiding this comment.
非 win32 模块级强制 import magic，未达成延迟导入目标
注释声称仅在 detect_content_type() 首次调用时才 import magic，但 if sys.platform != 'win32' 分支在模块 import 时即执行 import magic。若 magic 导入触发原生库加载副作用，仍会在 import trpc_agent_sdk 时发生，违背 PR 初衷。建议非 win32 也走延迟导入。
| _magic_checked = True | ||
| except Exception: | ||
| _magic_checked = True | ||
| HAS_MAGIC = _magic_module is not None |
There was a problem hiding this comment.
HAS_MAGIC 在 win32 上语义被破坏性变更
改动后 win32 上 HAS_MAGIC 恒为 False（即使实际安装可用），而注释称其为向后兼容的公开别名。外部代码若依赖其值将被静默改变。建议在变更说明中标注，或保留 win32 下也尝试探测。
1. _files.py: remove module-level import magic on non-win32 entirely (was still triggering at import time). Now truly deferred to first detect_content_type() call. HAS_MAGIC defaults False, updated lazily. 2. Remove unused _has_magic() function (dead code per review) 3. _container_cli.py: remove unused _docker_container_cls 4. _container_cli.py: widen except ImportError to except Exception ensuring _docker_imported=True always set, preventing retry storms
AI Code Review现在我已经有了足够的上下文。让我来总结一下审查意见。 发现的问题
|
Address latest AI review warnings (no Critical this round): 1. Add logger.debug(exc_info=True) for _import_docker except path to aid debugging when docker import fails for non-obvious reasons 2. Move import threading to standard library group at top of file 3. Remove duplicate import threading below the third-party imports
AI Code Review现在我来总结一下我的分析。主要观察结果如下：
让我更仔细地看一下一个具体问题 —— 现在，真正值得关注的问题：在 让我确认一下没有其他值得标记的真正问题。差异（diff）主要是关于延迟导入重构 + 测试适配。这很干净。让我写下审查结果。 发现的问题🚨 Critical（无）
|
2 participants
Fixes #230. Defer top-level import docker and import magic to call-time to prevent hang/segfault on Windows without Docker/libmagic. See issue for full details.