Don't re-download a tarball we already have, even after the URL cache expires

Nix re-checks a downloaded tarball's URL every hour by default (tarball-ttl),
even when the caller already pinned the exact content hash (narHash). That
pinned hash is enough to know the content is still correct without
contacting the server again, but nothing was checking for that case, so
fetches kept hitting the network on a schedule for no reason.

If the cached entry for a URL has gone stale but its content still matches
the pinned hash, reuse it instead of re-downloading. This is skipped when
the TTL is explicitly 0 (as with `--refresh`), since that means the caller
wants the source re-verified regardless of any hash we already have on file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: tomberek

src/libfetchers/tarball.cc

@@ -105,7 +105,11 @@ DownloadFileResult downloadFile(
 }
 
 static DownloadTarballResult downloadTarball_(
-    const Settings & settings, const std::string & urlS, const Headers & headers, const std::string & displayPrefix)
+    const Settings & settings,
+    const std::string & urlS,
+    const Headers & headers,
+    const std::string & displayPrefix,
+    const std::optional<Hash> & expectedNarHash)
 {
     ParsedURL url = parseURL(urlS);
 
@@ -150,10 +154,23 @@ static DownloadTarballResult downloadTarball_(
     if (cached && !settings.getTarballCache()->hasObject(getRevAttr(cached->value, "treeHash")))
         cached.reset();
 
-    if (cached && !cached->expired)
-        /* We previously downloaded this tarball and it's younger than
-           `tarballTtl`, so no need to check the server. */
-        return attrsToResult(cached->value);
+    if (cached) {
+        if (!cached->expired)
+            /* We previously downloaded this tarball and it's younger than
+               `tarballTtl`, so no need to check the server. */
+            return attrsToResult(cached->value);
+
+        /* The cached entry is stale, but if its content still matches
+           the pinned NAR hash, there's no need to re-check the server
+           either. Exception: a TTL of 0 (e.g. `--refresh`) means the
+           caller explicitly wants us to verify against the server, so
+           don't let a merely-matching old hash short-circuit that. */
+        if (expectedNarHash && settings.tarballTtl.get() != 0) {
+            auto treeHash = getRevAttr(cached->value, "treeHash");
+            if (settings.getTarballCache()->treeHashToNarHash(settings, treeHash) == *expectedNarHash)
+                return attrsToResult(cached->value);
+        }
+    }
 
     auto _res = std::make_shared<Sync<FileTransferResult>>();
 
@@ -491,7 +508,8 @@ struct TarballInputScheme : CurlInputScheme
     {
         auto input(_input);
 
-        auto result = downloadTarball_(settings, getStrAttr(input.attrs, "url"), {}, "«" + input.to_string() + "»");
+        auto result = downloadTarball_(
+            settings, getStrAttr(input.attrs, "url"), {}, "«" + input.to_string() + "»", input.getNarHash());
 
         if (result.immutableUrl) {
             auto immutableInput = Input::fromURL(*result.immutableUrl);

tests/functional/tarball.sh

@@ -31,6 +31,33 @@ test_tarball() {
     nix-build  -o "$TEST_ROOT"/result -E "import (fetchTree { type = \"tarball\"; url = \"file://$tarball\"; })"
     nix-build  -o "$TEST_ROOT"/result -E "import (fetchTree { type = \"tarball\"; url = \"file://$tarball\"; narHash = \"$hash\"; })"
 
+    # Once a URL has been fetched with a given narHash, re-fetching the same
+    # URL after its cache entry has genuinely expired (a nonzero TTL that has
+    # elapsed) should not require re-reading it. Prove this by corrupting the
+    # file in place and expecting the fetch to still succeed. (`--tarball-ttl
+    # 0` is excluded here since that's the explicit "always re-verify"
+    # signal used by `--refresh`, which must NOT be short-circuited.)
+    cp "$tarball" "$tarball.bak"
+    echo garbage > "$tarball"
+    sleep 2
+    nix-build -o "$TEST_ROOT"/result --tarball-ttl 1 -E "import (fetchTree { type = \"tarball\"; url = \"file://$tarball\"; narHash = \"$hash\"; })"
+    mv "$tarball.bak" "$tarball"
+
+    # `--refresh` (tarball-ttl 0) must always re-verify against the source,
+    # even if a narHash is pinned, so a mismatch must still be reported.
+    # Use a different URL with different (but validly archived) content, so
+    # the failure is a hash mismatch rather than an unpack error, and so the
+    # cache entry for `$tarball` itself isn't disturbed by the expected
+    # failure.
+    otherRoot=$TEST_ROOT/tarball-other
+    rm -rf "$otherRoot"
+    mkdir -p "$otherRoot"
+    echo "different content" > "$otherRoot/default.nix"
+    cp "${config_nix}" dependencies.builder*.sh "$otherRoot/"
+    otherTarball=$TEST_ROOT/tarball-other.tar$ext
+    (cd "$TEST_ROOT" && GNUTAR_REPRODUCIBLE=1 tar --mtime="$otherRoot"/default.nix --owner=0 --group=0 --numeric-owner --sort=name -c -f - tarball-other) | $compressor > "$otherTarball"
+    expectStderr 102 nix eval --refresh --raw --expr "(fetchTree { type = \"tarball\"; url = \"file://$otherTarball\"; narHash = \"$hash\"; }).outPath" | grepQuiet "NAR hash mismatch"
+
     [[ $(nix eval --impure --expr "(fetchTree \"file://$tarball\").lastModified") = 1000000000 ]]
 
     nix-instantiate --strict --eval -E "!((import (fetchTree { type = \"tarball\"; url = \"file://$tarball\"; narHash = \"$hash\"; })) ? submodules)" >&2