diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 1fb1cf4132c30a0bb788e8701e3cad538c4c086f..7a90dfee8e1e9377d4cf3bdc0b76570f6e80306a 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -1656,12 +1656,6 @@ def cancel_async_on_job_failure end end - private - - def add_message(severity, content) - messages.build(severity: severity, content: content, project_id: project_id) - end - def merge_request_diff_sha return unless merge_request? @@ -1672,6 +1666,12 @@ def merge_request_diff_sha end end + private + + def add_message(severity, content) + messages.build(severity: severity, content: content, project_id: project_id) + end + def push_details strong_memoize(:push_details) do Gitlab::Git::Push.new(project, before_sha, sha, git_ref) diff --git a/app/models/ci/pipeline_creation/requests.rb b/app/models/ci/pipeline_creation/requests.rb index a062a67daa2f388700235a1339b0da0610b6d6d9..5d1418180b9f31488d47a55089d230c62fb3424a 100644 --- a/app/models/ci/pipeline_creation/requests.rb +++ b/app/models/ci/pipeline_creation/requests.rb @@ -97,6 +97,21 @@ def merge_request_key(merge_request) format(MERGE_REQUEST_REDIS_KEY, project_id: merge_request.project_id, mr_id: merge_request.id) end + # Extracts merge request ID from Redis key and returns the MergeRequest object + # + # @param key [String] Redis key in MERGE_REQUEST_REDIS_KEY format + # @return [MergeRequest, nil] The MergeRequest object or nil if not found + def merge_request_from_key(key) + match = key.match(/mrs:\{(?\d+)\}/) + return unless match + + mr_id = match[:mr_id].to_i + + MergeRequest.find(mr_id) + rescue ActiveRecord::RecordNotFound + nil + end + def hset(request, status, pipeline_id: nil, error: nil) Gitlab::Redis::SharedState.with do |redis| redis.multi do |transaction| diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb index 6851924d651280cd1cd6209b46be6a2df86c46f7..d2fa8b9030f9a80dc95e956875688ad5edc6212d 100644 --- a/app/services/merge_requests/create_pipeline_service.rb +++ b/app/services/merge_requests/create_pipeline_service.rb @@ -3,7 +3,10 @@ module MergeRequests class CreatePipelineService < MergeRequests::BaseService def execute(merge_request) - return cannot_create_pipeline_error unless can_create_pipeline_for?(merge_request) + return cannot_create_pipeline_error('no commits to build') if merge_request.has_no_commits? + + duplicate_error = check_duplicate_pipeline(merge_request) + return duplicate_error if duplicate_error create_merge_request_pipeline(merge_request) end @@ -97,12 +100,31 @@ def can_update_source_branch_in_target_project?(merge_request) .can_update_branch?(merge_request.source_branch) end - def cannot_create_pipeline_error - error_message = 'Cannot create a pipeline for this merge request.' + def check_duplicate_pipeline(merge_request) + return if allow_duplicate + + existing_pipeline = merge_request.find_diff_head_pipeline - ::Ci::PipelineCreation::Requests.failed(params[:pipeline_creation_request], error_message) + if existing_pipeline&.merge_request? && existing_pipeline&.merge_request_diff_sha == merge_request.diff_head_sha + if existing_pipeline.running? || existing_pipeline.pending? + return cannot_create_pipeline_error('duplicate pipeline still in progress', retriable: true) + end - ServiceResponse.error(message: error_message, payload: nil) + return cannot_create_pipeline_error('duplicate pipeline') + end + + nil + end + + def cannot_create_pipeline_error(reason, retriable: false) + message = "Cannot create a pipeline for this merge request: #{reason}." + + if retriable + ServiceResponse.error(message: message, payload: nil, reason: :retriable_error) + else + ::Ci::PipelineCreation::Requests.failed(params[:pipeline_creation_request], message) + ServiceResponse.error(message: message, payload: nil) + end end end end diff --git a/app/workers/merge_requests/create_pipeline_worker.rb b/app/workers/merge_requests/create_pipeline_worker.rb index d2d10dedcb278b58eb032df0c324852f539f7198..41abce49ed26e1125c877115d152eb91153ed30b 100644 --- a/app/workers/merge_requests/create_pipeline_worker.rb +++ b/app/workers/merge_requests/create_pipeline_worker.rb @@ -7,6 +7,9 @@ class CreatePipelineWorker data_consistency :sticky sidekiq_options retry: 3 + sidekiq_retry_in do |_count| + 10 + end include PipelineQueue queue_namespace :pipeline_creation @@ -15,6 +18,19 @@ class CreatePipelineWorker worker_resource_boundary :cpu idempotent! + sidekiq_retries_exhausted do |job, _exception| + pipeline_creation_request = job['args'][3]&.dig('pipeline_creation_request') + next unless pipeline_creation_request + + error_message = 'Cannot create a pipeline for this merge request after multiple retries.' + + ::Ci::PipelineCreation::Requests.failed(pipeline_creation_request, error_message) + + merge_request = ::Ci::PipelineCreation::Requests.merge_request_from_key(pipeline_creation_request&.dig('key')) + + GraphqlTriggers.ci_pipeline_creation_requests_updated(merge_request) if merge_request + end + def perform(project_id, user_id, merge_request_id, params = {}) Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/464679') @@ -32,7 +48,7 @@ def perform(project_id, user_id, merge_request_id, params = {}) push_options = params.with_indifferent_access[:push_options] gitaly_context = params.with_indifferent_access[:gitaly_context] - MergeRequests::CreatePipelineService + result = MergeRequests::CreatePipelineService .new( project: project, current_user: user, @@ -44,6 +60,8 @@ def perform(project_id, user_id, merge_request_id, params = {}) } ).execute(merge_request) + raise StandardError, result.message if result&.error? && result.reason == :retriable_error + merge_request.update_head_pipeline after_perform(merge_request) diff --git a/ee/app/services/ee/merge_requests/create_pipeline_service.rb b/ee/app/services/ee/merge_requests/create_pipeline_service.rb index 8d3b3ce1f53af37d61d3f17b6f9122305474978a..e963db3bc3e0e2e5463a1661b8d16661be909ffc 100644 --- a/ee/app/services/ee/merge_requests/create_pipeline_service.rb +++ b/ee/app/services/ee/merge_requests/create_pipeline_service.rb @@ -15,13 +15,28 @@ def execute(merge_request) end def allowed?(merge_request) - (can_create_merged_result_pipeline_for?(merge_request) && user_can_run_pipeline?(merge_request)) || super + ( + merge_request.project.merge_pipelines_enabled? && + can_create_merged_result_pipeline_for?(merge_request) && + user_can_run_pipeline?(merge_request) + ) || super end private def create_merged_result_pipeline_for(merge_request) - return cannot_create_pipeline_error unless can_create_merged_result_pipeline_for?(merge_request) + unless merge_request.project.merge_pipelines_enabled? + return ServiceResponse.error( + message: 'Cannot create a pipeline for this merge request: merge pipelines not enabled' + ) + end + + unless can_create_merged_result_pipeline_for?(merge_request) + return cannot_create_pipeline_error('merged result pipeline not allowed') + end + + duplicate_error = check_duplicate_pipeline(merge_request) + return duplicate_error if duplicate_error result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute(recheck: true) @@ -38,15 +53,16 @@ def create_merged_result_pipeline_for(merge_request) push_options: params[:push_options]) .execute(:merge_request_event, merge_request: merge_request) else - cannot_create_pipeline_error + cannot_create_pipeline_error('mergeability check failed') end end def can_create_merged_result_pipeline_for?(merge_request) - return false unless merge_request.project.merge_pipelines_enabled? return false unless can_create_pipeline_in_target_project?(merge_request) - can_create_pipeline_for?(merge_request) + return false if merge_request.has_no_commits? + + true end def merge_status_race?(merge_request, result) diff --git a/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb b/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb index fdf4894d440ebde96745da94e25441050c998210..bd5a82c6ed9f6b182f75ed3773f0a1474dfb2b91 100644 --- a/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb @@ -204,7 +204,9 @@ it 'responds with error', :aggregate_failures do expect(subject).to be_error - expect(subject.message).to eq('Cannot create a pipeline for this merge request.') + expect(subject.message).to eq( + 'Cannot create a pipeline for this merge request: duplicate pipeline.' + ) end end end @@ -214,6 +216,7 @@ subject(:allowed) { service.allowed?(merge_request) } + let(:merge_pipelines_enabled) { true } let(:user_without_permissions) { create(:user) } where(:merged_result_pipeline_supported, :detached_mr_pipeline_supported, :user_can_run_pipeline, :result) do @@ -240,6 +243,9 @@ .and_return(detached_mr_pipeline_supported) project.add_developer(user) if user_can_run_pipeline + + source_project.merge_pipelines_enabled = merge_pipelines_enabled + stub_licensed_features(merge_pipelines: true) end it 'matches the expected result' do diff --git a/spec/services/merge_requests/create_pipeline_service_spec.rb b/spec/services/merge_requests/create_pipeline_service_spec.rb index 19d71d8a90d11e5bb3f868afa847705e60a60457..485ba9987a75e1c7be2b51b02b210dcb091406f0 100644 --- a/spec/services/merge_requests/create_pipeline_service_spec.rb +++ b/spec/services/merge_requests/create_pipeline_service_spec.rb @@ -173,7 +173,10 @@ context 'when service is called multiple times' do it 'creates a pipeline once' do expect do - service.execute(merge_request) + first_pipeline = service.execute(merge_request) + first_pipeline.payload.update!(status: :running) + + allow(merge_request).to receive(:find_diff_head_pipeline).and_call_original service.execute(merge_request) end.to change { Ci::Pipeline.count }.by(1) end @@ -183,7 +186,10 @@ it 'creates pipelines multiple times' do expect do - service.execute(merge_request) + first_pipeline = service.execute(merge_request) + first_pipeline.payload.update!(status: :running) + + allow(merge_request).to receive(:find_diff_head_pipeline).and_call_original service.execute(merge_request) end.to change { Ci::Pipeline.count }.by(2) end @@ -266,12 +272,41 @@ expect { response }.not_to change { Ci::Pipeline.count } expect(response).to be_error - expect(response.message).to eq('Cannot create a pipeline for this merge request.') + expect(response.message).to eq('Cannot create a pipeline for this merge request: no commits to build.') expect(response.payload).to be_nil failed_creation = ::Ci::PipelineCreation::Requests.hget(request) expect(failed_creation['status']).to eq(::Ci::PipelineCreation::Requests::FAILED) - expect(failed_creation['error']).to eq('Cannot create a pipeline for this merge request.') + expect(failed_creation['error']).to eq('Cannot create a pipeline for this merge request: no commits to build.') + end + end + + context 'when duplicate pipeline is still in progress' do + let(:request) { ::Ci::PipelineCreation::Requests.start_for_merge_request(merge_request) } + let(:params) { { pipeline_creation_request: request } } + + before do + existing_pipeline = create(:ci_pipeline, merge_requests_as_head_pipeline: [merge_request]) + allow(existing_pipeline).to receive_messages( + merge_request?: true, + running_or_pending?: true, + merge_request_diff_sha: merge_request.diff_head_sha + ) + allow(merge_request).to receive_messages(has_no_commits?: false, find_diff_head_pipeline: existing_pipeline) + end + + it 'calls cannot_create_pipeline_error with retriable flag' do + response = service.execute(merge_request) + + expect(response).to be_error + expect(response.message).to include('duplicate pipeline still in progress') + end + + it 'keeps pipeline creation in progress for retry' do + service.execute(merge_request) + + request_data = ::Ci::PipelineCreation::Requests.hget(params[:pipeline_creation_request]) + expect(request_data['status']).to eq(::Ci::PipelineCreation::Requests::IN_PROGRESS) end end @@ -394,4 +429,65 @@ it { is_expected.to be_falsey } end end + + describe '#cannot_create_pipeline_error' do + let(:service) { described_class.new(project: project, current_user: user, params: params) } + let(:params) { { pipeline_creation_request: { 'key' => '123', 'id' => '456' } } } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + context 'when called with retriable_error: false (default)' do + it 'returns an error response' do + response = service.send(:cannot_create_pipeline_error, 'test reason') + + expect(response).to be_error + expect(response.message).to eq('Cannot create a pipeline for this merge request: test reason.') + end + + it 'marks the pipeline creation request as failed' do + service.send(:cannot_create_pipeline_error, 'test reason') + + failed_request = ::Ci::PipelineCreation::Requests.hget(params[:pipeline_creation_request]) + expect(failed_request['status']).to eq(::Ci::PipelineCreation::Requests::FAILED) + expect(failed_request['error']).to eq('Cannot create a pipeline for this merge request: test reason.') + end + + it 'includes the reason in the error message' do + response = service.send(:cannot_create_pipeline_error, 'custom failure reason') + + expect(response.message).to include('custom failure reason') + end + end + end + + describe '#cannot_create_pipeline_error with retriable_error flag' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:request) { ::Ci::PipelineCreation::Requests.start_for_merge_request(merge_request) } + let(:service) { described_class.new(project: project, current_user: user, params: params) } + let(:params) { { pipeline_creation_request: request } } + + context 'when called with retriable: true' do + it 'returns an error response' do + response = service.send(:cannot_create_pipeline_error, 'test reason', retriable: true) + + expect(response).to be_error + expect(response.message).to eq('Cannot create a pipeline for this merge request: test reason.') + end + + it 'does NOT mark the pipeline creation request as failed' do + service.send(:cannot_create_pipeline_error, 'test reason', retriable: true) + + # The request should still be in progress, not marked as failed + request_data = ::Ci::PipelineCreation::Requests.hget(params[:pipeline_creation_request]) + expect(request_data['status']).to eq(::Ci::PipelineCreation::Requests::IN_PROGRESS) + expect(request_data['error']).to be_nil + end + + it 'allows the worker to retry the job' do + response = service.send(:cannot_create_pipeline_error, 'duplicate pipeline still in progress', retriable: true) + + expect(response).to be_error + expect(response.reason).to eq(:retriable_error) + end + end + end end diff --git a/spec/workers/merge_requests/create_pipeline_worker_spec.rb b/spec/workers/merge_requests/create_pipeline_worker_spec.rb index 5752fe987599a87e3ff998993c8160b4f1ec4898..0f77ca9467ea9ba31f611e1743cf8003df64555c 100644 --- a/spec/workers/merge_requests/create_pipeline_worker_spec.rb +++ b/spec/workers/merge_requests/create_pipeline_worker_spec.rb @@ -104,4 +104,39 @@ it_behaves_like 'when object does not exist' end end + + describe 'retry behavior' do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:merge_request) { create(:merge_request) } + let(:worker) { described_class.new } + let(:pipeline_creation_request) { Ci::PipelineCreation::Requests.start_for_merge_request(merge_request) } + let(:params) do + { + 'pipeline_creation_request' => pipeline_creation_request, + 'gitaly_context' => {} + } + end + + subject { worker.perform(project.id, user.id, merge_request.id, params) } + + context 'when service raises a retriable error' do + before do + allow_next_instance_of(MergeRequests::CreatePipelineService) do |service| + allow(service).to receive(:execute).and_raise(StandardError, 'Temporary failure') + end + end + + it 'raises the error to trigger Sidekiq retry' do + expect { subject }.to raise_error(StandardError, 'Temporary failure') + end + + it 'keeps status as IN_PROGRESS during retries', :clean_gitlab_redis_shared_state do + expect { subject }.to raise_error(StandardError) + + result = Ci::PipelineCreation::Requests.hget(pipeline_creation_request) + expect(result['status']).to eq('in_progress') + end + end + end end