Validate the existence of archived traces before removing live trace

Often live traces are removed even though the archived trace
doesn't exist. This commit checkes the existence strictly.

8 files changed, 154 insertions, 12 deletions

diff --git a/spec/lib/gitlab/ci/trace/chunked_io_spec.rb b/spec/lib/gitlab/ci/trace/chunked_io_spec.rb
index 546a9e7d0cc..3e5cd81f929 100644
--- a/spec/lib/gitlab/ci/trace/chunked_io_spec.rb
+++ b/spec/lib/gitlab/ci/trace/chunked_io_spec.rb
@@ -442,5 +442,15 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do
 
       expect(Ci::BuildTraceChunk.where(build: build).count).to eq(0)
     end
+
+    context 'when the job does not have archived trace' do
+      it 'leaves a message in sidekiq log' do
+        expect(Sidekiq.logger).to receive(:warn).with(
+          message: 'The job does not have archived trace but going to be destroyed.',
+          job_id: build.id).and_call_original
+
+        subject
+      end
+    end
   end
 end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 78862de0657..c30cb70e1c1 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -692,6 +692,34 @@ describe Ci::Build do
     end
   end
 
+  describe '#has_live_trace?' do
+    subject { build.has_live_trace? }
+
+    let(:build) { create(:ci_build, :trace_live) }
+
+    it { is_expected.to be_truthy }
+
+    context 'when build does not have live trace' do
+      let(:build) { create(:ci_build) }
+
+      it { is_expected.to be_falsy }
+    end
+  end
+
+  describe '#has_archived_trace?' do
+    subject { build.has_archived_trace? }
+
+    let(:build) { create(:ci_build, :trace_artifact) }
+
+    it { is_expected.to be_truthy }
+
+    context 'when build does not have archived trace' do
+      let(:build) { create(:ci_build) }
+
+      it { is_expected.to be_falsy }
+    end
+  end
+
   describe '#has_job_artifacts?' do
     subject { build.has_job_artifacts? }
 
diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb
index 1ba66565e03..1413da231e0 100644
--- a/spec/models/ci/job_artifact_spec.rb
+++ b/spec/models/ci/job_artifact_spec.rb
@@ -70,6 +70,31 @@ describe Ci::JobArtifact do
     end
   end
 
+  describe '.archived_trace_exists_for?' do
+    subject { described_class.archived_trace_exists_for?(job_id) }
+
+    let!(:artifact) { create(:ci_job_artifact, :trace, job: job) }
+    let(:job) { create(:ci_build) }
+
+    context 'when the specified job_id exists' do
+      let(:job_id) { job.id }
+
+      it { is_expected.to be_truthy }
+
+      context 'when the job does have archived trace' do
+        let!(:artifact) { }
+
+        it { is_expected.to be_falsy }
+      end
+    end
+
+    context 'when the specified job_id does not exist' do
+      let(:job_id) { 10000 }
+
+      it { is_expected.to be_falsy }
+    end
+  end
+
   describe 'callbacks' do
     subject { create(:ci_job_artifact, :archive) }
 
diff --git a/spec/services/ci/archive_trace_service_spec.rb b/spec/services/ci/archive_trace_service_spec.rb
index 44a77c29086..454db3d5a48 100644
--- a/spec/services/ci/archive_trace_service_spec.rb
+++ b/spec/services/ci/archive_trace_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Ci::ArchiveTraceService, '#execute' do
-  subject { described_class.new.execute(job) }
+  subject { described_class.new.execute(job, worker_name: ArchiveTraceWorker.name) }
 
   context 'when job is finished' do
     let(:job) { create(:ci_build, :success, :trace_live) }
@@ -25,6 +25,34 @@ describe Ci::ArchiveTraceService, '#execute' do
         expect { subject }.not_to change { Ci::JobArtifact.trace.count }
       end
     end
+
+    context 'when job does not have trace' do
+      let(:job) { create(:ci_build, :success) }
+
+      it 'leaves a warning message in sidekiq log' do
+        expect(Sidekiq.logger).to receive(:warn).with(
+          class: ArchiveTraceWorker.name,
+          message: 'The job does not have live trace but going to be archived.',
+          job_id: job.id)
+
+        subject
+      end
+    end
+
+    context 'when job failed to archive trace but did not raise an exception' do
+      before do
+        allow_any_instance_of(Gitlab::Ci::Trace).to receive(:archive!) {}
+      end
+
+      it 'leaves a warning message in sidekiq log' do
+        expect(Sidekiq.logger).to receive(:warn).with(
+          class: ArchiveTraceWorker.name,
+          message: 'The job does not have archived trace after archiving.',
+          job_id: job.id)
+
+        subject
+      end
+    end
   end
 
   context 'when job is running' do
@@ -37,10 +65,10 @@ describe Ci::ArchiveTraceService, '#execute' do
               issue_url: 'https://gitlab.com/gitlab-org/gitlab-ce/issues/51502',
               extra: { job_id: job.id } ).once
 
-      expect(Rails.logger)
-        .to receive(:error)
-        .with("Failed to archive trace. id: #{job.id} message: Job is not finished yet")
-        .and_call_original
+      expect(Sidekiq.logger).to receive(:warn).with(
+        class: ArchiveTraceWorker.name,
+        message: "Failed to archive trace. message: Job is not finished yet.",
+        job_id: job.id).and_call_original
 
       expect(Gitlab::Metrics)
         .to receive(:counter)
diff --git a/spec/support/shared_examples/ci_trace_shared_examples.rb b/spec/support/shared_examples/ci_trace_shared_examples.rb
index ab0550e2613..68c2b6e10e2 100644
--- a/spec/support/shared_examples/ci_trace_shared_examples.rb
+++ b/spec/support/shared_examples/ci_trace_shared_examples.rb
@@ -720,6 +720,58 @@ shared_examples_for 'trace with enabled live trace feature' do
     end
   end
 
+  describe '#archived_trace_exist?' do
+    subject { trace.archived_trace_exist? }
+
+    context 'when trace does not exist' do
+      it { is_expected.to be_falsy }
+    end
+
+    context 'when archived trace exists' do
+      before do
+        create(:ci_job_artifact, :trace, job: build)
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when live trace exists' do
+      before do
+        Gitlab::Ci::Trace::ChunkedIO.new(build) do |stream|
+          stream.write('abc')
+        end
+      end
+
+      it { is_expected.to be_falsy }
+    end
+  end
+
+  describe '#live_trace_exist?' do
+    subject { trace.live_trace_exist? }
+
+    context 'when trace does not exist' do
+      it { is_expected.to be_falsy }
+    end
+
+    context 'when archived trace exists' do
+      before do
+        create(:ci_job_artifact, :trace, job: build)
+      end
+
+      it { is_expected.to be_falsy }
+    end
+
+    context 'when live trace exists' do
+      before do
+        Gitlab::Ci::Trace::ChunkedIO.new(build) do |stream|
+          stream.write('abc')
+        end
+      end
+
+      it { is_expected.to be_truthy }
+    end
+  end
+
   describe '#archive!' do
     subject { trace.archive! }
 
diff --git a/spec/workers/archive_trace_worker_spec.rb b/spec/workers/archive_trace_worker_spec.rb
index 368ed3f3db1..44f7be15201 100644
--- a/spec/workers/archive_trace_worker_spec.rb
+++ b/spec/workers/archive_trace_worker_spec.rb
@@ -11,7 +11,7 @@ describe ArchiveTraceWorker do
 
       it 'executes service' do
         expect_any_instance_of(Ci::ArchiveTraceService)
-          .to receive(:execute).with(job)
+          .to receive(:execute).with(job, anything)
 
         subject
       end
diff --git a/spec/workers/ci/archive_traces_cron_worker_spec.rb b/spec/workers/ci/archive_traces_cron_worker_spec.rb
index eca6cf5235f..28381fdc3be 100644
--- a/spec/workers/ci/archive_traces_cron_worker_spec.rb
+++ b/spec/workers/ci/archive_traces_cron_worker_spec.rb
@@ -34,7 +34,7 @@ describe Ci::ArchiveTracesCronWorker do
 
     it 'executes service' do
       expect_any_instance_of(Ci::ArchiveTraceService)
-        .to receive(:execute).with(build)
+        .to receive(:execute).with(build, anything)
 
       subject
     end
@@ -60,7 +60,10 @@ describe Ci::ArchiveTracesCronWorker do
       end
 
       it 'puts a log' do
-        expect(Rails.logger).to receive(:error).with("Failed to archive trace. id: #{build.id} message: Unexpected error")
+        expect(Sidekiq.logger).to receive(:warn).with(
+          class: described_class.name,
+          message: "Failed to archive trace. message: Unexpected error.",
+          job_id: build.id)
 
         subject
       end
diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb
index 72de62f1188..c3d577e2dae 100644
--- a/spec/workers/stuck_ci_jobs_worker_spec.rb
+++ b/spec/workers/stuck_ci_jobs_worker_spec.rb
@@ -7,8 +7,6 @@ describe StuckCiJobsWorker do
 
   let!(:runner) { create :ci_runner }
   let!(:job) { create :ci_build, runner: runner }
-  let(:trace_lease_key) { "trace:write:lock:#{job.id}" }
-  let(:trace_lease_uuid) { SecureRandom.uuid }
   let(:worker_lease_key) { StuckCiJobsWorker::EXCLUSIVE_LEASE_KEY }
   let(:worker_lease_uuid) { SecureRandom.uuid }
 
@@ -16,7 +14,6 @@ describe StuckCiJobsWorker do
 
   before do
     stub_exclusive_lease(worker_lease_key, worker_lease_uuid)
-    stub_exclusive_lease(trace_lease_key, trace_lease_uuid)
     job.update!(status: status, updated_at: updated_at)
   end
 
@@ -195,7 +192,6 @@ describe StuckCiJobsWorker do
     end
 
     it 'cancels exclusive leases after worker perform' do
-      expect_to_cancel_exclusive_lease(trace_lease_key, trace_lease_uuid)
       expect_to_cancel_exclusive_lease(worker_lease_key, worker_lease_uuid)
 
       worker.perform