From 4ac4f675df3da01b6d814cd328dd3219dd472ac9 Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Tue, 5 Jun 2018 16:04:41 +0200
Subject: [PATCH] Don't require --fallback to recover from disappeared binary
 cache NARs

---
 src/libstore/binary-cache-store.cc |  6 +++++-
 src/libstore/build.cc              | 32 ++++++++++++++++++++----------
 src/libstore/store-api.hh          |  1 +
 tests/binary-cache.sh              | 19 +++++++++++++-----
 4 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc
index 11fa3cae2..76c0a1a89 100644
--- a/src/libstore/binary-cache-store.cc
+++ b/src/libstore/binary-cache-store.cc
@@ -218,7 +218,11 @@ void BinaryCacheStore::narFromPath(const Path & storePath, Sink & sink)
     auto info = queryPathInfo(storePath).cast<const NarInfo>();
 
     auto source = sinkToSource([this, url{info->url}](Sink & sink) {
-        getFile(url, sink);
+        try {
+            getFile(url, sink);
+        } catch (NoSuchBinaryCacheFile & e) {
+            throw SubstituteGone(e.what());
+        }
     });
 
     stats.narRead++;
diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 07b533783..8eb192059 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -733,7 +733,7 @@ private:
 
     /* Whether to retry substituting the outputs after building the
        inputs. */
-    bool retrySubstitution = false;
+    bool retrySubstitution;
 
     /* The derivation stored at drvPath. */
     std::unique_ptr<BasicDerivation> drv;
@@ -1123,6 +1123,8 @@ void DerivationGoal::haveDerivation()
 {
     trace("have derivation");
 
+    retrySubstitution = false;
+
     for (auto & i : drv->outputs)
         worker.store.addTempRoot(i.second.path);
 
@@ -1161,7 +1163,7 @@ void DerivationGoal::outputsSubstituted()
     /*  If the substitutes form an incomplete closure, then we should
         build the dependencies of this derivation, but after that, we
         can still use the substitutes for this derivation itself. */
-    if (nrIncompleteClosure > 0 && !retrySubstitution) retrySubstitution = true;
+    if (nrIncompleteClosure > 0) retrySubstitution = true;
 
     nrFailed = nrNoSubstituters = nrIncompleteClosure = 0;
 
@@ -3524,8 +3526,8 @@ private:
     /* The current substituter. */
     std::shared_ptr<Store> sub;
 
-    /* Whether any substituter can realise this path. */
-    bool hasSubstitute;
+    /* Whether a substituter failed. */
+    bool substituterFailed = false;
 
     /* Path info returned by the substituter's query info operation. */
     std::shared_ptr<const ValidPathInfo> info;
@@ -3589,7 +3591,6 @@ public:
 
 SubstitutionGoal::SubstitutionGoal(const Path & storePath, Worker & worker, RepairFlag repair)
     : Goal(worker)
-    , hasSubstitute(false)
     , repair(repair)
 {
     this->storePath = storePath;
@@ -3653,9 +3654,9 @@ void SubstitutionGoal::tryNext()
         /* Hack: don't indicate failure if there were no substituters.
            In that case the calling derivation should just do a
            build. */
-        amDone(hasSubstitute ? ecFailed : ecNoSubstituters);
+        amDone(substituterFailed ? ecFailed : ecNoSubstituters);
 
-        if (hasSubstitute) {
+        if (substituterFailed) {
             worker.failedSubstitutions++;
             worker.updateProgress();
         }
@@ -3691,8 +3692,6 @@ void SubstitutionGoal::tryNext()
 
     worker.updateProgress();
 
-    hasSubstitute = true;
-
     /* Bail out early if this substituter lacks a valid
        signature. LocalStore::addToStore() also checks for this, but
        only after we've downloaded the path. */
@@ -3807,8 +3806,19 @@ void SubstitutionGoal::finished()
         state = &SubstitutionGoal::init;
         worker.waitForAWhile(shared_from_this());
         return;
-    } catch (Error & e) {
-        printError(e.msg());
+    } catch (std::exception & e) {
+        printError(e.what());
+
+        /* Cause the parent build to fail unless --fallback is given,
+           or the substitute has disappeared. The latter case behaves
+           the same as the substitute never having existed in the
+           first place. */
+        try {
+            throw;
+        } catch (SubstituteGone &) {
+        } catch (...) {
+            substituterFailed = true;
+        }
 
         /* Try the next substitute. */
         state = &SubstitutionGoal::tryNext;
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index 6ee2d5506..7c5b495a4 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -22,6 +22,7 @@ MakeError(SubstError, Error)
 MakeError(BuildError, Error) /* denotes a permanent build failure */
 MakeError(InvalidPath, Error)
 MakeError(Unsupported, Error)
+MakeError(SubstituteGone, Error)
 
 
 struct BasicDerivation;
diff --git a/tests/binary-cache.sh b/tests/binary-cache.sh
index cbcdca89b..eb58ae7c1 100644
--- a/tests/binary-cache.sh
+++ b/tests/binary-cache.sh
@@ -76,19 +76,28 @@ if nix-store --substituters "file://$cacheDir" -r $outPath; then
 fi
 
 
-# Test whether fallback works if we have cached info but the
-# corresponding NAR has disappeared.
+# Test whether fallback works if a NAR has disappeared. This does not require --fallback.
 clearStore
 
-nix-build --substituters "file://$cacheDir" dependencies.nix --dry-run # get info
-
-mkdir $cacheDir/tmp
 mv $cacheDir/nar $cacheDir/nar2
 
+nix-build --substituters "file://$cacheDir" --no-require-sigs dependencies.nix -o $TEST_ROOT/result
+
+mv $cacheDir/nar2 $cacheDir/nar
+
+
+# Test whether fallback works if a NAR is corrupted. This does require --fallback.
+clearStore
+
+mv $cacheDir/nar $cacheDir/nar2
+mkdir $cacheDir/nar
+for i in $(cd $cacheDir/nar2 && echo *); do touch $cacheDir/nar/$i; done
+
 (! nix-build --substituters "file://$cacheDir" --no-require-sigs dependencies.nix -o $TEST_ROOT/result)
 
 nix-build --substituters "file://$cacheDir" --no-require-sigs dependencies.nix -o $TEST_ROOT/result --fallback
 
+rm -rf $cacheDir/nar
 mv $cacheDir/nar2 $cacheDir/nar