nixos/nix
1
1
Fork 0
mirror of https://github.com/NixOS/nix.git synced 2025-12-22 17:01:08 +01:00
Code Activity

libstore: simplify registerValidPaths by removing redundant checkInvariants loop

Browse source 
The separate checkInvariants loop after addValidPath was added in 2014
(d210cdc43) to work around an assertion failure:

  nix-store: derivations.cc:242: Assertion 'store.isValidPath(i->first)' failed.

At that time, hashDerivationModulo() contained assert(store.isValidPath(...))
which required input derivations to be registered as valid in the database
before computing their hash. The workaround was to:
1. Call addValidPath with checkOutputs=false
2. Add all references to the database
3. Run checkInvariants in a separate loop after paths were valid

In 2020 (bccff827d), the isValidPath assertion was removed to fix a
deadlock in IFD through the daemon (issue #4235). The fix changed
hashDerivationModulo to use readInvalidDerivation, which reads directly
from the filesystem without requiring database validity.

This made the separate checkInvariants loop unnecessary, but nobody
noticed the code could be simplified. The comment "We can't do this in
addValidPath() above, because the references might not be valid yet"
became stale.

Now we simply call addValidPath() with the default checkOutputs=true,
which runs checkInvariants internally using the already-parsed
derivation. This commit eliminates the separate loop over derivations.
This commit is contained in:
Jörg Thalheim 2025-12-18 09:28:12 +01:00
parent cccfa385e6
commit 1f739961e5
2 changed files with 5 additions and 29 deletions
Download patch file Download diff file Expand all files Collapse all files

3
src/libstore/include/nix/store/local-store.hh
View file

@ -420,8 +420,7 @@ private:
uint64_t queryValidPathId(State & state, const StorePath & path); uint64_t queryValidPathId(State & state, const StorePath & path);
uint64_t uint64_t addValidPath(State & state, const ValidPathInfo & info);
addValidPath(State & state, const ValidPathInfo & info, bool checkOutputs = true, Derivation * drv = nullptr);
void invalidatePath(State & state, const StorePath & path); void invalidatePath(State & state, const StorePath & path);

31
src/libstore/local-store.cc
View file

@ -647,7 +647,7 @@ void LocalStore::cacheDrvOutputMapping(
[&]() { state.stmts->AddDerivationOutput.use()(deriver)(outputName) (printStorePath(output)).exec(); }); [&]() { state.stmts->AddDerivationOutput.use()(deriver)(outputName) (printStorePath(output)).exec(); });
} }
uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info, bool checkOutputs, Derivation * drv) uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info)
{ {
if (info.ca.has_value() && !info.isContentAddressed(*this)) if (info.ca.has_value() && !info.isContentAddressed(*this))
throw Error( throw Error(
@ -675,8 +675,7 @@ uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info, boo
derivations). Note that if this throws an error, then the derivations). Note that if this throws an error, then the
DB transaction is rolled back, so the path validity DB transaction is rolled back, so the path validity
registration above is undone. */ registration above is undone. */
if (checkOutputs) parsedDrv.checkInvariants(*this, info.path);
parsedDrv.checkInvariants(*this, info.path);
for (auto & i : parsedDrv.outputsAndOptPaths(*this)) { for (auto & i : parsedDrv.outputsAndOptPaths(*this)) {
/* Floating CA derivations have indeterminate output paths until /* Floating CA derivations have indeterminate output paths until
@ -684,9 +683,6 @@ uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info, boo
if (i.second.second) if (i.second.second)
cacheDrvOutputMapping(state, id, i.first, *i.second.second); cacheDrvOutputMapping(state, id, i.first, *i.second.second);
} }
if (drv)
*drv = std::move(parsedDrv);
} }
pathInfoCache->lock()->upsert(info.path, PathInfoCacheValue{.value = std::make_shared<const ValidPathInfo>(info)}); pathInfoCache->lock()->upsert(info.path, PathInfoCacheValue{.value = std::make_shared<const ValidPathInfo>(info)});
@ -927,19 +923,12 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos)
SQLiteTxn txn(state->db); SQLiteTxn txn(state->db);
StorePathSet paths; StorePathSet paths;
std::map<StorePath, Derivation> derivations;
for (auto & [_, i] : infos) { for (auto & [_, i] : infos) {
assert(i.narHash.algo == HashAlgorithm::SHA256); assert(i.narHash.algo == HashAlgorithm::SHA256);
if (isValidPath_(*state, i.path)) if (isValidPath_(*state, i.path))
updatePathInfo(*state, i); updatePathInfo(*state, i);
else if (i.path.isDerivation()) { else
Derivation drv; addValidPath(*state, i);
addValidPath(*state, i, false, &drv);
derivations.emplace(i.path, std::move(drv));
} else {
addValidPath(*state, i, false);
}
paths.insert(i.path); paths.insert(i.path);
} }
@ -949,18 +938,6 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos)
state->stmts->AddReference.use()(referrer)(queryValidPathId(*state, j)).exec(); state->stmts->AddReference.use()(referrer)(queryValidPathId(*state, j)).exec();
} }
/* Check that the derivation outputs are correct. We can't do
this in addValidPath() above, because the references might
not be valid yet. */
for (auto & [_, i] : infos)
if (i.path.isDerivation()) {
if (auto drv = derivations.find(i.path); drv != derivations.end()) {
drv->second.checkInvariants(*this, i.path);
} else {
readInvalidDerivation(i.path).checkInvariants(*this, i.path);
}
}
/* Do a topological sort of the paths. This will throw an /* Do a topological sort of the paths. This will throw an
error if a cycle is detected and roll back the error if a cycle is detected and roll back the
transaction. Cycles can only occur when a derivation transaction. Cycles can only occur when a derivation