Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions apport/packaging_impl/apt_dpkg.py
Original file line number Diff line number Diff line change
Expand Up @@ -1434,13 +1434,7 @@ def is_transitional_package(package: str) -> bool:

last_written = time.time()
# fetch packages
try:
apt_cache.fetch_archives(fetcher=fetcher)
except apt.cache.FetchFailedException as error:
apport.logging.error(
"Package download error, try again later: %s", str(error)
)
sys.exit(1) # transient error
self._fetch_packages(apt_cache, fetcher)

if verbose:
print("Extracting downloaded debs...")
Expand Down Expand Up @@ -1594,6 +1588,31 @@ def _get_primary_mirror_from_apt_sources(apt_dir: str) -> str:
f" couldn't find configured source contains the `deb` type in {apt_dir}"
)

@staticmethod
def _fetch_packages(
apt_cache: apt.cache.Cache, fetcher: apt_pkg.Acquire | None = None
) -> None:
backoff = 1.0
while True:
try:
apt_cache.fetch_archives(fetcher=fetcher)
break
except apt.cache.FetchFailedException as error:
if backoff <= 3600 and "Too Many Requests" in str(error):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: this is misleading wrt the git log

While it is technically correct because the git log says "around 1h" so if the next delay is 4096s we've already waited 4095s (yay maths), backoff is still a derivation of the wall time and not the actual wall time. I'd really like a comment here to that effect like elapsed time is backoff-1 due to exponentiation so that the next person reading this code doesn't have to come to that conclusion on their own.

apport.logging.warning(
"Package download error, retrying in %i second(s): %s",
backoff,
str(error),
)
else:
apport.logging.error(
"Package download error, try again later: %s", str(error)
)
sys.exit(1) # transient error

time.sleep(backoff)
backoff *= 2

def _get_mirror(self, arch: str) -> str:
"""Return the distribution mirror URL.

Expand Down
38 changes: 38 additions & 0 deletions tests/unit/test_packaging_apt_dpkg.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from unittest.mock import MagicMock

import apt
import apt.cache
import apt.package

from apport.packaging_impl.apt_dpkg import (
Expand Down Expand Up @@ -44,6 +45,43 @@ def tearDown(self) -> None:
# pylint: disable-next=protected-access
impl._apt_cache = None

def test_fetch_packages_download_error(self) -> None:
"""Test _fetch_packages() with a download error."""
apt_cache = MagicMock()
apt_cache.fetch_archives.side_effect = apt.cache.FetchFailedException(
"Failed to fetch example.deb 404 Not Found"
)

with self.assertRaises(SystemExit):
# pylint: disable-next=protected-access
impl._fetch_packages(apt_cache)

apt_cache.fetch_archives.assert_called_once_with(fetcher=None)

@unittest.mock.patch("time.sleep")
def test_fetch_packages_too_many_requests(self, sleep_mock: MagicMock) -> None:
"""Test _fetch_packages() to retry on HTTP 429 'Too Many Requests'."""
too_many_requests_failure = apt.cache.FetchFailedException(
"Failed to fetch http://ddebs.ubuntu.com/pool/main"
"/p/pcre2/libpcre2-8-0-dbgsym_10.39-3build1_amd64.ddeb"
" 429 Too Many Requests [IP: 185.125.190.18 80]\n"
"Failed to fetch http://ddebs.ubuntu.com/pool/main"
"/libs/libselinux/libselinux1-dbgsym_3.3-1build2_amd64.ddeb"
" 429 Too Many Requests [IP: 185.125.190.18 80]\n"
)
apt_cache = MagicMock()
apt_cache.fetch_archives.side_effect = [
too_many_requests_failure,
too_many_requests_failure,
None,
]

# pylint: disable-next=protected-access
impl._fetch_packages(apt_cache)

apt_cache.fetch_archives.assert_called_with(fetcher=None)
self.assertEqual(sleep_mock.call_count, 2)

@unittest.mock.patch("apt.Cache", spec=apt.Cache)
def test_is_distro_package_no_candidate(self, apt_cache_mock: MagicMock) -> None:
"""is_distro_package() for package that has no candidate."""
Expand Down