diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e500e09c2..cd7b0317d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Only write entries that are worth mentioning to users. ## Unreleased - Kosong: Stop sending an empty `anthropic-beta` header when no beta features are declared — adaptive thinking removes the interleaved-thinking beta, which previously left an empty header value that some backends reject +- Shell: Fix the shell tool blocking until the full command timeout when a detached child process inherits stdout/stderr, then wrongly reporting a timeout kill. The tool now returns shortly after the shell itself exits and drains remaining pipe output for a bounded grace period ## 1.49.0 (2026-07-16) diff --git a/src/kimi_cli/tools/shell/__init__.py b/src/kimi_cli/tools/shell/__init__.py index 94d2056f28..42d63b6364 100644 --- a/src/kimi_cli/tools/shell/__init__.py +++ b/src/kimi_cli/tools/shell/__init__.py @@ -1,4 +1,5 @@ import asyncio +import contextlib from collections.abc import Callable from pathlib import Path from typing import Self, override @@ -21,6 +22,18 @@ MAX_FOREGROUND_TIMEOUT = 5 * 60 MAX_BACKGROUND_TIMEOUT = 24 * 60 * 60 +PIPE_DRAIN_GRACE = 2.0 +"""Seconds to keep draining stdout/stderr after the shell process exits. + +A detached child that inherited the pipes can keep them open long after the +shell itself has exited; without a bound the tool would block until the full +command timeout waiting for an EOF that may never come.""" +EXIT_POLL_INTERVAL = 0.05 +"""Seconds between exit checks while the shell process is running. + +`KaosProcess.wait()` may not resolve until all pipes close (asyncio gates its +exit waiters on pipe disconnection), so process exit is observed by polling +`returncode` instead.""" class Params(BaseModel): @@ -244,21 +257,39 @@ async def _read_stream(stream: AsyncReadable, cb: Callable[[bytes], None]): # EOF instead of hanging forever waiting for input that will never come. process.stdin.close() + async def _wait_exit() -> int: + while (exitcode := process.returncode) is None: + await asyncio.sleep(EXIT_POLL_INTERVAL) + return exitcode + + def _consume_exception(task: asyncio.Future[tuple[None, None]]) -> None: + # When the read task is abandoned after cancel (user interrupt or + # command timeout), retrieve its outcome so the event loop does + # not report "exception was never retrieved". + if not task.cancelled(): + task.exception() + + read_task = asyncio.gather( + _read_stream(process.stdout, stdout_cb), + _read_stream(process.stderr, stderr_cb), + ) + read_task.add_done_callback(_consume_exception) try: - await asyncio.wait_for( - asyncio.gather( - _read_stream(process.stdout, stdout_cb), - _read_stream(process.stderr, stderr_cb), - ), - timeout, - ) - return await process.wait() + exitcode = await asyncio.wait_for(_wait_exit(), timeout) except asyncio.CancelledError: + read_task.cancel() await process.kill() raise except TimeoutError: + read_task.cancel() await process.kill() raise + # The shell has exited, but a detached child that inherited the + # pipes can keep them open indefinitely; drain what is left + # instead of waiting for an EOF that may never come. + with contextlib.suppress(TimeoutError): + await asyncio.wait_for(read_task, PIPE_DRAIN_GRACE) + return exitcode def _shell_args(self, command: str) -> tuple[str, ...]: return (str(self._shell_path), "-c", command) diff --git a/tests/tools/test_shell_bash.py b/tests/tools/test_shell_bash.py index 6039882361..27cb74f439 100644 --- a/tests/tools/test_shell_bash.py +++ b/tests/tools/test_shell_bash.py @@ -4,6 +4,7 @@ import asyncio import platform +import time import pytest from inline_snapshot import snapshot @@ -97,6 +98,23 @@ async def test_command_timeout_expires(shell_tool: Shell): assert result.brief == snapshot("Killed by timeout (1s)") +async def test_detached_child_holding_pipes_does_not_block_until_timeout(shell_tool: Shell): + """A detached child that inherits stdout/stderr must not stall the tool. + + The shell exits immediately, but the backgrounded sleep keeps the pipes + open; the tool should return shortly after the shell exits instead of + blocking until the command timeout waiting for pipe EOF. + """ + start = time.monotonic() + result = await shell_tool(Params(command="sleep 30 & echo started", timeout=25)) + elapsed = time.monotonic() - start + assert not result.is_error + assert "started" in result.output + # Shell exit plus PIPE_DRAIN_GRACE, with slack for slow CI; the old + # behavior would block for the full 25s timeout and report an error. + assert elapsed < 20 + + async def test_environment_variables(shell_tool: Shell): """Test setting and using environment variables.""" result = await shell_tool(Params(command="export TEST_VAR='test_value' && echo $TEST_VAR")) @@ -249,6 +267,7 @@ class _FakeProc: stdin = _NullStdin() stdout = _EmptyStream() stderr = _EmptyStream() + returncode: int | None = 0 async def wait(self) -> int: return 0 @@ -375,12 +394,16 @@ def __init__(self) -> None: self.stdout = BlockingReadable() self.stderr = BlockingReadable() self.kill_calls = 0 + self.returncode: int | None = None async def wait(self) -> int: - return 0 + while self.returncode is None: + await asyncio.sleep(0.01) + return self.returncode async def kill(self) -> None: self.kill_calls += 1 + self.returncode = -9 fake_process = FakeProcess()