Clear the lastLimboFreeSnapshot version on existence filter mismatch (#5835)

Author: schmidt-sebastian

.changeset/unlucky-files-smoke.md

@@ -0,0 +1,5 @@
+---
+"@firebase/firestore": patch
+---
+
+Fixed an issue that can cause incomplete Query snapshots when the SDK is backgrounded during query execution.

packages/firestore/src/local/local_store_impl.ts

@@ -501,24 +501,34 @@ export function localStoreApplyRemoteEventToLocalCache(
             })
         );
 
-        const resumeToken = change.resumeToken;
-        // Update the resume token if the change includes one.
-        if (resumeToken.approximateByteSize() > 0) {
-          const newTargetData = oldTargetData
-            .withResumeToken(resumeToken, remoteVersion)
-            .withSequenceNumber(txn.currentSequenceNumber);
-          newTargetDataByTargetMap = newTargetDataByTargetMap.insert(
-            targetId,
-            newTargetData
+        let newTargetData = oldTargetData.withSequenceNumber(
+          txn.currentSequenceNumber
+        );
+        if (remoteEvent.targetMismatches.has(targetId)) {
+          newTargetData = newTargetData
+            .withResumeToken(
+              ByteString.EMPTY_BYTE_STRING,
+              SnapshotVersion.min()
+            )
+            .withLastLimboFreeSnapshotVersion(SnapshotVersion.min());
+        } else if (change.resumeToken.approximateByteSize() > 0) {
+          newTargetData = newTargetData.withResumeToken(
+            change.resumeToken,
+            remoteVersion
           );
+        }
 
-          // Update the target data if there are target changes (or if
-          // sufficient time has passed since the last update).
-          if (shouldPersistTargetData(oldTargetData, newTargetData, change)) {
-            promises.push(
-              localStoreImpl.targetCache.updateTargetData(txn, newTargetData)
-            );
-          }
+        newTargetDataByTargetMap = newTargetDataByTargetMap.insert(
+          targetId,
+          newTargetData
+        );
+
+        // Update the target data if there are target changes (or if
+        // sufficient time has passed since the last update).
+        if (shouldPersistTargetData(oldTargetData, newTargetData, change)) {
+          promises.push(
+            localStoreImpl.targetCache.updateTargetData(txn, newTargetData)
+          );
         }
       });
 
@@ -675,11 +685,6 @@ function shouldPersistTargetData(
   newTargetData: TargetData,
   change: TargetChange
 ): boolean {
-  hardAssert(
-    newTargetData.resumeToken.approximateByteSize() > 0,
-    'Attempted to persist target data with no resume token'
-  );
-
   // Always persist target data if we don't already have a resume token.
   if (oldTargetData.resumeToken.approximateByteSize() === 0) {
     return true;

packages/firestore/test/unit/generate_spec_json.sh

@@ -24,8 +24,8 @@ NPM_BIN_DIR="$(npm bin)"
 TSNODE="$NPM_BIN_DIR/ts-node "
 GENERATE_SPEC_JS="$DIR/generate_spec_json.js"
 
-export TS_NODE_CACHE=NO 
-export TS_NODE_COMPILER_OPTIONS='{"module":"commonjs"}' 
+export TS_NODE_CACHE=NO
+export TS_NODE_COMPILER_OPTIONS='{"module":"commonjs"}'
 export TS_NODE_PROJECT="$DIR/../../tsconfig.json"
 
-$TSNODE --require ../../index.node.ts $GENERATE_SPEC_JS "$@"
+$TSNODE --require ../../src/index.node.ts $GENERATE_SPEC_JS "$@"

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

@@ -93,6 +93,7 @@ import {
   doc,
   docAddedRemoteEvent,
   docUpdateRemoteEvent,
+  existenceFilterEvent,
   expectEqual,
   filter,
   key,
@@ -130,6 +131,7 @@ class LocalStoreTester {
 
   constructor(
     public localStore: LocalStore,
+    private readonly persistence: Persistence,
     private readonly queryEngine: CountingQueryEngine,
     readonly gcIsEager: boolean
   ) {
@@ -379,6 +381,30 @@ class LocalStoreTester {
     return this;
   }
 
+  toContainTargetData(
+    target: Target,
+    snapshotVersion: number,
+    lastLimboFreeSnapshotVersion: number,
+    resumeToken: ByteString
+  ): LocalStoreTester {
+    this.promiseChain = this.promiseChain.then(async () => {
+      const targetData = await this.persistence.runTransaction(
+        'getTargetData',
+        'readonly',
+        txn => localStoreGetTargetData(this.localStore, txn, target)
+      );
+      expect(targetData!.snapshotVersion.isEqual(version(snapshotVersion))).to
+        .be.true;
+      expect(
+        targetData!.lastLimboFreeSnapshotVersion.isEqual(
+          version(lastLimboFreeSnapshotVersion)
+        )
+      ).to.be.true;
+      expect(targetData!.resumeToken.isEqual(resumeToken)).to.be.true;
+    });
+    return this;
+  }
+
   toReturnChanged(...docs: Document[]): LocalStoreTester {
     this.promiseChain = this.promiseChain.then(() => {
       debugAssert(
@@ -583,7 +609,12 @@ function genericLocalStoreTests(
   });
 
   function expectLocalStore(): LocalStoreTester {
-    return new LocalStoreTester(localStore, queryEngine, gcIsEager);
+    return new LocalStoreTester(
+      localStore,
+      persistence,
+      queryEngine,
+      gcIsEager
+    );
   }
 
   it('handles SetMutation', () => {
@@ -1876,6 +1907,47 @@ function genericLocalStoreTests(
     }
   });
 
+  // eslint-disable-next-line no-restricted-properties
+  (gcIsEager ? it.skip : it)(
+    'ignores target mapping after existence filter mismatch',
+    async () => {
+      const query1 = query('foo', filter('matches', '==', true));
+      const target = queryToTarget(query1);
+      const targetId = 2;
+
+      return (
+        expectLocalStore()
+          .afterAllocatingQuery(query1)
+          .toReturnTargetId(targetId)
+          // Persist a mapping with a single document
+          .after(
+            docAddedRemoteEvent(
+              doc('foo/a', 10, { matches: true }),
+              [targetId],
+              [],
+              [targetId]
+            )
+          )
+          .after(noChangeEvent(targetId, 10, byteStringFromString('foo')))
+          .after(localViewChanges(targetId, /* fromCache= */ false, {}))
+          .afterExecutingQuery(query1)
+          .toReturnChanged(doc('foo/a', 10, { matches: true }))
+          .toHaveRead({ documentsByKey: 1 })
+          .toContainTargetData(target, 10, 10, byteStringFromString('foo'))
+          // Create an existence filter mismatch and verify that the last limbo
+          // free snapshot version is deleted
+          .after(existenceFilterEvent(targetId, 2, 20))
+          .after(noChangeEvent(targetId, 20))
+          .toContainTargetData(target, 0, 0, ByteString.EMPTY_BYTE_STRING)
+          // Re-run the query as a collection scan
+          .afterExecutingQuery(query1)
+          .toReturnChanged(doc('foo/a', 10, { matches: true }))
+          .toHaveRead({ documentsByQuery: 1 })
+          .finish()
+      );
+    }
+  );
+
   // eslint-disable-next-line no-restricted-properties
   (gcIsEager ? it.skip : it)(
     'queries include locally modified documents',

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

@@ -211,4 +211,33 @@ describeSpec('Existence Filters:', [], () => {
         })
     );
   });
+
+  specTest(
+    'Existence filter clears resume token',
+    ['durable-persistence'],
+    () => {
+      // This is a test for https://github.com/firebase/firebase-android-sdk/issues/3249
+      // In this particular scenario, the user received an existence filter
+      // mismatch, but the SDK only cleared the target-to-document mapping and
+      // not the lastLimboFreeSnapshot version. This caused the SDK to resume
+      // the query but not include old documents.
+      const query1 = query('collection');
+      const doc1 = doc('collection/1', 1000, { v: 1 });
+      const doc2 = doc('collection/2', 1000, { v: 2 });
+      return (
+        spec()
+          .userListens(query1)
+          .watchAcksFull(query1, 1000, doc1, doc2)
+          .expectEvents(query1, { added: [doc1, doc2] })
+          .watchFilters([query1], doc1.key) // doc2 was deleted
+          .watchSnapshots(2000)
+          .expectEvents(query1, { fromCache: true })
+          // The SDK is unable to re-run the query, and does not remove doc2
+          .restart()
+          .userListens(query1)
+          // We check that the data is still consistent with the local cache
+          .expectEvents(query1, { added: [doc1, doc2], fromCache: true })
+      );
+    }
+  );
 });

packages/firestore/test/util/helpers.ts

@@ -87,6 +87,7 @@ import {
   LimitType as ProtoLimitType
 } from '../../src/protos/firestore_bundle_proto';
 import * as api from '../../src/protos/firestore_proto_api';
+import { ExistenceFilter } from '../../src/remote/existence_filter';
 import { RemoteEvent, TargetChange } from '../../src/remote/remote_event';
 import {
   JsonProtoSerializer,
@@ -98,6 +99,7 @@ import {
 } from '../../src/remote/serializer';
 import {
   DocumentWatchChange,
+  ExistenceFilterChange,
   WatchChangeAggregator,
   WatchTargetChange,
   WatchTargetChangeState
@@ -352,6 +354,22 @@ export function noChangeEvent(
   return aggregator.createRemoteEvent(version(snapshotVersion));
 }
 
+export function existenceFilterEvent(
+  targetId: number,
+  count: number,
+  snapshotVersion: number
+): RemoteEvent {
+  const aggregator = new WatchChangeAggregator({
+    getRemoteKeysForTarget: () => documentKeySet(),
+    getTargetDataForTarget: targetId =>
+      targetData(targetId, TargetPurpose.Listen, 'foo')
+  });
+  aggregator.handleExistenceFilter(
+    new ExistenceFilterChange(targetId, new ExistenceFilter(count))
+  );
+  return aggregator.createRemoteEvent(version(snapshotVersion));
+}
+
 export function docAddedRemoteEvent(
   docOrDocs: MutableDocument | MutableDocument[],
   updatedInTargets?: TargetId[],