Improve error handling in buildcache downloads (#35568)

The checksum exception was not detailed enough and not reraised when using cache only, resulting in useless error messages.

Now it dumps the file path, expected
hash, computed hash, and the downloaded file summary.

6 files changed, 67 insertions, 44 deletions

diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py
index 330ee35911..4dfea7975d 100644
--- a/lib/spack/llnl/util/filesystem.py
+++ b/lib/spack/llnl/util/filesystem.py
@@ -2630,3 +2630,28 @@ def temporary_dir(
             yield tmp_dir
     finally:
         remove_directory_contents(tmp_dir)
+
+
+def filesummary(path, print_bytes=16) -> Tuple[int, bytes]:
+    """Create a small summary of the given file. Does not error
+    when file does not exist.
+
+    Args:
+        print_bytes (int): Number of bytes to print from start/end of file
+
+    Returns:
+        Tuple of size and byte string containing first n .. last n bytes.
+        Size is 0 if file cannot be read."""
+    try:
+        n = print_bytes
+        with open(path, "rb") as f:
+            size = os.fstat(f.fileno()).st_size
+            if size <= 2 * n:
+                short_contents = f.read(2 * n)
+            else:
+                short_contents = f.read(n)
+                f.seek(-n, 2)
+                short_contents += b"..." + f.read(n)
+        return size, short_contents
+    except OSError:
+        return 0, b""
diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 01f428c633..e9e48b4010 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -41,6 +41,7 @@ import spack.relocate as relocate
 import spack.repo
 import spack.store
 import spack.traverse as traverse
+import spack.util.crypto
 import spack.util.file_cache as file_cache
 import spack.util.gpg
 import spack.util.spack_json as sjson
@@ -553,7 +554,12 @@ class NoChecksumException(spack.error.SpackError):
     Raised if file fails checksum verification.
     """
 
-    pass
+    def __init__(self, path, size, contents, algorithm, expected, computed):
+        super(NoChecksumException, self).__init__(
+            f"{algorithm} checksum failed for {path}",
+            f"Expected {expected} but got {computed}. "
+            f"File size = {size} bytes. Contents = {contents!r}",
+        )
 
 
 class NewLayoutException(spack.error.SpackError):
@@ -1763,14 +1769,15 @@ def _extract_inner_tarball(spec, filename, extract_to, unsigned, remote_checksum
             raise UnsignedPackageException(
                 "To install unsigned packages, use the --no-check-signature option."
             )
-    # get the sha256 checksum of the tarball
+
+    # compute the sha256 checksum of the tarball
     local_checksum = checksum_tarball(tarfile_path)
+    expected = remote_checksum["hash"]
 
     # if the checksums don't match don't install
-    if local_checksum != remote_checksum["hash"]:
-        raise NoChecksumException(
-            "Package tarball failed checksum verification.\n" "It cannot be installed."
-        )
+    if local_checksum != expected:
+        size, contents = fsys.filesummary(tarfile_path)
+        raise NoChecksumException(tarfile_path, size, contents, "sha256", expected, local_checksum)
 
     return tarfile_path
 
@@ -1828,12 +1835,14 @@ def extract_tarball(spec, download_result, allow_root=False, unsigned=False, for
 
         # compute the sha256 checksum of the tarball
         local_checksum = checksum_tarball(tarfile_path)
+        expected = bchecksum["hash"]
 
         # if the checksums don't match don't install
-        if local_checksum != bchecksum["hash"]:
+        if local_checksum != expected:
+            size, contents = fsys.filesummary(tarfile_path)
             _delete_staged_downloads(download_result)
             raise NoChecksumException(
-                "Package tarball failed checksum verification.\n" "It cannot be installed."
+                tarfile_path, size, contents, "sha256", expected, local_checksum
             )
 
     new_relative_prefix = str(os.path.relpath(spec.prefix, spack.store.layout.root))
@@ -1924,8 +1933,11 @@ def install_root_node(spec, allow_root, unsigned=False, force=False, sha256=None
         tarball_path = download_result["tarball_stage"].save_filename
         msg = msg.format(tarball_path, sha256)
         if not checker.check(tarball_path):
+            size, contents = fsys.filesummary(tarball_path)
             _delete_staged_downloads(download_result)
-            raise spack.binary_distribution.NoChecksumException(msg)
+            raise NoChecksumException(
+                tarball_path, size, contents, checker.hash_name, sha256, checker.sum
+            )
         tty.debug("Verified SHA256 checksum of the build cache")
 
     # don't print long padded paths while extracting/relocating binaries
diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 19ad454ec5..0ea42ecb36 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -89,22 +89,6 @@ def _ensure_one_stage_entry(stage_path):
     return os.path.join(stage_path, stage_entries[0])
 
 
-def _filesummary(path, print_bytes=16):
-    try:
-        n = print_bytes
-        with open(path, "rb") as f:
-            size = os.fstat(f.fileno()).st_size
-            if size <= 2 * n:
-                short_contents = f.read(2 * n)
-            else:
-                short_contents = f.read(n)
-                f.seek(-n, 2)
-                short_contents += b"..." + f.read(n)
-        return size, short_contents
-    except OSError:
-        return 0, b""
-
-
 def fetcher(cls):
     """Decorator used to register fetch strategies."""
     all_strategies.append(cls)
@@ -513,7 +497,7 @@ class URLFetchStrategy(FetchStrategy):
             # On failure, provide some information about the file size and
             # contents, so that we can quickly see what the issue is (redirect
             # was not followed, empty file, text instead of binary, ...)
-            size, contents = _filesummary(self.archive_file)
+            size, contents = fs.filesummary(self.archive_file)
             raise ChecksumError(
                 f"{checker.hash_name} checksum failed for {self.archive_file}",
                 f"Expected {self.digest} but got {checker.sum}. "
diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py
index 7bbe8b2dd2..18eb00c2a9 100644
--- a/lib/spack/spack/installer.py
+++ b/lib/spack/spack/installer.py
@@ -1755,14 +1755,16 @@ class PackageInstaller(object):
                 raise
 
             except binary_distribution.NoChecksumException as exc:
-                if not task.cache_only:
-                    # Checking hash on downloaded binary failed.
-                    err = "Failed to install {0} from binary cache due to {1}:"
-                    err += " Requeueing to install from source."
-                    tty.error(err.format(pkg.name, str(exc)))
-                    task.use_cache = False
-                    self._requeue_task(task)
-                    continue
+                if task.cache_only:
+                    raise
+
+                # Checking hash on downloaded binary failed.
+                err = "Failed to install {0} from binary cache due to {1}:"
+                err += " Requeueing to install from source."
+                tty.error(err.format(pkg.name, str(exc)))
+                task.use_cache = False
+                self._requeue_task(task)
+                continue
 
             except (Exception, SystemExit) as exc:
                 self._update_failed(task, True, exc)
diff --git a/lib/spack/spack/test/fetch_strategy.py b/lib/spack/spack/test/fetch_strategy.py
index 932a527a52..59c645d4e0 100644
--- a/lib/spack/spack/test/fetch_strategy.py
+++ b/lib/spack/spack/test/fetch_strategy.py
@@ -14,13 +14,3 @@ def test_fetchstrategy_bad_url_scheme():
 
     with pytest.raises(ValueError):
         fetcher = fetch_strategy.from_url_scheme("bogus-scheme://example.com/a/b/c")  # noqa: F841
-
-
-def test_filesummary(tmpdir):
-    p = str(tmpdir.join("xyz"))
-    with open(p, "wb") as f:
-        f.write(b"abcdefghijklmnopqrstuvwxyz")
-
-    assert fetch_strategy._filesummary(p, print_bytes=8) == (26, b"abcdefgh...stuvwxyz")
-    assert fetch_strategy._filesummary(p, print_bytes=13) == (26, b"abcdefghijklmnopqrstuvwxyz")
-    assert fetch_strategy._filesummary(p, print_bytes=100) == (26, b"abcdefghijklmnopqrstuvwxyz")
diff --git a/lib/spack/spack/test/llnl/util/filesystem.py b/lib/spack/spack/test/llnl/util/filesystem.py
index bbadfcbca9..5e81b52c53 100644
--- a/lib/spack/spack/test/llnl/util/filesystem.py
+++ b/lib/spack/spack/test/llnl/util/filesystem.py
@@ -861,3 +861,13 @@ def test_remove_linked_tree_doesnt_change_file_permission(tmpdir, initial_mode):
     fs.remove_linked_tree(str(file_instead_of_dir))
     final_stat = os.stat(str(file_instead_of_dir))
     assert final_stat == initial_stat
+
+
+def test_filesummary(tmpdir):
+    p = str(tmpdir.join("xyz"))
+    with open(p, "wb") as f:
+        f.write(b"abcdefghijklmnopqrstuvwxyz")
+
+    assert fs.filesummary(p, print_bytes=8) == (26, b"abcdefgh...stuvwxyz")
+    assert fs.filesummary(p, print_bytes=13) == (26, b"abcdefghijklmnopqrstuvwxyz")
+    assert fs.filesummary(p, print_bytes=100) == (26, b"abcdefghijklmnopqrstuvwxyz")