Fix docs unnecessarily undergo limbo resolution while resuming query bug (#7740)

Author: milaGGL

.changeset/olive-dogs-play.md

@@ -0,0 +1,7 @@
+---
+'@firebase/firestore': patch
+'firebase': patch
+---
+
+Fixed an issue in the local cache synchronization logic where all locally-cached documents that matched a resumed query would be unnecessarily re-downloaded; with the fix it now only downloads the documents that are known to be out-of-sync.
+

packages/firestore/src/core/firestore_client.ts

@@ -729,7 +729,7 @@ async function executeQueryFromCache(
     const viewDocChanges = view.computeDocChanges(queryResult.documents);
     const viewChange = view.applyChanges(
       viewDocChanges,
-      /* updateLimboDocuments= */ false
+      /* limboResolutionEnabled= */ false
     );
     result.resolve(viewChange.snapshot!);
   } catch (e) {

packages/firestore/src/core/sync_engine_impl.ts

@@ -368,7 +368,7 @@ async function initializeViewAndComputeSnapshot(
     );
   const viewChange = view.applyChanges(
     viewDocChanges,
-    /* updateLimboDocuments= */ syncEngineImpl.isPrimaryClient,
+    /* limboResolutionEnabled= */ syncEngineImpl.isPrimaryClient,
     synthesizedTargetChange
   );
   updateTrackedLimbos(syncEngineImpl, targetId, viewChange.limboChanges);
@@ -1081,10 +1081,13 @@ async function applyDocChanges(
 
   const targetChange =
     remoteEvent && remoteEvent.targetChanges.get(queryView.targetId);
+  const targetIsPendingReset =
+    remoteEvent && remoteEvent.targetMismatches.get(queryView.targetId) != null;
   const viewChange = queryView.view.applyChanges(
     viewDocChanges,
-    /* updateLimboDocuments= */ syncEngineImpl.isPrimaryClient,
-    targetChange
+    /* limboResolutionEnabled= */ syncEngineImpl.isPrimaryClient,
+    targetChange,
+    targetIsPendingReset
   );
   updateTrackedLimbos(
     syncEngineImpl,

packages/firestore/src/core/view.ts

@@ -271,17 +271,21 @@ export class View {
    * Updates the view with the given ViewDocumentChanges and optionally updates
    * limbo docs and sync state from the provided target change.
    * @param docChanges - The set of changes to make to the view's docs.
-   * @param updateLimboDocuments - Whether to update limbo documents based on
+   * @param limboResolutionEnabled - Whether to update limbo documents based on
    *        this change.
    * @param targetChange - A target change to apply for computing limbo docs and
    *        sync state.
+   * @param targetIsPendingReset - Whether the target is pending to reset due to
+   *        existence filter mismatch. If not explicitly specified, it is treated
+   *        equivalently to `false`.
    * @returns A new ViewChange with the given docs, changes, and sync state.
    */
   // PORTING NOTE: The iOS/Android clients always compute limbo document changes.
   applyChanges(
     docChanges: ViewDocumentChanges,
-    updateLimboDocuments: boolean,
-    targetChange?: TargetChange
+    limboResolutionEnabled: boolean,
+    targetChange?: TargetChange,
+    targetIsPendingReset?: boolean
   ): ViewChange {
     debugAssert(
       !docChanges.needsRefill,
@@ -300,10 +304,18 @@ export class View {
     });
 
     this.applyTargetChange(targetChange);
-    const limboChanges = updateLimboDocuments
-      ? this.updateLimboDocuments()
-      : [];
-    const synced = this.limboDocuments.size === 0 && this.current;
+
+    targetIsPendingReset = targetIsPendingReset ?? false;
+    const limboChanges =
+      limboResolutionEnabled && !targetIsPendingReset
+        ? this.updateLimboDocuments()
+        : [];
+
+    // We are at synced state if there is no limbo docs are waiting to be resolved, view is current
+    // with the backend, and the query is not pending to reset due to existence filter mismatch.
+    const synced =
+      this.limboDocuments.size === 0 && this.current && !targetIsPendingReset;
+
     const newSyncState = synced ? SyncState.Synced : SyncState.Local;
     const syncStateChanged = newSyncState !== this.syncState;
     this.syncState = newSyncState;
@@ -350,7 +362,7 @@ export class View {
           mutatedKeys: this.mutatedKeys,
           needsRefill: false
         },
-        /* updateLimboDocuments= */ false
+        /* limboResolutionEnabled= */ false
       );
     } else {
       // No effect, just return a no-op ViewChange.
@@ -458,7 +470,7 @@ export class View {
     this._syncedDocuments = queryResult.remoteKeys;
     this.limboDocuments = documentKeySet();
     const docChanges = this.computeDocChanges(queryResult.documents);
-    return this.applyChanges(docChanges, /*updateLimboDocuments=*/ true);
+    return this.applyChanges(docChanges, /* limboResolutionEnabled= */ true);
   }
 
   /**

packages/firestore/test/unit/core/view.test.ts

@@ -304,19 +304,19 @@ describe('View', () => {
     let changes = view.computeDocChanges(documentUpdates(doc1));
     let viewChange = view.applyChanges(
       changes,
-      /* updateLimboDocuments= */ true
+      /* limboResolutionEnabled= */ true
     );
     expect(viewChange.snapshot!.fromCache).to.be.true;
 
     // Add doc2 to generate a snapshot. Doc1 is still missing.
     changes = view.computeDocChanges(documentUpdates(doc2));
-    viewChange = view.applyChanges(changes, /* updateLimboDocuments= */ true);
+    viewChange = view.applyChanges(changes, /* limboResolutionEnabled= */ true);
     expect(viewChange.snapshot!.fromCache).to.be.true;
 
     // Add doc2 to the backend's result set.
     viewChange = view.applyChanges(
       changes,
-      /* updateLimboDocuments= */ true,
+      /* limboResolutionEnabled= */ true,
       updateMapping(version(0), [doc2], [], [], /* current= */ true)
     );
     // We are CURRENT but doc1 is in limbo.
@@ -325,7 +325,7 @@ describe('View', () => {
     // Add doc1 to the backend's result set.
     viewChange = view.applyChanges(
       changes,
-      /* updateLimboDocuments= */ true,
+      /* limboResolutionEnabled= */ true,
       updateMapping(version(0), [doc1], [], [], /* current= */ true)
     );
     expect(viewChange.snapshot!.fromCache).to.be.false;

packages/firestore/test/unit/local/query_engine.test.ts

@@ -253,7 +253,7 @@ function genericQueryEngineTest(
               const viewDocChanges = view.computeDocChanges(docs);
               return view.applyChanges(
                 viewDocChanges,
-                /*updateLimboDocuments=*/ true
+                /* limboResolutionEnabled= */ true
               ).snapshot!.docs;
             });
         });

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

@@ -889,6 +889,7 @@ describeSpec('Limbo Documents:', [], () => {
           // to just send the docBs with an existence filter with a count of 3.
           .watchSends({ affects: [query1] }, docB1, docB2, docB3)
           .watchFilters([query1], [docB1.key, docB2.key, docB3.key])
+          .watchCurrents(query1, 'resume-token-1001')
           .watchSnapshots(1001)
           .expectEvents(query1, {
             added: [docB1, docB2, docB3],
@@ -927,6 +928,46 @@ describeSpec('Limbo Documents:', [], () => {
     }
   );
 
+  // Regression test for the bug in https://github.com/firebase/firebase-android-sdk/issues/5357
+  specTest(
+    'Limbo resolution should wait for full re-query result if there is an existence filter mismatch ',
+    [],
+    () => {
+      const query1 = query('collection');
+      const docA = doc('collection/a', 1000, { v: 1 });
+      const docB = doc('collection/b', 1000, { v: 2 });
+      return (
+        spec()
+          .userListens(query1)
+          .watchAcksFull(query1, 1000, docA, docB)
+          .expectEvents(query1, { added: [docA, docB] })
+          // Simulate that the client loses network connection.
+          .disableNetwork()
+          .expectEvents(query1, { fromCache: true })
+          .enableNetwork()
+          .restoreListen(query1, 'resume-token-1000')
+          .watchAcks(query1)
+          // DocB is deleted in the next sync.
+          .watchFilters([query1], [docA.key])
+          .watchCurrents(query1, 'resume-token-2000')
+          .watchSnapshots(2000)
+          .expectActiveTargets({
+            query: query1,
+            resumeToken: '',
+            targetPurpose: TargetPurpose.ExistenceFilterMismatch
+          })
+          .watchRemoves(query1)
+          .watchAcksFull(query1, 3000, docA)
+          // Only the deleted doc is moved to limbo after re-query result.
+          .expectLimboDocs(docB.key)
+          .ackLimbo(3000, deletedDoc(docB.key.toString(), 3000))
+          .expectLimboDocs()
+          .expectEvents(query1, {
+            removed: [docB]
+          })
+      );
+    }
+  );
   specTest(
     'Limbo resolution throttling with bloom filter application',
     [],
@@ -968,6 +1009,7 @@ describeSpec('Limbo Documents:', [], () => {
             [docB1.key, docB2.key, docB3.key],
             bloomFilterProto
           )
+          .watchCurrents(query1, 'resume-token-1001')
           .watchSnapshots(1001)
           .expectEvents(query1, {
             added: [docB1, docB2, docB3],
@@ -978,8 +1020,6 @@ describeSpec('Limbo Documents:', [], () => {
           // existence filter mismatch. Bloom filter checks membership of the
           // docs, and filters out docAs, while docBs returns true. Number of
           // existing docs matches the expected count, so skip the re-query.
-          .watchCurrents(query1, 'resume-token-1002')
-          .watchSnapshots(1002)
           // The docAs are now in limbo; the client begins limbo resolution.
           .expectLimboDocs(docA1.key, docA2.key)
           .expectEnqueuedLimboDocs(docA3.key)

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

@@ -343,6 +343,7 @@ describeSpec('Limits:', [], () => {
           // out of sync.
           .watchSends({ affects: [limitQuery] }, secondDocument)
           .watchFilters([limitQuery], [secondDocument.key])
+          .watchCurrents(limitQuery, 'resume-token-1004')
           .watchSnapshots(1004)
           .expectActiveTargets({
             query: limitQuery,