RFC/Not dropping primary status right away

Author: schmidt-sebastian

packages/firestore/src/core/sync_engine.ts

@@ -504,16 +504,16 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       // connection is disabled.
       await this.remoteStore.fillWritePipeline();
     } else if (batchState === 'acknowledged' || batchState === 'rejected') {
-      if (this.isPrimary) {
-        // If we receive a notification of an `acknowledged` or `rejected` batch
-        // via Web Storage, we are either already secondary or another tab has
-        // taken the primary lease.
-        log.debug(
-          LOG_TAG,
-          'Unexpectedly received mutation batch notification when already primary. Releasing primary lease.'
-        );
-        await this.applyPrimaryState(false);
-      }
+      // if (this.isPrimary) {
+      //   // If we receive a notification of an `acknowledged` or `rejected` batch
+      //   // via Web Storage, we are either already secondary or another tab has
+      //   // taken the primary lease.
+      //   log.debug(
+      //     LOG_TAG,
+      //     'Unexpectedly received mutation batch notification when already primary. Releasing primary lease.'
+      //   );
+      //   await this.applyPrimaryState(false);
+      // }
 
       // NOTE: Both these methods are no-ops for batches that originated from
       // other clients.
@@ -842,6 +842,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       await this.remoteStore.disableNetwork();
       objUtils.forEachNumber(this.queryViewsByTarget, targetId => {
         // TODO(multitab): Remove query views for non-local queries.
+
+        const queryView = this.queryViewsByTarget[targetId];
+        const viewChange = queryView.view.clearLimboDocuments();
+        if (viewChange.snapshot) {
+          this.viewHandler!([viewChange.snapshot]);
+        }
         this.remoteStore.unlisten(targetId);
       });
     }
@@ -858,15 +864,15 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
     state: QueryTargetState,
     error?: FirestoreError
   ): Promise<void> {
-    if (this.isPrimary) {
-      // If we receive a target state notification via Web Storage, we are
-      // either already secondary or another tab has taken the primary lease.
-      log.debug(
-        LOG_TAG,
-        'Unexpectedly received query state notification when already primary. Releasing primary lease.'
-      );
-      await this.applyPrimaryState(false);
-    }
+    // if (this.isPrimary) {
+    //   // If we receive a target state notification via Web Storage, we are
+    //   // either already secondary or another tab has taken the primary lease.
+    //   log.debug(
+    //     LOG_TAG,
+    //     'Unexpectedly received query state notification when already primary. Releasing primary lease.'
+    //   );
+    //   await this.applyPrimaryState(false);
+    // }
 
     if (this.queryViewsByTarget[targetId]) {
       switch (state) {

packages/firestore/src/core/view.ts

@@ -402,6 +402,31 @@ export class View {
       !this.mutatedKeys.isEmpty()
     );
   }
+
+  clearLimboDocuments(): ViewChange {
+    this.limboDocuments = documentKeySet();
+    const synced = this.current;
+    const newSyncState = synced ? SyncState.Synced : SyncState.Local;
+    const syncStateChanged = newSyncState !== this.syncState;
+
+    if (syncStateChanged) {
+      this.syncState = newSyncState;
+
+      const snap: ViewSnapshot = new ViewSnapshot(
+        this.query,
+        this.documentSet,
+        this.documentSet,
+        [],
+        this.syncState === SyncState.Local,
+        !this.mutatedKeys.isEmpty(),
+        syncStateChanged,
+        /* excludesMetadataChanges= */ false
+      );
+      return { snapshot: snap, limboChanges: [] };
+    } else {
+      return { limboChanges: [] };
+    }
+  }
 }
 
 function compareChangeType(c1: ChangeType, c2: ChangeType): number {

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

@@ -819,23 +819,37 @@ describeSpec('Listens:', [], () => {
     const query = Query.atPath(path('collection'));
     const docA = doc('collection/a', 1000, { key: 'a' });
     const docB = doc('collection/b', 2000, { key: 'b' });
+    const docC = doc('collection/c', 3000, { key: 'c' });
 
-    return client(0)
-      .expectPrimaryState(true)
-      .userListens(query)
-      .watchAcksFull(query, 1000, docA)
-      .expectEvents(query, { added: [docA] })
-      .client(1)
-      .userListens(query)
-      .expectEvents(query, { added: [docA] })
-      .stealPrimaryLease()
-      .expectListen(query, 'resume-token-1000')
-      .watchAcksFull(query, 2000, docB)
-      .expectEvents(query, { added: [docB] })
-      .client(0)
-      .expectPrimaryState(false)
-      .expectEvents(query, { fromCache: true })
-      .expectEvents(query, { added: [docB] });
+    // TODO(multitab): Once #994 is merged, this query in this test should flip
+    // back to `fromCache:false` even without docC
+    return (
+      client(0)
+        .expectPrimaryState(true)
+        .userListens(query)
+        .watchAcksFull(query, 1000, docA)
+        .expectEvents(query, { added: [docA] })
+        .client(1)
+        .userListens(query)
+        .expectEvents(query, { added: [docA] })
+        .stealPrimaryLease()
+        .expectListen(query, 'resume-token-1000')
+        .watchAcksFull(query, 2000, docB)
+        .expectEvents(query, { added: [docB] })
+        // Client 0 sends the query into limbo since it does not know about docB,
+        // but it still thinks it is primary.
+        .expectEvents(query, { fromCache: true })
+        .client(0)
+        .expectEvents(query, { added: [docB], fromCache: true })
+        .expectLimboDocs(docB.key)
+        .client(1)
+        .watchSends({ affects: [query] }, docC)
+        .watchSnapshots(3000)
+        .expectEvents(query, { added: [docC] })
+        .client(0)
+        .expectPrimaryState(false)
+        .expectEvents(query, { added: [docC] })
+    );
   });
 
   specTest(

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

@@ -950,7 +950,10 @@ abstract class TestRunner {
         this.expectedActiveTargets = expectation.activeTargets!;
       }
       if ('isPrimary' in expectation) {
-        expect(this.syncEngine.isPrimaryClient).to.eq(expectation.isPrimary!);
+        expect(this.syncEngine.isPrimaryClient).to.eq(
+          expectation.isPrimary!,
+          'isPrimary'
+        );
       }
       if ('userCallbacks' in expectation) {
         expect(this.acknowledgedDocs).to.have.members(