RUBY-3700 Do not clear pool on retryable errors (#2990)

Author: comandeo-mongo

lib/mongo/server.rb

@@ -507,11 +507,13 @@ def with_connection(connection_global_id: nil, context: nil, &block)
     def handle_handshake_failure!
       yield
     rescue Mongo::Error::SocketError, Mongo::Error::SocketTimeoutError => e
-      unknown!(
-        generation: e.generation,
-        service_id: e.service_id,
-        stop_push_monitor: true,
-      )
+      unless e.label?('SystemOverloadedError')
+        unknown!(
+          generation: e.generation,
+          service_id: e.service_id,
+          stop_push_monitor: true,
+        )
+      end
       raise
     end
 

lib/mongo/server/connection_pool.rb

@@ -995,17 +995,26 @@ def maybe_raise_pool_cleared!(connection, e)
       def connect_connection(connection, context = nil)
         begin
           connection.connect!(context)
-        rescue Exception
+        rescue Exception => exc
+          # When a connection encounters an error during creation
+          # (including handshake and authentication), mark the server
+          # as Unknown so the pool is cleared (per CMAP spec).
+          # The unknown! call must happen before disconnect! so that
+          # the ConnectionPoolCleared event is published before the
+          # ConnectionClosed event.
+          # Errors with the SystemOverloadedError label (network errors
+          # during handshake) are excluded per the SDAM spec — those
+          # must not change the server description.
+          if exc.is_a?(Mongo::Error) && !exc.label?('SystemOverloadedError')
+            @server.unknown!(
+              generation: exc.generation,
+              service_id: exc.service_id,
+              stop_push_monitor: true,
+            )
+          end
           connection.disconnect!(reason: :error)
           raise
         end
-      rescue Error::SocketError, Error::SocketTimeoutError => exc
-        @server.unknown!(
-          generation: exc.generation,
-          service_id: exc.service_id,
-          stop_push_monitor: true,
-        )
-        raise
       end
 
       def check_invariants

lib/mongo/server/pending_connection.rb

@@ -160,6 +160,19 @@ def handshake!(speculative_auth_doc: nil)
               log_prefix: options[:log_prefix],
               bg_error_backtrace: options[:bg_error_backtrace],
             )
+            if exc.is_a?(::Mongo::Error)
+              # The SystemOverloadedError label marks errors as back-pressure
+              # signals that should not cause the server to be marked Unknown.
+              # Per the SDAM spec, this label only applies to network errors
+              # during the handshake. Command errors (e.g. OperationFailure
+              # with "node is shutting down" codes) must still flow through
+              # normal SDAM error handling so the server is marked Unknown
+              # and the pool is cleared.
+              if exc.is_a?(Error::SocketError) || exc.is_a?(Error::SocketTimeoutError)
+                exc.add_label('SystemOverloadedError')
+                exc.add_label('RetryableError')
+              end
+            end
             raise
           end
         end

lib/mongo/session.rb

@@ -456,13 +456,26 @@ def with_transaction(options = nil)
                    Utils.monotonic_time + 120
                  end
       transaction_in_progress = false
+      transaction_attempt = 0
+      last_error = nil
+
       loop do
+        if transaction_attempt > 0
+          backoff = backoff_seconds_for_retry(transaction_attempt)
+          if backoff_would_exceed_deadline?(deadline, backoff)
+            raise(last_error)
+          end
+          sleep(backoff)
+        end
+
         commit_options = {}
         if options
           commit_options[:write_concern] = options[:write_concern]
         end
         start_transaction(options)
         transaction_in_progress = true
+        transaction_attempt += 1
+
         begin
           rv = yield self
         rescue Exception => e
@@ -479,6 +492,7 @@ def with_transaction(options = nil)
           end
 
           if e.is_a?(Mongo::Error) && e.label?('TransientTransactionError')
+            last_error = e
             next
           end
 
@@ -495,7 +509,7 @@ def with_transaction(options = nil)
             return rv
           rescue Mongo::Error => e
             if e.label?('UnknownTransactionCommitResult')
-              if  deadline_expired?(deadline) ||
+              if deadline_expired?(deadline) ||
                 e.is_a?(Error::OperationFailure::Family) && e.max_time_ms_expired?
               then
                 transaction_in_progress = false
@@ -516,6 +530,7 @@ def with_transaction(options = nil)
                 transaction_in_progress = false
                 raise
               end
+              last_error = e
               @state = NO_TRANSACTION_STATE
               next
             else
@@ -1312,5 +1327,20 @@ def deadline_expired?(deadline)
         Utils.monotonic_time >= deadline
       end
     end
+
+    # Exponential backoff settings for with_transaction retries.
+    BACKOFF_INITIAL = 0.005
+    BACKOFF_MAX = 0.5
+
+    def backoff_seconds_for_retry(transaction_attempt)
+      exponential = BACKOFF_INITIAL * (1.5 ** (transaction_attempt - 1))
+      Random.rand * [exponential, BACKOFF_MAX].min
+    end
+
+    def backoff_would_exceed_deadline?(deadline, backoff_seconds)
+      return false if deadline.zero?
+
+      Utils.monotonic_time + backoff_seconds >= deadline
+    end
   end
 end

spec/mongo/server/connection_spec.rb

@@ -257,7 +257,6 @@ class ConnectionSpecTestException < Exception; end
         end
 
         it_behaves_like 'failing connection with server diagnostics'
-        it_behaves_like 'marks server unknown'
         it_behaves_like 'logs a warning'
         it_behaves_like 'adds server diagnostics'
 

spec/mongo/session_transaction_prose_spec.rb

@@ -0,0 +1,92 @@
+# frozen_string_literal: true
+# rubocop:todo all
+
+require 'spec_helper'
+
+describe Mongo::Session do
+  require_topology :replica_set
+
+  describe 'transactions convenient API prose tests' do
+    let(:client) { authorized_client }
+    let(:admin_client) { authorized_client.use('admin') }
+    let(:collection) { client['session-transaction-prose-test'] }
+
+    before do
+      collection.delete_many
+    end
+
+    after do
+      disable_fail_command
+    end
+
+    # Prose test from:
+    # specifications/source/transactions-convenient-api/tests/README.md
+    # ### Retry Backoff is Enforced
+    it 'adds measurable delay when jitter is enabled' do
+      skip 'failCommand fail point is not available' unless fail_command_available?
+
+      no_backoff_time = with_fixed_jitter(0) do
+        with_commit_failures(13) do
+          measure_with_transaction_time do |session|
+            collection.insert_one({}, session: session)
+          end
+        end
+      end
+
+      with_backoff_time = with_fixed_jitter(1) do
+        with_commit_failures(13) do
+          measure_with_transaction_time do |session|
+            collection.insert_one({}, session: session)
+          end
+        end
+      end
+
+      # Sum of 13 backoffs per spec is approximately 1.8 seconds.
+      expect(with_backoff_time).to be_within(0.5).of(no_backoff_time + 1.8)
+    end
+
+    private
+
+    def measure_with_transaction_time
+      start_time = Mongo::Utils.monotonic_time
+      client.start_session do |session|
+        session.with_transaction do
+          yield(session)
+        end
+      end
+      Mongo::Utils.monotonic_time - start_time
+    end
+
+    def with_fixed_jitter(value)
+      allow(Random).to receive(:rand).and_return(value)
+      yield
+    end
+
+    def with_commit_failures(times)
+      admin_client.command(
+        configureFailPoint: 'failCommand',
+        mode: { times: times },
+        data: {
+          failCommands: ['commitTransaction'],
+          errorCode: 251,
+        },
+      )
+      yield
+    ensure
+      disable_fail_command
+    end
+
+    def disable_fail_command
+      admin_client.command(configureFailPoint: 'failCommand', mode: 'off')
+    rescue Mongo::Error
+      # Ignore cleanup failures.
+    end
+
+    def fail_command_available?
+      admin_client.command(configureFailPoint: 'failCommand', mode: 'off')
+      true
+    rescue Mongo::Error
+      false
+    end
+  end
+end

spec/mongo/session_transaction_spec.rb

@@ -264,5 +264,45 @@ class SessionTransactionSpecError < StandardError; end
         end
       end
     end
+
+    context 'backoff calculation' do
+      require_topology :replica_set
+
+      it 'calculates exponential backoff correctly' do
+        # Test backoff formula: jitter * min(BACKOFF_INITIAL * 1.5^(attempt-1), BACKOFF_MAX)
+        backoff_initial = Mongo::Session::BACKOFF_INITIAL
+        backoff_max = Mongo::Session::BACKOFF_MAX
+
+        # Test attempt 1: 1.5^0 = 1
+        expected_attempt_1 = backoff_initial * (1.5 ** 0)
+        expect(expected_attempt_1).to eq(0.005)
+
+        # Test attempt 2: 1.5^1 = 1.5
+        expected_attempt_2 = backoff_initial * (1.5 ** 1)
+        expect(expected_attempt_2).to eq(0.0075)
+
+        # Test attempt 3: 1.5^2 = 2.25
+        expected_attempt_3 = backoff_initial * (1.5 ** 2)
+        expect(expected_attempt_3).to eq(0.01125)
+
+        # Test cap at BACKOFF_MAX
+        expected_attempt_large = [backoff_initial * (1.5 ** 20), backoff_max].min
+        expect(expected_attempt_large).to eq(backoff_max)
+      end
+
+      it 'applies jitter to backoff' do
+        # Jitter should be a random value between 0 and 1
+        # When multiplied with backoff, it should reduce the actual sleep time
+        backoff = 0.100  # 100ms
+        jitter_min = 0
+        jitter_max = 1
+
+        actual_min = jitter_min * backoff
+        actual_max = jitter_max * backoff
+
+        expect(actual_min).to eq(0)
+        expect(actual_max).to eq(0.100)
+      end
+    end
   end
 end

spec/spec_tests/data/cmap/pool-create-min-size-error.yml

@@ -2,18 +2,17 @@ version: 1
 style: integration
 description: error during minPoolSize population clears pool
 runOn:
-  -
-    # required for appName in fail point
+  - # required for appName in fail point
     minServerVersion: "4.9.0"
   # Remove the topology runOn requirement when cmap specs are adjusted for lbs
-  - topology: [ "single", "replicaset", "sharded" ]
+  - topology: ["single", "replicaset", "sharded"]
 failPoint:
   configureFailPoint: failCommand
   # high amount to ensure not interfered with by monitor checks.
   mode: { times: 50 }
   data:
-    failCommands: ["isMaster","hello"]
-    closeConnection: true
+    failCommands: ["isMaster", "hello"]
+    errorCode: 91
     appName: "poolCreateMinSizeErrorTest"
 poolOptions:
   minPoolSize: 1

spec/spec_tests/data/sdam_unified/backpressure-network-error-fail-replicaset.yml

@@ -0,0 +1,80 @@
+description: backpressure-network-error-fail-replicaset
+schemaVersion: "1.17"
+runOnRequirements:
+  - minServerVersion: "4.4"
+    serverless: forbid
+    topologies:
+      - replicaset
+createEntities:
+  - client:
+      id: setupClient
+      useMultipleMongoses: false
+initialData:
+  - collectionName: backpressure-network-error-fail
+    databaseName: sdam-tests
+    documents:
+      - _id: 1
+      - _id: 2
+tests:
+  - description: apply backpressure on network connection errors during connection establishment
+    operations:
+      - name: createEntities
+        object: testRunner
+        arguments:
+          entities:
+            - client:
+                id: client
+                useMultipleMongoses: false
+                observeEvents:
+                  - serverDescriptionChangedEvent
+                  - poolClearedEvent
+                uriOptions:
+                  retryWrites: false
+                  heartbeatFrequencyMS: 1000000
+                  serverMonitoringMode: poll
+                  appname: backpressureNetworkErrorFailTest
+            - database:
+                id: database
+                client: client
+                databaseName: sdam-tests
+            - collection:
+                id: collection
+                database: database
+                collectionName: backpressure-network-error-fail
+      - name: waitForEvent
+        object: testRunner
+        arguments:
+          client: client
+          event:
+            serverDescriptionChangedEvent:
+              newDescription:
+                type: RSPrimary
+          count: 1
+      - name: failPoint
+        object: testRunner
+        arguments:
+          client: setupClient
+          failPoint:
+            configureFailPoint: failCommand
+            mode: alwaysOn
+            data:
+              failCommands:
+                - isMaster
+                - hello
+              appName: backpressureNetworkErrorFailTest
+              closeConnection: true
+      - name: insertMany
+        object: collection
+        arguments:
+          documents:
+            - _id: 3
+            - _id: 4
+        expectError:
+          isError: true
+          errorLabelsContain:
+            - SystemOverloadedError
+            - RetryableError
+    expectEvents:
+      - client: client
+        eventType: cmap
+        events: []