From 2dd5a0f92d67ef0bb3859a7c46fc8403d497bab1 Mon Sep 17 00:00:00 2001 From: Ranjan G Date: Wed, 9 Sep 2026 23:54:12 +0530 Subject: [PATCH] fix(sandbox): keep UnixLocal workspace-root removal off the event loop UnixLocalSandboxClient.delete() removed the workspace root with an inline shutil.rmtree call. That removal walks the whole workspace tree, so it held the event loop for its full duration and no other task could advance. Route it through run_blocking_workspace_io, as rm(recursive=True), persist_workspace, and hydrate_workspace already do. The helper forwards positional arguments only, so the call now relies on shutil.rmtree's stdlib default ignore_errors=False, which is how the sibling rm(recursive=True) call site invokes it. One released behavior changes. For a valid UnixLocalSandboxSession whose manifest has no ephemeral mount targets, delete() previously had no suspension point and so returned the session; it can now raise CancelledError, because delivering cancellation requires one. The removal still runs to completion and the caller still waits for it, since the helper keeps the worker owned. A caller bounding the call with asyncio.wait_for now receives TimeoutError after that same wait. Sessions with ephemeral mounts already awaited in the unmount loop and could already raise, and the pre-existing TypeError guard for a foreign session type is unchanged. Add a regression test that pins loop responsiveness across the removal itself via an event handshake, so an await elsewhere in delete() cannot satisfy it. --- src/agents/sandbox/sandboxes/unix_local.py | 2 +- tests/sandbox/test_unix_local.py | 64 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index 28eeb265ef..0c1fbe6858 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -1223,7 +1223,7 @@ async def delete(self, session: SandboxSession) -> SandboxSession: if unmount_failed: return session try: - shutil.rmtree(Path(inner.state.manifest.root), ignore_errors=False) + await run_blocking_workspace_io(shutil.rmtree, Path(inner.state.manifest.root)) except FileNotFoundError: pass except Exception: diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index 9188b1fc33..ba7d7d6077 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -1,7 +1,9 @@ from __future__ import annotations import asyncio +import contextlib import io +import shutil import signal import tarfile import threading @@ -610,3 +612,65 @@ def _slow_extract(tar: object, **kwargs: object) -> None: # the workspace root are only released once nothing is still writing to them. assert events == ["extract-start", "extract-end"] assert not buf.closed + + +@pytest.mark.asyncio +async def test_client_delete_keeps_workspace_removal_off_the_event_loop( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The event loop must keep running while `delete()` removes the workspace root. + + The removal walks the whole workspace tree, so running it inline starves every other + task on the loop for its full duration. `rm(recursive=True)`, `persist_workspace`, and + `hydrate_workspace` already hand that work to `run_blocking_workspace_io`. + + The handshake below measures the removal itself rather than the whole `delete()` call, + so an `await` elsewhere in the method, such as the ephemeral unmount loop, cannot + satisfy it. + """ + workspace = tmp_path / "workspace" + workspace.mkdir() + (workspace / "payload.txt").write_text("payload", encoding="utf-8") + + client = UnixLocalSandboxClient() + session = await client.resume( + UnixLocalSandboxSessionState( + manifest=Manifest(root=str(workspace)), + snapshot=NoopSnapshot(id="noop"), + workspace_root_owned=True, + ) + ) + + real_rmtree = shutil.rmtree + removal_started = threading.Event() + loop_advanced = threading.Event() + loop_advanced_during_removal: list[bool] = [] + + def _slow_rmtree(path: object, *args: object, **kwargs: object) -> None: + removal_started.set() + # The observer can only answer while the removal is in flight if the loop is + # still free. An inline removal holds the loop here until this call returns. + loop_advanced_during_removal.append(loop_advanced.wait(timeout=5.0)) + real_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(unix_local_module.shutil, "rmtree", _slow_rmtree) + + async def _observe_loop() -> None: + while not removal_started.is_set(): + await asyncio.sleep(0) + loop_advanced.set() + + observer = asyncio.create_task(_observe_loop()) + try: + returned = await client.delete(session) + finally: + observer.cancel() + with contextlib.suppress(asyncio.CancelledError): + await observer + + assert removal_started.is_set() + assert loop_advanced_during_removal == [True] + # The removal still targets the manifest root, and `delete()` still hands the same + # session back to the caller. + assert not workspace.exists() + assert returned is session