fix(NODE-3688): make handshake errors retryable

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')));
+});

test/spec/retryable-reads/README.rst

@@ -9,8 +9,10 @@ Retryable Reads Tests
 Introduction
 ============
 
-The YAML and JSON files in this directory tree are platform-independent tests
-that drivers can use to prove their conformance to the Retryable Reads spec.
+The YAML and JSON files in the ``legacy`` and ``unified`` sub-directories are platform-independent tests
+that drivers can use to prove their conformance to the Retryable Reads spec. Tests in the
+``unified`` directory are written using the `Unified Test Format <../../unified-test-format/unified-test-format.rst>`_.
+Tests in the ``legacy`` directory are written using the format described below.
 
 Prose tests, which are not easily expressed in YAML, are also presented
 in this file. Those tests will need to be manually implemented by each driver.
@@ -79,29 +81,49 @@ Each YAML file has the following keys:
     the default is all topologies (i.e. ``["single", "replicaset", "sharded",
     "load-balanced"]``).
 
+  - ``serverless``: Optional string. Whether or not the test should be run on
+    serverless instances imitating sharded clusters. Valid values are "require",
+    "forbid", and "allow". If "require", the test MUST only be run on serverless
+    instances. If "forbid", the test MUST NOT be run on serverless instances. If
+    omitted or "allow", this option has no effect.
+
+    The test runner MUST be informed whether or not serverless is being used in
+    order to determine if this requirement is met (e.g. through an environment
+    variable or configuration option). Since the serverless proxy imitates a
+    mongos, the runner is not capable of determining this by issuing a server
+    command such as ``buildInfo`` or ``hello``.
+
 - ``database_name`` and ``collection_name``: Optional. The database and
   collection to use for testing.
-  
+
 - ``bucket_name``: Optional. The GridFS bucket name to use for testing.
 
 - ``data``: The data that should exist in the collection(s) under test before
   each test run. This will typically be an array of documents to be inserted
   into the collection under test (i.e. ``collection_name``); however, this field
   may also be an object mapping collection names to arrays of documents to be
   inserted into the specified collection.
-    
+
 - ``tests``: An array of tests that are to be run independently of each other.
   Each test will have some or all of the following fields:
 
   - ``description``: The name of the test.
-    
+
   - ``clientOptions``: Optional, parameters to pass to MongoClient().
 
-  - ``useMultipleMongoses`` (optional): If ``true``, the MongoClient for this
-    test should be initialized with multiple mongos seed addresses. If ``false``
-    or omitted, only a single mongos address should be specified. This field has
-    no effect for non-sharded topologies.
-
+  - ``useMultipleMongoses`` (optional): If ``true``, and the topology type is
+    ``Sharded``, the MongoClient for this test should be initialized with multiple
+    mongos seed addresses. If ``false`` or omitted, only a single mongos address
+    should be specified.
+
+    If ``true``, and the topology type is ``LoadBalanced``, the MongoClient for
+    this test should be initialized with the URI of the load balancer fronting
+    multiple servers. If ``false`` or omitted, the MongoClient for this test
+    should be initialized with the URI of the load balancer fronting a single
+    server.
+
+    ``useMultipleMongoses`` only affects ``Sharded`` and ``LoadBalanced`` topologies.
+
   - ``skipReason``: Optional, string describing why this test should be skipped.
 
   - ``failPoint``: Optional, a server fail point to enable, expressed as the
@@ -120,10 +142,10 @@ Each YAML file has the following keys:
     - ``result``: Optional. The return value from the operation, if any. This
       field may be a scalar (e.g. in the case of a count), a single document, or
       an array of documents in the case of a multi-document read.
-      
+
     - ``error``: Optional. If ``true``, the test should expect an error or
       exception.
-        
+
   - ``expectations``: Optional list of command-started events.
 
 GridFS Tests
@@ -147,7 +169,8 @@ data.
 
 
 .. _GridFSBucket spec: https://github.com/mongodb/specifications/blob/master/source/gridfs/gridfs-spec.rst#configurable-gridfsbucket-class
-
+
+
 Speeding Up Tests
 -----------------
 
@@ -165,9 +188,53 @@ Optional Enumeration Commands
 A driver only needs to test the optional enumeration commands it has chosen to
 implement (e.g. ``Database.listCollectionNames()``).
 
+PoolClearedError Retryability Test
+==================================
+
+This test will be used to ensure drivers properly retry after encountering PoolClearedErrors.
+It MUST be implemented by any driver that implements the CMAP specification.
+This test requires MongoDB 4.2.9+ for ``blockConnection`` support in the failpoint.
+
+1. Create a client with maxPoolSize=1 and retryReads=true. If testing against a
+   sharded deployment, be sure to connect to only a single mongos.
+
+2. Enable the following failpoint::
+
+     {
+         configureFailPoint: "failCommand",
+         mode: { times: 1 },
+         data: {
+             failCommands: ["find"],
+             errorCode: 91,
+             blockConnection: true,
+             blockTimeMS: 1000
+         }
+     }
+
+3. Start two threads and attempt to perform a ``findOne`` simultaneously on both.
+
+4. Verify that both ``findOne`` attempts succeed.
+
+5. Via CMAP monitoring, assert that the first check out succeeds.
+
+6. Via CMAP monitoring, assert that a PoolClearedEvent is then emitted.
+
+7. Via CMAP monitoring, assert that the second check out then fails due to a
+   connection error.
+
+8. Via Command Monitoring, assert that exactly three ``find`` CommandStartedEvents
+   were observed in total.
+
+9. Disable the failpoint.
+
+
 Changelog
 =========
 
+:2022-01-10: Create legacy and unified subdirectories for new unified tests
+
+:2021-08-27: Clarify behavior of ``useMultipleMongoses`` for ``LoadBalanced`` topologies.
+
 :2019-03-19: Add top-level ``runOn`` field to denote server version and/or
              topology requirements requirements for the test file. Removes the
              ``minServerVersion`` and ``topology`` top-level fields, which are
@@ -177,4 +244,6 @@ Changelog
 
 :2020-09-16: Suggest lowering heartbeatFrequencyMS in addition to minHeartbeatFrequencyMS.
 
-:2021-04-23: Add ``load-balanced`` to test topology requirements.
+:2021-03-23: Add prose test for retrying PoolClearedErrors
+
+:2021-04-29: Add ``load-balanced`` to test topology requirements.