From d5e3f930c068cbae76dc0d4b35e528f6bf3539cd Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 14 Oct 2026 19:58:40 +0000 Subject: [PATCH 1/2] fix(docker-compose): Download v5.6.0 from GitHub with a stall timeout Install docker compose v5.6.0 (latest) from docker/compose GitHub releases instead of the GCS mirror, which was pinned to v2.29.7. urlretrieve has no timeout, so a stalled download hung until the caller's timeout (10m in sentry CI). Downloads now use urlopen with a 15s connect/read timeout and 3 attempts with backoff, so a stalled download fails in ~48s worst case. The dependency zipball download gets the same timeout. Co-Authored-By: Joshua Li <14209781+joshuarli@users.noreply.github.com> --- devservices/constants.py | 12 +++--- devservices/utils/dependencies.py | 3 +- devservices/utils/install_binary.py | 21 ++++++--- testing/utils.py | 4 +- tests/utils/test_docker_compose.py | 34 +++++++-------- tests/utils/test_install_binary.py | 66 ++++++++++++++++++++++++----- 6 files changed, 98 insertions(+), 42 deletions(-) diff --git a/devservices/constants.py b/devservices/constants.py index 5832a49a..f08279a5 100644 --- a/devservices/constants.py +++ b/devservices/constants.py @@ -22,7 +22,7 @@ class DependencyType(StrEnum): SUPERVISOR = "supervisor" -MINIMUM_DOCKER_COMPOSE_VERSION = "2.29.7" +MINIMUM_DOCKER_COMPOSE_VERSION = "5.6.0" DEVSERVICES_DIR_NAME = "devservices" CONFIG_FILE_NAME = "config.yml" DOCKER_CONFIG_DIR = os.environ.get("DOCKER_CONFIG", os.path.expanduser("~/.docker")) @@ -42,15 +42,13 @@ class DependencyType(StrEnum): "https://api.github.com/repos/getsentry/devservices/releases/latest" ) -# We mirror this in our GCP bucket since GitHub downloads can be flaky at times. -# gsutil cp docker-compose-darwin-aarch64 gs://sentry-dev-infra-assets/docker-compose/v2.29.7/docker-compose-darwin-aarch64 -# gsutil cp docker-compose-linux-x86_64 gs://sentry-dev-infra-assets/docker-compose/v2.29.7/docker-compose-linux-x86_64 -DOCKER_COMPOSE_DOWNLOAD_URL = ( - "https://storage.googleapis.com/sentry-dev-infra-assets/docker-compose" -) +DOCKER_COMPOSE_DOWNLOAD_URL = "https://github.com/docker/compose/releases/download" DEVSERVICES_DOWNLOAD_URL = "https://github.com/getsentry/devservices/releases/download" BINARY_PERMISSIONS = 0o755 +# Seconds a download may stall (connecting or between reads) before the attempt +# is abandoned and retried. +DOWNLOAD_TIMEOUT = 15 MAX_LOG_LINES = "100" LOGGER_NAME = "devservices" DOCKER_NETWORK_NAME = "devservices" diff --git a/devservices/utils/dependencies.py b/devservices/utils/dependencies.py index 2ffe39f9..42c6cd53 100644 --- a/devservices/utils/dependencies.py +++ b/devservices/utils/dependencies.py @@ -23,6 +23,7 @@ from devservices.constants import DEPENDENCY_CONFIG_VERSION from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR from devservices.constants import DEVSERVICES_DIR_NAME +from devservices.constants import DOWNLOAD_TIMEOUT from devservices.constants import DependencyType from devservices.exceptions import ConfigNotFoundError from devservices.exceptions import ConfigParseError @@ -441,7 +442,7 @@ def _fetch_dependency( def _download() -> bytes: req = github.build_api_request(zip_url) - with urllib.request.urlopen(req) as response: + with urllib.request.urlopen(req, timeout=DOWNLOAD_TIMEOUT) as response: return bytes(response.read()) try: diff --git a/devservices/utils/install_binary.py b/devservices/utils/install_binary.py index 9049b53f..9236f147 100644 --- a/devservices/utils/install_binary.py +++ b/devservices/utils/install_binary.py @@ -3,13 +3,23 @@ import os import shutil import tempfile -from urllib.request import urlretrieve +from urllib.request import urlopen from devservices.constants import BINARY_PERMISSIONS +from devservices.constants import DOWNLOAD_TIMEOUT from devservices.exceptions import BinaryInstallError from devservices.utils.console import Console from devservices.utils.retry import retry +DOWNLOAD_ATTEMPTS = 3 + + +def download_file(url: str, path: str) -> None: + # urlretrieve has no timeout, so a stalled connection would hang forever. + # The timeout applies to connecting and to each read, not the whole download. + with urlopen(url, timeout=DOWNLOAD_TIMEOUT) as response, open(path, "wb") as f: + shutil.copyfileobj(response, f) + def install_binary( binary_name: str, @@ -24,16 +34,17 @@ def install_binary( console.info(f"Downloading {binary_name} {version} from {url}...") try: retry( - lambda: urlretrieve(url, temp_file), - retries=3, + lambda: download_file(url, temp_file), + retries=DOWNLOAD_ATTEMPTS, delay=1.0, + backoff=2.0, on_retry=lambda e, remaining: console.warning( - f"Download failed. Retrying in 1 seconds... ({remaining} retries left)" + f"Download failed ({e}). Retrying... ({remaining} retries left)" ), ) except Exception as e: raise BinaryInstallError( - f"Failed to download {binary_name} after 3 attempts: {e}" + f"Failed to download {binary_name} after {DOWNLOAD_ATTEMPTS} attempts: {e}" ) from e try: diff --git a/testing/utils.py b/testing/utils.py index efbc52fb..ebb5e0e1 100644 --- a/testing/utils.py +++ b/testing/utils.py @@ -69,10 +69,10 @@ def make_urlopen_response(zip_bytes: bytes) -> mock.MagicMock: def url_dispatch( zip_bytes_by_repo_name: dict[str, bytes], -) -> Callable[[mock.MagicMock], mock.MagicMock]: +) -> Callable[..., mock.MagicMock]: """Return a urlopen side_effect that routes requests by repo name in the URL.""" - def _side_effect(request: mock.MagicMock) -> mock.MagicMock: + def _side_effect(request: mock.MagicMock, timeout: float) -> mock.MagicMock: url = request.full_url for repo_name, zip_bytes in zip_bytes_by_repo_name.items(): if f"/{repo_name}/" in url: diff --git a/tests/utils/test_docker_compose.py b/tests/utils/test_docker_compose.py index e80e827c..7b4eaede 100644 --- a/tests/utils/test_docker_compose.py +++ b/tests/utils/test_docker_compose.py @@ -186,7 +186,7 @@ def test_install_docker_compose_binary_install_error( @mock.patch("platform.system", return_value="Darwin") @mock.patch("platform.machine", return_value="arm64") -@mock.patch("devservices.utils.install_binary.urlretrieve") +@mock.patch("devservices.utils.install_binary.download_file") @mock.patch("devservices.utils.install_binary.os.chmod") @mock.patch("devservices.utils.install_binary.shutil.move") @mock.patch( @@ -202,7 +202,7 @@ def test_install_docker_compose_compose_verification_error( _mock_get_docker_compose_version: mock.Mock, _mock_shutil_move: mock.Mock, _mock_chmod: mock.Mock, - _mock_urlretrieve: mock.Mock, + _mock_download_file: mock.Mock, _mock_machine: mock.Mock, _mock_system: mock.Mock, ) -> None: @@ -216,26 +216,26 @@ def test_install_docker_compose_compose_verification_error( @mock.patch("devservices.utils.install_binary.tempfile.TemporaryDirectory") @mock.patch("platform.system", return_value="Darwin") @mock.patch("platform.machine", return_value="arm64") -@mock.patch("devservices.utils.install_binary.urlretrieve") +@mock.patch("devservices.utils.install_binary.download_file") @mock.patch("devservices.utils.install_binary.os.chmod") @mock.patch("devservices.utils.install_binary.shutil.move") @mock.patch( "devservices.utils.docker_compose.get_docker_compose_version", - return_value="2.29.7\n", + return_value="5.6.0\n", ) def test_install_docker_compose_macos_arm64( mock_get_docker_compose_version: mock.Mock, mock_shutil_move: mock.Mock, mock_chmod: mock.Mock, - mock_urlretrieve: mock.Mock, + mock_download_file: mock.Mock, _mock_machine: mock.Mock, _mock_system: mock.Mock, mock_tempdir: mock.Mock, ) -> None: mock_tempdir.return_value.__enter__.return_value = "tempdir" install_docker_compose() - mock_urlretrieve.assert_called_once_with( - "https://storage.googleapis.com/sentry-dev-infra-assets/docker-compose/v2.29.7/docker-compose-darwin-aarch64", + mock_download_file.assert_called_once_with( + "https://github.com/docker/compose/releases/download/v5.6.0/docker-compose-darwin-aarch64", "tempdir/docker-compose", ) mock_chmod.assert_called_once_with("tempdir/docker-compose", 0o755) @@ -249,26 +249,26 @@ def test_install_docker_compose_macos_arm64( @mock.patch("devservices.utils.install_binary.tempfile.TemporaryDirectory") @mock.patch("platform.system", return_value="Linux") @mock.patch("platform.machine", return_value="x86_64") -@mock.patch("devservices.utils.install_binary.urlretrieve") +@mock.patch("devservices.utils.install_binary.download_file") @mock.patch("devservices.utils.install_binary.os.chmod") @mock.patch("devservices.utils.install_binary.shutil.move") @mock.patch( "devservices.utils.docker_compose.get_docker_compose_version", - return_value="2.29.7\n", + return_value="5.6.0\n", ) def test_install_docker_compose_linux_x86( mock_get_docker_compose_version: mock.Mock, mock_shutil_move: mock.Mock, mock_chmod: mock.Mock, - mock_urlretrieve: mock.Mock, + mock_download_file: mock.Mock, _mock_machine: mock.Mock, _mock_system: mock.Mock, mock_tempdir: mock.Mock, ) -> None: mock_tempdir.return_value.__enter__.return_value = "tempdir" install_docker_compose() - mock_urlretrieve.assert_called_once_with( - "https://storage.googleapis.com/sentry-dev-infra-assets/docker-compose/v2.29.7/docker-compose-linux-x86_64", + mock_download_file.assert_called_once_with( + "https://github.com/docker/compose/releases/download/v5.6.0/docker-compose-linux-x86_64", "tempdir/docker-compose", ) mock_chmod.assert_called_once_with("tempdir/docker-compose", 0o755) @@ -282,18 +282,18 @@ def test_install_docker_compose_linux_x86( @mock.patch("devservices.utils.install_binary.tempfile.TemporaryDirectory") @mock.patch("platform.system", return_value="Darwin") @mock.patch("platform.machine", return_value="arm64") -@mock.patch("devservices.utils.install_binary.urlretrieve") +@mock.patch("devservices.utils.install_binary.download_file") @mock.patch("devservices.utils.install_binary.os.chmod") @mock.patch("devservices.utils.install_binary.shutil.move") @mock.patch( "devservices.utils.docker_compose.get_docker_compose_version", - return_value="2.29.7\n", + return_value="5.6.0\n", ) def test_install_docker_compose_custom_docker_config_dir( mock_get_docker_compose_version: mock.Mock, mock_shutil_move: mock.Mock, mock_chmod: mock.Mock, - mock_urlretrieve: mock.Mock, + mock_download_file: mock.Mock, _mock_machine: mock.Mock, _mock_system: mock.Mock, mock_tempdir: mock.Mock, @@ -304,8 +304,8 @@ def test_install_docker_compose_custom_docker_config_dir( "tempdir/docker/config/cli-plugins", ): install_docker_compose() - mock_urlretrieve.assert_called_once_with( - "https://storage.googleapis.com/sentry-dev-infra-assets/docker-compose/v2.29.7/docker-compose-darwin-aarch64", + mock_download_file.assert_called_once_with( + "https://github.com/docker/compose/releases/download/v5.6.0/docker-compose-darwin-aarch64", "tempdir/docker-compose", ) mock_chmod.assert_called_once_with("tempdir/docker-compose", 0o755) diff --git a/tests/utils/test_install_binary.py b/tests/utils/test_install_binary.py index 83ae884c..c7070604 100644 --- a/tests/utils/test_install_binary.py +++ b/tests/utils/test_install_binary.py @@ -1,19 +1,23 @@ from __future__ import annotations +import io +import pathlib from unittest import mock import pytest +from devservices.constants import DOWNLOAD_TIMEOUT from devservices.exceptions import BinaryInstallError +from devservices.utils.install_binary import download_file from devservices.utils.install_binary import install_binary @mock.patch( - "devservices.utils.install_binary.urlretrieve", + "devservices.utils.install_binary.download_file", side_effect=Exception("Connection error"), ) def test_install_docker_compose_connection_error( - mock_urlretrieve: mock.Mock, capsys: pytest.CaptureFixture[str] + mock_download_file: mock.Mock, capsys: pytest.CaptureFixture[str] ) -> None: with pytest.raises( BinaryInstallError, @@ -33,9 +37,9 @@ def test_install_docker_compose_connection_error( assert "Download failed. Retrying in 1 seconds... (Attempt 2/2)" in captured.out -@mock.patch("devservices.utils.install_binary.urlretrieve") +@mock.patch("devservices.utils.install_binary.download_file") def test_install_docker_compose_chmod_file_not_found_error( - mock_urlretrieve: mock.Mock, + mock_download_file: mock.Mock, ) -> None: with pytest.raises( BinaryInstallError, @@ -49,11 +53,11 @@ def test_install_docker_compose_chmod_file_not_found_error( ) -@mock.patch("devservices.utils.install_binary.urlretrieve") +@mock.patch("devservices.utils.install_binary.download_file") @mock.patch("devservices.utils.install_binary.os.chmod") def test_install_docker_compose_shutil_file_not_found_error( mock_chmod: mock.Mock, - mock_urlretrieve: mock.Mock, + mock_download_file: mock.Mock, ) -> None: with pytest.raises( BinaryInstallError, @@ -67,14 +71,14 @@ def test_install_docker_compose_shutil_file_not_found_error( ) -@mock.patch("devservices.utils.install_binary.urlretrieve") +@mock.patch("devservices.utils.install_binary.download_file") @mock.patch( "devservices.utils.install_binary.os.chmod", side_effect=PermissionError("Insufficient Permissions"), ) def test_install_docker_compose_chmod_permission_error( mock_chmod: mock.Mock, - mock_urlretrieve: mock.Mock, + mock_download_file: mock.Mock, ) -> None: with pytest.raises( BinaryInstallError, @@ -88,7 +92,7 @@ def test_install_docker_compose_chmod_permission_error( ) -@mock.patch("devservices.utils.install_binary.urlretrieve") +@mock.patch("devservices.utils.install_binary.download_file") @mock.patch("devservices.utils.install_binary.os.chmod") @mock.patch( "devservices.utils.install_binary.shutil.move", @@ -96,7 +100,7 @@ def test_install_docker_compose_chmod_permission_error( ) def test_install_docker_compose_shutil_move_permission_error( mock_chmod: mock.Mock, - mock_urlretrieve: mock.Mock, + mock_download_file: mock.Mock, mock_move: mock.Mock, ) -> None: with pytest.raises( @@ -109,3 +113,45 @@ def test_install_docker_compose_shutil_move_permission_error( "1.0.0", "http:://example.com", ) + + +@mock.patch("devservices.utils.install_binary.urlopen") +def test_download_file_uses_timeout( + mock_urlopen: mock.Mock, tmp_path: pathlib.Path +) -> None: + mock_urlopen.return_value.__enter__.return_value = io.BytesIO(b"binary") + path = tmp_path / "binary-name" + + download_file("https://example.com/binary", str(path)) + + mock_urlopen.assert_called_once_with( + "https://example.com/binary", timeout=DOWNLOAD_TIMEOUT + ) + assert path.read_bytes() == b"binary" + + +@mock.patch("devservices.utils.retry.time.sleep") +@mock.patch( + "devservices.utils.install_binary.download_file", + side_effect=[TimeoutError("timed out"), None], +) +@mock.patch("devservices.utils.install_binary.os.chmod") +@mock.patch("devservices.utils.install_binary.shutil.move") +def test_install_binary_retries_stalled_download( + mock_move: mock.Mock, + mock_chmod: mock.Mock, + mock_download_file: mock.Mock, + mock_sleep: mock.Mock, + capsys: pytest.CaptureFixture[str], +) -> None: + install_binary( + "binary-name", + "exec_path", + "1.0.0", + "https://example.com/binary", + ) + + assert mock_download_file.call_count == 2 + mock_sleep.assert_called_once_with(1.0) + mock_move.assert_called_once() + assert "Download failed (timed out)" in capsys.readouterr().out From 061e677d76c787d59a3e0f3a4ea288afabedd977 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:08:46 +0000 Subject: [PATCH 2/2] test(docker-compose): Use the pinned version in the version-check success test The test still reported 2.29.7, so after the bump it no longer matched the pinned version and fell through to a real install_docker_compose download. Use MINIMUM_DOCKER_COMPOSE_VERSION and mock the installer so the test can never download. --- tests/utils/test_docker_compose.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/utils/test_docker_compose.py b/tests/utils/test_docker_compose.py index 7b4eaede..ab6dd2fc 100644 --- a/tests/utils/test_docker_compose.py +++ b/tests/utils/test_docker_compose.py @@ -11,6 +11,7 @@ from devservices.configs.service_config import load_service_config_from_file from devservices.constants import CONFIG_FILE_NAME from devservices.constants import DEVSERVICES_DIR_NAME +from devservices.constants import MINIMUM_DOCKER_COMPOSE_VERSION from devservices.exceptions import BinaryInstallError from devservices.exceptions import DockerComposeError from devservices.exceptions import DockerComposeInstallationError @@ -44,9 +45,13 @@ def _run( @mock.patch("subprocess.run") -def test_check_docker_compose_version_success(mock_run: mock.Mock) -> None: - mock_run.return_value.stdout = "2.29.7\n" +@mock.patch("devservices.utils.docker_compose.install_docker_compose") +def test_check_docker_compose_version_success( + mock_install_docker_compose: mock.Mock, mock_run: mock.Mock +) -> None: + mock_run.return_value.stdout = f"{MINIMUM_DOCKER_COMPOSE_VERSION}\n" check_docker_compose_version() # Should not raise any exception + mock_install_docker_compose.assert_not_called() @mock.patch("subprocess.run")