b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes.

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)
+      );
+    }
+  );
 });