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

* sync(acli): give run_command a real timeout and kill the whole tree

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.

* test(acli): pin the command timeout and the process-group kill in the 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.

---------

Co-authored-by: zhansheng.lzs <zhansheng.lzs@alibaba-inc.com>

Author: agentscope-ai-java

dashscope/acli/cli/dispatch.py

@@ -200,7 +200,15 @@ def _handle_slash_command(
         )
         console.print(f"  Loop mode:    [cyan]{config.loop_mode}[/cyan]")
         console.print(f"  Max turns:    [cyan]{config.max_turns}[/cyan]")
-        console.print(f"  Timeout:      [cyan]{config.timeout}s[/cyan]")
+        # config.timeout is the LLM request timeout; run_command enforces its
+        # own, so showing one "Timeout" line reported a limit nothing used.
+        from dashscope.acli.tools.shell import default_timeout
+
+        console.print(f"  LLM timeout:  [cyan]{config.timeout}s[/cyan]")
+        console.print(
+            f"  Cmd timeout:  [cyan]{default_timeout()}s "
+            f"(ACLI_COMMAND_TIMEOUT)[/cyan]",
+        )
         console.print(
             f"  TUI:          {'[green]on' if config.tui else '[dim]off'}[/]",
         )

dashscope/acli/config.py

@@ -253,6 +253,8 @@ class Config:
     #   "dangerous" - only DANGEROUS prompts; CONFIRM auto-passes
     confirm_mode: str = "dangerous"
     max_turns: int = 50
+    # LLM request timeout (providers/profile.py). run_command has its own,
+    # separate limit: see acli.tools.shell.default_timeout().
     timeout: int = 30
     mcp_servers: list[MCPServerConfig] = field(default_factory=list)
     memory_enabled: bool = True

dashscope/acli/tools/shell.py

@@ -8,14 +8,38 @@
 import re
 import shlex
 import shutil
+import signal
+import subprocess
 import sys
 
 from dashscope.acli.tools.registry import PermissionLevel, tool
 
 MAX_OUTPUT_LENGTH = 10000
 MAX_OUTPUT_BYTES = 5 * 1024 * 1024  # 5 MB hard limit before truncation
-DEFAULT_TIMEOUT = 30
+# Sized for real work: a project's test or build command. The previous 30s
+# silently killed both, and since the model could not see the limit it spent
+# turns guessing at it (e.g. discovering `sleep 25` works and `sleep 150`
+# does not). Callers override per call with the ``timeout`` argument.
+DEFAULT_TIMEOUT = 600
+TIMEOUT_ENV = "ACLI_COMMAND_TIMEOUT"
 IS_WINDOWS = os.name == "nt"
+# Windows has no setsid/killpg; a new process group is the closest thing, and
+# what lets ``taskkill /T`` reach grandchildren. The flag does not exist on
+# POSIX, hence getattr.
+_NEW_GROUP_FLAGS = getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0)
+
+
+def default_timeout() -> int:
+    """Seconds a run_command call gets before it is killed.
+
+    Resolved per call rather than at import, so exporting
+    ``ACLI_COMMAND_TIMEOUT`` takes effect without a restart.
+    """
+    try:
+        value = int(os.environ.get(TIMEOUT_ENV, "") or DEFAULT_TIMEOUT)
+    except ValueError:
+        return DEFAULT_TIMEOUT
+    return value if value > 0 else DEFAULT_TIMEOUT
 
 
 def _resolve_output_encoding() -> str:
@@ -519,11 +543,101 @@ def _is_safe_single(chunk: str) -> bool:
     return False
 
 
+def _signal_tree(proc: asyncio.subprocess.Process | None) -> None:
+    """Kill the process group. Synchronous: no awaits, so it is safe to call
+    from a ``CancelledError`` handler where awaiting is unreliable.
+    """
+    if proc is None or proc.returncode is not None:
+        return
+    if IS_WINDOWS:
+        try:
+            subprocess.run(
+                ["taskkill", "/F", "/T", "/PID", str(proc.pid)],
+                stdout=subprocess.DEVNULL,
+                stderr=subprocess.DEVNULL,
+                check=False,
+                timeout=10,
+            )
+        except Exception:  # pylint: disable=broad-except
+            pass
+        return
+    try:
+        # proc.pid is the group id: the child was started with
+        # start_new_session, which is what makes this reach grandchildren.
+        os.killpg(proc.pid, signal.SIGKILL)
+    except OSError:
+        try:
+            proc.kill()
+        except Exception:  # pylint: disable=broad-except
+            pass
+
+
+async def _kill_tree(proc: asyncio.subprocess.Process | None) -> None:
+    """Kill the tree, then reap it.
+
+    Without the reap the transport stays open until GC, which asyncio reports
+    as "Exception ignored in BaseSubprocessTransport.__del__ ... Event loop is
+    closed" when the process exits first.
+    """
+    if proc is None:
+        # The spawn itself failed, so there is nothing to clean up.
+        return
+    _signal_tree(proc)
+    try:
+        await proc.wait()
+    except Exception:  # pylint: disable=broad-except
+        pass
+
+
+async def _spawn(command: str) -> asyncio.subprocess.Process:
+    """Start the command in its own process group.
+
+    The group is what lets ``_signal_tree`` reach grandchildren. Without it a
+    timeout killed only the shell, so ``cmd | slow`` kept running after the
+    caller was told the command had timed out.
+    """
+    if IS_WINDOWS:
+        # argv form, never a shell string: interpolating into
+        # 'powershell -Command "..."' routes it through cmd.exe first and
+        # any embedded double quote corrupts the command.
+        return await asyncio.create_subprocess_exec(
+            *WIN_SHELL,
+            command,
+            stdout=asyncio.subprocess.PIPE,
+            stderr=asyncio.subprocess.PIPE,
+            cwd=os.getcwd(),
+            creationflags=_NEW_GROUP_FLAGS,
+        )
+    # Optional OS sandbox (opt-in; degrades to normal execution
+    # when disabled or when no backend is available).
+    from dashscope.acli import sandbox
+
+    sandbox_argv = None
+    if sandbox.is_enabled():
+        sandbox_argv = sandbox.build_argv(command, os.getcwd())
+    if sandbox_argv is not None:
+        return await asyncio.create_subprocess_exec(
+            *sandbox_argv,
+            stdout=asyncio.subprocess.PIPE,
+            stderr=asyncio.subprocess.PIPE,
+            cwd=os.getcwd(),
+            start_new_session=True,
+        )
+    return await asyncio.create_subprocess_shell(
+        command,
+        stdout=asyncio.subprocess.PIPE,
+        stderr=asyncio.subprocess.PIPE,
+        cwd=os.getcwd(),
+        start_new_session=True,
+    )
+
+
 @tool(
     name="run_command",
     description=(
         "Execute a shell command and return its output. "
-        "Dangerous commands are blocked."
+        "Dangerous commands are blocked. Pass timeout (seconds) to override "
+        f"the {DEFAULT_TIMEOUT}s default for long builds or test runs."
     ),
     permission=PermissionLevel.CONFIRM,
 )
@@ -555,57 +669,33 @@ async def run_command(command: str, timeout: int | None = None) -> str:
     # fall back to the default instead of letting asyncio.wait_for raise
     # TypeError on `<= 0`.
     try:
-        timeout = (
-            int(timeout) if timeout not in (None, "", 0) else DEFAULT_TIMEOUT
-        )
+        timeout = int(timeout) if timeout not in (None, "", 0) else 0
     except (TypeError, ValueError):
-        timeout = DEFAULT_TIMEOUT
+        timeout = 0
+    if timeout <= 0:
+        timeout = default_timeout()
 
+    proc = None
     try:
-        if IS_WINDOWS:
-            # argv form, never a shell string: interpolating into
-            # 'powershell -Command "..."' routes it through cmd.exe first and
-            # any embedded double quote corrupts the command.
-            proc = await asyncio.create_subprocess_exec(
-                *WIN_SHELL,
-                command,
-                stdout=asyncio.subprocess.PIPE,
-                stderr=asyncio.subprocess.PIPE,
-                cwd=os.getcwd(),
-            )
-        else:
-            # Optional OS sandbox (opt-in; degrades to normal execution
-            # when disabled or when no backend is available).
-            from dashscope.acli import sandbox
-
-            sandbox_argv = None
-            if sandbox.is_enabled():
-                sandbox_argv = sandbox.build_argv(command, os.getcwd())
-            if sandbox_argv is not None:
-                proc = await asyncio.create_subprocess_exec(
-                    *sandbox_argv,
-                    stdout=asyncio.subprocess.PIPE,
-                    stderr=asyncio.subprocess.PIPE,
-                    cwd=os.getcwd(),
-                )
-            else:
-                proc = await asyncio.create_subprocess_shell(
-                    command,
-                    stdout=asyncio.subprocess.PIPE,
-                    stderr=asyncio.subprocess.PIPE,
-                    cwd=os.getcwd(),
-                )
+        proc = await _spawn(command)
         stdout, stderr = await asyncio.wait_for(
             proc.communicate(),
             timeout=timeout,
         )
     except asyncio.TimeoutError:
-        try:
-            proc.kill()
-        except Exception:
-            pass
-        return f"Error: command timed out ({timeout}s)"
+        await _kill_tree(proc)
+        return (
+            f"Error: command timed out after {timeout}s and was killed. "
+            "Pass a larger timeout argument (seconds) if it needs longer; "
+            f"the default is {default_timeout()}s."
+        )
+    except asyncio.CancelledError:
+        # Ctrl-C: kill synchronously, because awaiting inside a cancellation
+        # handler can be interrupted before the kill lands.
+        _signal_tree(proc)
+        raise
     except Exception as e:
+        await _kill_tree(proc)
         return f"Error: execution failed - {e}"
 
     # Enforce hard size limits to prevent OOM on huge outputs