From 448c522a1b538817544194a6882a3e529815de7a Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Tue, 17 Jul 2018 15:07:56 -0700
Subject: [PATCH 01/12] Write a spec test for the busted cache

---
 .../test/unit/specs/listen_spec.test.ts       | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts
index 42da894ff7b..1f8b7b470e2 100644
--- a/packages/firestore/test/unit/specs/listen_spec.test.ts
+++ b/packages/firestore/test/unit/specs/listen_spec.test.ts
@@ -283,6 +283,37 @@ describeSpec('Listens:', [], () => {
     );
   });
 
+  specTest('Deleted documents in cache are fixed', [], () => {
+    const allQuery = Query.atPath(path('collection'));
+    const docAv1 = doc('collection/a', 1000, { key: 'a' });
+    const docDeleted = deletedDoc('collection/a', 2000);
+
+    return (
+      spec()
+        // Presuppose an initial state where the remote document cache has a
+        // broken synthesized delete at a timestamp later than the true version
+        // of the document. This requires both adding and later removing the
+        // document in order to force the watch change aggregator to propagate
+        // the deletion.
+        .withGCEnabled(false)
+        .userListens(allQuery)
+        .watchAcksFull(allQuery, 1000, docAv1)
+        .expectEvents(allQuery, { added: [docAv1], fromCache: false })
+        .watchSends({ removed: [allQuery] }, docDeleted)
+        .watchSnapshots(2000, [allQuery], 'resume-token-2000')
+        .watchSnapshots(2000)
+        .expectEvents(allQuery, { removed: [docAv1], fromCache: false })
+        .userUnlistens(allQuery)
+        .watchRemoves(allQuery)
+
+        // Now when the client listens expect the cached NoDocument to be
+        // discarded because the global snapshot version exceeds what came before.
+        .userListens(allQuery, 'resume-token-2000')
+        .watchAcksFull(allQuery, 3000, docAv1)
+        .expectEvents(allQuery, { added: [docAv1], fromCache: false })
+    );
+  });
+
   specTest('Listens are reestablished after network disconnect', [], () => {
     const expectRequestCount = requestCounts =>
       requestCounts.addTarget + requestCounts.removeTarget;

From 6ce8ed4f6665dc1fdb9b69178ffb2e70302224d9 Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Tue, 17 Jul 2018 13:57:49 -0700
Subject: [PATCH 02/12] Relax individual document revert constraints

---
 packages/firestore/src/local/local_store.ts | 41 ++++-----------------
 1 file changed, 7 insertions(+), 34 deletions(-)

diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts
index e944f36b707..fd0333aa6b7 100644
--- a/packages/firestore/src/local/local_store.ts
+++ b/packages/firestore/src/local/local_store.ts
@@ -458,40 +458,6 @@ export class LocalStore {
         }
       );
 
-      let changedDocKeys = documentKeySet();
-      remoteEvent.documentUpdates.forEach((key, doc) => {
-        changedDocKeys = changedDocKeys.add(key);
-        promises.push(
-          documentBuffer.getEntry(txn, key).next(existingDoc => {
-            // Make sure we don't apply an old document version to the remote
-            // cache, though we make an exception for SnapshotVersion.MIN which
-            // can happen for manufactured events (e.g. in the case of a limbo
-            // document resolution failing).
-            if (
-              existingDoc == null ||
-              doc.version.isEqual(SnapshotVersion.MIN) ||
-              doc.version.compareTo(existingDoc.version) >= 0
-            ) {
-              documentBuffer.addEntry(doc);
-            } else {
-              log.debug(
-                LOG_TAG,
-                'Ignoring outdated watch update for ',
-                key,
-                '. Current version:',
-                existingDoc.version,
-                ' Watch version:',
-                doc.version
-              );
-            }
-
-            // The document might be garbage because it was unreferenced by
-            // everything. Make sure to mark it as garbage if it is...
-            this.garbageCollector.addPotentialGarbageKey(key);
-          })
-        );
-      });
-
       // HACK: The only reason we allow a null snapshot version is so that we
       // can synthesize remote events when we get permission denied errors while
       // trying to resolve the state of a locally cached document that is in
@@ -511,6 +477,13 @@ export class LocalStore {
         );
       }
 
+      let changedDocKeys = documentKeySet();
+      remoteEvent.documentUpdates.forEach((key, doc) => {
+        changedDocKeys = changedDocKeys.add(key);
+        documentBuffer.addEntry(doc);
+        this.garbageCollector.addPotentialGarbageKey(key);
+      });
+
       let releasedWriteKeys: DocumentKeySet;
       return PersistencePromise.waitFor(promises)
         .next(() => this.releaseHeldBatchResults(txn, documentBuffer))

From 44850e53e563e18ec8d87ef9ec541edcbc7003ba Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Tue, 17 Jul 2018 13:58:16 -0700
Subject: [PATCH 03/12] Use a more realistic scenario for watch sending a
 version back in time

---
 packages/firestore/test/unit/specs/listen_spec.test.ts | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts
index 1f8b7b470e2..7d51cb481d9 100644
--- a/packages/firestore/test/unit/specs/listen_spec.test.ts
+++ b/packages/firestore/test/unit/specs/listen_spec.test.ts
@@ -271,14 +271,19 @@ describeSpec('Listens:', [], () => {
         // (the document falls out) and send us a snapshot that's ahead of
         // docAv3 (which is already in our cache).
         .userListens(visibleQuery, 'resume-token-1000')
-        .watchAcksFull(visibleQuery, 5000, docAv2)
+        .watchAcks(visibleQuery)
+        .watchSends({ affects: [visibleQuery] }, docAv2)
+        .watchSnapshots(5000)
+        .watchSends({ affects: [visibleQuery] }, docAv3)
+        .watchCurrents(visibleQuery, 'resume-token-5000')
+        .watchSnapshots(6000)
         .expectEvents(visibleQuery, { fromCache: false })
         .userUnlistens(visibleQuery)
         .watchRemoves(visibleQuery)
         // Listen to allQuery again and make sure we still get docAv3.
         .userListens(allQuery, 'resume-token-4000')
         .expectEvents(allQuery, { added: [docAv3], fromCache: true })
-        .watchAcksFull(allQuery, 6000)
+        .watchAcksFull(allQuery, 7000)
         .expectEvents(allQuery, { fromCache: false })
     );
   });

From 9bc9ba4d9f66fa4dcf927dfc3636b8e82938939e Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Tue, 17 Jul 2018 16:38:39 -0700
Subject: [PATCH 04/12] Revert "Use a more realistic scenario for watch sending
 a version back in time"

This reverts commit 44850e53e563e18ec8d87ef9ec541edcbc7003ba.
---
 packages/firestore/test/unit/specs/listen_spec.test.ts | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts
index 7d51cb481d9..1f8b7b470e2 100644
--- a/packages/firestore/test/unit/specs/listen_spec.test.ts
+++ b/packages/firestore/test/unit/specs/listen_spec.test.ts
@@ -271,19 +271,14 @@ describeSpec('Listens:', [], () => {
         // (the document falls out) and send us a snapshot that's ahead of
         // docAv3 (which is already in our cache).
         .userListens(visibleQuery, 'resume-token-1000')
-        .watchAcks(visibleQuery)
-        .watchSends({ affects: [visibleQuery] }, docAv2)
-        .watchSnapshots(5000)
-        .watchSends({ affects: [visibleQuery] }, docAv3)
-        .watchCurrents(visibleQuery, 'resume-token-5000')
-        .watchSnapshots(6000)
+        .watchAcksFull(visibleQuery, 5000, docAv2)
         .expectEvents(visibleQuery, { fromCache: false })
         .userUnlistens(visibleQuery)
         .watchRemoves(visibleQuery)
         // Listen to allQuery again and make sure we still get docAv3.
         .userListens(allQuery, 'resume-token-4000')
         .expectEvents(allQuery, { added: [docAv3], fromCache: true })
-        .watchAcksFull(allQuery, 7000)
+        .watchAcksFull(allQuery, 6000)
         .expectEvents(allQuery, { fromCache: false })
     );
   });

From fd9de637a67951d55f243960fc060293ae326301 Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Tue, 17 Jul 2018 16:38:46 -0700
Subject: [PATCH 05/12] Revert "Relax individual document revert constraints"

This reverts commit 6ce8ed4f6665dc1fdb9b69178ffb2e70302224d9.
---
 packages/firestore/src/local/local_store.ts | 41 +++++++++++++++++----
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts
index fd0333aa6b7..e944f36b707 100644
--- a/packages/firestore/src/local/local_store.ts
+++ b/packages/firestore/src/local/local_store.ts
@@ -458,6 +458,40 @@ export class LocalStore {
         }
       );
 
+      let changedDocKeys = documentKeySet();
+      remoteEvent.documentUpdates.forEach((key, doc) => {
+        changedDocKeys = changedDocKeys.add(key);
+        promises.push(
+          documentBuffer.getEntry(txn, key).next(existingDoc => {
+            // Make sure we don't apply an old document version to the remote
+            // cache, though we make an exception for SnapshotVersion.MIN which
+            // can happen for manufactured events (e.g. in the case of a limbo
+            // document resolution failing).
+            if (
+              existingDoc == null ||
+              doc.version.isEqual(SnapshotVersion.MIN) ||
+              doc.version.compareTo(existingDoc.version) >= 0
+            ) {
+              documentBuffer.addEntry(doc);
+            } else {
+              log.debug(
+                LOG_TAG,
+                'Ignoring outdated watch update for ',
+                key,
+                '. Current version:',
+                existingDoc.version,
+                ' Watch version:',
+                doc.version
+              );
+            }
+
+            // The document might be garbage because it was unreferenced by
+            // everything. Make sure to mark it as garbage if it is...
+            this.garbageCollector.addPotentialGarbageKey(key);
+          })
+        );
+      });
+
       // HACK: The only reason we allow a null snapshot version is so that we
       // can synthesize remote events when we get permission denied errors while
       // trying to resolve the state of a locally cached document that is in
@@ -477,13 +511,6 @@ export class LocalStore {
         );
       }
 
-      let changedDocKeys = documentKeySet();
-      remoteEvent.documentUpdates.forEach((key, doc) => {
-        changedDocKeys = changedDocKeys.add(key);
-        documentBuffer.addEntry(doc);
-        this.garbageCollector.addPotentialGarbageKey(key);
-      });
-
       let releasedWriteKeys: DocumentKeySet;
       return PersistencePromise.waitFor(promises)
         .next(() => this.releaseHeldBatchResults(txn, documentBuffer))

From e694eade4327334d5cd1260f131f6af7c9b429db Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Tue, 17 Jul 2018 16:43:07 -0700
Subject: [PATCH 06/12] Disregard NoDocuments when checking against existing
 document versions.

---
 packages/firestore/src/local/local_store.ts | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts
index e944f36b707..54f3b081a5e 100644
--- a/packages/firestore/src/local/local_store.ts
+++ b/packages/firestore/src/local/local_store.ts
@@ -26,7 +26,7 @@ import {
   DocumentMap,
   MaybeDocumentMap
 } from '../model/collections';
-import { MaybeDocument } from '../model/document';
+import { MaybeDocument, NoDocument } from '../model/document';
 import { DocumentKey } from '../model/document_key';
 import { Mutation } from '../model/mutation';
 import {
@@ -467,8 +467,13 @@ export class LocalStore {
             // cache, though we make an exception for SnapshotVersion.MIN which
             // can happen for manufactured events (e.g. in the case of a limbo
             // document resolution failing).
+            //
+            // Also disregard any existing NoDocument. These documents can have
+            // synthesized versions based on read times after any actual
+            // committed version on the server.
             if (
               existingDoc == null ||
+              (existingDoc instanceof NoDocument) ||
               doc.version.isEqual(SnapshotVersion.MIN) ||
               doc.version.compareTo(existingDoc.version) >= 0
             ) {

From 8cbb935b32fc165423761545ecca7adf937cdff7 Mon Sep 17 00:00:00 2001
From: Michael Lehenbauer <mikelehen@gmail.com>
Date: Wed, 18 Jul 2018 07:17:13 -0700
Subject: [PATCH 07/12] Modify spec test to demonstrate deletedDoc issue.
 (#1017)

---
 .../test/unit/specs/listen_spec.test.ts       | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts
index 1f8b7b470e2..a0a5a2d28cf 100644
--- a/packages/firestore/test/unit/specs/listen_spec.test.ts
+++ b/packages/firestore/test/unit/specs/listen_spec.test.ts
@@ -283,6 +283,50 @@ describeSpec('Listens:', [], () => {
     );
   });
 
+  specTest('Individual (deleted) documents cannot revert', [], () => {
+    const allQuery = Query.atPath(path('collection'));
+    const visibleQuery = Query.atPath(path('collection')).addFilter(
+      filter('visible', '==', true)
+    );
+    const docAv1 = doc('collection/a', 1000, { visible: true, v: 'v1000' });
+    const docAv2 = doc('collection/a', 2000, { visible: false, v: 'v2000' });
+    const docAv3 = deletedDoc('collection/a', 3000);
+
+    return (
+      spec()
+        // Disable GC so the cache persists across listens.
+        .withGCEnabled(false)
+        .userListens(visibleQuery)
+        .watchAcksFull(visibleQuery, 1000, docAv1)
+        .expectEvents(visibleQuery, { added: [docAv1] })
+        .userUnlistens(visibleQuery)
+        .watchRemoves(visibleQuery)
+        .userListens(allQuery)
+        .expectEvents(allQuery, { added: [docAv1], fromCache: true })
+        .watchAcks(allQuery)
+        .watchSends( { removed: [allQuery]}, docAv3)
+        .watchCurrents(allQuery, 'resume-token-3000')
+        .watchSnapshots(3000)
+        .expectEvents(allQuery, { removed: [docAv1], fromCache: false })
+        .userUnlistens(allQuery)
+        .watchRemoves(allQuery)
+        // Supposing we sent a resume token for visibleQuery, watch could catch
+        // us up to docAV2 since that's the last relevant change to the query
+        // (the document falls out) and send us a snapshot that's ahead of
+        // docAv3 (which is already in our cache).
+        .userListens(visibleQuery, 'resume-token-1000')
+        .watchAcksFull(visibleQuery, 5000, docAv2)
+        .expectEvents(visibleQuery, { fromCache: false })
+        .userUnlistens(visibleQuery)
+        .watchRemoves(visibleQuery)
+        // Listen to allQuery again and make sure we still get no docs.
+        .userListens(allQuery, 'resume-token-4000')
+        .expectEvents(allQuery, { fromCache: true })
+        .watchAcksFull(allQuery, 6000)
+        .expectEvents(allQuery, { fromCache: false })
+    );
+  });
+
   specTest('Deleted documents in cache are fixed', [], () => {
     const allQuery = Query.atPath(path('collection'));
     const docAv1 = doc('collection/a', 1000, { key: 'a' });

From 168d86d07667b5d0839bfa6c8583025e363457df Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Wed, 18 Jul 2018 08:27:44 -0700
Subject: [PATCH 08/12] Allow updates for targets where the document is
 modified

---
 packages/firestore/src/local/local_store.ts         | 12 ++++++++++--
 .../firestore/test/unit/specs/listen_spec.test.ts   | 13 +++++++++----
 .../firestore/test/unit/specs/spec_test_runner.ts   |  3 +++
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts
index 54f3b081a5e..5c823a22c82 100644
--- a/packages/firestore/src/local/local_store.ts
+++ b/packages/firestore/src/local/local_store.ts
@@ -26,7 +26,7 @@ import {
   DocumentMap,
   MaybeDocumentMap
 } from '../model/collections';
-import { MaybeDocument, NoDocument } from '../model/document';
+import { MaybeDocument } from '../model/document';
 import { DocumentKey } from '../model/document_key';
 import { Mutation } from '../model/mutation';
 import {
@@ -425,6 +425,7 @@ export class LocalStore {
     const documentBuffer = new RemoteDocumentChangeBuffer(this.remoteDocuments);
     return this.persistence.runTransaction('Apply remote event', txn => {
       const promises = [] as Array<PersistencePromise<void>>;
+      let updatedDocuments = documentKeySet();
       objUtils.forEachNumber(
         remoteEvent.targetChanges,
         (targetId: TargetId, change: TargetChange) => {
@@ -432,6 +433,13 @@ export class LocalStore {
           let queryData = this.targetIds[targetId];
           if (!queryData) return;
 
+          change.addedDocuments.forEach(key => {
+            updatedDocuments = updatedDocuments.add(key);
+          });
+          change.modifiedDocuments.forEach(key => {
+            updatedDocuments = updatedDocuments.add(key);
+          });
+
           promises.push(
             this.queryCache
               .removeMatchingKeys(txn, change.removedDocuments, targetId)
@@ -473,8 +481,8 @@ export class LocalStore {
             // committed version on the server.
             if (
               existingDoc == null ||
-              (existingDoc instanceof NoDocument) ||
               doc.version.isEqual(SnapshotVersion.MIN) ||
+              updatedDocuments.has(doc.key) ||
               doc.version.compareTo(existingDoc.version) >= 0
             ) {
               documentBuffer.addEntry(doc);
diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts
index a0a5a2d28cf..80bc470ed58 100644
--- a/packages/firestore/test/unit/specs/listen_spec.test.ts
+++ b/packages/firestore/test/unit/specs/listen_spec.test.ts
@@ -271,7 +271,10 @@ describeSpec('Listens:', [], () => {
         // (the document falls out) and send us a snapshot that's ahead of
         // docAv3 (which is already in our cache).
         .userListens(visibleQuery, 'resume-token-1000')
-        .watchAcksFull(visibleQuery, 5000, docAv2)
+        .watchAcks(visibleQuery)
+        .watchSends({ removed: [visibleQuery] }, docAv2)
+        .watchCurrents(visibleQuery, 'resume-token-5000')
+        .watchSnapshots(5000)
         .expectEvents(visibleQuery, { fromCache: false })
         .userUnlistens(visibleQuery)
         .watchRemoves(visibleQuery)
@@ -304,7 +307,7 @@ describeSpec('Listens:', [], () => {
         .userListens(allQuery)
         .expectEvents(allQuery, { added: [docAv1], fromCache: true })
         .watchAcks(allQuery)
-        .watchSends( { removed: [allQuery]}, docAv3)
+        .watchSends({ removed: [allQuery] }, docAv3)
         .watchCurrents(allQuery, 'resume-token-3000')
         .watchSnapshots(3000)
         .expectEvents(allQuery, { removed: [docAv1], fromCache: false })
@@ -315,13 +318,15 @@ describeSpec('Listens:', [], () => {
         // (the document falls out) and send us a snapshot that's ahead of
         // docAv3 (which is already in our cache).
         .userListens(visibleQuery, 'resume-token-1000')
-        .watchAcksFull(visibleQuery, 5000, docAv2)
+        .watchAcks(visibleQuery)
+        .watchSends({ removed: [visibleQuery] }, docAv2)
+        .watchCurrents(visibleQuery, 'resume-token-5000')
+        .watchSnapshots(5000)
         .expectEvents(visibleQuery, { fromCache: false })
         .userUnlistens(visibleQuery)
         .watchRemoves(visibleQuery)
         // Listen to allQuery again and make sure we still get no docs.
         .userListens(allQuery, 'resume-token-4000')
-        .expectEvents(allQuery, { fromCache: true })
         .watchAcksFull(allQuery, 6000)
         .expectEvents(allQuery, { fromCache: false })
     );
diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts
index a146585fbc4..fb07acbb377 100644
--- a/packages/firestore/test/unit/specs/spec_test_runner.ts
+++ b/packages/firestore/test/unit/specs/spec_test_runner.ts
@@ -443,6 +443,9 @@ abstract class TestRunner {
   }
 
   private doStep(step: SpecStep): Promise<void> {
+    // tslint:disable-next-line:no-console
+    console.log('  step: ' + JSON.stringify(step));
+
     if ('userListen' in step) {
       return this.doListen(step.userListen!);
     } else if ('userUnlisten' in step) {

From ac4aa80161f5896f8ca36a98298040a570c24dc1 Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Wed, 18 Jul 2018 08:44:36 -0700
Subject: [PATCH 09/12] Use version/resume token to match deleted and
 non-deleted cases

---
 packages/firestore/test/unit/specs/listen_spec.test.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts
index 80bc470ed58..017b4548ff2 100644
--- a/packages/firestore/test/unit/specs/listen_spec.test.ts
+++ b/packages/firestore/test/unit/specs/listen_spec.test.ts
@@ -308,8 +308,8 @@ describeSpec('Listens:', [], () => {
         .expectEvents(allQuery, { added: [docAv1], fromCache: true })
         .watchAcks(allQuery)
         .watchSends({ removed: [allQuery] }, docAv3)
-        .watchCurrents(allQuery, 'resume-token-3000')
-        .watchSnapshots(3000)
+        .watchCurrents(allQuery, 'resume-token-4000')
+        .watchSnapshots(4000)
         .expectEvents(allQuery, { removed: [docAv1], fromCache: false })
         .userUnlistens(allQuery)
         .watchRemoves(allQuery)

From 0dc3eeddff51ff42fde3b6d1bbef17868d12e391 Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Wed, 18 Jul 2018 08:54:43 -0700
Subject: [PATCH 10/12] Adjust comments and only accept document updates when
 athoritative

---
 packages/firestore/src/local/local_store.ts | 40 ++++++++++++---------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts
index 5c823a22c82..481f908512c 100644
--- a/packages/firestore/src/local/local_store.ts
+++ b/packages/firestore/src/local/local_store.ts
@@ -425,7 +425,7 @@ export class LocalStore {
     const documentBuffer = new RemoteDocumentChangeBuffer(this.remoteDocuments);
     return this.persistence.runTransaction('Apply remote event', txn => {
       const promises = [] as Array<PersistencePromise<void>>;
-      let updatedDocuments = documentKeySet();
+      let authoritativeUpdates = documentKeySet();
       objUtils.forEachNumber(
         remoteEvent.targetChanges,
         (targetId: TargetId, change: TargetChange) => {
@@ -433,12 +433,23 @@ export class LocalStore {
           let queryData = this.targetIds[targetId];
           if (!queryData) return;
 
-          change.addedDocuments.forEach(key => {
-            updatedDocuments = updatedDocuments.add(key);
-          });
-          change.modifiedDocuments.forEach(key => {
-            updatedDocuments = updatedDocuments.add(key);
-          });
+          // When a change is current and contains updates (either add or
+          // modify) we can completely trust these updates as authoritative.
+          //
+          // If the change is not current, then watch may have issued a global
+          // snapshot even though still catching up on this target.
+          //
+          // If the document is only updated while removing it from a target
+          // then watch isn't obligated to send the absolute latest version: it
+          // can send the first version that caused the document not to match.
+          if (change.current) {
+            change.addedDocuments.forEach(key => {
+              authoritativeUpdates = authoritativeUpdates.add(key);
+            });
+            change.modifiedDocuments.forEach(key => {
+              authoritativeUpdates = authoritativeUpdates.add(key);
+            });
+          }
 
           promises.push(
             this.queryCache
@@ -471,18 +482,15 @@ export class LocalStore {
         changedDocKeys = changedDocKeys.add(key);
         promises.push(
           documentBuffer.getEntry(txn, key).next(existingDoc => {
-            // Make sure we don't apply an old document version to the remote
-            // cache, though we make an exception for SnapshotVersion.MIN which
-            // can happen for manufactured events (e.g. in the case of a limbo
-            // document resolution failing).
-            //
-            // Also disregard any existing NoDocument. These documents can have
-            // synthesized versions based on read times after any actual
-            // committed version on the server.
+            // If a document update isn't authoritative, make sure we don't
+            // apply an old document version to the remote cache. We make an
+            // exception for SnapshotVersion.MIN which can happen for
+            // manufactured events (e.g. in the case of a limbo document
+            // resolution failing).
             if (
               existingDoc == null ||
               doc.version.isEqual(SnapshotVersion.MIN) ||
-              updatedDocuments.has(doc.key) ||
+              authoritativeUpdates.has(doc.key) ||
               doc.version.compareTo(existingDoc.version) >= 0
             ) {
               documentBuffer.addEntry(doc);

From 317467ee8a6622f217bf89075431e63ae51dce82 Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Wed, 18 Jul 2018 09:08:29 -0700
Subject: [PATCH 11/12] Remove the need for current consider a change
 authoritative

---
 packages/firestore/src/local/local_store.ts | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts
index 481f908512c..d93c92df4f5 100644
--- a/packages/firestore/src/local/local_store.ts
+++ b/packages/firestore/src/local/local_store.ts
@@ -433,23 +433,18 @@ export class LocalStore {
           let queryData = this.targetIds[targetId];
           if (!queryData) return;
 
-          // When a change is current and contains updates (either add or
-          // modify) we can completely trust these updates as authoritative.
-          //
-          // If the change is not current, then watch may have issued a global
-          // snapshot even though still catching up on this target.
+          // When a global snapshot contains updates (either add or modify) we
+          // can completely trust these updates as authoritative.
           //
           // If the document is only updated while removing it from a target
           // then watch isn't obligated to send the absolute latest version: it
           // can send the first version that caused the document not to match.
-          if (change.current) {
-            change.addedDocuments.forEach(key => {
-              authoritativeUpdates = authoritativeUpdates.add(key);
-            });
-            change.modifiedDocuments.forEach(key => {
-              authoritativeUpdates = authoritativeUpdates.add(key);
-            });
-          }
+          change.addedDocuments.forEach(key => {
+            authoritativeUpdates = authoritativeUpdates.add(key);
+          });
+          change.modifiedDocuments.forEach(key => {
+            authoritativeUpdates = authoritativeUpdates.add(key);
+          });
 
           promises.push(
             this.queryCache

From 7bfb951073671a95e5ae9939b4c4395b10e77475 Mon Sep 17 00:00:00 2001
From: Marek Gilbert <mcg@google.com>
Date: Wed, 18 Jul 2018 10:18:48 -0700
Subject: [PATCH 12/12] Implement review feedback

---
 packages/firestore/src/local/local_store.ts   |  5 +++-
 .../test/unit/specs/listen_spec.test.ts       | 23 +++++++++++--------
 .../test/unit/specs/spec_test_runner.ts       |  3 ---
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts
index d93c92df4f5..378e0078452 100644
--- a/packages/firestore/src/local/local_store.ts
+++ b/packages/firestore/src/local/local_store.ts
@@ -434,7 +434,10 @@ export class LocalStore {
           if (!queryData) return;
 
           // When a global snapshot contains updates (either add or modify) we
-          // can completely trust these updates as authoritative.
+          // can completely trust these updates as authoritative and blindly
+          // apply them to our cache (as a defensive measure to promote
+          // self-healing in the unfortunate case that our cache is ever somehow
+          // corrupted / out-of-sync).
           //
           // If the document is only updated while removing it from a target
           // then watch isn't obligated to send the absolute latest version: it
diff --git a/packages/firestore/test/unit/specs/listen_spec.test.ts b/packages/firestore/test/unit/specs/listen_spec.test.ts
index 017b4548ff2..1b7f4f73134 100644
--- a/packages/firestore/test/unit/specs/listen_spec.test.ts
+++ b/packages/firestore/test/unit/specs/listen_spec.test.ts
@@ -334,6 +334,8 @@ describeSpec('Listens:', [], () => {
 
   specTest('Deleted documents in cache are fixed', [], () => {
     const allQuery = Query.atPath(path('collection'));
+    const setupQuery = allQuery.addFilter(filter('key', '==', 'a'));
+
     const docAv1 = doc('collection/a', 1000, { key: 'a' });
     const docDeleted = deletedDoc('collection/a', 2000);
 
@@ -345,19 +347,20 @@ describeSpec('Listens:', [], () => {
         // document in order to force the watch change aggregator to propagate
         // the deletion.
         .withGCEnabled(false)
-        .userListens(allQuery)
-        .watchAcksFull(allQuery, 1000, docAv1)
-        .expectEvents(allQuery, { added: [docAv1], fromCache: false })
-        .watchSends({ removed: [allQuery] }, docDeleted)
-        .watchSnapshots(2000, [allQuery], 'resume-token-2000')
+        .userListens(setupQuery)
+        .watchAcksFull(setupQuery, 1000, docAv1)
+        .expectEvents(setupQuery, { added: [docAv1], fromCache: false })
+        .watchSends({ removed: [setupQuery] }, docDeleted)
+        .watchSnapshots(2000, [setupQuery], 'resume-token-2000')
         .watchSnapshots(2000)
-        .expectEvents(allQuery, { removed: [docAv1], fromCache: false })
-        .userUnlistens(allQuery)
-        .watchRemoves(allQuery)
+        .expectEvents(setupQuery, { removed: [docAv1], fromCache: false })
+        .userUnlistens(setupQuery)
+        .watchRemoves(setupQuery)
 
         // Now when the client listens expect the cached NoDocument to be
-        // discarded because the global snapshot version exceeds what came before.
-        .userListens(allQuery, 'resume-token-2000')
+        // discarded because the global snapshot version exceeds what came
+        // before.
+        .userListens(allQuery)
         .watchAcksFull(allQuery, 3000, docAv1)
         .expectEvents(allQuery, { added: [docAv1], fromCache: false })
     );
diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts
index fb07acbb377..a146585fbc4 100644
--- a/packages/firestore/test/unit/specs/spec_test_runner.ts
+++ b/packages/firestore/test/unit/specs/spec_test_runner.ts
@@ -443,9 +443,6 @@ abstract class TestRunner {
   }
 
   private doStep(step: SpecStep): Promise<void> {
-    // tslint:disable-next-line:no-console
-    console.log('  step: ' + JSON.stringify(step));
-
     if ('userListen' in step) {
       return this.doListen(step.userListen!);
     } else if ('userUnlisten' in step) {