Fix add_index_in_background to be idempotent

CHANGELOG.md

@@ -1,5 +1,7 @@
 ## master (unreleased)
 
+- Fix `add_index_in_background` to be idempotent
+
 ## 0.19.0 (2024-05-21)
 
 - Add ability to cancel background migrations

lib/online_migrations/background_schema_migrations/migration_helpers.rb

@@ -6,14 +6,24 @@ module MigrationHelpers
       def add_index_in_background(table_name, column_name, **options)
         migration_options = options.extract!(:max_attempts, :statement_timeout, :connection_class_name)
 
+        options[:algorithm] = :concurrently
+        index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
+
+        # Need to check this first, because `index_exists?` does not check for `:where`s.
+        if index_name_exists?(table_name, index.name)
+          Utils.raise_or_say(<<~MSG)
+            Index creation was not enqueued because the index with name '#{index.name}' already exists.
+            This can be due to an aborted migration or you need to explicitly provide another name
+            via `:name` option.
+          MSG
+          return
+        end
+
         if index_exists?(table_name, column_name, **options)
           Utils.raise_or_say("Index creation was not enqueued because the index already exists.")
           return
         end
 
-        options[:algorithm] = :concurrently
-        index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options)
-
         create_index = ActiveRecord::ConnectionAdapters::CreateIndexDefinition.new(index, algorithm, if_not_exists)
         schema_creation = ActiveRecord::ConnectionAdapters::PostgreSQL::SchemaCreation.new(self)
         definition = schema_creation.accept(create_index)
@@ -77,22 +87,21 @@ def enqueue_background_schema_migration(name, table_name, **options)
       # @private
       def create_background_schema_migration(migration_name, table_name, **options)
         options.assert_valid_keys(:definition, :max_attempts, :statement_timeout, :connection_class_name)
-        migration = Migration.new(migration_name: migration_name, table_name: table_name, **options)
-
-        shards = Utils.shard_names(migration.connection_class)
-        if shards.size > 1
-          migration.children = shards.map do |shard|
-            child = migration.dup
-            child.shard = shard
-            child
-          end
 
-          migration.composite = true
-        end
+        Migration.find_or_create_by!(migration_name: migration_name, shard: nil) do |migration|
+          migration.assign_attributes(**options, table_name: table_name)
 
-        # This will save all the records using a transaction.
-        migration.save!
-        migration
+          shards = Utils.shard_names(migration.connection_class)
+          if shards.size > 1
+            migration.children = shards.map do |shard|
+              child = migration.dup
+              child.shard = shard
+              child
+            end
+
+            migration.composite = true
+          end
+        end
       end
     end
   end

lib/online_migrations/background_schema_migrations/migration_runner.rb

@@ -11,7 +11,7 @@ def initialize(migration)
       end
 
       def run
-        return if migration.cancelled?
+        return if migration.cancelled? || migration.succeeded?
 
         mark_as_running if migration.enqueued? || migration.failed?
 

test/schema_statements/indexes_test.rb

@@ -101,11 +101,34 @@ def test_add_index_in_background
       assert_equal 'CREATE UNIQUE INDEX CONCURRENTLY "index_users_on_name" ON "users" ("name")', m.definition
     end
 
-    def test_add_index_in_background_when_index_already_exists
+    def test_add_index_in_background_is_idempotent
+      # Emulate created, but not yet executed, schema migration.
+      OnlineMigrations.config.stub(:run_background_migrations_inline, -> { false }) do
+        @connection.add_index_in_background(:users, :name, connection_class_name: "User")
+      end
+
+      @connection.add_index_in_background(:users, :name, connection_class_name: "User")
+      assert_equal 1, OnlineMigrations::BackgroundSchemaMigrations::Migration.count
+    end
+
+    def test_add_index_in_background_when_different_index_with_same_name_already_exists
       @connection.add_index(:users, :name, unique: true)
-      assert_raises_with_message(RuntimeError, /Index creation was not enqueued/i) do
-        OnlineMigrations::Utils.stub(:multiple_databases?, false) do
-          @connection.add_index_in_background(:users, :name, unique: true, connection_class_name: "User")
+
+      OnlineMigrations::Utils.stub(:multiple_databases?, false) do
+        assert_raises_with_message(RuntimeError, /index with name 'index_users_on_name' already exists/i) do
+          @connection.add_index_in_background(:users, :name, unique: true, where: "company_id", connection_class_name: "User")
+        end
+      end
+      assert @connection.index_exists?(:users, :name)
+      assert_equal 0, OnlineMigrations::BackgroundSchemaMigrations::Migration.count
+    end
+
+    def test_add_index_in_background_when_unfinished_migration_exists
+      @connection.add_index(:users, :name, unique: true)
+
+      OnlineMigrations::Utils.stub(:multiple_databases?, false) do
+        assert_raises_with_message(RuntimeError, /index with name 'index_users_on_name' already exists/i) do
+          @connection.add_index_in_background(:users, :name, unique: true, where: "company_id", connection_class_name: "User")
         end
       end
       assert @connection.index_exists?(:users, :name)