Skip to content

Input::getAccessorUnchecked(): Wrap fetches in a path lock #410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
edolstra merged 1 commit into from
Apr 7, 2026
Merged

Input::getAccessorUnchecked(): Wrap fetches in a path lock #410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/libfetchers/fetchers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
#include "nix/util/url.hh"
#include "nix/util/forwarding-source-accessor.hh"
#include "nix/util/archive.hh"
#include "nix/util/users.hh"
#include "nix/store/pathlocks.hh"
#include "nix/util/environment-variables.hh"

#include <nlohmann/json.hpp>

Expand Down Expand Up @@ -367,6 +370,18 @@ std::pair<ref<SourceAccessor>, Input> Input::getAccessorUnchecked(const Settings
return {accessor, result};
};

/* Acquire a path lock on this input. Note that fetching the same input in parallel is supposed to be safe (it's up
* to the fetchers to guarantee this), so this is merely intended to avoid work duplication. */
auto lockFilePath =
getCacheDir() / "fetcher-locks"
/ hashString(HashAlgorithm::SHA256, attrsToJSON(toAttrs()).dump()).to_string(HashFormat::Base16, false);
Comment on lines +375 to +377

@coderabbitai coderabbitai Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Lock coordination may fail across different user contexts.

getCacheDir() returns user-specific paths based on NIX_CACHE_HOME, XDG_CACHE_HOME, or ~/.cache/nix. If the Nix daemon runs as root and a client runs as a regular user, they'll have different cache directories and won't see each other's locks.

Per the PR description, this is intended for parallel nix-eval-jobs instances. If those run as the same user, this works. However, this limitation should be documented or considered if daemon-client coordination is ever expected.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/libfetchers/fetchers.cc` around lines 375 - 377, The lock path built in
fetchers.cc uses getCacheDir() which is per-user, so lock coordination across
different user contexts (daemon vs client) can fail; update the implementation
to either (a) use a shared, daemon-visible directory (e.g. respect a configured
global lock directory such as NIX_STATE_DIR or an explicit env var like
NIX_FETCHER_LOCK_DIR) when available, or (b) document the user-scoped limitation
clearly; modify the code around getCacheDir(), "fetcher-locks", and the
hashString(attrsToJSON(toAttrs()).dump()) construction to read and prefer the
global/configurable lock dir before falling back to getCacheDir() so
daemon-client coordination will see the same lock files.

std::filesystem::create_directories(lockFilePath.parent_path());
PathLocks lock(
{lockFilePath.string()}, fmt("waiting for another Nix process to finish fetching input '%s'...", to_string()));

if (getEnv("_NIX_TEST_CONCURRENT_FETCHES"))
std::this_thread::sleep_for(std::chrono::seconds(1));

/* See if the input is in the cache of the fetcher. */
try {
if (auto res = scheme->getAccessor(settings, store, *this, true))
Expand Down
14 changes: 14 additions & 0 deletions tests/functional/tarball.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,17 @@ path="$(nix flake prefetch --refresh --json "tarball+file://$TEST_ROOT/tar.tar"
[[ $(cat "$path/a/b/xyzzy") = xyzzy ]]
[[ $(cat "$path/a/b/foo") = foo ]]
[[ $(cat "$path/bla") = abc ]]

# Test that concurrent invocations of Nix will fetch the tarball only once.
rm -rf "$TEST_HOME/.cache"
store="$TEST_ROOT/prefetch-store"
nix-store --store "$store" --init # needed because concurrent creation of the store can give SQLite errors
_NIX_TEST_CONCURRENT_FETCHES=1 _NIX_FORCE_HTTP=1 nix flake prefetch --store "$store" -v "tarball+file://$TEST_ROOT/tar.tar" 2> "$TEST_ROOT/log1" &
pid1="$!"
_NIX_TEST_CONCURRENT_FETCHES=1 _NIX_FORCE_HTTP=1 nix flake prefetch --store "$store" -v "tarball+file://$TEST_ROOT/tar.tar" 2> "$TEST_ROOT/log2" &
pid2="$!"
wait "$pid1"
wait "$pid2"
[[ $(cat "$TEST_ROOT/log1" "$TEST_ROOT/log2" | grep -c "Download.*to") -eq 2 ]]
[[ $(cat "$TEST_ROOT/log1" "$TEST_ROOT/log2" | grep -c "downloading.*tar.tar") -eq 1 ]]
[[ $(cat "$TEST_ROOT/log1" "$TEST_ROOT/log2" | grep -c "waiting for another Nix process to finish fetching input") -eq 1 ]]
Comment on lines +119 to +131

@coderabbitai coderabbitai Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Test does not verify process exit status before assertions.

The wait commands capture exit status but it's not checked. If one process fails, the test may still pass the grep assertions (e.g., if only one process ran successfully and produced the expected log output).

🛠️ Suggested fix to check exit status 
 wait "$pid1"
+rc1=$?
 wait "$pid2"
+rc2=$?
+[[ $rc1 -eq 0 ]] || { echo "First prefetch failed"; exit 1; }
+[[ $rc2 -eq 0 ]] || { echo "Second prefetch failed"; exit 1; }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Test that concurrent invocations of Nix will fetch the tarball only once.
rm -rf "$TEST_HOME/.cache"
store="$TEST_ROOT/prefetch-store"
nix-store --store "$store" --init # needed because concurrent creation of the store can give SQLite errors
_NIX_TEST_CONCURRENT_FETCHES=1 _NIX_FORCE_HTTP=1 nix flake prefetch --store "$store" -v "tarball+file://$TEST_ROOT/tar.tar" 2> "$TEST_ROOT/log1" &
pid1="$!"
_NIX_TEST_CONCURRENT_FETCHES=1 _NIX_FORCE_HTTP=1 nix flake prefetch --store "$store" -v "tarball+file://$TEST_ROOT/tar.tar" 2> "$TEST_ROOT/log2" &
pid2="$!"
wait "$pid1"
wait "$pid2"
[[ $(cat "$TEST_ROOT/log1" "$TEST_ROOT/log2" | grep -c "Download.*to") -eq 2 ]]
[[ $(cat "$TEST_ROOT/log1" "$TEST_ROOT/log2" | grep -c "downloading.*tar.tar") -eq 1 ]]
[[ $(cat "$TEST_ROOT/log1" "$TEST_ROOT/log2" | grep -c "waiting for another Nix process to finish fetching input") -eq 1 ]]
# Test that concurrent invocations of Nix will fetch the tarball only once.
rm -rf "$TEST_HOME/.cache"
store="$TEST_ROOT/prefetch-store"
nix-store --store "$store" --init # needed because concurrent creation of the store can give SQLite errors
_NIX_TEST_CONCURRENT_FETCHES=1 _NIX_FORCE_HTTP=1 nix flake prefetch --store "$store" -v "tarball+file://$TEST_ROOT/tar.tar" 2> "$TEST_ROOT/log1" &
pid1="$!"
_NIX_TEST_CONCURRENT_FETCHES=1 _NIX_FORCE_HTTP=1 nix flake prefetch --store "$store" -v "tarball+file://$TEST_ROOT/tar.tar" 2> "$TEST_ROOT/log2" &
pid2="$!"
wait "$pid1"
rc1=$?
wait "$pid2"
rc2=$?
[[ $rc1 -eq 0 ]] || { echo "First prefetch failed"; exit 1; }
[[ $rc2 -eq 0 ]] || { echo "Second prefetch failed"; exit 1; }
[[ $(cat "$TEST_ROOT/log1" "$TEST_ROOT/log2" | grep -c "Download.*to") -eq 2 ]]
[[ $(cat "$TEST_ROOT/log1" "$TEST_ROOT/log2" | grep -c "downloading.*tar.tar") -eq 1 ]]
[[ $(cat "$TEST_ROOT/log1" "$TEST_ROOT/log2" | grep -c "waiting for another Nix process to finish fetching input") -eq 1 ]]
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/functional/tarball.sh` around lines 119 - 131, The test launches two
background prefetch processes (pid1 and pid2) and currently calls wait without
checking their exit codes; capture each wait's exit status (e.g., wait "$pid1";
status1=$? and wait "$pid2"; status2=$?) and assert both status1 and status2 are
zero before performing the grep assertions so a failing process causes the test
to fail; update the test around the wait calls (referencing pid1, pid2, and the
wait invocations) to record and check these exit statuses and fail the test if
either is non-zero.

Loading