fix: only mark server session dirty if the client session is alive

There are race conditions where a `ClientSession` ends before the
error handling code for an operation marks the session as dirty.
This error handling code needs to check if the session has not
ended before marking the server session dirty.

NODE-2545

Author: mbroadst

lib/core/sdam/server.js

@@ -463,11 +463,13 @@ function markServerUnknown(server, error) {
 }
 
 function makeOperationHandler(server, options, callback) {
+  const session = options && options.session;
+
   return function handleOperationResult(err, result) {
     if (err) {
       if (err instanceof MongoNetworkError) {
-        if (options && options.session) {
-          options.session.serverSession.isDirty = true;
+        if (session && !session.hasEnded) {
+          session.serverSession.isDirty = true;
         }
 
         if (!isNetworkTimeoutError(err)) {

lib/core/sessions.js

@@ -123,14 +123,14 @@ class ClientSession extends EventEmitter {
       this.abortTransaction(); // pass in callback?
     }
 
-    // mark the session as ended, and emit a signal
-    this.hasEnded = true;
-    this.emit('ended', this);
-
     // release the server session back to the pool
     this.sessionPool.release(this.serverSession);
     this.serverSession = null;
 
+    // mark the session as ended, and emit a signal
+    this.hasEnded = true;
+    this.emit('ended', this);
+
     // spec indicates that we should ignore all errors for `endSessions`
     if (typeof callback === 'function') callback(null, null);
   }

test/unit/core/sessions.test.js

@@ -214,4 +214,52 @@ describe('Sessions', function() {
       }
     });
   });
+
+  context('error handling', function() {
+    let mockServer;
+
+    afterEach(() => mock.cleanup());
+    beforeEach(() => {
+      return mock.createServer().then(server => (mockServer = server));
+    });
+
+    it('should not mark session as dirty on network error if already ended', function(done) {
+      mockServer.setMessageHandler(request => {
+        const doc = request.document;
+        if (doc.ismaster) {
+          request.reply(
+            Object.assign({}, mock.DEFAULT_ISMASTER, { logicalSessionTimeoutMinutes: 10 })
+          );
+        } else if (doc.ping) {
+          request.reply({ ok: 1 });
+        } else if (doc.endSessions) {
+          request.reply({ ok: 1 });
+        } else if (doc.insert) {
+          request.connection.destroy();
+        }
+      });
+
+      const client = this.configuration.newClient(`mongodb://${mockServer.uri()}/test`, {
+        useUnifiedTopology: true
+      });
+
+      client.connect(err => {
+        expect(err).to.not.exist;
+        this.defer(() => client.close());
+
+        const session = client.startSession();
+        const collection = client.db('test').collection('foo');
+
+        client.db('admin').command({ ping: 1 }, { session }, err => {
+          expect(err).to.not.exist;
+
+          process.nextTick(() => session.endSession());
+          collection.insertOne({ a: 42 }, { session }, err => {
+            expect(err).to.exist;
+            done();
+          });
+        });
+      });
+    });
+  });
 });