diff --git a/app/services/project_identifiers/convert_project_to_semantic_service.rb b/app/services/project_identifiers/convert_project_to_semantic_service.rb index e0f41697f92..b4a311868de 100644 --- a/app/services/project_identifiers/convert_project_to_semantic_service.rb +++ b/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 diff --git a/app/services/project_identifiers/revert_project_to_classic_service.rb b/app/services/project_identifiers/revert_project_to_classic_service.rb index da3a37a89b2..7b45ce6da6b 100644 --- a/app/services/project_identifiers/revert_project_to_classic_service.rb +++ b/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 diff --git a/app/workers/project_identifiers/convert_project_to_semantic_ids_job.rb b/app/workers/project_identifiers/convert_project_to_semantic_ids_job.rb index 451622e9065..9c8432e89d7 100644 --- a/app/workers/project_identifiers/convert_project_to_semantic_ids_job.rb +++ b/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) diff --git a/app/workers/project_identifiers/revert_instance_to_classic_ids_job.rb b/app/workers/project_identifiers/revert_instance_to_classic_ids_job.rb index 18f5e5c9459..6246b50a0a7 100644 --- a/app/workers/project_identifiers/revert_instance_to_classic_ids_job.rb +++ b/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? diff --git a/spec/services/project_identifiers/convert_project_to_semantic_service_spec.rb b/spec/services/project_identifiers/convert_project_to_semantic_service_spec.rb index ca41c09ae26..ffbef70ac55 100644 --- a/spec/services/project_identifiers/convert_project_to_semantic_service_spec.rb +++ b/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| diff --git a/spec/services/project_identifiers/revert_project_to_classic_service_spec.rb b/spec/services/project_identifiers/revert_project_to_classic_service_spec.rb index 8ab8640f559..5eca4d502b3 100644 --- a/spec/services/project_identifiers/revert_project_to_classic_service_spec.rb +++ b/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") }