b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. (#1014)

This fixes an issue occurring when a limbo target receives a documentUpdate,
then a global snapshot, and then a CURRENT. Because there was a global
snapshot before the CURRENT, WatchChangeAggregator has no pending document
updates and calls SyncEngine.targetContainsDocument() to see if we previously got any
document from the backend for the target. See:
https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422

Prior to this change, targetContainsDocument() returned false because
it relies on our Views to track the contents of the target, and we don't
have Views for limbo targets. Thus WatchChangeAggregator incorrectly
manufactures a NoDocument document update which deletes data from our
cache.

The fix is to have SyncEngine track the fact that we did indeed get
a document for the limbo resolution and return true from
targetContainsDocument().

Author: mikelehen

packages/firestore/src/core/sync_engine.ts

@@ -36,6 +36,7 @@ import { assert, fail } from '../util/assert';
 import { FirestoreError } from '../util/error';
 import * as log from '../util/log';
 import { AnyJs, primitiveComparator } from '../util/misc';
+import * as objUtils from '../util/obj';
 import { ObjectMap } from '../util/obj_map';
 import { Deferred } from '../util/promise';
 import { SortedMap } from '../util/sorted_map';
@@ -92,6 +93,19 @@ class QueryView {
   ) {}
 }
 
+/** Tracks a limbo resolution. */
+class LimboResolution {
+  constructor(public key: DocumentKey) {}
+
+  /**
+   * Set to true once we've received a document. This is used in
+   * targetContainsDocument() and ultimately used by WatchChangeAggregator to
+   * decide whether it needs to manufacture a delete event for the target once
+   * the target is CURRENT.
+   */
+  receivedDocument: boolean;
+}
+
 /**
  * SyncEngine is the central controller in the client SDK architecture. It is
  * the glue code between the EventManager, LocalStore, and RemoteStore. Some of
@@ -117,7 +131,9 @@ export class SyncEngine implements RemoteSyncer {
   private limboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
     DocumentKey.comparator
   );
-  private limboKeysByTarget: { [targetId: number]: DocumentKey } = {};
+  private limboResolutionsByTarget: {
+    [targetId: number]: LimboResolution;
+  } = {};
   private limboDocumentRefs = new ReferenceSet();
   private limboCollector = new EagerGarbageCollector();
   /** Stores user completion handlers, indexed by User and BatchId. */
@@ -301,6 +317,38 @@ export class SyncEngine implements RemoteSyncer {
   applyRemoteEvent(remoteEvent: RemoteEvent): Promise<void> {
     this.assertSubscribed('applyRemoteEvent()');
 
+    // Update `receivedDocument` as appropriate for any limbo targets.
+    objUtils.forEach(remoteEvent.targetChanges, (targetId, targetChange) => {
+      const limboResolution = this.limboResolutionsByTarget[targetId];
+      if (limboResolution) {
+        // Since this is a limbo resolution lookup, it's for a single document
+        // and it could be added, modified, or removed, but not a combination.
+        assert(
+          targetChange.addedDocuments.size +
+            targetChange.modifiedDocuments.size +
+            targetChange.removedDocuments.size <=
+            1,
+          'Limbo resolution for single document contains multiple changes.'
+        );
+        if (targetChange.addedDocuments.size > 0) {
+          limboResolution.receivedDocument = true;
+        } else if (targetChange.modifiedDocuments.size > 0) {
+          assert(
+            limboResolution.receivedDocument,
+            'Received change for limbo target document without add.'
+          );
+        } else if (targetChange.removedDocuments.size > 0) {
+          assert(
+            limboResolution.receivedDocument,
+            'Received remove for limbo target document without add.'
+          );
+          limboResolution.receivedDocument = false;
+        } else {
+          // This was probably just a CURRENT targetChange or similar.
+        }
+      }
+    });
+
     return this.localStore.applyRemoteEvent(remoteEvent).then(changes => {
       return this.emitNewSnapsAndNotifyLocalStore(changes, remoteEvent);
     });
@@ -327,12 +375,13 @@ export class SyncEngine implements RemoteSyncer {
 
   rejectListen(targetId: TargetId, err: FirestoreError): Promise<void> {
     this.assertSubscribed('rejectListens()');
-    const limboKey = this.limboKeysByTarget[targetId];
+    const limboResolution = this.limboResolutionsByTarget[targetId];
+    const limboKey = limboResolution && limboResolution.key;
     if (limboKey) {
       // Since this query failed, we won't want to manually unlisten to it.
       // So go ahead and remove it from bookkeeping.
       this.limboTargetsByKey = this.limboTargetsByKey.remove(limboKey);
-      delete this.limboKeysByTarget[targetId];
+      delete this.limboResolutionsByTarget[targetId];
 
       // TODO(klimt): We really only should do the following on permission
       // denied errors, but we don't have the cause code here.
@@ -476,7 +525,7 @@ export class SyncEngine implements RemoteSyncer {
       log.debug(LOG_TAG, 'New document in limbo: ' + key);
       const limboTargetId = this.targetIdGenerator.next();
       const query = Query.atPath(key.path);
-      this.limboKeysByTarget[limboTargetId] = key;
+      this.limboResolutionsByTarget[limboTargetId] = new LimboResolution(key);
       this.remoteStore.listen(
         new QueryData(query, limboTargetId, QueryPurpose.LimboResolution)
       );
@@ -501,7 +550,7 @@ export class SyncEngine implements RemoteSyncer {
           }
           this.remoteStore.unlisten(limboTargetId);
           this.limboTargetsByKey = this.limboTargetsByKey.remove(key);
-          delete this.limboKeysByTarget[limboTargetId];
+          delete this.limboResolutionsByTarget[limboTargetId];
         });
       })
       .toPromise();
@@ -588,8 +637,13 @@ export class SyncEngine implements RemoteSyncer {
   }
 
   getRemoteKeysForTarget(targetId: TargetId): DocumentKeySet {
-    return this.queryViewsByTarget[targetId]
-      ? this.queryViewsByTarget[targetId].view.syncedDocuments
-      : documentKeySet();
+    const limboResolution = this.limboResolutionsByTarget[targetId];
+    if (limboResolution && limboResolution.receivedDocument) {
+      return documentKeySet().add(limboResolution.key);
+    } else {
+      return this.queryViewsByTarget[targetId]
+        ? this.queryViewsByTarget[targetId].view.syncedDocuments
+        : documentKeySet();
+    }
   }
 }

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

@@ -188,7 +188,7 @@ describeSpec('Limbo Documents:', [], () => {
   });
 
   // Regression test for b/72533250.
-  specTest('Limbo documents handle receiving ack and then current', [], () => {
+  specTest('Limbo resolution handles snapshot before CURRENT', [], () => {
     const fullQuery = Query.atPath(path('collection'));
     const limitQuery = Query.atPath(path('collection'))
       .addFilter(filter('include', '==', true))
@@ -236,10 +236,7 @@ describeSpec('Limbo Documents:', [], () => {
         .watchAcks(docBQuery)
         .watchSends({ affects: [docBQuery] }, docB)
 
-        // TODO(b/72533250): If you uncomment this totally valid watch
-        // snapshot, then the test fails because the subsequent CURRENT below
-        // is turned into a delete of docB.
-        //.watchSnapshots(2000)
+        .watchSnapshots(2000)
 
         // Additionally CURRENT the query (should have no effect)
         .watchCurrents(docBQuery, 'resume-token-3000')
@@ -254,4 +251,72 @@ describeSpec('Limbo Documents:', [], () => {
         .expectLimboDocs()
     );
   });
+
+  // Same as above test, except docB no longer exists so we do not get a
+  // documentUpdate for it during limbo resolution, so a delete should be
+  // synthesized.
+  specTest(
+    'Limbo resolution handles snapshot before CURRENT [no document update]',
+    [],
+    () => {
+      const fullQuery = Query.atPath(path('collection'));
+      const limitQuery = Query.atPath(path('collection'))
+        .addFilter(filter('include', '==', true))
+        .withLimit(1);
+      const docA = doc('collection/a', 1000, { key: 'a', include: true });
+      const docB = doc('collection/b', 1000, { key: 'b', include: true });
+      const docBQuery = Query.atPath(docB.key.path);
+      return (
+        spec()
+          // No GC so we can keep the cache populated.
+          .withGCEnabled(false)
+
+          // Full query to populate the cache with docA and docB
+          .userListens(fullQuery)
+          .watchAcksFull(fullQuery, 1000, docA, docB)
+          .expectEvents(fullQuery, {
+            added: [docA, docB]
+          })
+          .userUnlistens(fullQuery)
+
+          // Perform limit(1) query.
+          .userListens(limitQuery)
+          .expectEvents(limitQuery, {
+            added: [docA],
+            fromCache: true
+          })
+          .watchAcksFull(limitQuery, 2000, docA)
+          .expectEvents(limitQuery, {
+            fromCache: false
+          })
+
+          // Edit docA so it no longer matches the query and we pull in docB
+          // from cache.
+          .userPatches('collection/a', { include: false })
+          .expectEvents(limitQuery, {
+            removed: [docA],
+            added: [docB],
+            fromCache: true
+          })
+          // docB is in limbo since we haven't gotten the watch update to pull
+          // it in yet.
+          .expectLimboDocs(docB.key)
+
+          // Suppose docB was actually deleted server-side and so we receive an
+          // ack, a snapshot, CURRENT, and then another snapshot. This should
+          // resolve the limbo resolution and docB should disappear.
+          .watchAcks(docBQuery)
+          .watchSnapshots(2000)
+          .watchCurrents(docBQuery, 'resume-token-3000')
+          .watchSnapshots(3000)
+          .expectLimboDocs()
+          .expectEvents(limitQuery, { removed: [docB], fromCache: false })
+
+          // Watch catches up to the local write to docA, and broadcasts its
+          // removal.
+          .watchSends({ removed: [limitQuery] }, docA)
+          .watchSnapshots(4000)
+      );
+    }
+  );
 });