From 0be8cc1466f317e33977590510bac4b18471f0ce Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 14 Oct 2021 13:28:22 +0200 Subject: [PATCH] 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. --- src/libstore/binary-cache-store.cc | 8 ++++---- src/libstore/local-store.cc | 4 ++-- src/libstore/store-api.cc | 26 ++++++++++---------------- src/libstore/store-api.hh | 1 - tests/gc.sh | 11 +++++++++++ 5 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc index 8fce94264..943132754 100644 --- a/src/libstore/binary-cache-store.cc +++ b/src/libstore/binary-cache-store.cc @@ -111,15 +111,15 @@ void BinaryCacheStore::writeNarInfo(ref narInfo) upsertFile(narInfoFile, narInfo->to_string(*this), "text/x-nix-narinfo"); - std::string hashPart(narInfo->path.hashPart()); - { auto state_(state.lock()); - state_->pathInfoCache.upsert(hashPart, PathInfoCacheValue { .value = std::shared_ptr(narInfo) }); + state_->pathInfoCache.upsert( + std::string(narInfo->path.to_string()), + PathInfoCacheValue { .value = std::shared_ptr(narInfo) }); } if (diskCache) - diskCache->upsertNarInfo(getUri(), hashPart, std::shared_ptr(narInfo)); + diskCache->upsertNarInfo(getUri(), std::string(narInfo->path.hashPart()), std::shared_ptr(narInfo)); } AutoCloseFD openFile(const Path & path) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 0e6556e11..c853e9a9c 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -834,7 +834,7 @@ uint64_t LocalStore::addValidPath(State & state, { 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(info) }); } @@ -1207,7 +1207,7 @@ void LocalStore::invalidatePath(State & state, const StorePath & path) { auto state_(Store::state.lock()); - state_->pathInfoCache.erase(std::string(path.hashPart())); + state_->pathInfoCache.erase(std::string(path.to_string())); } } diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index b5ff3dccf..10aa1035e 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -414,11 +414,9 @@ StorePathSet Store::queryDerivationOutputs(const StorePath & path) bool Store::isValidPath(const StorePath & storePath) { - std::string hashPart(storePath.hashPart()); - { auto state_(state.lock()); - auto res = state_->pathInfoCache.get(hashPart); + auto res = state_->pathInfoCache.get(std::string(storePath.to_string())); if (res && res->isKnownNow()) { stats.narInfoReadAverted++; return res->didExist(); @@ -426,11 +424,11 @@ bool Store::isValidPath(const StorePath & storePath) } if (diskCache) { - auto res = diskCache->lookupNarInfo(getUri(), hashPart); + auto res = diskCache->lookupNarInfo(getUri(), std::string(storePath.hashPart())); if (res.first != NarInfoDiskCache::oUnknown) { stats.narInfoReadAverted++; 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 }); return res.first == NarInfoDiskCache::oValid; } @@ -440,7 +438,7 @@ bool Store::isValidPath(const StorePath & storePath) if (diskCache && !valid) // FIXME: handle valid = true case. - diskCache->upsertNarInfo(getUri(), hashPart, 0); + diskCache->upsertNarInfo(getUri(), std::string(storePath.hashPart()), 0); return valid; } @@ -487,13 +485,11 @@ static bool goodStorePath(const StorePath & expected, const StorePath & actual) void Store::queryPathInfo(const StorePath & storePath, Callback> callback) noexcept { - std::string hashPart; + auto hashPart = std::string(storePath.hashPart()); 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()) { stats.narInfoReadAverted++; if (!res->didExist()) @@ -508,7 +504,7 @@ void Store::queryPathInfo(const StorePath & storePath, stats.narInfoReadAverted++; { 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 }); if (res.first == NarInfoDiskCache::oInvalid || !goodStorePath(storePath, res.second->path)) @@ -523,7 +519,7 @@ void Store::queryPathInfo(const StorePath & storePath, auto callbackPtr = std::make_shared(std::move(callback)); queryPathInfoUncached(storePath, - {[this, storePathS{printStorePath(storePath)}, hashPart, callbackPtr](std::future> fut) { + {[this, storePath, hashPart, callbackPtr](std::future> fut) { try { auto info = fut.get(); @@ -533,14 +529,12 @@ void Store::queryPathInfo(const StorePath & storePath, { 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)) { stats.narInfoMissing++; - throw InvalidPath("path '%s' is not valid", storePathS); + throw InvalidPath("path '%s' is not valid", printStorePath(storePath)); } (*callbackPtr)(ref(info)); diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 35461b76d..7d02340df 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -232,7 +232,6 @@ protected: struct State { - // FIXME: fix key LRUCache pathInfoCache; }; diff --git a/tests/gc.sh b/tests/gc.sh index cf0e2c32d..33d627dae 100644 --- a/tests/gc.sh +++ b/tests/gc.sh @@ -1,5 +1,7 @@ source common.sh +clearStore + drvPath=$(nix-instantiate dependencies.nix) outPath=$(nix-store -rvv "$drvPath") @@ -23,6 +25,11 @@ test -e $inUse if nix-store --delete $outPath; then false; fi test -e $outPath +for i in $NIX_STORE_DIR/*; do + touch $i.lock + touch $i.chroot +done + nix-collect-garbage # 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. if test -e $outPath/foobar; then false; fi + +# Check that the store is empty. +rmdir $NIX_STORE_DIR/.links +rmdir $NIX_STORE_DIR