From 72cad8918b21343ee55e14d7c2f1eeff1874ee91 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Tue, 22 Jul 2025 15:12:01 +0200 Subject: [PATCH] libstore: fix remote build failures killing everything remote builds failures used to be signaled via exit status 1 of the build hook, which in turn only happened because the build errors we got from remote stores was thrown and bubbled up to main which then logged the error and exited with code 1. with rpc we cannot do this any more. barring a rewrite of the worker infra to allow for errors being reported with something other than process exit codes this is the best can do. ideally we would wrap remote builds in a new goal. (and then remove all exit code shenanigans from DerivationGoal too) fixes #928 Change-Id: Idc3ede3cbaca34c8c8e40247da52794f2a5013b9 --- lix/libstore/build/derivation-goal.cc | 64 ++++++++++++++++++--------- lix/libstore/build/derivation-goal.hh | 3 +- tests/functional/build-remote.sh | 22 +++++++++ 3 files changed, 68 insertions(+), 21 deletions(-) diff --git a/lix/libstore/build/derivation-goal.cc b/lix/libstore/build/derivation-goal.cc index 7eb93292d..0349b503c 100644 --- a/lix/libstore/build/derivation-goal.cc +++ b/lix/libstore/build/derivation-goal.cc @@ -904,7 +904,8 @@ void runPostBuildHook( proc.getStdout()->drainInto(sink); } -kj::Promise> DerivationGoal::buildDone() noexcept +kj::Promise> DerivationGoal::buildDone(std::shared_ptr remoteError +) noexcept try { trace("build done"); @@ -917,7 +918,21 @@ try { to have terminated. In fact, the builder could also have simply have closed its end of the pipe, so just to be sure, kill it. */ - int status = getChildStatus(); + int rawStatus = getChildStatus(); + const auto [exited, exitCode, exitMsg] = [&]() -> std::tuple { + // override exit status with 1 if we received an exception via rpc for + // historical reasons: the build hook used to turn build errors into a + // log line and an `exit(1)` previously, now it returns the full error + if (remoteError) { + return {true, 1, "failed on remote builder"}; + } else { + if (WIFEXITED(rawStatus)) { + return {true, WEXITSTATUS(rawStatus), statusToString(rawStatus)}; + } else { + return {false, -1, statusToString(rawStatus)}; + } + } + }(); debug("builder process for '%s' finished", worker.store.printStorePath(drvPath)); @@ -933,11 +948,13 @@ try { cleanupPostChildKill(); if (buildResult.cpuUser && buildResult.cpuSystem) { - debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", + debug( + "builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", worker.store.printStorePath(drvPath), - status, + rawStatus, ((double) buildResult.cpuUser->count()) / 1000000, - ((double) buildResult.cpuSystem->count()) / 1000000); + ((double) buildResult.cpuSystem->count()) / 1000000 + ); } bool diskFull = false; @@ -945,13 +962,12 @@ try { try { /* Check the exit status. */ - if (!statusOk(status)) { + if (!exited || exitCode != 0) { diskFull |= cleanupDecideWhetherDiskFull(); - auto msg = fmt("builder for '%s' %s", - Magenta(worker.store.printStorePath(drvPath)), - statusToString(status)); + auto msg = + fmt("builder for '%s' %s", Magenta(worker.store.printStorePath(drvPath)), exitMsg); if (!logger->isVerbose() && !logTail.empty()) { msg += fmt(";\nlast %d log lines:\n", logTail.size()); @@ -1002,19 +1018,21 @@ try { BuildResult::Status st = BuildResult::MiscFailure; - if (hook && WIFEXITED(status) && WEXITSTATUS(status) == 101) + if (hook && exited && exitCode == 101) { st = BuildResult::TimedOut; - - else if (hook && (!WIFEXITED(status) || WEXITSTATUS(status) != 100)) { } - else { + else if (hook && (!exited || exitCode != 100)) + { + } + + else + { assert(derivationType); - st = - dynamic_cast(&e) ? BuildResult::NotDeterministic : - statusOk(status) ? BuildResult::OutputRejected : - !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : - BuildResult::PermanentFailure; + st = dynamic_cast(&e) ? BuildResult::NotDeterministic + : exited && exitCode == 0 ? BuildResult::OutputRejected + : !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure + : BuildResult::PermanentFailure; } co_return done(st, {}, std::move(e)); @@ -1118,7 +1136,7 @@ try { buildResult.startTime = time(0); // inexact started(); - TRY_AWAIT_RPC(runPromise); + auto result = co_await runPromise; // close the rpc connection to have the hook exit hook->rpc = nullptr; @@ -1128,7 +1146,13 @@ try { if (auto error = TRY_AWAIT(output)) { co_return HookResult::Accept{std::move(*error)}; } - co_return HookResult::Accept{TRY_AWAIT(buildDone())}; + + std::shared_ptr remoteError; + if (result.getResult().isBad()) { + remoteError = std::make_shared(from(result.getResult().getBad())); + logger->logEI(remoteError->info()); + } + co_return HookResult::Accept{TRY_AWAIT(buildDone(remoteError))}; } catch (...) { co_return result::current_exception(); } diff --git a/lix/libstore/build/derivation-goal.hh b/lix/libstore/build/derivation-goal.hh index 12c685cf1..c78a1d1a7 100644 --- a/lix/libstore/build/derivation-goal.hh +++ b/lix/libstore/build/derivation-goal.hh @@ -273,7 +273,8 @@ struct DerivationGoal : public Goal kj::Promise> inputsRealised() noexcept; kj::Promise> tryToBuild() noexcept; virtual kj::Promise> tryLocalBuild() noexcept; - kj::Promise> buildDone() noexcept; + kj::Promise> + buildDone(std::shared_ptr remoteError = nullptr) noexcept; /** * Is the build hook willing to perform the build? diff --git a/tests/functional/build-remote.sh b/tests/functional/build-remote.sh index 509978291..653dbf70c 100644 --- a/tests/functional/build-remote.sh +++ b/tests/functional/build-remote.sh @@ -78,3 +78,25 @@ out="$(nix-build 2>&1 failing.nix \ build_dir="$(grep "note: keeping build" <<< "$out" | sed -E "s/^(.*)note: keeping build directory '(.*)'(.*)$/\2/")" [[ "foo" = $(<"$build_dir"/b/bar) ]] + +# regression fj#928: --keep-going doesn't keep going with remote builders +output="$(nix-build 2>&1 \ + --store $TEST_ROOT/machine0 \ + --builders "ssh-ng://localhost?remote-store=$TEST_ROOT/machine3 - - 1 1" \ + --keep-going \ + --max-jobs 0 \ + --expr ' + let + fail = n: derivation { + name = n; + system = builtins.currentSystem; + builder = "/bin/sh"; + args = [ "-c" "false" ]; + }; + in { + a = fail "a"; + b = fail "b"; + } + ' || true)" +grep 'a.drv. failed on remote builder' <<<"$output" +grep 'b.drv. failed on remote builder' <<<"$output"