fix(docker-compose): Download v5.6.0 from GitHub with a stall timeout

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"

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:

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:

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:

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")
@@ -186,7 +191,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 +207,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 +221,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 +254,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 +287,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 +309,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)

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,15 +92,15 @@ 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",
     side_effect=PermissionError("Insufficient Permissions"),
 )
 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