Fix for 8474 - Prevent a possible condition of slow snapshots, caused by a rapid series of document update(s) followed by a delete.

.changeset/tasty-boxes-brake.md

@@ -0,0 +1,5 @@
+---
+"@firebase/firestore": patch
+---
+
+Prevent a possible condition of slow snapshots, caused by a rapid series of document update(s) followed by a delete.

packages/firestore/src/remote/watch_change.ts

@@ -291,6 +291,7 @@ export class WatchChangeAggregator {
 
   /** Keeps track of the documents to update since the last raised snapshot. */
   private pendingDocumentUpdates = mutableDocumentMap();
+  private pendingDocumentUpdatesByTarget = documentTargetMap();
 
   /** A mapping of document keys to their set of target IDs. */
   private pendingDocumentTargetMapping = documentTargetMap();
@@ -587,16 +588,16 @@ export class WatchChangeAggregator {
         if (targetState.current && targetIsDocumentTarget(targetData.target)) {
           // Document queries for document that don't exist can produce an empty
           // result set. To update our local cache, we synthesize a document
-          // delete if we have not previously received the document. This
-          // resolves the limbo state of the document, removing it from
-          // limboDocumentRefs.
+          // delete if we have not previously received the document for this
+          // target. This resolves the limbo state of the document, removing it
+          // from limboDocumentRefs.
           //
           // TODO(dimond): Ideally we would have an explicit lookup target
           // instead resulting in an explicit delete message and we could
           // remove this special logic.
           const key = new DocumentKey(targetData.target.path);
           if (
-            this.pendingDocumentUpdates.get(key) === null &&
+            !this.ensureDocumentUpdateByTarget(key).has(targetId) &&
             !this.targetContainsDocument(targetId, key)
           ) {
             this.removeDocumentFromTarget(
@@ -655,6 +656,7 @@ export class WatchChangeAggregator {
     );
 
     this.pendingDocumentUpdates = mutableDocumentMap();
+    this.pendingDocumentUpdatesByTarget = documentTargetMap();
     this.pendingDocumentTargetMapping = documentTargetMap();
     this.pendingTargetResets = new SortedMap<TargetId, TargetPurpose>(
       primitiveComparator
@@ -685,6 +687,12 @@ export class WatchChangeAggregator {
       document
     );
 
+    this.pendingDocumentUpdatesByTarget =
+      this.pendingDocumentUpdatesByTarget.insert(
+        document.key,
+        this.ensureDocumentUpdateByTarget(document.key).add(targetId)
+      );
+
     this.pendingDocumentTargetMapping =
       this.pendingDocumentTargetMapping.insert(
         document.key,
@@ -724,6 +732,12 @@ export class WatchChangeAggregator {
         this.ensureDocumentTargetMapping(key).delete(targetId)
       );
 
+    this.pendingDocumentTargetMapping =
+      this.pendingDocumentTargetMapping.insert(
+        key,
+        this.ensureDocumentTargetMapping(key).add(targetId)
+      );
+
     if (updatedDocument) {
       this.pendingDocumentUpdates = this.pendingDocumentUpdates.insert(
         key,
@@ -782,6 +796,18 @@ export class WatchChangeAggregator {
     return targetMapping;
   }
 
+  private ensureDocumentUpdateByTarget(key: DocumentKey): SortedSet<TargetId> {
+    let targetMapping = this.pendingDocumentUpdatesByTarget.get(key);
+
+    if (!targetMapping) {
+      targetMapping = new SortedSet<TargetId>(primitiveComparator);
+      this.pendingDocumentUpdatesByTarget =
+        this.pendingDocumentUpdatesByTarget.insert(key, targetMapping);
+    }
+
+    return targetMapping;
+  }
+
   /**
    * Verifies that the user is still interested in this target (by calling
    * `getTargetDataForTarget()`) and that we are not waiting for pending ADDs

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

@@ -1171,4 +1171,284 @@ describeSpec('Limbo Documents:', [], () => {
       );
     }
   );
+
+  specTest(
+    'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred before documentDelete in the global snapshot window',
+    [],
+    () => {
+      // onSnapshot(fullQuery)
+      const fullQuery = query('collection');
+
+      // getDocs(filterQuery)
+      const filterQuery = query('collection', filter('included', '==', true));
+
+      const docA = doc('collection/a', 1000, { key: 'a', included: false });
+      const docA2 = doc('collection/a', 1007, { key: 'a', included: true });
+      const docC = doc('collection/c', 1002, { key: 'c', included: true });
+
+      const limboQueryC = newQueryForPath(docC.key.path);
+
+      return (
+        spec()
+          // onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
+          .userListens(fullQuery)
+          .watchAcksFull(fullQuery, 1001, docA, docC)
+          .expectEvents(fullQuery, {
+            fromCache: false,
+            added: [docA, docC]
+          })
+
+          // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
+          .watchRemovesDoc(docC.key, fullQuery)
+          .watchCurrents(fullQuery, 'resume-token-1002')
+          .watchSnapshots(1002)
+          .expectLimboDocs(docC.key)
+          .expectEvents(fullQuery, {
+            fromCache: true
+          })
+
+          // User begins getDocs(filterQuery)
+          .userListensForGet(filterQuery)
+
+          // getDocs(filterQuery) will not resolve on the snapshot from cache
+          .expectEvents(filterQuery, {
+            fromCache: true,
+            added: [docC]
+          })
+
+          // Watch acks limbo and filter queries
+          .watchAcks(limboQueryC)
+          .watchAcks(filterQuery)
+
+          // Watch responds to limboQueryC - docC was deleted
+          .watchDeletesDoc(docC.key, 1009, limboQueryC)
+          .watchCurrents(limboQueryC, 'resume-token-1009')
+          .watchSnapshots(1009, [limboQueryC, fullQuery])
+
+          // However, docC is still in limbo because there has not been a global snapshot
+          .expectLimboDocs(docC.key)
+
+          // Rapid events of document update and delete caused by application
+          .watchRemovesDoc(docA.key, filterQuery)
+          .watchCurrents(filterQuery, 'resume-token-1004')
+          .watchSends({ affects: [filterQuery] }, docC)
+          .watchCurrents(filterQuery, 'resume-token-1005')
+          .watchRemovesDoc(docC.key, filterQuery)
+          .watchSends({ affects: [filterQuery] }, docA2)
+          .watchCurrents(filterQuery, 'resume-token-1007')
+
+          .watchSnapshots(1010, [fullQuery, limboQueryC])
+
+          // All changes are current and we get a global snapshot
+          .watchSnapshots(1010, [])
+
+          // Now docC is out of limbo
+          .expectLimboDocs()
+          .expectEvents(fullQuery, {
+            fromCache: false,
+            modified: [docA2],
+            removed: [docC]
+          })
+          // Now getDocs(filterQuery) can be resolved
+          .expectEvents(filterQuery, {
+            fromCache: false,
+            removed: [docC],
+            added: [docA2]
+          })
+
+          // No more expected events
+          .watchSnapshots(1100, [])
+      );
+    }
+  );
+
+  specTest(
+    'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred in the global snapshot window and no document delete was received for the limbo resolution query',
+    [],
+    () => {
+      // onSnapshot(fullQuery)
+      const fullQuery = query('collection');
+
+      // getDocs(filterQuery)
+      const filterQuery = query('collection', filter('included', '==', true));
+
+      const docA = doc('collection/a', 1000, { key: 'a', included: false });
+      const docA2 = doc('collection/a', 1007, { key: 'a', included: true });
+      const docC = doc('collection/c', 1002, { key: 'c', included: true });
+
+      const limboQueryC = newQueryForPath(docC.key.path);
+
+      return (
+        spec()
+          // onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
+          .userListens(fullQuery)
+          .watchAcksFull(fullQuery, 1001, docA, docC)
+          .expectEvents(fullQuery, {
+            fromCache: false,
+            added: [docA, docC]
+          })
+
+          // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
+          .watchRemovesDoc(docC.key, fullQuery)
+          .watchCurrents(fullQuery, 'resume-token-1002')
+          .watchSnapshots(1002)
+          .expectLimboDocs(docC.key)
+          .expectEvents(fullQuery, {
+            fromCache: true
+          })
+
+          // User begins getDocs(filterQuery)
+          .userListensForGet(filterQuery)
+
+          // getDocs(filterQuery) will not resolve on the snapshot from cache
+          .expectEvents(filterQuery, {
+            fromCache: true,
+            added: [docC]
+          })
+
+          // Watch acks limbo and filter queries
+          .watchAcks(limboQueryC)
+          .watchAcks(filterQuery)
+
+          // Watch currents the limbo query, but did not send a document delete.
+          // This is and unexpected code path, but something that is called
+          // out as possible in the watch change aggregator.
+          .watchCurrents(limboQueryC, 'resume-token-1009')
+          .watchSnapshots(1009, [limboQueryC, fullQuery])
+
+          // However, docC is still in limbo because there has not been a global snapshot
+          .expectLimboDocs(docC.key)
+
+          // Rapid events of document update and delete caused by application
+          .watchRemovesDoc(docA.key, filterQuery)
+          .watchCurrents(filterQuery, 'resume-token-1004')
+          .watchSends({ affects: [filterQuery] }, docC)
+          .watchCurrents(filterQuery, 'resume-token-1005')
+          .watchRemovesDoc(docC.key, filterQuery)
+          .watchSends({ affects: [filterQuery] }, docA2)
+          .watchCurrents(filterQuery, 'resume-token-1007')
+
+          .watchSnapshots(1010, [fullQuery, limboQueryC])
+
+          // All changes are current and we get a global snapshot
+          .watchSnapshots(1010, [])
+
+          // Now docC is out of limbo
+          .expectLimboDocs()
+          .expectEvents(fullQuery, {
+            fromCache: false,
+            modified: [docA2],
+            removed: [docC]
+          })
+          // Now getDocs(filterQuery) can be resolved
+          .expectEvents(filterQuery, {
+            fromCache: false,
+            removed: [docC],
+            added: [docA2]
+          })
+
+          // No more expected events
+          .watchSnapshots(1100, [])
+      );
+    }
+  );
+
+  specTest(
+    'Fix #8474 - Handles code path of no ack for limbo resolution query before global snapshot',
+    [],
+    () => {
+      // onSnapshot(fullQuery)
+      const fullQuery = query('collection');
+
+      // getDocs(filterQuery)
+      const filterQuery = query('collection', filter('included', '==', true));
+
+      const docA = doc('collection/a', 1000, { key: 'a', included: false });
+      const docA2 = doc('collection/a', 1007, { key: 'a', included: true });
+      const docC = doc('collection/c', 1002, { key: 'c', included: true });
+
+      const limboQueryC = newQueryForPath(docC.key.path);
+
+      return (
+        spec()
+          // onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
+          .userListens(fullQuery)
+          .watchAcksFull(fullQuery, 1001, docA, docC)
+          .expectEvents(fullQuery, {
+            fromCache: false,
+            added: [docA, docC]
+          })
+
+          // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
+          .watchRemovesDoc(docC.key, fullQuery)
+          .watchCurrents(fullQuery, 'resume-token-1002')
+          .watchSnapshots(1002)
+          .expectLimboDocs(docC.key)
+          .expectEvents(fullQuery, {
+            fromCache: true
+          })
+
+          // User begins getDocs(filterQuery)
+          .userListensForGet(filterQuery)
+
+          // getDocs(filterQuery) will not resolve on the snapshot from cache
+          .expectEvents(filterQuery, {
+            fromCache: true,
+            added: [docC]
+          })
+
+          // Watch filter query
+          .watchAcks(filterQuery)
+
+          // However, docC is still in limbo because there has not been a global snapshot
+          .expectLimboDocs(docC.key)
+
+          // Rapid events of document update and delete caused by application
+          .watchRemovesDoc(docA.key, filterQuery)
+          .watchCurrents(filterQuery, 'resume-token-1004')
+          .watchSends({ affects: [filterQuery] }, docC)
+          .watchCurrents(filterQuery, 'resume-token-1005')
+          .watchRemovesDoc(docC.key, filterQuery)
+          .watchSends({ affects: [filterQuery] }, docA2)
+          .watchCurrents(filterQuery, 'resume-token-1007')
+
+          .watchSnapshots(1010, [fullQuery, limboQueryC])
+
+          // All changes are current and we get a global snapshot
+          .watchSnapshots(1010, [])
+
+          .expectEvents(fullQuery, {
+            fromCache: true,
+            modified: [docA2]
+          })
+          // Now getDocs(filterQuery) can be resolved
+          .expectEvents(filterQuery, {
+            fromCache: true,
+            added: [docA2]
+          })
+
+          // Watch acks limbo query
+          .watchAcks(limboQueryC)
+
+          // Watch responds to limboQueryC - docC was deleted
+          .watchDeletesDoc(docC.key, 1009, limboQueryC)
+          .watchCurrents(limboQueryC, 'resume-token-1009')
+          .watchSnapshots(1100, [limboQueryC])
+
+          // No more expected events
+          .watchSnapshots(1101, [])
+
+          .expectLimboDocs()
+          .expectEvents(fullQuery, {
+            fromCache: false,
+            removed: [docC]
+          })
+          // Now getDocs(filterQuery) can be resolved
+          .expectEvents(filterQuery, {
+            fromCache: false,
+            removed: [docC]
+          })
+      );
+    }
+  );
 });