From 8e8e54633fdfd1047f3aa7fc2c09f614e7e25385 Mon Sep 17 00:00:00 2001 From: evalstate <1936278+evalstate@users.noreply.github.com> Date: Thu, 23 Jul 2026 13:29:28 +0100 Subject: [PATCH] fix(shell): harden persistent process handling for 0.9.22 --- pyproject.toml | 2 +- .../tools/docker_shell_environment.py | 5 +- src/fast_agent/tools/execution_environment.py | 25 +++++++--- src/fast_agent/tools/local_shell_executor.py | 13 +++-- src/fast_agent/tools/shell_output_spool.py | 30 ++++++++++- .../tools/test_execution_environment.py | 17 +++++++ .../test_huggingface_sandbox_environment.py | 1 + .../tools/test_local_shell_output_spool.py | 50 +++++++++++++++++++ uv.lock | 2 +- 9 files changed, 128 insertions(+), 17 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index daf4780d6..1d9447ed1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "fast-agent-mcp" -version = "0.9.21" +version = "0.9.22" description = "Code, Build and Evaluate agents - excellent Model and Skills/MCP/ACP/A2A Support" readme = "README.md" license = { file = "LICENSE" } diff --git a/src/fast_agent/tools/docker_shell_environment.py b/src/fast_agent/tools/docker_shell_environment.py index d7bb03531..bb8bf8956 100644 --- a/src/fast_agent/tools/docker_shell_environment.py +++ b/src/fast_agent/tools/docker_shell_environment.py @@ -32,6 +32,9 @@ _STREAM_READ_CHUNK_SIZE = 4096 _PROCESS_EXIT_POLL_SECONDS = 0.1 +# Each spool poll spawns one `docker exec` per stream, so detached tailing uses +# a slower cadence than in-process exit polling to keep dockerd load bounded. +_SPOOL_TAIL_POLL_SECONDS = 0.5 _IDLE_POLL_SECONDS = 1.0 _MANAGED_PROCESS_DISCOVERY_TIMEOUT_SECONDS = 5.0 _MANAGED_PROCESS_TERM_GRACE_SECONDS = 2.0 @@ -322,7 +325,7 @@ async def process_exited() -> bool: asyncio.create_task( tailer.tail_until( process_exited, - poll_interval=_PROCESS_EXIT_POLL_SECONDS, + poll_interval=_SPOOL_TAIL_POLL_SECONDS, ) ) ] diff --git a/src/fast_agent/tools/execution_environment.py b/src/fast_agent/tools/execution_environment.py index e4db727b3..cb0a3fa28 100644 --- a/src/fast_agent/tools/execution_environment.py +++ b/src/fast_agent/tools/execution_environment.py @@ -54,13 +54,17 @@ class ShellExecutionRequest: ``terminate_on_cancel`` controls whether cancellation terminates the process tree. ``detach`` requires the child to survive runtime exit, so adapters must - capture output without parent-owned pipes. ``retain_output`` controls whether - joined output strings are retained in the returned result; callbacks remain - independent of result retention. - - Owners may set ``terminate_on_cancel`` before cancelling an active request to - distinguish explicit termination from persistent runtime shutdown. Adapters - may populate ``output_spool_path`` while detached output remains on disk. + capture output without parent-owned pipes. A request that starts with + ``terminate_on_cancel=False`` must therefore also set ``detach=True``; a + surviving child attached to parent-owned pipes would block or die on SIGPIPE + once the runtime exits. ``retain_output`` controls whether joined output + strings are retained in the returned result; callbacks remain independent of + result retention. + + Owners may set ``terminate_on_cancel`` to ``True`` before cancelling an + active request to distinguish explicit termination from persistent runtime + shutdown. Adapters may populate ``output_spool_path`` while detached output + remains on disk. """ command: str @@ -73,6 +77,13 @@ class ShellExecutionRequest: detach: bool = False output_spool_path: str | None = None + def __post_init__(self) -> None: + if not self.terminate_on_cancel and not self.detach: + raise ValueError( + "terminate_on_cancel=False requires detach=True: a child that " + "survives cancellation cannot use parent-owned output pipes" + ) + @dataclass(frozen=True, slots=True) class ShellExecutionResult: diff --git a/src/fast_agent/tools/local_shell_executor.py b/src/fast_agent/tools/local_shell_executor.py index 88141906e..8aad07950 100644 --- a/src/fast_agent/tools/local_shell_executor.py +++ b/src/fast_agent/tools/local_shell_executor.py @@ -219,15 +219,18 @@ async def execute( detach=request.detach, ) try: - process = await self._start_shell_process(request.command, plan) + try: + process = await self._start_shell_process(request.command, plan) + finally: + # Close before any spool deletion: Windows cannot remove a + # directory containing open files. + if plan.output_files is not None: + for output_file in plan.output_files: + output_file.close() except BaseException: if plan.output_spool is not None: delete_local_output_spool(plan.output_spool) raise - finally: - if plan.output_files is not None: - for output_file in plan.output_files: - output_file.close() if plan.output_spool is not None: request.output_spool_path = plan.output_spool.directory if callbacks is not None: diff --git a/src/fast_agent/tools/shell_output_spool.py b/src/fast_agent/tools/shell_output_spool.py index 5b2387b93..347e3f6a6 100644 --- a/src/fast_agent/tools/shell_output_spool.py +++ b/src/fast_agent/tools/shell_output_spool.py @@ -5,12 +5,15 @@ import asyncio import codecs import os +import stat import tempfile from dataclasses import dataclass from pathlib import Path from shutil import rmtree from typing import BinaryIO, Protocol +_FINAL_DRAIN_PAUSE_SECONDS = 0.05 + class SpoolChunkReader(Protocol): async def __call__(self, path: str, offset: int, size: int) -> bytes: ... @@ -68,7 +71,9 @@ async def tail_until( await asyncio.sleep(poll_interval) while not await self._emit_deltas(): - pass + # A surviving descendant may still be appending; yield between + # catch-up rounds instead of spinning at full read speed. + await asyncio.sleep(_FINAL_DRAIN_PAUSE_SECONDS) stdout_tail = self._stdout_decoder.decode(b"", final=True) stderr_tail = self._stderr_decoder.decode(b"", final=True) @@ -107,8 +112,29 @@ async def _read_available(self, path: str, offset: int) -> tuple[bytes, bool]: return b"".join(chunks), False +def _local_spool_root() -> str | None: + """Return a stable per-user root so leftover spools stay discoverable. + + Falls back to ``None`` (system temp) unless the root is a private directory + owned by the current user, so an attacker-created shared-temp entry can + never become the parent of a spool. + """ + suffix = f"-{os.getuid()}" if hasattr(os, "getuid") else "" + root = Path(tempfile.gettempdir()) / f"fast-agent-managed{suffix}" + try: + root.mkdir(mode=0o700, exist_ok=True) + details = root.lstat() + if not stat.S_ISDIR(details.st_mode) or details.st_mode & 0o022: + return None + if hasattr(os, "getuid") and details.st_uid != os.getuid(): + return None + except OSError: + return None + return str(root) + + def create_local_output_spool() -> ShellOutputSpoolPaths: - directory = Path(tempfile.mkdtemp(prefix="fast-agent-managed-")) + directory = Path(tempfile.mkdtemp(prefix="fast-agent-managed-", dir=_local_spool_root())) directory.chmod(0o700) stdout = directory / "stdout.log" stderr = directory / "stderr.log" diff --git a/tests/unit/fast_agent/tools/test_execution_environment.py b/tests/unit/fast_agent/tools/test_execution_environment.py index 12592cf8a..6e192591a 100644 --- a/tests/unit/fast_agent/tools/test_execution_environment.py +++ b/tests/unit/fast_agent/tools/test_execution_environment.py @@ -1,5 +1,7 @@ from __future__ import annotations +import pytest + from fast_agent.tools.execution_environment import ( ShellEnvironment, ShellExecution, @@ -47,3 +49,18 @@ def test_shell_runtime_info_is_typed() -> None: assert info.name == "bash" assert info.kind == "docker" + + +def test_shell_execution_request_rejects_surviving_child_with_pipes() -> None: + with pytest.raises(ValueError, match="requires detach=True"): + ShellExecutionRequest(command="sleep 60", terminate_on_cancel=False) + + +def test_shell_execution_request_allows_detached_survivor() -> None: + request = ShellExecutionRequest( + command="sleep 60", + terminate_on_cancel=False, + detach=True, + ) + + assert request.detach is True diff --git a/tests/unit/fast_agent/tools/test_huggingface_sandbox_environment.py b/tests/unit/fast_agent/tools/test_huggingface_sandbox_environment.py index 6036556c4..7455b983a 100644 --- a/tests/unit/fast_agent/tools/test_huggingface_sandbox_environment.py +++ b/tests/unit/fast_agent/tools/test_huggingface_sandbox_environment.py @@ -500,6 +500,7 @@ async def test_managed_execute_persistent_cancellation_leaves_remote_process_run command="sleep 60", terminate_after_idle=False, terminate_on_cancel=False, + detach=True, ), callbacks=callbacks, ) diff --git a/tests/unit/fast_agent/tools/test_local_shell_output_spool.py b/tests/unit/fast_agent/tools/test_local_shell_output_spool.py index 00dadf731..abca46d9e 100644 --- a/tests/unit/fast_agent/tools/test_local_shell_output_spool.py +++ b/tests/unit/fast_agent/tools/test_local_shell_output_spool.py @@ -191,6 +191,56 @@ async def test_completed_detached_execution_removes_drained_spool( assert request.output_spool_path is None +@pytest.mark.asyncio +@pytest.mark.skipif(platform.system() == "Windows", reason="Unix process groups") +async def test_runtime_close_leaves_real_persistent_background_process_running( + tmp_path: Path, +) -> None: + script = tmp_path / "service.py" + script.write_text( + "\n".join( + [ + "import time", + "while True:", + " print('serving', flush=True)", + " time.sleep(0.05)", + ] + ), + encoding="utf-8", + ) + runtime = ShellRuntime( + activation_reason="test", + logger=logging.getLogger(__name__), + working_directory=tmp_path, + config=Settings(shell_execution=ShellSettings(show_bash=False)), + ) + + await runtime.execute( + { + "command": f'exec "{sys.executable}" "{script}"', + "background": True, + } + ) + snapshot = (await runtime.process_snapshots())[0] + assert snapshot.os_process_id is not None + assert snapshot.output_spool_path is not None + spool_path = Path(snapshot.output_spool_path) + stdout_path = spool_path / "stdout.log" + + await runtime.close() + + try: + os.kill(snapshot.os_process_id, 0) + initial_size = await _wait_for_file_growth(stdout_path) + await _wait_for_file_growth(stdout_path, initial_size=initial_size) + finally: + try: + os.killpg(snapshot.os_process_id, signal.SIGTERM) + except ProcessLookupError: + pass + rmtree(spool_path, ignore_errors=True) + + @pytest.mark.asyncio @pytest.mark.skipif(platform.system() == "Windows", reason="Unix process groups") async def test_runtime_close_terminates_real_session_background_process( diff --git a/uv.lock b/uv.lock index 773afd251..22593d328 100644 --- a/uv.lock +++ b/uv.lock @@ -863,7 +863,7 @@ requires-dist = [{ name = "fast-agent-mcp", editable = "." }] [[package]] name = "fast-agent-mcp" -version = "0.9.21" +version = "0.9.22" source = { editable = "." } dependencies = [ { name = "a2a-sdk" },