Address potential IndexedDB failure in synchronizeQueryViewsAndRaiseSnapshots (#3113)

Author: schmidt-sebastian

packages/firestore/src/core/sync_engine.ts

@@ -1110,14 +1110,10 @@ export class MultiTabSyncEngine extends SyncEngine
       const queries = this.queriesByTarget.get(targetId);
 
       if (queries && queries.length !== 0) {
-        // For queries that have a local View, we need to update their state
-        // in LocalStore (as the resume token and the snapshot version
+        // For queries that have a local View, we fetch their current state
+        // from LocalStore (as the resume token and the snapshot version
         // might have changed) and reconcile their views with the persisted
         // state (the list of syncedDocuments may have gotten out of sync).
-        await this.localStore.releaseTarget(
-          targetId,
-          /*keepPersistedTargetData=*/ true
-        );
         targetData = await this.localStore.allocateTarget(
           queries[0].toTarget()
         );
@@ -1136,7 +1132,7 @@ export class MultiTabSyncEngine extends SyncEngine
       } else {
         debugAssert(
           transitionToPrimary,
-          'A secondary tab should never have an active target without an active query.'
+          'A secondary tab should never have an active view without an active target.'
         );
         // For queries that never executed on this client, we need to
         // allocate the target in LocalStore and initialize a new View.

packages/firestore/src/local/indexeddb_persistence.ts

@@ -404,7 +404,7 @@ export class IndexedDbPersistence implements Persistence {
               return this.verifyPrimaryLease(txn).next(success => {
                 if (!success) {
                   this.isPrimary = false;
-                  this.queue.enqueueAndForget(() =>
+                  this.queue.enqueueRetryable(() =>
                     this.primaryStateListener(false)
                   );
                 }
@@ -444,7 +444,7 @@ export class IndexedDbPersistence implements Persistence {
       })
       .then(isPrimary => {
         if (this.isPrimary !== isPrimary) {
-          this.queue.enqueueAndForget(() =>
+          this.queue.enqueueRetryable(() =>
             this.primaryStateListener(isPrimary)
           );
         }
@@ -805,7 +805,7 @@ export class IndexedDbPersistence implements Persistence {
                   `Failed to obtain primary lease for action '${action}'.`
                 );
                 this.isPrimary = false;
-                this.queue.enqueueAndForget(() =>
+                this.queue.enqueueRetryable(() =>
                   this.primaryStateListener(false)
                 );
                 throw new FirestoreError(

packages/firestore/src/local/local_store.ts

@@ -831,7 +831,17 @@ export class LocalStore {
           });
       })
       .then(targetData => {
-        if (this.targetDataByTarget.get(targetData.targetId) === null) {
+        // If Multi-Tab is enabled, the existing target data may be newer than
+        // the in-memory data
+        const cachedTargetData = this.targetDataByTarget.get(
+          targetData.targetId
+        );
+        if (
+          cachedTargetData === null ||
+          targetData.snapshotVersion.compareTo(
+            cachedTargetData.snapshotVersion
+          ) > 0
+        ) {
           this.targetDataByTarget = this.targetDataByTarget.insert(
             targetData.targetId,
             targetData

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

@@ -17,7 +17,7 @@
 
 import { Query } from '../../../src/core/query';
 import { Code } from '../../../src/util/error';
-import { deletedDoc, doc, filter, path, orderBy } from '../../util/helpers';
+import { deletedDoc, doc, filter, orderBy, path } from '../../util/helpers';
 
 import { TimerId } from '../../../src/util/async_queue';
 import { describeSpec, specTest } from './describe_spec';

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

@@ -88,7 +88,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
   );
 
   specTest(
-    'Query raises events in secondary client  (with recovery)',
+    'Query raises events in secondary client (with recovery)',
     ['multi-client'],
     () => {
       const query = Query.atPath(path('collection'));
@@ -144,6 +144,84 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
     }
   );
 
+  specTest(
+    'Query with active view recovers after primary tab failover (with recovery)',
+    ['multi-client'],
+    () => {
+      const query = Query.atPath(path('collection'));
+      const docA = doc('collection/a', 1000, { key: 'a' });
+      const docB = doc('collection/b', 2000, { key: 'b' });
+
+      return (
+        client(0)
+          .expectPrimaryState(true)
+          .client(1)
+          // Register a query in the secondary client
+          .userListens(query)
+          .client(0)
+          .expectListen(query)
+          .watchAcksFull(query, 1000, docA)
+          // Shutdown the primary client to release its lease
+          .shutdown()
+          .client(1)
+          .expectEvents(query, { added: [docA] })
+          // Run the lease refresh to attempt taking over the primary lease. The
+          // first lease refresh fails with a simulated transaction failure.
+          .failDatabaseTransactions('Allocate target')
+          .runTimer(TimerId.ClientMetadataRefresh)
+          .expectPrimaryState(false)
+          .recoverDatabase()
+          .runTimer(TimerId.AsyncQueueRetry)
+          .expectPrimaryState(true)
+          .expectListen(query, 'resume-token-1000')
+          .watchAcksFull(query, 2000, docB)
+          .expectEvents(query, { added: [docB] })
+      );
+    }
+  );
+
+  specTest(
+    'Query without active view recovers after primary tab failover (with recovery)',
+    ['multi-client'],
+    () => {
+      const query = Query.atPath(path('collection'));
+      const docA = doc('collection/a', 1000, { key: 'a' });
+      const docB = doc('collection/b', 2000, { key: 'b' });
+
+      return (
+        client(0)
+          .expectPrimaryState(true)
+          // Initialize a second client that doesn't have any active targets
+          .client(1)
+          .client(2)
+          // Register a query in the third client
+          .userListens(query)
+          .client(0)
+          .expectListen(query)
+          .watchAcksFull(query, 1000, docA)
+          .client(2)
+          .expectEvents(query, { added: [docA] })
+          .client(0)
+          // Shutdown the primary client to release its lease
+          .shutdown()
+          .client(1)
+          // Run the lease refresh in the second client, which does not yet have
+          // an active view for the third client's query. The lease refresh fails
+          // at first, but then recovers and initializes the view.
+          .failDatabaseTransactions('Allocate target')
+          .runTimer(TimerId.ClientMetadataRefresh)
+          .expectPrimaryState(false)
+          .recoverDatabase()
+          .runTimer(TimerId.AsyncQueueRetry)
+          .expectPrimaryState(true)
+          .expectListen(query, 'resume-token-1000')
+          .watchAcksFull(query, 2000, docB)
+          .client(2)
+          .expectEvents(query, { added: [docB] })
+      );
+    }
+  );
+
   specTest(
     'Ignores intermittent lease refresh failures (with recovery)',
     ['multi-client'],