Fix concurrent builder failure empty message bugs

src/libstore/build/derivation-building-goal.cc

@@ -1186,28 +1186,22 @@ DerivationBuildingGoal::checkPathValidity(std::map<std::string, InitialOutput> &
 
 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)
         worker.doneBuilds++;
 
     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

src/libstore/build/derivation-goal.cc

@@ -465,37 +465,31 @@ 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)
         worker.doneBuilds++;
 
     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

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);

src/libstore/build/entry-points.cc

@@ -63,12 +63,18 @@ std::vector<KeyedBuildResult> Store::buildPathsWithResults(
     std::vector<KeyedBuildResult> 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;
 }

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<Error> 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<Error> ex)
 {
     trace("done");

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()

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<Error> 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<Error> ex = {});
+
 public:
     virtual void cleanup() {}
 

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

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
+          '';
+        };
+      };
+    };
+}