forked from lix-project/lix
pathInfoCache: Use the entire base name as the cache key
This fixes a bug in the garbage collector where if a path /nix/store/abcd-foo is valid, but we do a isValidPath("/nix/store/abcd-foo.lock") first, then a negative entry for /nix/store/abcd is added to pathInfoCache, so /nix/store/abcd-foo is subsequently considered invalid and deleted.
This commit is contained in:
parent
eab934cb2a
commit
0be8cc1466
5 changed files with 27 additions and 23 deletions
|
@ -111,15 +111,15 @@ void BinaryCacheStore::writeNarInfo(ref<NarInfo> narInfo)
|
||||||
|
|
||||||
upsertFile(narInfoFile, narInfo->to_string(*this), "text/x-nix-narinfo");
|
upsertFile(narInfoFile, narInfo->to_string(*this), "text/x-nix-narinfo");
|
||||||
|
|
||||||
std::string hashPart(narInfo->path.hashPart());
|
|
||||||
|
|
||||||
{
|
{
|
||||||
auto state_(state.lock());
|
auto state_(state.lock());
|
||||||
state_->pathInfoCache.upsert(hashPart, PathInfoCacheValue { .value = std::shared_ptr<NarInfo>(narInfo) });
|
state_->pathInfoCache.upsert(
|
||||||
|
std::string(narInfo->path.to_string()),
|
||||||
|
PathInfoCacheValue { .value = std::shared_ptr<NarInfo>(narInfo) });
|
||||||
}
|
}
|
||||||
|
|
||||||
if (diskCache)
|
if (diskCache)
|
||||||
diskCache->upsertNarInfo(getUri(), hashPart, std::shared_ptr<NarInfo>(narInfo));
|
diskCache->upsertNarInfo(getUri(), std::string(narInfo->path.hashPart()), std::shared_ptr<NarInfo>(narInfo));
|
||||||
}
|
}
|
||||||
|
|
||||||
AutoCloseFD openFile(const Path & path)
|
AutoCloseFD openFile(const Path & path)
|
||||||
|
|
|
@ -834,7 +834,7 @@ uint64_t LocalStore::addValidPath(State & state,
|
||||||
|
|
||||||
{
|
{
|
||||||
auto state_(Store::state.lock());
|
auto state_(Store::state.lock());
|
||||||
state_->pathInfoCache.upsert(std::string(info.path.hashPart()),
|
state_->pathInfoCache.upsert(std::string(info.path.to_string()),
|
||||||
PathInfoCacheValue{ .value = std::make_shared<const ValidPathInfo>(info) });
|
PathInfoCacheValue{ .value = std::make_shared<const ValidPathInfo>(info) });
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -1207,7 +1207,7 @@ void LocalStore::invalidatePath(State & state, const StorePath & path)
|
||||||
|
|
||||||
{
|
{
|
||||||
auto state_(Store::state.lock());
|
auto state_(Store::state.lock());
|
||||||
state_->pathInfoCache.erase(std::string(path.hashPart()));
|
state_->pathInfoCache.erase(std::string(path.to_string()));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -414,11 +414,9 @@ StorePathSet Store::queryDerivationOutputs(const StorePath & path)
|
||||||
|
|
||||||
bool Store::isValidPath(const StorePath & storePath)
|
bool Store::isValidPath(const StorePath & storePath)
|
||||||
{
|
{
|
||||||
std::string hashPart(storePath.hashPart());
|
|
||||||
|
|
||||||
{
|
{
|
||||||
auto state_(state.lock());
|
auto state_(state.lock());
|
||||||
auto res = state_->pathInfoCache.get(hashPart);
|
auto res = state_->pathInfoCache.get(std::string(storePath.to_string()));
|
||||||
if (res && res->isKnownNow()) {
|
if (res && res->isKnownNow()) {
|
||||||
stats.narInfoReadAverted++;
|
stats.narInfoReadAverted++;
|
||||||
return res->didExist();
|
return res->didExist();
|
||||||
|
@ -426,11 +424,11 @@ bool Store::isValidPath(const StorePath & storePath)
|
||||||
}
|
}
|
||||||
|
|
||||||
if (diskCache) {
|
if (diskCache) {
|
||||||
auto res = diskCache->lookupNarInfo(getUri(), hashPart);
|
auto res = diskCache->lookupNarInfo(getUri(), std::string(storePath.hashPart()));
|
||||||
if (res.first != NarInfoDiskCache::oUnknown) {
|
if (res.first != NarInfoDiskCache::oUnknown) {
|
||||||
stats.narInfoReadAverted++;
|
stats.narInfoReadAverted++;
|
||||||
auto state_(state.lock());
|
auto state_(state.lock());
|
||||||
state_->pathInfoCache.upsert(hashPart,
|
state_->pathInfoCache.upsert(std::string(storePath.to_string()),
|
||||||
res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue { .value = res.second });
|
res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue { .value = res.second });
|
||||||
return res.first == NarInfoDiskCache::oValid;
|
return res.first == NarInfoDiskCache::oValid;
|
||||||
}
|
}
|
||||||
|
@ -440,7 +438,7 @@ bool Store::isValidPath(const StorePath & storePath)
|
||||||
|
|
||||||
if (diskCache && !valid)
|
if (diskCache && !valid)
|
||||||
// FIXME: handle valid = true case.
|
// FIXME: handle valid = true case.
|
||||||
diskCache->upsertNarInfo(getUri(), hashPart, 0);
|
diskCache->upsertNarInfo(getUri(), std::string(storePath.hashPart()), 0);
|
||||||
|
|
||||||
return valid;
|
return valid;
|
||||||
}
|
}
|
||||||
|
@ -487,13 +485,11 @@ static bool goodStorePath(const StorePath & expected, const StorePath & actual)
|
||||||
void Store::queryPathInfo(const StorePath & storePath,
|
void Store::queryPathInfo(const StorePath & storePath,
|
||||||
Callback<ref<const ValidPathInfo>> callback) noexcept
|
Callback<ref<const ValidPathInfo>> callback) noexcept
|
||||||
{
|
{
|
||||||
std::string hashPart;
|
auto hashPart = std::string(storePath.hashPart());
|
||||||
|
|
||||||
try {
|
try {
|
||||||
hashPart = storePath.hashPart();
|
|
||||||
|
|
||||||
{
|
{
|
||||||
auto res = state.lock()->pathInfoCache.get(hashPart);
|
auto res = state.lock()->pathInfoCache.get(std::string(storePath.to_string()));
|
||||||
if (res && res->isKnownNow()) {
|
if (res && res->isKnownNow()) {
|
||||||
stats.narInfoReadAverted++;
|
stats.narInfoReadAverted++;
|
||||||
if (!res->didExist())
|
if (!res->didExist())
|
||||||
|
@ -508,7 +504,7 @@ void Store::queryPathInfo(const StorePath & storePath,
|
||||||
stats.narInfoReadAverted++;
|
stats.narInfoReadAverted++;
|
||||||
{
|
{
|
||||||
auto state_(state.lock());
|
auto state_(state.lock());
|
||||||
state_->pathInfoCache.upsert(hashPart,
|
state_->pathInfoCache.upsert(std::string(storePath.to_string()),
|
||||||
res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second });
|
res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second });
|
||||||
if (res.first == NarInfoDiskCache::oInvalid ||
|
if (res.first == NarInfoDiskCache::oInvalid ||
|
||||||
!goodStorePath(storePath, res.second->path))
|
!goodStorePath(storePath, res.second->path))
|
||||||
|
@ -523,7 +519,7 @@ void Store::queryPathInfo(const StorePath & storePath,
|
||||||
auto callbackPtr = std::make_shared<decltype(callback)>(std::move(callback));
|
auto callbackPtr = std::make_shared<decltype(callback)>(std::move(callback));
|
||||||
|
|
||||||
queryPathInfoUncached(storePath,
|
queryPathInfoUncached(storePath,
|
||||||
{[this, storePathS{printStorePath(storePath)}, hashPart, callbackPtr](std::future<std::shared_ptr<const ValidPathInfo>> fut) {
|
{[this, storePath, hashPart, callbackPtr](std::future<std::shared_ptr<const ValidPathInfo>> fut) {
|
||||||
|
|
||||||
try {
|
try {
|
||||||
auto info = fut.get();
|
auto info = fut.get();
|
||||||
|
@ -533,14 +529,12 @@ void Store::queryPathInfo(const StorePath & storePath,
|
||||||
|
|
||||||
{
|
{
|
||||||
auto state_(state.lock());
|
auto state_(state.lock());
|
||||||
state_->pathInfoCache.upsert(hashPart, PathInfoCacheValue { .value = info });
|
state_->pathInfoCache.upsert(std::string(storePath.to_string()), PathInfoCacheValue { .value = info });
|
||||||
}
|
}
|
||||||
|
|
||||||
auto storePath = parseStorePath(storePathS);
|
|
||||||
|
|
||||||
if (!info || !goodStorePath(storePath, info->path)) {
|
if (!info || !goodStorePath(storePath, info->path)) {
|
||||||
stats.narInfoMissing++;
|
stats.narInfoMissing++;
|
||||||
throw InvalidPath("path '%s' is not valid", storePathS);
|
throw InvalidPath("path '%s' is not valid", printStorePath(storePath));
|
||||||
}
|
}
|
||||||
|
|
||||||
(*callbackPtr)(ref<const ValidPathInfo>(info));
|
(*callbackPtr)(ref<const ValidPathInfo>(info));
|
||||||
|
|
|
@ -232,7 +232,6 @@ protected:
|
||||||
|
|
||||||
struct State
|
struct State
|
||||||
{
|
{
|
||||||
// FIXME: fix key
|
|
||||||
LRUCache<std::string, PathInfoCacheValue> pathInfoCache;
|
LRUCache<std::string, PathInfoCacheValue> pathInfoCache;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
11
tests/gc.sh
11
tests/gc.sh
|
@ -1,5 +1,7 @@
|
||||||
source common.sh
|
source common.sh
|
||||||
|
|
||||||
|
clearStore
|
||||||
|
|
||||||
drvPath=$(nix-instantiate dependencies.nix)
|
drvPath=$(nix-instantiate dependencies.nix)
|
||||||
outPath=$(nix-store -rvv "$drvPath")
|
outPath=$(nix-store -rvv "$drvPath")
|
||||||
|
|
||||||
|
@ -23,6 +25,11 @@ test -e $inUse
|
||||||
if nix-store --delete $outPath; then false; fi
|
if nix-store --delete $outPath; then false; fi
|
||||||
test -e $outPath
|
test -e $outPath
|
||||||
|
|
||||||
|
for i in $NIX_STORE_DIR/*; do
|
||||||
|
touch $i.lock
|
||||||
|
touch $i.chroot
|
||||||
|
done
|
||||||
|
|
||||||
nix-collect-garbage
|
nix-collect-garbage
|
||||||
|
|
||||||
# Check that the root and its dependencies haven't been deleted.
|
# Check that the root and its dependencies haven't been deleted.
|
||||||
|
@ -38,3 +45,7 @@ nix-collect-garbage
|
||||||
|
|
||||||
# Check that the output has been GC'd.
|
# Check that the output has been GC'd.
|
||||||
if test -e $outPath/foobar; then false; fi
|
if test -e $outPath/foobar; then false; fi
|
||||||
|
|
||||||
|
# Check that the store is empty.
|
||||||
|
rmdir $NIX_STORE_DIR/.links
|
||||||
|
rmdir $NIX_STORE_DIR
|
||||||
|
|
Loading…
Reference in a new issue