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

@@ -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

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

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")

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)]