Ignore failures for notifyLocalViewChanges (#3062)

Author: schmidt-sebastian

packages/firestore/src/local/local_store.ts

@@ -694,57 +694,71 @@ export class LocalStore {
   /**
    * Notify local store of the changed views to locally pin documents.
    */
-  notifyLocalViewChanges(viewChanges: LocalViewChanges[]): Promise<void> {
-    return this.persistence
-      .runTransaction('notifyLocalViewChanges', 'readwrite', txn => {
-        return PersistencePromise.forEach(
-          viewChanges,
-          (viewChange: LocalViewChanges) => {
-            return PersistencePromise.forEach(
-              viewChange.addedKeys,
-              (key: DocumentKey) =>
-                this.persistence.referenceDelegate.addReference(
-                  txn,
-                  viewChange.targetId,
-                  key
-                )
-            ).next(() =>
-              PersistencePromise.forEach(
-                viewChange.removedKeys,
+  async notifyLocalViewChanges(viewChanges: LocalViewChanges[]): Promise<void> {
+    try {
+      await this.persistence.runTransaction(
+        'notifyLocalViewChanges',
+        'readwrite',
+        txn => {
+          return PersistencePromise.forEach(
+            viewChanges,
+            (viewChange: LocalViewChanges) => {
+              return PersistencePromise.forEach(
+                viewChange.addedKeys,
                 (key: DocumentKey) =>
-                  this.persistence.referenceDelegate.removeReference(
+                  this.persistence.referenceDelegate.addReference(
                     txn,
                     viewChange.targetId,
                     key
                   )
-              )
-            );
-          }
+              ).next(() =>
+                PersistencePromise.forEach(
+                  viewChange.removedKeys,
+                  (key: DocumentKey) =>
+                    this.persistence.referenceDelegate.removeReference(
+                      txn,
+                      viewChange.targetId,
+                      key
+                    )
+                )
+              );
+            }
+          );
+        }
+      );
+    } catch (e) {
+      if (e.name === 'IndexedDbTransactionError') {
+        // If `notifyLocalViewChanges` fails, we did not advance the sequence
+        // number for the documents that were included in this transaction.
+        // This might trigger them to be deleted earlier than they otherwise
+        // would have, but it should not invalidate the integrity of the data.
+        logDebug(LOG_TAG, 'Failed to update sequence numbers: ' + e);
+      } else {
+        throw e;
+      }
+    }
+
+    for (const viewChange of viewChanges) {
+      const targetId = viewChange.targetId;
+
+      if (!viewChange.fromCache) {
+        const targetData = this.targetDataByTarget.get(targetId);
+        debugAssert(
+          targetData !== null,
+          `Can't set limbo-free snapshot version for unknown target: ${targetId}`
         );
-      })
-      .then(() => {
-        for (const viewChange of viewChanges) {
-          const targetId = viewChange.targetId;
-
-          if (!viewChange.fromCache) {
-            const targetData = this.targetDataByTarget.get(targetId);
-            debugAssert(
-              targetData !== null,
-              `Can't set limbo-free snapshot version for unknown target: ${targetId}`
-            );
 
-            // Advance the last limbo free snapshot version
-            const lastLimboFreeSnapshotVersion = targetData.snapshotVersion;
-            const updatedTargetData = targetData.withLastLimboFreeSnapshotVersion(
-              lastLimboFreeSnapshotVersion
-            );
-            this.targetDataByTarget = this.targetDataByTarget.insert(
-              targetId,
-              updatedTargetData
-            );
-          }
-        }
-      });
+        // Advance the last limbo free snapshot version
+        const lastLimboFreeSnapshotVersion = targetData.snapshotVersion;
+        const updatedTargetData = targetData.withLastLimboFreeSnapshotVersion(
+          lastLimboFreeSnapshotVersion
+        );
+        this.targetDataByTarget = this.targetDataByTarget.insert(
+          targetId,
+          updatedTargetData
+        );
+      }
+    }
   }
 
   /**

packages/firestore/test/unit/specs/recovery_spec.test.ts

@@ -20,7 +20,7 @@ import { client, spec } from './spec_builder';
 import { TimerId } from '../../../src/util/async_queue';
 import { Query } from '../../../src/core/query';
 import { Code } from '../../../src/util/error';
-import { doc, filter, path } from '../../util/helpers';
+import { deletedDoc, doc, filter, path } from '../../util/helpers';
 import { RpcError } from './spec_rpc_error';
 
 describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
@@ -124,24 +124,34 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
   );
 
   specTest('Recovers when write cannot be persisted', [], () => {
-    return spec()
-      .userSets('collection/key1', { foo: 'a' })
-      .expectNumOutstandingWrites(1)
-      .failDatabaseTransactions({
-        'Locally write mutations': true,
-        notifyLocalViewChanges: true,
-        'Get next mutation batch': true,
-        'Get last stream token': true
-      })
-      .userSets('collection/key2', { bar: 'b' })
-      .expectUserCallbacks({ rejected: ['collection/key2'] })
-      .recoverDatabase()
-      .expectNumOutstandingWrites(1)
-      .userSets('collection/key3', { baz: 'c' })
-      .expectNumOutstandingWrites(2)
-      .writeAcks('collection/key1', 1)
-      .writeAcks('collection/key3', 2)
-      .expectNumOutstandingWrites(0);
+    return (
+      spec()
+        .userSets('collection/key1', { foo: 'a' })
+        .expectNumOutstandingWrites(1)
+        // We fail the write if we cannot persist the local mutation (via
+        // 'Locally write mutations').
+        .failDatabaseTransactions({
+          'Locally write mutations': true
+        })
+        .userSets('collection/key2', { bar: 'b' })
+        .expectUserCallbacks({ rejected: ['collection/key2'] })
+        // The write is considered successful if we can persist the local mutation
+        // but fail to update view assignments (via 'notifyLocalViewChanges').
+        .failDatabaseTransactions({
+          'Locally write mutations': false,
+          notifyLocalViewChanges: true,
+          'Get next mutation batch': false
+        })
+        .userSets('collection/key3', { bar: 'b' })
+        .recoverDatabase()
+        .expectNumOutstandingWrites(2)
+        .userSets('collection/key4', { baz: 'c' })
+        .expectNumOutstandingWrites(3)
+        .writeAcks('collection/key1', 1)
+        .writeAcks('collection/key3', 2)
+        .writeAcks('collection/key4', 3)
+        .expectNumOutstandingWrites(0)
+    );
   });
 
   specTest('Does not surface non-persisted writes', [], () => {
@@ -188,6 +198,82 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       .expectEvents(query, { metadata: [doc1, doc3] });
   });
 
+  specTest(
+    'Surfaces local documents if notifyLocalViewChanges fails',
+    [],
+    () => {
+      const query = Query.atPath(path('collection'));
+      const doc1Local = doc(
+        'collection/key1',
+        0,
+        { foo: 'a' },
+        { hasLocalMutations: true }
+      );
+      const doc1 = doc('collection/key1', 1, { foo: 'a' });
+      const doc2 = doc('collection/key2', 2, { foo: 'b' });
+      return spec()
+        .userListens(query)
+        .failDatabaseTransactions({
+          'Locally write mutations': false,
+          notifyLocalViewChanges: true,
+          'Get next mutation batch': false,
+          'Set last stream token': false
+        })
+        .userSets('collection/key1', { foo: 'a' })
+        .expectEvents(query, {
+          added: [doc1Local],
+          fromCache: true,
+          hasPendingWrites: true
+        })
+        .recoverDatabase()
+        .runTimer(TimerId.AsyncQueueRetry)
+        .writeAcks('collection/key1', 1)
+        .failDatabaseTransactions({
+          'Apply remote event': false,
+          notifyLocalViewChanges: true,
+          'Get last remote snapshot version': false
+        })
+        .watchAcksFull(query, 1000, doc1, doc2)
+        .expectEvents(query, {
+          metadata: [doc1],
+          added: [doc2]
+        });
+    }
+  );
+
+  specTest(
+    'Excludes documents from future queries even if notifyLocalViewChanges fails',
+    [],
+    () => {
+      const query = Query.atPath(path('collection'));
+      const doc1 = doc('collection/key1', 1000, { foo: 'a' });
+      const deletedDoc1 = deletedDoc('collection/key1', 2000);
+      return (
+        spec()
+          .withGCEnabled(false)
+          .userListens(query)
+          .watchAcksFull(query, 1000, doc1)
+          .expectEvents(query, {
+            added: [doc1]
+          })
+          .failDatabaseTransactions({
+            'Apply remote event': false,
+            notifyLocalViewChanges: true,
+            'Get last remote snapshot version': false
+          })
+          .watchSends({ removed: [query] }, deletedDoc1)
+          .watchSnapshots(2000)
+          .expectEvents(query, {
+            removed: [doc1]
+          })
+          .recoverDatabase()
+          .userUnlistens(query)
+          // No event since the document was removed
+          .userListens(query, 'resume-token-1000')
+      );
+    }
+  );
+
   specTest('Fails targets that cannot be allocated', [], () => {
     const query1 = Query.atPath(path('collection1'));
     const query2 = Query.atPath(path('collection2'));