Treat ABORTED writes as retryable (#1456)

Author: wilhuff

packages/firestore/src/remote/remote_store.ts

@@ -27,7 +27,7 @@ import {
 } from '../model/mutation_batch';
 import { emptyByteString } from '../platform/platform';
 import { assert } from '../util/assert';
-import { Code, FirestoreError } from '../util/error';
+import { FirestoreError } from '../util/error';
 import * as log from '../util/log';
 import * as objUtils from '../util/obj';
 
@@ -41,7 +41,7 @@ import {
   PersistentWriteStream
 } from './persistent_stream';
 import { RemoteSyncer } from './remote_syncer';
-import { isPermanentError } from './rpc_error';
+import { isPermanentError, isPermanentWriteError } from './rpc_error';
 import {
   DocumentWatchChange,
   ExistenceFilterChange,
@@ -628,9 +628,10 @@ export class RemoteStore implements TargetMetadataProvider {
   }
 
   private async handleHandshakeError(error: FirestoreError): Promise<void> {
-    // Reset the token if it's a permanent error or the error code is
-    // ABORTED, signaling the write stream is no longer valid.
-    if (isPermanentError(error.code) || error.code === Code.ABORTED) {
+    // Reset the token if it's a permanent error, signaling the write stream is
+    // no longer valid. Note that the handshake does not count as a write: see
+    // comments on isPermanentWriteError for details.
+    if (isPermanentError(error.code)) {
       log.debug(
         LOG_TAG,
         'RemoteStore error before completed handshake; resetting stream token: ',
@@ -648,7 +649,9 @@ export class RemoteStore implements TargetMetadataProvider {
   }
 
   private async handleWriteError(error: FirestoreError): Promise<void> {
-    if (isPermanentError(error.code)) {
+    // Only handle permanent errors here. If it's transient, just let the retry
+    // logic kick in.
+    if (isPermanentWriteError(error.code)) {
       // This was a permanent error, the request itself was the problem
       // so it's not going to succeed if we resend it.
       const batch = this.writePipeline.shift()!;

packages/firestore/src/remote/rpc_error.ts

@@ -48,6 +48,12 @@ enum RpcCode {
   DATA_LOSS = 15
 }
 
+/**
+ * Determines whether an error code represents a permanent error when received
+ * in response to a non-write operation.
+ *
+ * See isPermanentWriteError for classifying write errors.
+ */
 export function isPermanentError(code: Code): boolean {
   switch (code) {
     case Code.OK:
@@ -80,6 +86,22 @@ export function isPermanentError(code: Code): boolean {
   }
 }
 
+/**
+ * Determines whether an error code represents a permanent error when received
+ * in response to a write operation.
+ *
+ * Write operations must be handled specially because as of b/119437764, ABORTED
+ * errors on the write stream should be retried too (even though ABORTED errors
+ * are not generally retryable).
+ *
+ * Note that during the initial handshake on the write stream an ABORTED error
+ * signals that we should discard our stream token (i.e. it is permanent). This
+ * means a handshake error should be classified with isPermanentError, above.
+ */
+export function isPermanentWriteError(code: Code): boolean {
+  return isPermanentError(code) && code !== Code.ABORTED;
+}
+
 /**
  * Maps an error Code from a GRPC status identifier like 'NOT_FOUND'.
  *

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

@@ -25,7 +25,7 @@ import {
 import { DocumentKey } from '../../../src/model/document_key';
 import { JsonObject } from '../../../src/model/field_value';
 import {
-  isPermanentError,
+  isPermanentWriteError,
   mapCodeFromRpcCode,
   mapRpcCodeFromCode
 } from '../../../src/remote/rpc_error';
@@ -504,7 +504,8 @@ export class SpecBuilder {
 
     // If this is a permanent error, the write is not expected to be sent
     // again.
-    const isPermanentFailure = isPermanentError(mapCodeFromRpcCode(error.code));
+    const code = mapCodeFromRpcCode(error.code);
+    const isPermanentFailure = isPermanentWriteError(code);
     const keepInQueue =
       options.keepInQueue !== undefined
         ? options.keepInQueue

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

@@ -633,7 +633,6 @@ describeSpec('Writes:', [], () => {
     Code.ALREADY_EXISTS,
     Code.PERMISSION_DENIED,
     Code.FAILED_PRECONDITION,
-    Code.ABORTED,
     Code.OUT_OF_RANGE,
     Code.UNIMPLEMENTED,
     Code.DATA_LOSS
@@ -697,6 +696,7 @@ describeSpec('Writes:', [], () => {
   );
 
   for (const code of [
+    Code.ABORTED,
     Code.CANCELLED,
     Code.UNKNOWN,
     Code.DEADLINE_EXCEEDED,