Fix for 8474 - 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();
@@ -596,7 +597,7 @@ export class WatchChangeAggregator {
           // 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,278 @@ describeSpec('Limbo Documents:', [], () => {
       );
     }
   );
+
+  specTest(
+    'Fix #8474 - Limbo resolution for documentC is removed even if documentC updates occurred earlier 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)
+          .watchSends({ removed: [fullQuery] }, docC)
+          .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
+          .watchSends({ affects: [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
+          .watchSends({ removed: [filterQuery] }, docA)
+          .watchCurrents(filterQuery, 'resume-token-1004')
+          .watchSends({ affects: [filterQuery] }, docC)
+          .watchCurrents(filterQuery, 'resume-token-1005')
+          .watchSends({ removed: [filterQuery] }, docC)
+          .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]
+          })
+      );
+    }
+  );
+
+  specTest('Fix #8474 - Watch sends deletedDoc on limbo resolution', [], () => {
+    // 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 docCDeleted = deletedDoc('collection/c', 1009);
+
+    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)
+        .watchSends({ removed: [fullQuery] }, docC)
+        .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
+        .watchSends({ affects: [limboQueryC] }, docCDeleted)
+        .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
+        .watchSends({ removed: [filterQuery] }, docA)
+        .watchCurrents(filterQuery, 'resume-token-1004')
+        .watchSends({ affects: [filterQuery] }, docC)
+        .watchCurrents(filterQuery, 'resume-token-1005')
+        .watchSends({ removed: [filterQuery] }, docC)
+        .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 docCDeleted = deletedDoc('collection/c', 1009);
+
+      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)
+          .watchSends({ removed: [fullQuery] }, docC)
+          .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
+          .watchSends({ removed: [filterQuery] }, docA)
+          .watchCurrents(filterQuery, 'resume-token-1004')
+          .watchSends({ affects: [filterQuery] }, docC)
+          .watchCurrents(filterQuery, 'resume-token-1005')
+          .watchSends({ removed: [filterQuery] }, docC)
+          .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
+          .watchSends({ affects: [limboQueryC] }, docCDeleted)
+          .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]
+          })
+      );
+    }
+  );
 });

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

@@ -315,6 +315,15 @@ export class SpecBuilder {
     return this;
   }
 
+  /** Listen to query using the same options as executing a getDoc or getDocs */
+  userListensForGet(query: Query, resume?: ResumeSpec): this {
+    this.addUserListenStep(query, resume, {
+      includeMetadataChanges: true,
+      waitForSyncWhenOnline: true
+    });
+    return this;
+  }
+
   userListensToCache(query: Query, resume?: ResumeSpec): this {
     this.addUserListenStep(query, resume, { source: Source.Cache });
     return this;