[Multi-Tab] Recover from unexpected primary lease loss #984

schmidt-sebastian · 2018-07-06T08:01:57Z

This PR examines all the places where we need a Primary lease in IndexedDb and adds recovery logic. The recovery logic is pretty simple - it just flips the client state to secondary.

I tried to plumb through some checks on whether the client is actually no longer primary, but decided this is not worth it:

Neither Sync Engine nor Remote Store have access to the persistence layer. I would have needed to plumb the primary state through to LocalStore.
If the client should be primary, IndexedDb will reset the state in the next lease refresh.

schmidt-sebastian · 2018-07-06T08:05:09Z

packages/firestore/src/remote/remote_store.ts

+          }
+        },
+        err => {
+          if (err.code === Code.FAILED_PRECONDITION) {


FYI: Instead of relying on the error code, I tried to make this more specific and added a new error class at one point. I wanted to do an instanceof CustomPrimaryLeaseLostError check here. It turns out that instanceof checks don't work with errors though, and so I abandoned this approach.

mikelehen

Nice! This mostly LGTM but a few nits / questions / comments.

mikelehen · 2018-07-11T15:21:01Z

packages/firestore/src/core/sync_engine.ts

+   * Marks the client as secondary if an IndexedDb operation fails because the
+   * primary lease has been taken by another client. This can happen when the
+   * client is temporarily CPU throttled and fails to renew its lease in time,
+   * in which we treat the current client as secondary. We can always revert


in which case

mikelehen · 2018-07-11T15:22:30Z

packages/firestore/src/core/sync_engine.ts

+   * in which we treat the current client as secondary. We can always revert
+   * back to primary status via the lease refresh in our persistence layer.
+   *
+   * @param err An error returned by an IndexedDb operation.


"IndexedDb operation" seems more specific than reality. It could be any "persistence operation" or even "LocalStore operation", no?

Yes, this can get called with a number of errors. All errors get plumbed through LocalStore, so I changed "IndexedDb" to "LocalStore" as suggested.

mikelehen · 2018-07-11T15:24:52Z

packages/firestore/src/core/sync_engine.ts

+   * @return A Promise that resolves after we recovered, or the original error.
+   */
+  private async tryRecoverClient(err: FirestoreError): Promise<void> {
+    if (err.code === Code.FAILED_PRECONDITION) {


What would you think about adding an IndexedDbPersistence.isLostLeaseError() helper that checks both the code and the message?

Also, it may be useful to add a log message here...

I am not too big of a fan of checking the error string and even if we were to re-use the FAILED_PRECONDITION code somewhere else in our persistence layer, flipping the primary state to secondary seems like a reasonable action. Without this, I think we can just do the check here inline.

We already re-use FAILED_PRECONDITION for the PRIMARY_LEASE_EXCLUSIVE_ERROR_MSG error, and I don't think it makes sense to fall back to secondary in that case.

I'd rather pessimistically assume that this error handling should not apply to future unknown errors and only expand it if/when we have specific cases we want to let it handle.

If we do keep this error handling fairly inclusive, we should definitely include the error in the log message.

I moved the error check to IndexedDb and included the error message. Another option would be to statically define the error and compare against the instance, but that messes with the stacktraces.

mikelehen · 2018-07-11T15:31:40Z

packages/firestore/src/local/indexeddb_persistence.ts

-          }
+          this.isPrimary = canActAsPrimary;
+          // Always call the primary state listener, since SyncEngine may have
+          // changed the primary state to 'false'.


FWIW this feels a hacky to me, but if we're going to do this and rely on it, you should document it on PrimaryStateListener interface and/or the setPrimaryStateListener() method ("Called when the primary state changes and again every N seconds while primary")

A cleaner (though perhaps more code?) approach would be to aggressively give up our primary lease in tryRecoverClient() so that we are definitely not the primary anymore.

I agree with you here, but I don't think the plumbing to make this work is necessarily a worthwhile investment. For now, I changed the method comment in setPrimaryStateListener.

It might even be possible to remove the primary flag from the persistence layer. It is currently only used at startup and at shutdown.

mikelehen · 2018-07-11T15:35:21Z

packages/firestore/src/core/sync_engine.ts

-          /*keepPersistedQueryData=*/ false
-        );
-        await this.localStore.collectGarbage();
+        return this.localStore


mikelehen · 2018-07-11T17:25:37Z

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

+        .expectPrimaryState(false)
+        // TODO(multitab): Suppres the additional `fromCache`, which is raised
+        // locally because we disable the network when we lose the primary
+        // lease.


FWIW I'm not sure if this is wrong / harmful. When a primary switch happens we collectively will be without a network connection for a period of time and it seems correct for views to trigger fromCache=true events... but we should return to fromCache=false once the primary switch completes and a new primary establishes the listens...

I am still undecided what the best desired behavior should be. I don't want clients flip-flopping between fromCache states. But on the other hand, the client will be offline at that point, so it technically correct to raise this event. I removed this todo, which also meant that I didn't have to fix the typo in Suppres. Win win.

mikelehen · 2018-07-11T17:27:58Z

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

+          .stealPrimaryLease()
+          .client(0)
+          // Send a watch update to client 0, who is longer primary (but doesn't
+          // it yet). The watch update gets ignored.


missing "know"

mikelehen · 2018-07-11T17:33:12Z

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

+    options = options || {};
+
+    this.currentStep = {
+      writeAck: { version, keepInQueue: !!options.keepInQueue }


FWIW it may be worth adding a TODO(multitab) to update the spec schemas on the other clients with the changes you've made... Just so we don't forget to do that before / soon-after merging multitab into master.

The changes that I made in Master are already ported to the other platforms (firebase/firebase-ios-sdk#1498, https://github.com/FirebasePrivate/firebase-android-sdk/pull/95). The rest should hopefully be obvious when I merge this branch.

Please do remind me of this comment when I spend a week on this merge.

mikelehen · 2018-07-11T17:38:51Z

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

    if (state.visibility) {
      this.platform.raiseVisibilityEvent(state.visibility!);
    }
+
+    if (state.primary) {
+      await writeOwnerToIndexedDb(this.clientId);


nit: Would it be more realistic to just clear (or expire?) the current owner and then let ClientMetadataRefresh detect that it needs to steal the lease? I don't care strongly though.

Yes, it would be. That takes a way the deterministic order of these tests though, as any secondary could then become primary.

mikelehen · 2018-07-11T17:42:27Z

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

+    if (this.expectedActiveTargets) {
+      if (!obj.isEmpty(this.expectedActiveTargets)) {
+        await this.connection.waitForWatchOpen();
+      }
      await this.queue.drain();


Er, why do we need to call/wait-for queue.drain() even when we don't call waitForWatchOpen()? validateActiveTargets() is called by validateStateExpectations() which is called by run(step) which already calls queue.drain()...

Hrm. This was causing test failures for me before when this.expectedActiveTargets was set but empty. It's passing now though and you raised a very good point.

mikelehen · 2018-07-14T21:11:02Z

Just to double check... should this be assigned back to me? Or are you still working on review feedback?

schmidt-sebastian · 2018-07-15T03:18:49Z

Just to double check... should this be assigned back to me? Or are you still working on review feedback?

Back to you :)

mikelehen

I'm still wary of race conditions... I'm not 100% sure how likely it is that they can actually manifest or how best to guard against them. Feel free to ping me if you want to chat through this together.

mikelehen · 2018-07-16T23:13:18Z

packages/firestore/src/core/sync_engine.ts

@@ -490,6 +493,11 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
      // connection is disabled.
      await this.remoteStore.fillWritePipeline();
    } else if (batchState === 'acknowledged' || batchState === 'rejected') {
+      // 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.


I'm not sure I follow / agree-with your argument. I don't think we can deterministically "finish these notifications in the state it was supposed to be in".

At the end of the day it's possible for two tabs to send the same write to the backend and broadcast LocalStorage notifications for it. E.g. Tab A is primary, performs a write, gets the response, and gets paused / throttled right before sending the LocalStorage notification. Tab B takes over as primary, re-sends the write, and processes the response (sending a LocalStorage notification). Tab A resumes and its LocalStorage notification (for the same write) now goes out.

I think this means that handling write acknowledgement notifications must be idempotent.

But I also don't understand where the extra query events you mentioned are coming from... It may be worth talking through this or something.

mikelehen · 2018-07-16T23:17:29Z

packages/firestore/src/core/sync_engine.ts

+    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.
+      await this.applyPrimaryState(false);


I'd prefer that too, but I don't think we can guarantee it. The only thing we can guarantee is that only one tab can make IndexedDb writes as primary at a time (via transactions that require the primary lease). But we could have two tabs with WriteStreams open for some period of time.

Ideally our design would allow for this. If it's infeasible, then I guess we should acknowledge that there are race conditions but convince ourselves that it'll be sufficiently unlikely and/or non-catastrophic or something...

mikelehen · 2018-07-16T23:19:33Z

packages/firestore/src/core/sync_engine.ts

+        // taken the primary lease.
+        log.debug(
+          LOG_TAG,
+          'Unexpectedly received mutation batch notification when already primary. Releasing primary lease.'


If we keep this logic, we should rephrase this comment since we're not actually releasing the primary lease. We're just pretending we don't have it. :-)

mikelehen · 2018-07-16T23:24:32Z

packages/firestore/src/core/sync_engine.ts

+   * @return A Promise that resolves after we recovered, or the original error.
+   */
+  private async tryRecoverClient(err: FirestoreError): Promise<void> {
+    if (err.code === Code.FAILED_PRECONDITION) {


We already re-use FAILED_PRECONDITION for the PRIMARY_LEASE_EXCLUSIVE_ERROR_MSG error, and I don't think it makes sense to fall back to secondary in that case.

I'd rather pessimistically assume that this error handling should not apply to future unknown errors and only expand it if/when we have specific cases we want to let it handle.

If we do keep this error handling fairly inclusive, we should definitely include the error in the log message.

schmidt-sebastian

I updated the PR with the result of our discussion on Tuesday. The tests are the same, but the logic is pretty different. IndexedDb is now driving all primary state transitions and might call the primaryStateListener when it detects that the lease has gone missing.

schmidt-sebastian · 2018-07-19T23:13:30Z

packages/firestore/src/core/sync_engine.ts

@@ -490,6 +493,11 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
      // connection is disabled.
      await this.remoteStore.fillWritePipeline();
    } else if (batchState === 'acknowledged' || batchState === 'rejected') {
+      // 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.


I updated this PR to ignore events that were not supposed to happen. As discussed on Tuesday, I then reconcile the query state when we get the notification that the client is secondary (using the same code that I use to reconcile the query views when we transition to primary).
Write notifications are actually idempotent and it doesn't matter how often we process the same write, with the exception that only the first call triggers the user callback.

schmidt-sebastian · 2018-07-19T23:14:11Z

packages/firestore/src/core/sync_engine.ts

+   * @return A Promise that resolves after we recovered, or the original error.
+   */
+  private async tryRecoverClient(err: FirestoreError): Promise<void> {
+    if (err.code === Code.FAILED_PRECONDITION) {


I moved the error check to IndexedDb and included the error message. Another option would be to statically define the error and compare against the instance, but that messes with the stacktraces.

mikelehen

Thanks. I think this basically looks good though I'm a little fuzzy on the reset-limbo stuff going on...

mikelehen · 2018-07-20T20:22:56Z

packages/firestore/src/core/sync_engine.ts

+        .remoteDocumentKeys(targetId)
+        .then(async remoteKeys => {
+          const queryView = this.queryViewsByTarget[targetId];
+          assert(!!queryView, 'Cannot reconcile missing view');


FWIW I think it'd be pretty straightforward to just pass in the queryView (as the only argument since it contains the query, targetId, and view) and then we don't need this assert.

Done. This also helped me realize that I am doing one extra IndexedDb lookup for the query when I already have the QueryView (in synchronizeQueryViewsAndRaiseSnapshots).

mikelehen · 2018-07-20T20:55:03Z

packages/firestore/src/core/sync_engine.ts

+            docs,
+            remoteKeys,
+            /* resetLimboDocuments= */ this.isPrimary,
+            /* resetCurrent= */ this.isPrimary


I'm having trouble understanding what these flags do. I wonder if "reset" should be "recompute" or "update" (reset makes it sound like they'll just be cleared, which I don't think is the case)... And if we pass false does that mean current and limboDocuments will be left 100% as-is? I guess ultimately I'm wondering why we can't just unconditionally recompute these. I also wonder if we could sensibly combine these into a single argument. Since current depends on limboDocuments, it probably never makes sense to recompute current without computing limbo documents?

Anyway, at minimum it will probably take some comments to appease me.

I simplified this a little to unconditionally re-compute the list of limbo documents. In clients that are in-sync this is a no-op, and in clients that are out of sync, this will bring them back into sync. The only bit that I still need to provide is whether to set current to false. I added some documentation to clear this up a little.

Notice that I also added a TODO to mark the queries as non-current in SharedClientState. That's probably a three line change but I wanted to keep this PR from exploding even further.

mikelehen · 2018-07-20T21:57:44Z

packages/firestore/src/core/sync_engine.ts

      log.debug(
        LOG_TAG,
-        'Unexpectedly lost primary lease, reverting to secondary'
+        'Unexpectedly lost primary lease, attempting to recover'


We log "attempting to recover" and then do nothing... Seems like maybe this method should now be ignorePrimaryLeaseLoss() and the log message should say "ignoring"

... alternatively, as-is will there be a long (up to 4s) delay before we officially notice we're not primary? Maybe this should schedule a lease refresh or something.

I removed "attempting to recover" altogether and didn't replace it with anything.

Note that the persistence layer schedules a lease refresh when it throws this error, so the delay should be fairly minimal.

mikelehen · 2018-07-20T22:02:14Z

packages/firestore/src/core/sync_engine.ts

+  }
+
+  /**
+   * Reconcile the local query views with those the state from persistence.


Yesssss. Fixed.

mikelehen · 2018-07-20T22:03:46Z

packages/firestore/src/core/sync_engine.ts

+          // For queries that never executed on this client, we need to
+          // allocate the query in LocalStore and initialize a new View.
+          queryData = await this.localStore.allocateQuery(query);
+          await this.initializeViewAndComputeSnapshot(queryData, false);


/current=/

mikelehen · 2018-07-20T22:06:47Z

packages/firestore/src/core/sync_engine.ts

+            queryData.targetId
+          );
+          if (viewChange.snapshot) {
+            this.syncEngineListener!.onWatchChange([viewChange.snapshot]);


Should we batch up all the snapshots and call onWatchChange() only once?

In practice I'm not sure how much it matters right now, but since all of the snapshots were the result of a single event, I think we should send them to onWatchChange() as a single batch. In the future we might expose an API like:

firebase.firestore().onGlobalSnapshot(() => { /* I know that all listeners are in a consistent state. */ });

I was doing this at an earlier stage of this PR, but then ultimately decided to remove it to save one line of code. I put it back for now. I am wondering though if there are ever non-current snapshots for active queries that are not globally consistent?

I'm not sure I understand your question, but I'm hoping it's a response to my poor job of explaining the onGlobalSnapshot() thing.

The basic idea is that if you had lots of listeners that were related (e.g. for whose turn it is in a turn-based game and for the board state), you could delay your handling of updates until an onGlobalSnapshot() callback [e.g. so you don't try to take your turn until you know it's your turn and you have the updated board state]. Said differently: if somebody does multiple writes atomically in a batch write, you could use onGlobalSnapshot() to make sure all of your listeners have seen all of the writes.

So that's the motivation for onWatchChange() accepting a list of snapshots rather than just a single one... I think.

Ok, got it. It would allow us to know that we have received all events from the last consistent global snapshot.

mikelehen · 2018-07-20T22:08:25Z

packages/firestore/src/core/sync_engine.ts

+
+  /**
+   * Reconcile the local query views with those the state from persistence.
+   * Raises snapshots for any changes that affect the current client.


and "Returns the set of queries for which we have local views." or something?

This method actually returns the query data for the targets that are passed in. I updated the comment to:

* Reconcile the query views of the provided query targets with the state from * persistence. Raises snapshots for any changes that affect the current * client and returns the updated state of all target's query data.

mikelehen · 2018-07-20T22:20:49Z

packages/firestore/src/util/obj.ts

@@ -39,6 +39,15 @@ export function size<V>(obj: Dict<V>): number {
  return count;
 }

+/** Extracts the numeric indices from a dictionary. */
+export function indices<V>(obj: Dict<V>): number[] {


Would it make sense to use [numberKey: number]: V instead of Dict<V>?

Nice. This even gives us type safety instead of the "i!sNaN" check below.

schmidt-sebastian

Feedback addressed.

schmidt-sebastian · 2018-07-23T17:24:37Z

packages/firestore/src/core/sync_engine.ts

+        .remoteDocumentKeys(targetId)
+        .then(async remoteKeys => {
+          const queryView = this.queryViewsByTarget[targetId];
+          assert(!!queryView, 'Cannot reconcile missing view');


Done. This also helped me realize that I am doing one extra IndexedDb lookup for the query when I already have the QueryView (in synchronizeQueryViewsAndRaiseSnapshots).

schmidt-sebastian · 2018-07-23T17:26:59Z

packages/firestore/src/core/sync_engine.ts

      log.debug(
        LOG_TAG,
-        'Unexpectedly lost primary lease, reverting to secondary'
+        'Unexpectedly lost primary lease, attempting to recover'


I removed "attempting to recover" altogether and didn't replace it with anything.

Note that the persistence layer schedules a lease refresh when it throws this error, so the delay should be fairly minimal.

schmidt-sebastian · 2018-07-23T17:27:26Z

packages/firestore/src/core/sync_engine.ts

+  }
+
+  /**
+   * Reconcile the local query views with those the state from persistence.


Yesssss. Fixed.

schmidt-sebastian · 2018-07-23T17:33:43Z

packages/firestore/src/core/sync_engine.ts

+
+  /**
+   * Reconcile the local query views with those the state from persistence.
+   * Raises snapshots for any changes that affect the current client.


This method actually returns the query data for the targets that are passed in. I updated the comment to:

* Reconcile the query views of the provided query targets with the state from * persistence. Raises snapshots for any changes that affect the current * client and returns the updated state of all target's query data.

schmidt-sebastian · 2018-07-23T17:34:28Z

packages/firestore/src/core/sync_engine.ts

+          // For queries that never executed on this client, we need to
+          // allocate the query in LocalStore and initialize a new View.
+          queryData = await this.localStore.allocateQuery(query);
+          await this.initializeViewAndComputeSnapshot(queryData, false);


schmidt-sebastian · 2018-07-23T17:42:18Z

packages/firestore/src/core/sync_engine.ts

+            queryData.targetId
+          );
+          if (viewChange.snapshot) {
+            this.syncEngineListener!.onWatchChange([viewChange.snapshot]);


I was doing this at an earlier stage of this PR, but then ultimately decided to remove it to save one line of code. I put it back for now. I am wondering though if there are ever non-current snapshots for active queries that are not globally consistent?

schmidt-sebastian · 2018-07-23T17:45:10Z

packages/firestore/src/util/obj.ts

@@ -39,6 +39,15 @@ export function size<V>(obj: Dict<V>): number {
  return count;
 }

+/** Extracts the numeric indices from a dictionary. */
+export function indices<V>(obj: Dict<V>): number[] {


Nice. This even gives us type safety instead of the "i!sNaN" check below.

schmidt-sebastian · 2018-07-23T18:42:42Z

packages/firestore/src/core/sync_engine.ts

+            docs,
+            remoteKeys,
+            /* resetLimboDocuments= */ this.isPrimary,
+            /* resetCurrent= */ this.isPrimary


I simplified this a little to unconditionally re-compute the list of limbo documents. In clients that are in-sync this is a no-op, and in clients that are out of sync, this will bring them back into sync. The only bit that I still need to provide is whether to set current to false. I added some documentation to clear this up a little.

Notice that I also added a TODO to mark the queries as non-current in SharedClientState. That's probably a three line change but I wanted to keep this PR from exploding even further.

mikelehen

Some nits and unfortunately I'm still struggling with the resetCurrent logic. Feel free to ping me on chat to expedite the review process.

mikelehen · 2018-07-24T21:39:09Z

packages/firestore/src/core/sync_engine.ts

+            queryData.targetId
+          );
+          if (viewChange.snapshot) {
+            this.syncEngineListener!.onWatchChange([viewChange.snapshot]);


I'm not sure I understand your question, but I'm hoping it's a response to my poor job of explaining the onGlobalSnapshot() thing.

The basic idea is that if you had lots of listeners that were related (e.g. for whose turn it is in a turn-based game and for the board state), you could delay your handling of updates until an onGlobalSnapshot() callback [e.g. so you don't try to take your turn until you know it's your turn and you have the updated board state]. Said differently: if somebody does multiple writes atomically in a batch write, you could use onGlobalSnapshot() to make sure all of your listeners have seen all of the writes.

So that's the motivation for onWatchChange() accepting a list of snapshots rather than just a single one... I think.

mikelehen · 2018-07-24T21:42:42Z

packages/firestore/src/core/sync_engine.ts

-          assert(!!queryView, 'Cannot reconcile missing view');
-          const viewChange = queryView.view.synchronizeWithPersistedState(
+          // We do not update our tracking of limbo documents since
+          //`resetCurrent` will set `current` to false for the querie's view


missing space after // and querie's typo

mikelehen · 2018-07-24T21:52:28Z

packages/firestore/src/core/sync_engine.ts

-            viewChange.limboChanges
+          assert(
+            !this.isPrimary || viewSnapshot.limboChanges.length === 0,
+            'Received limboChanges when query is not CURRENT.'


I stared at the comment and this assert for a while before I fully understood what's going on. One thing that would help is if you changed the message to "Primary received ..."

I removed this assert and am now tracking these documents in the if I am the primary client. I could also pass in a updateLimboDocuments flag and not update the limbo documents for secondaries.

mikelehen · 2018-07-24T21:59:18Z

packages/firestore/src/core/sync_engine.ts

+          //`resetCurrent` will set `current` to false for the querie's view
+          // (and won't return any limbo documents). We also explicitly don't
+          // track limbo documents as secondary.
+          const viewSnapshot = queryView.view.synchronizeWithPersistedState(


This comment still doesn't explain why we are resetting 'current' and why we only do it as primary.

I (non-confidently) think maybe the reasons are:

We reset 'current' as primary because we expect synchronizeViewAndComputeSnapshot() to only be called immediately after we become primary and since we haven't had an active network connection, we should treat the queries as non-current (and raise fromCache=true events) until we get the initial response from Watch.

Secondaries don't need to set current to false because it'll happen... somewhere else? (I guess where you added the TODO?)

In general I'm fuzzy on what the desired behavior is and how we're implementing it. Perhaps the desired behavior is that at the point of a primary change, all clients (secondary and master) mark all views as non-current? But if that's the case, why can't we just unconditionally pass true here for resetCurrent?

Sorry if I'm just confused. Feel free to ping me on chat to explain what's going on and we can figure out what comments/whatever might help this to make sense to me.

As per our discussion yesterday, I changed this logic to no longer set current to false. That means that we now transition primary state without any user feedback and also that we could theoretically get limbo documents at this point.

Note that I am not quite sure how I can trigger limbo documents at this point (the new statement that I added). Any test case that I can think of has the query already marked Non-CURRENT by the previous primary tab (based on the limbo docs from the previous computation).

mikelehen · 2018-07-24T22:02:36Z

packages/firestore/src/core/sync_engine.ts

-        LOG_TAG,
-        'Unexpectedly lost primary lease, attempting to recover'
-      );
+      log.debug(LOG_TAG, 'Unexpectedly lost primary lease');


You still need to update the method name and comment... Please mention that when this error is thrown, there will automatically be a lease refresh scheduled so we should very quickly get an applyPrimaryState() call from the persistence layer.

Update the method and comment. Method is now ignoreIfPrimaryLeaseLoss.

mikelehen · 2018-07-24T22:21:37Z

packages/firestore/src/remote/remote_store.ts

@@ -585,10 +585,7 @@ export class RemoteStore implements TargetMetadataProvider {

  private tryRecoverFromPrimaryLeaseLoss(err: FirestoreError): void {


Please rename this if you rename the other one.

schmidt-sebastian requested review from mikelehen and wilhuff as code owners July 6, 2018 08:01

google-oss-bot added the needs-triage label Jul 6, 2018

schmidt-sebastian commented Jul 6, 2018

View reviewed changes

schmidt-sebastian assigned mikelehen Jul 6, 2018

schmidt-sebastian force-pushed the multitab-recovery branch 2 times, most recently from 354e01b to f9f4d9c Compare July 9, 2018 12:46

Recover from unexpected primary lease loss

c40ff09

schmidt-sebastian force-pushed the multitab-recovery branch from f9f4d9c to c40ff09 Compare July 9, 2018 16:41

mikelehen suggested changes Jul 11, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jul 11, 2018

schmidt-sebastian added 2 commits July 12, 2018 14:20

firestore-multi-tab Merge

e51265b

Review comments

a7c8497

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Jul 15, 2018

mikelehen reviewed Jul 16, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jul 16, 2018

schmidt-sebastian added 3 commits July 17, 2018 13:33

RFC/Not dropping primary status right away

f9c7872

Some clever review comment before I merge firestore-multi-tab.

34d52ae

Merge

692b789

schmidt-sebastian force-pushed the multitab-recovery branch from b786739 to cb49563 Compare July 19, 2018 23:56

schmidt-sebastian added 3 commits July 19, 2018 17:16

Don't release primary lease

3de1af4

Merge branch 'firestore-multi-tab' into multitab-recovery

c243b26

[AUTOMATED]: Prettier Code Styling

ee94d57

schmidt-sebastian force-pushed the multitab-recovery branch from cb49563 to ee94d57 Compare July 20, 2018 00:18

Dropping the primary lease in IndexedDb

3da6855

schmidt-sebastian commented Jul 20, 2018

View reviewed changes

[AUTOMATED]: Prettier Code Styling

cbd58e4

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Jul 20, 2018

mikelehen suggested changes Jul 20, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jul 20, 2018

More feedback

72aa4a9

schmidt-sebastian commented Jul 23, 2018

View reviewed changes

schmidt-sebastian assigned mikelehen Jul 23, 2018

schmidt-sebastian added 2 commits July 23, 2018 11:51

Fixing some nits

7f685f9

[AUTOMATED]: Prettier Code Styling

b1c7e8d

schmidt-sebastian removed their assignment Jul 23, 2018

mikelehen suggested changes Jul 24, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jul 24, 2018

schmidt-sebastian added 2 commits July 25, 2018 11:34

Addresseing one more round of feedback.

6d3628d

[AUTOMATED]: Prettier Code Styling

794e9f7

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Jul 25, 2018

mikelehen approved these changes Jul 25, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jul 25, 2018

schmidt-sebastian added 2 commits July 25, 2018 12:08

Merge

e203796

[AUTOMATED]: Prettier Code Styling

5c56479

schmidt-sebastian force-pushed the multitab-recovery branch from a6fef04 to 5c56479 Compare July 25, 2018 19:12

schmidt-sebastian merged commit 7bb0b9a into firestore-multi-tab Jul 25, 2018

schmidt-sebastian deleted the multitab-recovery branch August 3, 2018 17:15

firebase locked and limited conversation to collaborators Oct 18, 2019

		@@ -585,10 +585,7 @@ export class RemoteStore implements TargetMetadataProvider {

		private tryRecoverFromPrimaryLeaseLoss(err: FirestoreError): void {

[Multi-Tab] Recover from unexpected primary lease loss #984

[Multi-Tab] Recover from unexpected primary lease loss #984

Uh oh!

Conversation

schmidt-sebastian commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen commented Jul 14, 2018

Uh oh!

schmidt-sebastian commented Jul 15, 2018

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!