feat(NODE-4650): handle handshake errors with SDAM

src/cmap/connection_pool.ts

@@ -16,7 +16,13 @@ import {
   CONNECTION_POOL_READY,
   CONNECTION_READY
 } from '../constants';
-import { MongoError, MongoInvalidArgumentError, MongoRuntimeError } from '../error';
+import {
+  MongoError,
+  MongoInvalidArgumentError,
+  MongoNetworkError,
+  MongoRuntimeError,
+  MongoServerError
+} from '../error';
 import { Logger } from '../logger';
 import { CancellationToken, TypedEventEmitter } from '../mongo_types';
 import type { Server } from '../sdam/server';
@@ -601,6 +607,9 @@ export class ConnectionPool extends TypedEventEmitter<ConnectionPoolEvents> {
           ConnectionPool.CONNECTION_CLOSED,
           new ConnectionClosedEvent(this, { id: connectOptions.id, serviceId: undefined }, 'error')
         );
+        if (err instanceof MongoNetworkError || err instanceof MongoServerError) {
+          err.connectionGeneration = connectOptions.generation;
+        }
         callback(err ?? new MongoRuntimeError('Connection creation failed without error'));
         return;
       }

src/error.ts

@@ -122,6 +122,7 @@ export class MongoError extends Error {
    */
   code?: number | string;
   topologyVersion?: TopologyVersion;
+  connectionGeneration?: number;
   // eslint-disable-next-line @typescript-eslint/ban-ts-comment
   // @ts-ignore
   cause?: Error; // depending on the node version, this may or may not exist on the base

src/sdam/server.ts

@@ -29,6 +29,7 @@ import {
   MongoInvalidArgumentError,
   MongoNetworkError,
   MongoNetworkTimeoutError,
+  MongoRuntimeError,
   MongoServerClosedError,
   MongoServerError,
   MongoUnexpectedServerResponseError,
@@ -348,8 +349,11 @@ export class Server extends TypedEventEmitter<ServerEvents> {
       (err, conn, cb) => {
         if (err || !conn) {
           this.s.operationCount -= 1;
+          if (!err) {
+            return cb(new MongoRuntimeError('Failed to create connection without error'));
+          }
           if (!(err instanceof PoolClearedError)) {
-            markServerUnknown(this, err);
+            this.handleError(err);
           } else {
             err.addErrorLabel(MongoErrorLabel.RetryableWriteError);
           }
@@ -378,17 +382,25 @@ export class Server extends TypedEventEmitter<ServerEvents> {
     if (!(error instanceof MongoError)) {
       return;
     }
-    if (error instanceof MongoNetworkError) {
-      if (!(error instanceof MongoNetworkTimeoutError) || isNetworkErrorBeforeHandshake(error)) {
-        // In load balanced mode we never mark the server as unknown and always
-        // clear for the specific service id.
-
-        if (!this.loadBalanced) {
-          error.addErrorLabel(MongoErrorLabel.ResetPool);
-          markServerUnknown(this, error);
-        } else if (connection) {
-          this.s.pool.clear(connection.serviceId);
-        }
+
+    const isStaleError =
+      error.connectionGeneration && error.connectionGeneration < this.s.pool.generation;
+    if (isStaleError) {
+      return;
+    }
+
+    const isNetworkNonTimeoutError =
+      error instanceof MongoNetworkError && !(error instanceof MongoNetworkTimeoutError);
+    const isNetworkTimeoutBeforeHandshakeError = isNetworkErrorBeforeHandshake(error);
+    const isAuthHandshakeError = error.hasErrorLabel(MongoErrorLabel.HandshakeError);
+    if (isNetworkNonTimeoutError || isNetworkTimeoutBeforeHandshakeError || isAuthHandshakeError) {
+      // In load balanced mode we never mark the server as unknown and always
+      // clear for the specific service id.
+      if (!this.loadBalanced) {
+        error.addErrorLabel(MongoErrorLabel.ResetPool);
+        markServerUnknown(this, error);
+      } else if (connection) {
+        this.s.pool.clear(connection.serviceId);
       }
     } else {
       if (isSDAMUnrecoverableError(error)) {

test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts

@@ -8,17 +8,7 @@ import { TestFilter } from '../../tools/unified-spec-runner/schema';
 import { sleep } from '../../tools/utils';
 
 const filter: TestFilter = ({ description }) => {
-  const isAuthEnabled = process.env.AUTH === 'auth';
   switch (description) {
-    case 'Reset server and pool after AuthenticationFailure error':
-    case 'Reset server and pool after misc command error':
-    case 'Reset server and pool after network error during authentication':
-    case 'Reset server and pool after network timeout error during authentication':
-    case 'Reset server and pool after shutdown error during authentication':
-      // These tests time out waiting for the PoolCleared event
-      return isAuthEnabled
-        ? 'TODO(NODE-3135): handle auth errors, also see NODE-3891: fix tests broken when AUTH enabled'
-        : false;
     case 'Network error on Monitor check':
     case 'Network timeout on Monitor check':
       return 'TODO(NODE-4608): Disallow parallel monitor checks';

test/unit/sdam/server.test.ts

@@ -0,0 +1,162 @@
+/* eslint-disable @typescript-eslint/no-non-null-assertion */
+import { expect } from 'chai';
+import { once } from 'events';
+import * as sinon from 'sinon';
+
+import {
+  Connection,
+  MongoError,
+  MongoErrorLabel,
+  MongoNetworkError,
+  MongoNetworkTimeoutError,
+  ObjectId,
+  ServerType,
+  TopologyType
+} from '../../../src';
+import { Server } from '../../../src/sdam/server';
+import { ServerDescription } from '../../../src/sdam/server_description';
+import { Topology } from '../../../src/sdam/topology';
+import { sleep } from '../../tools/utils';
+
+const handledErrors = [
+  {
+    description: 'any non-timeout network error',
+    errorClass: MongoNetworkError,
+    errorArgs: ['TestError']
+  },
+  {
+    description: 'a network timeout error before handshake',
+    errorClass: MongoNetworkTimeoutError,
+    errorArgs: ['TestError', { beforeHandshake: true }]
+  },
+  {
+    description: 'an auth handshake error',
+    errorClass: MongoError,
+    errorArgs: ['TestError'],
+    errorLabel: MongoErrorLabel.HandshakeError
+  }
+];
+
+const unhandledErrors = [
+  {
+    description: 'a non-MongoError',
+    errorClass: Error,
+    errorArgs: ['TestError']
+  },
+  {
+    description: 'a network timeout error after handshake',
+    errorClass: MongoNetworkTimeoutError,
+    errorArgs: ['TestError', { beforeHandshake: false }]
+  },
+  {
+    description: 'a non-network non-handshake MongoError',
+    errorClass: MongoError,
+    errorArgs: ['TestError']
+  }
+];
+
+describe('Server', () => {
+  describe('#handleError', () => {
+    let server: Server, connection: Connection | undefined;
+    beforeEach(() => {
+      server = new Server(new Topology([], {} as any), new ServerDescription('a:1'), {} as any);
+    });
+    for (const loadBalanced of [true, false]) {
+      const mode = loadBalanced ? 'loadBalanced' : 'non-loadBalanced';
+      const contextSuffix = loadBalanced ? ' with connection provided' : '';
+      context(`in ${mode} mode${contextSuffix}`, () => {
+        beforeEach(() => {
+          if (loadBalanced) {
+            server.s.topology.description.type = TopologyType.LoadBalanced;
+            connection = { serviceId: new ObjectId() } as Connection;
+            server.s.pool.clear = sinon.stub();
+          } else {
+            connection = undefined;
+          }
+        });
+        for (const { description, errorClass, errorArgs, errorLabel } of handledErrors) {
+          const handledDescription = loadBalanced
+            ? `should reset the pool but not attach a ResetPool label to the error or mark the server unknown on ${description}`
+            : `should attach a ResetPool label to the error and mark the server unknown on ${description}`;
+          it(`${handledDescription}`, async () => {
+            // @ts-expect-error because of varied number of args
+            const error = new errorClass(...errorArgs);
+            if (errorLabel) {
+              error.addErrorLabel(errorLabel);
+            }
+            const newDescriptionEvent = Promise.race([
+              once(server, Server.DESCRIPTION_RECEIVED),
+              sleep(1000)
+            ]);
+            server.handleError(error, connection);
+            if (!loadBalanced) {
+              expect(
+                error.hasErrorLabel(MongoErrorLabel.ResetPool),
+                'expected error to have a ResetPool label'
+              ).to.be.true;
+            } else {
+              expect(
+                error.hasErrorLabel(MongoErrorLabel.ResetPool),
+                'expected error NOT to have a ResetPool label'
+              ).to.be.false;
+            }
+            const newDescription = await newDescriptionEvent;
+            if (!loadBalanced) {
+              expect(newDescription).to.have.nested.property('[0].type', ServerType.Unknown);
+            } else {
+              expect(newDescription).to.be.undefined;
+              expect(server.s.pool.clear).to.have.been.calledOnceWith(connection!.serviceId);
+            }
+          });
+
+          it(`should not attach a ResetPool label to the error or mark the server unknown on ${description} if it is stale`, async () => {
+            // @ts-expect-error because of varied number of args
+            const error = new errorClass(...errorArgs);
+            if (errorLabel) {
+              error.addErrorLabel(errorLabel);
+            }
+
+            error.connectionGeneration = -1;
+            expect(
+              server.s.pool.generation,
+              'expected test server to have a pool of generation 0'
+            ).to.equal(0); // sanity check
+
+            const newDescriptionEvent = Promise.race([
+              once(server, Server.DESCRIPTION_RECEIVED),
+              sleep(1000)
+            ]);
+            server.handleError(error, connection);
+            expect(
+              error.hasErrorLabel(MongoErrorLabel.ResetPool),
+              'expected error NOT to have a ResetPool label'
+            ).to.be.false;
+            const newDescription = await newDescriptionEvent;
+            expect(newDescription).to.be.undefined;
+          });
+        }
+
+        for (const { description, errorClass, errorArgs } of unhandledErrors) {
+          it(`should not attach a ResetPool label to the error or mark the server unknown on ${description}`, async () => {
+            // @ts-expect-error because of varied number of args
+            const error = new errorClass(...errorArgs);
+
+            const newDescriptionEvent = Promise.race([
+              once(server, Server.DESCRIPTION_RECEIVED),
+              sleep(1000)
+            ]);
+            server.handleError(error, connection);
+            if (error instanceof MongoError) {
+              expect(
+                error.hasErrorLabel(MongoErrorLabel.ResetPool),
+                'expected error NOT to have a ResetPool label'
+              ).to.be.false;
+            }
+            const newDescription = await newDescriptionEvent;
+            expect(newDescription).to.be.undefined;
+          });
+        }
+      });
+    }
+  });
+});