diff --git a/strix/runtime/docker_client.py b/strix/runtime/docker_client.py index 969267d5..979fdcb4 100644 --- a/strix/runtime/docker_client.py +++ b/strix/runtime/docker_client.py @@ -276,7 +276,17 @@ class StrixDockerSandboxClient(DockerSandboxClient): return session async def delete(self, session: SandboxSession) -> SandboxSession: - container_id = getattr(getattr(session._inner, "state", None), "container_id", None) + 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) if container_id: # Best-effort kill: NotFound/APIError cover a gone or unhappy # container. RequestException covers a torn-down daemon socket — diff --git a/strix/telemetry/logging.py b/strix/telemetry/logging.py index e3f8e592..3fcd4e5b 100644 --- a/strix/telemetry/logging.py +++ b/strix/telemetry/logging.py @@ -5,6 +5,7 @@ from __future__ import annotations import contextlib import logging import os +import re import sys import warnings from contextvars import ContextVar @@ -94,12 +95,25 @@ def configure_dependency_logging() -> None: _unraisable_hook_installed = False +_FINALIZER_NOISE_MODULES = frozenset({"urllib3", "http"}) +_FINALIZER_FILE_RE = re.compile(r"finalizing file <(urllib3|http\.client)\.") + + def _is_urllib3_closed_file_noise(unraisable: sys.UnraisableHookArgs) -> bool: - return ( + """An HTTP response (urllib3 or http.client) whose socket was closed first. + + Python 3.14 reports IOBase finalizer failures with ``object=None`` and the + object's repr inside ``err_msg`` ("Exception ignored while finalizing file + <...>"); earlier versions pass the object itself. + """ + if not ( isinstance(unraisable.exc_value, ValueError) and "I/O operation on closed file" in str(unraisable.exc_value) - and type(unraisable.object).__module__.split(".")[0] == "urllib3" - ) + ): + return False + if unraisable.object is not None: + return type(unraisable.object).__module__.split(".")[0] in _FINALIZER_NOISE_MODULES + return _FINALIZER_FILE_RE.search(unraisable.err_msg or "") is not None def _silence_urllib3_finalizer_noise() -> None: @@ -111,6 +125,12 @@ def _silence_urllib3_finalizer_noise() -> None: def hook(unraisable: sys.UnraisableHookArgs) -> None: if _is_urllib3_closed_file_noise(unraisable): + with contextlib.suppress(Exception): # the interpreter may be finalizing + logging.getLogger("strix.telemetry").debug( + "Ignored HTTP response finalizer error: %s: %s", + unraisable.err_msg or type(unraisable.object).__name__, + unraisable.exc_value, + ) return previous(unraisable) diff --git a/tests/test_docker_client_delete.py b/tests/test_docker_client_delete.py index ef93723c..241e86f7 100644 --- a/tests/test_docker_client_delete.py +++ b/tests/test_docker_client_delete.py @@ -36,9 +36,17 @@ def _client_with_kill_error(exc: Exception) -> StrixDockerSandboxClient: return client -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))) +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(), + ) + ) return cast("SandboxSession", fake) @@ -90,3 +98,39 @@ 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() diff --git a/tests/test_unraisable_filter.py b/tests/test_unraisable_filter.py index 83d267ba..33c305d0 100644 --- a/tests/test_unraisable_filter.py +++ b/tests/test_unraisable_filter.py @@ -1,3 +1,5 @@ +import http.client +import socket import sys import pytest @@ -8,11 +10,13 @@ from strix.telemetry.logging import _is_urllib3_closed_file_noise class _Args: - def __init__(self, exc_value: BaseException | None, obj: object) -> None: + def __init__( + self, exc_value: BaseException | None, obj: object, err_msg: str | None = None + ) -> None: self.exc_type = type(exc_value) if exc_value is not None else None self.exc_value = exc_value self.exc_traceback = None - self.err_msg = None + self.err_msg = err_msg self.object = obj @@ -25,6 +29,45 @@ def test_filters_urllib3_closed_file_noise() -> None: assert _is_urllib3_closed_file_noise(args) # type: ignore[arg-type] +def test_filters_http_client_closed_file_noise() -> None: + sock = socket.socket() + try: + response = http.client.HTTPResponse(sock) + finally: + sock.close() + args = _Args(ValueError("I/O operation on closed file."), response) + assert _is_urllib3_closed_file_noise(args) # type: ignore[arg-type] + + +@pytest.mark.parametrize( + "repr_text", + [ + "", + "", + ], +) +def test_filters_python314_finalizer_shape(repr_text: str) -> None: + """3.14 reports IOBase finalizer failures with object=None and the repr in err_msg.""" + args = _Args( + ValueError("I/O operation on closed file."), + None, + err_msg=f"Exception ignored while finalizing file {repr_text}", + ) + assert _is_urllib3_closed_file_noise(args) # type: ignore[arg-type] + + +def test_python314_shape_passes_through_other_files() -> None: + args = _Args( + ValueError("I/O operation on closed file."), + None, + err_msg="Exception ignored while finalizing file <_io.TextIOWrapper name='x' mode='w'>", + ) + assert not _is_urllib3_closed_file_noise(args) # type: ignore[arg-type] + assert not _is_urllib3_closed_file_noise( + _Args(ValueError("I/O operation on closed file."), None) # type: ignore[arg-type] + ) + + def test_passes_through_other_unraisables() -> None: assert not _is_urllib3_closed_file_noise( _Args(ValueError("I/O operation on closed file."), object()) # type: ignore[arg-type] @@ -46,6 +89,16 @@ def test_installed_hook_filters_and_delegates(monkeypatch: pytest.MonkeyPatch) - assert hook is not calls.append hook(_Args(ValueError("I/O operation on closed file."), _urllib3_response())) # type: ignore[arg-type] + hook( + _Args( # type: ignore[arg-type] + ValueError("I/O operation on closed file."), + None, + err_msg=( + "Exception ignored while finalizing file " + "" + ), + ) + ) assert calls == [] other = _Args(RuntimeError("boom"), object())