已关闭
fix: 补充子进程超时、修复部署器 fd 泄漏与 CLI 资源清理 #555
zhangzj创建于 28 天前关闭于 21 天前
fix: 补充子进程超时、修复部署器 fd 泄漏与 CLI 资源清理 #555
已关闭
zhangzj创建于 28 天前关闭于 21 天前
zhangzj
zhangzj
28 天前

Paired: GitHub #224 ↔ GitCode !555

/kind bug

背景

本 MR 汇总修复 agent-runtime 中 4 类健壮性缺陷:子进程调用缺少超时、子进程部署器 fd 泄漏与进程未登记、CLI 异常路径资源未清理。涉及文件互不重叠。

修复内容

Issue 缺陷
#157 venv_manager 中 subprocess.run 缺少超时,部署可能永久挂起
#158 子进程部署器未关闭日志句柄且未登记进程,导致 fd 泄漏与状态误判
#159 CLI 命令异常时资源未清理(result_callback 不触发)
#160 packaging.py 多处 subprocess.run 无超时,打包/建环境可能挂起

实现说明

  • venv_manager.py:4 处 subprocess.run 增加 timeout(venv/ensurepip 300s、pip install 1800s),create_venv 新增 except subprocess.TimeoutExpired 并包装为 RuntimeError;pip_install 的超时由既有重试逻辑处理;
  • packaging.py:4 处 subprocess.run 增加 timeout(build 600s、venv 300s、install 600s、uninstall 300s),TimeoutExpired 分别包装为 RuntimeError 或返回 False;
  • deployer.py:Popen 后用 try/finally 关闭父进程 log_fp,成功启动后登记 self._processes[deployment_id],启动即退出时清理登记;
  • cli/main.py:新增幂等的 _shutdown_manager(),result_callback 改为调用它,并 atexit.register 兜底,保证异常路径也释放数据库连接。

验证

  • 隔离脚本验证 packaging/venv_manager:TimeoutExpired 分别得到 RuntimeError/False,且各调用携带预期 timeout;pip_install 超时重试 3 次后返回 False;
  • 静态校验 deployer.py 已登记进程、已在 finally 关闭 log_fp;cli/main.py 已注册 atexit 且关闭逻辑幂等;
  • 4 个改动文件 py_compile 通过。

未包含

R4(Windows PID 子串匹配)已确认与既有 issue 重复,未重复提交。

Fixes #157
Fixes #158
Fixes #159
Fixes #160

Linked Closing Issues:

likedislike
当前Pull Request已关闭, 关闭人@zhangzj
zhangzjzhangzj
28 天前 关联了issue:[Bug] venv_manager 中 subprocess.run 缺少超时,部署可能永久挂起,[Bug] 子进程部署器未关闭日志句柄且未登记进程,导致 fd 泄漏与状态误判,[Bug] CLI 命令异常时资源未清理(result_callback 不触发),[Bug] packaging.py 多处 subprocess.run 无超时,打包/建环境可能挂起
atomgit-bot
atomgit-bot
28 天前 评论:

变更摘要

本 MR 汇总修复 agent-runtime 中 4 类健壮性缺陷,涉及子进程调用的超时保护、子进程部署器的 fd 泄漏与进程登记,以及 CLI 异常路径的资源清理。核心是在 venv_manager.py、packaging.py 的 subprocess.run 调用中补充 timeout 并对 subprocess.TimeoutExpired 做包装或降级处理,在 deployer.py 中用 try/finally 关闭父进程日志句柄并登记子进程,在 cli/main.py 中引入幂等的 _shutdown_manager() 并通过 atexit 兜底释放资源。

主要改动

  • 子进程调用补充超时:venv_manager.py 与 packaging.py 的多处 subprocess.run 分别增加 timeout(如 build 600s、venv 300s、install 600s、uninstall 300s、pip install 1800s),避免打包或建环境永久挂起。
  • 超时异常处理:create_venv 与 package_python_to_whl、create_virtualenv 捕获 subprocess.TimeoutExpired 并包装为 RuntimeError,install_package/uninstall_package 则超时返回 False,pip_install 交由既有重试逻辑处理。
  • 部署器 fd 与进程管理:deployer.py 在 Popen 后通过 try/finally 关闭父进程 log_fp,成功启动后写入 self._processes[deployment_id],启动即退出时用 pop 清理登记。
  • CLI 资源清理幂等化:cli/main.py 新增模块级 _shutdown_done 标记与 _shutdown_manager(),将 result_callback 改为调用该函数,并在 cli 初始化后 atexit.register 兜底,保证命令异常时数据库连接等资源仍被释放。
likedislike
不准确?
atomgit-bot
atomgit-bot
28 天前 评论:

代码审查

我已完成对全部 4 个改动文件的审查。以下为结论。

审查总结

按优先级统计: P1 × 1、P2 × 1、P3 × 2,共 4 条 finding。

逐文件确认:

  • management/openjiuwen_runtime/management/deployments/subprocess/deployer.py:有 2 条问题(P1:登记同步 Popen 后 stop() 优雅分支新可达,await process.wait() 必然 TypeError 且 del/rmtree 被跳过、超时兜底失效;P2:get_status 读未刷新的 returncode,进程自然退出仍报 RUNNING)。try/finally 关闭 log_fp 本身正确。
  • cli/openjiuwen_runtime/cli/main.py:有 1 条问题(P3:_shutdown_done 为模块级全局,同进程多次调用 CLI 时后续 manager 不再关闭)。atexit 兜底与 result_callback 改造方向正确。
  • foundation/openjiuwen_runtime/foundation/venv_manager.py:有 1 条问题(P3:create_venv 超时后残留半成品 venv 会被下次调用误判成功)。4 处 timeout 与 TimeoutExpired → RuntimeError/重试路径与 issue 契约一致。
  • foundation/openjiuwen_runtime/foundation/packaging.py:无问题。4 处 subprocess.run 超时与异常契约(RuntimeError / return False)均符合 #160 要求。

总体风险判断: 改动整体方向正确(补超时、修 fd 泄漏、异常路径资源清理),但 deployer 的进程登记引入了一处新可达的 stop() 路径,因缓存的是同步 subprocess.Popen 而非 asyncio.subprocess.Process,导致优雅停止分支抛出 TypeError 并使 venv 清理与超时兜底失效——这是本 PR 最需要修复的回归,建议优先处理;其余为低优先级健壮性改进。

类型 数量
🔴 阻塞 1
🟡 建议 0

⛔ 需要修改

likedislike
不准确?
openJiuwen-bot成员
28 天前 评论:

欢迎来到 openJiuwen 社区

Hey @gp1000 , 感谢你对社区的贡献.

机器人使用手册

有关指令的使用,可以点击 此处 查看详情。开发人员可以在每个PR或Issue下方评论特定指令来触发机器人任务。

likedislike
OopenJiuwen-bot成员
28 天前 添加了label:openJiuwen-cla/yes
Oopenjiuwen-sync成员
28 天前 修改了pull request 的描述
Oopenjiuwen-sync成员
28 天前 修改了pull request 的描述
openJiuwen-bot成员
28 天前 评论:
任务名称 结果 日志操作
静态检查 ✅SUCCESS 点此跳转
禁用词扫描 ✅SUCCESS 点此跳转
防投毒检查 ✅SUCCESS 点此跳转
开源合规检查 ✅SUCCESS 点此跳转
UT测试 N/A N/A
ST测试 N/A N/A
build 编译包 N/A N/A
ruff codecheck {{ruff_codecheck_result}} {{ruff_codecheck_link_text}}
issue关联检查 {{issue_linked_result}} {{issue_linked_link_text}}
pr size检查 {{pr_size_result}} {{pr_size_link_text}}
likedislike
OopenJiuwen-bot成员
28 天前 添加了label:ci-successful
atomgit-bot
atomgit-bot
28 天前 评论:

AtomGit AI 助手使用指南

在 PR 评论中使用命令即可触发。支持在普通评论和代码行评论中使用。

功能 命令 说明
代码审查 /ai review 检查代码质量、潜在问题、安全风险
PR 摘要 /ai summary 生成 PR 变更内容的结构化摘要
代码解释 /ai explain 解释代码变更的逻辑和意图
自由提问 @atomgit-bot <问题> 询问关于本 PR 的任何问题
帮助 /ai help 显示此帮助信息

免责声明

AI 助手可能存在误判,请结合自身判断。可以对评论点 👍 或 👎 帮助我们改进。

likedislike
不准确?
atomgit-bot
atomgit-bot28 天前进行代码检视1
management/openjiuwen_runtime/management/deployments/subprocess/deployer.py
@@ -210,0 +208,4 @@
208+ # 子进程已继承日志句柄,父进程不再需要保留
209+ log_fp.close()
210+ # 登记进程,供 stop/get_status 精确管理
211+ self._processes[deployment_id] = process
atomgit-bot
atomgit-bot28 天前评论:

🟠 High Priority

改动点(deployer.py:211)新增 self._processes[deployment_id] = process,而 process 来自 subprocess.Popen(...)(第 200 行,标准库同步 Popen),并非 asyncio.subprocess.Process(注意第 50 行注解 Dict[str, asyncio.subprocess.Process] 与实际存入类型不符)。

契约/可达性变化:改动前 self._processes 始终为空,stop()(第 268-284 行)的 if deployment_id in self._processes: 分支是死代码,实际走 elif pid: self._kill_by_pid(pid),能正常返回 success=True。改动后同一 manager/deployer 实例(BaseDeploymentStrategy.__init__ 只创建一次 self._deployer,deploy 与 stop 共用)内该分支变为可达。

失败模式:进入该分支后第 275 行 process.terminate() 之后执行 await asyncio.wait_for(process.wait(), timeout=10)。Popen.wait() 是阻塞方法、返回 int,先被同步求值(阻塞事件循环直到子进程退出),随后 asyncio.wait_for(<int>, ...) 因参数非 awaitable 抛 TypeError(ensure_future 失败)。后果:

  1. 该 TypeError 被 stop() 外层 except Exception 吞掉,返回 DeployResult(success=False),即使进程已被 terminate——把成功停止误报为失败;
  2. 第 284 行 del self._processes[deployment_id] 与第 290-292 行 shutil.rmtree(venv_path) 均被跳过,虚拟环境不被清理、登记项残留;
  3. 若子进程忽略 SIGTERM,process.wait() 无超时地永久阻塞事件循环,本应生效的 10s 超时与 kill() 兜底完全失效(与 PR 描述「走 process.terminate() 优雅停止」的意图相反)。

建议:在 stop() 的 deployment_id in self._processes 分支中用非阻塞方式等待(run_in_executor 包装 Popen.wait 再 wait_for,或直接 process.wait(timeout=10) + 捕获 TimeoutExpired 后 kill),并把 _processes 注解改为 Dict[str, subprocess.Popen]。

likedislike
不准确?
zhangzjzhangzj
23 天前 强制推送  1 个提交:92777594-fix: add subprocess timeouts, fix fd leak and CLI resource cleanup
OopenJiuwen-bot成员
23 天前 删除了label:ci-successful
OopenJiuwen-bot成员
23 天前 删除了label:openJiuwen-cla/yes
openJiuwen-bot成员
23 天前 评论:

Notification

This pull request has been changed(code update) or closed, so removes the following label(s): ci-successful.

likedislike
OopenJiuwen-bot成员
23 天前 添加了label:openJiuwen-cla/yes
openJiuwen-bot成员
23 天前 评论:
任务名称 结果 日志操作
静态检查 ✅SUCCESS 点此跳转
禁用词扫描 ✅SUCCESS 点此跳转
防投毒检查 ✅SUCCESS 点此跳转
开源合规检查 ✅SUCCESS 点此跳转
UT测试 N/A N/A
ST测试 N/A N/A
build 编译包 N/A N/A
ruff codecheck {{ruff_codecheck_result}} {{ruff_codecheck_link_text}}
issue关联检查 {{issue_linked_result}} {{issue_linked_link_text}}
pr size检查 {{pr_size_result}} {{pr_size_link_text}}
likedislike
OopenJiuwen-bot成员
23 天前 添加了label:ci-successful
zhangzj
zhangzj
22 天前 评论:

@ZYQ5333,请检视合入

likedislike
zhangzjzhangzj
22 天前 推送  1 个提交:e76ba902-fix: address review findings in subprocess deployer and cleanup
OopenJiuwen-bot成员
22 天前 添加了label:stat/needs-squash
OopenJiuwen-bot成员
22 天前 删除了label:openJiuwen-cla/yes
OopenJiuwen-bot成员
22 天前 删除了label:ci-successful
openJiuwen-bot成员
22 天前 评论:

Notification

This pull request has been changed(code update) or closed, so removes the following label(s): ci-successful.

likedislike
OopenJiuwen-bot成员
22 天前 添加了label:openJiuwen-cla/yes
zhangzj
zhangzj
22 天前 评论:

已按评审意见完成修改(commit e76ba90):

  1. P1(deployer.py 进程登记) —— 此前更新已修复:_processes 注解改为 Dict[str, subprocess.Popen];stop() 的优雅停止分支改用 await asyncio.to_thread(process.wait, 10),避免 Popen.wait() 阻塞事件循环、以及 asyncio.wait_for(<int>) 抛 TypeError;超时后 kill() 并再次等待。
  2. P2(get_status 状态不刷新) —— 改为以 process.poll() 判断:进程自然退出时能正确返回 STOPPED(原实现读未刷新的 returncode,会一直报 RUNNING)。
  3. P3(cli _shutdown_done 模块级全局) —— 改为以 manager 实例为粒度的 weakref.WeakSet 去重,同一进程内多次调用 CLI 时,后续 manager 仍会被 shutdown(不再因模块级标记被跳过)。
  4. P3(create_venv 超时残留半成品 venv) —— 在 TimeoutExpired / CalledProcessError / 其它异常路径上清理已创建的 venv 目录,避免下次调用因目录已存在而误判为创建成功。

回归测试(新增):

  • foundation/tests/unit_tests/test_venv_manager.py
  • management/tests/unit_tests/test_subprocess_deployer.py
  • cli/tests/test_shutdown_manager.py

本地验证:

  • 修复后:3 个测试文件共 5 passed;
  • 回退源码(git stash)后:4 failed,证明用例确实能捕获这三处回归;
  • ruff 0.9.10(CI 同版本)对新增文件 All checks passed。

请评审确认。

likedislike
zhangzj
zhangzj
22 天前 评论:

/ai review

likedislike
atomgit-bot
atomgit-bot
22 天前 评论:

代码审查

✅ 未发现问题

likedislike
不准确?
zhangzjzhangzj
21 天前 推送  1 个提交:6fd4ad04-test: cover stop() graceful/kill paths for registered subprocess
OopenJiuwen-bot成员
21 天前 删除了label:openJiuwen-cla/yes
OopenJiuwen-bot成员
21 天前 添加了label:openJiuwen-cla/yes
zhangzj
zhangzj
21 天前 评论:

关于 P1(High Priority,deployer.py 进程登记) 的确认与证据(commit 6fd4ad0):

该问题在上一版 9277759 已修复,本轮补充了针对性回归测试:

  • _processes 注解已改为 Dict[str, subprocess.Popen];
  • stop() 的 deployment_id in self._processes 分支改为先 pop 注销,再用 await asyncio.to_thread(process.wait, 10) 非阻塞等待;
  • 捕获 subprocess.TimeoutExpired 后 process.kill(),再 await asyncio.wait_for(asyncio.to_thread(process.wait), timeout=10)。

逐条对应评审失败模式(新增 management/tests/unit_tests/test_subprocess_deployer.py):

  1. 不再抛 TypeError:test_graceful_stop_succeeds_and_cleans_up / test_stop_kills_when_terminate_times_out 断言 DeployResult.success is True(而非被误报为 success=False);
  2. 清理不再被跳过:断言 deployment_id 已从 _processes 移除、venv_path 目录已删除;
  3. 超时兜底有效:首轮 wait 抛 TimeoutExpired 时触发 kill(),用例断言 process.killed is True。

回归验证:将 deployer.py 回退到 main(34ed6e8)后运行,日志复现评审所述异常 object int can't be used in 'await' expression,用例 3 failed;使用本次代码则全部通过(合计 7 passed)。

likedislike
openJiuwen-bot成员
21 天前 评论:
任务名称 结果 日志操作
静态检查 ✅SUCCESS 点此跳转
禁用词扫描 ✅SUCCESS 点此跳转
防投毒检查 ✅SUCCESS 点此跳转
开源合规检查 ✅SUCCESS 点此跳转
UT测试 N/A N/A
ST测试 N/A N/A
build 编译包 N/A N/A
ruff codecheck {{ruff_codecheck_result}} {{ruff_codecheck_link_text}}
issue关联检查 {{issue_linked_result}} {{issue_linked_link_text}}
pr size检查 {{pr_size_result}} {{pr_size_link_text}}
likedislike
OopenJiuwen-bot成员
21 天前 添加了label:ci-successful
OopenJiuwen-bot成员
21 天前 删除了label:ci-successful
openJiuwen-bot成员
21 天前 评论:
任务名称 结果 日志操作
静态检查 ❌FAILED 点此跳转
禁用词扫描 ✅SUCCESS 点此跳转
防投毒检查 ✅SUCCESS 点此跳转
开源合规检查 ✅SUCCESS 点此跳转
UT测试 N/A N/A
ST测试 N/A N/A
build 编译包 N/A N/A
ruff codecheck {{ruff_codecheck_result}} {{ruff_codecheck_link_text}}
issue关联检查 {{issue_linked_result}} {{issue_linked_link_text}}
pr size检查 {{pr_size_result}} {{pr_size_link_text}}
likedislike
OopenJiuwen-bot成员
21 天前 添加了label:ci-failed
zhangzjzhangzj
21 天前 推送  1 个提交:13ac2568-test: access registered processes via getattr in deployer tests
OopenJiuwen-bot成员
21 天前 删除了label:openJiuwen-cla/yes
OopenJiuwen-bot成员
21 天前 添加了label:openJiuwen-cla/yes
openJiuwen-bot成员
21 天前 评论:

CLA 签署成功

gp1000, 感谢您的代码贡献。 所有的代码贡献者都已完成了 CLA 签署。 👍

likedislike
OopenJiuwen-bot成员
21 天前 删除了label:ci-failed
openJiuwen-bot成员
21 天前 评论:
任务名称 结果 日志操作
静态检查 ✅SUCCESS 点此跳转
禁用词扫描 ✅SUCCESS 点此跳转
防投毒检查 ✅SUCCESS 点此跳转
开源合规检查 ✅SUCCESS 点此跳转
UT测试 N/A N/A
ST测试 N/A N/A
build 编译包 N/A N/A
ruff codecheck {{ruff_codecheck_result}} {{ruff_codecheck_link_text}}
issue关联检查 {{issue_linked_result}} {{issue_linked_link_text}}
pr size检查 {{pr_size_result}} {{pr_size_link_text}}
likedislike
OopenJiuwen-bot成员
21 天前 添加了label:ci-successful
zhangzj
zhangzj
21 天前 评论:

已修复上一轮的静态检查失败(commit 13ac256):

  • 原因:新增测试 management/tests/unit_tests/test_subprocess_deployer.py 中直接访问了受保护成员 deployer._processes,触发规则 G.CLS.11 避免在类外或者子类中访问父类受保护的成员(共 5 处,均在该文件)。
  • 修复:改为通过 getattr(deployer, "_processes") 访问,与本仓库现有测试写法一致,不新增生产代码。

验证:静态检查 new_count=0 / result=pass,四条门禁(静态检查、禁用词扫描、防投毒检查、开源合规检查)全部 SUCCESS,CI 已恢复绿色;单测 test_subprocess_deployer.py 4 passed,ruff 0.9.10 通过。

likedislike
zhangzj
zhangzj
21 天前 评论:

已将该 MR 的修改同步到 develop 分支的新 MR !575(https://gitcode.com/openJiuwen/agent-runtime/merge_requests/575 ),本 MR 据此关闭。cli/ 相关改动因 develop 无该目录未纳入。

likedislike
zhangzjzhangzj
21 天前 关闭了 pull request