From 8449b3cac3f2f383ec61bd9cd275d4aa156acee1 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 17 Jan 2023 19:56:06 +0100 Subject: [PATCH] NarInfoDiskCache: Keep BinaryCache.id stable and improve test Fixes #3898 The entire `BinaryCaches` row used to get replaced after it became stale according to the `timestamp` column. In a concurrent scenario, this leads to foreign key conflicts as different instances of the in-process `state.caches` cache now differ, with the consequence that the older process still tries to use the `id` number of the old record. Furthermore, this phenomenon appears to have caused the cache for actual narinfos to be erased about every week, while the default ttl for narinfos was supposed to be 30 days. (cherry picked from commit fb94d5cabd51d57aa82a9857ab20f5f4bd323378) --- src/libstore/nar-info-disk-cache.cc | 55 +++++++++++++++--- src/libstore/nar-info-disk-cache.hh | 2 +- src/libstore/tests/nar-info-disk-cache.cc | 69 ++++++++++++++--------- 3 files changed, 92 insertions(+), 34 deletions(-) diff --git a/src/libstore/nar-info-disk-cache.cc b/src/libstore/nar-info-disk-cache.cc index 94c7d7c2e..b6ea71880 100644 --- a/src/libstore/nar-info-disk-cache.cc +++ b/src/libstore/nar-info-disk-cache.cc @@ -97,7 +97,7 @@ public: state->db.exec(schema); state->insertCache.create(state->db, - "insert or replace into BinaryCaches(url, timestamp, storeDir, wantMassQuery, priority) values (?, ?, ?, ?, ?)"); + "insert into BinaryCaches(url, timestamp, storeDir, wantMassQuery, priority) values (?1, ?2, ?3, ?4, ?5) on conflict (url) do update set timestamp = ?2, storeDir = ?3, wantMassQuery = ?4, priority = ?5 returning id;"); state->queryCache.create(state->db, "select id, storeDir, wantMassQuery, priority from BinaryCaches where url = ? and timestamp > ?"); @@ -165,16 +165,57 @@ public: return i->second; } - void createCache(const std::string & uri, const Path & storeDir, bool wantMassQuery, int priority) override +private: + + std::optional queryCacheRaw(State & state, const std::string & uri) { - retrySQLite([&]() { + auto i = state.caches.find(uri); + if (i == state.caches.end()) { + auto queryCache(state.queryCache.use()(uri)(time(0) - cacheInfoTtl)); + if (!queryCache.next()) + return std::nullopt; + auto cache = Cache { + .id = (int) queryCache.getInt(0), + .storeDir = queryCache.getStr(1), + .wantMassQuery = queryCache.getInt(2) != 0, + .priority = (int) queryCache.getInt(3), + }; + state.caches.emplace(uri, cache); + } + return getCache(state, uri); + } + +public: + int createCache(const std::string & uri, const Path & storeDir, bool wantMassQuery, int priority) override + { + return retrySQLite([&]() { auto state(_state.lock()); + SQLiteTxn txn(state->db); - // FIXME: race + // To avoid the race, we have to check if maybe someone hasn't yet created + // the cache for this URI in the meantime. + auto cache(queryCacheRaw(*state, uri)); - state->insertCache.use()(uri)(time(0))(storeDir)(wantMassQuery)(priority).exec(); - assert(sqlite3_changes(state->db) == 1); - state->caches[uri] = Cache{(int) sqlite3_last_insert_rowid(state->db), storeDir, wantMassQuery, priority}; + if (cache) + return cache->id; + + Cache ret { + .id = -1, // set below + .storeDir = storeDir, + .wantMassQuery = wantMassQuery, + .priority = priority, + }; + + { + auto r(state->insertCache.use()(uri)(time(0))(storeDir)(wantMassQuery)(priority)); + assert(r.next()); + ret.id = (int) r.getInt(0); + } + + state->caches[uri] = ret; + + txn.commit(); + return ret.id; }); } diff --git a/src/libstore/nar-info-disk-cache.hh b/src/libstore/nar-info-disk-cache.hh index adc14f3bc..4877f56d8 100644 --- a/src/libstore/nar-info-disk-cache.hh +++ b/src/libstore/nar-info-disk-cache.hh @@ -13,7 +13,7 @@ public: virtual ~NarInfoDiskCache() { } - virtual void createCache(const std::string & uri, const Path & storeDir, + virtual int createCache(const std::string & uri, const Path & storeDir, bool wantMassQuery, int priority) = 0; struct CacheInfo diff --git a/src/libstore/tests/nar-info-disk-cache.cc b/src/libstore/tests/nar-info-disk-cache.cc index c3bc14fbb..971a251af 100644 --- a/src/libstore/tests/nar-info-disk-cache.cc +++ b/src/libstore/tests/nar-info-disk-cache.cc @@ -7,69 +7,68 @@ namespace nix { -RC_GTEST_PROP( - NarInfoDiskCacheImpl, - create_and_read, - (int prio, bool wantMassQuery) - ) -{ +TEST(NarInfoDiskCacheImpl, create_and_read) { + int prio = 12345; + bool wantMassQuery = true; + Path tmpDir = createTempDir(); AutoDelete delTmpDir(tmpDir); Path dbPath(tmpDir + "/test-narinfo-disk-cache.sqlite"); auto cache = getTestNarInfoDiskCache(dbPath); - cache->createCache("other://uri", "/nix/storedir", wantMassQuery, prio); - cache->createCache("other://uri-2", "/nix/storedir", wantMassQuery, prio); - - cache->createCache("the://uri", "/nix/storedir", wantMassQuery, prio); - { - auto r = cache->upToDateCacheExists("the://uri"); + auto bc1 = cache->createCache("https://bar", "/nix/storedir", wantMassQuery, prio); + auto bc2 = cache->createCache("https://xyz", "/nix/storedir", false, 12); + ASSERT_NE(bc1, bc2); + } + + int savedId = cache->createCache("http://foo", "/nix/storedir", wantMassQuery, prio);; + { + auto r = cache->upToDateCacheExists("http://foo"); ASSERT_TRUE(r); ASSERT_EQ(r->priority, prio); ASSERT_EQ(r->wantMassQuery, wantMassQuery); + ASSERT_EQ(savedId, r->id); } SQLite db(dbPath); SQLiteStmt getIds; - getIds.create(db, "SELECT id FROM BinaryCaches WHERE url = 'the://uri'"); + getIds.create(db, "SELECT id FROM BinaryCaches WHERE url = 'http://foo'"); - int savedId; { auto q(getIds.use()); ASSERT_TRUE(q.next()); - savedId = q.getInt(0); + ASSERT_EQ(savedId, q.getInt(0)); ASSERT_FALSE(q.next()); } - db.exec("UPDATE BinaryCaches SET timestamp = timestamp - 1 - 7 * 24 * 3600;"); // Relies on memory cache { - auto r = cache->upToDateCacheExists("the://uri"); + auto r = cache->upToDateCacheExists("http://foo"); ASSERT_TRUE(r); ASSERT_EQ(r->priority, prio); ASSERT_EQ(r->wantMassQuery, wantMassQuery); } + // We can't clear the in-memory cache, so we use a new cache object. auto cache2 = getTestNarInfoDiskCache(dbPath); { - auto r = cache2->upToDateCacheExists("the://uri"); + auto r = cache2->upToDateCacheExists("http://foo"); ASSERT_FALSE(r); } - cache2->createCache("the://uri", "/nix/storedir", wantMassQuery, prio); + // Update, same data, check that the id number is reused + cache2->createCache("http://foo", "/nix/storedir", wantMassQuery, prio); { - auto r = cache->upToDateCacheExists("the://uri"); + auto r = cache2->upToDateCacheExists("http://foo"); ASSERT_TRUE(r); ASSERT_EQ(r->priority, prio); ASSERT_EQ(r->wantMassQuery, wantMassQuery); - // FIXME, reproduces #3898 - // ASSERT_EQ(r->id, savedId); - (void) savedId; + ASSERT_EQ(r->id, savedId); } { @@ -77,10 +76,28 @@ RC_GTEST_PROP( ASSERT_TRUE(q.next()); auto currentId = q.getInt(0); ASSERT_FALSE(q.next()); - // FIXME, reproduces #3898 - // ASSERT_EQ(currentId, savedId); - (void) currentId; + ASSERT_EQ(currentId, savedId); } + + // Check that the fields can be modified + { + auto r0 = cache2->upToDateCacheExists("https://bar"); + ASSERT_FALSE(r0); + + cache2->createCache("https://bar", "/nix/storedir", !wantMassQuery, prio + 10); + auto r = cache2->upToDateCacheExists("https://bar"); + ASSERT_EQ(r->wantMassQuery, !wantMassQuery); + ASSERT_EQ(r->priority, prio + 10); + } + + // // Force update (no use case yet; we only retrieve cache metadata when stale based on timestamp) + // { + // cache2->createCache("https://bar", "/nix/storedir", wantMassQuery, prio + 20); + // auto r = cache2->upToDateCacheExists("https://bar"); + // ASSERT_EQ(r->wantMassQuery, wantMassQuery); + // ASSERT_EQ(r->priority, prio + 20); + // } + } }