Lock service instance to prevent concurrent binding creation

Author: johha

app/actions/service_credential_binding_app_create.rb

@@ -30,7 +30,10 @@ def precursor(service_instance, message:, app: nil, volume_mount_services_enable
 
         ServiceBinding.new.tap do |new_binding|
           ServiceBinding.db.transaction do
-            validate_app_guid_name_uniqueness!(app.guid, message.name, service_instance.guid) # if max_bindings_per_app_service_instance == 1
+            # Lock the service instance to prevent multiple bindings being created when not allowed
+            service_instance.lock! if max_bindings_per_app_service_instance == 1
+
+            validate_app_guid_name_uniqueness!(app.guid, message.name, service_instance.guid)
 
             num_valid_bindings = 0
             existing_bindings = ServiceBinding.where(service_instance:, app:)

spec/unit/actions/service_credential_binding_app_create_spec.rb

@@ -197,11 +197,6 @@ module V3
                 action.precursor(service_instance, app:, message:)
               end.not_to raise_error
 
-              # Mock the validation for the second request to simulate the race condition and trigger a
-              # unique constraint violation on service_instance_guid + app_guid
-              allow_any_instance_of(ServiceBinding).to receive(:validate).and_return(true)
-              allow(ServiceBinding).to receive(:first).with(service_instance:, app:).and_return(nil)
-
               # Second request, should fail with correct error
               expect do
                 action.precursor(service_instance, app: app, message: message2)
@@ -233,6 +228,29 @@ module V3
             )
           end
 
+          context 'concurrent credential binding creation' do
+            let(:name) { nil }
+
+            # TODO: Once the unique constraints to allow multiple bindings are removed, this needs to be set to 1
+            # before { TestConfig.override(max_service_credential_bindings_per_app_service_instance: 1) }
+
+            def attempt_precursor
+              action.precursor(service_instance, app:, message:)
+              :ok
+            rescue StandardError => e
+              e
+            end
+
+            it 'allows only one binding when two creates run in parallel' do
+              results = [Thread.new { attempt_precursor }, Thread.new { attempt_precursor }].map(&:value)
+
+              expect(ServiceBinding.where(app:, service_instance:).count).to eq(1)
+              expect(results.count(:ok)).to eq(1)
+              expect(results.count { |r| r.is_a?(VCAP::CloudController::V3::ServiceBindingCreate::UnprocessableCreate) }).to eq(1)
+              expect(results.grep(Exception).map(&:message)).to include('The app is already bound to the service instance')
+            end
+          end
+
           context 'when multiple bindings are allowed' do
             before do
               # TODO: Remove skip when the service bindings unique constraints are removed