WIP

Author: schmidt-sebastian

packages/firestore/src/core/firestore_client.ts

@@ -394,7 +394,7 @@ export class FirestoreClient {
     if (this.clientTerminated) {
       return;
     }
-    this.asyncQueue.enqueueRetryable(() => this.eventMgr.unlisten(listener));
+    this.asyncQueue.enqueueAndForget(() => this.eventMgr.unlisten(listener));
   }
 
   async getDocumentFromLocalCache(

packages/firestore/src/local/local_store.ts

@@ -911,7 +911,7 @@ class LocalStoreImpl implements LocalStore {
     }
   }
 
-  releaseTarget(
+  async releaseTarget(
     targetId: number,
     keepPersistedTargetData: boolean
   ): Promise<void> {
@@ -922,21 +922,33 @@ class LocalStoreImpl implements LocalStore {
     );
 
     const mode = keepPersistedTargetData ? 'readwrite' : 'readwrite-primary';
-    return this.persistence
-      .runTransaction('Release target', mode, txn => {
-        if (!keepPersistedTargetData) {
+
+    try {
+      if (!keepPersistedTargetData) {
+        await this.persistence.runTransaction('Release target', mode, txn => {
           return this.persistence.referenceDelegate.removeTarget(
             txn,
             targetData!
           );
-        } else {
-          return PersistencePromise.resolve();
-        }
-      })
-      .then(() => {
-        this.targetDataByTarget = this.targetDataByTarget.remove(targetId);
-        this.targetIdByTarget.delete(targetData!.target);
-      });
+        });
+      }
+    } catch (e) {
+      if (isIndexedDbTransactionError(e)) {
+        // If `releaseTarget` fails, we did not advance the sequence
+        // number for the target. This target might be deleted earlier than
+        // it otherwise would have, but it should not invalidate the integrity
+        // of the data.
+        logDebug(
+          LOG_TAG,
+          `Failed to update sequence numbers for target ${targetId}: ${e}`
+        );
+      } else {
+        throw e;
+      }
+    }
+
+    this.targetDataByTarget = this.targetDataByTarget.remove(targetId);
+    this.targetIdByTarget.delete(targetData!.target);
   }
 
   executeQuery(

packages/firestore/src/remote/remote_store.ts

@@ -408,18 +408,7 @@ export class RemoteStore implements TargetMetadataProvider {
     ) {
       // There was an error on a target, don't wait for a consistent snapshot
       // to raise events
-      try {
-        await this.handleTargetError(watchChange);
-      } catch (e) {
-        logDebug(
-          LOG_TAG,
-          'Failed to remove targets %s: %s ',
-          watchChange.targetIds.join(','),
-          e
-        );
-        await this.disableNetworkUntilRecovery(e);
-      }
-      return;
+      return this.handleTargetError(watchChange);
     }
 
     if (watchChange instanceof DocumentWatchChange) {

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

@@ -569,7 +569,10 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
     );
   });
 
-  specTest('Recovers when watch rejection cannot be persisted', [], () => {
+  specTest('Handles rejections that cannot be persisted', ['exclusive'], () => {
+    // This test verifies that the client ignores failures during the
+    // 'Release target' transaction.
+
     const doc1Query = Query.atPath(path('collection/key1'));
     const doc2Query = Query.atPath(path('collection/key2'));
     const doc1a = doc('collection/key1', 1000, { foo: 'a' });
@@ -593,25 +596,22 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
           doc1Query,
           new RpcError(Code.PERMISSION_DENIED, 'Simulated target error')
         )
-        // `failDatabaseTransactions()` causes us to go offline.
-        .expectActiveTargets()
-        .expectEvents(doc1Query, { fromCache: true })
-        .expectEvents(doc2Query, { fromCache: true })
+        .expectEvents(doc1Query, { errorCode: Code.PERMISSION_DENIED })
         .recoverDatabase()
-        .runTimer(TimerId.AsyncQueueRetry)
-        .expectActiveTargets(
-          { query: doc1Query, resumeToken: 'resume-token-1000' },
-          { query: doc2Query, resumeToken: 'resume-token-2000' }
-        )
-        .watchAcksFull(doc1Query, 3000)
-        .expectEvents(doc1Query, {})
         .watchRemoves(
           doc2Query,
           new RpcError(Code.PERMISSION_DENIED, 'Simulated target error')
         )
         .expectEvents(doc2Query, { errorCode: Code.PERMISSION_DENIED })
-        .watchSends({ affects: [doc1Query] }, doc1b)
-        .watchSnapshots(4000)
+        // Verify that `doc1Query` can be listened to again. Note that the
+        // resume token is slightly outdated since we failed the final
+        // target update during the release.
+        .userListens(doc1Query, 'resume-token-1000')
+        .expectEvents(doc1Query, {
+          added: [doc1a],
+          fromCache: true
+        })
+        .watchAcksFull(doc1Query, 4000, doc1b)
         .expectEvents(doc1Query, {
           modified: [doc1b]
         })