fix: Stability fixes for the identifier conversion procedure

app/services/project_identifiers/convert_project_to_semantic_service.rb

@@ -70,10 +70,22 @@ module ProjectIdentifiers
 
       raise "Generated identifier is blank for project #{project.id}" if new_identifier.blank?
 
+      save_identifier!(new_identifier)
+    rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique => e
+      handle_identifier_conflict(new_identifier, e)
+    end
+
+    def handle_identifier_conflict(attempted_id, error)
+      Rails.logger.warn "#{self.class}: Could not set identifier '#{attempted_id}' for project #{project.id}; " \
+                        "falling back to a random identifier. (#{error.message})"
+      save_identifier!("P#{SecureRandom.alphanumeric(5).upcase}")
+    end
+
+    def save_identifier!(new_identifier)
       project.identifier = new_identifier
-      # Save with the validation context that allows to save semantic ID while system is in classic mode.
       # Suppress notifications: this is a background system operation, not a user edit.
       Journal::NotificationConfiguration.with(false) do
+        # Uses :semantic_conversion context to allow saving a semantic ID while the system is in classic mode.
         project.save!(context: :semantic_conversion)
       end
     end

app/services/project_identifiers/revert_project_to_classic_service.rb

@@ -53,18 +53,22 @@ module ProjectIdentifiers
     def restore_classic_identifier
       classic_id = identifier_generator.restore_identifier(project) ||
                    identifier_generator.suggest_identifier(project.name)
-      # Suppress notifications: this is a background system operation, not a user edit.
-      Journal::NotificationConfiguration.with(false) do
-        project.update!(identifier: classic_id)
-      rescue ActiveRecord::RecordInvalid => e
-        handle_update_failure(classic_id, e)
-      end
+      save_identifier!(classic_id)
+    rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique => e
+      handle_update_failure(classic_id, e)
     end
 
     def handle_update_failure(classic_id, error)
       Rails.logger.warn "#{self.class}: Could not set identifier '#{classic_id}' for project #{project.id}; " \
-                        "falling back to a randomized suffix. (#{error.message})"
-      project.update!(identifier: "project-#{SecureRandom.alphanumeric(5).downcase}")
+                        "falling back to a random identifier. (#{error.message})"
+      save_identifier!("project-#{SecureRandom.alphanumeric(5).downcase}")
+    end
+
+    def save_identifier!(identifier)
+      # Suppress notifications: this is a background system operation, not a user edit.
+      Journal::NotificationConfiguration.with(false) do
+        project.update!(identifier:)
+      end
     end
 
     def identifier_generator

app/workers/project_identifiers/convert_project_to_semantic_ids_job.rb

@@ -30,7 +30,9 @@
 
 class ProjectIdentifiers::ConvertProjectToSemanticIdsJob < ApplicationJob
   queue_with_priority :above_normal
-  retry_on StandardError, wait: :polynomially_longer, attempts: 8
+  # Cap at ~16 min of cumulative backoff; this is deemed enough for transient infra issues, and longer wai
+  # increases customer pain due to stuck conversion in the UI
+  retry_on StandardError, wait: :polynomially_longer, attempts: 5
   discard_on ActiveRecord::RecordNotFound
 
   def perform(project_id)

app/workers/project_identifiers/revert_instance_to_classic_ids_job.rb

@@ -38,7 +38,9 @@ class ProjectIdentifiers::RevertInstanceToClassicIdsJob < ApplicationJob
   include GoodJob::ActiveJobExtensions::Concurrency
 
   good_job_control_concurrency_with(total_limit: 1)
-  retry_on StandardError, wait: :polynomially_longer, attempts: 8
+  # Cap at ~16 min of cumulative backoff; this is deemed enough for transient infra issues, and longer wait
+  # increases customer pain due to stuck conversion in the UI
+  retry_on StandardError, wait: :polynomially_longer, attempts: 5
 
   def perform
     raise "expected Setting.work_packages_identifier to be classic" unless Setting::WorkPackageIdentifier.classic?

spec/services/project_identifiers/convert_project_to_semantic_service_spec.rb

@@ -67,6 +67,40 @@ RSpec.describe ProjectIdentifiers::ConvertProjectToSemanticService,
       end
     end
 
+    context "when saving the generated identifier fails due to a race condition" do
+      let!(:project) do
+        create(:project).tap { |p| p.update_columns(identifier: "my-app", wp_sequence_counter: 0) }
+      end
+
+      before do
+        allow(project).to receive(:previous_semantic_identifier).and_return("MYAPP")
+        raised = false
+        allow(project).to receive(:save!).and_wrap_original do |original, *args, **kwargs|
+          unless raised
+            raised = true
+            raise ActiveRecord::RecordNotUnique, "stubbed"
+          end
+          original.call(*args, **kwargs)
+        end
+      end
+
+      it "does not raise" do
+        expect { described_class.new(project).call }.not_to raise_error
+      end
+
+      it "assigns a random P-NNNNN fallback identifier" do
+        described_class.new(project).call
+        expect(project.reload.identifier).to match(/\AP[A-Z0-9]{5}\z/)
+      end
+
+      it "logs a warning containing the project id and the conflicting identifier" do
+        allow(Rails.logger).to receive(:warn)
+        described_class.new(project).call
+        expect(Rails.logger).to have_received(:warn)
+          .with(a_string_including(project.id.to_s, "MYAPP"))
+      end
+    end
+
     context "when the suggested identifier case-insensitively matches a historical classic identifier" do
       let!(:project_one) do
         create(:project, name: "Project 1", identifier: "project_one").tap do |p|

spec/services/project_identifiers/revert_project_to_classic_service_spec.rb

@@ -110,6 +110,43 @@ RSpec.describe ProjectIdentifiers::RevertProjectToClassicService do
       end
     end
 
+    context "when saving the restored identifier raises a DB-level uniqueness error" do
+      let!(:project) do
+        create(:project).tap do |p|
+          p.update_columns(identifier: "MYAPP", wp_sequence_counter: 0)
+          FriendlyId::Slug.where(sluggable: p).delete_all
+          FriendlyId::Slug.create!(sluggable: p, slug: "my-app")
+        end
+      end
+
+      before do
+        raised = false
+        allow(project).to receive(:update!).and_wrap_original do |original, *args, **kwargs|
+          unless raised
+            raised = true
+            raise ActiveRecord::RecordNotUnique, "stubbed"
+          end
+          original.call(*args, **kwargs)
+        end
+      end
+
+      it "does not raise" do
+        expect { described_class.new(project).call }.not_to raise_error
+      end
+
+      it "assigns a project-NNNNN fallback identifier" do
+        described_class.new(project).call
+        expect(project.reload.identifier).to match(/\Aproject-[a-z0-9]{5}\z/)
+      end
+
+      it "logs a warning containing the project id and the conflicting identifier" do
+        allow(Rails.logger).to receive(:warn)
+        described_class.new(project).call
+        expect(Rails.logger).to have_received(:warn)
+          .with(a_string_including(project.id.to_s, "my-app"))
+      end
+    end
+
     context "when the classic slug from FriendlyId history is already taken by another project" do
       let!(:blocking_project) { create(:project, identifier: "my-app") }