Prevent 500 on parallel object creation (#3918)

kathap · web-flow · commit 38855fd752ac · 2024-08-22T14:27:15.000+02:00
* Prevent 500 on parallel object creation While fixing #3899, we found further possible occurrences of 500 errors in severel _create actions when doing parallel requests. In this PR we want to address them by adding an around_save to the corresponding model in which we catch Sequel::UniqueConstraintViolation errors for the case when ne POST bypasses the other one and inserts the record into the db after the first POST has passed the validate function but before the actual insert.
diff --git a/app/actions/service_credential_binding_app_create.rb b/app/actions/service_credential_binding_app_create.rb
@@ -40,7 +40,13 @@ def precursor(service_instance, message:, app: nil, volume_mount_services_enable
             MetadataUpdate.update(b, message)
           end
         end
-      rescue Sequel::ValidationFailed, Sequel::UniqueConstraintViolation => e
+      rescue Sequel::ValidationFailed => e
+        if e.message.include?('app_guid and name unique')
+          raise UnprocessableCreate.new("The binding name is invalid. App binding names must be unique. The app already has a binding with name '#{message.name}'.")
+        elsif e.message.include?('service_instance_guid and app_guid unique')
+          raise UnprocessableCreate.new('The app is already bound to the service instance.')
+        end
+
         raise UnprocessableCreate.new(e.full_message)
       end
 
diff --git a/app/actions/space_create.rb b/app/actions/space_create.rb
@@ -25,7 +25,7 @@ def create(org, message)
     attr_reader :user_audit_info
 
     def validation_error!(error)
-      error!('Name must be unique per organization') if error.is_a?(Space::DBNameUniqueRaceError) || error.errors.on(%i[organization_id name])&.include?(:unique)
+      error!('Name must be unique per organization') if error.errors.on(%i[organization_id name])&.include?(:unique)
       error!(error.message)
     end
 
diff --git a/app/actions/space_update.rb b/app/actions/space_update.rb
@@ -23,7 +23,7 @@ def update(space, message)
     end
 
     def validation_error!(error, space)
-      error!("Organization '#{space.organization.name}' already contains a space with name '#{space.name}'.") if error.is_a?(Space::DBNameUniqueRaceError) || error.errors.on(%i[
+      error!("Organization '#{space.organization.name}' already contains a space with name '#{space.name}'.") if error.errors.on(%i[
         organization_id name
       ])&.include?(:unique)
       error!(error.message)
diff --git a/app/models/runtime/app_model.rb b/app/models/runtime/app_model.rb
@@ -76,6 +76,15 @@ def after_initialize
       self.enable_ssh = Config.config.get(:default_app_ssh_access) if enable_ssh.nil?
     end
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('apps_v3_space_guid_name_index')
+
+      errors.add(%i[space_guid name], :unique)
+      raise validation_failed_error
+    end
+
     def validate
       super
       validates_presence :name
diff --git a/app/models/runtime/domain.rb b/app/models/runtime/domain.rb
@@ -85,6 +85,15 @@ def shared_or_owned_by(organization_ids)
     add_association_dependencies labels: :destroy
     add_association_dependencies annotations: :destroy
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('domains_name_index')
+
+      errors.add(:name, :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_presence :name
       validates_unique :name, dataset: Domain.dataset
diff --git a/app/models/runtime/helpers/organization_role_mixin.rb b/app/models/runtime/helpers/organization_role_mixin.rb
@@ -13,6 +13,16 @@ def before_create
       self.guid ||= SecureRandom.uuid
     end
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      unique_indexes = %w[org_users_idx org_auditors_idx org_managers_idx org_billing_managers_idx]
+      raise e unless unique_indexes.any? { |pattern| e.message.include?(pattern) }
+
+      errors.add(%i[organization_id user_id], :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_unique %i[organization_id user_id]
       validates_presence :organization_id
diff --git a/app/models/runtime/helpers/space_role_mixin.rb b/app/models/runtime/helpers/space_role_mixin.rb
@@ -13,6 +13,16 @@ def before_create
       self.guid ||= SecureRandom.uuid
     end
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      unique_indexes = %w[space_developers_idx space_auditors_idx space_managers_idx spaces_supporters_user_space_index]
+      raise e unless unique_indexes.any? { |pattern| e.message.include?(pattern) }
+
+      errors.add(%i[space_id user_id], :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_presence :space_id
       validates_presence :user_id
diff --git a/app/models/runtime/isolation_segment_model.rb b/app/models/runtime/isolation_segment_model.rb
@@ -19,6 +19,15 @@ class IsolationSegmentModel < Sequel::Model(:isolation_segments)
     add_association_dependencies labels: :destroy
     add_association_dependencies annotations: :destroy
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('isolation_segment_name_unique_constraint')
+
+      errors.add(:name, :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_format ISOLATION_SEGMENT_MODEL_REGEX, :name, message: Sequel.lit('Isolation Segment names can only contain non-blank unicode characters')
 
diff --git a/app/models/runtime/organization.rb b/app/models/runtime/organization.rb
@@ -207,6 +207,15 @@ def before_save
       super
     end
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('organizations_name_index')
+
+      errors.add(:name, :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_presence :name
       validates_unique :name
diff --git a/app/models/runtime/space.rb b/app/models/runtime/space.rb
@@ -9,6 +9,7 @@ class InvalidSupporterRelation < CloudController::Errors::InvalidRelation; end
     class InvalidManagerRelation < CloudController::Errors::InvalidRelation; end
     class InvalidSpaceQuotaRelation < CloudController::Errors::InvalidRelation; end
     class UnauthorizedAccessToPrivateDomain < RuntimeError; end
+    # needed for v2 spaces_controller
     class DBNameUniqueRaceError < Sequel::ValidationFailed; end
 
     SPACE_NAME_REGEX = /\A[[:alnum:][:punct:][:print:]]+\Z/
@@ -218,9 +219,10 @@ def in_organization?(user)
     def around_save
       yield
     rescue Sequel::UniqueConstraintViolation => e
-      raise DBNameUniqueRaceError.new(e) if e.message.try(:include?, 'spaces_org_id_name_index')
+      raise e unless e.message.include?('spaces_org_id_name_index')
 
-      raise
+      errors.add(%i[organization_id name], :unique)
+      raise validation_failed_error
     end
 
     def validate
diff --git a/app/models/runtime/space_quota_definition.rb b/app/models/runtime/space_quota_definition.rb
@@ -25,6 +25,15 @@ class SpaceQuotaDefinition < Sequel::Model
 
     add_association_dependencies spaces: :nullify
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('sqd_org_id_index')
+
+      errors.add(%i[organization_id name], :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_presence :name
       validates_presence :non_basic_services_allowed
diff --git a/app/models/runtime/stack.rb b/app/models/runtime/stack.rb
@@ -30,6 +30,15 @@ class AppsStillPresentError < StandardError
 
     strip_attributes :name
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('stacks_name_index')
+
+      errors.add(:name, :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_presence :name
       validates_unique :name
diff --git a/app/models/runtime/user.rb b/app/models/runtime/user.rb
@@ -76,6 +76,15 @@ class InvalidOrganizationRelation < CloudController::Errors::InvalidRelation
                       :audited_space_guids,
                       :default_space_guid
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('users_guid_index')
+
+      errors.add(:guid, :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_presence :guid
       validates_unique :guid
diff --git a/app/models/services/service_binding.rb b/app/models/services/service_binding.rb
@@ -35,6 +35,20 @@ class ServiceBinding < Sequel::Model
 
     delegate :service, :service_plan, to: :service_instance
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      if e.message.include?('unique_service_binding_app_guid_name')
+        errors.add(%i[app_guid name], :unique)
+        raise validation_failed_error
+      elsif e.message.include?('unique_service_binding_service_instance_guid_app_guid')
+        errors.add(%i[service_instance_guid app_guid], :unique)
+        raise validation_failed_error
+      else
+        raise e
+      end
+    end
+
     def validate
       validates_presence :app
       validates_presence :service_instance
diff --git a/app/models/services/service_broker.rb b/app/models/services/service_broker.rb
@@ -20,6 +20,15 @@ class ServiceBroker < Sequel::Model
 
     set_field_as_encrypted :auth_password
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('service_brokers_name_index')
+
+      errors.add(:name, :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_presence :name
       validates_presence :broker_url
diff --git a/app/models/services/service_key.rb b/app/models/services/service_key.rb
@@ -37,6 +37,15 @@ def credhub_reference
 
     delegate :space, to: :service_instance
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('svc_key_name_instance_id_index')
+
+      errors.add(%i[name service_instance_id], :unique)
+      raise validation_failed_error
+    end
+
     def validate
       validates_presence :name
       validates_presence :service_instance
diff --git a/spec/unit/actions/app_create_spec.rb b/spec/unit/actions/app_create_spec.rb
@@ -199,6 +199,23 @@ module VCAP::CloudController
           end.to raise_v3_api_error(:UniquenessError)
         end
       end
+
+      context 'when creating apps concurrently' do
+        it 'ensures one creation is successful and the other fails due to name conflict' do
+          # First request, should succeed
+          expect do
+            app_create.create(message, lifecycle)
+          end.not_to raise_error
+
+          # Mock the validation for the second request to simulate the race condition and trigger a unique constraint violation
+          allow_any_instance_of(AppModel).to receive(:validate).and_return(true)
+
+          # Second request, should fail with correct error
+          expect do
+            app_create.create(message, lifecycle)
+          end.to raise_error(CloudController::Errors::V3::ApiError)
+        end
+      end
     end
   end
 end
diff --git a/spec/unit/actions/domain_create_spec.rb b/spec/unit/actions/domain_create_spec.rb
@@ -33,6 +33,33 @@ module VCAP::CloudController
           end
         end
 
+        context 'when creating domains concurrently' do
+          let(:internal) { true }
+          let(:p_name) { 'perfect.domain-1.example.com' }
+          let(:message) do
+            DomainCreateMessage.new({
+                                      name: p_name,
+                                      internal: internal,
+                                      metadata: metadata
+                                    })
+          end
+
+          it 'ensures one creation is successful and the other fails due to name conflict' do
+            # First request, should succeed
+            expect do
+              subject.create(message:)
+            end.not_to raise_error
+
+            # Mock the validation for the second request to simulate the race condition and trigger a unique constraint violation
+            allow_any_instance_of(Domain).to receive(:validate).and_return(true)
+
+            # Second request, should fail with correct error
+            expect do
+              subject.create(message:)
+            end.to raise_error(DomainCreate::Error, 'The domain name "perfect.domain-1.example.com" is already in use')
+          end
+        end
+
         context 'when the error is a uniqueness error' do
           let(:existing_domain) { SharedDomain.make }
           let(:message) { DomainCreateMessage.new({ name: existing_domain.name }) }
diff --git a/spec/unit/actions/organization_create_spec.rb b/spec/unit/actions/organization_create_spec.rb
@@ -110,6 +110,26 @@ module VCAP::CloudController
             end.to raise_error(OrganizationCreate::Error, "Organization '#{name}' already exists.")
           end
         end
+
+        context 'when creating organizations concurrently' do
+          let(:name) { 'Alfredo' }
+
+          it 'ensures one creation is successful and the other fails due to name conflict' do
+            # First request, should succeed
+            message = VCAP::CloudController::OrganizationUpdateMessage.new(name:)
+            expect do
+              org_create.create(message)
+            end.not_to raise_error
+
+            # Mock the validation for the second request to simulate the race condition and trigger a unique constraint violation
+            allow_any_instance_of(Organization).to receive(:validate).and_return(true)
+
+            # Second request, should fail with correct error
+            expect do
+              org_create.create(message)
+            end.to raise_error(OrganizationCreate::Error, "Organization 'Alfredo' already exists.")
+          end
+        end
       end
     end
   end
diff --git a/spec/unit/actions/role_create_spec.rb b/spec/unit/actions/role_create_spec.rb
@@ -75,6 +75,23 @@ module VCAP::CloudController
               end.to raise_error(RoleCreate::Error, "User '#{user.presentation_name}' already has '#{type}' role in space '#{space.name}'.")
             end
           end
+
+          context 'when creating space roles concurrently' do
+            it 'ensures one creation is successful and the other fails due to name conflict' do
+              # First request, should succeed
+              expect do
+                subject.create_space_role(type:, user:, space:)
+              end.not_to raise_error
+
+              # Mock the validation for the second request to simulate the race condition and trigger a unique constraint violation
+              allow_any_instance_of(SpaceRoleMixin).to receive(:validate).and_return(true)
+
+              # Second request, should fail with correct error
+              expect do
+                subject.create_space_role(type:, user:, space:)
+              end.to raise_error(RoleCreate::Error, "User '#{user.presentation_name}' already has '#{type}' role in space '#{space.name}'.")
+            end
+          end
         end
       end
 
@@ -185,6 +202,23 @@ module VCAP::CloudController
               end.to raise_error(RoleCreate::Error, "User '#{user.presentation_name}' already has '#{type}' role in organization '#{org.name}'.")
             end
           end
+
+          context 'when creating organization roles concurrently' do
+            it 'ensures one creation is successful and the other fails due to name conflict' do
+              # First request, should succeed
+              expect do
+                subject.create_organization_role(type: type, user: user, organization: org)
+              end.not_to raise_error
+
+              # Mock the validation for the second request to simulate the race condition and trigger a unique constraint violation
+              allow_any_instance_of(OrganizationRoleMixin).to receive(:validate).and_return(true)
+
+              # Second request, should fail with correct error
+              expect do
+                subject.create_organization_role(type: type, user: user, organization: org)
+              end.to raise_error(RoleCreate::Error, "User '#{user.presentation_name}' already has '#{type}' role in organization '#{org.name}'.")
+            end
+          end
         end
       end
 
diff --git a/spec/unit/actions/service_broker_create_spec.rb b/spec/unit/actions/service_broker_create_spec.rb
diff --git a/spec/unit/actions/service_credential_binding_app_create_spec.rb b/spec/unit/actions/service_credential_binding_app_create_spec.rb
diff --git a/spec/unit/actions/service_credential_binding_key_create_spec.rb b/spec/unit/actions/service_credential_binding_key_create_spec.rb
diff --git a/spec/unit/actions/space_create_spec.rb b/spec/unit/actions/space_create_spec.rb
diff --git a/spec/unit/actions/space_quotas_create_spec.rb b/spec/unit/actions/space_quotas_create_spec.rb
diff --git a/spec/unit/actions/stack_create_spec.rb b/spec/unit/actions/stack_create_spec.rb
diff --git a/spec/unit/actions/user_create_spec.rb b/spec/unit/actions/user_create_spec.rb
diff --git a/spec/unit/isolation_segment_create_spec.rb b/spec/unit/isolation_segment_create_spec.rb
diff --git a/spec/unit/models/runtime/space_spec.rb b/spec/unit/models/runtime/space_spec.rb
