Add unique constraint for sidecar process types (#4632)

Adds a database-level unique constraint to the `sidecar_process_types`
and `revision_sidecar_process_types` tables on the `(sidecar_guid, type)`
and `(revision_sidecar_guid, type)` columns, respectively.

This prevents duplicate process type entries for a given sidecar, which
could previously occur under concurrent requests.

The migration handles de-duplicating any existing records before applying
the new unique index. Additionally, the corresponding models have been
updated to gracefully handle the `UniqueConstraintViolation` and raise a
user-friendly validation error. The previous application-level validates_unique check, which was susceptible to race conditions, has been replaced.

Author: johha

app/models/runtime/revision_sidecar_process_type_model.rb

@@ -6,11 +6,19 @@ class RevisionSidecarProcessTypeModel < Sequel::Model(:revision_sidecar_process_
                 key: :revision_sidecar_guid,
                 without_guid_generation: true
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('revision_sidecar_process_types_revision_sidecar_guid_type_index')
+
+      errors.add(%i[revision_sidecar_guid type], "Sidecar is already associated with process type #{type}")
+      raise Sequel::ValidationFailed.new(self)
+    end
+
     def validate
       super
       validates_presence [:type]
       validates_max_length 255, :type, message: Sequel.lit('Process type is too long (maximum is 255 characters)')
-      validates_unique %i[revision_sidecar_guid type], message: Sequel.lit("Sidecar is already associated with process type #{type}")
     end
   end
 end

app/models/runtime/sidecar_process_type_model.rb

@@ -6,6 +6,15 @@ class SidecarProcessTypeModel < Sequel::Model(:sidecar_process_types)
                 key: :sidecar_guid,
                 without_guid_generation: true
 
+    def around_save
+      yield
+    rescue Sequel::UniqueConstraintViolation => e
+      raise e unless e.message.include?('sidecar_process_types_sidecar_guid_type_index')
+
+      errors.add(%i[sidecar_guid type], "Sidecar is already associated with process type #{type}")
+      raise Sequel::ValidationFailed.new(self)
+    end
+
     def validate
       super
       validates_presence [:type]

db/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types.rb

@@ -0,0 +1,68 @@
+Sequel.migration do
+  up do
+    transaction do
+      # Clean up sidecar_process_types
+      duplicates = self[:sidecar_process_types].
+                   select(:sidecar_guid, :type).
+                   group(:sidecar_guid, :type).
+                   having { count(1) > 1 }
+
+      duplicates.each do |dup|
+        pks_to_remove = self[:sidecar_process_types].
+                        where(sidecar_guid: dup[:sidecar_guid], type: dup[:type]).
+                        select(:id).
+                        order(:id).
+                        offset(1).
+                        map(:id)
+
+        self[:sidecar_process_types].where(id: pks_to_remove).delete
+      end
+
+      alter_table(:sidecar_process_types) do
+        unless @db.indexes(:sidecar_process_types).key?(:sidecar_process_types_sidecar_guid_type_index)
+          add_unique_constraint %i[sidecar_guid type],
+                                name: :sidecar_process_types_sidecar_guid_type_index
+        end
+      end
+    end
+
+    transaction do
+      # Clean up revision_sidecar_process_types
+      duplicates = self[:revision_sidecar_process_types].
+                   select(:revision_sidecar_guid, :type).
+                   group(:revision_sidecar_guid, :type).
+                   having { count(1) > 1 }
+
+      duplicates.each do |dup|
+        pks_to_remove = self[:revision_sidecar_process_types].
+                        where(revision_sidecar_guid: dup[:revision_sidecar_guid], type: dup[:type]).
+                        select(:id).
+                        order(:id).
+                        offset(1).
+                        map(:id)
+
+        self[:revision_sidecar_process_types].where(id: pks_to_remove).delete
+      end
+
+      alter_table(:revision_sidecar_process_types) do
+        unless @db.indexes(:revision_sidecar_process_types).key?(:revision_sidecar_process_types_revision_sidecar_guid_type_index)
+          add_unique_constraint %i[revision_sidecar_guid type],
+                                name: :revision_sidecar_process_types_revision_sidecar_guid_type_index
+        end
+      end
+    end
+  end
+
+  down do
+    alter_table(:sidecar_process_types) do
+      drop_constraint(:sidecar_process_types_sidecar_guid_type_index, type: :unique) if @db.indexes(:sidecar_process_types).key?(:sidecar_process_types_sidecar_guid_type_index)
+    end
+
+    alter_table(:revision_sidecar_process_types) do
+      if @db.indexes(:revision_sidecar_process_types).key?(:revision_sidecar_process_types_revision_sidecar_guid_type_index)
+        drop_constraint(:revision_sidecar_process_types_revision_sidecar_guid_type_index,
+                        type: :unique)
+      end
+    end
+  end
+end

spec/migrations/20251030100000_add_unique_constraint_to_sidecar_process_types_spec.rb

@@ -0,0 +1,61 @@
+require 'spec_helper'
+require 'migrations/helpers/migration_shared_context'
+
+RSpec.describe 'add unique constraint to sidecar process types', isolation: :truncation, type: :migration do
+  include_context 'migration' do
+    let(:migration_filename) { '20251030100000_add_unique_constraint_to_sidecar_process_types.rb' }
+  end
+
+  let!(:app) { VCAP::CloudController::AppModel.make }
+  let!(:sidecar) { VCAP::CloudController::SidecarModel.make(app:) }
+  let!(:revision) { VCAP::CloudController::RevisionModel.make(app:) }
+  let!(:revision_sidecar) { VCAP::CloudController::RevisionSidecarModel.make(revision:) }
+
+  it 'removes duplicates, adds unique constraints, and is reversible' do
+    # =========================================================================================
+    # SETUP: Create duplicate entries for both tables to test the de-duplication logic.
+    # =========================================================================================
+    db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid)
+    db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid)
+    expect(db[:sidecar_process_types].where(sidecar_guid: sidecar.guid, type: 'web').count).to eq(2)
+
+    db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid)
+    db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid)
+    expect(db[:revision_sidecar_process_types].where(revision_sidecar_guid: revision_sidecar.guid, type: 'worker').count).to eq(2)
+
+    # =========================================================================================
+    # UP MIGRATION: Run the migration to apply the unique constraints.
+    # =========================================================================================
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
+
+    # =========================================================================================
+    # ASSERT UP MIGRATION: Verify that duplicates are removed and constraints are enforced.
+    # =========================================================================================
+    expect(db[:sidecar_process_types].where(sidecar_guid: sidecar.guid, type: 'web').count).to eq(1)
+    expect(db.indexes(:sidecar_process_types)).to include(:sidecar_process_types_sidecar_guid_type_index)
+    expect { db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) }.to raise_error(Sequel::UniqueConstraintViolation)
+
+    expect(db[:revision_sidecar_process_types].where(revision_sidecar_guid: revision_sidecar.guid, type: 'worker').count).to eq(1)
+    expect(db.indexes(:revision_sidecar_process_types)).to include(:revision_sidecar_process_types_revision_sidecar_guid_type_index)
+    expect { db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) }.to raise_error(Sequel::UniqueConstraintViolation)
+
+    # =========================================================================================
+    # TEST IDEMPOTENCY: Running the migration again should not cause any errors.
+    # =========================================================================================
+    expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
+
+    # =========================================================================================
+    # DOWN MIGRATION: Roll back the migration to remove the constraints.
+    # =========================================================================================
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true)
+
+    # =========================================================================================
+    # ASSERT DOWN MIGRATION: Verify that constraints are removed and duplicates can be re-inserted.
+    # =========================================================================================
+    expect(db.indexes(:sidecar_process_types)).not_to include(:sidecar_process_types_sidecar_guid_type_index)
+    expect { db[:sidecar_process_types].insert(sidecar_guid: sidecar.guid, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid) }.not_to raise_error
+
+    expect(db.indexes(:revision_sidecar_process_types)).not_to include(:revision_sidecar_process_types_revision_sidecar_guid_type_index)
+    expect { db[:revision_sidecar_process_types].insert(revision_sidecar_guid: revision_sidecar.guid, type: 'worker', guid: SecureRandom.uuid) }.not_to raise_error
+  end
+end

spec/unit/models/runtime/revision_sidecar_model_spec.rb

@@ -24,5 +24,24 @@ module VCAP::CloudController
                                                })
       end
     end
+
+    describe 'revision_sidecar_process_types: #around_save' do
+      it 'raises validation error on unique constraint violation for sidecar_process_types' do
+        expect do
+          expect(RevisionSidecarProcessTypeModel.where(revision_sidecar: revision_sidecar, type: 'web').count).to eq(1)
+          RevisionSidecarProcessTypeModel.create(revision_sidecar: revision_sidecar, type: 'web', guid: SecureRandom.uuid)
+        end.to raise_error(Sequel::ValidationFailed) { |error|
+          expect(error.message).to include('Sidecar is already associated with process type web')
+        }
+      end
+
+      it 'raises original error on other unique constraint violations' do
+        expect do
+          expect(RevisionSidecarProcessTypeModel.where(revision_sidecar: revision_sidecar, type: 'web').count).to eq(1)
+          RevisionSidecarProcessTypeModel.create(revision_sidecar: revision_sidecar, type: 'worker',
+                                                 guid: RevisionSidecarProcessTypeModel.where(revision_sidecar: revision_sidecar, type: 'web').first.guid)
+        end.to raise_error(Sequel::UniqueConstraintViolation)
+      end
+    end
   end
 end

spec/unit/models/runtime/sidecar_model_spec.rb

@@ -36,5 +36,28 @@ module VCAP::CloudController
                                       })
       end
     end
+
+    describe 'sidecar_process_types: #around_save' do
+      let(:sidecar) { SidecarModel.make }
+      let(:app) { AppModel.make }
+
+      it 'raises validation error on unique constraint violation for sidecar_process_types' do
+        SidecarProcessTypeModel.create(sidecar: sidecar, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid)
+
+        expect do
+          SidecarProcessTypeModel.create(sidecar: sidecar, type: 'web', app_guid: app.guid, guid: SecureRandom.uuid)
+        end.to raise_error(Sequel::ValidationFailed) { |error|
+          expect(error.message).to include('Sidecar is already associated with process type web')
+        }
+      end
+
+      it 'raises original error on other unique constraint violations' do
+        same_guid = SecureRandom.uuid
+        SidecarProcessTypeModel.create(sidecar: sidecar, type: 'web', app_guid: app.guid, guid: same_guid)
+        expect do
+          SidecarProcessTypeModel.create(sidecar: sidecar, type: 'worker', app_guid: app.guid, guid: same_guid)
+        end.to raise_error(Sequel::UniqueConstraintViolation)
+      end
+    end
   end
 end