From 36d3c2f1dacad17e3d08e7f4fe6328af06c9b431 Mon Sep 17 00:00:00 2001 From: tbereknyei Date: Sat, 29 Aug 2026 08:28:28 -0400 Subject: [PATCH] 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 --- src/libfetchers/tarball.cc | 30 ++++++++++++++++++++++++------ tests/functional/tarball.sh | 27 +++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/src/libfetchers/tarball.cc b/src/libfetchers/tarball.cc index fdf6c8ad1167..74678a6dbd89 100644 --- a/src/libfetchers/tarball.cc +++ b/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 & 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>(); @@ -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); diff --git a/tests/functional/tarball.sh b/tests/functional/tarball.sh index cfb6c674665d..bc48b2835e50 100755 --- a/tests/functional/tarball.sh +++ b/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