diff --git a/strix/runtime/docker_client.py b/strix/runtime/docker_client.py index 979fdcb4..969267d5 100644 --- a/strix/runtime/docker_client.py +++ b/strix/runtime/docker_client.py @@ -276,17 +276,7 @@ class StrixDockerSandboxClient(DockerSandboxClient): return session async def delete(self, session: SandboxSession) -> SandboxSession: - inner = session._inner - # The SDK's delete() only runs inner.shutdown() (which terminates the - # agent's PTY exec streams) when the container still exists. Terminate - # them unconditionally first: otherwise the hijacked exec sockets are - # left to the garbage collector and their HTTP responses fail to close - # at interpreter exit ("Exception ignored while finalizing file"). - pty_terminate_all = getattr(inner, "pty_terminate_all", None) - if pty_terminate_all is not None: - with contextlib.suppress(Exception): - await pty_terminate_all() - container_id = getattr(getattr(inner, "state", None), "container_id", None) + container_id = getattr(getattr(session._inner, "state", None), "container_id", None) if container_id: # Best-effort kill: NotFound/APIError cover a gone or unhappy # container. RequestException covers a torn-down daemon socket — diff --git a/tests/test_docker_client_delete.py b/tests/test_docker_client_delete.py index e950fc40..ef93723c 100644 --- a/tests/test_docker_client_delete.py +++ b/tests/test_docker_client_delete.py @@ -11,28 +11,22 @@ would let it escape and surface a traceback on every teardown. from __future__ import annotations -import uuid -from pathlib import Path from types import SimpleNamespace -from typing import cast +from typing import TYPE_CHECKING, cast from unittest.mock import AsyncMock, MagicMock, patch import pytest -from agents.sandbox.manifest import Manifest -from agents.sandbox.sandboxes.docker import ( - DockerSandboxClient, - DockerSandboxSession, - DockerSandboxSessionState, - _DockerExecSocket, - _DockerPtyProcessEntry, -) -from agents.sandbox.session.sandbox_session import SandboxSession +from agents.sandbox.sandboxes.docker import DockerSandboxClient from docker import errors as docker_errors from requests.exceptions import ConnectionError as RequestsConnectionError from strix.runtime.docker_client import StrixDockerSandboxClient +if TYPE_CHECKING: + from agents.sandbox.session.sandbox_session import SandboxSession + + def _client_with_kill_error(exc: Exception) -> StrixDockerSandboxClient: """A StrixDockerSandboxClient whose containers.get(...).kill() raises ``exc``.""" client = StrixDockerSandboxClient.__new__(StrixDockerSandboxClient) @@ -42,17 +36,9 @@ def _client_with_kill_error(exc: Exception) -> StrixDockerSandboxClient: return client -def _session( - container_id: str | None = "abc123", pty_terminate_all: AsyncMock | None = None -) -> SandboxSession: - # delete() reads the inner state's container_id and awaits the inner - # session's PTY teardown. - fake = SimpleNamespace( - _inner=SimpleNamespace( - state=SimpleNamespace(container_id=container_id), - pty_terminate_all=pty_terminate_all or AsyncMock(), - ) - ) +def _session(container_id: str | None = "abc123") -> SandboxSession: + # delete() reads session._inner.state.container_id + fake = SimpleNamespace(_inner=SimpleNamespace(state=SimpleNamespace(container_id=container_id))) return cast("SandboxSession", fake) @@ -104,101 +90,3 @@ async def test_delete_noop_without_container_id() -> None: client.docker_client.containers.get.assert_not_called() super_delete.assert_awaited_once() - - -@pytest.mark.asyncio -async def test_delete_terminates_pty_streams_even_when_the_container_is_gone() -> None: - """The SDK's delete() skips shutdown() (and with it PTY teardown) when the - container no longer exists, which leaves the agent's exec sockets to the - garbage collector. delete() must terminate them itself, before anything - else, whatever the container's state.""" - client = _client_with_kill_error(docker_errors.NotFound("gone")) - order: list[str] = [] - session = _session( - pty_terminate_all=AsyncMock(side_effect=lambda: order.append("pty_terminate_all")) - ) - - async def _super_delete(_self: object, _session: object) -> SandboxSession: - order.append("super.delete") - return session - - with patch.object(DockerSandboxClient, "delete", new=_super_delete): - await client.delete(session) - - assert order == ["pty_terminate_all", "super.delete"] - - -@pytest.mark.asyncio -async def test_delete_survives_pty_termination_errors() -> None: - client = StrixDockerSandboxClient.__new__(StrixDockerSandboxClient) - client.docker_client = MagicMock() - session = _session(pty_terminate_all=AsyncMock(side_effect=RuntimeError("daemon gone"))) - - with patch.object( - DockerSandboxClient, "delete", new=AsyncMock(return_value=session) - ) as super_delete: - await client.delete(session) - - super_delete.assert_awaited_once() - - -def _real_session_with_open_pty( - container_id: str = "abc123", -) -> tuple[SandboxSession, _DockerExecSocket]: - """A real SDK DockerSandboxSession holding one live PTY exec stream, the - shape exec_command leaves behind: a hijacked socket plus the streamed HTTP - response docker-py pins to it.""" - state = DockerSandboxSessionState.model_construct( - type="docker", - image="sandbox:test", - container_id=container_id, - manifest=Manifest(), - session_id=uuid.uuid4(), - exposed_ports=(), - workspace_root_ready=True, - ) - container = MagicMock() - container.client.api.exec_inspect.return_value = {"Running": False, "ExitCode": 0} - inner = DockerSandboxSession(docker_client=MagicMock(), container=container, state=state) - exec_socket = _DockerExecSocket(sock=MagicMock(), raw_sock=MagicMock(), response=MagicMock()) - inner._pty_processes[1] = _DockerPtyProcessEntry( - exec_id="exec-1", - sock=exec_socket, - raw_sock=exec_socket.raw_sock, - pid_path=Path("/workspace/.pty/1.pid"), - tty=True, - ) - return SandboxSession(inner), exec_socket - - -@pytest.mark.asyncio -async def test_sdk_delete_alone_leaves_pty_exec_streams_open_when_the_container_is_gone() -> None: - """Documents the SDK gap delete() compensates for: with the container gone, - DockerSandboxClient.delete() never terminates the PTY entries.""" - client = StrixDockerSandboxClient.__new__(StrixDockerSandboxClient) - client.docker_client = MagicMock() - client.docker_client.containers.get.side_effect = docker_errors.NotFound("gone") - session, exec_socket = _real_session_with_open_pty() - - await DockerSandboxClient.delete(client, session) - - assert session._inner._pty_processes # type: ignore[attr-defined] - cast("MagicMock", exec_socket.sock).close.assert_not_called() - cast("MagicMock", exec_socket.response).close.assert_not_called() - - -@pytest.mark.asyncio -async def test_delete_closes_real_pty_exec_streams_when_the_container_is_gone() -> None: - """End to end through the real SDK session and the real SDK delete(): the - exec socket and its pinned HTTP response are closed, so nothing is left for - the garbage collector at interpreter exit.""" - client = StrixDockerSandboxClient.__new__(StrixDockerSandboxClient) - client.docker_client = MagicMock() - client.docker_client.containers.get.side_effect = docker_errors.NotFound("gone") - session, exec_socket = _real_session_with_open_pty() - - await client.delete(session) - - assert not session._inner._pty_processes # type: ignore[attr-defined] - cast("MagicMock", exec_socket.sock).close.assert_called_once() - cast("MagicMock", exec_socket.response).close.assert_called_once()