Add a schema migration that drops the query cache (#1558)

This is a force fix for potential existence filter mismatches caused by
#1548

The essential problem is that when resuming a query, the server is
allowed to forget deletes. If the number of incorrectly synthesized
deletes matches the number of server-forgotten deletes then the
existence filter can give a false positive, preventing the cache from
self healing.

Dropping the query cache clears any client-side resume token which
prevents a false positive existence filter mismatch.

Note that the remote document cache and mutation queues are unaffected
so any cached documents that do exist will still work while offline

firebase/firebase-js-sdk#1019

Author: wilhuff

Firestore/Example/Tests/Local/FSTLevelDBMigrationsTests.mm

@@ -18,22 +18,29 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #import "Firestore/Protos/objc/firestore/local/Target.pbobjc.h"
 #import "Firestore/Source/Local/FSTLevelDB.h"
 #import "Firestore/Source/Local/FSTLevelDBKey.h"
 #import "Firestore/Source/Local/FSTLevelDBMigrations.h"
+#import "Firestore/Source/Local/FSTLevelDBMutationQueue.h"
 #import "Firestore/Source/Local/FSTLevelDBQueryCache.h"
 
 #include "Firestore/core/src/firebase/firestore/util/ordered_code.h"
+#include "Firestore/core/src/firebase/firestore/util/status.h"
+#include "Firestore/core/test/firebase/firestore/testutil/testutil.h"
+#include "absl/strings/match.h"
 #include "leveldb/db.h"
 
 #import "Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h"
 
 NS_ASSUME_NONNULL_BEGIN
 
+using firebase::firestore::FirestoreErrorCode;
 using firebase::firestore::local::LevelDbTransaction;
 using firebase::firestore::util::OrderedCode;
+using firebase::firestore::testutil::Key;
 using leveldb::DB;
 using leveldb::Options;
 using leveldb::Status;
@@ -64,54 +71,136 @@ - (void)tearDown {
 - (void)testAddsTargetGlobal {
   FSTPBTargetGlobal *metadata = [FSTLevelDBQueryCache readTargetMetadataFromDB:_db.get()];
   XCTAssertNil(metadata, @"Not expecting metadata yet, we should have an empty db");
-  LevelDbTransaction transaction(_db.get(), "testAddsTargetGlobal");
-  [FSTLevelDBMigrations runMigrationsWithTransaction:&transaction];
-  transaction.Commit();
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get()];
+
   metadata = [FSTLevelDBQueryCache readTargetMetadataFromDB:_db.get()];
   XCTAssertNotNil(metadata, @"Migrations should have added the metadata");
 }
 
 - (void)testSetsVersionNumber {
-  LevelDbTransaction transaction(_db.get(), "testSetsVersionNumber");
-  FSTLevelDBSchemaVersion initial =
-      [FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
-  XCTAssertEqual(0, initial, "No version should be equivalent to 0");
-
-  // Pick an arbitrary high migration number and migrate to it.
-  [FSTLevelDBMigrations runMigrationsWithTransaction:&transaction];
-  FSTLevelDBSchemaVersion actual = [FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
-  XCTAssertGreaterThan(actual, 0, @"Expected to migrate to a schema version > 0");
+  {
+    LevelDbTransaction transaction(_db.get(), "testSetsVersionNumber before");
+    FSTLevelDBSchemaVersion initial =
+        [FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
+    XCTAssertEqual(0, initial, "No version should be equivalent to 0");
+  }
+
+  {
+    // Pick an arbitrary high migration number and migrate to it.
+    [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get()];
+
+    LevelDbTransaction transaction(_db.get(), "testSetsVersionNumber after");
+    FSTLevelDBSchemaVersion actual =
+        [FSTLevelDBMigrations schemaVersionWithTransaction:&transaction];
+    XCTAssertGreaterThan(actual, 0, @"Expected to migrate to a schema version > 0");
+  }
 }
 
-- (void)testCountsQueries {
-  NSUInteger expected = 50;
+#define ASSERT_NOT_FOUND(transaction, key)                \
+  do {                                                    \
+    std::string unused_result;                            \
+    Status status = transaction.Get(key, &unused_result); \
+    XCTAssertTrue(status.IsNotFound());                   \
+  } while (0)
+
+#define ASSERT_FOUND(transaction, key)                    \
+  do {                                                    \
+    std::string unused_result;                            \
+    Status status = transaction.Get(key, &unused_result); \
+    XCTAssertTrue(status.ok());                           \
+  } while (0)
+
+- (void)testDropsTheQueryCache {
+  NSString *userID = @"user";
+  FSTBatchID batchID = 1;
+  FSTTargetID targetID = 2;
+
+  FSTDocumentKey *key1 = Key("documents/1");
+  FSTDocumentKey *key2 = Key("documents/2");
+
+  std::string targetKeys[] = {
+      [FSTLevelDBTargetKey keyWithTargetID:targetID],
+      [FSTLevelDBTargetDocumentKey keyWithTargetID:targetID documentKey:key1],
+      [FSTLevelDBTargetDocumentKey keyWithTargetID:targetID documentKey:key2],
+      [FSTLevelDBDocumentTargetKey keyWithDocumentKey:key1 targetID:targetID],
+      [FSTLevelDBDocumentTargetKey keyWithDocumentKey:key2 targetID:targetID],
+      [FSTLevelDBQueryTargetKey keyWithCanonicalID:"foo.bar.baz" targetID:targetID],
+  };
+
+  // Keys that should not be modified by the dropping the query cache
+  std::string preservedKeys[] = {
+      [self dummyKeyForTable:"targetA"],
+      [FSTLevelDBMutationQueueKey keyWithUserID:userID],
+      [FSTLevelDBMutationKey keyWithUserID:userID batchID:batchID],
+  };
+
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:2];
   {
     // Setup some targets to be counted in the migration.
-    LevelDbTransaction transaction(_db.get(), "testCountsQueries setup");
-    for (int i = 0; i < expected; i++) {
-      std::string key = [FSTLevelDBTargetKey keyWithTargetID:i];
-      transaction.Put(key, "dummy");
+    LevelDbTransaction transaction(_db.get(), "testDropsTheQueryCache setup");
+    for (const std::string &key : targetKeys) {
+      transaction.Put(key, "target");
+    }
+    for (const std::string &key : preservedKeys) {
+      transaction.Put(key, "preserved");
     }
-    // Add a dummy entry after the targets to make sure the iteration is correctly bounded.
-    // Use a table that would sort logically right after that table 'target'.
-    std::string dummyKey;
-    // Magic number that indicates a table name follows. Needed to mimic the prefix to the target
-    // table.
-    OrderedCode::WriteSignedNumIncreasing(&dummyKey, 5);
-    OrderedCode::WriteString(&dummyKey, "targetA");
-    transaction.Put(dummyKey, "dummy");
     transaction.Commit();
   }
 
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:3];
   {
-    LevelDbTransaction transaction(_db.get(), "testCountsQueries");
-    [FSTLevelDBMigrations runMigrationsWithTransaction:&transaction];
-    transaction.Commit();
+    LevelDbTransaction transaction(_db.get(), "testDropsTheQueryCache");
+    for (const std::string &key : targetKeys) {
+      ASSERT_NOT_FOUND(transaction, key);
+    }
+    for (const std::string &key : preservedKeys) {
+      ASSERT_FOUND(transaction, key);
+    }
+
     FSTPBTargetGlobal *metadata = [FSTLevelDBQueryCache readTargetMetadataFromDB:_db.get()];
-    XCTAssertEqual(expected, metadata.targetCount, @"Failed to count all of the targets we added");
+    XCTAssertNotNil(metadata, @"Metadata should have been added");
+    XCTAssertEqual(metadata.targetCount, 0);
   }
 }
 
+- (void)testDropsTheQueryCacheWithThousandsOfEntries {
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:2];
+  {
+    // Setup some targets to be destroyed.
+    LevelDbTransaction transaction(_db.get(), "testDropsTheQueryCacheWithThousandsOfEntries setup");
+    for (int i = 0; i < 10000; ++i) {
+      transaction.Put([FSTLevelDBTargetKey keyWithTargetID:i], "");
+    }
+    transaction.Commit();
+  }
+
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_db.get() upToVersion:3];
+  {
+    LevelDbTransaction transaction(_db.get(), "Verify");
+    std::string prefix = [FSTLevelDBTargetKey keyPrefix];
+
+    auto it = transaction.NewIterator();
+    std::vector<std::string> found_keys;
+    for (it->Seek(prefix); it->Valid() && absl::StartsWith(it->key(), prefix); it->Next()) {
+      found_keys.push_back(std::string{it->key()});
+    }
+
+    XCTAssertEqual(found_keys, std::vector<std::string>{});
+  }
+}
+
+/**
+ * Creates the name of a dummy entry to make sure the iteration is correctly bounded.
+ */
+- (std::string)dummyKeyForTable:(const char *)tableName {
+  std::string dummyKey;
+  // Magic number that indicates a table name follows. Needed to mimic the prefix to the target
+  // table.
+  OrderedCode::WriteSignedNumIncreasing(&dummyKey, 5);
+  OrderedCode::WriteString(&dummyKey, tableName);
+  return dummyKey;
+}
+
 @end
 
 NS_ASSUME_NONNULL_END

Firestore/Source/Local/FSTLevelDB.mm

@@ -150,9 +150,7 @@ - (BOOL)start:(NSError **)error {
     return NO;
   }
   _ptr.reset(database);
-  LevelDbTransaction transaction(_ptr.get(), "Start LevelDB");
-  [FSTLevelDBMigrations runMigrationsWithTransaction:&transaction];
-  transaction.Commit();
+  [FSTLevelDBMigrations runMigrationsWithDatabase:_ptr.get()];
   return YES;
 }
 

Firestore/Source/Local/FSTLevelDBMigrations.h

@@ -36,7 +36,13 @@ typedef int32_t FSTLevelDBSchemaVersion;
 /**
  * Runs any migrations needed to bring the given database up to the current schema version
  */
-+ (void)runMigrationsWithTransaction:(firebase::firestore::local::LevelDbTransaction *)transaction;
++ (void)runMigrationsWithDatabase:(leveldb::DB *)database;
+
+/**
+ * Runs any migrations needed to bring the given database up to the given schema version
+ */
++ (void)runMigrationsWithDatabase:(leveldb::DB *)database
+                      upToVersion:(FSTLevelDBSchemaVersion)version;
 
 @end
 

Firestore/Source/Local/FSTLevelDBMigrations.mm

@@ -24,32 +24,34 @@
 
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 #include "absl/base/macros.h"
+#include "absl/memory/memory.h"
 #include "absl/strings/match.h"
 #include "leveldb/write_batch.h"
 
 NS_ASSUME_NONNULL_BEGIN
 
-// Current version of the schema defined in this file.
-static FSTLevelDBSchemaVersion kSchemaVersion = 2;
+/**
+ * Schema version for the iOS client.
+ *
+ * Note that tables aren't a concept in LevelDB. They exist in our schema as just prefixes on keys.
+ * This means tables don't need to be created but they also can't easily be dropped and re-created.
+ *
+ * Migrations:
+ *   * Migration 1 used to ensure the target_global row existed, without clearing it. No longer
+ *     required because migration 3 unconditionally clears it.
+ *   * Migration 2 used to ensure that the target_global row had a correct count of targets. No
+ *     longer required because migration 3 deletes them all.
+ *   * Migration 3 deletes the entire query cache to deal with cache corruption related to
+ *     limbo resolution. Addresses https://github.com/firebase/firebase-ios-sdk/issues/1548.
+ */
+static FSTLevelDBSchemaVersion kSchemaVersion = 3;
 
 using firebase::firestore::local::LevelDbTransaction;
-using leveldb::DB;
 using leveldb::Iterator;
 using leveldb::Status;
 using leveldb::Slice;
 using leveldb::WriteOptions;
 
-/**
- * Ensures that the global singleton target metadata row exists in LevelDB.
- */
-static void EnsureTargetGlobal(LevelDbTransaction *transaction) {
-  FSTPBTargetGlobal *targetGlobal =
-      [FSTLevelDBQueryCache readTargetMetadataWithTransaction:transaction];
-  if (!targetGlobal) {
-    transaction->Put([FSTLevelDBTargetGlobalKey key], [FSTPBTargetGlobal message]);
-  }
-}
-
 /**
  * Save the given version number as the current version of the schema of the database.
  * @param version The version to save
@@ -61,30 +63,39 @@ static void SaveVersion(FSTLevelDBSchemaVersion version, LevelDbTransaction *tra
   transaction->Put(key, version_string);
 }
 
-/**
- * This function counts the number of targets that currently exist in the given db. It
- * then reads the target global row, adds the count to the metadata from that row, and writes
- * the metadata back.
- *
- * It assumes the metadata has already been written and is able to be read in this transaction.
- */
-static void AddTargetCount(LevelDbTransaction *transaction) {
-  auto it = transaction->NewIterator();
-  std::string start_key = [FSTLevelDBTargetKey keyPrefix];
-  it->Seek(start_key);
-
-  int32_t count = 0;
-  while (it->Valid() && absl::StartsWith(it->key(), start_key)) {
-    count++;
-    it->Next();
+static void DeleteEverythingWithPrefix(const std::string &prefix, leveldb::DB *db) {
+  bool more_deletes = true;
+  while (more_deletes) {
+    LevelDbTransaction transaction(db, "Delete everything with prefix");
+    auto it = transaction.NewIterator();
+
+    more_deletes = false;
+    for (it->Seek(prefix); it->Valid() && absl::StartsWith(it->key(), prefix); it->Next()) {
+      if (transaction.changed_keys() >= 1000) {
+        more_deletes = true;
+        break;
+      }
+      transaction.Delete(it->key());
+    }
+
+    transaction.Commit();
   }
+}
+
+/** Migration 3. */
+static void ClearQueryCache(leveldb::DB *db) {
+  DeleteEverythingWithPrefix([FSTLevelDBTargetKey keyPrefix], db);
+  DeleteEverythingWithPrefix([FSTLevelDBDocumentTargetKey keyPrefix], db);
+  DeleteEverythingWithPrefix([FSTLevelDBTargetDocumentKey keyPrefix], db);
+  DeleteEverythingWithPrefix([FSTLevelDBQueryTargetKey keyPrefix], db);
+
+  LevelDbTransaction transaction(db, "Drop query cache");
 
-  FSTPBTargetGlobal *targetGlobal =
-      [FSTLevelDBQueryCache readTargetMetadataWithTransaction:transaction];
-  HARD_ASSERT(targetGlobal != nil,
-              "We should have a metadata row as it was added in an earlier migration");
-  targetGlobal.targetCount = count;
-  transaction->Put([FSTLevelDBTargetGlobalKey key], targetGlobal);
+  // Reset the target global entry too (to reset the target count).
+  transaction.Put([FSTLevelDBTargetGlobalKey key], [FSTPBTargetGlobal message]);
+
+  SaveVersion(3, &transaction);
+  transaction.Commit();
 }
 
 @implementation FSTLevelDBMigrations
@@ -101,23 +112,19 @@ + (FSTLevelDBSchemaVersion)schemaVersionWithTransaction:
   }
 }
 
-+ (void)runMigrationsWithTransaction:(firebase::firestore::local::LevelDbTransaction *)transaction {
-  FSTLevelDBSchemaVersion currentVersion = [self schemaVersionWithTransaction:transaction];
-  // Each case in this switch statement intentionally falls through. This lets us
-  // start at the current schema version and apply any migrations that have not yet
-  // been applied, to bring us up to current, as defined by the kSchemaVersion constant.
-  switch (currentVersion) {
-    case 0:
-      EnsureTargetGlobal(transaction);
-      ABSL_FALLTHROUGH_INTENDED;
-    case 1:
-      // We're now guaranteed that the target global exists. We can safely add a count to it.
-      AddTargetCount(transaction);
-      ABSL_FALLTHROUGH_INTENDED;
-    default:
-      if (currentVersion < kSchemaVersion) {
-        SaveVersion(kSchemaVersion, transaction);
-      }
++ (void)runMigrationsWithDatabase:(leveldb::DB *)database {
+  [self runMigrationsWithDatabase:database upToVersion:kSchemaVersion];
+}
+
++ (void)runMigrationsWithDatabase:(leveldb::DB *)database
+                      upToVersion:(FSTLevelDBSchemaVersion)toVersion {
+  LevelDbTransaction transaction{database, "Read schema version"};
+  FSTLevelDBSchemaVersion fromVersion = [self schemaVersionWithTransaction:&transaction];
+
+  // This must run unconditionally because schema migrations were added to iOS after the first
+  // release. There may be clients that have never run any migrations that have existing targets.
+  if (fromVersion < 3 && toVersion >= 3) {
+    ClearQueryCache(database);
   }
 }
 

Firestore/core/src/firebase/firestore/local/leveldb_transaction.h

@@ -144,6 +144,10 @@ class LevelDbTransaction {
    */
   static const leveldb::WriteOptions& DefaultWriteOptions();
 
+  size_t changed_keys() const {
+    return mutations_.size() + deletions_.size();
+  }
+
   /**
    * Remove the database entry (if any) for "key".  It is not an error if "key"
    * did not exist in the database.