From 0fb005c73f80ff4c35ca905ed1015e54c41d20bd Mon Sep 17 00:00:00 2001 From: seanturner83 Date: Fri, 10 Jul 2026 05:13:35 +0100 Subject: [PATCH] fix(runtime): swallow torn-down docker socket in sandbox delete() (#721) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit StrixDockerSandboxClient.delete() best-effort-kills the sandbox container via containers.get(id).kill() before delegating to the SDK's delete(), suppressing docker NotFound/APIError. But when the docker daemon socket is already going away — the normal case on a host/CI teardown — containers.get() -> inspect_container raises requests' ConnectionError, which is a *sibling* of docker.errors.APIError under requests.RequestException, not a subclass. So it escapes the APIError-only suppress and surfaces a full traceback on teardown even though the kill is meant to be best-effort. Add RequestException to the suppress so the best-effort kill is genuinely best-effort regardless of daemon reachability. Test: tests/test_docker_client_delete.py — the kill raising ConnectionError (and NotFound/APIError) is swallowed and delete() still delegates; unrelated errors still propagate; no-container_id is a no-op. The ConnectionError case fails against the pre-fix APIError-only suppress. Co-authored-by: Claude Opus 4.8 --- strix/runtime/docker_client.py | 12 ++++- tests/test_docker_client_delete.py | 86 ++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 tests/test_docker_client_delete.py diff --git a/strix/runtime/docker_client.py b/strix/runtime/docker_client.py index 497ae2f..13ac05b 100644 --- a/strix/runtime/docker_client.py +++ b/strix/runtime/docker_client.py @@ -40,6 +40,7 @@ from docker import errors as docker_errors # type: ignore[import-untyped, unuse from docker.models.containers import Container # type: ignore[import-untyped, unused-ignore] from docker.types import Mount as DockerSDKMount # type: ignore[import-untyped, unused-ignore] from docker.utils import parse_repository_tag # type: ignore[import-untyped, unused-ignore] +from requests.exceptions import RequestException logger = logging.getLogger(__name__) @@ -148,6 +149,15 @@ class StrixDockerSandboxClient(DockerSandboxClient): async def delete(self, session: SandboxSession) -> SandboxSession: container_id = getattr(getattr(session._inner, "state", None), "container_id", None) if container_id: - with contextlib.suppress(docker_errors.NotFound, docker_errors.APIError): + # Best-effort kill: NotFound/APIError cover a gone or unhappy + # container. RequestException covers a torn-down daemon socket — + # containers.get() -> inspect_container raises requests' + # ConnectionError, which is a sibling of docker.errors.APIError + # under requests.RequestException (not a subclass), so it escapes + # an APIError-only suppress and surfaces a full traceback even + # though this teardown is meant to be best-effort. + with contextlib.suppress( + docker_errors.NotFound, docker_errors.APIError, RequestException + ): self.docker_client.containers.get(container_id).kill() return await super().delete(session) diff --git a/tests/test_docker_client_delete.py b/tests/test_docker_client_delete.py new file mode 100644 index 0000000..1080cd0 --- /dev/null +++ b/tests/test_docker_client_delete.py @@ -0,0 +1,86 @@ +"""StrixDockerSandboxClient.delete() best-effort teardown. + +delete() kills the sandbox container before delegating to the SDK's delete(). +The kill is meant to be best-effort, but the ``contextlib.suppress`` around it +must cover the case where the docker daemon socket is already gone: then +``containers.get()`` -> ``inspect_container`` raises requests' +``ConnectionError``, which is a *sibling* of ``docker.errors.APIError`` under +``requests.RequestException`` (not a subclass), so an APIError-only suppress +would let it escape and surface a traceback on every teardown. +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +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 + + +def _client_with_kill_error(exc: Exception) -> StrixDockerSandboxClient: + """A StrixDockerSandboxClient whose containers.get(...).kill() raises ``exc``.""" + client = StrixDockerSandboxClient.__new__(StrixDockerSandboxClient) + docker_client = MagicMock() + docker_client.containers.get.side_effect = exc + client.docker_client = docker_client + return client + + +def _session() -> object: + # delete() reads session._inner.state.container_id + return SimpleNamespace(_inner=SimpleNamespace(state=SimpleNamespace(container_id="abc123"))) + + +@pytest.mark.parametrize( + "exc", + [ + RequestsConnectionError("Connection aborted", FileNotFoundError(2, "No such file")), + docker_errors.NotFound("gone"), + docker_errors.APIError("unhappy"), + ], +) +@pytest.mark.asyncio +async def test_delete_swallows_best_effort_kill_errors(exc): + """A torn-down socket (ConnectionError) or a gone/unhappy container + (NotFound/APIError) during the kill must not propagate; delete() still + delegates to the SDK's delete().""" + client = _client_with_kill_error(exc) + session = _session() + + with patch.object( + DockerSandboxClient, "delete", new=AsyncMock(return_value=session) + ) as super_delete: + result = await client.delete(session) + + assert result is session + super_delete.assert_awaited_once() # teardown proceeded despite the kill error + + +@pytest.mark.asyncio +async def test_delete_does_not_swallow_unrelated_errors(): + """A programming error (e.g. ValueError) is not part of best-effort kill and + must still propagate.""" + client = _client_with_kill_error(ValueError("boom")) + with pytest.raises(ValueError): + await client.delete(_session()) + + +@pytest.mark.asyncio +async def test_delete_noop_without_container_id(): + """No container_id -> no kill attempt, just delegate.""" + client = StrixDockerSandboxClient.__new__(StrixDockerSandboxClient) + client.docker_client = MagicMock() + session = SimpleNamespace(_inner=SimpleNamespace(state=SimpleNamespace(container_id=None))) + + with patch.object( + DockerSandboxClient, "delete", new=AsyncMock(return_value=session) + ) as super_delete: + await client.delete(session) + + client.docker_client.containers.get.assert_not_called() + super_delete.assert_awaited_once()