From 97976111d0a26f6460161c18d2a9257c15a7221a Mon Sep 17 00:00:00 2001 From: Adam Dangoor Date: Mon, 14 Sep 2026 12:12:26 +0100 Subject: [PATCH 1/3] Retry transient Vale download failures --- CHANGELOG.md | 4 ++++ vale/main.py | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f2d37e..88059de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Retry transient failures when downloading Vale. + ## 3.21.0.0 (2026-09-10) - Using Vale v3.21.0 diff --git a/vale/main.py b/vale/main.py index 4f74d02..adbcfad 100755 --- a/vale/main.py +++ b/vale/main.py @@ -8,10 +8,12 @@ import sys import tarfile import tempfile +import time import zipfile from functools import partial from pathlib import Path -from typing import Optional, Tuple +from typing import Any, Optional, Tuple +from urllib.error import HTTPError, URLError from urllib.request import urlopen import vale @@ -34,6 +36,9 @@ def major_minor_patch(version: str) -> str: # re-release or upload deleted versions, every uploaded version must be unique. vale_bin_version = major_minor_patch(importlib_metadata.version("vale")) +_DOWNLOAD_ATTEMPTS = 3 +_RETRYABLE_HTTP_STATUS_CODES = {408, 429} + def get_target() -> Tuple[str, str, str]: """Return Vale's target OS, architecture, and extension to download.""" @@ -106,6 +111,30 @@ def extract_vale( return f"{vale_tmp_path}" +def _open_download_url(url: str) -> Any: + """Open a download URL, retrying transient failures.""" + for attempt in range(_DOWNLOAD_ATTEMPTS): + try: + return urlopen(url) + except HTTPError as error: + retryable = error.code in _RETRYABLE_HTTP_STATUS_CODES or 500 <= error.code < 600 + if not retryable or attempt == _DOWNLOAD_ATTEMPTS - 1: + raise + error.close() + except URLError: + if attempt == _DOWNLOAD_ATTEMPTS - 1: + raise + + delay = 2**attempt + print( + f"* Download failed. Retrying in {delay} second(s)...", + file=sys.stderr, + ) + time.sleep(delay) + + raise AssertionError("unreachable") + + def download_vale_if_missing() -> str: """Download vale only if missing.""" vale_bin_path = Path(vale.__file__).parent / "vale_bin" @@ -125,7 +154,7 @@ def download_vale_if_missing() -> str: f"/v{vale_bin_version}/{vale_archive_file_name}" ) - url = urlopen(url_str) + url = _open_download_url(url_str) with tempfile.TemporaryDirectory() as temp_dir: temp_dir_path = Path(temp_dir) From 711f6d6f38af99cf351b23439d585ea758a8a35f Mon Sep 17 00:00:00 2001 From: Adam Dangoor Date: Fri, 23 Oct 2026 09:55:03 +0100 Subject: [PATCH 2/3] Address download retry review feedback and test failure handling --- .github/workflows/test.yml | 2 + tests/test_download.py | 117 +++++++++++++++++++++++++++++++++++++ vale/main.py | 32 ++++++---- 3 files changed, 140 insertions(+), 11 deletions(-) create mode 100644 tests/test_download.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f812dd8..6366f87 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,6 +29,8 @@ jobs: export VALE_VERSION="$(cat pyproject.toml | grep '^version' | cut -d ' ' -f 3 | sed -e 's/"//g')" echo "Installing $VALE_VERSION ..." python -m pip install -v "./dist/vale-$VALE_VERSION-py3-none-any.whl" + - name: Test download retries + run: python -m unittest discover -s tests -v - name: Test Vale run: | vale --help diff --git a/tests/test_download.py b/tests/test_download.py new file mode 100644 index 0000000..f1d9ef9 --- /dev/null +++ b/tests/test_download.py @@ -0,0 +1,117 @@ +"""Tests for transient download failures without network access or real delays.""" + +import io +import unittest +from contextlib import redirect_stderr +from unittest.mock import call, patch +from urllib.error import HTTPError, URLError + +from vale.main import _open_download_url + + +class DownloadTests(unittest.TestCase): + def setUp(self): + self.urlopen = self.patch("vale.main.urlopen") + self.sleep = self.patch("vale.main.time.sleep") + self.jitter = self.patch("vale.main.random.uniform") + self.jitter.return_value = 3.5 + self.stderr = io.StringIO() + self.enter_stderr = redirect_stderr(self.stderr) + self.enter_stderr.__enter__() + self.addCleanup(self.enter_stderr.__exit__, None, None, None) + + def patch(self, target): + patcher = patch(target) + self.addCleanup(patcher.stop) + return patcher.start() + + def http_error(self, code): + return HTTPError( + "https://example.com/vale", code, "test reason", {}, io.BytesIO() + ) + + def test_success_uses_timeout_without_retry(self): + response = _open_download_url("https://example.com/vale") + + self.assertIs(response, self.urlopen.return_value) + self.urlopen.assert_called_once_with("https://example.com/vale", timeout=60) + self.sleep.assert_not_called() + self.jitter.assert_not_called() + self.assertEqual(self.stderr.getvalue(), "") + + def test_retries_all_transient_http_codes_and_closes_failed_response(self): + for code in (408, 429, *range(500, 600)): + with self.subTest(code=code): + error = self.http_error(code) + response = object() + self.urlopen.side_effect = [error, response] + + self.assertIs(_open_download_url("url"), response) + self.assertTrue(error.fp.closed) + self.assertIn(f"HTTP {code}: test reason", self.stderr.getvalue()) + + def test_permanent_http_errors_fail_immediately(self): + for code in (400, 401, 403, 404, 407, 409, 428, 430, 499, 600): + with self.subTest(code=code): + error = self.http_error(code) + self.addCleanup(error.close) + self.urlopen.reset_mock() + self.urlopen.side_effect = error + + with self.assertRaises(HTTPError) as raised: + _open_download_url("url") + + self.assertIs(raised.exception, error) + self.urlopen.assert_called_once_with("url", timeout=60) + self.sleep.assert_not_called() + self.jitter.assert_not_called() + self.assertEqual(self.stderr.getvalue(), "") + + def test_url_error_and_timeout_retry_with_reason(self): + for error in (URLError("connection reset"), TimeoutError("timed out")): + with self.subTest(error=error): + response = object() + self.urlopen.side_effect = [error, response] + + self.assertIs(_open_download_url("url"), response) + self.assertIn(str(error), self.stderr.getvalue()) + + def test_five_attempts_with_exponential_backoff_and_jitter(self): + error = URLError("connection reset") + self.urlopen.side_effect = [error] * 4 + [object()] + + response = _open_download_url("url") + + self.assertIsNotNone(response) + self.assertEqual(self.urlopen.call_args_list, [call("url", timeout=60)] * 5) + self.assertEqual( + self.sleep.call_args_list, [call(n) for n in (5.5, 7.5, 11.5, 19.5)] + ) + self.assertEqual(self.jitter.call_args_list, [call(0, 10)] * 4) + self.assertIn("Retrying in 5.5 second(s)", self.stderr.getvalue()) + + def test_final_failure_is_preserved_without_another_delay(self): + errors = (self.http_error(503), URLError("offline"), TimeoutError("timed out")) + for error in errors: + with self.subTest(error=error): + if isinstance(error, HTTPError): + self.addCleanup(error.close) + self.urlopen.reset_mock() + self.sleep.reset_mock() + self.jitter.reset_mock() + self.stderr.seek(0) + self.stderr.truncate() + self.urlopen.side_effect = error + + with self.assertRaises(type(error)) as raised: + _open_download_url("url") + + self.assertIs(raised.exception, error) + self.assertEqual(self.urlopen.call_count, 5) + self.assertEqual(self.sleep.call_count, 4) + self.assertEqual(self.jitter.call_count, 4) + self.assertEqual(self.stderr.getvalue().count("Retrying"), 4) + + +if __name__ == "__main__": + unittest.main() diff --git a/vale/main.py b/vale/main.py index adbcfad..0ffbe22 100755 --- a/vale/main.py +++ b/vale/main.py @@ -3,6 +3,7 @@ import os import platform +import random import shutil import subprocess import sys @@ -36,8 +37,7 @@ def major_minor_patch(version: str) -> str: # re-release or upload deleted versions, every uploaded version must be unique. vale_bin_version = major_minor_patch(importlib_metadata.version("vale")) -_DOWNLOAD_ATTEMPTS = 3 -_RETRYABLE_HTTP_STATUS_CODES = {408, 429} +_RETRYABLE_HTTP_STATUS_CODES = {408, 429, *range(500, 600)} def get_target() -> Tuple[str, str, str]: @@ -113,24 +113,34 @@ def extract_vale( def _open_download_url(url: str) -> Any: """Open a download URL, retrying transient failures.""" - for attempt in range(_DOWNLOAD_ATTEMPTS): + timeout_seconds = 60 + max_download_attempts = 5 + scale_factor = 2 + + for attempt in range(max_download_attempts): try: - return urlopen(url) + return urlopen(url, timeout=timeout_seconds) except HTTPError as error: - retryable = error.code in _RETRYABLE_HTTP_STATUS_CODES or 500 <= error.code < 600 - if not retryable or attempt == _DOWNLOAD_ATTEMPTS - 1: + if ( + error.code not in _RETRYABLE_HTTP_STATUS_CODES + or attempt == max_download_attempts - 1 + ): raise + failure_reason = f"HTTP {error.code}: {error.reason}" error.close() - except URLError: - if attempt == _DOWNLOAD_ATTEMPTS - 1: + except (URLError, TimeoutError) as error: + if attempt == max_download_attempts - 1: raise + failure_reason = str(error) - delay = 2**attempt + jitter_seconds = random.uniform(0, 10) + delay_seconds = (scale_factor * (2**attempt)) + jitter_seconds print( - f"* Download failed. Retrying in {delay} second(s)...", + f"* Download failed ({failure_reason}). " + f"Retrying in {delay_seconds} second(s)...", file=sys.stderr, ) - time.sleep(delay) + time.sleep(delay_seconds) raise AssertionError("unreachable") From 9250a5a5583c290bbc013599989399e9c665b1ba Mon Sep 17 00:00:00 2001 From: Adam Dangoor Date: Fri, 23 Oct 2026 10:46:44 +0100 Subject: [PATCH 3/3] Remove unrequested retry tests and CI step --- .github/workflows/test.yml | 2 - tests/test_download.py | 117 ------------------------------------- 2 files changed, 119 deletions(-) delete mode 100644 tests/test_download.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6366f87..f812dd8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,8 +29,6 @@ jobs: export VALE_VERSION="$(cat pyproject.toml | grep '^version' | cut -d ' ' -f 3 | sed -e 's/"//g')" echo "Installing $VALE_VERSION ..." python -m pip install -v "./dist/vale-$VALE_VERSION-py3-none-any.whl" - - name: Test download retries - run: python -m unittest discover -s tests -v - name: Test Vale run: | vale --help diff --git a/tests/test_download.py b/tests/test_download.py deleted file mode 100644 index f1d9ef9..0000000 --- a/tests/test_download.py +++ /dev/null @@ -1,117 +0,0 @@ -"""Tests for transient download failures without network access or real delays.""" - -import io -import unittest -from contextlib import redirect_stderr -from unittest.mock import call, patch -from urllib.error import HTTPError, URLError - -from vale.main import _open_download_url - - -class DownloadTests(unittest.TestCase): - def setUp(self): - self.urlopen = self.patch("vale.main.urlopen") - self.sleep = self.patch("vale.main.time.sleep") - self.jitter = self.patch("vale.main.random.uniform") - self.jitter.return_value = 3.5 - self.stderr = io.StringIO() - self.enter_stderr = redirect_stderr(self.stderr) - self.enter_stderr.__enter__() - self.addCleanup(self.enter_stderr.__exit__, None, None, None) - - def patch(self, target): - patcher = patch(target) - self.addCleanup(patcher.stop) - return patcher.start() - - def http_error(self, code): - return HTTPError( - "https://example.com/vale", code, "test reason", {}, io.BytesIO() - ) - - def test_success_uses_timeout_without_retry(self): - response = _open_download_url("https://example.com/vale") - - self.assertIs(response, self.urlopen.return_value) - self.urlopen.assert_called_once_with("https://example.com/vale", timeout=60) - self.sleep.assert_not_called() - self.jitter.assert_not_called() - self.assertEqual(self.stderr.getvalue(), "") - - def test_retries_all_transient_http_codes_and_closes_failed_response(self): - for code in (408, 429, *range(500, 600)): - with self.subTest(code=code): - error = self.http_error(code) - response = object() - self.urlopen.side_effect = [error, response] - - self.assertIs(_open_download_url("url"), response) - self.assertTrue(error.fp.closed) - self.assertIn(f"HTTP {code}: test reason", self.stderr.getvalue()) - - def test_permanent_http_errors_fail_immediately(self): - for code in (400, 401, 403, 404, 407, 409, 428, 430, 499, 600): - with self.subTest(code=code): - error = self.http_error(code) - self.addCleanup(error.close) - self.urlopen.reset_mock() - self.urlopen.side_effect = error - - with self.assertRaises(HTTPError) as raised: - _open_download_url("url") - - self.assertIs(raised.exception, error) - self.urlopen.assert_called_once_with("url", timeout=60) - self.sleep.assert_not_called() - self.jitter.assert_not_called() - self.assertEqual(self.stderr.getvalue(), "") - - def test_url_error_and_timeout_retry_with_reason(self): - for error in (URLError("connection reset"), TimeoutError("timed out")): - with self.subTest(error=error): - response = object() - self.urlopen.side_effect = [error, response] - - self.assertIs(_open_download_url("url"), response) - self.assertIn(str(error), self.stderr.getvalue()) - - def test_five_attempts_with_exponential_backoff_and_jitter(self): - error = URLError("connection reset") - self.urlopen.side_effect = [error] * 4 + [object()] - - response = _open_download_url("url") - - self.assertIsNotNone(response) - self.assertEqual(self.urlopen.call_args_list, [call("url", timeout=60)] * 5) - self.assertEqual( - self.sleep.call_args_list, [call(n) for n in (5.5, 7.5, 11.5, 19.5)] - ) - self.assertEqual(self.jitter.call_args_list, [call(0, 10)] * 4) - self.assertIn("Retrying in 5.5 second(s)", self.stderr.getvalue()) - - def test_final_failure_is_preserved_without_another_delay(self): - errors = (self.http_error(503), URLError("offline"), TimeoutError("timed out")) - for error in errors: - with self.subTest(error=error): - if isinstance(error, HTTPError): - self.addCleanup(error.close) - self.urlopen.reset_mock() - self.sleep.reset_mock() - self.jitter.reset_mock() - self.stderr.seek(0) - self.stderr.truncate() - self.urlopen.side_effect = error - - with self.assertRaises(type(error)) as raised: - _open_download_url("url") - - self.assertIs(raised.exception, error) - self.assertEqual(self.urlopen.call_count, 5) - self.assertEqual(self.sleep.call_count, 4) - self.assertEqual(self.jitter.call_count, 4) - self.assertEqual(self.stderr.getvalue().count("Retrying"), 4) - - -if __name__ == "__main__": - unittest.main()