Skip to content

forceDerivation(): Wait for async path write after forcing value #176

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 2 commits into from
Aug 19, 2025
Merged

forceDerivation(): Wait for async path write after forcing value #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d52893
forceDerivation(): Wait for async path write after forcing value
edolstra Aug 18, 2025
07ed0a2
Add test
edolstra Aug 19, 2025
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
2 changes: 1 addition & 1 deletion src/libexpr/eval-cache.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,11 +705,11 @@ StorePath AttrCursor::forceDerivation()
auto aDrvPath = getAttr(root->state.sDrvPath);
auto drvPath = root->state.store->parseStorePath(aDrvPath->getString());
drvPath.requireDerivation();
root->state.waitForPath(drvPath);
Copy link
Member

@colemickens colemickens Aug 18, 2025

Choose a reason for hiding this comment

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

I'm not really qualified to review this, but I'm curious - why did you move this line and not duplicate it?

Is there no chance that requireDerivation might also require waiting, and would otherwise avoid having to forceValue if you had waited?

Copy link
Collaborator Author

@edolstra edolstra Aug 19, 2025

Choose a reason for hiding this comment

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

See my other comment.

Copy link
Member

@cole-h cole-h Aug 18, 2025

Choose a reason for hiding this comment

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

What happens right now, with the waitForPath here? (I guess my real question is: what does this really fix? It seems like waitForPath is now being called in less cases than before, which may be fine and correct but seems weird for "calling it more times than necessary" to cause issues...)

Copy link
Member

@colemickens colemickens Aug 18, 2025

Choose a reason for hiding this comment

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

I understood it as the critical bit was needing to make sure there's a wait after calling aDrvPath->forceValue();. I'm likely hitting the case in the comment - the nix eval cache is shared between CI runs, and occasionally I manually run GC on the box.

Copy link
Collaborator Author

@edolstra edolstra Aug 19, 2025

Choose a reason for hiding this comment

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

The original waitForPath() was basically pointless, because it waits before the attribute has been evaluated. So at that point, the asynchronous creation of store paths (the .drv file and its dependencies) hasn't even started yet.

In the common case where the attribute is cached and the corresponding .drv file exists in the store, there is no need to wait for anything. And if the .drv file has been GC'ed, we need to evaluate the attribute to recreate it and then wait.

the nix eval cache is shared between CI runs, and occasionally I manually run GC on the box.

Right, this bug triggers if the eval cache gets out of sync with what's in the store.

if (!root->state.store->isValidPath(drvPath) && !settings.readOnlyMode) {
/* The eval cache contains 'drvPath', but the actual path has
been garbage-collected. So force it to be regenerated. */
aDrvPath->forceValue();
root->state.waitForPath(drvPath);
if (!root->state.store->isValidPath(drvPath))
throw Error(
"don't know how to recreate store derivation '%s'!", root->state.store->printStorePath(drvPath));
Expand Down
8 changes: 8 additions & 0 deletions tests/functional/flakes/eval-cache.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,11 @@ nix build --no-link "$flake1Dir#stack-depth"
expect 1 nix build "$flake1Dir#ifd" --option allow-import-from-derivation false 2>&1 \
| grepQuiet 'error: cannot build .* during evaluation because the option '\''allow-import-from-derivation'\'' is disabled'
nix build --no-link "$flake1Dir#ifd"

# Test that a store derivation is recreated when it has been deleted
# but the corresponding attribute is still cached.
if ! isTestOnNixOS; then
nix build "$flake1Dir#drv"
clearStore
nix build "$flake1Dir#drv"
fi
Loading