From b707d9f8223b1d1cdd52e218852b177a1c756c7f Mon Sep 17 00:00:00 2001 From: soynchux Date: Wed, 20 May 2026 11:44:57 +0300 Subject: [PATCH] fix(docker): remove POSIX shell cleanup commands from DockerEnvironment.cleanup --- tests/tools/test_docker_environment.py | 29 ++++++--- tools/environments/docker.py | 87 ++++++++++++++++++++++---- 2 files changed, 96 insertions(+), 20 deletions(-) diff --git a/tests/tools/test_docker_environment.py b/tests/tools/test_docker_environment.py index cd3b7aae6f6f5..915bbe92e43b2 100644 --- a/tests/tools/test_docker_environment.py +++ b/tests/tools/test_docker_environment.py @@ -207,21 +207,34 @@ def test_non_persistent_cleanup_removes_container(monkeypatch): monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") calls = _mock_subprocess_run(monkeypatch) - popen_cmds = [] - monkeypatch.setattr( - docker_env.subprocess, "Popen", - lambda cmd, **kw: (popen_cmds.append(cmd), type("P", (), {"poll": lambda s: 0, "wait": lambda s, **k: None, "returncode": 0, "stdout": iter([]), "stdin": None})())[1], - ) + class ImmediateThread: + def __init__(self, target, daemon=False): + self.target = target + self.daemon = daemon + + def start(self): + self.target() + + monkeypatch.setattr(docker_env, "threading", types.SimpleNamespace(Thread=ImmediateThread)) + monkeypatch.setattr(docker_env, "time", types.SimpleNamespace(sleep=lambda _seconds: None)) env = _make_dummy_env(persistent_filesystem=False, task_id="ephemeral-task") assert env._container_id container_id = env._container_id + env._docker_exe = r"C:\Program Files\Docker\Docker\resources\bin\docker.exe" env.cleanup() - # Should have stop and rm calls via Popen - stop_cmds = [c for c in popen_cmds if container_id in str(c) and "stop" in str(c)] - assert len(stop_cmds) >= 1, f"cleanup() should schedule docker stop for {container_id}" + stop_cmds = [ + cmd for cmd, _kwargs in calls + if cmd == [env._docker_exe, "stop", container_id] + ] + rm_cmds = [ + cmd for cmd, _kwargs in calls + if cmd == [env._docker_exe, "rm", "-f", container_id] + ] + assert len(stop_cmds) == 1, f"cleanup() should stop {container_id} with argv" + assert len(rm_cmds) == 1, f"cleanup() should remove {container_id} with argv" class _FakePopen: diff --git a/tools/environments/docker.py b/tools/environments/docker.py index 1cd72ce855261..d147ea0f08158 100644 --- a/tools/environments/docker.py +++ b/tools/environments/docker.py @@ -11,6 +11,8 @@ import shutil import subprocess import sys +import threading +import time import uuid from typing import Optional @@ -33,6 +35,46 @@ _ENV_VAR_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +def _run_docker_cleanup_cmd( + cmd: list[str], + *, + timeout: int, + action: str, + container_id: str, +) -> bool: + """Run a Docker cleanup command without invoking a shell.""" + try: + result = subprocess.run( + cmd, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + timeout=timeout, + ) + if result.returncode == 0: + return True + logger.debug( + "Docker cleanup %s failed for container %s with exit code %s", + action, + container_id, + result.returncode, + ) + except subprocess.TimeoutExpired: + logger.debug( + "Docker cleanup %s timed out for container %s after %ss", + action, + container_id, + timeout, + ) + except Exception as e: + logger.debug( + "Docker cleanup %s failed for container %s: %s", + action, + container_id, + e, + ) + return False + + def _normalize_forward_env_names(forward_env: list[str] | None) -> list[str]: """Return a deduplicated list of valid environment variable names.""" normalized: list[str] = [] @@ -629,23 +671,44 @@ def _storage_opt_supported() -> bool: def cleanup(self): """Stop and remove the container. Bind-mount dirs persist if persistent=True.""" if self._container_id: - try: - # Stop in background so cleanup doesn't block - stop_cmd = ( - f"(timeout 60 {self._docker_exe} stop {self._container_id} || " - f"{self._docker_exe} rm -f {self._container_id}) >/dev/null 2>&1 &" + container_id = self._container_id + docker_exe = self._docker_exe + remove_after_stop = not self._persistent + + def _stop_container(): + stopped = _run_docker_cleanup_cmd( + [docker_exe, "stop", container_id], + timeout=60, + action="stop", + container_id=container_id, + ) + if not stopped: + _run_docker_cleanup_cmd( + [docker_exe, "rm", "-f", container_id], + timeout=10, + action="force remove", + container_id=container_id, + ) + + def _remove_container_after_delay(): + time.sleep(3) + _run_docker_cleanup_cmd( + [docker_exe, "rm", "-f", container_id], + timeout=10, + action="remove", + container_id=container_id, ) - subprocess.Popen(stop_cmd, shell=True) + + try: + # Stop in background so cleanup doesn't block. + threading.Thread(target=_stop_container, daemon=True).start() except Exception as e: - logger.warning("Failed to stop container %s: %s", self._container_id, e) + logger.warning("Failed to stop container %s: %s", container_id, e) - if not self._persistent: + if remove_after_stop: # Also schedule removal (stop only leaves it as stopped) try: - subprocess.Popen( - f"sleep 3 && {self._docker_exe} rm -f {self._container_id} >/dev/null 2>&1 &", - shell=True, - ) + threading.Thread(target=_remove_container_after_delay, daemon=True).start() except Exception: pass self._container_id = None