diff mbox series

fetch/wget: rename the partial download on checksum mismatch

Message ID 20260929012831.4193448-1-ricardo.salveti@oss.qualcomm.com
State New
Headers show
Series fetch/wget: rename the partial download on checksum mismatch | expand

Commit Message

Ricardo Salveti Sept. 29, 2026, 1:28 a.m. UTC
The wget fetcher downloads into a .tmp file which is resumed with
--continue and is shared with the mirrors of the same file. When it
fails checksum verification it is left in place, so MIRRORS reuse the
bad file instead of downloading their own copy and, without a mirror,
every retry fails the same way until it is removed by hand. Rename it
with rename_bad_checksum(), as done for any other file with a bad
checksum, so it can be inspected and the next attempt starts from
scratch. Mirror downloads skip this check and are still handled by the
existing code once moved into place.

Seen with parallel CI runners sharing DL_DIR on Amazon FSx for OpenZFS
mounted over NFSv4.2. What corrupts the file there is not addressed,
only the recovery on retry. Tested with the new tests, which fail
without the fix.

AI-Generated: Uses Claude Code (Claude Fable 5.1)
Signed-off-by: Ricardo Salveti <ricardo.salveti@oss.qualcomm.com>
---
 lib/bb/fetch/__init__.py | 11 ++++++---
 lib/bb/fetch/wget.py     |  8 +++++-
 lib/bb/tests/fetch.py    | 53 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 5 deletions(-)
diff mbox series

Patch

diff --git a/lib/bb/fetch/__init__.py b/lib/bb/fetch/__init__.py
index fb6278439..eb3483a48 100644
--- a/lib/bb/fetch/__init__.py
+++ b/lib/bb/fetch/__init__.py
@@ -1079,7 +1079,7 @@  def build_mirroruris(origud, mirrors, ld):
 
     return uris, uds
 
-def rename_bad_checksum(ud, suffix):
+def rename_bad_checksum(ud, suffix, localpath=None):
     """
     Renames files to have suffix from parameter
     """
@@ -1087,10 +1087,13 @@  def rename_bad_checksum(ud, suffix):
     if ud.localpath is None:
         return
 
+    if localpath is None:
+        localpath = ud.localpath
+
     new_localpath = "%s_bad-checksum_%s" % (ud.localpath, suffix)
-    bb.warn("Renaming %s to %s" % (ud.localpath, new_localpath))
-    if not bb.utils.movefile(ud.localpath, new_localpath):
-        bb.warn("Renaming %s to %s failed, grep movefile in log.do_fetch to see why" % (ud.localpath, new_localpath))
+    bb.warn("Renaming %s to %s" % (localpath, new_localpath))
+    if not bb.utils.movefile(localpath, new_localpath):
+        bb.warn("Renaming %s to %s failed, grep movefile in log.do_fetch to see why" % (localpath, new_localpath))
 
 
 def try_mirror_url(fetch, origud, ud, ld, check = False):
diff --git a/lib/bb/fetch/wget.py b/lib/bb/fetch/wget.py
index 972c84048..6259c1245 100644
--- a/lib/bb/fetch/wget.py
+++ b/lib/bb/fetch/wget.py
@@ -167,7 +167,13 @@  class Wget(FetchMethod):
         # Try and verify any checksum now, meaning if it isn't correct, we don't remove the
         # original file, which might be a race (imagine two recipes referencing the same
         # source, one with an incorrect checksum)
-        bb.fetch.verify_checksum(ud, d, localpath=localpath, fatal_nochecksum=False)
+        try:
+            bb.fetch.verify_checksum(ud, d, localpath=localpath, fatal_nochecksum=False)
+        except bb.fetch.ChecksumError as e:
+            # The download is resumed with --continue, so a bad file left in place
+            # would be reused by the mirrors and by every later attempt
+            bb.fetch.rename_bad_checksum(ud, e.checksum, localpath=localpath)
+            raise
 
         # Remove the ".tmp" and move the file into position atomically
         # Our lock prevents multiple writers but mirroring code may grab incomplete files
diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 71b0a63fe..9627b3b1b 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -1706,6 +1706,59 @@  class FetchLatestVersionTest(FetcherTest):
                 r = bb.utils.vercmp_string(verstring, v_larger)
                 self.assertTrue(r == -1, msg="Package %s, version: %s <= %s" % (k[0], v_larger, verstring))
 
+class WgetChecksumTest(FetcherTest):
+    content = b"bitbake wget test data\n" * 64
+    # As large as the real file, so wget --continue has nothing left to fetch
+    # and keeps it as is
+    corrupt = b"x" * len(content)
+
+    def setUp(self):
+        super().setUp()
+        self.serverdir = os.path.join(self.tempdir, "server")
+        for subdir in ["upstream", "mirror"]:
+            os.makedirs(os.path.join(self.serverdir, subdir))
+        self.server = HTTPService(self.serverdir, host="127.0.0.1")
+        self.server.start()
+        self.baseurl = "http://127.0.0.1:%s" % self.server.port
+        self.url = "%s/upstream/test.bin;sha256sum=%s" % (
+            self.baseurl, hashlib.sha256(self.content).hexdigest())
+
+    def tearDown(self):
+        self.server.stop()
+        super().tearDown()
+
+    def write(self, path, data):
+        with open(path, "wb") as f:
+            f.write(data)
+
+    def assertDownloaded(self):
+        with open(os.path.join(self.dldir, "test.bin"), "rb") as f:
+            self.assertEqual(f.read(), self.content)
+        self.assertFalse(os.path.exists(os.path.join(self.dldir, "test.bin.tmp")))
+        self.assertTrue(os.path.exists(os.path.join(self.dldir,
+            "test.bin_bad-checksum_%s" % hashlib.sha256(self.corrupt).hexdigest())))
+
+    def test_wget_corrupt_partial_download(self):
+        self.write(os.path.join(self.serverdir, "upstream", "test.bin"), self.content)
+        self.write(os.path.join(self.dldir, "test.bin.tmp"), self.corrupt)
+
+        fetcher = bb.fetch.Fetch([self.url], self.d)
+        with self.assertRaises(bb.fetch.FetchError):
+            fetcher.download()
+        self.assertFalse(os.path.exists(os.path.join(self.dldir, "test.bin.tmp")))
+
+        fetcher.download()
+        self.assertDownloaded()
+
+    def test_wget_mirror_after_checksum_failure(self):
+        self.write(os.path.join(self.serverdir, "upstream", "test.bin"), self.corrupt)
+        self.write(os.path.join(self.serverdir, "mirror", "test.bin"), self.content)
+        self.d.setVar("MIRRORS", "%s/upstream/ %s/mirror/" % (self.baseurl, self.baseurl))
+
+        fetcher = bb.fetch.Fetch([self.url], self.d)
+        fetcher.download()
+        self.assertDownloaded()
+
 class FetchCheckStatusTest(FetcherTest):
     test_wget_uris = ["https://downloads.yoctoproject.org/releases/sato/sato-engine-0.1.tar.gz",
                       "https://downloads.yoctoproject.org/releases/sato/sato-engine-0.2.tar.gz",