From 2ce9bdc0036a3cc6d234437bcad0f057c7a99adf Mon Sep 17 00:00:00 2001 From: Willem Jiang Date: Sun, 25 Oct 2026 11:49:44 +0800 Subject: [PATCH] fix(sandbox): close pipe fds once after post-spawn failures (#7430) * fix(sandbox): close pipe fds once after post-spawn failures * fix(sandbox): reap children after startup failures --- README.md | 2 +- .../harness/deerflow/sandbox/AGENTS.md | 5 +- .../deerflow/sandbox/local/local_sandbox.py | 70 ++++++----- .../test_local_sandbox_pipe_fd_lifecycle.py | 109 +++++++++++++++++- 4 files changed, 151 insertions(+), 35 deletions(-) diff --git a/README.md b/README.md index d14f48396c..e1e6ad97ed 100644 --- a/README.md +++ b/README.md @@ -2981,7 +2981,7 @@ continue to accept URL-encoded paths. The `write_file` guidance reflects the active model's output token limit, including custom-agent and thinking-mode overrides. For longer documents, the agent is guided to write sections with `append=True`; models without a known limit receive no numeric budget hint. -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. On Windows, Git Bash/MSYS argument-conversion exclusions are limited to safe non-root virtual path prefixes, so host-native CLI launchers retain their normal MSYS compatibility. When the local sandbox falls back to PowerShell, it captures output as UTF-8 so CJK text does not depend on the Gateway host locale. +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. If local command startup fails after spawning a child, DeerFlow attempts to kill and reap that child before propagating the startup error. On Windows, Git Bash/MSYS argument-conversion exclusions are limited to safe non-root virtual path prefixes, so host-native CLI launchers retain their normal MSYS compatibility. When the local sandbox falls back to PowerShell, it captures output as UTF-8 so CJK text does not depend on the Gateway host locale. Docker AIO sandboxes default to their existing open egress behavior for compatibility. Operators can set `sandbox.network.mode` to `isolated` or diff --git a/backend/packages/harness/deerflow/sandbox/AGENTS.md b/backend/packages/harness/deerflow/sandbox/AGENTS.md index 93e9ef443c..a92ed6221b 100644 --- a/backend/packages/harness/deerflow/sandbox/AGENTS.md +++ b/backend/packages/harness/deerflow/sandbox/AGENTS.md @@ -2,6 +2,9 @@ Sandbox restore requires a thread ID, even forks. +On startup failure, close reads and kill/reap the child, preserving the error. +Only `finally` closes write ends. + On Linux/macOS, local non-append text saves use `_publish_file_revision` and the shared atomic byte publisher. Check write access without truncation and keep the source descriptor open through metadata transfer. Resolve symlinks under existing mapping rules; publish only regular files. Failure before replace retains the old file and cleans staging. Append and other platforms keep their in-place behavior. Replacement requires parent-directory write and metadata-preservation permissions; inode identity changes, so hard-link aliases retain the old version. It is not concurrent-writer arbitration or power-loss recovery. Regressions: `tests/test_local_sandbox_atomic_writes.py`. PowerShell command mapping escapes host-path whitespace/metacharacters for the caller's quote context in both user-data and provider mappings. Preserve wildcards; Bash/cmd mapping stays unchanged. Keep portable string assertions alongside native Windows smoke tests. The public bash tool's existing POSIX-quoted cwd prefix requires PowerShell 7 (`&&`) and workspace paths without apostrophes; mapping does not repair that prefix. @@ -133,7 +136,7 @@ subshell. Keep parser filtering as a backstop; test ignored roots, metacharacter symlinks, and visible depth/size bounds. - Every sandbox tool keeps a model-visible `description` field for a human-readable progress label, but the field is optional and defaults to an empty string. Tool execution must depend only on its operational arguments; the frontend supplies localized fallback labels when a provider omits `description`. -- `bash` - Execute commands with path translation and error handling. For `LocalSandbox`, POSIX/Windows output uses bounded pipe-drain threads with `/dev/null` stdin; Windows decodes locale-code-page/UTF-8/CRLF/bare-CR output with universal-newline translation, while POSIX stays byte-decoded. POSIX background commands return without blocking on inherited pipes; unredirected output is drained without unbounded temp files. Commands that read stdin get immediate EOF. `bash_command_timeout` is forwarded to providers with a per-command timeout API (Local/AIO/OpenSandbox/E2B/BoxLite/Tenki); providers without one keep their own defaults. Local: T is a wall-clock process-group deadline. Supported-semver AIO: server-side `hard_timeout`; frozen legacy `all-in-one-sandbox:latest`: command requests get a bounded host `T+5s` wait (`max_retries=0`), so a wedged command request cannot hold the sandbox lock for the SDK's full 600s budget; file/list RPCs are not bounded by it. Explicit AIO shell/bash session creation uses a separate 5s/no-retry host bound; an ambiguous create is never replayed and forces container recycle before warm reuse. Never replay ambiguous outcomes: transport timeout, terminated, no_change_timeout, unknown statuses. `hard_timeout` => terminated + Exit Code: 124, keep session; `no_change_timeout` => may still be running, fence session generation. The description scopes host-environment probes to LocalSandbox (`uname -s`, then `sw_vers` on Darwin; Linux files only when policy permits), gives conditional recovery for local path/`file://` rejections, and tells the model to background long-lived processes. See `LocalSandbox.execute_command`, its platform runners, and `bash_tool`'s docstring. +- `bash` - Path-translated commands with error handling. Local POSIX/Windows use bounded pipe-drain threads and `/dev/null` stdin; Windows decodes locale-code-page/UTF-8/CRLF/bare-CR output with universal-newline translation, while POSIX stays byte-decoded. POSIX background commands do not wait for inherited pipes; output drains without unbounded temp files. Stdin gets immediate EOF. `bash_command_timeout` is forwarded to providers with a per-command timeout API (Local/AIO/OpenSandbox/E2B/BoxLite/Tenki); providers without one keep their own defaults. Local: T is a wall-clock process-group deadline. Supported-semver AIO: server-side `hard_timeout`; frozen legacy `all-in-one-sandbox:latest`: command requests get a bounded host `T+5s` wait (`max_retries=0`), avoiding the SDK's 600s sandbox-lock hold; file/list RPCs are not bounded by it. Explicit AIO shell/bash session creation uses a separate 5s/no-retry host bound; an ambiguous create is never replayed and forces container recycle before warm reuse. Never replay ambiguous outcomes: transport timeout, terminated, no_change_timeout, unknown statuses. `hard_timeout` => terminated + Exit Code: 124, keep session; `no_change_timeout` => may still be running, fence session generation. The description scopes host-environment probes to LocalSandbox (`uname -s`, then `sw_vers` on Darwin; Linux files only when policy permits), gives conditional recovery for local path/`file://` rejections, and tells the model to background long-lived processes. See `LocalSandbox.execute_command`, its platform runners, and `bash_tool`'s docstring. - `ls` - Directory listing (tree format, max 2 levels). Remote commands precheck root existence and emit `__DF_FIND_STATUS__:missing`; `find` status 1 is always an incomplete-traversal `OSError`. `AioSandbox.list_dir` parses only `completed`/`None` on #5634's selected generation: `hard_timeout` raises `TimeoutError` and keeps that generation reusable; an ambiguous timeout or status raises `OSError`, replays nothing, and fences that generation. Missing-session 404s drop the stale id without replay. Persistent/scoped shells fence all `httpx.TransportError`s before cleanup. - `glob` - Find files or directories below a root directory with bounded results - `grep` - Search one text file or recursively search a directory, with optional glob filtering and bounded line-level results diff --git a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py index 99f432ef5e..102d15a4a9 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py @@ -887,7 +887,7 @@ class LocalSandbox(Sandbox): timed_out = False stdout_read_fd, stdout_write_fd = os.pipe() stderr_read_fd, stderr_write_fd = os.pipe() - process: subprocess.Popen | None = None + process = None try: process = subprocess.Popen( args, @@ -899,27 +899,30 @@ class LocalSandbox(Sandbox): env=env, ) except BaseException: - # BaseException, not Exception: a KeyboardInterrupt mid-spawn must - # still release the fds; each fd is closed exactly once here, and - # the guarded finally below never closes them a second time. - for fd in (stdout_read_fd, stdout_write_fd, stderr_read_fd, stderr_write_fd): + # Include interruptions during and after Popen; the read ends have + # no drain-thread owner yet. The finally block owns the write ends. + for fd in (stdout_read_fd, stderr_read_fd): try: os.close(fd) except OSError: - # Preserve the original Popen failure; fd cleanup is best-effort. + # Preserve the original startup failure; fd cleanup is best-effort. pass + if process is not None: + # Cleanup must not replace the startup error, even on another + # interruption. Reap independently if killing the child fails. + with suppress(BaseException): + process.kill() + with suppress(BaseException): + process.wait(timeout=10) raise finally: - # Only the successful-spawn path owes the parent's write-end close; - # on failure the except block already released all four fds, and a - # second close could hit a descriptor number another thread claimed - # in between. - if process is not None: - for fd in (stdout_write_fd, stderr_write_fd): - try: - os.close(fd) - except OSError: - pass + # A single owner prevents a second close from hitting a recycled fd. + for fd in (stdout_write_fd, stderr_write_fd): + try: + os.close(fd) + except OSError: + # Closing is best-effort; preserve the command's result or startup error. + pass if encoding is None: encoding = locale.getpreferredencoding(False) @@ -1019,7 +1022,7 @@ class LocalSandbox(Sandbox): timed_out = False stdout_read_fd, stdout_write_fd = os.pipe() stderr_read_fd, stderr_write_fd = os.pipe() - process: subprocess.Popen | None = None + process = None try: process = subprocess.Popen( args, @@ -1034,27 +1037,30 @@ class LocalSandbox(Sandbox): # process-group leader, so its PID is the stable group identifier. process_group_id = process.pid except BaseException: - # BaseException, not Exception: a KeyboardInterrupt mid-spawn must - # still release the fds; each fd is closed exactly once here, and - # the guarded finally below never closes them a second time. - for fd in (stdout_read_fd, stdout_write_fd, stderr_read_fd, stderr_write_fd): + # Include interruptions during and after Popen; the read ends have + # no drain-thread owner yet. The finally block owns the write ends. + for fd in (stdout_read_fd, stderr_read_fd): try: os.close(fd) except OSError: - # Preserve the original Popen failure; fd cleanup is best-effort. + # Preserve the original startup failure; fd cleanup is best-effort. pass + if process is not None: + # Cleanup must not replace the startup error, even on another + # interruption. Reap independently if killing the child fails. + with suppress(BaseException): + process.kill() + with suppress(BaseException): + process.wait(timeout=10) raise finally: - # Only the successful-spawn path owes the parent's write-end close; - # on failure the except block already released all four fds, and a - # second close could hit a descriptor number another thread claimed - # in between. - if process is not None: - for fd in (stdout_write_fd, stderr_write_fd): - try: - os.close(fd) - except OSError: - pass + # A single owner prevents a second close from hitting a recycled fd. + for fd in (stdout_write_fd, stderr_write_fd): + try: + os.close(fd) + except OSError: + # Closing is best-effort; preserve the command's result or startup error. + 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") diff --git a/backend/tests/test_local_sandbox_pipe_fd_lifecycle.py b/backend/tests/test_local_sandbox_pipe_fd_lifecycle.py index 0c03aa40f5..ac36d5f9a7 100644 --- a/backend/tests/test_local_sandbox_pipe_fd_lifecycle.py +++ b/backend/tests/test_local_sandbox_pipe_fd_lifecycle.py @@ -76,8 +76,36 @@ def test_spawn_interrupt_closes_each_pipe_fd_exactly_once(monkeypatch: pytest.Mo assert len(closed) == 4, f"expected exactly one close per pipe end, got {closed}" +@pytest.mark.parametrize("exception_type", [RuntimeError, KeyboardInterrupt]) +def test_post_spawn_failure_closes_each_pipe_fd_exactly_once(monkeypatch: pytest.MonkeyPatch, exception_type: type[BaseException]) -> None: + """An error after Popen returns must not give two handlers ownership of the write ends.""" + closed = _record_os_close(monkeypatch) + pipe_fds: list[int] = [] + real_pipe = os.pipe + error = exception_type("post-spawn failure") + + def recording_pipe() -> tuple[int, int]: + fds = real_pipe() + pipe_fds.extend(fds) + return fds + + class StartedProcess: + @property + def pid(self) -> int: + raise error + + monkeypatch.setattr(os, "pipe", recording_pipe) + with patch.object(subprocess, "Popen", return_value=StartedProcess()): + with pytest.raises(exception_type) as exc_info: + LocalSandbox._run_posix_command(["whatever"], 1.0, None) + + assert exc_info.value is error + assert len(pipe_fds) == 4 + assert sorted(closed) == sorted(pipe_fds), f"expected each pipe fd to close once, got {closed} for {pipe_fds}" + + def test_successful_spawn_closes_write_fds_exactly_once(monkeypatch: pytest.MonkeyPatch) -> None: - """The guard must not skip the parent's write-end close after a real spawn.""" + """Successful startup must still close the parent's write ends.""" if sys.platform == "win32": runner = LocalSandbox._run_windows_command command = ["cmd", "/c", "echo hi"] @@ -102,3 +130,82 @@ def test_successful_spawn_closes_write_fds_exactly_once(monkeypatch: pytest.Monk assert len(write_fds) == 2, "expected the runner to hand both pipe write ends to Popen" for fd in write_fds: assert closed.count(fd) == 1, f"write fd {fd} closed {closed.count(fd)} times: {closed}" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX post-spawn PID access") +@pytest.mark.parametrize("exception_type", [RuntimeError, KeyboardInterrupt]) +def test_post_spawn_failure_kills_and_reaps_real_child(monkeypatch: pytest.MonkeyPatch, exception_type: type[BaseException]) -> None: + """A startup failure must not leave a real child running in its own session.""" + error = exception_type("post-spawn failure") + children: list[subprocess.Popen] = [] + cleanup_calls: list[object] = [] + real_popen = subprocess.Popen + + class InterruptedProcess: + def __init__(self, child: subprocess.Popen) -> None: + self.child = child + + @property + def pid(self) -> int: + raise error + + def kill(self) -> None: + cleanup_calls.append("kill") + self.child.kill() + + def wait(self, timeout: float) -> int: + cleanup_calls.append(("wait", timeout)) + return self.child.wait(timeout=timeout) + + def interrupted_popen(*args, **kwargs): + child = real_popen(*args, **kwargs) + children.append(child) + return InterruptedProcess(child) + + monkeypatch.setattr(subprocess, "Popen", interrupted_popen) + try: + with pytest.raises(exception_type) as exc_info: + LocalSandbox._run_posix_command([sys.executable, "-c", "import time; time.sleep(60)"], 1.0, None) + + assert exc_info.value is error + assert len(children) == 1 + # Do not poll or wait here: those could reap a child that the runner left + # alive or unreaped and accidentally make this assertion pass. + assert children[0].returncode is not None, "startup failure left the child running or unreaped" + assert cleanup_calls == ["kill", ("wait", 10)] + finally: + for child in children: + if child.poll() is None: + child.kill() + child.wait(timeout=10) + + +@pytest.mark.parametrize("cleanup_step", ["kill", "wait"]) +@pytest.mark.parametrize("cleanup_error_type", [RuntimeError, KeyboardInterrupt]) +def test_post_spawn_cleanup_failure_preserves_original_error(monkeypatch: pytest.MonkeyPatch, cleanup_step: str, cleanup_error_type: type[BaseException]) -> None: + original_error = RuntimeError("post-spawn failure") + cleanup_error = cleanup_error_type("cleanup failed") + calls: list[object] = [] + + class InterruptedProcess: + @property + def pid(self) -> int: + raise original_error + + def kill(self) -> None: + calls.append("kill") + if cleanup_step == "kill": + raise cleanup_error + + def wait(self, timeout: float) -> int: + calls.append(("wait", timeout)) + if cleanup_step == "wait": + raise cleanup_error + return -1 + + monkeypatch.setattr(subprocess, "Popen", lambda *args, **kwargs: InterruptedProcess()) + with pytest.raises(RuntimeError) as exc_info: + LocalSandbox._run_posix_command(["whatever"], 1.0, None) + + assert exc_info.value is original_error + assert calls == ["kill", ("wait", 10)]