diff --git a/app/workers/stuck_import_jobs_worker.rb b/app/workers/stuck_import_jobs_worker.rb index e0e6d1418de..fbb14efc525 100644 --- a/app/workers/stuck_import_jobs_worker.rb +++ b/app/workers/stuck_import_jobs_worker.rb @@ -16,43 +16,41 @@ class StuckImportJobsWorker private def mark_projects_without_jid_as_failed! - started_projects_without_jid.each do |project| + enqueued_projects_without_jid.each do |project| project.mark_import_as_failed(error_message) end.count end def mark_projects_with_jid_as_failed! - completed_jids_count = 0 + jids_and_ids = enqueued_projects_with_jid.pluck(:import_jid, :id).to_h - started_projects_with_jid.find_in_batches(batch_size: 500) do |group| - jids = group.map(&:import_jid) + # Find the jobs that aren't currently running or that exceeded the threshold. + completed_jids = Gitlab::SidekiqStatus.completed_jids(jids_and_ids.keys) + return unless completed_jids.any? - # Find the jobs that aren't currently running or that exceeded the threshold. - completed_jids = Gitlab::SidekiqStatus.completed_jids(jids).to_set + completed_project_ids = jids_and_ids.values_at(*completed_jids) - if completed_jids.any? - completed_jids_count += completed_jids.count - group.each do |project| - project.mark_import_as_failed(error_message) if completed_jids.include?(project.import_jid) - end + # We select the projects again, because they may have transitioned from + # scheduled/started to finished/failed while we were looking up their Sidekiq status. + completed_projects = enqueued_projects_with_jid.where(id: completed_project_ids) - Rails.logger.info("Marked stuck import jobs as failed. JIDs: #{completed_jids.to_a.join(', ')}") - end - end + Rails.logger.info("Marked stuck import jobs as failed. JIDs: #{completed_projects.map(&:import_jid).join(', ')}") - completed_jids_count + completed_projects.each do |project| + project.mark_import_as_failed(error_message) + end.count end - def started_projects - Project.with_import_status(:started) + def enqueued_projects + Project.with_import_status(:scheduled, :started) end - def started_projects_with_jid - started_projects.where.not(import_jid: nil) + def enqueued_projects_with_jid + enqueued_projects.where.not(import_jid: nil) end - def started_projects_without_jid - started_projects.where(import_jid: nil) + def enqueued_projects_without_jid + enqueued_projects.where(import_jid: nil) end def error_message diff --git a/changelogs/unreleased/dm-stuck-import-jobs-verify.yml b/changelogs/unreleased/dm-stuck-import-jobs-verify.yml new file mode 100644 index 00000000000..ed2c2d30f0d --- /dev/null +++ b/changelogs/unreleased/dm-stuck-import-jobs-verify.yml @@ -0,0 +1,5 @@ +--- +title: Verify project import status again before marking as failed +merge_request: +author: +type: fixed diff --git a/spec/workers/stuck_import_jobs_worker_spec.rb b/spec/workers/stuck_import_jobs_worker_spec.rb index a82eb54ffe4..069514552b1 100644 --- a/spec/workers/stuck_import_jobs_worker_spec.rb +++ b/spec/workers/stuck_import_jobs_worker_spec.rb @@ -2,35 +2,59 @@ require 'spec_helper' describe StuckImportJobsWorker do let(:worker) { described_class.new } - let(:exclusive_lease_uuid) { SecureRandom.uuid } - before do - allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(exclusive_lease_uuid) - end + shared_examples 'project import job detection' do + context 'when the job has completed' do + context 'when the import status was already updated' do + before do + allow(Gitlab::SidekiqStatus).to receive(:completed_jids) do + project.import_start + project.import_finish - describe 'with started import_status' do - let(:project) { create(:project, :import_started, import_jid: '123') } + [project.import_jid] + end + end - describe 'long running import' do - it 'marks the project as failed' do - allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return(['123']) + it 'does not mark the project as failed' do + worker.perform - expect { worker.perform }.to change { project.reload.import_status }.to('failed') - end - end - - describe 'running import' do - it 'does not mark the project as failed' do - allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return([]) - - expect { worker.perform }.not_to change { project.reload.import_status } + expect(project.reload.import_status).to eq('finished') + end end - describe 'import without import_jid' do + context 'when the import status was not updated' do + before do + allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return([project.import_jid]) + end + it 'marks the project as failed' do - expect { worker.perform }.to change { project.reload.import_status }.to('failed') + worker.perform + + expect(project.reload.import_status).to eq('failed') end end end + + context 'when the job is still in Sidekiq' do + before do + allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return([]) + end + + it 'does not mark the project as failed' do + expect { worker.perform }.not_to change { project.reload.import_status } + end + end + end + + describe 'with scheduled import_status' do + it_behaves_like 'project import job detection' do + let(:project) { create(:project, :import_scheduled, import_jid: '123') } + end + end + + describe 'with started import_status' do + it_behaves_like 'project import job detection' do + let(:project) { create(:project, :import_started, import_jid: '123') } + end end end