Skip to content

Add missing temp roots #373

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
grahamc merged 2 commits into from
Feb 27, 2026
Merged

Add missing temp roots #373

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
97ea761
Add missing temproots for cached sources and existing derivations
xokdvium Feb 14, 2026
618c4df
AsyncPathWriter: Register a temp root before creating the path
edolstra Feb 27, 2026
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
6 changes: 6 additions & 0 deletions src/libfetchers/fetch-to-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ std::pair<StorePath, Hash> fetchToStore2(
auto hash = Hash::parseSRI(fetchers::getStrAttr(*res, "hash"));
auto storePath =
store.makeFixedOutputPathFromCA(name, ContentAddressWithReferences::fromParts(method, hash, {}));

/* Add a temproot before the call to isValidPath to prevent accidental GC in case the
input is cached. Note that this must be done before to avoid races. */
if (mode != FetchMode::DryRun)
store.addTempRoot(storePath);

if (mode == FetchMode::DryRun || store.maybeQueryPathInfo(storePath)) {
debug(
"source path '%s' cache hit in '%s' (hash '%s')",
Expand Down
1 change: 1 addition & 0 deletions src/libstore/async-path-writer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ struct AsyncPathWriterImpl : AsyncPathWriter

for (auto & item : items) {
StringSource source(item.contents);
store->addTempRoot(item.storePath);
auto storePath = store->addToStoreFromDump(
source,
Comment on lines 164 to 165

@xokdvium xokdvium Mar 3, 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.

Btw I think that in this case the remote side already adds temp roots https://github.com/NixOS/nix/blob/fa1a370c6fb827883d1d34317f31f948a1f44b7b/src/libstore/local-store.cc#L1210

It's more of the pattern of isValidPath that needs to have a temp root added first.

item.storePath.name(),
Expand Down
6 changes: 6 additions & 0 deletions src/libstore/derivations.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,12 @@ Store::writeDerivation(const Derivation & drv, RepairFlag repair, std::shared_pt
{
auto [suffix, contents, references, path] = infoForDerivation(*this, drv);

/* In case the derivation is already valid, we bail out early since that's
faster. But we need to make sure that the derivation has a corresponding
temproot. It is added by the remote in addToStoreFromDump, but we'd like
to avoid sending a lot of drv contents to the daemon. */
addTempRoot(path);

if (isValidPath(path) && !repair)
return path;

Expand Down