From c27269722487cb55019bab5885fcb4cbe2fd7292 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Thu, 15 Jan 2026 23:37:30 +0300 Subject: [PATCH 01/24] Bump version --- .version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.version b/.version index ba13d3caf21..0b82b8d70e2 100644 --- a/.version +++ b/.version @@ -1 +1 @@ -2.33.1 +2.33.2 From c0c13d73233c740b7d278c71b161da7b16217564 Mon Sep 17 00:00:00 2001 From: Sergei Trofimovich Date: Fri, 16 Jan 2026 22:48:25 +0000 Subject: [PATCH 02/24] libutil: fix `linux` build on fresh `glibc` and `gcc` Without the change the build fails for me as: ../unix/file-descriptor.cc:404:70: error: 'RESOLVE_BENEATH' was not declared in this scope 404 | dirFd, path.rel_c_str(), flags, static_cast(mode), RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS); | ^~~~~~~~~~~~~~~ This happens for 2 reasons: 1. `__NR_openat2` constant was not pulled in from the according headers and as a result `` was not included. 2. `define HAVE_OPENAT2 0` build is broken: refers to missing `RESOLVE_BENEATH` normally pulled in from `` This changes fixes both. (cherry picked from commit 3256aba6a2c9cc970bffd54fd13bc6a19367ffbf) --- src/libutil/unix/file-descriptor.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libutil/unix/file-descriptor.cc b/src/libutil/unix/file-descriptor.cc index d90342ff0ec..501f07aec65 100644 --- a/src/libutil/unix/file-descriptor.cc +++ b/src/libutil/unix/file-descriptor.cc @@ -8,9 +8,12 @@ #include #include +#if defined(__linux__) +# include /* pull __NR_* definitions */ +#endif + #if defined(__linux__) && defined(__NR_openat2) # define HAVE_OPENAT2 1 -# include # include #else # define HAVE_OPENAT2 0 @@ -323,7 +326,7 @@ Descriptor unix::openFileEnsureBeneathNoSymlinks(Descriptor dirFd, const CanonPa { assert(!path.rel().starts_with('/')); /* Just in case the invariant is somehow broken. */ assert(!path.isRoot()); -#ifdef __linux__ +#if HAVE_OPENAT2 auto maybeFd = linux::openat2( dirFd, path.rel_c_str(), flags, static_cast(mode), RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS); if (maybeFd) { From fbffd5683e3002d28aa8a661edfa141fb77c75e0 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 21 Jan 2026 06:14:34 +0300 Subject: [PATCH 03/24] ci: Bump magic-nix-cache to disable on 429 (cherry picked from commit 1555677cd55772c974ea92d73f15643eb2e1738f) --- .github/actions/install-nix-action/action.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/actions/install-nix-action/action.yaml b/.github/actions/install-nix-action/action.yaml index 00d02d6a2f2..3e6d25378e3 100644 --- a/.github/actions/install-nix-action/action.yaml +++ b/.github/actions/install-nix-action/action.yaml @@ -128,4 +128,4 @@ runs: diagnostic-endpoint: '' use-flakehub: false use-gha-cache: true - source-revision: 92d9581367be2233c2d5714a2640e1339f4087d8 # main + source-revision: 93bcd50961a03a468b29fac9d96b7efd037cb507 # main From ed28ceb12fbd739713a0a39c1eb5f34ce694b495 Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Costa Date: Thu, 22 Jan 2026 06:24:41 +0000 Subject: [PATCH 04/24] fix(libstore/filetransfer): skip Accept-Encoding header for S3 SigV4 requests Some S3-compatible services (like GCS) modify the Accept-Encoding header in transit, which breaks AWS SigV4 signature verification since curl's implementation signs all headers including Accept-Encoding. Fixes: #15019 (cherry picked from commit fcfa1dc8ab37a5a5ca987a52f964c2e6cb565028) --- src/libstore/filetransfer.cc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 26ceba729e1..54f423af77e 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -131,7 +131,16 @@ struct curlFileTransfer : public FileTransfer { result.urls.push_back(request.uri.to_string()); - requestHeaders = curl_slist_append(requestHeaders, "Accept-Encoding: zstd, br, gzip, deflate, bzip2, xz"); + /* Don't set Accept-Encoding for S3 requests that use AWS SigV4 signing. + curl's SigV4 implementation signs all headers including Accept-Encoding, + but some S3-compatible services (like GCS) modify this header in transit, + causing signature verification to fail. + See https://github.com/NixOS/nix/issues/15019 */ +#if NIX_WITH_AWS_AUTH + if (!request.awsSigV4Provider) +#endif + requestHeaders = + curl_slist_append(requestHeaders, "Accept-Encoding: zstd, br, gzip, deflate, bzip2, xz"); if (!request.expectedETag.empty()) requestHeaders = curl_slist_append(requestHeaders, ("If-None-Match: " + request.expectedETag).c_str()); if (!request.mimeType.empty()) From 7808b682bb32cd7a50f17bd5ad3e8a3792c634a1 Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Costa Date: Thu, 22 Jan 2026 06:01:26 +0000 Subject: [PATCH 05/24] fix(libstore/filetransfer): restart source before upload retries When an upload fails with a transient HTTP error (e.g., S3 rate limiting with HTTP 503), retries would fail with "curl error: Failed to open/read local data from file/application" because the upload source was already exhausted from the previous attempt. Restart the source in init() to ensure it's at the beginning for both first attempts (no-op) and retries (necessary fix). Fixes: #15023 (cherry picked from commit fbd787b9105fc98973e73128289839f28b1b4ca6) --- src/libstore/filetransfer.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 54f423af77e..8a7577146ef 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -450,6 +450,11 @@ struct curlFileTransfer : public FileTransfer curl_easy_setopt(req, CURLOPT_CUSTOMREQUEST, "DELETE"); if (request.data) { + // Restart the source to ensure it's at the beginning. + // This is necessary for retries, where the source was + // already consumed by a previous attempt. + request.data->source->restart(); + if (request.method == HttpMethod::Post) { curl_easy_setopt(req, CURLOPT_POST, 1L); curl_easy_setopt(req, CURLOPT_POSTFIELDSIZE_LARGE, (curl_off_t) request.data->sizeHint); From f42de47a40387b47d3123d9396b678e3d87dae9e Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Fri, 23 Jan 2026 02:02:10 +0300 Subject: [PATCH 06/24] ci: Drop magic-nix-cache We are now seeing. I guess we are out with the cache. When the API responds with 418 (I'm a teapot) it seems like the only reasonable solution is to oblige. error: unable to download 'http://127.0.0.1:37515/7ms9f25xyxavf32pvdc3vb28nzzmkbn3.narinfo': HTTP error 418 response body: GitHub API error: GitHub Actions Cache throttled Magic Nix Cache. Not trying to use it again on this run. (cherry picked from commit dae41e06e8030e7b77414901055e4635cc99119f) --- .github/actions/install-nix-action/action.yaml | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/.github/actions/install-nix-action/action.yaml b/.github/actions/install-nix-action/action.yaml index 3e6d25378e3..535ae9d08fd 100644 --- a/.github/actions/install-nix-action/action.yaml +++ b/.github/actions/install-nix-action/action.yaml @@ -24,8 +24,8 @@ inputs: description: "Github token" required: true use_cache: - description: "Whether to setup magic-nix-cache" - default: true + description: "Whether to setup github actions cache (not implemented currently)" + default: false required: false runs: using: "composite" @@ -122,10 +122,3 @@ runs: source-url: ${{ inputs.experimental-installer-version != 'latest' && 'https://artifacts.nixos.org/experimental-installer/tag/${{ inputs.experimental-installer-version }}/${{ env.EXPERIMENTAL_INSTALLER_ARTIFACT }}' || '' }} nix-package-url: ${{ inputs.dogfood == 'true' && steps.download-nix-installer.outputs.tarball-path || (inputs.tarball_url || '') }} extra-conf: ${{ inputs.extra_nix_config }} - - uses: DeterminateSystems/magic-nix-cache-action@565684385bcd71bad329742eefe8d12f2e765b39 # v13 - if: ${{ inputs.use_cache == 'true' }} - with: - diagnostic-endpoint: '' - use-flakehub: false - use-gha-cache: true - source-revision: 93bcd50961a03a468b29fac9d96b7efd037cb507 # main From 1d39afac38d24317364c2568dcc264a44fa725fd Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 10 Jan 2026 16:15:48 +0100 Subject: [PATCH 07/24] Goal: add doneSuccess/doneFailure helpers to base class Add helpers to the base Goal class that set buildResult and call amDone, ensuring buildResult is always populated when a goal terminates. Derived class helpers now call the base class versions. This reorders operations: previously buildResult was set before bookkeeping (counter resets, worker stats), now it's set after. This is safe because the bookkeeping code (mcExpectedBuilds.reset(), worker.doneBuilds++, worker.updateProgress(), etc.) only accesses worker counters, not buildResult. (cherry picked from commit cb2ade20d4b45ae1f1838d453c5484287516308f) --- .../build/derivation-building-goal.cc | 24 +++++------ src/libstore/build/derivation-goal.cc | 42 +++++++++---------- src/libstore/build/goal.cc | 13 ++++++ src/libstore/build/substitution-goal.cc | 19 +++++---- src/libstore/include/nix/store/build/goal.hh | 19 +++++++++ 5 files changed, 75 insertions(+), 42 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 8221e12c697..12344a8c219 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -1176,11 +1176,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) @@ -1188,16 +1183,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) @@ -1209,7 +1203,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 b36685a242c..72e62cdb6d2 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -452,20 +452,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) @@ -473,16 +459,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) @@ -494,7 +488,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/goal.cc b/src/libstore/build/goal.cc index 6266329e7e5..1c5c6bfe48c 100644 --- a/src/libstore/build/goal.cc +++ b/src/libstore/build/goal.cc @@ -133,6 +133,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 ac18de304b7..46924437d03 100644 --- a/src/libstore/build/substitution-goal.cc +++ b/src/libstore/build/substitution-goal.cc @@ -29,20 +29,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 4d57afc0f7c..cb1cb94e462 100644 --- a/src/libstore/include/nix/store/build/goal.hh +++ b/src/libstore/include/nix/store/build/goal.hh @@ -393,9 +393,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() {} From a6c201e039caec3d1d3e363a0928a8697574f65a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 10 Jan 2026 16:20:34 +0100 Subject: [PATCH 08/24] DerivationTrampolineGoal: use doneFailure to set buildResult DerivationTrampolineGoal is the top-level goal whose buildResult is returned by buildPathsWithResults. When it failed without setting buildResult.inner, buildPathsWithResults would return failures with empty errorMsg, producing error messages like: error: failed to build attribute 'checks.x86_64-linux.foo', build of '/nix/store/...drv^*' failed: (note the empty message after "failed:") Use the new doneFailure helper to ensure buildResult is populated with meaningful error information. (cherry picked from commit 25eb07a91b377f62322600d45d520453111a79eb) --- .../build/derivation-trampoline-goal.cc | 8 ++++- tests/functional/dyn-drv/failing-outer.sh | 33 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/libstore/build/derivation-trampoline-goal.cc b/src/libstore/build/derivation-trampoline-goal.cc index 963156aa584..4cfd2055999 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("cannot build missing derivation '%s'", drvReq->to_string(worker.store)), + }, + Error("cannot build missing derivation '%s'", drvReq->to_string(worker.store))); } StorePath drvPath = resolveDerivedPath(worker.store, *drvReq); diff --git a/tests/functional/dyn-drv/failing-outer.sh b/tests/functional/dyn-drv/failing-outer.sh index 596efe43dbd..3884a5853db 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 missing derivation +echo "$out" | grepQuiet "cannot build missing derivation" From 753bf479f9505e01b63a1470243ccbeb50a000d0 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 11 Jan 2026 01:06:31 +0100 Subject: [PATCH 09/24] tests: don't expect cancelled goals to be reported as failures When keepGoing=false and a build fails, other goals are cancelled. Previously, these cancelled goals were reported in the "build of ... failed" error message alongside actual failures. This was misleading since cancelled goals didn't actually fail - they were never tried. Update the test to expect only the actual failure (hash mismatch) to be reported, not the cancelled goals. (cherry picked from commit 3fd85c7d64327df3b145f239211ebf7519a69c2a) --- tests/functional/build.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/functional/build.sh b/tests/functional/build.sh index 0b06dcd917d..9ffaca918f4 100755 --- a/tests/functional/build.sh +++ b/tests/functional/build.sh @@ -158,14 +158,14 @@ 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=$? +out="$(nix build -f fod-failing.nix -j1 -L 2>&1)" && status=0 || status=$? test "$status" = 1 -# one "hash mismatch" error, one "build of ... failed" -test "$(<<<"$out" grep -cE '^error:')" = 2 +# Only the hash mismatch error for the first failing goal (x1). +# The other goals (x2, x3, x4) are cancelled and not reported as failures. +test "$(<<<"$out" grep -cE '^error:')" = 1 <<<"$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 From 09884e2c1ac213fd34321739168e6b4891d1131d Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 11 Jan 2026 01:26:40 +0100 Subject: [PATCH 10/24] buildPathsWithResults: don't report cancelled goals as failures When !keepGoing and a goal fails, other goals are cancelled and remain with exitCode == ecBusy. These cancelled goals have a default BuildResult::Failure{} with empty errorMsg. Previously, buildPathsWithResults would return these cancelled goals, and throwBuildErrors would report them as failures. When only one such cancelled goal was present, it would throw an error with an empty message like: error: build of '/nix/store/...drv^*' failed: Now we skip goals with ecBusy since their state is indeterminate. Cancelled goals could be reported, but this keeps the output relevant. Other indeterminate goal states were already not being reported, for instance: derivations that weren't started for being blocked on a concurrency limit, or blocked on a currently building dependency. (cherry picked from commit 68f549def46586fc5aee8b5ce62e7e8bb5a16703) --- src/libstore/build/entry-points.cc | 8 ++- tests/functional/build.sh | 74 +++++++++++++++++++-- tests/functional/cancelled-builds/flake.nix | 64 ++++++++++++++++++ 3 files changed, 140 insertions(+), 6 deletions(-) create mode 100644 tests/functional/cancelled-builds/flake.nix 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/tests/functional/build.sh b/tests/functional/build.sh index 9ffaca918f4..f07827fd2db 100755 --- a/tests/functional/build.sh +++ b/tests/functional/build.sh @@ -158,11 +158,21 @@ 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 -j1 -L 2>&1)" && status=0 || status=$? -test "$status" = 1 -# Only the hash mismatch error for the first failing goal (x1). -# The other goals (x2, x3, x4) are cancelled and not reported as failures. -test "$(<<<"$out" grep -cE '^error:')" = 1 +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'" @@ -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 + ''; + }; + }; + }; +} From 2ec62f197469d8ffbb3b10aa927706794d8b213a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 11 Jan 2026 19:34:52 +0100 Subject: [PATCH 11/24] DerivationTrampolineGoal: improve error message wording Change "cannot build missing derivation" to "failed to obtain derivation of" since the path (e.g. '...drv^out') is a derivation output, not a derivation. The message could be improved further to resolve ambiguity when multiple outputOf links are involved, but for now we err on the side of brevity since this message is already merged into larger error messages with other context from the Worker and CLI. (cherry picked from commit 3c3ceb18e9a3c421ab5990e381dfbe40e3dc3cec) --- src/libstore/build/derivation-trampoline-goal.cc | 4 ++-- tests/functional/dyn-drv/failing-outer.sh | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libstore/build/derivation-trampoline-goal.cc b/src/libstore/build/derivation-trampoline-goal.cc index 4cfd2055999..cfa0c538f95 100644 --- a/src/libstore/build/derivation-trampoline-goal.cc +++ b/src/libstore/build/derivation-trampoline-goal.cc @@ -102,9 +102,9 @@ Goal::Co DerivationTrampolineGoal::init() ecFailed, BuildResult::Failure{ .status = BuildResult::Failure::DependencyFailed, - .errorMsg = fmt("cannot build missing derivation '%s'", drvReq->to_string(worker.store)), + .errorMsg = fmt("failed to obtain derivation of '%s'", drvReq->to_string(worker.store)), }, - Error("cannot build missing derivation '%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/tests/functional/dyn-drv/failing-outer.sh b/tests/functional/dyn-drv/failing-outer.sh index 3884a5853db..dcf3e830ed5 100644 --- a/tests/functional/dyn-drv/failing-outer.sh +++ b/tests/functional/dyn-drv/failing-outer.sh @@ -41,5 +41,5 @@ out=$(nix build --impure --no-link --expr ' builtins.outputOf failingProducer.outPath "out" ' 2>&1) || true -# The error message must NOT be empty - it should mention the missing derivation -echo "$out" | grepQuiet "cannot build missing derivation" +# The error message must NOT be empty - it should mention the failed derivation +echo "$out" | grepQuiet "failed to obtain derivation of" From 48bf9a8e506875b2a8ff06a027e85389cf88c5b2 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sat, 24 Jan 2026 12:39:56 +0100 Subject: [PATCH 12/24] tests: fix sandbox-paths in cancelled-builds test Don't add the whole store to sandbox-paths unconditionally. Exposing the entire store defeats the purpose of sandboxing, and when the test store is the same as the system store (NixOS VM), it causes an obscure "Permission denied" error. Only add sandbox-paths when not on NixOS, indicating a separate test store that needs access to system store build tools. (cherry picked from commit 7b4444f17441fa2057a47c66f098487e6ee45d3d) --- tests/functional/build.sh | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/functional/build.sh b/tests/functional/build.sh index f07827fd2db..dbc72d99135 100755 --- a/tests/functional/build.sh +++ b/tests/functional/build.sh @@ -224,9 +224,17 @@ if isDaemonNewer "2.34pre" && canUseSandbox; then mkdir -p "$fifoDir" mkfifo "$fifoDir/fifo" chmod a+rw "$fifoDir/fifo" + # When using a separate test store, we need sandbox-paths to access + # the system store (where bash/coreutils live). On NixOS, the test + # uses the system store directly, so this isn't needed (and would + # conflict with input paths). + sandboxPathsArg=() + if ! isTestOnNixOS; then + sandboxPathsArg=(--option sandbox-paths "/nix/store") + fi out="$(nix flake check ./cancelled-builds --impure -L -j2 \ --option sandbox true \ - --option sandbox-paths "${NIX_STORE:-/nix/store}" \ + "${sandboxPathsArg[@]}" \ --option sandbox-build-dir /build-tmp \ --option extra-sandbox-paths "/cancelled-builds-fifo=$fifoDir" \ 2>&1)" && status=0 || status=$? @@ -245,10 +253,14 @@ if isDaemonNewer "2.34pre" && canUseSandbox; then mkdir -p "$fifoDir" mkfifo "$fifoDir/fifo" chmod a+rw "$fifoDir/fifo" + sandboxPathsArg=() + if ! isTestOnNixOS; then + sandboxPathsArg=(--option sandbox-paths "/nix/store") + fi system=$(nix eval --raw --impure --expr builtins.currentSystem) out="$(nix build --impure -L -j2 \ --option sandbox true \ - --option sandbox-paths "${NIX_STORE:-/nix/store}" \ + "${sandboxPathsArg[@]}" \ --option sandbox-build-dir /build-tmp \ --option extra-sandbox-paths "/cancelled-builds-fifo=$fifoDir" \ "./cancelled-builds#checks.$system.slow" \ From 70ecd8c8a9123bb6e79a19559fc1b5a5310ddbc8 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sat, 24 Jan 2026 23:31:11 +0300 Subject: [PATCH 13/24] Fix destruction of DerivationBuilder implementations This unsures that we call the correct virtual functions when destroying a particular DerivationBuilder. Usually the order of destructors is in the reverse order of inheritance: ChrootLinuxDerivationBuilder -> ChrootDerivationBuilder -> DerivationBuilderImpl autoDelChroot was being destroyed before the DerivationBuilderImpl::killChild was run and it would fail to clean up the chroot directory, since there were still processes writing to it. Note that ChrootLinuxDerivationBuilder::killSandbox was never run in the interrupted case at all, since virtual functions in destructors do not call derived class methods. I could reproduce the issue with the following derivation: let pkgs = import { }; in pkgs.runCommand "chroot-cleanup-race" { } '' mkdir -p $out for i in $(seq 1 200); do ( mkfifo $out/fifo$i cat $out/fifo$i > /dev/null & while true; do : > $out/file$i done ) & done sleep 0.05 echo done > $out/main '' While interrupting it manually when it would hang. Wrapping the unique pointer in a custom deleter function we can run all of the necessary clean up code consistently and calling the right virtual functions. Ideally we'd have a lint that bans the usage of virtual functions in destructors completely. (cherry picked from commit b752c5cb64c2675dc51aef6eb6b97d16a2a477e4) --- .../build/derivation-building-goal.cc | 21 ++++++------ .../nix/store/build/derivation-builder.hh | 11 +++++-- .../store/build/derivation-building-goal.hh | 3 +- src/libstore/unix/build/derivation-builder.cc | 32 +++++++++++++++---- .../unix/build/external-derivation-builder.cc | 5 +-- 5 files changed, 49 insertions(+), 23 deletions(-) diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 12344a8c219..6c43dcc9871 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -563,8 +563,7 @@ Goal::Co DerivationBuildingGoal::tryToBuild() { DerivationBuildingGoal & goal; - DerivationBuildingGoalCallbacks( - DerivationBuildingGoal & goal, std::unique_ptr & builder) + DerivationBuildingGoalCallbacks(DerivationBuildingGoal & goal) : goal{goal} { } @@ -632,15 +631,15 @@ Goal::Co DerivationBuildingGoal::tryToBuild() /* If we have to wait and retry (see below), then `builder` will already be created, so we don't need to create it again. */ - builder = externalBuilder ? makeExternalDerivationBuilder( - *localStoreP, - std::make_unique(*this, builder), - std::move(params), - *externalBuilder) - : makeDerivationBuilder( - *localStoreP, - std::make_unique(*this, builder), - std::move(params)); + builder = + externalBuilder + ? makeExternalDerivationBuilder( + *localStoreP, + std::make_unique(*this), + std::move(params), + *externalBuilder) + : makeDerivationBuilder( + *localStoreP, std::make_unique(*this), std::move(params)); } if (auto builderOutOpt = builder->startBuild()) { diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index f28fd8e5630..ca6319855ad 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -189,15 +189,22 @@ struct ExternalBuilder std::vector args; }; +struct DerivationBuilderDeleter +{ + void operator()(DerivationBuilder * builder) noexcept; +}; + +using DerivationBuilderUnique = std::unique_ptr; + #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows -std::unique_ptr makeDerivationBuilder( +DerivationBuilderUnique makeDerivationBuilder( LocalStore & store, std::unique_ptr miscMethods, DerivationBuilderParams params); /** * @param handler Must be chosen such that it supports the given * derivation. */ -std::unique_ptr makeExternalDerivationBuilder( +DerivationBuilderUnique makeExternalDerivationBuilder( LocalStore & store, std::unique_ptr miscMethods, DerivationBuilderParams params, diff --git a/src/libstore/include/nix/store/build/derivation-building-goal.hh b/src/libstore/include/nix/store/build/derivation-building-goal.hh index be95c796b05..e745c12c7ba 100644 --- a/src/libstore/include/nix/store/build/derivation-building-goal.hh +++ b/src/libstore/include/nix/store/build/derivation-building-goal.hh @@ -5,6 +5,7 @@ #include "nix/store/parsed-derivations.hh" #include "nix/store/derivation-options.hh" #include "nix/store/build/derivation-building-misc.hh" +#include "nix/store/build/derivation-builder.hh" #include "nix/store/outputs-spec.hh" #include "nix/store/store-api.hh" #include "nix/store/pathlocks.hh" @@ -89,7 +90,7 @@ private: */ std::unique_ptr hook; - std::unique_ptr builder; + DerivationBuilderUnique builder; #endif BuildMode buildMode; diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index eb0bf575735..360b6a48487 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -98,10 +98,13 @@ class DerivationBuilderImpl : public DerivationBuilder, public DerivationBuilder { } - ~DerivationBuilderImpl() + /** + * Cleanup code to run when destroying any DerivationBuilderImpl implementation. + */ + void cleanupOnDestruction() noexcept { /* Careful: we should never ever throw an exception from a - destructor. */ + noexcept function. */ try { killChild(); } catch (...) { @@ -1962,7 +1965,20 @@ StorePath DerivationBuilderImpl::makeFallbackPath(const StorePath & path) namespace nix { -std::unique_ptr makeDerivationBuilder( +void DerivationBuilderDeleter::operator()(DerivationBuilder * builder) noexcept +{ + if (!builder) /* Idempotent and handles nullptr as any deleter must. */ + return; + + if (auto builderImpl = dynamic_cast(builder)) + /* Note that this might call into virtual functions, which we can't do in a destructor of + the DerivationBuilderImpl itself. */ + builderImpl->cleanupOnDestruction(); + + delete builder; +} + +std::unique_ptr makeDerivationBuilder( LocalStore & store, std::unique_ptr miscMethods, DerivationBuilderParams params) { bool useSandbox = false; @@ -2013,17 +2029,19 @@ std::unique_ptr makeDerivationBuilder( throw Error("feature 'uid-range' is only supported in sandboxed builds"); #ifdef __APPLE__ - return std::make_unique(store, std::move(miscMethods), std::move(params), useSandbox); + return DerivationBuilderUnique( + new DarwinDerivationBuilder(store, std::move(miscMethods), std::move(params), useSandbox)); #elif defined(__linux__) if (useSandbox) - return std::make_unique(store, std::move(miscMethods), std::move(params)); + return DerivationBuilderUnique( + new ChrootLinuxDerivationBuilder(store, std::move(miscMethods), std::move(params))); - return std::make_unique(store, std::move(miscMethods), std::move(params)); + return DerivationBuilderUnique(new LinuxDerivationBuilder(store, std::move(miscMethods), std::move(params))); #else if (useSandbox) throw Error("sandboxing builds is not supported on this platform"); - return std::make_unique(store, std::move(miscMethods), std::move(params)); + return DerivationBuilderUnique(new DerivationBuilderImpl(store, std::move(miscMethods), std::move(params))); #endif } diff --git a/src/libstore/unix/build/external-derivation-builder.cc b/src/libstore/unix/build/external-derivation-builder.cc index 7ddb6e093b1..6c1a00f9106 100644 --- a/src/libstore/unix/build/external-derivation-builder.cc +++ b/src/libstore/unix/build/external-derivation-builder.cc @@ -106,13 +106,14 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl } }; -std::unique_ptr makeExternalDerivationBuilder( +DerivationBuilderUnique makeExternalDerivationBuilder( LocalStore & store, std::unique_ptr miscMethods, DerivationBuilderParams params, const ExternalBuilder & handler) { - return std::make_unique(store, std::move(miscMethods), std::move(params), handler); + return DerivationBuilderUnique( + new ExternalDerivationBuilder(store, std::move(miscMethods), std::move(params), handler)); } } // namespace nix From 3034589047af8b54abbabd63fb1dca3ce6915cbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Mon, 24 Nov 2025 21:06:59 +0100 Subject: [PATCH 14/24] libstore: add AWS SSO support for S3 authentication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This enables seamless AWS SSO authentication for S3 binary caches without requiring users to manually export credentials. This adds SSO support by calling aws_credentials_provider_new_sso() from the C library directly. It builds a custom credential chain: Env → SSO → Profile → IMDS The SSO provider requires a TLS context for HTTPS connections to SSO endpoints, which is created once and shared across all providers. (cherry picked from commit ec91479076ec29e9356a58ddb237d943f5277c73) --- src/libstore/aws-creds.cc | 97 ++++++++++++++++++++++++++++++++++++--- src/libstore/meson.build | 2 + 2 files changed, 92 insertions(+), 7 deletions(-) diff --git a/src/libstore/aws-creds.cc b/src/libstore/aws-creds.cc index dfdd81abbc4..cb7c4fae953 100644 --- a/src/libstore/aws-creds.cc +++ b/src/libstore/aws-creds.cc @@ -13,6 +13,9 @@ # include # include +// C library headers for SSO provider support +# include + # include # include @@ -30,6 +33,44 @@ AwsAuthError::AwsAuthError(int errorCode) namespace { +/** + * Helper function to wrap a C credentials provider in the C++ interface. + * This replicates the static s_CreateWrappedProvider from aws-crt-cpp. + */ +static std::shared_ptr createWrappedProvider( + aws_credentials_provider * rawProvider, Aws::Crt::Allocator * allocator = Aws::Crt::ApiAllocator()) +{ + if (rawProvider == nullptr) { + return nullptr; + } + + auto provider = Aws::Crt::MakeShared(allocator, rawProvider, allocator); + return std::static_pointer_cast(provider); +} + +/** + * Create an SSO credentials provider using the C library directly. + * The C++ wrapper doesn't expose SSO, so we call the C library and wrap the result. + * Returns nullptr if SSO provider creation fails (e.g., profile doesn't have SSO config). + */ +static std::shared_ptr createSSOProvider( + const std::string & profileName, + Aws::Crt::Io::ClientBootstrap * bootstrap, + Aws::Crt::Io::TlsContext * tlsContext, + Aws::Crt::Allocator * allocator = Aws::Crt::ApiAllocator()) +{ + aws_credentials_provider_sso_options options; + AWS_ZERO_STRUCT(options); + + options.bootstrap = bootstrap->GetUnderlyingHandle(); + options.tls_ctx = tlsContext ? tlsContext->GetUnderlyingHandle() : nullptr; + options.profile_name_override = aws_byte_cursor_from_c_str(profileName.c_str()); + + // Create the SSO provider - will return nullptr if SSO isn't configured for this profile + // createWrappedProvider handles nullptr gracefully + return createWrappedProvider(aws_credentials_provider_new_sso(allocator, &options), allocator); +} + static AwsCredentials getCredentialsFromProvider(std::shared_ptr provider) { if (!provider || !provider->IsValid()) { @@ -91,6 +132,12 @@ class AwsCredentialProviderImpl : public AwsCredentialProvider logLevel = Aws::Crt::LogLevel::Warn; } apiHandle.InitializeLogging(logLevel, stderr); + + // Create a shared TLS context for SSO (required for HTTPS connections) + auto allocator = Aws::Crt::ApiAllocator(); + auto tlsCtxOptions = Aws::Crt::Io::TlsContextOptions::InitDefaultClient(allocator); + tlsContext = + std::make_shared(tlsCtxOptions, Aws::Crt::Io::TlsMode::CLIENT, allocator); } AwsCredentials getCredentialsRaw(const std::string & profile); @@ -111,6 +158,7 @@ class AwsCredentialProviderImpl : public AwsCredentialProvider private: Aws::Crt::ApiHandle apiHandle; + std::shared_ptr tlsContext; boost::concurrent_flat_map> credentialProviderCache; }; @@ -123,18 +171,53 @@ AwsCredentialProviderImpl::createProviderForProfile(const std::string & profile) getpid(), profile.empty() ? "(default)" : profile.c_str()); + auto bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); + + // If no profile specified, use the default chain if (profile.empty()) { Aws::Crt::Auth::CredentialsProviderChainDefaultConfig config; - config.Bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); + config.Bootstrap = bootstrap; return Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderChainDefault(config); } - Aws::Crt::Auth::CredentialsProviderProfileConfig config; - config.Bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); - // This is safe because the underlying C library will copy this string - // c.f. https://github.com/awslabs/aws-c-auth/blob/main/source/credentials_provider_profile.c#L220 - config.ProfileNameOverride = Aws::Crt::ByteCursorFromCString(profile.c_str()); - return Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderProfile(config); + // For named profiles, build a custom credential chain: Environment → SSO → Profile → IMDS + // The SSO provider will gracefully fail if SSO isn't configured for this profile, + // and the chain will move on to the next provider. + Aws::Crt::Auth::CredentialsProviderChainConfig chainConfig; + auto allocator = Aws::Crt::ApiAllocator(); + + // 1. Environment variables (highest priority) + auto envProvider = Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderEnvironment(allocator); + if (envProvider) { + chainConfig.Providers.push_back(envProvider); + } + + // 2. SSO provider (try it, will fail gracefully if not configured) + auto ssoProvider = createSSOProvider(profile, bootstrap, tlsContext.get(), allocator); + if (ssoProvider) { + debug("[pid=%d] added SSO provider to credential chain for profile '%s'", getpid(), profile.c_str()); + chainConfig.Providers.push_back(ssoProvider); + } + + // 3. Profile provider (for static credentials) + Aws::Crt::Auth::CredentialsProviderProfileConfig profileConfig; + profileConfig.Bootstrap = bootstrap; + profileConfig.ProfileNameOverride = Aws::Crt::ByteCursorFromCString(profile.c_str()); + auto profileProvider = + Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderProfile(profileConfig, allocator); + if (profileProvider) { + chainConfig.Providers.push_back(profileProvider); + } + + // 4. IMDS provider (for EC2 instances, lowest priority) + Aws::Crt::Auth::CredentialsProviderImdsConfig imdsConfig; + imdsConfig.Bootstrap = bootstrap; + auto imdsProvider = Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderImds(imdsConfig, allocator); + if (imdsProvider) { + chainConfig.Providers.push_back(imdsProvider); + } + + return Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderChain(chainConfig, allocator); } AwsCredentials AwsCredentialProviderImpl::getCredentialsRaw(const std::string & profile) diff --git a/src/libstore/meson.build b/src/libstore/meson.build index d8927c3a6c3..088d8bcbb59 100644 --- a/src/libstore/meson.build +++ b/src/libstore/meson.build @@ -160,6 +160,8 @@ if s3_aws_auth.enabled() deps_other += aws_crt_cpp aws_c_common = cxx.find_library('aws-c-common', required : true) deps_other += aws_c_common + aws_c_auth = cxx.find_library('aws-c-auth', required : true) + deps_other += aws_c_auth endif configdata_pub.set('NIX_WITH_AWS_AUTH', s3_aws_auth.enabled().to_int()) From 344a0eaed1f0c93e144f135e296045542fb7f34f Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Date: Tue, 25 Nov 2025 13:59:25 -0500 Subject: [PATCH 15/24] refactor(libstore/aws-creds): improve error handling and logging Add validation for TLS context and client bootstrap initialization, with appropriate error messages when these fail. The TLS context failure is now a warning that gracefully disables SSO, while bootstrap failure throws since it's required for all providers. (cherry picked from commit 3c8e45c061baaf04163e02c97ad37f6a17342b60) --- src/libstore/aws-creds.cc | 56 ++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/src/libstore/aws-creds.cc b/src/libstore/aws-creds.cc index cb7c4fae953..634534f3021 100644 --- a/src/libstore/aws-creds.cc +++ b/src/libstore/aws-creds.cc @@ -138,6 +138,10 @@ class AwsCredentialProviderImpl : public AwsCredentialProvider auto tlsCtxOptions = Aws::Crt::Io::TlsContextOptions::InitDefaultClient(allocator); tlsContext = std::make_shared(tlsCtxOptions, Aws::Crt::Io::TlsMode::CLIENT, allocator); + if (!tlsContext || !*tlsContext) { + warn("failed to create TLS context for AWS SSO; SSO authentication will be unavailable"); + tlsContext = nullptr; + } } AwsCredentials getCredentialsRaw(const std::string & profile); @@ -172,6 +176,9 @@ AwsCredentialProviderImpl::createProviderForProfile(const std::string & profile) profile.empty() ? "(default)" : profile.c_str()); auto bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); + if (!bootstrap) { + throw AwsAuthError("failed to create AWS client bootstrap"); + } // If no profile specified, use the default chain if (profile.empty()) { @@ -186,36 +193,41 @@ AwsCredentialProviderImpl::createProviderForProfile(const std::string & profile) Aws::Crt::Auth::CredentialsProviderChainConfig chainConfig; auto allocator = Aws::Crt::ApiAllocator(); + auto addProviderToChain = [&](std::string_view name, auto createProvider) { + if (auto provider = createProvider()) { + chainConfig.Providers.push_back(provider); + debug("Added AWS %s Credential Provider to chain for profile '%s'", name, profile); + } else { + debug("Skipped AWS %s Credential Provider for profile '%s'", name, profile); + } + }; + // 1. Environment variables (highest priority) - auto envProvider = Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderEnvironment(allocator); - if (envProvider) { - chainConfig.Providers.push_back(envProvider); - } + addProviderToChain("Environment", [&]() { + return Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderEnvironment(allocator); + }); // 2. SSO provider (try it, will fail gracefully if not configured) - auto ssoProvider = createSSOProvider(profile, bootstrap, tlsContext.get(), allocator); - if (ssoProvider) { - debug("[pid=%d] added SSO provider to credential chain for profile '%s'", getpid(), profile.c_str()); - chainConfig.Providers.push_back(ssoProvider); + if (tlsContext) { + addProviderToChain("SSO", [&]() { return createSSOProvider(profile, bootstrap, tlsContext.get(), allocator); }); + } else { + debug("Skipped AWS SSO Credential Provider for profile '%s': TLS context unavailable", profile); } // 3. Profile provider (for static credentials) - Aws::Crt::Auth::CredentialsProviderProfileConfig profileConfig; - profileConfig.Bootstrap = bootstrap; - profileConfig.ProfileNameOverride = Aws::Crt::ByteCursorFromCString(profile.c_str()); - auto profileProvider = - Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderProfile(profileConfig, allocator); - if (profileProvider) { - chainConfig.Providers.push_back(profileProvider); - } + addProviderToChain("Profile", [&]() { + Aws::Crt::Auth::CredentialsProviderProfileConfig profileConfig; + profileConfig.Bootstrap = bootstrap; + profileConfig.ProfileNameOverride = Aws::Crt::ByteCursorFromCString(profile.c_str()); + return Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderProfile(profileConfig, allocator); + }); // 4. IMDS provider (for EC2 instances, lowest priority) - Aws::Crt::Auth::CredentialsProviderImdsConfig imdsConfig; - imdsConfig.Bootstrap = bootstrap; - auto imdsProvider = Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderImds(imdsConfig, allocator); - if (imdsProvider) { - chainConfig.Providers.push_back(imdsProvider); - } + addProviderToChain("IMDS", [&]() { + Aws::Crt::Auth::CredentialsProviderImdsConfig imdsConfig; + imdsConfig.Bootstrap = bootstrap; + return Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderImds(imdsConfig, allocator); + }); return Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderChain(chainConfig, allocator); } From 8a4d8aabb271c0fb45fa64daffdddf37c44d6408 Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Date: Tue, 25 Nov 2025 17:34:16 -0500 Subject: [PATCH 16/24] fix(libstore/aws-creds): add STS support for default profile The default (empty) profile case was using CreateCredentialsProviderChainDefault which didn't properly support role_arn/source_profile based role assumption via STS because TLS context wasn't being passed to the Profile provider. This change unifies the credential chain for all profiles (default and named), ensuring: - Consistent behavior between default and named profiles - Proper TLS context is passed for STS operations - SSO support works for both cases (cherry picked from commit 508d4463e57b21cf22c5b085a80361db27e59c4f) --- src/libstore/aws-creds.cc | 45 +++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/libstore/aws-creds.cc b/src/libstore/aws-creds.cc index 634534f3021..3a584a9c664 100644 --- a/src/libstore/aws-creds.cc +++ b/src/libstore/aws-creds.cc @@ -142,6 +142,12 @@ class AwsCredentialProviderImpl : public AwsCredentialProvider warn("failed to create TLS context for AWS SSO; SSO authentication will be unavailable"); tlsContext = nullptr; } + + // Get bootstrap (lives as long as apiHandle) + bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); + if (!bootstrap) { + throw AwsAuthError("failed to create AWS client bootstrap"); + } } AwsCredentials getCredentialsRaw(const std::string & profile); @@ -163,6 +169,7 @@ class AwsCredentialProviderImpl : public AwsCredentialProvider private: Aws::Crt::ApiHandle apiHandle; std::shared_ptr tlsContext; + Aws::Crt::Io::ClientBootstrap * bootstrap; boost::concurrent_flat_map> credentialProviderCache; }; @@ -170,35 +177,24 @@ class AwsCredentialProviderImpl : public AwsCredentialProvider std::shared_ptr AwsCredentialProviderImpl::createProviderForProfile(const std::string & profile) { - debug( - "[pid=%d] creating new AWS credential provider for profile '%s'", - getpid(), - profile.empty() ? "(default)" : profile.c_str()); - - auto bootstrap = Aws::Crt::ApiHandle::GetOrCreateStaticDefaultClientBootstrap(); - if (!bootstrap) { - throw AwsAuthError("failed to create AWS client bootstrap"); - } + // profileDisplayName is only used for debug logging - SDK uses its default profile + // when ProfileNameOverride is not set + const char * profileDisplayName = profile.empty() ? "(default)" : profile.c_str(); - // If no profile specified, use the default chain - if (profile.empty()) { - Aws::Crt::Auth::CredentialsProviderChainDefaultConfig config; - config.Bootstrap = bootstrap; - return Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderChainDefault(config); - } + debug("[pid=%d] creating new AWS credential provider for profile '%s'", getpid(), profileDisplayName); - // For named profiles, build a custom credential chain: Environment → SSO → Profile → IMDS - // The SSO provider will gracefully fail if SSO isn't configured for this profile, - // and the chain will move on to the next provider. + // Build a custom credential chain: Environment → SSO → Profile → IMDS + // This works for both default and named profiles, ensuring consistent behavior + // including SSO support and proper TLS context for STS-based role assumption. Aws::Crt::Auth::CredentialsProviderChainConfig chainConfig; auto allocator = Aws::Crt::ApiAllocator(); auto addProviderToChain = [&](std::string_view name, auto createProvider) { if (auto provider = createProvider()) { chainConfig.Providers.push_back(provider); - debug("Added AWS %s Credential Provider to chain for profile '%s'", name, profile); + debug("Added AWS %s Credential Provider to chain for profile '%s'", name, profileDisplayName); } else { - debug("Skipped AWS %s Credential Provider for profile '%s'", name, profile); + debug("Skipped AWS %s Credential Provider for profile '%s'", name, profileDisplayName); } }; @@ -211,14 +207,17 @@ AwsCredentialProviderImpl::createProviderForProfile(const std::string & profile) if (tlsContext) { addProviderToChain("SSO", [&]() { return createSSOProvider(profile, bootstrap, tlsContext.get(), allocator); }); } else { - debug("Skipped AWS SSO Credential Provider for profile '%s': TLS context unavailable", profile); + debug("Skipped AWS SSO Credential Provider for profile '%s': TLS context unavailable", profileDisplayName); } - // 3. Profile provider (for static credentials) + // 3. Profile provider (for static credentials and role_arn/source_profile with STS) addProviderToChain("Profile", [&]() { Aws::Crt::Auth::CredentialsProviderProfileConfig profileConfig; profileConfig.Bootstrap = bootstrap; - profileConfig.ProfileNameOverride = Aws::Crt::ByteCursorFromCString(profile.c_str()); + profileConfig.TlsContext = tlsContext.get(); + if (!profile.empty()) { + profileConfig.ProfileNameOverride = Aws::Crt::ByteCursorFromCString(profile.c_str()); + } return Aws::Crt::Auth::CredentialsProvider::CreateCredentialsProviderProfile(profileConfig, allocator); }); From cb1c413db8fab5295580998c77dfa06ae90dbc76 Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Date: Tue, 25 Nov 2025 14:20:38 -0500 Subject: [PATCH 17/24] chore(libstore/aws-creds): remove unused includes (cherry picked from commit 128b2b5c56d534a0220622cbaa58527561ff5185) --- src/libstore/aws-creds.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/libstore/aws-creds.cc b/src/libstore/aws-creds.cc index 3a584a9c664..3e4176c8907 100644 --- a/src/libstore/aws-creds.cc +++ b/src/libstore/aws-creds.cc @@ -4,10 +4,7 @@ # include # include "nix/store/s3-url.hh" -# include "nix/util/finally.hh" # include "nix/util/logging.hh" -# include "nix/util/url.hh" -# include "nix/util/util.hh" # include # include From 73a0311ef83100877e19b06f5487cec7f7ad4614 Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Date: Tue, 25 Nov 2025 15:46:47 -0500 Subject: [PATCH 18/24] test(s3-binary-cache-store): add profile support for setup_for_s3 (cherry picked from commit 11f108d8984564b9a26ee1c85f0727e4d738a973) --- tests/nixos/s3-binary-cache-store.nix | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/nixos/s3-binary-cache-store.nix b/tests/nixos/s3-binary-cache-store.nix index a2ba1dae6c7..1645d8a8dad 100644 --- a/tests/nixos/s3-binary-cache-store.nix +++ b/tests/nixos/s3-binary-cache-store.nix @@ -147,7 +147,7 @@ in else: machine.fail(f"nix path-info {pkg}") - def setup_s3(populate_bucket=[], public=False, versioned=False): + def setup_s3(populate_bucket=[], public=False, versioned=False, profiles=None): """ Decorator that creates/destroys a unique bucket for each test. Optionally pre-populates bucket with specified packages. @@ -157,6 +157,10 @@ in populate_bucket: List of packages to upload before test runs public: If True, make the bucket publicly accessible versioned: If True, enable versioning on the bucket before populating + profiles: Dict of AWS profiles to create, e.g.: + {"valid": {"access_key": "...", "secret_key": "..."}, + "invalid": {"access_key": "WRONG", "secret_key": "WRONG"}} + Profiles are created on the client machine at /root/.aws/credentials """ def decorator(test_func): def wrapper(): @@ -167,6 +171,15 @@ in server.succeed(f"mc anonymous set download minio/{bucket}") if versioned: server.succeed(f"mc version enable minio/{bucket}") + if profiles: + # Build credentials file content + creds_content = "" + for name, creds in profiles.items(): + creds_content += f"[{name}]\n" + creds_content += f"aws_access_key_id = {creds['access_key']}\n" + creds_content += f"aws_secret_access_key = {creds['secret_key']}\n\n" + client.succeed("mkdir -p /root/.aws") + client.succeed(f"cat > /root/.aws/credentials << 'AWSCREDS'\n{creds_content}AWSCREDS") if populate_bucket: store_url = make_s3_url(bucket) for pkg in populate_bucket: @@ -174,6 +187,9 @@ in test_func(bucket) finally: server.succeed(f"mc rb --force minio/{bucket}") + # Clean up AWS profiles if created + if profiles: + client.succeed("rm -rf /root/.aws") # Clean up client store - only delete if path exists for pkg in PKGS.values(): client.succeed(f"[ ! -e {pkg} ] || nix store delete --ignore-liveness {pkg}") From 2d85a6ba323b5d86b3abfe63c9a64ae4e3ad0103 Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Date: Tue, 25 Nov 2025 15:47:27 -0500 Subject: [PATCH 19/24] test(s3-binary-cache-store): clear credential cache between tests (cherry picked from commit 0595c5f7ee33331e2d1140a292560943b7e48522) --- tests/nixos/s3-binary-cache-store.nix | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/nixos/s3-binary-cache-store.nix b/tests/nixos/s3-binary-cache-store.nix index 1645d8a8dad..0e01d0a0134 100644 --- a/tests/nixos/s3-binary-cache-store.nix +++ b/tests/nixos/s3-binary-cache-store.nix @@ -164,6 +164,15 @@ in """ def decorator(test_func): def wrapper(): + # Restart nix-daemon on both machines to clear the credential provider cache. + # The AwsCredentialProviderImpl singleton persists in the daemon process, + # and its cache can cause credentials from previous tests to be reused. + # We reset-failed first to avoid systemd's start rate limiting. + server.succeed("systemctl reset-failed nix-daemon.service nix-daemon.socket") + server.succeed("systemctl restart nix-daemon") + client.succeed("systemctl reset-failed nix-daemon.service nix-daemon.socket") + client.succeed("systemctl restart nix-daemon") + bucket = str(uuid.uuid4()) server.succeed(f"mc mb minio/{bucket}") try: From 50aecffbf9f8d954b88841a1dad0efa8799a346d Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Date: Tue, 25 Nov 2025 15:49:17 -0500 Subject: [PATCH 20/24] test(s3-binary-cache-store): test profiles and provider chain (cherry picked from commit 71bdb33a36ae666b2248170699f31a0b048ff0e9) --- tests/nixos/s3-binary-cache-store.nix | 105 ++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/tests/nixos/s3-binary-cache-store.nix b/tests/nixos/s3-binary-cache-store.nix index 0e01d0a0134..154c1fb1b8b 100644 --- a/tests/nixos/s3-binary-cache-store.nix +++ b/tests/nixos/s3-binary-cache-store.nix @@ -789,6 +789,108 @@ in print(" ✓ Compressed log uploaded with multipart") + @setup_s3( + populate_bucket=[PKGS['A']], + profiles={ + "valid": {"access_key": ACCESS_KEY, "secret_key": SECRET_KEY}, + "invalid": {"access_key": "INVALIDKEY", "secret_key": "INVALIDSECRET"}, + } + ) + def test_profile_credentials(bucket): + """Test that profile-based credentials work without environment variables""" + print("\n=== Testing Profile-Based Credentials ===") + + store_url = make_s3_url(bucket, profile="valid") + + # Verify store info works with profile credentials (no env vars) + client.succeed(f"HOME=/root nix store info --store '{store_url}' >&2") + print(" ✓ nix store info works with profile credentials") + + # Verify we can copy from the store using profile + verify_packages_in_store(client, PKGS['A'], should_exist=False) + client.succeed(f"HOME=/root nix copy --no-check-sigs --from '{store_url}' {PKGS['A']}") + verify_packages_in_store(client, PKGS['A']) + print(" ✓ nix copy works with profile credentials") + + # Clean up the package we just copied so we can test invalid profile + client.succeed(f"nix store delete --ignore-liveness {PKGS['A']}") + verify_packages_in_store(client, PKGS['A'], should_exist=False) + + # Verify invalid profile fails when trying to copy + invalid_url = make_s3_url(bucket, profile="invalid") + client.fail(f"HOME=/root nix copy --no-check-sigs --from '{invalid_url}' {PKGS['A']} 2>&1") + print(" ✓ Invalid profile credentials correctly rejected") + + @setup_s3( + populate_bucket=[PKGS['A']], + profiles={ + "wrong": {"access_key": "WRONGKEY", "secret_key": "WRONGSECRET"}, + } + ) + def test_env_vars_precedence(bucket): + """Test that environment variables take precedence over profile credentials""" + print("\n=== Testing Environment Variables Precedence ===") + + # Use profile with wrong credentials, but provide correct creds via env vars + store_url = make_s3_url(bucket, profile="wrong") + + # Ensure package is not in client store + verify_packages_in_store(client, PKGS['A'], should_exist=False) + + # This should succeed because env vars (correct) override profile (wrong) + output = client.succeed( + f"HOME=/root {ENV_WITH_CREDS} nix copy --no-check-sigs --debug --from '{store_url}' {PKGS['A']} 2>&1" + ) + print(" ✓ nix copy succeeded with env vars overriding wrong profile") + + # Verify the credential chain shows Environment provider was added + if "Added AWS Environment Credential Provider" not in output: + print("Debug output:") + print(output) + raise Exception("Expected Environment provider to be added to chain") + print(" ✓ Environment provider added to credential chain") + + # Clean up the package so we can test again without env vars + client.succeed(f"nix store delete --ignore-liveness {PKGS['A']}") + verify_packages_in_store(client, PKGS['A'], should_exist=False) + + # Without env vars, same URL should fail (proving profile creds are actually wrong) + client.fail(f"HOME=/root nix copy --no-check-sigs --from '{store_url}' {PKGS['A']} 2>&1") + print(" ✓ Without env vars, wrong profile credentials correctly fail") + + @setup_s3( + populate_bucket=[PKGS['A']], + profiles={ + "testprofile": {"access_key": ACCESS_KEY, "secret_key": SECRET_KEY}, + } + ) + def test_credential_provider_chain(bucket): + """Test that debug logging shows which providers are added to the chain""" + print("\n=== Testing Credential Provider Chain Logging ===") + + store_url = make_s3_url(bucket, profile="testprofile") + + output = client.succeed( + f"HOME=/root nix store info --debug --store '{store_url}' 2>&1" + ) + + # For a named profile, we expect to see these providers in the chain + expected_providers = ["Environment", "Profile", "IMDS"] + for provider in expected_providers: + msg = f"Added AWS {provider} Credential Provider to chain for profile 'testprofile'" + if msg not in output: + print("Debug output:") + print(output) + raise Exception(f"Expected to find: {msg}") + print(f" ✓ {provider} provider added to chain") + + # SSO should be skipped (no SSO config for this profile) + if "Skipped AWS SSO Credential Provider for profile 'testprofile'" not in output: + print("Debug output:") + print(output) + raise Exception("Expected SSO provider to be skipped") + print(" ✓ SSO provider correctly skipped (not configured)") + # ============================================================================ # Main Test Execution # ============================================================================ @@ -822,6 +924,9 @@ in test_multipart_upload_basic() test_multipart_threshold() test_multipart_with_log_compression() + test_profile_credentials() + test_env_vars_precedence() + test_credential_provider_chain() print("\n" + "="*80) print("✓ All S3 Binary Cache Store Tests Passed!") From 65c7ec71cee6489b484aa5172ce0b4ee454f6215 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Mon, 15 Dec 2025 19:40:34 +0100 Subject: [PATCH 21/24] fix(libstore/aws-creds): respect AWS_PROFILE environment variable The SSO provider was unconditionally setting profile_name_override to the (potentially empty) profile string from the S3 URL. When profile was empty, this prevented the AWS CRT SDK from falling back to the AWS_PROFILE environment variable. Only set profile_name_override when a profile is explicitly specified in the URL, allowing the SDK's built-in AWS_PROFILE handling to work. (cherry picked from commit 453dbab1e8918a7f2462b5066f2c41968e3fe51f) --- src/libstore/aws-creds.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libstore/aws-creds.cc b/src/libstore/aws-creds.cc index 3e4176c8907..1ea37b12976 100644 --- a/src/libstore/aws-creds.cc +++ b/src/libstore/aws-creds.cc @@ -61,7 +61,9 @@ static std::shared_ptr createSSOProvider( options.bootstrap = bootstrap->GetUnderlyingHandle(); options.tls_ctx = tlsContext ? tlsContext->GetUnderlyingHandle() : nullptr; - options.profile_name_override = aws_byte_cursor_from_c_str(profileName.c_str()); + if (!profileName.empty()) { + options.profile_name_override = aws_byte_cursor_from_c_str(profileName.c_str()); + } // Create the SSO provider - will return nullptr if SSO isn't configured for this profile // createWrappedProvider handles nullptr gracefully From d297aeca2c375d202102b2d812e9f85de0eccb99 Mon Sep 17 00:00:00 2001 From: Bernardo Meurer Costa Date: Thu, 22 Jan 2026 21:01:24 +0000 Subject: [PATCH 22/24] feat(libstore/aws-creds): route AWS CRT logs through Nix logger Previously AWS CRT logs went directly to stderr via ApiHandle::InitializeLogging, causing log spam that didn't respect Nix's verbosity settings. This implements a custom aws_logger using the aws-c-common C API that: - Routes all AWS logs through nix::logger - Maps AWS log levels conservatively (ERROR/WARN -> lvlInfo) since the SDK treats expected conditions like missing IMDS as errors - Prefixes messages with (aws) for clarity - Respects Nix's verbosity flags (-v, -vv, etc.) (cherry picked from commit 3b8b764e29475d9677471fcf52c1eb2e09eaf723) --- src/libstore/aws-creds.cc | 115 +++++++++++++++++++++++--- tests/nixos/s3-binary-cache-store.nix | 42 ++++++++++ 2 files changed, 145 insertions(+), 12 deletions(-) diff --git a/src/libstore/aws-creds.cc b/src/libstore/aws-creds.cc index 1ea37b12976..9b0ddefdca8 100644 --- a/src/libstore/aws-creds.cc +++ b/src/libstore/aws-creds.cc @@ -13,6 +13,11 @@ // C library headers for SSO provider support # include +// C library headers for custom logging +# include + +# include + # include # include @@ -30,6 +35,101 @@ AwsAuthError::AwsAuthError(int errorCode) namespace { +/** + * Map AWS log level to Nix verbosity. + * AWS levels: AWS_LL_NONE=0, AWS_LL_FATAL=1, AWS_LL_ERROR=2, AWS_LL_WARN=3, + * AWS_LL_INFO=4, AWS_LL_DEBUG=5, AWS_LL_TRACE=6 + * + * We map very conservatively because the AWS SDK is extremely noisy. What AWS + * considers "info" includes low-level details like "Initializing epoll" and + * "Starting event-loop thread". What it considers "errors" includes expected + * conditions like missing ~/.aws/config or IMDS being unavailable on non-EC2. + * + * To avoid spamming users, we only show FATAL at default verbosity. Everything + * else requires -vvvvv (lvlDebug) or higher to see. + */ +static Verbosity awsLogLevelToVerbosity(enum aws_log_level level) +{ + switch (level) { + case AWS_LL_FATAL: + return lvlError; + case AWS_LL_ERROR: + case AWS_LL_WARN: + case AWS_LL_INFO: + return lvlDebug; + case AWS_LL_DEBUG: + case AWS_LL_TRACE: + return lvlVomit; + // AWS_LL_NONE and AWS_LL_COUNT are enum sentinels, not real log levels + case AWS_LL_NONE: + case AWS_LL_COUNT: + return lvlDebug; + } + unreachable(); +} + +/** + * Custom AWS logger that routes logs through Nix's logging infrastructure. + * + * The AWS CRT C++ wrapper (ApiHandle::InitializeLogging) only supports FILE* + * or filename-based logging. The underlying C library supports custom loggers + * via aws_logger struct with a vtable containing callback functions. + */ +static int nixAwsLoggerLog( + struct aws_logger * logger, enum aws_log_level logLevel, aws_log_subject_t subject, const char * format, ...) +{ + Verbosity nixLevel = awsLogLevelToVerbosity(logLevel); + if (nixLevel > verbosity) + return AWS_OP_SUCCESS; /* Bail out early to avoid formatting the message unnecessarily. */ + + va_list args; + va_start(args, format); + std::array buffer{}; + auto res = vsnprintf(buffer.data(), buffer.size(), format, args); + va_end(args); + if (res < 0) /* Skip garbage debug messages in case the SDK is busted. */ + return AWS_OP_SUCCESS; + + const char * subjectName = aws_log_subject_name(subject); + printMsgUsing(nix::logger, nixLevel, "(aws:%s) %s", subjectName ? subjectName : "unknown", chomp(buffer.data())); + return AWS_OP_SUCCESS; +} + +/** + * Get current log level for a subject - determines which messages will be logged. + * Must be consistent with awsLogLevelToVerbosity mapping. + */ +static aws_log_level nixAwsLoggerGetLevel(struct aws_logger * logger, aws_log_subject_t subject) +{ + // Map Nix verbosity back to AWS log level (inverse of awsLogLevelToVerbosity) + if (verbosity >= lvlVomit) + return AWS_LL_TRACE; + if (verbosity >= lvlDebug) + return AWS_LL_INFO; // error/warn/info are all mapped to lvlDebug + return AWS_LL_FATAL; +} + +static void initialiseAwsLogger() +{ + static std::once_flag initialised; /* aws_logger_set must only be called once */ + std::call_once(initialised, []() { + static aws_logger_vtable nixAwsLoggerVtable = { + .log = nixAwsLoggerLog, + .get_log_level = nixAwsLoggerGetLevel, + .clean_up = [](struct aws_logger *) {}, // No resources to clean up + .set_log_level = nullptr, + }; + + static aws_logger nixAwsLogger = { + .vtable = &nixAwsLoggerVtable, + .allocator = nullptr, + .p_impl = nullptr, + }; + + aws_logger_set(&nixAwsLogger); + }); +} + /** * Helper function to wrap a C credentials provider in the C++ interface. * This replicates the static s_CreateWrappedProvider from aws-crt-cpp. @@ -119,18 +219,9 @@ class AwsCredentialProviderImpl : public AwsCredentialProvider public: AwsCredentialProviderImpl() { - // Map Nix's verbosity to AWS CRT log level - Aws::Crt::LogLevel logLevel; - if (verbosity >= lvlVomit) { - logLevel = Aws::Crt::LogLevel::Trace; - } else if (verbosity >= lvlDebug) { - logLevel = Aws::Crt::LogLevel::Debug; - } else if (verbosity >= lvlChatty) { - logLevel = Aws::Crt::LogLevel::Info; - } else { - logLevel = Aws::Crt::LogLevel::Warn; - } - apiHandle.InitializeLogging(logLevel, stderr); + // Install custom logger that routes AWS CRT logs through Nix's logging infrastructure. + // This ensures AWS logs respect Nix's verbosity settings and are formatted consistently. + initialiseAwsLogger(); // Create a shared TLS context for SSO (required for HTTPS connections) auto allocator = Aws::Crt::ApiAllocator(); diff --git a/tests/nixos/s3-binary-cache-store.nix b/tests/nixos/s3-binary-cache-store.nix index 154c1fb1b8b..65a41fc82a9 100644 --- a/tests/nixos/s3-binary-cache-store.nix +++ b/tests/nixos/s3-binary-cache-store.nix @@ -229,6 +229,47 @@ in print("✓ Credential provider created once and cached") + @setup_s3() + def test_aws_log_integration(bucket): + """Test that AWS SDK logs are properly routed through Nix logger""" + print("\n=== Testing AWS Log Integration ===") + + store_url = make_s3_url(bucket) + + # With default verbosity, AWS noise should NOT appear + # All AWS messages are demoted to lvlDebug or lvlVomit + output_default = server.succeed( + f"{ENV_WITH_CREDS} nix copy --to '{store_url}' {PKGS['A']} 2>&1" + ) + + if "(aws:" in output_default: + print("Output at default verbosity:") + print(output_default) + raise Exception("Found AWS noise at default verbosity") + + print(" ✓ Default verbosity filters AWS noise") + + # With --debug (lvlDebug), we should see AWS messages with (aws:subject) prefix + output_debug = server.succeed( + f"{ENV_WITH_CREDS} nix copy --debug --to '{store_url}' {PKGS['B']} 2>&1" + ) + + # Check for the (aws:subject) prefix format + if "(aws:" not in output_debug: + print("Output at --debug verbosity:") + print(output_debug) + raise Exception("Expected to see (aws:subject) prefix in debug output") + + print(" ✓ Debug output shows AWS messages with (aws:subject) prefix") + + # Should also see Nix's own credential provider creation message + if "creating new AWS credential provider" not in output_debug: + print("Debug output:") + print(output_debug) + raise Exception("Expected to see credential provider creation at debug level") + + print(" ✓ Debug verbosity shows credential provider messages") + @setup_s3(populate_bucket=[PKGS['A']]) def test_fetchurl_basic(bucket): """Test builtins.fetchurl works with s3:// URLs""" @@ -909,6 +950,7 @@ in # Run tests (each gets isolated bucket via decorator) test_credential_caching() + test_aws_log_integration() test_fetchurl_basic() test_error_message_formatting() test_fork_credential_preresolution() From e96a3f7a731b038c78b4b532b3f3366f4ae39c40 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Sun, 25 Jan 2026 19:26:11 +0300 Subject: [PATCH 23/24] tests/nixos/s3-binary-cache-store: Drop superfluous prints As requested in review. (cherry picked from commit e3b788b4ca839987937f1a348bdbb5772c40c798) --- tests/nixos/s3-binary-cache-store.nix | 77 --------------------------- 1 file changed, 77 deletions(-) diff --git a/tests/nixos/s3-binary-cache-store.nix b/tests/nixos/s3-binary-cache-store.nix index 65a41fc82a9..5804057487d 100644 --- a/tests/nixos/s3-binary-cache-store.nix +++ b/tests/nixos/s3-binary-cache-store.nix @@ -227,8 +227,6 @@ in "Credential provider caching failed" ) - print("✓ Credential provider created once and cached") - @setup_s3() def test_aws_log_integration(bucket): """Test that AWS SDK logs are properly routed through Nix logger""" @@ -247,8 +245,6 @@ in print(output_default) raise Exception("Found AWS noise at default verbosity") - print(" ✓ Default verbosity filters AWS noise") - # With --debug (lvlDebug), we should see AWS messages with (aws:subject) prefix output_debug = server.succeed( f"{ENV_WITH_CREDS} nix copy --debug --to '{store_url}' {PKGS['B']} 2>&1" @@ -260,16 +256,12 @@ in print(output_debug) raise Exception("Expected to see (aws:subject) prefix in debug output") - print(" ✓ Debug output shows AWS messages with (aws:subject) prefix") - # Should also see Nix's own credential provider creation message if "creating new AWS credential provider" not in output_debug: print("Debug output:") print(output_debug) raise Exception("Expected to see credential provider creation at debug level") - print(" ✓ Debug verbosity shows credential provider messages") - @setup_s3(populate_bucket=[PKGS['A']]) def test_fetchurl_basic(bucket): """Test builtins.fetchurl works with s3:// URLs""" @@ -284,8 +276,6 @@ in f"'builtins.fetchurl {{ name = \"foo\"; url = \"{cache_info_url}\"; }}'" ) - print("✓ builtins.fetchurl works with s3:// URLs") - @setup_s3() def test_error_message_formatting(bucket): """Verify error messages display URLs correctly""" @@ -304,8 +294,6 @@ in print(error_msg) raise Exception("Error message formatting failed - should show actual URL, not %s placeholder") - print("✓ Error messages format URLs correctly") - @setup_s3(populate_bucket=[PKGS['A']]) def test_fork_credential_preresolution(bucket): """Test credential pre-resolution in forked processes""" @@ -345,8 +333,6 @@ in print(output) raise Exception("Expected to find FileTransfer creation in forked process") - print(" ✓ Forked process creates fresh FileTransfer") - # Verify pre-resolution in parent required_messages = [ "Pre-resolving AWS credentials for S3 URL in builtin:fetchurl", @@ -359,8 +345,6 @@ in print(output) raise Exception(f"Missing expected message: {msg}") - print(" ✓ Parent pre-resolves credentials") - # Verify child uses pre-resolved credentials if "Using pre-resolved AWS credentials from parent process" not in output: print("Debug output:") @@ -384,8 +368,6 @@ in print(output) raise Exception(f"Child process (pid={child_pid}) should NOT create new credential providers") - print(" ✓ Child uses pre-resolved credentials (no new providers)") - @setup_s3(populate_bucket=[PKGS['A'], PKGS['B'], PKGS['C']]) def test_store_operations(bucket): """Test nix store info and copy operations""" @@ -403,8 +385,6 @@ in if not store_info.get("url"): raise Exception("Store should have a URL") - print(f" ✓ Store URL: {store_info['url']}") - # Test copy from store verify_packages_in_store(client, PKGS['A'], should_exist=False) @@ -422,9 +402,6 @@ in verify_packages_in_store(client, [PKGS['A'], PKGS['B'], PKGS['C']]) - print(" ✓ nix copy works") - print(" ✓ Credentials cached on client") - @setup_s3(populate_bucket=[PKGS['A'], PKGS['B']], public=True) def test_public_bucket_operations(bucket): """Test store operations on public bucket without credentials""" @@ -434,7 +411,6 @@ in # Verify store info works without credentials client.succeed(f"nix store info --store '{store_url}' >&2") - print(" ✓ nix store info works without credentials") # Get and validate store info JSON info_json = client.succeed(f"nix store info --json --store '{store_url}'") @@ -443,8 +419,6 @@ in if not store_info.get("url"): raise Exception("Store should have a URL") - print(f" ✓ Store URL: {store_info['url']}") - # Verify packages are not yet in client store verify_packages_in_store(client, [PKGS['A'], PKGS['B']], should_exist=False) @@ -457,8 +431,6 @@ in # Verify packages were copied successfully verify_packages_in_store(client, [PKGS['A'], PKGS['B']]) - print(" ✓ nix copy from public bucket works without credentials") - @setup_s3(populate_bucket=[PKGS['A']]) def test_url_format_variations(bucket): """Test different S3 URL parameter combinations""" @@ -467,12 +439,10 @@ in # Test parameter order variation (region before endpoint) url1 = f"s3://{bucket}?region={REGION}&endpoint={ENDPOINT}" client.succeed(f"{ENV_WITH_CREDS} nix store info --store '{url1}' >&2") - print(" ✓ Parameter order: region before endpoint works") # Test parameter order variation (endpoint before region) url2 = f"s3://{bucket}?endpoint={ENDPOINT}®ion={REGION}" client.succeed(f"{ENV_WITH_CREDS} nix store info --store '{url2}' >&2") - print(" ✓ Parameter order: endpoint before region works") @setup_s3(populate_bucket=[PKGS['A']]) def test_concurrent_fetches(bucket): @@ -526,9 +496,6 @@ in providers_created = output.count("creating new AWS credential provider") transfers_created = output.count("builtin:fetchurl creating fresh FileTransfer instance") - print(f" ✓ {providers_created} credential providers created") - print(f" ✓ {transfers_created} FileTransfer instances created") - if transfers_created != 5: print("Debug output:") print(output) @@ -554,14 +521,10 @@ in pkg_hash = get_package_hash(PKGS['B']) verify_content_encoding(server, bucket, f"{pkg_hash}.narinfo", "gzip") - print(" ✓ .narinfo has Content-Encoding: gzip") - # Verify client can download and decompress client.succeed(f"{ENV_WITH_CREDS} nix copy --from '{store_url}' --no-check-sigs {PKGS['B']}") verify_packages_in_store(client, PKGS['B']) - print(" ✓ Client decompressed .narinfo successfully") - @setup_s3() def test_compression_mixed(bucket): """Test mixed compression (narinfo=xz, ls=gzip)""" @@ -578,18 +541,14 @@ in # Verify .narinfo has xz compression verify_content_encoding(server, bucket, f"{pkg_hash}.narinfo", "xz") - print(" ✓ .narinfo has Content-Encoding: xz") # Verify .ls has gzip compression verify_content_encoding(server, bucket, f"{pkg_hash}.ls", "gzip") - print(" ✓ .ls has Content-Encoding: gzip") # Verify client can download with mixed compression client.succeed(f"{ENV_WITH_CREDS} nix copy --from '{store_url}' --no-check-sigs {PKGS['C']}") verify_packages_in_store(client, PKGS['C']) - print(" ✓ Client downloaded package with mixed compression") - @setup_s3() def test_compression_disabled(bucket): """Verify no compression by default""" @@ -601,8 +560,6 @@ in pkg_hash = get_package_hash(PKGS['A']) verify_no_compression(server, bucket, f"{pkg_hash}.narinfo") - print(" ✓ No compression applied by default") - @setup_s3() def test_nix_prefetch_url(bucket): """Test that nix-prefetch-url retrieves actual file content from S3, not empty files (issue #8862)""" @@ -622,8 +579,6 @@ in "nix hash file --type sha256 --base32 /tmp/test-file.txt" ).strip() - print(f" ✓ Uploaded test file to S3 ({test_file_size} bytes)") - # Use nix-prefetch-url to download from S3 s3_url = make_s3_url(bucket, path="/test-file.txt") @@ -643,8 +598,6 @@ in f"Hash mismatch: expected {expected_hash}, got {prefetch_hash}" ) - print(" ✓ nix-prefetch-url completed with correct hash") - # Verify the downloaded file is NOT empty (the bug in #8862) file_size = int(client.succeed(f"stat -c %s {store_path}").strip()) @@ -656,16 +609,12 @@ in f"File size mismatch: expected {test_file_size}, got {file_size}" ) - print(f" ✓ File has correct size ({file_size} bytes, not empty)") - # Verify actual content matches by comparing hashes instead of printing entire file downloaded_hash = client.succeed(f"nix hash file --type sha256 --base32 {store_path}").strip() if downloaded_hash != expected_hash: raise Exception(f"Content hash mismatch: expected {expected_hash}, got {downloaded_hash}") - print(" ✓ File content verified correct (hash matches)") - @setup_s3(populate_bucket=[PKGS['A']], versioned=True) def test_versioned_urls(bucket): """Test that versionId parameter is accepted in S3 URLs""" @@ -679,7 +628,6 @@ in f"{ENV_WITH_CREDS} nix eval --impure --expr " f"'builtins.fetchurl {{ name = \"cache-info\"; url = \"{cache_info_url}\"; }}'" ) - print(" ✓ Fetch without versionId works") # List versions to get a version ID # MinIO output format: [timestamp] size tier versionId versionNumber method filename @@ -693,8 +641,6 @@ in raise Exception("Could not extract version ID from MinIO output") version_id = version_match.group(1) - print(f" ✓ Found version ID: {version_id}") - # Version ID should not be "null" since versioning was enabled before upload if version_id == "null": raise Exception("Version ID is 'null' - versioning may not be working correctly") @@ -705,7 +651,6 @@ in f"{ENV_WITH_CREDS} nix eval --impure --expr " f"'builtins.fetchurl {{ name = \"cache-info-versioned\"; url = \"{versioned_url}\"; }}'" ) - print(" ✓ Fetch with versionId parameter works") @setup_s3() def test_multipart_upload_basic(bucket): @@ -741,13 +686,9 @@ in print(output) raise Exception(f"Expected '{expected_msg}' in output") - print(f" ✓ Multipart upload used with {expected_parts} parts") - client.succeed(f"{ENV_WITH_CREDS} nix copy --from '{store_url}' {large_pkg} --no-check-sigs") verify_packages_in_store(client, large_pkg, should_exist=True) - print(" ✓ Large file downloaded and verified") - @setup_s3() def test_multipart_threshold(bucket): """Test that files below threshold use regular upload""" @@ -770,13 +711,9 @@ in if "using S3 regular upload" not in output: raise Exception("Expected regular upload to be used") - print(" ✓ Regular upload used for file below threshold") - client.succeed(f"{ENV_WITH_CREDS} nix copy --no-check-sigs --from '{store_url}' {PKGS['A']}") verify_packages_in_store(client, PKGS['A'], should_exist=True) - print(" ✓ Small file uploaded and verified") - @setup_s3() def test_multipart_with_log_compression(bucket): """Test multipart upload with compressed build logs""" @@ -828,8 +765,6 @@ in print(output) raise Exception("Expected multipart completion message") - print(" ✓ Compressed log uploaded with multipart") - @setup_s3( populate_bucket=[PKGS['A']], profiles={ @@ -845,13 +780,11 @@ in # Verify store info works with profile credentials (no env vars) client.succeed(f"HOME=/root nix store info --store '{store_url}' >&2") - print(" ✓ nix store info works with profile credentials") # Verify we can copy from the store using profile verify_packages_in_store(client, PKGS['A'], should_exist=False) client.succeed(f"HOME=/root nix copy --no-check-sigs --from '{store_url}' {PKGS['A']}") verify_packages_in_store(client, PKGS['A']) - print(" ✓ nix copy works with profile credentials") # Clean up the package we just copied so we can test invalid profile client.succeed(f"nix store delete --ignore-liveness {PKGS['A']}") @@ -860,7 +793,6 @@ in # Verify invalid profile fails when trying to copy invalid_url = make_s3_url(bucket, profile="invalid") client.fail(f"HOME=/root nix copy --no-check-sigs --from '{invalid_url}' {PKGS['A']} 2>&1") - print(" ✓ Invalid profile credentials correctly rejected") @setup_s3( populate_bucket=[PKGS['A']], @@ -882,14 +814,12 @@ in output = client.succeed( f"HOME=/root {ENV_WITH_CREDS} nix copy --no-check-sigs --debug --from '{store_url}' {PKGS['A']} 2>&1" ) - print(" ✓ nix copy succeeded with env vars overriding wrong profile") # Verify the credential chain shows Environment provider was added if "Added AWS Environment Credential Provider" not in output: print("Debug output:") print(output) raise Exception("Expected Environment provider to be added to chain") - print(" ✓ Environment provider added to credential chain") # Clean up the package so we can test again without env vars client.succeed(f"nix store delete --ignore-liveness {PKGS['A']}") @@ -897,7 +827,6 @@ in # Without env vars, same URL should fail (proving profile creds are actually wrong) client.fail(f"HOME=/root nix copy --no-check-sigs --from '{store_url}' {PKGS['A']} 2>&1") - print(" ✓ Without env vars, wrong profile credentials correctly fail") @setup_s3( populate_bucket=[PKGS['A']], @@ -923,14 +852,12 @@ in print("Debug output:") print(output) raise Exception(f"Expected to find: {msg}") - print(f" ✓ {provider} provider added to chain") # SSO should be skipped (no SSO config for this profile) if "Skipped AWS SSO Credential Provider for profile 'testprofile'" not in output: print("Debug output:") print(output) raise Exception("Expected SSO provider to be skipped") - print(" ✓ SSO provider correctly skipped (not configured)") # ============================================================================ # Main Test Execution @@ -969,9 +896,5 @@ in test_profile_credentials() test_env_vars_precedence() test_credential_provider_chain() - - print("\n" + "="*80) - print("✓ All S3 Binary Cache Store Tests Passed!") - print("="*80) ''; } From 5f96c8a6557f418026be4e51764479dd2c9aace8 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 2 Feb 2026 21:48:24 +0100 Subject: [PATCH 24/24] Revert "buildPathsWithResults: don't report cancelled goals as failures" This reverts commit 09884e2c1ac213fd34321739168e6b4891d1131d. It breaks `buildPathsWithResults` returning cancelled builds, which is needed by `nix flake check` and `nix build` to report cancelled builds. --- src/libstore/build/entry-points.cc | 8 +- tests/functional/build.sh | 86 ++------------------- tests/functional/cancelled-builds/flake.nix | 64 --------------- 3 files changed, 6 insertions(+), 152 deletions(-) delete mode 100644 tests/functional/cancelled-builds/flake.nix diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 04b16f5a8b1..4bbd4c8f059 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -63,18 +63,12 @@ std::vector Store::buildPathsWithResults( std::vector results; results.reserve(state.size()); - 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; + for (auto & [req, goalPtr] : state) results.emplace_back( KeyedBuildResult{ goalPtr->buildResult, /* .path = */ req, }); - } return results; } diff --git a/tests/functional/build.sh b/tests/functional/build.sh index 8039b1c731c..6a4c90a4378 100755 --- a/tests/functional/build.sh +++ b/tests/functional/build.sh @@ -158,21 +158,11 @@ 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 -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="$(nix build -f fod-failing.nix -j1 -L 2>&1)" && status=0 || status=$? +test "$status" = 1 +# Only the hash mismatch error for the first failing goal (x1). +# The other goals (x2, x3, x4) are cancelled and not reported as failures. +test "$(<<<"$out" grep -cE '^error:')" = 1 <<<"$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'" @@ -212,69 +202,3 @@ 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" - # When using a separate test store, we need sandbox-paths to access - # the system store (where bash/coreutils live). On NixOS, the test - # uses the system store directly, so this isn't needed (and would - # conflict with input paths). - sandboxPathsArg=() - if ! isTestOnNixOS; then - sandboxPathsArg=(--option sandbox-paths "/nix/store") - fi - out="$(nix flake check ./cancelled-builds --impure -L -j2 \ - --option sandbox true \ - "${sandboxPathsArg[@]}" \ - --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" - sandboxPathsArg=() - if ! isTestOnNixOS; then - sandboxPathsArg=(--option sandbox-paths "/nix/store") - fi - system=$(nix eval --raw --impure --expr builtins.currentSystem) - out="$(nix build --impure -L -j2 \ - --option sandbox true \ - "${sandboxPathsArg[@]}" \ - --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 deleted file mode 100644 index 0b8bf1ca5d8..00000000000 --- a/tests/functional/cancelled-builds/flake.nix +++ /dev/null @@ -1,64 +0,0 @@ -# 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 - ''; - }; - }; - }; -}