Ignore errors enumerating files during GC (#5892)

Author: wilhuff

Firestore/CHANGELOG.md

@@ -1,7 +1,10 @@
 # Unreleased
 - [fixed] Removed a delay that may have prevented Firestore from immediately
   establishing a network connection if a connectivity change occurred while
-  the app was in the background.
+  the app was in the background (#5783).
+- [fixed] Fixed a rare crash that could happen if the garbage collection
+  process for old documents in the cache happened to run during a LevelDB
+  compaction (#5881).
 
 # v1.16.0
 - [fixed] Fixed an issue that may have prevented the client from connecting

Firestore/core/src/local/leveldb_lru_reference_delegate.cc

@@ -27,6 +27,7 @@
 #include "Firestore/core/src/local/target_data.h"
 #include "Firestore/core/src/model/resource_path.h"
 #include "Firestore/core/src/model/types.h"
+#include "Firestore/core/src/util/statusor.h"
 #include "absl/memory/memory.h"
 #include "absl/strings/match.h"
 
@@ -37,6 +38,7 @@ namespace local {
 using model::DocumentKey;
 using model::ListenSequenceNumber;
 using model::ResourcePath;
+using util::StatusOr;
 
 LevelDbLruReferenceDelegate::LevelDbLruReferenceDelegate(
     LevelDbPersistence* persistence, LruParams lru_params)
@@ -104,7 +106,7 @@ LruGarbageCollector* LevelDbLruReferenceDelegate::garbage_collector() {
   return gc_.get();
 }
 
-int64_t LevelDbLruReferenceDelegate::CalculateByteSize() {
+StatusOr<int64_t> LevelDbLruReferenceDelegate::CalculateByteSize() {
   return db_->CalculateByteSize();
 }
 

Firestore/core/src/local/leveldb_lru_reference_delegate.h

@@ -57,7 +57,7 @@ class LevelDbLruReferenceDelegate : public LruDelegate {
 
   LruGarbageCollector* garbage_collector() override;
 
-  int64_t CalculateByteSize() override;
+  util::StatusOr<int64_t> CalculateByteSize() override;
   size_t GetSequenceNumberCount() override;
 
   void EnumerateTargets(const TargetCallback& callback) override;

Firestore/core/src/local/leveldb_persistence.cc

@@ -76,7 +76,7 @@ std::set<std::string> CollectUserSet(LevelDbTransaction* transaction) {
 
 }  // namespace
 
-util::StatusOr<std::unique_ptr<LevelDbPersistence>> LevelDbPersistence::Create(
+StatusOr<std::unique_ptr<LevelDbPersistence>> LevelDbPersistence::Create(
     util::Path dir, LocalSerializer serializer, const LruParams& lru_params) {
   auto* fs = Filesystem::Default();
   Status status = EnsureDirectory(dir);
@@ -178,21 +178,35 @@ util::Status LevelDbPersistence::ClearPersistence(
   return fs->RecursivelyRemove(leveldb_dir);
 }
 
-int64_t LevelDbPersistence::CalculateByteSize() {
+StatusOr<int64_t> LevelDbPersistence::CalculateByteSize() {
   auto* fs = Filesystem::Default();
 
-  int64_t count = 0;
+  // Accumulate the total size in an unsigned integer to avoid undefined
+  // behavior on overflow.
+  uint64_t count = 0;
   auto iter = util::DirectoryIterator::Create(directory_);
   for (; iter->Valid(); iter->Next()) {
-    int64_t file_size = fs->FileSize(iter->file()).ValueOrDie();
+    StatusOr<int64_t> maybe_size = fs->FileSize(iter->file());
+    if (!maybe_size.ok()) {
+      return Status::FromCause("Failed to size LevelDB directory",
+                               maybe_size.status());
+    }
+
+    uint64_t old_count = count;
+    int64_t file_size = maybe_size.ValueOrDie();
     count += file_size;
+
+    if (count < old_count || count > std::numeric_limits<int64_t>::max()) {
+      return Status(Error::kErrorOutOfRange,
+                    "Failed to size LevelDB: count overflowed");
+    }
   }
 
-  HARD_ASSERT(iter->status().ok(), "Failed to iterate LevelDB directory: %s",
-              iter->status().error_message().c_str());
-  HARD_ASSERT(count >= 0 && count <= std::numeric_limits<int64_t>::max(),
-              "Overflowed counting bytes cached");
-  return count;
+  if (!iter->status().ok()) {
+    return Status::FromCause("Failed to iterate over LevelDB files",
+                             iter->status());
+  }
+  return static_cast<int64_t>(count);
 }
 
 // MARK: - Persistence

Firestore/core/src/local/leveldb_persistence.h

@@ -69,7 +69,7 @@ class LevelDbPersistence : public Persistence {
 
   static util::Status ClearPersistence(const core::DatabaseInfo& database_info);
 
-  int64_t CalculateByteSize();
+  util::StatusOr<int64_t> CalculateByteSize();
 
   // MARK: Persistence overrides
 

Firestore/core/src/local/lru_garbage_collector.cc

@@ -26,16 +26,18 @@
 #include "Firestore/core/src/local/target_data.h"
 #include "Firestore/core/src/model/document_key.h"
 #include "Firestore/core/src/util/log.h"
+#include "Firestore/core/src/util/statusor.h"
 
 namespace firebase {
 namespace firestore {
 namespace local {
 namespace {
 
-using firebase::firestore::api::Settings;
-using firebase::firestore::model::DocumentKey;
-using firebase::firestore::model::ListenSequenceNumber;
-using firebase::firestore::model::TargetId;
+using api::Settings;
+using model::DocumentKey;
+using model::ListenSequenceNumber;
+using model::TargetId;
+using util::StatusOr;
 
 using Millis = std::chrono::milliseconds;
 
@@ -108,13 +110,26 @@ LruGarbageCollector::LruGarbageCollector(LruDelegate* delegate,
     : delegate_(delegate), params_(std::move(params)) {
 }
 
+StatusOr<int64_t> LruGarbageCollector::CalculateByteSize() const {
+  return delegate_->CalculateByteSize();
+}
+
 LruResults LruGarbageCollector::Collect(const LiveQueryMap& live_targets) {
   if (params_.min_bytes_threshold == Settings::CacheSizeUnlimited) {
     LOG_DEBUG("Garbage collection skipped; disabled");
     return LruResults::DidNotRun();
   }
 
-  int64_t current_size = CalculateByteSize();
+  StatusOr<int64_t> maybe_current_size = CalculateByteSize();
+  if (!maybe_current_size.ok()) {
+    LOG_ERROR(
+        "Garbage collection skipped; failed to estimate the size of the "
+        "cache: %s",
+        maybe_current_size.status().ToString());
+    return LruResults::DidNotRun();
+  }
+
+  int64_t current_size = maybe_current_size.ValueOrDie();
   if (current_size < params_.min_bytes_threshold) {
     // Not enough on disk to warrant collection. Wait another timeout cycle.
     LOG_DEBUG(

Firestore/core/src/local/lru_garbage_collector.h

@@ -23,6 +23,7 @@
 #include "Firestore/core/src/local/target_cache.h"
 #include "Firestore/core/src/local/target_data.h"
 #include "Firestore/core/src/model/types.h"
+#include "Firestore/core/src/util/status_fwd.h"
 
 namespace firebase {
 namespace firestore {
@@ -71,7 +72,7 @@ class LruDelegate : public ReferenceDelegate {
   /** Access to the underlying LRU Garbage collector instance. */
   virtual LruGarbageCollector* garbage_collector() = 0;
 
-  virtual int64_t CalculateByteSize() = 0;
+  virtual util::StatusOr<int64_t> CalculateByteSize() = 0;
 
   /** Returns the number of targets and orphaned documents cached. */
   virtual size_t GetSequenceNumberCount() = 0;
@@ -114,9 +115,7 @@ class LruGarbageCollector {
  public:
   LruGarbageCollector(LruDelegate* delegate, LruParams params);
 
-  int64_t CalculateByteSize() const {
-    return delegate_->CalculateByteSize();
-  }
+  util::StatusOr<int64_t> CalculateByteSize() const;
 
   /**
    * Given a target percentile, return the number of queries that make up that

Firestore/core/src/local/memory_lru_reference_delegate.cc

@@ -27,6 +27,7 @@
 #include "Firestore/core/src/local/remote_document_cache.h"
 #include "Firestore/core/src/local/sizer.h"
 #include "Firestore/core/src/local/target_data.h"
+#include "Firestore/core/src/util/statusor.h"
 #include "absl/memory/memory.h"
 
 namespace firebase {
@@ -35,6 +36,7 @@ namespace local {
 
 using model::DocumentKey;
 using model::ListenSequenceNumber;
+using util::StatusOr;
 
 MemoryLruReferenceDelegate::MemoryLruReferenceDelegate(
     MemoryPersistence* persistence,
@@ -172,7 +174,7 @@ bool MemoryLruReferenceDelegate::IsPinnedAtSequenceNumber(
   return false;
 }
 
-int64_t MemoryLruReferenceDelegate::CalculateByteSize() {
+StatusOr<int64_t> MemoryLruReferenceDelegate::CalculateByteSize() {
   // Note that this method is only used for testing because this delegate is
   // only used for testing. The algorithm here (loop through everything,
   // serialize it and count bytes) is inefficient and inexact, but won't run in

Firestore/core/src/local/memory_lru_reference_delegate.h

@@ -65,7 +65,7 @@ class MemoryLruReferenceDelegate : public LruDelegate {
 
   LruGarbageCollector* garbage_collector() override;
 
-  int64_t CalculateByteSize() override;
+  util::StatusOr<int64_t> CalculateByteSize() override;
   size_t GetSequenceNumberCount() override;
 
   void EnumerateTargets(const TargetCallback& callback) override;

Firestore/core/src/util/status.cc

@@ -32,6 +32,14 @@ Status::Status(Error code, std::string msg) {
   state_ = State::MakePtr(code, std::move(msg));
 }
 
+Status Status::FromCause(std::string message, const Status& cause) {
+  if (cause.ok()) {
+    return cause;
+  }
+
+  return Status(cause.code(), std::move(message)).CausedBy(cause);
+}
+
 void Status::Update(const Status& new_status) {
   if (ok()) {
     *this = new_status;