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

3 files changed, 43 insertions, 8 deletions

diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index e7639599874..d57993ab454 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/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'
diff --git a/spec/support/shared_examples/ci_trace_shared_examples.rb b/spec/support/shared_examples/ci_trace_shared_examples.rb
index db723a323f8..8a91e05b2d0 100644
--- a/spec/support/shared_examples/ci_trace_shared_examples.rb
+++ b/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
diff --git a/spec/workers/ci/archive_traces_cron_worker_spec.rb b/spec/workers/ci/archive_traces_cron_worker_spec.rb
index 9af51b7d4d8..23f5dda298a 100644
--- a/spec/workers/ci/archive_traces_cron_worker_spec.rb
+++ b/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