Merge branch 'fix-trace-archive-cron-worker-race-condition' into 'master'

Check if the existences of archived traces before archive it

Closes #48705

See merge request gitlab-org/gitlab-ce!20297

app/workers/archive_trace_worker.rb

@@ -5,7 +5,7 @@ class ArchiveTraceWorker
   include PipelineBackgroundQueue
 
   def perform(job_id)
-    Ci::Build.find_by(id: job_id).try do |job|
+    Ci::Build.without_archived_trace.find_by(id: job_id).try do |job|
       job.trace.archive!
     end
   end

app/workers/ci/archive_traces_cron_worker.rb

@@ -12,6 +12,7 @@ module Ci
       Ci::Build.finished.with_live_trace.find_each(batch_size: 100) do |build|
         begin
           build.trace.archive!
+        rescue ::Gitlab::Ci::Trace::AlreadyArchivedError
         rescue => e
           failed_archive_counter.increment
           Rails.logger.error "Failed to archive stale live trace. id: #{build.id} message: #{e.message}"

changelogs/unreleased/fix-trace-archive-cron-worker-race-condition.yml

+---
+title: Check if archived trace exist before archive it
+merge_request: 20297
+author:
+type: fixed

lib/gitlab/ci/trace.rb

@@ -6,6 +6,7 @@ module Gitlab
       LEASE_TIMEOUT = 1.hour
 
       ArchiveError = Class.new(StandardError)
+      AlreadyArchivedError = Class.new(StandardError)
 
       attr_reader :job
 
@@ -81,7 +82,9 @@ module Gitlab
 
       def write(mode)
         stream = Gitlab::Ci::Trace::Stream.new do
-          if current_path
+          if trace_artifact
+            raise AlreadyArchivedError, 'Could not write to the archived trace'
+          elsif current_path
             File.open(current_path, mode)
           elsif Feature.enabled?('ci_enable_live_trace')
             Gitlab::Ci::Trace::ChunkedIO.new(job)
@@ -117,7 +120,7 @@ module Gitlab
       private
 
       def unsafe_archive!
-        raise ArchiveError, 'Already archived' if trace_artifact
+        raise AlreadyArchivedError, 'Could not archive again' if trace_artifact
         raise ArchiveError, 'Job is not finished yet' unless job.complete?
 
         if job.trace_chunks.any?

spec/requests/api/runner_spec.rb

@@ -851,6 +851,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
         it 'does not update job status and job trace' do
           update_job(state: 'success', trace: 'BUILD TRACE UPDATED')
 
+          job.reload
           expect(response).to have_gitlab_http_status(403)
           expect(response.header['Job-Status']).to eq 'failed'
           expect(job.trace.raw).to eq 'Job failed'

spec/support/shared_examples/ci_trace_shared_examples.rb

@@ -138,6 +138,28 @@ shared_examples_for 'common trace features' do
     end
   end
 
+  describe '#write' do
+    subject { trace.send(:write, mode) { } }
+
+    let(:mode) { 'wb' }
+
+    context 'when arhicved trace does not exist yet' do
+      it 'does not raise an error' do
+        expect { subject }.not_to raise_error
+      end
+    end
+
+    context 'when arhicved trace already exists' do
+      before do
+        create(:ci_job_artifact, :trace, job: build)
+      end
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(Gitlab::Ci::Trace::AlreadyArchivedError)
+      end
+    end
+  end
+
   describe '#set' do
     before do
       trace.set("12")
@@ -574,7 +596,7 @@ shared_examples_for 'trace with disabled live trace feature' do
 
       it 'does not archive' do
         expect_any_instance_of(described_class).not_to receive(:archive_stream!)
-        expect { subject }.to raise_error('Already archived')
+        expect { subject }.to raise_error(Gitlab::Ci::Trace::AlreadyArchivedError)
         expect(build.job_artifacts_trace.file.exists?).to be_truthy
       end
     end
@@ -761,7 +783,7 @@ shared_examples_for 'trace with enabled live trace feature' do
 
       it 'does not archive' do
         expect_any_instance_of(described_class).not_to receive(:archive_stream!)
-        expect { subject }.to raise_error('Already archived')
+        expect { subject }.to raise_error(Gitlab::Ci::Trace::AlreadyArchivedError)
         expect(build.job_artifacts_trace.file.exists?).to be_truthy
       end
     end

spec/workers/ci/archive_traces_cron_worker_spec.rb

@@ -25,24 +25,36 @@ describe Ci::ArchiveTracesCronWorker do
     end
   end
 
-  context 'when a job was succeeded' do
+  context 'when a job succeeded' do
     let!(:build) { create(:ci_build, :success, :trace_live) }
 
     it_behaves_like 'archives trace'
 
-    context 'when archive raised an exception' do
-      let!(:build) { create(:ci_build, :success, :trace_artifact, :trace_live) }
+    context 'when a trace had already been archived' do
+      let!(:build) { create(:ci_build, :success, :trace_live, :trace_artifact) }
       let!(:build2) { create(:ci_build, :success, :trace_live) }
 
-      it 'archives valid targets' do
-        expect(Rails.logger).to receive(:error).with("Failed to archive stale live trace. id: #{build.id} message: Already archived")
-
+      it 'continues to archive live traces' do
         subject
 
         build2.reload
         expect(build2.job_artifacts_trace).to be_exist
       end
     end
+
+    context 'when an unexpected exception happened during archiving' do
+      let!(:build) { create(:ci_build, :success, :trace_live) }
+
+      before do
+        allow_any_instance_of(Gitlab::Ci::Trace).to receive(:archive!).and_raise('Unexpected error')
+      end
+
+      it 'puts a log' do
+        expect(Rails.logger).to receive(:error).with("Failed to archive stale live trace. id: #{build.id} message: Unexpected error")
+
+        subject
+      end
+    end
   end
 
   context 'when a job was cancelled' do