diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index a86a01fd8bb..7d29472332a 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -1186,11 +1186,6 @@ DerivationBuildingGoal::checkPathValidity(std::map & Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Success::Status status, SingleDrvOutputs builtOutputs) { - buildResult.inner = BuildResult::Success{ - .status = status, - .builtOutputs = std::move(builtOutputs), - }; - mcRunningBuilds.reset(); if (status == BuildResult::Success::Built) @@ -1198,16 +1193,15 @@ Goal::Done DerivationBuildingGoal::doneSuccess(BuildResult::Success::Status stat worker.updateProgress(); - return amDone(ecSuccess, std::nullopt); + return Goal::doneSuccess( + BuildResult::Success{ + .status = status, + .builtOutputs = std::move(builtOutputs), + }); } Goal::Done DerivationBuildingGoal::doneFailure(BuildError ex) { - buildResult.inner = BuildResult::Failure{ - .status = ex.status, - .errorMsg = fmt("%s", Uncolored(ex.info().msg)), - }; - mcRunningBuilds.reset(); if (ex.status == BuildResult::Failure::TimedOut) @@ -1219,7 +1213,13 @@ Goal::Done DerivationBuildingGoal::doneFailure(BuildError ex) worker.updateProgress(); - return amDone(ecFailed, {std::move(ex)}); + return Goal::doneFailure( + ecFailed, + BuildResult::Failure{ + .status = ex.status, + .errorMsg = fmt("%s", Uncolored(ex.info().msg)), + }, + std::move(ex)); } } // namespace nix diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index e92fa25f3d8..4c536bb1d90 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -465,20 +465,6 @@ UnkeyedRealisation DerivationGoal::assertPathValidity() Goal::Done DerivationGoal::doneSuccess(BuildResult::Success::Status status, UnkeyedRealisation builtOutput) { - buildResult.inner = BuildResult::Success{ - .status = status, - .builtOutputs = {{ - wantedOutput, - { - std::move(builtOutput), - DrvOutput{ - .drvHash = outputHash, - .outputName = wantedOutput, - }, - }, - }}, - }; - mcExpectedBuilds.reset(); if (status == BuildResult::Success::Built) @@ -486,16 +472,24 @@ Goal::Done DerivationGoal::doneSuccess(BuildResult::Success::Status status, Unke worker.updateProgress(); - return amDone(ecSuccess, std::nullopt); + return Goal::doneSuccess( + BuildResult::Success{ + .status = status, + .builtOutputs = {{ + wantedOutput, + { + std::move(builtOutput), + DrvOutput{ + .drvHash = outputHash, + .outputName = wantedOutput, + }, + }, + }}, + }); } Goal::Done DerivationGoal::doneFailure(BuildError ex) { - buildResult.inner = BuildResult::Failure{ - .status = ex.status, - .errorMsg = fmt("%s", Uncolored(ex.info().msg)), - }; - mcExpectedBuilds.reset(); if (ex.status == BuildResult::Failure::TimedOut) @@ -507,7 +501,13 @@ Goal::Done DerivationGoal::doneFailure(BuildError ex) worker.updateProgress(); - return amDone(ecFailed, {std::move(ex)}); + return Goal::doneFailure( + ecFailed, + BuildResult::Failure{ + .status = ex.status, + .errorMsg = fmt("%s", Uncolored(ex.info().msg)), + }, + std::move(ex)); } } // namespace nix diff --git a/src/libstore/build/derivation-trampoline-goal.cc b/src/libstore/build/derivation-trampoline-goal.cc index 963156aa584..cfa0c538f95 100644 --- a/src/libstore/build/derivation-trampoline-goal.cc +++ b/src/libstore/build/derivation-trampoline-goal.cc @@ -98,7 +98,13 @@ Goal::Co DerivationTrampolineGoal::init() trace("outer load and build derivation"); if (nrFailed != 0) { - co_return amDone(ecFailed, Error("cannot build missing derivation '%s'", drvReq->to_string(worker.store))); + co_return doneFailure( + ecFailed, + BuildResult::Failure{ + .status = BuildResult::Failure::DependencyFailed, + .errorMsg = fmt("failed to obtain derivation of '%s'", drvReq->to_string(worker.store)), + }, + Error("failed to obtain derivation of '%s'", drvReq->to_string(worker.store))); } StorePath drvPath = resolveDerivedPath(worker.store, *drvReq); diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 4bbd4c8f059..04b16f5a8b1 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -63,12 +63,18 @@ std::vector Store::buildPathsWithResults( std::vector results; results.reserve(state.size()); - for (auto & [req, goalPtr] : state) + for (auto & [req, goalPtr] : state) { + /* Goals that were never started or were cancelled have exitCode + ecBusy and a default buildResult with empty errorMsg. Skip them + to avoid reporting spurious failures with empty messages. */ + if (goalPtr->exitCode == Goal::ecBusy) + continue; results.emplace_back( KeyedBuildResult{ goalPtr->buildResult, /* .path = */ req, }); + } return results; } diff --git a/src/libstore/build/goal.cc b/src/libstore/build/goal.cc index 92437040cbe..d119384da3f 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -183,6 +183,19 @@ Co Goal::await(Goals new_waitees) co_return Return{}; } +Goal::Done Goal::doneSuccess(BuildResult::Success success) +{ + buildResult.inner = std::move(success); + return amDone(ecSuccess); +} + +Goal::Done Goal::doneFailure(ExitCode result, BuildResult::Failure failure, std::optional ex) +{ + assert(result == ecFailed || result == ecNoSubstituters); + buildResult.inner = std::move(failure); + return amDone(result, std::move(ex)); +} + Goal::Done Goal::amDone(ExitCode result, std::optional ex) { trace("done"); diff --git a/src/libstore/build/substitution-goal.cc b/src/libstore/build/substitution-goal.cc index b1273835c68..4eaf017a894 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -28,20 +28,21 @@ PathSubstitutionGoal::~PathSubstitutionGoal() Goal::Done PathSubstitutionGoal::doneSuccess(BuildResult::Success::Status status) { - buildResult.inner = BuildResult::Success{ - .status = status, - }; - return amDone(ecSuccess); + return Goal::doneSuccess( + BuildResult::Success{ + .status = status, + }); } Goal::Done PathSubstitutionGoal::doneFailure(ExitCode result, BuildResult::Failure::Status status, std::string errorMsg) { debug(errorMsg); - buildResult.inner = BuildResult::Failure{ - .status = status, - .errorMsg = std::move(errorMsg), - }; - return amDone(result); + return Goal::doneFailure( + result, + BuildResult::Failure{ + .status = status, + .errorMsg = std::move(errorMsg), + }); } Goal::Co PathSubstitutionGoal::init() diff --git a/src/libstore/include/nix/store/build/goal.hh b/src/libstore/include/nix/store/build/goal.hh index 703b14bd805..f1df869ec8f 100644 --- a/src/libstore/include/nix/store/build/goal.hh +++ b/src/libstore/include/nix/store/build/goal.hh @@ -500,9 +500,28 @@ protected: * Signals that the goal is done. * `co_return` the result. If you're not inside a coroutine, you can ignore * the return value safely. + * + * Prefer using `doneSuccess` or `doneFailure` instead, which ensure + * `buildResult` is set correctly. */ Done amDone(ExitCode result, std::optional ex = {}); + /** + * Signals successful completion of the goal. + * Sets `buildResult` and calls `amDone`. + */ + Done doneSuccess(BuildResult::Success success); + + /** + * Signals failed completion of the goal. + * Sets `buildResult` and calls `amDone`. + * + * @param result The exit code (ecFailed or ecNoSubstituters) + * @param failure The failure details including status and error message + * @param ex Optional exception to store/log + */ + Done doneFailure(ExitCode result, BuildResult::Failure failure, std::optional ex = {}); + public: virtual void cleanup() {} diff --git a/tests/functional/build.sh b/tests/functional/build.sh index 0b06dcd917d..f07827fd2db 100755 --- a/tests/functional/build.sh +++ b/tests/functional/build.sh @@ -158,14 +158,24 @@ printf "" | nix build --no-link --stdin --json | jq --exit-status '. == []' printf "%s\n" "$drv^*" | nix build --no-link --stdin --json | jq --exit-status '.[0]|has("drvPath")' # --keep-going and FOD -out="$(nix build -f fod-failing.nix -L 2>&1)" && status=0 || status=$? -test "$status" = 1 -# one "hash mismatch" error, one "build of ... failed" -test "$(<<<"$out" grep -cE '^error:')" = 2 +if isDaemonNewer "2.34pre"; then + # With the fix, cancelled goals are not reported as failures. + # Use -j1 so only x1 starts and fails; x2, x3, x4 are cancelled. + out="$(nix build -f fod-failing.nix -j1 -L 2>&1)" && status=0 || status=$? + test "$status" = 1 + # Only the hash mismatch error for x1. Cancelled goals not reported. + test "$(<<<"$out" grep -cE '^error:')" = 1 + # Regression test: error messages should not be empty (end with just "failed:") + <<<"$out" grepQuietInverse -E "^error:.*failed: *$" +else + out="$(nix build -f fod-failing.nix -L 2>&1)" && status=0 || status=$? + test "$status" = 1 + # At minimum, check that x1 is reported as failing + <<<"$out" grepQuiet -E "error:.*-x1" +fi <<<"$out" grepQuiet -E "hash mismatch in fixed-output derivation '.*-x1\\.drv'" <<<"$out" grepQuiet -vE "hash mismatch in fixed-output derivation '.*-x3\\.drv'" <<<"$out" grepQuiet -vE "hash mismatch in fixed-output derivation '.*-x2\\.drv'" -<<<"$out" grepQuiet -E "error: build of '.*-x[1-4]\\.drv\\^out', '.*-x[1-4]\\.drv\\^out', '.*-x[1-4]\\.drv\\^out', '.*-x[1-4]\\.drv\\^out' failed" out="$(nix build -f fod-failing.nix -L x1 x2 x3 --keep-going 2>&1)" && status=0 || status=$? test "$status" = 1 @@ -203,3 +213,57 @@ else fi <<<"$out" grepQuiet -vE "hash mismatch in fixed-output derivation '.*-x3\\.drv'" <<<"$out" grepQuiet -vE "hash mismatch in fixed-output derivation '.*-x2\\.drv'" + +# Regression test: cancelled builds should not be reported as failures +# When fast-fail fails, slow and depends-on-slow are cancelled (not failed). +# Only fast-fail should be reported as a failure. +# Uses fifo for synchronization to ensure deterministic behavior. +# Requires -j2 so slow and fast-fail run concurrently (fifo deadlocks if serialized). +if isDaemonNewer "2.34pre" && canUseSandbox; then + fifoDir="$TEST_ROOT/cancelled-builds-fifo" + mkdir -p "$fifoDir" + mkfifo "$fifoDir/fifo" + chmod a+rw "$fifoDir/fifo" + out="$(nix flake check ./cancelled-builds --impure -L -j2 \ + --option sandbox true \ + --option sandbox-paths "${NIX_STORE:-/nix/store}" \ + --option sandbox-build-dir /build-tmp \ + --option extra-sandbox-paths "/cancelled-builds-fifo=$fifoDir" \ + 2>&1)" && status=0 || status=$? + rm -rf "$fifoDir" + test "$status" = 1 + # The error should be for fast-fail, not for cancelled goals + <<<"$out" grepQuiet -E "Cannot build.*fast-fail" + # Cancelled goals should NOT appear in error messages (but may appear in "will be built" list) + <<<"$out" grepQuietInverse -E "^error:.*slow" + <<<"$out" grepQuietInverse -E "^error:.*depends-on-slow" + <<<"$out" grepQuietInverse -E "^error:.*depends-on-fail" + # Error messages should not be empty (end with just "failed:") + <<<"$out" grepQuietInverse -E "^error:.*failed: *$" + + # Test that nix build follows the same rules (uses a slightly different code path) + mkdir -p "$fifoDir" + mkfifo "$fifoDir/fifo" + chmod a+rw "$fifoDir/fifo" + system=$(nix eval --raw --impure --expr builtins.currentSystem) + out="$(nix build --impure -L -j2 \ + --option sandbox true \ + --option sandbox-paths "${NIX_STORE:-/nix/store}" \ + --option sandbox-build-dir /build-tmp \ + --option extra-sandbox-paths "/cancelled-builds-fifo=$fifoDir" \ + "./cancelled-builds#checks.$system.slow" \ + "./cancelled-builds#checks.$system.depends-on-slow" \ + "./cancelled-builds#checks.$system.fast-fail" \ + "./cancelled-builds#checks.$system.depends-on-fail" \ + 2>&1)" && status=0 || status=$? + rm -rf "$fifoDir" + test "$status" = 1 + # The error should be for fast-fail, not for cancelled goals + <<<"$out" grepQuiet -E "Cannot build.*fast-fail" + # Cancelled goals should NOT appear in error messages + <<<"$out" grepQuietInverse -E "^error:.*slow" + <<<"$out" grepQuietInverse -E "^error:.*depends-on-slow" + <<<"$out" grepQuietInverse -E "^error:.*depends-on-fail" + # Error messages should not be empty (end with just "failed:") + <<<"$out" grepQuietInverse -E "^error:.*failed: *$" +fi diff --git a/tests/functional/cancelled-builds/flake.nix b/tests/functional/cancelled-builds/flake.nix new file mode 100644 index 00000000000..0b8bf1ca5d8 --- /dev/null +++ b/tests/functional/cancelled-builds/flake.nix @@ -0,0 +1,64 @@ +# Regression test for cancelled builds not being reported as failures. +# +# Scenario: When a build fails while other builds are running, those other +# builds (and their dependents) get cancelled. Previously, cancelled builds +# were incorrectly reported as failures with empty error messages. +# +# Uses a fifo for synchronization: fast-fail waits for slow to start before +# failing, ensuring slow is actually running when it gets cancelled. +# +# See: tests/functional/build.sh (search for "cancelled-builds") +{ + outputs = + { self }: + let + config = import "${builtins.getEnv "_NIX_TEST_BUILD_DIR"}/config.nix"; + in + with config; + { + checks.${system} = { + # A derivation that signals it started via fifo, then waits + slow = mkDerivation { + name = "slow"; + buildCommand = '' + echo "slow: started, signaling via fifo" + echo started > /cancelled-builds-fifo/fifo + echo "slow: waiting..." + sleep 10 + touch $out + ''; + }; + + # Depends on slow - will be cancelled when fast-fail fails + depends-on-slow = mkDerivation { + name = "depends-on-slow"; + slow = self.checks.${system}.slow; + buildCommand = '' + echo "depends-on-slow: slow finished at $slow" + touch $out + ''; + }; + + # Waits for slow to start via fifo, then fails + fast-fail = mkDerivation { + name = "fast-fail"; + buildCommand = '' + echo "fast-fail: waiting for slow to start..." + read line < /cancelled-builds-fifo/fifo + echo "fast-fail: slow started, now failing" >&2 + exit 1 + ''; + }; + + # Depends on fast-fail - will fail with DependencyFailed + depends-on-fail = mkDerivation { + name = "depends-on-fail"; + fast-fail = self.checks.${system}.fast-fail; + buildCommand = '' + echo "depends-on-fail: fast-fail finished (should never get here)" + touch $out + ''; + }; + }; + }; +} diff --git a/tests/functional/dyn-drv/failing-outer.sh b/tests/functional/dyn-drv/failing-outer.sh index 596efe43dbd..dcf3e830ed5 100644 --- a/tests/functional/dyn-drv/failing-outer.sh +++ b/tests/functional/dyn-drv/failing-outer.sh @@ -10,3 +10,36 @@ if [[ -v NIX_DAEMON_PACKAGE ]]; then expected=1; fi # work around the daemon not expectStderr "$expected" nix-build ./text-hashed-output.nix -A failingWrapper --no-out-link \ | grepQuiet "build of resolved derivation '.*use-dynamic-drv-in-non-dynamic-drv-wrong.drv' failed" + +# Test that error messages are not empty when a producer derivation fails. +# This exercises the nrFailed path in DerivationTrampolineGoal::init(). +# +# Using `nix build --expr` with builtins.outputOf creates a top-level +# DerivationTrampolineGoal that goes through buildPathsWithResults. +# When the producer fails, the nrFailed path must use doneFailure (not amDone) +# to set buildResult.inner with a proper error message. +# +# Without the fix, the error message would be empty because amDone doesn't +# set buildResult.inner, so rethrow() throws Error("") - an empty message. + +out=$(nix build --impure --no-link --expr ' + let + config = import (builtins.getEnv "_NIX_TEST_BUILD_DIR" + "/config.nix"); + inherit (config) mkDerivation; + + # A CA derivation that fails before producing a .drv + failingProducer = mkDerivation { + name = "failing-producer"; + buildCommand = "echo This producer fails; exit 1"; + __contentAddressed = true; + outputHashMode = "text"; + outputHashAlgo = "sha256"; + }; + in + # Build the dynamic derivation output directly - this creates a top-level + # DerivationTrampolineGoal, not a nested one inside a DerivationGoal + builtins.outputOf failingProducer.outPath "out" +' 2>&1) || true + +# The error message must NOT be empty - it should mention the failed derivation +echo "$out" | grepQuiet "failed to obtain derivation of"