fix(NODE-3688): make handshake errors retryable (mongodb#3165)

Author: durran

src/cmap/connect.ts

@@ -10,11 +10,14 @@ import { LEGACY_HELLO_COMMAND } from '../constants';
 import {
   AnyError,
   MongoCompatibilityError,
+  MongoError,
+  MongoErrorLabel,
   MongoInvalidArgumentError,
   MongoNetworkError,
   MongoNetworkTimeoutError,
   MongoRuntimeError,
-  MongoServerError
+  MongoServerError,
+  needsRetryableWriteLabel
 } from '../error';
 import { Callback, ClientMetadata, HostAddress, makeClientMetadata, ns } from '../utils';
 import { AuthContext, AuthProvider } from './auth/auth_provider';
@@ -182,7 +185,15 @@ function performInitialHandshake(
           );
         }
         provider.auth(authContext, err => {
-          if (err) return callback(err);
+          if (err) {
+            if (err instanceof MongoError) {
+              err.addErrorLabel(MongoErrorLabel.HandshakeError);
+              if (needsRetryableWriteLabel(err, response.maxWireVersion)) {
+                err.addErrorLabel(MongoErrorLabel.RetryableWriteError);
+              }
+            }
+            return callback(err);
+          }
           callback(undefined, conn);
         });
 

src/error.ts

@@ -88,7 +88,8 @@ export const MongoErrorLabel = Object.freeze({
   RetryableWriteError: 'RetryableWriteError',
   TransientTransactionError: 'TransientTransactionError',
   UnknownTransactionCommitResult: 'UnknownTransactionCommitResult',
-  ResumableChangeStreamError: 'ResumableChangeStreamError'
+  ResumableChangeStreamError: 'ResumableChangeStreamError',
+  HandshakeError: 'HandshakeError'
 } as const);
 
 /** @public */
@@ -744,21 +745,22 @@ const RETRYABLE_WRITE_ERROR_CODES = new Set<number>([
 ]);
 
 export function needsRetryableWriteLabel(error: Error, maxWireVersion: number): boolean {
-  if (maxWireVersion >= 9) {
-    // 4.4+ servers attach their own retryable write error
-    return false;
-  }
   // pre-4.4 server, then the driver adds an error label for every valid case
   // execute operation will only inspect the label, code/message logic is handled here
-
   if (error instanceof MongoNetworkError) {
     return true;
   }
 
-  if (error instanceof MongoError && error.hasErrorLabel(MongoErrorLabel.RetryableWriteError)) {
-    // Before 4.4 the error label can be one way of identifying retry
-    // so we can return true if we have the label, but fall back to code checking below
-    return true;
+  if (error instanceof MongoError) {
+    if (
+      (maxWireVersion >= 9 || error.hasErrorLabel(MongoErrorLabel.RetryableWriteError)) &&
+      !error.hasErrorLabel(MongoErrorLabel.HandshakeError)
+    ) {
+      // If we already have the error label no need to add it again. 4.4+ servers add the label.
+      // In the case where we have a handshake error, need to fall down to the logic checking
+      // the codes.
+      return false;
+    }
   }
 
   if (error instanceof MongoWriteConcernError) {
@@ -782,6 +784,10 @@ export function needsRetryableWriteLabel(error: Error, maxWireVersion: number):
   return false;
 }
 
+export function isRetryableWriteError(error: MongoError): boolean {
+  return error.hasErrorLabel(MongoErrorLabel.RetryableWriteError);
+}
+
 /** Determines whether an error is something the driver should attempt to retry */
 export function isRetryableReadError(error: MongoError): boolean {
   const hasRetryableErrorCode =

src/operations/execute_operation.ts

@@ -1,10 +1,10 @@
 import type { Document } from '../bson';
 import {
   isRetryableReadError,
+  isRetryableWriteError,
   MongoCompatibilityError,
   MONGODB_ERROR_CODES,
   MongoError,
-  MongoErrorLabel,
   MongoExpiredSessionError,
   MongoNetworkError,
   MongoRuntimeError,
@@ -195,7 +195,7 @@ function executeWithServerSelection<TResult>(
       );
     }
 
-    if (isWriteOperation && !originalError.hasErrorLabel(MongoErrorLabel.RetryableWriteError)) {
+    if (isWriteOperation && !isRetryableWriteError(originalError)) {
       return callback(originalError);
     }
 

test/integration/retryable-reads/retryable_reads.spec.test.js

@@ -1,11 +1,11 @@
-'use strict';
-
+const path = require('path');
 const { TestRunnerContext, generateTopologyTests } = require('../../tools/spec-runner');
 const { loadSpecTests } = require('../../spec');
+const { runUnifiedSuite } = require('../../tools/unified-spec-runner/runner');
 
-describe('Retryable Reads', function () {
+describe('Retryable Reads (legacy)', function () {
   const testContext = new TestRunnerContext();
-  const testSuites = loadSpecTests('retryable-reads');
+  const testSuites = loadSpecTests(path.join('retryable-reads', 'legacy'));
 
   after(() => testContext.teardown());
   before(function () {
@@ -28,3 +28,7 @@ describe('Retryable Reads', function () {
     );
   });
 });
+
+describe('Retryable Reads (unified)', function () {
+  runUnifiedSuite(loadSpecTests(path.join('retryable-reads', 'unified')));
+});

test/integration/retryable-writes/retryable_writes.spec.test.ts

@@ -1,8 +1,10 @@
 import { expect } from 'chai';
+import * as path from 'path';
 
 import type { Collection, Db, MongoClient } from '../../../src';
 import { loadSpecTests } from '../../spec';
 import { legacyRunOnToRunOnRequirement } from '../../tools/spec-runner';
+import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner';
 import { isAnyRequirementSatisfied } from '../../tools/unified-spec-runner/unified-utils';
 
 interface RetryableWriteTestContext {
@@ -196,3 +198,7 @@ async function turnOffFailPoint(client, name) {
     mode: 'off'
   });
 }
+
+describe('Retryable Writes (unified)', function () {
+  runUnifiedSuite(loadSpecTests(path.join('retryable-writes', 'unified')));
+});