From 7e9e82398de272967c90c3c10970d5b0f5901a61 Mon Sep 17 00:00:00 2001
From: Graham Christensen <graham@grahamc.com>
Date: Thu, 20 Jan 2022 15:04:57 -0500
Subject: [PATCH] build-remote: copy missing paths from the binary cache to
 localhost
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In a Hydra instance I saw:

    possibly transient failure building ‘/nix/store/X.drv’ on ‘localhost’:
      dependency '/nix/store/Y' of '/nix/store/Y.drv' does not exist,
      and substitution is disabled

This is confusing because the Hydra in question does have substitution enabled.

This instance uses:

  keep-outputs = true
  keep-derivations = true

and an S3 binary cache which is not configured as a substituter in the nix.conf.

It appears this instance encountered a situation where store path Y was built
and present in the binary cache, and Y.drv was GC rooted on the instance,
however Y was not on the host.

When Hydra would try to build this path locally, it would look in the binary
cache to see if it was cached:

    (nix)
    439      bool valid = isValidPathUncached(storePath);
    440
    441      if (diskCache && !valid)
    442          // FIXME: handle valid = true case.
    443          diskCache->upsertNarInfo(getUri(), hashPart, 0);
    444
    445      return valid;

Since it was cached, the store path was considered Valid.

The queue monitor would then not put this input in for substitution, because
the path is valid:

    (hydra)
    470          if (!destStore->isValidPath(*i.second.path(*localStore, step->drv->name, i.first))) {
    471              valid = false;
    472              missing.insert_or_assign(i.first, i.second);
    473          }

Hydra appears to correctly handle the case of missing paths that need
to be substituted from the binary cache already, but since most
Hydra instances use `keep-outputs` *and* all paths in the binary cache
originate from that machine, it is not common for a path to be cached
and not GC rooted locally.

I'll run Hydra with this patch for a while and see if we run in to the
problem again.

A big thanks to John Ericson who helped debug this particular issue.
---
 src/hydra-queue-runner/build-remote.cc | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/hydra-queue-runner/build-remote.cc b/src/hydra-queue-runner/build-remote.cc
index 9f789978..a82e911e 100644
--- a/src/hydra-queue-runner/build-remote.cc
+++ b/src/hydra-queue-runner/build-remote.cc
@@ -271,7 +271,11 @@ void State::buildRemote(ref<Store> destStore,
         }
 
         /* Copy the input closure. */
-        if (!machine->isLocalhost()) {
+        if (machine->isLocalhost()) {
+            StorePathSet closure;
+            destStore->computeFSClosure(inputs, closure);
+            copyPaths(*destStore, *localStore, closure, NoRepair, NoCheckSigs, NoSubstitute);
+        } else {
             auto mc1 = std::make_shared<MaintainCount<counter>>(nrStepsWaiting);
             mc1.reset();
             MaintainCount<counter> mc2(nrStepsCopyingTo);