From 0fb4911d64528efb70ad159a04ef9488bdc805ac Mon Sep 17 00:00:00 2001 From: ayaangazali Date: Tue, 21 Jul 2026 15:17:19 -0700 Subject: [PATCH] fix(shell): stop blocking until timeout when a detached child holds the pipes The foreground shell path waited for stdout/stderr EOF before checking the exit code, so a detached child that inherited the pipes kept the tool blocked for the full command timeout, after which the run was wrongly reported as killed by timeout. Process exit is now observed by polling returncode (asyncio gates its exit waiters on pipe disconnection), and remaining output is drained for a bounded grace period after the shell exits. --- CHANGELOG.md | 1 + src/kimi_cli/tools/shell/__init__.py | 47 +++++++++++++++++++++++----- tests/tools/test_shell_bash.py | 25 ++++++++++++++- 3 files changed, 64 insertions(+), 9 deletions(-) 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()