mirror of
https://github.com/usestrix/strix.git
synced 2026-10-05 02:41:38 +00:00
revert(runtime): drop the PTY teardown change in StrixDockerSandboxClient.delete()
Keep this PR to the logging change only: the finalizer reports are routed to strix.log by the unraisable hook, so the sandbox teardown stays as on main.
This commit is contained in:
parent
fa5f99c4e0
commit
8ea2c99225
2 changed files with 10 additions and 132 deletions
|
|
@ -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 —
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue