diff --git a/README.md b/README.md index 5e81ec6b4..43a674b74 100644 --- a/README.md +++ b/README.md @@ -651,7 +651,7 @@ DeerFlow doesn't just *talk* about doing things. It has its own computer. Each task gets its own execution environment with a full filesystem view — skills, workspace, uploads, outputs. The agent reads, writes, and edits files. It can view images and, when configured safely, execute shell commands. -With `AioSandboxProvider`, shell execution runs inside isolated containers. With `LocalSandboxProvider`, file tools still map to per-thread directories on the host, but host `bash` is disabled by default because it is not a secure isolation boundary. Re-enable host bash only for fully trusted local workflows. +With `AioSandboxProvider`, shell execution runs inside isolated containers. With `LocalSandboxProvider`, file tools still map to per-thread directories on the host, but host `bash` is disabled by default because it is not a secure isolation boundary. Re-enable host bash only for fully trusted local workflows. Host bash commands have a wall-clock timeout, and long-lived processes should be started in the background with output redirected to a workspace log. This is the difference between a chatbot with tool access and an agent with an actual execution environment. diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 5d0ab434d..4e850c4e1 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -315,7 +315,7 @@ Proxied through nginx: `/api/langgraph/*` → Gateway LangGraph-compatible runti - Detection: `is_local_sandbox()` accepts both `sandbox_id == "local"` (legacy / no-thread) and `sandbox_id.startswith("local:")` (per-thread) **Sandbox Tools** (in `packages/harness/deerflow/sandbox/tools.py`): -- `bash` - Execute commands with path translation and error handling +- `bash` - Execute commands with path translation and error handling. For `LocalSandbox` (host bash), POSIX output is captured through bounded pipe-drain threads and stdin is `/dev/null`, so a backgrounded long-lived process (`server &`) returns immediately instead of blocking the turn on an inherited pipe, while unredirected background output is drained without growing anonymous temp files. Commands that read stdin get immediate EOF. The command runs in its own process group with a wall-clock timeout (`sandbox.bash_command_timeout`, default 600s); on timeout the whole group is killed and the agent gets a notice telling it to background long-lived processes. The bash tool description itself also instructs the model to background long-lived processes (e.g. servers) up front so it doesn't waste the turn waiting on a foreground server. See `LocalSandbox.execute_command` / `_run_posix_command` and `bash_tool`'s docstring. - `ls` - Directory listing (tree format, max 2 levels) - `read_file` - Read file contents with optional line range - `write_file` - Write/append to files, creates directories; overwrites by default and exposes the `append` argument in the model-facing schema for end-of-file writes diff --git a/backend/packages/harness/deerflow/config/sandbox_config.py b/backend/packages/harness/deerflow/config/sandbox_config.py index 7aac60003..d8434ccf8 100644 --- a/backend/packages/harness/deerflow/config/sandbox_config.py +++ b/backend/packages/harness/deerflow/config/sandbox_config.py @@ -92,5 +92,13 @@ class SandboxConfig(BaseModel): ge=0, description="Maximum characters to keep from ls tool output. Output exceeding this limit is head-truncated. Set to 0 to disable truncation.", ) + bash_command_timeout: int = Field( + default=600, + gt=0, + description=( + "Maximum wall-clock seconds a host bash command may run before it is terminated, process group and all (LocalSandboxProvider). " + "Keeps a blocking foreground command (e.g. an un-backgrounded server) from hanging the turn; background `&` processes return immediately." + ), + ) model_config = ConfigDict(extra="allow") diff --git a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py index 4f3a6e315..a1d337f9e 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py @@ -4,7 +4,9 @@ import ntpath import os import re import shutil +import signal import subprocess +import threading from dataclasses import dataclass from functools import cached_property from pathlib import Path @@ -17,6 +19,49 @@ from deerflow.sandbox.search import GrepMatch, find_glob_matches, find_grep_matc logger = logging.getLogger(__name__) +# Default wall-clock timeout (seconds) for a single host bash command. A +# blocking foreground command (for example a server started without +# backgrounding) is terminated after this long so the agent's turn cannot hang +# indefinitely. Overridable per call via ``execute_command(timeout=...)`` and, +# for the bash tool, via ``sandbox.bash_command_timeout`` in config.yaml. +DEFAULT_COMMAND_TIMEOUT_SECONDS = 600 +_COMMAND_CAPTURE_LIMIT_BYTES = 10 * 1024 * 1024 +_PIPE_DRAIN_JOIN_TIMEOUT_SECONDS = 0.2 + + +class _BoundedPipeCapture: + """Drain a subprocess pipe while keeping only bounded output in memory.""" + + def __init__(self, *, limit_bytes: int = _COMMAND_CAPTURE_LIMIT_BYTES) -> None: + self._limit_bytes = limit_bytes + self._chunks: list[bytes] = [] + self._kept_bytes = 0 + self._total_bytes = 0 + self._lock = threading.Lock() + + def append(self, chunk: bytes) -> None: + with self._lock: + self._total_bytes += len(chunk) + if self._kept_bytes >= self._limit_bytes: + return + remaining = self._limit_bytes - self._kept_bytes + kept = chunk[:remaining] + self._chunks.append(kept) + self._kept_bytes += len(kept) + + def read(self) -> str: + with self._lock: + data = b"".join(self._chunks) + truncated = self._total_bytes > self._kept_bytes + total_bytes = self._total_bytes + kept_bytes = self._kept_bytes + + output = data.decode("utf-8", errors="replace") + if truncated: + notice = f"\n... [output truncated after {kept_bytes} of {total_bytes} bytes; remaining output discarded] ..." + output += notice + return output + @dataclass(frozen=True) class PathMapping: @@ -70,6 +115,67 @@ class LocalSandbox(Sandbox): return None + @staticmethod + def _format_timeout_duration(timeout: float) -> str: + seconds = float(timeout) + if seconds.is_integer(): + amount = str(int(seconds)) + else: + amount = f"{seconds:g}" + unit = "second" if seconds == 1 else "seconds" + return f"{amount} {unit}" + + @staticmethod + def _format_timeout_notice(timeout: float) -> str: + return ( + f"Command timed out after {LocalSandbox._format_timeout_duration(timeout)} and was terminated. " + "To run a long-lived process such as a web server, start it in the background " + "and redirect its output, e.g. `your-command > /mnt/user-data/workspace/server.log 2>&1 &`." + ) + + @staticmethod + def _coerce_process_output(value: str | bytes | None) -> str: + if value is None: + return "" + if isinstance(value, bytes): + return value.decode("utf-8", errors="replace") + return value + + @staticmethod + def _drain_pipe(fd: int, capture: _BoundedPipeCapture) -> None: + try: + while chunk := os.read(fd, 8192): + capture.append(chunk) + except OSError: + logger.debug("Subprocess output pipe closed while draining", exc_info=True) + finally: + try: + os.close(fd) + except OSError: + # The fd may already be closed during pipe teardown; cleanup is best-effort. + pass + + @staticmethod + def _start_pipe_drain(fd: int, name: str) -> tuple[_BoundedPipeCapture, threading.Thread]: + capture = _BoundedPipeCapture() + thread = threading.Thread(target=LocalSandbox._drain_pipe, args=(fd, capture), name=name, daemon=True) + thread.start() + return capture, thread + + @staticmethod + def _process_group_exists(pgid: int | None) -> bool: + if pgid is None: + return False + try: + os.killpg(pgid, 0) + return True + except ProcessLookupError: + return False + except PermissionError: + return True + except OSError: + return False + def __init__(self, id: str, path_mappings: list[PathMapping] | None = None): """ Initialize local sandbox with optional path mappings. @@ -327,11 +433,14 @@ class LocalSandbox(Sandbox): raise RuntimeError("No suitable shell executable found. Tried /bin/zsh, /bin/bash, /bin/sh, and `sh` on PATH.") - def execute_command(self, command: str) -> str: + def execute_command(self, command: str, timeout: float | None = None) -> str: # Resolve container paths in command before execution resolved_command = self._resolve_paths_in_command(command) shell = self._get_shell() + if timeout is None: + timeout = DEFAULT_COMMAND_TIMEOUT_SECONDS + timed_out = False if os.name == "nt": env = None if self._is_powershell(shell): @@ -347,33 +456,132 @@ class LocalSandbox(Sandbox): "MSYS2_ARG_CONV_EXCL": "*", } - result = subprocess.run( - args, - shell=False, - capture_output=True, - text=True, - timeout=600, - env=env, - ) + try: + result = subprocess.run( + args, + shell=False, + capture_output=True, + text=True, + timeout=timeout, + env=env, + ) + stdout, stderr, returncode = result.stdout, result.stderr, result.returncode + except subprocess.TimeoutExpired as exc: + timed_out = True + stdout = self._coerce_process_output(exc.stdout if exc.stdout is not None else exc.output) + stderr = self._coerce_process_output(exc.stderr) + returncode = 0 else: args = [shell, "-c", resolved_command] - result = subprocess.run( - args, - shell=False, - capture_output=True, - text=True, - timeout=600, - ) - output = result.stdout - if result.stderr: - output += f"\nStd Error:\n{result.stderr}" if output else result.stderr - if result.returncode != 0: - output += f"\nExit Code: {result.returncode}" + stdout, stderr, returncode, timed_out = self._run_posix_command(args, timeout) + + output = stdout + if stderr: + output += f"\nStd Error:\n{stderr}" if output else stderr + if timed_out: + notice = self._format_timeout_notice(timeout) + output += f"\n{notice}" if output else notice + elif returncode != 0: + output += f"\nExit Code: {returncode}" final_output = output if output else "(no output)" # Reverse resolve local paths back to container paths in output return self._reverse_resolve_paths_in_output(final_output) + @staticmethod + def _run_posix_command(args: list[str], timeout: float) -> tuple[str, str, int, bool]: + """Run a command on POSIX with bounded pipe capture. + + ``subprocess.communicate()`` cannot be used here: a backgrounded + long-lived process (``server &``) inherits stdout/stderr and keeps the + pipes open, so ``communicate()`` would block until timeout even though + the foreground shell already returned. Instead, daemon drain threads + keep the pipes flowing while retaining only bounded output in memory. + This lets the call return as soon as the foreground shell exits without + handing backgrounded processes anonymous temp files that can grow + invisibly. ``stdin`` is taken from ``/dev/null`` so commands that read + stdin get immediate EOF, and ``start_new_session`` puts the command in + its own process group so a genuinely blocking foreground command can be + killed in full (children included) when it times out. + + Returns ``(stdout, stderr, returncode, timed_out)``. + """ + timed_out = False + stdout_read_fd, stdout_write_fd = os.pipe() + stderr_read_fd, stderr_write_fd = os.pipe() + try: + process = subprocess.Popen( + args, + shell=False, + stdin=subprocess.DEVNULL, + stdout=stdout_write_fd, + stderr=stderr_write_fd, + start_new_session=True, + ) + except Exception: + for fd in (stdout_read_fd, stdout_write_fd, stderr_read_fd, stderr_write_fd): + try: + os.close(fd) + except OSError: + # Preserve the original Popen failure; fd cleanup is best-effort. + pass + raise + finally: + for fd in (stdout_write_fd, stderr_write_fd): + try: + os.close(fd) + except OSError: + # The write fd may already be closed by the exception cleanup above. + pass + + stdout_capture, stdout_thread = LocalSandbox._start_pipe_drain(stdout_read_fd, "deerflow-bash-stdout-drain") + stderr_capture, stderr_thread = LocalSandbox._start_pipe_drain(stderr_read_fd, "deerflow-bash-stderr-drain") + try: + process_group_id = os.getpgid(process.pid) + except OSError: + process_group_id = None + + try: + try: + process.wait(timeout=timeout) + except subprocess.TimeoutExpired: + timed_out = True + LocalSandbox._terminate_process_group(process) + returncode = process.returncode if process.returncode is not None else 0 + finally: + join_timeout = 10 if timed_out or not LocalSandbox._process_group_exists(process_group_id) else _PIPE_DRAIN_JOIN_TIMEOUT_SECONDS + for thread in (stdout_thread, stderr_thread): + thread.join(timeout=join_timeout) + if thread.is_alive(): + logger.debug("Subprocess output drain thread still active after command returned") + + stdout = stdout_capture.read() + stderr = stderr_capture.read() + return stdout, stderr, returncode, timed_out + + @staticmethod + def _terminate_process_group(process: subprocess.Popen) -> None: + """Kill the command's whole process group, then reap it. + + Falls back to killing just the direct child if the group is already + gone (e.g. the command exited between the timeout and this call). + """ + try: + os.killpg(os.getpgid(process.pid), signal.SIGKILL) + except (ProcessLookupError, PermissionError, OSError): + # The process group is already gone (the command exited in the race + # between the timeout and this call); fall back to killing just the + # direct child. + try: + process.kill() + except OSError: + # Direct child already reaped too — nothing left to kill. + logger.debug("Process %s already exited before fallback kill", process.pid) + try: + process.wait(timeout=10) + except subprocess.TimeoutExpired: + logger.warning("Process group for pid %s did not exit after SIGKILL", process.pid) + def list_dir(self, path: str, max_depth=2) -> list[str]: resolved_path = self._resolve_path(path) entries = list_dir(resolved_path, max_depth) diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index 8d322ae1a..c963ec427 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -1393,6 +1393,10 @@ def bash_tool(runtime: Runtime, description: str, command: str) -> str: - Use `python` to run Python code. - Prefer a thread-local virtual environment in `/mnt/user-data/workspace/.venv`. - Use `python -m pip` (inside the virtual environment) to install Python packages. + - To start a long-lived process such as a web server, ALWAYS run it in the background with its + output redirected, e.g. `your-command > /mnt/user-data/workspace/server.log 2>&1 &`, then check + the log file or poll the port. A long-lived process run in the foreground blocks the turn until + it is killed at the command timeout. Args: description: Explain why you are running this command in short words. ALWAYS PROVIDE THIS PARAMETER FIRST. @@ -1408,14 +1412,16 @@ def bash_tool(runtime: Runtime, description: str, command: str) -> str: validate_local_bash_command_paths(command, thread_data) command = replace_virtual_paths_in_command(command, thread_data) command = _apply_cwd_prefix(command, thread_data) - output = sandbox.execute_command(command) try: from deerflow.config.app_config import get_app_config sandbox_cfg = get_app_config().sandbox max_chars = sandbox_cfg.bash_output_max_chars if sandbox_cfg else 20000 + command_timeout = sandbox_cfg.bash_command_timeout if sandbox_cfg else None except Exception: max_chars = 20000 + command_timeout = None + output = sandbox.execute_command(command, timeout=command_timeout) return _truncate_bash_output(mask_local_paths_in_output(output, thread_data), max_chars) ensure_thread_directories_exist(runtime) try: diff --git a/backend/tests/test_local_sandbox_command_timeout.py b/backend/tests/test_local_sandbox_command_timeout.py new file mode 100644 index 000000000..29cf84b59 --- /dev/null +++ b/backend/tests/test_local_sandbox_command_timeout.py @@ -0,0 +1,158 @@ +"""Regression tests for blocking-command timeout handling in LocalSandbox. + +These pin the fix for the "starting a server hangs the whole turn" bug: +a backgrounded long-lived process must not keep the bash tool blocked until +the timeout, and a genuinely blocking foreground command must be terminated +(process group and all) once it exceeds the timeout. + +The POSIX cases exercise real subprocess/process-group semantics, so they are +skipped on Windows. Windows keeps the ``subprocess.run`` path, but timeout +errors still use the same user-facing notice. +""" + +import os +import shlex +import sys +import time +from pathlib import Path + +import pytest + +from deerflow.config.sandbox_config import SandboxConfig +from deerflow.sandbox.local import local_sandbox +from deerflow.sandbox.local.local_sandbox import LocalSandbox + +posix_only = pytest.mark.skipif(os.name == "nt", reason="POSIX process-group semantics") +linux_proc_fd_only = pytest.mark.skipif(not Path("/proc/self/fd").exists(), reason="requires Linux /proc fd links") + + +@posix_only +def test_backgrounded_process_returns_promptly(): + """A backgrounded long-lived process (e.g. a dev server started with `&`) + must return as soon as the foreground command finishes, instead of + blocking the bash tool until the timeout because it inherited the + captured pipe.""" + sandbox = LocalSandbox("t") + start = time.monotonic() + output = sandbox.execute_command("sleep 5 & echo serving", timeout=10) + elapsed = time.monotonic() - start + + assert elapsed < 3, f"expected prompt return, took {elapsed:.1f}s" + assert "serving" in output + + +@posix_only +@linux_proc_fd_only +def test_backgrounded_process_does_not_inherit_deleted_temp_capture(tmp_path): + """A backgrounded process that forgets to redirect output must not inherit + an anonymous deleted temp file for fd 1. That would be an invisible, + unbounded disk leak for long-lived processes that keep writing.""" + marker = tmp_path / "fd1" + script = f"import os, pathlib, time; pathlib.Path({str(marker)!r}).write_text(os.readlink('/proc/self/fd/1')); time.sleep(2)" + sandbox = LocalSandbox("t") + + output = sandbox.execute_command(f"{shlex.quote(sys.executable)} -c {shlex.quote(script)} & echo launched", timeout=10) + + assert "launched" in output + for _ in range(50): + if marker.exists(): + break + time.sleep(0.1) + assert marker.exists() + assert " (deleted)" not in marker.read_text() + + +@posix_only +def test_foreground_blocking_command_times_out_with_notice(): + """A foreground command that never exits is terminated at the timeout and + the agent receives an explanatory notice instead of a generic error.""" + sandbox = LocalSandbox("t") + start = time.monotonic() + output = sandbox.execute_command("while true; do sleep 0.2; done", timeout=1) + elapsed = time.monotonic() - start + + assert elapsed < 5, f"timeout not enforced, took {elapsed:.1f}s" + assert "timed out" in output.lower() + + +def test_timeout_notice_formats_fractional_and_singular_timeouts(monkeypatch): + monkeypatch.setattr(LocalSandbox, "_get_shell", lambda self: "/bin/sh") + monkeypatch.setattr(LocalSandbox, "_run_posix_command", staticmethod(lambda args, timeout: ("", "", 0, True))) + + assert "after 1.5 seconds" in LocalSandbox("t").execute_command("wait", timeout=1.5) + assert "after 1 second" in LocalSandbox("t").execute_command("wait", timeout=1) + + +def test_windows_timeout_expired_returns_notice(monkeypatch): + def fake_run(*args, **kwargs): + raise local_sandbox.subprocess.TimeoutExpired(cmd=args[0], timeout=kwargs["timeout"], output="partial out", stderr="partial err") + + monkeypatch.setattr(local_sandbox.os, "name", "nt") + monkeypatch.setattr(LocalSandbox, "_get_shell", lambda self: "cmd.exe") + monkeypatch.setattr(local_sandbox.subprocess, "run", fake_run) + + output = LocalSandbox("t").execute_command("wait", timeout=1.5) + + assert "partial out" in output + assert "Std Error:" in output + assert "partial err" in output + assert "after 1.5 seconds" in output + assert "Unexpected error" not in output + + +@posix_only +def test_foreground_timeout_kills_whole_process_group(tmp_path): + """On timeout the entire process group is killed, not just the direct + child, so child processes spawned by the command do not survive.""" + marker = tmp_path / "alive" + sandbox = LocalSandbox("t") + sandbox.execute_command(f"while true; do touch {marker}; sleep 0.2; done", timeout=1) + + assert marker.exists() + first_mtime = marker.stat().st_mtime + time.sleep(1.5) + assert marker.stat().st_mtime == first_mtime, "process group survived the timeout" + + +@posix_only +def test_command_reading_stdin_does_not_block(): + """stdin is redirected from /dev/null, so a command that reads stdin gets + immediate EOF instead of blocking until the timeout.""" + sandbox = LocalSandbox("t") + start = time.monotonic() + output = sandbox.execute_command("read x; echo got", timeout=10) + elapsed = time.monotonic() - start + + assert elapsed < 3, f"stdin read blocked, took {elapsed:.1f}s" + assert "got" in output + + +@posix_only +def test_normal_command_output_exit_code_and_stderr(): + """Ordinary commands keep their existing output contract: stdout, + appended Std Error section, and a non-zero Exit Code line.""" + sandbox = LocalSandbox("t") + + assert "hello" in sandbox.execute_command("echo hello") + assert "Exit Code: 3" in sandbox.execute_command("exit 3") + + combined = sandbox.execute_command("echo out; echo oops >&2") + assert "out" in combined + assert "Std Error:" in combined + assert "oops" in combined + + +def test_sandbox_config_exposes_command_timeout_default(): + cfg = SandboxConfig(use="deerflow.sandbox.local:LocalSandboxProvider") + assert cfg.bash_command_timeout == 600 + + +def test_bash_tool_description_guides_backgrounding_long_lived_processes(): + """The bash tool description (seen by the model) must tell it to background + long-lived processes like servers, so it doesn't block the turn in the + foreground. This is the prompt-side half of the server-hang fix.""" + from deerflow.sandbox.tools import bash_tool + + description = bash_tool.description.lower() + assert "background" in description + assert "server" in description diff --git a/backend/tests/test_local_sandbox_provider_mounts.py b/backend/tests/test_local_sandbox_provider_mounts.py index a9b0ec63d..8f87d03f8 100644 --- a/backend/tests/test_local_sandbox_provider_mounts.py +++ b/backend/tests/test_local_sandbox_provider_mounts.py @@ -488,16 +488,18 @@ class TestMultipleMounts: ], ) - # Mock subprocess to capture the resolved command + # Mock subprocess to capture the resolved command. The POSIX path runs + # commands via subprocess.Popen, so wrap that and still execute the real + # command. captured = {} - original_run = __import__("subprocess").run + original_popen = __import__("subprocess").Popen - def mock_run(*args, **kwargs): + def mock_popen(*args, **kwargs): if len(args) > 0: captured["command"] = args[0] - return original_run(*args, **kwargs) + return original_popen(*args, **kwargs) - monkeypatch.setattr("deerflow.sandbox.local.local_sandbox.subprocess.run", mock_run) + monkeypatch.setattr("deerflow.sandbox.local.local_sandbox.subprocess.Popen", mock_popen) monkeypatch.setattr("deerflow.sandbox.local.local_sandbox.LocalSandbox._get_shell", lambda self: "/bin/sh") sandbox.execute_command("cat /mnt/data/test.txt") diff --git a/config.example.yaml b/config.example.yaml index 76ac63476..30fd05136 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -937,6 +937,15 @@ sandbox: read_file_output_max_chars: 50000 ls_output_max_chars: 20000 + # Maximum wall-clock seconds a single host bash command may run before it is + # terminated (process group and all). A blocking foreground command — e.g. a + # server started without backgrounding — is killed after this long so the + # agent's turn cannot hang. Start long-lived processes in the background with + # output redirected (e.g. `your-command > /tmp/server.log 2>&1 &`) when you + # need logs; unredirected background output is drained with bounded capture + # and excess output is discarded. + bash_command_timeout: 600 + # Option 2: Container-based AIO Sandbox # Executes commands in isolated containers (Docker or Apple Container) # On macOS: Automatically prefers Apple Container if available, falls back to Docker