Skip to content

fix(acli): give run_command a real timeout and kill the whole tree - #201

Merged
lzsweb merged 2 commits into
mainfrom
release/agentic-cli-5-1
Sep 17, 2026
Merged

lzsweb merged 2 commits into
mainfrom
release/agentic-cli-5-1

Conversation

@lzsweb

@lzsweb lzsweb commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • run_command 的默认超时从 30s 提到 600s,并让它可见。 30s 会杀掉正常的测试/构建命令,而且模型看不到这个上限 —— 实测它在解 PR Dev/errors #162 冲突时烧了好几轮去试出 sleep 25 行、sleep 150 不行。现在新增 ACLI_COMMAND_TIMEOUT 环境变量(每次调用解析,改完不用重启),工具 description 里写明了 timeout 参数,超时报错也直接点名这个逃生口。
  • 超时时杀整个进程组,不再只杀直接子进程。 原来的 proc.kill() 只够到 shell,够不到它启动的管道,所以任务在被判超时后仍在跑,stdout 管道还开着,asyncio 随后报 Exception ignored in BaseSubprocessTransport.__del__ ... Event loop is closed。现在子进程一律起在自己的进程组里(POSIX start_new_session,Windows CREATE_NEW_PROCESS_GROUP),超时杀组并 reap;Ctrl-C(CancelledError)走同一条路径。
  • /info 不再谎报超时。 它原来只打一行 Timeout: {config.timeout}s,而 config.timeout 其实是 LLM 请求超时(唯一读者是 providers/profile.py),跟 run_command 强制的值毫无关系 —— 用户唯一能查的地方报了一个没人执行的限制。现在拆成 LLM timeout / Cmd timeout 两行,后者显示真正生效的值和环境变量名。

改动源头在 agenticCLI(3878ac4,已合入其 main),经 scripts/sync_acli.sh 同步到本仓库的 vendored 树。

Verification

结果
本仓库全量测试(基线 fde7be5) 1002 passed, 45 skipped, 0 failed
agenticCLI 全量测试 2708 passed, 2 skipped
两边 pre-commit 全绿(mypy / black / flake8 / pylint)
L3 深度安全审查 0 findings

真机复现:(sleep 2; touch MARKER) & sleep 30 + timeout=1。修复前 MARKER 出现(孤儿存活)且有 transport 泄漏告警;修复后两者都消失。

每个修复都做了反证 —— 把它改回去,确认恰好是预期的测试挂掉。两个值得记录的结果:

  • 把 600 改回 30,只有 test_default_covers_a_real_test_run 挂;其余超时断言是对 DEFAULT_TIMEOUT 自指的,并没有真正钉住数值。
  • 去掉 _kill_tree 里的 proc is None 守卫,没有任何测试挂 —— None.wait() 的 AttributeError 被宽 except 吞了。守卫保留是为了让意图不依赖"异常被吞",但我不声称它有覆盖。

Test plan

  • tests/unit/test_acli_command_timeout.py 23 个用例(默认值/环境变量覆盖/参数覆盖、超时消息、三个 spawn 分支的进程组、超时与取消的孤儿检测、killpg 失败回退、Windows taskkill、spawn 失败)
  • 全量 pytest tests/
  • CI 全绿

zhansheng.lzs added 2 commits September 17, 2026 17:34
The 30s default killed ordinary test and build commands, and nothing told
the model the limit existed -- so it burned turns rediscovering it by
experiment (`sleep 25` fits, `sleep 150` does not). Raise the default to
600s, allow ACLI_COMMAND_TIMEOUT to move it, advertise the `timeout`
argument in the tool description, and name it in the timeout message.

On expiry only the direct child died. proc.kill() reaches the shell, not the
pipeline it started, so the work kept running with its stdout pipe open and
asyncio later logged "Exception ignored in BaseSubprocessTransport.__del__
... Event loop is closed". Start every child in its own process group
(start_new_session / CREATE_NEW_PROCESS_GROUP), kill the group, then reap it.
Ctrl-C takes the same path.

/info printed config.timeout as "Timeout", but that field is the LLM request
timeout (providers/profile.py), so the one place a user could look reported a
limit nothing enforced. Split it into two labelled lines.
… vendored tree

Orphan coverage is marker-based rather than pid-based: a SIGKILLed child
nobody reaps is a zombie, and os.kill(pid, 0) still succeeds on a zombie, so
the pid alone says nothing about whether the tree died.

Each fix was falsified by reverting it and confirming exactly the intended
tests fail. Two results worth recording: reverting 600 -> 30 fails only
test_default_covers_a_real_test_run, because the other timeout assertions are
self-referential to DEFAULT_TIMEOUT; and dropping the `proc is None` guard in
_kill_tree fails nothing, since the AttributeError from None.wait() is
swallowed by the broad except. The guard stays so the intent does not depend
on an exception being swallowed, but it is not claimed as covered.
@lzsweb
lzsweb merged commit b0b4469 into main Sep 17, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant