[IC] Handling eviction edge cases

Change-Id: Ic82b2f89b7b7e02566ca2b4e2605b52cd2adcb03
Reviewed-on: https://chromium-review.googlesource.com/1234774Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593380}

components/image_fetcher/core/storage/image_cache.cc

@@ -25,17 +25,19 @@ constexpr char kPrefLastEvictionKey[] =
     "cached_image_fetcher_last_eviction_time";
 
 // TODO(wylieb): Control these parameters server-side.
-constexpr size_t kCacheMaxSize = 64 * 1024 * 1024;  // 64mb.
+constexpr int kCacheMaxSize = 64 * 1024 * 1024;         // 64mb.
+constexpr int kCacheResizeWhenFull = 48 * 1024 * 1024;  // 48mb.
 
 // Cache items are allowed to live for the given amount of days.
-constexpr size_t kCacheItemsTimeToLiveDays = 7;
-constexpr size_t kImageCacheEvictionIntervalHours = 24;
-constexpr size_t kImageCacheEvictionDelayMinutes = 5;
+constexpr int kCacheItemsTimeToLiveDays = 7;
+constexpr int kImageCacheEvictionIntervalHours = 24;
 
 std::string HashUrlToKey(const std::string& input) {
   return base32::Base32Encode(base::SHA1HashString(input));
 }
 
+void OnStartupEvictionQueued() {}
+
 }  // namespace
 
 namespace image_fetcher {
@@ -125,17 +127,22 @@ void ImageCache::OnDependencyInitialized() {
 
   // TODO(wylieb): Consider delaying eviction as new requests come in via
   // seperate weak pointers.
+  // TODO(wylieb): Log UMA data about starting GC eviction here, then again
+  // when it's finished.
   // Once all the queued requests are taken care of, run eviction.
-  task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(&ImageCache::RunEviction, weak_ptr_factory_.GetWeakPtr()),
-      base::TimeDelta::FromMinutes(kImageCacheEvictionDelayMinutes));
+  base::PostTaskWithTraitsAndReply(
+      FROM_HERE, {base::TaskPriority::BEST_EFFORT},
+      base::BindOnce(OnStartupEvictionQueued),
+      base::BindOnce(&ImageCache::RunEvictionOnStartup,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ImageCache::SaveImageImpl(const std::string& url, std::string image_data) {
   std::string key = HashUrlToKey(url);
 
-  // TODO(wylieb): Run eviction if size is larger than desired size.
+  // If the cache is full, evict some stuff.
+  RunEvictionWhenFull();
+
   size_t length = image_data.length();
   data_store_->SaveImage(key, std::move(image_data));
   metadata_store_->SaveImageMetadata(key, length);
@@ -156,8 +163,7 @@ void ImageCache::DeleteImageImpl(const std::string& url) {
   metadata_store_->DeleteImageMetadata(key);
 }
 
-// TODO(wylieb): Support an eviction and reconciliation routine.
-void ImageCache::RunEviction() {
+void ImageCache::RunEvictionOnStartup() {
   base::Time last_eviction_time = pref_service_->GetTime(kPrefLastEvictionKey);
   // If we've already garbage collected in the past interval, bail out.
   if (last_eviction_time >
@@ -166,19 +172,72 @@ void ImageCache::RunEviction() {
     return;
   }
 
+  RunEviction(kCacheMaxSize, base::BindOnce(&ImageCache::RunReconciliation,
+                                            weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ImageCache::RunEvictionWhenFull() {
+  // Storage is within limits, bail out.
+  if (metadata_store_->GetEstimatedSize() < kCacheMaxSize) {
+    return;
+  }
+
+  RunEviction(kCacheResizeWhenFull, base::OnceClosure());
+}
+
+void ImageCache::RunEviction(size_t bytes_left,
+                             base::OnceClosure on_completion) {
   base::Time eviction_time = clock_->Now();
   pref_service_->SetTime(kPrefLastEvictionKey, eviction_time);
   metadata_store_->EvictImageMetadata(
       eviction_time - base::TimeDelta::FromDays(kCacheItemsTimeToLiveDays),
-      kCacheMaxSize,
-      base::BindOnce(&ImageCache::OnKeysEvicted,
-                     weak_ptr_factory_.GetWeakPtr()));
+      bytes_left,
+      base::BindOnce(&ImageCache::OnKeysEvicted, weak_ptr_factory_.GetWeakPtr(),
+                     std::move(on_completion)));
 }
 
-void ImageCache::OnKeysEvicted(std::vector<std::string> keys) {
+void ImageCache::OnKeysEvicted(base::OnceClosure on_completion,
+                               std::vector<std::string> keys) {
   for (const std::string& key : keys) {
     data_store_->DeleteImage(key);
   }
+
+  std::move(on_completion).Run();
+}
+
+void ImageCache::RunReconciliation() {
+  metadata_store_->GetAllKeys(base::BindOnce(&ImageCache::ReconcileMetadataKeys,
+                                             weak_ptr_factory_.GetWeakPtr()));
+}
+
+void ImageCache::ReconcileMetadataKeys(std::vector<std::string> metadata_keys) {
+  data_store_->GetAllKeys(base::BindOnce(&ImageCache::ReconcileDataKeys,
+                                         weak_ptr_factory_.GetWeakPtr(),
+                                         std::move(metadata_keys)));
+}
+
+void ImageCache::ReconcileDataKeys(std::vector<std::string> metadata_keys,
+                                   std::vector<std::string> data_keys) {
+  std::sort(metadata_keys.begin(), metadata_keys.end());
+  std::sort(data_keys.begin(), data_keys.end());
+
+  std::vector<std::string> diff;
+  // Get the keys that should be removed from metadata.
+  std::set_difference(metadata_keys.begin(), metadata_keys.end(),
+                      data_keys.begin(), data_keys.end(),
+                      std::inserter(diff, diff.begin()));
+
+  for (const std::string& key : diff) {
+    metadata_store_->DeleteImageMetadata(key);
+  }
+
+  diff.clear();
+  // Get the keys that should be removed from data.
+  std::set_difference(data_keys.begin(), data_keys.end(), metadata_keys.begin(),
+                      metadata_keys.end(), std::inserter(diff, diff.begin()));
+  for (const std::string& key : diff) {
+    data_store_->DeleteImage(key);
+  }
 }
 
 }  // namespace image_fetcher

components/image_fetcher/core/storage/image_cache.h

@@ -67,10 +67,23 @@ class ImageCache {
   // Deletes the data for |url|.
   void DeleteImageImpl(const std::string& url);
 
-  // Runs eviction on the data stores.
-  void RunEviction();
+  // Runs eviction on the data stores (run on startup).
+  void RunEvictionOnStartup();
+  // Runs eviction on the data stores (run when cache is full).
+  void RunEvictionWhenFull();
+  // Catch-all method for eviction, runs reconciliation routine after if
+  // |run_reconciliation| is specified. Evicts until there are |bytes_left|
+  // left in storage.
+  void RunEviction(size_t bytes_left, base::OnceClosure on_completion);
   // Deletes the given keys from the data store.
-  void OnKeysEvicted(std::vector<std::string> keys);
+  void OnKeysEvicted(base::OnceClosure on_completion,
+                     std::vector<std::string> keys);
+  // Reconcile what's on disk against what's in metadata. Any mismatches will
+  // result in eviction.
+  void RunReconciliation();
+  void ReconcileMetadataKeys(std::vector<std::string> md_keys);
+  void ReconcileDataKeys(std::vector<std::string> md_keys,
+                         std::vector<std::string> data_keys);
 
   bool initialization_attempted_;
   std::vector<base::OnceClosure> queued_requests_;

components/image_fetcher/core/storage/image_cache_unittest.cc

@@ -38,13 +38,15 @@ class ImageCacheTest : public testing::Test {
   void CreateImageCache() {
     clock_.SetNow(base::Time());
 
-    auto db =
-        std::make_unique<FakeDB<CachedImageMetadataProto>>(&metadata_store_);
+    auto db = std::make_unique<FakeDB<CachedImageMetadataProto>>(&db_store_);
     db_ = db.get();
     auto metadata_store = std::make_unique<ImageMetadataStoreLevelDB>(
         base::FilePath(), std::move(db), &clock_);
+    metadata_store_ = metadata_store.get();
+
     auto data_store = std::make_unique<ImageDataStoreDisk>(
         temp_dir_.GetPath(), base::SequencedTaskRunnerHandle::Get());
+    data_store_ = data_store.get();
 
     ImageCache::RegisterProfilePrefs(test_prefs_.registry());
     image_cache_ = std::make_unique<ImageCache>(
@@ -54,7 +56,8 @@ class ImageCacheTest : public testing::Test {
 
   void InitializeImageCache() {
     image_cache_->MaybeStartInitialization();
-    db_->InitCallback(true);
+    db()->InitCallback(true);
+
     RunUntilIdle();
   }
 
@@ -69,8 +72,8 @@ class ImageCacheTest : public testing::Test {
     return image_cache()->AreAllDependenciesInitialized();
   }
 
-  void RunCacheEviction(bool success) {
-    image_cache()->RunEviction();
+  void RunEvictionOnStartup(bool success) {
+    image_cache()->RunEvictionOnStartup();
 
     if (success) {
       db_->LoadCallback(true);
@@ -80,23 +83,48 @@ class ImageCacheTest : public testing::Test {
     RunUntilIdle();
   }
 
+  bool IsMetadataPresent(const std::string& key) {
+    return db_store_.find(key) != db_store_.end();
+  }
+
+  void RunReconciliation() {
+    image_cache()->RunReconciliation();
+    db()->LoadKeysCallback(true);
+    RunUntilIdle();
+    db()->UpdateCallback(true);
+    RunUntilIdle();
+  }
+
+  void InjectMetadata(std::string key, int data_size) {
+    metadata_store_->SaveImageMetadata(key, data_size);
+  }
+
+  void InjectData(std::string key, std::string data) {
+    data_store_->SaveImage(key, data);
+    RunUntilIdle();
+  }
+
   void RunUntilIdle() { base::RunLoop().RunUntilIdle(); }
 
   TestingPrefServiceSimple* prefs() { return &test_prefs_; }
   base::SimpleTestClock* clock() { return &clock_; }
   ImageCache* image_cache() { return image_cache_.get(); }
+  ImageDataStoreDisk* data_store() { return data_store_; }
+  ImageMetadataStoreLevelDB* metadata_store() { return metadata_store_; }
   FakeDB<CachedImageMetadataProto>* db() { return db_; }
 
   MOCK_METHOD1(DataCallback, void(std::string));
 
  private:
   std::unique_ptr<ImageCache> image_cache_;
+  ImageMetadataStoreLevelDB* metadata_store_;
+  ImageDataStoreDisk* data_store_;
   base::SimpleTestClock clock_;
 
   TestingPrefServiceSimple test_prefs_;
   base::ScopedTempDir temp_dir_;
   FakeDB<CachedImageMetadataProto>* db_;
-  std::map<std::string, CachedImageMetadataProto> metadata_store_;
+  std::map<std::string, CachedImageMetadataProto> db_store_;
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
@@ -184,7 +212,7 @@ TEST_F(ImageCacheTest, Eviction) {
   PrepareImageCache();
 
   clock()->SetNow(clock()->Now() + base::TimeDelta::FromDays(7));
-  RunCacheEviction(/* success */ true);
+  RunEvictionOnStartup(/* success */ true);
 
   EXPECT_CALL(*this, DataCallback(std::string()));
   image_cache()->LoadImage(
@@ -197,7 +225,7 @@ TEST_F(ImageCacheTest, EvictionTooSoon) {
   PrepareImageCache();
 
   clock()->SetNow(clock()->Now() + base::TimeDelta::FromDays(6));
-  RunCacheEviction(/* success */ true);
+  RunEvictionOnStartup(/* success */ true);
 
   EXPECT_CALL(*this, DataCallback(kImageData));
   image_cache()->LoadImage(
@@ -211,7 +239,7 @@ TEST_F(ImageCacheTest, EvictionWhenEvictionAlreadyPerformed) {
 
   prefs()->SetTime("cached_image_fetcher_last_eviction_time", clock()->Now());
   clock()->SetNow(clock()->Now() + base::TimeDelta::FromHours(23));
-  RunCacheEviction(/* success */ false);
+  RunEvictionOnStartup(/* success */ false);
 
   EXPECT_CALL(*this, DataCallback(kImageData));
   image_cache()->LoadImage(
@@ -220,4 +248,54 @@ TEST_F(ImageCacheTest, EvictionWhenEvictionAlreadyPerformed) {
   RunUntilIdle();
 }
 
+TEST_F(ImageCacheTest, Reconciliation) {
+  CreateImageCache();
+  InitializeImageCache();
+
+  // Inject differing keys so they mismatch, then run reconciliation.
+  InjectData("foo", "z");
+  InjectMetadata("bar", 10);
+  RunReconciliation();
+
+  // Data should be gone.
+  EXPECT_CALL(*this, DataCallback(std::string()));
+  image_cache()->LoadImage("foo", base::BindOnce(&ImageCacheTest::DataCallback,
+                                                 base::Unretained(this)));
+  RunUntilIdle();
+
+  // Metadata should be gone.
+  ASSERT_FALSE(IsMetadataPresent("bar"));
+}
+
+TEST_F(ImageCacheTest, ReconciliationMismatchData) {
+  CreateImageCache();
+  InitializeImageCache();
+
+  // Inject differing keys so they mismatch, then run reconciliation.
+  InjectData("foo", "z");
+  InjectData("bar", "z");
+  InjectMetadata("foo", 10);
+  RunReconciliation();
+
+  // Data should be gone.
+  EXPECT_CALL(*this, DataCallback(std::string()));
+  image_cache()->LoadImage("bar", base::BindOnce(&ImageCacheTest::DataCallback,
+                                                 base::Unretained(this)));
+  RunUntilIdle();
+}
+
+TEST_F(ImageCacheTest, ReconciliationMismatchMetadata) {
+  CreateImageCache();
+  InitializeImageCache();
+
+  // Inject differing keys so they mismatch, then run reconciliation.
+  InjectData("foo", "z");
+  InjectMetadata("foo", 10);
+  InjectMetadata("bar", 10);
+  RunReconciliation();
+
+  // Metadata should be gone.
+  ASSERT_FALSE(IsMetadataPresent("bar"));
+}
+
 }  // namespace image_fetcher

components/image_fetcher/core/storage/image_metadata_store.h

@@ -40,6 +40,9 @@ class ImageMetadataStore {
   // Returns all the keys this store has.
   virtual void GetAllKeys(KeysCallback callback) = 0;
 
+  // Returns the total size of what's in metadata, possibly incorrect.
+  virtual int GetEstimatedSize() = 0;
+
   // Deletes all metadata that's been cached before the boundary given as
   // |expiration_time|.
   void EvictImageMetadata(base::Time expiration_time, KeysCallback callback) {

components/image_fetcher/core/storage/image_metadata_store_leveldb.cc

@@ -69,7 +69,8 @@ ImageMetadataStoreLevelDB::ImageMetadataStoreLevelDB(
     std::unique_ptr<leveldb_proto::ProtoDatabase<CachedImageMetadataProto>>
         database,
     base::Clock* clock)
-    : initialization_status_(InitializationStatus::UNINITIALIZED),
+    : estimated_size_(0),
+      initialization_status_(InitializationStatus::UNINITIALIZED),
       database_dir_(database_dir),
       database_(std::move(database)),
       clock_(clock),
@@ -98,6 +99,7 @@ void ImageMetadataStoreLevelDB::SaveImageMetadata(const std::string& key,
   if (!IsInitialized()) {
     return;
   }
+  estimated_size_ += data_size;
 
   int64_t current_time = ToDatabaseTime(clock_->Now());
   CachedImageMetadataProto metadata_proto;
@@ -152,6 +154,10 @@ void ImageMetadataStoreLevelDB::GetAllKeys(KeysCallback callback) {
                                      std::move(callback)));
 }
 
+int ImageMetadataStoreLevelDB::GetEstimatedSize() {
+  return estimated_size_;
+}
+
 void ImageMetadataStoreLevelDB::EvictImageMetadata(base::Time expiration_time,
                                                    const size_t bytes_left,
                                                    KeysCallback callback) {
@@ -248,6 +254,8 @@ void ImageMetadataStoreLevelDB::EvictImageMetadataImpl(
     }
   }
 
+  estimated_size_ = total_bytes_stored;
+
   if (keys_to_remove.empty()) {
     std::move(callback).Run({});
     return;

components/image_fetcher/core/storage/image_metadata_store_leveldb.h

@@ -50,6 +50,13 @@ class ImageMetadataStoreLevelDB : public ImageMetadataStore {
   void DeleteImageMetadata(const std::string& key) override;
   void UpdateImageMetadata(const std::string& key) override;
   void GetAllKeys(KeysCallback callback) override;
+  // Gets a size estimate. This is updated when things are added to the cache
+  // and when eviction is performed. This doesn't update more often because
+  // we want to minimize disk io. If the estimate is inaccurate, it's either 0
+  // which means it hasn't been calculated yet, or it's an over-estimate. In
+  // the case of an over estimate, it will be recified if you call into an
+  // eviction routine.
+  int GetEstimatedSize() override;
   void EvictImageMetadata(base::Time expiration_time,
                           const size_t bytes_left,
                           KeysCallback callback) override;
@@ -73,6 +80,7 @@ class ImageMetadataStoreLevelDB : public ImageMetadataStore {
                                 std::vector<std::string> deleted_keys,
                                 bool success);
 
+  int estimated_size_;
   InitializationStatus initialization_status_;
   base::FilePath database_dir_;
   std::unique_ptr<leveldb_proto::ProtoDatabase<CachedImageMetadataProto>>

components/image_fetcher/core/storage/image_metadata_store_leveldb_unittest.cc

@@ -282,6 +282,12 @@ TEST_F(ImageMetadataStoreLevelDBTest, GetAllKeysLoadFailed) {
   db()->LoadKeysCallback(false);
 }
 
+TEST_F(ImageMetadataStoreLevelDBTest, GetEstimatedSize) {
+  PrepareDatabase(true);
+
+  EXPECT_EQ(5, metadata_store()->GetEstimatedSize());
+}
+
 TEST_F(ImageMetadataStoreLevelDBTest, GarbageCollectBeforeInit) {
   PrepareDatabase(false);