Repository navigation
fix(acli): give run_command a real timeout and kill the whole tree - #201
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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。现在子进程一律起在自己的进程组里(POSIXstart_new_session,WindowsCREATE_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)真机复现:
(sleep 2; touch MARKER) & sleep 30+timeout=1。修复前 MARKER 出现(孤儿存活)且有 transport 泄漏告警;修复后两者都消失。每个修复都做了反证 —— 把它改回去,确认恰好是预期的测试挂掉。两个值得记录的结果:
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.py23 个用例(默认值/环境变量覆盖/参数覆盖、超时消息、三个 spawn 分支的进程组、超时与取消的孤儿检测、killpg 失败回退、Windows taskkill、spawn 失败)pytest tests/